[PATCH v2 RESEND 0/3] clocksource/vt8500: Fix hangs in small delays

[PATCH v2 RESEND 0/3] 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

v2:
Address comments by Alexey Charkov. Merge patches to get less amount of
changes (three patches instead of four).

Tested on my WM8650, no issues in three days uptime.

Roman Volkov (3):
  clocksource/vt8500: Use MIN_OSCR_DELTA from PXA
  clocksource/vt8500: Remove the 'loops' variable
  clocksource/vt8500: Add register R/W functions

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

-- 
2.6.2

[PATCH v2 RESEND 1/3] 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 a92e94b..dfc3bb4 100644
--- a/drivers/clocksource/vt8500_timer.c
+++ b/drivers/clocksource/vt8500_timer.c
@@ -50,6 +50,8 @@
 
 #define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
 
+#define MIN_OSCR_DELTA		16
+
 static void __iomem *regbase;
 
 static cycle_t vt8500_timer_read(struct clocksource *cs)
@@ -80,7 +82,7 @@ static int vt8500_timer_set_next_event(unsigned long cycles,
 		cpu_relax();
 	writel((unsigned long)alarm, regbase + TIMER_MATCH_VAL);
 
-	if ((signed)(alarm - clocksource.read(&clocksource)) <= 16)
+	if ((signed)(alarm - clocksource.read(&clocksource)) <= MIN_OSCR_DELTA)
 		return -ETIME;
 
 	writel(1, regbase + TIMER_IER_VAL);
@@ -151,7 +153,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 v2 RESEND 2/3] 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).

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

diff --git a/drivers/clocksource/vt8500_timer.c b/drivers/clocksource/vt8500_timer.c
index dfc3bb4..eb08d96 100644
--- a/drivers/clocksource/vt8500_timer.c
+++ b/drivers/clocksource/vt8500_timer.c
@@ -48,18 +48,14 @@
 #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 MIN_OSCR_DELTA		16
 
 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)
+	while (readl(regbase + TIMER_AS_VAL) & TIMER_COUNT_R_ACTIVE)
 		cpu_relax();
 	return readl(regbase + TIMER_COUNT_VAL);
 }
@@ -75,10 +71,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 ((readl(regbase + TIMER_AS_VAL) & TIMER_MATCH_W_ACTIVE)
-						&& --loops)
+	while (readl(regbase + TIMER_AS_VAL) & TIMER_MATCH_W_ACTIVE)
 		cpu_relax();
 	writel((unsigned long)alarm, regbase + TIMER_MATCH_VAL);
 
-- 
2.6.2

[PATCH v2 RESEND 3/3] 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.

Use relaxed read/write, according to the following:
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."

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 | 88 ++++++++++++++++++++++++++++----------
 1 file changed, 66 insertions(+), 22 deletions(-)

diff --git a/drivers/clocksource/vt8500_timer.c b/drivers/clocksource/vt8500_timer.c
index eb08d96..fd88a5b 100644
--- a/drivers/clocksource/vt8500_timer.c
+++ b/drivers/clocksource/vt8500_timer.c
@@ -38,32 +38,75 @@
 
 #define VT8500_TIMER_OFFSET	0x0100
 #define VT8500_TIMER_HZ		3000000
-#define TIMER_MATCH_VAL		0x0000
+#define TIMER_MATCH0_VAL	0x0000
+#define TIMER_MATCH1_VAL	0x0004
+#define TIMER_MATCH2_VAL	0x0008
+#define TIMER_MATCH3_VAL	0x000c
 #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 */
+/* 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 MIN_OSCR_DELTA		16
 
+#define vt8500_timer_sync(bit)	{ while (readl_relaxed \
+				    (regbase + TIMER_AS_VAL) & bit) \
+					cpu_relax(); }
+
 static void __iomem *regbase;
 
-static cycle_t vt8500_timer_read(struct clocksource *cs)
+static void vt8500_timer_write(u32 value, unsigned long reg)
 {
-	writel(3, regbase + TIMER_CTRL_VAL);
-	while (readl(regbase + TIMER_AS_VAL) & TIMER_COUNT_R_ACTIVE)
-		cpu_relax();
-	return readl(regbase + TIMER_COUNT_VAL);
+	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)
+{
+	if (reg == TIMER_COUNT_VAL) {
+		vt8500_timer_write(3, TIMER_CTRL_VAL);
+		vt8500_timer_sync(TIMER_COUNT_R_ACTIVE);
+
+		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,
 };
@@ -71,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 (readl(regbase + TIMER_AS_VAL) & TIMER_MATCH_W_ACTIVE)
-		cpu_relax();
-	writel((unsigned long)alarm, regbase + 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(alarm, TIMER_MATCH0_VAL);
+	if ((signed)(alarm - vt8500_timer_read(
+				TIMER_COUNT_VAL)) <= MIN_OSCR_DELTA) {
 		return -ETIME;
+	}
 
-	writel(1, regbase + TIMER_IER_VAL);
+	vt8500_timer_write(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);
+	vt8500_timer_write(vt8500_timer_read(TIMER_CTRL_VAL) | 1,
+					TIMER_CTRL_VAL);
+	vt8500_timer_write(0, TIMER_IER_VAL);
 	return 0;
 }
 
@@ -103,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;
-	writel(0xf, regbase + TIMER_STATUS_VAL);
+	vt8500_timer_write(0xf, TIMER_STATUS_VAL);
 	evt->event_handler(evt);
 
 	return IRQ_HANDLED;
@@ -133,9 +177,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);
+	vt8500_timer_write(1, TIMER_CTRL_VAL);
+	vt8500_timer_write(0xf, TIMER_STATUS_VAL);
+	vt8500_timer_write(~0, TIMER_MATCH0_VAL);
 
 	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 v2 RESEND 1/3] clocksource/vt8500: Use MIN_OSCR_DELTA from PXA

[Thomas Gleixner]

Roman,

On Thu, 31 Dec 2015, Roman Volkov wrote:
> 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.

Now that brings up the obvious question:

If the vt8500 and PXA timers are identical why has vt8500 it's own slightly
different implementation and does not use the PXA timer?

Thanks,

	tglx

Re: [PATCH v2 RESEND 1/3] clocksource/vt8500: Use MIN_OSCR_DELTA from PXA

[Roman Volkov]

В Thu, 31 Dec 2015 23:33:45 +0100 (CET)
Thomas Gleixner <tglx@linutronix.de> пишет:

> Roman,
> 
> On Thu, 31 Dec 2015, Roman Volkov wrote:
> > 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.  
> 
> Now that brings up the obvious question:
> 
> If the vt8500 and PXA timers are identical why has vt8500 it's own
> slightly different implementation and does not use the PXA timer?

Thomas,

I occasionally noticed that the PXA can be reused, when working on the
bugfix for vt8500. Another good question would be how exactly this code
can be reused. We may rework PXA driver to make it working under
vt8500, or include the C code from the vt8500 and get two slightly
different modules. You may look at our previous discussion with Alexey:

https://lkml.org/lkml/2015/12/21/437

Adding Robert and Bill to get more opinions. At this step, fixing the
vt8500 nanosleep bug is a priority.

Happy New Year,
Roman

Re: [PATCH v2 RESEND 1/3] clocksource/vt8500: Use MIN_OSCR_DELTA from PXA

[Thomas Gleixner]

On Thu, 31 Dec 2015, Roman Volkov wrote:

> 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 changelog makes no sense at all.
 
> 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 a92e94b..dfc3bb4 100644
> --- a/drivers/clocksource/vt8500_timer.c
> +++ b/drivers/clocksource/vt8500_timer.c
> @@ -50,6 +50,8 @@
>  
>  #define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
>  
> +#define MIN_OSCR_DELTA		16
> +
>  static void __iomem *regbase;
>  
>  static cycle_t vt8500_timer_read(struct clocksource *cs)
> @@ -80,7 +82,7 @@ static int vt8500_timer_set_next_event(unsigned long cycles,
>  		cpu_relax();
>  	writel((unsigned long)alarm, regbase + TIMER_MATCH_VAL);
>  
> -	if ((signed)(alarm - clocksource.read(&clocksource)) <= 16)
> +	if ((signed)(alarm - clocksource.read(&clocksource)) <= MIN_OSCR_DELTA)

So how is that value bigger? MIN_OSCR_DELTA is still 16

>  		return -ETIME;
>  
>  	writel(1, regbase + TIMER_IER_VAL);
> @@ -151,7 +153,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);

Now here is the real change. You use a larger minimum tick value so that the
above check in vt8500_timer_set_next_event() actually works. That's what you
want to explain in the changelog. The blurb about reusing MIN_OSCR_DELTA is
just useless.

A proper changelog describes:

  1) the problem and the resulting wreckage

  2) the solution

Thanks,

	tglx

Re: [PATCH v2 RESEND 1/3] clocksource/vt8500: Use MIN_OSCR_DELTA from PXA

[Robert Jarzmik]

Roman Volkov <v1ron@mail.ru> writes:

> В Thu, 31 Dec 2015 23:33:45 +0100 (CET)
> Thomas Gleixner <tglx@linutronix.de> пишет:
>
>> Roman,
>> 
>> On Thu, 31 Dec 2015, Roman Volkov wrote:
>> > 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.  
>> 
>> Now that brings up the obvious question:
>> 
>> If the vt8500 and PXA timers are identical why has vt8500 it's own
>> slightly different implementation and does not use the PXA timer?
>
> Thomas,
>
> I occasionally noticed that the PXA can be reused, when working on the
> bugfix for vt8500. Another good question would be how exactly this code
> can be reused. We may rework PXA driver to make it working under
> vt8500, or include the C code from the vt8500 and get two slightly
> different modules. You may look at our previous discussion with Alexey:
>
> https://lkml.org/lkml/2015/12/21/437
>
> Adding Robert and Bill to get more opinions. At this step, fixing the
> vt8500 nanosleep bug is a priority.

Personnaly I'm not very thrilled by combining pxa and vt8500 drivers into one.

The rationale I have behind is that :
 - the new driver will have new ifs to switch form vt8500 to pxa
   For example, suspend/resume functions will be different.
   Moreover in order to not impact the pxa runtime some ifs will be necessary
   (will that be if (IS_ENABLED(CONFIG_ARCH_PXA) && ...)
 - the IPs do not look that similar to me
   They seems inter-operable, but that seems to me to be just because of the 3
   common register placement : register at match (@0x00), counter (@0x10) and
   interrupt enabled (@0x1c).
   The register acces semantics are different (vt8500 needs a bit to access),
   the input clock seems different.

For ~140 lines of code, I prefer the simplicity brought by drivers
separation. The diffstat should be pretty equivalent between 2 drivers and 1
combined driver with many ifs.

Cheers.

-- 
Robert