[PATCH] clocksource: moxart: Add AST2500 compatible string

[Andrew Jeffery]

Also clean up space-before-tab issues in the documentation.

Signed-off-by: Andrew Jeffery <***@aj.id.au>
---
Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt | 5 +++--
drivers/clocksource/moxart_timer.c | 3 ++-
2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt b/Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt
index e207c11630af..b6b8b4836d3b 100644
--- a/Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt
+++ b/Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt
@@ -3,8 +3,9 @@ MOXA ART timer
Required properties:

- compatible : Must be one of:
- - "moxa,moxart-timer"
- - "aspeed,ast2400-timer"
+ - "moxa,moxart-timer"
+ - "aspeed,ast2400-timer"
+ - "aspeed,ast2500-timer"
- reg : Should contain registers location and length
- interrupts : Should contain the timer interrupt number
- clocks : Should contain phandle for the clock that drives the counter
diff --git a/drivers/clocksource/moxart_timer.c b/drivers/clocksource/moxart_timer.c
index 7f3430654fbd..b6fa319396c1 100644
--- a/drivers/clocksource/moxart_timer.c
+++ b/drivers/clocksource/moxart_timer.c
@@ -253,4 +253,5 @@ static int __init moxart_timer_init(struct device_node *node)
return ret;
}
CLOCKSOURCE_OF_DECLARE(moxart, "moxa,moxart-timer", moxart_timer_init);
-CLOCKSOURCE_OF_DECLARE(aspeed, "aspeed,ast2400-timer", moxart_timer_init);
+CLOCKSOURCE_OF_DECLARE(ast2400, "aspeed,ast2400-timer", moxart_timer_init);
+CLOCKSOURCE_OF_DECLARE(ast2500, "aspeed,ast2500-timer", moxart_timer_init);

[Joel Stanley]

Post by Andrew Jeffery
Also clean up space-before-tab issues in the documentation.
---
Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt | 5 +++--
drivers/clocksource/moxart_timer.c | 3 ++-
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt b/Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt
index e207c11630af..b6b8b4836d3b 100644
--- a/Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt
+++ b/Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt
@@ -3,8 +3,9 @@ MOXA ART timer
- - "moxa,moxart-timer"
- - "aspeed,ast2400-timer"
+ - "moxa,moxart-timer"
+ - "aspeed,ast2400-timer"
+ - "aspeed,ast2500-timer"
- reg : Should contain registers location and length
- interrupts : Should contain the timer interrupt number
- clocks : Should contain phandle for the clock that drives the counter
diff --git a/drivers/clocksource/moxart_timer.c b/drivers/clocksource/moxart_timer.c
index 7f3430654fbd..b6fa319396c1 100644
--- a/drivers/clocksource/moxart_timer.c
+++ b/drivers/clocksource/moxart_timer.c
@@ -253,4 +253,5 @@ static int __init moxart_timer_init(struct device_node *node)
return ret;
}
CLOCKSOURCE_OF_DECLARE(moxart, "moxa,moxart-timer", moxart_timer_init);
-CLOCKSOURCE_OF_DECLARE(aspeed, "aspeed,ast2400-timer", moxart_timer_init);
+CLOCKSOURCE_OF_DECLARE(ast2400, "aspeed,ast2400-timer", moxart_timer_init);
+CLOCKSOURCE_OF_DECLARE(ast2500, "aspeed,ast2500-timer", moxart_timer_init);
--
2.9.3

[Rob Herring]

Post by Andrew Jeffery
Also clean up space-before-tab issues in the documentation.
---
Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt | 5 +++--
drivers/clocksource/moxart_timer.c | 3 ++-
2 files changed, 5 insertions(+), 3 deletions(-)

Acked-by: Rob Herring <***@kernel.org>

[Daniel Lezcano]

Post by Andrew Jeffery
Also clean up space-before-tab issues in the documentation.

Andrew,

I reworked the patch to apply to the changes Linus did recently to convert to
the fttrm010 driver.

Please have a look at:

https://git.linaro.org/people/daniel.lezcano/linux.git/commit/?h=clockevents/4.13&id=3ca904162ffdd72f4fad3ab731fc94a12c50f682

Shouldn't the compatible string be:

"aspeed,ast2400-timer", "faraday,fttmr010"
"aspeed,ast2500-timer", "faraday,fttmr010"


-- Daniel

[Andrew Jeffery]

Post by Daniel Lezcano

Post by Andrew Jeffery
Also clean up space-before-tab issues in the documentation.

Andrew,
I reworked the patch to apply to the changes Linus did recently to convert to
the fttrm010 driver.
https://git.linaro.org/people/daniel.lezcano/linux.git/commit/?h=clockevents/4.13&id=3ca904162ffdd72f4fad3ab731fc94a12c50f682

I think we're going to run into trouble here:

https://git.linaro.org/people/daniel.lezcano/linux.git/tree/drivers/clocksource/timer-fttmr010.c?h=clockevents/4.13&id=3ca904162ffdd72f4fad3ab731fc94a12c50f682#n260

As it stands if a aspeed,ast2500-timer compatible is provided we'll
take the else branch and hit the issues Joel found with Linus' original
series counting up on the Aspeed hardware.

My change was somewhat cosmetic - Ben (now Cc'ed) didn't seemed too
concerned about using the the aspeed,ast2400-timer compatible string
for ast2500 dts. My motivation for the patch was that by describing the
aspeed,ast2500-timer compatible it signals that someone had taken a
look and judged it so. However, my point is maybe one solution is
simply to drop the patch and continue to use aspeed,ast2400-timer
compatible where we need.

Another is to rework your change to switch to
of_device_compatible_match() in drivers/clocksource/timer-fttmr010.c
and also check against aspeed,ast2500-timer.

What direction should we go?

Post by Daniel Lezcano
"aspeed,ast2400-timer", "faraday,fttmr010"
"aspeed,ast2500-timer", "faraday,fttmr010"

Does it makes sense in the face of the Aspeed quirks? If so it seems
reasonable, but falling back to the faraday,fttmr010 compatible could
lead to failures (if the compatible driver counted up).

Cheers,

Andrew

Post by Daniel Lezcano
  -- Daniel

[Daniel Lezcano]

The recent changes made the fttmr010 to be more generic and support different
timers with a very few differences like moxart or aspeed.

The aspeed timer uses a countdown and there is a test against the aspeed2400
compatible string to set a flag.

With the previous patch, we added the aspeed2500 compatible string but without
taking care of setting the countdown flag.

Fix this by specifiying a init function and pass the aspeed flag to a common
init function.

Signed-off-by: Daniel Lezcano <***@linaro.org>
---
drivers/clocksource/timer-fttmr010.c | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/clocksource/timer-fttmr010.c b/drivers/clocksource/timer-fttmr010.c
index 68982ad..d96190e 100644
--- a/drivers/clocksource/timer-fttmr010.c
+++ b/drivers/clocksource/timer-fttmr010.c
@@ -210,10 +210,9 @@ static irqreturn_t fttmr010_timer_interrupt(int irq, void *dev_id)
return IRQ_HANDLED;
}

-static int __init fttmr010_timer_init(struct device_node *np)
+static int __init fttmr010_common_init(struct device_node *np, bool is_aspeed)
{
struct fttmr010 *fttmr010;
- bool is_ast2400;
int irq;
struct clk *clk;
int ret;
@@ -260,8 +259,7 @@ static int __init fttmr010_timer_init(struct device_node *np)
* The Aspeed AST2400 moves bits around in the control register,
* otherwise it works the same.
*/
- is_ast2400 = of_device_is_compatible(np, "aspeed,ast2400-timer");
- if (is_ast2400) {
+ if (is_aspeed) {
fttmr010->t1_enable_val = TIMER_1_CR_ASPEED_ENABLE |
TIMER_1_CR_ASPEED_INT;
/* Downward not available */
@@ -280,7 +278,7 @@ static int __init fttmr010_timer_init(struct device_node *np)
* Enable timer 1 count up, timer 2 count up, except on Aspeed,
* where everything just counts down.
*/
- if (is_ast2400)
+ if (is_aspeed)
val = TIMER_2_CR_ASPEED_ENABLE;
else {
val = TIMER_2_CR_ENABLE;
@@ -355,8 +353,19 @@ static int __init fttmr010_timer_init(struct device_node *np)

return ret;
}
+
+static __init int aspeed_timer_init(struct device_node *np)
+{
+ return fttmr010_common_init(np, true);
+}
+
+static __init int fttmr010_timer_init(struct device_node *np)
+{
+ return fttmr010_common_init(np, false);
+}
+
CLOCKSOURCE_OF_DECLARE(fttmr010, "faraday,fttmr010", fttmr010_timer_init);
CLOCKSOURCE_OF_DECLARE(gemini, "cortina,gemini-timer", fttmr010_timer_init);
CLOCKSOURCE_OF_DECLARE(moxart, "moxa,moxart-timer", fttmr010_timer_init);
-CLOCKSOURCE_OF_DECLARE(ast2400, "aspeed,ast2400-timer", fttmr010_timer_init);
-CLOCKSOURCE_OF_DECLARE(ast2500, "aspeed,ast2500-timer", fttmr010_timer_init);
+CLOCKSOURCE_OF_DECLARE(ast2400, "aspeed,ast2400-timer", aspeed_timer_init);
+CLOCKSOURCE_OF_DECLARE(ast2500, "aspeed,ast2500-timer", aspeed_timer_init);

[Linus Walleij]

On Fri, May 26, 2017 at 10:48 AM, Daniel Lezcano

Post by Daniel Lezcano
The recent changes made the fttmr010 to be more generic and support different
timers with a very few differences like moxart or aspeed.
The aspeed timer uses a countdown and there is a test against the aspeed2400
compatible string to set a flag.
With the previous patch, we added the aspeed2500 compatible string but without
taking care of setting the countdown flag.
Fix this by specifiying a init function and pass the aspeed flag to a common
init function.

Sorry for the mistake :(
I don't have the Aspeed systems myself but I bet this works.

Reviewed-by: Linus Walleij <***@linaro.org>

Yours,
Linus Walleij

[Andrew Jeffery]

Post by Daniel Lezcano
The recent changes made the fttmr010 to be more generic and support different
timers with a very few differences like moxart or aspeed.
The aspeed timer uses a countdown and there is a test against the aspeed2400
compatible string to set a flag.
With the previous patch, we added the aspeed2500 compatible string but without
taking care of setting the countdown flag.
Fix this by specifiying a init function and pass the aspeed flag to a common
init function.
---
 drivers/clocksource/timer-fttmr010.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/clocksource/timer-fttmr010.c b/drivers/clocksource/timer-fttmr010.c
index 68982ad..d96190e 100644
--- a/drivers/clocksource/timer-fttmr010.c
+++ b/drivers/clocksource/timer-fttmr010.c
@@ -210,10 +210,9 @@ static irqreturn_t fttmr010_timer_interrupt(int irq, void *dev_id)

  return IRQ_HANDLED;

 }
 
-static int __init fttmr010_timer_init(struct device_node *np)
+static int __init fttmr010_common_init(struct device_node *np, bool is_aspeed)
 {

  struct fttmr010 *fttmr010;
- bool is_ast2400;
  int irq;
  struct clk *clk;
  int ret;

@@ -260,8 +259,7 @@ static int __init fttmr010_timer_init(struct device_node *np)

   * The Aspeed AST2400 moves bits around in the control register,
   * otherwise it works the same.
   */
- is_ast2400 = of_device_is_compatible(np, "aspeed,ast2400-timer");
- if (is_ast2400) {
+ if (is_aspeed) {
  fttmr010->t1_enable_val = TIMER_1_CR_ASPEED_ENABLE |
  TIMER_1_CR_ASPEED_INT;
  /* Downward not available */

@@ -280,7 +278,7 @@ static int __init fttmr010_timer_init(struct device_node *np)

   * Enable timer 1 count up, timer 2 count up, except on Aspeed,
   * where everything just counts down.
   */
- if (is_ast2400)
+ if (is_aspeed)
  val = TIMER_2_CR_ASPEED_ENABLE;
  else {
  val = TIMER_2_CR_ENABLE;

@@ -355,8 +353,19 @@ static int __init fttmr010_timer_init(struct device_node *np)
 

  return ret;

 }
+
+static __init int aspeed_timer_init(struct device_node *np)
+{

+ return fttmr010_common_init(np, true);

+}
+
+static __init int fttmr010_timer_init(struct device_node *np)
+{

+ return fttmr010_common_init(np, false);

+}
+
 CLOCKSOURCE_OF_DECLARE(fttmr010, "faraday,fttmr010", fttmr010_timer_init);
 CLOCKSOURCE_OF_DECLARE(gemini, "cortina,gemini-timer", fttmr010_timer_init);
 CLOCKSOURCE_OF_DECLARE(moxart, "moxa,moxart-timer", fttmr010_timer_init);
-CLOCKSOURCE_OF_DECLARE(ast2400, "aspeed,ast2400-timer", fttmr010_timer_init);
-CLOCKSOURCE_OF_DECLARE(ast2500, "aspeed,ast2500-timer", fttmr010_timer_init);
+CLOCKSOURCE_OF_DECLARE(ast2400, "aspeed,ast2400-timer", aspeed_timer_init);
+CLOCKSOURCE_OF_DECLARE(ast2500, "aspeed,ast2500-timer", aspeed_timer_init);

[Daniel Lezcano]

Post by Daniel Lezcano
The recent changes made the fttmr010 to be more generic and support different
timers with a very few differences like moxart or aspeed.
The aspeed timer uses a countdown and there is a test against the aspeed2400
compatible string to set a flag.
With the previous patch, we added the aspeed2500 compatible string but without
taking care of setting the countdown flag.
Fix this by specifiying a init function and pass the aspeed flag to a common
init function.

Thanks for testing.

-- Daniel

[Joel Stanley]

On Mon, May 29, 2017 at 5:15 PM, Daniel Lezcano

Post by Daniel Lezcano

Post by Daniel Lezcano
The recent changes made the fttmr010 to be more generic and support different
timers with a very few differences like moxart or aspeed.
The aspeed timer uses a countdown and there is a test against the aspeed2400
compatible string to set a flag.
With the previous patch, we added the aspeed2500 compatible string but without
taking care of setting the countdown flag.
Fix this by specifiying a init function and pass the aspeed flag to a common
init function.

Thanks everyone.

Acked-by: Joel Stanley <***@jms.id.au>

Andrew, I think you had to fix up the clock device tree entries in the
Aspeed device tree. Can you please send me a patch for that?

Cheers,

Joel

Post by Daniel Lezcano
Thanks for testing.
-- Daniel
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[Linus Walleij]

On Thu, May 25, 2017 at 10:28 PM, Daniel Lezcano

Post by Daniel Lezcano
"aspeed,ast2400-timer", "faraday,fttmr010"
"aspeed,ast2500-timer", "faraday,fttmr010"

Actually not this time. The Gemini and Moxart timers are compatible with
the pure Faraday fttmr010 IP-block but Aspeed has played around with the
silicon IP so it is not compatible anymore.

Yours,
Linus Walleij

[Linus Walleij]

Post by Andrew Jeffery
Also clean up space-before-tab issues in the documentation.

Reviewed-by: Linus Walleij <***@linaro.org>

Does this collide with Daniel's 2500 patch?

Sorry for the mess, tell me if I need to fix something up :(

Yours,
Linus Walleij

[Daniel Lezcano]

Post by Linus Walleij

Post by Andrew Jeffery
Also clean up space-before-tab issues in the documentation.

Does this collide with Daniel's 2500 patch?
Sorry for the mess, tell me if I need to fix something up :(

It is ok, an usual collision change to be fixed when merging ;)

By the way, can you have a look at fttmr010_read_sched_clock to remove
the local_fttmr->count_down test?

Thanks.

-- Daniel

[Linus Walleij]

On Sun, May 28, 2017 at 8:27 PM, Daniel Lezcano

Post by Daniel Lezcano

Post by Linus Walleij

Post by Andrew Jeffery
Also clean up space-before-tab issues in the documentation.

Does this collide with Daniel's 2500 patch?
Sorry for the mess, tell me if I need to fix something up :(

It is ok, an usual collision change to be fixed when merging ;)
By the way, can you have a look at fttmr010_read_sched_clock to remove
the local_fttmr->count_down test?

I guess what we can do is make two different sched_clock()
callbacks: one for upward and one for downward counting.

Would you like an optimization like that?

(It makes sense.)

Yours,
Linus Walleij

[Daniel Lezcano]

Post by Linus Walleij
On Sun, May 28, 2017 at 8:27 PM, Daniel Lezcano

Post by Daniel Lezcano

Post by Linus Walleij

Post by Andrew Jeffery
Also clean up space-before-tab issues in the documentation.

Does this collide with Daniel's 2500 patch?
Sorry for the mess, tell me if I need to fix something up :(

It is ok, an usual collision change to be fixed when merging ;)
By the way, can you have a look at fttmr010_read_sched_clock to remove
the local_fttmr->count_down test?

I guess what we can do is make two different sched_clock()
callbacks: one for upward and one for downward counting.
Would you like an optimization like that?
(It makes sense.)

Yes, thanks Linus!

-- Daniel

[Linus Walleij]

On Tue, May 30, 2017 at 10:54 AM, Daniel Lezcano

Post by Daniel Lezcano

Post by Linus Walleij
I guess what we can do is make two different sched_clock()
callbacks: one for upward and one for downward counting.
Would you like an optimization like that?
(It makes sense.)

Yes, thanks Linus!

Can you point me to a git tree with all your patches so I can work
on top of what you have?

I think I will also look into adding delay timers.

Yours,
Linus Walleij

[Daniel Lezcano]

Post by Linus Walleij
On Tue, May 30, 2017 at 10:54 AM, Daniel Lezcano

Post by Daniel Lezcano

Post by Linus Walleij
I guess what we can do is make two different sched_clock()
callbacks: one for upward and one for downward counting.
Would you like an optimization like that?
(It makes sense.)

Yes, thanks Linus!

Can you point me to a git tree with all your patches so I can work
on top of what you have?
I think I will also look into adding delay timers.

https://git.linaro.org/people/daniel.lezcano/linux.git clockevents/4.13

(subject to rebase)