Re: [RFC net-next 2/2] net: phy: bcm7xxx: request and manage GPHY clock

[Florian Fainelli <f.fainelli@gmail.com>]

On 9/3/2020 1:09 PM, Adam Rudziński wrote:
> 
> W dniu 2020-09-03 o 21:35, Florian Fainelli pisze:
>>
>>
>> On 9/3/2020 12:21 PM, Adam Rudziński wrote:
>>>
>>> W dniu 2020-09-03 o 19:17, Florian Fainelli pisze:
>>>>
>>>>
>>>> On 9/3/2020 10:13 AM, Adam Rudziński wrote:
>>>>>
>>>>> W dniu 2020-09-03 o 17:21, Florian Fainelli pisze:
>>>>>>
>>>>>>
>>>>>> On 9/2/2020 11:00 PM, Adam Rudziński wrote:
>>>>>>>
>>>>>>> W dniu 2020-09-03 o 04:13, Florian Fainelli pisze:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 9/2/2020 3:20 PM, Andrew Lunn wrote:
>>>>>>>>>> +    priv->clk = devm_clk_get_optional(&phydev->mdio.dev, 
>>>>>>>>>> "sw_gphy");
>>>>>>>>>> +    if (IS_ERR(priv->clk))
>>>>>>>>>> +        return PTR_ERR(priv->clk);
>>>>>>>>>> +
>>>>>>>>>> +    /* To get there, the mdiobus registration logic already 
>>>>>>>>>> enabled our
>>>>>>>>>> +     * clock otherwise we would not have probed this device 
>>>>>>>>>> since we would
>>>>>>>>>> +     * not be able to read its ID. To avoid artificially 
>>>>>>>>>> bumping up the
>>>>>>>>>> +     * clock reference count, only do the clock enable from a 
>>>>>>>>>> phy_remove ->
>>>>>>>>>> +     * phy_probe path (driver unbind, then rebind).
>>>>>>>>>> +     */
>>>>>>>>>> +    if (!__clk_is_enabled(priv->clk))
>>>>>>>>>> +        ret = clk_prepare_enable(priv->clk);
>>>>>>>>>
>>>>>>>>> This i don't get. The clock subsystem does reference counting. 
>>>>>>>>> So what
>>>>>>>>> i would expect to happen is that during scanning of the bus, 
>>>>>>>>> phylib
>>>>>>>>> enables the clock and keeps it enabled until after probe. To keep
>>>>>>>>> things balanced, phylib would disable the clock after probe.
>>>>>>>>
>>>>>>>> That would be fine, although it assumes that the individual PHY 
>>>>>>>> drivers have obtained the clocks and called 
>>>>>>>> clk_prepare_enable(), which is a fair assumption I suppose.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> If the driver wants the clock enabled all the time, it can 
>>>>>>>>> enable it
>>>>>>>>> in the probe method. The common clock framework will then have two
>>>>>>>>> reference counts for the clock, so that when the probe exists, and
>>>>>>>>> phylib disables the clock, the CCF keeps the clock ticking. The 
>>>>>>>>> PHY
>>>>>>>>> driver can then disable the clock in .remove.
>>>>>>>>
>>>>>>>> But then the lowest count you will have is 1, which will lead to 
>>>>>>>> the clock being left on despite having unbound the PHY driver 
>>>>>>>> from the device (->remove was called). This does not allow 
>>>>>>>> saving any power unfortunately.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> There are some PHYs which will enumerate with the clock 
>>>>>>>>> disabled. They
>>>>>>>>> only need it ticking for packet transfer. Such PHY drivers can 
>>>>>>>>> enable
>>>>>>>>> the clock only when needed in order to save some power when the
>>>>>>>>> interface is administratively down.
>>>>>>>>
>>>>>>>> Then the best approach would be for the OF scanning code to 
>>>>>>>> enable all clocks reference by the Ethernet PHY node (like it 
>>>>>>>> does in the proposed patch), since there is no knowledge of 
>>>>>>>> which clock is necessary and all must be assumed to be critical 
>>>>>>>> for MDIO bus scanning. Right before drv->probe() we drop all 
>>>>>>>> resources reference counts, and from there on ->probe() is 
>>>>>>>> assumed to manage the necessary clocks.
>>>>>>>>
>>>>>>>> It looks like another solution may be to use the assigned-clocks 
>>>>>>>> property which will take care of assigning clock references to 
>>>>>>>> devices and having those applied as soon as the clock provider 
>>>>>>>> is available.
>>>>>>>
>>>>>>> Hi Guys,
>>>>>>>
>>>>>>> I've just realized that a PHY may also have a reset signal 
>>>>>>> connected. The reset signal may be controlled by the dedicated 
>>>>>>> peripheral or by GPIO.
>>>>>>
>>>>>> There is already support for such a thing within 
>>>>>> drivers/net/phy/mdio_bus.c though it assumes we could bind the PHY 
>>>>>> device to its driver already.
>>>>>>
>>>>>>>
>>>>>>> In general terms, there might be a set of control signals needed 
>>>>>>> to enable the PHY. It seems that the clock and the reset would be 
>>>>>>> the typical useful options.
>>>>>>>
>>>>>>> Going further with my imagination of how evil the hardware design 
>>>>>>> could be, in general the signals for the PHY may have some 
>>>>>>> relations to other control signals.
>>>>>>>
>>>>>>> I think that from the software point of view this comes down to 
>>>>>>> assumption that the PHY is to be controlled "driver only knows how".
>>>>>>
>>>>>> That is all well and good as long as we can actually bind the PHY 
>>>>>> device which its driver, and right now this means that we either 
>>>>>> have:
>>>>>>
>>>>>> - a compatible string in Device Tree which is of the form 
>>>>>> ethernet-phy-id%4x.%4x (see of_get_phy_id) which means that we 
>>>>>> *know* already which PHY we have and we avoid doing reads of 
>>>>>> MII_PHYSID1 and MII_PHYSID2. This is a Linux implementation detail 
>>>>>> that should not have to be known to systems designer IMHO
>>>>>>
>>>>>> - a successful read of MII_PHYSID1 and MII_PHYSID2 (or an 
>>>>>> equivalent for the clause 45 PHYs) that allows us to know what PHY 
>>>>>> device we have, which is something that needs to happen eventually.
>>>>>>
>>>>>> The problem is when there are essential resources such as clocks, 
>>>>>> regulators, reset signals that must be enabled, respectively 
>>>>>> de-asserted in order for a successful MDIO read of MII_PHYSID1 and 
>>>>>> MII_PHYSID2 to succeed.
>>>>>>
>>>>>> There is no driver involvement at that stage because we have no 
>>>>>> phy_device to bind it to *yet*. Depending on what we read from 
>>>>>> MII_PHYSID1/PHY_ID2 we will either successfully bind to the 
>>>>>> Generic PHY driver (assuming we did not read all Fs) or not and we 
>>>>>> will return -ENODEV and then it is game over.
>>>>>>
>>>>>> This is the chicken and egg problem that this patch series is 
>>>>>> addressing, for clocks, because we can retrieve clock devices with 
>>>>>> just a device_node reference.
>>>>>
>>>>> I have an impression that here the effort goes in the wrong 
>>>>> direction. If I understand correctly, the goal is to have the 
>>>>> kernel find out what will the driver need to use the phy. But, the 
>>>>> kernel anyway calls a probe function of the driver, doesn't it? To 
>>>>> me it looks as if you were trying to do something that the driver 
>>>>> will/would/might do later, and possibly "undo" it in the meantime. 
>>>>> In this regard, this becomes kind of a workaround, not solution of 
>>>>> the problem. Also, having taken a glance at your previous messages, 
>>>>> I can tell that this is all becoming even more complex.
>>>>
>>>> What is the problem according to you, and what would an acceptable 
>>>> solution look like then?
>>>>
>>>>>
>>>>> I think that the effort should be to allow any node in the device 
>>>>> tree to take care about its child nodes by itself, and just 
>>>>> "report" to the kernel, or "install" in the kernel whatever is 
>>>>> necessary, but without any initiative of the kernel. Let the 
>>>>> drivers be as complicated as necessary, not the kernel.
>>>>
>>>> The Device Tree representation is already correct in that it lists 
>>>> clocks/regulators/reset signals that are needed by the PHY. The 
>>>> problem is its implementation with the Linux device driver model. 
>>>> Please read again what I wrote.
>>>
>>> The problem which I had, was that kernel was unable to read ID of 
>>> second PHY, because it needed to have the clock enabled, and during 
>>> probing it still didn't have. I don't know what problems are 
>>> addressed by the discussed patches - only the same one, or if 
>>> something else too.
>>
>> That is precisely the problem being addressed here, an essential clock 
>> to the PHY must be enabled for the PHY to be accessible on the MDIO bus.
>>
>>> In my solution, of my problem, which now works well for me, instead 
>>> of teaching kernel any new tricks, I've taught the kernel to listen 
>>> to the driver, by adding a function allowing the driver to register 
>>> its PHY on the MDIO bus at any time. Then, each driver instance (for 
>>> a particular interface) configures whatever is necessary, finds its 
>>> PHY when probing the shared MDIO bus, and tells the kernel(?) to add 
>>> it to the shared MDIO bus. Since each one does that, when the time 
>>> comes, all PHYs are known and all interfaces are up.
>>
>> Except this is bending the Linux driver model backwards, a driver does 
>> not register devices, a bus does, and then it binds the driver to that 
>> device object. For the bus to scan a hardware device, said hardware 
>> device must be in a functional state to be discovered.
> 
> OK. Probably I'd better just have said that all the mentioned actions 
> were a consequence of calling the driver's probe function (fec_probe() 
> in the example of IMX FEC driver) and were not relying on any 
> pre-configured features. My description in fact was very... imprecise.

Yes it was unfortunately, and it is still a complete layering violation, 
the FEC should provide a MDIO bus instance with read/write operations, 
and that is all that should be needed. There should be no poking into 
its Device Tree nodes as much as possible.
-- 
Florian