clocksource: dw_apb_timer: support for timer variant used in rk3188 SoCs

PATCH In review Heiko Stübner 3 participants 21 messages Jul 5, 2013 – Jul 11, 2013

Subsystems (4)

Device tree bindings x86 Arm Rockchip Clocksource, clockevent drivers

lore

Share & tools

lore

Heiko Stübner

author maintainer

2013-07-05 22:51 UTC #0 lore raw

cover clocksource: dw_apb_timer: support for timer variant used in rk3188 SoCs

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

Signature · 1 line

1.7.10.4

Heiko Stübner

author maintainer

2013-07-05 22:51 UTC #1 lore raw

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

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 <[email protected]>

---
 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(-)

arch/x86/kernel/apb_timer.c +2 -2

index c9876ef..e7fe0f6 100644
@@ -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;

drivers/clocksource/dw_apb_timer.c +5 -2

index 8c2a35f..01bdac0 100644
@@ -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;

drivers/clocksource/dw_apb_timer_of.c +2 -2

index cef5544..b5412af 100644
@@ -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");

include/linux/dw_apb_timer.h +4 -2

index 07261d5..67d09c7 100644
@@ -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);

Signature · 1 line

1.7.10.4

Heiko Stübner

author maintainer

2013-07-05 22:52 UTC #2 lore raw

PATCH 2/9 clocksource: dw_apb_timer: flexible register addresses

In reply to 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 <[email protected]>

---
 drivers/clocksource/dw_apb_timer.c |   83 ++++++++++++++++++++++--------------
 include/linux/dw_apb_timer.h       |    5 +++
 2 files changed, 56 insertions(+), 32 deletions(-)

drivers/clocksource/dw_apb_timer.c +51 ~~-32~~

index 01bdac0..f5e7be8 100644
@@ -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);
 }
 
 /**

include/linux/dw_apb_timer.h +5 -0

index 67d09c7..7dc7166 100644
@@ -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 {

Signature · 1 line

1.7.10.4

Heiko Stübner

author maintainer

2013-07-05 22:53 UTC #3 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/clocksource/dw_apb_timer.c |   27 +++++++++++++++++++++++++++
 include/linux/dw_apb_timer.h       |    6 ++++++
 2 files changed, 33 insertions(+)

drivers/clocksource/dw_apb_timer.c +27 -0

index f5e7be8..bd45351 100644
@@ -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);

include/linux/dw_apb_timer.h +6 -0

index 7dc7166..80f6686 100644
@@ -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;

Signature · 1 line

1.7.10.4

Thomas Gleixner

2013-07-05 23:45 UTC #4 lore raw

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

Quoted 22 lines · 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 <[email protected]>
---
 drivers/clocksource/dw_apb_timer.c |   27 +++++++++++++++++++++++++++
 include/linux/dw_apb_timer.h       |    6 ++++++
 2 files changed, 33 insertions(+)
drivers/clocksource/dw_apb_timer.c +2 -0
index f5e7be8..bd45351 100644
@@ -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;
+
+	/*

drivers/clocksource/dw_apb_timer.c static void apbt_init_regs(struct dw_apb_timer *timer, int quirks) · line 70

Quoted 10 lines

drivers/clocksource/dw_apb_timer.c +9 -0

+	 * 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.

drivers/clocksource/dw_apb_timer.c static void apbt_set_mode(enum clock_event_mode mode, · line 162

Quoted 9 lines

drivers/clocksource/dw_apb_timer.c +4 -0

 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.

drivers/clocksource/dw_apb_timer.c static void apbt_set_mode(enum clock_event_mode mode, · line 189

Quoted 11 lines

drivers/clocksource/dw_apb_timer.c +4 -0

 		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.

drivers/clocksource/dw_apb_timer.c static int apbt_next_event(unsigned long delta, · line 224

Quoted 11 lines

drivers/clocksource/dw_apb_timer.c +4 -0

 		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.

drivers/clocksource/dw_apb_timer.c void dw_apb_clocksource_start(struct dw_apb_clocksource *dw_cs) · line 354

Quoted 11 lines

drivers/clocksource/dw_apb_timer.c +4 -0

 	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

Ulrich Prinz

2013-07-06 20:19 UTC #5 lore raw

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

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:

Quoted 37 lines · Heiko Stübner

Quoted 23 lines
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 <[email protected]>
---
 drivers/clocksource/dw_apb_timer.c |   27 +++++++++++++++++++++++++++
 include/linux/dw_apb_timer.h       |    6 ++++++
 2 files changed, 33 insertions(+)
drivers/clocksource/dw_apb_timer.c +2 -0
index f5e7be8..bd45351 100644
@@ -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;
+
+	/*
drivers/clocksource/dw_apb_timer.c static void apbt_init_regs(struct dw_apb_timer *timer, int quirks) · line 70
Quoted 10 lines
drivers/clocksource/dw_apb_timer.c +9 -0
+	 * 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.

Quoted 1 line

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

ACK

Quoted 2 lines

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

Ok, noted and will be reworked.

Quoted 9 lines

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.

Quoted 14 lines

drivers/clocksource/dw_apb_timer.c static void apbt_set_mode(enum clock_event_mode mode, · line 162
Quoted 9 lines
drivers/clocksource/dw_apb_timer.c +4 -0
 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.

Quoted 4 lines

     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

Quoted 5 lines

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

Heiko Stübner

author maintainer

2013-07-05 22:53 UTC #6 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/clocksource/dw_apb_timer.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

drivers/clocksource/dw_apb_timer.c +7 -4

index bd45351..5f80a30 100644
@@ -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);
 }
 
 /**

Signature · 1 line

1.7.10.4

Heiko Stübner

author maintainer

2013-07-05 22:59 UTC #7 lore raw

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

this patch should have had a

From: Ulrich Prinz <[email protected]>

sorry for the mistake

Am Samstag, 6. Juli 2013, 00:53:36 schrieb Heiko St?bner:

Quoted 49 lines

Signed-off-by Ulrich Prinz <[email protected]>

---
 drivers/clocksource/dw_apb_timer.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

(quoted diff) +0 -0

b/drivers/clocksource/dw_apb_timer.c index bd45351..5f80a30 100644

drivers/clocksource/dw_apb_timer.c +7 -4

@@ -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);
 }

/**

Heiko Stübner

author maintainer

2013-07-05 22:54 UTC #8 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/clocksource/dw_apb_timer.c |   10 +++++++++-
 include/linux/dw_apb_timer.h       |    5 +++++
 2 files changed, 14 insertions(+), 1 deletion(-)

drivers/clocksource/dw_apb_timer.c +9 -1

index 5f80a30..23cd7c6 100644
@@ -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");

include/linux/dw_apb_timer.h +5 -0

index 80f6686..fbe4c6b 100644
@@ -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;

Signature · 1 line

1.7.10.4

Heiko Stübner

author maintainer

2013-07-05 22:58 UTC #9 lore raw

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

this patch should have had a

From: Ulrich Prinz <[email protected]>

sorry for the mistake

Am Samstag, 6. Juli 2013, 00:54:07 schrieb Heiko St?bner:

Quoted 53 lines

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 <[email protected]>

---
 drivers/clocksource/dw_apb_timer.c |   10 +++++++++-
 include/linux/dw_apb_timer.h       |    5 +++++
 2 files changed, 14 insertions(+), 1 deletion(-)

(quoted diff) +0 -0

b/drivers/clocksource/dw_apb_timer.c index 5f80a30..23cd7c6 100644

drivers/clocksource/dw_apb_timer.c +5 -0

@@ -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 |

drivers/clocksource/dw_apb_timer.c +4 -1

 					  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");

include/linux/dw_apb_timer.h +5 -0

index 80f6686..fbe4c6b 100644
@@ -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;

Thomas Gleixner

2013-07-05 23:49 UTC #10 lore raw

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

In reply to Heiko Stübner

drivers/clocksource/dw_apb_timer.c dw_apb_clockevent_init(int cpu, const char *name, unsigned rating, · line 297

Quoted 5 lines · Heiko St?bner

drivers/clocksource/dw_apb_timer.c +4 -1

-	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

Heiko Stübner

author maintainer

2013-07-05 22:54 UTC #11 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/clocksource/dw_apb_timer.c |   23 ++++++++++++++++++-----
 include/linux/dw_apb_timer.h       |    6 ++++++
 2 files changed, 24 insertions(+), 5 deletions(-)

drivers/clocksource/dw_apb_timer.c +18 -5

index 23cd7c6..7705d13 100644
@@ -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);

include/linux/dw_apb_timer.h +6 -0

index fbe4c6b..7d36d91 100644
@@ -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;

Signature · 1 line

1.7.10.4

Heiko Stübner

author maintainer

2013-07-05 22:58 UTC #12 lore raw

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

this patch should have had a

From: Ulrich Prinz <[email protected]>

sorry for the mistake

Am Samstag, 6. Juli 2013, 00:54:35 schrieb Heiko St?bner:

Quoted 82 lines

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 <[email protected]>

---
 drivers/clocksource/dw_apb_timer.c |   23 ++++++++++++++++++-----
 include/linux/dw_apb_timer.h       |    6 ++++++
 2 files changed, 24 insertions(+), 5 deletions(-)

(quoted diff) +0 -0

b/drivers/clocksource/dw_apb_timer.c index 23cd7c6..7705d13 100644

drivers/clocksource/dw_apb_timer.c +4 -1

@@ -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 */

drivers/clocksource/dw_apb_timer.c +14 -4

 	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);

include/linux/dw_apb_timer.h +2 -0

index fbe4c6b..7d36d91 100644
@@ -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.
+ */

include/linux/dw_apb_timer.h +2 -0

+#define APBTMR_QUIRK_INVERSE_INTMASK	BIT(2)
+
 struct dw_apb_timer {
 	void __iomem				*base;
 	unsigned long				freq;

Thomas Gleixner

2013-07-05 23:51 UTC #13 lore raw

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

In reply to Heiko Stübner

Quoted 2 lines · Heiko St?bner

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.

Heiko Stübner

author maintainer

2013-07-05 22:55 UTC #14 lore raw

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

In reply to Heiko Stübner

From: Ulrich Prinz <[email protected]>

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 <[email protected]>

---
 drivers/clocksource/dw_apb_timer.c |   11 +++++++++--
 include/linux/dw_apb_timer.h       |    6 ++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/clocksource/dw_apb_timer.c
index 7705d13..a2e8306 100644

drivers/clocksource/dw_apb_timer.c +9 -2

@@ -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 */

include/linux/dw_apb_timer.h +6 -0

index 7d36d91..5d9210cc 100644
@@ -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;

Signature · 1 line

1.7.10.4

Heiko Stübner

author maintainer

2013-07-05 22:56 UTC #15 lore raw

PATCH 8/9 clocksource: dw_apb_timer_of: add quirk handling

In reply to 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 <[email protected]>

---
 drivers/clocksource/dw_apb_timer_of.c |   21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

drivers/clocksource/dw_apb_timer_of.c +15 -6

index b5412af..4bcc1c1 100644
@@ -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);
 	}

Signature · 1 line

1.7.10.4

Heiko Stübner

author maintainer

2013-07-05 22:56 UTC #16 lore raw

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

In reply to 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 <[email protected]>

---
 .../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

Documentation/devicetree/bindings/arm/rockchip/rk3188-timer.txt new file +20 -0

new file mode 100644
index 0000000..ccbb389
@@ -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 at ffe00000 {
+			compatible = "rockchip,rk3188-dw-apb-timer-osc";
+			interrupts = <0 170 4>;
+			reg = <0xffe00000 0x1000>;
+			clocks = <&timer_clk>, <&timer_pclk>;
+			clock-names = "timer", "pclk";
+		};

drivers/clocksource/dw_apb_timer_of.c +6 -0

index 4bcc1c1..7824796 100644
@@ -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);

Signature · 1 line

1.7.10.4

Thomas Gleixner

2013-07-06 00:12 UTC #17 lore raw

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

drivers/clocksource/dw_apb_timer_of.c static void timer_get_base_and_rate(struct device_node *np, · line 44

Quoted 4 lines · Heiko St?bner

drivers/clocksource/dw_apb_timer_of.c +4 -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;

Brilliant. Next time we add

drivers/clocksource/dw_apb_timer_of.c static void timer_get_base_and_rate(struct device_node *np,

Quoted 4 lines

drivers/clocksource/dw_apb_timer_of.c +4 -0

+	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

Review

Quoted 2 lines

(quoted diff) +2 -0

+	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

Ulrich Prinz

2013-07-06 20:28 UTC #18 lore raw

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

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

Thomas Gleixner

2013-07-06 21:00 UTC #19 lore raw

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

Ulrich,

Quoted 3 lines · Ulrich Prinz

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.

Quoted 1 line

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.

Quoted 5 lines

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.

Quoted 3 lines

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

Ulrich Prinz

2013-07-11 22:44 UTC #20 lore raw

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

Hey guys,

according the dw_apb_timer I need your statement...

Am 06.07.2013 23:00, schrieb Thomas Gleixner:

Quoted 22 lines

Ulrich,
Quoted 3 lines · Ulrich Prinz
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.
Quoted 3 lines
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.

Quoted 6 lines

Quoted 3 lines
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