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!
