Date:   Thu, 3 Sep 2020 08:00:45 +0200
From:   Adam Rudziński <adam.rudzinski@....net.pl>
To:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org, m.felsch@...gutronix.de,
        hkallweit1@...il.com, richard.leitner@...data.com,
        zhengdejin5@...il.com, devicetree@...r.kernel.org,
        kernel@...gutronix.de, kuba@...nel.org, robh+dt@...nel.org
Subject: Re: [RFC net-next 2/2] net: phy: bcm7xxx: request and manage GPHY
 clock


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.

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".

Best regards,
Adam

Powered by blists - more mailing lists