| Message ID | 20210601090408.22025-3-qiangqing.zhang@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | net: phy: add dt property for realtek phy | expand |
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.
> +struct rtl821x_priv { > + u32 quirks; I'm not sure quirks is the correct word here. I would probably use features, or flags. Andrew
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,