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

[Khuong Dinh]

Hi Lorenzo,


On Mon, Aug 14, 2017 at 11:15 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@xxxxxxx> 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@xxxxxxx>
>>         [Take over from Duc Dang]
>> Acked-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>> ---
>> 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@xxxxxxx>
>> + *
>> + * 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