zynq COMMON_CLK support

Move the sys_timer definition out of ttc driver and make it part of the common zynq code. This is preparation for renaming and COMMON_CLK support.

Signed-off-by Josh Cartwright <[email protected]>

---
 arch/arm/mach-zynq/common.c | 13 +++++++++++++
 arch/arm/mach-zynq/common.h |  4 +---
 arch/arm/mach-zynq/timer.c  | 10 +---------
 3 files changed, 15 insertions(+), 12 deletions(-)

Suggested cleanup by Arnd Bergmann. Move the ttc timer.c code to drivers/clocksource, and out of the mach-zynq directory.

The common.h (which only held the timer declaration) was renamed to xilinx_ttc.h and moved into include/linux.

Signed-off-by Josh Cartwright <[email protected]>

Cc Arnd Bergmann <[email protected]>

---
 arch/arm/mach-zynq/Makefile                                    | 2 +-
 arch/arm/mach-zynq/common.c                                    | 2 +-
 drivers/clocksource/Makefile                                   | 1 +
 arch/arm/mach-zynq/timer.c => drivers/clocksource/xilinx_ttc.c | 1 -
 arch/arm/mach-zynq/common.h => include/linux/xilinx_ttc.h      | 4 ++--
 5 files changed, 5 insertions(+), 5 deletions(-)
 rename arch/arm/mach-zynq/timer.c => drivers/clocksource/xilinx_ttc.c (99%)
 rename arch/arm/mach-zynq/common.h => include/linux/xilinx_ttc.h (91%)

2012/10/29 Josh Cartwright <[email protected]>:

Quoted 8 lines

Suggested cleanup by Arnd Bergmann. Move the ttc timer.c code to drivers/clocksource, and out of the mach-zynq directory.

The common.h (which only held the timer declaration) was renamed to xilinx_ttc.h and moved into include/linux.

Signed-off-by Josh Cartwright <[email protected]>

Cc Arnd Bergmann <[email protected]>

Quoted 9 lines

---
 arch/arm/mach-zynq/Makefile                                    | 2 +-
 arch/arm/mach-zynq/common.c                                    | 2 +-
 drivers/clocksource/Makefile                                   | 1 +
 arch/arm/mach-zynq/timer.c => drivers/clocksource/xilinx_ttc.c | 1 -
 arch/arm/mach-zynq/common.h => include/linux/xilinx_ttc.h      | 4 ++--
 5 files changed, 5 insertions(+), 5 deletions(-)
 rename arch/arm/mach-zynq/timer.c => drivers/clocksource/xilinx_ttc.c (99%)
 rename arch/arm/mach-zynq/common.h => include/linux/xilinx_ttc.h (91%)

Not going to apply this patch till there is clean way how to move all drivers there.
Especially I don't like to add xilinx_ttc.h to include/linux folder.

2012/10/29 Josh Cartwright <[email protected]>:

Quoted 8 lines

Suggested cleanup by Arnd Bergmann. Move the ttc timer.c code to drivers/clocksource, and out of the mach-zynq directory.

The common.h (which only held the timer declaration) was renamed to xilinx_ttc.h and moved into include/linux.

Signed-off-by Josh Cartwright <[email protected]>

Cc Arnd Bergmann <[email protected]>

Quoted 9 lines

---
 arch/arm/mach-zynq/Makefile                                    | 2 +-
 arch/arm/mach-zynq/common.c                                    | 2 +-
 drivers/clocksource/Makefile                                   | 1 +
 arch/arm/mach-zynq/timer.c => drivers/clocksource/xilinx_ttc.c | 1 -
 arch/arm/mach-zynq/common.h => include/linux/xilinx_ttc.h      | 4 ++--
 5 files changed, 5 insertions(+), 5 deletions(-)
 rename arch/arm/mach-zynq/timer.c => drivers/clocksource/xilinx_ttc.c (99%)
 rename arch/arm/mach-zynq/common.h => include/linux/xilinx_ttc.h (91%)

Not going to apply this patch till there is clean way how to move all drivers there. Especially I don't like to add xilinx_ttc.h to include/linux folder.

Quoted 19 lines

2012/10/29 Josh Cartwright <[email protected]>:

Quoted 8 lines

Suggested cleanup by Arnd Bergmann. Move the ttc timer.c code to drivers/clocksource, and out of the mach-zynq directory.

The common.h (which only held the timer declaration) was renamed to xilinx_ttc.h and moved into include/linux.

Signed-off-by Josh Cartwright <[email protected]>

Cc Arnd Bergmann <[email protected]>

Quoted 9 lines

---
 arch/arm/mach-zynq/Makefile                                    | 2 +-
 arch/arm/mach-zynq/common.c                                    | 2 +-
 drivers/clocksource/Makefile                                   | 1 +
 arch/arm/mach-zynq/timer.c => drivers/clocksource/xilinx_ttc.c | 1 -
 arch/arm/mach-zynq/common.h => include/linux/xilinx_ttc.h      | 4 ++--
 5 files changed, 5 insertions(+), 5 deletions(-)
 rename arch/arm/mach-zynq/timer.c => drivers/clocksource/xilinx_ttc.c (99%)
 rename arch/arm/mach-zynq/common.h => include/linux/xilinx_ttc.h (91%)

Quoted 3 lines

Not going to apply this patch till there is clean way how to move all drivers there. Especially I don't like to add xilinx_ttc.h to include/linux folder.

Okay; I think it's best to defer the moving of the ttc driver from this patchset. It is not a dependency of the clk driver support stuff. If you agree, I can spin up a v2 of the patchset w/o this change, and without the serial CONFIG_OF stuff. Should v2 contain the patches you've already pulled into testing?

The zynq-7000 has an additional UART at 0xE0001000. Describe it in the device tree.

Signed-off-by Josh Cartwright <[email protected]>

---
 arch/arm/boot/dts/zynq-ep107.dts | 7 +++++++
 1 file changed, 7 insertions(+)

The purpose of the created zynq-7000.dtsi file is to describe the hardware common to all Zynq 7000-based boards. Also, get rid of the zynq-ep107 device tree, since it is not hardware anyone can purchase.

Add a zc702 dts file based on the zynq-7000.dtsi. Add it to the dts/Makefile so it is built with the 'dtbs' target.

Signed-off-by Josh Cartwright <[email protected]>

---
 arch/arm/boot/dts/Makefile                         |  1 +
 .../boot/dts/{zynq-ep107.dts => zynq-7000.dtsi}    | 19 +++-----------
 arch/arm/boot/dts/zynq-zc702.dts                   | 30 ++++++++++++++++++++++
 arch/arm/mach-zynq/common.c                        |  3 ++-
 4 files changed, 36 insertions(+), 17 deletions(-)
 rename arch/arm/boot/dts/{zynq-ep107.dts => zynq-7000.dtsi} (79%)
 create mode 100644 arch/arm/boot/dts/zynq-zc702.dts

drivers/clk/clk-zynq.c new file +13 -0

+
+	periph = kzalloc(sizeof(*periph), GFP_KERNEL);
+	WARN_ON(!periph);
+
+	periph->clk_ctrl = slcr_base + reg;
+	spin_lock_init(&periph->clkact_lock);
+
+	init.name = np->name;
+	init.ops = &zynq_periph_clk_ops;
+	for (i = 0; i < ARRAY_SIZE(parent_names); i++)
+		parent_names[i] = of_clk_get_parent_name(np, i);
+	init.parent_names = parent_names;
+	init.num_parents = ARRAY_SIZE(parent_names);

drivers/clk/clk-zynq.c new file +19 -0

+		const char *name;
+
+		err = of_property_read_string_index(np, "clock-output-names", i,
+						    &name);
+		WARN_ON(err);
+
+		periph->gates[i] = clk_register_gate(NULL, name, np->name, 0,
+						     periph->clk_ctrl, i, 0,
+						     &periph->clkact_lock);
+		WARN_ON(IS_ERR(periph->gates[i]));
+	}
+
+	periph->onecell_data.clks = periph->gates;
+	periph->onecell_data.clk_num = i;
+
+	err = of_clk_add_provider(np, of_clk_src_onecell_get,
+				  &periph->onecell_data);
+	WARN_ON(err);
+}
[...]

drivers/clk/clk-zynq.c line 95

Quoted 3 lines · Josh Cartwright

[...]

drivers/clk/clk-zynq.c new file +2 -0

+#define PERIPH_CLK_CTRL_SRC(x)	(periph_clk_parent_map[((x)&3)>>4])
+#define PERIPH_CLK_CTRL_DIV(x)	(((x)&0x3F00)>>8)

A few more spaces wouldn't hurt ;)

Quoted 8 lines

[...]

drivers/clk/clk-zynq.c new file +7 -0

+static void __init zynq_periph_clk_setup(struct device_node *np)
+{
+	struct zynq_periph_clk *periph;
+	const char *parent_names[3];
+	struct clk_init_data init;
+	struct clk *clk;
+	int err;

drivers/clk/clk-zynq.c line 126

Quoted 5 lines

drivers/clk/clk-zynq.c new file +5 -0

+	u32 reg;
+	int i;
+
+	err = of_property_read_u32(np, "reg", &reg);
+	WARN_ON(err);

Shouldn't the function abort if a error happens somewhere? Continuing here will lead to undefined behavior. Same is probably true for the other WARN_ONs.

Quoted 18 lines

drivers/clk/clk-zynq.c new file +18 -0

+
+	periph = kzalloc(sizeof(*periph), GFP_KERNEL);
+	WARN_ON(!periph);
+
+	periph->clk_ctrl = slcr_base + reg;
+	spin_lock_init(&periph->clkact_lock);
+
+	init.name = np->name;
+	init.ops = &zynq_periph_clk_ops;
+	for (i = 0; i < ARRAY_SIZE(parent_names); i++)
+		parent_names[i] = of_clk_get_parent_name(np, i);
+	init.parent_names = parent_names;
+	init.num_parents = ARRAY_SIZE(parent_names);
+
+	periph->hw.init = &init;
+
+	clk = clk_register(NULL, &periph->hw);
+	WARN_ON(IS_ERR(clk));

drivers/clk/clk-zynq.c line 149

Quoted 5 lines

drivers/clk/clk-zynq.c new file +5 -0

+
+	err = of_clk_add_provider(np, of_clk_src_simple_get, clk);
+	WARN_ON(err);
+
+	for (i = 0; i < 2; i++) {

Not all of the peripheral clock generators have two output clocks. I think it makes sense to use the number entries in clock-output-names here.

Thanks for the review.

Quoted 6 lines · Lars-Peter Clausen

drivers/clk/clk-zynq.c line 95

Quoted 3 lines · Josh Cartwright

[...]

drivers/clk/clk-zynq.c new file +2 -0

+#define PERIPH_CLK_CTRL_SRC(x)	(periph_clk_parent_map[((x)&3)>>4])
+#define PERIPH_CLK_CTRL_DIV(x)	(((x)&0x3F00)>>8)

A few more spaces wouldn't hurt ;)

Okay, sure.

Quoted 8 lines

Quoted 8 lines

[...]

drivers/clk/clk-zynq.c new file +7 -0

+static void __init zynq_periph_clk_setup(struct device_node *np)
+{
+	struct zynq_periph_clk *periph;
+	const char *parent_names[3];
+	struct clk_init_data init;
+	struct clk *clk;
+	int err;

Quoted 8 lines

drivers/clk/clk-zynq.c line 126

Quoted 5 lines

drivers/clk/clk-zynq.c new file +5 -0

+	u32 reg;
+	int i;
+
+	err = of_property_read_u32(np, "reg", &reg);
+	WARN_ON(err);

Shouldn't the function abort if a error happens somewhere? Continuing here will lead to undefined behavior. Same is probably true for the other WARN_ONs.

The way I see it is: the kernel is will be left in a bad state in the case of any failure, regardless of if we bail out or continue. AFAICT, there is no clean way to recover from a failure this early.

Given that, it seems simpler (albeit marginally so) just to continue; so that's what I chose to do. I'm not opposed to bailing out, just not convinced it does anything for us.

Quoted 18 lines

Quoted 18 lines

drivers/clk/clk-zynq.c new file +18 -0

+
+	periph = kzalloc(sizeof(*periph), GFP_KERNEL);
+	WARN_ON(!periph);
+
+	periph->clk_ctrl = slcr_base + reg;
+	spin_lock_init(&periph->clkact_lock);
+
+	init.name = np->name;
+	init.ops = &zynq_periph_clk_ops;
+	for (i = 0; i < ARRAY_SIZE(parent_names); i++)
+		parent_names[i] = of_clk_get_parent_name(np, i);
+	init.parent_names = parent_names;
+	init.num_parents = ARRAY_SIZE(parent_names);
+
+	periph->hw.init = &init;
+
+	clk = clk_register(NULL, &periph->hw);
+	WARN_ON(IS_ERR(clk));

Quoted 8 lines

drivers/clk/clk-zynq.c line 149

Quoted 5 lines

drivers/clk/clk-zynq.c new file +5 -0

+
+	err = of_clk_add_provider(np, of_clk_src_simple_get, clk);
+	WARN_ON(err);
+
+	for (i = 0; i < 2; i++) {

Not all of the peripheral clock generators have two output clocks. I think it makes sense to use the number entries in clock-output-names here.

Yes, I agree. I'll also update the bindings documentation.

Thanks again,
Josh

Quoted 2 lines · Josh Cartwright

Quoted 1 line · Lars-Peter Clausen

On 10/31/2012 07:58 PM, Josh Cartwright wrote:

Quoted 16 lines

Quoted 7 lines

Quoted 7 lines

drivers/clk/clk-zynq.c new file +7 -0

+static void __init zynq_periph_clk_setup(struct device_node *np)
+{
+	struct zynq_periph_clk *periph;
+	const char *parent_names[3];
+	struct clk_init_data init;
+	struct clk *clk;
+	int err;

Quoted 8 lines

drivers/clk/clk-zynq.c line 126

Quoted 5 lines

drivers/clk/clk-zynq.c new file +5 -0

+	u32 reg;
+	int i;
+
+	err = of_property_read_u32(np, "reg", &reg);
+	WARN_ON(err);

Shouldn't the function abort if a error happens somewhere? Continuing here will lead to undefined behavior. Same is probably true for the other WARN_ONs.

Quoted 8 lines

The way I see it is: the kernel is will be left in a bad state in the case of any failure, regardless of if we bail out or continue. AFAICT, there is no clean way to recover from a failure this early.

Given that, it seems simpler (albeit marginally so) just to continue; so that's what I chose to do. I'm not opposed to bailing out, just not convinced it does anything for us.

The issue with this approach is that, while you get a warning, unexpected seemingly unrelated side-effects may happen later on. E.g. if no reg property for the clock is specified the reg variable will be uninitialized and contain whatever was on the stack before. The clock will be registered nonetheless and the boot process continues. Now if the clock is enabled a bit in a random register will be modified, which could result in strange and abnormal behavior, which can be very hard to track down.

Quoted 3 lines · Josh Cartwright

Quoted 2 lines · Lars-Peter Clausen

Quoted 1 line · Josh Cartwright

[...]

Quoted 24 lines

Quoted 16 lines

Quoted 7 lines

Quoted 7 lines

drivers/clk/clk-zynq.c new file +7 -0

+static void __init zynq_periph_clk_setup(struct device_node *np)
+{
+ struct zynq_periph_clk *periph;
+ const char *parent_names[3];
+ struct clk_init_data init;
+ struct clk *clk;
+ int err;

Quoted 8 lines

Quoted 5 lines

+ u32 reg;
+ int i;
+
+ err = of_property_read_u32(np, "reg", &reg);
+ WARN_ON(err);

Shouldn't the function abort if a error happens somewhere? Continuing here will lead to undefined behavior. Same is probably true for the other WARN_ONs.

Quoted 8 lines

The way I see it is: the kernel is will be left in a bad state in the case of any failure, regardless of if we bail out or continue. AFAICT, there is no clean way to recover from a failure this early.

Given that, it seems simpler (albeit marginally so) just to continue; so that's what I chose to do. I'm not opposed to bailing out, just not convinced it does anything for us.

Quoted 7 lines

The issue with this approach is that, while you get a warning, unexpected seemingly unrelated side-effects may happen later on. E.g. if no reg property for the clock is specified the reg variable will be uninitialized and contain whatever was on the stack before. The clock will be registered nonetheless and the boot process continues. Now if the clock is enabled a bit in a random register will be modified, which could result in strange and abnormal behavior, which can be very hard to track down.

Okay.....but any reasonable person would start their debugging quest at the source of the WARN_ON. If someone sees the WARN_ON message but stupidly chooses to ignore it, they deserves to spend the time trying to track down abnormal behavior, so I'm still not convinced.

The Zynq platform requires the use of CONFIG_OF. Remove the #ifdef conditionals in the uartps driver.

Signed-off-by Josh Cartwright <[email protected]>

---
 drivers/tty/serial/xilinx_uartps.c | 9 ---------
 1 file changed, 9 deletions(-)

Add support for specifying clock information for the uart clk via the device tree. This eliminates the need to hardcode rates in the device tree.

Signed-off-by Josh Cartwright <[email protected]>

---
 arch/arm/boot/dts/zynq-7000.dtsi   |  4 ++--
 drivers/tty/serial/xilinx_uartps.c | 30 +++++++++++++++++-------------
 2 files changed, 19 insertions(+), 15 deletions(-)

arch/arm/boot/dts/zynq-7000.dtsi +0 -0

index bb3085c..5fb763f 100644
@@ -44,14 +44,14 @@
 			compatible = "xlnx,xuartps";
 			reg = <0xE0000000 0x1000>;
 			interrupts = <0 27 4>;

arch/arm/boot/dts/zynq-7000.dtsi +0 -0

 		};
 
 		slcr: slcr at f8000000 {

Quoted 22 lines · Josh Cartwright

Add support for specifying clock information for the uart clk via the device tree. This eliminates the need to hardcode rates in the device tree.

Signed-off-by Josh Cartwright <[email protected]>

---
 arch/arm/boot/dts/zynq-7000.dtsi   |  4 ++--
 drivers/tty/serial/xilinx_uartps.c | 30 +++++++++++++++++-------------
 2 files changed, 19 insertions(+), 15 deletions(-)

arch/arm/boot/dts/zynq-7000.dtsi +1 -1

index bb3085c..5fb763f 100644
@@ -44,14 +44,14 @@
 			compatible = "xlnx,xuartps";
 			reg = <0xE0000000 0x1000>;
 			interrupts = <0 27 4>;
-			clock = <50000000>;
+			clocks = <&uart_clk 0>;
 		};
 

arch/arm/boot/dts/zynq-7000.dtsi line 54

Quoted 6 lines

arch/arm/boot/dts/zynq-7000.dtsi +1 -1

 		uart1: uart at e0001000 {
 			compatible = "xlnx,xuartps";
 			reg = <0xE0001000 0x1000>;
 			interrupts = <0 50 4>;
-			clock = <50000000>;
+			clocks = <&uart_clk 0>;

Shouldn't this be <&uart_clk 1>?

Add support for retrieving TTC configuration from device tree. This includes the ability to pull information about the driving clocks from the of_clk bindings.

Signed-off-by Josh Cartwright <[email protected]>

---

drivers/clocksource/xilinx_ttc.c +2 -1

index ff38b3e..a4718f7 100644
@@ -209,7 +153,8 @@ static struct clocksource clocksource_xttcpss = {
 static int xttcpss_set_next_event(unsigned long cycles,
 					struct clock_event_device *evt)
 {
-	struct xttcpss_timer *timer = &timers[XTTCPSS_CLOCKEVENT];
+	struct xttcpss_timer_clockevent *xttce = to_xttcpss_timer_clkevent(evt);
+	struct xttcpss_timer *timer = &xttce->xttc;
 
 	xttcpss_set_interval(timer, cycles);
 	return 0;

@@ -224,12 +169,14 @@ static int xttcpss_set_next_event(unsigned long cycles,
 static void xttcpss_set_mode(enum clock_event_mode mode,
 					struct clock_event_device *evt)
 {

Add support for retrieving TTC configuration from device tree. This includes the ability to pull information about the driving clocks from the of_clk bindings.

Signed-off-by Josh Cartwright <[email protected]>

---
 arch/arm/boot/dts/zynq-7000.dtsi |  53 ++++++++
 arch/arm/boot/dts/zynq-zc702.dts |  10 ++
 drivers/clocksource/xilinx_ttc.c | 273 ++++++++++++++++++++++-----------------
 3 files changed, 218 insertions(+), 118 deletions(-)

arch/arm/boot/dts/zynq-7000.dtsi +18 -0

index 5fb763f..9a2442c 100644
@@ -109,5 +109,58 @@
                                };
                        };
                };
+
+               ttc0: ttc0 at f8001000 {
+                       #address-cells = <1>;
+                       #size-cells = <0>;
+                       compatible = "xlnx,ttc";
+                       reg = <0xF8001000 0x1000>;
+                       clocks = <&cpu_clk 3>;
+                       clock-names = "cpu_1x";
+                       clock-ranges;
+
+                       ttc0_0: ttc0.0 {
+                               status = "disabled";
+                               reg = <0>;
+                               interrupts = <0 10 4>;
+                       };
+                       ttc0_1: ttc0.1 {
+                               status = "disabled";
+                               reg = <1>;