Re: [PATCH net-next v13 4/9] net: dsa: lan9645x: add basic dsa driver for LAN9645X

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] lan9645x_probe() gets every register target (gcb, qs, chip_top,
  rew, sys, hsio, dev0..dev8, qsys, ana) through…
- [Low] The binding microchip,lan96455s-switch.yaml requires reg and reg-
  names (17 windows) on the switch node, but the driver never reads its…
- [Low] The v4 changelog says lan9645x_tag_npi_setup() rejects cascaded
  setups, but it only catches a LAN9645x that sits downstream.
- [Low] In lan9645x_reset_switch(), the decision to skip memory init
  depends on lan_rd(lan9645x, SYS_RESET_CFG).

--

net: dsa: lan9645x: add basic dsa driver for LAN9645X

This adds a basic DSA driver for the Microchip LAN9645x switch. It covers
switch init, NPI and CPU port module setup, front port setup, and phylink
MAC configuration for the internal CuPHYs and the RGMII ports 7-8. Register
targets come from a parent device as named regmaps, and the parent MFD
driver is planned for a later series.

> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> new file mode 100644
> index 0000000000000..c7de3836e6967
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> @@ -0,0 +1,456 @@

[ ... ]

> +static int lan9645x_tag_npi_setup(struct dsa_switch *ds)
> +{
> +	struct dsa_port *dp, *first_cpu_dp = NULL;
> +	struct lan9645x *lan9645x = ds->priv;
> +	struct phylink_config c = {};
> +
> +	dsa_switch_for_each_user_port(dp, ds) {
> +		if (dp->cpu_dp->ds != ds) {
> +			dev_err(ds->dev,
> +				"NPI port on a remote switch is not supported\n");
> +			return -EINVAL;
> +		}

[Severity: Low]
Does this check catch every cascaded setup? The v4 changelog says
"tag_npi_setup: reject cascaded setups". The loop only looks at this
switch's user ports and only rejects when their cpu_dp is on another
switch, so it only covers a LAN9645x that sits downstream.

Take a LAN9645x at the top of a dsa,member cluster that owns the NPI port
and has a link port to a switch from another family. Every local user port
then has a local cpu_dp, so the check passes. The DSA link port and the
remote user ports behind it are never checked.

The DSA core doesn't seem to reject the mixed tree either.
dsa_switch_setup_tag_protocol() in net/dsa/dsa.c goes straight to connect
when the protocol matches the tree default:

	if (tag_ops->proto == dst->default_proto)
		goto connect;

lan9645x_port_setup() then sets up the link port like a normal front
port. lan9645x_setup() zeroes the source mask for every port:

	for (int p = 0; p < lan9645x->num_phys_ports; p++) {
		lan_wr(0, lan9645x, ANA_PGID(PGID_SRC + p));

so frames arriving on the link port are not forwarded. On transmit,
lan9645x_xmit() sets IFH_DSTS = BIT(dp->index) for a remote user port,
which selects the local port with the same index.

The tree would probe successfully, but cascaded traffic would be dropped
or misdirected with nothing logged. Could setup fail instead when a
dsa_switch_for_each_port() loop finds a port where dsa_port_is_dsa(dp) is
true?

[ ... ]

> +static int lan9645x_reset_switch(struct lan9645x *lan9645x)
> +{
> +	u32 val = 0;
> +	int err;
> +
> +	/* The parent must reset the whole chip before instantiating its
> +	 * children, so the switch core is disabled when we get here. The
> +	 * sequence below must not run more than once: initializing the
> +	 * memories on a running core corrupts pointers inside the queue
> +	 * system, which eventually fills up and stops passing traffic. If the
> +	 * core is already enabled the chip was not reset, so skip it.
> +	 */
> +	if (SYS_RESET_CFG_CORE_ENA_GET(lan_rd(lan9645x, SYS_RESET_CFG)))
> +		return 0;
> +
> +	lan_wr(SYS_RESET_CFG_CORE_ENA_SET(0), lan9645x, SYS_RESET_CFG);
> +	lan_wr(SYS_RAM_INIT_RAM_INIT_SET(1), lan9645x, SYS_RAM_INIT);

[Severity: Low]
What happens here if the SYS_RESET_CFG read fails? After the WARN_ON_ONCE,
lan_rd() in lan9645x_main.h returns 0:

	u32 addr, val = 0;
	...
	WARN_ON_ONCE(regmap_read(lan_tgt2rmap(lan9645x, t, tinst), addr, &val));

	return val;

That reads the same as CORE_ENA=0, so the RAM_INIT sequence could run on a
core that is already running. The comment above says that corrupts the
queue system.

In the same way, lan_wr() returns void, so a failed SYS_RAM_INIT write is
ignored. CORE_ENA could then be set on memories that were never
initialized.

This needs both a chip the parent did not reset and a bus error on exactly
this access. There is no in-tree regmap provider yet, so it is unclear
whether these regmap calls can fail in practice.

Should the regmap_read() result be checked before deciding whether to skip
or run the memory init?

[ ... ]

> +static int lan9645x_request_target_regmaps(struct lan9645x *lan9645x)
> +{
> +	const char *resource_name;
> +	struct regmap *tgt_map;
> +
> +	for (int i = 0; i < NUM_TARGETS; i++) {
> +		resource_name = lan9645x_resource_names[i];
> +		if (!resource_name)
> +			continue;
> +
> +		tgt_map = dev_get_regmap(lan9645x->dev->parent, resource_name);
> +		if (IS_ERR_OR_NULL(tgt_map)) {
> +			dev_err(lan9645x->dev, "Failed to get regmap=%s\n",
> +				resource_name);
> +			return -ENODEV;
> +		}

[Severity: Low]
Is there any in-tree configuration where this lookup can succeed? Nothing
under drivers/mfd registers regmaps with these names (gcb, qs, chip_top,
rew, sys, hsio, dev0..dev8, qsys, ana) for a lan9645x or lan96455 parent.

That means lan9645x_probe() returns -ENODEV every time, including for a DT
node placed under simple-bus. None of the setup, NPI, phylink or port code
in this patch can run in-tree until the MFD parent lands.

The commit message does say "The MFD driver will come in a later series",
so this is mostly a question for the maintainers: should the driver be
merged before anything in-tree can bind it?

[Severity: Low]
Should the driver also be able to use the switch node's own register
windows?

Documentation/devicetree/bindings/net/dsa/microchip,lan96455s-switch.yaml
requires reg and reg-names on the switch node, with 17 windows. The driver
never reads those resources itself, and only asks dev->parent for named
regmaps.

For comparison, ocelot_regmap_from_resource() tries the device's own
IORESOURCE_MEM first and only then falls back to the parent.

If the plan is for the future MFD parent to build the named regmaps from
the child's reg and reg-names, as with VSC7512, then this matches, since
the binding's reg-names match lan9645x_resource_names[]. Otherwise, would a
fallback to the node's own resources be useful here?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929-dsa_lan9645x_switch_driver_base-v13-0-827c2d3617f2%40microchip.com