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

Marc Zyngier
2021-02-15T13:29:17Z

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.