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
This patch documents the qca8k's SGMII related properties that allow
configuration of the SGMII port.
Signed-off-by: Jonathan McDowell <[email protected]>
---
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
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 <[email protected]>
---
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
On Fri, 5 Jun 2020 19:10:58 +0100
Jonathan McDowell <[email protected]> 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 <[email protected]>
> ---
> 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
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
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.
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
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.
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
> > > > 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
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.
\- |
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 <[email protected]>
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
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
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
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.
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
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 <[email protected]>
---
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
On Sat, 13 Jun 2020 at 14:32, Jonathan McDowell <[email protected]> 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 <[email protected]>
> ---
> 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
>
On Sat, Jun 13, 2020 at 11:10:49PM +0300, Vladimir Oltean wrote:
> On Sat, 13 Jun 2020 at 14:32, Jonathan McDowell <[email protected]> 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 <[email protected]>
> > ---
> > 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.
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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 [] [
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 <[email protected]>
Reported-by: Dan Carpenter <[email protected]>
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/[email protected]
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
Update the driver to use the new PHYLINK callbacks, removing the
legacy adjust_link callback.
Signed-off-by: Jonathan McDowell <[email protected]>
---
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
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 <[email protected]>
---
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
Signed-off-by: Jonathan McDowell <[email protected]>
---
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
From: Jonathan McDowell <[email protected]>
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.