IOMMU Probe deferral support
Subsystems (6)
cover IOMMU Probe deferral support
This is on top initial post from Laurent Pinchart[1]. This is a try to see if the iommu ops configuration for the devices can be called at a generic place so that it works for all busses. The dma_configure_ops for a device is now called during the device_attach callback just before the probe of the bus/driver is called. Similarly dma_deconfigure is called during device_detach. of_configure_dma_masks is still called for each of the busses separately, but that can be combined with calling dma_configure_ops if its safe to have masks not set till probe time.
Also based on comments [2] from last post [3], add_device callback is called at the point when the ops for that iommu is found.
Have tested this with arm-smmu on platform bus and yet to test for other busses (pci, amba), but those busses go through the same path as well.
[1] http://lists.linuxfoundation.org/pipermail/iommu/2015-May/013016.html
[2] http://www.spinics.net/lists/arm-kernel/msg499962.html,
[3] http://www.spinics.net/lists/arm-kernel/msg506072.html
Laurent Pinchart (5):
arm: dma-mapping: Don't override dma_ops in arch_setup_dma_ops()
of: dma: Move range size workaround to of_dma_get_range()
of: dma: Make of_dma_deconfigure() public
of: dma: Split of_configure_dma() into mask and ops configuration
iommu: of: Handle IOMMU lookup failure with deferred probing or error
Sricharan R (3):
drivers: platform: Configure dma operations at probe time
drivers: platform: Remove call to of_dma_(con/decon)figure_ops
drivers: iommu: arm-smmu: Set iommu_ops in probe
arch/arm/mm/dma-mapping.c | 9 ++++++ drivers/base/dd.c | 11 +++++++ drivers/base/dma-mapping.c | 11 +++++++ drivers/iommu/arm-smmu.c | 17 ++-------- drivers/iommu/of_iommu.c | 21 ++++++++++--- drivers/of/address.c | 20 ++++++++++-- drivers/of/device.c | 77 ++++++++++++++++++++++++++++----------------- drivers/of/platform.c | 11 ++----- drivers/pci/probe.c | 3 +- include/linux/dma-mapping.h | 3 ++ include/linux/of_device.h | 14 +++++++-- 11 files changed, 136 insertions(+), 61 deletions(-)
Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH 1/8 arm: dma-mapping: Don't override dma_ops in arch_setup_dma_ops()
From: Laurent Pinchart <[email protected]>
The arch_setup_dma_ops() function is in charge of setting dma_ops with a call to set_dma_ops(). set_dma_ops() is also called from
- highbank and mvebu bus notifiers
- dmabounce (to be replaced with swiotlb)
- arm_iommu_attach_device
(arm_iommu_attach_device is itself called from IOMMU and bus master device drivers)
To allow the arch_setup_dma_ops() call to be moved from device add time to device probe time we must ensure that dma_ops already setup by any of the above callers will not be overridden.
Aftering replacing dmabounce with swiotlb, converting IOMMU drivers to of_xlate and taking care of highbank and mvebu, the workaround should be removed.
--- arch/arm/mm/dma-mapping.c | 9 +++++++++ 1 file changed, 9 insertions(+)
arch/arm/mm/dma-mapping.c +9 -0
@@ -2259,6 +2259,15 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,struct dma_map_ops *dma_ops; dev->archdata.dma_coherent = coherent; + +/* +* Don't override the dma_ops if they have already been set. Ideally +* this should be the only location where dma_ops are set, remove this +* check when all other callers of set_dma_ops will have disappeared. +*/ +if (dev->archdata.dma_ops) +return ; +if (arm_setup_iommu_dma_ops (dev, dma_base, size, iommu)) dma_ops =arm_get_iommu_dma_map_ops (coherent);else
Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Marek Szyprowski
maintainerPATCH 1/8 arm: dma-mapping: Don't override dma_ops in arch_setup_dma_ops()
Hi Sricharan and Laurent,
Quoted 35 lines · Sricharan R
From: Laurent Pinchart <[email protected]>
The
arch_setup_dma_ops()function is in charge of settingdma_opswith a call toset_dma_ops().set_dma_ops()is also called from- highbank and mvebu bus notifiers
- dmabounce (to be replaced with swiotlb)
-arm_iommu_attach_device(
arm_iommu_attach_deviceis itself called from IOMMU and bus master device drivers)To allow the
arch_setup_dma_ops()call to be moved from device add time to device probe time we must ensure thatdma_opsalready setup by any of the above callers will not be overridden.Aftering replacing dmabounce with swiotlb, converting IOMMU drivers to
of_xlateand taking care of highbank and mvebu, the workaround should be removed.Signed-off-by Laurent Pinchart <[email protected]>--- arch/arm/mm/dma-mapping.c | 9 +++++++++ 1 file changed, 9 insertions(+)arch/arm/mm/dma-mapping.c +2
-0@@ -2259,6 +2259,15 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,struct dma_map_ops *dma_ops; dev->archdata.dma_coherent = coherent; + +/*
arch/arm/mm/dma-mapping.c void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, · line 2273
Quoted 10 lines
arch/arm/mm/dma-mapping.c +7
-0+* Don't override the dma_ops if they have already been set. Ideally +* this should be the only location where dma_ops are set, remove this +* check when all other callers of set_dma_ops will have disappeared. +*/ +if (dev->archdata.dma_ops) +return ; +if (arm_setup_iommu_dma_ops (dev, dma_base, size, iommu)) dma_ops =arm_get_iommu_dma_map_ops (coherent);else
Please add also set_dma_ops(dev, NULL) to arm_teardown_iommu_dma_ops() function
to this patch, because otherwise dma_ops won't be configured properly if deferred
probe of the given device happens: iommu dma ops will be set for the first probe
try, then iommu structures will be teared down after EPROBEDEFER error, and then
before next probe ties dma_ops will be already set, but no iommu structures are
available.
For a convenience, this a fixup patch:
https://git.linaro.org/people/marek.szyprowski/linux-srpol.git/commitdiff/55adefd43cee9d4beb15cb1bbd805c5059b56b4f
Best regards
Signature · 2 lines
Marek Szyprowski, PhD Samsung R&D Institute Poland
PATCH 1/8 arm: dma-mapping: Don't override dma_ops in arch_setup_dma_ops()
Hi Marek,
Quoted 62 lines
Hi Sricharan and Laurent,
Quoted 35 lines · Sricharan R
From: Laurent Pinchart <[email protected]>
The
arch_setup_dma_ops()function is in charge of settingdma_opswith a call toset_dma_ops().set_dma_ops()is also called from- highbank and mvebu bus notifiers
- dmabounce (to be replaced with swiotlb)
-arm_iommu_attach_device(
arm_iommu_attach_deviceis itself called from IOMMU and bus master device drivers)To allow the
arch_setup_dma_ops()call to be moved from device add time to device probe time we must ensure thatdma_opsalready setup by any of the above callers will not be overridden.Aftering replacing dmabounce with swiotlb, converting IOMMU drivers to
of_xlateand taking care of highbank and mvebu, the workaround should be removed.Signed-off-by Laurent Pinchart <[email protected]>--- arch/arm/mm/dma-mapping.c | 9 +++++++++ 1 file changed, 9 insertions(+)arch/arm/mm/dma-mapping.c +2
-0@@ -2259,6 +2259,15 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,struct dma_map_ops *dma_ops; dev->archdata.dma_coherent = coherent; + +/* arch/arm/mm/dma-mapping.c void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, · line 2273
Quoted 10 lines
arch/arm/mm/dma-mapping.c +7
-0+* Don't override the dma_ops if they have already been set. Ideally +* this should be the only location where dma_ops are set, remove this +* check when all other callers of set_dma_ops will have disappeared. +*/ +if (dev->archdata.dma_ops) +return ; +if (arm_setup_iommu_dma_ops (dev, dma_base, size, iommu)) dma_ops =arm_get_iommu_dma_map_ops (coherent);else Please add also
set_dma_ops(dev, NULL)toarm_teardown_iommu_dma_ops()function
to this patch, because otherwisedma_opswon't be configured properly if deferred
probe of the given device happens: iommu dma ops will be set for the first probe
try, then iommu structures will be teared down after EPROBEDEFER error, and then
before next probe tiesdma_opswill be already set, but no iommu structures are
available.
Quoted 3 lines
For a convenience, this a fixup patch:
https://git.linaro.org/people/marek.szyprowski/linux-srpol.git/commitdiff/55adefd43cee9d4beb15cb1bbd805c5059b56b4f
Right, this and will also add the remove_device during deconfigure as well.
Regards,
Sricharan
PATCH 2/8 of: dma: Move range size workaround to of_dma_get_range()
In reply to Sricharan RFrom: Laurent Pinchart <[email protected]>
Invalid dma-ranges values should be worked around when retrieving the DMA range in of_dma_get_range(), not by all callers of the function. This isn't much of a problem now that we have a single caller, but that situation will change when moving DMA configuration to device probe time.
--- drivers/of/address.c | 20 ++++++++++++++++++-- drivers/of/device.c | 15 --------------- 2 files changed, 18 insertions(+), 17 deletions(-)
drivers/of/address.c +18 -2
@@ -817,8 +817,8 @@ EXPORT_SYMBOL(of_io_request_and_map);* CPU addr (phys_addr_t) : pna cells * size : nsize cells * -* It returns -ENODEV if "dma-ranges" property was not found -* for this device in DT. +* Return 0 on success, -ENODEV if the "dma-ranges" property was not found for +* this device in DT, or -EINVAL if the CPU address or size is invalid. */ int of_dma_get_range (struct device_node *np,u64 *dma_addr,u64 *paddr,u64 *size) {
@@ -879,6 +879,22 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *siz *dma_addr = dmaaddr; *size =of_read_number (ranges + naddr + pna, nsize); +/* +* DT nodes sometimes incorrectly set the size as a mask. Work around +* those incorrect DT by computing the size as mask + 1. +*/ +if (*size &1 ) { +pr_warn ("%s: size 0x%llx for dma-range in node(%s) set as mask\n" , + __func__, *size, np->full_name); + *size = *size +1 ; + } + +if (!*size) { +pr_err ("%s: invalid size zero for dma-range in node(%s)\n" , + __func__, np->full_name); + ret = -EINVAL ; + goto out; + }pr_debug ("dma_addr(%llx) cpu_addr(%llx) size(%llx)\n" , *dma_addr, *paddr, *size);
drivers/of/device.c +0 -15
@@ -110,21 +110,6 @@ void of_dma_configure(struct device *dev, struct device_node *np) size = dev->coherent_dma_mask +1 ; }else { offset =PFN_DOWN (paddr - dma_addr); - -/* -* Add a work around to treat the size as mask + 1 in case -* it is defined in DT as a mask. -*/ -if (size &1 ) { -dev_warn (dev,"Invalid size 0x%llx for dma-range\n" , - size); - size = size +1 ; - } - -if (!size) { -dev_err (dev,"Adjusted size 0x%llx invalid\n" , size); -return ; - }dev_dbg (dev,"dma_pfn_offset(%#08lx)\n" , offset); }
Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH 3/8 of: dma: Make of_dma_deconfigure() public
In reply to Sricharan RFrom: Laurent Pinchart <[email protected]>
As part of moving DMA initializing to probe time the of_dma_deconfigure() function will need to be called from different source files. Make it public and move it to drivers/of/device.c where the of_dma_configure() function is.
--- drivers/of/device.c | 12 ++++++++++++ drivers/of/platform.c | 5 ----- include/linux/of_device.h | 3 +++ 3 files changed, 15 insertions(+), 5 deletions(-)
drivers/of/device.c +12 -0
@@ -136,6 +136,18 @@ void of_dma_configure(struct device *dev, struct device_node *np) }EXPORT_SYMBOL_GPL (of_dma_configure); +/** +* of_dma_deconfigure - Clean up DMA configuration +* @dev: Device for which to clean up DMA configuration +* +* Clean up all configuration performed by of_dma_configure_ops() and free all +* resources that have been allocated. +*/ +void of_dma_deconfigure (struct device *dev) +{ +arch_teardown_dma_ops (dev); +} +int of_device_register (struct platform_device *pdev) {device_initialize (&pdev->dev);
drivers/of/platform.c +0 -5
@@ -151,11 +151,6 @@ struct platform_device *of_device_alloc(struct device_node *np, }EXPORT_SYMBOL (of_device_alloc); -static void of_dma_deconfigure (struct device *dev) -{ -arch_teardown_dma_ops (dev); -} -/** * of_platform_device_create_pdata - Alloc, initialize and register an of_device * @np: pointer to node to create device for
include/linux/of_device.h +3 -0
@@ -56,6 +56,7 @@ static inline struct device_node *of_cpu_device_node_get(int cpu) }void of_dma_configure (struct device *dev,struct device_node *np); +void of_dma_deconfigure (struct device *dev);#else /* CONFIG_OF */ static inline int of_driver_match_device(struct device *dev,
@@ -100,6 +101,8 @@ static inline struct device_node *of_cpu_device_node_get(int cpu) }static inline void of_dma_configure (struct device *dev,struct device_node *np) {} +static inline void of_dma_deconfigure (struct device *dev) +{}#endif /* CONFIG_OF */ #endif /* _LINUX_OF_DEVICE_H */
Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH 4/8 of: dma: Split of_configure_dma() into mask and ops configuration
In reply to Sricharan RFrom: Laurent Pinchart <[email protected]>
The of_configure_dma() function configures both the DMA masks and ops. Moving DMA ops configuration to probe time would thus also delay configuration of the DMA masks, which might not be safe. To avoid issues split the configuration in two to allow keeping masks configuration at device add time and move ops configuration to device probe time.
--- drivers/of/device.c | 48 ++++++++++++++++++++++++++++++++++------------- drivers/of/platform.c | 6 ++++-- drivers/pci/probe.c | 3 ++- include/linux/of_device.h | 11 +++++++++-- 4 files changed, 50 insertions(+), 18 deletions(-)
drivers/of/device.c +35 -13
@@ -71,7 +71,7 @@ int of_device_add(struct platform_device *ofdev) }/** -* of_dma_configure - Setup DMA configuration +* of_dma_configure - Setup DMA masks and offset * @dev: Device to apply DMA configuration * @np: Pointer to OF node having DMA configuration *
@@ -82,13 +82,11 @@ int of_device_add(struct platform_device *ofdev)* can use a platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE events * to fix up DMA configuration. */ -void of_dma_configure (struct device *dev,struct device_node *np) +void of_dma_configure_masks (struct device *dev,struct device_node *np) { -u64 dma_addr, paddr, size; -int ret; -bool coherent; +u64 dma_addr, paddr, size, range_mask;unsigned long offset; -const struct iommu_ops *iommu; +int ret;/* * Set default coherent_dma_mask to 32 bit. Drivers are expected to
@@ -106,9 +104,10 @@ void of_dma_configure(struct device *dev, struct device_node *np) ret =of_dma_get_range (np, &dma_addr, &paddr, &size);if (ret <0 ) { - dma_addr = offset =0 ; - size = dev->coherent_dma_mask +1 ; + range_mask = dev->coherent_dma_mask +1 ; + offset =0 ; }else { + range_mask =DMA_BIT_MASK (ilog2 (dma_addr + size)); offset =PFN_DOWN (paddr - dma_addr);dev_dbg (dev,"dma_pfn_offset(%#08lx)\n" , offset); }
@@ -119,10 +118,31 @@ void of_dma_configure(struct device *dev, struct device_node *np)* Limit coherent and dma mask based on size and default mask * set by the driver. */ - dev->coherent_dma_mask =min (dev->coherent_dma_mask, -DMA_BIT_MASK (ilog2 (dma_addr + size))); - *dev->dma_mask =min ((*dev->dma_mask), -DMA_BIT_MASK (ilog2 (dma_addr + size))); + dev->coherent_dma_mask =min (dev->coherent_dma_mask, range_mask); + *dev->dma_mask =min ((*dev->dma_mask), range_mask); +} +EXPORT_SYMBOL_GPL (of_dma_configure_masks); + +/** +* of_dma_configure_ops - Setup DMA operations +* @dev: Device to apply DMA configuration +* @np: Pointer to OF node having DMA configuration +* +* Try to get devices's DMA configuration from DT and update it +* accordingly. +*/ +int of_dma_configure_ops (struct device *dev,struct device_node *np) +{ +u64 dma_addr, paddr, size; +const struct iommu_ops *iommu; +bool coherent; +int ret; + + ret =of_dma_get_range (np, &dma_addr, &paddr, &size); +if (ret <0 ) { + dma_addr =0 ; + size = dev->coherent_dma_mask +1 ; + } coherent =of_dma_is_coherent (np); dev_dbg(dev, "device is%sdma coherent\n",
@@ -133,8 +153,10 @@ void of_dma_configure(struct device *dev, struct device_node *np) iommu ? " " : " not ");arch_setup_dma_ops (dev, dma_addr, size, iommu, coherent); + +return 0 ; } -EXPORT_SYMBOL_GPL (of_dma_configure); +EXPORT_SYMBOL_GPL (of_dma_configure_ops);/** * of_dma_deconfigure - Clean up DMA configuration
drivers/of/platform.c +4 -2
@@ -179,7 +179,8 @@ static struct platform_device *of_platform_device_create_pdata( dev->dev.bus = &platform_bus_type; dev->dev.platform_data = platform_data; -of_dma_configure (&dev->dev, dev->dev.of_node); +of_dma_configure_masks (&dev->dev, dev->dev.of_node); +of_dma_configure_ops (&dev->dev, dev->dev.of_node);of_msi_configure (&dev->dev, dev->dev.of_node);if (of_device_add (dev) !=0 ) {
@@ -243,7 +244,8 @@ static struct amba_device *of_amba_device_create(struct device_node *node,dev_set_name (&dev->dev,"%s" , bus_id); else of_device_make_bus_id(&dev->dev); -of_dma_configure (&dev->dev, dev->dev.of_node); +of_dma_configure_masks (&dev->dev, dev->dev.of_node); +of_dma_configure_ops (&dev->dev, dev->dev.of_node);/* Allow the HW Peripheral ID to be overridden */ prop =of_get_property (node,"arm,primecell-periphid" ,NULL );
drivers/pci/probe.c +2 -1
@@ -1717,7 +1717,8 @@ static void pci_dma_configure(struct pci_dev *dev)if (IS_ENABLED (CONFIG_OF ) && bridge->parent && bridge->parent->of_node) { -of_dma_configure (&dev->dev, bridge->parent->of_node); +of_dma_configure_masks (&dev->dev, bridge->parent->of_node); +of_dma_configure_ops (&dev->dev, bridge->parent->of_node); }else if (has_acpi_companion (bridge)) {struct acpi_device *adev =to_acpi_device_node (bridge->fwnode);enum dev_dma_attr attr =acpi_get_dma_attr (adev);
include/linux/of_device.h +9 -2
@@ -55,7 +55,8 @@ static inline struct device_node *of_cpu_device_node_get(int cpu) return of_node_get(cpu_dev->of_node); } -void of_dma_configure (struct device *dev,struct device_node *np); +void of_dma_configure_masks (struct device *dev,struct device_node *np); +int of_dma_configure_ops (struct device *dev,struct device_node *np);void of_dma_deconfigure (struct device *dev);#else /* CONFIG_OF */
@@ -99,8 +100,14 @@ static inline struct device_node *of_cpu_device_node_get(int cpu) {return NULL ; } -static inline void of_dma_configure (struct device *dev,struct device_node *np) +static inline void of_dma_configure_masks (struct device *dev, +struct device_node *np) {} +static inline int of_dma_configure_ops (struct device *dev, +struct device_node *np) +{ +return 0 ; +}static inline void of_dma_deconfigure (struct device *dev) {}#endif /* CONFIG_OF */
Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH 4/8 of: dma: Split of_configure_dma() into mask and ops configuration
Hi,
Quoted 5 lines · Sricharan R
From: Laurent Pinchart <[email protected]>
The
of_configure_dma()function configures both the DMA masks and ops. Moving DMA ops configuration to probe time would thus also delay configuration of the DMA masks, which might not be safe. To avoid issues
Do we know any example cases when it might be unsafe? I think we kind of rely on the fact that DMA mapping (and so DMA masks as well) is not used before probing the device anyway, because we let the IOMMU attachment happen at probe time, which essentially makes any earlier attempts to use DMA mapping on such device incorrect.
Best regards,
Tomasz
PATCH 4/8 of: dma: Split of_configure_dma() into mask and ops configuration
Hi Tomasz,
Quoted 7 lines · On Friday 12 Aug 2016 16:31:47 Tomasz Figa wrote:
Quoted 5 lines · Sricharan R
From: Laurent Pinchart <[email protected]>
The
of_configure_dma()function configures both the DMA masks and ops. Moving DMA ops configuration to probe time would thus also delay configuration of the DMA masks, which might not be safe. To avoid issues
Quoted 5 lines
Do we know any example cases when it might be unsafe? I think we kind of rely on the fact that DMA mapping (and so DMA masks as well) is not used before probing the device anyway, because we let the IOMMU attachment happen at probe time, which essentially makes any earlier attempts to use DMA mapping on such device incorrect.
I don't know of any such situation, but (if I remember correctly) when I discussed the IOMMU rework with Arnd Bergman and Will Deacon there was a concern that someone, somewhere was relying on the mask being set early.
I personally would like to drop this patch, but it might be difficult to ensure this wouldn't cause a regression. There should certainly be no DMA mapping created before IOMMU attachment, but the DMA mask could possibly be used somewhere else.
Signature · 3 lines
Regards, Laurent Pinchart
PATCH 4/8 of: dma: Split of_configure_dma() into mask and ops configuration
Hi,
Quoted 10 lines
Quoted 9 lines
Quoted 3 lines
The
of_configure_dma()function configures both the DMA masks and ops. Moving DMA ops configuration to probe time would thus also delay configuration of the DMA masks, which might not be safe. To avoid issuesDo we know any example cases when it might be unsafe? I think we kind of rely on the fact that DMA mapping (and so DMA masks as well) is not used before probing the device anyway, because we let the IOMMU attachment happen at probe time, which essentially makes any earlier attempts to use DMA mapping on such device incorrect.
Quoted 9 lines
I don't know of any such situation, but (if I remember correctly) when I discussed the IOMMU rework with Arnd Bergman and Will Deacon there was a concern that someone, somewhere was relying on the mask being set early.
I personally would like to drop this patch, but it might be difficult to ensure this wouldn't cause a regression. There should certainly be no DMA mapping created before IOMMU attachment, but the DMA mask could possibly be used somewhere else.
Infact i also wanted this to be dropped and to have masks also done along with ops, but thought of getting some feedback if its really needed before probe anywhere.
Regards,
Sricharan
PATCH 4/8 of: dma: Split of_configure_dma() into mask and ops configuration
In reply to Sricharan RQuoted 7 lines · Sricharan R
From: Laurent Pinchart <[email protected]>
The
of_configure_dma()function configures both the DMA masks and ops. Moving DMA ops configuration to probe time would thus also delay configuration of the DMA masks, which might not be safe. To avoid issues split the configuration in two to allow keeping masks configuration at device add time and move ops configuration to device probe time.
Hi Sricharan, Laurent, everyone,
I'm currently having a look at this series. Want to give it a spin with the IPMMU driver, however it takes a bit of time to wrap around my head on init ordering issues with both OF method and standard platform device setup on two different architectures...
FWIW, here's some cosmetic feedback for this patch to begin with:
In the patch title and commit message you probably want to do perform a s/of_configure_dma()/of_dma_configure()/g.
Quoted 13 lines
Signed-off-by Laurent Pinchart <[email protected]>--- drivers/of/device.c | 48 ++++++++++++++++++++++++++++++++++------------- drivers/of/platform.c | 6 ++++-- drivers/pci/probe.c | 3 ++- include/linux/of_device.h | 11 +++++++++-- 4 files changed, 50 insertions(+), 18 deletions(-)drivers/of/device.c +0
-0
drivers/of/device.c int of_device_add(struct platform_device *ofdev) · line 77
Quoted 8 lines
drivers/of/device.c +1
-1@@ -71,7 +71,7 @@ int of_device_add(struct platform_device *ofdev) }/** -* of_dma_configure - Setup DMA configuration +* of_dma_configure - Setup DMA masks and offset * @dev: Device to apply DMA configuration * @np: Pointer to OF node having DMA configuration *
The hunk above should be "of_dma_configure_masks" to make the documentation match the function name.
Quoted 7 lines
drivers/of/device.c +1
-1@@ -82,13 +82,11 @@ int of_device_add(struct platform_device *ofdev)* can use a platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE events * to fix up DMA configuration. */ -void of_dma_configure (struct device *dev,struct device_node *np) +void of_dma_configure_masks (struct device *dev,struct device_node *np) {
drivers/of/device.c int of_device_add(struct platform_device *ofdev) · line 92
Quoted 10 lines
drivers/of/device.c +2
-4-u64 dma_addr, paddr, size; -int ret; -bool coherent; +u64 dma_addr, paddr, size, range_mask;unsigned long offset; -const struct iommu_ops *iommu; +int ret;/* * Set default coherent_dma_mask to 32 bit. Drivers are expected to
Thanks,
/ magnus
PATCH 4/8 of: dma: Split of_configure_dma() into mask and ops configuration
Hi Magnus,
Quoted 16 lines · Sricharan R
Quoted 7 lines
From: Laurent Pinchart <[email protected]>
The
of_configure_dma()function configures both the DMA masks and ops. Moving DMA ops configuration to probe time would thus also delay configuration of the DMA masks, which might not be safe. To avoid issues split the configuration in two to allow keeping masks configuration at device add time and move ops configuration to device probe time.Hi Sricharan, Laurent, everyone,
I'm currently having a look at this series. Want to give it a spin with the IPMMU driver, however it takes a bit of time to wrap around my head on init ordering issues with both OF method and standard platform device setup on two different architectures...
Quoted 5 lines
FWIW, here's some cosmetic feedback for this patch to begin with:
In the patch title and commit message you probably want to do perform a s/of_configure_dma()/of_dma_configure()/g.
Ya, that right.
Thanks for having a look. I am going to post V2 shortly
Infact on the next post, this split patch won't be there.
Also, there is a bug in this series which missed before posting,
https://patchwork.kernel.org/patch/9269913/
Regards,
Sricharan
Quoted 51 lines
Quoted 13 lines
Signed-off-by Laurent Pinchart <[email protected]>--- drivers/of/device.c | 48 ++++++++++++++++++++++++++++++++++------------- drivers/of/platform.c | 6 ++++-- drivers/pci/probe.c | 3 ++- include/linux/of_device.h | 11 +++++++++-- 4 files changed, 50 insertions(+), 18 deletions(-)drivers/of/device.c +0
-0drivers/of/device.c int of_device_add(struct platform_device *ofdev) · line 77
Quoted 8 lines
drivers/of/device.c +1
-1@@ -71,7 +71,7 @@ int of_device_add(struct platform_device *ofdev) }/** -* of_dma_configure - Setup DMA configuration +* of_dma_configure - Setup DMA masks and offset * @dev: Device to apply DMA configuration * @np: Pointer to OF node having DMA configuration * The hunk above should be "
of_dma_configure_masks" to make the documentation match the function name.Quoted 7 lines
drivers/of/device.c +1
-1@@ -82,13 +82,11 @@ int of_device_add(struct platform_device *ofdev)* can use a platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE events * to fix up DMA configuration. */ -void of_dma_configure (struct device *dev,struct device_node *np) +void of_dma_configure_masks (struct device *dev,struct device_node *np) {drivers/of/device.c int of_device_add(struct platform_device *ofdev) · line 92
Quoted 10 lines
drivers/of/device.c +2
-4-u64 dma_addr, paddr, size; -int ret; -bool coherent; +u64 dma_addr, paddr, size, range_mask;unsigned long offset; -const struct iommu_ops *iommu; +int ret;/* * Set default coherent_dma_mask to 32 bit. Drivers are expected to Thanks,
/ magnus
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH 5/8 drivers: platform: Configure dma operations at probe time
In reply to Sricharan RConfiguring DMA ops at probe time will allow deferring device probe when the IOMMU isn't available yet.
--- drivers/base/dd.c | 11 +++++++++++ drivers/base/dma-mapping.c | 11 +++++++++++ include/linux/dma-mapping.h | 3 +++ 3 files changed, 25 insertions(+)
drivers/base/dd.c +11 -0
@@ -19,6 +19,7 @@#include <linux/device.h> #include <linux/delay.h> +#include <linux/dma-mapping.h> #include <linux/module.h> #include <linux/kthread.h> #include <linux/wait.h>
@@ -353,6 +354,10 @@ static int really_probe(struct device *dev, struct device_driver *drv)if (ret) goto pinctrl_bind_failed; + ret =dma_configure_ops (dev); +if (ret) + goto dma_failed; +if (driver_sysfs_add (dev)) {printk (KERN_ERR "%s: driver_sysfs_add(%s) failed\n" , __func__,dev_name (dev));
@@ -394,7 +399,10 @@ static int really_probe(struct device *dev, struct device_driver *drv) drv->bus->name, __func__, dev_name(dev), drv->name); goto done; + probe_failed: +dma_deconfigure (dev); +dma_failed:if (dev->bus)blocking_notifier_call_chain (&dev->bus->p->bus_notifier,BUS_NOTIFY_DRIVER_NOT_BOUND , dev);
@@ -780,6 +788,9 @@ static void __device_release_driver(struct device *dev) dev->bus->remove (dev); else if (drv->remove) drv->remove(dev); + +dma_deconfigure (dev ); +devres_release_all (dev); dev->driver =NULL ;dev_set_drvdata (dev,NULL );
drivers/base/dma-mapping.c +11 -0
@@ -10,6 +10,7 @@#include <linux/dma-mapping.h> #include <linux/export.h> #include <linux/gfp.h> +#include <linux/of_device.h> #include <linux/slab.h> #include <linux/vmalloc.h>
@@ -166,6 +167,16 @@ void dmam_free_noncoherent(struct device *dev, size_t size, void *vaddr, }EXPORT_SYMBOL (dmam_free_noncoherent); +int dma_configure_ops (struct device *dev) +{ +return of_dma_configure_ops (dev, dev->of_node); +} + +void dma_deconfigure (struct device *dev) +{ +of_dma_deconfigure (dev); +} +#ifdef CONFIG_HAVE_GENERIC_DMA_COHERENT static void dmam_coherent_decl_release (struct device *dev,void *res)
include/linux/dma-mapping.h +3 -0
@@ -613,6 +613,9 @@ dma_mark_declared_memory_occupied(struct device *dev, }#endif /* CONFIG_HAVE_GENERIC_DMA_COHERENT */ +int dma_configure_ops (struct device *dev); +void dma_deconfigure (struct device *dev); +/* * Managed DMA API */
Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH 5/8 drivers: platform: Configure dma operations at probe time
Hi Sricharan,
Thank you for the patch.
Quoted 23 lines · On Tuesday 09 Aug 2016 04:19:07 Sricharan R wrote:
Configuring DMA ops at probe time will allow deferring device probe when the IOMMU isn't available yet.
Signed-off-by Sricharan R <[email protected]>--- drivers/base/dd.c | 11 +++++++++++ drivers/base/dma-mapping.c | 11 +++++++++++ include/linux/dma-mapping.h | 3 +++ 3 files changed, 25 insertions(+)drivers/base/dd.c +1
-0@@ -19,6 +19,7 @@#include <linux/device.h> #include <linux/delay.h> +#include <linux/dma-mapping.h> #include <linux/module.h> #include <linux/kthread.h> #include <linux/wait.h>
drivers/base/dd.c static int really_probe(struct device *dev, struct · line 357
Quoted 4 lines
drivers/base/dd.c +1
-0@@ -353,6 +354,10 @@ static int really_probe(struct device *dev, struct device_driver *drv) if (ret) goto pinctrl_bind_failed; + ret =dma_configure_ops (dev);
Your patch doesn't remove the of_dma_configure_ops() from of_platform_device_create_pdata(). Unless I'm mistaken, you will then end up configuring the DMA ops twice, which at least on ARM will be a no-op the second time:
(arch_setup_dma_ops)
/*
* Don't override the dma_ops if they have already been set. Ideally
* this should be the only location where dma_ops are set, remove this
* check when all other callers of set_dma_ops will have disappeared.
*/
if (dev->archdata.dma_ops)
return;For the same reason I'm wondering whether you shouldn't also remove the of_dma_configure_ops() call from of_amba_device_create() (I don't remember why I left it there in my original patch series).
Quoted 7 lines
drivers/base/dd.c +3
-0+if (ret) + goto dma_failed; +if (driver_sysfs_add (dev)) {printk (KERN_ERR "%s: driver_sysfs_add(%s) failed\n" , __func__,dev_name (dev));
drivers/base/dd.c static int really_probe(struct device *dev, struct · line 408
Quoted 10 lines
drivers/base/dd.c +3
-0@@ -394,7 +399,10 @@ static int really_probe(struct device *dev, struct device_driver *drv) drv->bus->name, __func__, dev_name(dev), drv->name); goto done; + probe_failed: +dma_deconfigure (dev); +dma_failed:if (dev->bus) blocking_notifier_call_chain(&dev->bus->p->bus_notifier, BUS_NOTIFY_DRIVER_NOT_BOUND,
dev);
Quoted 53 lines
drivers/base/dd.c +3
-0@@ -780,6 +788,9 @@ static void __device_release_driver(struct device *dev) dev->bus->remove (dev); else if (drv->remove) drv->remove(dev); + +dma_deconfigure (dev ); +devres_release_all (dev); dev->driver =NULL ;dev_set_drvdata (dev,NULL );drivers/base/dma-mapping.c +11
-0@@ -10,6 +10,7 @@#include <linux/dma-mapping.h> #include <linux/export.h> #include <linux/gfp.h> +#include <linux/of_device.h> #include <linux/slab.h> #include <linux/vmalloc.h> @@ -166,6 +167,16 @@ void dmam_free_noncoherent(struct device *dev, size_t size, void *vaddr, }EXPORT_SYMBOL (dmam_free_noncoherent); +int dma_configure_ops (struct device *dev) +{ +return of_dma_configure_ops (dev, dev->of_node); +} + +void dma_deconfigure (struct device *dev) +{ +of_dma_deconfigure (dev); +} +#ifdef CONFIG_HAVE_GENERIC_DMA_COHERENT static void dmam_coherent_decl_release (struct device *dev,void *res)include/linux/dma-mapping.h +3
-0@@ -613,6 +613,9 @@ dma_mark_declared_memory_occupied(struct device *dev, }#endif /* CONFIG_HAVE_GENERIC_DMA_COHERENT */ +int dma_configure_ops (struct device *dev); +void dma_deconfigure (struct device *dev); +/* * Managed DMA API */
Signature · 3 lines
Regards, Laurent Pinchart
PATCH 5/8 drivers: platform: Configure dma operations at probe time
Hi Laurent,
Quoted 31 lines
Hi Sricharan,
Thank you for the patch.
Quoted 26 lines · On Tuesday 09 Aug 2016 04:19:07 Sricharan R wrote:
Configuring DMA ops at probe time will allow deferring device probe when the IOMMU isn't available yet.
Signed-off-by Sricharan R <[email protected]>--- drivers/base/dd.c | 11 +++++++++++ drivers/base/dma-mapping.c | 11 +++++++++++ include/linux/dma-mapping.h | 3 +++ 3 files changed, 25 insertions(+)drivers/base/dd.c +1
-0@@ -19,6 +19,7 @@#include <linux/device.h> #include <linux/delay.h> +#include <linux/dma-mapping.h> #include <linux/module.h> #include <linux/kthread.h> #include <linux/wait.h> @@ -353,6 +354,10 @@ static int really_probe(struct device *dev, struct device_driver *drv) if (ret) goto pinctrl_bind_failed;
Quoted 6 lines
drivers/base/dd.c static int really_probe(struct device *dev, struct device_driver *drv) · line 357
Quoted 1 line
drivers/base/dd.c +1
-0+ ret =dma_configure_ops (dev);Your patch doesn't remove the
of_dma_configure_ops()fromof_platform_device_create_pdata(). Unless I'm mistaken, you will then end up configuring the DMA ops twice, which at least on ARM will be a no-op the second time:
I did remove both of_dmaconfigure/deconfigure from other places, but i did as a separate next patch [1] (should have been squashed here).
I will squash for the next repost.
[1] https://www.spinics.net/lists/arm-kernel/msg522510.html
Regards,
Sricharan
PATCH 6/8 drivers: platform: Remove call to of_dma_(con/decon)figure_ops
In reply to Sricharan RThe dma_ops gets configured for the device generically during the device_attach call. So remove it from the platform bus probe. Similarly remove the deconfigure calls as well, since it is now called form the device_detach path.
--- drivers/of/platform.c | 4 ---- 1 file changed, 4 deletions(-)
drivers/of/platform.c +0 -4
@@ -180,11 +180,9 @@ static struct platform_device *of_platform_device_create_pdata( dev->dev.bus = &platform_bus_type; dev->dev.platform_data = platform_data;of_dma_configure_masks (&dev->dev, dev->dev.of_node); -of_dma_configure_ops (&dev->dev, dev->dev.of_node);of_msi_configure (&dev->dev, dev->dev.of_node);if (of_device_add (dev) !=0 ) { -of_dma_deconfigure (&dev->dev);platform_device_put (dev); goto err_clear_flag; }
@@ -245,7 +243,6 @@ static struct amba_device *of_amba_device_create(struct device_node *node,else of_device_make_bus_id (&dev->dev);of_dma_configure_masks (&dev->dev, dev->dev.of_node); -of_dma_configure_ops (&dev->dev, dev->dev.of_node);/* Allow the HW Peripheral ID to be overridden */ prop =of_get_property (node,"arm,primecell-periphid" ,NULL );
@@ -503,7 +500,6 @@ static int of_platform_device_destroy(struct device *dev, void *data)amba_device_unregister (to_amba_device (dev));#endif -of_dma_deconfigure (dev);of_node_clear_flag (dev->of_node,OF_POPULATED );of_node_clear_flag (dev->of_node,OF_POPULATED_BUS );return 0 ;
Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH 6/8 drivers: platform: Remove call to of_dma_(con/decon)figure_ops
Hi,
Quoted 4 lines · Sricharan R
The
dma_opsgets configured for the device generically during thedevice_attachcall. So remove it from the platform bus probe. Similarly remove the deconfigure calls as well, since it is now called form thedevice_detachpath.
Doesn't this patch need to be squashed with previous one to avoid breaking things in between by having the of_dma_configure_ops() called two times?
Best regards,
Tomasz
PATCH 6/8 drivers: platform: Remove call to of_dma_(con/decon)figure_ops
Hi Tomaz,
Quoted 9 lines
Quoted 4 lines
The
dma_opsgets configured for the device generically during thedevice_attachcall. So remove it from the platform bus probe. Similarly remove the deconfigure calls as well, since it is now called form thedevice_detachpath.Doesn't this patch need to be squashed with previous one to avoid breaking things in between by having the
of_dma_configure_ops()called two times?
okay, i did it this way, for the sake of review. But ya will squash it for the next post.
Regards,
Sricharan
PATCH 7/8 iommu: of: Handle IOMMU lookup failure with deferred probing or error
In reply to Sricharan RFrom: Laurent Pinchart <[email protected]>
Failures to look up an IOMMU when parsing the DT iommus property need to be handled separately from the .of_xlate() failures to support deferred probing.
The lack of a registered IOMMU can be caused by the lack of a driver for the IOMMU, the IOMMU device probe not having been performed yet, having been deferred, or having failed.
The first case occurs when the device tree describes the bus master and IOMMU topology correctly but no device driver exists for the IOMMU yet or the device driver has not been compiled in. Return NULL, the caller will configure the device without an IOMMU.
The second and third cases are handled by deferring the probe of the bus master device which will eventually get reprobed after the IOMMU.
The last case is currently handled by deferring the probe of the bus master device as well. A mechanism to either configure the bus master device without an IOMMU or to fail the bus master device probe depending on whether the IOMMU is optional or mandatory would be a good enhancement.
--- drivers/iommu/of_iommu.c | 21 +++++++++++++++++---- drivers/of/device.c | 2 ++ 2 files changed, 19 insertions(+), 4 deletions(-)
drivers/iommu/of_iommu.c +17 -4
@@ -149,7 +149,7 @@ const struct iommu_ops *of_iommu_configure(struct device *dev, {struct of_phandle_args iommu_spec;struct device_node *np =NULL ; -struct iommu_ops *ops =NULL ; +const struct iommu_ops *ops =NULL ;int idx =0 ;if (dev_is_pci (dev)) {
@@ -189,8 +189,21 @@ const struct iommu_ops *of_iommu_configure(struct device *dev, np = iommu_spec.np; ops =of_iommu_get_ops (np); -if (!ops || !ops->of_xlate || ops->of_xlate (dev, &iommu_spec)) +if (!ops) { +const struct of_device_id *oid; + + oid =of_match_node (&__iommu_of_table, np); + ops = oid ?ERR_PTR (-EPROBE_DEFER ) :NULL ; goto err_put_node; + } + +if (!ops->of_xlate || ops->of_xlate (dev, &iommu_spec)) { + ops =NULL ; + goto err_put_node; + } + +if (ops->add_device) + ops = ops->add_device (dev) ? ops :NULL ;of_node_put (np); idx++;
@@ -200,7 +213,7 @@ const struct iommu_ops *of_iommu_configure(struct device *dev, err_put_node:of_node_put (np); -return NULL ; +return ops; }void __init of_iommu_init (void )
@@ -211,7 +224,7 @@ void __init of_iommu_init(void)for_each_matching_node_and_match (np, matches, &match) {const of_iommu_init_fn init_fn = match->data; -if (init_fn (np)) +if (init_fn &&init_fn (np))pr_err ("Failed to initialise IOMMU %s\n" ,of_node_full_name (np)); }
drivers/of/device.c +2 -0
@@ -149,6 +149,8 @@ int of_dma_configure_ops(struct device *dev, struct device_node *np) coherent ? " " : " not "); iommu =of_iommu_configure (dev, np); +if (IS_ERR (iommu)) +return PTR_ERR (iommu);dev_dbg (dev,"device is%sbehind an iommu\n" , iommu ?" " :" not " );
Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH 7/8 iommu: of: Handle IOMMU lookup failure with deferred probing or error
Hi,
drivers/iommu/of_iommu.c const struct iommu_ops *of_iommu_configure(struct device *dev, · line 206
Quoted 3 lines · Sricharan R
drivers/iommu/of_iommu.c +3
-0+ +if (ops->add_device) + ops = ops->add_device (dev) ? ops :NULL ;
Patch description fails to mention anything about this change. Also it looks slightly incorrect to lose the error condition here. I think we should at least print some error message here and tell the user that we are falling back to non-IOMMU setup.
Quoted 13 lines
of_node_put(np); idx++;drivers/iommu/of_iommu.c +1
-1@@ -200,7 +213,7 @@ const struct iommu_ops *of_iommu_configure(struct device *dev, err_put_node:of_node_put (np); -return NULL ; +return ops; }void __init of_iommu_init (void )
drivers/iommu/of_iommu.c void __init of_iommu_init(void) · line 227
Quoted 5 lines
drivers/iommu/of_iommu.c +1
-1@@ -211,7 +224,7 @@ void __init of_iommu_init(void)for_each_matching_node_and_match (np, matches, &match) {const of_iommu_init_fn init_fn = match->data; -if (init_fn (np)) +if (init_fn &&init_fn (np))
When is it possible to have NULL init_fn?
drivers/of/device.c int of_dma_configure_ops(struct device *dev, struct device_node *np) · line 153
Quoted 13 lines
drivers/iommu/of_iommu.c +0
-0pr_err ("Failed to initialise IOMMU %s\n" ,of_node_full_name (np)); }drivers/of/device.c +2
-0@@ -149,6 +149,8 @@ int of_dma_configure_ops(struct device *dev, struct device_node *np) coherent ? " " : " not "); iommu =of_iommu_configure (dev, np); +if (IS_ERR (iommu)) +return PTR_ERR (iommu);
nit: Would be good to add a blank line here for improved readability.
drivers/of/device.c int of_dma_configure_ops(struct device *dev, struct device_node *np) · line 156
Quoted 3 lines
drivers/of/device.c +0
-0dev_dbg (dev,"device is%sbehind an iommu\n" , iommu ?" " :" not " );
Best regards,
Tomasz
PATCH 7/8 iommu: of: Handle IOMMU lookup failure with deferred probing or error
Hi Tomaz,
Quoted 7 lines
drivers/iommu/of_iommu.c const struct iommu_ops *of_iommu_configure(struct device *dev, · line 206
Quoted 2 lines
drivers/iommu/of_iommu.c +2
-0+if (ops->add_device) + ops = ops->add_device (dev) ? ops :NULL ;Patch description fails to mention anything about this change. Also it looks slightly incorrect to lose the error condition here. I think we should at least print some error message here and tell the user that we are falling back to non-IOMMU setup.
Ok, will have to improve the patch description to add this and will fix the error value as well. Will also add the remove_device during
deconfigure as well.
Quoted 17 lines
Quoted 17 lines
of_node_put(np); idx++;drivers/iommu/of_iommu.c +1
-1@@ -200,7 +213,7 @@ const struct iommu_ops *of_iommu_configure(struct device *dev, err_put_node:of_node_put (np); -return NULL ; +return ops; }void __init of_iommu_init (void )@@ -211,7 +224,7 @@ void __init of_iommu_init(void)for_each_matching_node_and_match (np, matches, &match) {const of_iommu_init_fn init_fn = match->data;
Quoted 4 lines
drivers/iommu/of_iommu.c void __init of_iommu_init(void) · line 227
Quoted 2 lines
drivers/iommu/of_iommu.c +1
-1-if (init_fn (np)) +if (init_fn &&init_fn (np))When is it possible to have NULL
init_fn?
ya wrong, should not be needed.
Quoted 11 lines
Quoted 11 lines
drivers/iommu/of_iommu.c +0
-0pr_err ("Failed to initialise IOMMU %s\n" ,of_node_full_name (np)); }drivers/of/device.c +0
-0@@ -149,6 +149,8 @@ int of_dma_configure_ops(struct device *dev, struct device_node *np) coherent ? " " : " not ");
Quoted 5 lines
drivers/of/device.c int of_dma_configure_ops(struct device *dev, struct device_node *np) · line 153
Quoted 3 lines
drivers/of/device.c +2
-0iommu =of_iommu_configure (dev, np); +if (IS_ERR (iommu)) +return PTR_ERR (iommu);nit: Would be good to add a blank line here for improved readability.
ok, will add.
Regards,
Sricharan
Marek Szyprowski
maintainerPATCH 7/8 iommu: of: Handle IOMMU lookup failure with deferred probing or error
Hi Sricharan,
Quoted 28 lines · Sricharan
Hi Tomaz,
Quoted 6 lines
drivers/iommu/of_iommu.c const struct iommu_ops *of_iommu_configure(struct device *dev, · line 206
Quoted 2 lines
drivers/iommu/of_iommu.c +2
-0+if (ops->add_device) + ops = ops->add_device (dev) ? ops :NULL ;Patch description fails to mention anything about this change. Also it looks slightly incorrect to lose the error condition here. I think we should at least print some error message here and tell the user that we are falling back to non-IOMMU setup.
Ok, will have to improve the patch description to add this and will fix the error value as well. Will also add the
remove_deviceduring
deconfigure as well.Quoted 16 lines
Quoted 16 lines
of_node_put(np); idx++;drivers/iommu/of_iommu.c +1
-1@@ -200,7 +213,7 @@ const struct iommu_ops *of_iommu_configure(struct device *dev, err_put_node:of_node_put (np); -return NULL ; +return ops; }void __init of_iommu_init (void )@@ -211,7 +224,7 @@ void __init of_iommu_init(void)for_each_matching_node_and_match (np, matches, &match) {const of_iommu_init_fn init_fn = match->data;
Quoted 4 lines
Quoted 3 lines
drivers/iommu/of_iommu.c void __init of_iommu_init(void) · line 227
Quoted 2 lines
drivers/iommu/of_iommu.c +1
-1-if (init_fn (np)) +if (init_fn &&init_fn (np))When is it possible to have NULL
init_fn?ya wrong, should not be needed.
init_fn can be NULL if you convert IOMMU driver to use platform device infrastructure based initialization and you don't need to do anything before the driver gets probed, so please keep this check.
I used this approach here:
https://git.linaro.org/people/marek.szyprowski/linux-srpol.git/commitdiff/a30735973573128b14bb4a25cf4debaa0979a655
(this commit is a part of v4.8-clocks-pm branch)
IOMMU_OF_DECLARE() with NULL init_fn is needed to notify IOMMU core that this IOMMU driver is available in the system and core has to defer probing of drivers before the IOMMU driver gets initialized from its controller's platform device.
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
drivers/iommu/of_iommu.c +0
-0pr_err ("Failed to initialise IOMMU %s\n" ,of_node_full_name (np)); }drivers/of/device.c +0
-0@@ -149,6 +149,8 @@ int of_dma_configure_ops(struct device *dev, struct device_node *np) coherent ? " " : " not ");
Quoted 5 lines
Quoted 4 lines
drivers/of/device.c int of_dma_configure_ops(struct device *dev, struct device_node *np) · line 153
Quoted 3 lines
drivers/of/device.c +2
-0iommu =of_iommu_configure (dev, np); +if (IS_ERR (iommu)) +return PTR_ERR (iommu);nit: Would be good to add a blank line here for improved readability.
ok, will add.
Best regards
Signature · 2 lines
Marek Szyprowski, PhD Samsung R&D Institute Poland
PATCH 7/8 iommu: of: Handle IOMMU lookup failure with deferred probing or error
Hi Marek,
Quoted 41 lines
Hi Sricharan,
Quoted 28 lines · Sricharan
Hi Tomaz,
Quoted 6 lines
drivers/iommu/of_iommu.c const struct iommu_ops *of_iommu_configure(struct device *dev, · line 206
Quoted 2 lines
drivers/iommu/of_iommu.c +2
-0+if (ops->add_device) + ops = ops->add_device (dev) ? ops :NULL ;Patch description fails to mention anything about this change. Also it looks slightly incorrect to lose the error condition here. I think we should at least print some error message here and tell the user that we are falling back to non-IOMMU setup.
Ok, will have to improve the patch description to add this and will fix the error value as well. Will also add the
remove_deviceduring
deconfigure as well.Quoted 16 lines
Quoted 16 lines
of_node_put(np); idx++;drivers/iommu/of_iommu.c +1
-1@@ -200,7 +213,7 @@ const struct iommu_ops *of_iommu_configure(struct device *dev, err_put_node:of_node_put (np); -return NULL ; +return ops; }void __init of_iommu_init (void )@@ -211,7 +224,7 @@ void __init of_iommu_init(void)for_each_matching_node_and_match (np, matches, &match) {const of_iommu_init_fn init_fn = match->data;Quoted 4 lines
Quoted 3 lines
drivers/iommu/of_iommu.c void __init of_iommu_init(void) · line 227
Quoted 2 lines
drivers/iommu/of_iommu.c +1
-1-if (init_fn (np)) +if (init_fn &&init_fn (np))When is it possible to have NULL
init_fn?ya wrong, should not be needed.
init_fncan be NULL if you convert IOMMU driver to use platform device infrastructure based initialization and you don't need to do anything before the driver gets probed, so please keep this check.
Quoted 9 lines
I used this approach here:
https://git.linaro.org/people/marek.szyprowski/linux-srpol.git/commitdiff/a30735973573128b14bb4a25cf4debaa0979a655
(this commit is a part of v4.8-clocks-pm branch)
IOMMU_OF_DECLARE()with NULLinit_fnis needed to notify IOMMU core that this IOMMU driver is available in the system and core has to defer probing of drivers before the IOMMU driver gets initialized from its controller's platform device.
ok, thanks, understand. I was not thinking about the non-dt case.
I will keep this check then.
Regards,
Sricharan
Marek Szyprowski
maintainerPATCH 7/8 iommu: of: Handle IOMMU lookup failure with deferred probing or error
In reply to Sricharan RHi Sricharan,
Quoted 18 lines · Sricharan R
From: Laurent Pinchart <[email protected]>
Failures to look up an IOMMU when parsing the DT iommus property need to be handled separately from the .of_xlate() failures to support deferred probing.
The lack of a registered IOMMU can be caused by the lack of a driver for the IOMMU, the IOMMU device probe not having been performed yet, having been deferred, or having failed.
The first case occurs when the device tree describes the bus master and IOMMU topology correctly but no device driver exists for the IOMMU yet or the device driver has not been compiled in. Return NULL, the caller will configure the device without an IOMMU.
The second and third cases are handled by deferring the probe of the bus master device which will eventually get reprobed after the IOMMU.
Quoted 7 lines
The last case is currently handled by deferring the probe of the bus master device as well. A mechanism to either configure the bus master device without an IOMMU or to fail the bus master device probe depending on whether the IOMMU is optional or mandatory would be a good enhancement.
Signed-off-by Laurent Pinchart
It is a common practice to briefly describe here what has been changed since the original patch if you have modified it (see commit 855ed04a3758b205e84b269f92d26ab36ed8e2f7 for the example).
drivers/iommu/of_iommu.c const struct iommu_ops *of_iommu_configure(struct
Quoted 11 lines
Signed-off-by Sricharan R <[email protected]>--- drivers/iommu/of_iommu.c | 21 +++++++++++++++++---- drivers/of/device.c | 2 ++ 2 files changed, 19 insertions(+), 4 deletions(-)drivers/iommu/of_iommu.c +0
-0@@ -149,7 +149,7 @@ const struct iommu_ops *of_iommu_configure(struct
device *dev,
drivers/iommu/of_iommu.c const struct iommu_ops
Quoted 9 lines
drivers/iommu/of_iommu.c +1
-1{struct of_phandle_args iommu_spec;struct device_node *np =NULL ; -struct iommu_ops *ops =NULL ; +const struct iommu_ops *ops =NULL ;int idx =0 ;if (dev_is_pci (dev)) {@@ -189,8 +189,21 @@ const struct iommu_ops
*of_iommu_configure(struct device *dev,
Quoted 9 lines
drivers/iommu/of_iommu.c +5
-1np = iommu_spec.np; ops =of_iommu_get_ops (np); -if (!ops || !ops->of_xlate || ops->of_xlate (dev, &iommu_spec)) +if (!ops) { +const struct of_device_id *oid; + + oid =of_match_node (&__iommu_of_table, np); + ops = oid ?ERR_PTR (-EPROBE_DEFER ) :NULL ;
drivers/iommu/of_iommu.c const struct iommu_ops *of_iommu_configure(struct device *dev, · line 206
Quoted 10 lines
drivers/iommu/of_iommu.c +9
-0goto err_put_node; + } + +if (!ops->of_xlate || ops->of_xlate (dev, &iommu_spec)) { + ops =NULL ; + goto err_put_node; + } + +if (ops->add_device) + ops = ops->add_device (dev) ? ops :NULL ;
ops->add_device() returns ZERO on success or error code on failure, so the above
line should be changed to:
ops = (ops->add_device(dev) == 0) ? ops : NULL;
drivers/iommu/of_iommu.c const struct iommu_ops *of_iommu_configure(struct
Quoted 3 lines
drivers/iommu/of_iommu.c +0
-0of_node_put (np); idx++;@@ -200,7 +213,7 @@ const struct iommu_ops *of_iommu_configure(struct
device *dev,
drivers/of/device.c int of_dma_configure_ops(struct device *dev,
Quoted 22 lines
drivers/iommu/of_iommu.c +2
-2err_put_node:of_node_put (np); -return NULL ; +return ops; }void __init of_iommu_init (void )@@ -211,7 +224,7 @@ void __init of_iommu_init(void)for_each_matching_node_and_match (np, matches, &match) {const of_iommu_init_fn init_fn = match->data; -if (init_fn (np)) +if (init_fn &&init_fn (np))pr_err ("Failed to initialise IOMMU %s\n" ,of_node_full_name (np)); }drivers/of/device.c +0
-0@@ -149,6 +149,8 @@ int of_dma_configure_ops(struct device *dev,
struct device_node *np)
drivers/of/device.c int of_dma_configure_ops(struct device *dev, struct device_node *np) · line 156
Quoted 8 lines
drivers/of/device.c +2
-0coherent ? " " : " not "); iommu =of_iommu_configure (dev, np); +if (IS_ERR (iommu)) +return PTR_ERR (iommu);dev_dbg (dev,"device is%sbehind an iommu\n" , iommu ?" " :" not " );
Best regards
Signature · 2 lines
Marek Szyprowski, PhD Samsung R&D Institute Poland
PATCH 7/8 iommu: of: Handle IOMMU lookup failure with deferred probing or error
Hi Marek,
Quoted 27 lines
Hi Sricharan,
Quoted 24 lines · Sricharan R
From: Laurent Pinchart <[email protected]>
Failures to look up an IOMMU when parsing the DT iommus property need to be handled separately from the .of_xlate() failures to support deferred probing.
The lack of a registered IOMMU can be caused by the lack of a driver for the IOMMU, the IOMMU device probe not having been performed yet, having been deferred, or having failed.
The first case occurs when the device tree describes the bus master and IOMMU topology correctly but no device driver exists for the IOMMU yet or the device driver has not been compiled in. Return NULL, the caller will configure the device without an IOMMU.
The second and third cases are handled by deferring the probe of the bus master device which will eventually get reprobed after the IOMMU.
The last case is currently handled by deferring the probe of the bus master device as well. A mechanism to either configure the bus master device without an IOMMU or to fail the bus master device probe depending on whether the IOMMU is optional or mandatory would be a good enhancement.
Quoted 7 lines
Quoted 1 line
Signed-off-by Laurent PinchartIt is a common practice to briefly describe here what has been changed since the original patch if you have modified it (see commit
855ed04a3758b205e84b269f92d26ab36ed8e2f7for the example).
Sorry i missed updating it. i will update the log.
Quoted 45 lines
drivers/iommu/of_iommu.c const struct iommu_ops *of_iommu_configure(struct
Quoted 11 lines
Signed-off-by Sricharan R <[email protected]>--- drivers/iommu/of_iommu.c | 21 +++++++++++++++++---- drivers/of/device.c | 2 ++ 2 files changed, 19 insertions(+), 4 deletions(-)drivers/iommu/of_iommu.c +0
-0@@ -149,7 +149,7 @@ const struct iommu_ops *of_iommu_configure(structdevice *dev,
drivers/iommu/of_iommu.c const struct iommu_ops
Quoted 9 lines
drivers/iommu/of_iommu.c +1
-1{struct of_phandle_args iommu_spec;struct device_node *np =NULL ; -struct iommu_ops *ops =NULL ; +const struct iommu_ops *ops =NULL ;int idx =0 ;if (dev_is_pci (dev)) {@@ -189,8 +189,21 @@ const struct iommu_ops*of_iommu_configure(struct device *dev,
Quoted 9 lines
drivers/iommu/of_iommu.c +5
-1np = iommu_spec.np; ops =of_iommu_get_ops (np); -if (!ops || !ops->of_xlate || ops->of_xlate (dev, &iommu_spec)) +if (!ops) { +const struct of_device_id *oid; + + oid =of_match_node (&__iommu_of_table, np); + ops = oid ?ERR_PTR (-EPROBE_DEFER ) :NULL ;drivers/iommu/of_iommu.c const struct iommu_ops *of_iommu_configure(struct device *dev, · line 206
Quoted 10 lines
drivers/iommu/of_iommu.c +9
-0goto err_put_node; + } + +if (!ops->of_xlate || ops->of_xlate (dev, &iommu_spec)) { + ops =NULL ; + goto err_put_node; + } + +if (ops->add_device) + ops = ops->add_device (dev) ? ops :NULL ;
ops->add_device()returns ZERO on success or error code on failure, so the above
line should be changed to:
Quoted 1 line
ops = (ops->add_device(dev) == 0) ? ops : NULL;
Yes, i fixed it while testing, but somehow have missed it while sending it.
will fix it.
Regards,
Sricharan
PATCH 8/8 drivers: iommu: arm-smmu: Set iommu_ops in probe
In reply to Sricharan Riommu_ops should be set after the iommu is probed. This ensures that the iommu is really ready when master's iommu ops are set during their probe or else deferred.
--- drivers/iommu/arm-smmu.c | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-)
drivers/iommu/arm-smmu.c +2 -15
@@ -2071,6 +2071,7 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)if (of_property_read_bool (dev->of_node,"mmu-masters" ))arm_smmu_probe_mmu_masters (smmu);arm_smmu_device_reset (smmu); +of_iommu_set_ops (dev->of_node, &arm_smmu_ops);return 0 ; out_free_irqs:
@@ -2160,21 +2161,7 @@ module_exit(arm_smmu_exit);static int __init arm_smmu_of_init (struct device_node *np) { -struct arm_smmu_device *smmu; -struct platform_device *pdev; -int ret =arm_smmu_init (); - -if (ret) -return ret; - - pdev =of_platform_device_create (np,NULL , platform_bus_type.dev_root); -if (!pdev) -return -ENODEV ; - - smmu =platform_get_drvdata (pdev); -of_iommu_set_ops (np, &arm_smmu_ops); - -return 0 ; +return arm_smmu_init (); }IOMMU_OF_DECLARE (arm_smmuv1,"arm,smmu-v1" , arm_smmu_of_init);IOMMU_OF_DECLARE (arm_smmuv2,"arm,smmu-v2" , arm_smmu_of_init);
Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation