> Subject: Re: [PATCH net-next v3 5/9] net: dsa: add support for phylink_pcs_ops > > 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.
I said before why the PHY use case is different from a PCS tightly coupled inside the SoC. > 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. I was mainly looking at this from a software architecture perspective, not having any explicit erratum in mind. > > > 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. > I am not going to argue ad infinitum on this. If you think keeping the phylink_pcs_ops structure outside is the better approach then let's take that route. I will go through your feedback on the actual Lynx module and respond/make the necessary adjustments. Beside this, what should be our next move? Will you submit the new method of working with the phylink_pcs_ops structure? Ioana
