Message-ID: <CAJ+vNU2c+kt3vX6PzqALakdwzyb5qh6O7Oj4xyxsWnsXCAUaRw@mail.gmail.com>
Date: Fri, 30 Jan 2026 13:46:52 -0800
From: Tim Harvey <tharvey@...eworks.com>
To: Heinrich Toews <ht@...-software.de>, netdev <netdev@...r.kernel.org>
Cc: Andrew Lunn <andrew@...n.ch>, Florian Fainelli <f.fainelli@...il.com>, 
	Vladimir Oltean <olteanv@...il.com>, Rob Herring <robh+dt@...nel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>
Subject: Re: [v1,3/3] net: dsa: microchip: add single-led-mode support for KSZ9477

>
> diff --git a/drivers/net/dsa/microchip/ksz_common.c
> b/drivers/net/dsa/microchip/ksz_common.c
> index e5fa1f5fc09b3..bef8ec51d9483 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -815,6 +815,9 @@  static const u16 ksz9477_regs[] = {
>   [PTP_RTC_SEC] = 0x0508,
>   [PTP_SUBNANOSEC_RATE] = 0x050C,
>   [PTP_MSG_CONF1] = 0x0514,
> + [P_PHY_MMD_SETUP] = 0x011A,
> + [P_PHY_MMD_DATA] = 0x011C,
> + [P_PHY_DIGITAL_DEBUG_3] = 0x013C,
>  };
>
> @@ -3241,6 +3244,14 @@  static u32 ksz_get_phy_flags(struct dsa_switch
> *ds, int port)
>   if (!port)
>   return MICREL_KSZ8_P1_ERRATA;
>   break;
> + case KSZ9477_CHIP_ID:
> + /* KSZ9477S Errata DS80000754F
> + *
> + * Module 19: Single-LED Mode Setting Requires Two Register Writes
> + *   The PHY Port LEDx_0 pin does not go low in the presence of link
> + *   activity when Single-LED Mode is selected in the MMD LED Mode Register.
> + */
> + return MICREL_KSZ9_LED_ERRATA;
>   }
>

Hi Heinrich,

Thanks for doing this. I have two boards I work with that have a
KSZ9897S switch and use single-led mode (imx8mp-venice-gw74xx.dts and
imx8mm-venice-gw7901.dts) that suffer from this issue. In my scenario
I take care of this in the bootloader but when ksz9477_reset_switch()
is called and sets the SW_RESET bit of REG_SW_OPERATION it resets all
registers and reverts to tri-color dual-led mode thus we really do
need a dt prop and handling for this.

You can add the errata for KSZ9897 as well here:
+       case KSZ9897_CHIP_ID:
+               /* KSZ9897S Errata DS80000759F: Module 18 */
+               return MICREL_KSZ9_LED_ERRATA;

And because this errata applies to multiple chips it's probably best
to describe the workaround once and refer to the errata # in the cases
statements.

> +static void ksz_enable_single_led_mode(struct ksz_device *dev, int
> port, struct phy_device *phydev)
> +{
> + const u16 *regs = dev->info->regs;
> +
> + ksz_pwrite16(dev, port, regs[P_PHY_MMD_SETUP], 0x0002); /* Set up
> register address for MMD */
> + ksz_pwrite16(dev, port,  regs[P_PHY_MMD_DATA], 0x0000); /* Select
> Register 00h of MMD */
> + ksz_pwrite16(dev, port, regs[P_PHY_MMD_SETUP], 0x4002); /* Select
> register data for MMD */
> + ksz_pwrite16(dev, port,  regs[P_PHY_MMD_DATA], 0x0011); /* Write
> value 0011h to MMD */

These are wrong: The MMD_SETUP and MMD_DATA regs you are using here
0x011a and 0x11c are actually 0xN11a 0x 0xN11c (where N is the port)
so they port specific so the above only would set them for port0.
Because these registers correspond to standardPHY register 0x0d and
0x0e which are defined in mii.h you can just use those.

What you want here is to use ksz_phy_write16 on the port which will
call the chip-specific w_phy which will handle writing to the port
specific register:
ksz_phy_write16(dev, port, MII_MMD_CTRL, 0x02);
ksz_phy_write16(dev, port, MII_MMD_DATA, 0x00);
ksz_phy_write16(dev, port, MII_MMD_CTRL, MII_MMD_CTRL_NOINCR | 0x02);
ksz_phy_write16(dev, port, MII_MMD_DATA, 0x10);

Note on that last one, the errata says to write 0x11 but that's a typo
as if you read closely it says 'setting bit 4' and looking at the
datasheet bit0 is reserved, so the 0x11 should be 0x10.

> +
> + /* According to KSZ9477S Errata DS80000754F (Module 19) 0xfa00 has to
> + * be written to the Debug Register 3 to enable Single-LED Mode.
> + */
> + if (phydev->dev_flags & MICREL_KSZ9_LED_ERRATA)
> + ksz_pwrite16(dev, port, regs[P_PHY_DIGITAL_DEBUG_3], 0xfa00);

Similarly the errata specifies this as PHY register 0x1E (which is a
vendor specific register so not defined in mii.h):

ksz_phy_write16(dev, port, 0x1e, 0xfa00);

Your original patch doesn't work in my case (maybe it worked for you
because you were trying to address port0?). With the above changes the
LED's are properly configured on my boards.

Best Regards,

Tim

> +
> + dev_info(dev->dev, "port-%d: single-led mode enabled.\n", port);
> +}
> +
>  static void ksz_set_100_10mbit(struct ksz_device *dev, int port, int speed)
>  {
>   const u8 *bitval = dev->info->xmii_ctrl0;
> @@ -3977,6 +4006,9 @@  static void ksz9477_phylink_mac_link_up(struct
> phylink_config *config,
>   int port = dp->index;
>   struct ksz_port *p;
>
> + if (dev->single_led_mode && port != dev->cpu_port)
> + ksz_enable_single_led_mode(dev, port, phydev);
> +
>   p = &dev->ports[port];
>
>   /* Internal PHYs */
> @@ -5526,6 +5558,8 @@  int ksz_switch_register(struct ksz_device *dev)
>     "wakeup-source");
>   dev->pme_active_high = of_property_read_bool(dev->dev->of_node,
>       "microchip,pme-active-high");
> + dev->single_led_mode = of_property_read_bool(dev->dev->of_node,
> +     "microchip,single-led-mode");
>   }
>
>   ret = dsa_register_switch(dev->ds);
> diff --git a/drivers/net/dsa/microchip/ksz_common.h
> b/drivers/net/dsa/microchip/ksz_common.h
> index 929aff4c55de5..9900e9dd91810 100644
> --- a/drivers/net/dsa/microchip/ksz_common.h
> +++ b/drivers/net/dsa/microchip/ksz_common.h
> @@ -187,6 +187,7 @@  struct ksz_device {
>   bool synclko_disable;
>   bool wakeup_source;
>   bool pme_active_high;
> + bool single_led_mode; /* Enable Single LED Mode */
>
>   struct vlan_table *vlan_cache;
>
> @@ -277,6 +278,9 @@  enum ksz_regs {
>   PTP_RTC_SUB_NANOSEC,
>   PTP_SUBNANOSEC_RATE,
>   PTP_MSG_CONF1,
> + P_PHY_MMD_SETUP,
> + P_PHY_MMD_DATA,
> + P_PHY_DIGITAL_DEBUG_3,
>  };
>
>  enum ksz_masks {

Powered by blists - more mailing lists