| Message ID | 1267640583.4760.0@antares (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Grant Likely |
| Headers | show |
Hi Albrecht, I like this version much better. Comments below... On Wed, Mar 3, 2010 at 11:23 AM, Albrecht Dreß <albrecht.dress@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.dress@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; > >
On Thu, Mar 4, 2010 at 2:56 AM, Albrecht Dreß <albrecht.dress@arcor.de> wrote: >> 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. > > Ouch. I don't have a 512x, but isn't the current code plain wrong then? It uses mpc5xxx_get_bus_frequency() as input for the baud rate calculation, and if the serial code assumes /16 instead of /10, the result must be terribly off. Or did I miss something here? If you are, then I'm missing the same thing. Do you best to keep the 5121 calculation work out to the same value it uses now. We'll ask someone with a 5121 to test it out before I add the patch to my -next branch. g.
--- 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 @@ -144,7 +144,8 @@ struct psc_ops { unsigned char (*read_char)(struct uart_port *port); void (*cw_disable_ints)(struct uart_port *port); void (*cw_restore_ints)(struct uart_port *port); - unsigned long (*getuartclk)(void *p); + void (*set_divisor)(struct uart_port *port, + unsigned int divisor); }; #ifdef CONFIG_PPC_MPC52xx @@ -154,9 +155,6 @@ static void mpc52xx_psc_fifo_init(struct struct mpc52xx_psc __iomem *psc = PSC(port); struct mpc52xx_psc_fifo __iomem *fifo = FIFO_52xx(port); - /* /32 prescaler */ - out_be16(&psc->mpc52xx_psc_clock_select, 0xdd00); - out_8(&fifo->rfcntl, 0x00); out_be16(&fifo->rfalarm, 0x1ff); out_8(&fifo->tfcntl, 0x07); @@ -245,15 +243,55 @@ static void mpc52xx_psc_cw_restore_ints( out_be16(&PSC(port)->mpc52xx_psc_imr, port->read_status_mask); } -/* Search for bus-frequency property in this node or a parent */ -static unsigned long mpc52xx_getuartclk(void *p) +static void mpc52xx_psc_set_divisor(struct uart_port *port, + unsigned int divisor) { - /* - * 5200 UARTs have a / 32 prescaler - * but the generic serial code assumes 16 - * so return ipb freq / 2 - */ - return mpc5xxx_get_bus_frequency(p) / 2; + struct mpc52xx_psc __iomem *psc = PSC(port); + + /* adjust divisor for a /32 prescaler; see note in + * mpc52xx_uart_of_probe() */ + divisor = (divisor + 4) / 8; + 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; + } + + /* prescaler */ + out_be16(&psc->mpc52xx_psc_clock_select, 0xdd00); /* /32 */ + + /* ctr */ + out_8(&psc->ctur, divisor >> 8); + out_8(&psc->ctlr, divisor & 0xff); +} + +static void mpc5200b_psc_set_divisor(struct uart_port *port, + unsigned int divisor) +{ + struct mpc52xx_psc __iomem *psc = PSC(port); + + /* set prescaler; see note in mpc52xx_uart_of_probe() */ + if (divisor > 0xffff) { + divisor = (divisor + 4) / 8; + out_be16(&psc->mpc52xx_psc_clock_select, 0xdd00); /* /32 */ + } else + out_be16(&psc->mpc52xx_psc_clock_select, 0xff00); /* /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; + } + + /* ctr */ + out_8(&psc->ctur, divisor >> 8); + out_8(&psc->ctlr, divisor & 0xff); } static struct psc_ops mpc52xx_psc_ops = { @@ -272,7 +310,26 @@ static struct psc_ops mpc52xx_psc_ops = .read_char = mpc52xx_psc_read_char, .cw_disable_ints = mpc52xx_psc_cw_disable_ints, .cw_restore_ints = mpc52xx_psc_cw_restore_ints, - .getuartclk = mpc52xx_getuartclk, + .set_divisor = mpc52xx_psc_set_divisor, +}; + +static struct psc_ops mpc5200b_psc_ops = { + .fifo_init = mpc52xx_psc_fifo_init, + .raw_rx_rdy = mpc52xx_psc_raw_rx_rdy, + .raw_tx_rdy = mpc52xx_psc_raw_tx_rdy, + .rx_rdy = mpc52xx_psc_rx_rdy, + .tx_rdy = mpc52xx_psc_tx_rdy, + .tx_empty = mpc52xx_psc_tx_empty, + .stop_rx = mpc52xx_psc_stop_rx, + .start_tx = mpc52xx_psc_start_tx, + .stop_tx = mpc52xx_psc_stop_tx, + .rx_clr_irq = mpc52xx_psc_rx_clr_irq, + .tx_clr_irq = mpc52xx_psc_tx_clr_irq, + .write_char = mpc52xx_psc_write_char, + .read_char = mpc52xx_psc_read_char, + .cw_disable_ints = mpc52xx_psc_cw_disable_ints, + .cw_restore_ints = mpc52xx_psc_cw_restore_ints, + .set_divisor = mpc5200b_psc_set_divisor, }; #endif /* CONFIG_MPC52xx */ @@ -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); } 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); 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); 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 + * 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;
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.dress@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.