[net PATCH 1/2] net: dsa: qca8k: fix internal delay applied to the wrong PAD config

[net PATCH 1/2] net: dsa: qca8k: fix internal delay applied to the wrong PAD config

[Ansuel Smith]

With SGMII phy the internal delay is always applied to the PAD0 config.
This is caused by the falling edge configuration that hardcode the reg
to PAD0 (as the falling edge bits are present only in PAD0 reg)
Move the delay configuration before the reg overwrite to correctly apply
the delay.

Fixes: cef08115846e ("net: dsa: qca8k: set internal delay also for sgmii")
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index a429c9750add..d7bcecbc1c53 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -1433,6 +1433,12 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 
 		qca8k_write(priv, QCA8K_REG_SGMII_CTRL, val);
 
+		/* From original code is reported port instability as SGMII also
+		 * require delay set. Apply advised values here or take them from DT.
+		 */
+		if (state->interface == PHY_INTERFACE_MODE_SGMII)
+			qca8k_mac_config_setup_internal_delay(priv, cpu_port_index, reg);
+
 		/* For qca8327/qca8328/qca8334/qca8338 sgmii is unique and
 		 * falling edge is set writing in the PORT0 PAD reg
 		 */
@@ -1455,12 +1461,6 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 					QCA8K_PORT0_PAD_SGMII_TXCLK_FALLING_EDGE,
 					val);
 
-		/* From original code is reported port instability as SGMII also
-		 * require delay set. Apply advised values here or take them from DT.
-		 */
-		if (state->interface == PHY_INTERFACE_MODE_SGMII)
-			qca8k_mac_config_setup_internal_delay(priv, cpu_port_index, reg);
-
 		break;
 	default:
 		dev_err(ds->dev, "xMII mode %s not supported for port %d\n",
-- 
2.32.0

[net PATCH 2/2] net: dsa: qca8k: fix MTU calculation

[Ansuel Smith]

From: Robert Marko <robert.marko@sartura.hr>

qca8k has a global MTU, so its tracking the MTU per port to make sure
that the largest MTU gets applied.
Since it uses the frame size instead of MTU the driver MTU change function
will then add the size of Ethernet header and checksum on top of MTU.

The driver currently populates the per port MTU size as Ethernet frame
length + checksum which equals 1518.

The issue is that then MTU change function will go through all of the
ports, find the largest MTU and apply the Ethernet header + checksum on
top of it again, so for a desired MTU of 1500 you will end up with 1536.

This is obviously incorrect, so to correct it populate the per port struct
MTU with just the MTU and not include the Ethernet header + checksum size
as those will be added by the MTU change function.

Fixes: f58d2598cf70 ("net: dsa: qca8k: implement the port MTU callbacks")
Signed-off-by: Robert Marko <robert.marko@sartura.hr>
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index d7bcecbc1c53..147ca39531a3 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -1256,8 +1256,12 @@ qca8k_setup(struct dsa_switch *ds)
 		/* Set initial MTU for every port.
 		 * We have only have a general MTU setting. So track
 		 * every port and set the max across all port.
+		 * Set per port MTU to 1500 as the MTU change function
+		 * will add the overhead and if its set to 1518 then it
+		 * will apply the overhead again and we will end up with
+		 * MTU of 1536 instead of 1518
 		 */
-		priv->port_mtu[i] = ETH_FRAME_LEN + ETH_FCS_LEN;
+		priv->port_mtu[i] = ETH_DATA_LEN;
 	}
 
 	/* Special GLOBAL_FC_THRESH value are needed for ar8327 switch */
-- 
2.32.0

Re: [net PATCH 2/2] net: dsa: qca8k: fix MTU calculation

[Vladimir Oltean]

On Fri, Nov 19, 2021 at 03:03:50AM +0100, Ansuel Smith wrote:
> From: Robert Marko <robert.marko@sartura.hr>
> 
> qca8k has a global MTU, so its tracking the MTU per port to make sure
> that the largest MTU gets applied.
> Since it uses the frame size instead of MTU the driver MTU change function
> will then add the size of Ethernet header and checksum on top of MTU.
> 
> The driver currently populates the per port MTU size as Ethernet frame
> length + checksum which equals 1518.
> 
> The issue is that then MTU change function will go through all of the
> ports, find the largest MTU and apply the Ethernet header + checksum on
> top of it again, so for a desired MTU of 1500 you will end up with 1536.
> 
> This is obviously incorrect, so to correct it populate the per port struct
> MTU with just the MTU and not include the Ethernet header + checksum size
> as those will be added by the MTU change function.
> 
> Fixes: f58d2598cf70 ("net: dsa: qca8k: implement the port MTU callbacks")
> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [net PATCH 1/2] net: dsa: qca8k: fix internal delay applied to the wrong PAD config

[Vladimir Oltean]

On Fri, Nov 19, 2021 at 03:03:49AM +0100, Ansuel Smith wrote:
> With SGMII phy the internal delay is always applied to the PAD0 config.
> This is caused by the falling edge configuration that hardcode the reg
> to PAD0 (as the falling edge bits are present only in PAD0 reg)
> Move the delay configuration before the reg overwrite to correctly apply
> the delay.
> 
> Fixes: cef08115846e ("net: dsa: qca8k: set internal delay also for sgmii")
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---

This removes the need for your other patch "net: dsa: qca8k: skip sgmii
delay on double cpu conf", right?

>  drivers/net/dsa/qca8k.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index a429c9750add..d7bcecbc1c53 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -1433,6 +1433,12 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  
>  		qca8k_write(priv, QCA8K_REG_SGMII_CTRL, val);
>  
> +		/* From original code is reported port instability as SGMII also
> +		 * require delay set. Apply advised values here or take them from DT.
> +		 */
> +		if (state->interface == PHY_INTERFACE_MODE_SGMII)
> +			qca8k_mac_config_setup_internal_delay(priv, cpu_port_index, reg);
> +
>  		/* For qca8327/qca8328/qca8334/qca8338 sgmii is unique and
>  		 * falling edge is set writing in the PORT0 PAD reg
>  		 */
> @@ -1455,12 +1461,6 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  					QCA8K_PORT0_PAD_SGMII_TXCLK_FALLING_EDGE,
>  					val);
>  
> -		/* From original code is reported port instability as SGMII also
> -		 * require delay set. Apply advised values here or take them from DT.
> -		 */
> -		if (state->interface == PHY_INTERFACE_MODE_SGMII)
> -			qca8k_mac_config_setup_internal_delay(priv, cpu_port_index, reg);
> -
>  		break;
>  	default:
>  		dev_err(ds->dev, "xMII mode %s not supported for port %d\n",
> -- 
> 2.32.0
> 

Re: [net PATCH 1/2] net: dsa: qca8k: fix internal delay applied to the wrong PAD config

[Ansuel Smith]

On Sun, Nov 21, 2021 at 08:18:29PM +0200, Vladimir Oltean wrote:
> On Fri, Nov 19, 2021 at 03:03:49AM +0100, Ansuel Smith wrote:
> > With SGMII phy the internal delay is always applied to the PAD0 config.
> > This is caused by the falling edge configuration that hardcode the reg
> > to PAD0 (as the falling edge bits are present only in PAD0 reg)
> > Move the delay configuration before the reg overwrite to correctly apply
> > the delay.
> > 
> > Fixes: cef08115846e ("net: dsa: qca8k: set internal delay also for sgmii")
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> 
> This removes the need for your other patch "net: dsa: qca8k: skip sgmii
> delay on double cpu conf", right?
>

Correct the problem was really with overwriting and not with some
malfunction with setting the delay to both rgmii and sgmii.
And also as you pointed out, the delay was ignored with sgmii phy mode
and delay not declared in the dts.

> >  drivers/net/dsa/qca8k.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index a429c9750add..d7bcecbc1c53 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -1433,6 +1433,12 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> >  
> >  		qca8k_write(priv, QCA8K_REG_SGMII_CTRL, val);
> >  
> > +		/* From original code is reported port instability as SGMII also
> > +		 * require delay set. Apply advised values here or take them from DT.
> > +		 */
> > +		if (state->interface == PHY_INTERFACE_MODE_SGMII)
> > +			qca8k_mac_config_setup_internal_delay(priv, cpu_port_index, reg);
> > +
> >  		/* For qca8327/qca8328/qca8334/qca8338 sgmii is unique and
> >  		 * falling edge is set writing in the PORT0 PAD reg
> >  		 */
> > @@ -1455,12 +1461,6 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> >  					QCA8K_PORT0_PAD_SGMII_TXCLK_FALLING_EDGE,
> >  					val);
> >  
> > -		/* From original code is reported port instability as SGMII also
> > -		 * require delay set. Apply advised values here or take them from DT.
> > -		 */
> > -		if (state->interface == PHY_INTERFACE_MODE_SGMII)
> > -			qca8k_mac_config_setup_internal_delay(priv, cpu_port_index, reg);
> > -
> >  		break;
> >  	default:
> >  		dev_err(ds->dev, "xMII mode %s not supported for port %d\n",
> > -- 
> > 2.32.0
> > 

-- 
	Ansuel

Re: [net PATCH 1/2] net: dsa: qca8k: fix internal delay applied to the wrong PAD config

[Vladimir Oltean]

On Fri, Nov 19, 2021 at 03:03:49AM +0100, Ansuel Smith wrote:
> With SGMII phy the internal delay is always applied to the PAD0 config.
> This is caused by the falling edge configuration that hardcode the reg
> to PAD0 (as the falling edge bits are present only in PAD0 reg)
> Move the delay configuration before the reg overwrite to correctly apply
> the delay.
> 
> Fixes: cef08115846e ("net: dsa: qca8k: set internal delay also for sgmii")
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [net PATCH 1/2] net: dsa: qca8k: fix internal delay applied to the wrong PAD config

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Fri, 19 Nov 2021 03:03:49 +0100 you wrote:
> With SGMII phy the internal delay is always applied to the PAD0 config.
> This is caused by the falling edge configuration that hardcode the reg
> to PAD0 (as the falling edge bits are present only in PAD0 reg)
> Move the delay configuration before the reg overwrite to correctly apply
> the delay.
> 
> Fixes: cef08115846e ("net: dsa: qca8k: set internal delay also for sgmii")
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> 
> [...]

Here is the summary with links:
  - [net,1/2] net: dsa: qca8k: fix internal delay applied to the wrong PAD config
    https://git.kernel.org/netdev/net/c/3b00a07c2443
  - [net,2/2] net: dsa: qca8k: fix MTU calculation
    https://git.kernel.org/netdev/net/c/65258b9d8cde

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html