Re: [PATCH net-next 2/3] net: phy: aquantia: report PHY details like firmware version

[Heiner Kallweit <hkallweit1@gmail.com>]

On 24.03.2019 02:04, Florian Fainelli wrote:
> 
> 
> On 3/23/2019 6:15 AM, Heiner Kallweit wrote:
>> Add reporting firmware details. These details are available only once
>> the firmware has finished initializing the chip. This can take some
>> time and we need to poll for init completion.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
> 
> [snip]
>>  
>> +/* If we configure settings whilst firmware is still initializing the chip,
>> + * then these settings may be overwritten. Therefore make sure chip
>> + * initialization has completed. Use presence of the firmware ID as
>> + * indicator for initialization having completed.
>> + * The chip also provides a "reset completed" bit, but it's cleared after
>> + * read. Therefore function would time out if called again.
>> + */
> 
> Is there a way to say, run a checksum calculation on the firmware image
> to assess its health/validity as well as read the firmware ID? What
> happens if the PHY is not provisioned with a firmware image? Is it
> expecting for a specific set of MDIO vendor commands to load it over MDIO?
> 
It's also possible to load firmware over MDIO, but typically an internal
boot loader reads the firmware directly from a SPI NOR flash.
Vendor registers allow access to the flash and it should be possible to
expose the flash as MTD, but that's a different exercise (I think some
experimental work has been done in that direction already).
Based on the datasheet I'm not sure whether the PHY can work w/o firmware.
The firmware is more than the provisioned register defaults.


>> +static void aqr107_wait_reset_complete(struct phy_device *phydev)
>> +{
>> +	int val, retries = 100;
>> +
>> +	do {
>> +		val = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_FW_ID);
>> +		if (val < 0)
>> +			return;
>> +		msleep(20);
>> +	} while (!val && --retries);
> 
> Should not this return 0/-ETIMEDOUT and have the caller propagate that
> error code?
> 
Yes, most likely it's better to do this. I'll add it in a v2.