Patchwork mpc5200b: improve baud rate calculation (reach high baud rates, better accuracy)

login
register
mail settings
Submitter Albrecht Dreß
Date March 1, 2010, 6:11 p.m.
Message ID <1267467114.2218.0@antares>
Download mbox | patch
Permalink /patch/46580/
State Superseded
Delegated to: Grant Likely
Headers show

Comments

Albrecht Dreß - March 1, 2010, 6:11 p.m.
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>

---
Wolfram Sang - March 2, 2010, 12:32 a.m.
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
Grant Likely - March 2, 2010, 8:22 p.m.
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.

Patch

--- 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)