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

[Wei Fang]

> -----Original Message-----
> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: 2024年12月13日 18:45
> To: Wei Fang <wei.fang@xxxxxxx>
> Cc: hkallweit1@xxxxxxxxx; linux@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx;
> edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx;
> florian.fainelli@xxxxxxxxxxxx; heiko.stuebner@xxxxxxxxx; Frank Li
> <frank.li@xxxxxxx>; netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> imx@xxxxxxxxxxxxxxx
> 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