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:

quoted

Instead of a fixed CPU port, assume that DSA is correct.

Tested-by: Arınç ÜNAL <redacted>
Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.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.

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.

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.

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. :-)

quoted

  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)

quoted

+		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