RE: [PATCH net-next 2/4] net: phy: realtek: add dt property to disable CLKOUT clock

[Joakim Zhang]

Hi Russell,

Thanks for your reviewing.

> -----Original Message-----
> From: Russell King <linux@xxxxxxxxxxxxxxx>
> Sent: 2021年6月1日 19:49
> To: Joakim Zhang <qiangqing.zhang@xxxxxxx>
> Cc: davem@xxxxxxxxxxxxx; kuba@xxxxxxxxxx; robh+dt@xxxxxxxxxx;
> andrew@xxxxxxx; hkallweit1@xxxxxxxxx; f.fainelli@xxxxxxxxx; dl-linux-imx
> <linux-imx@xxxxxxx>; netdev@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH net-next 2/4] net: phy: realtek: add dt property to disable
> CLKOUT clock
> 
> On Tue, Jun 01, 2021 at 05:04:06PM +0800, Joakim Zhang wrote:
> > Add "rtl821x,clkout-disable" property for user to disable CLKOUT clock
> > to save PHY power.
> >
> > Per RTL8211F guide, a PHY reset should be issued after setting these
> > bits in PHYCR2 register. After this patch, CLKOUT clock output to be
> > disabled.
> >
> > Signed-off-by: Fugang Duan <fugang.duan@xxxxxxx>
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@xxxxxxx>
> > ---
> >  drivers/net/phy/realtek.c | 48
> > ++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 47 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> > index 821e85a97367..4219c23ff2b0 100644
> > --- a/drivers/net/phy/realtek.c
> > +++ b/drivers/net/phy/realtek.c
> > @@ -8,6 +8,7 @@
> >   * Copyright (c) 2004 Freescale Semiconductor, Inc.
> >   */
> >  #include <linux/bitops.h>
> > +#include <linux/of.h>
> >  #include <linux/phy.h>
> >  #include <linux/module.h>
> >  #include <linux/delay.h>
> > @@ -27,6 +28,7 @@
> >  #define RTL821x_PAGE_SELECT			0x1f
> >
> >  #define RTL8211F_PHYCR1				0x18
> > +#define RTL8211F_PHYCR2				0x19
> >  #define RTL8211F_INSR				0x1d
> >
> >  #define RTL8211F_TX_DELAY			BIT(8)
> > @@ -40,6 +42,8 @@
> >  #define RTL8211E_TX_DELAY			BIT(12)
> >  #define RTL8211E_RX_DELAY			BIT(11)
> >
> > +#define RTL8211F_CLKOUT_EN			BIT(0)
> > +
> >  #define RTL8201F_ISR				0x1e
> >  #define RTL8201F_ISR_ANERR			BIT(15)
> >  #define RTL8201F_ISR_DUPLEX			BIT(13)
> > @@ -67,10 +71,17 @@
> >
> >  #define RTL_GENERIC_PHYID			0x001cc800
> >
> > +/* quirks for realtek phy */
> > +#define RTL821X_CLKOUT_DISABLE_FEATURE		BIT(0)
> > +
> >  MODULE_DESCRIPTION("Realtek PHY driver");
> MODULE_AUTHOR("Johnson
> > Leung");  MODULE_LICENSE("GPL");
> >
> > +struct rtl821x_priv {
> > +	u32 quirks;
> > +};
> > +
> >  static int rtl821x_read_page(struct phy_device *phydev)  {
> >  	return __phy_read(phydev, RTL821x_PAGE_SELECT); @@ -81,6 +92,23
> @@
> > static int rtl821x_write_page(struct phy_device *phydev, int page)
> >  	return __phy_write(phydev, RTL821x_PAGE_SELECT, page);  }
> >
> > +static int rtl821x_probe(struct phy_device *phydev) {
> > +	struct device *dev = &phydev->mdio.dev;
> > +	struct rtl821x_priv *priv;
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	if (of_property_read_bool(dev->of_node, "rtl821x,clkout-disable"))
> > +		priv->quirks |= RTL821X_CLKOUT_DISABLE_FEATURE;
> > +
> > +	phydev->priv = priv;
> > +
> > +	return 0;
> > +}
> > +
> >  static int rtl8201_ack_interrupt(struct phy_device *phydev)  {
> >  	int err;
> > @@ -291,6 +319,7 @@ static int rtl8211c_config_init(struct phy_device
> > *phydev)
> >
> >  static int rtl8211f_config_init(struct phy_device *phydev)  {
> > +	struct rtl821x_priv *priv = phydev->priv;
> >  	struct device *dev = &phydev->mdio.dev;
> >  	u16 val_txdly, val_rxdly;
> >  	u16 val;
> > @@ -354,7 +383,23 @@ static int rtl8211f_config_init(struct phy_device
> *phydev)
> >  			val_rxdly ? "enabled" : "disabled");
> >  	}
> >
> > -	return 0;
> > +	if (priv->quirks & RTL821X_CLKOUT_DISABLE_FEATURE) {
> > +		ret = phy_modify_paged(phydev, 0xa43, RTL8211F_PHYCR2,
> > +				       RTL8211F_CLKOUT_EN, 0);
> > +		if (ret < 0) {
> > +			dev_err(&phydev->mdio.dev, "clkout disable failed\n");
> 
> You already have "dev" above, so this can be simplified to:
> 
> 			dev_err(dev, "clkout disable failed\n");
Yes. Will fix it.

> > +			return ret;
> > +		}
> > +	} else {
> > +		ret = phy_modify_paged(phydev, 0xa43, RTL8211F_PHYCR2,
> > +				       RTL8211F_CLKOUT_EN, RTL8211F_CLKOUT_EN);
> > +		if (ret < 0) {
> > +			dev_err(&phydev->mdio.dev, "clkout enable failed\n");
> 
> Same here.
Yes.

> > +			return ret;
> > +		}
> > +	}
> 
> Do you really need to distinguish between the enable and disable operation?
> Would the following be better?
> 
> 	if (priv->quirks & RTL821X_CLKOUT_DISABLE_FEATURE)
> 		clkout = 0;
> 	else
> 		clkout = RTL8211_CLKOUT_EN;
> 
> 	ret = phy_modify_paged(phydev, 0xa43, RTL8211F_PHYCR2,
> 			       RTL8211_CLKOUT_EN, clkout);
> 	if (ret < 0) {
> 		dev_err(dev, "clkout configuration failed: %pe\n",
> 			ERR_PTR(ret));
> 		return ret;
> 	}
Thanks, more clear, will improve it.

> Other questions:
> - Does the clock output default to enabled or disabled during kernel
>   boot without this patch? Does this state depend on the boot loader?
CLKOUT clock default is enabled after PHY reset. What the meaning of depending on the boot loader? I think also can enable it in boot loader.

> - Do we really need the use of negative logic here (which depends on
>   the answer to the above question)?
Yes, we need, Since the CLKOUT default is enabled, some board design may need this clock feed MAC, so we shouldn't
break existing design. 

> - Could other bits in the RTL8211F_PHYCR2 register also require future
>   configuration? Would it be better to store the desired PHYCR2
>   register value in "priv" rather than using "quirks". Quirks can become
>   very unweildy over time.
Make sense, it seems more universal.

> By way of example on the last point... probe() would become:
> 
> 	priv->phycr2 = RTL8211_CLKOUT_EN;
> 
> 	if (of_property_read_bool(dev->of_node, "rtl821x,clkout-disable"))
> 		priv->phycr2 &= ~RTL8211_CLKOUT_EN;
> 
> and config_init():
> 
> 	ret = phy_modify_paged(phydev, 0xa43, RTL8211F_PHYCR2,
> 			       RTL8211_CLKOUT_EN, priv->phycr2);
> 	if (ret < 0) {
> 		dev_err(dev, "clkout configuration failed: %pe\n",
> 			ERR_PTR(ret));
> 		return ret;
> 	}
> 
> Note that phycr2 would be a u16 value.
Thanks.

Best Regards,
Joakim Zhang
> Thanks.
> 
> --
> RMK's Patch system:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.ar
> mlinux.org.uk%2Fdeveloper%2Fpatches%2F&amp;data=04%7C01%7Cqiangqin
> g.zhang%40nxp.com%7Ca2666831ffdc43a0a27908d924f33ee9%7C686ea1d3b
> c2b4c6fa92cd99c5c301635%7C0%7C0%7C637581449420768288%7CUnknown
> %7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haW
> wiLCJXVCI6Mn0%3D%7C1000&amp;sdata=4SFmi5Fst3r7jPzy%2F%2B%2Bej88
> GN7N91e1DxD7GyrA2dig%3D&amp;reserved=0
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!