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

[Bartosz Golaszewski]

sob., 25 maj 2019 o 16:16 Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> napisaÅ(a):
>
> On 24/05/2019 13:53, Bartosz Golaszewski wrote:
> > pt., 24 maj 2019 o 10:59 Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> napisaÅ(a):
> >>
> >> On 24/05/2019 09:28, Bartosz Golaszewski wrote:
> >>> czw., 23 maj 2019 o 18:38 Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> napisaÅ(a):
> >>>>
> >>>> On 23/05/2019 14:58, Bartosz Golaszewski wrote:
> >>>>> From: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> >>>>>
> >>>>> Currently the clocksource and clockevent support for davinci platforms
> >>>>> lives in mach-davinci. It hard-codes many things, uses global variables,
> >>>>> implements functionalities unused by any platform and has code fragments
> >>>>> scattered across many (often unrelated) files.
> >>>>>
> >>>>> Implement a new, modern and simplified timer driver and put it into
> >>>>> drivers/clocksource. We still need to support legacy board files so
> >>>>> export a config structure and a function that allows machine code to
> >>>>> register the timer.
> >>>>>
> >>>>> The timer we're using is 64-bit but can be programmed in dual 32-bit
> >>>>> mode (both chained and unchained). We're using dual 32-bit mode to
> >>>>> have separate counters for clockevents and clocksource.
> >>>>>
> >>>>> This patch contains the core code and support for clockevent. The
> >>>>> clocksource code will be included in a subsequent patch.
> >>>>>
>
> [ ... ]
>
> >>>>> +static unsigned int
> >>>>> +davinci_clockevent_read(struct davinci_clockevent *clockevent,
> >>>>> +                     unsigned int reg)
> >>>>> +{
> >>>>> +     return readl_relaxed(clockevent->base + reg);
> >>>>> +}
> >>>>> +
> >>>>> +static void davinci_clockevent_write(struct davinci_clockevent *clockevent,
> >>>>> +                                  unsigned int reg, unsigned int val)
> >>>>> +{
> >>>>> +     writel_relaxed(val, clockevent->base + reg);
> >>>>> +}
> >>>>> +
> >>>>> +static void davinci_tcr_update(void __iomem *base,
> >>>>> +                            unsigned int mask, unsigned int val)
> >>>>> +{
> >>>>> +     davinci_tcr &= ~mask;
> >>>>> +     davinci_tcr |= val & mask;
> >>>>
> >>>>
> >>>> I don't see when the davinci_tcr is initialized.
> >>>>
> >>>
> >>> It's set to 0x0 by the compiler and we're setting the register to 0x0
> >>> in davinci_timer_init().
> >>
> >> Why did you need to readl before in the previous version? The idea of
> >> caching the value was to save an extra readl.
> >>
> >> If it is always zero, then we don't need this variable neither the read,
> >> just doing:
> >>
> >> writel_relaxed(val & mask, base + DAVINCI_TIMER_REG_TCR);
> >>
> >> should work no ?
> >
> > It's not always zero. Its reset value is zero and we write 0 to it at
> > init time just to make sure, but then we modify it according to the
> > configuration. The single TCR register controls both halves of the
> > timer, so we do need an actual update, not a simple write.
>
> Ok but the driver can be oneshot or disabled in the code (mutually
> exclusive), no ?
>
> So doing
>
>  - writel(oneshot, base);
>  - writel(disabled, base);
>
> works without any mask computation, no?
>
> Well the above assumes other part of the register aren't changed by
> other subsystems (or by the timer itself).
>
>

I'm not sure I understand. You can be using two timers. Both
controlled by a single TCR register. In your example oneshot can equal
(0x00, or 0x01) and either be shifted left by 6 or 22 for TIM12 and
TIM34 respectively. If you do writel(oneshot-for-time12, base) you'll
set tim34 to disabled.

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
>