[PATCH 2/2] armv8: generic_timer: Use event stream for udelay

[Peter Hoyes]

On 4/29/24 16:16, Andre Przywara wrote:
> On Tue, 23 Apr 2024 09:10:05 +0100
> Peter Hoyes <peter.hoyes at arm.com> wrote:
>
> Hi,
>
>> From: Peter Hoyes <Peter.Hoyes at arm.com>
>>
>> Polling cntpct_el0 in a tight loop for delays is inefficient.
>> This is particularly apparent on Arm FVPs, which do not simulate
>> real time, meaning that a 1s sleep can take a couple of orders
>> of magnitude longer to execute in wall time.
>>
>> If running at EL2 or above (where CNTHCTL_EL2 is available), enable
>> the cntpct_el0 event stream temporarily and use wfe to implement
>> the delay more efficiently. The event period is chosen as a
>> trade-off between efficiency and the fact that Arm FVPs do not
>> typically simulate real time.
>>
>> This is only implemented for Armv8 boards, where an architectural
>> timer exists.
>>
>> Some mach-socfpga AArch64 boards already override __udelay to make
>> it always inline, so guard the functionality with a new
>> ARMV8_UDELAY_EVENT_STREAM Kconfig, enabled by default.
> So the code looks alright, and it works for me, tested on some hardware I
> just had at hand.
> However I am a bit worried about the scope of this patch. While it's
> indeed purely architectural and should work on all systems, I think we
> need to be careful with messing with the arch timer *while the OS is
> running*. U-Boot code will run for UEFI runtime services and for serving
> PSCI on some platforms, and I guess messing with CNTHCTL_EL2 is not a good
> idea then. And udelay sounds like a basic function that this code might
> use.
> So I wonder if we should limit this to the Arm FVPs for now? To not create
> subtle and hard-to-diagnose problems for a lot of boards?
>
> Or maybe there is some mechanism to limit this to U-Boot boot time/UEFI
> boot services?
>
>> Signed-off-by: Peter Hoyes <Peter.Hoyes at arm.com>
>> ---
>>   arch/arm/cpu/armv8/Kconfig         |  8 ++++++++
>>   arch/arm/cpu/armv8/generic_timer.c | 27 +++++++++++++++++++++++++++
>>   arch/arm/include/asm/system.h      |  6 ++++--
>>   3 files changed, 39 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/cpu/armv8/Kconfig b/arch/arm/cpu/armv8/Kconfig
>> index 9f0fb369f7..544c5e2d74 100644
>> --- a/arch/arm/cpu/armv8/Kconfig
>> +++ b/arch/arm/cpu/armv8/Kconfig
>> @@ -191,6 +191,14 @@ config ARMV8_EA_EL3_FIRST
>>   	  Exception handling at all exception levels for External Abort and
>>   	  SError interrupt exception are taken in EL3.
>>   
>> +config ARMV8_UDELAY_EVENT_STREAM
>> +	bool "Use the event stream for udelay"
>> +	default y if !ARCH_SOCFPGA
> As described above, change this to something like "... if ARCH_VEXPRESS64".
>
> Cheers,
> Andre
>
Agreed, this is limited to ARCH_VEXPRESS64 in v2.

Peter

>> +	help
>> +	  Use the event stream provided by the AArch64 architectural timer for
>> +	  delays. This is more efficient than the default polling
>> +	  implementation.
>> +
>>   menuconfig ARMV8_CRYPTO
>>   	bool "ARM64 Accelerated Cryptographic Algorithms"
>>   
>> diff --git a/arch/arm/cpu/armv8/generic_timer.c b/arch/arm/cpu/armv8/generic_timer.c
>> index 8f83372cbc..e18b5c8187 100644
>> --- a/arch/arm/cpu/armv8/generic_timer.c
>> +++ b/arch/arm/cpu/armv8/generic_timer.c
>> @@ -115,3 +115,30 @@ ulong timer_get_boot_us(void)
>>   
>>   	return val / get_tbclk();
>>   }
>> +
>> +#if CONFIG_IS_ENABLED(ARMV8_UDELAY_EVENT_STREAM)
>> +void __udelay(unsigned long usec)
>> +{
>> +	u64 target = get_ticks() + usec_to_tick(usec);
>> +
>> +	/* At EL2 or above, use the event stream to avoid polling CNTPCT_EL0 so often */
>> +	if (current_el() >= 2) {
>> +		u32 cnthctl_val;
>> +		const u8 event_period = 0x7;
>> +
>> +		asm volatile("mrs %0, cnthctl_el2" : "=r" (cnthctl_val));
>> +		asm volatile("msr cnthctl_el2, %0" : : "r"
>> +			(cnthctl_val | CNTHCTL_EL2_EVNT_EN | CNTHCTL_EL2_EVNT_I(event_period)));
>> +
>> +		while (get_ticks() + (1ULL << event_period) <= target)
>> +			wfe();
>> +
>> +		/* Reset the event stream */
>> +		asm volatile("msr cnthctl_el2, %0" : : "r" (cnthctl_val));
>> +	}
>> +
>> +	/* Fall back to polling CNTPCT_EL0 */
>> +	while (get_ticks() <= target)
>> +		;
>> +}
>> +#endif
>> diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h
>> index 51123c2968..7e30cac32a 100644
>> --- a/arch/arm/include/asm/system.h
>> +++ b/arch/arm/include/asm/system.h
>> @@ -69,8 +69,10 @@
>>   /*
>>    * CNTHCTL_EL2 bits definitions
>>    */
>> -#define CNTHCTL_EL2_EL1PCEN_EN	(1 << 1)  /* Physical timer regs accessible   */
>> -#define CNTHCTL_EL2_EL1PCTEN_EN	(1 << 0)  /* Physical counter accessible      */
>> +#define CNTHCTL_EL2_EVNT_EN	BIT(2)	     /* Enable the event stream       */
>> +#define CNTHCTL_EL2_EVNT_I(val)	((val) << 4) /* Event stream trigger bits     */
>> +#define CNTHCTL_EL2_EL1PCEN_EN	(1 << 1)     /* Physical timer regs accessible */
>> +#define CNTHCTL_EL2_EL1PCTEN_EN	(1 << 0)     /* Physical counter accessible   */
>>   
>>   /*
>>    * HCR_EL2 bits definitions