| Message ID | 20250325115736.1732721-1-lukma@denx.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: mtip: Add support for MTIP imx287 L2 switch driver | expand |
On 25/03/2025 12:57, Lukasz Majewski wrote: > Add myself as a maintainer for this particular network driver. > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > --- > MAINTAINERS | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 5959513a7359..255edd825fa1 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -9270,6 +9270,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/fec,mtip-switch.yaml > +F: drivers/net/ethernet/freescale/mtipsw/* You need to re-order the patches, there are no such files yet, so this causes warnings. Best regards, 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@denx.de> > --- > 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 > + 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. > 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. ... > + > +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. > + } > + > + 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? > + 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. > + > + fep = kzalloc(sizeof(*fep), GFP_KERNEL); Why not devm? See last comment here (at the end). > + 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. > + 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 > + > + 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? > + } > + > + fep->clk_ipg = devm_clk_get(&pdev->dev, "ipg"); > + if (IS_ERR(fep->clk_ipg)) > + fep->clk_ipg = NULL; Why? > + > + 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; > + > + 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, 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. Best regards, Krzysztof
Hi Krzysztof, > On 25/03/2025 12:57, Lukasz Majewski wrote: > > Add myself as a maintainer for this particular network driver. > > > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > --- > > MAINTAINERS | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 5959513a7359..255edd825fa1 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -9270,6 +9270,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/fec,mtip-switch.yaml > > +F: drivers/net/ethernet/freescale/mtipsw/* > > You need to re-order the patches, there are no such files yet, so this > causes warnings. This shall be probably squashed with the patch adding mtipl2sw* files. > > 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@denx.de
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@denx.de> > > --- > > 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@denx.de
On 25/03/2025 14:28, Lukasz Majewski wrote: > 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@denx.de> >>> --- >>> 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). You miss dependency on compile test. > >> >>> + 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.... That's correct, but you do not use it :/ > > 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? No, it is not, just can be simpler with scoped variant. ... >> >>> + 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 which is correct example of that usage, but most drivers were converted to different usage. Probably cocci also has rule for that. >>> + >>> +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. That's not how upstreaming should work with explanation below. > >> 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. ^^^ this one. It does not matter if it works. You can send us working code with hungarian notation and the argument "it works" is not correct. It still will be hungarian notation. > > I cannot agree with this statement. > > Even better, the code has passed net-next's checkpatch.pl without > complaining about the "THIS_MODULE" statement. > It's not part of checkpatch. It's part of coccinelle, which - just like smatch and sparse - are supposed to report zero or almost zero issues on new drivers. Best regards, Krzysztof
On Tue, 25 Mar 2025 12:57:31 +0100, 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. > > 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 (5): > MAINTAINERS: Add myself as the MTIP L2 switch maintainer (IMX SoCs: > imx287) > dt-bindings: net: Add MTIP L2 switch description > (fec,mtip-switch.yaml) > arm: dts: Adjust the 'reg' range for imx287 L2 switch description > arm: dts: imx287: Provide description for MTIP L2 switch > net: mtip: The L2 switch driver for imx287 > > .../bindings/net/fec,mtip-switch.yaml | 160 ++ > MAINTAINERS | 7 + > arch/arm/boot/dts/nxp/mxs/imx28-xea.dts | 56 + > arch/arm/boot/dts/nxp/mxs/imx28.dtsi | 4 +- > 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 ++++ > 12 files changed, 3682 insertions(+), 2 deletions(-) > create mode 100644 Documentation/devicetree/bindings/net/fec,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 > > -- > 2.39.5 > > > My bot found new DTB warnings on the .dts files added or changed in this series. Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings are fixed by another series. Ultimately, it is up to the platform maintainer whether these warnings are acceptable or not. No need to reply unless the platform maintainer has comments. If you already ran DT checks and didn't see these error(s), then make sure dt-schema is up to date: pip3 install dtschema --upgrade New warnings running 'make CHECK_DTBS=y for arch/arm/boot/dts/nxp/' for 20250325115736.1732721-1-lukma@denx.de: arch/arm/boot/dts/nxp/mxs/imx28-xea.dtb: /ahb@80080000/switch@800f0000: failed to match any schema with compatible: ['fsl,imx287-mtip-switch'] arch/arm/boot/dts/nxp/imx/imx6q-tx6q-1110.dtb: /lvds1-panel: failed to match any schema with compatible: ['nlt,nl12880bc20-spwg-24']
> > > +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? config FEC tristate "FEC ethernet controller (of ColdFire and some i.MX CPUs)" depends on (M523x || M527x || M5272 || M528x || M520x || M532x || \ ARCH_MXC || ARCH_S32 || SOC_IMX28 || COMPILE_TEST) || COMPILE_TEST So that it also gets build on amd64, s390, powerpc etc. The code should cleanly build on all architectures. It if does not, it probably means the code is using the kernel abstractions wrong. Also, most developers build test on amd64, not arm, so if they are making tree wide changes, you want this driver to build on amd64 so such tree wide changes get build tested. > 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. With Ethernet switches, you need to look at the context. Is it something specific to one interface? The netdev_err(). If it is global to the whole switch, then dev_err(). > > > + 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. There is only one version in mainline, this version (maybe). Mainline does not care about other versions. Such version information is also useless, because once it is merged, it never changes. What you actually want to know is the kernel git hash, so you can find the exact sources. ethtool -I will return that, assuming your ethtool code is correct. > > > + 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. You can report this via ethtool -I. But i suggest you leave that for later patches. > > > + 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. This is the problem with trying to use old code. It needs more work than just making it compile. It needs to be brought up to HEAD of mainline standard, which often nearly ends in a re-write. > > > + 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. FYI: NULL is a valid clock. But do you actually want _optional()? This is the sort of thing a 10 year old codebase will be missing, and you need to re-write. You might also be able to use the clk _bulk_ API? Andrew
Hi Andrew, > > > > +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? > > config FEC > tristate "FEC ethernet controller (of ColdFire and some i.MX > CPUs)" depends on (M523x || M527x || M5272 || M528x || M520x || M532x > || \ ARCH_MXC || ARCH_S32 || SOC_IMX28 || COMPILE_TEST) > > || COMPILE_TEST ^^^^^^^^^^^^^^^^^^ - ach ... this "compile test" :-) (Not that I've posted the code not even compiling ... :-D) > > So that it also gets build on amd64, s390, powerpc etc. The code > should cleanly build on all architectures. It if does not, it probably > means the code is using the kernel abstractions wrong. Also, most > developers build test on amd64, not arm, so if they are making tree > wide changes, you want this driver to build on amd64 so such tree wide > changes get build tested. > > > 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. > > With Ethernet switches, you need to look at the context. Is it > something specific to one interface? The netdev_err(). If it is global > to the whole switch, then dev_err(). Ok. > > > > > + 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. > > There is only one version in mainline, this version (maybe). Mainline > does not care about other versions. Such version information is also > useless, because once it is merged, it never changes. What you > actually want to know is the kernel git hash, so you can find the > exact sources. ethtool -I will return that, assuming your ethtool code > is correct. Ok. I will. > > > > > + 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. > > You can report this via ethtool -I. But i suggest you leave that for > later patches. I will remove it. > > > > > + 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. > > This is the problem with trying to use old code. It needs more work > than just making it compile. It needs to be brought up to HEAD of > mainline standard, which often nearly ends in a re-write. But you cannot rewrite this code from scratch, as the IP block is not so well documented, and there maybe are some issues that you are not aware of. Moreover, this code is already in production use, and you don't want to be in situation when regression tests cannot be run. > > > > > + 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. > > FYI: NULL is a valid clock. But do you actually want _optional()? Yes, I will use _optional() and _bulk_ if applicable. > > This is the sort of thing a 10 year old codebase will be missing, and > you need to re-write. You might also be able to use the clk _bulk_ > API? The goal is to have this driver upstreamed (finally), so the starting point of further development would be in kernel. > > Andrew 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
On 25/03/2025 17:38, Lukasz Majewski wrote: >>>> >>>> I don't understand this code. Do you want to re-implement >>>> get_optional? But why? >>> >>> Here the get_optional() shall be used. >> >> This is the problem with trying to use old code. It needs more work >> than just making it compile. It needs to be brought up to HEAD of >> mainline standard, which often nearly ends in a re-write. > > But you cannot rewrite this code from scratch, as the IP block is not > so well documented, and there maybe are some issues that you are not > aware of. > > Moreover, this code is already in production use, and you don't want to > be in situation when regression tests cannot be run. This is a good reason to add it to staging, but not to mainline. Just because someone has somewhere products with poor code is not the reason to accept that poor code. Otherwise all the people and companies who upstream BEFORE would be quite disappointed. Why anyone would care to work on upstreaming BEFORE hardware release, if you can ship whatever to production and then ask mainline to pick up "because it is in production use". Best regards, Krzysztof
Hi Krzysztof, > On 25/03/2025 17:38, Lukasz Majewski wrote: > >>>> > >>>> I don't understand this code. Do you want to re-implement > >>>> get_optional? But why? > >>> > >>> Here the get_optional() shall be used. > >> > >> This is the problem with trying to use old code. It needs more work > >> than just making it compile. It needs to be brought up to HEAD of > >> mainline standard, which often nearly ends in a re-write. > > > > But you cannot rewrite this code from scratch, as the IP block is > > not so well documented, and there maybe are some issues that you > > are not aware of. > > > > Moreover, this code is already in production use, and you don't > > want to be in situation when regression tests cannot be run. > > This is a good reason to add it to staging, but not to mainline. Just > because someone has somewhere products with poor code is not the > reason to accept that poor code. I've tried to upstream this driver several times. Attempts were made for 4.19 and 5.12. The reason the code was not accepted was that conceptually the code had to be written in a different way (exact discussion is available [1]). What I've tried to say above - was that I need to have working device at any point of development. And, yes "upstream first" is a great policy, but imx287 based HW was in the kernel long time ago. > Otherwise all the people and > companies who upstream BEFORE would be quite disappointed. Why anyone > would care to work on upstreaming BEFORE hardware release, Yes, this shall be appreciated. > if you can > ship whatever to production and then ask mainline to pick up "because > it is in production use". Where I've stated this? My point is that for regression testing I prefer to gradually update the code and not start from scratch. I do appreciate your and Andrew's feedback and try to make the driver eligible for upstreaming. To sum up: ########## - Yes, I'm aware that this code needs some more adjustments/update - Yes, fsl,fec.yaml was the wrong file to use as a starting point - Yes, bindings are ABI and shall be done right (that was one of the reason the driver from 5.12 was not accepted). Links: [1] - https://lore.kernel.org/netdev/20210629140104.70a3da1a@ktm/T/ > > 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@denx.de
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 (5): MAINTAINERS: Add myself as the MTIP L2 switch maintainer (IMX SoCs: imx287) dt-bindings: net: Add MTIP L2 switch description (fec,mtip-switch.yaml) arm: dts: Adjust the 'reg' range for imx287 L2 switch description arm: dts: imx287: Provide description for MTIP L2 switch net: mtip: The L2 switch driver for imx287 .../bindings/net/fec,mtip-switch.yaml | 160 ++ MAINTAINERS | 7 + arch/arm/boot/dts/nxp/mxs/imx28-xea.dts | 56 + arch/arm/boot/dts/nxp/mxs/imx28.dtsi | 4 +- 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 ++++ 12 files changed, 3682 insertions(+), 2 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/fec,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