All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC] clk: shmobile: r8a7795: Add SD divider support
@ 2016-01-22 12:26 Dirk Behme
  2016-01-25  8:59 ` Geert Uytterhoeven
                   ` (8 more replies)
  0 siblings, 9 replies; 10+ messages in thread
From: Dirk Behme @ 2016-01-22 12:26 UTC (permalink / raw)
  To: linux-sh

From: Takeshi Kihara <takeshi.kihara.df@renesas.com>

This patch adds SD[0..3] clock divider support for R-Car Gen3 SoC.

Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com>
Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
---
Notes:

* This is based on https://git.kernel.org/cgit/linux/kernel/git/horms/renesas-bsp.git/commit/drivers/clk/shmobile/clk-rcar-gen3.c?h=v4.2/rcar-3.0.x&idÍ10385afc15cef6bfbaea4aa5da41193b24fe82

* This is compile tested *only*. The intention is to get some feedback
  if this is the way we want to go.

 drivers/clk/shmobile/r8a7795-cpg-mssr.c | 248 ++++++++++++++++++++++++++++++++
 drivers/clk/shmobile/renesas-cpg-mssr.h |   2 +
 2 files changed, 250 insertions(+)

diff --git a/drivers/clk/shmobile/r8a7795-cpg-mssr.c b/drivers/clk/shmobile/r8a7795-cpg-mssr.c
index 23e98fe..a7ac1b0 100644
--- a/drivers/clk/shmobile/r8a7795-cpg-mssr.c
+++ b/drivers/clk/shmobile/r8a7795-cpg-mssr.c
@@ -20,6 +20,7 @@
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/of.h>
+#include <linux/slab.h>
 
 #include <dt-bindings/clock/r8a7795-cpg-mssr.h>
 
@@ -61,6 +62,7 @@ enum r8a7795_clk_types {
 	CLK_TYPE_GEN3_PLL2,
 	CLK_TYPE_GEN3_PLL3,
 	CLK_TYPE_GEN3_PLL4,
+	CLK_TYPE_GEN3_SD,
 };
 
 static const struct cpg_core_clk r8a7795_core_clks[] __initconst = {
@@ -99,6 +101,12 @@ static const struct cpg_core_clk r8a7795_core_clks[] __initconst = {
 	DEF_FIXED("s3d1",       R8A7795_CLK_S3D1,  CLK_S3,         1, 1),
 	DEF_FIXED("s3d2",       R8A7795_CLK_S3D2,  CLK_S3,         2, 1),
 	DEF_FIXED("s3d4",       R8A7795_CLK_S3D4,  CLK_S3,         4, 1),
+
+	DEF_SD("sd0",           R8A7795_CLK_SD0,   CLK_PLL1_DIV2, 0x0074),
+	DEF_SD("sd1",           R8A7795_CLK_SD1,   CLK_PLL1_DIV2, 0x0078),
+	DEF_SD("sd2",           R8A7795_CLK_SD2,   CLK_PLL1_DIV2, 0x0268),
+	DEF_SD("sd3",           R8A7795_CLK_SD3,   CLK_PLL1_DIV2, 0x026c),
+
 	DEF_FIXED("cl",         R8A7795_CLK_CL,    CLK_PLL1_DIV2, 48, 1),
 	DEF_FIXED("cp",         R8A7795_CLK_CP,    CLK_EXTAL,      2, 1),
 
@@ -199,6 +207,243 @@ static const unsigned int r8a7795_crit_mod_clks[] __initconst = {
 	MOD_CLK_ID(408),	/* INTC-AP (GIC) */
 };
 
+/* -----------------------------------------------------------------------------
+ * SDn Clock
+ *
+ */
+#define CPG_SD_STP_N_HCK	BIT(9)
+#define CPG_SD_STP_N_CK		BIT(8)
+#define CPG_SD_SD_N_SRCFC_MASK	(0x7 << CPG_SD_SD_N_SRCFC_SHIFT)
+#define CPG_SD_SD_N_SRCFC_SHIFT	2
+#define CPG_SD_SD_N_FC_MASK	(0x3 << CPG_SD_SD_N_FC_SHIFT)
+#define CPG_SD_SD_N_FC_SHIFT	0
+
+#define CPG_SD_STP_MASK		(CPG_SD_STP_N_HCK | CPG_SD_STP_N_CK)
+#define CPG_SD_FC_MASK		(CPG_SD_SD_N_SRCFC_MASK | CPG_SD_SD_N_FC_MASK)
+
+/* CPG_SD_DIV_TABLE_DATA(stp_n_hck, stp_n_ck, sd_n_srcfc, sd_n_fc, div) */
+#define CPG_SD_DIV_TABLE_DATA(_a, _b, _c, _d, _e) \
+{ \
+	.val = ((_a) ? CPG_SD_STP_N_HCK : 0) | \
+	       ((_b) ? CPG_SD_STP_N_CK : 0) | \
+	       (((_c) << CPG_SD_SD_N_SRCFC_SHIFT) & CPG_SD_SD_N_SRCFC_MASK) | \
+	       (((_d) << CPG_SD_SD_N_FC_SHIFT) & CPG_SD_SD_N_FC_MASK), \
+	.div = (_e), \
+}
+
+struct sd_div_table {
+	u32 val;
+	unsigned int div;
+};
+
+struct sd_clock {
+	struct clk_hw hw;
+	void __iomem *reg;
+	const struct sd_div_table *div_table;
+	int div_num;
+	unsigned int div_min;
+	unsigned int div_max;
+};
+
+/* SDn divider
+ *                     sd_n_srcfc sd_n_fc   div
+ * stp_n_hck stp_n_ck  (div)      (div)     = sd_n_srcfc x sd_n_fc
+ *-------------------------------------------------------------------
+ *  0         0         0 (1)      1 (4)      4
+ *  0         0         1 (2)      1 (4)      8
+ *  1         0         2 (4)      1 (4)     16
+ *  1         0         3 (8)      1 (4)     32
+ *  1         0         4 (16)     1 (4)     64
+ *  0         0         0 (1)      0 (2)      2
+ *  0         0         1 (2)      0 (2)      4
+ *  1         0         2 (4)      0 (2)      8
+ *  1         0         3 (8)      0 (2)     16
+ *  1         0         4 (16)     0 (2)     32
+ */
+static const struct sd_div_table cpg_sd_div_table[] = {
+/*	CPG_SD_DIV_TABLE_DATA(stp_n_hck, stp_n_ck, sd_n_srcfc, sd_n_fc, div) */
+	CPG_SD_DIV_TABLE_DATA(0,        0,        0,          1,        4),
+	CPG_SD_DIV_TABLE_DATA(0,        0,        1,          1,        8),
+	CPG_SD_DIV_TABLE_DATA(1,        0,        2,          1,       16),
+	CPG_SD_DIV_TABLE_DATA(1,        0,        3,          1,       32),
+	CPG_SD_DIV_TABLE_DATA(1,        0,        4,          1,       64),
+	CPG_SD_DIV_TABLE_DATA(0,        0,        0,          0,        2),
+	CPG_SD_DIV_TABLE_DATA(0,        0,        1,          0,        4),
+	CPG_SD_DIV_TABLE_DATA(1,        0,        2,          0,        8),
+	CPG_SD_DIV_TABLE_DATA(1,        0,        3,          0,       16),
+	CPG_SD_DIV_TABLE_DATA(1,        0,        4,          0,       32),
+};
+
+#define to_sd_clock(_hw) container_of(_hw, struct sd_clock, hw)
+
+static int cpg_sd_clock_endisable(struct clk_hw *hw, bool enable)
+{
+	struct sd_clock *clock = to_sd_clock(hw);
+	u32 val, sd_fc;
+	int i;
+
+	val = clk_readl(clock->reg);
+
+	if (enable) {
+		sd_fc = val & CPG_SD_FC_MASK;
+		for (i = 0; i < clock->div_num; i++)
+			if (sd_fc = (clock->div_table[i].val & CPG_SD_FC_MASK))
+				break;
+
+		if (i >= clock->div_num) {
+			pr_err("%s: 0x%4x is not support of division ratio.\n",
+				__func__, sd_fc);
+			return -ENODATA;
+		}
+
+		val &= ~(CPG_SD_STP_MASK);
+		val |= clock->div_table[i].val & CPG_SD_STP_MASK;
+	} else
+		val |= CPG_SD_STP_MASK;
+
+	clk_writel(val, clock->reg);
+
+	return 0;
+}
+
+static int cpg_sd_clock_enable(struct clk_hw *hw)
+{
+	return cpg_sd_clock_endisable(hw, true);
+}
+
+static void cpg_sd_clock_disable(struct clk_hw *hw)
+{
+	cpg_sd_clock_endisable(hw, false);
+}
+
+static int cpg_sd_clock_is_enabled(struct clk_hw *hw)
+{
+	struct sd_clock *clock = to_sd_clock(hw);
+
+	return !(clk_readl(clock->reg) & CPG_SD_STP_MASK);
+}
+
+static unsigned long cpg_sd_clock_recalc_rate(struct clk_hw *hw,
+						unsigned long parent_rate)
+{
+	struct sd_clock *clock = to_sd_clock(hw);
+	u32 rate = parent_rate;
+	u32 val, sd_fc;
+	int i;
+
+	val = clk_readl(clock->reg);
+
+	sd_fc = val & CPG_SD_FC_MASK;
+	for (i = 0; i < clock->div_num; i++)
+		if (sd_fc = (clock->div_table[i].val & CPG_SD_FC_MASK))
+			break;
+
+	if (i >= clock->div_num) {
+		pr_err("%s: 0x%4x is not support of division ratio.\n",
+			__func__, sd_fc);
+		return 0;
+	}
+
+	do_div(rate, clock->div_table[i].div);
+
+	return rate;
+}
+
+static unsigned int cpg_sd_clock_calc_div(struct sd_clock *clock,
+					  unsigned long rate,
+					  unsigned long parent_rate)
+{
+	unsigned int div;
+
+	if (!rate)
+		rate = 1;
+
+	div = DIV_ROUND_CLOSEST(parent_rate, rate);
+
+	return clamp_t(unsigned int, div, clock->div_min, clock->div_max);
+}
+
+static long cpg_sd_clock_round_rate(struct clk_hw *hw, unsigned long rate,
+				      unsigned long *parent_rate)
+{
+	struct sd_clock *clock = to_sd_clock(hw);
+	unsigned int div = cpg_sd_clock_calc_div(clock, rate, *parent_rate);
+
+	return *parent_rate / div;
+}
+
+static int cpg_sd_clock_set_rate(struct clk_hw *hw, unsigned long rate,
+				   unsigned long parent_rate)
+{
+	struct sd_clock *clock = to_sd_clock(hw);
+	unsigned int div = cpg_sd_clock_calc_div(clock, rate, parent_rate);
+	u32 val;
+	int i;
+
+	for (i = 0; i < clock->div_num; i++)
+		if (div = clock->div_table[i].div)
+			break;
+
+	if (i >= clock->div_num) {
+		pr_err("%s: Not support divider range : div=%d (%lu/%lu).\n",
+			__func__, div, parent_rate, rate);
+		return -EINVAL;
+	}
+
+	val = clk_readl(clock->reg);
+	val &= ~(CPG_SD_STP_MASK | CPG_SD_FC_MASK);
+	val |= clock->div_table[i].val & (CPG_SD_STP_MASK | CPG_SD_FC_MASK);
+	clk_writel(val, clock->reg);
+
+	return 0;
+}
+
+static const struct clk_ops cpg_sd_clock_ops = {
+	.enable = cpg_sd_clock_enable,
+	.disable = cpg_sd_clock_disable,
+	.is_enabled = cpg_sd_clock_is_enabled,
+	.recalc_rate = cpg_sd_clock_recalc_rate,
+	.round_rate = cpg_sd_clock_round_rate,
+	.set_rate = cpg_sd_clock_set_rate,
+};
+
+static struct clk * __init cpg_sd_clk_register(const struct cpg_core_clk *core,
+					       void __iomem *base,
+					       const char *parent_name)
+{
+	struct clk_init_data init;
+	struct sd_clock *clock;
+	struct clk *clk;
+	int i;
+
+	clock = kzalloc(sizeof(*clock), GFP_KERNEL);
+	if (!clock)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = core->name;
+	init.ops = &cpg_sd_clock_ops;
+	init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT;
+	init.parent_names = &parent_name;
+	init.num_parents = 1;
+
+	clock->reg = base + core->offset;
+	clock->hw.init = &init;
+	clock->div_table = cpg_sd_div_table;
+	clock->div_num = ARRAY_SIZE(cpg_sd_div_table);
+
+	clock->div_max = clock->div_table[0].div;
+	clock->div_min = clock->div_max;
+	for (i = 1; i < clock->div_num; i++) {
+		clock->div_max = max(clock->div_max, clock->div_table[i].div);
+		clock->div_min = min(clock->div_min, clock->div_table[i].div);
+	}
+
+	clk = clk_register(NULL, &clock->hw);
+	if (IS_ERR(clk))
+		kfree(clock);
+
+	return clk;
+}
 
 #define CPG_PLL0CR	0x00d8
 #define CPG_PLL2CR	0x002c
@@ -324,6 +569,9 @@ struct clk * __init r8a7795_cpg_clk_register(struct device *dev,
 		mult = (((value >> 24) & 0x7f) + 1) * 2;
 		break;
 
+	case CLK_TYPE_GEN3_SD:
+		return cpg_sd_clk_register(core, base, __clk_get_name(parent));
+
 	default:
 		return ERR_PTR(-EINVAL);
 	}
diff --git a/drivers/clk/shmobile/renesas-cpg-mssr.h b/drivers/clk/shmobile/renesas-cpg-mssr.h
index e09f03c..952b695 100644
--- a/drivers/clk/shmobile/renesas-cpg-mssr.h
+++ b/drivers/clk/shmobile/renesas-cpg-mssr.h
@@ -53,6 +53,8 @@ enum clk_types {
 	DEF_BASE(_name, _id, CLK_TYPE_FF, _parent, .div = _div, .mult = _mult)
 #define DEF_DIV6P1(_name, _id, _parent, _offset)	\
 	DEF_BASE(_name, _id, CLK_TYPE_DIV6P1, _parent, .offset = _offset)
+#define DEF_SD(_name, _id, _parent, _offset)	\
+	DEF_BASE(_name, _id, CLK_TYPE_GEN3_SD, _parent, .offset = _offset)
 
 
     /*
-- 
2.5.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [RFC] clk: shmobile: r8a7795: Add SD divider support
  2016-01-22 12:26 [RFC] clk: shmobile: r8a7795: Add SD divider support Dirk Behme
@ 2016-01-25  8:59 ` Geert Uytterhoeven
  2016-01-25  9:11 ` Dirk Behme
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Geert Uytterhoeven @ 2016-01-25  8:59 UTC (permalink / raw)
  To: linux-sh

Hi Dirk,

On Fri, Jan 22, 2016 at 1:26 PM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
> From: Takeshi Kihara <takeshi.kihara.df@renesas.com>
>
> This patch adds SD[0..3] clock divider support for R-Car Gen3 SoC.
>
> Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com>
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> ---
> Notes:
>
> * This is based on https://git.kernel.org/cgit/linux/kernel/git/horms/renesas-bsp.git/commit/drivers/clk/shmobile/clk-rcar-gen3.c?h=v4.2/rcar-3.0.x&idÍ10385afc15cef6bfbaea4aa5da41193b24fe82
>
> * This is compile tested *only*. The intention is to get some feedback
>   if this is the way we want to go.

I didn't look into all details, but in general this is what I intended.
Thanks!

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC] clk: shmobile: r8a7795: Add SD divider support
  2016-01-22 12:26 [RFC] clk: shmobile: r8a7795: Add SD divider support Dirk Behme
  2016-01-25  8:59 ` Geert Uytterhoeven
@ 2016-01-25  9:11 ` Dirk Behme
  2016-01-25 19:23 ` Wolfram Sang
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Dirk Behme @ 2016-01-25  9:11 UTC (permalink / raw)
  To: linux-sh

On 25.01.2016 09:59, Geert Uytterhoeven wrote:
> Hi Dirk,
>
> On Fri, Jan 22, 2016 at 1:26 PM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
>> From: Takeshi Kihara <takeshi.kihara.df@renesas.com>
>>
>> This patch adds SD[0..3] clock divider support for R-Car Gen3 SoC.
>>
>> Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com>
>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>> ---
>> Notes:
>>
>> * This is based on https://git.kernel.org/cgit/linux/kernel/git/horms/renesas-bsp.git/commit/drivers/clk/shmobile/clk-rcar-gen3.c?h=v4.2/rcar-3.0.x&idÍ10385afc15cef6bfbaea4aa5da41193b24fe82
>>
>> * This is compile tested *only*. The intention is to get some feedback
>>    if this is the way we want to go.
>
> I didn't look into all details, but in general this is what I intended.
> Thanks!


Great, thanks! :)


Wolfram: Maybe you like to include this [1] in your testing branch, then?

If it works for you, I'll remove the RFC, then.


Best regards

Dirk


[1] http://marc.info/?l=linux-sh&m\x145346559429931

See also

https://github.com/dirkbehme/linux-renesas-rcar-gen3/commit/018438cf8ff31e66a6511b4aa0b51e47a39392ed

https://github.com/dirkbehme/linux-renesas-rcar-gen3/commit/f31b7e18f89c447ad0d3b31c64cf360afe7d1d9f




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC] clk: shmobile: r8a7795: Add SD divider support
  2016-01-22 12:26 [RFC] clk: shmobile: r8a7795: Add SD divider support Dirk Behme
  2016-01-25  8:59 ` Geert Uytterhoeven
  2016-01-25  9:11 ` Dirk Behme
@ 2016-01-25 19:23 ` Wolfram Sang
  2016-01-27  7:14 ` Wolfram Sang
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Wolfram Sang @ 2016-01-25 19:23 UTC (permalink / raw)
  To: linux-sh

[-- Attachment #1: Type: text/plain, Size: 176 bytes --]


> Wolfram: Maybe you like to include this [1] in your testing branch, then?
> 
> If it works for you, I'll remove the RFC, then.

Yes, I'll try to test this tomorrow.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC] clk: shmobile: r8a7795: Add SD divider support
  2016-01-22 12:26 [RFC] clk: shmobile: r8a7795: Add SD divider support Dirk Behme
                   ` (2 preceding siblings ...)
  2016-01-25 19:23 ` Wolfram Sang
@ 2016-01-27  7:14 ` Wolfram Sang
  2016-01-27  8:43 ` Geert Uytterhoeven
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Wolfram Sang @ 2016-01-27  7:14 UTC (permalink / raw)
  To: linux-sh

[-- Attachment #1: Type: text/plain, Size: 1452 bytes --]

On Fri, Jan 22, 2016 at 01:26:26PM +0100, Dirk Behme wrote:
> From: Takeshi Kihara <takeshi.kihara.df@renesas.com>
> 
> This patch adds SD[0..3] clock divider support for R-Car Gen3 SoC.
> 
> Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com>
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>

So, I tested this patch and it basically works. I say basically because
the SDHI code currently does not change the clock rate, only
en-/disables it. UHS support will need to change the clock later.

One thing I noticed: SD0-2 are 50MHz like the docs say. SD3 is 200MHz
and I couldn't find a reason for that when having a glimpse. Dirk, can
you check?

> +		if (i >= clock->div_num) {
> +			pr_err("%s: 0x%4x is not support of division ratio.\n",
> +				__func__, sd_fc);
> +			return -ENODATA;
> +		}
...
> +	if (i >= clock->div_num) {
> +		pr_err("%s: 0x%4x is not support of division ratio.\n",
> +			__func__, sd_fc);
> +		return 0;
> +	}
...
> +	if (i >= clock->div_num) {
> +		pr_err("%s: Not support divider range : div=%d (%lu/%lu).\n",
> +			__func__, div, parent_rate, rate);
> +		return -EINVAL;
> +	}

For the above code blocks:

a) I'd think that a consistent -EINVAL would be a proper return value.

b) The driver doesn't do much error printouts, so I wonder if those
messages above are favourable. If so, they should probably print which
sd clock is affected?

Thanks,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC] clk: shmobile: r8a7795: Add SD divider support
  2016-01-22 12:26 [RFC] clk: shmobile: r8a7795: Add SD divider support Dirk Behme
                   ` (3 preceding siblings ...)
  2016-01-27  7:14 ` Wolfram Sang
@ 2016-01-27  8:43 ` Geert Uytterhoeven
  2016-01-27  9:01 ` Dirk Behme
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Geert Uytterhoeven @ 2016-01-27  8:43 UTC (permalink / raw)
  To: linux-sh

Hi Dirk,

On Fri, Jan 22, 2016 at 1:26 PM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
> From: Takeshi Kihara <takeshi.kihara.df@renesas.com>
>
> This patch adds SD[0..3] clock divider support for R-Car Gen3 SoC.
>
> Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com>
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> ---
> Notes:
>
> * This is based on https://git.kernel.org/cgit/linux/kernel/git/horms/renesas-bsp.git/commit/drivers/clk/shmobile/clk-rcar-gen3.c?h=v4.2/rcar-3.0.x&idÍ10385afc15cef6bfbaea4aa5da41193b24fe82
>
> * This is compile tested *only*. The intention is to get some feedback
>   if this is the way we want to go.

Thanks for your patch!
As you want to drop the RFC state, here are some comments from me.


> --- a/drivers/clk/shmobile/r8a7795-cpg-mssr.c
> +++ b/drivers/clk/shmobile/r8a7795-cpg-mssr.c

> @@ -199,6 +207,243 @@ static const unsigned int r8a7795_crit_mod_clks[] __initconst = {
>         MOD_CLK_ID(408),        /* INTC-AP (GIC) */
>  };
>
> +/* -----------------------------------------------------------------------------
> + * SDn Clock
> + *
> + */
> +#define CPG_SD_STP_N_HCK       BIT(9)
> +#define CPG_SD_STP_N_CK                BIT(8)
> +#define CPG_SD_SD_N_SRCFC_MASK (0x7 << CPG_SD_SD_N_SRCFC_SHIFT)
> +#define CPG_SD_SD_N_SRCFC_SHIFT        2
> +#define CPG_SD_SD_N_FC_MASK    (0x3 << CPG_SD_SD_N_FC_SHIFT)
> +#define CPG_SD_SD_N_FC_SHIFT   0

The "N_" in the macro names doesn't gain us much, so I'd remove it.

> +#define CPG_SD_STP_MASK                (CPG_SD_STP_N_HCK | CPG_SD_STP_N_CK)
> +#define CPG_SD_FC_MASK         (CPG_SD_SD_N_SRCFC_MASK | CPG_SD_SD_N_FC_MASK)
> +
> +/* CPG_SD_DIV_TABLE_DATA(stp_n_hck, stp_n_ck, sd_n_srcfc, sd_n_fc, div) */
> +#define CPG_SD_DIV_TABLE_DATA(_a, _b, _c, _d, _e) \

I would use descriptive macro parameter names, and drop the comment. E.g.

    #define CPG_SD_DIV_TABLE_DATA(stp_hck, stp_ck, sd_srcfc, sd_fc, div)

> +{ \
> +       .val = ((_a) ? CPG_SD_STP_N_HCK : 0) | \
> +              ((_b) ? CPG_SD_STP_N_CK : 0) | \
> +              (((_c) << CPG_SD_SD_N_SRCFC_SHIFT) & CPG_SD_SD_N_SRCFC_MASK) | \
> +              (((_d) << CPG_SD_SD_N_FC_SHIFT) & CPG_SD_SD_N_FC_MASK), \

As these are only used here, already abstracted by the macro, and the
masks never
really do anything giving the table values below, I would
  - drop the "& CPG_SD_*_MASK" operations, and the CPG_SD_*_MASK definitions,
  - hardcode the shifts, and drop the CPG_SD_*_SHIFT definitions.


> +       .div = (_e), \
> +}
> +
> +struct sd_div_table {
> +       u32 val;
> +       unsigned int div;
> +};
> +
> +struct sd_clock {
> +       struct clk_hw hw;
> +       void __iomem *reg;
> +       const struct sd_div_table *div_table;
> +       int div_num;

unsigned int

> +       unsigned int div_min;
> +       unsigned int div_max;
> +};
> +
> +/* SDn divider
> + *                     sd_n_srcfc sd_n_fc   div
> + * stp_n_hck stp_n_ck  (div)      (div)     = sd_n_srcfc x sd_n_fc
> + *-------------------------------------------------------------------
> + *  0         0         0 (1)      1 (4)      4
> + *  0         0         1 (2)      1 (4)      8
> + *  1         0         2 (4)      1 (4)     16
> + *  1         0         3 (8)      1 (4)     32
> + *  1         0         4 (16)     1 (4)     64
> + *  0         0         0 (1)      0 (2)      2
> + *  0         0         1 (2)      0 (2)      4
> + *  1         0         2 (4)      0 (2)      8
> + *  1         0         3 (8)      0 (2)     16
> + *  1         0         4 (16)     0 (2)     32
> + */
> +static const struct sd_div_table cpg_sd_div_table[] = {
> +/*     CPG_SD_DIV_TABLE_DATA(stp_n_hck, stp_n_ck, sd_n_srcfc, sd_n_fc, div) */
> +       CPG_SD_DIV_TABLE_DATA(0,        0,        0,          1,        4),
> +       CPG_SD_DIV_TABLE_DATA(0,        0,        1,          1,        8),
> +       CPG_SD_DIV_TABLE_DATA(1,        0,        2,          1,       16),
> +       CPG_SD_DIV_TABLE_DATA(1,        0,        3,          1,       32),
> +       CPG_SD_DIV_TABLE_DATA(1,        0,        4,          1,       64),
> +       CPG_SD_DIV_TABLE_DATA(0,        0,        0,          0,        2),
> +       CPG_SD_DIV_TABLE_DATA(0,        0,        1,          0,        4),
> +       CPG_SD_DIV_TABLE_DATA(1,        0,        2,          0,        8),
> +       CPG_SD_DIV_TABLE_DATA(1,        0,        3,          0,       16),
> +       CPG_SD_DIV_TABLE_DATA(1,        0,        4,          0,       32),
> +};
> +
> +#define to_sd_clock(_hw) container_of(_hw, struct sd_clock, hw)
> +
> +static int cpg_sd_clock_endisable(struct clk_hw *hw, bool enable)
> +{
> +       struct sd_clock *clock = to_sd_clock(hw);
> +       u32 val, sd_fc;
> +       int i;

unsigned int;

> +
> +       val = clk_readl(clock->reg);
> +
> +       if (enable) {
> +               sd_fc = val & CPG_SD_FC_MASK;
> +               for (i = 0; i < clock->div_num; i++)
> +                       if (sd_fc = (clock->div_table[i].val & CPG_SD_FC_MASK))
> +                               break;
> +
> +               if (i >= clock->div_num) {
> +                       pr_err("%s: 0x%4x is not support of division ratio.\n",
> +                               __func__, sd_fc);
> +                       return -ENODATA;
> +               }
> +
> +               val &= ~(CPG_SD_STP_MASK);
> +               val |= clock->div_table[i].val & CPG_SD_STP_MASK;
> +       } else
> +               val |= CPG_SD_STP_MASK;
> +
> +       clk_writel(val, clock->reg);
> +
> +       return 0;
> +}
> +
> +static int cpg_sd_clock_enable(struct clk_hw *hw)
> +{
> +       return cpg_sd_clock_endisable(hw, true);
> +}
> +
> +static void cpg_sd_clock_disable(struct clk_hw *hw)
> +{
> +       cpg_sd_clock_endisable(hw, false);
> +}

Given there's not much similarity between the enable and disable cases,
there's not much gained from having a common cpg_sd_clock_endisable().
Perhaps just inline the code in cpg_sd_clock_{en,dis}able()?

> +static unsigned long cpg_sd_clock_recalc_rate(struct clk_hw *hw,
> +                                               unsigned long parent_rate)
> +{
> +       struct sd_clock *clock = to_sd_clock(hw);
> +       u32 rate = parent_rate;

unsigned long

> +       u32 val, sd_fc;
> +       int i;

unsigned int

> +
> +       val = clk_readl(clock->reg);
> +
> +       sd_fc = val & CPG_SD_FC_MASK;
> +       for (i = 0; i < clock->div_num; i++)
> +               if (sd_fc = (clock->div_table[i].val & CPG_SD_FC_MASK))
> +                       break;
> +
> +       if (i >= clock->div_num) {
> +               pr_err("%s: 0x%4x is not support of division ratio.\n",
> +                       __func__, sd_fc);
> +               return 0;
> +       }
> +
> +       do_div(rate, clock->div_table[i].div);

do_div() is meant for 64-by-32 division, while this is 32-by-32 on 32-bit
platforms.

    rate = DIV_ROUND_CLOSEST(rate, clock->div_table[i].div);

> +
> +       return rate;
> +}
> +
> +static unsigned int cpg_sd_clock_calc_div(struct sd_clock *clock,
> +                                         unsigned long rate,
> +                                         unsigned long parent_rate)
> +{
> +       unsigned int div;
> +
> +       if (!rate)
> +               rate = 1;
> +
> +       div = DIV_ROUND_CLOSEST(parent_rate, rate);
> +
> +       return clamp_t(unsigned int, div, clock->div_min, clock->div_max);
> +}
> +
> +static long cpg_sd_clock_round_rate(struct clk_hw *hw, unsigned long rate,
> +                                     unsigned long *parent_rate)
> +{
> +       struct sd_clock *clock = to_sd_clock(hw);
> +       unsigned int div = cpg_sd_clock_calc_div(clock, rate, *parent_rate);
> +
> +       return *parent_rate / div;

DIV_ROUND_CLOSEST()?

> +}
> +
> +static int cpg_sd_clock_set_rate(struct clk_hw *hw, unsigned long rate,
> +                                  unsigned long parent_rate)
> +{
> +       struct sd_clock *clock = to_sd_clock(hw);
> +       unsigned int div = cpg_sd_clock_calc_div(clock, rate, parent_rate);
> +       u32 val;
> +       int i;

unsigned int

> +
> +       for (i = 0; i < clock->div_num; i++)
> +               if (div = clock->div_table[i].div)
> +                       break;
> +
> +       if (i >= clock->div_num) {
> +               pr_err("%s: Not support divider range : div=%d (%lu/%lu).\n",

%u for div

> +                       __func__, div, parent_rate, rate);
> +               return -EINVAL;
> +       }
> +
> +       val = clk_readl(clock->reg);
> +       val &= ~(CPG_SD_STP_MASK | CPG_SD_FC_MASK);
> +       val |= clock->div_table[i].val & (CPG_SD_STP_MASK | CPG_SD_FC_MASK);
> +       clk_writel(val, clock->reg);
> +
> +       return 0;
> +}

> +static struct clk * __init cpg_sd_clk_register(const struct cpg_core_clk *core,
> +                                              void __iomem *base,
> +                                              const char *parent_name)
> +{
> +       struct clk_init_data init;
> +       struct sd_clock *clock;
> +       struct clk *clk;
> +       int i;

unsigned int

> +
> +       clock = kzalloc(sizeof(*clock), GFP_KERNEL);
> +       if (!clock)
> +               return ERR_PTR(-ENOMEM);
> +
> +       init.name = core->name;
> +       init.ops = &cpg_sd_clock_ops;
> +       init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT;
> +       init.parent_names = &parent_name;
> +       init.num_parents = 1;
> +
> +       clock->reg = base + core->offset;
> +       clock->hw.init = &init;
> +       clock->div_table = cpg_sd_div_table;
> +       clock->div_num = ARRAY_SIZE(cpg_sd_div_table);
> +
> +       clock->div_max = clock->div_table[0].div;
> +       clock->div_min = clock->div_max;
> +       for (i = 1; i < clock->div_num; i++) {
> +               clock->div_max = max(clock->div_max, clock->div_table[i].div);
> +               clock->div_min = min(clock->div_min, clock->div_table[i].div);
> +       }
> +
> +       clk = clk_register(NULL, &clock->hw);
> +       if (IS_ERR(clk))
> +               kfree(clock);

Shouldn't this provide the SDnH clocks, too? Or aren't they needed?

> +       return clk;
> +}

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC] clk: shmobile: r8a7795: Add SD divider support
  2016-01-22 12:26 [RFC] clk: shmobile: r8a7795: Add SD divider support Dirk Behme
                   ` (4 preceding siblings ...)
  2016-01-27  8:43 ` Geert Uytterhoeven
@ 2016-01-27  9:01 ` Dirk Behme
  2016-01-27 13:14 ` Dirk Behme
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Dirk Behme @ 2016-01-27  9:01 UTC (permalink / raw)
  To: linux-sh

Hi Geert,

many thanks for looking at this!

On 27.01.2016 09:43, Geert Uytterhoeven wrote:
> Hi Dirk,
>
> On Fri, Jan 22, 2016 at 1:26 PM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
>> From: Takeshi Kihara <takeshi.kihara.df@renesas.com>
>>
>> This patch adds SD[0..3] clock divider support for R-Car Gen3 SoC.
>>
>> Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com>
>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>> ---
>> Notes:
>>
>> * This is based on https://git.kernel.org/cgit/linux/kernel/git/horms/renesas-bsp.git/commit/drivers/clk/shmobile/clk-rcar-gen3.c?h=v4.2/rcar-3.0.x&idÍ10385afc15cef6bfbaea4aa5da41193b24fe82
>>
>> * This is compile tested *only*. The intention is to get some feedback
>>    if this is the way we want to go.
>
> Thanks for your patch!
> As you want to drop the RFC state, here are some comments from me.
>
>
>> --- a/drivers/clk/shmobile/r8a7795-cpg-mssr.c
>> +++ b/drivers/clk/shmobile/r8a7795-cpg-mssr.c
>
>> @@ -199,6 +207,243 @@ static const unsigned int r8a7795_crit_mod_clks[] __initconst = {
>>          MOD_CLK_ID(408),        /* INTC-AP (GIC) */
>>   };
>>
>> +/* -----------------------------------------------------------------------------
>> + * SDn Clock
>> + *
>> + */
>> +#define CPG_SD_STP_N_HCK       BIT(9)
>> +#define CPG_SD_STP_N_CK                BIT(8)
>> +#define CPG_SD_SD_N_SRCFC_MASK (0x7 << CPG_SD_SD_N_SRCFC_SHIFT)
>> +#define CPG_SD_SD_N_SRCFC_SHIFT        2
>> +#define CPG_SD_SD_N_FC_MASK    (0x3 << CPG_SD_SD_N_FC_SHIFT)
>> +#define CPG_SD_SD_N_FC_SHIFT   0
>
> The "N_" in the macro names doesn't gain us much, so I'd remove it.
>
>> +#define CPG_SD_STP_MASK                (CPG_SD_STP_N_HCK | CPG_SD_STP_N_CK)
>> +#define CPG_SD_FC_MASK         (CPG_SD_SD_N_SRCFC_MASK | CPG_SD_SD_N_FC_MASK)
>> +
>> +/* CPG_SD_DIV_TABLE_DATA(stp_n_hck, stp_n_ck, sd_n_srcfc, sd_n_fc, div) */
>> +#define CPG_SD_DIV_TABLE_DATA(_a, _b, _c, _d, _e) \
>
> I would use descriptive macro parameter names, and drop the comment. E.g.
>
>      #define CPG_SD_DIV_TABLE_DATA(stp_hck, stp_ck, sd_srcfc, sd_fc, div)
>
>> +{ \
>> +       .val = ((_a) ? CPG_SD_STP_N_HCK : 0) | \
>> +              ((_b) ? CPG_SD_STP_N_CK : 0) | \
>> +              (((_c) << CPG_SD_SD_N_SRCFC_SHIFT) & CPG_SD_SD_N_SRCFC_MASK) | \
>> +              (((_d) << CPG_SD_SD_N_FC_SHIFT) & CPG_SD_SD_N_FC_MASK), \
>
> As these are only used here, already abstracted by the macro, and the
> masks never
> really do anything giving the table values below, I would
>    - drop the "& CPG_SD_*_MASK" operations, and the CPG_SD_*_MASK definitions,
>    - hardcode the shifts, and drop the CPG_SD_*_SHIFT definitions.
>
>
>> +       .div = (_e), \
>> +}
>> +
>> +struct sd_div_table {
>> +       u32 val;
>> +       unsigned int div;
>> +};
>> +
>> +struct sd_clock {
>> +       struct clk_hw hw;
>> +       void __iomem *reg;
>> +       const struct sd_div_table *div_table;
>> +       int div_num;
>
> unsigned int
>
>> +       unsigned int div_min;
>> +       unsigned int div_max;
>> +};
>> +
>> +/* SDn divider
>> + *                     sd_n_srcfc sd_n_fc   div
>> + * stp_n_hck stp_n_ck  (div)      (div)     = sd_n_srcfc x sd_n_fc
>> + *-------------------------------------------------------------------
>> + *  0         0         0 (1)      1 (4)      4
>> + *  0         0         1 (2)      1 (4)      8
>> + *  1         0         2 (4)      1 (4)     16
>> + *  1         0         3 (8)      1 (4)     32
>> + *  1         0         4 (16)     1 (4)     64
>> + *  0         0         0 (1)      0 (2)      2
>> + *  0         0         1 (2)      0 (2)      4
>> + *  1         0         2 (4)      0 (2)      8
>> + *  1         0         3 (8)      0 (2)     16
>> + *  1         0         4 (16)     0 (2)     32
>> + */
>> +static const struct sd_div_table cpg_sd_div_table[] = {
>> +/*     CPG_SD_DIV_TABLE_DATA(stp_n_hck, stp_n_ck, sd_n_srcfc, sd_n_fc, div) */
>> +       CPG_SD_DIV_TABLE_DATA(0,        0,        0,          1,        4),
>> +       CPG_SD_DIV_TABLE_DATA(0,        0,        1,          1,        8),
>> +       CPG_SD_DIV_TABLE_DATA(1,        0,        2,          1,       16),
>> +       CPG_SD_DIV_TABLE_DATA(1,        0,        3,          1,       32),
>> +       CPG_SD_DIV_TABLE_DATA(1,        0,        4,          1,       64),
>> +       CPG_SD_DIV_TABLE_DATA(0,        0,        0,          0,        2),
>> +       CPG_SD_DIV_TABLE_DATA(0,        0,        1,          0,        4),
>> +       CPG_SD_DIV_TABLE_DATA(1,        0,        2,          0,        8),
>> +       CPG_SD_DIV_TABLE_DATA(1,        0,        3,          0,       16),
>> +       CPG_SD_DIV_TABLE_DATA(1,        0,        4,          0,       32),
>> +};
>> +
>> +#define to_sd_clock(_hw) container_of(_hw, struct sd_clock, hw)
>> +
>> +static int cpg_sd_clock_endisable(struct clk_hw *hw, bool enable)
>> +{
>> +       struct sd_clock *clock = to_sd_clock(hw);
>> +       u32 val, sd_fc;
>> +       int i;
>
> unsigned int;
>
>> +
>> +       val = clk_readl(clock->reg);
>> +
>> +       if (enable) {
>> +               sd_fc = val & CPG_SD_FC_MASK;
>> +               for (i = 0; i < clock->div_num; i++)
>> +                       if (sd_fc = (clock->div_table[i].val & CPG_SD_FC_MASK))
>> +                               break;
>> +
>> +               if (i >= clock->div_num) {
>> +                       pr_err("%s: 0x%4x is not support of division ratio.\n",
>> +                               __func__, sd_fc);
>> +                       return -ENODATA;
>> +               }
>> +
>> +               val &= ~(CPG_SD_STP_MASK);
>> +               val |= clock->div_table[i].val & CPG_SD_STP_MASK;
>> +       } else
>> +               val |= CPG_SD_STP_MASK;
>> +
>> +       clk_writel(val, clock->reg);
>> +
>> +       return 0;
>> +}
>> +
>> +static int cpg_sd_clock_enable(struct clk_hw *hw)
>> +{
>> +       return cpg_sd_clock_endisable(hw, true);
>> +}
>> +
>> +static void cpg_sd_clock_disable(struct clk_hw *hw)
>> +{
>> +       cpg_sd_clock_endisable(hw, false);
>> +}
>
> Given there's not much similarity between the enable and disable cases,
> there's not much gained from having a common cpg_sd_clock_endisable().
> Perhaps just inline the code in cpg_sd_clock_{en,dis}able()?
>
>> +static unsigned long cpg_sd_clock_recalc_rate(struct clk_hw *hw,
>> +                                               unsigned long parent_rate)
>> +{
>> +       struct sd_clock *clock = to_sd_clock(hw);
>> +       u32 rate = parent_rate;
>
> unsigned long
>
>> +       u32 val, sd_fc;
>> +       int i;
>
> unsigned int
>
>> +
>> +       val = clk_readl(clock->reg);
>> +
>> +       sd_fc = val & CPG_SD_FC_MASK;
>> +       for (i = 0; i < clock->div_num; i++)
>> +               if (sd_fc = (clock->div_table[i].val & CPG_SD_FC_MASK))
>> +                       break;
>> +
>> +       if (i >= clock->div_num) {
>> +               pr_err("%s: 0x%4x is not support of division ratio.\n",
>> +                       __func__, sd_fc);
>> +               return 0;
>> +       }
>> +
>> +       do_div(rate, clock->div_table[i].div);
>
> do_div() is meant for 64-by-32 division, while this is 32-by-32 on 32-bit
> platforms.
>
>      rate = DIV_ROUND_CLOSEST(rate, clock->div_table[i].div);
>
>> +
>> +       return rate;
>> +}
>> +
>> +static unsigned int cpg_sd_clock_calc_div(struct sd_clock *clock,
>> +                                         unsigned long rate,
>> +                                         unsigned long parent_rate)
>> +{
>> +       unsigned int div;
>> +
>> +       if (!rate)
>> +               rate = 1;
>> +
>> +       div = DIV_ROUND_CLOSEST(parent_rate, rate);
>> +
>> +       return clamp_t(unsigned int, div, clock->div_min, clock->div_max);
>> +}
>> +
>> +static long cpg_sd_clock_round_rate(struct clk_hw *hw, unsigned long rate,
>> +                                     unsigned long *parent_rate)
>> +{
>> +       struct sd_clock *clock = to_sd_clock(hw);
>> +       unsigned int div = cpg_sd_clock_calc_div(clock, rate, *parent_rate);
>> +
>> +       return *parent_rate / div;
>
> DIV_ROUND_CLOSEST()?
>
>> +}
>> +
>> +static int cpg_sd_clock_set_rate(struct clk_hw *hw, unsigned long rate,
>> +                                  unsigned long parent_rate)
>> +{
>> +       struct sd_clock *clock = to_sd_clock(hw);
>> +       unsigned int div = cpg_sd_clock_calc_div(clock, rate, parent_rate);
>> +       u32 val;
>> +       int i;
>
> unsigned int
>
>> +
>> +       for (i = 0; i < clock->div_num; i++)
>> +               if (div = clock->div_table[i].div)
>> +                       break;
>> +
>> +       if (i >= clock->div_num) {
>> +               pr_err("%s: Not support divider range : div=%d (%lu/%lu).\n",
>
> %u for div
>
>> +                       __func__, div, parent_rate, rate);
>> +               return -EINVAL;
>> +       }
>> +
>> +       val = clk_readl(clock->reg);
>> +       val &= ~(CPG_SD_STP_MASK | CPG_SD_FC_MASK);
>> +       val |= clock->div_table[i].val & (CPG_SD_STP_MASK | CPG_SD_FC_MASK);
>> +       clk_writel(val, clock->reg);
>> +
>> +       return 0;
>> +}
>
>> +static struct clk * __init cpg_sd_clk_register(const struct cpg_core_clk *core,
>> +                                              void __iomem *base,
>> +                                              const char *parent_name)
>> +{
>> +       struct clk_init_data init;
>> +       struct sd_clock *clock;
>> +       struct clk *clk;
>> +       int i;
>
> unsigned int
>
>> +
>> +       clock = kzalloc(sizeof(*clock), GFP_KERNEL);
>> +       if (!clock)
>> +               return ERR_PTR(-ENOMEM);
>> +
>> +       init.name = core->name;
>> +       init.ops = &cpg_sd_clock_ops;
>> +       init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT;
>> +       init.parent_names = &parent_name;
>> +       init.num_parents = 1;
>> +
>> +       clock->reg = base + core->offset;
>> +       clock->hw.init = &init;
>> +       clock->div_table = cpg_sd_div_table;
>> +       clock->div_num = ARRAY_SIZE(cpg_sd_div_table);
>> +
>> +       clock->div_max = clock->div_table[0].div;
>> +       clock->div_min = clock->div_max;
>> +       for (i = 1; i < clock->div_num; i++) {
>> +               clock->div_max = max(clock->div_max, clock->div_table[i].div);
>> +               clock->div_min = min(clock->div_min, clock->div_table[i].div);
>> +       }
>> +
>> +       clk = clk_register(NULL, &clock->hw);
>> +       if (IS_ERR(clk))
>> +               kfree(clock);


I'll have a look to the topics above. Thanks!


> Shouldn't this provide the SDnH clocks, too? Or aren't they needed?


This is a very good question :)

I was thinking about this a short moment, too, and haven't a good 
answer. The BSP this is taken from doesn't touch the SDnH explicitly.

Looking at the resulting clock tree I think I've seen that the SDnH 
clocks are there, too.

Anybody with an idea?

Best regards

Dirk

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC] clk: shmobile: r8a7795: Add SD divider support
  2016-01-22 12:26 [RFC] clk: shmobile: r8a7795: Add SD divider support Dirk Behme
                   ` (5 preceding siblings ...)
  2016-01-27  9:01 ` Dirk Behme
@ 2016-01-27 13:14 ` Dirk Behme
  2016-01-29  8:14 ` Wolfram Sang
  2016-01-29  8:29 ` Dirk Behme
  8 siblings, 0 replies; 10+ messages in thread
From: Dirk Behme @ 2016-01-27 13:14 UTC (permalink / raw)
  To: linux-sh

On 27.01.2016 08:14, Wolfram Sang wrote:
> On Fri, Jan 22, 2016 at 01:26:26PM +0100, Dirk Behme wrote:
>> From: Takeshi Kihara <takeshi.kihara.df@renesas.com>
>>
>> This patch adds SD[0..3] clock divider support for R-Car Gen3 SoC.
>>
>> Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com>
>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>
> So, I tested this patch and it basically works. I say basically because
> the SDHI code currently does not change the clock rate, only
> en-/disables it. UHS support will need to change the clock later.
>
> One thing I noticed: SD0-2 are 50MHz like the docs say. SD3 is 200MHz
> and I couldn't find a reason for that when having a glimpse. Dirk, can
> you check?


Having the clock patch applied on top of your renesas/v8-sdhi branch [1] 
I get

root@salvator-x:/sys/kernel/debug/clk# cat clk_summary
    clock             enable_cnt  prepare_cnt        rate
--------------------------------------------------------
...
     .main                     1            1     8333333
     ...
        .pll1                  1            1  1599999936
           .pll1_div2          2            2   799999968
              hdmi             0            0    24999999
                 hdmi0         0            0    24999999
                 hdmi1         0            0    24999999
              cl               0            0    16666666
              sd3              0            0    99999996
                 sdif3         0            0    99999996
              sd2              0            0    99999996
                 sdif2         0            0    99999996
              sd1              0            0    49999998
                 sdif1         0            0    49999998
              sd0              0            0    49999998
                 sdif0         0            0    49999998


Best regards

Dirk

[1] 
https://github.com/dirkbehme/linux-renesas-rcar-gen3/commits/wsa-renesas/v8-sdhi

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC] clk: shmobile: r8a7795: Add SD divider support
  2016-01-22 12:26 [RFC] clk: shmobile: r8a7795: Add SD divider support Dirk Behme
                   ` (6 preceding siblings ...)
  2016-01-27 13:14 ` Dirk Behme
@ 2016-01-29  8:14 ` Wolfram Sang
  2016-01-29  8:29 ` Dirk Behme
  8 siblings, 0 replies; 10+ messages in thread
From: Wolfram Sang @ 2016-01-29  8:14 UTC (permalink / raw)
  To: linux-sh

[-- Attachment #1: Type: text/plain, Size: 2640 bytes --]

> Having the clock patch applied on top of your renesas/v8-sdhi branch [1] I
> get
> 
> root@salvator-x:/sys/kernel/debug/clk# cat clk_summary
>    clock             enable_cnt  prepare_cnt        rate
> --------------------------------------------------------
> ...
>     .main                     1            1     8333333
>     ...
>        .pll1                  1            1  1599999936
>           .pll1_div2          2            2   799999968
>              hdmi             0            0    24999999
>                 hdmi0         0            0    24999999
>                 hdmi1         0            0    24999999
>              cl               0            0    16666666
>              sd3              0            0    99999996
>                 sdif3         0            0    99999996
>              sd2              0            0    99999996
>                 sdif2         0            0    99999996
>              sd1              0            0    49999998
>                 sdif1         0            0    49999998
>              sd0              0            0    49999998
>                 sdif0         0            0    49999998

This is what I get (with your cleanup patch also applied):

...
    .main                                 1            1     8333333          0 0  
    ...
       .pll1                              1            1  1599999936          0 0  
          .pll1_div2                      3            3   799999968          0 0  
             hdmi                         0            0    24999999          0 0  
                hdmi0                     0            0    24999999          0 0  
                hdmi1                     0            0    24999999          0 0  
             cl                           0            0    16666666          0 0  
             sd3                          1            1   199999992          0 0  
                sdif3                     1            2   199999992          0 0  
             sd2                          0            0    49999998          0 0  
                sdif2                     0            0    49999998          0 0  
             sd1                          0            0    49999998          0 0  
                sdif1                     0            0    49999998          0 0  
             sd0                          1            1    49999998          0 0  
                sdif0                     1            2    49999998          0 0  

Could it be a firmware difference (i.e. bootloader trying to boot from eMMC and
changing values)?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC] clk: shmobile: r8a7795: Add SD divider support
  2016-01-22 12:26 [RFC] clk: shmobile: r8a7795: Add SD divider support Dirk Behme
                   ` (7 preceding siblings ...)
  2016-01-29  8:14 ` Wolfram Sang
@ 2016-01-29  8:29 ` Dirk Behme
  8 siblings, 0 replies; 10+ messages in thread
From: Dirk Behme @ 2016-01-29  8:29 UTC (permalink / raw)
  To: linux-sh

On 29.01.2016 09:14, Wolfram Sang wrote:
>> Having the clock patch applied on top of your renesas/v8-sdhi branch [1] I
>> get
>>
>> root@salvator-x:/sys/kernel/debug/clk# cat clk_summary
>>     clock             enable_cnt  prepare_cnt        rate
>> --------------------------------------------------------
>> ...
>>      .main                     1            1     8333333
>>      ...
>>         .pll1                  1            1  1599999936
>>            .pll1_div2          2            2   799999968
>>               hdmi             0            0    24999999
>>                  hdmi0         0            0    24999999
>>                  hdmi1         0            0    24999999
>>               cl               0            0    16666666
>>               sd3              0            0    99999996
>>                  sdif3         0            0    99999996
>>               sd2              0            0    99999996
>>                  sdif2         0            0    99999996
>>               sd1              0            0    49999998
>>                  sdif1         0            0    49999998
>>               sd0              0            0    49999998
>>                  sdif0         0            0    49999998
>
> This is what I get (with your cleanup patch also applied):
>
> ...
>      .main                                 1            1     8333333          0 0
>      ...
>         .pll1                              1            1  1599999936          0 0
>            .pll1_div2                      3            3   799999968          0 0
>               hdmi                         0            0    24999999          0 0
>                  hdmi0                     0            0    24999999          0 0
>                  hdmi1                     0            0    24999999          0 0
>               cl                           0            0    16666666          0 0
>               sd3                          1            1   199999992          0 0
>                  sdif3                     1            2   199999992          0 0
>               sd2                          0            0    49999998          0 0
>                  sdif2                     0            0    49999998          0 0
>               sd1                          0            0    49999998          0 0
>                  sdif1                     0            0    49999998          0 0
>               sd0                          1            1    49999998          0 0
>                  sdif0                     1            2    49999998          0 0
>
> Could it be a firmware difference (i.e. bootloader trying to boot from eMMC and
> changing values)?


Yes, its U-Boot. I have U-Boot from rcar-3.0.3 BSP on the board and 
verified that it configures exactly the clock configuration I gave above.

Best regards

Dirk

Bes

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2016-01-29  8:29 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-22 12:26 [RFC] clk: shmobile: r8a7795: Add SD divider support Dirk Behme
2016-01-25  8:59 ` Geert Uytterhoeven
2016-01-25  9:11 ` Dirk Behme
2016-01-25 19:23 ` Wolfram Sang
2016-01-27  7:14 ` Wolfram Sang
2016-01-27  8:43 ` Geert Uytterhoeven
2016-01-27  9:01 ` Dirk Behme
2016-01-27 13:14 ` Dirk Behme
2016-01-29  8:14 ` Wolfram Sang
2016-01-29  8:29 ` Dirk Behme

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.