[PATCH master] dma: correctly honour dma-noncoherent device tree property

[PATCH master] dma: correctly honour dma-noncoherent device tree property

[Ahmad Fatoum]

Most barebox platforms are either completely cache-coherent with respect to
DMA or completely non-coherent. To avoid having to walk the device tree for
non-existent dma-coherent and dma-noncohrent properties, barebox thus
only does this when CONFIG_OF_DMA_COHERENCY is selected.

CONFIG_OF_DMA_COHERENCY is to be selected by platforms that have mixed
coherency for DMA masters. Forgetting to select this option can be
annoying to debug, which is why devinfo will inform the user of default
DMA coherency assignments:

  DMA Coherent: false (default)

In order to allow devinfo to differentiate implicit default and explicit
device tree DMA coherency configuration, the struct device::dma_coherent
member is not of boolean type, but Instead an enumeration that besides
coherent and non-coherent also encodes a DEV_DMA_COHERENCE_DEFAULT state.

In practice though, a boolean was saved in this dma_coherent member,
leading to any dma-noncoherent property being ignored and the default
coherency setting being taken for the node.

By fixing the type confusion, we restore working Ethernet on the
StarFive JH7100, which is the only SoC we currently support that
requires /soc/dma-noncohrent to be set for proper operation of its DMA
masters.

Fixes: fbdea8fd54fe ("of: populate new device_d::dma_coherent attribute")
Reported-by: Antony Pavlov <antonynpavlov@gmail.com>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/of/platform.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index ec1482b27797..e2c252b1ffee 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -90,6 +90,21 @@ static struct device_node *of_get_next_dma_parent(const struct device_node *np)
 	return args.np;
 }
 
+static enum dev_dma_coherence of_dma_get_coherence(struct device_node *node)
+{
+	if (IS_ENABLED(CONFIG_OF_DMA_COHERENCY)) {
+		while (node) {
+			if (of_property_read_bool(node, "dma-coherent"))
+				return DEV_DMA_COHERENT;
+			if (of_property_read_bool(node, "dma-noncoherent"))
+				return DEV_DMA_NON_COHERENT;
+			node = of_get_next_dma_parent(node);
+		}
+	}
+
+	return DEV_DMA_COHERENCE_DEFAULT;
+}
+
 /**
  * of_dma_is_coherent - Check if device is coherent
  * @np:	device node
@@ -101,17 +116,14 @@ static struct device_node *of_get_next_dma_parent(const struct device_node *np)
  */
 bool of_dma_is_coherent(struct device_node *node)
 {
-	if (IS_ENABLED(CONFIG_OF_DMA_COHERENCY)) {
-		while (node) {
-			if (of_property_read_bool(node, "dma-coherent"))
-				return true;
-			if (of_property_read_bool(node, "dma-noncoherent"))
-				return false;
-			node = of_get_next_dma_parent(node);
-		}
+	switch (of_dma_get_coherence(node)) {
+	case DEV_DMA_COHERENT:
+		return true;
+	case DEV_DMA_NON_COHERENT:
+		return false;
+	case DEV_DMA_COHERENCE_DEFAULT:
+		return IS_ENABLED(CONFIG_ARCH_DMA_DEFAULT_COHERENT);
 	}
-
-	return IS_ENABLED(CONFIG_ARCH_DMA_DEFAULT_COHERENT);
 }
 EXPORT_SYMBOL_GPL(of_dma_is_coherent);
 
@@ -129,7 +141,7 @@ static void of_dma_configure(struct device *dev, struct device_node *np)
 	}
 
 	dev->dma_offset = offset;
-	dev->dma_coherent = of_dma_is_coherent(np);
+	dev->dma_coherent = of_dma_get_coherence(np);
 }
 
 /**
-- 
2.39.2

Re: [PATCH master] dma: correctly honour dma-noncoherent device tree property

[Sascha Hauer]

On Fri, 22 Mar 2024 15:24:24 +0100, Ahmad Fatoum wrote:
> Most barebox platforms are either completely cache-coherent with respect to
> DMA or completely non-coherent. To avoid having to walk the device tree for
> non-existent dma-coherent and dma-noncohrent properties, barebox thus
> only does this when CONFIG_OF_DMA_COHERENCY is selected.
> 
> CONFIG_OF_DMA_COHERENCY is to be selected by platforms that have mixed
> coherency for DMA masters. Forgetting to select this option can be
> annoying to debug, which is why devinfo will inform the user of default
> DMA coherency assignments:
> 
> [...]

Applied, thanks!

[1/1] dma: correctly honour dma-noncoherent device tree property
      https://git.pengutronix.de/cgit/barebox/commit/?id=9caee28f7130 (link may not be stable)

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>

Re: [PATCH master] dma: correctly honour dma-noncoherent device tree property

[Sascha Hauer]

On Fri, Mar 22, 2024 at 03:24:24PM +0100, Ahmad Fatoum wrote:
> Most barebox platforms are either completely cache-coherent with respect to
> DMA or completely non-coherent. To avoid having to walk the device tree for
> non-existent dma-coherent and dma-noncohrent properties, barebox thus
> only does this when CONFIG_OF_DMA_COHERENCY is selected.
> 
> CONFIG_OF_DMA_COHERENCY is to be selected by platforms that have mixed
> coherency for DMA masters. Forgetting to select this option can be
> annoying to debug, which is why devinfo will inform the user of default
> DMA coherency assignments:
> 
>   DMA Coherent: false (default)
> 
> In order to allow devinfo to differentiate implicit default and explicit
> device tree DMA coherency configuration, the struct device::dma_coherent
> member is not of boolean type, but Instead an enumeration that besides
> coherent and non-coherent also encodes a DEV_DMA_COHERENCE_DEFAULT state.
> 
> In practice though, a boolean was saved in this dma_coherent member,
> leading to any dma-noncoherent property being ignored and the default
> coherency setting being taken for the node.
> 
> By fixing the type confusion, we restore working Ethernet on the
> StarFive JH7100, which is the only SoC we currently support that
> requires /soc/dma-noncohrent to be set for proper operation of its DMA
> masters.
> 
> Fixes: fbdea8fd54fe ("of: populate new device_d::dma_coherent attribute")
> Reported-by: Antony Pavlov <antonynpavlov@gmail.com>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  drivers/of/platform.c | 34 +++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index ec1482b27797..e2c252b1ffee 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -90,6 +90,21 @@ static struct device_node *of_get_next_dma_parent(const struct device_node *np)
>  	return args.np;
>  }
>  
> +static enum dev_dma_coherence of_dma_get_coherence(struct device_node *node)
> +{
> +	if (IS_ENABLED(CONFIG_OF_DMA_COHERENCY)) {
> +		while (node) {
> +			if (of_property_read_bool(node, "dma-coherent"))
> +				return DEV_DMA_COHERENT;
> +			if (of_property_read_bool(node, "dma-noncoherent"))
> +				return DEV_DMA_NON_COHERENT;
> +			node = of_get_next_dma_parent(node);
> +		}
> +	}
> +
> +	return DEV_DMA_COHERENCE_DEFAULT;
> +}
> +
>  /**
>   * of_dma_is_coherent - Check if device is coherent
>   * @np:	device node
> @@ -101,17 +116,14 @@ static struct device_node *of_get_next_dma_parent(const struct device_node *np)
>   */
>  bool of_dma_is_coherent(struct device_node *node)
>  {
> -	if (IS_ENABLED(CONFIG_OF_DMA_COHERENCY)) {
> -		while (node) {
> -			if (of_property_read_bool(node, "dma-coherent"))
> -				return true;
> -			if (of_property_read_bool(node, "dma-noncoherent"))
> -				return false;
> -			node = of_get_next_dma_parent(node);
> -		}
> +	switch (of_dma_get_coherence(node)) {
> +	case DEV_DMA_COHERENT:
> +		return true;
> +	case DEV_DMA_NON_COHERENT:
> +		return false;
> +	case DEV_DMA_COHERENCE_DEFAULT:
> +		return IS_ENABLED(CONFIG_ARCH_DMA_DEFAULT_COHERENT);
>  	}
> -
> -	return IS_ENABLED(CONFIG_ARCH_DMA_DEFAULT_COHERENT);
>  }

This brings us:

drivers/of/platform.c: In function 'of_dma_is_coherent':
drivers/of/platform.c:127:1: warning: control reaches end of non-void function [-Wreturn-type]

what shall we return in the default case? also
IS_ENABLED(CONFIG_ARCH_DMA_DEFAULT_COHERENT)?

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH master] dma: correctly honour dma-noncoherent device tree property

[Ahmad Fatoum]

On 26.03.24 11:33, Sascha Hauer wrote:
> On Fri, Mar 22, 2024 at 03:24:24PM +0100, Ahmad Fatoum wrote:
>> Most barebox platforms are either completely cache-coherent with respect to
>> DMA or completely non-coherent. To avoid having to walk the device tree for
>> non-existent dma-coherent and dma-noncohrent properties, barebox thus
>> only does this when CONFIG_OF_DMA_COHERENCY is selected.
>>
>> CONFIG_OF_DMA_COHERENCY is to be selected by platforms that have mixed
>> coherency for DMA masters. Forgetting to select this option can be
>> annoying to debug, which is why devinfo will inform the user of default
>> DMA coherency assignments:
>>
>>   DMA Coherent: false (default)
>>
>> In order to allow devinfo to differentiate implicit default and explicit
>> device tree DMA coherency configuration, the struct device::dma_coherent
>> member is not of boolean type, but Instead an enumeration that besides
>> coherent and non-coherent also encodes a DEV_DMA_COHERENCE_DEFAULT state.
>>
>> In practice though, a boolean was saved in this dma_coherent member,
>> leading to any dma-noncoherent property being ignored and the default
>> coherency setting being taken for the node.
>>
>> By fixing the type confusion, we restore working Ethernet on the
>> StarFive JH7100, which is the only SoC we currently support that
>> requires /soc/dma-noncohrent to be set for proper operation of its DMA
>> masters.
>>
>> Fixes: fbdea8fd54fe ("of: populate new device_d::dma_coherent attribute")
>> Reported-by: Antony Pavlov <antonynpavlov@gmail.com>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  drivers/of/platform.c | 34 +++++++++++++++++++++++-----------
>>  1 file changed, 23 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index ec1482b27797..e2c252b1ffee 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -90,6 +90,21 @@ static struct device_node *of_get_next_dma_parent(const struct device_node *np)
>>  	return args.np;
>>  }
>>  
>> +static enum dev_dma_coherence of_dma_get_coherence(struct device_node *node)
>> +{
>> +	if (IS_ENABLED(CONFIG_OF_DMA_COHERENCY)) {
>> +		while (node) {
>> +			if (of_property_read_bool(node, "dma-coherent"))
>> +				return DEV_DMA_COHERENT;
>> +			if (of_property_read_bool(node, "dma-noncoherent"))
>> +				return DEV_DMA_NON_COHERENT;
>> +			node = of_get_next_dma_parent(node);
>> +		}
>> +	}
>> +
>> +	return DEV_DMA_COHERENCE_DEFAULT;
>> +}
>> +
>>  /**
>>   * of_dma_is_coherent - Check if device is coherent
>>   * @np:	device node
>> @@ -101,17 +116,14 @@ static struct device_node *of_get_next_dma_parent(const struct device_node *np)
>>   */
>>  bool of_dma_is_coherent(struct device_node *node)
>>  {
>> -	if (IS_ENABLED(CONFIG_OF_DMA_COHERENCY)) {
>> -		while (node) {
>> -			if (of_property_read_bool(node, "dma-coherent"))
>> -				return true;
>> -			if (of_property_read_bool(node, "dma-noncoherent"))
>> -				return false;
>> -			node = of_get_next_dma_parent(node);
>> -		}
>> +	switch (of_dma_get_coherence(node)) {
>> +	case DEV_DMA_COHERENT:
>> +		return true;
>> +	case DEV_DMA_NON_COHERENT:
>> +		return false;
>> +	case DEV_DMA_COHERENCE_DEFAULT:
>> +		return IS_ENABLED(CONFIG_ARCH_DMA_DEFAULT_COHERENT);
>>  	}
>> -
>> -	return IS_ENABLED(CONFIG_ARCH_DMA_DEFAULT_COHERENT);
>>  }
> 
> This brings us:
> 
> drivers/of/platform.c: In function 'of_dma_is_coherent':
> drivers/of/platform.c:127:1: warning: control reaches end of non-void function [-Wreturn-type]
> 
> what shall we return in the default case? also
> IS_ENABLED(CONFIG_ARCH_DMA_DEFAULT_COHERENT)?

Yes. Either that or BUG()/__builtin_unreachable();

Thanks,
Ahmad

> 
> Sascha
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |