| Message ID | 20220218060959.6631-3-luizluca@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: realtek: add rtl8_4t tag | expand |
Luiz Angelo Daros de Luca <luizluca@gmail.com> writes: > The trailing tag is also supported by this family. The default is still > rtl8_4 but now the switch supports changing the tag to rtl8_4t. > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > --- > drivers/net/dsa/realtek/rtl8365mb.c | 78 ++++++++++++++++++++++++----- > 1 file changed, 66 insertions(+), 12 deletions(-) > > diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c > index 2ed592147c20..043cac34e906 100644 > --- a/drivers/net/dsa/realtek/rtl8365mb.c > +++ b/drivers/net/dsa/realtek/rtl8365mb.c > @@ -524,9 +524,7 @@ enum rtl8365mb_cpu_rxlen { > * @mask: port mask of ports that parse should parse CPU tags > * @trap_port: forward trapped frames to this port > * @insert: CPU tag insertion mode in switch->CPU frames > - * @position: position of CPU tag in frame > * @rx_length: minimum CPU RX length > - * @format: CPU tag format > * > * Represents the CPU tagging and CPU port configuration of the switch. These > * settings are configurable at runtime. > @@ -536,9 +534,7 @@ struct rtl8365mb_cpu { > u32 mask; > u32 trap_port; > enum rtl8365mb_cpu_insert insert; > - enum rtl8365mb_cpu_position position; > enum rtl8365mb_cpu_rxlen rx_length; > - enum rtl8365mb_cpu_format format; This struct is meant to represent the whole CPU config register. Rather than pulling it out and adding tag_protocol to struct rtl8365mb, can you instead do something like: - keep these members of _cpu - put back the cpu member of struct rtl8365mb (I don't know why it was removed...) - in get_tag_protocol: return mb->cpu.position == AFTER_SA ? RTL8_4 : RTL8_4T; - in change_tag_protocol: just update mb->cpu.position and call rtl8365mb_cpu_config again - avoid the arcane call to rtl8365mb_change_tag_protocol in _setup - avoid the need to do regmap_update_bits instead of a clean regmap_write in one place The reason I'm saying this is because, in the original version of the driver, CPU configuration was in a single place. Now it is scattered. I would kindly ask that you try to respect the existing design because I can already see that things are starting to get a bit messy. If we subsequently want to configure other CPU parameters on the fly, it will be as easy as updating the cpu struct and calling cpu_config again. This register is also non-volatile so the state we keep will always conform with the switch configuration. Sorry if you find the feedback too opinionated - I don't mean anything personally. But the original design was not by accident, so I would appreciate if we can keep it that way unless there is a good reason to change it. > }; > > /** > @@ -566,6 +562,7 @@ 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 > + * @tag_protocol: current switch CPU tag protocol > * @mib_lock: prevent concurrent reads of MIB counters > * @ports: per-port data > * @jam_table: chip-specific initialization jam table > @@ -580,6 +577,7 @@ struct rtl8365mb { > u32 chip_ver; > u32 port_mask; > u32 learn_limit_max; > + enum dsa_tag_protocol tag_protocol; > struct mutex mib_lock; > struct rtl8365mb_port ports[RTL8365MB_MAX_NUM_PORTS]; > const struct rtl8365mb_jam_tbl_entry *jam_table; > @@ -770,7 +768,54 @@ static enum dsa_tag_protocol > rtl8365mb_get_tag_protocol(struct dsa_switch *ds, int port, > enum dsa_tag_protocol mp) > { > - return DSA_TAG_PROTO_RTL8_4; > + struct realtek_priv *priv = ds->priv; > + struct rtl8365mb *chip_data; Please stick to the convention and call this struct rtl8365mb pointer mb. > + > + chip_data = priv->chip_data; > + > + return chip_data->tag_protocol; > +} > + > +static int rtl8365mb_change_tag_protocol(struct dsa_switch *ds, int cpu, > + enum dsa_tag_protocol proto) > +{ > + struct realtek_priv *priv = ds->priv; > + struct rtl8365mb *chip_data; s/chip_data/mb/ per convention > + int tag_position; > + int tag_format; > + int ret; > + > + switch (proto) { > + case DSA_TAG_PROTO_RTL8_4: > + tag_format = RTL8365MB_CPU_FORMAT_8BYTES; > + tag_position = RTL8365MB_CPU_POS_AFTER_SA; > + break; > + case DSA_TAG_PROTO_RTL8_4T: > + tag_format = RTL8365MB_CPU_FORMAT_8BYTES; > + tag_position = RTL8365MB_CPU_POS_BEFORE_CRC; > + break; > + /* The switch also supports a 4-byte format, similar to rtl4a but with > + * the same 0x04 8-bit version and probably 8-bit port source/dest. > + * There is no public doc about it. Not supported yet. > + */ > + default: > + return -EPROTONOSUPPORT; > + } > + > + ret = regmap_update_bits(priv->map, RTL8365MB_CPU_CTRL_REG, > + RTL8365MB_CPU_CTRL_TAG_POSITION_MASK | > + RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK, > + FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_POSITION_MASK, > + tag_position) | > + FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK, > + tag_format)); > + if (ret) > + return ret; > + > + chip_data = priv->chip_data; nit: I would put this assignment up top like in the rest of the driver, respecting reverse-christmass-tree order. It's nice to stick to the existing style. > + chip_data->tag_protocol = proto; > + > + return 0; > } > > static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port, > @@ -1739,13 +1784,18 @@ static int rtl8365mb_cpu_config(struct realtek_priv *priv, const struct rtl8365m > > val = FIELD_PREP(RTL8365MB_CPU_CTRL_EN_MASK, cpu->enable ? 1 : 0) | > FIELD_PREP(RTL8365MB_CPU_CTRL_INSERTMODE_MASK, cpu->insert) | > - FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_POSITION_MASK, cpu->position) | > FIELD_PREP(RTL8365MB_CPU_CTRL_RXBYTECOUNT_MASK, cpu->rx_length) | > - FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK, cpu->format) | > FIELD_PREP(RTL8365MB_CPU_CTRL_TRAP_PORT_MASK, cpu->trap_port & 0x7) | > FIELD_PREP(RTL8365MB_CPU_CTRL_TRAP_PORT_EXT_MASK, > cpu->trap_port >> 3 & 0x1); > - ret = regmap_write(priv->map, RTL8365MB_CPU_CTRL_REG, val); > + > + ret = regmap_update_bits(priv->map, RTL8365MB_CPU_CTRL_REG, > + RTL8365MB_CPU_CTRL_EN_MASK | > + RTL8365MB_CPU_CTRL_INSERTMODE_MASK | > + RTL8365MB_CPU_CTRL_RXBYTECOUNT_MASK | > + RTL8365MB_CPU_CTRL_TRAP_PORT_MASK | > + RTL8365MB_CPU_CTRL_TRAP_PORT_EXT_MASK, > + val); > if (ret) > return ret; > > @@ -1827,6 +1877,11 @@ static int rtl8365mb_setup(struct dsa_switch *ds) > dev_info(priv->dev, "no interrupt support\n"); > > /* Configure CPU tagging */ > + ret = rtl8365mb_change_tag_protocol(priv->ds, -1, DSA_TAG_PROTO_RTL8_4); > + if (ret) { > + dev_err(priv->dev, "failed to set default tag protocol: %d\n", ret); > + return ret; > + } > cpu.trap_port = RTL8365MB_MAX_NUM_PORTS; > dsa_switch_for_each_cpu_port(cpu_dp, priv->ds) { > cpu.mask |= BIT(cpu_dp->index); > @@ -1834,13 +1889,9 @@ static int rtl8365mb_setup(struct dsa_switch *ds) > if (cpu.trap_port == RTL8365MB_MAX_NUM_PORTS) > cpu.trap_port = cpu_dp->index; > } > - > cpu.enable = cpu.mask > 0; > 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; Like I said above, I think it would be nice to put this cpu struct back in the rtl8365mb private data. > - > ret = rtl8365mb_cpu_config(priv, &cpu); > if (ret) > goto out_teardown_irq; > @@ -1982,6 +2033,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv) > mb->learn_limit_max = RTL8365MB_LEARN_LIMIT_MAX; > mb->jam_table = rtl8365mb_init_jam_8365mb_vc; > mb->jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc); > + mb->tag_protocol = DSA_TAG_PROTO_RTL8_4; > > break; > default: > @@ -1996,6 +2048,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv) > > static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = { > .get_tag_protocol = rtl8365mb_get_tag_protocol, > + .change_tag_protocol = rtl8365mb_change_tag_protocol, > .setup = rtl8365mb_setup, > .teardown = rtl8365mb_teardown, > .phylink_get_caps = rtl8365mb_phylink_get_caps, > @@ -2014,6 +2067,7 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = { > > static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = { > .get_tag_protocol = rtl8365mb_get_tag_protocol, > + .change_tag_protocol = rtl8365mb_change_tag_protocol, > .setup = rtl8365mb_setup, > .teardown = rtl8365mb_teardown, > .phylink_get_caps = rtl8365mb_phylink_get_caps,
> > The trailing tag is also supported by this family. The default is still > > rtl8_4 but now the switch supports changing the tag to rtl8_4t. > > > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > > --- > > drivers/net/dsa/realtek/rtl8365mb.c | 78 ++++++++++++++++++++++++----- > > 1 file changed, 66 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c > > index 2ed592147c20..043cac34e906 100644 > > --- a/drivers/net/dsa/realtek/rtl8365mb.c > > +++ b/drivers/net/dsa/realtek/rtl8365mb.c > > @@ -524,9 +524,7 @@ enum rtl8365mb_cpu_rxlen { > > * @mask: port mask of ports that parse should parse CPU tags > > * @trap_port: forward trapped frames to this port > > * @insert: CPU tag insertion mode in switch->CPU frames > > - * @position: position of CPU tag in frame > > * @rx_length: minimum CPU RX length > > - * @format: CPU tag format > > * > > * Represents the CPU tagging and CPU port configuration of the switch. These > > * settings are configurable at runtime. > > @@ -536,9 +534,7 @@ struct rtl8365mb_cpu { > > u32 mask; > > u32 trap_port; > > enum rtl8365mb_cpu_insert insert; > > - enum rtl8365mb_cpu_position position; > > enum rtl8365mb_cpu_rxlen rx_length; > > - enum rtl8365mb_cpu_format format; > > This struct is meant to represent the whole CPU config register. Rather > than pulling it out and adding tag_protocol to struct rtl8365mb, can you > instead do something like: > > - keep these members of _cpu > - put back the cpu member of struct rtl8365mb (I don't know why it was removed...) The cpu was dropped from the struct rtl8365mb because it had no use for it. It was only used outside setup to unreliably detect ext int ports. When I got no other use for it, I removed it (stingily saving some bytes). > - in get_tag_protocol: return mb->cpu.position == AFTER_SA ? RTL8_4 : RTL8_4T; I was doing just that but I changed to an enum dsa_tag_protocol. mb->cpu.position works together with mb->cpu.format and if it is RTL8365MB_CPU_FORMAT_4BYTES, the code will have an undefined behavior (and get_tag_protocol() cannot return an error). My idea was to always do "DSA tag" to "Realtek registers" and never the opposite to avoid that situation. get_tag_protocol() is called even before the CPU port is configured. And although AFTER_SA and cpu format bits unset is the desired default value, I would like to make it safe by design, not coincidence. > - in change_tag_protocol: just update mb->cpu.position and call > rtl8365mb_cpu_config again > - avoid the arcane call to rtl8365mb_change_tag_protocol in _setup > - avoid the need to do regmap_update_bits instead of a clean > regmap_write in one place The rtl8365mb_cpu_config() was already a multi-register update, doing a regmap_update_bits(RTL8365MB_CPU_PORT_MASK_REG) and a regmap_write(RTL8365MB_CPU_CTRL_REG). I thought it would touch too much just to change a single bit. After the indirect reg access, I'm trying to touch exclusively what is strictly necessary. > The reason I'm saying this is because, in the original version of the > driver, CPU configuration was in a single place. Now it is scattered. I > would kindly ask that you try to respect the existing design because I > can already see that things are starting to get a bit messy. My idea was to bring closer what was asked with what strictly needs to be done. We agree on having a single place where a setting is applied. We disagree on the granularity: I think it should be the smallest unit a caller might be interested to change (a bit in this case), and you that it should be the cpu-related registers. I don't know which one is the best option. I think it is easier to track changes when there is an individual function that touches it (like adding a printk), instead of conditionally printing that message from a shared function. Anyway, I might be exaggerating for this case. > If we subsequently want to configure other CPU parameters on the fly, it > will be as easy as updating the cpu struct and calling cpu_config > again. This register is also non-volatile so the state we keep will > always conform with the switch configuration. I'm averse to any copies of data when I could have them at a single place. Using the CPU struct, it is a two step job: 1) change the driver cpu struct, 2) apply. In a similar generic situation, I need to be cautious if someone could potentially change the struct between step 1) and 2), or even something else before step 1) could have it changed in memory (row hammer, for example). It might not apply to this driver but I always try to be skeptical "by design". > Sorry if you find the feedback too opinionated - I don't mean anything > personally. But the original design was not by accident, so I would > appreciate if we can keep it that way unless there is a good reason to > change it. Thanks, Alvin. No need to feel sorry. The worst you can do is to offend my code, my ideas, not me. ;-) It's always good to hear from you and other devs. I always learn something. > > > }; > > > > /** > > @@ -566,6 +562,7 @@ 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 > > + * @tag_protocol: current switch CPU tag protocol > > * @mib_lock: prevent concurrent reads of MIB counters > > * @ports: per-port data > > * @jam_table: chip-specific initialization jam table > > @@ -580,6 +577,7 @@ struct rtl8365mb { > > u32 chip_ver; > > u32 port_mask; > > u32 learn_limit_max; > > + enum dsa_tag_protocol tag_protocol; > > struct mutex mib_lock; > > struct rtl8365mb_port ports[RTL8365MB_MAX_NUM_PORTS]; > > const struct rtl8365mb_jam_tbl_entry *jam_table; > > @@ -770,7 +768,54 @@ static enum dsa_tag_protocol > > rtl8365mb_get_tag_protocol(struct dsa_switch *ds, int port, > > enum dsa_tag_protocol mp) > > { > > - return DSA_TAG_PROTO_RTL8_4; > > + struct realtek_priv *priv = ds->priv; > > + struct rtl8365mb *chip_data; > > Please stick to the convention and call this struct rtl8365mb pointer mb. That's a great opportunity to ask. I always wondered what mb really means. I was already asked in an old thread but nobody answered it. The only "mb" I found is the driver suffix (rtl8365'mb') but it would not make sense. > > > + > > + chip_data = priv->chip_data; > > + > > + return chip_data->tag_protocol; > > +} > > + > > +static int rtl8365mb_change_tag_protocol(struct dsa_switch *ds, int cpu, > > + enum dsa_tag_protocol proto) > > +{ > > + struct realtek_priv *priv = ds->priv; > > + struct rtl8365mb *chip_data; > > s/chip_data/mb/ per convention > > > + int tag_position; > > + int tag_format; > > + int ret; > > + > > + switch (proto) { > > + case DSA_TAG_PROTO_RTL8_4: > > + tag_format = RTL8365MB_CPU_FORMAT_8BYTES; > > + tag_position = RTL8365MB_CPU_POS_AFTER_SA; > > + break; > > + case DSA_TAG_PROTO_RTL8_4T: > > + tag_format = RTL8365MB_CPU_FORMAT_8BYTES; > > + tag_position = RTL8365MB_CPU_POS_BEFORE_CRC; > > + break; > > + /* The switch also supports a 4-byte format, similar to rtl4a but with > > + * the same 0x04 8-bit version and probably 8-bit port source/dest. > > + * There is no public doc about it. Not supported yet. > > + */ > > + default: > > + return -EPROTONOSUPPORT; > > + } > > + > > + ret = regmap_update_bits(priv->map, RTL8365MB_CPU_CTRL_REG, > > + RTL8365MB_CPU_CTRL_TAG_POSITION_MASK | > > + RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK, > > + FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_POSITION_MASK, > > + tag_position) | > > + FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK, > > + tag_format)); > > + if (ret) > > + return ret; > > + > > + chip_data = priv->chip_data; > > nit: I would put this assignment up top like in the rest of the driver, > respecting reverse-christmass-tree order. It's nice to stick to the > existing style. ok > > > + chip_data->tag_protocol = proto; > > + > > + return 0; > > } > > > > static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port, > > @@ -1739,13 +1784,18 @@ static int rtl8365mb_cpu_config(struct realtek_priv *priv, const struct rtl8365m > > > > val = FIELD_PREP(RTL8365MB_CPU_CTRL_EN_MASK, cpu->enable ? 1 : 0) | > > FIELD_PREP(RTL8365MB_CPU_CTRL_INSERTMODE_MASK, cpu->insert) | > > - FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_POSITION_MASK, cpu->position) | > > FIELD_PREP(RTL8365MB_CPU_CTRL_RXBYTECOUNT_MASK, cpu->rx_length) | > > - FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK, cpu->format) | > > FIELD_PREP(RTL8365MB_CPU_CTRL_TRAP_PORT_MASK, cpu->trap_port & 0x7) | > > FIELD_PREP(RTL8365MB_CPU_CTRL_TRAP_PORT_EXT_MASK, > > cpu->trap_port >> 3 & 0x1); > > - ret = regmap_write(priv->map, RTL8365MB_CPU_CTRL_REG, val); > > + > > + ret = regmap_update_bits(priv->map, RTL8365MB_CPU_CTRL_REG, > > + RTL8365MB_CPU_CTRL_EN_MASK | > > + RTL8365MB_CPU_CTRL_INSERTMODE_MASK | > > + RTL8365MB_CPU_CTRL_RXBYTECOUNT_MASK | > > + RTL8365MB_CPU_CTRL_TRAP_PORT_MASK | > > + RTL8365MB_CPU_CTRL_TRAP_PORT_EXT_MASK, > > + val); > > if (ret) > > return ret; > > > > @@ -1827,6 +1877,11 @@ static int rtl8365mb_setup(struct dsa_switch *ds) > > dev_info(priv->dev, "no interrupt support\n"); > > > > /* Configure CPU tagging */ > > + ret = rtl8365mb_change_tag_protocol(priv->ds, -1, DSA_TAG_PROTO_RTL8_4); > > + if (ret) { > > + dev_err(priv->dev, "failed to set default tag protocol: %d\n", ret); > > + return ret; > > + } > > cpu.trap_port = RTL8365MB_MAX_NUM_PORTS; > > dsa_switch_for_each_cpu_port(cpu_dp, priv->ds) { > > cpu.mask |= BIT(cpu_dp->index); > > @@ -1834,13 +1889,9 @@ static int rtl8365mb_setup(struct dsa_switch *ds) > > if (cpu.trap_port == RTL8365MB_MAX_NUM_PORTS) > > cpu.trap_port = cpu_dp->index; > > } > > - > > cpu.enable = cpu.mask > 0; > > 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; > > Like I said above, I think it would be nice to put this cpu struct back > in the rtl8365mb private data. It would require to split CPU initialization between pre dsa register (where format must be defined) and dsa_setup (where cpu port is read from dsa ports and settings applied to the switch). get_tag_protocol() is called between these two to get the default tag protocol. DSA calls change_tag_protocol afterwards if the defined tag protocol in the devicetree does not match. > > - > > ret = rtl8365mb_cpu_config(priv, &cpu); > > if (ret) > > goto out_teardown_irq; > > @@ -1982,6 +2033,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv) > > mb->learn_limit_max = RTL8365MB_LEARN_LIMIT_MAX; > > mb->jam_table = rtl8365mb_init_jam_8365mb_vc; > > mb->jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc); > > + mb->tag_protocol = DSA_TAG_PROTO_RTL8_4; > > > > break; > > default: > > @@ -1996,6 +2048,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv) > > > > static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = { > > .get_tag_protocol = rtl8365mb_get_tag_protocol, > > + .change_tag_protocol = rtl8365mb_change_tag_protocol, > > .setup = rtl8365mb_setup, > > .teardown = rtl8365mb_teardown, > > .phylink_get_caps = rtl8365mb_phylink_get_caps, > > @@ -2014,6 +2067,7 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = { > > > > static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = { > > .get_tag_protocol = rtl8365mb_get_tag_protocol, > > + .change_tag_protocol = rtl8365mb_change_tag_protocol, > > .setup = rtl8365mb_setup, > > .teardown = rtl8365mb_teardown, > > .phylink_get_caps = rtl8365mb_phylink_get_caps,
Luiz Angelo Daros de Luca <luizluca@gmail.com> writes: >> > The trailing tag is also supported by this family. The default is still >> > rtl8_4 but now the switch supports changing the tag to rtl8_4t. >> > >> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> >> > --- >> > drivers/net/dsa/realtek/rtl8365mb.c | 78 ++++++++++++++++++++++++----- >> > 1 file changed, 66 insertions(+), 12 deletions(-) >> > >> > diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c >> > index 2ed592147c20..043cac34e906 100644 >> > --- a/drivers/net/dsa/realtek/rtl8365mb.c >> > +++ b/drivers/net/dsa/realtek/rtl8365mb.c >> > @@ -524,9 +524,7 @@ enum rtl8365mb_cpu_rxlen { >> > * @mask: port mask of ports that parse should parse CPU tags >> > * @trap_port: forward trapped frames to this port >> > * @insert: CPU tag insertion mode in switch->CPU frames >> > - * @position: position of CPU tag in frame >> > * @rx_length: minimum CPU RX length >> > - * @format: CPU tag format >> > * >> > * Represents the CPU tagging and CPU port configuration of the switch. These >> > * settings are configurable at runtime. >> > @@ -536,9 +534,7 @@ struct rtl8365mb_cpu { >> > u32 mask; >> > u32 trap_port; >> > enum rtl8365mb_cpu_insert insert; >> > - enum rtl8365mb_cpu_position position; >> > enum rtl8365mb_cpu_rxlen rx_length; >> > - enum rtl8365mb_cpu_format format; >> >> This struct is meant to represent the whole CPU config register. Rather >> than pulling it out and adding tag_protocol to struct rtl8365mb, can you >> instead do something like: >> >> - keep these members of _cpu >> - put back the cpu member of struct rtl8365mb (I don't know why it was removed...) > > The cpu was dropped from the struct rtl8365mb because it had no use > for it. It was only used outside setup to unreliably detect ext int > ports. When I got no other use for it, I removed it (stingily saving > some bytes). This is not a good approach in general as evidenced by this discussion. > >> - in get_tag_protocol: return mb->cpu.position == AFTER_SA ? RTL8_4 : RTL8_4T; > > I was doing just that but I changed to an enum dsa_tag_protocol. > mb->cpu.position works together with mb->cpu.format and if it is > RTL8365MB_CPU_FORMAT_4BYTES, the code will have an undefined behavior > (and get_tag_protocol() cannot return an error). My idea was to always > do "DSA tag" to "Realtek registers" and never the opposite to avoid > that situation. get_tag_protocol() is called even before the CPU port > is configured. And although AFTER_SA and cpu format bits unset is the > desired default value, I would like to make it safe by design, not > coincidence. Just check mb->cpu.format as well when adding support for 4-byte tags then? BTW, I don't suggest adding the 4-byte tag support unless there is a very good reason. It contains much less information and will complicate matters when we want to add e.g. TX forward offloading. That will not be possible with the 4-byte format, and so we will have to put traps all over the driver to ensure coherency. Ultimately it is better just not to add support unless somebody has a hardware requirement which demands it. > >> - in change_tag_protocol: just update mb->cpu.position and call >> rtl8365mb_cpu_config again >> - avoid the arcane call to rtl8365mb_change_tag_protocol in _setup >> - avoid the need to do regmap_update_bits instead of a clean >> regmap_write in one place > > The rtl8365mb_cpu_config() was already a multi-register update, doing > a regmap_update_bits(RTL8365MB_CPU_PORT_MASK_REG) and a > regmap_write(RTL8365MB_CPU_CTRL_REG). I thought it would touch too > much just to change a single bit. After the indirect reg access, I'm > trying to touch exclusively what is strictly necessary. Luiz, it is more important for the code to be coherent. Register writes are very cheap here. Changing tag protocol is not something that happens all the time. Same applies to dropping bytes due to stinginess - never sacrifice the code for some purely theoretical performance optimization. Also, don't worry about the indirect PHY register access affair. We fixed it, right? Do not be afraid of the hardware, it is not that scary. Be afraid of long review cycles because people don't like your code ;-) > >> The reason I'm saying this is because, in the original version of the >> driver, CPU configuration was in a single place. Now it is scattered. I >> would kindly ask that you try to respect the existing design because I >> can already see that things are starting to get a bit messy. > > My idea was to bring closer what was asked with what strictly needs to > be done. We agree on having a single place where a setting is applied. > We disagree on the granularity: I think it should be the smallest unit > a caller might be interested to change (a bit in this case), and you > that it should be the cpu-related registers. I don't know which one is > the best option. Have you looked at the implementation of regmap_update_bits? It actually does two operations: a read and a write. I fear we have a very divergent philosophy on this matter. By far and away the most important thing is to keep the code clean and consistent. I already explained that these registers are non-volatile. > > I think it is easier to track changes when there is an individual > function that touches it (like adding a printk), instead of > conditionally printing that message from a shared function. Anyway, I > might be exaggerating for this case. This argument is not helpful at all. > >> If we subsequently want to configure other CPU parameters on the fly, it >> will be as easy as updating the cpu struct and calling cpu_config >> again. This register is also non-volatile so the state we keep will >> always conform with the switch configuration. > > I'm averse to any copies of data when I could have them at a single > place. Using the CPU struct, it is a two step job: 1) change the > driver cpu struct, 2) apply. In a similar generic situation, I need to > be cautious if someone could potentially change the struct between > step 1) and 2), or even something else before step 1) could have it > changed in memory (row hammer, for example). It might not apply to > this driver but I always try to be skeptical "by design". This argument is also pie-in-the-sky. > >> Sorry if you find the feedback too opinionated - I don't mean anything >> personally. But the original design was not by accident, so I would >> appreciate if we can keep it that way unless there is a good reason to >> change it. > > Thanks, Alvin. No need to feel sorry. The worst you can do is to > offend my code, my ideas, not me. ;-) It's always good to hear from > you and other devs. I always learn something. > >> >> > }; >> > >> > /** >> > @@ -566,6 +562,7 @@ 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 >> > + * @tag_protocol: current switch CPU tag protocol >> > * @mib_lock: prevent concurrent reads of MIB counters >> > * @ports: per-port data >> > * @jam_table: chip-specific initialization jam table >> > @@ -580,6 +577,7 @@ struct rtl8365mb { >> > u32 chip_ver; >> > u32 port_mask; >> > u32 learn_limit_max; >> > + enum dsa_tag_protocol tag_protocol; >> > struct mutex mib_lock; >> > struct rtl8365mb_port ports[RTL8365MB_MAX_NUM_PORTS]; >> > const struct rtl8365mb_jam_tbl_entry *jam_table; >> > @@ -770,7 +768,54 @@ static enum dsa_tag_protocol >> > rtl8365mb_get_tag_protocol(struct dsa_switch *ds, int port, >> > enum dsa_tag_protocol mp) >> > { >> > - return DSA_TAG_PROTO_RTL8_4; >> > + struct realtek_priv *priv = ds->priv; >> > + struct rtl8365mb *chip_data; >> >> Please stick to the convention and call this struct rtl8365mb pointer mb. > > That's a great opportunity to ask. I always wondered what mb really > means. I was already asked in an old thread but nobody answered it. > The only "mb" I found is the driver suffix (rtl8365'mb') but it would > not make sense. Yeah it was just the suffix. You can think of it as the nickname of the chip in the driver. Many other drivers do this too. Sorry that it doesn't make sense. > >> >> > + >> > + chip_data = priv->chip_data; >> > + >> > + return chip_data->tag_protocol; >> > +} >> > + >> > +static int rtl8365mb_change_tag_protocol(struct dsa_switch *ds, int cpu, >> > + enum dsa_tag_protocol proto) >> > +{ >> > + struct realtek_priv *priv = ds->priv; >> > + struct rtl8365mb *chip_data; >> >> s/chip_data/mb/ per convention >> >> > + int tag_position; >> > + int tag_format; >> > + int ret; >> > + >> > + switch (proto) { >> > + case DSA_TAG_PROTO_RTL8_4: >> > + tag_format = RTL8365MB_CPU_FORMAT_8BYTES; >> > + tag_position = RTL8365MB_CPU_POS_AFTER_SA; >> > + break; >> > + case DSA_TAG_PROTO_RTL8_4T: >> > + tag_format = RTL8365MB_CPU_FORMAT_8BYTES; >> > + tag_position = RTL8365MB_CPU_POS_BEFORE_CRC; >> > + break; >> > + /* The switch also supports a 4-byte format, similar to rtl4a but with >> > + * the same 0x04 8-bit version and probably 8-bit port source/dest. >> > + * There is no public doc about it. Not supported yet. >> > + */ >> > + default: >> > + return -EPROTONOSUPPORT; >> > + } >> > + >> > + ret = regmap_update_bits(priv->map, RTL8365MB_CPU_CTRL_REG, >> > + RTL8365MB_CPU_CTRL_TAG_POSITION_MASK | >> > + RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK, >> > + FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_POSITION_MASK, >> > + tag_position) | >> > + FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK, >> > + tag_format)); >> > + if (ret) >> > + return ret; >> > + >> > + chip_data = priv->chip_data; >> >> nit: I would put this assignment up top like in the rest of the driver, >> respecting reverse-christmass-tree order. It's nice to stick to the >> existing style. > > ok > >> >> > + chip_data->tag_protocol = proto; >> > + >> > + return 0; >> > } >> > >> > static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port, >> > @@ -1739,13 +1784,18 @@ static int rtl8365mb_cpu_config(struct realtek_priv *priv, const struct rtl8365m >> > >> > val = FIELD_PREP(RTL8365MB_CPU_CTRL_EN_MASK, cpu->enable ? 1 : 0) | >> > FIELD_PREP(RTL8365MB_CPU_CTRL_INSERTMODE_MASK, cpu->insert) | >> > - FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_POSITION_MASK, cpu->position) | >> > FIELD_PREP(RTL8365MB_CPU_CTRL_RXBYTECOUNT_MASK, cpu->rx_length) | >> > - FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK, cpu->format) | >> > FIELD_PREP(RTL8365MB_CPU_CTRL_TRAP_PORT_MASK, cpu->trap_port & 0x7) | >> > FIELD_PREP(RTL8365MB_CPU_CTRL_TRAP_PORT_EXT_MASK, >> > cpu->trap_port >> 3 & 0x1); >> > - ret = regmap_write(priv->map, RTL8365MB_CPU_CTRL_REG, val); >> > + >> > + ret = regmap_update_bits(priv->map, RTL8365MB_CPU_CTRL_REG, >> > + RTL8365MB_CPU_CTRL_EN_MASK | >> > + RTL8365MB_CPU_CTRL_INSERTMODE_MASK | >> > + RTL8365MB_CPU_CTRL_RXBYTECOUNT_MASK | >> > + RTL8365MB_CPU_CTRL_TRAP_PORT_MASK | >> > + RTL8365MB_CPU_CTRL_TRAP_PORT_EXT_MASK, >> > + val); >> > if (ret) >> > return ret; >> > >> > @@ -1827,6 +1877,11 @@ static int rtl8365mb_setup(struct dsa_switch *ds) >> > dev_info(priv->dev, "no interrupt support\n"); >> > >> > /* Configure CPU tagging */ >> > + ret = rtl8365mb_change_tag_protocol(priv->ds, -1, DSA_TAG_PROTO_RTL8_4); >> > + if (ret) { >> > + dev_err(priv->dev, "failed to set default tag protocol: %d\n", ret); >> > + return ret; >> > + } >> > cpu.trap_port = RTL8365MB_MAX_NUM_PORTS; >> > dsa_switch_for_each_cpu_port(cpu_dp, priv->ds) { >> > cpu.mask |= BIT(cpu_dp->index); >> > @@ -1834,13 +1889,9 @@ static int rtl8365mb_setup(struct dsa_switch *ds) >> > if (cpu.trap_port == RTL8365MB_MAX_NUM_PORTS) >> > cpu.trap_port = cpu_dp->index; >> > } >> > - >> > cpu.enable = cpu.mask > 0; >> > 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; >> >> Like I said above, I think it would be nice to put this cpu struct back >> in the rtl8365mb private data. > > It would require to split CPU initialization between pre dsa register > (where format must be defined) and dsa_setup (where cpu port is read > from dsa ports and settings applied to the switch). get_tag_protocol() > is called between these two to get the default tag protocol. DSA calls > change_tag_protocol afterwards if the defined tag protocol in the > devicetree does not match. I don't see the problem, sorry. I am basically suggesting to go back to the way it was done before. You can always change the tag protocol as you can the rest of the CPU configuration. I even put a comment at the top of struct rtl8365mb_cpu to this effect: /** * ... * Represents the CPU tagging and CPU port configuration of the switch. These * settings are configurable at runtime. */ struct rtl8365mb_cpu { > >> > - >> > ret = rtl8365mb_cpu_config(priv, &cpu); >> > if (ret) >> > goto out_teardown_irq; >> > @@ -1982,6 +2033,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv) >> > mb->learn_limit_max = RTL8365MB_LEARN_LIMIT_MAX; >> > mb->jam_table = rtl8365mb_init_jam_8365mb_vc; >> > mb->jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc); >> > + mb->tag_protocol = DSA_TAG_PROTO_RTL8_4; >> > >> > break; >> > default: >> > @@ -1996,6 +2048,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv) >> > >> > static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = { >> > .get_tag_protocol = rtl8365mb_get_tag_protocol, >> > + .change_tag_protocol = rtl8365mb_change_tag_protocol, >> > .setup = rtl8365mb_setup, >> > .teardown = rtl8365mb_teardown, >> > .phylink_get_caps = rtl8365mb_phylink_get_caps, >> > @@ -2014,6 +2067,7 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = { >> > >> > static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = { >> > .get_tag_protocol = rtl8365mb_get_tag_protocol, >> > + .change_tag_protocol = rtl8365mb_change_tag_protocol, >> > .setup = rtl8365mb_setup, >> > .teardown = rtl8365mb_teardown, >> > .phylink_get_caps = rtl8365mb_phylink_get_caps,
> >> > The trailing tag is also supported by this family. The default is still > >> > rtl8_4 but now the switch supports changing the tag to rtl8_4t. > >> > > >> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > >> > --- > >> > drivers/net/dsa/realtek/rtl8365mb.c | 78 ++++++++++++++++++++++++----- > >> > 1 file changed, 66 insertions(+), 12 deletions(-) > >> > > >> > diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c > >> > index 2ed592147c20..043cac34e906 100644 > >> > --- a/drivers/net/dsa/realtek/rtl8365mb.c > >> > +++ b/drivers/net/dsa/realtek/rtl8365mb.c > >> > @@ -524,9 +524,7 @@ enum rtl8365mb_cpu_rxlen { > >> > * @mask: port mask of ports that parse should parse CPU tags > >> > * @trap_port: forward trapped frames to this port > >> > * @insert: CPU tag insertion mode in switch->CPU frames > >> > - * @position: position of CPU tag in frame > >> > * @rx_length: minimum CPU RX length > >> > - * @format: CPU tag format > >> > * > >> > * Represents the CPU tagging and CPU port configuration of the switch. These > >> > * settings are configurable at runtime. > >> > @@ -536,9 +534,7 @@ struct rtl8365mb_cpu { > >> > u32 mask; > >> > u32 trap_port; > >> > enum rtl8365mb_cpu_insert insert; > >> > - enum rtl8365mb_cpu_position position; > >> > enum rtl8365mb_cpu_rxlen rx_length; > >> > - enum rtl8365mb_cpu_format format; > >> > >> This struct is meant to represent the whole CPU config register. Rather > >> than pulling it out and adding tag_protocol to struct rtl8365mb, can you > >> instead do something like: > >> > >> - keep these members of _cpu > >> - put back the cpu member of struct rtl8365mb (I don't know why it was removed...) > > > > The cpu was dropped from the struct rtl8365mb because it had no use > > for it. It was only used outside setup to unreliably detect ext int > > ports. When I got no other use for it, I removed it (stingily saving > > some bytes). > > This is not a good approach in general as evidenced by this > discussion. OK. I'll revert it back and use the old code path. I do not have a strong opinion on either approach. > > > > >> - in get_tag_protocol: return mb->cpu.position == AFTER_SA ? RTL8_4 : RTL8_4T; > > > > I was doing just that but I changed to an enum dsa_tag_protocol. > > mb->cpu.position works together with mb->cpu.format and if it is > > RTL8365MB_CPU_FORMAT_4BYTES, the code will have an undefined behavior > > (and get_tag_protocol() cannot return an error). My idea was to always > > do "DSA tag" to "Realtek registers" and never the opposite to avoid > > that situation. get_tag_protocol() is called even before the CPU port > > is configured. And although AFTER_SA and cpu format bits unset is the > > desired default value, I would like to make it safe by design, not > > coincidence. > > Just check mb->cpu.format as well when adding support for 4-byte tags > then? > > BTW, I don't suggest adding the 4-byte tag support unless there is a > very good reason. It contains much less information and will complicate > matters when we want to add e.g. TX forward offloading. That will not be > possible with the 4-byte format, and so we will have to put traps all > over the driver to ensure coherency. Ultimately it is better just not to > add support unless somebody has a hardware requirement which demands it. > > > > >> - in change_tag_protocol: just update mb->cpu.position and call > >> rtl8365mb_cpu_config again > >> - avoid the arcane call to rtl8365mb_change_tag_protocol in _setup > >> - avoid the need to do regmap_update_bits instead of a clean > >> regmap_write in one place > > > > The rtl8365mb_cpu_config() was already a multi-register update, doing > > a regmap_update_bits(RTL8365MB_CPU_PORT_MASK_REG) and a > > regmap_write(RTL8365MB_CPU_CTRL_REG). I thought it would touch too > > much just to change a single bit. After the indirect reg access, I'm > > trying to touch exclusively what is strictly necessary. > > Luiz, it is more important for the code to be coherent. Register writes > are very cheap here. Changing tag protocol is not something that happens > all the time. Same applies to dropping bytes due to stinginess - never > sacrifice the code for some purely theoretical performance optimization. > > Also, don't worry about the indirect PHY register access affair. We > fixed it, right? Do not be afraid of the hardware, it is not that > scary. Be afraid of long review cycles because people don't like your > code ;-) > > > > >> The reason I'm saying this is because, in the original version of the > >> driver, CPU configuration was in a single place. Now it is scattered. I > >> would kindly ask that you try to respect the existing design because I > >> can already see that things are starting to get a bit messy. > > > > My idea was to bring closer what was asked with what strictly needs to > > be done. We agree on having a single place where a setting is applied. > > We disagree on the granularity: I think it should be the smallest unit > > a caller might be interested to change (a bit in this case), and you > > that it should be the cpu-related registers. I don't know which one is > > the best option. > > Have you looked at the implementation of regmap_update_bits? It actually > does two operations: a read and a write. Indeed I did, but it does it in a locked context. > > I fear we have a very divergent philosophy on this matter. By far and > away the most important thing is to keep the code clean and > consistent. As I said, no strong feelings for any option. I'll do whatever is the best option. > I already explained that these registers are non-volatile. > > > > > I think it is easier to track changes when there is an individual > > function that touches it (like adding a printk), instead of > > conditionally printing that message from a shared function. Anyway, I > > might be exaggerating for this case. > > This argument is not helpful at all. > > > > >> If we subsequently want to configure other CPU parameters on the fly, it > >> will be as easy as updating the cpu struct and calling cpu_config > >> again. This register is also non-volatile so the state we keep will > >> always conform with the switch configuration. > > > > I'm averse to any copies of data when I could have them at a single > > place. Using the CPU struct, it is a two step job: 1) change the > > driver cpu struct, 2) apply. In a similar generic situation, I need to > > be cautious if someone could potentially change the struct between > > step 1) and 2), or even something else before step 1) could have it > > changed in memory (row hammer, for example). It might not apply to > > this driver but I always try to be skeptical "by design". > > This argument is also pie-in-the-sky. > > > > >> Sorry if you find the feedback too opinionated - I don't mean anything > >> personally. But the original design was not by accident, so I would > >> appreciate if we can keep it that way unless there is a good reason to > >> change it. > > > > Thanks, Alvin. No need to feel sorry. The worst you can do is to > > offend my code, my ideas, not me. ;-) It's always good to hear from > > you and other devs. I always learn something. > > > >> > >> > }; > >> > > >> > /** > >> > @@ -566,6 +562,7 @@ 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 > >> > + * @tag_protocol: current switch CPU tag protocol > >> > * @mib_lock: prevent concurrent reads of MIB counters > >> > * @ports: per-port data > >> > * @jam_table: chip-specific initialization jam table > >> > @@ -580,6 +577,7 @@ struct rtl8365mb { > >> > u32 chip_ver; > >> > u32 port_mask; > >> > u32 learn_limit_max; > >> > + enum dsa_tag_protocol tag_protocol; > >> > struct mutex mib_lock; > >> > struct rtl8365mb_port ports[RTL8365MB_MAX_NUM_PORTS]; > >> > const struct rtl8365mb_jam_tbl_entry *jam_table; > >> > @@ -770,7 +768,54 @@ static enum dsa_tag_protocol > >> > rtl8365mb_get_tag_protocol(struct dsa_switch *ds, int port, > >> > enum dsa_tag_protocol mp) > >> > { > >> > - return DSA_TAG_PROTO_RTL8_4; > >> > + struct realtek_priv *priv = ds->priv; > >> > + struct rtl8365mb *chip_data; > >> > >> Please stick to the convention and call this struct rtl8365mb pointer mb. > > > > That's a great opportunity to ask. I always wondered what mb really > > means. I was already asked in an old thread but nobody answered it. > > The only "mb" I found is the driver suffix (rtl8365'mb') but it would > > not make sense. > > Yeah it was just the suffix. You can think of it as the nickname of the > chip in the driver. Many other drivers do this too. Sorry that it > doesn't make sense. MB is some kind of model classification, like cars use S, SV, SL. It's as strange as to name a variable that represents the car family as SV. MB is used by incompatible models, like RTL8304MB-CG. As it is only used internally and it is purely cosmetic, I think it might be nice to rename it, even to a generic name as chip_data, just like "car_data" is better than "sv". For now, I'll stick with mb. > > > > >> > >> > + > >> > + chip_data = priv->chip_data; > >> > + > >> > + return chip_data->tag_protocol; > >> > +} > >> > + > >> > +static int rtl8365mb_change_tag_protocol(struct dsa_switch *ds, int cpu, > >> > + enum dsa_tag_protocol proto) > >> > +{ > >> > + struct realtek_priv *priv = ds->priv; > >> > + struct rtl8365mb *chip_data; > >> > >> s/chip_data/mb/ per convention > >> > >> > + int tag_position; > >> > + int tag_format; > >> > + int ret; > >> > + > >> > + switch (proto) { > >> > + case DSA_TAG_PROTO_RTL8_4: > >> > + tag_format = RTL8365MB_CPU_FORMAT_8BYTES; > >> > + tag_position = RTL8365MB_CPU_POS_AFTER_SA; > >> > + break; > >> > + case DSA_TAG_PROTO_RTL8_4T: > >> > + tag_format = RTL8365MB_CPU_FORMAT_8BYTES; > >> > + tag_position = RTL8365MB_CPU_POS_BEFORE_CRC; > >> > + break; > >> > + /* The switch also supports a 4-byte format, similar to rtl4a but with > >> > + * the same 0x04 8-bit version and probably 8-bit port source/dest. > >> > + * There is no public doc about it. Not supported yet. > >> > + */ > >> > + default: > >> > + return -EPROTONOSUPPORT; > >> > + } > >> > + > >> > + ret = regmap_update_bits(priv->map, RTL8365MB_CPU_CTRL_REG, > >> > + RTL8365MB_CPU_CTRL_TAG_POSITION_MASK | > >> > + RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK, > >> > + FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_POSITION_MASK, > >> > + tag_position) | > >> > + FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK, > >> > + tag_format)); > >> > + if (ret) > >> > + return ret; > >> > + > >> > + chip_data = priv->chip_data; > >> > >> nit: I would put this assignment up top like in the rest of the driver, > >> respecting reverse-christmass-tree order. It's nice to stick to the > >> existing style. > > > > ok > > > >> > >> > + chip_data->tag_protocol = proto; > >> > + > >> > + return 0; > >> > } > >> > > >> > static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port, > >> > @@ -1739,13 +1784,18 @@ static int rtl8365mb_cpu_config(struct realtek_priv *priv, const struct rtl8365m > >> > > >> > val = FIELD_PREP(RTL8365MB_CPU_CTRL_EN_MASK, cpu->enable ? 1 : 0) | > >> > FIELD_PREP(RTL8365MB_CPU_CTRL_INSERTMODE_MASK, cpu->insert) | > >> > - FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_POSITION_MASK, cpu->position) | > >> > FIELD_PREP(RTL8365MB_CPU_CTRL_RXBYTECOUNT_MASK, cpu->rx_length) | > >> > - FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK, cpu->format) | > >> > FIELD_PREP(RTL8365MB_CPU_CTRL_TRAP_PORT_MASK, cpu->trap_port & 0x7) | > >> > FIELD_PREP(RTL8365MB_CPU_CTRL_TRAP_PORT_EXT_MASK, > >> > cpu->trap_port >> 3 & 0x1); > >> > - ret = regmap_write(priv->map, RTL8365MB_CPU_CTRL_REG, val); > >> > + > >> > + ret = regmap_update_bits(priv->map, RTL8365MB_CPU_CTRL_REG, > >> > + RTL8365MB_CPU_CTRL_EN_MASK | > >> > + RTL8365MB_CPU_CTRL_INSERTMODE_MASK | > >> > + RTL8365MB_CPU_CTRL_RXBYTECOUNT_MASK | > >> > + RTL8365MB_CPU_CTRL_TRAP_PORT_MASK | > >> > + RTL8365MB_CPU_CTRL_TRAP_PORT_EXT_MASK, > >> > + val); > >> > if (ret) > >> > return ret; > >> > > >> > @@ -1827,6 +1877,11 @@ static int rtl8365mb_setup(struct dsa_switch *ds) > >> > dev_info(priv->dev, "no interrupt support\n"); > >> > > >> > /* Configure CPU tagging */ > >> > + ret = rtl8365mb_change_tag_protocol(priv->ds, -1, DSA_TAG_PROTO_RTL8_4); > >> > + if (ret) { > >> > + dev_err(priv->dev, "failed to set default tag protocol: %d\n", ret); > >> > + return ret; > >> > + } > >> > cpu.trap_port = RTL8365MB_MAX_NUM_PORTS; > >> > dsa_switch_for_each_cpu_port(cpu_dp, priv->ds) { > >> > cpu.mask |= BIT(cpu_dp->index); > >> > @@ -1834,13 +1889,9 @@ static int rtl8365mb_setup(struct dsa_switch *ds) > >> > if (cpu.trap_port == RTL8365MB_MAX_NUM_PORTS) > >> > cpu.trap_port = cpu_dp->index; > >> > } > >> > - > >> > cpu.enable = cpu.mask > 0; > >> > 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; > >> > >> Like I said above, I think it would be nice to put this cpu struct back > >> in the rtl8365mb private data. > > > > It would require to split CPU initialization between pre dsa register > > (where format must be defined) and dsa_setup (where cpu port is read > > from dsa ports and settings applied to the switch). get_tag_protocol() > > is called between these two to get the default tag protocol. DSA calls > > change_tag_protocol afterwards if the defined tag protocol in the > > devicetree does not match. > > I don't see the problem, sorry. I am basically suggesting to go back to > the way it was done before. You can always change the tag protocol as > you can the rest of the CPU configuration. I even put a comment at the > top of struct rtl8365mb_cpu to this effect: > > /** > * ... > * Represents the CPU tagging and CPU port configuration of the switch. These > * settings are configurable at runtime. > */ > struct rtl8365mb_cpu { > > > > >> > - > >> > ret = rtl8365mb_cpu_config(priv, &cpu); > >> > if (ret) > >> > goto out_teardown_irq; > >> > @@ -1982,6 +2033,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv) > >> > mb->learn_limit_max = RTL8365MB_LEARN_LIMIT_MAX; > >> > mb->jam_table = rtl8365mb_init_jam_8365mb_vc; > >> > mb->jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc); > >> > + mb->tag_protocol = DSA_TAG_PROTO_RTL8_4; > >> > > >> > break; > >> > default: > >> > @@ -1996,6 +2048,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv) > >> > > >> > static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = { > >> > .get_tag_protocol = rtl8365mb_get_tag_protocol, > >> > + .change_tag_protocol = rtl8365mb_change_tag_protocol, > >> > .setup = rtl8365mb_setup, > >> > .teardown = rtl8365mb_teardown, > >> > .phylink_get_caps = rtl8365mb_phylink_get_caps, > >> > @@ -2014,6 +2067,7 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = { > >> > > >> > static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = { > >> > .get_tag_protocol = rtl8365mb_get_tag_protocol, > >> > + .change_tag_protocol = rtl8365mb_change_tag_protocol, > >> > .setup = rtl8365mb_setup, > >> > .teardown = rtl8365mb_teardown, > >> > .phylink_get_caps = rtl8365mb_phylink_get_caps,
diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c index 2ed592147c20..043cac34e906 100644 --- a/drivers/net/dsa/realtek/rtl8365mb.c +++ b/drivers/net/dsa/realtek/rtl8365mb.c @@ -524,9 +524,7 @@ enum rtl8365mb_cpu_rxlen { * @mask: port mask of ports that parse should parse CPU tags * @trap_port: forward trapped frames to this port * @insert: CPU tag insertion mode in switch->CPU frames - * @position: position of CPU tag in frame * @rx_length: minimum CPU RX length - * @format: CPU tag format * * Represents the CPU tagging and CPU port configuration of the switch. These * settings are configurable at runtime. @@ -536,9 +534,7 @@ struct rtl8365mb_cpu { u32 mask; u32 trap_port; enum rtl8365mb_cpu_insert insert; - enum rtl8365mb_cpu_position position; enum rtl8365mb_cpu_rxlen rx_length; - enum rtl8365mb_cpu_format format; }; /** @@ -566,6 +562,7 @@ 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 + * @tag_protocol: current switch CPU tag protocol * @mib_lock: prevent concurrent reads of MIB counters * @ports: per-port data * @jam_table: chip-specific initialization jam table @@ -580,6 +577,7 @@ struct rtl8365mb { u32 chip_ver; u32 port_mask; u32 learn_limit_max; + enum dsa_tag_protocol tag_protocol; struct mutex mib_lock; struct rtl8365mb_port ports[RTL8365MB_MAX_NUM_PORTS]; const struct rtl8365mb_jam_tbl_entry *jam_table; @@ -770,7 +768,54 @@ static enum dsa_tag_protocol rtl8365mb_get_tag_protocol(struct dsa_switch *ds, int port, enum dsa_tag_protocol mp) { - return DSA_TAG_PROTO_RTL8_4; + struct realtek_priv *priv = ds->priv; + struct rtl8365mb *chip_data; + + chip_data = priv->chip_data; + + return chip_data->tag_protocol; +} + +static int rtl8365mb_change_tag_protocol(struct dsa_switch *ds, int cpu, + enum dsa_tag_protocol proto) +{ + struct realtek_priv *priv = ds->priv; + struct rtl8365mb *chip_data; + int tag_position; + int tag_format; + int ret; + + switch (proto) { + case DSA_TAG_PROTO_RTL8_4: + tag_format = RTL8365MB_CPU_FORMAT_8BYTES; + tag_position = RTL8365MB_CPU_POS_AFTER_SA; + break; + case DSA_TAG_PROTO_RTL8_4T: + tag_format = RTL8365MB_CPU_FORMAT_8BYTES; + tag_position = RTL8365MB_CPU_POS_BEFORE_CRC; + break; + /* The switch also supports a 4-byte format, similar to rtl4a but with + * the same 0x04 8-bit version and probably 8-bit port source/dest. + * There is no public doc about it. Not supported yet. + */ + default: + return -EPROTONOSUPPORT; + } + + ret = regmap_update_bits(priv->map, RTL8365MB_CPU_CTRL_REG, + RTL8365MB_CPU_CTRL_TAG_POSITION_MASK | + RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK, + FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_POSITION_MASK, + tag_position) | + FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK, + tag_format)); + if (ret) + return ret; + + chip_data = priv->chip_data; + chip_data->tag_protocol = proto; + + return 0; } static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port, @@ -1739,13 +1784,18 @@ static int rtl8365mb_cpu_config(struct realtek_priv *priv, const struct rtl8365m val = FIELD_PREP(RTL8365MB_CPU_CTRL_EN_MASK, cpu->enable ? 1 : 0) | FIELD_PREP(RTL8365MB_CPU_CTRL_INSERTMODE_MASK, cpu->insert) | - FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_POSITION_MASK, cpu->position) | FIELD_PREP(RTL8365MB_CPU_CTRL_RXBYTECOUNT_MASK, cpu->rx_length) | - FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK, cpu->format) | FIELD_PREP(RTL8365MB_CPU_CTRL_TRAP_PORT_MASK, cpu->trap_port & 0x7) | FIELD_PREP(RTL8365MB_CPU_CTRL_TRAP_PORT_EXT_MASK, cpu->trap_port >> 3 & 0x1); - ret = regmap_write(priv->map, RTL8365MB_CPU_CTRL_REG, val); + + ret = regmap_update_bits(priv->map, RTL8365MB_CPU_CTRL_REG, + RTL8365MB_CPU_CTRL_EN_MASK | + RTL8365MB_CPU_CTRL_INSERTMODE_MASK | + RTL8365MB_CPU_CTRL_RXBYTECOUNT_MASK | + RTL8365MB_CPU_CTRL_TRAP_PORT_MASK | + RTL8365MB_CPU_CTRL_TRAP_PORT_EXT_MASK, + val); if (ret) return ret; @@ -1827,6 +1877,11 @@ static int rtl8365mb_setup(struct dsa_switch *ds) dev_info(priv->dev, "no interrupt support\n"); /* Configure CPU tagging */ + ret = rtl8365mb_change_tag_protocol(priv->ds, -1, DSA_TAG_PROTO_RTL8_4); + if (ret) { + dev_err(priv->dev, "failed to set default tag protocol: %d\n", ret); + return ret; + } cpu.trap_port = RTL8365MB_MAX_NUM_PORTS; dsa_switch_for_each_cpu_port(cpu_dp, priv->ds) { cpu.mask |= BIT(cpu_dp->index); @@ -1834,13 +1889,9 @@ static int rtl8365mb_setup(struct dsa_switch *ds) if (cpu.trap_port == RTL8365MB_MAX_NUM_PORTS) cpu.trap_port = cpu_dp->index; } - cpu.enable = cpu.mask > 0; 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; - ret = rtl8365mb_cpu_config(priv, &cpu); if (ret) goto out_teardown_irq; @@ -1982,6 +2033,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv) mb->learn_limit_max = RTL8365MB_LEARN_LIMIT_MAX; mb->jam_table = rtl8365mb_init_jam_8365mb_vc; mb->jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc); + mb->tag_protocol = DSA_TAG_PROTO_RTL8_4; break; default: @@ -1996,6 +2048,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv) static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = { .get_tag_protocol = rtl8365mb_get_tag_protocol, + .change_tag_protocol = rtl8365mb_change_tag_protocol, .setup = rtl8365mb_setup, .teardown = rtl8365mb_teardown, .phylink_get_caps = rtl8365mb_phylink_get_caps, @@ -2014,6 +2067,7 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = { static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = { .get_tag_protocol = rtl8365mb_get_tag_protocol, + .change_tag_protocol = rtl8365mb_change_tag_protocol, .setup = rtl8365mb_setup, .teardown = rtl8365mb_teardown, .phylink_get_caps = rtl8365mb_phylink_get_caps,
The trailing tag is also supported by this family. The default is still rtl8_4 but now the switch supports changing the tag to rtl8_4t. Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> --- drivers/net/dsa/realtek/rtl8365mb.c | 78 ++++++++++++++++++++++++----- 1 file changed, 66 insertions(+), 12 deletions(-)