| Message ID | 1389135041-16062-2-git-send-email-fkan@apm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 07, 2014 at 10:50:36PM +0000, Feng Kan wrote: > Add X-Gene reboot device tree node documentation. > > Signed-off-by: Feng Kan <fkan@apm.com> > --- > .../devicetree/bindings/arm64/xgene/reboot.txt | 10 ++++++++++ > 1 files changed, 10 insertions(+), 0 deletions(-) > create mode 100644 Documentation/devicetree/bindings/arm64/xgene/reboot.txt > > diff --git a/Documentation/devicetree/bindings/arm64/xgene/reboot.txt b/Documentation/devicetree/bindings/arm64/xgene/reboot.txt > new file mode 100644 > index 0000000..51d23c1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/arm64/xgene/reboot.txt > @@ -0,0 +1,10 @@ > +Reboot property to control system reboot on storm platforms: > + > +The register attribute contain the address of the reset register. Required properties: - compatible: should contain "apm,xgene-reboot" - reg: the base address and size of the reset register. > + > +Example: > + > + reboot@0 { > + compatible = "apm,xgene-reboot"; > + reg = <0x0 0x17000014 0x0 0x4>; > + }; Given this seems to be a 64-bit address, the unit address should preferably be 0,17000014 rather than just 0. Thanks, Mark.
On Wednesday 08 January 2014 10:05:50 Mark Rutland wrote: > > + > > +Example: > > + > > + reboot@0 { > > + compatible = "apm,xgene-reboot"; > > + reg = <0x0 0x17000014 0x0 0x4>; > > + }; > > Given this seems to be a 64-bit address, the unit address should > preferably be 0,17000014 rather than just 0. I also wonder about the size and location of the register. Are you sure the device is just a single 4-byte register at offset 0x14, rather than a complex device that happens to contain the reset register? Arnd
On Wed, Jan 8, 2014 at 2:08 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Wednesday 08 January 2014 10:05:50 Mark Rutland wrote: >> > + >> > +Example: >> > + >> > + reboot@0 { >> > + compatible = "apm,xgene-reboot"; >> > + reg = <0x0 0x17000014 0x0 0x4>; >> > + }; >> >> Given this seems to be a 64-bit address, the unit address should >> preferably be 0,17000014 rather than just 0. > > I also wonder about the size and location of the register. > Are you sure the device is just a single 4-byte register at offset > 0x14, rather than a complex device that happens to contain the > reset register? FKAN: this is a single 4 byte register when clicked perform the system reset of the platform. > > Arnd
On Wed, Jan 8, 2014 at 2:05 AM, Mark Rutland <mark.rutland@arm.com> wrote: > On Tue, Jan 07, 2014 at 10:50:36PM +0000, Feng Kan wrote: >> Add X-Gene reboot device tree node documentation. >> >> Signed-off-by: Feng Kan <fkan@apm.com> >> --- >> .../devicetree/bindings/arm64/xgene/reboot.txt | 10 ++++++++++ >> 1 files changed, 10 insertions(+), 0 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/arm64/xgene/reboot.txt >> >> diff --git a/Documentation/devicetree/bindings/arm64/xgene/reboot.txt b/Documentation/devicetree/bindings/arm64/xgene/reboot.txt >> new file mode 100644 >> index 0000000..51d23c1 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/arm64/xgene/reboot.txt >> @@ -0,0 +1,10 @@ >> +Reboot property to control system reboot on storm platforms: >> + >> +The register attribute contain the address of the reset register. > > Required properties: > > - compatible: should contain "apm,xgene-reboot" > > - reg: the base address and size of the reset register. > >> + >> +Example: >> + >> + reboot@0 { >> + compatible = "apm,xgene-reboot"; >> + reg = <0x0 0x17000014 0x0 0x4>; >> + }; > > Given this seems to be a 64-bit address, the unit address should > preferably be 0,17000014 rather than just 0. FKAN: Mark, can I keep this as it is to be consistent with the reset of the file? > > Thanks, > Mark.
On Thursday 09 January 2014, Feng Kan wrote: > On Wed, Jan 8, 2014 at 2:08 AM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Wednesday 08 January 2014 10:05:50 Mark Rutland wrote: > >> > + > >> > +Example: > >> > + > >> > + reboot@0 { > >> > + compatible = "apm,xgene-reboot"; > >> > + reg = <0x0 0x17000014 0x0 0x4>; > >> > + }; > >> > >> Given this seems to be a 64-bit address, the unit address should > >> preferably be 0,17000014 rather than just 0. > > > > I also wonder about the size and location of the register. > > Are you sure the device is just a single 4-byte register at offset > > 0x14, rather than a complex device that happens to contain the > > reset register? > > FKAN: this is a single 4 byte register when clicked perform the system > reset of the platform. That is not what I was asking about. The problem with your binding is that it doesn't seem to describe the hardware structure at all, but rather try to invent devices because of how it's convenient for how you write the Linux drivers. This is never a good idea and it will become a problem once you try to port a different operating system, or when the structure of the Linux drivers changes. Remember that in Linux we can freely reorganize our code as requirements change, but the bindings have to remain stable to allow running new kernels with an older device tree that may be shipped with the firmware. (Side note: if you end up doing an ACPI driver for this after all, the problem is even worse because it's basically impossible to update the ACPI tables in field, while with DT you can at least override them in the firmware if something goes really wrong.) On most SoCs I've seen, the system reset registers are part of some on-chip device, most likely one of - A SoC-specific system control register set containing various global registers that may be of interest to other parts of the kernel. - A watchdog timer device that will reset the system after a crash, or when explicit requested by the OS - A dedicated reset controller that has registers for individually resetting a number of subsystems of the SoC, or all at once in case of system reboot. - an interface to an external or integrated power management IC that controls voltages and/or clock frequencies provided to the SoC and other parts of the system. In each of those cases, we already have a subsystem that is supposed to handle the entire register set and provide a system reset method as one of the things it does. Given that your register is at offset 0x14 from the start of a larger MMIO range, I strongly suspect that the same is true here. Arnd
> That is not what I was asking about. > > The problem with your binding is that it doesn't seem to describe > the hardware structure at all, but rather try to invent devices > because of how it's convenient for how you write the Linux drivers. > > This is never a good idea and it will become a problem once you > try to port a different operating system, or when the structure > of the Linux drivers changes. Remember that in Linux we can freely > reorganize our code as requirements change, but the bindings have > to remain stable to allow running new kernels with an older device > tree that may be shipped with the firmware. > > (Side note: if you end up doing an ACPI driver for this after all, > the problem is even worse because it's basically impossible to update > the ACPI tables in field, while with DT you can at least override > them in the firmware if something goes really wrong.) > > On most SoCs I've seen, the system reset registers are part of > some on-chip device, most likely one of > - A SoC-specific system control register set containing various > global registers that may be of interest to other parts of the > kernel. FKAN: our system block in your reference would be the system clock unit (SCU). It handles number of things (JTAG, clock control, power control not touched by OS). The register that is relevant to the system reset is this lone register. I reduce the range of the dts aperture due to this fact. > - A watchdog timer device that will reset the system after a crash, > or when explicit requested by the OS > - A dedicated reset controller that has registers for individually > resetting a number of subsystems of the SoC, or all at once in > case of system reboot. > - an interface to an external or integrated power management IC > that controls voltages and/or clock frequencies provided to > the SoC and other parts of the system. > > In each of those cases, we already have a subsystem that is supposed > to handle the entire register set and provide a system reset method > as one of the things it does. Given that your register is at offset > 0x14 from the start of a larger MMIO range, I strongly suspect > that the same is true here. FKAN: I could remove this dts node and create another dts node that describe the range of registers on the SCU and use that node in this driver. I am not sure which subsystem I can use to handle this case, I do see a reset subsystem in the kernel but more used for ip resets. Please kindly let me know. Thanks for the great feedback. > > Arnd
On Monday 13 January 2014, Feng Kan wrote: > FKAN: I could remove this dts node and create another dts node that > describe the range of registers on the SCU and use that node in this driver. > I am not sure which subsystem I can use to handle this case, I do see a reset > subsystem in the kernel but more used for ip resets. Please kindly let > me know. Thanks for the great feedback. Is this related to the standard ARM SCU that manages multiprocessor systems, or a different unit that uses the same name? Since this is a global register range with a variety of things in it, the best candidate IMHO would be to use the "syscon" driver. You can mark the device node as 'compatible="apm,xgene-scu","syscon"' to let it get picked up by the drivers/mfd/syscon driver, which creates a "regmap" structure for it. The reset driver then uses a DT reference with a phandle to the SCU node and calls syscon_regmap_lookup_by_phandle() to get the regmap. It can get the register number inside the regmap from DT as well and use the regmap API to perform the reset. Arnd
> > Is this related to the standard ARM SCU that manages multiprocessor > systems, or a different unit that uses the same name? FKAN: You mean the snoop control unit in ARM. This is different from that, the main function of this unit is clk control. > > Since this is a global register range with a variety of things in it, > the best candidate IMHO would be to use the "syscon" driver. You can > mark the device node as 'compatible="apm,xgene-scu","syscon"' to let > it get picked up by the drivers/mfd/syscon driver, which creates > a "regmap" structure for it. > > The reset driver then uses a DT reference with a phandle to the > SCU node and calls syscon_regmap_lookup_by_phandle() to get the > regmap. It can get the register number inside the regmap from DT > as well and use the regmap API to perform the reset. Thanks, will do. > > Arnd > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 14 January 2014, Feng Kan wrote: > > > > > Is this related to the standard ARM SCU that manages multiprocessor > > systems, or a different unit that uses the same name? > > FKAN: You mean the snoop control unit in ARM. This is different from > that, the main function of this unit is clk control. Hmm, if it's *mainly* used for clocks, you may actually just want to have a driver in drivers/clk for it that also performs a few of the other things on the side. Arnd
On Mon, Jan 13, 2014 at 10:06 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 14 January 2014, Feng Kan wrote: >> >> > >> > Is this related to the standard ARM SCU that manages multiprocessor >> > systems, or a different unit that uses the same name? >> >> FKAN: You mean the snoop control unit in ARM. This is different from >> that, the main function of this unit is clk control. > > Hmm, if it's *mainly* used for clocks, you may actually just want to > have a driver in drivers/clk for it that also performs a few of the > other things on the side. > FKAN: there among other thing, reset controls, fuse id's and deep sleep resume registers... I like your idea of using syscon, we can have a node for SCU and then the specific driver to parse out the needed chunk to use. > Arnd
On Tuesday 14 January 2014 10:35:33 Feng Kan wrote: > On Mon, Jan 13, 2014 at 10:06 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Tuesday 14 January 2014, Feng Kan wrote: > >> > >> > > >> > Is this related to the standard ARM SCU that manages multiprocessor > >> > systems, or a different unit that uses the same name? > >> > >> FKAN: You mean the snoop control unit in ARM. This is different from > >> that, the main function of this unit is clk control. > > > > Hmm, if it's *mainly* used for clocks, you may actually just want to > > have a driver in drivers/clk for it that also performs a few of the > > other things on the side. > > > > FKAN: there among other thing, reset controls, fuse id's and deep sleep > resume registers... I like your idea of using syscon, we can have a node > for SCU and then the specific driver to parse out the needed chunk to use. Yes, sounds good. Arnd
diff --git a/Documentation/devicetree/bindings/arm64/xgene/reboot.txt b/Documentation/devicetree/bindings/arm64/xgene/reboot.txt new file mode 100644 index 0000000..51d23c1 --- /dev/null +++ b/Documentation/devicetree/bindings/arm64/xgene/reboot.txt @@ -0,0 +1,10 @@ +Reboot property to control system reboot on storm platforms: + +The register attribute contain the address of the reset register. + +Example: + + reboot@0 { + compatible = "apm,xgene-reboot"; + reg = <0x0 0x17000014 0x0 0x4>; + };
Add X-Gene reboot device tree node documentation. Signed-off-by: Feng Kan <fkan@apm.com> --- .../devicetree/bindings/arm64/xgene/reboot.txt | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/arm64/xgene/reboot.txt