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

[Florian Fainelli <f.fainelli@gmail.com>]

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