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

From: Daniel Lezcano
Date: Thu May 23 2019 - 09:28:42 EST



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@xxxxxxxxxxxx>
>>> +// (with tiny parts adopted from code by Kevin Hilman <khilman@xxxxxxxxxxxx>)
>>
>> 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