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

[Ioana Ciornei <ioana.ciornei@nxp.com>]

> Subject: Re: [PATCH net-next v3 5/9] net: dsa: add support for phylink_pcs_ops
> 
> 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.)
> > >

The veneers that you are talking about are one phylink_pcs_ops structure
and 4 functions that call lynx_pcs_* subsequently. We have the same thing
for the MAC operations.

Also, the "veneers" in DSA are just how it works, and I don't want to change
its structure without a really good reason and without a green light from
DSA maintainers.

> > > 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.

What I am most concerned about is that by passing the PCS ops directly to the
PCS module we would lose any ability to apply SoC specific quirks at runtime
such as errata workarounds.

On the other hand, I am not sure what is the concrete benefit of doing it your way.
I understand that for a PHY device the MAC is not involved in the call path but in the
case of the PCS the expectation is that it's tightly coupled in the silicon
and not plug-and-play.

- Ioana

> > >
> > > 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.
>