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

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

[Julia Lawall]

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?

thanks,
julia

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

[Vladimir Oltean]

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?
>
> 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

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

[Heiner Kallweit]

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
> 

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

[Joe Perches]

On Fri, 2020-04-03 at 23:02 +0200, Heiner Kallweit wrote:
> It's right that this is not correct. You can check genphy_read_status_fixed()
> for how it's done there.

There is no SPEED_UNKNOWN in that function.

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

[Vladimir Oltean]

On Sat, 4 Apr 2020 at 00:12, Joe Perches <joe@perches.com> wrote:
>
> On Fri, 2020-04-03 at 23:02 +0200, Heiner Kallweit wrote:
> > It's right that this is not correct. You can check genphy_read_status_fixed()
> > for how it's done there.
>
> There is no SPEED_UNKNOWN in that function.
>
>

Correct, there isn't.
The bitwise value of the 2 speed bits from MII_BMCR is:
Bits [6,13]:
1 1 = Reserved
1 0 = 1000 Mbps
0 1 = 100 Mbps
0 0 = 10 Mbps
So basically the PHY library assumes that no piece of hardware will
ever set the speed bits to 11, as that would invalidate the spec.

-Vladimir