LinuxLists.cc - [PATCH net-next 00/12] add support for Renesas RZ/N1 ethernet subsystem devices

[permalink] [raw]

Subject: [PATCH net-next 06/12] net: dsa: rzn1-a5psw: add Renesas RZ/N1 advanced 5 port switch driver

Add Renesas RZ/N1 advanced 5 port switch driver. This switch handles 5
ports including 1 CPU management port. A MDIO bus is also exposed by
this switch and allows to communicate with PHYs connected to the ports.
Each switch port (except for the CPU management ports) are connected to
the MII converter.

This driver include basic bridging support, more support will be added
later (vlan, etc).

Suggested-by: Laurent Gonzales <[email protected]>
Suggested-by: Jean-Pierre Geslin <[email protected]>
Suggested-by: Phil Edworthy <[email protected]>
Signed-off-by: Clément Léger <[email protected]>
---
drivers/net/dsa/Kconfig | 9 +
drivers/net/dsa/Makefile | 2 +
drivers/net/dsa/rzn1_a5psw.c | 676 +++++++++++++++++++++++++++++++++++
drivers/net/dsa/rzn1_a5psw.h | 196 ++++++++++
4 files changed, 883 insertions(+)
create mode 100644 drivers/net/dsa/rzn1_a5psw.c
create mode 100644 drivers/net/dsa/rzn1_a5psw.h

diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
index 37a3dabdce31..0896c5fd9dec 100644
--- a/drivers/net/dsa/Kconfig
+++ b/drivers/net/dsa/Kconfig
@@ -70,6 +70,15 @@ config NET_DSA_QCA8K

source "drivers/net/dsa/realtek/Kconfig"

+config NET_DSA_RZN1_A5PSW
+ tristate "Renesas RZ/N1 A5PSW Ethernet switch support"
+ depends on NET_DSA
+ select NET_DSA_TAG_RZN1_A5PSW
+ select PCS_RZN1_MIIC
+ help
+ This driver supports the A5PSW switch, which is embedded in Renesas
+ RZ/N1 SoC.
+
config NET_DSA_SMSC_LAN9303
tristate
depends on VLAN_8021Q || VLAN_8021Q=n
diff --git a/drivers/net/dsa/Makefile b/drivers/net/dsa/Makefile
index e73838c12256..5daf3da4344e 100644
--- a/drivers/net/dsa/Makefile
+++ b/drivers/net/dsa/Makefile
@@ -9,12 +9,14 @@ obj-$(CONFIG_NET_DSA_LANTIQ_GSWIP) += lantiq_gswip.o
obj-$(CONFIG_NET_DSA_MT7530) += mt7530.o
obj-$(CONFIG_NET_DSA_MV88E6060) += mv88e6060.o
obj-$(CONFIG_NET_DSA_QCA8K) += qca8k.o
+obj-$(CONFIG_NET_DSA_RZN1_A5PSW) += rzn1_a5psw.o
obj-$(CONFIG_NET_DSA_SMSC_LAN9303) += lan9303-core.o
obj-$(CONFIG_NET_DSA_SMSC_LAN9303_I2C) += lan9303_i2c.o
obj-$(CONFIG_NET_DSA_SMSC_LAN9303_MDIO) += lan9303_mdio.o
obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX) += vitesse-vsc73xx-core.o
obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX_PLATFORM) += vitesse-vsc73xx-platform.o
obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX_SPI) += vitesse-vsc73xx-spi.o
+
obj-y += b53/
obj-y += hirschmann/
obj-y += microchip/
diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c
new file mode 100644
index 000000000000..5bee999f7050
--- /dev/null
+++ b/drivers/net/dsa/rzn1_a5psw.c
@@ -0,0 +1,676 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022 Schneider-Electric
+ *
+ * Clément Léger <[email protected]>
+ */
+
+#include <linux/clk.h>
+#include <linux/etherdevice.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_mdio.h>
+#include <net/dsa.h>
+#include <uapi/linux/if_bridge.h>
+
+#include "rzn1_a5psw.h"
+
+static void a5psw_reg_writel(struct a5psw *a5psw, int offset, u32 value)
+{
+ writel(value, a5psw->base + offset);
+}
+
+static u32 a5psw_reg_readl(struct a5psw *a5psw, int offset)
+{
+ return readl(a5psw->base + offset);
+}
+
+static void a5psw_reg_rmw(struct a5psw *a5psw, int offset, u32 mask, u32 val)
+{
+ u32 reg;
+
+ spin_lock(&a5psw->reg_lock);
+
+ reg = a5psw_reg_readl(a5psw, offset);
+ reg &= ~mask;
+ reg |= val;
+ a5psw_reg_writel(a5psw, offset, reg);
+
+ spin_unlock(&a5psw->reg_lock);
+}
+
+static enum dsa_tag_protocol a5psw_get_tag_protocol(struct dsa_switch *ds,
+ int port,
+ enum dsa_tag_protocol mp)
+{
+ return DSA_TAG_PROTO_RZN1_A5PSW;
+}
+
+static void a5psw_port_pattern_set(struct a5psw *a5psw, int port, int pattern,
+ bool enable)
+{
+ u32 rx_match = 0;
+
+ if (enable)
+ rx_match |= A5PSW_RXMATCH_CONFIG_PATTERN(pattern);
+
+ a5psw_reg_rmw(a5psw, A5PSW_RXMATCH_CONFIG(port),
+ A5PSW_RXMATCH_CONFIG_PATTERN(pattern), rx_match);
+}
+
+static void a5psw_port_mgmtfwd_set(struct a5psw *a5psw, int port, bool enable)
+{
+ a5psw_port_pattern_set(a5psw, port, A5PSW_PATTERN_MGMTFWD, enable);
+}
+
+static void a5psw_port_enable_set(struct a5psw *a5psw, int port, bool enable)
+{
+ u32 port_ena = 0;
+
+ if (enable)
+ port_ena |= A5PSW_PORT_ENA_TX_RX(port);
+
+ a5psw_reg_rmw(a5psw, A5PSW_PORT_ENA, A5PSW_PORT_ENA_TX_RX(port),
+ port_ena);
+}
+
+static int a5psw_lk_execute_ctrl(struct a5psw *a5psw, u32 *ctrl)
+{
+ int ret;
+
+ a5psw_reg_writel(a5psw, A5PSW_LK_ADDR_CTRL, *ctrl);
+
+ ret = readl_poll_timeout(a5psw->base + A5PSW_LK_ADDR_CTRL,
+ *ctrl,
+ !(*ctrl & A5PSW_LK_ADDR_CTRL_BUSY),
+ A5PSW_LK_BUSY_USEC_POLL,
+ A5PSW_CTRL_TIMEOUT);
+ if (ret)
+ dev_err(a5psw->dev, "LK_CTRL timeout waiting for BUSY bit\n");
+
+ return ret;
+}
+
+static void a5psw_port_fdb_flush(struct a5psw *a5psw, int port)
+{
+ u32 ctrl = A5PSW_LK_ADDR_CTRL_DELETE_PORT | BIT(port);
+
+ spin_lock(&a5psw->lk_lock);
+ a5psw_lk_execute_ctrl(a5psw, &ctrl);
+ spin_unlock(&a5psw->lk_lock);
+}
+
+static void a5psw_port_authorize_set(struct a5psw *a5psw, int port,
+ bool authorize)
+{
+ u32 reg = a5psw_reg_readl(a5psw, A5PSW_AUTH_PORT(port));
+
+ if (authorize)
+ reg |= A5PSW_AUTH_PORT_AUTHORIZED;
+ else
+ reg &= ~A5PSW_AUTH_PORT_AUTHORIZED;
+
+ a5psw_reg_writel(a5psw, A5PSW_AUTH_PORT(port), reg);
+}
+
+static void a5psw_port_disable(struct dsa_switch *ds, int port)
+{
+ struct a5psw *a5psw = ds->priv;
+
+ a5psw_port_authorize_set(a5psw, port, false);
+ a5psw_port_enable_set(a5psw, port, false);
+ a5psw_port_fdb_flush(a5psw, port);
+}
+
+static int a5psw_port_enable(struct dsa_switch *ds, int port,
+ struct phy_device *phy)
+{
+ struct a5psw *a5psw = ds->priv;
+
+ a5psw_port_authorize_set(a5psw, port, true);
+ a5psw_port_enable_set(a5psw, port, true);
+
+ return 0;
+}
+
+static int a5psw_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
+{
+ struct a5psw *a5psw = ds->priv;
+
+ new_mtu += ETH_HLEN + A5PSW_EXTRA_MTU_LEN + ETH_FCS_LEN;
+ a5psw_reg_writel(a5psw, A5PSW_FRM_LENGTH(port), new_mtu);
+
+ return 0;
+}
+
+static int a5psw_port_max_mtu(struct dsa_switch *ds, int port)
+{
+ return A5PSW_JUMBO_LEN - A5PSW_TAG_LEN;
+}
+
+static void a5psw_phylink_validate(struct dsa_switch *ds, int port,
+ unsigned long *supported,
+ struct phylink_link_state *state)
+{
+ __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0 };
+
+ phylink_set_port_modes(mask);
+
+ phylink_set(mask, Autoneg);
+ phylink_set(mask, Pause);
+ phylink_set(mask, Asym_Pause);
+
+ phylink_set(mask, 1000baseT_Full);
+ if (!dsa_is_cpu_port(ds, port)) {
+ phylink_set(mask, 10baseT_Half);
+ phylink_set(mask, 10baseT_Full);
+ phylink_set(mask, 100baseT_Half);
+ phylink_set(mask, 100baseT_Full);
+ }
+
+ linkmode_and(supported, supported, mask);
+ linkmode_and(state->advertising, state->advertising, mask);
+}
+
+static struct phylink_pcs *
+a5psw_phylink_mac_select_pcs(struct dsa_switch *ds, int port,
+ phy_interface_t interface)
+{
+ struct dsa_port *dp = dsa_to_port(ds, port);
+ struct a5psw *a5psw = ds->priv;
+
+ if (!dsa_port_is_cpu(dp) && a5psw->pcs[port])
+ return a5psw->pcs[port];
+
+ return NULL;
+}
+
+static void a5psw_phylink_mac_link_down(struct dsa_switch *ds, int port,
+ unsigned int mode,
+ phy_interface_t interface)
+{
+ struct a5psw *a5psw = ds->priv;
+ u32 cmd_cfg;
+
+ cmd_cfg = a5psw_reg_readl(a5psw, A5PSW_CMD_CFG(port));
+ cmd_cfg &= ~(A5PSW_CMD_CFG_RX_ENA | A5PSW_CMD_CFG_TX_ENA);
+ a5psw_reg_writel(a5psw, A5PSW_CMD_CFG(port), cmd_cfg);
+}
+
+static void a5psw_phylink_mac_link_up(struct dsa_switch *ds, int port,
+ unsigned int mode,
+ phy_interface_t interface,
+ struct phy_device *phydev, int speed,
+ int duplex, bool tx_pause,
+ bool rx_pause)
+{
+ u32 cmd_cfg = A5PSW_CMD_CFG_RX_ENA | A5PSW_CMD_CFG_TX_ENA |
+ A5PSW_CMD_CFG_TX_CRC_APPEND;
+ struct a5psw *a5psw = ds->priv;
+
+ if (speed == SPEED_1000)
+ cmd_cfg |= A5PSW_CMD_CFG_ETH_SPEED;
+
+ if (duplex == DUPLEX_HALF)
+ cmd_cfg |= A5PSW_CMD_CFG_HD_ENA;
+
+ cmd_cfg |= A5PSW_CMD_CFG_CNTL_FRM_ENA;
+
+ if (!rx_pause)
+ cmd_cfg &= ~A5PSW_CMD_CFG_PAUSE_IGNORE;
+
+ a5psw_reg_writel(a5psw, A5PSW_CMD_CFG(port), cmd_cfg);
+}
+
+static int a5psw_set_ageing_time(struct dsa_switch *ds, unsigned int msecs)
+{
+ struct a5psw *a5psw = ds->priv;
+ unsigned long rate;
+ u64 max, tmp;
+ u32 agetime;
+
+ rate = clk_get_rate(a5psw->clk);
+ max = div64_ul(((u64)A5PSW_LK_AGETIME_MASK * A5PSW_TABLE_ENTRIES * 1024),
+ rate) * 1000;
+ if (msecs > max)
+ return -EINVAL;
+
+ tmp = div_u64(rate, MSEC_PER_SEC);
+ agetime = div_u64(msecs * tmp, 1024 * A5PSW_TABLE_ENTRIES);
+
+ a5psw_reg_writel(a5psw, A5PSW_LK_AGETIME, agetime);
+
+ return 0;
+}
+
+static void a5psw_flooding_set_resolution(struct a5psw *a5psw, int port,
+ bool set)
+{
+ u8 offsets[] = {A5PSW_UCAST_DEF_MASK, A5PSW_BCAST_DEF_MASK,
+ A5PSW_MCAST_DEF_MASK};
+ int i;
+
+ if (set)
+ a5psw->flooding_ports |= BIT(port);
+ else
+ a5psw->flooding_ports &= ~BIT(port);
+
+ for (i = 0; i < ARRAY_SIZE(offsets); i++)
+ a5psw_reg_writel(a5psw, offsets[i], a5psw->flooding_ports);
+}
+
+static int a5psw_port_bridge_join(struct dsa_switch *ds, int port,
+ struct dsa_bridge bridge,
+ bool *tx_fwd_offload,
+ struct netlink_ext_ack *extack)
+{
+ struct a5psw *a5psw = ds->priv;
+
+ a5psw_flooding_set_resolution(a5psw, port, true);
+ a5psw_port_mgmtfwd_set(a5psw, port, false);
+
+ return 0;
+}
+
+static void a5psw_port_bridge_leave(struct dsa_switch *ds, int port,
+ struct dsa_bridge bridge)
+{
+ struct a5psw *a5psw = ds->priv;
+
+ a5psw_flooding_set_resolution(a5psw, port, false);
+ a5psw_port_mgmtfwd_set(a5psw, port, true);
+}
+
+static void a5psw_port_stp_state_set(struct dsa_switch *ds, int port,
+ u8 state)
+{
+ struct a5psw *a5psw = ds->priv;
+ u32 reg = 0;
+ u32 mask = A5PSW_INPUT_LEARN_DIS(port) | A5PSW_INPUT_LEARN_BLOCK(port);
+
+ switch (state) {
+ case BR_STATE_DISABLED:
+ case BR_STATE_BLOCKING:
+ reg |= A5PSW_INPUT_LEARN_DIS(port);
+ reg |= A5PSW_INPUT_LEARN_BLOCK(port);
+ break;
+ case BR_STATE_LISTENING:
+ reg |= A5PSW_INPUT_LEARN_DIS(port);
+ break;
+ case BR_STATE_LEARNING:
+ reg |= A5PSW_INPUT_LEARN_BLOCK(port);
+ break;
+ case BR_STATE_FORWARDING:
+ default:
+ break;
+ }
+
+ a5psw_reg_rmw(a5psw, A5PSW_INPUT_LEARN, mask, reg);
+}
+
+static void a5psw_port_fast_age(struct dsa_switch *ds, int port)
+{
+ struct a5psw *a5psw = ds->priv;
+
+ a5psw_port_fdb_flush(a5psw, port);
+}
+
+static int a5psw_setup(struct dsa_switch *ds)
+{
+ struct a5psw *a5psw = ds->priv;
+ int port, vlan, ret;
+ u32 reg;
+
+ /* Configure management port */
+ reg = A5PSW_CPU_PORT | A5PSW_MGMT_CFG_DISCARD;
+ a5psw_reg_writel(a5psw, A5PSW_MGMT_CFG, reg);
+
+ /* Set pattern 0 to forward all frame to mgmt port */
+ a5psw_reg_writel(a5psw, A5PSW_PATTERN_CTRL(0),
+ A5PSW_PATTERN_CTRL_MGMTFWD);
+
+ /* Enable port tagging */
+ reg = FIELD_PREP(A5PSW_MGMT_TAG_CFG_TAGFIELD, A5PSW_MGMT_TAG_VALUE);
+ reg |= A5PSW_MGMT_TAG_CFG_ENABLE | A5PSW_MGMT_TAG_CFG_ALL_FRAMES;
+ a5psw_reg_writel(a5psw, A5PSW_MGMT_TAG_CFG, reg);
+
+ /* Enable normal switch operation */
+ reg = A5PSW_LK_ADDR_CTRL_BLOCKING | A5PSW_LK_ADDR_CTRL_LEARNING |
+ A5PSW_LK_ADDR_CTRL_AGEING | A5PSW_LK_ADDR_CTRL_ALLOW_MIGR |
+ A5PSW_LK_ADDR_CTRL_CLEAR_TABLE;
+ a5psw_reg_writel(a5psw, A5PSW_LK_CTRL, reg);
+
+ ret = readl_poll_timeout(a5psw->base + A5PSW_LK_CTRL,
+ reg,
+ !(reg & A5PSW_LK_ADDR_CTRL_CLEAR_TABLE),
+ A5PSW_LK_BUSY_USEC_POLL,
+ A5PSW_CTRL_TIMEOUT);
+ if (ret) {
+ dev_err(a5psw->dev, "Failed to clear lookup table\n");
+ return ret;
+ }
+
+ /* Reset learn count to 0 */
+ reg = A5PSW_LK_LEARNCOUNT_MODE_SET;
+ a5psw_reg_writel(a5psw, A5PSW_LK_LEARNCOUNT, reg);
+
+ /* Clear VLAN resource table */
+ reg = A5PSW_VLAN_RES_WR_PORTMASK | A5PSW_VLAN_RES_WR_TAGMASK;
+ for (vlan = 0; vlan < A5PSW_VLAN_COUNT; vlan++)
+ a5psw_reg_writel(a5psw, A5PSW_VLAN_RES(vlan), reg);
+
+ /* Reset all ports */
+ for (port = 0; port < ds->num_ports; port++) {
+ /* Reset the port */
+ a5psw_reg_writel(a5psw, A5PSW_CMD_CFG(port),
+ A5PSW_CMD_CFG_SW_RESET);
+
+ /* Enable only CPU port */
+ a5psw_port_enable_set(a5psw, port, dsa_is_cpu_port(ds, port));
+
+ if (dsa_is_unused_port(ds, port))
+ continue;
+
+ /* Enable egress flooding for CPU port */
+ if (dsa_is_cpu_port(ds, port))
+ a5psw_flooding_set_resolution(a5psw, port, true);
+
+ /* Enable management forward only for user ports */
+ if (dsa_is_user_port(ds, port))
+ a5psw_port_mgmtfwd_set(a5psw, port, true);
+ }
+
+ return 0;
+}
+
+const struct dsa_switch_ops a5psw_switch_ops = {
+ .get_tag_protocol = a5psw_get_tag_protocol,
+ .setup = a5psw_setup,
+ .port_disable = a5psw_port_disable,
+ .port_enable = a5psw_port_enable,
+ .phylink_validate = a5psw_phylink_validate,
+ .phylink_mac_select_pcs = a5psw_phylink_mac_select_pcs,
+ .phylink_mac_link_down = a5psw_phylink_mac_link_down,
+ .phylink_mac_link_up = a5psw_phylink_mac_link_up,
+ .port_change_mtu = a5psw_port_change_mtu,
+ .port_max_mtu = a5psw_port_max_mtu,
+ .set_ageing_time = a5psw_set_ageing_time,
+ .port_bridge_join = a5psw_port_bridge_join,
+ .port_bridge_leave = a5psw_port_bridge_leave,
+ .port_stp_state_set = a5psw_port_stp_state_set,
+ .port_fast_age = a5psw_port_fast_age,
+
+};
+
+static int a5psw_mdio_wait_busy(struct a5psw *a5psw)
+{
+ u32 status;
+ int err;
+
+ err = readl_poll_timeout(a5psw->base + A5PSW_MDIO_CFG_STATUS,
+ status,
+ !(status & A5PSW_MDIO_CFG_STATUS_BUSY),
+ 10,
+ 1000 * USEC_PER_MSEC);
+ if (err)
+ dev_err(a5psw->dev, "MDIO command timeout\n");
+
+ return err;
+}
+
+static int a5psw_mdio_read(struct mii_bus *bus, int phy_id, int phy_reg)
+{
+ struct a5psw *a5psw = bus->priv;
+ u32 cmd, status;
+ int ret;
+
+ cmd = A5PSW_MDIO_COMMAND_READ;
+ cmd |= FIELD_PREP(A5PSW_MDIO_COMMAND_REG_ADDR, phy_reg);
+ cmd |= FIELD_PREP(A5PSW_MDIO_COMMAND_PHY_ADDR, phy_id);
+
+ a5psw_reg_writel(a5psw, A5PSW_MDIO_COMMAND, cmd);
+
+ ret = a5psw_mdio_wait_busy(a5psw);
+ if (ret)
+ return ret;
+
+ ret = a5psw_reg_readl(a5psw, A5PSW_MDIO_DATA) & A5PSW_MDIO_DATA_MASK;
+
+ status = a5psw_reg_readl(a5psw, A5PSW_MDIO_CFG_STATUS);
+ if (status & A5PSW_MDIO_CFG_STATUS_READERR)
+ return -EIO;
+
+ return ret;
+}
+
+static int a5psw_mdio_write(struct mii_bus *bus, int phy_id, int phy_reg,
+ u16 phy_data)
+{
+ struct a5psw *a5psw = bus->priv;
+ u32 cmd;
+
+ cmd = FIELD_PREP(A5PSW_MDIO_COMMAND_REG_ADDR, phy_reg);
+ cmd |= FIELD_PREP(A5PSW_MDIO_COMMAND_PHY_ADDR, phy_id);
+
+ a5psw_reg_writel(a5psw, A5PSW_MDIO_COMMAND, cmd);
+ a5psw_reg_writel(a5psw, A5PSW_MDIO_DATA, phy_data);
+
+ return a5psw_mdio_wait_busy(a5psw);
+}
+
+static int a5psw_mdio_reset(struct mii_bus *bus)
+{
+ struct a5psw *a5psw = bus->priv;
+ unsigned long rate;
+ unsigned long div;
+ u32 cfgstatus;
+
+ rate = clk_get_rate(a5psw->hclk);
+ div = ((rate / a5psw->mdio_freq) / 2);
+ if (div >= 511 || div <= 5) {
+ dev_err(a5psw->dev, "MDIO clock div %ld out of range\n", div);
+ return -ERANGE;
+ }
+
+ cfgstatus = FIELD_PREP(A5PSW_MDIO_CFG_STATUS_CLKDIV, div);
+
+ a5psw_reg_writel(a5psw, A5PSW_MDIO_CFG_STATUS, cfgstatus);
+
+ return 0;
+}
+
+static int a5psw_probe_mdio(struct a5psw *a5psw)
+{
+ struct device *dev = a5psw->dev;
+ struct device_node *mdio_node;
+ struct mii_bus *bus;
+ int err;
+
+ if (of_property_read_u32(dev->of_node, "clock-frequency",
+ &a5psw->mdio_freq))
+ a5psw->mdio_freq = A5PSW_MDIO_DEF_FREQ;
+
+ bus = devm_mdiobus_alloc(dev);
+ if (!bus)
+ return -ENOMEM;
+
+ bus->name = "a5psw_mdio";
+ bus->read = a5psw_mdio_read;
+ bus->write = a5psw_mdio_write;
+ bus->reset = a5psw_mdio_reset;
+ bus->priv = a5psw;
+ bus->parent = dev;
+ snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
+
+ a5psw->mii_bus = bus;
+
+ mdio_node = of_get_child_by_name(dev->of_node, "mdio");
+ err = devm_of_mdiobus_register(dev, bus, mdio_node);
+ of_node_put(mdio_node);
+ if (err)
+ return err;
+
+ return 0;
+}
+
+static void a5psw_pcs_free(struct a5psw *a5psw)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(a5psw->pcs); i++) {
+ if (a5psw->pcs[i])
+ miic_destroy(a5psw->pcs[i]);
+ }
+}
+
+static int a5psw_pcs_get(struct a5psw *a5psw)
+{
+ struct device_node *ports, *port, *pcs_node;
+ struct phylink_pcs *pcs;
+ int ret;
+ u32 reg;
+
+ ports = of_get_child_by_name(a5psw->dev->of_node, "ports");
+ if (!ports)
+ return -EINVAL;
+
+ for_each_available_child_of_node(ports, port) {
+ pcs_node = of_parse_phandle(port, "pcs-handle", 0);
+ if (!pcs_node)
+ continue;
+
+ if (of_property_read_u32(port, "reg", &reg)) {
+ ret = -EINVAL;
+ goto free_pcs;
+ }
+
+ if (reg >= ARRAY_SIZE(a5psw->pcs)) {
+ ret = -ENODEV;
+ goto free_pcs;
+ }
+
+ pcs = miic_create(pcs_node);
+ if (IS_ERR(pcs)) {
+ ret = PTR_ERR(pcs);
+ goto free_pcs;
+ }
+
+ a5psw->pcs[reg] = pcs;
+ }
+ of_node_put(ports);
+
+ return 0;
+
+free_pcs:
+ a5psw_pcs_free(a5psw);
+
+ return ret;
+}
+
+static int a5psw_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct dsa_switch *ds;
+ struct a5psw *a5psw;
+ int ret;
+
+ a5psw = devm_kzalloc(dev, sizeof(*a5psw), GFP_KERNEL);
+ if (!a5psw)
+ return -ENOMEM;
+
+ a5psw->dev = dev;
+ spin_lock_init(&a5psw->lk_lock);
+ spin_lock_init(&a5psw->reg_lock);
+ a5psw->base = devm_platform_ioremap_resource(pdev, 0);
+ if (!a5psw->base)
+ return -EINVAL;
+
+ /* Probe PCS */
+ ret = a5psw_pcs_get(a5psw);
+ if (ret)
+ return ret;
+
+ a5psw->hclk = devm_clk_get(dev, "hclk");
+ if (IS_ERR(a5psw->hclk)) {
+ dev_err(dev, "failed get hclk clock\n");
+ ret = PTR_ERR(a5psw->hclk);
+ goto free_pcs;
+ }
+
+ a5psw->clk = devm_clk_get(dev, "clk");
+ if (IS_ERR(a5psw->clk)) {
+ dev_err(dev, "failed get clk_switch clock\n");
+ ret = PTR_ERR(a5psw->clk);
+ goto free_pcs;
+ }
+
+ ret = clk_prepare_enable(a5psw->clk);
+ if (ret)
+ goto free_pcs;
+
+ ret = clk_prepare_enable(a5psw->hclk);
+ if (ret)
+ goto clk_disable;
+
+ ret = a5psw_probe_mdio(a5psw);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to register MDIO: %d\n", ret);
+ goto hclk_disable;
+ }
+
+ ds = &a5psw->ds;
+ ds->dev = &pdev->dev;
+ ds->num_ports = A5PSW_PORTS_NUM;
+ ds->ops = &a5psw_switch_ops;
+ ds->priv = a5psw;
+
+ ret = dsa_register_switch(ds);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to register DSA switch: %d\n", ret);
+ goto hclk_disable;
+ }
+
+ return 0;
+
+hclk_disable:
+ clk_disable_unprepare(a5psw->hclk);
+clk_disable:
+ clk_disable_unprepare(a5psw->clk);
+free_pcs:
+ a5psw_pcs_free(a5psw);
+
+ return ret;
+}
+
+static int a5psw_remove(struct platform_device *pdev)
+{
+ struct a5psw *a5psw = platform_get_drvdata(pdev);
+
+ dsa_unregister_switch(&a5psw->ds);
+ a5psw_pcs_free(a5psw);
+ clk_disable_unprepare(a5psw->hclk);
+ clk_disable_unprepare(a5psw->clk);
+
+ return 0;
+}
+
+static const struct of_device_id a5psw_of_mtable[] = {
+ { .compatible = "renesas,rzn1-a5psw", },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, a5psw_of_mtable);
+
+static struct platform_driver a5psw_driver = {
+ .driver = {
+ .name = "rzn1_a5psw",
+ .of_match_table = of_match_ptr(a5psw_of_mtable),
+ },
+ .probe = a5psw_probe,
+ .remove = a5psw_remove,
+};
+module_platform_driver(a5psw_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Renesas RZ/N1 Advanced 5-port Switch driver");
+MODULE_AUTHOR("Clément Léger <[email protected]>");
diff --git a/drivers/net/dsa/rzn1_a5psw.h b/drivers/net/dsa/rzn1_a5psw.h
new file mode 100644
index 000000000000..2d96a2afbc3a
--- /dev/null
+++ b/drivers/net/dsa/rzn1_a5psw.h
@@ -0,0 +1,196 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2022 Schneider Electric
+ *
+ * Clément Léger <[email protected]>
+ */
+
+#include <linux/clk.h>
+#include <linux/debugfs.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_mdio.h>
+#include <linux/platform_device.h>
+#include <linux/pcs-rzn1-miic.h>
+#include <net/dsa.h>
+
+#define A5PSW_REVISION 0x0
+#define A5PSW_PORT_OFFSET(port) (0x400 * (port))
+
+#define A5PSW_PORT_ENA 0x8
+#define A5PSW_PORT_ENA_RX_SHIFT 16
+#define A5PSW_PORT_ENA_TX_RX(port) (BIT((port) + A5PSW_PORT_ENA_RX_SHIFT) | \
+ BIT(port))
+#define A5PSW_UCAST_DEF_MASK 0xC
+
+#define A5PSW_VLAN_VERIFY 0x10
+#define A5PSW_VLAN_VERI_SHIFT 0
+#define A5PSW_VLAN_DISC_SHIFT 16
+
+#define A5PSW_BCAST_DEF_MASK 0x14
+#define A5PSW_MCAST_DEF_MASK 0x18
+
+#define A5PSW_INPUT_LEARN 0x1C
+#define A5PSW_INPUT_LEARN_DIS(p) BIT((p) + 16)
+#define A5PSW_INPUT_LEARN_BLOCK(p) BIT(p)
+
+#define A5PSW_MGMT_CFG 0x20
+#define A5PSW_MGMT_CFG_DISCARD BIT(7)
+
+#define A5PSW_MODE_CFG 0x24
+#define A5PSW_MODE_STATS_RESET BIT(31)
+
+#define A5PSW_VLAN_IN_MODE 0x28
+#define A5PSW_VLAN_IN_MODE_PORT_SHIFT(port) ((port) * 2)
+#define A5PSW_VLAN_IN_MODE_PORT(port) (GENMASK(1, 0) << \
+ A5PSW_VLAN_IN_MODE_PORT_SHIFT(port))
+#define A5PSW_VLAN_IN_MODE_SINGLE_PASSTHROUGH 0x0
+#define A5PSW_VLAN_IN_MODE_SINGLE_REPLACE 0x1
+#define A5PSW_VLAN_IN_MODE_TAG_ALWAYS 0x2
+
+#define A5PSW_VLAN_OUT_MODE 0x2C
+#define A5PSW_VLAN_OUT_MODE_PORT(port) (GENMASK(1, 0) << ((port) * 2))
+#define A5PSW_VLAN_OUT_MODE_DIS 0x0
+#define A5PSW_VLAN_OUT_MODE_STRIP 0x1
+#define A5PSW_VLAN_OUT_MODE_TAG_THROUGH 0x2
+#define A5PSW_VLAN_OUT_MODE_TRANSPARENT 0x3
+
+#define A5PSW_VLAN_IN_MODE_ENA 0x30
+#define A5PSW_VLAN_TAG_ID 0x34
+
+#define A5PSW_SYSTEM_TAGINFO(port) (0x200 + A5PSW_PORT_OFFSET(port))
+
+#define A5PSW_AUTH_PORT(port) (0x240 + 4 * (port))
+#define A5PSW_AUTH_PORT_AUTHORIZED BIT(0)
+
+#define A5PSW_VLAN_RES(entry) (0x280 + 4 * (entry))
+#define A5PSW_VLAN_RES_WR_PORTMASK BIT(30)
+#define A5PSW_VLAN_RES_WR_TAGMASK BIT(29)
+#define A5PSW_VLAN_RES_RD_TAGMASK BIT(28)
+#define A5PSW_VLAN_RES_ID GENMASK(16, 5)
+#define A5PSW_VLAN_RES_PORTMASK GENMASK(4, 0)
+
+#define A5PSW_RXMATCH_CONFIG(port) (0x3e80 + 4 * (port))
+#define A5PSW_RXMATCH_CONFIG_PATTERN(p) BIT(p)
+
+#define A5PSW_PATTERN_CTRL(p) (0x3eb0 + 4 * (p))
+#define A5PSW_PATTERN_CTRL_MGMTFWD BIT(1)
+
+#define A5PSW_LK_CTRL 0x400
+#define A5PSW_LK_ADDR_CTRL_BLOCKING BIT(0)
+#define A5PSW_LK_ADDR_CTRL_LEARNING BIT(1)
+#define A5PSW_LK_ADDR_CTRL_AGEING BIT(2)
+#define A5PSW_LK_ADDR_CTRL_ALLOW_MIGR BIT(3)
+#define A5PSW_LK_ADDR_CTRL_CLEAR_TABLE BIT(6)
+
+#define A5PSW_LK_ADDR_CTRL 0x408
+#define A5PSW_LK_ADDR_CTRL_BUSY BIT(31)
+#define A5PSW_LK_ADDR_CTRL_DELETE_PORT BIT(30)
+#define A5PSW_LK_ADDR_CTRL_CLEAR BIT(29)
+#define A5PSW_LK_ADDR_CTRL_LOOKUP BIT(28)
+#define A5PSW_LK_ADDR_CTRL_WAIT BIT(27)
+#define A5PSW_LK_ADDR_CTRL_READ BIT(26)
+#define A5PSW_LK_ADDR_CTRL_WRITE BIT(25)
+#define A5PSW_LK_ADDR_CTRL_ADDRESS GENMASK(12, 0)
+
+#define A5PSW_LK_DATA_LO 0x40C
+#define A5PSW_LK_DATA_HI 0x410
+#define A5PSW_LK_DATA_HI_VALID BIT(16)
+#define A5PSW_LK_DATA_HI_PORT BIT(16)
+
+#define A5PSW_LK_LEARNCOUNT 0x418
+#define A5PSW_LK_LEARNCOUNT_COUNT GENMASK(13, 0)
+#define A5PSW_LK_LEARNCOUNT_MODE GENMASK(31, 30)
+#define A5PSW_LK_LEARNCOUNT_MODE_SET 0x0
+#define A5PSW_LK_LEARNCOUNT_MODE_INC 0x1
+#define A5PSW_LK_LEARNCOUNT_MODE_DEC 0x2
+
+#define A5PSW_MGMT_TAG_CFG 0x480
+#define A5PSW_MGMT_TAG_CFG_TAGFIELD GENMASK(31, 16)
+#define A5PSW_MGMT_TAG_CFG_ALL_FRAMES BIT(1)
+#define A5PSW_MGMT_TAG_CFG_ENABLE BIT(0)
+
+#define A5PSW_LK_AGETIME 0x41C
+#define A5PSW_LK_AGETIME_MASK GENMASK(23, 0)
+
+#define A5PSW_MDIO_CFG_STATUS 0x700
+#define A5PSW_MDIO_CFG_STATUS_CLKDIV GENMASK(15, 7)
+#define A5PSW_MDIO_CFG_STATUS_READERR BIT(1)
+#define A5PSW_MDIO_CFG_STATUS_BUSY BIT(0)
+
+#define A5PSW_MDIO_COMMAND 0x704
+/* Register is named TRAININIT in datasheet and should be set when reading */
+#define A5PSW_MDIO_COMMAND_READ BIT(15)
+#define A5PSW_MDIO_COMMAND_PHY_ADDR GENMASK(9, 5)
+#define A5PSW_MDIO_COMMAND_REG_ADDR GENMASK(4, 0)
+
+#define A5PSW_MDIO_DATA 0x708
+#define A5PSW_MDIO_DATA_MASK GENMASK(15, 0)
+
+#define A5PSW_CMD_CFG(port) (0x808 + A5PSW_PORT_OFFSET(port))
+#define A5PSW_CMD_CFG_CNTL_FRM_ENA BIT(23)
+#define A5PSW_CMD_CFG_SW_RESET BIT(13)
+#define A5PSW_CMD_CFG_TX_CRC_APPEND BIT(11)
+#define A5PSW_CMD_CFG_HD_ENA BIT(10)
+#define A5PSW_CMD_CFG_PAUSE_IGNORE BIT(8)
+#define A5PSW_CMD_CFG_CRC_FWD BIT(6)
+#define A5PSW_CMD_CFG_ETH_SPEED BIT(3)
+#define A5PSW_CMD_CFG_RX_ENA BIT(1)
+#define A5PSW_CMD_CFG_TX_ENA BIT(0)
+
+#define A5PSW_FRM_LENGTH(port) (0x814 + A5PSW_PORT_OFFSET(port))
+#define A5PSW_FRM_LENGTH_MASK GENMASK(13, 0)
+
+#define A5PSW_STATUS(port) (0x840 + A5PSW_PORT_OFFSET(port))
+
+#define A5PSW_STATS_HIWORD 0x900
+
+#define A5PSW_DUMMY_WORKAROUND 0x5000
+
+#define A5PSW_VLAN_TAG(prio, id) (((prio) << 12) | (id))
+#define A5PSW_PORTS_NUM 5
+#define A5PSW_CPU_PORT (A5PSW_PORTS_NUM - 1)
+#define A5PSW_MDIO_DEF_FREQ 2500000
+#define A5PSW_MDIO_TIMEOUT 100
+#define A5PSW_JUMBO_LEN (10 * SZ_1K)
+#define A5PSW_TAG_LEN 8
+#define A5PSW_VLAN_COUNT 32
+
+/* Ensure enough space for 2 VLAN tags */
+#define A5PSW_EXTRA_MTU_LEN (A5PSW_TAG_LEN + 8)
+#define A5PSW_MGMT_TAG_VALUE 0xE001
+
+#define A5PSW_PATTERN_MGMTFWD 0
+
+#define A5PSW_LK_BUSY_USEC_POLL 10
+#define A5PSW_CTRL_TIMEOUT 1000
+#define A5PSW_TABLE_ENTRIES 8192
+
+/**
+ * struct a5psw - switch struct
+ * @base: Base address of the switch
+ * @hclk_rate: hclk_switch clock rate
+ * @clk_rate: clk_switch clock rate
+ * @dev: Device associated to the switch
+ * @mii_bus: MDIO bus struct
+ * @mdio_freq: MDIO bus frequency requested
+ * @pcs: Array of PCS connected to the switch ports (not for the CPU)
+ * @ds: DSA switch struct
+ * @lk_lock: Lock for the lookup table
+ * @reg_lock: Lock for register read-modify-write operation
+ * @flooding_ports: List of ports that should be flooded
+ */
+struct a5psw {
+ void __iomem *base;
+ struct clk* hclk;
+ struct clk* clk;
+ struct device *dev;
+ struct mii_bus *mii_bus;
+ u32 mdio_freq;
+ struct phylink_pcs *pcs[A5PSW_PORTS_NUM - 1];
+ struct dsa_switch ds;
+ spinlock_t lk_lock;
+ spinlock_t reg_lock;
+ u32 flooding_ports;
+};
--
2.34.1

2022-04-15 13:21:41

[permalink] [raw]

Subject: [PATCH net-next 07/12] net: dsa: rzn1-a5psw: add statistics support

Add per-port statistics. This support requries to add a stat lock since
statistics are stored in two 32 bits registers, the hi part one being
global and latched when accessing the lo part.

Signed-off-by: Clément Léger <[email protected]>
---
drivers/net/dsa/rzn1_a5psw.c | 101 +++++++++++++++++++++++++++++++++++
drivers/net/dsa/rzn1_a5psw.h | 2 +
2 files changed, 103 insertions(+)

diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c
index 5bee999f7050..7ab7d9054427 100644
--- a/drivers/net/dsa/rzn1_a5psw.c
+++ b/drivers/net/dsa/rzn1_a5psw.c
@@ -16,6 +16,59 @@

#include "rzn1_a5psw.h"

+struct a5psw_stats {
+ u16 offset;
+ const char *name;
+};
+
+#define STAT_DESC(_offset, _name) {.offset = _offset, .name = _name}
+
+static const struct a5psw_stats a5psw_stats[] = {
+ STAT_DESC(0x868, "aFrameTransmitted"),
+ STAT_DESC(0x86C, "aFrameReceived"),
+ STAT_DESC(0x870, "aFrameCheckSequenceErrors"),
+ STAT_DESC(0x874, "aAlignmentErrors"),
+ STAT_DESC(0x878, "aOctetsTransmitted"),
+ STAT_DESC(0x87C, "aOctetsReceived"),
+ STAT_DESC(0x880, "aTxPAUSEMACCtrlFrames"),
+ STAT_DESC(0x884, "aRxPAUSEMACCtrlFrames"),
+ /* If */
+ STAT_DESC(0x888, "ifInErrors"),
+ STAT_DESC(0x88C, "ifOutErrors"),
+ STAT_DESC(0x890, "ifInUcastPkts"),
+ STAT_DESC(0x894, "ifInMulticastPkts"),
+ STAT_DESC(0x898, "ifInBroadcastPkts"),
+ STAT_DESC(0x89C, "ifOutDiscards"),
+ STAT_DESC(0x8A0, "ifOutUcastPkts"),
+ STAT_DESC(0x8A4, "ifOutMulticastPkts"),
+ STAT_DESC(0x8A8, "ifOutBroadcastPkts"),
+ /* Ether */
+ STAT_DESC(0x8AC, "etherStatsDropEvents"),
+ STAT_DESC(0x8B0, "etherStatsOctets"),
+ STAT_DESC(0x8B4, "etherStatsPkts"),
+ STAT_DESC(0x8B8, "etherStatsUndersizePkts"),
+ STAT_DESC(0x8BC, "etherStatsetherStatsOversizePktsDropEvents"),
+ STAT_DESC(0x8C0, "etherStatsPkts64Octets"),
+ STAT_DESC(0x8C4, "etherStatsPkts65to127Octets"),
+ STAT_DESC(0x8C8, "etherStatsPkts128to255Octets"),
+ STAT_DESC(0x8CC, "etherStatsPkts256to511Octets"),
+ STAT_DESC(0x8D0, "etherStatsPkts512to1023Octets"),
+ STAT_DESC(0x8D4, "etherStatsPkts1024to1518Octets"),
+ STAT_DESC(0x8D8, "etherStatsPkts1519toXOctets"),
+ STAT_DESC(0x8DC, "etherStatsJabbers"),
+ STAT_DESC(0x8E0, "etherStatsFragments"),
+
+ STAT_DESC(0x8E8, "VLANReceived"),
+ STAT_DESC(0x8EC, "VLANTransmitted"),
+
+ STAT_DESC(0x910, "aDeferred"),
+ STAT_DESC(0x914, "aMultipleCollisions"),
+ STAT_DESC(0x918, "aSingleCollisions"),
+ STAT_DESC(0x91C, "aLateCollisions"),
+ STAT_DESC(0x920, "aExcessiveCollisions"),
+ STAT_DESC(0x924, "aCarrierSenseErrors"),
+};
+
static void a5psw_reg_writel(struct a5psw *a5psw, int offset, u32 value)
{
writel(value, a5psw->base + offset);
@@ -316,6 +369,50 @@ static void a5psw_port_fast_age(struct dsa_switch *ds, int port)
a5psw_port_fdb_flush(a5psw, port);
}

+static void a5psw_get_strings(struct dsa_switch *ds, int port, u32 stringset,
+ uint8_t *data)
+{
+ unsigned int u;
+
+ if (stringset != ETH_SS_STATS)
+ return;
+
+ for (u = 0; u < ARRAY_SIZE(a5psw_stats); u++) {
+ strncpy(data + u * ETH_GSTRING_LEN, a5psw_stats[u].name,
+ ETH_GSTRING_LEN);
+ }
+}
+
+static void a5psw_get_ethtool_stats(struct dsa_switch *ds, int port,
+ uint64_t *data)
+{
+ struct a5psw *a5psw = ds->priv;
+ u32 reg_lo, reg_hi;
+ unsigned int u;
+
+ for (u = 0; u < ARRAY_SIZE(a5psw_stats); u++) {
+ /* A5PSW_STATS_HIWORD is global and thus, access must be
+ * exclusive
+ */
+ spin_lock(&a5psw->stats_lock);
+ reg_lo = a5psw_reg_readl(a5psw, a5psw_stats[u].offset +
+ A5PSW_PORT_OFFSET(port));
+ /* A5PSW_STATS_HIWORD is latched on stat read */
+ reg_hi = a5psw_reg_readl(a5psw, A5PSW_STATS_HIWORD);
+
+ data[u] = ((u64)reg_hi << 32) | reg_lo;
+ spin_unlock(&a5psw->stats_lock);
+ }
+}
+
+static int a5psw_get_sset_count(struct dsa_switch *ds, int port, int sset)
+{
+ if (sset != ETH_SS_STATS)
+ return 0;
+
+ return ARRAY_SIZE(a5psw_stats);
+}
+
static int a5psw_setup(struct dsa_switch *ds)
{
struct a5psw *a5psw = ds->priv;
@@ -395,6 +492,9 @@ const struct dsa_switch_ops a5psw_switch_ops = {
.phylink_mac_link_up = a5psw_phylink_mac_link_up,
.port_change_mtu = a5psw_port_change_mtu,
.port_max_mtu = a5psw_port_max_mtu,
+ .get_sset_count = a5psw_get_sset_count,
+ .get_strings = a5psw_get_strings,
+ .get_ethtool_stats = a5psw_get_ethtool_stats,
.set_ageing_time = a5psw_set_ageing_time,
.port_bridge_join = a5psw_port_bridge_join,
.port_bridge_leave = a5psw_port_bridge_leave,
@@ -580,6 +680,7 @@ static int a5psw_probe(struct platform_device *pdev)
return -ENOMEM;

a5psw->dev = dev;
+ spin_lock_init(&a5psw->stats_lock);
spin_lock_init(&a5psw->lk_lock);
spin_lock_init(&a5psw->reg_lock);
a5psw->base = devm_platform_ioremap_resource(pdev, 0);
diff --git a/drivers/net/dsa/rzn1_a5psw.h b/drivers/net/dsa/rzn1_a5psw.h
index 2d96a2afbc3a..b34ea549e936 100644
--- a/drivers/net/dsa/rzn1_a5psw.h
+++ b/drivers/net/dsa/rzn1_a5psw.h
@@ -177,6 +177,7 @@
* @mdio_freq: MDIO bus frequency requested
* @pcs: Array of PCS connected to the switch ports (not for the CPU)
* @ds: DSA switch struct
+ * @stats_lock: lock to access statistics (shared HI counter)
* @lk_lock: Lock for the lookup table
* @reg_lock: Lock for register read-modify-write operation
* @flooding_ports: List of ports that should be flooded
@@ -190,6 +191,7 @@ struct a5psw {
u32 mdio_freq;
struct phylink_pcs *pcs[A5PSW_PORTS_NUM - 1];
struct dsa_switch ds;
+ spinlock_t stats_lock;
spinlock_t lk_lock;
spinlock_t reg_lock;
u32 flooding_ports;
--
2.34.1

2022-04-15 14:34:14

by Russell King (Oracle)

[permalink] [raw]

Subject: Re: [PATCH net-next 06/12] net: dsa: rzn1-a5psw: add Renesas RZ/N1 advanced 5 port switch driver

On Fri, Apr 15, 2022 at 10:40:29AM +0200, Cl?ment L?ger wrote:
> Le Thu, 14 Apr 2022 14:02:10 +0100,
> "Russell King (Oracle)" <[email protected]> a ?crit :
>
> > On Thu, Apr 14, 2022 at 02:22:44PM +0200, Cl?ment L?ger wrote:
> > > Add Renesas RZ/N1 advanced 5 port switch driver. This switch handles 5
> > > ports including 1 CPU management port. A MDIO bus is also exposed by
> > > this switch and allows to communicate with PHYs connected to the ports.
> > > Each switch port (except for the CPU management ports) are connected to
> > > the MII converter.
> > >
> > > This driver include basic bridging support, more support will be added
> > > later (vlan, etc).
> >
> > This patch looks to me like it needs to be updated...
>
> Hi Russell,
>
> When you say so, do you expect the VLAN support to be included ?

I was referring to the use of .phylink_validate rather than
.phylink_get_caps - all but one DSA driver have been recently updated
to use the latter, and the former should now only be used in
exceptional circumstances.

Thanks.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

2022-04-15 18:16:19

[permalink] [raw]

Subject: [PATCH net-next 08/12] net: dsa: rzn1-a5psw: add FDB support

This commits add forwarding database support to the driver. It
implements fdb_add(), fdb_del() and fdb_dump().

Signed-off-by: Clément Léger <[email protected]>
---
drivers/net/dsa/rzn1_a5psw.c | 163 +++++++++++++++++++++++++++++++++++
drivers/net/dsa/rzn1_a5psw.h | 16 ++++
2 files changed, 179 insertions(+)

diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c
index 7ab7d9054427..8c763c2a1a1f 100644
--- a/drivers/net/dsa/rzn1_a5psw.c
+++ b/drivers/net/dsa/rzn1_a5psw.c
@@ -369,6 +369,166 @@ static void a5psw_port_fast_age(struct dsa_switch *ds, int port)
a5psw_port_fdb_flush(a5psw, port);
}

+static int a5psw_lk_execute_lookup(struct a5psw *a5psw, union lk_data *lk_data,
+ u16 *entry)
+{
+ u32 ctrl;
+ int ret;
+
+ a5psw_reg_writel(a5psw, A5PSW_LK_DATA_LO, lk_data->lo);
+ a5psw_reg_writel(a5psw, A5PSW_LK_DATA_HI, lk_data->hi);
+
+ ctrl = A5PSW_LK_ADDR_CTRL_LOOKUP;
+ ret = a5psw_lk_execute_ctrl(a5psw, &ctrl);
+ if (ret)
+ return ret;
+
+ *entry = ctrl & A5PSW_LK_ADDR_CTRL_ADDRESS;
+
+ return 0;
+}
+
+static int a5psw_port_fdb_add(struct dsa_switch *ds, int port,
+ const unsigned char *addr, u16 vid,
+ struct dsa_db db)
+{
+ struct a5psw *a5psw = ds->priv;
+ union lk_data lk_data = {0};
+ bool inc_learncount = false;
+ int ret = 0;
+ u16 entry;
+ u32 reg;
+
+ ether_addr_copy(lk_data.entry.mac, addr);
+ lk_data.entry.port_mask = BIT(port);
+
+ spin_lock(&a5psw->lk_lock);
+
+ /* Set the value to be written in the lookup table */
+ ret = a5psw_lk_execute_lookup(a5psw, &lk_data, &entry);
+ if (ret)
+ goto lk_unlock;
+
+ lk_data.hi = a5psw_reg_readl(a5psw, A5PSW_LK_DATA_HI);
+ if (!lk_data.entry.valid) {
+ inc_learncount = true;
+ /* port_mask set to 0x1f when entry is not valid, clear it */
+ lk_data.entry.port_mask = 0;
+ lk_data.entry.prio = 0;
+ }
+
+ lk_data.entry.port_mask |= BIT(port);
+ lk_data.entry.is_static = 1;
+ lk_data.entry.valid = 1;
+
+ a5psw_reg_writel(a5psw, A5PSW_LK_DATA_HI, lk_data.hi);
+
+ reg = A5PSW_LK_ADDR_CTRL_WRITE | entry;
+ ret = a5psw_lk_execute_ctrl(a5psw, &reg);
+ if (ret)
+ goto lk_unlock;
+
+ if (inc_learncount) {
+ reg = A5PSW_LK_LEARNCOUNT_MODE_INC;
+ a5psw_reg_writel(a5psw, A5PSW_LK_LEARNCOUNT, reg);
+ }
+
+lk_unlock:
+ spin_unlock(&a5psw->lk_lock);
+
+ return ret;
+}
+
+static int a5psw_port_fdb_del(struct dsa_switch *ds, int port,
+ const unsigned char *addr, u16 vid,
+ struct dsa_db db)
+{
+ struct a5psw *a5psw = ds->priv;
+ union lk_data lk_data = {0};
+ bool clear = false;
+ int ret = 0;
+ u16 entry;
+ u32 reg;
+
+ ether_addr_copy(lk_data.entry.mac, addr);
+
+ spin_lock(&a5psw->lk_lock);
+
+ ret = a5psw_lk_execute_lookup(a5psw, &lk_data, &entry);
+ if (ret) {
+ dev_err(a5psw->dev, "Failed to lookup mac address\n");
+ goto lk_unlock;
+ }
+
+ lk_data.hi = a5psw_reg_readl(a5psw, A5PSW_LK_DATA_HI);
+ if (!lk_data.entry.valid) {
+ dev_err(a5psw->dev, "Tried to remove non-existing entry\n");
+ ret = -ENOENT;
+ goto lk_unlock;
+ }
+
+ lk_data.entry.port_mask &= ~BIT(port);
+ /* If there is no more port in the mask, clear the entry */
+ if (lk_data.entry.port_mask == 0)
+ clear = true;
+
+ a5psw_reg_writel(a5psw, A5PSW_LK_DATA_HI, lk_data.hi);
+
+ reg = entry;
+ if (clear)
+ reg |= A5PSW_LK_ADDR_CTRL_CLEAR;
+ else
+ reg |= A5PSW_LK_ADDR_CTRL_WRITE;
+
+ ret = a5psw_lk_execute_ctrl(a5psw, &reg);
+ if (ret)
+ goto lk_unlock;
+
+ /* Decrement LEARNCOUNT */
+ if (clear) {
+ reg = A5PSW_LK_LEARNCOUNT_MODE_DEC;
+ a5psw_reg_writel(a5psw, A5PSW_LK_LEARNCOUNT, reg);
+ }
+
+lk_unlock:
+ spin_unlock(&a5psw->lk_lock);
+
+ return ret;
+}
+
+static int a5psw_port_fdb_dump(struct dsa_switch *ds, int port,
+ dsa_fdb_dump_cb_t *cb, void *data)
+{
+ struct a5psw *a5psw = ds->priv;
+ union lk_data lk_data;
+ int i = 0, ret;
+ u32 reg;
+
+ for (i = 0; i < A5PSW_TABLE_ENTRIES; i++) {
+ reg = A5PSW_LK_ADDR_CTRL_READ | A5PSW_LK_ADDR_CTRL_WAIT | i;
+ spin_lock(&a5psw->lk_lock);
+
+ ret = a5psw_lk_execute_ctrl(a5psw, &reg);
+ if (ret)
+ return ret;
+
+ lk_data.hi = a5psw_reg_readl(a5psw, A5PSW_LK_DATA_HI);
+ /* If entry is not valid or does not contain the port, skip */
+ if (!lk_data.entry.valid ||
+ !(lk_data.entry.port_mask & BIT(port))) {
+ spin_unlock(&a5psw->lk_lock);
+ continue;
+ }
+
+ lk_data.lo = a5psw_reg_readl(a5psw, A5PSW_LK_DATA_LO);
+ spin_unlock(&a5psw->lk_lock);
+
+ cb(lk_data.entry.mac, 0, lk_data.entry.is_static, data);
+ }
+
+ return 0;
+}
+
static void a5psw_get_strings(struct dsa_switch *ds, int port, u32 stringset,
uint8_t *data)
{
@@ -500,6 +660,9 @@ const struct dsa_switch_ops a5psw_switch_ops = {
.port_bridge_leave = a5psw_port_bridge_leave,
.port_stp_state_set = a5psw_port_stp_state_set,
.port_fast_age = a5psw_port_fast_age,
+ .port_fdb_add = a5psw_port_fdb_add,
+ .port_fdb_del = a5psw_port_fdb_del,
+ .port_fdb_dump = a5psw_port_fdb_dump,

};

diff --git a/drivers/net/dsa/rzn1_a5psw.h b/drivers/net/dsa/rzn1_a5psw.h
index b34ea549e936..37aa89383e70 100644
--- a/drivers/net/dsa/rzn1_a5psw.h
+++ b/drivers/net/dsa/rzn1_a5psw.h
@@ -167,6 +167,22 @@
#define A5PSW_CTRL_TIMEOUT 1000
#define A5PSW_TABLE_ENTRIES 8192

+struct fdb_entry {
+ u8 mac[ETH_ALEN];
+ u8 valid:1;
+ u8 is_static:1;
+ u8 prio:3;
+ u8 port_mask:5;
+} __packed;
+
+union lk_data {
+ struct {
+ u32 lo;
+ u32 hi;
+ };
+ struct fdb_entry entry;
+};
+
/**
* struct a5psw - switch struct
* @base: Base address of the switch
--
2.34.1

2022-04-15 18:47:27

[permalink] [raw]

Subject: [PATCH net-next 12/12] MAINTAINERS: add Renesas RZ/N1 switch related driver entry

After contributing the drivers, volunteer for maintenance and add
myself as the maintainer for Renesas RZ/N1 switch related drivers.

Signed-off-by: Clément Léger <[email protected]>
---
MAINTAINERS | 11 +++++++++++
1 file changed, 11 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 9b0480f1b153..8b25308d523e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16856,6 +16856,17 @@ S: Supported
F: Documentation/devicetree/bindings/iio/adc/renesas,rzg2l-adc.yaml
F: drivers/iio/adc/rzg2l_adc.c

+RENESAS RZ/N1 A5PSW SWITCH DRIVER
+M: Clément Léger <[email protected]>
+S: Maintained
+F: Documentation/devicetree/bindings/net/dsa/renesas,rzn1-a5psw.yaml
+F: Documentation/devicetree/bindings/net/pcs/renesas,rzn1-miic.yaml
+F: drivers/net/dsa/rzn1_a5psw*
+F: drivers/net/pcs/pcs-rzn1-miic.c
+F: include/dt-bindings/net/pcs-rzn1-miic.h
+F: include/linux/pcs-rzn1-miic.h
+F: net/dsa/tag_rzn1_a5psw.c
+
RENESAS R-CAR GEN3 & RZ/N1 NAND CONTROLLER DRIVER
M: Miquel Raynal <[email protected]>
L: [email protected]
--
2.34.1

2022-04-15 21:28:56

[permalink] [raw]

Subject: Re: [PATCH net-next 06/12] net: dsa: rzn1-a5psw: add Renesas RZ/N1 advanced 5 port switch driver

On Fri, Apr 15, 2022 at 12:23:30PM +0100, Russell King (Oracle) wrote:
> If ->shutdown has been called, the system is going down, and userspace
> is probably already dead.

There isn't anything preventing ->remove from being called after ->shutdown
has been called.

It all starts with the pattern that some driver authors prefer, which is
to redirect their ->shutdown method to ->remove. They argue that it
provides for a well-tested common path, so in turn, this pattern is
quite widespread and I'm not one to argue for removing it.

When such driver (redirecting ->shutdown to ->remove) is a bus driver
(SPI, I2C, lately even the fsl-mc bus), the implication is that the
controller will be unregistered on shutdown. To unregister a bus, you
need to unregister all devices on the bus too.

Due to implicit device ordering on the dpm_list, the ->shutdown() method
of children on said bus has already executed, now we're in the context
of the ->shutdown() procedure of the bus driver itself.

You can argue "hey, that's SPI/I2C and this is a platform driver, there
isn't any bus that unregisters on shutdown here", and you may have a
point there. But platform devices aren't just memory-mapped devices,
they can also be children of mfd devices on SPI/I2C buses. So in theory
you can see this pattern happen on platform devices as well.

This is the reason why I insist for uniformity in the DSA layer in the
way that shutdown is handled. People copy and paste code a lot, and by
leaving them with less variance in the code that they copy, subtle
differences that are not understood but do matter are less likely to
creep in.

2022-04-15 23:02:37

[permalink] [raw]

Subject: Re: [PATCH net-next 06/12] net: dsa: rzn1-a5psw: add Renesas RZ/N1 advanced 5 port switch driver

Le Fri, 15 Apr 2022 14:05:24 +0300,
Vladimir Oltean <[email protected]> a écrit :

> On Fri, Apr 15, 2022 at 01:55:03PM +0300, Vladimir Oltean wrote:
> > > > The selftests don't cover nearly enough, but just to make sure that they
> > > > pass for your switch, when you use 2 switch ports as h1 and h2 (hosts),
> > > > and 2 ports as swp1 and swp2? There's surprisingly little that you do on
> > > > .port_bridge_join, I need to study the code more.
> > >
> > > Port isolation is handled by using a pattern matcher which is enabled
> > > for each port at setup. If set, the port packet will only be forwarded
> > > to the CPU port. When bridging is needed, the pattern matching is
> > > disabled and thus, the packets are forwarded between all the ports that
> > > are enabled in the bridge.
> >
> > Is there some public documentation for this pattern matcher?
>
> Again, I realize I haven't made it clear what concerns me here.
> On ->port_bridge_join() and ->port_bridge_leave(), the "bridge" is given
> to you as argument. 2 ports may join br0, and 2 ports may join br1.
> You disregard the "bridge" argument. So you enable forwarding between
> br0 and br1. What I'd like to see is what the hardware can do in terms
> of this "pattern matching", to improve on this situation.

Yes, you are right, the driver currently won't support 2 differents
bridges. Either I add checks to support explicitely only one, or I add
support for multiple bridges. This would probably requires to use VLAN
internally to separate trafic.

--
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

2022-04-16 00:12:55

by Russell King (Oracle)

[permalink] [raw]

Subject: Re: [PATCH net-next 06/12] net: dsa: rzn1-a5psw: add Renesas RZ/N1 advanced 5 port switch driver

On Fri, Apr 15, 2022 at 02:14:19PM +0300, Vladimir Oltean wrote:
> On Fri, Apr 15, 2022 at 12:02:14PM +0100, Russell King (Oracle) wrote:
> > On Fri, Apr 15, 2022 at 01:55:03PM +0300, Vladimir Oltean wrote:
> > > I meant that for a DSA switch driver is mandatory to call dsa_switch_shutdown()
> > > from your ->shutdown method, otherwise subtle things break, sorry for being unclear.
> > >
> > > Please blindly copy-paste the odd pattern that all other DSA drivers use
> > > in ->shutdown and ->remove (with the platform_set_drvdata(dev, NULL) calls),
> > > like a normal person :)
> >
> > Those platform_set_drvdata(, NULL) calls should be killed - the
> > driver model will set the driver data to NULL after ->remove has
> > been called - so having drivers also setting the driver data to
> > NULL is mere duplication.
>
> I can see why you say that, but the reverse is not true.
> A driver can be removed from a device after said device has been shut
> down, and DSA does things in dsa_unregister_switch() and in
> dsa_switch_shutdown() that are incompatible with each other, so either
> one or the other should be called, but not both.

How would ->remove be called after ->shutdown has been called? Aren't
the two calls already exclusive - if ->remove has been called, the
device is no longer bound to the driver, so ->shutdown can't be called
for the device. If ->shutdown has been called, the system is going
down, and userspace is probably already dead.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

2022-04-16 00:13:49

[permalink] [raw]

Subject: Re: [PATCH net-next 06/12] net: dsa: rzn1-a5psw: add Renesas RZ/N1 advanced 5 port switch driver

+static int a5psw_mdio_reset(struct mii_bus *bus)
> +{
> + struct a5psw *a5psw = bus->priv;
> + unsigned long rate;
> + unsigned long div;
> + u32 cfgstatus;
> +
> + rate = clk_get_rate(a5psw->hclk);
> + div = ((rate / a5psw->mdio_freq) / 2);
> + if (div >= 511 || div <= 5) {
> + dev_err(a5psw->dev, "MDIO clock div %ld out of range\n", div);
> + return -ERANGE;
> + }
> +
> + cfgstatus = FIELD_PREP(A5PSW_MDIO_CFG_STATUS_CLKDIV, div);
> +
> + a5psw_reg_writel(a5psw, A5PSW_MDIO_CFG_STATUS, cfgstatus);

I don't see anything here which does an actual reset. So i think this
function has the wrong name. Please also pass the frequency as a
parameter, because at a quick glance it was not easy to see where it
was used. There does not seem to be any need to store it in a5psw.

> +static int a5psw_probe_mdio(struct a5psw *a5psw)
> +{
> + struct device *dev = a5psw->dev;
> + struct device_node *mdio_node;
> + struct mii_bus *bus;
> + int err;
> +
> + if (of_property_read_u32(dev->of_node, "clock-frequency",
> + &a5psw->mdio_freq))
> + a5psw->mdio_freq = A5PSW_MDIO_DEF_FREQ;
> +
> + bus = devm_mdiobus_alloc(dev);
> + if (!bus)
> + return -ENOMEM;
> +
> + bus->name = "a5psw_mdio";
> + bus->read = a5psw_mdio_read;
> + bus->write = a5psw_mdio_write;
> + bus->reset = a5psw_mdio_reset;

As far as i can see, the read and write functions don't support
C45. Please return -EOPNOTSUPP if they are passed C45 addresses.

Andrew

2022-04-16 00:20:06

[permalink] [raw]

Subject: Re: [PATCH net-next 06/12] net: dsa: rzn1-a5psw: add Renesas RZ/N1 advanced 5 port switch driver

On Fri, Apr 15, 2022 at 12:02:14PM +0100, Russell King (Oracle) wrote:
> On Fri, Apr 15, 2022 at 01:55:03PM +0300, Vladimir Oltean wrote:
> > I meant that for a DSA switch driver is mandatory to call dsa_switch_shutdown()
> > from your ->shutdown method, otherwise subtle things break, sorry for being unclear.
> >
> > Please blindly copy-paste the odd pattern that all other DSA drivers use
> > in ->shutdown and ->remove (with the platform_set_drvdata(dev, NULL) calls),
> > like a normal person :)
>
> Those platform_set_drvdata(, NULL) calls should be killed - the
> driver model will set the driver data to NULL after ->remove has
> been called - so having drivers also setting the driver data to
> NULL is mere duplication.

I can see why you say that, but the reverse is not true.
A driver can be removed from a device after said device has been shut
down, and DSA does things in dsa_unregister_switch() and in
dsa_switch_shutdown() that are incompatible with each other, so either
one or the other should be called, but not both.
The platform_set_drvdata(dev, NULL) from the ->remove path may be
redundant for the reason you mentioned, but it doesn't really hurt
anything, really (it's a pointer assignment), and perhaps would lead to
even more confusion (why are we setting the drvdata to NULL from
->shutdown but not also from ->remove?).

> The only case it would matter is if someone is looking up the device
> and then accessing the driver data - and one would hope that's done
> with appropriate locking or other guarantees (e.g. driver can never
> be unbound once the driver data has been set.)

No, this isn't what is happening.

2022-04-16 00:22:17

[permalink] [raw]

Subject: Re: [PATCH net-next 07/12] net: dsa: rzn1-a5psw: add statistics support

Le Fri, 15 Apr 2022 15:37:38 +0200,
Andrew Lunn <[email protected]> a écrit :

> > > > +static void a5psw_get_ethtool_stats(struct dsa_switch *ds, int port,
> > > > + uint64_t *data)
> > > > +{
> > > > + struct a5psw *a5psw = ds->priv;
> > > > + u32 reg_lo, reg_hi;
> > > > + unsigned int u;
> > > > +
> > > > + for (u = 0; u < ARRAY_SIZE(a5psw_stats); u++) {
> > > > + /* A5PSW_STATS_HIWORD is global and thus, access must be
> > > > + * exclusive
> > > > + */
> > >
> > > Could you explain that a bit more. The RTNL lock will prevent two
> > > parallel calls to this function.
> >
> > Ok, I wasn't sure of the locking applicable here.
>
> In general, RTNL protects you for any user space management like
> operation on the driver. In this case, if you look in net/ethtool, you
> will find the IOCTL handler code takes RTNL before calling into the
> main IOCTL dispatcher. If you want to be paranoid/document the
> assumption, you can add an ASSERT_RTNL().

Ok, I'll look at the call stack in details to see what locking is
applied.

>
> The semantics for some of the other statistics Vladimir requested can
> be slightly different. One of them is in atomic context, because a
> spinlock is held. But i don't remember if RTNL is also held. This is
> less of an issue for your switch, since it uses MMIO, however many
> switches need to perform blocking IO over MDIO, SPI, IC2 etc to get
> stats, which you cannot do in atomic context. So they end up returning
> cached values.
>
> Look in the mailing list for past discussion for details.

Ok, thanks,

>
> Andrew

--
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

2022-04-16 00:22:25

[permalink] [raw]

Subject: Re: [PATCH net-next 06/12] net: dsa: rzn1-a5psw: add Renesas RZ/N1 advanced 5 port switch driver

On Thu, Apr 14, 2022 at 02:22:44PM +0200, Cl?ment L?ger wrote:
> Add Renesas RZ/N1 advanced 5 port switch driver. This switch handles 5
> ports including 1 CPU management port. A MDIO bus is also exposed by
> this switch and allows to communicate with PHYs connected to the ports.
> Each switch port (except for the CPU management ports) are connected to

s/are/is/

> the MII converter.
>
> This driver include basic bridging support, more support will be added

s/include/includes/

> later (vlan, etc).
>
> Suggested-by: Laurent Gonzales <[email protected]>
> Suggested-by: Jean-Pierre Geslin <[email protected]>
> Suggested-by: Phil Edworthy <[email protected]>

Suggested? What did they suggest? "You should write a driver"?
We have a Co-developed-by: tag, maybe it's more appropriate here?

> Signed-off-by: Cl?ment L?ger <[email protected]>
> ---
> drivers/net/dsa/Kconfig | 9 +
> drivers/net/dsa/Makefile | 2 +
> drivers/net/dsa/rzn1_a5psw.c | 676 +++++++++++++++++++++++++++++++++++
> drivers/net/dsa/rzn1_a5psw.h | 196 ++++++++++
> 4 files changed, 883 insertions(+)
> create mode 100644 drivers/net/dsa/rzn1_a5psw.c
> create mode 100644 drivers/net/dsa/rzn1_a5psw.h
>
> diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
> index 37a3dabdce31..0896c5fd9dec 100644
> --- a/drivers/net/dsa/Kconfig
> +++ b/drivers/net/dsa/Kconfig
> @@ -70,6 +70,15 @@ config NET_DSA_QCA8K
>
> source "drivers/net/dsa/realtek/Kconfig"
>
> +config NET_DSA_RZN1_A5PSW
> + tristate "Renesas RZ/N1 A5PSW Ethernet switch support"
> + depends on NET_DSA
> + select NET_DSA_TAG_RZN1_A5PSW
> + select PCS_RZN1_MIIC
> + help
> + This driver supports the A5PSW switch, which is embedded in Renesas
> + RZ/N1 SoC.
> +
> config NET_DSA_SMSC_LAN9303
> tristate
> depends on VLAN_8021Q || VLAN_8021Q=n
> diff --git a/drivers/net/dsa/Makefile b/drivers/net/dsa/Makefile
> index e73838c12256..5daf3da4344e 100644
> --- a/drivers/net/dsa/Makefile
> +++ b/drivers/net/dsa/Makefile
> @@ -9,12 +9,14 @@ obj-$(CONFIG_NET_DSA_LANTIQ_GSWIP) += lantiq_gswip.o
> obj-$(CONFIG_NET_DSA_MT7530) += mt7530.o
> obj-$(CONFIG_NET_DSA_MV88E6060) += mv88e6060.o
> obj-$(CONFIG_NET_DSA_QCA8K) += qca8k.o
> +obj-$(CONFIG_NET_DSA_RZN1_A5PSW) += rzn1_a5psw.o
> obj-$(CONFIG_NET_DSA_SMSC_LAN9303) += lan9303-core.o
> obj-$(CONFIG_NET_DSA_SMSC_LAN9303_I2C) += lan9303_i2c.o
> obj-$(CONFIG_NET_DSA_SMSC_LAN9303_MDIO) += lan9303_mdio.o
> obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX) += vitesse-vsc73xx-core.o
> obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX_PLATFORM) += vitesse-vsc73xx-platform.o
> obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX_SPI) += vitesse-vsc73xx-spi.o
> +

Unrelated change.

> obj-y += b53/
> obj-y += hirschmann/
> obj-y += microchip/
> diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c
> new file mode 100644
> index 000000000000..5bee999f7050
> --- /dev/null
> +++ b/drivers/net/dsa/rzn1_a5psw.c
> @@ -0,0 +1,676 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2022 Schneider-Electric
> + *
> + * Cl?ment L?ger <[email protected]>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/etherdevice.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_mdio.h>
> +#include <net/dsa.h>
> +#include <uapi/linux/if_bridge.h>

Why do you need to include this header?

> +
> +#include "rzn1_a5psw.h"
> +
> +static void a5psw_reg_writel(struct a5psw *a5psw, int offset, u32 value)
> +{
> + writel(value, a5psw->base + offset);
> +}
> +
> +static u32 a5psw_reg_readl(struct a5psw *a5psw, int offset)
> +{
> + return readl(a5psw->base + offset);
> +}
> +
> +static void a5psw_reg_rmw(struct a5psw *a5psw, int offset, u32 mask, u32 val)
> +{
> + u32 reg;
> +
> + spin_lock(&a5psw->reg_lock);
> +
> + reg = a5psw_reg_readl(a5psw, offset);
> + reg &= ~mask;
> + reg |= val;
> + a5psw_reg_writel(a5psw, offset, reg);
> +
> + spin_unlock(&a5psw->reg_lock);
> +}
> +
> +static enum dsa_tag_protocol a5psw_get_tag_protocol(struct dsa_switch *ds,
> + int port,
> + enum dsa_tag_protocol mp)
> +{
> + return DSA_TAG_PROTO_RZN1_A5PSW;
> +}
> +
> +static void a5psw_port_pattern_set(struct a5psw *a5psw, int port, int pattern,
> + bool enable)
> +{
> + u32 rx_match = 0;
> +
> + if (enable)
> + rx_match |= A5PSW_RXMATCH_CONFIG_PATTERN(pattern);
> +
> + a5psw_reg_rmw(a5psw, A5PSW_RXMATCH_CONFIG(port),
> + A5PSW_RXMATCH_CONFIG_PATTERN(pattern), rx_match);
> +}
> +
> +static void a5psw_port_mgmtfwd_set(struct a5psw *a5psw, int port, bool enable)

Some explanation on what "management forward" means/does?

> +{
> + a5psw_port_pattern_set(a5psw, port, A5PSW_PATTERN_MGMTFWD, enable);
> +}
> +
> +static void a5psw_port_enable_set(struct a5psw *a5psw, int port, bool enable)
> +{
> + u32 port_ena = 0;
> +
> + if (enable)
> + port_ena |= A5PSW_PORT_ENA_TX_RX(port);
> +
> + a5psw_reg_rmw(a5psw, A5PSW_PORT_ENA, A5PSW_PORT_ENA_TX_RX(port),
> + port_ena);
> +}
> +
> +static int a5psw_lk_execute_ctrl(struct a5psw *a5psw, u32 *ctrl)
> +{
> + int ret;
> +
> + a5psw_reg_writel(a5psw, A5PSW_LK_ADDR_CTRL, *ctrl);
> +
> + ret = readl_poll_timeout(a5psw->base + A5PSW_LK_ADDR_CTRL,
> + *ctrl,
> + !(*ctrl & A5PSW_LK_ADDR_CTRL_BUSY),
> + A5PSW_LK_BUSY_USEC_POLL,
> + A5PSW_CTRL_TIMEOUT);
> + if (ret)
> + dev_err(a5psw->dev, "LK_CTRL timeout waiting for BUSY bit\n");
> +
> + return ret;
> +}
> +
> +static void a5psw_port_fdb_flush(struct a5psw *a5psw, int port)
> +{
> + u32 ctrl = A5PSW_LK_ADDR_CTRL_DELETE_PORT | BIT(port);
> +
> + spin_lock(&a5psw->lk_lock);
> + a5psw_lk_execute_ctrl(a5psw, &ctrl);
> + spin_unlock(&a5psw->lk_lock);
> +}
> +
> +static void a5psw_port_authorize_set(struct a5psw *a5psw, int port,
> + bool authorize)
> +{
> + u32 reg = a5psw_reg_readl(a5psw, A5PSW_AUTH_PORT(port));
> +
> + if (authorize)
> + reg |= A5PSW_AUTH_PORT_AUTHORIZED;
> + else
> + reg &= ~A5PSW_AUTH_PORT_AUTHORIZED;
> +
> + a5psw_reg_writel(a5psw, A5PSW_AUTH_PORT(port), reg);
> +}
> +
> +static void a5psw_port_disable(struct dsa_switch *ds, int port)
> +{
> + struct a5psw *a5psw = ds->priv;
> +
> + a5psw_port_authorize_set(a5psw, port, false);
> + a5psw_port_enable_set(a5psw, port, false);
> + a5psw_port_fdb_flush(a5psw, port);
> +}
> +
> +static int a5psw_port_enable(struct dsa_switch *ds, int port,
> + struct phy_device *phy)
> +{
> + struct a5psw *a5psw = ds->priv;
> +
> + a5psw_port_authorize_set(a5psw, port, true);
> + a5psw_port_enable_set(a5psw, port, true);
> +
> + return 0;
> +}
> +
> +static int a5psw_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
> +{
> + struct a5psw *a5psw = ds->priv;
> +
> + new_mtu += ETH_HLEN + A5PSW_EXTRA_MTU_LEN + ETH_FCS_LEN;
> + a5psw_reg_writel(a5psw, A5PSW_FRM_LENGTH(port), new_mtu);
> +
> + return 0;
> +}
> +
> +static int a5psw_port_max_mtu(struct dsa_switch *ds, int port)
> +{
> + return A5PSW_JUMBO_LEN - A5PSW_TAG_LEN;
> +}

> +static int a5psw_set_ageing_time(struct dsa_switch *ds, unsigned int msecs)
> +{
> + struct a5psw *a5psw = ds->priv;
> + unsigned long rate;
> + u64 max, tmp;
> + u32 agetime;
> +
> + rate = clk_get_rate(a5psw->clk);
> + max = div64_ul(((u64)A5PSW_LK_AGETIME_MASK * A5PSW_TABLE_ENTRIES * 1024),
> + rate) * 1000;
> + if (msecs > max)
> + return -EINVAL;
> +
> + tmp = div_u64(rate, MSEC_PER_SEC);
> + agetime = div_u64(msecs * tmp, 1024 * A5PSW_TABLE_ENTRIES);
> +
> + a5psw_reg_writel(a5psw, A5PSW_LK_AGETIME, agetime);
> +
> + return 0;
> +}
> +
> +static void a5psw_flooding_set_resolution(struct a5psw *a5psw, int port,
> + bool set)
> +{
> + u8 offsets[] = {A5PSW_UCAST_DEF_MASK, A5PSW_BCAST_DEF_MASK,
> + A5PSW_MCAST_DEF_MASK};
> + int i;
> +
> + if (set)
> + a5psw->flooding_ports |= BIT(port);
> + else
> + a5psw->flooding_ports &= ~BIT(port);
> +
> + for (i = 0; i < ARRAY_SIZE(offsets); i++)
> + a5psw_reg_writel(a5psw, offsets[i], a5psw->flooding_ports);
> +}
> +
> +static int a5psw_port_bridge_join(struct dsa_switch *ds, int port,
> + struct dsa_bridge bridge,
> + bool *tx_fwd_offload,
> + struct netlink_ext_ack *extack)
> +{
> + struct a5psw *a5psw = ds->priv;
> +
> + a5psw_flooding_set_resolution(a5psw, port, true);
> + a5psw_port_mgmtfwd_set(a5psw, port, false);
> +
> + return 0;
> +}
> +
> +static void a5psw_port_bridge_leave(struct dsa_switch *ds, int port,
> + struct dsa_bridge bridge)
> +{
> + struct a5psw *a5psw = ds->priv;
> +
> + a5psw_flooding_set_resolution(a5psw, port, false);
> + a5psw_port_mgmtfwd_set(a5psw, port, true);
> +}
> +
> +static void a5psw_port_stp_state_set(struct dsa_switch *ds, int port,
> + u8 state)
> +{
> + struct a5psw *a5psw = ds->priv;
> + u32 reg = 0;
> + u32 mask = A5PSW_INPUT_LEARN_DIS(port) | A5PSW_INPUT_LEARN_BLOCK(port);
> +
> + switch (state) {
> + case BR_STATE_DISABLED:
> + case BR_STATE_BLOCKING:
> + reg |= A5PSW_INPUT_LEARN_DIS(port);
> + reg |= A5PSW_INPUT_LEARN_BLOCK(port);
> + break;
> + case BR_STATE_LISTENING:
> + reg |= A5PSW_INPUT_LEARN_DIS(port);
> + break;
> + case BR_STATE_LEARNING:
> + reg |= A5PSW_INPUT_LEARN_BLOCK(port);
> + break;
> + case BR_STATE_FORWARDING:
> + default:
> + break;
> + }
> +
> + a5psw_reg_rmw(a5psw, A5PSW_INPUT_LEARN, mask, reg);
> +}
> +
> +static void a5psw_port_fast_age(struct dsa_switch *ds, int port)
> +{
> + struct a5psw *a5psw = ds->priv;
> +
> + a5psw_port_fdb_flush(a5psw, port);
> +}
> +
> +static int a5psw_setup(struct dsa_switch *ds)
> +{
> + struct a5psw *a5psw = ds->priv;
> + int port, vlan, ret;
> + u32 reg;
> +
> + /* Configure management port */
> + reg = A5PSW_CPU_PORT | A5PSW_MGMT_CFG_DISCARD;
> + a5psw_reg_writel(a5psw, A5PSW_MGMT_CFG, reg);

Perhaps you should validate the DT blob that the CPU port is the one
that you think it is?

> +
> + /* Set pattern 0 to forward all frame to mgmt port */
> + a5psw_reg_writel(a5psw, A5PSW_PATTERN_CTRL(0),
> + A5PSW_PATTERN_CTRL_MGMTFWD);
> +
> + /* Enable port tagging */
> + reg = FIELD_PREP(A5PSW_MGMT_TAG_CFG_TAGFIELD, A5PSW_MGMT_TAG_VALUE);
> + reg |= A5PSW_MGMT_TAG_CFG_ENABLE | A5PSW_MGMT_TAG_CFG_ALL_FRAMES;
> + a5psw_reg_writel(a5psw, A5PSW_MGMT_TAG_CFG, reg);
> +
> + /* Enable normal switch operation */
> + reg = A5PSW_LK_ADDR_CTRL_BLOCKING | A5PSW_LK_ADDR_CTRL_LEARNING |
> + A5PSW_LK_ADDR_CTRL_AGEING | A5PSW_LK_ADDR_CTRL_ALLOW_MIGR |
> + A5PSW_LK_ADDR_CTRL_CLEAR_TABLE;
> + a5psw_reg_writel(a5psw, A5PSW_LK_CTRL, reg);
> +
> + ret = readl_poll_timeout(a5psw->base + A5PSW_LK_CTRL,
> + reg,
> + !(reg & A5PSW_LK_ADDR_CTRL_CLEAR_TABLE),
> + A5PSW_LK_BUSY_USEC_POLL,
> + A5PSW_CTRL_TIMEOUT);
> + if (ret) {
> + dev_err(a5psw->dev, "Failed to clear lookup table\n");
> + return ret;
> + }
> +
> + /* Reset learn count to 0 */
> + reg = A5PSW_LK_LEARNCOUNT_MODE_SET;
> + a5psw_reg_writel(a5psw, A5PSW_LK_LEARNCOUNT, reg);
> +
> + /* Clear VLAN resource table */
> + reg = A5PSW_VLAN_RES_WR_PORTMASK | A5PSW_VLAN_RES_WR_TAGMASK;
> + for (vlan = 0; vlan < A5PSW_VLAN_COUNT; vlan++)
> + a5psw_reg_writel(a5psw, A5PSW_VLAN_RES(vlan), reg);
> +
> + /* Reset all ports */
> + for (port = 0; port < ds->num_ports; port++) {

Because dsa_is_cpu_port() internally calls dsa_to_port() which iterates
through a list, we tend to avoid the pattern where we call a list
iterating function from a loop over essentially the same data.
Instead, we have:

dsa_switch_for_each_port(dp, ds) {
if (dsa_port_is_unused(dp))
do stuff with dp->index
if (dsa_port_is_cpu(dp))
...
if (dsa_port_is_user(dp))
...
}

> + /* Reset the port */
> + a5psw_reg_writel(a5psw, A5PSW_CMD_CFG(port),
> + A5PSW_CMD_CFG_SW_RESET);
> +
> + /* Enable only CPU port */
> + a5psw_port_enable_set(a5psw, port, dsa_is_cpu_port(ds, port));
> +
> + if (dsa_is_unused_port(ds, port))
> + continue;
> +
> + /* Enable egress flooding for CPU port */
> + if (dsa_is_cpu_port(ds, port))
> + a5psw_flooding_set_resolution(a5psw, port, true);
> +
> + /* Enable management forward only for user ports */
> + if (dsa_is_user_port(ds, port))
> + a5psw_port_mgmtfwd_set(a5psw, port, true);
> + }
> +
> + return 0;
> +}
> +
> +const struct dsa_switch_ops a5psw_switch_ops = {
> + .get_tag_protocol = a5psw_get_tag_protocol,
> + .setup = a5psw_setup,
> + .port_disable = a5psw_port_disable,
> + .port_enable = a5psw_port_enable,
> + .phylink_validate = a5psw_phylink_validate,
> + .phylink_mac_select_pcs = a5psw_phylink_mac_select_pcs,
> + .phylink_mac_link_down = a5psw_phylink_mac_link_down,
> + .phylink_mac_link_up = a5psw_phylink_mac_link_up,
> + .port_change_mtu = a5psw_port_change_mtu,
> + .port_max_mtu = a5psw_port_max_mtu,
> + .set_ageing_time = a5psw_set_ageing_time,
> + .port_bridge_join = a5psw_port_bridge_join,
> + .port_bridge_leave = a5psw_port_bridge_leave,
> + .port_stp_state_set = a5psw_port_stp_state_set,
> + .port_fast_age = a5psw_port_fast_age,
> +

Stray empty line.

> +};
> +
> +static int a5psw_mdio_wait_busy(struct a5psw *a5psw)
> +{
> + u32 status;
> + int err;
> +
> + err = readl_poll_timeout(a5psw->base + A5PSW_MDIO_CFG_STATUS,
> + status,
> + !(status & A5PSW_MDIO_CFG_STATUS_BUSY),
> + 10,
> + 1000 * USEC_PER_MSEC);
> + if (err)
> + dev_err(a5psw->dev, "MDIO command timeout\n");
> +
> + return err;
> +}
> +
> +static int a5psw_mdio_read(struct mii_bus *bus, int phy_id, int phy_reg)
> +{
> + struct a5psw *a5psw = bus->priv;
> + u32 cmd, status;
> + int ret;
> +
> + cmd = A5PSW_MDIO_COMMAND_READ;
> + cmd |= FIELD_PREP(A5PSW_MDIO_COMMAND_REG_ADDR, phy_reg);
> + cmd |= FIELD_PREP(A5PSW_MDIO_COMMAND_PHY_ADDR, phy_id);
> +
> + a5psw_reg_writel(a5psw, A5PSW_MDIO_COMMAND, cmd);
> +
> + ret = a5psw_mdio_wait_busy(a5psw);
> + if (ret)
> + return ret;
> +
> + ret = a5psw_reg_readl(a5psw, A5PSW_MDIO_DATA) & A5PSW_MDIO_DATA_MASK;
> +
> + status = a5psw_reg_readl(a5psw, A5PSW_MDIO_CFG_STATUS);
> + if (status & A5PSW_MDIO_CFG_STATUS_READERR)
> + return -EIO;
> +
> + return ret;
> +}
> +
> +static int a5psw_mdio_write(struct mii_bus *bus, int phy_id, int phy_reg,
> + u16 phy_data)
> +{
> + struct a5psw *a5psw = bus->priv;
> + u32 cmd;
> +
> + cmd = FIELD_PREP(A5PSW_MDIO_COMMAND_REG_ADDR, phy_reg);
> + cmd |= FIELD_PREP(A5PSW_MDIO_COMMAND_PHY_ADDR, phy_id);
> +
> + a5psw_reg_writel(a5psw, A5PSW_MDIO_COMMAND, cmd);
> + a5psw_reg_writel(a5psw, A5PSW_MDIO_DATA, phy_data);
> +
> + return a5psw_mdio_wait_busy(a5psw);
> +}
> +
> +static int a5psw_mdio_reset(struct mii_bus *bus)
> +{
> + struct a5psw *a5psw = bus->priv;
> + unsigned long rate;
> + unsigned long div;
> + u32 cfgstatus;
> +
> + rate = clk_get_rate(a5psw->hclk);
> + div = ((rate / a5psw->mdio_freq) / 2);
> + if (div >= 511 || div <= 5) {
> + dev_err(a5psw->dev, "MDIO clock div %ld out of range\n", div);
> + return -ERANGE;
> + }
> +
> + cfgstatus = FIELD_PREP(A5PSW_MDIO_CFG_STATUS_CLKDIV, div);
> +
> + a5psw_reg_writel(a5psw, A5PSW_MDIO_CFG_STATUS, cfgstatus);
> +
> + return 0;
> +}
> +
> +static int a5psw_probe_mdio(struct a5psw *a5psw)
> +{
> + struct device *dev = a5psw->dev;
> + struct device_node *mdio_node;
> + struct mii_bus *bus;
> + int err;
> +
> + if (of_property_read_u32(dev->of_node, "clock-frequency",
> + &a5psw->mdio_freq))
> + a5psw->mdio_freq = A5PSW_MDIO_DEF_FREQ;

Shouldn't the clock-frequency be a property of the "mdio" node?
At least I see it in Documentation/devicetree/bindings/net/mdio.yaml.

> +
> + bus = devm_mdiobus_alloc(dev);
> + if (!bus)
> + return -ENOMEM;
> +
> + bus->name = "a5psw_mdio";
> + bus->read = a5psw_mdio_read;
> + bus->write = a5psw_mdio_write;
> + bus->reset = a5psw_mdio_reset;
> + bus->priv = a5psw;
> + bus->parent = dev;
> + snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
> +
> + a5psw->mii_bus = bus;
> +
> + mdio_node = of_get_child_by_name(dev->of_node, "mdio");
> + err = devm_of_mdiobus_register(dev, bus, mdio_node);
> + of_node_put(mdio_node);
> + if (err)
> + return err;
> +
> + return 0;
> +}
> +

> +static int a5psw_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct dsa_switch *ds;
> + struct a5psw *a5psw;
> + int ret;
> +
> + a5psw = devm_kzalloc(dev, sizeof(*a5psw), GFP_KERNEL);
> + if (!a5psw)
> + return -ENOMEM;
> +
> + a5psw->dev = dev;
> + spin_lock_init(&a5psw->lk_lock);
> + spin_lock_init(&a5psw->reg_lock);
> + a5psw->base = devm_platform_ioremap_resource(pdev, 0);
> + if (!a5psw->base)
> + return -EINVAL;
> +
> + /* Probe PCS */
> + ret = a5psw_pcs_get(a5psw);
> + if (ret)
> + return ret;
> +
> + a5psw->hclk = devm_clk_get(dev, "hclk");
> + if (IS_ERR(a5psw->hclk)) {
> + dev_err(dev, "failed get hclk clock\n");
> + ret = PTR_ERR(a5psw->hclk);
> + goto free_pcs;
> + }
> +
> + a5psw->clk = devm_clk_get(dev, "clk");
> + if (IS_ERR(a5psw->clk)) {
> + dev_err(dev, "failed get clk_switch clock\n");
> + ret = PTR_ERR(a5psw->clk);
> + goto free_pcs;
> + }
> +
> + ret = clk_prepare_enable(a5psw->clk);
> + if (ret)
> + goto free_pcs;
> +
> + ret = clk_prepare_enable(a5psw->hclk);
> + if (ret)
> + goto clk_disable;
> +
> + ret = a5psw_probe_mdio(a5psw);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to register MDIO: %d\n", ret);
> + goto hclk_disable;
> + }
> +
> + ds = &a5psw->ds;
> + ds->dev = &pdev->dev;
> + ds->num_ports = A5PSW_PORTS_NUM;
> + ds->ops = &a5psw_switch_ops;
> + ds->priv = a5psw;
> +
> + ret = dsa_register_switch(ds);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to register DSA switch: %d\n", ret);
> + goto hclk_disable;
> + }
> +
> + return 0;
> +
> +hclk_disable:
> + clk_disable_unprepare(a5psw->hclk);
> +clk_disable:
> + clk_disable_unprepare(a5psw->clk);
> +free_pcs:
> + a5psw_pcs_free(a5psw);
> +
> + return ret;
> +}
> +
> +static int a5psw_remove(struct platform_device *pdev)
> +{
> + struct a5psw *a5psw = platform_get_drvdata(pdev);
> +
> + dsa_unregister_switch(&a5psw->ds);
> + a5psw_pcs_free(a5psw);
> + clk_disable_unprepare(a5psw->hclk);
> + clk_disable_unprepare(a5psw->clk);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id a5psw_of_mtable[] = {
> + { .compatible = "renesas,rzn1-a5psw", },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, a5psw_of_mtable);
> +
> +static struct platform_driver a5psw_driver = {
> + .driver = {
> + .name = "rzn1_a5psw",
> + .of_match_table = of_match_ptr(a5psw_of_mtable),
> + },
> + .probe = a5psw_probe,
> + .remove = a5psw_remove,

Please implement .shutdown too, it's non-optional.

> +/**
> + * struct a5psw - switch struct
> + * @base: Base address of the switch
> + * @hclk_rate: hclk_switch clock rate
> + * @clk_rate: clk_switch clock rate
> + * @dev: Device associated to the switch
> + * @mii_bus: MDIO bus struct
> + * @mdio_freq: MDIO bus frequency requested
> + * @pcs: Array of PCS connected to the switch ports (not for the CPU)
> + * @ds: DSA switch struct
> + * @lk_lock: Lock for the lookup table
> + * @reg_lock: Lock for register read-modify-write operation

Interesting concept. Generally we see higher-level locking schemes
(i.e. a rmw lock won't really ensure much in terms of consistency of
settings if that's the only thing that serializes concurrent thread
accesses to some register).

Anyway, probably doesn't hurt to have it.

> + * @flooding_ports: List of ports that should be flooded
> + */
> +struct a5psw {
> + void __iomem *base;
> + struct clk* hclk;
> + struct clk* clk;
> + struct device *dev;
> + struct mii_bus *mii_bus;
> + u32 mdio_freq;
> + struct phylink_pcs *pcs[A5PSW_PORTS_NUM - 1];
> + struct dsa_switch ds;
> + spinlock_t lk_lock;
> + spinlock_t reg_lock;
> + u32 flooding_ports;
> +};
> --
> 2.34.1
>

We have some selftests in tools/testing/selftests/net/forwarding/, like
for example bridge_vlan_unaware.sh. They create veth pairs by default,
but if you edit the NETIF_CREATE configuration you should be able to
pass your DSA interfaces.
The selftests don't cover nearly enough, but just to make sure that they
pass for your switch, when you use 2 switch ports as h1 and h2 (hosts),
and 2 ports as swp1 and swp2? There's surprisingly little that you do on
.port_bridge_join, I need to study the code more.

2022-04-16 00:28:50

[permalink] [raw]

Subject: Re: [PATCH net-next 06/12] net: dsa: rzn1-a5psw: add Renesas RZ/N1 advanced 5 port switch driver

Le Thu, 14 Apr 2022 19:55:55 +0200,
Andrew Lunn <[email protected]> a écrit :

> +static int a5psw_mdio_reset(struct mii_bus *bus)
> > +{
> > + struct a5psw *a5psw = bus->priv;
> > + unsigned long rate;
> > + unsigned long div;
> > + u32 cfgstatus;
> > +
> > + rate = clk_get_rate(a5psw->hclk);
> > + div = ((rate / a5psw->mdio_freq) / 2);
> > + if (div >= 511 || div <= 5) {
> > + dev_err(a5psw->dev, "MDIO clock div %ld out of range\n", div);
> > + return -ERANGE;
> > + }
> > +
> > + cfgstatus = FIELD_PREP(A5PSW_MDIO_CFG_STATUS_CLKDIV, div);
> > +
> > + a5psw_reg_writel(a5psw, A5PSW_MDIO_CFG_STATUS, cfgstatus);
>
> I don't see anything here which does an actual reset. So i think this
> function has the wrong name. Please also pass the frequency as a
> parameter, because at a quick glance it was not easy to see where it
> was used. There does not seem to be any need to store it in a5psw.

Indeed, the reset callback can be removed entirely and the mdio bus
could be setup directly from a5psw_probe_mdio().

>
> > +static int a5psw_probe_mdio(struct a5psw *a5psw)
> > +{
> > + struct device *dev = a5psw->dev;
> > + struct device_node *mdio_node;
> > + struct mii_bus *bus;
> > + int err;
> > +
> > + if (of_property_read_u32(dev->of_node, "clock-frequency",
> > + &a5psw->mdio_freq))
> > + a5psw->mdio_freq = A5PSW_MDIO_DEF_FREQ;
> > +
> > + bus = devm_mdiobus_alloc(dev);
> > + if (!bus)
> > + return -ENOMEM;
> > +
> > + bus->name = "a5psw_mdio";
> > + bus->read = a5psw_mdio_read;
> > + bus->write = a5psw_mdio_write;
> > + bus->reset = a5psw_mdio_reset;
>
> As far as i can see, the read and write functions don't support
> C45. Please return -EOPNOTSUPP if they are passed C45 addresses.

Ok.

>
> Andrew

--
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

2022-04-16 00:32:34

[permalink] [raw]

Subject: Re: [PATCH net-next 07/12] net: dsa: rzn1-a5psw: add statistics support

> > > +static void a5psw_get_ethtool_stats(struct dsa_switch *ds, int port,
> > > + uint64_t *data)
> > > +{
> > > + struct a5psw *a5psw = ds->priv;
> > > + u32 reg_lo, reg_hi;
> > > + unsigned int u;
> > > +
> > > + for (u = 0; u < ARRAY_SIZE(a5psw_stats); u++) {
> > > + /* A5PSW_STATS_HIWORD is global and thus, access must be
> > > + * exclusive
> > > + */
> >
> > Could you explain that a bit more. The RTNL lock will prevent two
> > parallel calls to this function.
>
> Ok, I wasn't sure of the locking applicable here.

In general, RTNL protects you for any user space management like
operation on the driver. In this case, if you look in net/ethtool, you
will find the IOCTL handler code takes RTNL before calling into the
main IOCTL dispatcher. If you want to be paranoid/document the
assumption, you can add an ASSERT_RTNL().

The semantics for some of the other statistics Vladimir requested can
be slightly different. One of them is in atomic context, because a
spinlock is held. But i don't remember if RTNL is also held. This is
less of an issue for your switch, since it uses MMIO, however many
switches need to perform blocking IO over MDIO, SPI, IC2 etc to get
stats, which you cannot do in atomic context. So they end up returning
cached values.

Look in the mailing list for past discussion for details.

Andrew

2022-04-16 00:35:32

[permalink] [raw]

Subject: Re: [PATCH net-next 08/12] net: dsa: rzn1-a5psw: add FDB support

On Thu, Apr 14, 2022 at 02:22:46PM +0200, Cl?ment L?ger wrote:
> This commits add forwarding database support to the driver. It
> implements fdb_add(), fdb_del() and fdb_dump().
>
> Signed-off-by: Cl?ment L?ger <[email protected]>
> ---
> drivers/net/dsa/rzn1_a5psw.c | 163 +++++++++++++++++++++++++++++++++++
> drivers/net/dsa/rzn1_a5psw.h | 16 ++++
> 2 files changed, 179 insertions(+)
>
> diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c
> index 7ab7d9054427..8c763c2a1a1f 100644
> --- a/drivers/net/dsa/rzn1_a5psw.c
> +++ b/drivers/net/dsa/rzn1_a5psw.c
> @@ -369,6 +369,166 @@ static void a5psw_port_fast_age(struct dsa_switch *ds, int port)
> a5psw_port_fdb_flush(a5psw, port);
> }
>
> +static int a5psw_lk_execute_lookup(struct a5psw *a5psw, union lk_data *lk_data,
> + u16 *entry)
> +{
> + u32 ctrl;
> + int ret;
> +
> + a5psw_reg_writel(a5psw, A5PSW_LK_DATA_LO, lk_data->lo);
> + a5psw_reg_writel(a5psw, A5PSW_LK_DATA_HI, lk_data->hi);
> +
> + ctrl = A5PSW_LK_ADDR_CTRL_LOOKUP;
> + ret = a5psw_lk_execute_ctrl(a5psw, &ctrl);
> + if (ret)
> + return ret;
> +
> + *entry = ctrl & A5PSW_LK_ADDR_CTRL_ADDRESS;
> +
> + return 0;
> +}
> +
> +static int a5psw_port_fdb_add(struct dsa_switch *ds, int port,
> + const unsigned char *addr, u16 vid,
> + struct dsa_db db)

This isn't something that is documented because I haven't had time to
update that, but new drivers should comply to the requirements for FDB
isolation (not ignore the passed "db" here) and eventually set
ds->fdb_isolation = true. Doing so would allow your switch to behave
correctly when
- there is more than one bridge spanning its ports,
- some ports are standalone and some ports are bridged
- standalone ports are looped back via an external cable with bridged
ports
- unrecognized upper interfaces (bond, team) are used, and those are
bridged directly with some other switch ports

The most basic thing you need to do to satisfy the requirements is to
figure out what mechanism for FDB partitioning does your hardware have.
If the answer is "none", then we'll have to use VLANs for that: all
standalone ports to share a VLAN, each VLAN-unaware bridge to share a
VLAN across all member ports, each VLAN of a VLAN-aware bridge to
reserve its own VLAN. Up to a total of 32 VLANs, since I notice that's
what the limit for your hardware is.

But I see this patch set doesn't include VLAN functionality (and also
ignores the "vid" from FDB entries), so I can't really say more right now.
But if you could provide more information about the hardware
capabilities we can discuss implementation options.

> +{
> + struct a5psw *a5psw = ds->priv;
> + union lk_data lk_data = {0};
> + bool inc_learncount = false;
> + int ret = 0;
> + u16 entry;
> + u32 reg;
> +
> + ether_addr_copy(lk_data.entry.mac, addr);
> + lk_data.entry.port_mask = BIT(port);
> +
> + spin_lock(&a5psw->lk_lock);
> +
> + /* Set the value to be written in the lookup table */
> + ret = a5psw_lk_execute_lookup(a5psw, &lk_data, &entry);
> + if (ret)
> + goto lk_unlock;
> +
> + lk_data.hi = a5psw_reg_readl(a5psw, A5PSW_LK_DATA_HI);
> + if (!lk_data.entry.valid) {
> + inc_learncount = true;
> + /* port_mask set to 0x1f when entry is not valid, clear it */
> + lk_data.entry.port_mask = 0;
> + lk_data.entry.prio = 0;
> + }
> +
> + lk_data.entry.port_mask |= BIT(port);
> + lk_data.entry.is_static = 1;
> + lk_data.entry.valid = 1;
> +
> + a5psw_reg_writel(a5psw, A5PSW_LK_DATA_HI, lk_data.hi);
> +
> + reg = A5PSW_LK_ADDR_CTRL_WRITE | entry;
> + ret = a5psw_lk_execute_ctrl(a5psw, &reg);
> + if (ret)
> + goto lk_unlock;
> +
> + if (inc_learncount) {
> + reg = A5PSW_LK_LEARNCOUNT_MODE_INC;
> + a5psw_reg_writel(a5psw, A5PSW_LK_LEARNCOUNT, reg);
> + }
> +
> +lk_unlock:
> + spin_unlock(&a5psw->lk_lock);
> +
> + return ret;
> +}
> +
> +static int a5psw_port_fdb_del(struct dsa_switch *ds, int port,
> + const unsigned char *addr, u16 vid,
> + struct dsa_db db)
> +{
> + struct a5psw *a5psw = ds->priv;
> + union lk_data lk_data = {0};
> + bool clear = false;
> + int ret = 0;
> + u16 entry;
> + u32 reg;
> +
> + ether_addr_copy(lk_data.entry.mac, addr);
> +
> + spin_lock(&a5psw->lk_lock);
> +
> + ret = a5psw_lk_execute_lookup(a5psw, &lk_data, &entry);
> + if (ret) {
> + dev_err(a5psw->dev, "Failed to lookup mac address\n");
> + goto lk_unlock;
> + }
> +
> + lk_data.hi = a5psw_reg_readl(a5psw, A5PSW_LK_DATA_HI);
> + if (!lk_data.entry.valid) {
> + dev_err(a5psw->dev, "Tried to remove non-existing entry\n");
> + ret = -ENOENT;
> + goto lk_unlock;
> + }
> +
> + lk_data.entry.port_mask &= ~BIT(port);
> + /* If there is no more port in the mask, clear the entry */
> + if (lk_data.entry.port_mask == 0)
> + clear = true;
> +
> + a5psw_reg_writel(a5psw, A5PSW_LK_DATA_HI, lk_data.hi);
> +
> + reg = entry;
> + if (clear)
> + reg |= A5PSW_LK_ADDR_CTRL_CLEAR;
> + else
> + reg |= A5PSW_LK_ADDR_CTRL_WRITE;
> +
> + ret = a5psw_lk_execute_ctrl(a5psw, &reg);
> + if (ret)
> + goto lk_unlock;
> +
> + /* Decrement LEARNCOUNT */
> + if (clear) {
> + reg = A5PSW_LK_LEARNCOUNT_MODE_DEC;
> + a5psw_reg_writel(a5psw, A5PSW_LK_LEARNCOUNT, reg);
> + }
> +
> +lk_unlock:
> + spin_unlock(&a5psw->lk_lock);
> +
> + return ret;
> +}
> +
> +static int a5psw_port_fdb_dump(struct dsa_switch *ds, int port,
> + dsa_fdb_dump_cb_t *cb, void *data)
> +{
> + struct a5psw *a5psw = ds->priv;
> + union lk_data lk_data;
> + int i = 0, ret;
> + u32 reg;
> +
> + for (i = 0; i < A5PSW_TABLE_ENTRIES; i++) {
> + reg = A5PSW_LK_ADDR_CTRL_READ | A5PSW_LK_ADDR_CTRL_WAIT | i;
> + spin_lock(&a5psw->lk_lock);
> +
> + ret = a5psw_lk_execute_ctrl(a5psw, &reg);
> + if (ret)
> + return ret;
> +
> + lk_data.hi = a5psw_reg_readl(a5psw, A5PSW_LK_DATA_HI);
> + /* If entry is not valid or does not contain the port, skip */
> + if (!lk_data.entry.valid ||
> + !(lk_data.entry.port_mask & BIT(port))) {
> + spin_unlock(&a5psw->lk_lock);
> + continue;
> + }
> +
> + lk_data.lo = a5psw_reg_readl(a5psw, A5PSW_LK_DATA_LO);
> + spin_unlock(&a5psw->lk_lock);
> +
> + cb(lk_data.entry.mac, 0, lk_data.entry.is_static, data);

ret = cb(...)
if (ret)
return ret;

This actually matters because the netlink skb used for the FDB dump may
run out of space and you'll have missing FDB entries.

> + }
> +
> + return 0;
> +}
> +
> static void a5psw_get_strings(struct dsa_switch *ds, int port, u32 stringset,
> uint8_t *data)
> {
> @@ -500,6 +660,9 @@ const struct dsa_switch_ops a5psw_switch_ops = {
> .port_bridge_leave = a5psw_port_bridge_leave,
> .port_stp_state_set = a5psw_port_stp_state_set,
> .port_fast_age = a5psw_port_fast_age,
> + .port_fdb_add = a5psw_port_fdb_add,
> + .port_fdb_del = a5psw_port_fdb_del,
> + .port_fdb_dump = a5psw_port_fdb_dump,
>
> };
>
> diff --git a/drivers/net/dsa/rzn1_a5psw.h b/drivers/net/dsa/rzn1_a5psw.h
> index b34ea549e936..37aa89383e70 100644
> --- a/drivers/net/dsa/rzn1_a5psw.h
> +++ b/drivers/net/dsa/rzn1_a5psw.h
> @@ -167,6 +167,22 @@
> #define A5PSW_CTRL_TIMEOUT 1000
> #define A5PSW_TABLE_ENTRIES 8192
>
> +struct fdb_entry {

Shouldn't this contain something along the lines of a VID, FID, something?

> + u8 mac[ETH_ALEN];
> + u8 valid:1;
> + u8 is_static:1;
> + u8 prio:3;
> + u8 port_mask:5;
> +} __packed;
> +
> +union lk_data {
> + struct {
> + u32 lo;
> + u32 hi;
> + };
> + struct fdb_entry entry;
> +};
> +
> /**
> * struct a5psw - switch struct
> * @base: Base address of the switch
> --
> 2.34.1
>

2022-04-16 00:55:53

[permalink] [raw]

Subject: [PATCH net-next 03/12] dt-bindings: net: pcs: add bindings for Renesas RZ/N1 MII converter

This MII converter can be found on the RZ/N1 processor family. The MII
converter ports are declared as subnodes which are then referenced by
users of the PCS driver such as the switch.

Signed-off-by: Clément Léger <[email protected]>
---
.../bindings/net/pcs/renesas,rzn1-miic.yaml | 95 +++++++++++++++++++
include/dt-bindings/net/pcs-rzn1-miic.h | 19 ++++
2 files changed, 114 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/pcs/renesas,rzn1-miic.yaml
create mode 100644 include/dt-bindings/net/pcs-rzn1-miic.h

diff --git a/Documentation/devicetree/bindings/net/pcs/renesas,rzn1-miic.yaml b/Documentation/devicetree/bindings/net/pcs/renesas,rzn1-miic.yaml
new file mode 100644
index 000000000000..ccb25ce6cbde
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/pcs/renesas,rzn1-miic.yaml
@@ -0,0 +1,95 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/pcs/renesas,rzn1-miic.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas RZ/N1 MII converter
+
+maintainers:
+ - Clément Léger <[email protected]>
+
+description: |
+ This MII converter is present on the Renesas RZ/N1 SoC family. It is
+ responsible to do MII passthrough or convert it to RMII/RGMII.
+
+properties:
+ compatible:
+ const: renesas,rzn1-miic
+
+ reg:
+ maxItems: 1
+
+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 0
+
+ clocks:
+ items:
+ - description: MII reference clock
+ - description: RGMII reference clock
+ - description: RMII reference clock
+ - description: AHB clock used for the MII converter register interface
+
+ renesas,miic-cfg-mode:
+ description: MII mux configuration mode. This value should use one of the
+ value defined in dt-bindings/net/pcs-rzn1-miic.h.
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+patternProperties:
+ "^mii-conv@[0-4]$":
+ type: object
+ description: MII converter port
+
+ properties:
+ reg:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - renesas,miic-cfg-mode
+ - "#address-cells"
+ - "#size-cells"
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/net/pcs-rzn1-miic.h>
+ #include <dt-bindings/clock/r9a06g032-sysctrl.h>
+
+ eth-miic@44030000 {
+ compatible = "renesas,rzn1-miic";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x44030000 0x10000>;
+ clocks = <&sysctrl R9A06G032_CLK_MII_REF>,
+ <&sysctrl R9A06G032_CLK_RGMII_REF>,
+ <&sysctrl R9A06G032_CLK_RMII_REF>,
+ <&sysctrl R9A06G032_HCLK_SWITCH_RG>;
+ renesas,miic-cfg-mode = <MIIC_MUX_MAC2_MAC1_SWD_SWC_SWB_SWA>;
+
+ mii_conv0: mii-conv@0 {
+ reg = <0>;
+ };
+
+ mii_conv1: mii-conv@1 {
+ reg = <1>;
+ };
+
+ mii_conv2: mii-conv@2 {
+ reg = <2>;
+ };
+
+ mii_conv3: mii-conv@3 {
+ reg = <3>;
+ };
+
+ mii_conv4: mii-conv@4 {
+ reg = <4>;
+ };
+ };
\ No newline at end of file
diff --git a/include/dt-bindings/net/pcs-rzn1-miic.h b/include/dt-bindings/net/pcs-rzn1-miic.h
new file mode 100644
index 000000000000..c5a0f382967b
--- /dev/null
+++ b/include/dt-bindings/net/pcs-rzn1-miic.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2022 Schneider-Electric
+ *
+ * Clément Léger <[email protected]>
+ */
+
+#ifndef _DT_BINDINGS_PCS_RZN1_MIIC
+#define _DT_BINDINGS_PCS_RZN1_MIIC
+
+/*
+ * Reefer to the datasheet [1] section 8.2.1, Internal Connection of Ethernet
+ * Ports to check the meaning of these values.
+ *
+ * [1] REN_r01uh0750ej0140-rzn1-introduction_MAT_20210228.pdf
+ */
+#define MIIC_MUX_MAC2_MAC1_SWD_SWC_SWB_SWA 0x13
+
+#endif
--
2.34.1

2022-04-16 00:58:26

[permalink] [raw]

Subject: Re: [PATCH net-next 06/12] net: dsa: rzn1-a5psw: add Renesas RZ/N1 advanced 5 port switch driver

On Fri, Apr 15, 2022 at 02:28:57PM +0200, Cl?ment L?ger wrote:
> > Most things as seen by a DSA switch driver are implicitly serialized by
> > the rtnl_mutex anyway.
>
> Is there a list of the functions that are protected by the RTNL lock
> without having to deep dive in the whole stacks ? That would be really
> useful to remove useless locking from my driver. But I guess I'll have
> to look at other drivers to see that.

No, there isn't, but in Documentation/networking/dsa/dsa.rst we do have
a list of dsa_switch_ops functions which used to be comprehensive
(but now needs to be updated again due to development that happened in
the meantime). I suppose that if you do a thorough job of documenting
the synchronization rules, you could add that information to this file.
This would be similar to how we have the "struct net_device synchronization
rules" section in Documentation/networking/netdevices.rst.

2022-04-16 01:03:22

[permalink] [raw]

Subject: Re: [PATCH net-next 06/12] net: dsa: rzn1-a5psw: add Renesas RZ/N1 advanced 5 port switch driver

Le Fri, 15 Apr 2022 13:55:03 +0300,
Vladimir Oltean <[email protected]> a écrit :

> On Fri, Apr 15, 2022 at 11:34:53AM +0200, Clément Léger wrote:
> > Le Thu, 14 Apr 2022 17:47:09 +0300,
> > Vladimir Oltean <[email protected]> a écrit :
> > > > later (vlan, etc).
> > > >
> > > > Suggested-by: Laurent Gonzales <[email protected]>
> > > > Suggested-by: Jean-Pierre Geslin <[email protected]>
> > > > Suggested-by: Phil Edworthy <[email protected]>
> > >
> > > Suggested? What did they suggest? "You should write a driver"?
> > > We have a Co-developed-by: tag, maybe it's more appropriate here?
> >
> > This driver was written from scratch but some ideas (port isolation
> > using pattern matcher) was inspired from a previous driver. I thought it
> > would be nice to give them credit for that.
> >
> > [...]
>
> Ok, in that case I don't really know how to mark sources of inspiration
> in the commit message, maybe your approach is fine.
>
> > > > obj-y += hirschmann/
> > > > obj-y += microchip/
> > > > diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c
> > > > new file mode 100644
> > > > index 000000000000..5bee999f7050
> > > > --- /dev/null
> > > > +++ b/drivers/net/dsa/rzn1_a5psw.c
> > > > @@ -0,0 +1,676 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > +/*
> > > > + * Copyright (C) 2022 Schneider-Electric
> > > > + *
> > > > + * Clément Léger <[email protected]>
> > > > + */
> > > > +
> > > > +#include <linux/clk.h>
> > > > +#include <linux/etherdevice.h>
> > > > +#include <linux/kernel.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/of.h>
> > > > +#include <linux/of_mdio.h>
> > > > +#include <net/dsa.h>
> > > > +#include <uapi/linux/if_bridge.h>
> > >
> > > Why do you need to include this header?
> >
> > It defines BR_STATE_* but I guess linux/if_bridge.h does include it.
>
> Yes.
>
> > > > +static void a5psw_port_pattern_set(struct a5psw *a5psw, int port, int pattern,
> > > > + bool enable)
> > > > +{
> > > > + u32 rx_match = 0;
> > > > +
> > > > + if (enable)
> > > > + rx_match |= A5PSW_RXMATCH_CONFIG_PATTERN(pattern);
> > > > +
> > > > + a5psw_reg_rmw(a5psw, A5PSW_RXMATCH_CONFIG(port),
> > > > + A5PSW_RXMATCH_CONFIG_PATTERN(pattern), rx_match);
> > > > +}
> > > > +
> > > > +static void a5psw_port_mgmtfwd_set(struct a5psw *a5psw, int port, bool enable)
> > >
> > > Some explanation on what "management forward" means/does?
> >
> > I'll probably rename that cpu_port_forward to match the dsa naming.
> > It'll actually isolate the port from other ports by only forwarding the
> > packets to the CPU port.
>
> You could probably do without a rename by just adding a comment that
> says that it enables forwarding only towards the management port.
>
> > > Please implement .shutdown too, it's non-optional.
> >
> > Hum, platform_shutdown does seems to check for the .shutdown callback:
> >
> > static void platform_shutdown(struct device *_dev)
> > {
> > struct platform_device *dev = to_platform_device(_dev);
> > struct platform_driver *drv;
> >
> > if (!_dev->driver)
> > return;
> >
> > drv = to_platform_driver(_dev->driver);
> > if (drv->shutdown)
> > drv->shutdown(dev);
> > }
> >
> > Is there some documentation specifying that this is mandatory ?
> > If so, should I just set it to point to an empty shutdown function then
> > ?
>
> I meant that for a DSA switch driver is mandatory to call dsa_switch_shutdown()
> from your ->shutdown method, otherwise subtle things break, sorry for being unclear.
>
> Please blindly copy-paste the odd pattern that all other DSA drivers use
> in ->shutdown and ->remove (with the platform_set_drvdata(dev, NULL) calls),
> like a normal person :)
>
> > > > + * @reg_lock: Lock for register read-modify-write operation
> > >
> > > Interesting concept. Generally we see higher-level locking schemes
> > > (i.e. a rmw lock won't really ensure much in terms of consistency of
> > > settings if that's the only thing that serializes concurrent thread
> > > accesses to some register).
> >
> > Agreed, this does not guarantee consistency of settings but guarantees
> > that rmw modifications are atomic between devices. I wasn't sure about
> > the locking guarantee that I could have. After looking at other
> > drivers, I guess I will switch to something more common such as using
> > a global mutex for register accesses.
>
> LOL, that isn't better...
>
> Ideally locking would be done per functionality that the hardware can
> perform independently (like lookup table access, VLAN table access,
> forwarding domain control, PTP block, link state control, etc).
> You don't want to artificially serialize unrelated stuff.
> A "read-modify-write" lock would similarly artificially serialize
> unrelated stuff for you, even if you intend it to only serialize
> something entirely different.
>
> Most things as seen by a DSA switch driver are implicitly serialized by
> the rtnl_mutex anyway.

Is there a list of the functions that are protected by the RTNL lock
without having to deep dive in the whole stacks ? That would be really
useful to remove useless locking from my driver. But I guess I'll have
to look at other drivers to see that.

> Some things aren't (->port_fdb_add, ->port_fdb_del).

Ok, looks like for them a mutex is often used which also seems more
appropriate in my case since the operation on the learn table can take
some times.

> There is a point to be made about adding locks for stuff that is
> implicitly serialized by the rtnl_mutex, since you can't really test
> their effectiveness. This makes it more difficult for the driver writer
> to make the right decision about locking, since in some cases, the
> serialization given by the rtnl_mutex isn't something fundamental and
> may be removed, to reduce contention on that lock. In that case, it is
> always a nice surprise to find a backup locking scheme in converted
> drivers. With the mention that said backup locking scheme was never
> really tested, so it may be that it needs further work anyway.

Ok understood.

>
> > > The selftests don't cover nearly enough, but just to make sure that they
> > > pass for your switch, when you use 2 switch ports as h1 and h2 (hosts),
> > > and 2 ports as swp1 and swp2? There's surprisingly little that you do on
> > > .port_bridge_join, I need to study the code more.
> >
> > Port isolation is handled by using a pattern matcher which is enabled
> > for each port at setup. If set, the port packet will only be forwarded
> > to the CPU port. When bridging is needed, the pattern matching is
> > disabled and thus, the packets are forwarded between all the ports that
> > are enabled in the bridge.
>
> Is there some public documentation for this pattern matcher?

Yes, the manual is public [1]. See the Advanced 5 Ports Switch section.

[1]
https://www.renesas.com/us/en/document/mah/rzn1d-group-rzn1s-group-rzn1l-group-users-manual-r-engine-and-ethernet-peripherals

--
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

2022-04-16 01:10:43

[permalink] [raw]

Subject: [PATCH net-next 10/12] ARM: dts: r9a06g032: describe GMAC2

RZ/N1 SoC includes two MAC named GMACx that are compatible with the
"snps,dwmac" driver. GMAC1 is connected directly to the MII converter
port 1. GMAC2 however can be used as the MAC for the switch CPU
management port or can be muxed to be connected directly to the MII
converter port 2. This commit add description for the GMAC2 which will
be used by the switch description.

Signed-off-by: Clément Léger <[email protected]>
---
arch/arm/boot/dts/r9a06g032.dtsi | 17 +++++++++++++++++
1 file changed, 17 insertions(+)

diff --git a/arch/arm/boot/dts/r9a06g032.dtsi b/arch/arm/boot/dts/r9a06g032.dtsi
index fd174df268e8..9be55957b8e5 100644
--- a/arch/arm/boot/dts/r9a06g032.dtsi
+++ b/arch/arm/boot/dts/r9a06g032.dtsi
@@ -200,6 +200,23 @@ nand_controller: nand-controller@40102000 {
status = "disabled";
};

+ gmac2: ethernet@44002000 {
+ compatible = "snps,dwmac-3.72a", "snps,dwmac";
+ reg = <0x44002000 0x2000>;
+ interrupt-parent = <&gic>;
+ interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "macirq", "eth_lpi", "eth_wake_irq";
+ clock-names = "stmmaceth";
+ clocks = <&sysctrl R9A06G032_HCLK_GMAC1>;
+ snps,multicast-filter-bins = <256>;
+ snps,perfect-filter-entries = <128>;
+ tx-fifo-depth = <2048>;
+ rx-fifo-depth = <4096>;
+ status = "disabled";
+ };
+
eth_miic: eth-miic@44030000 {
compatible = "renesas,rzn1-miic";
#address-cells = <1>;
--
2.34.1

2022-04-16 01:24:57

[permalink] [raw]

Subject: [PATCH net-next 01/12] net: dsa: add support for Renesas RZ/N1 A5PSW switch tag code

Add DSA tag code for Renesas RZ/N1 Advanced 5 port switch. This switch
uses a special VLAN type followed by 6 bytes which contains other
useful information (port, timestamp, etc).

Signed-off-by: Clément Léger <[email protected]>
---
include/net/dsa.h | 2 ++
1 file changed, 2 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 934958fda962..2aa8eaae4eb9 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -53,6 +53,7 @@ struct phylink_link_state;
#define DSA_TAG_PROTO_SJA1110_VALUE 23
#define DSA_TAG_PROTO_RTL8_4_VALUE 24
#define DSA_TAG_PROTO_RTL8_4T_VALUE 25
+#define DSA_TAG_PROTO_RZN1_A5PSW_VALUE 26

enum dsa_tag_protocol {
DSA_TAG_PROTO_NONE = DSA_TAG_PROTO_NONE_VALUE,
@@ -81,6 +82,7 @@ enum dsa_tag_protocol {
DSA_TAG_PROTO_SJA1110 = DSA_TAG_PROTO_SJA1110_VALUE,
DSA_TAG_PROTO_RTL8_4 = DSA_TAG_PROTO_RTL8_4_VALUE,
DSA_TAG_PROTO_RTL8_4T = DSA_TAG_PROTO_RTL8_4T_VALUE,
+ DSA_TAG_PROTO_RZN1_A5PSW = DSA_TAG_PROTO_RZN1_A5PSW_VALUE,
};

struct dsa_switch;
--
2.34.1

2022-04-16 01:37:23

[permalink] [raw]

Subject: Re: [PATCH net-next 06/12] net: dsa: rzn1-a5psw: add Renesas RZ/N1 advanced 5 port switch driver

Le Thu, 14 Apr 2022 17:47:09 +0300,
Vladimir Oltean <[email protected]> a écrit :

>
> > later (vlan, etc).
> >
> > Suggested-by: Laurent Gonzales <[email protected]>
> > Suggested-by: Jean-Pierre Geslin <[email protected]>
> > Suggested-by: Phil Edworthy <[email protected]>
>
> Suggested? What did they suggest? "You should write a driver"?
> We have a Co-developed-by: tag, maybe it's more appropriate here?

This driver was written from scratch but some ideas (port isolation
using pattern matcher) was inspired from a previous driver. I thought it
would be nice to give them credit for that.

[...]

> > obj-y += hirschmann/
> > obj-y += microchip/
> > diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c
> > new file mode 100644
> > index 000000000000..5bee999f7050
> > --- /dev/null
> > +++ b/drivers/net/dsa/rzn1_a5psw.c
> > @@ -0,0 +1,676 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2022 Schneider-Electric
> > + *
> > + * Clément Léger <[email protected]>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/etherdevice.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_mdio.h>
> > +#include <net/dsa.h>
> > +#include <uapi/linux/if_bridge.h>
>
> Why do you need to include this header?

It defines BR_STATE_* but I guess linux/if_bridge.h does include it.

> > +
> > +static void a5psw_port_pattern_set(struct a5psw *a5psw, int port, int pattern,
> > + bool enable)
> > +{
> > + u32 rx_match = 0;
> > +
> > + if (enable)
> > + rx_match |= A5PSW_RXMATCH_CONFIG_PATTERN(pattern);
> > +
> > + a5psw_reg_rmw(a5psw, A5PSW_RXMATCH_CONFIG(port),
> > + A5PSW_RXMATCH_CONFIG_PATTERN(pattern), rx_match);
> > +}
> > +
> > +static void a5psw_port_mgmtfwd_set(struct a5psw *a5psw, int port, bool enable)
>
> Some explanation on what "management forward" means/does?

I'll probably rename that cpu_port_forward to match the dsa naming.
It'll actually isolate the port from other ports by only forwarding the
packets to the CPU port.

> > +
> > +static int a5psw_setup(struct dsa_switch *ds)
> > +{
> > + struct a5psw *a5psw = ds->priv;
> > + int port, vlan, ret;
> > + u32 reg;
> > +
> > + /* Configure management port */
> > + reg = A5PSW_CPU_PORT | A5PSW_MGMT_CFG_DISCARD;
> > + a5psw_reg_writel(a5psw, A5PSW_MGMT_CFG, reg);
>
> Perhaps you should validate the DT blob that the CPU port is the one
> that you think it is?

You are right, the datasheet says that the management port should
actually always be the CPU port so I guess a check would be nice.

> > +
> > + /* Reset learn count to 0 */
> > + reg = A5PSW_LK_LEARNCOUNT_MODE_SET;
> > + a5psw_reg_writel(a5psw, A5PSW_LK_LEARNCOUNT, reg);
> > +
> > + /* Clear VLAN resource table */
> > + reg = A5PSW_VLAN_RES_WR_PORTMASK | A5PSW_VLAN_RES_WR_TAGMASK;
> > + for (vlan = 0; vlan < A5PSW_VLAN_COUNT; vlan++)
> > + a5psw_reg_writel(a5psw, A5PSW_VLAN_RES(vlan), reg);
> > +
> > + /* Reset all ports */
> > + for (port = 0; port < ds->num_ports; port++) {
>
> Because dsa_is_cpu_port() internally calls dsa_to_port() which iterates
> through a list, we tend to avoid the pattern where we call a list
> iterating function from a loop over essentially the same data.
> Instead, we have:
>
> dsa_switch_for_each_port(dp, ds) {
> if (dsa_port_is_unused(dp))
> do stuff with dp->index
> if (dsa_port_is_cpu(dp))
> ...
> if (dsa_port_is_user(dp))
> ...
> }

Nice catch indeed, I'll convert that.

> > +
> > +static int a5psw_probe_mdio(struct a5psw *a5psw)
> > +{
> > + struct device *dev = a5psw->dev;
> > + struct device_node *mdio_node;
> > + struct mii_bus *bus;
> > + int err;
> > +
> > + if (of_property_read_u32(dev->of_node, "clock-frequency",
> > + &a5psw->mdio_freq))
> > + a5psw->mdio_freq = A5PSW_MDIO_DEF_FREQ;
>
> Shouldn't the clock-frequency be a property of the "mdio" node?
> At least I see it in Documentation/devicetree/bindings/net/mdio.yaml.

Yes, totally.

> > +static const struct of_device_id a5psw_of_mtable[] = {
> > + { .compatible = "renesas,rzn1-a5psw", },
> > + { /* sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, a5psw_of_mtable);
> > +
> > +static struct platform_driver a5psw_driver = {
> > + .driver = {
> > + .name = "rzn1_a5psw",
> > + .of_match_table = of_match_ptr(a5psw_of_mtable),
> > + },
> > + .probe = a5psw_probe,
> > + .remove = a5psw_remove,
>
> Please implement .shutdown too, it's non-optional.

Hum, platform_shutdown does seems to check for the .shutdown callback:

static void platform_shutdown(struct device *_dev)
{
struct platform_device *dev = to_platform_device(_dev);
struct platform_driver *drv;

if (!_dev->driver)
return;

drv = to_platform_driver(_dev->driver);
if (drv->shutdown)
drv->shutdown(dev);
}

Is there some documentation specifying that this is mandatory ?
If so, should I just set it to point to an empty shutdown function then
?

>
> > +/**
> > + * struct a5psw - switch struct
> > + * @base: Base address of the switch
> > + * @hclk_rate: hclk_switch clock rate
> > + * @clk_rate: clk_switch clock rate
> > + * @dev: Device associated to the switch
> > + * @mii_bus: MDIO bus struct
> > + * @mdio_freq: MDIO bus frequency requested
> > + * @pcs: Array of PCS connected to the switch ports (not for the CPU)
> > + * @ds: DSA switch struct
> > + * @lk_lock: Lock for the lookup table
> > + * @reg_lock: Lock for register read-modify-write operation
>
> Interesting concept. Generally we see higher-level locking schemes
> (i.e. a rmw lock won't really ensure much in terms of consistency of
> settings if that's the only thing that serializes concurrent thread
> accesses to some register).

Agreed, this does not guarantee consistency of settings but guarantees
that rmw modifications are atomic between devices. I wasn't sure about
the locking guarantee that I could have. After looking at other
drivers, I guess I will switch to something more common such as using
a global mutex for register accesses.

>
> Anyway, probably doesn't hurt to have it.
>
> > + * @flooding_ports: List of ports that should be flooded
> > + */
> > +struct a5psw {
> > + void __iomem *base;
> > + struct clk* hclk;
> > + struct clk* clk;
> > + struct device *dev;
> > + struct mii_bus *mii_bus;
> > + u32 mdio_freq;
> > + struct phylink_pcs *pcs[A5PSW_PORTS_NUM - 1];
> > + struct dsa_switch ds;
> > + spinlock_t lk_lock;
> > + spinlock_t reg_lock;
> > + u32 flooding_ports;
> > +};
> > --
> > 2.34.1
> >
>
> We have some selftests in tools/testing/selftests/net/forwarding/, like
> for example bridge_vlan_unaware.sh. They create veth pairs by default,
> but if you edit the NETIF_CREATE configuration you should be able to
> pass your DSA interfaces.

Ok, great to know that there are some tests that can be used.

> The selftests don't cover nearly enough, but just to make sure that they
> pass for your switch, when you use 2 switch ports as h1 and h2 (hosts),
> and 2 ports as swp1 and swp2? There's surprisingly little that you do on
> .port_bridge_join, I need to study the code more.

Port isolation is handled by using a pattern matcher which is enabled
for each port at setup. If set, the port packet will only be forwarded
to the CPU port. When bridging is needed, the pattern matching is
disabled and thus, the packets are forwarded between all the ports that
are enabled in the bridge.

Thanks,

--
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

2022-04-16 01:38:20

[permalink] [raw]

Subject: Re: [PATCH net-next 06/12] net: dsa: rzn1-a5psw: add Renesas RZ/N1 advanced 5 port switch driver

> > +static int a5psw_probe_mdio(struct a5psw *a5psw)
> > +{
> > + struct device *dev = a5psw->dev;
> > + struct device_node *mdio_node;
> > + struct mii_bus *bus;
> > + int err;
> > +
> > + if (of_property_read_u32(dev->of_node, "clock-frequency",
> > + &a5psw->mdio_freq))
> > + a5psw->mdio_freq = A5PSW_MDIO_DEF_FREQ;
>
> Shouldn't the clock-frequency be a property of the "mdio" node?
> At least I see it in Documentation/devicetree/bindings/net/mdio.yaml.

Yes. And the example in the binding document for this driver also
places it in the mdio node.

Andrew

2022-04-16 01:58:11

[permalink] [raw]

Subject: [PATCH net-next 04/12] net: pcs: add Renesas MII converter driver

Add PCS driver for the MII converter that is present on Renesas RZ/N1
SoC. This MII converter is reponsible of converting MII to RMII/RGMII
or act as a MII passtrough. Exposing it as a PCS allows to reuse it
in both the switch driver and the stmmac driver. Currently, this driver
only allows the PCS to be used by the dual Cortex-A7 subsystem since
the register locking system is not used.

Signed-off-by: Clément Léger <[email protected]>
---
drivers/net/pcs/Kconfig | 7 +
drivers/net/pcs/Makefile | 1 +
drivers/net/pcs/pcs-rzn1-miic.c | 350 ++++++++++++++++++++++++++++++++
include/linux/pcs-rzn1-miic.h | 18 ++
4 files changed, 376 insertions(+)
create mode 100644 drivers/net/pcs/pcs-rzn1-miic.c
create mode 100644 include/linux/pcs-rzn1-miic.h

diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
index 22ba7b0b476d..fb0d36d46ffb 100644
--- a/drivers/net/pcs/Kconfig
+++ b/drivers/net/pcs/Kconfig
@@ -18,4 +18,11 @@ config PCS_LYNX
This module provides helpers to phylink for managing the Lynx PCS
which is part of the Layerscape and QorIQ Ethernet SERDES.

+config PCS_RZN1_MIIC
+ tristate "Renesas RZN1 MII converter"
+ help
+ This module provides a driver for the MII converter that is available
+ on RZN1 SoC. This PCS convert MII to RMII/RGMII or can be in
+ passthrough mode for MII.
+
endmenu
diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
index 0603d469bd57..0ff5388fcdea 100644
--- a/drivers/net/pcs/Makefile
+++ b/drivers/net/pcs/Makefile
@@ -5,3 +5,4 @@ pcs_xpcs-$(CONFIG_PCS_XPCS) := pcs-xpcs.o pcs-xpcs-nxp.o

obj-$(CONFIG_PCS_XPCS) += pcs_xpcs.o
obj-$(CONFIG_PCS_LYNX) += pcs-lynx.o
+obj-$(CONFIG_PCS_RZN1_MIIC) += pcs-rzn1-miic.o
diff --git a/drivers/net/pcs/pcs-rzn1-miic.c b/drivers/net/pcs/pcs-rzn1-miic.c
new file mode 100644
index 000000000000..84599e767689
--- /dev/null
+++ b/drivers/net/pcs/pcs-rzn1-miic.c
@@ -0,0 +1,350 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2022 Schneider Electric
+ *
+ * Clément Léger <[email protected]>
+ */
+
+#include <linux/clk.h>
+#include <linux/mdio.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/phylink.h>
+#include <linux/pcs-rzn1-miic.h>
+
+#define MIIC_PRCMD 0x0
+#define MIIC_ESID_CODE 0x4
+
+#define MIIC_MODCTRL 0x20
+#define MIIC_MODCTRL_SW_MODE GENMASK(4, 0)
+
+#define MIIC_CONVCTRL(port) (0x100 + (port) * 4)
+#define MIIC_CONVCTRL_CONV_MODE GENMASK(4, 0)
+#define CONV_MODE_MII 0
+#define CONV_MODE_RMII BIT(2)
+#define CONV_MODE_RGMII BIT(3)
+#define CONV_MODE_10MBPS 0
+#define CONV_MODE_100MBPS BIT(0)
+#define CONV_MODE_1000MBPS BIT(1)
+#define MIIC_CONVCTRL_FULLD BIT(8)
+#define MIIC_CONVCTRL_RGMII_LINK BIT(12)
+#define MIIC_CONVCTRL_RGMII_DUPLEX BIT(13)
+#define MIIC_CONVCTRL_RGMII_SPEED GENMASK(15, 14)
+
+#define MIIC_CONVRST 0x114
+#define MIIC_CONVRST_PHYIF_RST(port) BIT(port)
+#define MIIC_CONVRST_PHYIF_RST_MASK GENMASK(4, 0)
+
+#define MIIC_SWCTRL 0x304
+#define MIIC_SWDUPC 0x308
+
+#define MIIC_MAX_NR_PORTS 5
+
+#define phylink_pcs_to_miic_port(pcs) container_of((pcs), struct miic_port, pcs)
+
+/**
+ * struct miic - MII converter structure
+ * @base: base address of the MII converter
+ * @dev: Device associated to the MII converter
+ * @lock: Lock used for read-modify-write access
+ */
+struct miic {
+ void __iomem *base;
+ struct device *dev;
+ spinlock_t lock;
+};
+
+/**
+ * struct miic_port - Per port MII converter struct
+ * @miic: backiling to MII converter structure
+ * @pcs: PCS structure associated to the port
+ * @port: port number
+ */
+struct miic_port {
+ struct miic *miic;
+ struct phylink_pcs pcs;
+ int port;
+};
+
+static void miic_reg_writel(struct miic *miic, int offset, u32 value)
+{
+ writel(value, miic->base + offset);
+}
+
+static u32 miic_reg_readl(struct miic *miic, int offset)
+{
+ return readl(miic->base + offset);
+}
+
+static void miic_reg_rmw(struct miic *miic, int offset, u32 mask, u32 val)
+{
+ u32 reg;
+
+ spin_lock(&miic->lock);
+
+ reg = miic_reg_readl(miic, offset);
+ reg &= ~mask;
+ reg |= val;
+ miic_reg_writel(miic, offset, reg);
+
+ spin_unlock(&miic->lock);
+}
+
+static void miic_converter_enable(struct miic *miic, int port, int enable)
+{
+ u32 val = 0;
+
+ if (enable)
+ val = MIIC_CONVRST_PHYIF_RST(port);
+
+ miic_reg_rmw(miic, MIIC_CONVRST, MIIC_CONVRST_PHYIF_RST(port), val);
+}
+
+static int miic_config(struct phylink_pcs *pcs, unsigned int mode,
+ phy_interface_t interface,
+ const unsigned long *advertising, bool permit)
+{
+ struct miic_port *miic_port = phylink_pcs_to_miic_port(pcs);
+ struct miic *miic = miic_port->miic;
+ int port = miic_port->port;
+ u32 val;
+
+ switch (interface) {
+ case PHY_INTERFACE_MODE_RMII:
+ val = CONV_MODE_RMII | CONV_MODE_1000MBPS;
+ break;
+ case PHY_INTERFACE_MODE_RGMII:
+ val = CONV_MODE_RGMII | CONV_MODE_1000MBPS;
+ break;
+ case PHY_INTERFACE_MODE_MII:
+ val = CONV_MODE_MII;
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ miic_reg_rmw(miic, MIIC_CONVCTRL(port), MIIC_CONVCTRL_CONV_MODE, val);
+ miic_converter_enable(miic_port->miic, miic_port->port, 1);
+
+ return 0;
+}
+
+static void miic_link_up(struct phylink_pcs *pcs, unsigned int mode,
+ phy_interface_t interface, int speed, int duplex)
+{
+ struct miic_port *miic_port = phylink_pcs_to_miic_port(pcs);
+ struct miic *miic = miic_port->miic;
+ int port = miic_port->port;
+ u32 val = 0;
+
+ if (duplex == DUPLEX_FULL)
+ val |= MIIC_CONVCTRL_FULLD;
+
+ switch (interface) {
+ case PHY_INTERFACE_MODE_RMII:
+ val |= CONV_MODE_RMII;
+ break;
+ case PHY_INTERFACE_MODE_RGMII:
+ val |= CONV_MODE_RGMII;
+ break;
+ case PHY_INTERFACE_MODE_MII:
+ val |= CONV_MODE_MII;
+ break;
+ default:
+ dev_err(miic->dev, "Unsupported interface %s\n",
+ phy_modes(interface));
+ return;
+ }
+
+ /* No speed in MII through-mode */
+ if (interface != PHY_INTERFACE_MODE_MII) {
+ switch (speed) {
+ case SPEED_1000:
+ val |= CONV_MODE_1000MBPS;
+ break;
+ case SPEED_100:
+ val |= CONV_MODE_100MBPS;
+ break;
+ case SPEED_10:
+ val |= CONV_MODE_10MBPS;
+ break;
+ case SPEED_UNKNOWN:
+ pr_err("Invalid speed\n");
+ /* Silently don't do anything */
+ return;
+ default:
+ dev_err(miic->dev, "Invalid PCS speed %d\n", speed);
+ return;
+ }
+ }
+
+ miic_reg_rmw(miic, MIIC_CONVCTRL(port),
+ (MIIC_CONVCTRL_CONV_MODE | MIIC_CONVCTRL_FULLD), val);
+}
+
+static bool miic_mode_supported(phy_interface_t interface)
+{
+ return (interface == PHY_INTERFACE_MODE_RGMII ||
+ interface == PHY_INTERFACE_MODE_RMII ||
+ interface == PHY_INTERFACE_MODE_MII);
+}
+
+static int miic_validate(struct phylink_pcs *pcs, unsigned long *supported,
+ const struct phylink_link_state *state)
+{
+ struct miic_port *miic_port = phylink_pcs_to_miic_port(pcs);
+ struct miic *miic = miic_port->miic;
+
+ if (state->interface != PHY_INTERFACE_MODE_NA &&
+ !miic_mode_supported(state->interface)) {
+ dev_err(miic->dev, "phy mode %s is unsupported on port %d\n",
+ phy_modes(state->interface), miic_port->port);
+ linkmode_zero(supported);
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
+static const struct phylink_pcs_ops miic_phylink_ops = {
+ .pcs_config = miic_config,
+ .pcs_link_up = miic_link_up,
+ .pcs_validate = miic_validate,
+};
+
+struct phylink_pcs *miic_create(struct device_node *np)
+{
+ struct platform_device *pdev;
+ struct miic_port *miic_port;
+ struct device_node *pcs_np;
+ u32 port;
+
+ if (of_property_read_u32(np, "reg", &port))
+ return ERR_PTR(-EINVAL);
+
+ if (port >= MIIC_MAX_NR_PORTS)
+ return ERR_PTR(-EINVAL);
+
+ /* The PCS pdev is attached to the parent node */
+ pcs_np = of_get_parent(np);
+ if (!pcs_np)
+ return ERR_PTR(-EINVAL);
+
+ pdev = of_find_device_by_node(pcs_np);
+ if (!pdev || !platform_get_drvdata(pdev))
+ return ERR_PTR(-EPROBE_DEFER);
+
+ miic_port = kzalloc(sizeof(*miic_port), GFP_KERNEL);
+ if (!miic_port)
+ return ERR_PTR(-ENOMEM);
+
+ miic_port->miic = platform_get_drvdata(pdev);
+ miic_port->port = port;
+ miic_port->pcs.ops = &miic_phylink_ops;
+
+ return &miic_port->pcs;
+}
+EXPORT_SYMBOL(miic_create);
+
+void miic_destroy(struct phylink_pcs *pcs)
+{
+ struct miic_port *miic_port = phylink_pcs_to_miic_port(pcs);
+
+ miic_converter_enable(miic_port->miic, miic_port->port, 0);
+ kfree(miic_port);
+}
+EXPORT_SYMBOL(miic_destroy);
+
+static int miic_init_hw(struct miic *miic, u32 mode)
+{
+ int port;
+
+ /* Unlock write access to accessory registers (cf datasheet). If this
+ * is going to be used in conjunction with the Cortex-M3, this sequence
+ * will have to be moved in register write
+ */
+ miic_reg_writel(miic, MIIC_PRCMD, 0x00A5);
+ miic_reg_writel(miic, MIIC_PRCMD, 0x0001);
+ miic_reg_writel(miic, MIIC_PRCMD, 0xFFFE);
+ miic_reg_writel(miic, MIIC_PRCMD, 0x0001);
+
+ miic_reg_writel(miic, MIIC_MODCTRL,
+ FIELD_PREP(MIIC_MODCTRL_SW_MODE, mode));
+
+ for (port = 0; port < MIIC_MAX_NR_PORTS; port++) {
+ miic_converter_enable(miic, port, 0);
+ /* Disable speed/duplex control from these registers, datasheet
+ * says switch registers should be used to setup switch port
+ * speed and duplex.
+ */
+ miic_reg_writel(miic, MIIC_SWCTRL, 0x0);
+ miic_reg_writel(miic, MIIC_SWDUPC, 0x0);
+ }
+
+ return 0;
+}
+
+static int miic_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct clk_bulk_data *clks;
+ struct miic *miic;
+ u32 mode_cfg;
+ int nclk;
+ int ret;
+
+ if (of_property_read_u32(dev->of_node, "renesas,miic-cfg-mode", &mode_cfg)) {
+ dev_err(dev, "Missing renesas,miic-cfg-mode property for miic\n");
+ return -EINVAL;
+ }
+
+ miic = devm_kzalloc(dev, sizeof(*miic), GFP_KERNEL);
+ if (!miic)
+ return -ENOMEM;
+
+ spin_lock_init(&miic->lock);
+ miic->dev = dev;
+ miic->base = devm_platform_ioremap_resource(pdev, 0);
+ if (!miic->base)
+ return -EINVAL;
+
+ nclk = devm_clk_bulk_get_all(dev, &clks);
+ if (nclk < 0)
+ return nclk;
+
+ ret = clk_bulk_prepare_enable(nclk, clks);
+ if (ret)
+ return ret;
+
+ ret = miic_init_hw(miic, mode_cfg);
+ if (ret)
+ goto disable_clocks;
+
+ platform_set_drvdata(pdev, miic);
+
+ return 0;
+
+disable_clocks:
+ clk_bulk_disable_unprepare(nclk, clks);
+
+ return ret;
+}
+
+static const struct of_device_id miic_of_mtable[] = {
+ { .compatible = "renesas,rzn1-miic" },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, miic_of_mtable);
+
+static struct platform_driver miic_driver = {
+ .driver = {
+ .name = "mtip_miic",
+ .of_match_table = of_match_ptr(miic_of_mtable),
+ },
+ .probe = miic_probe,
+};
+module_platform_driver(miic_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Renesas MII converter PCS driver");
+MODULE_AUTHOR("Clément Léger <[email protected]>");
diff --git a/include/linux/pcs-rzn1-miic.h b/include/linux/pcs-rzn1-miic.h
new file mode 100644
index 000000000000..7a303af57e40
--- /dev/null
+++ b/include/linux/pcs-rzn1-miic.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2022 Schneider Electric
+ *
+ * Clément Léger <[email protected]>
+ */
+
+#ifndef __LINUX_PCS_MIIC_H
+#define __LINUX_PCS_MIIC_H
+
+struct phylink;
+struct device_node;
+
+struct phylink_pcs *miic_create(struct device_node *np);
+
+void miic_destroy(struct phylink_pcs *pcs);
+
+#endif /* __LINUX_PCS_MIIC_H */
--
2.34.1

2022-04-16 01:58:30

[permalink] [raw]

Subject: Re: [PATCH net-next 07/12] net: dsa: rzn1-a5psw: add statistics support

Le Fri, 15 Apr 2022 01:16:11 +0200,
Andrew Lunn <[email protected]> a écrit :

> On Thu, Apr 14, 2022 at 02:22:45PM +0200, Clément Léger wrote:
> > Add per-port statistics. This support requries to add a stat lock since
> > statistics are stored in two 32 bits registers, the hi part one being
> > global and latched when accessing the lo part.
> >
> > Signed-off-by: Clément Léger <[email protected]>
> > ---
> > drivers/net/dsa/rzn1_a5psw.c | 101 +++++++++++++++++++++++++++++++++++
> > drivers/net/dsa/rzn1_a5psw.h | 2 +
> > 2 files changed, 103 insertions(+)
> >
> > diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c
> > index 5bee999f7050..7ab7d9054427 100644
> > --- a/drivers/net/dsa/rzn1_a5psw.c
> > +++ b/drivers/net/dsa/rzn1_a5psw.c
> > @@ -16,6 +16,59 @@
> >
> > #include "rzn1_a5psw.h"
> >
> > +struct a5psw_stats {
> > + u16 offset;
> > + const char *name;
> > +};
> > +
> > +#define STAT_DESC(_offset, _name) {.offset = _offset, .name = _name}
> > +
> > +static const struct a5psw_stats a5psw_stats[] = {
> > + STAT_DESC(0x868, "aFrameTransmitted"),
> > + STAT_DESC(0x86C, "aFrameReceived"),
> > + STAT_DESC(0x8BC, "etherStatsetherStatsOversizePktsDropEvents"),
>
> > +};
>
>
> > +static void a5psw_get_strings(struct dsa_switch *ds, int port, u32 stringset,
> > + uint8_t *data)
> > +{
> > + unsigned int u;
> > +
> > + if (stringset != ETH_SS_STATS)
> > + return;
> > +
> > + for (u = 0; u < ARRAY_SIZE(a5psw_stats); u++) {
> > + strncpy(data + u * ETH_GSTRING_LEN, a5psw_stats[u].name,
> > + ETH_GSTRING_LEN);
> > + }
>
> The kernel strncpy() is like the user space one. It does not add a
> NULL if the string is longer than ETH_GSTRING_LEN and it needs to
> truncate. So there is a danger here.
>
> What you find most drivers do is
>
> struct a5psw_stats {
> u16 offset;
> const char name[ETH_GSTRING_LEN];
> };
>
> You should then get a compiler warning/error if you string is ever
> longer than allowed. And use memcpy() rather than strcpy(), which is
> faster anyway. But you do use up a bit more memory.

Acked.

>
> > +static void a5psw_get_ethtool_stats(struct dsa_switch *ds, int port,
> > + uint64_t *data)
> > +{
> > + struct a5psw *a5psw = ds->priv;
> > + u32 reg_lo, reg_hi;
> > + unsigned int u;
> > +
> > + for (u = 0; u < ARRAY_SIZE(a5psw_stats); u++) {
> > + /* A5PSW_STATS_HIWORD is global and thus, access must be
> > + * exclusive
> > + */
>
> Could you explain that a bit more. The RTNL lock will prevent two
> parallel calls to this function.

Ok, I wasn't sure of the locking applicable here.

--
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

2022-04-16 02:06:40

[permalink] [raw]

Subject: Re: [PATCH net-next 06/12] net: dsa: rzn1-a5psw: add Renesas RZ/N1 advanced 5 port switch driver

On Fri, Apr 15, 2022 at 01:55:03PM +0300, Vladimir Oltean wrote:
> > > The selftests don't cover nearly enough, but just to make sure that they
> > > pass for your switch, when you use 2 switch ports as h1 and h2 (hosts),
> > > and 2 ports as swp1 and swp2? There's surprisingly little that you do on
> > > .port_bridge_join, I need to study the code more.
> >
> > Port isolation is handled by using a pattern matcher which is enabled
> > for each port at setup. If set, the port packet will only be forwarded
> > to the CPU port. When bridging is needed, the pattern matching is
> > disabled and thus, the packets are forwarded between all the ports that
> > are enabled in the bridge.
>
> Is there some public documentation for this pattern matcher?

Again, I realize I haven't made it clear what concerns me here.
On ->port_bridge_join() and ->port_bridge_leave(), the "bridge" is given
to you as argument. 2 ports may join br0, and 2 ports may join br1.
You disregard the "bridge" argument. So you enable forwarding between
br0 and br1. What I'd like to see is what the hardware can do in terms
of this "pattern matching", to improve on this situation.

2022-04-16 02:19:03

[permalink] [raw]

Subject: Re: [PATCH net-next 01/12] net: dsa: add support for Renesas RZ/N1 A5PSW switch tag code

On Thu, Apr 14, 2022 at 02:22:39PM +0200, Cl?ment L?ger wrote:
> Add DSA tag code for Renesas RZ/N1 Advanced 5 port switch. This switch
> uses a special VLAN type followed by 6 bytes which contains other
> useful information (port, timestamp, etc).
>
> Signed-off-by: Cl?ment L?ger <[email protected]>
> ---

Please squash this with the next patch.

> include/net/dsa.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 934958fda962..2aa8eaae4eb9 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -53,6 +53,7 @@ struct phylink_link_state;
> #define DSA_TAG_PROTO_SJA1110_VALUE 23
> #define DSA_TAG_PROTO_RTL8_4_VALUE 24
> #define DSA_TAG_PROTO_RTL8_4T_VALUE 25
> +#define DSA_TAG_PROTO_RZN1_A5PSW_VALUE 26
>
> enum dsa_tag_protocol {
> DSA_TAG_PROTO_NONE = DSA_TAG_PROTO_NONE_VALUE,
> @@ -81,6 +82,7 @@ enum dsa_tag_protocol {
> DSA_TAG_PROTO_SJA1110 = DSA_TAG_PROTO_SJA1110_VALUE,
> DSA_TAG_PROTO_RTL8_4 = DSA_TAG_PROTO_RTL8_4_VALUE,
> DSA_TAG_PROTO_RTL8_4T = DSA_TAG_PROTO_RTL8_4T_VALUE,
> + DSA_TAG_PROTO_RZN1_A5PSW = DSA_TAG_PROTO_RZN1_A5PSW_VALUE,
> };
>
> struct dsa_switch;
> --
> 2.34.1
>

2022-04-16 02:27:29

[permalink] [raw]

Subject: [PATCH net-next 09/12] ARM: dts: r9a06g032: describe MII converter

Add the MII converter node which describes the MII converter that is
present on the RZ/N1 SoC.

Signed-off-by: Clément Léger <[email protected]>
---
arch/arm/boot/dts/r9a06g032.dtsi | 33 ++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)

diff --git a/arch/arm/boot/dts/r9a06g032.dtsi b/arch/arm/boot/dts/r9a06g032.dtsi
index 636a6ab31c58..fd174df268e8 100644
--- a/arch/arm/boot/dts/r9a06g032.dtsi
+++ b/arch/arm/boot/dts/r9a06g032.dtsi
@@ -200,6 +200,39 @@ nand_controller: nand-controller@40102000 {
status = "disabled";
};

+ eth_miic: eth-miic@44030000 {
+ compatible = "renesas,rzn1-miic";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x44030000 0x10000>;
+ clocks = <&sysctrl R9A06G032_CLK_MII_REF>,
+ <&sysctrl R9A06G032_CLK_RGMII_REF>,
+ <&sysctrl R9A06G032_CLK_RMII_REF>,
+ <&sysctrl R9A06G032_HCLK_SWITCH_RG>;
+ clock-names = "mii_ref", "rgmii_ref", "rmii_ref", "hclk_switch_rg";
+ status = "disabled";
+
+ mii_conv0: mii-conv@0 {
+ reg = <0>;
+ };
+
+ mii_conv1: mii-conv@1 {
+ reg = <1>;
+ };
+
+ mii_conv2: mii-conv@2 {
+ reg = <2>;
+ };
+
+ mii_conv3: mii-conv@3 {
+ reg = <3>;
+ };
+
+ mii_conv4: mii-conv@4 {
+ reg = <4>;
+ };
+ };
+
gic: interrupt-controller@44101000 {
compatible = "arm,gic-400", "arm,cortex-a7-gic";
interrupt-controller;
--
2.34.1

2022-04-16 02:30:46

[permalink] [raw]

Subject: Re: [PATCH net-next 07/12] net: dsa: rzn1-a5psw: add statistics support

On Thu, Apr 14, 2022 at 02:22:45PM +0200, Cl?ment L?ger wrote:
> Add per-port statistics. This support requries to add a stat lock since
> statistics are stored in two 32 bits registers, the hi part one being
> global and latched when accessing the lo part.
>
> Signed-off-by: Cl?ment L?ger <[email protected]>
> ---
> drivers/net/dsa/rzn1_a5psw.c | 101 +++++++++++++++++++++++++++++++++++
> drivers/net/dsa/rzn1_a5psw.h | 2 +
> 2 files changed, 103 insertions(+)
>
> diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c
> index 5bee999f7050..7ab7d9054427 100644
> --- a/drivers/net/dsa/rzn1_a5psw.c
> +++ b/drivers/net/dsa/rzn1_a5psw.c
> @@ -16,6 +16,59 @@
>
> #include "rzn1_a5psw.h"
>
> +struct a5psw_stats {
> + u16 offset;
> + const char *name;
> +};
> +
> +#define STAT_DESC(_offset, _name) {.offset = _offset, .name = _name}
> +
> +static const struct a5psw_stats a5psw_stats[] = {
> + STAT_DESC(0x868, "aFrameTransmitted"),
> + STAT_DESC(0x86C, "aFrameReceived"),
> + STAT_DESC(0x8BC, "etherStatsetherStatsOversizePktsDropEvents"),

> +};

> +static void a5psw_get_strings(struct dsa_switch *ds, int port, u32 stringset,
> + uint8_t *data)
> +{
> + unsigned int u;
> +
> + if (stringset != ETH_SS_STATS)
> + return;
> +
> + for (u = 0; u < ARRAY_SIZE(a5psw_stats); u++) {
> + strncpy(data + u * ETH_GSTRING_LEN, a5psw_stats[u].name,
> + ETH_GSTRING_LEN);
> + }

The kernel strncpy() is like the user space one. It does not add a
NULL if the string is longer than ETH_GSTRING_LEN and it needs to
truncate. So there is a danger here.

What you find most drivers do is

struct a5psw_stats {
u16 offset;
const char name[ETH_GSTRING_LEN];
};

You should then get a compiler warning/error if you string is ever
longer than allowed. And use memcpy() rather than strcpy(), which is
faster anyway. But you do use up a bit more memory.

> +static void a5psw_get_ethtool_stats(struct dsa_switch *ds, int port,
> + uint64_t *data)
> +{
> + struct a5psw *a5psw = ds->priv;
> + u32 reg_lo, reg_hi;
> + unsigned int u;
> +
> + for (u = 0; u < ARRAY_SIZE(a5psw_stats); u++) {
> + /* A5PSW_STATS_HIWORD is global and thus, access must be
> + * exclusive
> + */

Could you explain that a bit more. The RTNL lock will prevent two
parallel calls to this function.

> + spin_lock(&a5psw->stats_lock);
> + reg_lo = a5psw_reg_readl(a5psw, a5psw_stats[u].offset +
> + A5PSW_PORT_OFFSET(port));
> + /* A5PSW_STATS_HIWORD is latched on stat read */
> + reg_hi = a5psw_reg_readl(a5psw, A5PSW_STATS_HIWORD);
> +
> + data[u] = ((u64)reg_hi << 32) | reg_lo;
> + spin_unlock(&a5psw->stats_lock);
> + }
> +}

Andrew

2022-04-16 02:41:51

[permalink] [raw]

Subject: Re: [PATCH net-next 06/12] net: dsa: rzn1-a5psw: add Renesas RZ/N1 advanced 5 port switch driver

On Fri, Apr 15, 2022 at 11:34:53AM +0200, Cl?ment L?ger wrote:
> Le Thu, 14 Apr 2022 17:47:09 +0300,
> Vladimir Oltean <[email protected]> a ?crit :
> > > later (vlan, etc).
> > >
> > > Suggested-by: Laurent Gonzales <[email protected]>
> > > Suggested-by: Jean-Pierre Geslin <[email protected]>
> > > Suggested-by: Phil Edworthy <[email protected]>
> >
> > Suggested? What did they suggest? "You should write a driver"?
> > We have a Co-developed-by: tag, maybe it's more appropriate here?
>
> This driver was written from scratch but some ideas (port isolation
> using pattern matcher) was inspired from a previous driver. I thought it
> would be nice to give them credit for that.
>
> [...]

Ok, in that case I don't really know how to mark sources of inspiration
in the commit message, maybe your approach is fine.

> > > obj-y += hirschmann/
> > > obj-y += microchip/
> > > diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c
> > > new file mode 100644
> > > index 000000000000..5bee999f7050
> > > --- /dev/null
> > > +++ b/drivers/net/dsa/rzn1_a5psw.c
> > > @@ -0,0 +1,676 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright (C) 2022 Schneider-Electric
> > > + *
> > > + * Cl?ment L?ger <[email protected]>
> > > + */
> > > +
> > > +#include <linux/clk.h>
> > > +#include <linux/etherdevice.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_mdio.h>
> > > +#include <net/dsa.h>
> > > +#include <uapi/linux/if_bridge.h>
> >
> > Why do you need to include this header?
>
> It defines BR_STATE_* but I guess linux/if_bridge.h does include it.

Yes.

> > > +static void a5psw_port_pattern_set(struct a5psw *a5psw, int port, int pattern,
> > > + bool enable)
> > > +{
> > > + u32 rx_match = 0;
> > > +
> > > + if (enable)
> > > + rx_match |= A5PSW_RXMATCH_CONFIG_PATTERN(pattern);
> > > +
> > > + a5psw_reg_rmw(a5psw, A5PSW_RXMATCH_CONFIG(port),
> > > + A5PSW_RXMATCH_CONFIG_PATTERN(pattern), rx_match);
> > > +}
> > > +
> > > +static void a5psw_port_mgmtfwd_set(struct a5psw *a5psw, int port, bool enable)
> >
> > Some explanation on what "management forward" means/does?
>
> I'll probably rename that cpu_port_forward to match the dsa naming.
> It'll actually isolate the port from other ports by only forwarding the
> packets to the CPU port.

You could probably do without a rename by just adding a comment that
says that it enables forwarding only towards the management port.

> > Please implement .shutdown too, it's non-optional.
>
> Hum, platform_shutdown does seems to check for the .shutdown callback:
>
> static void platform_shutdown(struct device *_dev)
> {
> struct platform_device *dev = to_platform_device(_dev);
> struct platform_driver *drv;
>
> if (!_dev->driver)
> return;
>
> drv = to_platform_driver(_dev->driver);
> if (drv->shutdown)
> drv->shutdown(dev);
> }
>
> Is there some documentation specifying that this is mandatory ?
> If so, should I just set it to point to an empty shutdown function then
> ?

I meant that for a DSA switch driver is mandatory to call dsa_switch_shutdown()
from your ->shutdown method, otherwise subtle things break, sorry for being unclear.

Please blindly copy-paste the odd pattern that all other DSA drivers use
in ->shutdown and ->remove (with the platform_set_drvdata(dev, NULL) calls),
like a normal person :)

> > > + * @reg_lock: Lock for register read-modify-write operation
> >
> > Interesting concept. Generally we see higher-level locking schemes
> > (i.e. a rmw lock won't really ensure much in terms of consistency of
> > settings if that's the only thing that serializes concurrent thread
> > accesses to some register).
>
> Agreed, this does not guarantee consistency of settings but guarantees
> that rmw modifications are atomic between devices. I wasn't sure about
> the locking guarantee that I could have. After looking at other
> drivers, I guess I will switch to something more common such as using
> a global mutex for register accesses.

LOL, that isn't better...

Ideally locking would be done per functionality that the hardware can
perform independently (like lookup table access, VLAN table access,
forwarding domain control, PTP block, link state control, etc).
You don't want to artificially serialize unrelated stuff.
A "read-modify-write" lock would similarly artificially serialize
unrelated stuff for you, even if you intend it to only serialize
something entirely different.

Most things as seen by a DSA switch driver are implicitly serialized by
the rtnl_mutex anyway. Some things aren't (->port_fdb_add, ->port_fdb_del).
There is a point to be made about adding locks for stuff that is
implicitly serialized by the rtnl_mutex, since you can't really test
their effectiveness. This makes it more difficult for the driver writer
to make the right decision about locking, since in some cases, the
serialization given by the rtnl_mutex isn't something fundamental and
may be removed, to reduce contention on that lock. In that case, it is
always a nice surprise to find a backup locking scheme in converted
drivers. With the mention that said backup locking scheme was never
really tested, so it may be that it needs further work anyway.

> > The selftests don't cover nearly enough, but just to make sure that they
> > pass for your switch, when you use 2 switch ports as h1 and h2 (hosts),
> > and 2 ports as swp1 and swp2? There's surprisingly little that you do on
> > .port_bridge_join, I need to study the code more.
>
> Port isolation is handled by using a pattern matcher which is enabled
> for each port at setup. If set, the port packet will only be forwarded
> to the CPU port. When bridging is needed, the pattern matching is
> disabled and thus, the packets are forwarded between all the ports that
> are enabled in the bridge.

Is there some public documentation for this pattern matcher?

2022-04-16 02:41:58

by Russell King (Oracle)

[permalink] [raw]

Subject: Re: [PATCH net-next 06/12] net: dsa: rzn1-a5psw: add Renesas RZ/N1 advanced 5 port switch driver

On Fri, Apr 15, 2022 at 01:55:03PM +0300, Vladimir Oltean wrote:
> I meant that for a DSA switch driver is mandatory to call dsa_switch_shutdown()
> from your ->shutdown method, otherwise subtle things break, sorry for being unclear.
>
> Please blindly copy-paste the odd pattern that all other DSA drivers use
> in ->shutdown and ->remove (with the platform_set_drvdata(dev, NULL) calls),
> like a normal person :)

Those platform_set_drvdata(, NULL) calls should be killed - the
driver model will set the driver data to NULL after ->remove has
been called - so having drivers also setting the driver data to
NULL is mere duplication.

The only case it would matter is if someone is looking up the device
and then accessing the driver data - and one would hope that's done
with appropriate locking or other guarantees (e.g. driver can never
be unbound once the driver data has been set.)

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

2022-04-20 06:45:43

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH net-next 03/12] dt-bindings: net: pcs: add bindings for Renesas RZ/N1 MII converter

On Thu, Apr 14, 2022 at 02:22:41PM +0200, Cl?ment L?ger wrote:
> This MII converter can be found on the RZ/N1 processor family. The MII
> converter ports are declared as subnodes which are then referenced by
> users of the PCS driver such as the switch.
>
> Signed-off-by: Cl?ment L?ger <[email protected]>
> ---
> .../bindings/net/pcs/renesas,rzn1-miic.yaml | 95 +++++++++++++++++++
> include/dt-bindings/net/pcs-rzn1-miic.h | 19 ++++
> 2 files changed, 114 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/pcs/renesas,rzn1-miic.yaml
> create mode 100644 include/dt-bindings/net/pcs-rzn1-miic.h
>
> diff --git a/Documentation/devicetree/bindings/net/pcs/renesas,rzn1-miic.yaml b/Documentation/devicetree/bindings/net/pcs/renesas,rzn1-miic.yaml
> new file mode 100644
> index 000000000000..ccb25ce6cbde
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/pcs/renesas,rzn1-miic.yaml
> @@ -0,0 +1,95 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/pcs/renesas,rzn1-miic.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas RZ/N1 MII converter
> +
> +maintainers:
> + - Cl?ment L?ger <[email protected]>
> +
> +description: |
> + This MII converter is present on the Renesas RZ/N1 SoC family. It is
> + responsible to do MII passthrough or convert it to RMII/RGMII.
> +
> +properties:
> + compatible:
> + const: renesas,rzn1-miic

Need SoC specific compatibles.

> +
> + reg:
> + maxItems: 1
> +
> + "#address-cells":
> + const: 1
> +
> + "#size-cells":
> + const: 0
> +
> + clocks:
> + items:
> + - description: MII reference clock
> + - description: RGMII reference clock
> + - description: RMII reference clock
> + - description: AHB clock used for the MII converter register interface
> +
> + renesas,miic-cfg-mode:
> + description: MII mux configuration mode. This value should use one of the
> + value defined in dt-bindings/net/pcs-rzn1-miic.h.

Describe possible values here as constraints. At present, I don't see
the point of this property if there is only 1 possible value and it is
required.

> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> +patternProperties:
> + "^mii-conv@[0-4]$":
> + type: object

additionalProperties: false

> + description: MII converter port
> +
> + properties:
> + reg:
> + maxItems: 1

Why do you need sub-nodes? They don't have any properties. A simple mask
property could tell you which ports are present/active/enabled if that's
what you are tracking. Or the SoC specific compatibles you need to add
can imply the ports if they are SoC specific.

> +
> +required:
> + - compatible
> + - reg
> + - clocks
> + - renesas,miic-cfg-mode
> + - "#address-cells"
> + - "#size-cells"
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/net/pcs-rzn1-miic.h>
> + #include <dt-bindings/clock/r9a06g032-sysctrl.h>
> +
> + eth-miic@44030000 {
> + compatible = "renesas,rzn1-miic";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0x44030000 0x10000>;
> + clocks = <&sysctrl R9A06G032_CLK_MII_REF>,
> + <&sysctrl R9A06G032_CLK_RGMII_REF>,
> + <&sysctrl R9A06G032_CLK_RMII_REF>,
> + <&sysctrl R9A06G032_HCLK_SWITCH_RG>;
> + renesas,miic-cfg-mode = <MIIC_MUX_MAC2_MAC1_SWD_SWC_SWB_SWA>;
> +
> + mii_conv0: mii-conv@0 {
> + reg = <0>;
> + };
> +
> + mii_conv1: mii-conv@1 {
> + reg = <1>;
> + };
> +
> + mii_conv2: mii-conv@2 {
> + reg = <2>;
> + };
> +
> + mii_conv3: mii-conv@3 {
> + reg = <3>;
> + };
> +
> + mii_conv4: mii-conv@4 {
> + reg = <4>;
> + };
> + };
> \ No newline at end of file

Fix this.

> diff --git a/include/dt-bindings/net/pcs-rzn1-miic.h b/include/dt-bindings/net/pcs-rzn1-miic.h
> new file mode 100644
> index 000000000000..c5a0f382967b
> --- /dev/null
> +++ b/include/dt-bindings/net/pcs-rzn1-miic.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

Dual license please.

> +/*
> + * Copyright (C) 2022 Schneider-Electric
> + *
> + * Cl?ment L?ger <[email protected]>
> + */
> +
> +#ifndef _DT_BINDINGS_PCS_RZN1_MIIC
> +#define _DT_BINDINGS_PCS_RZN1_MIIC
> +
> +/*
> + * Reefer to the datasheet [1] section 8.2.1, Internal Connection of Ethernet
> + * Ports to check the meaning of these values.
> + *
> + * [1] REN_r01uh0750ej0140-rzn1-introduction_MAT_20210228.pdf
> + */
> +#define MIIC_MUX_MAC2_MAC1_SWD_SWC_SWB_SWA 0x13
> +
> +#endif
> --
> 2.34.1
>
>

2022-04-20 13:47:48

by Geert Uytterhoeven

[permalink] [raw]

Subject: Re: [PATCH net-next 04/12] net: pcs: add Renesas MII converter driver

Hi Clément,

Thanks for your patch!
Only cosmetic comments from me, as I'm not too familiar with MII.

On Thu, Apr 14, 2022 at 2:24 PM Clément Léger <[email protected]> wrote:
> Add PCS driver for the MII converter that is present on Renesas RZ/N1

Add a ... on the ...

> SoC. This MII converter is reponsible of converting MII to RMII/RGMII

responsible for

> or act as a MII passtrough. Exposing it as a PCS allows to reuse it

pass-through

> in both the switch driver and the stmmac driver. Currently, this driver
> only allows the PCS to be used by the dual Cortex-A7 subsystem since
> the register locking system is not used.
>
> Signed-off-by: Clément Léger <[email protected]>

> --- a/drivers/net/pcs/Kconfig
> +++ b/drivers/net/pcs/Kconfig
> @@ -18,4 +18,11 @@ config PCS_LYNX
> This module provides helpers to phylink for managing the Lynx PCS
> which is part of the Layerscape and QorIQ Ethernet SERDES.
>
> +config PCS_RZN1_MIIC
> + tristate "Renesas RZN1 MII converter"

RZ/N1

> + help
> + This module provides a driver for the MII converter that is available
> + on RZN1 SoC. This PCS convert MII to RMII/RGMII or can be in

RZ/N1

> + passthrough mode for MII.
> +
> endmenu

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2022-04-21 08:13:56

[permalink] [raw]

Subject: Re: [PATCH net-next 03/12] dt-bindings: net: pcs: add bindings for Renesas RZ/N1 MII converter

Le Wed, 20 Apr 2022 15:11:26 -0500,
Rob Herring <[email protected]> a écrit :

> On Tue, Apr 19, 2022 at 05:00:44PM +0200, Clément Léger wrote:
> > Le Tue, 19 Apr 2022 08:43:47 -0500,
> > Rob Herring <[email protected]> a écrit :
> >
> > > > + clocks:
> > > > + items:
> > > > + - description: MII reference clock
> > > > + - description: RGMII reference clock
> > > > + - description: RMII reference clock
> > > > + - description: AHB clock used for the MII converter register interface
> > > > +
> > > > + renesas,miic-cfg-mode:
> > > > + description: MII mux configuration mode. This value should use one of the
> > > > + value defined in dt-bindings/net/pcs-rzn1-miic.h.
> > >
> > > Describe possible values here as constraints. At present, I don't see
> > > the point of this property if there is only 1 possible value and it is
> > > required.
> >
> > The ethernet subsystem contains a number of internal muxes that allows
> > to configure ethernet routing. This configuration option allows to set
> > the register that configure these muxes.
> >
> > After talking with Andrew, I considered moving to something like this:
> >
> > eth-miic@44030000 {
> > compatible = "renesas,rzn1-miic";
> >
> > mii_conv1: mii-conv-1 {
> > renesas,miic-input = <MIIC_GMAC1_PORT>;
> > port = <1>;
>
> 'port' is already used, find another name. Maybe 'reg' here. Don't know.
> What do 1 and 2 represent?

'port' represent the port index in the MII converter IP. I went with reg
first, but according to Andrew feedback, it looked like it was a bad
idea since it was not really a "bus". However, this pattern is already
used for dsa ports.

> > >
> > > Why do you need sub-nodes? They don't have any properties. A simple mask
> > > property could tell you which ports are present/active/enabled if that's
> > > what you are tracking. Or the SoC specific compatibles you need to add
> > > can imply the ports if they are SoC specific.
> >
> > The MACs are using phandles to these sub-nodes to query a specific MII
> > converter port PCS:
> >
> > switch@44050000 {
> > compatible = "renesas,rzn1-a5psw";
> >
> > ports {
> > port@0 {
>
> ethernet-ports and ethernet-port so we don't collide with the graph
> binding.

Acked.

>
>
> > reg = <0>;
> > label = "lan0";
> > phy-handle = <&switch0phy3>;
> > pcs-handle = <&mii_conv4>;
> > };
> > };
> > };
> >
> > According to Andrew, this is not a good idea to represent the PCS as a
> > bus since it is indeed not a bus. I could also switch to something like
> > pcs-handle = <&eth_mii 4> but i'm not sure what you'd prefer. We could
> > also remove this from the device-tree and consider each driver to
> > request the MII ouput to be configured using something like this for
> > instance:
>
> I'm pretty sure we already defined pcs-handle as only a phandle. If you
> want variable cells, then it's got to be extended like all the other
> properties using that pattern.

Yep, it seems to be used in some other driver as a single phandle too.
I'll kept that.

>
> >
> > miic_request_pcs(pcs_np, miic_port_nr, MIIC_SWITCHD_PORT);
> >
> > But I'm not really fan of this because it requires the drivers to
> > assume some specificities of the MII converter (port number are not in
> > the same order of the switch for instance) and thus I would prefer this
> > to be in the device-tree.
> >
> > Let me know if you can think of something that would suit you
> > better but keep in mind that I need to correctly match a switch/MAC
> > port with a PCS port and that I also need to configure MII internal
> > muxes.
> >
> > For more information, you can look at section 8 of the manual at [1].
>
> I can't really. Other people want their bindings reviewed too.

No worries.

>
> Rob

--
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

2022-04-21 16:25:32

by Geert Uytterhoeven

[permalink] [raw]

Subject: Re: [PATCH net-next 10/12] ARM: dts: r9a06g032: describe GMAC2

Hi Clément,

On Thu, Apr 14, 2022 at 2:24 PM Clément Léger <[email protected]> wrote:
> RZ/N1 SoC includes two MAC named GMACx that are compatible with the
> "snps,dwmac" driver. GMAC1 is connected directly to the MII converter
> port 1. GMAC2 however can be used as the MAC for the switch CPU
> management port or can be muxed to be connected directly to the MII
> converter port 2. This commit add description for the GMAC2 which will
> be used by the switch description.
>
> Signed-off-by: Clément Léger <[email protected]>

Thanks for your patch!

> --- a/arch/arm/boot/dts/r9a06g032.dtsi
> +++ b/arch/arm/boot/dts/r9a06g032.dtsi
> @@ -200,6 +200,23 @@ nand_controller: nand-controller@40102000 {
> status = "disabled";
> };
>
> + gmac2: ethernet@44002000 {
> + compatible = "snps,dwmac-3.72a", "snps,dwmac";

"make dtbs_check":
arch/arm/boot/dts/r9a06g032-rzn1d400-db.dtb:0:0:
/soc/ethernet@44002000: failed to match any schema with compatible:
['snps,dwmac-3.72a', 'snps,dwmac']

> + reg = <0x44002000 0x2000>;
> + interrupt-parent = <&gic>;
> + interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-names = "macirq", "eth_lpi", "eth_wake_irq";

arch/arm/boot/dts/r9a06g032-rzn1d400-db.dtb: ethernet@44002000:
interrupt-names:1: 'eth_wake_irq' was expected
arch/arm/boot/dts/r9a06g032-rzn1d400-db.dtb: ethernet@44002000:
interrupt-names:2: 'eth_lpi' was expected
From schema: Documentation/devicetree/bindings/net/snps,dwmac.yaml

> + clock-names = "stmmaceth";
> + clocks = <&sysctrl R9A06G032_HCLK_GMAC1>;
> + snps,multicast-filter-bins = <256>;
> + snps,perfect-filter-entries = <128>;
> + tx-fifo-depth = <2048>;
> + rx-fifo-depth = <4096>;
> + status = "disabled";
> + };
> +
> eth_miic: eth-miic@44030000 {
> compatible = "renesas,rzn1-miic";
> #address-cells = <1>;

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2022-04-21 20:37:18

[permalink] [raw]

Subject: Re: [PATCH net-next 08/12] net: dsa: rzn1-a5psw: add FDB support

Le Thu, 14 Apr 2022 20:51:40 +0300,
Vladimir Oltean <[email protected]> a écrit :

> > +
> > +static int a5psw_port_fdb_add(struct dsa_switch *ds, int port,
> > + const unsigned char *addr, u16 vid,
> > + struct dsa_db db)
>
> This isn't something that is documented because I haven't had time to
> update that, but new drivers should comply to the requirements for FDB
> isolation (not ignore the passed "db" here) and eventually set
> ds->fdb_isolation = true. Doing so would allow your switch to behave
> correctly when
> - there is more than one bridge spanning its ports,
> - some ports are standalone and some ports are bridged
> - standalone ports are looped back via an external cable with bridged
> ports
> - unrecognized upper interfaces (bond, team) are used, and those are
> bridged directly with some other switch ports
>
> The most basic thing you need to do to satisfy the requirements is to
> figure out what mechanism for FDB partitioning does your hardware have.
> If the answer is "none", then we'll have to use VLANs for that: all
> standalone ports to share a VLAN, each VLAN-unaware bridge to share a
> VLAN across all member ports, each VLAN of a VLAN-aware bridge to
> reserve its own VLAN. Up to a total of 32 VLANs, since I notice that's
> what the limit for your hardware is.

Ok, I see the idea. In the mean time, could we make a first step with a
single bridge and without VLAN support ? This is expected to come later
anyway.

>
> But I see this patch set doesn't include VLAN functionality (and also
> ignores the "vid" from FDB entries), so I can't really say more right now.
> But if you could provide more information about the hardware
> capabilities we can discuss implementation options.

That's indeed the problem. The FDB table does not seems to have
partitionning at all (except for ports) and entries (such as seen below)
do not contain any VLAN information.

> > diff --git a/drivers/net/dsa/rzn1_a5psw.h b/drivers/net/dsa/rzn1_a5psw.h
> > index b34ea549e936..37aa89383e70 100644
> > --- a/drivers/net/dsa/rzn1_a5psw.h
> > +++ b/drivers/net/dsa/rzn1_a5psw.h
> > @@ -167,6 +167,22 @@
> > #define A5PSW_CTRL_TIMEOUT 1000
> > #define A5PSW_TABLE_ENTRIES 8192
> >
> > +struct fdb_entry {
>
> Shouldn't this contain something along the lines of a VID, FID, something?

This is extracted directly from the datasheet [1]. The switch FDB table
does not seems to store the VID with the entries (See page 300).

[1]
https://www.renesas.com/us/en/document/mah/rzn1d-group-rzn1s-group-rzn1l-group-users-manual-r-engine-and-ethernet-peripherals
>
> > + u8 mac[ETH_ALEN];
> > + u8 valid:1;
> > + u8 is_static:1;
> > + u8 prio:3;
> > + u8 port_mask:5;
> > +} __packed;
> > +
> > +union lk_data {
> > + struct {
> > + u32 lo;
> > + u32 hi;
> > + };
> > + struct fdb_entry entry;
> > +};
> > +
> > /**
> > * struct a5psw - switch struct
> > * @base: Base address of the switch
> > --
> > 2.34.1
> >
>

--
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

2022-04-22 05:50:54

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH net-next 03/12] dt-bindings: net: pcs: add bindings for Renesas RZ/N1 MII converter

On Tue, Apr 19, 2022 at 05:00:44PM +0200, Cl?ment L?ger wrote:
> Le Tue, 19 Apr 2022 08:43:47 -0500,
> Rob Herring <[email protected]> a ?crit :
>
> > > + clocks:
> > > + items:
> > > + - description: MII reference clock
> > > + - description: RGMII reference clock
> > > + - description: RMII reference clock
> > > + - description: AHB clock used for the MII converter register interface
> > > +
> > > + renesas,miic-cfg-mode:
> > > + description: MII mux configuration mode. This value should use one of the
> > > + value defined in dt-bindings/net/pcs-rzn1-miic.h.
> >
> > Describe possible values here as constraints. At present, I don't see
> > the point of this property if there is only 1 possible value and it is
> > required.
>
> The ethernet subsystem contains a number of internal muxes that allows
> to configure ethernet routing. This configuration option allows to set
> the register that configure these muxes.
>
> After talking with Andrew, I considered moving to something like this:
>
> eth-miic@44030000 {
> compatible = "renesas,rzn1-miic";
>
> mii_conv1: mii-conv-1 {
> renesas,miic-input = <MIIC_GMAC1_PORT>;
> port = <1>;

'port' is already used, find another name. Maybe 'reg' here. Don't know.
What do 1 and 2 represent?

> };
> mii_conv2: mii-conv-2 {
> renesas,miic-input = <MIIC_SWITCHD_PORT>;
> port = <2>;
> };
> ...
> };
>
> Which would allow embedding the configuration inside the port
> sub-nodes. Moreover, it allows a better validation of the values using
> the schema validation directly since only a limited number of values
> are allowed for each port.
>
> >
> > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > +
> > > +patternProperties:
> > > + "^mii-conv@[0-4]$":
> > > + type: object
> >
> > additionalProperties: false
> >
> > > + description: MII converter port
> > > +
> > > + properties:
> > > + reg:
> > > + maxItems: 1
> >
> > Why do you need sub-nodes? They don't have any properties. A simple mask
> > property could tell you which ports are present/active/enabled if that's
> > what you are tracking. Or the SoC specific compatibles you need to add
> > can imply the ports if they are SoC specific.
>
> The MACs are using phandles to these sub-nodes to query a specific MII
> converter port PCS:
>
> switch@44050000 {
> compatible = "renesas,rzn1-a5psw";
>
> ports {
> port@0 {

ethernet-ports and ethernet-port so we don't collide with the graph
binding.

> reg = <0>;
> label = "lan0";
> phy-handle = <&switch0phy3>;
> pcs-handle = <&mii_conv4>;
> };
> };
> };
>
> According to Andrew, this is not a good idea to represent the PCS as a
> bus since it is indeed not a bus. I could also switch to something like
> pcs-handle = <&eth_mii 4> but i'm not sure what you'd prefer. We could
> also remove this from the device-tree and consider each driver to
> request the MII ouput to be configured using something like this for
> instance:

I'm pretty sure we already defined pcs-handle as only a phandle. If you
want variable cells, then it's got to be extended like all the other
properties using that pattern.

>
> miic_request_pcs(pcs_np, miic_port_nr, MIIC_SWITCHD_PORT);
>
> But I'm not really fan of this because it requires the drivers to
> assume some specificities of the MII converter (port number are not in
> the same order of the switch for instance) and thus I would prefer this
> to be in the device-tree.
>
> Let me know if you can think of something that would suit you
> better but keep in mind that I need to correctly match a switch/MAC
> port with a PCS port and that I also need to configure MII internal
> muxes.
>
> For more information, you can look at section 8 of the manual at [1].

I can't really. Other people want their bindings reviewed too.

Rob

2022-04-22 20:09:20

[permalink] [raw]

Subject: Re: [PATCH net-next 08/12] net: dsa: rzn1-a5psw: add FDB support

On Wed, Apr 20, 2022 at 10:16:48AM +0200, Cl?ment L?ger wrote:
> Le Thu, 14 Apr 2022 20:51:40 +0300,
> Vladimir Oltean <[email protected]> a ?crit :
>
> > > +
> > > +static int a5psw_port_fdb_add(struct dsa_switch *ds, int port,
> > > + const unsigned char *addr, u16 vid,
> > > + struct dsa_db db)
> >
> > This isn't something that is documented because I haven't had time to
> > update that, but new drivers should comply to the requirements for FDB
> > isolation (not ignore the passed "db" here) and eventually set
> > ds->fdb_isolation = true. Doing so would allow your switch to behave
> > correctly when
> > - there is more than one bridge spanning its ports,
> > - some ports are standalone and some ports are bridged
> > - standalone ports are looped back via an external cable with bridged
> > ports
> > - unrecognized upper interfaces (bond, team) are used, and those are
> > bridged directly with some other switch ports
> >
> > The most basic thing you need to do to satisfy the requirements is to
> > figure out what mechanism for FDB partitioning does your hardware have.
> > If the answer is "none", then we'll have to use VLANs for that: all
> > standalone ports to share a VLAN, each VLAN-unaware bridge to share a
> > VLAN across all member ports, each VLAN of a VLAN-aware bridge to
> > reserve its own VLAN. Up to a total of 32 VLANs, since I notice that's
> > what the limit for your hardware is.
>
> Ok, I see the idea. In the mean time, could we make a first step with a
> single bridge and without VLAN support ? This is expected to come later
> anyway.
>
> >
> > But I see this patch set doesn't include VLAN functionality (and also
> > ignores the "vid" from FDB entries), so I can't really say more right now.
> > But if you could provide more information about the hardware
> > capabilities we can discuss implementation options.
>
> That's indeed the problem. The FDB table does not seems to have
> partitionning at all (except for ports) and entries (such as seen below)
> do not contain any VLAN information.
>
> > > diff --git a/drivers/net/dsa/rzn1_a5psw.h b/drivers/net/dsa/rzn1_a5psw.h
> > > index b34ea549e936..37aa89383e70 100644
> > > --- a/drivers/net/dsa/rzn1_a5psw.h
> > > +++ b/drivers/net/dsa/rzn1_a5psw.h
> > > @@ -167,6 +167,22 @@
> > > #define A5PSW_CTRL_TIMEOUT 1000
> > > #define A5PSW_TABLE_ENTRIES 8192
> > >
> > > +struct fdb_entry {
> >
> > Shouldn't this contain something along the lines of a VID, FID, something?
>
> This is extracted directly from the datasheet [1]. The switch FDB table
> does not seems to store the VID with the entries (See page 300).
>
> [1]
> https://www.renesas.com/us/en/document/mah/rzn1d-group-rzn1s-group-rzn1l-group-users-manual-r-engine-and-ethernet-peripherals

Thanks for the link. I see that the switch has a non-partitionable
lookup table, not even by VLAN. A shame.

This is also in contrast with the software bridge driver, where FDB and
MDB entries can have independent destinations per VID.

So there's nothing you can do beyond limiting to a single offloaded
bridge and hoping for the best w.r.t. per-VLAN forwarding destinations.

Note that if you limit to a single bridge does not mean that you can
declare ds->fdb_isolation = true. Declaring that would opt you into
unicast and multicast filtering towards the CPU, i.o.w. a method for
software to only receive the addresses it has expressed an interest in,
rather than all packets received on standalone ports. The way that is
implemented in DSA is by adding FDB and MDB entries on the management
port, and it would break a lot of things without a partitioning scheme
for the lookup table.

2022-04-22 21:29:37

[permalink] [raw]

Subject: Re: [PATCH net-next 08/12] net: dsa: rzn1-a5psw: add FDB support

Le Wed, 20 Apr 2022 22:52:14 +0300,
Vladimir Oltean <[email protected]> a écrit :

> > >
> > > Shouldn't this contain something along the lines of a VID, FID, something?
> >
> > This is extracted directly from the datasheet [1]. The switch FDB table
> > does not seems to store the VID with the entries (See page 300).
> >
> > [1]
> > https://www.renesas.com/us/en/document/mah/rzn1d-group-rzn1s-group-rzn1l-group-users-manual-r-engine-and-ethernet-peripherals
>
> Thanks for the link. I see that the switch has a non-partitionable
> lookup table, not even by VLAN. A shame.
>
> This is also in contrast with the software bridge driver, where FDB and
> MDB entries can have independent destinations per VID.
>
> So there's nothing you can do beyond limiting to a single offloaded
> bridge and hoping for the best w.r.t. per-VLAN forwarding destinations.
>
> Note that if you limit to a single bridge does not mean that you can
> declare ds->fdb_isolation = true. Declaring that would opt you into
> unicast and multicast filtering towards the CPU, i.o.w. a method for
> software to only receive the addresses it has expressed an interest in,
> rather than all packets received on standalone ports. The way that is
> implemented in DSA is by adding FDB and MDB entries on the management
> port, and it would break a lot of things without a partitioning scheme
> for the lookup table.

Thanks Vladimir, it confirms what I thought.

--
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

2022-04-22 22:56:14