Re: [PATCH net] net: phy: micrel: Dynamically control external clock of KSZ PHY

[Frank Li <Frank.li@nxp.com>]

On Mon, Nov 25, 2024 at 10:29:05AM +0800, Wei Fang wrote:
> On the i.MX6ULL-14x14-EVK board, when using the 6.12 kernel, it is found
> that after disabling the two network ports, the clk_enable_count of the
> enet1_ref and enet2_ref clocks is not 0 (these two clocks are used as the
> clock source of the RMII reference clock of the two KSZ8081 PHYs), but
> there is no such problem in the 6.6 kernel.

skip your debug progress, just descript the problem itself.

>
> After analysis, we found 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 resume() of the PHY
> driver is called, and the clock is disabled when suspend() is called.
> Since the PHY driver's resume() and suspend() interfaces are not called
> in pairs, an additional clk_enable flag is added. When suspend() is

Why  resume() and suspend() is not call paired?


> called, the clock is disabled only if clk_enable is true. Conversely,
> when 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>
> ---
>  drivers/net/phy/micrel.c | 103 ++++++++++++++++++++++++++++++++++++---
>  1 file changed, 95 insertions(+), 8 deletions(-)
>
> 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;
> +	}
> +}

Generally, clock not check enable status, just call enable/disable pair.

Frank

> +
>  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,
>  }, {
> --
> 2.34.1
>