mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Antony Pavlov <antonynpavlov@gmail.com>
To: Lucas Stach <dev@lynxeye.de>
Cc: barebox@lists.infradead.org
Subject: Re: [PATCH 6/7] tegra: add proper timer driver
Date: Mon, 4 Mar 2013 23:14:26 +0400	[thread overview]
Message-ID: <CAA4bVAETDP3+s-j8Hpr+g4ZX2ZPjUuYrPoa_LZMsX8jztvqX6g@mail.gmail.com> (raw)
In-Reply-To: <1362416972.1957.3.camel@antimon>

On 4 March 2013 21:09, Lucas Stach <dev@lynxeye.de> wrote:
> Am Sonntag, den 03.03.2013, 11:07 +0400 schrieb Antony Pavlov:
>> On 3 March 2013 03:13, Lucas Stach <dev@lynxeye.de> wrote:
>> > Am Freitag, den 01.03.2013, 18:23 +0100 schrieb Sascha Hauer:
>> >> On Fri, Mar 01, 2013 at 10:22:52AM +0100, Lucas Stach wrote:
>> >> > Replace the ad-hoc clocksource implementation with a proper driver for
>> >> > the Tegra 20 timer. This driver is able to do the required hardware
>> >> > initialisation itself.
>> >> >
>> >> > +
>> >> > +static int tegra20_timer_probe(struct device_d *dev)
>> >> > +{
>> >> > +   struct clk *timer_clk;
>> >> > +   unsigned long rate;
>> >> > +
>> >> > +   /* use only one timer */
>> >> > +   if (timer_base)
>> >> > +           return -EBUSY;
>> >> > +
>> >> > +   timer_base = dev_request_mem_region(dev, 0);
>> >> > +   if (!timer_base) {
>> >> > +           dev_err(dev, "could not get memory region\n");
>> >> > +           return -ENODEV;
>> >> > +   }
>> >> > +
>> >> > +   timer_clk = clk_get(dev, NULL);
>> >> > +   if (!timer_clk) {
>> >> > +           dev_err(dev, "could not get clock\n");
>> >> > +           return -ENODEV;
>> >> > +   }
>> >> > +
>> >> > +   clk_enable(timer_clk);
>> >> > +
>> >> > +   /*
>> >> > +    * calibrate timer to run at 1MHz
>> >>
>> >> We don't need the timer to be running at a certain frequency, you can
>> >> just use clocks_calc_mult_shift to calculate the correct values from
>> >> whatever frequency.
>> >
>> > Other hardware blocks like the flow controller might assume the timer to
>> > be running at 1MHz. The timer and time register is named US (like usec)
>> > for a reason. It's the officially correct way to initialize this timer
>> > (as documented in the Tegra TRM).
>>
>> IMHO, then Jean-Christophe speaking about 'just use
>> clocks_calc_mult_shift'  he mean this part of your
>> arch/arm/mach-tegra/tegra20-timer.c:
>>
>> +static struct clocksource cs = {
>> +       .read   = tegra20_timer_cs_read,
>> +       .mask   = CLOCKSOURCE_MASK(32),
>> +       .mult   = 1000,
>> +};
>>
>> He want to say "please don't use fixed 'mult' value, use
>> clocks_calc_mult_shift to calculate it".
>>
>> Please try to examine existing clocksources.
>>
>> The command
>>    grep -R -A 5 "static struct clocksource.*=" arch/arm/
>> will show you some results like this
>>
>> arch/arm/mach-imx/clocksource.c:static struct clocksource cs = {
>> arch/arm/mach-imx/clocksource.c-        .read   = imx_clocksource_read,
>> arch/arm/mach-imx/clocksource.c-        .mask   = CLOCKSOURCE_MASK(32),
>> arch/arm/mach-imx/clocksource.c-        .shift  = 10,
>> arch/arm/mach-imx/clocksource.c-};
>>
>> or even like that
>> arch/arm/mach-clps711x/clock.c:static struct clocksource cs = {
>> arch/arm/mach-clps711x/clock.c- .read   = clocksource_read,
>> arch/arm/mach-clps711x/clock.c- .mask   = CLOCKSOURCE_MASK(16),
>> arch/arm/mach-clps711x/clock.c-};
>>
>> But I can't find any example of 'struct clocksource' definition with
>> fixed 'mult' value.
>>
>
> I've looked at other clocksource drivers before implementing the Tegra
> one and it's right that all other clocksources calculate the mult at
> runtime. But as the Tegra clocksource is guaranteed to run at 1MHz at
> every point in time I don't really see any benefit of calculating the
> mult over just having it as static init data.
>
> Is there anything I'm missing?

You use your knowlege about the clock rate twice:
 * then you actually set up the clock rate;
 * then you set up the clocksource device data structure (the values
of the 'shift' and the 'mult' fields).

But there is no any EXPLICIT relation between the clock rate and the
'shift' and 'mult' values; It is not clear and evident that if you
change the clock frequency you must change the 'mult' value too. There
are a techical means to change clock frequency in spite of the
recommended clock frequency is 1 MHz. Don't forget the quote from the
book 'Murphy's laws and corollaries':

   Anything that can go wrong will go wrong.

In the short-term your tricky realisation looks good, but in the
long-term it is a potential source of a problems.

-- 
Best regards,
  Antony Pavlov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

  reply	other threads:[~2013-03-04 19:14 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-03-01  9:22 [PATCH 0/7] Rework current Tegra support Lucas Stach
2013-03-01  9:22 ` [PATCH 1/7] tegra: move address map to tegra20-silicon.h Lucas Stach
2013-03-01 13:17   ` Jean-Christophe PLAGNIOL-VILLARD
2013-03-02 23:25     ` Lucas Stach
2013-03-04 19:26       ` Antony Pavlov
2013-03-01  9:22 ` [PATCH 2/7] tegra: remove debug_ll Lucas Stach
2013-03-01 13:17   ` Jean-Christophe PLAGNIOL-VILLARD
2013-03-01 17:19   ` Sascha Hauer
2013-03-01  9:22 ` [PATCH 3/7] tegra: switch to ARMv7 cpu type Lucas Stach
2013-03-01  9:22 ` [PATCH 4/7] tegra: add blank tegra20 platform init Lucas Stach
2013-03-01 13:16   ` Jean-Christophe PLAGNIOL-VILLARD
2013-03-01  9:22 ` [PATCH 5/7] tegra: add driver for the clock and reset module Lucas Stach
2013-03-01 17:26   ` Sascha Hauer
2013-03-02 23:16     ` Lucas Stach
2013-03-01  9:22 ` [PATCH 6/7] tegra: add proper timer driver Lucas Stach
2013-03-01 13:14   ` Jean-Christophe PLAGNIOL-VILLARD
2013-03-01 17:23   ` Sascha Hauer
2013-03-02 23:13     ` Lucas Stach
2013-03-03  7:07       ` Antony Pavlov
2013-03-04 17:09         ` Lucas Stach
2013-03-04 19:14           ` Antony Pavlov [this message]
2013-03-01  9:22 ` [PATCH 7/7] tegra: add power management controller driver Lucas Stach
2013-03-01 13:15   ` Jean-Christophe PLAGNIOL-VILLARD
2013-03-01 17:28     ` Sascha Hauer
2013-03-01 18:00       ` Antony Pavlov
2013-03-02 23:21         ` Lucas Stach
2013-03-07 10:33 ` [PATCH v2 0/5] Rework current Tegra support Lucas Stach
2013-03-07 10:33   ` [PATCH v2 1/5] tegra: pull in iomap.h from the Linux kernel Lucas Stach
2013-03-07 10:33   ` [PATCH v2 2/5] tegra: switch to ARMv7 cpu type Lucas Stach
2013-03-08  6:03     ` Antony Pavlov
2013-03-08 13:17       ` Lucas Stach
2013-03-08 16:56         ` Antony Pavlov
2013-03-08 14:32       ` Sascha Hauer
2013-03-08 17:15         ` Antony Pavlov
2013-03-07 10:33   ` [PATCH v2 3/5] tegra: add driver for the clock and reset module Lucas Stach
2013-03-10  7:41     ` Antony Pavlov
2013-03-10  9:53       ` Sascha Hauer
2013-03-07 10:33   ` [PATCH v2 4/5] tegra: add proper timer driver Lucas Stach
2013-03-10  7:12     ` Antony Pavlov
2013-03-07 10:33   ` [PATCH v2 5/5] tegra: add power management controller driver Lucas Stach
2013-03-10  8:19     ` Antony Pavlov

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=CAA4bVAETDP3+s-j8Hpr+g4ZX2ZPjUuYrPoa_LZMsX8jztvqX6g@mail.gmail.com \
    --to=antonynpavlov@gmail.com \
    --cc=barebox@lists.infradead.org \
    --cc=dev@lynxeye.de \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox