Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i0ad843c9:Fastmail
Subject: Re: [PATCH 2/2] clocksource: timer-riscv: Set CLOCK_EVT_FEAT_C3STOP
 based on DT property
To:     Anup Patel <apatel@ventanamicro.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        Thomas Gleixner <tglx@linutronix.de>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Atish Patra <atishp@atishpatra.org>,
        Anup Patel <anup@brainfault.org>, devicetree@vger.kernel.org,
        linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org
References: <20220719054729.2224766-1-apatel@ventanamicro.com>
 <20220719054729.2224766-3-apatel@ventanamicro.com>
From:   Samuel Holland <samuel@sholland.org>
Message-ID: <315d9850-e660-f71e-2321-b0684f7df950@sholland.org>
Date:   Tue, 19 Jul 2022 01:42:47 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.6.0
MIME-Version: 1.0
In-Reply-To: <20220719054729.2224766-3-apatel@ventanamicro.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Anup,

On 7/19/22 12:47 AM, Anup Patel wrote:
> We should set CLOCK_EVT_FEAT_C3STOP for a clock_event_device only when
> riscv,timer-always-on DT property is not present for the corresponding
> CPU.

The timer maintaining its context (and continuing to count) during non-retentive
CPU suspend is not sufficient to drop CLOCK_EVT_FEAT_C3STOP.

Another requirement is that the timer interrupt is generated and routed outside
the CPU's power/reset domain, to whatever hardware is responsible for turning
the CPU back on. It does not matter if the timer interrupt fires, if that
interrupt cannot wake up the CPU.

So something closer to "riscv,timer-can-wake-cpu" would be a more accurate
property name for how you are using it.

And even then, that ability is a property of the SBI implementation, not just
the hardware. In the motivating example for the flag (Allwinner D1), the CLINT
cannot wake the CPU from reset, but the SoC contains other MMIO timers that can.
So the capability of the SBI timer extension depends on which timer hardware the
SBI implementation chooses to use. So I am not sure that the property belongs in
the CPU node.

Maybe it makes sense to report this capability via a function in the SBI timer
extension?

Regards,
Samuel

> This way CLOCK_EVT_FEAT_C3STOP feature is set for clock_event_device
> based on RISC-V platform capabilities rather than having it set for
> all RISC-V platforms.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  drivers/clocksource/timer-riscv.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
> index 593d5a957b69..3015324f2b59 100644
> --- a/drivers/clocksource/timer-riscv.c
> +++ b/drivers/clocksource/timer-riscv.c
> @@ -34,7 +34,7 @@ static int riscv_clock_next_event(unsigned long delta,
>  static unsigned int riscv_clock_event_irq;
>  static DEFINE_PER_CPU(struct clock_event_device, riscv_clock_event) = {
>  	.name			= "riscv_timer_clockevent",
> -	.features		= CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_C3STOP,
> +	.features		= CLOCK_EVT_FEAT_ONESHOT,
>  	.rating			= 100,
>  	.set_next_event		= riscv_clock_next_event,
>  };
> @@ -65,9 +65,13 @@ static struct clocksource riscv_clocksource = {
>  static int riscv_timer_starting_cpu(unsigned int cpu)
>  {
>  	struct clock_event_device *ce = per_cpu_ptr(&riscv_clock_event, cpu);
> +	struct device_node *np = of_get_cpu_node(cpu, NULL);
>  
>  	ce->cpumask = cpumask_of(cpu);
>  	ce->irq = riscv_clock_event_irq;
> +	if (!of_property_read_bool(np, "riscv,timer-always-on"))
> +		ce->features |= CLOCK_EVT_FEAT_C3STOP;
> +	of_node_put(np);
>  	clockevents_config_and_register(ce, riscv_timebase, 100, 0x7fffffff);
>  
>  	enable_percpu_irq(riscv_clock_event_irq,
>