Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
To:     Saravana Kannan <saravanak@google.com>
Cc:     Andrew Lunn <andrew@lunn.ch>, Russell King <linux@armlinux.org.uk>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Android Kernel Team <kernel-team@android.com>,
        netdev <netdev@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Jon Hunter <jonathanh@nvidia.com>
References: <CAGETcx9YpCUMmHjyydMtOJP9SKBbVsHNB-9SspD9u=txJ12Gug@mail.gmail.com>
 <YCRjmpKjK0pxKTCP@lunn.ch>
 <CAGETcx-tBw_=VPvQVYcpPJBJjgQvp8UASrdMdSbSduahZpJf9w@mail.gmail.com>
 <4f0086ad-1258-063d-0ace-fe4c6c114991@gmail.com>
 <CAGETcx_9bmeLzOvDp8eCGdWtfwZNajCBCNSbyx7a_0T=FcSvwA@mail.gmail.com>
From:   Heiner Kallweit <hkallweit1@gmail.com>
Subject: Re: phy_attach_direct()'s use of device_bind_driver()
Message-ID: <2c5c95d8-07f3-1617-f98e-8b0a57dd1c97@gmail.com>
Date:   Thu, 11 Feb 2021 10:29:07 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.7.1
MIME-Version: 1.0
In-Reply-To: <CAGETcx_9bmeLzOvDp8eCGdWtfwZNajCBCNSbyx7a_0T=FcSvwA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 11.02.2021 09:57, Saravana Kannan wrote:
> On Wed, Feb 10, 2021 at 11:31 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>
>> On 11.02.2021 00:29, Saravana Kannan wrote:
>>> On Wed, Feb 10, 2021 at 2:52 PM Andrew Lunn <andrew@lunn.ch> wrote:
>>>>
>>>> On Wed, Feb 10, 2021 at 02:13:48PM -0800, Saravana Kannan wrote:
>>>>> Hi,
>>>>>
>>>>> This email was triggered by this other email[1].
>>>>>
>>>>> Why is phy_attach_direct() directly calling device_bind_driver()
>>>>> instead of using bus_probe_device()?
>>>>
>>>> Hi Saravana
>>>>
>>>> So this is to do with the generic PHY, which is a special case.
>>>>
>>>> First the normal case. The MDIO bus driver registers an MDIO bus using
>>>> mdiobus_register(). This will enumerate the bus, finding PHYs on
>>>> it. Each PHY device is registered with the device core, using the
>>>> usual device_add(). The core will go through the registered PHY
>>>> drivers and see if one can drive this hardware, based on the ID
>>>> registers the PHY has at address 2 and 3. If a match is found, the
>>>> driver probes the device, all in the usual way.
>>>>
>>>> Sometime later, the MAC driver wants to make use of the PHY
>>>> device. This is often in the open() call of the MAC driver, when the
>>>> interface is configured up. The MAC driver asks phylib to associate a
>>>> PHY devices to the MAC device. In the normal case, the PHY has been
>>>> probed, and everything is good to go.
>>>>
>>>> However, sometimes, there is no driver for the PHY. There is no driver
>>>> for that hardware. Or the driver has not been built, or it is not on
>>>> the disk, etc. So the device core has not been able to probe
>>>> it. However, IEEE 802.3 clause 22 defines a minimum set of registers a
>>>> PHY should support. And most PHY devices have this minimum. So there
>>>> is a fall back driver, the generic PHY driver. It assumes the minimum
>>>> registers are available, and does its best to drive the hardware. It
>>>> often works, but not always. So if the MAC asks phylib to connect to a
>>>> PHY which does not have a driver, we forcefully bind the generic
>>>> driver to the device, and hope for the best.
>>>
>>> Thanks for the detailed answer Andrew! I think it gives me enough
>>> info/context to come up with a proper fix.
>>>
>>>> We don't actually recommend using the generic driver. Use the specific
>>>> driver for the hardware. But the generic driver can at least get you
>>>> going, allow you to scp the correct driver onto the system, etc.
>>>
>>> I'm not sure if I can control what driver they use. If I can fix this
>>> warning, I'll probably try to do that.
>>>
>> The genphy driver is a last resort, at least they lose functionality like
>> downshift detection and control. Therefore they should go with the
>> dedicated Marvell PHY driver.
>>
>> But right, this avoids the warning, but the underlying issue (probably
>> in device_bind_driver()) still exists. Would be good if you can fix it.
> 
> Yeah, I plan to fix this. So I have a few more questions. In the
> example I gave, what should happen if the gpios listed in the phy's DT
> node aren't ready yet? The generic phy driver itself probably isn't
> using any GPIO? But will the phy work without the GPIO hardware being
> initialized? The reason I'm asking this question is, if the phy is
> linked to a supplier and the supplier is not ready, should the
> device_bind_driver() succeed or not?
> 

There may be situations where the gpio is used for the PHY reset and
default state is "reset assigned". If we can't control the reset signal
then PHY isn't usable. Therefore I'm inclined to say we should not
succeed. Let's see which opinions Andrew and Russell have.

However I have a little bit of a hard time to imagine how this scenario
can happen. device_bind_driver(), as part of phy_attach_direct(),
is typically called from ndo_open() of the net device, like in
your stmmac case. Means user space would open the network interface
before the gpio controller has even been probed.

> -Saravana
>