Messages in this thread | | | Date | Mon, 15 Mar 2021 23:49:44 +0200 | From | Vladimir Oltean <> | Subject | Re: [PATCH net-next] net: dsa: rtl8366rb: support bridge offloading |
| |
On Tue, Mar 16, 2021 at 01:01:44AM +0800, LGA1150 wrote: > From: DENG Qingfang <dqfext@gmail.com> > > Use port isolation registers to configure bridge offloading. > Remove the VLAN init, as we have proper CPU tag and bridge offloading > support now. > > Signed-off-by: DENG Qingfang <dqfext@gmail.com> > --- > Changes since RFC: > Fix build error > > drivers/net/dsa/rtl8366rb.c | 71 +++++++++++++++++++++++++++++++++---- > 1 file changed, 65 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c > index a89093bc6c6a..bbcfdd84f0e9 100644 > --- a/drivers/net/dsa/rtl8366rb.c > +++ b/drivers/net/dsa/rtl8366rb.c > @@ -300,6 +300,12 @@ > #define RTL8366RB_INTERRUPT_STATUS_REG 0x0442 > #define RTL8366RB_NUM_INTERRUPT 14 /* 0..13 */ > > +/* Port isolation registers */ > +#define RTL8366RB_PORT_ISO_BASE 0x0F08 > +#define RTL8366RB_PORT_ISO(pnum) (RTL8366RB_PORT_ISO_BASE + (pnum)) > +#define RTL8366RB_PORT_ISO_EN BIT(0) > +#define RTL8366RB_PORT_ISO_PORTS_MASK GENMASK(7, 1) > + > /* bits 0..5 enable force when cleared */ > #define RTL8366RB_MAC_FORCE_CTRL_REG 0x0F11 > > @@ -835,6 +841,15 @@ static int rtl8366rb_setup(struct dsa_switch *ds) > if (ret) > return ret; > > + /* Isolate user ports */ > + for (i = 0; i < RTL8366RB_PORT_NUM_CPU; i++) { > + ret = regmap_write(smi->map, RTL8366RB_PORT_ISO(i), > + RTL8366RB_PORT_ISO_EN | > + BIT(RTL8366RB_PORT_NUM_CPU + 1));
You have a RTL8366RB_PORT_ISO_PORTS_MASK that you don't use, and you open-code a +1 here and everywhere below. Wouldn't it be more useful to have a macro for RTL8366RB_PORT_ISO_PORTS(mask)?
> + if (ret) > + return ret; > + } > + > /* Set up the "green ethernet" feature */ > ret = rtl8366rb_jam_table(rtl8366rb_green_jam, > ARRAY_SIZE(rtl8366rb_green_jam), smi, false); > @@ -963,10 +978,6 @@ static int rtl8366rb_setup(struct dsa_switch *ds) > return ret; > } > > - ret = rtl8366_init_vlan(smi); > - if (ret) > - return ret; > -
You can actually delete rtl8366_init_vlan now, it is unused.
> ret = rtl8366rb_setup_cascaded_irq(smi); > if (ret) > dev_info(smi->dev, "no interrupt support\n"); > @@ -977,8 +988,6 @@ static int rtl8366rb_setup(struct dsa_switch *ds) > return -ENODEV; > } > > - ds->configure_vlan_while_not_filtering = false; > - > return 0; > } > > @@ -1127,6 +1136,54 @@ rtl8366rb_port_disable(struct dsa_switch *ds, int port) > rb8366rb_set_port_led(smi, port, false); > } > > +static int > +rtl8366rb_port_bridge_join(struct dsa_switch *ds, int port, > + struct net_device *bridge) > +{ > + struct realtek_smi *smi = ds->priv; > + unsigned int port_bitmap = 0; > + int ret, i; > + > + for (i = 0; i < RTL8366RB_PORT_NUM_CPU; i++) {
Could you use ds->num_ports please?
> + if (i == port) > + continue; > + if (dsa_to_port(ds, i)->bridge_dev != bridge) > + continue; > + ret = regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(i), > + 0, BIT(port + 1));
You call regmap_update_bits with a mask of 0? What does it mean? This is tested, right?
> + if (ret) > + return ret; > + > + port_bitmap |= BIT(i); > + } > + > + return regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(port), > + 0, port_bitmap << 1); > +} > + > +static void > +rtl8366rb_port_bridge_leave(struct dsa_switch *ds, int port, > + struct net_device *bridge) > +{ > + struct realtek_smi *smi = ds->priv; > + unsigned int port_bitmap = 0; > + int i; > + > + for (i = 0; i < RTL8366RB_PORT_NUM_CPU; i++) { > + if (i == port) > + continue; > + if (dsa_to_port(ds, i)->bridge_dev != bridge) > + continue; > + regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(i), > + BIT(port + 1), 0); > + > + port_bitmap |= BIT(i); > + } > + > + regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(port), > + port_bitmap << 1, 0); > +} > +
| |