[PATCH v2 2/4] net: phy: ksz90x1: Handle ksz9131 LED errata

[Quentin Schulz]

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 at 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 at cherry.de> # RK3588 Tiger

Thanks!
Quentin