Messages in this thread |  | | Date | Fri, 11 Apr 2025 18:23:15 +0200 | | From | Lukasz Majewski <> | | Subject | Re: [net-next v4 4/5] net: mtip: The L2 switch driver for imx287 |
| |
Hi Stefan,
> Am 10.04.25 um 15:37 schrieb Lukasz Majewski: > > Hi Stefan, > > > >> Hi Lukasz, > >> > >> thanks for sending this to linux-arm-kernel > >> > >> Am 07.04.25 um 16:51 schrieb Lukasz Majewski: > >>> This patch series provides support for More Than IP L2 switch > >>> embedded in the imx287 SoC. > >>> > >>> This is a two port switch (placed between uDMA[01] and > >>> MAC-NET[01]), which can be used for offloading the network > >>> traffic. > >>> > >>> It can be used interchangeably with current FEC driver - to be > >>> more specific: one can use either of it, depending on the > >>> requirements. > >>> > >>> The biggest difference is the usage of DMA - when FEC is used, > >>> separate DMAs are available for each ENET-MAC block. > >>> However, with switch enabled - only the DMA0 is used to > >>> send/receive data to/form switch (and then switch sends them to > >>> respecitive ports). > >>> > >>> Signed-off-by: Lukasz Majewski<lukma@denx.de> > >>> --- > >>> Changes for v2: > >>> > >>> - Remove not needed comments > >>> - Restore udelay(10) for switch reset (such delay is explicitly > >>> specifed in the documentation > >>> - Add COMPILE_TEST > >>> - replace pr_* with dev_* > >>> - Use for_each_available_child_of_node_scoped() > >>> - Use devm_* function for memory allocation > >>> - Remove printing information about the HW and SW revision of the > >>> driver > >>> - Use devm_regulator_get_optional() > >>> - Change compatible prefix from 'fsl' to more up to date 'nxp' > >>> - Remove .owner = THIS_MODULE > >>> - Use devm_platform_ioremap_resource(pdev, 0); > >>> - Use devm_request_irq() > >>> - Use devm_regulator_get_enable_optional() > >>> - Replace clk_prepare_enable() and devm_clk_get() with single > >>> call to devm_clk_get_optional_enabled() > >>> - Cleanup error patch when function calls in probe fail > >>> - Refactor the mtip_reset_phy() to serve as mdio bus reset > >>> callback > >>> - Add myself as the MTIP L2 switch maintainer (squashed the > >>> separated commit) > >>> - More descriptive help paragraphs (> 4 lines) > >>> > >>> Changes for v3: > >>> - Remove 'bridge_offloading' module parameter (to bridge ports > >>> just after probe) > >>> - Remove forward references > >>> - Fix reverse christmas tree formatting in functions > >>> - Convert eligible comments to kernel doc format > >>> - Remove extra MAC address validation check at > >>> esw_mac_addr_static() > >>> - Remove mtip_print_link_status() and replace it with > >>> phy_print_status() > >>> - Avoid changing phy device state in the driver (instead use > >>> functions exported by the phy API) > >>> - Do not print extra information regarding PHY (which is printed > >>> by phylib) - e.g. net lan0: lan0: MTIP eth L2 switch > >>> 1e:ce:a5:0b:4c:12 > >>> - Remove VERSION from the driver - now we rely on the SHA1 in > >>> Linux mainline tree > >>> - Remove zeroing of the net device private area (shall be already > >>> done during allocation) > >>> - Refactor the code to remove mtip_ndev_setup() > >>> - Use -ENOMEM instead of -1 return code when allocation fails > >>> - Replace dev_info() with dev_dbg() to reduce number of > >>> information print on normal operation > >>> - Return ret instead of 0 from mtip_ndev_init() > >>> - Remove fep->mii_timeout flag from the driver > >>> - Remove not used stop_gpr_* fields in mtip_devinfo struct > >>> - Remove platform_device_id description for mtipl2sw driver > >>> - Add MODULE_DEVICE_TABLE() for mtip_of_match > >>> - Remove MODULE_ALIAS() > >>> > >>> Changes for v4: > >>> - Rename imx287 with imx28 (as the former is not used in kernel > >>> anymore) > >>> - Reorder the place where ENET interface is initialized - without > >>> this change the enet_out clock has default (25 MHz) value, which > >>> causes issues during reset (RMII's 50 MHz is required for proper > >>> PHY reset). > >>> - Use PAUR instead of PAUR register to program MAC address > >>> - Replace eth_mac_addr() with eth_hw_addr_set() > >>> - Write to HW the randomly generated MAC address (if required) > >>> - Adjust the reset code > >>> - s/read_atable/mtip_read_atable/g and > >>> s/write_atable/mtip_write_atable/g > >>> - Add clk_disable() and netif_napi_del() when errors occur during > >>> mtip_open() - refactor the error handling path. > >>> - Refactor the mtip_set_multicast_list() to write (now) correct > >>> values to ENET-FEC registers. > >>> - Replace dev_warn() with dev_err() > >>> - Use GPIO_ACTIVE_LOW to indicate polarity in DTS > >>> - Refactor code to check if network device is the switch device > >>> - Remove mtip_port_dev_check() > >>> - Refactor mtip_ndev_port_link() avoid starting HW offloading for > >>> bridge when MTIP ports are parts of two distinct bridges > >>> - Replace del_timer() with timer_delete_sync() > >>> --- > >>> MAINTAINERS | 7 + > >>> 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 | 1970 > >>> +++++++++++++++++ .../net/ethernet/freescale/mtipsw/mtipl2sw.h | > >>> 782 +++++++ .../ethernet/freescale/mtipsw/mtipl2sw_br.c | 122 + > >>> .../ethernet/freescale/mtipsw/mtipl2sw_mgnt.c | 449 ++++ > >>> 9 files changed, 3348 insertions(+) > >>> 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 > >>> > >>> diff --git a/MAINTAINERS b/MAINTAINERS > >>> index 4c5c2e2c1278..9c5626c2b3b7 100644 > >>> --- a/MAINTAINERS > >>> +++ b/MAINTAINERS > >>> @@ -9455,6 +9455,13 @@ S: Maintained > >>> F: Documentation/devicetree/bindings/i2c/i2c-mpc.yaml > >>> F: drivers/i2c/busses/i2c-mpc.c > >>> > >>> +FREESCALE MTIP ETHERNET SWITCH DRIVER > >>> +M: Lukasz Majewski<lukma@denx.de> > >>> +L: netdev@vger.kernel.org > >>> +S: Maintained > >>> +F: > >>> Documentation/devicetree/bindings/net/nxp,imx28-mtip-switch.yaml > >>> +F: drivers/net/ethernet/freescale/mtipsw/* + > >>> FREESCALE QORIQ DPAA ETHERNET DRIVER > >>> M: Madalin Bucur<madalin.bucur@nxp.com> > >>> L: netdev@vger.kernel.org > >>> diff --git a/drivers/net/ethernet/freescale/Kconfig > >>> b/drivers/net/ethernet/freescale/Kconfig index > >>> a2d7300925a8..056a11c3a74e 100644 --- > >>> a/drivers/net/ethernet/freescale/Kconfig +++ > >>> b/drivers/net/ethernet/freescale/Kconfig @@ -60,6 +60,7 @@ config > >>> FEC_MPC52xx_MDIO > >>> > >>> source "drivers/net/ethernet/freescale/fs_enet/Kconfig" > >>> source "drivers/net/ethernet/freescale/fman/Kconfig" > >>> +source "drivers/net/ethernet/freescale/mtipsw/Kconfig" > >>> > >>> config FSL_PQ_MDIO > >>> tristate "Freescale PQ MDIO" diff --git > >>> a/drivers/net/ethernet/freescale/Makefile > >>> b/drivers/net/ethernet/freescale/Makefile index > >>> de7b31842233..0e6cacb0948a 100644 --- > >>> a/drivers/net/ethernet/freescale/Makefile +++ > >>> b/drivers/net/ethernet/freescale/Makefile @@ -25,3 +25,4 @@ > >>> obj-$(CONFIG_FSL_DPAA_ETH) += dpaa/ obj-$(CONFIG_FSL_DPAA2_ETH) += > >>> dpaa2/ obj-y += enetc/ +obj-y += mtipsw/ diff --git > >>> a/drivers/net/ethernet/freescale/mtipsw/Kconfig > >>> b/drivers/net/ethernet/freescale/mtipsw/Kconfig new file mode > >>> 100644 index 000000000000..450ff734a321 --- /dev/null +++ > >>> b/drivers/net/ethernet/freescale/mtipsw/Kconfig @@ -0,0 +1,13 @@ > >>> +# SPDX-License-Identifier: GPL-2.0-only +config FEC_MTIP_L2SW + > >>> tristate "MoreThanIP L2 switch support to FEC driver" > >>> + depends on OF > >>> + depends on NET_SWITCHDEV > >>> + depends on BRIDGE > >>> + depends on ARCH_MXS || ARCH_MXC || COMPILE_TEST > >>> + help > >>> + This enables support for the MoreThan IP L2 switch on > >>> i.MX > >>> + SoCs (e.g. iMX28, vf610). It offloads bridging to this > >>> IP block's > >> This is confusing. The Kconfig and most of the code looks prepared > >> for other platforms than i.MX28, but there is only a i.MX28 OF > >> compatible. > > I've took the approach to upstream the driver in several steps. > > The first step is this patch set - add the code for a single > > platform (imx28). > > > > And Yes, I also have on my desk another board with soc having this > > IP block (vf610). > > > > However, I will not start any other upstream work until patches from > > this "step" are not pulled. > > > > (To follow "one things at a time" principle) > > > >> I don't like that Kconfig pretent something, which is not > >> true. > > If you prefer I can remove 'depends on ARCH_MXC' and the vf610 > > SoC... (only to add it afterwards). > In case you want to do "one thing at a time", please remove this.
+1
> >>> + > >>> +static void mtip_config_switch(struct switch_enet_private *fep) > >>> +{ > >>> + struct switch_t *fecp = fep->hwp; > >>> + > >>> + esw_mac_addr_static(fep); > >>> + > >>> + writel(0, &fecp->ESW_BKLR); > >>> + > >>> + /* Do NOT disable learning */ > >>> + mtip_port_learning_config(fep, 0, 0, 0); > >>> + mtip_port_learning_config(fep, 1, 0, 0); > >>> + mtip_port_learning_config(fep, 2, 0, 0); > >> Looks like the last 2 parameter are always 0? > > Those functions are defined in mtipl2sw_mgnt.c file. > > > > I've followed the way legacy (i.e. vendor) driver has defined them. > > In this particular case the last '0' is to not enable interrupt for > > learning. > This wasn't my concern. The question was "why do we need a parameter > if it's always the same?". But you answered this further below, so > i'm fine.
Ok.
> >>> + > >>> +static irqreturn_t mtip_interrupt(int irq, void *ptr_fep) > >>> +{ > >>> + struct switch_enet_private *fep = ptr_fep; > >>> + struct switch_t *fecp = fep->hwp; > >>> + irqreturn_t ret = IRQ_NONE; > >>> + u32 int_events, int_imask; > >>> + > >>> + /* Get the interrupt events that caused us to be here */ > >>> + do { > >>> + int_events = readl(&fecp->ESW_ISR); > >>> + writel(int_events, &fecp->ESW_ISR); > >>> + > >>> + if (int_events & (MCF_ESW_ISR_RXF | > >>> MCF_ESW_ISR_TXF)) { > >>> + ret = IRQ_HANDLED; > >>> + /* Disable the RX interrupt */ > >>> + if (napi_schedule_prep(&fep->napi)) { > >>> + int_imask = > >>> readl(&fecp->ESW_IMR); > >>> + int_imask &= ~MCF_ESW_IMR_RXF; > >>> + writel(int_imask, > >>> &fecp->ESW_IMR); > >>> + __napi_schedule(&fep->napi); > >>> + } > >>> + } > >>> + } while (int_events); > >> This looks bad, in case of bad hardware / timing behavior this > >> interrupt handler will loop forever. > > The 'writel(int_events, &fecp->ESW_ISR);' > > > > clears the interrupts, so after reading them and clearing (by > > writing the same value), the int_events shall be 0. > > > > Also, during probe the IRQ mask for switch IRQ is written, so only > > expected (and served) interrupts are delivered. > This was not my point. A possible endless loop especially in a > interrupt handler should be avoided. The kernel shouldn't trust the > hardware and the driver should be fair to all the other interrupts > which might have occurred.
Ok.
> > + > > +static int mtip_rx_napi(struct napi_struct *napi, int budget) > > +{ > > + struct mtip_ndev_priv *priv = netdev_priv(napi->dev); > > + struct switch_enet_private *fep = priv->fep; > > + struct switch_t *fecp = fep->hwp; > > + int pkts, port; > > + > > + pkts = mtip_switch_rx(napi->dev, budget, &port); > > + if (!fep->br_offload && > > + (port == 1 || port == 2) && fep->ndev[port - 1]) > >> (port > 0) && fep->ndev[port - 1]) > > This needs to be kept as is - only when port is set to 1 or 2 (after > > reading the switch internal register) this shall be executed. > Oops, missed that > > (port also can be 0xFF or 0x3 -> then we shall send frame to both > > egress ports). > Maybe we should use defines for such special values
Port = 1 or port = 2 seems to be self explanatory.
Nonetheless, I've added MTIP_PORT_FORWARDING_INIT for 0xFF initial forwarding value.
> > This code is responsible for port separation when bridge HW > > offloading is disabled. > > > > > >>> + of_get_mac_address(port, &fep->mac[port_num - > >>> 1][0]); > >> This can fail > > I will add: > > > > ret = of_get_mac_address(port, &fep->mac[port_num - 1][0]); > > if (ret) > > dev_warn(dev, "of_get_mac_address(%pOF) failed\n", port); > > > > as it is also valid to not have the mac address defined in DTS (then > > some random based value is generated). > AFAIK this is a little bit more complex. It's possible that the MAC is > stored within a NVMEM and the driver isn't ready (EPROBE_DEFER). >
In my use case - always the local mac address is provided during production (to ethernet-port nodes).
The other possibility is no MAC address provided, that is why there is dev_warn().
> Are you sure about the randomize fallback behavior of > of_get_mac_address() ? > > I tought you still need to call eth_random_addr(). >
The "random" MAC address is generated in another place - namely mtip_setup_mac();
> > >>> + > >>> +int mtip_set_vlan_verification(struct switch_enet_private *fep, > >>> int port, > >>> + int vlan_domain_verify_en, > >>> + int vlan_discard_unknown_en) > >>> +{ > >>> + struct switch_t *fecp = fep->hwp; > >>> + > >>> + if (port < 0 || port > 2) { > >>> + dev_err(&fep->pdev->dev, "%s: Port (%d) not > >>> supported!\n", > >>> + __func__, port); > >>> + return -EINVAL; > >>> + } > >>> + > >>> + if (vlan_domain_verify_en == 1) { > >>> + if (port == 0) > >>> + writel(readl(&fecp->ESW_VLANV) | > >>> MCF_ESW_VLANV_VV0, > >>> + &fecp->ESW_VLANV); > >>> + else if (port == 1) > >>> + writel(readl(&fecp->ESW_VLANV) | > >>> MCF_ESW_VLANV_VV1, > >>> + &fecp->ESW_VLANV); > >>> + else if (port == 2) > >>> + writel(readl(&fecp->ESW_VLANV) | > >>> MCF_ESW_VLANV_VV2, > >>> + &fecp->ESW_VLANV); > >>> + } else if (vlan_domain_verify_en == 0) { > >>> + if (port == 0) > >>> + writel(readl(&fecp->ESW_VLANV) & > >>> ~MCF_ESW_VLANV_VV0, > >>> + &fecp->ESW_VLANV); > >>> + else if (port == 1) > >>> + writel(readl(&fecp->ESW_VLANV) & > >>> ~MCF_ESW_VLANV_VV1, > >>> + &fecp->ESW_VLANV); > >>> + else if (port == 2) > >>> + writel(readl(&fecp->ESW_VLANV) & > >>> ~MCF_ESW_VLANV_VV2, > >>> + &fecp->ESW_VLANV); > >>> + } > >>> + > >>> + if (vlan_discard_unknown_en == 1) { > >>> + if (port == 0) > >>> + writel(readl(&fecp->ESW_VLANV) | > >>> MCF_ESW_VLANV_DU0, > >>> + &fecp->ESW_VLANV); > >>> + else if (port == 1) > >>> + writel(readl(&fecp->ESW_VLANV) | > >>> MCF_ESW_VLANV_DU1, > >>> + &fecp->ESW_VLANV); > >>> + else if (port == 2) > >>> + writel(readl(&fecp->ESW_VLANV) | > >>> MCF_ESW_VLANV_DU2, > >>> + &fecp->ESW_VLANV); > >>> + } else if (vlan_discard_unknown_en == 0) { > >>> + if (port == 0) > >>> + writel(readl(&fecp->ESW_VLANV) & > >>> ~MCF_ESW_VLANV_DU0, > >>> + &fecp->ESW_VLANV); > >>> + else if (port == 1) > >>> + writel(readl(&fecp->ESW_VLANV) & > >>> ~MCF_ESW_VLANV_DU1, > >>> + &fecp->ESW_VLANV); > >>> + else if (port == 2) > >>> + writel(readl(&fecp->ESW_VLANV) & > >>> ~MCF_ESW_VLANV_DU2, > >>> + &fecp->ESW_VLANV); > >>> + } > >> This looks like a lot of copy & paste > > IMHO, the readability of the code is OK. > Actually the concern was about maintenance.
IMHO, (for me) the maintenance cost of this code is acceptable.
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 [unhandled content-type:application/pgp-signature] |  |