Re: [PATCH net-next v3 2/5] net: phy: mxl-gpy: add MxL86211C VDDP supply

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · 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] In this commit, the MxL86211C pad-voltage select
  (VSPEC1_PM_CTRL_MDIO_VOL, bit 14 of VSPEC1_PM_CTRL) is written only…
- [Medium] The VDDP regulator powers the MDIO/MDC/MDINT pads, but
  mxl86211c_probe() (drivers/net/phy/mxl-gpy.c:437-457) first gets and…

--

net: phy: mxl-gpy: add MxL86211C VDDP supply

This adds an optional VDDP supply consumer to the MxL86211C probe. If the
supply is described, it is enabled for the PHY lifetime, and the
VSPEC1_PM_CTRL MDIO pad voltage bit is set to match the reported 1.8 V or
3.3 V level. If the supply is absent, the hardware reset default is kept.

> diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
> index 089a1a9e5bc6..952272faac1e 100644
> --- a/drivers/net/phy/mxl-gpy.c
> +++ b/drivers/net/phy/mxl-gpy.c

[ ... ]

> +static int mxl86211c_probe(struct phy_device *phydev)
> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	struct gpy_priv *priv;
> +	struct regulator *vddp;
> +	int ret;
> +
> +	vddp = devm_regulator_get_optional(dev, "vddp");
> +	if (PTR_ERR_OR_ZERO(vddp) == -ENODEV)
> +		vddp = NULL;
> +	else if (IS_ERR(vddp))
> +		return dev_err_probe(dev, PTR_ERR(vddp),
> +				     "failed to get VDDP supply\n");
> +
> +	if (vddp) {
> +		ret = regulator_enable(vddp);

[Severity: Medium]
Can a switchable vddp-supply that is off at boot work with this ordering?
VDDP powers the MDIO/MDC/MDINT pads. Here it is first enabled from the PHY
driver's .probe callback. By then phylib has already accessed the PHY over
MDIO and has already released its reset.

During discovery, fwnode_mdiobus_register_phy() reads the PHY ID over MDIO
before any driver binds:

drivers/net/mdio/fwnode_mdio.c:fwnode_mdiobus_register_phy() {
    ...
	is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45");
	if (is_c45 || fwnode_get_phy_id(child, &phy_id))
		phy = get_phy_device(bus, addr, is_c45);
    ...
}

The binding's vddp-supply example uses compatible =
"ethernet-phy-idc133.5400", "ethernet-phy-ieee802.3-c45". That makes is_c45
true, so the ID reads go out over unpowered pads.

Wouldn't registration fail at that point, before mxl86211c_probe() gets to
enable VDDP?

If only the ID compatible is listed, discovery is skipped. Even then,
phy_probe() deasserts reset before it calls the driver probe:

drivers/net/phy/phy_device.c:phy_probe() {
    ...
	/* Deassert the reset signal */
	phy_device_reset(phydev, 0);

	if (phydev->drv->probe) {
		err = phydev->drv->probe(phydev);
    ...
}

So the PHY comes out of reset while VDDP is still off. mxl86211c_probe()
then enables VDDP and writes VSPEC1_PM_CTRL straight away, with no reset
cycle after power-up.

This seems to work only when VDDP is already on (always-on, boot-on, or
left on by the bootloader). Should the commit message or the binding
document that limitation? Or should the supply be handled earlier?

> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "failed to enable VDDP supply\n");
> +
> +		ret = devm_add_action_or_reset(dev, mxl86211c_disable_vddp, vddp);
> +		if (ret)
> +			return ret;
> +
> +		ret = mxl86211c_configure_mdio_voltage(phydev, vddp);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = gpy_probe(phydev);
> +	if (ret)
> +		return ret;
> +
> +	priv = phydev->priv;
> +	priv->vddp = vddp;
> +
> +	return 0;
> +}

[ ... ]

> @@ -1406,7 +1482,7 @@ static struct phy_driver gpy_drivers[] = {
>  		.name		= "Maxlinear Ethernet MxL86211C",
>  		.get_features	= genphy_c45_pma_read_abilities,
>  		.config_init	= gpy21x_config_init,
> -		.probe		= gpy_probe,
> +		.probe		= mxl86211c_probe,

[Severity: Medium]
Is the VSPEC1_PM_CTRL_MDIO_VOL setting lost after a later hardware reset?
This patch writes the bit only once, from mxl86211c_probe(). The MxL86211C
entry still uses gpy21x_config_init(), which never writes VSPEC1_PM_CTRL.

On a board with reset-gpios, a detach/attach cycle runs:

phy_detach()
  phy_device_reset(phydev, 1)
phy_attach_direct()
  phy_init_hw()
    phy_device_reset(phydev, 0)
    phydev->drv->config_init() -> gpy21x_config_init()

After that the pad mode is back at its reset default and no longer matches
a 1.8 V VDDP. A suspend that drops PHY power would have the same result.
This commit stores priv->vddp but never reads it.

A later commit in this series, "net: phy: mxl-gpy: release MxL86211C Super
Isolate", seems to fix this. It switches the entry to
mxl86211c_config_init(), which calls
mxl86211c_configure_mdio_voltage(phydev, priv->vddp) whenever priv->vddp is
set. Before this patch the bit was never written at all.

Could the setting be restored from config_init in this patch, so that the
intermediate commit is correct on its own?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020093655.344585-1-robert.marko%40sartura.hr