Re: [PATCH] net: mdiobus: Prevent spike on MDIO bus reset signal

From: Mike Looijmans <mike.looijmans@topic.nl>
To: Andrew Lunn <andrew@lunn.ch>
Cc: netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Jakub Kicinski <kuba@kernel.org>,
	Russell King <linux@armlinux.org.uk>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] net: mdiobus: Prevent spike on MDIO bus reset signal
Date: Thu, 28 Jan 2021 09:45:41 +0100	[thread overview]
Message-ID: <956acc58-6ec8-c3d5-1310-7305c3b5a471@topic.nl> (raw)
In-Reply-To: <YBIZyWZNoQeJ7Bt4@lunn.ch>

Hi Andrew,

Response below...

Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert

TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topicproducts.com
W: www.topicproducts.com

Please consider the environment before printing this e-mail
On 28-01-2021 02:56, Andrew Lunn wrote:
> On Tue, Jan 26, 2021 at 08:33:37AM +0100, Mike Looijmans wrote:
>> The mdio_bus reset code first de-asserted the reset by allocating with
>> GPIOD_OUT_LOW, then asserted and de-asserted again. In other words, if
>> the reset signal defaulted to asserted, there'd be a short "spike"
>> before the reset.
>>
>> Instead, directly assert the reset signal using GPIOD_OUT_HIGH, this
>> removes the spike and also removes a line of code since the signal
>> is already high.
> 
> Hi Mike
> 
> Did you look at the per PHY reset? mdiobus_register_gpiod() gets the
> GPIO with GPIOD_OUT_LOW. mdiobus_register_device() then immediately
> sets it high.
> 
> So it looks like it suffers from the same problem.

Well, now that I have your attention...

The per PHY reset was more broken, it first probes the MDIO bus to see if the 
PHY is there, and only after that it controls the reset line. So if the reset 
defaults to "asserted", the PHY will not work because it didn't respond when 
the MDIO went looking for it. I haven't quite figured out how this was 
supposed to work, but at least for the case of one MDIO bus, one PHY 
configured through devicetree it didn't work as one would expect. I added a 
few printk statements to reveal that this was indeed the case.

This issue also makes the PHY hardware reset useless - if the PHY is in some 
non-responsive state, the MDIO won't get a response and report the PHY as 
missing before even attempting to assert/de-assert the reset line.

This was with a 5.4 kernel, but as far as I could see this hasn't changed 
since then.

My solution to that was to move to the MDIO bus reset, since that at least 
happened before interrogating the devices on the bus. This revealed the issue 
with the extra "spike" while evaluating that, which is something that I could 
easily solve and upstream.

Probably these issues were never dicovered because usually there's a pull-up 
of some kind on the (active-low) reset signal of the PHYs. That hides the 
spike and also hides the fact that the per-phy reset doesn't actually work. We 
only discovered the issue when we changed that to a pull-down and suddenly the 
phy failed to probe.

The way that the MDIO bus is being populated seemed rather complex to me, so 
chances of breaking things are quite high there...