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

[Bartosz Golaszewski]

czw., 23 maj 2019 o 15:25 Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> napisaÅ(a):
>
>
> 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);
> }
>

Yes, this is pretty much what I did in v2.

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

I'll wait for your comments on v2 before changing it in the final submission.

Thanks,
Bart

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