| Message ID | 1392312816-17657-8-git-send-email-gregory.clement@free-electrons.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 >
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
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 {
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(-)