LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [net PATCH 1/2] net: dsa: qca8k: fix internal delay applied to the wrong PAD config
@ 2021-11-19  2:03 Ansuel Smith
  2021-11-19  2:03 ` [net PATCH 2/2] net: dsa: qca8k: fix MTU calculation Ansuel Smith
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Ansuel Smith @ 2021-11-19  2:03 UTC (permalink / raw)
  To: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean,
	David S. Miller, Jakub Kicinski, Ansuel Smith, Jonathan McDowell,
	netdev, linux-kernel

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [net PATCH 2/2] net: dsa: qca8k: fix MTU calculation
  2021-11-19  2:03 [net PATCH 1/2] net: dsa: qca8k: fix internal delay applied to the wrong PAD config Ansuel Smith
@ 2021-11-19  2:03 ` Ansuel Smith
  2021-11-21 18:14   ` Vladimir Oltean
  2021-11-21 18:18 ` [net PATCH 1/2] net: dsa: qca8k: fix internal delay applied to the wrong PAD config Vladimir Oltean
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 7+ messages in thread
From: Ansuel Smith @ 2021-11-19  2:03 UTC (permalink / raw)
  To: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean,
	David S. Miller, Jakub Kicinski, Ansuel Smith, Jonathan McDowell,
	netdev, linux-kernel
  Cc: Robert Marko

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [net PATCH 2/2] net: dsa: qca8k: fix MTU calculation
  2021-11-19  2:03 ` [net PATCH 2/2] net: dsa: qca8k: fix MTU calculation Ansuel Smith
@ 2021-11-21 18:14   ` Vladimir Oltean
  0 siblings, 0 replies; 7+ messages in thread
From: Vladimir Oltean @ 2021-11-21 18:14 UTC (permalink / raw)
  To: Ansuel Smith
  Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
	Jakub Kicinski, Jonathan McDowell, netdev, linux-kernel,
	Robert Marko

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>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [net PATCH 1/2] net: dsa: qca8k: fix internal delay applied to the wrong PAD config
  2021-11-19  2:03 [net PATCH 1/2] net: dsa: qca8k: fix internal delay applied to the wrong PAD config Ansuel Smith
  2021-11-19  2:03 ` [net PATCH 2/2] net: dsa: qca8k: fix MTU calculation Ansuel Smith
@ 2021-11-21 18:18 ` Vladimir Oltean
  2021-11-21 18:22   ` Ansuel Smith
  2021-11-21 18:36 ` Vladimir Oltean
  2021-11-22 12:50 ` patchwork-bot+netdevbpf
  3 siblings, 1 reply; 7+ messages in thread
From: Vladimir Oltean @ 2021-11-21 18:18 UTC (permalink / raw)
  To: Ansuel Smith
  Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
	Jakub Kicinski, Jonathan McDowell, netdev, linux-kernel

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
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [net PATCH 1/2] net: dsa: qca8k: fix internal delay applied to the wrong PAD config
  2021-11-21 18:18 ` [net PATCH 1/2] net: dsa: qca8k: fix internal delay applied to the wrong PAD config Vladimir Oltean
@ 2021-11-21 18:22   ` Ansuel Smith
  0 siblings, 0 replies; 7+ messages in thread
From: Ansuel Smith @ 2021-11-21 18:22 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
	Jakub Kicinski, Jonathan McDowell, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [net PATCH 1/2] net: dsa: qca8k: fix internal delay applied to the wrong PAD config
  2021-11-19  2:03 [net PATCH 1/2] net: dsa: qca8k: fix internal delay applied to the wrong PAD config Ansuel Smith
  2021-11-19  2:03 ` [net PATCH 2/2] net: dsa: qca8k: fix MTU calculation Ansuel Smith
  2021-11-21 18:18 ` [net PATCH 1/2] net: dsa: qca8k: fix internal delay applied to the wrong PAD config Vladimir Oltean
@ 2021-11-21 18:36 ` Vladimir Oltean
  2021-11-22 12:50 ` patchwork-bot+netdevbpf
  3 siblings, 0 replies; 7+ messages in thread
From: Vladimir Oltean @ 2021-11-21 18:36 UTC (permalink / raw)
  To: Ansuel Smith
  Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
	Jakub Kicinski, Jonathan McDowell, netdev, linux-kernel

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>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [net PATCH 1/2] net: dsa: qca8k: fix internal delay applied to the wrong PAD config
  2021-11-19  2:03 [net PATCH 1/2] net: dsa: qca8k: fix internal delay applied to the wrong PAD config Ansuel Smith
                   ` (2 preceding siblings ...)
  2021-11-21 18:36 ` Vladimir Oltean
@ 2021-11-22 12:50 ` patchwork-bot+netdevbpf
  3 siblings, 0 replies; 7+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-11-22 12:50 UTC (permalink / raw)
  To: Ansuel Smith
  Cc: andrew, vivien.didelot, f.fainelli, olteanv, davem, kuba,
	noodles, netdev, linux-kernel

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



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-11-22 12:50 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-19  2:03 [net PATCH 1/2] net: dsa: qca8k: fix internal delay applied to the wrong PAD config Ansuel Smith
2021-11-19  2:03 ` [net PATCH 2/2] net: dsa: qca8k: fix MTU calculation Ansuel Smith
2021-11-21 18:14   ` Vladimir Oltean
2021-11-21 18:18 ` [net PATCH 1/2] net: dsa: qca8k: fix internal delay applied to the wrong PAD config Vladimir Oltean
2021-11-21 18:22   ` Ansuel Smith
2021-11-21 18:36 ` Vladimir Oltean
2021-11-22 12:50 ` patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).