| Message ID | 05dba86946541267e64438c2001aaeea16916391.1592047530.git.noodles@earth.li |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: dsa: qca8k: Improve SGMII interface handling | expand |
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 >
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.
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
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(-)