| Message ID | E1YW7tB-0003mM-1L@rmk-PC.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Dear Russell King, On Thu, 12 Mar 2015 18:31:21 +0000, Russell King wrote: > + pmu: power-management@d0000 { > + compatible = "marvell,dove-pmu"; > + reg = <0xd0000 0x8000>, <0xd8000 0x8000>; If I'm not wrong, the registers of the RTC are at 0xd8500, which is also covered by this new pmu node. It does work because you're using of_iomap(), but it isn't really ideal. Maybe some sort of syscon/regmap thing should be used? Best regards, Thomas
On Fri, Mar 13, 2015 at 01:30:38PM +0100, Thomas Petazzoni wrote: > Dear Russell King, > > On Thu, 12 Mar 2015 18:31:21 +0000, Russell King wrote: > > > + pmu: power-management@d0000 { > > + compatible = "marvell,dove-pmu"; > > + reg = <0xd0000 0x8000>, <0xd8000 0x8000>; > > If I'm not wrong, the registers of the RTC are at 0xd8500, which is > also covered by this new pmu node. It does work because you're using > of_iomap(), but it isn't really ideal. > > Maybe some sort of syscon/regmap thing should be used? Please look at the locking that would require and the locking required for PM domains (and as I've previously explained, apparently the hardware requires a specific sequence of writes.)
Hi Russell, On 12/03/2015 19:31, Russell King wrote: > Add the PMU description to the Dove DT file. The PMU provides multiple > features, including an interrupt, reset, power and isolation controller. > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > --- > arch/arm/boot/dts/dove.dtsi | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi > index a5441d5482a6..43d4ebf414be 100644 > --- a/arch/arm/boot/dts/dove.dtsi > +++ b/arch/arm/boot/dts/dove.dtsi > @@ -380,6 +380,15 @@ > status = "disabled"; > }; > > + pmu: power-management@d0000 { > + compatible = "marvell,dove-pmu"; > + reg = <0xd0000 0x8000>, <0xd8000 0x8000>; Here you overlap some other nodes such as the thermal one (from 0xd001c to 0xd0028), the clock gate one (from 0xd0038 to 0xd003c), the gpio one, the pinctrl one ... Could you be more specific? Thanks, Gregory > + interrupts = <33>; > + interrupt-controller; > + #interrupt-cells = <1>; > + #reset-cells = <1>; > + }; > + > thermal: thermal-diode@d001c { > compatible = "marvell,dove-thermal"; > reg = <0xd001c 0x0c>, <0xd005c 0x08>; >
On Mon, Mar 16, 2015 at 07:27:30PM +0100, Gregory CLEMENT wrote: > > + pmu: power-management@d0000 { > > + compatible = "marvell,dove-pmu"; > > + reg = <0xd0000 0x8000>, <0xd8000 0x8000>; > > Here you overlap some other nodes such as the thermal one (from 0xd001c > to 0xd0028), the clock gate one (from 0xd0038 to 0xd003c), the gpio one, > the pinctrl one ... Yes, I'm well aware of that, but here's the thing... I'm describing the hardware here. The real problem is that Dove (etc) went down the path of breaking up the PMU device into multiple smaller devices each specifying a sub-set of the register range. By doing that, Dove moved away from describing the hardware - instead, we've described the Linux _implementation_ with its separate (sub-)devices - its the implementation's choice that we'd break up the PMU into these separate devices, almost to the point of specifying each individual register. What could be done to work around this oversight is to mvoe these devices beneath the PMU node, which IMHO makes complete sense as these are sub-devices of the PMU/PMC rather than separate devices.
On 03/17/2015 02:43 PM, Russell King - ARM Linux wrote: > On Mon, Mar 16, 2015 at 07:27:30PM +0100, Gregory CLEMENT wrote: >>> + pmu: power-management@d0000 { >>> + compatible = "marvell,dove-pmu"; >>> + reg = <0xd0000 0x8000>, <0xd8000 0x8000>; >> >> Here you overlap some other nodes such as the thermal one (from 0xd001c >> to 0xd0028), the clock gate one (from 0xd0038 to 0xd003c), the gpio one, >> the pinctrl one ... > > Yes, I'm well aware of that, but here's the thing... > > I'm describing the hardware here. > > The real problem is that Dove (etc) went down the path of breaking up > the PMU device into multiple smaller devices each specifying a sub-set > of the register range. By doing that, Dove moved away from describing Russell, I totally agree that from today's point-of-view having a single pmu node with sub-nodes would have been the better approach. > the hardware - instead, we've described the Linux _implementation_ with > its separate (sub-)devices - its the implementation's choice that we'd > break up the PMU into these separate devices, almost to the point of > specifying each individual register. > > What could be done to work around this oversight is to mvoe these > devices beneath the PMU node, which IMHO makes complete sense as these > are sub-devices of the PMU/PMC rather than separate devices. If we do this, we should have a look at syscon and potentially also simple-mfd which will allow us to share the registers and register platform_devices for the sub-nodes. The current pmu power/reset/irq can still use the extra locking and directly access MMIO registers - but for the others we should really look into using regmaps. The only thing here is that power domain descriptions are also encoded as direct sub-nodes of the pmu node, right? Sebastian
On Tue, Mar 17, 2015 at 04:09:19PM +0100, Sebastian Hesselbarth wrote: > If we do this, we should have a look at syscon and potentially also > simple-mfd which will allow us to share the registers and register > platform_devices for the sub-nodes. > > The current pmu power/reset/irq can still use the extra locking > and directly access MMIO registers - but for the others we should > really look into using regmaps. > > The only thing here is that power domain descriptions are also encoded > as direct sub-nodes of the pmu node, right? Yes, I've been debating about putting those under a domains { } sub-node, like we do with the ports { } sub-node for of-graph stuff.
On Tue, Mar 17, 2015 at 4:09 PM, Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote: > If we do this, we should have a look at syscon and potentially also > simple-mfd which will allow us to share the registers and register > platform_devices for the sub-nodes. I've just sent a pull request for simple-mfd to the ARM SoC maintainers. I see that you Free Electron guys make good use of it. Yours, Linus Walleij
diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi index a5441d5482a6..43d4ebf414be 100644 --- a/arch/arm/boot/dts/dove.dtsi +++ b/arch/arm/boot/dts/dove.dtsi @@ -380,6 +380,15 @@ status = "disabled"; }; + pmu: power-management@d0000 { + compatible = "marvell,dove-pmu"; + reg = <0xd0000 0x8000>, <0xd8000 0x8000>; + interrupts = <33>; + interrupt-controller; + #interrupt-cells = <1>; + #reset-cells = <1>; + }; + thermal: thermal-diode@d001c { compatible = "marvell,dove-thermal"; reg = <0xd001c 0x0c>, <0xd005c 0x08>;
Add the PMU description to the Dove DT file. The PMU provides multiple features, including an interrupt, reset, power and isolation controller. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- arch/arm/boot/dts/dove.dtsi | 9 +++++++++ 1 file changed, 9 insertions(+)