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

[Lukasz Majewski <lukma@denx.de>]

[-- Attachment #1: Type: text/plain, Size: 15121 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]