| Message ID | 20210215121713.57687-7-marcan@marcan.st (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Apple M1 SoC platform bring-up | expand |
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.
On 15/02/2021 22.28, Marc Zyngier wrote: > 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. Good point, changed it for v3. >> >> - arch_timers_present |= ARCH_TIMER_TYPE_CP15; > > You probably didn't want to drop this line, did you? Ouch. Thanks for catching that.
* Hector Martin <marcan@marcan.st> [210215 12:18]: > 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. I like the idea of using interrupt-names property for mapping timers :) Similar approach might help other SoCs too. And clocksources never really had similar issues. With Marc's comments addressed, please feel free to add: Reviewed-by: Tony Lindgren <tony@atomide.com>
On 16/02/2021 03.23, Tony Lindgren wrote: > * Hector Martin <marcan@marcan.st> [210215 12:18]: >> 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. > > I like the idea of using interrupt-names property for mapping timers :) > > Similar approach might help other SoCs too. And clocksources never really > had similar issues. Yeah, there are some SoCs using dummy IRQs right now for this reason, so this should help with those too (though it's too late to introduce it now into those DTs without breaking backwards-compat with older kernels, sadly). > With Marc's comments addressed, please feel free to add: > > Reviewed-by: Tony Lindgren <tony@atomide.com> Thank you!
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", +}; + 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; - 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 };
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(-)