diff mbox

[RFCv2,1/3] docs: dts: Added documentation for Xilinx Zynq Reset Controller bindings.

Message ID 1437783682-13632-2-git-send-email-moritz.fischer@ettus.com (mailing list archive)
State New, archived
Headers show

Commit Message

Moritz Fischer July 25, 2015, 12:21 a.m. UTC
Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
---
 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt | 13 +++++++++++++
 1 file changed, 13 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt

Comments

Michal Simek July 27, 2015, 5:09 a.m. UTC | #1
On 07/25/2015 02:21 AM, Moritz Fischer wrote:
> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
> ---
>  Documentation/devicetree/bindings/reset/zynq-reset-pl.txt | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> 
> diff --git a/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> new file mode 100644
> index 0000000..ac4499e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> @@ -0,0 +1,13 @@
> +Xilinx Zynq PL Reset Manager

here

> +
> +Required properties:
> +- compatible: "xlnx,zynq-reset-pl"

Currently it is not just PL reset controller.

> +- syscon <&slcr>;


missing : and please be more descriptive here.

> +- #reset-cells: 1
> +
> +Example:
> +	rstc: rstc@240 {
> +		#reset-cells = <1>;
> +		compatible = "xlnx,zynq-reset-pl";

Compatible property should go first.

I am missing that reg property

> +		syscon = <&slcr>;
> +	};
> 

Thanks,
Michal
Soren Brinkmann July 28, 2015, 2:58 a.m. UTC | #2
Hi Moritz,

On Fri, 2015-07-24 at 05:21PM -0700, Moritz Fischer wrote:
> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
> ---
>  Documentation/devicetree/bindings/reset/zynq-reset-pl.txt | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> 
> diff --git a/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> new file mode 100644
> index 0000000..ac4499e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> @@ -0,0 +1,13 @@
> +Xilinx Zynq PL Reset Manager
> +
> +Required properties:
> +- compatible: "xlnx,zynq-reset-pl"
> +- syscon <&slcr>;
> +- #reset-cells: 1
> +
> +Example:
> +	rstc: rstc@240 {
> +		#reset-cells = <1>;
> +		compatible = "xlnx,zynq-reset-pl";
> +		syscon = <&slcr>;
> +	};

I think you also have to add the outputs and make them part of the
binding. Otherwise you'd have to read the implementation to find
out what device should be hooked up to which output of the reset-controller.

	Sören
Moritz Fischer July 28, 2015, 4:52 a.m. UTC | #3
Hi Sören,

thanks for your feedback.

On Mon, Jul 27, 2015 at 7:58 PM, Sören Brinkmann
<soren.brinkmann@xilinx.com> wrote:
> Hi Moritz,
>
> On Fri, 2015-07-24 at 05:21PM -0700, Moritz Fischer wrote:
>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
>> ---
>>  Documentation/devicetree/bindings/reset/zynq-reset-pl.txt | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>>
>> diff --git a/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>> new file mode 100644
>> index 0000000..ac4499e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>> @@ -0,0 +1,13 @@
>> +Xilinx Zynq PL Reset Manager
>> +
>> +Required properties:
>> +- compatible: "xlnx,zynq-reset-pl"
>> +- syscon <&slcr>;
>> +- #reset-cells: 1
>> +
>> +Example:
>> +     rstc: rstc@240 {
>> +             #reset-cells = <1>;
>> +             compatible = "xlnx,zynq-reset-pl";
>> +             syscon = <&slcr>;
>> +     };
>
> I think you also have to add the outputs and make them part of the
> binding. Otherwise you'd have to read the implementation to find
> out what device should be hooked up to which output of the reset-controller.

Is something like this what you had in mind? I had that prepared for
the next round of patches:

Reset outputs:
 0  : soft reset
 32 : ddr reset
 64 : topsw reset
 96 : dmac reset
 128: usb0 reset
 129: usb1 reset
 160: gem0 reset
 161: gem1 reset
 164: gem0 rx reset
 165: gem1 rx reset
 166: gem0 ref reset
 167: gem1 ref reset
 192: sdio0 reset
 193: sdio1 reset
 196: sdio0 ref reset
 197: sdio1 ref reset
 224: spi0 reset
 225: spi1 reset
 226: spi0 ref reset
 227: spi1 ref reset
 256: can0 reset
 257: can1 reset
 258: can0 ref reset
 259: can1 ref reset
 288: i2c0 reset
 289: i2c1 reset
 320: uart0 reset
 321: uart1 reset
 322: uart0 ref reset
 323: uart1 ref reset
 352: gpio reset
 384: lqspi reset
 385: qspi ref reset
 416: smc reset
 417: smc ref reset
 448: ocm reset
 512: fpga0 out reset
 513: fpga1 out reset
 514: fpga2 out reset
 515: fpga3 out reset
 544: a9 reset 0
 545: a9 reset 1
 552: peri reset

>         Sören
Moritz Fischer July 28, 2015, 4:55 a.m. UTC | #4
Hi Michal,

On Sun, Jul 26, 2015 at 10:09 PM, Michal Simek <monstr@monstr.eu> wrote:
> On 07/25/2015 02:21 AM, Moritz Fischer wrote:
>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
>> ---
>>  Documentation/devicetree/bindings/reset/zynq-reset-pl.txt | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>>
>> diff --git a/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>> new file mode 100644
>> index 0000000..ac4499e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>> @@ -0,0 +1,13 @@
>> +Xilinx Zynq PL Reset Manager
>
> here
>
>> +
>> +Required properties:
>> +- compatible: "xlnx,zynq-reset-pl"
>
> Currently it is not just PL reset controller.
>
>> +- syscon <&slcr>;
>
>
> missing : and please be more descriptive here.
>
>> +- #reset-cells: 1
>> +
>> +Example:
>> +     rstc: rstc@240 {
>> +             #reset-cells = <1>;
>> +             compatible = "xlnx,zynq-reset-pl";
>
> Compatible property should go first.
>
> I am missing that reg property
>
>> +             syscon = <&slcr>;
>> +     };
>>
>
Would something like this work:

Xilinx Zynq Reset Manager

The Zynq AP-SoC has several different resets.

See Chapter 26 of the Zynq TRM (UG585) for more information about Zynq resets.

Required properties:
- compatible: "xlnx,zynq-reset"
- reg: SLCR offset and size taken via syscon <0x200 0x50>
- syscon: <&slcr>
  This should be a phandle to the Zynq's SLCR register.
- #reset-cells: Must be 1

The Zynq Reset Manager needs to be a child node of the SLCR.

Example:
        rstc: rstc@200 {
                compatible = "xlnx,zynq-reset";
                reg = <0x200 0x50>;
                #reset-cells = <1>;
                syscon = <&slcr>;
        };

> Thanks,
> Michal
>
>
> --
> Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
> w: www.monstr.eu p: +42-0-721842854
> Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
> Maintainer of Linux kernel - Xilinx Zynq ARM architecture
> Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
>
>

Thanks for your feedback,

Moritz
Michal Simek July 28, 2015, 5:41 a.m. UTC | #5
On 07/28/2015 06:55 AM, Moritz Fischer wrote:
> Hi Michal,
> 
> On Sun, Jul 26, 2015 at 10:09 PM, Michal Simek <monstr@monstr.eu> wrote:
>> On 07/25/2015 02:21 AM, Moritz Fischer wrote:
>>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
>>> ---
>>>  Documentation/devicetree/bindings/reset/zynq-reset-pl.txt | 13 +++++++++++++
>>>  1 file changed, 13 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>>> new file mode 100644
>>> index 0000000..ac4499e
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>>> @@ -0,0 +1,13 @@
>>> +Xilinx Zynq PL Reset Manager
>>
>> here
>>
>>> +
>>> +Required properties:
>>> +- compatible: "xlnx,zynq-reset-pl"
>>
>> Currently it is not just PL reset controller.
>>
>>> +- syscon <&slcr>;
>>
>>
>> missing : and please be more descriptive here.
>>
>>> +- #reset-cells: 1
>>> +
>>> +Example:
>>> +     rstc: rstc@240 {
>>> +             #reset-cells = <1>;
>>> +             compatible = "xlnx,zynq-reset-pl";
>>
>> Compatible property should go first.
>>
>> I am missing that reg property
>>
>>> +             syscon = <&slcr>;
>>> +     };
>>>
>>
> Would something like this work:
> 
> Xilinx Zynq Reset Manager
> 
> The Zynq AP-SoC has several different resets.
> 
> See Chapter 26 of the Zynq TRM (UG585) for more information about Zynq resets.
> 
> Required properties:
> - compatible: "xlnx,zynq-reset"
> - reg: SLCR offset and size taken via syscon <0x200 0x50>
> - syscon: <&slcr>
>   This should be a phandle to the Zynq's SLCR register.
> - #reset-cells: Must be 1
> 
> The Zynq Reset Manager needs to be a child node of the SLCR.
> 
> Example:
>         rstc: rstc@200 {
>                 compatible = "xlnx,zynq-reset";
>                 reg = <0x200 0x50>;
>                 #reset-cells = <1>;
>                 syscon = <&slcr>;
>         };

Looks good to me.

Thanks,
Michal
Philipp Zabel July 28, 2015, 8:05 a.m. UTC | #6
Am Freitag, den 24.07.2015, 17:21 -0700 schrieb Moritz Fischer:
> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
> ---
>  Documentation/devicetree/bindings/reset/zynq-reset-pl.txt | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> 
> diff --git a/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> new file mode 100644
> index 0000000..ac4499e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> @@ -0,0 +1,13 @@
> +Xilinx Zynq PL Reset Manager
> +
> +Required properties:
> +- compatible: "xlnx,zynq-reset-pl"
> +- syscon <&slcr>;
> +- #reset-cells: 1
> +
> +Example:
> +	rstc: rstc@240 {
> +		#reset-cells = <1>;
> +		compatible = "xlnx,zynq-reset-pl";
> +		syscon = <&slcr>;

Why the syscon phandle if rstc always is the child of slcr? Why not just
request the syscon for the rstc's parent node.

> +	};

regards
Philipp
Michal Simek July 28, 2015, 8:25 a.m. UTC | #7
On 07/28/2015 10:05 AM, Philipp Zabel wrote:
> Am Freitag, den 24.07.2015, 17:21 -0700 schrieb Moritz Fischer:
>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
>> ---
>>  Documentation/devicetree/bindings/reset/zynq-reset-pl.txt | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>>
>> diff --git a/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>> new file mode 100644
>> index 0000000..ac4499e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>> @@ -0,0 +1,13 @@
>> +Xilinx Zynq PL Reset Manager
>> +
>> +Required properties:
>> +- compatible: "xlnx,zynq-reset-pl"
>> +- syscon <&slcr>;
>> +- #reset-cells: 1
>> +
>> +Example:
>> +	rstc: rstc@240 {
>> +		#reset-cells = <1>;
>> +		compatible = "xlnx,zynq-reset-pl";
>> +		syscon = <&slcr>;
> 
> Why the syscon phandle if rstc always is the child of slcr? Why not just
> request the syscon for the rstc's parent node.

We are using this description for pincntrl which was properly reviewed
that's why I expect Moritz just use the same style.
But yes also referencing parent should work.

TBH I don't have strong preference but having unified style is something
what I would prefer.

Also I see that using parent is used by others and it looks like that
having something like syscon_regmap_lookup_parent will be worth to have.

Thanks,
Michal
Moritz Fischer July 28, 2015, 1:57 p.m. UTC | #8
Philip, Michal,

thanks for your reviews.

On Tue, Jul 28, 2015 at 1:25 AM, Michal Simek <michal.simek@xilinx.com> wrote:
> On 07/28/2015 10:05 AM, Philipp Zabel wrote:
>> Am Freitag, den 24.07.2015, 17:21 -0700 schrieb Moritz Fischer:
>>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
>>> ---
>>>  Documentation/devicetree/bindings/reset/zynq-reset-pl.txt | 13 +++++++++++++
>>>  1 file changed, 13 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>>> new file mode 100644
>>> index 0000000..ac4499e
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>>> @@ -0,0 +1,13 @@
>>> +Xilinx Zynq PL Reset Manager
>>> +
>>> +Required properties:
>>> +- compatible: "xlnx,zynq-reset-pl"
>>> +- syscon <&slcr>;
>>> +- #reset-cells: 1
>>> +
>>> +Example:
>>> +    rstc: rstc@240 {
>>> +            #reset-cells = <1>;
>>> +            compatible = "xlnx,zynq-reset-pl";
>>> +            syscon = <&slcr>;
>>
>> Why the syscon phandle if rstc always is the child of slcr? Why not just
>> request the syscon for the rstc's parent node.
>
> We are using this description for pincntrl which was properly reviewed
> that's why I expect Moritz just use the same style.
> But yes also referencing parent should work.

Michal is right, I tried to be consistent with the pinctrl. Either one
is fine for me.
We'll just have to make a decision :-)
>
> TBH I don't have strong preference but having unified style is something
> what I would prefer.
>
> Also I see that using parent is used by others and it looks like that
> having something like syscon_regmap_lookup_parent will be worth to have.
>
> Thanks,
> Michal
>
>
Moritz
Philipp Zabel July 28, 2015, 3:16 p.m. UTC | #9
Hi Moritz, Michal,

Am Dienstag, den 28.07.2015, 06:57 -0700 schrieb Moritz Fischer:
[...]
> >>> +Example:
> >>> +    rstc: rstc@240 {
> >>> +            #reset-cells = <1>;
> >>> +            compatible = "xlnx,zynq-reset-pl";
> >>> +            syscon = <&slcr>;
> >>
> >> Why the syscon phandle if rstc always is the child of slcr? Why not just
> >> request the syscon for the rstc's parent node.
> >
> > We are using this description for pincntrl which was properly reviewed
> > that's why I expect Moritz just use the same style.
> > But yes also referencing parent should work.
> 
> Michal is right, I tried to be consistent with the pinctrl. Either one
> is fine for me.
> We'll just have to make a decision :-)

Do you have a pointer to the pinctrl review? I'd like to know if
somebody had a good reason to use the phandle over the parent-child
relationship. I'd rather not add DT properties if they are not
necessary.
Regarding consistency, since the pinctrl node is also a child of the
slcr, you could just as well make the syscon phandle optional there and
remove it from the DT without breaking backwards compatibility.

> > TBH I don't have strong preference but having unified style is something
> > what I would prefer.
> >
> > Also I see that using parent is used by others and it looks like that
> > having something like syscon_regmap_lookup_parent will be worth to have.

That would be useful, yes.

regards
Philipp
Soren Brinkmann July 28, 2015, 10:53 p.m. UTC | #10
On Mon, 2015-07-27 at 09:52PM -0700, Moritz Fischer wrote:
> Hi Sören,
> 
> thanks for your feedback.
> 
> On Mon, Jul 27, 2015 at 7:58 PM, Sören Brinkmann
> <soren.brinkmann@xilinx.com> wrote:
> > Hi Moritz,
> >
> > On Fri, 2015-07-24 at 05:21PM -0700, Moritz Fischer wrote:
> >> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
> >> ---
> >>  Documentation/devicetree/bindings/reset/zynq-reset-pl.txt | 13 +++++++++++++
> >>  1 file changed, 13 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> >> new file mode 100644
> >> index 0000000..ac4499e
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> >> @@ -0,0 +1,13 @@
> >> +Xilinx Zynq PL Reset Manager
> >> +
> >> +Required properties:
> >> +- compatible: "xlnx,zynq-reset-pl"
> >> +- syscon <&slcr>;
> >> +- #reset-cells: 1
> >> +
> >> +Example:
> >> +     rstc: rstc@240 {
> >> +             #reset-cells = <1>;
> >> +             compatible = "xlnx,zynq-reset-pl";
> >> +             syscon = <&slcr>;
> >> +     };
> >
> > I think you also have to add the outputs and make them part of the
> > binding. Otherwise you'd have to read the implementation to find
> > out what device should be hooked up to which output of the reset-controller.
> 
> Is something like this what you had in mind? I had that prepared for
> the next round of patches:
> 
> Reset outputs:
>  0  : soft reset
>  32 : ddr reset
>  64 : topsw reset
>  96 : dmac reset
>  128: usb0 reset
>  129: usb1 reset
>  160: gem0 reset
>  161: gem1 reset
>  164: gem0 rx reset
>  165: gem1 rx reset
>  166: gem0 ref reset
>  167: gem1 ref reset
>  192: sdio0 reset
>  193: sdio1 reset
>  196: sdio0 ref reset
>  197: sdio1 ref reset
>  224: spi0 reset
>  225: spi1 reset
>  226: spi0 ref reset
>  227: spi1 ref reset
>  256: can0 reset
>  257: can1 reset
>  258: can0 ref reset
>  259: can1 ref reset
>  288: i2c0 reset
>  289: i2c1 reset
>  320: uart0 reset
>  321: uart1 reset
>  322: uart0 ref reset
>  323: uart1 ref reset
>  352: gpio reset
>  384: lqspi reset
>  385: qspi ref reset
>  416: smc reset
>  417: smc ref reset
>  448: ocm reset
>  512: fpga0 out reset
>  513: fpga1 out reset
>  514: fpga2 out reset
>  515: fpga3 out reset
>  544: a9 reset 0
>  545: a9 reset 1
>  552: peri reset

Basically, yes. I guess the gaps are due to directly mapping this number
to bank and bit instead of doing some more complex mapping in between?
I'm not sure whether I like this :) I guess if a number is off the
driver would still toggle the addressed bit? I'm not sure, is it worth
to do some explicit mapping from logical outputs to a physical reset? It
seems it would be a little safer since it would be easy to check that
the addressed reset is valid and there wouldn't be any reserved/invalid
bits be toggled. Also, it would make the outputs in here a continuous
series of numbers without these gaps. Not sure though whether
it's worth the additional complexity in the implementation.

	Thanks,
	Sören
Moritz Fischer July 29, 2015, 6:14 a.m. UTC | #11
Hi Sören,

On Tue, Jul 28, 2015 at 3:53 PM, Sören Brinkmann
<soren.brinkmann@xilinx.com> wrote:
> On Mon, 2015-07-27 at 09:52PM -0700, Moritz Fischer wrote:
>> Hi Sören,
>>
>> thanks for your feedback.
>>
>> On Mon, Jul 27, 2015 at 7:58 PM, Sören Brinkmann
>> <soren.brinkmann@xilinx.com> wrote:
>> > Hi Moritz,
>> >
>> > On Fri, 2015-07-24 at 05:21PM -0700, Moritz Fischer wrote:
>> >> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
>> >> ---
>> >>  Documentation/devicetree/bindings/reset/zynq-reset-pl.txt | 13 +++++++++++++
>> >>  1 file changed, 13 insertions(+)
>> >>  create mode 100644 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>> >> new file mode 100644
>> >> index 0000000..ac4499e
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>> >> @@ -0,0 +1,13 @@
>> >> +Xilinx Zynq PL Reset Manager
>> >> +
>> >> +Required properties:
>> >> +- compatible: "xlnx,zynq-reset-pl"
>> >> +- syscon <&slcr>;
>> >> +- #reset-cells: 1
>> >> +
>> >> +Example:
>> >> +     rstc: rstc@240 {
>> >> +             #reset-cells = <1>;
>> >> +             compatible = "xlnx,zynq-reset-pl";
>> >> +             syscon = <&slcr>;
>> >> +     };
>> >
>> > I think you also have to add the outputs and make them part of the
>> > binding. Otherwise you'd have to read the implementation to find
>> > out what device should be hooked up to which output of the reset-controller.
>>
>> Is something like this what you had in mind? I had that prepared for
>> the next round of patches:
>>
>> Reset outputs:
>>  0  : soft reset
>>  32 : ddr reset
>>  64 : topsw reset
>>  96 : dmac reset
>>  128: usb0 reset
>>  129: usb1 reset
>>  160: gem0 reset
>>  161: gem1 reset
>>  164: gem0 rx reset
>>  165: gem1 rx reset
>>  166: gem0 ref reset
>>  167: gem1 ref reset
>>  192: sdio0 reset
>>  193: sdio1 reset
>>  196: sdio0 ref reset
>>  197: sdio1 ref reset
>>  224: spi0 reset
>>  225: spi1 reset
>>  226: spi0 ref reset
>>  227: spi1 ref reset
>>  256: can0 reset
>>  257: can1 reset
>>  258: can0 ref reset
>>  259: can1 ref reset
>>  288: i2c0 reset
>>  289: i2c1 reset
>>  320: uart0 reset
>>  321: uart1 reset
>>  322: uart0 ref reset
>>  323: uart1 ref reset
>>  352: gpio reset
>>  384: lqspi reset
>>  385: qspi ref reset
>>  416: smc reset
>>  417: smc ref reset
>>  448: ocm reset
>>  512: fpga0 out reset
>>  513: fpga1 out reset
>>  514: fpga2 out reset
>>  515: fpga3 out reset
>>  544: a9 reset 0
>>  545: a9 reset 1
>>  552: peri reset
>
> Basically, yes. I guess the gaps are due to directly mapping this number
> to bank and bit instead of doing some more complex mapping in between?
> I'm not sure whether I like this :) I guess if a number is off the
> driver would still toggle the addressed bit?

My assumption was that people would use a #include
<dt-bindings/xlnx,zynq-reset.h> in their dts. Assuming I got the
numbers in there right this makes it hard to misuse by accident.
I'm not saying it's perfect ...

> I'm not sure, is it worth
> to do some explicit mapping from logical outputs to a physical reset? It
> seems it would be a little safer since it would be easy to check that
> the addressed reset is valid and there wouldn't be any reserved/invalid
> bits be toggled. Also, it would make the outputs in here a continuous
> series of numbers without these gaps. Not sure though whether
> it's worth the additional complexity in the implementation.

So your suggestion is to have a large switch case kind of thingy? I
thought about it and it seemed complex as there's quite a bunch of
resets with gaps. Do you know of a driver that does something similar?
When I wrote this I looked at the other reset drivers and figured they
all had this kind of bank mapping of sorts so I assumed that's how
people usually do it.

>
>         Thanks,
>         Sören

Thanks again for your input,

Moritz
Soren Brinkmann July 29, 2015, 5:38 p.m. UTC | #12
On Tue, 2015-07-28 at 11:14PM -0700, Moritz Fischer wrote:
> Hi Sören,
> 
> On Tue, Jul 28, 2015 at 3:53 PM, Sören Brinkmann
> <soren.brinkmann@xilinx.com> wrote:
> > On Mon, 2015-07-27 at 09:52PM -0700, Moritz Fischer wrote:
> >> Hi Sören,
> >>
> >> thanks for your feedback.
> >>
> >> On Mon, Jul 27, 2015 at 7:58 PM, Sören Brinkmann
> >> <soren.brinkmann@xilinx.com> wrote:
> >> > Hi Moritz,
> >> >
> >> > On Fri, 2015-07-24 at 05:21PM -0700, Moritz Fischer wrote:
> >> >> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
> >> >> ---
> >> >>  Documentation/devicetree/bindings/reset/zynq-reset-pl.txt | 13 +++++++++++++
> >> >>  1 file changed, 13 insertions(+)
> >> >>  create mode 100644 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> >> >>
> >> >> diff --git a/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> >> >> new file mode 100644
> >> >> index 0000000..ac4499e
> >> >> --- /dev/null
> >> >> +++ b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> >> >> @@ -0,0 +1,13 @@
> >> >> +Xilinx Zynq PL Reset Manager
> >> >> +
> >> >> +Required properties:
> >> >> +- compatible: "xlnx,zynq-reset-pl"
> >> >> +- syscon <&slcr>;
> >> >> +- #reset-cells: 1
> >> >> +
> >> >> +Example:
> >> >> +     rstc: rstc@240 {
> >> >> +             #reset-cells = <1>;
> >> >> +             compatible = "xlnx,zynq-reset-pl";
> >> >> +             syscon = <&slcr>;
> >> >> +     };
> >> >
> >> > I think you also have to add the outputs and make them part of the
> >> > binding. Otherwise you'd have to read the implementation to find
> >> > out what device should be hooked up to which output of the reset-controller.
> >>
> >> Is something like this what you had in mind? I had that prepared for
> >> the next round of patches:
> >>
> >> Reset outputs:
> >>  0  : soft reset
> >>  32 : ddr reset
> >>  64 : topsw reset
> >>  96 : dmac reset
> >>  128: usb0 reset
> >>  129: usb1 reset
> >>  160: gem0 reset
> >>  161: gem1 reset
> >>  164: gem0 rx reset
> >>  165: gem1 rx reset
> >>  166: gem0 ref reset
> >>  167: gem1 ref reset
> >>  192: sdio0 reset
> >>  193: sdio1 reset
> >>  196: sdio0 ref reset
> >>  197: sdio1 ref reset
> >>  224: spi0 reset
> >>  225: spi1 reset
> >>  226: spi0 ref reset
> >>  227: spi1 ref reset
> >>  256: can0 reset
> >>  257: can1 reset
> >>  258: can0 ref reset
> >>  259: can1 ref reset
> >>  288: i2c0 reset
> >>  289: i2c1 reset
> >>  320: uart0 reset
> >>  321: uart1 reset
> >>  322: uart0 ref reset
> >>  323: uart1 ref reset
> >>  352: gpio reset
> >>  384: lqspi reset
> >>  385: qspi ref reset
> >>  416: smc reset
> >>  417: smc ref reset
> >>  448: ocm reset
> >>  512: fpga0 out reset
> >>  513: fpga1 out reset
> >>  514: fpga2 out reset
> >>  515: fpga3 out reset
> >>  544: a9 reset 0
> >>  545: a9 reset 1
> >>  552: peri reset
> >
> > Basically, yes. I guess the gaps are due to directly mapping this number
> > to bank and bit instead of doing some more complex mapping in between?
> > I'm not sure whether I like this :) I guess if a number is off the
> > driver would still toggle the addressed bit?
> 
> My assumption was that people would use a #include
> <dt-bindings/xlnx,zynq-reset.h> in their dts. Assuming I got the
> numbers in there right this makes it hard to misuse by accident.
> I'm not saying it's perfect ...

Michal always turned down the #include patches with the argument of
re-using the dts files outside of the Linux sources where those includes
etc may not be available in this form.

> 
> > I'm not sure, is it worth
> > to do some explicit mapping from logical outputs to a physical reset? It
> > seems it would be a little safer since it would be easy to check that
> > the addressed reset is valid and there wouldn't be any reserved/invalid
> > bits be toggled. Also, it would make the outputs in here a continuous
> > series of numbers without these gaps. Not sure though whether
> > it's worth the additional complexity in the implementation.
> 
> So your suggestion is to have a large switch case kind of thingy? I
> thought about it and it seemed complex as there's quite a bunch of
> resets with gaps. Do you know of a driver that does something similar?

Yeah, that was what I had in mind. Some big switch-case lookup that maps
a logical reset number from DT to the HW.
No, I haven't looked around what other drivers do. So, probably the
right thing is to just do what everybody else does.
I was more thinking about how easy it might be to re-use the driver for
the next-gen device.

> When I wrote this I looked at the other reset drivers and figured they
> all had this kind of bank mapping of sorts so I assumed that's how
> people usually do it.

Yeah, I don't think we should make things overly complicated without a
good reason. So, unless DT or reset folks have any objections, I'm fine
with it.

	Thanks,
	Sören
Michal Simek July 30, 2015, 2:37 p.m. UTC | #13
On 07/29/2015 07:38 PM, Sören Brinkmann wrote:
> On Tue, 2015-07-28 at 11:14PM -0700, Moritz Fischer wrote:
>> Hi Sören,
>>
>> On Tue, Jul 28, 2015 at 3:53 PM, Sören Brinkmann
>> <soren.brinkmann@xilinx.com> wrote:
>>> On Mon, 2015-07-27 at 09:52PM -0700, Moritz Fischer wrote:
>>>> Hi Sören,
>>>>
>>>> thanks for your feedback.
>>>>
>>>> On Mon, Jul 27, 2015 at 7:58 PM, Sören Brinkmann
>>>> <soren.brinkmann@xilinx.com> wrote:
>>>>> Hi Moritz,
>>>>>
>>>>> On Fri, 2015-07-24 at 05:21PM -0700, Moritz Fischer wrote:
>>>>>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
>>>>>> ---
>>>>>>  Documentation/devicetree/bindings/reset/zynq-reset-pl.txt | 13 +++++++++++++
>>>>>>  1 file changed, 13 insertions(+)
>>>>>>  create mode 100644 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..ac4499e
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>>>>>> @@ -0,0 +1,13 @@
>>>>>> +Xilinx Zynq PL Reset Manager
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible: "xlnx,zynq-reset-pl"
>>>>>> +- syscon <&slcr>;
>>>>>> +- #reset-cells: 1
>>>>>> +
>>>>>> +Example:
>>>>>> +     rstc: rstc@240 {
>>>>>> +             #reset-cells = <1>;
>>>>>> +             compatible = "xlnx,zynq-reset-pl";
>>>>>> +             syscon = <&slcr>;
>>>>>> +     };
>>>>>
>>>>> I think you also have to add the outputs and make them part of the
>>>>> binding. Otherwise you'd have to read the implementation to find
>>>>> out what device should be hooked up to which output of the reset-controller.
>>>>
>>>> Is something like this what you had in mind? I had that prepared for
>>>> the next round of patches:
>>>>
>>>> Reset outputs:
>>>>  0  : soft reset
>>>>  32 : ddr reset
>>>>  64 : topsw reset
>>>>  96 : dmac reset
>>>>  128: usb0 reset
>>>>  129: usb1 reset
>>>>  160: gem0 reset
>>>>  161: gem1 reset
>>>>  164: gem0 rx reset
>>>>  165: gem1 rx reset
>>>>  166: gem0 ref reset
>>>>  167: gem1 ref reset
>>>>  192: sdio0 reset
>>>>  193: sdio1 reset
>>>>  196: sdio0 ref reset
>>>>  197: sdio1 ref reset
>>>>  224: spi0 reset
>>>>  225: spi1 reset
>>>>  226: spi0 ref reset
>>>>  227: spi1 ref reset
>>>>  256: can0 reset
>>>>  257: can1 reset
>>>>  258: can0 ref reset
>>>>  259: can1 ref reset
>>>>  288: i2c0 reset
>>>>  289: i2c1 reset
>>>>  320: uart0 reset
>>>>  321: uart1 reset
>>>>  322: uart0 ref reset
>>>>  323: uart1 ref reset
>>>>  352: gpio reset
>>>>  384: lqspi reset
>>>>  385: qspi ref reset
>>>>  416: smc reset
>>>>  417: smc ref reset
>>>>  448: ocm reset
>>>>  512: fpga0 out reset
>>>>  513: fpga1 out reset
>>>>  514: fpga2 out reset
>>>>  515: fpga3 out reset
>>>>  544: a9 reset 0
>>>>  545: a9 reset 1
>>>>  552: peri reset
>>>
>>> Basically, yes. I guess the gaps are due to directly mapping this number
>>> to bank and bit instead of doing some more complex mapping in between?
>>> I'm not sure whether I like this :) I guess if a number is off the
>>> driver would still toggle the addressed bit?
>>
>> My assumption was that people would use a #include
>> <dt-bindings/xlnx,zynq-reset.h> in their dts. Assuming I got the
>> numbers in there right this makes it hard to misuse by accident.
>> I'm not saying it's perfect ...
> 
> Michal always turned down the #include patches with the argument of
> re-using the dts files outside of the Linux sources where those includes
> etc may not be available in this form.

yes. All these includes end up in more work when you want to generate
them or move to any other project.
I don't think that make sense to caused another problem just because of
that. Simple add these values to reset binding doc and then you <&rstc
number> in nodes.
Because we agreed that it has to be done on driver basis this is just
copy that macros from one file to another.

Thanks,
Michal
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
new file mode 100644
index 0000000..ac4499e
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
@@ -0,0 +1,13 @@ 
+Xilinx Zynq PL Reset Manager
+
+Required properties:
+- compatible: "xlnx,zynq-reset-pl"
+- syscon <&slcr>;
+- #reset-cells: 1
+
+Example:
+	rstc: rstc@240 {
+		#reset-cells = <1>;
+		compatible = "xlnx,zynq-reset-pl";
+		syscon = <&slcr>;
+	};