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

[Jisheng Zhang]

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@xxxxxxxxxx>
> > > > ---
> > > >   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