[PATCH v2 0/3] r7s72100: dts: enable mmcif

[PATCH v2 0/3] r7s72100: dts: enable mmcif

[Chris Brandt]

This series enables the mmc driver for the RZ/A1.
Nothing needed to be changed with the actual sh_mmcif driver.
It worked fine as-is.

As you can see, the status in the rskrza1 dst was left
as disabled because I wanted to leave that code in there
for refernce for someone later that actually has a board with
an actaul eMMC chip. I tested with an MMC Plus card.

For version 2:
* First first patch that adds the clock has already been accepted
* Add in the 3rd "card detect" interrupt that no one uses, but it
  is documented in the hardware manual so it is reflected in the
  DT.

Chris Brandt (3):
  ARM: dts: r7s72100: add mmcif to device tree
  mmc: sh_mmcif: Document r7s72100 DT bindings
  ARM: dts: rskrza1: add mmc DT support

 Documentation/devicetree/bindings/mmc/renesas,mmcif.txt |  4 ++++
 arch/arm/boot/dts/r7s72100-rskrza1.dts                  | 17 +++++++++++++++++
 arch/arm/boot/dts/r7s72100.dtsi                         | 12 ++++++++++++
 3 files changed, 33 insertions(+)

-- 
2.9.2

[PATCH v2 1/3] ARM: dts: r7s72100: add mmcif to device tree

[Chris Brandt]

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
v2:
* added card detect as 3rd interupt source
---
 arch/arm/boot/dts/r7s72100.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi
index e18d4e6..50f9f3b 100644
--- a/arch/arm/boot/dts/r7s72100.dtsi
+++ b/arch/arm/boot/dts/r7s72100.dtsi
@@ -450,4 +450,16 @@
 		#size-cells = <0>;
 		status = "disabled";
 	};
+
+	mmcif: mmc@e804c800 {
+		compatible = "renesas,mmcif-r7s72100", "renesas,sh-mmcif";
+		reg = <0xe804c800 0x80>;
+		interrupts = <GIC_SPI 268 IRQ_TYPE_LEVEL_HIGH
+			      GIC_SPI 269 IRQ_TYPE_LEVEL_HIGH
+			      GIC_SPI 267 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&mstp8_clks R7S72100_CLK_MMCIF>;
+		reg-io-width = <4>;
+		bus-width = <8>;
+		status = "disabled";
+	};
 };
-- 
2.9.2

[PATCH v2 2/3] mmc: sh_mmcif: Document r7s72100 DT bindings

[Chris Brandt]

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
v2:
* added interrupt description
---
 Documentation/devicetree/bindings/mmc/renesas,mmcif.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt b/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt
index ff611fa..1443edb 100644
--- a/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt
+++ b/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt
@@ -8,12 +8,16 @@ Required properties:
 
 - compatible: should be "renesas,mmcif-<soctype>", "renesas,sh-mmcif" as a
   fallback. Examples with <soctype> are:
+	- "renesas,mmcif-r7s72100" for the MMCIF found in r7s72100 SoCs
 	- "renesas,mmcif-r8a7740" for the MMCIF found in r8a7740 SoCs
 	- "renesas,mmcif-r8a7790" for the MMCIF found in r8a7790 SoCs
 	- "renesas,mmcif-r8a7791" for the MMCIF found in r8a7791 SoCs
 	- "renesas,mmcif-r8a7793" for the MMCIF found in r8a7793 SoCs
 	- "renesas,mmcif-r8a7794" for the MMCIF found in r8a7794 SoCs
 
+- interrupts: either 1 shared interrupt, 2 separate interrupts (error and int),
+  or 3 separate interrupts (error, int, card detect)
+
 - clocks: reference to the functional clock
 
 - dmas: reference to the DMA channels, one per channel name listed in the
-- 
2.9.2

Re: [PATCH v2 2/3] mmc: sh_mmcif: Document r7s72100 DT bindings

[Rob Herring]

On Tue, Sep 20, 2016 at 11:46:19AM -0400, Chris Brandt wrote:
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
> v2:
> * added interrupt description
> ---
>  Documentation/devicetree/bindings/mmc/renesas,mmcif.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt b/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt
> index ff611fa..1443edb 100644
> --- a/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt
> +++ b/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt
> @@ -8,12 +8,16 @@ Required properties:
>  
>  - compatible: should be "renesas,mmcif-<soctype>", "renesas,sh-mmcif" as a
>    fallback. Examples with <soctype> are:
> +	- "renesas,mmcif-r7s72100" for the MMCIF found in r7s72100 SoCs
>  	- "renesas,mmcif-r8a7740" for the MMCIF found in r8a7740 SoCs
>  	- "renesas,mmcif-r8a7790" for the MMCIF found in r8a7790 SoCs
>  	- "renesas,mmcif-r8a7791" for the MMCIF found in r8a7791 SoCs
>  	- "renesas,mmcif-r8a7793" for the MMCIF found in r8a7793 SoCs
>  	- "renesas,mmcif-r8a7794" for the MMCIF found in r8a7794 SoCs
>  
> +- interrupts: either 1 shared interrupt, 2 separate interrupts (error and int),
> +  or 3 separate interrupts (error, int, card detect)

This should be based on the compatible strings. Surely, 1, 2, or 3 is 
not valid for all compatibles.

Rob

RE: [PATCH v2 2/3] mmc: sh_mmcif: Document r7s72100 DT bindings

[Chris Brandt]

On 9/23/2016, Rob Herring wrote:
> > diff --git a/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt
> > b/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt
> > index ff611fa..1443edb 100644
> > --- a/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt
> > +++ b/Documentation/devicetree/bindings/mmc/renesas,mmcif.txt
> > @@ -8,12 +8,16 @@ Required properties:
> >
> >  - compatible: should be "renesas,mmcif-<soctype>", "renesas,sh-mmcif"
> as a
> >    fallback. Examples with <soctype> are:
> > +	- "renesas,mmcif-r7s72100" for the MMCIF found in r7s72100 SoCs
> >  	- "renesas,mmcif-r8a7740" for the MMCIF found in r8a7740 SoCs
> >  	- "renesas,mmcif-r8a7790" for the MMCIF found in r8a7790 SoCs
> >  	- "renesas,mmcif-r8a7791" for the MMCIF found in r8a7791 SoCs
> >  	- "renesas,mmcif-r8a7793" for the MMCIF found in r8a7793 SoCs
> >  	- "renesas,mmcif-r8a7794" for the MMCIF found in r8a7794 SoCs
> >
> > +- interrupts: either 1 shared interrupt, 2 separate interrupts (error
> > +and int),
> > +  or 3 separate interrupts (error, int, card detect)
> 
> This should be based on the compatible strings. Surely, 1, 2, or 3 is not
> valid for all compatibles.


That is true. What is your suggestion? Add the specific number of interrupts for each SoC to the documentation so there is no confusion?

For example:

- interrupts: Some SoCs have only 1 shared interrupt, while others have either
  2 or 3 individual interrupts (error, int, card detect). Below is the number
  of interrupts for each SoC.
    3: r7s72100 SoC
    2: r8a7740 SoC
    1: r8a7790 SoC
    1: r8a7791 SoC
    1: r8a7793 SoC
    1: r8a7794 SoC


Chris

[PATCH v2 3/3] ARM: dts: rskrza1: add mmc DT support

[Chris Brandt]

Since the MMC and SDHI1 on the RSK share the same socket connector (CN1),
you cannot enable MMC and SDHI1 at the same time. Therefore the status
has been set to disabled because SDHI is more popular with this board.
However, keeping this code in here serves as a good way to document how
the MMC on the RZ/A1 has been known to work for someone that does want
to use MMC instead of SDHI1.

A fixed 3.3 regulator is included because it is required by the mmc
driver.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 arch/arm/boot/dts/r7s72100-rskrza1.dts | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm/boot/dts/r7s72100-rskrza1.dts b/arch/arm/boot/dts/r7s72100-rskrza1.dts
index e5dea5b..9228b04 100644
--- a/arch/arm/boot/dts/r7s72100-rskrza1.dts
+++ b/arch/arm/boot/dts/r7s72100-rskrza1.dts
@@ -33,6 +33,15 @@
 		#address-cells = <1>;
 		#size-cells = <1>;
 	};
+
+	d3_3v: regulator-d3-3v {
+		compatible = "regulator-fixed";
+		regulator-name = "D3.3V";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		regulator-boot-on;
+		regulator-always-on;
+	};
 };
 
 &extal_clk {
@@ -56,6 +65,14 @@
 	};
 };
 
+&mmcif {
+	vmmc-supply = <&d3_3v>;
+	vqmmc-supply = <&d3_3v>;
+	bus-width = <8>;
+	non-removable;
+	status = "disabled"; /* shares CN1 with sdhi1 */
+};
+
 &scif2 {
 	status = "okay";
 };
-- 
2.9.2

Re: [PATCH v2 3/3] ARM: dts: rskrza1: add mmc DT support

[Geert Uytterhoeven]

Hi Chris,

On Tue, Sep 20, 2016 at 5:46 PM, Chris Brandt <chris.brandt@renesas.com> wrote:
> Since the MMC and SDHI1 on the RSK share the same socket connector (CN1),
> you cannot enable MMC and SDHI1 at the same time. Therefore the status
> has been set to disabled because SDHI is more popular with this board.
> However, keeping this code in here serves as a good way to document how
> the MMC on the RZ/A1 has been known to work for someone that does want
> to use MMC instead of SDHI1.
>
> A fixed 3.3 regulator is included because it is required by the mmc
> driver.

I have mixed feelings about adding these "disabled" nodes, purely for
documentation.

What do other people think?

> --- a/arch/arm/boot/dts/r7s72100-rskrza1.dts
> +++ b/arch/arm/boot/dts/r7s72100-rskrza1.dts
> @@ -33,6 +33,15 @@
>                 #address-cells = <1>;
>                 #size-cells = <1>;
>         };
> +
> +       d3_3v: regulator-d3-3v {
> +               compatible = "regulator-fixed";
> +               regulator-name = "D3.3V";
> +               regulator-min-microvolt = <3300000>;
> +               regulator-max-microvolt = <3300000>;
> +               regulator-boot-on;
> +               regulator-always-on;

'status = "disabled";', too?

> +       };

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 v2 3/3] ARM: dts: rskrza1: add mmc DT support

[Chris Brandt]

Hi Geert,

> > Since the MMC and SDHI1 on the RSK share the same socket connector
> > (CN1), you cannot enable MMC and SDHI1 at the same time. Therefore the
> > status has been set to disabled because SDHI is more popular with this
> board.
> > However, keeping this code in here serves as a good way to document
> > how the MMC on the RZ/A1 has been known to work for someone that does
> > want to use MMC instead of SDHI1.
> >
> > A fixed 3.3 regulator is included because it is required by the mmc
> > driver.
> 
> I have mixed feelings about adding these "disabled" nodes, purely for
> documentation.
> 
> What do other people think?

From what I found, users just want to 'copy/paste' working code and delete then parts they don't need. Deleting code is easier than adding code.

However...
If this kind of 'dead sample code' in the DT is considered ugly for upstream, I can take it out and only include it in the Renesas BSP packages.
My main focus is for the drivers and the .dtsi files to be upstream. I think of the board dts files as more like 'sample configurations' anyway, so providing them separately is not an issue.



> > --- a/arch/arm/boot/dts/r7s72100-rskrza1.dts
> > +++ b/arch/arm/boot/dts/r7s72100-rskrza1.dts
> > @@ -33,6 +33,15 @@
> >                 #address-cells = <1>;
> >                 #size-cells = <1>;
> >         };
> > +
> > +       d3_3v: regulator-d3-3v {
> > +               compatible = "regulator-fixed";
> > +               regulator-name = "D3.3V";
> > +               regulator-min-microvolt = <3300000>;
> > +               regulator-max-microvolt = <3300000>;
> > +               regulator-boot-on;
> > +               regulator-always-on;
> 
> 'status = "disabled";', too?

Good point. That would be "disabled" too.
But, as I mentioned above, if you decide to have me remove the disabled mmcif node, I'll remove this node too.


Chris