| Message ID | 1447417883-7881-1-git-send-email-jszhang@marvell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Friday 13 November 2015 20:31:23 Jisheng Zhang wrote: > diff --git a/drivers/clocksource/dw_apb_timer.c b/drivers/clocksource/dw_apb_timer.c > index c76c750..04282ee 100644 > --- a/drivers/clocksource/dw_apb_timer.c > +++ b/drivers/clocksource/dw_apb_timer.c > @@ -51,13 +51,13 @@ clocksource_to_dw_apb_clocksource(struct clocksource *cs) > > static unsigned long apbt_readl(struct dw_apb_timer *timer, unsigned long offs) > { > - return readl(timer->base + offs); > + return readl_relaxed(timer->base + offs); > } > > static void apbt_writel(struct dw_apb_timer *timer, unsigned long val, > unsigned long offs) > { > - writel(val, timer->base + offs); > + writel_relaxed(val, timer->base + offs); > } > > As with the other patch, I think it would be nicer to only change the functions that benefit from the change, to make it easier to prove that the conversion is correct. You could introduce apbt_readl_releaxed() etc functions and call them from __apbt_read_clocksource() and apbt_next_event(). Arnd
Dear Arnd, On Fri, 13 Nov 2015 13:42:09 +0100 Arnd Bergmann <arnd@arndb.de> wrote: > On Friday 13 November 2015 20:31:23 Jisheng Zhang wrote: > > diff --git a/drivers/clocksource/dw_apb_timer.c b/drivers/clocksource/dw_apb_timer.c > > index c76c750..04282ee 100644 > > --- a/drivers/clocksource/dw_apb_timer.c > > +++ b/drivers/clocksource/dw_apb_timer.c > > @@ -51,13 +51,13 @@ clocksource_to_dw_apb_clocksource(struct clocksource *cs) > > > > static unsigned long apbt_readl(struct dw_apb_timer *timer, unsigned long offs) > > { > > - return readl(timer->base + offs); > > + return readl_relaxed(timer->base + offs); > > } > > > > static void apbt_writel(struct dw_apb_timer *timer, unsigned long val, > > unsigned long offs) > > { > > - writel(val, timer->base + offs); > > + writel_relaxed(val, timer->base + offs); > > } > > > > > > As with the other patch, I think it would be nicer to only change the > functions that benefit from the change, to make it easier to prove I think to show performance improvement for this dw_apb_timer case is much easier, because the clocksource ->read() function also calls the apbt_readl. I will add some performance numbers in v3 > that the conversion is correct. You could introduce apbt_readl_releaxed() > etc functions and call them from __apbt_read_clocksource() > and apbt_next_event(). I'm not sure whether such changes would make the code a bit complex. From another side, it's safe to always use relaxed version in this driver, so is it better to switch to relaxed version no matter the code path benefit from it or not? PS: for the global timer related patch, I just hack code a bit to make it works as clockevent on my platform, and I still try to think about a test case to measure the improvement, cyclictest? Any idea is appreciated. Thanks in advance, Jisheng
diff --git a/drivers/clocksource/dw_apb_timer.c b/drivers/clocksource/dw_apb_timer.c index c76c750..04282ee 100644 --- a/drivers/clocksource/dw_apb_timer.c +++ b/drivers/clocksource/dw_apb_timer.c @@ -51,13 +51,13 @@ clocksource_to_dw_apb_clocksource(struct clocksource *cs) static unsigned long apbt_readl(struct dw_apb_timer *timer, unsigned long offs) { - return readl(timer->base + offs); + return readl_relaxed(timer->base + offs); } static void apbt_writel(struct dw_apb_timer *timer, unsigned long val, unsigned long offs) { - writel(val, timer->base + offs); + writel_relaxed(val, timer->base + offs); } static void apbt_disable_int(struct dw_apb_timer *timer)
The driver is safe to use the relaxed version. From another side, the relaxed io accessor macros are available on all architectures now, so we can use the relaxed versions to get a trivial overall system performance improvement and reduce the latency a bit on some architectures. Signed-off-by: Jisheng Zhang <jszhang@marvell.com> --- since v1: - correct the wrong sentence in commit msg about writel performance on CA9 with outer L2 cache. Thank Arnd for pointing out this in another thread drivers/clocksource/dw_apb_timer.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)