Re: [PATCH net-next v4 11/11] net: dsa: realtek: rtl8365mb: multiple cpu ports, non cpu extint

["Alvin Šipraga" <ALSI@bang-olufsen.dk>]

Luiz Angelo Daros de Luca <luizluca@gmail.com> writes:

> Now CPU port is not limited to a single port. Also, extint can be used
> as non-cpu ports, as long as it defines relatek,ext-int. The last cpu
> port will be used as trap_port.
>
> The CPU information was dropped from chip data as it was not used
> outside setup. The only other place it was used is when it wrongly
> checks for CPU port when it should check for extint.
>
> realtek_priv->cpu_port is now only used by rtl8366rb.c

Great work with this series! If I understood correctly from your last
emails, you weren't actually able to test this due to hardware
constraints. While I think this change is not going to introduce any
surprises, I think you should still mention that it is not tested.

Some more comments below but in general the change makes sense to me.

>
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> ---
>  drivers/net/dsa/realtek/rtl8365mb.c | 53 +++++++++++++++--------------
>  1 file changed, 27 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> index 59e08b192c06..6a00a162b2ac 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> @@ -556,7 +556,6 @@ struct rtl8365mb_port {
>   * @chip_ver: chip silicon revision
>   * @port_mask: mask of all ports
>   * @learn_limit_max: maximum number of L2 addresses the chip can learn
> - * @cpu: CPU tagging and CPU port configuration for this chip
>   * @mib_lock: prevent concurrent reads of MIB counters
>   * @ports: per-port data
>   * @jam_table: chip-specific initialization jam table
> @@ -571,7 +570,6 @@ struct rtl8365mb {
>  	u32 chip_ver;
>  	u32 port_mask;
>  	u32 learn_limit_max;
> -	struct rtl8365mb_cpu cpu;
>  	struct mutex mib_lock;
>  	struct rtl8365mb_port ports[RTL8365MB_MAX_NUM_PORTS];
>  	const struct rtl8365mb_jam_tbl_entry *jam_table;
> @@ -769,17 +767,20 @@ static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port,
>  	u32 val;
>  	int ret;
>  
> -	if (port != priv->cpu_port) {
> -		dev_err(priv->dev, "only one EXT interface is currently supported\n");
> +	mb = priv->chip_data;
> +	p = &mb->ports[port];
> +	ext_int = p->ext_int;
> +
> +	if (ext_int == RTL8365MB_NOT_EXT) {
> +		dev_err(priv->dev,
> +			"Port %d is not identified as extenal interface.\n",

Maybe just a warning?
also: s/as extenal/as an external/

> +			port);
>  		return -EINVAL;
>  	}
>  
>  	dp = dsa_to_port(priv->ds, port);
>  	dn = dp->dn;
>  
> -	mb = priv->chip_data;
> -	p = &mb->ports[port];
> -	ext_int = p->ext_int;
>  
>  	/* Set the RGMII TX/RX delay
>  	 *
> @@ -859,15 +860,17 @@ static int rtl8365mb_ext_config_forcemode(struct realtek_priv *priv, int port,
>  	int val;
>  	int ret;
>  
> -	if (port != priv->cpu_port) {
> -		dev_err(priv->dev, "only one EXT interface is currently supported\n");
> -		return -EINVAL;
> -	}
> -
>  	mb = priv->chip_data;
>  	p = &mb->ports[port];
>  	ext_int = p->ext_int;
>  
> +	if (ext_int == RTL8365MB_NOT_EXT) {
> +		dev_err(priv->dev,
> +			"Port %d is not identified as extenal interface.\n",

ditto

> +			port);
> +		return -EINVAL;
> +	}
> +
>  	if (link) {
>  		/* Force the link up with the desired configuration */
>  		r_link = 1;
> @@ -1734,10 +1737,8 @@ static void rtl8365mb_irq_teardown(struct realtek_priv *priv)
>  	}
>  }
>  
> -static int rtl8365mb_cpu_config(struct realtek_priv *priv)
> +static int rtl8365mb_cpu_config(struct realtek_priv *priv, struct rtl8365mb_cpu *cpu)

const struct rtl8365mb_cpu?

>  {
> -	struct rtl8365mb *mb = priv->chip_data;
> -	struct rtl8365mb_cpu *cpu = &mb->cpu;
>  	u32 val;
>  	int ret;
>  
> @@ -1839,11 +1840,17 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
>  		dev_info(priv->dev, "no interrupt support\n");
>  
>  	/* Configure CPU tagging */
> +	cpu.mask = 0;

I guess the unused cpu variable in the earlier patch belongs in this
one, in which case you can just initialize it = { 0 } so that you don't
need to explicitly set cpu.mask = 0.

>  	dsa_switch_for_each_cpu_port(cpu_dp, priv->ds) {
> -		priv->cpu_port = cpu_dp->index;
> -		mb->cpu.mask = BIT(priv->cpu_port);
> -		mb->cpu.trap_port = priv->cpu_port;
> -		ret = rtl8365mb_cpu_config(priv);
> +		cpu.enable = 1;
> +		cpu.insert = RTL8365MB_CPU_INSERT_TO_ALL;
> +		cpu.position = RTL8365MB_CPU_POS_AFTER_SA;
> +		cpu.rx_length = RTL8365MB_CPU_RXLEN_64BYTES;
> +		cpu.format = RTL8365MB_CPU_FORMAT_8BYTES;
> +		cpu.trap_port = cpu_dp->index;

If you are going to do this, perhaps it's better specified as a device
tree property like the external interface index? Making the "last" CPU
port the trap port is not incorrect, but it seems quite arbitrary.

> +		cpu.mask |= BIT(cpu_dp->index);
> +
> +		ret = rtl8365mb_cpu_config(priv, &cpu);

Shouldn't this go outside the loop to avoid potentially calling it twice
in a row?

>  		if (ret)
>  			goto out_teardown_irq;
>  
> @@ -1862,7 +1869,7 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
>  		dn = dsa_to_port(priv->ds, i)->dn;
>  
>  		/* Forward only to the CPU */
> -		ret = rtl8365mb_port_set_isolation(priv, i, BIT(priv->cpu_port));
> +		ret = rtl8365mb_port_set_isolation(priv, i, cpu.mask);
>  		if (ret)
>  			goto out_teardown_irq;
>  
> @@ -2003,12 +2010,6 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
>  		mb->jam_table = rtl8365mb_init_jam_8365mb_vc;
>  		mb->jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc);
>  
> -		mb->cpu.enable = 1;
> -		mb->cpu.insert = RTL8365MB_CPU_INSERT_TO_ALL;
> -		mb->cpu.position = RTL8365MB_CPU_POS_AFTER_SA;
> -		mb->cpu.rx_length = RTL8365MB_CPU_RXLEN_64BYTES;
> -		mb->cpu.format = RTL8365MB_CPU_FORMAT_8BYTES;
> -
>  		break;
>  	default:
>  		dev_err(priv->dev,