Re: [RFC 1/2] clocksource: davinci-timer: add support for clockevents

[Daniel Lezcano <daniel.lezcano@linaro.org>]

Hi Bartosz,



On 23/05/2019 14:58, Bartosz Golaszewski wrote:

[ ... ]

>>> +++ b/drivers/clocksource/timer-davinci.c
>>> @@ -0,0 +1,272 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +//
>>> +// TI DaVinci clocksource driver
>>> +//
>>> +// Copyright (C) 2019 Texas Instruments
>>> +// Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>> +// (with tiny parts adopted from code by Kevin Hilman <khilman@baylibre.com>)
>>
>> The header format is wrong, it should be:
>>
>> // SPDX-License-Identifier: GPL-2.0-only
>> /*
>>  * TI DaVinci clocksource driver
>>  *
>>  * ...
>>  * ...
>>  *
>>  */
> 
> It's not wrong. It looks like it's at the maintainers discretion and
> I've been asked to use both forms by different maintainers. Seems you
> just can't get it right. :) I've changed it in v2 though.

Right, I've been through the documentation but it is still unclear for
me. So let's stick to whatever you want for now.

[ ... ]

>>> +static int
>>> +davinci_clockevent_set_next_event_std(unsigned long cycles,
>>> +                                   struct clock_event_device *dev)
>>> +{
>>> +     struct davinci_clockevent *clockevent;
>>> +     unsigned int enamode;
>>> +
>>> +     clockevent = to_davinci_clockevent(dev);
>>> +     enamode = clockevent->enamode_disabled;
>>> +
>>> +     davinci_clockevent_update(clockevent, DAVINCI_TIMER_REG_TCR,
>>> +                               clockevent->enamode_mask,
>>> +                               clockevent->enamode_disabled);
>>
>> What is for this function with the DAVINCI_TIMER_REG_TCR parameter?
> 
> I'm not sure I understand the question. :(

I meant davinci_clockevent_update is always called with the
DAVINCI_TIMER_REG_TCR parameter.

So it can be changed to:
static void davinci_clockevent_update(struct davinci_clockevent 	
						*clockevent,
					unsigned int mask,
					unsigned int val)
{
	davinci_reg_update(clockevent->base, DAVINCI_TIMER_REG_TCR,
				 mask, val);
}


Alternatively davinci_clockevent_update can be replaced by a direct call
to davinci_reg_update.

[ ... ]

>>> +     clockevent->dev.cpumask = cpumask_of(0);
>>> +
>>> +     clockevent->base = base;
>>> +     clockevent->tim_off = DAVINCI_TIMER_REG_TIM12;
>>> +     clockevent->prd_off = DAVINCI_TIMER_REG_PRD12;
>>> +
>>> +     shift = DAVINCI_TIMER_ENAMODE_SHIFT_TIM12;
>>> +     clockevent->enamode_disabled = DAVINCI_TIMER_ENAMODE_DISABLED << shift;
>>> +     clockevent->enamode_oneshot = DAVINCI_TIMER_ENAMODE_ONESHOT << shift;
>>> +     clockevent->enamode_periodic = DAVINCI_TIMER_ENAMODE_PERIODIC << shift;
>>> +     clockevent->enamode_mask = DAVINCI_TIMER_ENAMODE_MASK << shift;
>>
>> I don't see where 'shift' can be different from TIM12 here neither in
>> the second patch for those values. Why create these fields instead of
>> pre-computed macros?
>>
> 
> The variable 'shift' here is only to avoid breaking the lines (just a helper).
> 
> The shift itself can be different though in the second patch -
> specifically when calling davinci_clocksource_init().
> 
> If I were to use predefined values for clockevent, we'd still need
> another set of values for clocksource. I think it's clearer the way it
> is.

Ah yes, I see, it is passed as parameter. Ok, let's keep it this way if
you prefer.

>>> +     if (timer_cfg->cmp_off) {
>>> +             clockevent->cmp_off = timer_cfg->cmp_off;
>>> +             clockevent->dev.set_next_event =
>>> +                             davinci_clockevent_set_next_event_cmp;
>>> +     } else {
>>> +             clockevent->dev.set_next_event =
>>> +                             davinci_clockevent_set_next_event_std;
>>> +     }
>>> +
>>> +     rv = request_irq(timer_cfg->irq[DAVINCI_TIMER_CLOCKEVENT_IRQ].start,
>>> +                      davinci_timer_irq_timer, IRQF_TIMER,
>>> +                      "clockevent", clockevent);
>>
>> May be replace "clockevent" by eg. "tim12"?
>>
> 
> I don't think this is a good idea. Now if you look at /proc/interrupts
> you can tell immediately what the interrupt is for ("clockevent").
> With "tim12" it's no longer that clear.

Yes, "tim12" can be confusing. However, it is good practice to add a
device name aside with its purpose in case there are several timers
defined on the system. "clockevent" is a kernel internal representation
of a timer, so may be a name like "timer/tim12" or something in the same
spirit would be more adequate.




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