Message-ID: <20211123181515.qqo7e4xbuu2ntwgt@skbuf>
Date:   Tue, 23 Nov 2021 20:15:15 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Sean Anderson <sean.anderson@...o.com>
Cc:     "Russell King (Oracle)" <linux@...linux.org.uk>,
        Chris Snook <chris.snook@...il.com>,
        Felix Fietkau <nbd@....name>,
        Florian Fainelli <f.fainelli@...il.com>,
        John Crispin <john@...ozen.org>,
        Mark Lee <Mark-MC.Lee@...iatek.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Michal Simek <michal.simek@...inx.com>,
        Radhey Shyam Pandey <radhey.shyam.pandey@...inx.com>,
        Sean Wang <sean.wang@...iatek.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        "David S. Miller" <davem@...emloft.net>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, netdev@...r.kernel.org
Subject: Re: [PATCH RFC net-next 8/8] net: phylink: allow PCS to be removed

On Tue, Nov 23, 2021 at 12:30:33PM -0500, Sean Anderson wrote:
> On 11/23/21 11:08 AM, Russell King (Oracle) wrote:
> > On Tue, Nov 23, 2021 at 02:08:25PM +0200, Vladimir Oltean wrote:
> > > On Tue, Nov 23, 2021 at 10:00:50AM +0000, Russell King (Oracle) wrote:
> > > > Allow phylink_set_pcs() to be called with a NULL pcs argument to remove
> > > > the PCS from phylink. This is only supported on non-legacy drivers
> > > > where doing so will have no effect on the mac_config() calling
> > > > behaviour.
> > > >
> > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>
> > > > ---
> > > >  drivers/net/phy/phylink.c | 20 +++++++++++++++-----
> > > >  1 file changed, 15 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > > index a935655c39c0..9f0f0e0aad55 100644
> > > > --- a/drivers/net/phy/phylink.c
> > > > +++ b/drivers/net/phy/phylink.c
> > > > @@ -1196,15 +1196,25 @@ EXPORT_SYMBOL_GPL(phylink_create);
> > > >   * in mac_prepare() or mac_config() methods if it is desired to dynamically
> > > >   * change the PCS.
> > > >   *
> > > > - * Please note that there are behavioural changes with the mac_config()
> > > > - * callback if a PCS is present (denoting a newer setup) so removing a PCS
> > > > - * is not supported, and if a PCS is going to be used, it must be registered
> > > > - * by calling phylink_set_pcs() at the latest in the first mac_config() call.
> > > > + * Please note that for legacy phylink users, there are behavioural changes
> > > > + * with the mac_config() callback if a PCS is present (denoting a newer setup)
> > > > + * so removing a PCS is not supported. If a PCS is going to be used, it must
> > > > + * be registered by calling phylink_set_pcs() at the latest in the first
> > > > + * mac_config() call.
> > > > + *
> > > > + * For modern drivers, this may be called with a NULL pcs argument to
> > > > + * disconnect the PCS from phylink.
> > > >   */
> > > >  void phylink_set_pcs(struct phylink *pl, struct phylink_pcs *pcs)
> > > >  {
> > > > +	if (pl->config->legacy_pre_march2020 && pl->pcs && !pcs) {
> > > > +		phylink_warn(pl,
> > > > +			     "Removing PCS is not supported in a legacy driver");
> > > > +		return;
> > > > +	}
> > > > +
> > > >  	pl->pcs = pcs;
> > > > -	pl->pcs_ops = pcs->ops;
> > > > +	pl->pcs_ops = pcs ? pcs->ops : NULL;
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(phylink_set_pcs);
> > > >
> > > > --
> > > > 2.30.2
> > > >
> > > 
> > > I've read the discussion at
> > > https://lore.kernel.org/netdev/cfcb368f-a785-9ea5-c446-1906eacd8c03@seco.com/
> > > and I still am not sure that I understand what is the use case behind
> > > removing a PCS?
> > 
> > Passing that to Sean to answer in detail...
> 
> My original feedback was regarding selecting the correct PCS to use. In
> response to the question "What PCS do you want to use for this phy
> interface mode?" a valid response is "I don't need a PCS," even if for a
> different mode a valid response might be "Please use X PCS."

Yes, but that is not a reason why you'd want to _remove_ one. Just don't
call phylink_set_pcs() in the first place, there you go, no PCS.

> Because this function is used in validate(), it is necessary to
> evaluate "what-if" scenarios, even if a scenario requiring a PCS and
> one requiring no PCS would never actually be configured.

Yes, but on the same port on the same board? The MAC-side PCS is an
integral part of serial Ethernet links, be it modeled as a discrete part
by hardware manufacturers or not. We are effectively talking about a
situation where a serial link would become parallel, or the other way
around. Have you seen such a thing?

> Typically the PCS is physically attached to the next layer in the link,
> even if the hardware could be configured not to use the PCS. So it does
> not usually make sense to configure a link to use modes both requiring a
> PCS and requiring no PCS. However, it is possible that such a system
> could exist. Most systems should use `phy-mode` to restrict the phy
> interfaces modes to whatever makes sense for the board. I think Marek's
> series (and specifically [1]) is an good step in this regard.
> 
> --Sean
> 
> [1] https://lore.kernel.org/netdev/20211123164027.15618-5-kabel@kernel.org/

Marek's patches are for reconfiguring the SERDES protocol on the same
lanes. But the lanes are still physically there, and you'd need a PCS to
talk to them no matter what you do, they won't magically turn into RGMII.
If you need to switch the MAC PCS you're configuring with another MAC
PCS (within the same hardware block more or less) due to the fact that
the SERDES protocol is changing, that doesn't count as removing the PCS,
does it? Or what are you thinking of when you say PCS? Phylink doesn't
support any other kind of PCS than a MAC-side PCS.

Powered by blists - more mailing lists