Message-ID: <VI1PR0402MB387117BC6F2B53E521F6D7EBE0940@VI1PR0402MB3871.eurprd04.prod.outlook.com>
Date:   Tue, 23 Jun 2020 11:49:28 +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>,
        "olteanv@...il.com" <olteanv@...il.com>
Subject: RE: [PATCH net-next v3 4/9] net: phy: add Lynx PCS module


> Subject: Re: [PATCH net-next v3 4/9] net: phy: add Lynx PCS module
> 
> On Mon, Jun 22, 2020 at 01:54:46AM +0300, Ioana Ciornei wrote:
> > Add a Lynx PCS 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 module since these were also supported by Felix.
> >
> > The module can only be enabled by the drivers in need and not user
> > selectable.
> >
> > Signed-off-by: Ioana Ciornei <ioana.ciornei@....com>
> > ---
> > Changes in v2:
> >  * got rid of the mdio_lynx_pcs structure and directly exported the
> > functions without the need of an indirection
> >  * solved the broken allmodconfig build test by making the module
> > tristate instead of bool
> >
> > Changes in v3:
> >  * renamed the file to pcs-lynx.c
> >
> >
> >  MAINTAINERS                |   7 +
> >  drivers/net/phy/Kconfig    |   6 +
> >  drivers/net/phy/Makefile   |   1 +
> >  drivers/net/phy/pcs-lynx.c | 337
> +++++++++++++++++++++++++++++++++++++
> >  include/linux/pcs-lynx.h   |  25 +++
> >  5 files changed, 376 insertions(+)
> >  create mode 100644 drivers/net/phy/pcs-lynx.c  create mode 100644
> > include/linux/pcs-lynx.h
> >

(...)

> > diff --git a/drivers/net/phy/pcs-lynx.c b/drivers/net/phy/pcs-lynx.c
> > new file mode 100644 index 000000000000..23bdd9db4340
> > --- /dev/null
> > +++ b/drivers/net/phy/pcs-lynx.c
> > @@ -0,0 +1,337 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> > +/* Copyright 2020 NXP
> > + * Lynx PCS MDIO helpers
> > + */
> > +
> > +#include <linux/mdio.h>
> > +#include <linux/phylink.h>
> > +#include <linux/pcs-lynx.h>
> > +
> > +#define SGMII_CLOCK_PERIOD_NS		8 /* PCS is clocked at 125 MHz
> */
> > +#define SGMII_LINK_TIMER_VAL(ns)	((u32)((ns) /
> SGMII_CLOCK_PERIOD_NS))
> > +
> > +#define SGMII_AN_LINK_TIMER_NS		1600000 /* defined by SGMII
> spec */
> > +
> > +#define SGMII_LINK_TIMER_LO		0x12
> > +#define SGMII_LINK_TIMER_HI		0x13
> > +#define SGMII_IF_MODE			0x14
> > +#define SGMII_IF_MODE_SGMII_EN		BIT(0)
> > +#define SGMII_IF_MODE_USE_SGMII_AN	BIT(1)
> > +#define SGMII_IF_MODE_SPEED(x)		(((x) << 2) & GENMASK(3, 2))
> > +#define SGMII_IF_MODE_SPEED_MSK		GENMASK(3, 2)
> > +#define SGMII_IF_MODE_DUPLEX		BIT(4)
> 
> Given that this is in the .c file, and this code will be re-used in other places where
> there is support for more than Cisco SGMII, can we lose the SGMII_ prefix
> please?  Maybe use names such as those I have in "dpaa2-mac: add 1000BASE-
> X/SGMII PCS support" ?

Yep, I can do that. 

> 
> (I hate the way a single lane gigabit serdes link that supports 1000base-x gets
> incorrectly called "SGMII".)
> 
> > +
> > +#define USXGMII_ADVERTISE_LSTATUS(x)	(((x) << 15) & BIT(15))
> > +#define USXGMII_ADVERTISE_FDX		BIT(12)
> > +#define USXGMII_ADVERTISE_SPEED(x)	(((x) << 9) & GENMASK(11, 9))
> > +
> > +#define USXGMII_LPA_LSTATUS(lpa)	((lpa) >> 15)
> > +#define USXGMII_LPA_DUPLEX(lpa)		(((lpa) & GENMASK(12, 12)) >>
> 12)
> > +#define USXGMII_LPA_SPEED(lpa)		(((lpa) & GENMASK(11, 9)) >> 9)
> > +
> > +enum usxgmii_speed {
> > +	USXGMII_SPEED_10	= 0,
> > +	USXGMII_SPEED_100	= 1,
> > +	USXGMII_SPEED_1000	= 2,
> > +	USXGMII_SPEED_2500	= 4,
> > +};
> 
> These are not specific to the Lynx PCS, but are the standard layout of the
> USXGMII word.  These ought to be in a header file similar to what we do with
> the SGMII definitions in include/uapi/linux/mii.h.
> I think as these are Clause 45, they possibly belong in include/uapi/linux/mdio.h
> ?  In any case, one of my comments below suggests that some of the uses of
> these definitions should be moved into phylink's helpers.
>

Ok, since these are described in the USXGMII standard I can move them to the generic header (mdio.h).
I will comment below about their usage in phylink's helpers.
 
> > +
> > +enum sgmii_speed {
> > +	SGMII_SPEED_10		= 0,
> > +	SGMII_SPEED_100		= 1,
> > +	SGMII_SPEED_1000	= 2,
> > +	SGMII_SPEED_2500	= 2,
> > +};
> > +
> > +static void lynx_pcs_an_restart_usxgmii(struct mdio_device *pcs) {
> > +	mdiobus_c45_write(pcs->bus, pcs->addr,
> > +			  MDIO_MMD_VEND2, MII_BMCR,
> > +			  BMCR_RESET | BMCR_ANENABLE |
> BMCR_ANRESTART); }
> 
> Phylink will not call *_an_restart() for USXGMII, so this code is unreachable.
> 
> > +
> > +void lynx_pcs_an_restart(struct mdio_device *pcs, phy_interface_t
> > +ifmode) {
> > +	switch (ifmode) {
> > +	case PHY_INTERFACE_MODE_SGMII:
> > +	case PHY_INTERFACE_MODE_QSGMII:
> > +		phylink_mii_c22_pcs_an_restart(pcs);
> 
> Phylink will not call *_an_restart() for SGMII, so this code is unreachable.
> 

Good point. I'll remove this and the above one.
> > +		break;
> > +	case PHY_INTERFACE_MODE_USXGMII:
> > +		lynx_pcs_an_restart_usxgmii(pcs);
> > +		break;
> > +	case PHY_INTERFACE_MODE_2500BASEX:
> > +		break;
> > +	default:
> > +		dev_err(&pcs->dev, "Invalid PCS interface type %s\n",
> > +			phy_modes(ifmode));
> > +		break;
> > +	}
> > +}
> > +EXPORT_SYMBOL(lynx_pcs_an_restart);
> > +
> > +static void lynx_pcs_get_state_usxgmii(struct mdio_device *pcs,
> > +				       struct phylink_link_state *state) {
> > +	struct mii_bus *bus = pcs->bus;
> > +	int addr = pcs->addr;
> > +	int status, lpa;
> > +
> > +	status = mdiobus_c45_read(bus, addr, MDIO_MMD_VEND2,
> MII_BMSR);
> > +	if (status < 0)
> > +		return;
> > +
> > +	state->link = !!(status & MDIO_STAT1_LSTATUS);
> > +	state->an_complete = !!(status & MDIO_AN_STAT1_COMPLETE);
> > +	if (!state->link || !state->an_complete)
> > +		return;
> > +
> > +	lpa = mdiobus_c45_read(bus, addr, MDIO_MMD_VEND2, MII_LPA);
> > +	if (lpa < 0)
> > +		return;
> > +
> > +	switch (USXGMII_LPA_SPEED(lpa)) {
> > +	case USXGMII_SPEED_10:
> > +		state->speed = SPEED_10;
> > +		break;
> > +	case USXGMII_SPEED_100:
> > +		state->speed = SPEED_100;
> > +		break;
> > +	case USXGMII_SPEED_1000:
> > +		state->speed = SPEED_1000;
> > +		break;
> > +	case USXGMII_SPEED_2500:
> > +		state->speed = SPEED_2500;
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	if (USXGMII_LPA_DUPLEX(lpa))
> > +		state->duplex = DUPLEX_FULL;
> > +	else
> > +		state->duplex = DUPLEX_HALF;
> 
> This should be added to phylink_mii_c45_pcs_get_state().  There is nothing that
> is Lynx PCS specific here.

The USXGMII standard only describes the auto-negotiation word, not the MMD
where this can be found (MMD_VEND2 in this case).
I would not add a generic phylink herper that reads the MMD and also decodes it.
Maybe a helper that just decodes the USXGMII word read from the
Lynx module - phylink_decode_usxgmii_word(state, lpa) ?

> 
> > +}
> > +
> > +static void lynx_pcs_get_state_2500basex(struct mdio_device *pcs,
> > +					 struct phylink_link_state *state) {
> > +	struct mii_bus *bus = pcs->bus;
> > +	int addr = pcs->addr;
> > +	int bmsr, lpa;
> > +
> > +	bmsr = mdiobus_read(bus, addr, MII_BMSR);
> > +	lpa = mdiobus_read(bus, addr, MII_LPA);
> > +	if (bmsr < 0 || lpa < 0) {
> > +		state->link = false;
> > +		return;
> > +	}
> > +
> > +	state->link = !!(bmsr & BMSR_LSTATUS);
> > +	state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
> > +	if (!state->link)
> > +		return;
> > +
> > +	state->speed = SPEED_2500;
> > +	state->pause |= MLO_PAUSE_TX | MLO_PAUSE_RX;
> 
> How do you know the other side is using pause frames, or is capable of dealing
> with them?

Isn't this done by also looking into the PHY's pause frame bits and enabling pause
frames in the MAC only when both the PCS and the PHY have flow enabled?

> 
> In any case, phylink_mii_c22_pcs_get_state() should be expanded to deal with
> the non-inband cases, where we are only interested in the link state.  It isn't
> passed the link AN mode, which may be an issue that needs resolving in some
> way.
>

Agreed, but I wouldn't just add all of this into this patch set.. it already is getting out of hand.

 
> > +}
> > +
> > +void lynx_pcs_get_state(struct mdio_device *pcs, phy_interface_t ifmode,
> > +			struct phylink_link_state *state)
> > +{
> > +	switch (ifmode) {
> > +	case PHY_INTERFACE_MODE_SGMII:
> > +	case PHY_INTERFACE_MODE_QSGMII:
> > +		phylink_mii_c22_pcs_get_state(pcs, state);
> > +		break;
> > +	case PHY_INTERFACE_MODE_2500BASEX:
> > +		lynx_pcs_get_state_2500basex(pcs, state);
> > +		break;
> > +	case PHY_INTERFACE_MODE_USXGMII:
> > +		lynx_pcs_get_state_usxgmii(pcs, state);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	dev_dbg(&pcs->dev,
> > +		"mode=%s/%s/%s link=%u an_enabled=%u
> an_complete=%u\n",
> > +		phy_modes(ifmode),
> > +		phy_speed_to_str(state->speed),
> > +		phy_duplex_to_str(state->duplex),
> > +		state->link, state->an_enabled, state->an_complete); }
> > +EXPORT_SYMBOL(lynx_pcs_get_state);
> > +
> > +static int lynx_pcs_config_sgmii(struct mdio_device *pcs, unsigned int mode,
> > +				 const unsigned long *advertising) {
> > +	struct mii_bus *bus = pcs->bus;
> > +	int addr = pcs->addr;
> > +	u16 if_mode;
> > +	int err;
> > +
> > +	/* SGMII spec requires tx_config_Reg[15:0] to be exactly 0x4001
> > +	 * for the MAC PCS in order to acknowledge the AN.
> > +	 */
> > +	mdiobus_write(bus, addr, MII_ADVERTISE,
> > +		      ADVERTISE_SGMII | ADVERTISE_LPACK);
> 
> This will be overwritten by phylink_mii_c22_pcs_config() below.
> 

Yep, will remove. I've gone through the documentation and the register
should be initialized to 0x0001 when in SGMII mode
(as done by phylink_mii_c22_pcs_config()).

> > +
> > +	if_mode = SGMII_IF_MODE_SGMII_EN;
> > +	if (mode == MLO_AN_INBAND) {
> > +		u32 link_timer;
> > +
> > +		if_mode |= SGMII_IF_MODE_USE_SGMII_AN;
> > +
> > +		/* Adjust link timer for SGMII */
> > +		link_timer =
> SGMII_LINK_TIMER_VAL(SGMII_AN_LINK_TIMER_NS);
> > +		mdiobus_write(bus, addr, SGMII_LINK_TIMER_LO, link_timer &
> 0xffff);
> > +		mdiobus_write(bus, addr, SGMII_LINK_TIMER_HI, link_timer >>
> 16);
> > +	}
> > +	mdiobus_modify(bus, addr, SGMII_IF_MODE,
> > +		       SGMII_IF_MODE_SGMII_EN |
> SGMII_IF_MODE_USE_SGMII_AN,
> > +		       if_mode);
> > +
> > +	err = phylink_mii_c22_pcs_config(pcs, mode,
> PHY_INTERFACE_MODE_SGMII,
> > +					 advertising);
> > +	return err;
> > +}
> > +
> > +static int lynx_pcs_config_usxgmii(struct mdio_device *pcs, unsigned int
> mode,
> > +				   const unsigned long *advertising) {
> > +	struct mii_bus *bus = pcs->bus;
> > +	int addr = pcs->addr;
> > +
> > +	/* Configure device ability for the USXGMII Replicator */
> > +	mdiobus_c45_write(bus, addr, MDIO_MMD_VEND2, MII_ADVERTISE,
> > +			  USXGMII_ADVERTISE_SPEED(USXGMII_SPEED_2500) |
> > +			  USXGMII_ADVERTISE_LSTATUS(1) |
> > +			  ADVERTISE_SGMII |
> > +			  ADVERTISE_LPACK |
> > +			  USXGMII_ADVERTISE_FDX);
> > +	return 0;
> > +}
> > +
> > +int lynx_pcs_config(struct mdio_device *pcs, unsigned int mode,
> > +		    phy_interface_t ifmode,
> > +		    const unsigned long *advertising) {
> > +	switch (ifmode) {
> > +	case PHY_INTERFACE_MODE_SGMII:
> > +	case PHY_INTERFACE_MODE_QSGMII:
> > +		lynx_pcs_config_sgmii(pcs, mode, advertising);
> > +		break;
> > +	case PHY_INTERFACE_MODE_2500BASEX:
> > +		/* 2500Base-X only works without in-band AN,
> > +		 * thus nothing to do here
> > +		 */
> > +		break;
> > +	case PHY_INTERFACE_MODE_USXGMII:
> > +		lynx_pcs_config_usxgmii(pcs, mode, advertising);
> > +		break;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(lynx_pcs_config);
> > +
> > +static void lynx_pcs_link_up_sgmii(struct mdio_device *pcs, unsigned int
> mode,
> > +				   int speed, int duplex)
> > +{
> > +	struct mii_bus *bus = pcs->bus;
> > +	u16 if_mode = 0, sgmii_speed;
> > +	int addr = pcs->addr;
> > +
> > +	/* The PCS needs to be configured manually only
> > +	 * when not operating on in-band mode
> > +	 */
> > +	if (mode == MLO_AN_INBAND)
> > +		return;
> > +
> > +	if (duplex == DUPLEX_HALF)
> > +		if_mode |= SGMII_IF_MODE_DUPLEX;
> > +
> > +	switch (speed) {
> > +	case SPEED_1000:
> > +		sgmii_speed = SGMII_SPEED_1000;
> > +		break;
> > +	case SPEED_100:
> > +		sgmii_speed = SGMII_SPEED_100;
> > +		break;
> > +	case SPEED_10:
> > +		sgmii_speed = SGMII_SPEED_10;
> > +		break;
> > +	case SPEED_UNKNOWN:
> > +		/* Silently don't do anything */
> > +		return;
> > +	default:
> > +		dev_err(&pcs->dev, "Invalid PCS speed %d\n", speed);
> > +		return;
> > +	}
> > +	if_mode |= SGMII_IF_MODE_SPEED(sgmii_speed);
> > +
> > +	mdiobus_modify(bus, addr, SGMII_IF_MODE,
> > +		       SGMII_IF_MODE_DUPLEX | SGMII_IF_MODE_SPEED_MSK,
> > +		       if_mode);
> > +}
> > +
> > +/* 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.
> > + * 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.
> > + */
> > +static void lynx_pcs_link_up_2500basex(struct mdio_device *pcs,
> > +				       unsigned int mode,
> > +				       int speed, int duplex)
> > +{
> > +	struct mii_bus *bus = pcs->bus;
> > +	int addr = pcs->addr;
> > +
> > +	if (mode == MLO_AN_INBAND) {
> > +		dev_err(&pcs->dev, "AN not supported for 2500BaseX\n");
> > +		return;
> > +	}
> > +
> > +	mdiobus_write(bus, addr, SGMII_IF_MODE,
> > +		      SGMII_IF_MODE_SGMII_EN |
> > +		      SGMII_IF_MODE_SPEED(SGMII_SPEED_2500));
> > +}
> > +
> > +void lynx_pcs_link_up(struct mdio_device *pcs, unsigned int mode,
> > +		      phy_interface_t interface,
> > +		      int speed, int duplex)
> > +{
> > +	switch (interface) {
> > +	case PHY_INTERFACE_MODE_SGMII:
> > +	case PHY_INTERFACE_MODE_QSGMII:
> > +		lynx_pcs_link_up_sgmii(pcs, mode, speed, duplex);
> > +		break;
> > +	case PHY_INTERFACE_MODE_2500BASEX:
> > +		lynx_pcs_link_up_2500basex(pcs, mode, speed, duplex);
> > +		break;
> > +	case PHY_INTERFACE_MODE_USXGMII:
> > +		/* At the moment, only in-band AN is supported for USXGMII
> > +		 * so nothing to do in link_up
> > +		 */
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +}
> > +EXPORT_SYMBOL(lynx_pcs_link_up);
> > +
> > +MODULE_LICENSE("Dual BSD/GPL");
> > diff --git a/include/linux/pcs-lynx.h b/include/linux/pcs-lynx.h new
> > file mode 100644 index 000000000000..336fccb8c55f
> > --- /dev/null
> > +++ b/include/linux/pcs-lynx.h
> > @@ -0,0 +1,25 @@
> > +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
> > +/* Copyright 2020 NXP
> > + * Lynx PCS helpers
> > + */
> > +
> > +#ifndef __LINUX_PCS_LYNX_H
> > +#define __LINUX_PCS_LYNX_H
> > +
> > +#include <linux/phy.h>
> > +#include <linux/mdio.h>
> > +
> > +void lynx_pcs_an_restart(struct mdio_device *pcs, phy_interface_t
> > +ifmode);
> > +
> > +void lynx_pcs_get_state(struct mdio_device *pcs, phy_interface_t ifmode,
> > +			struct phylink_link_state *state);
> > +
> > +int lynx_pcs_config(struct mdio_device *pcs, unsigned int mode,
> > +		    phy_interface_t ifmode,
> > +		    const unsigned long *advertising);
> > +
> > +void lynx_pcs_link_up(struct mdio_device *pcs, unsigned int mode,
> > +		      phy_interface_t interface,
> > +		      int speed, int duplex);
> > +
> > +#endif /* __LINUX_PCS_LYNX_H */
> > --
> > 2.25.1
> >
> >
> 

Powered by blists - more mailing lists