[PATCH v2 4/4] net: mtip: The L2 switch driver for imx287

[Lukasz Majewski]

Hi Krzysztof,

> On 28/03/2025 14:35, Lukasz Majewski wrote:
> > +
> > +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 = {  
> 
> That's const.
> 
> > +	.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;
> > +	int ret;
> > +
> > +	fep = devm_kzalloc(&pdev->dev, sizeof(*fep), GFP_KERNEL);
> > +	if (!fep)
> > +		return -ENOMEM;
> > +
> > +	pdev->id_entry = &pdev_id;  
> 
> Hm? This is some odd pattern. You are supposed to use OF table and get
> matched by it, not populate some custom/odd handling of platform
> tables.

I've removed it and fully utilized struct of_device_id. I will just use
the OF approach without utilizing platform device.

I think that it is better to just switch to OF.

> 
> > +
> > +	dev_info = (struct fec_devinfo
> > *)pdev->id_entry->driver_data;  
> 
> I did not notice it before, but that's a no - you cannot drop the
> cast. Driver data is always const.

The platform device ID approach has been dropped and completely
replaced with OF.

> 
> > +	if (dev_info)
> > +		fep->quirks = dev_info->quirks;
> > +
> > +	fep->pdev = pdev;
> > +	platform_set_drvdata(pdev, fep);
> > +
> > +	fep->enet_addr = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(fep->enet_addr))
> > +		return PTR_ERR(fep->enet_addr);
> > +
> > +	fep->irq = platform_get_irq(pdev, 0);
> > +	if (fep->irq < 0)
> > +		return fep->irq;
> > +
> > +	ret = mtip_parse_of(fep, np);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "%s: OF parse error (%d)!\n",
> > __func__,
> > +			ret);
> > +		return ret;
> > +	}
> > +
> > +	/* Create an Ethernet device instance.
> > +	 * The switch lookup address memory starts at 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);
> > +
> > +	ret = devm_regulator_get_enable_optional(&pdev->dev,
> > "phy");
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, ret,
> > +				     "Unable to get and enable
> > 'phy'\n"); +
> > +	fep->clk_ipg = devm_clk_get_enabled(&pdev->dev, "ipg");
> > +	if (IS_ERR(fep->clk_ipg))
> > +		return dev_err_probe(&pdev->dev,
> > PTR_ERR(fep->clk_ipg),
> > +				     "Unable to acquire 'ipg'
> > clock\n"); +
> > +	fep->clk_ahb = devm_clk_get_enabled(&pdev->dev, "ahb");
> > +	if (IS_ERR(fep->clk_ahb))
> > +		return dev_err_probe(&pdev->dev,
> > PTR_ERR(fep->clk_ahb),
> > +				     "Unable to acquire 'ahb'
> > clock\n"); +
> > +	fep->clk_enet_out =
> > devm_clk_get_optional_enabled(&pdev->dev,
> > +
> > "enet_out");
> > +	if (IS_ERR(fep->clk_enet_out))
> > +		return dev_err_probe(&pdev->dev,
> > PTR_ERR(fep->clk_enet_out),
> > +				     "Unable to acquire 'enet_out'
> > clock\n"); +
> > +	spin_lock_init(&fep->learn_lock);
> > +	spin_lock_init(&fep->hw_lock);
> > +	spin_lock_init(&fep->mii_lock);
> > +
> > +	ret = devm_request_irq(&pdev->dev, fep->irq,
> > mtip_interrupt, 0,
> > +			       "mtip_l2sw", fep);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, fep->irq,
> > +				     "Could not alloc IRQ\n");
> > +
> > +	ret = mtip_register_notifiers(fep);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = mtip_ndev_init(fep);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "%s: Failed to create virtual
> > ndev (%d)\n",
> > +			__func__, ret);
> > +		goto ndev_init_err;
> > +	}
> > +
> > +	ret = mtip_switch_dma_init(fep);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "%s: ethernet switch init fail
> > (%d)!\n",
> > +			__func__, ret);
> > +		goto dma_init_err;
> > +	}
> > +
> > +	ret = mtip_mii_init(fep, pdev);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "%s: Cannot init phy bus
> > (%d)!\n", __func__,
> > +			ret);
> > +		goto mii_init_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);
> > +		dev_err(&pdev->dev, "%s: learning kthread_run
> > error (%d)!\n",
> > +			__func__, ret);
> > +		goto task_learning_err;
> > +	}
> > +
> > +	/* setup MII interface for external switch ports*/
> > +	mtip_enet_init(fep, 1);
> > +	mtip_enet_init(fep, 2);
> > +
> > +	return 0;
> > +
> > + task_learning_err:
> > +	del_timer(&fep->timer_aging);
> > +	mtip_mii_unregister(fep);
> > + mii_init_err:
> > + dma_init_err:
> > +	mtip_ndev_cleanup(fep);
> > + ndev_init_err:
> > +	mtip_unregister_notifiers(fep);
> > +
> > +	return ret;
> > +}
> > +
> > +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 = "nxp,imx287-mtip-switch", },
> > +	{ /* sentinel */ }
> > +};  
> 
> Missing module device table.

Ok. I will add it.

> 
> > +
> > +static struct platform_driver mtipl2plat_driver = {
> > +	.driver         = {
> > +		.name   = "mtipl2sw",
> > +		.of_match_table = mtipl2_of_match,
> > +		.suppress_bind_attrs = true,
> > +	},
> > +	.probe          = mtip_sw_probe,
> > +	.remove_new     = mtip_sw_remove,
> > +};
> > +
> > +module_platform_driver(mtipl2plat_driver);
> > +MODULE_AUTHOR("Lukasz Majewski <lukma at denx.de>");
> > +MODULE_DESCRIPTION("Driver for MTIP L2 on SOC switch");
> > +MODULE_VERSION(VERSION);  
> 
> What is the point of paralell versioning with the kernel? Are you
> going to keep this updated or - just like in other cases - it will
> stay always theh same. Look for example at net/bridge/br.c or some
> other files - they are always the same even if driver changed
> significantly.
> 
> BTW, this would be 1.0, not 1.4. Your out of tree versioning does not
> matter.

I'm going to drop it totally. The "versioning" was only required when
switching between major LTS kernels.

I'd be more than happy to just use kernel SHA1, when this driver is
pulled.


> 
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:mtipl2sw");  
> 
> You should not need MODULE_ALIAS() in normal cases. If you need it,
> usually it means your device ID table is wrong (e.g. misses either
> entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
> for incomplete ID table.
> 

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 at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20250331/a19dc68d/attachment.sig>