Hi Rafael,

Happy holidays! reply inline.

On 2016/12/26 8:31, Rafael J. Wysocki wrote:

On Sat, Dec 24, 2016 at 8:34 AM, Hanjun Guo [off-list ref] wrote:

quoted

Hi Rafael,

Thank you for your comments, when I was demoing your suggestion,
I got a little bit confusions, please see my comments below.

[cut]

quoted

quoted

quoted

+
+/**
  * acpi_create_platform_device - Create platform device for ACPI device node
  * @adev: ACPI device node to create a platform device for.
  * @properties: Optional collection of build-in properties.

@@ -109,6 +119,7 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev,
        pdevinfo.num_res = count;
        pdevinfo.fwnode = acpi_fwnode_handle(adev);
        pdevinfo.properties = properties;
+       pdevinfo.pre_add_cb = acpi_platform_pre_add_cb;

Why don't you point that directly to acpi_configure_pmsi_domain()?  It
doesn't look like the wrapper is necessary at all.

I was thinking that we can add something more in the future
if we need to extend the function of the callback, I can just
use acpi_configure_pmsi_domain() here.

So you can add the wrapper in the future just fine as well.  At this
point it is just redundant.

quoted

quoted

And I'm not sure why the new callback is necessary ->

I was demoing your suggestion but...

quoted

quoted

        if (acpi_dma_supported(adev))
                pdevinfo.dma_mask = DMA_BIT_MASK(32);

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index bc68d93..6b72fcb 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c

@@ -527,6 +527,49 @@ struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id)
        return irq_find_matching_fwnode(handle, DOMAIN_BUS_PCI_MSI);
 }

+/**
+ * iort_get_platform_device_domain() - Find MSI domain related to a
+ * platform device
+ * @dev: the dev pointer associated with the platform device
+ *
+ * Returns: the MSI domain for this device, NULL otherwise
+ */
+static struct irq_domain *iort_get_platform_device_domain(struct device *dev)
+{
+       struct acpi_iort_node *node, *msi_parent;
+       struct fwnode_handle *iort_fwnode;
+       struct acpi_iort_its_group *its;
+
+       /* find its associated iort node */
+       node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
+                             iort_match_node_callback, dev);
+       if (!node)
+               return NULL;
+
+       /* then find its msi parent node */
+       msi_parent = iort_node_get_id(node, NULL, IORT_MSI_TYPE, 0);
+       if (!msi_parent)
+               return NULL;
+
+       /* Move to ITS specific data */
+       its = (struct acpi_iort_its_group *)msi_parent->node_data;
+
+       iort_fwnode = iort_find_domain_token(its->identifiers[0]);
+       if (!iort_fwnode)
+               return NULL;
+
+       return irq_find_matching_fwnode(iort_fwnode, DOMAIN_BUS_PLATFORM_MSI);
+}
+
+void acpi_configure_pmsi_domain(struct device *dev)
+{
+       struct irq_domain *msi_domain;
+
+       msi_domain = iort_get_platform_device_domain(dev);
+       if (msi_domain)
+               dev_set_msi_domain(dev, msi_domain);
+}
+
 static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
 {
        u32 *rid = data;

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index c4af003..3e68f31 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c

@@ -537,6 +537,9 @@ struct platform_device *platform_device_register_full(
                        goto err;
        }

+       if (pdevinfo->pre_add_cb)
+               pdevinfo->pre_add_cb(&pdev->dev);
+

-> because it looks like this might be done in acpi_platform_notify()
for platform devices.

It works and I just simply add the code below:

diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
index f8d6564..e0cd649 100644
--- a/drivers/acpi/glue.c
+++ b/drivers/acpi/glue.c

@@ -13,6 +13,7 @@
 #include <linux/slab.h>
 #include <linux/rwsem.h>
 #include <linux/acpi.h>
+#include <linux/acpi_iort.h>
 #include <linux/dma-mapping.h>

 #include "internal.h"

@@ -315,6 +316,8 @@ static int acpi_platform_notify(struct device *dev)
        if (!adev)
                goto out;

+ acpi_configure_pmsi_domain(dev);
+

But that should apply to platform devices only I suppose?

Yes, it's only for the platform device.

quoted

        if (type && type->setup)
                type->setup(dev);
        else if (adev->handler && adev->handler->bind)

Do you suggesting to configure the msi domain in this way?
or add the function in the type->setup() callback (which needs
to introduce a new acpi bus type)?

A type->setup() would be somewhat cleaner I think, but then it's more
code.  Whichever works better I guess. :-)

Agree, I will demo the type->setup() way and send out the patch for review,
also I find one minor issue for the IORT code, will update that also for next
version.

Thanks
Hanjun