linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] of: Make of_dma_get_range() work on bus nodes
@ 2019-10-08 19:52 Rob Herring
  2019-10-08 20:51 ` Nicolas Saenz Julienne
  2019-10-09 10:56 ` Robin Murphy
  0 siblings, 2 replies; 7+ messages in thread
From: Rob Herring @ 2019-10-08 19:52 UTC (permalink / raw)
  To: devicetree
  Cc: Florian Fainelli, Geert Uytterhoeven, Arnd Bergmann,
	Frank Rowand, linux-kernel, Christoph Hellwig, Marek Vasut,
	Lorenzo Pieralisi, linux-arm-kernel, Simon Horman, Robin Murphy,
	Nicolas Saenz Julienne, Stefan Wahren

From: Robin Murphy <robin.murphy@arm.com>

Since the "dma-ranges" property is only valid for a node representing a
bus, of_dma_get_range() currently assumes the node passed in is a leaf
representing a device, and starts the walk from its parent. In cases
like PCI host controllers on typical FDT systems, however, where the PCI
endpoints are probed dynamically the initial leaf node represents the
'bus' itself, and this logic means we fail to consider any "dma-ranges"
describing the host bridge itself. Rework the logic such that
of_dma_get_range() also works correctly starting from a bus node
containing "dma-ranges".

While this does mean "dma-ranges" could incorrectly be in a device leaf
node, there isn't really any way in this function to ensure that a leaf
node is or isn't a bus node.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
[robh: Allow for the bus child node to still be passed in]
Signed-off-by: Rob Herring <robh@kernel.org>
---
Resending, hit send too quickly.

v2:
 - Ensure once we find dma-ranges, every parent has it.
 - Only get the #{size,address}-cells after we find non-empty dma-ranges
 - Add a check on the 'dma-ranges' length

This is all that remains of the dma-ranges series. I've applied the rest 
of the series prep and fixes. I dropped "of: Ratify of_dma_configure() 
interface" as the assertions that the node pointer being the parent only 
when struct device doesn't have a DT node pointer is not always 
true.

I didn't include any tested-bys as this has changed a bit. A git branch 
is here[1].

Rob

[1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git dma-masks-v2

 drivers/of/address.c | 44 ++++++++++++++++++--------------------------
 1 file changed, 18 insertions(+), 26 deletions(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 5ce69d026584..99c1b8058559 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -930,47 +930,39 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *siz
 	const __be32 *ranges = NULL;
 	int len, naddr, nsize, pna;
 	int ret = 0;
+	bool found_dma_ranges = false;
 	u64 dmaaddr;
 
-	if (!node)
-		return -EINVAL;
-
-	while (1) {
-		struct device_node *parent;
-
-		naddr = of_n_addr_cells(node);
-		nsize = of_n_size_cells(node);
-
-		parent = __of_get_dma_parent(node);
-		of_node_put(node);
-
-		node = parent;
-		if (!node)
-			break;
-
+	while (node) {
 		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;
+		/* Once we find 'dma-ranges', then a missing one is an error */
+		if (found_dma_ranges && !ranges) {
+			ret = -ENODEV;
+			goto out;
+		}
+		found_dma_ranges = true;
+
+		node = of_get_next_dma_parent(node);
 	}
 
-	if (!ranges) {
+	if (!node || !ranges) {
 		pr_debug("no dma-ranges found for node(%pOF)\n", np);
 		ret = -ENODEV;
 		goto out;
 	}
 
-	len /= sizeof(u32);
-
+	naddr = of_bus_n_addr_cells(node);
+	nsize = of_bus_n_size_cells(node);
 	pna = of_n_addr_cells(node);
+	if ((len / sizeof(__be32)) % (pna + naddr + nsize)) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	/* dma-ranges format:
 	 * DMA addr	: naddr cells
@@ -978,7 +970,7 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *siz
 	 * size		: nsize cells
 	 */
 	dmaaddr = of_read_number(ranges, naddr);
-	*paddr = of_translate_dma_address(np, ranges);
+	*paddr = of_translate_dma_address(node, ranges + naddr);
 	if (*paddr == OF_BAD_ADDR) {
 		pr_err("translation of DMA address(%llx) to CPU address failed node(%pOF)\n",
 		       dmaaddr, np);
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] of: Make of_dma_get_range() work on bus nodes
  2019-10-08 19:52 [PATCH v2] of: Make of_dma_get_range() work on bus nodes Rob Herring
@ 2019-10-08 20:51 ` Nicolas Saenz Julienne
  2019-10-09  1:03   ` Rob Herring
  2019-10-09 10:56 ` Robin Murphy
  1 sibling, 1 reply; 7+ messages in thread
From: Nicolas Saenz Julienne @ 2019-10-08 20:51 UTC (permalink / raw)
  To: Rob Herring, devicetree
  Cc: Florian Fainelli, Geert Uytterhoeven, Arnd Bergmann,
	Robin Murphy, linux-kernel, Christoph Hellwig, Marek Vasut,
	Lorenzo Pieralisi, Stefan Wahren, Simon Horman, Frank Rowand,
	linux-arm-kernel


[-- Attachment #1.1: Type: text/plain, Size: 4546 bytes --]

Hi Rob/Robin,

On Tue, 2019-10-08 at 14:52 -0500, Rob Herring wrote:
> From: Robin Murphy <robin.murphy@arm.com>
> 
> Since the "dma-ranges" property is only valid for a node representing a
> bus, of_dma_get_range() currently assumes the node passed in is a leaf
> representing a device, and starts the walk from its parent. In cases
> like PCI host controllers on typical FDT systems, however, where the PCI
> endpoints are probed dynamically the initial leaf node represents the
> 'bus' itself, and this logic means we fail to consider any "dma-ranges"
> describing the host bridge itself. Rework the logic such that
> of_dma_get_range() also works correctly starting from a bus node
> containing "dma-ranges".
> 
> While this does mean "dma-ranges" could incorrectly be in a device leaf
> node, there isn't really any way in this function to ensure that a leaf
> node is or isn't a bus node.

Sorry, I'm not totally sure if this is what you're pointing out with the last
sentence. But, what about the case of a bus configuring a device which also
happens to be a memory mapped bus (say a PCI platform device). It'll get it's
dma config based on its own dma-ranges which is not what we want.

> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> [robh: Allow for the bus child node to still be passed in]
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> Resending, hit send too quickly.
> 
> v2:
>  - Ensure once we find dma-ranges, every parent has it.

I like this new approach.

Regards,
Nicolas

>  - Only get the #{size,address}-cells after we find non-empty dma-ranges
>  - Add a check on the 'dma-ranges' length
> 
> This is all that remains of the dma-ranges series. I've applied the rest 
> of the series prep and fixes. I dropped "of: Ratify of_dma_configure() 
> interface" as the assertions that the node pointer being the parent only 
> when struct device doesn't have a DT node pointer is not always 
> true.
> 
> I didn't include any tested-bys as this has changed a bit. A git branch 
> is here[1].
> 
> Rob
> 
> [1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git dma-masks-v2
> 
>  drivers/of/address.c | 44 ++++++++++++++++++--------------------------
>  1 file changed, 18 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 5ce69d026584..99c1b8058559 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -930,47 +930,39 @@ int of_dma_get_range(struct device_node *np, u64
> *dma_addr, u64 *paddr, u64 *siz
>  	const __be32 *ranges = NULL;
>  	int len, naddr, nsize, pna;
>  	int ret = 0;
> +	bool found_dma_ranges = false;
>  	u64 dmaaddr;
>  
> -	if (!node)
> -		return -EINVAL;
> -
> -	while (1) {
> -		struct device_node *parent;
> -
> -		naddr = of_n_addr_cells(node);
> -		nsize = of_n_size_cells(node);
> -
> -		parent = __of_get_dma_parent(node);
> -		of_node_put(node);
> -
> -		node = parent;
> -		if (!node)
> -			break;
> -
> +	while (node) {
>  		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;
> +		/* Once we find 'dma-ranges', then a missing one is an error */
> +		if (found_dma_ranges && !ranges) {
> +			ret = -ENODEV;
> +			goto out;
> +		}
> +		found_dma_ranges = true;
> +
> +		node = of_get_next_dma_parent(node);
>  	}
>  
> -	if (!ranges) {
> +	if (!node || !ranges) {
>  		pr_debug("no dma-ranges found for node(%pOF)\n", np);
>  		ret = -ENODEV;
>  		goto out;
>  	}
>  
> -	len /= sizeof(u32);
> -
> +	naddr = of_bus_n_addr_cells(node);
> +	nsize = of_bus_n_size_cells(node);
>  	pna = of_n_addr_cells(node);
> +	if ((len / sizeof(__be32)) % (pna + naddr + nsize)) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
>  
>  	/* dma-ranges format:
>  	 * DMA addr	: naddr cells
> @@ -978,7 +970,7 @@ int of_dma_get_range(struct device_node *np, u64
> *dma_addr, u64 *paddr, u64 *siz
>  	 * size		: nsize cells
>  	 */
>  	dmaaddr = of_read_number(ranges, naddr);
> -	*paddr = of_translate_dma_address(np, ranges);
> +	*paddr = of_translate_dma_address(node, ranges + naddr);
>  	if (*paddr == OF_BAD_ADDR) {
>  		pr_err("translation of DMA address(%llx) to CPU address failed
> node(%pOF)\n",
>  		       dmaaddr, np);


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] of: Make of_dma_get_range() work on bus nodes
  2019-10-08 20:51 ` Nicolas Saenz Julienne
@ 2019-10-09  1:03   ` Rob Herring
  2019-10-09 15:24     ` Nicolas Saenz Julienne
  0 siblings, 1 reply; 7+ messages in thread
From: Rob Herring @ 2019-10-09  1:03 UTC (permalink / raw)
  To: Nicolas Saenz Julienne
  Cc: devicetree, Florian Fainelli, Geert Uytterhoeven, Arnd Bergmann,
	Robin Murphy, linux-kernel, Christoph Hellwig, Marek Vasut,
	Lorenzo Pieralisi, Stefan Wahren, Simon Horman, Frank Rowand,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE

On Tue, Oct 8, 2019 at 3:52 PM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> Hi Rob/Robin,
>
> On Tue, 2019-10-08 at 14:52 -0500, Rob Herring wrote:
> > From: Robin Murphy <robin.murphy@arm.com>
> >
> > Since the "dma-ranges" property is only valid for a node representing a
> > bus, of_dma_get_range() currently assumes the node passed in is a leaf
> > representing a device, and starts the walk from its parent. In cases
> > like PCI host controllers on typical FDT systems, however, where the PCI
> > endpoints are probed dynamically the initial leaf node represents the
> > 'bus' itself, and this logic means we fail to consider any "dma-ranges"
> > describing the host bridge itself. Rework the logic such that
> > of_dma_get_range() also works correctly starting from a bus node
> > containing "dma-ranges".
> >
> > While this does mean "dma-ranges" could incorrectly be in a device leaf
> > node, there isn't really any way in this function to ensure that a leaf
> > node is or isn't a bus node.
>
> Sorry, I'm not totally sure if this is what you're pointing out with the last
> sentence. But, what about the case of a bus configuring a device which also
> happens to be a memory mapped bus (say a PCI platform device). It'll get it's
> dma config based on its own dma-ranges which is not what we want.

What I was trying to say is we just can't tell if we should be looking
in the current node or the parent. 'dma-ranges' in a leaf node can be
correct or incorrect.

Your example is a bit different. I'm not sure that case is valid or
can ever work if it is. It's certainly valid that a PCI bridge's
parent has dma-ranges and now we'll actually handle any translation.
The bridge itself is not a DMA-capable device, but just passing thru
DMA. Do we ever need to know the parent's dma-ranges in that case? Or
to put it another way, is looking at anything other than leaf
dma-ranges useful?

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] of: Make of_dma_get_range() work on bus nodes
  2019-10-08 19:52 [PATCH v2] of: Make of_dma_get_range() work on bus nodes Rob Herring
  2019-10-08 20:51 ` Nicolas Saenz Julienne
@ 2019-10-09 10:56 ` Robin Murphy
  1 sibling, 0 replies; 7+ messages in thread
From: Robin Murphy @ 2019-10-09 10:56 UTC (permalink / raw)
  To: Rob Herring, devicetree
  Cc: Florian Fainelli, Geert Uytterhoeven, Arnd Bergmann,
	linux-kernel, Christoph Hellwig, Marek Vasut, Lorenzo Pieralisi,
	linux-arm-kernel, Simon Horman, Frank Rowand,
	Nicolas Saenz Julienne, Stefan Wahren

On 2019-10-08 8:52 pm, Rob Herring wrote:
> From: Robin Murphy <robin.murphy@arm.com>
> 
> Since the "dma-ranges" property is only valid for a node representing a
> bus, of_dma_get_range() currently assumes the node passed in is a leaf
> representing a device, and starts the walk from its parent. In cases
> like PCI host controllers on typical FDT systems, however, where the PCI
> endpoints are probed dynamically the initial leaf node represents the
> 'bus' itself, and this logic means we fail to consider any "dma-ranges"
> describing the host bridge itself. Rework the logic such that
> of_dma_get_range() also works correctly starting from a bus node
> containing "dma-ranges".
> 
> While this does mean "dma-ranges" could incorrectly be in a device leaf
> node, there isn't really any way in this function to ensure that a leaf
> node is or isn't a bus node.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> [robh: Allow for the bus child node to still be passed in]
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> Resending, hit send too quickly.
> 
> v2:
>   - Ensure once we find dma-ranges, every parent has it.
>   - Only get the #{size,address}-cells after we find non-empty dma-ranges
>   - Add a check on the 'dma-ranges' length
> 
> This is all that remains of the dma-ranges series. I've applied the rest
> of the series prep and fixes. I dropped "of: Ratify of_dma_configure()
> interface" as the assertions that the node pointer being the parent only
> when struct device doesn't have a DT node pointer is not always
> true.

I'd still like to rework of_dma_configure() so that callers don't have 
to pass a redundant node in the common case, but that can wait. For now, 
this looks good enough to un-block the various 32-bit-PCI folks at 
least, and we can consider further improvements on top. For the changes:

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

Cheers,
Robin.

> I didn't include any tested-bys as this has changed a bit. A git branch
> is here[1].
> 
> Rob
> 
> [1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git dma-masks-v2
> 
>   drivers/of/address.c | 44 ++++++++++++++++++--------------------------
>   1 file changed, 18 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 5ce69d026584..99c1b8058559 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -930,47 +930,39 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *siz
>   	const __be32 *ranges = NULL;
>   	int len, naddr, nsize, pna;
>   	int ret = 0;
> +	bool found_dma_ranges = false;
>   	u64 dmaaddr;
>   
> -	if (!node)
> -		return -EINVAL;
> -
> -	while (1) {
> -		struct device_node *parent;
> -
> -		naddr = of_n_addr_cells(node);
> -		nsize = of_n_size_cells(node);
> -
> -		parent = __of_get_dma_parent(node);
> -		of_node_put(node);
> -
> -		node = parent;
> -		if (!node)
> -			break;
> -
> +	while (node) {
>   		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;
> +		/* Once we find 'dma-ranges', then a missing one is an error */
> +		if (found_dma_ranges && !ranges) {
> +			ret = -ENODEV;
> +			goto out;
> +		}
> +		found_dma_ranges = true;
> +
> +		node = of_get_next_dma_parent(node);
>   	}
>   
> -	if (!ranges) {
> +	if (!node || !ranges) {
>   		pr_debug("no dma-ranges found for node(%pOF)\n", np);
>   		ret = -ENODEV;
>   		goto out;
>   	}
>   
> -	len /= sizeof(u32);
> -
> +	naddr = of_bus_n_addr_cells(node);
> +	nsize = of_bus_n_size_cells(node);
>   	pna = of_n_addr_cells(node);
> +	if ((len / sizeof(__be32)) % (pna + naddr + nsize)) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
>   
>   	/* dma-ranges format:
>   	 * DMA addr	: naddr cells
> @@ -978,7 +970,7 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *siz
>   	 * size		: nsize cells
>   	 */
>   	dmaaddr = of_read_number(ranges, naddr);
> -	*paddr = of_translate_dma_address(np, ranges);
> +	*paddr = of_translate_dma_address(node, ranges + naddr);
>   	if (*paddr == OF_BAD_ADDR) {
>   		pr_err("translation of DMA address(%llx) to CPU address failed node(%pOF)\n",
>   		       dmaaddr, np);
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] of: Make of_dma_get_range() work on bus nodes
  2019-10-09  1:03   ` Rob Herring
@ 2019-10-09 15:24     ` Nicolas Saenz Julienne
  2019-10-09 15:24       ` Nicolas Saenz Julienne
  0 siblings, 1 reply; 7+ messages in thread
From: Nicolas Saenz Julienne @ 2019-10-09 15:24 UTC (permalink / raw)
  To: Rob Herring
  Cc: devicetree, Florian Fainelli, Geert Uytterhoeven, Arnd Bergmann,
	Frank Rowand, linux-kernel, Christoph Hellwig, Marek Vasut,
	Lorenzo Pieralisi, Stefan Wahren, Simon Horman, Robin Murphy,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE


[-- Attachment #1.1: Type: text/plain, Size: 2472 bytes --]

On Tue, 2019-10-08 at 20:03 -0500, Rob Herring wrote:
> On Tue, Oct 8, 2019 at 3:52 PM Nicolas Saenz Julienne
> <nsaenzjulienne@suse.de> wrote:
> > Hi Rob/Robin,
> > 
> > On Tue, 2019-10-08 at 14:52 -0500, Rob Herring wrote:
> > > From: Robin Murphy <robin.murphy@arm.com>
> > > 
> > > Since the "dma-ranges" property is only valid for a node representing a
> > > bus, of_dma_get_range() currently assumes the node passed in is a leaf
> > > representing a device, and starts the walk from its parent. In cases
> > > like PCI host controllers on typical FDT systems, however, where the PCI
> > > endpoints are probed dynamically the initial leaf node represents the
> > > 'bus' itself, and this logic means we fail to consider any "dma-ranges"
> > > describing the host bridge itself. Rework the logic such that
> > > of_dma_get_range() also works correctly starting from a bus node
> > > containing "dma-ranges".
> > > 
> > > While this does mean "dma-ranges" could incorrectly be in a device leaf
> > > node, there isn't really any way in this function to ensure that a leaf
> > > node is or isn't a bus node.
> > 
> > Sorry, I'm not totally sure if this is what you're pointing out with the
> > last
> > sentence. But, what about the case of a bus configuring a device which also
> > happens to be a memory mapped bus (say a PCI platform device). It'll get
> > it's
> > dma config based on its own dma-ranges which is not what we want.
> 
> What I was trying to say is we just can't tell if we should be looking
> in the current node or the parent. 'dma-ranges' in a leaf node can be
> correct or incorrect.
> 
> Your example is a bit different. I'm not sure that case is valid or
> can ever work if it is. It's certainly valid that a PCI bridge's
> parent has dma-ranges and now we'll actually handle any translation.
> The bridge itself is not a DMA-capable device, but just passing thru
> DMA.

Yes, you're right, I hadn't thought of it from that perspective. Thanks!

> Do we ever need to know the parent's dma-ranges in that case? Or
> to put it another way, is looking at anything other than leaf
> dma-ranges useful?

There is no need at all indeed.

With that,

Reviewed-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>

and

Tested-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>

On a Raspberry Pi 4 with pcie-brcmstb.c which is still work in progress but
depends on this.

Regards,
Nicolas


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] of: Make of_dma_get_range() work on bus nodes
  2019-10-09 15:24     ` Nicolas Saenz Julienne
@ 2019-10-09 15:24       ` Nicolas Saenz Julienne
  0 siblings, 0 replies; 7+ messages in thread
From: Nicolas Saenz Julienne @ 2019-10-09 15:24 UTC (permalink / raw)
  To: Rob Herring
  Cc: devicetree, Florian Fainelli, Geert Uytterhoeven, Arnd Bergmann,
	Frank Rowand, linux-kernel, Christoph Hellwig, Marek Vasut,
	Lorenzo Pieralisi, Stefan Wahren, Simon Horman, Robin Murphy,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE


[-- Attachment #1.1: Type: text/plain, Size: 2472 bytes --]

On Tue, 2019-10-08 at 20:03 -0500, Rob Herring wrote:
> On Tue, Oct 8, 2019 at 3:52 PM Nicolas Saenz Julienne
> <nsaenzjulienne@suse.de> wrote:
> > Hi Rob/Robin,
> > 
> > On Tue, 2019-10-08 at 14:52 -0500, Rob Herring wrote:
> > > From: Robin Murphy <robin.murphy@arm.com>
> > > 
> > > Since the "dma-ranges" property is only valid for a node representing a
> > > bus, of_dma_get_range() currently assumes the node passed in is a leaf
> > > representing a device, and starts the walk from its parent. In cases
> > > like PCI host controllers on typical FDT systems, however, where the PCI
> > > endpoints are probed dynamically the initial leaf node represents the
> > > 'bus' itself, and this logic means we fail to consider any "dma-ranges"
> > > describing the host bridge itself. Rework the logic such that
> > > of_dma_get_range() also works correctly starting from a bus node
> > > containing "dma-ranges".
> > > 
> > > While this does mean "dma-ranges" could incorrectly be in a device leaf
> > > node, there isn't really any way in this function to ensure that a leaf
> > > node is or isn't a bus node.
> > 
> > Sorry, I'm not totally sure if this is what you're pointing out with the
> > last
> > sentence. But, what about the case of a bus configuring a device which also
> > happens to be a memory mapped bus (say a PCI platform device). It'll get
> > it's
> > dma config based on its own dma-ranges which is not what we want.
> 
> What I was trying to say is we just can't tell if we should be looking
> in the current node or the parent. 'dma-ranges' in a leaf node can be
> correct or incorrect.
> 
> Your example is a bit different. I'm not sure that case is valid or
> can ever work if it is. It's certainly valid that a PCI bridge's
> parent has dma-ranges and now we'll actually handle any translation.
> The bridge itself is not a DMA-capable device, but just passing thru
> DMA.

Yes, you're right, I hadn't thought of it from that perspective. Thanks!

> Do we ever need to know the parent's dma-ranges in that case? Or
> to put it another way, is looking at anything other than leaf
> dma-ranges useful?

There is no need at all indeed.

With that,

Reviewed-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>

and

Tested-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>

On a Raspberry Pi 4 with pcie-brcmstb.c which is still work in progress but
depends on this.

Regards,
Nicolas


[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2] of: Make of_dma_get_range() work on bus nodes
@ 2019-10-08 19:43 Rob Herring
  0 siblings, 0 replies; 7+ messages in thread
From: Rob Herring @ 2019-10-08 19:43 UTC (permalink / raw)
  To: devicetree; +Cc: Robin Murphy, linux-kernel, linux-arm-kernel

From: Robin Murphy <robin.murphy@arm.com>

Since the "dma-ranges" property is only valid for a node representing a
bus, of_dma_get_range() currently assumes the node passed in is a leaf
representing a device, and starts the walk from its parent. In cases
like PCI host controllers on typical FDT systems, however, where the PCI
endpoints are probed dynamically the initial leaf node represents the
'bus' itself, and this logic means we fail to consider any "dma-ranges"
describing the host bridge itself. Rework the logic such that
of_dma_get_range() also works correctly starting from a bus node
containing "dma-ranges".

While this does mean "dma-ranges" could incorrectly be in a device leaf
node, there isn't really any way in this function to ensure that a leaf
node is or isn't a bus node.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
[robh: Allow for the bus child node to still be passed in]
Signed-off-by: Rob Herring <robh@kernel.org>

---
v2:
 - Ensure once we find dma-ranges, every parent has it.
 - Only get the #{size,address}-cells after we find non-empty dma-ranges
 - Add a check on the 'dma-ranges' length
---
 drivers/of/address.c | 44 ++++++++++++++++++--------------------------
 1 file changed, 18 insertions(+), 26 deletions(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 5ce69d026584..99c1b8058559 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -930,47 +930,39 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *siz
 	const __be32 *ranges = NULL;
 	int len, naddr, nsize, pna;
 	int ret = 0;
+	bool found_dma_ranges = false;
 	u64 dmaaddr;
 
-	if (!node)
-		return -EINVAL;
-
-	while (1) {
-		struct device_node *parent;
-
-		naddr = of_n_addr_cells(node);
-		nsize = of_n_size_cells(node);
-
-		parent = __of_get_dma_parent(node);
-		of_node_put(node);
-
-		node = parent;
-		if (!node)
-			break;
-
+	while (node) {
 		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;
+		/* Once we find 'dma-ranges', then a missing one is an error */
+		if (found_dma_ranges && !ranges) {
+			ret = -ENODEV;
+			goto out;
+		}
+		found_dma_ranges = true;
+
+		node = of_get_next_dma_parent(node);
 	}
 
-	if (!ranges) {
+	if (!node || !ranges) {
 		pr_debug("no dma-ranges found for node(%pOF)\n", np);
 		ret = -ENODEV;
 		goto out;
 	}
 
-	len /= sizeof(u32);
-
+	naddr = of_bus_n_addr_cells(node);
+	nsize = of_bus_n_size_cells(node);
 	pna = of_n_addr_cells(node);
+	if ((len / sizeof(__be32)) % (pna + naddr + nsize)) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	/* dma-ranges format:
 	 * DMA addr	: naddr cells
@@ -978,7 +970,7 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *siz
 	 * size		: nsize cells
 	 */
 	dmaaddr = of_read_number(ranges, naddr);
-	*paddr = of_translate_dma_address(np, ranges);
+	*paddr = of_translate_dma_address(node, ranges + naddr);
 	if (*paddr == OF_BAD_ADDR) {
 		pr_err("translation of DMA address(%llx) to CPU address failed node(%pOF)\n",
 		       dmaaddr, np);
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-10-13  3:57 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-08 19:52 [PATCH v2] of: Make of_dma_get_range() work on bus nodes Rob Herring
2019-10-08 20:51 ` Nicolas Saenz Julienne
2019-10-09  1:03   ` Rob Herring
2019-10-09 15:24     ` Nicolas Saenz Julienne
2019-10-09 15:24       ` Nicolas Saenz Julienne
2019-10-09 10:56 ` Robin Murphy
  -- strict thread matches above, loose matches on Subject: below --
2019-10-08 19:43 Rob Herring

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).