> 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
