Re: [PATCH 02/20] net: xilinx: tsn: add TSN endpoint wrapper driver

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: xilinx: tsn: add TSN endpoint wrapper driver

Adds a thin wrapper platform driver for the Xilinx TSN Endpoint Ethernet
MAC IP node, along with a new Kconfig symbol and Makefile under
drivers/net/ethernet/xilinx/tsn/. The wrapper enables the six IP-wide
clocks and then populates the sub-nodes so each can bind to its own driver.

A few questions below, all fairly minor.

> diff --git a/drivers/net/ethernet/xilinx/tsn/Kconfig b/drivers/net/ethernet/xilinx/tsn/Kconfig
> new file mode 100644
> index 000000000000..45af4d3f10e6
> --- /dev/null
> +++ b/drivers/net/ethernet/xilinx/tsn/Kconfig
> @@ -0,0 +1,15 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# Xilinx TSN device configuration
> +#
> +
> +config XILINX_TSN
> +	tristate "Xilinx TSN Ethernet driver"
> +	depends on OF && HAS_IOMEM

[Severity: Low]
Should this depend on OF_ADDRESS rather than OF && HAS_IOMEM?

The only functional statement in tsn_ip_probe() is:

	return devm_of_platform_populate(dev);

and in include/linux/of_platform.h the real implementation is compiled
only under CONFIG_OF_ADDRESS; otherwise the inline stub is:

	static inline int devm_of_platform_populate(struct device *dev)
	{
		return -ENODEV;
	}

OF && HAS_IOMEM does not imply OF_ADDRESS, per drivers/of/Kconfig:

	config OF_ADDRESS
		def_bool y
		depends on !SPARC && (HAS_IOMEM || UML)

sparc selects OF and provides HAS_IOMEM, and NET_VENDOR_XILINX has no
architecture dependency, so XILINX_TSN is selectable there with
OF_ADDRESS=n.

In that configuration tsn_ip_probe() still runs devm_kzalloc(),
devm_clk_bulk_get() and clk_bulk_prepare_enable() for all six clocks and
registers the disable action, and then returns -ENODEV, so the driver can
never bind and no sub-node device is created. The clocks are unwound by
devres, so nothing leaks, but the whole driver is dead code in that
configuration.

This also stays as is through the end of the series, where the symbol is
still "depends on OF && HAS_IOMEM" plus "depends on XILINX_DMA", and
XILINX_DMA only has "depends on HAS_IOMEM".

> +	help
> +	  This driver supports the AMD/Xilinx Time-Sensitive Networking
> +	  (TSN) Endpoint Ethernet MAC IP. It provides the wrapper device
> +	  and the endpoint MAC that connects the IP to the host CPU. The
> +	  three-port switch is supported by NET_DSA_XILINX_TSN.

[Severity: Low]
The help text describes "the endpoint MAC that connects the IP to the host
CPU" and refers the reader to NET_DSA_XILINX_TSN, but at this commit the
symbol only builds xilinx_tsn_main.o (clock enable plus
devm_of_platform_populate()) and no NET_DSA_XILINX_TSN symbol exists in the
tree yet.

Both do appear later in the same series: the end-of-series Makefile has

	xilinx_tsn-y := xilinx_tsn_main.o xilinx_tsn_ep.o

with &xlnx_tsn_ep_driver registered from xilinx_tsn_main.c, and
NET_DSA_XILINX_TSN is added in drivers/net/dsa/xilinx/Kconfig.

Since the text only appears in prose and not in a select or depends on,
there is no build effect; would it be worth wording the help text so it
matches what this commit actually builds, or is the forward reference
intentional for the series?

> +
> +	  If unsure, say N.

[ ... ]

> diff --git a/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_main.c b/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_main.c
> new file mode 100644
> index 000000000000..1ef1b0c2ee6e
> --- /dev/null
> +++ b/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_main.c
> @@ -0,0 +1,105 @@

[ ... ]

> +static int tsn_ip_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct xlnx_tsn_ip *w;
> +	int ret;
> +
> +	w = devm_kzalloc(dev, sizeof(*w), GFP_KERNEL);
> +	if (!w)
> +		return -ENOMEM;
> +
> +	for (int i = 0; i < TSN_NUM_CLOCKS; i++)
> +		w->clks[i].id = tsn_clk_names[i];
> +
> +	ret = devm_clk_bulk_get(dev, TSN_NUM_CLOCKS, w->clks);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get clocks\n");
> +
> +	ret = clk_bulk_prepare_enable(TSN_NUM_CLOCKS, w->clks);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to enable clocks\n");
> +
> +	ret = devm_add_action_or_reset(dev, tsn_clk_bulk_disable, w);
> +	if (ret)
> +		return ret;
> +
> +	return devm_of_platform_populate(dev);
> +}

[Severity: Low]
Is there a consumer for xlnx,num-priorities anywhere?

Documentation/devicetree/bindings/net/xlnx,tsn-endpoint-ethernet-mac.yaml
lists it in the top-level required block for this same node:

	required:
	  - compatible
	  - reg
	  - clocks
	  - clock-names
	  - xlnx,num-priorities
	  - ranges

tsn_ip_probe() is the only driver binding
"xlnx,tsn-endpoint-ethernet-mac-3.0", and it only fills clks[].id and
calls devm_clk_bulk_get(), clk_bulk_prepare_enable() and
devm_of_platform_populate(). Grepping for num-priorities or
num_priorities at the end of the series finds hits only inside the YAML
itself plus unrelated fsl-mc and xe files.

So device tree authors must supply the property to pass dt_binding_check
while nothing reads it. Would it make sense to drop it from required
until a driver actually uses it?

[ ... ]