| Message ID | 20241202084535.2520151-1-wei.fang@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,net] net: phy: micrel: Dynamically control external clock of KSZ PHY | expand |
On Mon, Dec 02, 2024 at 04:45:35PM +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. The referenced commit is a one liner: @@ -2001,7 +2001,7 @@ static int kszphy_probe(struct phy_device *phydev) kszphy_parse_led_mode(phydev); - clk = devm_clk_get(&phydev->mdio.dev, "rmii-ref"); + clk = devm_clk_get_optional_enabled(&phydev->mdio.dev, "rmii-ref"); /* NOTE: clk may be NULL if building without CONFIG_HAVE_CLK */ and here you are adding 103 lines as a fix. This seems out of proportion. Did this truly work before this change? The commit message says: While the external clock input will most likely be enabled, it's not guaranteed and clk_get_rate in some suppliers will even just return valid results when the clock is running. So it seems like a much simpler fix is to put a clock_enable/clock_disable around clk_get_rate. Andrew
> -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: 2024年12月2日 22:49 > 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 v2 net] net: phy: micrel: Dynamically control external clock > of KSZ PHY > > On Mon, Dec 02, 2024 at 04:45:35PM +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. > > The referenced commit is a one liner: > > @@ -2001,7 +2001,7 @@ static int kszphy_probe(struct phy_device *phydev) > > kszphy_parse_led_mode(phydev); > > - clk = devm_clk_get(&phydev->mdio.dev, "rmii-ref"); > + clk = devm_clk_get_optional_enabled(&phydev->mdio.dev, > + "rmii-ref"); > /* NOTE: clk may be NULL if building without CONFIG_HAVE_CLK */ > > and here you are adding 103 lines as a fix. This seems out of proportion. Did this > truly work before this change? For i.MX6ULL-14X14-EVK, the RMII reference clock is provided by the SoC and enabled by FEC driver. Before the commit 985329462723, we can see that the clk_enable_count of enet1_ref and enet2_ref is 1 when the FEC ports are up, and the count is 0 when the ports are down. After the commit 985329462723, the clk_enable_count is 2 when then ports are up, because the KSZ PHY driver enables the clock as well, so the count is 2. But the PHY driver does not disable clock except the PHY driver is removed. That is to say, the RMII reference clock is always working no matter the port is down or the system enters suspend state. > > The commit message says: > > While the external clock input will most likely be enabled, it's not > guaranteed and clk_get_rate in some suppliers will even just return > valid results when the clock is running. > > So it seems like a much simpler fix is to put a clock_enable/clock_disable around > clk_get_rate. > Sorry, I was not aware of that some suppliers need to be enabled so that clk_get_rate() can get the correct clk rate. So we can keep devm_clk_get_optional_enabled() to get the clk rate and then disable the clock. In addition, there is another situation that the clock is only enabled by the PHY driver. In this case, my current modification is necessary. Of course, this is a very corner case, after all, the clock was enabled by other drivers before commit 985329462723. But after this commit, this possibility exists. So what do you think? Should I simply disable the clock after getting the clk rate? Or should I add the following modifications based on my current patch? @@ -2245,7 +2245,7 @@ static int kszphy_probe(struct phy_device *phydev) kszphy_parse_led_mode(phydev); - clk = devm_clk_get_optional(&phydev->mdio.dev, "rmii-ref"); + clk = devm_clk_get_optional_enabled(&phydev->mdio.dev, "rmii-ref"); /* NOTE: clk may be NULL if building without CONFIG_HAVE_CLK */ if (!IS_ERR_OR_NULL(clk)) { unsigned long rate = clk_get_rate(clk); @@ -2267,13 +2267,15 @@ static int kszphy_probe(struct phy_device *phydev) } } else if (!clk) { /* unnamed clock from the generic ethernet-phy binding */ - clk = devm_clk_get_optional(&phydev->mdio.dev, NULL); + clk = devm_clk_get_optional_enabled(&phydev->mdio.dev, NULL); if (IS_ERR(clk)) return PTR_ERR(clk); } - if (!IS_ERR_OR_NULL(clk)) + if (!IS_ERR_OR_NULL(clk)) { priv->clk = clk; + clk_disable_unprepare(priv->clk); + }
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index 3ef508840674..44577b5d48d5 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) @@ -2194,7 +2245,7 @@ static int kszphy_probe(struct phy_device *phydev) kszphy_parse_led_mode(phydev); - clk = devm_clk_get_optional_enabled(&phydev->mdio.dev, "rmii-ref"); + clk = devm_clk_get_optional(&phydev->mdio.dev, "rmii-ref"); /* NOTE: clk may be NULL if building without CONFIG_HAVE_CLK */ if (!IS_ERR_OR_NULL(clk)) { unsigned long rate = clk_get_rate(clk); @@ -2216,11 +2267,14 @@ static int kszphy_probe(struct phy_device *phydev) } } else if (!clk) { /* unnamed clock from the generic ethernet-phy binding */ - clk = devm_clk_get_optional_enabled(&phydev->mdio.dev, NULL); + clk = devm_clk_get_optional(&phydev->mdio.dev, NULL); if (IS_ERR(clk)) return PTR_ERR(clk); } + if (!IS_ERR_OR_NULL(clk)) + priv->clk = clk; + if (ksz8041_fiber_mode(phydev)) phydev->port = PORT_FIBRE; @@ -5290,15 +5344,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 +5442,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 +5594,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 +5613,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. 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. 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. --- drivers/net/phy/micrel.c | 103 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 95 insertions(+), 8 deletions(-)