Re: [PATCH net-next v3 5/9] net: dsa: add support for phylink_pcs_ops

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Mon, Jun 22, 2020 at 12:10:57PM +0100, Russell King - ARM Linux admin wrote:
> On Mon, Jun 22, 2020 at 11:22:13AM +0100, Russell King - ARM Linux admin wrote:
> > On Mon, Jun 22, 2020 at 01:54:47AM +0300, Ioana Ciornei wrote:
> > > In order to support split PCS using PHYLINK properly, we need to add a
> > > phylink_pcs_ops structure.
> > > 
> > > Note that a DSA driver that wants to use these needs to implement all 4
> > > of them: the DSA core checks the presence of these 4 function pointers
> > > in dsa_switch_ops and only then does it add a PCS to PHYLINK. This is
> > > done in order to preserve compatibility with drivers that have not yet
> > > been converted, or don't need, a split PCS setup.
> > > 
> > > Also, when pcs_get_state() and pcs_an_restart() are present, their mac
> > > counterparts (mac_pcs_get_state(), mac_an_restart()) will no longer get
> > > called, as can be seen in phylink.c.
> > 
> > I don't like this at all, it means we've got all this useless layering,
> > and that layering will force similar layering veneers into other parts
> > of the kernel (such as the DPAA2 MAC driver, when we eventually come to
> > re-use pcs-lynx there.)
> > 
> > I don't think we need that - I think we can get to a position where
> > pcs-lynx is called requesting that it bind to phylink as the PCS, and
> > it calls phylink_add_pcs() directly, which means we do not end up with
> > veneers in DSA nor in the DPAA2 MAC driver - they just need to call
> > the pcs-lynx initialisation function with the phylink instance for it
> > to attach to.
> > 
> > Yes, that requires phylink_add_pcs() to change slightly, and for there
> > to be a PCS private pointer, as I have previously stated.  At the
> > moment, changing that is really easy - the phylink PCS backend has no
> > in-tree users at the moment.  So there is no reason not to get this
> > correct right now.
> 
> How about something like this?  I don't want to embed a mdio_device
> inside struct phylink_pcs because that would not be appropriate for
> some potential users (e.g., Marvell 88E6xxx DSA drivers, Marvell
> NETA, PP2, etc.)

Just to be clear, I'm expecting discussion on this approach, rather
than a patch series appearing - I've already tweaked this patch
slightly, adding a "poll" option to cater for the "pcs_poll" facility
that was introduced into the phylink_config structure - which I think
makes more sense here, as it's all part of the PCS.

I'd like there to be some concensus _before_ I go through the users
I have in my tree converting them to this, rather than converting
them, and then having to convert them to something else.

So, if we can agree on what this should look like, then I feel it
would make the development process a lot easier for everyone
concerned.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!