Message-ID: <20251121203453.y6s46k4ttdtq5mgh@skbuf>
Date: Fri, 21 Nov 2025 22:34:53 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Prabhakar <prabhakar.csengg@...il.com>
Cc: Clément Léger <clement.leger@...tlin.com>,
	Andrew Lunn <andrew@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Simon Horman <horms@...nel.org>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	Russell King <linux@...linux.org.uk>,
	Geert Uytterhoeven <geert+renesas@...der.be>,
	Magnus Damm <magnus.damm@...il.com>,
	linux-renesas-soc@...r.kernel.org, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	Biju Das <biju.das.jz@...renesas.com>,
	Fabrizio Castro <fabrizio.castro.jz@...esas.com>,
	Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH net-next 07/11] net: dsa: rzn1-a5psw: Make switch
 topology configurable via OF data

On Fri, Nov 21, 2025 at 11:35:33AM +0000, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> 
> Move the switch topology description-the number of ports and the CPU-port
> index-out of hard-coded constants and into SoC-specific OF match data. The
> driver previously assumed a fixed 5-port layout with the last port acting
> as the CPU port. That assumption does not hold for newer Renesas variants,
> and embedding it in the code made the driver inflexible and error-prone.

That assumption has 2 parts: that the port has 5 ports, and that the
last port is the CPU port. It's unclear from your statement which part
does not hold. I see that for the new switches, the CPU port is still
the last port (not that there's a problem with still parameterizing it).

> 
> Introduce a small a5psw_of_data structure carrying both the total number
> of ports and the CPU-port identifier, and rely on this data everywhere the
> driver previously used fixed values. This ensures that port loops, PCS
> allocation, management-port setup, and bridge bookkeeping all reflect the
> actual hardware configuration.
> 
> Making these attributes runtime-selectable allows the driver to support
> RZ/T2H and RZ/N2H SoCs which use different port counts and CPU-port
> assignments-without rewriting common logic or forking the driver, while
> preserving correct behaviour on existing RZN1 systems.

The code is mostly fine, but reading the commit message had me jumping
or twitching any time I would see the words "configure" or "make attributes
runtime-selectable". These expressions have their own meanings having to
do with adding kernel APIs through which these parameters can be changed
(by the user), so I wasn't really sure what I was going to review. None
of that is the case, according to the code. Please choose other wording.
You're not making the driver attributes configurable, you're just
replacing constants hardcoded in the .text section with constants
hardcoded in structured data in the .rodata section, selected at probe
time based on compatible string.

I'm sorry for saying this, but the commit message is too long for the
amount of information that it transmits. You repeated 3 times the
properties that need to be parameterized (port count and CPU port index),
and there's more bla bla about irrelevant things like forking the driver.

The commit message has to serve as an aid in understanding the change
itself, not detract from it. In this case, giving the motivation and
context in one paragraph or two is fine, but then you can use the space
to focus on listing the transformations that need to be followed when
reviewing the patch, and if not obvious, explain what led to those
choices. What you want is obviously correct changes.

For example, why ARRAY_SIZE(a5psw->pcs) transforms into
a5psw->of_data->nports - 1. Is this the best choice? The code looks
worse, and it's not obvious that the last port would not have a PCS as a
matter of architecture. You had several other options: introduce an
"npcs" extra parameter, or even compare with "cpu_port" and place
comments explaining the lack of a PCS for the CPU port (since "cpu_port"
is "nports - 1").

> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> ---
>  drivers/net/dsa/rzn1_a5psw.c | 26 +++++++++++++++++---------
>  drivers/net/dsa/rzn1_a5psw.h | 17 ++++++++++++++---
>  2 files changed, 31 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c
> index 99098bc06efe..d957b6d40f05 100644
> --- a/drivers/net/dsa/rzn1_a5psw.c
> +++ b/drivers/net/dsa/rzn1_a5psw.c
> @@ -382,13 +382,14 @@ static void a5psw_port_bridge_leave(struct dsa_switch *ds, int port,
>  				    struct dsa_bridge bridge)
>  {
>  	struct a5psw *a5psw = ds->priv;
> +	unsigned int cpu_port = a5psw->of_data->cpu_port;
>  
>  	a5psw->bridged_ports &= ~BIT(port);
>  
>  	a5psw_port_set_standalone(a5psw, port, true);
>  
>  	/* No more ports bridged */
> -	if (a5psw->bridged_ports == BIT(A5PSW_CPU_PORT))
> +	if (a5psw->bridged_ports == BIT(cpu_port))
>  		a5psw->br_dev = NULL;
>  }
>  
> @@ -924,20 +925,21 @@ static void a5psw_vlan_setup(struct a5psw *a5psw, int port)
>  static int a5psw_setup(struct dsa_switch *ds)
>  {
>  	struct a5psw *a5psw = ds->priv;
> +	unsigned int cpu_port = a5psw->of_data->cpu_port;
>  	int port, vlan, ret;
>  	struct dsa_port *dp;
>  	u32 reg;
>  
> -	/* Validate that there is only 1 CPU port with index A5PSW_CPU_PORT */
> +	/* Validate that there is only 1 CPU port with index matching cpu_port */
>  	dsa_switch_for_each_cpu_port(dp, ds) {
> -		if (dp->index != A5PSW_CPU_PORT) {
> +		if (dp->index != cpu_port) {
>  			dev_err(a5psw->dev, "Invalid CPU port\n");
>  			return -EINVAL;
>  		}
>  	}
>  
>  	/* Configure management port */
> -	reg = A5PSW_CPU_PORT | A5PSW_MGMT_CFG_ENABLE;
> +	reg = cpu_port | A5PSW_MGMT_CFG_ENABLE;
>  	a5psw_reg_writel(a5psw, A5PSW_MGMT_CFG, reg);
>  
>  	/* Set pattern 0 to forward all frame to mgmt port */
> @@ -1147,7 +1149,7 @@ static void a5psw_pcs_free(struct a5psw *a5psw)
>  {
>  	int i;
>  
> -	for (i = 0; i < ARRAY_SIZE(a5psw->pcs); i++) {
> +	for (i = 0; i < a5psw->of_data->nports - 1; i++) {
>  		if (a5psw->pcs[i])
>  			miic_destroy(a5psw->pcs[i]);
>  	}
> @@ -1174,7 +1176,7 @@ static int a5psw_pcs_get(struct a5psw *a5psw)
>  			goto free_pcs;
>  		}
>  
> -		if (reg >= ARRAY_SIZE(a5psw->pcs)) {
> +		if (reg >= a5psw->of_data->nports - 1) {
>  			ret = -ENODEV;
>  			goto free_pcs;
>  		}
> @@ -1223,7 +1225,8 @@ static int a5psw_probe(struct platform_device *pdev)
>  	if (IS_ERR(a5psw->base))
>  		return PTR_ERR(a5psw->base);
>  
> -	a5psw->bridged_ports = BIT(A5PSW_CPU_PORT);
> +	a5psw->of_data = of_device_get_match_data(dev);
> +	a5psw->bridged_ports = BIT(a5psw->of_data->cpu_port);
>  
>  	ret = a5psw_pcs_get(a5psw);
>  	if (ret)
> @@ -1268,7 +1271,7 @@ static int a5psw_probe(struct platform_device *pdev)
>  
>  	ds = &a5psw->ds;
>  	ds->dev = dev;
> -	ds->num_ports = A5PSW_PORTS_NUM;
> +	ds->num_ports = a5psw->of_data->nports;
>  	ds->ops = &a5psw_switch_ops;
>  	ds->phylink_mac_ops = &a5psw_phylink_mac_ops;
>  	ds->priv = a5psw;
> @@ -1310,8 +1313,13 @@ static void a5psw_shutdown(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, NULL);
>  }
>  
> +static const struct a5psw_of_data rzn1_of_data = {
> +	.nports = 5,
> +	.cpu_port = 4,
> +};
> +
>  static const struct of_device_id a5psw_of_mtable[] = {
> -	{ .compatible = "renesas,rzn1-a5psw", },
> +	{ .compatible = "renesas,rzn1-a5psw", .data = &rzn1_of_data },
>  	{ /* sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, a5psw_of_mtable);
> diff --git a/drivers/net/dsa/rzn1_a5psw.h b/drivers/net/dsa/rzn1_a5psw.h
> index 81be30d6c55f..d1b2cc5b43e6 100644
> --- a/drivers/net/dsa/rzn1_a5psw.h
> +++ b/drivers/net/dsa/rzn1_a5psw.h
> @@ -195,8 +195,7 @@
>  #define A5PSW_aCarrierSenseErrors		0x924
>  
>  #define A5PSW_VLAN_TAG(prio, id)	(((prio) << 12) | (id))
> -#define A5PSW_PORTS_NUM			5
> -#define A5PSW_CPU_PORT			(A5PSW_PORTS_NUM - 1)
> +#define A5PSW_MAX_PORTS			4

Poor naming choice - it makes nports larger than A5PSW_MAX_PORTS, which
according to their name should be directly comparable.

Perhaps A5PSW_MAX_NUM_PCS (a comment explaining the relationship with
the CPU port would be good).

>  #define A5PSW_MDIO_DEF_FREQ		2500000
>  #define A5PSW_MDIO_TIMEOUT		100
>  #define A5PSW_JUMBO_LEN			(10 * SZ_1K)
> @@ -231,6 +230,16 @@ union lk_data {
>  	struct fdb_entry entry;
>  };
>  
> +/**
> + * struct a5psw_of_data - OF data structure
> + * @nports: Number of ports in the switch
> + * @cpu_port: CPU port number
> + */
> +struct a5psw_of_data {
> +	unsigned int nports;
> +	unsigned int cpu_port;
> +};
> +
>  /**
>   * struct a5psw - switch struct
>   * @base: Base address of the switch
> @@ -238,6 +247,7 @@ union lk_data {
>   * @clk: clk_switch clock
>   * @ts: Timestamp clock
>   * @dev: Device associated to the switch
> + * @of_data: Pointer to OF data
>   * @mii_bus: MDIO bus struct
>   * @mdio_freq: MDIO bus frequency requested
>   * @pcs: Array of PCS connected to the switch ports (not for the CPU)
> @@ -254,8 +264,9 @@ struct a5psw {
>  	struct clk *clk;
>  	struct clk *ts;
>  	struct device *dev;
> +	const struct a5psw_of_data *of_data;
>  	struct mii_bus	*mii_bus;
> -	struct phylink_pcs *pcs[A5PSW_PORTS_NUM - 1];
> +	struct phylink_pcs *pcs[A5PSW_MAX_PORTS];
>  	struct dsa_switch ds;
>  	struct mutex lk_lock;
>  	spinlock_t reg_lock;
> -- 
> 2.52.0
> 

Powered by blists - more mailing lists