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&data=04%7C01%7Cqiangqin
> g.zhang%40nxp.com%7Ca2666831ffdc43a0a27908d924f33ee9%7C686ea1d3b
> c2b4c6fa92cd99c5c301635%7C0%7C0%7C637581449420768288%7CUnknown
> %7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haW
> wiLCJXVCI6Mn0%3D%7C1000&sdata=4SFmi5Fst3r7jPzy%2F%2B%2Bej88
> GN7N91e1DxD7GyrA2dig%3D&reserved=0
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
