Re: [PATCH 5/6] riscv: don't use the rdtime(h) pseudo-instructions

From: Atish Patra <Atish.Patra@wdc.com>
To: "hch@lst.de" <hch@lst.de>,
	"paul.walmsley@sifive.com" <paul.walmsley@sifive.com>,
	"palmer@sifive.com" <palmer@sifive.com>
Cc: "linux-riscv@lists.infradead.org" <linux-riscv@lists.infradead.org>
Subject: Re: [PATCH 5/6] riscv: don't use the rdtime(h) pseudo-instructions
Date: Sat, 24 Aug 2019 00:43:20 +0000	[thread overview]
Message-ID: <bcc437eca5ed4196fc6cba5b852bd9d4e858f405.camel@wdc.com> (raw)
In-Reply-To: <d15a578760061a5a0ebad8ca6024768f831686d2.camel@wdc.com>

On Fri, 2019-08-23 at 17:37 -0700, Atish Patra wrote:
> On Wed, 2019-08-21 at 23:58 +0900, Christoph Hellwig wrote:
> > If we just use the CSRs that these map to directly the code is
> > simpler
> > and doesn't require extra inline assembly code.  Also fix up the
> > top-
> > level
> > comment in timer-riscv.c to not talk about the cycle count or
> > mention
> > details of the clocksource interface, of which this file is just a
> > consumer.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  arch/riscv/include/asm/timex.h    | 44 +++++++++++++++------------
> > ----
> >  drivers/clocksource/timer-riscv.c | 17 +++---------
> >  2 files changed, 25 insertions(+), 36 deletions(-)
> > 
> > diff --git a/arch/riscv/include/asm/timex.h
> > b/arch/riscv/include/asm/timex.h
> > index 6a703ec9d796..c7ef131b9e4c 100644
> > --- a/arch/riscv/include/asm/timex.h
> > +++ b/arch/riscv/include/asm/timex.h
> > @@ -6,43 +6,41 @@
> >  #ifndef _ASM_RISCV_TIMEX_H
> >  #define _ASM_RISCV_TIMEX_H
> >  
> > -#include <asm/param.h>
> > +#include <asm/csr.h>
> >  
> >  typedef unsigned long cycles_t;
> >  
> > -static inline cycles_t get_cycles_inline(void)
> > +static inline cycles_t get_cycles(void)
> >  {
> > -	cycles_t n;
> > -
> > -	__asm__ __volatile__ (
> > -		"rdtime %0"
> > -		: "=r" (n));
> > -	return n;
> > +	return csr_read(CSR_TIME);
> 
> Does this work correctly in QEMU ? I was looking at the qemu code and
> it looks like it returns cpu_get_host_ticks which seems wrong to me.
> 
> https://github.com/qemu/qemu/blob/master/target/riscv/csr.c#L213
> 
> 

Nevermind. I missed the CONFIG_USER_ONLY and got confused.
csr_read will also trap and get the correct value.

Regards,
Atish
> >  }
> > -#define get_cycles get_cycles_inline
> > +#define get_cycles get_cycles
> >  
> >  #ifdef CONFIG_64BIT
> > -static inline uint64_t get_cycles64(void)
> > +static inline u64 get_cycles64(void)
> > +{
> > +	return get_cycles();
> > +}
> > +#else /* CONFIG_64BIT */
> > +static inline u32 get_cycles_hi(void)
> >  {
> > -        return get_cycles();
> > +	return csr_read(CSR_TIMEH);
> >  }
> > -#else
> > -static inline uint64_t get_cycles64(void)
> > +
> > +static inline u64 get_cycles64(void)
> >  {
> > -	u32 lo, hi, tmp;
> > -	__asm__ __volatile__ (
> > -		"1:\n"
> > -		"rdtimeh %0\n"
> > -		"rdtime %1\n"
> > -		"rdtimeh %2\n"
> > -		"bne %0, %2, 1b"
> > -		: "=&r" (hi), "=&r" (lo), "=&r" (tmp));
> > +	u32 hi, lo;
> > +
> > +	do {
> > +		hi = get_cycles_hi();
> > +		lo = get_cycles();
> > +	} while (hi != get_cycles_hi());
> > +
> >  	return ((u64)hi << 32) | lo;
> >  }
> > -#endif
> > +#endif /* CONFIG_64BIT */
> >  
> >  #define ARCH_HAS_READ_CURRENT_TIMER
> > -
> >  static inline int read_current_timer(unsigned long *timer_val)
> >  {
> >  	*timer_val = get_cycles();
> > diff --git a/drivers/clocksource/timer-riscv.c
> > b/drivers/clocksource/timer-riscv.c
> > index 09e031176bc6..470c7ef02ea4 100644
> > --- a/drivers/clocksource/timer-riscv.c
> > +++ b/drivers/clocksource/timer-riscv.c
> > @@ -2,6 +2,10 @@
> >  /*
> >   * Copyright (C) 2012 Regents of the University of California
> >   * Copyright (C) 2017 SiFive
> > + *
> > + * All RISC-V systems have a timer attached to every hart.  These
> > timers can be
> > + * read from the "time" and "timeh" CSRs, and can use the SBI to
> > setup
> > + * events.
> >   */
> >  #include <linux/clocksource.h>
> >  #include <linux/clockchips.h>
> > @@ -12,19 +16,6 @@
> >  #include <asm/smp.h>
> >  #include <asm/sbi.h>
> >  
> > -/*
> > - * All RISC-V systems have a timer attached to every hart.  These
> > timers can be
> > - * read by the 'rdcycle' pseudo instruction, and can use the SBI
> > to
> > setup
> > - * events.  In order to abstract the architecture-specific timer
> > reading and
> > - * setting functions away from the clock event insertion code, we
> > provide
> > - * function pointers to the clockevent subsystem that perform two
> > basic
> > - * operations: rdtime() reads the timer on the current CPU, and
> > - * next_event(delta) sets the next timer event to 'delta' cycles
> > in
> > the future.
> > - * As the timers are inherently a per-cpu resource, these
> > callbacks
> > perform
> > - * operations on the current hart.  There is guaranteed to be
> > exactly one timer
> > - * per hart on all RISC-V systems.
> > - */
> > -
> >  static int riscv_clock_next_event(unsigned long delta,
> >  		struct clock_event_device *ce)
> >  {

-- 
Regards,
Atish
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv