Re: [PATCH net-next] net: phy: qcom: qca808x: Add regulator management

[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] qca808x_power_on() in drivers/net/phy/qcom/qca808x.c adds four
  new DT supply properties: avdd18-supply, vdd-supply, vdd18-supply and…
- [Medium] The power-up sequence in qca808x_power_on()
  (drivers/net/phy/qcom/qca808x.c) is wrong in two ways.
- [Low] qca808x_power_on() counts a return value of 0 from
  devm_regulator_get_enable_optional() as "a regulator was found and…

--

net: phy: qcom: qca808x: Add regulator management

This commit makes qca808x_probe() enable up to four optional supplies
(avdd18, vdd, vdd18, vdd125). If any supply is found, it then sleeps for
reset_assert_delay. The supplies stay on for as long as the PHY device
exists.

> diff --git a/drivers/net/phy/qcom/qca808x.c b/drivers/net/phy/qcom/qca808x.c
> index bab26e4c61408..6e2bd753257d9 100644
> --- a/drivers/net/phy/qcom/qca808x.c
> +++ b/drivers/net/phy/qcom/qca808x.c

[ ... ]

> @@ -181,15 +182,44 @@ static void qca808x_fill_possible_interfaces(struct phy_device *phydev)
>  		__set_bit(PHY_INTERFACE_MODE_2500BASEX, possible);
>  }
>  
> +static int qca808x_power_on(struct phy_device *phydev)
> +{
> +#ifdef CONFIG_OF
> +	static const char * const regulator_names[] = {
> +		"avdd18", "vdd", "vdd18", "vdd125"
> +	};

[Severity: Medium]
These names add new DT properties on the PHY node: avdd18-supply,
vdd-supply, vdd18-supply and vdd125-supply. Are they documented in a
binding anywhere?

Documentation/devicetree/bindings/net/qca,qca808x.yaml declares only
compatible, and it ends with:

  unevaluatedProperties: false

The common ethernet-phy.yaml schema doesn't define any supply properties
either. Wouldn't any board DTS that uses these supplies fail dtbs_check
with "Unevaluated properties are not allowed"?

Could this series include a binding update that describes each rail?
That would also make clear what "vdd" means next to "vdd18" and
"avdd18".

> +	struct device *dev = &phydev->mdio.dev;
> +	u32 count = 0;
> +	int i, ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(regulator_names); i++) {
> +		ret = devm_regulator_get_enable_optional(dev, regulator_names[i]);
> +		if (!ret)
> +			count++;
> +		else if (ret != -ENODEV)
> +			return ret;
> +	}

[Severity: Low]
What happens here with CONFIG_OF=y and CONFIG_REGULATOR=n? In that case
the stub in include/linux/regulator/consumer.h always returns 0:

static inline int devm_regulator_get_enable_optional(struct device *dev,
						     const char *id)
{
	return 0;
}

All four lookups would count as present (count == 4), even though
nothing was powered on, and the fsleep() below would run for no reason.
QCA808X_PHY doesn't depend on or select REGULATOR, so this configuration
can be built.

Would it be better to guard on CONFIG_REGULATOR, or to stop treating
ret == 0 as proof that the supply exists?

> +
> +	if (count)
> +		fsleep(phydev->mdio.reset_assert_delay);

[Severity: Medium]
Is reset_assert_delay the right delay to use after powering up the
rails?

This value comes only from the optional reset-assert-us property.
ethernet-phy.yaml describes that property as "Delay after the reset was
asserted in microseconds". When the property is missing the value is 0,
so boards without reset-assert-us get no wait at all after the supplies
are enabled.

The ordering also looks off. mdio_device_register_reset() requests the
reset GPIO as GPIOD_OUT_LOW. So by the time drv->probe() runs, reset has
already been deasserted and reset_deassert_delay has already been spent:

phy_device_register()
    phy_device_reset(phydev, 0);
...
phy_probe()
    phy_device_reset(phydev, 0);
    phydev->drv->probe(phydev)
        qca808x_probe()
            qca808x_power_on()
                devm_regulator_get_enable_optional() ...
                fsleep(phydev->mdio.reset_assert_delay);
    phy_disable_interrupts(phydev);
    ...

This means the rails come up while reset is already released, and reset
is never pulsed once the supplies are stable. A later
phy_init_hw()->phy_device_reset(phydev, 0) does nothing, because
reset_state is already 0.

Can the MDIO accesses that phy_probe() makes right after drv->probe()
returns, such as phy_disable_interrupts() and the get_features path,
reach a PHY that isn't ready yet?

Existing QCA8081 users such as arch/arm64/boot/dts/qcom/lemans-evk.dts
have:

    reset-assert-us = <11000>;
    reset-deassert-us = <70000>;

So even on those boards, the PHY gets only the 11ms assert time after
power-up, not the 70ms post-deassert time. The commit message doesn't
explain why this delay was chosen.

> +#endif
> +	return 0;
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014172300.292324-1-elder%40riscstar.com