| Message ID | 1383854642-21660-3-git-send-email-valentine.barshak@cogentembedded.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Valentine, Thank you for the patch. Two small comments below. On Friday 08 November 2013 00:04:02 Valentine Barshak wrote: > These bindings can be used to register SATA devices found on R-Car SoC. > > Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com> > Acked-by: Kumar Gala <galak@codeaurora.org> > --- > Documentation/devicetree/bindings/ata/sata_rcar.txt | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > create mode 100644 Documentation/devicetree/bindings/ata/sata_rcar.txt > > diff --git a/Documentation/devicetree/bindings/ata/sata_rcar.txt > b/Documentation/devicetree/bindings/ata/sata_rcar.txt new file mode 100644 > index 0000000..d6b20a6 > --- /dev/null > +++ b/Documentation/devicetree/bindings/ata/sata_rcar.txt > @@ -0,0 +1,18 @@ > +* Renesas R-Car SATA > + > +Required properties: > +- compatible : should contain one of the following: > + - "renesas,sata-r8a7779" for R-Car H1 > + - "renesas,sata-r8a7790" for R-Car H2 > + - "renesas,sata-r8a7791" for R-Car M2 > +- reg : offset and length of the SATA registers; Maybe address instead of offset ? > +- interrupts : must consist of one interrupt specifier. > + > +Example: > + > +sata: sata@fc600000 { > + compatible = "renesas,sata-r8a7779"; > + reg = <0xfc600000 0x2000>; > + interrupt-parent = <&gic>; > + interrupts = <0 100 0x4>; Please use IRQ_TYPE_LEVEL_HIGH instead of 0x4. > +};
Hi Laurent, Simon One question > > +- interrupts : must consist of one interrupt specifier. > > + > > +Example: > > + > > +sata: sata@fc600000 { > > + compatible = "renesas,sata-r8a7779"; > > + reg = <0xfc600000 0x2000>; > > + interrupt-parent = <&gic>; > > + interrupts = <0 100 0x4>; > > Please use IRQ_TYPE_LEVEL_HIGH instead of 0x4. Now, our many SoC codes are using 0x4 in "interrupts". Do you think we should use IRQ_TYPE_LEVEL_HIGH ? Best regards --- Kuninori Morimoto -- 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 Thu, Nov 07, 2013 at 10:27:08PM -0800, Kuninori Morimoto wrote: > > Hi Laurent, Simon > > One question > > > > +- interrupts : must consist of one interrupt specifier. > > > + > > > +Example: > > > + > > > +sata: sata@fc600000 { > > > + compatible = "renesas,sata-r8a7779"; > > > + reg = <0xfc600000 0x2000>; > > > + interrupt-parent = <&gic>; > > > + interrupts = <0 100 0x4>; > > > > Please use IRQ_TYPE_LEVEL_HIGH instead of 0x4. > > Now, our many SoC codes are using 0x4 in "interrupts". > Do you think we should use IRQ_TYPE_LEVEL_HIGH ? Personally I think that would be nice. -- 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 11/08/2013 05:02 AM, Laurent Pinchart wrote: > Hi Valentine, > Hi Laurent, > Thank you for the patch. Two small comments below. > > On Friday 08 November 2013 00:04:02 Valentine Barshak wrote: >> These bindings can be used to register SATA devices found on R-Car SoC. >> >> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com> >> Acked-by: Kumar Gala <galak@codeaurora.org> >> --- >> Documentation/devicetree/bindings/ata/sata_rcar.txt | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/ata/sata_rcar.txt >> >> diff --git a/Documentation/devicetree/bindings/ata/sata_rcar.txt >> b/Documentation/devicetree/bindings/ata/sata_rcar.txt new file mode 100644 >> index 0000000..d6b20a6 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/ata/sata_rcar.txt >> @@ -0,0 +1,18 @@ >> +* Renesas R-Car SATA >> + >> +Required properties: >> +- compatible : should contain one of the following: >> + - "renesas,sata-r8a7779" for R-Car H1 >> + - "renesas,sata-r8a7790" for R-Car H2 >> + - "renesas,sata-r8a7791" for R-Car M2 >> +- reg : offset and length of the SATA registers; > > Maybe address instead of offset ? OK. > >> +- interrupts : must consist of one interrupt specifier. >> + >> +Example: >> + >> +sata: sata@fc600000 { >> + compatible = "renesas,sata-r8a7779"; >> + reg = <0xfc600000 0x2000>; >> + interrupt-parent = <&gic>; >> + interrupts = <0 100 0x4>; > > Please use IRQ_TYPE_LEVEL_HIGH instead of 0x4. OK, I'll do that. Please, note that all the Renesas dts[i] files do not include <dt-bindings/interrupt-controller/irq.h> Thus, blindly following this bindings documentation when adding new SATA nodes to the existing r8a7790.dtsi, for example, will cause DT compilation error. > >> +}; Thanks, Val. -- 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 11/08/2013 03:26 PM, Valentine wrote: > On 11/08/2013 05:02 AM, Laurent Pinchart wrote: >> Hi Valentine, >> > > Hi Laurent, > >> Thank you for the patch. Two small comments below. >> >> On Friday 08 November 2013 00:04:02 Valentine Barshak wrote: >>> These bindings can be used to register SATA devices found on R-Car SoC. >>> >>> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com> >>> Acked-by: Kumar Gala <galak@codeaurora.org> >>> --- >>> Documentation/devicetree/bindings/ata/sata_rcar.txt | 18 ++++++++++++++++++ >>> 1 file changed, 18 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/ata/sata_rcar.txt >>> >>> diff --git a/Documentation/devicetree/bindings/ata/sata_rcar.txt >>> b/Documentation/devicetree/bindings/ata/sata_rcar.txt new file mode 100644 >>> index 0000000..d6b20a6 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/ata/sata_rcar.txt >>> @@ -0,0 +1,18 @@ >>> +* Renesas R-Car SATA >>> + >>> +Required properties: >>> +- compatible : should contain one of the following: >>> + - "renesas,sata-r8a7779" for R-Car H1 >>> + - "renesas,sata-r8a7790" for R-Car H2 >>> + - "renesas,sata-r8a7791" for R-Car M2 >>> +- reg : offset and length of the SATA registers; >> >> Maybe address instead of offset ? > > OK. > >> >>> +- interrupts : must consist of one interrupt specifier. >>> + >>> +Example: >>> + >>> +sata: sata@fc600000 { >>> + compatible = "renesas,sata-r8a7779"; >>> + reg = <0xfc600000 0x2000>; >>> + interrupt-parent = <&gic>; >>> + interrupts = <0 100 0x4>; >> >> Please use IRQ_TYPE_LEVEL_HIGH instead of 0x4. > > OK, I'll do that. > > Please, note that all the Renesas dts[i] files do not include <dt-bindings/interrupt-controller/irq.h> > Thus, blindly following this bindings documentation when adding new SATA nodes to the existing > r8a7790.dtsi, for example, will cause DT compilation error. > BTW, we'll also have to switch to #include instead of /include/ to make it work. Thanks, Val. -- 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 Valentine, On Friday 08 November 2013 15:52:45 Valentine wrote: > On 11/08/2013 03:26 PM, Valentine wrote: > > On 11/08/2013 05:02 AM, Laurent Pinchart wrote: > >> On Friday 08 November 2013 00:04:02 Valentine Barshak wrote: > >>> These bindings can be used to register SATA devices found on R-Car SoC. > >>> > >>> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com> > >>> Acked-by: Kumar Gala <galak@codeaurora.org> > >>> --- > >>> > >>> Documentation/devicetree/bindings/ata/sata_rcar.txt | 18 +++++++++++++ > >>> > >>> 1 file changed, 18 insertions(+) > >>> > >>> create mode 100644 Documentation/devicetree/bindings/ata/sata_rcar.txt > >>> > >>> diff --git a/Documentation/devicetree/bindings/ata/sata_rcar.txt > >>> b/Documentation/devicetree/bindings/ata/sata_rcar.txt new file mode > >>> 100644 > >>> index 0000000..d6b20a6 > >>> --- /dev/null > >>> +++ b/Documentation/devicetree/bindings/ata/sata_rcar.txt > >>> @@ -0,0 +1,18 @@ > >>> +* Renesas R-Car SATA > >>> + > >>> +Required properties: > >>> +- compatible : should contain one of the following: > >>> + - "renesas,sata-r8a7779" for R-Car H1 > >>> + - "renesas,sata-r8a7790" for R-Car H2 > >>> + - "renesas,sata-r8a7791" for R-Car M2 > >>> +- reg : offset and length of the SATA registers; > >> > >> Maybe address instead of offset ? > > > > OK. > > > >>> +- interrupts : must consist of one interrupt specifier. > >>> + > >>> +Example: > >>> + > >>> +sata: sata@fc600000 { > >>> + compatible = "renesas,sata-r8a7779"; > >>> + reg = <0xfc600000 0x2000>; > >>> + interrupt-parent = <&gic>; > >>> + interrupts = <0 100 0x4>; > >> > >> Please use IRQ_TYPE_LEVEL_HIGH instead of 0x4. > > > > OK, I'll do that. > > > > Please, note that all the Renesas dts[i] files do not include > > <dt-bindings/interrupt-controller/irq.h> Thus, blindly following this > > bindings documentation when adding new SATA nodes to the existing > > r8a7790.dtsi, for example, will cause DT compilation error. > > BTW, we'll also have to switch to #include instead of /include/ to make it > work. Sure, of course. I was actually thinking about sending a patch to convert our DT sources to use the IRQ_* macros. Morimoto-san, are you working on that already ? If not I'll do it. Simon, would you like one patch per SoC or would a single patch do ? Only .dts(i) files will be touched.
On Fri, Nov 08, 2013 at 04:31:03PM +0100, Laurent Pinchart wrote: > Hi Valentine, > > On Friday 08 November 2013 15:52:45 Valentine wrote: > > On 11/08/2013 03:26 PM, Valentine wrote: > > > On 11/08/2013 05:02 AM, Laurent Pinchart wrote: > > >> On Friday 08 November 2013 00:04:02 Valentine Barshak wrote: > > >>> These bindings can be used to register SATA devices found on R-Car SoC. > > >>> > > >>> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com> > > >>> Acked-by: Kumar Gala <galak@codeaurora.org> > > >>> --- > > >>> > > >>> Documentation/devicetree/bindings/ata/sata_rcar.txt | 18 +++++++++++++ > > >>> > > >>> 1 file changed, 18 insertions(+) > > >>> > > >>> create mode 100644 Documentation/devicetree/bindings/ata/sata_rcar.txt > > >>> > > >>> diff --git a/Documentation/devicetree/bindings/ata/sata_rcar.txt > > >>> b/Documentation/devicetree/bindings/ata/sata_rcar.txt new file mode > > >>> 100644 > > >>> index 0000000..d6b20a6 > > >>> --- /dev/null > > >>> +++ b/Documentation/devicetree/bindings/ata/sata_rcar.txt > > >>> @@ -0,0 +1,18 @@ > > >>> +* Renesas R-Car SATA > > >>> + > > >>> +Required properties: > > >>> +- compatible : should contain one of the following: > > >>> + - "renesas,sata-r8a7779" for R-Car H1 > > >>> + - "renesas,sata-r8a7790" for R-Car H2 > > >>> + - "renesas,sata-r8a7791" for R-Car M2 > > >>> +- reg : offset and length of the SATA registers; > > >> > > >> Maybe address instead of offset ? > > > > > > OK. > > > > > >>> +- interrupts : must consist of one interrupt specifier. > > >>> + > > >>> +Example: > > >>> + > > >>> +sata: sata@fc600000 { > > >>> + compatible = "renesas,sata-r8a7779"; > > >>> + reg = <0xfc600000 0x2000>; > > >>> + interrupt-parent = <&gic>; > > >>> + interrupts = <0 100 0x4>; > > >> > > >> Please use IRQ_TYPE_LEVEL_HIGH instead of 0x4. > > > > > > OK, I'll do that. > > > > > > Please, note that all the Renesas dts[i] files do not include > > > <dt-bindings/interrupt-controller/irq.h> Thus, blindly following this > > > bindings documentation when adding new SATA nodes to the existing > > > r8a7790.dtsi, for example, will cause DT compilation error. > > > > BTW, we'll also have to switch to #include instead of /include/ to make it > > work. > > Sure, of course. > > I was actually thinking about sending a patch to convert our DT sources to use > the IRQ_* macros. Morimoto-san, are you working on that already ? If not I'll > do it. > > Simon, would you like one patch per SoC or would a single patch do ? Only > .dts(i) files will be touched. I would prefer one patch per SoC. The reason is that in general that approach makes backporting easier. -- 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 Simon, On Tuesday 12 November 2013 12:42:58 Simon Horman wrote: > On Fri, Nov 08, 2013 at 04:31:03PM +0100, Laurent Pinchart wrote: > > On Friday 08 November 2013 15:52:45 Valentine wrote: > > > On 11/08/2013 03:26 PM, Valentine wrote: > > > > On 11/08/2013 05:02 AM, Laurent Pinchart wrote: > > > >> On Friday 08 November 2013 00:04:02 Valentine Barshak wrote: > > > >>> These bindings can be used to register SATA devices found on R-Car > > > >>> SoC. > > > >>> > > > >>> Signed-off-by: Valentine Barshak > > > >>> <valentine.barshak@cogentembedded.com> > > > >>> Acked-by: Kumar Gala <galak@codeaurora.org> > > > >>> --- > > > >>> > > > >>> Documentation/devicetree/bindings/ata/sata_rcar.txt | 18 +++++++++++ > > > >>> 1 file changed, 18 insertions(+) > > > >>> > > > >>> create mode 100644 > > > >>> Documentation/devicetree/bindings/ata/sata_rcar.txt > > > >>> > > > >>> diff --git a/Documentation/devicetree/bindings/ata/sata_rcar.txt > > > >>> b/Documentation/devicetree/bindings/ata/sata_rcar.txt new file mode > > > >>> 100644 > > > >>> index 0000000..d6b20a6 > > > >>> --- /dev/null > > > >>> +++ b/Documentation/devicetree/bindings/ata/sata_rcar.txt > > > >>> @@ -0,0 +1,18 @@ > > > >>> +* Renesas R-Car SATA > > > >>> + > > > >>> +Required properties: > > > >>> +- compatible : should contain one of the following: > > > >>> + - "renesas,sata-r8a7779" for R-Car H1 > > > >>> + - "renesas,sata-r8a7790" for R-Car H2 > > > >>> + - "renesas,sata-r8a7791" for R-Car M2 > > > >>> +- reg : offset and length of the SATA registers; > > > >> > > > >> Maybe address instead of offset ? > > > > > > > > OK. > > > > > > > >>> +- interrupts : must consist of one interrupt specifier. > > > >>> + > > > >>> +Example: > > > >>> + > > > >>> +sata: sata@fc600000 { > > > >>> + compatible = "renesas,sata-r8a7779"; > > > >>> + reg = <0xfc600000 0x2000>; > > > >>> + interrupt-parent = <&gic>; > > > >>> + interrupts = <0 100 0x4>; > > > >> > > > >> Please use IRQ_TYPE_LEVEL_HIGH instead of 0x4. > > > > > > > > OK, I'll do that. > > > > > > > > Please, note that all the Renesas dts[i] files do not include > > > > <dt-bindings/interrupt-controller/irq.h> Thus, blindly following this > > > > bindings documentation when adding new SATA nodes to the existing > > > > r8a7790.dtsi, for example, will cause DT compilation error. > > > > > > BTW, we'll also have to switch to #include instead of /include/ to make > > > it work. > > > > Sure, of course. > > > > I was actually thinking about sending a patch to convert our DT sources to > > use the IRQ_* macros. Morimoto-san, are you working on that already ? If > > not I'll do it. > > > > Simon, would you like one patch per SoC or would a single patch do ? Only > > .dts(i) files will be touched. > > I would prefer one patch per SoC. > The reason is that in general that approach makes backporting easier. I've already posted the series on Saturday, it's called "[PATCH 0/7] ARM: shmobile: Use interrupt macros in DT files". Could you please have a look ? I can split patches 1/7 to 3/7 in 28 patches and repost if you would like me to, but that sounds like a really high number of patches for something so simple, especially for patch 1/7.
diff --git a/Documentation/devicetree/bindings/ata/sata_rcar.txt b/Documentation/devicetree/bindings/ata/sata_rcar.txt new file mode 100644 index 0000000..d6b20a6 --- /dev/null +++ b/Documentation/devicetree/bindings/ata/sata_rcar.txt @@ -0,0 +1,18 @@ +* Renesas R-Car SATA + +Required properties: +- compatible : should contain one of the following: + - "renesas,sata-r8a7779" for R-Car H1 + - "renesas,sata-r8a7790" for R-Car H2 + - "renesas,sata-r8a7791" for R-Car M2 +- reg : offset and length of the SATA registers; +- interrupts : must consist of one interrupt specifier. + +Example: + +sata: sata@fc600000 { + compatible = "renesas,sata-r8a7779"; + reg = <0xfc600000 0x2000>; + interrupt-parent = <&gic>; + interrupts = <0 100 0x4>; +};