Messages in this thread Patch in this message | | | Date | Sat, 4 Feb 2023 01:47:42 +0200 | From | Vladimir Oltean <> | Subject | Re: [RFC PATCH net-next 09/11] net: dsa: microchip: lan937x: update port membership with dsa port |
| |
On Thu, Feb 02, 2023 at 06:29:28PM +0530, Rakesh Sankaranarayanan wrote: > Like cpu port, cascaded port will act as host port in second switch. And > all ports from both switches should be able to forward packets to cascaded > ports. Add cascaded port (dev->dsa_port) to each port membership. > > Current design add bit map of user ports as cpu port membership. Include > cascaded port index as well to this group. > > Signed-off-by: Rakesh Sankaranarayanan <rakesh.sankaranarayanan@microchip.com> > --- > drivers/net/dsa/microchip/ksz_common.c | 7 ++++--- > drivers/net/dsa/microchip/lan937x_main.c | 2 +- > include/net/dsa.h | 15 +++++++++++++++ > 3 files changed, 20 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c > index 913296c5dd50..b8b7b5b7b52d 100644 > --- a/drivers/net/dsa/microchip/ksz_common.c > +++ b/drivers/net/dsa/microchip/ksz_common.c > @@ -1748,9 +1748,9 @@ static void ksz_get_strings(struct dsa_switch *ds, int port, > > static void ksz_update_port_member(struct ksz_device *dev, int port) > { > + u8 port_member = 0, cpu_port, dsa_port;
Don't need these unimaginative names ("cpu_port", "dsa_port" for what is actually a port *mask*). You can bitwise-OR the upstream port and the downstream cascade port directly into "port_member", and the code in ksz_update_port_member() would tolerate it just fine.
> struct ksz_port *p = &dev->ports[port]; > struct dsa_switch *ds = dev->ds; > - u8 port_member = 0, cpu_port; > const struct dsa_port *dp; > int i, j; > > @@ -1759,6 +1759,7 @@ static void ksz_update_port_member(struct ksz_device *dev, int port) > > dp = dsa_to_port(ds, port); > cpu_port = BIT(dsa_upstream_port(ds, port)); > + dsa_port = BIT(dev->dsa_port);
If dev->dsa_port is 0xff, what is BIT(0xff)?
> > for (i = 0; i < ds->num_ports; i++) { > const struct dsa_port *other_dp = dsa_to_port(ds, i); > @@ -1798,10 +1799,10 @@ static void ksz_update_port_member(struct ksz_device *dev, int port) > val |= BIT(j); > } > > - dev->dev_ops->cfg_port_member(dev, i, val | cpu_port); > + dev->dev_ops->cfg_port_member(dev, i, val | cpu_port | dsa_port); > } > > - dev->dev_ops->cfg_port_member(dev, port, port_member | cpu_port); > + dev->dev_ops->cfg_port_member(dev, port, port_member | cpu_port | dsa_port); > } > > static int ksz_sw_mdio_read(struct mii_bus *bus, int addr, int regnum) > diff --git a/drivers/net/dsa/microchip/lan937x_main.c b/drivers/net/dsa/microchip/lan937x_main.c > index 5108a3f4bf76..b17bb1ea2a4a 100644 > --- a/drivers/net/dsa/microchip/lan937x_main.c > +++ b/drivers/net/dsa/microchip/lan937x_main.c > @@ -198,7 +198,7 @@ void lan937x_port_setup(struct ksz_device *dev, int port, bool cpu_port) > true); > > if (cpu_port) > - member = dsa_user_ports(ds); > + member = dsa_user_ports(ds) | dsa_dsa_ports(ds);
I'm wondering if a single dsa_switch_for_each_port() list traversal plus an "if ()" wouldn't be more efficient here.
> else > member = BIT(dsa_upstream_port(ds, port)); > > diff --git a/include/net/dsa.h b/include/net/dsa.h > index 55651ad29193..939aa6ff1a38 100644 > --- a/include/net/dsa.h > +++ b/include/net/dsa.h > @@ -591,6 +591,10 @@ static inline bool dsa_is_user_port(struct dsa_switch *ds, int p) > dsa_switch_for_each_port((_dp), (_ds)) \ > if (dsa_port_is_cpu((_dp))) > > +#define dsa_switch_for_each_dsa_port(_dp, _ds) \ > + dsa_switch_for_each_port((_dp), (_ds)) \ > + if (dsa_port_is_dsa((_dp))) > + > #define dsa_switch_for_each_cpu_port_continue_reverse(_dp, _ds) \ > dsa_switch_for_each_port_continue_reverse((_dp), (_ds)) \ > if (dsa_port_is_cpu((_dp))) > @@ -617,6 +621,17 @@ static inline u32 dsa_cpu_ports(struct dsa_switch *ds) > return mask; > } > > +static inline u32 dsa_dsa_ports(struct dsa_switch *ds) > +{ > + struct dsa_port *dsa_dp;
can name this just "dp"
> + u32 mask = 0; > + > + dsa_switch_for_each_dsa_port(dsa_dp, ds) > + mask |= BIT(dsa_dp->index); > + > + return mask; > +} > + > /* Return the local port used to reach an arbitrary switch device */ > static inline unsigned int dsa_routing_port(struct dsa_switch *ds, int device) > { > -- > 2.34.1 >
| |