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

[netdev-bot+sashiko@kernel.org]

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