netdev - Re: [v1,3/3] net: dsa: microchip: add single-led-mode support for KSZ9477

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJ+vNU2Qw2mJv4QwUee0UAxpis03238oJksRSdXZ2UG2MM=xXw@mail.gmail.com>
Date: Tue, 3 Feb 2026 16:56:30 -0800
From: Tim Harvey <tharvey@...eworks.com>
To: Heinrich Toews <ht@...-software.de>
Cc: netdev <netdev@...r.kernel.org>, Andrew Lunn <andrew@...n.ch>, 
	Florian Fainelli <f.fainelli@...il.com>, Vladimir Oltean <olteanv@...il.com>, 
	Rob Herring <robh+dt@...nel.org>
Subject: Re: [v1,3/3] net: dsa: microchip: add single-led-mode support for KSZ9477

On Tue, Feb 3, 2026 at 8:58 AM Heinrich Toews <ht@...-software.de> wrote:
>
> Hi Rob,
>
> On Fri, Jan 30, 2026 at 01:46:52PM -0800, Tim Harvey wrote:
> > >
> > > 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.
>
> I agree. To support these use cases, a specific Device Tree property
> would be the right thing here.
>
> > 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.
>
> I did some research on this and it seems that nearly all members of the
> KSZ9xxx series are affected, including:
>
> KSZ9477 / KSZ9897 / KSZ9896
> KSZ9567 / KSZ8567 / KSZ9893
> KSZ9563 / KSZ8563 / KSZ8565
>
> So MICREL_KSZ9_LED_ERRATA should be enabled for these CHIP_IDs.
>
> > > +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);
>
> I added debug output and tested my setup on all 4 external ports and it
> works:
>
> ksz-switch spi1.0: ksz_pwrite16: port: 1, offset: 0x011a, data: 0x0002
> ksz-switch spi1.0: ksz_write16: reg: 0x0000211a, value: 0x0002
>                                                           ^^^^
>                                                           The port index
>                                                           is set
>                                                           correctly.
>
> [...]
>
> ksz-switch spi1.0: ksz_pwrite16: port: 2, offset: 0x011a, data: 0x0002
> ksz-switch spi1.0: ksz_write16: reg: 0x0000311a, value: 0x0002
>                                                           ^^^^
>                                                           The same here
>                                                           on port 2.
>
> Is see activity on the leds connected to LEDx_0.
>

Hi Heinrich,

My apologies - you are correct. I accidentally missed the hunk where
you set your registers in ksz9477_regs causing it to not work for me.

> ksz9477_get_port_addr() is adding the port index by PORT_CTRL_ADDR(port, offset).
>
> But since I'm accessing PHY registers here, I'll switch to ksz_phy_write16() in v2.

Yes, that would be best as you would no longer need to add custom registers.

I have:

        ksz_phy_write16(dev->ds, port, MII_MMD_CTRL, 0x0002);
        ksz_phy_write16(dev->ds, port,  MII_MMD_DATA, 0x0000);
        ksz_phy_write16(dev->ds, port, MII_MMD_CTRL,
MII_MMD_CTRL_NOINCR | 0x02);
        ksz_phy_write16(dev->ds, port,  MII_MMD_DATA, 0x0010); /* set bit 4 */
        /* LED_ERRATA requires writing 0xfa00 to Debug Register 3 (PHY
register 0x1e) */
        if (phydev->dev_flags & MICREL_KSZ9_LED_ERRATA)
                ksz_phy_write16(dev->ds, port, 0x1e, 0xfa00);

I look forward to seeing your next submission.

Best Regards,

Tim

>
> > 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.
>
> Yes, you're right. I remember seeing it before. Thanks.
>
> > 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);
>
> May be I should add a local macro define for it.
>
> > 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.
>
> Okay. But I don't really understand why its not working in your
> case.
>
> Thanks for you're feedback.
>
> Regards,
> Heinrich.