Re: [PATCH 5/5] net: mtip: The L2 switch driver for imx287

From: Lukasz Majewski
Date: Tue Mar 25 2025 - 09:41:30 EST

Next message: Aaro Mäkinen: "[PATCH] hfsplus: Return null terminated string from hfsplus_uni2asc()"
Previous message: Andy Yan: "[PATCH v2 6/6] Revert "ARM: dts: rockchip: drop grf reference from rk3036 hdmi""
In reply to: Krzysztof Kozlowski: "Re: [PATCH 5/5] net: mtip: The L2 switch driver for imx287"
Next in thread: Krzysztof Kozlowski: "Re: [PATCH 5/5] net: mtip: The L2 switch driver for imx287"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Krzysztof,

> On 25/03/2025 12:57, Lukasz Majewski wrote:
> > 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 interchangeable 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.
> >
> > Signed-off-by: Lukasz Majewski <lukma@xxxxxxx>
> > ---
> > drivers/net/ethernet/freescale/Kconfig | 1 +
> > drivers/net/ethernet/freescale/Makefile | 1 +
> > drivers/net/ethernet/freescale/mtipsw/Kconfig | 10 +
> > .../net/ethernet/freescale/mtipsw/Makefile | 6 +
> > .../net/ethernet/freescale/mtipsw/mtipl2sw.c | 2108
> > +++++++++++++++++ .../net/ethernet/freescale/mtipsw/mtipl2sw.h |
> > 784 ++++++ .../ethernet/freescale/mtipsw/mtipl2sw_br.c | 113 +
> > .../ethernet/freescale/mtipsw/mtipl2sw_mgnt.c | 434 ++++
> > 8 files changed, 3457 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/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..5cc9b758bad4
> > --- /dev/null
> > +++ b/drivers/net/ethernet/freescale/mtipsw/Kconfig
> > @@ -0,0 +1,10 @@
> > +# 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
>
> Missing compile test

Could you be more specific?

I'm compiling and testing this driver for the last week... (6.6 LTS +
net-next).

>
> > + help
> > + This enables support for the MoreThan IP L2 switch
> > on i.MX
> > + SoCs (e.g. iMX28, vf610).
>
> Wrong indentation. Look at other Kconfig files.

The indentation from Kconfig from FEC:

<tab>help
<tab><2 spaces>FOO BAR....

also causes the checkpatch on net-next to generated WARNING.

>
> > diff --git a/drivers/net/ethernet/freescale/mtipsw/Makefile
> > b/drivers/net/ethernet/freescale/mtipsw/Makefile new file mode
> > 100644 index 000000000000..e87e06d6870a
> > --- /dev/null
> > +++ b/drivers/net/ethernet/freescale/mtipsw/Makefile
> > @@ -0,0 +1,6 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# Makefile for the L2 switch from MTIP embedded in NXP SoCs
>
> Drop, obvious.

Ok.

>
>
> ...
>
> > +
> > +static int mtip_parse_of(struct switch_enet_private *fep,
> > + struct device_node *np)
> > +{
> > + struct device_node *port, *p;
> > + unsigned int port_num;
> > + int ret;
> > +
> > + p = of_find_node_by_name(np, "ethernet-ports");
> > +
> > + for_each_available_child_of_node(p, port) {
> > + if (of_property_read_u32(port, "reg", &port_num))
> > + continue;
> > +
> > + fep->n_ports = port_num;
> > + of_get_mac_address(port, &fep->mac[port_num -
> > 1][0]); +
> > + ret = of_property_read_string(port, "label",
> > +
> > &fep->ndev_name[port_num - 1]);
> > + if (ret < 0) {
> > + pr_err("%s: Cannot get ethernet port name
> > (%d)!\n",
> > + __func__, ret);
> > + goto of_get_err;
>
> Just use scoped loop.

I've used for_each_available_child_of_node(p, port) {} to iterate
through children.

Is it wrong?

>
> > + }
> > +
> > + ret = of_get_phy_mode(port,
> > &fep->phy_interface[port_num - 1]);
> > + if (ret < 0) {
> > + pr_err("%s: Cannot get PHY mode (%d)!\n",
> > __func__,
> > + ret);
>
> No, drivers do not use pr_xxx. From where did you get this code?

There have been attempts to upstream this driver since 2020...
The original code is from - v4.4 for vf610 and 2.6.35 for imx287.

It has been then subsequently updated/rewritten for:

- 4.19-cip
- 5.12 (two versions for switchdev and DSA)
- 6.6 LTS
- net-next.

The pr_err() were probably added by me as replacement for
"printk(KERN_ERR". I can change them to dev_* or netdev_*. This shall
not be a problem.

>
> > + goto of_get_err;
> > + }
> > +
> > + fep->phy_np[port_num - 1] = of_parse_phandle(port,
> > +
> > "phy-handle", 0);
> > + }
> > +
> > + of_get_err:
> > + of_node_put(p);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtip_sw_learning(void *arg)
> > +{
> > + struct switch_enet_private *fep = arg;
> > +
> > + while (!kthread_should_stop()) {
> > + set_current_state(TASK_INTERRUPTIBLE);
> > + /* check learning record valid */
> > + mtip_atable_dynamicms_learn_migration(fep,
> > fep->curr_time,
> > + NULL, NULL);
> > + schedule_timeout(HZ / 100);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void mtip_mii_unregister(struct switch_enet_private *fep)
> > +{
> > + mdiobus_unregister(fep->mii_bus);
> > + mdiobus_free(fep->mii_bus);
> > +}
> > +
> > +static const struct fec_devinfo fec_imx28_l2switch_info = {
> > + .quirks = FEC_QUIRK_BUG_CAPTURE | FEC_QUIRK_SINGLE_MDIO,
> > +};
> > +
> > +static struct platform_device_id pdev_id = {
> > + .name = "imx28-l2switch",
> > + .driver_data = (kernel_ulong_t)&fec_imx28_l2switch_info,
> > +};
> > +
> > +static int __init mtip_sw_probe(struct platform_device *pdev)
> > +{
> > + struct device_node *np = pdev->dev.of_node;
> > + struct switch_enet_private *fep;
> > + struct fec_devinfo *dev_info;
> > + struct switch_t *fecp;
> > + struct resource *r;
> > + int err, ret;
> > + u32 rev;
> > +
> > + pr_info("Ethernet Switch Version %s\n", VERSION);
>
> Drivers use dev, not pr. Anyway drop. Drivers do not have versions and
> should be silent on success.

As I've written above - there are several "versions" on this particular
driver. Hence the information.

I would opt for dev_info() for backwards compatibility.

>
> > +
> > + fep = kzalloc(sizeof(*fep), GFP_KERNEL);
>
> Why not devm? See last comment here (at the end).

As I've written above - no problem to change it.

>
> > + if (!fep)
> > + return -ENOMEM;
> > +
> > + pdev->id_entry = &pdev_id;
> > +
> > + dev_info = (struct fec_devinfo
> > *)pdev->id_entry->driver_data;
> > + if (dev_info)
> > + fep->quirks = dev_info->quirks;
> > +
> > + fep->pdev = pdev;
> > + platform_set_drvdata(pdev, fep);
> > +
> > + r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + fep->enet_addr = devm_ioremap_resource(&pdev->dev, r);
>
> Use proper wrapper.

I've followed following pattern:
https://elixir.bootlin.com/linux/v6.13.7/source/lib/devres.c#L180

>
> > + if (IS_ERR(fep->enet_addr)) {
> > + ret = PTR_ERR(fep->enet_addr);
> > + goto failed_ioremap;
> > + }
> > +
> > + fep->irq = platform_get_irq(pdev, 0);
> > +
> > + ret = mtip_parse_of(fep, np);
> > + if (ret < 0) {
> > + pr_err("%s: OF parse error (%d)!\n", __func__,
> > ret);
> > + goto failed_parse_of;
> > + }
> > +
> > + /* Create an Ethernet device instance.
> > + * The switch lookup address memory starts 0x800FC000
> > + */
> > + fep->hwp_enet = fep->enet_addr;
> > + fecp = (struct switch_t *)(fep->enet_addr +
> > ENET_SWI_PHYS_ADDR_OFFSET); +
> > + fep->hwp = fecp;
> > + fep->hwentry = (struct mtip_addr_table_t *)
> > + ((unsigned long)fecp + MCF_ESW_LOOKUP_MEM_OFFSET);
> > +
> > + rev = readl(&fecp->ESW_REVISION);
> > + pr_info("Ethernet Switch HW rev 0x%x:0x%x\n",
> > + MCF_MTIP_REVISION_CUSTOMER_REVISION(rev),
> > + MCF_MTIP_REVISION_CORE_REVISION(rev));
>
> Drop

Ok.

>
> > +
> > + fep->reg_phy = devm_regulator_get(&pdev->dev, "phy");
> > + if (!IS_ERR(fep->reg_phy)) {
> > + ret = regulator_enable(fep->reg_phy);
> > + if (ret) {
> > + dev_err(&pdev->dev,
> > + "Failed to enable phy regulator:
> > %d\n", ret);
> > + goto failed_regulator;
> > + }
> > + } else {
> > + if (PTR_ERR(fep->reg_phy) == -EPROBE_DEFER) {
> > + ret = -EPROBE_DEFER;
> > + goto failed_regulator;
> > + }
> > + fep->reg_phy = NULL;
>
> I don't understand this code. Do you want to re-implement
> get_optional? But why?

Here the get_optional() shall be used.

>
> > + }
> > +
> > + fep->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> > + if (IS_ERR(fep->clk_ipg))
> > + fep->clk_ipg = NULL;
>
> Why?

I will update the code.

>
> > +
> > + fep->clk_ahb = devm_clk_get(&pdev->dev, "ahb");
> > + if (IS_ERR(fep->clk_ahb))
> > + fep->clk_ahb = NULL;
> > +
> > + fep->clk_enet_out = devm_clk_get(&pdev->dev, "enet_out");
> > + if (IS_ERR(fep->clk_enet_out))
> > + fep->clk_enet_out = NULL;
> > +

devm_clk_get_optional() shall be used for 'enet_out'.

> > + ret = clk_prepare_enable(fep->clk_ipg);
> > + if (ret) {
> > + pr_err("%s: clock ipg cannot be enabled\n",
> > __func__);
> > + goto failed_clk_ipg;
> > + }
> > +
> > + ret = clk_prepare_enable(fep->clk_ahb);
> > + if (ret) {
> > + pr_err("%s: clock ahb cannot be enabled\n",
> > __func__);
> > + goto failed_clk_ahb;
> > + }
> > +
> > + ret = clk_prepare_enable(fep->clk_enet_out);
> > + if (ret)
> > + pr_err("%s: clock clk_enet_out cannot be
> > enabled\n", __func__); +
> > + spin_lock_init(&fep->learn_lock);
> > +
> > + ret = mtip_reset_phy(pdev);
> > + if (ret < 0) {
> > + pr_err("%s: GPIO PHY RST error (%d)!\n", __func__,
> > ret);
> > + goto get_phy_mode_err;
> > + }
> > +
> > + ret = request_irq(fep->irq, mtip_interrupt, 0,
> > "mtip_l2sw", fep);
> > + if (ret) {
> > + pr_err("MTIP_L2: Could not alloc IRQ(%d)!\n",
> > fep->irq);
> > + goto request_irq_err;
> > + }
> > +
> > + spin_lock_init(&fep->hw_lock);
> > + spin_lock_init(&fep->mii_lock);
> > +
> > + ret = mtip_register_notifiers(fep);
> > + if (ret)
> > + goto clean_unregister_netdev;
> > +
> > + ret = mtip_ndev_init(fep);
> > + if (ret) {
> > + pr_err("%s: Failed to create virtual ndev (%d)\n",
> > + __func__, ret);
> > + goto mtip_ndev_init_err;
> > + }
> > +
> > + err = mtip_switch_dma_init(fep);
> > + if (err) {
> > + pr_err("%s: ethernet switch init fail (%d)!\n",
> > __func__, err);
> > + goto mtip_switch_dma_init_err;
> > + }
> > +
> > + err = mtip_mii_init(fep, pdev);
> > + if (err)
> > + pr_err("%s: Cannot init phy bus (%d)!\n",
> > __func__, err); +
> > + /* setup timer for learning aging function */
> > + timer_setup(&fep->timer_aging, mtip_aging_timer, 0);
> > + mod_timer(&fep->timer_aging,
> > + jiffies +
> > msecs_to_jiffies(LEARNING_AGING_INTERVAL)); +
> > + fep->task = kthread_run(mtip_sw_learning, fep,
> > "mtip_l2sw_learning");
> > + if (IS_ERR(fep->task)) {
> > + ret = PTR_ERR(fep->task);
> > + pr_err("%s: learning kthread_run error (%d)!\n",
> > __func__, ret);
> > + goto fail_task_learning;
> > + }
> > +
> > + /* setup MII interface for external switch ports*/
> > + mtip_enet_init(fep, 1);
> > + mtip_enet_init(fep, 2);
> > +
> > + return 0;
> > +
> > + fail_task_learning:
> > + mtip_mii_unregister(fep);
> > + del_timer(&fep->timer_aging);
> > + mtip_switch_dma_init_err:
> > + mtip_ndev_cleanup(fep);
> > + mtip_ndev_init_err:
> > + mtip_unregister_notifiers(fep);
> > + clean_unregister_netdev:
> > + free_irq(fep->irq, NULL);
> > + request_irq_err:
> > + platform_set_drvdata(pdev, NULL);
> > + get_phy_mode_err:
> > + if (fep->clk_enet_out)
> > + clk_disable_unprepare(fep->clk_enet_out);
> > + clk_disable_unprepare(fep->clk_ahb);
> > + failed_clk_ahb:
> > + clk_disable_unprepare(fep->clk_ipg);
> > + failed_clk_ipg:
> > + if (fep->reg_phy) {
> > + regulator_disable(fep->reg_phy);
> > + devm_regulator_put(fep->reg_phy);
> > + }
> > + failed_regulator:
> > + failed_parse_of:
> > + devm_ioremap_release(&pdev->dev, r);
> > + failed_ioremap:
> > + kfree(fep);
> > + return err;
> > +}
> > +
> > +static void mtip_sw_remove(struct platform_device *pdev)
> > +{
> > + struct switch_enet_private *fep =
> > platform_get_drvdata(pdev); +
> > + mtip_unregister_notifiers(fep);
> > + mtip_ndev_cleanup(fep);
> > +
> > + mtip_mii_remove(fep);
> > +
> > + kthread_stop(fep->task);
> > + del_timer(&fep->timer_aging);
> > + platform_set_drvdata(pdev, NULL);
> > +
> > + kfree(fep);
> > +}
> > +
> > +static const struct of_device_id mtipl2_of_match[] = {
> > + { .compatible = "fsl,imx287-mtip-switch", },
> > + { /* sentinel */ }
> > +};
> > +
> > +static struct platform_driver mtipl2plat_driver = {
> > + .probe = mtip_sw_probe,
> > + .remove = mtip_sw_remove,
> > + .driver = {
> > + .name = "mtipl2sw",
> > + .owner = THIS_MODULE,
>
> Oh no, please do not send us 10-12 year old code. This is long, looong
> time gone. If you copied this pattern,

I've stated the chronology of this particular driver. It is working
with recent kernels.

> then for sure you copied all
> other issues we fixed during last 10 years, so basically you ask us to
> re-review the same code we already fixed.

I cannot agree with this statement.

Even better, the code has passed net-next's checkpatch.pl without
complaining about the "THIS_MODULE" statement.

I will remove it.

>
> Best regards,
> Krzysztof

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@xxxxxxx

Attachment: pgp5FOIHa_fcZ.pgp
Description: OpenPGP digital signature

Next message: Aaro Mäkinen: "[PATCH] hfsplus: Return null terminated string from hfsplus_uni2asc()"
Previous message: Andy Yan: "[PATCH v2 6/6] Revert "ARM: dts: rockchip: drop grf reference from rk3036 hdmi""
In reply to: Krzysztof Kozlowski: "Re: [PATCH 5/5] net: mtip: The L2 switch driver for imx287"
Next in thread: Krzysztof Kozlowski: "Re: [PATCH 5/5] net: mtip: The L2 switch driver for imx287"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]