[PATCH v3 1/1] Serial: imx: add dev_pm_ops to support suspend to ram/disk

[PATCH v3 1/1] Serial: imx: add dev_pm_ops to support suspend to ram/disk

[Shenwei Wang]

When system goes into low power states like SUSPEND_MEM and
HIBERNATION, the hardware IP block may be powered off to reduce
the power consumption. This power down may cause problems on
some imx platforms, because the hardware settings are reset to
its power on default values which may differ from the ones when
it power off. This patch added the dev_pm_ops and implemented
two callbacks: suspend_noirq and resume_noirq, which will save
the necessory hardware parameters right before power down and
recover them before system uses the hardware.

Because added the dev_pm_ops, the old suspend/resume callbacks
under platform_driver will not be called any more. Changed their
prototypes and moved those two callbacks into dev_pm_ops too.

Signed-off-by: Shenwei Wang <shenwei.wang@freescale.com>
---
Change log:

PATCH v3
	After added the dev_pm_ops, the old suspend/resume callbacks
	under platform_driver will not be called. Changed their
	prototypes and moved them into dev_pm_ops too.

PATCH v2
	Change pr_debug to dev_dbg per GregK's review feedback.

 drivers/tty/serial/imx.c | 120 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 88 insertions(+), 32 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 2c90dc3..10dece3 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -216,6 +216,7 @@ struct imx_port {
 	unsigned int		tx_bytes;
 	unsigned int		dma_tx_nents;
 	wait_queue_head_t	dma_wait;
+	unsigned int            saved_reg[10];
 };

 struct imx_port_ucrs {
@@ -1811,36 +1812,6 @@ static struct uart_driver imx_reg = {
 	.cons           = IMX_CONSOLE,
 };

-static int serial_imx_suspend(struct platform_device *dev, pm_message_t state)
-{
-	struct imx_port *sport = platform_get_drvdata(dev);
-	unsigned int val;
-
-	/* enable wakeup from i.MX UART */
-	val = readl(sport->port.membase + UCR3);
-	val |= UCR3_AWAKEN;
-	writel(val, sport->port.membase + UCR3);
-
-	uart_suspend_port(&imx_reg, &sport->port);
-
-	return 0;
-}
-
-static int serial_imx_resume(struct platform_device *dev)
-{
-	struct imx_port *sport = platform_get_drvdata(dev);
-	unsigned int val;
-
-	/* disable wakeup from i.MX UART */
-	val = readl(sport->port.membase + UCR3);
-	val &= ~UCR3_AWAKEN;
-	writel(val, sport->port.membase + UCR3);
-
-	uart_resume_port(&imx_reg, &sport->port);
-
-	return 0;
-}
-
 #ifdef CONFIG_OF
 /*
  * This function returns 1 iff pdev isn't a device instatiated by dt, 0 iff it
@@ -1992,16 +1963,101 @@ static int serial_imx_remove(struct platform_device *pdev)
 	return uart_remove_one_port(&imx_reg, &sport->port);
 }

+static int imx_serial_port_suspend_noirq(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct imx_port *sport = platform_get_drvdata(pdev);
+
+	/* Save necessary regs */
+	clk_enable(sport->clk_ipg);
+	sport->saved_reg[0] = readl(sport->port.membase + UCR1);
+	sport->saved_reg[1] = readl(sport->port.membase + UCR2);
+	sport->saved_reg[2] = readl(sport->port.membase + UCR3);
+	sport->saved_reg[3] = readl(sport->port.membase + UCR4);
+	sport->saved_reg[4] = readl(sport->port.membase + UFCR);
+	sport->saved_reg[5] = readl(sport->port.membase + UESC);
+	sport->saved_reg[6] = readl(sport->port.membase + UTIM);
+	sport->saved_reg[7] = readl(sport->port.membase + UBIR);
+	sport->saved_reg[8] = readl(sport->port.membase + UBMR);
+	sport->saved_reg[9] = readl(sport->port.membase + IMX21_UTS);
+	clk_disable(sport->clk_ipg);
+
+	dev_dbg(dev, "0x%p (%d)\r\n", sport->port.membase, sport->port.line);
+
+	return 0;
+}
+
+static int imx_serial_port_resume_noirq(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct imx_port *sport = platform_get_drvdata(pdev);
+
+	clk_enable(sport->clk_ipg);
+	writel(sport->saved_reg[4], sport->port.membase + UFCR);
+	writel(sport->saved_reg[5], sport->port.membase + UESC);
+	writel(sport->saved_reg[6], sport->port.membase + UTIM);
+	writel(sport->saved_reg[7], sport->port.membase + UBIR);
+	writel(sport->saved_reg[8], sport->port.membase + UBMR);
+	writel(sport->saved_reg[9], sport->port.membase + IMX21_UTS);
+	writel(sport->saved_reg[0], sport->port.membase + UCR1);
+	writel(sport->saved_reg[1] | 0x1, sport->port.membase + UCR2);
+	writel(sport->saved_reg[2], sport->port.membase + UCR3);
+	writel(sport->saved_reg[3], sport->port.membase + UCR4);
+	clk_disable(sport->clk_ipg);
+
+	dev_dbg(dev, "0x%p (%d)\r\n", sport->port.membase, sport->port.line);
+
+	return 0;
+}
+
+static int imx_serial_port_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct imx_port *sport = platform_get_drvdata(pdev);
+	unsigned int val;
+
+	/* enable wakeup from i.MX UART */
+	val = readl(sport->port.membase + UCR3);
+	val |= UCR3_AWAKEN;
+	writel(val, sport->port.membase + UCR3);
+
+	uart_suspend_port(&imx_reg, &sport->port);
+
+	return 0;
+}
+
+static int imx_serial_port_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct imx_port *sport = platform_get_drvdata(pdev);
+	unsigned int val;
+
+	/* disable wakeup from i.MX UART */
+	val = readl(sport->port.membase + UCR3);
+	val &= ~UCR3_AWAKEN;
+	writel(val, sport->port.membase + UCR3);
+
+	uart_resume_port(&imx_reg, &sport->port);
+
+	return 0;
+}
+
+static const struct dev_pm_ops imx_serial_port_pm_ops = {
+	.suspend_noirq = imx_serial_port_suspend_noirq,
+	.resume_noirq = imx_serial_port_resume_noirq,
+	.suspend = imx_serial_port_suspend,
+	.resume = imx_serial_port_resume,
+};
+
 static struct platform_driver serial_imx_driver = {
 	.probe		= serial_imx_probe,
 	.remove		= serial_imx_remove,

-	.suspend	= serial_imx_suspend,
-	.resume		= serial_imx_resume,
 	.id_table	= imx_uart_devtype,
 	.driver		= {
 		.name	= "imx-uart",
 		.of_match_table = imx_uart_dt_ids,
+		.pm	= &imx_serial_port_pm_ops,
 	},
 };

--
2.5.0.rc2

[PATCH v3 1/1] Serial: imx: add dev_pm_ops to support suspend to ram/disk

[Fabio Estevam]

On Wed, Jul 29, 2015 at 5:58 PM, Shenwei Wang
<shenwei.wang@freescale.com> wrote:

> +static int imx_serial_port_suspend_noirq(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct imx_port *sport = platform_get_drvdata(pdev);
> +
> +       /* Save necessary regs */
> +       clk_enable(sport->clk_ipg);

clk_enable() may fail, so you should check its return value.

> +       sport->saved_reg[0] = readl(sport->port.membase + UCR1);
> +       sport->saved_reg[1] = readl(sport->port.membase + UCR2);
> +       sport->saved_reg[2] = readl(sport->port.membase + UCR3);
> +       sport->saved_reg[3] = readl(sport->port.membase + UCR4);
> +       sport->saved_reg[4] = readl(sport->port.membase + UFCR);
> +       sport->saved_reg[5] = readl(sport->port.membase + UESC);
> +       sport->saved_reg[6] = readl(sport->port.membase + UTIM);
> +       sport->saved_reg[7] = readl(sport->port.membase + UBIR);
> +       sport->saved_reg[8] = readl(sport->port.membase + UBMR);
> +       sport->saved_reg[9] = readl(sport->port.membase + IMX21_UTS);
> +       clk_disable(sport->clk_ipg);
> +
> +       dev_dbg(dev, "0x%p (%d)\r\n", sport->port.membase, sport->port.line);
> +
> +       return 0;
> +}
> +
> +static int imx_serial_port_resume_noirq(struct device *dev)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct imx_port *sport = platform_get_drvdata(pdev);
> +
> +       clk_enable(sport->clk_ipg);

Same here.

> +       writel(sport->saved_reg[4], sport->port.membase + UFCR);
> +       writel(sport->saved_reg[5], sport->port.membase + UESC);
> +       writel(sport->saved_reg[6], sport->port.membase + UTIM);
> +       writel(sport->saved_reg[7], sport->port.membase + UBIR);
> +       writel(sport->saved_reg[8], sport->port.membase + UBMR);
> +       writel(sport->saved_reg[9], sport->port.membase + IMX21_UTS);
> +       writel(sport->saved_reg[0], sport->port.membase + UCR1);
> +       writel(sport->saved_reg[1] | 0x1, sport->port.membase + UCR2);

Why this magic '| 0x1'? Can't you use a define instead?

[PATCH v3 1/1] Serial: imx: add dev_pm_ops to support suspend to ram/disk

[Shenwei Wang]

> -----Original Message-----
> From: Fabio Estevam [mailto:festevam at gmail.com]
> Sent: 2015?7?29? 16:21
> To: Wang Shenwei-B38339
> Cc: Greg Kroah-Hartman; linux-arm-kernel at lists.infradead.org;
> linux-serial at vger.kernel.org
> Subject: Re: [PATCH v3 1/1] Serial: imx: add dev_pm_ops to support suspend to
> ram/disk
> 
> On Wed, Jul 29, 2015 at 5:58 PM, Shenwei Wang <shenwei.wang@freescale.com>
> wrote:
> 
> > +static int imx_serial_port_suspend_noirq(struct device *dev) {
> > +       struct platform_device *pdev = to_platform_device(dev);
> > +       struct imx_port *sport = platform_get_drvdata(pdev);
> > +
> > +       /* Save necessary regs */
> > +       clk_enable(sport->clk_ipg);
> 
> clk_enable() may fail, so you should check its return value.

The check seems a little superfluous because the failure condition for clk_enable is 
(clk == NULL || IS_ERR(clk))
Once this function is called which means the initialization of the driver is successful.

> > +       sport->saved_reg[0] = readl(sport->port.membase + UCR1);
> > +       sport->saved_reg[1] = readl(sport->port.membase + UCR2);
> > +       sport->saved_reg[2] = readl(sport->port.membase + UCR3);
> > +       sport->saved_reg[3] = readl(sport->port.membase + UCR4);
> > +       sport->saved_reg[4] = readl(sport->port.membase + UFCR);
> > +       sport->saved_reg[5] = readl(sport->port.membase + UESC);
> > +       sport->saved_reg[6] = readl(sport->port.membase + UTIM);
> > +       sport->saved_reg[7] = readl(sport->port.membase + UBIR);
> > +       sport->saved_reg[8] = readl(sport->port.membase + UBMR);
> > +       sport->saved_reg[9] = readl(sport->port.membase + IMX21_UTS);
> > +       clk_disable(sport->clk_ipg);
> > +
> > +       dev_dbg(dev, "0x%p (%d)\r\n", sport->port.membase,
> > + sport->port.line);
> > +
> > +       return 0;
> > +}
> > +
> > +static int imx_serial_port_resume_noirq(struct device *dev) {
> > +       struct platform_device *pdev = to_platform_device(dev);
> > +       struct imx_port *sport = platform_get_drvdata(pdev);
> > +
> > +       clk_enable(sport->clk_ipg);
> 
> Same here.
> 
> > +       writel(sport->saved_reg[4], sport->port.membase + UFCR);
> > +       writel(sport->saved_reg[5], sport->port.membase + UESC);
> > +       writel(sport->saved_reg[6], sport->port.membase + UTIM);
> > +       writel(sport->saved_reg[7], sport->port.membase + UBIR);
> > +       writel(sport->saved_reg[8], sport->port.membase + UBMR);
> > +       writel(sport->saved_reg[9], sport->port.membase + IMX21_UTS);
> > +       writel(sport->saved_reg[0], sport->port.membase + UCR1);
> > +       writel(sport->saved_reg[1] | 0x1, sport->port.membase + UCR2);
> 
> Why this magic '| 0x1'? Can't you use a define instead?

Yes, should be a macro.

Thanks,
Shenwei

[PATCH v3 1/1] Serial: imx: add dev_pm_ops to support suspend to ram/disk

[Fabio Estevam]

On Wed, Jul 29, 2015 at 6:41 PM, Shenwei Wang
<Shenwei.Wang@freescale.com> wrote:

>> clk_enable() may fail, so you should check its return value.
>
> The check seems a little superfluous because the failure condition for clk_enable is
> (clk == NULL || IS_ERR(clk))
> Once this function is called which means the initialization of the driver is successful.

Not superfluos. You can't assume that clk_enable() will always be
successful. Better check its return value.

[PATCH v3 1/1] Serial: imx: add dev_pm_ops to support suspend to ram/disk

[Shenwei Wang]

> -----Original Message-----
> From: Fabio Estevam [mailto:festevam at gmail.com]
> Sent: 2015?7?29? 16:46
> To: Wang Shenwei-B38339
> Cc: Greg Kroah-Hartman; linux-arm-kernel at lists.infradead.org;
> linux-serial at vger.kernel.org
> Subject: Re: [PATCH v3 1/1] Serial: imx: add dev_pm_ops to support suspend to
> ram/disk
> 
> On Wed, Jul 29, 2015 at 6:41 PM, Shenwei Wang <Shenwei.Wang@freescale.com>
> wrote:
> 
> >> clk_enable() may fail, so you should check its return value.
> >
> > The check seems a little superfluous because the failure condition for
> > clk_enable is (clk == NULL || IS_ERR(clk)) Once this function is
> > called which means the initialization of the driver is successful.
> 
> Not superfluos. You can't assume that clk_enable() will always be successful.
> Better check its return value.

I am very interesting to know if you could provide an example condition that may cause 
clk_enable failed in this callback function?

Regards,
Shenwei

[PATCH v3 1/1] Serial: imx: add dev_pm_ops to support suspend to ram/disk

[Fabio Estevam]

On Wed, Jul 29, 2015 at 6:54 PM, Shenwei Wang
<Shenwei.Wang@freescale.com> wrote:

> I am very interesting to know if you could provide an example condition that may cause
> clk_enable failed in this callback function?

Let's check clk_enable definition:

int clk_enable(struct clk *clk)
{
    unsigned long flags;
    int ret;

    if (!clk)
        return 0;

    flags = clk_enable_lock();
    ret = clk_core_enable(clk->core);
    clk_enable_unlock(flags);

    return ret;
}

So if I see it right it returns 'int' not 'void' ;-)

[PATCH v3 1/1] Serial: imx: add dev_pm_ops to support suspend to ram/disk

[Shenwei Wang]

> -----Original Message-----
> From: Fabio Estevam [mailto:festevam at gmail.com]
> Sent: 2015?7?29? 17:11
> To: Wang Shenwei-B38339
> Cc: Greg Kroah-Hartman; linux-arm-kernel at lists.infradead.org;
> linux-serial at vger.kernel.org
> Subject: Re: [PATCH v3 1/1] Serial: imx: add dev_pm_ops to support suspend to
> ram/disk
> 
> On Wed, Jul 29, 2015 at 6:54 PM, Shenwei Wang <Shenwei.Wang@freescale.com>
> wrote:
> 
> > I am very interesting to know if you could provide an example
> > condition that may cause clk_enable failed in this callback function?
> 
> Let's check clk_enable definition:
> 
> int clk_enable(struct clk *clk)
> {
>     unsigned long flags;
>     int ret;
> 
>     if (!clk)
>         return 0;
> 
>     flags = clk_enable_lock();
>     ret = clk_core_enable(clk->core);
>     clk_enable_unlock(flags);
> 
>     return ret;
> }
> 
> So if I see it right it returns 'int' not 'void' ;-)

Actually, the function shows even if it is in error status like the parameter "clk" is null the return value is zero. 
Inside the function "clk_core_enable", if everything goes smooth, it still returns zero. 

Moreover, this patch does not care about the return value of clk_enable, whatever value it returns, the following codes keep 
the same.

Regards,
Shenwei


 

[PATCH v3 1/1] Serial: imx: add dev_pm_ops to support suspend to ram/disk

[Lothar Waßmann]

Hi,

> > -----Original Message-----
> > From: Fabio Estevam [mailto:festevam at gmail.com]
> > Sent: 2015?7?29? 17:11
> > To: Wang Shenwei-B38339
> > Cc: Greg Kroah-Hartman; linux-arm-kernel at lists.infradead.org;
> > linux-serial at vger.kernel.org
> > Subject: Re: [PATCH v3 1/1] Serial: imx: add dev_pm_ops to support suspend to
> > ram/disk
> > 
> > On Wed, Jul 29, 2015 at 6:54 PM, Shenwei Wang <Shenwei.Wang@freescale.com>
> > wrote:
> > 
> > > I am very interesting to know if you could provide an example
> > > condition that may cause clk_enable failed in this callback function?
> > 
> > Let's check clk_enable definition:
> > 
> > int clk_enable(struct clk *clk)
> > {
> >     unsigned long flags;
> >     int ret;
> > 
> >     if (!clk)
> >         return 0;
> > 
> >     flags = clk_enable_lock();
> >     ret = clk_core_enable(clk->core);
> >     clk_enable_unlock(flags);
> > 
> >     return ret;
> > }
> > 
> > So if I see it right it returns 'int' not 'void' ;-)
> 
> Actually, the function shows even if it is in error status like the parameter "clk" is null the return value is zero. 
> Inside the function "clk_core_enable", if everything goes smooth, it still returns zero. 
> 
A NULL clk is a valid clk per definition.

> Moreover, this patch does not care about the return value of clk_enable, whatever value it returns, the following codes keep 
> the same.
>
Nevertheless it would be good to inform the user of a failure,
since that may affect the system's stability after returning from
suspend.
Even if the current implementation of a function cannot return an error
code in some specific circumstance, that doesn't mean that this will be
always the case. Thus, checking the return code and acting upon it is
always advisable.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________