Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1446637 > unrolled thread

[PATCH v8 4/9] clocksource/drivers/arm_arch_timer: use readq to get 64-bit CNTVCT

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v8 4/9] clocksource/drivers/arm_arch_timer: use readq to get 64-bit CNTVCT fu.wei@linaro.org - 2016-07-19 20:20 +0200
    Re: [PATCH v8 4/9] clocksource/drivers/arm_arch_timer: use readq to  get 64-bit CNTVCT kbuild test robot <lkp@intel.com> - 2016-07-24 22:30 +0200
    Re: [PATCH v8 4/9] clocksource/drivers/arm_arch_timer: use readq to  get 64-bit CNTVCT Will Deacon <will.deacon@arm.com> - 2016-07-25 11:10 +0200
      Re: [PATCH v8 4/9] clocksource/drivers/arm_arch_timer: use readq to  get 64-bit CNTVCT Fu Wei <fu.wei@linaro.org> - 2016-07-25 18:00 +0200
        Re: [PATCH v8 4/9] clocksource/drivers/arm_arch_timer: use readq to  get 64-bit CNTVCT Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-07-26 00:50 +0200

#1446637 — [PATCH v8 4/9] clocksource/drivers/arm_arch_timer: use readq to get 64-bit CNTVCT

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);
 }
 
 /*
-- 
2.5.5

[toc] | [next] | [standalone]

#1449203 — Re: [PATCH v8 4/9] clocksource/drivers/arm_arch_timer: use readq to get 64-bit CNTVCT

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1449387 — Re: [PATCH v8 4/9] clocksource/drivers/arm_arch_timer: use readq to get 64-bit CNTVCT

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

[toc] | [prev] | [next] | [standalone]

#1449611 — Re: [PATCH v8 4/9] clocksource/drivers/arm_arch_timer: use readq to get 64-bit CNTVCT

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



-- 
Best regards,

Fu Wei
Software Engineer
Red Hat

[toc] | [prev] | [next] | [standalone]

#1450233 — Re: [PATCH v8 4/9] clocksource/drivers/arm_arch_timer: use readq to get 64-bit CNTVCT

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.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web