Hi Albrecht, I like this version much better. Comments below...
On Wed, Mar 3, 2010 at 11:23 AM, Albrecht Dreß <[email protected]> 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ß <[email protected]> > > --- > > 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 [email protected] https://lists.ozlabs.org/listinfo/linuxppc-dev
