[PATCH v4 07/13] ARM: dts: mvebu: Add a new set of registers to the PMSU node

[PATCH v4 07/13] ARM: dts: mvebu: Add a new set of registers to the PMSU node

[Gregory CLEMENT]

The Power Management Unit Service block also controls the Coherency
Fabric subsystem. This new set of registers is needed for the CPU idle
implementation for the Armada XP, it allows to enter in a deep CPU
idle state where the Coherency Fabric and the L2 cache are powerdown.

Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 Documentation/devicetree/bindings/arm/armada-370-xp-pmsu.txt | 12 +++++++-----
 arch/arm/boot/dts/armada-xp.dtsi                             |  2 +-
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/armada-370-xp-pmsu.txt b/Documentation/devicetree/bindings/arm/armada-370-xp-pmsu.txt
index 926b4d6aae7e..8a9db0c32ba5 100644
--- a/Documentation/devicetree/bindings/arm/armada-370-xp-pmsu.txt
+++ b/Documentation/devicetree/bindings/arm/armada-370-xp-pmsu.txt
@@ -7,14 +7,16 @@ Required properties:
 - compatible: "marvell,armada-370-xp-pmsu"
 
 - reg: Should contain PMSU registers location and length. First pair
-  for the per-CPU SW Reset Control registers, second pair for the
-  Power Management Service Unit.
+  for the per-CPU SW Reset Control registers, second pair for the CPU
+  Power Management Service Unit registers, third pair for the Fabric Power
+  Management Service Unit registers.
 
 Example:
 
-armada-370-xp-pmsu@d0022000 {
+armada-370-xp-pmsu@22000 {
 	compatible = "marvell,armada-370-xp-pmsu";
-	reg = <0xd0022100 0x430>,
-	      <0xd0020800 0x20>;
+	reg = <0x22100 0x430>,
+	      <0x20800 0x20>,
+	      <0x22000 0x24>;
 };
 
diff --git a/arch/arm/boot/dts/armada-xp.dtsi b/arch/arm/boot/dts/armada-xp.dtsi
index b8b84a22f0f3..f717da4f4d97 100644
--- a/arch/arm/boot/dts/armada-xp.dtsi
+++ b/arch/arm/boot/dts/armada-xp.dtsi
@@ -113,7 +113,7 @@
 
 			armada-370-xp-pmsu@22000 {
 				compatible = "marvell,armada-370-xp-pmsu";
-				reg = <0x22100 0x400>, <0x20800 0x20>;
+				reg = <0x22100 0x400>, <0x20800 0x20>, <0x22000 0x24>;
 			};
 
 			eth2: ethernet@30000 {
-- 
1.8.1.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 07/13] ARM: dts: mvebu: Add a new set of registers to the PMSU node

[Jason Cooper]

On Thu, Feb 13, 2014 at 06:33:30PM +0100, Gregory CLEMENT wrote:
> The Power Management Unit Service block also controls the Coherency
> Fabric subsystem. This new set of registers is needed for the CPU idle
> implementation for the Armada XP, it allows to enter in a deep CPU
> idle state where the Coherency Fabric and the L2 cache are powerdown.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  Documentation/devicetree/bindings/arm/armada-370-xp-pmsu.txt | 12 +++++++-----
>  arch/arm/boot/dts/armada-xp.dtsi                             |  2 +-
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/armada-370-xp-pmsu.txt b/Documentation/devicetree/bindings/arm/armada-370-xp-pmsu.txt
> index 926b4d6aae7e..8a9db0c32ba5 100644
> --- a/Documentation/devicetree/bindings/arm/armada-370-xp-pmsu.txt
> +++ b/Documentation/devicetree/bindings/arm/armada-370-xp-pmsu.txt
> @@ -7,14 +7,16 @@ Required properties:
>  - compatible: "marvell,armada-370-xp-pmsu"
>  
>  - reg: Should contain PMSU registers location and length. First pair
> -  for the per-CPU SW Reset Control registers, second pair for the
> -  Power Management Service Unit.
> +  for the per-CPU SW Reset Control registers, second pair for the CPU
> +  Power Management Service Unit registers, third pair for the Fabric Power
> +  Management Service Unit registers.

Please mention explicitly that previous versions of this binding only
specified the first two registers.  I haven't dug through the rest of
this series yet, but I assume the driver will behave sanely when
encountering an old dtb w/o the third reg entry?

thx,

Jason.

>  
>  Example:
>  
> -armada-370-xp-pmsu@d0022000 {
> +armada-370-xp-pmsu@22000 {
>  	compatible = "marvell,armada-370-xp-pmsu";
> -	reg = <0xd0022100 0x430>,
> -	      <0xd0020800 0x20>;
> +	reg = <0x22100 0x430>,
> +	      <0x20800 0x20>,
> +	      <0x22000 0x24>;
>  };
>  
> diff --git a/arch/arm/boot/dts/armada-xp.dtsi b/arch/arm/boot/dts/armada-xp.dtsi
> index b8b84a22f0f3..f717da4f4d97 100644
> --- a/arch/arm/boot/dts/armada-xp.dtsi
> +++ b/arch/arm/boot/dts/armada-xp.dtsi
> @@ -113,7 +113,7 @@
>  
>  			armada-370-xp-pmsu@22000 {
>  				compatible = "marvell,armada-370-xp-pmsu";
> -				reg = <0x22100 0x400>, <0x20800 0x20>;
> +				reg = <0x22100 0x400>, <0x20800 0x20>, <0x22000 0x24>;
>  			};
>  
>  			eth2: ethernet@30000 {
> -- 
> 1.8.1.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 07/13] ARM: dts: mvebu: Add a new set of registers to the PMSU node

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Thu, 13 Feb 2014 18:33:30 +0100, Gregory CLEMENT wrote:

>  - reg: Should contain PMSU registers location and length. First pair
> -  for the per-CPU SW Reset Control registers, second pair for the
> -  Power Management Service Unit.
> +  for the per-CPU SW Reset Control registers, second pair for the CPU
> +  Power Management Service Unit registers, third pair for the Fabric Power
> +  Management Service Unit registers.
>  
>  Example:
>  
> -armada-370-xp-pmsu@d0022000 {
> +armada-370-xp-pmsu@22000 {
>  	compatible = "marvell,armada-370-xp-pmsu";
> -	reg = <0xd0022100 0x430>,
> -	      <0xd0020800 0x20>;
> +	reg = <0x22100 0x430>,
> +	      <0x20800 0x20>,
> +	      <0x22000 0x24>;

I am not too happy with this because:

 *) We have to remove the second register pair from the PMSU. I haven't
    yet posted the patches on LAKML for SMP on 375 and 38x, but the 375
    doesn't have a PMSU. It however has the same CPU reset control
    registers as 370 and XP. Therefore, these 0x20800 registers have to
    be handled by a separate driver (that uses the reset framework),
    and not handled by the PMSU driver. This way, Armada 370/XP can use
    PMSU+CPU reset, while 375 will only use CPU reset.

 *) I think making the PMSU register start at 0x22100 was a mistake.
    The PMSU registers actually start at 0x22000 and they seem to go all
    the way to 0x23000. So we should have only one register pair. This
    of course raises some backward compatibility questions for the DT.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 07/13] ARM: dts: mvebu: Add a new set of registers to the PMSU node

[Gregory CLEMENT]

On 19/02/2014 17:00, Thomas Petazzoni wrote:
> Dear Gregory CLEMENT,
> 
> On Thu, 13 Feb 2014 18:33:30 +0100, Gregory CLEMENT wrote:
> 
>>  - reg: Should contain PMSU registers location and length. First pair
>> -  for the per-CPU SW Reset Control registers, second pair for the
>> -  Power Management Service Unit.
>> +  for the per-CPU SW Reset Control registers, second pair for the CPU
>> +  Power Management Service Unit registers, third pair for the Fabric Power
>> +  Management Service Unit registers.
>>  
>>  Example:
>>  
>> -armada-370-xp-pmsu@d0022000 {
>> +armada-370-xp-pmsu@22000 {
>>  	compatible = "marvell,armada-370-xp-pmsu";
>> -	reg = <0xd0022100 0x430>,
>> -	      <0xd0020800 0x20>;
>> +	reg = <0x22100 0x430>,
>> +	      <0x20800 0x20>,
>> +	      <0x22000 0x24>;
> 
> I am not too happy with this because:
> 
>  *) We have to remove the second register pair from the PMSU. I haven't
>     yet posted the patches on LAKML for SMP on 375 and 38x, but the 375
>     doesn't have a PMSU. It however has the same CPU reset control
>     registers as 370 and XP. Therefore, these 0x20800 registers have to
>     be handled by a separate driver (that uses the reset framework),
>     and not handled by the PMSU driver. This way, Armada 370/XP can use
>     PMSU+CPU reset, while 375 will only use CPU reset.
> 
>  *) I think making the PMSU register start at 0x22100 was a mistake.
>     The PMSU registers actually start at 0x22000 and they seem to go all
>     the way to 0x23000. So we should have only one register pair. This
>     of course raises some backward compatibility questions for the DT.

I agree to use something like:

armada-370-xp-pmsu@22000 {
	compatible = "marvell,armada-xp-pmsu"; /* new compatible string */
	reg = <0x22000 0x1000>;
	};


and I think the best option would be to introduce a new compatible string
for this. In the same time we continue to support the old compatible string
but we print a big warning during the kernel boot that this compatible string
is deprecated, and we will finally remove it a few release.

Thanks,

Gregory


> 
> Thomas
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
+33 602 196 044

Re: [PATCH v4 07/13] ARM: dts: mvebu: Add a new set of registers to the PMSU node

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Wed, 19 Feb 2014 18:49:13 +0100, Gregory CLEMENT wrote:

> I agree to use something like:
> 
> armada-370-xp-pmsu@22000 {
> 	compatible = "marvell,armada-xp-pmsu"; /* new compatible string */

But this PMSU is identical on 370, no? So the marvell,armada-xp-pmsu
compatible string is maybe not the most appropriate one?

> 	reg = <0x22000 0x1000>;
> 	};
> 
> 
> and I think the best option would be to introduce a new compatible string
> for this. In the same time we continue to support the old compatible string
> but we print a big warning during the kernel boot that this compatible string
> is deprecated, and we will finally remove it a few release.

How do you support the new features of the L2 stuff with the old
compatible string? By substracting 0x100 to the register base address?

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com