| Message ID | 1482384922-21507-10-git-send-email-guohanjun@huawei.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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
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
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);