| Message ID | 20210601090408.22025-3-qiangqing.zhang@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | net: phy: add dt property for realtek phy | expand |
Hi Russell, Thanks for your reviewing. > -----Original Message----- > From: Russell King <linux@armlinux.org.uk> > Sent: 2021年6月1日 19:49 > To: Joakim Zhang <qiangqing.zhang@nxp.com> > Cc: davem@davemloft.net; kuba@kernel.org; robh+dt@kernel.org; > andrew@lunn.ch; hkallweit1@gmail.com; f.fainelli@gmail.com; dl-linux-imx > <linux-imx@nxp.com>; netdev@vger.kernel.org; devicetree@vger.kernel.org; > linux-kernel@vger.kernel.org > 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@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"); 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!
Hi Andrew, > -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: 2021年6月2日 6:08 > To: Joakim Zhang <qiangqing.zhang@nxp.com> > Cc: davem@davemloft.net; kuba@kernel.org; robh+dt@kernel.org; > hkallweit1@gmail.com; linux@armlinux.org.uk; f.fainelli@gmail.com; > dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH net-next 2/4] net: phy: realtek: add dt property to disable > CLKOUT clock > > > +struct rtl821x_priv { > > + u32 quirks; > > I'm not sure quirks is the correct word here. I would probably use features, or > flags. Ok, I will change to flags. As Russell king suggests another solution, which would you prefer to? I need a conclusion so that I can send out the V2, thanks. Best Regards, Joakim Zhang > Andrew
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"); + 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"); + return ret; + } + } + + return genphy_soft_reset(phydev); } static int rtl8211e_config_init(struct phy_device *phydev) @@ -847,6 +892,7 @@ static struct phy_driver realtek_drvs[] = { }, { PHY_ID_MATCH_EXACT(0x001cc916), .name = "RTL8211F Gigabit Ethernet", + .probe = rtl821x_probe, .config_init = &rtl8211f_config_init, .read_status = rtlgen_read_status, .config_intr = &rtl8211f_config_intr,