Re: [PATCH v3 5/7] clocksource/cadence_ttc: Use only one counter

2014-02-10 Thread Sören Brinkmann 
Hi Thomas,

On Tue, Feb 04, 2014 at 09:41:53PM +0100, Thomas Gleixner wrote:
> On Mon, 3 Feb 2014, Soren Brinkmann wrote:
> 
> > Currently the driver uses two of the three counters the TTC provides to
> > implement a clocksource and a clockevent device. By using the TTC's
> > match feature we can implement both use cases using a single counter
> > only.
> 
> Are you entirely sure that this match feature is free of the infamous
> HPET match feature issues?
> 
> See arch/x86/kernel/hpet.c: hpet_next_event()
> 
> If yes, please add a comment. If no 
Good catch. Looks like the comparator compares on == as well. So, I
assume it may show similar issues. I'll have to spend some more time
looking into this.

Thanks for applying the first two patches of the series.

Thanks,
Sören


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 5/7] clocksource/cadence_ttc: Use only one counter

2014-02-10 Thread Sören Brinkmann 
Hi Thomas,

On Tue, Feb 04, 2014 at 09:41:53PM +0100, Thomas Gleixner wrote:
 On Mon, 3 Feb 2014, Soren Brinkmann wrote:
 
  Currently the driver uses two of the three counters the TTC provides to
  implement a clocksource and a clockevent device. By using the TTC's
  match feature we can implement both use cases using a single counter
  only.
 
 Are you entirely sure that this match feature is free of the infamous
 HPET match feature issues?
 
 See arch/x86/kernel/hpet.c: hpet_next_event()
 
 If yes, please add a comment. If no 
Good catch. Looks like the comparator compares on == as well. So, I
assume it may show similar issues. I'll have to spend some more time
looking into this.

Thanks for applying the first two patches of the series.

Thanks,
Sören


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 5/7] clocksource/cadence_ttc: Use only one counter

2014-02-04 Thread Thomas Gleixner 
On Mon, 3 Feb 2014, Soren Brinkmann wrote:

> Currently the driver uses two of the three counters the TTC provides to
> implement a clocksource and a clockevent device. By using the TTC's
> match feature we can implement both use cases using a single counter
> only.

Are you entirely sure that this match feature is free of the infamous
HPET match feature issues?

See arch/x86/kernel/hpet.c: hpet_next_event()

If yes, please add a comment. If no 

Thanks,

tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 5/7] clocksource/cadence_ttc: Use only one counter

2014-02-04 Thread Thomas Gleixner 
On Mon, 3 Feb 2014, Soren Brinkmann wrote:

 Currently the driver uses two of the three counters the TTC provides to
 implement a clocksource and a clockevent device. By using the TTC's
 match feature we can implement both use cases using a single counter
 only.

Are you entirely sure that this match feature is free of the infamous
HPET match feature issues?

See arch/x86/kernel/hpet.c: hpet_next_event()

If yes, please add a comment. If no 

Thanks,

tglx
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v3 5/7] clocksource/cadence_ttc: Use only one counter

2014-02-03 Thread Soren Brinkmann 
Currently the driver uses two of the three counters the TTC provides to
implement a clocksource and a clockevent device. By using the TTC's
match feature we can implement both use cases using a single counter
only.
The old approach is to use timer over-/underflow to generate an
interrupt. Using the match register allows to generate an interrupt on
arbitrary counter values. This way a dedicated clockevent counter can be
avoided.

Signed-off-by: Soren Brinkmann 
---
 drivers/clocksource/cadence_ttc_timer.c | 92 +++--
 1 file changed, 31 insertions(+), 61 deletions(-)

diff --git a/drivers/clocksource/cadence_ttc_timer.c 
b/drivers/clocksource/cadence_ttc_timer.c
index 49fbe2847c84..d922e982af95 100644
--- a/drivers/clocksource/cadence_ttc_timer.c
+++ b/drivers/clocksource/cadence_ttc_timer.c
@@ -47,6 +47,7 @@
 #define TTC_CNT_CNTRL_OFFSET   0x0C /* Counter Control Reg, RW */
 #define TTC_COUNT_VAL_OFFSET   0x18 /* Counter Value Reg, RO */
 #define TTC_INTR_VAL_OFFSET0x24 /* Interval Count Reg, RW */
+#define TTC_MATCH1_OFFSET  0x30 /* Match reg, RW */
 #define TTC_ISR_OFFSET 0x54 /* Interrupt Status Reg, RO */
 #define TTC_IER_OFFSET 0x60 /* Interrupt Enable Reg, RW */
 
@@ -64,7 +65,10 @@
 #define PRESCALE   2048/* The exponent must match this */
 #define CLK_CNTRL_PRESCALE ((PRESCALE_EXPONENT - 1) << 1)
 #define CLK_CNTRL_PRESCALE_EN  1
-#define CNT_CNTRL_RESET(1 << 4)
+#define CNT_CNTRL_RESETBIT(4)
+#define CNT_CNTRL_MATCHBIT(3)
+
+#define TTC_INTERRUPT_MATCH1   BIT(1)
 
 #define MAX_F_ERR 50
 
@@ -99,6 +103,7 @@ struct ttc_timer_clocksource {
 struct ttc_timer_clockevent {
struct ttc_timerttc;
struct clock_event_device   ce;
+   unsigned long   interval;
 };
 
 #define to_ttc_timer_clkevent(x) \
@@ -112,25 +117,20 @@ static void __iomem *ttc_sched_clock_val_reg;
  * @timer: Pointer to the timer instance
  * @cycles:Timer interval ticks
  **/
-static void ttc_set_interval(struct ttc_timer *timer,
-   unsigned long cycles)
+static void ttc_set_interval(struct ttc_timer *timer, unsigned long cycles)
 {
-   u32 ctrl_reg;
+   struct ttc_timer_clockevent *ttcce = container_of(timer,
+   struct ttc_timer_clockevent, ttc);
 
-   /* Disable the counter, set the counter value  and re-enable counter */
-   ctrl_reg = __raw_readl(timer->base_addr + TTC_CNT_CNTRL_OFFSET);
-   ctrl_reg |= TTC_CNT_CNTRL_DISABLE_MASK;
-   __raw_writel(ctrl_reg, timer->base_addr + TTC_CNT_CNTRL_OFFSET);
+   /* set interval */
+   u32 reg = __raw_readl(timer->base_addr + TTC_COUNT_VAL_OFFSET);
+   reg += cycles;
+   __raw_writel(reg, timer->base_addr + TTC_MATCH1_OFFSET);
 
-   __raw_writel(cycles, timer->base_addr + TTC_INTR_VAL_OFFSET);
+   /* enable match interrupt */
+   __raw_writel(TTC_INTERRUPT_MATCH1, timer->base_addr + TTC_IER_OFFSET);
 
-   /*
-* Reset the counter (0x10) so that it starts from 0, one-shot
-* mode makes this needed for timing to be right.
-*/
-   ctrl_reg |= CNT_CNTRL_RESET;
-   ctrl_reg &= ~TTC_CNT_CNTRL_DISABLE_MASK;
-   __raw_writel(ctrl_reg, timer->base_addr + TTC_CNT_CNTRL_OFFSET);
+   ttcce->interval = cycles;
 }
 
 /**
@@ -148,6 +148,8 @@ static irqreturn_t ttc_clock_event_interrupt(int irq, void 
*dev_id)
 
/* Acknowledge the interrupt and call event handler */
__raw_readl(timer->base_addr + TTC_ISR_OFFSET);
+   if (ttce->ce.mode == CLOCK_EVT_MODE_PERIODIC)
+   ttc_set_interval(timer, ttce->interval);
 
ttce->ce.event_handler(>ce);
 
@@ -201,7 +203,6 @@ static void ttc_set_mode(enum clock_event_mode mode,
 {
struct ttc_timer_clockevent *ttce = to_ttc_timer_clkevent(evt);
struct ttc_timer *timer = >ttc;
-   u32 ctrl_reg;
 
switch (mode) {
case CLOCK_EVT_MODE_PERIODIC:
@@ -211,18 +212,9 @@ static void ttc_set_mode(enum clock_event_mode mode,
case CLOCK_EVT_MODE_ONESHOT:
case CLOCK_EVT_MODE_UNUSED:
case CLOCK_EVT_MODE_SHUTDOWN:
-   ctrl_reg = __raw_readl(timer->base_addr +
-   TTC_CNT_CNTRL_OFFSET);
-   ctrl_reg |= TTC_CNT_CNTRL_DISABLE_MASK;
-   __raw_writel(ctrl_reg,
-   timer->base_addr + TTC_CNT_CNTRL_OFFSET);
+   __raw_writel(0, timer->base_addr + TTC_IER_OFFSET);
break;
case CLOCK_EVT_MODE_RESUME:
-   ctrl_reg = __raw_readl(timer->base_addr +
-   TTC_CNT_CNTRL_OFFSET);
-   ctrl_reg &= ~TTC_CNT_CNTRL_DISABLE_MASK;
-   __raw_writel(ctrl_reg,
-   timer->base_addr + TTC_CNT_CNTRL_OFFSET);
break;
}

[PATCH v3 5/7] clocksource/cadence_ttc: Use only one counter

2014-02-03 Thread Soren Brinkmann 
Currently the driver uses two of the three counters the TTC provides to
implement a clocksource and a clockevent device. By using the TTC's
match feature we can implement both use cases using a single counter
only.
The old approach is to use timer over-/underflow to generate an
interrupt. Using the match register allows to generate an interrupt on
arbitrary counter values. This way a dedicated clockevent counter can be
avoided.

Signed-off-by: Soren Brinkmann soren.brinkm...@xilinx.com
---
 drivers/clocksource/cadence_ttc_timer.c | 92 +++--
 1 file changed, 31 insertions(+), 61 deletions(-)

diff --git a/drivers/clocksource/cadence_ttc_timer.c 
b/drivers/clocksource/cadence_ttc_timer.c
index 49fbe2847c84..d922e982af95 100644
--- a/drivers/clocksource/cadence_ttc_timer.c
+++ b/drivers/clocksource/cadence_ttc_timer.c
@@ -47,6 +47,7 @@
 #define TTC_CNT_CNTRL_OFFSET   0x0C /* Counter Control Reg, RW */
 #define TTC_COUNT_VAL_OFFSET   0x18 /* Counter Value Reg, RO */
 #define TTC_INTR_VAL_OFFSET0x24 /* Interval Count Reg, RW */
+#define TTC_MATCH1_OFFSET  0x30 /* Match reg, RW */
 #define TTC_ISR_OFFSET 0x54 /* Interrupt Status Reg, RO */
 #define TTC_IER_OFFSET 0x60 /* Interrupt Enable Reg, RW */
 
@@ -64,7 +65,10 @@
 #define PRESCALE   2048/* The exponent must match this */
 #define CLK_CNTRL_PRESCALE ((PRESCALE_EXPONENT - 1)  1)
 #define CLK_CNTRL_PRESCALE_EN  1
-#define CNT_CNTRL_RESET(1  4)
+#define CNT_CNTRL_RESETBIT(4)
+#define CNT_CNTRL_MATCHBIT(3)
+
+#define TTC_INTERRUPT_MATCH1   BIT(1)
 
 #define MAX_F_ERR 50
 
@@ -99,6 +103,7 @@ struct ttc_timer_clocksource {
 struct ttc_timer_clockevent {
struct ttc_timerttc;
struct clock_event_device   ce;
+   unsigned long   interval;
 };
 
 #define to_ttc_timer_clkevent(x) \
@@ -112,25 +117,20 @@ static void __iomem *ttc_sched_clock_val_reg;
  * @timer: Pointer to the timer instance
  * @cycles:Timer interval ticks
  **/
-static void ttc_set_interval(struct ttc_timer *timer,
-   unsigned long cycles)
+static void ttc_set_interval(struct ttc_timer *timer, unsigned long cycles)
 {
-   u32 ctrl_reg;
+   struct ttc_timer_clockevent *ttcce = container_of(timer,
+   struct ttc_timer_clockevent, ttc);
 
-   /* Disable the counter, set the counter value  and re-enable counter */
-   ctrl_reg = __raw_readl(timer-base_addr + TTC_CNT_CNTRL_OFFSET);
-   ctrl_reg |= TTC_CNT_CNTRL_DISABLE_MASK;
-   __raw_writel(ctrl_reg, timer-base_addr + TTC_CNT_CNTRL_OFFSET);
+   /* set interval */
+   u32 reg = __raw_readl(timer-base_addr + TTC_COUNT_VAL_OFFSET);
+   reg += cycles;
+   __raw_writel(reg, timer-base_addr + TTC_MATCH1_OFFSET);
 
-   __raw_writel(cycles, timer-base_addr + TTC_INTR_VAL_OFFSET);
+   /* enable match interrupt */
+   __raw_writel(TTC_INTERRUPT_MATCH1, timer-base_addr + TTC_IER_OFFSET);
 
-   /*
-* Reset the counter (0x10) so that it starts from 0, one-shot
-* mode makes this needed for timing to be right.
-*/
-   ctrl_reg |= CNT_CNTRL_RESET;
-   ctrl_reg = ~TTC_CNT_CNTRL_DISABLE_MASK;
-   __raw_writel(ctrl_reg, timer-base_addr + TTC_CNT_CNTRL_OFFSET);
+   ttcce-interval = cycles;
 }
 
 /**
@@ -148,6 +148,8 @@ static irqreturn_t ttc_clock_event_interrupt(int irq, void 
*dev_id)
 
/* Acknowledge the interrupt and call event handler */
__raw_readl(timer-base_addr + TTC_ISR_OFFSET);
+   if (ttce-ce.mode == CLOCK_EVT_MODE_PERIODIC)
+   ttc_set_interval(timer, ttce-interval);
 
ttce-ce.event_handler(ttce-ce);
 
@@ -201,7 +203,6 @@ static void ttc_set_mode(enum clock_event_mode mode,
 {
struct ttc_timer_clockevent *ttce = to_ttc_timer_clkevent(evt);
struct ttc_timer *timer = ttce-ttc;
-   u32 ctrl_reg;
 
switch (mode) {
case CLOCK_EVT_MODE_PERIODIC:
@@ -211,18 +212,9 @@ static void ttc_set_mode(enum clock_event_mode mode,
case CLOCK_EVT_MODE_ONESHOT:
case CLOCK_EVT_MODE_UNUSED:
case CLOCK_EVT_MODE_SHUTDOWN:
-   ctrl_reg = __raw_readl(timer-base_addr +
-   TTC_CNT_CNTRL_OFFSET);
-   ctrl_reg |= TTC_CNT_CNTRL_DISABLE_MASK;
-   __raw_writel(ctrl_reg,
-   timer-base_addr + TTC_CNT_CNTRL_OFFSET);
+   __raw_writel(0, timer-base_addr + TTC_IER_OFFSET);
break;
case CLOCK_EVT_MODE_RESUME:
-   ctrl_reg = __raw_readl(timer-base_addr +
-   TTC_CNT_CNTRL_OFFSET);
-   ctrl_reg = ~TTC_CNT_CNTRL_DISABLE_MASK;
-   __raw_writel(ctrl_reg,
-   timer-base_addr + TTC_CNT_CNTRL_OFFSET);
break;