Re: [PATCH net] net: phy: air_en8811h: restore LED GPIO output after MCU restart

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
Verified. Report follows.

commit 90353ad40d9c49a8f67a9cf009916549d394f63d
Author: Ziyou Xu <xuziyougm@xxxxxxxxx>

net: phy: air_en8811h: restore LED GPIO output after MCU restart

The LED GPIO3/4/5 output-enable write is moved out of en8811h_probe() and
into en8811h_config_init(), just before air_leds_init(), so the pads are
re-enabled after every MD32 restart.

> diff --git a/drivers/net/phy/air_en8811h.c b/drivers/net/phy/air_en8811h.c
> index edd49c193e476..fbd87fc712bfb 100644
> --- a/drivers/net/phy/air_en8811h.c
> +++ b/drivers/net/phy/air_en8811h.c

[ ... ]

> @@ -1317,6 +1310,16 @@ static int en8811h_config_init(struct phy_device *phydev)
>  	if (ret < 0)
>  		return ret;
>  
> +	/*
> +	 * Restarting MD32 clears the GPIO3/4/5 output enable bits. Restore
> +	 * them after every restart, before enabling the corresponding LEDs.
> +	 */
> +	ret = air_phy_buckpbus_reg_modify(phydev, EN8811H_GPIO_OUTPUT,
> +					  EN8811H_GPIO_OUTPUT_345,
> +					  EN8811H_GPIO_OUTPUT_345);
> +	if (ret < 0)
> +		return ret;
> +

[Severity: Medium]
This isn't a bug introduced by this patch, but should the sibling AN8811HB
path in the same file get the same treatment?

AN8811HB_GPIO_OUTPUT_345 is written in exactly one place, an8811hb_probe():

	/* Configure led gpio pins as output */
	ret = air_phy_buckpbus_reg_modify(phydev, AN8811HB_GPIO_OUTPUT,
					  AN8811HB_GPIO_OUTPUT_345,
					  AN8811HB_GPIO_OUTPUT_345);

an8811hb_config_init() then restarts the MCU on every call after the first
and goes straight to the LEDs without touching that register:

	if (priv->mcu_needs_restart) {
		ret = en8811h_restart_mcu(phydev);
		...
	}
	ret = an8811hb_config_serdes_polarity(phydev);
	...
	ret = air_leds_init(phydev, EN8811H_LED_COUNT, AIR_PHY_LED_DUR,
			    AIR_LED_MODE_USER_DEFINE);

For AN8811HB, en8811h_restart_mcu() does a superset of the EN8811H
sequence:

	if (phy_id_compare_model(phydev->phy_id, AN8811HB_PHY_ID)) {
		ret = an8811hb_mcu_assert(phydev);
		...
		ret = an8811hb_mcu_deassert(phydev);
		...
	}
	ret = air_phy_buckpbus_reg_write(phydev, EN8811H_FW_CTRL_1,
					 EN8811H_FW_CTRL_1_START);

If the premise stated in the new comment ("Restarting MD32 clears the
GPIO3/4/5 output enable bits") holds for this device family, do the
AN8811HB LEDs also go dark on the second and later .config_init() call
(ifdown/ifup, or a repeated phy_init_hw())?

After this patch the two peer paths in the file disagree: en8811h_config_init()
restores the pad output-enable bits on every restart, while
an8811hb_config_init() still assumes they survive it.

Would it make sense to place the restore in the shared en8811h_restart_mcu()
or air_leds_init() path so both models are covered, or duplicate it in
an8811hb_config_init()? If AN8811HB is genuinely unaffected, could the commit
message say why?

Note this concern is based on the register definitions rather than measured
hardware behaviour: AN8811HB_GPIO_OUTPUT is the same 0xcf8b8 offset within
the AN8811HB PBUS window with an identical BIT(3) | BIT(4) | BIT(5) mask, and
commit 6b9c9def95cb ("net: phy: air_en8811h: Introduce resume/suspend and
clk_restore_context...") notes that PHY reinitialisation reverts chip-config
registers such as CKO to their initial values.

>  	ret = air_leds_init(phydev, EN8811H_LED_COUNT, AIR_PHY_LED_DUR,
>  			    AIR_LED_MODE_USER_DEFINE);
>  	if (ret < 0) {
-- 
pw-bot: cr