| Message ID | 1464974960-26672-1-git-send-email-vpalatin@chromium.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi, Am Freitag, 3. Juni 2016, 10:29:20 schrieb Vincent Palatin: > Do not shutdown the PHY if Wake-on-Lan is enabled, else it cannot wake > us up. > > Signed-off-by: Vincent Palatin <vpalatin@chromium.org> > --- > drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c index 0cd3ecf..2e45e75 > 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > @@ -534,6 +534,10 @@ static int rk_gmac_init(struct platform_device *pdev, > void *priv) struct rk_priv_data *bsp_priv = priv; > int ret; > > + /* Keep the PHY up if we use Wake-on-Lan. */ > + if (device_may_wakeup(&pdev->dev)) > + return 0; > + Hmm, this looks like it would also block the initial setup of clocks and phy? platform_device + device struct are created before probe gets called, so something could set the wakeup flag before the driver initially probes? Confused, Heiko
On Mon, Jun 6, 2016 at 1:45 PM, Heiko Stübner <heiko@sntech.de> wrote: > Hi, > > Am Freitag, 3. Juni 2016, 10:29:20 schrieb Vincent Palatin: >> Do not shutdown the PHY if Wake-on-Lan is enabled, else it cannot wake >> us up. >> >> Signed-off-by: Vincent Palatin <vpalatin@chromium.org> >> --- >> drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c >> b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c index 0cd3ecf..2e45e75 >> 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c >> @@ -534,6 +534,10 @@ static int rk_gmac_init(struct platform_device *pdev, >> void *priv) struct rk_priv_data *bsp_priv = priv; >> int ret; >> >> + /* Keep the PHY up if we use Wake-on-Lan. */ >> + if (device_may_wakeup(&pdev->dev)) >> + return 0; >> + > > Hmm, this looks like it would also block the initial setup of clocks and phy? Yes, that's bad. Doug told me so but I forget to CC him on the previous submission. I will do another version. > platform_device + device struct are created before probe gets called, so > something could set the wakeup flag before the driver initially probes? The device tree 'wakeup' attribute likely does it.
Hello On 6/3/2016 7:29 PM, Vincent Palatin wrote: > Do not shutdown the PHY if Wake-on-Lan is enabled, else it cannot wake > us up. > I do not understand why you need that. This is done inside the PHY layer and it is tested on our platforms he idea is: If the parent wants to Wake the system then the PHY should not power-down. Peppe > Signed-off-by: Vincent Palatin <vpalatin@chromium.org> > --- > drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > index 0cd3ecf..2e45e75 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > @@ -534,6 +534,10 @@ static int rk_gmac_init(struct platform_device *pdev, void *priv) > struct rk_priv_data *bsp_priv = priv; > int ret; > > + /* Keep the PHY up if we use Wake-on-Lan. */ > + if (device_may_wakeup(&pdev->dev)) > + return 0; > + > ret = phy_power_on(bsp_priv, true); > if (ret) > return ret; > @@ -549,6 +553,10 @@ static void rk_gmac_exit(struct platform_device *pdev, void *priv) > { > struct rk_priv_data *gmac = priv; > > + /* The PHY was up for Wake-on-Lan. */ > + if (device_may_wakeup(&pdev->dev)) > + return; > + > phy_power_on(gmac, false); > gmac_clk_enable(gmac, false); > } >
On Tue, Jun 7, 2016 at 12:23 AM, Giuseppe CAVALLARO <peppe.cavallaro@st.com> wrote: > Hello > > On 6/3/2016 7:29 PM, Vincent Palatin wrote: >> >> Do not shutdown the PHY if Wake-on-Lan is enabled, else it cannot wake >> us up. >> > > I do not understand why you need that. > This is done inside the PHY layer and it is tested on our platforms > he idea is: If the parent wants to Wake the system then the PHY should > not power-down. I'm not sure I understand : you mean that this path is not called if WoL is enabled ? [ currently stmmac_pltfr_suspend() is calling priv->plat->exit() which is the rk_gmac_exit() code I'm modifying ] or the RK driver code should not power down the phy in its exit() callback ? >> Signed-off-by: Vincent Palatin <vpalatin@chromium.org> >> --- >> drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c >> b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c >> index 0cd3ecf..2e45e75 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c >> @@ -534,6 +534,10 @@ static int rk_gmac_init(struct platform_device *pdev, >> void *priv) >> struct rk_priv_data *bsp_priv = priv; >> int ret; >> >> + /* Keep the PHY up if we use Wake-on-Lan. */ >> + if (device_may_wakeup(&pdev->dev)) >> + return 0; >> + >> ret = phy_power_on(bsp_priv, true); >> if (ret) >> return ret; >> @@ -549,6 +553,10 @@ static void rk_gmac_exit(struct platform_device >> *pdev, void *priv) >> { >> struct rk_priv_data *gmac = priv; >> >> + /* The PHY was up for Wake-on-Lan. */ >> + if (device_may_wakeup(&pdev->dev)) >> + return; >> + >> phy_power_on(gmac, false); >> gmac_clk_enable(gmac, false); >> } >> >
On Wed, Jun 08, 2016 at 03:25:38PM -0700, Vincent Palatin wrote: > On Tue, Jun 7, 2016 at 12:23 AM, Giuseppe CAVALLARO > <peppe.cavallaro@st.com> wrote: > > Hello > > > > On 6/3/2016 7:29 PM, Vincent Palatin wrote: > >> > >> Do not shutdown the PHY if Wake-on-Lan is enabled, else it cannot wake > >> us up. > >> > > > > I do not understand why you need that. > > This is done inside the PHY layer and it is tested on our platforms > > he idea is: If the parent wants to Wake the system then the PHY should > > not power-down. > > I'm not sure I understand : > you mean that this path is not called if WoL is enabled ? > [ currently stmmac_pltfr_suspend() is calling priv->plat->exit() which > is the rk_gmac_exit() code I'm modifying ] > or the RK driver code should not power down the phy in its exit() callback ? Take a look at phy_suspend(). Andrew
On Wed, Jun 8, 2016 at 5:17 PM, Andrew Lunn <andrew@lunn.ch> wrote: > On Wed, Jun 08, 2016 at 03:25:38PM -0700, Vincent Palatin wrote: >> On Tue, Jun 7, 2016 at 12:23 AM, Giuseppe CAVALLARO >> <peppe.cavallaro@st.com> wrote: >> > Hello >> > >> > On 6/3/2016 7:29 PM, Vincent Palatin wrote: >> >> >> >> Do not shutdown the PHY if Wake-on-Lan is enabled, else it cannot wake >> >> us up. >> >> >> > >> > I do not understand why you need that. >> > This is done inside the PHY layer and it is tested on our platforms >> > he idea is: If the parent wants to Wake the system then the PHY should >> > not power-down. >> >> I'm not sure I understand : >> you mean that this path is not called if WoL is enabled ? >> [ currently stmmac_pltfr_suspend() is calling priv->plat->exit() which >> is the rk_gmac_exit() code I'm modifying ] >> or the RK driver code should not power down the phy in its exit() callback ? > > Take a look at phy_suspend(). phy_suspend() sends (or not) the PowerDown command to the PHY through the MDIO bus, depending if WoL is disabled, but most of my question still stands as far as I can tell : I was trying to get a proper WoL support on the following setup : dwmac (inside a RK3288 SoC) connected to RTL8211 PHY The current upstream code for this case will call rk_gmac_exit() when the MAC suspends (after the PHY has already suspended). Effectively doing a phy_power_on(, false) which is calling regulator_disable() on the LDO defined by the 'phy-supply' attribute. So my reading is that the RK specific MAC code is turning off unconditionally the PHY power regulator. Unless I'm mistaken, either this code is incorrect for the WoL case or the naming 'phy-supply' is misleading and should be the MAC supply.
Hello Vincent On 6/10/2016 1:00 AM, Vincent Palatin wrote: > On Wed, Jun 8, 2016 at 5:17 PM, Andrew Lunn <andrew@lunn.ch> wrote: >> On Wed, Jun 08, 2016 at 03:25:38PM -0700, Vincent Palatin wrote: >>> On Tue, Jun 7, 2016 at 12:23 AM, Giuseppe CAVALLARO >>> <peppe.cavallaro@st.com> wrote: >>>> Hello >>>> >>>> On 6/3/2016 7:29 PM, Vincent Palatin wrote: >>>>> >>>>> Do not shutdown the PHY if Wake-on-Lan is enabled, else it cannot wake >>>>> us up. >>>>> >>>> >>>> I do not understand why you need that. >>>> This is done inside the PHY layer and it is tested on our platforms >>>> he idea is: If the parent wants to Wake the system then the PHY should >>>> not power-down. >>> >>> I'm not sure I understand : >>> you mean that this path is not called if WoL is enabled ? >>> [ currently stmmac_pltfr_suspend() is calling priv->plat->exit() which >>> is the rk_gmac_exit() code I'm modifying ] >>> or the RK driver code should not power down the phy in its exit() callback ? >> >> Take a look at phy_suspend(). > > phy_suspend() sends (or not) the PowerDown command to the PHY through > the MDIO bus, depending if WoL is disabled, > but most of my question still stands as far as I can tell : > I was trying to get a proper WoL support on the following setup : > dwmac (inside a RK3288 SoC) connected to RTL8211 PHY > The current upstream code for this case will call rk_gmac_exit() when > the MAC suspends (after the PHY has already suspended). Effectively > doing a phy_power_on(, false) which is calling regulator_disable() on > the LDO defined by the 'phy-supply' attribute. > So my reading is that the RK specific MAC code is turning off > unconditionally the PHY power regulator. Unless I'm mistaken, either > this code is incorrect for the WoL case or the naming 'phy-supply' is > misleading and should be the MAC supply. ok now clear. And you are right. I can conclude that the patch is ok for me. I just ask you to resend it elaborating a bit the subject and surrounding the code with a comment. I do not know your SoC but indeed, when doing WoL, some parts of the MAC + PHY must be powered so IMO it is legal that you do not cut the power by invoking regulator. Peppe
In order to support Wake-On-Lan when using the RK3288 integrated MAC (with an external RGMII PHY), we need to avoid shutting down the regulator of the external PHY when the MAC is suspended as it's currently done in the MAC platform code. As a first step, create independant callbacks for suspend/resume rather than re-using exit/init callbacks. So the dwmac platform driver can behave differently on suspend where it might skip shutting the PHY and at module unloading. Then update the dwmac-rk driver to switch off the PHY regulator only if we are not planning to wake up from the LAN. Finally add the PMT interrupt to the MAC device tree configuration, so we can wake up the core from it when the PHY has received the magic packet.
From: Vincent Palatin <vpalatin@chromium.org> Date: Fri, 10 Jun 2016 18:00:36 -0700 > In order to support Wake-On-Lan when using the RK3288 integrated MAC > (with an external RGMII PHY), we need to avoid shutting down the regulator > of the external PHY when the MAC is suspended as it's currently done in the MAC > platform code. > As a first step, create independant callbacks for suspend/resume rather than > re-using exit/init callbacks. So the dwmac platform driver can behave differently > on suspend where it might skip shutting the PHY and at module unloading. > Then update the dwmac-rk driver to switch off the PHY regulator only if we are > not planning to wake up from the LAN. > Finally add the PMT interrupt to the MAC device tree configuration, so we can > wake up the core from it when the PHY has received the magic packet. > Ignore my previous email, but in the future please tag these postings properly with "[PATCH 0/N] " at the beginning of the subject line. Thanks.
On 6/11/2016 3:00 AM, Vincent Palatin wrote: > In order to support Wake-On-Lan when using the RK3288 integrated MAC > (with an external RGMII PHY), we need to avoid shutting down the regulator > of the external PHY when the MAC is suspended as it's currently done in the MAC > platform code. > As a first step, create independant callbacks for suspend/resume rather than > re-using exit/init callbacks. So the dwmac platform driver can behave differently > on suspend where it might skip shutting the PHY and at module unloading. > Then update the dwmac-rk driver to switch off the PHY regulator only if we are > not planning to wake up from the LAN. > Finally add the PMT interrupt to the MAC device tree configuration, so we can > wake up the core from it when the PHY has received the magic packet. IMO these could be sent for net-next and also other glue logic files should be reworked in order to use the new API for coherence. Peppe
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c index 0cd3ecf..2e45e75 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c @@ -534,6 +534,10 @@ static int rk_gmac_init(struct platform_device *pdev, void *priv) struct rk_priv_data *bsp_priv = priv; int ret; + /* Keep the PHY up if we use Wake-on-Lan. */ + if (device_may_wakeup(&pdev->dev)) + return 0; + ret = phy_power_on(bsp_priv, true); if (ret) return ret; @@ -549,6 +553,10 @@ static void rk_gmac_exit(struct platform_device *pdev, void *priv) { struct rk_priv_data *gmac = priv; + /* The PHY was up for Wake-on-Lan. */ + if (device_may_wakeup(&pdev->dev)) + return; + phy_power_on(gmac, false); gmac_clk_enable(gmac, false); }
Do not shutdown the PHY if Wake-on-Lan is enabled, else it cannot wake us up. Signed-off-by: Vincent Palatin <vpalatin@chromium.org> --- drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 8 ++++++++ 1 file changed, 8 insertions(+)