| Message ID | 20220414122250.158113-7-clement.leger@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | add support for Renesas RZ/N1 ethernet subsystem devices | expand |
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... > +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); > + } If the port supports e.g. RGMII (as it does via the media converter) then it also supports 1000baseX modes as well - because a PHY attached to the RGMII port can convert to 1000baseX. > + > + linkmode_and(supported, supported, mask); > + linkmode_and(state->advertising, state->advertising, mask); > +} This basically means "I support every phy_interface_t mode that has ever been implemented" which surely is not what you want. I doubt from the above that you support 10GBASE-KR for example. Please instead implement the .phylink_get_caps DSA switch interface, and fill in the config->supported_interfaces for all interface modes that the port supports (that including the media converter as well) and the config->mac_capabilities members.
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 <laurent.gonzales@non.se.com> > Suggested-by: Jean-Pierre Geslin <jean-pierre.geslin@non.se.com> > Suggested-by: Phil Edworthy <phil.edworthy@renesas.com> 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 <clement.leger@bootlin.com> > --- > 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 <clement.leger@bootlin.com> > + */ > + > +#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.
> > +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
+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
Le Thu, 14 Apr 2022 14:02:10 +0100, "Russell King (Oracle)" <linux@armlinux.org.uk> 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 ? > > > +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); > > + } > > If the port supports e.g. RGMII (as it does via the media converter) > then it also supports 1000baseX modes as well - because a PHY attached > to the RGMII port can convert to 1000baseX. > > > + > > + linkmode_and(supported, supported, mask); > > + linkmode_and(state->advertising, state->advertising, mask); > > +} > > This basically means "I support every phy_interface_t mode that has ever > been implemented" which surely is not what you want. I doubt from the > above that you support 10GBASE-KR for example. Hmmm yes indeed, that's not what I meant *at all*. > > Please instead implement the .phylink_get_caps DSA switch interface, and > fill in the config->supported_interfaces for all interface modes that > the port supports (that including the media converter as well) and the > config->mac_capabilities members. > Ok, looks indeed more fitted and easier to understand.
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)" <linux@armlinux.org.uk> 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.
Le Thu, 14 Apr 2022 17:47:09 +0300, Vladimir Oltean <olteanv@gmail.com> a écrit : > > > later (vlan, etc). > > > > Suggested-by: Laurent Gonzales <laurent.gonzales@non.se.com> > > Suggested-by: Jean-Pierre Geslin <jean-pierre.geslin@non.se.com> > > Suggested-by: Phil Edworthy <phil.edworthy@renesas.com> > > 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 <clement.leger@bootlin.com> > > + */ > > + > > +#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,
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 <olteanv@gmail.com> a écrit : > > > later (vlan, etc). > > > > > > Suggested-by: Laurent Gonzales <laurent.gonzales@non.se.com> > > > Suggested-by: Jean-Pierre Geslin <jean-pierre.geslin@non.se.com> > > > Suggested-by: Phil Edworthy <phil.edworthy@renesas.com> > > > > 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 <clement.leger@bootlin.com> > > > + */ > > > + > > > +#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?
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.)
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.
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.
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.
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.
Le Fri, 15 Apr 2022 13:55:03 +0300, Vladimir Oltean <olteanv@gmail.com> 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 <olteanv@gmail.com> a écrit : > > > > later (vlan, etc). > > > > > > > > Suggested-by: Laurent Gonzales <laurent.gonzales@non.se.com> > > > > Suggested-by: Jean-Pierre Geslin <jean-pierre.geslin@non.se.com> > > > > Suggested-by: Phil Edworthy <phil.edworthy@renesas.com> > > > > > > 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 <clement.leger@bootlin.com> > > > > + */ > > > > + > > > > +#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
Le Fri, 15 Apr 2022 14:05:24 +0300, Vladimir Oltean <olteanv@gmail.com> 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.
Le Thu, 14 Apr 2022 19:55:55 +0200, Andrew Lunn <andrew@lunn.ch> 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
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.
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 <clement.leger@bootlin.com> + */ + +#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", ®)) { + 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 <clement.leger@bootlin.com>"); 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 <clement.leger@bootlin.com> + */ + +#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; +};
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 <laurent.gonzales@non.se.com> Suggested-by: Jean-Pierre Geslin <jean-pierre.geslin@non.se.com> Suggested-by: Phil Edworthy <phil.edworthy@renesas.com> Signed-off-by: Clément Léger <clement.leger@bootlin.com> --- 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