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

[Joel Stanley]

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.

> +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?

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

Cheers,

Joel