Re: [PATCH v2 06/25] arm64: arch_timer: implement support for interrupt-names

From: Marc Zyngier <maz@kernel.org>
To: Hector Martin <marcan@marcan.st>
Cc: linux-arm-kernel@lists.infradead.org,
	Rob Herring <robh@kernel.org>, Arnd Bergmann <arnd@kernel.org>,
	Olof Johansson <olof@lixom.net>,
	Krzysztof Kozlowski <krzk@kernel.org>,
	Mark Kettenis <mark.kettenis@xs4all.nl>,
	Tony Lindgren <tony@atomide.com>,
	Mohamed Mediouni <mohamed.mediouni@caramail.com>,
	Stan Skowronek <stan@corellium.com>,
	Alexander Graf <graf@amazon.com>, Will Deacon <will@kernel.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	Mark Rutland <mark.rutland@arm.com>,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 06/25] arm64: arch_timer: implement support for interrupt-names
Date: Mon, 15 Feb 2021 13:28:05 +0000	[thread overview]
Message-ID: <87tuqdpije.wl-maz@kernel.org> (raw)
In-Reply-To: <20210215121713.57687-7-marcan@marcan.st>

On Mon, 15 Feb 2021 12:16:54 +0000,
Hector Martin <marcan@marcan.st> wrote:
> 
> This allows the devicetree to correctly represent the available set of
> timers, which varies from device to device, without the need for fake
> dummy interrupts for unavailable slots.
> 
> Also add the hyp-virt timer/PPI, which is not currently used, but worth
> representing.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  drivers/clocksource/arm_arch_timer.c | 25 +++++++++++++++++++++----
>  include/clocksource/arm_arch_timer.h |  1 +
>  2 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index d0177824c518..0e87b6d1ce97 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -63,6 +63,14 @@ struct arch_timer {
>  static u32 arch_timer_rate;
>  static int arch_timer_ppi[ARCH_TIMER_MAX_TIMER_PPI];
>  
> +static const char *arch_timer_ppi_names[ARCH_TIMER_MAX_TIMER_PPI] = {
> +	"phys-secure",
> +	"phys",
> +	"virt",
> +	"hyp-phys",
> +	"hyp-virt",

nit: I'd prefer it if the array was described as:

	[ARCH_TIMER_PHYS_SECURE_PPI] = "phys-secure",
	[...]

just to avoid that the two get out of sync.

> +};
> +
>  static struct clock_event_device __percpu *arch_timer_evt;
>  
>  static enum arch_timer_ppi_nr arch_timer_uses_ppi = ARCH_TIMER_VIRT_PPI;
> @@ -1280,17 +1288,26 @@ static void __init arch_timer_populate_kvm_info(void)
>  
>  static int __init arch_timer_of_init(struct device_node *np)
>  {
> -	int i, ret;
> +	int i, irq, ret;
>  	u32 rate;
> +	bool has_names;
>  
>  	if (arch_timers_present & ARCH_TIMER_TYPE_CP15) {
>  		pr_warn("multiple nodes in dt, skipping\n");
>  		return 0;
>  	}
>  
> -	arch_timers_present |= ARCH_TIMER_TYPE_CP15;

You probably didn't want to drop this line, did you?

> -	for (i = ARCH_TIMER_PHYS_SECURE_PPI; i < ARCH_TIMER_MAX_TIMER_PPI; i++)
> -		arch_timer_ppi[i] = irq_of_parse_and_map(np, i);
> +
> +	has_names = of_property_read_bool(np, "interrupt-names");
> +
> +	for (i = ARCH_TIMER_PHYS_SECURE_PPI; i < ARCH_TIMER_MAX_TIMER_PPI; i++) {
> +		if (has_names)
> +			irq = of_irq_get_byname(np, arch_timer_ppi_names[i]);
> +		else
> +			irq = of_irq_get(np, i);
> +		if (irq > 0)
> +			arch_timer_ppi[i] = irq;
> +	}
>  
>  	arch_timer_populate_kvm_info();
>  
> diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
> index 1d68d5613dae..73c7139c866f 100644
> --- a/include/clocksource/arm_arch_timer.h
> +++ b/include/clocksource/arm_arch_timer.h
> @@ -32,6 +32,7 @@ enum arch_timer_ppi_nr {
>  	ARCH_TIMER_PHYS_NONSECURE_PPI,
>  	ARCH_TIMER_VIRT_PPI,
>  	ARCH_TIMER_HYP_PPI,
> +	ARCH_TIMER_HYP_VIRT_PPI,
>  	ARCH_TIMER_MAX_TIMER_PPI
>  };

Otherwise looks OK to me.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.