serial: 8250_dw: Handle no_console_suspend when uart loses state
diff mbox

Message ID 1416011556-23422-1-git-send-email-dianders@chromium.org
State New, archived
Headers show

Commit Message

Doug Anderson Nov. 15, 2014, 12:32 a.m. UTC
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.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
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(+)

Comments

Loic Poulain Nov. 17, 2014, 8:56 a.m. UTC | #1
> 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.

If I'm not wrong, it's already done in acpi_lpss suspend/resume (for 
8250_dw).
Commit c78b0830667a7e7c1f0ca65b76b33166a84806b3.

Regards,
Loic
Heiko Stübner Nov. 17, 2014, 9:20 a.m. UTC | #2
Am Montag, 17. November 2014, 09:56:08 schrieb Loic Poulain:
> > 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.
> 
> If I'm not wrong, it's already done in acpi_lpss suspend/resume (for
> 8250_dw).
> Commit c78b0830667a7e7c1f0ca65b76b33166a84806b3.

It looks like it, but of course only takes into account devices probed via 
acpi, missing the devices probed via devicetree.

So it looks like we need Doug's solution somehow, question would be if it 
should be excluded on acpi platforms.


Heiko
Heikki Krogerus Nov. 28, 2014, 12:09 p.m. UTC | #3
On Mon, Nov 17, 2014 at 09:56:08AM +0100, Loic Poulain 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.
> 
> If I'm not wrong, it's already done in acpi_lpss suspend/resume (for
> 8250_dw).
> Commit c78b0830667a7e7c1f0ca65b76b33166a84806b3.

FYI, we are not saving any UART specific registers in acpi_lpss.c nor
any other bus controller's registers that are part of the LPSS block.
We are saving/restoring only the context of lpss specific register
there.

Cheers,
Andy Shevchenko Nov. 28, 2014, 12:30 p.m. UTC | #4
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@chromium.org>
> ---
> 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;

Patch
diff mbox

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;
+	int			saved_dll;
+	int			saved_dlm;
+	int			saved_ier;
+	int			saved_fcr;
 };
 
 #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;
+
 	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;