Message-ID: <0c9f262b-0f06-d1d5-636e-8ad6baa3380f@gmail.com>
Date:   Fri, 24 Mar 2023 22:16:16 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Russell King - ARM Linux <linux@...linux.org.uk>,
        Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...emloft.net>,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@...adcom.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 4/4] net: phy: bcm7xxx: remove getting reference
 clock

On 24.03.2023 21:11, Florian Fainelli wrote:
> On 3/24/23 12:50, Heiner Kallweit wrote:
>> On 24.03.2023 20:03, Florian Fainelli wrote:
>>> On 3/24/23 11:05, Heiner Kallweit wrote:
>>>> Now that getting the reference clock has been moved to phylib,
>>>> we can remove it here.
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
>>>
>>> This is not the reference clock for bcm7xxx this is the SoC internal clock that feeds the Soc internal PHY.
>>
>> Ah, good to know. Then indeed we may have to allow drivers to disable this feature.
>>
>> Another aspect: When looking at ba4ee3c05365 "net: phy: bcm7xxx: request and manage GPHY clock"
>> I stumbled across statement "PHY driver can be probed with the clocks turned off".
>> I interpret this in a way that dynamic PHY detection doesn't work because the PHY ID
>> registers aren't accessible before the PHY driver has been loaded and probed. Is this right?
> 
> Yes this is correct we actually probe with the clock turned off as we try to run as low power as possible upon boot.
> 
>> Should the MDIO bus driver enable the clock for the PHY?
>>
> 
> This is what I had done in our downstream MDIO bus driver initially and this was fine because we were guaranteed to use a specific MDIO bus driver since the PHY is integrated.
> 
> Eventually when this landed upstream I went with specifying the Ethernet PHY compatible with the "ethernet-phy-idAAAA.BBBB" notation which forces the PHY library to match the compatible with the driver directly without requiring to read from MII_PHYSID1/2.
> 
> The problems I saw with the MDIO bus approach was that:
> 
> - you would have to enable the clock prior to scanning the bus which could be done in mii_bus::reset for a driver specific way of doing it, or directly in mdiobus_scan() and then you would have to balance the clock enable count within the PHY driver's probe function which required using __clk_is_enabled() to ensure the clock could be disabled later on when you unbind the PHY device from its driver or during remove, or suspend/resume
> 
> - if the PHY device tree node specified multiple clocks, you would not necessarily know which one(s) to enable and which one(s) not to. Enabling all of them would be a waste of power and could also possibly create sequencing issues if we have a situation similar to the reference clock you are trying to address. Not enabling any would obviously not work at all.
> 
> Using the "ethernet-phy-idAAAA.BBBB" ensured that the PHY driver could enable the clock(s) it needs and ensure that probe() and remove() would have balanced clock enable/disable calls.

I see, thanks for the comprehensive explanation. If we need an additional
PHY driver flag or other measures, then I wonder whether it's worth it
to add refclock handling to phylib for two drivers. Maybe not.

Powered by blists - more mailing lists