From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suraj Jitindar Singh Subject: Re: [kvm-unit-tests PATCH V4 4/5] lib/powerpc: Implement generic delay function for use in unit tests Date: Thu, 18 Aug 2016 14:41:40 +1000 Message-ID: <1471495300.2138.19.camel@gmail.com> References: <1471416538-14088-1-git-send-email-sjitindarsingh@gmail.com> <1471416538-14088-4-git-send-email-sjitindarsingh@gmail.com> <1de94cc4-d520-acdb-4552-2112e7916a9f@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit Cc: pbonzini@redhat.com, rkrcmar@redhat.com, kvm-ppc@vger.kernel.org, lvivier@redhat.com, drjones@redhat.com To: Thomas Huth , kvm@vger.kernel.org Return-path: Received: from mail-pf0-f194.google.com ([209.85.192.194]:34184 "EHLO mail-pf0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750816AbcHREl7 (ORCPT ); Thu, 18 Aug 2016 00:41:59 -0400 In-Reply-To: <1de94cc4-d520-acdb-4552-2112e7916a9f@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Wed, 2016-08-17 at 10:19 +0200, Thomas Huth wrote: > On 17.08.2016 08:48, Suraj Jitindar Singh wrote: > > > > It would be nice if we had a generic delay function which could be > > used > > in unit tests, add one. > > > > Add the variable tb_hz used to store the time base frequency which > > is read > > from the device tree on setup. > > > > Add functions mdelay, udelay and delay in processor.c to delay for > > a given > > number of milliseconds, microseconds and time base ticks > > respectively. > > > > Signed-off-by: Suraj Jitindar Singh > > --- > > > > Change Log: > > > > V2 -> V3: > > - Add patch to series > > V3 -> V4: > > - Reword sleep->delay > > - Move cpu_relax to asm-generic/barrier.h > > - Add assert to catch when delay fns called with too large > > values > > --- > >  lib/asm-generic/barrier.h   |  4 ++++ > >  lib/powerpc/asm/processor.h | 19 +++++++++++++++++++ > >  lib/powerpc/asm/setup.h     |  2 ++ > >  lib/powerpc/processor.c     | 20 ++++++++++++++++++++ > >  lib/powerpc/setup.c         |  7 +++++++ > >  5 files changed, 52 insertions(+) > > > > diff --git a/lib/asm-generic/barrier.h b/lib/asm-generic/barrier.h > > index 12ae782..6a990ff 100644 > > --- a/lib/asm-generic/barrier.h > > +++ b/lib/asm-generic/barrier.h > > @@ -28,4 +28,8 @@ > >  #define smp_wmb() wmb() > >  #endif > >   > > +#ifndef cpu_relax > > +#define cpu_relax() asm volatile ("":::"memory") > > +#endif > > + > >  #endif /* _ASM_BARRIER_H_ */ > > diff --git a/lib/powerpc/asm/processor.h > > b/lib/powerpc/asm/processor.h > > index 09692bd..ac001e1 100644 > > --- a/lib/powerpc/asm/processor.h > > +++ b/lib/powerpc/asm/processor.h > > @@ -1,6 +1,7 @@ > >  #ifndef _ASMPOWERPC_PROCESSOR_H_ > >  #define _ASMPOWERPC_PROCESSOR_H_ > >   > > +#include > >  #include > >   > >  #ifndef __ASSEMBLY__ > > @@ -8,4 +9,22 @@ void handle_exception(int trap, void > > (*func)(struct pt_regs *, void *), void *); > >  void do_handle_exception(struct pt_regs *regs); > >  #endif /* __ASSEMBLY__ */ > >   > > +static inline uint64_t get_tb(void) > > +{ > > + uint64_t tb; > > + > > + asm volatile ("mfspr %[tb],268" : [tb] "=r" (tb)); > > + > > + return tb; > > +} > > + > > +extern void delay(uint64_t cycles); > > +extern void udelay(uint64_t us); > > + > > +static inline void mdelay(uint64_t ms) > > +{ > > + while (ms--) > > + udelay(1000); > > +} > > + > >  #endif /* _ASMPOWERPC_PROCESSOR_H_ */ > > diff --git a/lib/powerpc/asm/setup.h b/lib/powerpc/asm/setup.h > > index b1e1e5a..23b4156 100644 > > --- a/lib/powerpc/asm/setup.h > > +++ b/lib/powerpc/asm/setup.h > > @@ -11,6 +11,8 @@ > >  extern u32 cpus[NR_CPUS]; > >  extern int nr_cpus; > >   > > +extern uint64_t tb_hz; > > + > >  #define NR_MEM_REGIONS 8 > >  #define MR_F_PRIMARY (1U << 0) > >  struct mem_region { > > diff --git a/lib/powerpc/processor.c b/lib/powerpc/processor.c > > index a78bc3c..a28f2f0 100644 > > --- a/lib/powerpc/processor.c > > +++ b/lib/powerpc/processor.c > > @@ -5,6 +5,8 @@ > >  #include > >  #include > >  #include > > +#include > > +#include > >   > >  static struct { > >   void (*func)(struct pt_regs *, void *data); > > @@ -36,3 +38,21 @@ void do_handle_exception(struct pt_regs *regs) > >   printf("unhandled cpu exception 0x%lx\n", regs->trap); > >   abort(); > >  } > > + > > +void delay(uint64_t cycles) > > +{ > > + uint64_t start = get_tb(); > > + /* > > +  * Pretty unlikely unless your server has been on for, or > > you want to > > +  * delay for, over 1000 years, but still. > > +  */ > > + assert(cycles < (UINT64_MAX - start)); > > + while ((get_tb() - start) < cycles) > You could drop the parentheses around "get_tb() - start" ... > > > > > + cpu_relax(); > > +} > > + > > +void udelay(uint64_t us) > > +{ > > + assert(us < (UINT64_MAX / tb_hz)); > > + delay((us * tb_hz) / 1000000); > ... and around "us * tb_hz". > > > > > +} > > diff --git a/lib/powerpc/setup.c b/lib/powerpc/setup.c > > index e3d2afa..65bedf5 100644 > > --- a/lib/powerpc/setup.c > > +++ b/lib/powerpc/setup.c > > @@ -24,6 +24,7 @@ extern void setup_args_progname(const char > > *args); > >   > >  u32 cpus[NR_CPUS] = { [0 ... NR_CPUS-1] = (~0U) }; > >  int nr_cpus; > > +uint64_t tb_hz; > >   > >  struct mem_region mem_regions[NR_MEM_REGIONS]; > >  phys_addr_t __physical_start, __physical_end; > > @@ -72,6 +73,12 @@ static void cpu_set(int fdtnode, u32 regval, > > void *info) > >   data = (u32 *)prop->data; > >   params->dcache_bytes = fdt32_to_cpu(*data); > >   > > + prop = fdt_get_property(dt_fdt(), fdtnode, > > + "timebase-frequency", > > NULL); > > + assert(prop != NULL); > > + data = (u32 *)prop->data; > > + tb_hz = fdt32_to_cpu(*data); > > + > >   read_common_info = true; > >   } > >  } > Two minor cosmetic nits, patch already looks also fine to me as it > currently is, so: I'm a bit of a bracket over user, but I like them in these cases if only to prove that what I think is happening actually is when I can't remember c operator precedence. > > Reviewed-by: Thomas Huth > Thanks