[PATCH 0/2] ARM: imx6ul-14x14-evk: Fix suspend over nfs by phy cleanup

[PATCH 0/2] ARM: imx6ul-14x14-evk: Fix suspend over nfs by phy cleanup

[Leonard Crestez]

Right now attempting doing suspend/resume while root is mounted over NFS hangs
on imx6ul-14x14-evk. This is happening because ksz8081 phy fixups are lost on
resume.

Fix this by using equivalent devicetree properties instead of a phy fixup and
handling those properties on resume in the micrel driver.

Other solutions would be possible such as having a way to run phy fixups again
on resume. This alternative is precisely targeted.

In theory it might now be possible to remove the phy fixup from mach-imx6ul
entirely but it is possible that this would break other imx6ul boards which use
the same phy. The solution would be to patch their dts but it's not clear how
to identify affected boards.

This code is shared with imx6ull-14x14-evk but 6ull suspend needs an unrelated
patch: https://lkml.org/lkml/2017/5/30/584

Leonard Crestez (2):
  ARM: dts: imx6ul-14x14-evk: Add ksz8081 phy properties
  net: phy: micrel: Restore led_mode and clk_sel on resume

 arch/arm/boot/dts/imx6ul-14x14-evk.dts |  6 ++++++
 drivers/net/phy/micrel.c               | 15 +++++++++++++++
 2 files changed, 21 insertions(+)

-- 
2.7.4

[PATCH 1/2] ARM: dts: imx6ul-14x14-evk: Add ksz8081 phy properties

[Leonard Crestez]

Right now mach-imx6ul registers a fixup for the ksz8081 phy. The same
register values can be set through the micrel phy driver by using dts
properties.

This seems preferable and allows cleanly fixing suspend/resume.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 arch/arm/boot/dts/imx6ul-14x14-evk.dts | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/imx6ul-14x14-evk.dts b/arch/arm/boot/dts/imx6ul-14x14-evk.dts
index f18e1f1..d2be8aa 100644
--- a/arch/arm/boot/dts/imx6ul-14x14-evk.dts
+++ b/arch/arm/boot/dts/imx6ul-14x14-evk.dts
@@ -120,10 +120,16 @@
 
 		ethphy0: ethernet-phy@2 {
 			reg = <2>;
+			micrel,led-mode = <1>;
+			clocks = <&clks IMX6UL_CLK_ENET_REF>;
+			clock-names = "rmii-ref";
 		};
 
 		ethphy1: ethernet-phy@1 {
 			reg = <1>;
+			micrel,led-mode = <1>;
+			clocks = <&clks IMX6UL_CLK_ENET2_REF>;
+			clock-names = "rmii-ref";
 		};
 	};
 };
-- 
2.7.4

Re: [PATCH 1/2] ARM: dts: imx6ul-14x14-evk: Add ksz8081 phy properties

[Florian Fainelli]

On 05/30/2017 10:34 AM, Leonard Crestez wrote:
> Right now mach-imx6ul registers a fixup for the ksz8081 phy. The same
> register values can be set through the micrel phy driver by using dts
> properties.
> 
> This seems preferable and allows cleanly fixing suspend/resume.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

Should you have a Fixes: tag for that? Sounds like something you'd want
to backport to stable trees as well, no?

> ---
>  arch/arm/boot/dts/imx6ul-14x14-evk.dts | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx6ul-14x14-evk.dts b/arch/arm/boot/dts/imx6ul-14x14-evk.dts
> index f18e1f1..d2be8aa 100644
> --- a/arch/arm/boot/dts/imx6ul-14x14-evk.dts
> +++ b/arch/arm/boot/dts/imx6ul-14x14-evk.dts
> @@ -120,10 +120,16 @@
>  
>  		ethphy0: ethernet-phy@2 {
>  			reg = <2>;
> +			micrel,led-mode = <1>;
> +			clocks = <&clks IMX6UL_CLK_ENET_REF>;
> +			clock-names = "rmii-ref";
>  		};
>  
>  		ethphy1: ethernet-phy@1 {
>  			reg = <1>;
> +			micrel,led-mode = <1>;
> +			clocks = <&clks IMX6UL_CLK_ENET2_REF>;
> +			clock-names = "rmii-ref";
>  		};
>  	};
>  };
> 


-- 
Florian

Re: [PATCH 1/2] ARM: dts: imx6ul-14x14-evk: Add ksz8081 phy properties

[Leonard Crestez]

On Tue, 2017-05-30 at 11:10 -0700, Florian Fainelli wrote:
> On 05/30/2017 10:34 AM, Leonard Crestez wrote:
> > Right now mach-imx6ul registers a fixup for the ksz8081 phy. The same
> > register values can be set through the micrel phy driver by using dts
> > properties.
> > 
> > This seems preferable and allows cleanly fixing suspend/resume.
> > 
> > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> 
> Should you have a Fixes: tag for that? Sounds like something you'd want
> to backport to stable trees as well, no?

I don't know if suspend over nfs ever worked on this board in upstream
so there is no commit that this patch "Fixes: ". It would make sense to
CC stable.

Re: [PATCH 1/2] ARM: dts: imx6ul-14x14-evk: Add ksz8081 phy properties

[Fabio Estevam]

On Tue, May 30, 2017 at 2:34 PM, Leonard Crestez
<leonard.crestez@nxp.com> wrote:
> Right now mach-imx6ul registers a fixup for the ksz8081 phy. The same
> register values can be set through the micrel phy driver by using dts
> properties.
>
> This seems preferable and allows cleanly fixing suspend/resume.
>
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>

[PATCH 2/2] net: phy: micrel: Restore led_mode and clk_sel on resume

[Leonard Crestez]

These bits seem to be lost after a suspend/resume cycle so just set them
again.

This patch fixes ethernet suspend/resume on imx6ul-14x14-evk boards.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/net/phy/micrel.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 6a5fd18..c53ee17 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -700,6 +700,9 @@ static int kszphy_suspend(struct phy_device *phydev)
 
 static int kszphy_resume(struct phy_device *phydev)
 {
+	struct kszphy_priv *priv = phydev->priv;
+	int ret;
+
 	genphy_resume(phydev);
 
 	/* Enable PHY Interrupts */
@@ -709,6 +712,18 @@ static int kszphy_resume(struct phy_device *phydev)
 			phydev->drv->config_intr(phydev);
 	}
 
+	if (priv->rmii_ref_clk_sel) {
+		ret = kszphy_rmii_clk_sel(phydev, priv->rmii_ref_clk_sel_val);
+		if (ret) {
+			phydev_err(phydev,
+				   "failed to set rmii reference clock\n");
+			return ret;
+		}
+	}
+
+	if (priv->led_mode >= 0)
+		kszphy_setup_led(phydev, priv->type->led_mode_reg, priv->led_mode);
+
 	return 0;
 }
 
-- 
2.7.4

Re: [PATCH 2/2] net: phy: micrel: Restore led_mode and clk_sel on resume

[Florian Fainelli]

On 05/30/2017 10:34 AM, Leonard Crestez wrote:
> These bits seem to be lost after a suspend/resume cycle so just set them
> again.
> 
> This patch fixes ethernet suspend/resume on imx6ul-14x14-evk boards.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/net/phy/micrel.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 6a5fd18..c53ee17 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -700,6 +700,9 @@ static int kszphy_suspend(struct phy_device *phydev)
>  
>  static int kszphy_resume(struct phy_device *phydev)
>  {
> +	struct kszphy_priv *priv = phydev->priv;
> +	int ret;
> +
>  	genphy_resume(phydev);
>  
>  	/* Enable PHY Interrupts */
> @@ -709,6 +712,18 @@ static int kszphy_resume(struct phy_device *phydev)
>  			phydev->drv->config_intr(phydev);
>  	}
>  
> +	if (priv->rmii_ref_clk_sel) {
> +		ret = kszphy_rmii_clk_sel(phydev, priv->rmii_ref_clk_sel_val);
> +		if (ret) {
> +			phydev_err(phydev,
> +				   "failed to set rmii reference clock\n");
> +			return ret;
> +		}
> +	}
> +
> +	if (priv->led_mode >= 0)
> +		kszphy_setup_led(phydev, priv->type->led_mode_reg, priv->led_mode);

Should not we actually call kszphy_config_init() in order to restore
broadcast and nand disable bits as well?

If not, I would be more comfortable if we did create a specific function
that takes care of setting the reference clock and LED mode.

Other than that, LGTM!
-- 
Florian

Re: [PATCH 2/2] net: phy: micrel: Restore led_mode and clk_sel on resume

[Leonard Crestez]

On Tue, 2017-05-30 at 11:05 -0700, Florian Fainelli wrote:
> On 05/30/2017 10:34 AM, Leonard Crestez wrote:
> > These bits seem to be lost after a suspend/resume cycle so just set them
> > again.
> > 
> > This patch fixes ethernet suspend/resume on imx6ul-14x14-evk boards.
> > 
> > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> > ---
> >  drivers/net/phy/micrel.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> > index 6a5fd18..c53ee17 100644
> > --- a/drivers/net/phy/micrel.c
> > +++ b/drivers/net/phy/micrel.c
> > @@ -700,6 +700,9 @@ static int kszphy_suspend(struct phy_device *phydev)
> >  
> >  static int kszphy_resume(struct phy_device *phydev)
> >  {
> > +	struct kszphy_priv *priv = phydev->priv;
> > +	int ret;
> > +
> >  	genphy_resume(phydev);
> >  
> >  	/* Enable PHY Interrupts */
> > @@ -709,6 +712,18 @@ static int kszphy_resume(struct phy_device *phydev)
> >  			phydev->drv->config_intr(phydev);
> >  	}
> >  
> > +	if (priv->rmii_ref_clk_sel) {
> > +		ret = kszphy_rmii_clk_sel(phydev, priv->rmii_ref_clk_sel_val);
> > +		if (ret) {
> > +			phydev_err(phydev,
> > +				   "failed to set rmii reference clock\n");
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	if (priv->led_mode >= 0)
> > +		kszphy_setup_led(phydev, priv->type->led_mode_reg, priv->led_mode);
> 
> Should not we actually call kszphy_config_init() in order to restore
> broadcast and nand disable bits as well?

I don't know. In my case the B_CAST_OFF bit doesn't seem to be lost and
NAND_TREE_ON is already off by the time it gets to linux.

The bit that get lost seem to disappear just as the phy is resumed. I
added some prints and they look like this:

PM: early resume of devices complete after 6.534 msecs
begin resume
0x1F=0x8190 0x16=0x202
after genphy_resume 0x1F=0x8100 0x16=0x202
end
resume 0x1F=0x8190 0x16=0x202

> If not, I would be more comfortable if we did create a specific function
> that takes care of setting the reference clock and LED mode.

Ok, I can add a function called kszphy_config_reset() with a comment
explaining it's for bits lost on reset/resume.

Or perhaps a better option would be to just save/restore the entire
0x1F register?

Re: [PATCH 2/2] net: phy: micrel: Restore led_mode and clk_sel on resume

[Florian Fainelli]

On 05/30/2017 03:08 PM, Leonard Crestez wrote:
> On Tue, 2017-05-30 at 11:05 -0700, Florian Fainelli wrote:
>> On 05/30/2017 10:34 AM, Leonard Crestez wrote:
>>> These bits seem to be lost after a suspend/resume cycle so just set them
>>> again.
>>>
>>> This patch fixes ethernet suspend/resume on imx6ul-14x14-evk boards.
>>>
>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>>> ---
>>>  drivers/net/phy/micrel.c | 15 +++++++++++++++
>>>  1 file changed, 15 insertions(+)
>>>
>>> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
>>> index 6a5fd18..c53ee17 100644
>>> --- a/drivers/net/phy/micrel.c
>>> +++ b/drivers/net/phy/micrel.c
>>> @@ -700,6 +700,9 @@ static int kszphy_suspend(struct phy_device *phydev)
>>>  
>>>  static int kszphy_resume(struct phy_device *phydev)
>>>  {
>>> +	struct kszphy_priv *priv = phydev->priv;
>>> +	int ret;
>>> +
>>>  	genphy_resume(phydev);
>>>  
>>>  	/* Enable PHY Interrupts */
>>> @@ -709,6 +712,18 @@ static int kszphy_resume(struct phy_device *phydev)
>>>  			phydev->drv->config_intr(phydev);
>>>  	}
>>>  
>>> +	if (priv->rmii_ref_clk_sel) {
>>> +		ret = kszphy_rmii_clk_sel(phydev, priv->rmii_ref_clk_sel_val);
>>> +		if (ret) {
>>> +			phydev_err(phydev,
>>> +				   "failed to set rmii reference clock\n");
>>> +			return ret;
>>> +		}
>>> +	}
>>> +
>>> +	if (priv->led_mode >= 0)
>>> +		kszphy_setup_led(phydev, priv->type->led_mode_reg, priv->led_mode);
>>
>> Should not we actually call kszphy_config_init() in order to restore
>> broadcast and nand disable bits as well?
> 
> I don't know. In my case the B_CAST_OFF bit doesn't seem to be lost and
> NAND_TREE_ON is already off by the time it gets to linux.
> 
> The bit that get lost seem to disappear just as the phy is resumed. I
> added some prints and they look like this:
> 
> PM: early resume of devices complete after 6.534 msecs
> begin resume
> 0x1F=0x8190 0x16=0x202
> after genphy_resume 0x1F=0x8100 0x16=0x202
> end
> resume 0x1F=0x8190 0x16=0x202

OK, so it actually would not hurt to call config_init() again here, right?

> 
>> If not, I would be more comfortable if we did create a specific function
>> that takes care of setting the reference clock and LED mode.
> 
> Ok, I can add a function called kszphy_config_reset() with a comment
> explaining it's for bits lost on reset/resume.
> 
> Or perhaps a better option would be to just save/restore the entire
> 0x1F register?

Register 0 through 15 are standardized, but those after that are not,
and PHYs with a gazillion of registers already need to have a specific
set of suspend/resume sequence due to their proprietary indirect
register scheme, so we cannot quite come up with a generic function that
would cache all 16 or 32 registers and flat out restore those.

Similarly, having phy_resume() systematically calling into config_init()
is a bit of a stretch and can be pretty inefficient.
-- 
Florian

Re: [PATCH 2/2] net: phy: micrel: Restore led_mode and clk_sel on resume

[Leonard Crestez]

On Tue, 2017-05-30 at 15:19 -0700, Florian Fainelli wrote:
> On 05/30/2017 03:08 PM, Leonard Crestez wrote:
> > On Tue, 2017-05-30 at 11:05 -0700, Florian Fainelli wrote:

> > > Should not we actually call kszphy_config_init() in order to restore
> > > broadcast and nand disable bits as well?
> > 
> > I don't know. In my case the B_CAST_OFF bit doesn't seem to be lost and
> > NAND_TREE_ON is already off by the time it gets to linux.
> > 
> > The bit that get lost seem to disappear just as the phy is resumed. I
> > added some prints and they look like this:
> > 
> > PM: early resume of devices complete after 6.534 msecs
> > begin resume
> > 0x1F=0x8190 0x16=0x202
> > after genphy_resume 0x1F=0x8100 0x16=0x202
> > end
> > resume 0x1F=0x8190 0x16=0x202
> 
> OK, so it actually would not hurt to call config_init() again here, right?

No, but if only some registers are lost then why reconfigure others? In
particular it seems that only MII_KSZPHY_CTRL_2 is lost but not
MII_KSZPHY_OMSO.

However a few extra phy reads don't really matter. Calling config_init
is the path of least resistance.

Another problem is that the ksz9031 driver uses kszphy_resume but has a
completely different init function. The bits I am concerned about do
not seem to exist in hardware but it does something completely
unrelated with parsing OF properties. Should I split this into
ksz8021_resume and ksz9031_resume?

It seems likely that more of the kszphy drivers need suspend/resume
code but nobody got around to testing them.

> > > If not, I would be more comfortable if we did create a specific function
> > > that takes care of setting the reference clock and LED mode.
> > 
> > Ok, I can add a function called kszphy_config_reset() with a comment
> > explaining it's for bits lost on reset/resume.
> > 
> > Or perhaps a better option would be to just save/restore the entire
> > 0x1F register?
> 
> Register 0 through 15 are standardized, but those after that are not,
> and PHYs with a gazillion of registers already need to have a specific
> set of suspend/resume sequence due to their proprietary indirect
> register scheme, so we cannot quite come up with a generic function that
> would cache all 16 or 32 registers and flat out restore those.
> 
> Similarly, having phy_resume() systematically calling into config_init()
> is a bit of a stretch and can be pretty inefficient.

I'm not suggesting changing this at the phy core level. Just that maybe
kszphy_resume specifically could save/restore register 0x1F instead of
going through config logic again (which would involve extra reads and
writes).

However it seems that this has complications, for example on some
versions the leds are written to a different register. It's not really
worth optimizing to such an extent.

Re: [PATCH 2/2] net: phy: micrel: Restore led_mode and clk_sel on resume

[Fabio Estevam]

Hi Leonard,

On Tue, May 30, 2017 at 2:34 PM, Leonard Crestez
<leonard.crestez@nxp.com> wrote:
> These bits seem to be lost after a suspend/resume cycle so just set them
> again.
>
> This patch fixes ethernet suspend/resume on imx6ul-14x14-evk boards.
>
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

When you send a v2 addressing Florian's comment, please make sure you
send it to David Miller (netdev maintainer).