Re: [PATCH net-next 07/11] net: dsa: rzn1-a5psw: Make switch topology configurable via OF data

Lad, Prabhakar
2025-11-21T21:41:45Z

Hi Vladimir,

Thank you for the review.

On Fri, Nov 21, 2025 at 8:34 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Fri, Nov 21, 2025 at 11:35:33AM +0000, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.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.
>
Thanks for the detailed feedback.

I understand your point regarding the terminology using phrases like
“configure” and “runtime-selectable” was misleading, and that wasn’t
my intention. I’ll revise the commit message to avoid implying
user-accessible configuration and instead describe the change more
accurately.

> 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").
>
Agreed, I'll rework on it.

> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.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).
>
Agreed, I will rename it.

Cheers,
Prabhakar