|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v7 14/28] xen/arm: ITS: Initialize physical ITS and export lpi support
Hi Vijay,
On 18/09/15 14:09, vijay.kilari@xxxxxxxxx wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
>
> Initialize physical ITS if HW supports LPIs.
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
> ---
> v7: - Export lpi support information to vgic-v3 driver from gic-v3.
> - Drop gic_lpi_supported() helper function
> - Add boot param to enable or disable physical ITS
> v6: - Updated lpi_supported gic_info member for GICv2 and GICv3
> - Introduced helper gic_lpi_supported() and exported
> v5: - Made check of its dt node availability before
> setting lpi_supported flag
> ---
> xen/arch/arm/gic-v3.c | 38
> ++++++++++++++++++++++++++++++++++---
> xen/arch/arm/vgic-v3.c | 5 ++++-
> xen/include/asm-arm/gic_v3_defs.h | 4 +++-
> xen/include/asm-arm/vgic.h | 2 +-
> 4 files changed, 43 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index c4c77a7..ac8a0ea 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -55,6 +55,18 @@ static struct {
> } gicv3;
>
> static struct gic_info gicv3_info;
> +/* Enable/Disable ITS support */
> +static bool_t its_enable = 1;
> +/* Availability of ITS support after successful ITS initialization */
> +static bool_t its_enabled = 0;
> +
> +static void __init parse_its_param(char *s)
> +{
> + if ( !parse_bool(s) )
> + its_enable = 0;
> +}
> +
> +custom_param("its", parse_its_param);
Why do you need a command line option to enable/disable the physical ITS?
>
> /* per-cpu re-distributor base */
> DEFINE_PER_CPU(struct rdist, rdist);
> @@ -590,7 +602,7 @@ static void __init gicv3_dist_init(void)
> * Here we override HW supported number of LPIs and
> * limit to to LPIs specified in nr_lpis.
> */
> - if ( gicv3_dist_supports_lpis() )
> + if ( its_enabled && gicv3_dist_supports_lpis() )
> gicv3_info.nr_irq_ids = nr_lpis + FIRST_GIC_LPI;
> else
> {
> @@ -714,6 +726,10 @@ static int __cpuinit gicv3_cpu_init(void)
> if ( gicv3_enable_redist() )
> return -ENODEV;
>
> + /* Give LPIs a spin */
> + if ( its_enabled && gicv3_dist_supports_lpis() )
If the ITS is not enabled, the list of ITS nodes will be empty and
therefore its_cpu_init will return directly.
So its_enabled is not necessary.
> + its_cpu_init();
> +
> /* Set priority on PPI and SGI interrupts */
> priority = (GIC_PRI_IPI << 24 | GIC_PRI_IPI << 16 | GIC_PRI_IPI << 8 |
> GIC_PRI_IPI);
> @@ -1303,14 +1319,30 @@ static int __init gicv3_init(void)
> i, r->base, r->base + r->size);
> }
>
> - vgic_v3_setup_hw(dbase, gicv3.rdist_count, gicv3.rdist_regions,
> - gicv3.rdist_stride);
> + reg = readl_relaxed(GICD + GICD_TYPER);
> +
> + gicv3.rdist_data.id_bits = ((reg >> GICD_TYPE_ID_BITS_SHIFT) &
> + GICD_TYPE_ID_BITS_MASK) + 1;
> +
> gicv3_init_v2(node, dbase);
>
> spin_lock_init(&gicv3.lock);
>
> spin_lock(&gicv3.lock);
>
> + if ( its_enable && gicv3_dist_supports_lpis() )
> + {
> + /*
> + * LPI support is enabled only if HW supports it and
> + * ITS dt node is available
> + */
> + if ( !its_init(&gicv3.rdist_data) )
> + its_enabled = 1;
> + }
> +
> + vgic_v3_setup_hw(dbase, gicv3.rdist_count, gicv3.rdist_regions,
> + gicv3.rdist_stride, its_enabled);
> +
Why do you need to execute all this new code with the gicv3.lock taken?
AFAICT there is no use of GICv3 registers inside the ITS initialization.
> gicv3_dist_init();
> res = gicv3_cpu_init();
> gicv3_hyp_init();
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 12c5d87..52d4277 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -51,6 +51,8 @@
>
> static struct {
> bool_t enabled;
> + /* Check if its supported */
> + bool_t lpi_support;
While ITS means LPIs is supported, the invert is not true.
Can you please rename this variable to its_enabled.
> /* Distributor interface address */
> paddr_t dbase;
> /* Re-distributor regions */
> @@ -62,9 +64,10 @@ static struct {
> void vgic_v3_setup_hw(paddr_t dbase,
> unsigned int nr_rdist_regions,
> const struct rdist_region *regions,
> - uint32_t rdist_stride)
> + uint32_t rdist_stride, bool_t lpi_support)
Ditto.
> {
> vgic_v3_hw.enabled = 1;
> + vgic_v3_hw.lpi_support = lpi_support;
Ditto.
> vgic_v3_hw.dbase = dbase;
> vgic_v3_hw.nr_rdist_regions = nr_rdist_regions;
> vgic_v3_hw.regions = regions;
> diff --git a/xen/include/asm-arm/gic_v3_defs.h
> b/xen/include/asm-arm/gic_v3_defs.h
> index 1bc88f6..f819589 100644
> --- a/xen/include/asm-arm/gic_v3_defs.h
> +++ b/xen/include/asm-arm/gic_v3_defs.h
> @@ -46,7 +46,9 @@
> #define GICC_SRE_EL2_ENEL1 (1UL << 3)
>
> /* Additional bits in GICD_TYPER defined by GICv3 */
> -#define GICD_TYPE_ID_BITS_SHIFT 19
> +#define GICD_TYPE_ID_BITS_SHIFT (19)
> +#define GICD_TYPE_ID_BITS_MASK (0x1f)
> +#define GICD_TYPE_LPIS (0x1UL << 17)
Please correct the typo in the name rather than keeping it everywhere. I.e
s/TYPE/TYPER/
Also, you've introduced GICD_TYPER_LPIS_SUPPORTED in patch #5 which does
exactly the same things as GICD_TYPE_LPIS. Please use it rather
introducing a new one.
>
> #define GICD_TYPER_LPIS_SUPPORTED (1U << 17)
> #define GICD_CTLR_RWP (1UL << 31)
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index cf5a790..2bf061f 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -362,7 +362,7 @@ struct rdist_region;
> void vgic_v3_setup_hw(paddr_t dbase,
> unsigned int nr_rdist_regions,
> const struct rdist_region *regions,
> - uint32_t rdist_stride);
> + uint32_t rdist_stride, bool_t lpi_support);
Ditto.
> #endif
>
> #endif /* __ASM_ARM_VGIC_H__ */
>
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |