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@xxxxxxxxxx> 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@xxxxxxxx>
>>> Reviewed-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
>>> Signed-off-by: Joel Stanley <joel@xxxxxxxxx>
>>
>> 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