| Message ID | 1374723797-22169-2-git-send-email-dinguyen@altera.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am Donnerstag, 25. Juli 2013, 05:43:17 schrieb dinguyen@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. > > 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@altera.com> > Acked-by: Jamie Iles <jamie@jamieiles.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: 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@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
Hi Heiko, On Thu, 2013-07-25 at 11:02 +0200, Heiko Stübner wrote: > Am Donnerstag, 25. Juli 2013, 05:43:17 schrieb dinguyen@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. > > > > 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@altera.com> > > Acked-by: Jamie Iles <jamie@jamieiles.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: 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@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. I agree with you, but I was under the impression that Arnd wanted to keep DTS changes in a separate commit. Dinh > > > Heiko > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
Hi! > 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(). ...for altera boards. But AFAICT: > 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 */ }, > }; > picochip,pc3x2-rtc is _not_ compatible with snps,dw-apb-timer, and does count up. You correctly remove snps,dw-apb-timer-sp compatibility, but AFAICT the ~value change will do something interesting on picochip boards. Or maybe I'm just confused about all the timers? Jamie acked this so it should be fine...? Thanks, Pavel
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 */ }, }; @@ -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);