[PATCH net v3] net: phy: qcom: at803x: Apply IPQ5018 analog settings at probe

[PATCH net v3] net: phy: qcom: at803x: Apply IPQ5018 analog settings at probe

[Yongzhao Chen]

ipq5018_probe() pulses the PHY reset, after which the IPQ5018 internal
GE PHY starts autonegotiating with its reset-default analog settings.
ipq5018_config_init() writes the analog settings only when the MAC
attaches the PHY, usually when the interface is opened. On a board
where the PHY connects to a QCA8337 PHY without a cable, 1000BASE-T
does not come up in the meantime, SmartSpeed downshifts both PHYs, and
only the IPQ5018 side restores its 1000BASE-T advertisement at attach,
so the link stays down.

Apply the analog settings in probe right after the reset and restart
autonegotiation. Move the settings into a helper shared with
config_init() and return MDIO errors from it instead of ignoring them.
The values written do not change, only the time of the write.

Fixes: d46502279a11 ("net: phy: qcom: at803x: Add Qualcomm IPQ5018 Internal PHY support")
Signed-off-by: Yongzhao Chen <yongzhao.derek@gmail.com>
Assisted-by: LLM
---
Changes since v2 [1]:
- Dropped the DAC value fix (v2 1/2), now in net as commit
  20f3599d85b4 ("net: phy: qcom: at803x: Fix IPQ5018 short-cable DAC
  values").
- Shortened the commit message as Andrew suggested [2]. No code change.

Tested on a Redmi AX5400, where the IPQ5018 PHY connects to PHY4 of a
QCA8337, with OpenWrt's Linux 6.18.52. config_init() ran about 39 s
after the reset. With the probe-time writes, 1 Gb/s came up in 3/3 warm
boots, against 0/3 without. With the backport and SmartSpeed enabled,
the link was verified to recover to 1 Gb/s after a power-off cold boot,
reboots, interface down/up cycles and renegotiations. Splitting the
settings over 26 warm boots showed that the early DAC write decides
the outcome on this board; the patch still applies the whole set, as
config_init() does.

George, probe accesses the PHY right after reset_control_reset()
pulses GCC_GEPHY_MISC_ARES for about 1 us, with no delay. The vendor
SDK waits 200 ms after every Ethernet reset. Does the GE PHY need a
minimum delay or a readiness check after ARES is deasserted?

[1] https://lore.kernel.org/netdev/20260928220717.939-1-yongzhao.derek@gmail.com/
[2] https://lore.kernel.org/netdev/f8e1f642-8250-4c1f-be1c-713a0026282e@lunn.ch/

 drivers/net/phy/qcom/at803x.c | 98 +++++++++++++++++++++++++----------
 1 file changed, 70 insertions(+), 28 deletions(-)

diff --git a/drivers/net/phy/qcom/at803x.c b/drivers/net/phy/qcom/at803x.c
index cacbadf1f48..09105f1e373 100644
--- a/drivers/net/phy/qcom/at803x.c
+++ b/drivers/net/phy/qcom/at803x.c
@@ -1019,10 +1019,10 @@ static int ipq5018_cable_test_start(struct phy_device *phydev)
 	return 0;
 }
 
-static int ipq5018_config_init(struct phy_device *phydev)
+static int ipq5018_analog_init(struct phy_device *phydev)
 {
 	struct ipq5018_priv *priv = phydev->priv;
-	u16 val;
+	int val, ret;
 
 	/*
 	 * set LDO efuse: first temporarily store ANA_DAC_FILTER value from
@@ -1030,39 +1030,66 @@ static int ipq5018_config_init(struct phy_device *phydev)
 	 * is written to
 	 */
 	val = at803x_debug_reg_read(phydev, IPQ5018_PHY_DEBUG_ANA_DAC_FILTER);
-	at803x_debug_reg_mask(phydev, IPQ5018_PHY_DEBUG_ANA_LDO_EFUSE,
-			      IPQ5018_PHY_DEBUG_ANA_LDO_EFUSE_MASK,
-			      IPQ5018_PHY_DEBUG_ANA_LDO_EFUSE_DEFAULT);
-	at803x_debug_reg_write(phydev, IPQ5018_PHY_DEBUG_ANA_DAC_FILTER, val);
+	if (val < 0)
+		return val;
+
+	ret = at803x_debug_reg_mask(phydev, IPQ5018_PHY_DEBUG_ANA_LDO_EFUSE,
+				    IPQ5018_PHY_DEBUG_ANA_LDO_EFUSE_MASK,
+				    IPQ5018_PHY_DEBUG_ANA_LDO_EFUSE_DEFAULT);
+	if (ret)
+		return ret;
+
+	ret = at803x_debug_reg_write(phydev, IPQ5018_PHY_DEBUG_ANA_DAC_FILTER,
+				     val);
+	if (ret)
+		return ret;
 
 	/* set 8023AZ EEE TX and RX timer values */
-	phy_write_mmd(phydev, MDIO_MMD_PCS, IPQ5018_PHY_PCS_EEE_TX_TIMER,
-		      IPQ5018_PHY_PCS_EEE_TX_TIMER_VAL);
-	phy_write_mmd(phydev, MDIO_MMD_PCS, IPQ5018_PHY_PCS_EEE_RX_TIMER,
-		      IPQ5018_PHY_PCS_EEE_RX_TIMER_VAL);
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, IPQ5018_PHY_PCS_EEE_TX_TIMER,
+			    IPQ5018_PHY_PCS_EEE_TX_TIMER_VAL);
+	if (ret)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PCS, IPQ5018_PHY_PCS_EEE_RX_TIMER,
+			    IPQ5018_PHY_PCS_EEE_RX_TIMER_VAL);
+	if (ret)
+		return ret;
 
 	/* set MSE threshold values */
-	phy_write_mmd(phydev, MDIO_MMD_PMAPMD, IPQ5018_PHY_MMD1_MSE_THRESH1,
-		      IPQ5018_PHY_MMD1_MSE_THRESH1_VAL);
-	phy_write_mmd(phydev, MDIO_MMD_PMAPMD, IPQ5018_PHY_MMD1_MSE_THRESH2,
-		      IPQ5018_PHY_MMD1_MSE_THRESH2_VAL);
+	ret = phy_write_mmd(phydev, MDIO_MMD_PMAPMD,
+			    IPQ5018_PHY_MMD1_MSE_THRESH1,
+			    IPQ5018_PHY_MMD1_MSE_THRESH1_VAL);
+	if (ret)
+		return ret;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_PMAPMD,
+			    IPQ5018_PHY_MMD1_MSE_THRESH2,
+			    IPQ5018_PHY_MMD1_MSE_THRESH2_VAL);
+	if (ret)
+		return ret;
 
 	/* PHY DAC values are optional and only set in a PHY to PHY link architecture */
-	if (priv->set_short_cable_dac) {
-		/* setting MDAC (Multi-level Digital-to-Analog Converter) in MMD1 */
-		phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, IPQ5018_PHY_MMD1_MDAC,
-			       IPQ5018_PHY_DAC_MASK,
-			       FIELD_PREP(IPQ5018_PHY_DAC_MASK,
-					  IPQ5018_PHY_MMD1_MDAC_VAL));
-
-		/* setting EDAC (Error-detection and Correction) in debug register */
-		at803x_debug_reg_mask(phydev, IPQ5018_PHY_DEBUG_EDAC,
-				      IPQ5018_PHY_DAC_MASK,
-				      FIELD_PREP(IPQ5018_PHY_DAC_MASK,
-						 IPQ5018_PHY_DEBUG_EDAC_VAL));
-	}
+	if (!priv->set_short_cable_dac)
+		return 0;
 
-	return 0;
+	/* setting MDAC (Multi-level Digital-to-Analog Converter) in MMD1 */
+	ret = phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, IPQ5018_PHY_MMD1_MDAC,
+			     IPQ5018_PHY_DAC_MASK,
+			     FIELD_PREP(IPQ5018_PHY_DAC_MASK,
+					IPQ5018_PHY_MMD1_MDAC_VAL));
+	if (ret)
+		return ret;
+
+	/* setting EDAC (Error-detection and Correction) in debug register */
+	return at803x_debug_reg_mask(phydev, IPQ5018_PHY_DEBUG_EDAC,
+				     IPQ5018_PHY_DAC_MASK,
+				     FIELD_PREP(IPQ5018_PHY_DAC_MASK,
+						IPQ5018_PHY_DEBUG_EDAC_VAL));
+}
+
+static int ipq5018_config_init(struct phy_device *phydev)
+{
+	return ipq5018_analog_init(phydev);
 }
 
 static void ipq5018_link_change_notify(struct phy_device *phydev)
@@ -1111,6 +1138,21 @@ static int ipq5018_probe(struct phy_device *phydev)
 
 	phydev->priv = priv;
 
+	/*
+	 * The PHY starts autonegotiation as soon as it leaves reset. Apply the
+	 * analog settings now instead of waiting for config_init() at attach
+	 * time, and restart autonegotiation so that it uses them.
+	 */
+	ret = ipq5018_analog_init(phydev);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to apply analog settings\n");
+
+	ret = genphy_restart_aneg(phydev);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to restart autonegotiation\n");
+
 	return 0;
 }
 

base-commit: 6dc989ea46b96ce170840174b4a38c4a387fb005
-- 
2.43.0

Re: [PATCH net v3] net: phy: qcom: at803x: Apply IPQ5018 analog settings at probe

[netdev-bot+sashiko]

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

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

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] ipq5018_probe() (drivers/net/phy/qcom/at803x.c) calls
  reset_control_reset(priv->rst) at line 1135.

--

net: phy: qcom: at803x: Apply IPQ5018 analog settings at probe

This commit moves the IPQ5018 internal GE PHY analog settings into a
helper, ipq5018_analog_init(), that both config_init() and probe call.
Probe now applies the settings right after it pulses the PHY reset, then
restarts autonegotiation, so the PHY no longer negotiates with the
reset-default analog settings until the MAC attaches. The helper also
returns MDIO errors now instead of ignoring them.

> diff --git a/drivers/net/phy/qcom/at803x.c b/drivers/net/phy/qcom/at803x.c
> index cacbadf1f4841..09105f1e37379 100644
> --- a/drivers/net/phy/qcom/at803x.c
> +++ b/drivers/net/phy/qcom/at803x.c

[ ... ]

> @@ -1111,6 +1138,21 @@ static int ipq5018_probe(struct phy_device *phydev)
>  
>  	phydev->priv = priv;
>  
> +	/*
> +	 * The PHY starts autonegotiation as soon as it leaves reset. Apply the
> +	 * analog settings now instead of waiting for config_init() at attach
> +	 * time, and restart autonegotiation so that it uses them.
> +	 */
> +	ret = ipq5018_analog_init(phydev);

[Severity: Medium]

Is the GE PHY ready for these debug and MMD register accesses this soon
after the reset?

Earlier in ipq5018_probe(), reset_control_reset(priv->rst) pulses the
reset. That call ends up in qcom_reset():

drivers/clk/qcom/reset.c:qcom_reset() {
	rcdev->ops->assert(rcdev, id);
	fsleep(rst->reset_map[id].udelay ?: 1); /* use 1 us as default */

	rcdev->ops->deassert(rcdev, id);
	return 0;
}

The GCC entry sets no udelay. It covers several GE PHY sub-block resets
at once, including the DSP reset:

drivers/clk/qcom/gcc-ipq5018.c:
	[GCC_GEPHY_MISC_ARES] = { 0x56004, .bitmask = GENMASK(3, 0) },

So nothing waits after deassert before ipq5018_analog_init() runs.
Before this patch, the first analog access happened in config_init().
On the tested board that was about 39 s after the reset.

The patch notes say the vendor SDK waits 200 ms after every Ethernet
reset. If the PHY needs that kind of settle time after ARES deassert,
could the PHY's own init lose or overwrite the probe-time LDO_EFUSE,
DAC_FILTER, EEE, MSE, MDAC and EDAC writes? The PHY would then still
negotiate with reset defaults, which is the failure this patch is meant
to fix.

ipq5018_analog_init() also reads ANA_DAC_FILTER and writes the value
back:

	val = at803x_debug_reg_read(phydev, IPQ5018_PHY_DEBUG_ANA_DAC_FILTER);
	...
	ret = at803x_debug_reg_write(phydev, IPQ5018_PHY_DEBUG_ANA_DAC_FILTER,
				     val);

If the read returns a transient value this early, that value gets
written back. Wouldn't config_init() later re-read the wrong value and
keep it when it rewrites LDO_EFUSE, instead of correcting it?

MDIO transactions don't fail while the target block is busy, so the new
error checks would not catch either case.

The patch notes already ask whether a minimum delay or a readiness check
is needed after ARES is deasserted. Would it make sense to add one after
reset_control_reset() and before ipq5018_analog_init()?

> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to apply analog settings\n");
> +
> +	ret = genphy_restart_aneg(phydev);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to restart autonegotiation\n");
> +
>  	return 0;
>  }
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002210408.730-1-yongzhao.derek%40gmail.com