| Message ID | 20210601090408.22025-1-qiangqing.zhang@nxp.com |
|---|---|
| Headers | show |
| Series | net: phy: add dt property for realtek phy | expand |
On Tue, Jun 01, 2021 at 05:04:07PM +0800, Joakim Zhang wrote: > @@ -325,8 +329,10 @@ static int rtl8211f_config_init(struct phy_device *phydev) > u16 val; > int ret; > > - val = RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_PLL_OFF | RTL8211F_ALDPS_XTAL_OFF; > - phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1, val, val); > + if (!(priv->quirks & RTL821X_ALDPS_DISABLE_FEATURE)) { > + val = RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_PLL_OFF | RTL8211F_ALDPS_XTAL_OFF; > + phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1, val, val); > + } Similar questions as with the previous patch, but also... this doesn't actually disable the feature if it was previously turned on. E.g. a kexec() from a current kernel that has set these features into a subsequent kernel that the DT requests the feature to be disabled. Or a boot loader that has enabled this feature. If DT specifies that this feature is disabled, shouldn't this code be disabling it explicitly?
Hi Russell, > -----Original Message----- > From: Russell King <linux@armlinux.org.uk> > Sent: 2021年6月1日 19:52 > 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 3/4] net: phy: realteck: add dt property to disable > ALDPS mode > > On Tue, Jun 01, 2021 at 05:04:07PM +0800, Joakim Zhang wrote: > > @@ -325,8 +329,10 @@ static int rtl8211f_config_init(struct phy_device > *phydev) > > u16 val; > > int ret; > > > > - val = RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_PLL_OFF | > RTL8211F_ALDPS_XTAL_OFF; > > - phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1, val, val); > > + if (!(priv->quirks & RTL821X_ALDPS_DISABLE_FEATURE)) { > > + val = RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_PLL_OFF | > RTL8211F_ALDPS_XTAL_OFF; > > + phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1, val, > val); > > + } > > Similar questions as with the previous patch, but also... this doesn't actually > disable the feature if it was previously turned on. E.g. a > kexec() from a current kernel that has set these features into a subsequent > kernel that the DT requests the feature to be disabled. Or a boot loader that > has enabled this feature. Sorry, I don't know what your meanings. As I explained before, boot loader also can configure PHY registers, but kernel should not depend on what boot loader did. If you use kexec() to boot kernel with DT request to disable this clock, PHY probe also can parse this property to disable it. I know little about kexec(), could you please explain more if I misunderstand? > If DT specifies that this feature is disabled, shouldn't this code be disabling it > explicitly? Yes, exactly should. Best Regards, Joakim Zhang > -- > 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%7Ceacabd25941448301acb08d924f3b6de%7C686ea1d3b > c2b4c6fa92cd99c5c301635%7C0%7C0%7C637581451436345731%7CUnknown > %7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haW > wiLCJXVCI6Mn0%3D%7C1000&sdata=3jyYeGVBXXb5jCDtyTrt3DI9MwVE > OT5Et9tpCZG26gY%3D&reserved=0 > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> +properties: > + rtl821x,clkout-disable: > + description: Disable CLKOUT clock. > + type: boolean > + > + rtl821x,aldps-disable: > + description: Disable ALDPS mode. > + type: boolean I think most of the problems are the ambiguity in the binding. If rtl821x,clkout-disable is not present, should it enable the CLKOUT? That needs clear define here. Do we actually want a tristate here? rtl821x,clkout = <true>; means ensure the clock is outputting. rtl821x,clkout = <false>; means ensure the clock is not outputting. And if the property is not in DT at all, leave the hardware alone, at either its default value, or whatever came before has set it to? Andrew
Hi Andrew, > -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: 2021年6月2日 10:39 > 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 1/4] dt-bindings: net: add dt binding for realtek > rtl82xx phy > > > +properties: > > + rtl821x,clkout-disable: > > + description: Disable CLKOUT clock. > > + type: boolean > > + > > + rtl821x,aldps-disable: > > + description: Disable ALDPS mode. > > + type: boolean > > I think most of the problems are the ambiguity in the binding. > > If rtl821x,clkout-disable is not present, should it enable the CLKOUT? > That needs clear define here. No, don't need to, CLKOUT clock default is enabled after PHY hardware reset. Add this property for users request to disable this clock output. I will improve the description. > Do we actually want a tristate here? > > rtl821x,clkout = <true>; > > means ensure the clock is outputting. > > rtl821x,clkout = <false>; > > means ensure the clock is not outputting. I think it's unnecessary. A boolean type here is enough. > And if the property is not in DT at all, leave the hardware alone, at either its > default value, or whatever came before has set it to? Seems not. 1. If enable CLKOUT in boot loader or keep the hardware default value (CLKOUT enabled), DT would work with this patch. 2. If disable CLKOUT in boot loader, with this patch, driver would enable this clock if this property is not in DT. So, I need first read PHYCR2 register, if DT has property then disable the clock, if not, keep the original value? However, for ALDPS mode, the hardware default value is disabled. The driver enable ALDPS by default which caused issue at my side. So need a property to disable it. We had better add a property like " rtl821x,aldps-enable", but It seems break the existing behavior. Best Regards, Joakim Zhang > Andrew