Re: [net-next v4 4/5] net: mtip: The L2 switch driver for imx287

Stefan Wahren
2025-04-11T14:43:25Z

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.
>>> +
>>> +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.
>>> +
>>> +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.
> +
> +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
> 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).

Are you sure about the randomize fallback behavior of of_get_mac_address() ?

I tought you still need to call eth_random_addr().

>>> +
>>> +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.