| Message ID | 1471416538-14088-4-git-send-email-sjitindarsingh@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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 <sjitindarsingh@gmail.com> > --- > > 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 <libcflat.h> > #include <asm/ptrace.h> > > #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 <libcflat.h> > #include <asm/processor.h> > #include <asm/ptrace.h> > +#include <asm/setup.h> > +#include <asm/barrier.h> > > 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: Reviewed-by: Thomas Huth <thuth@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 17, 2016 at 04:48:57PM +1000, 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 <sjitindarsingh@gmail.com> > --- > > 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 <libcflat.h> > #include <asm/ptrace.h> > > #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 <libcflat.h> > #include <asm/processor.h> > #include <asm/ptrace.h> > +#include <asm/setup.h> > +#include <asm/barrier.h> > > 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) I don't think the above assert is necessary. As long as the subtraction (get_tb() - start) produces a uint64_t, then the condition should always work - per C99. Maybe it should be written as (uint64_t)(get_tb() - start) to be 100% correct though. > + cpu_relax(); > +} > + > +void udelay(uint64_t us) > +{ > + assert(us < (UINT64_MAX / tb_hz)); Same comment here. I'm pretty sure (wrap around wraps my head, so I could be wrong) that the main concern is maintaining unsigned integer subtraction, which the C99 guarantees to wrap modulo 2^N, N being the number of bits of the unsigned integer. > + delay((us * tb_hz) / 1000000); > +} > 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); Arguably the dance I do with cpu_set_params to pass values back to cpu_init, where they simply get assigned to globals, is pointless. It's trying to maintain encapsulation (which I violate for nr_cpus anyway...) That said, I'd like to see tb_hz either integrate with the cpu_set_params pattern, or a cleanup patch come before this one, which removes cpu_set_params, allowing icache/dcache_bytes setting to match the tb_hz pattern. I won't hold this series up on that though. > + > read_common_info = true; > } > } > -- > 2.5.5 Neither of my comments are deal breakers, so Reviewed-by: Andrew Jones <drjones@redhat.com> Thanks, drew -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2016-08-17 at 15:04 +0200, Andrew Jones wrote: > On Wed, Aug 17, 2016 at 04:48:57PM +1000, 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 <sjitindarsingh@gmail.com> > > --- > > > > 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 <libcflat.h> > > #include <asm/ptrace.h> > > > > #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 <libcflat.h> > > #include <asm/processor.h> > > #include <asm/ptrace.h> > > +#include <asm/setup.h> > > +#include <asm/barrier.h> > > > > 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) > I don't think the above assert is necessary. As long as the > subtraction > (get_tb() - start) produces a uint64_t, then the condition should > always > work - per C99. Maybe it should be written as (uint64_t)(get_tb() - > start) > to be 100% correct though. This is to catch the case where the caller passes a ridiculously large cycles value (e.g. UINT64_MAX - 1) and/or start is sufficiently large that get_tb() - start will never be >= to cycles because the time-base counter will overflow and wrap around before that ever becomes true. This is super unlikely but will avoid an infinite loop in the event someone does it. > > > > > + cpu_relax(); > > +} > > + > > +void udelay(uint64_t us) > > +{ > > + assert(us < (UINT64_MAX / tb_hz)); > Same comment here. I'm pretty sure (wrap around wraps my head, so I > could be wrong) that the main concern is maintaining unsigned integer > subtraction, which the C99 guarantees to wrap modulo 2^N, N being the > number of bits of the unsigned integer. This is to catch when the caller tries to sleep for > 36000000000us (10 hrs), which I realise is highly unlikely. But in this case us * tb_hz will be too big to store in a u64. Thus this won't delay for the intended time, hence the assert. > > > > > + delay((us * tb_hz) / 1000000); > > +} > > 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); > Arguably the dance I do with cpu_set_params to pass values back to > cpu_init, where they simply get assigned to globals, is pointless. > It's > trying to maintain encapsulation (which I violate for nr_cpus > anyway...) > That said, I'd like to see tb_hz either integrate with the > cpu_set_params > pattern, or a cleanup patch come before this one, which removes > cpu_set_params, allowing icache/dcache_bytes setting to match the > tb_hz > pattern. I won't hold this series up on that though. Yeah I see whats happening here, I'll make it match what is done for i/dcache. > > > > > + > > read_common_info = true; > > } > > } Thanks -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <sjitindarsingh@gmail.com> > > --- > > > > 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 <libcflat.h> > > #include <asm/ptrace.h> > > > > #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 <libcflat.h> > > #include <asm/processor.h> > > #include <asm/ptrace.h> > > +#include <asm/setup.h> > > +#include <asm/barrier.h> > > > > 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 <thuth@redhat.com> > Thanks -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 18, 2016 at 02:39:07PM +1000, Suraj Jitindar Singh wrote: > On Wed, 2016-08-17 at 15:04 +0200, Andrew Jones wrote: > > On Wed, Aug 17, 2016 at 04:48:57PM +1000, Suraj Jitindar Singh wrote: > > > + > > > +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) > > I don't think the above assert is necessary. As long as the > > subtraction > > (get_tb() - start) produces a uint64_t, then the condition should > > always > > work - per C99. Maybe it should be written as (uint64_t)(get_tb() - > > start) > > to be 100% correct though. > This is to catch the case where the caller passes a ridiculously large > cycles value (e.g. UINT64_MAX - 1) and/or start is sufficiently large > that get_tb() - start will never be >= to cycles because the time-base > counter will overflow and wrap around before that ever becomes true. > This is super unlikely but will avoid an infinite loop in the event > someone does it. I understand that. What I'm saying is that with unsigned integer subtraction, per C99, you don't have to worry about it. Just try #include <stdio.h> #include <stdint.h> int main(void) { uint64_t start = UINT64_MAX - 1; uint64_t tb = 5; // tb wrapped uint64_t cycles = 10; if ((tb - start) < cycles) printf("works fine\n"); return 0; } to see that it works fine. As for guarding against ridiculously large cycles inputs, don't bother. How do you even define what's ridiculous? I'd say it's anything more than a minute... > > > > > > > > + cpu_relax(); > > > +} > > > + > > > +void udelay(uint64_t us) > > > +{ > > > + assert(us < (UINT64_MAX / tb_hz)); > > Same comment here. I'm pretty sure (wrap around wraps my head, so I > > could be wrong) that the main concern is maintaining unsigned integer > > subtraction, which the C99 guarantees to wrap modulo 2^N, N being the > > number of bits of the unsigned integer. > This is to catch when the caller tries to sleep for > 36000000000us (10 > hrs), which I realise is highly unlikely. But in this case us * tb_hz > will be too big to store in a u64. Thus this won't delay for the > intended time, hence the assert. If the caller does that, the we're doing him a favor by wrapping the input... More seriously, while I'm in favor of asserts for external inputs (DT reads, command line inputs), I think it's safe to expect unit test developers to just not do something like this, or to be able to debug it themselves when they do, without the aid of asserts pinpointing the mistake for them. Thanks, drew -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2016-08-18 at 12:24 +0200, Andrew Jones wrote: > On Thu, Aug 18, 2016 at 02:39:07PM +1000, Suraj Jitindar Singh wrote: > > > > On Wed, 2016-08-17 at 15:04 +0200, Andrew Jones wrote: > > > > > > On Wed, Aug 17, 2016 at 04:48:57PM +1000, Suraj Jitindar Singh > > > wrote: > > > > > > > > + > > > > +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) > > > I don't think the above assert is necessary. As long as the > > > subtraction > > > (get_tb() - start) produces a uint64_t, then the condition should > > > always > > > work - per C99. Maybe it should be written as (uint64_t)(get_tb() > > > - > > > start) > > > to be 100% correct though. > > This is to catch the case where the caller passes a ridiculously > > large > > cycles value (e.g. UINT64_MAX - 1) and/or start is sufficiently > > large > > that get_tb() - start will never be >= to cycles because the time- > > base > > counter will overflow and wrap around before that ever becomes > > true. > > This is super unlikely but will avoid an infinite loop in the event > > someone does it. > I understand that. What I'm saying is that with unsigned integer > subtraction, per C99, you don't have to worry about it. Just try > > #include <stdio.h> > #include <stdint.h> > int main(void) > { > uint64_t start = UINT64_MAX - 1; > uint64_t tb = 5; // tb wrapped > uint64_t cycles = 10; > > if ((tb - start) < cycles) > printf("works fine\n"); > return 0; > } > > to see that it works fine. As for guarding against ridiculously large > cycles inputs, don't bother. How do you even define what's > ridiculous? > I'd say it's anything more than a minute... Ok I understand what you're saying now. I'll bin the assert. Thanks. > > > > > > > > > > > > > > > > > > > > > + cpu_relax(); > > > > +} > > > > + > > > > +void udelay(uint64_t us) > > > > +{ > > > > + assert(us < (UINT64_MAX / tb_hz)); > > > Same comment here. I'm pretty sure (wrap around wraps my head, so > > > I > > > could be wrong) that the main concern is maintaining unsigned > > > integer > > > subtraction, which the C99 guarantees to wrap modulo 2^N, N being > > > the > > > number of bits of the unsigned integer. > > This is to catch when the caller tries to sleep for > 36000000000us > > (10 > > hrs), which I realise is highly unlikely. But in this case us * > > tb_hz > > will be too big to store in a u64. Thus this won't delay for the > > intended time, hence the assert. > If the caller does that, the we're doing him a favor by wrapping > the input... More seriously, while I'm in favor of asserts for > external inputs (DT reads, command line inputs), I think it's safe > to expect unit test developers to just not do something like this, > or to be able to debug it themselves when they do, without the aid > of asserts pinpointing the mistake for them. Makes sense, I'll ditch this assert as well. > > Thanks, > drew Thanks -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <libcflat.h> #include <asm/ptrace.h> #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 <libcflat.h> #include <asm/processor.h> #include <asm/ptrace.h> +#include <asm/setup.h> +#include <asm/barrier.h> 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) + cpu_relax(); +} + +void udelay(uint64_t us) +{ + assert(us < (UINT64_MAX / tb_hz)); + delay((us * tb_hz) / 1000000); +} 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; } }
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 <sjitindarsingh@gmail.com> --- 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(+)