[PATCH 1/1] net: phy: Add the Airoha EN8811H PHY driver

[Marek Vasut]

On 4/20/25 3:43 AM, Daniel Golle wrote:
> Hi Lucien,
> Hi Marek,

Hi,

> On Fri, Apr 18, 2025 at 11:57:19PM +0800, Lucien.Jheng wrote:
>> Hi Marek Vasut
>>
>> Thanks for the review.
>>
>> The next commit will change the loading of the PHY firmware to the
>> filesystem on a block device.
> 
> Please keep in mind that most systems using this PHY are (like the
> OpenWrt One, for example) simple embedded devices with only a small
> amount of NAND or NOR flash. Loading the firmware from a filesystem
> means overhead and complexity, while loading it from a simple
> MTD partition, raw UBI volume or one of the EMMC boot hw partitions
> is a better match to the reality of the devices it is being used on,
> making the process less complex and more deterministic, and hence
> more robust.

That works until someone else needs to load the firmware from somewhere 
else. U-Boot is not a one-board-special-hack but a generic bootloader, 
which is why special board-specific Kconfig options and other such 
ad-hoc board-specific hacks in drivers are frowned upon. Having a PHY 
driver depend on loading firmware in a non-generic manner is not helpful.

Whatever board specific firmware loading should not be in the driver, it 
should be either in board code, or in some generic firmware loader (if 
that is something someone wants to write, but that is more involved).

I am not buying the "reality of devices" argument, sorry, this is a 
generic ethernet PHY, users can use it on whatever device they populate 
it on, and they can very well chose to place the firmware into 
/lib/firmware to share it with Linux expectations or in some EEPROM .

>> [...]
>> Marek Vasut 於 2025/4/6 下午 09:30 寫道:
>>> [...]
>>>
>>> Can this do the same thing as Aquatia PHY and load the PHY firmware from
>>> filesystem on block device instead , using the blk* or fs* commands ?
>>> Then the hard-coded filesize and co. won't be necessary.
> 
> Note that the size is afaik given by the hardware in this case, it won't
> every change. Hence any handling of a more dynamic approach of loading
> the firmware will also have to come with a size-check against the (again)
> hard-coded size anyway.

If the firmware size is fixed, it shouldn't be exposed via Kconfig in 
the first place. Linux doesn't do that either, why is it done here ?

>>> If you need board specific loading override, implement __weak default
>>> loading from file on filesystem by default (see what Aquatia does) and
>>> then override the loading function on board or architecture level.
> 
> That can work, but in practice it will lead to code duplication, as all
> platforms using those PHYs are kinda similar (ie. MT7981, MT7986 and
> probably Airoha/EcoNet router SoCs as well)
Surely there is a way to factor common loader code parts into dedicated 
loader-implementation.c to avoid duplication.