Re: [PATCH net-next v2 2/2] net: dsa: realtek: rtl8365mb: add support for rtl8_4t
From: Alvin Šipraga <ALSI@bang-olufsen.dk>
Date: 2022-02-18 12:26:38
Luiz Angelo Daros de Luca [off-list ref] writes:
quoted hunk ↗ jump to hunk
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 <redacted> --- 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.
quoted hunk ↗ jump to hunk
}; /**@@ -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.
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
- 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,