Re: [Intel-wired-lan] [PATCH iwl-next v2 2/9] ethtool: Add forced speed to supported link modes maps

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Andrew Lunn <andrew@lunn.ch>
Date: Fri, 25 Aug 2023 15:47:20 +0200

>> Let me think how we could do that.
>> Andrew's idea is good. But most high-speed NICs, which have a standalone
>> management firmware for PHY, don't use phylib/phylink.
>> So in order to be able to unify all that, they should have ->supported
>> bitmap somewhere else. Not sure struct net_device is the best place...
> 
> I would probably keep it in the driver priv structure, and just pass
> it as needed. So long as you only need one or two values, i don't see
> the need for a shared structure.
> 
>> If I recall Phylink logics correctly (it's been a while since I last
>> time was working with my embedded project),
>>
>> 1) in the NIC (MAC) driver, you initialize ->supported with *speeds* and
>>    stuff like duplex, no link modes;
>> 2) Phylink core sets the corresponding link mode bits;
>> 3) phylib core then clears the bits unsupported by the PHY IIRC
> 
> No, not really.
> 
> All i think you need is a low level helper. So don't worry too much
> about how phylink works, just implement that low level helper passing
> in values as needed, not phylib or phylink structure.
> 
> What i don't want is a second infrastructure to be built for those MAC
> drivers which don't use Linux to control the PHY. Either share a few
> helpers, or swap to phylink.

I'd love those drivers to be swapped to phylink, but I doubt that will
happen :D

> 
>> The third step in case with those NICs with FW-managed PHYs should be
>> done manually in the MAC driver somewhere. Like "I am qede and I don't
>> support mode XX at 50Gbps, but support the rest, so I clear that one bit".
> 
> I don't think that will work. New bits keep getting added, more speeds
> added. So 'support the rest' is not well defined. You need an explicit

Ah, correct.

> list of link modes the driver needs. We already have code to convert
> an array of link mode bits into an actual mask, e.g:
> 
>         linkmode_set_bit_array(phy_basic_t1_features_array,
>                                ARRAY_SIZE(phy_basic_t1_features_array),
>                                phy_basic_t1_features);

Now I got lost a bit in what we do really want to share now, as less
sharing was indirectly rejected by "you can share more, let PHY/whatever
take care of this" and now wider sharing was indirectly rejected by
"that won't work" :D
From what I understood, all we want now is the stuff introduced by the
original patch from that thread, but without "generic" speed arrays
definitions?

> 
> 	Andrew			       

Thanks,
Olek
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan