Mailing List Archive: [PATCH 5/5] clocksource/drivers/timer-clint: Add T-Head C9xx clint support

[PATCH 5/5] clocksource/drivers/timer-clint: Add T-Head C9xx clint support

Mar 25, 2024, 9:40 AM

Post #1 of 6 (11 views)

The mtimecmp in T-Head C9xx clint only supports 32bit read/write,
implement such support.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
drivers/clocksource/timer-clint.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)

diff --git a/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer-clint.c
index 4537c77e623c..71188732e8a3 100644
--- a/drivers/clocksource/timer-clint.c
+++ b/drivers/clocksource/timer-clint.c
@@ -34,6 +34,7 @@ static unsigned int clint_ipi_irq;
static u64 __iomem *clint_timer_cmp;
static unsigned long clint_timer_freq;
static unsigned int clint_timer_irq;
+static bool is_c900_clint;

#ifdef CONFIG_SMP
static void clint_send_ipi(unsigned int cpu)
@@ -88,6 +89,19 @@ static int clint_clock_next_event(unsigned long delta,
return 0;
}

+static int c900_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());
+ u64 val = clint_get_cycles64() + delta;
+
+ csr_set(CSR_IE, IE_TIE);
+ writel_relaxed(val, r);
+ writel_relaxed(val >> 32, r + 4);
+ return 0;
+}
+
static DEFINE_PER_CPU(struct clock_event_device, clint_clock_event) = {
.name = "clint_clockevent",
.features = CLOCK_EVT_FEAT_ONESHOT,
@@ -99,6 +113,9 @@ static int clint_timer_starting_cpu(unsigned int cpu)
{
struct clock_event_device *ce = per_cpu_ptr(&clint_clock_event, cpu);

+ if (is_c900_clint)
+ ce->set_next_event = c900_clint_clock_next_event;
+
ce->cpumask = cpumask_of(cpu);
clockevents_config_and_register(ce, clint_timer_freq, 100, ULONG_MAX);

@@ -233,5 +250,12 @@ static int __init clint_timer_init_dt(struct device_node *np)
return rc;
}

+static int __init c900_clint_timer_init_dt(struct device_node *np)
+{
+ is_c900_clint = true;
+ return clint_timer_init_dt(np);
+}
+
TIMER_OF_DECLARE(clint_timer, "riscv,clint0", clint_timer_init_dt);
TIMER_OF_DECLARE(clint_timer1, "sifive,clint0", clint_timer_init_dt);
+TIMER_OF_DECLARE(clint_timer2, "thead,c900-clint", clint_timer_init_dt);
--
2.43.0

Re: [PATCH 5/5] clocksource/drivers/timer-clint: Add T-Head C9xx clint support [ In reply to ]

ganboing at gmail

Mar 25, 2024, 3:22 PM

Post #2 of 6 (11 views)

Permalink

On 3/25/24 9:40 AM, Jisheng Zhang wrote:
> The mtimecmp in T-Head C9xx clint only supports 32bit read/write,
> implement such support.
>
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
> drivers/clocksource/timer-clint.c | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer-clint.c
> index 4537c77e623c..71188732e8a3 100644
> --- a/drivers/clocksource/timer-clint.c
> +++ b/drivers/clocksource/timer-clint.c
> @@ -34,6 +34,7 @@ static unsigned int clint_ipi_irq;
> static u64 __iomem *clint_timer_cmp;
> static unsigned long clint_timer_freq;
> static unsigned int clint_timer_irq;
> +static bool is_c900_clint;
>
> #ifdef CONFIG_SMP
> static void clint_send_ipi(unsigned int cpu)
> @@ -88,6 +89,19 @@ static int clint_clock_next_event(unsigned long delta,
> return 0;
> }
>
> +static int c900_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());
> + u64 val = clint_get_cycles64() + delta;
> +
> + csr_set(CSR_IE, IE_TIE);
Perhaps you should do a writel_relaxed(-1, r) here. just like openSBI, because the update
to mtimecmp is now split into 2 parts.
https://github.com/riscv-software-src/opensbi/blob/v1.4/lib/utils/timer/aclint_mtimer.c#L54

> + writel_relaxed(val, r);
> + writel_relaxed(val >> 32, r + 4);
> + return 0;
> +}
> +> static DEFINE_PER_CPU(struct clock_event_device, clint_clock_event) = {
> .name = "clint_clockevent",
> .features = CLOCK_EVT_FEAT_ONESHOT,
> @@ -99,6 +113,9 @@ static int clint_timer_starting_cpu(unsigned int cpu)
> {
> struct clock_event_device *ce = per_cpu_ptr(&clint_clock_event, cpu);
>
> + if (is_c900_clint)
> + ce->set_next_event = c900_clint_clock_next_event;
> +
> ce->cpumask = cpumask_of(cpu);
> clockevents_config_and_register(ce, clint_timer_freq, 100, ULONG_MAX);
>
> @@ -233,5 +250,12 @@ static int __init clint_timer_init_dt(struct device_node *np)
> return rc;
> }
>
> +static int __init c900_clint_timer_init_dt(struct device_node *np)
> +{
> + is_c900_clint = true;
> + return clint_timer_init_dt(np);
> +}
> +
> TIMER_OF_DECLARE(clint_timer, "riscv,clint0", clint_timer_init_dt);
> TIMER_OF_DECLARE(clint_timer1, "sifive,clint0", clint_timer_init_dt);
> +TIMER_OF_DECLARE(clint_timer2, "thead,c900-clint", clint_timer_init_dt);
>
Better use a more generic term to describe the fact that mtimecmp doesn't support
64-bit mmio, just like what openSBI is currently doing, instead of making it c900 specific:

https://github.com/riscv-software-src/opensbi/blob/v1.4/lib/utils/timer/fdt_timer_mtimer.c#L152

Then your `is_c900_clint` becomes something like `timecmp_64bit_mmio`.

Bo

Re: [PATCH 5/5] clocksource/drivers/timer-clint: Add T-Head C9xx clint support [ In reply to ]

jszhang at kernel

Mar 25, 2024, 6:25 PM

Post #3 of 6 (11 views)

Permalink

On Mon, Mar 25, 2024 at 03:22:11PM -0700, Bo Gan wrote:
> On 3/25/24 9:40 AM, Jisheng Zhang wrote:
> > The mtimecmp in T-Head C9xx clint only supports 32bit read/write,
> > implement such support.
> >
> > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > ---
> > drivers/clocksource/timer-clint.c | 24 ++++++++++++++++++++++++
> > 1 file changed, 24 insertions(+)
> >
> > diff --git a/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer-clint.c
> > index 4537c77e623c..71188732e8a3 100644
> > --- a/drivers/clocksource/timer-clint.c
> > +++ b/drivers/clocksource/timer-clint.c
> > @@ -34,6 +34,7 @@ static unsigned int clint_ipi_irq;
> > static u64 __iomem *clint_timer_cmp;
> > static unsigned long clint_timer_freq;
> > static unsigned int clint_timer_irq;
> > +static bool is_c900_clint;
> > #ifdef CONFIG_SMP
> > static void clint_send_ipi(unsigned int cpu)
> > @@ -88,6 +89,19 @@ static int clint_clock_next_event(unsigned long delta,
> > return 0;
> > }
> > +static int c900_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());
> > + u64 val = clint_get_cycles64() + delta;
> > +
> > + csr_set(CSR_IE, IE_TIE);
> Perhaps you should do a writel_relaxed(-1, r) here. just like openSBI, because the update
> to mtimecmp is now split into 2 parts.
> https://github.com/riscv-software-src/opensbi/blob/v1.4/lib/utils/timer/aclint_mtimer.c#L54

Thanks, I also noticed the mtimecmp update on 32bit platforms doesn't
strictly follow the riscv spec:

# New comparand is in a1:a0.
li t0, -1
la t1, mtimecmp
sw t0, 0(t1) # No smaller than old value.
sw a1, 4(t1) # No smaller than new value.
sw a0, 0(t1) # New value.

So A new bug fix patch will be added in v2.

>
> > + writel_relaxed(val, r);
> > + writel_relaxed(val >> 32, r + 4);
> > + return 0;
> > +}
> > +> static DEFINE_PER_CPU(struct clock_event_device, clint_clock_event) = {
> > .name = "clint_clockevent",
> > .features = CLOCK_EVT_FEAT_ONESHOT,
> > @@ -99,6 +113,9 @@ static int clint_timer_starting_cpu(unsigned int cpu)
> > {
> > struct clock_event_device *ce = per_cpu_ptr(&clint_clock_event, cpu);
> > + if (is_c900_clint)
> > + ce->set_next_event = c900_clint_clock_next_event;
> > +
> > ce->cpumask = cpumask_of(cpu);
> > clockevents_config_and_register(ce, clint_timer_freq, 100, ULONG_MAX);
> > @@ -233,5 +250,12 @@ static int __init clint_timer_init_dt(struct device_node *np)
> > return rc;
> > }
> > +static int __init c900_clint_timer_init_dt(struct device_node *np)
> > +{
> > + is_c900_clint = true;
> > + return clint_timer_init_dt(np);
> > +}
> > +
> > TIMER_OF_DECLARE(clint_timer, "riscv,clint0", clint_timer_init_dt);
> > TIMER_OF_DECLARE(clint_timer1, "sifive,clint0", clint_timer_init_dt);
> > +TIMER_OF_DECLARE(clint_timer2, "thead,c900-clint", clint_timer_init_dt);
> >
> Better use a more generic term to describe the fact that mtimecmp doesn't support
> 64-bit mmio, just like what openSBI is currently doing, instead of making it c900 specific:

This has been mentioned in commit msg, but adding a code comment seems fine.
>
> https://github.com/riscv-software-src/opensbi/blob/v1.4/lib/utils/timer/fdt_timer_mtimer.c#L152
>
> Then your `is_c900_clint` becomes something like `timecmp_64bit_mmio`.
>
> Bo

Re: [PATCH 5/5] clocksource/drivers/timer-clint: Add T-Head C9xx clint support [ In reply to ]

jszhang at kernel

Mar 25, 2024, 6:31 PM

Post #4 of 6 (11 views)

Permalink

On Tue, Mar 26, 2024 at 09:25:09AM +0800, Jisheng Zhang wrote:
> On Mon, Mar 25, 2024 at 03:22:11PM -0700, Bo Gan wrote:
> > On 3/25/24 9:40 AM, Jisheng Zhang wrote:
> > > The mtimecmp in T-Head C9xx clint only supports 32bit read/write,
> > > implement such support.
> > >
> > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > > ---
> > > drivers/clocksource/timer-clint.c | 24 ++++++++++++++++++++++++
> > > 1 file changed, 24 insertions(+)
> > >
> > > diff --git a/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer-clint.c
> > > index 4537c77e623c..71188732e8a3 100644
> > > --- a/drivers/clocksource/timer-clint.c
> > > +++ b/drivers/clocksource/timer-clint.c
> > > @@ -34,6 +34,7 @@ static unsigned int clint_ipi_irq;
> > > static u64 __iomem *clint_timer_cmp;
> > > static unsigned long clint_timer_freq;
> > > static unsigned int clint_timer_irq;
> > > +static bool is_c900_clint;
> > > #ifdef CONFIG_SMP
> > > static void clint_send_ipi(unsigned int cpu)
> > > @@ -88,6 +89,19 @@ static int clint_clock_next_event(unsigned long delta,
> > > return 0;
> > > }
> > > +static int c900_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());
> > > + u64 val = clint_get_cycles64() + delta;
> > > +
> > > + csr_set(CSR_IE, IE_TIE);
> > Perhaps you should do a writel_relaxed(-1, r) here. just like openSBI, because the update
> > to mtimecmp is now split into 2 parts.
> > https://github.com/riscv-software-src/opensbi/blob/v1.4/lib/utils/timer/aclint_mtimer.c#L54
>
> Thanks, I also noticed the mtimecmp update on 32bit platforms doesn't
> strictly follow the riscv spec:
>
> # New comparand is in a1:a0.
> li t0, -1
> la t1, mtimecmp
> sw t0, 0(t1) # No smaller than old value.
> sw a1, 4(t1) # No smaller than new value.
> sw a0, 0(t1) # New value.
>
> So A new bug fix patch will be added in v2.
>

wait, I found a similar bug in timer-riscv.c, and since these are fixes,
I'd like to send fixes as a seperate series. I'm cooking the patches

>
> >
> > > + writel_relaxed(val, r);
> > > + writel_relaxed(val >> 32, r + 4);
> > > + return 0;
> > > +}
> > > +> static DEFINE_PER_CPU(struct clock_event_device, clint_clock_event) = {
> > > .name = "clint_clockevent",
> > > .features = CLOCK_EVT_FEAT_ONESHOT,
> > > @@ -99,6 +113,9 @@ static int clint_timer_starting_cpu(unsigned int cpu)
> > > {
> > > struct clock_event_device *ce = per_cpu_ptr(&clint_clock_event, cpu);
> > > + if (is_c900_clint)
> > > + ce->set_next_event = c900_clint_clock_next_event;
> > > +
> > > ce->cpumask = cpumask_of(cpu);
> > > clockevents_config_and_register(ce, clint_timer_freq, 100, ULONG_MAX);
> > > @@ -233,5 +250,12 @@ static int __init clint_timer_init_dt(struct device_node *np)
> > > return rc;
> > > }
> > > +static int __init c900_clint_timer_init_dt(struct device_node *np)
> > > +{
> > > + is_c900_clint = true;
> > > + return clint_timer_init_dt(np);
> > > +}
> > > +
> > > TIMER_OF_DECLARE(clint_timer, "riscv,clint0", clint_timer_init_dt);
> > > TIMER_OF_DECLARE(clint_timer1, "sifive,clint0", clint_timer_init_dt);
> > > +TIMER_OF_DECLARE(clint_timer2, "thead,c900-clint", clint_timer_init_dt);
> > >
> > Better use a more generic term to describe the fact that mtimecmp doesn't support
> > 64-bit mmio, just like what openSBI is currently doing, instead of making it c900 specific:
>
> This has been mentioned in commit msg, but adding a code comment seems fine.
> >
> > https://github.com/riscv-software-src/opensbi/blob/v1.4/lib/utils/timer/fdt_timer_mtimer.c#L152
> >
> > Then your `is_c900_clint` becomes something like `timecmp_64bit_mmio`.
> >
> > Bo

Re: [PATCH 5/5] clocksource/drivers/timer-clint: Add T-Head C9xx clint support [ In reply to ]

jszhang at kernel

Mar 26, 2024, 9:33 AM

Post #5 of 6 (11 views)

Permalink

On Tue, Mar 26, 2024 at 09:31:14AM +0800, Jisheng Zhang wrote:
> On Tue, Mar 26, 2024 at 09:25:09AM +0800, Jisheng Zhang wrote:
> > On Mon, Mar 25, 2024 at 03:22:11PM -0700, Bo Gan wrote:
> > > On 3/25/24 9:40 AM, Jisheng Zhang wrote:
> > > > The mtimecmp in T-Head C9xx clint only supports 32bit read/write,
> > > > implement such support.
> > > >
> > > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > > > ---
> > > > drivers/clocksource/timer-clint.c | 24 ++++++++++++++++++++++++
> > > > 1 file changed, 24 insertions(+)
> > > >
> > > > diff --git a/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer-clint.c
> > > > index 4537c77e623c..71188732e8a3 100644
> > > > --- a/drivers/clocksource/timer-clint.c
> > > > +++ b/drivers/clocksource/timer-clint.c
> > > > @@ -34,6 +34,7 @@ static unsigned int clint_ipi_irq;
> > > > static u64 __iomem *clint_timer_cmp;
> > > > static unsigned long clint_timer_freq;
> > > > static unsigned int clint_timer_irq;
> > > > +static bool is_c900_clint;
> > > > #ifdef CONFIG_SMP
> > > > static void clint_send_ipi(unsigned int cpu)
> > > > @@ -88,6 +89,19 @@ static int clint_clock_next_event(unsigned long delta,
> > > > return 0;
> > > > }
> > > > +static int c900_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());
> > > > + u64 val = clint_get_cycles64() + delta;
> > > > +
> > > > + csr_set(CSR_IE, IE_TIE);
> > > Perhaps you should do a writel_relaxed(-1, r) here. just like openSBI, because the update
> > > to mtimecmp is now split into 2 parts.
> > > https://github.com/riscv-software-src/opensbi/blob/v1.4/lib/utils/timer/aclint_mtimer.c#L54

Hi,

The reason of "writel_relaxed(-1, r)" is to avoid spurious irq, this is
well explained by riscv spec.
After more thoughts, this is not needed here in linux kernel because
set_next_event() is only called in ISR, so the irq has been disabled,
we don't suffer from spurious irq problem.

Thanks
> >
> > Thanks, I also noticed the mtimecmp update on 32bit platforms doesn't
> > strictly follow the riscv spec:
> >
> > # New comparand is in a1:a0.
> > li t0, -1
> > la t1, mtimecmp
> > sw t0, 0(t1) # No smaller than old value.
> > sw a1, 4(t1) # No smaller than new value.
> > sw a0, 0(t1) # New value.
> >
> > So A new bug fix patch will be added in v2.
> >
>
> wait, I found a similar bug in timer-riscv.c, and since these are fixes,
> I'd like to send fixes as a seperate series. I'm cooking the patches
>
> >
> > >
> > > > + writel_relaxed(val, r);
> > > > + writel_relaxed(val >> 32, r + 4);
> > > > + return 0;
> > > > +}
> > > > +> static DEFINE_PER_CPU(struct clock_event_device, clint_clock_event) = {
> > > > .name = "clint_clockevent",
> > > > .features = CLOCK_EVT_FEAT_ONESHOT,
> > > > @@ -99,6 +113,9 @@ static int clint_timer_starting_cpu(unsigned int cpu)
> > > > {
> > > > struct clock_event_device *ce = per_cpu_ptr(&clint_clock_event, cpu);
> > > > + if (is_c900_clint)
> > > > + ce->set_next_event = c900_clint_clock_next_event;
> > > > +
> > > > ce->cpumask = cpumask_of(cpu);
> > > > clockevents_config_and_register(ce, clint_timer_freq, 100, ULONG_MAX);
> > > > @@ -233,5 +250,12 @@ static int __init clint_timer_init_dt(struct device_node *np)
> > > > return rc;
> > > > }
> > > > +static int __init c900_clint_timer_init_dt(struct device_node *np)
> > > > +{
> > > > + is_c900_clint = true;
> > > > + return clint_timer_init_dt(np);
> > > > +}
> > > > +
> > > > TIMER_OF_DECLARE(clint_timer, "riscv,clint0", clint_timer_init_dt);
> > > > TIMER_OF_DECLARE(clint_timer1, "sifive,clint0", clint_timer_init_dt);
> > > > +TIMER_OF_DECLARE(clint_timer2, "thead,c900-clint", clint_timer_init_dt);
> > > >
> > > Better use a more generic term to describe the fact that mtimecmp doesn't support
> > > 64-bit mmio, just like what openSBI is currently doing, instead of making it c900 specific:
> >
> > This has been mentioned in commit msg, but adding a code comment seems fine.
> > >
> > > https://github.com/riscv-software-src/opensbi/blob/v1.4/lib/utils/timer/fdt_timer_mtimer.c#L152
> > >
> > > Then your `is_c900_clint` becomes something like `timecmp_64bit_mmio`.
> > >
> > > Bo

Re: [PATCH 5/5] clocksource/drivers/timer-clint: Add T-Head C9xx clint support [ In reply to ]

lkp at intel

Mar 27, 2024, 3:53 PM

Post #6 of 6 (11 views)

Permalink

Hi Jisheng,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.9-rc1 next-20240327]
[cannot apply to tip/timers/core]
[.If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Jisheng-Zhang/riscv-nommu-remove-PAGE_OFFSET-hardcoding/20240326-021401
base: linus/master
patch link: https://lore.kernel.org/r/20240325164021.3229-6-jszhang%40kernel.org
patch subject: [PATCH 5/5] clocksource/drivers/timer-clint: Add T-Head C9xx clint support
config: riscv-randconfig-r131-20240326 (https://download.01.org/0day-ci/archive/20240328/202403280636.SLv93x6B-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20240328/202403280636.SLv93x6B-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403280636.SLv93x6B-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/clocksource/timer-clint.c:253:19: warning: 'c900_clint_timer_init_dt' defined but not used [-Wunused-function]
253 | static int __init c900_clint_timer_init_dt(struct device_node *np)
| ^~~~~~~~~~~~~~~~~~~~~~~~

vim +/c900_clint_timer_init_dt +253 drivers/clocksource/timer-clint.c

252
> 253 static int __init c900_clint_timer_init_dt(struct device_node *np)
254 {
255 is_c900_clint = true;
256 return clint_timer_init_dt(np);
257 }
258

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki