| Message ID | 20190821145837.3686-6-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/6] riscv: refactor the IPI code | expand |
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 > } > -#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) > {
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) > > {
On Sat, Aug 24, 2019 at 12:37:02AM +0000, Atish Patra wrote: > > -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. It better should. rdtime is just a pseudo-instruction that the assembler translates to a CSR read. (in other words something totally pointless, no idea why it even is in the spec).
On Mon, 2019-08-26 at 13:30 +0200, hch@lst.de wrote: > On Sat, Aug 24, 2019 at 12:37:02AM +0000, Atish Patra wrote: > > > -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. > > It better should. rdtime is just a pseudo-instruction that the > assembler translates to a CSR read. (in other words something > totally > pointless, no idea why it even is in the spec). Yes. I did not see the usermode macro carefully in qemu code. Reviewed-by: Atish Patra <atish.patra@wdc.com>
On Wed, 21 Aug 2019, 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> Thanks, queued for v5.4-rc1 with Atish's Reviewed-by:. - Paul
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); } -#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) {
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(-)