[2/3] Documentation: dt: binding: fsl: update property description for RCPM
diff mbox series

Message ID 20180831035219.31619-2-ran.wang_1@nxp.com
State New
Headers show
Series
  • [1/3] soc: fsl: add Platform PM driver QorIQ platforms
Related show

Checks

Context Check Description
snowpatch_ozlabs/checkpatch success Test checkpatch on branch next
snowpatch_ozlabs/apply_patch success next/apply_patch Successfully applied

Commit Message

Ran Wang Aug. 31, 2018, 3:52 a.m. UTC
Add property 'big-endian' and supportted IP's configuration info.
Remove property 'fsl,#rcpm-wakeup-cell'.

Signed-off-by: Ran Wang <ran.wang_1@nxp.com>
---
 Documentation/devicetree/bindings/soc/fsl/rcpm.txt |   42 ++++++-------------
 1 files changed, 13 insertions(+), 29 deletions(-)

Comments

Rob Herring Sept. 4, 2018, 1:25 a.m. UTC | #1
On Fri, Aug 31, 2018 at 11:52:18AM +0800, Ran Wang wrote:
> Add property 'big-endian' and supportted IP's configuration info.
> Remove property 'fsl,#rcpm-wakeup-cell'.

"dt-bindings: soc: ..." for the subject

It is obvious reading the diff you are removing fsl,#rcpm-wakeup-cell. 
What is not obvious is why? The commit msg should answer that.

You also are mixing several things in this patch like adding ls1012 
which you don't mention. Please split.

> 
> Signed-off-by: Ran Wang <ran.wang_1@nxp.com>
> ---
>  Documentation/devicetree/bindings/soc/fsl/rcpm.txt |   42 ++++++-------------
>  1 files changed, 13 insertions(+), 29 deletions(-)
Ran Wang Sept. 5, 2018, 2:22 a.m. UTC | #2
Hi Rob

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Tuesday, September 04, 2018 09:25
> To: Ran Wang <ran.wang_1@nxp.com>
> Cc: Leo Li <leoyang.li@nxp.com>; Mark Rutland <mark.rutland@arm.com>;
> linuxppc-dev@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/3] Documentation: dt: binding: fsl: update property
> description for RCPM
> 
> On Fri, Aug 31, 2018 at 11:52:18AM +0800, Ran Wang wrote:
> > Add property 'big-endian' and supportted IP's configuration info.
> > Remove property 'fsl,#rcpm-wakeup-cell'.
> 
> "dt-bindings: soc: ..." for the subject
> 
> It is obvious reading the diff you are removing fsl,#rcpm-wakeup-cell.
> What is not obvious is why? The commit msg should answer that.

Sure, I will add this in next version patch.

> You also are mixing several things in this patch like adding ls1012 which you
> don't mention. Please split.

Got it, will split them and add more information in next version.
 
Ran
> >
> > Signed-off-by: Ran Wang <ran.wang_1@nxp.com>
> > ---
> >  Documentation/devicetree/bindings/soc/fsl/rcpm.txt |   42 ++++++-----------
> --
> >  1 files changed, 13 insertions(+), 29 deletions(-)
Scott Wood Sept. 7, 2018, 8:22 p.m. UTC | #3
On Fri, 2018-08-31 at 11:52 +0800, Ran Wang wrote:
> +Optional properties:
> + - big-endian : Indicate RCPM registers is big-endian. A RCPM node
> +   that doesn't have this property will be regarded as little-endian.

You've just broken all the existing powerpc device trees that are big-endian
and have no big-endian property.

> + - <property 'compatible' string of consumer device> : This string
> +   is referred by RCPM driver to judge if the consumer (such as flex timer)
> +   is able to be regards as wakeup source or not, such as 'fsl,ls1012a-
> ftm'.
> +   Further, this property will carry the bit mask info to control
> +   coresponding wake up source.

What will you do if there are multiple instances of a device with the same
compatible, and different wakeup bits?  Plus, it's an awkward design in
general, and you don't describe what the value actually means (bits in which
register?).  What was wrong with the existing binding?  Alternatively, use the
clock bindings.

> -
> -Example:
> -	lpuart0: serial@2950000 {
> -		compatible = "fsl,ls1021a-lpuart";
> -		reg = <0x0 0x2950000 0x0 0x1000>;
> -		interrupts = <GIC_SPI 80 IRQ_TYPE_LEVEL_HIGH>;
> -		clocks = <&sysclk>;
> -		clock-names = "ipg";
> -		fsl,rcpm-wakeup = <&rcpm 0x0 0x40000000>;
> +		big-endian;
> +		fsl,ls1012a-ftm = <0x20000>;
> +		fsl,pfe = <0xf0000020>;

fsl,pfe is not documented.

-Scott
Ran Wang Sept. 10, 2018, 8:44 a.m. UTC | #4
Hi Scott,

On 2018/9/8 4:23, Scott Wood wrote:
> 
> On Fri, 2018-08-31 at 11:52 +0800, Ran Wang wrote:
> > +Optional properties:
> > + - big-endian : Indicate RCPM registers is big-endian. A RCPM node
> > +   that doesn't have this property will be regarded as little-endian.
> 
> You've just broken all the existing powerpc device trees that are big-endian
> and have no big-endian property.

Yes, powerpc RCPM driver (arch/powerpc/sysdev/fsl_rcpm.c) will not refer
to big-endian. However, I think if Layerscape RCPM driver use different compatible
id (such as ' fsl,qoriq-rcpm-2.2'), it might be safe. Is that OK?

> > + - <property 'compatible' string of consumer device> : This string
> > +   is referred by RCPM driver to judge if the consumer (such as flex timer)
> > +   is able to be regards as wakeup source or not, such as
> > + 'fsl,ls1012a-
> > ftm'.
> > +   Further, this property will carry the bit mask info to control
> > +   coresponding wake up source.
> 
> What will you do if there are multiple instances of a device with the same
> compatible, and different wakeup bits?  

You got me! This is a problem in current version. Well, we have to decouple wake up
source IP and RCPM driver. That's why I add a plat_pm driver to prevent wake up IP
knowing any information of RCPM. So in current context, one idea come to me is to
redesign property ' fsl,ls1012a-ftm = <0x20000>;', change to 'fsl,ls1012a-ftm = <&ftm0 0x20000>;'
What do you say? And could you tell me which API I can use to check if that device's
name is ftm0 (for example we want to find a node ftm0: ftm@29d000)?

>Plus, it's an awkward design in
> general, and you don't describe what the value actually means (bits in which
> register?). 

Yes, I admit my design looks ugly and not flexible and extensible enough. However, for above reason, 
do you have any good idea, please? :)

As to the register information, I can explain here in details (will add to commit
message of next version): There is a RCPM HW block which has register named IPPDEXPCR.
It controls whether prevent certain IP (such as timer, usb, etc) from entering low
power mode when system suspended. So it's necessary to program it if we want one
of those IP work as a wakeup source. However, different Layerscape SoCs have different bit vs.
IP mapping  layout. So I have to record this information in device tree and fetched by RCPM driver
directly.

Do I need to list all SoC's mapping information in this binding doc for reference?

>What was wrong with the existing binding?  

There was one version of RCPM patch which requiring property 'fsl,#rcpm-wakeup-cells' but was not
accepted by upstream finally. Now we will no need it any longer due to new design allow case of multiple
RCPM devices existing case.

>Alternatively, use the clock bindings.

Sorry I didn't get your point.

> > -
> > -Example:
> > -	lpuart0: serial@2950000 {
> > -		compatible = "fsl,ls1021a-lpuart";
> > -		reg = <0x0 0x2950000 0x0 0x1000>;
> > -		interrupts = <GIC_SPI 80 IRQ_TYPE_LEVEL_HIGH>;
> > -		clocks = <&sysclk>;
> > -		clock-names = "ipg";
> > -		fsl,rcpm-wakeup = <&rcpm 0x0 0x40000000>;
> > +		big-endian;
> > +		fsl,ls1012a-ftm = <0x20000>;
> > +		fsl,pfe = <0xf0000020>;
> 
> fsl,pfe is not documented.

pfe patch is not upstream yet, will remove it.

Regards,
Ran

> -Scott
Li Yang Sept. 11, 2018, 10:42 p.m. UTC | #5
On Mon, Sep 10, 2018 at 3:46 AM Ran Wang <ran.wang_1@nxp.com> wrote:
>
> Hi Scott,
>
> On 2018/9/8 4:23, Scott Wood wrote:
> >
> > On Fri, 2018-08-31 at 11:52 +0800, Ran Wang wrote:
> > > +Optional properties:
> > > + - big-endian : Indicate RCPM registers is big-endian. A RCPM node
> > > +   that doesn't have this property will be regarded as little-endian.
> >
> > You've just broken all the existing powerpc device trees that are big-endian
> > and have no big-endian property.
>
> Yes, powerpc RCPM driver (arch/powerpc/sysdev/fsl_rcpm.c) will not refer
> to big-endian. However, I think if Layerscape RCPM driver use different compatible
> id (such as ' fsl,qoriq-rcpm-2.2'), it might be safe. Is that OK?

For Layerscape Chassis(gen3) based chips, the Reference Manual named
the power management IP as COP_PMU instead of RCPM which makes sense
to me as the register map is also quite different from the reg map of
RCPM.  So I think it is reasonable to create a new binding and driver
for the Chassis Gen3 based PMU.  However, the
arch/powerpc/sysdev/fsl_rcpm.c driver probably should be moved to
drivers/soc/fsl, as the Gen2.x Chassis and RCPM IP are also used in
some non-PowerPC Layerscape SoCs.  From what I know, all the RCPM
based IP are all big endian, so there is no need to add this property
for the old binding.

>
> > > + - <property 'compatible' string of consumer device> : This string
> > > +   is referred by RCPM driver to judge if the consumer (such as flex timer)
> > > +   is able to be regards as wakeup source or not, such as
> > > + 'fsl,ls1012a-
> > > ftm'.
> > > +   Further, this property will carry the bit mask info to control
> > > +   coresponding wake up source.
> >
> > What will you do if there are multiple instances of a device with the same
> > compatible, and different wakeup bits?
>
> You got me! This is a problem in current version. Well, we have to decouple wake up
> source IP and RCPM driver. That's why I add a plat_pm driver to prevent wake up IP
> knowing any information of RCPM. So in current context, one idea come to me is to
> redesign property ' fsl,ls1012a-ftm = <0x20000>;', change to 'fsl,ls1012a-ftm = <&ftm0 0x20000>;'
> What do you say? And could you tell me which API I can use to check if that device's
> name is ftm0 (for example we want to find a node ftm0: ftm@29d000)?
>
> >Plus, it's an awkward design in
> > general, and you don't describe what the value actually means (bits in which
> > register?).
>
> Yes, I admit my design looks ugly and not flexible and extensible enough. However, for above reason,
> do you have any good idea, please? :)

Why do you have to move the wakeup information from device nodes to
the RCPM/PMU node?  For information related to both on-chip device and
SoC integration, the information normally goes into the node of
on-chip device instead of the integration IP's device node.  Existing
example like: interrupt, clock, memory map, and etc.  It is much
cleaner than listing all the interrupts in the interrupt controller's
node, right?

>
> As to the register information, I can explain here in details (will add to commit
> message of next version): There is a RCPM HW block which has register named IPPDEXPCR.
> It controls whether prevent certain IP (such as timer, usb, etc) from entering low
> power mode when system suspended. So it's necessary to program it if we want one
> of those IP work as a wakeup source. However, different Layerscape SoCs have different bit vs.
> IP mapping  layout. So I have to record this information in device tree and fetched by RCPM driver
> directly.
>
> Do I need to list all SoC's mapping information in this binding doc for reference?
>
> >What was wrong with the existing binding?
>
> There was one version of RCPM patch which requiring property 'fsl,#rcpm-wakeup-cells' but was not
> accepted by upstream finally. Now we will no need it any longer due to new design allow case of multiple
> RCPM devices existing case.
>
> >Alternatively, use the clock bindings.
>
> Sorry I didn't get your point.
>
> > > -
> > > -Example:
> > > -   lpuart0: serial@2950000 {
> > > -           compatible = "fsl,ls1021a-lpuart";
> > > -           reg = <0x0 0x2950000 0x0 0x1000>;
> > > -           interrupts = <GIC_SPI 80 IRQ_TYPE_LEVEL_HIGH>;
> > > -           clocks = <&sysclk>;
> > > -           clock-names = "ipg";
> > > -           fsl,rcpm-wakeup = <&rcpm 0x0 0x40000000>;
> > > +           big-endian;
> > > +           fsl,ls1012a-ftm = <0x20000>;
> > > +           fsl,pfe = <0xf0000020>;
> >
> > fsl,pfe is not documented.
>
> pfe patch is not upstream yet, will remove it.
>
> Regards,
> Ran
>
> > -Scott
>

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/soc/fsl/rcpm.txt b/Documentation/devicetree/bindings/soc/fsl/rcpm.txt
index e284e4e..7fc630a 100644
--- a/Documentation/devicetree/bindings/soc/fsl/rcpm.txt
+++ b/Documentation/devicetree/bindings/soc/fsl/rcpm.txt
@@ -5,8 +5,6 @@  and power management.
 
 Required properites:
   - reg : Offset and length of the register set of the RCPM block.
-  - fsl,#rcpm-wakeup-cells : The number of IPPDEXPCR register cells in the
-	fsl,rcpm-wakeup property.
   - compatible : Must contain a chip-specific RCPM block compatible string
 	and (if applicable) may contain a chassis-version RCPM compatible
 	string. Chip-specific strings are of the form "fsl,<chip>-rcpm",
@@ -27,37 +25,23 @@  Chassis Version		Example Chips
 ---------------		-------------------------------
 1.0				p4080, p5020, p5040, p2041, p3041
 2.0				t4240, b4860, b4420
-2.1				t1040, ls1021
+2.1				t1040, ls1021, ls1012
+
+Optional properties:
+ - big-endian : Indicate RCPM registers is big-endian. A RCPM node
+   that doesn't have this property will be regarded as little-endian.
+ - <property 'compatible' string of consumer device> : This string
+   is referred by RCPM driver to judge if the consumer (such as flex timer)
+   is able to be regards as wakeup source or not, such as 'fsl,ls1012a-ftm'.
+   Further, this property will carry the bit mask info to control
+   coresponding wake up source.
 
 Example:
 The RCPM node for T4240:
 	rcpm: global-utilities@e2000 {
 		compatible = "fsl,t4240-rcpm", "fsl,qoriq-rcpm-2.0";
 		reg = <0xe2000 0x1000>;
-		fsl,#rcpm-wakeup-cells = <2>;
-	};
-
-* Freescale RCPM Wakeup Source Device Tree Bindings
--------------------------------------------
-Required fsl,rcpm-wakeup property should be added to a device node if the device
-can be used as a wakeup source.
-
-  - fsl,rcpm-wakeup: Consists of a phandle to the rcpm node and the IPPDEXPCR
-	register cells. The number of IPPDEXPCR register cells is defined in
-	"fsl,#rcpm-wakeup-cells" in the rcpm node. The first register cell is
-	the bit mask that should be set in IPPDEXPCR0, and the second register
-	cell is for IPPDEXPCR1, and so on.
-
-	Note: IPPDEXPCR(IP Powerdown Exception Control Register) provides a
-	mechanism for keeping certain blocks awake during STANDBY and MEM, in
-	order to use them as wake-up sources.
-
-Example:
-	lpuart0: serial@2950000 {
-		compatible = "fsl,ls1021a-lpuart";
-		reg = <0x0 0x2950000 0x0 0x1000>;
-		interrupts = <GIC_SPI 80 IRQ_TYPE_LEVEL_HIGH>;
-		clocks = <&sysclk>;
-		clock-names = "ipg";
-		fsl,rcpm-wakeup = <&rcpm 0x0 0x40000000>;
+		big-endian;
+		fsl,ls1012a-ftm = <0x20000>;
+		fsl,pfe = <0xf0000020>;
 	};