Hi Andrei,
> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS)
> <andrei.cherech...@oss.nxp.com> wrote:
>
> From: Andrei Cherechesu <andrei.cherech...@nxp.com>
>
> Added support for parsing the ARM generic timer interrupts DT
> node by the "interrupt-names" property, if it is available.
>
> If not available, the usual parsing based on the expected
> IRQ order is performed.
>
> Also added the "hyp-virt" PPI to the timer PPI list, even
> though it's currently not in use. If the "hyp-virt" PPI is
> not found, the hypervisor won't panic.
>
> Signed-off-by: Andrei Cherechesu <andrei.cherech...@nxp.com>
> ---
> xen/arch/arm/include/asm/time.h | 3 ++-
> xen/arch/arm/time.c | 26 ++++++++++++++++++++++----
> 2 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/time.h b/xen/arch/arm/include/asm/time.h
> index 4b401c1110..49ad8c1a6d 100644
> --- a/xen/arch/arm/include/asm/time.h
> +++ b/xen/arch/arm/include/asm/time.h
> @@ -82,7 +82,8 @@ enum timer_ppi
> TIMER_PHYS_NONSECURE_PPI = 1,
> TIMER_VIRT_PPI = 2,
> TIMER_HYP_PPI = 3,
> - MAX_TIMER_PPI = 4,
> + TIMER_HYP_VIRT_PPI = 4,
> + MAX_TIMER_PPI = 5,
> };
>
> /*
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index 433d7be909..794da646d6 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -38,6 +38,14 @@ uint32_t __read_mostly timer_dt_clock_frequency;
>
> static unsigned int timer_irq[MAX_TIMER_PPI];
>
> +static const char *timer_irq_names[MAX_TIMER_PPI] = {
> + [TIMER_PHYS_SECURE_PPI] = "sec-phys",
> + [TIMER_PHYS_NONSECURE_PPI] = "phys",
> + [TIMER_VIRT_PPI] = "virt",
> + [TIMER_HYP_PPI] = "hyp-phys",
> + [TIMER_HYP_VIRT_PPI] = "hyp-virt",
> +};
> +
I would need some reference or a pointer to some doc to check those.
> unsigned int timer_get_irq(enum timer_ppi ppi)
> {
> ASSERT(ppi >= TIMER_PHYS_SECURE_PPI && ppi < MAX_TIMER_PPI);
> @@ -149,15 +157,25 @@ static void __init init_dt_xen_time(void)
> {
> int res;
> unsigned int i;
> + bool has_names;
> +
> + has_names = dt_property_read_bool(timer, "interrupt-names");
>
> /* Retrieve all IRQs for the timer */
> for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
> {
> - res = platform_get_irq(timer, i);
> -
> - if ( res < 0 )
> + if ( has_names )
> + res = platform_get_irq_byname(timer, timer_irq_names[i]);
> + else
> + res = platform_get_irq(timer, i);
> +
> + if ( res > 0 )
The behaviour of the code is changed here compared to the current
version as res = 0 will now generate a panic.
Some device tree might not specify an interrupt number and just put
0 and Xen will now panic on those systems.
As I have no idea if such systems exists and the behaviour is modified
you should justify this and mention it in the commit message or keep
the old behaviour and let 0 go through without a panic.
@stefano, julien any idea here ? should just keep the old behaviour ?
> + timer_irq[i] = res;
> + /* Do not panic if "hyp-virt" PPI is not found, since it's not
> + * currently used.
> + */
Please respect the standard for comments and keep the first line empty:
/*
* comment
*/
> + else if ( i != TIMER_HYP_VIRT_PPI )
> panic("Timer: Unable to retrieve IRQ %u from the device tree\n",
> i);
> - timer_irq[i] = res;
> }
> }
Cheers
Bertrand
>
> --
> 2.35.1
>
>
