[PATCH v2 0/2] PCI: get DMA configuration from parent device

[PATCH v2 0/2] 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:
	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 (2):
  of/pci: add of_pci_dma_configure() update dma configuration
  PCI: update dma configuration from DT

 drivers/of/of_pci.c    |   73 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/probe.c    |    2 ++
 include/linux/of_pci.h |   12 ++++++++
 3 files changed, 87 insertions(+)

-- 
1.7.9.5

[PATCH v2 1/2] 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    |   73 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_pci.h |   12 ++++++++
 2 files changed, 85 insertions(+)

diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
index 88471d3..6d43f59 100644
--- a/drivers/of/of_pci.c
+++ b/drivers/of/of_pci.c
@@ -229,6 +229,79 @@ parse_failed:
 	return err;
 }
 EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources);
+
+/**
+ * of_get_pci_root_bridge_parent - Helper function to 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 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
+ *
+ * Try to get PCI devices's DMA configuration from DT and update it
+ * accordingly. This is similar to of_dma_configure() in of/platform.c
+ */
+void of_pci_dma_configure(struct pci_dev *pci_dev)
+{
+	struct device *dev = &pci_dev->dev;
+	u64 dma_addr, paddr, size;
+	struct device_node *parent_np;
+	unsigned long offset;
+	bool coherent;
+	int ret;
+
+	parent_np = of_get_pci_root_bridge_parent(pci_dev);
+
+	if (parent_np) {
+		/*
+		 * 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(parent_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(parent_np);
+		dev_dbg(dev, "device is%sdma coherent\n",
+			coherent ? " " : " not ");
+
+		arch_setup_dma_ops(dev, dma_addr, size, NULL, coherent);
+	}
+}
+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

Re: [PATCH v2 1/2] of/pci: add of_pci_dma_configure() update dma configuration

[Rob Herring]

On Wed, Dec 24, 2014 at 4:11 PM, Murali Karicheri <m-karicheri2@ti.com> 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    |   73 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_pci.h |   12 ++++++++
>  2 files changed, 85 insertions(+)
>
> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
> index 88471d3..6d43f59 100644
> --- a/drivers/of/of_pci.c
> +++ b/drivers/of/of_pci.c
> @@ -229,6 +229,79 @@ parse_failed:
>         return err;
>  }
>  EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources);
> +
> +/**
> + * of_get_pci_root_bridge_parent - Helper function to get the OF node of
> + *             the root bridge's parent

I believe this has to be a one line description for DocBook.

> + * @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 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
> + *
> + * Try to get PCI devices's DMA configuration from DT and update it
> + * accordingly. This is similar to of_dma_configure() in of/platform.c
> + */
> +void of_pci_dma_configure(struct pci_dev *pci_dev)
> +{
> +       struct device *dev = &pci_dev->dev;
> +       u64 dma_addr, paddr, size;
> +       struct device_node *parent_np;
> +       unsigned long offset;
> +       bool coherent;
> +       int ret;
> +
> +       parent_np = of_get_pci_root_bridge_parent(pci_dev);
> +
> +       if (parent_np) {

Save a level of indentation and do:

if (!parent_np)
  return;

> +               /*
> +                * 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(parent_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(parent_np);
> +               dev_dbg(dev, "device is%sdma coherent\n",
> +                       coherent ? " " : " not ");
> +
> +               arch_setup_dma_ops(dev, dma_addr, size, NULL, coherent);

This is the same code as of_dma_configure. The only difference I see
is which node ptr is passed to of_dma_get_range. You need to make that
a function param of of_dma_configure.

of_dma_configure also has iommu handling now. You will probably need
something similar for PCI in that you setup an iommu based on the root
bus DT properties.

Rob

Re: [PATCH v2 1/2] of/pci: add of_pci_dma_configure() update dma configuration

[Murali Karicheri]

Rob,

See my response below. Arnd and Will, please review this as well.

On 12/26/2014 02:33 PM, Rob Herring wrote:
> On Wed, Dec 24, 2014 at 4:11 PM, Murali Karicheri<m-karicheri2@ti.com>  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    |   73 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/of_pci.h |   12 ++++++++
>>   2 files changed, 85 insertions(+)
>>
----------------------CUT--------------------------------------------
>> +       unsigned long offset;
>> +       bool coherent;
>> +       int ret;
>> +
>> +       parent_np = of_get_pci_root_bridge_parent(pci_dev);
>> +
>> +       if (parent_np) {
>
> Save a level of indentation and do:
>
> if (!parent_np)
>    return;

Agree. This will not be needed if I go with changes proposed by your 
next comment below.
>
>> +               /*
-------CUT------------------------------------------------------
>> +
>> +               coherent = of_dma_is_coherent(parent_np);
>> +               dev_dbg(dev, "device is%sdma coherent\n",
>> +                       coherent ? " " : " not ");
>> +
>> +               arch_setup_dma_ops(dev, dma_addr, size, NULL, coherent);
>
> This is the same code as of_dma_configure. The only difference I see
> is which node ptr is passed to of_dma_get_range. You need to make that
> a function param of of_dma_configure.
>
> of_dma_configure also has iommu handling now. You will probably need
> something similar for PCI in that you setup an iommu based on the root
> bus DT properties.
>
Initially I had the same idea to re-use the existing function 
of_dma_configure() for this. I wanted to defer this until we have an 
agreement on the changes required for the subject functionality. My 
quick review of the code suggestio this would require additional API 
changes as below. I did a quick test of the changes and it works for 
Keystone, but need to be reviewed by everyone as I touch the IOMMU 
functionality here and I don't have a platform with IOMMU. Need test by 
someone to make sure I don't break anything.

Here are the changes required to implement this suggestion.

1. Move the of_dma_configure() to drivers/of/device.c (include the API 
in of_device.h) and make it global (using proper EXPORT macro). 
Otherwise, we will have to include of_platform.h in drivers/of/of_pci.c 
assuming the prototype is defined in of_platform.h which doesn't look 
appropriate to me. Would require following additional include files in 
drivers/of/device.c as well.

+#include <linux/of_address.h>
+#include <linux/of_iommu.h>
+#include <linux/dma-mapping.h>

2. drivers/iommu/of_iommu.c. This is needed so that of_iommu_configure() 
can take confuguration from the root bus DT as you have suggested.

-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;
@@ -145,7 +145,7 @@ struct iommu_ops *of_iommu_configure(struct device *dev)
          * 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)) {

3. drivers/of/of_pci.c. The existing function (added in this patch) will 
make call to of_dma_configure() as

parent_np = of_get_pci_root_bridge_parent(pci_dev);
of_dma_configure(dev, parent_np);

4. drivers/of/platform.c. Add a wrapper function 
of_platform_dma_configure() that calls of_dma_configure() as
of_dma_configure(dev, dev->of_node). All existing calls converted to 
this wrapper.

If the above looks good, I can post v3 of the patch with these changes.

> Rob


-- 
Murali Karicheri
Linux Kernel, Texas Instruments

Re: [PATCH v2 1/2] of/pci: add of_pci_dma_configure() update dma configuration

[Rob Herring]

On Fri, Jan 2, 2015 at 11:20 AM, Murali Karicheri <m-karicheri2@ti.com> wrote:
> Rob,
>
> See my response below. Arnd and Will, please review this as well.
>
> On 12/26/2014 02:33 PM, Rob Herring wrote:
>>
>> On Wed, Dec 24, 2014 at 4:11 PM, Murali Karicheri<m-karicheri2@ti.com>
>> 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    |   73
>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>   include/linux/of_pci.h |   12 ++++++++
>>>   2 files changed, 85 insertions(+)
>>>

[...]

>>> +               coherent = of_dma_is_coherent(parent_np);
>>> +               dev_dbg(dev, "device is%sdma coherent\n",
>>> +                       coherent ? " " : " not ");
>>> +
>>> +               arch_setup_dma_ops(dev, dma_addr, size, NULL, coherent);
>>
>>
>> This is the same code as of_dma_configure. The only difference I see
>> is which node ptr is passed to of_dma_get_range. You need to make that
>> a function param of of_dma_configure.
>>
>> of_dma_configure also has iommu handling now. You will probably need
>> something similar for PCI in that you setup an iommu based on the root
>> bus DT properties.
>>
> Initially I had the same idea to re-use the existing function
> of_dma_configure() for this. I wanted to defer this until we have an
> agreement on the changes required for the subject functionality. My quick
> review of the code suggestio this would require additional API changes as
> below. I did a quick test of the changes and it works for Keystone, but need
> to be reviewed by everyone as I touch the IOMMU functionality here and I
> don't have a platform with IOMMU. Need test by someone to make sure I don't
> break anything.

The IOMMU changes look trivial. We may want to address the comment in
of_iommu_configure about parent nodes. We should be sure these changes
work with how we would do searching parent nodes.

> Here are the changes required to implement this suggestion.
>
> 1. Move the of_dma_configure() to drivers/of/device.c (include the API in
> of_device.h) and make it global (using proper EXPORT macro). Otherwise, we
> will have to include of_platform.h in drivers/of/of_pci.c assuming the
> prototype is defined in of_platform.h which doesn't look appropriate to me.
> Would require following additional include files in drivers/of/device.c as
> well.
>
> +#include <linux/of_address.h>
> +#include <linux/of_iommu.h>
> +#include <linux/dma-mapping.h>

Okay.

> 2. drivers/iommu/of_iommu.c. This is needed so that of_iommu_configure() can
> take confuguration from the root bus DT as you have suggested.
>
> -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;
> @@ -145,7 +145,7 @@ struct iommu_ops *of_iommu_configure(struct device *dev)
>          * 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)) {
>

Seems safe enough.

> 3. drivers/of/of_pci.c. The existing function (added in this patch) will
> make call to of_dma_configure() as
>
> parent_np = of_get_pci_root_bridge_parent(pci_dev);
> of_dma_configure(dev, parent_np);

Okay.

> 4. drivers/of/platform.c. Add a wrapper function of_platform_dma_configure()
> that calls of_dma_configure() as
> of_dma_configure(dev, dev->of_node). All existing calls converted to this
> wrapper.

There's only a few callers of of_dma_configure, so I don't think this
is necessary. The only thing platform bus specific is who is calling
the function.

Rob

Re: [PATCH v2 1/2] of/pci: add of_pci_dma_configure() update dma configuration

[Murali Karicheri]

On 01/02/2015 03:45 PM, Rob Herring wrote:
> On Fri, Jan 2, 2015 at 11:20 AM, Murali Karicheri<m-karicheri2@ti.com>  wrote:
>> Rob,
>>
>> See my response below. Arnd and Will, please review this as well.
>>
>> On 12/26/2014 02:33 PM, Rob Herring wrote:
>>>
>>> On Wed, Dec 24, 2014 at 4:11 PM, Murali Karicheri<m-karicheri2@ti.com>
>>> 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    |   73
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    include/linux/of_pci.h |   12 ++++++++
>>>>    2 files changed, 85 insertions(+)
>>>>
>
> [...]
>
>>>> +               coherent = of_dma_is_coherent(parent_np);
>>>> +               dev_dbg(dev, "device is%sdma coherent\n",
>>>> +                       coherent ? " " : " not ");
>>>> +
>>>> +               arch_setup_dma_ops(dev, dma_addr, size, NULL, coherent);
>>>
>>>
>>> This is the same code as of_dma_configure. The only difference I see
>>> is which node ptr is passed to of_dma_get_range. You need to make that
>>> a function param of of_dma_configure.
>>>
>>> of_dma_configure also has iommu handling now. You will probably need
>>> something similar for PCI in that you setup an iommu based on the root
>>> bus DT properties.
>>>
>> Initially I had the same idea to re-use the existing function
>> of_dma_configure() for this. I wanted to defer this until we have an
>> agreement on the changes required for the subject functionality. My quick
>> review of the code suggestio this would require additional API changes as
>> below. I did a quick test of the changes and it works for Keystone, but need
>> to be reviewed by everyone as I touch the IOMMU functionality here and I
>> don't have a platform with IOMMU. Need test by someone to make sure I don't
>> break anything.
>
> The IOMMU changes look trivial. We may want to address the comment in
> of_iommu_configure about parent nodes. We should be sure these changes
> work with how we would do searching parent nodes.

I have no experience with IOMMU and may not offer much help here as I 
originally wrote above. Will Deacon has added this API and probably able 
to offer some help in this discussion.

Will Deacon,

Any comment?

Looking at the iommu documentation and of_iommu.c, I get a feeling that 
this API is not really used at present as there are no callers of 
of_iommu_set_ops() and I assume this is a WIP. I believe the way it is 
expected to work is to have the iommu driver of the master IOMMU devices 
call of_iommu_set_ops(). The device node of this master IOMMU device is 
specified as a phandle in the OF node of the device (various bus devices 
such as platform, PCI etc). This allow to retrieve the iommu ops though 
the of_iommu_configure() API and use it in arch_setup_dma_ops(). So my 
gut feeling is that for PCI devices, as there are no DT node, the root 
bus node may specify iommus phandle to IOMMU master OF nodes.

W.r.t your comment "We may want to address the comment in
of_iommu_configure about parent nodes. We should be sure these changes 
work with how we would do searching parent nodes",

I believe, the parent node search itself should work the same way in the 
case of PCI as with platform bus case. PCI's case, we are providing the 
OF node of the root bus host bridge. Why should this be any different in 
terms of search?

I see a potential issue with dma-ranges as described in the notes below.
As noted below the usage of dma-range for iommu is to be determined. For 
keystone, the of_iommu_configure() always return false as we don't use 
the iommu. But don't know if this has any consequences for other 
platforms. Or I got your questions wrong. Any help here from others on 
the list?

========================================================================
One possible extension to the above is to use an "iommus" property along 
with a "dma-ranges" property in a bus device node (such as PCI host 
bridges). This can be useful to describe how children on the bus relate 
to the IOMMU if they are not explicitly listed in the device tree (e.g. 
PCI devices). However, the requirements of that use-case haven't been 
fully determined yet. Implementing this is therefore not recommended 
without further discussion and extension of this binding.
=========================================================================

The code is

struct iommu_ops *of_iommu_configure(struct device *dev)
{
	struct of_phandle_args iommu_spec;
	struct device_node *np;
	struct iommu_ops *ops = NULL;
	int idx = 0;

	/*
	 * 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",
					   "#iommu-cells", idx,
					   &iommu_spec)) {
		np = iommu_spec.np;
		ops = of_iommu_get_ops(np);

		if (!ops || !ops->of_xlate || ops->of_xlate(dev, &iommu_spec))
			goto err_put_node;

		of_node_put(np);
		idx++;
	}

	return ops;

err_put_node:
	of_node_put(np);
	return NULL;
}


>> that calls of_dma_configure() as
>> of_dma_configure(dev, dev->of_node). All existing calls converted to this
>> wrapper.
>
> There's only a few callers of of_dma_configure, so I don't think this
> is necessary. The only thing platform bus specific is who is calling
> the function.

Ok.

>
> Rob


-- 
Murali Karicheri
Linux Kernel, Texas Instruments

Re: [PATCH v2 1/2] of/pci: add of_pci_dma_configure() update dma configuration

[Arnd Bergmann]

On Friday 02 January 2015 17:33:53 Murali Karicheri wrote:
> 
> I have no experience with IOMMU and may not offer much help here as I 
> originally wrote above. Will Deacon has added this API and probably able 
> to offer some help in this discussion.
> 
> Will Deacon,
> 
> Any comment?

It's complicated :(

> Looking at the iommu documentation and of_iommu.c, I get a feeling that 
> this API is not really used at present as there are no callers of 
> of_iommu_set_ops() and I assume this is a WIP.

Right, but we have patches for some iommu drivers based on this API,
and we should migrate all of them eventually.

> I believe the way it is 
> expected to work is to have the iommu driver of the master IOMMU devices 
> call of_iommu_set_ops(). The device node of this master IOMMU device is 
> specified as a phandle in the OF node of the device (various bus devices 
> such as platform, PCI etc). This allow to retrieve the iommu ops though 
> the of_iommu_configure() API and use it in arch_setup_dma_ops(). So my 
> gut feeling is that for PCI devices, as there are no DT node, the root 
> bus node may specify iommus phandle to IOMMU master OF nodes.

Yes, but we also need to pass a PCI device specific identifier along
with the root bus node, because some iommu drivers take the PCI
bus/device/function number into account for creating per-function
i/o page tables.

> W.r.t your comment "We may want to address the comment in
> of_iommu_configure about parent nodes. We should be sure these changes 
> work with how we would do searching parent nodes",
> 
> I believe, the parent node search itself should work the same way in the 
> case of PCI as with platform bus case. PCI's case, we are providing the 
> OF node of the root bus host bridge. Why should this be any different in 
> terms of search?
> 
> I see a potential issue with dma-ranges as described in the notes below.
> As noted below the usage of dma-range for iommu is to be determined. For 
> keystone, the of_iommu_configure() always return false as we don't use 
> the iommu. But don't know if this has any consequences for other 
> platforms. Or I got your questions wrong. Any help here from others on 
> the list?
> 
> ========================================================================
> One possible extension to the above is to use an "iommus" property along 
> with a "dma-ranges" property in a bus device node (such as PCI host 
> bridges). This can be useful to describe how children on the bus relate 
> to the IOMMU if they are not explicitly listed in the device tree (e.g. 
> PCI devices). However, the requirements of that use-case haven't been 
> fully determined yet. Implementing this is therefore not recommended 
> without further discussion and extension of this binding.
> =========================================================================

This probably won't ever apply to PCI devices, so let's ignore it for now.
For the moment (and for PCI), we should assume that we either configure
an iommu directly or we use dma-ranges if no iommu is in use.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] of/pci: add of_pci_dma_configure() update dma configuration

[Murali Karicheri]

On 01/03/2015 04:37 PM, Arnd Bergmann wrote:
> On Friday 02 January 2015 17:33:53 Murali Karicheri wrote:
>>
>> I have no experience with IOMMU and may not offer much help here as I
>> originally wrote above. Will Deacon has added this API and probably able
>> to offer some help in this discussion.
>>
>> Will Deacon,
>>
>> Any comment?
>
> It's complicated :(
>
>> Looking at the iommu documentation and of_iommu.c, I get a feeling that
>> this API is not really used at present as there are no callers of
>> of_iommu_set_ops() and I assume this is a WIP.
>
> Right, but we have patches for some iommu drivers based on this API,
> and we should migrate all of them eventually.
>
>> I believe the way it is
>> expected to work is to have the iommu driver of the master IOMMU devices
>> call of_iommu_set_ops(). The device node of this master IOMMU device is
>> specified as a phandle in the OF node of the device (various bus devices
>> such as platform, PCI etc). This allow to retrieve the iommu ops though
>> the of_iommu_configure() API and use it in arch_setup_dma_ops(). So my
>> gut feeling is that for PCI devices, as there are no DT node, the root
>> bus node may specify iommus phandle to IOMMU master OF nodes.
>
> Yes, but we also need to pass a PCI device specific identifier along
> with the root bus node, because some iommu drivers take the PCI
> bus/device/function number into account for creating per-function
> i/o page tables.
>
>> W.r.t your comment "We may want to address the comment in
>> of_iommu_configure about parent nodes. We should be sure these changes
>> work with how we would do searching parent nodes",
>>
>> I believe, the parent node search itself should work the same way in the
>> case of PCI as with platform bus case. PCI's case, we are providing the
>> OF node of the root bus host bridge. Why should this be any different in
>> terms of search?
>>
>> I see a potential issue with dma-ranges as described in the notes below.
>> As noted below the usage of dma-range for iommu is to be determined. For
>> keystone, the of_iommu_configure() always return false as we don't use
>> the iommu. But don't know if this has any consequences for other
>> platforms. Or I got your questions wrong. Any help here from others on
>> the list?
>>
>> ========================================================================
>> One possible extension to the above is to use an "iommus" property along
>> with a "dma-ranges" property in a bus device node (such as PCI host
>> bridges). This can be useful to describe how children on the bus relate
>> to the IOMMU if they are not explicitly listed in the device tree (e.g.
>> PCI devices). However, the requirements of that use-case haven't been
>> fully determined yet. Implementing this is therefore not recommended
>> without further discussion and extension of this binding.
>> =========================================================================
>
> This probably won't ever apply to PCI devices, so let's ignore it for now.
> For the moment (and for PCI), we should assume that we either configure
> an iommu directly or we use dma-ranges if no iommu is in use.
>
Thanks Arnd,

I will post v3 of the patch with what is agreed before in my response 
and I understand there is no additional change required based on this 
particular discussion about iommu. Right?

Murali
> 	Arnd


-- 
Murali Karicheri
Linux Kernel, Texas Instruments

Re: [PATCH v2 1/2] of/pci: add of_pci_dma_configure() update dma configuration

[Arnd Bergmann]

On Monday 05 January 2015 15:06:57 Murali Karicheri wrote:
> On 01/03/2015 04:37 PM, Arnd Bergmann wrote:
> > Yes, but we also need to pass a PCI device specific identifier along
> > with the root bus node, because some iommu drivers take the PCI
> > bus/device/function number into account for creating per-function
> > i/o page tables.

> ...

> I will post v3 of the patch with what is agreed before in my response 
> and I understand there is no additional change required based on this 
> particular discussion about iommu. Right?

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

Re: [PATCH v2 1/2] of/pci: add of_pci_dma_configure() update dma configuration

[Murali Karicheri]

On 01/05/2015 05:26 PM, Arnd Bergmann wrote:
> On Monday 05 January 2015 15:06:57 Murali Karicheri wrote:
>> On 01/03/2015 04:37 PM, Arnd Bergmann wrote:
>>> Yes, but we also need to pass a PCI device specific identifier along
>>> with the root bus node, because some iommu drivers take the PCI
>>> bus/device/function number into account for creating per-function
>>> i/o page tables.
>
>> ...
>
>> I will post v3 of the patch with what is agreed before in my response
>> and I understand there is no additional change required based on this
>> particular discussion about iommu. Right?
>
> 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.


Murali
>
> 	Arnd


-- 
Murali Karicheri
Linux Kernel, Texas Instruments

Re: [PATCH v2 1/2] of/pci: add of_pci_dma_configure() update dma configuration

[Will Deacon]

On Fri, Jan 02, 2015 at 10:33:53PM +0000, Murali Karicheri wrote:
> On 01/02/2015 03:45 PM, Rob Herring wrote:
> > On Fri, Jan 2, 2015 at 11:20 AM, Murali Karicheri<m-karicheri2@ti.com>  wrote:
> >> On 12/26/2014 02:33 PM, Rob Herring wrote:
> >>> On Wed, Dec 24, 2014 at 4:11 PM, Murali Karicheri<m-karicheri2@ti.com>
> >>>> +               coherent = of_dma_is_coherent(parent_np);
> >>>> +               dev_dbg(dev, "device is%sdma coherent\n",
> >>>> +                       coherent ? " " : " not ");
> >>>> +
> >>>> +               arch_setup_dma_ops(dev, dma_addr, size, NULL, coherent);
> >>>
> >>>
> >>> This is the same code as of_dma_configure. The only difference I see
> >>> is which node ptr is passed to of_dma_get_range. You need to make that
> >>> a function param of of_dma_configure.
> >>>
> >>> of_dma_configure also has iommu handling now. You will probably need
> >>> something similar for PCI in that you setup an iommu based on the root
> >>> bus DT properties.
> >>>
> >> Initially I had the same idea to re-use the existing function
> >> of_dma_configure() for this. I wanted to defer this until we have an
> >> agreement on the changes required for the subject functionality. My quick
> >> review of the code suggestio this would require additional API changes as
> >> below. I did a quick test of the changes and it works for Keystone, but need
> >> to be reviewed by everyone as I touch the IOMMU functionality here and I
> >> don't have a platform with IOMMU. Need test by someone to make sure I don't
> >> break anything.
> >
> > The IOMMU changes look trivial. We may want to address the comment in
> > of_iommu_configure about parent nodes. We should be sure these changes
> > work with how we would do searching parent nodes.
> 
> I have no experience with IOMMU and may not offer much help here as I 
> originally wrote above. Will Deacon has added this API and probably able 
> to offer some help in this discussion.
> 
> Will Deacon,
> 
> Any comment?

Sorry for the delay; I'm still catching up on email after the Christmas
break, but I *will* get around to looking at this. If you have a new version
to post based on the comments from Rob and Arnd, feel free to send that and
I'll look at that instead.

Will

Re: [PATCH v2 1/2] of/pci: add of_pci_dma_configure() update dma configuration

[Murali Karicheri]

On 01/06/2015 02:50 PM, Will Deacon wrote:
> On Fri, Jan 02, 2015 at 10:33:53PM +0000, Murali Karicheri wrote:
>> On 01/02/2015 03:45 PM, Rob Herring wrote:
>>> On Fri, Jan 2, 2015 at 11:20 AM, Murali Karicheri<m-karicheri2@ti.com>   wrote:
>>>> On 12/26/2014 02:33 PM, Rob Herring wrote:
>>>>> On Wed, Dec 24, 2014 at 4:11 PM, Murali Karicheri<m-karicheri2@ti.com>
>>>>>> +               coherent = of_dma_is_coherent(parent_np);
>>>>>> +               dev_dbg(dev, "device is%sdma coherent\n",
>>>>>> +                       coherent ? " " : " not ");
>>>>>> +
>>>>>> +               arch_setup_dma_ops(dev, dma_addr, size, NULL, coherent);
>>>>>
>>>>>
>>>>> This is the same code as of_dma_configure. The only difference I see
>>>>> is which node ptr is passed to of_dma_get_range. You need to make that
>>>>> a function param of of_dma_configure.
>>>>>
>>>>> of_dma_configure also has iommu handling now. You will probably need
>>>>> something similar for PCI in that you setup an iommu based on the root
>>>>> bus DT properties.
>>>>>
>>>> Initially I had the same idea to re-use the existing function
>>>> of_dma_configure() for this. I wanted to defer this until we have an
>>>> agreement on the changes required for the subject functionality. My quick
>>>> review of the code suggestio this would require additional API changes as
>>>> below. I did a quick test of the changes and it works for Keystone, but need
>>>> to be reviewed by everyone as I touch the IOMMU functionality here and I
>>>> don't have a platform with IOMMU. Need test by someone to make sure I don't
>>>> break anything.
>>>
>>> The IOMMU changes look trivial. We may want to address the comment in
>>> of_iommu_configure about parent nodes. We should be sure these changes
>>> work with how we would do searching parent nodes.
>>
>> I have no experience with IOMMU and may not offer much help here as I
>> originally wrote above. Will Deacon has added this API and probably able
>> to offer some help in this discussion.
>>
>> Will Deacon,
>>
>> Any comment?
>
> Sorry for the delay; I'm still catching up on email after the Christmas
> break, but I *will* get around to looking at this. If you have a new version
> to post based on the comments from Rob and Arnd, feel free to send that and
> I'll look at that instead.
>
> Will
Will,

I will be posting v3 of the patch tomorrow and you can review that.

Regards,

Murali


-- 
Murali Karicheri
Linux Kernel, Texas Instruments

Re: [PATCH v2 1/2] of/pci: add of_pci_dma_configure() update dma configuration

[Arnd Bergmann]

On Friday 02 January 2015 12:20:53 Murali Karicheri wrote:
> Initially I had the same idea to re-use the existing function 
> of_dma_configure() for this. I wanted to defer this until we have an 
> agreement on the changes required for the subject functionality. My 
> quick review of the code suggestio this would require additional API 
> changes as below. I did a quick test of the changes and it works for 
> Keystone, but need to be reviewed by everyone as I touch the IOMMU 
> functionality here and I don't have a platform with IOMMU. Need test by 
> someone to make sure I don't break anything.
> 
> Here are the changes required to implement this suggestion.
> 
> 1. Move the of_dma_configure() to drivers/of/device.c (include the API 
> in of_device.h) and make it global (using proper EXPORT macro). 
> Otherwise, we will have to include of_platform.h in drivers/of/of_pci.c 
> assuming the prototype is defined in of_platform.h which doesn't look 
> appropriate to me. Would require following additional include files in 
> drivers/of/device.c as well.
> 
> +#include <linux/of_address.h>
> +#include <linux/of_iommu.h>
> +#include <linux/dma-mapping.h>

Yes, sounds good.

> 2. drivers/iommu/of_iommu.c. This is needed so that of_iommu_configure() 
> can take confuguration from the root bus DT as you have suggested.
> 
> -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;
> @@ -145,7 +145,7 @@ struct iommu_ops *of_iommu_configure(struct device *dev)
>           * 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)) {

Right.

> 3. drivers/of/of_pci.c. The existing function (added in this patch) will 
> make call to of_dma_configure() as
> 
> parent_np = of_get_pci_root_bridge_parent(pci_dev);
> of_dma_configure(dev, parent_np);

With dev = &pci_dev->dev, I assume?

> 4. drivers/of/platform.c. Add a wrapper function 
> of_platform_dma_configure() that calls of_dma_configure() as
> of_dma_configure(dev, dev->of_node). All existing calls converted to 
> this wrapper.

There are only two callers, I don't think we need a wrapper for it,
just change the calling conventions along with step 2.

> If the above looks good, I can post v3 of the patch with these changes.

With that one minor change, sounds perfect to me. The same can also
be used by other bus types if we ever need it.

	Arnd

Re: [PATCH v2 1/2] of/pci: add of_pci_dma_configure() update dma configuration

[Murali Karicheri]

On 01/02/2015 03:57 PM, Arnd Bergmann wrote:
> On Friday 02 January 2015 12:20:53 Murali Karicheri wrote:
>> Initially I had the same idea to re-use the existing function
>> of_dma_configure() for this. I wanted to defer this until we have an
>> agreement on the changes required for the subject functionality. My
>> quick review of the code suggestio this would require additional API
>> changes as below. I did a quick test of the changes and it works for
>> Keystone, but need to be reviewed by everyone as I touch the IOMMU
>> functionality here and I don't have a platform with IOMMU. Need test by
>> someone to make sure I don't break anything.
>>
>> Here are the changes required to implement this suggestion.
>>
>> 1. Move the of_dma_configure() to drivers/of/device.c (include the API
>> in of_device.h) and make it global (using proper EXPORT macro).
>> Otherwise, we will have to include of_platform.h in drivers/of/of_pci.c
>> assuming the prototype is defined in of_platform.h which doesn't look
>> appropriate to me. Would require following additional include files in
>> drivers/of/device.c as well.
>>
>> +#include<linux/of_address.h>
>> +#include<linux/of_iommu.h>
>> +#include<linux/dma-mapping.h>
>
> Yes, sounds good.
>
>> 2. drivers/iommu/of_iommu.c. This is needed so that of_iommu_configure()
>> can take confuguration from the root bus DT as you have suggested.
>>
>> -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;
>> @@ -145,7 +145,7 @@ struct iommu_ops *of_iommu_configure(struct device *dev)
>>            * 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)) {
>
> Right.
>
>> 3. drivers/of/of_pci.c. The existing function (added in this patch) will
>> make call to of_dma_configure() as
>>
>> parent_np = of_get_pci_root_bridge_parent(pci_dev);
>> of_dma_configure(dev, parent_np);
>
> With dev =&pci_dev->dev, I assume?

Yes

>
>> 4. drivers/of/platform.c. Add a wrapper function
>> of_platform_dma_configure() that calls of_dma_configure() as
>> of_dma_configure(dev, dev->of_node). All existing calls converted to
>> this wrapper.
>
> There are only two callers, I don't think we need a wrapper for it,
> just change the calling conventions along with step 2.

Ok. That is what Rob's comment as well.

>
>> If the above looks good, I can post v3 of the patch with these changes.
>
> With that one minor change, sounds perfect to me. The same can also
> be used by other bus types if we ever need it.
>
> 	Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Murali Karicheri
Linux Kernel, Texas Instruments
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] 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-l0cyMroinI0@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html