[PATCH 00/02] ARM: shmobile: Koelsch SDHI DT support

[PATCH 00/02] ARM: shmobile: Koelsch SDHI DT support

[Magnus Damm]

ARM: shmobile: Koelsch SDHI DT support

[PATCH v2 01/02] ARM: shmobile: Add SDHI devices to r8a7791 DTSI
[PATCH 02/02] ARM: shmobile: Add SDHI devices for Koelsch DTS

Add SDHI devices to the Koelsch DT board support. The same run
time dependencies as SDHI for Lager applies here too, and there
is a PFC fix for SD1_CLK that is needed for r8a7791 as well.

Can be merged independently without waiting for v3.15 patches.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 Written against renesas-devel-v3.14-rc1-20140207

 Requires the following patch for correct run time operation:
 [PATCH 02/03] pinctrl: sh-pfc: r8a7791: SD1_CLK fix

 arch/arm/boot/dts/r8a7791-koelsch.dts |  131 +++++++++++++++++++++++++++++++++
 arch/arm/boot/dts/r8a7791.dtsi        |   27 ++++++
 2 files changed, 158 insertions(+)

[PATCH v2 01/02] ARM: shmobile: Add SDHI devices to r8a7791 DTSI

[Magnus Damm]

From: Magnus Damm <damm@opensource.se>

Add SDHI0, SDHI1 and SDHI2 to the r8a7791 DTSI.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

Changes since V1:
 - add r8a7790 suffix as fall back in case r8a7791 is missing from the driver

 arch/arm/boot/dts/r8a7791.dtsi |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

--- 0001/arch/arm/boot/dts/r8a7791.dtsi
+++ work/arch/arm/boot/dts/r8a7791.dtsi	2014-02-12 16:29:40.000000000 +0900
@@ -186,6 +186,33 @@
 		#gpio-range-cells = <3>;
 	};
 
+	sdhi0: sd@ee100000 {
+		compatible = "renesas,sdhi-r8a7791", "renesas,sdhi-r8a7790";
+		reg = <0 0xee100000 0 0x200>;
+		interrupt-parent = <&gic>;
+		interrupts = <0 165 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&mstp3_clks R8A7791_CLK_SDHI0>;
+		status = "disabled";
+	};
+
+	sdhi1: sd@ee140000 {
+		compatible = "renesas,sdhi-r8a7791", "renesas,sdhi-r8a7790";
+		reg = <0 0xee140000 0 0x100>;
+		interrupt-parent = <&gic>;
+		interrupts = <0 167 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&mstp3_clks R8A7791_CLK_SDHI1>;
+		status = "disabled";
+	};
+
+	sdhi2: sd@ee160000 {
+		compatible = "renesas,sdhi-r8a7791", "renesas,sdhi-r8a7790";
+		reg = <0 0xee160000 0 0x100>;
+		interrupt-parent = <&gic>;
+		interrupts = <0 168 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&mstp3_clks R8A7791_CLK_SDHI2>;
+		status = "disabled";
+	};
+
 	scifa0: serial@e6c40000 {
 		compatible = "renesas,scifa-r8a7791", "renesas,scifa";
 		reg = <0 0xe6c40000 0 64>;

Re: [PATCH v2 01/02] ARM: shmobile: Add SDHI devices to r8a7791 DTSI

[Geert Uytterhoeven]

Hi Magnus,

On Wed, Feb 12, 2014 at 8:41 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
> Changes since V1:
>  - add r8a7790 suffix as fall back in case r8a7791 is missing from the driver

I'm afraid that's not such a good idea: what if r8a7791 SDHI turns out to have a
slight incompatibility with r8a7790 SDHI later?

What about adding "renesas,sdhi-rcar-gen2" as a fallback instead, for
both r8a7790
and r8a7791?
This would reduce the size of the sh_mobile_sdhi_of_match[] table in
drivers/mmc/host/sh_mobile_sdhi.c, as no SoC-specific entries are needed at
the moment.

A similar thing can be done for "renesas,sdhi-rcar-gen1" (as long as those DTSes
are not in mainline).

>  arch/arm/boot/dts/r8a7791.dtsi |   27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> --- 0001/arch/arm/boot/dts/r8a7791.dtsi
> +++ work/arch/arm/boot/dts/r8a7791.dtsi 2014-02-12 16:29:40.000000000 +0900
> @@ -186,6 +186,33 @@
>                 #gpio-range-cells = <3>;
>         };
>
> +       sdhi0: sd@ee100000 {
> +               compatible = "renesas,sdhi-r8a7791", "renesas,sdhi-r8a7790";

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 01/02] ARM: shmobile: Add SDHI devices to r8a7791 DTSI

[Magnus Damm]

Hi Geert,

On Wed, Feb 12, 2014 at 5:19 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Magnus,
>
> On Wed, Feb 12, 2014 at 8:41 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>> Changes since V1:
>>  - add r8a7790 suffix as fall back in case r8a7791 is missing from the driver
>
> I'm afraid that's not such a good idea: what if r8a7791 SDHI turns out to have a
> slight incompatibility with r8a7790 SDHI later?

It's a fallback so later we will simply use r8a7791 instead. r8a7790
is currently used with the v3.14-rc version of the SDHI driver. Future
ones always include r8a7791. How does that introduce any issues?

To be clear, I'm not introducing a new ABI here, just using something
that already exists. Of course, I can simply not use the fallback
which is what I did in V1 of the patch. But then I can't use the
v3.14-rc1 driver with r8a7791.

Morimoto-san has just posted a patch to add r8a7791 support to the
SDHI driver, so at some point in the future the r8a7790 association
won't be used anymore. Maybe v3.15-rc1, maybe later. This of course
depends on patch pick up pace.

> What about adding "renesas,sdhi-rcar-gen2" as a fallback instead, for
> both r8a7790
> and r8a7791?

Not sure if that makes much sense really, wouldn't that require driver
changes? If so we may as well modify the driver to add r8a7791
directly. =)

Also, I don't like to use "gen2" as compatible string. This because
all gen2 SoCs have not been released yet. For platform devices this is
not so serious due to lack of ABI, so there I am willing to accept
"gen2" or "r8a77xx" or something since we can change that any time.
But for DT bindings...

Ideally I'd prefer not to use the SoC string at all, but hey...

> This would reduce the size of the sh_mobile_sdhi_of_match[] table in
> drivers/mmc/host/sh_mobile_sdhi.c, as no SoC-specific entries are needed at
> the moment.

I understand the space saving benefit, but due to documentation issues
we don't really know the difference between various SoCs, so assuming
all the SoCs in the same generation is most likely not such a good
idea.

> A similar thing can be done for "renesas,sdhi-rcar-gen1" (as long as those DTSes
> are not in mainline).

We've had discussions internally about using the VERSION register in
the SDHI hardware. This would be needed in some cases because withing
a certain SoC the SDHI hardware varies with instance. So for r8a7790
SDHI0 and SDHI1 are different from SDHI2 and SDHI3. Now we get by
because they have different I/O space size.

Cheers,

/ magnus

Re: [PATCH v2 01/02] ARM: shmobile: Add SDHI devices to r8a7791 DTSI

[Geert Uytterhoeven]

On Wed, Feb 12, 2014 at 9:37 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>> On Wed, Feb 12, 2014 at 8:41 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>>> Changes since V1:
>>>  - add r8a7790 suffix as fall back in case r8a7791 is missing from the driver
>>
>> I'm afraid that's not such a good idea: what if r8a7791 SDHI turns out to have a
>> slight incompatibility with r8a7790 SDHI later?
>
> It's a fallback so later we will simply use r8a7791 instead. r8a7790
> is currently used with the v3.14-rc version of the SDHI driver. Future
> ones always include r8a7791. How does that introduce any issues?

Bummer, yes, it prefers the first compatible entry.

Will refrain from sending more emails until my coffee has been digested ;-)

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 01/02] ARM: shmobile: Add SDHI devices to r8a7791 DTSI

[Magnus Damm]

On Wed, Feb 12, 2014 at 5:48 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Wed, Feb 12, 2014 at 9:37 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>>> On Wed, Feb 12, 2014 at 8:41 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>>>> Changes since V1:
>>>>  - add r8a7790 suffix as fall back in case r8a7791 is missing from the driver
>>>
>>> I'm afraid that's not such a good idea: what if r8a7791 SDHI turns out to have a
>>> slight incompatibility with r8a7790 SDHI later?
>>
>> It's a fallback so later we will simply use r8a7791 instead. r8a7790
>> is currently used with the v3.14-rc version of the SDHI driver. Future
>> ones always include r8a7791. How does that introduce any issues?
>
> Bummer, yes, it prefers the first compatible entry.
>
> Will refrain from sending more emails until my coffee has been digested ;-)

Heheh. But I think you raise a valid point:

Using the wrong SoC in the compatible string is pretty darn ugly.

What is the best practise here?

/ magnus

Re: [PATCH v2 01/02] ARM: shmobile: Add SDHI devices to r8a7791 DTSI

[Geert Uytterhoeven]

On Wed, Feb 12, 2014 at 9:51 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>>>>>  - add r8a7790 suffix as fall back in case r8a7791 is missing from the driver

>>>>> +       sdhi0: sd@ee100000 {
>>>>> +               compatible = "renesas,sdhi-r8a7791", "renesas,sdhi-r8a7790";

>>>> I'm afraid that's not such a good idea: what if r8a7791 SDHI turns out to have a
>>>> slight incompatibility with r8a7790 SDHI later?
>>>
>>> It's a fallback so later we will simply use r8a7791 instead. r8a7790
>>> is currently used with the v3.14-rc version of the SDHI driver. Future
>>> ones always include r8a7791. How does that introduce any issues?
>>
>> Bummer, yes, it prefers the first compatible entry.
>>
>> Will refrain from sending more emails until my coffee has been digested ;-)
>
> Heheh. But I think you raise a valid point:
>
> Using the wrong SoC in the compatible string is pretty darn ugly.
>
> What is the best practise here?

(adding devicetree)

It should be a reference to the hardware block.

Before the advent of SoCs, a hardware block was an IC, with a part number
(e.g. de21040), and sometimes a name (e.g. tulip). Further evolutions of the
hardware block got (usually, but not always) different part numbers.

With SoCs, the part numbers of the hardware blocks were lost. Only the SoC
still carries a part number. The hardware blocks (now called "IP cores") still
have names (e.g. RSPI), but they're more abstract, and it's difficult to know
what exact version of the hardware block they're referring to[*].

Using "<manufacturer>,<name>-<soc>" is future-proof, but cumbersome.
Having a more generic fallback name to group SoCs with the same IP core
is convenient.

So we have to "invent" generic fallback names with versioning ourselves?

  - SoC family name? But there's no guarantee a new SoC from the same
    family will be 100% compatible.
  - "-v1", "-v2" suffixes? The new SoC may have something in between
    v1 and v2
  - ???

[*] With OpenRISC it's easier, as we have the hardware source files, but
    it's still cumbersome to find good version naming.
    The OpenCores-mandated "<name>-rtlsvn<version>" is not such a good
    match for todays distributed development, with IP cores being imported
    into git repositories and modified there.

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 01/02] ARM: shmobile: Add SDHI devices to r8a7791 DTSI

[Geert Uytterhoeven]

On Wed, Feb 12, 2014 at 9:51 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>>>>>  - add r8a7790 suffix as fall back in case r8a7791 is missing from the driver

>>>>> +       sdhi0: sd@ee100000 {
>>>>> +               compatible = "renesas,sdhi-r8a7791", "renesas,sdhi-r8a7790";

>>>> I'm afraid that's not such a good idea: what if r8a7791 SDHI turns out to have a
>>>> slight incompatibility with r8a7790 SDHI later?
>>>
>>> It's a fallback so later we will simply use r8a7791 instead. r8a7790
>>> is currently used with the v3.14-rc version of the SDHI driver. Future
>>> ones always include r8a7791. How does that introduce any issues?
>>
>> Bummer, yes, it prefers the first compatible entry.
>>
>> Will refrain from sending more emails until my coffee has been digested ;-)
>
> Heheh. But I think you raise a valid point:
>
> Using the wrong SoC in the compatible string is pretty darn ugly.
>
> What is the best practise here?

(adding devicetree)

It should be a reference to the hardware block.

Before the advent of SoCs, a hardware block was an IC, with a part number
(e.g. de21040), and sometimes a name (e.g. tulip). Further evolutions of the
hardware block got (usually, but not always) different part numbers.

With SoCs, the part numbers of the hardware blocks were lost. Only the SoC
still carries a part number. The hardware blocks (now called "IP cores") still
have names (e.g. RSPI), but they're more abstract, and it's difficult to know
what exact version of the hardware block they're referring to[*].

Using "<manufacturer>,<name>-<soc>" is future-proof, but cumbersome.
Having a more generic fallback name to group SoCs with the same IP core
is convenient.

So we have to "invent" generic fallback names with versioning ourselves?

  - SoC family name? But there's no guarantee a new SoC from the same
    family will be 100% compatible.
  - "-v1", "-v2" suffixes? The new SoC may have something in between
    v1 and v2
  - ???

[*] With OpenRISC it's easier, as we have the hardware source files, but
    it's still cumbersome to find good version naming.
    The OpenCores-mandated "<name>-rtlsvn<version>" is not such a good
    match for todays distributed development, with IP cores being imported
    into git repositories and modified there.

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