[PATCH 0/2] net: dsa: lantiq_gswip: two fixes for -net/-stable

[PATCH 0/2] net: dsa: lantiq_gswip: two fixes for -net/-stable

[Martin Blumenstingl]

While testing the lantiq_gswip driver in OpenWrt at least one board had
a non-working Ethernet port connected to an internal 100Mbit/s PHY22F
GPHY. The problem which could be observed:
- the PHY would detect the link just fine
- ethtool stats would see the TX counter rise
- the RX counter in ethtool was stuck at zero

It turns out that two independent patches are needed to fix this:
- first we need to enable the MII data lines also for internal PHYs
- second we need to program the GSWIP_MII_CFG registers for all ports
  except the CPU port

These two patches have also been tested by back-porting them on top of
Linux 5.4.86 in OpenWrt.

Special thanks to Hauke for debugging and brainstorming this on IRC
with me!


Martin Blumenstingl (2):
  net: dsa: lantiq_gswip: Enable GSWIP_MII_CFG_EN also for internal PHYs
  net: dsa: lantiq_gswip: Fix GSWIP_MII_CFG(p) register access

 drivers/net/dsa/lantiq_gswip.c | 27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)

-- 
2.30.0

[PATCH 1/2] net: dsa: lantiq_gswip: Enable GSWIP_MII_CFG_EN also for internal PHYs

[Martin Blumenstingl]

Enable GSWIP_MII_CFG_EN also for internal PHYs to make traffic flow.
Without this the PHY link is detected properly and ethtool statistics
for TX are increasing but there's no RX traffic coming in.

Fixes: 14fceff4771e51 ("net: dsa: Add Lantiq / Intel DSA driver for vrx200")
Cc: stable@vger.kernel.org
Suggested-by: Hauke Mehrtens <hauke@hauke-m.de>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/dsa/lantiq_gswip.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index 09701c17f3f6..5d378c8026f0 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -1541,9 +1541,7 @@ static void gswip_phylink_mac_link_up(struct dsa_switch *ds, int port,
 {
 	struct gswip_priv *priv = ds->priv;
 
-	/* Enable the xMII interface only for the external PHY */
-	if (interface != PHY_INTERFACE_MODE_INTERNAL)
-		gswip_mii_mask_cfg(priv, 0, GSWIP_MII_CFG_EN, port);
+	gswip_mii_mask_cfg(priv, 0, GSWIP_MII_CFG_EN, port);
 }
 
 static void gswip_get_strings(struct dsa_switch *ds, int port, u32 stringset,
-- 
2.30.0

[PATCH 2/2] net: dsa: lantiq_gswip: Fix GSWIP_MII_CFG(p) register access

[Martin Blumenstingl]

There is one GSWIP_MII_CFG register for each switch-port except the CPU
port. The register offset for the first port is 0x0, 0x02 for the
second, 0x04 for the third and so on.

Update the driver to not only restrict the GSWIP_MII_CFG registers to
ports 0, 1 and 5. Handle ports 0..5 instead but skip the CPU port. This
means we are not overwriting the configuration for the third port (port
two since we start counting from zero) with the settings for the sixth
port (with number five) anymore.

The GSWIP_MII_PCDU(p) registers are not updated because there's really
only three (one for each of the following ports: 0, 1, 5).

Fixes: 14fceff4771e51 ("net: dsa: Add Lantiq / Intel DSA driver for vrx200")
Cc: stable@vger.kernel.org
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/dsa/lantiq_gswip.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index 5d378c8026f0..4b36d89bec06 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -92,9 +92,7 @@
 					 GSWIP_MDIO_PHY_FDUP_MASK)
 
 /* GSWIP MII Registers */
-#define GSWIP_MII_CFG0			0x00
-#define GSWIP_MII_CFG1			0x02
-#define GSWIP_MII_CFG5			0x04
+#define GSWIP_MII_CFGp(p)		(0x2 * (p))
 #define  GSWIP_MII_CFG_EN		BIT(14)
 #define  GSWIP_MII_CFG_LDCLKDIS		BIT(12)
 #define  GSWIP_MII_CFG_MODE_MIIP	0x0
@@ -392,17 +390,9 @@ static void gswip_mii_mask(struct gswip_priv *priv, u32 clear, u32 set,
 static void gswip_mii_mask_cfg(struct gswip_priv *priv, u32 clear, u32 set,
 			       int port)
 {
-	switch (port) {
-	case 0:
-		gswip_mii_mask(priv, clear, set, GSWIP_MII_CFG0);
-		break;
-	case 1:
-		gswip_mii_mask(priv, clear, set, GSWIP_MII_CFG1);
-		break;
-	case 5:
-		gswip_mii_mask(priv, clear, set, GSWIP_MII_CFG5);
-		break;
-	}
+	/* There's no MII_CFG register for the CPU port */
+	if (!dsa_is_cpu_port(priv->ds, port))
+		gswip_mii_mask(priv, clear, set, GSWIP_MII_CFGp(port));
 }
 
 static void gswip_mii_mask_pcdu(struct gswip_priv *priv, u32 clear, u32 set,
@@ -822,9 +812,8 @@ static int gswip_setup(struct dsa_switch *ds)
 	gswip_mdio_mask(priv, 0xff, 0x09, GSWIP_MDIO_MDC_CFG1);
 
 	/* Disable the xMII link */
-	gswip_mii_mask_cfg(priv, GSWIP_MII_CFG_EN, 0, 0);
-	gswip_mii_mask_cfg(priv, GSWIP_MII_CFG_EN, 0, 1);
-	gswip_mii_mask_cfg(priv, GSWIP_MII_CFG_EN, 0, 5);
+	for (i = 0; i < priv->hw_info->max_ports; i++)
+		gswip_mii_mask_cfg(priv, GSWIP_MII_CFG_EN, 0, i);
 
 	/* enable special tag insertion on cpu port */
 	gswip_switch_mask(priv, 0, GSWIP_FDMA_PCTRL_STEN,
-- 
2.30.0

Re: [PATCH 1/2] net: dsa: lantiq_gswip: Enable GSWIP_MII_CFG_EN also for internal PHYs

[Hauke Mehrtens]

On 1/3/21 2:25 AM, Martin Blumenstingl wrote:
> Enable GSWIP_MII_CFG_EN also for internal PHYs to make traffic flow.
> Without this the PHY link is detected properly and ethtool statistics
> for TX are increasing but there's no RX traffic coming in.
> 
> Fixes: 14fceff4771e51 ("net: dsa: Add Lantiq / Intel DSA driver for vrx200")
> Cc: stable@vger.kernel.org
> Suggested-by: Hauke Mehrtens <hauke@hauke-m.de>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Acked-by: Hauke Mehrtens <hauke@hauke-m.de>

> ---
>   drivers/net/dsa/lantiq_gswip.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
> index 09701c17f3f6..5d378c8026f0 100644
> --- a/drivers/net/dsa/lantiq_gswip.c
> +++ b/drivers/net/dsa/lantiq_gswip.c
> @@ -1541,9 +1541,7 @@ static void gswip_phylink_mac_link_up(struct dsa_switch *ds, int port,
>   {
>   	struct gswip_priv *priv = ds->priv;
>   
> -	/* Enable the xMII interface only for the external PHY */
> -	if (interface != PHY_INTERFACE_MODE_INTERNAL)
> -		gswip_mii_mask_cfg(priv, 0, GSWIP_MII_CFG_EN, port);
> +	gswip_mii_mask_cfg(priv, 0, GSWIP_MII_CFG_EN, port);
>   }
>   
>   static void gswip_get_strings(struct dsa_switch *ds, int port, u32 stringset,
> 

Re: [PATCH 2/2] net: dsa: lantiq_gswip: Fix GSWIP_MII_CFG(p) register access

[Hauke Mehrtens]

On 1/3/21 2:25 AM, Martin Blumenstingl wrote:
> There is one GSWIP_MII_CFG register for each switch-port except the CPU
> port. The register offset for the first port is 0x0, 0x02 for the
> second, 0x04 for the third and so on.
> 
> Update the driver to not only restrict the GSWIP_MII_CFG registers to
> ports 0, 1 and 5. Handle ports 0..5 instead but skip the CPU port. This
> means we are not overwriting the configuration for the third port (port
> two since we start counting from zero) with the settings for the sixth
> port (with number five) anymore.
> 
> The GSWIP_MII_PCDU(p) registers are not updated because there's really
> only three (one for each of the following ports: 0, 1, 5).
> 
> Fixes: 14fceff4771e51 ("net: dsa: Add Lantiq / Intel DSA driver for vrx200")
> Cc: stable@vger.kernel.org
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Acked-by: Hauke Mehrtens <hauke@hauke-m.de>

> ---
>   drivers/net/dsa/lantiq_gswip.c | 23 ++++++-----------------
>   1 file changed, 6 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
> index 5d378c8026f0..4b36d89bec06 100644
> --- a/drivers/net/dsa/lantiq_gswip.c
> +++ b/drivers/net/dsa/lantiq_gswip.c
> @@ -92,9 +92,7 @@
>   					 GSWIP_MDIO_PHY_FDUP_MASK)
>   
>   /* GSWIP MII Registers */
> -#define GSWIP_MII_CFG0			0x00
> -#define GSWIP_MII_CFG1			0x02
> -#define GSWIP_MII_CFG5			0x04
> +#define GSWIP_MII_CFGp(p)		(0x2 * (p))
>   #define  GSWIP_MII_CFG_EN		BIT(14)
>   #define  GSWIP_MII_CFG_LDCLKDIS		BIT(12)
>   #define  GSWIP_MII_CFG_MODE_MIIP	0x0
> @@ -392,17 +390,9 @@ static void gswip_mii_mask(struct gswip_priv *priv, u32 clear, u32 set,
>   static void gswip_mii_mask_cfg(struct gswip_priv *priv, u32 clear, u32 set,
>   			       int port)
>   {
> -	switch (port) {
> -	case 0:
> -		gswip_mii_mask(priv, clear, set, GSWIP_MII_CFG0);
> -		break;
> -	case 1:
> -		gswip_mii_mask(priv, clear, set, GSWIP_MII_CFG1);
> -		break;
> -	case 5:
> -		gswip_mii_mask(priv, clear, set, GSWIP_MII_CFG5);
> -		break;
> -	}
> +	/* There's no MII_CFG register for the CPU port */
> +	if (!dsa_is_cpu_port(priv->ds, port))
> +		gswip_mii_mask(priv, clear, set, GSWIP_MII_CFGp(port));
>   }
>   
>   static void gswip_mii_mask_pcdu(struct gswip_priv *priv, u32 clear, u32 set,
> @@ -822,9 +812,8 @@ static int gswip_setup(struct dsa_switch *ds)
>   	gswip_mdio_mask(priv, 0xff, 0x09, GSWIP_MDIO_MDC_CFG1);
>   
>   	/* Disable the xMII link */
> -	gswip_mii_mask_cfg(priv, GSWIP_MII_CFG_EN, 0, 0);
> -	gswip_mii_mask_cfg(priv, GSWIP_MII_CFG_EN, 0, 1);
> -	gswip_mii_mask_cfg(priv, GSWIP_MII_CFG_EN, 0, 5);
> +	for (i = 0; i < priv->hw_info->max_ports; i++)
> +		gswip_mii_mask_cfg(priv, GSWIP_MII_CFG_EN, 0, i);
>   
>   	/* enable special tag insertion on cpu port */
>   	gswip_switch_mask(priv, 0, GSWIP_FDMA_PCTRL_STEN,
> 

Re: [PATCH 1/2] net: dsa: lantiq_gswip: Enable GSWIP_MII_CFG_EN also for internal PHYs

[Andrew Lunn]

On Sun, Jan 03, 2021 at 02:25:43AM +0100, Martin Blumenstingl wrote:
> Enable GSWIP_MII_CFG_EN also for internal PHYs to make traffic flow.
> Without this the PHY link is detected properly and ethtool statistics
> for TX are increasing but there's no RX traffic coming in.
> 
> Fixes: 14fceff4771e51 ("net: dsa: Add Lantiq / Intel DSA driver for vrx200")
> Cc: stable@vger.kernel.org

Hi Martin

No need to Cc: stable. David or Jakub will handle the backport to
stable.  You should however set the subject to [PATCH net 1/2] and
base the patches on the net tree, not net-next.

     Andrew

Re: [PATCH 1/2] net: dsa: lantiq_gswip: Enable GSWIP_MII_CFG_EN also for internal PHYs

[Martin Blumenstingl]

Hi Andrew,

On Sun, Jan 3, 2021 at 3:09 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Sun, Jan 03, 2021 at 02:25:43AM +0100, Martin Blumenstingl wrote:
> > Enable GSWIP_MII_CFG_EN also for internal PHYs to make traffic flow.
> > Without this the PHY link is detected properly and ethtool statistics
> > for TX are increasing but there's no RX traffic coming in.
> >
> > Fixes: 14fceff4771e51 ("net: dsa: Add Lantiq / Intel DSA driver for vrx200")
> > Cc: stable@vger.kernel.org
>
> Hi Martin
>
> No need to Cc: stable. David or Jakub will handle the backport to
> stable.  You should however set the subject to [PATCH net 1/2] and
> base the patches on the net tree, not net-next.
do you recommend re-sending these patches and changing the subject?
the lantiq_gswip.c driver is identical in -net and -net-next and so
the patch will apply fine in both cases


Best regards,
Martin

Re: [PATCH 1/2] net: dsa: lantiq_gswip: Enable GSWIP_MII_CFG_EN also for internal PHYs

[Florian Fainelli]

On 1/2/21 5:25 PM, Martin Blumenstingl wrote:
> Enable GSWIP_MII_CFG_EN also for internal PHYs to make traffic flow.
> Without this the PHY link is detected properly and ethtool statistics
> for TX are increasing but there's no RX traffic coming in.
> 
> Fixes: 14fceff4771e51 ("net: dsa: Add Lantiq / Intel DSA driver for vrx200")
> Cc: stable@vger.kernel.org
> Suggested-by: Hauke Mehrtens <hauke@hauke-m.de>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH 2/2] net: dsa: lantiq_gswip: Fix GSWIP_MII_CFG(p) register access

[Florian Fainelli]

On 1/2/21 5:25 PM, Martin Blumenstingl wrote:
> There is one GSWIP_MII_CFG register for each switch-port except the CPU
> port. The register offset for the first port is 0x0, 0x02 for the
> second, 0x04 for the third and so on.
> 
> Update the driver to not only restrict the GSWIP_MII_CFG registers to
> ports 0, 1 and 5. Handle ports 0..5 instead but skip the CPU port. This
> means we are not overwriting the configuration for the third port (port
> two since we start counting from zero) with the settings for the sixth
> port (with number five) anymore.
> 
> The GSWIP_MII_PCDU(p) registers are not updated because there's really
> only three (one for each of the following ports: 0, 1, 5).
> 
> Fixes: 14fceff4771e51 ("net: dsa: Add Lantiq / Intel DSA driver for vrx200")
> Cc: stable@vger.kernel.org
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH 1/2] net: dsa: lantiq_gswip: Enable GSWIP_MII_CFG_EN also for internal PHYs

[Jakub Kicinski]

On Sun, 3 Jan 2021 03:12:21 +0100 Martin Blumenstingl wrote:
> Hi Andrew,
> 
> On Sun, Jan 3, 2021 at 3:09 AM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Sun, Jan 03, 2021 at 02:25:43AM +0100, Martin Blumenstingl wrote:  
> > > Enable GSWIP_MII_CFG_EN also for internal PHYs to make traffic flow.
> > > Without this the PHY link is detected properly and ethtool statistics
> > > for TX are increasing but there's no RX traffic coming in.
> > >
> > > Fixes: 14fceff4771e51 ("net: dsa: Add Lantiq / Intel DSA driver for vrx200")
> > > Cc: stable@vger.kernel.org  
> >
> > Hi Martin
> >
> > No need to Cc: stable. David or Jakub will handle the backport to
> > stable.  You should however set the subject to [PATCH net 1/2] and
> > base the patches on the net tree, not net-next.  
> do you recommend re-sending these patches and changing the subject?
> the lantiq_gswip.c driver is identical in -net and -net-next and so
> the patch will apply fine in both cases

Resend is pretty much always a safe bet. But since as you said trees 
are identical at the moment I made an exception applied as is :)

Thanks everyone!

Re: [PATCH 1/2] net: dsa: lantiq_gswip: Enable GSWIP_MII_CFG_EN also for internal PHYs

[Martin Blumenstingl]

Hi Jakub,


On Mon, Jan 4, 2021 at 10:52 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Sun, 3 Jan 2021 03:12:21 +0100 Martin Blumenstingl wrote:
> > Hi Andrew,
> >
> > On Sun, Jan 3, 2021 at 3:09 AM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > On Sun, Jan 03, 2021 at 02:25:43AM +0100, Martin Blumenstingl wrote:
> > > > Enable GSWIP_MII_CFG_EN also for internal PHYs to make traffic flow.
> > > > Without this the PHY link is detected properly and ethtool statistics
> > > > for TX are increasing but there's no RX traffic coming in.
> > > >
> > > > Fixes: 14fceff4771e51 ("net: dsa: Add Lantiq / Intel DSA driver for vrx200")
> > > > Cc: stable@vger.kernel.org
> > >
> > > Hi Martin
> > >
> > > No need to Cc: stable. David or Jakub will handle the backport to
> > > stable.  You should however set the subject to [PATCH net 1/2] and
> > > base the patches on the net tree, not net-next.
> > do you recommend re-sending these patches and changing the subject?
> > the lantiq_gswip.c driver is identical in -net and -net-next and so
> > the patch will apply fine in both cases
>
> Resend is pretty much always a safe bet. But since as you said trees
> are identical at the moment I made an exception applied as is :)
awesome, thank you! :-)


Best regards,
Martin