Message-ID: <32d93a90-3601-4094-8054-2737a57acbc7@kernel.org>
Date: Tue, 25 Mar 2025 13:11:07 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Lukasz Majewski <lukma@...x.de>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
 Sascha Hauer <s.hauer@...gutronix.de>, Paolo Abeni <pabeni@...hat.com>,
 Jakub Kicinski <kuba@...nel.org>, Eric Dumazet <edumazet@...gle.com>,
 davem@...emloft.net, Andrew Lunn <andrew+netdev@...n.ch>
Cc: Pengutronix Kernel Team <kernel@...gutronix.de>,
 Fabio Estevam <festevam@...il.com>, devicetree@...r.kernel.org,
 imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, Richard Cochran <richardcochran@...il.com>,
 netdev@...r.kernel.org, Maxime Chevallier <maxime.chevallier@...tlin.com>
Subject: Re: [PATCH 5/5] net: mtip: The L2 switch driver for imx287

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@...x.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

Powered by blists - more mailing lists