[PATCHv4 1/2] clocksource: dw_apb_timer: Move timer defines to header file.

[PATCHv4 1/2] clocksource: dw_apb_timer: Move timer defines to header file.

[dinguyen at altera.com]

From: Dinh Nguyen <dinguyen@altera.com>

The timer defines currently reside in dw_apb_timer.c, and
dw_apb_timer_of.c needs to make use of these defines as well.

Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
CC: Rob Herring <rob.herring@calxeda.com>
CC: Arnd Bergmann <arnd@arndb.de>
Cc: Olof Johansson <olof@lixom.net>
CC: Jamie Iles <jamie@jamieiles.com>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Pavel Machek <pavel@denx.de>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: linux-arm-kernel at lists.infradead.org
---
v3:
- Explain WHY in the changelog.

v2:
- Remove the defines in dw_apb_timer.c

 drivers/clocksource/dw_apb_timer.c |   19 -------------------
 include/linux/dw_apb_timer.h       |   19 +++++++++++++++++++
 2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/clocksource/dw_apb_timer.c b/drivers/clocksource/dw_apb_timer.c
index e54ca10..c3a8f52 100644
--- a/drivers/clocksource/dw_apb_timer.c
+++ b/drivers/clocksource/dw_apb_timer.c
@@ -18,25 +18,6 @@
 #include <linux/io.h>
 #include <linux/slab.h>
 
-#define APBT_MIN_PERIOD			4
-#define APBT_MIN_DELTA_USEC		200
-
-#define APBTMR_N_LOAD_COUNT		0x00
-#define APBTMR_N_CURRENT_VALUE		0x04
-#define APBTMR_N_CONTROL		0x08
-#define APBTMR_N_EOI			0x0c
-#define APBTMR_N_INT_STATUS		0x10
-
-#define APBTMRS_INT_STATUS		0xa0
-#define APBTMRS_EOI			0xa4
-#define APBTMRS_RAW_INT_STATUS		0xa8
-#define APBTMRS_COMP_VERSION		0xac
-
-#define APBTMR_CONTROL_ENABLE		(1 << 0)
-/* 1: periodic, 0:free running. */
-#define APBTMR_CONTROL_MODE_PERIODIC	(1 << 1)
-#define APBTMR_CONTROL_INT		(1 << 2)
-
 static inline struct dw_apb_clock_event_device *
 ced_to_dw_apb_ced(struct clock_event_device *evt)
 {
diff --git a/include/linux/dw_apb_timer.h b/include/linux/dw_apb_timer.h
index 1f79b20..1d2b949 100644
--- a/include/linux/dw_apb_timer.h
+++ b/include/linux/dw_apb_timer.h
@@ -19,6 +19,25 @@
 
 #define APBTMRS_REG_SIZE       0x14
 
+#define APBT_MIN_PERIOD                 4
+#define APBT_MIN_DELTA_USEC             200
+
+#define APBTMR_N_LOAD_COUNT             0x00
+#define APBTMR_N_CURRENT_VALUE          0x04
+#define APBTMR_N_CONTROL                0x08
+#define APBTMR_N_EOI                    0x0c
+#define APBTMR_N_INT_STATUS             0x10
+
+#define APBTMRS_INT_STATUS              0xa0
+#define APBTMRS_EOI                     0xa4
+#define APBTMRS_RAW_INT_STATUS          0xa8
+#define APBTMRS_COMP_VERSION            0xac
+
+#define APBTMR_CONTROL_ENABLE           (1 << 0)
+/* 1: periodic, 0:free running. */
+#define APBTMR_CONTROL_MODE_PERIODIC    (1 << 1)
+#define APBTMR_CONTROL_INT              (1 << 2)
+
 struct dw_apb_timer {
 	void __iomem				*base;
 	unsigned long				freq;
-- 
1.7.9.5

[PATCHv4 2/2] clocksource: dw_apb_timer_of: Fix read_sched_clock

[dinguyen at altera.com]

From: Dinh Nguyen <dinguyen@altera.com>

The read_sched_clock should return the ~value because the clock is a
countdown implementation. read_sched_clock() should be the same as
__apbt_read_clocksource().

If a separate timer for the sched_clock exist, then read_sched_clock()
will return an incorrect value. The (sched_io_base + 0x4) needs to be in
the function for both cases.

Maintain backwards compatibility for "dw-apb-timer-sp" and
"dw-apb-timer-osc".

Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
Acked-by: Jamie Iles <jamie@jamieiles.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
CC: Rob Herring <rob.herring@calxeda.com>
CC: Arnd Bergmann <arnd@arndb.de>
Cc: Olof Johansson <olof@lixom.net>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Pavel Machek <pavel@denx.de>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: linux-arm-kernel at lists.infradead.org
---
v3:
- Use APBTMR_N_CURRENT_VALUE define in read_sched_clock()
    
v2:
- Maintain backwards compatibility for "dw-apb-timer-sp" and
"dw-apb-timer-osc".

 drivers/clocksource/dw_apb_timer_of.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/clocksource/dw_apb_timer_of.c b/drivers/clocksource/dw_apb_timer_of.c
index 4cbae4f..01c1238 100644
--- a/drivers/clocksource/dw_apb_timer_of.c
+++ b/drivers/clocksource/dw_apb_timer_of.c
@@ -102,18 +102,17 @@ static void add_clocksource(struct device_node *source_timer)
 	 * timer is found. sched_io_base then points to the current_value
 	 * register of the clocksource timer.
 	 */
-	sched_io_base = iobase + 0x04;
+	sched_io_base = iobase;
 	sched_rate = rate;
 }
 
 static u32 read_sched_clock(void)
 {
-	return __raw_readl(sched_io_base);
+	return ~__raw_readl(sched_io_base + APBTMR_N_CURRENT_VALUE);
 }
 
 static const struct of_device_id sptimer_ids[] __initconst = {
 	{ .compatible = "picochip,pc3x2-rtc" },
-	{ .compatible = "snps,dw-apb-timer-sp" },
 	{ /* Sentinel */ },
 };
 
@@ -153,4 +152,6 @@ static void __init dw_apb_timer_init(struct device_node *timer)
 	num_called++;
 }
 CLOCKSOURCE_OF_DECLARE(pc3x2_timer, "picochip,pc3x2-timer", dw_apb_timer_init);
-CLOCKSOURCE_OF_DECLARE(apb_timer, "snps,dw-apb-timer-osc", dw_apb_timer_init);
+CLOCKSOURCE_OF_DECLARE(apb_timer_osc, "snps,dw-apb-timer-osc", dw_apb_timer_init);
+CLOCKSOURCE_OF_DECLARE(apb_timer_sp, "snps,dw-apb-timer-sp", dw_apb_timer_init);
+CLOCKSOURCE_OF_DECLARE(apb_timer, "snps,dw-apb-timer", dw_apb_timer_init);
-- 
1.7.9.5

[PATCHv4 2/2] clocksource: dw_apb_timer_of: Fix read_sched_clock

[Thomas Gleixner]

On Tue, 17 Sep 2013, dinguyen at altera.com wrote: 

> read_sched_clock() should be the same as __apbt_read_clocksource().

So why is it using a separate function in the first place?

> If a separate timer for the sched_clock exist, then read_sched_clock()
> will return an incorrect value. The (sched_io_base + 0x4) needs to be in
> the function for both cases.

It really took me some time to understand the "separate timer" part of
the above explanation. Though that made me look at the actual
implementation and I have to say, that this abuse of the device tree
is really a unique masterpiece:

static int num_called;
static void __init dw_apb_timer_init(struct device_node *timer)
{
        switch (num_called) {
        case 0:
                pr_debug("%s: found clockevent timer\n", __func__);
                add_clockevent(timer);
                of_node_put(timer);
                break;
        case 1:
                pr_debug("%s: found clocksource timer\n", __func__);
                add_clocksource(timer);
                of_node_put(timer);
                init_sched_clock();
                break;
        default:
                break;
        }

        num_called++;
}

So if you can use different nodes for clockevent and clocksource, why
is that supposed to be dependent on the ordering? That's not how DT is
supposed to be used. DT provides a clear description of the hardware,
not some ordering dependent magic amended by utterly useless pr_debug()
constructs.

Thanks,

	tglx

[PATCHv4 2/2] clocksource: dw_apb_timer_of: Fix read_sched_clock

[Pavel Machek]

On Wed 2013-09-18 00:42:36, Thomas Gleixner wrote:
> On Tue, 17 Sep 2013, dinguyen at altera.com wrote: 
> 
> > read_sched_clock() should be the same as __apbt_read_clocksource().
> 
> So why is it using a separate function in the first place?


> > If a separate timer for the sched_clock exist, then read_sched_clock()
> > will return an incorrect value. The (sched_io_base + 0x4) needs to be in
> > the function for both cases.
> 
> It really took me some time to understand the "separate timer" part of
> the above explanation. Though that made me look at the actual
> implementation and I have to say, that this abuse of the device tree
> is really a unique masterpiece:

Well. We did a patch to fix timers on socfpga. You merged it. Then it
clashed with Heiko Stubner's work. Our version was dropped, his
version contained this code you complain about.

_I also complained about that
code_. http://www.spinics.net/lists/arm-kernel/msg252732.html . Seems
it was merged anyway. (2 months ago.)

And now we can't get the code fixed so that it at least works on our
hardware, because, guess what, you noticed upstream merged the gem
below, and you don't like it?

> static int num_called;
> static void __init dw_apb_timer_init(struct device_node *timer)
> {
>         switch (num_called) {
>         case 0:
>                 pr_debug("%s: found clockevent timer\n", __func__);
>                 add_clockevent(timer);
>                 of_node_put(timer);
>                 break;
>         case 1:
>                 pr_debug("%s: found clocksource timer\n", __func__);
>                 add_clocksource(timer);
>                 of_node_put(timer);
>                 init_sched_clock();
>                 break;
>         default:
>                 break;
>         }
> 
>         num_called++;
> }
> 
> So if you can use different nodes for clockevent and clocksource, why
> is that supposed to be dependent on the ordering? That's not how DT is
> supposed to be used. DT provides a clear description of the hardware,
> not some ordering dependent magic amended by utterly useless pr_debug()
> constructs.

You already had non-ugly version in your tree.

Alternatively, tell us what you want done. These boards have 2 to 4
identical timers, that can serve as both clockevent and
clocksource. We'd like to use one as clockevent and one as
clocksource. 

Regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCHv4 2/2] clocksource: dw_apb_timer_of: Fix read_sched_clock

[Heiko Stübner]

Am Mittwoch, 18. September 2013, 13:01:59 schrieb Pavel Machek:
> On Wed 2013-09-18 00:42:36, Thomas Gleixner wrote:
> > On Tue, 17 Sep 2013, dinguyen at altera.com wrote:
> And now we can't get the code fixed so that it at least works on our
> hardware, because, guess what, you noticed upstream merged the gem
> below, and you don't like it?
> 
> > static int num_called;
> > static void __init dw_apb_timer_init(struct device_node *timer)
> > {
> > 
> >         switch (num_called) {
> >         
> >         case 0:
> >                 pr_debug("%s: found clockevent timer\n", __func__);
> >                 add_clockevent(timer);
> >                 of_node_put(timer);
> >                 break;
> >         
> >         case 1:
> >                 pr_debug("%s: found clocksource timer\n", __func__);
> >                 add_clocksource(timer);
> >                 of_node_put(timer);
> >                 init_sched_clock();
> >                 break;
> >         
> >         default:
> >                 break;
> >         
> >         }
> >         
> >         num_called++;
> > 
> > }
> > 
> > So if you can use different nodes for clockevent and clocksource, why
> > is that supposed to be dependent on the ordering? That's not how DT is
> > supposed to be used. DT provides a clear description of the hardware,
> > not some ordering dependent magic amended by utterly useless pr_debug()
> > constructs.
> 
> You already had non-ugly version in your tree.
> 
> Alternatively, tell us what you want done. These boards have 2 to 4
> identical timers, that can serve as both clockevent and
> clocksource. We'd like to use one as clockevent and one as
> clocksource.

I would also be interested in the "right" way to do this.

As Pavel already said, the hardware is identical for all N separate timer 
blocks, so as the DT should be describing the hardware only, there is no way 
to specifiy one for the clockevent and another for the clocksource there.

At first I kept using the non-standard init which required it being called from 
platform code, but got the request to convert the driver to use 
CLOCKSOURCE_OF_DECLARE to remove the need for separate call.

As you will know CLOCKSOURCE_OF_DECLARE calls the init function for each found 
dt node for a matching device, resulting in N calls to dw_apb_timer_init.
So my solution was to just grab the first one as clockevent and second one as 
clocksource.

Therefore I'm all ears for how to solve this in a better way :-)


Heiko

[PATCHv4 2/2] clocksource: dw_apb_timer_of: Fix read_sched_clock

[Dinh Nguyen]

Hi Thomas,

On Wed, 2013-09-18 at 13:32 +0200, Heiko St?bner wrote:
> Am Mittwoch, 18. September 2013, 13:01:59 schrieb Pavel Machek:
> > On Wed 2013-09-18 00:42:36, Thomas Gleixner wrote:
> > > On Tue, 17 Sep 2013, dinguyen at altera.com wrote:
> > And now we can't get the code fixed so that it at least works on our
> > hardware, because, guess what, you noticed upstream merged the gem
> > below, and you don't like it?
> > 
> > > static int num_called;
> > > static void __init dw_apb_timer_init(struct device_node *timer)
> > > {
> > > 
> > >         switch (num_called) {
> > >         
> > >         case 0:
> > >                 pr_debug("%s: found clockevent timer\n", __func__);
> > >                 add_clockevent(timer);
> > >                 of_node_put(timer);
> > >                 break;
> > >         
> > >         case 1:
> > >                 pr_debug("%s: found clocksource timer\n", __func__);
> > >                 add_clocksource(timer);
> > >                 of_node_put(timer);
> > >                 init_sched_clock();
> > >                 break;
> > >         
> > >         default:
> > >                 break;
> > >         
> > >         }
> > >         
> > >         num_called++;
> > > 
> > > }
> > > 
> > > So if you can use different nodes for clockevent and clocksource, why
> > > is that supposed to be dependent on the ordering? That's not how DT is
> > > supposed to be used. DT provides a clear description of the hardware,
> > > not some ordering dependent magic amended by utterly useless pr_debug()
> > > constructs.
> > 
> > You already had non-ugly version in your tree.
> > 
> > Alternatively, tell us what you want done. These boards have 2 to 4
> > identical timers, that can serve as both clockevent and
> > clocksource. We'd like to use one as clockevent and one as
> > clocksource.
> 
> I would also be interested in the "right" way to do this.
> 
> As Pavel already said, the hardware is identical for all N separate timer 
> blocks, so as the DT should be describing the hardware only, there is no way 
> to specifiy one for the clockevent and another for the clocksource there.
> 
> At first I kept using the non-standard init which required it being called from 
> platform code, but got the request to convert the driver to use 
> CLOCKSOURCE_OF_DECLARE to remove the need for separate call.
> 
> As you will know CLOCKSOURCE_OF_DECLARE calls the init function for each found 
> dt node for a matching device, resulting in N calls to dw_apb_timer_init.
> So my solution was to just grab the first one as clockevent and second one as 
> clocksource.
> 
> Therefore I'm all ears for how to solve this in a better way :-)
> 

I'm just wondering if you have gotten a chance to give this patch
anymore thought? The state of the socfpga platform for 3.12 is that it
will not boot without this patch(mainly because of a DTS binding
change). This patch mainly only fixes that issue.

If you would like dw_apb_timer_init() fix for 3.13, can you please give
us advice, so that we can get started on it in time for 3.13?

Thanks,
Dinh
> 
> Heiko
> 

[PATCHv4 2/2] clocksource: dw_apb_timer_of: Fix read_sched_clock

[Daniel Lezcano]

On 09/23/2013 05:58 PM, Dinh Nguyen wrote:
> Hi Thomas,
>
> On Wed, 2013-09-18 at 13:32 +0200, Heiko St?bner wrote:
>> Am Mittwoch, 18. September 2013, 13:01:59 schrieb Pavel Machek:
>>> On Wed 2013-09-18 00:42:36, Thomas Gleixner wrote:
>>>> On Tue, 17 Sep 2013, dinguyen at altera.com wrote:
>>> And now we can't get the code fixed so that it at least works on our
>>> hardware, because, guess what, you noticed upstream merged the gem
>>> below, and you don't like it?
>>>
>>>> static int num_called;
>>>> static void __init dw_apb_timer_init(struct device_node *timer)
>>>> {
>>>>
>>>>          switch (num_called) {
>>>>
>>>>          case 0:
>>>>                  pr_debug("%s: found clockevent timer\n", __func__);
>>>>                  add_clockevent(timer);
>>>>                  of_node_put(timer);
>>>>                  break;
>>>>
>>>>          case 1:
>>>>                  pr_debug("%s: found clocksource timer\n", __func__);
>>>>                  add_clocksource(timer);
>>>>                  of_node_put(timer);
>>>>                  init_sched_clock();
>>>>                  break;
>>>>
>>>>          default:
>>>>                  break;
>>>>
>>>>          }
>>>>
>>>>          num_called++;
>>>>
>>>> }
>>>>
>>>> So if you can use different nodes for clockevent and clocksource, why
>>>> is that supposed to be dependent on the ordering? That's not how DT is
>>>> supposed to be used. DT provides a clear description of the hardware,
>>>> not some ordering dependent magic amended by utterly useless pr_debug()
>>>> constructs.
>>>
>>> You already had non-ugly version in your tree.
>>>
>>> Alternatively, tell us what you want done. These boards have 2 to 4
>>> identical timers, that can serve as both clockevent and
>>> clocksource. We'd like to use one as clockevent and one as
>>> clocksource.
>>
>> I would also be interested in the "right" way to do this.
>>
>> As Pavel already said, the hardware is identical for all N separate timer
>> blocks, so as the DT should be describing the hardware only, there is no way
>> to specifiy one for the clockevent and another for the clocksource there.
>>
>> At first I kept using the non-standard init which required it being called from
>> platform code, but got the request to convert the driver to use
>> CLOCKSOURCE_OF_DECLARE to remove the need for separate call.
>>
>> As you will know CLOCKSOURCE_OF_DECLARE calls the init function for each found
>> dt node for a matching device, resulting in N calls to dw_apb_timer_init.
>> So my solution was to just grab the first one as clockevent and second one as
>> clocksource.
>>
>> Therefore I'm all ears for how to solve this in a better way :-)
>>
>
> I'm just wondering if you have gotten a chance to give this patch
> anymore thought? The state of the socfpga platform for 3.12 is that it
> will not boot without this patch(mainly because of a DTS binding
> change). This patch mainly only fixes that issue.
>
> If you would like dw_apb_timer_init() fix for 3.13, can you please give
> us advice, so that we can get started on it in time for 3.13?

Dinh,

I second Thomas comment's about the initialization code in the driver vs 
dt-binding, IMO it is worth to investigate what could be the "right 
way", as said Heiko, to do it. I suspect this situation will occur again 
for some other drivers. Perhaps, Grant Likely or Rob Herring can give us 
some advices (cc'ed).

Grant/Rob,

if a timer device description gives several timers where the driver use 
one for the clocksource and the other one for the clockevent, the 
initialization code has to deal multiple init and find a way to register 
them properly. This is the reason of the code snippet above. As the 
device tree is supposed to do stricly a hardware description, do you 
have an advice from a device tree perspective to implement something 
better than the code above ? Similar situation occurs with the efm32 
driver [1][2].

Thanks !
   -- Daniel

[1] http://www.spinics.net/lists/arm-kernel/msg275403.html (at the end)
[2] http://www.spinics.net/lists/arm-kernel/msg277261.html




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