[PATCH 5/6] ARM: shmobile: r8a73a4: add div6 clocks

[PATCH 5/6] ARM: shmobile: r8a73a4: add div6 clocks

[Kuninori Morimoto]

DIV6 clocks control each core clocks.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 arch/arm/mach-shmobile/clock-r8a73a4.c |  182 ++++++++++++++++++++++++++++----
 1 file changed, 163 insertions(+), 19 deletions(-)

diff --git a/arch/arm/mach-shmobile/clock-r8a73a4.c b/arch/arm/mach-shmobile/clock-r8a73a4.c
index 0149bbf..001b469 100644
--- a/arch/arm/mach-shmobile/clock-r8a73a4.c
+++ b/arch/arm/mach-shmobile/clock-r8a73a4.c
@@ -28,20 +28,35 @@
 #define CPG_BASE 0xe6150000
 #define CPG_LEN 0x270
 
-#define MPCKCR 0xe6150080
 #define SMSTPCR2 0xe6150138
 #define SMSTPCR5 0xe6150144
 
 #define FRQCRA		0xE6150000
 #define FRQCRB		0xE6150004
-#define CKSCR		0xE61500C0
+#define VCLKCR1		0xE6150008
+#define VCLKCR2		0xE615000C
+#define VCLKCR3		0xE615001C
+#define VCLKCR4		0xE6150014
+#define VCLKCR5		0xE6150034
+#define ZBCKCR		0xE6150010
+#define SD0CKCR		0xE6150074
+#define SD1CKCR		0xE6150078
+#define SD2CKCR		0xE615007C
+#define MMC0CKCR	0xE6150240
+#define MMC1CKCR	0xE6150244
+#define FSIACKCR	0xE6150018
+#define FSIBCKCR	0xE6150090
+#define MPCKCR		0xe6150080
+#define SPUVCKCR	0xE6150094
+#define HSICKCR		0xE615026C
+#define M4CKCR		0xE6150098
 #define PLLECR		0xE61500D0
 #define PLL1CR		0xE6150028
 #define PLL2CR		0xE615002C
 #define PLL3CR		0xE61500DC
 #define PLL2SCR		0xE61501F4
 #define PLL2HCR		0xE61501E4
-
+#define CKSCR		0xE61500C0
 
 #define CPG_MAP(o) ((o - CPG_BASE) + cpg_mapping.base)
 
@@ -82,6 +97,13 @@ SH_FIXED_RATIO_CLK(extal1_div2_clk,	extal1_clk,		div2);
 SH_FIXED_RATIO_CLK(extal2_div2_clk,	extal2_clk,		div2);
 SH_FIXED_RATIO_CLK(extal2_div4_clk,	extal2_clk,		div4);
 
+/* External FSIACK/FSIBCK clock */
+static struct clk fsiack_clk = {
+};
+
+static struct clk fsibck_clk = {
+};
+
 static struct clk *main_clks[] = {
 	&extalr_clk,
 	&extal1_clk,
@@ -91,6 +113,8 @@ static struct clk *main_clks[] = {
 	&extal2_div4_clk,
 	&main_clk,
 	&main_div2_clk,
+	&fsiack_clk,
+	&fsibck_clk,
 };
 
 /*
@@ -222,6 +246,111 @@ static struct clk div4_clks[DIV4_NR] = {
 	[DIV4_HP]	= SH_CLK_DIV4(&pll1_clk, FRQCRB,  4, 0x0dff, 0),
 };
 
+enum {
+	DIV6_ZB,
+	DIV6_SDHI0, DIV6_SDHI1, DIV6_SDHI2,
+	DIV6_MMC0, DIV6_MMC1,
+	DIV6_VCK1, DIV6_VCK2, DIV6_VCK3, DIV6_VCK4, DIV6_VCK5,
+	DIV6_FSIA, DIV6_FSIB,
+	DIV6_MP, DIV6_M4, DIV6_HSI, DIV6_SPUV,
+	DIV6_NR };
+
+static struct clk *div6_parents[8] = {
+	[0] = &pll1_div2_clk,
+	[1] = &pll2s_clk,
+	[3] = &extal2_clk,
+	[4] = &main_div2_clk,
+	[6] = &extalr_clk,
+};
+
+static struct clk *fsia_parents[4] = {
+	[0] = &pll1_div2_clk,
+	[1] = &pll2s_clk,
+	[2] = &fsiack_clk,
+};
+
+static struct clk *fsib_parents[4] = {
+	[0] = &pll1_div2_clk,
+	[1] = &pll2s_clk,
+	[2] = &fsibck_clk,
+};
+
+static struct clk *mp_parents[4] = {
+	[0] = &pll1_div2_clk,
+	[1] = &pll2s_clk,
+	[2] = &extal2_clk,
+	[3] = &extal2_clk,
+};
+
+static struct clk *m4_parents[2] = {
+	[0] = &pll2s_clk,
+};
+
+static struct clk *hsi_parents[4] = {
+	[0] = &pll2h_clk,
+	[1] = &pll1_div2_clk,
+	[3] = &pll2s_clk,
+};
+
+/*** FIXME ***
+ * SH_CLK_DIV6_EXT() macro doesn't care .mapping
+ * but, it is necessary on R-Car (= ioremap() base CPG)
+ * The difference between
+ * SH_CLK_DIV6_EXT() <--> SH_CLK_MAP_DIV6_EXT()
+ * is only .mapping
+ */
+#define SH_CLK_MAP_DIV6_EXT(_reg, _flags, _parents,			\
+			    _num_parents, _src_shift, _src_width)	\
+{									\
+	.enable_reg	= (void __iomem *)_reg,				\
+	.enable_bit	= 0, /* unused */				\
+	.flags		= _flags | CLK_MASK_DIV_ON_DISABLE,		\
+	.div_mask	= SH_CLK_DIV6_MSK,				\
+	.parent_table	= _parents,					\
+	.parent_num	= _num_parents,					\
+	.src_shift	= _src_shift,					\
+	.src_width	= _src_width,					\
+	.mapping	= &cpg_mapping,					\
+}
+
+static struct clk div6_clks[DIV6_NR] = {
+	[DIV6_ZB] = SH_CLK_MAP_DIV6_EXT(ZBCKCR, CLK_ENABLE_ON_INIT,
+				div6_parents, 2, 7, 1),
+	[DIV6_SDHI0] = SH_CLK_MAP_DIV6_EXT(SD0CKCR, 0,
+				div6_parents, 2, 6, 2),
+	[DIV6_SDHI1] = SH_CLK_MAP_DIV6_EXT(SD1CKCR, 0,
+				div6_parents, 2, 6, 2),
+	[DIV6_SDHI2] = SH_CLK_MAP_DIV6_EXT(SD2CKCR, 0,
+				div6_parents, 2, 6, 2),
+	[DIV6_MMC0] = SH_CLK_MAP_DIV6_EXT(MMC0CKCR, 0,
+				div6_parents, 2, 6, 2),
+	[DIV6_MMC1] = SH_CLK_MAP_DIV6_EXT(MMC1CKCR, 0,
+				div6_parents, 2, 6, 2),
+	[DIV6_VCK1] = SH_CLK_MAP_DIV6_EXT(VCLKCR1, 0, /* didn't care bit[6-7] */
+				div6_parents, ARRAY_SIZE(div6_parents), 12, 3),
+	[DIV6_VCK2] = SH_CLK_MAP_DIV6_EXT(VCLKCR2, 0, /* didn't care bit[6-7] */
+				div6_parents, ARRAY_SIZE(div6_parents), 12, 3),
+	[DIV6_VCK3] = SH_CLK_MAP_DIV6_EXT(VCLKCR3, 0, /* didn't care bit[6-7] */
+				div6_parents, ARRAY_SIZE(div6_parents), 12, 3),
+	[DIV6_VCK4] = SH_CLK_MAP_DIV6_EXT(VCLKCR4, 0, /* didn't care bit[6-7] */
+				div6_parents, ARRAY_SIZE(div6_parents), 12, 3),
+	[DIV6_VCK5] = SH_CLK_MAP_DIV6_EXT(VCLKCR5, 0, /* didn't care bit[6-7] */
+				div6_parents, ARRAY_SIZE(div6_parents), 12, 3),
+	[DIV6_FSIA] = SH_CLK_MAP_DIV6_EXT(FSIACKCR, 0,
+				fsia_parents, ARRAY_SIZE(fsia_parents), 6, 2),
+	[DIV6_FSIB] = SH_CLK_MAP_DIV6_EXT(FSIBCKCR, 0,
+				fsib_parents, ARRAY_SIZE(fsib_parents), 6, 2),
+	[DIV6_MP] = SH_CLK_MAP_DIV6_EXT(MPCKCR, 0, /* it needs bit[9-11] control */
+				mp_parents, ARRAY_SIZE(mp_parents), 6, 2),
+	/* pll2s will be selected always for M4 */
+	[DIV6_M4] = SH_CLK_MAP_DIV6_EXT(M4CKCR, 0, /* it needs bit[9] control */
+				m4_parents, ARRAY_SIZE(m4_parents), 6, 1),
+	[DIV6_HSI] = SH_CLK_MAP_DIV6_EXT(HSICKCR, 0, /* it needs bit[9] control */
+				hsi_parents, ARRAY_SIZE(hsi_parents), 6, 2),
+	[DIV6_SPUV] = SH_CLK_MAP_DIV6_EXT(SPUVCKCR, 0,
+				mp_parents, ARRAY_SIZE(mp_parents), 6, 2),
+};
+
 /* MSTP */
 enum {
 	MSTP217, MSTP216, MSTP207, MSTP206, MSTP204, MSTP203,
@@ -230,12 +359,12 @@ enum {
 };
 
 static struct clk mstp_clks[MSTP_NR] = {
-	[MSTP204] = SH_CLK_MSTP32(&extal2_clk, SMSTPCR2, 4, 0), /* SCIFA0 */
-	[MSTP203] = SH_CLK_MSTP32(&extal2_clk, SMSTPCR2, 3, 0), /* SCIFA1 */
-	[MSTP206] = SH_CLK_MSTP32(&extal2_clk, SMSTPCR2, 6, 0), /* SCIFB0 */
-	[MSTP207] = SH_CLK_MSTP32(&extal2_clk, SMSTPCR2, 7, 0), /* SCIFB1 */
-	[MSTP216] = SH_CLK_MSTP32(&extal2_clk, SMSTPCR2, 16, 0), /* SCIFB2 */
-	[MSTP217] = SH_CLK_MSTP32(&extal2_clk, SMSTPCR2, 17, 0), /* SCIFB3 */
+	[MSTP204] = SH_CLK_MSTP32(&div6_clks[DIV6_MP],	SMSTPCR2, 4, 0), /* SCIFA0 */
+	[MSTP203] = SH_CLK_MSTP32(&div6_clks[DIV6_MP],	SMSTPCR2, 3, 0), /* SCIFA1 */
+	[MSTP206] = SH_CLK_MSTP32(&div6_clks[DIV6_MP],	SMSTPCR2, 6, 0), /* SCIFB0 */
+	[MSTP207] = SH_CLK_MSTP32(&div6_clks[DIV6_MP],	SMSTPCR2, 7, 0), /* SCIFB1 */
+	[MSTP216] = SH_CLK_MSTP32(&div6_clks[DIV6_MP],	SMSTPCR2, 16, 0), /* SCIFB2 */
+	[MSTP217] = SH_CLK_MSTP32(&div6_clks[DIV6_MP],	SMSTPCR2, 17, 0), /* SCIFB3 */
 	[MSTP522] = SH_CLK_MSTP32(&extal2_clk, SMSTPCR5, 22, 0), /* Thermal */
 };
 
@@ -246,6 +375,8 @@ static struct clk_lookup lookups[] = {
 	CLKDEV_CON_ID("extal2",			&extal2_clk),
 	CLKDEV_CON_ID("extal2_div2",		&extal2_div2_clk),
 	CLKDEV_CON_ID("extal2_div4",		&extal2_div4_clk),
+	CLKDEV_CON_ID("fsiack",			&fsiack_clk),
+	CLKDEV_CON_ID("fsibck",			&fsibck_clk),
 
 	/* pll clock */
 	CLKDEV_CON_ID("pll1",			&pll1_clk),
@@ -254,6 +385,25 @@ static struct clk_lookup lookups[] = {
 	CLKDEV_CON_ID("pll2s",			&pll2s_clk),
 	CLKDEV_CON_ID("pll2h",			&pll2h_clk),
 
+	/* DIV6 */
+	CLKDEV_CON_ID("zb",			&div6_clks[DIV6_ZB]),
+	CLKDEV_CON_ID("sdhi0",			&div6_clks[DIV6_SDHI0]),
+	CLKDEV_CON_ID("sdhi1",			&div6_clks[DIV6_SDHI1]),
+	CLKDEV_CON_ID("sdhi2",			&div6_clks[DIV6_SDHI2]),
+	CLKDEV_CON_ID("mmc0",			&div6_clks[DIV6_MMC0]),
+	CLKDEV_CON_ID("mmc1",			&div6_clks[DIV6_MMC1]),
+	CLKDEV_CON_ID("vck1",			&div6_clks[DIV6_VCK1]),
+	CLKDEV_CON_ID("vck2",			&div6_clks[DIV6_VCK2]),
+	CLKDEV_CON_ID("vck3",			&div6_clks[DIV6_VCK3]),
+	CLKDEV_CON_ID("vck4",			&div6_clks[DIV6_VCK4]),
+	CLKDEV_CON_ID("vck5",			&div6_clks[DIV6_VCK5]),
+	CLKDEV_CON_ID("fsia",			&div6_clks[DIV6_FSIA]),
+	CLKDEV_CON_ID("fsib",			&div6_clks[DIV6_FSIB]),
+	CLKDEV_CON_ID("mp",			&div6_clks[DIV6_MP]),
+	CLKDEV_CON_ID("m4",			&div6_clks[DIV6_M4]),
+	CLKDEV_CON_ID("hsi",			&div6_clks[DIV6_HSI]),
+	CLKDEV_CON_ID("spuv",			&div6_clks[DIV6_SPUV]),
+
 	/* MSTP */
 	CLKDEV_DEV_ID("sh-sci.0", &mstp_clks[MSTP204]),
 	CLKDEV_DEV_ID("sh-sci.1", &mstp_clks[MSTP203]),
@@ -269,19 +419,10 @@ static struct clk_lookup lookups[] = {
 
 void __init r8a73a4_clock_init(void)
 {
-	void __iomem *cpg_base, *reg;
+	void __iomem *reg;
 	int k, ret = 0;
 	u32 ckscr;
 
-	/* fix MPCLK to EXTAL2 for now.
-	 * this is needed until more detailed clock topology is supported
-	 */
-	cpg_base = ioremap_nocache(CPG_BASE, CPG_LEN);
-	BUG_ON(!cpg_base);
-	reg = cpg_base + (MPCKCR - CPG_BASE);
-	iowrite32(ioread32(reg) | 1 << 7 | 0x0c, reg); /* set CKSEL */
-	iounmap(cpg_base);
-
 	reg = ioremap_nocache(CKSCR, PAGE_SIZE);
 	BUG_ON(!reg);
 	ckscr = ioread32(reg);
@@ -312,6 +453,9 @@ void __init r8a73a4_clock_init(void)
 		ret = sh_clk_div4_register(div4_clks, DIV4_NR, &div4_table);
 
 	if (!ret)
+		ret = sh_clk_div6_reparent_register(div6_clks, DIV6_NR);
+
+	if (!ret)
 		ret = sh_clk_mstp_register(mstp_clks, MSTP_NR);
 
 	clkdev_add_table(lookups, ARRAY_SIZE(lookups));
-- 
1.7.9.5

[PATCH 5/6] ARM: shmobile: r8a73a4: add MMCIF and SDHI DT templates

[Guennadi Liakhovetski]

This adds DT templates for all MMCIF and SDHI controllers on r8a73a4.
They are added with status="disabled". To use them platform-specific
DTs have to enable the required ones.

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

diff --git a/arch/arm/boot/dts/r8a73a4.dtsi b/arch/arm/boot/dts/r8a73a4.dtsi
index 4e1ddf0..064f045 100644
--- a/arch/arm/boot/dts/r8a73a4.dtsi
+++ b/arch/arm/boot/dts/r8a73a4.dtsi
@@ -166,4 +166,49 @@
 		interrupt-parent = <&gic>;
 		interrupts = <0 173 0x4>;
 	};
+
+	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 5/6] ARM: shmobile: r8a73a4: add MMCIF and SDHI DT templates

[Magnus Damm]

Hi Guennadi,

On Fri, Jul 5, 2013 at 5:38 AM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> This adds DT templates for all MMCIF and SDHI controllers on r8a73a4.
> They are added with status="disabled". To use them platform-specific
> DTs have to enable the required ones.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
> ---
>  arch/arm/boot/dts/r8a73a4.dtsi |   45 ++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 45 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/boot/dts/r8a73a4.dtsi b/arch/arm/boot/dts/r8a73a4.dtsi
> index 4e1ddf0..064f045 100644
> --- a/arch/arm/boot/dts/r8a73a4.dtsi
> +++ b/arch/arm/boot/dts/r8a73a4.dtsi
> @@ -166,4 +166,49 @@
>                 interrupt-parent = <&gic>;
>                 interrupts = <0 173 0x4>;
>         };
> +
> +       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";
> +       };

I don't think the size of the MMCIF I/O memory resource is correct,
please check with the data sheet and fix.

> +       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";
> +       };

And now we're here again using "r8a7740" for SDHI on r8a73a4.

I don't think you have full documentation for the SDHI block, do you?
So please don't assume it is compatible with r8a7740 just because it
happens to work for you.

Cheers,

/ magnus

Re: [PATCH 5/6] ARM: shmobile: r8a73a4: add MMCIF and SDHI DT templates

[Guennadi Liakhovetski]

On Fri, 5 Jul 2013, Magnus Damm wrote:

> Hi Guennadi,
> 
> On Fri, Jul 5, 2013 at 5:38 AM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:

[snip]

> > +       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";
> > +       };
> 
> And now we're here again using "r8a7740" for SDHI on r8a73a4.

Would this be ok:

		compatible = "renesas,r8a73a4-sdhi", "renesas,r8a7740-sdhi";

Thanks
Guennadi

> I don't think you have full documentation for the SDHI block, do you?
> So please don't assume it is compatible with r8a7740 just because it
> happens to work for you.

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

Re: [PATCH 5/6] ARM: shmobile: r8a73a4: add MMCIF and SDHI DT templates

[Magnus Damm]

Hi Guennadi,

On Fri, Jul 5, 2013 at 5:41 PM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> On Fri, 5 Jul 2013, Magnus Damm wrote:
>
>> Hi Guennadi,
>>
>> On Fri, Jul 5, 2013 at 5:38 AM, Guennadi Liakhovetski
>> <g.liakhovetski@gmx.de> wrote:
>
> [snip]
>
>> > +       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";
>> > +       };
>>
>> And now we're here again using "r8a7740" for SDHI on r8a73a4.
>
> Would this be ok:
>
>                 compatible = "renesas,r8a73a4-sdhi", "renesas,r8a7740-sdhi";

At least it is better. Thanks.

I prefer "renesas,r8a73a4-sdhi" only, but the above is acceptable to
me as long as we remember to add "renesas,r8a73a4-sdhi" to the SDHI
driver that keeps old behaviour before updating "renesas,r8a7740-sdhi"
with features that may break r8a73a4.

Maybe we should simply add per-soc bindings to the driver to avoid
future potential issues?

Thanks,

/ magnus

Re: [PATCH 5/6] ARM: shmobile: r8a73a4: add MMCIF and SDHI DT templates

[Guennadi Liakhovetski]

Hi Magnus

On Mon, 8 Jul 2013, Magnus Damm wrote:

> Hi Guennadi,
> 
> On Fri, Jul 5, 2013 at 5:41 PM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> > On Fri, 5 Jul 2013, Magnus Damm wrote:
> >
> >> Hi Guennadi,
> >>
> >> On Fri, Jul 5, 2013 at 5:38 AM, Guennadi Liakhovetski
> >> <g.liakhovetski@gmx.de> wrote:
> >
> > [snip]
> >
> >> > +       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";
> >> > +       };
> >>
> >> And now we're here again using "r8a7740" for SDHI on r8a73a4.
> >
> > Would this be ok:
> >
> >                 compatible = "renesas,r8a73a4-sdhi", "renesas,r8a7740-sdhi";
> 
> At least it is better. Thanks.
> 
> I prefer "renesas,r8a73a4-sdhi" only, but the above is acceptable to
> me as long as we remember to add "renesas,r8a73a4-sdhi" to the SDHI
> driver that keeps old behaviour before updating "renesas,r8a7740-sdhi"
> with features that may break r8a73a4.

I don't think that would make sense. The whole purpose of adding the 
compatible string to DT was to avoid touching the driver _unless_ needed. 
I.e. unless we do know SDHI on the new SoC has a feature that we want to 
support and we know, that all DTs out there contain the required 
compatibility string, so, now we can just hook to it in the driver and all 
platforms will start using that feature magically. If you anyway want to 
add _every_ new SoC compatibility to _every_ related driver, I don't think 
using two compatibility strings would make sense.

And no, I don't think "omap2," "omap3," "omap4" strings in OMAP drivers is 
a good example - as you know, those are SoC classes, not specific SoC 
versions. Using "generation 5," "generation 6" etc., or whatever you would 
prefer to call them, might be acceptable, but I'd really prefer to avoid 
adding every SoC to every driver.

Another option would certainly be to bite the bullet and admit, that going 
the compatibility string way was a mistake and add a binding for the SDHI 
wait-idle feature and keep both in the driver. That way we add redundancy 
to the driver - at least until we decide to finally remove the 
compatibility string. But at least we can transfer all mainline platforms 
to use the binding parameter.

> Maybe we should simply add per-soc bindings to the driver to avoid
> future potential issues?

A real binding - not a compatibility string? No, don't think I like this 
either.

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

Re: [PATCH 5/6] ARM: shmobile: r8a73a4: add MMCIF and SDHI DT templates

[Magnus Damm]

Hi Guenandi,

On Mon, Jul 8, 2013 at 4:06 PM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> Hi Magnus
>
> On Mon, 8 Jul 2013, Magnus Damm wrote:
>
>> Hi Guennadi,
>>
>> On Fri, Jul 5, 2013 at 5:41 PM, Guennadi Liakhovetski
>> <g.liakhovetski@gmx.de> wrote:
>> > On Fri, 5 Jul 2013, Magnus Damm wrote:
>> >
>> >> Hi Guennadi,
>> >>
>> >> On Fri, Jul 5, 2013 at 5:38 AM, Guennadi Liakhovetski
>> >> <g.liakhovetski@gmx.de> wrote:
>> >
>> > [snip]
>> >
>> >> > +       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";
>> >> > +       };
>> >>
>> >> And now we're here again using "r8a7740" for SDHI on r8a73a4.
>> >
>> > Would this be ok:
>> >
>> >                 compatible = "renesas,r8a73a4-sdhi", "renesas,r8a7740-sdhi";
>>
>> At least it is better. Thanks.
>>
>> I prefer "renesas,r8a73a4-sdhi" only, but the above is acceptable to
>> me as long as we remember to add "renesas,r8a73a4-sdhi" to the SDHI
>> driver that keeps old behaviour before updating "renesas,r8a7740-sdhi"
>> with features that may break r8a73a4.
>
> I don't think that would make sense. The whole purpose of adding the
> compatible string to DT was to avoid touching the driver _unless_ needed.

So instead of using DT to describe hardware we should use it to cut
slack from a software implementation point of view?

> I.e. unless we do know SDHI on the new SoC has a feature that we want to
> support and we know, that all DTs out there contain the required
> compatibility string, so, now we can just hook to it in the driver and all
> platforms will start using that feature magically. If you anyway want to
> add _every_ new SoC compatibility to _every_ related driver, I don't think
> using two compatibility strings would make sense.

I think you know what I think, and that is using a per-SoC
compatibility string is stupid since it has nothing to do with SoC.
But if we should go down that route, why don't we do it properly at
least?

I would like to know how you think the following case should be handled:

1. On r8a73a4 without documentation you introduce the following line in the DTS
  compatible = "renesas,r8a73a4-sdhi", "renesas,r8a7740-sdhi";

2. After this you happen to receive documentation about r8a7740 SDHI.

3. You update the SDHI driver with r8a7740 specific new features.

4. r8a73a4 now breaks unless you remember to also add "r8a73a4-sdhi"

This turns into a dependency mess IMO.

> And no, I don't think "omap2," "omap3," "omap4" strings in OMAP drivers is
> a good example - as you know, those are SoC classes, not specific SoC
> versions. Using "generation 5," "generation 6" etc., or whatever you would
> prefer to call them, might be acceptable, but I'd really prefer to avoid
> adding every SoC to every driver.

Yes, me too. So how the hell did the bindings turn into this state?

> Another option would certainly be to bite the bullet and admit, that going
> the compatibility string way was a mistake and add a binding for the SDHI
> wait-idle feature and keep both in the driver. That way we add redundancy
> to the driver - at least until we decide to finally remove the
> compatibility string. But at least we can transfer all mainline platforms
> to use the binding parameter.

Now since we for some unknown reason started using SoC in the
compatibility string I think we have to bite the bullet and keep on
updating the SoCs with proper information. To avoid the mess listed in
1 + 2 +3 + 4 above.

>> Maybe we should simply add per-soc bindings to the driver to avoid
>> future potential issues?
>
> A real binding - not a compatibility string? No, don't think I like this
> either.

So how do you propose that we handle this then? We have already
committed to per-SoC compatibility strings...

/ magnus

Re: [PATCH 5/6] ARM: shmobile: r8a73a4: add MMCIF and SDHI DT templates

[Guennadi Liakhovetski]

Hi Magnus

On Mon, 8 Jul 2013, Magnus Damm wrote:

> Hi Guenandi,
> 
> On Mon, Jul 8, 2013 at 4:06 PM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> > Hi Magnus
> >
> > On Mon, 8 Jul 2013, Magnus Damm wrote:
> >
> >> Hi Guennadi,
> >>
> >> On Fri, Jul 5, 2013 at 5:41 PM, Guennadi Liakhovetski
> >> <g.liakhovetski@gmx.de> wrote:
> >> > On Fri, 5 Jul 2013, Magnus Damm wrote:
> >> >
> >> >> Hi Guennadi,
> >> >>
> >> >> On Fri, Jul 5, 2013 at 5:38 AM, Guennadi Liakhovetski
> >> >> <g.liakhovetski@gmx.de> wrote:
> >> >
> >> > [snip]
> >> >
> >> >> > +       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";
> >> >> > +       };
> >> >>
> >> >> And now we're here again using "r8a7740" for SDHI on r8a73a4.
> >> >
> >> > Would this be ok:
> >> >
> >> >                 compatible = "renesas,r8a73a4-sdhi", "renesas,r8a7740-sdhi";
> >>
> >> At least it is better. Thanks.
> >>
> >> I prefer "renesas,r8a73a4-sdhi" only, but the above is acceptable to
> >> me as long as we remember to add "renesas,r8a73a4-sdhi" to the SDHI
> >> driver that keeps old behaviour before updating "renesas,r8a7740-sdhi"
> >> with features that may break r8a73a4.
> >
> > I don't think that would make sense. The whole purpose of adding the
> > compatible string to DT was to avoid touching the driver _unless_ needed.
> 
> So instead of using DT to describe hardware we should use it to cut
> slack from a software implementation point of view?

Sorry, don't understand how this is cutting slack. We do use that string, 
so, DT does describe the hardware. Only I don't understand why we shall 
unconditionally add these strings to the drivers, even when no action 
should be taken there?

> > I.e. unless we do know SDHI on the new SoC has a feature that we want to
> > support and we know, that all DTs out there contain the required
> > compatibility string, so, now we can just hook to it in the driver and all
> > platforms will start using that feature magically. If you anyway want to
> > add _every_ new SoC compatibility to _every_ related driver, I don't think
> > using two compatibility strings would make sense.
> 
> I think you know what I think, and that is using a per-SoC
> compatibility string is stupid since it has nothing to do with SoC.
> But if we should go down that route, why don't we do it properly at
> least?
> 
> I would like to know how you think the following case should be handled:
> 
> 1. On r8a73a4 without documentation you introduce the following line in the DTS
>   compatible = "renesas,r8a73a4-sdhi", "renesas,r8a7740-sdhi";
> 
> 2. After this you happen to receive documentation about r8a7740 SDHI.
> 
> 3. You update the SDHI driver with r8a7740 specific new features.
> 
> 4. r8a73a4 now breaks unless you remember to also add "r8a73a4-sdhi"

Again - how is this different from your proposal? Let's say these SoCs 
have a common feature A (idle-wait) and then you discover a new feature B 
on r8a7740 only. With your proposal we have an entry for r8a73a4 in the 
driver, which does nothing. Now, after updating the driver to handle B on 
r8a7740 you also have to remember to extend your r8a73a4 handling to 
disable B there. So, it is just exactly the same, the only difference is, 
with my approach you first add that r8a73a4 compatibility entry when you 
discover the need, with your approach you already have it there in the 
driver as an empty entry, and you anyway have to update it to disable B, 
when it is discovered.

Thanks
Guennadi

> This turns into a dependency mess IMO.
> 
> > And no, I don't think "omap2," "omap3," "omap4" strings in OMAP drivers is
> > a good example - as you know, those are SoC classes, not specific SoC
> > versions. Using "generation 5," "generation 6" etc., or whatever you would
> > prefer to call them, might be acceptable, but I'd really prefer to avoid
> > adding every SoC to every driver.
> 
> Yes, me too. So how the hell did the bindings turn into this state?
> 
> > Another option would certainly be to bite the bullet and admit, that going
> > the compatibility string way was a mistake and add a binding for the SDHI
> > wait-idle feature and keep both in the driver. That way we add redundancy
> > to the driver - at least until we decide to finally remove the
> > compatibility string. But at least we can transfer all mainline platforms
> > to use the binding parameter.
> 
> Now since we for some unknown reason started using SoC in the
> compatibility string I think we have to bite the bullet and keep on
> updating the SoCs with proper information. To avoid the mess listed in
> 1 + 2 +3 + 4 above.
> 
> >> Maybe we should simply add per-soc bindings to the driver to avoid
> >> future potential issues?
> >
> > A real binding - not a compatibility string? No, don't think I like this
> > either.
> 
> So how do you propose that we handle this then? We have already
> committed to per-SoC compatibility strings...
> 
> / magnus
> 

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

Re: [PATCH 5/6] ARM: shmobile: r8a73a4: add MMCIF and SDHI DT templates

[Magnus Damm]

Hi Guennadi,

On Mon, Jul 8, 2013 at 5:57 PM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> Hi Magnus
>
> On Mon, 8 Jul 2013, Magnus Damm wrote:
>
>> Hi Guenandi,
>>
>> On Mon, Jul 8, 2013 at 4:06 PM, Guennadi Liakhovetski
>> <g.liakhovetski@gmx.de> wrote:
>> > Hi Magnus
>> >
>> > On Mon, 8 Jul 2013, Magnus Damm wrote:
>> >
>> >> Hi Guennadi,
>> >>
>> >> On Fri, Jul 5, 2013 at 5:41 PM, Guennadi Liakhovetski
>> >> <g.liakhovetski@gmx.de> wrote:
>> >> > On Fri, 5 Jul 2013, Magnus Damm wrote:
>> >> >
>> >> >> Hi Guennadi,
>> >> >>
>> >> >> On Fri, Jul 5, 2013 at 5:38 AM, Guennadi Liakhovetski
>> >> >> <g.liakhovetski@gmx.de> wrote:
>> >> >
>> >> > [snip]
>> >> >
>> >> >> > +       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";
>> >> >> > +       };
>> >> >>
>> >> >> And now we're here again using "r8a7740" for SDHI on r8a73a4.
>> >> >
>> >> > Would this be ok:
>> >> >
>> >> >                 compatible = "renesas,r8a73a4-sdhi", "renesas,r8a7740-sdhi";
>> >>
>> >> At least it is better. Thanks.
>> >>
>> >> I prefer "renesas,r8a73a4-sdhi" only, but the above is acceptable to
>> >> me as long as we remember to add "renesas,r8a73a4-sdhi" to the SDHI
>> >> driver that keeps old behaviour before updating "renesas,r8a7740-sdhi"
>> >> with features that may break r8a73a4.
>> >
>> > I don't think that would make sense. The whole purpose of adding the
>> > compatible string to DT was to avoid touching the driver _unless_ needed.
>>
>> So instead of using DT to describe hardware we should use it to cut
>> slack from a software implementation point of view?
>
> Sorry, don't understand how this is cutting slack. We do use that string,
> so, DT does describe the hardware. Only I don't understand why we shall
> unconditionally add these strings to the drivers, even when no action
> should be taken there?

Well, at the time of writing the DTS, do you know if action is needed or not?

So using r8a7740 looks to me like it's an assumption about how r8a7740
software support happen to be at some certain point in time. It is not
guaranteed that r8a7740 compatibility would be kept in the future,
basically because we don't know how r8a7740 support is going to
evolve. So it's basically describing a certain software state IMO. I
want it to describe hardware instead.

I would be fine it the string was like this UART example:
compatible = "vendor,16550", "vendor,8250";

In the above example with UART we know that 16550 is backwards
compatible with 8250. So if the 16550 driver happens to be disabled in
the kernel then it should be possible to use the 8250 driver. In this
case the register layout and the documentation should point out that
they are compatible.

As a contrast, I believe the following are true for SDHI
- We have no decent documentation - neither for r8a7740 or r8a73a4
- r8a73a4 is most likely not in a parent-child relation ship with r8a7740

So I prefer not to make any assumptions to avoid future headache.

>> > I.e. unless we do know SDHI on the new SoC has a feature that we want to
>> > support and we know, that all DTs out there contain the required
>> > compatibility string, so, now we can just hook to it in the driver and all
>> > platforms will start using that feature magically. If you anyway want to
>> > add _every_ new SoC compatibility to _every_ related driver, I don't think
>> > using two compatibility strings would make sense.
>>
>> I think you know what I think, and that is using a per-SoC
>> compatibility string is stupid since it has nothing to do with SoC.
>> But if we should go down that route, why don't we do it properly at
>> least?
>>
>> I would like to know how you think the following case should be handled:
>>
>> 1. On r8a73a4 without documentation you introduce the following line in the DTS
>>   compatible = "renesas,r8a73a4-sdhi", "renesas,r8a7740-sdhi";
>>
>> 2. After this you happen to receive documentation about r8a7740 SDHI.
>>
>> 3. You update the SDHI driver with r8a7740 specific new features.
>>
>> 4. r8a73a4 now breaks unless you remember to also add "r8a73a4-sdhi"
>
> Again - how is this different from your proposal? Let's say these SoCs
> have a common feature A (idle-wait) and then you discover a new feature B
> on r8a7740 only. With your proposal we have an entry for r8a73a4 in the
> driver, which does nothing. Now, after updating the driver to handle B on
> r8a7740 you also have to remember to extend your r8a73a4 handling to
> disable B there. So, it is just exactly the same, the only difference is,
> with my approach you first add that r8a73a4 compatibility entry when you
> discover the need, with your approach you already have it there in the
> driver as an empty entry, and you anyway have to update it to disable B,
> when it is discovered.

I'm not sure what you mean with"my proposal", however..

Above I don't understand why you have to extend r8a73a4 handling to
disable B. From my point of view, when you update r8a7740 with feature
B you do it with a feature flag that only applies to r8a7740. When
updating r8a7740 you leave that particular feature flag not set on
r8a73a4. So r8a73a4 does not have to be touched.

/ magnus