Date:   Thu, 09 Feb 2023 15:39:32 -0800 (PST)
Subject:     Re: [PATCH] clocksource/drivers/riscv: Refuse to probe on T-Head
In-Reply-To: <87a0a7c9-6069-b043-2fc2-8d1bc05ddb1a@sholland.org>
CC:     linux-riscv@lists.infradead.org, daniel.lezcano@linaro.org,
        tglx@linutronix.de, Paul Walmsley <paul.walmsley@sifive.com>,
        aou@eecs.berkeley.edu, linux-kernel@vger.kernel.org,
        linux@rivosinc.com
From:   Palmer Dabbelt <palmer@rivosinc.com>
To:     samuel@sholland.org
Message-ID: <mhng-25d39bba-2df2-4976-a3c0-b867a54cb61c@palmer-ri-x1c9a>
Mime-Version: 1.0 (MHng)
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Thu, 09 Feb 2023 15:35:53 PST (-0800), samuel@sholland.org wrote:
> Hi Palmer,
>
> On 2/9/23 17:23, Palmer Dabbelt wrote:
>> From: Palmer Dabbelt <palmer@rivosinc.com>
>>
>> As of d9f15a9de44a ("Revert "clocksource/drivers/riscv: Events are
>> stopped during CPU suspend"") this driver no longer functions correctly
>> for the T-Head firmware.  That shouldn't impact any users, as we've got
>
> The current situation is that the C9xx CLINT binding was just accepted,
> so the CLINT is not yet described in any devicetree. So at least with
> upstream OpenSBI, which needs the CLINT DT node, the SBI timer extension
> never worked at all.
>
>> a functioning driver that's higher priority, but let's just be safe and
>> ban it from probing at all.
>>
>> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
>> ---
>> This feel super ugly to me, but I'm not sure how to do this more
>> cleanly.  I'm not even sure if it's necessary, but I just ran back into
>> the driver reviewing some other patches so I figured I'd say something.
>
> This is not necessary as long as we add the riscv,timer node with the
> riscv,timer-cannot-wake-cpu property before we add the CLINT node. So it
> should not be a problem for any C9xx platform going forward.

Awesome, that sounds way better.

>
> Regards,
> Samuel
>
>> ---
>>  drivers/clocksource/timer-riscv.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
>> index a0d66fabf073..d2d0236d1ae6 100644
>> --- a/drivers/clocksource/timer-riscv.c
>> +++ b/drivers/clocksource/timer-riscv.c
>> @@ -139,6 +139,22 @@ static int __init riscv_timer_init_dt(struct device_node *n)
>>  	if (cpuid != smp_processor_id())
>>  		return 0;
>>
>> +	/*
>> +	 * The T-Head firmware does not route timer interrups to the core
>> +	 * during non-retentive suspend.  This is allowed by the specifications
>> +	 * (no interrupts are required to wake up the core during non-retentive
>> +	 * suspend), but most systems don't behave that way and Linux just
>> +	 * assumes that interrupts work.
>> +	 *
>> +	 * There's another timer for the T-Head sytems that behave this way
>> +	 * that is already probed by default, but just to be sure skip
>> +	 * initializing the SBI driver as it'll just break things later.
>> +	 */
>> +	if (sbi_get_mvendorid() == THEAD_VENDOR_ID) {
>> +		pr_debug_once("Skipping SBI timer on T-Head due to missed wakeups");
>> +		return 0;
>> +	}
>> +
>>  	domain = NULL;
>>  	child = of_get_compatible_child(n, "riscv,cpu-intc");
>>  	if (!child) {