| Message ID | a8d0393c977b0d6fdab5d48ad13ca4d1ca893a07.1496164448.git.leonard.crestez@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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!
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).
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?
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.
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.
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; }
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(+)