Re: [PATCH V2] workaround for mpc52xx erratum #364 (serial may not be reset in break state)

[Wolfram Sang <w.sang@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3891 bytes --]

Hello Rene,

I haven't actually applied the patch, just a few comments from a
glimpse:

> +/* macro with helper macros to safely reset rx which mustn't be done in break state.
> + * This is a workaround for processor bug #364 described in MPC5200 (L25R) Errata.

Minor nit: Kernel CodingStyle suggests long comments like this

/*
 * comment starts here
 * ...
 */

Major nit: Please add to the comment that this bug is still present on
the MPC5200B, although it is not in its errata sheet. Thils will avoid
later confusion. (Out of interest, did you contact Freescale about this
bug?)

> + *
> + * The workaround resets the baudrate to 4800, waits for a stable state and resets break state repeatedly if necessary.
> + * Optionally it can release the lock while waiting.
> + * 1 character at 4800 baud takes 2ms, 3ms should be enough for 1 character at higher speed and 1 char at lowest
> + * works only with longer delays

Did I get it right that there are cases where the workaround won't work?

> + */
> +
> +static void mpc52xx_uart_reset_rx(struct uart_port *port, int unlock, unsigned long flags)

'bool unlock'?

> +{
> +	struct mpc52xx_psc __iomem *psc = PSC(port);
> +#ifdef CONFIG_PPC_MPC52xx
> +	out_8(&psc->ctur,0x01);
> +	out_8(&psc->ctlr,0xae);

Those are magic values. If you can't build them using defined
constants, at least document them, please.

> +	do {
> +		out_8(&psc->command,MPC52xx_PSC_RST_ERR_STAT);
> +		if (unlock) {
> +			disable_irq(port->irq);
> +			spin_unlock_irqrestore(&port->lock, flags);
> +		}
> +		mdelay(10);
> +		if (unlock) {
> +			spin_lock_irqsave(&port->lock, flags);
> +			enable_irq(port->irq);
> +		}
> +	} while ((in_be16(&psc->mpc52xx_psc_status)) & MPC52xx_PSC_SR_RB);
> +#endif
> +	out_8(&psc->command,MPC52xx_PSC_RST_RX);
> +}
> +
>  static int
>  mpc52xx_uart_startup(struct uart_port *port)
>  {
> @@ -510,7 +541,7 @@ mpc52xx_uart_startup(struct uart_port *port)
>  		return ret;
>  
>  	/* Reset/activate the port, clear and enable interrupts */
> -	out_8(&psc->command, MPC52xx_PSC_RST_RX);
> +	mpc52xx_uart_reset_rx(port, false, 0);
>  	out_8(&psc->command, MPC52xx_PSC_RST_TX);
>  
>  	out_be32(&psc->sicr, 0);	/* UART mode DCD ignored */
> @@ -529,7 +560,7 @@ mpc52xx_uart_shutdown(struct uart_port *port)
>  	struct mpc52xx_psc __iomem *psc = PSC(port);
>  
>  	/* Shut down the port.  Leave TX active if on a console port */
> -	out_8(&psc->command, MPC52xx_PSC_RST_RX);
> +	mpc52xx_uart_reset_rx(port, false, 0);
>  	if (!uart_console(port))
>  		out_8(&psc->command, MPC52xx_PSC_RST_TX);
>  
> @@ -588,9 +619,6 @@ mpc52xx_uart_set_termios(struct uart_port *port, struct ktermios *new,
>  	/* Get the lock */
>  	spin_lock_irqsave(&port->lock, flags);
>  
> -	/* Update the per-port timeout */
> -	uart_update_timeout(port, new->c_cflag, baud);
> -
>  	/* Do our best to flush TX & RX, so we don't loose anything */
>  	/* But we don't wait indefinitly ! */
>  	j = 5000000;	/* Maximum wait */
> @@ -607,9 +635,12 @@ mpc52xx_uart_set_termios(struct uart_port *port, struct ktermios *new,
>  			"Some chars may have been lost.\n");
>  
>  	/* Reset the TX & RX */
> -	out_8(&psc->command, MPC52xx_PSC_RST_RX);
> +	mpc52xx_uart_reset_rx(port, true, flags);
>  	out_8(&psc->command, MPC52xx_PSC_RST_TX);
>  
> +	/* Update the per-port timeout */
> +	uart_update_timeout(port, new->c_cflag, baud);
> +
>  	/* Send new mode settings */
>  	out_8(&psc->command, MPC52xx_PSC_SEL_MODE_REG_1);
>  	out_8(&psc->mode, mr1);

> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Thanks!

   Wolfram Sang

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]