[PATCH RFC net-next 06/23] net: dsa: lantiq_gswip: load model-specific microcode

[PATCH RFC net-next 06/23] net: dsa: lantiq_gswip: load model-specific microcode

[Daniel Golle]

Load microcode as specified in struct hw_info instead of relying on
a single array of instructions. This allows loading different microcode
for the MaxLinear GSW1xx family.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/dsa/lantiq_gswip.c | 14 +++++++++-----
 drivers/net/dsa/lantiq_gswip.h |  9 +++++++++
 drivers/net/dsa/lantiq_pce.h   |  9 ++-------
 3 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index 86e02ac0c221..355b8eb8ab9b 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -501,15 +501,15 @@ static int gswip_pce_load_microcode(struct gswip_priv *priv)
 			  GSWIP_PCE_TBL_CTRL_OPMOD_ADWR, GSWIP_PCE_TBL_CTRL);
 	gswip_switch_w(priv, 0, GSWIP_PCE_TBL_MASK);
 
-	for (i = 0; i < ARRAY_SIZE(gswip_pce_microcode); i++) {
+	for (i = 0; i < priv->hw_info->pce_microcode_size; i++) {
 		gswip_switch_w(priv, i, GSWIP_PCE_TBL_ADDR);
-		gswip_switch_w(priv, gswip_pce_microcode[i].val_0,
+		gswip_switch_w(priv, (*priv->hw_info->pce_microcode)[i].val_0,
 			       GSWIP_PCE_TBL_VAL(0));
-		gswip_switch_w(priv, gswip_pce_microcode[i].val_1,
+		gswip_switch_w(priv, (*priv->hw_info->pce_microcode)[i].val_1,
 			       GSWIP_PCE_TBL_VAL(1));
-		gswip_switch_w(priv, gswip_pce_microcode[i].val_2,
+		gswip_switch_w(priv, (*priv->hw_info->pce_microcode)[i].val_2,
 			       GSWIP_PCE_TBL_VAL(2));
-		gswip_switch_w(priv, gswip_pce_microcode[i].val_3,
+		gswip_switch_w(priv, (*priv->hw_info->pce_microcode)[i].val_3,
 			       GSWIP_PCE_TBL_VAL(3));
 
 		/* start the table access: */
@@ -2011,6 +2011,8 @@ static const struct gswip_hw_info gswip_xrx200 = {
 	.phy_ports = BIT(2) | BIT(3) | BIT(4) | BIT(5),
 	.mii_ports = BIT(0) | BIT(5),
 	.phylink_get_caps = gswip_xrx200_phylink_get_caps,
+	.pce_microcode = &gswip_pce_microcode,
+	.pce_microcode_size = ARRAY_SIZE(gswip_pce_microcode),
 };
 
 static const struct gswip_hw_info gswip_xrx300 = {
@@ -2019,6 +2021,8 @@ static const struct gswip_hw_info gswip_xrx300 = {
 	.phy_ports = BIT(1) | BIT(2) | BIT(3) | BIT(4) | BIT(5),
 	.mii_ports = BIT(0) | BIT(1) | BIT(5),
 	.phylink_get_caps = gswip_xrx300_phylink_get_caps,
+	.pce_microcode = &gswip_pce_microcode,
+	.pce_microcode_size = ARRAY_SIZE(gswip_pce_microcode),
 };
 
 static const struct of_device_id gswip_of_match[] = {
diff --git a/drivers/net/dsa/lantiq_gswip.h b/drivers/net/dsa/lantiq_gswip.h
index 3b19963f2073..e1384b22bafa 100644
--- a/drivers/net/dsa/lantiq_gswip.h
+++ b/drivers/net/dsa/lantiq_gswip.h
@@ -214,12 +214,21 @@
  */
 #define GSWIP_MAX_PACKET_LENGTH	2400
 
+struct gswip_pce_microcode {
+	u16 val_3;
+	u16 val_2;
+	u16 val_1;
+	u16 val_0;
+};
+
 struct gswip_hw_info {
 	int max_ports;
 	unsigned int allowed_cpu_ports;
 	unsigned int phy_ports;
 	unsigned int mii_ports;
 	unsigned int sgmii_ports;
+	const struct gswip_pce_microcode (*pce_microcode)[];
+	size_t pce_microcode_size;
 	void (*phylink_get_caps)(struct dsa_switch *ds, int port,
 				 struct phylink_config *config);
 };
diff --git a/drivers/net/dsa/lantiq_pce.h b/drivers/net/dsa/lantiq_pce.h
index e2be31f3672a..659f9a0638d9 100644
--- a/drivers/net/dsa/lantiq_pce.h
+++ b/drivers/net/dsa/lantiq_pce.h
@@ -7,6 +7,8 @@
  * Copyright (C) 2017 - 2018 Hauke Mehrtens <hauke@hauke-m.de>
  */
 
+#include "lantiq_gswip.h"
+
 enum {
 	OUT_MAC0 = 0,
 	OUT_MAC1,
@@ -74,13 +76,6 @@ enum {
 	FLAG_NO,	/*13*/
 };
 
-struct gswip_pce_microcode {
-	u16 val_3;
-	u16 val_2;
-	u16 val_1;
-	u16 val_0;
-};
-
 #define MC_ENTRY(val, msk, ns, out, len, type, flags, ipv4_len) \
 	{ val, msk, ((ns) << 10 | (out) << 4 | (len) >> 1),\
 		((len) & 1) << 15 | (type) << 13 | (flags) << 9 | (ipv4_len) << 8 }
-- 
2.50.1

Re: [PATCH RFC net-next 06/23] net: dsa: lantiq_gswip: load model-specific microcode

[Andrew Lunn]

>  
> +struct gswip_pce_microcode {
> +	u16 val_3;
> +	u16 val_2;
> +	u16 val_1;
> +	u16 val_0;
> +};
> +

I would leave this where it is, and just have

struct gswip_pce_microcode;

Since only a pointer is needed, the compiler does not need the full
type info, at this point.

The structure itself is rather opaque, and only makes some sort of
sense when next to the MAC_ENTRY macro.

    Andrew

---
pw-bot: cr
	

Re: [PATCH RFC net-next 06/23] net: dsa: lantiq_gswip: load model-specific microcode

[Daniel Golle]

On Sun, Aug 17, 2025 at 05:29:16PM +0200, Andrew Lunn wrote:
> >  
> > +struct gswip_pce_microcode {
> > +	u16 val_3;
> > +	u16 val_2;
> > +	u16 val_1;
> > +	u16 val_0;
> > +};
> > +
> 
> I would leave this where it is, and just have
> 
> struct gswip_pce_microcode;
> 
> Since only a pointer is needed, the compiler does not need the full
> type info, at this point.
> 
> The structure itself is rather opaque, and only makes some sort of
> sense when next to the MAC_ENTRY macro.

The structure is also used in the function gswip_pce_load_microcode().
Now, if we keep defining the struct fields along with the microcode this
will become a problem once there is more than one such set of microcode
instructions and additional header files for them. Each of them would
need to define struct gswip_pce_microcode with its fields.
The lantiq_pce.h header then becomes private to the driver for the
in-SoC switches, while gswip_pce_load_microcode() would be part of the
shared/common module use by both, in-SoC/MMIO switches as well as
(newer) MDIO-connected ones, and I would not want to include any of the
*_pce.h headers in the shared/common module.

Obviously I can just move the struct definition in the later commit
which actually separates the MMIO-specific parts of the driver and the
common/shared parts into different modules. Is it that what you had in
mind?

Re: [PATCH RFC net-next 06/23] net: dsa: lantiq_gswip: load model-specific microcode

[Daniel Golle]

On Sun, Aug 17, 2025 at 09:46:51PM +0100, Daniel Golle wrote:
> On Sun, Aug 17, 2025 at 05:29:16PM +0200, Andrew Lunn wrote:
> > >  
> > > +struct gswip_pce_microcode {
> > > +	u16 val_3;
> > > +	u16 val_2;
> > > +	u16 val_1;
> > > +	u16 val_0;
> > > +};
> > > +
> > 
> > I would leave this where it is, and just have
> > 
> > struct gswip_pce_microcode;
> > 
> > Since only a pointer is needed, the compiler does not need the full
> > type info, at this point.
> > 
> > The structure itself is rather opaque, and only makes some sort of
> > sense when next to the MAC_ENTRY macro.
> 
> The structure is also used in the function gswip_pce_load_microcode().
> Now, if we keep defining the struct fields along with the microcode this
> will become a problem once there is more than one such set of microcode
> instructions and additional header files for them. Each of them would
> need to define struct gswip_pce_microcode with its fields.
> The lantiq_pce.h header then becomes private to the driver for the
> in-SoC switches, while gswip_pce_load_microcode() would be part of the
> shared/common module use by both, in-SoC/MMIO switches as well as
> (newer) MDIO-connected ones, and I would not want to include any of the
> *_pce.h headers in the shared/common module.
> 
> Obviously I can just move the struct definition in the later commit
> which actually separates the MMIO-specific parts of the driver and the
> common/shared parts into different modules. Is it that what you had in
> mind?

I didn't consider that the size of the array elements needs to be known
when defining struct gswip_hw_info in lantiq_gswip.h.
So the only reasonable solution is to make also the definition of
struct gswip_pce_microcode into lantiq_gswip.h, so lantiq_pce.h won't
have to be included before or by lantiq_gswip.h itself.

In file included from drivers/net/dsa/lantiq_gswip.c:28:
drivers/net/dsa/lantiq_gswip.h:226:44: error: array type has incomplete element type 'struct gswip_pce_microcode'
  226 |         const struct gswip_pce_microcode (*pce_microcode)[];
      |                                            ^~~~~~~~~~~~~
 

Re: [PATCH RFC net-next 06/23] net: dsa: lantiq_gswip: load model-specific microcode

[Andrew Lunn]

> I didn't consider that the size of the array elements needs to be known
> when defining struct gswip_hw_info in lantiq_gswip.h.
> So the only reasonable solution is to make also the definition of
> struct gswip_pce_microcode into lantiq_gswip.h, so lantiq_pce.h won't
> have to be included before or by lantiq_gswip.h itself.

What i've done in the past is define a structure which describes the
firmware. Two members, a pointer to the list of values, and a length
of the list of values. You can construct this structure using
ARRAY_SIZE(), and export it. You should then be able to put the odd
val4, val3, val2, val1 structure, and the macro together in one header
file, and use it in two places to define the firmware blobs for the
different devices.

	  Andrew

Re: [PATCH RFC net-next 06/23] net: dsa: lantiq_gswip: load model-specific microcode

[Daniel Golle]

On Tue, Aug 19, 2025 at 03:17:31PM +0200, Andrew Lunn wrote:
> > I didn't consider that the size of the array elements needs to be known
> > when defining struct gswip_hw_info in lantiq_gswip.h.
> > So the only reasonable solution is to make also the definition of
> > struct gswip_pce_microcode into lantiq_gswip.h, so lantiq_pce.h won't
> > have to be included before or by lantiq_gswip.h itself.
> 
> What i've done in the past is define a structure which describes the
> firmware. Two members, a pointer to the list of values, and a length
> of the list of values. You can construct this structure using
> ARRAY_SIZE(), and export it.

It is true that with one more layer of indirection I could have a separate
(allocated) struct gswip_pce_firmware with two elements
	.pce_microcode = &gswip_pce_microcode,
	.pce_microcode_size = ARRAY_SIZE(gswip_pce_microcode),

However, see below why I don't think this would actually solve the whole
problem.

> You should then be able to put the odd
> val4, val3, val2, val1 structure, and the macro together in one header
> file, and use it in two places to define the firmware blobs for the
> different devices.

I think this is the root of the misunderstanding here.
The odd val4, val3, val2, val1 struct is not only used to define the
microcode instructions, but it is also used to load the microcode into
the hardware, see
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/dsa/lantiq_gswip.c?h=v6.17-rc2#n743

Now, as long as there is only one header lantiq_pce.h defining such
microcode along with the odd struct, all this is not a problem.
However, the problem arises once there is more than one such header,
which will be required to support the newer MaxLinear hardware.
Which of the two (lantiq_pce.h or gsw1xx_pce.h?) is suppose to define
the struct? Which of the two should be included in the share module
('lantiq_gswip_common') which will take care of loading the microcode
into the hardware? (imho: none of them, both headers are hardware
specific and should be included by the respective specific modules
for either switch family)