| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20210601114854.GT30436@shell.armlinux.org.uk>
Date: Tue, 1 Jun 2021 12:48:54 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Joakim Zhang <qiangqing.zhang@....com>
Cc: davem@...emloft.net, kuba@...nel.org, robh+dt@...nel.org,
andrew@...n.ch, hkallweit1@...il.com, f.fainelli@...il.com,
linux-imx@....com, netdev@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.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@....com>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@....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!
Powered by blists - more mailing lists