[PATCH 1/1] [U-Boot, v4, 1/1]net: phy: Add the Airoha EN8811H PHY driver

[Lucien.Jheng]

Hi Marek

Thank you for reviewing my patch.

Marek Vasut 於 2025/9/15 上午 01:49 寫道:
> On 9/14/25 6:41 AM, Lucien.Jheng wrote:
>> Add the driver for the Airoha EN8811H 2.5 Gigabit PHY. The PHY supports
>> 100/1000/2500 Mbps with auto negotiation only.
>>
>> The driver uses two firmware files, for which updated versions are 
>> added to
>> linux-firmware already.
>>
>> Locating the AIROHA FW within the filesystem at the designated partition
>> and path will trigger its automatic loading and writing to the PHY 
>> via MDIO.
>> If need board specific loading override,
>> please override the en8811h_read_fw function on board or architecture 
>> level.
>>
>> Linux upstream AIROHA EN8811H driver commit:
>> 71e79430117d56c409c5ea485a263bc0d8083390
>>
>> Based on the Linux upstream AIROHA EN8811H driver code(air_en8811h.c),
>> I have modified the relevant process to align with the U-Boot boot 
>> sequence.
>> and have validated this on Banana Pi BPI-R3 Mini.
>>
>> Signed-off-by: Lucien.Jheng <lucienzx159 at gmail.com>
>> ---
>> Change in PATCH v4:
>> air_en8811.c:
>>   * Create the airoha folder
>
> directory
>
>> to store all future airoha phy drivers.
>>   * Use `request_firmware_into_buf_via_script` for firmware loading.
>
>
> [...]
>
>> +++ b/drivers/net/phy/airoha/Kconfig
>> @@ -0,0 +1,11 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +menuconfig PHY_AIROHA
>> +    bool "Airoha Ethernet PHYs support"
>> +
>> +config PHY_AIROHA_EN8811
>> +    bool "Airoha Ethernet EN8811H support"
>> +    depends on PHY_AIROHA
>> +        select FS_LOADER
>
> It seems tabs/spaces are mixed here, please fix.
>
>> +    help
>> +      AIROHA EN8811H supported.
>
> [...]
>
>> +#define EN8811H_MD32_DM_SIZE    0x4000
>> +#define EN8811H_MD32_DSP_SIZE    0x20000
>> +
>> + #define EN8811H_FW_CTRL_1        0x0f0018
>> + #define EN8811H_FW_CTRL_1_START    0x0
>> + #define EN8811H_FW_CTRL_1_FINISH    0x1
>> + #define EN8811H_FW_CTRL_2        0x800000
>> + #define EN8811H_FW_CTRL_2_LOADING    BIT(11)
>
> Same here, avoid leading space.
>
> [...]
>
>> +__weak int en8811h_read_fw(void **addr)
>
> Does this still have to be a __weak function, now that it uses generic 
> fw loader ?
>
>> +{
>> +    void *buffer;
>> +    int ret;
>> +
>> +    if (!IS_ENABLED(CONFIG_FS_LOADER))
>> +        return -EOPNOTSUPP;
>> +
>> +    buffer = malloc(EN8811H_MD32_DM_SIZE + EN8811H_MD32_DSP_SIZE);
>> +    if (!buffer) {
>> +        printf("Failed to allocate memory for firmware\n");
>> +        return -ENOMEM;
>> +    }
>> +
>> +    ret = request_firmware_into_buf_via_script(&buffer,
>
> I think 'buffer' without the & should be used here.

Regarding buffer,

I've reviewed the function definition for 
request_firmware_into_buf_via_script.
The first parameter is void **buf, which is a pointer to a pointer.

I think passing &buffer is the correct way to call this function.

If we were to pass just buffer (a void *), the function would receive a 
copy of the pointer's value, and any memory allocation inside the 
function would not be reflected in our original buffer variable.

Please let me know if my understanding is incorrect, as I want to ensure 
I'm using the function as intended.

>
>> + EN8811H_MD32_DM_SIZE + EN8811H_MD32_DSP_SIZE,
>> +                           "en8811h_load_firmware");
> [...]