[PATCH v2 0/4] clocksource: Add ast2600 support to fttmr010

[PATCH v2 0/4] clocksource: Add ast2600 support to fttmr010

[Joel Stanley]

This series adds support for the AST2600 timer.

v2 adds r-b tags from Rob, Linus and Cédric (who reviewed the patches on the
openbmc mailing list[1]). I made two small naming changes in this
version that were suggested in review.

[1] https://patchwork.ozlabs.org/project/openbmc/list/?series=140990

Joel Stanley (4):
  clocksource: fttmr010: Parametrise shutdown
  clocksource: fttmr010: Set interrupt and shutdown
  clocksource: fttmr010: Add support for ast2600
  dt-bindings: fttmr010: Add ast2600 compatible

 .../bindings/timer/faraday,fttmr010.txt       |  1 +
 drivers/clocksource/timer-fttmr010.c          | 68 +++++++++++++++----
 2 files changed, 54 insertions(+), 15 deletions(-)

-- 
2.24.0.rc1

[PATCH v2 1/4] clocksource: fttmr010: Parametrise shutdown

[Joel Stanley]

In preparation for supporting the ast2600 which uses a different method
to clear bits in the control register, use a callback for performing the
shutdown sequence.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/clocksource/timer-fttmr010.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/clocksource/timer-fttmr010.c b/drivers/clocksource/timer-fttmr010.c
index fadff7915dd9..c2d30eb9dc72 100644
--- a/drivers/clocksource/timer-fttmr010.c
+++ b/drivers/clocksource/timer-fttmr010.c
@@ -97,6 +97,7 @@ struct fttmr010 {
 	bool is_aspeed;
 	u32 t1_enable_val;
 	struct clock_event_device clkevt;
+	int (*timer_shutdown)(struct clock_event_device *evt);
 #ifdef CONFIG_ARM
 	struct delay_timer delay_timer;
 #endif
@@ -140,9 +141,7 @@ static int fttmr010_timer_set_next_event(unsigned long cycles,
 	u32 cr;
 
 	/* Stop */
-	cr = readl(fttmr010->base + TIMER_CR);
-	cr &= ~fttmr010->t1_enable_val;
-	writel(cr, fttmr010->base + TIMER_CR);
+	fttmr010->timer_shutdown(evt);
 
 	if (fttmr010->is_aspeed) {
 		/*
@@ -183,9 +182,7 @@ static int fttmr010_timer_set_oneshot(struct clock_event_device *evt)
 	u32 cr;
 
 	/* Stop */
-	cr = readl(fttmr010->base + TIMER_CR);
-	cr &= ~fttmr010->t1_enable_val;
-	writel(cr, fttmr010->base + TIMER_CR);
+	fttmr010->timer_shutdown(evt);
 
 	/* Setup counter start from 0 or ~0 */
 	writel(0, fttmr010->base + TIMER1_COUNT);
@@ -211,9 +208,7 @@ static int fttmr010_timer_set_periodic(struct clock_event_device *evt)
 	u32 cr;
 
 	/* Stop */
-	cr = readl(fttmr010->base + TIMER_CR);
-	cr &= ~fttmr010->t1_enable_val;
-	writel(cr, fttmr010->base + TIMER_CR);
+	fttmr010->timer_shutdown(evt);
 
 	/* Setup timer to fire at 1/HZ intervals. */
 	if (fttmr010->is_aspeed) {
@@ -350,6 +345,8 @@ static int __init fttmr010_common_init(struct device_node *np, bool is_aspeed)
 				     fttmr010->tick_rate);
 	}
 
+	fttmr010->timer_shutdown = fttmr010_timer_shutdown;
+
 	/*
 	 * Setup clockevent timer (interrupt-driven) on timer 1.
 	 */
@@ -370,10 +367,10 @@ static int __init fttmr010_common_init(struct device_node *np, bool is_aspeed)
 	fttmr010->clkevt.features = CLOCK_EVT_FEAT_PERIODIC |
 		CLOCK_EVT_FEAT_ONESHOT;
 	fttmr010->clkevt.set_next_event = fttmr010_timer_set_next_event;
-	fttmr010->clkevt.set_state_shutdown = fttmr010_timer_shutdown;
+	fttmr010->clkevt.set_state_shutdown = fttmr010->timer_shutdown;
 	fttmr010->clkevt.set_state_periodic = fttmr010_timer_set_periodic;
 	fttmr010->clkevt.set_state_oneshot = fttmr010_timer_set_oneshot;
-	fttmr010->clkevt.tick_resume = fttmr010_timer_shutdown;
+	fttmr010->clkevt.tick_resume = fttmr010->timer_shutdown;
 	fttmr010->clkevt.cpumask = cpumask_of(0);
 	fttmr010->clkevt.irq = irq;
 	clockevents_config_and_register(&fttmr010->clkevt,
-- 
2.24.0.rc1

Re: [PATCH v2 1/4] clocksource: fttmr010: Parametrise shutdown

[Daniel Lezcano]

On 07/11/2019 10:42, Joel Stanley wrote:
> In preparation for supporting the ast2600 which uses a different method
> to clear bits in the control register, use a callback for performing the
> shutdown sequence.
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Joel Stanley <joel@jms.id.au>

It will be cleaner if you create a struct of_device_id array where you
store the different variant data.

eg.
struct myops {
	int (*shutdown)(struct clock_event_device *evt);
};

struct fttmr010 {
	...
	struct myops *ops;
};

...

static const struct of_device_id fttmr010_of_match[] = {
	{ .compatible = "faraday,fttmr010",     .data = &fttmr010_ops },
	...
	{ .compatible = "aspeed,ast2600-timer", .data = &as2600_ops, },
	{ /* sentinel */ }
};

Keep the generic timer_shutdown function, get the ops from there and
then call the shutdown callback.

At init time:

...

const struct of_device_id *match;

...

match = of_match_node(fttmr010_of_match, node);
fttmr010->ops = (struct myops *)match->data;

...

(also if you have time, remove the is_aspeed boolean test by a
corresponding callback).

> ---
>  drivers/clocksource/timer-fttmr010.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-fttmr010.c b/drivers/clocksource/timer-fttmr010.c
> index fadff7915dd9..c2d30eb9dc72 100644
> --- a/drivers/clocksource/timer-fttmr010.c
> +++ b/drivers/clocksource/timer-fttmr010.c
> @@ -97,6 +97,7 @@ struct fttmr010 {
>  	bool is_aspeed;
>  	u32 t1_enable_val;
>  	struct clock_event_device clkevt;
> +	int (*timer_shutdown)(struct clock_event_device *evt);
>  #ifdef CONFIG_ARM
>  	struct delay_timer delay_timer;
>  #endif
> @@ -140,9 +141,7 @@ static int fttmr010_timer_set_next_event(unsigned long cycles,
>  	u32 cr;
>  
>  	/* Stop */
> -	cr = readl(fttmr010->base + TIMER_CR);
> -	cr &= ~fttmr010->t1_enable_val;
> -	writel(cr, fttmr010->base + TIMER_CR);
> +	fttmr010->timer_shutdown(evt);
>  
>  	if (fttmr010->is_aspeed) {
>  		/*
> @@ -183,9 +182,7 @@ static int fttmr010_timer_set_oneshot(struct clock_event_device *evt)
>  	u32 cr;
>  
>  	/* Stop */
> -	cr = readl(fttmr010->base + TIMER_CR);
> -	cr &= ~fttmr010->t1_enable_val;
> -	writel(cr, fttmr010->base + TIMER_CR);
> +	fttmr010->timer_shutdown(evt);
>  
>  	/* Setup counter start from 0 or ~0 */
>  	writel(0, fttmr010->base + TIMER1_COUNT);
> @@ -211,9 +208,7 @@ static int fttmr010_timer_set_periodic(struct clock_event_device *evt)
>  	u32 cr;
>  
>  	/* Stop */
> -	cr = readl(fttmr010->base + TIMER_CR);
> -	cr &= ~fttmr010->t1_enable_val;
> -	writel(cr, fttmr010->base + TIMER_CR);
> +	fttmr010->timer_shutdown(evt);
>  
>  	/* Setup timer to fire at 1/HZ intervals. */
>  	if (fttmr010->is_aspeed) {
> @@ -350,6 +345,8 @@ static int __init fttmr010_common_init(struct device_node *np, bool is_aspeed)
>  				     fttmr010->tick_rate);
>  	}
>  
> +	fttmr010->timer_shutdown = fttmr010_timer_shutdown;
> +
>  	/*
>  	 * Setup clockevent timer (interrupt-driven) on timer 1.
>  	 */
> @@ -370,10 +367,10 @@ static int __init fttmr010_common_init(struct device_node *np, bool is_aspeed)
>  	fttmr010->clkevt.features = CLOCK_EVT_FEAT_PERIODIC |
>  		CLOCK_EVT_FEAT_ONESHOT;
>  	fttmr010->clkevt.set_next_event = fttmr010_timer_set_next_event;
> -	fttmr010->clkevt.set_state_shutdown = fttmr010_timer_shutdown;
> +	fttmr010->clkevt.set_state_shutdown = fttmr010->timer_shutdown;
>  	fttmr010->clkevt.set_state_periodic = fttmr010_timer_set_periodic;
>  	fttmr010->clkevt.set_state_oneshot = fttmr010_timer_set_oneshot;
> -	fttmr010->clkevt.tick_resume = fttmr010_timer_shutdown;
> +	fttmr010->clkevt.tick_resume = fttmr010->timer_shutdown;
>  	fttmr010->clkevt.cpumask = cpumask_of(0);
>  	fttmr010->clkevt.irq = irq;
>  	clockevents_config_and_register(&fttmr010->clkevt,
> 


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

Re: [PATCH v2 1/4] clocksource: fttmr010: Parametrise shutdown

[Joel Stanley]

On Wed, 29 Jan 2020 at 11:25, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On 07/11/2019 10:42, Joel Stanley wrote:
> > In preparation for supporting the ast2600 which uses a different method
> > to clear bits in the control register, use a callback for performing the
> > shutdown sequence.
> >
> > Reviewed-by: Cédric Le Goater <clg@kaod.org>
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Joel Stanley <joel@jms.id.au>
>
> It will be cleaner if you create a struct of_device_id array where you
> store the different variant data.

I agree, and that's the path I would have taken when writing a normal
driver. However as the timer drivers probe with the
TIMER_OF_DECLARE/timer_probe infrastructure, we can't register our own
.data pointer (TIMER_OF_DECLARE uses .data  to store the _init
function).

Unless I'm missing something?

Cheers,

Joel


>
> eg.
> struct myops {
>         int (*shutdown)(struct clock_event_device *evt);
> };
>
> struct fttmr010 {
>         ...
>         struct myops *ops;
> };
>
> ...
>
> static const struct of_device_id fttmr010_of_match[] = {
>         { .compatible = "faraday,fttmr010",     .data = &fttmr010_ops },
>         ...
>         { .compatible = "aspeed,ast2600-timer", .data = &as2600_ops, },
>         { /* sentinel */ }
> };
>
> Keep the generic timer_shutdown function, get the ops from there and
> then call the shutdown callback.
>
> At init time:
>
> ...
>
> const struct of_device_id *match;
>
> ...
>
> match = of_match_node(fttmr010_of_match, node);
> fttmr010->ops = (struct myops *)match->data;
>
> ...
>
> (also if you have time, remove the is_aspeed boolean test by a
> corresponding callback).
>
> > ---
> >  drivers/clocksource/timer-fttmr010.c | 19 ++++++++-----------
> >  1 file changed, 8 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/clocksource/timer-fttmr010.c b/drivers/clocksource/timer-fttmr010.c
> > index fadff7915dd9..c2d30eb9dc72 100644
> > --- a/drivers/clocksource/timer-fttmr010.c
> > +++ b/drivers/clocksource/timer-fttmr010.c
> > @@ -97,6 +97,7 @@ struct fttmr010 {
> >       bool is_aspeed;
> >       u32 t1_enable_val;
> >       struct clock_event_device clkevt;
> > +     int (*timer_shutdown)(struct clock_event_device *evt);
> >  #ifdef CONFIG_ARM
> >       struct delay_timer delay_timer;
> >  #endif
> > @@ -140,9 +141,7 @@ static int fttmr010_timer_set_next_event(unsigned long cycles,
> >       u32 cr;
> >
> >       /* Stop */
> > -     cr = readl(fttmr010->base + TIMER_CR);
> > -     cr &= ~fttmr010->t1_enable_val;
> > -     writel(cr, fttmr010->base + TIMER_CR);
> > +     fttmr010->timer_shutdown(evt);
> >
> >       if (fttmr010->is_aspeed) {
> >               /*
> > @@ -183,9 +182,7 @@ static int fttmr010_timer_set_oneshot(struct clock_event_device *evt)
> >       u32 cr;
> >
> >       /* Stop */
> > -     cr = readl(fttmr010->base + TIMER_CR);
> > -     cr &= ~fttmr010->t1_enable_val;
> > -     writel(cr, fttmr010->base + TIMER_CR);
> > +     fttmr010->timer_shutdown(evt);
> >
> >       /* Setup counter start from 0 or ~0 */
> >       writel(0, fttmr010->base + TIMER1_COUNT);
> > @@ -211,9 +208,7 @@ static int fttmr010_timer_set_periodic(struct clock_event_device *evt)
> >       u32 cr;
> >
> >       /* Stop */
> > -     cr = readl(fttmr010->base + TIMER_CR);
> > -     cr &= ~fttmr010->t1_enable_val;
> > -     writel(cr, fttmr010->base + TIMER_CR);
> > +     fttmr010->timer_shutdown(evt);
> >
> >       /* Setup timer to fire at 1/HZ intervals. */
> >       if (fttmr010->is_aspeed) {
> > @@ -350,6 +345,8 @@ static int __init fttmr010_common_init(struct device_node *np, bool is_aspeed)
> >                                    fttmr010->tick_rate);
> >       }
> >
> > +     fttmr010->timer_shutdown = fttmr010_timer_shutdown;
> > +
> >       /*
> >        * Setup clockevent timer (interrupt-driven) on timer 1.
> >        */
> > @@ -370,10 +367,10 @@ static int __init fttmr010_common_init(struct device_node *np, bool is_aspeed)
> >       fttmr010->clkevt.features = CLOCK_EVT_FEAT_PERIODIC |
> >               CLOCK_EVT_FEAT_ONESHOT;
> >       fttmr010->clkevt.set_next_event = fttmr010_timer_set_next_event;
> > -     fttmr010->clkevt.set_state_shutdown = fttmr010_timer_shutdown;
> > +     fttmr010->clkevt.set_state_shutdown = fttmr010->timer_shutdown;
> >       fttmr010->clkevt.set_state_periodic = fttmr010_timer_set_periodic;
> >       fttmr010->clkevt.set_state_oneshot = fttmr010_timer_set_oneshot;
> > -     fttmr010->clkevt.tick_resume = fttmr010_timer_shutdown;
> > +     fttmr010->clkevt.tick_resume = fttmr010->timer_shutdown;
> >       fttmr010->clkevt.cpumask = cpumask_of(0);
> >       fttmr010->clkevt.irq = irq;
> >       clockevents_config_and_register(&fttmr010->clkevt,
> >
>
>
> --
>  <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
>

Re: [PATCH v2 1/4] clocksource: fttmr010: Parametrise shutdown

[Daniel Lezcano]

On 29/01/2020 12:41, Joel Stanley wrote:
> On Wed, 29 Jan 2020 at 11:25, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>
>> On 07/11/2019 10:42, Joel Stanley wrote:
>>> In preparation for supporting the ast2600 which uses a different method
>>> to clear bits in the control register, use a callback for performing the
>>> shutdown sequence.
>>>
>>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>>
>> It will be cleaner if you create a struct of_device_id array where you
>> store the different variant data.
> 
> I agree, and that's the path I would have taken when writing a normal
> driver. However as the timer drivers probe with the
> TIMER_OF_DECLARE/timer_probe infrastructure, we can't register our own
> .data pointer (TIMER_OF_DECLARE uses .data  to store the _init
> function).
> 
> Unless I'm missing something?

I was suggesting to add the array like:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clocksource/timer-atmel-tcb.c#n351

TIMER_OF_DECLARE is used and the probe functions relies on the match array:

match = of_match_node(atmel_tcb_of_match, node->parent);

(note in this case, it is the node parent).

>>
>> eg.
>> struct myops {
>>         int (*shutdown)(struct clock_event_device *evt);
>> };
>>
>> struct fttmr010 {
>>         ...
>>         struct myops *ops;
>> };
>>
>> ...
>>
>> static const struct of_device_id fttmr010_of_match[] = {
>>         { .compatible = "faraday,fttmr010",     .data = &fttmr010_ops },
>>         ...
>>         { .compatible = "aspeed,ast2600-timer", .data = &as2600_ops, },
>>         { /* sentinel */ }
>> };
>>
>> Keep the generic timer_shutdown function, get the ops from there and
>> then call the shutdown callback.
>>
>> At init time:
>>
>> ...
>>
>> const struct of_device_id *match;
>>
>> ...
>>
>> match = of_match_node(fttmr010_of_match, node);
>> fttmr010->ops = (struct myops *)match->data;
>>
>> ...
>>
>> (also if you have time, remove the is_aspeed boolean test by a
>> corresponding callback).
>>
>>> ---
>>>  drivers/clocksource/timer-fttmr010.c | 19 ++++++++-----------
>>>  1 file changed, 8 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/clocksource/timer-fttmr010.c b/drivers/clocksource/timer-fttmr010.c
>>> index fadff7915dd9..c2d30eb9dc72 100644
>>> --- a/drivers/clocksource/timer-fttmr010.c
>>> +++ b/drivers/clocksource/timer-fttmr010.c
>>> @@ -97,6 +97,7 @@ struct fttmr010 {
>>>       bool is_aspeed;
>>>       u32 t1_enable_val;
>>>       struct clock_event_device clkevt;
>>> +     int (*timer_shutdown)(struct clock_event_device *evt);
>>>  #ifdef CONFIG_ARM
>>>       struct delay_timer delay_timer;
>>>  #endif
>>> @@ -140,9 +141,7 @@ static int fttmr010_timer_set_next_event(unsigned long cycles,
>>>       u32 cr;
>>>
>>>       /* Stop */
>>> -     cr = readl(fttmr010->base + TIMER_CR);
>>> -     cr &= ~fttmr010->t1_enable_val;
>>> -     writel(cr, fttmr010->base + TIMER_CR);
>>> +     fttmr010->timer_shutdown(evt);
>>>
>>>       if (fttmr010->is_aspeed) {
>>>               /*
>>> @@ -183,9 +182,7 @@ static int fttmr010_timer_set_oneshot(struct clock_event_device *evt)
>>>       u32 cr;
>>>
>>>       /* Stop */
>>> -     cr = readl(fttmr010->base + TIMER_CR);
>>> -     cr &= ~fttmr010->t1_enable_val;
>>> -     writel(cr, fttmr010->base + TIMER_CR);
>>> +     fttmr010->timer_shutdown(evt);
>>>
>>>       /* Setup counter start from 0 or ~0 */
>>>       writel(0, fttmr010->base + TIMER1_COUNT);
>>> @@ -211,9 +208,7 @@ static int fttmr010_timer_set_periodic(struct clock_event_device *evt)
>>>       u32 cr;
>>>
>>>       /* Stop */
>>> -     cr = readl(fttmr010->base + TIMER_CR);
>>> -     cr &= ~fttmr010->t1_enable_val;
>>> -     writel(cr, fttmr010->base + TIMER_CR);
>>> +     fttmr010->timer_shutdown(evt);
>>>
>>>       /* Setup timer to fire at 1/HZ intervals. */
>>>       if (fttmr010->is_aspeed) {
>>> @@ -350,6 +345,8 @@ static int __init fttmr010_common_init(struct device_node *np, bool is_aspeed)
>>>                                    fttmr010->tick_rate);
>>>       }
>>>
>>> +     fttmr010->timer_shutdown = fttmr010_timer_shutdown;
>>> +
>>>       /*
>>>        * Setup clockevent timer (interrupt-driven) on timer 1.
>>>        */
>>> @@ -370,10 +367,10 @@ static int __init fttmr010_common_init(struct device_node *np, bool is_aspeed)
>>>       fttmr010->clkevt.features = CLOCK_EVT_FEAT_PERIODIC |
>>>               CLOCK_EVT_FEAT_ONESHOT;
>>>       fttmr010->clkevt.set_next_event = fttmr010_timer_set_next_event;
>>> -     fttmr010->clkevt.set_state_shutdown = fttmr010_timer_shutdown;
>>> +     fttmr010->clkevt.set_state_shutdown = fttmr010->timer_shutdown;
>>>       fttmr010->clkevt.set_state_periodic = fttmr010_timer_set_periodic;
>>>       fttmr010->clkevt.set_state_oneshot = fttmr010_timer_set_oneshot;
>>> -     fttmr010->clkevt.tick_resume = fttmr010_timer_shutdown;
>>> +     fttmr010->clkevt.tick_resume = fttmr010->timer_shutdown;
>>>       fttmr010->clkevt.cpumask = cpumask_of(0);
>>>       fttmr010->clkevt.irq = irq;
>>>       clockevents_config_and_register(&fttmr010->clkevt,
>>>
>>
>>
>> --
>>  <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
>>


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

Re: [PATCH v2 1/4] clocksource: fttmr010: Parametrise shutdown

[Joel Stanley]

On Wed, 29 Jan 2020 at 11:53, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On 29/01/2020 12:41, Joel Stanley wrote:
> > On Wed, 29 Jan 2020 at 11:25, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 07/11/2019 10:42, Joel Stanley wrote:
> >>> In preparation for supporting the ast2600 which uses a different method
> >>> to clear bits in the control register, use a callback for performing the
> >>> shutdown sequence.
> >>>
> >>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> >>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> >>> Signed-off-by: Joel Stanley <joel@jms.id.au>
> >>
> >> It will be cleaner if you create a struct of_device_id array where you
> >> store the different variant data.
> >
> > I agree, and that's the path I would have taken when writing a normal
> > driver. However as the timer drivers probe with the
> > TIMER_OF_DECLARE/timer_probe infrastructure, we can't register our own
> > .data pointer (TIMER_OF_DECLARE uses .data  to store the _init
> > function).
> >
> > Unless I'm missing something?
>
> I was suggesting to add the array like:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clocksource/timer-atmel-tcb.c#n351
>
> TIMER_OF_DECLARE is used and the probe functions relies on the match array:
>
> match = of_match_node(atmel_tcb_of_match, node->parent);

I see. So we don't use this table for probing the driver, just getting
the associated data.

I'm not convinced that's an improvement over what we have. If you have
a strong preference I can try it out.

[PATCH v2 2/4] clocksource: fttmr010: Set interrupt and shutdown

[Joel Stanley]

In preparation for supporting the ast2600, pass the shutdown and
interrupt functions to the common init callback.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Joel Stanley <joel@jms.id.au>
--
v2: call parameter 'irq_handler' instead of 'handler'
---
 drivers/clocksource/timer-fttmr010.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/clocksource/timer-fttmr010.c b/drivers/clocksource/timer-fttmr010.c
index c2d30eb9dc72..7c20a3debd96 100644
--- a/drivers/clocksource/timer-fttmr010.c
+++ b/drivers/clocksource/timer-fttmr010.c
@@ -244,7 +244,10 @@ static irqreturn_t fttmr010_timer_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static int __init fttmr010_common_init(struct device_node *np, bool is_aspeed)
+static int __init fttmr010_common_init(struct device_node *np,
+		bool is_aspeed,
+		int (*timer_shutdown)(struct clock_event_device *),
+		irq_handler_t irq_handler)
 {
 	struct fttmr010 *fttmr010;
 	int irq;
@@ -345,7 +348,7 @@ static int __init fttmr010_common_init(struct device_node *np, bool is_aspeed)
 				     fttmr010->tick_rate);
 	}
 
-	fttmr010->timer_shutdown = fttmr010_timer_shutdown;
+	fttmr010->timer_shutdown = timer_shutdown;
 
 	/*
 	 * Setup clockevent timer (interrupt-driven) on timer 1.
@@ -354,7 +357,7 @@ static int __init fttmr010_common_init(struct device_node *np, bool is_aspeed)
 	writel(0, fttmr010->base + TIMER1_LOAD);
 	writel(0, fttmr010->base + TIMER1_MATCH1);
 	writel(0, fttmr010->base + TIMER1_MATCH2);
-	ret = request_irq(irq, fttmr010_timer_interrupt, IRQF_TIMER,
+	ret = request_irq(irq, irq_handler, IRQF_TIMER,
 			  "FTTMR010-TIMER1", &fttmr010->clkevt);
 	if (ret) {
 		pr_err("FTTMR010-TIMER1 no IRQ\n");
@@ -403,12 +406,16 @@ static int __init fttmr010_common_init(struct device_node *np, bool is_aspeed)
 
 static __init int aspeed_timer_init(struct device_node *np)
 {
-	return fttmr010_common_init(np, true);
+	return fttmr010_common_init(np, true,
+			fttmr010_timer_shutdown,
+			fttmr010_timer_interrupt);
 }
 
 static __init int fttmr010_timer_init(struct device_node *np)
 {
-	return fttmr010_common_init(np, false);
+	return fttmr010_common_init(np, false,
+			fttmr010_timer_shutdown,
+			fttmr010_timer_interrupt);
 }
 
 TIMER_OF_DECLARE(fttmr010, "faraday,fttmr010", fttmr010_timer_init);
-- 
2.24.0.rc1

[PATCH v2 3/4] clocksource: fttmr010: Add support for ast2600

[Joel Stanley]

The ast2600 has some minor differences to previous versions. The
interrupt handler must acknowledge the timer interrupt in a status
register. Secondly the control register becomes write to set only,
requiring the use of a separate set to clear register.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Joel Stanley <joel@jms.id.au>
--
v2: Add ast2600 prefix to define to make it clear it's for ast2600 only
---
 drivers/clocksource/timer-fttmr010.c | 34 ++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/clocksource/timer-fttmr010.c b/drivers/clocksource/timer-fttmr010.c
index 7c20a3debd96..1510ee106e8d 100644
--- a/drivers/clocksource/timer-fttmr010.c
+++ b/drivers/clocksource/timer-fttmr010.c
@@ -37,6 +37,11 @@
 #define TIMER3_MATCH2		(0x2c)
 #define TIMER_CR		(0x30)
 
+/*
+  Control register set to clear for ast2600 only.
+ */
+#define AST2600_TIMER_CR_CLR	(0x3c)
+
 /*
  * Control register (TMC30) bit fields for fttmr010/gemini/moxart timers.
  */
@@ -163,6 +168,16 @@ static int fttmr010_timer_set_next_event(unsigned long cycles,
 	return 0;
 }
 
+static int ast2600_timer_shutdown(struct clock_event_device *evt)
+{
+	struct fttmr010 *fttmr010 = to_fttmr010(evt);
+
+	/* Stop */
+	writel(fttmr010->t1_enable_val, fttmr010->base + AST2600_TIMER_CR_CLR);
+
+	return 0;
+}
+
 static int fttmr010_timer_shutdown(struct clock_event_device *evt)
 {
 	struct fttmr010 *fttmr010 = to_fttmr010(evt);
@@ -244,6 +259,17 @@ static irqreturn_t fttmr010_timer_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t ast2600_timer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = dev_id;
+	struct fttmr010 *fttmr010 = to_fttmr010(evt);
+
+	writel(0x1, fttmr010->base + TIMER_INTR_STATE);
+
+	evt->event_handler(evt);
+	return IRQ_HANDLED;
+}
+
 static int __init fttmr010_common_init(struct device_node *np,
 		bool is_aspeed,
 		int (*timer_shutdown)(struct clock_event_device *),
@@ -404,6 +430,13 @@ static int __init fttmr010_common_init(struct device_node *np,
 	return ret;
 }
 
+static __init int ast2600_timer_init(struct device_node *np)
+{
+	return fttmr010_common_init(np, true,
+			ast2600_timer_shutdown,
+			ast2600_timer_interrupt);
+}
+
 static __init int aspeed_timer_init(struct device_node *np)
 {
 	return fttmr010_common_init(np, true,
@@ -423,3 +456,4 @@ TIMER_OF_DECLARE(gemini, "cortina,gemini-timer", fttmr010_timer_init);
 TIMER_OF_DECLARE(moxart, "moxa,moxart-timer", fttmr010_timer_init);
 TIMER_OF_DECLARE(ast2400, "aspeed,ast2400-timer", aspeed_timer_init);
 TIMER_OF_DECLARE(ast2500, "aspeed,ast2500-timer", aspeed_timer_init);
+TIMER_OF_DECLARE(ast2600, "aspeed,ast2600-timer", ast2600_timer_init);
-- 
2.24.0.rc1

[PATCH v2 4/4] dt-bindings: fttmr010: Add ast2600 compatible

[Joel Stanley]

The ast2600 contains a fttmr010 derivative.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 Documentation/devicetree/bindings/timer/faraday,fttmr010.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/timer/faraday,fttmr010.txt b/Documentation/devicetree/bindings/timer/faraday,fttmr010.txt
index 195792270414..3cb2f4c98d64 100644
--- a/Documentation/devicetree/bindings/timer/faraday,fttmr010.txt
+++ b/Documentation/devicetree/bindings/timer/faraday,fttmr010.txt
@@ -11,6 +11,7 @@ Required properties:
   "moxa,moxart-timer", "faraday,fttmr010"
   "aspeed,ast2400-timer"
   "aspeed,ast2500-timer"
+  "aspeed,ast2600-timer"
 
 - reg : Should contain registers location and length
 - interrupts : Should contain the three timer interrupts usually with
-- 
2.24.0.rc1

Re: [PATCH v2 0/4] clocksource: Add ast2600 support to fttmr010

[Joel Stanley]

Hi Daniel, Thomas, I noticed that this series never made it into the
timer tree. Are you able to pick it up?

https://lore.kernel.org/lkml/20191107094218.13210-1-joel@jms.id.au/

Cheers,

Joel

On Thu, 7 Nov 2019 at 09:42, Joel Stanley <joel@jms.id.au> wrote:
>
> This series adds support for the AST2600 timer.
>
> v2 adds r-b tags from Rob, Linus and Cédric (who reviewed the patches on the
> openbmc mailing list[1]). I made two small naming changes in this
> version that were suggested in review.
>
> [1] https://patchwork.ozlabs.org/project/openbmc/list/?series=140990
>
> Joel Stanley (4):
>   clocksource: fttmr010: Parametrise shutdown
>   clocksource: fttmr010: Set interrupt and shutdown
>   clocksource: fttmr010: Add support for ast2600
>   dt-bindings: fttmr010: Add ast2600 compatible
>
>  .../bindings/timer/faraday,fttmr010.txt       |  1 +
>  drivers/clocksource/timer-fttmr010.c          | 68 +++++++++++++++----
>  2 files changed, 54 insertions(+), 15 deletions(-)
>
> --
> 2.24.0.rc1
>

Re: [PATCH v2 0/4] clocksource: Add ast2600 support to fttmr010

[Daniel Lezcano]

On 29/01/2020 11:30, Joel Stanley wrote:
> Hi Daniel, Thomas, I noticed that this series never made it into the
> timer tree. Are you able to pick it up?

Oops, missed it.

Incoming comments.

> https://lore.kernel.org/lkml/20191107094218.13210-1-joel@jms.id.au/
> 
> Cheers,
> 
> Joel
> 
> On Thu, 7 Nov 2019 at 09:42, Joel Stanley <joel@jms.id.au> wrote:
>>
>> This series adds support for the AST2600 timer.
>>
>> v2 adds r-b tags from Rob, Linus and Cédric (who reviewed the patches on the
>> openbmc mailing list[1]). I made two small naming changes in this
>> version that were suggested in review.
>>
>> [1] https://patchwork.ozlabs.org/project/openbmc/list/?series=140990
>>
>> Joel Stanley (4):
>>   clocksource: fttmr010: Parametrise shutdown
>>   clocksource: fttmr010: Set interrupt and shutdown
>>   clocksource: fttmr010: Add support for ast2600
>>   dt-bindings: fttmr010: Add ast2600 compatible
>>
>>  .../bindings/timer/faraday,fttmr010.txt       |  1 +
>>  drivers/clocksource/timer-fttmr010.c          | 68 +++++++++++++++----
>>  2 files changed, 54 insertions(+), 15 deletions(-)
>>
>> --
>> 2.24.0.rc1
>>


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