[PATCH v3] clk: tango4: clkgen driver for Tango4 ARM platforms

[PATCH v3] clk: tango4: clkgen driver for Tango4 ARM platforms

[Marc Gonzalez]

Add support for Sigma Designs Tango4 (ARM-based) clock generator.
NOTE: This driver is incompatible with Tango3 clkgen.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
Changes in v3:
Provide binding documentation
Don't unconditionally use hard-coded divider for sysclk

Changes in v2:
Provide missing includes
Use masks instead of bit-fields
Error handling
Style nits
Model the clkgen block as a single node
---
 .../devicetree/bindings/clock/tango4-clock.txt     | 27 ++++++++++
 drivers/clk/Makefile                               |  1 +
 drivers/clk/clk-tango4.c                           | 57 ++++++++++++++++++++++
 3 files changed, 85 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/tango4-clock.txt
 create mode 100644 drivers/clk/clk-tango4.c

diff --git a/Documentation/devicetree/bindings/clock/tango4-clock.txt b/Documentation/devicetree/bindings/clock/tango4-clock.txt
new file mode 100644
index 000000000000..b60dba5f1107
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/tango4-clock.txt
@@ -0,0 +1,27 @@
+* Sigma Designs Tango4 Clock Generator
+
+The Tango4 clock generator outputs cpu_clk and sys_clk (the latter is used
+for RAM and various peripheral devices). The clock binding described here
+is applicable to all Tango4 SoCs.
+
+Required Properties:
+
+- compatible: shall be "sigma,tango4-clkgen".
+
+- reg: physical base address of the device and length of memory mapped region.
+
+- clocks: phandle of the input (crystal oscillator).
+
+- clock-output-names: shall be "cpuclk" and "sysclk".
+
+- #clock-cells: shall be set to 1.
+
+Example:
+
+	clkgen: clkgen@10000 {
+		compatible = "sigma,tango4-clkgen";
+		reg = <0x10000 0x40>;
+		clocks = <&xtal>;
+		clock-output-names = "cpuclk", "sysclk";
+		#clock-cells = <1>;
+	};
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index c4cf075a2320..60f42251d32a 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -40,6 +40,7 @@ obj-$(CONFIG_COMMON_CLK_SI5351)		+= clk-si5351.o
 obj-$(CONFIG_COMMON_CLK_SI570)		+= clk-si570.o
 obj-$(CONFIG_COMMON_CLK_CDCE925)	+= clk-cdce925.o
 obj-$(CONFIG_ARCH_STM32)		+= clk-stm32f4.o
+obj-$(CONFIG_ARCH_TANGOX)		+= clk-tango4.o
 obj-$(CONFIG_CLK_TWL6040)		+= clk-twl6040.o
 obj-$(CONFIG_ARCH_U300)			+= clk-u300.o
 obj-$(CONFIG_ARCH_VT8500)		+= clk-vt8500.o
diff --git a/drivers/clk/clk-tango4.c b/drivers/clk/clk-tango4.c
new file mode 100644
index 000000000000..9fbee8ce4a95
--- /dev/null
+++ b/drivers/clk/clk-tango4.c
@@ -0,0 +1,57 @@
+#include <linux/clk-provider.h>
+#include <linux/of_address.h>
+#include <linux/init.h>
+#include <linux/io.h>
+
+static struct clk *out[2];
+static struct clk_onecell_data clk_data = { out, 2 };
+static void __iomem *clkgen_base;
+
+#define sysclk_div (clkgen_base + 0x20)
+#define cpuclk_div (clkgen_base + 0x24)
+
+#define PLL_N(val) ((val) >>  0 & 0x7f)
+#define PLL_K(val) ((val) >> 13 & 0x07)
+#define PLL_M(val) ((val) >> 16 & 0x07)
+
+#define DIV_INDEX ((readl_relaxed(clkgen_base + 0x3c) >> 8) & 15)
+
+static void __init make_pll(const char *name, const char *parent, int offset)
+{
+	unsigned int val, mul, div;
+
+	val = readl_relaxed(clkgen_base + offset);
+	mul =  PLL_N(val) + 1;
+	div = (PLL_M(val) + 1) << PLL_K(val);
+	clk_register_fixed_factor(NULL, name, parent, 0, mul, div);
+}
+
+static const u8 tab[16] __initconst = { 2, 4, 3, 3, 3, 3, 3, 3, 4, 4, 4, 4 };
+
+static void __init tango4_clkgen_setup(struct device_node *np)
+{
+	int ret, div;
+	const char *name = NULL;
+	const char *parent = of_clk_get_parent_name(np, 0);
+
+	clkgen_base = of_iomap(np, 0);
+	if (clkgen_base == NULL)
+		panic("%s: invalid address\n", np->full_name);
+
+	make_pll("pll0", parent, 0);
+	make_pll("pll1", parent, 8);
+
+	of_property_read_string_index(np, "clock-output-names", 0, &name);
+	out[0] = clk_register_divider(NULL, name, "pll0", 0,
+			cpuclk_div, 8, 8, CLK_DIVIDER_ONE_BASED, NULL);
+
+	div = readl_relaxed(sysclk_div) & BIT(23) ? tab[DIV_INDEX] : 4;
+	of_property_read_string_index(np, "clock-output-names", 1, &name);
+	out[1] = clk_register_fixed_factor(NULL, name, "pll1", 0, 1, div);
+
+	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);
+}
+
+CLK_OF_DECLARE(tango4_clkgen, "sigma,tango4-clkgen", tango4_clkgen_setup);
-- 
2.4.5

Re: [PATCH v3] clk: tango4: clkgen driver for Tango4 ARM platforms

[Marc Gonzalez]

Hello Michael & Stephen,

On 23/10/2015 17:40, Marc Gonzalez wrote:

> Add support for Sigma Designs Tango4 (ARM-based) clock generator.
> NOTE: This driver is incompatible with Tango3 clkgen.
> 
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
> Changes in v3:
> Provide binding documentation
> Don't unconditionally use hard-coded divider for sysclk
> 
> Changes in v2:
> Provide missing includes
> Use masks instead of bit-fields
> Error handling
> Style nits
> Model the clkgen block as a single node
> ---
>  .../devicetree/bindings/clock/tango4-clock.txt     | 27 ++++++++++
>  drivers/clk/Makefile                               |  1 +
>  drivers/clk/clk-tango4.c                           | 57 ++++++++++++++++++++++
>  3 files changed, 85 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/tango4-clock.txt
>  create mode 100644 drivers/clk/clk-tango4.c

Are there any remaining issues that have not been addressed?
(A cosmetic change: should I remove the double-spacing in the
"Required Properties" list of the binding document?)

I would need to send a v4, with a minor tweak of the commit message,
as I found out that there has existed one non-ARM Tango4 SoC.
(But it's not clear whether it ever went further than eval boards.)
I would bundle that change along with any additional required fixes.

How close/far is this driver from being accepted?

Regards.

Re: [PATCH v3] clk: tango4: clkgen driver for Tango4 ARM platforms

[Stephen Boyd]

On 10/23, Marc Gonzalez wrote:
> diff --git a/drivers/clk/clk-tango4.c b/drivers/clk/clk-tango4.c
> new file mode 100644
> index 000000000000..9fbee8ce4a95
> --- /dev/null
> +++ b/drivers/clk/clk-tango4.c
> @@ -0,0 +1,57 @@
> +#include <linux/clk-provider.h>
> +#include <linux/of_address.h>

#include <linux/of.h> for of_property_read_string_index() ?

> +#include <linux/init.h>
> +#include <linux/io.h>
> +
> +static struct clk *out[2];
> +static struct clk_onecell_data clk_data = { out, 2 };
> +static void __iomem *clkgen_base;

This can be a local variable that's passed to the make_pll
function.

> +
> +#define sysclk_div (clkgen_base + 0x20)
> +#define cpuclk_div (clkgen_base + 0x24)

And these macros can be numbers only.

> +
> +#define PLL_N(val) ((val) >>  0 & 0x7f)
> +#define PLL_K(val) ((val) >> 13 & 0x07)
> +#define PLL_M(val) ((val) >> 16 & 0x07)
> +
> +#define DIV_INDEX ((readl_relaxed(clkgen_base + 0x3c) >> 8) & 15)

Don't do this. It's just confusing. I read the usage of this
macro below and thought this was a constant. 0x3c should be some
other #define and then DIV_INDEX should be:

	#define DIV_INDEX(val) (((val) >> 8) & 0x15)

> +
> +static void __init make_pll(const char *name, const char *parent, int offset)
> +{
> +	unsigned int val, mul, div;

val should be u32 type. mul and div can be unsigned int.

> +
> +	val = readl_relaxed(clkgen_base + offset);
> +	mul =  PLL_N(val) + 1;
> +	div = (PLL_M(val) + 1) << PLL_K(val);
> +	clk_register_fixed_factor(NULL, name, parent, 0, mul, div);
> +}
> +
> +static const u8 tab[16] __initconst = { 2, 4, 3, 3, 3, 3, 3, 3, 4, 4, 4, 4 };
> +
> +static void __init tango4_clkgen_setup(struct device_node *np)
> +{
> +	int ret, div;
> +	const char *name = NULL;
> +	const char *parent = of_clk_get_parent_name(np, 0);
> +
> +	clkgen_base = of_iomap(np, 0);
> +	if (clkgen_base == NULL)

if (!clkgen_base) is more common/desired.

> +		panic("%s: invalid address\n", np->full_name);
> +
> +	make_pll("pll0", parent, 0);
> +	make_pll("pll1", parent, 8);
> +
> +	of_property_read_string_index(np, "clock-output-names", 0, &name);

Do we even need to do this? The clock names can come from the
driver because this is tango4 specific code.

> +	out[0] = clk_register_divider(NULL, name, "pll0", 0,
> +			cpuclk_div, 8, 8, CLK_DIVIDER_ONE_BASED, NULL);

I'd rather see clkgen_base + CPUCLK_DIV. Macro names are
typically uppercase.

> +
> +	div = readl_relaxed(sysclk_div) & BIT(23) ? tab[DIV_INDEX] : 4;
> +	of_property_read_string_index(np, "clock-output-names", 1, &name);
> +	out[1] = clk_register_fixed_factor(NULL, name, "pll1", 0, 1, div);
> +
> +	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);
> +}
> +
> +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

[PATCH v4] clk: tango4: clkgen driver for Tango4 platforms

[Marc Gonzalez]

Provide support for Sigma Designs Tango4 clock generator.
NOTE: This driver is incompatible with Tango3 clkgen.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
Changes in v4:
Make base a local variable
Don't use of_property_read_string_index()
Change macros to constant offsets, and use base + FOO
Style nits

Changes in v3:
Provide binding documentation
Don't unconditionally use hard-coded divider for sysclk

Changes in v2:
Provide missing includes
Use masks instead of bit-fields
Error handling
Style nits
Model the clkgen block as a single node
--- .../devicetree/bindings/clock/tango4-clock.txt     | 23 ++++++++
 drivers/clk/Makefile                               |  1 +
 drivers/clk/clk-tango4.c                           | 61 ++++++++++++++++++++++
 3 files changed, 85 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/tango4-clock.txt
 create mode 100644 drivers/clk/clk-tango4.c

diff --git a/Documentation/devicetree/bindings/clock/tango4-clock.txt b/Documentation/devicetree/bindings/clock/tango4-clock.txt
new file mode 100644
index 000000000000..19c580a7bda2
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/tango4-clock.txt
@@ -0,0 +1,23 @@
+* Sigma Designs Tango4 Clock Generator
+
+The Tango4 clock generator outputs cpu_clk and sys_clk (the latter is used
+for RAM and various peripheral devices). The clock binding described here
+is applicable to all Tango4 SoCs.
+
+Required Properties:
+
+- compatible: should be "sigma,tango4-clkgen".
+- reg: physical base address of the device and length of memory mapped region.
+- clocks: phandle of the input clock (crystal oscillator).
+- clock-output-names: should be "cpuclk" and "sysclk".
+- #clock-cells: should be set to 1.
+
+Example:
+
+	clkgen: clkgen@10000 {
+		compatible = "sigma,tango4-clkgen";
+		reg = <0x10000 0x40>;
+		clocks = <&xtal>;
+		clock-output-names = "cpuclk", "sysclk";
+		#clock-cells = <1>;
+	};
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index c4cf075a2320..60f42251d32a 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -40,6 +40,7 @@ obj-$(CONFIG_COMMON_CLK_SI5351)		+= clk-si5351.o
 obj-$(CONFIG_COMMON_CLK_SI570)		+= clk-si570.o
 obj-$(CONFIG_COMMON_CLK_CDCE925)	+= clk-cdce925.o
 obj-$(CONFIG_ARCH_STM32)		+= clk-stm32f4.o
+obj-$(CONFIG_ARCH_TANGOX)		+= clk-tango4.o
 obj-$(CONFIG_CLK_TWL6040)		+= clk-twl6040.o
 obj-$(CONFIG_ARCH_U300)			+= clk-u300.o
 obj-$(CONFIG_ARCH_VT8500)		+= clk-vt8500.o
diff --git a/drivers/clk/clk-tango4.c b/drivers/clk/clk-tango4.c
new file mode 100644
index 000000000000..1ef192289fd9
--- /dev/null
+++ b/drivers/clk/clk-tango4.c
@@ -0,0 +1,61 @@
+#include <linux/clk-provider.h>
+#include <linux/of_address.h>
+#include <linux/init.h>
+#include <linux/io.h>
+
+static struct clk *out[2];
+static struct clk_onecell_data clk_data = { out, 2 };
+
+#define SYSCLK_CTRL	0x20
+#define CPUCLK_CTRL	0x24
+#define LEGACY_DIV	0x3c
+
+#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)
+
+static void __init make_pll(int idx, const char *parent, void __iomem *base)
+{
+	char name[8];
+	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);
+	clk_register_fixed_factor(NULL, name, parent, 0, mul, div);
+}
+
+static int __init get_div(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));
+
+	return sysclk_tab[idx];
+}
+
+static void __init tango4_clkgen_setup(struct device_node *np)
+{
+	int div, ret;
+	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);
+
+	make_pll(0, parent, base);
+	make_pll(1, parent, base);
+
+	out[0] = clk_register_divider(NULL, "cpuclk", "pll0", 0,
+			base + CPUCLK_CTRL, 8, 8, CLK_DIVIDER_ONE_BASED, NULL);
+
+	div = readl_relaxed(base + SYSCLK_CTRL) & BIT(23) ? get_div(base) : 4;
+	out[1] = clk_register_fixed_factor(NULL, "sysclk", "pll1", 0, 1, div);
+
+	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);
+}
+
+CLK_OF_DECLARE(tango4_clkgen, "sigma,tango4-clkgen", tango4_clkgen_setup);
-- 
2.4.5

Re: [PATCH v4] clk: tango4: clkgen driver for Tango4 platforms

[Stephen Boyd]

On 10/30, Marc Gonzalez wrote:
> diff --git a/Documentation/devicetree/bindings/clock/tango4-clock.txt b/Documentation/devicetree/bindings/clock/tango4-clock.txt
> new file mode 100644
> index 000000000000..19c580a7bda2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/tango4-clock.txt
> @@ -0,0 +1,23 @@
> +* Sigma Designs Tango4 Clock Generator
> +
> +The Tango4 clock generator outputs cpu_clk and sys_clk (the latter is used
> +for RAM and various peripheral devices). The clock binding described here
> +is applicable to all Tango4 SoCs.
> +
> +Required Properties:
> +
> +- compatible: should be "sigma,tango4-clkgen".

You should add sigma to the vendor prefix file
Documentation/devicetree/bindings/vendor-prefixes.txt

Send that patch to the DT maintainers (Rob has been applying most
vendor prefix patches).

Otherwise this patch looks fine and I'll apply it to clk-next
after v4.4-rc1 comes out (in about 2 weeks). We'll target v4.5.

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

Re: [PATCH v4] clk: tango4: clkgen driver for Tango4 platforms

[Mason]

On 30/10/2015 17:29, Stephen Boyd wrote:

> You should add sigma to the vendor prefix file
> Documentation/devicetree/bindings/vendor-prefixes.txt
> 
> Send that patch to the DT maintainers (Rob has been applying
> most vendor prefix patches).

Hmmm, Mans took care of that a month ago... Did the patch fall
through the cracks perhaps?

http://thread.gmane.org/gmane.linux.kernel/2052776

Should I be looking in Rob's tree?
https://git.kernel.org/cgit/linux/kernel/git/robh/linux.git/

Regards.

Re: [PATCH v4] clk: tango4: clkgen driver for Tango4 platforms

[Stephen Boyd]

On 10/30, Mason wrote:
> On 30/10/2015 17:29, Stephen Boyd wrote:
> 
> > You should add sigma to the vendor prefix file
> > Documentation/devicetree/bindings/vendor-prefixes.txt
> > 
> > Send that patch to the DT maintainers (Rob has been applying
> > most vendor prefix patches).
> 
> Hmmm, Mans took care of that a month ago... Did the patch fall
> through the cracks perhaps?
> 
> http://thread.gmane.org/gmane.linux.kernel/2052776
> 
> Should I be looking in Rob's tree?
> https://git.kernel.org/cgit/linux/kernel/git/robh/linux.git/
> 

Probably to both questions.

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