Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754080AbbHEOA4 (ORCPT ); Wed, 5 Aug 2015 10:00:56 -0400 Received: from mail-pa0-f54.google.com ([209.85.220.54]:33414 "EHLO mail-pa0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753098AbbHEOAv (ORCPT ); Wed, 5 Aug 2015 10:00:51 -0400 Message-ID: <55C21709.3070907@linaro.org> Date: Wed, 05 Aug 2015 22:00:41 +0800 From: Hanjun Guo User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Marc Zyngier , Jason Cooper , Will Deacon , Catalin Marinas , "Rafael J. Wysocki" CC: Thomas Gleixner , Jiang Liu , Bjorn Helgaas , Lorenzo Pieralisi , "suravee.suthikulpanit@amd.com" , Timur Tabi , Tomasz Nowicki , "grant.likely@linaro.org" , Mark Brown , Wei Huang , "linux-arm-kernel@lists.infradead.org" , "linux-acpi@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linaro-acpi@lists.linaro.org" Subject: Re: [PATCH v4 06/10] irqchip / GICv3: Add ACPI support for GICv3+ initialization References: <1438164539-29256-1-git-send-email-hanjun.guo@linaro.org> <1438164539-29256-7-git-send-email-hanjun.guo@linaro.org> <55C0BB5E.2080706@arm.com> In-Reply-To: <55C0BB5E.2080706@arm.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10030 Lines: 321 On 08/04/2015 09:17 PM, Marc Zyngier wrote: > On 29/07/15 11:08, Hanjun Guo wrote: >> From: Tomasz Nowicki >> >> 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 >> [hj: Rework this patch and fix multi issues] >> Signed-off-by: Hanjun Guo >> --- >> 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 . >> */ >> >> +#include >> #include >> #include >> #include >> @@ -25,6 +26,7 @@ >> #include >> #include >> >> +#include >> #include >> >> #include >> @@ -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? > >> >> 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? > >> + 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. Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/