| Message ID | 1468952284-28942-5-git-send-email-fu.wei@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi,
[auto build test ERROR on stable/master]
[cannot apply to tip/timers/core pm/linux-next next-20160724]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/fu-wei-linaro-org/acpi-clocksource-add-GTDT-driver-and-GTDT-support-in-arm_arch_timer/20160725-022614
base: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git master
config: arm-sunxi_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
All errors (new ones prefixed by >>):
drivers/clocksource/arm_arch_timer.c: In function 'arch_counter_get_cntvct_mem':
>> drivers/clocksource/arm_arch_timer.c:421:9: error: implicit declaration of function 'readq' [-Werror=implicit-function-declaration]
return readq(arch_counter_base + CNTVCT_LO);
^
cc1: some warnings being treated as errors
vim +/readq +421 drivers/clocksource/arm_arch_timer.c
415 {
416 return arch_timer_rate;
417 }
418
419 static u64 arch_counter_get_cntvct_mem(void)
420 {
> 421 return readq(arch_counter_base + CNTVCT_LO);
422 }
423
424 /*
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Wed, Jul 20, 2016 at 02:17:59AM +0800, fu.wei@linaro.org wrote: > From: Fu Wei <fu.wei@linaro.org> > > This patch simplify arch_counter_get_cntvct_mem function by > using readq to get 64-bit CNTVCT value instead of readl_relaxed. > > Signed-off-by: Fu Wei <fu.wei@linaro.org> > --- > drivers/clocksource/arm_arch_timer.c | 10 +--------- > 1 file changed, 1 insertion(+), 9 deletions(-) > > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c > index e6fd42d..483d2f9 100644 > --- a/drivers/clocksource/arm_arch_timer.c > +++ b/drivers/clocksource/arm_arch_timer.c > @@ -418,15 +418,7 @@ u32 arch_timer_get_rate(void) > > static u64 arch_counter_get_cntvct_mem(void) > { > - u32 vct_lo, vct_hi, tmp_hi; > - > - do { > - vct_hi = readl_relaxed(arch_counter_base + CNTVCT_HI); > - vct_lo = readl_relaxed(arch_counter_base + CNTVCT_LO); > - tmp_hi = readl_relaxed(arch_counter_base + CNTVCT_HI); > - } while (vct_hi != tmp_hi); > - > - return ((u64) vct_hi << 32) | vct_lo; > + return readq(arch_counter_base + CNTVCT_LO); What's the benefit of doing this? If you use readq here, how can we guarantee that (a) the endpoint won't generate a SLVERR or similar and (b) that we get an atomic read? "If it ain't broke, don't fix it" Will
Hi Will, On 25 July 2016 at 17:02, Will Deacon <will.deacon@arm.com> wrote: > On Wed, Jul 20, 2016 at 02:17:59AM +0800, fu.wei@linaro.org wrote: >> From: Fu Wei <fu.wei@linaro.org> >> >> This patch simplify arch_counter_get_cntvct_mem function by >> using readq to get 64-bit CNTVCT value instead of readl_relaxed. >> >> Signed-off-by: Fu Wei <fu.wei@linaro.org> >> --- >> drivers/clocksource/arm_arch_timer.c | 10 +--------- >> 1 file changed, 1 insertion(+), 9 deletions(-) >> >> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c >> index e6fd42d..483d2f9 100644 >> --- a/drivers/clocksource/arm_arch_timer.c >> +++ b/drivers/clocksource/arm_arch_timer.c >> @@ -418,15 +418,7 @@ u32 arch_timer_get_rate(void) >> >> static u64 arch_counter_get_cntvct_mem(void) >> { >> - u32 vct_lo, vct_hi, tmp_hi; >> - >> - do { >> - vct_hi = readl_relaxed(arch_counter_base + CNTVCT_HI); >> - vct_lo = readl_relaxed(arch_counter_base + CNTVCT_LO); >> - tmp_hi = readl_relaxed(arch_counter_base + CNTVCT_HI); >> - } while (vct_hi != tmp_hi); >> - >> - return ((u64) vct_hi << 32) | vct_lo; >> + return readq(arch_counter_base + CNTVCT_LO); > Sorry, right after posting v9, I got your comment, > What's the benefit of doing this? If you use readq here, how can we benefit: 1. simplify the code 2. from arch/arm64/include/asm/io.h, I guess readq is more efficient > guarantee that (a) the endpoint won't generate a SLVERR or similar and > (b) that we get ? I think so, according to arch/arm64/include/asm/io.h. readq Implement by "LDR" and "LDAR", So I think It is an atomic read. Please correct me, If I misunderstand something, thanks > > "If it ain't broke, don't fix it" > > Will
On Mon, Jul 25, 2016 at 11:50:19PM +0800, Fu Wei wrote: > Hi Will, > > On 25 July 2016 at 17:02, Will Deacon <will.deacon@arm.com> wrote: > > On Wed, Jul 20, 2016 at 02:17:59AM +0800, fu.wei@linaro.org wrote: > >> From: Fu Wei <fu.wei@linaro.org> > >> > >> This patch simplify arch_counter_get_cntvct_mem function by > >> using readq to get 64-bit CNTVCT value instead of readl_relaxed. > >> > >> Signed-off-by: Fu Wei <fu.wei@linaro.org> > >> --- > >> drivers/clocksource/arm_arch_timer.c | 10 +--------- > >> 1 file changed, 1 insertion(+), 9 deletions(-) > >> > >> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c > >> index e6fd42d..483d2f9 100644 > >> --- a/drivers/clocksource/arm_arch_timer.c > >> +++ b/drivers/clocksource/arm_arch_timer.c > >> @@ -418,15 +418,7 @@ u32 arch_timer_get_rate(void) > >> > >> static u64 arch_counter_get_cntvct_mem(void) > >> { > >> - u32 vct_lo, vct_hi, tmp_hi; > >> - > >> - do { > >> - vct_hi = readl_relaxed(arch_counter_base + CNTVCT_HI); > >> - vct_lo = readl_relaxed(arch_counter_base + CNTVCT_LO); > >> - tmp_hi = readl_relaxed(arch_counter_base + CNTVCT_HI); > >> - } while (vct_hi != tmp_hi); > >> - > >> - return ((u64) vct_hi << 32) | vct_lo; > >> + return readq(arch_counter_base + CNTVCT_LO); > > > > Sorry, right after posting v9, I got your comment, > > > What's the benefit of doing this? If you use readq here, how can we > > benefit: > 1. simplify the code > 2. from arch/arm64/include/asm/io.h, I guess readq is more efficient And the harm is that it breaks the build on ARM, because ARM doesn't provide readq (as it can't, as not all CPUs on ARM support 64-bit reads) and it's not appropriate for architecture code to emulate it. Consider carefully why the original code has that loop present - only a device driver hows how to read a 64-bit register safely using two 32-bit reads. Such a loop may not be appropriate for some other device. So... as the 0-day builder detected a failure on ARM with this, NAK. If you want to make this conditional on readq() being present, that'd be acceptable, but you must support the case where readq() is not provided.
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index e6fd42d..483d2f9 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -418,15 +418,7 @@ u32 arch_timer_get_rate(void) static u64 arch_counter_get_cntvct_mem(void) { - u32 vct_lo, vct_hi, tmp_hi; - - do { - vct_hi = readl_relaxed(arch_counter_base + CNTVCT_HI); - vct_lo = readl_relaxed(arch_counter_base + CNTVCT_LO); - tmp_hi = readl_relaxed(arch_counter_base + CNTVCT_HI); - } while (vct_hi != tmp_hi); - - return ((u64) vct_hi << 32) | vct_lo; + return readq(arch_counter_base + CNTVCT_LO); } /*