Received-SPF: pass (google.com: domain of linux-kernel+bounces-118092-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Subject: Re: [PATCH 5/5] clocksource/drivers/timer-clint: Add T-Head C9xx
 clint support
To: Jisheng Zhang <jszhang@kernel.org>,
 Paul Walmsley <paul.walmsley@sifive.com>, Palmer Dabbelt
 <palmer@dabbelt.com>, Albert Ou <aou@eecs.berkeley.edu>,
 Daniel Lezcano <daniel.lezcano@linaro.org>,
 Thomas Gleixner <tglx@linutronix.de>
Cc: linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org
References: <20240325164021.3229-1-jszhang@kernel.org>
 <20240325164021.3229-6-jszhang@kernel.org>
From: Bo Gan <ganboing@gmail.com>
Message-ID: <72eac56e-61d4-e42f-cfbd-8bcc35ed7bb6@gmail.com>
Date: Mon, 25 Mar 2024 15:22:11 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <20240325164021.3229-6-jszhang@kernel.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit

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