[v3,pci] PCI/MSI: pci-xgene-msi: Enable MSI support in ACPI boot for X-Gene v1

This patch makes pci-xgene-msi driver ACPI-aware and provides
MSI capability for X-Gene v1 PCIe controllers in ACPI boot mode.

Signed-off-by: Khuong Dinh <kdinh@apm.com>
        [Take over from Duc Dang]
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
---
v3:
 - Input X-Gene MSI base address for irq_domain_alloc_fwnode
 - Add a hook to enforce X-Gene MSI be probed prior acpi_bus_scan happens
v2:
 - Verify with BIOS version 3.06.25 and 3.07.09
v1:
 - Initial version
---
 drivers/acpi/Makefile            |    2 +-
 drivers/acpi/acpi_msi.c          |   74 ++++++++++++++++++++++++++++++++++++++
 drivers/acpi/internal.h          |    1 +
 drivers/acpi/scan.c              |    1 +
 drivers/pci/host/pci-xgene-msi.c |   36 +++++++++++++++++--
 5 files changed, 110 insertions(+), 4 deletions(-)
 create mode 100644 drivers/acpi/acpi_msi.c

On 12/08/17 01:02, Khuong Dinh wrote:
> This patch makes pci-xgene-msi driver ACPI-aware and provides
> MSI capability for X-Gene v1 PCIe controllers in ACPI boot mode.
> 
> Signed-off-by: Khuong Dinh <kdinh@apm.com>
>         [Take over from Duc Dang]
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>

Given that the patch has changed very substantially, this Ack is no
longer valid.

> ---
> v3:
>  - Input X-Gene MSI base address for irq_domain_alloc_fwnode
>  - Add a hook to enforce X-Gene MSI be probed prior acpi_bus_scan happens
> v2:
>  - Verify with BIOS version 3.06.25 and 3.07.09
> v1:
>  - Initial version
> ---
>  drivers/acpi/Makefile            |    2 +-
>  drivers/acpi/acpi_msi.c          |   74 ++++++++++++++++++++++++++++++++++++++
>  drivers/acpi/internal.h          |    1 +
>  drivers/acpi/scan.c              |    1 +
>  drivers/pci/host/pci-xgene-msi.c |   36 +++++++++++++++++--
>  5 files changed, 110 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/acpi/acpi_msi.c
> 
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index b1aacfc..c15f5cd 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -40,7 +40,7 @@ acpi-y				+= ec.o
>  acpi-$(CONFIG_ACPI_DOCK)	+= dock.o
>  acpi-y				+= pci_root.o pci_link.o pci_irq.o
>  obj-$(CONFIG_ACPI_MCFG)		+= pci_mcfg.o
> -acpi-y				+= acpi_lpss.o acpi_apd.o
> +acpi-y				+= acpi_lpss.o acpi_apd.o acpi_msi.o
>  acpi-y				+= acpi_platform.o
>  acpi-y				+= acpi_pnp.o
>  acpi-$(CONFIG_ARM_AMBA)	+= acpi_amba.o
> diff --git a/drivers/acpi/acpi_msi.c b/drivers/acpi/acpi_msi.c
> new file mode 100644
> index 0000000..c2f8d26
> --- /dev/null
> +++ b/drivers/acpi/acpi_msi.c
> @@ -0,0 +1,74 @@
> +/*
> + * Enforce MSI driver loaded by PCIe controller driver
> + *
> + * Copyright (c) 2017, MACOM Technology Solutions Corporation
> + * Author: Khuong Dinh <kdinh@apm.com>
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/acpi.h>
> +#include "internal.h"
> +
> +static acpi_status acpi_create_msi_device(acpi_handle handle, u32 Level,
> +					void *context, void **retval)
> +{
> +	struct acpi_device *device = NULL;
> +	int type = ACPI_BUS_TYPE_DEVICE;
> +	unsigned long long sta;
> +	acpi_status status;
> +	int ret = 0;
> +
> +	acpi_bus_get_device(handle, &device);
> +	status = acpi_bus_get_status_handle(handle, &sta);
> +	if (ACPI_FAILURE(status))
> +		sta = 0;
> +
> +	device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
> +	if (!device)
> +		return -ENOMEM;
> +
> +	acpi_init_device_object(device, handle, type, sta);
> +	ret = acpi_device_add(device, NULL);
> +	if (ret)
> +		return ret;

Hello, memory leak.

> +	device->parent = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
> +	INIT_LIST_HEAD(&device->parent->physical_node_list);
> +
> +	acpi_device_add_finalize(device);
> +
> +	ret = device_attach(&device->dev);
> +	if (ret < 0)
> +		return ret;

And another one.

> +
> +	acpi_create_platform_device(device, NULL);
> +	acpi_device_set_enumerated(device);
> +
> +	return ret;
> +}
> +
> +static const struct acpi_device_id acpi_msi_device_ids[] = {
> +	{"APMC0D0E", 0},

Isn't this an APM-specific thing? Is that supposed to be populated by
all MSI controller drivers? If so, this would better be served by a
separate linker section.

> +	{ }
> +};
> +
> +int __init acpi_msi_init(void)
> +{
> +	acpi_status status;
> +	int ret = 0;
> +
> +	status = acpi_get_devices(acpi_msi_device_ids[0].id,
> +			acpi_create_msi_device, NULL, NULL);
> +	if (ACPI_FAILURE(status))
> +		ret = -ENODEV;
> +
> +	return ret;
> +}

Overall, this part should be a separate patch, and you should start by
explaining what it does exactly. Because so far, all I can see is that
it pre-populates random ACPI device structures, and that definitely seem
fishy to me.

> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index 58dd7ab..3da50d3 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -80,6 +80,7 @@ int acpi_scan_add_handler_with_hotplug(struct acpi_scan_handler *handler,
>  void acpi_lpss_init(void);
>  
>  void acpi_apd_init(void);
> +int acpi_msi_init(void);
>  
>  acpi_status acpi_hotplug_schedule(struct acpi_device *adev, u32 src);
>  bool acpi_queue_hotplug_work(struct work_struct *work);
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 3389729..8ec4d39 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -2029,6 +2029,7 @@ int __init acpi_scan_init(void)
>  	acpi_status status;
>  	struct acpi_table_stao *stao_ptr;
>  
> +	acpi_msi_init();
>  	acpi_pci_root_init();
>  	acpi_pci_link_init();
>  	acpi_processor_init();
> diff --git a/drivers/pci/host/pci-xgene-msi.c b/drivers/pci/host/pci-xgene-msi.c
> index f1b633b..b1768a1 100644
> --- a/drivers/pci/host/pci-xgene-msi.c
> +++ b/drivers/pci/host/pci-xgene-msi.c
> @@ -24,6 +24,7 @@
>  #include <linux/pci.h>
>  #include <linux/platform_device.h>
>  #include <linux/of_pci.h>
> +#include <linux/acpi.h>
>  
>  #define MSI_IR0			0x000000
>  #define MSI_INT0		0x800000
> @@ -39,7 +40,7 @@ struct xgene_msi_group {
>  };
>  
>  struct xgene_msi {
> -	struct device_node	*node;
> +	struct fwnode_handle	*fwnode;
>  	struct irq_domain	*inner_domain;
>  	struct irq_domain	*msi_domain;
>  	u64			msi_addr;
> @@ -249,6 +250,13 @@ static void xgene_irq_domain_free(struct irq_domain *domain,
>  	.free   = xgene_irq_domain_free,
>  };
>  
> +#ifdef CONFIG_ACPI
> +static struct fwnode_handle *xgene_msi_get_fwnode(struct device *dev)
> +{
> +	return xgene_msi_ctrl.fwnode;
> +}
> +#endif
> +
>  static int xgene_allocate_domains(struct xgene_msi *msi)
>  {
>  	msi->inner_domain = irq_domain_add_linear(NULL, NR_MSI_VEC,
> @@ -256,7 +264,7 @@ static int xgene_allocate_domains(struct xgene_msi *msi)
>  	if (!msi->inner_domain)
>  		return -ENOMEM;
>  
> -	msi->msi_domain = pci_msi_create_irq_domain(of_node_to_fwnode(msi->node),
> +	msi->msi_domain = pci_msi_create_irq_domain(msi->fwnode,
>  						    &xgene_msi_domain_info,
>  						    msi->inner_domain);
>  
> @@ -265,6 +273,9 @@ static int xgene_allocate_domains(struct xgene_msi *msi)
>  		return -ENOMEM;
>  	}
>  
> +#ifdef CONFIG_ACPI
> +	pci_msi_register_fwnode_provider(&xgene_msi_get_fwnode);
> +#endif
>  	return 0;
>  }
>  
> @@ -449,6 +460,13 @@ static int xgene_msi_hwirq_free(unsigned int cpu)
>  	{},
>  };
>  
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id xgene_msi_acpi_ids[] = {
> +	{"APMC0D0E", 0},
> +	{ },
> +};
> +#endif
> +
>  static int xgene_msi_probe(struct platform_device *pdev)
>  {
>  	struct resource *res;
> @@ -469,7 +487,18 @@ static int xgene_msi_probe(struct platform_device *pdev)
>  		goto error;
>  	}
>  	xgene_msi->msi_addr = res->start;
> -	xgene_msi->node = pdev->dev.of_node;
> +
> +	xgene_msi->fwnode = of_node_to_fwnode(pdev->dev.of_node);
> +	if (!xgene_msi->fwnode) {
> +		xgene_msi->fwnode =
> +			irq_domain_alloc_fwnode((void *)xgene_msi->msi_addr);
> +		if (!xgene_msi->fwnode) {
> +			dev_err(&pdev->dev, "Failed to create fwnode\n");
> +			rc = ENOMEM;
> +			goto error;
> +		}
> +	}
> +
>  	xgene_msi->num_cpus = num_possible_cpus();
>  
>  	rc = xgene_msi_init_allocator(xgene_msi);
> @@ -540,6 +569,7 @@ static int xgene_msi_probe(struct platform_device *pdev)
>  	.driver = {
>  		.name = "xgene-msi",
>  		.of_match_table = xgene_msi_match_table,
> +		.acpi_match_table = ACPI_PTR(xgene_msi_acpi_ids),
>  	},
>  	.probe = xgene_msi_probe,
>  	.remove = xgene_msi_remove,
> 

Thanks,

	M.

On Fri, Aug 11, 2017 at 06:02:53PM -0600, Khuong Dinh wrote:
> This patch makes pci-xgene-msi driver ACPI-aware and provides
> MSI capability for X-Gene v1 PCIe controllers in ACPI boot mode.
> 
> Signed-off-by: Khuong Dinh <kdinh@apm.com>
>         [Take over from Duc Dang]
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
> v3:
>  - Input X-Gene MSI base address for irq_domain_alloc_fwnode
>  - Add a hook to enforce X-Gene MSI be probed prior acpi_bus_scan happens
> v2:
>  - Verify with BIOS version 3.06.25 and 3.07.09
> v1:
>  - Initial version
> ---
>  drivers/acpi/Makefile            |    2 +-
>  drivers/acpi/acpi_msi.c          |   74 ++++++++++++++++++++++++++++++++++++++
>  drivers/acpi/internal.h          |    1 +
>  drivers/acpi/scan.c              |    1 +
>  drivers/pci/host/pci-xgene-msi.c |   36 +++++++++++++++++--
>  5 files changed, 110 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/acpi/acpi_msi.c
> 
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index b1aacfc..c15f5cd 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -40,7 +40,7 @@ acpi-y				+= ec.o
>  acpi-$(CONFIG_ACPI_DOCK)	+= dock.o
>  acpi-y				+= pci_root.o pci_link.o pci_irq.o
>  obj-$(CONFIG_ACPI_MCFG)		+= pci_mcfg.o
> -acpi-y				+= acpi_lpss.o acpi_apd.o
> +acpi-y				+= acpi_lpss.o acpi_apd.o acpi_msi.o
>  acpi-y				+= acpi_platform.o
>  acpi-y				+= acpi_pnp.o
>  acpi-$(CONFIG_ARM_AMBA)	+= acpi_amba.o
> diff --git a/drivers/acpi/acpi_msi.c b/drivers/acpi/acpi_msi.c
> new file mode 100644
> index 0000000..c2f8d26
> --- /dev/null
> +++ b/drivers/acpi/acpi_msi.c
> @@ -0,0 +1,74 @@
> +/*
> + * Enforce MSI driver loaded by PCIe controller driver
> + *
> + * Copyright (c) 2017, MACOM Technology Solutions Corporation
> + * Author: Khuong Dinh <kdinh@apm.com>
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/acpi.h>
> +#include "internal.h"
> +
> +static acpi_status acpi_create_msi_device(acpi_handle handle, u32 Level,
> +					void *context, void **retval)
> +{
> +	struct acpi_device *device = NULL;
> +	int type = ACPI_BUS_TYPE_DEVICE;
> +	unsigned long long sta;
> +	acpi_status status;
> +	int ret = 0;
> +
> +	acpi_bus_get_device(handle, &device);
> +	status = acpi_bus_get_status_handle(handle, &sta);
> +	if (ACPI_FAILURE(status))
> +		sta = 0;
> +
> +	device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
> +	if (!device)
> +		return -ENOMEM;
> +
> +	acpi_init_device_object(device, handle, type, sta);
> +	ret = acpi_device_add(device, NULL);
> +	if (ret)
> +		return ret;
> +	device->parent = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
> +	INIT_LIST_HEAD(&device->parent->physical_node_list);
> +
> +	acpi_device_add_finalize(device);
> +
> +	ret = device_attach(&device->dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	acpi_create_platform_device(device, NULL);
> +	acpi_device_set_enumerated(device);
> +
> +	return ret;
> +}

Can't this be implemented through acpi_bus_scan(handle) ?

(where handle is your MSI controller acpi_handle ?)

[...]

>  static int xgene_msi_probe(struct platform_device *pdev)
>  {
>  	struct resource *res;
> @@ -469,7 +487,18 @@ static int xgene_msi_probe(struct platform_device *pdev)
>  		goto error;
>  	}
>  	xgene_msi->msi_addr = res->start;
> -	xgene_msi->node = pdev->dev.of_node;
> +
> +	xgene_msi->fwnode = of_node_to_fwnode(pdev->dev.of_node);
> +	if (!xgene_msi->fwnode) {
> +		xgene_msi->fwnode =
> +			irq_domain_alloc_fwnode((void *)xgene_msi->msi_addr);
> +		if (!xgene_msi->fwnode) {
> +			dev_err(&pdev->dev, "Failed to create fwnode\n");
> +			rc = ENOMEM;
> +			goto error;
> +		}
> +	}
> +
>  	xgene_msi->num_cpus = num_possible_cpus();
>  
>  	rc = xgene_msi_init_allocator(xgene_msi);
> @@ -540,6 +569,7 @@ static int xgene_msi_probe(struct platform_device *pdev)
>  	.driver = {
>  		.name = "xgene-msi",
>  		.of_match_table = xgene_msi_match_table,
> +		.acpi_match_table = ACPI_PTR(xgene_msi_acpi_ids),
>  	},
>  	.probe = xgene_msi_probe,
>  	.remove = xgene_msi_remove,

AFAIK you still have not solved the link ordering problem, creating
the platform device before scanning the PCI root bridge is not enough,
because you are not guaranteed to probe the MSI driver first, there
has to be way for registering your driver from the ACPI scan hook.

Thanks,
Lorenzo

Hi Marc,

On Mon, Aug 14, 2017 at 1:21 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 12/08/17 01:02, Khuong Dinh wrote:
>> This patch makes pci-xgene-msi driver ACPI-aware and provides
>> MSI capability for X-Gene v1 PCIe controllers in ACPI boot mode.
>>
>> Signed-off-by: Khuong Dinh <kdinh@apm.com>
>>         [Take over from Duc Dang]
>> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
>
> Given that the patch has changed very substantially, this Ack is no
> longer valid.

Yes, I'll remove it.

>> ---
>> v3:
>>  - Input X-Gene MSI base address for irq_domain_alloc_fwnode
>>  - Add a hook to enforce X-Gene MSI be probed prior acpi_bus_scan happens
>> v2:
>>  - Verify with BIOS version 3.06.25 and 3.07.09
>> v1:
>>  - Initial version
>> ---
>>  drivers/acpi/Makefile            |    2 +-
>>  drivers/acpi/acpi_msi.c          |   74 ++++++++++++++++++++++++++++++++++++++
>>  drivers/acpi/internal.h          |    1 +
>>  drivers/acpi/scan.c              |    1 +
>>  drivers/pci/host/pci-xgene-msi.c |   36 +++++++++++++++++--
>>  5 files changed, 110 insertions(+), 4 deletions(-)
>>  create mode 100644 drivers/acpi/acpi_msi.c
>>
>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
>> index b1aacfc..c15f5cd 100644
>> --- a/drivers/acpi/Makefile
>> +++ b/drivers/acpi/Makefile
>> @@ -40,7 +40,7 @@ acpi-y                              += ec.o
>>  acpi-$(CONFIG_ACPI_DOCK)     += dock.o
>>  acpi-y                               += pci_root.o pci_link.o pci_irq.o
>>  obj-$(CONFIG_ACPI_MCFG)              += pci_mcfg.o
>> -acpi-y                               += acpi_lpss.o acpi_apd.o
>> +acpi-y                               += acpi_lpss.o acpi_apd.o acpi_msi.o
>>  acpi-y                               += acpi_platform.o
>>  acpi-y                               += acpi_pnp.o
>>  acpi-$(CONFIG_ARM_AMBA)      += acpi_amba.o
>> diff --git a/drivers/acpi/acpi_msi.c b/drivers/acpi/acpi_msi.c
>> new file mode 100644
>> index 0000000..c2f8d26
>> --- /dev/null
>> +++ b/drivers/acpi/acpi_msi.c
>> @@ -0,0 +1,74 @@
>> +/*
>> + * Enforce MSI driver loaded by PCIe controller driver
>> + *
>> + * Copyright (c) 2017, MACOM Technology Solutions Corporation
>> + * Author: Khuong Dinh <kdinh@apm.com>
>> + *
>> + * This program is free software; you can redistribute  it and/or modify it
>> + * under  the terms of  the GNU General  Public License as published by the
>> + * Free Software Foundation;  either version 2 of the  License, or (at your
>> + * option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/acpi.h>
>> +#include "internal.h"
>> +
>> +static acpi_status acpi_create_msi_device(acpi_handle handle, u32 Level,
>> +                                     void *context, void **retval)
>> +{
>> +     struct acpi_device *device = NULL;
>> +     int type = ACPI_BUS_TYPE_DEVICE;
>> +     unsigned long long sta;
>> +     acpi_status status;
>> +     int ret = 0;
>> +
>> +     acpi_bus_get_device(handle, &device);
>> +     status = acpi_bus_get_status_handle(handle, &sta);
>> +     if (ACPI_FAILURE(status))
>> +             sta = 0;
>> +
>> +     device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
>> +     if (!device)
>> +             return -ENOMEM;
>> +
>> +     acpi_init_device_object(device, handle, type, sta);
>> +     ret = acpi_device_add(device, NULL);
>> +     if (ret)
>> +             return ret;
>
> Hello, memory leak.

I'll update the code to release the memory when the error happens.

>> +     device->parent = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
>> +     INIT_LIST_HEAD(&device->parent->physical_node_list);
>> +
>> +     acpi_device_add_finalize(device);
>> +
>> +     ret = device_attach(&device->dev);
>> +     if (ret < 0)
>> +             return ret;
>
> And another one.

I'll update the code to release the memory when the error happens.

>> +
>> +     acpi_create_platform_device(device, NULL);
>> +     acpi_device_set_enumerated(device);
>> +
>> +     return ret;
>> +}
>> +
>> +static const struct acpi_device_id acpi_msi_device_ids[] = {
>> +     {"APMC0D0E", 0},
>
> Isn't this an APM-specific thing? Is that supposed to be populated by
> all MSI controller drivers? If so, this would better be served by a
> separate linker section.

The code is generic for MSI controller drivers.
It will walk through the HID to get the MSI device and enforce it
happens before acpi_bus_scan.
This mechanism will help to avoid MSI driver from relying on ACPI
device nodes ordering.
For now, it just supports for APM X-Gene v1.

>> +     { }
>> +};
>> +
>> +int __init acpi_msi_init(void)
>> +{
>> +     acpi_status status;
>> +     int ret = 0;
>> +
>> +     status = acpi_get_devices(acpi_msi_device_ids[0].id,
>> +                     acpi_create_msi_device, NULL, NULL);
>> +     if (ACPI_FAILURE(status))
>> +             ret = -ENODEV;
>> +
>> +     return ret;
>> +}
>
> Overall, this part should be a separate patch, and you should start by
> explaining what it does exactly. Because so far, all I can see is that
> it pre-populates random ACPI device structures, and that definitely seem
> fishy to me.

I'm still following Lorenzo's approach to resolve the general ACPI
dependence issue.
As soon as it's fine, I'll re-visit my prior suggestion to separate
the original patch to enable ACPI support for PCIe MSI and the general
ACPI dependence issue.
The patch is to add a hook in drivers/acpi/scan.c to enforce MSI
driver to be initialized and registered before acpi_bus_scan happens,
so that MSI driver will not rely on ACPI device nodes ordering.
It will walk through the HID to get the MSI device (e.g:
acip_get_devices). When the device is found, the callback will be
called and we will initialize MSI device by its handle, start to
attach MSI device to a driver, create ACPI platform device for MSI
APCI device node and marked it as visited.
In result, it will happen on top of acpi_bus_scan.

>> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
>> index 58dd7ab..3da50d3 100644
>> --- a/drivers/acpi/internal.h
>> +++ b/drivers/acpi/internal.h
>> @@ -80,6 +80,7 @@ int acpi_scan_add_handler_with_hotplug(struct acpi_scan_handler *handler,
>>  void acpi_lpss_init(void);
>>
>>  void acpi_apd_init(void);
>> +int acpi_msi_init(void);
>>
>>  acpi_status acpi_hotplug_schedule(struct acpi_device *adev, u32 src);
>>  bool acpi_queue_hotplug_work(struct work_struct *work);
>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> index 3389729..8ec4d39 100644
>> --- a/drivers/acpi/scan.c
>> +++ b/drivers/acpi/scan.c
>> @@ -2029,6 +2029,7 @@ int __init acpi_scan_init(void)
>>       acpi_status status;
>>       struct acpi_table_stao *stao_ptr;
>>
>> +     acpi_msi_init();
>>       acpi_pci_root_init();
>>       acpi_pci_link_init();
>>       acpi_processor_init();
>> diff --git a/drivers/pci/host/pci-xgene-msi.c b/drivers/pci/host/pci-xgene-msi.c
>> index f1b633b..b1768a1 100644
>> --- a/drivers/pci/host/pci-xgene-msi.c
>> +++ b/drivers/pci/host/pci-xgene-msi.c
>> @@ -24,6 +24,7 @@
>>  #include <linux/pci.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/of_pci.h>
>> +#include <linux/acpi.h>
>>
>>  #define MSI_IR0                      0x000000
>>  #define MSI_INT0             0x800000
>> @@ -39,7 +40,7 @@ struct xgene_msi_group {
>>  };
>>
>>  struct xgene_msi {
>> -     struct device_node      *node;
>> +     struct fwnode_handle    *fwnode;
>>       struct irq_domain       *inner_domain;
>>       struct irq_domain       *msi_domain;
>>       u64                     msi_addr;
>> @@ -249,6 +250,13 @@ static void xgene_irq_domain_free(struct irq_domain *domain,
>>       .free   = xgene_irq_domain_free,
>>  };
>>
>> +#ifdef CONFIG_ACPI
>> +static struct fwnode_handle *xgene_msi_get_fwnode(struct device *dev)
>> +{
>> +     return xgene_msi_ctrl.fwnode;
>> +}
>> +#endif
>> +
>>  static int xgene_allocate_domains(struct xgene_msi *msi)
>>  {
>>       msi->inner_domain = irq_domain_add_linear(NULL, NR_MSI_VEC,
>> @@ -256,7 +264,7 @@ static int xgene_allocate_domains(struct xgene_msi *msi)
>>       if (!msi->inner_domain)
>>               return -ENOMEM;
>>
>> -     msi->msi_domain = pci_msi_create_irq_domain(of_node_to_fwnode(msi->node),
>> +     msi->msi_domain = pci_msi_create_irq_domain(msi->fwnode,
>>                                                   &xgene_msi_domain_info,
>>                                                   msi->inner_domain);
>>
>> @@ -265,6 +273,9 @@ static int xgene_allocate_domains(struct xgene_msi *msi)
>>               return -ENOMEM;
>>       }
>>
>> +#ifdef CONFIG_ACPI
>> +     pci_msi_register_fwnode_provider(&xgene_msi_get_fwnode);
>> +#endif
>>       return 0;
>>  }
>>
>> @@ -449,6 +460,13 @@ static int xgene_msi_hwirq_free(unsigned int cpu)
>>       {},
>>  };
>>
>> +#ifdef CONFIG_ACPI
>> +static const struct acpi_device_id xgene_msi_acpi_ids[] = {
>> +     {"APMC0D0E", 0},
>> +     { },
>> +};
>> +#endif
>> +
>>  static int xgene_msi_probe(struct platform_device *pdev)
>>  {
>>       struct resource *res;
>> @@ -469,7 +487,18 @@ static int xgene_msi_probe(struct platform_device *pdev)
>>               goto error;
>>       }
>>       xgene_msi->msi_addr = res->start;
>> -     xgene_msi->node = pdev->dev.of_node;
>> +
>> +     xgene_msi->fwnode = of_node_to_fwnode(pdev->dev.of_node);
>> +     if (!xgene_msi->fwnode) {
>> +             xgene_msi->fwnode =
>> +                     irq_domain_alloc_fwnode((void *)xgene_msi->msi_addr);
>> +             if (!xgene_msi->fwnode) {
>> +                     dev_err(&pdev->dev, "Failed to create fwnode\n");
>> +                     rc = ENOMEM;
>> +                     goto error;
>> +             }
>> +     }
>> +
>>       xgene_msi->num_cpus = num_possible_cpus();
>>
>>       rc = xgene_msi_init_allocator(xgene_msi);
>> @@ -540,6 +569,7 @@ static int xgene_msi_probe(struct platform_device *pdev)
>>       .driver = {
>>               .name = "xgene-msi",
>>               .of_match_table = xgene_msi_match_table,
>> +             .acpi_match_table = ACPI_PTR(xgene_msi_acpi_ids),
>>       },
>>       .probe = xgene_msi_probe,
>>       .remove = xgene_msi_remove,
>>
>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny...

Hi Lorenzo,


On Mon, Aug 14, 2017 at 11:15 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Fri, Aug 11, 2017 at 06:02:53PM -0600, Khuong Dinh wrote:
>> This patch makes pci-xgene-msi driver ACPI-aware and provides
>> MSI capability for X-Gene v1 PCIe controllers in ACPI boot mode.
>>
>> Signed-off-by: Khuong Dinh <kdinh@apm.com>
>>         [Take over from Duc Dang]
>> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>> v3:
>>  - Input X-Gene MSI base address for irq_domain_alloc_fwnode
>>  - Add a hook to enforce X-Gene MSI be probed prior acpi_bus_scan happens
>> v2:
>>  - Verify with BIOS version 3.06.25 and 3.07.09
>> v1:
>>  - Initial version
>> ---
>>  drivers/acpi/Makefile            |    2 +-
>>  drivers/acpi/acpi_msi.c          |   74 ++++++++++++++++++++++++++++++++++++++
>>  drivers/acpi/internal.h          |    1 +
>>  drivers/acpi/scan.c              |    1 +
>>  drivers/pci/host/pci-xgene-msi.c |   36 +++++++++++++++++--
>>  5 files changed, 110 insertions(+), 4 deletions(-)
>>  create mode 100644 drivers/acpi/acpi_msi.c
>>
>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
>> index b1aacfc..c15f5cd 100644
>> --- a/drivers/acpi/Makefile
>> +++ b/drivers/acpi/Makefile
>> @@ -40,7 +40,7 @@ acpi-y                              += ec.o
>>  acpi-$(CONFIG_ACPI_DOCK)     += dock.o
>>  acpi-y                               += pci_root.o pci_link.o pci_irq.o
>>  obj-$(CONFIG_ACPI_MCFG)              += pci_mcfg.o
>> -acpi-y                               += acpi_lpss.o acpi_apd.o
>> +acpi-y                               += acpi_lpss.o acpi_apd.o acpi_msi.o
>>  acpi-y                               += acpi_platform.o
>>  acpi-y                               += acpi_pnp.o
>>  acpi-$(CONFIG_ARM_AMBA)      += acpi_amba.o
>> diff --git a/drivers/acpi/acpi_msi.c b/drivers/acpi/acpi_msi.c
>> new file mode 100644
>> index 0000000..c2f8d26
>> --- /dev/null
>> +++ b/drivers/acpi/acpi_msi.c
>> @@ -0,0 +1,74 @@
>> +/*
>> + * Enforce MSI driver loaded by PCIe controller driver
>> + *
>> + * Copyright (c) 2017, MACOM Technology Solutions Corporation
>> + * Author: Khuong Dinh <kdinh@apm.com>
>> + *
>> + * This program is free software; you can redistribute  it and/or modify it
>> + * under  the terms of  the GNU General  Public License as published by the
>> + * Free Software Foundation;  either version 2 of the  License, or (at your
>> + * option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/acpi.h>
>> +#include "internal.h"
>> +
>> +static acpi_status acpi_create_msi_device(acpi_handle handle, u32 Level,
>> +                                     void *context, void **retval)
>> +{
>> +     struct acpi_device *device = NULL;
>> +     int type = ACPI_BUS_TYPE_DEVICE;
>> +     unsigned long long sta;
>> +     acpi_status status;
>> +     int ret = 0;
>> +
>> +     acpi_bus_get_device(handle, &device);
>> +     status = acpi_bus_get_status_handle(handle, &sta);
>> +     if (ACPI_FAILURE(status))
>> +             sta = 0;
>> +
>> +     device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
>> +     if (!device)
>> +             return -ENOMEM;
>> +
>> +     acpi_init_device_object(device, handle, type, sta);
>> +     ret = acpi_device_add(device, NULL);
>> +     if (ret)
>> +             return ret;
>> +     device->parent = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
>> +     INIT_LIST_HEAD(&device->parent->physical_node_list);
>> +
>> +     acpi_device_add_finalize(device);
>> +
>> +     ret = device_attach(&device->dev);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     acpi_create_platform_device(device, NULL);
>> +     acpi_device_set_enumerated(device);
>> +
>> +     return ret;
>> +}
>
> Can't this be implemented through acpi_bus_scan(handle) ?
>
> (where handle is your MSI controller acpi_handle ?)

I had an experiment to use acpi_bus_scan(handle) to register MSI
driver, but it's not success as @handle for acpi_bus_scan is the root
of the namespace scope to scan.
The driver registration to ACPI platform happens with the following code path:
acpi_bus_scan
   acpi_bus_check_add
   acpi_bus_attach
      device_attach
      acpi_default_enumeration
         acpi_create_platform_device
         acpi_device_set_enumerated

acpi_bus_attach is recursively happened through ACPI nodes in order.


> [...]
>
>>  static int xgene_msi_probe(struct platform_device *pdev)
>>  {
>>       struct resource *res;
>> @@ -469,7 +487,18 @@ static int xgene_msi_probe(struct platform_device *pdev)
>>               goto error;
>>       }
>>       xgene_msi->msi_addr = res->start;
>> -     xgene_msi->node = pdev->dev.of_node;
>> +
>> +     xgene_msi->fwnode = of_node_to_fwnode(pdev->dev.of_node);
>> +     if (!xgene_msi->fwnode) {
>> +             xgene_msi->fwnode =
>> +                     irq_domain_alloc_fwnode((void *)xgene_msi->msi_addr);
>> +             if (!xgene_msi->fwnode) {
>> +                     dev_err(&pdev->dev, "Failed to create fwnode\n");
>> +                     rc = ENOMEM;
>> +                     goto error;
>> +             }
>> +     }
>> +
>>       xgene_msi->num_cpus = num_possible_cpus();
>>
>>       rc = xgene_msi_init_allocator(xgene_msi);
>> @@ -540,6 +569,7 @@ static int xgene_msi_probe(struct platform_device *pdev)
>>       .driver = {
>>               .name = "xgene-msi",
>>               .of_match_table = xgene_msi_match_table,
>> +             .acpi_match_table = ACPI_PTR(xgene_msi_acpi_ids),
>>       },
>>       .probe = xgene_msi_probe,
>>       .remove = xgene_msi_remove,
>
> AFAIK you still have not solved the link ordering problem, creating
> the platform device before scanning the PCI root bridge is not enough,
> because you are not guaranteed to probe the MSI driver first, there
> has to be way for registering your driver from the ACPI scan hook.

With this implementation, I added a hook to enforce the MSI driver
initialization and registration before acpi_bus_scan happens.
It will walk through the HID to get the MSI device (e.g:
acpi_get_devices). When the device is found, the callback will be
called and we will initialize MSI device by its handle, start to
attach MSI device to a driver, create ACPI platform device for MSI
APCI device node and marked it as visited.
In result, it will happen on top of acpi_bus_scan.


> Thanks,
> Lorenzo

On Mon, Aug 14, 2017 at 04:17:35PM -0700, Khuong Dinh wrote:

[...]

> >> +static acpi_status acpi_create_msi_device(acpi_handle handle, u32 Level,
> >> +                                     void *context, void **retval)
> >> +{
> >> +     struct acpi_device *device = NULL;
> >> +     int type = ACPI_BUS_TYPE_DEVICE;
> >> +     unsigned long long sta;
> >> +     acpi_status status;
> >> +     int ret = 0;
> >> +
> >> +     acpi_bus_get_device(handle, &device);
> >> +     status = acpi_bus_get_status_handle(handle, &sta);
> >> +     if (ACPI_FAILURE(status))
> >> +             sta = 0;
> >> +
> >> +     device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
> >> +     if (!device)
> >> +             return -ENOMEM;
> >> +
> >> +     acpi_init_device_object(device, handle, type, sta);
> >> +     ret = acpi_device_add(device, NULL);
> >> +     if (ret)
> >> +             return ret;
> >> +     device->parent = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
> >> +     INIT_LIST_HEAD(&device->parent->physical_node_list);
> >> +
> >> +     acpi_device_add_finalize(device);
> >> +
> >> +     ret = device_attach(&device->dev);
> >> +     if (ret < 0)
> >> +             return ret;
> >> +
> >> +     acpi_create_platform_device(device, NULL);
> >> +     acpi_device_set_enumerated(device);
> >> +
> >> +     return ret;
> >> +}
> >
> > Can't this be implemented through acpi_bus_scan(handle) ?
> >
> > (where handle is your MSI controller acpi_handle ?)
> 
> I had an experiment to use acpi_bus_scan(handle) to register MSI
> driver, but it's not success as @handle for acpi_bus_scan is the root
> of the namespace scope to scan.
> The driver registration to ACPI platform happens with the following code path:
> acpi_bus_scan
>    acpi_bus_check_add
>    acpi_bus_attach
>       device_attach
>       acpi_default_enumeration
>          acpi_create_platform_device
>          acpi_device_set_enumerated
> 
> acpi_bus_attach is recursively happened through ACPI nodes in order.

What I was saying is to call acpi_bus_scan() on the MSI controller
handle (as start_object - see acpi_walk_namespace()).

> > [...]
> >
> >>  static int xgene_msi_probe(struct platform_device *pdev)
> >>  {
> >>       struct resource *res;
> >> @@ -469,7 +487,18 @@ static int xgene_msi_probe(struct platform_device *pdev)
> >>               goto error;
> >>       }
> >>       xgene_msi->msi_addr = res->start;
> >> -     xgene_msi->node = pdev->dev.of_node;
> >> +
> >> +     xgene_msi->fwnode = of_node_to_fwnode(pdev->dev.of_node);
> >> +     if (!xgene_msi->fwnode) {
> >> +             xgene_msi->fwnode =
> >> +                     irq_domain_alloc_fwnode((void *)xgene_msi->msi_addr);
> >> +             if (!xgene_msi->fwnode) {
> >> +                     dev_err(&pdev->dev, "Failed to create fwnode\n");
> >> +                     rc = ENOMEM;
> >> +                     goto error;
> >> +             }
> >> +     }
> >> +
> >>       xgene_msi->num_cpus = num_possible_cpus();
> >>
> >>       rc = xgene_msi_init_allocator(xgene_msi);
> >> @@ -540,6 +569,7 @@ static int xgene_msi_probe(struct platform_device *pdev)
> >>       .driver = {
> >>               .name = "xgene-msi",
> >>               .of_match_table = xgene_msi_match_table,
> >> +             .acpi_match_table = ACPI_PTR(xgene_msi_acpi_ids),
> >>       },
> >>       .probe = xgene_msi_probe,
> >>       .remove = xgene_msi_remove,
> >
> > AFAIK you still have not solved the link ordering problem, creating
> > the platform device before scanning the PCI root bridge is not enough,
> > because you are not guaranteed to probe the MSI driver first, there
> > has to be way for registering your driver from the ACPI scan hook.
> 
> With this implementation, I added a hook to enforce the MSI driver
> initialization and registration before acpi_bus_scan happens.

I do not know if I am mistaken but all I see is still:

subsys_initcall(xgene_pcie_msi_init);

which is not guaranteed to be called before the PCI host bridge
is scanned. You have to have a way to _probe_ the MSI driver before
the PCI host bridge is scanned, which is different.

> It will walk through the HID to get the MSI device (e.g:
> acpi_get_devices). When the device is found, the callback will be
> called and we will initialize MSI device by its handle, start to
> attach MSI device to a driver, create ACPI platform device for MSI
> APCI device node and marked it as visited.

But if the MSI driver is still not registered the whole concept still
does not work. You are not guaranteed ordering between initcalls at
the same level.

Thanks,
Lorenzo