| Message ID | 20250331103116.2223899-1-lukma@denx.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: mtip: Add support for MTIP imx287 L2 switch driver | expand |
On Mon, 31 Mar 2025 12:31:12 +0200 Lukasz Majewski wrote: > This patch series adds support for More Than IP's L2 switch driver embedded > in some NXP's SoCs. This one has been tested on imx287, but is also available > in the vf610. Lukasz, please post with RFC in the subject tags during the merge window. As I already said net-next is closed.
Hi Jakub, > On Mon, 31 Mar 2025 12:31:12 +0200 Lukasz Majewski wrote: > > This patch series adds support for More Than IP's L2 switch driver > > embedded in some NXP's SoCs. This one has been tested on imx287, > > but is also available in the vf610. > > Lukasz, please post with RFC in the subject tags during the merge > window. As I already said net-next is closed. Ach, Ok. I hope, that we will finish all reviews till 07.04, so v4 would be the final version. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Lukasz, Am 31.03.25 um 21:11 schrieb Lukasz Majewski: > Hi Jakub, > >> On Mon, 31 Mar 2025 12:31:12 +0200 Lukasz Majewski wrote: >>> This patch series adds support for More Than IP's L2 switch driver >>> embedded in some NXP's SoCs. This one has been tested on imx287, >>> but is also available in the vf610. >> Lukasz, please post with RFC in the subject tags during the merge >> window. As I already said net-next is closed. > Ach, Ok. > > I hope, that we will finish all reviews till 07.04, so v4 would be the > final version. well i appreciate your work on this driver, but this is not how it's going to work. No version of this patch series had a review time of a week. It's about quality not about deadlines. Regards
Hi Stefan, > Hi Lukasz, > > Am 31.03.25 um 21:11 schrieb Lukasz Majewski: > > Hi Jakub, > > > >> On Mon, 31 Mar 2025 12:31:12 +0200 Lukasz Majewski wrote: > >>> This patch series adds support for More Than IP's L2 switch driver > >>> embedded in some NXP's SoCs. This one has been tested on imx287, > >>> but is also available in the vf610. > >> Lukasz, please post with RFC in the subject tags during the merge > >> window. As I already said net-next is closed. > > Ach, Ok. > > > > I hope, that we will finish all reviews till 07.04, so v4 would be > > the final version. > well i appreciate your work on this driver, Do you maintain some vf610 or imx28 devices, which would like to use L2 switch? > but this is not how it's > going to work. No version of this patch series had a review time of a > week. I just pointed out that patches would not be pulled (or anything else would be done with them) until the merge window is re-opened. > It's about quality not about deadlines. > :-D > Regards Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Am 31.03.25 um 23:42 schrieb Lukasz Majewski: > Hi Stefan, > >> Hi Lukasz, >> >> Am 31.03.25 um 21:11 schrieb Lukasz Majewski: >>> Hi Jakub, >>> >>>> On Mon, 31 Mar 2025 12:31:12 +0200 Lukasz Majewski wrote: >>>>> This patch series adds support for More Than IP's L2 switch driver >>>>> embedded in some NXP's SoCs. This one has been tested on imx287, >>>>> but is also available in the vf610. >>>> Lukasz, please post with RFC in the subject tags during the merge >>>> window. As I already said net-next is closed. >>> Ach, Ok. >>> >>> I hope, that we will finish all reviews till 07.04, so v4 would be >>> the final version. >> well i appreciate your work on this driver, > Do you maintain some vf610 or imx28 devices, which would like to use L2 > switch? I've have been working with some i.MX28 devices and still "maintain" one i.MX28 platform of my employer. But it doesn't have a L2 switch. Many years again, I tried to mainline a driver for the on-chip regulator of i.MX28 SoC with cpufreq support at the end. https://github.com/lategoodbye/linux-mxs-power Regards
> +static void read_atable(struct switch_enet_private *fep, int index, > + unsigned long *read_lo, unsigned long *read_hi) > +{ > + unsigned long atable_base = (unsigned long)fep->hwentry; > + > + *read_lo = readl((const void *)atable_base + (index << 3)); > + *read_hi = readl((const void *)atable_base + (index << 3) + 4); > +} > + > +static void write_atable(struct switch_enet_private *fep, int index, > + unsigned long write_lo, unsigned long write_hi) > +{ > + unsigned long atable_base = (unsigned long)fep->hwentry; > + > + writel(write_lo, (void *)atable_base + (index << 3)); > + writel(write_hi, (void *)atable_base + (index << 3) + 4); > +} It would be nice to have the mtip_ prefix on all functions. > +static int mtip_open(struct net_device *dev) > +{ > + struct mtip_ndev_priv *priv = netdev_priv(dev); > + struct switch_enet_private *fep = priv->fep; > + int ret, port_idx = priv->portnum - 1; > + > + if (fep->usage_count == 0) { > + clk_enable(fep->clk_ipg); > + netif_napi_add(dev, &fep->napi, mtip_rx_napi); > + > + ret = mtip_alloc_buffers(dev); > + if (ret) > + return ret; nitpick: You might want to turn the clock off before returning the error. > + } > + > + fep->link[port_idx] = 0; > + > + /* Probe and connect to PHY when open the interface, if already > + * NOT done in the switch driver probe (or when the device is > + * re-opened). > + */ > + ret = mtip_mii_probe(dev); > + if (ret) { > + mtip_free_buffers(dev); I've not checked. Does this do the opposite of netif_napi_add()? > +static void mtip_set_multicast_list(struct net_device *dev) > +{ > + unsigned int i, bit, data, crc; > + > + if (dev->flags & IFF_PROMISC) { > + dev_info(&dev->dev, "%s: IFF_PROMISC\n", __func__); You can save one level of indentation with a return here. > + } else { > + if (dev->flags & IFF_ALLMULTI) { > + dev_info(&dev->dev, "%s: IFF_ALLMULTI\n", __func__); and other level here. > + } else { > + struct netdev_hw_addr *ha; > + u_char *addrs; > + > + netdev_for_each_mc_addr(ha, dev) { > + addrs = ha->addr; > + /* Only support group multicast for now */ > + if (!(*addrs & 1)) > + continue; You could pull there CRC caluclation out into a helper. You might also want to search the tree and see if it exists somewhere else. > + > + /* calculate crc32 value of mac address */ > + crc = 0xffffffff; > + > + for (i = 0; i < 6; i++) { Is 6 the lengh of a MAC address? There is a #define for that. > + data = addrs[i]; > + for (bit = 0; bit < 8; > + bit++, data >>= 1) { > + crc = (crc >> 1) ^ > + (((crc ^ data) & 1) ? > + CRC32_POLY : 0); > + } > + } > + } > + } > + } > +} > + > +struct switch_enet_private *mtip_netdev_get_priv(const struct net_device *ndev) > +{ > + if (ndev->netdev_ops == &mtip_netdev_ops) > + return netdev_priv(ndev); > + > + return NULL; > +} > + > +static int __init mtip_switch_dma_init(struct switch_enet_private *fep) > +{ > + struct cbd_t *bdp, *cbd_base; > + int ret, i; > + > + /* Check mask of the streaming and coherent API */ > + ret = dma_set_mask_and_coherent(&fep->pdev->dev, DMA_BIT_MASK(32)); > + if (ret < 0) { > + dev_warn(&fep->pdev->dev, "No suitable DMA available\n"); Can you recover from this? Or should it be dev_err()? More later... Andrew
> +struct switch_enet_private *mtip_netdev_get_priv(const struct net_device *ndev) > +{ > + if (ndev->netdev_ops == &mtip_netdev_ops) > + return netdev_priv(ndev); > + > + return NULL; > +} > +static bool mtip_port_dev_check(const struct net_device *ndev) > +{ > + if (!mtip_netdev_get_priv(ndev)) > + return false; > + > + return true; > +} This appears to be the only use of mtip_netdev_get_priv(). It does not care what priv actually is. In my previous review i said i think you can simply this. I still think that is true. Andrew
> +struct switch_enet_private *mtip_netdev_get_priv(const struct net_device *ndev) > +{ > + if (ndev->netdev_ops == &mtip_netdev_ops) > + return netdev_priv(ndev); > + > + return NULL; > +} > +static bool mtip_port_dev_check(const struct net_device *ndev) > +{ > + if (!mtip_netdev_get_priv(ndev)) > + return false; > + > + return true; > +} > + Rearranging the code a bit to make my point.... mtip_port_dev_check() tells us if this ndev is one of the ports of this switch. > +/* netdev notifier */ > +static int mtip_netdevice_event(struct notifier_block *unused, > + unsigned long event, void *ptr) > +{ > + struct net_device *ndev = netdev_notifier_info_to_dev(ptr); > + struct netdev_notifier_changeupper_info *info; > + int ret = NOTIFY_DONE; > + > + if (!mtip_port_dev_check(ndev)) > + return NOTIFY_DONE; We have received a notification about some interface. This filters out all but the switches interfaces. > + > + switch (event) { > + case NETDEV_CHANGEUPPER: > + info = ptr; CHANGERUPPER is that a netdev has been added or removed from a bridge, or some other sort of master device, e.g. a bond. > + > + if (netif_is_bridge_master(info->upper_dev)) { > + if (info->linking) > + ret = mtip_ndev_port_link(ndev, > + info->upper_dev); Call mtip_ndev_port_link() has been added to some bridge. > +static int mtip_ndev_port_link(struct net_device *ndev, > + struct net_device *br_ndev) > +{ > + struct mtip_ndev_priv *priv = netdev_priv(ndev); > + struct switch_enet_private *fep = priv->fep; > + > + dev_dbg(&ndev->dev, "%s: ndev: %s br: %s fep: 0x%x\n", > + __func__, ndev->name, br_ndev->name, (unsigned int)fep); > + > + /* Check if MTIP switch is already enabled */ > + if (!fep->br_offload) { > + if (!priv->master_dev) > + priv->master_dev = br_ndev; > + > + fep->br_offload = 1; > + mtip_switch_dis_port_separation(fep); > + mtip_clear_atable(fep); > + } So lets consider ip link add br0 type bridge ip link add br1 type bridge ip link set dev lan1 master br0 We create two bridges, and add the first port to one of the bridges. fep->br_offload should be False priv->master_dev should be NULL. So fep->br_offload is set to 1, priv->master_dev is set to br0 and the separation between the ports is removed. It seems like the hardware will now be bridging packets between the two interfaces, despite lan2 not being a member of any bridge.... Now ip link set dev lan2 master br1 I make the second port a member of some other bridge. fep->br_offload is True, so nothing happens. This is why i said this code needs expanding. If you look at other switch drivers, you will see each port keeps track of what bridge it has been joined to. There is then logic which iterates over the ports, finds which ports are members of the same bridge, and enables packets to flow between those ports. With only two ports, you can make some simplifications, but you should only disable the separation once both ports are the member of the same bridge. Andrew
This patch series adds support for More Than IP's L2 switch driver embedded in some NXP's SoCs. This one has been tested on imx287, but is also available in the vf610. In the past there has been performed some attempts to upstream this driver: 1. The 4.19-cip based one [1] 2. DSA based one for 5.12 [2] - i.e. the switch itself was treat as a DSA switch with NO tag appended. 3. The extension for FEC driver for 5.12 [3] - the trick here was to fully reuse FEC when the in-HW switching is disabled. When bridge offloading is enabled, the driver uses already configured MAC and PHY to also configure PHY. All three approaches were not accepted as eligible for upstreaming. The driver from this series has floowing features: 1. It is fully separated from fec_main - i.e. can be used interchangeable with it. To be more specific - one can build them as modules and if required switch between them when e.g. bridge offloading is required. To be more specific: - Use FEC_MAIN: When one needs support for two ETH ports with separate uDMAs used for both and bridging can be realized in SW. - Use MTIPL2SW: When it is enough to support two ports with only uDMA0 attached to switch and bridging shall be offloaded to HW. 2. This driver uses MTIP's L2 switch internal VLAN feature to provide port separation at boot time. Port separation is disabled when bridging is required. 3. Example usage: Configuration: ip link set lan0 up; sleep 1; ip link set lan1 up; sleep 1; ip link add name br0 type bridge; ip link set br0 up; sleep 1; ip link set lan0 master br0; ip link set lan1 master br0; bridge link; ip addr add 192.168.2.17/24 dev br0; ping -c 5 192.168.2.222 Removal: ip link set br0 down; ip link delete br0 type bridge; ip link set dev lan1 down ip link set dev lan0 down 4. Limitations: - Driver enables and disables switch operation with learning and ageing. - Missing is the advanced configuration (e.g. adding entries to FBD). This is on purpose, as up till now we didn't had consensus about how the driver shall be added to Linux. Links: [1] - https://github.com/lmajewski/linux-imx28-l2switch/commits/master [2] - https://github.com/lmajewski/linux-imx28-l2switch/tree/imx28-v5.12-L2-upstream-RFC_v1 [3] - https://source.denx.de/linux/linux-imx28-l2switch/-/tree/imx28-v5.12-L2-upstream-switchdev-RFC_v1?ref_type=heads Lukasz Majewski (4): dt-bindings: net: Add MTIP L2 switch description ARM: dts: nxp: mxs: Adjust the imx28.dtsi L2 switch description ARM: dts: nxp: mxs: Adjust XEA board's DTS to support L2 switch net: mtip: The L2 switch driver for imx287 .../bindings/net/nxp,imx287-mtip-switch.yaml | 154 ++ MAINTAINERS | 7 + arch/arm/boot/dts/nxp/mxs/imx28-xea.dts | 54 + arch/arm/boot/dts/nxp/mxs/imx28.dtsi | 8 +- drivers/net/ethernet/freescale/Kconfig | 1 + drivers/net/ethernet/freescale/Makefile | 1 + drivers/net/ethernet/freescale/mtipsw/Kconfig | 13 + .../net/ethernet/freescale/mtipsw/Makefile | 3 + .../net/ethernet/freescale/mtipsw/mtipl2sw.c | 1964 +++++++++++++++++ .../net/ethernet/freescale/mtipsw/mtipl2sw.h | 780 +++++++ .../ethernet/freescale/mtipsw/mtipl2sw_br.c | 113 + .../ethernet/freescale/mtipsw/mtipl2sw_mgnt.c | 449 ++++ 12 files changed, 3545 insertions(+), 2 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.yaml create mode 100644 drivers/net/ethernet/freescale/mtipsw/Kconfig create mode 100644 drivers/net/ethernet/freescale/mtipsw/Makefile create mode 100644 drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c create mode 100644 drivers/net/ethernet/freescale/mtipsw/mtipl2sw.h create mode 100644 drivers/net/ethernet/freescale/mtipsw/mtipl2sw_br.c create mode 100644 drivers/net/ethernet/freescale/mtipsw/mtipl2sw_mgnt.c