Message-ID: <CA+V-a8sRr9FzR8pCXJbeuyidv+zVLykhPnj_71zuEGf8U4xS7A@mail.gmail.com>
Date: Fri, 21 Nov 2025 21:41:18 +0000
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Vladimir Oltean <olteanv@...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

Hi Vladimir,

Thank you for the review.

On Fri, Nov 21, 2025 at 8:34 PM Vladimir Oltean <olteanv@...il.com> wrote:
>
> 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.
>
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@...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

Powered by blists - more mailing lists