Re: [Patch v.2] mpc5200b/uart: improve baud rate calculation (reach high baud rates, better accuracy)

[Grant Likely]

Hi Albrecht,

I like this version much better.  Comments below...

On Wed, Mar 3, 2010 at 11:23 AM, Albrecht Dreß <albrecht.dr...@arcor.de> wrote:
> On the MPC5200B, make very high baud rates (e.g. 3 MBaud) accessible and
> achieve a higher precision for high baud rates in general.  This is done by
> selecting the appropriate prescaler (/4 or /32).  As to keep the code clean,
> the getuartclk method has been dropped, and all calculations are done with a
> "virtual" /4 prescaler for all chips for maximum accuracy.  A new set_divisor
> method scales down the divisor values found by the generic serial code
> appropriately.
>
> Note: only "fsl,mpc5200b-psc-uart" compatible devices benefit from these
> improvements.
>
> Tested on a custom 5200B based board, with up to 3 MBaud, and with both
> "fsl,mpc5200b-psc-uart" and "fsl,mpc5200-psc-uart" devices.
>
> Signed-off-by: Albrecht Dreß <albrecht.dr...@arcor.de>
>
> ---
>
> Changes vs. v.1: include improvements suggested by Wolfram and Grant (thanks a
> lot for your helpful input!!): drop getuartclk method and use the highest
> possible frequency for calculation, use new psc_ops for the 5200b, let the
> set_divisor method do all the dirty work, emit warnings if bad divisor values
> have been selected.
>
>
> --- linux-2.6.33-orig/drivers/serial/mpc52xx_uart.c     2010-02-24 
> 19:52:17.000000000 +0100
> +++ linux-2.6.33/drivers/serial/mpc52xx_uart.c  2010-03-03 10:52:04.000000000 
> +0100
> @@ -388,9 +445,25 @@ static void mpc512x_psc_cw_restore_ints(
>        out_be32(&FIFO_512x(port)->rximr, port->read_status_mask & 0x7f);
>  }
>
> -static unsigned long mpc512x_getuartclk(void *p)
> +static void mpc512x_psc_set_divisor(struct uart_port *port,
> +                                   unsigned int divisor)
>  {
> -       return mpc5xxx_get_bus_frequency(p);
> +       struct mpc52xx_psc __iomem *psc = PSC(port);
> +
> +       /* adjust divisor for a /16 prescaler; see note in
> +        * mpc52xx_uart_of_probe() */
> +       divisor = (divisor + 2) / 4;
> +       if (divisor > 0xffff) {
> +               pr_warning("%s: divisor overflow (%x), use 0xffff\n", 
> __func__,
> +                          divisor);
> +               divisor = 0xffff;
> +       } else if (divisor == 0) {
> +               pr_warning("%s: divisor 0, use 1\n", __func__);
> +               divisor = 1;
> +       }
> +
> +       out_8(&psc->ctur, divisor >> 8);
> +       out_8(&psc->ctlr, divisor & 0xff);

Save yourself some duplicated code here.  The above 14 lines can be
shared between the 512x, 52xx and 5200b versions.  Create yourself an
internal __mpc5xxx_psc_set_divisor() function that is passed the *psc,
the divisor, and the clock select register setting (both the 5200 and
the 5121 have the clock select register).

>  }
>
>  static struct psc_ops mpc512x_psc_ops = {
> @@ -409,7 +482,7 @@ static struct psc_ops mpc512x_psc_ops =
>        .read_char = mpc512x_psc_read_char,
>        .cw_disable_ints = mpc512x_psc_cw_disable_ints,
>        .cw_restore_ints = mpc512x_psc_cw_restore_ints,
> -       .getuartclk = mpc512x_getuartclk,
> +       .set_divisor = mpc512x_psc_set_divisor,
>  };
>  #endif
>
> @@ -564,7 +637,6 @@ mpc52xx_uart_set_termios(struct uart_por
>        struct mpc52xx_psc __iomem *psc = PSC(port);
>        unsigned long flags;
>        unsigned char mr1, mr2;
> -       unsigned short ctr;
>        unsigned int j, baud, quot;
>
>        /* Prepare what we're gonna write */
> @@ -604,7 +676,6 @@ mpc52xx_uart_set_termios(struct uart_por
>
>        baud = uart_get_baud_rate(port, new, old, 0, port->uartclk/16);

I'm probably nitpicking, because I don't know if the io pin will
handle this speed but uartclk/16 is no longer the maximum baudrate if
a /4 prescaler is used.

>        quot = uart_get_divisor(port, baud);
> -       ctr = quot & 0xffff;
>
>        /* Get the lock */
>        spin_lock_irqsave(&port->lock, flags);
> @@ -635,8 +706,7 @@ mpc52xx_uart_set_termios(struct uart_por
>        out_8(&psc->command, MPC52xx_PSC_SEL_MODE_REG_1);
>        out_8(&psc->mode, mr1);
>        out_8(&psc->mode, mr2);
> -       out_8(&psc->ctur, ctr >> 8);
> -       out_8(&psc->ctlr, ctr & 0xff);
> +       psc_ops->set_divisor(port, quot);

Hmmm.  The divisor calculations have some tricky bits to them.  I
would consider changing the set_divisor() function to accept a baud
rate, and modify the set_divisor function to call uart_get_divisor().
That way each set_divisor() can do whatever makes the most sense for
the divisors available to it.  The 5121 for example has both a /10 and
a /32 divisor, plus it can use an external clock.

>
>        if (UART_ENABLE_MS(port, new->c_cflag))
>                mpc52xx_uart_enable_ms(port);
> @@ -1007,7 +1077,8 @@ mpc52xx_console_setup(struct console *co
>                return ret;
>        }
>
> -       uartclk = psc_ops->getuartclk(np);
> +       /* see remarks about the uart clock in mpc52xx_uart_of_probe() */
> +       uartclk = mpc5xxx_get_bus_frequency(np) * 4;
>        if (uartclk == 0) {
>                pr_debug("Could not find uart clock frequency!\n");
>                return -EINVAL;
> @@ -1090,6 +1161,7 @@ static struct uart_driver mpc52xx_uart_d
>
>  static struct of_device_id mpc52xx_uart_of_match[] = {
>  #ifdef CONFIG_PPC_MPC52xx
> +       { .compatible = "fsl,mpc5200b-psc-uart", .data = &mpc5200b_psc_ops, },
>        { .compatible = "fsl,mpc5200-psc-uart", .data = &mpc52xx_psc_ops, },
>        /* binding used by old lite5200 device trees: */
>        { .compatible = "mpc5200-psc-uart", .data = &mpc52xx_psc_ops, },
> @@ -1122,7 +1194,24 @@ mpc52xx_uart_of_probe(struct of_device *
>        pr_debug("Found %s assigned to ttyPSC%x\n",
>                 mpc52xx_uart_nodes[idx]->full_name, idx);
>
> -       uartclk = psc_ops->getuartclk(op->node);
> +       /*
> +        * Note about the uart clock:
> +        * This series of processors use the ipb clock frequency for the clock
> +        * generation scaled down by prescalers and a 16-bit counter register:
> +        * - the 5200 has a /32 prescaler
> +        * - the 5200B has selectable /4 or /32 prescalers (i.e. the counter
> +        *   reg can be viewed as a 19-bit value, of which we can use either
> +        *   the upper or the lower 16 bits - in the latter case the three
> +        *   MSB's must of course be 0)
> +        * - the 512x has a /16 prescaler

According to the user manual, the 5121 has both a /32 and /10
prescaler.  As such, I'd rather see uartclk get set to the raw value
returned from mpc5xxx_get_bus_frequency() and do all the
transformations in the set_divisor() hook.

Also, I looked at the generic code, and while it does assume a /16
prescaler, that is pretty easy to handle at the point of calling the
uart_get_divisor() function.

> +        * The generic serial code assumes a prescaler of /16.  As we want to
> +        * achieve the maximum accuracy possible, we let the generic serial
> +        * code perform all calculations with the /4 prescaler, i.e. we have
> +        * to set the uart clock to ipb freq * 4 here.  The set_divisor 
> methods
> +        * for the different chips are responsible for scaling down the 
> divisor
> +        * value appropriately.
> +        */
> +       uartclk = mpc5xxx_get_bus_frequency(op->node) * 4;
>        if (uartclk == 0) {
>                dev_dbg(&op->dev, "Could not find uart clock frequency!\n");
>                return -EINVAL;
>
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev