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...@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...@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.sch...@cherry.de> # RK3588 Tiger
Thanks!
Quentin
