| Message ID | 1444865156-9870-7-git-send-email-Suravee.Suthikulpanit@amd.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Suravee, On 15.10.2015 01:25, Suravee Suthikulpanit wrote: > This patch introduces gicv2m_acpi_init(), which uses information > in MADT GIC MSI frames structure to initialize GICv2m driver. > > Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > --- > drivers/irqchip/irq-gic-v2m.c | 94 +++++++++++++++++++++++++++++++++++++++++ > drivers/irqchip/irq-gic.c | 3 ++ > include/linux/irqchip/arm-gic.h | 6 +++ > 3 files changed, 103 insertions(+) > > diff --git a/drivers/irqchip/irq-gic-v2m.c b/drivers/irqchip/irq-gic-v2m.c > index 7e60f7e..290f5b3 100644 > --- a/drivers/irqchip/irq-gic-v2m.c > +++ b/drivers/irqchip/irq-gic-v2m.c > @@ -15,9 +15,11 @@ > > #define pr_fmt(fmt) "GICv2m: " fmt > > +#include <linux/acpi.h> > #include <linux/irq.h> > #include <linux/irqdomain.h> > #include <linux/kernel.h> > +#include <linux/msi.h> > #include <linux/of_address.h> > #include <linux/of_pci.h> > #include <linux/slab.h> > @@ -138,6 +140,11 @@ static int gicv2m_irq_gic_domain_alloc(struct irq_domain *domain, > fwspec.param[0] = 0; > fwspec.param[1] = hwirq - 32; > fwspec.param[2] = IRQ_TYPE_EDGE_RISING; > + } else if (is_fwnode_irqchip(domain->parent->fwnode)) { > + fwspec.fwnode = domain->parent->fwnode; > + fwspec.param_count = 2; > + fwspec.param[0] = hwirq; > + fwspec.param[1] = IRQ_TYPE_EDGE_RISING & IRQ_TYPE_SENSE_MASK; How about just: fwspec.param[1] = IRQ_TYPE_EDGE_RISING; > } else { > return -EINVAL; > } > @@ -255,6 +262,8 @@ static void gicv2m_teardown(void) > kfree(v2m->bm); > iounmap(v2m->base); > of_node_put(to_of_node(v2m->fwnode)); > + if (is_fwnode_irqchip(v2m->fwnode)) > + irq_domain_free_fwnode(v2m->fwnode); > kfree(v2m); > } > } > @@ -359,6 +368,8 @@ static int __init gicv2m_init_one(struct fwnode_handle *fwnode, > > if (to_of_node(fwnode)) > name = to_of_node(fwnode)->name; > + else > + name = irq_domain_get_irqchip_fwnode_name(fwnode); > > pr_info("Frame %s: range[%#lx:%#lx], SPI[%d:%d]\n", name, > (unsigned long)res->start, (unsigned long)res->end, > @@ -415,3 +426,86 @@ int __init gicv2m_of_init(struct device_node *node, struct irq_domain *parent) > gicv2m_teardown(); > return ret; > } > + > +#ifdef CONFIG_ACPI > +static int acpi_num_msi; > + > +static struct fwnode_handle *gicv2m_get_fwnode(struct device *dev) > +{ > + struct v2m_data *data; > + > + if (WARN_ON(acpi_num_msi <= 0)) > + return NULL; > + > + /* We only return the fwnode of the first MSI frame. */ > + data = list_first_entry_or_null(&v2m_nodes, > + struct v2m_data, entry); This can be one line and still fits within 80 characters. > + if (!data) > + return NULL; > + > + return data->fwnode; > +} > + > +static int __init > +acpi_parse_madt_msi(struct acpi_subtable_header *header, > + const unsigned long end) > +{ > + int ret; > + struct resource res; > + u32 spi_start = 0, nr_spis = 0; > + struct acpi_madt_generic_msi_frame *m; > + struct fwnode_handle *fwnode = NULL; > + > + m = (struct acpi_madt_generic_msi_frame *)header; > + if (BAD_MADT_ENTRY(m, end)) > + return -EINVAL; > + > + res.start = m->base_address; > + res.end = m->base_address + 0x1000; > + > + if (m->flags & ACPI_MADT_OVERRIDE_SPI_VALUES) { > + spi_start = m->spi_base; > + nr_spis = m->spi_count; > + > + pr_info("ACPI overriding V2M MSI_TYPER (base:%u, num:%u)\n", > + spi_start, nr_spis); > + } > + > + fwnode = irq_domain_alloc_fwnode((void *)m->base_address); > + if (!fwnode) { > + pr_err("Unable to allocate GICv2m domain token\n"); > + return -EINVAL; > + } > + > + ret = gicv2m_init_one(fwnode, spi_start, nr_spis, &res); I case of error, we should call here: irq_domain_free_fwnode(fwnode); > + > + return ret; > +} > + > +int __init gicv2m_acpi_init(struct irq_domain *parent) > +{ > + int ret; > + > + if (acpi_num_msi > 0) > + return 0; > + > + acpi_num_msi = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_MSI_FRAME, > + acpi_parse_madt_msi, 0); > + > + if (acpi_num_msi <= 0) > + goto err_out; If acpi_table_parse_madt return 0, then we don't need to call gicv2m_teardown(). Instead we can simply return, optionally add some pr_info. Well, gicv2m_teardown would do nothing, so this is just cosmetic and up to you. > + > + ret = gicv2m_allocate_domains(parent); > + if (ret) > + goto err_out; > + > + pci_msi_register_fwnode_provider(&gicv2m_get_fwnode); > + > + return 0; > + > +err_out: > + gicv2m_teardown(); > + return -EINVAL; > +} > + > +#endif /* CONFIG_ACPI */ > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > index 6685b33..bb3e1f2 100644 > --- a/drivers/irqchip/irq-gic.c > +++ b/drivers/irqchip/irq-gic.c > @@ -1329,6 +1329,9 @@ gic_v2_acpi_init(struct acpi_table_header *table) > > __gic_init_bases(0, -1, dist_base, cpu_base, 0, domain_handle); > > + if (IS_ENABLED(CONFIG_ARM_GIC_V2M)) > + gicv2m_acpi_init(gic_data[0].domain); > + > acpi_set_irq_model(ACPI_IRQ_MODEL_GIC, domain_handle); > return 0; > } > diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h > index bae69e5..7398538 100644 > --- a/include/linux/irqchip/arm-gic.h > +++ b/include/linux/irqchip/arm-gic.h > @@ -108,6 +108,12 @@ void gic_init(unsigned int nr, int start, > > int gicv2m_of_init(struct device_node *node, struct irq_domain *parent); > > +#ifdef CONFIG_ACPI > +#include <linux/acpi.h> I think, we don't need this include here. You already added it to itq-gic.c Moreover, seems we need to add irq_domain_free_fwnode to gicv2m_teardown(): static void gicv2m_teardown(void) { struct v2m_data *v2m, *tmp; list_for_each_entry_safe(v2m, tmp, &v2m_nodes, entry) { + struct fwnode_handle *handle = v2m->fwnode; + list_del(&v2m->entry); kfree(v2m->bm); iounmap(v2m->base); - of_node_put(to_of_node(v2m->fwnode)); + if (is_of_node(handle)) + of_node_put(to_of_node(handle)); + else if (is_irqchip_node(handle)) + irq_domain_free_fwnode(handle); kfree(v2m); } } Thanks, Tomasz -- 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
On 15.10.2015 08:15, Tomasz Nowicki wrote: > Hi Suravee, > [...] > Moreover, seems we need to add irq_domain_free_fwnode to gicv2m_teardown(): > static void gicv2m_teardown(void) > { > struct v2m_data *v2m, *tmp; > > list_for_each_entry_safe(v2m, tmp, &v2m_nodes, entry) { > + struct fwnode_handle *handle = v2m->fwnode; > + > list_del(&v2m->entry); > kfree(v2m->bm); > iounmap(v2m->base); > - of_node_put(to_of_node(v2m->fwnode)); > + if (is_of_node(handle)) > + of_node_put(to_of_node(handle)); > + else if (is_irqchip_node(handle)) > + irq_domain_free_fwnode(handle); > kfree(v2m); > } > } > Sorry, I missed you already did something similar in your patch. Tomasz -- 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 Tomasz, Thanks for the feedback. On 10/15/2015 1:15 AM, Tomasz Nowicki wrote: >> [..] >> @@ -138,6 +140,11 @@ static int gicv2m_irq_gic_domain_alloc(struct >> irq_domain *domain, >> fwspec.param[0] = 0; >> fwspec.param[1] = hwirq - 32; >> fwspec.param[2] = IRQ_TYPE_EDGE_RISING; >> + } else if (is_fwnode_irqchip(domain->parent->fwnode)) { >> + fwspec.fwnode = domain->parent->fwnode; >> + fwspec.param_count = 2; >> + fwspec.param[0] = hwirq; >> + fwspec.param[1] = IRQ_TYPE_EDGE_RISING & IRQ_TYPE_SENSE_MASK; > How about just: > fwspec.param[1] = IRQ_TYPE_EDGE_RISING; Right. > >> } else { >> return -EINVAL; >> } >> @@ -255,6 +262,8 @@ static void gicv2m_teardown(void) >> kfree(v2m->bm); >> iounmap(v2m->base); >> of_node_put(to_of_node(v2m->fwnode)); >> + if (is_fwnode_irqchip(v2m->fwnode)) >> + irq_domain_free_fwnode(v2m->fwnode); >> kfree(v2m); >> } >> } >> @@ -359,6 +368,8 @@ static int __init gicv2m_init_one(struct >> fwnode_handle *fwnode, >> >> if (to_of_node(fwnode)) >> name = to_of_node(fwnode)->name; >> + else >> + name = irq_domain_get_irqchip_fwnode_name(fwnode); >> >> pr_info("Frame %s: range[%#lx:%#lx], SPI[%d:%d]\n", name, >> (unsigned long)res->start, (unsigned long)res->end, >> @@ -415,3 +426,86 @@ int __init gicv2m_of_init(struct device_node >> *node, struct irq_domain *parent) >> gicv2m_teardown(); >> return ret; >> } >> + >> +#ifdef CONFIG_ACPI >> +static int acpi_num_msi; >> + >> +static struct fwnode_handle *gicv2m_get_fwnode(struct device *dev) >> +{ >> + struct v2m_data *data; >> + >> + if (WARN_ON(acpi_num_msi <= 0)) >> + return NULL; >> + >> + /* We only return the fwnode of the first MSI frame. */ >> + data = list_first_entry_or_null(&v2m_nodes, >> + struct v2m_data, entry); > This can be one line and still fits within 80 characters. Ok. >> + if (!data) >> + return NULL; >> + >> + return data->fwnode; >> +} >> + >> +static int __init >> +acpi_parse_madt_msi(struct acpi_subtable_header *header, >> + const unsigned long end) >> +{ >> + int ret; >> + struct resource res; >> + u32 spi_start = 0, nr_spis = 0; >> + struct acpi_madt_generic_msi_frame *m; >> + struct fwnode_handle *fwnode = NULL; >> + >> + m = (struct acpi_madt_generic_msi_frame *)header; >> + if (BAD_MADT_ENTRY(m, end)) >> + return -EINVAL; >> + >> + res.start = m->base_address; >> + res.end = m->base_address + 0x1000; >> + >> + if (m->flags & ACPI_MADT_OVERRIDE_SPI_VALUES) { >> + spi_start = m->spi_base; >> + nr_spis = m->spi_count; >> + >> + pr_info("ACPI overriding V2M MSI_TYPER (base:%u, num:%u)\n", >> + spi_start, nr_spis); >> + } >> + >> + fwnode = irq_domain_alloc_fwnode((void *)m->base_address); >> + if (!fwnode) { >> + pr_err("Unable to allocate GICv2m domain token\n"); >> + return -EINVAL; >> + } >> + >> + ret = gicv2m_init_one(fwnode, spi_start, nr_spis, &res); > I case of error, we should call here: > irq_domain_free_fwnode(fwnode); This should have already been handled when returning from the acpi_parse_madt_msi() in gicv2m_teardown() since we would need to iterate through all existing MSI frame to clean up. >> + >> + return ret; >> +} >> + >> +int __init gicv2m_acpi_init(struct irq_domain *parent) >> +{ >> + int ret; >> + >> + if (acpi_num_msi > 0) >> + return 0; >> + >> + acpi_num_msi = >> acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_MSI_FRAME, >> + acpi_parse_madt_msi, 0); >> + >> + if (acpi_num_msi <= 0) >> + goto err_out; > If acpi_table_parse_madt return 0, then we don't need to call > gicv2m_teardown(). Instead we can simply return, optionally add some > pr_info. Well, gicv2m_teardown would do nothing, so this is just > cosmetic and up to you. I'd be hesitate to add pr_info here since V2m is optional, we already print information for each frame found. I think I would just leave this one the way it is. >> [..] >> diff --git a/include/linux/irqchip/arm-gic.h >> b/include/linux/irqchip/arm-gic.h >> index bae69e5..7398538 100644 >> --- a/include/linux/irqchip/arm-gic.h >> +++ b/include/linux/irqchip/arm-gic.h >> @@ -108,6 +108,12 @@ void gic_init(unsigned int nr, int start, >> >> int gicv2m_of_init(struct device_node *node, struct irq_domain >> *parent); >> >> +#ifdef CONFIG_ACPI >> +#include <linux/acpi.h> > I think, we don't need this include here. You already added it to itq-gic.c Right. Thanks, Suravee -- 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 Tomasz, On 10/15/2015 9:03 AM, Suravee Suthikulanit wrote: >>> + if (!data) >>> + return NULL; >>> + >>> + return data->fwnode; >>> +} >>> + >>> +static int __init >>> +acpi_parse_madt_msi(struct acpi_subtable_header *header, >>> + const unsigned long end) >>> +{ >>> + int ret; >>> + struct resource res; >>> + u32 spi_start = 0, nr_spis = 0; >>> + struct acpi_madt_generic_msi_frame *m; >>> + struct fwnode_handle *fwnode = NULL; >>> + >>> + m = (struct acpi_madt_generic_msi_frame *)header; >>> + if (BAD_MADT_ENTRY(m, end)) >>> + return -EINVAL; >>> + >>> + res.start = m->base_address; >>> + res.end = m->base_address + 0x1000; >>> + >>> + if (m->flags & ACPI_MADT_OVERRIDE_SPI_VALUES) { >>> + spi_start = m->spi_base; >>> + nr_spis = m->spi_count; >>> + >>> + pr_info("ACPI overriding V2M MSI_TYPER (base:%u, num:%u)\n", >>> + spi_start, nr_spis); >>> + } >>> + >>> + fwnode = irq_domain_alloc_fwnode((void *)m->base_address); >>> + if (!fwnode) { >>> + pr_err("Unable to allocate GICv2m domain token\n"); >>> + return -EINVAL; >>> + } >>> + >>> + ret = gicv2m_init_one(fwnode, spi_start, nr_spis, &res); >> I case of error, we should call here: >> irq_domain_free_fwnode(fwnode); > > This should have already been handled when returning from the > acpi_parse_madt_msi() in gicv2m_teardown() since we would need to > iterate through all existing MSI frame to clean up. Actually, you are correct since the fwnode allocated here might not get assigned to the v2m_data.fwnode and added to the v2m_nodes list yet. So, we would need to call irq_domain_alloc_fwnode() here in case of error. Thanks, Suravee -- 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-v2m.c b/drivers/irqchip/irq-gic-v2m.c index 7e60f7e..290f5b3 100644 --- a/drivers/irqchip/irq-gic-v2m.c +++ b/drivers/irqchip/irq-gic-v2m.c @@ -15,9 +15,11 @@ #define pr_fmt(fmt) "GICv2m: " fmt +#include <linux/acpi.h> #include <linux/irq.h> #include <linux/irqdomain.h> #include <linux/kernel.h> +#include <linux/msi.h> #include <linux/of_address.h> #include <linux/of_pci.h> #include <linux/slab.h> @@ -138,6 +140,11 @@ static int gicv2m_irq_gic_domain_alloc(struct irq_domain *domain, fwspec.param[0] = 0; fwspec.param[1] = hwirq - 32; fwspec.param[2] = IRQ_TYPE_EDGE_RISING; + } else if (is_fwnode_irqchip(domain->parent->fwnode)) { + fwspec.fwnode = domain->parent->fwnode; + fwspec.param_count = 2; + fwspec.param[0] = hwirq; + fwspec.param[1] = IRQ_TYPE_EDGE_RISING & IRQ_TYPE_SENSE_MASK; } else { return -EINVAL; } @@ -255,6 +262,8 @@ static void gicv2m_teardown(void) kfree(v2m->bm); iounmap(v2m->base); of_node_put(to_of_node(v2m->fwnode)); + if (is_fwnode_irqchip(v2m->fwnode)) + irq_domain_free_fwnode(v2m->fwnode); kfree(v2m); } } @@ -359,6 +368,8 @@ static int __init gicv2m_init_one(struct fwnode_handle *fwnode, if (to_of_node(fwnode)) name = to_of_node(fwnode)->name; + else + name = irq_domain_get_irqchip_fwnode_name(fwnode); pr_info("Frame %s: range[%#lx:%#lx], SPI[%d:%d]\n", name, (unsigned long)res->start, (unsigned long)res->end, @@ -415,3 +426,86 @@ int __init gicv2m_of_init(struct device_node *node, struct irq_domain *parent) gicv2m_teardown(); return ret; } + +#ifdef CONFIG_ACPI +static int acpi_num_msi; + +static struct fwnode_handle *gicv2m_get_fwnode(struct device *dev) +{ + struct v2m_data *data; + + if (WARN_ON(acpi_num_msi <= 0)) + return NULL; + + /* We only return the fwnode of the first MSI frame. */ + data = list_first_entry_or_null(&v2m_nodes, + struct v2m_data, entry); + if (!data) + return NULL; + + return data->fwnode; +} + +static int __init +acpi_parse_madt_msi(struct acpi_subtable_header *header, + const unsigned long end) +{ + int ret; + struct resource res; + u32 spi_start = 0, nr_spis = 0; + struct acpi_madt_generic_msi_frame *m; + struct fwnode_handle *fwnode = NULL; + + m = (struct acpi_madt_generic_msi_frame *)header; + if (BAD_MADT_ENTRY(m, end)) + return -EINVAL; + + res.start = m->base_address; + res.end = m->base_address + 0x1000; + + if (m->flags & ACPI_MADT_OVERRIDE_SPI_VALUES) { + spi_start = m->spi_base; + nr_spis = m->spi_count; + + pr_info("ACPI overriding V2M MSI_TYPER (base:%u, num:%u)\n", + spi_start, nr_spis); + } + + fwnode = irq_domain_alloc_fwnode((void *)m->base_address); + if (!fwnode) { + pr_err("Unable to allocate GICv2m domain token\n"); + return -EINVAL; + } + + ret = gicv2m_init_one(fwnode, spi_start, nr_spis, &res); + + return ret; +} + +int __init gicv2m_acpi_init(struct irq_domain *parent) +{ + int ret; + + if (acpi_num_msi > 0) + return 0; + + acpi_num_msi = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_MSI_FRAME, + acpi_parse_madt_msi, 0); + + if (acpi_num_msi <= 0) + goto err_out; + + ret = gicv2m_allocate_domains(parent); + if (ret) + goto err_out; + + pci_msi_register_fwnode_provider(&gicv2m_get_fwnode); + + return 0; + +err_out: + gicv2m_teardown(); + return -EINVAL; +} + +#endif /* CONFIG_ACPI */ diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index 6685b33..bb3e1f2 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -1329,6 +1329,9 @@ gic_v2_acpi_init(struct acpi_table_header *table) __gic_init_bases(0, -1, dist_base, cpu_base, 0, domain_handle); + if (IS_ENABLED(CONFIG_ARM_GIC_V2M)) + gicv2m_acpi_init(gic_data[0].domain); + acpi_set_irq_model(ACPI_IRQ_MODEL_GIC, domain_handle); return 0; } diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h index bae69e5..7398538 100644 --- a/include/linux/irqchip/arm-gic.h +++ b/include/linux/irqchip/arm-gic.h @@ -108,6 +108,12 @@ void gic_init(unsigned int nr, int start, int gicv2m_of_init(struct device_node *node, struct irq_domain *parent); +#ifdef CONFIG_ACPI +#include <linux/acpi.h> + +int gicv2m_acpi_init(struct irq_domain *parent); +#endif + void gic_send_sgi(unsigned int cpu_id, unsigned int irq); int gic_get_cpu_id(unsigned int cpu); void gic_migrate_target(unsigned int new_cpu_id);