[PATCH v2] ravb: add wake-on-lan support via magic packet

[PATCH v2] ravb: add wake-on-lan support via magic packet

[Niklas Söderlund]

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 ravb was not suspended to reduce resume time. To reset the
hardware the driver closes the device, sets it in reset mode and reopens
the device just like it would do in a normal suspend/resume scenario
without WoL enabled, but it both closes and opens the device in the
resume callback since the device needs to be reset 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/ravb.h      |   2 +
 drivers/net/ethernet/renesas/ravb_main.c | 130 ++++++++++++++++++++++++++++++-
 2 files changed, 128 insertions(+), 4 deletions(-)

Changes from v1
- Fix issue where device would fail to resume from PSCI suspend if WoL 
  was enabled, reported by Geert. The fault was that the clock driver 
  thinks the clock is on, but PSCI have disabled it, added workaround 
  for this in ravb driver which can be removed once the clock driver is 
  aware of the PSCI behavior.
- Only try to restore from wol wake up if netif is running, since this 
  is a condition to enable wol in the first place this was a bug in v1.


diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 0525bd696d5d02e5..96a27b00c90e212a 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -991,6 +991,7 @@ struct ravb_private {
 	struct net_device *ndev;
 	struct platform_device *pdev;
 	void __iomem *addr;
+	struct clk *clk;
 	struct mdiobb_ctrl mdiobb;
 	u32 num_rx_ring[NUM_RX_QUEUE];
 	u32 num_tx_ring[NUM_TX_QUEUE];
@@ -1033,6 +1034,7 @@ struct ravb_private {
 
 	unsigned no_avb_link:1;
 	unsigned avb_link_active_low:1;
+	unsigned wol_enabled:1;
 };
 
 static inline u32 ravb_read(struct net_device *ndev, enum ravb_reg reg)
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 5931e859876c2aee..3d399f85417a83cf 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -680,6 +680,9 @@ static void ravb_emac_interrupt_unlocked(struct net_device *ndev)
 
 	ecsr = ravb_read(ndev, ECSR);
 	ravb_write(ndev, ecsr, ECSR);	/* clear interrupt */
+
+	if (ecsr & ECSR_MPD)
+		pm_wakeup_event(&priv->pdev->dev, 0);
 	if (ecsr & ECSR_ICD)
 		ndev->stats.tx_carrier_errors++;
 	if (ecsr & ECSR_LCHNG) {
@@ -1330,6 +1333,33 @@ static int ravb_get_ts_info(struct net_device *ndev,
 	return 0;
 }
 
+static void ravb_get_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	wol->supported = 0;
+	wol->wolopts = 0;
+
+	if (priv->clk) {
+		wol->supported = WAKE_MAGIC;
+		wol->wolopts = priv->wol_enabled ? WAKE_MAGIC : 0;
+	}
+}
+
+static int ravb_set_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	if (!priv->clk || wol->wolopts & ~WAKE_MAGIC)
+		return -EOPNOTSUPP;
+
+	priv->wol_enabled = !!(wol->wolopts & WAKE_MAGIC);
+
+	device_set_wakeup_enable(&priv->pdev->dev, priv->wol_enabled);
+
+	return 0;
+}
+
 static const struct ethtool_ops ravb_ethtool_ops = {
 	.nway_reset		= ravb_nway_reset,
 	.get_msglevel		= ravb_get_msglevel,
@@ -1343,6 +1373,8 @@ static const struct ethtool_ops ravb_ethtool_ops = {
 	.get_ts_info		= ravb_get_ts_info,
 	.get_link_ksettings	= ravb_get_link_ksettings,
 	.set_link_ksettings	= ravb_set_link_ksettings,
+	.get_wol		= ravb_get_wol,
+	.set_wol		= ravb_set_wol,
 };
 
 static inline int ravb_hook_irq(unsigned int irq, irq_handler_t handler,
@@ -2041,6 +2073,11 @@ static int ravb_probe(struct platform_device *pdev)
 
 	priv->chip_id = chip_id;
 
+	/* Get clock, if not found that's OK but Wake-On-Lan is unavailable */
+	priv->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(priv->clk))
+		priv->clk = NULL;
+
 	/* Set function */
 	ndev->netdev_ops = &ravb_netdev_ops;
 	ndev->ethtool_ops = &ravb_ethtool_ops;
@@ -2107,6 +2144,9 @@ static int ravb_probe(struct platform_device *pdev)
 	if (error)
 		goto out_napi_del;
 
+	if (priv->clk)
+		device_set_wakeup_capable(&pdev->dev, 1);
+
 	/* Print device information */
 	netdev_info(ndev, "Base address at %#x, %pM, IRQ %d.\n",
 		    (u32)ndev->base_addr, ndev->dev_addr, ndev->irq);
@@ -2160,15 +2200,66 @@ static int ravb_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int ravb_wol_setup(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	/* Disable interrupts by clearing the interrupt masks. */
+	ravb_write(ndev, 0, RIC0);
+	ravb_write(ndev, 0, RIC2);
+	ravb_write(ndev, 0, TIC);
+
+	/* Only allow ECI interrupts */
+	synchronize_irq(priv->emac_irq);
+	napi_disable(&priv->napi[RAVB_NC]);
+	napi_disable(&priv->napi[RAVB_BE]);
+	ravb_write(ndev, ECSIPR_MPDIP, ECSIPR);
+
+	/* Enable MagicPacket */
+	ravb_modify(ndev, ECMR, ECMR_MPDE, ECMR_MPDE);
+
+	/* Increased clock usage so device won't be suspended */
+	clk_enable(priv->clk);
+
+	return enable_irq_wake(priv->emac_irq);
+}
+
+static int ravb_wol_restore(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	int ret;
+
+	napi_enable(&priv->napi[RAVB_NC]);
+	napi_enable(&priv->napi[RAVB_BE]);
+
+	/* Disable MagicPacket */
+	ravb_modify(ndev, ECMR, ECMR_MPDE, 0);
+
+	ret = ravb_close(ndev);
+	if (ret < 0)
+		return ret;
+
+	/* Restore clock usage count */
+	clk_disable(priv->clk);
+
+	return disable_irq_wake(priv->emac_irq);
+}
+
 static int __maybe_unused ravb_suspend(struct device *dev)
 {
 	struct net_device *ndev = dev_get_drvdata(dev);
-	int ret = 0;
+	struct ravb_private *priv = netdev_priv(ndev);
+	int ret;
 
-	if (netif_running(ndev)) {
-		netif_device_detach(ndev);
+	if (!netif_running(ndev))
+		return 0;
+
+	netif_device_detach(ndev);
+
+	if (priv->wol_enabled)
+		ret = ravb_wol_setup(ndev);
+	else
 		ret = ravb_close(ndev);
-	}
 
 	return ret;
 }
@@ -2179,6 +2270,32 @@ static int __maybe_unused ravb_resume(struct device *dev)
 	struct ravb_private *priv = netdev_priv(ndev);
 	int ret = 0;
 
+	/* Reduce the usecount of the clock to zero and then
+	 * restore it to its original value. This is done to force
+	 * the clock to be re-enabled which is a workaround
+	 * for renesas-cpg-mssr driver which do not enable clocks
+	 * when resuming from PSCI suspend/resume.
+	 *
+	 * Without this workaround the driver fails to communicate
+	 * with the hardware if WoL was enabled when the system
+	 * entered PSCI suspend. This is due to that if WoL is enabled
+	 * we explicitly keep the clock from being turned off when
+	 * suspending, but in PSCI sleep power is cut so the clock
+	 * is disabled anyhow, the clock driver is not aware of this
+	 * so the clock is not turned back on when resuming.
+	 *
+	 * TODO: once the renesas-cpg-mssr suspend/resume is working
+	 *       this clock dance should be removed.
+	 */
+	clk_disable(priv->clk);
+	clk_disable(priv->clk);
+	clk_enable(priv->clk);
+	clk_enable(priv->clk);
+
+	/* If WoL is enabled set reset mode to rearm the WoL logic */
+	if (priv->wol_enabled)
+		ravb_write(ndev, CCC_OPC_RESET, CCC);
+
 	/* 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.
@@ -2202,6 +2319,11 @@ static int __maybe_unused ravb_resume(struct device *dev)
 	ravb_write(ndev, priv->desc_bat_dma, DBAT);
 
 	if (netif_running(ndev)) {
+		if (priv->wol_enabled) {
+			ret = ravb_wol_restore(ndev);
+			if (ret)
+				return ret;
+		}
 		ret = ravb_open(ndev);
 		if (ret < 0)
 			return ret;
-- 
2.13.3

Re: [PATCH v2] ravb: add wake-on-lan support via magic packet

[Andrew Lunn]

Hi Niklas

> @@ -2041,6 +2073,11 @@ static int ravb_probe(struct platform_device *pdev)
>  
>  	priv->chip_id = chip_id;
>  
> +	/* Get clock, if not found that's OK but Wake-On-Lan is unavailable */
> +	priv->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(priv->clk))
> +		priv->clk = NULL;

Can you get EPROBE_DEFER returned?

    Andrew

Re: [PATCH v2] ravb: add wake-on-lan support via magic packet

[Niklas Söderlund]

Hi Andrew,

On 2017-07-30 19:07:38 +0200, Andrew Lunn wrote:
> Hi Niklas
> 
> > @@ -2041,6 +2073,11 @@ static int ravb_probe(struct platform_device *pdev)
> >  
> >  	priv->chip_id = chip_id;
> >  
> > +	/* Get clock, if not found that's OK but Wake-On-Lan is unavailable */
> > +	priv->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(priv->clk))
> > +		priv->clk = NULL;
> 
> Can you get EPROBE_DEFER returned?

I don't think so, but I'm not sure :-)

The clock I'm trying to get is the module clock of the ravb itself, so 
if that clock is not available (and enabled) no register writes to the 
ravb would be possible in the first place, so i guess it's safe to 
assume -EPROBE_DEFER can not happen here?

I'm just trying to play it safe here since the clock is only needed to 
support WoL, I though it best to not change behavior here. Try to get 
the clock, if we can great we can do WoL if not then user-space will be 
prevented from enabling WoL and nothing in the current behavior changes.

> 
>     Andrew

-- 
Regards,
Niklas Söderlund

Re: [PATCH v2] ravb: add wake-on-lan support via magic packet

[Niklas Söderlund]

Hi Andrew,

On 2017-07-30 19:07:38 +0200, Andrew Lunn wrote:
> Hi Niklas
> 
> > @@ -2041,6 +2073,11 @@ static int ravb_probe(struct platform_device *pdev)
> >  
> >  	priv->chip_id = chip_id;
> >  
> > +	/* Get clock, if not found that's OK but Wake-On-Lan is unavailable */
> > +	priv->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(priv->clk))
> > +		priv->clk = NULL;
> 
> Can you get EPROBE_DEFER returned?

I don't think so, but I'm not sure :-)

The clock I'm trying to get is the module clock of the ravb itself, so 
if that clock is not available (and enabled) no register writes to the 
ravb would be possible in the first place, so i guess it's safe to 
assume -EPROBE_DEFER can not happen here?

I'm just trying to play it safe here since the clock is only needed to 
support WoL, I though it best to not change behavior here. Try to get 
the clock, if we can great we can do WoL if not then user-space will be 
prevented from enabling WoL and nothing in the current behavior changes.

> 
>     Andrew

-- 
Regards,
Niklas S�derlund

Re: [PATCH v2] ravb: add wake-on-lan support via magic packet

[Andrew Lunn]

On Sun, Jul 30, 2017 at 09:51:54PM +0200, Niklas Söderlund wrote:
> Hi Andrew,
> 
> On 2017-07-30 19:07:38 +0200, Andrew Lunn wrote:
> > Hi Niklas
> > 
> > > @@ -2041,6 +2073,11 @@ static int ravb_probe(struct platform_device *pdev)
> > >  
> > >  	priv->chip_id = chip_id;
> > >  
> > > +	/* Get clock, if not found that's OK but Wake-On-Lan is unavailable */
> > > +	priv->clk = devm_clk_get(&pdev->dev, NULL);
> > > +	if (IS_ERR(priv->clk))
> > > +		priv->clk = NULL;
> > 
> > Can you get EPROBE_DEFER returned?
> 
> I don't think so, but I'm not sure :-)
> 
> The clock I'm trying to get is the module clock of the ravb itself, so 
> if that clock is not available (and enabled) no register writes to the 
> ravb would be possible in the first place, so i guess it's safe to 
> assume -EPROBE_DEFER can not happen here?
> 
> I'm just trying to play it safe here since the clock is only needed to 
> support WoL, I though it best to not change behavior here. Try to get 
> the clock, if we can great we can do WoL if not then user-space will be 
> prevented from enabling WoL and nothing in the current behavior changes.

Hi Nikls

Well, if it can return -EPROBE_DEFER, it means sometimes WoL will be
avalable and other times not, depending on when the clock driver
probes. However, it sounds like this is the SoCs core clock driver. If
so, it gets loaded very early, so you are safe.

       Andrew

Re: [PATCH v2] ravb: add wake-on-lan support via magic packet

[Andrew Lunn]

On Sun, Jul 30, 2017 at 09:51:54PM +0200, Niklas S�derlund wrote:
> Hi Andrew,
> 
> On 2017-07-30 19:07:38 +0200, Andrew Lunn wrote:
> > Hi Niklas
> > 
> > > @@ -2041,6 +2073,11 @@ static int ravb_probe(struct platform_device *pdev)
> > >  
> > >  	priv->chip_id = chip_id;
> > >  
> > > +	/* Get clock, if not found that's OK but Wake-On-Lan is unavailable */
> > > +	priv->clk = devm_clk_get(&pdev->dev, NULL);
> > > +	if (IS_ERR(priv->clk))
> > > +		priv->clk = NULL;
> > 
> > Can you get EPROBE_DEFER returned?
> 
> I don't think so, but I'm not sure :-)
> 
> The clock I'm trying to get is the module clock of the ravb itself, so 
> if that clock is not available (and enabled) no register writes to the 
> ravb would be possible in the first place, so i guess it's safe to 
> assume -EPROBE_DEFER can not happen here?
> 
> I'm just trying to play it safe here since the clock is only needed to 
> support WoL, I though it best to not change behavior here. Try to get 
> the clock, if we can great we can do WoL if not then user-space will be 
> prevented from enabling WoL and nothing in the current behavior changes.

Hi Nikls

Well, if it can return -EPROBE_DEFER, it means sometimes WoL will be
avalable and other times not, depending on when the clock driver
probes. However, it sounds like this is the SoCs core clock driver. If
so, it gets loaded very early, so you are safe.

       Andrew

Re: [PATCH v2] ravb: add wake-on-lan support via magic packet

[Niklas Söderlund]

Hi Andrew,

On 2017-07-30 22:07:31 +0200, Andrew Lunn wrote:
> On Sun, Jul 30, 2017 at 09:51:54PM +0200, Niklas Söderlund wrote:
> > Hi Andrew,
> > 
> > On 2017-07-30 19:07:38 +0200, Andrew Lunn wrote:
> > > Hi Niklas
> > > 
> > > > @@ -2041,6 +2073,11 @@ static int ravb_probe(struct platform_device *pdev)
> > > >  
> > > >  	priv->chip_id = chip_id;
> > > >  
> > > > +	/* Get clock, if not found that's OK but Wake-On-Lan is unavailable */
> > > > +	priv->clk = devm_clk_get(&pdev->dev, NULL);
> > > > +	if (IS_ERR(priv->clk))
> > > > +		priv->clk = NULL;
> > > 
> > > Can you get EPROBE_DEFER returned?
> > 
> > I don't think so, but I'm not sure :-)
> > 
> > The clock I'm trying to get is the module clock of the ravb itself, so 
> > if that clock is not available (and enabled) no register writes to the 
> > ravb would be possible in the first place, so i guess it's safe to 
> > assume -EPROBE_DEFER can not happen here?
> > 
> > I'm just trying to play it safe here since the clock is only needed to 
> > support WoL, I though it best to not change behavior here. Try to get 
> > the clock, if we can great we can do WoL if not then user-space will be 
> > prevented from enabling WoL and nothing in the current behavior changes.
> 
> Hi Nikls
> 
> Well, if it can return -EPROBE_DEFER, it means sometimes WoL will be
> avalable and other times not, depending on when the clock driver

Ahh I see yes that would be indeed be bad.

> probes. However, it sounds like this is the SoCs core clock driver. If
> so, it gets loaded very early, so you are safe.

Yes this is renesas-cpg-mssr which if I understand things is a core 
clock driver. It is register in drivers/clk/renesas/renesas-cpg-mssr.c 
using:

 subsys_initcall(cpg_mssr_init);

So I take it I'm safe. Thanks however for bringing this to my attention 
I learnt something new today :-)

> 
>        Andrew

-- 
Regards,
Niklas Söderlund

Re: [PATCH v2] ravb: add wake-on-lan support via magic packet

[Niklas Söderlund]

Hi Andrew,

On 2017-07-30 22:07:31 +0200, Andrew Lunn wrote:
> On Sun, Jul 30, 2017 at 09:51:54PM +0200, Niklas S�derlund wrote:
> > Hi Andrew,
> > 
> > On 2017-07-30 19:07:38 +0200, Andrew Lunn wrote:
> > > Hi Niklas
> > > 
> > > > @@ -2041,6 +2073,11 @@ static int ravb_probe(struct platform_device *pdev)
> > > >  
> > > >  	priv->chip_id = chip_id;
> > > >  
> > > > +	/* Get clock, if not found that's OK but Wake-On-Lan is unavailable */
> > > > +	priv->clk = devm_clk_get(&pdev->dev, NULL);
> > > > +	if (IS_ERR(priv->clk))
> > > > +		priv->clk = NULL;
> > > 
> > > Can you get EPROBE_DEFER returned?
> > 
> > I don't think so, but I'm not sure :-)
> > 
> > The clock I'm trying to get is the module clock of the ravb itself, so 
> > if that clock is not available (and enabled) no register writes to the 
> > ravb would be possible in the first place, so i guess it's safe to 
> > assume -EPROBE_DEFER can not happen here?
> > 
> > I'm just trying to play it safe here since the clock is only needed to 
> > support WoL, I though it best to not change behavior here. Try to get 
> > the clock, if we can great we can do WoL if not then user-space will be 
> > prevented from enabling WoL and nothing in the current behavior changes.
> 
> Hi Nikls
> 
> Well, if it can return -EPROBE_DEFER, it means sometimes WoL will be
> avalable and other times not, depending on when the clock driver

Ahh I see yes that would be indeed be bad.

> probes. However, it sounds like this is the SoCs core clock driver. If
> so, it gets loaded very early, so you are safe.

Yes this is renesas-cpg-mssr which if I understand things is a core 
clock driver. It is register in drivers/clk/renesas/renesas-cpg-mssr.c 
using:

 subsys_initcall(cpg_mssr_init);

So I take it I'm safe. Thanks however for bringing this to my attention 
I learnt something new today :-)

> 
>        Andrew

-- 
Regards,
Niklas S�derlund

Re: [PATCH v2] ravb: add wake-on-lan support via magic packet

[Sergei Shtylyov]

Hello!

On 07/30/2017 05:06 PM, Niklas Söderlund wrote:

> 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 ravb was not suspended to reduce resume time. To reset the
> hardware the driver closes the device, sets it in reset mode and reopens
> the device just like it would do in a normal suspend/resume scenario
> without WoL enabled, but it both closes and opens the device in the
> resume callback since the device needs to be reset 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>
[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 5931e859876c2aee..3d399f85417a83cf 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> @@ -2179,6 +2270,32 @@ static int __maybe_unused ravb_resume(struct device *dev)
>   	struct ravb_private *priv = netdev_priv(ndev);
>   	int ret = 0;
>   
> +	/* Reduce the usecount of the clock to zero and then
> +	 * restore it to its original value. This is done to force
> +	 * the clock to be re-enabled which is a workaround
> +	 * for renesas-cpg-mssr driver which do not enable clocks
> +	 * when resuming from PSCI suspend/resume.
> +	 *
> +	 * Without this workaround the driver fails to communicate
> +	 * with the hardware if WoL was enabled when the system
> +	 * entered PSCI suspend. This is due to that if WoL is enabled
> +	 * we explicitly keep the clock from being turned off when
> +	 * suspending, but in PSCI sleep power is cut so the clock
> +	 * is disabled anyhow, the clock driver is not aware of this
> +	 * so the clock is not turned back on when resuming.
> +	 *
> +	 * TODO: once the renesas-cpg-mssr suspend/resume is working
> +	 *       this clock dance should be removed.
> +	 */
> +	clk_disable(priv->clk);
> +	clk_disable(priv->clk);
> +	clk_enable(priv->clk);
> +	clk_enable(priv->clk);

    After a small chat with Niklas, it became clear that this dance should be 
behind the *if* that follows. I'd also like to see this workaround as a 
separate patch since the isse it addresses is R-Car gen3 specific (and thus 
can be reverted once the CPG/MMSR driver is fixed)...

> +
> +	/* If WoL is enabled set reset mode to rearm the WoL logic */
> +	if (priv->wol_enabled)
> +		ravb_write(ndev, CCC_OPC_RESET, CCC);
> +
>   	/* 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.
> @@ -2202,6 +2319,11 @@ static int __maybe_unused ravb_resume(struct device *dev)
>  	ravb_write(ndev, priv->desc_bat_dma, DBAT);
>  
>  	if (netif_running(ndev)) {
> +		if (priv->wol_enabled) {
> +			ret = ravb_wol_restore(ndev);
> +			if (ret)
> +				return ret;
> +		}
>  		ret = ravb_open(ndev);

    Hm, perhaps worth calling sh_eth_open() outside sh_eth_wol_restore() as well?

MBR, Sergei

Re: [PATCH v2] ravb: add wake-on-lan support via magic packet

[Niklas Söderlund]

Hi Sergei,

Thanks for your feedback!

On 2017-07-31 22:47:12 +0300, Sergei Shtylyov wrote:
> Hello!
> 
> On 07/30/2017 05:06 PM, Niklas Söderlund wrote:
> 
> > 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 ravb was not suspended to reduce resume time. To reset the
> > hardware the driver closes the device, sets it in reset mode and reopens
> > the device just like it would do in a normal suspend/resume scenario
> > without WoL enabled, but it both closes and opens the device in the
> > resume callback since the device needs to be reset 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>
> [...]
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> > index 5931e859876c2aee..3d399f85417a83cf 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> [...]
> > @@ -2179,6 +2270,32 @@ static int __maybe_unused ravb_resume(struct device *dev)
> >   	struct ravb_private *priv = netdev_priv(ndev);
> >   	int ret = 0;
> > +	/* Reduce the usecount of the clock to zero and then
> > +	 * restore it to its original value. This is done to force
> > +	 * the clock to be re-enabled which is a workaround
> > +	 * for renesas-cpg-mssr driver which do not enable clocks
> > +	 * when resuming from PSCI suspend/resume.
> > +	 *
> > +	 * Without this workaround the driver fails to communicate
> > +	 * with the hardware if WoL was enabled when the system
> > +	 * entered PSCI suspend. This is due to that if WoL is enabled
> > +	 * we explicitly keep the clock from being turned off when
> > +	 * suspending, but in PSCI sleep power is cut so the clock
> > +	 * is disabled anyhow, the clock driver is not aware of this
> > +	 * so the clock is not turned back on when resuming.
> > +	 *
> > +	 * TODO: once the renesas-cpg-mssr suspend/resume is working
> > +	 *       this clock dance should be removed.
> > +	 */
> > +	clk_disable(priv->clk);
> > +	clk_disable(priv->clk);
> > +	clk_enable(priv->clk);
> > +	clk_enable(priv->clk);
> 
>    After a small chat with Niklas, it became clear that this dance should be
> behind the *if* that follows. I'd also like to see this workaround as a
> separate patch since the isse it addresses is R-Car gen3 specific (and thus
> can be reverted once the CPG/MMSR driver is fixed)...

Thanks for spotting my mistake! I will fix this and break out the 
workaround in a separate patch and send a v3.

> 
> > +
> > +	/* If WoL is enabled set reset mode to rearm the WoL logic */
> > +	if (priv->wol_enabled)
> > +		ravb_write(ndev, CCC_OPC_RESET, CCC);
> > +
> >   	/* 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.
> > @@ -2202,6 +2319,11 @@ static int __maybe_unused ravb_resume(struct device *dev)
> >  	ravb_write(ndev, priv->desc_bat_dma, DBAT);
> >  	if (netif_running(ndev)) {
> > +		if (priv->wol_enabled) {
> > +			ret = ravb_wol_restore(ndev);
> > +			if (ret)
> > +				return ret;
> > +		}
> >  		ret = ravb_open(ndev);
> 
>    Hm, perhaps worth calling sh_eth_open() outside sh_eth_wol_restore() as well?

Worth looking at, but this is outside the scope of this series as it 
touches the sh_eth driver :-)

> 
> MBR, Sergei

-- 
Regards,
Niklas Söderlund

Re: [PATCH v2] ravb: add wake-on-lan support via magic packet

[Niklas Söderlund]

Hi Sergei,

Thanks for your feedback!

On 2017-07-31 22:47:12 +0300, Sergei Shtylyov wrote:
> Hello!
> 
> On 07/30/2017 05:06 PM, Niklas S�derlund wrote:
> 
> > 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 ravb was not suspended to reduce resume time. To reset the
> > hardware the driver closes the device, sets it in reset mode and reopens
> > the device just like it would do in a normal suspend/resume scenario
> > without WoL enabled, but it both closes and opens the device in the
> > resume callback since the device needs to be reset 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>
> [...]
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> > index 5931e859876c2aee..3d399f85417a83cf 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> [...]
> > @@ -2179,6 +2270,32 @@ static int __maybe_unused ravb_resume(struct device *dev)
> >   	struct ravb_private *priv = netdev_priv(ndev);
> >   	int ret = 0;
> > +	/* Reduce the usecount of the clock to zero and then
> > +	 * restore it to its original value. This is done to force
> > +	 * the clock to be re-enabled which is a workaround
> > +	 * for renesas-cpg-mssr driver which do not enable clocks
> > +	 * when resuming from PSCI suspend/resume.
> > +	 *
> > +	 * Without this workaround the driver fails to communicate
> > +	 * with the hardware if WoL was enabled when the system
> > +	 * entered PSCI suspend. This is due to that if WoL is enabled
> > +	 * we explicitly keep the clock from being turned off when
> > +	 * suspending, but in PSCI sleep power is cut so the clock
> > +	 * is disabled anyhow, the clock driver is not aware of this
> > +	 * so the clock is not turned back on when resuming.
> > +	 *
> > +	 * TODO: once the renesas-cpg-mssr suspend/resume is working
> > +	 *       this clock dance should be removed.
> > +	 */
> > +	clk_disable(priv->clk);
> > +	clk_disable(priv->clk);
> > +	clk_enable(priv->clk);
> > +	clk_enable(priv->clk);
> 
>    After a small chat with Niklas, it became clear that this dance should be
> behind the *if* that follows. I'd also like to see this workaround as a
> separate patch since the isse it addresses is R-Car gen3 specific (and thus
> can be reverted once the CPG/MMSR driver is fixed)...

Thanks for spotting my mistake! I will fix this and break out the 
workaround in a separate patch and send a v3.

> 
> > +
> > +	/* If WoL is enabled set reset mode to rearm the WoL logic */
> > +	if (priv->wol_enabled)
> > +		ravb_write(ndev, CCC_OPC_RESET, CCC);
> > +
> >   	/* 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.
> > @@ -2202,6 +2319,11 @@ static int __maybe_unused ravb_resume(struct device *dev)
> >  	ravb_write(ndev, priv->desc_bat_dma, DBAT);
> >  	if (netif_running(ndev)) {
> > +		if (priv->wol_enabled) {
> > +			ret = ravb_wol_restore(ndev);
> > +			if (ret)
> > +				return ret;
> > +		}
> >  		ret = ravb_open(ndev);
> 
>    Hm, perhaps worth calling sh_eth_open() outside sh_eth_wol_restore() as well?

Worth looking at, but this is outside the scope of this series as it 
touches the sh_eth driver :-)

> 
> MBR, Sergei

-- 
Regards,
Niklas S�derlund

Re: [PATCH v2] ravb: add wake-on-lan support via magic packet

[Geert Uytterhoeven]

Hi Andrew, Niklas,

On Sun, Jul 30, 2017 at 9:51 PM, Niklas Söderlund
<niklas.soderlund@ragnatech.se> wrote:
> On 2017-07-30 19:07:38 +0200, Andrew Lunn wrote:
>> > @@ -2041,6 +2073,11 @@ static int ravb_probe(struct platform_device *pdev)
>> >
>> >     priv->chip_id = chip_id;
>> >
>> > +   /* Get clock, if not found that's OK but Wake-On-Lan is unavailable */
>> > +   priv->clk = devm_clk_get(&pdev->dev, NULL);
>> > +   if (IS_ERR(priv->clk))
>> > +           priv->clk = NULL;
>>
>> Can you get EPROBE_DEFER returned?
>
> I don't think so, but I'm not sure :-)
>
> The clock I'm trying to get is the module clock of the ravb itself, so
> if that clock is not available (and enabled) no register writes to the
> ravb would be possible in the first place, so i guess it's safe to
> assume -EPROBE_DEFER can not happen here?

In theory, the devm_clk_get() can fail.
In practice, it cannot, as the EtherAVB device is part of a clock domain,
through its "power-domains" property in DT.
Hence if the clock (which is the module clock, provided by the SoC's system
clock controller (CPG/MSSR)) is not yet available, the PM Domain subsystem
will prevent the device from being probed.

> I'm just trying to play it safe here since the clock is only needed to
> support WoL, I though it best to not change behavior here. Try to get
> the clock, if we can great we can do WoL if not then user-space will be
> prevented from enabling WoL and nothing in the current behavior changes.

And all this explicit clock handling is done only because
device_set_wakeup_enable() does not yet prevent the PM Domain code from
powering down the device during system suspend.  Once that's handled by genpd,
all clock magic can be removed.

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