Received-SPF: pass (google.com: domain of linux-kernel+bounces-101448-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Date: Wed, 13 Mar 2024 13:52:02 +0200
From: Justin Swartz <justin.swartz@risingedge.co.za>
To: =?UTF-8?Q?Ar=C4=B1n=C3=A7_=C3=9CNAL?= <arinc.unal@arinc9.com>
Cc: Daniel Golle <daniel@makrotopia.org>, DENG Qingfang <dqfext@gmail.com>,
 Sean Wang <sean.wang@mediatek.com>, Andrew Lunn <andrew@lunn.ch>, Florian
 Fainelli <f.fainelli@gmail.com>, Vladimir Oltean <olteanv@gmail.com>, "David
 S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub
 Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Matthias
 Brugger <matthias.bgg@gmail.com>, AngeloGioacchino Del Regno
 <angelogioacchino.delregno@collabora.com>, netdev@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
 linux-mediatek@lists.infradead.org
Subject: Re: [PATCH] net: dsa: mt7530: increase reset hold time
In-Reply-To: <bf0ed70f-e106-4a7f-a98c-de34658feb4d@arinc9.com>
References: <f594a72a91d12f1d027a06962caa9750@risingedge.co.za>
 <20240312192117.7789-1-justin.swartz@risingedge.co.za>
 <bf0ed70f-e106-4a7f-a98c-de34658feb4d@arinc9.com>
Message-ID: <e6525cac666a033a5866465abb3e63f1@risingedge.co.za>
User-Agent: Roundcube Webmail/1.3.17


On 2024-03-13 10:59, Arınç ÜNAL wrote:
> On 12.03.2024 22:21, Justin Swartz wrote:
>> Increase the MT7530 reset hold period from 1000-1100 usec
>> to 5000-5100 usec.
>> 
>> This should reduce the likelihood of an incorrect external
>> crystal frequency selection which may occur when reset is
>> deasserted too early under certain link conditions.
>> 
>> Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za>
>> ---
>>   drivers/net/dsa/mt7530.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 3c1f65759..5e9e1381a 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -2243,11 +2243,11 @@ mt7530_setup(struct dsa_switch *ds)
>>   	 */
>>   	if (priv->mcm) {
>>   		reset_control_assert(priv->rstc);
>> -		usleep_range(1000, 1100);
>> +		usleep_range(5000, 5100);
>>   		reset_control_deassert(priv->rstc);
>>   	} else {
>>   		gpiod_set_value_cansleep(priv->reset, 0);
>> -		usleep_range(1000, 1100);
>> +		usleep_range(5000, 5100);
>>   		gpiod_set_value_cansleep(priv->reset, 1);
>>   	}
>> 
> 
> Again, the patch subject is missing the indication of a tree. Read the
> networking subsystem (netdev) development page [1].

Thanks for that pointer.


> This ship has sailed anyway. Now the changes the first patch did must 
> be
> reverted too. I will deal with this from now on, you can stop sending
> patches regarding this.

At least if the first patch isn't reverted, the approach used is
less likely to result in the problem occuring, IMHO.

The delay between deliberately switching the LEDs off, instead of
only waiting on chip reset logic to handle that, and the reset
assertion could be considered a "reset setup" period to complement
the original reset hold period.

Increasing the hold period to what should be 5000 - 5100 usec,
definitely made the problem go away my testing, but the previous
approach is (if nothing else) more explicit in its intent.


> [1] 
> https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html
> 
> Arınç