Re: [PATCH v5 09/14] ACPI: platform: setup MSI domain for ACPI based platform device

2017-01-02 Thread Hanjun Guo 

On 01/01/2017 04:45 AM, Rafael J. Wysocki wrote:

On Fri, Dec 30, 2016 at 11:50 AM, Hanjun Guo  wrote:

[...]


So how about just add the code as below?


Works for me.


OK, will send out the updated patch set soon.




diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
index 11e63dd..37a8dfe 100644
--- a/drivers/acpi/glue.c
+++ b/drivers/acpi/glue.c
@@ -316,7 +316,8 @@ static int acpi_platform_notify(struct device *dev)
 if (!adev)
 goto out;

+ if (dev->bus == &platform_bus_type)
+ acpi_configure_pmsi_domain(dev);

 if (type && type->setup)
 type->setup(dev);


Thanks for your comments.

Hanjun

Re: [PATCH v5 09/14] ACPI: platform: setup MSI domain for ACPI based platform device

2016-12-31 Thread Rafael J. Wysocki 
On Fri, Dec 30, 2016 at 11:50 AM, Hanjun Guo  wrote:
> Hi Rafael,
>
> On 2016/12/26 9:31, Hanjun Guo wrote:
> [cut]
>>
>> +   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 
  #include 
  #include 
 +#include 
  #include 

  #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.
>>
 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.
>
> Just demo the code and find out it's seems to cut the feet to the 
> type->setup() code,
> because we need a match function (it's ok) and a find_companion() (we don't 
> need that
> and make the code worse because we will call the find_companion callback 
> which it not needed
> for platform devices:
>
> diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
> index 96983c9..654021d9b 100644
> --- a/drivers/acpi/acpi_platform.c
> +++ b/drivers/acpi/acpi_platform.c
> @@ -138,3 +138,31 @@ struct platform_device 
> *acpi_create_platform_device(struct acpi_device *adev,
> return pdev;
>  }
>  EXPORT_SYMBOL_GPL(acpi_create_platform_device);
> +
> +static bool platform_acpi_bus_match(struct device *dev)
> +{
> + return dev->bus == &platform_bus_type;
> +}
> +
> +static struct acpi_device *platform_acpi_bus_find_companion(struct device 
> *dev)
> +{
> + /* demo code, do nothing here */
> + return NULL;
> +}
> +
> +static void platform_acpi_setup(struct device *dev)
> +{
> + acpi_configure_pmsi_domain(dev);
> +}
> +
> +static struct acpi_bus_type acpi_platform_bus = {
> + .name = "Platform",
> + .match = platform_acpi_bus_match,
> + .find_companion = platform_acpi_bus_find_companion,
> + .setup = platform_acpi_setup,
> +};
> +
> +int acpi_platform_bus_register(void)
> +{
> + return register_acpi_bus_type(&acpi_platform_bus);
> +}
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 95855cb..0a0a639 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -1199,6 +1199,7 @@ static int __init acpi_init(void)
> }
>
> pci_mmcfg_late_init();
> + acpi_platform_bus_register();
> acpi_iort_init();
> acpi_scan_init();
> acpi_ec_init();
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 809b536..1d05f92 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -597,6 +597,8 @@ extern bool acpi_driver_match_device(struct device *dev,
>
>  struct platform_device *acpi_create_platform_device(struct acpi_device *,
> struct property_entry *);
> +int acpi_platform_bus_register(void);
> +
>  #define ACPI_PTR(_ptr) (_ptr)
>
>  static inline void acpi_device_set_enumerated(struct acpi_device *adev)
>
>
> So how about just add the code as below?

Works for me.

> diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
> index 11e63dd..37a8dfe 100644
> --- a/drivers/acpi/glue.c
> +++ b/drivers/acpi/glue.c
> @@ -316,7 +316,8 @@ static int acpi_platform_notify(struct device *dev)
> if (!adev)
> goto out;
>
> + if (dev->bus == &platform_bus_type)
> + acpi_configure_pmsi_domain(dev);
>
> if (type && type->setup)
> type->setup(dev);

Thanks,
Rafael

Re: [PATCH v5 09/14] ACPI: platform: setup MSI domain for ACPI based platform device

2016-12-30 Thread Hanjun Guo 
Hi Rafael,

On 2016/12/26 9:31, Hanjun Guo wrote:
[cut]
>
> +   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 
>>>  #include 
>>>  #include 
>>> +#include 
>>>  #include 
>>>
>>>  #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.
>
>>> 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.

Just demo the code and find out it's seems to cut the feet to the type->setup() 
code,
because we need a match function (it's ok) and a find_companion() (we don't 
need that
and make the code worse because we will call the find_companion callback which 
it not needed
for platform devices:

diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
index 96983c9..654021d9b 100644
--- a/drivers/acpi/acpi_platform.c
+++ b/drivers/acpi/acpi_platform.c
@@ -138,3 +138,31 @@ struct platform_device *acpi_create_platform_device(struct 
acpi_device *adev,
return pdev;
 }
 EXPORT_SYMBOL_GPL(acpi_create_platform_device);
+
+static bool platform_acpi_bus_match(struct device *dev)
+{
+ return dev->bus == &platform_bus_type;
+}
+
+static struct acpi_device *platform_acpi_bus_find_companion(struct device *dev)
+{
+ /* demo code, do nothing here */
+ return NULL;
+}
+
+static void platform_acpi_setup(struct device *dev)
+{
+ acpi_configure_pmsi_domain(dev);
+}
+
+static struct acpi_bus_type acpi_platform_bus = {
+ .name = "Platform",
+ .match = platform_acpi_bus_match,
+ .find_companion = platform_acpi_bus_find_companion,
+ .setup = platform_acpi_setup,
+};
+
+int acpi_platform_bus_register(void)
+{
+ return register_acpi_bus_type(&acpi_platform_bus);
+}
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 95855cb..0a0a639 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -1199,6 +1199,7 @@ static int __init acpi_init(void)
}
 
pci_mmcfg_late_init();
+ acpi_platform_bus_register();
acpi_iort_init();
acpi_scan_init();
acpi_ec_init();
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 809b536..1d05f92 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -597,6 +597,8 @@ extern bool acpi_driver_match_device(struct device *dev,
 
 struct platform_device *acpi_create_platform_device(struct acpi_device *,
struct property_entry *);
+int acpi_platform_bus_register(void);
+
 #define ACPI_PTR(_ptr) (_ptr)
 
 static inline void acpi_device_set_enumerated(struct acpi_device *adev)


So how about just add the code as below?

diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
index 11e63dd..37a8dfe 100644
--- a/drivers/acpi/glue.c
+++ b/drivers/acpi/glue.c
@@ -316,7 +316,8 @@ static int acpi_platform_notify(struct device *dev)
if (!adev)
goto out;
 
+ if (dev->bus == &platform_bus_type)
+ acpi_configure_pmsi_domain(dev);
 
if (type && type->setup)
type->setup(dev);

Thanks
Hanjun

Re: [PATCH v5 09/14] ACPI: platform: setup MSI domain for ACPI based platform device

2016-12-30 Thread Hanjun Guo 
On 2016/12/29 22:44, Sinan Kaya wrote:
> On 12/25/2016 8:31 PM, Hanjun Guo wrote:
>>> 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.
> Can you provide details on what the minor issue is with the IORT code?

It's about the mapping of NC (named component) -> SMMU -> ITS, we can
describe it as two ID mappings:
 - NC->SMMU
 - NC->ITS
And the code for now can work perfect for such id mappings, but if we
want to support chained mapping NC  -> SMMU -> ITS, we need to add
extra code which in my [PATCH v5 10/14] ACPI: ARM64: IORT: rework
iort_node_get_id() for NC->SMMU->ITS case, but I just scanned the first
id mapping for now, I think I need to scan all the id mappings (but seems
single id mappings don't need to do that, I will investigate it more).

Thanks
Hanjun

Re: [PATCH v5 09/14] ACPI: platform: setup MSI domain for ACPI based platform device

2016-12-30 Thread Xinwei Kong 

On 2016/12/22 13:35, Hanjun Guo wrote:

From: Hanjun Guo 

With the platform msi domain created, we can set up the msi domain
for a platform device when it's probed.

In order to do that, we need to get the domain that the platform
device connecting to, so the iort_get_platform_device_domain() is
introduced to retrieve the domain from iort.

After the domain is retrieved, we need a proper way to set the
domain to paltform device, as some platform devices such as an
irqchip needs the msi irqdomain to be the interrupt parent domain,
we need to get irqdomain before platform device is probed but after
the platform device is allocated, so introduce a callback (pre_add_cb)
in pdevinfo to prepare firmware related information which is needed
for device probe, then set the msi domain in that callback.

Signed-off-by: Hanjun Guo 
Cc: Marc Zyngier 
Cc: Rafael J. Wysocki 
Cc: Greg KH 
Cc: Lorenzo Pieralisi 
Cc: Thomas Gleixner 
---
  drivers/acpi/acpi_platform.c| 11 +++
  drivers/acpi/arm64/iort.c   | 43 +
  drivers/base/platform.c |  3 +++
  include/linux/acpi_iort.h   |  3 +++
  include/linux/platform_device.h |  3 +++
  5 files changed, 63 insertions(+)

diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
index b4c1a6a..5d8d61b4 100644
--- a/drivers/acpi/acpi_platform.c
+++ b/drivers/acpi/acpi_platform.c
@@ -12,6 +12,7 @@
   */
  
  #include 

+#include 
  #include 
  #include 
  #include 
@@ -48,6 +49,15 @@ static void acpi_platform_fill_resource(struct acpi_device 
*adev,
  }
  
  /**

+ * acpi_platform_pre_add_cb - callback before platform device is added, to
+ * prepare firmware related information which is needed for device probe
+ */
+static void acpi_platform_pre_add_cb(struct device *dev)
+{
+   acpi_configure_pmsi_domain(dev);
+}
+
+/**
   * 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;
  
  	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);
+
ret = platform_device_add(pdev);
if (ret) {
  err:
diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
index ef99fd52..33f5ac3 100644
--- a/include/linux/acpi_iort.h
+++ b/include/linux/acpi_iort.h
@@ -38,6 +38,7 @@
  /* IOMMU interface */
  void iort_set_dma_mask(struct device *dev);
  const struct iommu_ops *iort_iommu_configure(struct device *dev);

Re: [PATCH v5 09/14] ACPI: platform: setup MSI domain for ACPI based platform device

2016-12-29 Thread Sinan Kaya 
On 12/25/2016 8:31 PM, Hanjun Guo wrote:
>> 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.

Can you provide details on what the minor issue is with the IORT code?

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

Re: [PATCH v5 09/14] ACPI: platform: setup MSI domain for ACPI based platform device

2016-12-25 Thread Hanjun Guo 
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  wrote:
>> 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]
>
 +
 +/**
   * 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.
>
>>> And I'm not sure why the new callback is necessary ->
>> I was demoing your suggestion but...
>>
 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 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>
>>  #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.

>
>> 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

Re: [PATCH v5 09/14] ACPI: platform: setup MSI domain for ACPI based platform device

2016-12-25 Thread Rafael J. Wysocki 
On Sat, Dec 24, 2016 at 8:34 AM, Hanjun Guo  wrote:
> 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]

>>> +
>>> +/**
>>>   * 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.

>>
>> And I'm not sure why the new callback is necessary ->
>
> I was demoing your suggestion but...
>
>>
>>> 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 
>  #include 
>  #include 
> +#include 
>  #include 
>
>  #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?

> 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. :-)

Thanks,
Rafael

Re: [PATCH v5 09/14] ACPI: platform: setup MSI domain for ACPI based platform device

2016-12-23 Thread Hanjun Guo 
Hi Rafael,

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

On 2016/12/22 20:57, Rafael J. Wysocki wrote:
> On Thu, Dec 22, 2016 at 6:35 AM, Hanjun Guo  wrote:
>> From: Hanjun Guo 
>>
>> With the platform msi domain created, we can set up the msi domain
>> for a platform device when it's probed.
>>
>> In order to do that, we need to get the domain that the platform
>> device connecting to, so the iort_get_platform_device_domain() is
>> introduced to retrieve the domain from iort.
>>
>> After the domain is retrieved, we need a proper way to set the
>> domain to paltform device, as some platform devices such as an
>> irqchip needs the msi irqdomain to be the interrupt parent domain,
>> we need to get irqdomain before platform device is probed but after
>> the platform device is allocated, so introduce a callback (pre_add_cb)
>> in pdevinfo to prepare firmware related information which is needed
>> for device probe, then set the msi domain in that callback.
>>
>> Signed-off-by: Hanjun Guo 
>> Cc: Marc Zyngier 
>> Cc: Rafael J. Wysocki 
>> Cc: Greg KH 
>> Cc: Lorenzo Pieralisi 
>> Cc: Thomas Gleixner 
>> ---
>>  drivers/acpi/acpi_platform.c| 11 +++
>>  drivers/acpi/arm64/iort.c   | 43 
>> +
>>  drivers/base/platform.c |  3 +++
>>  include/linux/acpi_iort.h   |  3 +++
>>  include/linux/platform_device.h |  3 +++
>>  5 files changed, 63 insertions(+)
>>
>> diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
>> index b4c1a6a..5d8d61b4 100644
>> --- a/drivers/acpi/acpi_platform.c
>> +++ b/drivers/acpi/acpi_platform.c
>> @@ -12,6 +12,7 @@
>>   */
>>
>>  #include 
>> +#include 
>>  #include 
>>  #include 
>>  #include 
>> @@ -48,6 +49,15 @@ static void acpi_platform_fill_resource(struct 
>> acpi_device *adev,
>>  }
>>
>>  /**
>> + * acpi_platform_pre_add_cb - callback before platform device is added, to
>> + * prepare firmware related information which is needed for device probe
>> + */
>> +static void acpi_platform_pre_add_cb(struct device *dev)
>> +{
>> +   acpi_configure_pmsi_domain(dev);
>> +}
>> +
>> +/**
>>   * 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.

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

I was demoing your suggestion but...

>
>> 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);
>> +

Re: [PATCH v5 09/14] ACPI: platform: setup MSI domain for ACPI based platform device

2016-12-22 Thread Rafael J. Wysocki 
On Thu, Dec 22, 2016 at 6:35 AM, Hanjun Guo  wrote:
> From: Hanjun Guo 
>
> With the platform msi domain created, we can set up the msi domain
> for a platform device when it's probed.
>
> In order to do that, we need to get the domain that the platform
> device connecting to, so the iort_get_platform_device_domain() is
> introduced to retrieve the domain from iort.
>
> After the domain is retrieved, we need a proper way to set the
> domain to paltform device, as some platform devices such as an
> irqchip needs the msi irqdomain to be the interrupt parent domain,
> we need to get irqdomain before platform device is probed but after
> the platform device is allocated, so introduce a callback (pre_add_cb)
> in pdevinfo to prepare firmware related information which is needed
> for device probe, then set the msi domain in that callback.
>
> Signed-off-by: Hanjun Guo 
> Cc: Marc Zyngier 
> Cc: Rafael J. Wysocki 
> Cc: Greg KH 
> Cc: Lorenzo Pieralisi 
> Cc: Thomas Gleixner 
> ---
>  drivers/acpi/acpi_platform.c| 11 +++
>  drivers/acpi/arm64/iort.c   | 43 
> +
>  drivers/base/platform.c |  3 +++
>  include/linux/acpi_iort.h   |  3 +++
>  include/linux/platform_device.h |  3 +++
>  5 files changed, 63 insertions(+)
>
> diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
> index b4c1a6a..5d8d61b4 100644
> --- a/drivers/acpi/acpi_platform.c
> +++ b/drivers/acpi/acpi_platform.c
> @@ -12,6 +12,7 @@
>   */
>
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -48,6 +49,15 @@ static void acpi_platform_fill_resource(struct acpi_device 
> *adev,
>  }
>
>  /**
> + * acpi_platform_pre_add_cb - callback before platform device is added, to
> + * prepare firmware related information which is needed for device probe
> + */
> +static void acpi_platform_pre_add_cb(struct device *dev)
> +{
> +   acpi_configure_pmsi_domain(dev);
> +}
> +
> +/**
>   * 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.

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

>
> 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)
> +

[PATCH v5 09/14] ACPI: platform: setup MSI domain for ACPI based platform device

2016-12-21 Thread Hanjun Guo 
From: Hanjun Guo 

With the platform msi domain created, we can set up the msi domain
for a platform device when it's probed.

In order to do that, we need to get the domain that the platform
device connecting to, so the iort_get_platform_device_domain() is
introduced to retrieve the domain from iort.

After the domain is retrieved, we need a proper way to set the
domain to paltform device, as some platform devices such as an
irqchip needs the msi irqdomain to be the interrupt parent domain,
we need to get irqdomain before platform device is probed but after
the platform device is allocated, so introduce a callback (pre_add_cb)
in pdevinfo to prepare firmware related information which is needed
for device probe, then set the msi domain in that callback.

Signed-off-by: Hanjun Guo 
Cc: Marc Zyngier 
Cc: Rafael J. Wysocki 
Cc: Greg KH 
Cc: Lorenzo Pieralisi 
Cc: Thomas Gleixner 
---
 drivers/acpi/acpi_platform.c| 11 +++
 drivers/acpi/arm64/iort.c   | 43 +
 drivers/base/platform.c |  3 +++
 include/linux/acpi_iort.h   |  3 +++
 include/linux/platform_device.h |  3 +++
 5 files changed, 63 insertions(+)

diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
index b4c1a6a..5d8d61b4 100644
--- a/drivers/acpi/acpi_platform.c
+++ b/drivers/acpi/acpi_platform.c
@@ -12,6 +12,7 @@
  */
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -48,6 +49,15 @@ static void acpi_platform_fill_resource(struct acpi_device 
*adev,
 }
 
 /**
+ * acpi_platform_pre_add_cb - callback before platform device is added, to
+ * prepare firmware related information which is needed for device probe
+ */
+static void acpi_platform_pre_add_cb(struct device *dev)
+{
+   acpi_configure_pmsi_domain(dev);
+}
+
+/**
  * 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;
 
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);
+
ret = platform_device_add(pdev);
if (ret) {
 err:
diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
index ef99fd52..33f5ac3 100644
--- a/include/linux/acpi_iort.h
+++ b/include/linux/acpi_iort.h
@@ -38,6 +38,7 @@
 /* IOMMU interface */
 void iort_set_dma_mask(struct device *dev);
 const struct iommu_ops *iort_iommu_configure(struct device *dev);
+void acpi_configure_pmsi_domain(struct device *dev);
 #else
 s