| Message ID | 20241120094903.94-2-paul.barker.ct@bp.renesas.com |
|---|---|
| State | New |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | [v2,1/4] net: phy: Port set/clear bits from Linux | expand |
Hi Paul, On 11/20/24 10:49 AM, Paul Barker wrote: > Micrel KSZ9131 PHY LED behavior is not correct when configured in > Individual Mode, LED1 (Activity LED) is in the ON state when there is > no-link. > > Workaround this by setting bit 9 of register 0x1e after verifying that > the LED configuration is Individual Mode. > > This issue is described in KSZ9131RNX Silicon Errata DS80000693B [*] > and according to that it will not be corrected in a future silicon > revision. > > [*] https://ww1.microchip.com/downloads/en/DeviceDoc/KSZ9131RNX-Silicon-Errata-and-Data-Sheet-Clarification-80000863B.pdf > > Based on commit 0316c7e66bbd in the Linux kernel. > > Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com> > --- > Changes v1->v2: > - Split out of series adding RZ/G2L Ethernet support [1] > - Add symbols KSZ9131RN_COMMON_CTRL, KSZ9131RN_COMMON_CTRL_LED_MODE, > KSZ9131RN_LED_ERRATA_REG, KSZ9131RN_LED_ERRATA_BITS > > [1]: https://lore.kernel.org/all/20241024152448.102-1-paul.barker.ct@bp.renesas.com/ > > drivers/net/phy/micrel_ksz90x1.c | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/drivers/net/phy/micrel_ksz90x1.c b/drivers/net/phy/micrel_ksz90x1.c > index c48ae6e88f30..b33457910795 100644 > --- a/drivers/net/phy/micrel_ksz90x1.c > +++ b/drivers/net/phy/micrel_ksz90x1.c > @@ -389,6 +389,12 @@ U_BOOT_PHY_DRIVER(ksz9031) = { > #define KSZ9131RN_DLL_ENABLE_DELAY 0 > #define KSZ9131RN_DLL_DISABLE_DELAY BIT(12) > > +#define KSZ9131RN_COMMON_CTRL 0 > +#define KSZ9131RN_COMMON_CTRL_LED_MODE BIT(4) > + Can you rename or add a new symbol to explain what is what? This implies a mask basically but not what the mask being set means. Can suggest something like: KSZ9131RN_COMMON_CTRL_INDIVIDUAL_LED_MODE .... > +#define KSZ9131RN_LED_ERRATA_REG 0x1e > +#define KSZ9131RN_LED_ERRATA_BITS BIT(9) Yet we only set one bit, remove BITS or rename to BIT? > + > static int ksz9131_config_rgmii_delay(struct phy_device *phydev) > { > struct phy_driver *drv = phydev->drv; > @@ -436,6 +442,28 @@ static int ksz9131_config_rgmii_delay(struct phy_device *phydev) > return ret; > } > > +/* Silicon Errata DS80000693B > + * > + * When LEDs are configured in Individual Mode, LED1 is ON in a no-link > + * condition. Workaround is to set register 0x1e, bit 9, this way LED1 behaves > + * according to the datasheet (off if there is no link). > + */ > +static int ksz9131_led_errata(struct phy_device *phydev) > +{ > + int reg; > + > + reg = phy_read_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG, > + KSZ9131RN_COMMON_CTRL); > + if (reg < 0) > + return reg; > + > + if (!(reg & KSZ9131RN_COMMON_CTRL_LED_MODE)) .... or at the very least add a clear comment here what we're after :) (yes it's commented at the top of the function but right before the check is nice too I believe). > + return 0; > + > + return phy_set_bits(phydev, MDIO_DEVAD_NONE, KSZ9131RN_LED_ERRATA_REG, > + KSZ9131RN_LED_ERRATA_BITS); > +} > + > static int ksz9131_config(struct phy_device *phydev) > { > int ret; > @@ -446,6 +474,10 @@ static int ksz9131_config(struct phy_device *phydev) > return ret; > } > > + ret = ksz9131_led_errata(phydev); > + if (ret < 0) > + return ret; > + This seems to work but is happening a bit late. Indeed, I need something to call the network stack for this to be happening. Meaning until I e.g. run `dhcp` in the CLI, the LED is still following the bad behavior. I assume we cannot use the probe() callback for writing to registers? Tested-by: Quentin Schulz <quentin.schulz@cherry.de> # RK3588 Tiger Thanks! Quentin
On 11/25/24 6:23 PM, Quentin Schulz wrote: [...] >> @@ -446,6 +474,10 @@ static int ksz9131_config(struct phy_device *phydev) >> return ret; >> } >> + ret = ksz9131_led_errata(phydev); >> + if (ret < 0) >> + return ret; >> + > > This seems to work but is happening a bit late. > > Indeed, I need something to call the network stack for this to be > happening. Meaning until I e.g. run `dhcp` in the CLI, the LED is still > following the bad behavior. > > I assume we cannot use the probe() callback for writing to registers? I'm afraid there is an inconsistency between U-Boot MAC drivers and when they configure the PHY. This is well visible on MX8MP, which has both FEC and EQoS/DWMAC MACs and they behave differently in this aspect. The question is, whether it makes sense to initialize the PHY before the MAC has been used, probably no, because that brings up hardware and adds to boot time. But at least aligning the behavior of the various MAC drivers would be nice.
Hi Marek, Quentin, On 01/12/2024 18:57, Marek Vasut wrote: > On 11/25/24 6:23 PM, Quentin Schulz wrote: > > [...] > >>> @@ -446,6 +474,10 @@ static int ksz9131_config(struct phy_device *phydev) >>> return ret; >>> } >>> + ret = ksz9131_led_errata(phydev); >>> + if (ret < 0) >>> + return ret; >>> + >> >> This seems to work but is happening a bit late. >> >> Indeed, I need something to call the network stack for this to be >> happening. Meaning until I e.g. run `dhcp` in the CLI, the LED is still >> following the bad behavior. >> >> I assume we cannot use the probe() callback for writing to registers? > I'm afraid there is an inconsistency between U-Boot MAC drivers and when > they configure the PHY. This is well visible on MX8MP, which has both > FEC and EQoS/DWMAC MACs and they behave differently in this aspect. The > question is, whether it makes sense to initialize the PHY before the MAC > has been used, probably no, because that brings up hardware and adds to > boot time. But at least aligning the behavior of the various MAC drivers > would be nice. Is there anything we can do in this driver to fix this? Or should we merge this as-is and improve the MAC drivers later? Thanks,
Hi Paul, On 12/5/24 7:58 PM, Paul Barker wrote: > Hi Marek, Quentin, > > On 01/12/2024 18:57, Marek Vasut wrote: >> On 11/25/24 6:23 PM, Quentin Schulz wrote: >> >> [...] >> >>>> @@ -446,6 +474,10 @@ static int ksz9131_config(struct phy_device *phydev) >>>> return ret; >>>> } >>>> + ret = ksz9131_led_errata(phydev); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>> >>> This seems to work but is happening a bit late. >>> >>> Indeed, I need something to call the network stack for this to be >>> happening. Meaning until I e.g. run `dhcp` in the CLI, the LED is still >>> following the bad behavior. >>> >>> I assume we cannot use the probe() callback for writing to registers? >> I'm afraid there is an inconsistency between U-Boot MAC drivers and when >> they configure the PHY. This is well visible on MX8MP, which has both >> FEC and EQoS/DWMAC MACs and they behave differently in this aspect. The >> question is, whether it makes sense to initialize the PHY before the MAC >> has been used, probably no, because that brings up hardware and adds to >> boot time. But at least aligning the behavior of the various MAC drivers >> would be nice. > > Is there anything we can do in this driver to fix this? > > Or should we merge this as-is and improve the MAC drivers later? > This patch doesn't change anything or rely on some specific behavior so I'm of the opinion to merge it. Making the behavior of all MAC drivers consistent would be nice but has nothing to do with the PHY driver here I believe. Cheers, Quentin
On 12/5/24 7:58 PM, Paul Barker wrote: > Hi Marek, Quentin, > > On 01/12/2024 18:57, Marek Vasut wrote: >> On 11/25/24 6:23 PM, Quentin Schulz wrote: >> >> [...] >> >>>> @@ -446,6 +474,10 @@ static int ksz9131_config(struct phy_device *phydev) >>>> return ret; >>>> } >>>> + ret = ksz9131_led_errata(phydev); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>> >>> This seems to work but is happening a bit late. >>> >>> Indeed, I need something to call the network stack for this to be >>> happening. Meaning until I e.g. run `dhcp` in the CLI, the LED is still >>> following the bad behavior. >>> >>> I assume we cannot use the probe() callback for writing to registers? >> I'm afraid there is an inconsistency between U-Boot MAC drivers and when >> they configure the PHY. This is well visible on MX8MP, which has both >> FEC and EQoS/DWMAC MACs and they behave differently in this aspect. The >> question is, whether it makes sense to initialize the PHY before the MAC >> has been used, probably no, because that brings up hardware and adds to >> boot time. But at least aligning the behavior of the various MAC drivers >> would be nice. > > Is there anything we can do in this driver to fix this? > > Or should we merge this as-is and improve the MAC drivers later? I'd say merge it for 2025.04 , since this is an errata fix, but it would also be able to clean up the MAC driver mess eventually.
diff --git a/drivers/net/phy/micrel_ksz90x1.c b/drivers/net/phy/micrel_ksz90x1.c index c48ae6e88f30..b33457910795 100644 --- a/drivers/net/phy/micrel_ksz90x1.c +++ b/drivers/net/phy/micrel_ksz90x1.c @@ -389,6 +389,12 @@ U_BOOT_PHY_DRIVER(ksz9031) = { #define KSZ9131RN_DLL_ENABLE_DELAY 0 #define KSZ9131RN_DLL_DISABLE_DELAY BIT(12) +#define KSZ9131RN_COMMON_CTRL 0 +#define KSZ9131RN_COMMON_CTRL_LED_MODE BIT(4) + +#define KSZ9131RN_LED_ERRATA_REG 0x1e +#define KSZ9131RN_LED_ERRATA_BITS BIT(9) + static int ksz9131_config_rgmii_delay(struct phy_device *phydev) { struct phy_driver *drv = phydev->drv; @@ -436,6 +442,28 @@ static int ksz9131_config_rgmii_delay(struct phy_device *phydev) return ret; } +/* Silicon Errata DS80000693B + * + * When LEDs are configured in Individual Mode, LED1 is ON in a no-link + * condition. Workaround is to set register 0x1e, bit 9, this way LED1 behaves + * according to the datasheet (off if there is no link). + */ +static int ksz9131_led_errata(struct phy_device *phydev) +{ + int reg; + + reg = phy_read_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG, + KSZ9131RN_COMMON_CTRL); + if (reg < 0) + return reg; + + if (!(reg & KSZ9131RN_COMMON_CTRL_LED_MODE)) + return 0; + + return phy_set_bits(phydev, MDIO_DEVAD_NONE, KSZ9131RN_LED_ERRATA_REG, + KSZ9131RN_LED_ERRATA_BITS); +} + static int ksz9131_config(struct phy_device *phydev) { int ret; @@ -446,6 +474,10 @@ static int ksz9131_config(struct phy_device *phydev) return ret; } + ret = ksz9131_led_errata(phydev); + if (ret < 0) + return ret; + /* add an option to disable the gigabit feature of this PHY */ if (env_get("disable_giga")) { unsigned features;
Micrel KSZ9131 PHY LED behavior is not correct when configured in Individual Mode, LED1 (Activity LED) is in the ON state when there is no-link. Workaround this by setting bit 9 of register 0x1e after verifying that the LED configuration is Individual Mode. This issue is described in KSZ9131RNX Silicon Errata DS80000693B [*] and according to that it will not be corrected in a future silicon revision. [*] https://ww1.microchip.com/downloads/en/DeviceDoc/KSZ9131RNX-Silicon-Errata-and-Data-Sheet-Clarification-80000863B.pdf Based on commit 0316c7e66bbd in the Linux kernel. Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com> --- Changes v1->v2: - Split out of series adding RZ/G2L Ethernet support [1] - Add symbols KSZ9131RN_COMMON_CTRL, KSZ9131RN_COMMON_CTRL_LED_MODE, KSZ9131RN_LED_ERRATA_REG, KSZ9131RN_LED_ERRATA_BITS [1]: https://lore.kernel.org/all/20241024152448.102-1-paul.barker.ct@bp.renesas.com/ drivers/net/phy/micrel_ksz90x1.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)