Re: [PATCH RFC net-next 12/13] net: phylink: add struct phylink_pcs

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Mon, Jul 20, 2020 at 04:59:08PM +0000, Ioana Ciornei wrote:
> On 7/20/20 7:26 PM, Russell King - ARM Linux admin wrote:
> > On Mon, Jul 20, 2020 at 03:50:57PM +0000, Ioana Ciornei wrote:
> >> On 6/30/20 5:29 PM, Russell King wrote:
> >>> Add a way for MAC PCS to have private data while keeping independence
> >>> from struct phylink_config, which is used for the MAC itself. We need
> >>> this independence as we will have stand-alone code for PCS that is
> >>> independent of the MAC.  Introduce struct phylink_pcs, which is
> >>> designed to be embedded in a driver private data structure.
> >>>
> >>> This structure does not include a mdio_device as there are PCS
> >>> implementations such as the Marvell DSA and network drivers where this
> >>> is not necessary.
> >>>
> >>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> >>
> >> Reviewed-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> >>
> >> I integrated and used the phylink_pcs structure into the Lynx PCS just
> >> to see how everything fits. Pasting below the main parts so that we can
> >> catch early any possible different opinions on how to integrate this:
> >>
> >> The basic Lynx structure looks like below and the main idea is just to
> >> encapsulate the phylink_pcs structure and the mdio device (which in some
> >> other cases might not be needed).
> >>
> >> struct lynx_pcs {
> >>          struct phylink_pcs pcs;
> >>          struct mdio_device *mdio;
> >>          phy_interface_t interface;
> >> };
> >>
> >> The lynx_pcs structure is requested by the MAC driver with:
> >>
> >> struct lynx_pcs *lynx_pcs_create(struct mdio_device *mdio)
> >> {
> >> (...)
> >>          lynx_pcs->mdio = mdio;
> >>          lynx_pcs->pcs.ops = &lynx_pcs_phylink_ops;
> >>          lynx_pcs->pcs.poll = true;
> >>
> >>          return lynx_pcs;
> >> }
> >>
> >> And then passed to phylink with something like:
> >>
> >> phylink_set_pcs(pl, &lynx_pcs->pcs);
> >>
> >>
> >> For DSA it's a bit less straightforward because the .setup() callback
> >> from the dsa_switch_ops is run before any phylink structure has been
> >> created internally. For this, a new DSA helper can be created that just
> >> stores the phylink_pcs structure per port:
> >>
> >> void dsa_port_phylink_set_pcs(struct dsa_switch *ds, int port,
> >>                                struct phylink_pcs *pcs)
> >> {
> >>          struct dsa_port *dp = dsa_to_port(ds, port);
> >>
> >>          dp->pcs = pcs;                                         but I do
> >> }
> >>
> >> and at the appropriate time, from dsa_slave_setup, it can really install
> >> the phylink_pcs with phylink_set_pcs.
> >> The other option would be to add a new dsa_switch ops that requests the
> >> phylink_pcs for a specific port - something like phylink_get_pcs.
> > 
> > It is entirely possible to set the PCS in the mac_prepare() or
> > mac_config() callbacks - but DSA doesn't yet support the mac_prepare()
> > callback (because it needs to be propagated through the DSA way of
> > doing things.)
> > 
> > An example of this can be found at:
> > 
> > http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=mcbin&id=593d56ef8c7f7d395626752f6810210471a5f5c1
> > 
> > where we choose between the XLGMAC and GMAC pcs_ops structures
> > depending on the interface mode we are configuring for.  Note that
> > this is one of the devices that the PCS does not appear as a
> > distinctly separate entity in the register set, at least in the
> > GMAC side of things.
> > 
> 
> Thanks for the info, I didn't get that this is possible by reading the 
> previous patch. Maybe this would be useful in the documentation of the 
> callback?
> 
> Back to the DSA, I don't feel like we gain much by setting up the 
> phylink_pcs from another callback: we somehow force the driver to 
> implement a phylink_mac_prepare dsa_switch_ops just so that it sets up 
> the phylink_pcs, which for me at least would not be intuitive.

As I said, you can set it in mac_config() as well, which is the
absolute latest point. So, you don't actually need DSA to implement
anything extra.

I may need to add mac_prepare()/mac_finish() to DSA in any case if
I convert Marvell DSA switches to phylink PCS - on the face of it,
that looks like the logical thing, but there are a few quirks (like
auto-media ports) that make it less trivial.  Even so, we already
have no way to properly cope with the Marvell auto-media ports today.

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