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

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

[Ziyou Xu]

en8811h_probe() enables the GPIO3/4/5 output bits after loading the
MD32 firmware, but a subsequent config_init() restarts the MCU, which
clears EN8811H_GPIO_OUTPUT[5:3]. The LED event registers survive the
restart, so the LEDs work during early boot and then go dark after the
MCU is restarted.

Move the GPIO output setup into config_init() so it is reapplied after
every MCU restart.

Tested with warm reboot, cold boot and cable unplug/replug. The failure
was confirmed by reading EN8811H_GPIO_OUTPUT as 0x00; setting only
EN8811H_GPIO_OUTPUT_345 was sufficient to restore the LEDs.

Fixes: 71e79430117d ("net: phy: air_en8811h: Add the Airoha EN8811H PHY driver")
Cc: stable@vger.kernel.org
Signed-off-by: Ziyou Xu <xuziyougm@gmail.com>
---
 drivers/net/phy/air_en8811h.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/air_en8811h.c b/drivers/net/phy/air_en8811h.c
index edd49c193..fbd87fc71 100644
--- a/drivers/net/phy/air_en8811h.c
+++ b/drivers/net/phy/air_en8811h.c
@@ -1173,13 +1173,6 @@ static int en8811h_probe(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
-	/* Configure led gpio pins as output */
-	ret = air_phy_buckpbus_reg_modify(phydev, EN8811H_GPIO_OUTPUT,
-					  EN8811H_GPIO_OUTPUT_345,
-					  EN8811H_GPIO_OUTPUT_345);
-	if (ret < 0)
-		return ret;
-
 	return 0;
 }
 
@@ -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;
+
 	ret = air_leds_init(phydev, EN8811H_LED_COUNT, AIR_PHY_LED_DUR,
 			    AIR_LED_MODE_USER_DEFINE);
 	if (ret < 0) {

base-commit: 564973a259ec76f2dad0853420e7034cc43994c4
-- 
2.43.0

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@gmail.com>

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

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

[js node]

Thanks for the review.

I don't have AN8811HB hardware, so I can't confirm whether the MCU
restart clears AN8811HB_GPIO_OUTPUT in the same way.

Looking at the register layout, I agree it is very likely affected.
However, I'd prefer to keep this patch scoped to EN8811H where the
fix is verified, rather than extend it to AN8811HB based on
speculation.

If someone with AN8811HB hardware can confirm, a follow-up patch
adding the same restore to an8811hb_config_init() would make sense.


Jakub Kicinski <kuba@kernel.org> 于2026年8月25日周二 02:50写道：
>
> 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@gmail.com>
>
> 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