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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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@nxp.com>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  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");

> +			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.

> +			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;
	}

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?
- Do we really need the use of negative logic here (which depends on
  the answer to the above question)?
- 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.

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!