Re: [PATCH V2 6/6] gicv2m: acpi: Introducing GICv2m ACPI support

[Tomasz Nowicki <tn@semihalf.com>]

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