[PATCH 0/2] net: dsa: qca8k: Add SGMII configuration options

[PATCH 0/2] net: dsa: qca8k: Add SGMII configuration options

[Jonathan McDowell]

This pair of patches adds some SGMII device tree configuration options
for the QCA8K switch driver, and the associated documentation.

At present the driver does no configuration of the SGMII port, even if
it is selected. These changes allow configuration of how it is connected
up (i.e. connected to an external phy, or to a CPU, or to an SFP cage)
as well as allowing for autonegotiation to be disabled and a delay
configured.

Tested on a MikroTik RB3011; the second switch is connected to the CPU
via SGMII.

Jonathan McDowell (2):
  dt-bindings: net: dsa: qca8k: document SGMII properties
  net: dsa: qca8k: introduce SGMII configuration options

 .../devicetree/bindings/net/dsa/qca8k.txt     |  4 ++
 drivers/net/dsa/qca8k.c                       | 44 ++++++++++++++++++-
 drivers/net/dsa/qca8k.h                       | 12 +++++
 3 files changed, 59 insertions(+), 1 deletion(-)

-- 
2.20.1

[PATCH 1/2] dt-bindings: net: dsa: qca8k: document SGMII properties

[Jonathan McDowell]

This patch documents the qca8k's SGMII related properties that allow
configuration of the SGMII port.

Signed-off-by: Jonathan McDowell <noodles@earth.li>
---
 Documentation/devicetree/bindings/net/dsa/qca8k.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
index ccbc6d89325d..9e7d74a248ad 100644
--- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
+++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
@@ -11,7 +11,11 @@ Required properties:
 
 Optional properties:
 
+- disable-serdes-autoneg: Boolean, disables auto-negotiation on the SerDes
 - reset-gpios: GPIO to be used to reset the whole device
+- sgmii-delay: Boolean, presence delays SGMII clock by 2ns
+- sgmii-mode: String, operation mode of the SGMII interface.
+  Supported values are: "basex", "mac", "phy".
 
 Subnodes:
 
-- 
2.20.1

Re: [PATCH 1/2] dt-bindings: net: dsa: qca8k: document SGMII properties

[Rob Herring]

On Fri, Jun 05, 2020 at 07:10:02PM +0100, Jonathan McDowell wrote:
> This patch documents the qca8k's SGMII related properties that allow
> configuration of the SGMII port.
> 
> Signed-off-by: Jonathan McDowell <noodles@earth.li>
> ---
>  Documentation/devicetree/bindings/net/dsa/qca8k.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> index ccbc6d89325d..9e7d74a248ad 100644
> --- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> @@ -11,7 +11,11 @@ Required properties:
>  
>  Optional properties:
>  
> +- disable-serdes-autoneg: Boolean, disables auto-negotiation on the SerDes
>  - reset-gpios: GPIO to be used to reset the whole device
> +- sgmii-delay: Boolean, presence delays SGMII clock by 2ns
> +- sgmii-mode: String, operation mode of the SGMII interface.
> +  Supported values are: "basex", "mac", "phy".

Either these should be common properties and documented in a common 
spot or they need vendor prefixes. They seem like they former to me 
(though 'sgmii-delay' would need to be more general and take a time).

Rob

Re: [PATCH 1/2] dt-bindings: net: dsa: qca8k: document SGMII properties

[Jonathan McDowell]

On Mon, Jun 15, 2020 at 11:45:16AM -0600, Rob Herring wrote:
> On Fri, Jun 05, 2020 at 07:10:02PM +0100, Jonathan McDowell wrote:
> > This patch documents the qca8k's SGMII related properties that allow
> > configuration of the SGMII port.
> > 
> > Signed-off-by: Jonathan McDowell <noodles@earth.li>
> > ---
> >  Documentation/devicetree/bindings/net/dsa/qca8k.txt | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> > index ccbc6d89325d..9e7d74a248ad 100644
> > --- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> > +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> > @@ -11,7 +11,11 @@ Required properties:
> >  
> >  Optional properties:
> >  
> > +- disable-serdes-autoneg: Boolean, disables auto-negotiation on the SerDes
> >  - reset-gpios: GPIO to be used to reset the whole device
> > +- sgmii-delay: Boolean, presence delays SGMII clock by 2ns
> > +- sgmii-mode: String, operation mode of the SGMII interface.
> > +  Supported values are: "basex", "mac", "phy".
> 
> Either these should be common properties and documented in a common 
> spot or they need vendor prefixes. They seem like they former to me 
> (though 'sgmii-delay' would need to be more general and take a time).

I've managed to spin a subsequent revision which avoids the need for a
device tree change, based on comments similar to yours. I'll keep them
in mind should it become necessary to re-introduce the DT options.

J.

-- 
] https://www.earth.li/~noodles/ []  I'm a creep, I'm a weirdo, what   [
]  PGP/GPG Key @ the.earth.li    []     the hell am I doing here?      [
] via keyserver, web or email.   []                                    [
] RSA: 4096/0x94FA372B2DA8B985   []                                    [

[PATCH 2/2] net: dsa: qca8k: introduce SGMII configuration options

[Jonathan McDowell]

The QCA8337(N) has an SGMII port which can operate in MAC, PHY or BASE-X
mode depending on what it's connected to (e.g. CPU vs external PHY or
SFP). At present the driver does no configuration of this port even if
it is selected.

Add support for making sure the SGMII is enabled if it's in use, and
device tree support for configuring the connection details.

Signed-off-by: Jonathan McDowell <noodles@earth.li>
---
 drivers/net/dsa/qca8k.c | 44 ++++++++++++++++++++++++++++++++++++++++-
 drivers/net/dsa/qca8k.h | 12 +++++++++++
 2 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 9f4205b4439b..5b7979aca9b9 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -418,6 +418,40 @@ qca8k_mib_init(struct qca8k_priv *priv)
 	mutex_unlock(&priv->reg_mutex);
 }
 
+static int
+qca8k_setup_sgmii(struct qca8k_priv *priv)
+{
+	const char *mode;
+	u32 val;
+
+	val = qca8k_read(priv, QCA8K_REG_SGMII_CTRL);
+
+	val |= QCA8K_SGMII_EN_PLL | QCA8K_SGMII_EN_RX |
+		QCA8K_SGMII_EN_TX | QCA8K_SGMII_EN_SD;
+
+	if (of_property_read_bool(priv->dev->of_node, "sgmii-delay"))
+		val |= QCA8K_SGMII_CLK125M_DELAY;
+
+	if (of_property_read_string(priv->dev->of_node, "sgmii-mode", &mode)) {
+		val &= ~QCA8K_SGMII_MODE_CTRL_MASK;
+
+		if (!strcasecmp(mode, "basex")) {
+			val |= QCA8K_SGMII_MODE_CTRL_BASEX;
+		} else if (!strcasecmp(mode, "mac")) {
+			val |= QCA8K_SGMII_MODE_CTRL_MAC;
+		} else if (!strcasecmp(mode, "phy")) {
+			val |= QCA8K_SGMII_MODE_CTRL_PHY;
+		} else {
+			pr_err("Unrecognised SGMII mode %s\n", mode);
+			return -EINVAL;
+		}
+	}
+
+	qca8k_write(priv, QCA8K_REG_SGMII_CTRL, val);
+
+	return 0;
+}
+
 static int
 qca8k_set_pad_ctrl(struct qca8k_priv *priv, int port, int mode)
 {
@@ -458,7 +492,8 @@ qca8k_set_pad_ctrl(struct qca8k_priv *priv, int port, int mode)
 		break;
 	case PHY_INTERFACE_MODE_SGMII:
 		qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
-		break;
+
+		return qca8k_setup_sgmii(priv);
 	default:
 		pr_err("xMII mode %d not supported\n", mode);
 		return -EINVAL;
@@ -661,6 +696,13 @@ qca8k_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
+	if (of_property_read_bool(priv->dev->of_node,
+				  "disable-serdes-autoneg")) {
+		mask = qca8k_read(priv, QCA8K_REG_PWS) |
+		       QCA8K_PWS_SERDES_AEN_DIS;
+		qca8k_write(priv, QCA8K_REG_PWS, mask);
+	}
+
 	/* Initialize CPU port pad mode (xMII type, delays...) */
 	ret = of_get_phy_mode(dsa_to_port(ds, QCA8K_CPU_PORT)->dn, &phy_mode);
 	if (ret) {
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 42d6ea24eb14..cd97c212f3f8 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -36,6 +36,8 @@
 #define   QCA8K_MAX_DELAY				3
 #define   QCA8K_PORT_PAD_RGMII_RX_DELAY_EN		BIT(24)
 #define   QCA8K_PORT_PAD_SGMII_EN			BIT(7)
+#define QCA8K_REG_PWS					0x010
+#define   QCA8K_PWS_SERDES_AEN_DIS			BIT(7)
 #define QCA8K_REG_MODULE_EN				0x030
 #define   QCA8K_MODULE_EN_MIB				BIT(0)
 #define QCA8K_REG_MIB					0x034
@@ -77,6 +79,16 @@
 #define   QCA8K_PORT_HDR_CTRL_ALL			2
 #define   QCA8K_PORT_HDR_CTRL_MGMT			1
 #define   QCA8K_PORT_HDR_CTRL_NONE			0
+#define QCA8K_REG_SGMII_CTRL				0x0e0
+#define   QCA8K_SGMII_EN_PLL				BIT(1)
+#define   QCA8K_SGMII_EN_RX				BIT(2)
+#define   QCA8K_SGMII_EN_TX				BIT(3)
+#define   QCA8K_SGMII_EN_SD				BIT(4)
+#define   QCA8K_SGMII_CLK125M_DELAY			BIT(7)
+#define   QCA8K_SGMII_MODE_CTRL_MASK			(BIT(22) | BIT(23))
+#define   QCA8K_SGMII_MODE_CTRL_BASEX			0
+#define   QCA8K_SGMII_MODE_CTRL_PHY			BIT(22)
+#define   QCA8K_SGMII_MODE_CTRL_MAC			BIT(23)
 
 /* EEE control registers */
 #define QCA8K_REG_EEE_CTRL				0x100
-- 
2.20.1

Re: [PATCH 2/2] net: dsa: qca8k: introduce SGMII configuration options

[Marek Behun]

On Fri, 5 Jun 2020 19:10:58 +0100
Jonathan McDowell <noodles@earth.li> wrote:

> The QCA8337(N) has an SGMII port which can operate in MAC, PHY or BASE-X
> mode depending on what it's connected to (e.g. CPU vs external PHY or
> SFP). At present the driver does no configuration of this port even if
> it is selected.
> 
> Add support for making sure the SGMII is enabled if it's in use, and
> device tree support for configuring the connection details.
> 
> Signed-off-by: Jonathan McDowell <noodles@earth.li>
> ---
>  drivers/net/dsa/qca8k.c | 44 ++++++++++++++++++++++++++++++++++++++++-
>  drivers/net/dsa/qca8k.h | 12 +++++++++++
>  2 files changed, 55 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index 9f4205b4439b..5b7979aca9b9 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -418,6 +418,40 @@ qca8k_mib_init(struct qca8k_priv *priv)
>  	mutex_unlock(&priv->reg_mutex);
>  }
>  
> +static int
> +qca8k_setup_sgmii(struct qca8k_priv *priv)
> +{
> +	const char *mode;
> +	u32 val;
> +
> +	val = qca8k_read(priv, QCA8K_REG_SGMII_CTRL);
> +
> +	val |= QCA8K_SGMII_EN_PLL | QCA8K_SGMII_EN_RX |
> +		QCA8K_SGMII_EN_TX | QCA8K_SGMII_EN_SD;
> +
> +	if (of_property_read_bool(priv->dev->of_node, "sgmii-delay"))
> +		val |= QCA8K_SGMII_CLK125M_DELAY;
> +
> +	if (of_property_read_string(priv->dev->of_node, "sgmii-mode", &mode)) {
> +		val &= ~QCA8K_SGMII_MODE_CTRL_MASK;
> +
> +		if (!strcasecmp(mode, "basex")) {
> +			val |= QCA8K_SGMII_MODE_CTRL_BASEX;
> +		} else if (!strcasecmp(mode, "mac")) {
> +			val |= QCA8K_SGMII_MODE_CTRL_MAC;
> +		} else if (!strcasecmp(mode, "phy")) {
> +			val |= QCA8K_SGMII_MODE_CTRL_PHY;
> +		} else {
> +			pr_err("Unrecognised SGMII mode %s\n", mode);
> +			return -EINVAL;
> +		}
> +	}

There is no sgmii-mode device tree property documented. You should
infere this settings from the existing device tree bindings, ie look at
phy-mode. You can use of_get_phy_mode function, or something from
of_mdio.c, or better yet change the api in this driver to use the new
phylink API.

Marek


> +
> +	qca8k_write(priv, QCA8K_REG_SGMII_CTRL, val);
> +
> +	return 0;
> +}
> +
>  static int
>  qca8k_set_pad_ctrl(struct qca8k_priv *priv, int port, int mode)
>  {
> @@ -458,7 +492,8 @@ qca8k_set_pad_ctrl(struct qca8k_priv *priv, int port, int mode)
>  		break;
>  	case PHY_INTERFACE_MODE_SGMII:
>  		qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
> -		break;
> +
> +		return qca8k_setup_sgmii(priv);
>  	default:
>  		pr_err("xMII mode %d not supported\n", mode);
>  		return -EINVAL;
> @@ -661,6 +696,13 @@ qca8k_setup(struct dsa_switch *ds)
>  	if (ret)
>  		return ret;
>  
> +	if (of_property_read_bool(priv->dev->of_node,
> +				  "disable-serdes-autoneg")) {
> +		mask = qca8k_read(priv, QCA8K_REG_PWS) |
> +		       QCA8K_PWS_SERDES_AEN_DIS;
> +		qca8k_write(priv, QCA8K_REG_PWS, mask);
> +	}
> +
>  	/* Initialize CPU port pad mode (xMII type, delays...) */
>  	ret = of_get_phy_mode(dsa_to_port(ds, QCA8K_CPU_PORT)->dn, &phy_mode);
>  	if (ret) {
> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> index 42d6ea24eb14..cd97c212f3f8 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -36,6 +36,8 @@
>  #define   QCA8K_MAX_DELAY				3
>  #define   QCA8K_PORT_PAD_RGMII_RX_DELAY_EN		BIT(24)
>  #define   QCA8K_PORT_PAD_SGMII_EN			BIT(7)
> +#define QCA8K_REG_PWS					0x010
> +#define   QCA8K_PWS_SERDES_AEN_DIS			BIT(7)
>  #define QCA8K_REG_MODULE_EN				0x030
>  #define   QCA8K_MODULE_EN_MIB				BIT(0)
>  #define QCA8K_REG_MIB					0x034
> @@ -77,6 +79,16 @@
>  #define   QCA8K_PORT_HDR_CTRL_ALL			2
>  #define   QCA8K_PORT_HDR_CTRL_MGMT			1
>  #define   QCA8K_PORT_HDR_CTRL_NONE			0
> +#define QCA8K_REG_SGMII_CTRL				0x0e0
> +#define   QCA8K_SGMII_EN_PLL				BIT(1)
> +#define   QCA8K_SGMII_EN_RX				BIT(2)
> +#define   QCA8K_SGMII_EN_TX				BIT(3)
> +#define   QCA8K_SGMII_EN_SD				BIT(4)
> +#define   QCA8K_SGMII_CLK125M_DELAY			BIT(7)
> +#define   QCA8K_SGMII_MODE_CTRL_MASK			(BIT(22) | BIT(23))
> +#define   QCA8K_SGMII_MODE_CTRL_BASEX			0
> +#define   QCA8K_SGMII_MODE_CTRL_PHY			BIT(22)
> +#define   QCA8K_SGMII_MODE_CTRL_MAC			BIT(23)
>  
>  /* EEE control registers */
>  #define QCA8K_REG_EEE_CTRL				0x100

Re: [PATCH 2/2] net: dsa: qca8k: introduce SGMII configuration options

[Andrew Lunn]

On Fri, Jun 05, 2020 at 07:10:58PM +0100, Jonathan McDowell wrote:
> The QCA8337(N) has an SGMII port which can operate in MAC, PHY or BASE-X
> mode depending on what it's connected to (e.g. CPU vs external PHY or
> SFP). At present the driver does no configuration of this port even if
> it is selected.
> 
> Add support for making sure the SGMII is enabled if it's in use, and
> device tree support for configuring the connection details.

Hi Jonathan

It is good to include Russell King in Cc: for patches like this.

Also, netdev is closed at the moment, so please post patches as RFC.

It sounds like the hardware has a PCS which can support SGMII or
1000BaseX. phylink will tell you what mode to configure it to. e.g. A
fibre SFP module will want 1000BaseX. A copper SFP module will want
SGMII. A switch is likely to want 1000BaseX. A PHY is likely to want
SGMII. So remove the "sgmii-mode" property and configure it as phylink
is requesting.

What exactly does sgmii-delay do?

> +#define QCA8K_REG_SGMII_CTRL				0x0e0
> +#define   QCA8K_SGMII_EN_PLL				BIT(1)
> +#define   QCA8K_SGMII_EN_RX				BIT(2)
> +#define   QCA8K_SGMII_EN_TX				BIT(3)
> +#define   QCA8K_SGMII_EN_SD				BIT(4)
> +#define   QCA8K_SGMII_CLK125M_DELAY			BIT(7)
> +#define   QCA8K_SGMII_MODE_CTRL_MASK			(BIT(22) | BIT(23))
> +#define   QCA8K_SGMII_MODE_CTRL_BASEX			0
> +#define   QCA8K_SGMII_MODE_CTRL_PHY			BIT(22)
> +#define   QCA8K_SGMII_MODE_CTRL_MAC			BIT(23)

I guess these are not really bits. You cannot combine
QCA8K_SGMII_MODE_CTRL_MAC and QCA8K_SGMII_MODE_CTRL_PHY. So it makes
more sense to have:

#define   QCA8K_SGMII_MODE_CTRL_BASEX			(0x0 << 22)
#define   QCA8K_SGMII_MODE_CTRL_PHY			(0x1 << 22)
#define   QCA8K_SGMII_MODE_CTRL_MAC			(0x2 << 22)

   Andrew

Re: [PATCH 2/2] net: dsa: qca8k: introduce SGMII configuration options

[Jonathan McDowell]

On Fri, Jun 05, 2020 at 08:38:43PM +0200, Andrew Lunn wrote:
> On Fri, Jun 05, 2020 at 07:10:58PM +0100, Jonathan McDowell wrote:
> > The QCA8337(N) has an SGMII port which can operate in MAC, PHY or BASE-X
> > mode depending on what it's connected to (e.g. CPU vs external PHY or
> > SFP). At present the driver does no configuration of this port even if
> > it is selected.
> > 
> > Add support for making sure the SGMII is enabled if it's in use, and
> > device tree support for configuring the connection details.
> 
> It is good to include Russell King in Cc: for patches like this.

No problem, I can keep him in the thread; I used get_maintainer for the
initial set of people/lists to copy.

> Also, netdev is closed at the moment, so please post patches as RFC.

"closed"? If you mean this won't get into 5.8 then I wasn't expecting it
to, I'm aware the merge window for that is already open.

> It sounds like the hardware has a PCS which can support SGMII or
> 1000BaseX. phylink will tell you what mode to configure it to. e.g. A
> fibre SFP module will want 1000BaseX. A copper SFP module will want
> SGMII. A switch is likely to want 1000BaseX. A PHY is likely to want
> SGMII. So remove the "sgmii-mode" property and configure it as phylink
> is requesting.

It's more than SGMII or 1000BaseX as I read it. The port can act as if
it's talking to an SGMII MAC, i.e. a CPU, or an SGMII PHY, i.e. an
external PHY, or in BaseX mode for an SFP. I couldn't figure out a way
in the current framework to automatically work out if I wanted PHY or
MAC mode. For the port tagged CPU I can assume MAC mode, but a port that
doesn't have that might still be attached to the CPU rather than an
external PHY.

> What exactly does sgmii-delay do?

As per the device tree documentation update I sent it delays the SGMII
clock by 2ns. From the data sheet:

SGMII_SEL_CLK125M	sgmii_clk125m_rx_delay is delayed by 2ns

> > +#define QCA8K_REG_SGMII_CTRL				0x0e0
> > +#define   QCA8K_SGMII_EN_PLL				BIT(1)
> > +#define   QCA8K_SGMII_EN_RX				BIT(2)
> > +#define   QCA8K_SGMII_EN_TX				BIT(3)
> > +#define   QCA8K_SGMII_EN_SD				BIT(4)
> > +#define   QCA8K_SGMII_CLK125M_DELAY			BIT(7)
> > +#define   QCA8K_SGMII_MODE_CTRL_MASK			(BIT(22) | BIT(23))
> > +#define   QCA8K_SGMII_MODE_CTRL_BASEX			0
> > +#define   QCA8K_SGMII_MODE_CTRL_PHY			BIT(22)
> > +#define   QCA8K_SGMII_MODE_CTRL_MAC			BIT(23)
> 
> I guess these are not really bits. You cannot combine
> QCA8K_SGMII_MODE_CTRL_MAC and QCA8K_SGMII_MODE_CTRL_PHY. So it makes
> more sense to have:
> 
> #define   QCA8K_SGMII_MODE_CTRL_BASEX			(0x0 << 22)
> #define   QCA8K_SGMII_MODE_CTRL_PHY			(0x1 << 22)
> #define   QCA8K_SGMII_MODE_CTRL_MAC			(0x2 << 22)

Sure; given there's no 0x3 choice I just went for the bits that need
set, but that works too.

J.

-- 
Mistakes aren't always regrets.

Re: [PATCH 2/2] net: dsa: qca8k: introduce SGMII configuration options

[Russell King - ARM Linux admin]

On Sat, Jun 06, 2020 at 08:49:16AM +0100, Jonathan McDowell wrote:
> On Fri, Jun 05, 2020 at 08:38:43PM +0200, Andrew Lunn wrote:
> > On Fri, Jun 05, 2020 at 07:10:58PM +0100, Jonathan McDowell wrote:
> > > The QCA8337(N) has an SGMII port which can operate in MAC, PHY or BASE-X
> > > mode depending on what it's connected to (e.g. CPU vs external PHY or
> > > SFP). At present the driver does no configuration of this port even if
> > > it is selected.
> > > 
> > > Add support for making sure the SGMII is enabled if it's in use, and
> > > device tree support for configuring the connection details.
> > 
> > It is good to include Russell King in Cc: for patches like this.
> 
> No problem, I can keep him in the thread; I used get_maintainer for the
> initial set of people/lists to copy.

get_maintainer is not always "good" at selecting the right people,
especially when your patches don't match the criteria; MAINTAINERS
contains everything that is sensible, but Andrew is suggesting that
you copy me because in his opinion, you should be using phylink -
and that's something that you can't encode into a program.

Note that I haven't seen your patches.

> > Also, netdev is closed at the moment, so please post patches as RFC.
> 
> "closed"? If you mean this won't get into 5.8 then I wasn't expecting it
> to, I'm aware the merge window for that is already open.

See https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt
"How often do changes from these trees make it to the mainline Linus
tree?"

> > It sounds like the hardware has a PCS which can support SGMII or
> > 1000BaseX. phylink will tell you what mode to configure it to. e.g. A
> > fibre SFP module will want 1000BaseX. A copper SFP module will want
> > SGMII. A switch is likely to want 1000BaseX. A PHY is likely to want
> > SGMII. So remove the "sgmii-mode" property and configure it as phylink
> > is requesting.
> 
> It's more than SGMII or 1000BaseX as I read it. The port can act as if
> it's talking to an SGMII MAC, i.e. a CPU, or an SGMII PHY, i.e. an
> external PHY, or in BaseX mode for an SFP. I couldn't figure out a way
> in the current framework to automatically work out if I wanted PHY or
> MAC mode. For the port tagged CPU I can assume MAC mode, but a port that
> doesn't have that might still be attached to the CPU rather than an
> external PHY.

That depends what you're connected to. Some people call the two sides
of SGMII "System side" and "Media side". System side is where you're
receiving the results of AN from a PHY. Media side is where you're
telling the partner what you want it to do.

Media side is only useful if you're connected to another MAC, and
unless you have a requirement for it, I would suggest not implementing
that - you could come up with something using fixed-link, or it may
need some other model if the settings need to change.  That depends on
the application.

> > What exactly does sgmii-delay do?
> 
> As per the device tree documentation update I sent it delays the SGMII
> clock by 2ns. From the data sheet:
> 
> SGMII_SEL_CLK125M	sgmii_clk125m_rx_delay is delayed by 2ns

This sounds like a new world of RGMII delay pain but for SGMII. There
is no mention of "delay" in the SGMII v1.8 specification, so I guess
it's something the vendor is doing. Is this device capable of
recovering the clock from a single serdes pair carrying the data,
or does it always require the separate clock?

> > > +#define QCA8K_REG_SGMII_CTRL				0x0e0
> > > +#define   QCA8K_SGMII_EN_PLL				BIT(1)
> > > +#define   QCA8K_SGMII_EN_RX				BIT(2)
> > > +#define   QCA8K_SGMII_EN_TX				BIT(3)
> > > +#define   QCA8K_SGMII_EN_SD				BIT(4)
> > > +#define   QCA8K_SGMII_CLK125M_DELAY			BIT(7)
> > > +#define   QCA8K_SGMII_MODE_CTRL_MASK			(BIT(22) | BIT(23))
> > > +#define   QCA8K_SGMII_MODE_CTRL_BASEX			0
> > > +#define   QCA8K_SGMII_MODE_CTRL_PHY			BIT(22)
> > > +#define   QCA8K_SGMII_MODE_CTRL_MAC			BIT(23)
> > 
> > I guess these are not really bits. You cannot combine
> > QCA8K_SGMII_MODE_CTRL_MAC and QCA8K_SGMII_MODE_CTRL_PHY. So it makes
> > more sense to have:
> > 
> > #define   QCA8K_SGMII_MODE_CTRL_BASEX			(0x0 << 22)
> > #define   QCA8K_SGMII_MODE_CTRL_PHY			(0x1 << 22)
> > #define   QCA8K_SGMII_MODE_CTRL_MAC			(0x2 << 22)
> 
> Sure; given there's no 0x3 choice I just went for the bits that need
> set, but that works too.

I also prefer Andrew's suggestion, as it makes it clear that it's a two
bit field.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: [PATCH 2/2] net: dsa: qca8k: introduce SGMII configuration options

[Jonathan McDowell]

On Sat, Jun 06, 2020 at 09:37:41AM +0100, Russell King - ARM Linux admin wrote:
> On Sat, Jun 06, 2020 at 08:49:16AM +0100, Jonathan McDowell wrote:
> > On Fri, Jun 05, 2020 at 08:38:43PM +0200, Andrew Lunn wrote:
> > > On Fri, Jun 05, 2020 at 07:10:58PM +0100, Jonathan McDowell wrote:
> > > > The QCA8337(N) has an SGMII port which can operate in MAC, PHY or BASE-X
> > > > mode depending on what it's connected to (e.g. CPU vs external PHY or
> > > > SFP). At present the driver does no configuration of this port even if
> > > > it is selected.
> > > > 
> > > > Add support for making sure the SGMII is enabled if it's in use, and
> > > > device tree support for configuring the connection details.
> > > 
> > > It is good to include Russell King in Cc: for patches like this.
> > 
> > No problem, I can keep him in the thread; I used get_maintainer for the
> > initial set of people/lists to copy.
> 
> get_maintainer is not always "good" at selecting the right people,
> especially when your patches don't match the criteria; MAINTAINERS
> contains everything that is sensible, but Andrew is suggesting that
> you copy me because in his opinion, you should be using phylink -
> and that's something that you can't encode into a program.

Sure, and I appreciate the pointer to appropriate people who might
provide helpful comments.

> Note that I haven't seen your patches.

I'll make sure to copy you on v2.

> > > Also, netdev is closed at the moment, so please post patches as RFC.
> > 
> > "closed"? If you mean this won't get into 5.8 then I wasn't expecting it
> > to, I'm aware the merge window for that is already open.
> 
> See https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt
> "How often do changes from these trees make it to the mainline Linus
> tree?"

Ta. I'll hold off on a v2 until after -rc1 drops.

> > > It sounds like the hardware has a PCS which can support SGMII or
> > > 1000BaseX. phylink will tell you what mode to configure it to. e.g. A
> > > fibre SFP module will want 1000BaseX. A copper SFP module will want
> > > SGMII. A switch is likely to want 1000BaseX. A PHY is likely to want
> > > SGMII. So remove the "sgmii-mode" property and configure it as phylink
> > > is requesting.
> > 
> > It's more than SGMII or 1000BaseX as I read it. The port can act as if
> > it's talking to an SGMII MAC, i.e. a CPU, or an SGMII PHY, i.e. an
> > external PHY, or in BaseX mode for an SFP. I couldn't figure out a way
> > in the current framework to automatically work out if I wanted PHY or
> > MAC mode. For the port tagged CPU I can assume MAC mode, but a port that
> > doesn't have that might still be attached to the CPU rather than an
> > external PHY.
> 
> That depends what you're connected to. Some people call the two sides
> of SGMII "System side" and "Media side". System side is where you're
> receiving the results of AN from a PHY. Media side is where you're
> telling the partner what you want it to do.
> 
> Media side is only useful if you're connected to another MAC, and
> unless you have a requirement for it, I would suggest not implementing
> that - you could come up with something using fixed-link, or it may
> need some other model if the settings need to change.  That depends on
> the application.

So the device in question is a 7 port stand alone switch chip. There's a
single SGMII port which is configurable between port 0 + 6 (they can
also be configure up as RGMII, while the remaining 5 ports have their
own phys).

It sounds like there's a strong preference to try and auto configure
things as much as possible, so I should assume the CPU port is in MAC
mode, and anything not tagged as a CPU port is talking to a PHY/BASEX.

I assume I can use PHY_INTERFACE_MODE_1000BASEX on the
phylink_mac_config call to choose BASEX?

> > > What exactly does sgmii-delay do?
> > 
> > As per the device tree documentation update I sent it delays the SGMII
> > clock by 2ns. From the data sheet:
> > 
> > SGMII_SEL_CLK125M	sgmii_clk125m_rx_delay is delayed by 2ns
> 
> This sounds like a new world of RGMII delay pain but for SGMII. There
> is no mention of "delay" in the SGMII v1.8 specification, so I guess
> it's something the vendor is doing. Is this device capable of
> recovering the clock from a single serdes pair carrying the data,
> or does it always require the separate clock?

Pass, but I think I might be able to get away without having to
configure that for the moment.

I'll go away and roll a v2 moving qca8k over to phylink and then using
that to auto select the appropriate SGMII mode. Thanks for the feedback.

J.

-- 
I started out with nothing & still have most of it left.

Re: [PATCH 2/2] net: dsa: qca8k: introduce SGMII configuration options

[Russell King - ARM Linux admin]

On Sat, Jun 06, 2020 at 11:59:09AM +0100, Jonathan McDowell wrote:
> So the device in question is a 7 port stand alone switch chip. There's a
> single SGMII port which is configurable between port 0 + 6 (they can
> also be configure up as RGMII, while the remaining 5 ports have their
> own phys).
> 
> It sounds like there's a strong preference to try and auto configure
> things as much as possible, so I should assume the CPU port is in MAC
> mode, and anything not tagged as a CPU port is talking to a PHY/BASEX.
> 
> I assume I can use PHY_INTERFACE_MODE_1000BASEX on the
> phylink_mac_config call to choose BASEX?

Yes, but from what you've mentioned above, I think I need to ensure that
there's a proper understanding here.

1000BASE-X is the IEEE 802.3 defined 1G single lane Serdes protocol.
SGMII is different; it's a vendor derivative of 1000BASE-X which has
become a de-facto standard.

Both are somewhat compatible with each other; SGMII brings with it
additional data replication to achieve 100M and 10M speeds, while
keeping the link running at 1.25Gbaud.  In both cases, there is a
16-bit "configuration" word that is passed between the partners.

1000BASE-X uses this configuration word to advertise the abilities of
each end, which is limited to duplex and pause modes only.  This you
get by specifying the phy-mode="1000base-x" and
managed="in-band-status" in DT.

SGMII uses this configuration word for the media side to inform the
system side which mode it wishes to operate the link: the speed and
duplex.  Some vendors extend it to include EEE parameters as well,
or pause modes.  You get this via phy-mode="sgmii" and
managed="in-band-status" in DT.

Then there are variants where the configuration word is not present.
In this case, the link has to be manually configured, and without the
configuration word, SGMII operating at 1G is compatible with
1000base-X operating at 1G.  Fixed-link can be used for this, although
fixed-link will always report that the link is up at the moment; that
may change in the future, it's something that is being looked into at
the moment.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: [PATCH 2/2] net: dsa: qca8k: introduce SGMII configuration options

[Jonathan McDowell]

On Sat, Jun 06, 2020 at 02:43:56PM +0100, Russell King - ARM Linux admin wrote:
> On Sat, Jun 06, 2020 at 11:59:09AM +0100, Jonathan McDowell wrote:
> > So the device in question is a 7 port stand alone switch chip. There's a
> > single SGMII port which is configurable between port 0 + 6 (they can
> > also be configure up as RGMII, while the remaining 5 ports have their
> > own phys).
> > 
> > It sounds like there's a strong preference to try and auto configure
> > things as much as possible, so I should assume the CPU port is in MAC
> > mode, and anything not tagged as a CPU port is talking to a PHY/BASEX.
> > 
> > I assume I can use PHY_INTERFACE_MODE_1000BASEX on the
> > phylink_mac_config call to choose BASEX?
> 
> Yes, but from what you've mentioned above, I think I need to ensure that
> there's a proper understanding here.
> 
> 1000BASE-X is the IEEE 802.3 defined 1G single lane Serdes protocol.
> SGMII is different; it's a vendor derivative of 1000BASE-X which has
> become a de-facto standard.
> 
> Both are somewhat compatible with each other; SGMII brings with it
> additional data replication to achieve 100M and 10M speeds, while
> keeping the link running at 1.25Gbaud.  In both cases, there is a
> 16-bit "configuration" word that is passed between the partners.
> 
> 1000BASE-X uses this configuration word to advertise the abilities of
> each end, which is limited to duplex and pause modes only.  This you
> get by specifying the phy-mode="1000base-x" and
> managed="in-band-status" in DT.
> 
> SGMII uses this configuration word for the media side to inform the
> system side which mode it wishes to operate the link: the speed and
> duplex.  Some vendors extend it to include EEE parameters as well,
> or pause modes.  You get this via phy-mode="sgmii" and
> managed="in-band-status" in DT.
> 
> Then there are variants where the configuration word is not present.
> In this case, the link has to be manually configured, and without the
> configuration word, SGMII operating at 1G is compatible with
> 1000base-X operating at 1G.  Fixed-link can be used for this, although
> fixed-link will always report that the link is up at the moment; that
> may change in the future, it's something that is being looked into at
> the moment.

The hardware I'm using has the switch connected to the CPU via the SGMII
link, and all instances I can find completely disable inband
configuration for that case. However the data sheet has an SGMII control
register which allows configuration of the various auto-negotiation
parameters (as well as whether we're base-x or sgmii) so I think the
full flexibility is there.

I've got an initial port over to using phylink and picking up the
parameters that way (avoiding any device tree option changes) that seems
to be working, but I'll do a bit more testing before sending out a v2
RFC.

J.

-- 
/-\                             | There are always at least two ways
|@/  Debian GNU/Linux Developer |     to program the same thing.
\-                              |

Re: [PATCH 2/2] net: dsa: qca8k: introduce SGMII configuration options

[Andrew Lunn]

> > > > Also, netdev is closed at the moment, so please post patches as RFC.
> > > 
> > > "closed"? If you mean this won't get into 5.8 then I wasn't expecting it
> > > to, I'm aware the merge window for that is already open.
> > 
> > See https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt
> > "How often do changes from these trees make it to the mainline Linus
> > tree?"
> 
> Ta. I'll hold off on a v2 until after -rc1 drops.

You can post at the moment, but you need to put RFC in the subject
line, just to make it clear you are only interested in comments.

      Andrew

[RFC PATCH v2] net: dsa: qca8k: Improve SGMII interface handling

[Jonathan McDowell]

On Sat, Jun 06, 2020 at 11:59:09AM +0100, Jonathan McDowell wrote:
> I'll go away and roll a v2 moving qca8k over to phylink and then using
> that to auto select the appropriate SGMII mode. Thanks for the feedback.

Ok, take 2. I've switched the driver over to phylink which has let me
drop the need for any device tree configuration; if we're a CPU port
then we're in SGMII-PHY mode, otherwise we choose between SGMII-MAC +
Base-X on what phylink tells us.

----

This patch improves the handling of the SGMII interface on the QCA8K
devices. Previously the driver did no configuration of the port, even if
it was selected. We now configure it up in the appropriate
PHY/MAC/Base-X mode depending on what phylink tells us we are connected
to and ensure it is enabled.

Tested with a device where the CPU connection is RGMII (i.e. the common
current use case) + one where the CPU connection is SGMII. I don't have
any devices where the SGMII interface is brought out to something other
than the CPU.

v2:
- Switch to phylink
- Avoid need for device tree configuration options

Signed-off-by: Jonathan McDowell <noodles@earth.li>

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index d2b5ab403e06..62f609ea0e49 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -14,6 +14,7 @@
 #include <linux/of_platform.h>
 #include <linux/if_bridge.h>
 #include <linux/mdio.h>
+#include <linux/phylink.h>
 #include <linux/gpio/consumer.h>
 #include <linux/etherdevice.h>
 
@@ -418,55 +419,6 @@ qca8k_mib_init(struct qca8k_priv *priv)
 	mutex_unlock(&priv->reg_mutex);
 }
 
-static int
-qca8k_set_pad_ctrl(struct qca8k_priv *priv, int port, int mode)
-{
-	u32 reg, val;
-
-	switch (port) {
-	case 0:
-		reg = QCA8K_REG_PORT0_PAD_CTRL;
-		break;
-	case 6:
-		reg = QCA8K_REG_PORT6_PAD_CTRL;
-		break;
-	default:
-		pr_err("Can't set PAD_CTRL on port %d\n", port);
-		return -EINVAL;
-	}
-
-	/* Configure a port to be directly connected to an external
-	 * PHY or MAC.
-	 */
-	switch (mode) {
-	case PHY_INTERFACE_MODE_RGMII:
-		/* RGMII mode means no delay so don't enable the delay */
-		val = QCA8K_PORT_PAD_RGMII_EN;
-		qca8k_write(priv, reg, val);
-		break;
-	case PHY_INTERFACE_MODE_RGMII_ID:
-		/* RGMII_ID needs internal delay. This is enabled through
-		 * PORT5_PAD_CTRL for all ports, rather than individual port
-		 * registers
-		 */
-		qca8k_write(priv, reg,
-			    QCA8K_PORT_PAD_RGMII_EN |
-			    QCA8K_PORT_PAD_RGMII_TX_DELAY(QCA8K_MAX_DELAY) |
-			    QCA8K_PORT_PAD_RGMII_RX_DELAY(QCA8K_MAX_DELAY));
-		qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL,
-			    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
-		break;
-	case PHY_INTERFACE_MODE_SGMII:
-		qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
-		break;
-	default:
-		pr_err("xMII mode %d not supported\n", mode);
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 static void
 qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable)
 {
@@ -639,9 +591,7 @@ static int
 qca8k_setup(struct dsa_switch *ds)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
-	phy_interface_t phy_mode = PHY_INTERFACE_MODE_NA;
 	int ret, i;
-	u32 mask;
 
 	/* Make sure that port 0 is the cpu port */
 	if (!dsa_is_cpu_port(ds, 0)) {
@@ -661,24 +611,9 @@ qca8k_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
-	/* Initialize CPU port pad mode (xMII type, delays...) */
-	ret = of_get_phy_mode(dsa_to_port(ds, QCA8K_CPU_PORT)->dn, &phy_mode);
-	if (ret) {
-		pr_err("Can't find phy-mode for master device\n");
-		return ret;
-	}
-	ret = qca8k_set_pad_ctrl(priv, QCA8K_CPU_PORT, phy_mode);
-	if (ret < 0)
-		return ret;
-
-	/* Enable CPU Port, force it to maximum bandwidth and full-duplex */
-	mask = QCA8K_PORT_STATUS_SPEED_1000 | QCA8K_PORT_STATUS_TXFLOW |
-	       QCA8K_PORT_STATUS_RXFLOW | QCA8K_PORT_STATUS_DUPLEX;
-	qca8k_write(priv, QCA8K_REG_PORT_STATUS(QCA8K_CPU_PORT), mask);
+	/* Enable CPU Port */
 	qca8k_reg_set(priv, QCA8K_REG_GLOBAL_FW_CTRL0,
 		      QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN);
-	qca8k_port_set_status(priv, QCA8K_CPU_PORT, 1);
-	priv->port_sts[QCA8K_CPU_PORT].enabled = 1;
 
 	/* Enable MIB counters */
 	qca8k_mib_init(priv);
@@ -693,10 +628,9 @@ qca8k_setup(struct dsa_switch *ds)
 		qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
 			  QCA8K_PORT_LOOKUP_MEMBER, 0);
 
-	/* Disable MAC by default on all user ports */
+	/* Disable MAC by default on all ports */
 	for (i = 1; i < QCA8K_NUM_PORTS; i++)
-		if (dsa_is_user_port(ds, i))
-			qca8k_port_set_status(priv, i, 0);
+		qca8k_port_set_status(priv, i, 0);
 
 	/* Forward all unknown frames to CPU port for Linux processing */
 	qca8k_write(priv, QCA8K_REG_GLOBAL_FW_CTRL1,
@@ -713,7 +647,7 @@ qca8k_setup(struct dsa_switch *ds)
 				  QCA8K_PORT_LOOKUP_MEMBER, dsa_user_ports(ds));
 		}
 
-		/* Invividual user ports get connected to CPU port only */
+		/* Individual user ports get connected to CPU port only */
 		if (dsa_is_user_port(ds, i)) {
 			int shift = 16 * (i % 2);
 
@@ -743,44 +677,252 @@ qca8k_setup(struct dsa_switch *ds)
 }
 
 static void
-qca8k_adjust_link(struct dsa_switch *ds, int port, struct phy_device *phy)
+qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
+			 const struct phylink_link_state *state)
 {
 	struct qca8k_priv *priv = ds->priv;
-	u32 reg;
+	u32 reg, val;
 
-	/* Force fixed-link setting for CPU port, skip others. */
-	if (!phy_is_pseudo_fixed_link(phy))
+	switch (port) {
+	case 0: /* 1st CPU port */
+		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
+		    state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
+		    state->interface != PHY_INTERFACE_MODE_SGMII)
+			return;
+
+		reg = QCA8K_REG_PORT0_PAD_CTRL;
+		break;
+	case 1:
+	case 2:
+	case 3:
+	case 4:
+	case 5:
+		/* Internal PHY, nothing to do */
 		return;
+	case 6: /* 2nd CPU port / external PHY */
+		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
+		    state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
+		    state->interface != PHY_INTERFACE_MODE_SGMII &&
+		    state->interface != PHY_INTERFACE_MODE_1000BASEX)
+			return;
 
-	/* Set port speed */
-	switch (phy->speed) {
-	case 10:
-		reg = QCA8K_PORT_STATUS_SPEED_10;
+		reg = QCA8K_REG_PORT6_PAD_CTRL;
 		break;
-	case 100:
-		reg = QCA8K_PORT_STATUS_SPEED_100;
+	default:
+		dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
+		return;
+	}
+
+	if (port != 6 && phylink_autoneg_inband(mode)) {
+		dev_err(ds->dev, "%s: in-band negotiation unsupported\n",
+			__func__);
+		return;
+	}
+
+	switch (state->interface) {
+	case PHY_INTERFACE_MODE_RGMII:
+		/* RGMII mode means no delay so don't enable the delay */
+		val = QCA8K_PORT_PAD_RGMII_EN;
+		qca8k_write(priv, reg, val);
 		break;
-	case 1000:
-		reg = QCA8K_PORT_STATUS_SPEED_1000;
+	case PHY_INTERFACE_MODE_RGMII_ID:
+		/* RGMII_ID needs internal delay. This is enabled through
+		 * PORT5_PAD_CTRL for all ports, rather than individual port
+		 * registers
+		 */
+		qca8k_write(priv, reg,
+			    QCA8K_PORT_PAD_RGMII_EN |
+			    QCA8K_PORT_PAD_RGMII_TX_DELAY(QCA8K_MAX_DELAY) |
+			    QCA8K_PORT_PAD_RGMII_RX_DELAY(QCA8K_MAX_DELAY));
+		qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL,
+			    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
+		break;
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_1000BASEX:
+		/* Enable SGMII on the port */
+		qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
+
+		/* Enable/disable SerDes auto-negotiation as necessary */
+		val = qca8k_read(priv, QCA8K_REG_PWS);
+		if (phylink_autoneg_inband(mode))
+			val &= ~QCA8K_PWS_SERDES_AEN_DIS;
+		else
+			val |= QCA8K_PWS_SERDES_AEN_DIS;
+		qca8k_write(priv, QCA8K_REG_PWS, val);
+
+		/* Configure the SGMII parameters */
+		val = qca8k_read(priv, QCA8K_REG_SGMII_CTRL);
+
+		val |= QCA8K_SGMII_EN_PLL | QCA8K_SGMII_EN_RX |
+			QCA8K_SGMII_EN_TX | QCA8K_SGMII_EN_SD;
+
+		val &= ~QCA8K_SGMII_MODE_CTRL_MASK;
+		if (dsa_is_cpu_port(ds, port)) {
+			/* CPU port, we're talking to the CPU MAC, be a PHY */
+			val |= QCA8K_SGMII_MODE_CTRL_PHY;
+		} else if (state->interface == PHY_INTERFACE_MODE_SGMII) {
+			val |= QCA8K_SGMII_MODE_CTRL_MAC;
+		} else {
+			val |= QCA8K_SGMII_MODE_CTRL_BASEX;
+		}
+
+		qca8k_write(priv, QCA8K_REG_SGMII_CTRL, val);
 		break;
 	default:
-		dev_dbg(priv->dev, "port%d link speed %dMbps not supported.\n",
-			port, phy->speed);
+		dev_err(ds->dev, "xMII mode %s not supported for port %d\n",
+			phy_modes(state->interface), port);
 		return;
 	}
+}
+
+static void
+qca8k_phylink_validate(struct dsa_switch *ds, int port,
+		       unsigned long *supported,
+		       struct phylink_link_state *state)
+{
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
+
+	switch (port) {
+	case 0: /* 1st CPU port */
+		if (state->interface != PHY_INTERFACE_MODE_NA &&
+		    state->interface != PHY_INTERFACE_MODE_RGMII &&
+		    state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
+		    state->interface != PHY_INTERFACE_MODE_SGMII)
+			goto unsupported;
+		break;
+	case 1:
+	case 2:
+	case 3:
+	case 4:
+	case 5:
+		/* Internal PHY */
+		if (state->interface != PHY_INTERFACE_MODE_NA &&
+		    state->interface != PHY_INTERFACE_MODE_GMII)
+			goto unsupported;
+		break;
+	case 6: /* 2nd CPU port / external PHY */
+		if (state->interface != PHY_INTERFACE_MODE_NA &&
+		    state->interface != PHY_INTERFACE_MODE_RGMII &&
+		    state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
+		    state->interface != PHY_INTERFACE_MODE_SGMII &&
+		    state->interface != PHY_INTERFACE_MODE_1000BASEX)
+			goto unsupported;
+		break;
+	default:
+		dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
+unsupported:
+		linkmode_zero(supported);
+		return;
+	}
+
+	phylink_set_port_modes(mask);
+	phylink_set(mask, Autoneg);
+
+	phylink_set(mask, 1000baseT_Full);
+	phylink_set(mask, 10baseT_Half);
+	phylink_set(mask, 10baseT_Full);
+	phylink_set(mask, 100baseT_Half);
+	phylink_set(mask, 100baseT_Full);
+
+	if (state->interface == PHY_INTERFACE_MODE_1000BASEX)
+		phylink_set(mask, 1000baseX_Full);
+
+	phylink_set(mask, Pause);
+	phylink_set(mask, Asym_Pause);
+
+	linkmode_and(supported, supported, mask);
+	linkmode_and(state->advertising, state->advertising, mask);
+}
+
+static int
+qca8k_phylink_mac_link_state(struct dsa_switch *ds, int port,
+			     struct phylink_link_state *state)
+{
+	struct qca8k_priv *priv = ds->priv;
+	u32 reg;
+
+	reg = qca8k_read(priv, QCA8K_REG_PORT_STATUS(port));
+
+	state->link = (reg & QCA8K_PORT_STATUS_LINK_UP);
+	state->an_complete = state->link;
+	state->an_enabled = (reg & QCA8K_PORT_STATUS_LINK_AUTO);
+	state->duplex = (reg & QCA8K_PORT_STATUS_DUPLEX) ? DUPLEX_FULL :
+							   DUPLEX_HALF;
+
+	switch (reg & QCA8K_PORT_STATUS_SPEED) {
+	case QCA8K_PORT_STATUS_SPEED_10:
+		state->speed = SPEED_10;
+		break;
+	case QCA8K_PORT_STATUS_SPEED_100:
+		state->speed = SPEED_100;
+		break;
+	case QCA8K_PORT_STATUS_SPEED_1000:
+		state->speed = SPEED_1000;
+		break;
+	default:
+		state->speed = SPEED_UNKNOWN;
+		break;
+	}
 
-	/* Set duplex mode */
-	if (phy->duplex == DUPLEX_FULL)
-		reg |= QCA8K_PORT_STATUS_DUPLEX;
+	state->pause = MLO_PAUSE_NONE;
+	if (reg & QCA8K_PORT_STATUS_RXFLOW)
+		state->pause |= MLO_PAUSE_RX;
+	if (reg & QCA8K_PORT_STATUS_TXFLOW)
+		state->pause |= MLO_PAUSE_TX;
+	if (reg & QCA8K_PORT_STATUS_FLOW_AUTO)
+		state->pause |= MLO_PAUSE_AN;
 
-	/* Force flow control */
-	if (dsa_is_cpu_port(ds, port))
-		reg |= QCA8K_PORT_STATUS_RXFLOW | QCA8K_PORT_STATUS_TXFLOW;
+	return 1;
+}
+
+static void
+qca8k_phylink_mac_link_down(struct dsa_switch *ds, int port, unsigned int mode,
+			    phy_interface_t interface)
+{
+	struct qca8k_priv *priv = ds->priv;
 
-	/* Force link down before changing MAC options */
 	qca8k_port_set_status(priv, port, 0);
+}
+
+static void
+qca8k_phylink_mac_link_up(struct dsa_switch *ds, int port, unsigned int mode,
+			  phy_interface_t interface, struct phy_device *phydev,
+			  int speed, int duplex, bool tx_pause, bool rx_pause)
+{
+	struct qca8k_priv *priv = ds->priv;
+	u32 reg;
+
+	if (phylink_autoneg_inband(mode)) {
+		reg = QCA8K_PORT_STATUS_LINK_AUTO;
+	} else {
+		switch (speed) {
+		case SPEED_10:
+			reg = QCA8K_PORT_STATUS_SPEED_10;
+			break;
+		case SPEED_100:
+			reg = QCA8K_PORT_STATUS_SPEED_100;
+			break;
+		case SPEED_1000:
+			reg = QCA8K_PORT_STATUS_SPEED_1000;
+			break;
+		default:
+			reg = QCA8K_PORT_STATUS_LINK_AUTO;
+			break;
+		}
+
+		if (duplex == DUPLEX_FULL)
+			reg |= QCA8K_PORT_STATUS_DUPLEX;
+
+		if (rx_pause | dsa_is_cpu_port(ds, port))
+			reg |= QCA8K_PORT_STATUS_RXFLOW;
+
+		if (tx_pause | dsa_is_cpu_port(ds, port))
+			reg |= QCA8K_PORT_STATUS_TXFLOW;
+	}
+
+	reg |= QCA8K_PORT_STATUS_TXMAC | QCA8K_PORT_STATUS_RXMAC;
+
 	qca8k_write(priv, QCA8K_REG_PORT_STATUS(port), reg);
-	qca8k_port_set_status(priv, port, 1);
 }
 
 static void
@@ -937,13 +1079,11 @@ qca8k_port_enable(struct dsa_switch *ds, int port,
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 
-	if (!dsa_is_user_port(ds, port))
-		return 0;
-
 	qca8k_port_set_status(priv, port, 1);
 	priv->port_sts[port].enabled = 1;
 
-	phy_support_asym_pause(phy);
+	if (dsa_is_user_port(ds, port))
+		phy_support_asym_pause(phy);
 
 	return 0;
 }
@@ -1026,7 +1166,6 @@ qca8k_get_tag_protocol(struct dsa_switch *ds, int port,
 static const struct dsa_switch_ops qca8k_switch_ops = {
 	.get_tag_protocol	= qca8k_get_tag_protocol,
 	.setup			= qca8k_setup,
-	.adjust_link            = qca8k_adjust_link,
 	.get_strings		= qca8k_get_strings,
 	.get_ethtool_stats	= qca8k_get_ethtool_stats,
 	.get_sset_count		= qca8k_get_sset_count,
@@ -1040,6 +1179,11 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
 	.port_fdb_add		= qca8k_port_fdb_add,
 	.port_fdb_del		= qca8k_port_fdb_del,
 	.port_fdb_dump		= qca8k_port_fdb_dump,
+	.phylink_validate	= qca8k_phylink_validate,
+	.phylink_mac_link_state	= qca8k_phylink_mac_link_state,
+	.phylink_mac_config	= qca8k_phylink_mac_config,
+	.phylink_mac_link_down	= qca8k_phylink_mac_link_down,
+	.phylink_mac_link_up	= qca8k_phylink_mac_link_up,
 };
 
 static int
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 42d6ea24eb14..10ef2bca2cde 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -36,6 +36,8 @@
 #define   QCA8K_MAX_DELAY				3
 #define   QCA8K_PORT_PAD_RGMII_RX_DELAY_EN		BIT(24)
 #define   QCA8K_PORT_PAD_SGMII_EN			BIT(7)
+#define QCA8K_REG_PWS					0x010
+#define   QCA8K_PWS_SERDES_AEN_DIS			BIT(7)
 #define QCA8K_REG_MODULE_EN				0x030
 #define   QCA8K_MODULE_EN_MIB				BIT(0)
 #define QCA8K_REG_MIB					0x034
@@ -69,6 +71,7 @@
 #define   QCA8K_PORT_STATUS_LINK_UP			BIT(8)
 #define   QCA8K_PORT_STATUS_LINK_AUTO			BIT(9)
 #define   QCA8K_PORT_STATUS_LINK_PAUSE			BIT(10)
+#define   QCA8K_PORT_STATUS_FLOW_AUTO			BIT(12)
 #define QCA8K_REG_PORT_HDR_CTRL(_i)			(0x9c + (_i * 4))
 #define   QCA8K_PORT_HDR_CTRL_RX_MASK			GENMASK(3, 2)
 #define   QCA8K_PORT_HDR_CTRL_RX_S			2
@@ -77,6 +80,16 @@
 #define   QCA8K_PORT_HDR_CTRL_ALL			2
 #define   QCA8K_PORT_HDR_CTRL_MGMT			1
 #define   QCA8K_PORT_HDR_CTRL_NONE			0
+#define QCA8K_REG_SGMII_CTRL				0x0e0
+#define   QCA8K_SGMII_EN_PLL				BIT(1)
+#define   QCA8K_SGMII_EN_RX				BIT(2)
+#define   QCA8K_SGMII_EN_TX				BIT(3)
+#define   QCA8K_SGMII_EN_SD				BIT(4)
+#define   QCA8K_SGMII_CLK125M_DELAY			BIT(7)
+#define   QCA8K_SGMII_MODE_CTRL_MASK			(BIT(22) | BIT(23))
+#define   QCA8K_SGMII_MODE_CTRL_BASEX			(0 << 22)
+#define   QCA8K_SGMII_MODE_CTRL_PHY			(1 << 22)
+#define   QCA8K_SGMII_MODE_CTRL_MAC			(2 << 22)
 
 /* EEE control registers */
 #define QCA8K_REG_EEE_CTRL				0x100

Re: [RFC PATCH v2] net: dsa: qca8k: Improve SGMII interface handling

[Andrew Lunn]

On Mon, Jun 08, 2020 at 07:39:53PM +0100, Jonathan McDowell wrote:
> On Sat, Jun 06, 2020 at 11:59:09AM +0100, Jonathan McDowell wrote:
> > I'll go away and roll a v2 moving qca8k over to phylink and then using
> > that to auto select the appropriate SGMII mode. Thanks for the feedback.
> 
> Ok, take 2. I've switched the driver over to phylink which has let me
> drop the need for any device tree configuration; if we're a CPU port
> then we're in SGMII-PHY mode, otherwise we choose between SGMII-MAC +
> Base-X on what phylink tells us.

Hi Johnathan

Could you split this into two parts. One or more patches which
converts to phylink, and a patch which adds new functionality. If it
breaks, we then have a better idea what broke it.

Conversions to phylink are not easy to review because a lot of code
gets moved around. It sometimes helps if you can turn existing code
into helpers, rather than inline them.

     Andrew

Re: [RFC PATCH v2] net: dsa: qca8k: Improve SGMII interface handling

[Russell King - ARM Linux admin]

Hi Jonathan,

A quick read through on the first review...

On Mon, Jun 08, 2020 at 07:39:53PM +0100, Jonathan McDowell wrote:
> +static int
> +qca8k_phylink_mac_link_state(struct dsa_switch *ds, int port,
> +			     struct phylink_link_state *state)
> +{
> +	struct qca8k_priv *priv = ds->priv;
> +	u32 reg;
> +
> +	reg = qca8k_read(priv, QCA8K_REG_PORT_STATUS(port));
> +
> +	state->link = (reg & QCA8K_PORT_STATUS_LINK_UP);
> +	state->an_complete = state->link;
> +	state->an_enabled = (reg & QCA8K_PORT_STATUS_LINK_AUTO);

I much prefer to use !!(reg & ...) since these are single-bit
bitfields.

> +	state->duplex = (reg & QCA8K_PORT_STATUS_DUPLEX) ? DUPLEX_FULL :
> +							   DUPLEX_HALF;
> +
> +	switch (reg & QCA8K_PORT_STATUS_SPEED) {
> +	case QCA8K_PORT_STATUS_SPEED_10:
> +		state->speed = SPEED_10;
> +		break;
> +	case QCA8K_PORT_STATUS_SPEED_100:
> +		state->speed = SPEED_100;
> +		break;
> +	case QCA8K_PORT_STATUS_SPEED_1000:
> +		state->speed = SPEED_1000;
> +		break;
> +	default:
> +		state->speed = SPEED_UNKNOWN;
> +		break;
> +	}
>  
> -	/* Set duplex mode */
> -	if (phy->duplex == DUPLEX_FULL)
> -		reg |= QCA8K_PORT_STATUS_DUPLEX;
> +	state->pause = MLO_PAUSE_NONE;
> +	if (reg & QCA8K_PORT_STATUS_RXFLOW)
> +		state->pause |= MLO_PAUSE_RX;
> +	if (reg & QCA8K_PORT_STATUS_TXFLOW)
> +		state->pause |= MLO_PAUSE_TX;
> +	if (reg & QCA8K_PORT_STATUS_FLOW_AUTO)
> +		state->pause |= MLO_PAUSE_AN;

There is no need to report back MLO_PAUSE_AN.

From the quick read of this, nothing else obviously stood out.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 503kbps up

[RFC PATCH v3 0/2] net: dsa: qca8k: Improve SGMII interface handling

[Jonathan McDowell]

Ok, take 3. This splits out the PHYLINK change to a separate patch which
should have no effect on functionality, and then adds the SGMII clean-ups
(i.e. the missing initialisation) on top of that as a second patch.

As before, tested with a device where the CPU connection is RGMII (i.e.
the common current use case) + one where the CPU connection is SGMII. I
don't have any devices where the SGMII interface is brought out to
something other than the CPU.


v3:
- Move phylink changes to separate patch
- Address rmk review comments
v2:
- Switch to phylink
- Avoid need for device tree configuration options

Jonathan McDowell (2):
  net: dsa: qca8k: Switch to PHYLINK instead of PHYLIB
  net: dsa: qca8k: Improve SGMII interface handling

 drivers/net/dsa/qca8k.c | 337 ++++++++++++++++++++++++++++------------
 drivers/net/dsa/qca8k.h |  13 ++
 2 files changed, 252 insertions(+), 98 deletions(-)

-- 
2.20.1

[PATCH 1/2] net: dsa: qca8k: Switch to PHYLINK instead of PHYLIB

[Jonathan McDowell]

Update the driver to use the new PHYLINK callbacks, removing the
legacy adjust_link callback.

Signed-off-by: Jonathan McDowell <noodles@earth.li>
---
 drivers/net/dsa/qca8k.c | 309 +++++++++++++++++++++++++++-------------
 1 file changed, 212 insertions(+), 97 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index d2b5ab403e06..dcd9e8fa99b6 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -14,6 +14,7 @@
 #include <linux/of_platform.h>
 #include <linux/if_bridge.h>
 #include <linux/mdio.h>
+#include <linux/phylink.h>
 #include <linux/gpio/consumer.h>
 #include <linux/etherdevice.h>
 
@@ -418,55 +419,6 @@ qca8k_mib_init(struct qca8k_priv *priv)
 	mutex_unlock(&priv->reg_mutex);
 }
 
-static int
-qca8k_set_pad_ctrl(struct qca8k_priv *priv, int port, int mode)
-{
-	u32 reg, val;
-
-	switch (port) {
-	case 0:
-		reg = QCA8K_REG_PORT0_PAD_CTRL;
-		break;
-	case 6:
-		reg = QCA8K_REG_PORT6_PAD_CTRL;
-		break;
-	default:
-		pr_err("Can't set PAD_CTRL on port %d\n", port);
-		return -EINVAL;
-	}
-
-	/* Configure a port to be directly connected to an external
-	 * PHY or MAC.
-	 */
-	switch (mode) {
-	case PHY_INTERFACE_MODE_RGMII:
-		/* RGMII mode means no delay so don't enable the delay */
-		val = QCA8K_PORT_PAD_RGMII_EN;
-		qca8k_write(priv, reg, val);
-		break;
-	case PHY_INTERFACE_MODE_RGMII_ID:
-		/* RGMII_ID needs internal delay. This is enabled through
-		 * PORT5_PAD_CTRL for all ports, rather than individual port
-		 * registers
-		 */
-		qca8k_write(priv, reg,
-			    QCA8K_PORT_PAD_RGMII_EN |
-			    QCA8K_PORT_PAD_RGMII_TX_DELAY(QCA8K_MAX_DELAY) |
-			    QCA8K_PORT_PAD_RGMII_RX_DELAY(QCA8K_MAX_DELAY));
-		qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL,
-			    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
-		break;
-	case PHY_INTERFACE_MODE_SGMII:
-		qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
-		break;
-	default:
-		pr_err("xMII mode %d not supported\n", mode);
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 static void
 qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable)
 {
@@ -639,9 +591,7 @@ static int
 qca8k_setup(struct dsa_switch *ds)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
-	phy_interface_t phy_mode = PHY_INTERFACE_MODE_NA;
 	int ret, i;
-	u32 mask;
 
 	/* Make sure that port 0 is the cpu port */
 	if (!dsa_is_cpu_port(ds, 0)) {
@@ -661,24 +611,9 @@ qca8k_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
-	/* Initialize CPU port pad mode (xMII type, delays...) */
-	ret = of_get_phy_mode(dsa_to_port(ds, QCA8K_CPU_PORT)->dn, &phy_mode);
-	if (ret) {
-		pr_err("Can't find phy-mode for master device\n");
-		return ret;
-	}
-	ret = qca8k_set_pad_ctrl(priv, QCA8K_CPU_PORT, phy_mode);
-	if (ret < 0)
-		return ret;
-
-	/* Enable CPU Port, force it to maximum bandwidth and full-duplex */
-	mask = QCA8K_PORT_STATUS_SPEED_1000 | QCA8K_PORT_STATUS_TXFLOW |
-	       QCA8K_PORT_STATUS_RXFLOW | QCA8K_PORT_STATUS_DUPLEX;
-	qca8k_write(priv, QCA8K_REG_PORT_STATUS(QCA8K_CPU_PORT), mask);
+	/* Enable CPU Port */
 	qca8k_reg_set(priv, QCA8K_REG_GLOBAL_FW_CTRL0,
 		      QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN);
-	qca8k_port_set_status(priv, QCA8K_CPU_PORT, 1);
-	priv->port_sts[QCA8K_CPU_PORT].enabled = 1;
 
 	/* Enable MIB counters */
 	qca8k_mib_init(priv);
@@ -693,10 +628,9 @@ qca8k_setup(struct dsa_switch *ds)
 		qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
 			  QCA8K_PORT_LOOKUP_MEMBER, 0);
 
-	/* Disable MAC by default on all user ports */
+	/* Disable MAC by default on all ports */
 	for (i = 1; i < QCA8K_NUM_PORTS; i++)
-		if (dsa_is_user_port(ds, i))
-			qca8k_port_set_status(priv, i, 0);
+		qca8k_port_set_status(priv, i, 0);
 
 	/* Forward all unknown frames to CPU port for Linux processing */
 	qca8k_write(priv, QCA8K_REG_GLOBAL_FW_CTRL1,
@@ -713,7 +647,7 @@ qca8k_setup(struct dsa_switch *ds)
 				  QCA8K_PORT_LOOKUP_MEMBER, dsa_user_ports(ds));
 		}
 
-		/* Invividual user ports get connected to CPU port only */
+		/* Individual user ports get connected to CPU port only */
 		if (dsa_is_user_port(ds, i)) {
 			int shift = 16 * (i % 2);
 
@@ -743,44 +677,223 @@ qca8k_setup(struct dsa_switch *ds)
 }
 
 static void
-qca8k_adjust_link(struct dsa_switch *ds, int port, struct phy_device *phy)
+qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
+			 const struct phylink_link_state *state)
 {
 	struct qca8k_priv *priv = ds->priv;
 	u32 reg;
 
-	/* Force fixed-link setting for CPU port, skip others. */
-	if (!phy_is_pseudo_fixed_link(phy))
+	switch (port) {
+	case 0: /* 1st CPU port */
+		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
+		    state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
+		    state->interface != PHY_INTERFACE_MODE_SGMII)
+			return;
+
+		reg = QCA8K_REG_PORT0_PAD_CTRL;
+		break;
+	case 1:
+	case 2:
+	case 3:
+	case 4:
+	case 5:
+		/* Internal PHY, nothing to do */
+		return;
+	case 6: /* 2nd CPU port / external PHY */
+		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
+		    state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
+		    state->interface != PHY_INTERFACE_MODE_SGMII &&
+		    state->interface != PHY_INTERFACE_MODE_1000BASEX)
+			return;
+
+		reg = QCA8K_REG_PORT6_PAD_CTRL;
+		break;
+	default:
+		dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
+		return;
+	}
+
+	if (port != 6 && phylink_autoneg_inband(mode)) {
+		dev_err(ds->dev, "%s: in-band negotiation unsupported\n",
+			__func__);
+		return;
+	}
+
+	switch (state->interface) {
+	case PHY_INTERFACE_MODE_RGMII:
+		/* RGMII mode means no delay so don't enable the delay */
+		qca8k_write(priv, reg, QCA8K_PORT_PAD_RGMII_EN);
+		break;
+	case PHY_INTERFACE_MODE_RGMII_ID:
+		/* RGMII_ID needs internal delay. This is enabled through
+		 * PORT5_PAD_CTRL for all ports, rather than individual port
+		 * registers
+		 */
+		qca8k_write(priv, reg,
+			    QCA8K_PORT_PAD_RGMII_EN |
+			    QCA8K_PORT_PAD_RGMII_TX_DELAY(QCA8K_MAX_DELAY) |
+			    QCA8K_PORT_PAD_RGMII_RX_DELAY(QCA8K_MAX_DELAY));
+		qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL,
+			    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
+		break;
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_1000BASEX:
+		/* Enable SGMII on the port */
+		qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
+		break;
+	default:
+		dev_err(ds->dev, "xMII mode %s not supported for port %d\n",
+			phy_modes(state->interface), port);
 		return;
+	}
+}
 
-	/* Set port speed */
-	switch (phy->speed) {
-	case 10:
-		reg = QCA8K_PORT_STATUS_SPEED_10;
+static void
+qca8k_phylink_validate(struct dsa_switch *ds, int port,
+		       unsigned long *supported,
+		       struct phylink_link_state *state)
+{
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
+
+	switch (port) {
+	case 0: /* 1st CPU port */
+		if (state->interface != PHY_INTERFACE_MODE_NA &&
+		    state->interface != PHY_INTERFACE_MODE_RGMII &&
+		    state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
+		    state->interface != PHY_INTERFACE_MODE_SGMII)
+			goto unsupported;
 		break;
-	case 100:
-		reg = QCA8K_PORT_STATUS_SPEED_100;
+	case 1:
+	case 2:
+	case 3:
+	case 4:
+	case 5:
+		/* Internal PHY */
+		if (state->interface != PHY_INTERFACE_MODE_NA &&
+		    state->interface != PHY_INTERFACE_MODE_GMII)
+			goto unsupported;
 		break;
-	case 1000:
-		reg = QCA8K_PORT_STATUS_SPEED_1000;
+	case 6: /* 2nd CPU port / external PHY */
+		if (state->interface != PHY_INTERFACE_MODE_NA &&
+		    state->interface != PHY_INTERFACE_MODE_RGMII &&
+		    state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
+		    state->interface != PHY_INTERFACE_MODE_SGMII &&
+		    state->interface != PHY_INTERFACE_MODE_1000BASEX)
+			goto unsupported;
 		break;
 	default:
-		dev_dbg(priv->dev, "port%d link speed %dMbps not supported.\n",
-			port, phy->speed);
+		dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
+unsupported:
+		linkmode_zero(supported);
 		return;
 	}
 
-	/* Set duplex mode */
-	if (phy->duplex == DUPLEX_FULL)
-		reg |= QCA8K_PORT_STATUS_DUPLEX;
+	phylink_set_port_modes(mask);
+	phylink_set(mask, Autoneg);
+
+	phylink_set(mask, 1000baseT_Full);
+	phylink_set(mask, 10baseT_Half);
+	phylink_set(mask, 10baseT_Full);
+	phylink_set(mask, 100baseT_Half);
+	phylink_set(mask, 100baseT_Full);
+
+	if (state->interface == PHY_INTERFACE_MODE_1000BASEX)
+		phylink_set(mask, 1000baseX_Full);
+
+	phylink_set(mask, Pause);
+	phylink_set(mask, Asym_Pause);
+
+	linkmode_and(supported, supported, mask);
+	linkmode_and(state->advertising, state->advertising, mask);
+}
+
+static int
+qca8k_phylink_mac_link_state(struct dsa_switch *ds, int port,
+			     struct phylink_link_state *state)
+{
+	struct qca8k_priv *priv = ds->priv;
+	u32 reg;
 
-	/* Force flow control */
-	if (dsa_is_cpu_port(ds, port))
-		reg |= QCA8K_PORT_STATUS_RXFLOW | QCA8K_PORT_STATUS_TXFLOW;
+	reg = qca8k_read(priv, QCA8K_REG_PORT_STATUS(port));
+
+	state->link = !!(reg & QCA8K_PORT_STATUS_LINK_UP);
+	state->an_complete = state->link;
+	state->an_enabled = !!(reg & QCA8K_PORT_STATUS_LINK_AUTO);
+	state->duplex = (reg & QCA8K_PORT_STATUS_DUPLEX) ? DUPLEX_FULL :
+							   DUPLEX_HALF;
+
+	switch (reg & QCA8K_PORT_STATUS_SPEED) {
+	case QCA8K_PORT_STATUS_SPEED_10:
+		state->speed = SPEED_10;
+		break;
+	case QCA8K_PORT_STATUS_SPEED_100:
+		state->speed = SPEED_100;
+		break;
+	case QCA8K_PORT_STATUS_SPEED_1000:
+		state->speed = SPEED_1000;
+		break;
+	default:
+		state->speed = SPEED_UNKNOWN;
+		break;
+	}
+
+	state->pause = MLO_PAUSE_NONE;
+	if (reg & QCA8K_PORT_STATUS_RXFLOW)
+		state->pause |= MLO_PAUSE_RX;
+	if (reg & QCA8K_PORT_STATUS_TXFLOW)
+		state->pause |= MLO_PAUSE_TX;
+
+	return 1;
+}
+
+static void
+qca8k_phylink_mac_link_down(struct dsa_switch *ds, int port, unsigned int mode,
+			    phy_interface_t interface)
+{
+	struct qca8k_priv *priv = ds->priv;
 
-	/* Force link down before changing MAC options */
 	qca8k_port_set_status(priv, port, 0);
+}
+
+static void
+qca8k_phylink_mac_link_up(struct dsa_switch *ds, int port, unsigned int mode,
+			  phy_interface_t interface, struct phy_device *phydev,
+			  int speed, int duplex, bool tx_pause, bool rx_pause)
+{
+	struct qca8k_priv *priv = ds->priv;
+	u32 reg;
+
+	if (phylink_autoneg_inband(mode)) {
+		reg = QCA8K_PORT_STATUS_LINK_AUTO;
+	} else {
+		switch (speed) {
+		case SPEED_10:
+			reg = QCA8K_PORT_STATUS_SPEED_10;
+			break;
+		case SPEED_100:
+			reg = QCA8K_PORT_STATUS_SPEED_100;
+			break;
+		case SPEED_1000:
+			reg = QCA8K_PORT_STATUS_SPEED_1000;
+			break;
+		default:
+			reg = QCA8K_PORT_STATUS_LINK_AUTO;
+			break;
+		}
+
+		if (duplex == DUPLEX_FULL)
+			reg |= QCA8K_PORT_STATUS_DUPLEX;
+
+		if (rx_pause | dsa_is_cpu_port(ds, port))
+			reg |= QCA8K_PORT_STATUS_RXFLOW;
+
+		if (tx_pause | dsa_is_cpu_port(ds, port))
+			reg |= QCA8K_PORT_STATUS_TXFLOW;
+	}
+
+	reg |= QCA8K_PORT_STATUS_TXMAC | QCA8K_PORT_STATUS_RXMAC;
+
 	qca8k_write(priv, QCA8K_REG_PORT_STATUS(port), reg);
-	qca8k_port_set_status(priv, port, 1);
 }
 
 static void
@@ -937,13 +1050,11 @@ qca8k_port_enable(struct dsa_switch *ds, int port,
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 
-	if (!dsa_is_user_port(ds, port))
-		return 0;
-
 	qca8k_port_set_status(priv, port, 1);
 	priv->port_sts[port].enabled = 1;
 
-	phy_support_asym_pause(phy);
+	if (dsa_is_user_port(ds, port))
+		phy_support_asym_pause(phy);
 
 	return 0;
 }
@@ -1026,7 +1137,6 @@ qca8k_get_tag_protocol(struct dsa_switch *ds, int port,
 static const struct dsa_switch_ops qca8k_switch_ops = {
 	.get_tag_protocol	= qca8k_get_tag_protocol,
 	.setup			= qca8k_setup,
-	.adjust_link            = qca8k_adjust_link,
 	.get_strings		= qca8k_get_strings,
 	.get_ethtool_stats	= qca8k_get_ethtool_stats,
 	.get_sset_count		= qca8k_get_sset_count,
@@ -1040,6 +1150,11 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
 	.port_fdb_add		= qca8k_port_fdb_add,
 	.port_fdb_del		= qca8k_port_fdb_del,
 	.port_fdb_dump		= qca8k_port_fdb_dump,
+	.phylink_validate	= qca8k_phylink_validate,
+	.phylink_mac_link_state	= qca8k_phylink_mac_link_state,
+	.phylink_mac_config	= qca8k_phylink_mac_config,
+	.phylink_mac_link_down	= qca8k_phylink_mac_link_down,
+	.phylink_mac_link_up	= qca8k_phylink_mac_link_up,
 };
 
 static int
-- 
2.20.1

Re: [PATCH 1/2] net: dsa: qca8k: Switch to PHYLINK instead of PHYLIB

[Florian Fainelli]

On 6/10/2020 12:14 PM, Jonathan McDowell wrote:
> Update the driver to use the new PHYLINK callbacks, removing the
> legacy adjust_link callback.
> 
> Signed-off-by: Jonathan McDowell <noodles@earth.li>

This looks reasonable to me, Russell would be the person you want to get
an Acked-by/Reviewed-by tag from.
-- 
Florian

Re: [PATCH 1/2] net: dsa: qca8k: Switch to PHYLINK instead of PHYLIB

[Russell King - ARM Linux admin]

On Wed, Jun 10, 2020 at 08:14:03PM +0100, Jonathan McDowell wrote:
> Update the driver to use the new PHYLINK callbacks, removing the
> legacy adjust_link callback.

Looks good, there's a couple of issues / questions

>  static void
> +qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> +			 const struct phylink_link_state *state)
>  {
>  	struct qca8k_priv *priv = ds->priv;
>  	u32 reg;
>  
> +	switch (port) {
...
> +	case 6: /* 2nd CPU port / external PHY */
> +		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
> +		    state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
> +		    state->interface != PHY_INTERFACE_MODE_SGMII &&
> +		    state->interface != PHY_INTERFACE_MODE_1000BASEX)
> +			return;
> +
> +		reg = QCA8K_REG_PORT6_PAD_CTRL;
> +		break;
...
> +	}
> +
> +	if (port != 6 && phylink_autoneg_inband(mode)) {
> +		dev_err(ds->dev, "%s: in-band negotiation unsupported\n",
> +			__func__);
> +		return;
> +	}
> +
> +	switch (state->interface) {
...
> +	case PHY_INTERFACE_MODE_SGMII:
> +	case PHY_INTERFACE_MODE_1000BASEX:
> +		/* Enable SGMII on the port */
> +		qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
> +		break;

Is inband mode configurable?  What if the link partner does/doesn't
send the configuration word?  How is the link state communicated to
the MAC?

> +static int
> +qca8k_phylink_mac_link_state(struct dsa_switch *ds, int port,
> +			     struct phylink_link_state *state)
> +{
> +	struct qca8k_priv *priv = ds->priv;
> +	u32 reg;
>  
> +	reg = qca8k_read(priv, QCA8K_REG_PORT_STATUS(port));
> +
> +	state->link = !!(reg & QCA8K_PORT_STATUS_LINK_UP);
> +	state->an_complete = state->link;
> +	state->an_enabled = !!(reg & QCA8K_PORT_STATUS_LINK_AUTO);
> +	state->duplex = (reg & QCA8K_PORT_STATUS_DUPLEX) ? DUPLEX_FULL :
> +							   DUPLEX_HALF;
> +
> +	switch (reg & QCA8K_PORT_STATUS_SPEED) {
> +	case QCA8K_PORT_STATUS_SPEED_10:
> +		state->speed = SPEED_10;
> +		break;
> +	case QCA8K_PORT_STATUS_SPEED_100:
> +		state->speed = SPEED_100;
> +		break;
> +	case QCA8K_PORT_STATUS_SPEED_1000:
> +		state->speed = SPEED_1000;
> +		break;
> +	default:
> +		state->speed = SPEED_UNKNOWN;

Maybe also force the link down in this case, since the state is invalid?

Do you have access to the link partner's configuration word?  If you do,
you should use that to fill in state->lp_advertising.

> +		break;
> +	}
> +
> +	state->pause = MLO_PAUSE_NONE;
> +	if (reg & QCA8K_PORT_STATUS_RXFLOW)
> +		state->pause |= MLO_PAUSE_RX;
> +	if (reg & QCA8K_PORT_STATUS_TXFLOW)
> +		state->pause |= MLO_PAUSE_TX;
> +
> +	return 1;
> +}
> +
> +static void
> +qca8k_phylink_mac_link_down(struct dsa_switch *ds, int port, unsigned int mode,
> +			    phy_interface_t interface)
> +{
> +	struct qca8k_priv *priv = ds->priv;
>  
>  	qca8k_port_set_status(priv, port, 0);

If operating in in-band mode, forcing the link down unconditionally
will prevent the link coming up if the SGMII/1000base-X block
automatically updates the MAC, and if this takes precedence.

When using in-band mode, you need to call dsa_port_phylink_mac_change()
to keep phylink updated with the link status.

Alternatively, phylink supports polling mode, but due to the layered
way DSA is written, DSA drivers don't have access to that as that is
in the DSA upper levels in net/dsa/slave.c (dsa_slave_phy_setup(),
it would be dp->pl_config.pcs_poll).

Apart from those points, I think it looks fine, thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 503kbps up

Re: [PATCH 1/2] net: dsa: qca8k: Switch to PHYLINK instead of PHYLIB

[Vladimir Oltean]

Hi Russell,

On Thu, 11 Jun 2020 at 11:57, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>

>
> Alternatively, phylink supports polling mode, but due to the layered
> way DSA is written, DSA drivers don't have access to that as that is
> in the DSA upper levels in net/dsa/slave.c (dsa_slave_phy_setup(),
> it would be dp->pl_config.pcs_poll).
>

They do, see https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/ocelot/felix.c#n606

>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 503kbps up

Thanks,
-Vladimir

Re: [PATCH 1/2] net: dsa: qca8k: Switch to PHYLINK instead of PHYLIB

[Jonathan McDowell]

On Thu, Jun 11, 2020 at 09:55:23AM +0100, Russell King - ARM Linux admin wrote:
> On Wed, Jun 10, 2020 at 08:14:03PM +0100, Jonathan McDowell wrote:
> > Update the driver to use the new PHYLINK callbacks, removing the
> > legacy adjust_link callback.
> 
> Looks good, there's a couple of issues / questions
> 
> >  static void
> > +qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> > +			 const struct phylink_link_state *state)
> >  {
> >  	struct qca8k_priv *priv = ds->priv;
> >  	u32 reg;
> >  
> > +	switch (port) {
> ...
> > +	case 6: /* 2nd CPU port / external PHY */
> > +		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
> > +		    state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
> > +		    state->interface != PHY_INTERFACE_MODE_SGMII &&
> > +		    state->interface != PHY_INTERFACE_MODE_1000BASEX)
> > +			return;
> > +
> > +		reg = QCA8K_REG_PORT6_PAD_CTRL;
> > +		break;
> ...
> > +	}
> > +
> > +	if (port != 6 && phylink_autoneg_inband(mode)) {
> > +		dev_err(ds->dev, "%s: in-band negotiation unsupported\n",
> > +			__func__);
> > +		return;
> > +	}
> > +
> > +	switch (state->interface) {
> ...
> > +	case PHY_INTERFACE_MODE_SGMII:
> > +	case PHY_INTERFACE_MODE_1000BASEX:
> > +		/* Enable SGMII on the port */
> > +		qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
> > +		break;
> 
> Is inband mode configurable?  What if the link partner does/doesn't
> send the configuration word?  How is the link state communicated to
> the MAC?

I moved those over to the second patch on the request of Andrew Lunn,
who wanted the phylink change to be separate from the addition of the
SGMII changes. This first patch should result in no change of behaviour,
just the move to phylink.

> > +static int
> > +qca8k_phylink_mac_link_state(struct dsa_switch *ds, int port,
> > +			     struct phylink_link_state *state)
> > +{
> > +	struct qca8k_priv *priv = ds->priv;
> > +	u32 reg;
> >  
> > +	reg = qca8k_read(priv, QCA8K_REG_PORT_STATUS(port));
> > +
> > +	state->link = !!(reg & QCA8K_PORT_STATUS_LINK_UP);
> > +	state->an_complete = state->link;
> > +	state->an_enabled = !!(reg & QCA8K_PORT_STATUS_LINK_AUTO);
> > +	state->duplex = (reg & QCA8K_PORT_STATUS_DUPLEX) ? DUPLEX_FULL :
> > +							   DUPLEX_HALF;
> > +
> > +	switch (reg & QCA8K_PORT_STATUS_SPEED) {
> > +	case QCA8K_PORT_STATUS_SPEED_10:
> > +		state->speed = SPEED_10;
> > +		break;
> > +	case QCA8K_PORT_STATUS_SPEED_100:
> > +		state->speed = SPEED_100;
> > +		break;
> > +	case QCA8K_PORT_STATUS_SPEED_1000:
> > +		state->speed = SPEED_1000;
> > +		break;
> > +	default:
> > +		state->speed = SPEED_UNKNOWN;
> 
> Maybe also force the link down in this case, since the state is invalid?
> 
> Do you have access to the link partner's configuration word?  If you do,
> you should use that to fill in state->lp_advertising.

It doesn't seem to be available from my reading of the data sheet.

> > +		break;
> > +	}
> > +
> > +	state->pause = MLO_PAUSE_NONE;
> > +	if (reg & QCA8K_PORT_STATUS_RXFLOW)
> > +		state->pause |= MLO_PAUSE_RX;
> > +	if (reg & QCA8K_PORT_STATUS_TXFLOW)
> > +		state->pause |= MLO_PAUSE_TX;
> > +
> > +	return 1;
> > +}
> > +
> > +static void
> > +qca8k_phylink_mac_link_down(struct dsa_switch *ds, int port, unsigned int mode,
> > +			    phy_interface_t interface)
> > +{
> > +	struct qca8k_priv *priv = ds->priv;
> >  
> >  	qca8k_port_set_status(priv, port, 0);
> 
> If operating in in-band mode, forcing the link down unconditionally
> will prevent the link coming up if the SGMII/1000base-X block
> automatically updates the MAC, and if this takes precedence.
> 
> When using in-band mode, you need to call dsa_port_phylink_mac_change()
> to keep phylink updated with the link status.
> 
> Alternatively, phylink supports polling mode, but due to the layered
> way DSA is written, DSA drivers don't have access to that as that is
> in the DSA upper levels in net/dsa/slave.c (dsa_slave_phy_setup(),
> it would be dp->pl_config.pcs_poll).

My reading of Vladimir's mail is I can set pcs_poll to get that
functionality, and from the data sheet the link detection is keyed off a
separate bit than the mac link down piece so I think I'm ok there.

> Apart from those points, I think it looks fine, thanks.

Thanks for the review. I'll get a new version out soon.

J.

-- 
If a program is useless, it must be documented.

Re: [PATCH 1/2] net: dsa: qca8k: Switch to PHYLINK instead of PHYLIB

[Vladimir Oltean]

Hi Jonathan,

On Wed, 10 Jun 2020 at 23:19, Jonathan McDowell <noodles@earth.li> wrote:
>
> Update the driver to use the new PHYLINK callbacks, removing the
> legacy adjust_link callback.
>
> Signed-off-by: Jonathan McDowell <noodles@earth.li>
> ---
>  drivers/net/dsa/qca8k.c | 309 +++++++++++++++++++++++++++-------------
>  1 file changed, 212 insertions(+), 97 deletions(-)
>
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index d2b5ab403e06..dcd9e8fa99b6 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -14,6 +14,7 @@
>  #include <linux/of_platform.h>
>  #include <linux/if_bridge.h>
>  #include <linux/mdio.h>
> +#include <linux/phylink.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/etherdevice.h>
>
> @@ -418,55 +419,6 @@ qca8k_mib_init(struct qca8k_priv *priv)
>         mutex_unlock(&priv->reg_mutex);
>  }
>
> -static int
> -qca8k_set_pad_ctrl(struct qca8k_priv *priv, int port, int mode)
> -{
> -       u32 reg, val;
> -
> -       switch (port) {
> -       case 0:
> -               reg = QCA8K_REG_PORT0_PAD_CTRL;
> -               break;
> -       case 6:
> -               reg = QCA8K_REG_PORT6_PAD_CTRL;
> -               break;
> -       default:
> -               pr_err("Can't set PAD_CTRL on port %d\n", port);
> -               return -EINVAL;
> -       }
> -
> -       /* Configure a port to be directly connected to an external
> -        * PHY or MAC.
> -        */
> -       switch (mode) {
> -       case PHY_INTERFACE_MODE_RGMII:
> -               /* RGMII mode means no delay so don't enable the delay */
> -               val = QCA8K_PORT_PAD_RGMII_EN;
> -               qca8k_write(priv, reg, val);
> -               break;
> -       case PHY_INTERFACE_MODE_RGMII_ID:
> -               /* RGMII_ID needs internal delay. This is enabled through
> -                * PORT5_PAD_CTRL for all ports, rather than individual port
> -                * registers
> -                */
> -               qca8k_write(priv, reg,
> -                           QCA8K_PORT_PAD_RGMII_EN |
> -                           QCA8K_PORT_PAD_RGMII_TX_DELAY(QCA8K_MAX_DELAY) |
> -                           QCA8K_PORT_PAD_RGMII_RX_DELAY(QCA8K_MAX_DELAY));
> -               qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL,
> -                           QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
> -               break;
> -       case PHY_INTERFACE_MODE_SGMII:
> -               qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
> -               break;
> -       default:
> -               pr_err("xMII mode %d not supported\n", mode);
> -               return -EINVAL;
> -       }
> -
> -       return 0;
> -}
> -
>  static void
>  qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable)
>  {
> @@ -639,9 +591,7 @@ static int
>  qca8k_setup(struct dsa_switch *ds)
>  {
>         struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
> -       phy_interface_t phy_mode = PHY_INTERFACE_MODE_NA;
>         int ret, i;
> -       u32 mask;
>
>         /* Make sure that port 0 is the cpu port */
>         if (!dsa_is_cpu_port(ds, 0)) {
> @@ -661,24 +611,9 @@ qca8k_setup(struct dsa_switch *ds)
>         if (ret)
>                 return ret;
>
> -       /* Initialize CPU port pad mode (xMII type, delays...) */
> -       ret = of_get_phy_mode(dsa_to_port(ds, QCA8K_CPU_PORT)->dn, &phy_mode);
> -       if (ret) {
> -               pr_err("Can't find phy-mode for master device\n");
> -               return ret;
> -       }
> -       ret = qca8k_set_pad_ctrl(priv, QCA8K_CPU_PORT, phy_mode);
> -       if (ret < 0)
> -               return ret;
> -
> -       /* Enable CPU Port, force it to maximum bandwidth and full-duplex */
> -       mask = QCA8K_PORT_STATUS_SPEED_1000 | QCA8K_PORT_STATUS_TXFLOW |
> -              QCA8K_PORT_STATUS_RXFLOW | QCA8K_PORT_STATUS_DUPLEX;
> -       qca8k_write(priv, QCA8K_REG_PORT_STATUS(QCA8K_CPU_PORT), mask);
> +       /* Enable CPU Port */
>         qca8k_reg_set(priv, QCA8K_REG_GLOBAL_FW_CTRL0,
>                       QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN);
> -       qca8k_port_set_status(priv, QCA8K_CPU_PORT, 1);
> -       priv->port_sts[QCA8K_CPU_PORT].enabled = 1;
>
>         /* Enable MIB counters */
>         qca8k_mib_init(priv);
> @@ -693,10 +628,9 @@ qca8k_setup(struct dsa_switch *ds)
>                 qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
>                           QCA8K_PORT_LOOKUP_MEMBER, 0);
>
> -       /* Disable MAC by default on all user ports */
> +       /* Disable MAC by default on all ports */
>         for (i = 1; i < QCA8K_NUM_PORTS; i++)
> -               if (dsa_is_user_port(ds, i))
> -                       qca8k_port_set_status(priv, i, 0);
> +               qca8k_port_set_status(priv, i, 0);
>
>         /* Forward all unknown frames to CPU port for Linux processing */
>         qca8k_write(priv, QCA8K_REG_GLOBAL_FW_CTRL1,
> @@ -713,7 +647,7 @@ qca8k_setup(struct dsa_switch *ds)
>                                   QCA8K_PORT_LOOKUP_MEMBER, dsa_user_ports(ds));
>                 }
>
> -               /* Invividual user ports get connected to CPU port only */
> +               /* Individual user ports get connected to CPU port only */
>                 if (dsa_is_user_port(ds, i)) {
>                         int shift = 16 * (i % 2);
>
> @@ -743,44 +677,223 @@ qca8k_setup(struct dsa_switch *ds)
>  }
>
>  static void
> -qca8k_adjust_link(struct dsa_switch *ds, int port, struct phy_device *phy)
> +qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> +                        const struct phylink_link_state *state)
>  {
>         struct qca8k_priv *priv = ds->priv;
>         u32 reg;
>
> -       /* Force fixed-link setting for CPU port, skip others. */
> -       if (!phy_is_pseudo_fixed_link(phy))
> +       switch (port) {
> +       case 0: /* 1st CPU port */
> +               if (state->interface != PHY_INTERFACE_MODE_RGMII &&
> +                   state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
> +                   state->interface != PHY_INTERFACE_MODE_SGMII)
> +                       return;
> +
> +               reg = QCA8K_REG_PORT0_PAD_CTRL;
> +               break;
> +       case 1:
> +       case 2:
> +       case 3:
> +       case 4:
> +       case 5:
> +               /* Internal PHY, nothing to do */
> +               return;
> +       case 6: /* 2nd CPU port / external PHY */
> +               if (state->interface != PHY_INTERFACE_MODE_RGMII &&
> +                   state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
> +                   state->interface != PHY_INTERFACE_MODE_SGMII &&
> +                   state->interface != PHY_INTERFACE_MODE_1000BASEX)
> +                       return;
> +
> +               reg = QCA8K_REG_PORT6_PAD_CTRL;
> +               break;
> +       default:
> +               dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
> +               return;
> +       }
> +
> +       if (port != 6 && phylink_autoneg_inband(mode)) {
> +               dev_err(ds->dev, "%s: in-band negotiation unsupported\n",
> +                       __func__);
> +               return;
> +       }
> +
> +       switch (state->interface) {
> +       case PHY_INTERFACE_MODE_RGMII:
> +               /* RGMII mode means no delay so don't enable the delay */
> +               qca8k_write(priv, reg, QCA8K_PORT_PAD_RGMII_EN);
> +               break;
> +       case PHY_INTERFACE_MODE_RGMII_ID:
> +               /* RGMII_ID needs internal delay. This is enabled through
> +                * PORT5_PAD_CTRL for all ports, rather than individual port
> +                * registers
> +                */
> +               qca8k_write(priv, reg,
> +                           QCA8K_PORT_PAD_RGMII_EN |
> +                           QCA8K_PORT_PAD_RGMII_TX_DELAY(QCA8K_MAX_DELAY) |
> +                           QCA8K_PORT_PAD_RGMII_RX_DELAY(QCA8K_MAX_DELAY));

3 points here:
- Should you prevalidate the device tree bindings that in case rgmii*
mode are used, same delay settings are applied to all ports?
- Can any RGMII port be connected to a PHY? If it can, won't the PHY
enable delays too for RGMII_ID? Will the link work in that case?
- Should you treat RGMII_TX_DELAY and RGMII_RX_DELAY independently for
the case where there may be PCB traces?

> +               qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL,
> +                           QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
> +               break;
> +       case PHY_INTERFACE_MODE_SGMII:
> +       case PHY_INTERFACE_MODE_1000BASEX:
> +               /* Enable SGMII on the port */
> +               qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
> +               break;
> +       default:
> +               dev_err(ds->dev, "xMII mode %s not supported for port %d\n",
> +                       phy_modes(state->interface), port);
>                 return;
> +       }
> +}
>
> -       /* Set port speed */
> -       switch (phy->speed) {
> -       case 10:
> -               reg = QCA8K_PORT_STATUS_SPEED_10;
> +static void
> +qca8k_phylink_validate(struct dsa_switch *ds, int port,
> +                      unsigned long *supported,
> +                      struct phylink_link_state *state)
> +{
> +       __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> +
> +       switch (port) {
> +       case 0: /* 1st CPU port */
> +               if (state->interface != PHY_INTERFACE_MODE_NA &&
> +                   state->interface != PHY_INTERFACE_MODE_RGMII &&
> +                   state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
> +                   state->interface != PHY_INTERFACE_MODE_SGMII)
> +                       goto unsupported;
>                 break;
> -       case 100:
> -               reg = QCA8K_PORT_STATUS_SPEED_100;
> +       case 1:
> +       case 2:
> +       case 3:
> +       case 4:
> +       case 5:
> +               /* Internal PHY */
> +               if (state->interface != PHY_INTERFACE_MODE_NA &&
> +                   state->interface != PHY_INTERFACE_MODE_GMII)
> +                       goto unsupported;
>                 break;
> -       case 1000:
> -               reg = QCA8K_PORT_STATUS_SPEED_1000;
> +       case 6: /* 2nd CPU port / external PHY */
> +               if (state->interface != PHY_INTERFACE_MODE_NA &&
> +                   state->interface != PHY_INTERFACE_MODE_RGMII &&
> +                   state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
> +                   state->interface != PHY_INTERFACE_MODE_SGMII &&
> +                   state->interface != PHY_INTERFACE_MODE_1000BASEX)
> +                       goto unsupported;
>                 break;
>         default:
> -               dev_dbg(priv->dev, "port%d link speed %dMbps not supported.\n",
> -                       port, phy->speed);
> +               dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);

phylink has a better validation error message than this, I'd say this
is unnecessary.

> +unsupported:
> +               linkmode_zero(supported);
>                 return;
>         }
>
> -       /* Set duplex mode */
> -       if (phy->duplex == DUPLEX_FULL)
> -               reg |= QCA8K_PORT_STATUS_DUPLEX;
> +       phylink_set_port_modes(mask);
> +       phylink_set(mask, Autoneg);
> +
> +       phylink_set(mask, 1000baseT_Full);
> +       phylink_set(mask, 10baseT_Half);
> +       phylink_set(mask, 10baseT_Full);
> +       phylink_set(mask, 100baseT_Half);
> +       phylink_set(mask, 100baseT_Full);
> +
> +       if (state->interface == PHY_INTERFACE_MODE_1000BASEX)
> +               phylink_set(mask, 1000baseX_Full);
> +
> +       phylink_set(mask, Pause);
> +       phylink_set(mask, Asym_Pause);
> +
> +       linkmode_and(supported, supported, mask);
> +       linkmode_and(state->advertising, state->advertising, mask);
> +}
> +
> +static int
> +qca8k_phylink_mac_link_state(struct dsa_switch *ds, int port,
> +                            struct phylink_link_state *state)
> +{
> +       struct qca8k_priv *priv = ds->priv;
> +       u32 reg;
>
> -       /* Force flow control */
> -       if (dsa_is_cpu_port(ds, port))
> -               reg |= QCA8K_PORT_STATUS_RXFLOW | QCA8K_PORT_STATUS_TXFLOW;
> +       reg = qca8k_read(priv, QCA8K_REG_PORT_STATUS(port));
> +
> +       state->link = !!(reg & QCA8K_PORT_STATUS_LINK_UP);
> +       state->an_complete = state->link;
> +       state->an_enabled = !!(reg & QCA8K_PORT_STATUS_LINK_AUTO);
> +       state->duplex = (reg & QCA8K_PORT_STATUS_DUPLEX) ? DUPLEX_FULL :
> +                                                          DUPLEX_HALF;
> +
> +       switch (reg & QCA8K_PORT_STATUS_SPEED) {
> +       case QCA8K_PORT_STATUS_SPEED_10:
> +               state->speed = SPEED_10;
> +               break;
> +       case QCA8K_PORT_STATUS_SPEED_100:
> +               state->speed = SPEED_100;
> +               break;
> +       case QCA8K_PORT_STATUS_SPEED_1000:
> +               state->speed = SPEED_1000;
> +               break;
> +       default:
> +               state->speed = SPEED_UNKNOWN;
> +               break;
> +       }
> +
> +       state->pause = MLO_PAUSE_NONE;
> +       if (reg & QCA8K_PORT_STATUS_RXFLOW)
> +               state->pause |= MLO_PAUSE_RX;
> +       if (reg & QCA8K_PORT_STATUS_TXFLOW)
> +               state->pause |= MLO_PAUSE_TX;
> +
> +       return 1;
> +}
> +
> +static void
> +qca8k_phylink_mac_link_down(struct dsa_switch *ds, int port, unsigned int mode,
> +                           phy_interface_t interface)
> +{
> +       struct qca8k_priv *priv = ds->priv;
>
> -       /* Force link down before changing MAC options */
>         qca8k_port_set_status(priv, port, 0);
> +}
> +
> +static void
> +qca8k_phylink_mac_link_up(struct dsa_switch *ds, int port, unsigned int mode,
> +                         phy_interface_t interface, struct phy_device *phydev,
> +                         int speed, int duplex, bool tx_pause, bool rx_pause)
> +{
> +       struct qca8k_priv *priv = ds->priv;
> +       u32 reg;
> +
> +       if (phylink_autoneg_inband(mode)) {
> +               reg = QCA8K_PORT_STATUS_LINK_AUTO;
> +       } else {
> +               switch (speed) {
> +               case SPEED_10:
> +                       reg = QCA8K_PORT_STATUS_SPEED_10;
> +                       break;
> +               case SPEED_100:
> +                       reg = QCA8K_PORT_STATUS_SPEED_100;
> +                       break;
> +               case SPEED_1000:
> +                       reg = QCA8K_PORT_STATUS_SPEED_1000;
> +                       break;
> +               default:
> +                       reg = QCA8K_PORT_STATUS_LINK_AUTO;
> +                       break;
> +               }
> +
> +               if (duplex == DUPLEX_FULL)
> +                       reg |= QCA8K_PORT_STATUS_DUPLEX;
> +
> +               if (rx_pause | dsa_is_cpu_port(ds, port))

I think it is odd to do bitwise OR on booleans.

> +                       reg |= QCA8K_PORT_STATUS_RXFLOW;
> +
> +               if (tx_pause | dsa_is_cpu_port(ds, port))
> +                       reg |= QCA8K_PORT_STATUS_TXFLOW;
> +       }
> +
> +       reg |= QCA8K_PORT_STATUS_TXMAC | QCA8K_PORT_STATUS_RXMAC;
> +
>         qca8k_write(priv, QCA8K_REG_PORT_STATUS(port), reg);
> -       qca8k_port_set_status(priv, port, 1);
>  }
>
>  static void
> @@ -937,13 +1050,11 @@ qca8k_port_enable(struct dsa_switch *ds, int port,
>  {
>         struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
>
> -       if (!dsa_is_user_port(ds, port))
> -               return 0;
> -
>         qca8k_port_set_status(priv, port, 1);
>         priv->port_sts[port].enabled = 1;
>
> -       phy_support_asym_pause(phy);
> +       if (dsa_is_user_port(ds, port))
> +               phy_support_asym_pause(phy);
>
>         return 0;
>  }
> @@ -1026,7 +1137,6 @@ qca8k_get_tag_protocol(struct dsa_switch *ds, int port,
>  static const struct dsa_switch_ops qca8k_switch_ops = {
>         .get_tag_protocol       = qca8k_get_tag_protocol,
>         .setup                  = qca8k_setup,
> -       .adjust_link            = qca8k_adjust_link,
>         .get_strings            = qca8k_get_strings,
>         .get_ethtool_stats      = qca8k_get_ethtool_stats,
>         .get_sset_count         = qca8k_get_sset_count,
> @@ -1040,6 +1150,11 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
>         .port_fdb_add           = qca8k_port_fdb_add,
>         .port_fdb_del           = qca8k_port_fdb_del,
>         .port_fdb_dump          = qca8k_port_fdb_dump,
> +       .phylink_validate       = qca8k_phylink_validate,
> +       .phylink_mac_link_state = qca8k_phylink_mac_link_state,
> +       .phylink_mac_config     = qca8k_phylink_mac_config,
> +       .phylink_mac_link_down  = qca8k_phylink_mac_link_down,
> +       .phylink_mac_link_up    = qca8k_phylink_mac_link_up,
>  };
>
>  static int
> --
> 2.20.1
>

Cheers,
-Vladimir

Re: [PATCH 1/2] net: dsa: qca8k: Switch to PHYLINK instead of PHYLIB

[Jonathan McDowell]

On Thu, Jun 11, 2020 at 11:58:43AM +0300, Vladimir Oltean wrote:
> Hi Jonathan,
> 
> On Wed, 10 Jun 2020 at 23:19, Jonathan McDowell <noodles@earth.li> wrote:
> >
> > Update the driver to use the new PHYLINK callbacks, removing the
> > legacy adjust_link callback.
> >
> > Signed-off-by: Jonathan McDowell <noodles@earth.li>
...
> >  static void
> > -qca8k_adjust_link(struct dsa_switch *ds, int port, struct phy_device *phy)
> > +qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> > +                        const struct phylink_link_state *state)
> >  {
> >         struct qca8k_priv *priv = ds->priv;
> >         u32 reg;
> >
> > -       /* Force fixed-link setting for CPU port, skip others. */
> > -       if (!phy_is_pseudo_fixed_link(phy))
> > +       switch (port) {
> > +       case 0: /* 1st CPU port */
> > +               if (state->interface != PHY_INTERFACE_MODE_RGMII &&
> > +                   state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
> > +                   state->interface != PHY_INTERFACE_MODE_SGMII)
> > +                       return;
> > +
> > +               reg = QCA8K_REG_PORT0_PAD_CTRL;
> > +               break;
> > +       case 1:
> > +       case 2:
> > +       case 3:
> > +       case 4:
> > +       case 5:
> > +               /* Internal PHY, nothing to do */
> > +               return;
> > +       case 6: /* 2nd CPU port / external PHY */
> > +               if (state->interface != PHY_INTERFACE_MODE_RGMII &&
> > +                   state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
> > +                   state->interface != PHY_INTERFACE_MODE_SGMII &&
> > +                   state->interface != PHY_INTERFACE_MODE_1000BASEX)
> > +                       return;
> > +
> > +               reg = QCA8K_REG_PORT6_PAD_CTRL;
> > +               break;
> > +       default:
> > +               dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
> > +               return;
> > +       }
> > +
> > +       if (port != 6 && phylink_autoneg_inband(mode)) {
> > +               dev_err(ds->dev, "%s: in-band negotiation unsupported\n",
> > +                       __func__);
> > +               return;
> > +       }
> > +
> > +       switch (state->interface) {
> > +       case PHY_INTERFACE_MODE_RGMII:
> > +               /* RGMII mode means no delay so don't enable the delay */
> > +               qca8k_write(priv, reg, QCA8K_PORT_PAD_RGMII_EN);
> > +               break;
> > +       case PHY_INTERFACE_MODE_RGMII_ID:
> > +               /* RGMII_ID needs internal delay. This is enabled through
> > +                * PORT5_PAD_CTRL for all ports, rather than individual port
> > +                * registers
> > +                */
> > +               qca8k_write(priv, reg,
> > +                           QCA8K_PORT_PAD_RGMII_EN |
> > +                           QCA8K_PORT_PAD_RGMII_TX_DELAY(QCA8K_MAX_DELAY) |
> > +                           QCA8K_PORT_PAD_RGMII_RX_DELAY(QCA8K_MAX_DELAY));
> 
> 3 points here:
> - Should you prevalidate the device tree bindings that in case rgmii*
> mode are used, same delay settings are applied to all ports?
> - Can any RGMII port be connected to a PHY? If it can, won't the PHY
> enable delays too for RGMII_ID? Will the link work in that case?
> - Should you treat RGMII_TX_DELAY and RGMII_RX_DELAY independently for
> the case where there may be PCB traces?

The intent with this patch was to pull out the conversion to PHYLINK to
be stand-alone, with no functional changes, as request by Andrew. I
think there's room for some future clean-up here around the RGMII
options, but my main purpose in this patch set is to improve the SGMII
portion which my hardware uses that doesn't work with mainline.

> > +static void
> > +qca8k_phylink_validate(struct dsa_switch *ds, int port,
> > +                      unsigned long *supported,
> > +                      struct phylink_link_state *state)
> > +{
> > +       __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> > +
> > +       switch (port) {
> > +       case 0: /* 1st CPU port */
> > +               if (state->interface != PHY_INTERFACE_MODE_NA &&
> > +                   state->interface != PHY_INTERFACE_MODE_RGMII &&
> > +                   state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
> > +                   state->interface != PHY_INTERFACE_MODE_SGMII)
> > +                       goto unsupported;
> >                 break;
> > -       case 100:
> > -               reg = QCA8K_PORT_STATUS_SPEED_100;
> > +       case 1:
> > +       case 2:
> > +       case 3:
> > +       case 4:
> > +       case 5:
> > +               /* Internal PHY */
> > +               if (state->interface != PHY_INTERFACE_MODE_NA &&
> > +                   state->interface != PHY_INTERFACE_MODE_GMII)
> > +                       goto unsupported;
> >                 break;
> > -       case 1000:
> > -               reg = QCA8K_PORT_STATUS_SPEED_1000;
> > +       case 6: /* 2nd CPU port / external PHY */
> > +               if (state->interface != PHY_INTERFACE_MODE_NA &&
> > +                   state->interface != PHY_INTERFACE_MODE_RGMII &&
> > +                   state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
> > +                   state->interface != PHY_INTERFACE_MODE_SGMII &&
> > +                   state->interface != PHY_INTERFACE_MODE_1000BASEX)
> > +                       goto unsupported;
> >                 break;
> >         default:
> > -               dev_dbg(priv->dev, "port%d link speed %dMbps not supported.\n",
> > -                       port, phy->speed);
> > +               dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
> 
> phylink has a better validation error message than this, I'd say this
> is unnecessary.

Ok.

> > +static void
> > +qca8k_phylink_mac_link_up(struct dsa_switch *ds, int port, unsigned int mode,
> > +                         phy_interface_t interface, struct phy_device *phydev,
> > +                         int speed, int duplex, bool tx_pause, bool rx_pause)
> > +{
> > +       struct qca8k_priv *priv = ds->priv;
> > +       u32 reg;
> > +
> > +       if (phylink_autoneg_inband(mode)) {
> > +               reg = QCA8K_PORT_STATUS_LINK_AUTO;
> > +       } else {
> > +               switch (speed) {
> > +               case SPEED_10:
> > +                       reg = QCA8K_PORT_STATUS_SPEED_10;
> > +                       break;
> > +               case SPEED_100:
> > +                       reg = QCA8K_PORT_STATUS_SPEED_100;
> > +                       break;
> > +               case SPEED_1000:
> > +                       reg = QCA8K_PORT_STATUS_SPEED_1000;
> > +                       break;
> > +               default:
> > +                       reg = QCA8K_PORT_STATUS_LINK_AUTO;
> > +                       break;
> > +               }
> > +
> > +               if (duplex == DUPLEX_FULL)
> > +                       reg |= QCA8K_PORT_STATUS_DUPLEX;
> > +
> > +               if (rx_pause | dsa_is_cpu_port(ds, port))
> 
> I think it is odd to do bitwise OR on booleans.

I agree; will fix in the next spin.

J.

-- 
I get the feeling that I've been cheated.

[PATCH 2/2] net: dsa: qca8k: Improve SGMII interface handling

[Jonathan McDowell]

This patch improves the handling of the SGMII interface on the QCA8K
devices. Previously the driver did no configuration of the port, even if
it was selected. We now configure it up in the appropriate
PHY/MAC/Base-X mode depending on what phylink tells us we are connected
to and ensure it is enabled.

Tested with a device where the CPU connection is RGMII (i.e. the common
current use case) + one where the CPU connection is SGMII. I don't have
any devices where the SGMII interface is brought out to something other
than the CPU.

Signed-off-by: Jonathan McDowell <noodles@earth.li>
---
 drivers/net/dsa/qca8k.c | 28 +++++++++++++++++++++++++++-
 drivers/net/dsa/qca8k.h | 13 +++++++++++++
 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index dcd9e8fa99b6..33e62598289e 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -681,7 +681,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 			 const struct phylink_link_state *state)
 {
 	struct qca8k_priv *priv = ds->priv;
-	u32 reg;
+	u32 reg, val;
 
 	switch (port) {
 	case 0: /* 1st CPU port */
@@ -740,6 +740,32 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 	case PHY_INTERFACE_MODE_1000BASEX:
 		/* Enable SGMII on the port */
 		qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
+
+		/* Enable/disable SerDes auto-negotiation as necessary */
+		val = qca8k_read(priv, QCA8K_REG_PWS);
+		if (phylink_autoneg_inband(mode))
+			val &= ~QCA8K_PWS_SERDES_AEN_DIS;
+		else
+			val |= QCA8K_PWS_SERDES_AEN_DIS;
+		qca8k_write(priv, QCA8K_REG_PWS, val);
+
+		/* Configure the SGMII parameters */
+		val = qca8k_read(priv, QCA8K_REG_SGMII_CTRL);
+
+		val |= QCA8K_SGMII_EN_PLL | QCA8K_SGMII_EN_RX |
+			QCA8K_SGMII_EN_TX | QCA8K_SGMII_EN_SD;
+
+		val &= ~QCA8K_SGMII_MODE_CTRL_MASK;
+		if (dsa_is_cpu_port(ds, port)) {
+			/* CPU port, we're talking to the CPU MAC, be a PHY */
+			val |= QCA8K_SGMII_MODE_CTRL_PHY;
+		} else if (state->interface == PHY_INTERFACE_MODE_SGMII) {
+			val |= QCA8K_SGMII_MODE_CTRL_MAC;
+		} else {
+			val |= QCA8K_SGMII_MODE_CTRL_BASEX;
+		}
+
+		qca8k_write(priv, QCA8K_REG_SGMII_CTRL, val);
 		break;
 	default:
 		dev_err(ds->dev, "xMII mode %s not supported for port %d\n",
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 42d6ea24eb14..10ef2bca2cde 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -36,6 +36,8 @@
 #define   QCA8K_MAX_DELAY				3
 #define   QCA8K_PORT_PAD_RGMII_RX_DELAY_EN		BIT(24)
 #define   QCA8K_PORT_PAD_SGMII_EN			BIT(7)
+#define QCA8K_REG_PWS					0x010
+#define   QCA8K_PWS_SERDES_AEN_DIS			BIT(7)
 #define QCA8K_REG_MODULE_EN				0x030
 #define   QCA8K_MODULE_EN_MIB				BIT(0)
 #define QCA8K_REG_MIB					0x034
@@ -69,6 +71,7 @@
 #define   QCA8K_PORT_STATUS_LINK_UP			BIT(8)
 #define   QCA8K_PORT_STATUS_LINK_AUTO			BIT(9)
 #define   QCA8K_PORT_STATUS_LINK_PAUSE			BIT(10)
+#define   QCA8K_PORT_STATUS_FLOW_AUTO			BIT(12)
 #define QCA8K_REG_PORT_HDR_CTRL(_i)			(0x9c + (_i * 4))
 #define   QCA8K_PORT_HDR_CTRL_RX_MASK			GENMASK(3, 2)
 #define   QCA8K_PORT_HDR_CTRL_RX_S			2
@@ -77,6 +80,16 @@
 #define   QCA8K_PORT_HDR_CTRL_ALL			2
 #define   QCA8K_PORT_HDR_CTRL_MGMT			1
 #define   QCA8K_PORT_HDR_CTRL_NONE			0
+#define QCA8K_REG_SGMII_CTRL				0x0e0
+#define   QCA8K_SGMII_EN_PLL				BIT(1)
+#define   QCA8K_SGMII_EN_RX				BIT(2)
+#define   QCA8K_SGMII_EN_TX				BIT(3)
+#define   QCA8K_SGMII_EN_SD				BIT(4)
+#define   QCA8K_SGMII_CLK125M_DELAY			BIT(7)
+#define   QCA8K_SGMII_MODE_CTRL_MASK			(BIT(22) | BIT(23))
+#define   QCA8K_SGMII_MODE_CTRL_BASEX			(0 << 22)
+#define   QCA8K_SGMII_MODE_CTRL_PHY			(1 << 22)
+#define   QCA8K_SGMII_MODE_CTRL_MAC			(2 << 22)
 
 /* EEE control registers */
 #define QCA8K_REG_EEE_CTRL				0x100
-- 
2.20.1

Re: [PATCH 2/2] net: dsa: qca8k: Improve SGMII interface handling

[Florian Fainelli]

On 6/10/2020 12:15 PM, Jonathan McDowell wrote:
> This patch improves the handling of the SGMII interface on the QCA8K
> devices. Previously the driver did no configuration of the port, even if
> it was selected. We now configure it up in the appropriate
> PHY/MAC/Base-X mode depending on what phylink tells us we are connected
> to and ensure it is enabled.
> 
> Tested with a device where the CPU connection is RGMII (i.e. the common
> current use case) + one where the CPU connection is SGMII. I don't have
> any devices where the SGMII interface is brought out to something other
> than the CPU.
> 
> Signed-off-by: Jonathan McDowell <noodles@earth.li>
> ---
>  drivers/net/dsa/qca8k.c | 28 +++++++++++++++++++++++++++-
>  drivers/net/dsa/qca8k.h | 13 +++++++++++++
>  2 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index dcd9e8fa99b6..33e62598289e 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -681,7 +681,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  			 const struct phylink_link_state *state)
>  {
>  	struct qca8k_priv *priv = ds->priv;
> -	u32 reg;
> +	u32 reg, val;
>  
>  	switch (port) {
>  	case 0: /* 1st CPU port */
> @@ -740,6 +740,32 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  	case PHY_INTERFACE_MODE_1000BASEX:
>  		/* Enable SGMII on the port */
>  		qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
> +
> +		/* Enable/disable SerDes auto-negotiation as necessary */
> +		val = qca8k_read(priv, QCA8K_REG_PWS);
> +		if (phylink_autoneg_inband(mode))
> +			val &= ~QCA8K_PWS_SERDES_AEN_DIS;
> +		else
> +			val |= QCA8K_PWS_SERDES_AEN_DIS;
> +		qca8k_write(priv, QCA8K_REG_PWS, val);
> +
> +		/* Configure the SGMII parameters */
> +		val = qca8k_read(priv, QCA8K_REG_SGMII_CTRL);
> +
> +		val |= QCA8K_SGMII_EN_PLL | QCA8K_SGMII_EN_RX |
> +			QCA8K_SGMII_EN_TX | QCA8K_SGMII_EN_SD;
> +
> +		val &= ~QCA8K_SGMII_MODE_CTRL_MASK;
> +		if (dsa_is_cpu_port(ds, port)) {
> +			/* CPU port, we're talking to the CPU MAC, be a PHY */
> +			val |= QCA8K_SGMII_MODE_CTRL_PHY;

Since port 6 can be interfaced to an external PHY, do not you have to
differentiate here whether this port is connected to an actual PHY,
versus connected to a MAC? You should be able to use mode == MLO_AN_PHY
to differentiate that case from the others.

> +		} else if (state->interface == PHY_INTERFACE_MODE_SGMII) {
> +			val |= QCA8K_SGMII_MODE_CTRL_MAC;
> +		} else {
> +			val |= QCA8K_SGMII_MODE_CTRL_BASEX;

Better make this explicit and check for PHY_INTERFACE_MODE_1000BASEX,
even if those are the only two possible values covered by this part of
the case statement.
-- 
Florian

Re: [PATCH 2/2] net: dsa: qca8k: Improve SGMII interface handling

[Jonathan McDowell]

On Wed, Jun 10, 2020 at 08:31:11PM -0700, Florian Fainelli wrote:
> On 6/10/2020 12:15 PM, Jonathan McDowell wrote:
> > This patch improves the handling of the SGMII interface on the QCA8K
> > devices. Previously the driver did no configuration of the port, even if
> > it was selected. We now configure it up in the appropriate
> > PHY/MAC/Base-X mode depending on what phylink tells us we are connected
> > to and ensure it is enabled.
> > 
> > Tested with a device where the CPU connection is RGMII (i.e. the common
> > current use case) + one where the CPU connection is SGMII. I don't have
> > any devices where the SGMII interface is brought out to something other
> > than the CPU.
> > 
> > Signed-off-by: Jonathan McDowell <noodles@earth.li>
> > ---
> >  drivers/net/dsa/qca8k.c | 28 +++++++++++++++++++++++++++-
> >  drivers/net/dsa/qca8k.h | 13 +++++++++++++
> >  2 files changed, 40 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index dcd9e8fa99b6..33e62598289e 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -681,7 +681,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> >  			 const struct phylink_link_state *state)
> >  {
> >  	struct qca8k_priv *priv = ds->priv;
> > -	u32 reg;
> > +	u32 reg, val;
> >  
> >  	switch (port) {
> >  	case 0: /* 1st CPU port */
> > @@ -740,6 +740,32 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> >  	case PHY_INTERFACE_MODE_1000BASEX:
> >  		/* Enable SGMII on the port */
> >  		qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
> > +
> > +		/* Enable/disable SerDes auto-negotiation as necessary */
> > +		val = qca8k_read(priv, QCA8K_REG_PWS);
> > +		if (phylink_autoneg_inband(mode))
> > +			val &= ~QCA8K_PWS_SERDES_AEN_DIS;
> > +		else
> > +			val |= QCA8K_PWS_SERDES_AEN_DIS;
> > +		qca8k_write(priv, QCA8K_REG_PWS, val);
> > +
> > +		/* Configure the SGMII parameters */
> > +		val = qca8k_read(priv, QCA8K_REG_SGMII_CTRL);
> > +
> > +		val |= QCA8K_SGMII_EN_PLL | QCA8K_SGMII_EN_RX |
> > +			QCA8K_SGMII_EN_TX | QCA8K_SGMII_EN_SD;
> > +
> > +		val &= ~QCA8K_SGMII_MODE_CTRL_MASK;
> > +		if (dsa_is_cpu_port(ds, port)) {
> > +			/* CPU port, we're talking to the CPU MAC, be a PHY */
> > +			val |= QCA8K_SGMII_MODE_CTRL_PHY;
> 
> Since port 6 can be interfaced to an external PHY, do not you have to
> differentiate here whether this port is connected to an actual PHY,
> versus connected to a MAC? You should be able to use mode == MLO_AN_PHY
> to differentiate that case from the others.

I don't think MLO_AN_PHY is sufficient? If it's a fixed link we'll have
MLO_AN_FIXED and that could be talking to a PHY?

The logic I've gone for is assuming that a port hooked up to the CPU
should look like a PHY, and otherwise we're hooked up to a PHY so we're
acting as a MAC. That means we don't cope with the situation that we're
hooked up to something that isn't the CPU but wants us to look like a
PHY, but I don't think we have any current way to describe that.

> > +		} else if (state->interface == PHY_INTERFACE_MODE_SGMII) {
> > +			val |= QCA8K_SGMII_MODE_CTRL_MAC;
> > +		} else {
> > +			val |= QCA8K_SGMII_MODE_CTRL_BASEX;
> 
> Better make this explicit and check for PHY_INTERFACE_MODE_1000BASEX,
> even if those are the only two possible values covered by this part of
> the case statement.

Sure. I'll move the mask inside the if block too in that case, so we
don't change the setting if we get fed something invalid.

J.

-- 
Beware of programmers carrying screwdrivers.

Re: [PATCH 2/2] net: dsa: qca8k: Improve SGMII interface handling

[Russell King - ARM Linux admin]

On Wed, Jun 10, 2020 at 08:15:13PM +0100, Jonathan McDowell wrote:
> This patch improves the handling of the SGMII interface on the QCA8K
> devices. Previously the driver did no configuration of the port, even if
> it was selected. We now configure it up in the appropriate
> PHY/MAC/Base-X mode depending on what phylink tells us we are connected
> to and ensure it is enabled.
> 
> Tested with a device where the CPU connection is RGMII (i.e. the common
> current use case) + one where the CPU connection is SGMII. I don't have
> any devices where the SGMII interface is brought out to something other
> than the CPU.
> 
> Signed-off-by: Jonathan McDowell <noodles@earth.li>
> ---
>  drivers/net/dsa/qca8k.c | 28 +++++++++++++++++++++++++++-
>  drivers/net/dsa/qca8k.h | 13 +++++++++++++
>  2 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index dcd9e8fa99b6..33e62598289e 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -681,7 +681,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  			 const struct phylink_link_state *state)
>  {
>  	struct qca8k_priv *priv = ds->priv;
> -	u32 reg;
> +	u32 reg, val;
>  
>  	switch (port) {
>  	case 0: /* 1st CPU port */
> @@ -740,6 +740,32 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  	case PHY_INTERFACE_MODE_1000BASEX:
>  		/* Enable SGMII on the port */
>  		qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
> +
> +		/* Enable/disable SerDes auto-negotiation as necessary */
> +		val = qca8k_read(priv, QCA8K_REG_PWS);
> +		if (phylink_autoneg_inband(mode))
> +			val &= ~QCA8K_PWS_SERDES_AEN_DIS;
> +		else
> +			val |= QCA8K_PWS_SERDES_AEN_DIS;
> +		qca8k_write(priv, QCA8K_REG_PWS, val);
> +
> +		/* Configure the SGMII parameters */
> +		val = qca8k_read(priv, QCA8K_REG_SGMII_CTRL);
> +
> +		val |= QCA8K_SGMII_EN_PLL | QCA8K_SGMII_EN_RX |
> +			QCA8K_SGMII_EN_TX | QCA8K_SGMII_EN_SD;
> +
> +		val &= ~QCA8K_SGMII_MODE_CTRL_MASK;
> +		if (dsa_is_cpu_port(ds, port)) {
> +			/* CPU port, we're talking to the CPU MAC, be a PHY */
> +			val |= QCA8K_SGMII_MODE_CTRL_PHY;
> +		} else if (state->interface == PHY_INTERFACE_MODE_SGMII) {
> +			val |= QCA8K_SGMII_MODE_CTRL_MAC;
> +		} else {
> +			val |= QCA8K_SGMII_MODE_CTRL_BASEX;
> +		}
> +
> +		qca8k_write(priv, QCA8K_REG_SGMII_CTRL, val);

Ah, here it is!  Hmm, I suppose as the two patches will be applied
together, it's fine to split it like this.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 503kbps up

Re: [RFC PATCH v3 0/2] net: dsa: qca8k: Improve SGMII interface handling

[David Miller]

Please, when you post an RFC patch set, put "RFC" into the Subject lines
of the patches as well as the introductory posting.

This helps me categorize changes properly in patchwork.

Thank you.

[RFC PATCH v4 0/2] net: dsa: qca8k: Improve SGMII interface handling

[Jonathan McDowell]

Hopefully getting there, thanks for all the review comments.

This 2 patch series migrates the qca8k switch driver over to PHYLINK,
and then adds the SGMII clean-ups (i.e. the missing initialisation) on
top of that as a second patch.

As before, tested with a device where the CPU connection is RGMII (i.e.
the common current use case) + one where the CPU connection is SGMII. I
don't have any devices where the SGMII interface is brought out to
something other than the CPU.

v4:
- Enable pcs_poll so we keep phylink updated when doing in-band
  negotiation
- Explicitly check for PHY_INTERFACE_MODE_1000BASEX when setting SGMII
  port mode.
- Address Vladimir's review comments
v3:
- Move phylink changes to separate patch
- Address rmk review comments
v2:
- Switch to phylink
- Avoid need for device tree configuration options

Jonathan McDowell (2):
  net: dsa: qca8k: Switch to PHYLINK instead of PHYLIB
  net: dsa: qca8k: Improve SGMII interface handling

 drivers/net/dsa/qca8k.c | 341 ++++++++++++++++++++++++++++------------
 drivers/net/dsa/qca8k.h |  13 ++
 2 files changed, 256 insertions(+), 98 deletions(-)

-- 
2.20.1

[RFC PATCH v4 1/2] net: dsa: qca8k: Switch to PHYLINK instead of PHYLIB

[Jonathan McDowell]

Update the driver to use the new PHYLINK callbacks, removing the
legacy adjust_link callback.

Signed-off-by: Jonathan McDowell <noodles@earth.li>
---
 drivers/net/dsa/qca8k.c | 308 +++++++++++++++++++++++++++-------------
 1 file changed, 211 insertions(+), 97 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index d2b5ab403e06..dadf9ab2c14a 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -14,6 +14,7 @@
 #include <linux/of_platform.h>
 #include <linux/if_bridge.h>
 #include <linux/mdio.h>
+#include <linux/phylink.h>
 #include <linux/gpio/consumer.h>
 #include <linux/etherdevice.h>
 
@@ -418,55 +419,6 @@ qca8k_mib_init(struct qca8k_priv *priv)
 	mutex_unlock(&priv->reg_mutex);
 }
 
-static int
-qca8k_set_pad_ctrl(struct qca8k_priv *priv, int port, int mode)
-{
-	u32 reg, val;
-
-	switch (port) {
-	case 0:
-		reg = QCA8K_REG_PORT0_PAD_CTRL;
-		break;
-	case 6:
-		reg = QCA8K_REG_PORT6_PAD_CTRL;
-		break;
-	default:
-		pr_err("Can't set PAD_CTRL on port %d\n", port);
-		return -EINVAL;
-	}
-
-	/* Configure a port to be directly connected to an external
-	 * PHY or MAC.
-	 */
-	switch (mode) {
-	case PHY_INTERFACE_MODE_RGMII:
-		/* RGMII mode means no delay so don't enable the delay */
-		val = QCA8K_PORT_PAD_RGMII_EN;
-		qca8k_write(priv, reg, val);
-		break;
-	case PHY_INTERFACE_MODE_RGMII_ID:
-		/* RGMII_ID needs internal delay. This is enabled through
-		 * PORT5_PAD_CTRL for all ports, rather than individual port
-		 * registers
-		 */
-		qca8k_write(priv, reg,
-			    QCA8K_PORT_PAD_RGMII_EN |
-			    QCA8K_PORT_PAD_RGMII_TX_DELAY(QCA8K_MAX_DELAY) |
-			    QCA8K_PORT_PAD_RGMII_RX_DELAY(QCA8K_MAX_DELAY));
-		qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL,
-			    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
-		break;
-	case PHY_INTERFACE_MODE_SGMII:
-		qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
-		break;
-	default:
-		pr_err("xMII mode %d not supported\n", mode);
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 static void
 qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable)
 {
@@ -639,9 +591,7 @@ static int
 qca8k_setup(struct dsa_switch *ds)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
-	phy_interface_t phy_mode = PHY_INTERFACE_MODE_NA;
 	int ret, i;
-	u32 mask;
 
 	/* Make sure that port 0 is the cpu port */
 	if (!dsa_is_cpu_port(ds, 0)) {
@@ -661,24 +611,9 @@ qca8k_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
-	/* Initialize CPU port pad mode (xMII type, delays...) */
-	ret = of_get_phy_mode(dsa_to_port(ds, QCA8K_CPU_PORT)->dn, &phy_mode);
-	if (ret) {
-		pr_err("Can't find phy-mode for master device\n");
-		return ret;
-	}
-	ret = qca8k_set_pad_ctrl(priv, QCA8K_CPU_PORT, phy_mode);
-	if (ret < 0)
-		return ret;
-
-	/* Enable CPU Port, force it to maximum bandwidth and full-duplex */
-	mask = QCA8K_PORT_STATUS_SPEED_1000 | QCA8K_PORT_STATUS_TXFLOW |
-	       QCA8K_PORT_STATUS_RXFLOW | QCA8K_PORT_STATUS_DUPLEX;
-	qca8k_write(priv, QCA8K_REG_PORT_STATUS(QCA8K_CPU_PORT), mask);
+	/* Enable CPU Port */
 	qca8k_reg_set(priv, QCA8K_REG_GLOBAL_FW_CTRL0,
 		      QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN);
-	qca8k_port_set_status(priv, QCA8K_CPU_PORT, 1);
-	priv->port_sts[QCA8K_CPU_PORT].enabled = 1;
 
 	/* Enable MIB counters */
 	qca8k_mib_init(priv);
@@ -693,10 +628,9 @@ qca8k_setup(struct dsa_switch *ds)
 		qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
 			  QCA8K_PORT_LOOKUP_MEMBER, 0);
 
-	/* Disable MAC by default on all user ports */
+	/* Disable MAC by default on all ports */
 	for (i = 1; i < QCA8K_NUM_PORTS; i++)
-		if (dsa_is_user_port(ds, i))
-			qca8k_port_set_status(priv, i, 0);
+		qca8k_port_set_status(priv, i, 0);
 
 	/* Forward all unknown frames to CPU port for Linux processing */
 	qca8k_write(priv, QCA8K_REG_GLOBAL_FW_CTRL1,
@@ -713,7 +647,7 @@ qca8k_setup(struct dsa_switch *ds)
 				  QCA8K_PORT_LOOKUP_MEMBER, dsa_user_ports(ds));
 		}
 
-		/* Invividual user ports get connected to CPU port only */
+		/* Individual user ports get connected to CPU port only */
 		if (dsa_is_user_port(ds, i)) {
 			int shift = 16 * (i % 2);
 
@@ -743,44 +677,222 @@ qca8k_setup(struct dsa_switch *ds)
 }
 
 static void
-qca8k_adjust_link(struct dsa_switch *ds, int port, struct phy_device *phy)
+qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
+			 const struct phylink_link_state *state)
 {
 	struct qca8k_priv *priv = ds->priv;
 	u32 reg;
 
-	/* Force fixed-link setting for CPU port, skip others. */
-	if (!phy_is_pseudo_fixed_link(phy))
+	switch (port) {
+	case 0: /* 1st CPU port */
+		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
+		    state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
+		    state->interface != PHY_INTERFACE_MODE_SGMII)
+			return;
+
+		reg = QCA8K_REG_PORT0_PAD_CTRL;
+		break;
+	case 1:
+	case 2:
+	case 3:
+	case 4:
+	case 5:
+		/* Internal PHY, nothing to do */
+		return;
+	case 6: /* 2nd CPU port / external PHY */
+		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
+		    state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
+		    state->interface != PHY_INTERFACE_MODE_SGMII &&
+		    state->interface != PHY_INTERFACE_MODE_1000BASEX)
+			return;
+
+		reg = QCA8K_REG_PORT6_PAD_CTRL;
+		break;
+	default:
+		dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
+		return;
+	}
+
+	if (port != 6 && phylink_autoneg_inband(mode)) {
+		dev_err(ds->dev, "%s: in-band negotiation unsupported\n",
+			__func__);
+		return;
+	}
+
+	switch (state->interface) {
+	case PHY_INTERFACE_MODE_RGMII:
+		/* RGMII mode means no delay so don't enable the delay */
+		qca8k_write(priv, reg, QCA8K_PORT_PAD_RGMII_EN);
+		break;
+	case PHY_INTERFACE_MODE_RGMII_ID:
+		/* RGMII_ID needs internal delay. This is enabled through
+		 * PORT5_PAD_CTRL for all ports, rather than individual port
+		 * registers
+		 */
+		qca8k_write(priv, reg,
+			    QCA8K_PORT_PAD_RGMII_EN |
+			    QCA8K_PORT_PAD_RGMII_TX_DELAY(QCA8K_MAX_DELAY) |
+			    QCA8K_PORT_PAD_RGMII_RX_DELAY(QCA8K_MAX_DELAY));
+		qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL,
+			    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
+		break;
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_1000BASEX:
+		/* Enable SGMII on the port */
+		qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
+		break;
+	default:
+		dev_err(ds->dev, "xMII mode %s not supported for port %d\n",
+			phy_modes(state->interface), port);
 		return;
+	}
+}
 
-	/* Set port speed */
-	switch (phy->speed) {
-	case 10:
-		reg = QCA8K_PORT_STATUS_SPEED_10;
+static void
+qca8k_phylink_validate(struct dsa_switch *ds, int port,
+		       unsigned long *supported,
+		       struct phylink_link_state *state)
+{
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
+
+	switch (port) {
+	case 0: /* 1st CPU port */
+		if (state->interface != PHY_INTERFACE_MODE_NA &&
+		    state->interface != PHY_INTERFACE_MODE_RGMII &&
+		    state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
+		    state->interface != PHY_INTERFACE_MODE_SGMII)
+			goto unsupported;
 		break;
-	case 100:
-		reg = QCA8K_PORT_STATUS_SPEED_100;
+	case 1:
+	case 2:
+	case 3:
+	case 4:
+	case 5:
+		/* Internal PHY */
+		if (state->interface != PHY_INTERFACE_MODE_NA &&
+		    state->interface != PHY_INTERFACE_MODE_GMII)
+			goto unsupported;
 		break;
-	case 1000:
-		reg = QCA8K_PORT_STATUS_SPEED_1000;
+	case 6: /* 2nd CPU port / external PHY */
+		if (state->interface != PHY_INTERFACE_MODE_NA &&
+		    state->interface != PHY_INTERFACE_MODE_RGMII &&
+		    state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
+		    state->interface != PHY_INTERFACE_MODE_SGMII &&
+		    state->interface != PHY_INTERFACE_MODE_1000BASEX)
+			goto unsupported;
 		break;
 	default:
-		dev_dbg(priv->dev, "port%d link speed %dMbps not supported.\n",
-			port, phy->speed);
+unsupported:
+		linkmode_zero(supported);
 		return;
 	}
 
-	/* Set duplex mode */
-	if (phy->duplex == DUPLEX_FULL)
-		reg |= QCA8K_PORT_STATUS_DUPLEX;
+	phylink_set_port_modes(mask);
+	phylink_set(mask, Autoneg);
+
+	phylink_set(mask, 1000baseT_Full);
+	phylink_set(mask, 10baseT_Half);
+	phylink_set(mask, 10baseT_Full);
+	phylink_set(mask, 100baseT_Half);
+	phylink_set(mask, 100baseT_Full);
+
+	if (state->interface == PHY_INTERFACE_MODE_1000BASEX)
+		phylink_set(mask, 1000baseX_Full);
+
+	phylink_set(mask, Pause);
+	phylink_set(mask, Asym_Pause);
+
+	linkmode_and(supported, supported, mask);
+	linkmode_and(state->advertising, state->advertising, mask);
+}
+
+static int
+qca8k_phylink_mac_link_state(struct dsa_switch *ds, int port,
+			     struct phylink_link_state *state)
+{
+	struct qca8k_priv *priv = ds->priv;
+	u32 reg;
 
-	/* Force flow control */
-	if (dsa_is_cpu_port(ds, port))
-		reg |= QCA8K_PORT_STATUS_RXFLOW | QCA8K_PORT_STATUS_TXFLOW;
+	reg = qca8k_read(priv, QCA8K_REG_PORT_STATUS(port));
+
+	state->link = !!(reg & QCA8K_PORT_STATUS_LINK_UP);
+	state->an_complete = state->link;
+	state->an_enabled = !!(reg & QCA8K_PORT_STATUS_LINK_AUTO);
+	state->duplex = (reg & QCA8K_PORT_STATUS_DUPLEX) ? DUPLEX_FULL :
+							   DUPLEX_HALF;
+
+	switch (reg & QCA8K_PORT_STATUS_SPEED) {
+	case QCA8K_PORT_STATUS_SPEED_10:
+		state->speed = SPEED_10;
+		break;
+	case QCA8K_PORT_STATUS_SPEED_100:
+		state->speed = SPEED_100;
+		break;
+	case QCA8K_PORT_STATUS_SPEED_1000:
+		state->speed = SPEED_1000;
+		break;
+	default:
+		state->speed = SPEED_UNKNOWN;
+		break;
+	}
+
+	state->pause = MLO_PAUSE_NONE;
+	if (reg & QCA8K_PORT_STATUS_RXFLOW)
+		state->pause |= MLO_PAUSE_RX;
+	if (reg & QCA8K_PORT_STATUS_TXFLOW)
+		state->pause |= MLO_PAUSE_TX;
+
+	return 1;
+}
+
+static void
+qca8k_phylink_mac_link_down(struct dsa_switch *ds, int port, unsigned int mode,
+			    phy_interface_t interface)
+{
+	struct qca8k_priv *priv = ds->priv;
 
-	/* Force link down before changing MAC options */
 	qca8k_port_set_status(priv, port, 0);
+}
+
+static void
+qca8k_phylink_mac_link_up(struct dsa_switch *ds, int port, unsigned int mode,
+			  phy_interface_t interface, struct phy_device *phydev,
+			  int speed, int duplex, bool tx_pause, bool rx_pause)
+{
+	struct qca8k_priv *priv = ds->priv;
+	u32 reg;
+
+	if (phylink_autoneg_inband(mode)) {
+		reg = QCA8K_PORT_STATUS_LINK_AUTO;
+	} else {
+		switch (speed) {
+		case SPEED_10:
+			reg = QCA8K_PORT_STATUS_SPEED_10;
+			break;
+		case SPEED_100:
+			reg = QCA8K_PORT_STATUS_SPEED_100;
+			break;
+		case SPEED_1000:
+			reg = QCA8K_PORT_STATUS_SPEED_1000;
+			break;
+		default:
+			reg = QCA8K_PORT_STATUS_LINK_AUTO;
+			break;
+		}
+
+		if (duplex == DUPLEX_FULL)
+			reg |= QCA8K_PORT_STATUS_DUPLEX;
+
+		if (rx_pause || dsa_is_cpu_port(ds, port))
+			reg |= QCA8K_PORT_STATUS_RXFLOW;
+
+		if (tx_pause || dsa_is_cpu_port(ds, port))
+			reg |= QCA8K_PORT_STATUS_TXFLOW;
+	}
+
+	reg |= QCA8K_PORT_STATUS_TXMAC | QCA8K_PORT_STATUS_RXMAC;
+
 	qca8k_write(priv, QCA8K_REG_PORT_STATUS(port), reg);
-	qca8k_port_set_status(priv, port, 1);
 }
 
 static void
@@ -937,13 +1049,11 @@ qca8k_port_enable(struct dsa_switch *ds, int port,
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 
-	if (!dsa_is_user_port(ds, port))
-		return 0;
-
 	qca8k_port_set_status(priv, port, 1);
 	priv->port_sts[port].enabled = 1;
 
-	phy_support_asym_pause(phy);
+	if (dsa_is_user_port(ds, port))
+		phy_support_asym_pause(phy);
 
 	return 0;
 }
@@ -1026,7 +1136,6 @@ qca8k_get_tag_protocol(struct dsa_switch *ds, int port,
 static const struct dsa_switch_ops qca8k_switch_ops = {
 	.get_tag_protocol	= qca8k_get_tag_protocol,
 	.setup			= qca8k_setup,
-	.adjust_link            = qca8k_adjust_link,
 	.get_strings		= qca8k_get_strings,
 	.get_ethtool_stats	= qca8k_get_ethtool_stats,
 	.get_sset_count		= qca8k_get_sset_count,
@@ -1040,6 +1149,11 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
 	.port_fdb_add		= qca8k_port_fdb_add,
 	.port_fdb_del		= qca8k_port_fdb_del,
 	.port_fdb_dump		= qca8k_port_fdb_dump,
+	.phylink_validate	= qca8k_phylink_validate,
+	.phylink_mac_link_state	= qca8k_phylink_mac_link_state,
+	.phylink_mac_config	= qca8k_phylink_mac_config,
+	.phylink_mac_link_down	= qca8k_phylink_mac_link_down,
+	.phylink_mac_link_up	= qca8k_phylink_mac_link_up,
 };
 
 static int
-- 
2.20.1

Re: [RFC PATCH v4 1/2] net: dsa: qca8k: Switch to PHYLINK instead of PHYLIB

[Vladimir Oltean]

On Sat, 13 Jun 2020 at 14:31, Jonathan McDowell <noodles@earth.li> wrote:
>
> Update the driver to use the new PHYLINK callbacks, removing the
> legacy adjust_link callback.
>
> Signed-off-by: Jonathan McDowell <noodles@earth.li>
> ---
>  drivers/net/dsa/qca8k.c | 308 +++++++++++++++++++++++++++-------------
>  1 file changed, 211 insertions(+), 97 deletions(-)
>
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index d2b5ab403e06..dadf9ab2c14a 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -14,6 +14,7 @@
>  #include <linux/of_platform.h>
>  #include <linux/if_bridge.h>
>  #include <linux/mdio.h>
> +#include <linux/phylink.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/etherdevice.h>
>
> @@ -418,55 +419,6 @@ qca8k_mib_init(struct qca8k_priv *priv)
>         mutex_unlock(&priv->reg_mutex);
>  }
>
> -static int
> -qca8k_set_pad_ctrl(struct qca8k_priv *priv, int port, int mode)
> -{
> -       u32 reg, val;
> -
> -       switch (port) {
> -       case 0:
> -               reg = QCA8K_REG_PORT0_PAD_CTRL;
> -               break;
> -       case 6:
> -               reg = QCA8K_REG_PORT6_PAD_CTRL;
> -               break;
> -       default:
> -               pr_err("Can't set PAD_CTRL on port %d\n", port);
> -               return -EINVAL;
> -       }
> -
> -       /* Configure a port to be directly connected to an external
> -        * PHY or MAC.
> -        */
> -       switch (mode) {
> -       case PHY_INTERFACE_MODE_RGMII:
> -               /* RGMII mode means no delay so don't enable the delay */
> -               val = QCA8K_PORT_PAD_RGMII_EN;
> -               qca8k_write(priv, reg, val);
> -               break;
> -       case PHY_INTERFACE_MODE_RGMII_ID:
> -               /* RGMII_ID needs internal delay. This is enabled through
> -                * PORT5_PAD_CTRL for all ports, rather than individual port
> -                * registers
> -                */
> -               qca8k_write(priv, reg,
> -                           QCA8K_PORT_PAD_RGMII_EN |
> -                           QCA8K_PORT_PAD_RGMII_TX_DELAY(QCA8K_MAX_DELAY) |
> -                           QCA8K_PORT_PAD_RGMII_RX_DELAY(QCA8K_MAX_DELAY));
> -               qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL,
> -                           QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
> -               break;
> -       case PHY_INTERFACE_MODE_SGMII:
> -               qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
> -               break;
> -       default:
> -               pr_err("xMII mode %d not supported\n", mode);
> -               return -EINVAL;
> -       }
> -
> -       return 0;
> -}
> -
>  static void
>  qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable)
>  {
> @@ -639,9 +591,7 @@ static int
>  qca8k_setup(struct dsa_switch *ds)
>  {
>         struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
> -       phy_interface_t phy_mode = PHY_INTERFACE_MODE_NA;
>         int ret, i;
> -       u32 mask;
>
>         /* Make sure that port 0 is the cpu port */
>         if (!dsa_is_cpu_port(ds, 0)) {
> @@ -661,24 +611,9 @@ qca8k_setup(struct dsa_switch *ds)
>         if (ret)
>                 return ret;
>
> -       /* Initialize CPU port pad mode (xMII type, delays...) */
> -       ret = of_get_phy_mode(dsa_to_port(ds, QCA8K_CPU_PORT)->dn, &phy_mode);
> -       if (ret) {
> -               pr_err("Can't find phy-mode for master device\n");
> -               return ret;
> -       }
> -       ret = qca8k_set_pad_ctrl(priv, QCA8K_CPU_PORT, phy_mode);
> -       if (ret < 0)
> -               return ret;
> -
> -       /* Enable CPU Port, force it to maximum bandwidth and full-duplex */
> -       mask = QCA8K_PORT_STATUS_SPEED_1000 | QCA8K_PORT_STATUS_TXFLOW |
> -              QCA8K_PORT_STATUS_RXFLOW | QCA8K_PORT_STATUS_DUPLEX;
> -       qca8k_write(priv, QCA8K_REG_PORT_STATUS(QCA8K_CPU_PORT), mask);
> +       /* Enable CPU Port */
>         qca8k_reg_set(priv, QCA8K_REG_GLOBAL_FW_CTRL0,
>                       QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN);
> -       qca8k_port_set_status(priv, QCA8K_CPU_PORT, 1);
> -       priv->port_sts[QCA8K_CPU_PORT].enabled = 1;
>
>         /* Enable MIB counters */
>         qca8k_mib_init(priv);
> @@ -693,10 +628,9 @@ qca8k_setup(struct dsa_switch *ds)
>                 qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
>                           QCA8K_PORT_LOOKUP_MEMBER, 0);
>
> -       /* Disable MAC by default on all user ports */
> +       /* Disable MAC by default on all ports */
>         for (i = 1; i < QCA8K_NUM_PORTS; i++)
> -               if (dsa_is_user_port(ds, i))
> -                       qca8k_port_set_status(priv, i, 0);
> +               qca8k_port_set_status(priv, i, 0);
>
>         /* Forward all unknown frames to CPU port for Linux processing */
>         qca8k_write(priv, QCA8K_REG_GLOBAL_FW_CTRL1,
> @@ -713,7 +647,7 @@ qca8k_setup(struct dsa_switch *ds)
>                                   QCA8K_PORT_LOOKUP_MEMBER, dsa_user_ports(ds));
>                 }
>
> -               /* Invividual user ports get connected to CPU port only */
> +               /* Individual user ports get connected to CPU port only */

I would split typo fixes into separate patches.

>                 if (dsa_is_user_port(ds, i)) {
>                         int shift = 16 * (i % 2);
>
> @@ -743,44 +677,222 @@ qca8k_setup(struct dsa_switch *ds)
>  }
>
>  static void
> -qca8k_adjust_link(struct dsa_switch *ds, int port, struct phy_device *phy)
> +qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> +                        const struct phylink_link_state *state)
>  {
>         struct qca8k_priv *priv = ds->priv;
>         u32 reg;
>
> -       /* Force fixed-link setting for CPU port, skip others. */
> -       if (!phy_is_pseudo_fixed_link(phy))
> +       switch (port) {
> +       case 0: /* 1st CPU port */
> +               if (state->interface != PHY_INTERFACE_MODE_RGMII &&
> +                   state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
> +                   state->interface != PHY_INTERFACE_MODE_SGMII)
> +                       return;
> +
> +               reg = QCA8K_REG_PORT0_PAD_CTRL;
> +               break;
> +       case 1:
> +       case 2:
> +       case 3:
> +       case 4:
> +       case 5:
> +               /* Internal PHY, nothing to do */
> +               return;
> +       case 6: /* 2nd CPU port / external PHY */
> +               if (state->interface != PHY_INTERFACE_MODE_RGMII &&
> +                   state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
> +                   state->interface != PHY_INTERFACE_MODE_SGMII &&
> +                   state->interface != PHY_INTERFACE_MODE_1000BASEX)
> +                       return;
> +
> +               reg = QCA8K_REG_PORT6_PAD_CTRL;
> +               break;
> +       default:
> +               dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
> +               return;
> +       }
> +
> +       if (port != 6 && phylink_autoneg_inband(mode)) {
> +               dev_err(ds->dev, "%s: in-band negotiation unsupported\n",
> +                       __func__);
> +               return;
> +       }
> +
> +       switch (state->interface) {
> +       case PHY_INTERFACE_MODE_RGMII:
> +               /* RGMII mode means no delay so don't enable the delay */
> +               qca8k_write(priv, reg, QCA8K_PORT_PAD_RGMII_EN);
> +               break;
> +       case PHY_INTERFACE_MODE_RGMII_ID:
> +               /* RGMII_ID needs internal delay. This is enabled through
> +                * PORT5_PAD_CTRL for all ports, rather than individual port
> +                * registers
> +                */
> +               qca8k_write(priv, reg,
> +                           QCA8K_PORT_PAD_RGMII_EN |
> +                           QCA8K_PORT_PAD_RGMII_TX_DELAY(QCA8K_MAX_DELAY) |
> +                           QCA8K_PORT_PAD_RGMII_RX_DELAY(QCA8K_MAX_DELAY));
> +               qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL,
> +                           QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
> +               break;
> +       case PHY_INTERFACE_MODE_SGMII:
> +       case PHY_INTERFACE_MODE_1000BASEX:
> +               /* Enable SGMII on the port */
> +               qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
> +               break;
> +       default:
> +               dev_err(ds->dev, "xMII mode %s not supported for port %d\n",
> +                       phy_modes(state->interface), port);
>                 return;
> +       }
> +}
>
> -       /* Set port speed */
> -       switch (phy->speed) {
> -       case 10:
> -               reg = QCA8K_PORT_STATUS_SPEED_10;
> +static void
> +qca8k_phylink_validate(struct dsa_switch *ds, int port,
> +                      unsigned long *supported,
> +                      struct phylink_link_state *state)
> +{
> +       __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> +
> +       switch (port) {
> +       case 0: /* 1st CPU port */
> +               if (state->interface != PHY_INTERFACE_MODE_NA &&
> +                   state->interface != PHY_INTERFACE_MODE_RGMII &&
> +                   state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
> +                   state->interface != PHY_INTERFACE_MODE_SGMII)
> +                       goto unsupported;
>                 break;
> -       case 100:
> -               reg = QCA8K_PORT_STATUS_SPEED_100;
> +       case 1:
> +       case 2:
> +       case 3:
> +       case 4:
> +       case 5:
> +               /* Internal PHY */
> +               if (state->interface != PHY_INTERFACE_MODE_NA &&
> +                   state->interface != PHY_INTERFACE_MODE_GMII)
> +                       goto unsupported;
>                 break;
> -       case 1000:
> -               reg = QCA8K_PORT_STATUS_SPEED_1000;
> +       case 6: /* 2nd CPU port / external PHY */
> +               if (state->interface != PHY_INTERFACE_MODE_NA &&
> +                   state->interface != PHY_INTERFACE_MODE_RGMII &&
> +                   state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
> +                   state->interface != PHY_INTERFACE_MODE_SGMII &&
> +                   state->interface != PHY_INTERFACE_MODE_1000BASEX)
> +                       goto unsupported;
>                 break;
>         default:
> -               dev_dbg(priv->dev, "port%d link speed %dMbps not supported.\n",
> -                       port, phy->speed);
> +unsupported:
> +               linkmode_zero(supported);
>                 return;
>         }
>
> -       /* Set duplex mode */
> -       if (phy->duplex == DUPLEX_FULL)
> -               reg |= QCA8K_PORT_STATUS_DUPLEX;
> +       phylink_set_port_modes(mask);
> +       phylink_set(mask, Autoneg);
> +
> +       phylink_set(mask, 1000baseT_Full);
> +       phylink_set(mask, 10baseT_Half);
> +       phylink_set(mask, 10baseT_Full);
> +       phylink_set(mask, 100baseT_Half);
> +       phylink_set(mask, 100baseT_Full);
> +
> +       if (state->interface == PHY_INTERFACE_MODE_1000BASEX)
> +               phylink_set(mask, 1000baseX_Full);
> +
> +       phylink_set(mask, Pause);
> +       phylink_set(mask, Asym_Pause);
> +
> +       linkmode_and(supported, supported, mask);
> +       linkmode_and(state->advertising, state->advertising, mask);
> +}
> +
> +static int
> +qca8k_phylink_mac_link_state(struct dsa_switch *ds, int port,
> +                            struct phylink_link_state *state)
> +{
> +       struct qca8k_priv *priv = ds->priv;
> +       u32 reg;
>
> -       /* Force flow control */
> -       if (dsa_is_cpu_port(ds, port))
> -               reg |= QCA8K_PORT_STATUS_RXFLOW | QCA8K_PORT_STATUS_TXFLOW;
> +       reg = qca8k_read(priv, QCA8K_REG_PORT_STATUS(port));
> +
> +       state->link = !!(reg & QCA8K_PORT_STATUS_LINK_UP);
> +       state->an_complete = state->link;
> +       state->an_enabled = !!(reg & QCA8K_PORT_STATUS_LINK_AUTO);
> +       state->duplex = (reg & QCA8K_PORT_STATUS_DUPLEX) ? DUPLEX_FULL :
> +                                                          DUPLEX_HALF;
> +
> +       switch (reg & QCA8K_PORT_STATUS_SPEED) {
> +       case QCA8K_PORT_STATUS_SPEED_10:
> +               state->speed = SPEED_10;
> +               break;
> +       case QCA8K_PORT_STATUS_SPEED_100:
> +               state->speed = SPEED_100;
> +               break;
> +       case QCA8K_PORT_STATUS_SPEED_1000:
> +               state->speed = SPEED_1000;
> +               break;
> +       default:
> +               state->speed = SPEED_UNKNOWN;
> +               break;
> +       }
> +
> +       state->pause = MLO_PAUSE_NONE;
> +       if (reg & QCA8K_PORT_STATUS_RXFLOW)
> +               state->pause |= MLO_PAUSE_RX;
> +       if (reg & QCA8K_PORT_STATUS_TXFLOW)
> +               state->pause |= MLO_PAUSE_TX;
> +
> +       return 1;
> +}
> +
> +static void
> +qca8k_phylink_mac_link_down(struct dsa_switch *ds, int port, unsigned int mode,
> +                           phy_interface_t interface)
> +{
> +       struct qca8k_priv *priv = ds->priv;
>
> -       /* Force link down before changing MAC options */
>         qca8k_port_set_status(priv, port, 0);
> +}
> +
> +static void
> +qca8k_phylink_mac_link_up(struct dsa_switch *ds, int port, unsigned int mode,
> +                         phy_interface_t interface, struct phy_device *phydev,
> +                         int speed, int duplex, bool tx_pause, bool rx_pause)
> +{
> +       struct qca8k_priv *priv = ds->priv;
> +       u32 reg;
> +
> +       if (phylink_autoneg_inband(mode)) {
> +               reg = QCA8K_PORT_STATUS_LINK_AUTO;
> +       } else {
> +               switch (speed) {
> +               case SPEED_10:
> +                       reg = QCA8K_PORT_STATUS_SPEED_10;
> +                       break;
> +               case SPEED_100:
> +                       reg = QCA8K_PORT_STATUS_SPEED_100;
> +                       break;
> +               case SPEED_1000:
> +                       reg = QCA8K_PORT_STATUS_SPEED_1000;
> +                       break;
> +               default:
> +                       reg = QCA8K_PORT_STATUS_LINK_AUTO;
> +                       break;
> +               }
> +
> +               if (duplex == DUPLEX_FULL)
> +                       reg |= QCA8K_PORT_STATUS_DUPLEX;
> +
> +               if (rx_pause || dsa_is_cpu_port(ds, port))
> +                       reg |= QCA8K_PORT_STATUS_RXFLOW;
> +
> +               if (tx_pause || dsa_is_cpu_port(ds, port))
> +                       reg |= QCA8K_PORT_STATUS_TXFLOW;
> +       }
> +
> +       reg |= QCA8K_PORT_STATUS_TXMAC | QCA8K_PORT_STATUS_RXMAC;
> +
>         qca8k_write(priv, QCA8K_REG_PORT_STATUS(port), reg);
> -       qca8k_port_set_status(priv, port, 1);
>  }
>
>  static void
> @@ -937,13 +1049,11 @@ qca8k_port_enable(struct dsa_switch *ds, int port,
>  {
>         struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
>
> -       if (!dsa_is_user_port(ds, port))
> -               return 0;
> -
>         qca8k_port_set_status(priv, port, 1);
>         priv->port_sts[port].enabled = 1;
>
> -       phy_support_asym_pause(phy);
> +       if (dsa_is_user_port(ds, port))
> +               phy_support_asym_pause(phy);
>
>         return 0;
>  }
> @@ -1026,7 +1136,6 @@ qca8k_get_tag_protocol(struct dsa_switch *ds, int port,
>  static const struct dsa_switch_ops qca8k_switch_ops = {
>         .get_tag_protocol       = qca8k_get_tag_protocol,
>         .setup                  = qca8k_setup,
> -       .adjust_link            = qca8k_adjust_link,
>         .get_strings            = qca8k_get_strings,
>         .get_ethtool_stats      = qca8k_get_ethtool_stats,
>         .get_sset_count         = qca8k_get_sset_count,
> @@ -1040,6 +1149,11 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
>         .port_fdb_add           = qca8k_port_fdb_add,
>         .port_fdb_del           = qca8k_port_fdb_del,
>         .port_fdb_dump          = qca8k_port_fdb_dump,
> +       .phylink_validate       = qca8k_phylink_validate,
> +       .phylink_mac_link_state = qca8k_phylink_mac_link_state,
> +       .phylink_mac_config     = qca8k_phylink_mac_config,
> +       .phylink_mac_link_down  = qca8k_phylink_mac_link_down,
> +       .phylink_mac_link_up    = qca8k_phylink_mac_link_up,
>  };
>
>  static int
> --
> 2.20.1
>

[RFC PATCH v4 2/2] net: dsa: qca8k: Improve SGMII interface handling

[Jonathan McDowell]

This patch improves the handling of the SGMII interface on the QCA8K
devices. Previously the driver did no configuration of the port, even if
it was selected. We now configure it up in the appropriate
PHY/MAC/Base-X mode depending on what phylink tells us we are connected
to and ensure it is enabled.

Tested with a device where the CPU connection is RGMII (i.e. the common
current use case) + one where the CPU connection is SGMII. I don't have
any devices where the SGMII interface is brought out to something other
than the CPU.

Signed-off-by: Jonathan McDowell <noodles@earth.li>
---
 drivers/net/dsa/qca8k.c | 33 ++++++++++++++++++++++++++++++++-
 drivers/net/dsa/qca8k.h | 13 +++++++++++++
 2 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index dadf9ab2c14a..da7d2b92ed3e 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -673,6 +673,9 @@ qca8k_setup(struct dsa_switch *ds)
 	/* Flush the FDB table */
 	qca8k_fdb_flush(priv);
 
+	/* We don't have interrupts for link changes, so we need to poll */
+	priv->ds->pcs_poll = true;
+
 	return 0;
 }
 
@@ -681,7 +684,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 			 const struct phylink_link_state *state)
 {
 	struct qca8k_priv *priv = ds->priv;
-	u32 reg;
+	u32 reg, val;
 
 	switch (port) {
 	case 0: /* 1st CPU port */
@@ -740,6 +743,34 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 	case PHY_INTERFACE_MODE_1000BASEX:
 		/* Enable SGMII on the port */
 		qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
+
+		/* Enable/disable SerDes auto-negotiation as necessary */
+		val = qca8k_read(priv, QCA8K_REG_PWS);
+		if (phylink_autoneg_inband(mode))
+			val &= ~QCA8K_PWS_SERDES_AEN_DIS;
+		else
+			val |= QCA8K_PWS_SERDES_AEN_DIS;
+		qca8k_write(priv, QCA8K_REG_PWS, val);
+
+		/* Configure the SGMII parameters */
+		val = qca8k_read(priv, QCA8K_REG_SGMII_CTRL);
+
+		val |= QCA8K_SGMII_EN_PLL | QCA8K_SGMII_EN_RX |
+			QCA8K_SGMII_EN_TX | QCA8K_SGMII_EN_SD;
+
+		if (dsa_is_cpu_port(ds, port)) {
+			/* CPU port, we're talking to the CPU MAC, be a PHY */
+			val &= ~QCA8K_SGMII_MODE_CTRL_MASK;
+			val |= QCA8K_SGMII_MODE_CTRL_PHY;
+		} else if (state->interface == PHY_INTERFACE_MODE_SGMII) {
+			val &= ~QCA8K_SGMII_MODE_CTRL_MASK;
+			val |= QCA8K_SGMII_MODE_CTRL_MAC;
+		} else if (state->interface == PHY_INTERFACE_MODE_1000BASEX) {
+			val &= ~QCA8K_SGMII_MODE_CTRL_MASK;
+			val |= QCA8K_SGMII_MODE_CTRL_BASEX;
+		}
+
+		qca8k_write(priv, QCA8K_REG_SGMII_CTRL, val);
 		break;
 	default:
 		dev_err(ds->dev, "xMII mode %s not supported for port %d\n",
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 42d6ea24eb14..10ef2bca2cde 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -36,6 +36,8 @@
 #define   QCA8K_MAX_DELAY				3
 #define   QCA8K_PORT_PAD_RGMII_RX_DELAY_EN		BIT(24)
 #define   QCA8K_PORT_PAD_SGMII_EN			BIT(7)
+#define QCA8K_REG_PWS					0x010
+#define   QCA8K_PWS_SERDES_AEN_DIS			BIT(7)
 #define QCA8K_REG_MODULE_EN				0x030
 #define   QCA8K_MODULE_EN_MIB				BIT(0)
 #define QCA8K_REG_MIB					0x034
@@ -69,6 +71,7 @@
 #define   QCA8K_PORT_STATUS_LINK_UP			BIT(8)
 #define   QCA8K_PORT_STATUS_LINK_AUTO			BIT(9)
 #define   QCA8K_PORT_STATUS_LINK_PAUSE			BIT(10)
+#define   QCA8K_PORT_STATUS_FLOW_AUTO			BIT(12)
 #define QCA8K_REG_PORT_HDR_CTRL(_i)			(0x9c + (_i * 4))
 #define   QCA8K_PORT_HDR_CTRL_RX_MASK			GENMASK(3, 2)
 #define   QCA8K_PORT_HDR_CTRL_RX_S			2
@@ -77,6 +80,16 @@
 #define   QCA8K_PORT_HDR_CTRL_ALL			2
 #define   QCA8K_PORT_HDR_CTRL_MGMT			1
 #define   QCA8K_PORT_HDR_CTRL_NONE			0
+#define QCA8K_REG_SGMII_CTRL				0x0e0
+#define   QCA8K_SGMII_EN_PLL				BIT(1)
+#define   QCA8K_SGMII_EN_RX				BIT(2)
+#define   QCA8K_SGMII_EN_TX				BIT(3)
+#define   QCA8K_SGMII_EN_SD				BIT(4)
+#define   QCA8K_SGMII_CLK125M_DELAY			BIT(7)
+#define   QCA8K_SGMII_MODE_CTRL_MASK			(BIT(22) | BIT(23))
+#define   QCA8K_SGMII_MODE_CTRL_BASEX			(0 << 22)
+#define   QCA8K_SGMII_MODE_CTRL_PHY			(1 << 22)
+#define   QCA8K_SGMII_MODE_CTRL_MAC			(2 << 22)
 
 /* EEE control registers */
 #define QCA8K_REG_EEE_CTRL				0x100
-- 
2.20.1

Re: [RFC PATCH v4 2/2] net: dsa: qca8k: Improve SGMII interface handling

[Vladimir Oltean]

On Sat, 13 Jun 2020 at 14:32, Jonathan McDowell <noodles@earth.li> wrote:
>
> This patch improves the handling of the SGMII interface on the QCA8K
> devices. Previously the driver did no configuration of the port, even if
> it was selected. We now configure it up in the appropriate
> PHY/MAC/Base-X mode depending on what phylink tells us we are connected
> to and ensure it is enabled.
>
> Tested with a device where the CPU connection is RGMII (i.e. the common
> current use case) + one where the CPU connection is SGMII. I don't have
> any devices where the SGMII interface is brought out to something other
> than the CPU.
>
> Signed-off-by: Jonathan McDowell <noodles@earth.li>
> ---
>  drivers/net/dsa/qca8k.c | 33 ++++++++++++++++++++++++++++++++-
>  drivers/net/dsa/qca8k.h | 13 +++++++++++++
>  2 files changed, 45 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index dadf9ab2c14a..da7d2b92ed3e 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -673,6 +673,9 @@ qca8k_setup(struct dsa_switch *ds)
>         /* Flush the FDB table */
>         qca8k_fdb_flush(priv);
>
> +       /* We don't have interrupts for link changes, so we need to poll */
> +       priv->ds->pcs_poll = true;
> +

You can access ds directly here.

>         return 0;
>  }
>
> @@ -681,7 +684,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>                          const struct phylink_link_state *state)
>  {
>         struct qca8k_priv *priv = ds->priv;
> -       u32 reg;
> +       u32 reg, val;
>
>         switch (port) {
>         case 0: /* 1st CPU port */
> @@ -740,6 +743,34 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>         case PHY_INTERFACE_MODE_1000BASEX:
>                 /* Enable SGMII on the port */
>                 qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
> +
> +               /* Enable/disable SerDes auto-negotiation as necessary */
> +               val = qca8k_read(priv, QCA8K_REG_PWS);
> +               if (phylink_autoneg_inband(mode))
> +                       val &= ~QCA8K_PWS_SERDES_AEN_DIS;
> +               else
> +                       val |= QCA8K_PWS_SERDES_AEN_DIS;
> +               qca8k_write(priv, QCA8K_REG_PWS, val);
> +
> +               /* Configure the SGMII parameters */
> +               val = qca8k_read(priv, QCA8K_REG_SGMII_CTRL);
> +
> +               val |= QCA8K_SGMII_EN_PLL | QCA8K_SGMII_EN_RX |
> +                       QCA8K_SGMII_EN_TX | QCA8K_SGMII_EN_SD;
> +
> +               if (dsa_is_cpu_port(ds, port)) {

I don't see any device tree in mainline for qca,qca8334 that uses
SGMII on the CPU port, but there are some assumptions being made here,
and there are also going to be some assumptions made in the MAC
driver, and I just want to make sure that those assumptions are not
going to be incompatible, so I would like you to make some
clarifications.
So there's a single SGMII interface which can go to port 0 (the CPU
port) or to port 6, right? The SGMII port can behave as an AN master
or as an AN slave, depending on whether MODE_CTRL is 1 or 2, or can
have a forced speed (if SERDES_AEN is disabled)?
We don't have a standard way to describe an SGMII AN master that is
not a PHY in the device tree, because I don't think anybody needed to
do that so far.
Typically a MAC would describe the link towards the CPU port of the
switch as a fixed-link. In that case, if the phy-mode is sgmii, it
would disable in-band autoneg, because there's nothing really to
negotiate (the link speed and duplex is fixed). For these, I think the
expectation is that the switch does not enable in-band autoneg either,
and has a fixed-link too. Per your configuration, you would disable
SerDes AN, and you would configure the port as SGMII AN master (PHY),
but that setting would be ignored because AN is disabled.
In other configurations, the MAC might want to receive in-band status
from the CPU port. In those cases, your answer to that problem seems
to be to implement phylink ops on both drivers, and to set both to
managed = "in-band-status" (MLO_AN_INBAND). This isn't a use case
explicitly described by phylink (I would even go as far as saying that
MLO_AN_INBAND means to be an SGMII AN slave), but it would work
because of the check that we are a CPU port.
As for the case of a cascaded qca8334-to-qca8334 setup, this would
again work, because on one of the switches, dsa_is_cpu_port would be
true and on the other one it would be false.
So I'm not suggesting we should change anything, I just want to make
sure I understand if this is the reason why you are configuring it
like this.

> +                       /* CPU port, we're talking to the CPU MAC, be a PHY */
> +                       val &= ~QCA8K_SGMII_MODE_CTRL_MASK;
> +                       val |= QCA8K_SGMII_MODE_CTRL_PHY;
> +               } else if (state->interface == PHY_INTERFACE_MODE_SGMII) {
> +                       val &= ~QCA8K_SGMII_MODE_CTRL_MASK;
> +                       val |= QCA8K_SGMII_MODE_CTRL_MAC;
> +               } else if (state->interface == PHY_INTERFACE_MODE_1000BASEX) {
> +                       val &= ~QCA8K_SGMII_MODE_CTRL_MASK;
> +                       val |= QCA8K_SGMII_MODE_CTRL_BASEX;
> +               }
> +
> +               qca8k_write(priv, QCA8K_REG_SGMII_CTRL, val);
>                 break;
>         default:
>                 dev_err(ds->dev, "xMII mode %s not supported for port %d\n",
> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> index 42d6ea24eb14..10ef2bca2cde 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -36,6 +36,8 @@
>  #define   QCA8K_MAX_DELAY                              3
>  #define   QCA8K_PORT_PAD_RGMII_RX_DELAY_EN             BIT(24)
>  #define   QCA8K_PORT_PAD_SGMII_EN                      BIT(7)
> +#define QCA8K_REG_PWS                                  0x010
> +#define   QCA8K_PWS_SERDES_AEN_DIS                     BIT(7)
>  #define QCA8K_REG_MODULE_EN                            0x030
>  #define   QCA8K_MODULE_EN_MIB                          BIT(0)
>  #define QCA8K_REG_MIB                                  0x034
> @@ -69,6 +71,7 @@
>  #define   QCA8K_PORT_STATUS_LINK_UP                    BIT(8)
>  #define   QCA8K_PORT_STATUS_LINK_AUTO                  BIT(9)
>  #define   QCA8K_PORT_STATUS_LINK_PAUSE                 BIT(10)
> +#define   QCA8K_PORT_STATUS_FLOW_AUTO                  BIT(12)
>  #define QCA8K_REG_PORT_HDR_CTRL(_i)                    (0x9c + (_i * 4))
>  #define   QCA8K_PORT_HDR_CTRL_RX_MASK                  GENMASK(3, 2)
>  #define   QCA8K_PORT_HDR_CTRL_RX_S                     2
> @@ -77,6 +80,16 @@
>  #define   QCA8K_PORT_HDR_CTRL_ALL                      2
>  #define   QCA8K_PORT_HDR_CTRL_MGMT                     1
>  #define   QCA8K_PORT_HDR_CTRL_NONE                     0
> +#define QCA8K_REG_SGMII_CTRL                           0x0e0
> +#define   QCA8K_SGMII_EN_PLL                           BIT(1)
> +#define   QCA8K_SGMII_EN_RX                            BIT(2)
> +#define   QCA8K_SGMII_EN_TX                            BIT(3)
> +#define   QCA8K_SGMII_EN_SD                            BIT(4)
> +#define   QCA8K_SGMII_CLK125M_DELAY                    BIT(7)
> +#define   QCA8K_SGMII_MODE_CTRL_MASK                   (BIT(22) | BIT(23))
> +#define   QCA8K_SGMII_MODE_CTRL_BASEX                  (0 << 22)
> +#define   QCA8K_SGMII_MODE_CTRL_PHY                    (1 << 22)
> +#define   QCA8K_SGMII_MODE_CTRL_MAC                    (2 << 22)
>
>  /* EEE control registers */
>  #define QCA8K_REG_EEE_CTRL                             0x100
> --
> 2.20.1
>

Re: [RFC PATCH v4 2/2] net: dsa: qca8k: Improve SGMII interface handling

[Jonathan McDowell]

On Sat, Jun 13, 2020 at 11:10:49PM +0300, Vladimir Oltean wrote:
> On Sat, 13 Jun 2020 at 14:32, Jonathan McDowell <noodles@earth.li> wrote:
> >
> > This patch improves the handling of the SGMII interface on the QCA8K
> > devices. Previously the driver did no configuration of the port, even if
> > it was selected. We now configure it up in the appropriate
> > PHY/MAC/Base-X mode depending on what phylink tells us we are connected
> > to and ensure it is enabled.
> >
> > Tested with a device where the CPU connection is RGMII (i.e. the common
> > current use case) + one where the CPU connection is SGMII. I don't have
> > any devices where the SGMII interface is brought out to something other
> > than the CPU.
> >
> > Signed-off-by: Jonathan McDowell <noodles@earth.li>
> > ---
> >  drivers/net/dsa/qca8k.c | 33 ++++++++++++++++++++++++++++++++-
> >  drivers/net/dsa/qca8k.h | 13 +++++++++++++
> >  2 files changed, 45 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index dadf9ab2c14a..da7d2b92ed3e 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -673,6 +673,9 @@ qca8k_setup(struct dsa_switch *ds)
> >         /* Flush the FDB table */
> >         qca8k_fdb_flush(priv);
> >
> > +       /* We don't have interrupts for link changes, so we need to poll */
> > +       priv->ds->pcs_poll = true;
> > +
> 
> You can access ds directly here.

Good point, thanks.

> >         return 0;
> >  }
> >
> > @@ -681,7 +684,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> >                          const struct phylink_link_state *state)
> >  {
> >         struct qca8k_priv *priv = ds->priv;
> > -       u32 reg;
> > +       u32 reg, val;
> >
> >         switch (port) {
> >         case 0: /* 1st CPU port */
> > @@ -740,6 +743,34 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> >         case PHY_INTERFACE_MODE_1000BASEX:
> >                 /* Enable SGMII on the port */
> >                 qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
> > +
> > +               /* Enable/disable SerDes auto-negotiation as necessary */
> > +               val = qca8k_read(priv, QCA8K_REG_PWS);
> > +               if (phylink_autoneg_inband(mode))
> > +                       val &= ~QCA8K_PWS_SERDES_AEN_DIS;
> > +               else
> > +                       val |= QCA8K_PWS_SERDES_AEN_DIS;
> > +               qca8k_write(priv, QCA8K_REG_PWS, val);
> > +
> > +               /* Configure the SGMII parameters */
> > +               val = qca8k_read(priv, QCA8K_REG_SGMII_CTRL);
> > +
> > +               val |= QCA8K_SGMII_EN_PLL | QCA8K_SGMII_EN_RX |
> > +                       QCA8K_SGMII_EN_TX | QCA8K_SGMII_EN_SD;
> > +
> > +               if (dsa_is_cpu_port(ds, port)) {

> I don't see any device tree in mainline for qca,qca8334 that uses
> SGMII on the CPU port, but there are some assumptions being made here,
> and there are also going to be some assumptions made in the MAC
> driver, and I just want to make sure that those assumptions are not
> going to be incompatible, so I would like you to make some
> clarifications.

FWIW I have a DTS for the MikroTik RB3011 that should hopefully make it
to 5.9 via the linux-msm tree, which has 2 qca,qca8337 devices, one
connected via rgmii, one connected via sgmii. Sadly not chained to each
other, instead connected to different CPU ethernet ports.

> So there's a single SGMII interface which can go to port 0 (the CPU
> port) or to port 6, right? The SGMII port can behave as an AN master
> or as an AN slave, depending on whether MODE_CTRL is 1 or 2, or can
> have a forced speed (if SERDES_AEN is disabled)?

Yes.

> We don't have a standard way to describe an SGMII AN master that is
> not a PHY in the device tree, because I don't think anybody needed to
> do that so far.
>
> Typically a MAC would describe the link towards the CPU port of the
> switch as a fixed-link. In that case, if the phy-mode is sgmii, it
> would disable in-band autoneg, because there's nothing really to
> negotiate (the link speed and duplex is fixed). For these, I think the
> expectation is that the switch does not enable in-band autoneg either,
> and has a fixed-link too. Per your configuration, you would disable
> SerDes AN, and you would configure the port as SGMII AN master (PHY),
> but that setting would be ignored because AN is disabled.

Right; this is the situation with my device. There's a fixed-link stanza
in the DT.

> In other configurations, the MAC might want to receive in-band status
> from the CPU port. In those cases, your answer to that problem seems
> to be to implement phylink ops on both drivers, and to set both to
> managed = "in-band-status" (MLO_AN_INBAND). This isn't a use case
> explicitly described by phylink (I would even go as far as saying that
> MLO_AN_INBAND means to be an SGMII AN slave), but it would work
> because of the check that we are a CPU port.

> As for the case of a cascaded qca8334-to-qca8334 setup, this would
> again work, because on one of the switches, dsa_is_cpu_port would be
> true and on the other one it would be false.

> So I'm not suggesting we should change anything, I just want to make
> sure I understand if this is the reason why you are configuring it
> like this.

My primary concern was not breaking any existing users; after a reset
the switch enabled AN on the SGMII port. I agree it's unlikely that this
would be the case, but I erred on the side of caution, while also trying
to handle what seem to be the sensible common cases.

J.

-- 
He's weird? It's ok, I'm fluent in weird.

[PATCH net-next v5 0/3] net: dsa: qca8k: Improve SGMII interface handling

[Jonathan McDowell]

This 3 patch series migrates the qca8k switch driver over to PHYLINK,
and then adds the SGMII clean-ups (i.e. the missing initialisation) on
top of that as a second patch. The final patch is a simple spelling fix
in a comment.

As before, tested with a device where the CPU connection is RGMII (i.e.
the common current use case) + one where the CPU connection is SGMII. I
don't have any devices where the SGMII interface is brought out to
something other than the CPU.

v5:
- Move spelling fix to separate patch
- Use ds directly rather than ds->priv
v4:
- Enable pcs_poll so we keep phylink updated when doing in-band
  negotiation
- Explicitly check for PHY_INTERFACE_MODE_1000BASEX when setting SGMII
  port mode.
- Address Vladimir's review comments
v3:
- Move phylink changes to separate patch
- Address rmk review comments
v2:
- Switch to phylink
- Avoid need for device tree configuration options

Jonathan McDowell (3):
  net: dsa: qca8k: Switch to PHYLINK instead of PHYLIB
  net: dsa: qca8k: Improve SGMII interface handling
  net: dsa: qca8k: Minor comment spelling fix

 drivers/net/dsa/qca8k.c | 341 ++++++++++++++++++++++++++++------------
 drivers/net/dsa/qca8k.h |  13 ++
 2 files changed, 256 insertions(+), 98 deletions(-)

-- 
2.20.1

[PATCH net-next v5 1/3] net: dsa: qca8k: Switch to PHYLINK instead of PHYLIB

[Jonathan McDowell]

Update the driver to use the new PHYLINK callbacks, removing the
legacy adjust_link callback.

Signed-off-by: Jonathan McDowell <noodles@earth.li>
---
 drivers/net/dsa/qca8k.c | 306 +++++++++++++++++++++++++++-------------
 1 file changed, 210 insertions(+), 96 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index d2b5ab403e06..63b84789f16b 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -14,6 +14,7 @@
 #include <linux/of_platform.h>
 #include <linux/if_bridge.h>
 #include <linux/mdio.h>
+#include <linux/phylink.h>
 #include <linux/gpio/consumer.h>
 #include <linux/etherdevice.h>
 
@@ -418,55 +419,6 @@ qca8k_mib_init(struct qca8k_priv *priv)
 	mutex_unlock(&priv->reg_mutex);
 }
 
-static int
-qca8k_set_pad_ctrl(struct qca8k_priv *priv, int port, int mode)
-{
-	u32 reg, val;
-
-	switch (port) {
-	case 0:
-		reg = QCA8K_REG_PORT0_PAD_CTRL;
-		break;
-	case 6:
-		reg = QCA8K_REG_PORT6_PAD_CTRL;
-		break;
-	default:
-		pr_err("Can't set PAD_CTRL on port %d\n", port);
-		return -EINVAL;
-	}
-
-	/* Configure a port to be directly connected to an external
-	 * PHY or MAC.
-	 */
-	switch (mode) {
-	case PHY_INTERFACE_MODE_RGMII:
-		/* RGMII mode means no delay so don't enable the delay */
-		val = QCA8K_PORT_PAD_RGMII_EN;
-		qca8k_write(priv, reg, val);
-		break;
-	case PHY_INTERFACE_MODE_RGMII_ID:
-		/* RGMII_ID needs internal delay. This is enabled through
-		 * PORT5_PAD_CTRL for all ports, rather than individual port
-		 * registers
-		 */
-		qca8k_write(priv, reg,
-			    QCA8K_PORT_PAD_RGMII_EN |
-			    QCA8K_PORT_PAD_RGMII_TX_DELAY(QCA8K_MAX_DELAY) |
-			    QCA8K_PORT_PAD_RGMII_RX_DELAY(QCA8K_MAX_DELAY));
-		qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL,
-			    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
-		break;
-	case PHY_INTERFACE_MODE_SGMII:
-		qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
-		break;
-	default:
-		pr_err("xMII mode %d not supported\n", mode);
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 static void
 qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable)
 {
@@ -639,9 +591,7 @@ static int
 qca8k_setup(struct dsa_switch *ds)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
-	phy_interface_t phy_mode = PHY_INTERFACE_MODE_NA;
 	int ret, i;
-	u32 mask;
 
 	/* Make sure that port 0 is the cpu port */
 	if (!dsa_is_cpu_port(ds, 0)) {
@@ -661,24 +611,9 @@ qca8k_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
-	/* Initialize CPU port pad mode (xMII type, delays...) */
-	ret = of_get_phy_mode(dsa_to_port(ds, QCA8K_CPU_PORT)->dn, &phy_mode);
-	if (ret) {
-		pr_err("Can't find phy-mode for master device\n");
-		return ret;
-	}
-	ret = qca8k_set_pad_ctrl(priv, QCA8K_CPU_PORT, phy_mode);
-	if (ret < 0)
-		return ret;
-
-	/* Enable CPU Port, force it to maximum bandwidth and full-duplex */
-	mask = QCA8K_PORT_STATUS_SPEED_1000 | QCA8K_PORT_STATUS_TXFLOW |
-	       QCA8K_PORT_STATUS_RXFLOW | QCA8K_PORT_STATUS_DUPLEX;
-	qca8k_write(priv, QCA8K_REG_PORT_STATUS(QCA8K_CPU_PORT), mask);
+	/* Enable CPU Port */
 	qca8k_reg_set(priv, QCA8K_REG_GLOBAL_FW_CTRL0,
 		      QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN);
-	qca8k_port_set_status(priv, QCA8K_CPU_PORT, 1);
-	priv->port_sts[QCA8K_CPU_PORT].enabled = 1;
 
 	/* Enable MIB counters */
 	qca8k_mib_init(priv);
@@ -693,10 +628,9 @@ qca8k_setup(struct dsa_switch *ds)
 		qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
 			  QCA8K_PORT_LOOKUP_MEMBER, 0);
 
-	/* Disable MAC by default on all user ports */
+	/* Disable MAC by default on all ports */
 	for (i = 1; i < QCA8K_NUM_PORTS; i++)
-		if (dsa_is_user_port(ds, i))
-			qca8k_port_set_status(priv, i, 0);
+		qca8k_port_set_status(priv, i, 0);
 
 	/* Forward all unknown frames to CPU port for Linux processing */
 	qca8k_write(priv, QCA8K_REG_GLOBAL_FW_CTRL1,
@@ -743,44 +677,222 @@ qca8k_setup(struct dsa_switch *ds)
 }
 
 static void
-qca8k_adjust_link(struct dsa_switch *ds, int port, struct phy_device *phy)
+qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
+			 const struct phylink_link_state *state)
 {
 	struct qca8k_priv *priv = ds->priv;
 	u32 reg;
 
-	/* Force fixed-link setting for CPU port, skip others. */
-	if (!phy_is_pseudo_fixed_link(phy))
+	switch (port) {
+	case 0: /* 1st CPU port */
+		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
+		    state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
+		    state->interface != PHY_INTERFACE_MODE_SGMII)
+			return;
+
+		reg = QCA8K_REG_PORT0_PAD_CTRL;
+		break;
+	case 1:
+	case 2:
+	case 3:
+	case 4:
+	case 5:
+		/* Internal PHY, nothing to do */
+		return;
+	case 6: /* 2nd CPU port / external PHY */
+		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
+		    state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
+		    state->interface != PHY_INTERFACE_MODE_SGMII &&
+		    state->interface != PHY_INTERFACE_MODE_1000BASEX)
+			return;
+
+		reg = QCA8K_REG_PORT6_PAD_CTRL;
+		break;
+	default:
+		dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
+		return;
+	}
+
+	if (port != 6 && phylink_autoneg_inband(mode)) {
+		dev_err(ds->dev, "%s: in-band negotiation unsupported\n",
+			__func__);
+		return;
+	}
+
+	switch (state->interface) {
+	case PHY_INTERFACE_MODE_RGMII:
+		/* RGMII mode means no delay so don't enable the delay */
+		qca8k_write(priv, reg, QCA8K_PORT_PAD_RGMII_EN);
+		break;
+	case PHY_INTERFACE_MODE_RGMII_ID:
+		/* RGMII_ID needs internal delay. This is enabled through
+		 * PORT5_PAD_CTRL for all ports, rather than individual port
+		 * registers
+		 */
+		qca8k_write(priv, reg,
+			    QCA8K_PORT_PAD_RGMII_EN |
+			    QCA8K_PORT_PAD_RGMII_TX_DELAY(QCA8K_MAX_DELAY) |
+			    QCA8K_PORT_PAD_RGMII_RX_DELAY(QCA8K_MAX_DELAY));
+		qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL,
+			    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
+		break;
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_1000BASEX:
+		/* Enable SGMII on the port */
+		qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
+		break;
+	default:
+		dev_err(ds->dev, "xMII mode %s not supported for port %d\n",
+			phy_modes(state->interface), port);
 		return;
+	}
+}
 
-	/* Set port speed */
-	switch (phy->speed) {
-	case 10:
-		reg = QCA8K_PORT_STATUS_SPEED_10;
+static void
+qca8k_phylink_validate(struct dsa_switch *ds, int port,
+		       unsigned long *supported,
+		       struct phylink_link_state *state)
+{
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
+
+	switch (port) {
+	case 0: /* 1st CPU port */
+		if (state->interface != PHY_INTERFACE_MODE_NA &&
+		    state->interface != PHY_INTERFACE_MODE_RGMII &&
+		    state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
+		    state->interface != PHY_INTERFACE_MODE_SGMII)
+			goto unsupported;
 		break;
-	case 100:
-		reg = QCA8K_PORT_STATUS_SPEED_100;
+	case 1:
+	case 2:
+	case 3:
+	case 4:
+	case 5:
+		/* Internal PHY */
+		if (state->interface != PHY_INTERFACE_MODE_NA &&
+		    state->interface != PHY_INTERFACE_MODE_GMII)
+			goto unsupported;
 		break;
-	case 1000:
-		reg = QCA8K_PORT_STATUS_SPEED_1000;
+	case 6: /* 2nd CPU port / external PHY */
+		if (state->interface != PHY_INTERFACE_MODE_NA &&
+		    state->interface != PHY_INTERFACE_MODE_RGMII &&
+		    state->interface != PHY_INTERFACE_MODE_RGMII_ID &&
+		    state->interface != PHY_INTERFACE_MODE_SGMII &&
+		    state->interface != PHY_INTERFACE_MODE_1000BASEX)
+			goto unsupported;
 		break;
 	default:
-		dev_dbg(priv->dev, "port%d link speed %dMbps not supported.\n",
-			port, phy->speed);
+unsupported:
+		linkmode_zero(supported);
 		return;
 	}
 
-	/* Set duplex mode */
-	if (phy->duplex == DUPLEX_FULL)
-		reg |= QCA8K_PORT_STATUS_DUPLEX;
+	phylink_set_port_modes(mask);
+	phylink_set(mask, Autoneg);
+
+	phylink_set(mask, 1000baseT_Full);
+	phylink_set(mask, 10baseT_Half);
+	phylink_set(mask, 10baseT_Full);
+	phylink_set(mask, 100baseT_Half);
+	phylink_set(mask, 100baseT_Full);
+
+	if (state->interface == PHY_INTERFACE_MODE_1000BASEX)
+		phylink_set(mask, 1000baseX_Full);
+
+	phylink_set(mask, Pause);
+	phylink_set(mask, Asym_Pause);
+
+	linkmode_and(supported, supported, mask);
+	linkmode_and(state->advertising, state->advertising, mask);
+}
+
+static int
+qca8k_phylink_mac_link_state(struct dsa_switch *ds, int port,
+			     struct phylink_link_state *state)
+{
+	struct qca8k_priv *priv = ds->priv;
+	u32 reg;
 
-	/* Force flow control */
-	if (dsa_is_cpu_port(ds, port))
-		reg |= QCA8K_PORT_STATUS_RXFLOW | QCA8K_PORT_STATUS_TXFLOW;
+	reg = qca8k_read(priv, QCA8K_REG_PORT_STATUS(port));
+
+	state->link = !!(reg & QCA8K_PORT_STATUS_LINK_UP);
+	state->an_complete = state->link;
+	state->an_enabled = !!(reg & QCA8K_PORT_STATUS_LINK_AUTO);
+	state->duplex = (reg & QCA8K_PORT_STATUS_DUPLEX) ? DUPLEX_FULL :
+							   DUPLEX_HALF;
+
+	switch (reg & QCA8K_PORT_STATUS_SPEED) {
+	case QCA8K_PORT_STATUS_SPEED_10:
+		state->speed = SPEED_10;
+		break;
+	case QCA8K_PORT_STATUS_SPEED_100:
+		state->speed = SPEED_100;
+		break;
+	case QCA8K_PORT_STATUS_SPEED_1000:
+		state->speed = SPEED_1000;
+		break;
+	default:
+		state->speed = SPEED_UNKNOWN;
+		break;
+	}
+
+	state->pause = MLO_PAUSE_NONE;
+	if (reg & QCA8K_PORT_STATUS_RXFLOW)
+		state->pause |= MLO_PAUSE_RX;
+	if (reg & QCA8K_PORT_STATUS_TXFLOW)
+		state->pause |= MLO_PAUSE_TX;
+
+	return 1;
+}
+
+static void
+qca8k_phylink_mac_link_down(struct dsa_switch *ds, int port, unsigned int mode,
+			    phy_interface_t interface)
+{
+	struct qca8k_priv *priv = ds->priv;
 
-	/* Force link down before changing MAC options */
 	qca8k_port_set_status(priv, port, 0);
+}
+
+static void
+qca8k_phylink_mac_link_up(struct dsa_switch *ds, int port, unsigned int mode,
+			  phy_interface_t interface, struct phy_device *phydev,
+			  int speed, int duplex, bool tx_pause, bool rx_pause)
+{
+	struct qca8k_priv *priv = ds->priv;
+	u32 reg;
+
+	if (phylink_autoneg_inband(mode)) {
+		reg = QCA8K_PORT_STATUS_LINK_AUTO;
+	} else {
+		switch (speed) {
+		case SPEED_10:
+			reg = QCA8K_PORT_STATUS_SPEED_10;
+			break;
+		case SPEED_100:
+			reg = QCA8K_PORT_STATUS_SPEED_100;
+			break;
+		case SPEED_1000:
+			reg = QCA8K_PORT_STATUS_SPEED_1000;
+			break;
+		default:
+			reg = QCA8K_PORT_STATUS_LINK_AUTO;
+			break;
+		}
+
+		if (duplex == DUPLEX_FULL)
+			reg |= QCA8K_PORT_STATUS_DUPLEX;
+
+		if (rx_pause || dsa_is_cpu_port(ds, port))
+			reg |= QCA8K_PORT_STATUS_RXFLOW;
+
+		if (tx_pause || dsa_is_cpu_port(ds, port))
+			reg |= QCA8K_PORT_STATUS_TXFLOW;
+	}
+
+	reg |= QCA8K_PORT_STATUS_TXMAC | QCA8K_PORT_STATUS_RXMAC;
+
 	qca8k_write(priv, QCA8K_REG_PORT_STATUS(port), reg);
-	qca8k_port_set_status(priv, port, 1);
 }
 
 static void
@@ -937,13 +1049,11 @@ qca8k_port_enable(struct dsa_switch *ds, int port,
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 
-	if (!dsa_is_user_port(ds, port))
-		return 0;
-
 	qca8k_port_set_status(priv, port, 1);
 	priv->port_sts[port].enabled = 1;
 
-	phy_support_asym_pause(phy);
+	if (dsa_is_user_port(ds, port))
+		phy_support_asym_pause(phy);
 
 	return 0;
 }
@@ -1026,7 +1136,6 @@ qca8k_get_tag_protocol(struct dsa_switch *ds, int port,
 static const struct dsa_switch_ops qca8k_switch_ops = {
 	.get_tag_protocol	= qca8k_get_tag_protocol,
 	.setup			= qca8k_setup,
-	.adjust_link            = qca8k_adjust_link,
 	.get_strings		= qca8k_get_strings,
 	.get_ethtool_stats	= qca8k_get_ethtool_stats,
 	.get_sset_count		= qca8k_get_sset_count,
@@ -1040,6 +1149,11 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
 	.port_fdb_add		= qca8k_port_fdb_add,
 	.port_fdb_del		= qca8k_port_fdb_del,
 	.port_fdb_dump		= qca8k_port_fdb_dump,
+	.phylink_validate	= qca8k_phylink_validate,
+	.phylink_mac_link_state	= qca8k_phylink_mac_link_state,
+	.phylink_mac_config	= qca8k_phylink_mac_config,
+	.phylink_mac_link_down	= qca8k_phylink_mac_link_down,
+	.phylink_mac_link_up	= qca8k_phylink_mac_link_up,
 };
 
 static int
-- 
2.20.1

[PATCH net-next v5 2/3] net: dsa: qca8k: Improve SGMII interface handling

[Jonathan McDowell]

This patch improves the handling of the SGMII interface on the QCA8K
devices. Previously the driver did no configuration of the port, even if
it was selected. We now configure it up in the appropriate
PHY/MAC/Base-X mode depending on what phylink tells us we are connected
to and ensure it is enabled.

Tested with a device where the CPU connection is RGMII (i.e. the common
current use case) + one where the CPU connection is SGMII. I don't have
any devices where the SGMII interface is brought out to something other
than the CPU.

Signed-off-by: Jonathan McDowell <noodles@earth.li>
---
 drivers/net/dsa/qca8k.c | 33 ++++++++++++++++++++++++++++++++-
 drivers/net/dsa/qca8k.h | 13 +++++++++++++
 2 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 63b84789f16b..11d1c290d90f 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -673,6 +673,9 @@ qca8k_setup(struct dsa_switch *ds)
 	/* Flush the FDB table */
 	qca8k_fdb_flush(priv);
 
+	/* We don't have interrupts for link changes, so we need to poll */
+	ds->pcs_poll = true;
+
 	return 0;
 }
 
@@ -681,7 +684,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 			 const struct phylink_link_state *state)
 {
 	struct qca8k_priv *priv = ds->priv;
-	u32 reg;
+	u32 reg, val;
 
 	switch (port) {
 	case 0: /* 1st CPU port */
@@ -740,6 +743,34 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 	case PHY_INTERFACE_MODE_1000BASEX:
 		/* Enable SGMII on the port */
 		qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
+
+		/* Enable/disable SerDes auto-negotiation as necessary */
+		val = qca8k_read(priv, QCA8K_REG_PWS);
+		if (phylink_autoneg_inband(mode))
+			val &= ~QCA8K_PWS_SERDES_AEN_DIS;
+		else
+			val |= QCA8K_PWS_SERDES_AEN_DIS;
+		qca8k_write(priv, QCA8K_REG_PWS, val);
+
+		/* Configure the SGMII parameters */
+		val = qca8k_read(priv, QCA8K_REG_SGMII_CTRL);
+
+		val |= QCA8K_SGMII_EN_PLL | QCA8K_SGMII_EN_RX |
+			QCA8K_SGMII_EN_TX | QCA8K_SGMII_EN_SD;
+
+		if (dsa_is_cpu_port(ds, port)) {
+			/* CPU port, we're talking to the CPU MAC, be a PHY */
+			val &= ~QCA8K_SGMII_MODE_CTRL_MASK;
+			val |= QCA8K_SGMII_MODE_CTRL_PHY;
+		} else if (state->interface == PHY_INTERFACE_MODE_SGMII) {
+			val &= ~QCA8K_SGMII_MODE_CTRL_MASK;
+			val |= QCA8K_SGMII_MODE_CTRL_MAC;
+		} else if (state->interface == PHY_INTERFACE_MODE_1000BASEX) {
+			val &= ~QCA8K_SGMII_MODE_CTRL_MASK;
+			val |= QCA8K_SGMII_MODE_CTRL_BASEX;
+		}
+
+		qca8k_write(priv, QCA8K_REG_SGMII_CTRL, val);
 		break;
 	default:
 		dev_err(ds->dev, "xMII mode %s not supported for port %d\n",
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 42d6ea24eb14..10ef2bca2cde 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -36,6 +36,8 @@
 #define   QCA8K_MAX_DELAY				3
 #define   QCA8K_PORT_PAD_RGMII_RX_DELAY_EN		BIT(24)
 #define   QCA8K_PORT_PAD_SGMII_EN			BIT(7)
+#define QCA8K_REG_PWS					0x010
+#define   QCA8K_PWS_SERDES_AEN_DIS			BIT(7)
 #define QCA8K_REG_MODULE_EN				0x030
 #define   QCA8K_MODULE_EN_MIB				BIT(0)
 #define QCA8K_REG_MIB					0x034
@@ -69,6 +71,7 @@
 #define   QCA8K_PORT_STATUS_LINK_UP			BIT(8)
 #define   QCA8K_PORT_STATUS_LINK_AUTO			BIT(9)
 #define   QCA8K_PORT_STATUS_LINK_PAUSE			BIT(10)
+#define   QCA8K_PORT_STATUS_FLOW_AUTO			BIT(12)
 #define QCA8K_REG_PORT_HDR_CTRL(_i)			(0x9c + (_i * 4))
 #define   QCA8K_PORT_HDR_CTRL_RX_MASK			GENMASK(3, 2)
 #define   QCA8K_PORT_HDR_CTRL_RX_S			2
@@ -77,6 +80,16 @@
 #define   QCA8K_PORT_HDR_CTRL_ALL			2
 #define   QCA8K_PORT_HDR_CTRL_MGMT			1
 #define   QCA8K_PORT_HDR_CTRL_NONE			0
+#define QCA8K_REG_SGMII_CTRL				0x0e0
+#define   QCA8K_SGMII_EN_PLL				BIT(1)
+#define   QCA8K_SGMII_EN_RX				BIT(2)
+#define   QCA8K_SGMII_EN_TX				BIT(3)
+#define   QCA8K_SGMII_EN_SD				BIT(4)
+#define   QCA8K_SGMII_CLK125M_DELAY			BIT(7)
+#define   QCA8K_SGMII_MODE_CTRL_MASK			(BIT(22) | BIT(23))
+#define   QCA8K_SGMII_MODE_CTRL_BASEX			(0 << 22)
+#define   QCA8K_SGMII_MODE_CTRL_PHY			(1 << 22)
+#define   QCA8K_SGMII_MODE_CTRL_MAC			(2 << 22)
 
 /* EEE control registers */
 #define QCA8K_REG_EEE_CTRL				0x100
-- 
2.20.1

[PATCH net-next v5 3/3] net: dsa: qca8k: Minor comment spelling fix

[Jonathan McDowell]

Signed-off-by: Jonathan McDowell <noodles@earth.li>
---
 drivers/net/dsa/qca8k.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 11d1c290d90f..4acad5fa0c84 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -647,7 +647,7 @@ qca8k_setup(struct dsa_switch *ds)
 				  QCA8K_PORT_LOOKUP_MEMBER, dsa_user_ports(ds));
 		}
 
-		/* Invividual user ports get connected to CPU port only */
+		/* Individual user ports get connected to CPU port only */
 		if (dsa_is_user_port(ds, i)) {
 			int shift = 16 * (i % 2);
 
-- 
2.20.1

Re: [PATCH net-next v5 0/3] net: dsa: qca8k: Improve SGMII interface handling

[David Miller]

From: Jonathan McDowell <noodles@earth.li>
Date: Sat, 20 Jun 2020 11:30:03 +0100

> This 3 patch series migrates the qca8k switch driver over to PHYLINK,
> and then adds the SGMII clean-ups (i.e. the missing initialisation) on
> top of that as a second patch. The final patch is a simple spelling fix
> in a comment.
> 
> As before, tested with a device where the CPU connection is RGMII (i.e.
> the common current use case) + one where the CPU connection is SGMII. I
> don't have any devices where the SGMII interface is brought out to
> something other than the CPU.
 ...

Series applied, thanks.

Re: [PATCH 2/2] net: dsa: qca8k: introduce SGMII configuration options

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 3704 bytes --]

Hi Jonathan,

url:    https://github.com/0day-ci/linux/commits/Jonathan-McDowell/net-dsa-qca8k-Add-SGMII-configuration-options/20200606-021254
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: i386-randconfig-m021-20200618 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/net/dsa/qca8k.c:438 qca8k_setup_sgmii() error: uninitialized symbol 'mode'.

# https://github.com/0day-ci/linux/commit/27dd896d27e5048d2c402879fb04f6e23536ea72
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 27dd896d27e5048d2c402879fb04f6e23536ea72
vim +/mode +438 drivers/net/dsa/qca8k.c

27dd896d27e5048 Jonathan McDowell 2020-06-05  421  static int
27dd896d27e5048 Jonathan McDowell 2020-06-05  422  qca8k_setup_sgmii(struct qca8k_priv *priv)
27dd896d27e5048 Jonathan McDowell 2020-06-05  423  {
27dd896d27e5048 Jonathan McDowell 2020-06-05  424  	const char *mode;
                                                                    ^^^^

27dd896d27e5048 Jonathan McDowell 2020-06-05  425  	u32 val;
27dd896d27e5048 Jonathan McDowell 2020-06-05  426  
27dd896d27e5048 Jonathan McDowell 2020-06-05  427  	val = qca8k_read(priv, QCA8K_REG_SGMII_CTRL);
27dd896d27e5048 Jonathan McDowell 2020-06-05  428  
27dd896d27e5048 Jonathan McDowell 2020-06-05  429  	val |= QCA8K_SGMII_EN_PLL | QCA8K_SGMII_EN_RX |
27dd896d27e5048 Jonathan McDowell 2020-06-05  430  		QCA8K_SGMII_EN_TX | QCA8K_SGMII_EN_SD;
27dd896d27e5048 Jonathan McDowell 2020-06-05  431  
27dd896d27e5048 Jonathan McDowell 2020-06-05  432  	if (of_property_read_bool(priv->dev->of_node, "sgmii-delay"))
27dd896d27e5048 Jonathan McDowell 2020-06-05  433  		val |= QCA8K_SGMII_CLK125M_DELAY;
27dd896d27e5048 Jonathan McDowell 2020-06-05  434  
27dd896d27e5048 Jonathan McDowell 2020-06-05  435  	if (of_property_read_string(priv->dev->of_node, "sgmii-mode", &mode)) {
                                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This if condition is reversed.

27dd896d27e5048 Jonathan McDowell 2020-06-05  436  		val &= ~QCA8K_SGMII_MODE_CTRL_MASK;
27dd896d27e5048 Jonathan McDowell 2020-06-05  437  
27dd896d27e5048 Jonathan McDowell 2020-06-05 @438  		if (!strcasecmp(mode, "basex")) {
27dd896d27e5048 Jonathan McDowell 2020-06-05  439  			val |= QCA8K_SGMII_MODE_CTRL_BASEX;
27dd896d27e5048 Jonathan McDowell 2020-06-05  440  		} else if (!strcasecmp(mode, "mac")) {
27dd896d27e5048 Jonathan McDowell 2020-06-05  441  			val |= QCA8K_SGMII_MODE_CTRL_MAC;
27dd896d27e5048 Jonathan McDowell 2020-06-05  442  		} else if (!strcasecmp(mode, "phy")) {
27dd896d27e5048 Jonathan McDowell 2020-06-05  443  			val |= QCA8K_SGMII_MODE_CTRL_PHY;
27dd896d27e5048 Jonathan McDowell 2020-06-05  444  		} else {
27dd896d27e5048 Jonathan McDowell 2020-06-05  445  			pr_err("Unrecognised SGMII mode %s\n", mode);
27dd896d27e5048 Jonathan McDowell 2020-06-05  446  			return -EINVAL;
27dd896d27e5048 Jonathan McDowell 2020-06-05  447  		}
27dd896d27e5048 Jonathan McDowell 2020-06-05  448  	}
27dd896d27e5048 Jonathan McDowell 2020-06-05  449  
27dd896d27e5048 Jonathan McDowell 2020-06-05  450  	qca8k_write(priv, QCA8K_REG_SGMII_CTRL, val);
27dd896d27e5048 Jonathan McDowell 2020-06-05  451  
27dd896d27e5048 Jonathan McDowell 2020-06-05  452  	return 0;
27dd896d27e5048 Jonathan McDowell 2020-06-05  453  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35248 bytes --]

Re: [PATCH 2/2] net: dsa: qca8k: introduce SGMII configuration options

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 3768 bytes --]

Hi Jonathan,

url:    https://github.com/0day-ci/linux/commits/Jonathan-McDowell/net-dsa-qca8k-Add-SGMII-configuration-options/20200606-021254
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: i386-randconfig-m021-20200618 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/net/dsa/qca8k.c:438 qca8k_setup_sgmii() error: uninitialized symbol 'mode'.

# https://github.com/0day-ci/linux/commit/27dd896d27e5048d2c402879fb04f6e23536ea72
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 27dd896d27e5048d2c402879fb04f6e23536ea72
vim +/mode +438 drivers/net/dsa/qca8k.c

27dd896d27e5048 Jonathan McDowell 2020-06-05  421  static int
27dd896d27e5048 Jonathan McDowell 2020-06-05  422  qca8k_setup_sgmii(struct qca8k_priv *priv)
27dd896d27e5048 Jonathan McDowell 2020-06-05  423  {
27dd896d27e5048 Jonathan McDowell 2020-06-05  424  	const char *mode;
                                                                    ^^^^

27dd896d27e5048 Jonathan McDowell 2020-06-05  425  	u32 val;
27dd896d27e5048 Jonathan McDowell 2020-06-05  426  
27dd896d27e5048 Jonathan McDowell 2020-06-05  427  	val = qca8k_read(priv, QCA8K_REG_SGMII_CTRL);
27dd896d27e5048 Jonathan McDowell 2020-06-05  428  
27dd896d27e5048 Jonathan McDowell 2020-06-05  429  	val |= QCA8K_SGMII_EN_PLL | QCA8K_SGMII_EN_RX |
27dd896d27e5048 Jonathan McDowell 2020-06-05  430  		QCA8K_SGMII_EN_TX | QCA8K_SGMII_EN_SD;
27dd896d27e5048 Jonathan McDowell 2020-06-05  431  
27dd896d27e5048 Jonathan McDowell 2020-06-05  432  	if (of_property_read_bool(priv->dev->of_node, "sgmii-delay"))
27dd896d27e5048 Jonathan McDowell 2020-06-05  433  		val |= QCA8K_SGMII_CLK125M_DELAY;
27dd896d27e5048 Jonathan McDowell 2020-06-05  434  
27dd896d27e5048 Jonathan McDowell 2020-06-05  435  	if (of_property_read_string(priv->dev->of_node, "sgmii-mode", &mode)) {
                                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This if condition is reversed.

27dd896d27e5048 Jonathan McDowell 2020-06-05  436  		val &= ~QCA8K_SGMII_MODE_CTRL_MASK;
27dd896d27e5048 Jonathan McDowell 2020-06-05  437  
27dd896d27e5048 Jonathan McDowell 2020-06-05 @438  		if (!strcasecmp(mode, "basex")) {
27dd896d27e5048 Jonathan McDowell 2020-06-05  439  			val |= QCA8K_SGMII_MODE_CTRL_BASEX;
27dd896d27e5048 Jonathan McDowell 2020-06-05  440  		} else if (!strcasecmp(mode, "mac")) {
27dd896d27e5048 Jonathan McDowell 2020-06-05  441  			val |= QCA8K_SGMII_MODE_CTRL_MAC;
27dd896d27e5048 Jonathan McDowell 2020-06-05  442  		} else if (!strcasecmp(mode, "phy")) {
27dd896d27e5048 Jonathan McDowell 2020-06-05  443  			val |= QCA8K_SGMII_MODE_CTRL_PHY;
27dd896d27e5048 Jonathan McDowell 2020-06-05  444  		} else {
27dd896d27e5048 Jonathan McDowell 2020-06-05  445  			pr_err("Unrecognised SGMII mode %s\n", mode);
27dd896d27e5048 Jonathan McDowell 2020-06-05  446  			return -EINVAL;
27dd896d27e5048 Jonathan McDowell 2020-06-05  447  		}
27dd896d27e5048 Jonathan McDowell 2020-06-05  448  	}
27dd896d27e5048 Jonathan McDowell 2020-06-05  449  
27dd896d27e5048 Jonathan McDowell 2020-06-05  450  	qca8k_write(priv, QCA8K_REG_SGMII_CTRL, val);
27dd896d27e5048 Jonathan McDowell 2020-06-05  451  
27dd896d27e5048 Jonathan McDowell 2020-06-05  452  	return 0;
27dd896d27e5048 Jonathan McDowell 2020-06-05  453  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 35248 bytes --]

Re: [PATCH 2/2] net: dsa: qca8k: introduce SGMII configuration options

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 3768 bytes --]

Hi Jonathan,

url:    https://github.com/0day-ci/linux/commits/Jonathan-McDowell/net-dsa-qca8k-Add-SGMII-configuration-options/20200606-021254
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: i386-randconfig-m021-20200618 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/net/dsa/qca8k.c:438 qca8k_setup_sgmii() error: uninitialized symbol 'mode'.

# https://github.com/0day-ci/linux/commit/27dd896d27e5048d2c402879fb04f6e23536ea72
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 27dd896d27e5048d2c402879fb04f6e23536ea72
vim +/mode +438 drivers/net/dsa/qca8k.c

27dd896d27e5048 Jonathan McDowell 2020-06-05  421  static int
27dd896d27e5048 Jonathan McDowell 2020-06-05  422  qca8k_setup_sgmii(struct qca8k_priv *priv)
27dd896d27e5048 Jonathan McDowell 2020-06-05  423  {
27dd896d27e5048 Jonathan McDowell 2020-06-05  424  	const char *mode;
                                                                    ^^^^

27dd896d27e5048 Jonathan McDowell 2020-06-05  425  	u32 val;
27dd896d27e5048 Jonathan McDowell 2020-06-05  426  
27dd896d27e5048 Jonathan McDowell 2020-06-05  427  	val = qca8k_read(priv, QCA8K_REG_SGMII_CTRL);
27dd896d27e5048 Jonathan McDowell 2020-06-05  428  
27dd896d27e5048 Jonathan McDowell 2020-06-05  429  	val |= QCA8K_SGMII_EN_PLL | QCA8K_SGMII_EN_RX |
27dd896d27e5048 Jonathan McDowell 2020-06-05  430  		QCA8K_SGMII_EN_TX | QCA8K_SGMII_EN_SD;
27dd896d27e5048 Jonathan McDowell 2020-06-05  431  
27dd896d27e5048 Jonathan McDowell 2020-06-05  432  	if (of_property_read_bool(priv->dev->of_node, "sgmii-delay"))
27dd896d27e5048 Jonathan McDowell 2020-06-05  433  		val |= QCA8K_SGMII_CLK125M_DELAY;
27dd896d27e5048 Jonathan McDowell 2020-06-05  434  
27dd896d27e5048 Jonathan McDowell 2020-06-05  435  	if (of_property_read_string(priv->dev->of_node, "sgmii-mode", &mode)) {
                                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This if condition is reversed.

27dd896d27e5048 Jonathan McDowell 2020-06-05  436  		val &= ~QCA8K_SGMII_MODE_CTRL_MASK;
27dd896d27e5048 Jonathan McDowell 2020-06-05  437  
27dd896d27e5048 Jonathan McDowell 2020-06-05 @438  		if (!strcasecmp(mode, "basex")) {
27dd896d27e5048 Jonathan McDowell 2020-06-05  439  			val |= QCA8K_SGMII_MODE_CTRL_BASEX;
27dd896d27e5048 Jonathan McDowell 2020-06-05  440  		} else if (!strcasecmp(mode, "mac")) {
27dd896d27e5048 Jonathan McDowell 2020-06-05  441  			val |= QCA8K_SGMII_MODE_CTRL_MAC;
27dd896d27e5048 Jonathan McDowell 2020-06-05  442  		} else if (!strcasecmp(mode, "phy")) {
27dd896d27e5048 Jonathan McDowell 2020-06-05  443  			val |= QCA8K_SGMII_MODE_CTRL_PHY;
27dd896d27e5048 Jonathan McDowell 2020-06-05  444  		} else {
27dd896d27e5048 Jonathan McDowell 2020-06-05  445  			pr_err("Unrecognised SGMII mode %s\n", mode);
27dd896d27e5048 Jonathan McDowell 2020-06-05  446  			return -EINVAL;
27dd896d27e5048 Jonathan McDowell 2020-06-05  447  		}
27dd896d27e5048 Jonathan McDowell 2020-06-05  448  	}
27dd896d27e5048 Jonathan McDowell 2020-06-05  449  
27dd896d27e5048 Jonathan McDowell 2020-06-05  450  	qca8k_write(priv, QCA8K_REG_SGMII_CTRL, val);
27dd896d27e5048 Jonathan McDowell 2020-06-05  451  
27dd896d27e5048 Jonathan McDowell 2020-06-05  452  	return 0;
27dd896d27e5048 Jonathan McDowell 2020-06-05  453  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 35248 bytes --]

Re: [PATCH 2/2] net: dsa: qca8k: introduce SGMII configuration options

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4627 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <8ddd76e484e1bedd12c87ea0810826b60e004a65.1591380105.git.noodles@earth.li>
References: <8ddd76e484e1bedd12c87ea0810826b60e004a65.1591380105.git.noodles@earth.li>
TO: Jonathan McDowell <noodles@earth.li>
TO: Andrew Lunn <andrew@lunn.ch>
TO: Vivien Didelot <vivien.didelot@gmail.com>
TO: Florian Fainelli <f.fainelli@gmail.com>
TO: "David S. Miller" <davem@davemloft.net>
CC: netdev(a)vger.kernel.org
TO: Jakub Kicinski <kuba@kernel.org>
TO: linux-kernel(a)vger.kernel.org

Hi Jonathan,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on sparc-next/master net-next/master net/master linus/master v5.8-rc1 next-20200618]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jonathan-McDowell/net-dsa-qca8k-Add-SGMII-configuration-options/20200606-021254
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
:::::: branch date: 13 days ago
:::::: commit date: 13 days ago
config: i386-randconfig-m021-20200618 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/net/dsa/qca8k.c:438 qca8k_setup_sgmii() error: uninitialized symbol 'mode'.

# https://github.com/0day-ci/linux/commit/27dd896d27e5048d2c402879fb04f6e23536ea72
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 27dd896d27e5048d2c402879fb04f6e23536ea72
vim +/mode +438 drivers/net/dsa/qca8k.c

6b93fb46480a9cf John Crispin      2016-09-15  420  
27dd896d27e5048 Jonathan McDowell 2020-06-05  421  static int
27dd896d27e5048 Jonathan McDowell 2020-06-05  422  qca8k_setup_sgmii(struct qca8k_priv *priv)
27dd896d27e5048 Jonathan McDowell 2020-06-05  423  {
27dd896d27e5048 Jonathan McDowell 2020-06-05  424  	const char *mode;
27dd896d27e5048 Jonathan McDowell 2020-06-05  425  	u32 val;
27dd896d27e5048 Jonathan McDowell 2020-06-05  426  
27dd896d27e5048 Jonathan McDowell 2020-06-05  427  	val = qca8k_read(priv, QCA8K_REG_SGMII_CTRL);
27dd896d27e5048 Jonathan McDowell 2020-06-05  428  
27dd896d27e5048 Jonathan McDowell 2020-06-05  429  	val |= QCA8K_SGMII_EN_PLL | QCA8K_SGMII_EN_RX |
27dd896d27e5048 Jonathan McDowell 2020-06-05  430  		QCA8K_SGMII_EN_TX | QCA8K_SGMII_EN_SD;
27dd896d27e5048 Jonathan McDowell 2020-06-05  431  
27dd896d27e5048 Jonathan McDowell 2020-06-05  432  	if (of_property_read_bool(priv->dev->of_node, "sgmii-delay"))
27dd896d27e5048 Jonathan McDowell 2020-06-05  433  		val |= QCA8K_SGMII_CLK125M_DELAY;
27dd896d27e5048 Jonathan McDowell 2020-06-05  434  
27dd896d27e5048 Jonathan McDowell 2020-06-05  435  	if (of_property_read_string(priv->dev->of_node, "sgmii-mode", &mode)) {
27dd896d27e5048 Jonathan McDowell 2020-06-05  436  		val &= ~QCA8K_SGMII_MODE_CTRL_MASK;
27dd896d27e5048 Jonathan McDowell 2020-06-05  437  
27dd896d27e5048 Jonathan McDowell 2020-06-05 @438  		if (!strcasecmp(mode, "basex")) {
27dd896d27e5048 Jonathan McDowell 2020-06-05  439  			val |= QCA8K_SGMII_MODE_CTRL_BASEX;
27dd896d27e5048 Jonathan McDowell 2020-06-05  440  		} else if (!strcasecmp(mode, "mac")) {
27dd896d27e5048 Jonathan McDowell 2020-06-05  441  			val |= QCA8K_SGMII_MODE_CTRL_MAC;
27dd896d27e5048 Jonathan McDowell 2020-06-05  442  		} else if (!strcasecmp(mode, "phy")) {
27dd896d27e5048 Jonathan McDowell 2020-06-05  443  			val |= QCA8K_SGMII_MODE_CTRL_PHY;
27dd896d27e5048 Jonathan McDowell 2020-06-05  444  		} else {
27dd896d27e5048 Jonathan McDowell 2020-06-05  445  			pr_err("Unrecognised SGMII mode %s\n", mode);
27dd896d27e5048 Jonathan McDowell 2020-06-05  446  			return -EINVAL;
27dd896d27e5048 Jonathan McDowell 2020-06-05  447  		}
27dd896d27e5048 Jonathan McDowell 2020-06-05  448  	}
27dd896d27e5048 Jonathan McDowell 2020-06-05  449  
27dd896d27e5048 Jonathan McDowell 2020-06-05  450  	qca8k_write(priv, QCA8K_REG_SGMII_CTRL, val);
27dd896d27e5048 Jonathan McDowell 2020-06-05  451  
27dd896d27e5048 Jonathan McDowell 2020-06-05  452  	return 0;
27dd896d27e5048 Jonathan McDowell 2020-06-05  453  }
27dd896d27e5048 Jonathan McDowell 2020-06-05  454  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 35248 bytes --]