Re: [PATCH net-next v2 2/3] dsa: mv88e6xxx: Add support for RMU in select switches

[Mattias Forsblad <mattias.forsblad@gmail.com>]

On 2022-08-30 18:42, Vladimir Oltean wrote:
> On Tue, Aug 30, 2022 at 07:35:15PM +0300, Vladimir Oltean wrote:
>>> +void mv88e6xxx_rmu_master_change(struct dsa_switch *ds, const struct net_device *master,
>>> +				 bool operational)
>>> +{
>>> +	struct mv88e6xxx_chip *chip = ds->priv;
>>> +
>>> +	if (operational)
>>> +		chip->rmu.ops = &mv88e6xxx_bus_ops;
>>> +	else
>>> +		chip->rmu.ops = NULL;
>>> +}
>>
>> There is a subtle but very important point to be careful about here,
>> which is compatibility with multiple CPU ports. If there is a second DSA
>> master whose state flaps from up to down, this should not affect the
>> fact that you can still use RMU over the first DSA master. But in your
>> case it does, so this is a case of how not to write code that accounts
>> for that.
>>
>> In fact, given this fact, I think your function prototypes for
>> chip->info->ops->rmu_enable() are all wrong / not sufficiently
>> reflective of what the hardware can do. If the hardware has a bit mask
>> of ports on which RMU operations are possible, why hardcode using
>> dsa_switch_upstream_port() and not look at which DSA masters/CPU ports
>> are actually up? At least for the top-most switch. For downstream
>> switches we can use dsa_switch_upstream_port(), I guess (even that can
>> be refined, but I'm not aware of setups using multiple DSA links, where
>> each DSA link ultimately goes to a different upstream switch).
> 
> Hit "send" too soon. Wanted to give the extra hint that the "master"
> pointer is given to you here for a reason. You can look at struct
> dsa_port *cpu_dp = master->dsa_ptr, and figure out the index of the CPU

Would this work on a system where there are multiple switches? I.e.

SOC <->port6 SC#1 <->port10 SC#2

Both have the same master interface (chan0) which gives the same
cpu_dp->dsa_ptr->index but they have different upstream ports that should 
be enabled for RMU.

> port which can be used for RMU operations. I see that the macros are
> constructed in a very strange way:
> 
> #define MV88E6352_G1_CTL2_RMU_MODE_DISABLED	0x0000
> #define MV88E6352_G1_CTL2_RMU_MODE_PORT_4	0x1000
> #define MV88E6352_G1_CTL2_RMU_MODE_PORT_5	0x2000
> #define MV88E6352_G1_CTL2_RMU_MODE_PORT_6	0x3000
> 
> it's as if this is actually a bit mask of ports, and they all can be
> combined together. The bit in G1_CTL2 whose state we can flip can be
> made to depend on the number of the CPU port attached to the DSA master
> which changed state.