| Message ID | 1267467114.2218.0@antares (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Grant Likely |
| Headers | show |
Hi Albrecht, On Mon, Mar 01, 2010 at 07:11:54PM +0100, Albrecht Dreß wrote: > On the MPC5200B, select the baud rate prescaler as /4 by default to make very > high baud rates (e.g. 3 MBaud) accessible and to achieve a higher precision > for high baud rates in general. For baud rates below ~500 Baud, the code will > automatically fall back to the /32 prescaler. The original MPC5200 does only > have a /32 prescaler which is detected only once and stored in a global. A > new chip-dependent method is used to set the divisor. > > Tested on a custom 5200B based board, with up to 3 MBaud. > > Signed-off-by: Albrecht Dreß <albrecht.dress@arcor.de> > > --- > > --- linux-2.6.33/drivers/serial/mpc52xx_uart.c.orig 2010-02-24 19:52:17.000000000 +0100 > +++ linux-2.6.33/drivers/serial/mpc52xx_uart.c 2010-02-26 21:12:51.000000000 +0100 > @@ -144,9 +144,17 @@ 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); > + void (*set_divisor)(struct uart_port *port, > + unsigned int divisor); > unsigned long (*getuartclk)(void *p); > }; > > +/* We need to distinguish between the MPC5200 which has only a /32 prescaler, > + * and the MPC5200B which has a /32 and a /4 prescaler. The global is fine, > + * as the chip can be only either a 5200B or not. */ > +static int is_mpc5200b = -1; > + > + One empty line too much. Maybe we can also get rid of the static later in the process, but first things first. > #ifdef CONFIG_PPC_MPC52xx > #define FIFO_52xx(port) ((struct mpc52xx_psc_fifo __iomem *)(PSC(port)+1)) > static void mpc52xx_psc_fifo_init(struct uart_port *port) > @@ -154,9 +162,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 +250,40 @@ static void mpc52xx_psc_cw_restore_ints( > out_be16(&PSC(port)->mpc52xx_psc_imr, port->read_status_mask); > } > > +static void mpc52xx_psc_set_divisor(struct uart_port *port, > + unsigned int divisor) > +{ > + struct mpc52xx_psc __iomem *psc = PSC(port); > + > + /* prescaler */ > + if (is_mpc5200b != 1) > + out_be16(&psc->mpc52xx_psc_clock_select, 0xdd00); /* /32 */ > + else if (divisor > 0xffff) { > + out_be16(&psc->mpc52xx_psc_clock_select, 0xdd00); /* /32 */ > + divisor = (divisor + 4) / 8; > + } else > + out_be16(&psc->mpc52xx_psc_clock_select, 0xff00); /* /4 */ > + > + /* ctr */ > + divisor &= 0xffff; > + out_8(&psc->ctur, divisor >> 8); > + out_8(&psc->ctlr, divisor & 0xff); > +} > + > /* Search for bus-frequency property in this node or a parent */ > static unsigned long mpc52xx_getuartclk(void *p) > { > /* > - * 5200 UARTs have a / 32 prescaler > - * but the generic serial code assumes 16 > - * so return ipb freq / 2 > + * The 5200 has only /32 prescalers. > + * 5200B UARTs have a /4 or a /32 prescaler. For higher accuracy, we > + * do all calculations using the /4 prescaler for this chip. > + * The generic serial code assumes /16 so return ipb freq / 2 (5200) > + * or ipb freq * 4 (5200B). > */ > - return mpc5xxx_get_bus_frequency(p) / 2; > + if (is_mpc5200b == 1) > + return mpc5xxx_get_bus_frequency(p) * 4; > + else > + return mpc5xxx_get_bus_frequency(p) / 2; Isn't this wrong? You can also have /32 on the 5200B (the fallback). > } > > static struct psc_ops mpc52xx_psc_ops = { > @@ -272,6 +302,7 @@ 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, > + .set_divisor = mpc52xx_psc_set_divisor, > .getuartclk = mpc52xx_getuartclk, > }; > > @@ -388,6 +419,16 @@ static void mpc512x_psc_cw_restore_ints( > out_be32(&FIFO_512x(port)->rximr, port->read_status_mask & 0x7f); > } > > +static void mpc512x_psc_set_divisor(struct uart_port *port, > + unsigned int divisor) > +{ > + struct mpc52xx_psc __iomem *psc = PSC(port); > + > + divisor &= 0xffff; > + out_8(&psc->ctur, divisor >> 8); > + out_8(&psc->ctlr, divisor & 0xff); > +} > + > static unsigned long mpc512x_getuartclk(void *p) > { > return mpc5xxx_get_bus_frequency(p); > @@ -409,6 +450,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, > + .set_divisor = mpc512x_psc_set_divisor, > .getuartclk = mpc512x_getuartclk, > }; > #endif > @@ -564,7 +606,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 +645,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 +675,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); > @@ -1113,6 +1152,19 @@ mpc52xx_uart_of_probe(struct of_device * > > dev_dbg(&op->dev, "mpc52xx_uart_probe(op=%p, match=%p)\n", op, match); > > + /* Check only once if we are running on a mpc5200b or not */ > + if (is_mpc5200b == -1) { > + struct device_node *np; > + > + np = of_find_compatible_node(NULL, NULL, "fsl,mpc5200b-immr"); This should be handled using a new compatible-entry "fsl,mpc5200b-psc-uart". > + if (np) { > + is_mpc5200b = 1; > + dev_dbg(&op->dev, "mpc5200b: using /4 prescaler\n"); Does this message respect the fallback case? You could also have a set_divisor-function for 5200 and 5200B and set it here in the function struct (one reason less for the static ;)) > + of_node_put(np); > + } else > + is_mpc5200b = 0; > + } > + > /* Check validity & presence */ > for (idx = 0; idx < MPC52xx_PSC_MAXNUM; idx++) > if (mpc52xx_uart_nodes[idx] == op->node) > > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev
Hi Albrecht, Thanks for this work, comment below... On Mon, Mar 1, 2010 at 11:11 AM, Albrecht Dreß <albrecht.dress@arcor.de> wrote: > On the MPC5200B, select the baud rate prescaler as /4 by default to make very > high baud rates (e.g. 3 MBaud) accessible and to achieve a higher precision > for high baud rates in general. For baud rates below ~500 Baud, the code will > automatically fall back to the /32 prescaler. The original MPC5200 does only > have a /32 prescaler which is detected only once and stored in a global. A > new chip-dependent method is used to set the divisor. > > Tested on a custom 5200B based board, with up to 3 MBaud. > > Signed-off-by: Albrecht Dreß <albrecht.dress@arcor.de> > > --- > > --- linux-2.6.33/drivers/serial/mpc52xx_uart.c.orig 2010-02-24 19:52:17.000000000 +0100 > +++ linux-2.6.33/drivers/serial/mpc52xx_uart.c 2010-02-26 21:12:51.000000000 +0100 > /* Search for bus-frequency property in this node or a parent */ > static unsigned long mpc52xx_getuartclk(void *p) > { > /* > - * 5200 UARTs have a / 32 prescaler > - * but the generic serial code assumes 16 > - * so return ipb freq / 2 > + * The 5200 has only /32 prescalers. > + * 5200B UARTs have a /4 or a /32 prescaler. For higher accuracy, we > + * do all calculations using the /4 prescaler for this chip. > + * The generic serial code assumes /16 so return ipb freq / 2 (5200) > + * or ipb freq * 4 (5200B). > */ > - return mpc5xxx_get_bus_frequency(p) / 2; > + if (is_mpc5200b == 1) > + return mpc5xxx_get_bus_frequency(p) * 4; > + else > + return mpc5xxx_get_bus_frequency(p) / 2; > } Remove this function entirely and the associated .getuartclk() hook from the psc_ops structure. Callers can just call mpc5xxx_get_bus_frequency() directly. mpc5121 already just passes back the return value unmodified, and current mpc52xx code uses a /2, but that would be eliminated if the new set_divisor hook took that into account. That way all the chip-specific clock setup calculation is consolidated into a single function. I like patches that make things simpler.
--- linux-2.6.33/drivers/serial/mpc52xx_uart.c.orig 2010-02-24 19:52:17.000000000 +0100 +++ linux-2.6.33/drivers/serial/mpc52xx_uart.c 2010-02-26 21:12:51.000000000 +0100 @@ -144,9 +144,17 @@ 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); + void (*set_divisor)(struct uart_port *port, + unsigned int divisor); unsigned long (*getuartclk)(void *p); }; +/* We need to distinguish between the MPC5200 which has only a /32 prescaler, + * and the MPC5200B which has a /32 and a /4 prescaler. The global is fine, + * as the chip can be only either a 5200B or not. */ +static int is_mpc5200b = -1; + + #ifdef CONFIG_PPC_MPC52xx #define FIFO_52xx(port) ((struct mpc52xx_psc_fifo __iomem *)(PSC(port)+1)) static void mpc52xx_psc_fifo_init(struct uart_port *port) @@ -154,9 +162,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 +250,40 @@ static void mpc52xx_psc_cw_restore_ints( out_be16(&PSC(port)->mpc52xx_psc_imr, port->read_status_mask); } +static void mpc52xx_psc_set_divisor(struct uart_port *port, + unsigned int divisor) +{ + struct mpc52xx_psc __iomem *psc = PSC(port); + + /* prescaler */ + if (is_mpc5200b != 1) + out_be16(&psc->mpc52xx_psc_clock_select, 0xdd00); /* /32 */ + else if (divisor > 0xffff) { + out_be16(&psc->mpc52xx_psc_clock_select, 0xdd00); /* /32 */ + divisor = (divisor + 4) / 8; + } else + out_be16(&psc->mpc52xx_psc_clock_select, 0xff00); /* /4 */ + + /* ctr */ + divisor &= 0xffff; + out_8(&psc->ctur, divisor >> 8); + out_8(&psc->ctlr, divisor & 0xff); +} + /* Search for bus-frequency property in this node or a parent */ static unsigned long mpc52xx_getuartclk(void *p) { /* - * 5200 UARTs have a / 32 prescaler - * but the generic serial code assumes 16 - * so return ipb freq / 2 + * The 5200 has only /32 prescalers. + * 5200B UARTs have a /4 or a /32 prescaler. For higher accuracy, we + * do all calculations using the /4 prescaler for this chip. + * The generic serial code assumes /16 so return ipb freq / 2 (5200) + * or ipb freq * 4 (5200B). */ - return mpc5xxx_get_bus_frequency(p) / 2; + if (is_mpc5200b == 1) + return mpc5xxx_get_bus_frequency(p) * 4; + else + return mpc5xxx_get_bus_frequency(p) / 2; } static struct psc_ops mpc52xx_psc_ops = { @@ -272,6 +302,7 @@ 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, + .set_divisor = mpc52xx_psc_set_divisor, .getuartclk = mpc52xx_getuartclk, }; @@ -388,6 +419,16 @@ static void mpc512x_psc_cw_restore_ints( out_be32(&FIFO_512x(port)->rximr, port->read_status_mask & 0x7f); } +static void mpc512x_psc_set_divisor(struct uart_port *port, + unsigned int divisor) +{ + struct mpc52xx_psc __iomem *psc = PSC(port); + + divisor &= 0xffff; + out_8(&psc->ctur, divisor >> 8); + out_8(&psc->ctlr, divisor & 0xff); +} + static unsigned long mpc512x_getuartclk(void *p) { return mpc5xxx_get_bus_frequency(p); @@ -409,6 +450,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, + .set_divisor = mpc512x_psc_set_divisor, .getuartclk = mpc512x_getuartclk, }; #endif @@ -564,7 +606,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 +645,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 +675,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); @@ -1113,6 +1152,19 @@ mpc52xx_uart_of_probe(struct of_device * dev_dbg(&op->dev, "mpc52xx_uart_probe(op=%p, match=%p)\n", op, match); + /* Check only once if we are running on a mpc5200b or not */ + if (is_mpc5200b == -1) { + struct device_node *np; + + np = of_find_compatible_node(NULL, NULL, "fsl,mpc5200b-immr"); + if (np) { + is_mpc5200b = 1; + dev_dbg(&op->dev, "mpc5200b: using /4 prescaler\n"); + of_node_put(np); + } else + is_mpc5200b = 0; + } + /* Check validity & presence */ for (idx = 0; idx < MPC52xx_PSC_MAXNUM; idx++) if (mpc52xx_uart_nodes[idx] == op->node)
On the MPC5200B, select the baud rate prescaler as /4 by default to make very high baud rates (e.g. 3 MBaud) accessible and to achieve a higher precision for high baud rates in general. For baud rates below ~500 Baud, the code will automatically fall back to the /32 prescaler. The original MPC5200 does only have a /32 prescaler which is detected only once and stored in a global. A new chip-dependent method is used to set the divisor. Tested on a custom 5200B based board, with up to 3 MBaud. Signed-off-by: Albrecht Dreß <albrecht.dress@arcor.de> ---