[PATCH v5] net: phy: air_en8811h: add AN8811HB MCU assert/deassert support

[PATCH v5] net: phy: air_en8811h: add AN8811HB MCU assert/deassert support

[Lucien.Jheng]

AN8811HB needs a MCU soft-reset cycle before firmware loading begins.
Assert the MCU (hold it in reset) and immediately deassert (release)
via a dedicated PBUS register pair (0x5cf9f8 / 0x5cf9fc), accessed
through a registered mdio_device at PHY-addr+8.

Add __air_pbus_reg_write() as a low-level helper taking a struct
mdio_device *, create and register the PBUS mdio_device in
an8811hb_probe() and store it in priv->pbusdev, then implement
an8811hb_mcu_assert() / _deassert() on top of it. Add
an8811hb_remove() to unregister the PBUS device on teardown. Wire
both calls into an8811hb_load_firmware() and en8811h_restart_mcu()
so every firmware load or MCU restart on AN8811HB correctly sequences
the reset control registers.

Fixes: 5afda1d734ed ("net: phy: air_en8811h: add Airoha AN8811HB support")
Signed-off-by: Lucien Jheng <lucienzx159@gmail.com>

---
Changes in v5:
- Move the version changelog out of the commit message;
  keep it only in the patch changelog section.

Changes in v4:
- Correct Fixes tag to 5afda1d734ed.
- Refine an8811hb_probe() error paths: split cleanup into err_dev_free and
  err_dev_create so an unregistered PBUS mdio_device is only freed, while a
  registered one is removed then freed. Simplify with fallthrough pattern.

Changes in v3:
- Register a dedicated mdio_device for the PBUS node (addr+8) in
  an8811hb_probe() and store it in priv->pbusdev, replacing the
  inline addr+8 computation in v2.
- Change __air_pbus_reg_write() to take struct mdio_device * instead
  of struct phy_device *.
- Add an8811hb_remove() to unregister and free the PBUS device.
- Fix error handling in an8811hb_probe() with goto error paths to
  ensure the PBUS device is always cleaned up on failure.

Changes in v2:
- Rewrite commit message: The previous wording was ambiguous,
  as it suggested the MCU remains asserted during the entire
  firmware loading process, rather than a quick reset cycle
  before it starts.
  Clarify that assert and deassert is an immediate soft-reset
  cycle to clear MCU state before firmware loading begins.
- Add Fixes: 0a55766b7711 ("net: phy: air_en8811h: add Airoha AN8811HB
  support").
- Change phydev_info() to phydev_dbg() in an8811hb_mcu_assert() and
  an8811hb_mcu_deassert() to avoid noise during normal boot.

 drivers/net/phy/air_en8811h.c | 143 +++++++++++++++++++++++++++++++++-
 1 file changed, 139 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/air_en8811h.c b/drivers/net/phy/air_en8811h.c
index 29ae73e65..07978979d 100644
--- a/drivers/net/phy/air_en8811h.c
+++ b/drivers/net/phy/air_en8811h.c
@@ -170,9 +170,21 @@
 #define   AN8811HB_CLK_DRV_CKO_LDPWD		BIT(13)
 #define   AN8811HB_CLK_DRV_CKO_LPPWD		BIT(14)

+#define AN8811HB_MCU_SW_RST		0x5cf9f8
+#define   AN8811HB_MCU_SW_RST_HOLD		BIT(16)
+#define   AN8811HB_MCU_SW_RST_RUN		(BIT(16) | BIT(0))
+#define AN8811HB_MCU_SW_START		0x5cf9fc
+#define   AN8811HB_MCU_SW_START_EN		BIT(16)
+
+/* MII register constants for PBUS access (PHY addr + 8) */
+#define AIR_PBUS_ADDR_HIGH		0x1c
+#define AIR_PBUS_DATA_HIGH		0x10
+
 /* Led definitions */
 #define EN8811H_LED_COUNT	3

+#define EN8811H_PBUS_ADDR_OFFS	8
+
 /* Default LED setup:
  * GPIO5 <-> LED0  On: Link detected, blink Rx/Tx
  * GPIO4 <-> LED1  On: Link detected at 2500 or 1000 Mbps
@@ -201,6 +213,7 @@ struct en8811h_priv {
 	struct clk_hw		hw;
 	struct phy_device	*phydev;
 	unsigned int		cko_is_enabled;
+	struct mdio_device	*pbusdev;
 };

 enum {
@@ -254,6 +267,31 @@ static int air_phy_write_page(struct phy_device *phydev, int page)
 	return __phy_write(phydev, AIR_EXT_PAGE_ACCESS, page);
 }

+static int __air_pbus_reg_write(struct mdio_device *mdiodev,
+				u32 pbus_reg, u32 pbus_data)
+{
+	int ret;
+
+	ret = __mdiobus_write(mdiodev->bus, mdiodev->addr, AIR_EXT_PAGE_ACCESS,
+			      upper_16_bits(pbus_reg));
+	if (ret < 0)
+		return ret;
+
+	ret = __mdiobus_write(mdiodev->bus, mdiodev->addr, AIR_PBUS_ADDR_HIGH,
+			      (pbus_reg & GENMASK(15, 6)) >> 6);
+	if (ret < 0)
+		return ret;
+
+	ret = __mdiobus_write(mdiodev->bus, mdiodev->addr,
+			      (pbus_reg & GENMASK(5, 2)) >> 2,
+			      lower_16_bits(pbus_data));
+	if (ret < 0)
+		return ret;
+
+	return __mdiobus_write(mdiodev->bus, mdiodev->addr, AIR_PBUS_DATA_HIGH,
+			       upper_16_bits(pbus_data));
+}
+
 static int __air_buckpbus_reg_write(struct phy_device *phydev,
 				    u32 pbus_address, u32 pbus_data)
 {
@@ -570,10 +608,67 @@ static int an8811hb_load_file(struct phy_device *phydev, const char *name,
 	return ret;
 }

+static int an8811hb_mcu_assert(struct phy_device *phydev)
+{
+	struct en8811h_priv *priv = phydev->priv;
+	int ret;
+
+	phy_lock_mdio_bus(phydev);
+
+	ret = __air_pbus_reg_write(priv->pbusdev, AN8811HB_MCU_SW_RST,
+				   AN8811HB_MCU_SW_RST_HOLD);
+	if (ret < 0)
+		goto unlock;
+
+	ret = __air_pbus_reg_write(priv->pbusdev, AN8811HB_MCU_SW_START, 0);
+	if (ret < 0)
+		goto unlock;
+
+	msleep(50);
+	phydev_dbg(phydev, "MCU asserted\n");
+
+unlock:
+	phy_unlock_mdio_bus(phydev);
+	return ret;
+}
+
+static int an8811hb_mcu_deassert(struct phy_device *phydev)
+{
+	struct en8811h_priv *priv = phydev->priv;
+	int ret;
+
+	phy_lock_mdio_bus(phydev);
+
+	ret = __air_pbus_reg_write(priv->pbusdev, AN8811HB_MCU_SW_START,
+				   AN8811HB_MCU_SW_START_EN);
+	if (ret < 0)
+		goto unlock;
+
+	ret = __air_pbus_reg_write(priv->pbusdev, AN8811HB_MCU_SW_RST,
+				   AN8811HB_MCU_SW_RST_RUN);
+	if (ret < 0)
+		goto unlock;
+
+	msleep(50);
+	phydev_dbg(phydev, "MCU deasserted\n");
+
+unlock:
+	phy_unlock_mdio_bus(phydev);
+	return ret;
+}
+
 static int an8811hb_load_firmware(struct phy_device *phydev)
 {
 	int ret;

+	ret = an8811hb_mcu_assert(phydev);
+	if (ret < 0)
+		return ret;
+
+	ret = an8811hb_mcu_deassert(phydev);
+	if (ret < 0)
+		return ret;
+
 	ret = air_buckpbus_reg_write(phydev, EN8811H_FW_CTRL_1,
 				     EN8811H_FW_CTRL_1_START);
 	if (ret < 0)
@@ -662,6 +757,16 @@ static int en8811h_restart_mcu(struct phy_device *phydev)
 {
 	int ret;

+	if (phy_id_compare_model(phydev->phy_id, AN8811HB_PHY_ID)) {
+		ret = an8811hb_mcu_assert(phydev);
+		if (ret < 0)
+			return ret;
+
+		ret = an8811hb_mcu_deassert(phydev);
+		if (ret < 0)
+			return ret;
+	}
+
 	ret = air_buckpbus_reg_write(phydev, EN8811H_FW_CTRL_1,
 				     EN8811H_FW_CTRL_1_START);
 	if (ret < 0)
@@ -1166,6 +1271,7 @@ static int en8811h_leds_setup(struct phy_device *phydev)

 static int an8811hb_probe(struct phy_device *phydev)
 {
+	struct mdio_device *mdiodev;
 	struct en8811h_priv *priv;
 	int ret;

@@ -1175,10 +1281,21 @@ static int an8811hb_probe(struct phy_device *phydev)
 		return -ENOMEM;
 	phydev->priv = priv;

+	mdiodev = mdio_device_create(phydev->mdio.bus,
+				     phydev->mdio.addr + EN8811H_PBUS_ADDR_OFFS);
+	if (IS_ERR(mdiodev))
+		return PTR_ERR(mdiodev);
+
+	ret = mdio_device_register(mdiodev);
+	if (ret)
+		goto err_dev_free;
+
+	priv->pbusdev = mdiodev;
+
 	ret = an8811hb_load_firmware(phydev);
 	if (ret < 0) {
 		phydev_err(phydev, "Load firmware failed: %d\n", ret);
-		return ret;
+		goto err_dev_create;
 	}

 	en8811h_print_fw_version(phydev);
@@ -1191,22 +1308,29 @@ static int an8811hb_probe(struct phy_device *phydev)

 	ret = en8811h_leds_setup(phydev);
 	if (ret < 0)
-		return ret;
+		goto err_dev_create;

 	priv->phydev = phydev;
 	/* Co-Clock Output */
 	ret = an8811hb_clk_provider_setup(&phydev->mdio.dev, &priv->hw);
 	if (ret)
-		return ret;
+		goto err_dev_create;

 	/* Configure led gpio pins as output */
 	ret = air_buckpbus_reg_modify(phydev, AN8811HB_GPIO_OUTPUT,
 				      AN8811HB_GPIO_OUTPUT_345,
 				      AN8811HB_GPIO_OUTPUT_345);
 	if (ret < 0)
-		return ret;
+		goto err_dev_create;

 	return 0;
+
+err_dev_create:
+	mdio_device_remove(mdiodev);
+
+err_dev_free:
+	mdio_device_free(mdiodev);
+	return ret;
 }

 static int en8811h_probe(struct phy_device *phydev)
@@ -1561,6 +1685,16 @@ static int en8811h_suspend(struct phy_device *phydev)
 	return genphy_suspend(phydev);
 }

+static void an8811hb_remove(struct phy_device *phydev)
+{
+	struct en8811h_priv *priv = phydev->priv;
+
+	if (priv->pbusdev) {
+		mdio_device_remove(priv->pbusdev);
+		mdio_device_free(priv->pbusdev);
+	}
+}
+
 static struct phy_driver en8811h_driver[] = {
 {
 	PHY_ID_MATCH_MODEL(EN8811H_PHY_ID),
@@ -1587,6 +1721,7 @@ static struct phy_driver en8811h_driver[] = {
 	PHY_ID_MATCH_MODEL(AN8811HB_PHY_ID),
 	.name			= "Airoha AN8811HB",
 	.probe			= an8811hb_probe,
+	.remove			= an8811hb_remove,
 	.get_features		= en8811h_get_features,
 	.config_init		= an8811hb_config_init,
 	.get_rate_matching	= en8811h_get_rate_matching,
--
2.34.1

Re: [PATCH v5] net: phy: air_en8811h: add AN8811HB MCU assert/deassert support

[Jakub Kicinski]

On Sun, 17 May 2026 14:50:41 +0800 Lucien.Jheng wrote:
> +static int __air_pbus_reg_write(struct mdio_device *mdiodev,
> +				u32 pbus_reg, u32 pbus_data)
> +{
> +	int ret;
> +
> +	ret = __mdiobus_write(mdiodev->bus, mdiodev->addr, AIR_EXT_PAGE_ACCESS,
> +			      upper_16_bits(pbus_reg));
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = __mdiobus_write(mdiodev->bus, mdiodev->addr, AIR_PBUS_ADDR_HIGH,
> +			      (pbus_reg & GENMASK(15, 6)) >> 6);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = __mdiobus_write(mdiodev->bus, mdiodev->addr,
> +			      (pbus_reg & GENMASK(5, 2)) >> 2,
> +			      lower_16_bits(pbus_data));

Please add proper defines for these GENMASK'ed fields and use
FIELD_GET() to access the fields?

> +	if (ret < 0)
> +		return ret;
> +
> +	return __mdiobus_write(mdiodev->bus, mdiodev->addr, AIR_PBUS_DATA_HIGH,
> +			       upper_16_bits(pbus_data));
> +}

> @@ -1175,10 +1281,21 @@ static int an8811hb_probe(struct phy_device *phydev)
>  		return -ENOMEM;
>  	phydev->priv = priv;
> 
> +	mdiodev = mdio_device_create(phydev->mdio.bus,
> +				     phydev->mdio.addr + EN8811H_PBUS_ADDR_OFFS);
AI says:

What happens when phydev->mdio.addr is in the range 24..31?
MDIO addresses 0..31 are all valid 5-bit hardware addresses, so a
strap-selected AN8811HB at address >= 24 makes
phydev->mdio.addr + EN8811H_PBUS_ADDR_OFFS land in 32..39. struct
mii_bus has mdio_map[PHY_MAX_ADDR] with PHY_MAX_ADDR == 32, and
mdiobus_register_device() does:
	if (mdiodev->bus->mdio_map[mdiodev->addr])
		return -EBUSY;
	...
	mdiodev->bus->mdio_map[mdiodev->addr] = mdiodev;
with no bounds check on mdiodev->addr. The OOB read from
mdio_map[32..39] may return garbage that happens to be non-NULL
(probe fails with -EBUSY) or NULL (the OOB store overwrites adjacent
fields of struct mii_bus such as phy_mask, phy_ignore_ta_mask, or
the irq[] array).
Even without corruption, every later __air_pbus_reg_write() goes
through __mdiobus_write(), which has:
	if (addr >= PHY_MAX_ADDR)
		return -ENXIO;
so each MCU assert/deassert silently fails with -ENXIO, which seems
to contradict the commit message:
    so every firmware load or MCU restart on AN8811HB correctly
    sequences the reset control registers.
Should an8811hb_probe() reject configurations where
phydev->mdio.addr + EN8811H_PBUS_ADDR_OFFS >= PHY_MAX_ADDR, or
otherwise enforce the assumed strap-pin constraint?
-- 
pw-bot: cr

Re: [PATCH v5] net: phy: air_en8811h: add AN8811HB MCU assert/deassert support

[Lucien.Jheng]

Hi Jakub

Jakub Kicinski 於 2026/5/21 上午 07:54 寫道:
> On Sun, 17 May 2026 14:50:41 +0800 Lucien.Jheng wrote:
>> +static int __air_pbus_reg_write(struct mdio_device *mdiodev,
>> +				u32 pbus_reg, u32 pbus_data)
>> +{
>> +	int ret;
>> +
>> +	ret = __mdiobus_write(mdiodev->bus, mdiodev->addr, AIR_EXT_PAGE_ACCESS,
>> +			      upper_16_bits(pbus_reg));
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = __mdiobus_write(mdiodev->bus, mdiodev->addr, AIR_PBUS_ADDR_HIGH,
>> +			      (pbus_reg & GENMASK(15, 6)) >> 6);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = __mdiobus_write(mdiodev->bus, mdiodev->addr,
>> +			      (pbus_reg & GENMASK(5, 2)) >> 2,
>> +			      lower_16_bits(pbus_data));
> Please add proper defines for these GENMASK'ed fields and use
> FIELD_GET() to access the fields?
I will update it in the next patch.
>
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return __mdiobus_write(mdiodev->bus, mdiodev->addr, AIR_PBUS_DATA_HIGH,
>> +			       upper_16_bits(pbus_data));
>> +}
>> @@ -1175,10 +1281,21 @@ static int an8811hb_probe(struct phy_device *phydev)
>>   		return -ENOMEM;
>>   	phydev->priv = priv;
>>
>> +	mdiodev = mdio_device_create(phydev->mdio.bus,
>> +				     phydev->mdio.addr + EN8811H_PBUS_ADDR_OFFS);
> AI says:
>
> What happens when phydev->mdio.addr is in the range 24..31?
> MDIO addresses 0..31 are all valid 5-bit hardware addresses, so a
> strap-selected AN8811HB at address >= 24 makes
> phydev->mdio.addr + EN8811H_PBUS_ADDR_OFFS land in 32..39. struct
> mii_bus has mdio_map[PHY_MAX_ADDR] with PHY_MAX_ADDR == 32, and
> mdiobus_register_device() does:
> 	if (mdiodev->bus->mdio_map[mdiodev->addr])
> 		return -EBUSY;
> 	...
> 	mdiodev->bus->mdio_map[mdiodev->addr] = mdiodev;
> with no bounds check on mdiodev->addr. The OOB read from
> mdio_map[32..39] may return garbage that happens to be non-NULL
> (probe fails with -EBUSY) or NULL (the OOB store overwrites adjacent
> fields of struct mii_bus such as phy_mask, phy_ignore_ta_mask, or
> the irq[] array).
> Even without corruption, every later __air_pbus_reg_write() goes
> through __mdiobus_write(), which has:
> 	if (addr >= PHY_MAX_ADDR)
> 		return -ENXIO;
> so each MCU assert/deassert silently fails with -ENXIO, which seems
> to contradict the commit message:
>      so every firmware load or MCU restart on AN8811HB correctly
>      sequences the reset control registers.
> Should an8811hb_probe() reject configurations where
> phydev->mdio.addr + EN8811H_PBUS_ADDR_OFFS >= PHY_MAX_ADDR, or
> otherwise enforce the assumed strap-pin constraint?

As previously discussed, the fact that the AN8811HB PHY address is 
restricted to 8–15 (decimal)—meaning the PBUS address will only be 
within the range of 16–21 (decimal)—will be documented in the comments 
of the upcoming version.

Thanks