[PATCH 0/2] ravb: add sleep PM suspend/resume support

[PATCH 0/2] ravb: add sleep PM suspend/resume support

[Niklas Söderlund]

Hi,

This series adds sleep PM suspend/resume support to the ravb driver. It 
is based on v4.7 and tested on Salvator-X.

I used NFS root filesystem and was able to suspend/resume and keep the 
system working. I also tested suspend/resume with a initramfs when the 
interface was down (ifconfig eth0 down) and it was no problems starting 
after suspend/resume cycle.  Note that there is a known issue on 
Salvator-X where the system will wake up immediately after it have been 
suspended so the system is not suspended for long.

I was not able to test suspend/resume on a Gen2 board due to lack of 
daugterboard, so if anyone could do that it would be great. Sergei have 
promised to try and find the time to test it in his 'copious free time' 
:-) The test procedure is quiet simple:

1. Apply patches
2. Boot and verify network is working
3. Suspend system 'echo mem > /sys/power/state'
4. Wake up system
5. Verify network is still working

Niklas Söderlund (2):
  ravb: use SET_RUNTIME_PM_OPS macro
  ravb: add sleep PM suspend/resume support

 drivers/net/ethernet/renesas/ravb_main.c | 75 +++++++++++++++++++++++++++-----
 1 file changed, 65 insertions(+), 10 deletions(-)

-- 
2.9.0

[PATCH 1/2] ravb: use SET_RUNTIME_PM_OPS macro

[Niklas Söderlund]

Use macro to define the runtime PM operations.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/net/ethernet/renesas/ravb_main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 867caf6..da8da86 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2111,8 +2111,7 @@ static int ravb_runtime_nop(struct device *dev)
 }
 
 static const struct dev_pm_ops ravb_dev_pm_ops = {
-	.runtime_suspend = ravb_runtime_nop,
-	.runtime_resume = ravb_runtime_nop,
+	SET_RUNTIME_PM_OPS(ravb_runtime_nop, ravb_runtime_nop, NULL)
 };
 
 #define RAVB_PM_OPS (&ravb_dev_pm_ops)
-- 
2.9.0

Re: [PATCH 1/2] ravb: use SET_RUNTIME_PM_OPS macro

[Geert Uytterhoeven]

On Wed, Aug 3, 2016 at 3:56 PM, Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> Use macro to define the runtime PM operations.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Duplicate of  commit 524c6f691b99065577b245b250efe93fb0cda5c4
Author: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
Date:   Mon May 30 05:25:43 2016 +0900

    ravb: Add SET_RUNTIME_PM_OPS macro

    Use SET_RUNTIME_PM_OPS macro instead of assigning a member of
    dev_pm_ops directly.

    Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
    Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
    Acked-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] ravb: use SET_RUNTIME_PM_OPS macro

[Niklas Söderlund]

On 2016-08-04 15:33:18 +0200, Geert Uytterhoeven wrote:
> On Wed, Aug 3, 2016 at 3:56 PM, Niklas Söderlund
> <niklas.soderlund+renesas@ragnatech.se> wrote:
> > Use macro to define the runtime PM operations.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> Duplicate of  commit 524c6f691b99065577b245b250efe93fb0cda5c4
> Author: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> Date:   Mon May 30 05:25:43 2016 +0900
> 
>     ravb: Add SET_RUNTIME_PM_OPS macro
> 
>     Use SET_RUNTIME_PM_OPS macro instead of assigning a member of
>     dev_pm_ops directly.
> 
>     Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
>     Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
>     Acked-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>     Signed-off-by: David S. Miller <davem@davemloft.net>

Wops, I will drop my commit then. Thanks for pointing it out.

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

-- 
Regards,
Niklas Söderlund

[PATCH 2/2] ravb: add sleep PM suspend/resume support

[Niklas Söderlund]

The interface would not function after the system had been woken up
after have been suspended (echo mem > /sys/power/state) cycle. The
reason for this is that all device registers have been reset to its
default values. This patch adds sleep suspend and resume functions that
detached the interface at suspend and restore the registers and reattach
the interface at resume.

Only the registers that are only configured at probe time needs to be
explicitly restored by the resume handler. All other registers are
reconfigured by either reopening the device in the resume handler (if
the device was running when the system was suspended) or when the
interface is opened by a user at a later time.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/net/ethernet/renesas/ravb_main.c | 72 ++++++++++++++++++++++++++++----
 1 file changed, 64 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index da8da86..1de55c9 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1876,6 +1876,20 @@ static int ravb_set_gti(struct net_device *ndev)
 	return 0;
 }
 
+static void ravb_set_config_mode(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	if (priv->chip_id == RCAR_GEN2) {
+		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG);
+		/* Set CSEL value */
+		ravb_modify(ndev, CCC, CCC_CSEL, CCC_CSEL_HPB);
+	} else {
+		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG |
+			    CCC_GAC | CCC_CSEL_HPB);
+	}
+}
+
 static int ravb_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -1979,14 +1993,7 @@ static int ravb_probe(struct platform_device *pdev)
 	ndev->ethtool_ops = &ravb_ethtool_ops;
 
 	/* Set AVB config mode */
-	if (chip_id == RCAR_GEN2) {
-		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG);
-		/* Set CSEL value */
-		ravb_modify(ndev, CCC, CCC_CSEL, CCC_CSEL_HPB);
-	} else {
-		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG |
-			    CCC_GAC | CCC_CSEL_HPB);
-	}
+	ravb_set_config_mode(ndev);
 
 	/* Set GTI value */
 	error = ravb_set_gti(ndev);
@@ -2098,6 +2105,54 @@ static int ravb_remove(struct platform_device *pdev)
 }
 
 #ifdef CONFIG_PM
+static int ravb_runtime_suspend(struct device *dev)
+{
+	struct net_device *ndev = dev_get_drvdata(dev);
+	int ret = 0;
+
+	if (netif_running(ndev)) {
+		netif_device_detach(ndev);
+		ret = ravb_close(ndev);
+	}
+
+	return ret;
+}
+
+static int ravb_runtime_resume(struct device *dev)
+{
+	struct net_device *ndev = dev_get_drvdata(dev);
+	struct ravb_private *priv = netdev_priv(ndev);
+	int ret = 0;
+
+	/* All register have been reset to default values.
+	 * Restore all registers which where setup at probe time and
+	 * reopen device if it was running before system suspended.
+	 */
+
+	/* Set AVB config mode */
+	ravb_set_config_mode(ndev);
+
+	/* Set GTI value */
+	ret = ravb_set_gti(ndev);
+	if (ret)
+		return ret;
+
+	/* Request GTI loading */
+	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
+
+	/* Restore descriptor base address table */
+	ravb_write(ndev, priv->desc_bat_dma, DBAT);
+
+	if (netif_running(ndev)) {
+		ret = ravb_open(ndev);
+		if (ret < 0)
+			return ret;
+		netif_device_attach(ndev);
+	}
+
+	return ret;
+}
+
 static int ravb_runtime_nop(struct device *dev)
 {
 	/* Runtime PM callback shared between ->runtime_suspend()
@@ -2111,6 +2166,7 @@ static int ravb_runtime_nop(struct device *dev)
 }
 
 static const struct dev_pm_ops ravb_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(ravb_runtime_suspend, ravb_runtime_resume)
 	SET_RUNTIME_PM_OPS(ravb_runtime_nop, ravb_runtime_nop, NULL)
 };
 
-- 
2.9.0

Re: [PATCH 2/2] ravb: add sleep PM suspend/resume support

[David Miller]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Date: Wed,  3 Aug 2016 15:56:47 +0200

> The interface would not function after the system had been woken up
> after have been suspended (echo mem > /sys/power/state) cycle. The
> reason for this is that all device registers have been reset to its
> default values. This patch adds sleep suspend and resume functions that
> detached the interface at suspend and restore the registers and reattach
> the interface at resume.
> 
> Only the registers that are only configured at probe time needs to be
> explicitly restored by the resume handler. All other registers are
> reconfigured by either reopening the device in the resume handler (if
> the device was running when the system was suspended) or when the
> interface is opened by a user at a later time.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Applied to net-next, thanks.

Re: [PATCH 2/2] ravb: add sleep PM suspend/resume support

[Sergei Shtylyov]

Hello.

On 8/10/2016 2:16 AM, David Miller wrote:

>> The interface would not function after the system had been woken up
>> after have been suspended (echo mem > /sys/power/state) cycle. The
>> reason for this is that all device registers have been reset to its
>> default values. This patch adds sleep suspend and resume functions that
>> detached the interface at suspend and restore the registers and reattach
>> the interface at resume.
>>
>> Only the registers that are only configured at probe time needs to be
>> explicitly restored by the resume handler. All other registers are
>> reconfigured by either reopening the device in the resume handler (if
>> the device was running when the system was suspended) or when the
>> interface is opened by a user at a later time.
>>
>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>
> Applied to net-next, thanks.

    Ugh, I should have reviewed the patch earlier -- I was postponing this 
until I have the time to test it... Since the patch did have some visible 
issues, it should've been recast before applying. DaveM, please in the future 
could you ping me before merging?

MBR, Sergei

Re: [PATCH 2/2] ravb: add sleep PM suspend/resume support

[David Miller]

From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Wed, 10 Aug 2016 13:47:26 +0300

>    Ugh, I should have reviewed the patch earlier -- I was postponing this
>    until I have the time to test it... Since the patch did have some
>    visible issues, it should've been recast before applying. DaveM,
>    please in the future could you ping me before merging?

Review patches in a timely manner.

If I can go through a 5 day email outage and still see this patch
rotting in patchwork, you could have reviewed it in time.

Re: [PATCH 2/2] ravb: add sleep PM suspend/resume support

[Sergei Shtylyov]

Hello.

On 8/3/2016 4:56 PM, Niklas Söderlund wrote:

> The interface would not function after the system had been woken up
> after have been suspended (echo mem > /sys/power/state) cycle. The
> reason for this is that all device registers have been reset to its
> default values. This patch adds sleep suspend and resume functions that
> detached the interface at suspend and restore the registers and reattach
> the interface at resume.
>
> Only the registers that are only configured at probe time needs to be
> explicitly restored by the resume handler. All other registers are
> reconfigured by either reopening the device in the resume handler (if
> the device was running when the system was suspended) or when the
> interface is opened by a user at a later time.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 72 ++++++++++++++++++++++++++++----
>  1 file changed, 64 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index da8da86..1de55c9 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> @@ -2111,6 +2166,7 @@ static int ravb_runtime_nop(struct device *dev)
>  }
>
>  static const struct dev_pm_ops ravb_dev_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(ravb_runtime_suspend, ravb_runtime_resume)

    Why in the world you used runtime_ in the usual suspend/resume method 
names?! Since DaveM have already taken the patch, please send a follow-up 
patch to remove that infix.

>  	SET_RUNTIME_PM_OPS(ravb_runtime_nop, ravb_runtime_nop, NULL)
>  };
>

MBR, Sergei

Re: [PATCH 2/2] ravb: add sleep PM suspend/resume support

[Niklas Söderlund]

On 2016-08-10 13:40:51 +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 8/3/2016 4:56 PM, Niklas Söderlund wrote:
> 
> > The interface would not function after the system had been woken up
> > after have been suspended (echo mem > /sys/power/state) cycle. The
> > reason for this is that all device registers have been reset to its
> > default values. This patch adds sleep suspend and resume functions that
> > detached the interface at suspend and restore the registers and reattach
> > the interface at resume.
> > 
> > Only the registers that are only configured at probe time needs to be
> > explicitly restored by the resume handler. All other registers are
> > reconfigured by either reopening the device in the resume handler (if
> > the device was running when the system was suspended) or when the
> > interface is opened by a user at a later time.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/net/ethernet/renesas/ravb_main.c | 72 ++++++++++++++++++++++++++++----
> >  1 file changed, 64 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> > index da8da86..1de55c9 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> [...]
> > @@ -2111,6 +2166,7 @@ static int ravb_runtime_nop(struct device *dev)
> >  }
> > 
> >  static const struct dev_pm_ops ravb_dev_pm_ops = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(ravb_runtime_suspend, ravb_runtime_resume)
> 
>    Why in the world you used runtime_ in the usual suspend/resume method
> names?! Since DaveM have already taken the patch, please send a follow-up
> patch to remove that infix.

Sorry about that, stupid error on my part. Will send a follow-up patch.  
Thanks for finding it.

> 
> >  	SET_RUNTIME_PM_OPS(ravb_runtime_nop, ravb_runtime_nop, NULL)
> >  };
> > 
> 
> MBR, Sergei
> 

-- 
Regards,
Niklas Söderlund