Re: [PATCH] serial: 8250_dw: Handle no_console_suspend when uart loses state

[Andy Shevchenko]

On Fri, 2014-11-14 at 16:32 -0800, Doug Anderson wrote:
> In certain suspend modes on certain boards the 8250 UART may lose
> state when the device goes to suspend.  If we're using
> no_console_suspend this can cause lots of problems during resume.
> 
> Let's cache the basic UART config registers at suspend time and if we
> notice that the UART loses state (by looking at a canary stored in the
> scratch register) we'll restore it.
> 

Few comments below.

> Signed-off-by: Doug Anderson <dianders@xxxxxxxxxxxx>
> ---
> Note that I don't have a board that properly suspends and resumes from
> the deepest sleep state that runs atop mainline, so this is tested on
> a local tree that is based on 3.14 with backports (specifically on
> rk3288-pinky).  It is compile tested atop mainline.
> 
>  drivers/tty/serial/8250/8250_dw.c | 67 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index 0bfdccc..444ff540 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -38,6 +38,9 @@
>  #define DW_UART_CPR	0xf4 /* Component Parameter Register */
>  #define DW_UART_UCV	0xf8 /* UART Component Version */
>  
> +/* We'll place this canary in SCR while suspending; if gone we've lost state */
> +#define DW_UART_SCR_STATE		0x22
> +
>  /* Component Parameter Register bits */
>  #define DW_UART_CPR_ABP_DATA_WIDTH	(3 << 0)
>  #define DW_UART_CPR_AFCE_MODE		(1 << 4)
> @@ -63,6 +66,13 @@ struct dw8250_data {
>  	struct clk		*pclk;
>  	struct reset_control	*rst;
>  	struct uart_8250_dma	dma;
> +
> +	int			suspending;

> +	int			saved_lcr;

Why not u32?

> +	int			saved_dll;
> +	int			saved_dlm;
> +	int			saved_ier;
> +	int			saved_fcr;

Could it be separate structure (struct reg_context, for example)?

>  };
>  
>  #define BYT_PRV_CLK			0x800
> @@ -92,10 +102,35 @@ static void dw8250_force_idle(struct uart_port *p)
>  	(void)p->serial_in(p, UART_RX);
>  }
>  
> +static void dw8250_serial_restore(struct uart_port *p)
> +{
> +	struct dw8250_data *data = p->private_data;
> +	struct uart_8250_port *port8250 = serial8250_get_port(data->line);
> +
> +	data->suspending = 0;
> +
> +	serial_out(port8250, UART_LCR, data->saved_lcr | UART_LCR_DLAB);
> +	serial_out(port8250, UART_DLL, data->saved_dll);
> +	serial_out(port8250, UART_DLM, data->saved_dlm);
> +	serial_out(port8250, UART_LCR, data->saved_lcr);
> +
> +	serial_out(port8250, UART_IER, data->saved_ier);
> +	serial_out(port8250, UART_FCR, data->saved_fcr);
> +	serial_out(port8250, UART_MCR, data->last_mcr);
> +}
> +
>  static void dw8250_serial_out(struct uart_port *p, int offset, int value)
>  {
>  	struct dw8250_data *d = p->private_data;
>  
> +	/*
> +	 * If we started suspending and we see SCR went back to 0, assume we've
> +	 * suspended and resumed and lost state.  Restore it now.
> +	 */
> +	if (d->suspending &&
> +	    readb(p->membase + (UART_SCR << p->regshift)) != DW_UART_SCR_STATE)
> +		dw8250_serial_restore(p);
> +
>  	if (offset == UART_MCR)
>  		d->last_mcr = value;
>  
> @@ -133,6 +168,14 @@ static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
>  {
>  	struct dw8250_data *d = p->private_data;
>  
> +	/*
> +	 * If we started suspending and we see SCR went back to 0, assume we've
> +	 * suspended and resumed and lost state.  Restore it now.
> +	 */
> +	if (d->suspending &&
> +	    readb(p->membase + (UART_SCR << p->regshift)) != DW_UART_SCR_STATE)
> +		dw8250_serial_restore(p);
> +
>  	if (offset == UART_MCR)
>  		d->last_mcr = value;
>  
> @@ -487,9 +530,29 @@ static int dw8250_remove(struct platform_device *pdev)
>  static int dw8250_suspend(struct device *dev)
>  {
>  	struct dw8250_data *data = dev_get_drvdata(dev);
> +	struct uart_8250_port *port8250 = serial8250_get_port(data->line);
> +	struct uart_port *port = &port8250->port;
>  
>  	serial8250_suspend_port(data->line);
>  
> +	/* We only deal with ports that were left on (no_console_suspend) */
> +	if (port->suspended)
> +		return 0;
> +
> +	/* We'll save our registers in case we lose state in suspend */
> +	data->saved_fcr = serial_in(port8250, UART_FCR);
> +	data->saved_ier = serial_in(port8250, UART_IER);
> +	data->saved_lcr = serial_in(port8250, UART_LCR);
> +
> +	serial_out(port8250, UART_LCR, data->saved_lcr | UART_LCR_DLAB);
> +	data->saved_dlm = serial_in(port8250, UART_DLM);
> +	data->saved_dll = serial_in(port8250, UART_DLL);
> +	serial_out(port8250, UART_LCR, data->saved_lcr);
> +
> +	/* Put a special canary in the scratch so we tell when state is lost */
> +	serial_out(port8250, UART_SCR, DW_UART_SCR_STATE);
> +	data->suspending = 1;
> +

Could it be dw8250_serial_save() in analogue with _restore()?

>  	return 0;
>  }
>  
> @@ -497,6 +560,10 @@ static int dw8250_resume(struct device *dev)
>  {
>  	struct dw8250_data *data = dev_get_drvdata(dev);
>  
> +	/* We never lost state; stop checking for canary */
> +	if (data->suspending)
> +		data->suspending = 0;
> +
>  	serial8250_resume_port(data->line);
>  
>  	return 0;


-- 
Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/