[PATCH 0/2] clocksource: sp804: Add OX810SE 24bit mode Support

[PATCH 0/2] clocksource: sp804: Add OX810SE 24bit mode Support

[Neil Armstrong]

This patchset includes support for Oxford Semiconductor OX810SE SoC by adding
variable width support. It selects the 24bit mode with a new compatible string.

It adds a timer_sp804 structure used to configure the clocksource and
clockevents.

It was originally posted part of a platform patchset available here :
http://lkml.kernel.org/r/1458838215-23314-1-git-send-email-narmstrong at baylibre.com

Changes this v3 patchset :
- Get rid of PLX Technology, replace by Oxford Semiconductor
- Switch to timer_sp804 structure to configure clocksource and clockevents

Neil Armstrong (2):
  clocksource: sp804: Add support for OX810SE 24bit timer width
  dt-bindings: timer: sp804: add new compatible for OX810SE SoC

 .../devicetree/bindings/timer/arm,sp804.txt        |   2 +-
 drivers/clocksource/timer-sp804.c                  | 107 +++++++++++++--------
 include/clocksource/timer-sp804.h                  |  42 ++++++--
 3 files changed, 103 insertions(+), 48 deletions(-)

-- 
1.9.1

[PATCH 1/2] clocksource: sp804: Add support for OX810SE 24bit timer width

[Neil Armstrong]

In order to support the Dual-Timer on the Oxford Semiconductor OX810SE SoC,
implement variable counter width, keeping 32bit as default width.
Add new compatible string oxsemi,ox810se-rps-timer in order to select
the 24bit counter width.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/clocksource/timer-sp804.c | 107 ++++++++++++++++++++++++--------------
 include/clocksource/timer-sp804.h |  42 ++++++++++++---
 2 files changed, 102 insertions(+), 47 deletions(-)

diff --git a/drivers/clocksource/timer-sp804.c b/drivers/clocksource/timer-sp804.c
index 5f45b9a..e99269a 100644
--- a/drivers/clocksource/timer-sp804.c
+++ b/drivers/clocksource/timer-sp804.c
@@ -77,15 +77,15 @@ void __init sp804_timer_disable(void __iomem *base)
 	writel(0, base + TIMER_CTRL);
 }
 
-void __init __sp804_clocksource_and_sched_clock_init(void __iomem *base,
-						     const char *name,
-						     struct clk *clk,
-						     int use_sched_clock)
+void __init __sp804_clocksource_and_sched_clock_init(struct timer_sp804 *sp804,
+						     bool use_sched_clock)
 {
 	long rate;
+	u32 config = TIMER_CTRL_ENABLE | TIMER_CTRL_PERIODIC;
+	struct clk *clk = sp804->clocksource_clk;
 
 	if (!clk) {
-		clk = clk_get_sys("sp804", name);
+		clk = clk_get_sys("sp804", sp804->name);
 		if (IS_ERR(clk)) {
 			pr_err("sp804: clock not found: %d\n",
 			       (int)PTR_ERR(clk));
@@ -98,19 +98,22 @@ void __init __sp804_clocksource_and_sched_clock_init(void __iomem *base,
 	if (rate < 0)
 		return;
 
+	if (sp804->width == 32)
+		config |= TIMER_CTRL_32BIT;
+
 	/* setup timer 0 as free-running clocksource */
-	writel(0, base + TIMER_CTRL);
-	writel(0xffffffff, base + TIMER_LOAD);
-	writel(0xffffffff, base + TIMER_VALUE);
-	writel(TIMER_CTRL_32BIT | TIMER_CTRL_ENABLE | TIMER_CTRL_PERIODIC,
-		base + TIMER_CTRL);
+	writel(0, sp804->clocksource_base + TIMER_CTRL);
+	writel(0xffffffff, sp804->clocksource_base + TIMER_LOAD);
+	writel(0xffffffff, sp804->clocksource_base + TIMER_VALUE);
+	writel(config, sp804->clocksource_base + TIMER_CTRL);
 
-	clocksource_mmio_init(base + TIMER_VALUE, name,
-		rate, 200, 32, clocksource_mmio_readl_down);
+	clocksource_mmio_init(sp804->clocksource_base + TIMER_VALUE,
+			      sp804->name, rate, 200, sp804->width,
+			      clocksource_mmio_readl_down);
 
 	if (use_sched_clock) {
-		sched_clock_base = base;
-		sched_clock_register(sp804_read, 32, rate);
+		sched_clock_base = sp804->clocksource_base;
+		sched_clock_register(sp804_read, sp804->width, rate);
 	}
 }
 
@@ -186,15 +189,16 @@ static struct irqaction sp804_timer_irq = {
 	.dev_id		= &sp804_clockevent,
 };
 
-void __init __sp804_clockevents_init(void __iomem *base, unsigned int irq, struct clk *clk, const char *name)
+void __init __sp804_clockevents_init(struct timer_sp804 *sp804)
 {
 	struct clock_event_device *evt = &sp804_clockevent;
 	long rate;
+	struct clk *clk = sp804->clockevent_clk;
 
 	if (!clk)
-		clk = clk_get_sys("sp804", name);
+		clk = clk_get_sys("sp804", sp804->name);
 	if (IS_ERR(clk)) {
-		pr_err("sp804: %s clock not found: %d\n", name,
+		pr_err("sp804: %s clock not found: %d\n", sp804->name,
 			(int)PTR_ERR(clk));
 		return;
 	}
@@ -203,26 +207,27 @@ void __init __sp804_clockevents_init(void __iomem *base, unsigned int irq, struc
 	if (rate < 0)
 		return;
 
-	clkevt_base = base;
+	clkevt_base = sp804->clockevent_base;
 	clkevt_reload = DIV_ROUND_CLOSEST(rate, HZ);
-	evt->name = name;
-	evt->irq = irq;
+	evt->name = sp804->name;
+	evt->irq = sp804->irq;
 	evt->cpumask = cpu_possible_mask;
 
-	writel(0, base + TIMER_CTRL);
+	writel(0, sp804->clockevent_base + TIMER_CTRL);
 
-	setup_irq(irq, &sp804_timer_irq);
-	clockevents_config_and_register(evt, rate, 0xf, 0xffffffff);
+	setup_irq(sp804->irq, &sp804_timer_irq);
+	clockevents_config_and_register(evt, rate, 0xf,
+					GENMASK(sp804->width-1, 0));
 }
 
 static void __init sp804_of_init(struct device_node *np)
 {
 	static bool initialized = false;
+	struct timer_sp804 sp804;
 	void __iomem *base;
-	int irq;
 	u32 irq_num = 0;
 	struct clk *clk1, *clk2;
-	const char *name = of_get_property(np, "compatible", NULL);
+	u32 width = 32;
 
 	base = of_iomap(np, 0);
 	if (WARN_ON(!base))
@@ -250,19 +255,33 @@ static void __init sp804_of_init(struct device_node *np)
 	} else
 		clk2 = clk1;
 
-	irq = irq_of_parse_and_map(np, 0);
-	if (irq <= 0)
+	sp804.irq = irq_of_parse_and_map(np, 0);
+	if (sp804.irq <= 0)
 		goto err;
 
+	/* OX810SE Uses a special 24bit width */
+	if (of_device_is_compatible(np, "oxsemi,ox810se-rps-timer"))
+		width = 24;
+
 	of_property_read_u32(np, "arm,sp804-has-irq", &irq_num);
 	if (irq_num == 2) {
-		__sp804_clockevents_init(base + TIMER_2_BASE, irq, clk2, name);
-		__sp804_clocksource_and_sched_clock_init(base, name, clk1, 1);
+		sp804.clockevent_base = base + TIMER_2_BASE;
+		sp804.clocksource_base = base;
+		sp804.clockevent_clk = clk2;
+		sp804.clocksource_clk = clk1;
 	} else {
-		__sp804_clockevents_init(base, irq, clk1 , name);
-		__sp804_clocksource_and_sched_clock_init(base + TIMER_2_BASE,
-							 name, clk2, 1);
+		sp804.clockevent_base = base;
+		sp804.clocksource_base = base + TIMER_2_BASE;
+		sp804.clockevent_clk = clk1;
+		sp804.clocksource_clk = clk2;
 	}
+
+	sp804.name = of_get_property(np, "compatible", NULL);
+	sp804.width = width;
+
+	__sp804_clockevents_init(&sp804);
+	__sp804_clocksource_and_sched_clock_init(&sp804, true);
+
 	initialized = true;
 
 	return;
@@ -270,13 +289,13 @@ err:
 	iounmap(base);
 }
 CLOCKSOURCE_OF_DECLARE(sp804, "arm,sp804", sp804_of_init);
+CLOCKSOURCE_OF_DECLARE(ox810se, "oxsemi,ox810se-rps-timer", sp804_of_init);
 
 static void __init integrator_cp_of_init(struct device_node *np)
 {
 	static int init_count = 0;
+	struct timer_sp804 sp804;
 	void __iomem *base;
-	int irq;
-	const char *name = of_get_property(np, "compatible", NULL);
 	struct clk *clk;
 
 	base = of_iomap(np, 0);
@@ -292,14 +311,22 @@ static void __init integrator_cp_of_init(struct device_node *np)
 	if (init_count == 2 || !of_device_is_available(np))
 		goto err;
 
-	if (!init_count)
-		__sp804_clocksource_and_sched_clock_init(base, name, clk, 0);
-	else {
-		irq = irq_of_parse_and_map(np, 0);
-		if (irq <= 0)
+	sp804.name = of_get_property(np, "compatible", NULL);
+	sp804.width = 32;
+
+	if (!init_count) {
+		sp804.clocksource_base = base;
+		sp804.clocksource_clk = clk;
+
+		__sp804_clocksource_and_sched_clock_init(&sp804, false);
+	} else {
+		sp804.clockevent_base = base;
+		sp804.clockevent_clk = clk;
+		sp804.irq = irq_of_parse_and_map(np, 0);
+		if (sp804.irq <= 0)
 			goto err;
 
-		__sp804_clockevents_init(base, irq, clk, name);
+		__sp804_clockevents_init(&sp804);
 	}
 
 	init_count++;
diff --git a/include/clocksource/timer-sp804.h b/include/clocksource/timer-sp804.h
index 1f8a1ca..928091b 100644
--- a/include/clocksource/timer-sp804.h
+++ b/include/clocksource/timer-sp804.h
@@ -3,26 +3,54 @@
 
 struct clk;
 
-void __sp804_clocksource_and_sched_clock_init(void __iomem *,
-					      const char *, struct clk *, int);
-void __sp804_clockevents_init(void __iomem *, unsigned int,
-			      struct clk *, const char *);
+struct timer_sp804 {
+	void __iomem *clockevent_base;
+	void __iomem *clocksource_base;
+	const char *name;
+	struct clk *clockevent_clk;
+	struct clk *clocksource_clk;
+	unsigned int irq;
+	unsigned int width;
+};
+
+void __sp804_clocksource_and_sched_clock_init(struct timer_sp804 *sp804, bool);
+void __sp804_clockevents_init(struct timer_sp804 *sp804);
 void sp804_timer_disable(void __iomem *);
 
 static inline void sp804_clocksource_init(void __iomem *base, const char *name)
 {
-	__sp804_clocksource_and_sched_clock_init(base, name, NULL, 0);
+	struct timer_sp804 sp804 = {
+		.clocksource_base = base,
+		.name = name,
+		.clocksource_clk = NULL,
+		.width = 32,
+	};
+
+	__sp804_clocksource_and_sched_clock_init(&sp804, false);
 }
 
 static inline void sp804_clocksource_and_sched_clock_init(void __iomem *base,
 							  const char *name)
 {
-	__sp804_clocksource_and_sched_clock_init(base, name, NULL, 1);
+	struct timer_sp804 sp804 = {
+		.clocksource_base = base,
+		.name = name,
+		.clocksource_clk = NULL,
+		.width = 32,
+	};
+
+	__sp804_clocksource_and_sched_clock_init(&sp804, true);
 }
 
 static inline void sp804_clockevents_init(void __iomem *base, unsigned int irq, const char *name)
 {
-	__sp804_clockevents_init(base, irq, NULL, name);
+	struct timer_sp804 sp804 = {
+		.clockevent_base = base,
+		.name = name,
+		.clockevent_clk = NULL,
+		.width = 32,
+	};
 
+	__sp804_clockevents_init(&sp804);
 }
 #endif
-- 
1.9.1

[PATCH 1/2] clocksource: sp804: Add support for OX810SE 24bit timer width

[Mathieu Poirier]

On 1 April 2016 at 08:22, Neil Armstrong <narmstrong@baylibre.com> wrote:
> In order to support the Dual-Timer on the Oxford Semiconductor OX810SE SoC,
> implement variable counter width, keeping 32bit as default width.
> Add new compatible string oxsemi,ox810se-rps-timer in order to select
> the 24bit counter width.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/clocksource/timer-sp804.c | 107 ++++++++++++++++++++++++--------------
>  include/clocksource/timer-sp804.h |  42 ++++++++++++---
>  2 files changed, 102 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/clocksource/timer-sp804.c b/drivers/clocksource/timer-sp804.c
> index 5f45b9a..e99269a 100644
> --- a/drivers/clocksource/timer-sp804.c
> +++ b/drivers/clocksource/timer-sp804.c
> @@ -77,15 +77,15 @@ void __init sp804_timer_disable(void __iomem *base)
>         writel(0, base + TIMER_CTRL);
>  }
>
> -void __init __sp804_clocksource_and_sched_clock_init(void __iomem *base,
> -                                                    const char *name,
> -                                                    struct clk *clk,
> -                                                    int use_sched_clock)
> +void __init __sp804_clocksource_and_sched_clock_init(struct timer_sp804 *sp804,
> +                                                    bool use_sched_clock)
>  {
>         long rate;
> +       u32 config = TIMER_CTRL_ENABLE | TIMER_CTRL_PERIODIC;
> +       struct clk *clk = sp804->clocksource_clk;
>
>         if (!clk) {
> -               clk = clk_get_sys("sp804", name);
> +               clk = clk_get_sys("sp804", sp804->name);
>                 if (IS_ERR(clk)) {
>                         pr_err("sp804: clock not found: %d\n",
>                                (int)PTR_ERR(clk));
> @@ -98,19 +98,22 @@ void __init __sp804_clocksource_and_sched_clock_init(void __iomem *base,
>         if (rate < 0)
>                 return;
>
> +       if (sp804->width == 32)
> +               config |= TIMER_CTRL_32BIT;
> +
>         /* setup timer 0 as free-running clocksource */
> -       writel(0, base + TIMER_CTRL);
> -       writel(0xffffffff, base + TIMER_LOAD);
> -       writel(0xffffffff, base + TIMER_VALUE);
> -       writel(TIMER_CTRL_32BIT | TIMER_CTRL_ENABLE | TIMER_CTRL_PERIODIC,
> -               base + TIMER_CTRL);
> +       writel(0, sp804->clocksource_base + TIMER_CTRL);
> +       writel(0xffffffff, sp804->clocksource_base + TIMER_LOAD);
> +       writel(0xffffffff, sp804->clocksource_base + TIMER_VALUE);
> +       writel(config, sp804->clocksource_base + TIMER_CTRL);
>
> -       clocksource_mmio_init(base + TIMER_VALUE, name,
> -               rate, 200, 32, clocksource_mmio_readl_down);
> +       clocksource_mmio_init(sp804->clocksource_base + TIMER_VALUE,
> +                             sp804->name, rate, 200, sp804->width,
> +                             clocksource_mmio_readl_down);
>
>         if (use_sched_clock) {
> -               sched_clock_base = base;
> -               sched_clock_register(sp804_read, 32, rate);
> +               sched_clock_base = sp804->clocksource_base;
> +               sched_clock_register(sp804_read, sp804->width, rate);
>         }
>  }
>
> @@ -186,15 +189,16 @@ static struct irqaction sp804_timer_irq = {
>         .dev_id         = &sp804_clockevent,
>  };
>
> -void __init __sp804_clockevents_init(void __iomem *base, unsigned int irq, struct clk *clk, const char *name)
> +void __init __sp804_clockevents_init(struct timer_sp804 *sp804)
>  {
>         struct clock_event_device *evt = &sp804_clockevent;
>         long rate;
> +       struct clk *clk = sp804->clockevent_clk;
>
>         if (!clk)
> -               clk = clk_get_sys("sp804", name);
> +               clk = clk_get_sys("sp804", sp804->name);
>         if (IS_ERR(clk)) {
> -               pr_err("sp804: %s clock not found: %d\n", name,
> +               pr_err("sp804: %s clock not found: %d\n", sp804->name,
>                         (int)PTR_ERR(clk));
>                 return;
>         }
> @@ -203,26 +207,27 @@ void __init __sp804_clockevents_init(void __iomem *base, unsigned int irq, struc
>         if (rate < 0)
>                 return;
>
> -       clkevt_base = base;
> +       clkevt_base = sp804->clockevent_base;
>         clkevt_reload = DIV_ROUND_CLOSEST(rate, HZ);
> -       evt->name = name;
> -       evt->irq = irq;
> +       evt->name = sp804->name;
> +       evt->irq = sp804->irq;
>         evt->cpumask = cpu_possible_mask;
>
> -       writel(0, base + TIMER_CTRL);
> +       writel(0, sp804->clockevent_base + TIMER_CTRL);
>
> -       setup_irq(irq, &sp804_timer_irq);
> -       clockevents_config_and_register(evt, rate, 0xf, 0xffffffff);
> +       setup_irq(sp804->irq, &sp804_timer_irq);
> +       clockevents_config_and_register(evt, rate, 0xf,
> +                                       GENMASK(sp804->width-1, 0));
>  }
>
>  static void __init sp804_of_init(struct device_node *np)
>  {
>         static bool initialized = false;
> +       struct timer_sp804 sp804;
>         void __iomem *base;
> -       int irq;
>         u32 irq_num = 0;
>         struct clk *clk1, *clk2;
> -       const char *name = of_get_property(np, "compatible", NULL);
> +       u32 width = 32;
>
>         base = of_iomap(np, 0);
>         if (WARN_ON(!base))
> @@ -250,19 +255,33 @@ static void __init sp804_of_init(struct device_node *np)
>         } else
>                 clk2 = clk1;
>
> -       irq = irq_of_parse_and_map(np, 0);
> -       if (irq <= 0)
> +       sp804.irq = irq_of_parse_and_map(np, 0);
> +       if (sp804.irq <= 0)
>                 goto err;
>
> +       /* OX810SE Uses a special 24bit width */
> +       if (of_device_is_compatible(np, "oxsemi,ox810se-rps-timer"))
> +               width = 24;

Wouldn't it be better to add a new optional property for this?  If the
property is not specified then we default to a width of 32.  Otherwise
the new width is configured.  That way we don't have to add a new
compatible string every time (however often) a special width is
encountered.

Otherwise I think the code facelift looks good,
Mathieu

> +
>         of_property_read_u32(np, "arm,sp804-has-irq", &irq_num);
>         if (irq_num == 2) {
> -               __sp804_clockevents_init(base + TIMER_2_BASE, irq, clk2, name);
> -               __sp804_clocksource_and_sched_clock_init(base, name, clk1, 1);
> +               sp804.clockevent_base = base + TIMER_2_BASE;
> +               sp804.clocksource_base = base;
> +               sp804.clockevent_clk = clk2;
> +               sp804.clocksource_clk = clk1;
>         } else {
> -               __sp804_clockevents_init(base, irq, clk1 , name);
> -               __sp804_clocksource_and_sched_clock_init(base + TIMER_2_BASE,
> -                                                        name, clk2, 1);
> +               sp804.clockevent_base = base;
> +               sp804.clocksource_base = base + TIMER_2_BASE;
> +               sp804.clockevent_clk = clk1;
> +               sp804.clocksource_clk = clk2;
>         }
> +
> +       sp804.name = of_get_property(np, "compatible", NULL);
> +       sp804.width = width;
> +
> +       __sp804_clockevents_init(&sp804);
> +       __sp804_clocksource_and_sched_clock_init(&sp804, true);
> +
>         initialized = true;
>
>         return;
> @@ -270,13 +289,13 @@ err:
>         iounmap(base);
>  }
>  CLOCKSOURCE_OF_DECLARE(sp804, "arm,sp804", sp804_of_init);
> +CLOCKSOURCE_OF_DECLARE(ox810se, "oxsemi,ox810se-rps-timer", sp804_of_init);
>
>  static void __init integrator_cp_of_init(struct device_node *np)
>  {
>         static int init_count = 0;
> +       struct timer_sp804 sp804;
>         void __iomem *base;
> -       int irq;
> -       const char *name = of_get_property(np, "compatible", NULL);
>         struct clk *clk;
>
>         base = of_iomap(np, 0);
> @@ -292,14 +311,22 @@ static void __init integrator_cp_of_init(struct device_node *np)
>         if (init_count == 2 || !of_device_is_available(np))
>                 goto err;
>
> -       if (!init_count)
> -               __sp804_clocksource_and_sched_clock_init(base, name, clk, 0);
> -       else {
> -               irq = irq_of_parse_and_map(np, 0);
> -               if (irq <= 0)
> +       sp804.name = of_get_property(np, "compatible", NULL);
> +       sp804.width = 32;
> +
> +       if (!init_count) {
> +               sp804.clocksource_base = base;
> +               sp804.clocksource_clk = clk;
> +
> +               __sp804_clocksource_and_sched_clock_init(&sp804, false);
> +       } else {
> +               sp804.clockevent_base = base;
> +               sp804.clockevent_clk = clk;
> +               sp804.irq = irq_of_parse_and_map(np, 0);
> +               if (sp804.irq <= 0)
>                         goto err;
>
> -               __sp804_clockevents_init(base, irq, clk, name);
> +               __sp804_clockevents_init(&sp804);
>         }
>
>         init_count++;
> diff --git a/include/clocksource/timer-sp804.h b/include/clocksource/timer-sp804.h
> index 1f8a1ca..928091b 100644
> --- a/include/clocksource/timer-sp804.h
> +++ b/include/clocksource/timer-sp804.h
> @@ -3,26 +3,54 @@
>
>  struct clk;
>
> -void __sp804_clocksource_and_sched_clock_init(void __iomem *,
> -                                             const char *, struct clk *, int);
> -void __sp804_clockevents_init(void __iomem *, unsigned int,
> -                             struct clk *, const char *);
> +struct timer_sp804 {
> +       void __iomem *clockevent_base;
> +       void __iomem *clocksource_base;
> +       const char *name;
> +       struct clk *clockevent_clk;
> +       struct clk *clocksource_clk;
> +       unsigned int irq;
> +       unsigned int width;
> +};
> +
> +void __sp804_clocksource_and_sched_clock_init(struct timer_sp804 *sp804, bool);
> +void __sp804_clockevents_init(struct timer_sp804 *sp804);
>  void sp804_timer_disable(void __iomem *);
>
>  static inline void sp804_clocksource_init(void __iomem *base, const char *name)
>  {
> -       __sp804_clocksource_and_sched_clock_init(base, name, NULL, 0);
> +       struct timer_sp804 sp804 = {
> +               .clocksource_base = base,
> +               .name = name,
> +               .clocksource_clk = NULL,
> +               .width = 32,
> +       };
> +
> +       __sp804_clocksource_and_sched_clock_init(&sp804, false);
>  }
>
>  static inline void sp804_clocksource_and_sched_clock_init(void __iomem *base,
>                                                           const char *name)
>  {
> -       __sp804_clocksource_and_sched_clock_init(base, name, NULL, 1);
> +       struct timer_sp804 sp804 = {
> +               .clocksource_base = base,
> +               .name = name,
> +               .clocksource_clk = NULL,
> +               .width = 32,
> +       };
> +
> +       __sp804_clocksource_and_sched_clock_init(&sp804, true);
>  }
>
>  static inline void sp804_clockevents_init(void __iomem *base, unsigned int irq, const char *name)
>  {
> -       __sp804_clockevents_init(base, irq, NULL, name);
> +       struct timer_sp804 sp804 = {
> +               .clockevent_base = base,
> +               .name = name,
> +               .clockevent_clk = NULL,
> +               .width = 32,
> +       };
>
> +       __sp804_clockevents_init(&sp804);
>  }
>  #endif
> --
> 1.9.1
>

[PATCH 1/2] clocksource: sp804: Add support for OX810SE 24bit timer width

[Neil Armstrong]

On 04/01/2016 05:33 PM, Mathieu Poirier wrote:
> On 1 April 2016 at 08:22, Neil Armstrong <narmstrong@baylibre.com> wrote:
Hi Matthieu,
Thanks for the review.

>> +       /* OX810SE Uses a special 24bit width */
>> +       if (of_device_is_compatible(np, "oxsemi,ox810se-rps-timer"))
>> +               width = 24;
> 
> Wouldn't it be better to add a new optional property for this?  If the
> property is not specified then we default to a width of 32.  Otherwise
> the new width is configured.  That way we don't have to add a new
> compatible string every time (however often) a special width is
> encountered.
As explained by Rob Herring, this HW is compatible with SP804 but is /not/ an IP from ARM
and has been customized to support 24bit.
Having a DT property for this width is not wanted actually since it makes no sense for the
original SP804 IP, so it should be handled per vendor.

> 
> Otherwise I think the code facelift looks good,
> Mathieu

Is there any other blocking points ?

The point to have a change on sp804 is to economize a new very similar driver for oxnas.

Neil

[PATCH 1/2] clocksource: sp804: Add support for OX810SE 24bit timer width

[Daniel Lezcano]

On Fri, Apr 01, 2016 at 04:22:38PM +0200, Neil Armstrong wrote:
> In order to support the Dual-Timer on the Oxford Semiconductor OX810SE SoC,
> implement variable counter width, keeping 32bit as default width.
> Add new compatible string oxsemi,ox810se-rps-timer in order to select
> the 24bit counter width.
> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/clocksource/timer-sp804.c | 107 ++++++++++++++++++++++++--------------
>  include/clocksource/timer-sp804.h |  42 ++++++++++++---
>  2 files changed, 102 insertions(+), 47 deletions(-)

I will take those patches but this driver really deserves a cleanup.

[PATCH 1/2] clocksource: sp804: Add support for OX810SE 24bit timer width

[Thomas Gleixner]

On Fri, 22 Apr 2016, Daniel Lezcano wrote:

> On Fri, Apr 01, 2016 at 04:22:38PM +0200, Neil Armstrong wrote:
> > In order to support the Dual-Timer on the Oxford Semiconductor OX810SE SoC,
> > implement variable counter width, keeping 32bit as default width.
> > Add new compatible string oxsemi,ox810se-rps-timer in order to select
> > the 24bit counter width.
> > 
> > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> > ---
> >  drivers/clocksource/timer-sp804.c | 107 ++++++++++++++++++++++++--------------
> >  include/clocksource/timer-sp804.h |  42 ++++++++++++---
> >  2 files changed, 102 insertions(+), 47 deletions(-)
> 
> I will take those patches but this driver really deserves a cleanup.

If it deserves a cleanup, then this should happen _BEFORE_ we add new
functionality to it.

Thanks,

	tglx

[PATCH 1/2] clocksource: sp804: Add support for OX810SE 24bit timer width

[Neil Armstrong]

On 04/22/2016 09:53 AM, Thomas Gleixner wrote:
> On Fri, 22 Apr 2016, Daniel Lezcano wrote:
> 
>> On Fri, Apr 01, 2016 at 04:22:38PM +0200, Neil Armstrong wrote:
>>> In order to support the Dual-Timer on the Oxford Semiconductor OX810SE SoC,
>>> implement variable counter width, keeping 32bit as default width.
>>> Add new compatible string oxsemi,ox810se-rps-timer in order to select
>>> the 24bit counter width.
>>>
>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>> ---
>>>  drivers/clocksource/timer-sp804.c | 107 ++++++++++++++++++++++++--------------
>>>  include/clocksource/timer-sp804.h |  42 ++++++++++++---
>>>  2 files changed, 102 insertions(+), 47 deletions(-)
>>
>> I will take those patches but this driver really deserves a cleanup.
> 
> If it deserves a cleanup, then this should happen _BEFORE_ we add new
> functionality to it.
> 
> Thanks,
> 
> 	tglx
> 

Hi Thomas, Daniel,

Sure this driver should have a cleanup, but it still depends on old legacy vexpress calls.

The reason I submitted a change and a small cleanup over this driver is because the high
similarities of the HW and avoiding adding a brand new driver cloning much of its
functionalities.

I propose to study an eventual cleanup, but could this change be submitted for 4.7 since
it's part of a base platform support serie ?

Thanks,
Neil

[PATCH 1/2] clocksource: sp804: Add support for OX810SE 24bit timer width

[Daniel Lezcano]

On Mon, Apr 25, 2016 at 04:48:24PM +0200, Neil Armstrong wrote:
> On 04/22/2016 09:53 AM, Thomas Gleixner wrote:
> > On Fri, 22 Apr 2016, Daniel Lezcano wrote:
> > 
> >> On Fri, Apr 01, 2016 at 04:22:38PM +0200, Neil Armstrong wrote:
> >>> In order to support the Dual-Timer on the Oxford Semiconductor OX810SE SoC,
> >>> implement variable counter width, keeping 32bit as default width.
> >>> Add new compatible string oxsemi,ox810se-rps-timer in order to select
> >>> the 24bit counter width.
> >>>
> >>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> >>> ---
> >>>  drivers/clocksource/timer-sp804.c | 107 ++++++++++++++++++++++++--------------
> >>>  include/clocksource/timer-sp804.h |  42 ++++++++++++---
> >>>  2 files changed, 102 insertions(+), 47 deletions(-)
> >>
> >> I will take those patches but this driver really deserves a cleanup.
> > 
> > If it deserves a cleanup, then this should happen _BEFORE_ we add new
> > functionality to it.
> > 
> > Thanks,
> > 
> > 	tglx
> > 
> 
> Hi Thomas, Daniel,
> 
> Sure this driver should have a cleanup, but it still depends on old legacy vexpress calls.
> 
> The reason I submitted a change and a small cleanup over this driver is because the high
> similarities of the HW and avoiding adding a brand new driver cloning much of its
> functionalities.

Hi Neil,

it is a good idea to avoid having new drivers when it is possible to group 
them into a single one.
 
> I propose to study an eventual cleanup, but could this change be submitted for 4.7 since
> it's part of a base platform support serie ?

I am not against if the next patches you send are cleanups.

Thomas ?

[PATCH 1/2] clocksource: sp804: Add support for OX810SE 24bit timer width

[Neil Armstrong]

On 04/26/2016 06:30 PM, Daniel Lezcano wrote:
> On Mon, Apr 25, 2016 at 04:48:24PM +0200, Neil Armstrong wrote:
>> On 04/22/2016 09:53 AM, Thomas Gleixner wrote:
>>> On Fri, 22 Apr 2016, Daniel Lezcano wrote:
>>>
>>>> On Fri, Apr 01, 2016 at 04:22:38PM +0200, Neil Armstrong wrote:
>>>>> In order to support the Dual-Timer on the Oxford Semiconductor OX810SE SoC,
>>>>> implement variable counter width, keeping 32bit as default width.
>>>>> Add new compatible string oxsemi,ox810se-rps-timer in order to select
>>>>> the 24bit counter width.
>>>>>
>>>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>>>> ---
>>>>>  drivers/clocksource/timer-sp804.c | 107 ++++++++++++++++++++++++--------------
>>>>>  include/clocksource/timer-sp804.h |  42 ++++++++++++---
>>>>>  2 files changed, 102 insertions(+), 47 deletions(-)
>>>>
>>>> I will take those patches but this driver really deserves a cleanup.
>>>
>>> If it deserves a cleanup, then this should happen _BEFORE_ we add new
>>> functionality to it.
>>>
>>> Thanks,
>>>
>>> 	tglx
>>>
>>
>> Hi Thomas, Daniel,
>>
>> Sure this driver should have a cleanup, but it still depends on old legacy vexpress calls.
>>
>> The reason I submitted a change and a small cleanup over this driver is because the high
>> similarities of the HW and avoiding adding a brand new driver cloning much of its
>> functionalities.
> 
> Hi Neil,
> 
> it is a good idea to avoid having new drivers when it is possible to group 
> them into a single one.
>  
>> I propose to study an eventual cleanup, but could this change be submitted for 4.7 since
>> it's part of a base platform support serie ?
> 
> I am not against if the next patches you send are cleanups.
> 
> Thomas ?
> 

Hi Thomas,

Is the proposition acceptable ? When this patchset will hit 4.7, I will work on a rework for 4.8.

Thanks,
Neil

[PATCH 2/2] dt-bindings: timer: sp804: add new compatible for OX810SE SoC

[Neil Armstrong]

Add new oxsemi,ox810se-rps-timer compatible string to support the
Oxford Semiconductor OX810SE SoC Dual Timers variant.

Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 Documentation/devicetree/bindings/timer/arm,sp804.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/timer/arm,sp804.txt b/Documentation/devicetree/bindings/timer/arm,sp804.txt
index 5cd8eee7..ebe8753 100644
--- a/Documentation/devicetree/bindings/timer/arm,sp804.txt
+++ b/Documentation/devicetree/bindings/timer/arm,sp804.txt
@@ -2,7 +2,7 @@ ARM sp804 Dual Timers
 ---------------------------------------
 
 Required properties:
-- compatible: Should be "arm,sp804" & "arm,primecell"
+- compatible: Should be "arm,sp804" & "arm,primecell", or "oxsemi,ox810se-rps-timer"
 - interrupts: Should contain the list of Dual Timer interrupts. This is the
 	interrupt for timer 1 and timer 2. In the case of a single entry, it is
 	the combined interrupt or if "arm,sp804-has-irq" is present that
-- 
1.9.1