tango4 clk rewrite

RFC PATCH In review Mason 2 participants 9 messages Feb 12, 2016 – May 3, 2016 Common clk

lore

Share & tools

lore

Mason

author

2016-02-12 16:58 UTC #0 lore raw

RFC PATCH 1/1 tango4 clk rewrite

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(-)

drivers/clk/clk-tango4.c +43 ~~-21~~

index 004ab7dfcfe3..f8fa3955f9ee 100644
@@ -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);

Stephen Boyd

2016-02-25 22:08 UTC #1 lore raw

Quoted 7 lines · Mason

drivers/clk/clk-tango4.c +0 -0

index 004ab7dfcfe3..f8fa3955f9ee 100644
@@ -4,17 +4,26 @@
 #include <linux/init.h>
 #include <linux/io.h>

drivers/clk/clk-tango4.c line 10

Quoted 10 lines

drivers/clk/clk-tango4.c +3 -5

 
-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.

Quoted 10 lines

drivers/clk/clk-tango4.c +5 -4

-#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))

drivers/clk/clk-tango4.c line 26

Quoted 10 lines

drivers/clk/clk-tango4.c +10 -0

+
+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.

Quoted 12 lines

drivers/clk/clk-tango4.c +2 -1

 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)

drivers/clk/clk-tango4.c static void __init make_pll(int idx, const char *parent, void __iomem *base) · line 48

Quoted 10 lines

drivers/clk/clk-tango4.c +5 -3

 {
-	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)

Quoted 5 lines

drivers/clk/clk-tango4.c +1 -0

+	clk_register_fixed_factor(NULL, name, "pll2", 0, mul, div);
 }
 
 static void __init tango4_clkgen_setup(struct device_node *np)
 {

drivers/clk/clk-tango4.c static void __init make_pll(int idx, const char *parent, void __iomem *base) · line 62

Quoted 10 lines

drivers/clk/clk-tango4.c +3 -1

-	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.

Quoted 11 lines

drivers/clk/clk-tango4.c +9 -0

+
 	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);

drivers/clk/clk-tango4.c static void __init make_pll(int idx, const char *parent, void __iomem *base) · line 79

Quoted 10 lines

drivers/clk/clk-tango4.c +4 -4

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

Quoted 8 lines

drivers/clk/clk-tango4.c +1 -3

-	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);

Signature · 2 lines

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

Mason

author

2016-02-26 10:51 UTC #2 lore raw

Hello Stephen,

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

Quoted 5 lines · Stephen Boyd

Quoted 1 line · Mason
+#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.

Quoted 7 lines

drivers/clk/clk-tango4.c line 26
Quoted 4 lines
drivers/clk/clk-tango4.c +4 -0
+#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.

Quoted 9 lines

#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.

Quoted 4 lines

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.

Quoted 6 lines

drivers/clk/clk-tango4.c static void __init make_pll(int idx, const char *parent, void __iomem *base) · line 48
Quoted 1 line
drivers/clk/clk-tango4.c +1 -0
+	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?

Quoted 6 lines

drivers/clk/clk-tango4.c static void __init make_pll(int idx, const char *parent, void __iomem *base) · line 62
Quoted 2 lines
drivers/clk/clk-tango4.c +2 -0
+	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.

Mason

author

2016-02-26 14:52 UTC #3 lore raw

PATCH v2 1/1 clk: tango4: improve clkgen driver

In reply to Stephen Boyd

From: Marc Gonzalez <[email protected]>

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

Signed-off-by Marc Gonzalez <[email protected]>

---
 drivers/clk/clk-tango4.c | 71 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 46 insertions(+), 25 deletions(-)

drivers/clk/clk-tango4.c +46 ~~-25~~

index 004ab7dfcfe3..004ddf8c8950 100644
@@ -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);

Signature · 1 line

2.7.0

Mason

author

2016-04-04 09:21 UTC #4 lore raw

PATCH v3 1/1 clk: tango4: improve clkgen driver

From: Marc Gonzalez <[email protected]>

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

Signed-off-by Marc Gonzalez <[email protected]>

---
 drivers/clk/clk-tango4.c | 73 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 48 insertions(+), 25 deletions(-)

drivers/clk/clk-tango4.c +48 ~~-25~~

index 004ab7dfcfe3..eef75e305a59 100644
@@ -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);

Signature · 1 line

2.8.0

Mason

author

2016-04-13 20:44 UTC #5 lore raw

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

Quoted 7 lines · Mason

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

Are there rough edges remaining with this patch?

Regards.

Stephen Boyd

Applied

2016-04-16 00:16 UTC #6 lore raw

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

In reply to Mason

Quoted 7 lines · Mason

From: Marc Gonzalez <[email protected]>
Add support for USB and SDIO clocks.
Report unsupported setups and panic.
Signed-off-by Marc Gonzalez <[email protected]>
---

Applied to clk-next

Signature · 2 lines

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

Mason

author

2016-04-16 08:35 UTC #7 lore raw

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

Quoted 1 line · Stephen Boyd

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.

Stephen Boyd

2016-05-03 00:36 UTC #8 lore raw

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

Quoted 6 lines · Mason

Quoted 1 line · Stephen Boyd
Applied to clk-next
Great! Thanks.

Quoted 7 lines

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.

Signature · 2 lines

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