Re: [PATCH v4 06/10] irqchip / GICv3: Add ACPI support for GICv3+ initialization
From: Marc Zyngier
Date: Thu Aug 06 2015 - 12:42:13 EST
On 05/08/15 15:00, Hanjun Guo wrote:
> On 08/04/2015 09:17 PM, Marc Zyngier wrote:
>> On 29/07/15 11:08, Hanjun Guo wrote:
>>> From: Tomasz Nowicki <tomasz.nowicki@xxxxxxxxxx>
>>>
>>> With the refator of gic_of_init(), GICv3/4 can be initialized
>>> by gic_init_bases() with gic distributor base address and gic
>>> redistributor region(s).
>>>
>>> So get the redistributor region base addresses from MADT GIC
>>> redistributor subtable, and the distributor base address from
>>> GICD subtable to init GICv3 irqchip in ACPI way.
>>>
>>> Note: GIC redistributor base address may also be provided in
>>> GICC structures on systems supporting GICv3 and above if the GIC
>>> Redistributors are not in the always-on power domain, this
>>> patch didn't implement such feature yet.
>>>
>>> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@xxxxxxxxxx>
>>> [hj: Rework this patch and fix multi issues]
>>> Signed-off-by: Hanjun Guo <hanjun.guo@xxxxxxxxxx>
>>> ---
>>> drivers/irqchip/irq-gic-v3.c | 174 +++++++++++++++++++++++++++++++++++++++++--
>>> 1 file changed, 169 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>>> index c0b96c6..ebc5604 100644
>>> --- a/drivers/irqchip/irq-gic-v3.c
>>> +++ b/drivers/irqchip/irq-gic-v3.c
>>> @@ -15,6 +15,7 @@
>>> * along with this program. If not, see <http://www.gnu.org/licenses/>.
>>> */
>>>
>>> +#include <linux/acpi.h>
>>> #include <linux/cpu.h>
>>> #include <linux/cpu_pm.h>
>>> #include <linux/delay.h>
>>> @@ -25,6 +26,7 @@
>>> #include <linux/percpu.h>
>>> #include <linux/slab.h>
>>>
>>> +#include <linux/irqchip/arm-gic-acpi.h>
>>> #include <linux/irqchip/arm-gic-v3.h>
>>>
>>> #include <asm/cputype.h>
>>> @@ -802,7 +804,8 @@ static int __init gic_init_bases(void __iomem *dist_base,
>>> set_handle_irq(gic_handle_irq);
>>>
>>> if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis())
>>> - its_init(domain_token, &gic_data.rdists, gic_data.domain);
>>> + its_init(irq_domain_token_to_of_node(domain_token),
>>> + &gic_data.rdists, gic_data.domain);
>>
>> This doesn't make much sense. The first parameter to its_init is indeed
>> supposed to be an of_node, but what is the point of calling its_init if
>> you *know* you don't have the necessary topological information to parse
>> the firmware tables?
>>
>> I don't see *any* code that is going to parse the ITS table in this
>> series, so please don't call its_init passing a NULL pointer to it. This
>> is just gross.
>
> OK, the ITS ACPI code is in later patch which combined with IORT. How
> about moving it to the GIC of init code temporary?
Just don't call its_init if irq_domain_token_to_of_node(domain_token) is
NULL. But don't call it with a NULL parameter.
>>
>>>
>>> gic_smp_init();
>>> gic_dist_init();
>>> @@ -818,6 +821,16 @@ out_free:
>>> return err;
>>> }
>>>
>>> +static int __init detect_distributor(void __iomem *dist_base)
>>
>> We do have a naming convention in this file. All functions start with gic_.
>>
>>> +{
>>> + u32 reg = readl_relaxed(dist_base + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
>>> +
>>> + if (reg != GIC_PIDR2_ARCH_GICv3 && reg != GIC_PIDR2_ARCH_GICv4)
>>> + return -ENODEV;
>>> +
>>> + return 0;
>>> +}
>>
>> This function doesn't detect anything, it validates that we have
>> something sensible. Please rename it to gic_validate_dist_version, or
>> something similar.
>
> Ok.
>
>>
>>> +
>>> #ifdef CONFIG_OF
>>> static int __init gic_of_init(struct device_node *node, struct device_node *parent)
>>> {
>>> @@ -825,7 +838,6 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
>>> struct redist_region *rdist_regs;
>>> u64 redist_stride;
>>> u32 nr_redist_regions;
>>> - u32 reg;
>>> int err, i;
>>>
>>> dist_base = of_iomap(node, 0);
>>> @@ -835,11 +847,10 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
>>> return -ENXIO;
>>> }
>>>
>>> - reg = readl_relaxed(dist_base + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
>>> - if (reg != GIC_PIDR2_ARCH_GICv3 && reg != GIC_PIDR2_ARCH_GICv4) {
>>> + err = detect_distributor(dist_base);
>>> + if (err) {
>>> pr_err("%s: no distributor detected, giving up\n",
>>> node->full_name);
>>> - err = -ENODEV;
>>> goto out_unmap_dist;
>>> }
>>>
>>> @@ -887,3 +898,156 @@ out_unmap_dist:
>>>
>>> IRQCHIP_DECLARE(gic_v3, "arm,gic-v3", gic_of_init);
>>> #endif
>>> +
>>> +#ifdef CONFIG_ACPI
>>> +static struct redist_region *redist_regs __initdata;
>>> +static u32 nr_redist_regions __initdata;
>>> +static phys_addr_t dist_phys_base __initdata;
>>> +
>>> +static int __init
>>> +gic_acpi_register_redist(u64 phys_base, u64 size)
>>
>> A physical address must use phys_addr_t.
>
> OK.
>
>>
>>> +{
>>> + struct redist_region *redist_regs_new;
>>> + void __iomem *redist_base;
>>> +
>>> + redist_regs_new = krealloc(redist_regs,
>>> + sizeof(*redist_regs) * (nr_redist_regions + 1),
>>> + GFP_KERNEL);
>>
>> NAK. If you have multiple regions, you count them, you allocate the
>> right number of regions. This will be far more efficient than doing this
>> realloc dance. It is not like we're not parsing the tables a zillion
>> times already. Doing it one more time won't hurt.
>
> Agreed, will update in next version.
>
>>
>>> + if (!redist_regs_new) {
>>> + pr_err("Couldn't allocate resource for GICR region\n");
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + redist_regs = redist_regs_new;
>>> +
>>> + redist_base = ioremap(phys_base, size);
>>> + if (!redist_base) {
>>> + pr_err("Couldn't map GICR region @%llx\n", phys_base);
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + redist_regs[nr_redist_regions].phys_base = phys_base;
>>> + redist_regs[nr_redist_regions].redist_base = redist_base;
>>> + nr_redist_regions++;
>>> + return 0;
>>> +}
>>> +
>>> +static int __init
>>> +gic_acpi_parse_madt_redist(struct acpi_subtable_header *header,
>>> + const unsigned long end)
>>> +{
>>> + struct acpi_madt_generic_redistributor *redist;
>>> +
>>> + if (BAD_MADT_ENTRY(header, end))
>>> + return -EINVAL;
>>> +
>>> + redist = (struct acpi_madt_generic_redistributor *)header;
>>> + if (!redist->base_address)
>>> + return -EINVAL;
>>> +
>>> + return gic_acpi_register_redist(redist->base_address, redist->length);
>>> +}
>>> +
>>> +static int __init
>>> +gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header,
>>> + const unsigned long end)
>>> +{
>>
>> How many versions of gic_acpi_parse_madt_distributor are we going to
>> get? Why isn't the ACPI parsing in a common file? Why do I have to read
>> the same code on and on until my eyes bleed?
>
> I will try to move it to common file irq-gic-acpi.c.
>
>>
>>> + struct acpi_madt_generic_distributor *dist;
>>> +
>>> + dist = (struct acpi_madt_generic_distributor *)header;
>>> +
>>> + if (BAD_MADT_ENTRY(dist, end))
>>> + return -EINVAL;
>>> +
>>> + dist_phys_base = dist->base_address;
>>> + return 0;
>>> +}
>>> +
>>> +static int gic_acpi_gsi_desc_populate(struct acpi_gsi_descriptor *data,
>>> + u32 gsi, unsigned int irq_type)
>>> +{
>>> + /*
>>> + * Encode GSI and triggering information the way the GIC likes
>>> + * them.
>>> + */
>>> + if (WARN_ON(gsi < 16))
>>> + return -EINVAL;
>>> +
>>> + if (gsi >= 32) {
>>> + data->param[0] = 0; /* SPI */
>>> + data->param[1] = gsi - 32;
>>> + data->param[2] = irq_type;
>>> + } else {
>>> + data->param[0] = 1; /* PPI */
>>> + data->param[1] = gsi - 16;
>>> + data->param[2] = 0xff << 4 | irq_type;
>>> + }
>>> +
>>> + data->param_count = 3;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int __init
>>> +gic_acpi_init(struct acpi_table_header *table)
>>> +{
>>> + int count, i, err = 0;
>>> + void __iomem *dist_base;
>>> +
>>> + /* Get distributor base address */
>>> + count = acpi_parse_entries(ACPI_SIG_MADT,
>>> + sizeof(struct acpi_table_madt),
>>> + gic_acpi_parse_madt_distributor, table,
>>> + ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, 0);
>>> + if (count <= 0) {
>>> + pr_err("No valid GICD entry exist\n");
>>> + return -EINVAL;
>>> + } else if (count > 1) {
>>> + pr_err("More than one GICD entry detected\n");
>>> + return -EINVAL;
>>> + }
>>> +
>>> + dist_base = ioremap(dist_phys_base, ACPI_GICV3_DIST_MEM_SIZE);
>>> + if (!dist_base) {
>>> + pr_err("Unable to map GICD registers\n");
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + err = detect_distributor(dist_base);
>>> + if (err) {
>>> + pr_err("No distributor detected at @%p, giving up", dist_base);
>>> + goto out_dist_unmap;
>>> + }
>>> +
>>> + /* Collect redistributor base addresses */
>>> + count = acpi_parse_entries(ACPI_SIG_MADT,
>>> + sizeof(struct acpi_table_madt),
>>> + gic_acpi_parse_madt_redist, table,
>>> + ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, 0);
>>> + if (count <= 0) {
>>> + pr_info("No valid GICR entries exist\n");
>>> + err = -EINVAL;
>>> + goto out_redist_unmap;
>>> + }
>>> +
>>> + err = gic_init_bases(dist_base, redist_regs, nr_redist_regions, 0,
>>> + (void *)ACPI_IRQ_MODEL_GIC);
>>
>> There is no other global identifier for GICv3? It feels a bit weird to
>> use the same ID as GICv2 (though probably not harmful as you can't have
>> both as the same time). What are you planning to use for the ITS then?
>> You must make sure you have a global namespace.
>
> I will use the ITS physical base address as the token for ITS, maybe I
> can use the GICD physical base address here instead?
That should be fine as long as the physical address cannot be
interpreted as a kernel address. Which is still a bit dodgy. I'd be more
happy if you had a proper namespace.
>>
>>> + if (err)
>>> + goto out_redist_unmap;
>>> +
>>> + acpi_set_irq_model(ACPI_IRQ_MODEL_GIC, ACPI_IRQ_MODEL_GIC,
>>> + gic_acpi_gsi_desc_populate);
>>> + return 0;
>>> +
>>> +out_redist_unmap:
>>> + for (i = 0; i < nr_redist_regions; i++)
>>> + if (redist_regs[i].redist_base)
>>> + iounmap(redist_regs[i].redist_base);
>>> + kfree(redist_regs);
>>> +out_dist_unmap:
>>> + iounmap(dist_base);
>>> + return err;
>>> +}
>>> +IRQCHIP_ACPI_DECLARE(gic_v3, ACPI_MADT_GIC_VERSION_V3, gic_acpi_init);
>>> +IRQCHIP_ACPI_DECLARE(gic_v4, ACPI_MADT_GIC_VERSION_V4, gic_acpi_init);
>>
>> As mentioned before, this doesn't work.
>
> hmm, I think we need more discussion for this one, but we need to match
> V4 for GICv3 drivers, and everything will be the same in the dirver
> as I said before.
And as I said before, you don't need to distinguish v3 from v4 in the
ACPI code. Matching GICv3 is enough.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/