Link modes representation in phylib

Link modes representation in phylib

[Maxime Chevallier]

Hello everyone,

I'm currently working on adding support for 2.5GBaseT on some Marvell
PHYs (the marvell10g family, including the 88X3310).

However, phylib doesn't quite support these modes yet. Its stores the
different supported and advertised modes in u32 fields, which can't
contain the relevant values for 2500BaseT mode (and all other modes that
come after the 31st one).

I'm refering to the "advertising", "lp_advertising" and "supported"
fields in struct phy_device [1] and also to the "features" in
struct phy_driver [2].

>From what I read, I think there are plans to switch these fields to a
newer representation, using the ETHTOOL_G/SLINKSETTINGS API for example.

>From my quick analysis, it seems this would require changing the drivers
in net/ethernet that still directly access the "phydev->supported"
fields (about 30 of them) to use phy_ethtool_ksettings_{g/s}et, and also
using another way to declare the "features" in phy_driver.

There are quite a lot of references to the "legacy" ways of representing
modes, this would be another step closer to switching the a newer
representation, but since this has an impact in a lot of places in 
net/, so I wanted to have your opinion on that point before attempting
anything.

So would you have any advice on how we should start supporting newer
modes in PHY drivers ?

Thanks,

Maxime

[1]
https://elixir.bootlin.com/linux/v4.18-rc1/source/include/linux/phy.h#L441

[2]
https://elixir.bootlin.com/linux/v4.18-rc1/source/include/linux/phy.h#L512

-- 
Maxime Chevallier, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

Re: Link modes representation in phylib

[Andrew Lunn]

On Mon, Jun 18, 2018 at 05:02:24PM +0200, Maxime Chevallier wrote:
> Hello everyone,
> 
> I'm currently working on adding support for 2.5GBaseT on some Marvell
> PHYs (the marvell10g family, including the 88X3310).
> 
> However, phylib doesn't quite support these modes yet. Its stores the
> different supported and advertised modes in u32 fields, which can't
> contain the relevant values for 2500BaseT mode (and all other modes that
> come after the 31st one).

Hi Maxime

Did you look at phylink? I think it already gets this right.  It could
be, any MAC which needs to use > bit 31 should use phylink, not
phylib.

That narrows the problem down to just the PHY drivers. We might be
able to mass convert those. Or maybe we can consider just doing some
conversion work on PHYs which support > 1Gbps?

	   Andrew

Re: Link modes representation in phylib

[Maxime Chevallier]

Hello Andrew,

Thanks for your feedback !

>> I'm currently working on adding support for 2.5GBaseT on some Marvell
>> PHYs (the marvell10g family, including the 88X3310).
>> 
>> However, phylib doesn't quite support these modes yet. Its stores the
>> different supported and advertised modes in u32 fields, which can't
>> contain the relevant values for 2500BaseT mode (and all other modes that
>> come after the 31st one).  
>
>Hi Maxime
>
>Did you look at phylink? I think it already gets this right.  It could
>be, any MAC which needs to use > bit 31 should use phylink, not
>phylib.

Indeed, drivers that use phylink dont directly access these u32 fields.

>That narrows the problem down to just the PHY drivers. We might be
>able to mass convert those. Or maybe we can consider just doing some
>conversion work on PHYs which support > 1Gbps?

I think that we can consider converting only the concerned PHYs for the
moment.

What I propose is that we add 3 link_mode fields in phy_device, and keep
the legacy fields for now. It would be up to the driver to fill the new
"supported" field in config_init, kind of like what's done in the
marvell10g driver.

There already are phy_ethtool_ksettings_{g|s}et accessors, that are
used by phylink so that would easily integrate with the above solution
of only supporting phylink for these modes.

That would involve a bit of info duplication, but I think that would
allow for a smooth transition to a newer representation.

Would that be acceptable ?

Thanks,

Maxime

Re: Link modes representation in phylib

[Andrew Lunn]

> What I propose is that we add 3 link_mode fields in phy_device, and keep
> the legacy fields for now. It would be up to the driver to fill the new
> "supported" field in config_init, kind of like what's done in the
> marvell10g driver.

Hi Maxime

You can do this conversion in the core. If features == 0, and some
bits are set in the features link_mode, do the conversion at probe
time. The same can be done for lp_advertising, when the call into the
drivers read_status() has completed.

> Would that be acceptable ?

It sounds reasonable. Lets see what the code looks like.

   Andrew

Re: Link modes representation in phylib

[Maxime Chevallier]

Hello Andrew,

On Tue, 19 Jun 2018 17:21:55 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> What I propose is that we add 3 link_mode fields in phy_device, and keep
>> the legacy fields for now. It would be up to the driver to fill the new
>> "supported" field in config_init, kind of like what's done in the
>> marvell10g driver.  
>
>Hi Maxime
>
>You can do this conversion in the core. If features == 0, and some
>bits are set in the features link_mode, do the conversion at probe
>time. The same can be done for lp_advertising, when the call into the
>drivers read_status() has completed.

Thanks for the suggestion. I see how this can be done with
phydrv->supported and phydev->lp_advertising, however I'm not sure how
we should deal with the fact that ethernet drivers directly access
fields such as "advertising" or "supported".

Should we update the new fields in phy_device when functions such as
"phy_start" or "phy_start_aneg" are called, just in case the ethernet
driver modified the phydev->supported / phydev->advertising fields
in the meantime ?

My concern is that phylink will rely on the new link_mode
representation to be up-to-date, and ethernet drivers that aren't
converted to phylink, but link to a new PHY will rely on the legacy
representation.

That might be just fine if we take good care making sure both the
legacy and the new representation are well sync'd, but since I don't
have a good big-picture view of all this, I prefer having your opinion
first :)

>> Would that be acceptable ?  
>
>It sounds reasonable. Lets see what the code looks like.

Ok I'll be glad to send a series for that, I just want to make sure I
go in the right direction

Thanks,

Maxime

Re: Link modes representation in phylib

[Andrew Lunn]

> Thanks for the suggestion. I see how this can be done with
> phydrv->supported and phydev->lp_advertising, however I'm not sure how
> we should deal with the fact that ethernet drivers directly access
> fields such as "advertising" or "supported".

Hi Maxime

I started cleaning up some of the MAC drivers. Take a look at

https://github.com/lunn/linux.git v4.18-rc1-net-next-phy-cleanup

That moved a lot of the MAC code into helpers, making it easier to
update. It might make sense to add a couple of more helpers, for what
remains.

	Andrew

Re: Link modes representation in phylib

[Maxime Chevallier]

Hello Andrew,

On Fri, 29 Jun 2018 15:43:43 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> Thanks for the suggestion. I see how this can be done with
>> phydrv->supported and phydev->lp_advertising, however I'm not sure how
>> we should deal with the fact that ethernet drivers directly access
>> fields such as "advertising" or "supported".  
>
>Hi Maxime
>
>I started cleaning up some of the MAC drivers. Take a look at
>
>https://github.com/lunn/linux.git v4.18-rc1-net-next-phy-cleanup
>
>That moved a lot of the MAC code into helpers, making it easier to
>update. It might make sense to add a couple of more helpers, for what
>remains.

Wow indeed that will help a lot. Just so that we're in sync, do you
plan to add those helpers, or should I take this branch as a base for
the conversion and go on ?

Thanks for this,

Maxime

-- 
Maxime Chevallier, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

Re: Link modes representation in phylib

[Andrew Lunn]

> Wow indeed that will help a lot. Just so that we're in sync, do you
> plan to add those helpers, or should I take this branch as a base for
> the conversion and go on ?

I'm still working on it. I can probably push again in the next few
minutes. But they won't be compile tested, i.e. broken...

It might make more sense if you work on the core, and leave me to
cleanup the MAC drivers. You can use my branch, if you need to change
the helpers, just expect it to be unstable, get rebased, and might not
even compile...

     Andrew

Re: Link modes representation in phylib

[Maxime Chevallier]

On Fri, 29 Jun 2018 17:34:41 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> Wow indeed that will help a lot. Just so that we're in sync, do you
>> plan to add those helpers, or should I take this branch as a base for
>> the conversion and go on ?  
>
>I'm still working on it. I can probably push again in the next few
>minutes. But they won't be compile tested, i.e. broken...
>
>It might make more sense if you work on the core, and leave me to
>cleanup the MAC drivers. You can use my branch, if you need to change
>the helpers, just expect it to be unstable, get rebased, and might not
>even compile...

Ok no problem, I'll keep working on the core while keeping an eye on
your branch then.

Thanks a lot,

Maxime