Store the switch API version in struct gswip_priv (in host endian) to
prepare supporting newer features such as 4096 VLANs and per-port
configurable learning.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v3: use __force for version field endian exception (__le16 __force) to
    fix sparse warning.
v2: no changes

 drivers/net/dsa/lantiq_gswip.c | 3 +++
 drivers/net/dsa/lantiq_gswip.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index f8a43c351649..8999c3f2d290 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -28,6 +28,7 @@
 #include "lantiq_gswip.h"
 #include "lantiq_pce.h"
 
+#include <linux/byteorder/generic.h>
 #include <linux/delay.h>
 #include <linux/etherdevice.h>
 #include <linux/firmware.h>
@@ -1936,6 +1937,8 @@ static int gswip_probe(struct platform_device *pdev)
 					     "gphy fw probe failed\n");
 	}
 
+	priv->version = le16_to_cpu((__le16 __force)version);
+
 	/* bring up the mdio bus */
 	err = gswip_mdio(priv);
 	if (err) {
diff --git a/drivers/net/dsa/lantiq_gswip.h b/drivers/net/dsa/lantiq_gswip.h
index 0b7b6db4eab9..077d1928149b 100644
--- a/drivers/net/dsa/lantiq_gswip.h
+++ b/drivers/net/dsa/lantiq_gswip.h
@@ -258,6 +258,7 @@ struct gswip_priv {
 	struct gswip_gphy_fw *gphy_fw;
 	u32 port_vlan_filter;
 	struct mutex pce_table_lock;
+	u16 version;
 };
 
 #endif /* __LANTIQ_GSWIP_H */
-- 
2.50.1

On Wed, Aug 20, 2025 at 02:55:49AM +0100, Daniel Golle wrote:
> Store the switch API version in struct gswip_priv (in host endian) to
> prepare supporting newer features such as 4096 VLANs and per-port
> configurable learning.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
> v3: use __force for version field endian exception (__le16 __force) to
>     fix sparse warning.
> v2: no changes
> 
>  drivers/net/dsa/lantiq_gswip.c | 3 +++
>  drivers/net/dsa/lantiq_gswip.h | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
> index f8a43c351649..8999c3f2d290 100644
> --- a/drivers/net/dsa/lantiq_gswip.c
> +++ b/drivers/net/dsa/lantiq_gswip.c
> @@ -28,6 +28,7 @@
>  #include "lantiq_gswip.h"
>  #include "lantiq_pce.h"
>  
> +#include <linux/byteorder/generic.h>
>  #include <linux/delay.h>
>  #include <linux/etherdevice.h>
>  #include <linux/firmware.h>
> @@ -1936,6 +1937,8 @@ static int gswip_probe(struct platform_device *pdev)
>  					     "gphy fw probe failed\n");
>  	}
>  
> +	priv->version = le16_to_cpu((__le16 __force)version);

I've researched this a bit more and came to the conclusion that while the
above works fine because all Lantiq SoCs with built-in switch are
big-endian machines it is still wrong.
I base this conclusion on the fact that when dealing with more recent
MDIO-connected switches (MaxLinear GSW1xx series) the host endian doesn't
play a role in the driver -- when dealing with 16-bit values on the MDIO
bus, the bus abstraction takes care of converting from/to host endianess.

The above statement will turn into a no-op on little-endian machines
(lets ignore the truncation to 16-bit for now). However, also on little-
endian machine the 'version' field is byte-swapped.

Hence I believe my original approach (using swab16) is better in my
opinion because rather than delcaring a specific endian, what needs to be
expressed is simply that this field is in opposite byte order than all the
other fields.

Hence I believe this should simply be a swab16() which will always result
in the version being in the right byte order to use comparative operators
in a meaningful way.

Sorry for the confusion.

> +
>  	/* bring up the mdio bus */
>  	err = gswip_mdio(priv);
>  	if (err) {
> diff --git a/drivers/net/dsa/lantiq_gswip.h b/drivers/net/dsa/lantiq_gswip.h
> index 0b7b6db4eab9..077d1928149b 100644
> --- a/drivers/net/dsa/lantiq_gswip.h
> +++ b/drivers/net/dsa/lantiq_gswip.h
> @@ -258,6 +258,7 @@ struct gswip_priv {
>  	struct gswip_gphy_fw *gphy_fw;
>  	u32 port_vlan_filter;
>  	struct mutex pce_table_lock;
> +	u16 version;
>  };
>  
>  #endif /* __LANTIQ_GSWIP_H */
> -- 
> 2.50.1
>

> > +	priv->version = le16_to_cpu((__le16 __force)version);
> 
> I've researched this a bit more and came to the conclusion that while the
> above works fine because all Lantiq SoCs with built-in switch are
> big-endian machines it is still wrong.
> I base this conclusion on the fact that when dealing with more recent
> MDIO-connected switches (MaxLinear GSW1xx series) the host endian doesn't
> play a role in the driver -- when dealing with 16-bit values on the MDIO
> bus, the bus abstraction takes care of converting from/to host endianess.

I agree that all MDIO bus registers are host endian, 16 bit. The shift
register in the hardware is responsible for putting the bits on the
wire in the correct order for MDIO.

> Hence I believe this should simply be a swab16() which will always result
> in the version being in the right byte order to use comparative operators
> in a meaningful way.

How is this described in the datasheet? And is version special, or do
all registers need swapping?

	Andrew

On Thu, Aug 21, 2025 at 04:58:23AM +0200, Andrew Lunn wrote:
> > > +	priv->version = le16_to_cpu((__le16 __force)version);
> > 
> > I've researched this a bit more and came to the conclusion that while the
> > above works fine because all Lantiq SoCs with built-in switch are
> > big-endian machines it is still wrong.
> > I base this conclusion on the fact that when dealing with more recent
> > MDIO-connected switches (MaxLinear GSW1xx series) the host endian doesn't
> > play a role in the driver -- when dealing with 16-bit values on the MDIO
> > bus, the bus abstraction takes care of converting from/to host endianess.
> 
> I agree that all MDIO bus registers are host endian, 16 bit. The shift
> register in the hardware is responsible for putting the bits on the
> wire in the correct order for MDIO.
> 
> > Hence I believe this should simply be a swab16() which will always result
> > in the version being in the right byte order to use comparative operators
> > in a meaningful way.
> 
> How is this described in the datasheet? And is version special, or do
> all registers need swapping?

The (anyway public) datasheets I have access to don't describe the VERSION
register at all. In the existing precompiler macros, however, you can see
that most-significant and least-significant byte are swapped. REV is
more significant than MOD:

#define GSWIP_VERSION                   0x013
#define  GSWIP_VERSION_REV_SHIFT        0
#define  GSWIP_VERSION_REV_MASK         GENMASK(7, 0)
#define  GSWIP_VERSION_MOD_SHIFT        8
#define  GSWIP_VERSION_MOD_MASK         GENMASK(15, 8)
#define   GSWIP_VERSION_2_0             0x100
#define   GSWIP_VERSION_2_1             0x021
#define   GSWIP_VERSION_2_2             0x122
#define   GSWIP_VERSION_2_2_ETC         0x022

Now I'd like to add
#define   GSWIP_VERSION_2_3             0x023

and then have a simple way to make features available starting from a
GSWIP_VERSION. Now in order for GSWIP_VERSION_2_3 to be greater than
GSWIP_VERSION_2_2, and GSWIP_VERSION_2_1 to be greater than
GSWIP_VERSION_2_0, the bytes need to be swapped.

I don't think the vendor even considered any specific order of the two
bytes but just defines them as separate 8-bit fields, considering it a
16-bit unsigned integer is my interpretation which came from the need for
comparability.

> The (anyway public) datasheets I have access to don't describe the VERSION
> register at all. In the existing precompiler macros, however, you can see
> that most-significant and least-significant byte are swapped. REV is
> more significant than MOD:
> 
> #define GSWIP_VERSION                   0x013
> #define  GSWIP_VERSION_REV_SHIFT        0
> #define  GSWIP_VERSION_REV_MASK         GENMASK(7, 0)
> #define  GSWIP_VERSION_MOD_SHIFT        8
> #define  GSWIP_VERSION_MOD_MASK         GENMASK(15, 8)
> #define   GSWIP_VERSION_2_0             0x100
> #define   GSWIP_VERSION_2_1             0x021
> #define   GSWIP_VERSION_2_2             0x122
> #define   GSWIP_VERSION_2_2_ETC         0x022
> 
> Now I'd like to add
> #define   GSWIP_VERSION_2_3             0x023
> 
> and then have a simple way to make features available starting from a
> GSWIP_VERSION. Now in order for GSWIP_VERSION_2_3 to be greater than
> GSWIP_VERSION_2_2, and GSWIP_VERSION_2_1 to be greater than
> GSWIP_VERSION_2_0, the bytes need to be swapped.
> 
> I don't think the vendor even considered any specific order of the two
> bytes but just defines them as separate 8-bit fields, considering it a
> 16-bit unsigned integer is my interpretation which came from the need for
> comparability.

It would be good to put some of this into the commit message and
comments in the code. That the hardware has the 'major/minor' version
bytes in the wrong order preventing numerical comparisons. To make it
obvious, rather than use swap16(), maybe actually extract the major
and minor, and then construct the version in the form you want?

    Andrew