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@xxxxxxxxx>
> ---
>
> 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@xxxxxxxxxx>
Thanks,
drew
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
