[PATCH 0/7] of: setup dma parameters using dma-ranges and dma-coherent

[PATCH 0/7] of: setup dma parameters using dma-ranges and dma-coherent

[Santosh Shilimkar]

Based on comments from [1] discussion, it seems there is a need to
have a generic support to configure dma device parameters.
Series introduces support for setting up dma parameters based on
device tree properties like 'dma-ranges' and 'dma-coherent'.

The 'dma-ranges' helps to take care of few DMAable system memory restrictions
by use of dma_pfn_offset which we maintain now per device. Arch code then
uses it for dma address translations for such cases. We also setup
dma_mask accordingly during the device creation process.

The 'dma-coherent' property is used to setup coherent dma_ops. Arches
which are always coherent can make use of ARCH_IS_DMA_COHERENT to setup
coherent dma_ops always irrespective of 'dma-coherent' property.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Olof Johansson <olof@lixom.net>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Grygorii Strashko <grygorii.strashko@ti.com>

Grygorii Strashko (3):
  of: introduce of_dma_get_range() helper
  of: configure the platform device dma_mask and dma_pfn_offset
  ARM: dma: Use dma_pfn_offset for dma address translation

Santosh Shilimkar (4):
  device: introduce per device dma_pfn_offset
  of: introduce of_dma_is_coherent() helper
  of: Add set_arch_dma_coherent_ops() and setup coherent dma_ops
  ARM: dma: implement set_arch_dma_coherent_ops()

 arch/arm/include/asm/dma-mapping.h |   24 ++++-
 drivers/of/platform.c              |  196 +++++++++++++++++++++++++++++++++++-
 include/linux/device.h             |    2 +
 include/linux/dma-mapping.h        |    7 ++
 include/linux/of_platform.h        |   14 +++
 5 files changed, 236 insertions(+), 7 deletions(-)

Regards,
Santosh
[1] http://www.spinics.net/lists/arm-kernel/msg311678.html
-- 
1.7.9.5

[PATCH 1/7] device: introduce per device dma_pfn_offset

[Santosh Shilimkar]

On few architectures, there are few restrictions on DMAble area of system
RAM. That also means that devices needs to know about this restrictions so
that the dma_masks can be updated accordingly and dma address translation
helpers can add/subtract the dma offset.

In most of cases DMA addresses can be performed using offset value of
Bus address space relatively to physical address space as following:

PFN->DMA:  __pfn_to_phys(pfn + [-]dma_pfn_offset)
DMA->PFN:  __phys_to_pfn(dma_addr) + [-]dma_pfn_offset

So we introduce per device dma_pfn_offset which can be popullated
by architectures init code while creating the devices.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Olof Johansson <olof@lixom.net>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 include/linux/device.h |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index 952b010..6940b25 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -688,6 +688,7 @@ struct acpi_dev_node {
  * @coherent_dma_mask: Like dma_mask, but for alloc_coherent mapping as not all
  * 		hardware supports 64-bit addresses for consistent allocations
  * 		such descriptors.
+ * @dma_pfn_offset: offset of DMA memory range relatively of RAM
  * @dma_parms:	A low level driver may set these to teach IOMMU code about
  * 		segment limitations.
  * @dma_pools:	Dma pools (if dma'ble device).
@@ -753,6 +754,7 @@ struct device {
 					     not all hardware supports
 					     64 bit addresses for consistent
 					     allocations such descriptors. */
+	unsigned long	dma_pfn_offset;
 
 	struct device_dma_parameters *dma_parms;
 
-- 
1.7.9.5

[PATCH 2/7] of: introduce of_dma_get_range() helper

[Santosh Shilimkar]

From: Grygorii Strashko <grygorii.strashko@ti.com>

The of_dma_get_range() allows to find "dma-range" property for
the specified device and parse it.
 dma-ranges format:
   DMA addr (dma_addr)          : naddr cells
   CPU addr (phys_addr_t)       : pna cells
   size                         : nsize cells

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Olof Johansson <olof@lixom.net>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 drivers/of/platform.c       |   85 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_platform.h |    8 ++++
 2 files changed, 93 insertions(+)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 404d1da..2265a55 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -485,4 +485,89 @@ int of_platform_populate(struct device_node *root,
 	return rc;
 }
 EXPORT_SYMBOL_GPL(of_platform_populate);
+
+/**
+ * of_dma_get_range - Get DMA range info
+ * @np:		device node to get DMA range info
+ * @dma_addr:	pointer to store initial DMA address of DMA range
+ * @paddr:	pointer to store initial CPU address of DMA range
+ * @size:	pointer to store size of DMA range
+ *
+ * Look in bottom up direction for the first "dma-range" property
+ * and parse it.
+ *  dma-ranges format:
+ *	DMA addr (dma_addr)	: naddr cells
+ *	CPU addr (phys_addr_t)	: pna cells
+ *	size			: nsize cells
+ *
+ * It returns -ENODEV if "dma-ranges" property was not found
+ * for this device in DT.
+ */
+extern int of_dma_get_range(struct device_node *np, dma_addr_t *dma_addr,
+		phys_addr_t *paddr, phys_addr_t *size)
+{
+	struct device_node *node = np;
+	const u32 *ranges = NULL;
+	int len, naddr, nsize, pna;
+	int ret = 0;
+
+	if (!node)
+		return -EINVAL;
+
+	while (1) {
+		naddr = of_n_addr_cells(node);
+		nsize = of_n_size_cells(node);
+		node = of_get_next_parent(node);
+		if (!node)
+			break;
+
+		ranges = of_get_property(node, "dma-ranges", &len);
+
+		/* Ignore empty ranges, they imply no translation required */
+		if (ranges && len > 0)
+			break;
+
+		/*
+		 * At least empty ranges has to be defined for parent node if
+		 * DMA is supported
+		 */
+		if (!ranges)
+			break;
+	}
+
+	if (!ranges) {
+		pr_debug("%s: no dma-ranges found for node(%s)\n",
+			 __func__, np->full_name);
+		ret = -ENODEV;
+		goto out;
+	}
+
+	len /= sizeof(u32);
+
+	pna = of_n_addr_cells(node);
+
+	/* dma-ranges format:
+	 * DMA addr	: naddr cells
+	 * CPU addr	: pna cells
+	 * size		: nsize cells
+	 */
+	*dma_addr = of_read_number(ranges, naddr);
+	*paddr = of_translate_dma_address(np, ranges);
+	if (*paddr == OF_BAD_ADDR) {
+		pr_err("%s: translation of DMA address(%pad) to CPU address failed node(%s)\n",
+		       __func__, dma_addr, np->full_name);
+		ret = -EINVAL;
+	}
+
+	*size = of_read_number(ranges + naddr + pna, nsize);
+
+	pr_debug("dma_addr(%pad) cpu_addr(%pa) size(%pa)\n",
+		 dma_addr, paddr, size);
+
+out:
+	of_node_put(node);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(of_dma_get_range);
 #endif /* CONFIG_OF_ADDRESS */
diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
index 05cb4a9..ba7d3dc 100644
--- a/include/linux/of_platform.h
+++ b/include/linux/of_platform.h
@@ -72,6 +72,8 @@ extern int of_platform_populate(struct device_node *root,
 				const struct of_device_id *matches,
 				const struct of_dev_auxdata *lookup,
 				struct device *parent);
+extern int of_dma_get_range(struct device_node *np, dma_addr_t *dma_addr,
+				phys_addr_t *paddr, phys_addr_t *size);
 #else
 static inline int of_platform_populate(struct device_node *root,
 					const struct of_device_id *matches,
@@ -80,6 +82,12 @@ static inline int of_platform_populate(struct device_node *root,
 {
 	return -ENODEV;
 }
+
+static inline int of_dma_get_range(struct device_node *np, dma_addr_t *dma_addr,
+					phys_addr_t *paddr, phys_addr_t *size)
+{
+	return -ENODEV;
+}
 #endif
 
 #endif	/* _LINUX_OF_PLATFORM_H */
-- 
1.7.9.5

[PATCH 3/7] of: introduce of_dma_is_coherent() helper

[Santosh Shilimkar]

The of_dma_is_coherent() helper parses the given DT device
node to see if the "dma-coherent" property is supported and
returns true or false accordingly.

For the architectures which are fully dma coherent and don't need per device
property, it can enable CONFIG_ARCH_IS_DMA_COHERENT config option which
enables DMA coherent for all devices by default.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Olof Johansson <olof@lixom.net>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 drivers/of/platform.c       |   31 +++++++++++++++++++++++++++++++
 include/linux/of_platform.h |    6 ++++++
 2 files changed, 37 insertions(+)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 2265a55..bd080db 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -570,4 +570,35 @@ out:
 	return ret;
 }
 EXPORT_SYMBOL_GPL(of_dma_get_range);
+
+/**
+ * of_dma_is_coherent - Check if device is coherent
+ * @np:	device node
+ *
+ * It returns true if "dma-coherent" property was found
+ * for this device in DT.
+ */
+#ifndef CONFIG_ARCH_IS_DMA_COHERENT
+bool of_dma_is_coherent(struct device_node *np)
+{
+	struct device_node *node = np;
+
+	while (node) {
+		if (of_property_read_bool(node, "dma-coherent")) {
+			of_node_put(node);
+			return true;
+		}
+		node = of_get_next_parent(node);
+	}
+	return false;
+}
+EXPORT_SYMBOL_GPL(of_dma_is_coherent);
+#else
+inline bool of_dma_is_coherent(struct device_node *np)
+{
+	/* ARCH is fully dma coherent and don't need per device control */
+	return true;
+}
+EXPORT_SYMBOL_GPL(of_dma_is_coherent);
+#endif
 #endif /* CONFIG_OF_ADDRESS */
diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
index ba7d3dc..48e0748 100644
--- a/include/linux/of_platform.h
+++ b/include/linux/of_platform.h
@@ -74,6 +74,7 @@ extern int of_platform_populate(struct device_node *root,
 				struct device *parent);
 extern int of_dma_get_range(struct device_node *np, dma_addr_t *dma_addr,
 				phys_addr_t *paddr, phys_addr_t *size);
+extern bool of_dma_is_coherent(struct device_node *np);
 #else
 static inline int of_platform_populate(struct device_node *root,
 					const struct of_device_id *matches,
@@ -88,6 +89,11 @@ static inline int of_dma_get_range(struct device_node *np, dma_addr_t *dma_addr,
 {
 	return -ENODEV;
 }
+
+static inline bool of_dma_is_coherent(struct device_node *np)
+{
+	return false;
+}
 #endif
 
 #endif	/* _LINUX_OF_PLATFORM_H */
-- 
1.7.9.5

Re: [PATCH 3/7] of: introduce of_dma_is_coherent() helper

[Rob Herring]

On Thu, Mar 6, 2014 at 3:19 AM, Santosh Shilimkar
<santosh.shilimkar@ti.com> wrote:
> The of_dma_is_coherent() helper parses the given DT device
> node to see if the "dma-coherent" property is supported and
> returns true or false accordingly.
>
> For the architectures which are fully dma coherent and don't need per device
> property, it can enable CONFIG_ARCH_IS_DMA_COHERENT config option which
> enables DMA coherent for all devices by default.

This worries me. I killed off arch_is_coherent() for arm. Now we're
adding something back. Also, we already have HAVE_GENERIC_DMA_COHERENT
which is different, but the names will be confusing. MIPS also has
DMA_NONCOHERENT.

>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Olof Johansson <olof@lixom.net>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> ---
>  drivers/of/platform.c       |   31 +++++++++++++++++++++++++++++++
>  include/linux/of_platform.h |    6 ++++++
>  2 files changed, 37 insertions(+)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 2265a55..bd080db 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -570,4 +570,35 @@ out:
>         return ret;
>  }
>  EXPORT_SYMBOL_GPL(of_dma_get_range);
> +
> +/**
> + * of_dma_is_coherent - Check if device is coherent
> + * @np:        device node
> + *
> + * It returns true if "dma-coherent" property was found
> + * for this device in DT.
> + */
> +#ifndef CONFIG_ARCH_IS_DMA_COHERENT
> +bool of_dma_is_coherent(struct device_node *np)
> +{
> +       struct device_node *node = np;
> +
> +       while (node) {

This screams for a for_each_parent_of_node helper.

> +               if (of_property_read_bool(node, "dma-coherent")) {
> +                       of_node_put(node);
> +                       return true;
> +               }
> +               node = of_get_next_parent(node);
> +       }
> +       return false;
> +}
> +EXPORT_SYMBOL_GPL(of_dma_is_coherent);
> +#else
> +inline bool of_dma_is_coherent(struct device_node *np)

This is in the header and should not be here.

> +{
> +       /* ARCH is fully dma coherent and don't need per device control */
> +       return true;
> +}
> +EXPORT_SYMBOL_GPL(of_dma_is_coherent);
> +#endif
>  #endif /* CONFIG_OF_ADDRESS */
> diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
> index ba7d3dc..48e0748 100644
> --- a/include/linux/of_platform.h
> +++ b/include/linux/of_platform.h
> @@ -74,6 +74,7 @@ extern int of_platform_populate(struct device_node *root,
>                                 struct device *parent);
>  extern int of_dma_get_range(struct device_node *np, dma_addr_t *dma_addr,
>                                 phys_addr_t *paddr, phys_addr_t *size);
> +extern bool of_dma_is_coherent(struct device_node *np);
>  #else
>  static inline int of_platform_populate(struct device_node *root,
>                                         const struct of_device_id *matches,
> @@ -88,6 +89,11 @@ static inline int of_dma_get_range(struct device_node *np, dma_addr_t *dma_addr,
>  {
>         return -ENODEV;
>  }
> +
> +static inline bool of_dma_is_coherent(struct device_node *np)
> +{
> +       return false;
> +}
>  #endif

I don't think you have the ifdefs right here.

Rob

Re: [PATCH 3/7] of: introduce of_dma_is_coherent() helper

[Santosh Shilimkar]

On Friday 07 March 2014 11:13 AM, Rob Herring wrote:
> On Thu, Mar 6, 2014 at 3:19 AM, Santosh Shilimkar
> <santosh.shilimkar@ti.com> wrote:
>> The of_dma_is_coherent() helper parses the given DT device
>> node to see if the "dma-coherent" property is supported and
>> returns true or false accordingly.
>>
>> For the architectures which are fully dma coherent and don't need per device
>> property, it can enable CONFIG_ARCH_IS_DMA_COHERENT config option which
>> enables DMA coherent for all devices by default.
> 
> This worries me. I killed off arch_is_coherent() for arm. Now we're
> adding something back. Also, we already have HAVE_GENERIC_DMA_COHERENT
> which is different, but the names will be confusing. MIPS also has
> DMA_NONCOHERENT.
> 
Thanks for comments Rob. I will address them in next version.
Specifically about ARCH_IS_DMA_COHERENT, I wasn't very comfortable either
while adding it. But as Arnd mentioned, there is a need to have a way
for the arch's which are fully coherent to use coherent ops by default.

Am not sure whats the best way to have such support without imposing
any special updates on such arches.

Arnd, Any better alternative here ?

Regards,
Santosh

Re: [PATCH 3/7] of: introduce of_dma_is_coherent() helper

[Rob Herring]

On Thu, Mar 6, 2014 at 9:44 PM, Santosh Shilimkar
<santosh.shilimkar-l0cyMroinI0@public.gmane.org> wrote:
> On Friday 07 March 2014 11:13 AM, Rob Herring wrote:
>> On Thu, Mar 6, 2014 at 3:19 AM, Santosh Shilimkar
>> <santosh.shilimkar-l0cyMroinI0@public.gmane.org> wrote:
>>> The of_dma_is_coherent() helper parses the given DT device
>>> node to see if the "dma-coherent" property is supported and
>>> returns true or false accordingly.
>>>
>>> For the architectures which are fully dma coherent and don't need per device
>>> property, it can enable CONFIG_ARCH_IS_DMA_COHERENT config option which
>>> enables DMA coherent for all devices by default.
>>
>> This worries me. I killed off arch_is_coherent() for arm. Now we're
>> adding something back. Also, we already have HAVE_GENERIC_DMA_COHERENT
>> which is different, but the names will be confusing. MIPS also has
>> DMA_NONCOHERENT.
>>
> Thanks for comments Rob. I will address them in next version.
> Specifically about ARCH_IS_DMA_COHERENT, I wasn't very comfortable either
> while adding it. But as Arnd mentioned, there is a need to have a way
> for the arch's which are fully coherent to use coherent ops by default.
>
> Am not sure whats the best way to have such support without imposing
> any special updates on such arches.

Thinking about this some more, if the arch is always coherent or
always non-coherent, then the default ops are always fine. In that
case set_arch_dma_coherent_ops is always a nop and of_dma_is_coherent
is a don't care.

Rob
--
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 3/7] of: introduce of_dma_is_coherent() helper

[Santosh Shilimkar]

On Friday 07 March 2014 11:55 AM, Rob Herring wrote:
> On Thu, Mar 6, 2014 at 9:44 PM, Santosh Shilimkar
> <santosh.shilimkar@ti.com> wrote:
>> On Friday 07 March 2014 11:13 AM, Rob Herring wrote:
>>> On Thu, Mar 6, 2014 at 3:19 AM, Santosh Shilimkar
>>> <santosh.shilimkar@ti.com> wrote:
>>>> The of_dma_is_coherent() helper parses the given DT device
>>>> node to see if the "dma-coherent" property is supported and
>>>> returns true or false accordingly.
>>>>
>>>> For the architectures which are fully dma coherent and don't need per device
>>>> property, it can enable CONFIG_ARCH_IS_DMA_COHERENT config option which
>>>> enables DMA coherent for all devices by default.
>>>
>>> This worries me. I killed off arch_is_coherent() for arm. Now we're
>>> adding something back. Also, we already have HAVE_GENERIC_DMA_COHERENT
>>> which is different, but the names will be confusing. MIPS also has
>>> DMA_NONCOHERENT.
>>>
>> Thanks for comments Rob. I will address them in next version.
>> Specifically about ARCH_IS_DMA_COHERENT, I wasn't very comfortable either
>> while adding it. But as Arnd mentioned, there is a need to have a way
>> for the arch's which are fully coherent to use coherent ops by default.
>>
>> Am not sure whats the best way to have such support without imposing
>> any special updates on such arches.
> 
> Thinking about this some more, if the arch is always coherent or
> always non-coherent, then the default ops are always fine. In that
> case set_arch_dma_coherent_ops is always a nop and of_dma_is_coherent
> is a don't care.
> 
Hmmm.. I guess you are right. In that case we can drop the need of
config option.

Regards,
Santosh

Re: [PATCH 3/7] of: introduce of_dma_is_coherent() helper

[Arnd Bergmann]

On Friday 07 March 2014, Santosh Shilimkar wrote:
> On Friday 07 March 2014 11:55 AM, Rob Herring wrote:
>
> > Thinking about this some more, if the arch is always coherent or
> > always non-coherent, then the default ops are always fine. In that
> > case set_arch_dma_coherent_ops is always a nop and of_dma_is_coherent
> > is a don't care.
> > 
> Hmmm.. I guess you are right. In that case we can drop the need of
> config option.

A compile-time config option clearly would not work here, because it
breaks multiplatform support when some platforms are different from
others. I can still see the possible need for a global run-time setting
though, to give some platform init code the ability to override the
DT flags when it knows better. That is probably what you want to do
on keystone without LPAE.

	Arnd

Re: [PATCH 3/7] of: introduce of_dma_is_coherent() helper

[Santosh Shilimkar]

On Saturday 08 March 2014 12:09 AM, Arnd Bergmann wrote:
> On Friday 07 March 2014, Santosh Shilimkar wrote:
>> On Friday 07 March 2014 11:55 AM, Rob Herring wrote:
>>
>>> Thinking about this some more, if the arch is always coherent or
>>> always non-coherent, then the default ops are always fine. In that
>>> case set_arch_dma_coherent_ops is always a nop and of_dma_is_coherent
>>> is a don't care.
>>>
>> Hmmm.. I guess you are right. In that case we can drop the need of
>> config option.
> 
> A compile-time config option clearly would not work here, because it
> breaks multiplatform support when some platforms are different from
> others. I can still see the possible need for a global run-time setting
> though, to give some platform init code the ability to override the
> DT flags when it knows better. That is probably what you want to do
> on keystone without LPAE.
> 
The compile time flag added was keeping in mind to be used per architecture
like ARM, X86 or powerpc on need basis. I understand that it might
be used by machines within the architectures but that was not my
intention. In any case config bit isn't a good idea.

Keystone without LPAE actually I don't need such an option. For
non-LPAE case, I need to modify the memory node and same goes
with dma-ranges info to use aliased address space.

How about we care about such an option when we actually need
it. As Rob pointed out, we are just fine for now with sane
defaults.

Regards,
Santosh

[PATCH 4/7] of: configure the platform device dma_mask and dma_pfn_offset

[Santosh Shilimkar]

From: Grygorii Strashko <grygorii.strashko@ti.com>

Retrieve DMA configuration from DT and setup platform device's DMA
parameters.

The DMA configuration in DT has to be specified using "dma-ranges"
property if supported. The DMA configuration applied
by dt_dma_configure() as following:
 - call of_get_dma_range() and get supported DMA range info
   (dma_addr, cpu_addr, dma_size);
 - if "not found" then fill dma_mask as DMA_BIT_MASK(32)
   (this is default behaviour);
 - if "failed" then clean up dma_mask (DMA not supported)
 - if ok then update devices DMA configuration:
      set dma_mask to (dma_addr + dma_size - 1)
      set dma_pfn_offset to PFN_DOWN(cpu_addr - dma_addr)

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Olof Johansson <olof@lixom.net>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 drivers/of/platform.c |   75 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 72 insertions(+), 3 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index bd080db..53bb12f 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -187,6 +187,77 @@ struct platform_device *of_device_alloc(struct device_node *np,
 EXPORT_SYMBOL(of_device_alloc);
 
 /**
+ * dt_dma_configure - apply default DMA configuration from dt
+ * @dev:	Device to apply DMA configuration
+ *
+ * Try to get devices's DMA configuration from DT and apply it.
+ * The DMA configuration is represented in DT by "dma-ranges" property.
+ * It configures:
+ *	dma_pfn_offset, dma_mask and coherent_dma_mask.
+ *
+ * In case if DMA configuration can't be acquired from DT the default one is
+ * applied:
+ *	dma_pfn_offset = 0,
+ *	dma_mask = DMA_BIT_MASK(32)
+ *	coherent_dma_mask = DMA_BIT_MASK(32).
+ *
+ * In case if platform code need to use own DMA configuration - it can use
+ * Platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE event to fix up
+ * DMA configuration.
+ */
+static void dt_dma_configure(struct device *dev)
+{
+	dma_addr_t dma_addr;
+	phys_addr_t paddr, size;
+	dma_addr_t dma_mask;
+	int ret;
+
+	/*
+	 * if dma-ranges property doesn't exist - use 32 bits DMA mask
+	 * by default and don't set skip archdata.dma_pfn_offset
+	 */
+	ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size);
+	if (ret == -ENODEV) {
+		dev->coherent_dma_mask = DMA_BIT_MASK(32);
+		if (!dev->dma_mask)
+			dev->dma_mask = &dev->coherent_dma_mask;
+		return;
+	}
+
+	/* if failed - disable DMA for device */
+	if (ret < 0) {
+		dev_err(dev, "failed to configure DMA\n");
+		return;
+	}
+
+	/* DMA ranges found. Calculate and set dma_pfn_offset */
+	dev->dma_pfn_offset = PFN_DOWN(paddr - dma_addr);
+
+	/* Configure DMA mask */
+	dev->dma_mask = &dev->coherent_dma_mask;
+	if (!dev->dma_mask)
+		return;
+
+	dma_mask = dma_addr + size - 1;
+
+	ret = dma_set_mask(dev, dma_mask);
+	if (ret < 0) {
+		dev_err(dev, "failed to set DMA mask %pad\n", &dma_mask);
+		dev->dma_mask = NULL;
+		return;
+	}
+
+	dev_dbg(dev, "dma_pfn_offset(%#08lx) dma_mask(%pad)\n",
+		dev->dma_pfn_offset, dev->dma_mask);
+
+	ret = dma_set_coherent_mask(dev, dma_mask);
+	if (ret < 0) {
+		dev_err(dev, "failed to set coherent DMA mask %pad\n",
+			&dma_mask);
+	}
+}
+
+/**
  * of_platform_device_create_pdata - Alloc, initialize and register an of_device
  * @np: pointer to node to create device for
  * @bus_id: name to assign device
@@ -214,9 +285,7 @@ static struct platform_device *of_platform_device_create_pdata(
 #if defined(CONFIG_MICROBLAZE)
 	dev->archdata.dma_mask = 0xffffffffUL;
 #endif
-	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
-	if (!dev->dev.dma_mask)
-		dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
+	dt_dma_configure(&dev->dev);
 	dev->dev.bus = &platform_bus_type;
 	dev->dev.platform_data = platform_data;
 
-- 
1.7.9.5

Re: [PATCH 4/7] of: configure the platform device dma_mask and dma_pfn_offset

[Rob Herring]

On Thu, Mar 6, 2014 at 3:19 AM, Santosh Shilimkar
<santosh.shilimkar-l0cyMroinI0@public.gmane.org> wrote:
> From: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>
>
> Retrieve DMA configuration from DT and setup platform device's DMA
> parameters.
>
> The DMA configuration in DT has to be specified using "dma-ranges"
> property if supported. The DMA configuration applied
> by dt_dma_configure() as following:
>  - call of_get_dma_range() and get supported DMA range info
>    (dma_addr, cpu_addr, dma_size);
>  - if "not found" then fill dma_mask as DMA_BIT_MASK(32)
>    (this is default behaviour);
>  - if "failed" then clean up dma_mask (DMA not supported)
>  - if ok then update devices DMA configuration:
>       set dma_mask to (dma_addr + dma_size - 1)
>       set dma_pfn_offset to PFN_DOWN(cpu_addr - dma_addr)
>
> Cc: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
> Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> Cc: Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@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: Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org>
> Cc: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar-l0cyMroinI0@public.gmane.org>
> ---
>  drivers/of/platform.c |   75 +++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 72 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index bd080db..53bb12f 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -187,6 +187,77 @@ struct platform_device *of_device_alloc(struct device_node *np,
>  EXPORT_SYMBOL(of_device_alloc);
>
>  /**
> + * dt_dma_configure - apply default DMA configuration from dt

s/dt_/of_/

> + * @dev:       Device to apply DMA configuration
> + *
> + * Try to get devices's DMA configuration from DT and apply it.
> + * The DMA configuration is represented in DT by "dma-ranges" property.
> + * It configures:
> + *     dma_pfn_offset, dma_mask and coherent_dma_mask.
> + *
> + * In case if DMA configuration can't be acquired from DT the default one is
> + * applied:
> + *     dma_pfn_offset = 0,
> + *     dma_mask = DMA_BIT_MASK(32)

This is really set to &coherent_dma_mask.

> + *     coherent_dma_mask = DMA_BIT_MASK(32).
> +
> + * In case if platform code need to use own DMA configuration - it can use
> + * Platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE event to fix up
> + * DMA configuration.
> + */
> +static void dt_dma_configure(struct device *dev)

s/dt_/of_/

> +{
> +       dma_addr_t dma_addr;
> +       phys_addr_t paddr, size;
> +       dma_addr_t dma_mask;
> +       int ret;
> +
> +       /*
> +        * if dma-ranges property doesn't exist - use 32 bits DMA mask
> +        * by default and don't set skip archdata.dma_pfn_offset
> +        */
> +       ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size);
> +       if (ret == -ENODEV) {
> +               dev->coherent_dma_mask = DMA_BIT_MASK(32);
> +               if (!dev->dma_mask)
> +                       dev->dma_mask = &dev->coherent_dma_mask;
> +               return;
> +       }
> +
> +       /* if failed - disable DMA for device */
> +       if (ret < 0) {
> +               dev_err(dev, "failed to configure DMA\n");
> +               return;
> +       }
> +
> +       /* DMA ranges found. Calculate and set dma_pfn_offset */
> +       dev->dma_pfn_offset = PFN_DOWN(paddr - dma_addr);
> +
> +       /* Configure DMA mask */
> +       dev->dma_mask = &dev->coherent_dma_mask;
> +       if (!dev->dma_mask)

This condition is impossible.

> +               return;
> +
> +       dma_mask = dma_addr + size - 1;
> +
> +       ret = dma_set_mask(dev, dma_mask);
> +       if (ret < 0) {
> +               dev_err(dev, "failed to set DMA mask %pad\n", &dma_mask);
> +               dev->dma_mask = NULL;
> +               return;
> +       }
> +
> +       dev_dbg(dev, "dma_pfn_offset(%#08lx) dma_mask(%pad)\n",
> +               dev->dma_pfn_offset, dev->dma_mask);
> +
> +       ret = dma_set_coherent_mask(dev, dma_mask);

I think these 2 calls belong in the drivers, not here.

> +       if (ret < 0) {
> +               dev_err(dev, "failed to set coherent DMA mask %pad\n",
> +                       &dma_mask);
> +       }
> +}
> +
> +/**
>   * of_platform_device_create_pdata - Alloc, initialize and register an of_device
>   * @np: pointer to node to create device for
>   * @bus_id: name to assign device
> @@ -214,9 +285,7 @@ static struct platform_device *of_platform_device_create_pdata(
>  #if defined(CONFIG_MICROBLAZE)
>         dev->archdata.dma_mask = 0xffffffffUL;
>  #endif
> -       dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> -       if (!dev->dev.dma_mask)
> -               dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
> +       dt_dma_configure(&dev->dev);
>         dev->dev.bus = &platform_bus_type;
>         dev->dev.platform_data = platform_data;
>
> --
> 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 4/7] of: configure the platform device dma_mask and dma_pfn_offset

[Santosh Shilimkar]

On Friday 07 March 2014 11:49 AM, Rob Herring wrote:
> On Thu, Mar 6, 2014 at 3:19 AM, Santosh Shilimkar
> <santosh.shilimkar-l0cyMroinI0@public.gmane.org> wrote:
>> From: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>
>>
>> Retrieve DMA configuration from DT and setup platform device's DMA
>> parameters.
>>
>> The DMA configuration in DT has to be specified using "dma-ranges"
>> property if supported. The DMA configuration applied
>> by dt_dma_configure() as following:
>>  - call of_get_dma_range() and get supported DMA range info
>>    (dma_addr, cpu_addr, dma_size);
>>  - if "not found" then fill dma_mask as DMA_BIT_MASK(32)
>>    (this is default behaviour);
>>  - if "failed" then clean up dma_mask (DMA not supported)
>>  - if ok then update devices DMA configuration:
>>       set dma_mask to (dma_addr + dma_size - 1)
>>       set dma_pfn_offset to PFN_DOWN(cpu_addr - dma_addr)
>>
>> Cc: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
>> Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
>> Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
>> Cc: Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@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: Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org>
>> Cc: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>
>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar-l0cyMroinI0@public.gmane.org>
>> ---
>>  drivers/of/platform.c |   75 +++++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 72 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index bd080db..53bb12f 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -187,6 +187,77 @@ struct platform_device *of_device_alloc(struct device_node *np,
>>  EXPORT_SYMBOL(of_device_alloc);
>>
>>  /**
>> + * dt_dma_configure - apply default DMA configuration from dt

[..]

>> +
>> +       ret = dma_set_mask(dev, dma_mask);
>> +       if (ret < 0) {
>> +               dev_err(dev, "failed to set DMA mask %pad\n", &dma_mask);
>> +               dev->dma_mask = NULL;
>> +               return;
>> +       }
>> +
>> +       dev_dbg(dev, "dma_pfn_offset(%#08lx) dma_mask(%pad)\n",
>> +               dev->dma_pfn_offset, dev->dma_mask);
>> +
>> +       ret = dma_set_coherent_mask(dev, dma_mask);
> 
> I think these 2 calls belong in the drivers, not here.
> 
I also had same initial thought but Arnd mentioned that its a
shared responsibility between ARCH and drivers. Driver which
could be common between arches not always have the correct
mask information and it can change based on which arch it
is running.

With some discussion back and forth, we thought updating
the dma_mask while the device getting created, would be
better place since we can find the arch capability at
this centralise code and update it.

Ofcourse its bit debatable as the question you asked is
bit obvious as well. I let Arnd give his view here.

Regards,
Santosh


--
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 4/7] of: configure the platform device dma_mask and dma_pfn_offset

[Arnd Bergmann]

On Friday 07 March 2014, Santosh Shilimkar wrote:
> >> +
> >> +       ret = dma_set_mask(dev, dma_mask);
> >> +       if (ret < 0) {
> >> +               dev_err(dev, "failed to set DMA mask %pad\n", &dma_mask);
> >> +               dev->dma_mask = NULL;
> >> +               return;
> >> +       }
> >> +
> >> +       dev_dbg(dev, "dma_pfn_offset(%#08lx) dma_mask(%pad)\n",
> >> +               dev->dma_pfn_offset, dev->dma_mask);
> >> +
> >> +       ret = dma_set_coherent_mask(dev, dma_mask);
> > 
> > I think these 2 calls belong in the drivers, not here.
> > 
> I also had same initial thought but Arnd mentioned that its a
> shared responsibility between ARCH and drivers. Driver which
> could be common between arches not always have the correct
> mask information and it can change based on which arch it
> is running.
> 
> With some discussion back and forth, we thought updating
> the dma_mask while the device getting created, would be
> better place since we can find the arch capability at
> this centralise code and update it.
> 
> Ofcourse its bit debatable as the question you asked is
> bit obvious as well. I let Arnd give his view here.

If we set the mask *here*, we probably don't want to call 'dma_set_mask', but
write to the mask directly, or we could call dma_coerce_mask_and_coherent(),
which is really for overriding the mask pointer and value at once in cases
where you absolutely know what it should be.

We do need to decide what interface we want to use in platform device drivers,
and I'm hoping that Russell has some idea which one he prefers:

a) Follow what we do for PCI devices: assume that we can do DMA_MASK(32)
on any device, and have drivers call dma_set_mask(DMA_MASK(64)) on devices
that would like to do more than that, or call e.g. dma_set_mask(DMA_MASK(28))
for devices that can do less than 32 bit, as given in the argument. This
approach would be most consistent with the way PCI works, but it doesn't
really work well for the case where the mask is less than 32-bit and the
device driver doesn't know that.

b) Never have to call dma_set_mask() for platform devices and assume that the
platform code sets it up correctly. This would probably be the simpler
solution, and I can't think of any downsides at the moment.

In either case we probably want to call something like dt_dma_configure()
from dma_set_mask() again to make sure that we stay within the limits
imposed by the bus structure.

	Arnd

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

Re: [PATCH 4/7] of: configure the platform device dma_mask and dma_pfn_offset

[Rob Herring]

On Fri, Mar 7, 2014 at 10:02 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Friday 07 March 2014, Santosh Shilimkar wrote:
>> >> +
>> >> +       ret = dma_set_mask(dev, dma_mask);
>> >> +       if (ret < 0) {
>> >> +               dev_err(dev, "failed to set DMA mask %pad\n", &dma_mask);
>> >> +               dev->dma_mask = NULL;
>> >> +               return;
>> >> +       }
>> >> +
>> >> +       dev_dbg(dev, "dma_pfn_offset(%#08lx) dma_mask(%pad)\n",
>> >> +               dev->dma_pfn_offset, dev->dma_mask);
>> >> +
>> >> +       ret = dma_set_coherent_mask(dev, dma_mask);
>> >
>> > I think these 2 calls belong in the drivers, not here.
>> >
>> I also had same initial thought but Arnd mentioned that its a
>> shared responsibility between ARCH and drivers. Driver which
>> could be common between arches not always have the correct
>> mask information and it can change based on which arch it
>> is running.
>>
>> With some discussion back and forth, we thought updating
>> the dma_mask while the device getting created, would be
>> better place since we can find the arch capability at
>> this centralise code and update it.
>>
>> Ofcourse its bit debatable as the question you asked is
>> bit obvious as well. I let Arnd give his view here.
>
> If we set the mask *here*, we probably don't want to call 'dma_set_mask', but
> write to the mask directly, or we could call dma_coerce_mask_and_coherent(),
> which is really for overriding the mask pointer and value at once in cases
> where you absolutely know what it should be.
>
> We do need to decide what interface we want to use in platform device drivers,
> and I'm hoping that Russell has some idea which one he prefers:
>
> a) Follow what we do for PCI devices: assume that we can do DMA_MASK(32)
> on any device, and have drivers call dma_set_mask(DMA_MASK(64)) on devices
> that would like to do more than that, or call e.g. dma_set_mask(DMA_MASK(28))
> for devices that can do less than 32 bit, as given in the argument. This
> approach would be most consistent with the way PCI works, but it doesn't
> really work well for the case where the mask is less than 32-bit and the
> device driver doesn't know that.
>
> b) Never have to call dma_set_mask() for platform devices and assume that the
> platform code sets it up correctly. This would probably be the simpler
> solution, and I can't think of any downsides at the moment.

I don't think we want this. In the case of setting up 64-bit masters,
it is typically the device that knows if it can do 64-bit DMA either
thru a capabilities register or compatible value. That device specific
knowledge should really stay within the device's driver.

Rob

>
> In either case we probably want to call something like dt_dma_configure()
> from dma_set_mask() again to make sure that we stay within the limits
> imposed by the bus structure.
>
>         Arnd
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] of: configure the platform device dma_mask and dma_pfn_offset

[Arnd Bergmann]

On Saturday 08 March 2014, Rob Herring wrote:
> On Fri, Mar 7, 2014 at 10:02 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:

> > a) Follow what we do for PCI devices: assume that we can do DMA_MASK(32)
> > on any device, and have drivers call dma_set_mask(DMA_MASK(64)) on devices
> > that would like to do more than that, or call e.g. dma_set_mask(DMA_MASK(28))
> > for devices that can do less than 32 bit, as given in the argument. This
> > approach would be most consistent with the way PCI works, but it doesn't
> > really work well for the case where the mask is less than 32-bit and the
> > device driver doesn't know that.
> >
> > b) Never have to call dma_set_mask() for platform devices and assume that the
> > platform code sets it up correctly. This would probably be the simpler
> > solution, and I can't think of any downsides at the moment.
> 
> I don't think we want this. In the case of setting up 64-bit masters,
> it is typically the device that knows if it can do 64-bit DMA either
> thru a capabilities register or compatible value. That device specific
> knowledge should really stay within the device's driver.

So you think we should still set a 64-bit mask in the "ranges" property
for devices that can only do 32-bit and let the driver figure it out?

I think this approach is much less useful for platform devices than it is
for PCI devices, where we don't explicitly describe the "ranges" for each
device. Are you thinking of off-chip or on-chip DMA masters here? If
on-chip, I don't think it's likely that we would end up with different
versions of the chip that have the same device on there but not the
same DMA capabilities.

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

Re: [PATCH 4/7] of: configure the platform device dma_mask and dma_pfn_offset

[Rob Herring]

On Sun, Mar 9, 2014 at 12:39 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Saturday 08 March 2014, Rob Herring wrote:
>> On Fri, Mar 7, 2014 at 10:02 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>
>> > a) Follow what we do for PCI devices: assume that we can do DMA_MASK(32)
>> > on any device, and have drivers call dma_set_mask(DMA_MASK(64)) on devices
>> > that would like to do more than that, or call e.g. dma_set_mask(DMA_MASK(28))
>> > for devices that can do less than 32 bit, as given in the argument. This
>> > approach would be most consistent with the way PCI works, but it doesn't
>> > really work well for the case where the mask is less than 32-bit and the
>> > device driver doesn't know that.
>> >
>> > b) Never have to call dma_set_mask() for platform devices and assume that the
>> > platform code sets it up correctly. This would probably be the simpler
>> > solution, and I can't think of any downsides at the moment.
>>
>> I don't think we want this. In the case of setting up 64-bit masters,
>> it is typically the device that knows if it can do 64-bit DMA either
>> thru a capabilities register or compatible value. That device specific
>> knowledge should really stay within the device's driver.
>
> So you think we should still set a 64-bit mask in the "ranges" property
> for devices that can only do 32-bit and let the driver figure it out?

No, I think option a makes more sense. BTW, don't you mean dma-ranges?
A device has it's own mask which is typically going to be 32 or
64-bit. The core code may not know what the device supports, so the
device has to set it's own mask. I don't see a way around that. DT is
not a good option because it is discoverable in some cases.

Isn't the question here how do we handle restrictions added by the
bus? It seems while this series adds support for handling dma-ranges
to set the default case, it also needs to handle when the driver sets
the mask. Is this not simply a matter of having dma_set_mask also
parse dma-ranges (or reuse a saved bus mask)?

> I think this approach is much less useful for platform devices than it is
> for PCI devices, where we don't explicitly describe the "ranges" for each
> device. Are you thinking of off-chip or on-chip DMA masters here? If
> on-chip, I don't think it's likely that we would end up with different
> versions of the chip that have the same device on there but not the
> same DMA capabilities.

Sounds like a challenge to h/w designers. :)

I'm mainly thinking about the midway case where all masters are 32-bit
except AHCI which is 64-bit. The core libahci sets the mask based on
the capabilities register regardless of PCI or platform bus. Requiring
this to be setup by the platform or in the DT seems like a step
backwards. A slightly more complicated case would be something like
midway, but with RAM starting at say 2GB and DMA masters have the same
view as the cpu (i.e. no offset). Then the platform does need to set
the mask to (2^31 -1).

Rob
--
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 4/7] of: configure the platform device dma_mask and dma_pfn_offset

[Grygorii Strashko]

Hi Rob, Arnd,

On 03/12/2014 02:15 AM, Rob Herring wrote:
> On Sun, Mar 9, 2014 at 12:39 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>> On Saturday 08 March 2014, Rob Herring wrote:
>>> On Fri, Mar 7, 2014 at 10:02 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>>
>>>> a) Follow what we do for PCI devices: assume that we can do DMA_MASK(32)
>>>> on any device, and have drivers call dma_set_mask(DMA_MASK(64)) on devices
>>>> that would like to do more than that, or call e.g. dma_set_mask(DMA_MASK(28))
>>>> for devices that can do less than 32 bit, as given in the argument. This
>>>> approach would be most consistent with the way PCI works, but it doesn't
>>>> really work well for the case where the mask is less than 32-bit and the
>>>> device driver doesn't know that.
>>>>
>>>> b) Never have to call dma_set_mask() for platform devices and assume that the
>>>> platform code sets it up correctly. This would probably be the simpler
>>>> solution, and I can't think of any downsides at the moment.
>>>
>>> I don't think we want this. In the case of setting up 64-bit masters,
>>> it is typically the device that knows if it can do 64-bit DMA either
>>> thru a capabilities register or compatible value. That device specific
>>> knowledge should really stay within the device's driver.
>>
>> So you think we should still set a 64-bit mask in the "ranges" property
>> for devices that can only do 32-bit and let the driver figure it out?
> 
> No, I think option a makes more sense. BTW, don't you mean dma-ranges?
> A device has it's own mask which is typically going to be 32 or
> 64-bit. The core code may not know what the device supports, so the
> device has to set it's own mask. I don't see a way around that. DT is
> not a good option because it is discoverable in some cases.
> 
> Isn't the question here how do we handle restrictions added by the
> bus? It seems while this series adds support for handling dma-ranges
> to set the default case, it also needs to handle when the driver sets
> the mask. Is this not simply a matter of having dma_set_mask also
> parse dma-ranges (or reuse a saved bus mask)?
> 
>> I think this approach is much less useful for platform devices than it is
>> for PCI devices, where we don't explicitly describe the "ranges" for each
>> device. Are you thinking of off-chip or on-chip DMA masters here? If
>> on-chip, I don't think it's likely that we would end up with different
>> versions of the chip that have the same device on there but not the
>> same DMA capabilities.
> 
> Sounds like a challenge to h/w designers. :)
> 
> I'm mainly thinking about the midway case where all masters are 32-bit
> except AHCI which is 64-bit. The core libahci sets the mask based on
> the capabilities register regardless of PCI or platform bus. Requiring
> this to be setup by the platform or in the DT seems like a step
> backwards. A slightly more complicated case would be something like
> midway, but with RAM starting at say 2GB and DMA masters have the same
> view as the cpu (i.e. no offset). Then the platform does need to set
> the mask to (2^31 -1).
> 

mask has to be (2^32 -1).

I'd like to add my 5 cents here :)
First of all this solution is pretended to be generic, so by default it
can't handle all possible cases (corner cases) - only widely used. 
Arch or driver can always fix up it through bus notifier or driver's probe.

Second, it works only for DT-boot case when devices instantiated from DT in generic way.
So, if any bus creates devices in its own way - proper DMA configuration of such
devices will be private problem of this bus.

I've tried to formulate possible use cases which can be and can't be covered
by this approach:
[1] 32 bit SoC:
- no DMA range limitation: DMA range has to be absent or be equal to RAM
  (default DMA configuration will be applied)

- DMA range present and It's defined inside RAM (no address translation needed):
  DMA range can be present and depending on its size
  dma_mask will be equal or less than DMA_MASK(32).
  (default DMA configuration will be applied - if DMA range isn't present)

- DMA range present and It's defined outside of RAM (address translation needed):
  DMA range has to be present and depending on its size
  dma_mask will be equal or less than DMA_MASK(32); and dma_pfn_offset will be calculated.

Compatibility issues:
  - Old DT without DMA properties defined and DMA range is defined outside of RAM:
  arch or drivers code has to provide proper address translation routines.

[2] 64 bit SOC (only 64 bit masters):
- no DMA range limitation: DMA range has to be present and equal to RAM

- DMA range present and It's defined inside RAM (no address translation needed):
  DMA range has to be present and depending on its size
  dma_mask will be equal or less than DMA_MASK(64).

- DMA range present and It's defined outside of RAM (address translation needed):
  DMA range has to be present and depending on its size
  dma_mask will be equal or less than DMA_MASK(64); and dma_pfn_offset will be calculated.

Compatibility issues:
  - Old DT without DMA properties defined: Drivers may still need to call dma_set_mask(DMA_MASK(64)
  - Old DT without DMA properties defined and DMA range is defined outside of RAM:
  arch or drivers code still has to provide proper address translation routines.

[3] 64 bit SOC (only 32 bit masters):

- DMA range is present and It's defined inside RAM (no address translation needed):
  DMA range can be absent - default DMA configuration will be applied.

- DMA range present and It's defined outside of RAM (address translation needed):
  DMA range has to be present and depending on its size
  dma_mask will be equal or less than DMA_MASK(32); and dma_pfn_offset will be calculated.

Compatibility issues:
  - Old DT without DMA properties defined and DMA range is defined outside of RAM:
  arch or drivers code has to provide proper address translation routines.


[4] 64 bit SOC (32 bit and 64 masters):
- This is combination of above 2,3 cases.
  The current approach will allow to handle it by defining two buses in DT
  "simple-bus32" and "simple-bus64", and each one should have its own DMA properties
  defined in DT.

- The worst case will be if some device is able to change its DMA configuration
  dynamically by reading its DMA configuration from HW. The main difficulties will
  be to handle DMA addresses translation properly in this case. But, in my opinion,
  such behavior will be needed in very rare case as DT has to describe specific SoC
  and such device has to belong to only one bus as per SoC specification.

Compatibility issues:
  - Old DT without DMA properties defined and 32 bits DMA range is defined outside of RAM:
  arch or drivers code has to provide proper address translation routines.

As per above, this approach will allow to handle most of cases and configure DMA properly
with one exception - device is able to change its DMA configuration dynamically.

Also, for compatibility reasons, arches and drivers may need to continue maintain their own
DMA addresses translation routines to support the case
"Old DT without DMA properties defined and DMA range is defined outside of RAM".

Regards,
- grygorii

--
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 4/7] of: configure the platform device dma_mask and dma_pfn_offset

[Arnd Bergmann]

On Wednesday 12 March 2014 15:19:48 Grygorii Strashko wrote:
> On 03/12/2014 02:15 AM, Rob Herring wrote:
> > On Sun, Mar 9, 2014 at 12:39 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> >> On Saturday 08 March 2014, Rob Herring wrote:
> >>> On Fri, Mar 7, 2014 at 10:02 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> >>
> >>>> a) Follow what we do for PCI devices: assume that we can do DMA_MASK(32)
> >>>> on any device, and have drivers call dma_set_mask(DMA_MASK(64)) on devices
> >>>> that would like to do more than that, or call e.g. dma_set_mask(DMA_MASK(28))
> >>>> for devices that can do less than 32 bit, as given in the argument. This
> >>>> approach would be most consistent with the way PCI works, but it doesn't
> >>>> really work well for the case where the mask is less than 32-bit and the
> >>>> device driver doesn't know that.
> >>>>
> >>>> b) Never have to call dma_set_mask() for platform devices and assume that the
> >>>> platform code sets it up correctly. This would probably be the simpler
> >>>> solution, and I can't think of any downsides at the moment.
> >>>
> >>> I don't think we want this. In the case of setting up 64-bit masters,
> >>> it is typically the device that knows if it can do 64-bit DMA either
> >>> thru a capabilities register or compatible value. That device specific
> >>> knowledge should really stay within the device's driver.
> >>
> >> So you think we should still set a 64-bit mask in the "ranges" property
> >> for devices that can only do 32-bit and let the driver figure it out?
> > 
> > No, I think option a makes more sense. 

Ok.

> > BTW, don't you mean dma-ranges?

Yes, sorry about the confusion.

> > A device has it's own mask which is typically going to be 32 or
> > 64-bit. The core code may not know what the device supports, so the
> > device has to set it's own mask. I don't see a way around that. DT is
> > not a good option because it is discoverable in some cases.

What is "it"? Do you mean the mask of the device is discoverable,
or something else?

> > Isn't the question here how do we handle restrictions added by the
> > bus? It seems while this series adds support for handling dma-ranges
> > to set the default case, it also needs to handle when the driver sets
> > the mask. Is this not simply a matter of having dma_set_mask also
> > parse dma-ranges (or reuse a saved bus mask)?

With the current proposed implementation, the device also has to set
a "dma-ranges" property to get any DMA support, we don't just take
the mask of the parent bus. This is important because the translation
does not have to be 1:1 but there can be an offset between the device
and the parent. I'd prefer to leave this explicit.

> >> I think this approach is much less useful for platform devices than it is
> >> for PCI devices, where we don't explicitly describe the "ranges" for each
> >> device. Are you thinking of off-chip or on-chip DMA masters here? If
> >> on-chip, I don't think it's likely that we would end up with different
> >> versions of the chip that have the same device on there but not the
> >> same DMA capabilities.
> > 
> > Sounds like a challenge to h/w designers. :)
> > 
> > I'm mainly thinking about the midway case where all masters are 32-bit
> > except AHCI which is 64-bit. The core libahci sets the mask based on
> > the capabilities register regardless of PCI or platform bus. Requiring
> > this to be setup by the platform or in the DT seems like a step
> > backwards. A slightly more complicated case would be something like
> > midway, but with RAM starting at say 2GB and DMA masters have the same
> > view as the cpu (i.e. no offset). Then the platform does need to set
> > the mask to (2^31 -1).

So how would you show this in DT? I'd assume that some of the other
devices on midway have drivers that may also try to set a 64-bit mask,
like EHCI for instance does. Is AHCI the only one that sits on a 64-bit
wide bus, like this?

	axi {
		#address-cells = <2>;
		#size-cells = <2>;
		dma-ranges = <0 0 0 0 0x100 0>; /* max phys address space */

		ahci {
			dma-ranges = <0 0  0 0  0x100 0>;
			...
		};

		ahb {
			#address-cells = <1>;
			#size-cells = <1>;
			dma-ranges = <0 0  0  0x1 0>; /* only 4GB */

			ehci {
				dma-ranges = <0  0  0xffffffff>;
				/* side-question: is that the right way
				  to describe 4GB length? it seems missing
				  a byte */
			};
		};
	};

BTW, I hope I understood you right that you wouldn't actually trust the
capabilities register by itself but only allow setting the 64-bit mask
in the arch code if the DT doesn't have any information about the
bus being incapable of doing this, right?

> mask has to be (2^32 -1).

Right.
 
> I'd like to add my 5 cents here :)
> First of all this solution is pretended to be generic, so by default it
> can't handle all possible cases (corner cases) - only widely used. 
> Arch or driver can always fix up it through bus notifier or driver's probe.

Right, but we should strive to avoid that.

> Second, it works only for DT-boot case when devices instantiated from DT in generic way.
> So, if any bus creates devices in its own way - proper DMA configuration of such
> devices will be private problem of this bus.

True as well. Do you see any buses besides platform, amba and PCI that need
to describe bus masters?

> I've tried to formulate possible use cases which can be and can't be covered
> by this approach:
> [1] 32 bit SoC:
> - no DMA range limitation: DMA range has to be absent or be equal to RAM
>   (default DMA configuration will be applied)
> 
> - DMA range present and It's defined inside RAM (no address translation needed):
>   DMA range can be present and depending on its size
>   dma_mask will be equal or less than DMA_MASK(32).
>   (default DMA configuration will be applied - if DMA range isn't present)
> 
> - DMA range present and It's defined outside of RAM (address translation needed):
>   DMA range has to be present and depending on its size
>   dma_mask will be equal or less than DMA_MASK(32); and dma_pfn_offset will be calculated.
> 
> Compatibility issues:
>   - Old DT without DMA properties defined and DMA range is defined outside of RAM:
>   arch or drivers code has to provide proper address translation routines.

Can you clarify what you mean by "outside of RAM"? Is this the IOMMU case, no-IOMMU
but offset, or both?

> [2] 64 bit SOC (only 64 bit masters):
> - no DMA range limitation: DMA range has to be present and equal to RAM
> 
> - DMA range present and It's defined inside RAM (no address translation needed):
>   DMA range has to be present and depending on its size
>   dma_mask will be equal or less than DMA_MASK(64).
> 
> - DMA range present and It's defined outside of RAM (address translation needed):
>   DMA range has to be present and depending on its size
>   dma_mask will be equal or less than DMA_MASK(64); and dma_pfn_offset will be calculated.
> 
> Compatibility issues:
>   - Old DT without DMA properties defined: Drivers may still need to call dma_set_mask(DMA_MASK(64)
>   - Old DT without DMA properties defined and DMA range is defined outside of RAM:
>   arch or drivers code still has to provide proper address translation routines.

64-bit SOC includes 32-bit CPU with LPAE, right?

Would you want to allow dma_set_mask(DMA_MASK(64)) to succeed in the absence
of the dma-ranges properties?

> [3] 64 bit SOC (only 32 bit masters):
> 
> - DMA range is present and It's defined inside RAM (no address translation needed):
>   DMA range can be absent - default DMA configuration will be applied.
> 
> - DMA range present and It's defined outside of RAM (address translation needed):
>   DMA range has to be present and depending on its size
>   dma_mask will be equal or less than DMA_MASK(32); and dma_pfn_offset will be calculated.
> 
> Compatibility issues:
>   - Old DT without DMA properties defined and DMA range is defined outside of RAM:
>   arch or drivers code has to provide proper address translation routines.
> 
> 
> [4] 64 bit SOC (32 bit and 64 masters):
> - This is combination of above 2,3 cases.
>   The current approach will allow to handle it by defining two buses in DT
>   "simple-bus32" and "simple-bus64", and each one should have its own DMA properties
>   defined in DT.

We already try to describe the hierarchy of the buses, and only AXI4 buses can be
64-bit, unlike AHB or other types of AMBA buses AFAIK. We should just call them
what they are.

> - The worst case will be if some device is able to change its DMA configuration
>   dynamically by reading its DMA configuration from HW. The main difficulties will
>   be to handle DMA addresses translation properly in this case. But, in my opinion,
>   such behavior will be needed in very rare case as DT has to describe specific SoC
>   and such device has to belong to only one bus as per SoC specification.
> 
> Compatibility issues:
>   - Old DT without DMA properties defined and 32 bits DMA range is defined outside of RAM:
>   arch or drivers code has to provide proper address translation routines.
> 
> As per above, this approach will allow to handle most of cases and configure DMA properly
> with one exception - device is able to change its DMA configuration dynamically.
> 
> Also, for compatibility reasons, arches and drivers may need to continue maintain their own
> DMA addresses translation routines to support the case
> "Old DT without DMA properties defined and DMA range is defined outside of RAM".

Can we try to list all the platforms we already support in Linux? There should not be
a lot of examples of 64-bit SoCs, as we don't really support any /hardware/ in arm64
properly (apm xgene is incomplete enough that we don't have to worry about breaking it).

The ones I see are

midway
armada xp
keystone 2
r-car

It may be easier to see what their individual requirements are for backwards
compatibility. Some might never have shipped with more than 4GB, and in that
case we don't care at all. I believe exynos5 and tc2 are in that category,
so I didn't even list them.

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

Re: [PATCH 4/7] of: configure the platform device dma_mask and dma_pfn_offset

[Rob Herring]

On Wed, Mar 12, 2014 at 11:58 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Wednesday 12 March 2014 15:19:48 Grygorii Strashko wrote:
>> On 03/12/2014 02:15 AM, Rob Herring wrote:
>> > On Sun, Mar 9, 2014 at 12:39 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>> >> On Saturday 08 March 2014, Rob Herring wrote:
>> >>> On Fri, Mar 7, 2014 at 10:02 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>> >>
>> >>>> a) Follow what we do for PCI devices: assume that we can do DMA_MASK(32)
>> >>>> on any device, and have drivers call dma_set_mask(DMA_MASK(64)) on devices
>> >>>> that would like to do more than that, or call e.g. dma_set_mask(DMA_MASK(28))
>> >>>> for devices that can do less than 32 bit, as given in the argument. This
>> >>>> approach would be most consistent with the way PCI works, but it doesn't
>> >>>> really work well for the case where the mask is less than 32-bit and the
>> >>>> device driver doesn't know that.
>> >>>>
>> >>>> b) Never have to call dma_set_mask() for platform devices and assume that the
>> >>>> platform code sets it up correctly. This would probably be the simpler
>> >>>> solution, and I can't think of any downsides at the moment.
>> >>>
>> >>> I don't think we want this. In the case of setting up 64-bit masters,
>> >>> it is typically the device that knows if it can do 64-bit DMA either
>> >>> thru a capabilities register or compatible value. That device specific
>> >>> knowledge should really stay within the device's driver.
>> >>
>> >> So you think we should still set a 64-bit mask in the "ranges" property
>> >> for devices that can only do 32-bit and let the driver figure it out?
>> >
>> > No, I think option a makes more sense.
>
> Ok.
>
>> > BTW, don't you mean dma-ranges?
>
> Yes, sorry about the confusion.
>
>> > A device has it's own mask which is typically going to be 32 or
>> > 64-bit. The core code may not know what the device supports, so the
>> > device has to set it's own mask. I don't see a way around that. DT is
>> > not a good option because it is discoverable in some cases.
>
> What is "it"? Do you mean the mask of the device is discoverable,
> or something else?

Right, the mask of the device (before applying any restrictions of the
bus) is discoverable.

>> > Isn't the question here how do we handle restrictions added by the
>> > bus? It seems while this series adds support for handling dma-ranges
>> > to set the default case, it also needs to handle when the driver sets
>> > the mask. Is this not simply a matter of having dma_set_mask also
>> > parse dma-ranges (or reuse a saved bus mask)?
>
> With the current proposed implementation, the device also has to set
> a "dma-ranges" property to get any DMA support, we don't just take
> the mask of the parent bus. This is important because the translation
> does not have to be 1:1 but there can be an offset between the device
> and the parent. I'd prefer to leave this explicit.

But according to Ben H, dma-ranges is a bridge (i.e. parent) property
and not a device property[1]. So I don't think the device's mask
(again, before any bus restrictions) belongs in DT. A given IP block
is going to have some number of address bits for DMA. How it is hooked
up into an SOC is a separate issue. The driver should know its mask
whether it is fixed, discoverable, or tied to the compatible string.

Santosh's patch only looks for dma-ranges in parent nodes which I
believe is correct.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2012-March/088881.html

>> >> I think this approach is much less useful for platform devices than it is
>> >> for PCI devices, where we don't explicitly describe the "ranges" for each
>> >> device. Are you thinking of off-chip or on-chip DMA masters here? If
>> >> on-chip, I don't think it's likely that we would end up with different
>> >> versions of the chip that have the same device on there but not the
>> >> same DMA capabilities.
>> >
>> > Sounds like a challenge to h/w designers. :)
>> >
>> > I'm mainly thinking about the midway case where all masters are 32-bit
>> > except AHCI which is 64-bit. The core libahci sets the mask based on
>> > the capabilities register regardless of PCI or platform bus. Requiring
>> > this to be setup by the platform or in the DT seems like a step
>> > backwards. A slightly more complicated case would be something like
>> > midway, but with RAM starting at say 2GB and DMA masters have the same
>> > view as the cpu (i.e. no offset). Then the platform does need to set
>> > the mask to (2^31 -1).
>
> So how would you show this in DT? I'd assume that some of the other
> devices on midway have drivers that may also try to set a 64-bit mask,
> like EHCI for instance does. Is AHCI the only one that sits on a 64-bit
> wide bus, like this?
>
>         axi {
>                 #address-cells = <2>;
>                 #size-cells = <2>;
>                 dma-ranges = <0 0 0 0 0x100 0>; /* max phys address space */
>
>                 ahci {
>                         dma-ranges = <0 0  0 0  0x100 0>;

There is no point to this dma-ranges here. Based on the capabilities
reg, we know that we can do either 32 or 64 bit DMA. In the case of
64-bit DMA, the device should try to set its mask to DMA_MASK(64), but
the parent dma-ranges should restrict that to DMA_MASK(40).

>                         ...
>                 };
>
>                 ahb {
>                         #address-cells = <1>;
>                         #size-cells = <1>;

This would need to be 2. ePAPR says the size cells size is determined
by #size-cells in the same node.

>                         dma-ranges = <0 0  0  0x1 0>; /* only 4GB */
>
>                         ehci {
>                                 dma-ranges = <0  0  0xffffffff>;

Again, I don't think this is needed. Here, regardless of the device's
capabilities, the bus is restricting the mask to DMA_MASK(32).

>                                 /* side-question: is that the right way
>                                   to describe 4GB length? it seems missing
>                                   a byte */
>                         };
>                 };
>         };
>
> BTW, I hope I understood you right that you wouldn't actually trust the
> capabilities register by itself but only allow setting the 64-bit mask
> in the arch code if the DT doesn't have any information about the
> bus being incapable of doing this, right?

Ideally no, but it appears we are ATM for midway. We get away with it
since it is midway/highbank specific driver setup and know there are
not any restrictions in addressing RAM. I think we have to keep bus
and device masks separate and the final mask is the AND of them. There
isn't really a construct to do this currently AFAICT. dma_set_mask
could be modified to adjust the mask, but that would be a change in
how dma_set_mask works as it is supposed to fail if the requested mask
cannot be supported.

Perhaps using dma_get_required_mask to parse dma-ranges would be
appropriate here? It doesn't seem widely used though.

>
>> mask has to be (2^32 -1).
>
> Right.
>
>> I'd like to add my 5 cents here :)
>> First of all this solution is pretended to be generic, so by default it
>> can't handle all possible cases (corner cases) - only widely used.
>> Arch or driver can always fix up it through bus notifier or driver's probe.
>
> Right, but we should strive to avoid that.
>
>> Second, it works only for DT-boot case when devices instantiated from DT in generic way.
>> So, if any bus creates devices in its own way - proper DMA configuration of such
>> devices will be private problem of this bus.
>
> True as well. Do you see any buses besides platform, amba and PCI that need
> to describe bus masters?
>
>> I've tried to formulate possible use cases which can be and can't be covered
>> by this approach:
>> [1] 32 bit SoC:
>> - no DMA range limitation: DMA range has to be absent or be equal to RAM
>>   (default DMA configuration will be applied)
>>
>> - DMA range present and It's defined inside RAM (no address translation needed):
>>   DMA range can be present and depending on its size
>>   dma_mask will be equal or less than DMA_MASK(32).
>>   (default DMA configuration will be applied - if DMA range isn't present)
>>
>> - DMA range present and It's defined outside of RAM (address translation needed):
>>   DMA range has to be present and depending on its size
>>   dma_mask will be equal or less than DMA_MASK(32); and dma_pfn_offset will be calculated.
>>
>> Compatibility issues:
>>   - Old DT without DMA properties defined and DMA range is defined outside of RAM:
>>   arch or drivers code has to provide proper address translation routines.
>
> Can you clarify what you mean by "outside of RAM"? Is this the IOMMU case, no-IOMMU
> but offset, or both?
>
>> [2] 64 bit SOC (only 64 bit masters):
>> - no DMA range limitation: DMA range has to be present and equal to RAM
>>
>> - DMA range present and It's defined inside RAM (no address translation needed):
>>   DMA range has to be present and depending on its size
>>   dma_mask will be equal or less than DMA_MASK(64).
>>
>> - DMA range present and It's defined outside of RAM (address translation needed):
>>   DMA range has to be present and depending on its size
>>   dma_mask will be equal or less than DMA_MASK(64); and dma_pfn_offset will be calculated.
>>
>> Compatibility issues:
>>   - Old DT without DMA properties defined: Drivers may still need to call dma_set_mask(DMA_MASK(64)
>>   - Old DT without DMA properties defined and DMA range is defined outside of RAM:
>>   arch or drivers code still has to provide proper address translation routines.
>
> 64-bit SOC includes 32-bit CPU with LPAE, right?
>
> Would you want to allow dma_set_mask(DMA_MASK(64)) to succeed in the absence
> of the dma-ranges properties?

Yes, because the default should be no restrictions are imposed by the
bus. DT should describe the restrictions or translations. The default
should be masters have the same view of memory map as the cpu and can
use all address bits the device has.

>
>> [3] 64 bit SOC (only 32 bit masters):
>>
>> - DMA range is present and It's defined inside RAM (no address translation needed):
>>   DMA range can be absent - default DMA configuration will be applied.
>>
>> - DMA range present and It's defined outside of RAM (address translation needed):
>>   DMA range has to be present and depending on its size
>>   dma_mask will be equal or less than DMA_MASK(32); and dma_pfn_offset will be calculated.
>>
>> Compatibility issues:
>>   - Old DT without DMA properties defined and DMA range is defined outside of RAM:
>>   arch or drivers code has to provide proper address translation routines.
>>
>>
>> [4] 64 bit SOC (32 bit and 64 masters):
>> - This is combination of above 2,3 cases.
>>   The current approach will allow to handle it by defining two buses in DT
>>   "simple-bus32" and "simple-bus64", and each one should have its own DMA properties
>>   defined in DT.
>
> We already try to describe the hierarchy of the buses, and only AXI4 buses can be
> 64-bit, unlike AHB or other types of AMBA buses AFAIK. We should just call them
> what they are.

I don't think we do describe the hierarchy. We are describing the
slave side which is not necessarily the same as the master side.
Internal buses are also much more complex than any of the SOCs
describe in their DT.

>
>> - The worst case will be if some device is able to change its DMA configuration
>>   dynamically by reading its DMA configuration from HW. The main difficulties will
>>   be to handle DMA addresses translation properly in this case. But, in my opinion,
>>   such behavior will be needed in very rare case as DT has to describe specific SoC
>>   and such device has to belong to only one bus as per SoC specification.
>>
>> Compatibility issues:
>>   - Old DT without DMA properties defined and 32 bits DMA range is defined outside of RAM:
>>   arch or drivers code has to provide proper address translation routines.
>>
>> As per above, this approach will allow to handle most of cases and configure DMA properly
>> with one exception - device is able to change its DMA configuration dynamically.
>>
>> Also, for compatibility reasons, arches and drivers may need to continue maintain their own
>> DMA addresses translation routines to support the case
>> "Old DT without DMA properties defined and DMA range is defined outside of RAM".
>
> Can we try to list all the platforms we already support in Linux? There should not be
> a lot of examples of 64-bit SoCs, as we don't really support any /hardware/ in arm64
> properly (apm xgene is incomplete enough that we don't have to worry about breaking it).
>
> The ones I see are
>
> midway
> armada xp
> keystone 2
> r-car
>
> It may be easier to see what their individual requirements are for backwards
> compatibility. Some might never have shipped with more than 4GB, and in that
> case we don't care at all. I believe exynos5 and tc2 are in that category,
> so I didn't even list them.
>
>         Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] of: configure the platform device dma_mask and dma_pfn_offset

[Arnd Bergmann]

On Friday 14 March 2014, Rob Herring wrote:
> On Wed, Mar 12, 2014 at 11:58 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> > On Wednesday 12 March 2014 15:19:48 Grygorii Strashko wrote:
> >> > Isn't the question here how do we handle restrictions added by the
> >> > bus? It seems while this series adds support for handling dma-ranges
> >> > to set the default case, it also needs to handle when the driver sets
> >> > the mask. Is this not simply a matter of having dma_set_mask also
> >> > parse dma-ranges (or reuse a saved bus mask)?
> >
> > With the current proposed implementation, the device also has to set
> > a "dma-ranges" property to get any DMA support, we don't just take
> > the mask of the parent bus. This is important because the translation
> > does not have to be 1:1 but there can be an offset between the device
> > and the parent. I'd prefer to leave this explicit.
> 
> But according to Ben H, dma-ranges is a bridge (i.e. parent) property
> and not a device property[1]. So I don't think the device's mask
> (again, before any bus restrictions) belongs in DT. A given IP block
> is going to have some number of address bits for DMA. How it is hooked
> up into an SOC is a separate issue. The driver should know its mask
> whether it is fixed, discoverable, or tied to the compatible string.
> 
> Santosh's patch only looks for dma-ranges in parent nodes which I
> believe is correct.

Ok, good point.

> >> >> I think this approach is much less useful for platform devices than it is
> >> >> for PCI devices, where we don't explicitly describe the "ranges" for each
> >> >> device. Are you thinking of off-chip or on-chip DMA masters here? If
> >> >> on-chip, I don't think it's likely that we would end up with different
> >> >> versions of the chip that have the same device on there but not the
> >> >> same DMA capabilities.
> >> >
> >> > Sounds like a challenge to h/w designers. :)
> >> >
> >> > I'm mainly thinking about the midway case where all masters are 32-bit
> >> > except AHCI which is 64-bit. The core libahci sets the mask based on
> >> > the capabilities register regardless of PCI or platform bus. Requiring
> >> > this to be setup by the platform or in the DT seems like a step
> >> > backwards. A slightly more complicated case would be something like
> >> > midway, but with RAM starting at say 2GB and DMA masters have the same
> >> > view as the cpu (i.e. no offset). Then the platform does need to set
> >> > the mask to (2^31 -1).
> >
> > So how would you show this in DT? I'd assume that some of the other
> > devices on midway have drivers that may also try to set a 64-bit mask,
> > like EHCI for instance does. Is AHCI the only one that sits on a 64-bit
> > wide bus, like this?
> >
> >         axi {
> >                 #address-cells = <2>;
> >                 #size-cells = <2>;
> >                 dma-ranges = <0 0 0 0 0x100 0>; /* max phys address space */
> >
> >                 ahci {
> >                         dma-ranges = <0 0  0 0  0x100 0>;
> 
> There is no point to this dma-ranges here. Based on the capabilities
> reg, we know that we can do either 32 or 64 bit DMA.

Ok

> In the case of
> 64-bit DMA, the device should try to set its mask to DMA_MASK(64), but
> the parent dma-ranges should restrict that to DMA_MASK(40).

Now I'm confused about what dma_set_mask is actually supposed to do here.
I /think/ it should fail the DMA_MASK(64) call if the bus supports
less than 64 bits as in the above example. However it seems like a
valid shortcut to always succeed here if the effective mask covers
all of the installed RAM.

That would mean that even if we only have a 32-bit bus, but all RAM
resides below 4GB, a call to dma_set_mask using DMA_MASK(64) would
also succeed.

> >                         ...
> >                 };
> >
> >                 ahb {
> >                         #address-cells = <1>;
> >                         #size-cells = <1>;
> 
> This would need to be 2. ePAPR says the size cells size is determined
> by #size-cells in the same node.

Ah, of course.

> >                         dma-ranges = <0 0  0  0x1 0>; /* only 4GB */
> >
> >                         ehci {
> >                                 dma-ranges = <0  0  0xffffffff>;
> 
> Again, I don't think this is needed. Here, regardless of the device's
> capabilities, the bus is restricting the mask to DMA_MASK(32).

Right.

> >                                 /* side-question: is that the right way
> >                                   to describe 4GB length? it seems missing
> >                                   a byte */
> >                         };
> >                 };
> >         };
> >
> > BTW, I hope I understood you right that you wouldn't actually trust the
> > capabilities register by itself but only allow setting the 64-bit mask
> > in the arch code if the DT doesn't have any information about the
> > bus being incapable of doing this, right?
> 
> Ideally no, but it appears we are ATM for midway. We get away with it
> since it is midway/highbank specific driver setup and know there are
> not any restrictions in addressing RAM. I think we have to keep bus
> and device masks separate and the final mask is the AND of them. There
> isn't really a construct to do this currently AFAICT. dma_set_mask
> could be modified to adjust the mask, but that would be a change in
> how dma_set_mask works as it is supposed to fail if the requested mask
> cannot be supported.
> 
> Perhaps using dma_get_required_mask to parse dma-ranges would be
> appropriate here? It doesn't seem widely used though.

Hmm, I've never even heard of that interface.

> >> Compatibility issues:
> >>   - Old DT without DMA properties defined: Drivers may still need to call dma_set_mask(DMA_MASK(64)
> >>   - Old DT without DMA properties defined and DMA range is defined outside of RAM:
> >>   arch or drivers code still has to provide proper address translation routines.
> >
> > 64-bit SOC includes 32-bit CPU with LPAE, right?
> >
> > Would you want to allow dma_set_mask(DMA_MASK(64)) to succeed in the absence
> > of the dma-ranges properties?
> 
> Yes, because the default should be no restrictions are imposed by the
> bus. DT should describe the restrictions or translations. The default
> should be masters have the same view of memory map as the cpu and can
> use all address bits the device has.

Hmm, I was rather thinking we should mirror what we have for the "ranges"
property where an empty property signifies that there are no restrictions
and the parent and child bus have the same view.

In case of "ranges", the absence of the property means that there is
no possible translation, but we can't do that for "dma-ranges" because
that would break every single 32-bit SoC in existence today.

Defining this case to mean "only 32-bit translations are possible" is
a bit hacky but I think it would be a more pragmatic approach.

> >> [3] 64 bit SOC (only 32 bit masters):
> >>
> >> - DMA range is present and It's defined inside RAM (no address translation needed):
> >>   DMA range can be absent - default DMA configuration will be applied.
> >>
> >> - DMA range present and It's defined outside of RAM (address translation needed):
> >>   DMA range has to be present and depending on its size
> >>   dma_mask will be equal or less than DMA_MASK(32); and dma_pfn_offset will be calculated.
> >>
> >> Compatibility issues:
> >>   - Old DT without DMA properties defined and DMA range is defined outside of RAM:
> >>   arch or drivers code has to provide proper address translation routines.
> >>
> >>
> >> [4] 64 bit SOC (32 bit and 64 masters):
> >> - This is combination of above 2,3 cases.
> >>   The current approach will allow to handle it by defining two buses in DT
> >>   "simple-bus32" and "simple-bus64", and each one should have its own DMA properties
> >>   defined in DT.
> >
> > We already try to describe the hierarchy of the buses, and only AXI4 buses can be
> > 64-bit, unlike AHB or other types of AMBA buses AFAIK. We should just call them
> > what they are.
> 
> I don't think we do describe the hierarchy. We are describing the
> slave side which is not necessarily the same as the master side.
> Internal buses are also much more complex than any of the SOCs
> describe in their DT.

I think we try to describe them as good as we can if we have access to
the documentation. I keep hearing people mention the case where the
slave side is different from the master side, but is that actually
a common scenario? If it's as rare as I suspect, we can fake a hierarchy
for the cases we need, but describe most of them correctly.

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

Re: [PATCH 4/7] of: configure the platform device dma_mask and dma_pfn_offset

[Rob Herring]

On Fri, Mar 14, 2014 at 12:25 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 14 March 2014, Rob Herring wrote:
>> On Wed, Mar 12, 2014 at 11:58 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Wednesday 12 March 2014 15:19:48 Grygorii Strashko wrote:

>> In the case of
>> 64-bit DMA, the device should try to set its mask to DMA_MASK(64), but
>> the parent dma-ranges should restrict that to DMA_MASK(40).
>
> Now I'm confused about what dma_set_mask is actually supposed to do here.
> I /think/ it should fail the DMA_MASK(64) call if the bus supports
> less than 64 bits as in the above example. However it seems like a
> valid shortcut to always succeed here if the effective mask covers
> all of the installed RAM.
>
> That would mean that even if we only have a 32-bit bus, but all RAM
> resides below 4GB, a call to dma_set_mask using DMA_MASK(64) would
> also succeed.

Which is correct? The mask being bigger than all of RAM is okay. The mask only matters when it is less than all of RAM.

It should fail if RAM is >40 bits of address space.

>
>> >                         ...
>> >                 };
>> >
>> >                 ahb {
>> >                         #address-cells = <1>;
>> >                         #size-cells = <1>;
>>
>> This would need to be 2. ePAPR says the size cells size is determined
>> by #size-cells in the same node.
>
> Ah, of course.
>
>> >                         dma-ranges = <0 0  0  0x1 0>; /* only 4GB */
>> >
>> >                         ehci {
>> >                                 dma-ranges = <0  0  0xffffffff>;
>>
>> Again, I don't think this is needed. Here, regardless of the device's
>> capabilities, the bus is restricting the mask to DMA_MASK(32).
>
> Right.
>
>> >                                 /* side-question: is that the right way
>> >                                   to describe 4GB length? it seems missing
>> >                                   a byte */
>> >                         };
>> >                 };
>> >         };
>> >
>> > BTW, I hope I understood you right that you wouldn't actually trust the
>> > capabilities register by itself but only allow setting the 64-bit mask
>> > in the arch code if the DT doesn't have any information about the
>> > bus being incapable of doing this, right?
>>
>> Ideally no, but it appears we are ATM for midway. We get away with it
>> since it is midway/highbank specific driver setup and know there are
>> not any restrictions in addressing RAM. I think we have to keep bus
>> and device masks separate and the final mask is the AND of them. There
>> isn't really a construct to do this currently AFAICT. dma_set_mask
>> could be modified to adjust the mask, but that would be a change in
>> how dma_set_mask works as it is supposed to fail if the requested mask
>> cannot be supported.
>>
>> Perhaps using dma_get_required_mask to parse dma-ranges would be
>> appropriate here? It doesn't seem widely used though.
>
> Hmm, I've never even heard of that interface.
>
>> >> Compatibility issues:
>> >>   - Old DT without DMA properties defined: Drivers may still need to call dma_set_mask(DMA_MASK(64)
>> >>   - Old DT without DMA properties defined and DMA range is defined outside of RAM:
>> >>   arch or drivers code still has to provide proper address translation routines.
>> >
>> > 64-bit SOC includes 32-bit CPU with LPAE, right?
>> >
>> > Would you want to allow dma_set_mask(DMA_MASK(64)) to succeed in the absence
>> > of the dma-ranges properties?
>>
>> Yes, because the default should be no restrictions are imposed by the
>> bus. DT should describe the restrictions or translations. The default
>> should be masters have the same view of memory map as the cpu and can
>> use all address bits the device has.
>
> Hmm, I was rather thinking we should mirror what we have for the "ranges"
> property where an empty property signifies that there are no restrictions
> and the parent and child bus have the same view.
>
> In case of "ranges", the absence of the property means that there is
> no possible translation, but we can't do that for "dma-ranges" because
> that would break every single 32-bit SoC in existence today.
>
> Defining this case to mean "only 32-bit translations are possible" is
> a bit hacky but I think it would be a more pragmatic approach.
>
>> >> [3] 64 bit SOC (only 32 bit masters):
>> >>
>> >> - DMA range is present and It's defined inside RAM (no address translation needed):
>> >>   DMA range can be absent - default DMA configuration will be applied.
>> >>
>> >> - DMA range present and It's defined outside of RAM (address translation needed):
>> >>   DMA range has to be present and depending on its size
>> >>   dma_mask will be equal or less than DMA_MASK(32); and dma_pfn_offset will be calculated.
>> >>
>> >> Compatibility issues:
>> >>   - Old DT without DMA properties defined and DMA range is defined outside of RAM:
>> >>   arch or drivers code has to provide proper address translation routines.
>> >>
>> >>
>> >> [4] 64 bit SOC (32 bit and 64 masters):
>> >> - This is combination of above 2,3 cases.
>> >>   The current approach will allow to handle it by defining two buses in DT
>> >>   "simple-bus32" and "simple-bus64", and each one should have its own DMA properties
>> >>   defined in DT.
>> >
>> > We already try to describe the hierarchy of the buses, and only AXI4 buses can be
>> > 64-bit, unlike AHB or other types of AMBA buses AFAIK. We should just call them
>> > what they are.
>>
>> I don't think we do describe the hierarchy. We are describing the
>> slave side which is not necessarily the same as the master side.
>> Internal buses are also much more complex than any of the SOCs
>> describe in their DT.
>
> I think we try to describe them as good as we can if we have access to
> the documentation. I keep hearing people mention the case where the
> slave side is different from the master side, but is that actually
> a common scenario? If it's as rare as I suspect, we can fake a hierarchy
> for the cases we need, but describe most of them correctly.
>
>         Arnd

Re: [PATCH 4/7] of: configure the platform device dma_mask and dma_pfn_offset

[Santosh Shilimkar]

Arnd, Rob,

On Friday 14 March 2014 01:25 PM, Arnd Bergmann wrote:
> On Friday 14 March 2014, Rob Herring wrote:
>> On Wed, Mar 12, 2014 at 11:58 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Wednesday 12 March 2014 15:19:48 Grygorii Strashko wrote:
>>>>> Isn't the question here how do we handle restrictions added by the
>>>>> bus? It seems while this series adds support for handling dma-ranges
>>>>> to set the default case, it also needs to handle when the driver sets
>>>>> the mask. Is this not simply a matter of having dma_set_mask also
>>>>> parse dma-ranges (or reuse a saved bus mask)?
>>>
>>> With the current proposed implementation, the device also has to set
>>> a "dma-ranges" property to get any DMA support, we don't just take
>>> the mask of the parent bus. This is important because the translation
>>> does not have to be 1:1 but there can be an offset between the device
>>> and the parent. I'd prefer to leave this explicit.
>>
>> But according to Ben H, dma-ranges is a bridge (i.e. parent) property
>> and not a device property[1]. So I don't think the device's mask
>> (again, before any bus restrictions) belongs in DT. A given IP block
>> is going to have some number of address bits for DMA. How it is hooked
>> up into an SOC is a separate issue. The driver should know its mask
>> whether it is fixed, discoverable, or tied to the compatible string.
>>
>> Santosh's patch only looks for dma-ranges in parent nodes which I
>> believe is correct.
> 
> Ok, good point.
> 
>>>>>> I think this approach is much less useful for platform devices than it is
>>>>>> for PCI devices, where we don't explicitly describe the "ranges" for each
>>>>>> device. Are you thinking of off-chip or on-chip DMA masters here? If
>>>>>> on-chip, I don't think it's likely that we would end up with different
>>>>>> versions of the chip that have the same device on there but not the
>>>>>> same DMA capabilities.
>>>>>
>>>>> Sounds like a challenge to h/w designers. :)
>>>>>
>>>>> I'm mainly thinking about the midway case where all masters are 32-bit
>>>>> except AHCI which is 64-bit. The core libahci sets the mask based on
>>>>> the capabilities register regardless of PCI or platform bus. Requiring
>>>>> this to be setup by the platform or in the DT seems like a step
>>>>> backwards. A slightly more complicated case would be something like
>>>>> midway, but with RAM starting at say 2GB and DMA masters have the same
>>>>> view as the cpu (i.e. no offset). Then the platform does need to set
>>>>> the mask to (2^31 -1).
>>>
>>> So how would you show this in DT? I'd assume that some of the other
>>> devices on midway have drivers that may also try to set a 64-bit mask,
>>> like EHCI for instance does. Is AHCI the only one that sits on a 64-bit
>>> wide bus, like this?
>>>
>>>         axi {
>>>                 #address-cells = <2>;
>>>                 #size-cells = <2>;
>>>                 dma-ranges = <0 0 0 0 0x100 0>; /* max phys address space */
>>>
>>>                 ahci {
>>>                         dma-ranges = <0 0  0 0  0x100 0>;
>>
>> There is no point to this dma-ranges here. Based on the capabilities
>> reg, we know that we can do either 32 or 64 bit DMA.
> 
> Ok
> 
>> In the case of
>> 64-bit DMA, the device should try to set its mask to DMA_MASK(64), but
>> the parent dma-ranges should restrict that to DMA_MASK(40).
> 
> Now I'm confused about what dma_set_mask is actually supposed to do here.
> I /think/ it should fail the DMA_MASK(64) call if the bus supports
> less than 64 bits as in the above example. However it seems like a
> valid shortcut to always succeed here if the effective mask covers
> all of the installed RAM.
> 
> That would mean that even if we only have a 32-bit bus, but all RAM
> resides below 4GB, a call to dma_set_mask using DMA_MASK(64) would
> also succeed.
> 
>>>                         ...
>>>                 };
>>>
>>>                 ahb {
>>>                         #address-cells = <1>;
>>>                         #size-cells = <1>;
>>
>> This would need to be 2. ePAPR says the size cells size is determined
>> by #size-cells in the same node.
> 
> Ah, of course.
> 
>>>                         dma-ranges = <0 0  0  0x1 0>; /* only 4GB */
>>>
>>>                         ehci {
>>>                                 dma-ranges = <0  0  0xffffffff>;
>>
>> Again, I don't think this is needed. Here, regardless of the device's
>> capabilities, the bus is restricting the mask to DMA_MASK(32).
> 
> Right.
> 
>>>                                 /* side-question: is that the right way
>>>                                   to describe 4GB length? it seems missing
>>>                                   a byte */
>>>                         };
>>>                 };
>>>         };
>>>
>>> BTW, I hope I understood you right that you wouldn't actually trust the
>>> capabilities register by itself but only allow setting the 64-bit mask
>>> in the arch code if the DT doesn't have any information about the
>>> bus being incapable of doing this, right?
>>
>> Ideally no, but it appears we are ATM for midway. We get away with it
>> since it is midway/highbank specific driver setup and know there are
>> not any restrictions in addressing RAM. I think we have to keep bus
>> and device masks separate and the final mask is the AND of them. There
>> isn't really a construct to do this currently AFAICT. dma_set_mask
>> could be modified to adjust the mask, but that would be a change in
>> how dma_set_mask works as it is supposed to fail if the requested mask
>> cannot be supported.
>>
>> Perhaps using dma_get_required_mask to parse dma-ranges would be
>> appropriate here? It doesn't seem widely used though.
> 
> Hmm, I've never even heard of that interface.
> 
>>>> Compatibility issues:
>>>>   - Old DT without DMA properties defined: Drivers may still need to call dma_set_mask(DMA_MASK(64)
>>>>   - Old DT without DMA properties defined and DMA range is defined outside of RAM:
>>>>   arch or drivers code still has to provide proper address translation routines.
>>>
>>> 64-bit SOC includes 32-bit CPU with LPAE, right?
>>>
>>> Would you want to allow dma_set_mask(DMA_MASK(64)) to succeed in the absence
>>> of the dma-ranges properties?
>>
>> Yes, because the default should be no restrictions are imposed by the
>> bus. DT should describe the restrictions or translations. The default
>> should be masters have the same view of memory map as the cpu and can
>> use all address bits the device has.
> 
> Hmm, I was rather thinking we should mirror what we have for the "ranges"
> property where an empty property signifies that there are no restrictions
> and the parent and child bus have the same view.
> 
> In case of "ranges", the absence of the property means that there is
> no possible translation, but we can't do that for "dma-ranges" because
> that would break every single 32-bit SoC in existence today.
> 
> Defining this case to mean "only 32-bit translations are possible" is
> a bit hacky but I think it would be a more pragmatic approach.
> 
>>>> [3] 64 bit SOC (only 32 bit masters):
>>>>
>>>> - DMA range is present and It's defined inside RAM (no address translation needed):
>>>>   DMA range can be absent - default DMA configuration will be applied.
>>>>
>>>> - DMA range present and It's defined outside of RAM (address translation needed):
>>>>   DMA range has to be present and depending on its size
>>>>   dma_mask will be equal or less than DMA_MASK(32); and dma_pfn_offset will be calculated.
>>>>
>>>> Compatibility issues:
>>>>   - Old DT without DMA properties defined and DMA range is defined outside of RAM:
>>>>   arch or drivers code has to provide proper address translation routines.
>>>>
>>>>
>>>> [4] 64 bit SOC (32 bit and 64 masters):
>>>> - This is combination of above 2,3 cases.
>>>>   The current approach will allow to handle it by defining two buses in DT
>>>>   "simple-bus32" and "simple-bus64", and each one should have its own DMA properties
>>>>   defined in DT.
>>>
>>> We already try to describe the hierarchy of the buses, and only AXI4 buses can be
>>> 64-bit, unlike AHB or other types of AMBA buses AFAIK. We should just call them
>>> what they are.
>>
>> I don't think we do describe the hierarchy. We are describing the
>> slave side which is not necessarily the same as the master side.
>> Internal buses are also much more complex than any of the SOCs
>> describe in their DT.
> 
> I think we try to describe them as good as we can if we have access to
> the documentation. I keep hearing people mention the case where the
> slave side is different from the master side, but is that actually
> a common scenario? If it's as rare as I suspect, we can fake a hierarchy
> for the cases we need, but describe most of them correctly.
> 
Looks like we don't have agreement here on the mask setup. Whats the
way forward. I really like to get some sort of support so that
the dma address translation is possible as needed for Keystone.

Regards,
Santosh

Re: [PATCH 4/7] of: configure the platform device dma_mask and dma_pfn_offset

[Rob Herring]

On Wed, Mar 12, 2014 at 8:19 AM, Grygorii Strashko <grygorii.strashko@ti.com> wrote:
> Hi Rob, Arnd,
>
> On 03/12/2014 02:15 AM, Rob Herring wrote:
>> On Sun, Mar 9, 2014 at 12:39 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Saturday 08 March 2014, Rob Herring wrote:
>>>> On Fri, Mar 7, 2014 at 10:02 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>
>>>>> a) Follow what we do for PCI devices: assume that we can do DMA_MASK(32)
>>>>> on any device, and have drivers call dma_set_mask(DMA_MASK(64)) on devices
>>>>> that would like to do more than that, or call e.g. dma_set_mask(DMA_MASK(28))
>>>>> for devices that can do less than 32 bit, as given in the argument. This
>>>>> approach would be most consistent with the way PCI works, but it doesn't
>>>>> really work well for the case where the mask is less than 32-bit and the
>>>>> device driver doesn't know that.
>>>>>
>>>>> b) Never have to call dma_set_mask() for platform devices and assume that the
>>>>> platform code sets it up correctly. This would probably be the simpler
>>>>> solution, and I can't think of any downsides at the moment.
>>>>
>>>> I don't think we want this. In the case of setting up 64-bit masters,
>>>> it is typically the device that knows if it can do 64-bit DMA either
>>>> thru a capabilities register or compatible value. That device specific
>>>> knowledge should really stay within the device's driver.
>>>
>>> So you think we should still set a 64-bit mask in the "ranges" property
>>> for devices that can only do 32-bit and let the driver figure it out?
>>
>> No, I think option a makes more sense. BTW, don't you mean dma-ranges?
>> A device has it's own mask which is typically going to be 32 or
>> 64-bit. The core code may not know what the device supports, so the
>> device has to set it's own mask. I don't see a way around that. DT is
>> not a good option because it is discoverable in some cases.
>>
>> Isn't the question here how do we handle restrictions added by the
>> bus? It seems while this series adds support for handling dma-ranges
>> to set the default case, it also needs to handle when the driver sets
>> the mask. Is this not simply a matter of having dma_set_mask also
>> parse dma-ranges (or reuse a saved bus mask)?
>>
>>> I think this approach is much less useful for platform devices than it is
>>> for PCI devices, where we don't explicitly describe the "ranges" for each
>>> device. Are you thinking of off-chip or on-chip DMA masters here? If
>>> on-chip, I don't think it's likely that we would end up with different
>>> versions of the chip that have the same device on there but not the
>>> same DMA capabilities.
>>
>> Sounds like a challenge to h/w designers. :)
>>
>> I'm mainly thinking about the midway case where all masters are 32-bit
>> except AHCI which is 64-bit. The core libahci sets the mask based on
>> the capabilities register regardless of PCI or platform bus. Requiring
>> this to be setup by the platform or in the DT seems like a step
>> backwards. A slightly more complicated case would be something like
>> midway, but with RAM starting at say 2GB and DMA masters have the same
>> view as the cpu (i.e. no offset). Then the platform does need to set
>> the mask to (2^31 -1).
>>
>
> mask has to be (2^32 -1).
>
> I'd like to add my 5 cents here :)
> First of all this solution is pretended to be generic, so by default it
> can't handle all possible cases (corner cases) - only widely used.
> Arch or driver can always fix up it through bus notifier or driver's probe.
>
> Second, it works only for DT-boot case when devices instantiated from DT in generic way.
> So, if any bus creates devices in its own way - proper DMA configuration of such
> devices will be private problem of this bus.
>
> I've tried to formulate possible use cases which can be and can't be covered
> by this approach:
> [1] 32 bit SoC:
> - no DMA range limitation: DMA range has to be absent or be equal to RAM
>   (default DMA configuration will be applied)
>
> - DMA range present and It's defined inside RAM (no address translation needed):
>   DMA range can be present and depending on its size
>   dma_mask will be equal or less than DMA_MASK(32).
>   (default DMA configuration will be applied - if DMA range isn't present)
>
> - DMA range present and It's defined outside of RAM (address translation needed):
>   DMA range has to be present and depending on its size
>   dma_mask will be equal or less than DMA_MASK(32); and dma_pfn_offset will be calculated.
>
> Compatibility issues:
>   - Old DT without DMA properties defined and DMA range is defined outside of RAM:
>   arch or drivers code has to provide proper address translation routines.
>
> [2] 64 bit SOC (only 64 bit masters):
> - no DMA range limitation: DMA range has to be present and equal to RAM
>
> - DMA range present and It's defined inside RAM (no address translation needed):
>   DMA range has to be present and depending on its size
>   dma_mask will be equal or less than DMA_MASK(64).
>
> - DMA range present and It's defined outside of RAM (address translation needed):
>   DMA range has to be present and depending on its size
>   dma_mask will be equal or less than DMA_MASK(64); and dma_pfn_offset will be calculated.
>
> Compatibility issues:
>   - Old DT without DMA properties defined: Drivers may still need to call dma_set_mask(DMA_MASK(64)
>   - Old DT without DMA properties defined and DMA range is defined outside of RAM:
>   arch or drivers code still has to provide proper address translation routines.
>
> [3] 64 bit SOC (only 32 bit masters):

and LPAE SOC

>
> - DMA range is present and It's defined inside RAM (no address translation needed):
>   DMA range can be absent - default DMA configuration will be applied.
>
> - DMA range present and It's defined outside of RAM (address translation needed):
>   DMA range has to be present and depending on its size
>   dma_mask will be equal or less than DMA_MASK(32); and dma_pfn_offset will be calculated.
>
> Compatibility issues:
>   - Old DT without DMA properties defined and DMA range is defined outside of RAM:
>   arch or drivers code has to provide proper address translation routines.
>
>
> [4] 64 bit SOC (32 bit and 64 masters):

and LPAE SOC

> - This is combination of above 2,3 cases.
>   The current approach will allow to handle it by defining two buses in DT
>   "simple-bus32" and "simple-bus64", and each one should have its own DMA properties
>   defined in DT.

This does not really reflect reality. The width of a bus master is a property of that master, not the bus.

> - The worst case will be if some device is able to change its DMA configuration
>   dynamically by reading its DMA configuration from HW. The main difficulties will
>   be to handle DMA addresses translation properly in this case. But, in my opinion,
>   such behavior will be needed in very rare case as DT has to describe specific SoC
>   and such device has to belong to only one bus as per SoC specification.
>
> Compatibility issues:
>   - Old DT without DMA properties defined and 32 bits DMA range is defined outside of RAM:
>   arch or drivers code has to provide proper address translation routines.
>
> As per above, this approach will allow to handle most of cases and configure DMA properly
> with one exception - device is able to change its DMA configuration dynamically.
>
> Also, for compatibility reasons, arches and drivers may need to continue maintain their own
> DMA addresses translation routines to support the case
> "Old DT without DMA properties defined and DMA range is defined outside of RAM".
>
> Regards,
> - grygorii
>

[PATCH 5/7] of: Add set_arch_dma_coherent_ops() and setup coherent dma_ops

[Santosh Shilimkar]

Add set_arch_dma_coherent_ops() for architectures to setup coherent dma_ops.
Update dt_dma_configure() to look for a "dma-coherent" property in the
device's node, and its ancestors If this property is found, we call
set_arch_dma_coherent_ops() per device to apply coherent DMA configuartion.

The set_arch_dma_coherent_ops() is declared as __weak and it's expected
that the arch's needing it will imlepment it.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Olof Johansson <olof@lixom.net>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 drivers/of/platform.c       |    5 +++++
 include/linux/dma-mapping.h |    7 +++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 53bb12f..272e390 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -255,6 +255,11 @@ static void dt_dma_configure(struct device *dev)
 		dev_err(dev, "failed to set coherent DMA mask %pad\n",
 			&dma_mask);
 	}
+
+	if (of_dma_is_coherent(dev->of_node)) {
+		set_arch_dma_coherent_ops(dev);
+		dev_dbg(dev, "device is dma coherent\n");
+	}
 }
 
 /**
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index fd4aee2..c7d9b1b 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -123,6 +123,13 @@ static inline int dma_coerce_mask_and_coherent(struct device *dev, u64 mask)
 
 extern u64 dma_get_required_mask(struct device *dev);
 
+#ifndef set_arch_dma_coherent_ops
+static inline int set_arch_dma_coherent_ops(struct device *dev)
+{
+	return 0;
+}
+#endif
+
 static inline unsigned int dma_get_max_seg_size(struct device *dev)
 {
 	return dev->dma_parms ? dev->dma_parms->max_segment_size : 65536;
-- 
1.7.9.5

[PATCH 6/7] ARM: dma: Use dma_pfn_offset for dma address translation

[Santosh Shilimkar]

From: Grygorii Strashko <grygorii.strashko@ti.com>

In most of cases DMA addresses can be performed using offset value of
 Bus address space relatively to physical address space as following:

PFN->DMA:
 __pfn_to_phys(pfn + [-]dma_pfn_offset)

DMA->PFN:
 __phys_to_pfn(dma_addr) + [-]dma_pfn_offset

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Olof Johansson <olof@lixom.net>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 arch/arm/include/asm/dma-mapping.h |   17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index e701a4d..8c12149 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -58,22 +58,31 @@ static inline int dma_set_mask(struct device *dev, u64 mask)
 #ifndef __arch_pfn_to_dma
 static inline dma_addr_t pfn_to_dma(struct device *dev, unsigned long pfn)
 {
-	return (dma_addr_t)__pfn_to_bus(pfn);
+	if (!dev)
+		return DMA_ERROR_CODE;
+	return (dma_addr_t)__pfn_to_bus(pfn - dev->dma_pfn_offset);
 }
 
 static inline unsigned long dma_to_pfn(struct device *dev, dma_addr_t addr)
 {
-	return __bus_to_pfn(addr);
+	if (!dev)
+		return 0;
+	return __bus_to_pfn(addr) + dev->dma_pfn_offset;
 }
 
 static inline void *dma_to_virt(struct device *dev, dma_addr_t addr)
 {
-	return (void *)__bus_to_virt((unsigned long)addr);
+	if (!dev)
+		return NULL;
+	return (void *)__bus_to_virt(__pfn_to_bus(dma_to_pfn(dev, addr)));
 }
 
 static inline dma_addr_t virt_to_dma(struct device *dev, void *addr)
 {
-	return (dma_addr_t)__virt_to_bus((unsigned long)(addr));
+	if (!dev)
+		return DMA_ERROR_CODE;
+	return pfn_to_dma(dev,
+			   __bus_to_pfn(__virt_to_bus((unsigned long)(addr))));
 }
 
 #else
-- 
1.7.9.5

[PATCH 7/7] ARM: dma: implement set_arch_dma_coherent_ops()

[Santosh Shilimkar]

Implement the set_arch_dma_coherent_ops() for ARM architecture.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Olof Johansson <olof@lixom.net>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 arch/arm/include/asm/dma-mapping.h |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index 8c12149..765c9e7 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -114,6 +114,13 @@ static inline unsigned long dma_max_pfn(struct device *dev)
 }
 #define dma_max_pfn(dev) dma_max_pfn(dev)
 
+static inline int set_arch_dma_coherent_ops(struct device *dev)
+{
+	set_dma_ops(dev, &arm_coherent_dma_ops);
+	return 0;
+}
+#define set_arch_dma_coherent_ops(dev)	set_arch_dma_coherent_ops(dev)
+
 static inline dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr)
 {
 	unsigned int offset = paddr & ~PAGE_MASK;
-- 
1.7.9.5