All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH 0/3] arc: introduce timer driver and update device tree
@ 2017-01-16 13:49 Vlad Zakharov
  2017-01-16 13:49 ` [U-Boot] [PATCH 1/3] drivers: timer: Introduce ARC timer driver Vlad Zakharov
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Vlad Zakharov @ 2017-01-16 13:49 UTC (permalink / raw)
  To: u-boot

ARC cores may have up to 2 built-in timers: timer0 and timer1,
usually at least one of them exists. They both are driven by the 
same core clock. 

They are controlled through auxiliary registers and so we
don't have to remap their control registers as we used to do
with MMIO registers of external peripheral devices.

This patch series replaces legacy approach to access ARC timer
via specific code in "arch/arc/lib/time.c" and uses timer
driver instead.

We want to have common device tree blobs for both Linux and U-Boot.
To achieve this the patch updates arc device trees with the following:
1. Separate axs10x.dts device tree in order to reflect the real
structure of AXS101 and AXS103 development boards.  
2. Add timer device to skeleton ARC device tree.
3. Add core_clk devices to all ARC boards as it is referenced from
timer device.

Vlad Zakharov (3):
  drivers: timer: Introduce ARC timer driver
  arc: dts: separate single axs10x.dts file
  arc: use timer driver for ARC boards

 arch/Kconfig                                 |   3 +
 arch/arc/Kconfig                             |   9 ++-
 arch/arc/dts/Makefile                        |   3 +-
 arch/arc/dts/abilis_tb100.dts                |  12 +++-
 arch/arc/dts/axc001.dtsi                     |  19 +++++
 arch/arc/dts/axc003.dtsi                     |  19 +++++
 arch/arc/dts/axs101.dts                      |  19 +++++
 arch/arc/dts/axs103.dts                      |  19 +++++
 arch/arc/dts/axs10x.dts                      |  57 ---------------
 arch/arc/dts/axs10x_mb.dtsi                  |  66 +++++++++++++++++
 arch/arc/dts/nsim.dts                        |  12 +++-
 arch/arc/dts/skeleton.dtsi                   |  19 ++++-
 arch/arc/include/asm/arcregs.h               |   4 ++
 board/synopsys/axs10x/Kconfig                |   2 +-
 configs/axs101_defconfig                     |   4 +-
 configs/axs103_defconfig                     |   3 +-
 doc/device-tree-bindings/timer/arc_timer.txt |  24 +++++++
 drivers/timer/Kconfig                        |   9 +++
 drivers/timer/Makefile                       |   1 +
 drivers/timer/arc_timer.c                    | 103 +++++++++++++++++++++++++++
 include/configs/axs10x.h                     |   2 -
 include/configs/nsim.h                       |   5 --
 include/configs/tb100.h                      |   5 --
 23 files changed, 334 insertions(+), 85 deletions(-)
 create mode 100644 arch/arc/dts/axc001.dtsi
 create mode 100644 arch/arc/dts/axc003.dtsi
 create mode 100644 arch/arc/dts/axs101.dts
 create mode 100644 arch/arc/dts/axs103.dts
 delete mode 100644 arch/arc/dts/axs10x.dts
 create mode 100644 arch/arc/dts/axs10x_mb.dtsi
 create mode 100644 doc/device-tree-bindings/timer/arc_timer.txt
 create mode 100644 drivers/timer/arc_timer.c

-- 
2.7.4

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH 1/3] drivers: timer: Introduce ARC timer driver
  2017-01-16 13:49 [U-Boot] [PATCH 0/3] arc: introduce timer driver and update device tree Vlad Zakharov
@ 2017-01-16 13:49 ` Vlad Zakharov
  2017-01-21  3:51   ` Simon Glass
  2017-01-16 13:49 ` [U-Boot] [PATCH 2/3] arc: dts: separate single axs10x.dts file Vlad Zakharov
  2017-01-16 13:49 ` [U-Boot] [PATCH 3/3] arc: use timer driver for ARC boards Vlad Zakharov
  2 siblings, 1 reply; 11+ messages in thread
From: Vlad Zakharov @ 2017-01-16 13:49 UTC (permalink / raw)
  To: u-boot

This commit introduces timer driver for ARC.

ARC timers are configured via ARC AUX registers so we use special
functions to access timer control registers.

This driver allows utilization of either timer0 or timer1
depending on which one is available in real hardware. Essentially
only existing timers should be mentioned in board's Device Tree
description.

Signed-off-by: Vlad Zakharov <vzakhar@synopsys.com>
---
 arch/arc/include/asm/arcregs.h               |   4 ++
 doc/device-tree-bindings/timer/arc_timer.txt |  24 +++++++
 drivers/timer/Kconfig                        |   9 +++
 drivers/timer/Makefile                       |   1 +
 drivers/timer/arc_timer.c                    | 103 +++++++++++++++++++++++++++
 5 files changed, 141 insertions(+)
 create mode 100644 doc/device-tree-bindings/timer/arc_timer.txt
 create mode 100644 drivers/timer/arc_timer.c

diff --git a/arch/arc/include/asm/arcregs.h b/arch/arc/include/asm/arcregs.h
index cf999b0..54a9b00 100644
--- a/arch/arc/include/asm/arcregs.h
+++ b/arch/arc/include/asm/arcregs.h
@@ -33,6 +33,10 @@
 #define ARC_AUX_TIMER0_CTRL	0x22	/* Timer 0 control */
 #define ARC_AUX_TIMER0_LIMIT	0x23	/* Timer 0 limit */
 
+#define ARC_AUX_TIMER1_CNT	0x100	/* Timer 1 count */
+#define ARC_AUX_TIMER1_CTRL	0x101	/* Timer 1 control */
+#define ARC_AUX_TIMER1_LIMIT	0x102	/* Timer 1 limit */
+
 #define ARC_AUX_INTR_VEC_BASE	0x25
 
 /* Data cache related auxiliary registers */
diff --git a/doc/device-tree-bindings/timer/arc_timer.txt b/doc/device-tree-bindings/timer/arc_timer.txt
new file mode 100644
index 0000000..441f2f3
--- /dev/null
+++ b/doc/device-tree-bindings/timer/arc_timer.txt
@@ -0,0 +1,24 @@
+ARC Timer
+
+Required properties:
+
+- compatible : should be "snps,arc-timer".
+- reg : Specifies timer ID, could be either 0 or 1.
+- clocks : Specifies clocks that drives the counter.
+
+Examples:
+
+timer at 0 {
+	compatible = "snps,arc-timer";
+	clocks = <&core_clk>;
+	reg = <0>;
+};
+
+timer at 1 {
+	compatible = "snps,arc-timer";
+	clocks = <&core_clk>;
+	reg = <1>;
+};
+
+NOTE: if you specify both timers, clocks always should be the same 
+as each timer is driven by the same core clock.
diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig
index cb18f12..a6c77ea 100644
--- a/drivers/timer/Kconfig
+++ b/drivers/timer/Kconfig
@@ -46,4 +46,13 @@ config OMAP_TIMER
 	help
 	  Select this to enable an timer for Omap devices.
 
+config ARC_TIMER
+	bool "ARC timer support"
+	depends on TIMER && ARC && CLK
+	help
+	  Select this to enable built-in ARC timers.
+	  ARC cores may have up to 2 built-in timers: timer0 and timer1,
+	  usually at least one of them exists. Either of them is supported
+	  in U-Boot.
+
 endmenu
diff --git a/drivers/timer/Makefile b/drivers/timer/Makefile
index f351fbb..e9624dd 100644
--- a/drivers/timer/Makefile
+++ b/drivers/timer/Makefile
@@ -9,3 +9,4 @@ obj-$(CONFIG_ALTERA_TIMER)	+= altera_timer.o
 obj-$(CONFIG_SANDBOX_TIMER)	+= sandbox_timer.o
 obj-$(CONFIG_X86_TSC_TIMER)	+= tsc_timer.o
 obj-$(CONFIG_OMAP_TIMER)	+= omap-timer.o
+obj-$(CONFIG_ARC_TIMER)	+= arc_timer.o
diff --git a/drivers/timer/arc_timer.c b/drivers/timer/arc_timer.c
new file mode 100644
index 0000000..07274f2
--- /dev/null
+++ b/drivers/timer/arc_timer.c
@@ -0,0 +1,103 @@
+/*
+ * Copyright (C) 2016 Synopsys, Inc. All rights reserved.
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <errno.h>
+#include <timer.h>
+#include <asm/io.h>
+#include <asm/arcregs.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+#define NH_MODE (1 << 1)
+
+/*
+ * ARC timer control registers are mapped to auxiliary address space.
+ * There are special ARC asm command to access that addresses.
+ * Therefore we use built-in functions to read from and write to timer
+ * control register.
+ */
+
+/* Driver private data. Contains timer id. Could be either 0 or 1. */
+struct arc_timer_priv {
+		uint timer_id;
+};
+
+static int arc_timer_get_count(struct udevice *dev, u64 *count)
+{
+	u32 val = 0;
+	struct arc_timer_priv *priv = dev_get_priv(dev);
+	switch (priv->timer_id) {
+	case 0:
+		val = read_aux_reg(ARC_AUX_TIMER0_CNT);
+		break;
+	case 1:
+		val = read_aux_reg(ARC_AUX_TIMER1_CNT);
+		break;
+	}
+	*count = timer_conv_64(val);
+
+	return 0;
+}
+
+static int arc_timer_probe(struct udevice *dev)
+{
+	int id;
+
+	struct arc_timer_priv *priv = dev_get_priv(dev);
+
+	/* Get registers offset and size */
+	id = fdtdec_get_int(gd->fdt_blob, dev->of_offset, "reg", -1);
+	if (id < 0)
+		return -EINVAL;
+
+	if (id > 1)
+		return -ENXIO;
+
+	priv->timer_id = (uint)id;
+
+	switch (priv->timer_id) {
+	case 0:
+		/* Disable timer if CPU is halted */
+		write_aux_reg(ARC_AUX_TIMER0_CTRL, NH_MODE);
+		/* Set max value for counter/timer */
+		write_aux_reg(ARC_AUX_TIMER0_LIMIT, 0xffffffff);
+		/* Set initial count value and restart counter/timer */
+		write_aux_reg(ARC_AUX_TIMER0_CNT, 0);
+		break;
+	case 1:
+		/* Disable timer if CPU is halted */
+		write_aux_reg(ARC_AUX_TIMER1_CTRL, NH_MODE);
+		/* Set max value for counter/timer */
+		write_aux_reg(ARC_AUX_TIMER1_LIMIT, 0xffffffff);
+		/* Set initial count value and restart counter/timer */
+		write_aux_reg(ARC_AUX_TIMER1_CNT, 0);
+		break;
+	}
+
+	return 0;
+}
+
+
+static const struct timer_ops arc_timer_ops = {
+	.get_count = arc_timer_get_count,
+};
+
+static const struct udevice_id arc_timer_ids[] = {
+	{ .compatible = "snps,arc-timer" },
+	{}
+};
+
+U_BOOT_DRIVER(arc_timer) = {
+	.name	= "arc_timer",
+	.id	= UCLASS_TIMER,
+	.of_match = arc_timer_ids,
+	.probe = arc_timer_probe,
+	.ops	= &arc_timer_ops,
+	.flags = DM_FLAG_PRE_RELOC,
+	.priv_auto_alloc_size = sizeof(struct arc_timer_priv),
+};
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH 2/3] arc: dts: separate single axs10x.dts file
  2017-01-16 13:49 [U-Boot] [PATCH 0/3] arc: introduce timer driver and update device tree Vlad Zakharov
  2017-01-16 13:49 ` [U-Boot] [PATCH 1/3] drivers: timer: Introduce ARC timer driver Vlad Zakharov
@ 2017-01-16 13:49 ` Vlad Zakharov
  2017-01-21  3:51   ` Simon Glass
  2017-01-16 13:49 ` [U-Boot] [PATCH 3/3] arc: use timer driver for ARC boards Vlad Zakharov
  2 siblings, 1 reply; 11+ messages in thread
From: Vlad Zakharov @ 2017-01-16 13:49 UTC (permalink / raw)
  To: u-boot

We want to use the same device tree blobs in both Linux and U-Boot for
ARC boards.

Earlier device tree sources in U-Boot were very simplified and hadn't been
updated for quite a long period of time.

So this commit is the first step on the road to unified device tree blobs.

First of all we re-organize device tree sources for AXS10X boards.
As AXS101 and AXS103 boards consist of AXS10X motherboard and AXC001 and
AXC003 cpu tiles respectively we add corresponding device tree source
files: axs10x_mb.dtsi for motherboard, axc001.dtsi and axc003.dtsi for
CPU tiles and axs101.dts and axs103.dts to represent actual boards.

Also we delete axs10x.dts as it is no longer used.

One more important change - we add timer device to ARC skeleton device
tree sources as both ARC700 and ARCHS cores contain such timer.
We add core_clk nodes to abilis_tb100, nsim, axc001 and axc003 device tree
sources as it is referenced via phandle from timer node in common
skeleton.dtsi file.

Signed-off-by: Vlad Zakharov <vzakhar@synopsys.com>
---
 arch/arc/Kconfig              |  9 ++++--
 arch/arc/dts/Makefile         |  3 +-
 arch/arc/dts/abilis_tb100.dts | 12 ++++++--
 arch/arc/dts/axc001.dtsi      | 19 +++++++++++++
 arch/arc/dts/axc003.dtsi      | 19 +++++++++++++
 arch/arc/dts/axs101.dts       | 19 +++++++++++++
 arch/arc/dts/axs103.dts       | 19 +++++++++++++
 arch/arc/dts/axs10x.dts       | 57 -------------------------------------
 arch/arc/dts/axs10x_mb.dtsi   | 66 +++++++++++++++++++++++++++++++++++++++++++
 arch/arc/dts/nsim.dts         | 12 ++++++--
 arch/arc/dts/skeleton.dtsi    | 19 ++++++++++++-
 board/synopsys/axs10x/Kconfig |  2 +-
 configs/axs101_defconfig      |  3 +-
 configs/axs103_defconfig      |  2 +-
 14 files changed, 190 insertions(+), 71 deletions(-)
 create mode 100644 arch/arc/dts/axc001.dtsi
 create mode 100644 arch/arc/dts/axc003.dtsi
 create mode 100644 arch/arc/dts/axs101.dts
 create mode 100644 arch/arc/dts/axs103.dts
 delete mode 100644 arch/arc/dts/axs10x.dts
 create mode 100644 arch/arc/dts/axs10x_mb.dtsi

diff --git a/arch/arc/Kconfig b/arch/arc/Kconfig
index 4c5696b..6e4b1d0 100644
--- a/arch/arc/Kconfig
+++ b/arch/arc/Kconfig
@@ -118,7 +118,7 @@ config SYS_DCACHE_OFF
 
 choice
 	prompt "Target select"
-	default TARGET_AXS10X
+	default TARGET_AXS103
 
 config TARGET_TB100
 	bool "Support tb100"
@@ -126,8 +126,11 @@ config TARGET_TB100
 config TARGET_NSIM
 	bool "Support standalone nSIM & Free nSIM"
 
-config TARGET_AXS10X
-	bool "Support Synopsys Designware SDP board (AXS101 & AXS103)"
+config TARGET_AXS101
+	bool "Support Synopsys Designware SDP board AXS101"
+
+config TARGET_AXS103
+	bool "Support Synopsys Designware SDP board AXS103"
 
 endchoice
 
diff --git a/arch/arc/dts/Makefile b/arch/arc/dts/Makefile
index 1d94c08..218a647 100644
--- a/arch/arc/dts/Makefile
+++ b/arch/arc/dts/Makefile
@@ -2,7 +2,8 @@
 # SPDX-License-Identifier:	GPL-2.0+
 #
 
-dtb-$(CONFIG_TARGET_AXS10X) +=  axs10x.dtb
+dtb-$(CONFIG_TARGET_AXS101) +=  axs101.dtb
+dtb-$(CONFIG_TARGET_AXS103) +=  axs103.dtb
 dtb-$(CONFIG_TARGET_NSIM) +=  nsim.dtb
 dtb-$(CONFIG_TARGET_TB100) +=  abilis_tb100.dtb
 
diff --git a/arch/arc/dts/abilis_tb100.dts b/arch/arc/dts/abilis_tb100.dts
index cf395c4..23329ec 100644
--- a/arch/arc/dts/abilis_tb100.dts
+++ b/arch/arc/dts/abilis_tb100.dts
@@ -8,13 +8,19 @@
 #include "skeleton.dtsi"
 
 / {
-	#address-cells = <1>;
-	#size-cells = <1>;
-
 	aliases {
 		console = &uart0;
 	};
 
+	cpu_card {
+		core_clk: core_clk {
+			#clock-cells = <0>;
+			compatible = "fixed-clock";
+			clock-frequency = <500000000>;
+			u-boot,dm-pre-reloc;
+		};
+	};
+
 	uart0: serial at ff100000 {
 			compatible = "snps,dw-apb-uart";
 			reg = <0xff100000 0x1000>;
diff --git a/arch/arc/dts/axc001.dtsi b/arch/arc/dts/axc001.dtsi
new file mode 100644
index 0000000..1cf630d
--- /dev/null
+++ b/arch/arc/dts/axc001.dtsi
@@ -0,0 +1,19 @@
+/*
+ * Copyright (C) 2017 Synopsys, Inc. All rights reserved.
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+/include/ "skeleton.dtsi"
+
+/ {
+	cpu_card {
+		core_clk: core_clk {
+			#clock-cells = <0>;
+			compatible = "fixed-clock";
+			clock-frequency = <750000000>;
+			u-boot,dm-pre-reloc;
+		};
+	};
+};
+
diff --git a/arch/arc/dts/axc003.dtsi b/arch/arc/dts/axc003.dtsi
new file mode 100644
index 0000000..5e9270a
--- /dev/null
+++ b/arch/arc/dts/axc003.dtsi
@@ -0,0 +1,19 @@
+/*
+ * Copyright (C) 2017 Synopsys, Inc. All rights reserved.
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+/include/ "skeleton.dtsi"
+
+/ {
+	cpu_card {
+		core_clk: core_clk {
+			#clock-cells = <0>;
+			compatible = "fixed-clock";
+			clock-frequency = <100000000>;
+			u-boot,dm-pre-reloc;
+		};
+	};
+};
+
diff --git a/arch/arc/dts/axs101.dts b/arch/arc/dts/axs101.dts
new file mode 100644
index 0000000..0a7b982
--- /dev/null
+++ b/arch/arc/dts/axs101.dts
@@ -0,0 +1,19 @@
+/*
+ * Copyright (C) 2017 Synopsys, Inc. All rights reserved.
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+/dts-v1/;
+
+/include/ "axc001.dtsi"
+/include/ "axs10x_mb.dtsi"
+
+
+/ {
+    model = "snps,axs101";
+
+	chosen {
+		stdout-path = &uart0;
+	};
+};
+
diff --git a/arch/arc/dts/axs103.dts b/arch/arc/dts/axs103.dts
new file mode 100644
index 0000000..5ffcef8
--- /dev/null
+++ b/arch/arc/dts/axs103.dts
@@ -0,0 +1,19 @@
+/*
+ * Copyright (C) 2017 Synopsys, Inc. All rights reserved.
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+/dts-v1/;
+
+/include/ "axc003.dtsi"
+/include/ "axs10x_mb.dtsi"
+
+
+/ {
+    model = "snps,axs103";
+
+	chosen {
+		stdout-path = &uart0;
+	};
+};
+
diff --git a/arch/arc/dts/axs10x.dts b/arch/arc/dts/axs10x.dts
deleted file mode 100644
index 391d067..0000000
--- a/arch/arc/dts/axs10x.dts
+++ /dev/null
@@ -1,57 +0,0 @@
-/*
- * Copyright (C) 2015 Synopsys, Inc. All rights reserved.
- *
- * SPDX-License-Identifier:	GPL-2.0+
- */
-/dts-v1/;
-
-#include "skeleton.dtsi"
-
-/ {
-	#address-cells = <1>;
-	#size-cells = <1>;
-
-	aliases {
-		console = &uart0;
-	};
-
-	clocks {
-		apbclk: apbclk {
-			compatible = "fixed-clock";
-			clock-frequency = <50000000>;
-			#clock-cells = <0>;
-		};
-	};
-
-	uart0: serial0 at e0022000 {
-		compatible = "snps,dw-apb-uart";
-		reg = <0xe0022000 0x1000>;
-		reg-shift = <2>;
-		reg-io-width = <4>;
-	};
-
-	ethernet at e0018000 {
-		#interrupt-cells = <1>;
-		compatible = "altr,socfpga-stmmac";
-		reg = < 0xe0018000 0x2000 >;
-		interrupts = < 25 >;
-		interrupt-names = "macirq";
-		phy-mode = "gmii";
-		snps,pbl = < 32 >;
-		clocks = <&apbclk>;
-		clock-names = "stmmaceth";
-		max-speed = <100>;
-	};
-
-	ehci at 0xe0040000 {
-		compatible = "generic-ehci";
-		reg = < 0xe0040000 0x100 >;
-		interrupts = < 8 >;
-	};
-
-	ohci at 0xe0060000 {
-		compatible = "generic-ohci";
-		reg = < 0xe0060000 0x100 >;
-		interrupts = < 8 >;
-	};
-};
diff --git a/arch/arc/dts/axs10x_mb.dtsi b/arch/arc/dts/axs10x_mb.dtsi
new file mode 100644
index 0000000..b74d3c8
--- /dev/null
+++ b/arch/arc/dts/axs10x_mb.dtsi
@@ -0,0 +1,66 @@
+/*
+ * Copyright (C) 2017 Synopsys, Inc. All rights reserved.
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+/ {
+	axs10x_mb at e0000000 {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0x00000000 0xe0000000 0x10000000>;
+		u-boot,dm-pre-reloc;
+
+		clocks {
+			compatible = "simple-bus";
+			u-boot,dm-pre-reloc;
+
+			apbclk: apbclk {
+				compatible = "fixed-clock";
+				clock-frequency = <50000000>;
+				#clock-cells = <0>;
+			};
+
+			uartclk: uartclk {
+				compatible = "fixed-clock";
+				clock-frequency = <33333333>;
+				#clock-cells = <0>;
+				u-boot,dm-pre-reloc;
+			};
+		};
+
+		ethernet at 18000 {
+			#interrupt-cells = <1>;
+			compatible = "altr,socfpga-stmmac";
+			reg = < 0x18000 0x2000 >;
+			interrupts = < 25 >;
+			interrupt-names = "macirq";
+			phy-mode = "gmii";
+			snps,pbl = < 32 >;
+			clocks = <&apbclk>;
+			clock-names = "stmmaceth";
+			max-speed = <100>;
+		};
+
+		ehci at 0x40000 {
+			compatible = "generic-ehci";
+			reg = < 0x40000 0x100 >;
+			interrupts = < 8 >;
+		};
+
+		ohci at 0x60000 {
+			compatible = "generic-ohci";
+			reg = < 0x60000 0x100 >;
+			interrupts = < 8 >;
+		};
+
+		uart0: serial0 at 22000 {
+			compatible = "snps,dw-apb-uart";
+			reg = <0x22000 0x100>;
+			clocks = <&uartclk>;
+			reg-shift = <2>;
+			reg-io-width = <4>;
+		};
+	};
+};
diff --git a/arch/arc/dts/nsim.dts b/arch/arc/dts/nsim.dts
index 69e16c2..af80422 100644
--- a/arch/arc/dts/nsim.dts
+++ b/arch/arc/dts/nsim.dts
@@ -8,13 +8,19 @@
 #include "skeleton.dtsi"
 
 / {
-	#address-cells = <1>;
-	#size-cells = <1>;
-
 	aliases {
 		console = &arcuart0;
 	};
 
+	cpu_card {
+		core_clk: core_clk {
+			#clock-cells = <0>;
+			compatible = "fixed-clock";
+			clock-frequency = <70000000>;
+			u-boot,dm-pre-reloc;
+		};
+	};
+
 	arcuart0: serial at 0xc0fc1000 {
 		compatible = "snps,arc-uart";
 		reg = <0xc0fc1000 0x100>;
diff --git a/arch/arc/dts/skeleton.dtsi b/arch/arc/dts/skeleton.dtsi
index b41d241..279fc6c 100644
--- a/arch/arc/dts/skeleton.dtsi
+++ b/arch/arc/dts/skeleton.dtsi
@@ -9,5 +9,22 @@
 	#size-cells = <1>;
 	chosen { };
 	aliases { };
-	memory { device_type = "memory"; reg = <0 0>; };
+
+	cpu_card {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		u-boot,dm-pre-reloc;
+
+		timer at 0 {
+			compatible = "snps,arc-timer";
+			clocks = <&core_clk>;
+			reg = <0 1>;
+		};
+	};
+
+	memory at 80000000 {
+		device_type = "memory";
+		reg = <0x80000000 0x10000000>; /* 256M */
+	};
 };
diff --git a/board/synopsys/axs10x/Kconfig b/board/synopsys/axs10x/Kconfig
index c60b6a2..dd1305a 100644
--- a/board/synopsys/axs10x/Kconfig
+++ b/board/synopsys/axs10x/Kconfig
@@ -1,4 +1,4 @@
-if TARGET_AXS10X
+if TARGET_AXS101 || TARGET_AXS103
 
 config SYS_BOARD
 	default "axs10x"
diff --git a/configs/axs101_defconfig b/configs/axs101_defconfig
index 3793c42..a8983df 100644
--- a/configs/axs101_defconfig
+++ b/configs/axs101_defconfig
@@ -1,9 +1,10 @@
 CONFIG_ARC=y
 CONFIG_SYS_DCACHE_OFF=y
+CONFIG_TARGET_AXS101=y
 CONFIG_SYS_CLK_FREQ=750000000
 CONFIG_MMC=y
 CONFIG_SYS_TEXT_BASE=0x81000000
-CONFIG_DEFAULT_DEVICE_TREE="axs10x"
+CONFIG_DEFAULT_DEVICE_TREE="axs101"
 CONFIG_BOOTDELAY=3
 CONFIG_SYS_PROMPT="AXS# "
 # CONFIG_CMD_IMLS is not set
diff --git a/configs/axs103_defconfig b/configs/axs103_defconfig
index 30a4021..2a0e3cc 100644
--- a/configs/axs103_defconfig
+++ b/configs/axs103_defconfig
@@ -3,7 +3,7 @@ CONFIG_ISA_ARCV2=y
 CONFIG_SYS_CLK_FREQ=100000000
 CONFIG_MMC=y
 CONFIG_SYS_TEXT_BASE=0x81000000
-CONFIG_DEFAULT_DEVICE_TREE="axs10x"
+CONFIG_DEFAULT_DEVICE_TREE="axs103"
 CONFIG_BOOTDELAY=3
 CONFIG_SYS_PROMPT="AXS# "
 # CONFIG_CMD_IMLS is not set
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH 3/3] arc: use timer driver for ARC boards
  2017-01-16 13:49 [U-Boot] [PATCH 0/3] arc: introduce timer driver and update device tree Vlad Zakharov
  2017-01-16 13:49 ` [U-Boot] [PATCH 1/3] drivers: timer: Introduce ARC timer driver Vlad Zakharov
  2017-01-16 13:49 ` [U-Boot] [PATCH 2/3] arc: dts: separate single axs10x.dts file Vlad Zakharov
@ 2017-01-16 13:49 ` Vlad Zakharov
  2017-01-21  3:51   ` Simon Glass
  2 siblings, 1 reply; 11+ messages in thread
From: Vlad Zakharov @ 2017-01-16 13:49 UTC (permalink / raw)
  To: u-boot

This commit replaces legacy timer code with usage of arc timer
driver.

It removes arch/arc/lib/time.c file and selects CONFIG_CLK,
CONFIG_TIMER and CONFIG_ARC_TIMER options for all ARC boards by default.
Therefore we remove CONFIG_CLK option from less common axs101 and
axs103 defconfigs.

Also it removes legacy CONFIG_SYS_TIMER_RATE config symbol from
axs10x.h, tb100.h and nsim.h configs files as it is no longer required.

Signed-off-by: Vlad Zakharov <vzakhar@synopsys.com>
---
 arch/Kconfig             |  3 +++
 arch/arc/lib/Makefile    |  1 -
 arch/arc/lib/timer.c     | 24 ------------------------
 configs/axs101_defconfig |  1 -
 configs/axs103_defconfig |  1 -
 include/configs/axs10x.h |  2 --
 include/configs/nsim.h   |  5 -----
 include/configs/tb100.h  |  5 -----
 8 files changed, 3 insertions(+), 39 deletions(-)
 delete mode 100644 arch/arc/lib/timer.c

diff --git a/arch/Kconfig b/arch/Kconfig
index ffc7b45..1d2f4ef 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -12,6 +12,9 @@ config ARC
 	bool "ARC architecture"
 	select HAVE_PRIVATE_LIBGCC
 	select SUPPORT_OF_CONTROL
+	select CLK
+	select TIMER
+	select ARC_TIMER
 
 config ARM
 	bool "ARM architecture"
diff --git a/arch/arc/lib/Makefile b/arch/arc/lib/Makefile
index eb62b3c..12097bf 100644
--- a/arch/arc/lib/Makefile
+++ b/arch/arc/lib/Makefile
@@ -18,7 +18,6 @@ obj-y += memcmp.o
 obj-y += memcpy-700.o
 obj-y += memset.o
 obj-y += reset.o
-obj-y += timer.o
 obj-y += ints_low.o
 obj-y += init_helpers.o
 
diff --git a/arch/arc/lib/timer.c b/arch/arc/lib/timer.c
deleted file mode 100644
index a0acbbc..0000000
--- a/arch/arc/lib/timer.c
+++ /dev/null
@@ -1,24 +0,0 @@
-/*
- * Copyright (C) 2013-2014 Synopsys, Inc. All rights reserved.
- *
- * SPDX-License-Identifier:	GPL-2.0+
- */
-
-#include <asm/arcregs.h>
-
-#define NH_MODE	(1 << 1)	/* Disable timer if CPU is halted */
-
-int timer_init(void)
-{
-	write_aux_reg(ARC_AUX_TIMER0_CTRL, NH_MODE);
-	/* Set max value for counter/timer */
-	write_aux_reg(ARC_AUX_TIMER0_LIMIT, 0xffffffff);
-	/* Set initial count value and restart counter/timer */
-	write_aux_reg(ARC_AUX_TIMER0_CNT, 0);
-	return 0;
-}
-
-unsigned long timer_read_counter(void)
-{
-	return read_aux_reg(ARC_AUX_TIMER0_CNT);
-}
diff --git a/configs/axs101_defconfig b/configs/axs101_defconfig
index a8983df..45dd5c4 100644
--- a/configs/axs101_defconfig
+++ b/configs/axs101_defconfig
@@ -19,7 +19,6 @@ CONFIG_OF_CONTROL=y
 CONFIG_OF_EMBED=y
 CONFIG_NET_RANDOM_ETHADDR=y
 CONFIG_DM=y
-CONFIG_CLK=y
 CONFIG_SYS_I2C_DW=y
 CONFIG_MMC_DW=y
 CONFIG_DM_ETH=y
diff --git a/configs/axs103_defconfig b/configs/axs103_defconfig
index 2a0e3cc..e2f6265 100644
--- a/configs/axs103_defconfig
+++ b/configs/axs103_defconfig
@@ -18,7 +18,6 @@ CONFIG_OF_CONTROL=y
 CONFIG_OF_EMBED=y
 CONFIG_NET_RANDOM_ETHADDR=y
 CONFIG_DM=y
-CONFIG_CLK=y
 CONFIG_SYS_I2C_DW=y
 CONFIG_MMC_DW=y
 CONFIG_DM_ETH=y
diff --git a/include/configs/axs10x.h b/include/configs/axs10x.h
index 2dd9d31..9dbc9de 100644
--- a/include/configs/axs10x.h
+++ b/include/configs/axs10x.h
@@ -11,8 +11,6 @@
 /*
  *  CPU configuration
  */
-#define CONFIG_SYS_TIMER_RATE		CONFIG_SYS_CLK_FREQ
-
 #define ARC_FPGA_PERIPHERAL_BASE	0xE0000000
 #define ARC_APB_PERIPHERAL_BASE		0xF0000000
 #define ARC_DWMMC_BASE			(ARC_FPGA_PERIPHERAL_BASE + 0x15000)
diff --git a/include/configs/nsim.h b/include/configs/nsim.h
index 1edc560..630d529 100644
--- a/include/configs/nsim.h
+++ b/include/configs/nsim.h
@@ -10,11 +10,6 @@
 #include <linux/sizes.h>
 
 /*
- *  CPU configuration
- */
-#define CONFIG_SYS_TIMER_RATE		CONFIG_SYS_CLK_FREQ
-
-/*
  * Memory configuration
  */
 #define CONFIG_SYS_MONITOR_BASE		CONFIG_SYS_TEXT_BASE
diff --git a/include/configs/tb100.h b/include/configs/tb100.h
index 39bb5b3..856f8cb 100644
--- a/include/configs/tb100.h
+++ b/include/configs/tb100.h
@@ -10,11 +10,6 @@
 #include <linux/sizes.h>
 
 /*
- *  CPU configuration
- */
-#define CONFIG_SYS_TIMER_RATE		CONFIG_SYS_CLK_FREQ
-
-/*
  * Memory configuration
  */
 #define CONFIG_SYS_MONITOR_BASE		CONFIG_SYS_TEXT_BASE
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH 1/3] drivers: timer: Introduce ARC timer driver
  2017-01-16 13:49 ` [U-Boot] [PATCH 1/3] drivers: timer: Introduce ARC timer driver Vlad Zakharov
@ 2017-01-21  3:51   ` Simon Glass
  2017-01-31 14:44     ` Vlad Zakharov
  0 siblings, 1 reply; 11+ messages in thread
From: Simon Glass @ 2017-01-21  3:51 UTC (permalink / raw)
  To: u-boot

On 16 January 2017 at 06:49, Vlad Zakharov
<Vladislav.Zakharov@synopsys.com> wrote:
> This commit introduces timer driver for ARC.
>
> ARC timers are configured via ARC AUX registers so we use special
> functions to access timer control registers.
>
> This driver allows utilization of either timer0 or timer1
> depending on which one is available in real hardware. Essentially
> only existing timers should be mentioned in board's Device Tree
> description.
>
> Signed-off-by: Vlad Zakharov <vzakhar@synopsys.com>
> ---
>  arch/arc/include/asm/arcregs.h               |   4 ++
>  doc/device-tree-bindings/timer/arc_timer.txt |  24 +++++++
>  drivers/timer/Kconfig                        |   9 +++
>  drivers/timer/Makefile                       |   1 +
>  drivers/timer/arc_timer.c                    | 103 +++++++++++++++++++++++++++
>  5 files changed, 141 insertions(+)
>  create mode 100644 doc/device-tree-bindings/timer/arc_timer.txt
>  create mode 100644 drivers/timer/arc_timer.c

Reviewed-by: Simon Glass <sjg@chromium.org>

>
> diff --git a/arch/arc/include/asm/arcregs.h b/arch/arc/include/asm/arcregs.h
> index cf999b0..54a9b00 100644
> --- a/arch/arc/include/asm/arcregs.h
> +++ b/arch/arc/include/asm/arcregs.h
> @@ -33,6 +33,10 @@
>  #define ARC_AUX_TIMER0_CTRL    0x22    /* Timer 0 control */
>  #define ARC_AUX_TIMER0_LIMIT   0x23    /* Timer 0 limit */
>
> +#define ARC_AUX_TIMER1_CNT     0x100   /* Timer 1 count */
> +#define ARC_AUX_TIMER1_CTRL    0x101   /* Timer 1 control */
> +#define ARC_AUX_TIMER1_LIMIT   0x102   /* Timer 1 limit */
> +
>  #define ARC_AUX_INTR_VEC_BASE  0x25
>
>  /* Data cache related auxiliary registers */
> diff --git a/doc/device-tree-bindings/timer/arc_timer.txt b/doc/device-tree-bindings/timer/arc_timer.txt
> new file mode 100644
> index 0000000..441f2f3
> --- /dev/null
> +++ b/doc/device-tree-bindings/timer/arc_timer.txt
> @@ -0,0 +1,24 @@
> +ARC Timer
> +
> +Required properties:
> +
> +- compatible : should be "snps,arc-timer".
> +- reg : Specifies timer ID, could be either 0 or 1.
> +- clocks : Specifies clocks that drives the counter.
> +
> +Examples:
> +
> +timer at 0 {
> +       compatible = "snps,arc-timer";
> +       clocks = <&core_clk>;
> +       reg = <0>;
> +};
> +
> +timer at 1 {
> +       compatible = "snps,arc-timer";
> +       clocks = <&core_clk>;
> +       reg = <1>;
> +};
> +
> +NOTE: if you specify both timers, clocks always should be the same
> +as each timer is driven by the same core clock.
> diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig
> index cb18f12..a6c77ea 100644
> --- a/drivers/timer/Kconfig
> +++ b/drivers/timer/Kconfig
> @@ -46,4 +46,13 @@ config OMAP_TIMER
>         help
>           Select this to enable an timer for Omap devices.
>
> +config ARC_TIMER
> +       bool "ARC timer support"
> +       depends on TIMER && ARC && CLK
> +       help
> +         Select this to enable built-in ARC timers.
> +         ARC cores may have up to 2 built-in timers: timer0 and timer1,
> +         usually at least one of them exists. Either of them is supported
> +         in U-Boot.
> +
>  endmenu
> diff --git a/drivers/timer/Makefile b/drivers/timer/Makefile
> index f351fbb..e9624dd 100644
> --- a/drivers/timer/Makefile
> +++ b/drivers/timer/Makefile
> @@ -9,3 +9,4 @@ obj-$(CONFIG_ALTERA_TIMER)      += altera_timer.o
>  obj-$(CONFIG_SANDBOX_TIMER)    += sandbox_timer.o
>  obj-$(CONFIG_X86_TSC_TIMER)    += tsc_timer.o
>  obj-$(CONFIG_OMAP_TIMER)       += omap-timer.o
> +obj-$(CONFIG_ARC_TIMER)        += arc_timer.o
> diff --git a/drivers/timer/arc_timer.c b/drivers/timer/arc_timer.c
> new file mode 100644
> index 0000000..07274f2
> --- /dev/null
> +++ b/drivers/timer/arc_timer.c
> @@ -0,0 +1,103 @@
> +/*
> + * Copyright (C) 2016 Synopsys, Inc. All rights reserved.
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <errno.h>
> +#include <timer.h>
> +#include <asm/io.h>
> +#include <asm/arcregs.h>

put this line before the previous one.

> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +#define NH_MODE (1 << 1)
> +
> +/*
> + * ARC timer control registers are mapped to auxiliary address space.
> + * There are special ARC asm command to access that addresses.
> + * Therefore we use built-in functions to read from and write to timer
> + * control register.
> + */
> +
> +/* Driver private data. Contains timer id. Could be either 0 or 1. */
> +struct arc_timer_priv {
> +               uint timer_id;
> +};
> +
> +static int arc_timer_get_count(struct udevice *dev, u64 *count)
> +{
> +       u32 val = 0;
> +       struct arc_timer_priv *priv = dev_get_priv(dev);

Add blank line

> +       switch (priv->timer_id) {
> +       case 0:
> +               val = read_aux_reg(ARC_AUX_TIMER0_CNT);
> +               break;
> +       case 1:
> +               val = read_aux_reg(ARC_AUX_TIMER1_CNT);
> +               break;
> +       }
> +       *count = timer_conv_64(val);
> +
> +       return 0;
> +}
> +
> +static int arc_timer_probe(struct udevice *dev)
> +{
> +       int id;
> +

Drop bank line

> +       struct arc_timer_priv *priv = dev_get_priv(dev);
> +
> +       /* Get registers offset and size */
> +       id = fdtdec_get_int(gd->fdt_blob, dev->of_offset, "reg", -1);
> +       if (id < 0)
> +               return -EINVAL;
> +
> +       if (id > 1)
> +               return -ENXIO;
> +
> +       priv->timer_id = (uint)id;
> +
> +       switch (priv->timer_id) {
> +       case 0:
> +               /* Disable timer if CPU is halted */
> +               write_aux_reg(ARC_AUX_TIMER0_CTRL, NH_MODE);
> +               /* Set max value for counter/timer */
> +               write_aux_reg(ARC_AUX_TIMER0_LIMIT, 0xffffffff);
> +               /* Set initial count value and restart counter/timer */
> +               write_aux_reg(ARC_AUX_TIMER0_CNT, 0);
> +               break;
> +       case 1:
> +               /* Disable timer if CPU is halted */
> +               write_aux_reg(ARC_AUX_TIMER1_CTRL, NH_MODE);
> +               /* Set max value for counter/timer */
> +               write_aux_reg(ARC_AUX_TIMER1_LIMIT, 0xffffffff);
> +               /* Set initial count value and restart counter/timer */
> +               write_aux_reg(ARC_AUX_TIMER1_CNT, 0);

You are writing the same values in each case. Can you set a value to
either ARC_AUX_TIMER0 or ARC_AUX_TIMER1  and then just have the code
once?

> +               break;
> +       }
> +
> +       return 0;
> +}
> +
> +
> +static const struct timer_ops arc_timer_ops = {
> +       .get_count = arc_timer_get_count,
> +};
> +
> +static const struct udevice_id arc_timer_ids[] = {
> +       { .compatible = "snps,arc-timer" },
> +       {}
> +};
> +
> +U_BOOT_DRIVER(arc_timer) = {
> +       .name   = "arc_timer",
> +       .id     = UCLASS_TIMER,
> +       .of_match = arc_timer_ids,
> +       .probe = arc_timer_probe,
> +       .ops    = &arc_timer_ops,
> +       .flags = DM_FLAG_PRE_RELOC,
> +       .priv_auto_alloc_size = sizeof(struct arc_timer_priv),
> +};
> --
> 2.7.4
>

Regards,
Simon

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH 2/3] arc: dts: separate single axs10x.dts file
  2017-01-16 13:49 ` [U-Boot] [PATCH 2/3] arc: dts: separate single axs10x.dts file Vlad Zakharov
@ 2017-01-21  3:51   ` Simon Glass
  0 siblings, 0 replies; 11+ messages in thread
From: Simon Glass @ 2017-01-21  3:51 UTC (permalink / raw)
  To: u-boot

On 16 January 2017 at 06:49, Vlad Zakharov
<Vladislav.Zakharov@synopsys.com> wrote:
> We want to use the same device tree blobs in both Linux and U-Boot for
> ARC boards.
>
> Earlier device tree sources in U-Boot were very simplified and hadn't been
> updated for quite a long period of time.
>
> So this commit is the first step on the road to unified device tree blobs.
>
> First of all we re-organize device tree sources for AXS10X boards.
> As AXS101 and AXS103 boards consist of AXS10X motherboard and AXC001 and
> AXC003 cpu tiles respectively we add corresponding device tree source
> files: axs10x_mb.dtsi for motherboard, axc001.dtsi and axc003.dtsi for
> CPU tiles and axs101.dts and axs103.dts to represent actual boards.
>
> Also we delete axs10x.dts as it is no longer used.
>
> One more important change - we add timer device to ARC skeleton device
> tree sources as both ARC700 and ARCHS cores contain such timer.
> We add core_clk nodes to abilis_tb100, nsim, axc001 and axc003 device tree
> sources as it is referenced via phandle from timer node in common
> skeleton.dtsi file.
>
> Signed-off-by: Vlad Zakharov <vzakhar@synopsys.com>
> ---
>  arch/arc/Kconfig              |  9 ++++--
>  arch/arc/dts/Makefile         |  3 +-
>  arch/arc/dts/abilis_tb100.dts | 12 ++++++--
>  arch/arc/dts/axc001.dtsi      | 19 +++++++++++++
>  arch/arc/dts/axc003.dtsi      | 19 +++++++++++++
>  arch/arc/dts/axs101.dts       | 19 +++++++++++++
>  arch/arc/dts/axs103.dts       | 19 +++++++++++++
>  arch/arc/dts/axs10x.dts       | 57 -------------------------------------
>  arch/arc/dts/axs10x_mb.dtsi   | 66 +++++++++++++++++++++++++++++++++++++++++++
>  arch/arc/dts/nsim.dts         | 12 ++++++--
>  arch/arc/dts/skeleton.dtsi    | 19 ++++++++++++-
>  board/synopsys/axs10x/Kconfig |  2 +-
>  configs/axs101_defconfig      |  3 +-
>  configs/axs103_defconfig      |  2 +-
>  14 files changed, 190 insertions(+), 71 deletions(-)
>  create mode 100644 arch/arc/dts/axc001.dtsi
>  create mode 100644 arch/arc/dts/axc003.dtsi
>  create mode 100644 arch/arc/dts/axs101.dts
>  create mode 100644 arch/arc/dts/axs103.dts
>  delete mode 100644 arch/arc/dts/axs10x.dts
>  create mode 100644 arch/arc/dts/axs10x_mb.dtsi

Reviewed-by: Simon Glass <sjg@chromium.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH 3/3] arc: use timer driver for ARC boards
  2017-01-16 13:49 ` [U-Boot] [PATCH 3/3] arc: use timer driver for ARC boards Vlad Zakharov
@ 2017-01-21  3:51   ` Simon Glass
  0 siblings, 0 replies; 11+ messages in thread
From: Simon Glass @ 2017-01-21  3:51 UTC (permalink / raw)
  To: u-boot

On 16 January 2017 at 06:49, Vlad Zakharov
<Vladislav.Zakharov@synopsys.com> wrote:
> This commit replaces legacy timer code with usage of arc timer
> driver.
>
> It removes arch/arc/lib/time.c file and selects CONFIG_CLK,
> CONFIG_TIMER and CONFIG_ARC_TIMER options for all ARC boards by default.
> Therefore we remove CONFIG_CLK option from less common axs101 and
> axs103 defconfigs.
>
> Also it removes legacy CONFIG_SYS_TIMER_RATE config symbol from
> axs10x.h, tb100.h and nsim.h configs files as it is no longer required.
>
> Signed-off-by: Vlad Zakharov <vzakhar@synopsys.com>
> ---
>  arch/Kconfig             |  3 +++
>  arch/arc/lib/Makefile    |  1 -
>  arch/arc/lib/timer.c     | 24 ------------------------
>  configs/axs101_defconfig |  1 -
>  configs/axs103_defconfig |  1 -
>  include/configs/axs10x.h |  2 --
>  include/configs/nsim.h   |  5 -----
>  include/configs/tb100.h  |  5 -----
>  8 files changed, 3 insertions(+), 39 deletions(-)
>  delete mode 100644 arch/arc/lib/timer.c

Reviewed-by: Simon Glass <sjg@chromium.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH 1/3] drivers: timer: Introduce ARC timer driver
  2017-01-21  3:51   ` Simon Glass
@ 2017-01-31 14:44     ` Vlad Zakharov
  2017-01-31 15:35       ` Alexey Brodkin
  0 siblings, 1 reply; 11+ messages in thread
From: Vlad Zakharov @ 2017-01-31 14:44 UTC (permalink / raw)
  To: u-boot

Hi Simon,

On Fri, 2017-01-20 at 20:51 -0700, Simon Glass wrote:
> > +?????? switch (priv->timer_id) {
> > +?????? case 0:
> > +?????????????? /* Disable timer if CPU is halted */
> > +?????????????? write_aux_reg(ARC_AUX_TIMER0_CTRL, NH_MODE);
> > +?????????????? /* Set max value for counter/timer */
> > +?????????????? write_aux_reg(ARC_AUX_TIMER0_LIMIT, 0xffffffff);
> > +?????????????? /* Set initial count value and restart counter/timer */
> > +?????????????? write_aux_reg(ARC_AUX_TIMER0_CNT, 0);
> > +?????????????? break;
> > +?????? case 1:
> > +?????????????? /* Disable timer if CPU is halted */
> > +?????????????? write_aux_reg(ARC_AUX_TIMER1_CTRL, NH_MODE);
> > +?????????????? /* Set max value for counter/timer */
> > +?????????????? write_aux_reg(ARC_AUX_TIMER1_LIMIT, 0xffffffff);
> > +?????????????? /* Set initial count value and restart counter/timer */
> > +?????????????? write_aux_reg(ARC_AUX_TIMER1_CNT, 0);
> 
> You are writing the same values in each case. Can you set a value to
> either ARC_AUX_TIMER0 or ARC_AUX_TIMER1? and then just have the code
> once?

Yes, here we have some code duplication. But in fact we have a reason for this. ARC timers are controlled by auxiliary
registers that are not mapped anywhere. They have their fixed numbers and are being accessed using special ARC asm
commands. Of course I can write something like this:
---------------------------------->8------------------------------------
timer_base = priv->timer_id ? ARC_AUX_TIMER1_BASE : ARC_AUX_TIMER0_BASE;

write_aux_reg(timer_base + ARC_TIMER_CTRL, NH_MODE);
write_aux_reg(timer_base + ARC_TIMER_LIMIT, 0xffffffff);
write_aux_reg(timer_base + ARC_TIMER_CNT, 0);
---------------------------------->8------------------------------------
But unfortunately it will not reflect the real life. We don't have any ARC timer base addresses, only fixed register
numbers.

If you insist I will use the latter code snippet. But from our point of view the code is being duplicated in this patch
is very small but helps to understand what is really happening with ARC timers much better.

Thanks.

-- 
Best regards,
Vlad Zakharov <vzakhar@synopsys.com>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH 1/3] drivers: timer: Introduce ARC timer driver
  2017-01-31 14:44     ` Vlad Zakharov
@ 2017-01-31 15:35       ` Alexey Brodkin
  2017-02-06 15:33         ` Simon Glass
  0 siblings, 1 reply; 11+ messages in thread
From: Alexey Brodkin @ 2017-01-31 15:35 UTC (permalink / raw)
  To: u-boot

Hi Simon,

On Tue, 2017-01-31 at 14:44 +0000, Vlad Zakharov wrote:
> Hi Simon,
> 
> On Fri, 2017-01-20 at 20:51 -0700, Simon Glass wrote:
> > 
> > > 
> > > +?????? switch (priv->timer_id) {
> > > +?????? case 0:
> > > +?????????????? /* Disable timer if CPU is halted */
> > > +?????????????? write_aux_reg(ARC_AUX_TIMER0_CTRL, NH_MODE);
> > > +?????????????? /* Set max value for counter/timer */
> > > +?????????????? write_aux_reg(ARC_AUX_TIMER0_LIMIT, 0xffffffff);
> > > +?????????????? /* Set initial count value and restart counter/timer */
> > > +?????????????? write_aux_reg(ARC_AUX_TIMER0_CNT, 0);
> > > +?????????????? break;
> > > +?????? case 1:
> > > +?????????????? /* Disable timer if CPU is halted */
> > > +?????????????? write_aux_reg(ARC_AUX_TIMER1_CTRL, NH_MODE);
> > > +?????????????? /* Set max value for counter/timer */
> > > +?????????????? write_aux_reg(ARC_AUX_TIMER1_LIMIT, 0xffffffff);
> > > +?????????????? /* Set initial count value and restart counter/timer */
> > > +?????????????? write_aux_reg(ARC_AUX_TIMER1_CNT, 0);
> > 
> > You are writing the same values in each case. Can you set a value to
> > either ARC_AUX_TIMER0 or ARC_AUX_TIMER1? and then just have the code
> > once?
> 
> Yes, here we have some code duplication. But in fact we have a reason for this. ARC timers are controlled by auxiliary
> registers that are not mapped anywhere. They have their fixed numbers and are being accessed using special ARC asm
> commands. Of course I can write something like this:
> ---------------------------------->8------------------------------------
> timer_base = priv->timer_id ? ARC_AUX_TIMER1_BASE : ARC_AUX_TIMER0_BASE;
> 
> write_aux_reg(timer_base + ARC_TIMER_CTRL, NH_MODE);
> write_aux_reg(timer_base + ARC_TIMER_LIMIT, 0xffffffff);
> write_aux_reg(timer_base + ARC_TIMER_CNT, 0);
> ---------------------------------->8------------------------------------
> But unfortunately it will not reflect the real life. We don't have any ARC timer base addresses, only fixed register
> numbers.

Just a subtle clarification.

In ARC world we have addition address space for controlling built-in features of the core.
These are so-called AUX[iliary] registers. We access them via special commands like LR/SR
(load/store AUX reg) and each AUX reg has its own pre-defined index.

In other words?ARC_AUX_TIMER0_CTRL and?ARC_AUX_TIMER1_CTRL are 2 very special values/indexes (in terms they are always
the same within all ARC cores with the same ISA). Compared to common MMIO peripherals which might be mapped to different
memory location these are constant. And IMHO for ARC user it is much more natural to see a particular AUX reg number and
then just get all the info about it from ARC core documentation. Otherwise if we start using fake bases it may just
mislead a reader.

-Alexey

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH 1/3] drivers: timer: Introduce ARC timer driver
  2017-01-31 15:35       ` Alexey Brodkin
@ 2017-02-06 15:33         ` Simon Glass
  2017-02-06 16:46           ` Alexey Brodkin
  0 siblings, 1 reply; 11+ messages in thread
From: Simon Glass @ 2017-02-06 15:33 UTC (permalink / raw)
  To: u-boot

Hi Alexey,

On 31 January 2017 at 07:35, Alexey Brodkin <Alexey.Brodkin@synopsys.com> wrote:
> Hi Simon,
>
> On Tue, 2017-01-31 at 14:44 +0000, Vlad Zakharov wrote:
>> Hi Simon,
>>
>> On Fri, 2017-01-20 at 20:51 -0700, Simon Glass wrote:
>> >
>> > >
>> > > +       switch (priv->timer_id) {
>> > > +       case 0:
>> > > +               /* Disable timer if CPU is halted */
>> > > +               write_aux_reg(ARC_AUX_TIMER0_CTRL, NH_MODE);
>> > > +               /* Set max value for counter/timer */
>> > > +               write_aux_reg(ARC_AUX_TIMER0_LIMIT, 0xffffffff);
>> > > +               /* Set initial count value and restart counter/timer */
>> > > +               write_aux_reg(ARC_AUX_TIMER0_CNT, 0);
>> > > +               break;
>> > > +       case 1:
>> > > +               /* Disable timer if CPU is halted */
>> > > +               write_aux_reg(ARC_AUX_TIMER1_CTRL, NH_MODE);
>> > > +               /* Set max value for counter/timer */
>> > > +               write_aux_reg(ARC_AUX_TIMER1_LIMIT, 0xffffffff);
>> > > +               /* Set initial count value and restart counter/timer */
>> > > +               write_aux_reg(ARC_AUX_TIMER1_CNT, 0);
>> >
>> > You are writing the same values in each case. Can you set a value to
>> > either ARC_AUX_TIMER0 or ARC_AUX_TIMER1  and then just have the code
>> > once?
>>
>> Yes, here we have some code duplication. But in fact we have a reason for this. ARC timers are controlled by auxiliary
>> registers that are not mapped anywhere. They have their fixed numbers and are being accessed using special ARC asm
>> commands. Of course I can write something like this:
>> ---------------------------------->8------------------------------------
>> timer_base = priv->timer_id ? ARC_AUX_TIMER1_BASE : ARC_AUX_TIMER0_BASE;
>>
>> write_aux_reg(timer_base + ARC_TIMER_CTRL, NH_MODE);
>> write_aux_reg(timer_base + ARC_TIMER_LIMIT, 0xffffffff);
>> write_aux_reg(timer_base + ARC_TIMER_CNT, 0);
>> ---------------------------------->8------------------------------------
>> But unfortunately it will not reflect the real life. We don't have any ARC timer base addresses, only fixed register
>> numbers.
>
> Just a subtle clarification.
>
> In ARC world we have addition address space for controlling built-in features of the core.
> These are so-called AUX[iliary] registers. We access them via special commands like LR/SR
> (load/store AUX reg) and each AUX reg has its own pre-defined index.
>
> In other words ARC_AUX_TIMER0_CTRL and ARC_AUX_TIMER1_CTRL are 2 very special values/indexes (in terms they are always
> the same within all ARC cores with the same ISA). Compared to common MMIO peripherals which might be mapped to different
> memory location these are constant. And IMHO for ARC user it is much more natural to see a particular AUX reg number and
> then just get all the info about it from ARC core documentation. Otherwise if we start using fake bases it may just
> mislead a reader.

OK that's fine. I don't want to interfere in how ARC does things. It
just stood out as odd :-)

Regards,
Simon

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [U-Boot] [PATCH 1/3] drivers: timer: Introduce ARC timer driver
  2017-02-06 15:33         ` Simon Glass
@ 2017-02-06 16:46           ` Alexey Brodkin
  0 siblings, 0 replies; 11+ messages in thread
From: Alexey Brodkin @ 2017-02-06 16:46 UTC (permalink / raw)
  To: u-boot

Hi Simon,

On Mon, 2017-02-06 at 07:33 -0800, Simon Glass wrote:
> Hi Alexey,
> 
> On 31 January 2017 at 07:35, Alexey Brodkin <Alexey.Brodkin@synopsys.com> wrote:
> > 
> > Hi Simon,
> > 
> > On Tue, 2017-01-31 at 14:44 +0000, Vlad Zakharov wrote:
> > > 
> > > Hi Simon,
> > > 
> > > On Fri, 2017-01-20 at 20:51 -0700, Simon Glass wrote:
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > +???????switch (priv->timer_id) {
> > > > > +???????case 0:
> > > > > +???????????????/* Disable timer if CPU is halted */
> > > > > +???????????????write_aux_reg(ARC_AUX_TIMER0_CTRL, NH_MODE);
> > > > > +???????????????/* Set max value for counter/timer */
> > > > > +???????????????write_aux_reg(ARC_AUX_TIMER0_LIMIT, 0xffffffff);
> > > > > +???????????????/* Set initial count value and restart counter/timer */
> > > > > +???????????????write_aux_reg(ARC_AUX_TIMER0_CNT, 0);
> > > > > +???????????????break;
> > > > > +???????case 1:
> > > > > +???????????????/* Disable timer if CPU is halted */
> > > > > +???????????????write_aux_reg(ARC_AUX_TIMER1_CTRL, NH_MODE);
> > > > > +???????????????/* Set max value for counter/timer */
> > > > > +???????????????write_aux_reg(ARC_AUX_TIMER1_LIMIT, 0xffffffff);
> > > > > +???????????????/* Set initial count value and restart counter/timer */
> > > > > +???????????????write_aux_reg(ARC_AUX_TIMER1_CNT, 0);
> > > > 
> > > > You are writing the same values in each case. Can you set a value to
> > > > either ARC_AUX_TIMER0 or ARC_AUX_TIMER1??and then just have the code
> > > > once?
> > > 
> > > Yes, here we have some code duplication. But in fact we have a reason for this. ARC timers are controlled by
> > > auxiliary
> > > registers that are not mapped anywhere. They have their fixed numbers and are being accessed using special ARC asm
> > > commands. Of course I can write something like this:
> > > ---------------------------------->8------------------------------------
> > > timer_base = priv->timer_id ? ARC_AUX_TIMER1_BASE : ARC_AUX_TIMER0_BASE;
> > > 
> > > write_aux_reg(timer_base + ARC_TIMER_CTRL, NH_MODE);
> > > write_aux_reg(timer_base + ARC_TIMER_LIMIT, 0xffffffff);
> > > write_aux_reg(timer_base + ARC_TIMER_CNT, 0);
> > > ---------------------------------->8------------------------------------
> > > But unfortunately it will not reflect the real life. We don't have any ARC timer base addresses, only fixed
> > > register
> > > numbers.
> > 
> > Just a subtle clarification.
> > 
> > In ARC world we have addition address space for controlling built-in features of the core.
> > These are so-called AUX[iliary] registers. We access them via special commands like LR/SR
> > (load/store AUX reg) and each AUX reg has its own pre-defined index.
> > 
> > In other words ARC_AUX_TIMER0_CTRL and ARC_AUX_TIMER1_CTRL are 2 very special values/indexes (in terms they are
> > always
> > the same within all ARC cores with the same ISA). Compared to common MMIO peripherals which might be mapped to
> > different
> > memory location these are constant. And IMHO for ARC user it is much more natural to see a particular AUX reg number
> > and
> > then just get all the info about it from ARC core documentation. Otherwise if we start using fake bases it may just
> > mislead a reader.
> 
> OK that's fine. I don't want to interfere in how ARC does things. It
> just stood out as odd :-)

That was the whole point so people get it right :)
Maybe there's some sense in adding more comments to this section
to justify that solution.

-Alexey

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-02-06 16:46 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-16 13:49 [U-Boot] [PATCH 0/3] arc: introduce timer driver and update device tree Vlad Zakharov
2017-01-16 13:49 ` [U-Boot] [PATCH 1/3] drivers: timer: Introduce ARC timer driver Vlad Zakharov
2017-01-21  3:51   ` Simon Glass
2017-01-31 14:44     ` Vlad Zakharov
2017-01-31 15:35       ` Alexey Brodkin
2017-02-06 15:33         ` Simon Glass
2017-02-06 16:46           ` Alexey Brodkin
2017-01-16 13:49 ` [U-Boot] [PATCH 2/3] arc: dts: separate single axs10x.dts file Vlad Zakharov
2017-01-21  3:51   ` Simon Glass
2017-01-16 13:49 ` [U-Boot] [PATCH 3/3] arc: use timer driver for ARC boards Vlad Zakharov
2017-01-21  3:51   ` Simon Glass

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.