[PATCH v3 0/4] PCI: get DMA configuration from parent device

[PATCH v3 0/4] PCI: get DMA configuration from parent device

[Murali Karicheri]

PCI devices on Keystone doesn't have correct dma_pfn_offset set. This patch
add capability to set the dma configuration such as dma-mask, dma_pfn_offset,
and dma ops etc using the information from DT. The prior RFCs and discussions
are available at [1] and [2] below.

[2] : https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg790244.html
[1] : http://www.gossamer-threads.com/lists/linux/kernel/2024591

Change history:
	v3 - addressed comments to re-use of_dma_configure() for PCI
	   - To help re-use, change of_iommu_configure() function argument
		- Move of_dma_configure to of/device.c
		- Limit the of_iommu_configure to non pci devices
	v2 - update size to coherent_dma_mask + 1 if dma-range info is missing
	   - also check the np for null.
	v1 - updates based on the comments against initial RFC.
	   - Added a helper function to get the OF node of the parent
	   - Added an API in of_pci.c to update DMA configuration of the pci
	     device.

Murali Karicheri (4):
  of: iommu: add ptr to OF node arg to of_iommu_configure()
  of: move of_dma_configure() to device,c to help re-use
  of/pci: add of_pci_dma_configure() update dma configuration
  PCI: update dma configuration from DT

 drivers/iommu/of_iommu.c  |   10 ++++++--
 drivers/of/device.c       |   58 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/of/of_pci.c       |   39 ++++++++++++++++++++++++++++++
 drivers/of/platform.c     |   58 ++-------------------------------------------
 drivers/pci/probe.c       |    2 ++
 include/linux/of_device.h |    2 ++
 include/linux/of_iommu.h  |    6 +++--
 include/linux/of_pci.h    |   12 ++++++++++
 8 files changed, 127 insertions(+), 60 deletions(-)

-- 
1.7.9.5

[PATCH v3 1/4] of: iommu: add ptr to OF node arg to of_iommu_configure()

[Murali Karicheri]

Function of_iommu_configure() is called from of_dma_configure() to
setup iommu ops using DT property. This API is currently used for
platform devices for which DMA configuration (including iommu ops)
may come from device's parent. To extend this functionality for PCI
devices, this API need to take a parent node ptr as an argument
instead of assuming device's parent. This is needed since for PCI, the
dma configuration may be defined in the DT node of the root bus bridge's
parent device. Currently only dma-range is used for PCI and iommu is not
supported. So return error if the device is PCI.

Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
---
 drivers/iommu/of_iommu.c |   10 ++++++++--
 drivers/of/platform.c    |    2 +-
 include/linux/of_iommu.h |    6 ++++--
 3 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index af1dc6a..396bc77 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -133,19 +133,25 @@ struct iommu_ops *of_iommu_get_ops(struct device_node *np)
 	return ops;
 }
 
-struct iommu_ops *of_iommu_configure(struct device *dev)
+struct iommu_ops *of_iommu_configure(struct device *dev,
+				     struct device_node *node)
 {
 	struct of_phandle_args iommu_spec;
 	struct device_node *np;
 	struct iommu_ops *ops = NULL;
 	int idx = 0;
 
+	if (dev_is_pci(dev)) {
+		dev_err(dev, "iommu is currently not supported for PCI\n");
+		return NULL;
+	}
+
 	/*
 	 * We don't currently walk up the tree looking for a parent IOMMU.
 	 * See the `Notes:' section of
 	 * Documentation/devicetree/bindings/iommu/iommu.txt
 	 */
-	while (!of_parse_phandle_with_args(dev->of_node, "iommus",
+	while (!of_parse_phandle_with_args(node, "iommus",
 					   "#iommu-cells", idx,
 					   &iommu_spec)) {
 		np = iommu_spec.np;
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index a54ec10..7675b79 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -196,7 +196,7 @@ static void of_dma_configure(struct device *dev)
 	dev_dbg(dev, "device is%sdma coherent\n",
 		coherent ? " " : " not ");
 
-	iommu = of_iommu_configure(dev);
+	iommu = of_iommu_configure(dev, dev->of_node);
 	dev_dbg(dev, "device is%sbehind an iommu\n",
 		iommu ? " " : " not ");
 
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index 16c7554..3258cbb 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -12,7 +12,8 @@ extern int of_get_dma_window(struct device_node *dn, const char *prefix,
 			     size_t *size);
 
 extern void of_iommu_init(void);
-extern struct iommu_ops *of_iommu_configure(struct device *dev);
+extern struct iommu_ops *of_iommu_configure(struct device *dev,
+					struct device_node *np);
 
 #else
 
@@ -24,7 +25,8 @@ static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
 }
 
 static inline void of_iommu_init(void) { }
-static inline struct iommu_ops *of_iommu_configure(struct device *dev)
+static inline struct iommu_ops *of_iommu_configure(struct device *dev,
+					 struct device_node *np)
 {
 	return NULL;
 }
-- 
1.7.9.5

[PATCH v3 2/4] of: move of_dma_configure() to device,c to help re-use

[Murali Karicheri]

Move of_dma_configure() to device.c so that same function can be re-used
for PCI devices to obtain DMA configuration from DT. Also add a second
argument so that for PCI, DT node of root bus host bridge can be used to
obtain the DMA configuration for the slave PCI device. Additionally fix
the dma-range size when the DT attribute is missing. i.e  set size to
dev->coherent_dma_mask + 1 instead of dev->coherent_dma_mask.

Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
---
 drivers/of/device.c       |   58 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/of/platform.c     |   58 ++-------------------------------------------
 include/linux/of_device.h |    2 ++
 3 files changed, 62 insertions(+), 56 deletions(-)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 46d6c75c..ccbc958 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -2,6 +2,9 @@
 #include <linux/kernel.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/of_address.h>
+#include <linux/of_iommu.h>
+#include <linux/dma-mapping.h>
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
@@ -66,6 +69,61 @@ int of_device_add(struct platform_device *ofdev)
 	return device_add(&ofdev->dev);
 }
 
+/**
+ * of_dma_configure - Setup DMA configuration
+ * @dev:	Device to apply DMA configuration
+ * @np:		ptr to of node having dma configuration
+ *
+ * Try to get devices's DMA configuration from DT and update it
+ * accordingly.
+ *
+ * In case if platform code need to use own special DMA configuration,it
+ * can use Platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE event
+ * to fix up DMA configuration.
+ */
+void of_dma_configure(struct device *dev, struct device_node *np)
+{
+	u64 dma_addr, paddr, size;
+	int ret;
+	bool coherent;
+	unsigned long offset;
+	struct iommu_ops *iommu;
+
+	/*
+	 * Set default dma-mask to 32 bit. Drivers are expected to setup
+	 * the correct supported dma_mask.
+	 */
+	dev->coherent_dma_mask = DMA_BIT_MASK(32);
+
+	/*
+	 * Set it to coherent_dma_mask by default if the architecture
+	 * code has not set it.
+	 */
+	if (!dev->dma_mask)
+		dev->dma_mask = &dev->coherent_dma_mask;
+
+	ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
+	if (ret < 0) {
+		dma_addr = offset = 0;
+		size = dev->coherent_dma_mask + 1;
+	} else {
+		offset = PFN_DOWN(paddr - dma_addr);
+		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
+	}
+	dev->dma_pfn_offset = offset;
+
+	coherent = of_dma_is_coherent(np);
+	dev_dbg(dev, "device is%sdma coherent\n",
+		coherent ? " " : " not ");
+
+	iommu = of_iommu_configure(dev, np);
+	dev_dbg(dev, "device is%sbehind an iommu\n",
+		iommu ? " " : " not ");
+
+	arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
+}
+EXPORT_SYMBOL_GPL(of_dma_configure);
+
 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 7675b79..6403501 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -19,7 +19,6 @@
 #include <linux/slab.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
-#include <linux/of_iommu.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
@@ -150,59 +149,6 @@ struct platform_device *of_device_alloc(struct device_node *np,
 }
 EXPORT_SYMBOL(of_device_alloc);
 
-/**
- * of_dma_configure - Setup DMA configuration
- * @dev:	Device to apply DMA configuration
- *
- * Try to get devices's DMA configuration from DT and update it
- * accordingly.
- *
- * In case if platform code need to use own special DMA configuration,it
- * can use Platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE event
- * to fix up DMA configuration.
- */
-static void of_dma_configure(struct device *dev)
-{
-	u64 dma_addr, paddr, size;
-	int ret;
-	bool coherent;
-	unsigned long offset;
-	struct iommu_ops *iommu;
-
-	/*
-	 * Set default dma-mask to 32 bit. Drivers are expected to setup
-	 * the correct supported dma_mask.
-	 */
-	dev->coherent_dma_mask = DMA_BIT_MASK(32);
-
-	/*
-	 * Set it to coherent_dma_mask by default if the architecture
-	 * code has not set it.
-	 */
-	if (!dev->dma_mask)
-		dev->dma_mask = &dev->coherent_dma_mask;
-
-	ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size);
-	if (ret < 0) {
-		dma_addr = offset = 0;
-		size = dev->coherent_dma_mask;
-	} else {
-		offset = PFN_DOWN(paddr - dma_addr);
-		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
-	}
-	dev->dma_pfn_offset = offset;
-
-	coherent = of_dma_is_coherent(dev->of_node);
-	dev_dbg(dev, "device is%sdma coherent\n",
-		coherent ? " " : " not ");
-
-	iommu = of_iommu_configure(dev, dev->of_node);
-	dev_dbg(dev, "device is%sbehind an iommu\n",
-		iommu ? " " : " not ");
-
-	arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
-}
-
 static void of_dma_deconfigure(struct device *dev)
 {
 	arch_teardown_dma_ops(dev);
@@ -236,7 +182,7 @@ 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);
+	of_dma_configure(&dev->dev, dev->dev.of_node);
 
 	if (of_device_add(dev) != 0) {
 		of_dma_deconfigure(&dev->dev);
@@ -299,7 +245,7 @@ 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);
+	of_dma_configure(&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/include/linux/of_device.h b/include/linux/of_device.h
index ef37021..c661496 100644
--- a/include/linux/of_device.h
+++ b/include/linux/of_device.h
@@ -53,6 +53,7 @@ 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);
 #else /* CONFIG_OF */
 
 static inline int of_driver_match_device(struct device *dev,
@@ -90,6 +91,7 @@ static inline struct device_node *of_cpu_device_node_get(int cpu)
 {
 	return NULL;
 }
+void of_dma_configure(struct device *dev, struct device_node *np) { }
 #endif /* CONFIG_OF */
 
 #endif /* _LINUX_OF_DEVICE_H */
-- 
1.7.9.5

[PATCH v3 3/4] of/pci: add of_pci_dma_configure() update dma configuration

[Murali Karicheri]

Add of_pci_dma_configure() to allow updating the dma configuration
of the pci device using the configuration from DT of the parent of
the root bridge device.

Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
---
 drivers/of/of_pci.c    |   39 +++++++++++++++++++++++++++++++++++++++
 include/linux/of_pci.h |   12 ++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
index 88471d3..34878c9 100644
--- a/drivers/of/of_pci.c
+++ b/drivers/of/of_pci.c
@@ -2,6 +2,7 @@
 #include <linux/export.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_device.h>
 #include <linux/of_pci.h>
 #include <linux/slab.h>
 
@@ -229,6 +230,44 @@ parse_failed:
 	return err;
 }
 EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources);
+
+/**
+ * of_get_pci_root_bridge_parent - get the OF node of the root bridge's parent
+ * @dev: ptr to pci_dev struct of the pci device
+ *
+ * This function will traverse the bus up to the root bus starting with
+ * the child and return the OF node ptr to root bridge device's parent device.
+ */
+struct device_node *of_get_pci_root_bridge_parent(struct pci_dev *dev)
+{
+	struct pci_bus *bus = dev->bus;
+	struct device *bridge;
+
+	while (!pci_is_root_bus(bus))
+		bus = bus->parent;
+	bridge = bus->bridge;
+
+	return bridge->parent->of_node;
+}
+EXPORT_SYMBOL_GPL(of_get_pci_root_bridge_parent);
+
+/**
+ * of_pci_dma_configure - Setup DMA configuration
+ * @dev: ptr to pci_dev struct of the pci device
+ *
+ * Function to update PCI devices's DMA configuration using the same
+ * info from the OF node of root host bridge's parent.
+ */
+void of_pci_dma_configure(struct pci_dev *pci_dev)
+{
+	struct device *dev = &pci_dev->dev;
+	struct device_node *parent_np;
+
+	parent_np = of_get_pci_root_bridge_parent(pci_dev);
+	of_dma_configure(dev, parent_np);
+}
+EXPORT_SYMBOL_GPL(of_pci_dma_configure);
+
 #endif /* CONFIG_OF_ADDRESS */
 
 #ifdef CONFIG_PCI_MSI
diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
index ce0e5ab..0465a2a 100644
--- a/include/linux/of_pci.h
+++ b/include/linux/of_pci.h
@@ -16,6 +16,8 @@ int of_pci_get_devfn(struct device_node *np);
 int of_irq_parse_and_map_pci(const struct pci_dev *dev, u8 slot, u8 pin);
 int of_pci_parse_bus_range(struct device_node *node, struct resource *res);
 int of_get_pci_domain_nr(struct device_node *node);
+struct device_node *of_get_pci_root_bridge_parent(struct pci_dev *dev);
+void of_pci_dma_configure(struct pci_dev *pci_dev);
 #else
 static inline int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq)
 {
@@ -50,6 +52,16 @@ of_get_pci_domain_nr(struct device_node *node)
 {
 	return -1;
 }
+
+static inline struct device_node
+*of_get_pci_root_bridge_parent(struct pci_dev *dev)
+{
+	return NULL;
+}
+
+static inline void of_pci_dma_configure(struct pci_dev *pci_dev)
+{
+}
 #endif
 
 #if defined(CONFIG_OF_ADDRESS)
-- 
1.7.9.5

[PATCH v3 4/4] PCI: update dma configuration from DT

[Murali Karicheri]

If there is a DT node available for the root bridge's parent device,
use the dma configuration from that device node. For example, keystone
PCI devices would require dma_pfn_offset to be set correctly in the
device structure of the pci device in order to have the correct dma mask.
The DT node will have dma-ranges defined for this. Also support using
the DT property dma-coherent to allow coherent DMA operation by the
PCI device.

This patch use the new helper function of_pci_dma_configure() to update
the device dma configuration.

Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
---
 drivers/pci/probe.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 23212f8..d7dcd6c 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -6,6 +6,7 @@
 #include <linux/delay.h>
 #include <linux/init.h>
 #include <linux/pci.h>
+#include <linux/of_pci.h>
 #include <linux/pci_hotplug.h>
 #include <linux/slab.h>
 #include <linux/module.h>
@@ -1520,6 +1521,7 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 	dev->dev.dma_mask = &dev->dma_mask;
 	dev->dev.dma_parms = &dev->dma_parms;
 	dev->dev.coherent_dma_mask = 0xffffffffull;
+	of_pci_dma_configure(dev);
 
 	pci_set_dma_max_seg_size(dev, 65536);
 	pci_set_dma_seg_boundary(dev, 0xffffffff);
-- 
1.7.9.5

Re: [PATCH v3 0/4] PCI: get DMA configuration from parent device

[Arnd Bergmann]

On Wednesday 07 January 2015 13:49:50 Murali Karicheri wrote:
> PCI devices on Keystone doesn't have correct dma_pfn_offset set. This patch
> add capability to set the dma configuration such as dma-mask, dma_pfn_offset,
> and dma ops etc using the information from DT. The prior RFCs and discussions
> are available at [1] and [2] below.
> 
> [2] : https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg790244.html
> [1] : http://www.gossamer-threads.com/lists/linux/kernel/2024591

Looks all good to me in this version, I'm just unsure about one thing:

>                - Limit the of_iommu_configure to non pci devices

My last recommendation was to pass the b/d/f number into
of_pci_dma_configure to handle this correctly. What was your
reason for not doing it in the end?

We will likely have to change this again if we take your current version,
but that can be a follow-up patch. I believe AMD requires it to use
PCI on their their Seattle platform.

	Arnd

Re: [PATCH v3 0/4] PCI: get DMA configuration from parent device

[Murali Karicheri]

On 01/07/2015 04:18 PM, Arnd Bergmann wrote:
> On Wednesday 07 January 2015 13:49:50 Murali Karicheri wrote:
>> PCI devices on Keystone doesn't have correct dma_pfn_offset set. This patch
>> add capability to set the dma configuration such as dma-mask, dma_pfn_offset,
>> and dma ops etc using the information from DT. The prior RFCs and discussions
>> are available at [1] and [2] below.
>>
>> [2] : https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg790244.html
>> [1] : http://www.gossamer-threads.com/lists/linux/kernel/2024591
>
> Looks all good to me in this version, I'm just unsure about one thing:
>
>>                 - Limit the of_iommu_configure to non pci devices
>
> My last recommendation was to pass the b/d/f number into
> of_pci_dma_configure to handle this correctly. What was your
> reason for not doing it in the end?
Arnd,

I had responded to this already on the list and reproduced below your 
remark and my response below from that thread.

---------------cut-------------------------------------------------------
 > Actually regarding the bit I wrote above, it might be helpful to pass
 > the PCI_DEVID() into both of_iommu_configure and of_dma_configure.
 >
 > While this may or may not be sufficient, I think there is no question
 > about it being needed for the ARM SMMU with PCI, so we may as well add
 > it at the point when you touch the same lines already. In the platform
 > bus case, just pass zero here.
Arnd,

PCI_DEVID() is defined as

#define PCI_DEVID(bus, devfn)  ((((u16)(bus)) << 8) | (devfn))

So PCI_DEVID of 0 is a valid PCI DEVID.

How about checking if the device is PCI in of_iommu_configure() using 
dev_is_pci(dev) macro and return immediately for PCI? Need to include 
pci.h in this file though.

Some of the iommu drivers already include this.

a0868495@ares-ubuntu:~/projects/linux-keystone$ grep -r pci.h drivers/iommu/
drivers/iommu/amd_iommu_v2.c:#include <linux/pci.h>
drivers/iommu/dmar.c:#include <linux/pci.h>
drivers/iommu/amd_iommu_types.h:#include <linux/pci.h>
drivers/iommu/amd_iommu.c:#include <linux/pci.h>
drivers/iommu/fsl_pamu_domain.c:#include <sysdev/fsl_pci.h>
drivers/iommu/iommu.c:#include <linux/pci.h>
drivers/iommu/intel-iommu.c:#include <linux/pci.h>
drivers/iommu/intel_irq_remapping.c:#include <linux/pci.h>
drivers/iommu/arm-smmu.c:#include <linux/pci.h>
drivers/iommu/amd_iommu_init.c:#include <linux/pci.h>
drivers/iommu/irq_remapping.c:#include <linux/pci.h>

This will allow us to re-visit this later for IOMMU support for PCI 
without polluting the API.
-----------------------cut-end---------------------------------------

I assumed you want to use value of zero for b/d/f to indicate it is for
platform. Also use the non zero value to skip the DT attribute parsing 
for IOMMU DT attribute in of_iommu_configure() for PCI.
I did dev_is_pci() for skipping the processing for PCI. I thought it is 
better to add the b/d/f argument later when this is re-visited later.

BTW,

I still haven't received any comment from Bjorn w.r.t PCI part.

BJorn, could you review and comment on 3/4 and 4/4?

Regards,

Murali
>
> We will likely have to change this again if we take your current version,
> but that can be a follow-up patch. I believe AMD requires it to use
> PCI on their their Seattle platform.
>
> 	Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Murali Karicheri
Linux Kernel, Texas Instruments

Re: [PATCH v3 0/4] PCI: get DMA configuration from parent device

[Murali Karicheri]

On 01/07/2015 01:49 PM, Murali Karicheri wrote:
> PCI devices on Keystone doesn't have correct dma_pfn_offset set. This patch
> add capability to set the dma configuration such as dma-mask, dma_pfn_offset,
> and dma ops etc using the information from DT. The prior RFCs and discussions
> are available at [1] and [2] below.
>
> [2] : https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg790244.html
> [1] : http://www.gossamer-threads.com/lists/linux/kernel/2024591
>
> Change history:
> 	v3 - addressed comments to re-use of_dma_configure() for PCI
> 	   - To help re-use, change of_iommu_configure() function argument
> 		- Move of_dma_configure to of/device.c
> 		- Limit the of_iommu_configure to non pci devices
> 	v2 - update size to coherent_dma_mask + 1 if dma-range info is missing
> 	   - also check the np for null.
> 	v1 - updates based on the comments against initial RFC.
> 	   - Added a helper function to get the OF node of the parent
> 	   - Added an API in of_pci.c to update DMA configuration of the pci
> 	     device.
>
> Murali Karicheri (4):
>    of: iommu: add ptr to OF node arg to of_iommu_configure()
>    of: move of_dma_configure() to device,c to help re-use
>    of/pci: add of_pci_dma_configure() update dma configuration
>    PCI: update dma configuration from DT
>
>   drivers/iommu/of_iommu.c  |   10 ++++++--
>   drivers/of/device.c       |   58 +++++++++++++++++++++++++++++++++++++++++++++
>   drivers/of/of_pci.c       |   39 ++++++++++++++++++++++++++++++
>   drivers/of/platform.c     |   58 ++-------------------------------------------
>   drivers/pci/probe.c       |    2 ++
>   include/linux/of_device.h |    2 ++
>   include/linux/of_iommu.h  |    6 +++--
>   include/linux/of_pci.h    |   12 ++++++++++
>   8 files changed, 127 insertions(+), 60 deletions(-)
>
Bjorn,

Could you review this series from the PCI subsystem point of view?

Thanks
-- 
Murali Karicheri
Linux Kernel, Texas Instruments

Re: [PATCH v3 0/4] PCI: get DMA configuration from parent device

[Bjorn Helgaas]

On Wed, Jan 7, 2015 at 5:05 PM, Murali Karicheri <m-karicheri2@ti.com> wrote:
> On 01/07/2015 01:49 PM, Murali Karicheri wrote:
>>
>> PCI devices on Keystone doesn't have correct dma_pfn_offset set. This
>> patch
>> add capability to set the dma configuration such as dma-mask,
>> dma_pfn_offset,
>> and dma ops etc using the information from DT. The prior RFCs and
>> discussions
>> are available at [1] and [2] below.
>>
>> [2] :
>> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg790244.html
>> [1] : http://www.gossamer-threads.com/lists/linux/kernel/2024591
>>
>> Change history:
>>         v3 - addressed comments to re-use of_dma_configure() for PCI
>>            - To help re-use, change of_iommu_configure() function argument
>>                 - Move of_dma_configure to of/device.c
>>                 - Limit the of_iommu_configure to non pci devices
>>         v2 - update size to coherent_dma_mask + 1 if dma-range info is
>> missing
>>            - also check the np for null.
>>         v1 - updates based on the comments against initial RFC.
>>            - Added a helper function to get the OF node of the parent
>>            - Added an API in of_pci.c to update DMA configuration of the
>> pci
>>              device.
>>
>> Murali Karicheri (4):
>>    of: iommu: add ptr to OF node arg to of_iommu_configure()
>>    of: move of_dma_configure() to device,c to help re-use
>>    of/pci: add of_pci_dma_configure() update dma configuration
>>    PCI: update dma configuration from DT
>>
>>   drivers/iommu/of_iommu.c  |   10 ++++++--
>>   drivers/of/device.c       |   58
>> +++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/of/of_pci.c       |   39 ++++++++++++++++++++++++++++++
>>   drivers/of/platform.c     |   58
>> ++-------------------------------------------
>>   drivers/pci/probe.c       |    2 ++
>>   include/linux/of_device.h |    2 ++
>>   include/linux/of_iommu.h  |    6 +++--
>>   include/linux/of_pci.h    |   12 ++++++++++
>>   8 files changed, 127 insertions(+), 60 deletions(-)
>>
> Bjorn,
>
> Could you review this series from the PCI subsystem point of view?

Will do, it's in my patchwork queue.  I'm just returning after a
couple weeks of vacation, during which I moved across the country, so
it will take me a bit to catch up.

Bjorn

Re: [PATCH v3 1/4] of: iommu: add ptr to OF node arg to of_iommu_configure()

[Rob Herring]

On Wed, Jan 7, 2015 at 12:49 PM, Murali Karicheri <m-karicheri2@ti.com> wrote:
> Function of_iommu_configure() is called from of_dma_configure() to
> setup iommu ops using DT property. This API is currently used for
> platform devices for which DMA configuration (including iommu ops)
> may come from device's parent. To extend this functionality for PCI
> devices, this API need to take a parent node ptr as an argument
> instead of assuming device's parent. This is needed since for PCI, the
> dma configuration may be defined in the DT node of the root bus bridge's
> parent device. Currently only dma-range is used for PCI and iommu is not
> supported. So return error if the device is PCI.
>
> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> ---
>  drivers/iommu/of_iommu.c |   10 ++++++++--
>  drivers/of/platform.c    |    2 +-
>  include/linux/of_iommu.h |    6 ++++--
>  3 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index af1dc6a..396bc77 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -133,19 +133,25 @@ struct iommu_ops *of_iommu_get_ops(struct device_node *np)
>         return ops;
>  }
>
> -struct iommu_ops *of_iommu_configure(struct device *dev)
> +struct iommu_ops *of_iommu_configure(struct device *dev,
> +                                    struct device_node *node)

You use node here, but np in the declaration. However...

>  {
>         struct of_phandle_args iommu_spec;
>         struct device_node *np;

We also have np here. I would suggest we rename this to iommu_node or
iommu_np to be clear which one this is for.

>         struct iommu_ops *ops = NULL;
>         int idx = 0;
>
> +       if (dev_is_pci(dev)) {
> +               dev_err(dev, "iommu is currently not supported for PCI\n");
> +               return NULL;
> +       }
> +
>         /*
>          * We don't currently walk up the tree looking for a parent IOMMU.
>          * See the `Notes:' section of
>          * Documentation/devicetree/bindings/iommu/iommu.txt
>          */
> -       while (!of_parse_phandle_with_args(dev->of_node, "iommus",
> +       while (!of_parse_phandle_with_args(node, "iommus",
>                                            "#iommu-cells", idx,
>                                            &iommu_spec)) {
>                 np = iommu_spec.np;
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index a54ec10..7675b79 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -196,7 +196,7 @@ static void of_dma_configure(struct device *dev)
>         dev_dbg(dev, "device is%sdma coherent\n",
>                 coherent ? " " : " not ");
>
> -       iommu = of_iommu_configure(dev);
> +       iommu = of_iommu_configure(dev, dev->of_node);
>         dev_dbg(dev, "device is%sbehind an iommu\n",
>                 iommu ? " " : " not ");
>
> diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
> index 16c7554..3258cbb 100644
> --- a/include/linux/of_iommu.h
> +++ b/include/linux/of_iommu.h
> @@ -12,7 +12,8 @@ extern int of_get_dma_window(struct device_node *dn, const char *prefix,
>                              size_t *size);
>
>  extern void of_iommu_init(void);
> -extern struct iommu_ops *of_iommu_configure(struct device *dev);
> +extern struct iommu_ops *of_iommu_configure(struct device *dev,
> +                                       struct device_node *np);
>
>  #else
>
> @@ -24,7 +25,8 @@ static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
>  }
>
>  static inline void of_iommu_init(void) { }
> -static inline struct iommu_ops *of_iommu_configure(struct device *dev)
> +static inline struct iommu_ops *of_iommu_configure(struct device *dev,
> +                                        struct device_node *np)
>  {
>         return NULL;
>  }
> --
> 1.7.9.5
>

Re: [PATCH v3 2/4] of: move of_dma_configure() to device,c to help re-use

[Rob Herring]

On Wed, Jan 7, 2015 at 12:49 PM, Murali Karicheri <m-karicheri2@ti.com> wrote:
> Move of_dma_configure() to device.c so that same function can be re-used
> for PCI devices to obtain DMA configuration from DT. Also add a second
> argument so that for PCI, DT node of root bus host bridge can be used to
> obtain the DMA configuration for the slave PCI device. Additionally fix
> the dma-range size when the DT attribute is missing. i.e  set size to
> dev->coherent_dma_mask + 1 instead of dev->coherent_dma_mask.

Additionally is a red flag for put in another patch. There is an issue
I think as well.

> +       ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
> +       if (ret < 0) {
> +               dma_addr = offset = 0;
> +               size = dev->coherent_dma_mask + 1;

If coherent_dma_mask is DMA_BIT_MASK(64), then you will overflow and
have a size of 0. There may also be a problem when the mask is only
32-bit type.

Rob

Re: [PATCH v3 2/4] of: move of_dma_configure() to device,c to help re-use

[Arnd Bergmann]

On Wednesday 07 January 2015 17:37:56 Rob Herring wrote:
> On Wed, Jan 7, 2015 at 12:49 PM, Murali Karicheri <m-karicheri2@ti.com> wrote:
> 
> > +       ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
> > +       if (ret < 0) {
> > +               dma_addr = offset = 0;
> > +               size = dev->coherent_dma_mask + 1;
> 
> If coherent_dma_mask is DMA_BIT_MASK(64), then you will overflow and
> have a size of 0. There may also be a problem when the mask is only
> 32-bit type.

The mask is always a 64-bit type, it's not optional. But you are right,
the 64-bit mask case is broken, so I guess we have to fix it differently
by always passing the smaller value into arch_setup_dma_ops and
adapting that function instead.

	Arnd

Re: [PATCH v3 0/4] PCI: get DMA configuration from parent device

[Arnd Bergmann]

On Wednesday 07 January 2015 18:04:41 Murali Karicheri wrote:
> On 01/07/2015 04:18 PM, Arnd Bergmann wrote:
> > On Wednesday 07 January 2015 13:49:50 Murali Karicheri wrote:
> >> PCI devices on Keystone doesn't have correct dma_pfn_offset set. This patch
> >> add capability to set the dma configuration such as dma-mask, dma_pfn_offset,
> >> and dma ops etc using the information from DT. The prior RFCs and discussions
> >> are available at [1] and [2] below.
> >>
> >> [2] : https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg790244.html
> >> [1] : http://www.gossamer-threads.com/lists/linux/kernel/2024591
> >
> > Looks all good to me in this version, I'm just unsure about one thing:
> >
> >>                 - Limit the of_iommu_configure to non pci devices
> >
> > My last recommendation was to pass the b/d/f number into
> > of_pci_dma_configure to handle this correctly. What was your
> > reason for not doing it in the end?
> Arnd,
> 
> I had responded to this already on the list and reproduced below your 
> remark and my response below from that thread.

Ah right, sorry for missing a reply on that.

> ---------------cut-------------------------------------------------------
>  > Actually regarding the bit I wrote above, it might be helpful to pass
>  > the PCI_DEVID() into both of_iommu_configure and of_dma_configure.
>  >
>  > While this may or may not be sufficient, I think there is no question
>  > about it being needed for the ARM SMMU with PCI, so we may as well add
>  > it at the point when you touch the same lines already. In the platform
>  > bus case, just pass zero here.
> 
> PCI_DEVID() is defined as
> 
> #define PCI_DEVID(bus, devfn)  ((((u16)(bus)) << 8) | (devfn))
> 
> So PCI_DEVID of 0 is a valid PCI DEVID.

I think that is ok: the idea would be that we always just list the
first ID of the range and then let the iommu driver add the offset
in the appropriate way.

> How about checking if the device is PCI in of_iommu_configure() using 
> dev_is_pci(dev) macro and return immediately for PCI? Need to include 
> pci.h in this file though.
> 
> Some of the iommu drivers already include this.

I guess you are right, but the driver can even handle the PCI
device correctly without the extra parameter:

	devid = 0;
	if (dev_is_pci(dev)) {
		struct pci_dev *pdev = to_pci_dev(dev);
		devid = PCI_DEVID(pdev->bus->number, pdev->devfn);
	}

	streamid += devid;

Other IOMMUs won't even care about the devid, so they will just work.

> This will allow us to re-visit this later for IOMMU support for PCI 
> without polluting the API.
>
> -----------------------cut-end---------------------------------------
> 
> I assumed you want to use value of zero for b/d/f to indicate it is for
> platform. Also use the non zero value to skip the DT attribute parsing 
> for IOMMU DT attribute in of_iommu_configure() for PCI.
> I did dev_is_pci() for skipping the processing for PCI. I thought it is 
> better to add the b/d/f argument later when this is re-visited later.

I'd say just keep the iommu setup for PCI now and let drivers handle
this under the covers.

There is another interesting case, which is a USB host controller or
something similar behind a PCI bus. These are quite common and also
need to be handled in some form. Let's do just PCI first for now, but
be aware that this will come next. Should we assume that the ID that
is required for a device is either known from the device node, or
that it comes from a PCI device? That means for the USB case, we will
likely need to have some custom logic. There seems to be an implicit
assumption all over the kernel that all devices have the same IOMMU
instance, but we can't really rely on that here.

	Arnd

Re: [PATCH v3 0/4] PCI: get DMA configuration from parent device

[Murali Karicheri]

On 01/07/2015 06:08 PM, Bjorn Helgaas wrote:
> On Wed, Jan 7, 2015 at 5:05 PM, Murali Karicheri<m-karicheri2@ti.com>  wrote:
>> On 01/07/2015 01:49 PM, Murali Karicheri wrote:
>>>
>>> PCI devices on Keystone doesn't have correct dma_pfn_offset set. This
>>> patch
>>> add capability to set the dma configuration such as dma-mask,
>>> dma_pfn_offset,
>>> and dma ops etc using the information from DT. The prior RFCs and
>>> discussions
>>> are available at [1] and [2] below.
>>>
>>> [2] :
>>> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg790244.html
>>> [1] : http://www.gossamer-threads.com/lists/linux/kernel/2024591
>>>
>>> Change history:
>>>          v3 - addressed comments to re-use of_dma_configure() for PCI
>>>             - To help re-use, change of_iommu_configure() function argument
>>>                  - Move of_dma_configure to of/device.c
>>>                  - Limit the of_iommu_configure to non pci devices
>>>          v2 - update size to coherent_dma_mask + 1 if dma-range info is
>>> missing
>>>             - also check the np for null.
>>>          v1 - updates based on the comments against initial RFC.
>>>             - Added a helper function to get the OF node of the parent
>>>             - Added an API in of_pci.c to update DMA configuration of the
>>> pci
>>>               device.
>>>
>>> Murali Karicheri (4):
>>>     of: iommu: add ptr to OF node arg to of_iommu_configure()
>>>     of: move of_dma_configure() to device,c to help re-use
>>>     of/pci: add of_pci_dma_configure() update dma configuration
>>>     PCI: update dma configuration from DT
>>>
>>>    drivers/iommu/of_iommu.c  |   10 ++++++--
>>>    drivers/of/device.c       |   58
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>    drivers/of/of_pci.c       |   39 ++++++++++++++++++++++++++++++
>>>    drivers/of/platform.c     |   58
>>> ++-------------------------------------------
>>>    drivers/pci/probe.c       |    2 ++
>>>    include/linux/of_device.h |    2 ++
>>>    include/linux/of_iommu.h  |    6 +++--
>>>    include/linux/of_pci.h    |   12 ++++++++++
>>>    8 files changed, 127 insertions(+), 60 deletions(-)
>>>
>> Bjorn,
>>
>> Could you review this series from the PCI subsystem point of view?
>
> Will do, it's in my patchwork queue.  I'm just returning after a
> couple weeks of vacation, during which I moved across the country, so
> it will take me a bit to catch up.
>
> Bjorn
Bjorn,

Thanks and Happy New year!

-- 
Murali Karicheri
Linux Kernel, Texas Instruments

Re: [PATCH v3 0/4] PCI: get DMA configuration from parent device

[Will Deacon]

On Thu, Jan 08, 2015 at 08:56:39AM +0000, Arnd Bergmann wrote:
> On Wednesday 07 January 2015 18:04:41 Murali Karicheri wrote:
> > On 01/07/2015 04:18 PM, Arnd Bergmann wrote:
> > > On Wednesday 07 January 2015 13:49:50 Murali Karicheri wrote:
> > >> PCI devices on Keystone doesn't have correct dma_pfn_offset set. This patch
> > >> add capability to set the dma configuration such as dma-mask, dma_pfn_offset,
> > >> and dma ops etc using the information from DT. The prior RFCs and discussions
> > >> are available at [1] and [2] below.
> > >>
> > >> [2] : https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg790244.html
> > >> [1] : http://www.gossamer-threads.com/lists/linux/kernel/2024591
> > >
> > > Looks all good to me in this version, I'm just unsure about one thing:
> > >
> > >>                 - Limit the of_iommu_configure to non pci devices
> > >
> > > My last recommendation was to pass the b/d/f number into
> > > of_pci_dma_configure to handle this correctly. What was your
> > > reason for not doing it in the end?
> > Arnd,
> > 
> > I had responded to this already on the list and reproduced below your 
> > remark and my response below from that thread.
> 
> Ah right, sorry for missing a reply on that.
> 
> > ---------------cut-------------------------------------------------------
> >  > Actually regarding the bit I wrote above, it might be helpful to pass
> >  > the PCI_DEVID() into both of_iommu_configure and of_dma_configure.
> >  >
> >  > While this may or may not be sufficient, I think there is no question
> >  > about it being needed for the ARM SMMU with PCI, so we may as well add
> >  > it at the point when you touch the same lines already. In the platform
> >  > bus case, just pass zero here.
> > 
> > PCI_DEVID() is defined as
> > 
> > #define PCI_DEVID(bus, devfn)  ((((u16)(bus)) << 8) | (devfn))
> > 
> > So PCI_DEVID of 0 is a valid PCI DEVID.
> 
> I think that is ok: the idea would be that we always just list the
> first ID of the range and then let the iommu driver add the offset
> in the appropriate way.
> 
> > How about checking if the device is PCI in of_iommu_configure() using 
> > dev_is_pci(dev) macro and return immediately for PCI? Need to include 
> > pci.h in this file though.
> > 
> > Some of the iommu drivers already include this.
> 
> I guess you are right, but the driver can even handle the PCI
> device correctly without the extra parameter:
> 
> 	devid = 0;
> 	if (dev_is_pci(dev)) {
> 		struct pci_dev *pdev = to_pci_dev(dev);
> 		devid = PCI_DEVID(pdev->bus->number, pdev->devfn);
> 	}
> 
> 	streamid += devid;
> 
> Other IOMMUs won't even care about the devid, so they will just work.

I started looking at this in more depth for the ARM SMMU at the end of last
year and the PCI case ends up being remarkably similar to the DT case.

Although I initially thought we just needed the devid, actually we need to
walk the PCI topology to find the dma alias for the device. The current
code for doing that (iommu_group_get_for_pci_dev) only reports the resulting
IOMMu group, which can actually correspond to *multiple* DMA aliases due
to ACS restrictions.

I knocked up a couple of patches for dealing with this in the ARM SMMU
driver, but I'd really like that to be part of core code if possible :)

https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/commit/?h=iommu/pgtbl&id=41dc7b9e0ff1a4d79d1dd76619056fc0cfa8b84f
https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/commit/?h=iommu/pgtbl&id=5503996103138b41520d5eae90dda62abb65f04f

So, I think of_pci_dma_configure should determine the set of aliases for the
group. What isn't clear to me for the DT *or* the PCI cases is what we
actually do with the group at the dma-mapping level...

Will

Re: [PATCH v3 3/4] of/pci: add of_pci_dma_configure() update dma configuration

[Will Deacon]

On Wed, Jan 07, 2015 at 06:49:53PM +0000, Murali Karicheri wrote:
> Add of_pci_dma_configure() to allow updating the dma configuration
> of the pci device using the configuration from DT of the parent of
> the root bridge device.
> 
> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> ---
>  drivers/of/of_pci.c    |   39 +++++++++++++++++++++++++++++++++++++++
>  include/linux/of_pci.h |   12 ++++++++++++
>  2 files changed, 51 insertions(+)
> 
> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
> index 88471d3..34878c9 100644
> --- a/drivers/of/of_pci.c
> +++ b/drivers/of/of_pci.c
> @@ -2,6 +2,7 @@
>  #include <linux/export.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
> +#include <linux/of_device.h>
>  #include <linux/of_pci.h>
>  #include <linux/slab.h>
>  
> @@ -229,6 +230,44 @@ parse_failed:
>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources);
> +
> +/**
> + * of_get_pci_root_bridge_parent - get the OF node of the root bridge's parent
> + * @dev: ptr to pci_dev struct of the pci device
> + *
> + * This function will traverse the bus up to the root bus starting with
> + * the child and return the OF node ptr to root bridge device's parent device.
> + */
> +struct device_node *of_get_pci_root_bridge_parent(struct pci_dev *dev)
> +{
> +	struct pci_bus *bus = dev->bus;
> +	struct device *bridge;
> +
> +	while (!pci_is_root_bus(bus))
> +		bus = bus->parent;
> +	bridge = bus->bridge;
> +
> +	return bridge->parent->of_node;
> +}
> +EXPORT_SYMBOL_GPL(of_get_pci_root_bridge_parent);
> +
> +/**
> + * of_pci_dma_configure - Setup DMA configuration
> + * @dev: ptr to pci_dev struct of the pci device
> + *
> + * Function to update PCI devices's DMA configuration using the same
> + * info from the OF node of root host bridge's parent.
> + */
> +void of_pci_dma_configure(struct pci_dev *pci_dev)
> +{
> +	struct device *dev = &pci_dev->dev;
> +	struct device_node *parent_np;
> +
> +	parent_np = of_get_pci_root_bridge_parent(pci_dev);
> +	of_dma_configure(dev, parent_np);
> +}
> +EXPORT_SYMBOL_GPL(of_pci_dma_configure);

Whilst I think this is the right overall structure, I think this function
should determine the set of DMA aliases for the device and pass that through
to the IOMMU (as mentioned in my reply to the cover letter). Then we just
need to work out what we're doing for groups.

Will

Re: [PATCH v3 1/4] of: iommu: add ptr to OF node arg to of_iommu_configure()

[Murali Karicheri]

On 01/07/2015 06:30 PM, Rob Herring wrote:
> On Wed, Jan 7, 2015 at 12:49 PM, Murali Karicheri<m-karicheri2@ti.com>  wrote:
>> Function of_iommu_configure() is called from of_dma_configure() to
>> setup iommu ops using DT property. This API is currently used for
>> platform devices for which DMA configuration (including iommu ops)
>> may come from device's parent. To extend this functionality for PCI
>> devices, this API need to take a parent node ptr as an argument
>> instead of assuming device's parent. This is needed since for PCI, the
>> dma configuration may be defined in the DT node of the root bus bridge's
>> parent device. Currently only dma-range is used for PCI and iommu is not
>> supported. So return error if the device is PCI.
>>
>> Signed-off-by: Murali Karicheri<m-karicheri2@ti.com>
>> ---
>>   drivers/iommu/of_iommu.c |   10 ++++++++--
>>   drivers/of/platform.c    |    2 +-
>>   include/linux/of_iommu.h |    6 ++++--
>>   3 files changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
>> index af1dc6a..396bc77 100644
>> --- a/drivers/iommu/of_iommu.c
>> +++ b/drivers/iommu/of_iommu.c
>> @@ -133,19 +133,25 @@ struct iommu_ops *of_iommu_get_ops(struct device_node *np)
>>          return ops;
>>   }
>>
>> -struct iommu_ops *of_iommu_configure(struct device *dev)
>> +struct iommu_ops *of_iommu_configure(struct device *dev,
>> +                                    struct device_node *node)
>
> You use node here, but np in the declaration. However...
>
>>   {
>>          struct of_phandle_args iommu_spec;
>>          struct device_node *np;
>
> We also have np here. I would suggest we rename this to iommu_node or
> iommu_np to be clear which one this is for.
>
Rob,

Thanks for the comment. Will pick and use iommu_np consistently everywhere.

Murali
>>          struct iommu_ops *ops = NULL;
>>          int idx = 0;
>>
>> +       if (dev_is_pci(dev)) {
>> +               dev_err(dev, "iommu is currently not supported for PCI\n");
>> +               return NULL;
>> +       }
>> +
>>          /*
>>           * We don't currently walk up the tree looking for a parent IOMMU.
>>           * See the `Notes:' section of
>>           * Documentation/devicetree/bindings/iommu/iommu.txt
>>           */
>> -       while (!of_parse_phandle_with_args(dev->of_node, "iommus",
>> +       while (!of_parse_phandle_with_args(node, "iommus",
>>                                             "#iommu-cells", idx,
>>                                             &iommu_spec)) {
>>                  np = iommu_spec.np;
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index a54ec10..7675b79 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -196,7 +196,7 @@ static void of_dma_configure(struct device *dev)
>>          dev_dbg(dev, "device is%sdma coherent\n",
>>                  coherent ? " " : " not ");
>>
>> -       iommu = of_iommu_configure(dev);
>> +       iommu = of_iommu_configure(dev, dev->of_node);
>>          dev_dbg(dev, "device is%sbehind an iommu\n",
>>                  iommu ? " " : " not ");
>>
>> diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
>> index 16c7554..3258cbb 100644
>> --- a/include/linux/of_iommu.h
>> +++ b/include/linux/of_iommu.h
>> @@ -12,7 +12,8 @@ extern int of_get_dma_window(struct device_node *dn, const char *prefix,
>>                               size_t *size);
>>
>>   extern void of_iommu_init(void);
>> -extern struct iommu_ops *of_iommu_configure(struct device *dev);
>> +extern struct iommu_ops *of_iommu_configure(struct device *dev,
>> +                                       struct device_node *np);
>>
>>   #else
>>
>> @@ -24,7 +25,8 @@ static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
>>   }
>>
>>   static inline void of_iommu_init(void) { }
>> -static inline struct iommu_ops *of_iommu_configure(struct device *dev)
>> +static inline struct iommu_ops *of_iommu_configure(struct device *dev,
>> +                                        struct device_node *np)
>>   {
>>          return NULL;
>>   }
>> --
>> 1.7.9.5
>>


-- 
Murali Karicheri
Linux Kernel, Texas Instruments

Re: [PATCH v3 2/4] of: move of_dma_configure() to device,c to help re-use

[Murali Karicheri]

On 01/08/2015 03:40 AM, Arnd Bergmann wrote:
> On Wednesday 07 January 2015 17:37:56 Rob Herring wrote:
>> On Wed, Jan 7, 2015 at 12:49 PM, Murali Karicheri<m-karicheri2@ti.com>  wrote:
>>
>>> +       ret = of_dma_get_range(np,&dma_addr,&paddr,&size);
>>> +       if (ret<  0) {
>>> +               dma_addr = offset = 0;
>>> +               size = dev->coherent_dma_mask + 1;
>>
>> If coherent_dma_mask is DMA_BIT_MASK(64), then you will overflow and
>> have a size of 0. There may also be a problem when the mask is only
>> 32-bit type.
>
> The mask is always a 64-bit type, it's not optional. But you are right,
> the 64-bit mask case is broken, so I guess we have to fix it differently
> by always passing the smaller value into arch_setup_dma_ops and
> adapting that function instead.
Arnd,

What is the smaller value you are referring to in the below code? 
between *dev->dma_mask and size from DT? But overflow can still happen 
when size is to be calculated in arch_setup_dma_ops() for Non DT case or 
when DT size is configured to be equivalent of DMA_BIT_MASK(64) + 1. Can 
we discuss the code change you have in mind when you get a chance?

Murali
>
> 	Arnd


-- 
Murali Karicheri
Linux Kernel, Texas Instruments

Re: [PATCH v3 3/4] of/pci: add of_pci_dma_configure() update dma configuration

[Murali Karicheri]

On 01/08/2015 11:06 AM, Will Deacon wrote:
> On Wed, Jan 07, 2015 at 06:49:53PM +0000, Murali Karicheri wrote:
>> Add of_pci_dma_configure() to allow updating the dma configuration
>> of the pci device using the configuration from DT of the parent of
>> the root bridge device.
>>
>> Signed-off-by: Murali Karicheri<m-karicheri2@ti.com>
>> ---
>>   drivers/of/of_pci.c    |   39 +++++++++++++++++++++++++++++++++++++++
>>   include/linux/of_pci.h |   12 ++++++++++++
>>   2 files changed, 51 insertions(+)
>>
>> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
>> index 88471d3..34878c9 100644
>> --- a/drivers/of/of_pci.c
>> +++ b/drivers/of/of_pci.c
>> @@ -2,6 +2,7 @@
>>   #include<linux/export.h>
>>   #include<linux/of.h>
>>   #include<linux/of_address.h>
>> +#include<linux/of_device.h>
>>   #include<linux/of_pci.h>
>>   #include<linux/slab.h>
>>
>> @@ -229,6 +230,44 @@ parse_failed:
>>   	return err;
>>   }
>>   EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources);
>> +
>> +/**
>> + * of_get_pci_root_bridge_parent - get the OF node of the root bridge's parent
>> + * @dev: ptr to pci_dev struct of the pci device
>> + *
>> + * This function will traverse the bus up to the root bus starting with
>> + * the child and return the OF node ptr to root bridge device's parent device.
>> + */
>> +struct device_node *of_get_pci_root_bridge_parent(struct pci_dev *dev)
>> +{
>> +	struct pci_bus *bus = dev->bus;
>> +	struct device *bridge;
>> +
>> +	while (!pci_is_root_bus(bus))
>> +		bus = bus->parent;
>> +	bridge = bus->bridge;
>> +
>> +	return bridge->parent->of_node;
>> +}
>> +EXPORT_SYMBOL_GPL(of_get_pci_root_bridge_parent);
>> +
>> +/**
>> + * of_pci_dma_configure - Setup DMA configuration
>> + * @dev: ptr to pci_dev struct of the pci device
>> + *
>> + * Function to update PCI devices's DMA configuration using the same
>> + * info from the OF node of root host bridge's parent.
>> + */
>> +void of_pci_dma_configure(struct pci_dev *pci_dev)
>> +{
>> +	struct device *dev =&pci_dev->dev;
>> +	struct device_node *parent_np;
>> +
>> +	parent_np = of_get_pci_root_bridge_parent(pci_dev);
>> +	of_dma_configure(dev, parent_np);
>> +}
>> +EXPORT_SYMBOL_GPL(of_pci_dma_configure);
>
> Whilst I think this is the right overall structure, I think this function
> should determine the set of DMA aliases for the device and pass that through
> to the IOMMU (as mentioned in my reply to the cover letter). Then we just
> need to work out what we're doing for groups.
>
> Will
Will,

Could you add this as as a follow up patch as I don't have a platformm 
that support IOMMU and as such my understanding of the IOMMU is limited?

I can help test the change to make sure it has no side effect on 
Keystone that doesn't support IOMMU.

Thanks.
-- 
Murali Karicheri
Linux Kernel, Texas Instruments

Re: [PATCH v3 2/4] of: move of_dma_configure() to device,c to help re-use

[Arnd Bergmann]

On Thursday 08 January 2015 14:26:36 Murali Karicheri wrote:
> On 01/08/2015 03:40 AM, Arnd Bergmann wrote:
> > On Wednesday 07 January 2015 17:37:56 Rob Herring wrote:
> >> On Wed, Jan 7, 2015 at 12:49 PM, Murali Karicheri<m-karicheri2@ti.com>  wrote:
> >>
> >>> +       ret = of_dma_get_range(np,&dma_addr,&paddr,&size);
> >>> +       if (ret<  0) {
> >>> +               dma_addr = offset = 0;
> >>> +               size = dev->coherent_dma_mask + 1;
> >>
> >> If coherent_dma_mask is DMA_BIT_MASK(64), then you will overflow and
> >> have a size of 0. There may also be a problem when the mask is only
> >> 32-bit type.
> >
> > The mask is always a 64-bit type, it's not optional. But you are right,
> > the 64-bit mask case is broken, so I guess we have to fix it differently
> > by always passing the smaller value into arch_setup_dma_ops and
> > adapting that function instead.
> Arnd,
> 
> What is the smaller value you are referring to in the below code? 
> between *dev->dma_mask and size from DT? But overflow can still happen 
> when size is to be calculated in arch_setup_dma_ops() for Non DT case or 
> when DT size is configured to be equivalent of DMA_BIT_MASK(64) + 1. Can 
> we discuss the code change you have in mind when you get a chance?

I meant changing every function that the size values gets passed into
to take a mask like 0xffffffff instead of a size like 0x100000000, so
we can represent a 64-bit capable bus correctly.

This means we also need to adapt the value returned from of_dma_get_range.
A minor complication here is that the DT properties sometimes already
contain the mask value, in particular when we want to represent a
full mapping like 

	bus {
		#address-cells = <1>;
		#size-cells = <1>;
		dma-ranges = <0 0 0xffffffff>; /* all 4 GB, DMA_BIT_MASK(32) */
	};

as opposed to

	bus {
		#address-cells = <1>;
		#size-cells = <1>;
		dma-ranges = <0 0 0x80000000>; /* only lower 2GB, DMA_BIT_MASK(31) */
	};

or

	bus {
		#address-cells = <2>;
		#size-cells = <2>;
		dma-ranges = <0 0 0x0000000100000000>; /* 4GB of 64-bit address space */
	};


	Arnd

Re: [PATCH v3 3/4] of/pci: add of_pci_dma_configure() update dma configuration

[Arnd Bergmann]

On Thursday 08 January 2015 14:52:13 Murali Karicheri wrote:
> 
> Could you add this as as a follow up patch as I don't have a platformm 
> that support IOMMU and as such my understanding of the IOMMU is limited?
> 
> I can help test the change to make sure it has no side effect on 
> Keystone that doesn't support IOMMU.

I think that's fine. Let's get your series done first and add the
DMA aliases for iommus on top once we have worked out what to do.

	Arnd

Re: [PATCH v3 0/4] PCI: get DMA configuration from parent device

[Arnd Bergmann]

On Thursday 08 January 2015 09:56:39 Arnd Bergmann wrote:
> There is another interesting case, which is a USB host controller or
> something similar behind a PCI bus. These are quite common and also
> need to be handled in some form. Let's do just PCI first for now, but
> be aware that this will come next. Should we assume that the ID that
> is required for a device is either known from the device node, or
> that it comes from a PCI device? That means for the USB case, we will
> likely need to have some custom logic. There seems to be an implicit
> assumption all over the kernel that all devices have the same IOMMU
> instance, but we can't really rely on that here.
> 

Update: I've checked the USB implementation and we are lucky because
these interactions are all done by the USB host controller, and a
USB device is not itself DMA capable. We don't have to do anything
for USB.

	Arnd

Re: [PATCH v3 0/4] PCI: get DMA configuration from parent device

[Murali Karicheri]

On 01/08/2015 05:27 PM, Arnd Bergmann wrote:
> On Thursday 08 January 2015 09:56:39 Arnd Bergmann wrote:
>> There is another interesting case, which is a USB host controller or
>> something similar behind a PCI bus. These are quite common and also
>> need to be handled in some form. Let's do just PCI first for now, but
>> be aware that this will come next. Should we assume that the ID that
>> is required for a device is either known from the device node, or
>> that it comes from a PCI device? That means for the USB case, we will
>> likely need to have some custom logic. There seems to be an implicit
>> assumption all over the kernel that all devices have the same IOMMU
>> instance, but we can't really rely on that here.
>>
>
> Update: I've checked the USB implementation and we are lucky because
> these interactions are all done by the USB host controller, and a
> USB device is not itself DMA capable. We don't have to do anything
> for USB.

Ok Thanks.

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


-- 
Murali Karicheri
Linux Kernel, Texas Instruments

Re: [PATCH v3 3/4] of/pci: add of_pci_dma_configure() update dma configuration

[Murali Karicheri]

On 01/08/2015 05:25 PM, Arnd Bergmann wrote:
> On Thursday 08 January 2015 14:52:13 Murali Karicheri wrote:
>>
>> Could you add this as as a follow up patch as I don't have a platformm
>> that support IOMMU and as such my understanding of the IOMMU is limited?
>>
>> I can help test the change to make sure it has no side effect on
>> Keystone that doesn't support IOMMU.
>
> I think that's fine. Let's get your series done first and add the
> DMA aliases for iommus on top once we have worked out what to do.
>
> 	Arnd
Ok.

-- 
Murali Karicheri
Linux Kernel, Texas Instruments

Re: [PATCH v3 2/4] of: move of_dma_configure() to device,c to help re-use

[Murali Karicheri]

On 01/08/2015 05:24 PM, Arnd Bergmann wrote:
> On Thursday 08 January 2015 14:26:36 Murali Karicheri wrote:
>> On 01/08/2015 03:40 AM, Arnd Bergmann wrote:
>>> On Wednesday 07 January 2015 17:37:56 Rob Herring wrote:
>>>> On Wed, Jan 7, 2015 at 12:49 PM, Murali Karicheri<m-karicheri2@ti.com>   wrote:
>>>>
>>>>> +       ret = of_dma_get_range(np,&dma_addr,&paddr,&size);
>>>>> +       if (ret<   0) {
>>>>> +               dma_addr = offset = 0;
>>>>> +               size = dev->coherent_dma_mask + 1;
>>>>
>>>> If coherent_dma_mask is DMA_BIT_MASK(64), then you will overflow and
>>>> have a size of 0. There may also be a problem when the mask is only
>>>> 32-bit type.
>>>
>>> The mask is always a 64-bit type, it's not optional. But you are right,
>>> the 64-bit mask case is broken, so I guess we have to fix it differently
>>> by always passing the smaller value into arch_setup_dma_ops and
>>> adapting that function instead.
>> Arnd,
>>
>> What is the smaller value you are referring to in the below code?
>> between *dev->dma_mask and size from DT? But overflow can still happen
>> when size is to be calculated in arch_setup_dma_ops() for Non DT case or
>> when DT size is configured to be equivalent of DMA_BIT_MASK(64) + 1. Can
>> we discuss the code change you have in mind when you get a chance?
>
> I meant changing every function that the size values gets passed into
> to take a mask like 0xffffffff instead of a size like 0x100000000, so
> we can represent a 64-bit capable bus correctly.

The function here refers to  arch_setup_dma_ops() and anything that is 
called by this API, right?
>
> This means we also need to adapt the value returned from of_dma_get_range.
> A minor complication here is that the DT properties sometimes already
> contain the mask value, in particular when we want to represent a
> full mapping like
>
The grep of dma-ranges for arch/arm shows the size used is mask + 1 as

./boot/dts/keystone.dtsi:		dma-ranges = <0x80000000 0x8 0x00000000 
0x80000000>;
./boot/dts/keystone.dtsi:			dma-ranges;
./boot/dts/keystone.dtsi:			dma-ranges;
./boot/dts/r8a7790.dtsi:		dma-ranges = <0x42000000 0 0x40000000 0 
0x40000000 0 0x80000000
./boot/dts/integratorap.dts:	dma-ranges = <0x80000000 0x0 0x80000000>;
./boot/dts/r8a7791.dtsi:		dma-ranges = <0x42000000 0 0x40000000 0 
0x40000000 0 0x80000000
./boot/dts/.k2hk-evm.dtb.dts.tmp:  dma-ranges = <0x80000000 0x8 
0x00000000 0x80000000>;
./boot/dts/.k2hk-evm.dtb.dts.tmp:   dma-ranges;
./boot/dts/.k2l-evm.dtb.dts.tmp:  dma-ranges = <0x80000000 0x8 
0x00000000 0x80000000>;
./boot/dts/.k2l-evm.dtb.dts.tmp:   dma-ranges;
./boot/dts/.k2e-evm.dtb.dts.tmp:  dma-ranges = <0x80000000 0x8 
0x00000000 0x80000000>;
./boot/dts/.k2e-evm.dtb.dts.tmp:   dma-ranges;
./boot/dts/.k2e-evm.dtb.dts.tmp:   dma-ranges;
./boot/dts/k2e.dtsi:			dma-ranges;
./boot/dts/k2e.dtsi:			dma-ranges;

So for ARM 32 the below case doesn't seem to apply.

> 	bus {
> 		#address-cells =<1>;
> 		#size-cells =<1>;
> 		dma-ranges =<0 0 0xffffffff>; /* all 4 GB, DMA_BIT_MASK(32) */
> 	};
>
> as opposed to
>

A search under arch/arm64 and arch/powerpc showed another format for 
dma-ranges for PCI. Now I am bit nervous on how of_pci_dma_configure() 
behave for these platforms as PCI nodes on these platforms may have 
dma-ranges in a different format than that in platform bus nodes. These 
DT property has additional cell for resource identification (example 
0x42000000 as the first cell). Also one of them, amd-seattle-soc.dtsi 
uses the format of size (0x7fffffffff) similar to what you refered above.

./boot/dts/apm/apm-storm.dtsi:			dma-ranges = <0x42000000 0x80 
0x00000000 0x80 0x00000000 0x00 0x80000000
./boot/dts/apm/apm-storm.dtsi:			dma-ranges = <0x42000000 0x80 
0x00000000 0x80 0x00000000 0x00 0x80000000
./boot/dts/apm/apm-storm.dtsi:			dma-ranges = <0x42000000 0x80 
0x00000000 0x80 0x00000000 0x00 0x80000000
./boot/dts/apm/apm-storm.dtsi:			dma-ranges = <0x42000000 0x80 
0x00000000 0x80 0x00000000 0x00 0x80000000
./boot/dts/apm/apm-storm.dtsi:			dma-ranges = <0x42000000 0x80 
0x00000000 0x80 0x00000000 0x00 0x80000000
./boot/dts/amd/amd-seattle-soc.dtsi:		dma-ranges = <0x80 0x0 0x80 0x0 
0x7f 0xffffffff>;
./boot/dts/amd/amd-seattle-soc.dtsi:			dma-ranges = <0x43000000 0x80 0x0 
0x80 0x0 0x7f 0xffffffff>;

Or I got this wrong. Any comment?

For ARM 32, DT size value can be extracted and size-1 can be passed to 
arch_setup_dma_ops() provided it handles this value properly.

> 	bus {
> 		#address-cells =<1>;
> 		#size-cells =<1>;
> 		dma-ranges =<0 0 0x80000000>; /* only lower 2GB, DMA_BIT_MASK(31) */
> 	};
>
> or
>
> 	bus {
> 		#address-cells =<2>;
> 		#size-cells =<2>;
> 		dma-ranges =<0 0 0x0000000100000000>; /* 4GB of 64-bit address space */
> 	};
>
>
> 	Arnd


-- 
Murali Karicheri
Linux Kernel, Texas Instruments

Re: [PATCH v3 2/4] of: move of_dma_configure() to device,c to help re-use

[Arnd Bergmann]

On Thursday 08 January 2015 18:44:31 Murali Karicheri wrote:
> The grep of dma-ranges for arch/arm shows the size used is mask + 1 as
> 
> ./boot/dts/keystone.dtsi:               dma-ranges = <0x80000000 0x8 0x00000000 
> 0x80000000>;
> ./boot/dts/keystone.dtsi:                       dma-ranges;
> ./boot/dts/keystone.dtsi:                       dma-ranges;
> ./boot/dts/r8a7790.dtsi:                dma-ranges = <0x42000000 0 0x40000000 0 
> 0x40000000 0 0x80000000
> ./boot/dts/integratorap.dts:    dma-ranges = <0x80000000 0x0 0x80000000>;
> ./boot/dts/r8a7791.dtsi:                dma-ranges = <0x42000000 0 0x40000000 0 
> 0x40000000 0 0x80000000
> ./boot/dts/.k2hk-evm.dtb.dts.tmp:  dma-ranges = <0x80000000 0x8 
> 0x00000000 0x80000000>;
> ./boot/dts/.k2hk-evm.dtb.dts.tmp:   dma-ranges;
> ./boot/dts/.k2l-evm.dtb.dts.tmp:  dma-ranges = <0x80000000 0x8 
> 0x00000000 0x80000000>;
> ./boot/dts/.k2l-evm.dtb.dts.tmp:   dma-ranges;
> ./boot/dts/.k2e-evm.dtb.dts.tmp:  dma-ranges = <0x80000000 0x8 
> 0x00000000 0x80000000>;
> ./boot/dts/.k2e-evm.dtb.dts.tmp:   dma-ranges;
> ./boot/dts/.k2e-evm.dtb.dts.tmp:   dma-ranges;
> ./boot/dts/k2e.dtsi:                    dma-ranges;
> ./boot/dts/k2e.dtsi:                    dma-ranges;
> 
> So for ARM 32 the below case doesn't seem to apply.
> 

Ah, I guess that is because an empty dma-ranges property serves the same
purpose.

	Arnd

Re: [PATCH v3 3/4] of/pci: add of_pci_dma_configure() update dma configuration

[Will Deacon]

On Thu, Jan 08, 2015 at 10:25:15PM +0000, Arnd Bergmann wrote:
> On Thursday 08 January 2015 14:52:13 Murali Karicheri wrote:
> > 
> > Could you add this as as a follow up patch as I don't have a platformm 
> > that support IOMMU and as such my understanding of the IOMMU is limited?
> > 
> > I can help test the change to make sure it has no side effect on 
> > Keystone that doesn't support IOMMU.
> 
> I think that's fine. Let's get your series done first and add the
> DMA aliases for iommus on top once we have worked out what to do.

Yeah, I'm fine with that too. I think we're going to be working on this for
a while, so doing it bit-by-bit makes a lot of sense.

Will

Re: [PATCH v3 2/4] of: move of_dma_configure() to device,c to help re-use

[Rob Herring]

On Thu, Jan 8, 2015 at 4:24 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 08 January 2015 14:26:36 Murali Karicheri wrote:
>> On 01/08/2015 03:40 AM, Arnd Bergmann wrote:
>> > On Wednesday 07 January 2015 17:37:56 Rob Herring wrote:
>> >> On Wed, Jan 7, 2015 at 12:49 PM, Murali Karicheri<m-karicheri2@ti.com>  wrote:
>> >>
>> >>> +       ret = of_dma_get_range(np,&dma_addr,&paddr,&size);
>> >>> +       if (ret<  0) {
>> >>> +               dma_addr = offset = 0;
>> >>> +               size = dev->coherent_dma_mask + 1;
>> >>
>> >> If coherent_dma_mask is DMA_BIT_MASK(64), then you will overflow and
>> >> have a size of 0. There may also be a problem when the mask is only
>> >> 32-bit type.
>> >
>> > The mask is always a 64-bit type, it's not optional. But you are right,
>> > the 64-bit mask case is broken, so I guess we have to fix it differently
>> > by always passing the smaller value into arch_setup_dma_ops and
>> > adapting that function instead.
>> Arnd,
>>
>> What is the smaller value you are referring to in the below code?
>> between *dev->dma_mask and size from DT? But overflow can still happen
>> when size is to be calculated in arch_setup_dma_ops() for Non DT case or
>> when DT size is configured to be equivalent of DMA_BIT_MASK(64) + 1. Can
>> we discuss the code change you have in mind when you get a chance?
>
> I meant changing every function that the size values gets passed into
> to take a mask like 0xffffffff instead of a size like 0x100000000, so
> we can represent a 64-bit capable bus correctly.

Or you could special case a size of 0 to mean all/max? I'm not sure if
we need to handle size=0 for other reasons beyond just wrong DT data.

> This means we also need to adapt the value returned from of_dma_get_range.
> A minor complication here is that the DT properties sometimes already
> contain the mask value, in particular when we want to represent a
> full mapping like
>
>         bus {
>                 #address-cells = <1>;
>                 #size-cells = <1>;
>                 dma-ranges = <0 0 0xffffffff>; /* all 4 GB, DMA_BIT_MASK(32) */

This is wrong though, right? The DT should be size. Certainly, this
could be a valid size, but that would not make the mask 0xfffffffe. We
would still want it to be 0xffffffff.

We could do a fixup for these cases adding 1 if bit 0 is set (or not
subtracting 1 if we want the mask).

Rob

Re: [PATCH v3 2/4] of: move of_dma_configure() to device,c to help re-use

[Murali Karicheri]

On 01/09/2015 10:34 AM, Rob Herring wrote:
> On Thu, Jan 8, 2015 at 4:24 PM, Arnd Bergmann<arnd@arndb.de>  wrote:
>> On Thursday 08 January 2015 14:26:36 Murali Karicheri wrote:
>>> On 01/08/2015 03:40 AM, Arnd Bergmann wrote:
>>>> On Wednesday 07 January 2015 17:37:56 Rob Herring wrote:
>>>>> On Wed, Jan 7, 2015 at 12:49 PM, Murali Karicheri<m-karicheri2@ti.com>   wrote:
>>>>>
>>>>>> +       ret = of_dma_get_range(np,&dma_addr,&paddr,&size);
>>>>>> +       if (ret<   0) {
>>>>>> +               dma_addr = offset = 0;
>>>>>> +               size = dev->coherent_dma_mask + 1;
>>>>>
>>>>> If coherent_dma_mask is DMA_BIT_MASK(64), then you will overflow and
>>>>> have a size of 0. There may also be a problem when the mask is only
>>>>> 32-bit type.
>>>>
>>>> The mask is always a 64-bit type, it's not optional. But you are right,
>>>> the 64-bit mask case is broken, so I guess we have to fix it differently
>>>> by always passing the smaller value into arch_setup_dma_ops and
>>>> adapting that function instead.
>>> Arnd,
>>>
>>> What is the smaller value you are referring to in the below code?
>>> between *dev->dma_mask and size from DT? But overflow can still happen
>>> when size is to be calculated in arch_setup_dma_ops() for Non DT case or
>>> when DT size is configured to be equivalent of DMA_BIT_MASK(64) + 1. Can
>>> we discuss the code change you have in mind when you get a chance?
>>
>> I meant changing every function that the size values gets passed into
>> to take a mask like 0xffffffff instead of a size like 0x100000000, so
>> we can represent a 64-bit capable bus correctly.
>
> Or you could special case a size of 0 to mean all/max? I'm not sure if
> we need to handle size=0 for other reasons beyond just wrong DT data.
>
>> This means we also need to adapt the value returned from of_dma_get_range.
>> A minor complication here is that the DT properties sometimes already
>> contain the mask value, in particular when we want to represent a
>> full mapping like
>>
>>          bus {
>>                  #address-cells =<1>;
>>                  #size-cells =<1>;
>>                  dma-ranges =<0 0 0xffffffff>; /* all 4 GB, DMA_BIT_MASK(32) */
>
> This is wrong though, right? The DT should be size. Certainly, this
> could be a valid size, but that would not make the mask 0xfffffffe. We
> would still want it to be 0xffffffff.
>
> We could do a fixup for these cases adding 1 if bit 0 is set (or not
> subtracting 1 if we want the mask).
>
> Rob
Arnd, Rob, et all,

Do we have preference one way or other for the size format? If we need 
to follow the mask format, all of the calling functions below and the 
arm_iommu_create_mapping() has to change as well to use this changed format.

drivers/gpu/drm/rockchip/rockchip_drm_drv.c:	mapping = 
arm_iommu_create_mapping(&platform_bus_type, 0x00000000,
drivers/gpu/drm/exynos/exynos_drm_iommu.c:	mapping = 
arm_iommu_create_mapping(&platform_bus_type, priv->da_start,
drivers/media/platform/omap3isp/isp.c:	mapping = 
arm_iommu_create_mapping(&platform_bus_type, SZ_1G, SZ_2G);
drivers/iommu/shmobile-iommu.c:		mapping = 
arm_iommu_create_mapping(&platform_bus_type, 0,
drivers/iommu/ipmmu-vmsa.c:		mapping = 
arm_iommu_create_mapping(&platform_bus_type,

So IMO, keeping current convention of size and taking care of exception 
in DT handling is the right thing to do instead of changing all of the 
above functions. i.e in of_dma_configure(), if DT provide a mask for 
size (lsb set), we  will check that and add 1 to it. Only case in DTS 
that I can see this usage is

arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi:		dma-ranges = <0x80 0x0 
0x80 0x0 0x7f 0xffffffff>;
arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi:			dma-ranges = <0x43000000 
0x80 0x0 0x80 0x0 0x7f 0xffffffff>;

This logic should take care of setting the size to ox80_00000000 for 
these cases. if dma_coherent_mask is set to max of u64, then this will 
result in a zero size (both DT case and non DT case). So treat a size of 
zero as error being overflow.

Also arm_iommu_create_mapping() currently accept a size of type size_t 
which means, this function expect the size to be max of 0xffffffff. So 
in arm_setup_iommu_dma_ops(), we need to check if size if >0xffffffff 
and return an error. If in future this function support u64 for size, 
this check can be removed.

I will update the series with this change and post it if we have an 
agreement on this. Please repond.

Thanks

-- 
Murali Karicheri
Linux Kernel, Texas Instruments

Re: [PATCH v3 2/4] of: move of_dma_configure() to device,c to help re-use

[Rob Herring]

On Fri, Jan 23, 2015 at 12:19 PM, Murali Karicheri <m-karicheri2@ti.com> wrote:
> On 01/09/2015 10:34 AM, Rob Herring wrote:
>>
>> On Thu, Jan 8, 2015 at 4:24 PM, Arnd Bergmann<arnd@arndb.de>  wrote:
>>>
>>> On Thursday 08 January 2015 14:26:36 Murali Karicheri wrote:
>>>>
>>>> On 01/08/2015 03:40 AM, Arnd Bergmann wrote:
>>>>>
>>>>> On Wednesday 07 January 2015 17:37:56 Rob Herring wrote:
>>>>>>
>>>>>> On Wed, Jan 7, 2015 at 12:49 PM, Murali Karicheri<m-karicheri2@ti.com>
>>>>>> wrote:
>>>>>>
>>>>>>> +       ret = of_dma_get_range(np,&dma_addr,&paddr,&size);
>>>>>>> +       if (ret<   0) {
>>>>>>> +               dma_addr = offset = 0;
>>>>>>> +               size = dev->coherent_dma_mask + 1;
>>>>>>
>>>>>>
>>>>>> If coherent_dma_mask is DMA_BIT_MASK(64), then you will overflow and
>>>>>> have a size of 0. There may also be a problem when the mask is only
>>>>>> 32-bit type.
>>>>>
>>>>>
>>>>> The mask is always a 64-bit type, it's not optional. But you are right,
>>>>> the 64-bit mask case is broken, so I guess we have to fix it
>>>>> differently
>>>>> by always passing the smaller value into arch_setup_dma_ops and
>>>>> adapting that function instead.
>>>>
>>>> Arnd,
>>>>
>>>> What is the smaller value you are referring to in the below code?
>>>> between *dev->dma_mask and size from DT? But overflow can still happen
>>>> when size is to be calculated in arch_setup_dma_ops() for Non DT case or
>>>> when DT size is configured to be equivalent of DMA_BIT_MASK(64) + 1. Can
>>>> we discuss the code change you have in mind when you get a chance?
>>>
>>>
>>> I meant changing every function that the size values gets passed into
>>> to take a mask like 0xffffffff instead of a size like 0x100000000, so
>>> we can represent a 64-bit capable bus correctly.
>>
>>
>> Or you could special case a size of 0 to mean all/max? I'm not sure if
>> we need to handle size=0 for other reasons beyond just wrong DT data.
>>
>>> This means we also need to adapt the value returned from
>>> of_dma_get_range.
>>> A minor complication here is that the DT properties sometimes already
>>> contain the mask value, in particular when we want to represent a
>>> full mapping like
>>>
>>>          bus {
>>>                  #address-cells =<1>;
>>>                  #size-cells =<1>;
>>>                  dma-ranges =<0 0 0xffffffff>; /* all 4 GB,
>>> DMA_BIT_MASK(32) */
>>
>>
>> This is wrong though, right? The DT should be size. Certainly, this
>> could be a valid size, but that would not make the mask 0xfffffffe. We
>> would still want it to be 0xffffffff.
>>
>> We could do a fixup for these cases adding 1 if bit 0 is set (or not
>> subtracting 1 if we want the mask).
>>
>> Rob
>
> Arnd, Rob, et all,
>
> Do we have preference one way or other for the size format? If we need to
> follow the mask format, all of the calling functions below and the
> arm_iommu_create_mapping() has to change as well to use this changed format.
>
> drivers/gpu/drm/rockchip/rockchip_drm_drv.c:    mapping =
> arm_iommu_create_mapping(&platform_bus_type, 0x00000000,
> drivers/gpu/drm/exynos/exynos_drm_iommu.c:      mapping =
> arm_iommu_create_mapping(&platform_bus_type, priv->da_start,
> drivers/media/platform/omap3isp/isp.c:  mapping =
> arm_iommu_create_mapping(&platform_bus_type, SZ_1G, SZ_2G);
> drivers/iommu/shmobile-iommu.c:         mapping =
> arm_iommu_create_mapping(&platform_bus_type, 0,
> drivers/iommu/ipmmu-vmsa.c:             mapping =
> arm_iommu_create_mapping(&platform_bus_type,
>
> So IMO, keeping current convention of size and taking care of exception in
> DT handling is the right thing to do instead of changing all of the above
> functions. i.e in of_dma_configure(), if DT provide a mask for size (lsb
> set), we  will check that and add 1 to it. Only case in DTS that I can see
> this usage is
>
> arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi:           dma-ranges = <0x80
> 0x0 0x80 0x0 0x7f 0xffffffff>;
> arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi:                   dma-ranges =
> <0x43000000 0x80 0x0 0x80 0x0 0x7f 0xffffffff>;

This should be fixed regardless. I doubt anyone is worried about 512GB
quite yet.

> This logic should take care of setting the size to ox80_00000000 for these
> cases. if dma_coherent_mask is set to max of u64, then this will result in a
> zero size (both DT case and non DT case). So treat a size of zero as error
> being overflow.

I think this would work, but I really need to see patches.

> Also arm_iommu_create_mapping() currently accept a size of type size_t which
> means, this function expect the size to be max of 0xffffffff. So in
> arm_setup_iommu_dma_ops(), we need to check if size if >0xffffffff and
> return an error. If in future this function support u64 for size, this check
> can be removed.

The aim is to get rid of this function I believe.

Rob