Re: [PATCH v4 2/4] clocksource/drivers: Add CLINT timer driver

2020-07-21 Thread Anup Patel
On Tue, Jul 21, 2020 at 5:45 PM Daniel Lezcano
 wrote:
>
> On 21/07/2020 13:49, Anup Patel wrote:
> > On Tue, Jul 21, 2020 at 4:32 PM Daniel Lezcano
> >  wrote:
> >>
> >> On 17/07/2020 09:50, Anup Patel wrote:
> >>> We add a separate CLINT timer driver for Linux RISC-V M-mode (i.e.
> >>> RISC-V NoMMU kernel).
> >>>
> >>> The CLINT MMIO device provides three things:
> >>> 1. 64bit free running counter register
> >>> 2. 64bit per-CPU time compare registers
> >>> 3. 32bit per-CPU inter-processor interrupt registers
> >>>
> >>> Unlike other timer devices, CLINT provides IPI registers along with
> >>> timer registers. To use CLINT IPI registers, the CLINT timer driver
> >>> provides IPI related callbacks to arch/riscv.
> >>>
> >>> Signed-off-by: Anup Patel 
> >>> Tested-by: Emil Renner Berhing 
> >>> ---
> >>>  drivers/clocksource/Kconfig   |   9 ++
> >>>  drivers/clocksource/Makefile  |   1 +
> >>>  drivers/clocksource/timer-clint.c | 231 ++
> >>>  include/linux/cpuhotplug.h|   1 +
> >>>  4 files changed, 242 insertions(+)
> >>>  create mode 100644 drivers/clocksource/timer-clint.c
> >>>
> >>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> >>> index 91418381fcd4..e1ce0d510a03 100644
> >>> --- a/drivers/clocksource/Kconfig
> >>> +++ b/drivers/clocksource/Kconfig
> >>> @@ -658,6 +658,15 @@ config RISCV_TIMER
> >>> is accessed via both the SBI and the rdcycle instruction.  This is
> >>> required for all RISC-V systems.
> >>>
> >>> +config CLINT_TIMER
> >>> + bool "Timer for the RISC-V platform"
> >>> + depends on GENERIC_SCHED_CLOCK && RISCV_M_MODE
> >>> + select TIMER_PROBE
> >>> + select TIMER_OF
> >>> + help
> >>> +   This option enables the CLINT timer for RISC-V systems. The CLINT
> >>> +   driver is usually used for NoMMU RISC-V systems.
> >>
> >> V3 has a comment about fixing the Kconfig option.
> >
> > I have removed "default y" from the Kconfig option as-per your suggestions.
> >
> > I looked at other Timer Kconfig options. Most of them have menuconfig name.
> > Also, we can certainly have different timer MMIO timer drivers in future. Do
> > you still insist on making this kconfig option totally silent ??
>
> Yes, and there is an effort to change the entries to be silent as much
> as possible.
>
> Just add:
>
> bool "Timer for the RISC-V platform" if COMPILE_TEST

Okay, I will update.

>
> and remove the RISCV_M_MODE dependency.

CLINT driver depends on RISC-V specific symbols from asm/smp.h
so we should at least have "depends on RISCV" so that compile
test does not fail.

Agree ??

>
> Or alternatively:
>
> replace the RISCV_M_MODE dependency with COMPILE_TEST
>
> The goal is to be able to compile the driver on different platforms for
> compilation test covering.

Please see the above comment.

>
> Then when more mmio drivers will added we will figure out.
>
> >> [ ... ]
> >>
> >>> +{
> >>> + bool *registered = per_cpu_ptr(&clint_clock_event_registered, cpu);
> >>> + struct clock_event_device *ce = per_cpu_ptr(&clint_clock_event, 
> >>> cpu);
> >>> +
> >>> + if (!(*registered)) {
> >>> + ce->cpumask = cpumask_of(cpu);
> >>> + clockevents_config_and_register(ce, clint_timer_freq, 200,
> >>> +  ULONG_MAX);
> >>> + *registered = true;
> >>> + }
> >>
> >>
> >> I was unsure about the clockevents_config_and_register() multiple calls
> >> when doing the comment. It seems like it is fine to call it several
> >> times and that is done in several places like riscv or arch_arm_timer.
> >>
> >> It is probably safe to drop the 'registered' code here, sorry for the
> >> confusion.
> >
> > Okay, will revert these changes.
> >
> >>
> >>> + enable_percpu_irq(clint_timer_irq,
> >>> +   irq_get_trigger_type(clint_timer_irq));
> >>> + return 0;
> >>> +}
> >>> +
> >>
> >> [ ... ]
> >>
> >>
> >> --
> >>  Linaro.org │ Open source software for ARM SoCs
> >>
> >> Follow Linaro:   Facebook |
> >>  Twitter |
> >>  Blog
> >
> > Regards,
> > Anup
> >
>
>
> --
>  Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:   Facebook |
>  Twitter |
>  Blog

Regards,
Anup


Re: [PATCH v4 2/4] clocksource/drivers: Add CLINT timer driver

2020-07-21 Thread Atish Patra
On Tue, Jul 21, 2020 at 4:44 AM Anup Patel  wrote:
>
> On Tue, Jul 21, 2020 at 6:41 AM Atish Patra  wrote:
> >
> > On Fri, Jul 17, 2020 at 12:52 AM Anup Patel  wrote:
> > >
> > > We add a separate CLINT timer driver for Linux RISC-V M-mode (i.e.
> > > RISC-V NoMMU kernel).
> > >
> > > The CLINT MMIO device provides three things:
> > > 1. 64bit free running counter register
> > > 2. 64bit per-CPU time compare registers
> > > 3. 32bit per-CPU inter-processor interrupt registers
> > >
> > > Unlike other timer devices, CLINT provides IPI registers along with
> > > timer registers. To use CLINT IPI registers, the CLINT timer driver
> > > provides IPI related callbacks to arch/riscv.
> > >
> > > Signed-off-by: Anup Patel 
> > > Tested-by: Emil Renner Berhing 
> > > ---
> > >  drivers/clocksource/Kconfig   |   9 ++
> > >  drivers/clocksource/Makefile  |   1 +
> > >  drivers/clocksource/timer-clint.c | 231 ++
> > >  include/linux/cpuhotplug.h|   1 +
> > >  4 files changed, 242 insertions(+)
> > >  create mode 100644 drivers/clocksource/timer-clint.c
> > >
> > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > > index 91418381fcd4..e1ce0d510a03 100644
> > > --- a/drivers/clocksource/Kconfig
> > > +++ b/drivers/clocksource/Kconfig
> > > @@ -658,6 +658,15 @@ config RISCV_TIMER
> > >   is accessed via both the SBI and the rdcycle instruction.  This 
> > > is
> > >   required for all RISC-V systems.
> > >
> > > +config CLINT_TIMER
> > > +   bool "Timer for the RISC-V platform"
> > > +   depends on GENERIC_SCHED_CLOCK && RISCV_M_MODE
> > > +   select TIMER_PROBE
> > > +   select TIMER_OF
> > > +   help
> > > + This option enables the CLINT timer for RISC-V systems. The 
> > > CLINT
> > > + driver is usually used for NoMMU RISC-V systems.
> > > +
> > >  config CSKY_MP_TIMER
> > > bool "SMP Timer for the C-SKY platform" if COMPILE_TEST
> > > depends on CSKY
> > > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> > > index bdda1a2e4097..18e700e703a0 100644
> > > --- a/drivers/clocksource/Makefile
> > > +++ b/drivers/clocksource/Makefile
> > > @@ -87,6 +87,7 @@ obj-$(CONFIG_CLKSRC_ST_LPC)   += clksrc_st_lpc.o
> > >  obj-$(CONFIG_X86_NUMACHIP) += numachip.o
> > >  obj-$(CONFIG_ATCPIT100_TIMER)  += timer-atcpit100.o
> > >  obj-$(CONFIG_RISCV_TIMER)  += timer-riscv.o
> > > +obj-$(CONFIG_CLINT_TIMER)  += timer-clint.o
> > >  obj-$(CONFIG_CSKY_MP_TIMER)+= timer-mp-csky.o
> > >  obj-$(CONFIG_GX6605S_TIMER)+= timer-gx6605s.o
> > >  obj-$(CONFIG_HYPERV_TIMER) += hyperv_timer.o
> > > diff --git a/drivers/clocksource/timer-clint.c 
> > > b/drivers/clocksource/timer-clint.c
> > > new file mode 100644
> > > index ..e1698efa73a1
> > > --- /dev/null
> > > +++ b/drivers/clocksource/timer-clint.c
> > > @@ -0,0 +1,231 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (C) 2020 Western Digital Corporation or its affiliates.
> > > + *
> > > + * Most of the M-mode (i.e. NoMMU) RISC-V systems usually have a
> > > + * CLINT MMIO timer device.
> > > + */
> > > +
> > > +#define pr_fmt(fmt) "clint: " fmt
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +#include 
> > > +
> > > +#define CLINT_IPI_OFF  0
> > > +#define CLINT_TIMER_CMP_OFF0x4000
> > > +#define CLINT_TIMER_VAL_OFF0xbff8
> > > +
> > > +/* CLINT manages IPI and Timer for RISC-V M-mode  */
> > > +static u32 __iomem *clint_ipi_base;
> > > +static u64 __iomem *clint_timer_cmp;
> > > +static u64 __iomem *clint_timer_val;
> > > +static unsigned long clint_timer_freq;
> > > +static unsigned int clint_timer_irq;
> > > +
> > > +static void clint_send_ipi(const struct cpumask *target)
> > > +{
> > > +   unsigned int cpu;
> > > +
> > > +   for_each_cpu(cpu, target)
> > > +   writel(1, clint_ipi_base + cpuid_to_hartid_map(cpu));
> > > +}
> > > +
> > > +static void clint_clear_ipi(void)
> > > +{
> > > +   writel(0, clint_ipi_base + 
> > > cpuid_to_hartid_map(smp_processor_id()));
> > > +}
> > > +
> > > +static struct riscv_ipi_ops clint_ipi_ops = {
> > > +   .ipi_inject = clint_send_ipi,
> > > +   .ipi_clear = clint_clear_ipi,
> > > +};
> > > +
> > > +#ifdef CONFIG_64BIT
> > > +#define clint_get_cycles() readq_relaxed(clint_timer_val)
> > > +#else
> > > +#define clint_get_cycles() readl_relaxed(clint_timer_val)
> > > +#define clint_get_cycles_hi()  readl_relaxed(((u32 *)clint_timer_val) + 
> > > 1)
> > > +#endif
> > > +
> > > +#ifdef CONFIG_64BIT
> > > +static u64 notrace clint_get_cycles64(void)
> > > +{
> > > +   return clint_get_cycles();
> > > +}
> > > +#else /* CONFIG_64BIT */
> > > +static u64 notrace clint_get

Re: [PATCH v4 2/4] clocksource/drivers: Add CLINT timer driver

2020-07-21 Thread Daniel Lezcano
On 21/07/2020 13:49, Anup Patel wrote:
> On Tue, Jul 21, 2020 at 4:32 PM Daniel Lezcano
>  wrote:
>>
>> On 17/07/2020 09:50, Anup Patel wrote:
>>> We add a separate CLINT timer driver for Linux RISC-V M-mode (i.e.
>>> RISC-V NoMMU kernel).
>>>
>>> The CLINT MMIO device provides three things:
>>> 1. 64bit free running counter register
>>> 2. 64bit per-CPU time compare registers
>>> 3. 32bit per-CPU inter-processor interrupt registers
>>>
>>> Unlike other timer devices, CLINT provides IPI registers along with
>>> timer registers. To use CLINT IPI registers, the CLINT timer driver
>>> provides IPI related callbacks to arch/riscv.
>>>
>>> Signed-off-by: Anup Patel 
>>> Tested-by: Emil Renner Berhing 
>>> ---
>>>  drivers/clocksource/Kconfig   |   9 ++
>>>  drivers/clocksource/Makefile  |   1 +
>>>  drivers/clocksource/timer-clint.c | 231 ++
>>>  include/linux/cpuhotplug.h|   1 +
>>>  4 files changed, 242 insertions(+)
>>>  create mode 100644 drivers/clocksource/timer-clint.c
>>>
>>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>>> index 91418381fcd4..e1ce0d510a03 100644
>>> --- a/drivers/clocksource/Kconfig
>>> +++ b/drivers/clocksource/Kconfig
>>> @@ -658,6 +658,15 @@ config RISCV_TIMER
>>> is accessed via both the SBI and the rdcycle instruction.  This is
>>> required for all RISC-V systems.
>>>
>>> +config CLINT_TIMER
>>> + bool "Timer for the RISC-V platform"
>>> + depends on GENERIC_SCHED_CLOCK && RISCV_M_MODE
>>> + select TIMER_PROBE
>>> + select TIMER_OF
>>> + help
>>> +   This option enables the CLINT timer for RISC-V systems. The CLINT
>>> +   driver is usually used for NoMMU RISC-V systems.
>>
>> V3 has a comment about fixing the Kconfig option.
> 
> I have removed "default y" from the Kconfig option as-per your suggestions.
> 
> I looked at other Timer Kconfig options. Most of them have menuconfig name.
> Also, we can certainly have different timer MMIO timer drivers in future. Do
> you still insist on making this kconfig option totally silent ??

Yes, and there is an effort to change the entries to be silent as much
as possible.

Just add:

bool "Timer for the RISC-V platform" if COMPILE_TEST

and remove the RISCV_M_MODE dependency.

Or alternatively:

replace the RISCV_M_MODE dependency with COMPILE_TEST

The goal is to be able to compile the driver on different platforms for
compilation test covering.

Then when more mmio drivers will added we will figure out.

>> [ ... ]
>>
>>> +{
>>> + bool *registered = per_cpu_ptr(&clint_clock_event_registered, cpu);
>>> + struct clock_event_device *ce = per_cpu_ptr(&clint_clock_event, cpu);
>>> +
>>> + if (!(*registered)) {
>>> + ce->cpumask = cpumask_of(cpu);
>>> + clockevents_config_and_register(ce, clint_timer_freq, 200,
>>> +  ULONG_MAX);
>>> + *registered = true;
>>> + }
>>
>>
>> I was unsure about the clockevents_config_and_register() multiple calls
>> when doing the comment. It seems like it is fine to call it several
>> times and that is done in several places like riscv or arch_arm_timer.
>>
>> It is probably safe to drop the 'registered' code here, sorry for the
>> confusion.
> 
> Okay, will revert these changes.
> 
>>
>>> + enable_percpu_irq(clint_timer_irq,
>>> +   irq_get_trigger_type(clint_timer_irq));
>>> + return 0;
>>> +}
>>> +
>>
>> [ ... ]
>>
>>
>> --
>>  Linaro.org │ Open source software for ARM SoCs
>>
>> Follow Linaro:   Facebook |
>>  Twitter |
>>  Blog
> 
> Regards,
> Anup
> 


-- 
 Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog


Re: [PATCH v4 2/4] clocksource/drivers: Add CLINT timer driver

2020-07-21 Thread Anup Patel
On Tue, Jul 21, 2020 at 4:32 PM Daniel Lezcano
 wrote:
>
> On 17/07/2020 09:50, Anup Patel wrote:
> > We add a separate CLINT timer driver for Linux RISC-V M-mode (i.e.
> > RISC-V NoMMU kernel).
> >
> > The CLINT MMIO device provides three things:
> > 1. 64bit free running counter register
> > 2. 64bit per-CPU time compare registers
> > 3. 32bit per-CPU inter-processor interrupt registers
> >
> > Unlike other timer devices, CLINT provides IPI registers along with
> > timer registers. To use CLINT IPI registers, the CLINT timer driver
> > provides IPI related callbacks to arch/riscv.
> >
> > Signed-off-by: Anup Patel 
> > Tested-by: Emil Renner Berhing 
> > ---
> >  drivers/clocksource/Kconfig   |   9 ++
> >  drivers/clocksource/Makefile  |   1 +
> >  drivers/clocksource/timer-clint.c | 231 ++
> >  include/linux/cpuhotplug.h|   1 +
> >  4 files changed, 242 insertions(+)
> >  create mode 100644 drivers/clocksource/timer-clint.c
> >
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > index 91418381fcd4..e1ce0d510a03 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -658,6 +658,15 @@ config RISCV_TIMER
> > is accessed via both the SBI and the rdcycle instruction.  This is
> > required for all RISC-V systems.
> >
> > +config CLINT_TIMER
> > + bool "Timer for the RISC-V platform"
> > + depends on GENERIC_SCHED_CLOCK && RISCV_M_MODE
> > + select TIMER_PROBE
> > + select TIMER_OF
> > + help
> > +   This option enables the CLINT timer for RISC-V systems. The CLINT
> > +   driver is usually used for NoMMU RISC-V systems.
>
> V3 has a comment about fixing the Kconfig option.

I have removed "default y" from the Kconfig option as-per your suggestions.

I looked at other Timer Kconfig options. Most of them have menuconfig name.
Also, we can certainly have different timer MMIO timer drivers in future. Do
you still insist on making this kconfig option totally silent ??

>
> [ ... ]
>
> > +{
> > + bool *registered = per_cpu_ptr(&clint_clock_event_registered, cpu);
> > + struct clock_event_device *ce = per_cpu_ptr(&clint_clock_event, cpu);
> > +
> > + if (!(*registered)) {
> > + ce->cpumask = cpumask_of(cpu);
> > + clockevents_config_and_register(ce, clint_timer_freq, 200,
> > +  ULONG_MAX);
> > + *registered = true;
> > + }
>
>
> I was unsure about the clockevents_config_and_register() multiple calls
> when doing the comment. It seems like it is fine to call it several
> times and that is done in several places like riscv or arch_arm_timer.
>
> It is probably safe to drop the 'registered' code here, sorry for the
> confusion.

Okay, will revert these changes.

>
> > + enable_percpu_irq(clint_timer_irq,
> > +   irq_get_trigger_type(clint_timer_irq));
> > + return 0;
> > +}
> > +
>
> [ ... ]
>
>
> --
>  Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:   Facebook |
>  Twitter |
>  Blog

Regards,
Anup


Re: [PATCH v4 2/4] clocksource/drivers: Add CLINT timer driver

2020-07-21 Thread Anup Patel
On Tue, Jul 21, 2020 at 6:41 AM Atish Patra  wrote:
>
> On Fri, Jul 17, 2020 at 12:52 AM Anup Patel  wrote:
> >
> > We add a separate CLINT timer driver for Linux RISC-V M-mode (i.e.
> > RISC-V NoMMU kernel).
> >
> > The CLINT MMIO device provides three things:
> > 1. 64bit free running counter register
> > 2. 64bit per-CPU time compare registers
> > 3. 32bit per-CPU inter-processor interrupt registers
> >
> > Unlike other timer devices, CLINT provides IPI registers along with
> > timer registers. To use CLINT IPI registers, the CLINT timer driver
> > provides IPI related callbacks to arch/riscv.
> >
> > Signed-off-by: Anup Patel 
> > Tested-by: Emil Renner Berhing 
> > ---
> >  drivers/clocksource/Kconfig   |   9 ++
> >  drivers/clocksource/Makefile  |   1 +
> >  drivers/clocksource/timer-clint.c | 231 ++
> >  include/linux/cpuhotplug.h|   1 +
> >  4 files changed, 242 insertions(+)
> >  create mode 100644 drivers/clocksource/timer-clint.c
> >
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > index 91418381fcd4..e1ce0d510a03 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -658,6 +658,15 @@ config RISCV_TIMER
> >   is accessed via both the SBI and the rdcycle instruction.  This is
> >   required for all RISC-V systems.
> >
> > +config CLINT_TIMER
> > +   bool "Timer for the RISC-V platform"
> > +   depends on GENERIC_SCHED_CLOCK && RISCV_M_MODE
> > +   select TIMER_PROBE
> > +   select TIMER_OF
> > +   help
> > + This option enables the CLINT timer for RISC-V systems. The CLINT
> > + driver is usually used for NoMMU RISC-V systems.
> > +
> >  config CSKY_MP_TIMER
> > bool "SMP Timer for the C-SKY platform" if COMPILE_TEST
> > depends on CSKY
> > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> > index bdda1a2e4097..18e700e703a0 100644
> > --- a/drivers/clocksource/Makefile
> > +++ b/drivers/clocksource/Makefile
> > @@ -87,6 +87,7 @@ obj-$(CONFIG_CLKSRC_ST_LPC)   += clksrc_st_lpc.o
> >  obj-$(CONFIG_X86_NUMACHIP) += numachip.o
> >  obj-$(CONFIG_ATCPIT100_TIMER)  += timer-atcpit100.o
> >  obj-$(CONFIG_RISCV_TIMER)  += timer-riscv.o
> > +obj-$(CONFIG_CLINT_TIMER)  += timer-clint.o
> >  obj-$(CONFIG_CSKY_MP_TIMER)+= timer-mp-csky.o
> >  obj-$(CONFIG_GX6605S_TIMER)+= timer-gx6605s.o
> >  obj-$(CONFIG_HYPERV_TIMER) += hyperv_timer.o
> > diff --git a/drivers/clocksource/timer-clint.c 
> > b/drivers/clocksource/timer-clint.c
> > new file mode 100644
> > index ..e1698efa73a1
> > --- /dev/null
> > +++ b/drivers/clocksource/timer-clint.c
> > @@ -0,0 +1,231 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2020 Western Digital Corporation or its affiliates.
> > + *
> > + * Most of the M-mode (i.e. NoMMU) RISC-V systems usually have a
> > + * CLINT MMIO timer device.
> > + */
> > +
> > +#define pr_fmt(fmt) "clint: " fmt
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#define CLINT_IPI_OFF  0
> > +#define CLINT_TIMER_CMP_OFF0x4000
> > +#define CLINT_TIMER_VAL_OFF0xbff8
> > +
> > +/* CLINT manages IPI and Timer for RISC-V M-mode  */
> > +static u32 __iomem *clint_ipi_base;
> > +static u64 __iomem *clint_timer_cmp;
> > +static u64 __iomem *clint_timer_val;
> > +static unsigned long clint_timer_freq;
> > +static unsigned int clint_timer_irq;
> > +
> > +static void clint_send_ipi(const struct cpumask *target)
> > +{
> > +   unsigned int cpu;
> > +
> > +   for_each_cpu(cpu, target)
> > +   writel(1, clint_ipi_base + cpuid_to_hartid_map(cpu));
> > +}
> > +
> > +static void clint_clear_ipi(void)
> > +{
> > +   writel(0, clint_ipi_base + cpuid_to_hartid_map(smp_processor_id()));
> > +}
> > +
> > +static struct riscv_ipi_ops clint_ipi_ops = {
> > +   .ipi_inject = clint_send_ipi,
> > +   .ipi_clear = clint_clear_ipi,
> > +};
> > +
> > +#ifdef CONFIG_64BIT
> > +#define clint_get_cycles() readq_relaxed(clint_timer_val)
> > +#else
> > +#define clint_get_cycles() readl_relaxed(clint_timer_val)
> > +#define clint_get_cycles_hi()  readl_relaxed(((u32 *)clint_timer_val) + 1)
> > +#endif
> > +
> > +#ifdef CONFIG_64BIT
> > +static u64 notrace clint_get_cycles64(void)
> > +{
> > +   return clint_get_cycles();
> > +}
> > +#else /* CONFIG_64BIT */
> > +static u64 notrace clint_get_cycles64(void)
> > +{
> > +   u32 hi, lo;
> > +
> > +   do {
> > +   hi = clint_get_cycles_hi();
> > +   lo = clint_get_cycles();
> > +   } while (hi != clint_get_cycles_hi());
> > +
> > +   return ((u64)hi << 32) | lo;
> > +}
> > +#endif /* CONFIG_64BIT */
> > +
> > +static u64 clint_rdtime(struct 

Re: [PATCH v4 2/4] clocksource/drivers: Add CLINT timer driver

2020-07-21 Thread Daniel Lezcano
On 17/07/2020 09:50, Anup Patel wrote:
> We add a separate CLINT timer driver for Linux RISC-V M-mode (i.e.
> RISC-V NoMMU kernel).
> 
> The CLINT MMIO device provides three things:
> 1. 64bit free running counter register
> 2. 64bit per-CPU time compare registers
> 3. 32bit per-CPU inter-processor interrupt registers
> 
> Unlike other timer devices, CLINT provides IPI registers along with
> timer registers. To use CLINT IPI registers, the CLINT timer driver
> provides IPI related callbacks to arch/riscv.
> 
> Signed-off-by: Anup Patel 
> Tested-by: Emil Renner Berhing 
> ---
>  drivers/clocksource/Kconfig   |   9 ++
>  drivers/clocksource/Makefile  |   1 +
>  drivers/clocksource/timer-clint.c | 231 ++
>  include/linux/cpuhotplug.h|   1 +
>  4 files changed, 242 insertions(+)
>  create mode 100644 drivers/clocksource/timer-clint.c
> 
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 91418381fcd4..e1ce0d510a03 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -658,6 +658,15 @@ config RISCV_TIMER
> is accessed via both the SBI and the rdcycle instruction.  This is
> required for all RISC-V systems.
>  
> +config CLINT_TIMER
> + bool "Timer for the RISC-V platform"
> + depends on GENERIC_SCHED_CLOCK && RISCV_M_MODE
> + select TIMER_PROBE
> + select TIMER_OF
> + help
> +   This option enables the CLINT timer for RISC-V systems. The CLINT
> +   driver is usually used for NoMMU RISC-V systems.

V3 has a comment about fixing the Kconfig option.

[ ... ]

> +{
> + bool *registered = per_cpu_ptr(&clint_clock_event_registered, cpu);
> + struct clock_event_device *ce = per_cpu_ptr(&clint_clock_event, cpu);
> +
> + if (!(*registered)) {
> + ce->cpumask = cpumask_of(cpu);
> + clockevents_config_and_register(ce, clint_timer_freq, 200,
> +  ULONG_MAX);
> + *registered = true;
> + }


I was unsure about the clockevents_config_and_register() multiple calls
when doing the comment. It seems like it is fine to call it several
times and that is done in several places like riscv or arch_arm_timer.

It is probably safe to drop the 'registered' code here, sorry for the
confusion.

> + enable_percpu_irq(clint_timer_irq,
> +   irq_get_trigger_type(clint_timer_irq));
> + return 0;
> +}
> +

[ ... ]


-- 
 Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog


Re: [PATCH v4 2/4] clocksource/drivers: Add CLINT timer driver

2020-07-20 Thread Atish Patra
On Fri, Jul 17, 2020 at 12:52 AM Anup Patel  wrote:
>
> We add a separate CLINT timer driver for Linux RISC-V M-mode (i.e.
> RISC-V NoMMU kernel).
>
> The CLINT MMIO device provides three things:
> 1. 64bit free running counter register
> 2. 64bit per-CPU time compare registers
> 3. 32bit per-CPU inter-processor interrupt registers
>
> Unlike other timer devices, CLINT provides IPI registers along with
> timer registers. To use CLINT IPI registers, the CLINT timer driver
> provides IPI related callbacks to arch/riscv.
>
> Signed-off-by: Anup Patel 
> Tested-by: Emil Renner Berhing 
> ---
>  drivers/clocksource/Kconfig   |   9 ++
>  drivers/clocksource/Makefile  |   1 +
>  drivers/clocksource/timer-clint.c | 231 ++
>  include/linux/cpuhotplug.h|   1 +
>  4 files changed, 242 insertions(+)
>  create mode 100644 drivers/clocksource/timer-clint.c
>
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 91418381fcd4..e1ce0d510a03 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -658,6 +658,15 @@ config RISCV_TIMER
>   is accessed via both the SBI and the rdcycle instruction.  This is
>   required for all RISC-V systems.
>
> +config CLINT_TIMER
> +   bool "Timer for the RISC-V platform"
> +   depends on GENERIC_SCHED_CLOCK && RISCV_M_MODE
> +   select TIMER_PROBE
> +   select TIMER_OF
> +   help
> + This option enables the CLINT timer for RISC-V systems. The CLINT
> + driver is usually used for NoMMU RISC-V systems.
> +
>  config CSKY_MP_TIMER
> bool "SMP Timer for the C-SKY platform" if COMPILE_TEST
> depends on CSKY
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index bdda1a2e4097..18e700e703a0 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -87,6 +87,7 @@ obj-$(CONFIG_CLKSRC_ST_LPC)   += clksrc_st_lpc.o
>  obj-$(CONFIG_X86_NUMACHIP) += numachip.o
>  obj-$(CONFIG_ATCPIT100_TIMER)  += timer-atcpit100.o
>  obj-$(CONFIG_RISCV_TIMER)  += timer-riscv.o
> +obj-$(CONFIG_CLINT_TIMER)  += timer-clint.o
>  obj-$(CONFIG_CSKY_MP_TIMER)+= timer-mp-csky.o
>  obj-$(CONFIG_GX6605S_TIMER)+= timer-gx6605s.o
>  obj-$(CONFIG_HYPERV_TIMER) += hyperv_timer.o
> diff --git a/drivers/clocksource/timer-clint.c 
> b/drivers/clocksource/timer-clint.c
> new file mode 100644
> index ..e1698efa73a1
> --- /dev/null
> +++ b/drivers/clocksource/timer-clint.c
> @@ -0,0 +1,231 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2020 Western Digital Corporation or its affiliates.
> + *
> + * Most of the M-mode (i.e. NoMMU) RISC-V systems usually have a
> + * CLINT MMIO timer device.
> + */
> +
> +#define pr_fmt(fmt) "clint: " fmt
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define CLINT_IPI_OFF  0
> +#define CLINT_TIMER_CMP_OFF0x4000
> +#define CLINT_TIMER_VAL_OFF0xbff8
> +
> +/* CLINT manages IPI and Timer for RISC-V M-mode  */
> +static u32 __iomem *clint_ipi_base;
> +static u64 __iomem *clint_timer_cmp;
> +static u64 __iomem *clint_timer_val;
> +static unsigned long clint_timer_freq;
> +static unsigned int clint_timer_irq;
> +
> +static void clint_send_ipi(const struct cpumask *target)
> +{
> +   unsigned int cpu;
> +
> +   for_each_cpu(cpu, target)
> +   writel(1, clint_ipi_base + cpuid_to_hartid_map(cpu));
> +}
> +
> +static void clint_clear_ipi(void)
> +{
> +   writel(0, clint_ipi_base + cpuid_to_hartid_map(smp_processor_id()));
> +}
> +
> +static struct riscv_ipi_ops clint_ipi_ops = {
> +   .ipi_inject = clint_send_ipi,
> +   .ipi_clear = clint_clear_ipi,
> +};
> +
> +#ifdef CONFIG_64BIT
> +#define clint_get_cycles() readq_relaxed(clint_timer_val)
> +#else
> +#define clint_get_cycles() readl_relaxed(clint_timer_val)
> +#define clint_get_cycles_hi()  readl_relaxed(((u32 *)clint_timer_val) + 1)
> +#endif
> +
> +#ifdef CONFIG_64BIT
> +static u64 notrace clint_get_cycles64(void)
> +{
> +   return clint_get_cycles();
> +}
> +#else /* CONFIG_64BIT */
> +static u64 notrace clint_get_cycles64(void)
> +{
> +   u32 hi, lo;
> +
> +   do {
> +   hi = clint_get_cycles_hi();
> +   lo = clint_get_cycles();
> +   } while (hi != clint_get_cycles_hi());
> +
> +   return ((u64)hi << 32) | lo;
> +}
> +#endif /* CONFIG_64BIT */
> +
> +static u64 clint_rdtime(struct clocksource *cs)
> +{
> +   return clint_get_cycles64();
> +}
> +
> +static struct clocksource clint_clocksource = {
> +   .name   = "clint_clocksource",
> +   .rating = 300,

nit: Not aligned with other structure members.

> +   .mask   = CLOCKSOURCE_MASK(64),
> +   .flags  = CLOCK_SOURCE_I

[PATCH v4 2/4] clocksource/drivers: Add CLINT timer driver

2020-07-17 Thread Anup Patel
We add a separate CLINT timer driver for Linux RISC-V M-mode (i.e.
RISC-V NoMMU kernel).

The CLINT MMIO device provides three things:
1. 64bit free running counter register
2. 64bit per-CPU time compare registers
3. 32bit per-CPU inter-processor interrupt registers

Unlike other timer devices, CLINT provides IPI registers along with
timer registers. To use CLINT IPI registers, the CLINT timer driver
provides IPI related callbacks to arch/riscv.

Signed-off-by: Anup Patel 
Tested-by: Emil Renner Berhing 
---
 drivers/clocksource/Kconfig   |   9 ++
 drivers/clocksource/Makefile  |   1 +
 drivers/clocksource/timer-clint.c | 231 ++
 include/linux/cpuhotplug.h|   1 +
 4 files changed, 242 insertions(+)
 create mode 100644 drivers/clocksource/timer-clint.c

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 91418381fcd4..e1ce0d510a03 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -658,6 +658,15 @@ config RISCV_TIMER
  is accessed via both the SBI and the rdcycle instruction.  This is
  required for all RISC-V systems.
 
+config CLINT_TIMER
+   bool "Timer for the RISC-V platform"
+   depends on GENERIC_SCHED_CLOCK && RISCV_M_MODE
+   select TIMER_PROBE
+   select TIMER_OF
+   help
+ This option enables the CLINT timer for RISC-V systems. The CLINT
+ driver is usually used for NoMMU RISC-V systems.
+
 config CSKY_MP_TIMER
bool "SMP Timer for the C-SKY platform" if COMPILE_TEST
depends on CSKY
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index bdda1a2e4097..18e700e703a0 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -87,6 +87,7 @@ obj-$(CONFIG_CLKSRC_ST_LPC)   += clksrc_st_lpc.o
 obj-$(CONFIG_X86_NUMACHIP) += numachip.o
 obj-$(CONFIG_ATCPIT100_TIMER)  += timer-atcpit100.o
 obj-$(CONFIG_RISCV_TIMER)  += timer-riscv.o
+obj-$(CONFIG_CLINT_TIMER)  += timer-clint.o
 obj-$(CONFIG_CSKY_MP_TIMER)+= timer-mp-csky.o
 obj-$(CONFIG_GX6605S_TIMER)+= timer-gx6605s.o
 obj-$(CONFIG_HYPERV_TIMER) += hyperv_timer.o
diff --git a/drivers/clocksource/timer-clint.c 
b/drivers/clocksource/timer-clint.c
new file mode 100644
index ..e1698efa73a1
--- /dev/null
+++ b/drivers/clocksource/timer-clint.c
@@ -0,0 +1,231 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020 Western Digital Corporation or its affiliates.
+ *
+ * Most of the M-mode (i.e. NoMMU) RISC-V systems usually have a
+ * CLINT MMIO timer device.
+ */
+
+#define pr_fmt(fmt) "clint: " fmt
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define CLINT_IPI_OFF  0
+#define CLINT_TIMER_CMP_OFF0x4000
+#define CLINT_TIMER_VAL_OFF0xbff8
+
+/* CLINT manages IPI and Timer for RISC-V M-mode  */
+static u32 __iomem *clint_ipi_base;
+static u64 __iomem *clint_timer_cmp;
+static u64 __iomem *clint_timer_val;
+static unsigned long clint_timer_freq;
+static unsigned int clint_timer_irq;
+
+static void clint_send_ipi(const struct cpumask *target)
+{
+   unsigned int cpu;
+
+   for_each_cpu(cpu, target)
+   writel(1, clint_ipi_base + cpuid_to_hartid_map(cpu));
+}
+
+static void clint_clear_ipi(void)
+{
+   writel(0, clint_ipi_base + cpuid_to_hartid_map(smp_processor_id()));
+}
+
+static struct riscv_ipi_ops clint_ipi_ops = {
+   .ipi_inject = clint_send_ipi,
+   .ipi_clear = clint_clear_ipi,
+};
+
+#ifdef CONFIG_64BIT
+#define clint_get_cycles() readq_relaxed(clint_timer_val)
+#else
+#define clint_get_cycles() readl_relaxed(clint_timer_val)
+#define clint_get_cycles_hi()  readl_relaxed(((u32 *)clint_timer_val) + 1)
+#endif
+
+#ifdef CONFIG_64BIT
+static u64 notrace clint_get_cycles64(void)
+{
+   return clint_get_cycles();
+}
+#else /* CONFIG_64BIT */
+static u64 notrace clint_get_cycles64(void)
+{
+   u32 hi, lo;
+
+   do {
+   hi = clint_get_cycles_hi();
+   lo = clint_get_cycles();
+   } while (hi != clint_get_cycles_hi());
+
+   return ((u64)hi << 32) | lo;
+}
+#endif /* CONFIG_64BIT */
+
+static u64 clint_rdtime(struct clocksource *cs)
+{
+   return clint_get_cycles64();
+}
+
+static struct clocksource clint_clocksource = {
+   .name   = "clint_clocksource",
+   .rating = 300,
+   .mask   = CLOCKSOURCE_MASK(64),
+   .flags  = CLOCK_SOURCE_IS_CONTINUOUS,
+   .read   = clint_rdtime,
+};
+
+static int clint_clock_next_event(unsigned long delta,
+  struct clock_event_device *ce)
+{
+   void __iomem *r = clint_timer_cmp +
+ cpuid_to_hartid_map(smp_processor_id());
+
+   csr_set(CSR_IE, IE_TIE);
+   writeq_relaxed(clint_get_cycles64() + delta, r);
+