Re: [PATCH 2/2] net: dsa: microchip: Provide Module 4 KSZ9477 errata (DS80000754C)

[Florian Fainelli]

On 8/25/2023 1:39 AM, Lukasz Majewski wrote:
> Hi Tristram,
>
> > > +static int ksz9477_errata(struct dsa_switch *ds)
> > > +{
> > > +       struct ksz_device *dev = ds->priv;
> > > +       u16 val;
> > > +       int p;
> > > +
> > > +       /* KSZ9477 Errata DS80000754C
> > > +        *
> > > +        * Module 4: Energy Efficient Ethernet (EEE) feature select
> > > must be
> > > +        * manually disabled
> > > +        *   The EEE feature is enabled by default, but it is not
> > > fully
> > > +        *   operational. It must be manually disabled through
> > > register
> > > +        *   controls. If not disabled, the PHY ports can
> > > auto-negotiate
> > > +        *   to enable EEE, and this feature can cause link drops
> > > when linked
> > > +        *   to another device supporting EEE.
> > > +        *
> > > +        *   Only PHY ports (dsa user) [0-4] need to have the EEE
> > > advertisement
> > > +        *   bits cleared.
> > > +        */
> > > +
> > > +       for (p = 0; p < ds->num_ports; p++) {
> > > +               if (!dsa_is_user_port(ds, p))
> > > +                       continue;
> > > +
> > > +               ksz9477_port_mmd_read(dev, p, MMD_DEVICE_ID_EEE_ADV,
> > > +                                     MMD_EEE_ADV, &val, 1);
> > > +
> > > +               pr_err("%s: PORT: %d val: 0x%x pc: %d\n", __func__,
> > > p, val,
> > > +                      ds->num_ports);
> > > +
> > > +               val &= ~(EEE_ADV_100MBIT | EEE_ADV_1GBIT);
> > > +               ksz9477_port_mmd_write(dev, p,
> > > MMD_DEVICE_ID_EEE_ADV,
> > > +                                      MMD_EEE_ADV, &val, 1);
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> > >   int ksz9477_setup(struct dsa_switch *ds)
> > >   {
> > >          struct ksz_device *dev = ds->priv;
> > > @@ -1157,7 +1195,7 @@ int ksz9477_setup(struct dsa_switch *ds)
> > >          /* enable global MIB counter freeze function */
> > >          ksz_cfg(dev, REG_SW_MAC_CTRL_6, SW_MIB_COUNTER_FREEZE,
> > > true);
> > >
> > > -       return 0;
> > > +       return ksz9477_errata(ds);
> > >   }
> >
> > I would prefer to execute the code in ksz9477_config_cpu_port(), as at
> > the end there is already a loop to do something to each port.
>
> Just some explanation of the taken approach:
>
> 1. I've followed already in-mainline code for ksz8795.c
> (ksz8_handle_global_errata(ds)) which is executed in ksz8_setup
> function.
>
> 2. I do believe, that separate "errata" function would be more
> readable, as KSZ9477 has many more erratas to be added.
>
> > The
> > check to disable EEE or not should be dev->info->internal_phy[port],
> > as one of the user ports can be RGMII or SGMII, which does not have a
> > PHY that can be accessed inside the switch.
>
> Yes, this would be better solution. Thanks for the suggestion.
>
> > As the EEE register value is simply 6 it should be enough to just set
> > the register to zero.  If so we do not need to add back those
> > ksz9477_port_mmd_setup functions and just use ksz_pwrite16() to write
> > to the MMD register.
>
> IMHO adding functions to MMD modification would facilitate further
> development (for example LED setup).

We already have some KSZ9477 specific initialization done in the Micrel 
PHY driver under drivers/net/phy/micrel.c, can we converge on the PHY 
driver which has a reasonable amount of infrastructure for dealing with 
workarounds, indirect or direct MMD accesses etc.?
--
Florian