[PATCH net-next 0/2] implement microchip,no-tag-protocol flag

[PATCH net-next 0/2] implement microchip,no-tag-protocol flag

[vtpieter]

From: Pieter Van Trappen <pieter.van.trappen@cern.ch>

Add and implement microchip,no-tag-protocol flag to allow disabling
the switch' tagging protocol. For cases where the CPU MAC does not
support MTU size > 1500 such as the Zynq GEM.

This code was tested with a KSZ8794 chip.

Pieter Van Trappen (2):
  dt-bindings: net: dsa: microchip: add microchip,no-tag-protocol flag
  net: dsa: microchip: implement microchip,no-tag-protocol flag

 .../devicetree/bindings/net/dsa/microchip,ksz.yaml    |  5 +++++
 drivers/net/dsa/microchip/ksz8795.c                   |  2 +-
 drivers/net/dsa/microchip/ksz9477.c                   |  2 +-
 drivers/net/dsa/microchip/ksz_common.c                | 11 ++++++++---
 drivers/net/dsa/microchip/ksz_common.h                |  1 +
 drivers/net/dsa/microchip/lan937x_main.c              |  2 +-
 6 files changed, 17 insertions(+), 6 deletions(-)


base-commit: 0a658d088cc63745528cf0ec8a2c2df0f37742d9
-- 
2.43.0

[PATCH net-next 1/2] dt-bindings: net: dsa: microchip: add microchip,no-tag-protocol flag

[vtpieter]

From: Pieter Van Trappen <pieter.van.trappen@cern.ch>

Add microchip,no-tag-protocol flag to allow disabling the switch'
tagging protocol. For cases where the CPU MAC does not support MTU
size > 1500 such as the Zynq GEM.

Signed-off-by: Pieter Van Trappen <pieter.van.trappen@cern.ch>
---
 Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
index 52acc15ebcbf..798dd3c76ab6 100644
--- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
@@ -45,6 +45,11 @@ properties:
     description:
       Set if the output SYNCLKO frequency should be set to 125MHz instead of 25MHz.
 
+  microchip,no-tag-protocol:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Set if no switch tag protocol should be enabled.
+
   microchip,synclko-disable:
     $ref: /schemas/types.yaml#/definitions/flag
     description:
-- 
2.43.0

Re: [PATCH net-next 1/2] dt-bindings: net: dsa: microchip: add microchip,no-tag-protocol flag

[Rob Herring]

On Thu, Aug 01, 2024 at 02:31:42PM +0200, vtpieter@gmail.com wrote:
> From: Pieter Van Trappen <pieter.van.trappen@cern.ch>
> 
> Add microchip,no-tag-protocol flag to allow disabling the switch'

What is the ' for?

> tagging protocol. For cases where the CPU MAC does not support MTU
> size > 1500 such as the Zynq GEM.

What is "switch tag protocol"? Not defined anywhere? Is that VLAN 
tagging?

It seems to me that this doesn't need to be in DT. You know you have 
Zynq GEM because it should have a specific compatible. If it doesn't 
support some feature, then that should get propagated to the switch 
somehow.

Rob

Re: [PATCH net-next 1/2] dt-bindings: net: dsa: microchip: add microchip,no-tag-protocol flag

[Pieter]

On Wed 6 Aug 2024 at 19:17, Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Aug 01, 2024 at 02:31:42PM +0200, vtpieter@gmail.com wrote:
> > From: Pieter Van Trappen <pieter.van.trappen@cern.ch>
> >
> > Add microchip,no-tag-protocol flag to allow disabling the switch'
>
> What is the ' for?
Typo
>
> > tagging protocol. For cases where the CPU MAC does not support MTU
> > size > 1500 such as the Zynq GEM.
>
> What is "switch tag protocol"? Not defined anywhere? Is that VLAN
> tagging?
>
> It seems to me that this doesn't need to be in DT. You know you have
> Zynq GEM because it should have a specific compatible. If it doesn't
> support some feature, then that should get propagated to the switch
> somehow.

Hi Rob, indeed and as indicated by Vladimir Oltean in a reply to this
patch, such a property exists already, dsa-port's `dsa-tag-protocol`.

Driver support is to be added but rather I'm changing the Zynq GEM
driver to support MTU > 1500 as in fact it turns our the harware does.

Pieter

>
> Rob

[PATCH net-next 2/2] net: dsa: microchip: implement microchip,no-tag-protocol flag

[vtpieter]

From: Pieter Van Trappen <pieter.van.trappen@cern.ch>

Implement microchip,no-tag-protocol flag to allow disabling the
switch' tagging protocol. For cases where the CPU MAC does not support
MTU size > 1500 such as the Zynq GEM.

This code was tested with a KSZ8794 chip.

Signed-off-by: Pieter Van Trappen <pieter.van.trappen@cern.ch>
---
 drivers/net/dsa/microchip/ksz8795.c      |  2 +-
 drivers/net/dsa/microchip/ksz9477.c      |  2 +-
 drivers/net/dsa/microchip/ksz_common.c   | 11 ++++++++---
 drivers/net/dsa/microchip/ksz_common.h   |  1 +
 drivers/net/dsa/microchip/lan937x_main.c |  2 +-
 5 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index d27b9c36d73f..0442341d6d0f 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -1604,7 +1604,7 @@ void ksz8_config_cpu_port(struct dsa_switch *ds)
 	masks = dev->info->masks;
 	regs = dev->info->regs;
 
-	ksz_cfg(dev, regs[S_TAIL_TAG_CTRL], masks[SW_TAIL_TAG_ENABLE], true);
+	ksz_cfg(dev, regs[S_TAIL_TAG_CTRL], masks[SW_TAIL_TAG_ENABLE], !dev->no_tag_proto);
 
 	ksz8_port_setup(dev, dev->cpu_port, true);
 
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 425e20daf1e9..a3c68751f258 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -1195,7 +1195,7 @@ void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 	/* enable tag tail for host port */
 	if (cpu_port)
 		ksz_port_cfg(dev, port, REG_PORT_CTRL_0, PORT_TAIL_TAG_ENABLE,
-			     true);
+			     !dev->no_tag_proto);
 
 	ksz9477_port_queue_split(dev, port);
 
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index b074b4bb0629..fbbf26e940bc 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -2882,9 +2882,7 @@ static enum dsa_tag_protocol ksz_get_tag_protocol(struct dsa_switch *ds,
 	struct ksz_device *dev = ds->priv;
 	enum dsa_tag_protocol proto = DSA_TAG_PROTO_NONE;
 
-	if (dev->chip_id == KSZ8795_CHIP_ID ||
-	    dev->chip_id == KSZ8794_CHIP_ID ||
-	    dev->chip_id == KSZ8765_CHIP_ID)
+	if (ksz_is_ksz87xx(dev))
 		proto = DSA_TAG_PROTO_KSZ8795;
 
 	if (dev->chip_id == KSZ8830_CHIP_ID ||
@@ -2903,6 +2901,9 @@ static enum dsa_tag_protocol ksz_get_tag_protocol(struct dsa_switch *ds,
 	if (is_lan937x(dev))
 		proto = DSA_TAG_PROTO_LAN937X;
 
+	if (dev->no_tag_proto)
+		proto = DSA_TAG_PROTO_NONE;
+
 	return proto;
 }
 
@@ -2912,6 +2913,8 @@ static int ksz_connect_tag_protocol(struct dsa_switch *ds,
 	struct ksz_tagger_data *tagger_data;
 
 	switch (proto) {
+	case DSA_TAG_PROTO_NONE:
+		return 0;
 	case DSA_TAG_PROTO_KSZ8795:
 		return 0;
 	case DSA_TAG_PROTO_KSZ9893:
@@ -4459,6 +4462,8 @@ int ksz_switch_register(struct ksz_device *dev)
 
 		dev->wakeup_source = of_property_read_bool(dev->dev->of_node,
 							   "wakeup-source");
+		dev->no_tag_proto = of_property_read_bool(dev->dev->of_node,
+							  "microchip,no-tag-protocol");
 	}
 
 	ret = dsa_register_switch(dev->ds);
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 5f0a628b9849..19cc0dd0ac03 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -174,6 +174,7 @@ struct ksz_device {
 	bool synclko_125;
 	bool synclko_disable;
 	bool wakeup_source;
+	bool no_tag_proto;
 
 	struct vlan_table *vlan_cache;
 
diff --git a/drivers/net/dsa/microchip/lan937x_main.c b/drivers/net/dsa/microchip/lan937x_main.c
index 824d9309a3d3..a9345da31ef5 100644
--- a/drivers/net/dsa/microchip/lan937x_main.c
+++ b/drivers/net/dsa/microchip/lan937x_main.c
@@ -182,7 +182,7 @@ void lan937x_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 	/* enable tag tail for host port */
 	if (cpu_port)
 		lan937x_port_cfg(dev, port, REG_PORT_CTRL_0,
-				 PORT_TAIL_TAG_ENABLE, true);
+				 PORT_TAIL_TAG_ENABLE, !dev->no_tag_proto);
 
 	/* Enable the Port Queue split */
 	ksz9477_port_queue_split(dev, port);
-- 
2.43.0

Re: [PATCH net-next 0/2] implement microchip,no-tag-protocol flag

[Vladimir Oltean]

Hi Pieter,

On Thu, Aug 01, 2024 at 02:31:41PM +0200, vtpieter@gmail.com wrote:
> From: Pieter Van Trappen <pieter.van.trappen@cern.ch>
> 
> Add and implement microchip,no-tag-protocol flag to allow disabling
> the switch' tagging protocol. For cases where the CPU MAC does not
> support MTU size > 1500 such as the Zynq GEM.
> 
> This code was tested with a KSZ8794 chip.
> 
> Pieter Van Trappen (2):
>   dt-bindings: net: dsa: microchip: add microchip,no-tag-protocol flag
>   net: dsa: microchip: implement microchip,no-tag-protocol flag
> 
>  .../devicetree/bindings/net/dsa/microchip,ksz.yaml    |  5 +++++
>  drivers/net/dsa/microchip/ksz8795.c                   |  2 +-
>  drivers/net/dsa/microchip/ksz9477.c                   |  2 +-
>  drivers/net/dsa/microchip/ksz_common.c                | 11 ++++++++---
>  drivers/net/dsa/microchip/ksz_common.h                |  1 +
>  drivers/net/dsa/microchip/lan937x_main.c              |  2 +-
>  6 files changed, 17 insertions(+), 6 deletions(-)
> 
> 
> base-commit: 0a658d088cc63745528cf0ec8a2c2df0f37742d9
> -- 
> 2.43.0

Please use ./scripts/get_maintainer.pl when generating the To: and Cc: fields.

Not to say that they don't exist, but I have never seen a NIC where MTU=1500
is the absolute hard upper limit. How seriously did you study this before
determining that it is impossible to raise that? We're talking about one
byte for the tail tag, FWIW.

There are also alternative paths to explore, like reducing the DSA user ports
MTU to 1499. This is currently not done when dev_set_mtu() fails on the conduit,
because Andrew said in the past it's likelier that the conduit is coded
to accept up to 1500 but will still work for small oversized packets.

Disabling DSA tagging is a very heavy hammer, because it cuts off a whole lot
of functionality (the driver should no longer accept PTP hwtimestamping ioctls,
etc), so the patch set gets this tag from me currently, due to very shallow
justification:

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

Please carry it forward if you choose to resubmit.

Even assuming that a strong justification does exists, there already
exists a mechanism for disabling the tagging protocol from the device
tree. It is the same as for specifying any other alternative tagging
protocol (applied in this case to DSA_TAG_PROTO_NONE).

	ethernet-switch@N {
		dsa-tag-protocol = "none";
	};

it just needs implementing in the driver.

The fact that you chose to add a custom device tree property suggests to
me that you did not investigate the problem space very seriously.

Re: [PATCH net-next 0/2] implement microchip,no-tag-protocol flag

[Vladimir Oltean]

On Thu, Aug 01, 2024 at 04:44:01PM +0300, Vladimir Oltean wrote:
> 	ethernet-switch@N {
> 		dsa-tag-protocol = "none";
> 	};

My mistake - this was supposed to be:

	ethernet-switch@N {
		ethernet-ports {
			/* This is the CPU port */
			ethernet-port@0 {
				ethernet = <&conduit>;
				dsa-tag-protocol = "none";
			};
		};
	};

Re: [PATCH net-next 0/2] implement microchip,no-tag-protocol flag

[Pieter]

On Thu, Aug 01, 2024 at 15:44, Vladimir Oltean <olteanv@gmail.com> wrote:
>
> Hi Pieter,
>
> On Thu, Aug 01, 2024 at 02:31:41PM +0200, vtpieter@gmail.com wrote:
> > From: Pieter Van Trappen <pieter.van.trappen@cern.ch>
> >
> > Add and implement microchip,no-tag-protocol flag to allow disabling
> > the switch' tagging protocol. For cases where the CPU MAC does not
> > support MTU size > 1500 such as the Zynq GEM.
> >
> > This code was tested with a KSZ8794 chip.
> >
> > Pieter Van Trappen (2):
> >   dt-bindings: net: dsa: microchip: add microchip,no-tag-protocol flag
> >   net: dsa: microchip: implement microchip,no-tag-protocol flag
> >
> >  .../devicetree/bindings/net/dsa/microchip,ksz.yaml    |  5 +++++
> >  drivers/net/dsa/microchip/ksz8795.c                   |  2 +-
> >  drivers/net/dsa/microchip/ksz9477.c                   |  2 +-
> >  drivers/net/dsa/microchip/ksz_common.c                | 11 ++++++++---
> >  drivers/net/dsa/microchip/ksz_common.h                |  1 +
> >  drivers/net/dsa/microchip/lan937x_main.c              |  2 +-
> >  6 files changed, 17 insertions(+), 6 deletions(-)
> >
> >
> > base-commit: 0a658d088cc63745528cf0ec8a2c2df0f37742d9
> > --
> > 2.43.0
>
> Please use ./scripts/get_maintainer.pl when generating the To: and Cc: fields.
>
> Not to say that they don't exist, but I have never seen a NIC where MTU=1500
> is the absolute hard upper limit. How seriously did you study this before
> determining that it is impossible to raise that? We're talking about one
> byte for the tail tag, FWIW.
>
> There are also alternative paths to explore, like reducing the DSA user ports
> MTU to 1499. This is currently not done when dev_set_mtu() fails on the conduit,
> because Andrew said in the past it's likelier that the conduit is coded
> to accept up to 1500 but will still work for small oversized packets.
>
> Disabling DSA tagging is a very heavy hammer, because it cuts off a whole lot
> of functionality (the driver should no longer accept PTP hwtimestamping ioctls,
> etc), so the patch set gets this tag from me currently, due to very shallow
> justification:
>
> Nacked-by: Vladimir Oltean <olteanv@gmail.com>
>
> Please carry it forward if you choose to resubmit.

Hi Vladimir,

I do understand your reservation for this path, it defeats most of the
advantages
of DSA but I still do need the driver to handle the PHYs over SPI and for the
WoL functionality; using the switch in a bridge configuration. My reason for
mainlining is that I though there might be more people like me in a
similar situation.

This is actually an older issue and solution of mine so inspired by your comment
I revisited the documentation and indeed hardware-wise I can't find back the
MTU limitation. I see now that it's actually a limitation of the macb driver [1]
so I will try to rework this one instead. Or implement `dsa-tag-protocol` as you
propose. Or event better, both!

Patch can be thus be cancelled, sorry for the spam.

Cheers, Pieter

[1]: https://elixir.bootlin.com/linux/v6.11-rc1/source/drivers/net/ethernet/cadence/macb_main.c#L5127