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

[Joel Stanley]

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?

>>
>>> +
>>> +       /* 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