Skip to contentSkip to conversation

clk: tango4: clkgen driver for Tango4 ARM platforms

PATCH Superseded Marc Gonzalez 3 participants 7 messages Oct 23, 2015 – Oct 30, 2015

Subsystems (2)
Device tree bindings Common clk
lore
Share & tools
lore

Marc Gonzalez

author
#0 lore raw

PATCH v3 1/1 clk: tango4: clkgen driver for Tango4 ARM platforms

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

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

---
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
Documentation/devicetree/bindings/clock/tango4-clock.txt new file +27 -0
new file mode 100644
index 000000000000..b60dba5f1107
@@ -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>;
+	};
drivers/clk/Makefile +1 -0
index c4cf075a2320..60f42251d32a 100644
@@ -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
drivers/clk/clk-tango4.c new file +57 -0

57 lines not on this page · raw message

Signature · 1 line
2.4.5
author
#1 lore raw

Hello Michael & Stephen,

Quoted 15 lines · 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 <[email protected]>

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

Quoted 7 lines
---
 .../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.

#2 lore raw
In reply to Marc Gonzalez

drivers/clk/clk-tango4.c line 2

Quoted 8 lines · Marc Gonzalez
drivers/clk/clk-tango4.c new file +2 -0
new file mode 100644
index 000000000000..9fbee8ce4a95
@@ -0,0 +1,57 @@
+#include <linux/clk-provider.h>
+#include <linux/of_address.h>

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

drivers/clk/clk-tango4.c line 8

Quoted 6 lines
drivers/clk/clk-tango4.c new file +6 -0
+#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.

drivers/clk/clk-tango4.c line 11

Quoted 3 lines
drivers/clk/clk-tango4.c new file +3 -0
+
+#define sysclk_div (clkgen_base + 0x20)
+#define cpuclk_div (clkgen_base + 0x24)

And these macros can be numbers only.

drivers/clk/clk-tango4.c line 17

Quoted 6 lines
drivers/clk/clk-tango4.c new file +6 -0
+
+#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)

drivers/clk/clk-tango4.c line 21

Quoted 4 lines
drivers/clk/clk-tango4.c new file +4 -0
+
+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.

Quoted 7 lines
drivers/clk/clk-tango4.c new file +7 -0
+
+	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);
+}
+

drivers/clk/clk-tango4.c line 38

Quoted 10 lines
drivers/clk/clk-tango4.c new file +10 -0
+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.

drivers/clk/clk-tango4.c line 44

Quoted 6 lines
drivers/clk/clk-tango4.c new file +6 -0
+		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.

drivers/clk/clk-tango4.c line 46

Quoted 2 lines
drivers/clk/clk-tango4.c new file +2 -0
+	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.

Quoted 11 lines
drivers/clk/clk-tango4.c new file +11 -0
+
+	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);
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#3 lore raw

PATCH v4 1/1 clk: tango4: clkgen driver for Tango4 platforms

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

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

---
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
Documentation/devicetree/bindings/clock/tango4-clock.txt new file +23 -0
new file mode 100644
index 000000000000..19c580a7bda2
@@ -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>;
+	};
drivers/clk/Makefile +1 -0
index c4cf075a2320..60f42251d32a 100644
@@ -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
drivers/clk/clk-tango4.c new file +61 -0

61 lines not on this page · raw message

Signature · 1 line
2.4.5
#4 lore raw

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

Quoted 6 lines · Marc Gonzalez
Documentation/devicetree/bindings/clock/tango4-clock.txt new file +0 -0
new file mode 100644
index 000000000000..19c580a7bda2

Documentation/devicetree/bindings/clock/tango4-clock.txt line 9

Quoted 9 lines
Documentation/devicetree/bindings/clock/tango4-clock.txt new file +9 -0
@@ -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.

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
#5 lore raw

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

Quoted 5 lines · Stephen Boyd

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.

#6 lore raw

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

Quoted 8 lines · Mason
Quoted 5 lines · Stephen Boyd

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

Quoted 8 lines

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.

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