diff mbox series

[net-next,v2,11/13] net: dsa: realtek: rtl8365mb: use DSA CPU port

Message ID 20211218081425.18722-12-luizluca@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net-next,v2,01/13] dt-bindings: net: dsa: realtek-smi: mark unsupported switches | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter warning Series does not have a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 2 maintainers not CCed: kuba@kernel.org davem@davemloft.net
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 53 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Luiz Angelo Daros de Luca Dec. 18, 2021, 8:14 a.m. UTC 
Instead of a fixed CPU port, assume that DSA is correct.

Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/rtl8365mb.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

Comments

Vladimir Oltean Dec. 19, 2021, 10:19 p.m. UTC | #1 
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@arinc9.com>
> 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?

>  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
>
Alvin Šipraga Dec. 19, 2021, 11:19 p.m. UTC | #2 
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@arinc9.com>
>> 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. :-)

> 
>>   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
>>
Luiz Angelo Daros de Luca Dec. 28, 2021, 8:48 a.m. UTC | #3 
> 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@arinc9.com>
> >> 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.

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
> >>
>
diff mbox series

Patch

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;
+		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;