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

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

[dinguyen at altera.com]

From: Dinh Nguyen <dinguyen@altera.com>

Move all dw_apb_timer defines to include/linux/dw_apb_timer.h.

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

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

[RESEND PATCHv3 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

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

[Dinh Nguyen]

On Wed, 2013-08-28 at 21:53 -0500, Dinh Nguyen wrote:
> From: Dinh Nguyen <dinguyen@altera.com>
> 
> Move all dw_apb_timer defines to include/linux/dw_apb_timer.h.
> 
> 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
> 
> 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;

Ping? This patch series has sat idle for 2-weeks now.

Thanks,
Dinh

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

[Thomas Gleixner]

On Tue, 17 Sep 2013, Dinh Nguyen wrote:

> On Wed, 2013-08-28 at 21:53 -0500, Dinh Nguyen wrote:
> > From: Dinh Nguyen <dinguyen@altera.com>
> > 
> > Move all dw_apb_timer defines to include/linux/dw_apb_timer.h.

That's describing WHAT the patch does not WHY. I can see the WHAT part
without that pointless changelog.

> 
> Ping? This patch series has sat idle for 2-weeks now.

So what?

Sending non urgent (i.e. bug fixes, regression fixes etc.) patches at
the beginning of the merge window begs for a 2 weeks delay. Well
documented procedure...


Thanks,

	tglx

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

[Pavel Machek]

On Tue 2013-09-17 23:45:11, Thomas Gleixner wrote:
> On Tue, 17 Sep 2013, Dinh Nguyen wrote:
> 
> > On Wed, 2013-08-28 at 21:53 -0500, Dinh Nguyen wrote:
> > > From: Dinh Nguyen <dinguyen@altera.com>
> > > 
> > > Move all dw_apb_timer defines to include/linux/dw_apb_timer.h.
> 
> That's describing WHAT the patch does not WHY. I can see the WHAT part
> without that pointless changelog.

read_sched_clock() in dw_apb_timer_of.c accesses the same hardware as
dw_apb_timer.c, therefore it benefits from #define's that describe the
hardware. IOW 1/2 is neccessary for 2/2, but it can compile/work
indepedently.

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

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

[Dinh Nguyen]

On Tue, 2013-09-17 at 23:45 +0200, Thomas Gleixner wrote:
> On Tue, 17 Sep 2013, Dinh Nguyen wrote:
> 
> > On Wed, 2013-08-28 at 21:53 -0500, Dinh Nguyen wrote:
> > > From: Dinh Nguyen <dinguyen@altera.com>
> > > 
> > > Move all dw_apb_timer defines to include/linux/dw_apb_timer.h.
> 
> That's describing WHAT the patch does not WHY. I can see the WHAT part
> without that pointless changelog.

Will fix..
> 
> > 
> > Ping? This patch series has sat idle for 2-weeks now.
> 
> So what?
> 
> Sending non urgent (i.e. bug fixes, regression fixes etc.) patches at
> the beginning of the merge window begs for a 2 weeks delay. Well
> documented procedure...

Sorry, but it was just a friendly ping...didn't mean to imply any
urgency. The original patch was sent on 8/21, so patch has been around
for ~4 weeks..

Thanks for the review.

Dinh
> 
> 
> Thanks,
> 
> 	tglx
> 

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

[Thomas Gleixner]

Pavel,

On Tue, 17 Sep 2013, Pavel Machek wrote:
> On Tue 2013-09-17 23:45:11, Thomas Gleixner wrote:
> > On Tue, 17 Sep 2013, Dinh Nguyen wrote:
> > 
> > > On Wed, 2013-08-28 at 21:53 -0500, Dinh Nguyen wrote:
> > > > From: Dinh Nguyen <dinguyen@altera.com>
> > > > 
> > > > Move all dw_apb_timer defines to include/linux/dw_apb_timer.h.
> > 
> > That's describing WHAT the patch does not WHY. I can see the WHAT part
> > without that pointless changelog.
> 
> read_sched_clock() in dw_apb_timer_of.c accesses the same hardware as
> dw_apb_timer.c, therefore it benefits from #define's that describe the
> hardware. IOW 1/2 is neccessary for 2/2, but it can compile/work
> indepedently.

I'm really capable of figuring that out myself. But that's not the
point. I want patch submitters to explain in the changelog WHY they
are doing a change not WHAT they are doing, because that's (mostly)
obvious from the patch itself.

So thanks for your well meant, but completely superfluous explanation.

	tglx

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

[Pavel Machek]

Hi!

> > > > > From: Dinh Nguyen <dinguyen@altera.com>
> > > > > 
> > > > > Move all dw_apb_timer defines to include/linux/dw_apb_timer.h.
> > > 
> > > That's describing WHAT the patch does not WHY. I can see the WHAT part
> > > without that pointless changelog.
> > 
> > read_sched_clock() in dw_apb_timer_of.c accesses the same hardware as
> > dw_apb_timer.c, therefore it benefits from #define's that describe the
> > hardware. IOW 1/2 is neccessary for 2/2, but it can compile/work
> > indepedently.
> 
> I'm really capable of figuring that out myself. But that's not the
> point. I want patch submitters to explain in the changelog WHY they
> are doing a change not WHAT they are doing, because that's (mostly)
> obvious from the patch itself.

But the end result is that anything takes months to do...

We already have the dw_apb_timer issues merged in your tree, then it
got dropped in merge conflict, then you dislike the changelog, and now
you notice devicetree uglyness that we did not introduce.

Yes, dw_apb_timer code is not exactly clean, but if we have to wait
month before you lecture us how to write changelogs, we will not get
anywhere... discussion why the DT was done that way is forgotten by
now.

Because just now, we are running in circles (and time does not work on
socfpga for second major release).

Regards,

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