Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1650800 > unrolled thread

Re: [PATCH] clocksource: moxart: Add AST2500 compatible string

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] clocksource: moxart: Add AST2500 compatible string Daniel Lezcano <daniel.lezcano@linaro.org> - 2017-05-25 22:30 +0200
    Re: [PATCH] clocksource: moxart: Add AST2500 compatible string Andrew Jeffery <andrew@aj.id.au> - 2017-05-26 03:20 +0200
      [PATCH] clocksource/drivers/fttmr010: Fix aspeed-2500 initialization Daniel Lezcano <daniel.lezcano@linaro.org> - 2017-05-26 10:50 +0200
        Re: [PATCH] clocksource/drivers/fttmr010: Fix aspeed-2500 initialization Linus Walleij <linus.walleij@linaro.org> - 2017-05-28 16:00 +0200
        Re: [PATCH] clocksource/drivers/fttmr010: Fix aspeed-2500  initialization Andrew Jeffery <andrew@aj.id.au> - 2017-05-29 08:10 +0200
          Re: [PATCH] clocksource/drivers/fttmr010: Fix aspeed-2500  initialization Daniel Lezcano <daniel.lezcano@linaro.org> - 2017-05-29 09:50 +0200
            Re: [PATCH] clocksource/drivers/fttmr010: Fix aspeed-2500 initialization Joel Stanley <joel@jms.id.au> - 2017-05-30 06:40 +0200
    Re: [PATCH] clocksource: moxart: Add AST2500 compatible string Linus Walleij <linus.walleij@linaro.org> - 2017-05-28 16:10 +0200

#1650800 — Re: [PATCH] clocksource: moxart: Add AST2500 compatible string

On Tue, May 16, 2017 at 03:58:40PM +0800, Andrew Jeffery wrote:
> 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

[toc] | [next] | [standalone]

#1651019

[Multipart message — attachments visible in raw view] — view raw

On Thu, 2017-05-25 at 22:28 +0200, Daniel Lezcano wrote:
> On Tue, May 16, 2017 at 03:58:40PM +0800, Andrew Jeffery wrote:
> > 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
> 

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?

> Shouldn't the compatible string be:
> 
> 	"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

> 
>   -- Daniel
> 

[toc] | [prev] | [next] | [standalone]

#1651219 — [PATCH] clocksource/drivers/fttmr010: Fix aspeed-2500 initialization

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

[toc] | [prev] | [next] | [standalone]

#1652128 — Re: [PATCH] clocksource/drivers/fttmr010: Fix aspeed-2500 initialization

On Fri, May 26, 2017 at 10:48 AM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:

> 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 <daniel.lezcano@linaro.org>

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

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

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1652243 — Re: [PATCH] clocksource/drivers/fttmr010: Fix aspeed-2500 initialization

[Multipart message — attachments visible in raw view] — view raw

On Fri, 2017-05-26 at 10:48 +0200, Daniel Lezcano wrote:
> 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 <daniel.lezcano@linaro.org>

Tested-by: Andrew Jeffery <andrew@aj.id.au>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

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

[toc] | [prev] | [next] | [standalone]

#1652259 — Re: [PATCH] clocksource/drivers/fttmr010: Fix aspeed-2500 initialization

On 29/05/2017 08:05, Andrew Jeffery wrote:
> On Fri, 2017-05-26 at 10:48 +0200, Daniel Lezcano wrote:
>> 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 <daniel.lezcano@linaro.org>
> 
> Tested-by: Andrew Jeffery <andrew@aj.id.au>
> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
> 

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

[toc] | [prev] | [next] | [standalone]

#1652804 — Re: [PATCH] clocksource/drivers/fttmr010: Fix aspeed-2500 initialization

On Mon, May 29, 2017 at 5:15 PM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On 29/05/2017 08:05, Andrew Jeffery wrote:
>> On Fri, 2017-05-26 at 10:48 +0200, Daniel Lezcano wrote:
>>> 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 <daniel.lezcano@linaro.org>
>>
>> Tested-by: Andrew Jeffery <andrew@aj.id.au>
>> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

Thanks everyone.

Acked-by: Joel Stanley <joel@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



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

[toc] | [prev] | [next] | [standalone]

#1652134

On Thu, May 25, 2017 at 10:28 PM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:

> Shouldn't the compatible string be:
>
>         "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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web