| Message ID | 20241211072136.745553-1-wei.fang@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4,net] net: phy: micrel: Dynamically control external clock of KSZ PHY | expand |
On Wed, 11 Dec 2024 15:21:36 +0800 Wei Fang wrote: > On the i.MX6ULL-14x14-EVK board, enet1_ref and enet2_ref are used as the > clock sources for two external KSZ PHYs. However, after closing the two > FEC ports, the clk_enable_count of the enet1_ref and enet2_ref clocks is > not 0. The root cause is that since the commit 985329462723 ("net: phy: > micrel: use devm_clk_get_optional_enabled for the rmii-ref clock"), the > external clock of KSZ PHY has been enabled when the PHY driver probes, > and it can only be disabled when the PHY driver is removed. This causes > the clock to continue working when the system is suspended or the network > port is down. Andrew, could you pass the final judgment on this? :)
> To solve this problem, the clock is enabled when phy_driver::resume() is > called, and the clock is disabled when phy_driver::suspend() is called. > Since phy_driver::resume() and phy_driver::suspend() are not called in > pairs, an additional clk_enable flag is added. When phy_driver::suspend() > is called, the clock is disabled only if clk_enable is true. Conversely, > when phy_driver::resume() is called, the clock is enabled if clk_enable > is false. This is what i don't like too much, the fact suspend/resume is not called in pairs. That was probably a bad decision on my part, and maybe it should be revised. But that is out of scope for this patch, unless you want the challenge. > +static int ksz8041_resume(struct phy_device *phydev) > +{ > + struct kszphy_priv *priv = phydev->priv; > + > + kszphy_enable_clk(priv); > + > + return 0; > +} > + > +static int ksz8041_suspend(struct phy_device *phydev) > +{ > + struct kszphy_priv *priv = phydev->priv; > + > + kszphy_disable_clk(priv); > + > + return 0; > +} These two look odd. Why is there no call to genphy_suspend/genphy_resume? Probably a comment should be added here. > @@ -2150,8 +2198,11 @@ static int ksz9477_resume(struct phy_device *phydev) > > static int ksz8061_resume(struct phy_device *phydev) > { > + struct kszphy_priv *priv = phydev->priv; > int ret; > > + kszphy_enable_clk(priv); > + > /* This function can be called twice when the Ethernet device is on. */ > ret = phy_read(phydev, MII_BMCR); > if (ret < 0) What about 8061_suspend()? They normally are used in pairs. > @@ -2221,6 +2272,11 @@ static int kszphy_probe(struct phy_device *phydev) > return PTR_ERR(clk); > } > > + if (!IS_ERR_OR_NULL(clk)) { > + clk_disable_unprepare(clk); > + priv->clk = clk; > + } I think you should improve the error handling. If devm_clk_get_optional_enabled() returns an error, you should fail the probe. If the clock does not exist, devm_clk_get_optional_enabled() will return a NULL pointer, which is a valid clock. You can enable/disable it etc. So you then do not need this check. > +static int lan8804_suspend(struct phy_device *phydev) > +{ > + struct kszphy_priv *priv = phydev->priv; > + int ret; > + > + ret = genphy_suspend(phydev); > + if (ret) > + return ret; > + > + kszphy_disable_clk(priv); > + > + return 0; > +} > + > +static int lan8841_resume(struct phy_device *phydev) > +{ > + struct kszphy_priv *priv = phydev->priv; > + > + kszphy_enable_clk(priv); > + > + return genphy_resume(phydev); > +} > + > static int lan8841_suspend(struct phy_device *phydev) > { > struct kszphy_priv *priv = phydev->priv; > struct kszphy_ptp_priv *ptp_priv = &priv->ptp_priv; > + int ret; > > if (ptp_priv->ptp_clock) > ptp_cancel_worker_sync(ptp_priv->ptp_clock); > > - return genphy_suspend(phydev); > + ret = genphy_suspend(phydev); > + if (ret) > + return ret; > + > + kszphy_disable_clk(priv); > + > + return 0; This can be simplified to return lan8804_suspend(phydev); In general, you should define a common low level function which does what all PHYs need, in this case, genphy_suspend() and disable the clock. Then add wrappers which do additional things. Andrew
> -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: 2024年12月13日 18:45 > To: Wei Fang <wei.fang@nxp.com> > Cc: hkallweit1@gmail.com; linux@armlinux.org.uk; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; > florian.fainelli@broadcom.com; heiko.stuebner@cherry.de; Frank Li > <frank.li@nxp.com>; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > imx@lists.linux.dev > Subject: Re: [PATCH v4 net] net: phy: micrel: Dynamically control external > clock of KSZ PHY > > > To solve this problem, the clock is enabled when phy_driver::resume() is > > called, and the clock is disabled when phy_driver::suspend() is called. > > Since phy_driver::resume() and phy_driver::suspend() are not called in > > pairs, an additional clk_enable flag is added. When phy_driver::suspend() > > is called, the clock is disabled only if clk_enable is true. Conversely, > > when phy_driver::resume() is called, the clock is enabled if clk_enable > > is false. > > This is what i don't like too much, the fact suspend/resume is not > called in pairs. That was probably a bad decision on my part, and > maybe it should be revised. But that is out of scope for this patch, > unless you want the challenge. > > > +static int ksz8041_resume(struct phy_device *phydev) > > +{ > > + struct kszphy_priv *priv = phydev->priv; > > + > > + kszphy_enable_clk(priv); > > + > > + return 0; > > +} > > + > > +static int ksz8041_suspend(struct phy_device *phydev) > > +{ > > + struct kszphy_priv *priv = phydev->priv; > > + > > + kszphy_disable_clk(priv); > > + > > + return 0; > > +} > > These two look odd. Why is there no call to > genphy_suspend/genphy_resume? Probably a comment should be added > here. > I have added the comment in ksphy_driver[], maybe it would be better to move the comment here. - /* No suspend/resume callbacks because of errata DS80000700A, - * receiver error following software power down. + /* Because of errata DS80000700A, receiver error following software + * power down. Suspend and resume callbacks only disable and enable + * external rmii reference clock. */ > > @@ -2150,8 +2198,11 @@ static int ksz9477_resume(struct phy_device > *phydev) > > > > static int ksz8061_resume(struct phy_device *phydev) > > { > > + struct kszphy_priv *priv = phydev->priv; > > int ret; > > > > + kszphy_enable_clk(priv); > > + > > /* This function can be called twice when the Ethernet device is on. */ > > ret = phy_read(phydev, MII_BMCR); > > if (ret < 0) > > What about 8061_suspend()? They normally are used in pairs. Okay, I can add the ksz8061_suspend(), so that it is a pair with ksz8061_resume(). > > > @@ -2221,6 +2272,11 @@ static int kszphy_probe(struct phy_device > *phydev) > > return PTR_ERR(clk); > > } > > > > + if (!IS_ERR_OR_NULL(clk)) { > > + clk_disable_unprepare(clk); > > + priv->clk = clk; > > + } > > I think you should improve the error handling. If > devm_clk_get_optional_enabled() returns an error, you should fail the > probe. If the clock does not exist, devm_clk_get_optional_enabled() > will return a NULL pointer, which is a valid clock. You can > enable/disable it etc. So you then do not need this check. Okay, accept. > > > +static int lan8804_suspend(struct phy_device *phydev) > > +{ > > + struct kszphy_priv *priv = phydev->priv; > > + int ret; > > + > > + ret = genphy_suspend(phydev); > > + if (ret) > > + return ret; > > + > > + kszphy_disable_clk(priv); > > + > > + return 0; > > +} > > + > > +static int lan8841_resume(struct phy_device *phydev) > > +{ > > + struct kszphy_priv *priv = phydev->priv; > > + > > + kszphy_enable_clk(priv); > > + > > + return genphy_resume(phydev); > > +} > > + > > static int lan8841_suspend(struct phy_device *phydev) > > { > > struct kszphy_priv *priv = phydev->priv; > > struct kszphy_ptp_priv *ptp_priv = &priv->ptp_priv; > > + int ret; > > > > if (ptp_priv->ptp_clock) > > ptp_cancel_worker_sync(ptp_priv->ptp_clock); > > > > - return genphy_suspend(phydev); > > + ret = genphy_suspend(phydev); > > + if (ret) > > + return ret; > > + > > + kszphy_disable_clk(priv); > > + > > + return 0; > > This can be simplified to > > return lan8804_suspend(phydev); To be honest, I don't know the relationship between lan8804 and lan8841, So I'm not sure if changes added later in lan8804_suspend() will also work for lan8841. Otherwise, it will need to be reworked in the future. So I think there is no need to make this simplification. Unless you are very sure that the two platforms are compatible. > > In general, you should define a common low level function which does > what all PHYs need, in this case, genphy_suspend() and disable the > clock. Then add wrappers which do additional things. > Good suggestion, let me improve it, thanks
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index 3ef508840674..ffc2ac39fa48 100644 --- a/drivers/net/phy/micrel.c +++ b/drivers/net/phy/micrel.c @@ -432,10 +432,12 @@ struct kszphy_ptp_priv { struct kszphy_priv { struct kszphy_ptp_priv ptp_priv; const struct kszphy_type *type; + struct clk *clk; int led_mode; u16 vct_ctrl1000; bool rmii_ref_clk_sel; bool rmii_ref_clk_sel_val; + bool clk_enable; u64 stats[ARRAY_SIZE(kszphy_hw_stats)]; }; @@ -2050,8 +2052,27 @@ static void kszphy_get_stats(struct phy_device *phydev, data[i] = kszphy_get_stat(phydev, i); } +static void kszphy_enable_clk(struct kszphy_priv *priv) +{ + if (!priv->clk_enable && priv->clk) { + clk_prepare_enable(priv->clk); + priv->clk_enable = true; + } +} + +static void kszphy_disable_clk(struct kszphy_priv *priv) +{ + if (priv->clk_enable && priv->clk) { + clk_disable_unprepare(priv->clk); + priv->clk_enable = false; + } +} + static int kszphy_suspend(struct phy_device *phydev) { + struct kszphy_priv *priv = phydev->priv; + int ret; + /* Disable PHY Interrupts */ if (phy_interrupt_is_valid(phydev)) { phydev->interrupts = PHY_INTERRUPT_DISABLED; @@ -2059,7 +2080,13 @@ static int kszphy_suspend(struct phy_device *phydev) phydev->drv->config_intr(phydev); } - return genphy_suspend(phydev); + ret = genphy_suspend(phydev); + if (ret) + return ret; + + kszphy_disable_clk(priv); + + return 0; } static void kszphy_parse_led_mode(struct phy_device *phydev) @@ -2088,8 +2115,11 @@ static void kszphy_parse_led_mode(struct phy_device *phydev) static int kszphy_resume(struct phy_device *phydev) { + struct kszphy_priv *priv = phydev->priv; int ret; + kszphy_enable_clk(priv); + genphy_resume(phydev); /* After switching from power-down to normal mode, an internal global @@ -2112,6 +2142,24 @@ static int kszphy_resume(struct phy_device *phydev) return 0; } +static int ksz8041_resume(struct phy_device *phydev) +{ + struct kszphy_priv *priv = phydev->priv; + + kszphy_enable_clk(priv); + + return 0; +} + +static int ksz8041_suspend(struct phy_device *phydev) +{ + struct kszphy_priv *priv = phydev->priv; + + kszphy_disable_clk(priv); + + return 0; +} + static int ksz9477_resume(struct phy_device *phydev) { int ret; @@ -2150,8 +2198,11 @@ static int ksz9477_resume(struct phy_device *phydev) static int ksz8061_resume(struct phy_device *phydev) { + struct kszphy_priv *priv = phydev->priv; int ret; + kszphy_enable_clk(priv); + /* This function can be called twice when the Ethernet device is on. */ ret = phy_read(phydev, MII_BMCR); if (ret < 0) @@ -2221,6 +2272,11 @@ static int kszphy_probe(struct phy_device *phydev) return PTR_ERR(clk); } + if (!IS_ERR_OR_NULL(clk)) { + clk_disable_unprepare(clk); + priv->clk = clk; + } + if (ksz8041_fiber_mode(phydev)) phydev->port = PORT_FIBRE; @@ -5290,15 +5346,45 @@ static int lan8841_probe(struct phy_device *phydev) return 0; } +static int lan8804_suspend(struct phy_device *phydev) +{ + struct kszphy_priv *priv = phydev->priv; + int ret; + + ret = genphy_suspend(phydev); + if (ret) + return ret; + + kszphy_disable_clk(priv); + + return 0; +} + +static int lan8841_resume(struct phy_device *phydev) +{ + struct kszphy_priv *priv = phydev->priv; + + kszphy_enable_clk(priv); + + return genphy_resume(phydev); +} + static int lan8841_suspend(struct phy_device *phydev) { struct kszphy_priv *priv = phydev->priv; struct kszphy_ptp_priv *ptp_priv = &priv->ptp_priv; + int ret; if (ptp_priv->ptp_clock) ptp_cancel_worker_sync(ptp_priv->ptp_clock); - return genphy_suspend(phydev); + ret = genphy_suspend(phydev); + if (ret) + return ret; + + kszphy_disable_clk(priv); + + return 0; } static struct phy_driver ksphy_driver[] = { @@ -5358,9 +5444,12 @@ static struct phy_driver ksphy_driver[] = { .get_sset_count = kszphy_get_sset_count, .get_strings = kszphy_get_strings, .get_stats = kszphy_get_stats, - /* No suspend/resume callbacks because of errata DS80000700A, - * receiver error following software power down. + /* Because of errata DS80000700A, receiver error following software + * power down. Suspend and resume callbacks only disable and enable + * external rmii reference clock. */ + .suspend = ksz8041_suspend, + .resume = ksz8041_resume, }, { .phy_id = PHY_ID_KSZ8041RNLI, .phy_id_mask = MICREL_PHY_ID_MASK, @@ -5507,7 +5596,7 @@ static struct phy_driver ksphy_driver[] = { .get_sset_count = kszphy_get_sset_count, .get_strings = kszphy_get_strings, .get_stats = kszphy_get_stats, - .suspend = genphy_suspend, + .suspend = lan8804_suspend, .resume = kszphy_resume, .config_intr = lan8804_config_intr, .handle_interrupt = lan8804_handle_interrupt, @@ -5526,7 +5615,7 @@ static struct phy_driver ksphy_driver[] = { .get_strings = kszphy_get_strings, .get_stats = kszphy_get_stats, .suspend = lan8841_suspend, - .resume = genphy_resume, + .resume = lan8841_resume, .cable_test_start = lan8814_cable_test_start, .cable_test_get_status = ksz886x_cable_test_get_status, }, {
On the i.MX6ULL-14x14-EVK board, enet1_ref and enet2_ref are used as the clock sources for two external KSZ PHYs. However, after closing the two FEC ports, the clk_enable_count of the enet1_ref and enet2_ref clocks is not 0. The root cause is that since the commit 985329462723 ("net: phy: micrel: use devm_clk_get_optional_enabled for the rmii-ref clock"), the external clock of KSZ PHY has been enabled when the PHY driver probes, and it can only be disabled when the PHY driver is removed. This causes the clock to continue working when the system is suspended or the network port is down. Although Heiko explained in the commit message that the patch was because some clock suppliers need to enable the clock to get the valid clock rate , it seems that the simple fix is to disable the clock after getting the clock rate to solve the current problem. This is indeed true, but we need to admit that Heiko's patch has been applied for more than a year, and we cannot guarantee whether there are platforms that only enable rmii-ref in the KSZ PHY driver during this period. If this is the case, disabling rmii-ref will cause RMII on these platforms to not work. Secondly, commit 99ac4cbcc2a5 ("net: phy: micrel: allow usage of generic ethernet-phy clock") just simply enables the generic clock permanently, which seems like the generic clock may only be enabled in the PHY driver. If we simply disable the generic clock, RMII may not work. If we keep it as it is, the platform using the generic clock will have the same problem as the i.MX6ULL platform. To solve this problem, the clock is enabled when phy_driver::resume() is called, and the clock is disabled when phy_driver::suspend() is called. Since phy_driver::resume() and phy_driver::suspend() are not called in pairs, an additional clk_enable flag is added. When phy_driver::suspend() is called, the clock is disabled only if clk_enable is true. Conversely, when phy_driver::resume() is called, the clock is enabled if clk_enable is false. The changes that introduced the problem were only a few lines, while the current fix is about a hundred lines, which seems out of proportion, but it is necessary because kszphy_probe() is used by multiple KSZ PHYs and we need to fix all of them. Fixes: 985329462723 ("net: phy: micrel: use devm_clk_get_optional_enabled for the rmii-ref clock") Fixes: 99ac4cbcc2a5 ("net: phy: micrel: allow usage of generic ethernet-phy clock") Signed-off-by: Wei Fang <wei.fang@nxp.com> --- v1 link: https://lore.kernel.org/imx/20241125022906.2140428-1-wei.fang@nxp.com/ v2 changes: only refine the commit message. v2 link: https://lore.kernel.org/imx/20241202084535.2520151-1-wei.fang@nxp.com/ v3 changes: disable clock after getting the clock rate in kszphy_probe() v3 link: https://lore.kernel.org/imx/20241206012113.437029-1-wei.fang@nxp.com/ v4 changes: add more detailed explanation to the commit message. --- drivers/net/phy/micrel.c | 101 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 95 insertions(+), 6 deletions(-)