[PATCH net 0/2] Add DT property to disable hibernation mode

[PATCH net 0/2] Add DT property to disable hibernation mode

[wei.fang]

From: Wei Fang <wei.fang@nxp.com>

The patches add the ability to disable the hibernation mode of AR803x
PHYs. Hibernation mode defaults to enabled after hardware reset on
these PHYs. If the AR803x PHYs enter hibernation mode, they will not
provide any clock. For some MACs, they might need the clocks which
provided by the PHYs to support their own hardware logic.
So, the patches add the supoort to disable hibernation mode by adding
a boolean:
	qca,disable-hibernation
If one wished to disable hibernation mode to better match with the
specifical MAC, just add this property in the phy node of DT.

Wei Fang (2):
  dt: ar803x: Document disable-hibernation property
  net: phy: at803x: Add disable hibernation mode support

 .../devicetree/bindings/net/qca,ar803x.yaml   |  6 +++++
 drivers/net/phy/at803x.c                      | 25 +++++++++++++++++++
 2 files changed, 31 insertions(+)

-- 
2.25.1

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

[wei.fang]

From: Wei Fang <wei.fang@nxp.com>

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@nxp.com>
---
 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.
+    type: boolean
+
   qca,smarteee-tw-us-100m:
     description: EEE Tw parameter for 100M links.
     $ref: /schemas/types.yaml#/definitions/uint32
-- 
2.25.1

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

[Krzysztof Kozlowski]

On 12/08/2022 17:50, wei.fang@nxp.com wrote:
> From: Wei Fang <wei.fang@nxp.com>
> 

Please use subject prefix matching subsystem.

> 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@nxp.com>
> ---
>  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?

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.

Best regards,
Krzysztof

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

[Wei Fang]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: 2022年8月12日 15:28
> To: Wei Fang <wei.fang@nxp.com>; andrew@lunn.ch; hkallweit1@gmail.com;
> linux@armlinux.org.uk; davem@davemloft.net; edumazet@google.com;
> kuba@kernel.org; pabeni@redhat.com; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; f.fainelli@gmail.com;
> netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net 1/2] dt: ar803x: Document disable-hibernation
> property
> 
> On 12/08/2022 17:50, wei.fang@nxp.com wrote:
> > From: Wei Fang <wei.fang@nxp.com>
> >
> 
> 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@nxp.com>
> > ---
> >  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. 

> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

On 12/08/2022 12:02, Wei Fang wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: 2022年8月12日 15:28
>> To: Wei Fang <wei.fang@nxp.com>; andrew@lunn.ch; hkallweit1@gmail.com;
>> linux@armlinux.org.uk; davem@davemloft.net; edumazet@google.com;
>> kuba@kernel.org; pabeni@redhat.com; robh+dt@kernel.org;
>> krzysztof.kozlowski+dt@linaro.org; f.fainelli@gmail.com;
>> netdev@vger.kernel.org; devicetree@vger.kernel.org;
>> linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH net 1/2] dt: ar803x: Document disable-hibernation
>> property
>>
>> On 12/08/2022 17:50, wei.fang@nxp.com wrote:
>>> From: Wei Fang <wei.fang@nxp.com>
>>>
>>
>> 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@nxp.com>
>>> ---
>>>  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.

Best regards,
Krzysztof

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

[Russell King (Oracle)]

On Fri, Aug 12, 2022 at 02:25:42PM +0300, Krzysztof Kozlowski wrote:
> 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.

However, older DT descriptions need to be compatible with later kernels,
and as existing setups have hibernation enabled, introducing a property
to _enable_ hibernation (which implies if the property is not present,
hibernation is disabled) changes the behaviour with older DT, thereby
breaking backwards compatibility.

Yes, DT needs to describe hardware, but there are also other
constraints too.

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

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

[Krzysztof Kozlowski]

On 12/08/2022 14:36, Russell King (Oracle) wrote:
> On Fri, Aug 12, 2022 at 02:25:42PM +0300, Krzysztof Kozlowski wrote:
>> 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.
> 
> However, older DT descriptions need to be compatible with later kernels,
> and as existing setups have hibernation enabled, introducing a property
> to _enable_ hibernation (which implies if the property is not present,
> hibernation is disabled) changes the behaviour with older DT, thereby
> breaking backwards compatibility.
> 
> Yes, DT needs to describe hardware, but there are also other
> constraints too.

I did not propose a property to enable hibernation. The property must
describe hardware, so this piece is missing, regardless whether the
logic in the driver is "enable" or "disable".

The hardware property for example is: "broken foo, so hibernation should
be disabled" or "engineer forgot to wire cables, so hibernation won't
work"...

Best regards,
Krzysztof

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

[Russell King (Oracle)]

On Fri, Aug 12, 2022 at 03:04:41PM +0300, Krzysztof Kozlowski wrote:
> I did not propose a property to enable hibernation. The property must
> describe hardware, so this piece is missing, regardless whether the
> logic in the driver is "enable" or "disable".
> 
> The hardware property for example is: "broken foo, so hibernation should
> be disabled" or "engineer forgot to wire cables, so hibernation won't
> work"...

From the problem description, the PHY itself isn't broken. The stmmac
hardware doesn't reset properly when the clock from the PHY is stopped.
That could hardly be described as "broken" - it's quite common for
hardware specifications to state that clocks must be running for the
hardware to operate correctly.

This is a matter of configuring the hardware to inter-operate correctly.
Isn't that the whole purpose of DT?

So, nothing is broken. Nothing has forgotten to be wired. It's a matter
of properly configuring the hardware. Just the same as selecting the
correct interface mode to connect two devices together.

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

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

[Krzysztof Kozlowski]

On 12/08/2022 15:30, Russell King (Oracle) wrote:
> On Fri, Aug 12, 2022 at 03:04:41PM +0300, Krzysztof Kozlowski wrote:
>> I did not propose a property to enable hibernation. The property must
>> describe hardware, so this piece is missing, regardless whether the
>> logic in the driver is "enable" or "disable".
>>
>> The hardware property for example is: "broken foo, so hibernation should
>> be disabled" or "engineer forgot to wire cables, so hibernation won't
>> work"...
> 
> From the problem description, the PHY itself isn't broken. The stmmac
> hardware doesn't reset properly when the clock from the PHY is stopped.

There is nothing like that in the property name or property description.
Again - DT is not for describing driver behavior or driver policy.

> That could hardly be described as "broken" - it's quite common for
> hardware specifications to state that clocks must be running for the
> hardware to operate correctly.
> 
> This is a matter of configuring the hardware to inter-operate correctly.
> Isn't that the whole purpose of DT?
> 
> So, nothing is broken. Nothing has forgotten to be wired. It's a matter
> of properly configuring the hardware. Just the same as selecting the
> correct interface mode to connect two devices together.

I just gave you two examples what could be written, don't need to stick
them. You can use some real one...

Best regards,
Krzysztof

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

[Russell King (Oracle)]

On Fri, Aug 12, 2022 at 03:36:43PM +0300, Krzysztof Kozlowski wrote:
> On 12/08/2022 15:30, Russell King (Oracle) wrote:
> > On Fri, Aug 12, 2022 at 03:04:41PM +0300, Krzysztof Kozlowski wrote:
> >> I did not propose a property to enable hibernation. The property must
> >> describe hardware, so this piece is missing, regardless whether the
> >> logic in the driver is "enable" or "disable".
> >>
> >> The hardware property for example is: "broken foo, so hibernation should
> >> be disabled" or "engineer forgot to wire cables, so hibernation won't
> >> work"...
> > 
> > From the problem description, the PHY itself isn't broken. The stmmac
> > hardware doesn't reset properly when the clock from the PHY is stopped.
> 
> There is nothing like that in the property name or property description.
> Again - DT is not for describing driver behavior or driver policy.

Where have I said that it's describing driver behaviour or policy?
Hint: I haven't.

I have no idea why DT maintainers like to keep shoving that idiotic
statement down people's thoats. WE KNOW THIS. And that is NOT what
is being proposed here.

It is purely in your mind that this is a driver behaviour or driver
policy property. It *isn't*.

> > That could hardly be described as "broken" - it's quite common for
> > hardware specifications to state that clocks must be running for the
> > hardware to operate correctly.
> > 
> > This is a matter of configuring the hardware to inter-operate correctly.
> > Isn't that the whole purpose of DT?
> > 
> > So, nothing is broken. Nothing has forgotten to be wired. It's a matter
> > of properly configuring the hardware. Just the same as selecting the
> > correct interface mode to connect two devices together.
> 
> I just gave you two examples what could be written, don't need to stick
> them. You can use some real one...

So the original proposed property to _disable_ a feature implemented by
the hardware should be fine, because it's describing how the hardware
needs to be configured. It's not driver behaviour. It's not driver
policy. It's a configuration bit in a register.

I think you're thinking that the "hibernation" described here is
somehow related to system hibernation, which this has nothing to do
with. This is about configuring the PHY hardware to operate with the
MAC hardware.

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

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

[Wei Fang]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: 2022年8月12日 19:26
> To: Wei Fang <wei.fang@nxp.com>; andrew@lunn.ch; hkallweit1@gmail.com;
> linux@armlinux.org.uk; davem@davemloft.net; edumazet@google.com;
> kuba@kernel.org; pabeni@redhat.com; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; f.fainelli@gmail.com;
> netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org
> 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@linaro.org>
> >> Sent: 2022年8月12日 15:28
> >> To: Wei Fang <wei.fang@nxp.com>; andrew@lunn.ch;
> >> hkallweit1@gmail.com; linux@armlinux.org.uk; davem@davemloft.net;
> >> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> >> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> >> f.fainelli@gmail.com; netdev@vger.kernel.org;
> >> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> >> Subject: Re: [PATCH net 1/2] dt: ar803x: Document disable-hibernation
> >> property
> >>
> >> On 12/08/2022 17:50, wei.fang@nxp.com wrote:
> >>> From: Wei Fang <wei.fang@nxp.com>
> >>>
> >>
> >> 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@nxp.com>
> >>> ---
> >>>  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

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@linaro.org>
> > Sent: 2022年8月12日 19:26
> > To: Wei Fang <wei.fang@nxp.com>; andrew@lunn.ch; hkallweit1@gmail.com;
> > linux@armlinux.org.uk; davem@davemloft.net; edumazet@google.com;
> > kuba@kernel.org; pabeni@redhat.com; robh+dt@kernel.org;
> > krzysztof.kozlowski+dt@linaro.org; f.fainelli@gmail.com;
> > netdev@vger.kernel.org; devicetree@vger.kernel.org;
> > linux-kernel@vger.kernel.org
> > 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@linaro.org>
> > >> Sent: 2022年8月12日 15:28
> > >> To: Wei Fang <wei.fang@nxp.com>; andrew@lunn.ch;
> > >> hkallweit1@gmail.com; linux@armlinux.org.uk; davem@davemloft.net;
> > >> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> > >> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> > >> f.fainelli@gmail.com; netdev@vger.kernel.org;
> > >> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> > >> Subject: Re: [PATCH net 1/2] dt: ar803x: Document disable-hibernation
> > >> property
> > >>
> > >> On 12/08/2022 17:50, wei.fang@nxp.com wrote:
> > >>> From: Wei Fang <wei.fang@nxp.com>
> > >>>
> > >>
> > >> 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@nxp.com>
> > >>> ---
> > >>>  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!

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

[Andrew Lunn]

On Sat, Aug 13, 2022 at 12:50:08AM +1000, wei.fang@nxp.com wrote:
> From: Wei Fang <wei.fang@nxp.com>
> 
> 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.

It is not the first time we have seen this. What you should really be
concentrating on is the clock out. That is what the MAC requires here.

You already have the property qca,clk-out-frequency. You could maybe
piggy back off this. If that property is being used, you know the
clock output is used. So you should do what is needed to keep it
ticking.

You also have qca,keep-pll-enabled:

      If set, keep the PLL enabled even if there is no link. Useful if you
      want to use the clock output without an ethernet link.

To me, it seems like you already have enough properties, you just need
to imply that you need to disable hibernation in order to fulfil these
properties.

	Andrew

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

[Wei Fang]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: 2022年8月12日 22:15
> To: Wei Fang <wei.fang@nxp.com>
> Cc: hkallweit1@gmail.com; linux@armlinux.org.uk; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; f.fainelli@gmail.com;
> netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net 1/2] dt: ar803x: Document disable-hibernation property
> 
> On Sat, Aug 13, 2022 at 12:50:08AM +1000, wei.fang@nxp.com wrote:
> > From: Wei Fang <wei.fang@nxp.com>
> >
> > 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.
> 
> It is not the first time we have seen this. What you should really be
> concentrating on is the clock out. That is what the MAC requires here.
> 
> You already have the property qca,clk-out-frequency. You could maybe piggy
> back off this. If that property is being used, you know the clock output is used. So
> you should do what is needed to keep it ticking.
> 
> You also have qca,keep-pll-enabled:
> 
>       If set, keep the PLL enabled even if there is no link. Useful if you
>       want to use the clock output without an ethernet link.
> 
> To me, it seems like you already have enough properties, you just need to imply
> that you need to disable hibernation in order to fulfil these properties.
> 
> 	Andrew

Hi Andrew,

	Your suggestion is indeed an effective solution, but I checked both the datasheet
and the driver of AR803x PHYs and found that the qca,clk-out-frequency and the
qca,keep-pll-enabled properties are associated with the CLK_25M pin of AR803x PHYs.
But there is a case that CLK_25M pin is not used on some platforms.
Taking our i.MX8DXL platform as an example, the stmmac and AR8031 PHY are applied
on this platform, but the CLK_25M pin of AR8031 is not used. So when I used the method
you mentioned above, it did not work as expected. In this case, we can only disable the
hibernation mode of AR803x PHYs and keep the RX_CLK always outputting a valid clock
so that the stmmac can complete the software reset operation.

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

[Andrew Lunn]

> Hi Andrew,
> 
> 	Your suggestion is indeed an effective solution, but I checked both the datasheet
> and the driver of AR803x PHYs and found that the qca,clk-out-frequency and the
> qca,keep-pll-enabled properties are associated with the CLK_25M pin of AR803x PHYs.
> But there is a case that CLK_25M pin is not used on some platforms.
> Taking our i.MX8DXL platform as an example, the stmmac and AR8031 PHY are applied
> on this platform, but the CLK_25M pin of AR8031 is not used. So when I used the method
> you mentioned above, it did not work as expected. In this case, we can only disable the
> hibernation mode of AR803x PHYs and keep the RX_CLK always outputting a valid clock
> so that the stmmac can complete the software reset operation.

What happens to the RX_CLK when you unplug the cable? It is no longer
receiving anything, so i would expect the RX_CLK to stop ticking. Does
that cause problems for the MAC?

     Andrew

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

[Wei Fang]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: 2022年8月18日 9:20
> To: Wei Fang <wei.fang@nxp.com>
> Cc: hkallweit1@gmail.com; linux@armlinux.org.uk; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; f.fainelli@gmail.com;
> netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net 1/2] dt: ar803x: Document disable-hibernation
> property
> 
> > Hi Andrew,
> >
> > 	Your suggestion is indeed an effective solution, but I checked both
> > the datasheet and the driver of AR803x PHYs and found that the
> > qca,clk-out-frequency and the qca,keep-pll-enabled properties are associated
> with the CLK_25M pin of AR803x PHYs.
> > But there is a case that CLK_25M pin is not used on some platforms.
> > Taking our i.MX8DXL platform as an example, the stmmac and AR8031 PHY
> > are applied on this platform, but the CLK_25M pin of AR8031 is not
> > used. So when I used the method you mentioned above, it did not work
> > as expected. In this case, we can only disable the hibernation mode of
> > AR803x PHYs and keep the RX_CLK always outputting a valid clock so that the
> stmmac can complete the software reset operation.
> 
> What happens to the RX_CLK when you unplug the cable? It is no longer
> receiving anything, so i would expect the RX_CLK to stop ticking. Does that
> cause problems for the MAC?
> 
Yes, after the PHY enters hibernation mode that the RX_CLK stop ticking, but
for stmmac, it is essential that RX_CLK of PHY is present for software reset
completion. Otherwise, the stmmac is failed to complete the software reset
and can not init DMA.

>      Andrew

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

[Andrew Lunn]

> Yes, after the PHY enters hibernation mode that the RX_CLK stop ticking, but
> for stmmac, it is essential that RX_CLK of PHY is present for software reset
> completion. Otherwise, the stmmac is failed to complete the software reset
> and can not init DMA.

So the RX_CLK is more than the recovered clock from the bit stream on
the wire. The PHY has a way to generate a clock when there is no
bit stream?

To me, it sounds like your hardware design is wrong, and it should be
using the 25MHz reference clock. And what you are proposing is a
workaround for this hardware problem.

Anyway, i agree with Russell, a DT property is fine. But please make
it clear in the binding documentation that disabling hibernation has
the side affect of keeping the RX_CLK ticking when there is no
link. That is probably what people want this for, not to actual
disable hibernation.

	Andrew

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

[Wei Fang]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: 2022年8月18日 9:44
> To: Wei Fang <wei.fang@nxp.com>
> Cc: hkallweit1@gmail.com; linux@armlinux.org.uk; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; f.fainelli@gmail.com;
> netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net 1/2] dt: ar803x: Document disable-hibernation
> property
> 
> > Yes, after the PHY enters hibernation mode that the RX_CLK stop
> > ticking, but for stmmac, it is essential that RX_CLK of PHY is present
> > for software reset completion. Otherwise, the stmmac is failed to
> > complete the software reset and can not init DMA.
> 
> So the RX_CLK is more than the recovered clock from the bit stream on the
> wire. The PHY has a way to generate a clock when there is no bit stream?
> 
> To me, it sounds like your hardware design is wrong, and it should be using the
> 25MHz reference clock. And what you are proposing is a workaround for this
> hardware problem.
> 
> Anyway, i agree with Russell, a DT property is fine. But please make it clear in
> the binding documentation that disabling hibernation has the side affect of
> keeping the RX_CLK ticking when there is no link. That is probably what people
> want this for, not to actual disable hibernation.
> 
Ok, I will remodify the description of the property to make it more clear, thanks!

> 	Andrew

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

[Wei Fang]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: 2022年8月18日 9:44
> To: Wei Fang <wei.fang@nxp.com>
> Cc: hkallweit1@gmail.com; linux@armlinux.org.uk; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; f.fainelli@gmail.com;
> netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net 1/2] dt: ar803x: Document disable-hibernation
> property
> 
> > Yes, after the PHY enters hibernation mode that the RX_CLK stop
> > ticking, but for stmmac, it is essential that RX_CLK of PHY is present
> > for software reset completion. Otherwise, the stmmac is failed to
> > complete the software reset and can not init DMA.
> 
> So the RX_CLK is more than the recovered clock from the bit stream on the
> wire. The PHY has a way to generate a clock when there is no bit stream?
> 
Yes, when disable hibernation mode, the RX_CLK always output a valid clock.

> To me, it sounds like your hardware design is wrong, and it should be using the
> 25MHz reference clock. And what you are proposing is a workaround for this
> hardware problem.
> 
> Anyway, i agree with Russell, a DT property is fine. But please make it clear in
> the binding documentation that disabling hibernation has the side affect of
> keeping the RX_CLK ticking when there is no link. That is probably what people
> want this for, not to actual disable hibernation.
> 
> 	Andrew

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

[Rob Herring]

On Sat, 13 Aug 2022 00:50:08 +1000, wei.fang@nxp.com wrote:
> From: Wei Fang <wei.fang@nxp.com>
> 
> 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@nxp.com>
> ---
>  Documentation/devicetree/bindings/net/qca,ar803x.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/net/qca,ar803x.yaml:46:5: [error] syntax error: could not find expected ':' (syntax)

dtschema/dtc warnings/errors:
make[1]: *** Deleting file 'Documentation/devicetree/bindings/net/qca,ar803x.example.dts'
Documentation/devicetree/bindings/net/qca,ar803x.yaml:46:5: could not find expected ':'
make[1]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/net/qca,ar803x.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/net/qca,ar803x.yaml:46:5: could not find expected ':'
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/qca,ar803x.yaml: ignoring, error parsing file
make: *** [Makefile:1404: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

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

[Andrew Lunn]

On Sat, Aug 13, 2022 at 12:50:08AM +1000, wei.fang@nxp.com wrote:
> From: Wei Fang <wei.fang@nxp.com>
> 
> 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@nxp.com>
> ---
>  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.
> +    type: boolean

The description itself needs indenting 2 space.

I would suggest you do what the bot suggests and install the dtschema
tools.

	Andrew

[PATCH net 2/2] net: phy: at803x: Add disable hibernation mode support

[wei.fang]

From: Wei Fang <wei.fang@nxp.com>

When the cable is unplugged, the Atheros AR803x PHYs will enter
hibernation mode after about 10 seconds if the hibernation mode
is enabled and will not provide any clock to the MAC. But for
some MACs, this feature might cause unexpected issues due to the
logic of MACs.
Taking SYNP MAC (stmmac) as an example, if the cable is unplugged
and the "eth0" interface is down, the AR803x PHY will enter
hibernation mode. Then perform the "ifconfig eth0 up" operation,
the stmmac can't able to complete the software reset operation
and fail to init it's own DMA. Therefore, the "eth0" interface is
failed to ifconfig up. Why does it cause this issue? The truth is
that the software reset operation of the stmmac is designed to
depend on the input clock of PHY.
So, this patch offers an option for the user to determine whether
to disable the hibernation mode of AR803x PHYs.

Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 drivers/net/phy/at803x.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 59fe356942b5..3efc6df4eb97 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -115,6 +115,7 @@
 #define AT803X_DEBUG_REG_HIB_CTRL		0x0b
 #define   AT803X_DEBUG_HIB_CTRL_SEL_RST_80U	BIT(10)
 #define   AT803X_DEBUG_HIB_CTRL_EN_ANY_CHANGE	BIT(13)
+#define   AT803X_DEBUG_HIB_CTRL_PS_HIB_EN	BIT(15)
 
 #define AT803X_DEBUG_REG_3C			0x3C
 
@@ -192,6 +193,9 @@
 #define AT803X_KEEP_PLL_ENABLED			BIT(0)
 #define AT803X_DISABLE_SMARTEEE			BIT(1)
 
+/* disable hibernation mode */
+#define AT803X_DISABLE_HIBERNATION		BIT(2)
+
 /* ADC threshold */
 #define QCA808X_PHY_DEBUG_ADC_THRESHOLD		0x2c80
 #define QCA808X_ADC_THRESHOLD_MASK		GENMASK(7, 0)
@@ -730,6 +734,9 @@ static int at803x_parse_dt(struct phy_device *phydev)
 	if (of_property_read_bool(node, "qca,disable-smarteee"))
 		priv->flags |= AT803X_DISABLE_SMARTEEE;
 
+	if (of_property_read_bool(node, "qca,disable-hibernation"))
+		priv->flags |= AT803X_DISABLE_HIBERNATION;
+
 	if (!of_property_read_u32(node, "qca,smarteee-tw-us-1g", &tw)) {
 		if (!tw || tw > 255) {
 			phydev_err(phydev, "invalid qca,smarteee-tw-us-1g\n");
@@ -999,6 +1006,20 @@ static int at8031_pll_config(struct phy_device *phydev)
 					     AT803X_DEBUG_PLL_ON, 0);
 }
 
+static int at803x_hibernation_config(struct phy_device *phydev)
+{
+	struct at803x_priv *priv = phydev->priv;
+
+	/* The default after hardware reset is hibernation mode enabled. After
+	 * software reset, the value is retained.
+	 */
+	if (!(priv->flags & AT803X_DISABLE_HIBERNATION))
+		return 0;
+
+	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_HIB_CTRL,
+					 AT803X_DEBUG_HIB_CTRL_PS_HIB_EN, 0);
+}
+
 static int at803x_config_init(struct phy_device *phydev)
 {
 	struct at803x_priv *priv = phydev->priv;
@@ -1051,6 +1072,10 @@ static int at803x_config_init(struct phy_device *phydev)
 	if (ret < 0)
 		return ret;
 
+	ret = at803x_hibernation_config(phydev);
+	if (ret < 0)
+		return ret;
+
 	/* Ar803x extended next page bit is enabled by default. Cisco
 	 * multigig switches read this bit and attempt to negotiate 10Gbps
 	 * rates even if the next page bit is disabled. This is incorrect
-- 
2.25.1