On 2022-08-30 18:42, Vladimir Oltean wrote:

On Tue, Aug 30, 2022 at 07:35:15PM +0300, Vladimir Oltean wrote:

quoted

quoted

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