Date:   Wed, 21 Aug 2019 00:24:31 +0000
From:   Atish Patra <Atish.Patra@....com>
To:     "hch@....de" <hch@....de>,
        "paul.walmsley@...ive.com" <paul.walmsley@...ive.com>,
        "palmer@...ive.com" <palmer@...ive.com>
CC:     "linux-riscv@...ts.infradead.org" <linux-riscv@...ts.infradead.org>,
        Damien Le Moal <Damien.LeMoal@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 08/15] riscv: provide native clint access for M-mode

On Tue, 2019-08-13 at 17:47 +0200, Christoph Hellwig wrote:
> RISC-V has the concept of a cpu level interrupt controller.  Part of
> it
> is expose as bits in the status registers, and 2 new CSRs per 
//is expose/is to expose/

> privilege
> level in the instruction set, but the machanisms to trigger IPIs and
> timer events, as well as reading the actual timer value are not
> specified in the RISC-V spec but usually delegated to a block of MMIO
> registers.  This patch adds support for those MMIO registers in the
> timer and IPI code.  For now only the SiFive layout also supported by
> a few other implementations is supported, but the code should be
> easily extensible to others in the future.
> 
> Signed-off-by: Christoph Hellwig <hch@....de>
> ---
>  arch/riscv/include/asm/clint.h    | 40 +++++++++++++++++++++++++++
>  arch/riscv/include/asm/timex.h    | 17 ++++++++++++
>  arch/riscv/kernel/Makefile        |  1 +
>  arch/riscv/kernel/clint.c         | 45
> +++++++++++++++++++++++++++++++
>  arch/riscv/kernel/setup.c         |  2 ++
>  arch/riscv/kernel/smp.c           | 24 +++++++++++++++++
>  arch/riscv/kernel/smpboot.c       |  3 +++
>  drivers/clocksource/timer-riscv.c | 16 ++++++++---
>  8 files changed, 144 insertions(+), 4 deletions(-)
>  create mode 100644 arch/riscv/include/asm/clint.h
>  create mode 100644 arch/riscv/kernel/clint.c
> 
> diff --git a/arch/riscv/include/asm/clint.h
> b/arch/riscv/include/asm/clint.h
> new file mode 100644
> index 000000000000..46d182d9a4db
> --- /dev/null
> +++ b/arch/riscv/include/asm/clint.h
> @@ -0,0 +1,40 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#ifndef _ASM_CLINT_H
> +#define _ASM_CLINT_H 1
> +
> +#include <linux/smp.h>
> +
> +#ifdef CONFIG_M_MODE
> +extern u32 __iomem *clint_ipi_base;
> +extern u64 __iomem *clint_time_val;
> +extern u64 __iomem *clint_time_cmp;
> +
> +void clint_init_boot_cpu(void);
> +
> +static inline void clint_send_ipi(unsigned long hartid)
> +{
> +	writel(1, clint_ipi_base + hartid);
> +}
> +
> +static inline void clint_clear_ipi(unsigned long hartid)
> +{
> +	writel(0, clint_ipi_base + hartid);
> +}
> +
> +static inline u64 clint_read_timer(void)
> +{
> +	return readq_relaxed(clint_time_val);
> +}
> +
> +static inline void clint_set_timer(unsigned long delta)
> +{
> +	writeq_relaxed(clint_read_timer() + delta,
> +		clint_time_cmp +
> cpuid_to_hartid_map(smp_processor_id()));'

This is not compatible with 32 bit mode. IIRC, timecmp is a 64 bit on
RV32 as well. Here is the implementation in OpenSBI.

https://github.com/riscv/opensbi/blob/master/lib/utils/sys/clint.c#L104

> +}
> +
> +#else
> +#define clint_init_boot_cpu()	do { } while (0)
> +#define clint_clear_ipi(hartid)	do { } while (0)
> +#endif /* CONFIG_M_MODE */
> +
> +#endif /* _ASM_CLINT_H */
> diff --git a/arch/riscv/include/asm/timex.h
> b/arch/riscv/include/asm/timex.h
> index 6a703ec9d796..bf907997f107 100644
> --- a/arch/riscv/include/asm/timex.h
> +++ b/arch/riscv/include/asm/timex.h
> @@ -10,6 +10,22 @@
>  
>  typedef unsigned long cycles_t;
>  
> +#ifdef CONFIG_M_MODE
> +
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <asm/clint.h>
> +
> +static inline cycles_t get_cycles(void)
> +{
> +#ifdef CONFIG_64BIT
> +	return readq_relaxed(clint_time_val);
> +#else
> +	return readl_relaxed(clint_time_val);
> +#endif

Same comment as above. Both RV32 & RV64 bit have 64 bit have 64 bit
precission for timer val. You have to read 32 bits at a time and "or"
them to get 64 bit value. Here is the implementation from OpenSBI

https://github.com/riscv/opensbi/blob/master/lib/utils/sys/clint.c#L69

> +}
> +#define get_cycles	get_cycles
> +
> +#else /* CONFIG_M_MODE */
>  static inline cycles_t get_cycles_inline(void)
>  {
>  	cycles_t n;
> @@ -40,6 +56,7 @@ static inline uint64_t get_cycles64(void)
>  	return ((u64)hi << 32) | lo;
>  }
>  #endif
> +#endif /* CONFIG_M_MODE */
>  
>  #define ARCH_HAS_READ_CURRENT_TIMER
>  
> diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
> index 2420d37d96de..f933c04f89db 100644
> --- a/arch/riscv/kernel/Makefile
> +++ b/arch/riscv/kernel/Makefile
> @@ -29,6 +29,7 @@ obj-y	+= vdso.o
>  obj-y	+= cacheinfo.o
>  obj-y	+= vdso/
>  
> +obj-$(CONFIG_M_MODE)		+= clint.o
>  obj-$(CONFIG_FPU)		+= fpu.o
>  obj-$(CONFIG_SMP)		+= smpboot.o
>  obj-$(CONFIG_SMP)		+= smp.o
> diff --git a/arch/riscv/kernel/clint.c b/arch/riscv/kernel/clint.c
> new file mode 100644
> index 000000000000..15b9e7fa5416
> --- /dev/null
> +++ b/arch/riscv/kernel/clint.c
> @@ -0,0 +1,45 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019 Christoph Hellwig.
> + */
> +
> +#include <linux/io.h>
> +#include <linux/of_address.h>
> +#include <linux/types.h>
> +#include <asm/csr.h>
> +#include <asm/irq.h>
> +#include <asm/timex.h>
> +
> +/*
> + * This is the layout used by the SiFive clint, which is also shared
> by the qemu
> + * virt platform, and the Kendryte KD210 at least.
> + */
> +#define CLINT_IPI_OFF		0
> +#define CLINT_TIME_VAL_OFF	0xbff8
> +#define CLINT_TIME_CMP_OFF	0x4000;
> +
> +u32 __iomem *clint_ipi_base;
> +u64 __iomem *clint_time_val;
> +u64 __iomem *clint_time_cmp;
> +
> +void clint_init_boot_cpu(void)
> +{
> +	struct device_node *np;
> +	void __iomem *base;
> +
> +	np = of_find_compatible_node(NULL, NULL, "riscv,clint0");
> +	if (!np) {
> +		panic("clint not found");
> +		return;
> +	}
> +
> +	base = of_iomap(np, 0);
> +	if (!base)
> +		panic("could not map CLINT");
> +
> +	clint_ipi_base = base + CLINT_IPI_OFF;
> +	clint_time_val = base + CLINT_TIME_VAL_OFF;
> +	clint_time_cmp = base + CLINT_TIME_CMP_OFF;
> +
> +	clint_clear_ipi(boot_cpu_hartid);
> +}
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index a990a6cb184f..f4ba71b66c73 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -17,6 +17,7 @@
>  #include <linux/sched/task.h>
>  #include <linux/swiotlb.h>
>  
> +#include <asm/clint.h>
>  #include <asm/setup.h>
>  #include <asm/sections.h>
>  #include <asm/pgtable.h>
> @@ -65,6 +66,7 @@ void __init setup_arch(char **cmdline_p)
>  	setup_bootmem();
>  	paging_init();
>  	unflatten_device_tree();
> +	clint_init_boot_cpu();
>  
>  #ifdef CONFIG_SWIOTLB
>  	swiotlb_init(1);
> diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c
> index 8cd730239613..ee8599a7ca48 100644
> --- a/arch/riscv/kernel/smp.c
> +++ b/arch/riscv/kernel/smp.c
> @@ -13,7 +13,9 @@
>  #include <linux/sched.h>
>  #include <linux/seq_file.h>
>  #include <linux/delay.h>
> +#include <linux/io.h>
>  
> +#include <asm/clint.h>
>  #include <asm/sbi.h>
>  #include <asm/tlbflush.h>
>  #include <asm/cacheflush.h>
> @@ -78,6 +80,27 @@ static void ipi_stop(void)
>  		wait_for_interrupt();
>  }
>  
> +#ifdef CONFIG_M_MODE
> +static inline void send_ipi_single(int cpu, enum ipi_message_type
> op)
> +{
> +	set_bit(op, &ipi_data[cpu].bits);
> +	clint_send_ipi(cpuid_to_hartid_map(cpu));
> +}
> +
> +static inline void send_ipi_mask(const struct cpumask *mask,
> +		enum ipi_message_type op)
> +{
> +	int cpu;
> +
> +	for_each_cpu(cpu, mask)
> +		send_ipi_single(cpu, op);
> +}
> +
> +static inline void clear_ipi(void)
> +{
> +	clint_clear_ipi(cpuid_to_hartid_map(smp_processor_id()));
> +}
> +#else /* CONFIG_M_MODE */
>  static void send_ipi_mask(const struct cpumask *mask, enum
> ipi_message_type op)
>  {
>  	int cpuid, hartid;
> @@ -103,6 +126,7 @@ static inline void clear_ipi(void)
>  {
>  	csr_clear(CSR_SIP, SIE_SSIE);
>  }
> +#endif /* CONFIG_M_MODE */
>  
>  void riscv_software_interrupt(void)
>  {
> diff --git a/arch/riscv/kernel/smpboot.c
> b/arch/riscv/kernel/smpboot.c
> index 7462a44304fe..1b7678d86ec8 100644
> --- a/arch/riscv/kernel/smpboot.c
> +++ b/arch/riscv/kernel/smpboot.c
> @@ -23,6 +23,7 @@
>  #include <linux/of.h>
>  #include <linux/sched/task_stack.h>
>  #include <linux/sched/mm.h>
> +#include <asm/clint.h>
>  #include <asm/irq.h>
>  #include <asm/mmu_context.h>
>  #include <asm/tlbflush.h>
> @@ -132,6 +133,8 @@ asmlinkage void __init smp_callin(void)
>  {
>  	struct mm_struct *mm = &init_mm;
>  
> +	clint_clear_ipi(cpuid_to_hartid_map(smp_processor_id()));
> +
>  	/* All kernel threads share the same mm context.  */
>  	mmgrab(mm);
>  	current->active_mm = mm;
> diff --git a/drivers/clocksource/timer-riscv.c
> b/drivers/clocksource/timer-riscv.c
> index 3ad2fa52bac9..09f2165bd0a5 100644
> --- a/drivers/clocksource/timer-riscv.c
> +++ b/drivers/clocksource/timer-riscv.c
> @@ -24,12 +24,16 @@
>   * 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)
>  {
>  	csr_set(CSR_XIE, XIE_XTIE);
> +
> +#ifdef CONFIG_M_MODE
> +	clint_set_timer(delta);
> +#else
>  	sbi_set_timer(get_cycles64() + delta);
> +#endif
>  	return 0;
>  }
>  
> @@ -45,14 +49,18 @@ static DEFINE_PER_CPU(struct clock_event_device,
> riscv_clock_event) = {
>   * within one tick of each other, so while this could technically go
>   * backwards when hopping between CPUs, practically it won't happen.
>   */
> -static unsigned long long riscv_clocksource_rdtime(struct
> clocksource *cs)
> +static u64 riscv_sched_clock(void)
>  {
> +#ifdef CONFIG_M_MODE
> +	return clint_read_timer();
> +#else
>  	return get_cycles64();
> +#endif
>  }
>  
> -static u64 riscv_sched_clock(void)
> +static unsigned long long riscv_clocksource_rdtime(struct
> clocksource *cs)
>  {
> -	return get_cycles64();
> +	return riscv_sched_clock();
>  }
>  
>  static struct clocksource riscv_clocksource = {

-- 
Regards,
Atish

Powered by blists - more mailing lists