| Message ID | 20161207162843.4731-1-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Simon Horman |
| Headers | show |
Hi Niklas, On Wed, Dec 7, 2016 at 5:28 PM, Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> wrote: > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> Thanks, works fine on r8a7791/koelsch! Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- a/drivers/net/ethernet/renesas/sh_eth.c > +++ b/drivers/net/ethernet/renesas/sh_eth.c > @@ -624,7 +624,7 @@ static struct sh_eth_cpu_data r8a779x_data = { > > .register_type = SH_ETH_REG_FAST_RCAR, > > - .ecsr_value = ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD, > + .ecsr_value = ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD | ECSR_MPD, Interestingly, the ECSR_MPD bit is already set for several SoCs. Hence adding ".magic = 1" to the entry for r8a7740 instantly gave me working WoL support on r8a7740/armadillo. Cool! > --- 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 have PMDE in ECMR and MPDIP in ECSIPR */ Instead of adding a new flag, perhaps you can just check for the ECSR_MPD flag in ecsr_value? > @@ -529,6 +530,9 @@ struct sh_eth_private { > unsigned no_ether_link:1; > unsigned ether_link_active_low:1; > unsigned is_opened:1; > + > + bool wol_enabled; "unsigned wol_enabled:1", to merge with the bitfield above? > + struct clk *clk; It's a good practice to keep all pointers at the top of the struct, to avoid gaps due to alignment restrictions, especially on 64-bit (I know that's not the case here). 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
Hi Geert, Thanks for testing and your feedback. On 2016-12-07 19:14:40 +0100, Geert Uytterhoeven wrote: > Hi Niklas, > > On Wed, Dec 7, 2016 at 5:28 PM, Niklas Söderlund > <niklas.soderlund+renesas@ragnatech.se> wrote: > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > Thanks, works fine on r8a7791/koelsch! > > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > --- a/drivers/net/ethernet/renesas/sh_eth.c > > +++ b/drivers/net/ethernet/renesas/sh_eth.c > > @@ -624,7 +624,7 @@ static struct sh_eth_cpu_data r8a779x_data = { > > > > .register_type = SH_ETH_REG_FAST_RCAR, > > > > - .ecsr_value = ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD, > > + .ecsr_value = ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD | ECSR_MPD, > > Interestingly, the ECSR_MPD bit is already set for several SoCs. Yes, I noticed that and my assumption was that it was set 'just in case' to clear any MagicPacket interrupts at probe time. > > Hence adding ".magic = 1" to the entry for r8a7740 instantly gave me working > WoL support on r8a7740/armadillo. Cool! Cool, I will set ".magic = 1" for r8a7740 in v2. > > > --- 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 have PMDE in ECMR and MPDIP in ECSIPR */ > > Instead of adding a new flag, perhaps you can just check for the ECSR_MPD flag > in ecsr_value? I briefly considered this but decided against it since I do not have documentation for all versions of the device and no way to test it. You tested and confirmed functionality on r8a7740, which leaves: - sh7734-gether - sh7763-gether - sh7757-gether To figure out if they support MagicPacket in the same fashion as r8a7740 and r8a779x. If anyone have access to documentation or hardware to confirm this I be more then happy to get rid of the magic flag in favor och checking for ECSR_MPD in ecsr_value. > > > @@ -529,6 +530,9 @@ struct sh_eth_private { > > unsigned no_ether_link:1; > > unsigned ether_link_active_low:1; > > unsigned is_opened:1; > > + > > + bool wol_enabled; > > "unsigned wol_enabled:1", to merge with the bitfield above? Thanks, looking it it now I don't know what I was thinking. I will changes it for v2. > > > + struct clk *clk; > > It's a good practice to keep all pointers at the top of the struct, to avoid > gaps due to alignment restrictions, especially on 64-bit (I know that's not > the case here). Thanks, you learn new things everyday. I will move it for v2. > > 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
Hello!
Good to see that somebody cares still about this driver, one more task off
my back. :-)
On 12/07/2016 07:28 PM, Niklas Söderlund wrote:
You only enable the WOL support fo the R-Car gen2 chips but never say that
explicitly, neither in the subject nor here.
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> drivers/net/ethernet/renesas/sh_eth.c | 120 +++++++++++++++++++++++++++++++---
> drivers/net/ethernet/renesas/sh_eth.h | 4 ++
> 2 files changed, 116 insertions(+), 8 deletions(-)
> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> index 05b0dc5..3974046 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> @@ -624,7 +624,7 @@ static struct sh_eth_cpu_data r8a779x_data = {
>
> .register_type = SH_ETH_REG_FAST_RCAR,
>
> - .ecsr_value = ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD,
> + .ecsr_value = ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD | ECSR_MPD,
> .ecsipr_value = ECSIPR_PSRTOIP | ECSIPR_LCHNGIP | ECSIPR_ICDIP,
> .eesipr_value = 0x01ff009f,
>
> @@ -641,6 +641,7 @@ static struct sh_eth_cpu_data r8a779x_data = {
> .tpauser = 1,
> .hw_swap = 1,
> .rmiimode = 1,
> + .magic = 1,
> };
> #endif /* CONFIG_OF */
So I suggest that you add the general WOL support code in the 1st patch
and enable the new feature per SoC family in the follow up patches.
> @@ -1657,6 +1658,10 @@ static irqreturn_t sh_eth_interrupt(int irq, void *netdev)
> goto out;
>
> if (!likely(mdp->irq_enabled)) {
Oops, I guess unlikely(!mdp->irq_enabled) was meant here...
> + /* Handle MagicPacket interrupt */
> + if (sh_eth_read(ndev, ECSR) & ECSR_MPD)
Why do it here?
> + sh_eth_modify(ndev, ECSR, 0, ECSR_MPD);
> +
> sh_eth_write(ndev, 0, EESIPR);
> goto out;
> }
[...]
> @@ -3017,6 +3051,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);
Luckily, we have <linux/clk.h> #include'd...
> + if (IS_ERR(mdp->clk))
> + mdp->clk = NULL;
> +
> ndev->base_addr = res->start;
>
> spin_lock_init(&mdp->lock);
> @@ -3111,6 +3150,10 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
> if (ret)
> goto out_napi_del;
>
> + mdp->wol_enabled = false;
No need, the '*mdp' was kzalloc'ed.
> + 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 +3193,71 @@ 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 */
> + mdp->irq_enabled = false;
Why 'false' if you enable IRQs below?
> + synchronize_irq(ndev->irq);
> + napi_disable(&mdp->napi);
> + sh_eth_write(ndev, DMAC_M_ECI, EESIPR);
> +
> + /* Enable ECI MagicPacket interrupt */
> + sh_eth_write(ndev, ECSIPR_MPDIP, ECSIPR);
> +
> + /* Enable MagicPacket */
> + sh_eth_modify(ndev, ECMR, 0, ECMR_PMDE);
> +
> + /* Increased clock usage so device won't be suspended */
> + clk_enable(mdp->clk);
Hum, intermixiggn runtime PM with clock API doesn't look good...
> +
> + 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);
Hm... and RPM will think the clock is still enabled?
Why disable the clock here anyway if we're resuming?
> +
> + return disable_irq_wake(ndev->irq);
> +}
> +
[...]
> @@ -3166,14 +3265,19 @@ 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;
> +
> + if (!ret)
Please keep the original error return logic, so that you can return 0 at
the end...
> 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..26c6620 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 have PMDE in ECMR and MPDIP in ECSIPR */
OK, e.g. RZ/A1 doesn't have these bits...
[...]
MBR, Sergei
On do, dec 08, 2016 at 08:16:20 +0100, Niklas Söderlund wrote: > Hi Geert, > > Thanks for testing and your feedback. > > On 2016-12-07 19:14:40 +0100, Geert Uytterhoeven wrote: > > Hi Niklas, > > > > On Wed, Dec 7, 2016 at 5:28 PM, Niklas Söderlund > > <niklas.soderlund+renesas@ragnatech.se> wrote: > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > > Thanks, works fine on r8a7791/koelsch! > > > > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > --- a/drivers/net/ethernet/renesas/sh_eth.c > > > +++ b/drivers/net/ethernet/renesas/sh_eth.c > > > @@ -624,7 +624,7 @@ static struct sh_eth_cpu_data r8a779x_data = { > > > > > > .register_type = SH_ETH_REG_FAST_RCAR, > > > > > > - .ecsr_value = ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD, > > > + .ecsr_value = ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD | ECSR_MPD, > > > > Interestingly, the ECSR_MPD bit is already set for several SoCs. > > Yes, I noticed that and my assumption was that it was set 'just in case' > to clear any MagicPacket interrupts at probe time. > > > > > Hence adding ".magic = 1" to the entry for r8a7740 instantly gave me working > > WoL support on r8a7740/armadillo. Cool! > > Cool, I will set ".magic = 1" for r8a7740 in v2. > > > > > > --- 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 have PMDE in ECMR and MPDIP in ECSIPR */ > > > > Instead of adding a new flag, perhaps you can just check for the ECSR_MPD flag > > in ecsr_value? > > I briefly considered this but decided against it since I do not have > documentation for all versions of the device and no way to test it. You > tested and confirmed functionality on r8a7740, which leaves: > > - sh7734-gether > - sh7763-gether > - sh7757-gether > > To figure out if they support MagicPacket in the same fashion as r8a7740 > and r8a779x. If anyone have access to documentation or hardware to > confirm this I be more then happy to get rid of the magic flag in favor > och checking for ECSR_MPD in ecsr_value. Perhaps documentation can be found but if not I wonder if we can use some other mechanism to blacklist SoC which we are unsure about. From my POV it would be very nice if things just worked™ on SoCs where the feature has been verified. > > > @@ -529,6 +530,9 @@ struct sh_eth_private { > > > unsigned no_ether_link:1; > > > unsigned ether_link_active_low:1; > > > unsigned is_opened:1; > > > + > > > + bool wol_enabled; > > > > "unsigned wol_enabled:1", to merge with the bitfield above? > > Thanks, looking it it now I don't know what I was thinking. I will > changes it for v2. > > > > > > + struct clk *clk; > > > > It's a good practice to keep all pointers at the top of the struct, to avoid > > gaps due to alignment restrictions, especially on 64-bit (I know that's not > > the case here). > > Thanks, you learn new things everyday. I will move it for v2. > > > > > 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 >
Hi Sergei, Thanks for your feedback. On 2016-12-08 15:28:48 +0300, Sergei Shtylyov wrote: > Hello! > > Good to see that somebody cares still about this driver, one more task > off my back. :-) > > On 12/07/2016 07:28 PM, Niklas Söderlund wrote: > > You only enable the WOL support fo the R-Car gen2 chips but never say that > explicitly, neither in the subject nor here. > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > --- > > drivers/net/ethernet/renesas/sh_eth.c | 120 +++++++++++++++++++++++++++++++--- > > drivers/net/ethernet/renesas/sh_eth.h | 4 ++ > > 2 files changed, 116 insertions(+), 8 deletions(-) > > > diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c > > index 05b0dc5..3974046 100644 > > --- a/drivers/net/ethernet/renesas/sh_eth.c > > +++ b/drivers/net/ethernet/renesas/sh_eth.c > > @@ -624,7 +624,7 @@ static struct sh_eth_cpu_data r8a779x_data = { > > > > .register_type = SH_ETH_REG_FAST_RCAR, > > > > - .ecsr_value = ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD, > > + .ecsr_value = ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD | ECSR_MPD, > > .ecsipr_value = ECSIPR_PSRTOIP | ECSIPR_LCHNGIP | ECSIPR_ICDIP, > > .eesipr_value = 0x01ff009f, > > > > @@ -641,6 +641,7 @@ static struct sh_eth_cpu_data r8a779x_data = { > > .tpauser = 1, > > .hw_swap = 1, > > .rmiimode = 1, > > + .magic = 1, > > }; > > #endif /* CONFIG_OF */ > > So I suggest that you add the general WOL support code in the 1st patch > and enable the new feature per SoC family in the follow up patches. Ok I will split this up in v2. > > > @@ -1657,6 +1658,10 @@ static irqreturn_t sh_eth_interrupt(int irq, void *netdev) > > goto out; > > > > if (!likely(mdp->irq_enabled)) { > > Oops, I guess unlikely(!mdp->irq_enabled) was meant here... I can correct this in a separate patch if you wish. > > > + /* Handle MagicPacket interrupt */ > > + if (sh_eth_read(ndev, ECSR) & ECSR_MPD) > > Why do it here? See bellow. > > > + sh_eth_modify(ndev, ECSR, 0, ECSR_MPD); > > + > > sh_eth_write(ndev, 0, EESIPR); > > goto out; > > } > [...] > > @@ -3017,6 +3051,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); > > Luckily, we have <linux/clk.h> #include'd... > > > + if (IS_ERR(mdp->clk)) > > + mdp->clk = NULL; > > + > > ndev->base_addr = res->start; > > > > spin_lock_init(&mdp->lock); > > @@ -3111,6 +3150,10 @@ static int sh_eth_drv_probe(struct platform_device *pdev) > > if (ret) > > goto out_napi_del; > > > > + mdp->wol_enabled = false; > > No need, the '*mdp' was kzalloc'ed. OK, i prefer to explicitly set for easier reading of the code. But if you wish I will remove this in v2. > > > + 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 +3193,71 @@ 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 */ > > + mdp->irq_enabled = false; > > Why 'false' if you enable IRQs below? I mask all interrupts except MagicPacket (ECSIPR_MPDIP) interrupts form the ECI (DMAC_M_ECI) and by setting irq_enabled to false the interrupt handler will only ack any residue interrupt. This is how it's done in other parts of the driver when disabling interrupts. This is also why I only check for MagicPacket interrupts if irq_enabled is false. > > > + synchronize_irq(ndev->irq); > > + napi_disable(&mdp->napi); > > + sh_eth_write(ndev, DMAC_M_ECI, EESIPR); > > + > > + /* Enable ECI MagicPacket interrupt */ > > + sh_eth_write(ndev, ECSIPR_MPDIP, ECSIPR); > > + > > + /* Enable MagicPacket */ > > + sh_eth_modify(ndev, ECMR, 0, ECMR_PMDE); > > + > > + /* Increased clock usage so device won't be suspended */ > > + clk_enable(mdp->clk); > > Hum, intermixiggn runtime PM with clock API doesn't look good... I agree it looks weird but I need a way to increment the usage count for the clock otherwise the PM code will disable the module clock and WoL will not work. Note that this call will not enable the clock just increase the usage count so it won't be disabled when the PM code decrease it after the sh_eth suspend function is run. If you know of a different way of ensuring that the clock is not turned off I be happy to look at it. I did some investigation into this and calling clk_enable() directly is for example what happens in the enable_irq_wake() call path to ensure the clock for the irq_chip is not turned off if it is a wakeup source, se for example gpio_rcar_irq_set_wake() in drivers/gpio/gpio-rcar.c. > > > + > > + 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); > > Hm... and RPM will think the clock is still enabled? > Why disable the clock here anyway if we're resuming? This call is needed to decrease the usage count for the clock we increased with clk_enable() in the suspend path. > > > + > > + return disable_irq_wake(ndev->irq); > > +} > > + > [...] > > @@ -3166,14 +3265,19 @@ 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; > > + > > + if (!ret) > > Please keep the original error return logic, so that you can return 0 at > the end... Will fix for v2. > > > 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..26c6620 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 have PMDE in ECMR and MPDIP in ECSIPR */ > > OK, e.g. RZ/A1 doesn't have these bits... > > [...] > > MBR, Sergei >
Hi Simon, Thanks for your feedback. On 2016-12-08 14:22:44 +0100, Simon Horman wrote: <snip> > > > > > > > --- 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 have PMDE in ECMR and MPDIP in ECSIPR */ > > > > > > Instead of adding a new flag, perhaps you can just check for the ECSR_MPD flag > > > in ecsr_value? > > > > I briefly considered this but decided against it since I do not have > > documentation for all versions of the device and no way to test it. You > > tested and confirmed functionality on r8a7740, which leaves: > > > > - sh7734-gether > > - sh7763-gether > > - sh7757-gether > > > > To figure out if they support MagicPacket in the same fashion as r8a7740 > > and r8a779x. If anyone have access to documentation or hardware to > > confirm this I be more then happy to get rid of the magic flag in favor > > och checking for ECSR_MPD in ecsr_value. > > Perhaps documentation can be found but if not I wonder if we can use some > other mechanism to blacklist SoC which we are unsure about. > > From my POV it would be very nice if things just worked™ on SoCs where > the feature has been verified. I agree, I will follow Sergies advice and Geerts testing to enable Gen2 family and r8a7740/armadillo in two separate patches. Then if we later can confirm it works on other models we can enable them in separate patches by setting the magic flag in struct sh_eth_cpu_data for those models. Do you agree this is the best way to handle this?
On Thu, Dec 8, 2016 at 3:56 PM, Niklas Söderlund <niklas.soderlund@ragnatech.se> wrote: >> > + /* Increased clock usage so device won't be suspended */ >> > + clk_enable(mdp->clk); >> >> Hum, intermixiggn runtime PM with clock API doesn't look good... > > I agree it looks weird but I need a way to increment the usage count for > the clock otherwise the PM code will disable the module clock and WoL > will not work. Note that this call will not enable the clock just > increase the usage count so it won't be disabled when the PM code > decrease it after the sh_eth suspend function is run. > > If you know of a different way of ensuring that the clock is not turned > off I be happy to look at it. I did some investigation into this and > calling clk_enable() directly is for example what happens in the > enable_irq_wake() call path to ensure the clock for the irq_chip is not > turned off if it is a wakeup source, se for example > gpio_rcar_irq_set_wake() in drivers/gpio/gpio-rcar.c. Ideally we want to prevent the PM code from disabling the clock, and from powering down the power area, if present. The latter may matter on other SoCs. But on r8a7740, Ethernet is part of power area A4S, which we already prevent from being powered down because it contains the memory controller. 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
On Thu, Dec 08, 2016 at 04:01:05PM +0100, Niklas Söderlund wrote: > Hi Simon, > > Thanks for your feedback. > > On 2016-12-08 14:22:44 +0100, Simon Horman wrote: > > <snip> > > > > > > > > > > --- 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 have PMDE in ECMR and MPDIP in ECSIPR */ > > > > > > > > Instead of adding a new flag, perhaps you can just check for the ECSR_MPD flag > > > > in ecsr_value? > > > > > > I briefly considered this but decided against it since I do not have > > > documentation for all versions of the device and no way to test it. You > > > tested and confirmed functionality on r8a7740, which leaves: > > > > > > - sh7734-gether > > > - sh7763-gether > > > - sh7757-gether > > > > > > To figure out if they support MagicPacket in the same fashion as r8a7740 > > > and r8a779x. If anyone have access to documentation or hardware to > > > confirm this I be more then happy to get rid of the magic flag in favor > > > och checking for ECSR_MPD in ecsr_value. > > > > Perhaps documentation can be found but if not I wonder if we can use some > > other mechanism to blacklist SoC which we are unsure about. > > > > From my POV it would be very nice if things just worked™ on SoCs where > > the feature has been verified. > > I agree, I will follow Sergies advice and Geerts testing to enable Gen2 > family and r8a7740/armadillo in two separate patches. Then if we later > can confirm it works on other models we can enable them in separate > patches by setting the magic flag in struct sh_eth_cpu_data for those > models. Do you agree this is the best way to handle this? Yes, I think that is reasonable.
On 12/08/2016 03:28 PM, Sergei Shtylyov wrote: > Good to see that somebody cares still about this driver, one more task off > my back. :-) > > On 12/07/2016 07:28 PM, Niklas Söderlund wrote: > > You only enable the WOL support fo the R-Car gen2 chips but never say that > explicitly, neither in the subject nor here. Some patch description wouldn't hurt here, especially with the way you implemented this support, e.g. RPM vs clk API -- that needs some explanation... >> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> >> --- >> drivers/net/ethernet/renesas/sh_eth.c | 120 +++++++++++++++++++++++++++++++--- >> drivers/net/ethernet/renesas/sh_eth.h | 4 ++ >> 2 files changed, 116 insertions(+), 8 deletions(-) >> diff --git a/drivers/net/ethernet/renesas/sh_eth.c >> b/drivers/net/ethernet/renesas/sh_eth.c >> index 05b0dc5..3974046 100644 >> --- a/drivers/net/ethernet/renesas/sh_eth.c >> +++ b/drivers/net/ethernet/renesas/sh_eth.c [...] >> diff --git a/drivers/net/ethernet/renesas/sh_eth.h >> b/drivers/net/ethernet/renesas/sh_eth.h >> index d050f37..26c6620 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 have PMDE in ECMR and MPDIP in ECSIPR */ > > OK, e.g. RZ/A1 doesn't have these bits... However, I'd prefer that the comment be reworded as such: /* EtherC has ECMR.PMDE and ECSR.MPD */ or /* EtherC has ECMR_PMDE and ECSR_MPD */ MBR, Sergei
Hello! On 12/08/2016 05:56 PM, Niklas Söderlund wrote: >> You only enable the WOL support fo the R-Car gen2 chips but never say that >> explicitly, neither in the subject nor here. >> >>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> >>> --- >>> drivers/net/ethernet/renesas/sh_eth.c | 120 +++++++++++++++++++++++++++++++--- >>> drivers/net/ethernet/renesas/sh_eth.h | 4 ++ >>> 2 files changed, 116 insertions(+), 8 deletions(-) >> >>> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c >>> index 05b0dc5..3974046 100644 >>> --- a/drivers/net/ethernet/renesas/sh_eth.c >>> +++ b/drivers/net/ethernet/renesas/sh_eth.c [...] >>> @@ -1657,6 +1658,10 @@ static irqreturn_t sh_eth_interrupt(int irq, void *netdev) >>> goto out; >>> >>> if (!likely(mdp->irq_enabled)) { >> >> Oops, I guess unlikely(!mdp->irq_enabled) was meant here... > > I can correct this in a separate patch if you wish. I'll look into this myself, I think. >> + /* Handle MagicPacket interrupt */ >> + if (sh_eth_read(ndev, ECSR) & ECSR_MPD) What if it wasn't enabled ATM? [...] >>> @@ -3111,6 +3150,10 @@ static int sh_eth_drv_probe(struct platform_device *pdev) >>> if (ret) >>> goto out_napi_del; >>> >>> + mdp->wol_enabled = false; >> >> No need, the '*mdp' was kzalloc'ed. > > OK, i prefer to explicitly set for easier reading of the code. But if > you wish I will remove this in v2. Yes, remove it please. >>> @@ -3150,15 +3193,71 @@ 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 */ >>> + mdp->irq_enabled = false; >> >> Why 'false' if you enable IRQs below? > > I mask all interrupts except MagicPacket (ECSIPR_MPDIP) interrupts form > the ECI (DMAC_M_ECI) and by setting irq_enabled to false the interrupt > handler will only ack any residue interrupt. I don't see where it ack's anything, it just clears EESIPR and returns in this case. > This is how it's done in > other parts of the driver when disabling interrupts. Not in all parts of the driver that disable EESIPR interrupts... I must confess that I never liked that 'mdp->irq_enabled' flag and still suspect we can get things done without it... I need to look at this code again, sigh... > This is also why I only check for MagicPacket interrupts if irq_enabled > is false. I would have preferred that this was done with the other EMAC interrupts, in sh_eth_error(). >>> + synchronize_irq(ndev->irq); >>> + napi_disable(&mdp->napi); >>> + sh_eth_write(ndev, DMAC_M_ECI, EESIPR); >>> + >>> + /* Enable ECI MagicPacket interrupt */ >>> + sh_eth_write(ndev, ECSIPR_MPDIP, ECSIPR); I'd prefer if it was always enabled via 'ecsipr_value'. >>> + >>> + /* Enable MagicPacket */ >>> + sh_eth_modify(ndev, ECMR, 0, ECMR_PMDE); >>> + >>> + /* Increased clock usage so device won't be suspended */ >>> + clk_enable(mdp->clk); >> >> Hum, intermixiggn runtime PM with clock API doesn't look good... > > I agree it looks weird but I need a way to increment the usage count for > the clock otherwise the PM code will disable the module clock and WoL > will not work. How will it do it if you don't call sh_eth_close() in this case? > Note that this call will not enable the clock just > increase the usage count so it won't be disabled when the PM code > decrease it after the sh_eth suspend function is run. You mean that the PM code calls RPM or clk API on its own? That's strange... > If you know of a different way of ensuring that the clock is not turned > off I be happy to look at it. I did some investigation into this and > calling clk_enable() directly is for example what happens in the > enable_irq_wake() call path to ensure the clock for the irq_chip is not > turned off if it is a wakeup source, se for example > gpio_rcar_irq_set_wake() in drivers/gpio/gpio-rcar.c. Thanks, will look into it... [...] MBR, Sergei
Hi Sergei, Thanks for your feedback. On 2016-12-11 00:25:41 +0300, Sergei Shtylyov wrote: > Hello! > > On 12/08/2016 05:56 PM, Niklas Söderlund wrote: > > > > You only enable the WOL support fo the R-Car gen2 chips but never say that > > > explicitly, neither in the subject nor here. > > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > > --- > > > > drivers/net/ethernet/renesas/sh_eth.c | 120 +++++++++++++++++++++++++++++++--- > > > > drivers/net/ethernet/renesas/sh_eth.h | 4 ++ > > > > 2 files changed, 116 insertions(+), 8 deletions(-) > > > > > > > diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c > > > > index 05b0dc5..3974046 100644 > > > > --- a/drivers/net/ethernet/renesas/sh_eth.c > > > > +++ b/drivers/net/ethernet/renesas/sh_eth.c > [...] > > > > @@ -1657,6 +1658,10 @@ static irqreturn_t sh_eth_interrupt(int irq, void *netdev) > > > > goto out; > > > > > > > > if (!likely(mdp->irq_enabled)) { > > > > > > Oops, I guess unlikely(!mdp->irq_enabled) was meant here... > > > > I can correct this in a separate patch if you wish. > > I'll look into this myself, I think. OK. > > > > + /* Handle MagicPacket interrupt */ > > > + if (sh_eth_read(ndev, ECSR) & ECSR_MPD) > > What if it wasn't enabled ATM? Sorry I don't understand this comment. > > [...] > > > > @@ -3111,6 +3150,10 @@ static int sh_eth_drv_probe(struct platform_device *pdev) > > > > if (ret) > > > > goto out_napi_del; > > > > > > > > + mdp->wol_enabled = false; > > > > > > No need, the '*mdp' was kzalloc'ed. > > > > OK, i prefer to explicitly set for easier reading of the code. But if > > you wish I will remove this in v2. > > Yes, remove it please. Will remove for v2. > > > > > @@ -3150,15 +3193,71 @@ 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 */ > > > > + mdp->irq_enabled = false; > > > > > > Why 'false' if you enable IRQs below? > > > > I mask all interrupts except MagicPacket (ECSIPR_MPDIP) interrupts form > > the ECI (DMAC_M_ECI) and by setting irq_enabled to false the interrupt > > handler will only ack any residue interrupt. > > I don't see where it ack's anything, it just clears EESIPR and returns in > this case. You are right, I must have misread when looking at this. > > > This is how it's done in > > other parts of the driver when disabling interrupts. > > Not in all parts of the driver that disable EESIPR interrupts... I must > confess that I never liked that 'mdp->irq_enabled' flag and still suspect we > can get things done without it... I need to look at this code again, sigh... > > > This is also why I only check for MagicPacket interrupts if irq_enabled > > is false. > > I would have preferred that this was done with the other EMAC interrupts, > in sh_eth_error(). I removed the check for Magic Packet in sh_eth_interrupt() and running without setting mdp->irq_enabled = false. sh_eth_error() will then clear any ECI interrupt so no need to add Magic Packet detection to it since all that is needed on Magic Packet is to clear the interrupt which already is done. This works and I can do multiple suspend/resume cycles, will be in v2 thanks for the suggestion. > > > > > + synchronize_irq(ndev->irq); > > > > + napi_disable(&mdp->napi); > > > > + sh_eth_write(ndev, DMAC_M_ECI, EESIPR); > > > > + > > > > + /* Enable ECI MagicPacket interrupt */ > > > > + sh_eth_write(ndev, ECSIPR_MPDIP, ECSIPR); > > I'd prefer if it was always enabled via 'ecsipr_value'. Will do so in v2. > > > > > + > > > > + /* Enable MagicPacket */ > > > > + sh_eth_modify(ndev, ECMR, 0, ECMR_PMDE); > > > > + > > > > + /* Increased clock usage so device won't be suspended */ > > > > + clk_enable(mdp->clk); > > > > > > Hum, intermixiggn runtime PM with clock API doesn't look good... > > > > I agree it looks weird but I need a way to increment the usage count for > > the clock otherwise the PM code will disable the module clock and WoL > > will not work. > > How will it do it if you don't call sh_eth_close() in this case? > > > Note that this call will not enable the clock just > > increase the usage count so it won't be disabled when the PM code > > decrease it after the sh_eth suspend function is run. > > You mean that the PM code calls RPM or clk API on its own? That's strange... Yes it calls clk API. > > > If you know of a different way of ensuring that the clock is not turned > > off I be happy to look at it. I did some investigation into this and > > calling clk_enable() directly is for example what happens in the > > enable_irq_wake() call path to ensure the clock for the irq_chip is not > > turned off if it is a wakeup source, se for example > > gpio_rcar_irq_set_wake() in drivers/gpio/gpio-rcar.c. > > Thanks, will look into it... > > [...] > > MBR, Sergei >
On 12/12/2016 06:49 PM, Niklas Söderlund wrote: > Thanks for your feedback. Not at all, it's my duty now. :-) I should probably have warned you not to post the new version to netdev -- DaveM has closed his net-next.git tree (ahead of the usual time, which would have been 4.9 release), so you posting would only upset him... [...] >>>> You only enable the WOL support fo the R-Car gen2 chips but never say that >>>> explicitly, neither in the subject nor here. >>>> >>>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> >>>>> --- >>>>> drivers/net/ethernet/renesas/sh_eth.c | 120 +++++++++++++++++++++++++++++++--- >>>>> drivers/net/ethernet/renesas/sh_eth.h | 4 ++ >>>>> 2 files changed, 116 insertions(+), 8 deletions(-) >>>> >>>>> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c >>>>> index 05b0dc5..3974046 100644 >>>>> --- a/drivers/net/ethernet/renesas/sh_eth.c >>>>> +++ b/drivers/net/ethernet/renesas/sh_eth.c [...] >>>> + /* Handle MagicPacket interrupt */ >>>> + if (sh_eth_read(ndev, ECSR) & ECSR_MPD) >> >> What if it wasn't enabled ATM? > > Sorry I don't understand this comment. I'm trying to handle only the enabled interrupts but this hasn't been consistently done yet (only for EESR, not ECSR), so nevermind. :-) [...] >>>>> @@ -3150,15 +3193,71 @@ static int sh_eth_drv_remove(struct platform_device *pdev) [...] >>> This is how it's done in >>> other parts of the driver when disabling interrupts. >> >> Not in all parts of the driver that disable EESIPR interrupts... I must >> confess that I never liked that 'mdp->irq_enabled' flag and still suspect we >> can get things done without it... I need to look at this code again, sigh... Well, we can't most probably but I have a patch almost ready that turns the boolean flag into u32 field holding the EESIPR value to be written next. Would that help you? >>> This is also why I only check for MagicPacket interrupts if irq_enabled >>> is false. >> >> I would have preferred that this was done with the other EMAC interrupts, >> in sh_eth_error(). > > I removed the check for Magic Packet in sh_eth_interrupt() and running > without setting mdp->irq_enabled = false. sh_eth_error() will then clear > any ECI interrupt so no need to add Magic Packet detection to it since > all that is needed on Magic Packet is to clear the interrupt which > already is done. This works and I can do multiple suspend/resume cycles, > will be in v2 thanks for the suggestion. OK, let's see what you have when I have some more time. We have a lot of time for ironing things out till net-next is opened again -- which will happen after -rc1)... [...] >>>>> + >>>>> + /* Enable MagicPacket */ >>>>> + sh_eth_modify(ndev, ECMR, 0, ECMR_PMDE); >>>>> + >>>>> + /* Increased clock usage so device won't be suspended */ >>>>> + clk_enable(mdp->clk); >>>> >>>> Hum, intermixiggn runtime PM with clock API doesn't look good... >>> >>> I agree it looks weird but I need a way to increment the usage count for >>> the clock otherwise the PM code will disable the module clock and WoL >>> will not work. >> >> How will it do it if you don't call sh_eth_close() in this case? >> >>> Note that this call will not enable the clock just >>> increase the usage count so it won't be disabled when the PM code >>> decrease it after the sh_eth suspend function is run. >> >> You mean that the PM code calls RPM or clk API on its own? That's strange... > > Yes it calls clk API. Hum, will have to look into it as well... [...] MBR, Sergei
diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c index 05b0dc5..3974046 100644 --- a/drivers/net/ethernet/renesas/sh_eth.c +++ b/drivers/net/ethernet/renesas/sh_eth.c @@ -624,7 +624,7 @@ static struct sh_eth_cpu_data r8a779x_data = { .register_type = SH_ETH_REG_FAST_RCAR, - .ecsr_value = ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD, + .ecsr_value = ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD | ECSR_MPD, .ecsipr_value = ECSIPR_PSRTOIP | ECSIPR_LCHNGIP | ECSIPR_ICDIP, .eesipr_value = 0x01ff009f, @@ -641,6 +641,7 @@ static struct sh_eth_cpu_data r8a779x_data = { .tpauser = 1, .hw_swap = 1, .rmiimode = 1, + .magic = 1, }; #endif /* CONFIG_OF */ @@ -1657,6 +1658,10 @@ static irqreturn_t sh_eth_interrupt(int irq, void *netdev) goto out; if (!likely(mdp->irq_enabled)) { + /* Handle MagicPacket interrupt */ + if (sh_eth_read(ndev, ECSR) & ECSR_MPD) + sh_eth_modify(ndev, ECSR, 0, ECSR_MPD); + sh_eth_write(ndev, 0, EESIPR); goto out; } @@ -2199,6 +2204,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 +2245,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 +3051,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 +3150,10 @@ static int sh_eth_drv_probe(struct platform_device *pdev) if (ret) goto out_napi_del; + mdp->wol_enabled = false; + 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 +3193,71 @@ 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 */ + mdp->irq_enabled = false; + synchronize_irq(ndev->irq); + napi_disable(&mdp->napi); + sh_eth_write(ndev, DMAC_M_ECI, EESIPR); + + /* Enable ECI MagicPacket interrupt */ + sh_eth_write(ndev, ECSIPR_MPDIP, ECSIPR); + + /* 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 +3265,19 @@ 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; + + if (!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..26c6620 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 have PMDE in ECMR and MPDIP in ECSIPR */ }; struct sh_eth_private { @@ -529,6 +530,9 @@ struct sh_eth_private { unsigned no_ether_link:1; unsigned ether_link_active_low:1; unsigned is_opened:1; + + bool wol_enabled; + struct clk *clk; }; static inline void sh_eth_soft_swap(char *src, int len)
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- drivers/net/ethernet/renesas/sh_eth.c | 120 +++++++++++++++++++++++++++++++--- drivers/net/ethernet/renesas/sh_eth.h | 4 ++ 2 files changed, 116 insertions(+), 8 deletions(-)