RE: [PATCH net-next 2/4] net: phy: realtek: add dt property to disable CLKOUT clock
From: Joakim Zhang <hidden>
Date: 2021-06-02 02:19:11
Also in:
lkml, netdev
Hi Russell, Thanks for your reviewing.
-----Original Message----- From: Russell King <linux@armlinux.org.uk> Sent: 2021年6月1日 19:49 To: Joakim Zhang <redacted> Cc: davem@davemloft.net; kuba@kernel.org; robh+dt@kernel.org; andrew@lunn.ch; hkallweit1@gmail.com; f.fainelli@gmail.com; dl-linux-imx [off-list ref]; 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:quoted
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 <redacted> Signed-off-by: Joakim Zhang <redacted> --- 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("Johnsonquoted
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@@quoted
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)quoted
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.
quoted
+ 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.
quoted
+ 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!