Re: [PATCH 5/7] tty: 8250: workaround errata on disabling UART after using DMA

[Peter Hurley <peter-WaGBZJeGNqdsbIuE7sb01tBPR1lH4CV8@public.gmane.org>]

Hi Sekhar,

Patch title should start "serial: 8250_omap: .." since the patch
is specific to the 8250_omap serial driver.

On 07/06/2015 05:47 AM, Sekhar Nori wrote:
> AM335x, AM437x and DRA7x SoCs have an errata due to which UART
> cannot be disabled after it has been used with DMA.
             ^^^^^^
             idled

> OMAP3 has a similar sounding errata which has been worked around
> in a2fc36613ac1af2e9 ("ARM: OMAP3: Use manual idle for UARTs
> because of DMA errata"). But the workaround used there does not
> apply to AM335x, AM437x SoCs.

> These SoCs need a softreset of UART before disabling them.

"The UART module on these SoCs must be soft reset to go idle."
 
> This patch implements that errata workaround. It is expected that
> UART will be used with DMA so no explicit check for DMA usage
> has been added for errata applicability.

This changelog should also:

1. Reference the errata document.
2. Explain why SCR register has to be the context loss canary
   rather than MDR1.

> Signed-off-by: Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>
> ---
>  drivers/tty/serial/8250/8250_omap.c | 55 +++++++++++++++++++++++++++++++++----
>  1 file changed, 49 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index 52566387ec37..af25869d145f 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -33,6 +33,11 @@
>  #define UART_ERRATA_i202_MDR1_ACCESS	(1 << 0)
>  #define OMAP_UART_WER_HAS_TX_WAKEUP	(1 << 1)
>  #define OMAP_DMA_TX_KICK		(1 << 2)
> +/*
> + * See Advisory 21 in AM437x errata SPRZ408B, updated April 2015.
> + * The same errata is applicable to AM335x and DRA7x processors too.
> + */
> +#define UART_ERRATA_CLOCK_DISABLE	(1 << 3)
>  
>  #define OMAP_UART_FCR_RX_TRIG		6
>  #define OMAP_UART_FCR_TX_TRIG		4
> @@ -54,6 +59,12 @@
>  #define OMAP_UART_MVR_MAJ_SHIFT		8
>  #define OMAP_UART_MVR_MIN_MASK		0x3f
>  
> +/* SYSC register bitmasks */
> +#define OMAP_UART_SYSC_SOFTRESET	(1 << 1)
> +
> +/* SYSS register bitmasks */
> +#define OMAP_UART_SYSS_RESETDONE	(1 << 0)
> +
>  #define UART_TI752_TLR_TX	0
>  #define UART_TI752_TLR_RX	4
>  
> @@ -1062,13 +1073,15 @@ static int omap8250_no_handle_irq(struct uart_port *port)
>  	return 0;
>  }
>  
> -static const u8 am3352_habit = OMAP_DMA_TX_KICK;
> +static const u8 am3352_habit = OMAP_DMA_TX_KICK | UART_ERRATA_CLOCK_DISABLE;
> +static const u8 am4372_habit = UART_ERRATA_CLOCK_DISABLE;
>  
>  static const struct of_device_id omap8250_dt_ids[] = {
>  	{ .compatible = "ti,omap2-uart" },
>  	{ .compatible = "ti,omap3-uart" },
>  	{ .compatible = "ti,omap4-uart" },
>  	{ .compatible = "ti,am3352-uart", .data = &am3352_habit, },
> +	{ .compatible = "ti,am4372-uart", .data = &am4372_habit, },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, omap8250_dt_ids);
> @@ -1279,13 +1292,13 @@ static int omap8250_lost_context(struct uart_8250_port *up)
>  {
>  	u32 val;
>  
> -	val = serial_in(up, UART_OMAP_MDR1);
> +	val = serial_in(up, UART_OMAP_SCR);
>  	/*
> -	 * If we lose context, then MDR1 is set to its reset value which is
> -	 * UART_OMAP_MDR1_DISABLE. After set_termios() we set it either to 13x
> -	 * or 16x but never to disable again.
> +	 * If we lose context, then SCR is set to its reset value of zero.
> +	 * After set_termios() we set bit 3 of SCR (TX_EMPTY_CTL_IT) to 1,
> +	 * among other bits, to never set the register back to zero again.
>  	 */
> -	if (val == UART_OMAP_MDR1_DISABLE)
> +	if (!val)
>  		return 1;
>  	return 0;
>  }
> @@ -1307,6 +1320,36 @@ static int omap8250_runtime_suspend(struct device *dev)
>  			return -EBUSY;
>  	}
>  
> +	if (priv->habit & UART_ERRATA_CLOCK_DISABLE) {
> +		int sysc;
> +		int syss;
> +		int timeout = 100;
> +
> +		sysc = serial_in(up, UART_OMAP_SYSC);
> +
> +		/* softreset the UART */
> +		sysc |= OMAP_UART_SYSC_SOFTRESET;
> +		serial_out(up, UART_OMAP_SYSC, sysc);
> +
> +		/* By experiments, 1us enough for reset complete on AM335x */
> +		do {
> +			udelay(1);
> +			syss = serial_in(up, UART_OMAP_SYSS);
> +		} while (--timeout && !(syss & OMAP_UART_SYSS_RESETDONE));


If there is not a omap helper function to reset modules, there should be.
Open-coding the module reset is not appropriate.

> +		if (!timeout) {
> +			dev_err(dev, "timed out waiting for reset done\n");
> +			return -ETIMEDOUT;
> +		}
> +
> +		/*
> +		 * For UART module wake-up to work, MDR1.MODESELECT should
> +		 * not be set to "Disable", so update it.
> +		 */
> +		if (device_may_wakeup(dev))
> +			omap8250_update_mdr1(up, priv);

Should this be unconditional?

Regards,
Peter Hurley

> +	}
> +
>  	if (up->dma && up->dma->rxchan)
>  		omap_8250_rx_dma(up, UART_IIR_RX_TIMEOUT);
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html