* Re: [PATCH net-next v2 2/2] net: phy: qcom: qca808x: Add regulator management
2026-10-15 14:38 ` [PATCH net-next v2 2/2] net: phy: qcom: qca808x: Add regulator management Alex Elder
@ 2026-10-15 15:16 ` Maxime Chevallier
2026-10-15 15:40 ` Lorenzo Bianconi
2026-10-16 14:39 ` sashiko-bot
2 siblings, 0 replies; 11+ messages in thread
From: Maxime Chevallier @ 2026-10-15 15:16 UTC (permalink / raw)
To: Alex Elder, andrew+netdev, davem, edumazet, kuba, pabeni, robh,
krzk+dt, conor+dt, hkallweit1, linux
Cc: Daniel Thompson, ansuelsmth, donggeunyoo.kernel, mohd.anwar,
lorenzo.bianconi, devicetree, netdev, linux-arm-msm, linux-kernel
Hi,
On 10/15/26 16:38, 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 | 29 +++++++++++++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
> diff --git a/drivers/net/phy/qcom/qca808x.c b/drivers/net/phy/qcom/qca808x.c
> index bab26e4c61408..4aba513b9a512 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,43 @@ 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)
> +{
> + static const char * const regulator_names[] = {
> + "avdd18", "vdd", "vdd18", "vdd125"
> + };
> + struct device *dev = &phydev->mdio.dev;
> + u32 count = 0;
> + int ret;
> +
> + for (u32 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);
Kinda makes sense to use that delay as a basis, it may be too short in
some odd cases but I guess it's good enough of an approximation here,
> +
> + 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;
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Maxime
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH net-next v2 2/2] net: phy: qcom: qca808x: Add regulator management
2026-10-15 14:38 ` [PATCH net-next v2 2/2] net: phy: qcom: qca808x: Add regulator management Alex Elder
2026-10-15 15:16 ` Maxime Chevallier
@ 2026-10-15 15:40 ` Lorenzo Bianconi
2026-10-15 16:01 ` Alex Elder
2026-10-16 14:39 ` sashiko-bot
2 siblings, 1 reply; 11+ messages in thread
From: Lorenzo Bianconi @ 2026-10-15 15:40 UTC (permalink / raw)
To: Alex Elder
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, robh, krzk+dt,
conor+dt, hkallweit1, linux, Daniel Thompson, ansuelsmth,
donggeunyoo.kernel, mohd.anwar, devicetree, netdev, linux-arm-msm,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2458 bytes --]
On Oct 15, 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 | 29 +++++++++++++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
> diff --git a/drivers/net/phy/qcom/qca808x.c b/drivers/net/phy/qcom/qca808x.c
> index bab26e4c61408..4aba513b9a512 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,43 @@ 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)
> +{
> + static const char * const regulator_names[] = {
> + "avdd18", "vdd", "vdd18", "vdd125"
> + };
> + struct device *dev = &phydev->mdio.dev;
> + u32 count = 0;
> + int ret;
> +
> + for (u32 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);
nit: should we check reset_assert_delay field is actually set?
Regards,
Lorenzo
> +
> + 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
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH net-next v2 2/2] net: phy: qcom: qca808x: Add regulator management
2026-10-15 15:40 ` Lorenzo Bianconi
@ 2026-10-15 16:01 ` Alex Elder
2026-10-15 16:27 ` Andrew Lunn
0 siblings, 1 reply; 11+ messages in thread
From: Alex Elder @ 2026-10-15 16:01 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, robh, krzk+dt,
conor+dt, hkallweit1, linux, Daniel Thompson, ansuelsmth,
donggeunyoo.kernel, mohd.anwar, devicetree, netdev, linux-arm-msm,
linux-kernel
On 10/15/26 10:40 AM, Lorenzo Bianconi wrote:
>> + if (count)
>> + fsleep(phydev->mdio.reset_assert_delay);
> nit: should we check reset_assert_delay field is actually set?
Are you saying there should be an alternative delay used
if it is not set? Do you have a suggestion for what to
use in that case?
-Alex
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next v2 2/2] net: phy: qcom: qca808x: Add regulator management
2026-10-15 16:01 ` Alex Elder
@ 2026-10-15 16:27 ` Andrew Lunn
2026-10-15 16:37 ` Mohd Ayaan Anwar
0 siblings, 1 reply; 11+ messages in thread
From: Andrew Lunn @ 2026-10-15 16:27 UTC (permalink / raw)
To: Alex Elder
Cc: Lorenzo Bianconi, andrew+netdev, davem, edumazet, kuba, pabeni,
robh, krzk+dt, conor+dt, hkallweit1, linux, Daniel Thompson,
ansuelsmth, donggeunyoo.kernel, mohd.anwar, devicetree, netdev,
linux-arm-msm, linux-kernel
On Thu, Oct 15, 2026 at 11:01:43AM -0500, Alex Elder wrote:
> On 10/15/26 10:40 AM, Lorenzo Bianconi wrote:
> > > + if (count)
> > > + fsleep(phydev->mdio.reset_assert_delay);
> > nit: should we check reset_assert_delay field is actually set?
>
> Are you saying there should be an alternative delay used
> if it is not set? Do you have a suggestion for what to
> use in that case?
Does the binding require the value is set?
What happens it if is 0? I think fsleep() is happy to delay for 0 uS,
but does the hardware still work? Does the datasheet say anything
about power on delays?
Andrew
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH net-next v2 2/2] net: phy: qcom: qca808x: Add regulator management
2026-10-15 16:27 ` Andrew Lunn
@ 2026-10-15 16:37 ` Mohd Ayaan Anwar
2026-10-16 15:17 ` Andrew Lunn
0 siblings, 1 reply; 11+ messages in thread
From: Mohd Ayaan Anwar @ 2026-10-15 16:37 UTC (permalink / raw)
To: Andrew Lunn
Cc: Alex Elder, Lorenzo Bianconi, andrew+netdev, davem, edumazet,
kuba, pabeni, robh, krzk+dt, conor+dt, hkallweit1, linux,
Daniel Thompson, ansuelsmth, donggeunyoo.kernel, devicetree,
netdev, linux-arm-msm, linux-kernel
On Thu, Oct 15, 2026 at 06:27:01PM +0200, Andrew Lunn wrote:
> On Thu, Oct 15, 2026 at 11:01:43AM -0500, Alex Elder wrote:
> > On 10/15/26 10:40 AM, Lorenzo Bianconi wrote:
> > > > + if (count)
> > > > + fsleep(phydev->mdio.reset_assert_delay);
> > > nit: should we check reset_assert_delay field is actually set?
> >
> > Are you saying there should be an alternative delay used
> > if it is not set? Do you have a suggestion for what to
> > use in that case?
>
> Does the binding require the value is set?
>
> What happens it if is 0? I think fsleep() is happy to delay for 0 uS,
> but does the hardware still work? Does the datasheet say anything
> about power on delays?
>
I was going to share a text version of the power-up diagram given in
this PHY's datasheet. This might help.
Power-Up Sequence
1. DVDD asserted
|
|<-------- 10 ms -------->| (required for external power)
|
2. DVDD18 / AVDD18 asserted
3. VDD1P25 asserted
|
|<-------- 0.2 ms ------->| (required for LDO to settle)
|
4. VDD_LDO asserted
5. AVDD asserted
|
|<--------- 2 ms -------->| (required for XTL to oscillate)
|
6. Crystal 50 MHz starts oscillating
7. PAD reset deasserted
|
|<------- 10 ms --------->| (deglitch time)
So, maybe just fsleep(22200) or something a bit higher?
Ayaan
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next v2 2/2] net: phy: qcom: qca808x: Add regulator management
2026-10-15 16:37 ` Mohd Ayaan Anwar
@ 2026-10-16 15:17 ` Andrew Lunn
2026-10-17 8:12 ` Mohd Ayaan Anwar
0 siblings, 1 reply; 11+ messages in thread
From: Andrew Lunn @ 2026-10-16 15:17 UTC (permalink / raw)
To: Mohd Ayaan Anwar
Cc: Alex Elder, Lorenzo Bianconi, andrew+netdev, davem, edumazet,
kuba, pabeni, robh, krzk+dt, conor+dt, hkallweit1, linux,
Daniel Thompson, ansuelsmth, donggeunyoo.kernel, devicetree,
netdev, linux-arm-msm, linux-kernel
> I was going to share a text version of the power-up diagram given in
> this PHY's datasheet. This might help.
>
> Power-Up Sequence
>
> 1. DVDD asserted
> |
> |<-------- 10 ms -------->| (required for external power)
> |
> 2. DVDD18 / AVDD18 asserted
>
> 3. VDD1P25 asserted
> |
> |<-------- 0.2 ms ------->| (required for LDO to settle)
> |
> 4. VDD_LDO asserted
>
> 5. AVDD asserted
> |
> |<--------- 2 ms -------->| (required for XTL to oscillate)
> |
> 6. Crystal 50 MHz starts oscillating
>
> 7. PAD reset deasserted
> |
> |<------- 10 ms --------->| (deglitch time)
>
Are these supposed to be staggered? Or can you turn them all on at
once.
23ms does seem like the worst case.
Andrew
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next v2 2/2] net: phy: qcom: qca808x: Add regulator management
2026-10-16 15:17 ` Andrew Lunn
@ 2026-10-17 8:12 ` Mohd Ayaan Anwar
0 siblings, 0 replies; 11+ messages in thread
From: Mohd Ayaan Anwar @ 2026-10-17 8:12 UTC (permalink / raw)
To: Andrew Lunn
Cc: Alex Elder, Lorenzo Bianconi, andrew+netdev, davem, edumazet,
kuba, pabeni, robh, krzk+dt, conor+dt, hkallweit1, linux,
Daniel Thompson, ansuelsmth, donggeunyoo.kernel, devicetree,
netdev, linux-arm-msm, linux-kernel
On Fri, Oct 16, 2026 at 05:17:20PM +0200, Andrew Lunn wrote:
> > I was going to share a text version of the power-up diagram given in
> > this PHY's datasheet. This might help.
> >
> > Power-Up Sequence
> >
> > 1. DVDD asserted
> > |
> > |<-------- 10 ms -------->| (required for external power)
> > |
> > 2. DVDD18 / AVDD18 asserted
> >
> > 3. VDD1P25 asserted
> > |
> > |<-------- 0.2 ms ------->| (required for LDO to settle)
> > |
> > 4. VDD_LDO asserted
> >
> > 5. AVDD asserted
> > |
> > |<--------- 2 ms -------->| (required for XTL to oscillate)
> > |
> > 6. Crystal 50 MHz starts oscillating
> >
> > 7. PAD reset deasserted
> > |
> > |<------- 10 ms --------->| (deglitch time)
> >
>
> Are these supposed to be staggered? Or can you turn them all on at
> once.
Its a confusing diagram but from what I gather: there's no delay
required between 1., 3., and 5. They are shown as partially overlapping.
This might be a better representation of the diagram:
DVDD ..../-------------------------------------------------
|<--------- 10 ms ----------->|
| (external power) |
DVDD18/AVDD18 ................................../-------------------
VDD1P25 ............../---------------------------------------
|<0.2 ms>|
| LDO settling
VDD_LDO ......................../-----------------------------
AVDD ......................../-----------------------------
|< 2 ms >|
| crystal startup
Crystal 50 MHz ........................................../\/\/\/\/\/
PAD reset ..................................................../---
|<10 ms>|
deglitch
>
> 23ms does seem like the worst case.
>
Yes, I thought that 23ms would be a safe value after looking at the diagram.
Ayaan
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next v2 2/2] net: phy: qcom: qca808x: Add regulator management
2026-10-15 14:38 ` [PATCH net-next v2 2/2] net: phy: qcom: qca808x: Add regulator management Alex Elder
2026-10-15 15:16 ` Maxime Chevallier
2026-10-15 15:40 ` Lorenzo Bianconi
@ 2026-10-16 14:39 ` sashiko-bot
2 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-10-16 14:39 UTC (permalink / raw)
To: Alex Elder; +Cc: devicetree, robh, conor+dt
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: phy: qca808x: missing PHY reset pulse after enabling regulators
--
commit e2b47da72c4cf5c697d4c2078b3f0f943b751123
Author: Daniel Thompson <daniel@riscstar.com>
net: phy: qcom: qca808x: Add regulator management
This commit adds logic to power on GPIO-controlled regulators during the
probe of the QCA8081 PHY device. It uses an always-on approach, turning on
regulators if present and sleeping for the reset assert delay.
> diff --git a/drivers/net/phy/qcom/qca808x.c b/drivers/net/phy/qcom/qca808x.c
> index bab26e4c61408..4aba513b9a512 100644
> --- a/drivers/net/phy/qcom/qca808x.c
> +++ b/drivers/net/phy/qcom/qca808x.c
[ ... ]
> @@ -181,15 +182,43 @@ 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)
> +{
> + static const char * const regulator_names[] = {
> + "avdd18", "vdd", "vdd18", "vdd125"
> + };
> + struct device *dev = &phydev->mdio.dev;
> + u32 count = 0;
> + int ret;
> +
> + for (u32 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);
[Severity: High]
Does this sequence successfully initialize the PHY hardware? The driver applies
power here and sleeps for the reset delay, but doesn't seem to explicitly
toggle the reset line.
Since the MDIO core deasserts the reset line before invoking the driver's
probe() callback, the PHY appears to power up with the reset line already
deasserted. Could this regression result in the hardware failing to initialize
properly and leaving a dead link?
Perhaps an explicit assert and deassert (e.g., via phy_device_reset()) is
needed here to provide a valid reset pulse after the power is stable.
> +
> + return 0;
> +}
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261015143849.483339-1-elder@riscstar.com?part=2
^ permalink raw reply [flat|nested] 11+ messages in thread