From: Shanker Donthineni <shankerd@codeaurora.org>
To: Julien Grall <julien.grall@arm.com>,
xen-devel <xen-devel@lists.xensource.com>,
Stefano Stabellini <sstabellini@kernel.org>
Cc: Philip Elcan <pelcan@codeaurora.org>,
Vikram Sethi <vikrams@codeaurora.org>
Subject: Re: [PATCH 3/8] arm/gic-v3: Parse per-cpu redistributor entry in GICC subtable
Date: Tue, 21 Jun 2016 08:52:49 -0500 [thread overview]
Message-ID: <576946B1.3000303@codeaurora.org> (raw)
In-Reply-To: <576913F6.3020100@arm.com>
On 06/21/2016 05:16 AM, Julien Grall wrote:
> Hello Shanker,
>
> On 19/06/16 00:45, Shanker Donthineni wrote:
>> The redistributor address can be specified either as part of GICC or
>> GICR subtable depending on the power domain. The current driver
>> doesn't support parsing redistributor entry that is defined in GICC
>> subtable. The GIC CPU subtable entry holds the associated Redistributor
>> base address if it is not on always-on power domain.
>>
>> This patch adds necessary code to handle both types of Redistributors
>> base addresses.
>>
>> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
>> ---
>> xen/arch/arm/gic-v3.c | 97
> ++++++++++++++++++++++++++++++++-------
>> xen/include/asm-arm/gic.h | 2 +
>> xen/include/asm-arm/gic_v3_defs.h | 1 +
>> 3 files changed, 83 insertions(+), 17 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index af12ebc..42cf848 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -659,6 +659,10 @@ static int __init gicv3_populate_rdist(void)
>> smp_processor_id(), i, ptr);
>> return 0;
>> }
>> +
>> + if ( gicv3.rdist_regions[i].single_rdist )
>> + break;
>> +
>> if ( gicv3.rdist_stride )
>> ptr += gicv3.rdist_stride;
>> else
>> @@ -1282,6 +1286,11 @@ static int gicv3_iomem_deny_access(const struct
> domain *d)
>> }
>>
>> #ifdef CONFIG_ACPI
>> +static bool gic_dist_supports_dvis(void)
>
> static inline and please use bool_t here.
>
Still learning XEN coding style, I'll fix it.
>> +{
>> + return !!(readl_relaxed(GICD + GICD_TYPER) & GICD_TYPER_DVIS);
>> +}
>> +
>> static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
>> {
>> struct acpi_subtable_header *header;
>> @@ -1393,18 +1402,39 @@ gic_acpi_parse_madt_redistributor(struct
> acpi_subtable_header *header,
>> const unsigned long end)
>> {
>> struct acpi_madt_generic_redistributor *rdist;
>> + struct acpi_madt_generic_interrupt *processor;
>> struct rdist_region *region;
>>
>> region = gicv3.rdist_regions + gicv3.rdist_count;
>> - rdist = (struct acpi_madt_generic_redistributor *)header;
>> - if ( BAD_MADT_ENTRY(rdist, end) )
>> - return -EINVAL;
>> + if ( header->type == ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR )
>> + {
>> + rdist = (struct acpi_madt_generic_redistributor *)header;
>> + if ( BAD_MADT_ENTRY(rdist, end) )
>> + return -EINVAL;
>>
>> - if ( !rdist->base_address || !rdist->length )
>> - return -EINVAL;
>> + if ( !rdist->base_address || !rdist->length )
>> + return -EINVAL;
>> +
>> + region->base = rdist->base_address;
>> + region->size = rdist->length;
>> + }
>> + else if ( header->type == ACPI_MADT_TYPE_GENERIC_INTERRUPT )
>> + {
>
> Parsing the GICC and the redistributor is quite different. I would
> much prefer a function for parsing each table and an helper to add a
> new redistributor.
>
I'll do.
>> + processor = (struct acpi_madt_generic_interrupt *)header;
>> + if ( BAD_MADT_ENTRY(processor, end) )
>> + return -EINVAL;
>> +
>> + if ( !(processor->flags & ACPI_MADT_ENABLED) )
>> + return 0;
>> +
>> + if ( !processor->gicr_base_address )
>> + return -EINVAL;
>> +
>> + region->base = processor->gicr_base_address;
>> + region->size = gic_dist_supports_dvis() ? SZ_256K : SZ_128K;
>
> Please explain in the commit message how you find the size. I would
> also prefer if you use (4 x SZ_64K) and (2 * SZ_64K) as we do in
> populate_rdist.
>
>> + region->single_rdist = true;
>
> The indentation looks wrong.
>
>> + }
>
>
>>
>> - region->base = rdist->base_address;
>> - region->size = rdist->length;
>>
>> region->map_base = ioremap_nocache(region->base, region->size);
>> if ( !region->map_base )
>> @@ -1412,6 +1442,7 @@ gic_acpi_parse_madt_redistributor(struct
> acpi_subtable_header *header,
>> printk("Unable to map GICR registers\n");
>> return -ENOMEM;
>> }
>> +
>
> Spurious change.
>
>> gicv3.rdist_count++;
>>
>> return 0;
>> @@ -1421,9 +1452,22 @@ static int __init
>> gic_acpi_get_madt_redistributor_num(struct acpi_subtable_header
> *header,
>> const unsigned long end)
>> {
>> - /* Nothing to do here since it only wants to get the number of GIC
>> - * redistributors.
>> - */
>> + struct acpi_madt_generic_redistributor *rdist;
>> + struct acpi_madt_generic_interrupt *cpuif;
>> +
>> + if ( header->type == ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR )
>> + {
>> + rdist = (struct acpi_madt_generic_redistributor *)header;
>> + if ( BAD_MADT_ENTRY(rdist, end) || !rdist->base_address )
>> + return -EINVAL;
>> + }
>> + else if ( header->type == ACPI_MADT_TYPE_GENERIC_INTERRUPT )
>> + {
>> + cpuif = (struct acpi_madt_generic_interrupt *)header;
>> + if ( BAD_MADT_ENTRY(cpuif, end) || !cpuif->gicr_base_address )
>> + return -EINVAL;
>> + }
>> +
>
> Ditto for the parsing.
>
>> return 0;
>> }
>>
>
> [...]
>
>> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
>> index 44b9ef6..7f9ad86 100644
>> --- a/xen/include/asm-arm/gic.h
>> +++ b/xen/include/asm-arm/gic.h
>> @@ -101,6 +101,8 @@
>> #define GICD_TYPE_CPUS_SHIFT 5
>> #define GICD_TYPE_CPUS 0x0e0
>> #define GICD_TYPE_SEC 0x400
>> +#define GICD_TYPER_LPIS (1U << 17)
>
> This is not used by this patch. So please drop it.
>
I'll drop it.
>> +#define GICD_TYPER_DVIS (1U << 18)
>>
>> #define GICC_CTL_ENABLE 0x1
>> #define GICC_CTL_EOI (0x1 << 9)
>> diff --git a/xen/include/asm-arm/gic_v3_defs.h
> b/xen/include/asm-arm/gic_v3_defs.h
>> index 6d98491..6bd25a5 100644
>> --- a/xen/include/asm-arm/gic_v3_defs.h
>> +++ b/xen/include/asm-arm/gic_v3_defs.h
>> @@ -141,6 +141,7 @@ struct rdist_region {
>> paddr_t base;
>> paddr_t size;
>> void __iomem *map_base;
>> + bool single_rdist;
>> };
>>
>> #endif /* __ASM_ARM_GIC_V3_DEFS_H__ */
>>
>
> Regards,
>
--
Shanker Donthineni
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
next prev parent reply other threads:[~2016-06-21 13:52 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-18 23:45 [PATCH 0/8] Add support for parsing per CPU Redistributor entry Shanker Donthineni
2016-06-18 23:45 ` [PATCH 1/8] arm/gic-v3: Add a separate function for mapping GICD region Shanker Donthineni
2016-06-21 9:42 ` Julien Grall
2016-06-18 23:45 ` [PATCH 2/8] arm/gic-v3: Fold GICR subtable parsing into a new function Shanker Donthineni
2016-06-21 10:17 ` Julien Grall
2016-06-21 14:02 ` Shanker Donthineni
2016-06-18 23:45 ` [PATCH 3/8] arm/gic-v3: Parse per-cpu redistributor entry in GICC subtable Shanker Donthineni
2016-06-21 10:16 ` Julien Grall
2016-06-21 13:52 ` Shanker Donthineni [this message]
2016-06-22 13:06 ` Julien Grall
2016-06-18 23:45 ` [PATCH 4/8] xen/arm: vgic: Use dynamic memory allocation for vgic_rdist_region Shanker Donthineni
2016-06-21 10:26 ` Julien Grall
2016-06-18 23:45 ` [PATCH 5/8] arm/gic-v3: Remove an unused macro MAX_RDIST_COUNT Shanker Donthineni
2016-06-18 23:45 ` [PATCH 6/8] arm: vgic: Split vgic_domain_init() functionality into two functions Shanker Donthineni
2016-06-21 10:49 ` Julien Grall
2016-06-21 14:36 ` Shanker Donthineni
2016-06-21 14:48 ` Julien Grall
2016-06-21 15:09 ` Shanker Donthineni
2016-06-18 23:45 ` [PATCH 7/8] arm/mmio: Use separate memory allocation for mmio handlers Shanker Donthineni
2016-06-18 23:45 ` [PATCH 8/8] arm/vgic: Change fixed number of mmio handlers to variable number Shanker Donthineni
2016-06-21 9:28 ` [PATCH 0/8] Add support for parsing per CPU Redistributor entry Julien Grall
2016-06-21 13:37 ` Shanker Donthineni
2016-06-21 13:50 ` Julien Grall
2016-06-21 14:16 ` Shanker Donthineni
2016-06-21 14:44 ` Julien Grall
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=576946B1.3000303@codeaurora.org \
--to=shankerd@codeaurora.org \
--cc=julien.grall@arm.com \
--cc=pelcan@codeaurora.org \
--cc=sstabellini@kernel.org \
--cc=vikrams@codeaurora.org \
--cc=xen-devel@lists.xensource.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).