Subject: Re: [RFC net-next 2/5] net: phylink: add separate pcs operations structure

From: Russell King - ARM Linux admin <[email-masked]>
Date: Thu, 19 Mar 2020 12:14:18 +0000

On Tue, Mar 17, 2020 at 04:54:22PM +0000, Russell King - ARM Linux admin wrote:
> On Tue, Mar 17, 2020 at 05:38:02PM +0100, Andrew Lunn wrote:
> > On Tue, Mar 17, 2020 at 02:52:51PM +0000, Russell King wrote:
> > > *NOT FOR MERGING*
> > > 
> > > Add a separate set of PCS operations, which MAC drivers can use to
> > > couple phylink with their associated MAC PCS layer.  The PCS
> > > operations include:
> > > 
> > > - pcs_get_state() - reads the link up/down, resolved speed, duplex
> > >    and pause from the PCS.
> > > - pcs_config() - configures the PCS for the specified mode, PHY
> > >    interface type, and setting the advertisement.
> > > - pcs_an_restart() - restarts 802.3 in-band negotiation with the
> > >    link partner
> > > - pcs_link_up() - informs the PCS that link has come up, and the
> > >    parameters of the link. Link parameters are used to program the
> > >    PCS for fixed speed and non-inband modes.
> > 
> > Hi Russell
> > 
> > This API makes sense. But it seems quite common to have multiple
> > PCS's. Rather than have MAC drivers implement their own mux, i wonder
> > if there should be core support? Or at least a library to help the
> > implementation?
> 
> When each PCS has different characteristics, and may not even be
> available to be probed (because the hardware holds them in reset,
> so they don't even respond to MDIO cycles) that becomes very
> difficult.
> 
> That is the situation with LX2160A - when in 1G mode, the 10G C45
> PCS does not respond.  Already tested that.
> 
> So, determining when to switch can't be known by generic code.

Oh, I forgot to mention on the library point - that's what has already
been created in:

"net: phylink: pcs: add 802.3 clause 45 helpers"
"net: phylink: pcs: add 802.3 clause 22 helpers"

which add library implementations for the pcs_get_state(), pcs_config()
and pcs_an_restart() methods.

What remains is vendor specific - for pcs_link_up(), there is no
standard, since it requires fiddling with vendor specific registers to
program, e.g. the speed in SGMII mode when in-band is not being used.
The selection between different PCS is also vendor specific.

It would have been nice to use these helpers for Marvell DSA switches
too, but the complexities of DSA taking a multi-layered approach rather
than a library approach, plus the use of paging makes it very
difficult.

So, basically on the library point, "already considered and
implemented".

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up