[PATCHv2 0/5] sh_eth: add wake-on-lan support via magic packet

[PATCHv2 0/5] sh_eth: add wake-on-lan support via magic packet

[Niklas Söderlund]

Hi,

This series adds support for Wake-on-Lan using Magic Packet for a few 
models of the sh_eth driver. Patch 1/5 adds generic support to control 
and support WoL while patches 2/5 - 5/5 enable different models.

Changes since v1.
- Spit generic WoL functionality and device enablement to different 
  patches.
- Enable more devices then Gen2 after feedback from Geert and 
  datasheets.
- Do not set mdp->irq_enabled = false and remove specific MagicPacket 
  interrupt clearing, instead let sh_eth_error() clear the interrupt as 
  for other EMAC interrupts, thanks Sergei for the suggestion.
- Use the original return logic in sh_eth_resume().
- Moved sh_eth_private variable *clk to top of data structure  to avoid 
  possible gaps due to alignment restrictions.
- Make wol_enabled in sh_eth_private part of the already existing 
  bitfield instead of a bool.
- Do not initiate mdp->wol_enabled to 0, the struct is kzalloc'ed so 
  it's already set to 0.

Niklas Söderlund (5):
  sh_eth: add generic wake-on-lan support via magic packet
  sh_eth: enable wake-on-lan for Gen2 devices
  sh_eth: enable wake-on-lan for r8a7740/armadillo
  sh_eth: enable wake-on-lan for sh7743
  sh_eth: enable wake-on-lan for sh7763

 drivers/net/ethernet/renesas/sh_eth.c | 121 +++++++++++++++++++++++++++++++---
 drivers/net/ethernet/renesas/sh_eth.h |   3 +
 2 files changed, 114 insertions(+), 10 deletions(-)

-- 
2.10.2

[PATCHv2 1/5] sh_eth: add generic wake-on-lan support via magic packet

[Niklas Söderlund]

Add generic functionality to support Wake-on-Lan using MagicPacket which
are supported by at least a few versions of sh_eth. Only add
functionality for WoL, no specific sh_eth version are marked to support
WoL yet.

WoL is enabled in the suspend callback by setting MagicPacket detection
and disabling all interrupts expect MagicPacket. In the resume path the
driver needs to reset the hardware to rearm the WoL logic, this prevents
the driver from simply restoring the registers and to take advantage of
that sh_eth was not suspended to reduce resume time. To reset the
hardware the driver close and reopens the device just like it would do
in a normal suspend/resume scenario without WoL enabled, but it both
close and open the device in the resume callback since the device needs
to be open for WoL to work.

One quirk needed for WoL is that the module clock needs to be prevented
from being switched off by Runtime PM. To keep the clock alive the
suspend callback need to call clk_enable() directly to increase the
usage count of the clock. Then when Runtime PM decreases the clock usage
count it won't reach 0 and be switched off.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/net/ethernet/renesas/sh_eth.c | 112 +++++++++++++++++++++++++++++++---
 drivers/net/ethernet/renesas/sh_eth.h |   3 +
 2 files changed, 107 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 05b0dc5..87640b9 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -2199,6 +2199,33 @@ static int sh_eth_set_ringparam(struct net_device *ndev,
 	return 0;
 }
 
+static void sh_eth_get_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
+{
+	struct sh_eth_private *mdp = netdev_priv(ndev);
+
+	wol->supported = 0;
+	wol->wolopts = 0;
+
+	if (mdp->cd->magic && mdp->clk) {
+		wol->supported = WAKE_MAGIC;
+		wol->wolopts = mdp->wol_enabled ? WAKE_MAGIC : 0;
+	}
+}
+
+static int sh_eth_set_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
+{
+	struct sh_eth_private *mdp = netdev_priv(ndev);
+
+	if (!mdp->cd->magic || !mdp->clk || wol->wolopts & ~WAKE_MAGIC)
+		return -EOPNOTSUPP;
+
+	mdp->wol_enabled = !!(wol->wolopts & WAKE_MAGIC);
+
+	device_set_wakeup_enable(&mdp->pdev->dev, mdp->wol_enabled);
+
+	return 0;
+}
+
 static const struct ethtool_ops sh_eth_ethtool_ops = {
 	.get_regs_len	= sh_eth_get_regs_len,
 	.get_regs	= sh_eth_get_regs,
@@ -2213,6 +2240,8 @@ static const struct ethtool_ops sh_eth_ethtool_ops = {
 	.set_ringparam	= sh_eth_set_ringparam,
 	.get_link_ksettings = sh_eth_get_link_ksettings,
 	.set_link_ksettings = sh_eth_set_link_ksettings,
+	.get_wol	= sh_eth_get_wol,
+	.set_wol	= sh_eth_set_wol,
 };
 
 /* network device open function */
@@ -3017,6 +3046,11 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
 		goto out_release;
 	}
 
+	/* Get clock, if not found that's OK but Wake-On-Lan is unavailable */
+	mdp->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(mdp->clk))
+		mdp->clk = NULL;
+
 	ndev->base_addr = res->start;
 
 	spin_lock_init(&mdp->lock);
@@ -3111,6 +3145,9 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
 	if (ret)
 		goto out_napi_del;
 
+	if (mdp->cd->magic && mdp->clk)
+		device_set_wakeup_capable(&pdev->dev, 1);
+
 	/* print device information */
 	netdev_info(ndev, "Base address at 0x%x, %pM, IRQ %d.\n",
 		    (u32)ndev->base_addr, ndev->dev_addr, ndev->irq);
@@ -3150,15 +3187,67 @@ static int sh_eth_drv_remove(struct platform_device *pdev)
 
 #ifdef CONFIG_PM
 #ifdef CONFIG_PM_SLEEP
+static int sh_eth_wol_setup(struct net_device *ndev)
+{
+	struct sh_eth_private *mdp = netdev_priv(ndev);
+
+	/* Only allow ECI interrupts */
+	synchronize_irq(ndev->irq);
+	napi_disable(&mdp->napi);
+	sh_eth_write(ndev, DMAC_M_ECI, EESIPR);
+
+	/* Enable MagicPacket */
+	sh_eth_modify(ndev, ECMR, 0, ECMR_PMDE);
+
+	/* Increased clock usage so device won't be suspended */
+	clk_enable(mdp->clk);
+
+	return enable_irq_wake(ndev->irq);
+}
+
+static int sh_eth_wol_restore(struct net_device *ndev)
+{
+	struct sh_eth_private *mdp = netdev_priv(ndev);
+	int ret;
+
+	napi_enable(&mdp->napi);
+
+	/* Disable MagicPacket */
+	sh_eth_modify(ndev, ECMR, ECMR_PMDE, 0);
+
+	/* The device need to be reset to restore MagicPacket logic
+	 * for next wakeup. If we close and open the device it will
+	 * both be reset and all registers restored. This is what
+	 * happens during suspend and resume without WoL enabled.
+	 */
+	ret = sh_eth_close(ndev);
+	if (ret < 0)
+		return ret;
+	ret = sh_eth_open(ndev);
+	if (ret < 0)
+		return ret;
+
+	/* Restore clock usage count */
+	clk_disable(mdp->clk);
+
+	return disable_irq_wake(ndev->irq);
+}
+
 static int sh_eth_suspend(struct device *dev)
 {
 	struct net_device *ndev = dev_get_drvdata(dev);
+	struct sh_eth_private *mdp = netdev_priv(ndev);
 	int ret = 0;
 
-	if (netif_running(ndev)) {
-		netif_device_detach(ndev);
+	if (!netif_running(ndev))
+		return 0;
+
+	netif_device_detach(ndev);
+
+	if (mdp->wol_enabled)
+		ret = sh_eth_wol_setup(ndev);
+	else
 		ret = sh_eth_close(ndev);
-	}
 
 	return ret;
 }
@@ -3166,14 +3255,21 @@ static int sh_eth_suspend(struct device *dev)
 static int sh_eth_resume(struct device *dev)
 {
 	struct net_device *ndev = dev_get_drvdata(dev);
+	struct sh_eth_private *mdp = netdev_priv(ndev);
 	int ret = 0;
 
-	if (netif_running(ndev)) {
+	if (!netif_running(ndev))
+		return 0;
+
+	if (mdp->wol_enabled)
+		ret = sh_eth_wol_restore(ndev);
+	else
 		ret = sh_eth_open(ndev);
-		if (ret < 0)
-			return ret;
-		netif_device_attach(ndev);
-	}
+
+	if (ret < 0)
+		return ret;
+
+	netif_device_attach(ndev);
 
 	return ret;
 }
diff --git a/drivers/net/ethernet/renesas/sh_eth.h b/drivers/net/ethernet/renesas/sh_eth.h
index d050f37..4ceed00 100644
--- a/drivers/net/ethernet/renesas/sh_eth.h
+++ b/drivers/net/ethernet/renesas/sh_eth.h
@@ -493,6 +493,7 @@ struct sh_eth_cpu_data {
 	unsigned shift_rd0:1;	/* shift Rx descriptor word 0 right by 16 */
 	unsigned rmiimode:1;	/* EtherC has RMIIMODE register */
 	unsigned rtrate:1;	/* EtherC has RTRATE register */
+	unsigned magic:1;	/* EtherC has ECMR.PMDE and ECSR.MPD */
 };
 
 struct sh_eth_private {
@@ -501,6 +502,7 @@ struct sh_eth_private {
 	const u16 *reg_offset;
 	void __iomem *addr;
 	void __iomem *tsu_addr;
+	struct clk *clk;
 	u32 num_rx_ring;
 	u32 num_tx_ring;
 	dma_addr_t rx_desc_dma;
@@ -529,6 +531,7 @@ struct sh_eth_private {
 	unsigned no_ether_link:1;
 	unsigned ether_link_active_low:1;
 	unsigned is_opened:1;
+	unsigned wol_enabled:1;
 };
 
 static inline void sh_eth_soft_swap(char *src, int len)
-- 
2.10.2

[PATCHv2 2/5] sh_eth: enable wake-on-lan for Gen2 devices

[Niklas Söderlund]

Tested on Gen2 r8a7791/Koelsch.

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

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 87640b9..348ed22 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -624,8 +624,9 @@ static struct sh_eth_cpu_data r8a779x_data = {
 
 	.register_type	= SH_ETH_REG_FAST_RCAR,
 
-	.ecsr_value	= ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD,
-	.ecsipr_value	= ECSIPR_PSRTOIP | ECSIPR_LCHNGIP | ECSIPR_ICDIP,
+	.ecsr_value	= ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD | ECSR_MPD,
+	.ecsipr_value	= ECSIPR_PSRTOIP | ECSIPR_LCHNGIP | ECSIPR_ICDIP |
+			  ECSIPR_MPDIP,
 	.eesipr_value	= 0x01ff009f,
 
 	.tx_check	= EESR_FTC | EESR_CND | EESR_DLC | EESR_CD | EESR_RTO,
@@ -641,6 +642,7 @@ static struct sh_eth_cpu_data r8a779x_data = {
 	.tpauser	= 1,
 	.hw_swap	= 1,
 	.rmiimode	= 1,
+	.magic		= 1,
 };
 #endif /* CONFIG_OF */
 
-- 
2.10.2

[PATCHv2 3/5] sh_eth: enable wake-on-lan for r8a7740/armadillo

[Niklas Söderlund]

Geert Uytterhoeven reported WoL worked on his Armadillo board.

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

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 348ed22..fafde6f 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -577,6 +577,7 @@ static struct sh_eth_cpu_data r8a7740_data = {
 	.tsu		= 1,
 	.select_mii	= 1,
 	.shift_rd0	= 1,
+	.magic		= 1,
 };
 
 /* There is CPU dependent code */
-- 
2.10.2

[PATCHv2 4/5] sh_eth: enable wake-on-lan for sh7743

[Niklas Söderlund]

This is based on public datasheet for sh7743 which shows it have the
same behavior and registers for WoL as other versions of sh_eth.

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

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index fafde6f..927de77 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -822,6 +822,7 @@ static struct sh_eth_cpu_data sh7734_data = {
 	.tsu		= 1,
 	.hw_crc		= 1,
 	.select_mii	= 1,
+	.magic		= 1,
 };
 
 /* SH7763 */
-- 
2.10.2

[PATCHv2 5/5] sh_eth: enable wake-on-lan for sh7763

[Niklas Söderlund]

This is based on public datasheet for sh7763 which shows it have the
same behavior and registers for WoL as other versions of sh_eth.

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

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 927de77..6e80457 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -851,6 +851,7 @@ static struct sh_eth_cpu_data sh7763_data = {
 	.no_ade		= 1,
 	.tsu		= 1,
 	.irq_flags	= IRQF_SHARED,
+	.magic		= 1,
 };
 
 static struct sh_eth_cpu_data sh7619_data = {
-- 
2.10.2

Re: [PATCHv2 4/5] sh_eth: enable wake-on-lan for sh7743

[Geert Uytterhoeven]

On Mon, Dec 12, 2016 at 5:09 PM, Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> This is based on public datasheet for sh7743 which shows it have the

sh7734 (also in the one-line summary)
it has

> same behavior and registers for WoL as other versions of sh_eth.

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: [PATCHv2 3/5] sh_eth: enable wake-on-lan for r8a7740/armadillo

[Geert Uytterhoeven]

On Mon, Dec 12, 2016 at 5:09 PM, Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> Geert Uytterhoeven reported WoL worked on his Armadillo board.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

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: [PATCHv2 2/5] sh_eth: enable wake-on-lan for Gen2 devices

[Geert Uytterhoeven]

On Mon, Dec 12, 2016 at 5:09 PM, Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> Tested on Gen2 r8a7791/Koelsch.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

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: [PATCHv2 1/5] sh_eth: add generic wake-on-lan support via magic packet

[Geert Uytterhoeven]

CC linux-pm

On Mon, Dec 12, 2016 at 5:09 PM, Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> Add generic functionality to support Wake-on-Lan using MagicPacket which
> are supported by at least a few versions of sh_eth. Only add
> functionality for WoL, no specific sh_eth version are marked to support
> WoL yet.
>
> WoL is enabled in the suspend callback by setting MagicPacket detection
> and disabling all interrupts expect MagicPacket. In the resume path the
> driver needs to reset the hardware to rearm the WoL logic, this prevents
> the driver from simply restoring the registers and to take advantage of
> that sh_eth was not suspended to reduce resume time. To reset the
> hardware the driver close and reopens the device just like it would do
> in a normal suspend/resume scenario without WoL enabled, but it both
> close and open the device in the resume callback since the device needs
> to be open for WoL to work.
>
> One quirk needed for WoL is that the module clock needs to be prevented
> from being switched off by Runtime PM. To keep the clock alive the
> suspend callback need to call clk_enable() directly to increase the
> usage count of the clock. Then when Runtime PM decreases the clock usage
> count it won't reach 0 and be switched off.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Thanks for the update!

I've verified WoL is working on r8a7791/koelsch and r8a7740/armadillo.

However, if I look at /sys/kernel/debug/wakeup_sources, "active_count" and
"event_count" for the Ethernet device do not increase when using WoL, while
they do for the GPIO keyboard when using the keyboard for wake up.
So something seems to be missing from a bookkeeping point of view.

Interestingly, "wakeup_count" does not increase for both?
Has this been broken recently?

> ---
>  drivers/net/ethernet/renesas/sh_eth.c | 112 +++++++++++++++++++++++++++++++---
>  drivers/net/ethernet/renesas/sh_eth.h |   3 +
>  2 files changed, 107 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> index 05b0dc5..87640b9 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> @@ -2199,6 +2199,33 @@ static int sh_eth_set_ringparam(struct net_device *ndev,
>         return 0;
>  }
>
> +static void sh_eth_get_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
> +{
> +       struct sh_eth_private *mdp = netdev_priv(ndev);
> +
> +       wol->supported = 0;
> +       wol->wolopts = 0;
> +
> +       if (mdp->cd->magic && mdp->clk) {
> +               wol->supported = WAKE_MAGIC;
> +               wol->wolopts = mdp->wol_enabled ? WAKE_MAGIC : 0;
> +       }
> +}
> +
> +static int sh_eth_set_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
> +{
> +       struct sh_eth_private *mdp = netdev_priv(ndev);
> +
> +       if (!mdp->cd->magic || !mdp->clk || wol->wolopts & ~WAKE_MAGIC)
> +               return -EOPNOTSUPP;
> +
> +       mdp->wol_enabled = !!(wol->wolopts & WAKE_MAGIC);
> +
> +       device_set_wakeup_enable(&mdp->pdev->dev, mdp->wol_enabled);
> +
> +       return 0;
> +}
> +
>  static const struct ethtool_ops sh_eth_ethtool_ops = {
>         .get_regs_len   = sh_eth_get_regs_len,
>         .get_regs       = sh_eth_get_regs,
> @@ -2213,6 +2240,8 @@ static const struct ethtool_ops sh_eth_ethtool_ops = {
>         .set_ringparam  = sh_eth_set_ringparam,
>         .get_link_ksettings = sh_eth_get_link_ksettings,
>         .set_link_ksettings = sh_eth_set_link_ksettings,
> +       .get_wol        = sh_eth_get_wol,
> +       .set_wol        = sh_eth_set_wol,
>  };
>
>  /* network device open function */
> @@ -3017,6 +3046,11 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
>                 goto out_release;
>         }
>
> +       /* Get clock, if not found that's OK but Wake-On-Lan is unavailable */
> +       mdp->clk = devm_clk_get(&pdev->dev, NULL);
> +       if (IS_ERR(mdp->clk))
> +               mdp->clk = NULL;
> +
>         ndev->base_addr = res->start;
>
>         spin_lock_init(&mdp->lock);
> @@ -3111,6 +3145,9 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
>         if (ret)
>                 goto out_napi_del;
>
> +       if (mdp->cd->magic && mdp->clk)
> +               device_set_wakeup_capable(&pdev->dev, 1);
> +
>         /* print device information */
>         netdev_info(ndev, "Base address at 0x%x, %pM, IRQ %d.\n",
>                     (u32)ndev->base_addr, ndev->dev_addr, ndev->irq);
> @@ -3150,15 +3187,67 @@ static int sh_eth_drv_remove(struct platform_device *pdev)
>
>  #ifdef CONFIG_PM
>  #ifdef CONFIG_PM_SLEEP
> +static int sh_eth_wol_setup(struct net_device *ndev)
> +{
> +       struct sh_eth_private *mdp = netdev_priv(ndev);
> +
> +       /* Only allow ECI interrupts */
> +       synchronize_irq(ndev->irq);
> +       napi_disable(&mdp->napi);
> +       sh_eth_write(ndev, DMAC_M_ECI, EESIPR);
> +
> +       /* Enable MagicPacket */
> +       sh_eth_modify(ndev, ECMR, 0, ECMR_PMDE);
> +
> +       /* Increased clock usage so device won't be suspended */
> +       clk_enable(mdp->clk);
> +
> +       return enable_irq_wake(ndev->irq);
> +}
> +
> +static int sh_eth_wol_restore(struct net_device *ndev)
> +{
> +       struct sh_eth_private *mdp = netdev_priv(ndev);
> +       int ret;
> +
> +       napi_enable(&mdp->napi);
> +
> +       /* Disable MagicPacket */
> +       sh_eth_modify(ndev, ECMR, ECMR_PMDE, 0);
> +
> +       /* The device need to be reset to restore MagicPacket logic
> +        * for next wakeup. If we close and open the device it will
> +        * both be reset and all registers restored. This is what
> +        * happens during suspend and resume without WoL enabled.
> +        */
> +       ret = sh_eth_close(ndev);
> +       if (ret < 0)
> +               return ret;
> +       ret = sh_eth_open(ndev);
> +       if (ret < 0)
> +               return ret;
> +
> +       /* Restore clock usage count */
> +       clk_disable(mdp->clk);
> +
> +       return disable_irq_wake(ndev->irq);
> +}
> +
>  static int sh_eth_suspend(struct device *dev)
>  {
>         struct net_device *ndev = dev_get_drvdata(dev);
> +       struct sh_eth_private *mdp = netdev_priv(ndev);
>         int ret = 0;
>
> -       if (netif_running(ndev)) {
> -               netif_device_detach(ndev);
> +       if (!netif_running(ndev))
> +               return 0;
> +
> +       netif_device_detach(ndev);
> +
> +       if (mdp->wol_enabled)
> +               ret = sh_eth_wol_setup(ndev);
> +       else
>                 ret = sh_eth_close(ndev);
> -       }
>
>         return ret;
>  }
> @@ -3166,14 +3255,21 @@ static int sh_eth_suspend(struct device *dev)
>  static int sh_eth_resume(struct device *dev)
>  {
>         struct net_device *ndev = dev_get_drvdata(dev);
> +       struct sh_eth_private *mdp = netdev_priv(ndev);
>         int ret = 0;
>
> -       if (netif_running(ndev)) {
> +       if (!netif_running(ndev))
> +               return 0;
> +
> +       if (mdp->wol_enabled)
> +               ret = sh_eth_wol_restore(ndev);
> +       else
>                 ret = sh_eth_open(ndev);
> -               if (ret < 0)
> -                       return ret;
> -               netif_device_attach(ndev);
> -       }
> +
> +       if (ret < 0)
> +               return ret;
> +
> +       netif_device_attach(ndev);
>
>         return ret;
>  }
> diff --git a/drivers/net/ethernet/renesas/sh_eth.h b/drivers/net/ethernet/renesas/sh_eth.h
> index d050f37..4ceed00 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.h
> +++ b/drivers/net/ethernet/renesas/sh_eth.h
> @@ -493,6 +493,7 @@ struct sh_eth_cpu_data {
>         unsigned shift_rd0:1;   /* shift Rx descriptor word 0 right by 16 */
>         unsigned rmiimode:1;    /* EtherC has RMIIMODE register */
>         unsigned rtrate:1;      /* EtherC has RTRATE register */
> +       unsigned magic:1;       /* EtherC has ECMR.PMDE and ECSR.MPD */
>  };
>
>  struct sh_eth_private {
> @@ -501,6 +502,7 @@ struct sh_eth_private {
>         const u16 *reg_offset;
>         void __iomem *addr;
>         void __iomem *tsu_addr;
> +       struct clk *clk;
>         u32 num_rx_ring;
>         u32 num_tx_ring;
>         dma_addr_t rx_desc_dma;
> @@ -529,6 +531,7 @@ struct sh_eth_private {
>         unsigned no_ether_link:1;
>         unsigned ether_link_active_low:1;
>         unsigned is_opened:1;
> +       unsigned wol_enabled:1;
>  };
>
>  static inline void sh_eth_soft_swap(char *src, int len)
> --
> 2.10.2

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: [PATCHv2 1/5] sh_eth: add generic wake-on-lan support via magic packet

[Niklas Söderlund]

Hi Geert,

Thanks for feedback and testing!

On 2016-12-13 14:03:52 +0100, Geert Uytterhoeven wrote:
> CC linux-pm

I think you forgot to CC linux-pm :-)

> 
> On Mon, Dec 12, 2016 at 5:09 PM, Niklas Söderlund
> <niklas.soderlund+renesas@ragnatech.se> wrote:
> > Add generic functionality to support Wake-on-Lan using MagicPacket which
> > are supported by at least a few versions of sh_eth. Only add
> > functionality for WoL, no specific sh_eth version are marked to support
> > WoL yet.
> >
> > WoL is enabled in the suspend callback by setting MagicPacket detection
> > and disabling all interrupts expect MagicPacket. In the resume path the
> > driver needs to reset the hardware to rearm the WoL logic, this prevents
> > the driver from simply restoring the registers and to take advantage of
> > that sh_eth was not suspended to reduce resume time. To reset the
> > hardware the driver close and reopens the device just like it would do
> > in a normal suspend/resume scenario without WoL enabled, but it both
> > close and open the device in the resume callback since the device needs
> > to be open for WoL to work.
> >
> > One quirk needed for WoL is that the module clock needs to be prevented
> > from being switched off by Runtime PM. To keep the clock alive the
> > suspend callback need to call clk_enable() directly to increase the
> > usage count of the clock. Then when Runtime PM decreases the clock usage
> > count it won't reach 0 and be switched off.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> Thanks for the update!
> 
> I've verified WoL is working on r8a7791/koelsch and r8a7740/armadillo.
> 
> However, if I look at /sys/kernel/debug/wakeup_sources, "active_count" and
> "event_count" for the Ethernet device do not increase when using WoL, while
> they do for the GPIO keyboard when using the keyboard for wake up.
> So something seems to be missing from a bookkeeping point of view.

Cool, now I know why some net drivers call pm_wakeup_event() if the 
wakeup source was WoL :-) I added this to sh_eth and now the bookkeeping 
seems to work as you describe, "active_count" and "event_count" are 
incremented while waking up from WoL. I will include this in the next 
version, thanks for reporting I had no idea to check for this.

> 
> Interestingly, "wakeup_count" does not increase for both?
> Has this been broken recently?

I had a quick look at this and the 'wakeup_count' is increased in 
wakeup_source_report_event() which is in the call path from 
pm_wakeup_event().

pm_wakeup_event()
  __pm_wakeup_event()
    wakeup_source_report_event()

static void wakeup_source_report_event(struct wakeup_source *ws) 
{
        ws->event_count++;
        /* This is racy, but the counter is approximate anyway. */
        if (events_check_enabled)
                ws->wakeup_count++;

        if (!ws->active)
                wakeup_source_activate(ws);
}

So maybe 'wakeup_count' is not incremented due to the race with 
events_check_enabled? But then again I'm new to PM so I might miss 
something obvious. I'm also not sure if I can do anything in this series 
to improve the behavior of 'wakeup_count' for sh_eth?

> 
> > ---
> >  drivers/net/ethernet/renesas/sh_eth.c | 112 +++++++++++++++++++++++++++++++---
> >  drivers/net/ethernet/renesas/sh_eth.h |   3 +
> >  2 files changed, 107 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> > index 05b0dc5..87640b9 100644
> > --- a/drivers/net/ethernet/renesas/sh_eth.c
> > +++ b/drivers/net/ethernet/renesas/sh_eth.c
> > @@ -2199,6 +2199,33 @@ static int sh_eth_set_ringparam(struct net_device *ndev,
> >         return 0;
> >  }
> >
> > +static void sh_eth_get_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
> > +{
> > +       struct sh_eth_private *mdp = netdev_priv(ndev);
> > +
> > +       wol->supported = 0;
> > +       wol->wolopts = 0;
> > +
> > +       if (mdp->cd->magic && mdp->clk) {
> > +               wol->supported = WAKE_MAGIC;
> > +               wol->wolopts = mdp->wol_enabled ? WAKE_MAGIC : 0;
> > +       }
> > +}
> > +
> > +static int sh_eth_set_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
> > +{
> > +       struct sh_eth_private *mdp = netdev_priv(ndev);
> > +
> > +       if (!mdp->cd->magic || !mdp->clk || wol->wolopts & ~WAKE_MAGIC)
> > +               return -EOPNOTSUPP;
> > +
> > +       mdp->wol_enabled = !!(wol->wolopts & WAKE_MAGIC);
> > +
> > +       device_set_wakeup_enable(&mdp->pdev->dev, mdp->wol_enabled);
> > +
> > +       return 0;
> > +}
> > +
> >  static const struct ethtool_ops sh_eth_ethtool_ops = {
> >         .get_regs_len   = sh_eth_get_regs_len,
> >         .get_regs       = sh_eth_get_regs,
> > @@ -2213,6 +2240,8 @@ static const struct ethtool_ops sh_eth_ethtool_ops = {
> >         .set_ringparam  = sh_eth_set_ringparam,
> >         .get_link_ksettings = sh_eth_get_link_ksettings,
> >         .set_link_ksettings = sh_eth_set_link_ksettings,
> > +       .get_wol        = sh_eth_get_wol,
> > +       .set_wol        = sh_eth_set_wol,
> >  };
> >
> >  /* network device open function */
> > @@ -3017,6 +3046,11 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
> >                 goto out_release;
> >         }
> >
> > +       /* Get clock, if not found that's OK but Wake-On-Lan is unavailable */
> > +       mdp->clk = devm_clk_get(&pdev->dev, NULL);
> > +       if (IS_ERR(mdp->clk))
> > +               mdp->clk = NULL;
> > +
> >         ndev->base_addr = res->start;
> >
> >         spin_lock_init(&mdp->lock);
> > @@ -3111,6 +3145,9 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
> >         if (ret)
> >                 goto out_napi_del;
> >
> > +       if (mdp->cd->magic && mdp->clk)
> > +               device_set_wakeup_capable(&pdev->dev, 1);
> > +
> >         /* print device information */
> >         netdev_info(ndev, "Base address at 0x%x, %pM, IRQ %d.\n",
> >                     (u32)ndev->base_addr, ndev->dev_addr, ndev->irq);
> > @@ -3150,15 +3187,67 @@ static int sh_eth_drv_remove(struct platform_device *pdev)
> >
> >  #ifdef CONFIG_PM
> >  #ifdef CONFIG_PM_SLEEP
> > +static int sh_eth_wol_setup(struct net_device *ndev)
> > +{
> > +       struct sh_eth_private *mdp = netdev_priv(ndev);
> > +
> > +       /* Only allow ECI interrupts */
> > +       synchronize_irq(ndev->irq);
> > +       napi_disable(&mdp->napi);
> > +       sh_eth_write(ndev, DMAC_M_ECI, EESIPR);
> > +
> > +       /* Enable MagicPacket */
> > +       sh_eth_modify(ndev, ECMR, 0, ECMR_PMDE);
> > +
> > +       /* Increased clock usage so device won't be suspended */
> > +       clk_enable(mdp->clk);
> > +
> > +       return enable_irq_wake(ndev->irq);
> > +}
> > +
> > +static int sh_eth_wol_restore(struct net_device *ndev)
> > +{
> > +       struct sh_eth_private *mdp = netdev_priv(ndev);
> > +       int ret;
> > +
> > +       napi_enable(&mdp->napi);
> > +
> > +       /* Disable MagicPacket */
> > +       sh_eth_modify(ndev, ECMR, ECMR_PMDE, 0);
> > +
> > +       /* The device need to be reset to restore MagicPacket logic
> > +        * for next wakeup. If we close and open the device it will
> > +        * both be reset and all registers restored. This is what
> > +        * happens during suspend and resume without WoL enabled.
> > +        */
> > +       ret = sh_eth_close(ndev);
> > +       if (ret < 0)
> > +               return ret;
> > +       ret = sh_eth_open(ndev);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       /* Restore clock usage count */
> > +       clk_disable(mdp->clk);
> > +
> > +       return disable_irq_wake(ndev->irq);
> > +}
> > +
> >  static int sh_eth_suspend(struct device *dev)
> >  {
> >         struct net_device *ndev = dev_get_drvdata(dev);
> > +       struct sh_eth_private *mdp = netdev_priv(ndev);
> >         int ret = 0;
> >
> > -       if (netif_running(ndev)) {
> > -               netif_device_detach(ndev);
> > +       if (!netif_running(ndev))
> > +               return 0;
> > +
> > +       netif_device_detach(ndev);
> > +
> > +       if (mdp->wol_enabled)
> > +               ret = sh_eth_wol_setup(ndev);
> > +       else
> >                 ret = sh_eth_close(ndev);
> > -       }
> >
> >         return ret;
> >  }
> > @@ -3166,14 +3255,21 @@ static int sh_eth_suspend(struct device *dev)
> >  static int sh_eth_resume(struct device *dev)
> >  {
> >         struct net_device *ndev = dev_get_drvdata(dev);
> > +       struct sh_eth_private *mdp = netdev_priv(ndev);
> >         int ret = 0;
> >
> > -       if (netif_running(ndev)) {
> > +       if (!netif_running(ndev))
> > +               return 0;
> > +
> > +       if (mdp->wol_enabled)
> > +               ret = sh_eth_wol_restore(ndev);
> > +       else
> >                 ret = sh_eth_open(ndev);
> > -               if (ret < 0)
> > -                       return ret;
> > -               netif_device_attach(ndev);
> > -       }
> > +
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       netif_device_attach(ndev);
> >
> >         return ret;
> >  }
> > diff --git a/drivers/net/ethernet/renesas/sh_eth.h b/drivers/net/ethernet/renesas/sh_eth.h
> > index d050f37..4ceed00 100644
> > --- a/drivers/net/ethernet/renesas/sh_eth.h
> > +++ b/drivers/net/ethernet/renesas/sh_eth.h
> > @@ -493,6 +493,7 @@ struct sh_eth_cpu_data {
> >         unsigned shift_rd0:1;   /* shift Rx descriptor word 0 right by 16 */
> >         unsigned rmiimode:1;    /* EtherC has RMIIMODE register */
> >         unsigned rtrate:1;      /* EtherC has RTRATE register */
> > +       unsigned magic:1;       /* EtherC has ECMR.PMDE and ECSR.MPD */
> >  };
> >
> >  struct sh_eth_private {
> > @@ -501,6 +502,7 @@ struct sh_eth_private {
> >         const u16 *reg_offset;
> >         void __iomem *addr;
> >         void __iomem *tsu_addr;
> > +       struct clk *clk;
> >         u32 num_rx_ring;
> >         u32 num_tx_ring;
> >         dma_addr_t rx_desc_dma;
> > @@ -529,6 +531,7 @@ struct sh_eth_private {
> >         unsigned no_ether_link:1;
> >         unsigned ether_link_active_low:1;
> >         unsigned is_opened:1;
> > +       unsigned wol_enabled:1;
> >  };
> >
> >  static inline void sh_eth_soft_swap(char *src, int len)
> > --
> > 2.10.2
> 
> 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

Re: [PATCHv2 1/5] sh_eth: add generic wake-on-lan support via magic packet

[Niklas Söderlund]

Hi Geert,

Thanks for feedback and testing!

On 2016-12-13 14:03:52 +0100, Geert Uytterhoeven wrote:
> CC linux-pm

I think you forgot to CC linux-pm :-)

> 
> On Mon, Dec 12, 2016 at 5:09 PM, Niklas S�derlund
> <niklas.soderlund+renesas@ragnatech.se> wrote:
> > Add generic functionality to support Wake-on-Lan using MagicPacket which
> > are supported by at least a few versions of sh_eth. Only add
> > functionality for WoL, no specific sh_eth version are marked to support
> > WoL yet.
> >
> > WoL is enabled in the suspend callback by setting MagicPacket detection
> > and disabling all interrupts expect MagicPacket. In the resume path the
> > driver needs to reset the hardware to rearm the WoL logic, this prevents
> > the driver from simply restoring the registers and to take advantage of
> > that sh_eth was not suspended to reduce resume time. To reset the
> > hardware the driver close and reopens the device just like it would do
> > in a normal suspend/resume scenario without WoL enabled, but it both
> > close and open the device in the resume callback since the device needs
> > to be open for WoL to work.
> >
> > One quirk needed for WoL is that the module clock needs to be prevented
> > from being switched off by Runtime PM. To keep the clock alive the
> > suspend callback need to call clk_enable() directly to increase the
> > usage count of the clock. Then when Runtime PM decreases the clock usage
> > count it won't reach 0 and be switched off.
> >
> > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> 
> Thanks for the update!
> 
> I've verified WoL is working on r8a7791/koelsch and r8a7740/armadillo.
> 
> However, if I look at /sys/kernel/debug/wakeup_sources, "active_count" and
> "event_count" for the Ethernet device do not increase when using WoL, while
> they do for the GPIO keyboard when using the keyboard for wake up.
> So something seems to be missing from a bookkeeping point of view.

Cool, now I know why some net drivers call pm_wakeup_event() if the 
wakeup source was WoL :-) I added this to sh_eth and now the bookkeeping 
seems to work as you describe, "active_count" and "event_count" are 
incremented while waking up from WoL. I will include this in the next 
version, thanks for reporting I had no idea to check for this.

> 
> Interestingly, "wakeup_count" does not increase for both?
> Has this been broken recently?

I had a quick look at this and the 'wakeup_count' is increased in 
wakeup_source_report_event() which is in the call path from 
pm_wakeup_event().

pm_wakeup_event()
  __pm_wakeup_event()
    wakeup_source_report_event()

static void wakeup_source_report_event(struct wakeup_source *ws) 
{
        ws->event_count++;
        /* This is racy, but the counter is approximate anyway. */
        if (events_check_enabled)
                ws->wakeup_count++;

        if (!ws->active)
                wakeup_source_activate(ws);
}

So maybe 'wakeup_count' is not incremented due to the race with 
events_check_enabled? But then again I'm new to PM so I might miss 
something obvious. I'm also not sure if I can do anything in this series 
to improve the behavior of 'wakeup_count' for sh_eth?

> 
> > ---
> >  drivers/net/ethernet/renesas/sh_eth.c | 112 +++++++++++++++++++++++++++++++---
> >  drivers/net/ethernet/renesas/sh_eth.h |   3 +
> >  2 files changed, 107 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> > index 05b0dc5..87640b9 100644
> > --- a/drivers/net/ethernet/renesas/sh_eth.c
> > +++ b/drivers/net/ethernet/renesas/sh_eth.c
> > @@ -2199,6 +2199,33 @@ static int sh_eth_set_ringparam(struct net_device *ndev,
> >         return 0;
> >  }
> >
> > +static void sh_eth_get_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
> > +{
> > +       struct sh_eth_private *mdp = netdev_priv(ndev);
> > +
> > +       wol->supported = 0;
> > +       wol->wolopts = 0;
> > +
> > +       if (mdp->cd->magic && mdp->clk) {
> > +               wol->supported = WAKE_MAGIC;
> > +               wol->wolopts = mdp->wol_enabled ? WAKE_MAGIC : 0;
> > +       }
> > +}
> > +
> > +static int sh_eth_set_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
> > +{
> > +       struct sh_eth_private *mdp = netdev_priv(ndev);
> > +
> > +       if (!mdp->cd->magic || !mdp->clk || wol->wolopts & ~WAKE_MAGIC)
> > +               return -EOPNOTSUPP;
> > +
> > +       mdp->wol_enabled = !!(wol->wolopts & WAKE_MAGIC);
> > +
> > +       device_set_wakeup_enable(&mdp->pdev->dev, mdp->wol_enabled);
> > +
> > +       return 0;
> > +}
> > +
> >  static const struct ethtool_ops sh_eth_ethtool_ops = {
> >         .get_regs_len   = sh_eth_get_regs_len,
> >         .get_regs       = sh_eth_get_regs,
> > @@ -2213,6 +2240,8 @@ static const struct ethtool_ops sh_eth_ethtool_ops = {
> >         .set_ringparam  = sh_eth_set_ringparam,
> >         .get_link_ksettings = sh_eth_get_link_ksettings,
> >         .set_link_ksettings = sh_eth_set_link_ksettings,
> > +       .get_wol        = sh_eth_get_wol,
> > +       .set_wol        = sh_eth_set_wol,
> >  };
> >
> >  /* network device open function */
> > @@ -3017,6 +3046,11 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
> >                 goto out_release;
> >         }
> >
> > +       /* Get clock, if not found that's OK but Wake-On-Lan is unavailable */
> > +       mdp->clk = devm_clk_get(&pdev->dev, NULL);
> > +       if (IS_ERR(mdp->clk))
> > +               mdp->clk = NULL;
> > +
> >         ndev->base_addr = res->start;
> >
> >         spin_lock_init(&mdp->lock);
> > @@ -3111,6 +3145,9 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
> >         if (ret)
> >                 goto out_napi_del;
> >
> > +       if (mdp->cd->magic && mdp->clk)
> > +               device_set_wakeup_capable(&pdev->dev, 1);
> > +
> >         /* print device information */
> >         netdev_info(ndev, "Base address at 0x%x, %pM, IRQ %d.\n",
> >                     (u32)ndev->base_addr, ndev->dev_addr, ndev->irq);
> > @@ -3150,15 +3187,67 @@ static int sh_eth_drv_remove(struct platform_device *pdev)
> >
> >  #ifdef CONFIG_PM
> >  #ifdef CONFIG_PM_SLEEP
> > +static int sh_eth_wol_setup(struct net_device *ndev)
> > +{
> > +       struct sh_eth_private *mdp = netdev_priv(ndev);
> > +
> > +       /* Only allow ECI interrupts */
> > +       synchronize_irq(ndev->irq);
> > +       napi_disable(&mdp->napi);
> > +       sh_eth_write(ndev, DMAC_M_ECI, EESIPR);
> > +
> > +       /* Enable MagicPacket */
> > +       sh_eth_modify(ndev, ECMR, 0, ECMR_PMDE);
> > +
> > +       /* Increased clock usage so device won't be suspended */
> > +       clk_enable(mdp->clk);
> > +
> > +       return enable_irq_wake(ndev->irq);
> > +}
> > +
> > +static int sh_eth_wol_restore(struct net_device *ndev)
> > +{
> > +       struct sh_eth_private *mdp = netdev_priv(ndev);
> > +       int ret;
> > +
> > +       napi_enable(&mdp->napi);
> > +
> > +       /* Disable MagicPacket */
> > +       sh_eth_modify(ndev, ECMR, ECMR_PMDE, 0);
> > +
> > +       /* The device need to be reset to restore MagicPacket logic
> > +        * for next wakeup. If we close and open the device it will
> > +        * both be reset and all registers restored. This is what
> > +        * happens during suspend and resume without WoL enabled.
> > +        */
> > +       ret = sh_eth_close(ndev);
> > +       if (ret < 0)
> > +               return ret;
> > +       ret = sh_eth_open(ndev);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       /* Restore clock usage count */
> > +       clk_disable(mdp->clk);
> > +
> > +       return disable_irq_wake(ndev->irq);
> > +}
> > +
> >  static int sh_eth_suspend(struct device *dev)
> >  {
> >         struct net_device *ndev = dev_get_drvdata(dev);
> > +       struct sh_eth_private *mdp = netdev_priv(ndev);
> >         int ret = 0;
> >
> > -       if (netif_running(ndev)) {
> > -               netif_device_detach(ndev);
> > +       if (!netif_running(ndev))
> > +               return 0;
> > +
> > +       netif_device_detach(ndev);
> > +
> > +       if (mdp->wol_enabled)
> > +               ret = sh_eth_wol_setup(ndev);
> > +       else
> >                 ret = sh_eth_close(ndev);
> > -       }
> >
> >         return ret;
> >  }
> > @@ -3166,14 +3255,21 @@ static int sh_eth_suspend(struct device *dev)
> >  static int sh_eth_resume(struct device *dev)
> >  {
> >         struct net_device *ndev = dev_get_drvdata(dev);
> > +       struct sh_eth_private *mdp = netdev_priv(ndev);
> >         int ret = 0;
> >
> > -       if (netif_running(ndev)) {
> > +       if (!netif_running(ndev))
> > +               return 0;
> > +
> > +       if (mdp->wol_enabled)
> > +               ret = sh_eth_wol_restore(ndev);
> > +       else
> >                 ret = sh_eth_open(ndev);
> > -               if (ret < 0)
> > -                       return ret;
> > -               netif_device_attach(ndev);
> > -       }
> > +
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       netif_device_attach(ndev);
> >
> >         return ret;
> >  }
> > diff --git a/drivers/net/ethernet/renesas/sh_eth.h b/drivers/net/ethernet/renesas/sh_eth.h
> > index d050f37..4ceed00 100644
> > --- a/drivers/net/ethernet/renesas/sh_eth.h
> > +++ b/drivers/net/ethernet/renesas/sh_eth.h
> > @@ -493,6 +493,7 @@ struct sh_eth_cpu_data {
> >         unsigned shift_rd0:1;   /* shift Rx descriptor word 0 right by 16 */
> >         unsigned rmiimode:1;    /* EtherC has RMIIMODE register */
> >         unsigned rtrate:1;      /* EtherC has RTRATE register */
> > +       unsigned magic:1;       /* EtherC has ECMR.PMDE and ECSR.MPD */
> >  };
> >
> >  struct sh_eth_private {
> > @@ -501,6 +502,7 @@ struct sh_eth_private {
> >         const u16 *reg_offset;
> >         void __iomem *addr;
> >         void __iomem *tsu_addr;
> > +       struct clk *clk;
> >         u32 num_rx_ring;
> >         u32 num_tx_ring;
> >         dma_addr_t rx_desc_dma;
> > @@ -529,6 +531,7 @@ struct sh_eth_private {
> >         unsigned no_ether_link:1;
> >         unsigned ether_link_active_low:1;
> >         unsigned is_opened:1;
> > +       unsigned wol_enabled:1;
> >  };
> >
> >  static inline void sh_eth_soft_swap(char *src, int len)
> > --
> > 2.10.2
> 
> 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

Re: [PATCHv2 2/5] sh_eth: enable wake-on-lan for Gen2 devices

[Sergei Shtylyov]

Hello!

    You forgot "R-Car" before "Gen2" in the subject.

On 12/12/2016 07:09 PM, Niklas Söderlund wrote:

> Tested on Gen2 r8a7791/Koelsch.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/net/ethernet/renesas/sh_eth.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> index 87640b9..348ed22 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> @@ -624,8 +624,9 @@ static struct sh_eth_cpu_data r8a779x_data = {
>
>  	.register_type	= SH_ETH_REG_FAST_RCAR,
>
> -	.ecsr_value	= ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD,
> -	.ecsipr_value	= ECSIPR_PSRTOIP | ECSIPR_LCHNGIP | ECSIPR_ICDIP,
> +	.ecsr_value	= ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD | ECSR_MPD,
> +	.ecsipr_value	= ECSIPR_PSRTOIP | ECSIPR_LCHNGIP | ECSIPR_ICDIP |
> +			  ECSIPR_MPDIP,

   These expressions seem to have been sorted by the bit # before your patch, 
now they aren't... care to fix? :-)

[...]

MBR, Sergei

Re: [PATCHv2 1/5] sh_eth: add generic wake-on-lan support via magic packet

[Sergei Shtylyov]

Hello!

On 12/12/2016 07:09 PM, Niklas Söderlund wrote:

    Not the complete review yet, just some superficial comments.

> Add generic functionality to support Wake-on-Lan using MagicPacket which

    LAN.

> are supported by at least a few versions of sh_eth. Only add
> functionality for WoL, no specific sh_eth version are marked to support
> WoL yet.

[...]

> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> index 05b0dc5..87640b9 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
[...]
> @@ -3150,15 +3187,67 @@ static int sh_eth_drv_remove(struct platform_device *pdev)
[...]
> +static int sh_eth_wol_restore(struct net_device *ndev)
> +{
> +	struct sh_eth_private *mdp = netdev_priv(ndev);
> +	int ret;
> +
> +	napi_enable(&mdp->napi);
> +
> +	/* Disable MagicPacket */
> +	sh_eth_modify(ndev, ECMR, ECMR_PMDE, 0);
> +
> +	/* The device need to be reset to restore MagicPacket logic

    Needs.

[...]
> index d050f37..4ceed00 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.h
> +++ b/drivers/net/ethernet/renesas/sh_eth.h
> @@ -493,6 +493,7 @@ struct sh_eth_cpu_data {
>  	unsigned shift_rd0:1;	/* shift Rx descriptor word 0 right by 16 */
>  	unsigned rmiimode:1;	/* EtherC has RMIIMODE register */
>  	unsigned rtrate:1;	/* EtherC has RTRATE register */
> +	unsigned magic:1;	/* EtherC has ECMR.PMDE and ECSR.MPD */

    After looking at the SH7734/63 manuals it became obvious that PMDE was a 
result of typo, the bit is called MPDE actually, the current name doesn't make 
sense anyway. Care to fix?

MBR, Sergei

Re: [PATCHv2 2/5] sh_eth: enable wake-on-lan for Gen2 devices

[Sergei Shtylyov]

On 12/14/2016 04:37 PM, Sergei Shtylyov wrote:

>> Tested on Gen2 r8a7791/Koelsch.
>>
>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>> ---
>>  drivers/net/ethernet/renesas/sh_eth.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/renesas/sh_eth.c
>> b/drivers/net/ethernet/renesas/sh_eth.c
>> index 87640b9..348ed22 100644
>> --- a/drivers/net/ethernet/renesas/sh_eth.c
>> +++ b/drivers/net/ethernet/renesas/sh_eth.c
>> @@ -624,8 +624,9 @@ static struct sh_eth_cpu_data r8a779x_data = {
>>
>>      .register_type    = SH_ETH_REG_FAST_RCAR,
>>
>> -    .ecsr_value    = ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD,
>> -    .ecsipr_value    = ECSIPR_PSRTOIP | ECSIPR_LCHNGIP | ECSIPR_ICDIP,
>> +    .ecsr_value    = ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD | ECSR_MPD,
>> +    .ecsipr_value    = ECSIPR_PSRTOIP | ECSIPR_LCHNGIP | ECSIPR_ICDIP |
>> +              ECSIPR_MPDIP,
>
>   These expressions seem to have been sorted by the bit # before your patch,
> now they aren't... care to fix? :-)

    After looking at the SH7743/64 code, nevermind this request. Sorry. :-)

MBR, Sergei

Re: [PATCHv2 1/5] sh_eth: add generic wake-on-lan support via magic packet

[Sergei Shtylyov]

Hello.

On 12/12/2016 07:09 PM, Niklas Söderlund wrote:

> Add generic functionality to support Wake-on-Lan using MagicPacket which
> are supported by at least a few versions of sh_eth. Only add
> functionality for WoL, no specific sh_eth version are marked to support

    Versions.

> WoL yet.
>
> WoL is enabled in the suspend callback by setting MagicPacket detection
> and disabling all interrupts expect MagicPacket. In the resume path the
> driver needs to reset the hardware to rearm the WoL logic, this prevents
> the driver from simply restoring the registers and to take advantage of
> that sh_eth was not suspended to reduce resume time. To reset the
> hardware the driver close and reopens the device just like it would do

    Closes.

> in a normal suspend/resume scenario without WoL enabled, but it both
> close and open the device in the resume callback since the device needs

    Closes and opens.

> to be open for WoL to work.

> One quirk needed for WoL is that the module clock needs to be prevented
> from being switched off by Runtime PM. To keep the clock alive the

    I tried to find the code in question and failed, getting muddled in the 
RPM maze. Could you point at this code for my education? :-)

> suspend callback need to call clk_enable() directly to increase the

    My main concern is why we need to manipulate the clock directly --
can't you call RPM to achieve the same effect?

> usage count of the clock. Then when Runtime PM decreases the clock usage
> count it won't reach 0 and be switched off.

    You mean it does this even though we don't call pr_runtime_put_sync()
as done in sh_eth_close()?

> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
[...]

MBR, Sergei

Re: [PATCHv2 1/5] sh_eth: add generic wake-on-lan support via magic packet

[Niklas Söderlund]

Hi Sergei,

Thanks for the spelling feedback, will include your suggestions in v3.
Which I hope to post once rc1 is released and netdev opens, as you 
suggested to me previously.

On 2016-12-18 23:26:11 +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 12/12/2016 07:09 PM, Niklas Söderlund wrote:
> 
> > Add generic functionality to support Wake-on-Lan using MagicPacket which
> > are supported by at least a few versions of sh_eth. Only add
> > functionality for WoL, no specific sh_eth version are marked to support
> 
>    Versions.
> 
> > WoL yet.
> > 
> > WoL is enabled in the suspend callback by setting MagicPacket detection
> > and disabling all interrupts expect MagicPacket. In the resume path the
> > driver needs to reset the hardware to rearm the WoL logic, this prevents
> > the driver from simply restoring the registers and to take advantage of
> > that sh_eth was not suspended to reduce resume time. To reset the
> > hardware the driver close and reopens the device just like it would do
> 
>    Closes.
> 
> > in a normal suspend/resume scenario without WoL enabled, but it both
> > close and open the device in the resume callback since the device needs
> 
>    Closes and opens.
> 
> > to be open for WoL to work.
> 
> > One quirk needed for WoL is that the module clock needs to be prevented
> > from being switched off by Runtime PM. To keep the clock alive the
> 
>    I tried to find the code in question and failed, getting muddled in the
> RPM maze. Could you point at this code for my education? :-)

In my investigation I observed this (simplified) call graph with regards
to clocks for suspend:

pm_suspend
  pm_clk_suspend
    clk_disable
      clk_core_disable
        cpg_mstp_clock_disable

The interesting function here are clk_core_disable(). In that function a
'enable_count' for each clock is decremented and the clock is only
turned of if the count reaches zero, hence cpg_mstp_clock_disable() are
only called if the counter reaches 0. At runtime the enable_count can be
displayed by examining /sys/kernel/debug/clk/clk_summary.

However the value for enable_count show at runtime are not the values
which are used when the pm_clk_suspend() are called. For a clock to not
be switched off when suspending the enable_count needs to incremented,
which a few drivers do if they can be used as a wake-up source.

> 
> > suspend callback need to call clk_enable() directly to increase the
> 
>    My main concern is why we need to manipulate the clock directly --
> can't you call RPM to achieve the same effect?

In my early attempts to keep the clock alive when suspending I used 
pm_clk_resume()/pm_clk_suspend() to increment/decrement the clock usage 
count. pm_clk_resume()/pm_clk_suspend() calls clk_enable()/clk_disable() 
for all clocks in the device's PM clock list, among other things.

Geert pointed out to me that this might have side effects and to manages 
its clock directly is preferred. Looking how these functions are used at 
other places I can only agree with Geert that this feels like the wrong 
solution and a layer violation.

> 
> > usage count of the clock. Then when Runtime PM decreases the clock usage
> > count it won't reach 0 and be switched off.
> 
>    You mean it does this even though we don't call pr_runtime_put_sync()
> as done in sh_eth_close()?

Yes.

I had a look at the pm_runtime_* functions in include/linux/pm_runtime.h 
and drivers/base/power/runtime.c and could not find any clock handling.  
Maybe they only deal with power domains?

I might have missed something when looking in to this. But if I do not 
call clk_enable()/clk_disable() (or something equivalent) in the sh_eth 
suspend/resume callbacks WoL do not work. That is it suspends fine but 
sending a MagicPacket do not wake the system :-)

> 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> [...]
> 
> MBR, Sergei
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCHv2 1/5] sh_eth: add generic wake-on-lan support via magic packet

[Niklas Söderlund]

Hi Sergei,

Thanks for the spelling feedback, will include your suggestions in v3.
Which I hope to post once rc1 is released and netdev opens, as you 
suggested to me previously.

On 2016-12-18 23:26:11 +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 12/12/2016 07:09 PM, Niklas S�derlund wrote:
> 
> > Add generic functionality to support Wake-on-Lan using MagicPacket which
> > are supported by at least a few versions of sh_eth. Only add
> > functionality for WoL, no specific sh_eth version are marked to support
> 
>    Versions.
> 
> > WoL yet.
> > 
> > WoL is enabled in the suspend callback by setting MagicPacket detection
> > and disabling all interrupts expect MagicPacket. In the resume path the
> > driver needs to reset the hardware to rearm the WoL logic, this prevents
> > the driver from simply restoring the registers and to take advantage of
> > that sh_eth was not suspended to reduce resume time. To reset the
> > hardware the driver close and reopens the device just like it would do
> 
>    Closes.
> 
> > in a normal suspend/resume scenario without WoL enabled, but it both
> > close and open the device in the resume callback since the device needs
> 
>    Closes and opens.
> 
> > to be open for WoL to work.
> 
> > One quirk needed for WoL is that the module clock needs to be prevented
> > from being switched off by Runtime PM. To keep the clock alive the
> 
>    I tried to find the code in question and failed, getting muddled in the
> RPM maze. Could you point at this code for my education? :-)

In my investigation I observed this (simplified) call graph with regards
to clocks for suspend:

pm_suspend
  pm_clk_suspend
    clk_disable
      clk_core_disable
        cpg_mstp_clock_disable

The interesting function here are clk_core_disable(). In that function a
'enable_count' for each clock is decremented and the clock is only
turned of if the count reaches zero, hence cpg_mstp_clock_disable() are
only called if the counter reaches 0. At runtime the enable_count can be
displayed by examining /sys/kernel/debug/clk/clk_summary.

However the value for enable_count show at runtime are not the values
which are used when the pm_clk_suspend() are called. For a clock to not
be switched off when suspending the enable_count needs to incremented,
which a few drivers do if they can be used as a wake-up source.

> 
> > suspend callback need to call clk_enable() directly to increase the
> 
>    My main concern is why we need to manipulate the clock directly --
> can't you call RPM to achieve the same effect?

In my early attempts to keep the clock alive when suspending I used 
pm_clk_resume()/pm_clk_suspend() to increment/decrement the clock usage 
count. pm_clk_resume()/pm_clk_suspend() calls clk_enable()/clk_disable() 
for all clocks in the device's PM clock list, among other things.

Geert pointed out to me that this might have side effects and to manages 
its clock directly is preferred. Looking how these functions are used at 
other places I can only agree with Geert that this feels like the wrong 
solution and a layer violation.

> 
> > usage count of the clock. Then when Runtime PM decreases the clock usage
> > count it won't reach 0 and be switched off.
> 
>    You mean it does this even though we don't call pr_runtime_put_sync()
> as done in sh_eth_close()?

Yes.

I had a look at the pm_runtime_* functions in include/linux/pm_runtime.h 
and drivers/base/power/runtime.c and could not find any clock handling.  
Maybe they only deal with power domains?

I might have missed something when looking in to this. But if I do not 
call clk_enable()/clk_disable() (or something equivalent) in the sh_eth 
suspend/resume callbacks WoL do not work. That is it suspends fine but 
sending a MagicPacket do not wake the system :-)

> 
> > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> [...]
> 
> MBR, Sergei
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCHv2 1/5] sh_eth: add generic wake-on-lan support via magic packet

[Niklas Söderlund]

Hi Sergei,

On 2016-12-18 00:50:59 +0300, Sergei Shtylyov wrote:
> Hello!
> 
> On 12/12/2016 07:09 PM, Niklas Söderlund wrote:
> 
>    Not the complete review yet, just some superficial comments.
> 
> > Add generic functionality to support Wake-on-Lan using MagicPacket which
> 
>    LAN.
> 
> > are supported by at least a few versions of sh_eth. Only add
> > functionality for WoL, no specific sh_eth version are marked to support
> > WoL yet.
> 
> [...]
> 
> > diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> > index 05b0dc5..87640b9 100644
> > --- a/drivers/net/ethernet/renesas/sh_eth.c
> > +++ b/drivers/net/ethernet/renesas/sh_eth.c
> [...]
> > @@ -3150,15 +3187,67 @@ static int sh_eth_drv_remove(struct platform_device *pdev)
> [...]
> > +static int sh_eth_wol_restore(struct net_device *ndev)
> > +{
> > +	struct sh_eth_private *mdp = netdev_priv(ndev);
> > +	int ret;
> > +
> > +	napi_enable(&mdp->napi);
> > +
> > +	/* Disable MagicPacket */
> > +	sh_eth_modify(ndev, ECMR, ECMR_PMDE, 0);
> > +
> > +	/* The device need to be reset to restore MagicPacket logic
> 
>    Needs.
> 
> [...]
> > index d050f37..4ceed00 100644
> > --- a/drivers/net/ethernet/renesas/sh_eth.h
> > +++ b/drivers/net/ethernet/renesas/sh_eth.h
> > @@ -493,6 +493,7 @@ struct sh_eth_cpu_data {
> >  	unsigned shift_rd0:1;	/* shift Rx descriptor word 0 right by 16 */
> >  	unsigned rmiimode:1;	/* EtherC has RMIIMODE register */
> >  	unsigned rtrate:1;	/* EtherC has RTRATE register */
> > +	unsigned magic:1;	/* EtherC has ECMR.PMDE and ECSR.MPD */
> 
>    After looking at the SH7734/63 manuals it became obvious that PMDE was a
> result of typo, the bit is called MPDE actually, the current name doesn't
> make sense anyway. Care to fix?

Sure, I add fixup patch to the front of this series which cleans this up 
before adding WoL support. Or would you prefer I sent a separate patch 
and have the WoL series depend on it?

> 
> MBR, Sergei
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCHv2 1/5] sh_eth: add generic wake-on-lan support via magic packet

[Niklas Söderlund]

Hi Sergei,

On 2016-12-18 00:50:59 +0300, Sergei Shtylyov wrote:
> Hello!
> 
> On 12/12/2016 07:09 PM, Niklas S�derlund wrote:
> 
>    Not the complete review yet, just some superficial comments.
> 
> > Add generic functionality to support Wake-on-Lan using MagicPacket which
> 
>    LAN.
> 
> > are supported by at least a few versions of sh_eth. Only add
> > functionality for WoL, no specific sh_eth version are marked to support
> > WoL yet.
> 
> [...]
> 
> > diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> > index 05b0dc5..87640b9 100644
> > --- a/drivers/net/ethernet/renesas/sh_eth.c
> > +++ b/drivers/net/ethernet/renesas/sh_eth.c
> [...]
> > @@ -3150,15 +3187,67 @@ static int sh_eth_drv_remove(struct platform_device *pdev)
> [...]
> > +static int sh_eth_wol_restore(struct net_device *ndev)
> > +{
> > +	struct sh_eth_private *mdp = netdev_priv(ndev);
> > +	int ret;
> > +
> > +	napi_enable(&mdp->napi);
> > +
> > +	/* Disable MagicPacket */
> > +	sh_eth_modify(ndev, ECMR, ECMR_PMDE, 0);
> > +
> > +	/* The device need to be reset to restore MagicPacket logic
> 
>    Needs.
> 
> [...]
> > index d050f37..4ceed00 100644
> > --- a/drivers/net/ethernet/renesas/sh_eth.h
> > +++ b/drivers/net/ethernet/renesas/sh_eth.h
> > @@ -493,6 +493,7 @@ struct sh_eth_cpu_data {
> >  	unsigned shift_rd0:1;	/* shift Rx descriptor word 0 right by 16 */
> >  	unsigned rmiimode:1;	/* EtherC has RMIIMODE register */
> >  	unsigned rtrate:1;	/* EtherC has RTRATE register */
> > +	unsigned magic:1;	/* EtherC has ECMR.PMDE and ECSR.MPD */
> 
>    After looking at the SH7734/63 manuals it became obvious that PMDE was a
> result of typo, the bit is called MPDE actually, the current name doesn't
> make sense anyway. Care to fix?

Sure, I add fixup patch to the front of this series which cleans this up 
before adding WoL support. Or would you prefer I sent a separate patch 
and have the WoL series depend on it?

> 
> MBR, Sergei
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCHv2 1/5] sh_eth: add generic wake-on-lan support via magic packet

[Sergei Shtylyov]

Hello!

On 12/19/2016 07:41 PM, Niklas Söderlund wrote:

[...]

>>> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
>>> index 05b0dc5..87640b9 100644
>>> --- a/drivers/net/ethernet/renesas/sh_eth.c
>>> +++ b/drivers/net/ethernet/renesas/sh_eth.c
[...]
>>> index d050f37..4ceed00 100644
>>> --- a/drivers/net/ethernet/renesas/sh_eth.h
>>> +++ b/drivers/net/ethernet/renesas/sh_eth.h
>>> @@ -493,6 +493,7 @@ struct sh_eth_cpu_data {
>>>  	unsigned shift_rd0:1;	/* shift Rx descriptor word 0 right by 16 */
>>>  	unsigned rmiimode:1;	/* EtherC has RMIIMODE register */
>>>  	unsigned rtrate:1;	/* EtherC has RTRATE register */
>>> +	unsigned magic:1;	/* EtherC has ECMR.PMDE and ECSR.MPD */
>>
>>    After looking at the SH7734/63 manuals it became obvious that PMDE was a
>> result of typo, the bit is called MPDE actually, the current name doesn't
>> make sense anyway. Care to fix?
>
> Sure, I add fixup patch to the front of this series which cleans this up
> before adding WoL support.

    That would be fine, TIA!

[...]

MBR, Sergei

Re: [PATCHv2 1/5] sh_eth: add generic wake-on-lan support via magic packet

[Geert Uytterhoeven]

Hi Niklas,

On Mon, Dec 19, 2016 at 5:39 PM, Niklas Söderlund
<niklas.soderlund@ragnatech.se> wrote:
> On 2016-12-18 23:26:11 +0300, Sergei Shtylyov wrote:
>> On 12/12/2016 07:09 PM, Niklas Söderlund wrote:
>> > One quirk needed for WoL is that the module clock needs to be prevented
>> > from being switched off by Runtime PM. To keep the clock alive the
>>
>>    I tried to find the code in question and failed, getting muddled in the
>> RPM maze. Could you point at this code for my education? :-)
>
> In my investigation I observed this (simplified) call graph with regards
> to clocks for suspend:
>
> pm_suspend
>   pm_clk_suspend
>     clk_disable
>       clk_core_disable
>         cpg_mstp_clock_disable
>
> The interesting function here are clk_core_disable(). In that function a
> 'enable_count' for each clock is decremented and the clock is only
> turned of if the count reaches zero, hence cpg_mstp_clock_disable() are
> only called if the counter reaches 0. At runtime the enable_count can be
> displayed by examining /sys/kernel/debug/clk/clk_summary.
>
> However the value for enable_count show at runtime are not the values
> which are used when the pm_clk_suspend() are called. For a clock to not
> be switched off when suspending the enable_count needs to incremented,
> which a few drivers do if they can be used as a wake-up source.
>
>>
>> > suspend callback need to call clk_enable() directly to increase the
>>
>>    My main concern is why we need to manipulate the clock directly --
>> can't you call RPM to achieve the same effect?
>
> In my early attempts to keep the clock alive when suspending I used
> pm_clk_resume()/pm_clk_suspend() to increment/decrement the clock usage
> count. pm_clk_resume()/pm_clk_suspend() calls clk_enable()/clk_disable()
> for all clocks in the device's PM clock list, among other things.
>
> Geert pointed out to me that this might have side effects and to manages
> its clock directly is preferred. Looking how these functions are used at
> other places I can only agree with Geert that this feels like the wrong
> solution and a layer violation.

>> > usage count of the clock. Then when Runtime PM decreases the clock usage
>> > count it won't reach 0 and be switched off.
>>
>>    You mean it does this even though we don't call pr_runtime_put_sync()
>> as done in sh_eth_close()?
>
> Yes.
>
> I had a look at the pm_runtime_* functions in include/linux/pm_runtime.h
> and drivers/base/power/runtime.c and could not find any clock handling.
> Maybe they only deal with power domains?

There should be a generic way to prevent a device from being suspended.
This will make sure the module clock is not disabled, and the power domain
(if applicable) is not powered down.

Note that the clock handling is a special form of power domain handling,
as it uses a clock domain.

Gr{oetje,eeting}s,

                        Geert

Re: [PATCHv2 1/5] sh_eth: add generic wake-on-lan support via magic packet

[Geert Uytterhoeven]

Hi Niklas,

On Mon, Dec 19, 2016 at 5:39 PM, Niklas Söderlund
<niklas.soderlund@ragnatech.se> wrote:
> On 2016-12-18 23:26:11 +0300, Sergei Shtylyov wrote:
>> On 12/12/2016 07:09 PM, Niklas Söderlund wrote:
>> > One quirk needed for WoL is that the module clock needs to be prevented
>> > from being switched off by Runtime PM. To keep the clock alive the
>>
>>    I tried to find the code in question and failed, getting muddled in the
>> RPM maze. Could you point at this code for my education? :-)
>
> In my investigation I observed this (simplified) call graph with regards
> to clocks for suspend:
>
> pm_suspend
>   pm_clk_suspend
>     clk_disable
>       clk_core_disable
>         cpg_mstp_clock_disable
>
> The interesting function here are clk_core_disable(). In that function a
> 'enable_count' for each clock is decremented and the clock is only
> turned of if the count reaches zero, hence cpg_mstp_clock_disable() are
> only called if the counter reaches 0. At runtime the enable_count can be
> displayed by examining /sys/kernel/debug/clk/clk_summary.
>
> However the value for enable_count show at runtime are not the values
> which are used when the pm_clk_suspend() are called. For a clock to not
> be switched off when suspending the enable_count needs to incremented,
> which a few drivers do if they can be used as a wake-up source.
>
>>
>> > suspend callback need to call clk_enable() directly to increase the
>>
>>    My main concern is why we need to manipulate the clock directly --
>> can't you call RPM to achieve the same effect?
>
> In my early attempts to keep the clock alive when suspending I used
> pm_clk_resume()/pm_clk_suspend() to increment/decrement the clock usage
> count. pm_clk_resume()/pm_clk_suspend() calls clk_enable()/clk_disable()
> for all clocks in the device's PM clock list, among other things.
>
> Geert pointed out to me that this might have side effects and to manages
> its clock directly is preferred. Looking how these functions are used at
> other places I can only agree with Geert that this feels like the wrong
> solution and a layer violation.

>> > usage count of the clock. Then when Runtime PM decreases the clock usage
>> > count it won't reach 0 and be switched off.
>>
>>    You mean it does this even though we don't call pr_runtime_put_sync()
>> as done in sh_eth_close()?
>
> Yes.
>
> I had a look at the pm_runtime_* functions in include/linux/pm_runtime.h
> and drivers/base/power/runtime.c and could not find any clock handling.
> Maybe they only deal with power domains?

There should be a generic way to prevent a device from being suspended.
This will make sure the module clock is not disabled, and the power domain
(if applicable) is not powered down.

Note that the clock handling is a special form of power domain handling,
as it uses a clock domain.

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: [PATCHv2 1/5] sh_eth: add generic wake-on-lan support via magic packet

[Sergei Shtylyov]

Hello!

On 12/19/2016 08:11 PM, Geert Uytterhoeven wrote:

>>>> One quirk needed for WoL is that the module clock needs to be prevented
>>>> from being switched off by Runtime PM. To keep the clock alive the
>>>
>>>    I tried to find the code in question and failed, getting muddled in the
>>> RPM maze. Could you point at this code for my education? :-)
>>
>> In my investigation I observed this (simplified) call graph with regards
>> to clocks for suspend:
>>
>> pm_suspend

    There's a long list of the calls skipped here. :-)

>>   pm_clk_suspend
>>     clk_disable
>>       clk_core_disable
>>         cpg_mstp_clock_disable
>>
>> The interesting function here are clk_core_disable(). In that function a
>> 'enable_count' for each clock is decremented and the clock is only
>> turned of if the count reaches zero, hence cpg_mstp_clock_disable() are
>> only called if the counter reaches 0. At runtime the enable_count can be
>> displayed by examining /sys/kernel/debug/clk/clk_summary.

    Well, this is not new to me... it's more interesting how we get there... :-)

[...]
>>>> usage count of the clock. Then when Runtime PM decreases the clock usage
>>>> count it won't reach 0 and be switched off.
>>>
>>>    You mean it does this even though we don't call pr_runtime_put_sync()
>>> as done in sh_eth_close()?
>>
>> Yes.
>>
>> I had a look at the pm_runtime_* functions in include/linux/pm_runtime.h
>> and drivers/base/power/runtime.c and could not find any clock handling.
>> Maybe they only deal with power domains?
>
> There should be a generic way to prevent a device from being suspended.

    Indeed.

> This will make sure the module clock is not disabled, and the power domain
> (if applicable) is not powered down.

    I've just bumped into <linux/pm_wakeirq.h>, it looks promising...

[...]
> Gr{oetje,eeting}s,
>
>                         Geert

MBR, Sergei