[PATCH 0/4] clocksource/vt8500: Fix hangs in small delays

[PATCH 0/4] clocksource/vt8500: Fix hangs in small delays

[Roman Volkov]

From: Roman Volkov <rvolkov@v1ros.org>

vt8500 hangs in nanosleep() function, starting from commit
c6eb3f70d4482806dc2d3e1e3c7736f497b1d418, making the system unusable.
Per investigation, looks like set_next_event() now receives too small
delta and fails with -ETIME.

Google group discussion:
https://groups.google.com/forum/#!topic/vt8500-wm8505-linux-kernel/vDMF_mDOb1k

Roman Volkov (4):
  clocksource/vt8500: Use [read\write]l_relaxed()
  clocksource/vt8500: Remove the 'loops' variable
  clocksource/vt8500: Use MIN_OSCR_DELTA from PXA
  clocksource/vt8500: Add register R/W functions

 drivers/clocksource/vt8500_timer.c | 98 +++++++++++++++++++++++++++-----------
 1 file changed, 69 insertions(+), 29 deletions(-)

-- 
2.6.2

[PATCH 0/4] clocksource/vt8500: Fix hangs in small delays

[Roman Volkov]

From: Roman Volkov <rvolkov@v1ros.org>

vt8500 hangs in nanosleep() function, starting from commit
c6eb3f70d4482806dc2d3e1e3c7736f497b1d418, making the system unusable.
Per investigation, looks like set_next_event() now receives too small
delta and fails with -ETIME.

Google group discussion:
https://groups.google.com/forum/#!topic/vt8500-wm8505-linux-kernel/vDMF_mDOb1k

Roman Volkov (4):
  clocksource/vt8500: Use [read\write]l_relaxed()
  clocksource/vt8500: Remove the 'loops' variable
  clocksource/vt8500: Use MIN_OSCR_DELTA from PXA
  clocksource/vt8500: Add register R/W functions

 drivers/clocksource/vt8500_timer.c | 98 +++++++++++++++++++++++++++-----------
 1 file changed, 69 insertions(+), 29 deletions(-)

-- 
2.6.2

[PATCH 1/4] clocksource/vt8500: Use [read\write]l_relaxed()

[Roman Volkov]

From: Roman Volkov <rvolkov@v1ros.org>

It is more preferred way to use relaxed read\write without barriers on ARM.

http://permalink.gmane.org/gmane.linux.ports.arm.kernel/117658:
"For accesses to the same device we don't
actually need any barriers on ARM as this is guaranteed by the
architecture."

Also wrap these calls into macroses to save space.

Signed-off-by: Roman Volkov <rvolkov@v1ros.org>
---
 drivers/clocksource/vt8500_timer.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/clocksource/vt8500_timer.c b/drivers/clocksource/vt8500_timer.c
index a92e94b..13ed892 100644
--- a/drivers/clocksource/vt8500_timer.c
+++ b/drivers/clocksource/vt8500_timer.c
@@ -50,16 +50,18 @@
 
 #define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
 
+#define timer_readl(addr)	readl_relaxed(regbase + addr)
+#define timer_writel(v, addr)	writel_relaxed(v, regbase + addr)
+
 static void __iomem *regbase;
 
 static cycle_t vt8500_timer_read(struct clocksource *cs)
 {
 	int loops = msecs_to_loops(10);
-	writel(3, regbase + TIMER_CTRL_VAL);
-	while ((readl((regbase + TIMER_AS_VAL)) & TIMER_COUNT_R_ACTIVE)
-						&& --loops)
+	timer_writel(3, TIMER_CTRL_VAL);
+	while ((timer_readl((TIMER_AS_VAL)) & TIMER_COUNT_R_ACTIVE) && --loops)
 		cpu_relax();
-	return readl(regbase + TIMER_COUNT_VAL);
+	return timer_readl(TIMER_COUNT_VAL);
 }
 
 static struct clocksource clocksource = {
@@ -75,23 +77,22 @@ static int vt8500_timer_set_next_event(unsigned long cycles,
 {
 	int loops = msecs_to_loops(10);
 	cycle_t alarm = clocksource.read(&clocksource) + cycles;
-	while ((readl(regbase + TIMER_AS_VAL) & TIMER_MATCH_W_ACTIVE)
-						&& --loops)
+	while ((timer_readl(TIMER_AS_VAL) & TIMER_MATCH_W_ACTIVE) && --loops)
 		cpu_relax();
-	writel((unsigned long)alarm, regbase + TIMER_MATCH_VAL);
+	timer_writel((unsigned long)alarm, TIMER_MATCH_VAL);
 
 	if ((signed)(alarm - clocksource.read(&clocksource)) <= 16)
 		return -ETIME;
 
-	writel(1, regbase + TIMER_IER_VAL);
+	timer_writel(1, TIMER_IER_VAL);
 
 	return 0;
 }
 
 static int vt8500_shutdown(struct clock_event_device *evt)
 {
-	writel(readl(regbase + TIMER_CTRL_VAL) | 1, regbase + TIMER_CTRL_VAL);
-	writel(0, regbase + TIMER_IER_VAL);
+	timer_writel(timer_readl(TIMER_CTRL_VAL) | 1, TIMER_CTRL_VAL);
+	timer_writel(0, TIMER_IER_VAL);
 	return 0;
 }
 
@@ -107,7 +108,7 @@ static struct clock_event_device clockevent = {
 static irqreturn_t vt8500_timer_interrupt(int irq, void *dev_id)
 {
 	struct clock_event_device *evt = dev_id;
-	writel(0xf, regbase + TIMER_STATUS_VAL);
+	timer_writel(0xf, TIMER_STATUS_VAL);
 	evt->event_handler(evt);
 
 	return IRQ_HANDLED;
@@ -137,9 +138,9 @@ static void __init vt8500_timer_init(struct device_node *np)
 		return;
 	}
 
-	writel(1, regbase + TIMER_CTRL_VAL);
-	writel(0xf, regbase + TIMER_STATUS_VAL);
-	writel(~0, regbase + TIMER_MATCH_VAL);
+	timer_writel(1, TIMER_CTRL_VAL);
+	timer_writel(0xf, TIMER_STATUS_VAL);
+	timer_writel(~0, TIMER_MATCH_VAL);
 
 	if (clocksource_register_hz(&clocksource, VT8500_TIMER_HZ))
 		pr_err("%s: vt8500_timer_init: clocksource_register failed for %s\n",
-- 
2.6.2

[PATCH 1/4] clocksource/vt8500: Use [read\write]l_relaxed()

[Roman Volkov]

From: Roman Volkov <rvolkov@v1ros.org>

It is more preferred way to use relaxed read\write without barriers on ARM.

http://permalink.gmane.org/gmane.linux.ports.arm.kernel/117658:
"For accesses to the same device we don't
actually need any barriers on ARM as this is guaranteed by the
architecture."

Also wrap these calls into macroses to save space.

Signed-off-by: Roman Volkov <rvolkov@v1ros.org>
---
 drivers/clocksource/vt8500_timer.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/clocksource/vt8500_timer.c b/drivers/clocksource/vt8500_timer.c
index a92e94b..13ed892 100644
--- a/drivers/clocksource/vt8500_timer.c
+++ b/drivers/clocksource/vt8500_timer.c
@@ -50,16 +50,18 @@
 
 #define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
 
+#define timer_readl(addr)	readl_relaxed(regbase + addr)
+#define timer_writel(v, addr)	writel_relaxed(v, regbase + addr)
+
 static void __iomem *regbase;
 
 static cycle_t vt8500_timer_read(struct clocksource *cs)
 {
 	int loops = msecs_to_loops(10);
-	writel(3, regbase + TIMER_CTRL_VAL);
-	while ((readl((regbase + TIMER_AS_VAL)) & TIMER_COUNT_R_ACTIVE)
-						&& --loops)
+	timer_writel(3, TIMER_CTRL_VAL);
+	while ((timer_readl((TIMER_AS_VAL)) & TIMER_COUNT_R_ACTIVE) && --loops)
 		cpu_relax();
-	return readl(regbase + TIMER_COUNT_VAL);
+	return timer_readl(TIMER_COUNT_VAL);
 }
 
 static struct clocksource clocksource = {
@@ -75,23 +77,22 @@ static int vt8500_timer_set_next_event(unsigned long cycles,
 {
 	int loops = msecs_to_loops(10);
 	cycle_t alarm = clocksource.read(&clocksource) + cycles;
-	while ((readl(regbase + TIMER_AS_VAL) & TIMER_MATCH_W_ACTIVE)
-						&& --loops)
+	while ((timer_readl(TIMER_AS_VAL) & TIMER_MATCH_W_ACTIVE) && --loops)
 		cpu_relax();
-	writel((unsigned long)alarm, regbase + TIMER_MATCH_VAL);
+	timer_writel((unsigned long)alarm, TIMER_MATCH_VAL);
 
 	if ((signed)(alarm - clocksource.read(&clocksource)) <= 16)
 		return -ETIME;
 
-	writel(1, regbase + TIMER_IER_VAL);
+	timer_writel(1, TIMER_IER_VAL);
 
 	return 0;
 }
 
 static int vt8500_shutdown(struct clock_event_device *evt)
 {
-	writel(readl(regbase + TIMER_CTRL_VAL) | 1, regbase + TIMER_CTRL_VAL);
-	writel(0, regbase + TIMER_IER_VAL);
+	timer_writel(timer_readl(TIMER_CTRL_VAL) | 1, TIMER_CTRL_VAL);
+	timer_writel(0, TIMER_IER_VAL);
 	return 0;
 }
 
@@ -107,7 +108,7 @@ static struct clock_event_device clockevent = {
 static irqreturn_t vt8500_timer_interrupt(int irq, void *dev_id)
 {
 	struct clock_event_device *evt = dev_id;
-	writel(0xf, regbase + TIMER_STATUS_VAL);
+	timer_writel(0xf, TIMER_STATUS_VAL);
 	evt->event_handler(evt);
 
 	return IRQ_HANDLED;
@@ -137,9 +138,9 @@ static void __init vt8500_timer_init(struct device_node *np)
 		return;
 	}
 
-	writel(1, regbase + TIMER_CTRL_VAL);
-	writel(0xf, regbase + TIMER_STATUS_VAL);
-	writel(~0, regbase + TIMER_MATCH_VAL);
+	timer_writel(1, TIMER_CTRL_VAL);
+	timer_writel(0xf, TIMER_STATUS_VAL);
+	timer_writel(~0, TIMER_MATCH_VAL);
 
 	if (clocksource_register_hz(&clocksource, VT8500_TIMER_HZ))
 		pr_err("%s: vt8500_timer_init: clocksource_register failed for %s\n",
-- 
2.6.2

[PATCH 2/4] clocksource/vt8500: Remove the 'loops' variable

[Roman Volkov]

From: Roman Volkov <rvolkov@v1ros.org>

The purpose of the 'loops' variable is unclear. vt8500 hardware does not
require any protections, in case if these variables intended for preventing
infinite loops (identical PXA timer works perfectly without these ones). If
the loops count will not be calculated correctly, these variables will only
break the loop too early and introduce problems.

Signed-off-by: Roman Volkov <rvolkov@v1ros.org>
---
 drivers/clocksource/vt8500_timer.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/clocksource/vt8500_timer.c b/drivers/clocksource/vt8500_timer.c
index 13ed892..f40ded8 100644
--- a/drivers/clocksource/vt8500_timer.c
+++ b/drivers/clocksource/vt8500_timer.c
@@ -48,8 +48,6 @@
 #define TIMER_COUNT_W_ACTIVE	(1 << 4)	/* not ready for write */
 #define TIMER_MATCH_W_ACTIVE	(1 << 0)	/* not ready for write */
 
-#define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
-
 #define timer_readl(addr)	readl_relaxed(regbase + addr)
 #define timer_writel(v, addr)	writel_relaxed(v, regbase + addr)
 
@@ -57,10 +55,10 @@ static void __iomem *regbase;
 
 static cycle_t vt8500_timer_read(struct clocksource *cs)
 {
-	int loops = msecs_to_loops(10);
 	timer_writel(3, TIMER_CTRL_VAL);
-	while ((timer_readl((TIMER_AS_VAL)) & TIMER_COUNT_R_ACTIVE) && --loops)
+	while (timer_readl(TIMER_AS_VAL) & TIMER_COUNT_R_ACTIVE)
 		cpu_relax();
+
 	return timer_readl(TIMER_COUNT_VAL);
 }
 
@@ -75,9 +73,8 @@ static struct clocksource clocksource = {
 static int vt8500_timer_set_next_event(unsigned long cycles,
 				    struct clock_event_device *evt)
 {
-	int loops = msecs_to_loops(10);
 	cycle_t alarm = clocksource.read(&clocksource) + cycles;
-	while ((timer_readl(TIMER_AS_VAL) & TIMER_MATCH_W_ACTIVE) && --loops)
+	while (timer_readl(TIMER_AS_VAL) & TIMER_MATCH_W_ACTIVE)
 		cpu_relax();
 	timer_writel((unsigned long)alarm, TIMER_MATCH_VAL);
 
-- 
2.6.2

[PATCH 2/4] clocksource/vt8500: Remove the 'loops' variable

[Roman Volkov]

From: Roman Volkov <rvolkov@v1ros.org>

The purpose of the 'loops' variable is unclear. vt8500 hardware does not
require any protections, in case if these variables intended for preventing
infinite loops (identical PXA timer works perfectly without these ones). If
the loops count will not be calculated correctly, these variables will only
break the loop too early and introduce problems.

Signed-off-by: Roman Volkov <rvolkov@v1ros.org>
---
 drivers/clocksource/vt8500_timer.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/clocksource/vt8500_timer.c b/drivers/clocksource/vt8500_timer.c
index 13ed892..f40ded8 100644
--- a/drivers/clocksource/vt8500_timer.c
+++ b/drivers/clocksource/vt8500_timer.c
@@ -48,8 +48,6 @@
 #define TIMER_COUNT_W_ACTIVE	(1 << 4)	/* not ready for write */
 #define TIMER_MATCH_W_ACTIVE	(1 << 0)	/* not ready for write */
 
-#define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
-
 #define timer_readl(addr)	readl_relaxed(regbase + addr)
 #define timer_writel(v, addr)	writel_relaxed(v, regbase + addr)
 
@@ -57,10 +55,10 @@ static void __iomem *regbase;
 
 static cycle_t vt8500_timer_read(struct clocksource *cs)
 {
-	int loops = msecs_to_loops(10);
 	timer_writel(3, TIMER_CTRL_VAL);
-	while ((timer_readl((TIMER_AS_VAL)) & TIMER_COUNT_R_ACTIVE) && --loops)
+	while (timer_readl(TIMER_AS_VAL) & TIMER_COUNT_R_ACTIVE)
 		cpu_relax();
+
 	return timer_readl(TIMER_COUNT_VAL);
 }
 
@@ -75,9 +73,8 @@ static struct clocksource clocksource = {
 static int vt8500_timer_set_next_event(unsigned long cycles,
 				    struct clock_event_device *evt)
 {
-	int loops = msecs_to_loops(10);
 	cycle_t alarm = clocksource.read(&clocksource) + cycles;
-	while ((timer_readl(TIMER_AS_VAL) & TIMER_MATCH_W_ACTIVE) && --loops)
+	while (timer_readl(TIMER_AS_VAL) & TIMER_MATCH_W_ACTIVE)
 		cpu_relax();
 	timer_writel((unsigned long)alarm, TIMER_MATCH_VAL);
 
-- 
2.6.2

[PATCH 3/4] clocksource/vt8500: Use MIN_OSCR_DELTA from PXA

[Roman Volkov]

From: Roman Volkov <rvolkov@v1ros.org>

Since vt8500 and PXA timers are identical, use MIN_OSCR_DELTA from PXA,
which is bigger than existing value. It is required to determine the
minimum delay which hardware can generate.

This commit fixes vt8500 breakage in Linux 4.2 introduced by
c6eb3f7 ('hrtimer: Get rid of hrtimer softirq')

Signed-off-by: Roman Volkov <rvolkov@v1ros.org>
---
 drivers/clocksource/vt8500_timer.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/clocksource/vt8500_timer.c b/drivers/clocksource/vt8500_timer.c
index f40ded8..7649852 100644
--- a/drivers/clocksource/vt8500_timer.c
+++ b/drivers/clocksource/vt8500_timer.c
@@ -51,6 +51,8 @@
 #define timer_readl(addr)	readl_relaxed(regbase + addr)
 #define timer_writel(v, addr)	writel_relaxed(v, regbase + addr)
 
+#define MIN_OSCR_DELTA		16
+
 static void __iomem *regbase;
 
 static cycle_t vt8500_timer_read(struct clocksource *cs)
@@ -78,7 +80,7 @@ static int vt8500_timer_set_next_event(unsigned long cycles,
 		cpu_relax();
 	timer_writel((unsigned long)alarm, TIMER_MATCH_VAL);
 
-	if ((signed)(alarm - clocksource.read(&clocksource)) <= 16)
+	if ((signed)(alarm - clocksource.read(&clocksource)) <= MIN_OSCR_DELTA)
 		return -ETIME;
 
 	timer_writel(1, TIMER_IER_VAL);
@@ -149,7 +151,7 @@ static void __init vt8500_timer_init(struct device_node *np)
 		pr_err("%s: setup_irq failed for %s\n", __func__,
 							clockevent.name);
 	clockevents_config_and_register(&clockevent, VT8500_TIMER_HZ,
-					4, 0xf0000000);
+					MIN_OSCR_DELTA * 2, 0xf0000000);
 }
 
 CLOCKSOURCE_OF_DECLARE(vt8500, "via,vt8500-timer", vt8500_timer_init);
-- 
2.6.2

[PATCH 3/4] clocksource/vt8500: Use MIN_OSCR_DELTA from PXA

[Roman Volkov]

From: Roman Volkov <rvolkov@v1ros.org>

Since vt8500 and PXA timers are identical, use MIN_OSCR_DELTA from PXA,
which is bigger than existing value. It is required to determine the
minimum delay which hardware can generate.

This commit fixes vt8500 breakage in Linux 4.2 introduced by
c6eb3f7 ('hrtimer: Get rid of hrtimer softirq')

Signed-off-by: Roman Volkov <rvolkov@v1ros.org>
---
 drivers/clocksource/vt8500_timer.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/clocksource/vt8500_timer.c b/drivers/clocksource/vt8500_timer.c
index f40ded8..7649852 100644
--- a/drivers/clocksource/vt8500_timer.c
+++ b/drivers/clocksource/vt8500_timer.c
@@ -51,6 +51,8 @@
 #define timer_readl(addr)	readl_relaxed(regbase + addr)
 #define timer_writel(v, addr)	writel_relaxed(v, regbase + addr)
 
+#define MIN_OSCR_DELTA		16
+
 static void __iomem *regbase;
 
 static cycle_t vt8500_timer_read(struct clocksource *cs)
@@ -78,7 +80,7 @@ static int vt8500_timer_set_next_event(unsigned long cycles,
 		cpu_relax();
 	timer_writel((unsigned long)alarm, TIMER_MATCH_VAL);
 
-	if ((signed)(alarm - clocksource.read(&clocksource)) <= 16)
+	if ((signed)(alarm - clocksource.read(&clocksource)) <= MIN_OSCR_DELTA)
 		return -ETIME;
 
 	timer_writel(1, TIMER_IER_VAL);
@@ -149,7 +151,7 @@ static void __init vt8500_timer_init(struct device_node *np)
 		pr_err("%s: setup_irq failed for %s\n", __func__,
 							clockevent.name);
 	clockevents_config_and_register(&clockevent, VT8500_TIMER_HZ,
-					4, 0xf0000000);
+					MIN_OSCR_DELTA * 2, 0xf0000000);
 }
 
 CLOCKSOURCE_OF_DECLARE(vt8500, "via,vt8500-timer", vt8500_timer_init);
-- 
2.6.2

[PATCH 4/4] clocksource/vt8500: Add register R/W functions

[Roman Volkov]

From: Roman Volkov <rvolkov@v1ros.org>

vt8500 timer requires special synchronization for accessing some of its
registers. Define special read and write functions to handle this process
transparently.

To perform a read from the Timer Count register, user must write a one
to the Timer Control register and wait for completion flag by polling the
Timer Read Count Active bit.

To perform a write to the Count or Match registers, user must poll the
write completion flag for the corresponding register to ensure that the
previous write completed and then write the actual value.

Signed-off-by: Roman Volkov <rvolkov@v1ros.org>
---
 drivers/clocksource/vt8500_timer.c | 90 +++++++++++++++++++++++++++-----------
 1 file changed, 65 insertions(+), 25 deletions(-)

diff --git a/drivers/clocksource/vt8500_timer.c b/drivers/clocksource/vt8500_timer.c
index 7649852..4d7513f 100644
--- a/drivers/clocksource/vt8500_timer.c
+++ b/drivers/clocksource/vt8500_timer.c
@@ -38,36 +38,75 @@
 
 #define VT8500_TIMER_OFFSET	0x0100
 #define VT8500_TIMER_HZ		3000000
-#define TIMER_MATCH_VAL		0x0000
+#define TIMER_MATCH0_VAL	0
+#define TIMER_MATCH1_VAL	0x04
+#define TIMER_MATCH2_VAL	0x08
+#define TIMER_MATCH3_VAL	0x0c
 #define TIMER_COUNT_VAL		0x0010
 #define TIMER_STATUS_VAL	0x0014
 #define TIMER_IER_VAL		0x001c		/* interrupt enable */
 #define TIMER_CTRL_VAL		0x0020
 #define TIMER_AS_VAL		0x0024		/* access status */
-#define TIMER_COUNT_R_ACTIVE	(1 << 5)	/* not ready for read */
-#define TIMER_COUNT_W_ACTIVE	(1 << 4)	/* not ready for write */
-#define TIMER_MATCH_W_ACTIVE	(1 << 0)	/* not ready for write */
-
-#define timer_readl(addr)	readl_relaxed(regbase + addr)
-#define timer_writel(v, addr)	writel_relaxed(v, regbase + addr)
+/* R/W status flags */
+#define TIMER_COUNT_R_ACTIVE	(1 << 5)
+#define TIMER_COUNT_W_ACTIVE	(1 << 4)
+#define TIMER_MATCH3_W_ACTIVE	(1 << 3)
+#define TIMER_MATCH2_W_ACTIVE	(1 << 2)
+#define TIMER_MATCH1_W_ACTIVE	(1 << 1)
+#define TIMER_MATCH0_W_ACTIVE	(1 << 0)
+
+#define vt8500_timer_sync(bit)	{ while (readl_relaxed \
+				    (regbase + TIMER_AS_VAL) & bit) \
+					cpu_relax(); }
 
 #define MIN_OSCR_DELTA		16
 
 static void __iomem *regbase;
 
-static cycle_t vt8500_timer_read(struct clocksource *cs)
+static void vt8500_timer_write(unsigned long reg, u32 value)
+{
+	switch (reg) {
+	case TIMER_COUNT_VAL:
+		vt8500_timer_sync(TIMER_COUNT_W_ACTIVE);
+		break;
+	case TIMER_MATCH0_VAL:
+		vt8500_timer_sync(TIMER_MATCH0_W_ACTIVE);
+		break;
+	case TIMER_MATCH1_VAL:
+		vt8500_timer_sync(TIMER_MATCH1_W_ACTIVE);
+		break;
+	case TIMER_MATCH2_VAL:
+		vt8500_timer_sync(TIMER_MATCH2_W_ACTIVE);
+		break;
+	case TIMER_MATCH3_VAL:
+		vt8500_timer_sync(TIMER_MATCH3_W_ACTIVE);
+		break;
+	}
+
+	writel_relaxed(value, regbase + reg);
+}
+
+static u32 vt8500_timer_read(unsigned long reg)
 {
-	timer_writel(3, TIMER_CTRL_VAL);
-	while (timer_readl(TIMER_AS_VAL) & TIMER_COUNT_R_ACTIVE)
-		cpu_relax();
+	if (reg == TIMER_COUNT_VAL) {
+		vt8500_timer_write(TIMER_CTRL_VAL, 3);
+		vt8500_timer_sync(TIMER_COUNT_R_ACTIVE);
 
-	return timer_readl(TIMER_COUNT_VAL);
+		return readl_relaxed(regbase + TIMER_COUNT_VAL);
+	}
+
+	return readl_relaxed(regbase + reg);
+}
+
+static cycle_t vt8500_oscr0_read(struct clocksource *cs)
+{
+	return vt8500_timer_read(TIMER_COUNT_VAL);
 }
 
 static struct clocksource clocksource = {
 	.name           = "vt8500_timer",
 	.rating         = 200,
-	.read           = vt8500_timer_read,
+	.read           = vt8500_oscr0_read,
 	.mask           = CLOCKSOURCE_MASK(32),
 	.flags          = CLOCK_SOURCE_IS_CONTINUOUS,
 };
@@ -75,23 +114,24 @@ static struct clocksource clocksource = {
 static int vt8500_timer_set_next_event(unsigned long cycles,
 				    struct clock_event_device *evt)
 {
-	cycle_t alarm = clocksource.read(&clocksource) + cycles;
-	while (timer_readl(TIMER_AS_VAL) & TIMER_MATCH_W_ACTIVE)
-		cpu_relax();
-	timer_writel((unsigned long)alarm, TIMER_MATCH_VAL);
+	unsigned long alarm = vt8500_timer_read(TIMER_COUNT_VAL) + cycles;
 
-	if ((signed)(alarm - clocksource.read(&clocksource)) <= MIN_OSCR_DELTA)
+	vt8500_timer_write(TIMER_MATCH0_VAL, alarm);
+	if ((signed)(alarm - vt8500_timer_read(
+				TIMER_COUNT_VAL)) <= MIN_OSCR_DELTA) {
 		return -ETIME;
+	}
 
-	timer_writel(1, TIMER_IER_VAL);
+	vt8500_timer_write(TIMER_IER_VAL, 1);
 
 	return 0;
 }
 
 static int vt8500_shutdown(struct clock_event_device *evt)
 {
-	timer_writel(timer_readl(TIMER_CTRL_VAL) | 1, TIMER_CTRL_VAL);
-	timer_writel(0, TIMER_IER_VAL);
+	vt8500_timer_write(TIMER_CTRL_VAL,
+				vt8500_timer_read(TIMER_CTRL_VAL) | 1);
+	vt8500_timer_write(TIMER_IER_VAL, 0);
 	return 0;
 }
 
@@ -107,7 +147,7 @@ static struct clock_event_device clockevent = {
 static irqreturn_t vt8500_timer_interrupt(int irq, void *dev_id)
 {
 	struct clock_event_device *evt = dev_id;
-	timer_writel(0xf, TIMER_STATUS_VAL);
+	vt8500_timer_write(TIMER_STATUS_VAL, 0xf);
 	evt->event_handler(evt);
 
 	return IRQ_HANDLED;
@@ -137,9 +177,9 @@ static void __init vt8500_timer_init(struct device_node *np)
 		return;
 	}
 
-	timer_writel(1, TIMER_CTRL_VAL);
-	timer_writel(0xf, TIMER_STATUS_VAL);
-	timer_writel(~0, TIMER_MATCH_VAL);
+	vt8500_timer_write(TIMER_CTRL_VAL, 1);
+	vt8500_timer_write(TIMER_STATUS_VAL, 0xf);
+	vt8500_timer_write(TIMER_MATCH0_VAL, ~0);
 
 	if (clocksource_register_hz(&clocksource, VT8500_TIMER_HZ))
 		pr_err("%s: vt8500_timer_init: clocksource_register failed for %s\n",
-- 
2.6.2

[PATCH 4/4] clocksource/vt8500: Add register R/W functions

[Roman Volkov]

From: Roman Volkov <rvolkov@v1ros.org>

vt8500 timer requires special synchronization for accessing some of its
registers. Define special read and write functions to handle this process
transparently.

To perform a read from the Timer Count register, user must write a one
to the Timer Control register and wait for completion flag by polling the
Timer Read Count Active bit.

To perform a write to the Count or Match registers, user must poll the
write completion flag for the corresponding register to ensure that the
previous write completed and then write the actual value.

Signed-off-by: Roman Volkov <rvolkov@v1ros.org>
---
 drivers/clocksource/vt8500_timer.c | 90 +++++++++++++++++++++++++++-----------
 1 file changed, 65 insertions(+), 25 deletions(-)

diff --git a/drivers/clocksource/vt8500_timer.c b/drivers/clocksource/vt8500_timer.c
index 7649852..4d7513f 100644
--- a/drivers/clocksource/vt8500_timer.c
+++ b/drivers/clocksource/vt8500_timer.c
@@ -38,36 +38,75 @@
 
 #define VT8500_TIMER_OFFSET	0x0100
 #define VT8500_TIMER_HZ		3000000
-#define TIMER_MATCH_VAL		0x0000
+#define TIMER_MATCH0_VAL	0
+#define TIMER_MATCH1_VAL	0x04
+#define TIMER_MATCH2_VAL	0x08
+#define TIMER_MATCH3_VAL	0x0c
 #define TIMER_COUNT_VAL		0x0010
 #define TIMER_STATUS_VAL	0x0014
 #define TIMER_IER_VAL		0x001c		/* interrupt enable */
 #define TIMER_CTRL_VAL		0x0020
 #define TIMER_AS_VAL		0x0024		/* access status */
-#define TIMER_COUNT_R_ACTIVE	(1 << 5)	/* not ready for read */
-#define TIMER_COUNT_W_ACTIVE	(1 << 4)	/* not ready for write */
-#define TIMER_MATCH_W_ACTIVE	(1 << 0)	/* not ready for write */
-
-#define timer_readl(addr)	readl_relaxed(regbase + addr)
-#define timer_writel(v, addr)	writel_relaxed(v, regbase + addr)
+/* R/W status flags */
+#define TIMER_COUNT_R_ACTIVE	(1 << 5)
+#define TIMER_COUNT_W_ACTIVE	(1 << 4)
+#define TIMER_MATCH3_W_ACTIVE	(1 << 3)
+#define TIMER_MATCH2_W_ACTIVE	(1 << 2)
+#define TIMER_MATCH1_W_ACTIVE	(1 << 1)
+#define TIMER_MATCH0_W_ACTIVE	(1 << 0)
+
+#define vt8500_timer_sync(bit)	{ while (readl_relaxed \
+				    (regbase + TIMER_AS_VAL) & bit) \
+					cpu_relax(); }
 
 #define MIN_OSCR_DELTA		16
 
 static void __iomem *regbase;
 
-static cycle_t vt8500_timer_read(struct clocksource *cs)
+static void vt8500_timer_write(unsigned long reg, u32 value)
+{
+	switch (reg) {
+	case TIMER_COUNT_VAL:
+		vt8500_timer_sync(TIMER_COUNT_W_ACTIVE);
+		break;
+	case TIMER_MATCH0_VAL:
+		vt8500_timer_sync(TIMER_MATCH0_W_ACTIVE);
+		break;
+	case TIMER_MATCH1_VAL:
+		vt8500_timer_sync(TIMER_MATCH1_W_ACTIVE);
+		break;
+	case TIMER_MATCH2_VAL:
+		vt8500_timer_sync(TIMER_MATCH2_W_ACTIVE);
+		break;
+	case TIMER_MATCH3_VAL:
+		vt8500_timer_sync(TIMER_MATCH3_W_ACTIVE);
+		break;
+	}
+
+	writel_relaxed(value, regbase + reg);
+}
+
+static u32 vt8500_timer_read(unsigned long reg)
 {
-	timer_writel(3, TIMER_CTRL_VAL);
-	while (timer_readl(TIMER_AS_VAL) & TIMER_COUNT_R_ACTIVE)
-		cpu_relax();
+	if (reg == TIMER_COUNT_VAL) {
+		vt8500_timer_write(TIMER_CTRL_VAL, 3);
+		vt8500_timer_sync(TIMER_COUNT_R_ACTIVE);
 
-	return timer_readl(TIMER_COUNT_VAL);
+		return readl_relaxed(regbase + TIMER_COUNT_VAL);
+	}
+
+	return readl_relaxed(regbase + reg);
+}
+
+static cycle_t vt8500_oscr0_read(struct clocksource *cs)
+{
+	return vt8500_timer_read(TIMER_COUNT_VAL);
 }
 
 static struct clocksource clocksource = {
 	.name           = "vt8500_timer",
 	.rating         = 200,
-	.read           = vt8500_timer_read,
+	.read           = vt8500_oscr0_read,
 	.mask           = CLOCKSOURCE_MASK(32),
 	.flags          = CLOCK_SOURCE_IS_CONTINUOUS,
 };
@@ -75,23 +114,24 @@ static struct clocksource clocksource = {
 static int vt8500_timer_set_next_event(unsigned long cycles,
 				    struct clock_event_device *evt)
 {
-	cycle_t alarm = clocksource.read(&clocksource) + cycles;
-	while (timer_readl(TIMER_AS_VAL) & TIMER_MATCH_W_ACTIVE)
-		cpu_relax();
-	timer_writel((unsigned long)alarm, TIMER_MATCH_VAL);
+	unsigned long alarm = vt8500_timer_read(TIMER_COUNT_VAL) + cycles;
 
-	if ((signed)(alarm - clocksource.read(&clocksource)) <= MIN_OSCR_DELTA)
+	vt8500_timer_write(TIMER_MATCH0_VAL, alarm);
+	if ((signed)(alarm - vt8500_timer_read(
+				TIMER_COUNT_VAL)) <= MIN_OSCR_DELTA) {
 		return -ETIME;
+	}
 
-	timer_writel(1, TIMER_IER_VAL);
+	vt8500_timer_write(TIMER_IER_VAL, 1);
 
 	return 0;
 }
 
 static int vt8500_shutdown(struct clock_event_device *evt)
 {
-	timer_writel(timer_readl(TIMER_CTRL_VAL) | 1, TIMER_CTRL_VAL);
-	timer_writel(0, TIMER_IER_VAL);
+	vt8500_timer_write(TIMER_CTRL_VAL,
+				vt8500_timer_read(TIMER_CTRL_VAL) | 1);
+	vt8500_timer_write(TIMER_IER_VAL, 0);
 	return 0;
 }
 
@@ -107,7 +147,7 @@ static struct clock_event_device clockevent = {
 static irqreturn_t vt8500_timer_interrupt(int irq, void *dev_id)
 {
 	struct clock_event_device *evt = dev_id;
-	timer_writel(0xf, TIMER_STATUS_VAL);
+	vt8500_timer_write(TIMER_STATUS_VAL, 0xf);
 	evt->event_handler(evt);
 
 	return IRQ_HANDLED;
@@ -137,9 +177,9 @@ static void __init vt8500_timer_init(struct device_node *np)
 		return;
 	}
 
-	timer_writel(1, TIMER_CTRL_VAL);
-	timer_writel(0xf, TIMER_STATUS_VAL);
-	timer_writel(~0, TIMER_MATCH_VAL);
+	vt8500_timer_write(TIMER_CTRL_VAL, 1);
+	vt8500_timer_write(TIMER_STATUS_VAL, 0xf);
+	vt8500_timer_write(TIMER_MATCH0_VAL, ~0);
 
 	if (clocksource_register_hz(&clocksource, VT8500_TIMER_HZ))
 		pr_err("%s: vt8500_timer_init: clocksource_register failed for %s\n",
-- 
2.6.2

Re: [PATCH 0/4] clocksource/vt8500: Fix hangs in small delays

[Roman Volkov]

On Mon, 21 Dec 2015 01:28:08 +0300
Roman Volkov <v1ron@mail.ru> wrote:

> From: Roman Volkov <rvolkov@v1ros.org>
> 
> vt8500 hangs in nanosleep() function, starting from commit
> c6eb3f70d4482806dc2d3e1e3c7736f497b1d418, making the system unusable.
> Per investigation, looks like set_next_event() now receives too small
> delta and fails with -ETIME.
> 
> Google group discussion:
> https://groups.google.com/forum/#!topic/vt8500-wm8505-linux-kernel/vDMF_mDOb1k
> 
> Roman Volkov (4):
>   clocksource/vt8500: Use [read\write]l_relaxed()
>   clocksource/vt8500: Remove the 'loops' variable
>   clocksource/vt8500: Use MIN_OSCR_DELTA from PXA
>   clocksource/vt8500: Add register R/W functions
> 
>  drivers/clocksource/vt8500_timer.c | 98
> +++++++++++++++++++++++++++----------- 1 file changed, 69
> insertions(+), 29 deletions(-)
> 

Please note that vt8500 maintainer (Tony Prisk) was silent in the last
year. Any suggestions on this?

Regards,
Roman

[PATCH 0/4] clocksource/vt8500: Fix hangs in small delays

[Roman Volkov]

On Mon, 21 Dec 2015 01:28:08 +0300
Roman Volkov <v1ron@mail.ru> wrote:

> From: Roman Volkov <rvolkov@v1ros.org>
> 
> vt8500 hangs in nanosleep() function, starting from commit
> c6eb3f70d4482806dc2d3e1e3c7736f497b1d418, making the system unusable.
> Per investigation, looks like set_next_event() now receives too small
> delta and fails with -ETIME.
> 
> Google group discussion:
> https://groups.google.com/forum/#!topic/vt8500-wm8505-linux-kernel/vDMF_mDOb1k
> 
> Roman Volkov (4):
>   clocksource/vt8500: Use [read\write]l_relaxed()
>   clocksource/vt8500: Remove the 'loops' variable
>   clocksource/vt8500: Use MIN_OSCR_DELTA from PXA
>   clocksource/vt8500: Add register R/W functions
> 
>  drivers/clocksource/vt8500_timer.c | 98
> +++++++++++++++++++++++++++----------- 1 file changed, 69
> insertions(+), 29 deletions(-)
> 

Please note that vt8500 maintainer (Tony Prisk) was silent in the last
year. Any suggestions on this?

Regards,
Roman

Re: [PATCH 4/4] clocksource/vt8500: Add register R/W functions

[Alexey Charkov]

Roman Volkov <v1ron <at> mail.ru> writes:

> 
> From: Roman Volkov <rvolkov <at> v1ros.org>
> 
> vt8500 timer requires special synchronization for accessing some of its
> registers. Define special read and write functions to handle this process
> transparently.

Maybe introduce such accessor functions (conditionally) into the PXA driver
and kill this one altogether then?

If I understood you right, this extra bus synchronization is the only thing
that makes vt8500 different from PXA, so merging the two files right away
might be a better long-term option.

> To perform a read from the Timer Count register, user must write a one
> to the Timer Control register and wait for completion flag by polling the
> Timer Read Count Active bit.
> 
> To perform a write to the Count or Match registers, user must poll the
> write completion flag for the corresponding register to ensure that the
> previous write completed and then write the actual value.
> 
> Signed-off-by: Roman Volkov <rvolkov <at> v1ros.org>
> ---
>  drivers/clocksource/vt8500_timer.c | 90
+++++++++++++++++++++++++++-----------
>  1 file changed, 65 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/clocksource/vt8500_timer.c
b/drivers/clocksource/vt8500_timer.c
> index 7649852..4d7513f 100644
> --- a/drivers/clocksource/vt8500_timer.c
> +++ b/drivers/clocksource/vt8500_timer.c
>  <at>  <at>  -38,36 +38,75  <at>  <at> 
> 
>  #define VT8500_TIMER_OFFSET	0x0100
>  #define VT8500_TIMER_HZ		3000000
> -#define TIMER_MATCH_VAL		0x0000
> +#define TIMER_MATCH0_VAL	0
> +#define TIMER_MATCH1_VAL	0x04
> +#define TIMER_MATCH2_VAL	0x08
> +#define TIMER_MATCH3_VAL	0x0c
>  #define TIMER_COUNT_VAL		0x0010
>  #define TIMER_STATUS_VAL	0x0014
>  #define TIMER_IER_VAL		0x001c		/* interrupt enable */
>  #define TIMER_CTRL_VAL		0x0020
>  #define TIMER_AS_VAL		0x0024		/* access status */
> -#define TIMER_COUNT_R_ACTIVE	(1 << 5)	/* not ready for read */
> -#define TIMER_COUNT_W_ACTIVE	(1 << 4)	/* not ready for write */
> -#define TIMER_MATCH_W_ACTIVE	(1 << 0)	/* not ready for write */
> -
> -#define timer_readl(addr)	readl_relaxed(regbase + addr)
> -#define timer_writel(v, addr)	writel_relaxed(v, regbase + addr)
> +/* R/W status flags */
> +#define TIMER_COUNT_R_ACTIVE	(1 << 5)
> +#define TIMER_COUNT_W_ACTIVE	(1 << 4)
> +#define TIMER_MATCH3_W_ACTIVE	(1 << 3)
> +#define TIMER_MATCH2_W_ACTIVE	(1 << 2)
> +#define TIMER_MATCH1_W_ACTIVE	(1 << 1)
> +#define TIMER_MATCH0_W_ACTIVE	(1 << 0)
> +
> +#define vt8500_timer_sync(bit)	{ while (readl_relaxed \
> +				    (regbase + TIMER_AS_VAL) & bit) \
> +					cpu_relax(); }

The whole issue around 'loops' counter in these busy waits basically boils
down to whether we would like a way to try and recover from a potential
hardware misbehavior.

You can of course argue that when the system timer misbehaves you already
have bigger issues to worry about, but does a 10 msec limit that was in the
original version really hurt?

>  #define MIN_OSCR_DELTA		16
> 
>  static void __iomem *regbase;
> 
> -static cycle_t vt8500_timer_read(struct clocksource *cs)
> +static void vt8500_timer_write(unsigned long reg, u32 value)

Maybe define this with 'value' first, 'reg' second - to be in line with the
common prototype of writel and such?

Plus if you could take the same name for the macro above (timer_writel) and
this accessor (vt8500_timer_write) that would somewhat reduce extra
additions/deletions in this patch. Same for the read function.

<skip>

>  <at>  <at>  -75,23 +114,24  <at>  <at>  static struct clocksource
clocksource = {
>  static int vt8500_timer_set_next_event(unsigned long cycles,
>  				    struct clock_event_device *evt)
>  {
> -	cycle_t alarm = clocksource.read(&clocksource) + cycles;
> -	while (timer_readl(TIMER_AS_VAL) & TIMER_MATCH_W_ACTIVE)
> -		cpu_relax();
> -	timer_writel((unsigned long)alarm, TIMER_MATCH_VAL);
> +	unsigned long alarm = vt8500_timer_read(TIMER_COUNT_VAL) + cycles;

I personally like the form above better (via clocksource.read) - even if
just for the fact that it's shorter and reduces the number of places where
we use TIMER_COUNT_VAL definition.

Any specific reasons to rewrite it?

> -	if ((signed)(alarm - clocksource.read(&clocksource)) <= MIN_OSCR_DELTA)
> +	vt8500_timer_write(TIMER_MATCH0_VAL, alarm);
> +	if ((signed)(alarm - vt8500_timer_read(
> +				TIMER_COUNT_VAL)) <= MIN_OSCR_DELTA) {

Same here.

<skip>

Best regards,
Alexey

Re: [PATCH 0/4] clocksource/vt8500: Fix hangs in small delays

[Roman Volkov]

> Roman Volkov <v1ron <at> mail.ru> writes:
> 
> > 
> > From: Roman Volkov <rvolkov <at> v1ros.org>
> > 
> > vt8500 timer requires special synchronization for accessing some of
> > its registers. Define special read and write functions to handle
> > this process transparently.
> 
> Maybe introduce such accessor functions (conditionally) into the PXA
> driver and kill this one altogether then?

I don't think maintainers will accept a lot of #ifdefs. I have an idea
to move the common code from the PXA to something like pxa_common.c
(can we give the correct name for it?) and include it from the
vt8500_timer.c and pxa_timer.c.

> If I understood you right, this extra bus synchronization is the only
> thing that makes vt8500 different from PXA, so merging the two files
> right away might be a better long-term option.

Sure. But lets make the vt8500 working at this step.

> > To perform a read from the Timer Count register, user must write a
> > one to the Timer Control register and wait for completion flag by
> > polling the Timer Read Count Active bit.
> > 
> > To perform a write to the Count or Match registers, user must poll
> > the write completion flag for the corresponding register to ensure
> > that the previous write completed and then write the actual value.
> > 
> > Signed-off-by: Roman Volkov <rvolkov <at> v1ros.org>
> > ---
> >  drivers/clocksource/vt8500_timer.c | 90
> +++++++++++++++++++++++++++-----------
> >  1 file changed, 65 insertions(+), 25 deletions(-)
> 
> > diff --git a/drivers/clocksource/vt8500_timer.c
> b/drivers/clocksource/vt8500_timer.c
> > index 7649852..4d7513f 100644
> > --- a/drivers/clocksource/vt8500_timer.c
> > +++ b/drivers/clocksource/vt8500_timer.c
> >  <at>  <at>  -38,36 +38,75  <at>  <at> 
> > 
> >  #define VT8500_TIMER_OFFSET	0x0100
> >  #define VT8500_TIMER_HZ		3000000
> > -#define TIMER_MATCH_VAL		0x0000
> > +#define TIMER_MATCH0_VAL	0
> > +#define TIMER_MATCH1_VAL	0x04
> > +#define TIMER_MATCH2_VAL	0x08
> > +#define TIMER_MATCH3_VAL	0x0c
> >  #define TIMER_COUNT_VAL		0x0010
> >  #define TIMER_STATUS_VAL	0x0014
> >  #define TIMER_IER_VAL		0x001c		/*
> > interrupt enable */ #define TIMER_CTRL_VAL		0x0020
> >  #define TIMER_AS_VAL		0x0024		/*
> > access status */ -#define TIMER_COUNT_R_ACTIVE	(1 <<
> > 5)	/* not ready for read */ -#define
> > TIMER_COUNT_W_ACTIVE	(1 << 4)	/* not ready for write
> > */ -#define TIMER_MATCH_W_ACTIVE	(1 << 0)	/* not
> > ready for write */ - -#define timer_readl(addr)
> > readl_relaxed(regbase + addr) -#define timer_writel(v, addr)
> > writel_relaxed(v, regbase + addr) +/* R/W status flags */
> > +#define TIMER_COUNT_R_ACTIVE	(1 << 5)
> > +#define TIMER_COUNT_W_ACTIVE	(1 << 4)
> > +#define TIMER_MATCH3_W_ACTIVE	(1 << 3)
> > +#define TIMER_MATCH2_W_ACTIVE	(1 << 2)
> > +#define TIMER_MATCH1_W_ACTIVE	(1 << 1)
> > +#define TIMER_MATCH0_W_ACTIVE	(1 << 0)
> > +
> > +#define vt8500_timer_sync(bit)	{ while (readl_relaxed \
> > +				    (regbase + TIMER_AS_VAL) &
> > bit) \
> > +					cpu_relax(); }
> 
> The whole issue around 'loops' counter in these busy waits basically
> boils down to whether we would like a way to try and recover from a
> potential hardware misbehavior.
> 
> You can of course argue that when the system timer misbehaves you
> already have bigger issues to worry about, but does a 10 msec limit
> that was in the original version really hurt?

If we do the merging work with PXA, this code will go away. Have this
variable already fixed some _real_ problem? Otherwise this is excessive
coding.

> >  #define MIN_OSCR_DELTA		16
> > 
> >  static void __iomem *regbase;
> > 
> > -static cycle_t vt8500_timer_read(struct clocksource *cs)
> > +static void vt8500_timer_write(unsigned long reg, u32 value)
> 
> Maybe define this with 'value' first, 'reg' second - to be in line
> with the common prototype of writel and such?

Oh my right-handed habits :)

> Plus if you could take the same name for the macro above
> (timer_writel) and this accessor (vt8500_timer_write) that would
> somewhat reduce extra additions/deletions in this patch. Same for the
> read function.

When we perform our merging work, we have to use the following glue:
...
vt8500_timer_read { ... }
#define timer_read(reg)	vt8500_timer_read(reg)
...
#include pxa_common.c

> <skip>
> 
> >  <at>  <at>  -75,23 +114,24  <at>  <at>  static struct clocksource
> clocksource = {
> >  static int vt8500_timer_set_next_event(unsigned long cycles,
> >  				    struct clock_event_device *evt)
> >  {
> > -	cycle_t alarm = clocksource.read(&clocksource) + cycles;
> > -	while (timer_readl(TIMER_AS_VAL) & TIMER_MATCH_W_ACTIVE)
> > -		cpu_relax();
> > -	timer_writel((unsigned long)alarm, TIMER_MATCH_VAL);
> > +	unsigned long alarm = vt8500_timer_read(TIMER_COUNT_VAL) +
> > cycles;
> 
> I personally like the form above better (via clocksource.read) - even
> if just for the fact that it's shorter and reduces the number of
> places where we use TIMER_COUNT_VAL definition.
> 
> Any specific reasons to rewrite it?

To avoid calculation of the 64-bit cycle_t and remove type casts.

> > -	if ((signed)(alarm - clocksource.read(&clocksource)) <=
> > MIN_OSCR_DELTA)
> > +	vt8500_timer_write(TIMER_MATCH0_VAL, alarm);
> > +	if ((signed)(alarm - vt8500_timer_read(
> > +				TIMER_COUNT_VAL)) <=
> > MIN_OSCR_DELTA) {
> 
> Same here.

[PATCH 0/4] clocksource/vt8500: Fix hangs in small delays

[Roman Volkov]

> Roman Volkov <v1ron <at> mail.ru> writes:
> 
> > 
> > From: Roman Volkov <rvolkov <at> v1ros.org>
> > 
> > vt8500 timer requires special synchronization for accessing some of
> > its registers. Define special read and write functions to handle
> > this process transparently.
> 
> Maybe introduce such accessor functions (conditionally) into the PXA
> driver and kill this one altogether then?

I don't think maintainers will accept a lot of #ifdefs. I have an idea
to move the common code from the PXA to something like pxa_common.c
(can we give the correct name for it?) and include it from the
vt8500_timer.c and pxa_timer.c.

> If I understood you right, this extra bus synchronization is the only
> thing that makes vt8500 different from PXA, so merging the two files
> right away might be a better long-term option.

Sure. But lets make the vt8500 working at this step.

> > To perform a read from the Timer Count register, user must write a
> > one to the Timer Control register and wait for completion flag by
> > polling the Timer Read Count Active bit.
> > 
> > To perform a write to the Count or Match registers, user must poll
> > the write completion flag for the corresponding register to ensure
> > that the previous write completed and then write the actual value.
> > 
> > Signed-off-by: Roman Volkov <rvolkov <at> v1ros.org>
> > ---
> >  drivers/clocksource/vt8500_timer.c | 90
> +++++++++++++++++++++++++++-----------
> >  1 file changed, 65 insertions(+), 25 deletions(-)
> 
> > diff --git a/drivers/clocksource/vt8500_timer.c
> b/drivers/clocksource/vt8500_timer.c
> > index 7649852..4d7513f 100644
> > --- a/drivers/clocksource/vt8500_timer.c
> > +++ b/drivers/clocksource/vt8500_timer.c
> >  <at>  <at>  -38,36 +38,75  <at>  <at> 
> > 
> >  #define VT8500_TIMER_OFFSET	0x0100
> >  #define VT8500_TIMER_HZ		3000000
> > -#define TIMER_MATCH_VAL		0x0000
> > +#define TIMER_MATCH0_VAL	0
> > +#define TIMER_MATCH1_VAL	0x04
> > +#define TIMER_MATCH2_VAL	0x08
> > +#define TIMER_MATCH3_VAL	0x0c
> >  #define TIMER_COUNT_VAL		0x0010
> >  #define TIMER_STATUS_VAL	0x0014
> >  #define TIMER_IER_VAL		0x001c		/*
> > interrupt enable */ #define TIMER_CTRL_VAL		0x0020
> >  #define TIMER_AS_VAL		0x0024		/*
> > access status */ -#define TIMER_COUNT_R_ACTIVE	(1 <<
> > 5)	/* not ready for read */ -#define
> > TIMER_COUNT_W_ACTIVE	(1 << 4)	/* not ready for write
> > */ -#define TIMER_MATCH_W_ACTIVE	(1 << 0)	/* not
> > ready for write */ - -#define timer_readl(addr)
> > readl_relaxed(regbase + addr) -#define timer_writel(v, addr)
> > writel_relaxed(v, regbase + addr) +/* R/W status flags */
> > +#define TIMER_COUNT_R_ACTIVE	(1 << 5)
> > +#define TIMER_COUNT_W_ACTIVE	(1 << 4)
> > +#define TIMER_MATCH3_W_ACTIVE	(1 << 3)
> > +#define TIMER_MATCH2_W_ACTIVE	(1 << 2)
> > +#define TIMER_MATCH1_W_ACTIVE	(1 << 1)
> > +#define TIMER_MATCH0_W_ACTIVE	(1 << 0)
> > +
> > +#define vt8500_timer_sync(bit)	{ while (readl_relaxed \
> > +				    (regbase + TIMER_AS_VAL) &
> > bit) \
> > +					cpu_relax(); }
> 
> The whole issue around 'loops' counter in these busy waits basically
> boils down to whether we would like a way to try and recover from a
> potential hardware misbehavior.
> 
> You can of course argue that when the system timer misbehaves you
> already have bigger issues to worry about, but does a 10 msec limit
> that was in the original version really hurt?

If we do the merging work with PXA, this code will go away. Have this
variable already fixed some _real_ problem? Otherwise this is excessive
coding.

> >  #define MIN_OSCR_DELTA		16
> > 
> >  static void __iomem *regbase;
> > 
> > -static cycle_t vt8500_timer_read(struct clocksource *cs)
> > +static void vt8500_timer_write(unsigned long reg, u32 value)
> 
> Maybe define this with 'value' first, 'reg' second - to be in line
> with the common prototype of writel and such?

Oh my right-handed habits :)

> Plus if you could take the same name for the macro above
> (timer_writel) and this accessor (vt8500_timer_write) that would
> somewhat reduce extra additions/deletions in this patch. Same for the
> read function.

When we perform our merging work, we have to use the following glue:
...
vt8500_timer_read { ... }
#define timer_read(reg)	vt8500_timer_read(reg)
...
#include pxa_common.c

> <skip>
> 
> >  <at>  <at>  -75,23 +114,24  <at>  <at>  static struct clocksource
> clocksource = {
> >  static int vt8500_timer_set_next_event(unsigned long cycles,
> >  				    struct clock_event_device *evt)
> >  {
> > -	cycle_t alarm = clocksource.read(&clocksource) + cycles;
> > -	while (timer_readl(TIMER_AS_VAL) & TIMER_MATCH_W_ACTIVE)
> > -		cpu_relax();
> > -	timer_writel((unsigned long)alarm, TIMER_MATCH_VAL);
> > +	unsigned long alarm = vt8500_timer_read(TIMER_COUNT_VAL) +
> > cycles;
> 
> I personally like the form above better (via clocksource.read) - even
> if just for the fact that it's shorter and reduces the number of
> places where we use TIMER_COUNT_VAL definition.
> 
> Any specific reasons to rewrite it?

To avoid calculation of the 64-bit cycle_t and remove type casts.

> > -	if ((signed)(alarm - clocksource.read(&clocksource)) <=
> > MIN_OSCR_DELTA)
> > +	vt8500_timer_write(TIMER_MATCH0_VAL, alarm);
> > +	if ((signed)(alarm - vt8500_timer_read(
> > +				TIMER_COUNT_VAL)) <=
> > MIN_OSCR_DELTA) {
> 
> Same here.

Re: [PATCH 0/4] clocksource/vt8500: Fix hangs in small delays

[Alexey Charkov]

2015-12-21 23:33 GMT+03:00 Roman Volkov <v1ron@mail.ru>:
>> Roman Volkov <v1ron <at> mail.ru> writes:
>>
>> >
>> > From: Roman Volkov <rvolkov <at> v1ros.org>
>> >
>> > vt8500 timer requires special synchronization for accessing some of
>> > its registers. Define special read and write functions to handle
>> > this process transparently.
>>
>> Maybe introduce such accessor functions (conditionally) into the PXA
>> driver and kill this one altogether then?
>
> I don't think maintainers will accept a lot of #ifdefs. I have an idea
> to move the common code from the PXA to something like pxa_common.c
> (can we give the correct name for it?) and include it from the
> vt8500_timer.c and pxa_timer.c.

Can't this be done via different OF compatible strings, setting some
global 'static bool needs_bus_sync = true' or such for vt8500, and
then checking it in the accessor functions? Then no ifdefs needed, and
the driver can actually work for both variants within the same kernel
image.

<skip>

>> > +#define vt8500_timer_sync(bit)     { while (readl_relaxed \
>> > +                               (regbase + TIMER_AS_VAL) &
>> > bit) \
>> > +                                   cpu_relax(); }
>>
>> The whole issue around 'loops' counter in these busy waits basically
>> boils down to whether we would like a way to try and recover from a
>> potential hardware misbehavior.
>>
>> You can of course argue that when the system timer misbehaves you
>> already have bigger issues to worry about, but does a 10 msec limit
>> that was in the original version really hurt?
>
> If we do the merging work with PXA, this code will go away. Have this
> variable already fixed some _real_ problem? Otherwise this is excessive
> coding.

Never had any issues without the guarding loop counter. I got that
suggestion as part of feedback to my original submission of the timer
code, and thought it doesn't hurt. Can't say I'm particularly attached
to it, though, so fine for me if you drop it :)

>> >  #define MIN_OSCR_DELTA             16
>> >
>> >  static void __iomem *regbase;
>> >
>> > -static cycle_t vt8500_timer_read(struct clocksource *cs)
>> > +static void vt8500_timer_write(unsigned long reg, u32 value)
>>
>> Maybe define this with 'value' first, 'reg' second - to be in line
>> with the common prototype of writel and such?
>
> Oh my right-handed habits :)
>
>> Plus if you could take the same name for the macro above
>> (timer_writel) and this accessor (vt8500_timer_write) that would
>> somewhat reduce extra additions/deletions in this patch. Same for the
>> read function.
>
> When we perform our merging work, we have to use the following glue:
> ...
> vt8500_timer_read { ... }
> #define timer_read(reg) vt8500_timer_read(reg)
> ...
> #include pxa_common.c

...or make it a runtime switch without ifdefs or includes. Anyway,
this only matters for making the patch series more readable - just
reducing the number of insertions/additions that don't have much
semantic meaning.

Best regards,
Alexey

[PATCH 0/4] clocksource/vt8500: Fix hangs in small delays

[Alexey Charkov]

2015-12-21 23:33 GMT+03:00 Roman Volkov <v1ron@mail.ru>:
>> Roman Volkov <v1ron <at> mail.ru> writes:
>>
>> >
>> > From: Roman Volkov <rvolkov <at> v1ros.org>
>> >
>> > vt8500 timer requires special synchronization for accessing some of
>> > its registers. Define special read and write functions to handle
>> > this process transparently.
>>
>> Maybe introduce such accessor functions (conditionally) into the PXA
>> driver and kill this one altogether then?
>
> I don't think maintainers will accept a lot of #ifdefs. I have an idea
> to move the common code from the PXA to something like pxa_common.c
> (can we give the correct name for it?) and include it from the
> vt8500_timer.c and pxa_timer.c.

Can't this be done via different OF compatible strings, setting some
global 'static bool needs_bus_sync = true' or such for vt8500, and
then checking it in the accessor functions? Then no ifdefs needed, and
the driver can actually work for both variants within the same kernel
image.

<skip>

>> > +#define vt8500_timer_sync(bit)     { while (readl_relaxed \
>> > +                               (regbase + TIMER_AS_VAL) &
>> > bit) \
>> > +                                   cpu_relax(); }
>>
>> The whole issue around 'loops' counter in these busy waits basically
>> boils down to whether we would like a way to try and recover from a
>> potential hardware misbehavior.
>>
>> You can of course argue that when the system timer misbehaves you
>> already have bigger issues to worry about, but does a 10 msec limit
>> that was in the original version really hurt?
>
> If we do the merging work with PXA, this code will go away. Have this
> variable already fixed some _real_ problem? Otherwise this is excessive
> coding.

Never had any issues without the guarding loop counter. I got that
suggestion as part of feedback to my original submission of the timer
code, and thought it doesn't hurt. Can't say I'm particularly attached
to it, though, so fine for me if you drop it :)

>> >  #define MIN_OSCR_DELTA             16
>> >
>> >  static void __iomem *regbase;
>> >
>> > -static cycle_t vt8500_timer_read(struct clocksource *cs)
>> > +static void vt8500_timer_write(unsigned long reg, u32 value)
>>
>> Maybe define this with 'value' first, 'reg' second - to be in line
>> with the common prototype of writel and such?
>
> Oh my right-handed habits :)
>
>> Plus if you could take the same name for the macro above
>> (timer_writel) and this accessor (vt8500_timer_write) that would
>> somewhat reduce extra additions/deletions in this patch. Same for the
>> read function.
>
> When we perform our merging work, we have to use the following glue:
> ...
> vt8500_timer_read { ... }
> #define timer_read(reg) vt8500_timer_read(reg)
> ...
> #include pxa_common.c

...or make it a runtime switch without ifdefs or includes. Anyway,
this only matters for making the patch series more readable - just
reducing the number of insertions/additions that don't have much
semantic meaning.

Best regards,
Alexey