| Message ID | 1387886210-3634-5-git-send-email-geert+renesas@linux-m68k.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi Geert, Thank you for the patch. It's nice to see DT support for the RSPI driver. On Tuesday 24 December 2013 12:56:48 Geert Uytterhoeven wrote: > Signed-off-by: Geert Uytterhoeven <geert+renesas@linux-m68k.org> > Cc: devicetree@vger.kernel.org > --- > Documentation/devicetree/bindings/spi/spi-rspi.txt | 27 +++++++++++++++++ > 1 file changed, 27 insertions(+) > create mode 100644 Documentation/devicetree/bindings/spi/spi-rspi.txt > > diff --git a/Documentation/devicetree/bindings/spi/spi-rspi.txt > b/Documentation/devicetree/bindings/spi/spi-rspi.txt new file mode 100644 > index 000000000000..06ec17fce4c6 > --- /dev/null > +++ b/Documentation/devicetree/bindings/spi/spi-rspi.txt > @@ -0,0 +1,27 @@ > +Device tree configuration for Renesas RSPI/QSPI driver > + > +Required properties: > +- compatible : "renesas,rspi-<soctype>". "renesas,rspi-rz" as > fallback, > + or > + "renesas,qspi-<soctype>", "renesas,qspi-rcar" as > fallback. I think you need to be a bit more verbose here and explain when to use rspi and when to use qspi. I'm also wondering where we need the -rz and -rcar suffixes for the generic compatible strings. > +- reg : address start and address range size of device > +- interrupts : 3 interrupts for RSPI (SPEI, SPRI, SPTI), > + 1 interrupt for QSPI > +- num-cs : Number of chip selects > +- #address-cells : should be <1> > +- #size-cells : should be <0> I would say "must" instead of "should". > + > +Pinctrl properties might be needed, too. See there. > + > +Example: > + > + spi0: spi@e800c800 { > + compatible = "renesas,rspi-r7s72100", "renesas,rspi-rz"; > + reg = <0xe800c800 0x24>; > + interrupts = <0 238 IRQ_TYPE_LEVEL_HIGH>, > + <0 239 IRQ_TYPE_LEVEL_HIGH>, > + <0 240 IRQ_TYPE_LEVEL_HIGH>; You're missing the interrupt-parent property. Now that we have two ways to specify interrupts (interrupt-parent + interrupts or interrupts-extended) we should probably decide on a common wording for interrupt properties in individual DT bindings, but that's out of scope of this patch. > + num-cs = <1>; > + #address-cells = <1>; > + #size-cells = <0>; > + };
Hi Laurent, On Fri, Dec 27, 2013 at 5:18 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Tuesday 24 December 2013 12:56:48 Geert Uytterhoeven wrote: >> +++ b/Documentation/devicetree/bindings/spi/spi-rspi.txt >> @@ -0,0 +1,27 @@ >> +Device tree configuration for Renesas RSPI/QSPI driver >> + >> +Required properties: >> +- compatible : "renesas,rspi-<soctype>". "renesas,rspi-rz" as >> fallback, >> + or >> + "renesas,qspi-<soctype>", "renesas,qspi-rcar" as >> fallback. > > I think you need to be a bit more verbose here and explain when to use rspi > and when to use qspi. I'm also wondering where we need the -rz and -rcar OK. > suffixes for the generic compatible strings. The rationale behind this is that RSPI DT would apply to RSPI on RZ/A1H. The spi-rspi driver also supports legacy SH7757, which may not move to DT anytime soon. For symmetry, I did the same thing for QSPI, which applies to QSPI on R-Car H2 and M2 (upon second look, it should be "renesas,qspi-rcar-gen2", as E1/M1/H1 have HSPI). Does this makes sense? Or do you still prefer plain "renesas,rspi" and "renesas,qspi", and perhaps "renesas,rspi-sh" if we ever get DT there? >> +- reg : address start and address range size of device >> +- interrupts : 3 interrupts for RSPI (SPEI, SPRI, SPTI), >> + 1 interrupt for QSPI >> +- num-cs : Number of chip selects >> +- #address-cells : should be <1> >> +- #size-cells : should be <0> > > I would say "must" instead of "should". OK. >> +Example: >> + >> + spi0: spi@e800c800 { >> + compatible = "renesas,rspi-r7s72100", "renesas,rspi-rz"; >> + reg = <0xe800c800 0x24>; >> + interrupts = <0 238 IRQ_TYPE_LEVEL_HIGH>, >> + <0 239 IRQ_TYPE_LEVEL_HIGH>, >> + <0 240 IRQ_TYPE_LEVEL_HIGH>; > > You're missing the interrupt-parent property. OK. But we don't need it in the spi node in the actual .dts, as we already have "interrupt-parent = <&gic>;" in the enclosing node, right? Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On Friday 27 December 2013 20:01:53 Geert Uytterhoeven wrote: > On Fri, Dec 27, 2013 at 5:18 PM, Laurent Pinchart wrote: > > On Tuesday 24 December 2013 12:56:48 Geert Uytterhoeven wrote: > >> +++ b/Documentation/devicetree/bindings/spi/spi-rspi.txt > >> @@ -0,0 +1,27 @@ > >> +Device tree configuration for Renesas RSPI/QSPI driver > >> + > >> +Required properties: > >> +- compatible : "renesas,rspi-<soctype>". "renesas,rspi-rz" as > >> fallback, > >> + or > >> + "renesas,qspi-<soctype>", "renesas,qspi-rcar" as > >> fallback. > > > > I think you need to be a bit more verbose here and explain when to use > > rspi and when to use qspi. I'm also wondering where we need the -rz and - > > rcar > > OK. > > > suffixes for the generic compatible strings. > > The rationale behind this is that RSPI DT would apply to RSPI on RZ/A1H. > The spi-rspi driver also supports legacy SH7757, which may not move to > DT anytime soon. > For symmetry, I did the same thing for QSPI, which applies to QSPI > on R-Car H2 and M2 (upon second look, it should be "renesas,qspi-rcar-gen2", > as E1/M1/H1 have HSPI). > > Does this makes sense? Or do you still prefer plain "renesas,rspi" and > "renesas,qspi", and perhaps "renesas,rspi-sh" if we ever get DT there? The compatible strings should define what the device is compatible with. They should start with the most specific compability and end with the less specific one. In this case we definitely want to list the SoC-specific compatible string first, even if we don't need it, as experience with Renesas SoC shows that subtle differences between different versions of the same IP core can be discovered later. If all RSPI cores are similar enough to be supported by a single driver configuration, then I believe "renesas,rspi" would be a proper second compatible string. If the RSPI cores in the SH and RZ chips are incompatible then we need two different strings. The same is true for QSPI. A quick look at the driver shows that RSPI is supported without caring whether the SoC is an SH or ARM, so I believe "renesas,rspi" should do. The question in my opinion is whether we want to use compatible = "renesas,rspi-r7s72100", "renesas,rspi"; or compatible = "renesas,rspi-r7s72100", "renesas,rspi-rz", "renesas,rspi"; > >> +- reg : address start and address range size of device > >> +- interrupts : 3 interrupts for RSPI (SPEI, SPRI, SPTI), > >> + 1 interrupt for QSPI > >> +- num-cs : Number of chip selects > >> +- #address-cells : should be <1> > >> +- #size-cells : should be <0> > > > > I would say "must" instead of "should". > > OK. > > >> +Example: > >> + > >> + spi0: spi@e800c800 { > >> + compatible = "renesas,rspi-r7s72100", "renesas,rspi-rz"; > >> + reg = <0xe800c800 0x24>; > >> + interrupts = <0 238 IRQ_TYPE_LEVEL_HIGH>, > >> + <0 239 IRQ_TYPE_LEVEL_HIGH>, > >> + <0 240 IRQ_TYPE_LEVEL_HIGH>; > > > > You're missing the interrupt-parent property. > > OK. > > But we don't need it in the spi node in the actual .dts, as we already have > "interrupt-parent = <&gic>;" in the enclosing node, right? That's right.
Hi Laurent, Thanks for your comments! On Mon, Dec 30, 2013 at 12:22 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Friday 27 December 2013 20:01:53 Geert Uytterhoeven wrote: >> On Fri, Dec 27, 2013 at 5:18 PM, Laurent Pinchart wrote: >> > On Tuesday 24 December 2013 12:56:48 Geert Uytterhoeven wrote: >> >> +++ b/Documentation/devicetree/bindings/spi/spi-rspi.txt >> >> @@ -0,0 +1,27 @@ >> >> +Device tree configuration for Renesas RSPI/QSPI driver >> >> + >> >> +Required properties: >> >> +- compatible : "renesas,rspi-<soctype>". "renesas,rspi-rz" as >> >> fallback, >> >> + or >> >> + "renesas,qspi-<soctype>", "renesas,qspi-rcar" as >> >> fallback. >> > >> > I think you need to be a bit more verbose here and explain when to use >> > rspi and when to use qspi. I'm also wondering where we need the -rz and - >> > rcar >> >> OK. >> >> > suffixes for the generic compatible strings. >> >> The rationale behind this is that RSPI DT would apply to RSPI on RZ/A1H. >> The spi-rspi driver also supports legacy SH7757, which may not move to >> DT anytime soon. >> For symmetry, I did the same thing for QSPI, which applies to QSPI >> on R-Car H2 and M2 (upon second look, it should be "renesas,qspi-rcar-gen2", >> as E1/M1/H1 have HSPI). >> >> Does this makes sense? Or do you still prefer plain "renesas,rspi" and >> "renesas,qspi", and perhaps "renesas,rspi-sh" if we ever get DT there? > > The compatible strings should define what the device is compatible with. They > should start with the most specific compability and end with the less specific > one. In this case we definitely want to list the SoC-specific compatible > string first, even if we don't need it, as experience with Renesas SoC shows > that subtle differences between different versions of the same IP core can be > discovered later. If all RSPI cores are similar enough to be supported by a > single driver configuration, then I believe "renesas,rspi" would be a proper > second compatible string. If the RSPI cores in the SH and RZ chips are > incompatible then we need two different strings. The same is true for QSPI. > > A quick look at the driver shows that RSPI is supported without caring whether > the SoC is an SH or ARM, so I believe "renesas,rspi" should do. The question There are subtle register differences between RSPI on SH7757 and RZ/A1H, cfr. what rspi_parse_platform_data() does when no platform_data is passed[*]. RSPI on RZ/A1H seems to be an evolutionary step from RSPI on SH towards QSPI on RCar Gen2 (but without Dual/Quad). [*] setup-sh7757.c doesn't pass platform data, although the rspi driver and its platform data has support for DMA on SH7757, which is not used by setup-sh7757.c nor any other in-tree platform. > in my opinion is whether we want to use > > compatible = "renesas,rspi-r7s72100", "renesas,rspi"; > > or > > compatible = "renesas,rspi-r7s72100", "renesas,rspi-rz", "renesas,rspi"; So we end up with: compatible = "renesas,sh7757", "renesas,rspi-sh" (SH legacy) compatible = "renesas,rspi-r7s72100", "renesas,rspi-rz" (Genmai) compatible = "renesas,rspi-r8a7790", "renesas,qspi" (Lager) compatible = "renesas,rspi-r8a7791", "renesas,qspi" (Koelsch) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On Monday 30 December 2013 10:37:24 Geert Uytterhoeven wrote: > On Mon, Dec 30, 2013 at 12:22 AM, Laurent Pinchart wrote: > > On Friday 27 December 2013 20:01:53 Geert Uytterhoeven wrote: > >> On Fri, Dec 27, 2013 at 5:18 PM, Laurent Pinchart wrote: > >> > On Tuesday 24 December 2013 12:56:48 Geert Uytterhoeven wrote: > >> >> +++ b/Documentation/devicetree/bindings/spi/spi-rspi.txt > >> >> @@ -0,0 +1,27 @@ > >> >> +Device tree configuration for Renesas RSPI/QSPI driver > >> >> + > >> >> +Required properties: > >> >> +- compatible : "renesas,rspi-<soctype>". "renesas,rspi-rz" as > >> >> fallback, > >> >> + or > >> >> + "renesas,qspi-<soctype>", "renesas,qspi-rcar" as > >> >> fallback. > >> > > >> > I think you need to be a bit more verbose here and explain when to use > >> > rspi and when to use qspi. I'm also wondering where we need the -rz and > >> > - > >> > rcar > >> > >> OK. > >> > >> > suffixes for the generic compatible strings. > >> > >> The rationale behind this is that RSPI DT would apply to RSPI on RZ/A1H. > >> The spi-rspi driver also supports legacy SH7757, which may not move to > >> DT anytime soon. > >> For symmetry, I did the same thing for QSPI, which applies to QSPI > >> on R-Car H2 and M2 (upon second look, it should be > >> "renesas,qspi-rcar-gen2", as E1/M1/H1 have HSPI). > >> > >> Does this makes sense? Or do you still prefer plain "renesas,rspi" and > >> "renesas,qspi", and perhaps "renesas,rspi-sh" if we ever get DT there? > > > > The compatible strings should define what the device is compatible with. > > They should start with the most specific compability and end with the > > less specific one. In this case we definitely want to list the > > SoC-specific compatible string first, even if we don't need it, as > > experience with Renesas SoC shows that subtle differences between > > different versions of the same IP core can be discovered later. If all > > RSPI cores are similar enough to be supported by a single driver > > configuration, then I believe "renesas,rspi" would be a proper second > > compatible string. If the RSPI cores in the SH and RZ chips are > > incompatible then we need two different strings. The same is true for > > QSPI. > > > > A quick look at the driver shows that RSPI is supported without caring > > whether the SoC is an SH or ARM, so I believe "renesas,rspi" should do. > > The question > > There are subtle register differences between RSPI on SH7757 and RZ/A1H, > cfr. what rspi_parse_platform_data() does when no platform_data is > passed[*]. RSPI on RZ/A1H seems to be an evolutionary step from RSPI on SH > towards QSPI on RCar Gen2 (but without Dual/Quad). > > [*] setup-sh7757.c doesn't pass platform data, although the rspi driver and > its platform data has support for DMA on SH7757, which is not used by > setup-sh7757.c nor any other in-tree platform. The only important difference I can see (beside the hardcoded 16-bit width which could be selected as the default) is that the SSH7757 RSPI has no SPL bits in SPDCR. I don't have access tot he SH7757 datasheet, could you please confirm that ? > > in my opinion is whether we want to use > > > > compatible = "renesas,rspi-r7s72100", "renesas,rspi"; > > > > or > > > > compatible = "renesas,rspi-r7s72100", "renesas,rspi-rz", "renesas,rspi"; > > So we end up with: > > compatible = "renesas,sh7757", "renesas,rspi-sh" (SH legacy) > compatible = "renesas,rspi-r7s72100", "renesas,rspi-rz" (Genmai) > compatible = "renesas,rspi-r8a7790", "renesas,qspi" (Lager) > compatible = "renesas,rspi-r8a7791", "renesas,qspi" (Koelsch) Don't you mean "renesas,qspi-r8a7790" and "renesas,qspi-r8a7791" ?
On Mon, Dec 30, 2013 at 4:18 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: >> There are subtle register differences between RSPI on SH7757 and RZ/A1H, >> cfr. what rspi_parse_platform_data() does when no platform_data is >> passed[*]. RSPI on RZ/A1H seems to be an evolutionary step from RSPI on SH >> towards QSPI on RCar Gen2 (but without Dual/Quad). >> >> [*] setup-sh7757.c doesn't pass platform data, although the rspi driver and >> its platform data has support for DMA on SH7757, which is not used by >> setup-sh7757.c nor any other in-tree platform. > > The only important difference I can see (beside the hardcoded 16-bit width > which could be selected as the default) is that the SSH7757 RSPI has no SPL > bits in SPDCR. I don't have access tot he SH7757 datasheet, could you please > confirm that ? SH7757 has RSPI Control Register 2 and the TX only mode flag, while RSPI-RZ (and QSPI) haven't. The number of interrupts also differ, but that's easily handled via the interrupts property. BTW, I also don't have the SH7757 datasheet (will check), so my knowledge there is purely based on the behavior of the code. >> > in my opinion is whether we want to use >> > >> > compatible = "renesas,rspi-r7s72100", "renesas,rspi"; >> > >> > or >> > >> > compatible = "renesas,rspi-r7s72100", "renesas,rspi-rz", "renesas,rspi"; >> >> So we end up with: >> >> compatible = "renesas,sh7757", "renesas,rspi-sh" (SH legacy) >> compatible = "renesas,rspi-r7s72100", "renesas,rspi-rz" (Genmai) >> compatible = "renesas,rspi-r8a7790", "renesas,qspi" (Lager) >> compatible = "renesas,rspi-r8a7791", "renesas,qspi" (Koelsch) > > Don't you mean "renesas,qspi-r8a7790" and "renesas,qspi-r8a7791" ? Oops, yes, sorry about that. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-spi" 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/spi/spi-rspi.txt b/Documentation/devicetree/bindings/spi/spi-rspi.txt new file mode 100644 index 000000000000..06ec17fce4c6 --- /dev/null +++ b/Documentation/devicetree/bindings/spi/spi-rspi.txt @@ -0,0 +1,27 @@ +Device tree configuration for Renesas RSPI/QSPI driver + +Required properties: +- compatible : "renesas,rspi-<soctype>". "renesas,rspi-rz" as fallback, + or + "renesas,qspi-<soctype>", "renesas,qspi-rcar" as fallback. +- reg : address start and address range size of device +- interrupts : 3 interrupts for RSPI (SPEI, SPRI, SPTI), + 1 interrupt for QSPI +- num-cs : Number of chip selects +- #address-cells : should be <1> +- #size-cells : should be <0> + +Pinctrl properties might be needed, too. See there. + +Example: + + spi0: spi@e800c800 { + compatible = "renesas,rspi-r7s72100", "renesas,rspi-rz"; + reg = <0xe800c800 0x24>; + interrupts = <0 238 IRQ_TYPE_LEVEL_HIGH>, + <0 239 IRQ_TYPE_LEVEL_HIGH>, + <0 240 IRQ_TYPE_LEVEL_HIGH>; + num-cs = <1>; + #address-cells = <1>; + #size-cells = <0>; + };
Signed-off-by: Geert Uytterhoeven <geert+renesas@linux-m68k.org> Cc: devicetree@vger.kernel.org --- Documentation/devicetree/bindings/spi/spi-rspi.txt | 27 ++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 Documentation/devicetree/bindings/spi/spi-rspi.txt