mbox series

[CFT,net-next,0/6] net: stmmac/xpcs: modernise PCS support

Message ID Ybs7DNDkBrf73jDi@shell.armlinux.org.uk (mailing list archive)
Headers show
Series net: stmmac/xpcs: modernise PCS support | expand

Message

Russell King (Oracle) Dec. 16, 2021, 1:11 p.m. UTC
Hi,

This series updates xpcs and stmmac for the recent changes to phylink
to better support split PCS and to get rid of private MAC validation
functions.

This series is slightly more involved than other conversions as stmmac
has already had optional proper split PCS support.

The patches:

1) Provide a function to query the xpcs for the interface modes that
   are supported.

2) Populates the MAC capabilities and switches stmmac_validate() to use
   phylink_get_linkmodes(). We do not use phylink_generic_validate() yet
   as (a) we do not always have the supported interfaces populated, and
   (b) the existing code does not restrict based on interface. There
   should be no functional effect from this patch.

3) Populates phylink's supported interfaces from the xpcs when the xpcs
   is configured by firmware and also the firmware configured interface
   mode. Note: this will restrict stmmac to only supporting these
   interfaces modes - stmmac maintainers need to verify that this
   behaviour is acceptable.

4) stmmac_validate() tail-calls xpcs_validate(), but we don't need it to
   now that PCS have their own validation method. Convert stmmac and
   xpcs to use this method instead.

5) xpcs sets the poll field of phylink_pcs to true, meaning xpcs
   requires its status to be polled. There is no need to also set the
   phylink_config.pcs_poll. Remove this.

6) Switch to phylink_generic_validate(). This is probably the most
   contravertial change in this patch set as this will cause the MAC to
   restrict link modes based on the interface mode. From an inspection
   of the xpcs driver, this should be safe, as XPCS only further
   restricts the link modes to a subset of these (whether that is
   correct or not is not an issue I am addressing here.) For
   implementations that do not use xpcs, this is a more open question
   and needs feedback from stmmac maintainers.

Please review and test this series. Thanks!

 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 144 ++++++----------------
 drivers/net/pcs/pcs-xpcs.c                        |  41 +++---
 include/linux/pcs/pcs-xpcs.h                      |   3 +-
 3 files changed, 67 insertions(+), 121 deletions(-)

Comments

Wong Vee Khee Dec. 17, 2021, 5:57 a.m. UTC | #1
On Thu, Dec 16, 2021 at 01:11:40PM +0000, Russell King (Oracle) wrote:
> Hi,
> 
> This series updates xpcs and stmmac for the recent changes to phylink
> to better support split PCS and to get rid of private MAC validation
> functions.
> 
> This series is slightly more involved than other conversions as stmmac
> has already had optional proper split PCS support.
> 
> The patches:
> 
> 1) Provide a function to query the xpcs for the interface modes that
>    are supported.
> 
> 2) Populates the MAC capabilities and switches stmmac_validate() to use
>    phylink_get_linkmodes(). We do not use phylink_generic_validate() yet
>    as (a) we do not always have the supported interfaces populated, and
>    (b) the existing code does not restrict based on interface. There
>    should be no functional effect from this patch.
> 
> 3) Populates phylink's supported interfaces from the xpcs when the xpcs
>    is configured by firmware and also the firmware configured interface
>    mode. Note: this will restrict stmmac to only supporting these
>    interfaces modes - stmmac maintainers need to verify that this
>    behaviour is acceptable.
> 
> 4) stmmac_validate() tail-calls xpcs_validate(), but we don't need it to
>    now that PCS have their own validation method. Convert stmmac and
>    xpcs to use this method instead.
> 
> 5) xpcs sets the poll field of phylink_pcs to true, meaning xpcs
>    requires its status to be polled. There is no need to also set the
>    phylink_config.pcs_poll. Remove this.
> 
> 6) Switch to phylink_generic_validate(). This is probably the most
>    contravertial change in this patch set as this will cause the MAC to
>    restrict link modes based on the interface mode. From an inspection
>    of the xpcs driver, this should be safe, as XPCS only further
>    restricts the link modes to a subset of these (whether that is
>    correct or not is not an issue I am addressing here.) For
>    implementations that do not use xpcs, this is a more open question
>    and needs feedback from stmmac maintainers.
> 
> Please review and test this series. Thanks!
> 

Tested this patch series on my Intel Elkhart Lake setup with Marvell
88E1510 PHY. 

Everything works perfectly!

>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 144 ++++++----------------
>  drivers/net/pcs/pcs-xpcs.c                        |  41 +++---
>  include/linux/pcs/pcs-xpcs.h                      |   3 +-
>  3 files changed, 67 insertions(+), 121 deletions(-)
Russell King (Oracle) Jan. 7, 2022, 1:40 p.m. UTC | #2
On Fri, Dec 17, 2021 at 01:57:29PM +0800, Wong Vee Khee wrote:
> On Thu, Dec 16, 2021 at 01:11:40PM +0000, Russell King (Oracle) wrote:
> > Hi,
> > 
> > This series updates xpcs and stmmac for the recent changes to phylink
> > to better support split PCS and to get rid of private MAC validation
> > functions.
> > 
> > This series is slightly more involved than other conversions as stmmac
> > has already had optional proper split PCS support.
> > 
> > The patches:
> > 
> > 1) Provide a function to query the xpcs for the interface modes that
> >    are supported.
> > 
> > 2) Populates the MAC capabilities and switches stmmac_validate() to use
> >    phylink_get_linkmodes(). We do not use phylink_generic_validate() yet
> >    as (a) we do not always have the supported interfaces populated, and
> >    (b) the existing code does not restrict based on interface. There
> >    should be no functional effect from this patch.
> > 
> > 3) Populates phylink's supported interfaces from the xpcs when the xpcs
> >    is configured by firmware and also the firmware configured interface
> >    mode. Note: this will restrict stmmac to only supporting these
> >    interfaces modes - stmmac maintainers need to verify that this
> >    behaviour is acceptable.
> > 
> > 4) stmmac_validate() tail-calls xpcs_validate(), but we don't need it to
> >    now that PCS have their own validation method. Convert stmmac and
> >    xpcs to use this method instead.
> > 
> > 5) xpcs sets the poll field of phylink_pcs to true, meaning xpcs
> >    requires its status to be polled. There is no need to also set the
> >    phylink_config.pcs_poll. Remove this.
> > 
> > 6) Switch to phylink_generic_validate(). This is probably the most
> >    contravertial change in this patch set as this will cause the MAC to
> >    restrict link modes based on the interface mode. From an inspection
> >    of the xpcs driver, this should be safe, as XPCS only further
> >    restricts the link modes to a subset of these (whether that is
> >    correct or not is not an issue I am addressing here.) For
> >    implementations that do not use xpcs, this is a more open question
> >    and needs feedback from stmmac maintainers.
> > 
> > Please review and test this series. Thanks!
> > 
> 
> Tested this patch series on my Intel Elkhart Lake setup with Marvell
> 88E1510 PHY. 
> 
> Everything works perfectly!

Can I take that as a tested-by please?

It would be good to get some feedback from other stmmac users, since I
believe stmmac is used in multiple different configurations.

Thanks!
Wong Vee Khee Jan. 7, 2022, 1:47 p.m. UTC | #3
On Fri, Jan 07, 2022 at 01:40:06PM +0000, Russell King (Oracle) wrote:
> On Fri, Dec 17, 2021 at 01:57:29PM +0800, Wong Vee Khee wrote:
> > On Thu, Dec 16, 2021 at 01:11:40PM +0000, Russell King (Oracle) wrote:
> > > Hi,
> > > 
> > > This series updates xpcs and stmmac for the recent changes to phylink
> > > to better support split PCS and to get rid of private MAC validation
> > > functions.
> > > 
> > > This series is slightly more involved than other conversions as stmmac
> > > has already had optional proper split PCS support.
> > > 
> > > The patches:
> > > 
> > > 1) Provide a function to query the xpcs for the interface modes that
> > >    are supported.
> > > 
> > > 2) Populates the MAC capabilities and switches stmmac_validate() to use
> > >    phylink_get_linkmodes(). We do not use phylink_generic_validate() yet
> > >    as (a) we do not always have the supported interfaces populated, and
> > >    (b) the existing code does not restrict based on interface. There
> > >    should be no functional effect from this patch.
> > > 
> > > 3) Populates phylink's supported interfaces from the xpcs when the xpcs
> > >    is configured by firmware and also the firmware configured interface
> > >    mode. Note: this will restrict stmmac to only supporting these
> > >    interfaces modes - stmmac maintainers need to verify that this
> > >    behaviour is acceptable.
> > > 
> > > 4) stmmac_validate() tail-calls xpcs_validate(), but we don't need it to
> > >    now that PCS have their own validation method. Convert stmmac and
> > >    xpcs to use this method instead.
> > > 
> > > 5) xpcs sets the poll field of phylink_pcs to true, meaning xpcs
> > >    requires its status to be polled. There is no need to also set the
> > >    phylink_config.pcs_poll. Remove this.
> > > 
> > > 6) Switch to phylink_generic_validate(). This is probably the most
> > >    contravertial change in this patch set as this will cause the MAC to
> > >    restrict link modes based on the interface mode. From an inspection
> > >    of the xpcs driver, this should be safe, as XPCS only further
> > >    restricts the link modes to a subset of these (whether that is
> > >    correct or not is not an issue I am addressing here.) For
> > >    implementations that do not use xpcs, this is a more open question
> > >    and needs feedback from stmmac maintainers.
> > > 
> > > Please review and test this series. Thanks!
> > > 
> > 
> > Tested this patch series on my Intel Elkhart Lake setup with Marvell
> > 88E1510 PHY. 
> > 
> > Everything works perfectly!
> 
> Can I take that as a tested-by please?
> 

Sure.

Tested-by: Wong Vee Khee <vee.khee.wong@linux.intel.com> # Intel EHL

> It would be good to get some feedback from other stmmac users, since I
> believe stmmac is used in multiple different configurations.
> 
> Thanks!
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!