[PATCH RFC] tango4 clk rewrite

[PATCH RFC] tango4 clk rewrite

[Mason]

Hello,

I'd like to hear your comments on this rewrite of the clk driver
for tango4. The main changes are:

1) Flag unsupported setups (to make assumptions explicit)
2) Drop LEGACY_DIV method
3) Support two additional clocks (usb and sdio)

For the extract method, what is the preferred way:
A macro or an inline function?
(Too bad you guys don't like the bit-field approach)


 drivers/clk/clk-tango4.c | 64 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 43 insertions(+), 21 deletions(-)

diff --git a/drivers/clk/clk-tango4.c b/drivers/clk/clk-tango4.c
index 004ab7dfcfe3..f8fa3955f9ee 100644
--- a/drivers/clk/clk-tango4.c
+++ b/drivers/clk/clk-tango4.c
@@ -4,17 +4,26 @@
 #include <linux/init.h>
 #include <linux/io.h>
 
-static struct clk *out[2];
-static struct clk_onecell_data clk_data = { out, 2 };
+#define SYSCLK_DIV	0x20
+#define CPUCLK_DIV	0x24
 
-#define SYSCLK_CTRL	0x20
-#define CPUCLK_CTRL	0x24
-#define LEGACY_DIV	0x3c
+#define FATAL(cond) if (cond) panic("Unsupported clkgen setup!\n")
 
-#define PLL_N(val)	(((val) >>  0) & 0x7f)
-#define PLL_K(val)	(((val) >> 13) & 0x7)
-#define PLL_M(val)	(((val) >> 16) & 0x7)
-#define DIV_INDEX(val)	(((val) >>  8) & 0xf)
+#define CLK_COUNT 4 /* cpu_clk, sys_clk, usb_clk, sdio_clk */
+static struct clk *out[CLK_COUNT];
+static struct clk_onecell_data clk_data = { out, CLK_COUNT };
+
+#define EXTRACT(v,o,w) (((v) >> (o)) & ((1u << (w)) - 1))
+
+static inline u32 extract(u32 val, int offset, int width) {
+	return (val >> offset) & ((1u << width) - 1);
+}
+
+/*** CLKGEN_PLL ***/
+#define PLL_N(v)	extract(v,  0, 7)
+#define PLL_K(v)	extract(v, 13, 3)
+#define PLL_M(v)	extract(v, 16, 3)
+#define PLL_ISEL(v)	extract(v, 24, 3)
 
 static void __init make_pll(int idx, const char *parent, void __iomem *base)
 {
@@ -26,36 +35,49 @@ static void __init make_pll(int idx, const char *parent, void __iomem *base)
 	mul =  PLL_N(val) + 1;
 	div = (PLL_M(val) + 1) << PLL_K(val);
 	clk_register_fixed_factor(NULL, name, parent, 0, mul, div);
+	FATAL(PLL_ISEL(val) != 1);
 }
 
-static int __init get_div(void __iomem *base)
+static void __init make_cd(int idx, void __iomem *base)
 {
-	u8 sysclk_tab[16] = { 2, 4, 3, 3, 3, 3, 3, 3, 4, 4, 4, 4 };
-	int idx = DIV_INDEX(readl_relaxed(base + LEGACY_DIV));
+	char name[8];
+	u32 mul, div;
 
-	return sysclk_tab[idx];
+	sprintf(name, "cd%d", idx);
+	mul =  1 << 27;
+	div = (1 << 28) + readl_relaxed(base + idx*8);
+	clk_register_fixed_factor(NULL, name, "pll2", 0, mul, div);
 }
 
 static void __init tango4_clkgen_setup(struct device_node *np)
 {
-	int div, ret;
+	int i, err;
 	void __iomem *base = of_iomap(np, 0);
 	const char *parent = of_clk_get_parent_name(np, 0);
 
 	if (!base)
 		panic("%s: invalid address\n", np->full_name);
 
+	FATAL(readl_relaxed(base + CPUCLK_DIV) & BIT(23));
+	FATAL(readl_relaxed(base + SYSCLK_DIV) & BIT(23));
+
 	make_pll(0, parent, base);
 	make_pll(1, parent, base);
+	make_pll(2, parent, base);
+	make_cd(2, base + 0x80);
+	make_cd(6, base + 0x80);
+
+	out[0] = clk_register_divider(NULL, "cpu_clk", "pll0", 0,
+			base + CPUCLK_DIV, 8, 8, CLK_DIVIDER_ONE_BASED, NULL);
+	out[1] = clk_register_fixed_factor(NULL, "sys_clk", "pll1", 0, 1, 4);
+	out[2] = clk_register_fixed_factor(NULL,  "usb_clk", "cd2", 0, 1, 2);
+	out[3] = clk_register_fixed_factor(NULL, "sdio_clk", "cd6", 0, 1, 2);
 
-	out[0] = clk_register_divider(NULL, "cpuclk", "pll0", 0,
-			base + CPUCLK_CTRL, 8, 8, CLK_DIVIDER_ONE_BASED, NULL);
+	err = of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
 
-	div = readl_relaxed(base + SYSCLK_CTRL) & BIT(23) ? get_div(base) : 4;
-	out[1] = clk_register_fixed_factor(NULL, "sysclk", "pll1", 0, 1, div);
+	for (i = 0; i < CLK_COUNT; ++i)
+		if (IS_ERR(out[i])) err = 1;
 
-	ret = of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
-	if (IS_ERR(out[0]) || IS_ERR(out[1]) || ret < 0)
-		panic("%s: clk registration failed\n", np->full_name);
+	if (err) panic("%s: clk registration failed\n", np->full_name);
 }
 CLK_OF_DECLARE(tango4_clkgen, "sigma,tango4-clkgen", tango4_clkgen_setup);

Re: [PATCH RFC] tango4 clk rewrite

[Stephen Boyd]

On 02/12, Mason wrote:
> diff --git a/drivers/clk/clk-tango4.c b/drivers/clk/clk-tango4.c
> index 004ab7dfcfe3..f8fa3955f9ee 100644
> --- a/drivers/clk/clk-tango4.c
> +++ b/drivers/clk/clk-tango4.c
> @@ -4,17 +4,26 @@
>  #include <linux/init.h>
>  #include <linux/io.h>
>  
> -static struct clk *out[2];
> -static struct clk_onecell_data clk_data = { out, 2 };
> +#define SYSCLK_DIV	0x20
> +#define CPUCLK_DIV	0x24
>  
> -#define SYSCLK_CTRL	0x20
> -#define CPUCLK_CTRL	0x24
> -#define LEGACY_DIV	0x3c
> +#define FATAL(cond) if (cond) panic("Unsupported clkgen setup!\n")

Please just use BUG_ON instead.

>  
> -#define PLL_N(val)	(((val) >>  0) & 0x7f)
> -#define PLL_K(val)	(((val) >> 13) & 0x7)
> -#define PLL_M(val)	(((val) >> 16) & 0x7)
> -#define DIV_INDEX(val)	(((val) >>  8) & 0xf)
> +#define CLK_COUNT 4 /* cpu_clk, sys_clk, usb_clk, sdio_clk */
> +static struct clk *out[CLK_COUNT];
> +static struct clk_onecell_data clk_data = { out, CLK_COUNT };
> +
> +#define EXTRACT(v,o,w) (((v) >> (o)) & ((1u << (w)) - 1))
> +
> +static inline u32 extract(u32 val, int offset, int width) {
> +	return (val >> offset) & ((1u << width) - 1);
> +}
> +
> +/*** CLKGEN_PLL ***/
> +#define PLL_N(v)	extract(v,  0, 7)
> +#define PLL_K(v)	extract(v, 13, 3)
> +#define PLL_M(v)	extract(v, 16, 3)
> +#define PLL_ISEL(v)	extract(v, 24, 3)

I would have left it like it is, but made it lower case to
signify "function" and not "constant".

#define pll_n(val)	((val) & 0x7f)
#define pll_k(val)	(((val) >> 13) & 0x7)
#define pll_m(val)	(((val) >> 16) & 0x7)
#define pll_isel(val)	(((val) >> 24) & 0x7)

Note how I didn't use _another_ macro to make writing the
shifting and anding logic common. That's because I'd like to know
what the pll_n "function" does without having to go through
another macro that does little but obscure the code.

And to make things even clearer, maybe calling them
extract_pll_n(), extract_pll_k(), etc. would make it clear from
the calls sites that we're extracting the pll n and pll k values
without having to look at the macro definitions.

>  
>  static void __init make_pll(int idx, const char *parent, void __iomem *base)
>  {
> @@ -26,36 +35,49 @@ static void __init make_pll(int idx, const char *parent, void __iomem *base)
>  	mul =  PLL_N(val) + 1;
>  	div = (PLL_M(val) + 1) << PLL_K(val);
>  	clk_register_fixed_factor(NULL, name, parent, 0, mul, div);
> +	FATAL(PLL_ISEL(val) != 1);
>  }
>  
> -static int __init get_div(void __iomem *base)
> +static void __init make_cd(int idx, void __iomem *base)
>  {
> -	u8 sysclk_tab[16] = { 2, 4, 3, 3, 3, 3, 3, 3, 4, 4, 4, 4 };
> -	int idx = DIV_INDEX(readl_relaxed(base + LEGACY_DIV));
> +	char name[8];
> +	u32 mul, div;
>  
> -	return sysclk_tab[idx];
> +	sprintf(name, "cd%d", idx);
> +	mul =  1 << 27;
> +	div = (1 << 28) + readl_relaxed(base + idx*8);

Please add spaces around '*'. Also, just do it on two lines.

	div = 1 << 28;
	div += readl_relaxed(base + idx * 8)

> +	clk_register_fixed_factor(NULL, name, "pll2", 0, mul, div);
>  }
>  
>  static void __init tango4_clkgen_setup(struct device_node *np)
>  {
> -	int div, ret;
> +	int i, err;
>  	void __iomem *base = of_iomap(np, 0);
>  	const char *parent = of_clk_get_parent_name(np, 0);
>  
>  	if (!base)
>  		panic("%s: invalid address\n", np->full_name);
>  
> +	FATAL(readl_relaxed(base + CPUCLK_DIV) & BIT(23));
> +	FATAL(readl_relaxed(base + SYSCLK_DIV) & BIT(23));

BUG_ON would read much better here. Plus a comment on what
BIT(23) means or a macro definition of it would make it clear
what we're testing for.

> +
>  	make_pll(0, parent, base);
>  	make_pll(1, parent, base);
> +	make_pll(2, parent, base);
> +	make_cd(2, base + 0x80);
> +	make_cd(6, base + 0x80);
> +
> +	out[0] = clk_register_divider(NULL, "cpu_clk", "pll0", 0,
> +			base + CPUCLK_DIV, 8, 8, CLK_DIVIDER_ONE_BASED, NULL);
> +	out[1] = clk_register_fixed_factor(NULL, "sys_clk", "pll1", 0, 1, 4);
> +	out[2] = clk_register_fixed_factor(NULL,  "usb_clk", "cd2", 0, 1, 2);
> +	out[3] = clk_register_fixed_factor(NULL, "sdio_clk", "cd6", 0, 1, 2);
>  
> -	out[0] = clk_register_divider(NULL, "cpuclk", "pll0", 0,
> -			base + CPUCLK_CTRL, 8, 8, CLK_DIVIDER_ONE_BASED, NULL);
> +	err = of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
>  
> -	div = readl_relaxed(base + SYSCLK_CTRL) & BIT(23) ? get_div(base) : 4;
> -	out[1] = clk_register_fixed_factor(NULL, "sysclk", "pll1", 0, 1, div);
> +	for (i = 0; i < CLK_COUNT; ++i)
> +		if (IS_ERR(out[i])) err = 1;

Please format code properly.

	for (i = 0; i < CLK_COUNT; ++i)
		if (IS_ERR(out[i]))
			err = 1;

>  
> -	ret = of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
> -	if (IS_ERR(out[0]) || IS_ERR(out[1]) || ret < 0)
> -		panic("%s: clk registration failed\n", np->full_name);
> +	if (err) panic("%s: clk registration failed\n", np->full_name);
>  }
>  CLK_OF_DECLARE(tango4_clkgen, "sigma,tango4-clkgen", tango4_clkgen_setup);
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH RFC] tango4 clk rewrite

[Mason]

Hello Stephen,

Thanks for taking a look. Hope either you or Michael will be
available to discuss some of the issues you raised.

On 25/02/2016 23:08, Stephen Boyd wrote:

> On 02/12, Mason wrote:
>
>> +#define FATAL(cond) if (cond) panic("Unsupported clkgen setup!\n")
> 
> Please just use BUG_ON instead.

The issue (from my POV) is that, AFAICT, BUG_ON() can be disabled.
If one compiles a CONFIG_BUG=n kernel, then BUG_ON() is a NOP, right?

But, if the firmware set the clock generator up in a way not supported
by the driver, then bad things will happen, should the kernel be allowed
to proceed, so panic() here seems necessary.

>> +#define PLL_N(v)	extract(v,  0, 7)
>> +#define PLL_K(v)	extract(v, 13, 3)
>> +#define PLL_M(v)	extract(v, 16, 3)
>> +#define PLL_ISEL(v)	extract(v, 24, 3)
> 
> I would have left it like it is, but made it lower case to
> signify "function" and not "constant".

OK, I can change the function-macros to lower case.

> #define pll_n(val)	((val) & 0x7f)
> #define pll_k(val)	(((val) >> 13) & 0x7)
> #define pll_m(val)	(((val) >> 16) & 0x7)
> #define pll_isel(val)	(((val) >> 24) & 0x7)
> 
> Note how I didn't use _another_ macro to make writing the
> shifting and anding logic common. That's because I'd like to know
> what the pll_n "function" does without having to go through
> another macro that does little but obscure the code.

My rationale was: the documentation specifies offset and width
for the fields. And the code should mirror the documentation
(leaving the arithmetic to the compiler).

BTW, my preferred version used bit-fields:
#define REG(name, ...) union name { struct { u32 __VA_ARGS__; }; u32 val; }
REG(SYS_clkgen_pll, N:7, :6, K:3, M:3, :5, Isel:3, :3, T:1, B:1);

But you wrote:
"This is new to me. Using bitfields like this is not really a good
idea though. Please just use masks, shifts, etc. instead."

IMHO, this is one place where bit-fields /are/ in fact useful.

> And to make things even clearer, maybe calling them
> extract_pll_n(), extract_pll_k(), etc. would make it clear from
> the calls sites that we're extracting the pll n and pll k values
> without having to look at the macro definitions.

OK, I can change the names.

>> +	div = (1 << 28) + readl_relaxed(base + idx*8);
> 
> Please add spaces around '*'. Also, just do it on two lines.
> 
> 	div = 1 << 28;
> 	div += readl_relaxed(base + idx * 8)

Is that because of the function-like macro?

>> +	FATAL(readl_relaxed(base + CPUCLK_DIV) & BIT(23));
>> +	FATAL(readl_relaxed(base + SYSCLK_DIV) & BIT(23));
> 
> BUG_ON would read much better here. Plus a comment on what
> BIT(23) means or a macro definition of it would make it clear
> what we're testing for.

There is a problem if BUG_ON() is a NOP.

I can indeed define BIT(23) as BYPASS_ENABLE.

Thanks again for reviewing, hope to hear back from you.

Regards.

[PATCH v2] clk: tango4: improve clkgen driver

[Mason]

From: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>

Add support for USB and SDIO clocks.
Flag unsupported setups.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
 drivers/clk/clk-tango4.c | 71 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 46 insertions(+), 25 deletions(-)

diff --git a/drivers/clk/clk-tango4.c b/drivers/clk/clk-tango4.c
index 004ab7dfcfe3..004ddf8c8950 100644
--- a/drivers/clk/clk-tango4.c
+++ b/drivers/clk/clk-tango4.c
@@ -4,17 +4,19 @@
 #include <linux/init.h>
 #include <linux/io.h>
 
-static struct clk *out[2];
-static struct clk_onecell_data clk_data = { out, 2 };
+#define CLK_COUNT 4 /* cpu_clk, sys_clk, usb_clk, sdio_clk */
+static struct clk *clks[CLK_COUNT];
+static struct clk_onecell_data clk_data = { clks, CLK_COUNT };
 
-#define SYSCLK_CTRL	0x20
-#define CPUCLK_CTRL	0x24
-#define LEGACY_DIV	0x3c
+#define SYSCLK_DIV	0x20
+#define CPUCLK_DIV	0x24
+#define DIV_BYPASS	BIT(23)
 
-#define PLL_N(val)	(((val) >>  0) & 0x7f)
-#define PLL_K(val)	(((val) >> 13) & 0x7)
-#define PLL_M(val)	(((val) >> 16) & 0x7)
-#define DIV_INDEX(val)	(((val) >>  8) & 0xf)
+/*** CLKGEN_PLL ***/
+#define extract_pll_n(val)	((val >>  0) & ((1u << 7) - 1))
+#define extract_pll_k(val)	((val >> 13) & ((1u << 3) - 1))
+#define extract_pll_m(val)	((val >> 16) & ((1u << 3) - 1))
+#define extract_pll_isel(val)	((val >> 24) & ((1u << 3) - 1))
 
 static void __init make_pll(int idx, const char *parent, void __iomem *base)
 {
@@ -22,40 +24,59 @@ static void __init make_pll(int idx, const char *parent, void __iomem *base)
 	u32 val, mul, div;
 
 	sprintf(name, "pll%d", idx);
-	val = readl_relaxed(base + idx*8);
-	mul =  PLL_N(val) + 1;
-	div = (PLL_M(val) + 1) << PLL_K(val);
+	val = readl_relaxed(base + idx * 8);
+	mul =  extract_pll_n(val) + 1;
+	div = (extract_pll_m(val) + 1) << extract_pll_k(val);
 	clk_register_fixed_factor(NULL, name, parent, 0, mul, div);
+	if (extract_pll_isel(val) != 1)
+		panic("%s: input not set to XTAL_IN\n", name);
 }
 
-static int __init get_div(void __iomem *base)
+static void __init make_cd(int idx, void __iomem *base)
 {
-	u8 sysclk_tab[16] = { 2, 4, 3, 3, 3, 3, 3, 3, 4, 4, 4, 4 };
-	int idx = DIV_INDEX(readl_relaxed(base + LEGACY_DIV));
+	char name[8];
+	u32 val, mul, div;
 
-	return sysclk_tab[idx];
+	sprintf(name, "cd%d", idx);
+	val = readl_relaxed(base + idx * 8);
+	mul =  1 << 27;
+	div = (2 << 27) + val;
+	clk_register_fixed_factor(NULL, name, "pll2", 0, mul, div);
+	if (val > 0xf0000000)
+		panic("%s: unsupported divider %x\n", name, val);
 }
 
 static void __init tango4_clkgen_setup(struct device_node *np)
 {
-	int div, ret;
+	struct clk **pp = clk_data.clks;
 	void __iomem *base = of_iomap(np, 0);
 	const char *parent = of_clk_get_parent_name(np, 0);
 
 	if (!base)
-		panic("%s: invalid address\n", np->full_name);
+		panic("%s: invalid address\n", np->name);
+
+	if (readl_relaxed(base + CPUCLK_DIV) & DIV_BYPASS)
+		panic("%s: unsupported cpuclk setup\n", np->name);
+
+	if (readl_relaxed(base + SYSCLK_DIV) & DIV_BYPASS)
+		panic("%s: unsupported sysclk setup\n", np->name);
 
 	make_pll(0, parent, base);
 	make_pll(1, parent, base);
+	make_pll(2, parent, base);
+	make_cd(2, base + 0x80);
+	make_cd(6, base + 0x80);
 
-	out[0] = clk_register_divider(NULL, "cpuclk", "pll0", 0,
-			base + CPUCLK_CTRL, 8, 8, CLK_DIVIDER_ONE_BASED, NULL);
+	pp[0] = clk_register_divider(NULL, "cpu_clk", "pll0", 0,
+			base + CPUCLK_DIV, 8, 8, CLK_DIVIDER_ONE_BASED, NULL);
+	pp[1] = clk_register_fixed_factor(NULL, "sys_clk", "pll1", 0, 1, 4);
+	pp[2] = clk_register_fixed_factor(NULL,  "usb_clk", "cd2", 0, 1, 2);
+	pp[3] = clk_register_fixed_factor(NULL, "sdio_clk", "cd6", 0, 1, 2);
 
-	div = readl_relaxed(base + SYSCLK_CTRL) & BIT(23) ? get_div(base) : 4;
-	out[1] = clk_register_fixed_factor(NULL, "sysclk", "pll1", 0, 1, div);
+	if (IS_ERR(pp[0]) || IS_ERR(pp[1]) || IS_ERR(pp[2]) || IS_ERR(pp[3]))
+		panic("%s: clk registration failed\n", np->name);
 
-	ret = of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
-	if (IS_ERR(out[0]) || IS_ERR(out[1]) || ret < 0)
-		panic("%s: clk registration failed\n", np->full_name);
+	if (of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data))
+		panic("%s: clk provider registration failed\n", np->name);
 }
 CLK_OF_DECLARE(tango4_clkgen, "sigma,tango4-clkgen", tango4_clkgen_setup);
-- 
2.7.0

[PATCH v3] clk: tango4: improve clkgen driver

[Mason]

From: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>

Add support for USB and SDIO clocks.
Report unsupported setups and panic.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
 drivers/clk/clk-tango4.c | 73 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 48 insertions(+), 25 deletions(-)

diff --git a/drivers/clk/clk-tango4.c b/drivers/clk/clk-tango4.c
index 004ab7dfcfe3..eef75e305a59 100644
--- a/drivers/clk/clk-tango4.c
+++ b/drivers/clk/clk-tango4.c
@@ -4,17 +4,19 @@
 #include <linux/init.h>
 #include <linux/io.h>
 
-static struct clk *out[2];
-static struct clk_onecell_data clk_data = { out, 2 };
+#define CLK_COUNT 4 /* cpu_clk, sys_clk, usb_clk, sdio_clk */
+static struct clk *clks[CLK_COUNT];
+static struct clk_onecell_data clk_data = { clks, CLK_COUNT };
 
-#define SYSCLK_CTRL	0x20
-#define CPUCLK_CTRL	0x24
-#define LEGACY_DIV	0x3c
+#define SYSCLK_DIV	0x20
+#define CPUCLK_DIV	0x24
+#define DIV_BYPASS	BIT(23)
 
-#define PLL_N(val)	(((val) >>  0) & 0x7f)
-#define PLL_K(val)	(((val) >> 13) & 0x7)
-#define PLL_M(val)	(((val) >> 16) & 0x7)
-#define DIV_INDEX(val)	(((val) >>  8) & 0xf)
+/*** CLKGEN_PLL ***/
+#define extract_pll_n(val)	((val >>  0) & ((1u << 7) - 1))
+#define extract_pll_k(val)	((val >> 13) & ((1u << 3) - 1))
+#define extract_pll_m(val)	((val >> 16) & ((1u << 3) - 1))
+#define extract_pll_isel(val)	((val >> 24) & ((1u << 3) - 1))
 
 static void __init make_pll(int idx, const char *parent, void __iomem *base)
 {
@@ -22,40 +24,61 @@ static void __init make_pll(int idx, const char *parent, void __iomem *base)
 	u32 val, mul, div;
 
 	sprintf(name, "pll%d", idx);
-	val = readl_relaxed(base + idx*8);
-	mul =  PLL_N(val) + 1;
-	div = (PLL_M(val) + 1) << PLL_K(val);
+	val = readl(base + idx * 8);
+	mul =  extract_pll_n(val) + 1;
+	div = (extract_pll_m(val) + 1) << extract_pll_k(val);
 	clk_register_fixed_factor(NULL, name, parent, 0, mul, div);
+	if (extract_pll_isel(val) != 1)
+		panic("%s: input not set to XTAL_IN\n", name);
 }
 
-static int __init get_div(void __iomem *base)
+static void __init make_cd(int idx, void __iomem *base)
 {
-	u8 sysclk_tab[16] = { 2, 4, 3, 3, 3, 3, 3, 3, 4, 4, 4, 4 };
-	int idx = DIV_INDEX(readl_relaxed(base + LEGACY_DIV));
+	char name[8];
+	u32 val, mul, div;
 
-	return sysclk_tab[idx];
+	sprintf(name, "cd%d", idx);
+	val = readl(base + idx * 8);
+	mul =  1 << 27;
+	div = (2 << 27) + val;
+	clk_register_fixed_factor(NULL, name, "pll2", 0, mul, div);
+	if (val > 0xf0000000)
+		panic("%s: unsupported divider %x\n", name, val);
 }
 
 static void __init tango4_clkgen_setup(struct device_node *np)
 {
-	int div, ret;
+	struct clk **pp = clk_data.clks;
 	void __iomem *base = of_iomap(np, 0);
 	const char *parent = of_clk_get_parent_name(np, 0);
 
 	if (!base)
-		panic("%s: invalid address\n", np->full_name);
+		panic("%s: invalid address\n", np->name);
+
+	if (readl(base + CPUCLK_DIV) & DIV_BYPASS)
+		panic("%s: unsupported cpuclk setup\n", np->name);
+
+	if (readl(base + SYSCLK_DIV) & DIV_BYPASS)
+		panic("%s: unsupported sysclk setup\n", np->name);
+
+	writel(0x100, base + CPUCLK_DIV); /* disable frequency ramping */
 
 	make_pll(0, parent, base);
 	make_pll(1, parent, base);
+	make_pll(2, parent, base);
+	make_cd(2, base + 0x80);
+	make_cd(6, base + 0x80);
 
-	out[0] = clk_register_divider(NULL, "cpuclk", "pll0", 0,
-			base + CPUCLK_CTRL, 8, 8, CLK_DIVIDER_ONE_BASED, NULL);
+	pp[0] = clk_register_divider(NULL, "cpu_clk", "pll0", 0,
+			base + CPUCLK_DIV, 8, 8, CLK_DIVIDER_ONE_BASED, NULL);
+	pp[1] = clk_register_fixed_factor(NULL, "sys_clk", "pll1", 0, 1, 4);
+	pp[2] = clk_register_fixed_factor(NULL,  "usb_clk", "cd2", 0, 1, 2);
+	pp[3] = clk_register_fixed_factor(NULL, "sdio_clk", "cd6", 0, 1, 2);
 
-	div = readl_relaxed(base + SYSCLK_CTRL) & BIT(23) ? get_div(base) : 4;
-	out[1] = clk_register_fixed_factor(NULL, "sysclk", "pll1", 0, 1, div);
+	if (IS_ERR(pp[0]) || IS_ERR(pp[1]) || IS_ERR(pp[2]) || IS_ERR(pp[3]))
+		panic("%s: clk registration failed\n", np->name);
 
-	ret = of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
-	if (IS_ERR(out[0]) || IS_ERR(out[1]) || ret < 0)
-		panic("%s: clk registration failed\n", np->full_name);
+	if (of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data))
+		panic("%s: clk provider registration failed\n", np->name);
 }
 CLK_OF_DECLARE(tango4_clkgen, "sigma,tango4-clkgen", tango4_clkgen_setup);
-- 
2.8.0

Re: [PATCH v3] clk: tango4: improve clkgen driver

[Mason]

On 04/04/2016 11:21, Mason wrote:

> Add support for USB and SDIO clocks.
> Report unsupported setups and panic.
> 
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
>  drivers/clk/clk-tango4.c | 73 +++++++++++++++++++++++++++++++-----------------
>  1 file changed, 48 insertions(+), 25 deletions(-)

Are there rough edges remaining with this patch?

Regards.

Re: [PATCH v3] clk: tango4: improve clkgen driver

[Stephen Boyd]

On 04/04, Mason wrote:
> From: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> 
> Add support for USB and SDIO clocks.
> Report unsupported setups and panic.
> 
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3] clk: tango4: improve clkgen driver

[Mason]

On 16/04/2016 02:16, Stephen Boyd wrote:

> Applied to clk-next

Great! Thanks.

Hopefully, I won't have to write any more clk code until Tango5.

The last feature missing is support for fractional dividers.
We discussed this back in March on IRC, and you mentioned
"libifying" some parts of the code.

Could you say a bit more to put me on the right track?

Regards.

Re: [PATCH v3] clk: tango4: improve clkgen driver

[Stephen Boyd]

On 04/16, Mason wrote:
> On 16/04/2016 02:16, Stephen Boyd wrote:
> 
> > Applied to clk-next
> 
> Great! Thanks.
> 
> Hopefully, I won't have to write any more clk code until Tango5.
> 
> The last feature missing is support for fractional dividers.
> We discussed this back in March on IRC, and you mentioned
> "libifying" some parts of the code.
> 
> Could you say a bit more to put me on the right track?

I mean that clk-fractional-divider.c can become more like
clk-divider.c and grow functions like
fractional_divider_round_rate(), fractional_divider_get_val(),
and fractional_divider_recalc_rate() that would parallel
divider_round_rate(), divider_get_val() and divider_recalc_rate()
respectively. If any sort of flags are needed to change rounding
behavior, etc. then a flags argument could be used similar to how
the divider code has one.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project