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-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
