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

[Wei Fang]

> -----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? 


> Best regards,
> Krzysztof