LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Bartosz Golaszewski <brgl@bgdev.pl>
To: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Sekhar Nori <nsekhar@ti.com>, Kevin Hilman <khilman@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	David Lechner <david@lechnology.com>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>
Subject: Re: [RFC 1/2] clocksource: davinci-timer: add support for clockevents
Date: Thu, 23 May 2019 17:34:20 +0200	[thread overview]
Message-ID: <CAMRc=Mf37TZZO00tbXhAyxzNBYx9C-farKNQ=9vmVWZY59KC2w@mail.gmail.com> (raw)
In-Reply-To: <ca00f49f-0fa2-1907-feac-ba798dce365b@linaro.org>

czw., 23 maj 2019 o 15:25 Daniel Lezcano <daniel.lezcano@linaro.org> 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@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);
> }
>

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
>

  reply	other threads:[~2019-05-23 15:34 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-17 14:47 [RFC 0/2] clocksource: davinci-timer: new driver Bartosz Golaszewski
2019-04-17 14:47 ` [RFC 1/2] clocksource: davinci-timer: add support for clockevents Bartosz Golaszewski
2019-05-14 19:55   ` Daniel Lezcano
2019-05-23 12:58     ` Bartosz Golaszewski
2019-05-23 13:25       ` Daniel Lezcano
2019-05-23 15:34         ` Bartosz Golaszewski [this message]
2019-04-17 14:47 ` [RFC 2/2] clocksource: timer-davinci: add support for clocksource Bartosz Golaszewski
2019-05-13  7:54 ` [RFC 0/2] clocksource: davinci-timer: new driver Bartosz Golaszewski
2019-05-13  8:04   ` Daniel Lezcano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAMRc=Mf37TZZO00tbXhAyxzNBYx9C-farKNQ=9vmVWZY59KC2w@mail.gmail.com' \
    --to=brgl@bgdev.pl \
    --cc=bgolaszewski@baylibre.com \
    --cc=daniel.lezcano@linaro.org \
    --cc=david@lechnology.com \
    --cc=khilman@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=nsekhar@ti.com \
    --cc=tglx@linutronix.de \
    --subject='Re: [RFC 1/2] clocksource: davinci-timer: add support for clockevents' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).