[PATCH 7/9] ARM: shmobile: APE6EVM: add MMCIF and SDHI DT nodes

[PATCH 7/9] ARM: shmobile: APE6EVM: add MMCIF and SDHI DT nodes

[Guennadi Liakhovetski]

This patch adds all MMCIF and SDHI devices to r8a73a4.dtsi in the "disabled"
state, those of them, available on APE6EVM, are then added to the board DT.
This version assignes fixed regulators to all the interfaces, in future
versions support for regulators should be added.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
---
 arch/arm/boot/dts/r8a73a4-ape6evm.dts |   46 +++++++++++++++++++++++++++++++++
 arch/arm/boot/dts/r8a73a4.dtsi        |   45 ++++++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/r8a73a4-ape6evm.dts b/arch/arm/boot/dts/r8a73a4-ape6evm.dts
index b5b24eb..63eaf5a 100644
--- a/arch/arm/boot/dts/r8a73a4-ape6evm.dts
+++ b/arch/arm/boot/dts/r8a73a4-ape6evm.dts
@@ -53,6 +53,37 @@
 	};
 };
 
+&mmcif0 {
+	pinctrl-0 = <&mmcif0_pins>;
+	pinctrl-names = "default";
+
+	vmmc-supply = <&ape6evm_fixed_3v3>;
+	bus-width = <8>;
+	non-removable;
+	status = "okay";
+};
+
+&sdhi0 {
+	pinctrl-0 = <&sdhi0_pins>;
+	pinctrl-names = "default";
+
+	vmmc-supply = <&ape6evm_fixed_3v3>;
+	bus-width = <4>;
+	toshiba,mmc-wrprotect-disable;
+	status = "okay";
+};
+
+&sdhi1 {
+	pinctrl-0 = <&sdhi1_pins>;
+	pinctrl-names = "default";
+
+	vmmc-supply = <&ape6evm_fixed_3v3>;
+	bus-width = <4>;
+	broken-cd;
+	toshiba,mmc-wrprotect-disable;
+	status = "okay";
+};
+
 &pfc {
 	pinctrl-0 = <&scifa0_pins>;
 	pinctrl-names = "default";
@@ -61,4 +92,19 @@
 		renesas,groups = "scifa0_data";
 		renesas,function = "scifa0";
 	};
+
+	mmcif0_pins: mmcif0 {
+		renesas,groups = "mmc0_data8", "mmc0_ctrl";
+		renesas,function = "mmc0";
+	};
+
+	sdhi0_pins: sdhi0 {
+		renesas,groups = "sdhi0_data4", "sdhi0_ctrl", "sdhi0_cd", "sdhi0_wp";
+		renesas,function = "sdhi0";
+	};
+
+	sdhi1_pins: sdhi1 {
+		renesas,groups = "sdhi1_data4", "sdhi1_ctrl";
+		renesas,function = "sdhi1";
+	};
 };
diff --git a/arch/arm/boot/dts/r8a73a4.dtsi b/arch/arm/boot/dts/r8a73a4.dtsi
index 6e73367..33c9bac 100644
--- a/arch/arm/boot/dts/r8a73a4.dtsi
+++ b/arch/arm/boot/dts/r8a73a4.dtsi
@@ -98,4 +98,49 @@
 		gpio-controller;
 		#gpio-cells = <2>;
 	};
+
+	mmcif0: mmcif@ee200000 {
+		compatible = "renesas,sh-mmcif";
+		reg = <0 0xee200000 0 0x100>;
+		interrupt-parent = <&gic>;
+		interrupts = <0 169 0x4>;
+		reg-io-width = <4>;
+		status = "disabled";
+	};
+
+	mmcif1: mmcif@ee220000 {
+		compatible = "renesas,sh-mmcif";
+		reg = <0 0xee220000 0 0x100>;
+		interrupt-parent = <&gic>;
+		interrupts = <0 170 0x4>;
+		reg-io-width = <4>;
+		status = "disabled";
+	};
+
+	sdhi0: sdhi@ee100000 {
+		compatible = "renesas,r8a7740-sdhi";
+		reg = <0 0xee100000 0 0x100>;
+		interrupt-parent = <&gic>;
+		interrupts = <0 165 4>;
+		cap-sd-highspeed;
+		status = "disabled";
+	};
+
+	sdhi1: sdhi@ee120000 {
+		compatible = "renesas,r8a7740-sdhi";
+		reg = <0 0xee120000 0 0x100>;
+		interrupt-parent = <&gic>;
+		interrupts = <0 166 4>;
+		cap-sd-highspeed;
+		status = "disabled";
+	};
+
+	sdhi2: sdhi@ee140000 {
+		compatible = "renesas,r8a7740-sdhi";
+		reg = <0 0xee140000 0 0x100>;
+		interrupt-parent = <&gic>;
+		interrupts = <0 167 4>;
+		cap-sd-highspeed;
+		status = "disabled";
+	};
 };
-- 
1.7.2.5

Re: [PATCH 7/9] ARM: shmobile: APE6EVM: add MMCIF and SDHI DT nodes

[Magnus Damm]

Hi Guennadi,

On Fri, May 17, 2013 at 11:55 PM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> This patch adds all MMCIF and SDHI devices to r8a73a4.dtsi in the "disabled"
> state, those of them, available on APE6EVM, are then added to the board DT.
> This version assignes fixed regulators to all the interfaces, in future
> versions support for regulators should be added.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>

Thanks for your work on this. In general I'm happy to see your
progress but I wonder about the following:

> --- a/arch/arm/boot/dts/r8a73a4.dtsi
> +++ b/arch/arm/boot/dts/r8a73a4.dtsi
> @@ -98,4 +98,49 @@
>                 gpio-controller;
>                 #gpio-cells = <2>;
>         };
> +
> +       mmcif0: mmcif@ee200000 {
> +               compatible = "renesas,sh-mmcif";
> +               reg = <0 0xee200000 0 0x100>;
> +               interrupt-parent = <&gic>;
> +               interrupts = <0 169 0x4>;
> +               reg-io-width = <4>;
> +               status = "disabled";
> +       };
> +
> +       mmcif1: mmcif@ee220000 {
> +               compatible = "renesas,sh-mmcif";
> +               reg = <0 0xee220000 0 0x100>;
> +               interrupt-parent = <&gic>;
> +               interrupts = <0 170 0x4>;
> +               reg-io-width = <4>;
> +               status = "disabled";
> +       };
> +
> +       sdhi0: sdhi@ee100000 {
> +               compatible = "renesas,r8a7740-sdhi";
> +               reg = <0 0xee100000 0 0x100>;
> +               interrupt-parent = <&gic>;
> +               interrupts = <0 165 4>;
> +               cap-sd-highspeed;
> +               status = "disabled";
> +       };

Here you use the "r8a7740-sdhi" compatible string on the r8a73a4 SoC.
Sorry to say this but to me this seems like the first steps toward a
future binary compatibility mess.

Others seem to handle this differently. For instance, in
drivers/mmc/host/omap_hsmmc.c there are separate compatible strings
for omap2 and omap3.

As I stated before, I prefer to see using a compatible string based on
the version of the IP block, or if we have to use the SoC name then
use the name of the actual SoC used.

What is the common way to deal with the compatible strings? Just use
an existing one if it happens to match? Seems a lot like copy-paste
integration to me with the added benefit of forever binary
compatibility.

Perhaps we have agreed on something already? Care to remind me? =)

Cheers,

/ magnus

Re: [PATCH 7/9] ARM: shmobile: APE6EVM: add MMCIF and SDHI DT nodes

[Guennadi Liakhovetski]

On Wed, 19 Jun 2013, Magnus Damm wrote:

> Hi Guennadi,
> 
> On Fri, May 17, 2013 at 11:55 PM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> > This patch adds all MMCIF and SDHI devices to r8a73a4.dtsi in the "disabled"
> > state, those of them, available on APE6EVM, are then added to the board DT.
> > This version assignes fixed regulators to all the interfaces, in future
> > versions support for regulators should be added.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
> 
> Thanks for your work on this. In general I'm happy to see your
> progress but I wonder about the following:
> 
> > --- a/arch/arm/boot/dts/r8a73a4.dtsi
> > +++ b/arch/arm/boot/dts/r8a73a4.dtsi
> > @@ -98,4 +98,49 @@
> >                 gpio-controller;
> >                 #gpio-cells = <2>;
> >         };
> > +
> > +       mmcif0: mmcif@ee200000 {
> > +               compatible = "renesas,sh-mmcif";
> > +               reg = <0 0xee200000 0 0x100>;
> > +               interrupt-parent = <&gic>;
> > +               interrupts = <0 169 0x4>;
> > +               reg-io-width = <4>;
> > +               status = "disabled";
> > +       };
> > +
> > +       mmcif1: mmcif@ee220000 {
> > +               compatible = "renesas,sh-mmcif";
> > +               reg = <0 0xee220000 0 0x100>;
> > +               interrupt-parent = <&gic>;
> > +               interrupts = <0 170 0x4>;
> > +               reg-io-width = <4>;
> > +               status = "disabled";
> > +       };
> > +
> > +       sdhi0: sdhi@ee100000 {
> > +               compatible = "renesas,r8a7740-sdhi";
> > +               reg = <0 0xee100000 0 0x100>;
> > +               interrupt-parent = <&gic>;
> > +               interrupts = <0 165 4>;
> > +               cap-sd-highspeed;
> > +               status = "disabled";
> > +       };
> 
> Here you use the "r8a7740-sdhi" compatible string on the r8a73a4 SoC.
> Sorry to say this but to me this seems like the first steps toward a
> future binary compatibility mess.
> 
> Others seem to handle this differently. For instance, in
> drivers/mmc/host/omap_hsmmc.c there are separate compatible strings
> for omap2 and omap3.
> 
> As I stated before, I prefer to see using a compatible string based on
> the version of the IP block, or if we have to use the SoC name then
> use the name of the actual SoC used.
> 
> What is the common way to deal with the compatible strings? Just use
> an existing one if it happens to match? Seems a lot like copy-paste
> integration to me with the added benefit of forever binary
> compatibility.
> 
> Perhaps we have agreed on something already? Care to remind me? =)

Indeed this has been discussed extensively:

http://thread.gmane.org/gmane.linux.kernel.mmc/19136/focus\x19248

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 7/9] ARM: shmobile: APE6EVM: add MMCIF and SDHI DT nodes

[Magnus Damm]

Hi Guennadi,

On Fri, Jun 21, 2013 at 6:10 AM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> On Wed, 19 Jun 2013, Magnus Damm wrote:
>
>> Hi Guennadi,
>>
>> On Fri, May 17, 2013 at 11:55 PM, Guennadi Liakhovetski
>> <g.liakhovetski@gmx.de> wrote:
>> > This patch adds all MMCIF and SDHI devices to r8a73a4.dtsi in the "disabled"
>> > state, those of them, available on APE6EVM, are then added to the board DT.
>> > This version assignes fixed regulators to all the interfaces, in future
>> > versions support for regulators should be added.
>> >
>> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
>>
>> Thanks for your work on this. In general I'm happy to see your
>> progress but I wonder about the following:
>>
>> > --- a/arch/arm/boot/dts/r8a73a4.dtsi
>> > +++ b/arch/arm/boot/dts/r8a73a4.dtsi
>> > @@ -98,4 +98,49 @@
>> >                 gpio-controller;
>> >                 #gpio-cells = <2>;
>> >         };
>> > +
>> > +       mmcif0: mmcif@ee200000 {
>> > +               compatible = "renesas,sh-mmcif";
>> > +               reg = <0 0xee200000 0 0x100>;
>> > +               interrupt-parent = <&gic>;
>> > +               interrupts = <0 169 0x4>;
>> > +               reg-io-width = <4>;
>> > +               status = "disabled";
>> > +       };
>> > +
>> > +       mmcif1: mmcif@ee220000 {
>> > +               compatible = "renesas,sh-mmcif";
>> > +               reg = <0 0xee220000 0 0x100>;
>> > +               interrupt-parent = <&gic>;
>> > +               interrupts = <0 170 0x4>;
>> > +               reg-io-width = <4>;
>> > +               status = "disabled";
>> > +       };
>> > +
>> > +       sdhi0: sdhi@ee100000 {
>> > +               compatible = "renesas,r8a7740-sdhi";
>> > +               reg = <0 0xee100000 0 0x100>;
>> > +               interrupt-parent = <&gic>;
>> > +               interrupts = <0 165 4>;
>> > +               cap-sd-highspeed;
>> > +               status = "disabled";
>> > +       };
>>
>> Here you use the "r8a7740-sdhi" compatible string on the r8a73a4 SoC.
>> Sorry to say this but to me this seems like the first steps toward a
>> future binary compatibility mess.
>>
>> Others seem to handle this differently. For instance, in
>> drivers/mmc/host/omap_hsmmc.c there are separate compatible strings
>> for omap2 and omap3.
>>
>> As I stated before, I prefer to see using a compatible string based on
>> the version of the IP block, or if we have to use the SoC name then
>> use the name of the actual SoC used.
>>
>> What is the common way to deal with the compatible strings? Just use
>> an existing one if it happens to match? Seems a lot like copy-paste
>> integration to me with the added benefit of forever binary
>> compatibility.
>>
>> Perhaps we have agreed on something already? Care to remind me? =)
>
> Indeed this has been discussed extensively:
>
> http://thread.gmane.org/gmane.linux.kernel.mmc/19136/focus\x19248

Yes, I know that, but did we also come to a conclusion how to handle
the compatible string in case we have several SoCs that happen to have
similar compatible hardware block versions?

It seems that the SDHI driver has one compatible string for sh7372,
and when we remove sh7372 SoC support in the future it will be easy to
remove that too. But how about r8a7740? If we have multiple different
SoCs that use same SoC compatible string (r8a7740 in this case) then
how can we age out these strings?

My take on this that if you use the SoC name for the compatible string
then you should also keep on updating the driver with this information
so the DTS for the SoC can use the correct SoC name.

Thanks,

/ magnus

Re: [PATCH 7/9] ARM: shmobile: APE6EVM: add MMCIF and SDHI DT nodes

[Guennadi Liakhovetski]

Hi Magnus

On Fri, 21 Jun 2013, Magnus Damm wrote:

> Hi Guennadi,
> 
> On Fri, Jun 21, 2013 at 6:10 AM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> > On Wed, 19 Jun 2013, Magnus Damm wrote:
> >
> >> Hi Guennadi,
> >>
> >> On Fri, May 17, 2013 at 11:55 PM, Guennadi Liakhovetski
> >> <g.liakhovetski@gmx.de> wrote:
> >> > This patch adds all MMCIF and SDHI devices to r8a73a4.dtsi in the "disabled"
> >> > state, those of them, available on APE6EVM, are then added to the board DT.
> >> > This version assignes fixed regulators to all the interfaces, in future
> >> > versions support for regulators should be added.
> >> >
> >> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
> >>
> >> Thanks for your work on this. In general I'm happy to see your
> >> progress but I wonder about the following:
> >>
> >> > --- a/arch/arm/boot/dts/r8a73a4.dtsi
> >> > +++ b/arch/arm/boot/dts/r8a73a4.dtsi
> >> > @@ -98,4 +98,49 @@
> >> >                 gpio-controller;
> >> >                 #gpio-cells = <2>;
> >> >         };
> >> > +
> >> > +       mmcif0: mmcif@ee200000 {
> >> > +               compatible = "renesas,sh-mmcif";
> >> > +               reg = <0 0xee200000 0 0x100>;
> >> > +               interrupt-parent = <&gic>;
> >> > +               interrupts = <0 169 0x4>;
> >> > +               reg-io-width = <4>;
> >> > +               status = "disabled";
> >> > +       };
> >> > +
> >> > +       mmcif1: mmcif@ee220000 {
> >> > +               compatible = "renesas,sh-mmcif";
> >> > +               reg = <0 0xee220000 0 0x100>;
> >> > +               interrupt-parent = <&gic>;
> >> > +               interrupts = <0 170 0x4>;
> >> > +               reg-io-width = <4>;
> >> > +               status = "disabled";
> >> > +       };
> >> > +
> >> > +       sdhi0: sdhi@ee100000 {
> >> > +               compatible = "renesas,r8a7740-sdhi";
> >> > +               reg = <0 0xee100000 0 0x100>;
> >> > +               interrupt-parent = <&gic>;
> >> > +               interrupts = <0 165 4>;
> >> > +               cap-sd-highspeed;
> >> > +               status = "disabled";
> >> > +       };
> >>
> >> Here you use the "r8a7740-sdhi" compatible string on the r8a73a4 SoC.
> >> Sorry to say this but to me this seems like the first steps toward a
> >> future binary compatibility mess.
> >>
> >> Others seem to handle this differently. For instance, in
> >> drivers/mmc/host/omap_hsmmc.c there are separate compatible strings
> >> for omap2 and omap3.
> >>
> >> As I stated before, I prefer to see using a compatible string based on
> >> the version of the IP block, or if we have to use the SoC name then
> >> use the name of the actual SoC used.
> >>
> >> What is the common way to deal with the compatible strings? Just use
> >> an existing one if it happens to match? Seems a lot like copy-paste
> >> integration to me with the added benefit of forever binary
> >> compatibility.
> >>
> >> Perhaps we have agreed on something already? Care to remind me? =)
> >
> > Indeed this has been discussed extensively:
> >
> > http://thread.gmane.org/gmane.linux.kernel.mmc/19136/focus\x19248
> 
> Yes, I know that, but did we also come to a conclusion how to handle
> the compatible string in case we have several SoCs that happen to have
> similar compatible hardware block versions?
> 
> It seems that the SDHI driver has one compatible string for sh7372,
> and when we remove sh7372 SoC support in the future it will be easy to
> remove that too. But how about r8a7740? If we have multiple different
> SoCs that use same SoC compatible string (r8a7740 in this case) then
> how can we age out these strings?
> 
> My take on this that if you use the SoC name for the compatible string
> then you should also keep on updating the driver with this information
> so the DTS for the SoC can use the correct SoC name.

A quote from the above link:

"the next best solution would be naming it
after the chip that first used a particular version of that IP block,
like "renesas,shmobile1234-sdhi". The device tree include file for
shmobile5678 should then list the sdhi part as being compatible with
the "reneasas,shmobile1234-sdhi" if they are the same, or as
a separate "reneasas,shmobile5678-sdhi" if they behave in a different
way."

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 7/9] ARM: shmobile: APE6EVM: add MMCIF and SDHI DT nodes

[Magnus Damm]

Hi Guennadi,

[Added Arnd to CC]

On Fri, Jun 21, 2013 at 3:33 PM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> Hi Magnus
>
> On Fri, 21 Jun 2013, Magnus Damm wrote:
>
>> Hi Guennadi,
>>
>> On Fri, Jun 21, 2013 at 6:10 AM, Guennadi Liakhovetski
>> <g.liakhovetski@gmx.de> wrote:
>> > On Wed, 19 Jun 2013, Magnus Damm wrote:
>> >
>> >> Hi Guennadi,
>> >>
>> >> On Fri, May 17, 2013 at 11:55 PM, Guennadi Liakhovetski
>> >> <g.liakhovetski@gmx.de> wrote:
>> >> > This patch adds all MMCIF and SDHI devices to r8a73a4.dtsi in the "disabled"
>> >> > state, those of them, available on APE6EVM, are then added to the board DT.
>> >> > This version assignes fixed regulators to all the interfaces, in future
>> >> > versions support for regulators should be added.
>> >> >
>> >> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
>> >>
>> >> Thanks for your work on this. In general I'm happy to see your
>> >> progress but I wonder about the following:
>> >>
>> >> > --- a/arch/arm/boot/dts/r8a73a4.dtsi
>> >> > +++ b/arch/arm/boot/dts/r8a73a4.dtsi
>> >> > @@ -98,4 +98,49 @@
>> >> >                 gpio-controller;
>> >> >                 #gpio-cells = <2>;
>> >> >         };
>> >> > +
>> >> > +       mmcif0: mmcif@ee200000 {
>> >> > +               compatible = "renesas,sh-mmcif";
>> >> > +               reg = <0 0xee200000 0 0x100>;
>> >> > +               interrupt-parent = <&gic>;
>> >> > +               interrupts = <0 169 0x4>;
>> >> > +               reg-io-width = <4>;
>> >> > +               status = "disabled";
>> >> > +       };
>> >> > +
>> >> > +       mmcif1: mmcif@ee220000 {
>> >> > +               compatible = "renesas,sh-mmcif";
>> >> > +               reg = <0 0xee220000 0 0x100>;
>> >> > +               interrupt-parent = <&gic>;
>> >> > +               interrupts = <0 170 0x4>;
>> >> > +               reg-io-width = <4>;
>> >> > +               status = "disabled";
>> >> > +       };
>> >> > +
>> >> > +       sdhi0: sdhi@ee100000 {
>> >> > +               compatible = "renesas,r8a7740-sdhi";
>> >> > +               reg = <0 0xee100000 0 0x100>;
>> >> > +               interrupt-parent = <&gic>;
>> >> > +               interrupts = <0 165 4>;
>> >> > +               cap-sd-highspeed;
>> >> > +               status = "disabled";
>> >> > +       };
>> >>
>> >> Here you use the "r8a7740-sdhi" compatible string on the r8a73a4 SoC.
>> >> Sorry to say this but to me this seems like the first steps toward a
>> >> future binary compatibility mess.
>> >>
>> >> Others seem to handle this differently. For instance, in
>> >> drivers/mmc/host/omap_hsmmc.c there are separate compatible strings
>> >> for omap2 and omap3.
>> >>
>> >> As I stated before, I prefer to see using a compatible string based on
>> >> the version of the IP block, or if we have to use the SoC name then
>> >> use the name of the actual SoC used.
>> >>
>> >> What is the common way to deal with the compatible strings? Just use
>> >> an existing one if it happens to match? Seems a lot like copy-paste
>> >> integration to me with the added benefit of forever binary
>> >> compatibility.
>> >>
>> >> Perhaps we have agreed on something already? Care to remind me? =)
>> >
>> > Indeed this has been discussed extensively:
>> >
>> > http://thread.gmane.org/gmane.linux.kernel.mmc/19136/focus\x19248
>>
>> Yes, I know that, but did we also come to a conclusion how to handle
>> the compatible string in case we have several SoCs that happen to have
>> similar compatible hardware block versions?
>>
>> It seems that the SDHI driver has one compatible string for sh7372,
>> and when we remove sh7372 SoC support in the future it will be easy to
>> remove that too. But how about r8a7740? If we have multiple different
>> SoCs that use same SoC compatible string (r8a7740 in this case) then
>> how can we age out these strings?
>>
>> My take on this that if you use the SoC name for the compatible string
>> then you should also keep on updating the driver with this information
>> so the DTS for the SoC can use the correct SoC name.
>
> A quote from the above link:
>
> "the next best solution would be naming it
> after the chip that first used a particular version of that IP block,
> like "renesas,shmobile1234-sdhi". The device tree include file for
> shmobile5678 should then list the sdhi part as being compatible with
> the "reneasas,shmobile1234-sdhi" if they are the same, or as
> a separate "reneasas,shmobile5678-sdhi" if they behave in a different
> way."

Thanks for the quote. I can now see clearly about shmoble5678 and shmobile1234.

I hate to be difficult, but I disagree. =)

Perhaps my logic is flawed, but if I understand the above then we are
supposed to put DTS device nodes with compatible strings based on
current driver behaviour?

I see one problem with that. Say that a certain new hardware feature
is included in one SoC, but the driver does not yet implement it. And
perhaps the driver developer does not understand the difference fully
when the device node is added to the DTS. Then won't we end up in a
situation where people may roll out DTBs in products and that would
not allow us to enable the feature later in the driver with a software
update? Now, if we always used shmobile5678 for shmobile5678 then we
would never have any problems.

Of course, I prefer to use a version number for the hardware IP block
instead of the SoC. This since this is not a SoC property. But if we
now must be using SoC compatible strings then I think we should do it
in a sane way that would reduce or risk of shooting ourself it the
foot.

As I pointed out earlier, the MMC drivers from other vendors seem to
use compatible strings with names per-SoC or per-SoC-family. This even
though they don't seem to handle any difference from a software
perspective.

So it seems to me that exactly how to handle this varies from place to place.

Cheers,

/ magnus

Re: [PATCH 7/9] ARM: shmobile: APE6EVM: add MMCIF and SDHI DT nodes

[Guennadi Liakhovetski]

Hi Magnus

On Fri, 21 Jun 2013, Magnus Damm wrote:

> Hi Guennadi,
> 
> [Added Arnd to CC]
> 
> On Fri, Jun 21, 2013 at 3:33 PM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> > Hi Magnus
> >
> > On Fri, 21 Jun 2013, Magnus Damm wrote:
> >
> >> Hi Guennadi,
> >>
> >> On Fri, Jun 21, 2013 at 6:10 AM, Guennadi Liakhovetski
> >> <g.liakhovetski@gmx.de> wrote:
> >> > On Wed, 19 Jun 2013, Magnus Damm wrote:
> >> >
> >> >> Hi Guennadi,
> >> >>
> >> >> On Fri, May 17, 2013 at 11:55 PM, Guennadi Liakhovetski
> >> >> <g.liakhovetski@gmx.de> wrote:
> >> >> > This patch adds all MMCIF and SDHI devices to r8a73a4.dtsi in the "disabled"
> >> >> > state, those of them, available on APE6EVM, are then added to the board DT.
> >> >> > This version assignes fixed regulators to all the interfaces, in future
> >> >> > versions support for regulators should be added.
> >> >> >
> >> >> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
> >> >>
> >> >> Thanks for your work on this. In general I'm happy to see your
> >> >> progress but I wonder about the following:
> >> >>
> >> >> > --- a/arch/arm/boot/dts/r8a73a4.dtsi
> >> >> > +++ b/arch/arm/boot/dts/r8a73a4.dtsi
> >> >> > @@ -98,4 +98,49 @@
> >> >> >                 gpio-controller;
> >> >> >                 #gpio-cells = <2>;
> >> >> >         };
> >> >> > +
> >> >> > +       mmcif0: mmcif@ee200000 {
> >> >> > +               compatible = "renesas,sh-mmcif";
> >> >> > +               reg = <0 0xee200000 0 0x100>;
> >> >> > +               interrupt-parent = <&gic>;
> >> >> > +               interrupts = <0 169 0x4>;
> >> >> > +               reg-io-width = <4>;
> >> >> > +               status = "disabled";
> >> >> > +       };
> >> >> > +
> >> >> > +       mmcif1: mmcif@ee220000 {
> >> >> > +               compatible = "renesas,sh-mmcif";
> >> >> > +               reg = <0 0xee220000 0 0x100>;
> >> >> > +               interrupt-parent = <&gic>;
> >> >> > +               interrupts = <0 170 0x4>;
> >> >> > +               reg-io-width = <4>;
> >> >> > +               status = "disabled";
> >> >> > +       };
> >> >> > +
> >> >> > +       sdhi0: sdhi@ee100000 {
> >> >> > +               compatible = "renesas,r8a7740-sdhi";
> >> >> > +               reg = <0 0xee100000 0 0x100>;
> >> >> > +               interrupt-parent = <&gic>;
> >> >> > +               interrupts = <0 165 4>;
> >> >> > +               cap-sd-highspeed;
> >> >> > +               status = "disabled";
> >> >> > +       };
> >> >>
> >> >> Here you use the "r8a7740-sdhi" compatible string on the r8a73a4 SoC.
> >> >> Sorry to say this but to me this seems like the first steps toward a
> >> >> future binary compatibility mess.
> >> >>
> >> >> Others seem to handle this differently. For instance, in
> >> >> drivers/mmc/host/omap_hsmmc.c there are separate compatible strings
> >> >> for omap2 and omap3.
> >> >>
> >> >> As I stated before, I prefer to see using a compatible string based on
> >> >> the version of the IP block, or if we have to use the SoC name then
> >> >> use the name of the actual SoC used.
> >> >>
> >> >> What is the common way to deal with the compatible strings? Just use
> >> >> an existing one if it happens to match? Seems a lot like copy-paste
> >> >> integration to me with the added benefit of forever binary
> >> >> compatibility.
> >> >>
> >> >> Perhaps we have agreed on something already? Care to remind me? =)
> >> >
> >> > Indeed this has been discussed extensively:
> >> >
> >> > http://thread.gmane.org/gmane.linux.kernel.mmc/19136/focus\x19248
> >>
> >> Yes, I know that, but did we also come to a conclusion how to handle
> >> the compatible string in case we have several SoCs that happen to have
> >> similar compatible hardware block versions?
> >>
> >> It seems that the SDHI driver has one compatible string for sh7372,
> >> and when we remove sh7372 SoC support in the future it will be easy to
> >> remove that too. But how about r8a7740? If we have multiple different
> >> SoCs that use same SoC compatible string (r8a7740 in this case) then
> >> how can we age out these strings?
> >>
> >> My take on this that if you use the SoC name for the compatible string
> >> then you should also keep on updating the driver with this information
> >> so the DTS for the SoC can use the correct SoC name.
> >
> > A quote from the above link:
> >
> > "the next best solution would be naming it
> > after the chip that first used a particular version of that IP block,
> > like "renesas,shmobile1234-sdhi". The device tree include file for
> > shmobile5678 should then list the sdhi part as being compatible with
> > the "reneasas,shmobile1234-sdhi" if they are the same, or as
> > a separate "reneasas,shmobile5678-sdhi" if they behave in a different
> > way."
> 
> Thanks for the quote. I can now see clearly about shmoble5678 and shmobile1234.
> 
> I hate to be difficult, but I disagree. =)
> 
> Perhaps my logic is flawed, but if I understand the above then we are
> supposed to put DTS device nodes with compatible strings based on
> current driver behaviour?
> 
> I see one problem with that. Say that a certain new hardware feature
> is included in one SoC, but the driver does not yet implement it. And
> perhaps the driver developer does not understand the difference fully
> when the device node is added to the DTS. Then won't we end up in a
> situation where people may roll out DTBs in products and that would
> not allow us to enable the feature later in the driver with a software
> update? Now, if we always used shmobile5678 for shmobile5678 then we
> would never have any problems.

My preference still lies with specific per-feature driver bindings like in 
my original implementation. Adding compatible strings to every driver for 
every new SoC seems an absolute no-go to me. But with per-feature driver 
bindings you would have exactly the same problem: if you later add support 
for a new feature to the driver, and that feature is present on older 
SoCs, the only way to enable it would be to replace the DT.

Thanks
Guennadi

> Of course, I prefer to use a version number for the hardware IP block
> instead of the SoC. This since this is not a SoC property. But if we
> now must be using SoC compatible strings then I think we should do it
> in a sane way that would reduce or risk of shooting ourself it the
> foot.
> 
> As I pointed out earlier, the MMC drivers from other vendors seem to
> use compatible strings with names per-SoC or per-SoC-family. This even
> though they don't seem to handle any difference from a software
> perspective.
> 
> So it seems to me that exactly how to handle this varies from place to place.
> 
> Cheers,
> 
> / magnus
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 7/9] ARM: shmobile: APE6EVM: add MMCIF and SDHI DT nodes

[Laurent Pinchart]

Hi Guennadi and Magnus,

On Friday 21 June 2013 09:48:43 Guennadi Liakhovetski wrote:
> On Fri, 21 Jun 2013, Magnus Damm wrote:
> > On Fri, Jun 21, 2013 at 3:33 PM, Guennadi Liakhovetski wrote:
> > > On Fri, 21 Jun 2013, Magnus Damm wrote:
> > >> On Fri, Jun 21, 2013 at 6:10 AM, Guennadi Liakhovetski wrote:
> > >> > On Wed, 19 Jun 2013, Magnus Damm wrote:
> > >> >> On Fri, May 17, 2013 at 11:55 PM, Guennadi Liakhovetski wrote:
> > >> >> > This patch adds all MMCIF and SDHI devices to r8a73a4.dtsi in the
> > >> >> > "disabled" state, those of them, available on APE6EVM, are then
> > >> >> > added to the board DT. This version assignes fixed regulators to
> > >> >> > all the interfaces, in future versions support for regulators
> > >> >> > should be added.
> > >> >> > 
> > >> >> > Signed-off-by: Guennadi Liakhovetski
> > >> >> > <g.liakhovetski+renesas@gmail.com>
> > >> >> 
> > >> >> Thanks for your work on this. In general I'm happy to see your
> > >> >> 
> > >> >> progress but I wonder about the following:
> > >> >> > --- a/arch/arm/boot/dts/r8a73a4.dtsi
> > >> >> > +++ b/arch/arm/boot/dts/r8a73a4.dtsi
> > >> >> > @@ -98,4 +98,49 @@
> > >> >> > 
> > >> >> >                 gpio-controller;
> > >> >> >                 #gpio-cells = <2>;
> > >> >> >         
> > >> >> >         };
> > >> >> > 
> > >> >> > +
> > >> >> > +       mmcif0: mmcif@ee200000 {
> > >> >> > +               compatible = "renesas,sh-mmcif";
> > >> >> > +               reg = <0 0xee200000 0 0x100>;
> > >> >> > +               interrupt-parent = <&gic>;
> > >> >> > +               interrupts = <0 169 0x4>;
> > >> >> > +               reg-io-width = <4>;
> > >> >> > +               status = "disabled";
> > >> >> > +       };
> > >> >> > +
> > >> >> > +       mmcif1: mmcif@ee220000 {
> > >> >> > +               compatible = "renesas,sh-mmcif";
> > >> >> > +               reg = <0 0xee220000 0 0x100>;
> > >> >> > +               interrupt-parent = <&gic>;
> > >> >> > +               interrupts = <0 170 0x4>;
> > >> >> > +               reg-io-width = <4>;
> > >> >> > +               status = "disabled";
> > >> >> > +       };
> > >> >> > +
> > >> >> > +       sdhi0: sdhi@ee100000 {
> > >> >> > +               compatible = "renesas,r8a7740-sdhi";
> > >> >> > +               reg = <0 0xee100000 0 0x100>;
> > >> >> > +               interrupt-parent = <&gic>;
> > >> >> > +               interrupts = <0 165 4>;
> > >> >> > +               cap-sd-highspeed;
> > >> >> > +               status = "disabled";
> > >> >> > +       };
> > >> >> 
> > >> >> Here you use the "r8a7740-sdhi" compatible string on the r8a73a4
> > >> >> SoC. Sorry to say this but to me this seems like the first steps
> > >> >> toward a future binary compatibility mess.
> > >> >> 
> > >> >> Others seem to handle this differently. For instance, in
> > >> >> drivers/mmc/host/omap_hsmmc.c there are separate compatible strings
> > >> >> for omap2 and omap3.
> > >> >> 
> > >> >> As I stated before, I prefer to see using a compatible string based
> > >> >> on the version of the IP block, or if we have to use the SoC name
> > >> >> then use the name of the actual SoC used.
> > >> >> 
> > >> >> What is the common way to deal with the compatible strings? Just use
> > >> >> an existing one if it happens to match? Seems a lot like copy-paste
> > >> >> integration to me with the added benefit of forever binary
> > >> >> compatibility.
> > >> >> 
> > >> >> Perhaps we have agreed on something already? Care to remind me? =)
> > >> > 
> > >> > Indeed this has been discussed extensively:
> > >> > 
> > >> > http://thread.gmane.org/gmane.linux.kernel.mmc/19136/focus\x19248
> > >> 
> > >> Yes, I know that, but did we also come to a conclusion how to handle
> > >> the compatible string in case we have several SoCs that happen to have
> > >> similar compatible hardware block versions?
> > >> 
> > >> It seems that the SDHI driver has one compatible string for sh7372,
> > >> and when we remove sh7372 SoC support in the future it will be easy to
> > >> remove that too. But how about r8a7740? If we have multiple different
> > >> SoCs that use same SoC compatible string (r8a7740 in this case) then
> > >> how can we age out these strings?
> > >> 
> > >> My take on this that if you use the SoC name for the compatible string
> > >> then you should also keep on updating the driver with this information
> > >> so the DTS for the SoC can use the correct SoC name.
> > > 
> > > A quote from the above link:
> > > 
> > > "the next best solution would be naming it
> > > after the chip that first used a particular version of that IP block,
> > > like "renesas,shmobile1234-sdhi". The device tree include file for
> > > shmobile5678 should then list the sdhi part as being compatible with
> > > the "reneasas,shmobile1234-sdhi" if they are the same, or as
> > > a separate "reneasas,shmobile5678-sdhi" if they behave in a different
> > > way."
> > 
> > Thanks for the quote. I can now see clearly about shmoble5678 and
> > shmobile1234.
> > 
> > I hate to be difficult, but I disagree. =)
> > 
> > Perhaps my logic is flawed, but if I understand the above then we are
> > supposed to put DTS device nodes with compatible strings based on
> > current driver behaviour?
> > 
> > I see one problem with that. Say that a certain new hardware feature
> > is included in one SoC, but the driver does not yet implement it. And
> > perhaps the driver developer does not understand the difference fully
> > when the device node is added to the DTS. Then won't we end up in a
> > situation where people may roll out DTBs in products and that would
> > not allow us to enable the feature later in the driver with a software
> > update? Now, if we always used shmobile5678 for shmobile5678 then we
> > would never have any problems.
> 
> My preference still lies with specific per-feature driver bindings like in
> my original implementation. Adding compatible strings to every driver for
> every new SoC seems an absolute no-go to me.

Isn't it an existing practice used by many DT bindings in the upstream kernel 
?

> But with per-feature driver bindings you would have exactly the same
> problem: if you later add support for a new feature to the driver, and that
> feature is present on older SoCs, the only way to enable it would be to
> replace the DT.
>
> > Of course, I prefer to use a version number for the hardware IP block
> > instead of the SoC.

Let's start with the obvious. Could we get those version numbers ?

> > This since this is not a SoC property.

Probably not directly applicable to the MMCIF and SDHI, when it comes to how 
IP are integrated in an SoC some of the IP-specific properties are actually 
SoC-specific. For instance on R9A7790 the Video Signal Processor can output 
directly to the Display Unit, the way this is configured on both IP cores 
depend not only on the IP version, but is specific to the R8A7790. I will thus 
use SoC-specific compatible strings for those drivers.

> > But if we now must be using SoC compatible strings then I think we should
> > do it in a sane way that would reduce or risk of shooting ourself it the
> > foot.
> > 
> > As I pointed out earlier, the MMC drivers from other vendors seem to
> > use compatible strings with names per-SoC or per-SoC-family. This even
> > though they don't seem to handle any difference from a software
> > perspective.
> > 
> > So it seems to me that exactly how to handle this varies from place to
> > place.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 7/9] ARM: shmobile: APE6EVM: add MMCIF and SDHI DT nodes

[Arnd Bergmann]

On Friday 21 June 2013, Magnus Damm wrote:
> > "the next best solution would be naming it
> > after the chip that first used a particular version of that IP block,
> > like "renesas,shmobile1234-sdhi". The device tree include file for
> > shmobile5678 should then list the sdhi part as being compatible with
> > the "reneasas,shmobile1234-sdhi" if they are the same, or as
> > a separate "reneasas,shmobile5678-sdhi" if they behave in a different
> > way."
>
> Thanks for the quote. I can now see clearly about shmoble5678 and shmobile1234.
> 
> I hate to be difficult, but I disagree. =)
> 
> Perhaps my logic is flawed, but if I understand the above then we are
> supposed to put DTS device nodes with compatible strings based on
> current driver behaviour?
> 
> I see one problem with that. Say that a certain new hardware feature
> is included in one SoC, but the driver does not yet implement it. And
> perhaps the driver developer does not understand the difference fully
> when the device node is added to the DTS. Then won't we end up in a
> situation where people may roll out DTBs in products and that would
> not allow us to enable the feature later in the driver with a software
> update? Now, if we always used shmobile5678 for shmobile5678 then we
> would never have any problems.

I said that you should use compatible="reneasas,shmobile1234-sdhi" if
it is the /same/ block as the one used in 5678. If the new one is
actually backwards compatible but has additional features (or may
have them but you are not sure), then I would always list both,
so 'compatible="reneasas,shmobile5678-sdhi", "reneasas,shmobile1234-sdhi",
"renesas,shmobile-sdhi";'.

You always start with the most specific model string (and in doubt
you can be very specific here, up to a minor stepping release) and
list the more generic strings to the end. 

This way you can have both: the driver only needs to match shmobile1234
if it doesn't use any of the newer features, but can check for
shmobile5678 if it wants to.

For chips that you don't already know about, it is not a big thing
anyway, you can always use the most recent SoC string in the device
tree and put that into the driver. However, we want to ideally
support future SoCs with an unmodified driver, and that can only
work if the future part claims compatibility with an older one.

Using the same logic of putting the older 'compatible' strings
into the DT for compatible parts at the time of the initial binding
is not a technical necessity but is done for consistency with the
way we have to do it later.

> Of course, I prefer to use a version number for the hardware IP block
> instead of the SoC. This since this is not a SoC property. But if we
> now must be using SoC compatible strings then I think we should do it
> in a sane way that would reduce or risk of shooting ourself it the
> foot.

Not sure I get this. Do you have to use the SoC identifier because
you don't have access to the hardware IP block version, or because
someone told you to? Using the hardware IP block version is defintely
preferred in my opinion.

	Arnd

Re: [PATCH 7/9] ARM: shmobile: APE6EVM: add MMCIF and SDHI DT nodes

[Arnd Bergmann]

On Friday 21 June 2013, Guennadi Liakhovetski wrote:
> My preference still lies with specific per-feature driver bindings like in 
> my original implementation. Adding compatible strings to every driver for 
> every new SoC seems an absolute no-go to me. But with per-feature driver 
> bindings you would have exactly the same problem: if you later add support 
> for a new feature to the driver, and that feature is present on older 
> SoCs, the only way to enable it would be to replace the DT.

That is the main problem with the per-feature listing, but less of
a problem if you list both the original part and the specific name of
the new one in the compatible string.

It's also not an either-or thing. For a number of features, having
flags makes most sense, in particular when the feature is not
a property of the IP block but of the way it is connected to the
rest of the SoC.

	Arnd