Date:   Thu, 18 Jun 2020 16:17:56 +0000
From:   Ioana Ciornei <ioana.ciornei@....com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        Vladimir Oltean <vladimir.oltean@....com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandru Marginean <alexandru.marginean@....com>,
        "michael@...le.cc" <michael@...le.cc>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>
Subject: RE: [PATCH net-next 4/5] net: phy: add Lynx PCS MDIO module

> Subject: Re: [PATCH net-next 4/5] net: phy: add Lynx PCS MDIO module
> 
> On Thu, Jun 18, 2020 at 03:08:36PM +0300, Ioana Ciornei wrote:
> > Add a Lynx PCS MDIO module which exposes the necessary operations to
> > drive the PCS using PHYLINK.
> >
> > The majority of the code is extracted from the Felix DSA driver, which
> > will be also modified in a later patch, and exposed as a separate
> > module for code reusability purposes.
> >
> > At the moment, USXGMII (only with in-band AN and speeds up to 2500),
> > SGMII, QSGMII and 2500Base-X (only w/o in-band AN) are supported by
> > the Lynx PCS MDIO module since these were also supported by Felix.
> >
> > The module can only be enabled by the drivers in need and not user
> > selectable.
> 
> Is this the same PCS found in the LX2160A?  It looks very similar.
> 

Yes, it is.
I already tested these protocols on LX2160A (and some other DPAA2 SoCs).
The idea is to have this patch set without any functional changes accepted and
then I will wire up dpaa2-eth as well into this.

> > +/* 2500Base-X is SerDes protocol 7 on Felix and 6 on ENETC. It is a
> > +SerDes lane
> > + * clocked at 3.125 GHz which encodes symbols with 8b/10b and does
> > +not have
> > + * auto-negotiation of any link parameters. Electrically it is
> > +compatible with
> > + * a single lane of XAUI.
> > + * The hardware reference manual wants to call this mode SGMII, but
> > +it isn't
> > + * really, since the fundamental features of SGMII:
> > + * - Downgrading the link speed by duplicating symbols
> > + * - Auto-negotiation
> > + * are not there.
> 
> I welcome that others are waking up to the industry wide obfuscation of
> terminology surrounding "SGMII" and "1000base-X", and calling it out where it is
> blatently incorrectly described in documentation.

I will not take the credit for this since this is mainly just a comment being moved.

> 
> > + * The speed is configured at 1000 in the IF_MODE because the clock
> > + frequency
> > + * is actually given by a PLL configured in the Reset Configuration Word
> (RCW).
> > + * Since there is no difference between fixed speed SGMII w/o AN and
> > + 802.3z w/o
> > + * AN, we call this PHY interface type 2500Base-X. In case a PHY
> > + negotiates a
> > + * lower link speed on line side, the system-side interface remains
> > + fixed at
> > + * 2500 Mbps and we do rate adaptation through pause frames.
> 
> We have systems that do have AN with 2500base-X however - which is what you
> want when you couple two potentially remote systems over a fibre cable.  The
> AN in 802.3z (1000base-X) is used to negotiate:
> 
> - duplex
> - pause mode
> 
> although in practice, half-duplex is not supported by lots of hardware, which
> leaves just pause mode.  It is useful to have pause mode negotiation remain
> present, whether it's 1000base-X or 2500base-X, but obviously within the
> hardware boundaries.
> 
> I suspect the hardware is capable of supporting 802.3z AN when operating at
> 2500base-X, but not the SGMII symbol duplication for slower speeds.
> 

I don't have a definitive answer to this this right now, I'll have to actually test this
if I can get my hands on some hardware for this setup.

> > +struct mdio_lynx_pcs *mdio_lynx_pcs_create(struct mdio_device
> > +*mdio_dev) {
> > +	struct mdio_lynx_pcs *pcs;
> > +
> > +	if (WARN_ON(!mdio_dev))
> > +		return NULL;
> > +
> > +	pcs = kzalloc(sizeof(*pcs), GFP_KERNEL);
> > +	if (!pcs)
> > +		return NULL;
> > +
> > +	pcs->dev = mdio_dev;
> > +	pcs->an_restart = lynx_pcs_an_restart;
> > +	pcs->get_state = lynx_pcs_get_state;
> > +	pcs->link_up = lynx_pcs_link_up;
> > +	pcs->config = lynx_pcs_config;
> 
> We really should not have these private structure interfaces.  Private structure-
> based driver specific interfaces really don't constitute a sane approach to
> interface design.
> 
> Would it work if there was a "struct mdio_device" add to the phylink_config
> structure, and then you could have the phylink_pcs_ops embedded into this
> driver?

I think that would restrict too much the usage.
I am afraid there will be instances where the PCS is not recognizable by PHY_ID,
thus no way of knowing which driver to probe which mdio_device.
Also, I would leave to the driver the choice of using (or not) the functions 
exported by Lynx.

> 
> If not, then we need some kind of mdio_pcs_device that offers this kind of
> functionality.
> 

Maybe we can meet in the middle?

What if we directly export the helper functions directly as symbols which can
be used by the driver without any mdio_lynx_pcs in the middle
(just the mdio_device passed to the function).
This would be exactly as phylink_mii_c22_pcs_[an_restart/config] are currently
used.

We can somehow standardize the functions prototypes (which will likely mean
mdio_device instead of the phylink_pcs_ops's phylink_config).

Ioana

Powered by blists - more mailing lists