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

Message ID 1482384922-21507-10-git-send-email-guohanjun@huawei.com
State New
Headers show

Commit Message

Hanjun Guo Dec. 22, 2016, 5:35 a.m.
From: Hanjun Guo <hanjun.guo@linaro.org>


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 <hanjun.guo@linaro.org>

Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 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(+)

-- 
1.7.12.4

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Rafael J. Wysocki Dec. 22, 2016, 12:57 p.m. | #1
On Thu, Dec 22, 2016 at 6:35 AM, Hanjun Guo <guohanjun@huawei.com> wrote:
> From: Hanjun Guo <hanjun.guo@linaro.org>

>

> 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 <hanjun.guo@linaro.org>

> Cc: Marc Zyngier <marc.zyngier@arm.com>

> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>

> Cc: Greg KH <gregkh@linuxfoundation.org>

> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

> Cc: Thomas Gleixner <tglx@linutronix.de>

> ---

>  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 <linux/acpi.h>

> +#include <linux/acpi_iort.h>

>  #include <linux/device.h>

>  #include <linux/err.h>

>  #include <linux/kernel.h>

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

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

> +


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

>         ret = platform_device_add(pdev);

>         if (ret) {


Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki Dec. 26, 2016, 12:31 a.m. | #2
On Sat, Dec 24, 2016 at 8:34 AM, Hanjun Guo <guohanjun@huawei.com> 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 <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?

>         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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hanjun Guo Dec. 26, 2016, 1:31 a.m. | #3
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 <guohanjun@huawei.com> 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 <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.

>

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


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sinan Kaya Dec. 29, 2016, 2:44 p.m. | #4
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Xinwei Kong Dec. 30, 2016, 8:58 a.m. | #5
On 2016/12/22 13:35, Hanjun Guo wrote:
> From: Hanjun Guo <hanjun.guo@linaro.org>

>

> 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 <hanjun.guo@linaro.org>

> Cc: Marc Zyngier <marc.zyngier@arm.com>

> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>

> Cc: Greg KH <gregkh@linuxfoundation.org>

> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

> Cc: Thomas Gleixner <tglx@linutronix.de>

> ---

>   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 <linux/acpi.h>

> +#include <linux/acpi_iort.h>

>   #include <linux/device.h>

>   #include <linux/err.h>

>   #include <linux/kernel.h>

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

>   static inline void acpi_iort_init(void) { }

>   static inline bool iort_node_match(u8 type) { return false; }

> @@ -58,6 +59,8 @@ static inline void iort_set_dma_mask(struct device *dev) { }

>   static inline

>   const struct iommu_ops *iort_iommu_configure(struct device *dev)

>   { return NULL; }

> +

> +static inline void acpi_configure_pmsi_domain(struct device *dev) { }

>   #endif

>   

>   #define IORT_ACPI_DECLARE(name, table_id, fn)		\

> diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h

> index 98c2a7c..280d366fb 100644

> --- a/include/linux/platform_device.h

> +++ b/include/linux/platform_device.h

> @@ -74,6 +74,9 @@ struct platform_device_info {

>   		u64 dma_mask;

>   

>   		struct property_entry *properties;

> +

> +		/* preparation callback before the platform device is added */

> +		void (*pre_add_cb)(struct device *);

>   };

>   extern struct platform_device *platform_device_register_full(

>   		const struct platform_device_info *pdevinfo);

Tested-by:  Xinwei Kong <kong.kongxinwei@hisilicon.com>


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hanjun Guo Dec. 30, 2016, 10:40 a.m. | #6
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

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki Dec. 31, 2016, 8:45 p.m. | #7
On Fri, Dec 30, 2016 at 11:50 AM, Hanjun Guo <guohanjun@huawei.com> 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 <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.

>>

>>>>         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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hanjun Guo Jan. 2, 2017, 12:02 p.m. | #8
On 01/01/2017 04:45 AM, Rafael J. Wysocki wrote:
> On Fri, Dec 30, 2016 at 11:50 AM, Hanjun Guo <guohanjun@huawei.com> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

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 <linux/acpi.h>
+#include <linux/acpi_iort.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/kernel.h>
@@ -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
 static inline void acpi_iort_init(void) { }
 static inline bool iort_node_match(u8 type) { return false; }
@@ -58,6 +59,8 @@  static inline void iort_set_dma_mask(struct device *dev) { }
 static inline
 const struct iommu_ops *iort_iommu_configure(struct device *dev)
 { return NULL; }
+
+static inline void acpi_configure_pmsi_domain(struct device *dev) { }
 #endif
 
 #define IORT_ACPI_DECLARE(name, table_id, fn)		\
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 98c2a7c..280d366fb 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -74,6 +74,9 @@  struct platform_device_info {
 		u64 dma_mask;
 
 		struct property_entry *properties;
+
+		/* preparation callback before the platform device is added */
+		void (*pre_add_cb)(struct device *);
 };
 extern struct platform_device *platform_device_register_full(
 		const struct platform_device_info *pdevinfo);