[PATCH 4/6] [RFC] Documentation: dt: Add Renesas RSPI/QSPI bindings

[PATCH 4/6] [RFC] Documentation: dt: Add Renesas RSPI/QSPI bindings

[Geert Uytterhoeven]

Signed-off-by: Geert Uytterhoeven <geert+renesas-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.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.
+- 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>;
+	};
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/6] [RFC] ARM: shmobile: r7s72100 dtsi: Add RSPI nodes

[Geert Uytterhoeven]

Signed-off-by: Geert Uytterhoeven <geert+renesas@linux-m68k.org>
Cc: devicetree@vger.kernel.org
---
 arch/arm/boot/dts/r7s72100.dtsi |   65 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi
index ff0bd6be454f..42e9b423476c 100644
--- a/arch/arm/boot/dts/r7s72100.dtsi
+++ b/arch/arm/boot/dts/r7s72100.dtsi
@@ -34,6 +34,11 @@
 		gpio10 = &port10;
 		gpio11 = &port11;
 		gpio12 = &jtagport0;
+		spi0 = &spi0;
+		spi1 = &spi1;
+		spi2 = &spi2;
+		spi3 = &spi3;
+		spi4 = &spi4;
 	};
 
 	cpus {
@@ -289,4 +294,64 @@
 		clock-frequency = <100000>;
 		status = "disabled";
 	};
+
+	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>;
+		status = "disabled";
+	};
+
+	spi1: spi@e800d000 {
+		compatible = "renesas,rspi-r7s72100", "renesas,rspi-rz";
+		reg = <0xe800d000 0x24>;
+		interrupts = <0 241 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 242 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 243 IRQ_TYPE_LEVEL_HIGH>;
+		num-cs = <1>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		status = "disabled";
+	};
+
+	spi2: spi@e800d800 {
+		compatible = "renesas,rspi-r7s72100", "renesas,rspi-rz";
+		reg = <0xe800d800 0x24>;
+		interrupts = <0 244 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 245 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 246 IRQ_TYPE_LEVEL_HIGH>;
+		num-cs = <1>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		status = "disabled";
+	};
+
+	spi3: spi@e800e000 {
+		compatible = "renesas,rspi-r7s72100", "renesas,rspi-rz";
+		reg = <0xe800e000 0x24>;
+		interrupts = <0 247 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 248 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 249 IRQ_TYPE_LEVEL_HIGH>;
+		num-cs = <1>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		status = "disabled";
+	};
+
+	spi4: spi@e800e800 {
+		compatible = "renesas,rspi-r7s72100", "renesas,rspi-rz";
+		reg = <0xe800e800 0x24>;
+		interrupts = <0 250 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 251 IRQ_TYPE_LEVEL_HIGH>,
+			     <0 252 IRQ_TYPE_LEVEL_HIGH>;
+		num-cs = <1>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		status = "disabled";
+	};
 };
-- 
1.7.9.5

[PATCH 6/6] [RFC] arm: shmobile: genmai reference: Add RSPI nodes

[Geert Uytterhoeven]

Add pinctrl and SPI devices for RSPI on Genmai.

On this board, only rspi4 is in use. It's bus contains a single device
(a wm8978 audio codec), for which no bindings are defined yet.

Signed-off-by: Geert Uytterhoeven <geert+renesas-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 arch/arm/boot/dts/r7s72100-genmai-reference.dts |   18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/r7s72100-genmai-reference.dts b/arch/arm/boot/dts/r7s72100-genmai-reference.dts
index 114510f8bf09..6d99630627e4 100644
--- a/arch/arm/boot/dts/r7s72100-genmai-reference.dts
+++ b/arch/arm/boot/dts/r7s72100-genmai-reference.dts
@@ -43,7 +43,7 @@
 };
 
 &pfc {
-	pinctrl-0 = <&scif2_pins &ethernet_pins>;
+	pinctrl-0 = <&scif2_pins &ethernet_pins &rspi4_pins>;
 	pinctrl-names = "default";
 
 	scif2_pins: serial2 {
@@ -73,6 +73,12 @@
 				 "ethernet_int_p1_15";
 		renesas,function = "ethernet";
 	};
+
+	rspi4_pins: spi4 {
+		renesas,groups = "rspi4_rspck_p4_0", "rspi4_ssl0_p4_1",
+				 "rspi4_mosi_p4_2", "rspi4_miso_p4_3";
+		renesas,function = "rspi4";
+	};
 };
 
 &i2c2 {
@@ -85,3 +91,13 @@
 		pagesize = <64>;
 	};
 };
+
+&spi4 {
+	status = "okay";
+
+	codec: wm8978@0 {
+		compatible = "wlf,wm8978";
+		reg = <0>;
+		spi-max-frequency = <5000000>;
+	};
+};
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] [RFC] arm: shmobile: genmai reference: Add RSPI nodes

[Sergei Shtylyov]

Hello.

On 24-12-2013 15:56, Geert Uytterhoeven wrote:

> Add pinctrl and SPI devices for RSPI on Genmai.

> On this board, only rspi4 is in use. It's bus contains a single device
> (a wm8978 audio codec), for which no bindings are defined yet.

> Signed-off-by: Geert Uytterhoeven <geert+renesas@linux-m68k.org>
> Cc: devicetree@vger.kernel.org
> ---
>   arch/arm/boot/dts/r7s72100-genmai-reference.dts |   18 +++++++++++++++++-
>   1 file changed, 17 insertions(+), 1 deletion(-)

> diff --git a/arch/arm/boot/dts/r7s72100-genmai-reference.dts b/arch/arm/boot/dts/r7s72100-genmai-reference.dts
> index 114510f8bf09..6d99630627e4 100644
> --- a/arch/arm/boot/dts/r7s72100-genmai-reference.dts
> +++ b/arch/arm/boot/dts/r7s72100-genmai-reference.dts
[...]
> @@ -85,3 +91,13 @@
>   		pagesize = <64>;
>   	};
>   };
> +
> +&spi4 {
> +	status = "okay";
> +
> +	codec: wm8978@0 {

    Do not give chip names to device nodes, please use somewhat generic names 
instead, like "sound-codec@0". That's to comply with ePAPR spec.

> +		compatible = "wlf,wm8978";
> +		reg = <0>;
> +		spi-max-frequency = <5000000>;
> +	};
> +};

WBR, Sergei

Re: [PATCH 6/6] [RFC] arm: shmobile: genmai reference: Add RSPI nodes

[Geert Uytterhoeven]

Hi Sergei,

On Tue, Dec 24, 2013 at 4:45 PM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
>> +       codec: wm8978@0 {
>
>    Do not give chip names to device nodes, please use somewhat generic names
> instead, like "sound-codec@0". That's to comply with ePAPR spec.

Oh, so the generic codec alias is not sufficient? Last time you gave this
comment, the code didn't contain a generic name at all, so I thought
this would be OK...

Seems like Documentation/devicetree/bindings can use some cleanups,
as it's full of examples like "codec: wm8510@1a"...

Thanks for your comments!

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

Re: [PATCH 4/6] [RFC] Documentation: dt: Add Renesas RSPI/QSPI bindings

[Laurent Pinchart]

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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.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>;
> +	};

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] [RFC] arm: shmobile: genmai reference: Add RSPI nodes

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Tuesday 24 December 2013 12:56:50 Geert Uytterhoeven wrote:
> Add pinctrl and SPI devices for RSPI on Genmai.
> 
> On this board, only rspi4 is in use. It's bus contains a single device
> (a wm8978 audio codec), for which no bindings are defined yet.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@linux-m68k.org>
> Cc: devicetree@vger.kernel.org
> ---
>  arch/arm/boot/dts/r7s72100-genmai-reference.dts |   18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/r7s72100-genmai-reference.dts
> b/arch/arm/boot/dts/r7s72100-genmai-reference.dts index
> 114510f8bf09..6d99630627e4 100644
> --- a/arch/arm/boot/dts/r7s72100-genmai-reference.dts
> +++ b/arch/arm/boot/dts/r7s72100-genmai-reference.dts
> @@ -43,7 +43,7 @@
>  };
> 
>  &pfc {
> -	pinctrl-0 = <&scif2_pins &ethernet_pins>;
> +	pinctrl-0 = <&scif2_pins &ethernet_pins &rspi4_pins>;

You should add pinctrl-0 and pinctrl-names properties to the spi4 node 
instead. Device nodes should reference their pinctrl configuration directly, 
scif2 and ethernet are exceptions as DT bindings for those devices are not in 
mainline yet.

>  	pinctrl-names = "default";
> 
>  	scif2_pins: serial2 {
> @@ -73,6 +73,12 @@
>  				 "ethernet_int_p1_15";
>  		renesas,function = "ethernet";
>  	};
> +
> +	rspi4_pins: spi4 {
> +		renesas,groups = "rspi4_rspck_p4_0", "rspi4_ssl0_p4_1",
> +				 "rspi4_mosi_p4_2", "rspi4_miso_p4_3";
> +		renesas,function = "rspi4";
> +	};

This node is at the right place and doesn't need to be moved.

>  };
> 
>  &i2c2 {
> @@ -85,3 +91,13 @@
>  		pagesize = <64>;
>  	};
>  };
> +
> +&spi4 {
> +	status = "okay";
> +
> +	codec: wm8978@0 {
> +		compatible = "wlf,wm8978";
> +		reg = <0>;
> +		spi-max-frequency = <5000000>;
> +	};
> +};
-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/6] [RFC] Documentation: dt: Add Renesas RSPI/QSPI bindings

[Geert Uytterhoeven]

Hi Laurent,

On Fri, Dec 27, 2013 at 5:18 PM, Laurent Pinchart
<laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> 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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] [RFC] arm: shmobile: genmai reference: Add RSPI nodes

[Geert Uytterhoeven]

On Fri, Dec 27, 2013 at 5:20 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Tuesday 24 December 2013 12:56:50 Geert Uytterhoeven wrote:
>> Add pinctrl and SPI devices for RSPI on Genmai.
>>
>> On this board, only rspi4 is in use. It's bus contains a single device
>> (a wm8978 audio codec), for which no bindings are defined yet.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@linux-m68k.org>
>> Cc: devicetree@vger.kernel.org
>> ---
>>  arch/arm/boot/dts/r7s72100-genmai-reference.dts |   18 +++++++++++++++++-
>>  1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/r7s72100-genmai-reference.dts
>> b/arch/arm/boot/dts/r7s72100-genmai-reference.dts index
>> 114510f8bf09..6d99630627e4 100644
>> --- a/arch/arm/boot/dts/r7s72100-genmai-reference.dts
>> +++ b/arch/arm/boot/dts/r7s72100-genmai-reference.dts
>> @@ -43,7 +43,7 @@
>>  };
>>
>>  &pfc {
>> -     pinctrl-0 = <&scif2_pins &ethernet_pins>;
>> +     pinctrl-0 = <&scif2_pins &ethernet_pins &rspi4_pins>;
>
> You should add pinctrl-0 and pinctrl-names properties to the spi4 node
> instead. Device nodes should reference their pinctrl configuration directly,
> scif2 and ethernet are exceptions as DT bindings for those devices are not in
> mainline yet.

I put it there because the actual driver doesn't use DT yet. Is that OK?

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

Re: [PATCH 6/6] [RFC] arm: shmobile: genmai reference: Add RSPI nodes

[Laurent Pinchart]

Hi Geert,

On Friday 27 December 2013 20:08:18 Geert Uytterhoeven wrote:
> On Fri, Dec 27, 2013 at 5:20 PM, Laurent Pinchart wrote:
> > On Tuesday 24 December 2013 12:56:50 Geert Uytterhoeven wrote:
> >> Add pinctrl and SPI devices for RSPI on Genmai.
> >> 
> >> On this board, only rspi4 is in use. It's bus contains a single device
> >> (a wm8978 audio codec), for which no bindings are defined yet.
> >> 
> >> Signed-off-by: Geert Uytterhoeven <geert+renesas-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
> >> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >> ---
> >> 
> >>  arch/arm/boot/dts/r7s72100-genmai-reference.dts |   18 ++++++++++++++++-
> >>  1 file changed, 17 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/arch/arm/boot/dts/r7s72100-genmai-reference.dts
> >> b/arch/arm/boot/dts/r7s72100-genmai-reference.dts index
> >> 114510f8bf09..6d99630627e4 100644
> >> --- a/arch/arm/boot/dts/r7s72100-genmai-reference.dts
> >> +++ b/arch/arm/boot/dts/r7s72100-genmai-reference.dts
> >> @@ -43,7 +43,7 @@
> >>  };
> >>  
> >>  &pfc {
> >> -     pinctrl-0 = <&scif2_pins &ethernet_pins>;
> >> +     pinctrl-0 = <&scif2_pins &ethernet_pins &rspi4_pins>;
> > 
> > You should add pinctrl-0 and pinctrl-names properties to the spi4 node
> > instead. Device nodes should reference their pinctrl configuration
> > directly, scif2 and ethernet are exceptions as DT bindings for those
> > devices are not in mainline yet.
> 
> I put it there because the actual driver doesn't use DT yet. Is that OK?

I had missed that. That's fine, but shouldn't you then delay patch 5/6 until 
the driver gets DT support ?

You've already sent a DT bindings proposal (thanks for that !), wouldn't it 
make sense to implement support in the driver before this patch ? We're pretty 
close to that from what I can see.

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] [RFC] Documentation: dt: Add Renesas RSPI/QSPI bindings

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] [RFC] Documentation: dt: Add Renesas RSPI/QSPI bindings

[Geert Uytterhoeven]

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

Re: [PATCH 4/6] [RFC] Documentation: dt: Add Renesas RSPI/QSPI bindings

[Laurent Pinchart]

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" ?

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] [RFC] Documentation: dt: Add Renesas RSPI/QSPI bindings

[Geert Uytterhoeven]

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