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

["Lucien.Jheng" <lucienzx159@gmail.com>]

Hi Eric

Lucien.Jheng 於 2026/3/29 下午 01:04 寫道:
>
> Eric Woudstra 於 2026/3/27 下午 12:41 寫道:
>>
>> On 3/26/26 4:35 PM, Lucien.Jheng wrote:
>>> 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 the PHY-addr+8 MDIO bus node rather than the BUCKPBUS indirect
>>> path.  This clears the MCU state before the firmware loading sequence
>>> starts.
>>>
>>> Add __air_pbus_reg_write() as a low-level helper for this access, then
>>> implement an8811hb_mcu_assert() / _deassert() on top of it. Wire both
>>> into an8811hb_load_firmware() and en8811h_restart_mcu() so every
>>> firmware load or MCU restart on AN8811HB correctly sequences the reset
>>> control registers.
>>>
>>> Fixes: 0a55766b7711 ("net: phy: air_en8811h: add Airoha AN8811HB 
>>> support")
>>> Signed-off-by: Lucien Jheng <lucienzx159@gmail.com>
>>> ---
>>> 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 | 105 
>>> ++++++++++++++++++++++++++++++++++
>>>   1 file changed, 105 insertions(+)
>>>
>>> diff --git a/drivers/net/phy/air_en8811h.c 
>>> b/drivers/net/phy/air_en8811h.c
>>> index 29ae73e65caa..01fce1b93618 100644
>>> --- a/drivers/net/phy/air_en8811h.c
>>> +++ b/drivers/net/phy/air_en8811h.c
>>> @@ -170,6 +170,16 @@
>>>   #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
>>>
>>> @@ -254,6 +264,36 @@ 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 phy_device *phydev,
>>> +                u32 pbus_reg, u32 pbus_data)
>>> +{
>>> +    struct mii_bus *bus = phydev->mdio.bus;
>>> +    int pbus_addr = phydev->mdio.addr + 8;
>>> +    int ret;
>>> +
>>> +    ret = __mdiobus_write(bus, pbus_addr, AIR_EXT_PAGE_ACCESS,
>>> +                  upper_16_bits(pbus_reg));
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    ret = __mdiobus_write(bus, pbus_addr, AIR_PBUS_ADDR_HIGH,
>>> +                  (pbus_reg & GENMASK(15, 6)) >> 6);
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    ret = __mdiobus_write(bus, pbus_addr, (pbus_reg & GENMASK(5, 
>>> 2)) >> 2,
>>> +                  lower_16_bits(pbus_data));
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    ret = __mdiobus_write(bus, pbus_addr, AIR_PBUS_DATA_HIGH,
>>> +                  upper_16_bits(pbus_data));
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    return 0;
>>> +}
>>> +
>> Maybe you can use mutex_lock() and mutex_unlock() here also, like so:

After checking, I decided not to implement mutex_lock in pbus_read/write.

This is because these functions are only used within 
an8811hb_mcu_assert/deassert.

Since these sequences cannot be interrupted, a mutex lock is not required.

Thanks

>>
>> static int __air_pbus_reg_write(struct mdio_device *mdio, u32 
>> pbus_address,
>>                 u32 pbus_data)
>> {
>>     int ret;
>>
>>     ret = __mdiobus_write(mdio->bus, mdio->addr, AIR_EXT_PAGE_ACCESS,
>>                   (u16)(pbus_address >> 6));
>>     if (ret < 0)
>>         return ret;
>>
>>     ret = __mdiobus_write(mdio->bus, mdio->addr, (pbus_address >> 2) 
>> & 0xf,
>>                   (u16)pbus_data);
>>     if (ret < 0)
>>         return ret;
>>
>>     ret = __mdiobus_write(mdio->bus, mdio->addr, AIR_PBUS_DATA_HIGH,
>>                   (u16)(pbus_data >> 16));
>>     if (ret < 0)
>>         return ret;
>>
>>     return 0;
>> }
>>
>> static int air_pbus_reg_write(struct mdio_device *mdio, u32 
>> pbus_address,
>>                   u32 pbus_data)
>> {
>>     int ret;
>>
>>     mutex_lock(&mdio->bus->mdio_lock);
>>
>>     ret = __air_pbus_reg_write(mdio, pbus_address, pbus_data);
>>     if (ret < 0)
>>         dev_err(&mdio->dev, "%s 0x%08x failed: %d\n", __func__,
>>             pbus_address, ret);
>>
>>     mutex_unlock(&mdio->bus->mdio_lock);
>>
>>     return ret;
>> }
>>
>> static int __air_pbus_reg_read(struct mdio_device *mdio, u32 
>> pbus_address,
>>                    u32 *pbus_data)
>> {
>>     int ret, pbus_data_low;
>>
>>     ret = __mdiobus_write(mdio->bus, mdio->addr, AIR_EXT_PAGE_ACCESS,
>>                   (u16)(pbus_address >> 6));
>>     if (ret < 0)
>>         return ret;
>>
>>     ret = __mdiobus_read(mdio->bus, mdio->addr, (pbus_address >> 2) & 
>> 0xf);
>>     if (ret < 0)
>>         return ret;
>>     pbus_data_low = ret;
>>
>>     ret = __mdiobus_read(mdio->bus, mdio->addr, AIR_PBUS_DATA_HIGH);
>>     if (ret < 0)
>>         return ret;
>>
>>     *pbus_data = (ret << 16) + pbus_data_low;
>>
>>     return 0;
>> }
>>
>> static int air_pbus_reg_read(struct mdio_device *mdio, u32 pbus_address,
>>                  u32 *pbus_data)
>> {
>>     int ret;
>>
>>     mutex_lock(&mdio->bus->mdio_lock);
>>
>>     ret = __air_pbus_reg_read(mdio, pbus_address, pbus_data);
>>     if (ret < 0)
>>         dev_err(&mdio->dev, "%s 0x%08x failed: %d\n", __func__,
>>             pbus_address, ret);
>>
>>     mutex_unlock(&mdio->bus->mdio_lock);
>>
>>     return ret;
>> }
>>
>> With:
>>
>> #define AIR_PBUS_DATA_HIGH        0x10
>>
> Thank you for your feedback.
>
> I should add mutex_lock() and mutex_unlock() to protect these operations.
>
> I will include this in the next version.
>
>>>   static int __air_buckpbus_reg_write(struct phy_device *phydev,
>>>                       u32 pbus_address, u32 pbus_data)
>>>   {
>>> @@ -570,10 +610,65 @@ static int an8811hb_load_file(struct 
>>> phy_device *phydev, const char *name,
>>>       return ret;
>>>   }
>>>
>>> +static int an8811hb_mcu_assert(struct phy_device *phydev)
>>> +{
>>> +    int ret;
>>> +
>>> +    phy_lock_mdio_bus(phydev);
>>> +
>>> +    ret = __air_pbus_reg_write(phydev, AN8811HB_MCU_SW_RST,
>>> +                   AN8811HB_MCU_SW_RST_HOLD);
>>> +    if (ret < 0)
>>> +        goto unlock;
>>> +
>>> +    ret = __air_pbus_reg_write(phydev, 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)
>>> +{
>>> +    int ret;
>>> +
>>> +    phy_lock_mdio_bus(phydev);
>>> +
>>> +    ret = __air_pbus_reg_write(phydev, AN8811HB_MCU_SW_START,
>>> +                   AN8811HB_MCU_SW_START_EN);
>>> +    if (ret < 0)
>>> +        goto unlock;
>>> +
>>> +    ret = __air_pbus_reg_write(phydev, 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)
>>> -- 
>>> 2.34.1
>>>