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

[Lukasz Majewski]

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).

However, if we only plan to fix this errata, then the MMD access
functions are not required.


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@xxxxxxx

Attachment: pgpZdJVng2nNo.pgp
Description: OpenPGP digital signature