Subject: Re: [PATCH net 2/4] net: dsa: mv88e6xxx: Give chips more time to activate their PPUs

From: Tobias Waldekranz <[email-masked]>
Date: Fri, 06 Dec 2024 14:39:25 +0100

On fre, dec 06, 2024 at 14:18, Andrew Lunn <[email-masked]> wrote:
> On Fri, Dec 06, 2024 at 02:07:34PM +0100, Tobias Waldekranz wrote:
>> In a daisy-chain of three 6393X devices, delays of up to 750ms are
>> sometimes observed before completion of PPU initialization (Global 1,
>> register 0, bit 15) is signaled. Therefore, allow chips more time
>> before giving up.
>>  static int mv88e6352_g1_wait_ppu_polling(struct mv88e6xxx_chip *chip)
>>  {
>>  	int bit = __bf_shf(MV88E6352_G1_STS_PPU_STATE);
>> +	int err, i;
>>  
>> -	return mv88e6xxx_g1_wait_bit(chip, MV88E6XXX_G1_STS, bit, 1);
>> +	for (i = 0; i < 20; i++) {
>> +		err = _mv88e6xxx_wait_bit(chip, chip->info->global1_addr,
>> +					  MV88E6XXX_G1_STS, bit, 1, NULL);
>> +		if (err != -ETIMEDOUT)
>> +			break;
>> +	}
>
> The commit message does not indicate why it is necessary to swap to
> _mv88e6xxx_wait_bit().

It is not strictly necessary, I just wanted to avoid flooding the logs
with spurious timeout errors. Do you want me to update the message?

>> +
>> +	if (err) {
>> +		dev_err(chip->dev, "PPU did not come online: %d\n", err);
>> +		return err;
>> +	}
>> +
>> +	if (i)
>> +		dev_warn(chip->dev,
>> +			 "PPU was slow to come online, retried %d times\n", i);
>
> dev_dbg()? Does the user care if it took longer than one loop
> iteration?

My resoning was: While it does seem fine that the device takes this long
to initialize, if it turns out that this is an indication of some bigger
issue, it might be good to have it recorded in the log.