diff mbox series

[net-next,3/6] net: lan966x: Add QUSGMII support for lan966x

Message ID 20220519135647.465653-4-maxime.chevallier@bootlin.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: Introduce Ethernet Inband Extensions | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/apply fail Patch does not apply to net-next

Commit Message

Maxime Chevallier May 19, 2022, 1:56 p.m. UTC
The Lan996x controller supports the QUSGMII mode, which is very similar
to QSGMII in the way it's configured and the autonegociation
capababilities it provides.

This commit adds support for that mode, treating it most of the time
like QSGMII, making sure that we do configure the PCS how we should.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 .../ethernet/microchip/lan966x/lan966x_main.c |  2 ++
 .../ethernet/microchip/lan966x/lan966x_main.h |  6 +++++
 .../microchip/lan966x/lan966x_phylink.c       |  9 +++++++-
 .../ethernet/microchip/lan966x/lan966x_port.c | 22 ++++++++++++++-----
 .../ethernet/microchip/lan966x/lan966x_regs.h |  6 +++++
 5 files changed, 38 insertions(+), 7 deletions(-)

Comments

Russell King (Oracle) May 19, 2022, 2:26 p.m. UTC | #1
Hi,

On Thu, May 19, 2022 at 03:56:44PM +0200, Maxime Chevallier wrote:
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> index e6642083ab9e..304c784f48f6 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> @@ -452,4 +452,10 @@ static inline void lan_rmw(u32 val, u32 mask, struct lan966x *lan966x,
>  			      gcnt, gwidth, raddr, rinst, rcnt, rwidth));
>  }
>  
> +static inline bool lan966x_is_qsgmii(phy_interface_t mode)
> +{
> +	return (mode == PHY_INTERFACE_MODE_QSGMII) ||
> +	       (mode == PHY_INTERFACE_MODE_QUSGMII);
> +}

Maybe linux/phy.h should provide a helper, something like:

	phy_interface_serdes_lanes()

that returns how many serdes lanes the interface mode uses?

> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c b/drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c
> index 38a7e95d69b4..96708352f53e 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c
> @@ -28,11 +28,18 @@ static int lan966x_phylink_mac_prepare(struct phylink_config *config,
>  				       phy_interface_t iface)
>  {
>  	struct lan966x_port *port = netdev_priv(to_net_dev(config->dev));
> +	phy_interface_t serdes_mode = iface;
>  	int err;
>  
>  	if (port->serdes) {
> +		/* As far as the SerDes is concerned, QUSGMII is the same as
> +		 * QSGMII.
> +		 */
> +		if (lan966x_is_qsgmii(iface))
> +			serdes_mode = PHY_INTERFACE_MODE_QSGMII;
> +
>  		err = phy_set_mode_ext(port->serdes, PHY_MODE_ETHERNET,
> -				       iface);
> +				       serdes_mode);

I don't think that the ethernet MAC driver should be changing the
interface mode before passing it down to the generic PHY layer -
phy_set_mode_ext() is defined to take the phy interface mode, and any
aliasing of modes should really be up to the generic PHY driver not
the ethernet MAC driver.

Thanks.
Maxime Chevallier July 27, 2022, 1:48 p.m. UTC | #2
Hello Russell,

On Thu, 19 May 2022 15:26:23 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> Hi,
> 
> On Thu, May 19, 2022 at 03:56:44PM +0200, Maxime Chevallier wrote:
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
> > b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h index
> > e6642083ab9e..304c784f48f6 100644 ---
> > a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h +++
> > b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h @@ -452,4
> > +452,10 @@ static inline void lan_rmw(u32 val, u32 mask, struct
> > lan966x *lan966x, gcnt, gwidth, raddr, rinst, rcnt, rwidth)); }
> >  
> > +static inline bool lan966x_is_qsgmii(phy_interface_t mode)
> > +{
> > +	return (mode == PHY_INTERFACE_MODE_QSGMII) ||
> > +	       (mode == PHY_INTERFACE_MODE_QUSGMII);
> > +}  
>
> Maybe linux/phy.h should provide a helper, something like:
> 
> 	phy_interface_serdes_lanes()
> 
> that returns how many serdes lanes the interface mode uses?

Sorry about the delayed answer, I was resuming the work on this, and
realised that although a helper would be indeed great, especially for
generic PHY drivers, it won't help much in this case since
QSGMII/QUSGMII both use 1 serdes lane, as SGMII and such. If I'm not
mistaken, QSGMII is SGMII clocked at 5Gbps with a specific preamble
allowing to identify the src/dst port.

We could however imagine a helper identifying the number of links, or
lanes (or another terminology) that is carried by a given mode. I know
that besides QSGMII for 4 ports, there exists PSGMII for 5 ports, and
OSGMII for 8 ports, so this would definitely prove useful in the
future.

Sorry if this ends-up being a misunderstanding on the terminology,
we're probably already talking about the same thing, but I think that
"serdes lane" would better describe the number of physical differential
pairs that creates the link (like, 1 for SGMII, 2 for RXAUI, 4 for XAUI
and so on).

maybe something like

	phy_interface_lines() or
	phy_interface_num_ports() or simply
	phy_interface_lanes()

> > diff --git
> > a/drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c
> > b/drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c index
> > 38a7e95d69b4..96708352f53e 100644 ---
> > a/drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c +++
> > b/drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c @@
> > -28,11 +28,18 @@ static int lan966x_phylink_mac_prepare(struct
> > phylink_config *config, phy_interface_t iface) { struct
> > lan966x_port *port = netdev_priv(to_net_dev(config->dev));
> > +	phy_interface_t serdes_mode = iface;
> >  	int err;
> >  
> >  	if (port->serdes) {
> > +		/* As far as the SerDes is concerned, QUSGMII is
> > the same as
> > +		 * QSGMII.
> > +		 */
> > +		if (lan966x_is_qsgmii(iface))
> > +			serdes_mode = PHY_INTERFACE_MODE_QSGMII;
> > +
> >  		err = phy_set_mode_ext(port->serdes,
> > PHY_MODE_ETHERNET,
> > -				       iface);
> > +				       serdes_mode);  
> 
> I don't think that the ethernet MAC driver should be changing the
> interface mode before passing it down to the generic PHY layer -
> phy_set_mode_ext() is defined to take the phy interface mode, and any
> aliasing of modes should really be up to the generic PHY driver not
> the ethernet MAC driver.

Indeed, I'll split the series so that we first add support for the new
mode, and then send separate series for the generic PHY driver on one
side, and inband extensions on the other one.

Thanks,

Maxime

> Thanks.
>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
index ca1cef79b83f..b8c2ef905e46 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
@@ -728,6 +728,8 @@  static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
 		  port->phylink_config.supported_interfaces);
 	__set_bit(PHY_INTERFACE_MODE_QSGMII,
 		  port->phylink_config.supported_interfaces);
+	__set_bit(PHY_INTERFACE_MODE_QUSGMII,
+		  port->phylink_config.supported_interfaces);
 	__set_bit(PHY_INTERFACE_MODE_1000BASEX,
 		  port->phylink_config.supported_interfaces);
 	__set_bit(PHY_INTERFACE_MODE_2500BASEX,
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
index e6642083ab9e..304c784f48f6 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
@@ -452,4 +452,10 @@  static inline void lan_rmw(u32 val, u32 mask, struct lan966x *lan966x,
 			      gcnt, gwidth, raddr, rinst, rcnt, rwidth));
 }
 
+static inline bool lan966x_is_qsgmii(phy_interface_t mode)
+{
+	return (mode == PHY_INTERFACE_MODE_QSGMII) ||
+	       (mode == PHY_INTERFACE_MODE_QUSGMII);
+}
+
 #endif /* __LAN966X_MAIN_H__ */
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c b/drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c
index 38a7e95d69b4..96708352f53e 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c
@@ -28,11 +28,18 @@  static int lan966x_phylink_mac_prepare(struct phylink_config *config,
 				       phy_interface_t iface)
 {
 	struct lan966x_port *port = netdev_priv(to_net_dev(config->dev));
+	phy_interface_t serdes_mode = iface;
 	int err;
 
 	if (port->serdes) {
+		/* As far as the SerDes is concerned, QUSGMII is the same as
+		 * QSGMII.
+		 */
+		if (lan966x_is_qsgmii(iface))
+			serdes_mode = PHY_INTERFACE_MODE_QSGMII;
+
 		err = phy_set_mode_ext(port->serdes, PHY_MODE_ETHERNET,
-				       iface);
+				       serdes_mode);
 		if (err) {
 			netdev_err(to_net_dev(config->dev),
 				   "Could not set mode of SerDes\n");
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_port.c b/drivers/net/ethernet/microchip/lan966x/lan966x_port.c
index f141644e4372..ef65a44b2d34 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_port.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_port.c
@@ -168,7 +168,7 @@  static void lan966x_port_link_up(struct lan966x_port *port)
 	/* Also the GIGA_MODE_ENA(1) needs to be set regardless of the
 	 * port speed for QSGMII ports.
 	 */
-	if (config->portmode == PHY_INTERFACE_MODE_QSGMII)
+	if (lan966x_is_qsgmii(config->portmode))
 		mode = DEV_MAC_MODE_CFG_GIGA_MODE_ENA_SET(1);
 
 	lan_wr(config->duplex | mode,
@@ -331,10 +331,14 @@  int lan966x_port_pcs_set(struct lan966x_port *port,
 	struct lan966x *lan966x = port->lan966x;
 	bool inband_aneg = false;
 	bool outband;
+	bool full_preamble = false;
+
+	if (config->portmode == PHY_INTERFACE_MODE_QUSGMII)
+		full_preamble = true;
 
 	if (config->inband) {
 		if (config->portmode == PHY_INTERFACE_MODE_SGMII ||
-		    config->portmode == PHY_INTERFACE_MODE_QSGMII)
+		    lan966x_is_qsgmii(config->portmode))
 			inband_aneg = true; /* Cisco-SGMII in-band-aneg */
 		else if (config->portmode == PHY_INTERFACE_MODE_1000BASEX &&
 			 config->autoneg)
@@ -345,9 +349,15 @@  int lan966x_port_pcs_set(struct lan966x_port *port,
 		outband = true;
 	}
 
-	/* Disable or enable inband */
-	lan_rmw(DEV_PCS1G_MODE_CFG_SGMII_MODE_ENA_SET(outband),
-		DEV_PCS1G_MODE_CFG_SGMII_MODE_ENA,
+	/* Disable or enable inband.
+	 * For QUSGMII, we rely on the preamble to transmit data such as
+	 * timestamps, therefore force full preamble transmission, and prevent
+	 * premable shortening
+	 */
+	lan_rmw(DEV_PCS1G_MODE_CFG_SGMII_MODE_ENA_SET(outband) |
+		DEV_PCS1G_MODE_CFG_SAVE_PREAMBLE_ENA_SET(full_preamble),
+		DEV_PCS1G_MODE_CFG_SGMII_MODE_ENA |
+		DEV_PCS1G_MODE_CFG_SAVE_PREAMBLE_ENA,
 		lan966x, DEV_PCS1G_MODE_CFG(port->chip_port));
 
 	/* Enable PCS */
@@ -396,7 +406,7 @@  void lan966x_port_init(struct lan966x_port *port)
 	if (lan966x->fdma)
 		lan966x_fdma_netdev_init(lan966x, port->dev);
 
-	if (config->portmode != PHY_INTERFACE_MODE_QSGMII)
+	if (!lan966x_is_qsgmii(config->portmode))
 		return;
 
 	lan_rmw(DEV_CLOCK_CFG_PCS_RX_RST_SET(0) |
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h b/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h
index 2f59285bef29..d4839e4b8ed5 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h
@@ -504,6 +504,12 @@  enum lan966x_target {
 #define DEV_PCS1G_MODE_CFG_SGMII_MODE_ENA_GET(x)\
 	FIELD_GET(DEV_PCS1G_MODE_CFG_SGMII_MODE_ENA, x)
 
+#define DEV_PCS1G_MODE_CFG_SAVE_PREAMBLE_ENA        BIT(1)
+#define DEV_PCS1G_MODE_CFG_SAVE_PREAMBLE_ENA_SET(x)\
+	FIELD_PREP(DEV_PCS1G_MODE_CFG_SAVE_PREAMBLE_ENA, x)
+#define DEV_PCS1G_MODE_CFG_SAVE_PREAMBLE_ENA_GET(x)\
+	FIELD_GET(DEV_PCS1G_MODE_CFG_SAVE_PREAMBLE_ENA, x)
+
 /*      DEV:PCS1G_CFG_STATUS:PCS1G_SD_CFG */
 #define DEV_PCS1G_SD_CFG(t)       __REG(TARGET_DEV, t, 8, 72, 0, 1, 68, 8, 0, 1, 4)