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

Tue Mar 25 07:36:31 PDT 2025

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 at 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