Re: question about drivers/net/dsa/sja1105/sja1105_main.c

[Heiner Kallweit <hkallweit1@gmail.com>]

On 03.04.2020 16:36, Vladimir Oltean wrote:
> Hi Julia,
> 
> On Fri, 3 Apr 2020 at 16:46, Julia Lawall <julia.lawall@inria.fr> wrote:
>>
>> Hello,
>>
>> The function sja1105_static_config_reload in sja1105_main.c contains the
>> code:
>>
>>                 if (!an_enabled) {
>>                         int speed = SPEED_UNKNOWN;
>>
>>                         if (bmcr & BMCR_SPEED1000)
>>                                 speed = SPEED_1000;
>>                         else if (bmcr & BMCR_SPEED100)
>>                                 speed = SPEED_100;
>>                         else if (bmcr & BMCR_SPEED10)
>>                                 speed = SPEED_10;
>>
>>                         sja1105_sgmii_pcs_force_speed(priv, speed);
>>                 }
>>
>> The last test bmcr & BMCR_SPEED10 does not look correct, because according
>> to include/uapi/linux/mii.h, BMCR_SPEED10 is 0.  What should be done
>> instead?
>>

It's right that this is not correct. You can check genphy_read_status_fixed()
for how it's done there.


>> thanks,
>> julia
> 
> Thanks for pointing out, you raise a good point.
> Correct usage would be:
> 
> include/uapi/linux/mii.h:
> #define BMCR_SPEED_MASK 0x2040
> 
> drivers/net/dsa/sja1105/sja1105_main.c:
>                          int speed = SPEED_UNKNOWN;
> 
>                          if (bmcr & BMCR_SPEED_MASK == BMCR_SPEED1000)
>                                  speed = SPEED_1000;
>                          else if (bmcr & BMCR_SPEED_MASK == BMCR_SPEED100)
>                                  speed = SPEED_100;
>                          else if (bmcr & BMCR_SPEED_MASK == BMCR_SPEED10)
>                                  speed = SPEED_10;
> 
> but the BMCR_SPEED_MASK doesn't exist, it looks like. I believe that
> is because drivers (or the PHY library) don't typically need to read
> the speed from the MII_BMCR register, they just need to write it. If
> the PHY library maintainers think there is any value in defining
> BMCR_SPEED_MASK as part of the UAPI, we can do that. Otherwise, the
> definition can be restricted to drivers/net/dsa/sja1105/sja1105.h.
> 
> Thanks,
> -Vladimir
>