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

[Heiko Stübner]

Am Donnerstag, 25. Juli 2013, 05:43:17 schrieb dinguyen at altera.com:
> From: Dinh Nguyen <dinguyen at 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.
> 
> Also, remove the use of "dw-apb-timer-sp" and "dw-apb-timer-osc" since
> they are the same DW clock.
> 
> Signed-off-by: Dinh Nguyen <dinguyen at altera.com>
> Acked-by: Jamie Iles <jamie at jamieiles.com>
> CC: Rob Herring <rob.herring at calxeda.com>
> CC: Arnd Bergmann <arnd at arndb.de>
> Cc: Olof Johansson <olof at lixom.net>
> CC: Jamie Iles <jamie at jamieiles.com>
> Cc: John Stultz <john.stultz at linaro.org>
> Cc: Thomas Gleixner <tglx at linutronix.de>
> Cc: Linus Walleij <linus.walleij at linaro.org>
> Cc: Pavel Machek <pavel at denx.de>
> Cc: Heiko Stuebner <heiko at sntech.de>
> Cc: linux-arm-kernel at lists.infradead.org
> ---
>  drivers/clocksource/dw_apb_timer_of.c |    7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clocksource/dw_apb_timer_of.c
> b/drivers/clocksource/dw_apb_timer_of.c index 4cbae4f..8e00929 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 + 0x4);
>  }
> 



>  static const struct of_device_id sptimer_ids[] __initconst = {
>  	{ .compatible = "picochip,pc3x2-rtc" },
> -	{ .compatible = "snps,dw-apb-timer-sp" },
>  	{ /* Sentinel */ },
>  };

I'm not 100% sure, but maybe the same explaination as below applies to this - 
with it better being part of the first patch.


> @@ -153,4 +152,4 @@ 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, "snps,dw-apb-timer",
> dw_apb_timer_init);

I think this hunk would better be part of the first patch, as you rename the 
devices in the dtsi files there and it has nothing to do with the sched_clock 
fix.

Changing the clocksource name here also breaks bisectability on the affected 
platforms because they wouldn't be able to add the clocksources when only the 
first patch is applied.


Heiko