[PATCH linux dev-4.13 v2 2/2] serial: npcm: add NPCM UART driver

[Tomer Maimon]

Forget to add CC. Jeremy Kerr <jk at ozlabs.org>,
OpenBMC Maillist <openbmc at lists.ozlabs.org>

On 5 February 2018 at 11:05, Tomer Maimon <tmaimon77 at gmail.com> wrote:
> On 5 February 2018 at 08:19, Joel Stanley <joel at jms.id.au> wrote:
>> On Mon, Feb 5, 2018 at 6:24 AM, Tomer Maimon <tmaimon77 at gmail.com> wrote:
>>> On 2 February 2018 at 04:51, Joel Stanley <joel at jms.id.au> wrote:
>>>> On Wed, Jan 24, 2018 at 1:46 AM, Tomer Maimon <tmaimon77 at gmail.com> wrote:
>>>>> Add Nuvoton BMC NPCM UART driver.
>>>>>
>>>>> The NPCM7xx BMC contains four UART blocks and accessory logic.
>>>>>
>>>>> NPCM UART based on 8250 driver.
>>>>
>>>> Nice! Much less code to review and maintain than before :)
>>>>
>>>> I did some more digging, to see if we could add even less code. I'll
>>>> sent a patch to the list, but I need to clarify the divisor
>>>> calculation first.
>>>>
>>> Thanks!
>>>>> +static int npcm_uart_set_baud_rate(struct uart_port *port,
>>>>> +                                     NPCM_UART_BAUDRATE_T baudrate)
>>>>> +{
>>>>> +       struct uart_8250_port *up = up_to_u8250p(port);
>>>>> +       int divisor     = 0;
>>>>> +       int ret         = 0;
>>>>> +       unsigned long flags;
>>>>> +
>>>>> +       divisor = ((int)port->uartclk / ((int)baudrate * 16)) - 2;
>>>>
>>>> The 8250 core uses this calculation:
>>>>
>>>>  divisor = port->uartclk / (baudrate * 16).
>>>>
>>>> Checking the datasheet, this is the calculation that is documented for Poleg:
>>>>
>>>>  divisor = port->uartclk / (baudrate * 16 + 2).
>>>>
>>>> If I change the calculation to this, the UART doesn't work (due to the
>>>> precision of the integer division, there's no change from the 8250
>>>> core code). If I subtract 2 from the number, as you do in your patch,
>>>> it works!
>>>>
>>>> Can you explain what is happening here?
>>
>> Hi Tomer, you missed this question. Can you explain to me what is
>> happening here?
>>
>
> Opps sorry!
>
> I think you have little mistake...
>
> From the data sheet calculation:
>
> baudrate =  port->uartclk / (16* (divisor + 2))
>
> So if we isolate the divisor
>
> divisor = (port->uartclk / (16* baudrate)) - 2
>
> this is why if you subtract 2 from the divisor you get the right divisor.
>
>>>>
>>>>> +
>>>>> +       /* since divisor is rounded down check
>>>>> +        * if it is better when rounded up
>>>>> +        */
>>>>> +       if (((int)port->uartclk / (16 * (divisor + 2)) - (int)baudrate) >
>>>>> +               ((int)baudrate -
>>>>> +                (int)port->uartclk / (16 * ((divisor + 1) + 2))))
>>>>> +               divisor++;
>>>>
>>>> This bit confused me when I tried to understand what it was checking.
>>>> I couldn't find a mention of it in the datasheet either.
>>>>
>>>> Can you explain to me what it is trying to do? Do we need it?
>>> hope i will explain it right...
>>>
>>> we are calculating the nearest round up of the divisor according to
>>> the distance from the real divisor (with point) to the value we get
>>> from the round up and round down. (and it different from using
>>> DIV_ROUND_CLOSEST)
>>> for example:
>>>
>>> If the baud rate is 230,400
>>>
>>> the divisor is 4.510 (it means round up)
>>>
>>> if we doing the calculation above:
>>>
>>> (24Mhz/(16*(4+2))) - 230,400 = 19,600
>>> 230,400 - (24Mhz/(16*(4+3))) = 16,114
>>>
>>> also in here we see round - up, but the distance from the second
>>> (16,114) it better and the 4.510 not represent it correctly because 2
>>> result we get are not that the same.
>>>
>>> so maybe in clock rate of 24Mhz it will be fine but if we will modify
>>> the clock rate the calculation could be wrong with a use of
>>> DIV_ROUND_CLOSEST.
>>>
>>> so we think it is better to use the calculation above and not round up.
>>>
>>> Is it problematic to use it in the modify driver you sent?
>>
>> Thanks for the explanation. As I mention above, I need to understand
>> the first calculation in order to understand this part.
>>
>> It would be easy enough to add this calculation if it was strictly
>> necessary. However, to know if it is required, I first need to
>> understand what is happening.
>>
>> Cheers,
>>
>> Joel
>
> Cheers,
>
> Tomer