| Message ID | 20220812145009.1229094-2-wei.fang@nxp.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add DT property to disable hibernation mode | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
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
> -----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
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
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.
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
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.
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
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
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.
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.
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
> -----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.
> -----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
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."
> 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
> -----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
> 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
> -----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
> -----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
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