Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 01 Apr 2019 03:15:19 +0100
Message-ID: <86pnq65v48.wl-marc.zyngier@arm.com>
From:   Marc Zyngier <marc.zyngier@arm.com>
To:     Hanna Hawa <hhhawa@amazon.com>
Cc:     <tsahee@annapurnalabs.com>, <antoine.tenart@bootlin.com>,
        <linux@armlinux.org.uk>, <catalin.marinas@arm.com>,
        <will.deacon@arm.com>, <rjw@rjwysocki.net>, <lenb@kernel.org>,
        <tglx@linutronix.de>, <jason@lakedaemon.net>, <ronenk@amazon.com>,
        <dwmw@amazon.co.uk>, <vaerov@amazon.com>, <zeev@amazon.com>,
        <alisaidi@amazon.com>, <talel@amazon.com>, <jonnyc@amazon.com>,
        <hanochu@amazon.com>, <barakw@amazon.com>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>, <linux-acpi@vger.kernel.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Subject: Re: [PATCH 7/7] irqchip/al-msi: Add ACPI support
In-Reply-To: <1554035733-11827-3-git-send-email-hhhawa@amazon.com>
References: <1554035733-11827-1-git-send-email-hhhawa@amazon.com>
        <1554035733-11827-3-git-send-email-hhhawa@amazon.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 EasyPG/1.0.0 Emacs/26
 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO)
Organization: ARM Ltd
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, 31 Mar 2019 13:35:33 +0100,
Hanna Hawa <hhhawa@amazon.com> wrote:
> 
> This patch adds ACPI support for AL-MSIx driver.
> 
> The AL-MSIx controller is not standard, is not included in the UEFI
> specification, and will not be added. The driver ACPI binding is
> performed when the following conditions are true:
> - OEM ID is AMAZON
> - MADT table type is 0x80 (part of the OEM reserved range).
> 
> Signed-off-by: Hanna Hawa <hhhawa@amazon.com>
> Co-developed-by: Vladimir Aerov <vaerov@amazon.com>
> Signed-off-by: Vladimir Aerov <vaerov@amazon.com>
> ---
>  drivers/irqchip/irq-al-msi.c | 118 ++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 111 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-al-msi.c b/drivers/irqchip/irq-al-msi.c
> index ec27455..cb80c1e 100644
> --- a/drivers/irqchip/irq-al-msi.c
> +++ b/drivers/irqchip/irq-al-msi.c
> @@ -9,6 +9,7 @@
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
> +#include <linux/acpi.h>
>  #include <linux/dma-iommu.h>
>  #include <linux/irqchip.h>
>  #include <linux/irqchip/arm-gic.h>
> @@ -126,14 +127,20 @@ static int al_msix_gic_domain_alloc(struct irq_domain *domain,
>  	struct irq_data *d;
>  	int ret;
>  
> -	if (!is_of_node(domain->parent->fwnode))
> +	if (is_of_node(domain->parent->fwnode)) {
> +		fwspec.fwnode = domain->parent->fwnode;
> +		fwspec.param_count = 3;
> +		fwspec.param[0] = 0;
> +		fwspec.param[1] = spi;
> +		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] = spi + 32;
> +		fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
> +	} else {
>  		return -EINVAL;
> -
> -	fwspec.fwnode = domain->parent->fwnode;
> -	fwspec.param_count = 3;
> -	fwspec.param[0] = 0;
> -	fwspec.param[1] = spi;
> -	fwspec.param[2] = IRQ_TYPE_EDGE_RISING;
> +	}
>  
>  	ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec);
>  	if (ret)
> @@ -304,3 +311,100 @@ static int al_msix_init(struct device_node *node, struct device_node *parent)
>  }
>  IRQCHIP_DECLARE(alpine_msix, "al,alpine-msix", al_msix_init);
>  IRQCHIP_DECLARE(al_msix, "amazon,al-msix", al_msix_init);
> +
> +#ifdef CONFIG_ACPI
> +static struct al_msix_data *priv;
> +
> +#define ACPI_AMZN_MADT_OEM_TYPE		0x80
> +#define ACPI_AMZN_OEM_ID		"AMAZON"
> +
> +struct acpi_madt_msix_oem_frame {
> +	struct acpi_subtable_header header;
> +	u64 base_address;
> +	u32 base_address_len;
> +	u16 spi_count;
> +	u16 spi_base;
> +};
> +
> +static struct fwnode_handle *al_msi_get_fwnode(struct device *dev)
> +{
> +	return priv->msi_domain_handle;
> +}
> +
> +static int __init al_msix_acpi_probe(struct acpi_subtable_header *header,
> +				     const unsigned long end)
> +{
> +	struct irq_domain *gic_domain;
> +	struct fwnode_handle *gic_domain_handle;
> +	struct acpi_madt_msix_oem_frame *m;
> +	int ret;
> +
> +	m = container_of(header, struct acpi_madt_msix_oem_frame, header);
> +	if (BAD_MADT_ENTRY(m, end))
> +		return -EINVAL;
> +
> +	gic_domain_handle = acpi_get_gsi_domain_id();
> +	if (!gic_domain_handle) {
> +		pr_err("Failed to find the GIC domain handle\n");
> +		return -ENXIO;
> +	}
> +
> +	gic_domain = irq_find_matching_fwnode(gic_domain_handle,
> +					      DOMAIN_BUS_ANY);
> +	if (!gic_domain) {
> +		pr_err("Failed to find the GIC domain\n");
> +		return -ENXIO;
> +	}

Why do you do this in the iterator? It won't change over time, right?

> +
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->spi_first = m->spi_base;
> +	priv->num_spis = m->spi_count;
> +
> +	priv->msi_domain_handle = irq_domain_alloc_fwnode((void *)
> +							  m->base_address);
> +	if (!priv->msi_domain_handle) {
> +		pr_err("Unable to allocate msi domain token\n");
> +		ret = -EINVAL;
> +		goto err_acpi_priv;
> +	}
> +
> +	ret = al_msix_init_common(priv, m->base_address);
> +	if (ret)
> +		goto err_acpi_priv;
> +
> +	ret = al_msix_init_domains(priv, gic_domain);
> +	if (ret)
> +		goto err_acpi_map;
> +
> +	pci_msi_register_fwnode_provider(&al_msi_get_fwnode);
> +
> +	return 0;
> +
> +err_acpi_map:
> +	kfree(priv->msi_map);
> +err_acpi_priv:
> +	kfree(priv);
> +	return ret;
> +}
> +
> +static int __init al_msix_acpi_init(void)
> +{
> +	static struct acpi_table_madt *madt;
> +	acpi_status status;
> +
> +	/* if ACPI MADT table is not Amazon defined return */
> +	status = acpi_get_table(ACPI_SIG_MADT, 0,
> +				(struct acpi_table_header **)&madt);
> +	if (ACPI_FAILURE(status) || (madt && memcmp(madt->header.oem_id,
> +						    ACPI_AMZN_OEM_ID,
> +						    ACPI_OEM_ID_SIZE)))
> +		return -ENODEV;
> +
> +	return acpi_table_parse_madt(ACPI_AMZN_MADT_OEM_TYPE,
> +				     al_msix_acpi_probe, 0);
> +}
> +early_initcall(al_msix_acpi_init);

These initcalls are not maintainable in the long run. Either this is
part of the core ACPI discovery (IRQCHIP_ACPI_DECLARE), or it should
use another probing method. If you need to express dependencies, make
it an actual device driver (I suspect you're in complete control of
the FW anyway), which would make a lot more sense.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.