Date:   Tue, 28 Dec 2021 05:48:32 -0300
From:   Luiz Angelo Daros de Luca <luizluca@...il.com>
To:     Alvin Šipraga <ALSI@...g-olufsen.dk>
Cc:     Vladimir Oltean <olteanv@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "vivien.didelot@...il.com" <vivien.didelot@...il.com>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "arinc.unal@...nc9.com" <arinc.unal@...nc9.com>
Subject: Re: [PATCH net-next v2 11/13] net: dsa: realtek: rtl8365mb: use DSA
 CPU port

> On 12/19/21 23:19, Vladimir Oltean wrote:
> > On Sat, Dec 18, 2021 at 05:14:23AM -0300, Luiz Angelo Daros de Luca wrote:
> >> Instead of a fixed CPU port, assume that DSA is correct.
> >>
> >> Tested-by: Arınç ÜNAL <arinc.unal@...nc9.com>
> >> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@...il.com>
> >> ---
> >
> > I don't necessarily see the value added by this change. Since (I think)
> > only a single port can be a CPU port, a sanity check seems to be missing
> > that the CPU port in the device tree is the expected one. This seems to
> > be missing with or without your patch. You are unnaturally splitting the
> > initialization of a data structure between rtl8365mb_setup() and
> > rtl8365mb_detect(). Maybe what you should do is keep everything in
> > rtl8365mb_detect() where it is right now, and check in rtl8365mb_setup
> > that the cpu_dp->index is equal to priv->cpu_port?
>
> I'm quite sure the switch family does actually support multiple CPU
> ports. If you have a cascaded switch, CPU-tagged frames may pass between
> the external ports of the switches. Any port can be configured to parse
> CPU-tagged frames. And the CPU port configuration register allows for a
> mask of EXT ports to be configured for trapping.

Yes, you could have multiple CPU ports (it is a mask) and multiple
external ports (which might or not be a CPU port). Even a user port
can be defined as a CPU port, delegating the flow control to an
external device connected via ethernet ports. I don't want to assume
that the CPU port is always an external port, although it is almost
always the case.

Even though external ports are normally used as CPU ports, they might
also be used to connect to other switches or not connected at all. I
cannot assume that all Realtek external ports are CPU ports. Before
this patch, the CPU port was hardcoded. It will not work with any
dual-external port chip variant.

>
> However, this change requires a more thorough explanation of what it is
> trying to achieve. I already see that Vladimir is confused. The control
> flow also looks rather strange.
>
> If I am to guess what deficiency you are trying to address, it's that
> the driver assumes that the CPU port is the EXT port; since there is
> only one possible EXT port, this is hardcoded with
> RTL8365MB_CPU_PORT_NUM_8365MB_VC = 6. But now your new switch actually
> has _two_ EXT ports. Which of those is the CPU port is configured by
> setting the realtek,ext-int property in the device tree node of the CPU
> port. But that means that the CPU port cannot be hardcoded. So you want
> to get this information from DSA instead.

Instead of using a new custom property to define if a port should be
configured as a Realtek CPU port, I'm just assuming that the only port
that should be configured as a (Realtek) CPU port is the DSA CPU port.
I just added an extra property to map external port to port number as
I don't know if there is a general rule for mapping between these two
values like number_of_user_ports (2..7)+ext_number (0,1,2).

>
> Similar to my comment to another patch in your series, I think it's
> worth making the driver support multiple EXT interfaces. Then it should
> be clearer in the series why you are making these changes.

I didn't change a driver limitation of only configuring an ext port if
it is a CPU port. However, I think it might just need to drop the
check. I'm not confident to implement any new feature that I cannot
test in real HW. This is also the case for SGMII. My chip does have
multiple external ports but the SGMII is not connected.

>
> Please consider also Vladimir's point about unnaturally splitting code.
> I can see it elsewhere in the series a little bit too. It is nice to
> keep the structure of the driver consistent - at least while it is still
> young and innocent. :-)

If I'm using DSA CPU information to define the Realtek CPU port I
either need to read that information walking DSA device-tree nodes or
doing that after DSA has already parsed it. That's why it went to
setup instead of detect, where DSA info is already available and where
the CPU settings are actually used. I could even drop rtl8365mb_cpu
from priv->chip_data as it is used only at that step. I could go even
further and drop cpu_port value from priv as all other users of cpu
information are based on the false assumption that ext port == cpu
port.

We do need a way to tell if a port number is an external port and
which one it is. It would also be nice to warn the user their DSA is
mentioning a non-existing port.

>
> >
> >>   drivers/net/dsa/realtek/rtl8365mb.c | 23 ++++++++++++++---------
> >>   1 file changed, 14 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> >> index a8f44538a87a..b79a4639b283 100644
> >> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> >> +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> >> @@ -103,14 +103,13 @@
> >>
> >>   /* Chip-specific data and limits */
> >>   #define RTL8365MB_CHIP_ID_8365MB_VC                0x6367
> >> -#define RTL8365MB_CPU_PORT_NUM_8365MB_VC    6
> >>   #define RTL8365MB_LEARN_LIMIT_MAX_8365MB_VC        2112
> >>
> >>   /* Family-specific data and limits */
> >>   #define RTL8365MB_PHYADDRMAX       7
> >>   #define RTL8365MB_NUM_PHYREGS      32
> >>   #define RTL8365MB_PHYREGMAX        (RTL8365MB_NUM_PHYREGS - 1)
> >> -#define RTL8365MB_MAX_NUM_PORTS     (RTL8365MB_CPU_PORT_NUM_8365MB_VC + 1)
> >> +#define RTL8365MB_MAX_NUM_PORTS  7
> >>
> >>   /* Chip identification registers */
> >>   #define RTL8365MB_CHIP_ID_REG              0x1300
> >> @@ -1827,9 +1826,18 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
> >>              dev_info(priv->dev, "no interrupt support\n");
> >>
> >>      /* Configure CPU tagging */
> >> -    ret = rtl8365mb_cpu_config(priv);
> >> -    if (ret)
> >> -            goto out_teardown_irq;
> >> +    for (i = 0; i < priv->num_ports; i++) {
> >> +            if (!(dsa_is_cpu_port(priv->ds, i)))
> >> +                    continue;
> >
> > dsa_switch_for_each_cpu_port(cpu_dp, ds)
> >
> >> +            priv->cpu_port = i;
> >> +            mb->cpu.mask = BIT(priv->cpu_port);
> >> +            mb->cpu.trap_port = priv->cpu_port;
> >> +            ret = rtl8365mb_cpu_config(priv);
> >> +            if (ret)
> >> +                    goto out_teardown_irq;
> >> +
> >> +            break;
> >> +    }
> >>
> >>      /* Configure ports */
> >>      for (i = 0; i < priv->num_ports; i++) {
> >> @@ -1960,8 +1968,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
> >>                       "found an RTL8365MB-VC switch (ver=0x%04x)\n",
> >>                       chip_ver);
> >>
> >> -            priv->cpu_port = RTL8365MB_CPU_PORT_NUM_8365MB_VC;
> >> -            priv->num_ports = priv->cpu_port + 1;
> >> +            priv->num_ports = RTL8365MB_MAX_NUM_PORTS;
> >>
> >>              mb->priv = priv;
> >>              mb->chip_id = chip_id;
> >> @@ -1972,8 +1979,6 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
> >>              mb->jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc);
> >>
> >>              mb->cpu.enable = 1;
> >> -            mb->cpu.mask = BIT(priv->cpu_port);
> >> -            mb->cpu.trap_port = priv->cpu_port;
> >>              mb->cpu.insert = RTL8365MB_CPU_INSERT_TO_ALL;
> >>              mb->cpu.position = RTL8365MB_CPU_POS_AFTER_SA;
> >>              mb->cpu.rx_length = RTL8365MB_CPU_RXLEN_64BYTES;
> >> --
> >> 2.34.0
> >>
>

Powered by blists - more mailing lists