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:35:06PM +0000, Ioana Ciornei wrote:
> 
> > 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.

Right, but we're talking about hardware that is common not only in DSA
but elsewhere - and we already deal with that outside of DSA with PHYs.
So, what I'm proposing is really nothing new for DSA.

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

Do you know what those errata would be?  I'm only aware of A-011118 in
the LX2160A which I don't believe will impact this code.  I don't have
visibility of Ocelot/Felix.

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

The advantage is less lines of code to maintain, and a more efficient
and understandable code structure.  I would much rather start off
simple and then augment rather than start off with unnecessary
complexity and then get stuck with it while not really needing it.

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