[PATCH v4 0/6] PCI: get DMA configuration from parent device

[PATCH v4 0/6] 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg790244.html
[1] : http://www.gossamer-threads.com/lists/linux/kernel/2024591

Change history:
	v4 - moved size adjustments in of_iommu_configure() to a separate patch
	   - consistent node name comment from Rob
	   - patch 6 added for dma_mask adjustment and iommu mapping size
	     limiting.
	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.

Cc: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>

Murali Karicheri (6): 
  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: fix size when dma-range is not used
  of/pci: add of_pci_dma_configure() update dma configuration
  PCI: update dma configuration from DT
  arm: dma-mapping: updates to limit dma_mask and iommu mapping size

 arch/arm/mm/dma-mapping.c |   10 +++++++
 drivers/iommu/of_iommu.c  |   10 +++++--
 drivers/of/device.c       |   71 +++++++++++++++++++++++++++++++++++++++++++++
 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 ++++++++
 9 files changed, 150 insertions(+), 60 deletions(-)

-- 
1.7.9.5

[PATCH v4 1/6] 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.

Cc: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>

Signed-off-by: Murali Karicheri <m-karicheri2-l0cyMroinI0@public.gmane.org>
---
 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..439235b 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 *iommu_np)
 {
 	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(iommu_np, "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..a97e5bd 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 *iommu_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 *iommu_np)
 {
 	return NULL;
 }
-- 
1.7.9.5

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

[Laurent Pinchart]

Hi Murali,

Thank you for the patch.

On Friday 23 January 2015 17:32:34 Murali Karicheri 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.
> 
> Cc: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
> Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>
> 
> Signed-off-by: Murali Karicheri <m-karicheri2-l0cyMroinI0@public.gmane.org>
> ---
>  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..439235b 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 *iommu_np)
>  {
>  	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
>  	 */

Wouldn't it be better to fix this rather than passing the device node pointer 
to the function ? The solution would be more generic.

> -	while (!of_parse_phandle_with_args(dev->of_node, "iommus",
> +	while (!of_parse_phandle_with_args(iommu_np, "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..a97e5bd 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 *iommu_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 *iommu_np)
>  {
>  	return NULL;
>  }

-- 
Regards,

Laurent Pinchart

--
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 v4 1/6] of: iommu: add ptr to OF node arg to of_iommu_configure()

[Murali Karicheri]

On 01/25/2015 08:32 AM, Laurent Pinchart wrote:
> Hi Murali,
>
> Thank you for the patch.
>
> On Friday 23 January 2015 17:32:34 Murali Karicheri 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.
>>
>> Cc: Joerg Roedel<joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
>> Cc: Grant Likely<grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Cc: Rob Herring<robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Cc: Bjorn Helgaas<bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>> Cc: Will Deacon<will.deacon-5wv7dgnIgG8@public.gmane.org>
>> Cc: Russell King<linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
>> Cc: Arnd Bergmann<arnd-r2nGTMty4D4@public.gmane.org>
>> Cc: Suravee Suthikulpanit<Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>
>>
>> Signed-off-by: Murali Karicheri<m-karicheri2-l0cyMroinI0@public.gmane.org>
>> ---
>>   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..439235b 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 *iommu_np)
>>   {
>>   	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
>>   	 */
> Wouldn't it be better to fix this rather than passing the device node pointer
> to the function ? The solution would be more generic.
Laurent,

Will Deacon (Copied here) is working on this as we alteady discussed in 
this thread. So it will be
addressed by him in another series.

Murali
>
>> -	while (!of_parse_phandle_with_args(dev->of_node, "iommus",
>> +	while (!of_parse_phandle_with_args(iommu_np, "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..a97e5bd 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 *iommu_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 *iommu_np)
>>   {
>>   	return NULL;
>>   }


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

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

[Will Deacon]

On Mon, Jan 26, 2015 at 06:49:01PM +0000, Murali Karicheri wrote:
> On 01/25/2015 08:32 AM, Laurent Pinchart wrote:
> > Hi Murali,
> >
> > Thank you for the patch.
> >
> > On Friday 23 January 2015 17:32:34 Murali Karicheri 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.
> >>
> >> Cc: Joerg Roedel<joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
> >> Cc: Grant Likely<grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >> Cc: Rob Herring<robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >> Cc: Bjorn Helgaas<bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> >> Cc: Will Deacon<will.deacon-5wv7dgnIgG8@public.gmane.org>
> >> Cc: Russell King<linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> >> Cc: Arnd Bergmann<arnd-r2nGTMty4D4@public.gmane.org>
> >> Cc: Suravee Suthikulpanit<Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>
> >>
> >> Signed-off-by: Murali Karicheri<m-karicheri2-l0cyMroinI0@public.gmane.org>
> >> ---
> >>   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..439235b 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 *iommu_np)
> >>   {
> >>   	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
> >>   	 */
> > Wouldn't it be better to fix this rather than passing the device node pointer
> > to the function ? The solution would be more generic.
> Laurent,
> 
> Will Deacon (Copied here) is working on this as we alteady discussed in 
> this thread. So it will be
> addressed by him in another series.

Well, "working on this" equates to "has it somewhere near the bottom of
a very long list" :)

Will

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

[Laurent Pinchart]

Hi Will,

On Wednesday 28 January 2015 11:33:00 Will Deacon wrote:
> On Mon, Jan 26, 2015 at 06:49:01PM +0000, Murali Karicheri wrote:
> > On 01/25/2015 08:32 AM, Laurent Pinchart wrote:
> >> On Friday 23 January 2015 17:32:34 Murali Karicheri 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.
> >>> 
> >>> Cc: Joerg Roedel<joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
> >>> Cc: Grant Likely<grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >>> Cc: Rob Herring<robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >>> Cc: Bjorn Helgaas<bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> >>> Cc: Will Deacon<will.deacon-5wv7dgnIgG8@public.gmane.org>
> >>> Cc: Russell King<linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> >>> Cc: Arnd Bergmann<arnd-r2nGTMty4D4@public.gmane.org>
> >>> Cc: Suravee Suthikulpanit<Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>
> >>> 
> >>> Signed-off-by: Murali Karicheri<m-karicheri2-l0cyMroinI0@public.gmane.org>
> >>> ---
> >>> 
> >>>   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..439235b 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 *iommu_np)
> >>>  {
> >>>   	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
> >>>   	 */
> >> 
> >> Wouldn't it be better to fix this rather than passing the device node
> >> pointer to the function ? The solution would be more generic.
> > 
> > Laurent,
> > 
> > Will Deacon (Copied here) is working on this as we alteady discussed in
> > this thread. So it will be addressed by him in another series.
> 
> Well, "working on this" equates to "has it somewhere near the bottom of
> a very long list" :)

What's your opinion on this patch then, do you think adding the iommu_np 
argument makes sense as a temporary hack, or should we instead walk up the 
tree looking for an iommus attribute in parent nodes ? I don't expect that to 
be insanely difficult to code.

-- 
Regards,

Laurent Pinchart

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

[Will Deacon]

On Wed, Jan 28, 2015 at 12:23:03PM +0000, Laurent Pinchart wrote:
> On Wednesday 28 January 2015 11:33:00 Will Deacon wrote:
> > On Mon, Jan 26, 2015 at 06:49:01PM +0000, Murali Karicheri wrote:
> > > On 01/25/2015 08:32 AM, Laurent Pinchart wrote:
> > >> On Friday 23 January 2015 17:32:34 Murali Karicheri 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.
> > >>> 
> > >>> Cc: Joerg Roedel<joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
> > >>> Cc: Grant Likely<grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > >>> Cc: Rob Herring<robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > >>> Cc: Bjorn Helgaas<bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> > >>> Cc: Will Deacon<will.deacon-5wv7dgnIgG8@public.gmane.org>
> > >>> Cc: Russell King<linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> > >>> Cc: Arnd Bergmann<arnd-r2nGTMty4D4@public.gmane.org>
> > >>> Cc: Suravee Suthikulpanit<Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>
> > >>> 
> > >>> Signed-off-by: Murali Karicheri<m-karicheri2-l0cyMroinI0@public.gmane.org>
> > >>> ---
> > >>> 
> > >>>   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..439235b 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 *iommu_np)
> > >>>  {
> > >>>   	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
> > >>>   	 */
> > >> 
> > >> Wouldn't it be better to fix this rather than passing the device node
> > >> pointer to the function ? The solution would be more generic.
> > > 
> > > Will Deacon (Copied here) is working on this as we alteady discussed in
> > > this thread. So it will be addressed by him in another series.
> > 
> > Well, "working on this" equates to "has it somewhere near the bottom of
> > a very long list" :)
> 
> What's your opinion on this patch then, do you think adding the iommu_np 
> argument makes sense as a temporary hack, or should we instead walk up the 
> tree looking for an iommus attribute in parent nodes ? I don't expect that to 
> be insanely difficult to code.

If walking up the tree is useful, then I think we should do that and update
the Documentation to reflect the new behaviour. The only reason that I
didn't code that in of_iommu_configure originally is because I didn't want
to go against the binding spec for the initial version, especially as we
didn't have a reason to look at parent nodes.

The only thing to double-check is that we don't break any existing users
of the binding with this change, but I don't think that we do.

Will

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

[Laurent Pinchart]

On Wednesday 28 January 2015 12:29:42 Will Deacon wrote:
> On Wed, Jan 28, 2015 at 12:23:03PM +0000, Laurent Pinchart wrote:
> > On Wednesday 28 January 2015 11:33:00 Will Deacon wrote:
> >> On Mon, Jan 26, 2015 at 06:49:01PM +0000, Murali Karicheri wrote:
> >>> On 01/25/2015 08:32 AM, Laurent Pinchart wrote:
> >>>> On Friday 23 January 2015 17:32:34 Murali Karicheri 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.
> >>>>> 
> >>>>> Cc: Joerg Roedel<joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
> >>>>> Cc: Grant Likely<grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >>>>> Cc: Rob Herring<robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >>>>> Cc: Bjorn Helgaas<bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> >>>>> Cc: Will Deacon<will.deacon-5wv7dgnIgG8@public.gmane.org>
> >>>>> Cc: Russell King<linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> >>>>> Cc: Arnd Bergmann<arnd-r2nGTMty4D4@public.gmane.org>
> >>>>> Cc: Suravee Suthikulpanit<Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>
> >>>>> 
> >>>>> Signed-off-by: Murali Karicheri<m-karicheri2-l0cyMroinI0@public.gmane.org>
> >>>>> ---
> >>>>> 
> >>>>>   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..439235b 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 *iommu_np)
> >>>>>  {
> >>>>>   	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
> >>>>>   	 */
> >>>> 
> >>>> Wouldn't it be better to fix this rather than passing the device node
> >>>> pointer to the function ? The solution would be more generic.
> >>> 
> >>> Will Deacon (Copied here) is working on this as we alteady discussed
> >>> in this thread. So it will be addressed by him in another series.
> >> 
> >> Well, "working on this" equates to "has it somewhere near the bottom of
> >> a very long list" :)
> > 
> > What's your opinion on this patch then, do you think adding the iommu_np
> > argument makes sense as a temporary hack, or should we instead walk up the
> > tree looking for an iommus attribute in parent nodes ? I don't expect that
> > to be insanely difficult to code.
> 
> If walking up the tree is useful, then I think we should do that and update
> the Documentation to reflect the new behaviour.

If I understand Murali's patch set right (please correct me if that's not the 
case) the PCI code walks up the DT nodes hierarchy to the parent node that 
contains the iommus attribute and passes a pointer to that node to this 
function. It's thus a PCI-specific solution. As a temporary hack that's OK I 
suppose, but if implementing it right straight away isn't difficult that would 
be better.

> The only reason that I didn't code that in of_iommu_configure originally is
> because I didn't want to go against the binding spec for the initial
> version, especially as we didn't have a reason to look at parent nodes.
> 
> The only thing to double-check is that we don't break any existing users
> of the binding with this change, but I don't think that we do.

-- 
Regards,

Laurent Pinchart

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

[Will Deacon]

On Wed, Jan 28, 2015 at 01:15:10PM +0000, Laurent Pinchart wrote:
> On Wednesday 28 January 2015 12:29:42 Will Deacon wrote:
> > On Wed, Jan 28, 2015 at 12:23:03PM +0000, Laurent Pinchart wrote:
> > > On Wednesday 28 January 2015 11:33:00 Will Deacon wrote:
> > >> On Mon, Jan 26, 2015 at 06:49:01PM +0000, Murali Karicheri wrote:
> > >>> On 01/25/2015 08:32 AM, Laurent Pinchart wrote:
> > >>>> On Friday 23 January 2015 17:32:34 Murali Karicheri 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.
> > >>>>> 
> > >>>>> Cc: Joerg Roedel<joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
> > >>>>> Cc: Grant Likely<grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > >>>>> Cc: Rob Herring<robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > >>>>> Cc: Bjorn Helgaas<bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> > >>>>> Cc: Will Deacon<will.deacon-5wv7dgnIgG8@public.gmane.org>
> > >>>>> Cc: Russell King<linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> > >>>>> Cc: Arnd Bergmann<arnd-r2nGTMty4D4@public.gmane.org>
> > >>>>> Cc: Suravee Suthikulpanit<Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>
> > >>>>> 
> > >>>>> Signed-off-by: Murali Karicheri<m-karicheri2-l0cyMroinI0@public.gmane.org>
> > >>>>> ---
> > >>>>> 
> > >>>>>   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..439235b 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 *iommu_np)
> > >>>>>  {
> > >>>>>   	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
> > >>>>>   	 */
> > >>>> 
> > >>>> Wouldn't it be better to fix this rather than passing the device node
> > >>>> pointer to the function ? The solution would be more generic.
> > >>> 
> > >>> Will Deacon (Copied here) is working on this as we alteady discussed
> > >>> in this thread. So it will be addressed by him in another series.
> > >> 
> > >> Well, "working on this" equates to "has it somewhere near the bottom of
> > >> a very long list" :)
> > > 
> > > What's your opinion on this patch then, do you think adding the iommu_np
> > > argument makes sense as a temporary hack, or should we instead walk up the
> > > tree looking for an iommus attribute in parent nodes ? I don't expect that
> > > to be insanely difficult to code.
> > 
> > If walking up the tree is useful, then I think we should do that and update
> > the Documentation to reflect the new behaviour.
> 
> If I understand Murali's patch set right (please correct me if that's not the 
> case) the PCI code walks up the DT nodes hierarchy to the parent node that 
> contains the iommus attribute and passes a pointer to that node to this 
> function. It's thus a PCI-specific solution. As a temporary hack that's OK I 
> suppose, but if implementing it right straight away isn't difficult that would 
> be better.

It looks to me like the code walks the PCI topology to get the DT node for
the host controller, and passes *that* to of_dma_configure. That sounds
like the right thing to do to me, especially since the PCI topology is
likely not encoded in the device-tree. So actually, it is passing the
first parent node afaict.

The part I'm more interested in is the mapping of RequesterID (PCI dma
alias) to IOMMU ID when we transition from the PCI topology to the DT
topology. Currently, it seems to be 1:1 on the platforms I have, but I
doubt this will always be the case.

Will

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

[Murali Karicheri]

On 01/28/2015 08:32 AM, Will Deacon wrote:
> On Wed, Jan 28, 2015 at 01:15:10PM +0000, Laurent Pinchart wrote:
>> On Wednesday 28 January 2015 12:29:42 Will Deacon wrote:
>>> On Wed, Jan 28, 2015 at 12:23:03PM +0000, Laurent Pinchart wrote:
>>>> On Wednesday 28 January 2015 11:33:00 Will Deacon wrote:
>>>>> On Mon, Jan 26, 2015 at 06:49:01PM +0000, Murali Karicheri wrote:
>>>>>> On 01/25/2015 08:32 AM, Laurent Pinchart wrote:
>>>>>>> On Friday 23 January 2015 17:32:34 Murali Karicheri 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.
>>>>>>>>
>>>>>>>> Cc: Joerg Roedel<joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
>>>>>>>> Cc: Grant Likely<grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>>>>>> Cc: Rob Herring<robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>>>>>>> Cc: Bjorn Helgaas<bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>>>>>>>> Cc: Will Deacon<will.deacon-5wv7dgnIgG8@public.gmane.org>
>>>>>>>> Cc: Russell King<linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
>>>>>>>> Cc: Arnd Bergmann<arnd-r2nGTMty4D4@public.gmane.org>
>>>>>>>> Cc: Suravee Suthikulpanit<Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>
>>>>>>>>
>>>>>>>> Signed-off-by: Murali Karicheri<m-karicheri2-l0cyMroinI0@public.gmane.org>
>>>>>>>> ---
>>>>>>>>
>>>>>>>>    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..439235b 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 *iommu_np)
>>>>>>>>   {
>>>>>>>>    	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
>>>>>>>>    	 */
>>>>>>>
>>>>>>> Wouldn't it be better to fix this rather than passing the device node
>>>>>>> pointer to the function ? The solution would be more generic.
>>>>>>
>>>>>> Will Deacon (Copied here) is working on this as we alteady discussed
>>>>>> in this thread. So it will be addressed by him in another series.
>>>>>
>>>>> Well, "working on this" equates to "has it somewhere near the bottom of
>>>>> a very long list" :)
>>>>
>>>> What's your opinion on this patch then, do you think adding the iommu_np
>>>> argument makes sense as a temporary hack, or should we instead walk up the
>>>> tree looking for an iommus attribute in parent nodes ? I don't expect that
>>>> to be insanely difficult to code.
>>>
>>> If walking up the tree is useful, then I think we should do that and update
>>> the Documentation to reflect the new behaviour.
>>
>> If I understand Murali's patch set right (please correct me if that's not the
>> case) the PCI code walks up the DT nodes hierarchy to the parent node that
>> contains the iommus attribute and passes a pointer to that node to this
>> function. It's thus a PCI-specific solution. As a temporary hack that's OK I
>> suppose, but if implementing it right straight away isn't difficult that would
>> be better.
>
> It looks to me like the code walks the PCI topology to get the DT node for
> the host controller, and passes *that* to of_dma_configure. That sounds
> like the right thing to do to me, especially since the PCI topology is
> likely not encoded in the device-tree. So actually, it is passing the
> first parent node afaict.
>
Laurent, Will,

I don't have an IOMMU hardware to do more work on this. My patch series 
has been on this list for long and it is also increasing in scope :(

I would suggest a follow up patch to this from someone (probably Will) 
and my patch goes as is with out further update. Hope this is reasonable.

Murali
> The part I'm more interested in is the mapping of RequesterID (PCI dma
> alias) to IOMMU ID when we transition from the PCI topology to the DT
> topology. Currently, it seems to be 1:1 on the platforms I have, but I
> doubt this will always be the case.
>
> Will


-- 
Murali Karicheri
Linux Kernel, Texas Instruments

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

[Laurent Pinchart]

Hi Will,

On Wednesday 28 January 2015 13:32:19 Will Deacon wrote:
> On Wed, Jan 28, 2015 at 01:15:10PM +0000, Laurent Pinchart wrote:
> > On Wednesday 28 January 2015 12:29:42 Will Deacon wrote:
> >> On Wed, Jan 28, 2015 at 12:23:03PM +0000, Laurent Pinchart wrote:
> >>> On Wednesday 28 January 2015 11:33:00 Will Deacon wrote:
> >>>> On Mon, Jan 26, 2015 at 06:49:01PM +0000, Murali Karicheri wrote:
> >>>>> On 01/25/2015 08:32 AM, Laurent Pinchart wrote:
> >>>>>> On Friday 23 January 2015 17:32:34 Murali Karicheri 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.
> >>>>>>> 
> >>>>>>> Cc: Joerg Roedel<joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
> >>>>>>> Cc: Grant Likely<grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >>>>>>> Cc: Rob Herring<robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >>>>>>> Cc: Bjorn Helgaas<bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> >>>>>>> Cc: Will Deacon<will.deacon-5wv7dgnIgG8@public.gmane.org>
> >>>>>>> Cc: Russell King<linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> >>>>>>> Cc: Arnd Bergmann<arnd-r2nGTMty4D4@public.gmane.org>
> >>>>>>> Cc: Suravee Suthikulpanit<Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>
> >>>>>>> 
> >>>>>>> Signed-off-by: Murali Karicheri<m-karicheri2-l0cyMroinI0@public.gmane.org>
> >>>>>>> ---
> >>>>>>> 
> >>>>>>>   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..439235b 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 *iommu_np)
> >>>>>>>  {
> >>>>>>>   	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
> >>>>>>>   	 */
> >>>>>> 
> >>>>>> Wouldn't it be better to fix this rather than passing the device
> >>>>>> node pointer to the function ? The solution would be more generic.
> >>>>> 
> >>>>> Will Deacon (Copied here) is working on this as we alteady discussed
> >>>>> in this thread. So it will be addressed by him in another series.
> >>>> 
> >>>> Well, "working on this" equates to "has it somewhere near the bottom
> >>>> of a very long list" :)
> >>> 
> >>> What's your opinion on this patch then, do you think adding the
> >>> iommu_np argument makes sense as a temporary hack, or should we instead
> >>> walk up the tree looking for an iommus attribute in parent nodes ? I
> >>> don't expect that to be insanely difficult to code.
> >> 
> >> If walking up the tree is useful, then I think we should do that and
> >> update the Documentation to reflect the new behaviour.
> > 
> > If I understand Murali's patch set right (please correct me if that's not
> > the case) the PCI code walks up the DT nodes hierarchy to the parent node
> > that contains the iommus attribute and passes a pointer to that node to
> > this function. It's thus a PCI-specific solution. As a temporary hack
> > that's OK I suppose, but if implementing it right straight away isn't
> > difficult that would be better.
> 
> It looks to me like the code walks the PCI topology to get the DT node for
> the host controller, and passes *that* to of_dma_configure. That sounds
> like the right thing to do to me, especially since the PCI topology is
> likely not encoded in the device-tree. So actually, it is passing the
> first parent node afaict.

Indeed, that's right. I forgot for a moment that we have non-DT devices ;-)

Acked-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>

Murali, nitpicking a bit, shouldn't the iommu_np parameter be renamed ? It 
points to the node containing the iommus parameter, not to the iommu node, so 
the current name is slightly confusing. A brief kerneldoc above the function 
would also help. This can be the subject of a separate patch.

> The part I'm more interested in is the mapping of RequesterID (PCI dma
> alias) to IOMMU ID when we transition from the PCI topology to the DT
> topology. Currently, it seems to be 1:1 on the platforms I have, but I
> doubt this will always be the case.

-- 
Regards,

Laurent Pinchart

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

[Murali Karicheri]

On 01/28/2015 06:32 PM, Laurent Pinchart wrote:
> Hi Will,
>
> On Wednesday 28 January 2015 13:32:19 Will Deacon wrote:
>> On Wed, Jan 28, 2015 at 01:15:10PM +0000, Laurent Pinchart wrote:
>>> On Wednesday 28 January 2015 12:29:42 Will Deacon wrote:
>>>> On Wed, Jan 28, 2015 at 12:23:03PM +0000, Laurent Pinchart wrote:
>>>>> On Wednesday 28 January 2015 11:33:00 Will Deacon wrote:
>>>>>> On Mon, Jan 26, 2015 at 06:49:01PM +0000, Murali Karicheri wrote:
>>>>>>> On 01/25/2015 08:32 AM, Laurent Pinchart wrote:
>>>>>>>> On Friday 23 January 2015 17:32:34 Murali Karicheri 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.
>>>>>>>>>
>>>>>>>>> Cc: Joerg Roedel<joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
>>>>>>>>> Cc: Grant Likely<grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>>>>>>> Cc: Rob Herring<robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>>>>>>>> Cc: Bjorn Helgaas<bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>>>>>>>>> Cc: Will Deacon<will.deacon-5wv7dgnIgG8@public.gmane.org>
>>>>>>>>> Cc: Russell King<linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
>>>>>>>>> Cc: Arnd Bergmann<arnd-r2nGTMty4D4@public.gmane.org>
>>>>>>>>> Cc: Suravee Suthikulpanit<Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>
>>>>>>>>>
>>>>>>>>> Signed-off-by: Murali Karicheri<m-karicheri2-l0cyMroinI0@public.gmane.org>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>>    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..439235b 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 *iommu_np)
>>>>>>>>>   {
>>>>>>>>>    	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
>>>>>>>>>    	 */
>>>>>>>>
>>>>>>>> Wouldn't it be better to fix this rather than passing the device
>>>>>>>> node pointer to the function ? The solution would be more generic.
>>>>>>>
>>>>>>> Will Deacon (Copied here) is working on this as we alteady discussed
>>>>>>> in this thread. So it will be addressed by him in another series.
>>>>>>
>>>>>> Well, "working on this" equates to "has it somewhere near the bottom
>>>>>> of a very long list" :)
>>>>>
>>>>> What's your opinion on this patch then, do you think adding the
>>>>> iommu_np argument makes sense as a temporary hack, or should we instead
>>>>> walk up the tree looking for an iommus attribute in parent nodes ? I
>>>>> don't expect that to be insanely difficult to code.
>>>>
>>>> If walking up the tree is useful, then I think we should do that and
>>>> update the Documentation to reflect the new behaviour.
>>>
>>> If I understand Murali's patch set right (please correct me if that's not
>>> the case) the PCI code walks up the DT nodes hierarchy to the parent node
>>> that contains the iommus attribute and passes a pointer to that node to
>>> this function. It's thus a PCI-specific solution. As a temporary hack
>>> that's OK I suppose, but if implementing it right straight away isn't
>>> difficult that would be better.
>>
>> It looks to me like the code walks the PCI topology to get the DT node for
>> the host controller, and passes *that* to of_dma_configure. That sounds
>> like the right thing to do to me, especially since the PCI topology is
>> likely not encoded in the device-tree. So actually, it is passing the
>> first parent node afaict.
>
> Indeed, that's right. I forgot for a moment that we have non-DT devices ;-)
>
> Acked-by: Laurent Pinchart<laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
>
> Murali, nitpicking a bit, shouldn't the iommu_np parameter be renamed ? It
> points to the node containing the iommus parameter, not to the iommu node, so
> the current name is slightly confusing. A brief kerneldoc above the function
> would also help. This can be the subject of a separate patch.
Probably the doc part can be added by Will. The iommu_np was a 
suggestion from Rob, if I recollect. Isn't the iommu_np points to the 
iommu device, and if so, iommu_np make sense.

Murali
>
>> The part I'm more interested in is the mapping of RequesterID (PCI dma
>> alias) to IOMMU ID when we transition from the PCI topology to the DT
>> topology. Currently, it seems to be 1:1 on the platforms I have, but I
>> doubt this will always be the case.
>


-- 
Murali Karicheri
Linux Kernel, Texas Instruments

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

[Rob Herring]

On Wed, Jan 28, 2015 at 5:32 PM, Laurent Pinchart
<laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> wrote:
> Hi Will,
>
> On Wednesday 28 January 2015 13:32:19 Will Deacon wrote:
>> On Wed, Jan 28, 2015 at 01:15:10PM +0000, Laurent Pinchart wrote:
>> > On Wednesday 28 January 2015 12:29:42 Will Deacon wrote:
>> >> On Wed, Jan 28, 2015 at 12:23:03PM +0000, Laurent Pinchart wrote:
>> >>> On Wednesday 28 January 2015 11:33:00 Will Deacon wrote:
>> >>>> On Mon, Jan 26, 2015 at 06:49:01PM +0000, Murali Karicheri wrote:
>> >>>>> On 01/25/2015 08:32 AM, Laurent Pinchart wrote:
>> >>>>>> On Friday 23 January 2015 17:32:34 Murali Karicheri 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.

[...]

>> > If I understand Murali's patch set right (please correct me if that's not
>> > the case) the PCI code walks up the DT nodes hierarchy to the parent node
>> > that contains the iommus attribute and passes a pointer to that node to
>> > this function. It's thus a PCI-specific solution. As a temporary hack
>> > that's OK I suppose, but if implementing it right straight away isn't
>> > difficult that would be better.
>>
>> It looks to me like the code walks the PCI topology to get the DT node for
>> the host controller, and passes *that* to of_dma_configure. That sounds
>> like the right thing to do to me, especially since the PCI topology is
>> likely not encoded in the device-tree. So actually, it is passing the
>> first parent node afaict.
>
> Indeed, that's right. I forgot for a moment that we have non-DT devices ;-)
>
> Acked-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
>
> Murali, nitpicking a bit, shouldn't the iommu_np parameter be renamed ? It
> points to the node containing the iommus parameter, not to the iommu node, so
> the current name is slightly confusing. A brief kerneldoc above the function
> would also help. This can be the subject of a separate patch.

It was more confusing having np and node within the function.

Rob

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

[Laurent Pinchart]

Hi Rob,

On Thursday 29 January 2015 10:49:38 Rob Herring wrote:
> On Wed, Jan 28, 2015 at 5:32 PM, Laurent Pinchart wrote:
> > On Wednesday 28 January 2015 13:32:19 Will Deacon wrote:
> >> On Wed, Jan 28, 2015 at 01:15:10PM +0000, Laurent Pinchart wrote:
> >>> On Wednesday 28 January 2015 12:29:42 Will Deacon wrote:
> >>>> On Wed, Jan 28, 2015 at 12:23:03PM +0000, Laurent Pinchart wrote:
> >>>>> On Wednesday 28 January 2015 11:33:00 Will Deacon wrote:
> >>>>>> On Mon, Jan 26, 2015 at 06:49:01PM +0000, Murali Karicheri wrote:
> >>>>>> On 01/25/2015 08:32 AM, Laurent Pinchart wrote:
> >>>>>>>> On Friday 23 January 2015 17:32:34 Murali Karicheri 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.
> 
> [...]
> 
> >>> If I understand Murali's patch set right (please correct me if that's
> >>> not the case) the PCI code walks up the DT nodes hierarchy to the
> >>> parent node that contains the iommus attribute and passes a pointer to
> >>> that node to this function. It's thus a PCI-specific solution. As a
> >>> temporary hack that's OK I suppose, but if implementing it right
> >>> straight away isn't difficult that would be better.
> >> 
> >> It looks to me like the code walks the PCI topology to get the DT node
> >> for the host controller, and passes *that* to of_dma_configure. That
> >> sounds like the right thing to do to me, especially since the PCI
> >> topology is likely not encoded in the device-tree. So actually, it is
> >> passing the first parent node afaict.
> > 
> > Indeed, that's right. I forgot for a moment that we have non-DT devices
> > ;-)
> > 
> > Acked-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> > 
> > Murali, nitpicking a bit, shouldn't the iommu_np parameter be renamed ? It
> > points to the node containing the iommus parameter, not to the iommu node,
> > so the current name is slightly confusing. A brief kerneldoc above the
> > function would also help. This can be the subject of a separate patch.
> 
> It was more confusing having np and node within the function.

Agreed.

How about master_np or iommu_master_np ? The latter might be a bit long.

-- 
Regards,

Laurent Pinchart

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

[Murali Karicheri]

On 01/29/2015 07:24 PM, Laurent Pinchart wrote:
> Hi Rob,
>
> On Thursday 29 January 2015 10:49:38 Rob Herring wrote:
>> On Wed, Jan 28, 2015 at 5:32 PM, Laurent Pinchart wrote:
>>> On Wednesday 28 January 2015 13:32:19 Will Deacon wrote:
>>>> On Wed, Jan 28, 2015 at 01:15:10PM +0000, Laurent Pinchart wrote:
>>>>> On Wednesday 28 January 2015 12:29:42 Will Deacon wrote:
>>>>>> On Wed, Jan 28, 2015 at 12:23:03PM +0000, Laurent Pinchart wrote:
>>>>>>> On Wednesday 28 January 2015 11:33:00 Will Deacon wrote:
>>>>>>>> On Mon, Jan 26, 2015 at 06:49:01PM +0000, Murali Karicheri wrote:
>>>>>>>> On 01/25/2015 08:32 AM, Laurent Pinchart wrote:
>>>>>>>>>> On Friday 23 January 2015 17:32:34 Murali Karicheri 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.
>>
>> [...]
>>
>>>>> If I understand Murali's patch set right (please correct me if that's
>>>>> not the case) the PCI code walks up the DT nodes hierarchy to the
>>>>> parent node that contains the iommus attribute and passes a pointer to
>>>>> that node to this function. It's thus a PCI-specific solution. As a
>>>>> temporary hack that's OK I suppose, but if implementing it right
>>>>> straight away isn't difficult that would be better.
>>>>
>>>> It looks to me like the code walks the PCI topology to get the DT node
>>>> for the host controller, and passes *that* to of_dma_configure. That
>>>> sounds like the right thing to do to me, especially since the PCI
>>>> topology is likely not encoded in the device-tree. So actually, it is
>>>> passing the first parent node afaict.
>>>
>>> Indeed, that's right. I forgot for a moment that we have non-DT devices
>>> ;-)
>>>
>>> Acked-by: Laurent Pinchart<laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
>>>
>>> Murali, nitpicking a bit, shouldn't the iommu_np parameter be renamed ? It
>>> points to the node containing the iommus parameter, not to the iommu node,
>>> so the current name is slightly confusing. A brief kerneldoc above the
>>> function would also help. This can be the subject of a separate patch.
>>
>> It was more confusing having np and node within the function.
>
> Agreed.
>
> How about master_np or iommu_master_np ? The latter might be a bit long.
>
Probabaly master_np suffice as this function is named 
of_iommu_configure(). I will update it.

-- 
Murali Karicheri
Linux Kernel, Texas Instruments

[PATCH v4 2/6] 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.

Cc: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>

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

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 46d6c75c..2de320d 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,62 @@ 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;
+	} 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 v4 4/6] 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.

Cc: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>

Signed-off-by: Murali Karicheri <m-karicheri2-l0cyMroinI0@public.gmane.org>
---
 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

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

[Bjorn Helgaas]

On Fri, Jan 23, 2015 at 05:32:37PM -0500, 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.
> 
> Cc: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
> Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>
> 
> Signed-off-by: Murali Karicheri <m-karicheri2-l0cyMroinI0@public.gmane.org>
> ---
>  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)

I'm not an OF person, but this interface seems like it might be too
special-purpose.  Maybe it would be enough to add
"of_get_pci_root_bridge()", and the caller could do this:

    struct device *bridge = of_get_pci_root_bridge(dev);
    struct device_node *parent_np = bridge->parent->of_node;

Also, the name "of_get_..." suggests that it increments a refcount, as
of_get_parent() does.  But you aren't doing anything with the refcount.

But I guess an "of_get_pci_root_bridge()" isn't doing anything OF-related,
so maybe we should just add a "pci_get_host_bridge(struct pci_dev *)"
to PCI instead.

Bjorn

> +{
> +	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
> 
--
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 v4 4/6] of/pci: add of_pci_dma_configure() update dma configuration

[Murali Karicheri]

On 01/23/2015 06:41 PM, Bjorn Helgaas wrote:
> On Fri, Jan 23, 2015 at 05:32:37PM -0500, 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.
>>
>> Cc: Joerg Roedel<joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
>> Cc: Grant Likely<grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Cc: Rob Herring<robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Cc: Bjorn Helgaas<bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>> Cc: Will Deacon<will.deacon-5wv7dgnIgG8@public.gmane.org>
>> Cc: Russell King<linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
>> Cc: Arnd Bergmann<arnd-r2nGTMty4D4@public.gmane.org>
>> Cc: Suravee Suthikulpanit<Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>
>>
>> Signed-off-by: Murali Karicheri<m-karicheri2-l0cyMroinI0@public.gmane.org>
>> ---
>>   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)
>
> I'm not an OF person, but this interface seems like it might be too
> special-purpose.  Maybe it would be enough to add
> "of_get_pci_root_bridge()", and the caller could do this:
>
>      struct device *bridge = of_get_pci_root_bridge(dev);
>      struct device_node *parent_np = bridge->parent->of_node;
>
> Also, the name "of_get_..." suggests that it increments a refcount, as
> of_get_parent() does.  But you aren't doing anything with the refcount.
>
> But I guess an "of_get_pci_root_bridge()" isn't doing anything OF-related,
> so maybe we should just add a "pci_get_host_bridge(struct pci_dev *)"
> to PCI instead.

Bjorn,

Thanks for the comment.

I think adding pci_get_host_bridge() is a good idea. There is already 
similar function in host-bridge.c. I have added this function re-using 
existing function find_pci_root_bus(). See the incremental diff below 
after this change. Does this look good?

diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
index 34878c9..77b15b5 100644
--- a/drivers/of/of_pci.c
+++ b/drivers/of/of_pci.c
@@ -232,26 +232,6 @@ parse_failed:
  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
   *
@@ -261,10 +241,9 @@ EXPORT_SYMBOL_GPL(of_get_pci_root_bridge_parent);
  void of_pci_dma_configure(struct pci_dev *pci_dev)
  {
         struct device *dev = &pci_dev->dev;
-       struct device_node *parent_np;
+       struct device *bridge = pci_get_host_bridge(pci_dev);

-       parent_np = of_get_pci_root_bridge_parent(pci_dev);
-       of_dma_configure(dev, parent_np);
+       of_dma_configure(dev, bridge->parent->of_node);
  }
  EXPORT_SYMBOL_GPL(of_pci_dma_configure);

diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
index 0e5f3c9..9803aa6 100644
--- a/drivers/pci/host-bridge.c
+++ b/drivers/pci/host-bridge.c
@@ -23,6 +23,13 @@ static struct pci_host_bridge 
*find_pci_host_bridge(struct pci_bus *bus)
         return to_pci_host_bridge(root_bus->bridge);
  }

+struct device *pci_get_host_bridge(struct pci_dev *dev)
+{
+       struct pci_bus *root_bus = find_pci_root_bus(dev->bus);
+
+       return root_bus->bridge;
+}
+
  void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
                                  void (*release_fn)(struct 
pci_host_bridge *),
                                  void *release_data)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 9603094..5bcdfa6 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -513,6 +513,8 @@ static inline struct pci_dev 
*pci_upstream_bridge(struct pci_dev *dev)
         return dev->bus->self;
  }

+struct device *pci_get_host_bridge(struct pci_dev *dev);
+
  #ifdef CONFIG_PCI_MSI
  static inline bool pci_dev_msi_enabled(struct pci_dev *pci_dev)
  {
@@ -1823,6 +1825,7 @@ int pci_vpd_find_tag(const u8 *buf, unsigned int 
off, unsigned int len, u8 rdt);
  int pci_vpd_find_info_keyword(const u8 *buf, unsigned int off,
                               unsigned int len, const char *kw);

  /* PCI <-> OF binding helpers */
  #ifdef CONFIG_OF
  struct device_node;


>
> Bjorn
>
>> +{
>> +	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
>>


-- 
Murali Karicheri
Linux Kernel, Texas Instruments

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

[Bjorn Helgaas]

On Mon, Jan 26, 2015 at 5:25 PM, Murali Karicheri <m-karicheri2-l0cyMroinI0@public.gmane.org> wrote:
> On 01/23/2015 06:41 PM, Bjorn Helgaas wrote:
>>
>> On Fri, Jan 23, 2015 at 05:32:37PM -0500, 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.
>>>
>>> Cc: Joerg Roedel<joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
>>> Cc: Grant Likely<grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>> Cc: Rob Herring<robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>> Cc: Bjorn Helgaas<bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>>> Cc: Will Deacon<will.deacon-5wv7dgnIgG8@public.gmane.org>
>>> Cc: Russell King<linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
>>> Cc: Arnd Bergmann<arnd-r2nGTMty4D4@public.gmane.org>
>>> Cc: Suravee Suthikulpanit<Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>
>>>
>>> Signed-off-by: Murali Karicheri<m-karicheri2-l0cyMroinI0@public.gmane.org>
>>> ---
>>>   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)
>>
>>
>> I'm not an OF person, but this interface seems like it might be too
>> special-purpose.  Maybe it would be enough to add
>> "of_get_pci_root_bridge()", and the caller could do this:
>>
>>      struct device *bridge = of_get_pci_root_bridge(dev);
>>      struct device_node *parent_np = bridge->parent->of_node;
>>
>> Also, the name "of_get_..." suggests that it increments a refcount, as
>> of_get_parent() does.  But you aren't doing anything with the refcount.
>>
>> But I guess an "of_get_pci_root_bridge()" isn't doing anything OF-related,
>> so maybe we should just add a "pci_get_host_bridge(struct pci_dev *)"
>> to PCI instead.
>
>
> Bjorn,
>
> Thanks for the comment.
>
> I think adding pci_get_host_bridge() is a good idea. There is already
> similar function in host-bridge.c. I have added this function re-using
> existing function find_pci_root_bus(). See the incremental diff below after
> this change. Does this look good?

I like the implementation, but I think either we need to take a
reference on the host bridge, or change the name to  something like
"pci_find_host_bridge()", because using "_get_" is conventional for
functions that acquire a reference.

Since host bridges are hot-pluggable, at least in theory, I vote for
taking a reference.  Then of course, you'd have to add code to drop
the reference when you're finished with it.

Bjorn

> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
> index 34878c9..77b15b5 100644
> --- a/drivers/of/of_pci.c
> +++ b/drivers/of/of_pci.c
> @@ -232,26 +232,6 @@ parse_failed:
>  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
>   *
> @@ -261,10 +241,9 @@ EXPORT_SYMBOL_GPL(of_get_pci_root_bridge_parent);
>  void of_pci_dma_configure(struct pci_dev *pci_dev)
>  {
>         struct device *dev = &pci_dev->dev;
> -       struct device_node *parent_np;
> +       struct device *bridge = pci_get_host_bridge(pci_dev);
>
> -       parent_np = of_get_pci_root_bridge_parent(pci_dev);
> -       of_dma_configure(dev, parent_np);
> +       of_dma_configure(dev, bridge->parent->of_node);
>  }
>  EXPORT_SYMBOL_GPL(of_pci_dma_configure);
>
> diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
> index 0e5f3c9..9803aa6 100644
> --- a/drivers/pci/host-bridge.c
> +++ b/drivers/pci/host-bridge.c
> @@ -23,6 +23,13 @@ static struct pci_host_bridge
> *find_pci_host_bridge(struct pci_bus *bus)
>         return to_pci_host_bridge(root_bus->bridge);
>  }
>
> +struct device *pci_get_host_bridge(struct pci_dev *dev)
> +{
> +       struct pci_bus *root_bus = find_pci_root_bus(dev->bus);
> +
> +       return root_bus->bridge;
> +}
> +
>  void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
>                                  void (*release_fn)(struct pci_host_bridge
> *),
>                                  void *release_data)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 9603094..5bcdfa6 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -513,6 +513,8 @@ static inline struct pci_dev *pci_upstream_bridge(struct
> pci_dev *dev)
>         return dev->bus->self;
>  }
>
> +struct device *pci_get_host_bridge(struct pci_dev *dev);
> +
>  #ifdef CONFIG_PCI_MSI
>  static inline bool pci_dev_msi_enabled(struct pci_dev *pci_dev)
>  {
> @@ -1823,6 +1825,7 @@ int pci_vpd_find_tag(const u8 *buf, unsigned int off,
> unsigned int len, u8 rdt);
>  int pci_vpd_find_info_keyword(const u8 *buf, unsigned int off,
>                               unsigned int len, const char *kw);
>
>  /* PCI <-> OF binding helpers */
>  #ifdef CONFIG_OF
>  struct device_node;
>
>
>>
>> Bjorn
>>
>>> +{
>>> +       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
>>>
>
>
> --
> Murali Karicheri
> Linux Kernel, Texas Instruments

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

[Murali Karicheri]

On 01/26/2015 06:59 PM, Bjorn Helgaas wrote:
> On Mon, Jan 26, 2015 at 5:25 PM, Murali Karicheri<m-karicheri2-l0cyMroinI0@public.gmane.org>  wrote:
>> On 01/23/2015 06:41 PM, Bjorn Helgaas wrote:
>>>
>>> On Fri, Jan 23, 2015 at 05:32:37PM -0500, 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.
>>>>
   -- Cut ---
>>>>
>>>> Signed-off-by: Murali Karicheri<m-karicheri2-l0cyMroinI0@public.gmane.org>
>>>> ---
>>>>    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)
>>>
>>>
>>> I'm not an OF person, but this interface seems like it might be too
>>> special-purpose.  Maybe it would be enough to add
>>> "of_get_pci_root_bridge()", and the caller could do this:
>>>
>>>       struct device *bridge = of_get_pci_root_bridge(dev);
>>>       struct device_node *parent_np = bridge->parent->of_node;
>>>
>>> Also, the name "of_get_..." suggests that it increments a refcount, as
>>> of_get_parent() does.  But you aren't doing anything with the refcount.
>>>
>>> But I guess an "of_get_pci_root_bridge()" isn't doing anything OF-related,
>>> so maybe we should just add a "pci_get_host_bridge(struct pci_dev *)"
>>> to PCI instead.
>>
>>
>> Bjorn,
>>
>> Thanks for the comment.
>>
>> I think adding pci_get_host_bridge() is a good idea. There is already
>> similar function in host-bridge.c. I have added this function re-using
>> existing function find_pci_root_bus(). See the incremental diff below after
>> this change. Does this look good?
>
> I like the implementation, but I think either we need to take a
> reference on the host bridge, or change the name to  something like
> "pci_find_host_bridge()", because using "_get_" is conventional for
> functions that acquire a reference.
>
> Since host bridges are hot-pluggable, at least in theory, I vote for
> taking a reference.  Then of course, you'd have to add code to drop
> the reference when you're finished with it.
>
Bjorn,

Thanks. I agree with your suggestion even though the convention is not 
followed fully :) of_pci_get_devfn(), of_get_pci_domain_nr(), 
of_pci_get_host_bridge_resources() are some of those functions not 
following the convention. I plan to change the function as below. Also 
want to name functions as pci_get/put_host_bridge_device() as existing 
function find_pci_host_bridge() is actually returning ptr to struct 
pci_host_bridge  vs the new function returning ptr to device. Here are 
the new functions and how they will be used. Please review and respond 
so that I can avoid a re-spin.

in linux/include/pci.h add the prototypes of 
pci_get/put_host_bridge_device().

in drivers/pci/host-bridge.c add two new functions.

struct device *pci_get_host_bridge_device(struct pci_dev *dev)
{
	struct pci_bus *root_bus = find_pci_root_bus(dev->bus);
	struct device *bridge = root_bus->bridge;

	kobject_get(&bridge->kobj);
	return bridge;
}

void  pci_put_host_bridge_device(struct pci_dev *dev)
{
	struct pci_bus *root_bus = find_pci_root_bus(dev->bus);
	struct device *bridge = root_bus->bridge;

	kobject_put(&bridge->kobj);
}

drivers/of/of_pci.c

void of_pci_dma_configure(struct pci_dev *pci_dev)
{
	struct device *dev = &pci_dev->dev;
	struct device *bridge = pci_get_host_bridge_device(pci_dev);

	of_dma_configure(dev, bridge->parent->of_node);
	pci_put_host_bridge_device(pci_dev);
}

Murali

> Bjorn
>

>>>>
>>
>>
>> --
>> Murali Karicheri
>> Linux Kernel, Texas Instruments


-- 
Murali Karicheri
Linux Kernel, Texas Instruments

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

[Bjorn Helgaas]

On Tue, Jan 27, 2015 at 12:14 PM, Murali Karicheri <m-karicheri2-l0cyMroinI0@public.gmane.org> wrote:
> On 01/26/2015 06:59 PM, Bjorn Helgaas wrote:
>>
>> On Mon, Jan 26, 2015 at 5:25 PM, Murali Karicheri<m-karicheri2-l0cyMroinI0@public.gmane.org>
>> wrote:
>>>
>>> On 01/23/2015 06:41 PM, Bjorn Helgaas wrote:
>>>>
>>>>
>>>> On Fri, Jan 23, 2015 at 05:32:37PM -0500, 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.
>>>>>
>   -- Cut ---
>
>>>>>
>>>>> Signed-off-by: Murali Karicheri<m-karicheri2-l0cyMroinI0@public.gmane.org>
>>>>> ---
>>>>>    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)
>>>>
>>>>
>>>>
>>>> I'm not an OF person, but this interface seems like it might be too
>>>> special-purpose.  Maybe it would be enough to add
>>>> "of_get_pci_root_bridge()", and the caller could do this:
>>>>
>>>>       struct device *bridge = of_get_pci_root_bridge(dev);
>>>>       struct device_node *parent_np = bridge->parent->of_node;
>>>>
>>>> Also, the name "of_get_..." suggests that it increments a refcount, as
>>>> of_get_parent() does.  But you aren't doing anything with the refcount.
>>>>
>>>> But I guess an "of_get_pci_root_bridge()" isn't doing anything
>>>> OF-related,
>>>> so maybe we should just add a "pci_get_host_bridge(struct pci_dev *)"
>>>> to PCI instead.
>>>
>>>
>>>
>>> Bjorn,
>>>
>>> Thanks for the comment.
>>>
>>> I think adding pci_get_host_bridge() is a good idea. There is already
>>> similar function in host-bridge.c. I have added this function re-using
>>> existing function find_pci_root_bus(). See the incremental diff below
>>> after
>>> this change. Does this look good?
>>
>>
>> I like the implementation, but I think either we need to take a
>> reference on the host bridge, or change the name to  something like
>> "pci_find_host_bridge()", because using "_get_" is conventional for
>> functions that acquire a reference.
>>
>> Since host bridges are hot-pluggable, at least in theory, I vote for
>> taking a reference.  Then of course, you'd have to add code to drop
>> the reference when you're finished with it.
>>
> Bjorn,
>
> Thanks. I agree with your suggestion even though the convention is not
> followed fully :) of_pci_get_devfn(), of_get_pci_domain_nr(),
> of_pci_get_host_bridge_resources() are some of those functions not following
> the convention. I plan to change the function as below. Also want to name
> functions as pci_get/put_host_bridge_device() as existing function
> find_pci_host_bridge() is actually returning ptr to struct pci_host_bridge
> vs the new function returning ptr to device. Here are the new functions and
> how they will be used. Please review and respond so that I can avoid a
> re-spin.
>
> in linux/include/pci.h add the prototypes of
> pci_get/put_host_bridge_device().
>
> in drivers/pci/host-bridge.c add two new functions.
>
> struct device *pci_get_host_bridge_device(struct pci_dev *dev)
> {
>         struct pci_bus *root_bus = find_pci_root_bus(dev->bus);
>         struct device *bridge = root_bus->bridge;
>
>         kobject_get(&bridge->kobj);
>         return bridge;
> }

Looks good to me.

> void  pci_put_host_bridge_device(struct pci_dev *dev)
> {
>         struct pci_bus *root_bus = find_pci_root_bus(dev->bus);
>         struct device *bridge = root_bus->bridge;
>
>         kobject_put(&bridge->kobj);
> }

I think I would pass in the "struct device *" here so we don't have to
call find_pci_root_bus() again.

> drivers/of/of_pci.c
>
> void of_pci_dma_configure(struct pci_dev *pci_dev)
> {
>         struct device *dev = &pci_dev->dev;
>         struct device *bridge = pci_get_host_bridge_device(pci_dev);
>
>         of_dma_configure(dev, bridge->parent->of_node);
>         pci_put_host_bridge_device(pci_dev);

Then this would become "pci_put_host_bridge_device(bridge)"

> }
>
> Murali
>
>> Bjorn
>>
>
>>>>>
>>>
>>>
>>> --
>>> Murali Karicheri
>>> Linux Kernel, Texas Instruments
>
>
>
> --
> Murali Karicheri
> Linux Kernel, Texas Instruments

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

[Murali Karicheri]

On 01/27/2015 01:42 PM, Bjorn Helgaas wrote:
> On Tue, Jan 27, 2015 at 12:14 PM, Murali Karicheri<m-karicheri2-l0cyMroinI0@public.gmane.org>  wrote:
>> On 01/26/2015 06:59 PM, Bjorn Helgaas wrote:
>>>
>>> On Mon, Jan 26, 2015 at 5:25 PM, Murali Karicheri<m-karicheri2-l0cyMroinI0@public.gmane.org>
>>> wrote:
>>>>
>>>> On 01/23/2015 06:41 PM, Bjorn Helgaas wrote:
>>>>>
>>>>>
>>>>> On Fri, Jan 23, 2015 at 05:32:37PM -0500, 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.
>>>>>>
>>    -- Cut ---
>>
>>>>>>
>>>>>> Signed-off-by: Murali Karicheri<m-karicheri2-l0cyMroinI0@public.gmane.org>
>>>>>> ---
>>>>>>     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)
>>>>>
>>>>>
>>>>>
>>>>> I'm not an OF person, but this interface seems like it might be too
>>>>> special-purpose.  Maybe it would be enough to add
>>>>> "of_get_pci_root_bridge()", and the caller could do this:
>>>>>
>>>>>        struct device *bridge = of_get_pci_root_bridge(dev);
>>>>>        struct device_node *parent_np = bridge->parent->of_node;
>>>>>
>>>>> Also, the name "of_get_..." suggests that it increments a refcount, as
>>>>> of_get_parent() does.  But you aren't doing anything with the refcount.
>>>>>
>>>>> But I guess an "of_get_pci_root_bridge()" isn't doing anything
>>>>> OF-related,
>>>>> so maybe we should just add a "pci_get_host_bridge(struct pci_dev *)"
>>>>> to PCI instead.
>>>>
>>>>
>>>>
>>>> Bjorn,
>>>>
>>>> Thanks for the comment.
>>>>
>>>> I think adding pci_get_host_bridge() is a good idea. There is already
>>>> similar function in host-bridge.c. I have added this function re-using
>>>> existing function find_pci_root_bus(). See the incremental diff below
>>>> after
>>>> this change. Does this look good?
>>>
>>>
>>> I like the implementation, but I think either we need to take a
>>> reference on the host bridge, or change the name to  something like
>>> "pci_find_host_bridge()", because using "_get_" is conventional for
>>> functions that acquire a reference.
>>>
>>> Since host bridges are hot-pluggable, at least in theory, I vote for
>>> taking a reference.  Then of course, you'd have to add code to drop
>>> the reference when you're finished with it.
>>>
>> Bjorn,
>>
>> Thanks. I agree with your suggestion even though the convention is not
>> followed fully :) of_pci_get_devfn(), of_get_pci_domain_nr(),
>> of_pci_get_host_bridge_resources() are some of those functions not following
>> the convention. I plan to change the function as below. Also want to name
>> functions as pci_get/put_host_bridge_device() as existing function
>> find_pci_host_bridge() is actually returning ptr to struct pci_host_bridge
>> vs the new function returning ptr to device. Here are the new functions and
>> how they will be used. Please review and respond so that I can avoid a
>> re-spin.
>>
>> in linux/include/pci.h add the prototypes of
>> pci_get/put_host_bridge_device().
>>
>> in drivers/pci/host-bridge.c add two new functions.
>>
>> struct device *pci_get_host_bridge_device(struct pci_dev *dev)
>> {
>>          struct pci_bus *root_bus = find_pci_root_bus(dev->bus);
>>          struct device *bridge = root_bus->bridge;
>>
>>          kobject_get(&bridge->kobj);
>>          return bridge;
>> }
>
> Looks good to me.
>
>> void  pci_put_host_bridge_device(struct pci_dev *dev)
>> {
>>          struct pci_bus *root_bus = find_pci_root_bus(dev->bus);
>>          struct device *bridge = root_bus->bridge;
>>
>>          kobject_put(&bridge->kobj);
>> }
>
> I think I would pass in the "struct device *" here so we don't have to
> call find_pci_root_bus() again.
>
>> drivers/of/of_pci.c
>>
>> void of_pci_dma_configure(struct pci_dev *pci_dev)
>> {
>>          struct device *dev =&pci_dev->dev;
>>          struct device *bridge = pci_get_host_bridge_device(pci_dev);
>>
>>          of_dma_configure(dev, bridge->parent->of_node);
>>          pci_put_host_bridge_device(pci_dev);
>
> Then this would become "pci_put_host_bridge_device(bridge)"

Agree with both. Will become part of my v5 of the series. I am adding 
this as a separate commit.

Murali
>
>> }
>>
>> Murali
>>
>>> Bjorn
>>>
>>
>>>>>>
>>>>
>>>>
>>>> --
>>>> Murali Karicheri
>>>> Linux Kernel, Texas Instruments
>>
>>
>>
>> --
>> Murali Karicheri
>> Linux Kernel, Texas Instruments


-- 
Murali Karicheri
Linux Kernel, Texas Instruments

[PATCH v4 5/6] 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.

Cc: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>

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

Re: [PATCH v4 5/6] PCI: update dma configuration from DT

[Bjorn Helgaas]

On Fri, Jan 23, 2015 at 05:32:38PM -0500, Murali Karicheri wrote:
> 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.
> 
> Cc: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
> Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>
> 
> Signed-off-by: Murali Karicheri <m-karicheri2-l0cyMroinI0@public.gmane.org>

I assume this series will be merged via some non-PCI tree, so:

Acked-by: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@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
> 

Re: [PATCH v4 5/6] PCI: update dma configuration from DT

[Murali Karicheri]

On 01/23/2015 06:27 PM, Bjorn Helgaas wrote:
> On Fri, Jan 23, 2015 at 05:32:38PM -0500, Murali Karicheri wrote:
>> 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.
>>
>> Cc: Joerg Roedel<joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
>> Cc: Grant Likely<grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Cc: Rob Herring<robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Cc: Bjorn Helgaas<bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>> Cc: Will Deacon<will.deacon-5wv7dgnIgG8@public.gmane.org>
>> Cc: Russell King<linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
>> Cc: Arnd Bergmann<arnd-r2nGTMty4D4@public.gmane.org>
>> Cc: Suravee Suthikulpanit<Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>
>>
>> Signed-off-by: Murali Karicheri<m-karicheri2-l0cyMroinI0@public.gmane.org>
>
> I assume this series will be merged via some non-PCI tree, so:

Not sure who will pick this. Since this is for PCI, will it be possible 
to apply this to arm-pci/next?

>
> Acked-by: Bjorn Helgaas<bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

I will add your ack for v5.

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


-- 
Murali Karicheri
Linux Kernel, Texas Instruments

[PATCH v4 3/6] of: fix size when dma-range is not used

[Murali Karicheri]

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. To detect
overflow when mask is set to max of u64, add a check, log error and return.
Some platform use mask format for size in DTS. So add a work around to
catch this and fix.

Cc: Joerg Roedel <joro@8bytes.org>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>

Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
---
 drivers/of/device.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 2de320d..0a5ff54 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -105,12 +105,24 @@ void of_dma_configure(struct device *dev, struct device_node *np)
 	ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
 	if (ret < 0) {
 		dma_addr = offset = 0;
-		size = dev->coherent_dma_mask;
+		size = dev->coherent_dma_mask + 1;
 	} else {
 		offset = PFN_DOWN(paddr - dma_addr);
+		/*
+		 * Add a work around to treat the size as mask + 1 in case
+		 * it is defined in DT as a mask.
+		 */
+		if (size & 1)
+			size = size + 1;
 		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
 	}
 
+	/* if size is 0, we have an overflow of u64 */
+	if (!size) {
+		dev_err(dev, "invalid size\n");
+		return;
+	}
+
 	dev->dma_pfn_offset = offset;
 
 	coherent = of_dma_is_coherent(np);
-- 
1.7.9.5

Re: [PATCH v4 3/6] of: fix size when dma-range is not used

[Robin Murphy]

Hi Murali,

On 23/01/15 22:32, Murali Karicheri wrote:
> 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. To detect
> overflow when mask is set to max of u64, add a check, log error and return.
> Some platform use mask format for size in DTS. So add a work around to
> catch this and fix.
>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>
> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> ---
>   drivers/of/device.c |   14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 2de320d..0a5ff54 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -105,12 +105,24 @@ void of_dma_configure(struct device *dev, struct device_node *np)
>   	ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
>   	if (ret < 0) {
>   		dma_addr = offset = 0;
> -		size = dev->coherent_dma_mask;
> +		size = dev->coherent_dma_mask + 1;
>   	} else {
>   		offset = PFN_DOWN(paddr - dma_addr);
> +		/*
> +		 * Add a work around to treat the size as mask + 1 in case
> +		 * it is defined in DT as a mask.
> +		 */
> +		if (size & 1)
> +			size = size + 1;
>   		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
>   	}
>
> +	/* if size is 0, we have an overflow of u64 */
> +	if (!size) {
> +		dev_err(dev, "invalid size\n");
> +		return;
> +	}
> +

This seems potentially fragile to dodgy DTs given that we might also be 
using size to make a mask later. Would it make sense to double-up a 
sanity check as mask-format detection? Something like:

if is_power_of_2(size)
	// use size
else if is_power_of_2(size + 1)
	// use size + 1
else
	// cry


Robin.

>   	dev->dma_pfn_offset = offset;
>
>   	coherent = of_dma_is_coherent(np);
>

Re: [PATCH v4 3/6] of: fix size when dma-range is not used

[Murali Karicheri]

On 01/27/2015 06:27 AM, Robin Murphy wrote:
> Hi Murali,
>
> On 23/01/15 22:32, Murali Karicheri wrote:
>> 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. To detect
>> overflow when mask is set to max of u64, add a check, log error and
>> return.
>> Some platform use mask format for size in DTS. So add a work around to
>> catch this and fix.
>>
>> Cc: Joerg Roedel <joro@8bytes.org>
>> Cc: Grant Likely <grant.likely@linaro.org>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Russell King <linux@arm.linux.org.uk>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>>
>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
>> ---
>> drivers/of/device.c | 14 +++++++++++++-
>> 1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> index 2de320d..0a5ff54 100644
>> --- a/drivers/of/device.c
>> +++ b/drivers/of/device.c
>> @@ -105,12 +105,24 @@ void of_dma_configure(struct device *dev, struct
>> device_node *np)
>> ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
>> if (ret < 0) {
>> dma_addr = offset = 0;
>> - size = dev->coherent_dma_mask;
>> + size = dev->coherent_dma_mask + 1;
>> } else {
>> offset = PFN_DOWN(paddr - dma_addr);
>> + /*
>> + * Add a work around to treat the size as mask + 1 in case
>> + * it is defined in DT as a mask.
>> + */
>> + if (size & 1)
>> + size = size + 1;
>> dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
>> }
>>
>> + /* if size is 0, we have an overflow of u64 */
>> + if (!size) {
>> + dev_err(dev, "invalid size\n");
>> + return;
>> + }
>> +
>
> This seems potentially fragile to dodgy DTs given that we might also be
> using size to make a mask later. Would it make sense to double-up a
> sanity check as mask-format detection? Something like:
>
> if is_power_of_2(size)
> // use size
> else if is_power_of_2(size + 1)
> // use size + 1
> else
> // cry
>
Robin,

I think this is better. I will wait for some more time for anyone to 
respond and re-send my patch with this change.

Thanks
Murali
>
> Robin.
>
>> dev->dma_pfn_offset = offset;
>>
>> coherent = of_dma_is_coherent(np);
>>
>
>


-- 
Murali Karicheri
Linux Kernel, Texas Instruments

Re: [PATCH v4 3/6] of: fix size when dma-range is not used

[Murali Karicheri]

On 01/27/2015 06:27 AM, Robin Murphy wrote:
> Hi Murali,
>
> On 23/01/15 22:32, Murali Karicheri wrote:
>> 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. To detect
>> overflow when mask is set to max of u64, add a check, log error and
>> return.
>> Some platform use mask format for size in DTS. So add a work around to
>> catch this and fix.
>>
>> Cc: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
>> Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Cc: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>> Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
>> Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
>> Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
>> Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>
>>
>> Signed-off-by: Murali Karicheri <m-karicheri2-l0cyMroinI0@public.gmane.org>
>> ---
>> drivers/of/device.c | 14 +++++++++++++-
>> 1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> index 2de320d..0a5ff54 100644
>> --- a/drivers/of/device.c
>> +++ b/drivers/of/device.c
>> @@ -105,12 +105,24 @@ void of_dma_configure(struct device *dev, struct
>> device_node *np)
>> ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
>> if (ret < 0) {
>> dma_addr = offset = 0;
>> - size = dev->coherent_dma_mask;
>> + size = dev->coherent_dma_mask + 1;
>> } else {
>> offset = PFN_DOWN(paddr - dma_addr);
>> + /*
>> + * Add a work around to treat the size as mask + 1 in case
>> + * it is defined in DT as a mask.
>> + */
>> + if (size & 1)
>> + size = size + 1;
>> dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
>> }
>>
>> + /* if size is 0, we have an overflow of u64 */
>> + if (!size) {
>> + dev_err(dev, "invalid size\n");
>> + return;
>> + }
>> +
>
> This seems potentially fragile to dodgy DTs given that we might also be
> using size to make a mask later. Would it make sense to double-up a
> sanity check as mask-format detection? Something like:
>
> if is_power_of_2(size)
> // use size
> else if is_power_of_2(size + 1)
> // use size + 1
> else
> // cry
Robin,

How about having the logic like this?

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

	if (is_power_of_2(size + 1))
		size = size + 1;
	else if (!is_power_of_2(size))
	{
		dev_err(dev, "invalid size\n");
		return;
	}

Murali
>
>
> Robin.
>
>> dev->dma_pfn_offset = offset;
>>
>> coherent = of_dma_is_coherent(np);
>>
>
>


-- 
Murali Karicheri
Linux Kernel, Texas Instruments

Re: [PATCH v4 3/6] of: fix size when dma-range is not used

[Catalin Marinas]

On Tue, Jan 27, 2015 at 06:55:15PM +0000, Murali Karicheri wrote:
> On 01/27/2015 06:27 AM, Robin Murphy wrote:
> > On 23/01/15 22:32, Murali Karicheri wrote:
> >> 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. To detect
> >> overflow when mask is set to max of u64, add a check, log error and
> >> return.
> >> Some platform use mask format for size in DTS. So add a work around to
> >> catch this and fix.
> >>
> >> Cc: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
> >> Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >> Cc: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> >> Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> >> Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> >> Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> >> Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>
> >>
> >> Signed-off-by: Murali Karicheri <m-karicheri2-l0cyMroinI0@public.gmane.org>
> >> ---
> >> drivers/of/device.c | 14 +++++++++++++-
> >> 1 file changed, 13 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/of/device.c b/drivers/of/device.c
> >> index 2de320d..0a5ff54 100644
> >> --- a/drivers/of/device.c
> >> +++ b/drivers/of/device.c
> >> @@ -105,12 +105,24 @@ void of_dma_configure(struct device *dev, struct
> >> device_node *np)
> >> ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
> >> if (ret < 0) {
> >> dma_addr = offset = 0;
> >> - size = dev->coherent_dma_mask;
> >> + size = dev->coherent_dma_mask + 1;
> >> } else {
> >> offset = PFN_DOWN(paddr - dma_addr);
> >> + /*
> >> + * Add a work around to treat the size as mask + 1 in case
> >> + * it is defined in DT as a mask.
> >> + */
> >> + if (size & 1)
> >> + size = size + 1;
> >> dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
> >> }
> >>
> >> + /* if size is 0, we have an overflow of u64 */
> >> + if (!size) {
> >> + dev_err(dev, "invalid size\n");
> >> + return;
> >> + }
> >> +
> >
> > This seems potentially fragile to dodgy DTs given that we might also be
> > using size to make a mask later. Would it make sense to double-up a
> > sanity check as mask-format detection? Something like:
> >
> > if is_power_of_2(size)
> > // use size
> > else if is_power_of_2(size + 1)
> > // use size + 1
> > else
> > // cry
> 
> How about having the logic like this?
> 
> 	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);
> 	}
> 
> 	if (is_power_of_2(size + 1))
> 		size = size + 1;
> 	else if (!is_power_of_2(size))
> 	{
> 		dev_err(dev, "invalid size\n");
> 		return;
> 	}

In of_dma_configure(), we currently assume that the default coherent
mask is 32-bit. In this thread:

http://article.gmane.org/gmane.linux.kernel/1835096

we talked about setting the coherent mask based on size automatically.
I'm not sure about the size but I think we can assume is 32-bit mask + 1
if it is not specified in the DT. Instead of just assuming a default
mask, let's assume a default size and create the mask based on this
(untested):

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 5b33c6a21807..9ff8d1286b44 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -170,10 +170,10 @@ static void of_dma_configure(struct device *dev)
 	struct iommu_ops *iommu;
 
 	/*
-	 * Set default dma-mask to 32 bit. Drivers are expected to setup
-	 * the correct supported dma_mask.
+	 * Set default size to cover the 32-bit. Drivers are expected to setup
+	 * the correct size and dma_mask.
 	 */
-	dev->coherent_dma_mask = DMA_BIT_MASK(32);
+	size = 1ULL << 32;
 
 	/*
 	 * Set it to coherent_dma_mask by default if the architecture
@@ -185,13 +185,24 @@ static void of_dma_configure(struct device *dev)
 	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", dev->dma_pfn_offset);
 	}
 	dev->dma_pfn_offset = offset;
 
+	/*
+	 * Workaround for DTs setting the size to a mask or 0.
+	 */
+	if (is_power_of_2(size + 1))
+		size += 1;
+
+	/*
+	 * Coherent DMA masks larger than 32-bit must be explicitly set by the
+	 * driver.
+	 */
+	dev->coherent_dma_mask = min(DMA_BIT_MASK(32), DMA_BIT_MASK(ilog2(size)));
+
 	coherent = of_dma_is_coherent(dev->of_node);
 	dev_dbg(dev, "device is%sdma coherent\n",
 		coherent ? " " : " not ");

-- 
Catalin

Re: [PATCH v4 3/6] of: fix size when dma-range is not used

[Rob Herring]

On Wed, Jan 28, 2015 at 5:05 AM, Catalin Marinas
<catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
> On Tue, Jan 27, 2015 at 06:55:15PM +0000, Murali Karicheri wrote:
>> On 01/27/2015 06:27 AM, Robin Murphy wrote:
>> > On 23/01/15 22:32, Murali Karicheri wrote:
>> >> 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. To detect
>> >> overflow when mask is set to max of u64, add a check, log error and
>> >> return.
>> >> Some platform use mask format for size in DTS. So add a work around to
>> >> catch this and fix.
>> >>
>> >> Cc: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
>> >> Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> >> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> >> Cc: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>> >> Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
>> >> Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
>> >> Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
>> >> Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>
>> >>
>> >> Signed-off-by: Murali Karicheri <m-karicheri2-l0cyMroinI0@public.gmane.org>
>> >> ---
>> >> drivers/of/device.c | 14 +++++++++++++-
>> >> 1 file changed, 13 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> >> index 2de320d..0a5ff54 100644
>> >> --- a/drivers/of/device.c
>> >> +++ b/drivers/of/device.c
>> >> @@ -105,12 +105,24 @@ void of_dma_configure(struct device *dev, struct
>> >> device_node *np)
>> >> ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
>> >> if (ret < 0) {
>> >> dma_addr = offset = 0;
>> >> - size = dev->coherent_dma_mask;
>> >> + size = dev->coherent_dma_mask + 1;
>> >> } else {
>> >> offset = PFN_DOWN(paddr - dma_addr);
>> >> + /*
>> >> + * Add a work around to treat the size as mask + 1 in case
>> >> + * it is defined in DT as a mask.
>> >> + */
>> >> + if (size & 1)
>> >> + size = size + 1;
>> >> dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
>> >> }
>> >>
>> >> + /* if size is 0, we have an overflow of u64 */
>> >> + if (!size) {
>> >> + dev_err(dev, "invalid size\n");
>> >> + return;
>> >> + }
>> >> +
>> >
>> > This seems potentially fragile to dodgy DTs given that we might also be
>> > using size to make a mask later. Would it make sense to double-up a
>> > sanity check as mask-format detection? Something like:
>> >
>> > if is_power_of_2(size)
>> > // use size
>> > else if is_power_of_2(size + 1)
>> > // use size + 1
>> > else
>> > // cry
>>
>> How about having the logic like this?
>>
>>       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);
>>       }
>>
>>       if (is_power_of_2(size + 1))
>>               size = size + 1;
>>       else if (!is_power_of_2(size))
>>       {
>>               dev_err(dev, "invalid size\n");
>>               return;
>>       }
>
> In of_dma_configure(), we currently assume that the default coherent
> mask is 32-bit. In this thread:
>
> http://article.gmane.org/gmane.linux.kernel/1835096
>
> we talked about setting the coherent mask based on size automatically.
> I'm not sure about the size but I think we can assume is 32-bit mask + 1
> if it is not specified in the DT. Instead of just assuming a default
> mask, let's assume a default size and create the mask based on this
> (untested):
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 5b33c6a21807..9ff8d1286b44 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -170,10 +170,10 @@ static void of_dma_configure(struct device *dev)
>         struct iommu_ops *iommu;
>
>         /*
> -        * Set default dma-mask to 32 bit. Drivers are expected to setup
> -        * the correct supported dma_mask.
> +        * Set default size to cover the 32-bit. Drivers are expected to setup
> +        * the correct size and dma_mask.
>          */
> -       dev->coherent_dma_mask = DMA_BIT_MASK(32);
> +       size = 1ULL << 32;
>
>         /*
>          * Set it to coherent_dma_mask by default if the architecture
> @@ -185,13 +185,24 @@ static void of_dma_configure(struct device *dev)
>         ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size);
>         if (ret < 0) {
>                 dma_addr = offset = 0;
> -               size = dev->coherent_dma_mask;

Are we assuming dma_addr, paddr and size are not touched on error? If
so, we can get rid of this clause entirely.

>         } else {
>                 offset = PFN_DOWN(paddr - dma_addr);
>                 dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset);
>         }
>         dev->dma_pfn_offset = offset;
>
> +       /*
> +        * Workaround for DTs setting the size to a mask or 0.
> +        */
> +       if (is_power_of_2(size + 1))
> +               size += 1;

As I mentioned, I think power of 2 is too restrictive (from a DT
perspective even though the kernel may require a power of 2 here for
the mask). Just checking bit0 set should be enough.

Also, we need a WARN here so DTs get fixed.

> +
> +       /*
> +        * Coherent DMA masks larger than 32-bit must be explicitly set by the
> +        * driver.
> +        */
> +       dev->coherent_dma_mask = min(DMA_BIT_MASK(32), DMA_BIT_MASK(ilog2(size)));
> +
>         coherent = of_dma_is_coherent(dev->of_node);
>         dev_dbg(dev, "device is%sdma coherent\n",
>                 coherent ? " " : " not ");
>
> --
> Catalin

Re: [PATCH v4 3/6] of: fix size when dma-range is not used

[Catalin Marinas]

On Wed, Jan 28, 2015 at 03:45:19PM +0000, Rob Herring wrote:
> On Wed, Jan 28, 2015 at 5:05 AM, Catalin Marinas
> <catalin.marinas-5wv7dgnIgG8@public.gmane.org> wrote:
> > On Tue, Jan 27, 2015 at 06:55:15PM +0000, Murali Karicheri wrote:
> >> How about having the logic like this?
> >>
> >>       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);
> >>       }
> >>
> >>       if (is_power_of_2(size + 1))
> >>               size = size + 1;
> >>       else if (!is_power_of_2(size))
> >>       {
> >>               dev_err(dev, "invalid size\n");
> >>               return;
> >>       }
> >
> > In of_dma_configure(), we currently assume that the default coherent
> > mask is 32-bit. In this thread:
> >
> > http://article.gmane.org/gmane.linux.kernel/1835096
> >
> > we talked about setting the coherent mask based on size automatically.
> > I'm not sure about the size but I think we can assume is 32-bit mask + 1
> > if it is not specified in the DT. Instead of just assuming a default
> > mask, let's assume a default size and create the mask based on this
> > (untested):
> >
> > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > index 5b33c6a21807..9ff8d1286b44 100644
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -170,10 +170,10 @@ static void of_dma_configure(struct device *dev)
> >         struct iommu_ops *iommu;
> >
> >         /*
> > -        * Set default dma-mask to 32 bit. Drivers are expected to setup
> > -        * the correct supported dma_mask.
> > +        * Set default size to cover the 32-bit. Drivers are expected to setup
> > +        * the correct size and dma_mask.
> >          */
> > -       dev->coherent_dma_mask = DMA_BIT_MASK(32);
> > +       size = 1ULL << 32;
> >
> >         /*
> >          * Set it to coherent_dma_mask by default if the architecture
> > @@ -185,13 +185,24 @@ static void of_dma_configure(struct device *dev)
> >         ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size);
> >         if (ret < 0) {
> >                 dma_addr = offset = 0;
> > -               size = dev->coherent_dma_mask;
> 
> Are we assuming dma_addr, paddr and size are not touched on error? If
> so, we can get rid of this clause entirely.

We can if we initialise dma_addr and offset to 0 when declared in this
function. The dma_addr and size variables are later passed to the
arch_setup_dma_ops(), so they need to have some sane values independent
of the presence of dma-ranges in the DT.

> >         } else {
> >                 offset = PFN_DOWN(paddr - dma_addr);
> >                 dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset);
> >         }
> >         dev->dma_pfn_offset = offset;
> >
> > +       /*
> > +        * Workaround for DTs setting the size to a mask or 0.
> > +        */
> > +       if (is_power_of_2(size + 1))
> > +               size += 1;
> 
> As I mentioned, I think power of 2 is too restrictive (from a DT
> perspective even though the kernel may require a power of 2 here for
> the mask). Just checking bit0 set should be enough.

The power of 2 was mainly to cover the case where the size is wrongly
written as a mask in the DT. If the size is deliberately not a power of
two and not a full mask, the above check won't change it. With my
proposal, ilog2 gets rid of extra bits in size, only that if the size
was a mask because of DT error, we lose a bit in the coherent_dma_mask.

> Also, we need a WARN here so DTs get fixed.

I agree.

-- 
Catalin

Re: [PATCH v4 3/6] of: fix size when dma-range is not used

[Murali Karicheri]

On 01/28/2015 10:45 AM, Rob Herring wrote:
> On Wed, Jan 28, 2015 at 5:05 AM, Catalin Marinas
> <catalin.marinas-5wv7dgnIgG8@public.gmane.org>  wrote:
>> On Tue, Jan 27, 2015 at 06:55:15PM +0000, Murali Karicheri wrote:
>>> On 01/27/2015 06:27 AM, Robin Murphy wrote:
>>>> On 23/01/15 22:32, Murali Karicheri wrote:
>>>>> 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. To detect
>>>>> overflow when mask is set to max of u64, add a check, log error and
>>>>> return.
>>>>> Some platform use mask format for size in DTS. So add a work around to
>>>>> catch this and fix.
>>>>>
>>>>> Cc: Joerg Roedel<joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
>>>>> Cc: Grant Likely<grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>>> Cc: Rob Herring<robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>>>> Cc: Bjorn Helgaas<bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>>>>> Cc: Will Deacon<will.deacon-5wv7dgnIgG8@public.gmane.org>
>>>>> Cc: Russell King<linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
>>>>> Cc: Arnd Bergmann<arnd-r2nGTMty4D4@public.gmane.org>
>>>>> Cc: Suravee Suthikulpanit<Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>
>>>>>
>>>>> Signed-off-by: Murali Karicheri<m-karicheri2-l0cyMroinI0@public.gmane.org>
>>>>> ---
>>>>> drivers/of/device.c | 14 +++++++++++++-
>>>>> 1 file changed, 13 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>>>>> index 2de320d..0a5ff54 100644
>>>>> --- a/drivers/of/device.c
>>>>> +++ b/drivers/of/device.c
>>>>> @@ -105,12 +105,24 @@ void of_dma_configure(struct device *dev, struct
>>>>> device_node *np)
>>>>> ret = of_dma_get_range(np,&dma_addr,&paddr,&size);
>>>>> if (ret<  0) {
>>>>> dma_addr = offset = 0;
>>>>> - size = dev->coherent_dma_mask;
>>>>> + size = dev->coherent_dma_mask + 1;
>>>>> } else {
>>>>> offset = PFN_DOWN(paddr - dma_addr);
>>>>> + /*
>>>>> + * Add a work around to treat the size as mask + 1 in case
>>>>> + * it is defined in DT as a mask.
>>>>> + */
>>>>> + if (size&  1)
>>>>> + size = size + 1;
>>>>> dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
>>>>> }
>>>>>
>>>>> + /* if size is 0, we have an overflow of u64 */
>>>>> + if (!size) {
>>>>> + dev_err(dev, "invalid size\n");
>>>>> + return;
>>>>> + }
>>>>> +
>>>>
>>>> This seems potentially fragile to dodgy DTs given that we might also be
>>>> using size to make a mask later. Would it make sense to double-up a
>>>> sanity check as mask-format detection? Something like:
>>>>
>>>> if is_power_of_2(size)
>>>> // use size
>>>> else if is_power_of_2(size + 1)
>>>> // use size + 1
>>>> else
>>>> // cry
>>>
>>> How about having the logic like this?
>>>
>>>        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);
>>>        }
>>>
>>>        if (is_power_of_2(size + 1))
>>>                size = size + 1;
>>>        else if (!is_power_of_2(size))
>>>        {
>>>                dev_err(dev, "invalid size\n");
>>>                return;
>>>        }
>>
>> In of_dma_configure(), we currently assume that the default coherent
>> mask is 32-bit. In this thread:
>>
>> http://article.gmane.org/gmane.linux.kernel/1835096
>>
>> we talked about setting the coherent mask based on size automatically.
>> I'm not sure about the size but I think we can assume is 32-bit mask + 1
>> if it is not specified in the DT. Instead of just assuming a default
>> mask, let's assume a default size and create the mask based on this
>> (untested):
>>
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index 5b33c6a21807..9ff8d1286b44 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -170,10 +170,10 @@ static void of_dma_configure(struct device *dev)
>>          struct iommu_ops *iommu;
>>
>>          /*
>> -        * Set default dma-mask to 32 bit. Drivers are expected to setup
>> -        * the correct supported dma_mask.
>> +        * Set default size to cover the 32-bit. Drivers are expected to setup
>> +        * the correct size and dma_mask.
>>           */
>> -       dev->coherent_dma_mask = DMA_BIT_MASK(32);
>> +       size = 1ULL<<  32;
>>
>>          /*
>>           * Set it to coherent_dma_mask by default if the architecture
>> @@ -185,13 +185,24 @@ static void of_dma_configure(struct device *dev)
>>          ret = of_dma_get_range(dev->of_node,&dma_addr,&paddr,&size);
>>          if (ret<  0) {
>>                  dma_addr = offset = 0;
>> -               size = dev->coherent_dma_mask;
>
> Are we assuming dma_addr, paddr and size are not touched on error? If
> so, we can get rid of this clause entirely.
Checking the code for of_dma_get_range() I see paddr is modified on 
error case, but is used only for success case in this function. dma_addr 
and size are not modified. So setting dma_addr and offset to zero before 
hand like size might work as below

dma_addr = offset = 0;
size = 1ULL <<  32;

ret = of_dma_get_range(dev->of_node,&dma_addr,&paddr,&size);
if (!ret) {
	offset = PFN_DOWN(paddr - dma_addr);
}

.. rest of the code.

Murali


>
>>          } else {
>>                  offset = PFN_DOWN(paddr - dma_addr);
>>                  dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset);
>>          }
>>          dev->dma_pfn_offset = offset;
>>
>> +       /*
>> +        * Workaround for DTs setting the size to a mask or 0.
>> +        */
>> +       if (is_power_of_2(size + 1))
>> +               size += 1;
>
> As I mentioned, I think power of 2 is too restrictive (from a DT
> perspective even though the kernel may require a power of 2 here for
> the mask). Just checking bit0 set should be enough.
>
> Also, we need a WARN here so DTs get fixed.
>
>> +
>> +       /*
>> +        * Coherent DMA masks larger than 32-bit must be explicitly set by the
>> +        * driver.
>> +        */
>> +       dev->coherent_dma_mask = min(DMA_BIT_MASK(32), DMA_BIT_MASK(ilog2(size)));
>> +
>>          coherent = of_dma_is_coherent(dev->of_node);
>>          dev_dbg(dev, "device is%sdma coherent\n",
>>                  coherent ? " " : " not ");
>>
>> --
>> Catalin


-- 
Murali Karicheri
Linux Kernel, Texas Instruments

Re: [PATCH v4 3/6] of: fix size when dma-range is not used

[Robin Murphy]

On 28/01/15 11:05, Catalin Marinas wrote:
> On Tue, Jan 27, 2015 at 06:55:15PM +0000, Murali Karicheri wrote:
>> On 01/27/2015 06:27 AM, Robin Murphy wrote:
>>> On 23/01/15 22:32, Murali Karicheri wrote:
>>>> 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. To detect
>>>> overflow when mask is set to max of u64, add a check, log error and
>>>> return.
>>>> Some platform use mask format for size in DTS. So add a work around to
>>>> catch this and fix.
>>>>
>>>> Cc: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
>>>> Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>>> Cc: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>>>> Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
>>>> Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
>>>> Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
>>>> Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>
>>>>
>>>> Signed-off-by: Murali Karicheri <m-karicheri2-l0cyMroinI0@public.gmane.org>
>>>> ---
>>>> drivers/of/device.c | 14 +++++++++++++-
>>>> 1 file changed, 13 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>>>> index 2de320d..0a5ff54 100644
>>>> --- a/drivers/of/device.c
>>>> +++ b/drivers/of/device.c
>>>> @@ -105,12 +105,24 @@ void of_dma_configure(struct device *dev, struct
>>>> device_node *np)
>>>> ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
>>>> if (ret < 0) {
>>>> dma_addr = offset = 0;
>>>> - size = dev->coherent_dma_mask;
>>>> + size = dev->coherent_dma_mask + 1;
>>>> } else {
>>>> offset = PFN_DOWN(paddr - dma_addr);
>>>> + /*
>>>> + * Add a work around to treat the size as mask + 1 in case
>>>> + * it is defined in DT as a mask.
>>>> + */
>>>> + if (size & 1)
>>>> + size = size + 1;
>>>> dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
>>>> }
>>>>
>>>> + /* if size is 0, we have an overflow of u64 */
>>>> + if (!size) {
>>>> + dev_err(dev, "invalid size\n");
>>>> + return;
>>>> + }
>>>> +
>>>
>>> This seems potentially fragile to dodgy DTs given that we might also be
>>> using size to make a mask later. Would it make sense to double-up a
>>> sanity check as mask-format detection? Something like:
>>>
>>> if is_power_of_2(size)
>>> // use size
>>> else if is_power_of_2(size + 1)
>>> // use size + 1
>>> else
>>> // cry
>>
>> How about having the logic like this?
>>
>> 	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);
>> 	}
>>
>> 	if (is_power_of_2(size + 1))
>> 		size = size + 1;
>> 	else if (!is_power_of_2(size))
>> 	{
>> 		dev_err(dev, "invalid size\n");
>> 		return;
>> 	}
>
> In of_dma_configure(), we currently assume that the default coherent
> mask is 32-bit. In this thread:
>
> http://article.gmane.org/gmane.linux.kernel/1835096
>
> we talked about setting the coherent mask based on size automatically.
> I'm not sure about the size but I think we can assume is 32-bit mask + 1
> if it is not specified in the DT. Instead of just assuming a default
> mask, let's assume a default size and create the mask based on this
> (untested):
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 5b33c6a21807..9ff8d1286b44 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -170,10 +170,10 @@ static void of_dma_configure(struct device *dev)
>   	struct iommu_ops *iommu;
>
>   	/*
> -	 * Set default dma-mask to 32 bit. Drivers are expected to setup
> -	 * the correct supported dma_mask.
> +	 * Set default size to cover the 32-bit. Drivers are expected to setup
> +	 * the correct size and dma_mask.
>   	 */
> -	dev->coherent_dma_mask = DMA_BIT_MASK(32);
> +	size = 1ULL << 32;
>
>   	/*
>   	 * Set it to coherent_dma_mask by default if the architecture
> @@ -185,13 +185,24 @@ static void of_dma_configure(struct device *dev)
>   	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", dev->dma_pfn_offset);
>   	}
>   	dev->dma_pfn_offset = offset;
>
> +	/*
> +	 * Workaround for DTs setting the size to a mask or 0.
> +	 */
> +	if (is_power_of_2(size + 1))
> +		size += 1;

In fact, since the ilog2 below ends up effectively rounding down, we 
might as well do away with this check as well and just add 1 
unconditionally. The only time it makes any difference is when we want 
it to anyway!

I like this approach, it ends up looking a lot neater.

Robin.

> +
> +	/*
> +	 * Coherent DMA masks larger than 32-bit must be explicitly set by the
> +	 * driver.
> +	 */
> +	dev->coherent_dma_mask = min(DMA_BIT_MASK(32), DMA_BIT_MASK(ilog2(size)));
> +
>   	coherent = of_dma_is_coherent(dev->of_node);
>   	dev_dbg(dev, "device is%sdma coherent\n",
>   		coherent ? " " : " not ");
>

Re: [PATCH v4 3/6] of: fix size when dma-range is not used

[Catalin Marinas]

On Wed, Jan 28, 2015 at 03:55:57PM +0000, Robin Murphy wrote:
> On 28/01/15 11:05, Catalin Marinas wrote:
> > On Tue, Jan 27, 2015 at 06:55:15PM +0000, Murali Karicheri wrote:
> >> How about having the logic like this?
> >>
> >> 	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);
> >> 	}
> >>
> >> 	if (is_power_of_2(size + 1))
> >> 		size = size + 1;
> >> 	else if (!is_power_of_2(size))
> >> 	{
> >> 		dev_err(dev, "invalid size\n");
> >> 		return;
> >> 	}
> >
> > In of_dma_configure(), we currently assume that the default coherent
> > mask is 32-bit. In this thread:
> >
> > http://article.gmane.org/gmane.linux.kernel/1835096
> >
> > we talked about setting the coherent mask based on size automatically.
> > I'm not sure about the size but I think we can assume is 32-bit mask + 1
> > if it is not specified in the DT. Instead of just assuming a default
> > mask, let's assume a default size and create the mask based on this
> > (untested):
> >
> > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > index 5b33c6a21807..9ff8d1286b44 100644
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -170,10 +170,10 @@ static void of_dma_configure(struct device *dev)
> >   	struct iommu_ops *iommu;
> >
> >   	/*
> > -	 * Set default dma-mask to 32 bit. Drivers are expected to setup
> > -	 * the correct supported dma_mask.
> > +	 * Set default size to cover the 32-bit. Drivers are expected to setup
> > +	 * the correct size and dma_mask.
> >   	 */
> > -	dev->coherent_dma_mask = DMA_BIT_MASK(32);
> > +	size = 1ULL << 32;
> >
> >   	/*
> >   	 * Set it to coherent_dma_mask by default if the architecture
> > @@ -185,13 +185,24 @@ static void of_dma_configure(struct device *dev)
> >   	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", dev->dma_pfn_offset);
> >   	}
> >   	dev->dma_pfn_offset = offset;
> >
> > +	/*
> > +	 * Workaround for DTs setting the size to a mask or 0.
> > +	 */
> > +	if (is_power_of_2(size + 1))
> > +		size += 1;
> 
> In fact, since the ilog2 below ends up effectively rounding down, we 
> might as well do away with this check as well and just add 1 
> unconditionally. The only time it makes any difference is when we want 
> it to anyway!

Well, we could simply ignore the is_power_of_2() check but without
incrementing size as we don't know what arch_setup_dma_ops() does with
it.

I think we can remove this check altogether (we leaved without it for a
while) but we need to add 1 when calculating the mask:

	dev->coherent_dma_mask = min(DMA_BIT_MASK(32),
				     DMA_BIT_MASK(ilog2(size + 1)));

-- 
Catalin

Re: [PATCH v4 3/6] of: fix size when dma-range is not used

[Murali Karicheri]

On 01/28/2015 12:30 PM, Catalin Marinas wrote:
> On Wed, Jan 28, 2015 at 03:55:57PM +0000, Robin Murphy wrote:
>> On 28/01/15 11:05, Catalin Marinas wrote:
>>> On Tue, Jan 27, 2015 at 06:55:15PM +0000, Murali Karicheri wrote:
>>>> How about having the logic like this?
>>>>
>>>> 	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);
>>>> 	}
>>>>
>>>> 	if (is_power_of_2(size + 1))
>>>> 		size = size + 1;
>>>> 	else if (!is_power_of_2(size))
>>>> 	{
>>>> 		dev_err(dev, "invalid size\n");
>>>> 		return;
>>>> 	}
>>>
>>> In of_dma_configure(), we currently assume that the default coherent
>>> mask is 32-bit. In this thread:
>>>
>>> http://article.gmane.org/gmane.linux.kernel/1835096
>>>
>>> we talked about setting the coherent mask based on size automatically.
>>> I'm not sure about the size but I think we can assume is 32-bit mask + 1
>>> if it is not specified in the DT. Instead of just assuming a default
>>> mask, let's assume a default size and create the mask based on this
>>> (untested):
>>>
>>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>>> index 5b33c6a21807..9ff8d1286b44 100644
>>> --- a/drivers/of/platform.c
>>> +++ b/drivers/of/platform.c
>>> @@ -170,10 +170,10 @@ static void of_dma_configure(struct device *dev)
>>>    	struct iommu_ops *iommu;
>>>
>>>    	/*
>>> -	 * Set default dma-mask to 32 bit. Drivers are expected to setup
>>> -	 * the correct supported dma_mask.
>>> +	 * Set default size to cover the 32-bit. Drivers are expected to setup
>>> +	 * the correct size and dma_mask.
>>>    	 */
>>> -	dev->coherent_dma_mask = DMA_BIT_MASK(32);
>>> +	size = 1ULL<<  32;
>>>
>>>    	/*
>>>    	 * Set it to coherent_dma_mask by default if the architecture
>>> @@ -185,13 +185,24 @@ static void of_dma_configure(struct device *dev)
>>>    	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", dev->dma_pfn_offset);
>>>    	}
>>>    	dev->dma_pfn_offset = offset;
>>>
>>> +	/*
>>> +	 * Workaround for DTs setting the size to a mask or 0.
>>> +	 */
>>> +	if (is_power_of_2(size + 1))
>>> +		size += 1;
>>
>> In fact, since the ilog2 below ends up effectively rounding down, we
>> might as well do away with this check as well and just add 1
>> unconditionally. The only time it makes any difference is when we want
>> it to anyway!
>
> Well, we could simply ignore the is_power_of_2() check but without
> incrementing size as we don't know what arch_setup_dma_ops() does with
> it.
>
> I think we can remove this check altogether (we leaved without it for a
> while) but we need to add 1 when calculating the mask:
>
> 	dev->coherent_dma_mask = min(DMA_BIT_MASK(32),
> 				     DMA_BIT_MASK(ilog2(size + 1)));
>
For Keystone, the dma_addr is to be taken care as well to determine the 
mask. The above will not work.

Based on the discussion so far, this is the function I have come up with 
incorporating the suggestions. Please review this and see if I have 
missed out any. This works fine on Keystone.

void of_dma_configure(struct device *dev, struct device_node *np)
{
	u64 dma_addr = 0, paddr, size;
	int ret;
	bool coherent;
	unsigned long offset = 0;
	struct iommu_ops *iommu;

	/*
	 * Set default size to cover the 32-bit. Drivers are expected to setup
	 * the correct size and dma_mask.
   	 */
	size = 1ULL << 32;

	ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
	if (!ret) {
		offset = PFN_DOWN(paddr - dma_addr);
		if (!size) {
			dev_err(dev, "Invalid size (%llx)\n",
				size);
			return;
		}
		if (size & 1) {
			size = size + 1;
			dev_warn(dev, "Incorrect usage of size (%llx)\n",
				 size);
		}
		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
	}
	dev->dma_pfn_offset = offset;

	/*
	 * Coherent DMA masks larger than 32-bit must be explicitly set by the
	 * driver.
	 */
	dev->coherent_dma_mask = min(DMA_BIT_MASK(32),
				     DMA_BIT_MASK(ilog2(dma_addr + size)));
	/*
	 * Set dma_mask 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;

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


-- 
Murali Karicheri
Linux Kernel, Texas Instruments

Re: [PATCH v4 3/6] of: fix size when dma-range is not used

[Catalin Marinas]

On Fri, Jan 30, 2015 at 06:06:27PM +0000, Murali Karicheri wrote:
> On 01/28/2015 12:30 PM, Catalin Marinas wrote:
> > I think we can remove this check altogether (we leaved without it for a
> > while) but we need to add 1 when calculating the mask:
> >
> > 	dev->coherent_dma_mask = min(DMA_BIT_MASK(32),
> > 				     DMA_BIT_MASK(ilog2(size + 1)));
> >
> For Keystone, the dma_addr is to be taken care as well to determine the 
> mask. The above will not work.

This was discussed before (not on this thread) and dma_addr should not
affect the mask, it only affects the pfn offset.

> Based on the discussion so far, this is the function I have come up with 
> incorporating the suggestions. Please review this and see if I have 
> missed out any. This works fine on Keystone.
> 
> void of_dma_configure(struct device *dev, struct device_node *np)
> {
> 	u64 dma_addr = 0, paddr, size;
> 	int ret;
> 	bool coherent;
> 	unsigned long offset = 0;
> 	struct iommu_ops *iommu;
> 
> 	/*
> 	 * Set default size to cover the 32-bit. Drivers are expected to setup
> 	 * the correct size and dma_mask.
>    	 */
> 	size = 1ULL << 32;
> 
> 	ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
> 	if (!ret) {
> 		offset = PFN_DOWN(paddr - dma_addr);
> 		if (!size) {
> 			dev_err(dev, "Invalid size (%llx)\n",
> 				size);
> 			return;
> 		}
> 		if (size & 1) {
> 			size = size + 1;
> 			dev_warn(dev, "Incorrect usage of size (%llx)\n",
> 				 size);
> 		}
> 		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
> 	}
> 	dev->dma_pfn_offset = offset;
> 
> 	/*
> 	 * Coherent DMA masks larger than 32-bit must be explicitly set by the
> 	 * driver.
> 	 */
> 	dev->coherent_dma_mask = min(DMA_BIT_MASK(32),
> 				     DMA_BIT_MASK(ilog2(dma_addr + size)));

That's not correct, coherent_dma_mask should still be calculated solely
based on size, not dma_addr.

Functions like swiotlb_dma_supported() use phys_to_dma() which on ARM
(32-bit) subtracts the dma_pfn_offset, so the mask based on size works
fine.

In the arm64 tree, we haven't taken dma_pfn_offset into account for
phys_to_dma() yet but if needed for a SoC, we'll add it.

-- 
Catalin
--
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 v4 3/6] of: fix size when dma-range is not used

[Murali Karicheri]

On 02/02/2015 07:18 AM, Catalin Marinas wrote:
> On Fri, Jan 30, 2015 at 06:06:27PM +0000, Murali Karicheri wrote:
>> On 01/28/2015 12:30 PM, Catalin Marinas wrote:
>>> I think we can remove this check altogether (we leaved without it for a
>>> while) but we need to add 1 when calculating the mask:
>>>
>>> 	dev->coherent_dma_mask = min(DMA_BIT_MASK(32),
>>> 				     DMA_BIT_MASK(ilog2(size + 1)));
>>>
>> For Keystone, the dma_addr is to be taken care as well to determine the
>> mask. The above will not work.
>
> This was discussed before (not on this thread) and dma_addr should not
> affect the mask, it only affects the pfn offset.
>
>> Based on the discussion so far, this is the function I have come up with
>> incorporating the suggestions. Please review this and see if I have
>> missed out any. This works fine on Keystone.
>>
>> void of_dma_configure(struct device *dev, struct device_node *np)
>> {
>> 	u64 dma_addr = 0, paddr, size;
>> 	int ret;
>> 	bool coherent;
>> 	unsigned long offset = 0;
>> 	struct iommu_ops *iommu;
>>
>> 	/*
>> 	 * Set default size to cover the 32-bit. Drivers are expected to setup
>> 	 * the correct size and dma_mask.
>>     	 */
>> 	size = 1ULL<<  32;
>>
>> 	ret = of_dma_get_range(np,&dma_addr,&paddr,&size);
>> 	if (!ret) {
>> 		offset = PFN_DOWN(paddr - dma_addr);
>> 		if (!size) {
>> 			dev_err(dev, "Invalid size (%llx)\n",
>> 				size);
>> 			return;
>> 		}
>> 		if (size&  1) {
>> 			size = size + 1;
>> 			dev_warn(dev, "Incorrect usage of size (%llx)\n",
>> 				 size);
>> 		}
>> 		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
>> 	}
>> 	dev->dma_pfn_offset = offset;
>>
>> 	/*
>> 	 * Coherent DMA masks larger than 32-bit must be explicitly set by the
>> 	 * driver.
>> 	 */
>> 	dev->coherent_dma_mask = min(DMA_BIT_MASK(32),
>> 				     DMA_BIT_MASK(ilog2(dma_addr + size)));
>
> That's not correct, coherent_dma_mask should still be calculated solely
> based on size, not dma_addr.
>
> Functions like swiotlb_dma_supported() use phys_to_dma() which on ARM
> (32-bit) subtracts the dma_pfn_offset, so the mask based on size works
> fine.
>
> In the arm64 tree, we haven't taken dma_pfn_offset into account for
> phys_to_dma() yet but if needed for a SoC, we'll add it.
>
I need to hear Arnd's comment on this. I am seeing an issue without this 
change. Probably it needs a change else where. I will post the error I 
am getting to this list.

Murali

-- 
Murali Karicheri
Linux Kernel, Texas Instruments

Re: [PATCH v4 3/6] of: fix size when dma-range is not used

[Murali Karicheri]

On 02/02/2015 07:18 AM, Catalin Marinas wrote:
> On Fri, Jan 30, 2015 at 06:06:27PM +0000, Murali Karicheri wrote:
>> On 01/28/2015 12:30 PM, Catalin Marinas wrote:
>>> I think we can remove this check altogether (we leaved without it for a
>>> while) but we need to add 1 when calculating the mask:
>>>
>>> 	dev->coherent_dma_mask = min(DMA_BIT_MASK(32),
>>> 				     DMA_BIT_MASK(ilog2(size + 1)));
>>>
>> For Keystone, the dma_addr is to be taken care as well to determine the
>> mask. The above will not work.
>
> This was discussed before (not on this thread) and dma_addr should not
> affect the mask, it only affects the pfn offset.
>
>> Based on the discussion so far, this is the function I have come up with
>> incorporating the suggestions. Please review this and see if I have
>> missed out any. This works fine on Keystone.
>>
>> void of_dma_configure(struct device *dev, struct device_node *np)
>> {
>> 	u64 dma_addr = 0, paddr, size;
>> 	int ret;
>> 	bool coherent;
>> 	unsigned long offset = 0;
>> 	struct iommu_ops *iommu;
>>
>> 	/*
>> 	 * Set default size to cover the 32-bit. Drivers are expected to setup
>> 	 * the correct size and dma_mask.
>>     	 */
>> 	size = 1ULL<<  32;
>>
>> 	ret = of_dma_get_range(np,&dma_addr,&paddr,&size);
>> 	if (!ret) {
>> 		offset = PFN_DOWN(paddr - dma_addr);
>> 		if (!size) {
>> 			dev_err(dev, "Invalid size (%llx)\n",
>> 				size);
>> 			return;
>> 		}
>> 		if (size&  1) {
>> 			size = size + 1;
>> 			dev_warn(dev, "Incorrect usage of size (%llx)\n",
>> 				 size);
>> 		}
>> 		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
>> 	}
>> 	dev->dma_pfn_offset = offset;
>>
>> 	/*
>> 	 * Coherent DMA masks larger than 32-bit must be explicitly set by the
>> 	 * driver.
>> 	 */
>> 	dev->coherent_dma_mask = min(DMA_BIT_MASK(32),
>> 				     DMA_BIT_MASK(ilog2(dma_addr + size)));
>
> That's not correct, coherent_dma_mask should still be calculated solely
> based on size, not dma_addr.
>
> Functions like swiotlb_dma_supported() use phys_to_dma() which on ARM
> (32-bit) subtracts the dma_pfn_offset, so the mask based on size works
> fine.
>
> In the arm64 tree, we haven't taken dma_pfn_offset into account for
> phys_to_dma() yet but if needed for a SoC, we'll add it.
>
Catalin,

The size based dma mask calculation itself can be a separate topic 
patch. This series is adding important fix to get the PCI driver 
functional and I would like to get this merged as soon as possible. I 
also want to hear from Arnd about yout comment as we had discussed this 
in the initial discussion of this patch series and 8/8 is essentially 
added based on that discussion. I will add a simple check to catch and 
warn the incorrect size setting in DT for dma-range as suggested by Rob 
Herring and create a new patch to for size based mask calculation. I 
will be sending v6 (expected to be merged soon) today and will work to 
add a new patch. Before that we need to agree on what is the content of 
the patch.

1. On keystone, DMA address start at 0x8000_0000 and DMA-able memory is 
2G from the above base address. So without taking into account the 
dma_addr, mask calculated will be 0x7fff_ffff where as we need that to 
be 0xffff_ffff for keystone. So need to use this in the calculation.

2. W.r.t arm_pfn_offset, this was added to support Keystone as the DDR 
physical address for LPAE startes at 0x8_0000_0000 and the pfn offset is 
calculated as the PFN of (0x8_0000_0000 - 0x8000_0000) to do the dma 
address to DDR address translation. I haven't looked at 
swiotlb_dma_supported() but will do so.

Murali



-- 
Murali Karicheri
Linux Kernel, Texas Instruments

Re: [PATCH v4 3/6] of: fix size when dma-range is not used

[Catalin Marinas]

Murali,

On Thu, Feb 05, 2015 at 09:42:27PM +0000, Murali Karicheri wrote:
> On 02/02/2015 07:18 AM, Catalin Marinas wrote:
> > On Fri, Jan 30, 2015 at 06:06:27PM +0000, Murali Karicheri wrote:
> >> On 01/28/2015 12:30 PM, Catalin Marinas wrote:
> >>> I think we can remove this check altogether (we leaved without it for a
> >>> while) but we need to add 1 when calculating the mask:
> >>>
> >>> 	dev->coherent_dma_mask = min(DMA_BIT_MASK(32),
> >>> 				     DMA_BIT_MASK(ilog2(size + 1)));
> >>>
> >> For Keystone, the dma_addr is to be taken care as well to determine the
> >> mask. The above will not work.
> >
> > This was discussed before (not on this thread) and dma_addr should not
> > affect the mask, it only affects the pfn offset.
> >
> >> Based on the discussion so far, this is the function I have come up with
> >> incorporating the suggestions. Please review this and see if I have
> >> missed out any. This works fine on Keystone.
> >>
> >> void of_dma_configure(struct device *dev, struct device_node *np)
> >> {
> >> 	u64 dma_addr = 0, paddr, size;
> >> 	int ret;
> >> 	bool coherent;
> >> 	unsigned long offset = 0;
> >> 	struct iommu_ops *iommu;
> >>
> >> 	/*
> >> 	 * Set default size to cover the 32-bit. Drivers are expected to setup
> >> 	 * the correct size and dma_mask.
> >>     	 */
> >> 	size = 1ULL<<  32;
> >>
> >> 	ret = of_dma_get_range(np,&dma_addr,&paddr,&size);
> >> 	if (!ret) {
> >> 		offset = PFN_DOWN(paddr - dma_addr);
> >> 		if (!size) {
> >> 			dev_err(dev, "Invalid size (%llx)\n",
> >> 				size);
> >> 			return;
> >> 		}
> >> 		if (size&  1) {
> >> 			size = size + 1;
> >> 			dev_warn(dev, "Incorrect usage of size (%llx)\n",
> >> 				 size);
> >> 		}
> >> 		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
> >> 	}
> >> 	dev->dma_pfn_offset = offset;
> >>
> >> 	/*
> >> 	 * Coherent DMA masks larger than 32-bit must be explicitly set by the
> >> 	 * driver.
> >> 	 */
> >> 	dev->coherent_dma_mask = min(DMA_BIT_MASK(32),
> >> 				     DMA_BIT_MASK(ilog2(dma_addr + size)));
> >
> > That's not correct, coherent_dma_mask should still be calculated solely
> > based on size, not dma_addr.
> >
> > Functions like swiotlb_dma_supported() use phys_to_dma() which on ARM
> > (32-bit) subtracts the dma_pfn_offset, so the mask based on size works
> > fine.
> >
> > In the arm64 tree, we haven't taken dma_pfn_offset into account for
> > phys_to_dma() yet but if needed for a SoC, we'll add it.
> 
> The size based dma mask calculation itself can be a separate topic 
> patch. This series is adding important fix to get the PCI driver 
> functional and I would like to get this merged as soon as possible.

Well as long as you don't break the existing users of
of_dma_configure() ;).

> I also want to hear from Arnd about yout comment as we had discussed
> this in the initial discussion of this patch series and 8/8 is
> essentially added based on that discussion. I will add a simple check
> to catch and warn the incorrect size setting in DT for dma-range as
> suggested by Rob Herring and create a new patch to for size based mask
> calculation. I will be sending v6 (expected to be merged soon) today
> and will work to add a new patch. Before that we need to agree on what
> is the content of the patch.
> 
> 1. On keystone, DMA address start at 0x8000_0000 and DMA-able memory is 
> 2G from the above base address. So without taking into account the
> dma_addr, mask calculated will be 0x7fff_ffff where as we need that to 
> be 0xffff_ffff for keystone. So need to use this in the calculation.

Ah, you are right. I confused dma_addr in your patch with the offset.

> 2. W.r.t arm_pfn_offset, this was added to support Keystone as the DDR 
> physical address for LPAE startes at 0x8_0000_0000 and the pfn offset is 
> calculated as the PFN of (0x8_0000_0000 - 0x8000_0000) to do the dma 
> address to DDR address translation.

Indeed.

I'll look at your patches again tomorrow.

-- 
Catalin

[PATCH v4 6/6] arm: dma-mapping: updates to limit dma_mask and iommu mapping size

[Murali Karicheri]

Limit the dma_mask to minimum of dma_mask and dma_base + size - 1.

Also arm_iommu_create_mapping() has size parameter of size_t and
arm_setup_iommu_dma_ops() can take a value higher than that. So
limit the size to SIZE_MAX.

Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
---
 arch/arm/mm/dma-mapping.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 7864797..a1f9030 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -2004,6 +2004,13 @@ static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
 	if (!iommu)
 		return false;
 
+	/*
+	 * currently arm_iommu_create_mapping() takes a max of size_t
+	 * for size param. So check this limit for now.
+	 */
+	if (size > SIZE_MAX)
+		return false;
+
 	mapping = arm_iommu_create_mapping(dev->bus, dma_base, size);
 	if (IS_ERR(mapping)) {
 		pr_warn("Failed to create %llu-byte IOMMU mapping for device %s\n",
@@ -2053,6 +2060,9 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 {
 	struct dma_map_ops *dma_ops;
 
+	/* limit dma_mask to the lower of the two values */
+	*dev->dma_mask = min((*dev->dma_mask), (dma_base + size - 1));
+
 	dev->archdata.dma_coherent = coherent;
 	if (arm_setup_iommu_dma_ops(dev, dma_base, size, iommu))
 		dma_ops = arm_get_iommu_dma_map_ops(coherent);
-- 
1.7.9.5

Re: [PATCH v4 6/6] arm: dma-mapping: updates to limit dma_mask and iommu mapping size

[Robin Murphy]

Hi Murali,

On 23/01/15 22:32, Murali Karicheri wrote:
> Limit the dma_mask to minimum of dma_mask and dma_base + size - 1.
>
> Also arm_iommu_create_mapping() has size parameter of size_t and
> arm_setup_iommu_dma_ops() can take a value higher than that. So
> limit the size to SIZE_MAX.
>
> Signed-off-by: Murali Karicheri <m-karicheri2-l0cyMroinI0@public.gmane.org>
> ---
>   arch/arm/mm/dma-mapping.c |   10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 7864797..a1f9030 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -2004,6 +2004,13 @@ static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
>   	if (!iommu)
>   		return false;
>
> +	/*
> +	 * currently arm_iommu_create_mapping() takes a max of size_t
> +	 * for size param. So check this limit for now.
> +	 */
> +	if (size > SIZE_MAX)
> +		return false;
> +
>   	mapping = arm_iommu_create_mapping(dev->bus, dma_base, size);
>   	if (IS_ERR(mapping)) {
>   		pr_warn("Failed to create %llu-byte IOMMU mapping for device %s\n",
> @@ -2053,6 +2060,9 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>   {
>   	struct dma_map_ops *dma_ops;
>
> +	/* limit dma_mask to the lower of the two values */
> +	*dev->dma_mask = min((*dev->dma_mask), (dma_base + size - 1));
> +

Is there any reason not to do this in of_dma_configure? It seems like 
something everyone could benefit from - I'd cooked up a dodgy workaround 
for the same issue in my arm64 IOMMU code, but handling it generically 
in common code would be much nicer.

Robin.

>   	dev->archdata.dma_coherent = coherent;
>   	if (arm_setup_iommu_dma_ops(dev, dma_base, size, iommu))
>   		dma_ops = arm_get_iommu_dma_map_ops(coherent);
>


--
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 v4 6/6] arm: dma-mapping: updates to limit dma_mask and iommu mapping size

[Catalin Marinas]

On Tue, Jan 27, 2015 at 11:12:32AM +0000, Robin Murphy wrote:
> On 23/01/15 22:32, Murali Karicheri wrote:
> > Limit the dma_mask to minimum of dma_mask and dma_base + size - 1.
> >
> > Also arm_iommu_create_mapping() has size parameter of size_t and
> > arm_setup_iommu_dma_ops() can take a value higher than that. So
> > limit the size to SIZE_MAX.
> >
> > Signed-off-by: Murali Karicheri <m-karicheri2-l0cyMroinI0@public.gmane.org>
> > ---
> >   arch/arm/mm/dma-mapping.c |   10 ++++++++++
> >   1 file changed, 10 insertions(+)
> >
> > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> > index 7864797..a1f9030 100644
> > --- a/arch/arm/mm/dma-mapping.c
> > +++ b/arch/arm/mm/dma-mapping.c
> > @@ -2004,6 +2004,13 @@ static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
> >   	if (!iommu)
> >   		return false;
> >
> > +	/*
> > +	 * currently arm_iommu_create_mapping() takes a max of size_t
> > +	 * for size param. So check this limit for now.
> > +	 */
> > +	if (size > SIZE_MAX)
> > +		return false;
> > +
> >   	mapping = arm_iommu_create_mapping(dev->bus, dma_base, size);
> >   	if (IS_ERR(mapping)) {
> >   		pr_warn("Failed to create %llu-byte IOMMU mapping for device %s\n",
> > @@ -2053,6 +2060,9 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
> >   {
> >   	struct dma_map_ops *dma_ops;
> >
> > +	/* limit dma_mask to the lower of the two values */
> > +	*dev->dma_mask = min((*dev->dma_mask), (dma_base + size - 1));
> > +
> 
> Is there any reason not to do this in of_dma_configure? It seems like 
> something everyone could benefit from - I'd cooked up a dodgy workaround 
> for the same issue in my arm64 IOMMU code, but handling it generically 
> in common code would be much nicer.

I agree. I started something here:

http://article.gmane.org/gmane.linux.kernel/1835096

but I don't remember to have got to a clear conclusion.

-- 
Catalin

Re: [PATCH v4 6/6] arm: dma-mapping: updates to limit dma_mask and iommu mapping size

[Murali Karicheri]

On 01/27/2015 06:34 AM, Catalin Marinas wrote:
> On Tue, Jan 27, 2015 at 11:12:32AM +0000, Robin Murphy wrote:
>> On 23/01/15 22:32, Murali Karicheri wrote:
>>> Limit the dma_mask to minimum of dma_mask and dma_base + size - 1.
>>>
>>> Also arm_iommu_create_mapping() has size parameter of size_t and
>>> arm_setup_iommu_dma_ops() can take a value higher than that. So
>>> limit the size to SIZE_MAX.
>>>
>>> Signed-off-by: Murali Karicheri<m-karicheri2-l0cyMroinI0@public.gmane.org>
>>> ---
>>>    arch/arm/mm/dma-mapping.c |   10 ++++++++++
>>>    1 file changed, 10 insertions(+)
>>>
>>> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
>>> index 7864797..a1f9030 100644
>>> --- a/arch/arm/mm/dma-mapping.c
>>> +++ b/arch/arm/mm/dma-mapping.c
>>> @@ -2004,6 +2004,13 @@ static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
>>>    	if (!iommu)
>>>    		return false;
>>>
>>> +	/*
>>> +	 * currently arm_iommu_create_mapping() takes a max of size_t
>>> +	 * for size param. So check this limit for now.
>>> +	 */
>>> +	if (size>  SIZE_MAX)
>>> +		return false;
>>> +
>>>    	mapping = arm_iommu_create_mapping(dev->bus, dma_base, size);
>>>    	if (IS_ERR(mapping)) {
>>>    		pr_warn("Failed to create %llu-byte IOMMU mapping for device %s\n",
>>> @@ -2053,6 +2060,9 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>>>    {
>>>    	struct dma_map_ops *dma_ops;
>>>
>>> +	/* limit dma_mask to the lower of the two values */
>>> +	*dev->dma_mask = min((*dev->dma_mask), (dma_base + size - 1));
>>> +
>>
>> Is there any reason not to do this in of_dma_configure? It seems like
>> something everyone could benefit from - I'd cooked up a dodgy workaround
>> for the same issue in my arm64 IOMMU code, but handling it generically
>> in common code would be much nicer.

Ok Will move this to of_dma_configure().

Murali

>
> I agree. I started something here:
>
> http://article.gmane.org/gmane.linux.kernel/1835096
>
> but I don't remember to have got to a clear conclusion.
>


-- 
Murali Karicheri
Linux Kernel, Texas Instruments