Re: [PATCH net-next v2 2/2] net: dsa: mxl-gsw1xx: reset the switch through reset-gpios

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The hardware reset only happens when CONFIG_RESET_CONTROLLER
  and CONFIG_RESET_GPIO are both enabled, and NET_DSA_MXL_GSW1XX does…
- [Medium] The new full-chip hardware reset in gsw1xx_probe()
  (drivers/net/dsa/lantiq/mxl-gsw1xx.c:750-763) contradicts the binding…
- [Low] gsw1xx_probe() (drivers/net/dsa/lantiq/mxl-gsw1xx.c:750-763)
  asserts the reset, sleeps 250 ms, deasserts and sleeps another 1 s…

--

net: dsa: mxl-gsw1xx: reset the switch through reset-gpios

The commit adds a hardware reset pulse to gsw1xx_probe() through the reset
controller API. The reset is held for 250 ms, followed by a 1 s wait before
the first register access. The goal is to clear the bootloader state that
leaves the internal PHYs of the GSW150 at shifted MDIO addresses on the
GL.iNet GL-SFT1200.

> diff --git a/drivers/net/dsa/lantiq/mxl-gsw1xx.c b/drivers/net/dsa/lantiq/mxl-gsw1xx.c
> index a26f78e1a44ce..94a7eff5e0077 100644
> --- a/drivers/net/dsa/lantiq/mxl-gsw1xx.c
> +++ b/drivers/net/dsa/lantiq/mxl-gsw1xx.c

[ ... ]

> @@ -737,6 +739,29 @@ static int gsw1xx_probe(struct mdio_device *mdiodev)
>  	if (!priv->gswip.hw_info)
>  		return -EINVAL;
>  
> +	/* The MDIO core handles reset-gpios only for PHYs. Reset the switch
> +	 * here, so it does not run with whatever the bootloader configured.
> +	 */
> +	reset = devm_reset_control_get_optional_exclusive(dev, NULL);
> +	if (IS_ERR(reset))
> +		return dev_err_probe(dev, PTR_ERR(reset),
> +				     "failed to get reset control\n");

[Severity: Medium]
If CONFIG_RESET_GPIO is disabled, does this lookup silently return NULL even
when the switch node has reset-gpios?

drivers/reset/core.c:__fwnode_reset_control_get() {
    ...
	if (!IS_ENABLED(CONFIG_RESET_GPIO))
		return optional ? NULL : ERR_PTR(ret);
    ...
}

With CONFIG_RESET_CONTROLLER=n, the optional stub also returns NULL. In
both cases the if (reset) block is skipped and nothing is logged. The switch
then keeps the bootloader state, and on the GL-SFT1200 port 0 would still
fail with "failed to connect to PHY: -ENODEV".

NET_DSA_MXL_GSW1XX in drivers/net/dsa/lantiq/Kconfig does not select, imply
or depend on RESET_CONTROLLER or RESET_GPIO. The reset-gpios entry added to
lantiq,gswip.yaml in this series ("dt-bindings: net: dsa: lantiq,gswip: add
reset-gpios") describes the property with no such condition. The commit
message mentions the requirement, but neither Kconfig nor the driver at
runtime does.

No Kconfig in the tree selects RESET_GPIO, and it sits inside the
RESET_CONTROLLER menuconfig, so selecting it is not straightforward.

Other DSA drivers (mv88e6xxx, rtl83xx, qca8k, ksz_common, mt7530-mdio,
lan9303, vitesse-vsc73xx) read reset-gpios with
devm_gpiod_get_optional(dev, "reset", ...), which only needs GPIOLIB.

Would a gpiod based lookup, or a gpiod fallback, make the documented
property work whatever the reset framework config is?

> +
> +	if (reset) {
> +		ret = reset_control_assert(reset);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "failed to assert reset\n");

[Severity: Medium]
Does this full chip reset conflict with the slew rate binding? For
maxlinear,slew-rate-txc and maxlinear,slew-rate-txd, lantiq,gswip.yaml
says:

  If not present, the configuration made by the switch bootloader is
  preserved.

gsw1xx_rmii_slew_rate() relies on that and skips the write when the
property is absent:

	ret = of_property_read_u32(np, prop, &rate);
	/* Optional property */
	if (ret == -EINVAL)
		return 0;

When a board adds reset-gpios, the bootloader's GSW1XX_SHELL_RGMII_SLEW_CFG
setup would be replaced by power-on defaults before the driver touches the
switch.

RGMII_SLEW_CFG_RX_2_5_V and RGMII_SLEW_CFG_TX_2_5_V are defined in
mxl-gsw1xx.h but never written anywhere in drivers/net/dsa/lantiq/. Suppose
a bootloader set them for 2.5 V RGMII signalling. Could the RGMII CPU link
be degraded or stop working after this reset?

The actual hardware impact depends on the GSW1xx reset defaults and straps.
The GSWIP_SWRES_R0 reset in gswip_setup() only covers the switch core
register block, so this pad state was not reset before this patch.

> +
> +		fsleep(GSW1XX_RESET_ASSERT_US);
> +
> +		ret = reset_control_deassert(reset);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "failed to deassert reset\n");
> +
> +		fsleep(GSW1XX_RESET_DEASSERT_US);
> +	}

[Severity: Low]
This runs before gswip_probe_common()->dsa_register_switch(), which can
return -EPROBE_DEFER when the conduit netdev is not available yet:

drivers/net/dsa/lantiq/lantiq_gswip_common.c:gswip_probe_common() {
    ...
	err = dsa_register_switch(priv->ds);
	if (err)
		return dev_err_probe(priv->dev, err, "dsa switch registration failed\n");
    ...
}

Does each deferred probe then repeat the full 250 ms + 1 s blocking pulse?

If the conduit Ethernet driver probes late or is built as a module, each
retry would add about 1.25 s to boot and reset the switch again.

> +
>  	priv->gswip.gswip = gsw1xx_regmap_init(priv, "switch",
>  					       GSW1XX_SWITCH_BASE, 0xfff);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108-mxl-gsw1xx-reset-v2-0-46c1da093893%40telaak.dev