Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

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

52 views
Skip to first unread message

Hanjun Guo

unread,
Dec 22, 2016, 12:50:05 AM12/22/16
to
From: Hanjun Guo <hanju...@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 <hanju...@linaro.org>
Cc: Marc Zyngier <marc.z...@arm.com>
Cc: Rafael J. Wysocki <r...@rjwysocki.net>
Cc: Greg KH <gre...@linuxfoundation.org>
Cc: Lorenzo Pieralisi <lorenzo....@arm.com>
Cc: Thomas Gleixner <tg...@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);
--
1.7.12.4

Rafael J. Wysocki

unread,
Dec 22, 2016, 8:00:05 AM12/22/16
to
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 ->
-> because it looks like this might be done in acpi_platform_notify()
for platform devices.

> ret = platform_device_add(pdev);
> if (ret) {

Thanks,
Rafael

Hanjun Guo

unread,
Dec 24, 2016, 2:40:05 AM12/24/16
to
Hi Rafael,

Thank you for your comments, when I was demoing your suggestion,
I got a little bit confusions, please see my comments below.
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...
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);
+
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)?

Thanks
Hanjun

Rafael J. Wysocki

unread,
Dec 25, 2016, 7:40:05 PM12/25/16
to
On Sat, Dec 24, 2016 at 8:34 AM, Hanjun Guo <guoh...@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.
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

Hanjun Guo

unread,
Dec 25, 2016, 8:40:04 PM12/25/16
to
Hi Rafael,

Happy holidays! reply inline.
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

Sinan Kaya

unread,
Dec 29, 2016, 9:50:05 AM12/29/16
to
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.

Xinwei Kong

unread,
Dec 30, 2016, 4:50:06 AM12/30/16
to
Tested-by: Xinwei Kong <kong.ko...@hisilicon.com>

Hanjun Guo

unread,
Dec 30, 2016, 5:50:04 AM12/30/16
to
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

Hanjun Guo

unread,
Dec 30, 2016, 7:20:05 AM12/30/16
to
Hi Rafael,

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

Rafael J. Wysocki

unread,
Dec 31, 2016, 3:50:05 PM12/31/16
to
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

Hanjun Guo

unread,
Jan 2, 2017, 7:10:05 AM1/2/17
to
On 01/01/2017 04:45 AM, Rafael J. Wysocki wrote:
> On Fri, Dec 30, 2016 at 11:50 AM, Hanjun Guo <guoh...@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

Hanjun Guo

unread,
Jan 2, 2017, 8:40:05 AM1/2/17
to
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 (the time slot of setting the
msi domain also works for other cases). So simply call
acpi_configure_pmsi_domain() in acpi_platform_notify() for
platform devices will work.

Signed-off-by: Hanjun Guo <hanju...@linaro.org>
Cc: Rafael J. Wysocki <r...@rjwysocki.net>
Cc: Marc Zyngier <marc.z...@arm.com>
Cc: Lorenzo Pieralisi <lorenzo....@arm.com>
---
drivers/acpi/arm64/iort.c | 43 +++++++++++++++++++++++++++++++++++++++++++
drivers/acpi/glue.c | 6 ++++++
include/linux/acpi_iort.h | 3 +++
3 files changed, 52 insertions(+)

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/acpi/glue.c b/drivers/acpi/glue.c
index f8d6564..4a73f27 100644
--- a/drivers/acpi/glue.c
+++ b/drivers/acpi/glue.c
@@ -6,6 +6,8 @@
*
* This file is released under the GPLv2.
*/
+
+#include <linux/acpi_iort.h>
#include <linux/export.h>
#include <linux/init.h>
#include <linux/list.h>
@@ -14,6 +16,7 @@
#include <linux/rwsem.h>
#include <linux/acpi.h>
#include <linux/dma-mapping.h>
+#include <linux/platform_device.h>

#include "internal.h"

@@ -315,6 +318,9 @@ 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);
else if (adev->handler && adev->handler->bind)
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) \
--
1.9.1

Rafael J. Wysocki

unread,
Jan 2, 2017, 4:20:06 PM1/2/17
to
On Mon, Jan 2, 2017 at 2:31 PM, Hanjun Guo <hanju...@linaro.org> wrote:
> 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 (the time slot of setting the
> msi domain also works for other cases). So simply call
> acpi_configure_pmsi_domain() in acpi_platform_notify() for
> platform devices will work.
>
> Signed-off-by: Hanjun Guo <hanju...@linaro.org>
> Cc: Rafael J. Wysocki <r...@rjwysocki.net>
> Cc: Marc Zyngier <marc.z...@arm.com>
> Cc: Lorenzo Pieralisi <lorenzo....@arm.com>

ACK for the glue.c part.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majo...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
0 new messages