| Message ID | 1437783682-13632-2-git-send-email-moritz.fischer@ettus.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
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
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 -- 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
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 -- 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
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 -- 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 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
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 -- 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 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 -- 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
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 -- 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
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 -- 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 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 -- 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
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 -- 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 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 -- 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 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 -- 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
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>; + };
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