On Mon, Jun 22, 2020 at 12:35:06PM +0000, Ioana Ciornei wrote:

quoted

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:

quoted

On Mon, Jun 22, 2020 at 11:22:13AM +0100, Russell King - ARM Linux admin

wrote:

quoted

quoted

On Mon, Jun 22, 2020 at 01:54:47AM +0300, Ioana Ciornei wrote:

quoted

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.

quoted

quoted

quoted

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.

quoted

quoted

quoted

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!