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

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

[Alex Elder]

From: Daniel Thompson <daniel@riscstar.com>

QCA8081 appears in embedded board designs paired with GPIO controlled
regulators for its power rails. Add logic to allow these regulators to
be turned on during a probe.

In order to avoid the complexity of tracking state for suspend with and
without WoL we take a tremendously simple "always-on" approach to
regulator management, essentially relying on BMCR_PDOWN to conserve
power when the phy device exists.

Signed-off-by: Daniel Thompson <daniel@riscstar.com>
Co-developed-by: Alex Elder <elder@riscstar.com>
Signed-off-by: Alex Elder <elder@riscstar.com>
---
 drivers/net/phy/qcom/qca808x.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

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
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0+
 
 #include <linux/phy.h>
+#include <linux/regulator/consumer.h>
 #include <linux/module.h>
 
 #include "qcom.h"
@@ -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"
+	};
+	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;
+	}
+
+	if (count)
+		fsleep(phydev->mdio.reset_assert_delay);
+#endif
+	return 0;
+}
+
 static int qca808x_probe(struct phy_device *phydev)
 {
 	struct device *dev = &phydev->mdio.dev;
 	struct qca808x_priv *priv;
+	int ret;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
+	ret = qca808x_power_on(phydev);
+	if (ret)
+		return ret;
+
 	priv->led_polarity_mode = QCA808X_PHY_LED_UNSET;
 
 	phydev->priv = priv;
-- 
2.53.0

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

[Andrew Lunn]

On Wed, Oct 14, 2026 at 12:22:59PM -0500, Alex Elder wrote:
> From: Daniel Thompson <daniel@riscstar.com>
> 
> QCA8081 appears in embedded board designs paired with GPIO controlled
> regulators for its power rails. Add logic to allow these regulators to
> be turned on during a probe.
> 
> In order to avoid the complexity of tracking state for suspend with and
> without WoL we take a tremendously simple "always-on" approach to
> regulator management, essentially relying on BMCR_PDOWN to conserve
> power when the phy device exists.
> 
> Signed-off-by: Daniel Thompson <daniel@riscstar.com>
> Co-developed-by: Alex Elder <elder@riscstar.com>
> Signed-off-by: Alex Elder <elder@riscstar.com>
> ---
>  drivers/net/phy/qcom/qca808x.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> 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
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0+
>  
>  #include <linux/phy.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/module.h>
>  
>  #include "qcom.h"
> @@ -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

Does this need to be conditional? It seems like
devm_regulator_get_enable_optional() should return ENODEV, which you
already handle.

Making code this like conditional makes build testing less useful,
more combinations which need testing.

Another option is

	if (IS_ENABLED(CONFIG_OF)) {


	}

That at least compiles the code, and then the optimizier throws it
away when it sees  if (0) { }

     Andrew

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

[Alex Elder]

On 10/14/26 12:36 PM, Andrew Lunn wrote:
> On Wed, Oct 14, 2026 at 12:22:59PM -0500, Alex Elder wrote:
>> From: Daniel Thompson <daniel@riscstar.com>
>>
>> QCA8081 appears in embedded board designs paired with GPIO controlled
>> regulators for its power rails. Add logic to allow these regulators to
>> be turned on during a probe.
>>
>> In order to avoid the complexity of tracking state for suspend with and
>> without WoL we take a tremendously simple "always-on" approach to
>> regulator management, essentially relying on BMCR_PDOWN to conserve
>> power when the phy device exists.
>>
>> Signed-off-by: Daniel Thompson <daniel@riscstar.com>
>> Co-developed-by: Alex Elder <elder@riscstar.com>
>> Signed-off-by: Alex Elder <elder@riscstar.com>
>> ---
>>   drivers/net/phy/qcom/qca808x.c | 30 ++++++++++++++++++++++++++++++
>>   1 file changed, 30 insertions(+)
>>
>> 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
>> @@ -1,6 +1,7 @@
>>   // SPDX-License-Identifier: GPL-2.0+
>>   
>>   #include <linux/phy.h>
>> +#include <linux/regulator/consumer.h>
>>   #include <linux/module.h>
>>   
>>   #include "qcom.h"
>> @@ -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
> 
> Does this need to be conditional? It seems like
> devm_regulator_get_enable_optional() should return ENODEV, which you
> already handle.
> 
> Making code this like conditional makes build testing less useful,
> more combinations which need testing.

This is a really good point.

> Another option is
> 
> 	if (IS_ENABLED(CONFIG_OF)) {
> 
> 
> 	}
> 
> That at least compiles the code, and then the optimizier throws it
> away when it sees  if (0) { }

Yes I like doing it this way better.  I'll modify it
to be similar to this for v2, which I'll plan to send
tomorrow.

Thanks a lot.

					-Alex

> 
>       Andrew

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

[Daniel Thompson]

On Wed, Oct 14, 2026 at 03:41:20PM -0500, Alex Elder wrote:
> On 10/14/26 12:36 PM, Andrew Lunn wrote:
> > On Wed, Oct 14, 2026 at 12:22:59PM -0500, Alex Elder wrote:
> > > From: Daniel Thompson <daniel@riscstar.com>
> > >
> > > QCA8081 appears in embedded board designs paired with GPIO controlled
> > > regulators for its power rails. Add logic to allow these regulators to
> > > be turned on during a probe.
> > >
> > > In order to avoid the complexity of tracking state for suspend with and
> > > without WoL we take a tremendously simple "always-on" approach to
> > > regulator management, essentially relying on BMCR_PDOWN to conserve
> > > power when the phy device exists.
> > >
> > > Signed-off-by: Daniel Thompson <daniel@riscstar.com>
> > > Co-developed-by: Alex Elder <elder@riscstar.com>
> > > Signed-off-by: Alex Elder <elder@riscstar.com>
> > > ---
> > >   drivers/net/phy/qcom/qca808x.c | 30 ++++++++++++++++++++++++++++++
> > >   1 file changed, 30 insertions(+)
> > >
> > > 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
> > > @@ -1,6 +1,7 @@
> > >   // SPDX-License-Identifier: GPL-2.0+
> > >   #include <linux/phy.h>
> > > +#include <linux/regulator/consumer.h>
> > >   #include <linux/module.h>
> > >   #include "qcom.h"
> > > @@ -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
> >
> > Does this need to be conditional? It seems like
> > devm_regulator_get_enable_optional() should return ENODEV, which you
> > already handle.
> >
> > Making code this like conditional makes build testing less useful,
> > more combinations which need testing.
>
> This is a really good point.
>
> > Another option is
> >
> > 	if (IS_ENABLED(CONFIG_OF)) {
> >
> >
> > 	}
> >
> > That at least compiles the code, and then the optimizier throws it
> > away when it sees  if (0) { }
>
> Yes I like doing it this way better.  I'll modify it
> to be similar to this for v2, which I'll plan to send
> tomorrow.

Reading things back, I think Andrew's first suggestion of having no
conditional might be better.

devm_regulator_get_enable_optional() is log-silent on failure. I suspect
the reason this code was conditional is that an early version used
devm_regulator_get_enable() which is not log-silent when it falls back
to a dummy reg.

I suspect I was more influenced by the log-silence than the marginal
code size cost on a CONFIG_REGULATOR && !CONFIG_OF system.


Daniel.

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

[netdev-bot+sashiko]

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