[PATCH 0/9] clocksource: dw_apb_timer: support for timer variant used in rk3188 SoCs

[PATCH 0/9] clocksource: dw_apb_timer: support for timer variant used in rk3188 SoCs

[Heiko Stübner]

The Rockchip rk3188 SoCs use a variant of the timer with some slight
modifications. This series implements them as quirks for the dw_apb_timer.

Tested on a rk3188 for the quirk handling and on a rk3066a to check that
nothing broke.

Heiko Stuebner (5):
  clocksource: dw_apb_timer: infrastructure to handle quirks
  clocksource: dw_apb_timer: flexible register addresses
  clocksource: dw_apb_timer: quirk for variants with 64bit counter
  clocksource: dw_apb_timer_of: add quirk handling
  clocksource: dw_apb_timer: special variant for rockchip rk3188 timers

Ulrich Prinz (4):
  clocksource: dw_apb_timer: use the eoi callback to clear pending interrupts
  clocksource: dw_apb_timer: quirk for variants without EOI register
  clocksource: dw_apb_timer: quirk for inverted int mask
  clocksource: dw_apb_timer: quirk for inverted timer mode setting

 .../bindings/arm/rockchip/rk3188-timer.txt         |   20 +++
 arch/x86/kernel/apb_timer.c                        |    4 +-
 drivers/clocksource/dw_apb_timer.c                 |  166 +++++++++++++++-----
 drivers/clocksource/dw_apb_timer_of.c              |   27 +++-
 include/linux/dw_apb_timer.h                       |   34 +++-
 5 files changed, 198 insertions(+), 53 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/rockchip/rk3188-timer.txt

-- 
1.7.10.4

[PATCH 1/9] clocksource: dw_apb_timer: infrastructure to handle quirks

[Heiko Stübner]

There exist variants of the timer IP with some modified properties.

Therefore add infrastructure to handle hardware-quirks in the driver.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 arch/x86/kernel/apb_timer.c           |    4 ++--
 drivers/clocksource/dw_apb_timer.c    |    7 +++++--
 drivers/clocksource/dw_apb_timer_of.c |    4 ++--
 include/linux/dw_apb_timer.h          |    6 ++++--
 4 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/apb_timer.c b/arch/x86/kernel/apb_timer.c
index c9876ef..e7fe0f6 100644
--- a/arch/x86/kernel/apb_timer.c
+++ b/arch/x86/kernel/apb_timer.c
@@ -121,7 +121,7 @@ static inline void apbt_set_mapping(void)
 
 	clocksource_apbt = dw_apb_clocksource_init(APBT_CLOCKSOURCE_RATING,
 		"apbt0", apbt_virt_address + phy_cs_timer_id *
-		APBTMRS_REG_SIZE, apbt_freq);
+		APBTMRS_REG_SIZE, apbt_freq, 0);
 	return;
 
 panic_noapbt:
@@ -159,7 +159,7 @@ static int __init apbt_clockevent_register(void)
 	adev->timer = dw_apb_clockevent_init(smp_processor_id(), "apbt0",
 		mrst_timer_options == MRST_TIMER_LAPIC_APBT ?
 		APBT_CLOCKEVENT_RATING - 100 : APBT_CLOCKEVENT_RATING,
-		adev_virt_addr(adev), 0, apbt_freq);
+		adev_virt_addr(adev), 0, apbt_freq, 0);
 	/* Firmware does EOI handling for us. */
 	adev->timer->eoi = NULL;
 
diff --git a/drivers/clocksource/dw_apb_timer.c b/drivers/clocksource/dw_apb_timer.c
index 8c2a35f..01bdac0 100644
--- a/drivers/clocksource/dw_apb_timer.c
+++ b/drivers/clocksource/dw_apb_timer.c
@@ -213,7 +213,8 @@ static int apbt_next_event(unsigned long delta,
  */
 struct dw_apb_clock_event_device *
 dw_apb_clockevent_init(int cpu, const char *name, unsigned rating,
-		       void __iomem *base, int irq, unsigned long freq)
+		       void __iomem *base, int irq, unsigned long freq,
+		       int quirks)
 {
 	struct dw_apb_clock_event_device *dw_ced =
 		kzalloc(sizeof(*dw_ced), GFP_KERNEL);
@@ -225,6 +226,7 @@ dw_apb_clockevent_init(int cpu, const char *name, unsigned rating,
 	dw_ced->timer.base = base;
 	dw_ced->timer.irq = irq;
 	dw_ced->timer.freq = freq;
+	dw_ced->timer.quirks = quirks;
 
 	clockevents_calc_mult_shift(&dw_ced->ced, freq, APBT_MIN_PERIOD);
 	dw_ced->ced.max_delta_ns = clockevent_delta2ns(0x7fffffff,
@@ -349,7 +351,7 @@ static void apbt_restart_clocksource(struct clocksource *cs)
  */
 struct dw_apb_clocksource *
 dw_apb_clocksource_init(unsigned rating, const char *name, void __iomem *base,
-			unsigned long freq)
+			unsigned long freq, int quirks)
 {
 	struct dw_apb_clocksource *dw_cs = kzalloc(sizeof(*dw_cs), GFP_KERNEL);
 
@@ -358,6 +360,7 @@ dw_apb_clocksource_init(unsigned rating, const char *name, void __iomem *base,
 
 	dw_cs->timer.base = base;
 	dw_cs->timer.freq = freq;
+	dw_cs->timer.quirks = quirks;
 	dw_cs->cs.name = name;
 	dw_cs->cs.rating = rating;
 	dw_cs->cs.read = __apbt_read_clocksource;
diff --git a/drivers/clocksource/dw_apb_timer_of.c b/drivers/clocksource/dw_apb_timer_of.c
index cef5544..b5412af 100644
--- a/drivers/clocksource/dw_apb_timer_of.c
+++ b/drivers/clocksource/dw_apb_timer_of.c
@@ -74,7 +74,7 @@ static void add_clockevent(struct device_node *event_timer)
 	timer_get_base_and_rate(event_timer, &iobase, &rate);
 
 	ced = dw_apb_clockevent_init(0, event_timer->name, 300, iobase, irq,
-				     rate);
+				     rate, 0);
 	if (!ced)
 		panic("Unable to initialise clockevent device");
 
@@ -92,7 +92,7 @@ static void add_clocksource(struct device_node *source_timer)
 
 	timer_get_base_and_rate(source_timer, &iobase, &rate);
 
-	cs = dw_apb_clocksource_init(300, source_timer->name, iobase, rate);
+	cs = dw_apb_clocksource_init(300, source_timer->name, iobase, rate, 0);
 	if (!cs)
 		panic("Unable to initialise clocksource device");
 
diff --git a/include/linux/dw_apb_timer.h b/include/linux/dw_apb_timer.h
index 07261d5..67d09c7 100644
--- a/include/linux/dw_apb_timer.h
+++ b/include/linux/dw_apb_timer.h
@@ -23,6 +23,7 @@ struct dw_apb_timer {
 	void __iomem				*base;
 	unsigned long				freq;
 	int					irq;
+	int					quirks;
 };
 
 struct dw_apb_clock_event_device {
@@ -44,10 +45,11 @@ void dw_apb_clockevent_stop(struct dw_apb_clock_event_device *dw_ced);
 
 struct dw_apb_clock_event_device *
 dw_apb_clockevent_init(int cpu, const char *name, unsigned rating,
-		       void __iomem *base, int irq, unsigned long freq);
+		       void __iomem *base, int irq, unsigned long freq,
+		       int quirks);
 struct dw_apb_clocksource *
 dw_apb_clocksource_init(unsigned rating, const char *name, void __iomem *base,
-			unsigned long freq);
+			unsigned long freq, int quirks);
 void dw_apb_clocksource_register(struct dw_apb_clocksource *dw_cs);
 void dw_apb_clocksource_start(struct dw_apb_clocksource *dw_cs);
 cycle_t dw_apb_clocksource_read(struct dw_apb_clocksource *dw_cs);
-- 
1.7.10.4

[PATCH 2/9] clocksource: dw_apb_timer: flexible register addresses

[Heiko Stübner]

There exists variants of the apb-timer that use slightly different
register positions. To accomodate this, add elements to the timer struct
to hold the actual register offsets.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clocksource/dw_apb_timer.c |   83 ++++++++++++++++++++++--------------
 include/linux/dw_apb_timer.h       |    5 +++
 2 files changed, 56 insertions(+), 32 deletions(-)

diff --git a/drivers/clocksource/dw_apb_timer.c b/drivers/clocksource/dw_apb_timer.c
index 01bdac0..f5e7be8 100644
--- a/drivers/clocksource/dw_apb_timer.c
+++ b/drivers/clocksource/dw_apb_timer.c
@@ -49,6 +49,15 @@ clocksource_to_dw_apb_clocksource(struct clocksource *cs)
 	return container_of(cs, struct dw_apb_clocksource, cs);
 }
 
+static void apbt_init_regs(struct dw_apb_timer *timer, int quirks)
+{
+	timer->reg_load_count = APBTMR_N_LOAD_COUNT;
+	timer->reg_current_value = APBTMR_N_CURRENT_VALUE;
+	timer->reg_control = APBTMR_N_CONTROL;
+	timer->reg_eoi = APBTMR_N_EOI;
+	timer->reg_int_status = APBTMR_N_INT_STATUS;
+}
+
 static unsigned long apbt_readl(struct dw_apb_timer *timer, unsigned long offs)
 {
 	return readl(timer->base + offs);
@@ -62,10 +71,10 @@ static void apbt_writel(struct dw_apb_timer *timer, unsigned long val,
 
 static void apbt_disable_int(struct dw_apb_timer *timer)
 {
-	unsigned long ctrl = apbt_readl(timer, APBTMR_N_CONTROL);
+	unsigned long ctrl = apbt_readl(timer, timer->reg_control);
 
 	ctrl |= APBTMR_CONTROL_INT;
-	apbt_writel(timer, ctrl, APBTMR_N_CONTROL);
+	apbt_writel(timer, ctrl, timer->reg_control);
 }
 
 /**
@@ -81,7 +90,7 @@ void dw_apb_clockevent_pause(struct dw_apb_clock_event_device *dw_ced)
 
 static void apbt_eoi(struct dw_apb_timer *timer)
 {
-	apbt_readl(timer, APBTMR_N_EOI);
+	apbt_readl(timer, timer->reg_eoi);
 }
 
 static irqreturn_t dw_apb_clockevent_irq(int irq, void *data)
@@ -103,11 +112,11 @@ static irqreturn_t dw_apb_clockevent_irq(int irq, void *data)
 
 static void apbt_enable_int(struct dw_apb_timer *timer)
 {
-	unsigned long ctrl = apbt_readl(timer, APBTMR_N_CONTROL);
+	unsigned long ctrl = apbt_readl(timer, timer->reg_control);
 	/* clear pending intr */
-	apbt_readl(timer, APBTMR_N_EOI);
+	apbt_readl(timer, timer->reg_eoi);
 	ctrl &= ~APBTMR_CONTROL_INT;
-	apbt_writel(timer, ctrl, APBTMR_N_CONTROL);
+	apbt_writel(timer, ctrl, timer->reg_control);
 }
 
 static void apbt_set_mode(enum clock_event_mode mode,
@@ -116,31 +125,32 @@ static void apbt_set_mode(enum clock_event_mode mode,
 	unsigned long ctrl;
 	unsigned long period;
 	struct dw_apb_clock_event_device *dw_ced = ced_to_dw_apb_ced(evt);
+	struct dw_apb_timer *timer = &dw_ced->timer;
 
 	pr_debug("%s CPU %d mode=%d\n", __func__, first_cpu(*evt->cpumask),
 		 mode);
 
 	switch (mode) {
 	case CLOCK_EVT_MODE_PERIODIC:
-		period = DIV_ROUND_UP(dw_ced->timer.freq, HZ);
-		ctrl = apbt_readl(&dw_ced->timer, APBTMR_N_CONTROL);
+		period = DIV_ROUND_UP(timer->freq, HZ);
+		ctrl = apbt_readl(timer, timer->reg_control);
 		ctrl |= APBTMR_CONTROL_MODE_PERIODIC;
-		apbt_writel(&dw_ced->timer, ctrl, APBTMR_N_CONTROL);
+		apbt_writel(timer, ctrl, timer->reg_control);
 		/*
 		 * DW APB p. 46, have to disable timer before load counter,
 		 * may cause sync problem.
 		 */
 		ctrl &= ~APBTMR_CONTROL_ENABLE;
-		apbt_writel(&dw_ced->timer, ctrl, APBTMR_N_CONTROL);
+		apbt_writel(timer, ctrl, timer->reg_control);
 		udelay(1);
 		pr_debug("Setting clock period %lu for HZ %d\n", period, HZ);
-		apbt_writel(&dw_ced->timer, period, APBTMR_N_LOAD_COUNT);
+		apbt_writel(timer, period, timer->reg_load_count);
 		ctrl |= APBTMR_CONTROL_ENABLE;
-		apbt_writel(&dw_ced->timer, ctrl, APBTMR_N_CONTROL);
+		apbt_writel(timer, ctrl, timer->reg_control);
 		break;
 
 	case CLOCK_EVT_MODE_ONESHOT:
-		ctrl = apbt_readl(&dw_ced->timer, APBTMR_N_CONTROL);
+		ctrl = apbt_readl(timer, timer->reg_control);
 		/*
 		 * set free running mode, this mode will let timer reload max
 		 * timeout which will give time (3min on 25MHz clock) to rearm
@@ -149,29 +159,29 @@ static void apbt_set_mode(enum clock_event_mode mode,
 		ctrl &= ~APBTMR_CONTROL_ENABLE;
 		ctrl &= ~APBTMR_CONTROL_MODE_PERIODIC;
 
-		apbt_writel(&dw_ced->timer, ctrl, APBTMR_N_CONTROL);
+		apbt_writel(timer, ctrl, timer->reg_control);
 		/* write again to set free running mode */
-		apbt_writel(&dw_ced->timer, ctrl, APBTMR_N_CONTROL);
+		apbt_writel(timer, ctrl, timer->reg_control);
 
 		/*
 		 * DW APB p. 46, load counter with all 1s before starting free
 		 * running mode.
 		 */
-		apbt_writel(&dw_ced->timer, ~0, APBTMR_N_LOAD_COUNT);
+		apbt_writel(timer, ~0, timer->reg_load_count);
 		ctrl &= ~APBTMR_CONTROL_INT;
 		ctrl |= APBTMR_CONTROL_ENABLE;
-		apbt_writel(&dw_ced->timer, ctrl, APBTMR_N_CONTROL);
+		apbt_writel(timer, ctrl, timer->reg_control);
 		break;
 
 	case CLOCK_EVT_MODE_UNUSED:
 	case CLOCK_EVT_MODE_SHUTDOWN:
-		ctrl = apbt_readl(&dw_ced->timer, APBTMR_N_CONTROL);
+		ctrl = apbt_readl(timer, timer->reg_control);
 		ctrl &= ~APBTMR_CONTROL_ENABLE;
-		apbt_writel(&dw_ced->timer, ctrl, APBTMR_N_CONTROL);
+		apbt_writel(timer, ctrl, timer->reg_control);
 		break;
 
 	case CLOCK_EVT_MODE_RESUME:
-		apbt_enable_int(&dw_ced->timer);
+		apbt_enable_int(timer);
 		break;
 	}
 }
@@ -181,15 +191,16 @@ static int apbt_next_event(unsigned long delta,
 {
 	unsigned long ctrl;
 	struct dw_apb_clock_event_device *dw_ced = ced_to_dw_apb_ced(evt);
+	struct dw_apb_timer *timer = &dw_ced->timer;
 
 	/* Disable timer */
-	ctrl = apbt_readl(&dw_ced->timer, APBTMR_N_CONTROL);
+	ctrl = apbt_readl(timer, timer->reg_control);
 	ctrl &= ~APBTMR_CONTROL_ENABLE;
-	apbt_writel(&dw_ced->timer, ctrl, APBTMR_N_CONTROL);
+	apbt_writel(timer, ctrl, timer->reg_control);
 	/* write new count */
-	apbt_writel(&dw_ced->timer, delta, APBTMR_N_LOAD_COUNT);
+	apbt_writel(timer, delta, timer->reg_load_count);
 	ctrl |= APBTMR_CONTROL_ENABLE;
-	apbt_writel(&dw_ced->timer, ctrl, APBTMR_N_CONTROL);
+	apbt_writel(timer, ctrl, timer->reg_control);
 
 	return 0;
 }
@@ -227,6 +238,7 @@ dw_apb_clockevent_init(int cpu, const char *name, unsigned rating,
 	dw_ced->timer.irq = irq;
 	dw_ced->timer.freq = freq;
 	dw_ced->timer.quirks = quirks;
+	apbt_init_regs(&dw_ced->timer, quirks);
 
 	clockevents_calc_mult_shift(&dw_ced->ced, freq, APBT_MIN_PERIOD);
 	dw_ced->ced.max_delta_ns = clockevent_delta2ns(0x7fffffff,
@@ -286,9 +298,11 @@ void dw_apb_clockevent_stop(struct dw_apb_clock_event_device *dw_ced)
  */
 void dw_apb_clockevent_register(struct dw_apb_clock_event_device *dw_ced)
 {
-	apbt_writel(&dw_ced->timer, 0, APBTMR_N_CONTROL);
+	struct dw_apb_timer *timer = &dw_ced->timer;
+
+	apbt_writel(timer, 0, timer->reg_control);
 	clockevents_register_device(&dw_ced->ced);
-	apbt_enable_int(&dw_ced->timer);
+	apbt_enable_int(timer);
 }
 
 /**
@@ -301,19 +315,20 @@ void dw_apb_clockevent_register(struct dw_apb_clock_event_device *dw_ced)
  */
 void dw_apb_clocksource_start(struct dw_apb_clocksource *dw_cs)
 {
+	struct dw_apb_timer *timer = &dw_cs->timer;
 	/*
 	 * start count down from 0xffff_ffff. this is done by toggling the
 	 * enable bit then load initial load count to ~0.
 	 */
-	unsigned long ctrl = apbt_readl(&dw_cs->timer, APBTMR_N_CONTROL);
+	unsigned long ctrl = apbt_readl(timer, timer->reg_control);
 
 	ctrl &= ~APBTMR_CONTROL_ENABLE;
-	apbt_writel(&dw_cs->timer, ctrl, APBTMR_N_CONTROL);
-	apbt_writel(&dw_cs->timer, ~0, APBTMR_N_LOAD_COUNT);
+	apbt_writel(timer, ctrl, timer->reg_control);
+	apbt_writel(timer, ~0, timer->reg_load_count);
 	/* enable, mask interrupt */
 	ctrl &= ~APBTMR_CONTROL_MODE_PERIODIC;
 	ctrl |= (APBTMR_CONTROL_ENABLE | APBTMR_CONTROL_INT);
-	apbt_writel(&dw_cs->timer, ctrl, APBTMR_N_CONTROL);
+	apbt_writel(timer, ctrl, timer->reg_control);
 	/* read it once to get cached counter value initialized */
 	dw_apb_clocksource_read(dw_cs);
 }
@@ -323,8 +338,9 @@ static cycle_t __apbt_read_clocksource(struct clocksource *cs)
 	unsigned long current_count;
 	struct dw_apb_clocksource *dw_cs =
 		clocksource_to_dw_apb_clocksource(cs);
+	struct dw_apb_timer *timer = &dw_cs->timer;
 
-	current_count = apbt_readl(&dw_cs->timer, APBTMR_N_CURRENT_VALUE);
+	current_count = apbt_readl(timer, timer->reg_current_value);
 
 	return (cycle_t)~current_count;
 }
@@ -361,6 +377,8 @@ dw_apb_clocksource_init(unsigned rating, const char *name, void __iomem *base,
 	dw_cs->timer.base = base;
 	dw_cs->timer.freq = freq;
 	dw_cs->timer.quirks = quirks;
+	apbt_init_regs(&dw_cs->timer, quirks);
+
 	dw_cs->cs.name = name;
 	dw_cs->cs.rating = rating;
 	dw_cs->cs.read = __apbt_read_clocksource;
@@ -388,7 +406,8 @@ void dw_apb_clocksource_register(struct dw_apb_clocksource *dw_cs)
  */
 cycle_t dw_apb_clocksource_read(struct dw_apb_clocksource *dw_cs)
 {
-	return (cycle_t)~apbt_readl(&dw_cs->timer, APBTMR_N_CURRENT_VALUE);
+	struct dw_apb_timer *timer = &dw_cs->timer;
+	return (cycle_t)~apbt_readl(timer, timer->reg_current_value);
 }
 
 /**
diff --git a/include/linux/dw_apb_timer.h b/include/linux/dw_apb_timer.h
index 67d09c7..7dc7166 100644
--- a/include/linux/dw_apb_timer.h
+++ b/include/linux/dw_apb_timer.h
@@ -24,6 +24,11 @@ struct dw_apb_timer {
 	unsigned long				freq;
 	int					irq;
 	int					quirks;
+	u8					reg_load_count;
+	u8					reg_current_value;
+	u8					reg_control;
+	u8					reg_eoi;
+	u8					reg_int_status;
 };
 
 struct dw_apb_clock_event_device {
-- 
1.7.10.4

[PATCH 3/9] clocksource: dw_apb_timer: quirk for variants with 64bit counter

[Heiko Stübner]

This adds a quirk for IP variants containing two load_count and value
registers that are used to provide 64bit accuracy on 32bit systems.

The added accuracy is currently not used, the driver is only adapted to
handle the different register layout and make it work on affected devices.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clocksource/dw_apb_timer.c |   27 +++++++++++++++++++++++++++
 include/linux/dw_apb_timer.h       |    6 ++++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/clocksource/dw_apb_timer.c b/drivers/clocksource/dw_apb_timer.c
index f5e7be8..bd45351 100644
--- a/drivers/clocksource/dw_apb_timer.c
+++ b/drivers/clocksource/dw_apb_timer.c
@@ -56,6 +56,17 @@ static void apbt_init_regs(struct dw_apb_timer *timer, int quirks)
 	timer->reg_control = APBTMR_N_CONTROL;
 	timer->reg_eoi = APBTMR_N_EOI;
 	timer->reg_int_status = APBTMR_N_INT_STATUS;
+
+	/*
+	 * On variants with 64bit counters some registers are
+	 * moved further down.
+	 */
+	if (quirks & APBTMR_QUIRK_64BIT_COUNTER) {
+		timer->reg_current_value += 0x4;
+		timer->reg_control += 0x8;
+		timer->reg_eoi += 0x8;
+		timer->reg_int_status += 0x8;
+	}
 }
 
 static unsigned long apbt_readl(struct dw_apb_timer *timer, unsigned long offs)
@@ -145,6 +156,10 @@ static void apbt_set_mode(enum clock_event_mode mode,
 		udelay(1);
 		pr_debug("Setting clock period %lu for HZ %d\n", period, HZ);
 		apbt_writel(timer, period, timer->reg_load_count);
+
+		if (timer->quirks & APBTMR_QUIRK_64BIT_COUNTER)
+			apbt_writel(timer, 0, timer->reg_load_count + 0x4);
+
 		ctrl |= APBTMR_CONTROL_ENABLE;
 		apbt_writel(timer, ctrl, timer->reg_control);
 		break;
@@ -168,6 +183,10 @@ static void apbt_set_mode(enum clock_event_mode mode,
 		 * running mode.
 		 */
 		apbt_writel(timer, ~0, timer->reg_load_count);
+
+		if (timer->quirks & APBTMR_QUIRK_64BIT_COUNTER)
+			apbt_writel(timer, 0, timer->reg_load_count + 0x4);
+
 		ctrl &= ~APBTMR_CONTROL_INT;
 		ctrl |= APBTMR_CONTROL_ENABLE;
 		apbt_writel(timer, ctrl, timer->reg_control);
@@ -199,6 +218,10 @@ static int apbt_next_event(unsigned long delta,
 	apbt_writel(timer, ctrl, timer->reg_control);
 	/* write new count */
 	apbt_writel(timer, delta, timer->reg_load_count);
+
+	if (timer->quirks & APBTMR_QUIRK_64BIT_COUNTER)
+		apbt_writel(timer, 0, timer->reg_load_count + 0x4);
+
 	ctrl |= APBTMR_CONTROL_ENABLE;
 	apbt_writel(timer, ctrl, timer->reg_control);
 
@@ -325,6 +348,10 @@ void dw_apb_clocksource_start(struct dw_apb_clocksource *dw_cs)
 	ctrl &= ~APBTMR_CONTROL_ENABLE;
 	apbt_writel(timer, ctrl, timer->reg_control);
 	apbt_writel(timer, ~0, timer->reg_load_count);
+
+	if (timer->quirks & APBTMR_QUIRK_64BIT_COUNTER)
+		apbt_writel(timer, 0, timer->reg_load_count + 0x4);
+
 	/* enable, mask interrupt */
 	ctrl &= ~APBTMR_CONTROL_MODE_PERIODIC;
 	ctrl |= (APBTMR_CONTROL_ENABLE | APBTMR_CONTROL_INT);
diff --git a/include/linux/dw_apb_timer.h b/include/linux/dw_apb_timer.h
index 7dc7166..80f6686 100644
--- a/include/linux/dw_apb_timer.h
+++ b/include/linux/dw_apb_timer.h
@@ -19,6 +19,12 @@
 
 #define APBTMRS_REG_SIZE       0x14
 
+/* The IP uses two registers for count and values, to provide 64bit accuracy
+ * on 32bit platforms. The additional registers move the following registers
+ * down by 0x8 byte, as both the count and value registers are duplicated.
+ */
+#define APBTMR_QUIRK_64BIT_COUNTER	BIT(0)
+
 struct dw_apb_timer {
 	void __iomem				*base;
 	unsigned long				freq;
-- 
1.7.10.4

Re: [PATCH 3/9] clocksource: dw_apb_timer: quirk for variants with 64bit counter

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4146 bytes --]

On Sat, 6 Jul 2013, Heiko Stübner wrote:

> This adds a quirk for IP variants containing two load_count and value
> registers that are used to provide 64bit accuracy on 32bit systems.
> 
> The added accuracy is currently not used, the driver is only adapted to
> handle the different register layout and make it work on affected devices.
> 
> Signed-off-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
> ---
>  drivers/clocksource/dw_apb_timer.c |   27 +++++++++++++++++++++++++++
>  include/linux/dw_apb_timer.h       |    6 ++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/clocksource/dw_apb_timer.c b/drivers/clocksource/dw_apb_timer.c
> index f5e7be8..bd45351 100644
> --- a/drivers/clocksource/dw_apb_timer.c
> +++ b/drivers/clocksource/dw_apb_timer.c
> @@ -56,6 +56,17 @@ static void apbt_init_regs(struct dw_apb_timer *timer, int quirks)
>  	timer->reg_control = APBTMR_N_CONTROL;
>  	timer->reg_eoi = APBTMR_N_EOI;
>  	timer->reg_int_status = APBTMR_N_INT_STATUS;
> +
> +	/*
> +	 * On variants with 64bit counters some registers are
> +	 * moved further down.
> +	 */
> +	if (quirks & APBTMR_QUIRK_64BIT_COUNTER) {
> +		timer->reg_current_value += 0x4;
> +		timer->reg_control += 0x8;
> +		timer->reg_eoi += 0x8;
> +		timer->reg_int_status += 0x8;
> +	}
>  }

Oh, no. this is not how we handle these things.

1) We want proper constants for this 64bit IP block 

2) This is not a quirk, it's a property of that particular IP block

You already made the register offsets a part of the timer structure,
so why don't you supply a proper structure filled with that values to
the init function?

That's what we do all over the place. Either we instantiate those
structs at compile time or runtime fed by device tree or any other
configuration mechanism.
  
>  static unsigned long apbt_readl(struct dw_apb_timer *timer, unsigned long offs)
> @@ -145,6 +156,10 @@ static void apbt_set_mode(enum clock_event_mode mode,
>  		udelay(1);
>  		pr_debug("Setting clock period %lu for HZ %d\n", period, HZ);
>  		apbt_writel(timer, period, timer->reg_load_count);
> +
> +		if (timer->quirks & APBTMR_QUIRK_64BIT_COUNTER)
> +			apbt_writel(timer, 0, timer->reg_load_count + 0x4);
> +

No. We are not adding such conditional constructs when we can deal
with them just by providing a proper set of function pointers. And
definitely not with hardcoded magic 0x4 constants involved.

     timer->load_count(timer, value);

Provide a 32 bit and a 64 bit version of that function and be done
with it.

>  		ctrl |= APBTMR_CONTROL_ENABLE;
>  		apbt_writel(timer, ctrl, timer->reg_control);
>  		break;
> @@ -168,6 +183,10 @@ static void apbt_set_mode(enum clock_event_mode mode,
>  		 * running mode.
>  		 */
>  		apbt_writel(timer, ~0, timer->reg_load_count);
> +
> +		if (timer->quirks & APBTMR_QUIRK_64BIT_COUNTER)
> +			apbt_writel(timer, 0, timer->reg_load_count + 0x4);
> +

Makes this copy go away.

>  		ctrl &= ~APBTMR_CONTROL_INT;
>  		ctrl |= APBTMR_CONTROL_ENABLE;
>  		apbt_writel(timer, ctrl, timer->reg_control);
> @@ -199,6 +218,10 @@ static int apbt_next_event(unsigned long delta,
>  	apbt_writel(timer, ctrl, timer->reg_control);
>  	/* write new count */
>  	apbt_writel(timer, delta, timer->reg_load_count);
> +
> +	if (timer->quirks & APBTMR_QUIRK_64BIT_COUNTER)
> +		apbt_writel(timer, 0, timer->reg_load_count + 0x4);
> +

And this one as well.

>  	ctrl |= APBTMR_CONTROL_ENABLE;
>  	apbt_writel(timer, ctrl, timer->reg_control);
>  
> @@ -325,6 +348,10 @@ void dw_apb_clocksource_start(struct dw_apb_clocksource *dw_cs)
>  	ctrl &= ~APBTMR_CONTROL_ENABLE;
>  	apbt_writel(timer, ctrl, timer->reg_control);
>  	apbt_writel(timer, ~0, timer->reg_load_count);
> +
> +	if (timer->quirks & APBTMR_QUIRK_64BIT_COUNTER)
> +		apbt_writel(timer, 0, timer->reg_load_count + 0x4);
> +

Copy and paste is a conveniant thing, right? It just should have a pop
up window assigned which asks at the second instance of copying the
same thing whether you really thought about it.

Thanks,

	tglx

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 3/9] clocksource: dw_apb_timer: quirk for variants with 64bit counter

[Ulrich Prinz]

Hi Thomas,

sorry but it is my first release to the kernel and I was trained to take
as less as possible influence on existing code and reuse as much as
possible existing code.

Sometimes just two rules are to simple...

Am 06.07.2013 01:45, schrieb Thomas Gleixner:
> On Sat, 6 Jul 2013, Heiko Stübner wrote:
> 
>> This adds a quirk for IP variants containing two load_count and value
>> registers that are used to provide 64bit accuracy on 32bit systems.
>>
>> The added accuracy is currently not used, the driver is only adapted to
>> handle the different register layout and make it work on affected devices.
>>
>> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>> ---
>>  drivers/clocksource/dw_apb_timer.c |   27 +++++++++++++++++++++++++++
>>  include/linux/dw_apb_timer.h       |    6 ++++++
>>  2 files changed, 33 insertions(+)
>>
>> diff --git a/drivers/clocksource/dw_apb_timer.c b/drivers/clocksource/dw_apb_timer.c
>> index f5e7be8..bd45351 100644
>> --- a/drivers/clocksource/dw_apb_timer.c
>> +++ b/drivers/clocksource/dw_apb_timer.c
>> @@ -56,6 +56,17 @@ static void apbt_init_regs(struct dw_apb_timer *timer, int quirks)
>>  	timer->reg_control = APBTMR_N_CONTROL;
>>  	timer->reg_eoi = APBTMR_N_EOI;
>>  	timer->reg_int_status = APBTMR_N_INT_STATUS;
>> +
>> +	/*
>> +	 * On variants with 64bit counters some registers are
>> +	 * moved further down.
>> +	 */
>> +	if (quirks & APBTMR_QUIRK_64BIT_COUNTER) {
>> +		timer->reg_current_value += 0x4;
>> +		timer->reg_control += 0x8;
>> +		timer->reg_eoi += 0x8;
>> +		timer->reg_int_status += 0x8;
>> +	}
>>  }
> 
> Oh, no. this is not how we handle these things.
> 
> 1) We want proper constants for this 64bit IP block 

ACK
> 
> 2) This is not a quirk, it's a property of that particular IP block

Ok, noted and will be reworked.
> 
> You already made the register offsets a part of the timer structure,
> so why don't you supply a proper structure filled with that values to
> the init function?
> 
> That's what we do all over the place. Either we instantiate those
> structs at compile time or runtime fed by device tree or any other
> configuration mechanism.
>   
Lesson learned, will be reworked. But taking the other comments and
remarks into account this leads to an integration into a single new
patch inheriting this new driver path.

>>  static unsigned long apbt_readl(struct dw_apb_timer *timer, unsigned long offs)
>> @@ -145,6 +156,10 @@ static void apbt_set_mode(enum clock_event_mode mode,
>>  		udelay(1);
>>  		pr_debug("Setting clock period %lu for HZ %d\n", period, HZ);
>>  		apbt_writel(timer, period, timer->reg_load_count);
>> +
>> +		if (timer->quirks & APBTMR_QUIRK_64BIT_COUNTER)
>> +			apbt_writel(timer, 0, timer->reg_load_count + 0x4);
>> +
> 
> No. We are not adding such conditional constructs when we can deal
> with them just by providing a proper set of function pointers. And
> definitely not with hardcoded magic 0x4 constants involved.
> 
>      timer->load_count(timer, value);
> 
> Provide a 32 bit and a 64 bit version of that function and be done
> with it.

For the first version, 64bit will not be supported cause of lack of
information and testing. But is it an option to control the load of
functions by dtsi according this way:

dw-apb-timer-osc: will load standard function to load timer
rk3188-dw-apb-timer-osc: will load 32bit variant of 64bit registers
rk3188-dw-apb-timer64-osc: will load 64bit variant of 64bit registers

> 
> 
> Copy and paste is a conveniant thing, right? It just should have a pop
> up window assigned which asks at the second instance of copying the
> same thing whether you really thought about it.

We thought about and tested a lot. But the problem is that if you find a
bug in one copy of it, you (or anyone else inspecting the code) need to
find all the other copies. In such manor a c'n'p solution is always bad.

Regards,
	Ulrich

[PATCH 4/9] clocksource: dw_apb_timer: use the eoi callback to clear pending interrupts

[Heiko Stübner]

Some timer variants have different mechanisms to clear a pending timer
interrupt. Therefore don't hardcode the reading of the eoi register to
clear them, but instead use the already existing eoi callback for this.

Signed-off-by: Ulrich Prinz <ulrich.prinz@googlemail.com>
---
 drivers/clocksource/dw_apb_timer.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/clocksource/dw_apb_timer.c b/drivers/clocksource/dw_apb_timer.c
index bd45351..5f80a30 100644
--- a/drivers/clocksource/dw_apb_timer.c
+++ b/drivers/clocksource/dw_apb_timer.c
@@ -121,11 +121,14 @@ static irqreturn_t dw_apb_clockevent_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static void apbt_enable_int(struct dw_apb_timer *timer)
+static void apbt_enable_int(struct dw_apb_clock_event_device *dw_ced)
 {
+	struct dw_apb_timer *timer = &dw_ced->timer;
 	unsigned long ctrl = apbt_readl(timer, timer->reg_control);
+
 	/* clear pending intr */
-	apbt_readl(timer, timer->reg_eoi);
+	if (dw_ced->eoi)
+		dw_ced->eoi(timer);
 	ctrl &= ~APBTMR_CONTROL_INT;
 	apbt_writel(timer, ctrl, timer->reg_control);
 }
@@ -200,7 +203,7 @@ static void apbt_set_mode(enum clock_event_mode mode,
 		break;
 
 	case CLOCK_EVT_MODE_RESUME:
-		apbt_enable_int(timer);
+		apbt_enable_int(dw_ced);
 		break;
 	}
 }
@@ -325,7 +328,7 @@ void dw_apb_clockevent_register(struct dw_apb_clock_event_device *dw_ced)
 
 	apbt_writel(timer, 0, timer->reg_control);
 	clockevents_register_device(&dw_ced->ced);
-	apbt_enable_int(timer);
+	apbt_enable_int(dw_ced);
 }
 
 /**
-- 
1.7.10.4

Re: [PATCH 4/9] clocksource: dw_apb_timer: use the eoi callback to clear pending interrupts

[Heiko Stübner]

this patch should have had a

From: Ulrich Prinz <ulrich.prinz@googlemail.com>

sorry for the mistake

Am Samstag, 6. Juli 2013, 00:53:36 schrieb Heiko Stübner:
> Some timer variants have different mechanisms to clear a pending timer
> interrupt. Therefore don't hardcode the reading of the eoi register to
> clear them, but instead use the already existing eoi callback for this.
> 
> Signed-off-by: Ulrich Prinz <ulrich.prinz@googlemail.com>
> ---
>  drivers/clocksource/dw_apb_timer.c |   11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clocksource/dw_apb_timer.c
> b/drivers/clocksource/dw_apb_timer.c index bd45351..5f80a30 100644
> --- a/drivers/clocksource/dw_apb_timer.c
> +++ b/drivers/clocksource/dw_apb_timer.c
> @@ -121,11 +121,14 @@ static irqreturn_t dw_apb_clockevent_irq(int irq,
> void *data) return IRQ_HANDLED;
>  }
> 
> -static void apbt_enable_int(struct dw_apb_timer *timer)
> +static void apbt_enable_int(struct dw_apb_clock_event_device *dw_ced)
>  {
> +	struct dw_apb_timer *timer = &dw_ced->timer;
>  	unsigned long ctrl = apbt_readl(timer, timer->reg_control);
> +
>  	/* clear pending intr */
> -	apbt_readl(timer, timer->reg_eoi);
> +	if (dw_ced->eoi)
> +		dw_ced->eoi(timer);
>  	ctrl &= ~APBTMR_CONTROL_INT;
>  	apbt_writel(timer, ctrl, timer->reg_control);
>  }
> @@ -200,7 +203,7 @@ static void apbt_set_mode(enum clock_event_mode mode,
>  		break;
> 
>  	case CLOCK_EVT_MODE_RESUME:
> -		apbt_enable_int(timer);
> +		apbt_enable_int(dw_ced);
>  		break;
>  	}
>  }
> @@ -325,7 +328,7 @@ void dw_apb_clockevent_register(struct
> dw_apb_clock_event_device *dw_ced)
> 
>  	apbt_writel(timer, 0, timer->reg_control);
>  	clockevents_register_device(&dw_ced->ced);
> -	apbt_enable_int(timer);
> +	apbt_enable_int(dw_ced);
>  }
> 
>  /**

[PATCH 5/9] clocksource: dw_apb_timer: quirk for variants without EOI register

[Heiko Stübner]

Some variants of the dw_apb_timer don't have an eoi register but instead expect a
one to be written to the int_status register at eoi time.

Signed-off-by: Ulrich Prinz <ulrich.prinz@googlemail.com>
---
 drivers/clocksource/dw_apb_timer.c |   10 +++++++++-
 include/linux/dw_apb_timer.h       |    5 +++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/clocksource/dw_apb_timer.c b/drivers/clocksource/dw_apb_timer.c
index 5f80a30..23cd7c6 100644
--- a/drivers/clocksource/dw_apb_timer.c
+++ b/drivers/clocksource/dw_apb_timer.c
@@ -104,6 +104,11 @@ static void apbt_eoi(struct dw_apb_timer *timer)
 	apbt_readl(timer, timer->reg_eoi);
 }
 
+static void apbt_eoi_int_status(struct dw_apb_timer *timer)
+{
+	apbt_writel(timer, 1, timer->reg_int_status);
+}
+
 static irqreturn_t dw_apb_clockevent_irq(int irq, void *data)
 {
 	struct clock_event_device *evt = data;
@@ -286,7 +291,10 @@ dw_apb_clockevent_init(int cpu, const char *name, unsigned rating,
 					  IRQF_NOBALANCING |
 					  IRQF_DISABLED;
 
-	dw_ced->eoi = apbt_eoi;
+	if (quirks & APBTMR_QUIRK_NO_EOI)
+		dw_ced->eoi = apbt_eoi_int_status;
+	else
+		dw_ced->eoi = apbt_eoi;
 	err = setup_irq(irq, &dw_ced->irqaction);
 	if (err) {
 		pr_err("failed to request timer irq\n");
diff --git a/include/linux/dw_apb_timer.h b/include/linux/dw_apb_timer.h
index 80f6686..fbe4c6b 100644
--- a/include/linux/dw_apb_timer.h
+++ b/include/linux/dw_apb_timer.h
@@ -25,6 +25,11 @@
  */
 #define APBTMR_QUIRK_64BIT_COUNTER	BIT(0)
 
+/* The IP does not provide a end-of-interrupt register to clear pending
+ * interrupts, but requires to write a 1 to the interrupt-status register.
+ */
+#define APBTMR_QUIRK_NO_EOI		BIT(1)
+
 struct dw_apb_timer {
 	void __iomem				*base;
 	unsigned long				freq;
-- 
1.7.10.4

Re: [PATCH 5/9] clocksource: dw_apb_timer: quirk for variants without EOI register

[Heiko Stübner]

this patch should have had a

From: Ulrich Prinz <ulrich.prinz@googlemail.com>

sorry for the mistake

Am Samstag, 6. Juli 2013, 00:54:07 schrieb Heiko Stübner:
> Some variants of the dw_apb_timer don't have an eoi register but instead
> expect a one to be written to the int_status register at eoi time.
> 
> Signed-off-by: Ulrich Prinz <ulrich.prinz@googlemail.com>
> ---
>  drivers/clocksource/dw_apb_timer.c |   10 +++++++++-
>  include/linux/dw_apb_timer.h       |    5 +++++
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clocksource/dw_apb_timer.c
> b/drivers/clocksource/dw_apb_timer.c index 5f80a30..23cd7c6 100644
> --- a/drivers/clocksource/dw_apb_timer.c
> +++ b/drivers/clocksource/dw_apb_timer.c
> @@ -104,6 +104,11 @@ static void apbt_eoi(struct dw_apb_timer *timer)
>  	apbt_readl(timer, timer->reg_eoi);
>  }
> 
> +static void apbt_eoi_int_status(struct dw_apb_timer *timer)
> +{
> +	apbt_writel(timer, 1, timer->reg_int_status);
> +}
> +
>  static irqreturn_t dw_apb_clockevent_irq(int irq, void *data)
>  {
>  	struct clock_event_device *evt = data;
> @@ -286,7 +291,10 @@ dw_apb_clockevent_init(int cpu, const char *name,
> unsigned rating, IRQF_NOBALANCING |
>  					  IRQF_DISABLED;
> 
> -	dw_ced->eoi = apbt_eoi;
> +	if (quirks & APBTMR_QUIRK_NO_EOI)
> +		dw_ced->eoi = apbt_eoi_int_status;
> +	else
> +		dw_ced->eoi = apbt_eoi;
>  	err = setup_irq(irq, &dw_ced->irqaction);
>  	if (err) {
>  		pr_err("failed to request timer irq\n");
> diff --git a/include/linux/dw_apb_timer.h b/include/linux/dw_apb_timer.h
> index 80f6686..fbe4c6b 100644
> --- a/include/linux/dw_apb_timer.h
> +++ b/include/linux/dw_apb_timer.h
> @@ -25,6 +25,11 @@
>   */
>  #define APBTMR_QUIRK_64BIT_COUNTER	BIT(0)
> 
> +/* The IP does not provide a end-of-interrupt register to clear pending
> + * interrupts, but requires to write a 1 to the interrupt-status register.
> + */
> +#define APBTMR_QUIRK_NO_EOI		BIT(1)
> +
>  struct dw_apb_timer {
>  	void __iomem				*base;
>  	unsigned long				freq;

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 5/9] clocksource: dw_apb_timer: quirk for variants without EOI register

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 323 bytes --]

On Sat, 6 Jul 2013, Heiko Stübner wrote:
> -	dw_ced->eoi = apbt_eoi;
> +	if (quirks & APBTMR_QUIRK_NO_EOI)
> +		dw_ced->eoi = apbt_eoi_int_status;
> +	else
> +		dw_ced->eoi = apbt_eoi;

No again. This has nothing to do with quirks. We use quirks for
workarounds and not for refactoring of code.

Thanks,

	tglx

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

[PATCH 6/9] clocksource: dw_apb_timer: quirk for inverted int mask

[Heiko Stübner]

Some timer variants use an inverted setting to mask the timer interrupt.
Therefore add a quirk to handle these variants.

Signed-off-by: Ulrich Prinz <ulrich.prinz@googlemail.com>
---
 drivers/clocksource/dw_apb_timer.c |   23 ++++++++++++++++++-----
 include/linux/dw_apb_timer.h       |    6 ++++++
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/clocksource/dw_apb_timer.c b/drivers/clocksource/dw_apb_timer.c
index 23cd7c6..7705d13 100644
--- a/drivers/clocksource/dw_apb_timer.c
+++ b/drivers/clocksource/dw_apb_timer.c
@@ -84,7 +84,10 @@ static void apbt_disable_int(struct dw_apb_timer *timer)
 {
 	unsigned long ctrl = apbt_readl(timer, timer->reg_control);
 
-	ctrl |= APBTMR_CONTROL_INT;
+	if (timer->quirks & APBTMR_QUIRK_INVERSE_INTMASK)
+		ctrl &= ~APBTMR_CONTROL_INT;
+	else
+		ctrl |= APBTMR_CONTROL_INT;
 	apbt_writel(timer, ctrl, timer->reg_control);
 }
 
@@ -134,7 +137,10 @@ static void apbt_enable_int(struct dw_apb_clock_event_device *dw_ced)
 	/* clear pending intr */
 	if (dw_ced->eoi)
 		dw_ced->eoi(timer);
-	ctrl &= ~APBTMR_CONTROL_INT;
+	if (timer->quirks & APBTMR_QUIRK_INVERSE_INTMASK)
+		ctrl |= APBTMR_CONTROL_INT;
+	else
+		ctrl &= ~APBTMR_CONTROL_INT;
 	apbt_writel(timer, ctrl, timer->reg_control);
 }
 
@@ -195,7 +201,10 @@ static void apbt_set_mode(enum clock_event_mode mode,
 		if (timer->quirks & APBTMR_QUIRK_64BIT_COUNTER)
 			apbt_writel(timer, 0, timer->reg_load_count + 0x4);
 
-		ctrl &= ~APBTMR_CONTROL_INT;
+		if (timer->quirks & APBTMR_QUIRK_INVERSE_INTMASK)
+			ctrl |= APBTMR_CONTROL_INT;
+		else
+			ctrl &= ~APBTMR_CONTROL_INT;
 		ctrl |= APBTMR_CONTROL_ENABLE;
 		apbt_writel(timer, ctrl, timer->reg_control);
 		break;
@@ -363,9 +372,13 @@ void dw_apb_clocksource_start(struct dw_apb_clocksource *dw_cs)
 	if (timer->quirks & APBTMR_QUIRK_64BIT_COUNTER)
 		apbt_writel(timer, 0, timer->reg_load_count + 0x4);
 
-	/* enable, mask interrupt */
+	/* set periodic, mask interrupt, enable timer */
 	ctrl &= ~APBTMR_CONTROL_MODE_PERIODIC;
-	ctrl |= (APBTMR_CONTROL_ENABLE | APBTMR_CONTROL_INT);
+	if (timer->quirks & APBTMR_QUIRK_INVERSE_INTMASK)
+		ctrl &= ~APBTMR_CONTROL_INT;
+	else
+		ctrl |= APBTMR_CONTROL_INT;
+	ctrl |= APBTMR_CONTROL_ENABLE;
 	apbt_writel(timer, ctrl, timer->reg_control);
 	/* read it once to get cached counter value initialized */
 	dw_apb_clocksource_read(dw_cs);
diff --git a/include/linux/dw_apb_timer.h b/include/linux/dw_apb_timer.h
index fbe4c6b..7d36d91 100644
--- a/include/linux/dw_apb_timer.h
+++ b/include/linux/dw_apb_timer.h
@@ -30,6 +30,12 @@
  */
 #define APBTMR_QUIRK_NO_EOI		BIT(1)
 
+/* The IP uses an inverted interrupt-mask bit.
+ * Instead of activating interrupts clearing a maks bit, it needs an enable
+ * bit to be set 1.
+ */
+#define APBTMR_QUIRK_INVERSE_INTMASK	BIT(2)
+
 struct dw_apb_timer {
 	void __iomem				*base;
 	unsigned long				freq;
-- 
1.7.10.4

Re: [PATCH 6/9] clocksource: dw_apb_timer: quirk for inverted int mask

[Heiko Stübner]

this patch should have had a

From: Ulrich Prinz <ulrich.prinz@googlemail.com>

sorry for the mistake

Am Samstag, 6. Juli 2013, 00:54:35 schrieb Heiko Stübner:
> Some timer variants use an inverted setting to mask the timer interrupt.
> Therefore add a quirk to handle these variants.
> 
> Signed-off-by: Ulrich Prinz <ulrich.prinz@googlemail.com>
> ---
>  drivers/clocksource/dw_apb_timer.c |   23 ++++++++++++++++++-----
>  include/linux/dw_apb_timer.h       |    6 ++++++
>  2 files changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clocksource/dw_apb_timer.c
> b/drivers/clocksource/dw_apb_timer.c index 23cd7c6..7705d13 100644
> --- a/drivers/clocksource/dw_apb_timer.c
> +++ b/drivers/clocksource/dw_apb_timer.c
> @@ -84,7 +84,10 @@ static void apbt_disable_int(struct dw_apb_timer *timer)
>  {
>  	unsigned long ctrl = apbt_readl(timer, timer->reg_control);
> 
> -	ctrl |= APBTMR_CONTROL_INT;
> +	if (timer->quirks & APBTMR_QUIRK_INVERSE_INTMASK)
> +		ctrl &= ~APBTMR_CONTROL_INT;
> +	else
> +		ctrl |= APBTMR_CONTROL_INT;
>  	apbt_writel(timer, ctrl, timer->reg_control);
>  }
> 
> @@ -134,7 +137,10 @@ static void apbt_enable_int(struct
> dw_apb_clock_event_device *dw_ced) /* clear pending intr */
>  	if (dw_ced->eoi)
>  		dw_ced->eoi(timer);
> -	ctrl &= ~APBTMR_CONTROL_INT;
> +	if (timer->quirks & APBTMR_QUIRK_INVERSE_INTMASK)
> +		ctrl |= APBTMR_CONTROL_INT;
> +	else
> +		ctrl &= ~APBTMR_CONTROL_INT;
>  	apbt_writel(timer, ctrl, timer->reg_control);
>  }
> 
> @@ -195,7 +201,10 @@ static void apbt_set_mode(enum clock_event_mode mode,
>  		if (timer->quirks & APBTMR_QUIRK_64BIT_COUNTER)
>  			apbt_writel(timer, 0, timer->reg_load_count + 0x4);
> 
> -		ctrl &= ~APBTMR_CONTROL_INT;
> +		if (timer->quirks & APBTMR_QUIRK_INVERSE_INTMASK)
> +			ctrl |= APBTMR_CONTROL_INT;
> +		else
> +			ctrl &= ~APBTMR_CONTROL_INT;
>  		ctrl |= APBTMR_CONTROL_ENABLE;
>  		apbt_writel(timer, ctrl, timer->reg_control);
>  		break;
> @@ -363,9 +372,13 @@ void dw_apb_clocksource_start(struct
> dw_apb_clocksource *dw_cs) if (timer->quirks & APBTMR_QUIRK_64BIT_COUNTER)
>  		apbt_writel(timer, 0, timer->reg_load_count + 0x4);
> 
> -	/* enable, mask interrupt */
> +	/* set periodic, mask interrupt, enable timer */
>  	ctrl &= ~APBTMR_CONTROL_MODE_PERIODIC;
> -	ctrl |= (APBTMR_CONTROL_ENABLE | APBTMR_CONTROL_INT);
> +	if (timer->quirks & APBTMR_QUIRK_INVERSE_INTMASK)
> +		ctrl &= ~APBTMR_CONTROL_INT;
> +	else
> +		ctrl |= APBTMR_CONTROL_INT;
> +	ctrl |= APBTMR_CONTROL_ENABLE;
>  	apbt_writel(timer, ctrl, timer->reg_control);
>  	/* read it once to get cached counter value initialized */
>  	dw_apb_clocksource_read(dw_cs);
> diff --git a/include/linux/dw_apb_timer.h b/include/linux/dw_apb_timer.h
> index fbe4c6b..7d36d91 100644
> --- a/include/linux/dw_apb_timer.h
> +++ b/include/linux/dw_apb_timer.h
> @@ -30,6 +30,12 @@
>   */
>  #define APBTMR_QUIRK_NO_EOI		BIT(1)
> 
> +/* The IP uses an inverted interrupt-mask bit.
> + * Instead of activating interrupts clearing a maks bit, it needs an
> enable + * bit to be set 1.
> + */
> +#define APBTMR_QUIRK_INVERSE_INTMASK	BIT(2)
> +
>  struct dw_apb_timer {
>  	void __iomem				*base;
>  	unsigned long				freq;

Re: [PATCH 6/9] clocksource: dw_apb_timer: quirk for inverted int mask

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 254 bytes --]

On Sat, 6 Jul 2013, Heiko Stübner wrote:

> Some timer variants use an inverted setting to mask the timer interrupt.
> Therefore add a quirk to handle these variants.

And by that add even more pointless conditionals into critical code
pathes.


[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

[PATCH 7/9] clocksource: dw_apb_timer: quirk for inverted timer mode setting

[Heiko Stübner]

From: Ulrich Prinz <ulrich.prinz-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>

Some variants of SOCs using dw_apb_timer have inverted logic for the
bit that sets one-shot / periodic mode or free running timer. This
commit adds the new APBTMR_QUIRK_INVERSE_PERIODIC.

Signed-off-by: Ulrich Prinz <ulrich.prinz-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
---
 drivers/clocksource/dw_apb_timer.c |   11 +++++++++--
 include/linux/dw_apb_timer.h       |    6 ++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/clocksource/dw_apb_timer.c 
b/drivers/clocksource/dw_apb_timer.c
index 7705d13..a2e8306 100644
--- a/drivers/clocksource/dw_apb_timer.c
+++ b/drivers/clocksource/dw_apb_timer.c
@@ -159,7 +159,11 @@ static void apbt_set_mode(enum clock_event_mode mode,
 	case CLOCK_EVT_MODE_PERIODIC:
 		period = DIV_ROUND_UP(timer->freq, HZ);
 		ctrl = apbt_readl(timer, timer->reg_control);
-		ctrl |= APBTMR_CONTROL_MODE_PERIODIC;
+
+		if (timer->quirks & APBTMR_QUIRK_INVERSE_PERIODIC)
+			ctrl &= ~APBTMR_CONTROL_MODE_PERIODIC;
+		else
+			ctrl |= APBTMR_CONTROL_MODE_PERIODIC;
 		apbt_writel(timer, ctrl, timer->reg_control);
 		/*
 		 * DW APB p. 46, have to disable timer before load counter,
@@ -186,7 +190,10 @@ static void apbt_set_mode(enum clock_event_mode mode,
 		 * the next event, therefore emulate the one-shot mode.
 		 */
 		ctrl &= ~APBTMR_CONTROL_ENABLE;
-		ctrl &= ~APBTMR_CONTROL_MODE_PERIODIC;
+		if (timer->quirks & APBTMR_QUIRK_INVERSE_PERIODIC)
+			ctrl |= APBTMR_CONTROL_MODE_PERIODIC;
+		else
+			ctrl &= ~APBTMR_CONTROL_MODE_PERIODIC;
 
 		apbt_writel(timer, ctrl, timer->reg_control);
 		/* write again to set free running mode */
diff --git a/include/linux/dw_apb_timer.h b/include/linux/dw_apb_timer.h
index 7d36d91..5d9210cc 100644
--- a/include/linux/dw_apb_timer.h
+++ b/include/linux/dw_apb_timer.h
@@ -36,6 +36,12 @@
  */
 #define APBTMR_QUIRK_INVERSE_INTMASK	BIT(2)
 
+/* The IP uses inverted logic for the bit setting periodic mode.
+ * Periodic means it times out after the period is over and is set to
+ * 1 in the original IP. This IP uses 1 for free running mode.
+ */
+#define APBTMR_QUIRK_INVERSE_PERIODIC	BIT(3)
+
 struct dw_apb_timer {
 	void __iomem				*base;
 	unsigned long				freq;
-- 
1.7.10.4

[PATCH 8/9] clocksource: dw_apb_timer_of: add quirk handling

[Heiko Stübner]

timer_get_base_and_rate now also can extract informations about present
hardware-quirks from the devicetree node and transmit it to the
clocksource / clockevent init function.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clocksource/dw_apb_timer_of.c |   21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/clocksource/dw_apb_timer_of.c b/drivers/clocksource/dw_apb_timer_of.c
index b5412af..4bcc1c1 100644
--- a/drivers/clocksource/dw_apb_timer_of.c
+++ b/drivers/clocksource/dw_apb_timer_of.c
@@ -26,7 +26,7 @@
 #include <asm/sched_clock.h>
 
 static void timer_get_base_and_rate(struct device_node *np,
-				    void __iomem **base, u32 *rate)
+				    void __iomem **base, u32 *rate, int *quirks)
 {
 	struct clk *timer_clk;
 	struct clk *pclk;
@@ -36,6 +36,8 @@ static void timer_get_base_and_rate(struct device_node *np,
 	if (!*base)
 		panic("Unable to map regs for %s", np->name);
 
+	*quirks = 0;
+
 	/*
 	 * Not all implementations use a periphal clock, so don't panic
 	 * if it's not present
@@ -66,15 +68,16 @@ static void add_clockevent(struct device_node *event_timer)
 	void __iomem *iobase;
 	struct dw_apb_clock_event_device *ced;
 	u32 irq, rate;
+	int quirks;
 
 	irq = irq_of_parse_and_map(event_timer, 0);
 	if (irq == NO_IRQ)
 		panic("No IRQ for clock event timer");
 
-	timer_get_base_and_rate(event_timer, &iobase, &rate);
+	timer_get_base_and_rate(event_timer, &iobase, &rate, &quirks);
 
 	ced = dw_apb_clockevent_init(0, event_timer->name, 300, iobase, irq,
-				     rate, 0);
+				     rate, quirks);
 	if (!ced)
 		panic("Unable to initialise clockevent device");
 
@@ -89,10 +92,12 @@ static void add_clocksource(struct device_node *source_timer)
 	void __iomem *iobase;
 	struct dw_apb_clocksource *cs;
 	u32 rate;
+	int quirks;
 
-	timer_get_base_and_rate(source_timer, &iobase, &rate);
+	timer_get_base_and_rate(source_timer, &iobase, &rate, &quirks);
 
-	cs = dw_apb_clocksource_init(300, source_timer->name, iobase, rate, 0);
+	cs = dw_apb_clocksource_init(300, source_timer->name, iobase, rate,
+				     quirks);
 	if (!cs)
 		panic("Unable to initialise clocksource device");
 
@@ -106,6 +111,9 @@ static void add_clocksource(struct device_node *source_timer)
 	 */
 	sched_io_base = iobase + 0x04;
 	sched_rate = rate;
+
+	if (quirks & APBTMR_QUIRK_64BIT_COUNTER)
+		sched_io_base += 0x04;
 }
 
 static u32 read_sched_clock(void)
@@ -122,11 +130,12 @@ static const struct of_device_id sptimer_ids[] __initconst = {
 static void init_sched_clock(void)
 {
 	struct device_node *sched_timer;
+	int quirks;
 
 	sched_timer = of_find_matching_node(NULL, sptimer_ids);
 	if (sched_timer) {
 		timer_get_base_and_rate(sched_timer, &sched_io_base,
-					&sched_rate);
+					&sched_rate, &quirks);
 		of_node_put(sched_timer);
 	}
 
-- 
1.7.10.4

[PATCH 9/9] clocksource: dw_apb_timer: special variant for rockchip rk3188 timers

[Heiko Stübner]

The rk3188 uses a variant of the timer containing two registers for load_count
and current_values.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 .../bindings/arm/rockchip/rk3188-timer.txt         |   20 ++++++++++++++++++++
 drivers/clocksource/dw_apb_timer_of.c              |    6 ++++++
 2 files changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/rockchip/rk3188-timer.txt

diff --git a/Documentation/devicetree/bindings/arm/rockchip/rk3188-timer.txt b/Documentation/devicetree/bindings/arm/rockchip/rk3188-timer.txt
new file mode 100644
index 0000000..ccbb389
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/rockchip/rk3188-timer.txt
@@ -0,0 +1,20 @@
+Rockchip rk3188 timer:
+----------------------
+
+The rk3188 SoCs contain a slightly modified dw-apb-timer.
+
+Required node properties:
+- compatible value : = "rockchip,rk3188-dw-apb-timer-osc";
+
+For the other properties see the generic documentation in
+../../rtc/dw-apb.txt
+
+Example:
+
+	timer3: timer@ffe00000 {
+			compatible = "rockchip,rk3188-dw-apb-timer-osc";
+			interrupts = <0 170 4>;
+			reg = <0xffe00000 0x1000>;
+			clocks = <&timer_clk>, <&timer_pclk>;
+			clock-names = "timer", "pclk";
+		};
diff --git a/drivers/clocksource/dw_apb_timer_of.c b/drivers/clocksource/dw_apb_timer_of.c
index 4bcc1c1..7824796 100644
--- a/drivers/clocksource/dw_apb_timer_of.c
+++ b/drivers/clocksource/dw_apb_timer_of.c
@@ -38,6 +38,11 @@ static void timer_get_base_and_rate(struct device_node *np,
 
 	*quirks = 0;
 
+	if (of_device_is_compatible(np, "rockchip,rk3188-dw-apb-timer-osc"))
+		*quirks |= APBTMR_QUIRK_64BIT_COUNTER | APBTMR_QUIRK_NO_EOI |
+			   APBTMR_QUIRK_INVERSE_INTMASK |
+			   APBTMR_QUIRK_INVERSE_PERIODIC;
+
 	/*
 	 * Not all implementations use a periphal clock, so don't panic
 	 * if it's not present
@@ -165,3 +170,4 @@ static void __init dw_apb_timer_init(struct device_node *timer)
 }
 CLOCKSOURCE_OF_DECLARE(pc3x2_timer, "picochip,pc3x2-timer", dw_apb_timer_init);
 CLOCKSOURCE_OF_DECLARE(apb_timer, "snps,dw-apb-timer-osc", dw_apb_timer_init);
+CLOCKSOURCE_OF_DECLARE(rk3188_timer, "rockchip,rk3188-dw-apb-timer-osc", dw_apb_timer_init);
-- 
1.7.10.4

Re: [PATCH 9/9] clocksource: dw_apb_timer: special variant for rockchip rk3188 timers

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1468 bytes --]

On Sat, 6 Jul 2013, Heiko Stübner wrote:
> +	if (of_device_is_compatible(np, "rockchip,rk3188-dw-apb-timer-osc"))
> +		*quirks |= APBTMR_QUIRK_64BIT_COUNTER | APBTMR_QUIRK_NO_EOI |
> +			   APBTMR_QUIRK_INVERSE_INTMASK |
> +			   APBTMR_QUIRK_INVERSE_PERIODIC;

Brilliant. Next time we add

> +	if (of_device_is_compatible(np, "rockchip,rk3188-dw-apb-timer-osc2"))
> +		*quirks |= APBTMR_QUIRK_64BIT_COUNTER | APBTMR_QUIRK_NO_EOI |
> +			   APBTMR_QUIRK_INVERSE_INTMASK |
> +			   APBTMR_QUIRK_INVERSE_PERIODIC | MORE_NONSENSE;

Plus the extra conditionals all over the place

and a week later

> +	if (of_device_is_compatible(np, "rockchip,rk3188-dw-apb-timer-osc3"))
> +		*quirks |= ALLNONSENSE;


This has nothing to do with QUIRKS at all. QUIRKS are our last resort
when we cannot deal with the problem in a sane way.

In this case this is simply the wrong aproach.

We can deal with it in a sane way as I pointed out before and handle
it as simple properties of the IP block. We have all mechanisms in
place to handle such properties, device tree, platform data, static
init structures. It's all there and we really do not need to plaster
the code with random QUIRK conditionals.

Did you ever consider the runtime penalty of this? Probably not,
otherwise you would have spent time on speeding up that code by
caching frequently accessed registers instead of reading them back
over and over for no reason.

Thanks,

	tglx

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 9/9] clocksource: dw_apb_timer: special variant for rockchip rk3188 timers

[Ulrich Prinz]

Ok, ok...

I got the message. With modifying the existing driver to support more
function pointers in its system struct and assigning them at the
beginning, and using them on runtime, these quirks are obsolete.

Again, this is the first time I provide code to the kernel officially
and I learned from others that I should try it by modifying not too much
code if not needed.

Adding more function pointers to a system relevant structure, doubling
the number of functions and such didn't look non-invasive to me.

But, I totally agree with your argumentation and I even wanted to do it
in the way you explained in your replies. Just the courage was missing I
guess :)

Regards,
	Ulrich

Re: [PATCH 9/9] clocksource: dw_apb_timer: special variant for rockchip rk3188 timers

[Thomas Gleixner]

Ulrich,

On Sat, 6 Jul 2013, Ulrich Prinz wrote:

> I got the message. With modifying the existing driver to support more
> function pointers in its system struct and assigning them at the
> beginning, and using them on runtime, these quirks are obsolete.

Correct.
 
> Again, this is the first time I provide code to the kernel officially

No problem. That's what code review is about. First post or not.

> and I learned from others that I should try it by modifying not too much
> code if not needed.
> 
> Adding more function pointers to a system relevant structure, doubling
> the number of functions and such didn't look non-invasive to me.

Well, it always depends. If there is a single place to deal with some
oddball hardware, a flag is often the simplest way to go.

If you have to add 10 conditionals in several functions then in a
first step converting the existing implementation into function
pointer calls and then in the next step providing new implementations
for your hardware is most of the time simpler and cleaner.

> But, I totally agree with your argumentation and I even wanted to do it
> in the way you explained in your replies. Just the courage was missing I
> guess :)

Gut feelings are often a better guidance than our self-doubting
intellect. :)

But don't worry. I had to learn it the hard way as well and I still
trip over from time to time.

Thanks,

	tglx

Re: [PATCH 9/9] clocksource: dw_apb_timer: special variant for rockchip rk3188 timers

[Ulrich Prinz]

Hey guys,

according the dw_apb_timer I need your statement...

Am 06.07.2013 23:00, schrieb Thomas Gleixner:
> Ulrich,
> 
> On Sat, 6 Jul 2013, Ulrich Prinz wrote:
> 
>> I got the message. With modifying the existing driver to support more
>> function pointers in its system struct and assigning them at the
>> beginning, and using them on runtime, these quirks are obsolete.
> 
> Correct.
>  
>>
>> Adding more function pointers to a system relevant structure, doubling
>> the number of functions and such didn't look non-invasive to me.
> 
> Well, it always depends. If there is a single place to deal with some
> oddball hardware, a flag is often the simplest way to go.
> 
> If you have to add 10 conditionals in several functions then in a
> first step converting the existing implementation into function
> pointer calls and then in the next step providing new implementations
> for your hardware is most of the time simpler and cleaner.
> 
>> But, I totally agree with your argumentation and I even wanted to do it
>> in the way you explained in your replies. Just the courage was missing I
>> guess :)
> 
> Gut feelings are often a better guidance than our self-doubting
> intellect. :)

You can look at my github repo for the new driver, rewriting the
interface as discussed above:
https://github.com/Astralix/linux-rockchip/blob/wip/dw_timer64/drivers/clocksource/dw_apb_timer.c

Summary: It is a 90% rewrite of the complete dw_apb_timer.c and
modifications of dw_apb_timer_of.c and dw_apb_timer.h and the related
x86 drivers are needed.
And after that the 64bit functionality is still not given as the
existing structures do not take it. But for now scheduler doesn't
support 64bit either, I guess.

The drivers local struct stays, but instead of carrying pointers to
registers it now consists of function pointers. This could be used to
fit almost any kind of timer.

I would really appreciate your comments. Sure I need to clean up a bit,
but we're still testing. But before investing more time into that code I
need you comment if I should continue or split to a separate driver for
these kind of timers.

Thanks for any comments

Regards
	Ulrich