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 | 28 +++++++++++++++++++++++++++-
drivers/net/dsa/qca8k.h | 13 +++++++++++++
2 files changed, 40 insertions(+), 1 deletion(-)
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index dcd9e8fa99b6..33e62598289e 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -681,7 +681,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
const struct phylink_link_state *state)
{
struct qca8k_priv *priv = ds->priv;
- u32 reg;
+ u32 reg, val;
switch (port) {
case 0: /* 1st CPU port */
@@ -740,6 +740,32 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
case PHY_INTERFACE_MODE_1000BASEX:
/* Enable SGMII on the port */
qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
+
+ /* Enable/disable SerDes auto-negotiation as necessary */
+ val = qca8k_read(priv, QCA8K_REG_PWS);
+ if (phylink_autoneg_inband(mode))
+ val &= ~QCA8K_PWS_SERDES_AEN_DIS;
+ else
+ val |= QCA8K_PWS_SERDES_AEN_DIS;
+ qca8k_write(priv, QCA8K_REG_PWS, val);
+
+ /* Configure the SGMII parameters */
+ val = qca8k_read(priv, QCA8K_REG_SGMII_CTRL);
+
+ val |= QCA8K_SGMII_EN_PLL | QCA8K_SGMII_EN_RX |
+ QCA8K_SGMII_EN_TX | QCA8K_SGMII_EN_SD;
+
+ val &= ~QCA8K_SGMII_MODE_CTRL_MASK;
+ if (dsa_is_cpu_port(ds, port)) {
+ /* CPU port, we're talking to the CPU MAC, be a PHY */
+ val |= QCA8K_SGMII_MODE_CTRL_PHY;
+ } else if (state->interface == PHY_INTERFACE_MODE_SGMII) {
+ val |= QCA8K_SGMII_MODE_CTRL_MAC;
+ } else {
+ val |= QCA8K_SGMII_MODE_CTRL_BASEX;
+ }
+
+ qca8k_write(priv, QCA8K_REG_SGMII_CTRL, val);
break;
default:
dev_err(ds->dev, "xMII mode %s not supported for port %d\n",
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 42d6ea24eb14..10ef2bca2cde 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -36,6 +36,8 @@
#define QCA8K_MAX_DELAY 3
#define QCA8K_PORT_PAD_RGMII_RX_DELAY_EN BIT(24)
#define QCA8K_PORT_PAD_SGMII_EN BIT(7)
+#define QCA8K_REG_PWS 0x010
+#define QCA8K_PWS_SERDES_AEN_DIS BIT(7)
#define QCA8K_REG_MODULE_EN 0x030
#define QCA8K_MODULE_EN_MIB BIT(0)
#define QCA8K_REG_MIB 0x034
@@ -69,6 +71,7 @@
#define QCA8K_PORT_STATUS_LINK_UP BIT(8)
#define QCA8K_PORT_STATUS_LINK_AUTO BIT(9)
#define QCA8K_PORT_STATUS_LINK_PAUSE BIT(10)
+#define QCA8K_PORT_STATUS_FLOW_AUTO BIT(12)
#define QCA8K_REG_PORT_HDR_CTRL(_i) (0x9c + (_i * 4))
#define QCA8K_PORT_HDR_CTRL_RX_MASK GENMASK(3, 2)
#define QCA8K_PORT_HDR_CTRL_RX_S 2
@@ -77,6 +80,16 @@
#define QCA8K_PORT_HDR_CTRL_ALL 2
#define QCA8K_PORT_HDR_CTRL_MGMT 1
#define QCA8K_PORT_HDR_CTRL_NONE 0
+#define QCA8K_REG_SGMII_CTRL 0x0e0
+#define QCA8K_SGMII_EN_PLL BIT(1)
+#define QCA8K_SGMII_EN_RX BIT(2)
+#define QCA8K_SGMII_EN_TX BIT(3)
+#define QCA8K_SGMII_EN_SD BIT(4)
+#define QCA8K_SGMII_CLK125M_DELAY BIT(7)
+#define QCA8K_SGMII_MODE_CTRL_MASK (BIT(22) | BIT(23))
+#define QCA8K_SGMII_MODE_CTRL_BASEX (0 << 22)
+#define QCA8K_SGMII_MODE_CTRL_PHY (1 << 22)
+#define QCA8K_SGMII_MODE_CTRL_MAC (2 << 22)
/* EEE control registers */
#define QCA8K_REG_EEE_CTRL 0x100
--
2.20.1
On 6/10/2020 12:15 PM, Jonathan McDowell wrote:
> This patch improves the handling of the SGMII interface on the QCA8K
> devices. Previously the driver did no configuration of the port, even if
> it was selected. We now configure it up in the appropriate
> PHY/MAC/Base-X mode depending on what phylink tells us we are connected
> to and ensure it is enabled.
>
> Tested with a device where the CPU connection is RGMII (i.e. the common
> current use case) + one where the CPU connection is SGMII. I don't have
> any devices where the SGMII interface is brought out to something other
> than the CPU.
>
> Signed-off-by: Jonathan McDowell <[email protected]>
> ---
> drivers/net/dsa/qca8k.c | 28 +++++++++++++++++++++++++++-
> drivers/net/dsa/qca8k.h | 13 +++++++++++++
> 2 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index dcd9e8fa99b6..33e62598289e 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -681,7 +681,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> const struct phylink_link_state *state)
> {
> struct qca8k_priv *priv = ds->priv;
> - u32 reg;
> + u32 reg, val;
>
> switch (port) {
> case 0: /* 1st CPU port */
> @@ -740,6 +740,32 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> case PHY_INTERFACE_MODE_1000BASEX:
> /* Enable SGMII on the port */
> qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
> +
> + /* Enable/disable SerDes auto-negotiation as necessary */
> + val = qca8k_read(priv, QCA8K_REG_PWS);
> + if (phylink_autoneg_inband(mode))
> + val &= ~QCA8K_PWS_SERDES_AEN_DIS;
> + else
> + val |= QCA8K_PWS_SERDES_AEN_DIS;
> + qca8k_write(priv, QCA8K_REG_PWS, val);
> +
> + /* Configure the SGMII parameters */
> + val = qca8k_read(priv, QCA8K_REG_SGMII_CTRL);
> +
> + val |= QCA8K_SGMII_EN_PLL | QCA8K_SGMII_EN_RX |
> + QCA8K_SGMII_EN_TX | QCA8K_SGMII_EN_SD;
> +
> + val &= ~QCA8K_SGMII_MODE_CTRL_MASK;
> + if (dsa_is_cpu_port(ds, port)) {
> + /* CPU port, we're talking to the CPU MAC, be a PHY */
> + val |= QCA8K_SGMII_MODE_CTRL_PHY;
Since port 6 can be interfaced to an external PHY, do not you have to
differentiate here whether this port is connected to an actual PHY,
versus connected to a MAC? You should be able to use mode == MLO_AN_PHY
to differentiate that case from the others.
> + } else if (state->interface == PHY_INTERFACE_MODE_SGMII) {
> + val |= QCA8K_SGMII_MODE_CTRL_MAC;
> + } else {
> + val |= QCA8K_SGMII_MODE_CTRL_BASEX;
Better make this explicit and check for PHY_INTERFACE_MODE_1000BASEX,
even if those are the only two possible values covered by this part of
the case statement.
--
Florian
On Wed, Jun 10, 2020 at 08:15:13PM +0100, Jonathan McDowell wrote:
> This patch improves the handling of the SGMII interface on the QCA8K
> devices. Previously the driver did no configuration of the port, even if
> it was selected. We now configure it up in the appropriate
> PHY/MAC/Base-X mode depending on what phylink tells us we are connected
> to and ensure it is enabled.
>
> Tested with a device where the CPU connection is RGMII (i.e. the common
> current use case) + one where the CPU connection is SGMII. I don't have
> any devices where the SGMII interface is brought out to something other
> than the CPU.
>
> Signed-off-by: Jonathan McDowell <[email protected]>
> ---
> drivers/net/dsa/qca8k.c | 28 +++++++++++++++++++++++++++-
> drivers/net/dsa/qca8k.h | 13 +++++++++++++
> 2 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index dcd9e8fa99b6..33e62598289e 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -681,7 +681,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> const struct phylink_link_state *state)
> {
> struct qca8k_priv *priv = ds->priv;
> - u32 reg;
> + u32 reg, val;
>
> switch (port) {
> case 0: /* 1st CPU port */
> @@ -740,6 +740,32 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> case PHY_INTERFACE_MODE_1000BASEX:
> /* Enable SGMII on the port */
> qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
> +
> + /* Enable/disable SerDes auto-negotiation as necessary */
> + val = qca8k_read(priv, QCA8K_REG_PWS);
> + if (phylink_autoneg_inband(mode))
> + val &= ~QCA8K_PWS_SERDES_AEN_DIS;
> + else
> + val |= QCA8K_PWS_SERDES_AEN_DIS;
> + qca8k_write(priv, QCA8K_REG_PWS, val);
> +
> + /* Configure the SGMII parameters */
> + val = qca8k_read(priv, QCA8K_REG_SGMII_CTRL);
> +
> + val |= QCA8K_SGMII_EN_PLL | QCA8K_SGMII_EN_RX |
> + QCA8K_SGMII_EN_TX | QCA8K_SGMII_EN_SD;
> +
> + val &= ~QCA8K_SGMII_MODE_CTRL_MASK;
> + if (dsa_is_cpu_port(ds, port)) {
> + /* CPU port, we're talking to the CPU MAC, be a PHY */
> + val |= QCA8K_SGMII_MODE_CTRL_PHY;
> + } else if (state->interface == PHY_INTERFACE_MODE_SGMII) {
> + val |= QCA8K_SGMII_MODE_CTRL_MAC;
> + } else {
> + val |= QCA8K_SGMII_MODE_CTRL_BASEX;
> + }
> +
> + qca8k_write(priv, QCA8K_REG_SGMII_CTRL, val);
Ah, here it is! Hmm, I suppose as the two patches will be applied
together, it's fine to split it like this.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 503kbps up
On Wed, Jun 10, 2020 at 08:31:11PM -0700, Florian Fainelli wrote:
> On 6/10/2020 12:15 PM, Jonathan McDowell wrote:
> > This patch improves the handling of the SGMII interface on the QCA8K
> > devices. Previously the driver did no configuration of the port, even if
> > it was selected. We now configure it up in the appropriate
> > PHY/MAC/Base-X mode depending on what phylink tells us we are connected
> > to and ensure it is enabled.
> >
> > Tested with a device where the CPU connection is RGMII (i.e. the common
> > current use case) + one where the CPU connection is SGMII. I don't have
> > any devices where the SGMII interface is brought out to something other
> > than the CPU.
> >
> > Signed-off-by: Jonathan McDowell <[email protected]>
> > ---
> > drivers/net/dsa/qca8k.c | 28 +++++++++++++++++++++++++++-
> > drivers/net/dsa/qca8k.h | 13 +++++++++++++
> > 2 files changed, 40 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index dcd9e8fa99b6..33e62598289e 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -681,7 +681,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> > const struct phylink_link_state *state)
> > {
> > struct qca8k_priv *priv = ds->priv;
> > - u32 reg;
> > + u32 reg, val;
> >
> > switch (port) {
> > case 0: /* 1st CPU port */
> > @@ -740,6 +740,32 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> > case PHY_INTERFACE_MODE_1000BASEX:
> > /* Enable SGMII on the port */
> > qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
> > +
> > + /* Enable/disable SerDes auto-negotiation as necessary */
> > + val = qca8k_read(priv, QCA8K_REG_PWS);
> > + if (phylink_autoneg_inband(mode))
> > + val &= ~QCA8K_PWS_SERDES_AEN_DIS;
> > + else
> > + val |= QCA8K_PWS_SERDES_AEN_DIS;
> > + qca8k_write(priv, QCA8K_REG_PWS, val);
> > +
> > + /* Configure the SGMII parameters */
> > + val = qca8k_read(priv, QCA8K_REG_SGMII_CTRL);
> > +
> > + val |= QCA8K_SGMII_EN_PLL | QCA8K_SGMII_EN_RX |
> > + QCA8K_SGMII_EN_TX | QCA8K_SGMII_EN_SD;
> > +
> > + val &= ~QCA8K_SGMII_MODE_CTRL_MASK;
> > + if (dsa_is_cpu_port(ds, port)) {
> > + /* CPU port, we're talking to the CPU MAC, be a PHY */
> > + val |= QCA8K_SGMII_MODE_CTRL_PHY;
>
> Since port 6 can be interfaced to an external PHY, do not you have to
> differentiate here whether this port is connected to an actual PHY,
> versus connected to a MAC? You should be able to use mode == MLO_AN_PHY
> to differentiate that case from the others.
I don't think MLO_AN_PHY is sufficient? If it's a fixed link we'll have
MLO_AN_FIXED and that could be talking to a PHY?
The logic I've gone for is assuming that a port hooked up to the CPU
should look like a PHY, and otherwise we're hooked up to a PHY so we're
acting as a MAC. That means we don't cope with the situation that we're
hooked up to something that isn't the CPU but wants us to look like a
PHY, but I don't think we have any current way to describe that.
> > + } else if (state->interface == PHY_INTERFACE_MODE_SGMII) {
> > + val |= QCA8K_SGMII_MODE_CTRL_MAC;
> > + } else {
> > + val |= QCA8K_SGMII_MODE_CTRL_BASEX;
>
> Better make this explicit and check for PHY_INTERFACE_MODE_1000BASEX,
> even if those are the only two possible values covered by this part of
> the case statement.
Sure. I'll move the mask inside the if block too in that case, so we
don't change the setting if we get fed something invalid.
J.
--
Beware of programmers carrying screwdrivers.