[PATCH 0/8] IOMMU Probe deferral support

[PATCH 0/8] IOMMU Probe deferral support

[Sricharan R]

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

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

[Sricharan R]

From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

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.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 arch/arm/mm/dma-mapping.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index ff7ed56..4686d66 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -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
-- 
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()

[Marek Szyprowski]

Hi Sricharan and Laurent,


On 2016-08-09 00:49, Sricharan R wrote:
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> 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.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>   arch/arm/mm/dma-mapping.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index ff7ed56..4686d66 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -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

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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[PATCH 1/8] arm: dma-mapping: Don't override dma_ops in arch_setup_dma_ops()

[Sricharan]

Hi Marek,

>Hi Sricharan and Laurent,
>
>
>On 2016-08-09 00:49, Sricharan R wrote:
>> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>
>> 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.
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>> ---
>>   arch/arm/mm/dma-mapping.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
>> index ff7ed56..4686d66 100644
>> --- a/arch/arm/mm/dma-mapping.c
>> +++ b/arch/arm/mm/dma-mapping.c
>> @@ -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
>
>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
>
 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()

[Sricharan R]

From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

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.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/of/address.c | 20 ++++++++++++++++++--
 drivers/of/device.c  | 15 ---------------
 2 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 0a553c0..8cb1aee 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -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);
diff --git a/drivers/of/device.c b/drivers/of/device.c
index fd5cfad..d9898d9 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -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);
 	}
 
-- 
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

[Sricharan R]

From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

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.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/of/device.c       | 12 ++++++++++++
 drivers/of/platform.c     |  5 -----
 include/linux/of_device.h |  3 +++
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index d9898d9..1c843e2 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -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);
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 16e8daf..b6d6b41 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -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
diff --git a/include/linux/of_device.h b/include/linux/of_device.h
index cc7dd687..d20a31a 100644
--- a/include/linux/of_device.h
+++ b/include/linux/of_device.h
@@ -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 */
-- 
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

[Sricharan R]

From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

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.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 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(-)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 1c843e2..e1fad50 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -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
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index b6d6b41..af3b291 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -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);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 8e3ef72..f8f5f16 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -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);
diff --git a/include/linux/of_device.h b/include/linux/of_device.h
index d20a31a..4b8cb7d 100644
--- a/include/linux/of_device.h
+++ b/include/linux/of_device.h
@@ -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 */
-- 
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

[Tomasz Figa]

Hi,

On Tue, Aug 9, 2016 at 7:49 AM, Sricharan R <sricharan@codeaurora.org> wrote:
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> 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

[Laurent Pinchart]

Hi Tomasz,

On Friday 12 Aug 2016 16:31:47 Tomasz Figa wrote:
> On Tue, Aug 9, 2016 at 7:49 AM, Sricharan R wrote:
> > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > 
> > 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.

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.

-- 
Regards,

Laurent Pinchart

[PATCH 4/8] of: dma: Split of_configure_dma() into mask and ops configuration

[Sricharan]

Hi,

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

[Magnus Damm]

On Tue, Aug 9, 2016 at 7:49 AM, Sricharan R <sricharan@codeaurora.org> wrote:
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> 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.

> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  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(-)
>
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 1c843e2..e1fad50 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -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.

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

Thanks,

/ magnus

[PATCH 4/8] of: dma: Split of_configure_dma() into mask and ops configuration

[Sricharan]

Hi Magnus,

>On Tue, Aug 9, 2016 at 7:49 AM, Sricharan R <sricharan@codeaurora.org> wrote:
>> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>
>> 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.
>
  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
 

>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>> ---
>>  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(-)
>>
>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> index 1c843e2..e1fad50 100644
>> --- a/drivers/of/device.c
>> +++ b/drivers/of/device.c
>> @@ -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.
>
>> @@ -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
>
>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

[Sricharan R]

Configuring DMA ops at probe time will allow deferring device probe when
the IOMMU isn't available yet.

Signed-off-by: Sricharan R <sricharan@codeaurora.org>
---
 drivers/base/dd.c           | 11 +++++++++++
 drivers/base/dma-mapping.c  | 11 +++++++++++
 include/linux/dma-mapping.h |  3 +++
 3 files changed, 25 insertions(+)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 16688f5..b9978af 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -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);
diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c
index d799662..ccc1c6e 100644
--- a/drivers/base/dma-mapping.c
+++ b/drivers/base/dma-mapping.c
@@ -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)
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 71c1b21..a49aac9 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -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
  */
-- 
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

[Laurent Pinchart]

Hi Sricharan,

Thank you for the patch.

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 <sricharan@codeaurora.org>
> ---
>  drivers/base/dd.c           | 11 +++++++++++
>  drivers/base/dma-mapping.c  | 11 +++++++++++
>  include/linux/dma-mapping.h |  3 +++
>  3 files changed, 25 insertions(+)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 16688f5..b9978af 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -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);

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

> +	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);
> diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c
> index d799662..ccc1c6e 100644
> --- a/drivers/base/dma-mapping.c
> +++ b/drivers/base/dma-mapping.c
> @@ -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)
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 71c1b21..a49aac9 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -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
>   */

-- 
Regards,

Laurent Pinchart

[PATCH 5/8] drivers: platform: Configure dma operations at probe time

[Sricharan]

Hi Laurent,

>Hi Sricharan,
>
>Thank you for the patch.
>
>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 <sricharan@codeaurora.org>
>> ---
>>  drivers/base/dd.c           | 11 +++++++++++
>>  drivers/base/dma-mapping.c  | 11 +++++++++++
>>  include/linux/dma-mapping.h |  3 +++
>>  3 files changed, 25 insertions(+)
>>
>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> index 16688f5..b9978af 100644
>> --- a/drivers/base/dd.c
>> +++ b/drivers/base/dd.c
>> @@ -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);
>
>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:
    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

[Sricharan R]

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

Signed-off-by: Sricharan R <sricharan@codeaurora.org>
---
 drivers/of/platform.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index af3b291..b2872be 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -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;
-- 
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

[Tomasz Figa]

Hi,

On Tue, Aug 9, 2016 at 7:49 AM, Sricharan R <sricharan@codeaurora.org> wrote:
> The 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.

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

[Sricharan]

Hi Tomaz,

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

[Sricharan R]

From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

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.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Sricharan R <sricharan@codeaurora.org>
---
 drivers/iommu/of_iommu.c | 21 +++++++++++++++++----
 drivers/of/device.c      |  2 ++
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 4c4219d..3994cf5 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -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));
 	}
diff --git a/drivers/of/device.c b/drivers/of/device.c
index e1fad50..92e02dc 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -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 ");
 
-- 
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

[Tomasz Figa]

Hi,

On Tue, Aug 9, 2016 at 7:49 AM, Sricharan R <sricharan@codeaurora.org> wrote:
> +
> +               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.

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

When is it possible to have NULL init_fn?

>                         pr_err("Failed to initialise IOMMU %s\n",
>                                 of_node_full_name(np));
>         }
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index e1fad50..92e02dc 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -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.

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

[Sricharan]

Hi Tomaz,

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

>
>>
>>                 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))
>
>When is it possible to have NULL init_fn?
   ya wrong, should not be needed.
 
>
>>                         pr_err("Failed to initialise IOMMU %s\n",
>>                                 of_node_full_name(np));
>>         }
>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> index e1fad50..92e02dc 100644
>> --- a/drivers/of/device.c
>> +++ b/drivers/of/device.c
>> @@ -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.
   ok, will add.

Regards,
 Sricharan

[PATCH 7/8] iommu: of: Handle IOMMU lookup failure with deferred probing or error

[Marek Szyprowski]

Hi Sricharan,


On 2016-08-12 17:40, Sricharan wrote:
> Hi Tomaz,
>
>>> +               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.
>
>>
>>>                  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))
>> 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.

>
>>
>>>                          pr_err("Failed to initialise IOMMU %s\n",
>>>                                  of_node_full_name(np));
>>>          }
>>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>>> index e1fad50..92e02dc 100644
>>> --- a/drivers/of/device.c
>>> +++ b/drivers/of/device.c
>>> @@ -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.
>     ok, will add.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[PATCH 7/8] iommu: of: Handle IOMMU lookup failure with deferred probing or error

[Sricharan]

Hi Marek,

>Hi Sricharan,
>
>
>On 2016-08-12 17:40, Sricharan wrote:
>> Hi Tomaz,
>>
>>>> +               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.
>>
>>>
>>>>                  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))
>>> 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.
>
   ok, thanks, understand. I was not thinking about the non-dt case.
I will keep this check then.

Regards,
 Sricharan     

[PATCH 7/8] iommu: of: Handle IOMMU lookup failure with deferred probing or error

[Marek Szyprowski]

Hi Sricharan,

On 2016-08-09 00:49, Sricharan R wrote:
 > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
 >
 > 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.
 >
 > Signed-off-by: Laurent Pinchart 
<laurent.pinchart+renesas@ideasonboard.com>

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

 > Signed-off-by: Sricharan R <sricharan@codeaurora.org>
 > ---
 >  drivers/iommu/of_iommu.c | 21 +++++++++++++++++----
 >  drivers/of/device.c      |  2 ++
 >  2 files changed, 19 insertions(+), 4 deletions(-)
 >
 > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
 > index 4c4219d..3994cf5 100644
 > --- a/drivers/iommu/of_iommu.c
 > +++ b/drivers/iommu/of_iommu.c
 > @@ -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;

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;


 >          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));
 >      }
 > diff --git a/drivers/of/device.c b/drivers/of/device.c
 > index e1fad50..92e02dc 100644
 > --- a/drivers/of/device.c
 > +++ b/drivers/of/device.c
 > @@ -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 ");
 >

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[PATCH 7/8] iommu: of: Handle IOMMU lookup failure with deferred probing or error

[Sricharan]

Hi Marek,

>Hi Sricharan,
>
>On 2016-08-09 00:49, Sricharan R wrote:
> > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >
> > 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.
> >
> > Signed-off-by: Laurent Pinchart
><laurent.pinchart+renesas@ideasonboard.com>
>
>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).
>
 Sorry i missed updating it. i will update the log.

> > Signed-off-by: Sricharan R <sricharan@codeaurora.org>
> > ---
> >  drivers/iommu/of_iommu.c | 21 +++++++++++++++++----
> >  drivers/of/device.c      |  2 ++
> >  2 files changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> > index 4c4219d..3994cf5 100644
> > --- a/drivers/iommu/of_iommu.c
> > +++ b/drivers/iommu/of_iommu.c
> > @@ -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;
>
>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;

 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

[Sricharan R]

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

Signed-off-by: Sricharan R <sricharan@codeaurora.org>
---
 drivers/iommu/arm-smmu.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 5f12005..5e11bae 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -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);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation