diff mbox

[V4,5/5] Documentation: power: reset: Add documentation for generic SYSCON reboot driver

Message ID 1390504801-24483-6-git-send-email-fkan@apm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Feng Kan Jan. 23, 2014, 7:20 p.m. UTC
Add documentation for generic SYSCON reboot driver.

Signed-off-by: Feng Kan <fkan@apm.com>
---
 .../bindings/power/reset/syscon-reboot.txt         |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot.txt

Comments

Mark Rutland Jan. 24, 2014, 11:39 a.m. UTC | #1
On Thu, Jan 23, 2014 at 07:20:01PM +0000, Feng Kan wrote:
> Add documentation for generic SYSCON reboot driver.
> 
> Signed-off-by: Feng Kan <fkan@apm.com>
> ---
>  .../bindings/power/reset/syscon-reboot.txt         |   16 ++++++++++++++++
>  1 files changed, 16 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> 
> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> new file mode 100644
> index 0000000..e9eb1fe
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> @@ -0,0 +1,16 @@
> +Generic SYSCON mapped register reset driver

Bindings should describe hardware, not drivers.

What precisely does this binding describe?

> +
> +Required properties:
> +- compatible: should contain "syscon-reboot"
> +- regmap: this is phandle to the register map node 
> +- offset: offset in the register map for the reboot register
> +- mask: the reset value written to the reboot register
> +
> +Examples:
> +
> +reboot {
> +   compatible = "syscon-reboot";
> +   regmap = <&regmapnode>;
> +   offset = <0x0>;
> +   mask = <0x1>;
> +};

Access size? Endianness?

Why can we not have a binding for the register bank this exists in, and
have that pass on the appropriate details to a syscon-reboot driver?

That way we can change the way we poke things without requiring changes
to bindings or dts.

Thanks,
Mark.
Christopher Covington Jan. 24, 2014, 5:55 p.m. UTC | #2
On 01/24/2014 06:39 AM, Mark Rutland wrote:
> On Thu, Jan 23, 2014 at 07:20:01PM +0000, Feng Kan wrote:
>> Add documentation for generic SYSCON reboot driver.
>>
>> Signed-off-by: Feng Kan <fkan@apm.com>
>> ---
>>  .../bindings/power/reset/syscon-reboot.txt         |   16 ++++++++++++++++
>>  1 files changed, 16 insertions(+), 0 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>>
>> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>> new file mode 100644
>> index 0000000..e9eb1fe
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>> @@ -0,0 +1,16 @@
>> +Generic SYSCON mapped register reset driver
> 
> Bindings should describe hardware, not drivers.

How is this different than what's done for PSCI?

Thanks,
Christopher
Feng Kan Jan. 24, 2014, 6:03 p.m. UTC | #3
On Fri, Jan 24, 2014 at 3:39 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Jan 23, 2014 at 07:20:01PM +0000, Feng Kan wrote:
>> Add documentation for generic SYSCON reboot driver.
>>
>> Signed-off-by: Feng Kan <fkan@apm.com>
>> ---
>>  .../bindings/power/reset/syscon-reboot.txt         |   16 ++++++++++++++++
>>  1 files changed, 16 insertions(+), 0 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>>
>> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>> new file mode 100644
>> index 0000000..e9eb1fe
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>> @@ -0,0 +1,16 @@
>> +Generic SYSCON mapped register reset driver
>
> Bindings should describe hardware, not drivers.
>
> What precisely does this binding describe?
>
>> +
>> +Required properties:
>> +- compatible: should contain "syscon-reboot"
>> +- regmap: this is phandle to the register map node
>> +- offset: offset in the register map for the reboot register
>> +- mask: the reset value written to the reboot register
>> +
>> +Examples:
>> +
>> +reboot {
>> +   compatible = "syscon-reboot";
>> +   regmap = <&regmapnode>;
>> +   offset = <0x0>;
>> +   mask = <0x1>;
>> +};
>
> Access size? Endianness?
FKAN: are you asking for documentation? I don't see alot of example of
support for these.

>
> Why can we not have a binding for the register bank this exists in, and
> have that pass on the appropriate details to a syscon-reboot driver?

FKAN: Thats a good idea. But the hardware in this case (SCU) system
clock unit has a bunch of registers used for different functions. If syscon is
used alot in this case and we pile more attribute into it. It would get kinda
messy after a while.

FKAN: I still haven't figured out how to generically tie to
the reset handler? Maybe the next person can use #define to bridge in the
reset handler they want to use.

>
> That way we can change the way we poke things without requiring changes
> to bindings or dts.
>
> Thanks,
> Mark.
Mark Rutland Jan. 24, 2014, 6:19 p.m. UTC | #4
On Fri, Jan 24, 2014 at 05:55:13PM +0000, Christopher Covington wrote:
> On 01/24/2014 06:39 AM, Mark Rutland wrote:
> > On Thu, Jan 23, 2014 at 07:20:01PM +0000, Feng Kan wrote:
> >> Add documentation for generic SYSCON reboot driver.
> >>
> >> Signed-off-by: Feng Kan <fkan@apm.com>
> >> ---
> >>  .../bindings/power/reset/syscon-reboot.txt         |   16 ++++++++++++++++
> >>  1 files changed, 16 insertions(+), 0 deletions(-)
> >>  create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> >> new file mode 100644
> >> index 0000000..e9eb1fe
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> >> @@ -0,0 +1,16 @@
> >> +Generic SYSCON mapped register reset driver
> > 
> > Bindings should describe hardware, not drivers.
> 
> How is this different than what's done for PSCI?

A PSCI node in the DT defines a standard interface to a firmware which
is external to Linux. The PSCI binding does not contain the word
"driver", but describes the interface that the binding describes, with
reference to approriate documentation.

All I'm arguing for here is a description of the class of hardware this
is applicable to, rather than "this is what this particular driver
uses".

Thanks,
Mark.
Mark Rutland Jan. 24, 2014, 6:23 p.m. UTC | #5
On Fri, Jan 24, 2014 at 06:03:10PM +0000, Feng Kan wrote:
> On Fri, Jan 24, 2014 at 3:39 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Thu, Jan 23, 2014 at 07:20:01PM +0000, Feng Kan wrote:
> >> Add documentation for generic SYSCON reboot driver.
> >>
> >> Signed-off-by: Feng Kan <fkan@apm.com>
> >> ---
> >>  .../bindings/power/reset/syscon-reboot.txt         |   16 ++++++++++++++++
> >>  1 files changed, 16 insertions(+), 0 deletions(-)
> >>  create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> >> new file mode 100644
> >> index 0000000..e9eb1fe
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
> >> @@ -0,0 +1,16 @@
> >> +Generic SYSCON mapped register reset driver
> >
> > Bindings should describe hardware, not drivers.
> >
> > What precisely does this binding describe?
> >
> >> +
> >> +Required properties:
> >> +- compatible: should contain "syscon-reboot"
> >> +- regmap: this is phandle to the register map node
> >> +- offset: offset in the register map for the reboot register
> >> +- mask: the reset value written to the reboot register
> >> +
> >> +Examples:
> >> +
> >> +reboot {
> >> +   compatible = "syscon-reboot";
> >> +   regmap = <&regmapnode>;
> >> +   offset = <0x0>;
> >> +   mask = <0x1>;
> >> +};
> >
> > Access size? Endianness?
> FKAN: are you asking for documentation? I don't see alot of example of
> support for these.

If I used the enippet in the example, what endianness and access size
should I expect an OS to perform? That should be documented.

If this doesn't match the general case, we can add properties later to
adjust the access size and/or endianness. We just need to document what
the binding actually describes currently, or it's not possible to
implement anything based off of the binding documentation.

I should be able to read a binding document and write a dts. I shouldn't
have to read the code to figure out what the binding describes.

> 
> >
> > Why can we not have a binding for the register bank this exists in, and
> > have that pass on the appropriate details to a syscon-reboot driver?
> 
> FKAN: Thats a good idea. But the hardware in this case (SCU) system
> clock unit has a bunch of registers used for different functions. If syscon is
> used alot in this case and we pile more attribute into it. It would get kinda
> messy after a while.

Huh?

What's wrong with having a system clock unit binding, that the kernel
can decompose as appropriate?

I don't get your syscon argument.

Thanks,
Mark.
Feng Kan Jan. 24, 2014, 6:44 p.m. UTC | #6
On Fri, Jan 24, 2014 at 10:23 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Fri, Jan 24, 2014 at 06:03:10PM +0000, Feng Kan wrote:
>> On Fri, Jan 24, 2014 at 3:39 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > On Thu, Jan 23, 2014 at 07:20:01PM +0000, Feng Kan wrote:
>> >> Add documentation for generic SYSCON reboot driver.
>> >>
>> >> Signed-off-by: Feng Kan <fkan@apm.com>
>> >> ---
>> >>  .../bindings/power/reset/syscon-reboot.txt         |   16 ++++++++++++++++
>> >>  1 files changed, 16 insertions(+), 0 deletions(-)
>> >>  create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>> >> new file mode 100644
>> >> index 0000000..e9eb1fe
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>> >> @@ -0,0 +1,16 @@
>> >> +Generic SYSCON mapped register reset driver
>> >
>> > Bindings should describe hardware, not drivers.
>> >
>> > What precisely does this binding describe?
>> >
>> >> +
>> >> +Required properties:
>> >> +- compatible: should contain "syscon-reboot"
>> >> +- regmap: this is phandle to the register map node
>> >> +- offset: offset in the register map for the reboot register
>> >> +- mask: the reset value written to the reboot register
>> >> +
>> >> +Examples:
>> >> +
>> >> +reboot {
>> >> +   compatible = "syscon-reboot";
>> >> +   regmap = <&regmapnode>;
>> >> +   offset = <0x0>;
>> >> +   mask = <0x1>;
>> >> +};
>> >
>> > Access size? Endianness?
>> FKAN: are you asking for documentation? I don't see alot of example of
>> support for these.
>
> If I used the enippet in the example, what endianness and access size
> should I expect an OS to perform? That should be documented.
>
> If this doesn't match the general case, we can add properties later to
> adjust the access size and/or endianness. We just need to document what
> the binding actually describes currently, or it's not possible to
> implement anything based off of the binding documentation.
>
> I should be able to read a binding document and write a dts. I shouldn't
> have to read the code to figure out what the binding describes.
>
>>
>> >
>> > Why can we not have a binding for the register bank this exists in, and
>> > have that pass on the appropriate details to a syscon-reboot driver?
>>
>> FKAN: Thats a good idea. But the hardware in this case (SCU) system
>> clock unit has a bunch of registers used for different functions. If syscon is
>> used alot in this case and we pile more attribute into it. It would get kinda
>> messy after a while.
>
> Huh?
>
> What's wrong with having a system clock unit binding, that the kernel
> can decompose as appropriate?
>
> I don't get your syscon argument.

FKAN: I do have a SCU binding, I thought you wanted to move the offset and
mask to the SCU binding. The only issue I see in that case is when we use
more such methods, the SCU binding would look rather crowded. If this is not
the case, I am a bit confused at what I should do next.

>
> Thanks,
> Mark.
Marc C Jan. 24, 2014, 11:16 p.m. UTC | #7
Hi Mark,

>> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>> new file mode 100644
>> index 0000000..e9eb1fe
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>> @@ -0,0 +1,16 @@
>> +Generic SYSCON mapped register reset driver
>
> Bindings should describe hardware, not drivers

In a perfect world, the hardware designers would place _all_ of the registers needed to
support rebooting in a contiguous section of the memory map. However, this isn't the case
on some platforms, especially on ARM-based SoCs.

While I completely agree with you that the bindings describe hardware, I don't see how
Feng's work is contrary to that. Feng is working on logically-grouping an otherwise
"random" set of registers into a logical grouping. In this case, Feng is uniting a group
of registers and calling them the "reboot" register block.

> What's wrong with having a system clock unit binding, that the kernel
> can decompose as appropriate?

From what I understand, the arm-soc maintainers want to reduce (and perhaps even
eliminate) these board-specific constructs, and try to utilize common driver-code that
resides in the "driver" folder. I can vouch for the syscon/regmap framework as something
which would enable the effort.

Thanks,
Marc C

On 01/24/2014 10:23 AM, Mark Rutland wrote:
> On Fri, Jan 24, 2014 at 06:03:10PM +0000, Feng Kan wrote:
>> On Fri, Jan 24, 2014 at 3:39 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>>> On Thu, Jan 23, 2014 at 07:20:01PM +0000, Feng Kan wrote:
>>>> Add documentation for generic SYSCON reboot driver.
>>>>
>>>> Signed-off-by: Feng Kan <fkan@apm.com>
>>>> ---
>>>>  .../bindings/power/reset/syscon-reboot.txt         |   16 ++++++++++++++++
>>>>  1 files changed, 16 insertions(+), 0 deletions(-)
>>>>  create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>>>> new file mode 100644
>>>> index 0000000..e9eb1fe
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
>>>> @@ -0,0 +1,16 @@
>>>> +Generic SYSCON mapped register reset driver
>>>
>>> Bindings should describe hardware, not drivers.
>>>
>>> What precisely does this binding describe?
>>>
>>>> +
>>>> +Required properties:
>>>> +- compatible: should contain "syscon-reboot"
>>>> +- regmap: this is phandle to the register map node
>>>> +- offset: offset in the register map for the reboot register
>>>> +- mask: the reset value written to the reboot register
>>>> +
>>>> +Examples:
>>>> +
>>>> +reboot {
>>>> +   compatible = "syscon-reboot";
>>>> +   regmap = <&regmapnode>;
>>>> +   offset = <0x0>;
>>>> +   mask = <0x1>;
>>>> +};
>>>
>>> Access size? Endianness?
>> FKAN: are you asking for documentation? I don't see alot of example of
>> support for these.
> 
> If I used the enippet in the example, what endianness and access size
> should I expect an OS to perform? That should be documented.
> 
> If this doesn't match the general case, we can add properties later to
> adjust the access size and/or endianness. We just need to document what
> the binding actually describes currently, or it's not possible to
> implement anything based off of the binding documentation.
> 
> I should be able to read a binding document and write a dts. I shouldn't
> have to read the code to figure out what the binding describes.
> 
>>
>>>
>>> Why can we not have a binding for the register bank this exists in, and
>>> have that pass on the appropriate details to a syscon-reboot driver?
>>
>> FKAN: Thats a good idea. But the hardware in this case (SCU) system
>> clock unit has a bunch of registers used for different functions. If syscon is
>> used alot in this case and we pile more attribute into it. It would get kinda
>> messy after a while.
> 
> Huh?
> 
> What's wrong with having a system clock unit binding, that the kernel
> can decompose as appropriate?
> 
> I don't get your syscon argument.
> 
> Thanks,
> Mark.
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Arnd Bergmann Jan. 29, 2014, 3:08 p.m. UTC | #8
On Friday 24 January 2014 15:16:32 Marc C wrote:
> 
> > What's wrong with having a system clock unit binding, that the kernel
> > can decompose as appropriate?
> 
> From what I understand, the arm-soc maintainers want to reduce (and perhaps even
> eliminate) these board-specific constructs, and try to utilize common driver-code that
> resides in the "driver" folder. I can vouch for the syscon/regmap framework as something
> which would enable the effort.

While your statement is true in general, it doesn't seem to counter
what Mark R said above.

	Arnd
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
new file mode 100644
index 0000000..e9eb1fe
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.txt
@@ -0,0 +1,16 @@ 
+Generic SYSCON mapped register reset driver
+
+Required properties:
+- compatible: should contain "syscon-reboot"
+- regmap: this is phandle to the register map node 
+- offset: offset in the register map for the reboot register
+- mask: the reset value written to the reboot register
+
+Examples:
+
+reboot {
+   compatible = "syscon-reboot";
+   regmap = <&regmapnode>;
+   offset = <0x0>;
+   mask = <0x1>;
+};