Message-ID: <88a39d9c-1c89-3c05-c4d3-063e73c14cfc@gmail.com>
Date:   Fri, 24 Mar 2023 14:19:40 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Heiner Kallweit <hkallweit1@...il.com>,
        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 3/24/23 14:16, Heiner Kallweit wrote:
> 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.

Agreed that two drivers may not be that many. Situations like stmmac or 
other drivers where there may be a need for the PHY clock to run for the 
MAC's RX path to complete initializing might be solved using existing 
PHY library routines.
-- 
Florian

Powered by blists - more mailing lists