Re: [PATCH net-next v7 4/9] net: dsa: lan9645x: add basic dsa driver for LAN9645X

[Jens Emil Schulz Ostergaard <jensemil.schulzostergaard@microchip.com>]

On Tue, 2026-06-09 at 10:48 +0200, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Tue, Jun 09, 2026 at 09:08:13AM +0200, Jens Emil Schulz Ostergaard wrote:
> > On Mon, 2026-06-08 at 20:00 +0200, Andrew Lunn wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > > 
> > > > +     dsa_switch_for_each_user_port(dp, ds) {
> > > > +             if (dp->cpu_dp->ds != ds) {
> > > > +                     dev_err(ds->dev,
> > > > +                             "NPI port on a remote switch is not supported\n");
> > > > +                     return -EINVAL;
> > > > +             }
> > > > +
> > > > +             if (first_cpu_dp && dp->cpu_dp != first_cpu_dp) {
> > > > +                     dev_err(ds->dev, "Multiple NPI ports not supported\n");
> > > > +                     return -EINVAL;
> > > > +             }
> > > > +
> > > > +             first_cpu_dp = dp->cpu_dp;
> > > 
> > > The reason i asked about NPI ports is because this is looping over
> > > user ports. Yet you say one of these user ports is a CPU port. That
> > > cannot be correct.
> > > 
> > > The first port returned by dsa_tree_for_each_cpu_port() would be
> > > first_cpu_dp.
> > > 
> > >         Andrew
> > 
> > 
> > I tried to mimic the approach in drivers/net/dsa/ocelot/felix.c:
> > 
> > static int felix_tag_npi_setup(struct dsa_switch *ds)
> > {
> >       struct dsa_port *dp, *first_cpu_dp = NULL;
> >       struct ocelot *ocelot = ds->priv;
> > 
> >       dsa_switch_for_each_user_port(dp, ds) {
> >               if (first_cpu_dp && dp->cpu_dp != first_cpu_dp) {
> >                       dev_err(ds->dev, "Multiple NPI ports not supported\n");
> >                       return -EINVAL;
> >               }
> > 
> >               first_cpu_dp = dp->cpu_dp;
> >       }
> > 
> >       if (!first_cpu_dp)
> >               return -EINVAL;
> > 
> >       felix_npi_port_init(ocelot, first_cpu_dp->index);
> > 
> >       return 0;
> > }
> > 
> > Perhaps I misunderstand you, but there could be confusion about
> > terminology. The chip designers have a concept of CPU port, which is
> > used liberally in the datasheet, and in this driver code.
> 
> > However, the concept is different from the DSA concept of a CPU port.
> 
> And that is a problem because somebody reviewing this code is likely
> to know DSA concepts much more than the individual devices concepts.
> To aid overall Maintenance of all the DSA drivers, the driver should
> try to keep with DSA meanings.

I have tried to explain the relationship between switch CPU port and NPI
port in comments, but I am sure it could be clearer. I am also guilty of
thinking in terms of these concepts because it is how the terms are used
in the office, in the registers and datasheet and so on. It is hard
writing a driver without knowing this distinction.


> 
> > Let us call the first switch CPU port, and the second DSA CPU port.
> > 
> > The port we want to use as a DSA CPU port, i.e. the port with the
> > 'ethernet = <&host_port>;' property in the device tree, will be
> > configured to be an NPI port for injection/extraction for the switch CPU
> > port (which is not a physical port on the device).
> 
> > Therefore, this NPI port is not iterated by
> > dsa_switch_for_each_user_port. The switch CPU port (index 9) is also not
> > iterated by it. It is a chip internal construct with no representation
> > in the device tree, and no struct dsa_port.
> 
> So first_cpu_dp is not a DSA CPU port. It is also not a NAPI port,
> since that is a DSA CPU port. Then what is it? Why do you need the
> concept of a switch CPU port?
> 
>         Andrew

But first_cpu_dp _is_ a DSA CPU port (and the NPI port). What I meant is
that we are iterating user ports with dsa_switch_for_each_user_port, so
the iteration variable dp is never a DSA CPU port (and therefore not the
NPI port). But we set first_cpu_dp = dp->cpu_dp, not first_cpu_dp = dp,
so first_cpu_dp is indeed a DSA CPU port.

We find it here so we can pass it to lan9645x_npi_port_init to configure
the port module at index first_cpu_dp->index as an NPI port. In this
particular function, the switch CPU port does not occur or play a role,
I was just trying to explain the relationship in general.

Thanks,
Emil