| Message ID | 1438164539-29256-7-git-send-email-hanjun.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 08/04/2015 09:17 PM, Marc Zyngier wrote: > On 29/07/15 11:08, Hanjun Guo wrote: >> From: Tomasz Nowicki <tomasz.nowicki@linaro.org> >> >> 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@linaro.org> >> [hj: Rework this patch and fix multi issues] >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >> --- >> 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? > >> >> 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-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Marc, On 08/07/2015 12:42 AM, Marc Zyngier wrote: > 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@linaro.org> >>>> >>>> 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@linaro.org> >>>> [hj: Rework this patch and fix multi issues] >>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >>>> --- >>>> 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. OK. [...] >>>> +} >>>> +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. OK, how about the following code when parsing the GIC version? /* * GICv3 driver can find out it's V3 or V4 itself, just set the * gic_version to V3 to match the driver if firmware presented V4. */ if (gic_version == ACPI_MADT_GIC_VERSION_V4) gic_version = ACPI_MADT_GIC_VERSION_V3; Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); gic_smp_init(); gic_dist_init(); @@ -818,6 +821,16 @@ out_free: return err; } +static int __init detect_distributor(void __iomem *dist_base) +{ + 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; +} + #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) +{ + 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); + 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) +{ + 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); + 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); +#endif