Re: [PATCH net 1/2] dt: ar803x: Document disable-hibernation property

[Russell King (Oracle)]

On Mon, Aug 15, 2022 at 08:51:32AM +0000, Wei Fang wrote:
> 
> 
> > -----Original Message-----
> > From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> > Sent: 2022年8月12日 19:26
> > To: Wei Fang <wei.fang@xxxxxxx>; andrew@xxxxxxx; hkallweit1@xxxxxxxxx;
> > linux@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx;
> > kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; robh+dt@xxxxxxxxxx;
> > krzysztof.kozlowski+dt@xxxxxxxxxx; f.fainelli@xxxxxxxxx;
> > netdev@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH net 1/2] dt: ar803x: Document disable-hibernation
> > property
> > 
> > On 12/08/2022 12:02, Wei Fang wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> > >> Sent: 2022年8月12日 15:28
> > >> To: Wei Fang <wei.fang@xxxxxxx>; andrew@xxxxxxx;
> > >> hkallweit1@xxxxxxxxx; linux@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx;
> > >> edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx;
> > >> robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx;
> > >> f.fainelli@xxxxxxxxx; netdev@xxxxxxxxxxxxxxx;
> > >> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> > >> Subject: Re: [PATCH net 1/2] dt: ar803x: Document disable-hibernation
> > >> property
> > >>
> > >> On 12/08/2022 17:50, wei.fang@xxxxxxx wrote:
> > >>> From: Wei Fang <wei.fang@xxxxxxx>
> > >>>
> > >>
> > >> Please use subject prefix matching subsystem.
> > >>
> > > Ok, I'll add the subject prefix.
> > >
> > >>> The hibernation mode of Atheros AR803x PHYs is default enabled.
> > >>> When the cable is unplugged, the PHY will enter hibernation mode and
> > >>> the PHY clock does down. For some MACs, it needs the clock to
> > >>> support it's logic. For instance, stmmac needs the PHY inputs clock
> > >>> is present for software reset completion. Therefore, It is
> > >>> reasonable to add a DT property to disable hibernation mode.
> > >>>
> > >>> Signed-off-by: Wei Fang <wei.fang@xxxxxxx>
> > >>> ---
> > >>>  Documentation/devicetree/bindings/net/qca,ar803x.yaml | 6 ++++++
> > >>>  1 file changed, 6 insertions(+)
> > >>>
> > >>> diff --git a/Documentation/devicetree/bindings/net/qca,ar803x.yaml
> > >>> b/Documentation/devicetree/bindings/net/qca,ar803x.yaml
> > >>> index b3d4013b7ca6..d08431d79b83 100644
> > >>> --- a/Documentation/devicetree/bindings/net/qca,ar803x.yaml
> > >>> +++ b/Documentation/devicetree/bindings/net/qca,ar803x.yaml
> > >>> @@ -40,6 +40,12 @@ properties:
> > >>>        Only supported on the AR8031.
> > >>>      type: boolean
> > >>>
> > >>> +  qca,disable-hibernation:
> > >>> +    description: |
> > >>> +    If set, the PHY will not enter hibernation mode when the cable is
> > >>> +    unplugged.
> > >>
> > >> Wrong indentation. Did you test the bindings?
> > >>
> > > Sorry, I just checked the patch and forgot to check the dt-bindings.
> > >
> > >> Unfortunately the property describes driver behavior not hardware, so
> > >> it is not suitable for DT. Instead describe the hardware
> > >> characteristics/features/bugs/constraints. Not driver behavior. Both
> > >> in property name and property description.
> > >>
> > > Thanks for your review and feedback. Actually, the hibernation mode is a
> > feature of hardware, I will modify the property name and description to be
> > more in line with the requirements of the DT property.
> > 
> > hibernation is a feature, but 'disable-hibernation' is not. DTS describes the
> > hardware, not policy or driver bejhvior. Why disabling hibernation is a property
> > of hardware? How you described, it's not, therefore either property is not for
> > DT or it has to be phrased correctly to describe the hardware.
> > 
> Sorry, I'm a little confused. Hibernation mode is a feature of PHY hardware, the
> mode defaults to be enabled. We can configure it enabled or not through the
> register which the PHY provided. Now some MACs need the PHY clocks always
> output a valid clock so that MACs can operate correctly. And I add the property
> to disable hibernation mode to avoid this case. And I noticed that there are 
> some similar properties existed in the qca,ar803x,yaml, such as
> "qca,disable-smarteee" and "qca,keep-pll-enabled". So why can't I use the
> "qca,disable-hibernation" property? How should I do? 

To me, your proposal is entirely reasonable and in keeping with DT's
mandate, which is to describe not only how the hardware is connected
but also how the hardware needs to be configured to inter-operate. I
don't see any need for you to change your proposed property.

It is, as you point out above, no different from these other
properties that configure the PHY's internal settings.

I think Krzysztof is confusing the term "hibernation" with the system
level action, and believing that this property has something to do
with preventing the driver doing something when the system enters
that state. I can't fathom any other explanation.

Maybe changing the description of the property would help:

+  qca,disable-hibernation:
+    description: |
+      Configure the PHY to disable hardware hibernation power saving
+      mode, which is entered when the cable is disconnected.
+    type: boolean

Also, I'd suggest also fixing up the patch description to stress that
this is a hardware feature:

"The hardware hibernation mode of Atheros AR803x PHYs is defaults to
being enabled at hardware reset. When the cable is unplugged, the PHY
will enter hibernation mode after a delay and the PHY clock output to
the MAC can be stopped to save power.

"However, some MACs need this clock for proper functioning of their
logic. For instance, stmmac needs the PHY clock for software reset to
complete.

"Therefore, add a DT property to configure the PHY to disable this
hardware hibernation mode."

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!