Re: [PATCH] net: mdio: force deassert MDIO reset signal - Lars-Peter Clausen

From: Lars-Peter Clausen <lars@metafoo.de>
To: Pierluigi Passaro <pierluigi.passaro@gmail.com>
Cc: Andrew Lunn <andrew@lunn.ch>,
	hkallweit1@gmail.com, linux@armlinux.org.uk, davem@davemloft.net,
	edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	eran.m@variscite.com, nate.d@variscite.com,
	francesco.f@variscite.com, pierluigi.p@variscite.com
Subject: Re: [PATCH] net: mdio: force deassert MDIO reset signal
Date: Sun, 15 Jan 2023 14:56:22 -0800	[thread overview]
Message-ID: <54dd8952-dd39-cab2-8716-cd59d3266d1a@metafoo.de> (raw)
In-Reply-To: <CAJ=UCjUo3t+D9S=J_yEhxCOo5OMj3d-UW6Z6HdwY+O+Q6JO0+A@mail.gmail.com>

On 1/15/23 14:33, Pierluigi Passaro wrote:
> On Sun, Jan 15, 2023 at 10:59 PM Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 1/15/23 09:08, Andrew Lunn wrote:
>>> On Sun, Jan 15, 2023 at 05:10:06PM +0100, Pierluigi Passaro wrote:
>>>> When the reset gpio is defined within the node of the device tree
>>>> describing the PHY, the reset is initialized and managed only after
>>>> calling the fwnode_mdiobus_phy_device_register function.
>>>> However, before calling it, the MDIO communication is checked by the
>>>> get_phy_device function.
>>>> When this happen and the reset GPIO was somehow previously set down,
>>>> the get_phy_device function fails, preventing the PHY detection.
>>>> These changes force the deassert of the MDIO reset signal before
>>>> checking the MDIO channel.
>>>> The PHY may require a minimum deassert time before being responsive:
>>>> use a reasonable sleep time after forcing the deassert of the MDIO
>>>> reset signal.
>>>> Once done, free the gpio descriptor to allow managing it later.
>>> This has been discussed before. The problem is, it is not just a reset
>>> GPIO. There could also be a clock which needs turning on, a regulator,
>>> and/or a linux reset controller. And what order do you turn these on?
>>>
>>> The conclusions of the discussion is you assume the device cannot be
>>> found by enumeration, and you put the ID in the compatible. That is
>>> enough to get the driver to load, and the driver can then turn
>>> everything on in the correct order, with the correct delays, etc.
>> I've been running into this same problem again and again over the past
>> years.
>>
>> Specifying the ID as part of the compatible string works for clause 22
>> PHYs, but for clause 45 PHYs it does not work. The code always wants to
>> read the ID from the PHY itself. But I do not understand things well
>> enough to tell whether that's a fundamental restriction of C45 or just
>> our implementation and the implementation can be changed to fix it.
>>
>> Do you have some thoughts on this?
>>
> IMHO, since the framework allows defining the reset GPIO, it does not sound
> reasonable to manage it only after checking if the PHY can communicate:
> if the reset is asserted, the PHY cannot communicate at all.
> This patch just ensures that, if the reset GPIO is defined, it's not asserted
> while checking the communication.

I fully agree with you and I think this is the right approach, cause it 
is required to make systems work. But I've seen two attempts in the past 
that did the very same thing and they always got rejected. I can't find 
the patches anymore, but I think one was maybe 2 years ago.