[PATCHv2 0/3] ARM: mvebu: disable I/O coherency on !SMP

[PATCHv2 0/3] ARM: mvebu: disable I/O coherency on !SMP

[Thomas Petazzoni]

Hello,

The hardware I/O coherency feature of Marvell SoC relies on a number
of assumptions that are true only on SMP systems (shareable pages
being used, and cache policy set to write allocate). Making those
assumptions valid for non-SMP systems is challenging, and while the
discussion about solving those challenges is going, many users are
provided with a kernel on which we know the I/O coherency cannot
work. This is especially true for Armada 370 users, as it is a single
core CPU, and therefore, the required assumptions are always false,
even when CONFIG_SMP=y.

This patch series was already proposed in July, but the discussion
about the series itself never took place. Instead, it deviated towards
solving the complex challenge of making the various required
assumptions true on non-SMP systems.

Therefore, I'm proposing again to merge this series today, so that we
provide users with a known-working situation, and work from there
progressively to maybe re-enable hardware I/O coherency in non-SMP
configurations at a later point.

Changes since v1:

 - Rebased on top of v3.18-rc2.
 - Added some Fixes:/Cc: tags on the last patch to get it merged into
   the stable tree, as it fixes a real DT reference count leak.

Thomas Petazzoni (3):
  ARM: mvebu: make the coherency_ll.S functions work with no coherency
    fabric
  ARM: mvebu: disable I/O coherency on non-SMP situations on Armada
    370/XP
  ARM: mvebu: add missing of_node_put() call in coherency.c

 arch/arm/mach-mvebu/coherency.c    | 39 ++++++++++++++++++++++++--------------
 arch/arm/mach-mvebu/coherency_ll.S | 21 ++++++++++++++++++--
 2 files changed, 44 insertions(+), 16 deletions(-)

-- 
2.0.0

[PATCHv2 1/3] ARM: mvebu: make the coherency_ll.S functions work with no coherency fabric

[Thomas Petazzoni]

The ll_add_cpu_to_smp_group(), ll_enable_coherency() and
ll_disable_coherency() are used on Armada XP to control the coherency
fabric. However, they make the assumption that the coherency fabric is
always available, which is currently a correct assumption but will no
longer be true with a followup commit that disables the usage of the
coherency fabric when the conditions are not met to use it.

Therefore, this commit modifies those functions so that they check the
return value of ll_get_coherency_base(), and if the return value is 0,
they simply return without configuring anything in the coherency
fabric.

The ll_get_coherency_base() function is also modified to properly
return 0 when the function is called with the MMU disabled. In this
case, it normally returns the physical address of the coherency
fabric, but we now check if the virtual address is 0, and if that's
case, return a physical address of 0 to indicate that the coherency
fabric is not enabled.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: <stable@vger.kernel.org> # v3.8+
---
 arch/arm/mach-mvebu/coherency_ll.S | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-mvebu/coherency_ll.S b/arch/arm/mach-mvebu/coherency_ll.S
index f5d881b..0ed07c3 100644
--- a/arch/arm/mach-mvebu/coherency_ll.S
+++ b/arch/arm/mach-mvebu/coherency_ll.S
@@ -24,7 +24,10 @@
 #include <asm/cp15.h>
 
 	.text
-/* Returns the coherency base address in r1 (r0 is untouched) */
+/*
+ * Returns the coherency base address in r1 (r0 is untouched), or 0 if
+ * the coherency fabric is not enabled.
+ */
 ENTRY(ll_get_coherency_base)
 	mrc	p15, 0, r1, c1, c0, 0
 	tst	r1, #CR_M @ Check MMU bit enabled
@@ -32,8 +35,13 @@ ENTRY(ll_get_coherency_base)
 
 	/*
 	 * MMU is disabled, use the physical address of the coherency
-	 * base address.
+	 * base address. However, if the coherency fabric isn't mapped
+	 * (i.e its virtual address is zero), it means coherency is
+	 * not enabled, so we return 0.
 	 */
+	ldr	r1, =coherency_base
+	cmp	r1, #0
+	beq	2f
 	adr	r1, 3f
 	ldr	r3, [r1]
 	ldr	r1, [r1, r3]
@@ -85,6 +93,9 @@ ENTRY(ll_add_cpu_to_smp_group)
 	 */
 	mov 	r0, lr
 	bl	ll_get_coherency_base
+	/* Bail out if the coherency is not enabled */
+	cmp	r1, #0
+	moveq	pc, r0
 	bl	ll_get_coherency_cpumask
 	mov 	lr, r0
 	add	r0, r1, #ARMADA_XP_CFB_CFG_REG_OFFSET
@@ -107,6 +118,9 @@ ENTRY(ll_enable_coherency)
 	 */
 	mov r0, lr
 	bl	ll_get_coherency_base
+	/* Bail out if the coherency is not enabled */
+	cmp	r1, #0
+	moveq	pc, r0
 	bl	ll_get_coherency_cpumask
 	mov lr, r0
 	add	r0, r1, #ARMADA_XP_CFB_CTL_REG_OFFSET
@@ -131,6 +145,9 @@ ENTRY(ll_disable_coherency)
 	 */
 	mov 	r0, lr
 	bl	ll_get_coherency_base
+	/* Bail out if the coherency is not enabled */
+	cmp	r1, #0
+	moveq	pc, r0
 	bl	ll_get_coherency_cpumask
 	mov 	lr, r0
 	add	r0, r1, #ARMADA_XP_CFB_CTL_REG_OFFSET
-- 
2.0.0

[PATCHv2 1/3] ARM: mvebu: make the coherency_ll.S functions work with no coherency fabric

[Gregory CLEMENT]

Hi Thomas,

On 27/10/2014 16:32, Thomas Petazzoni wrote:
> The ll_add_cpu_to_smp_group(), ll_enable_coherency() and
> ll_disable_coherency() are used on Armada XP to control the coherency
> fabric. However, they make the assumption that the coherency fabric is
> always available, which is currently a correct assumption but will no
> longer be true with a followup commit that disables the usage of the
> coherency fabric when the conditions are not met to use it.
> 
> Therefore, this commit modifies those functions so that they check the
> return value of ll_get_coherency_base(), and if the return value is 0,
> they simply return without configuring anything in the coherency
> fabric.
> 
> The ll_get_coherency_base() function is also modified to properly
> return 0 when the function is called with the MMU disabled. In this
> case, it normally returns the physical address of the coherency
> fabric, but we now check if the virtual address is 0, and if that's
> case, return a physical address of 0 to indicate that the coherency
> fabric is not enabled.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: <stable@vger.kernel.org> # v3.8+
> ---
>  arch/arm/mach-mvebu/coherency_ll.S | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-mvebu/coherency_ll.S b/arch/arm/mach-mvebu/coherency_ll.S
> index f5d881b..0ed07c3 100644
> --- a/arch/arm/mach-mvebu/coherency_ll.S
> +++ b/arch/arm/mach-mvebu/coherency_ll.S
> @@ -24,7 +24,10 @@
>  #include <asm/cp15.h>
>  
>  	.text
> -/* Returns the coherency base address in r1 (r0 is untouched) */
> +/*
> + * Returns the coherency base address in r1 (r0 is untouched), or 0 if
> + * the coherency fabric is not enabled.
> + */
>  ENTRY(ll_get_coherency_base)
>  	mrc	p15, 0, r1, c1, c0, 0
>  	tst	r1, #CR_M @ Check MMU bit enabled
> @@ -32,8 +35,13 @@ ENTRY(ll_get_coherency_base)
>  
>  	/*
>  	 * MMU is disabled, use the physical address of the coherency
> -	 * base address.
> +	 * base address. However, if the coherency fabric isn't mapped
> +	 * (i.e its virtual address is zero), it means coherency is
> +	 * not enabled, so we return 0.
>  	 */
> +	ldr	r1, =coherency_base
> +	cmp	r1, #0
> +	beq	2f
>  	adr	r1, 3f
>  	ldr	r3, [r1]
>  	ldr	r1, [r1, r3]
> @@ -85,6 +93,9 @@ ENTRY(ll_add_cpu_to_smp_group)
>  	 */
>  	mov 	r0, lr
>  	bl	ll_get_coherency_base
> +	/* Bail out if the coherency is not enabled */
> +	cmp	r1, #0
> +	moveq	pc, r0

Since the commit "6ebbf2ce437b33022d30badd49dc94d33ecfa498:
ARM: convert all "mov.* pc, reg" to "bx reg" for ARMv6+", you should
use "reteq r0" instead of "moveq	pc, r0".

>  	bl	ll_get_coherency_cpumask
>  	mov 	lr, r0
>  	add	r0, r1, #ARMADA_XP_CFB_CFG_REG_OFFSET
> @@ -107,6 +118,9 @@ ENTRY(ll_enable_coherency)
>  	 */
>  	mov r0, lr
>  	bl	ll_get_coherency_base
> +	/* Bail out if the coherency is not enabled */
> +	cmp	r1, #0
> +	moveq	pc, r0

Same here

>  	bl	ll_get_coherency_cpumask
>  	mov lr, r0
>  	add	r0, r1, #ARMADA_XP_CFB_CTL_REG_OFFSET
> @@ -131,6 +145,9 @@ ENTRY(ll_disable_coherency)
>  	 */
>  	mov 	r0, lr
>  	bl	ll_get_coherency_base
> +	/* Bail out if the coherency is not enabled */
> +	cmp	r1, #0
> +	moveq	pc, r0

and same here

>  	bl	ll_get_coherency_cpumask
>  	mov 	lr, r0
>  	add	r0, r1, #ARMADA_XP_CFB_CTL_REG_OFFSET
> 

Besides this small changes:


Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>


Thanks,

Gregory


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCHv2 2/3] ARM: mvebu: disable I/O coherency on non-SMP situations on Armada 370/XP

[Thomas Petazzoni]

Enabling the hardware I/O coherency on Armada 370 and Armada XP
requires a certain number of conditions:

 - On Armada 370, the cache policy must be set to write-allocate.
 - On Armada XP, the cache policy must be set to write-allocate, the
   pages must be mapped with the shareable attribute, and the SMP bit
   must be set

Currently, on Armada XP, when CONFIG_SMP is enabled, those conditions
are met. However, when Armada XP is used in a !CONFIG_SMP kernel, none
of these conditions are met. With Armada 370, the situation is worse:
since the processor is single core, regardless of whether CONFIG_SMP
or !CONFIG_SMP is used, the cache policy will be set to write-back by
the kernel and not write-allocate.

Since solving this problem turns out to be quite complicated, and we
don't want to let users with a mainline kernel known to have
infrequent but existing data corruptions, this commit proposes to
simply disable hardware I/O coherency in situations where it is known
not to work.

And basically, the is_smp() function of the kernel tells us whether it
is OK to enable hardware I/O coherency or not, so this commit slightly
refactors the coherency_type() function to return
COHERENCY_FABRIC_TYPE_NONE when is_smp() is false, or the appropriate
type of the coherency fabric in the other case.

Thanks to this, the I/O coherency fabric will no longer be used at all
in !CONFIG_SMP configurations. It will continue to be used in
CONFIG_SMP configurations on Armada XP, Armada 375 and Armada 38x
(which are multiple cores processors), but will no longer be used on
Armada 370 (which is a single core processor).

In the process, it simplifies the implementation of the
coherency_type() function, and adds a missing call to of_node_put().

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Fixes: e60304f8cb7bb545e79fe62d9b9762460c254ec2 ("arm: mvebu: Add hardware I/O Coherency support")
Cc: <stable@vger.kernel.org> # v3.8+
---
 arch/arm/mach-mvebu/coherency.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/arch/arm/mach-mvebu/coherency.c b/arch/arm/mach-mvebu/coherency.c
index 2bdc323..abf4535 100644
--- a/arch/arm/mach-mvebu/coherency.c
+++ b/arch/arm/mach-mvebu/coherency.c
@@ -361,25 +361,34 @@ static int coherency_type(void)
 {
 	struct device_node *np;
 	const struct of_device_id *match;
+	int type;
 
-	np = of_find_matching_node_and_match(NULL, of_coherency_table, &match);
-	if (np) {
-		int type = (int) match->data;
+	/*
+	 * The coherency fabric is needed:
+	 * - For coherency between processors on Armada XP, so only
+	 *   when SMP is enabled.
+	 * - For coherency between the processor and I/O devices, but
+	 *   this coherency requires many pre-requisites (write
+	 *   allocate cache policy, shareable pages, SMP bit set) that
+	 *   are only meant in SMP situations.
+	 *
+	 * Note that this means that on Armada 370, there is currently
+	 * no way to use hardware I/O coherency, because even when
+	 * CONFIG_SMP is enabled, is_smp() returns false due to the
+	 * Armada 370 being a single-core processor.
+	 */
+	if (!is_smp())
+		return COHERENCY_FABRIC_TYPE_NONE;
 
-		/* Armada 370/XP coherency works in both UP and SMP */
-		if (type == COHERENCY_FABRIC_TYPE_ARMADA_370_XP)
-			return type;
+	np = of_find_matching_node_and_match(NULL, of_coherency_table, &match);
+	if (!np)
+		return COHERENCY_FABRIC_TYPE_NONE;
 
-		/* Armada 375 coherency works only on SMP */
-		else if (type == COHERENCY_FABRIC_TYPE_ARMADA_375 && is_smp())
-			return type;
+	type = (int) match->data;
 
-		/* Armada 380 coherency works only on SMP */
-		else if (type == COHERENCY_FABRIC_TYPE_ARMADA_380 && is_smp())
-			return type;
-	}
+	of_node_put(np);
 
-	return COHERENCY_FABRIC_TYPE_NONE;
+	return type;
 }
 
 int coherency_available(void)
-- 
2.0.0

[PATCHv2 2/3] ARM: mvebu: disable I/O coherency on non-SMP situations on Armada 370/XP

[Gregory CLEMENT]

Hi Thomas,

I agree that providing a kernel working reliably is important.

On 27/10/2014 16:32, Thomas Petazzoni wrote:
> Enabling the hardware I/O coherency on Armada 370 and Armada XP
> requires a certain number of conditions:
> 
>  - On Armada 370, the cache policy must be set to write-allocate.
>  - On Armada XP, the cache policy must be set to write-allocate, the
>    pages must be mapped with the shareable attribute, and the SMP bit
>    must be set
> 
> Currently, on Armada XP, when CONFIG_SMP is enabled, those conditions
> are met. However, when Armada XP is used in a !CONFIG_SMP kernel, none
> of these conditions are met. With Armada 370, the situation is worse:
> since the processor is single core, regardless of whether CONFIG_SMP
> or !CONFIG_SMP is used, the cache policy will be set to write-back by
> the kernel and not write-allocate.
> 
> Since solving this problem turns out to be quite complicated, and we
> don't want to let users with a mainline kernel known to have
> infrequent but existing data corruptions, this commit proposes to
> simply disable hardware I/O coherency in situations where it is known
> not to work.
> 
> And basically, the is_smp() function of the kernel tells us whether it
> is OK to enable hardware I/O coherency or not, so this commit slightly
> refactors the coherency_type() function to return
> COHERENCY_FABRIC_TYPE_NONE when is_smp() is false, or the appropriate
> type of the coherency fabric in the other case.
> 
> Thanks to this, the I/O coherency fabric will no longer be used at all
> in !CONFIG_SMP configurations. It will continue to be used in
> CONFIG_SMP configurations on Armada XP, Armada 375 and Armada 38x
> (which are multiple cores processors), but will no longer be used on
> Armada 370 (which is a single core processor).
> 
> In the process, it simplifies the implementation of the
> coherency_type() function, and adds a missing call to of_node_put().
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Fixes: e60304f8cb7bb545e79fe62d9b9762460c254ec2 ("arm: mvebu: Add hardware I/O Coherency support")
> Cc: <stable@vger.kernel.org> # v3.8+


Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>


Thanks,

Gregory



-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCHv2 2/3] ARM: mvebu: disable I/O coherency on non-SMP situations on Armada 370/XP

[Jason Cooper]

On Mon, Oct 27, 2014 at 04:32:34PM +0100, Thomas Petazzoni wrote:
> Enabling the hardware I/O coherency on Armada 370 and Armada XP
> requires a certain number of conditions:
> 
>  - On Armada 370, the cache policy must be set to write-allocate.
>  - On Armada XP, the cache policy must be set to write-allocate, the
>    pages must be mapped with the shareable attribute, and the SMP bit
>    must be set
> 
> Currently, on Armada XP, when CONFIG_SMP is enabled, those conditions
> are met. However, when Armada XP is used in a !CONFIG_SMP kernel, none
> of these conditions are met. With Armada 370, the situation is worse:
> since the processor is single core, regardless of whether CONFIG_SMP
> or !CONFIG_SMP is used, the cache policy will be set to write-back by
> the kernel and not write-allocate.
> 
> Since solving this problem turns out to be quite complicated, and we
> don't want to let users with a mainline kernel known to have
> infrequent but existing data corruptions, this commit proposes to
> simply disable hardware I/O coherency in situations where it is known
> not to work.
> 
> And basically, the is_smp() function of the kernel tells us whether it
> is OK to enable hardware I/O coherency or not, so this commit slightly
> refactors the coherency_type() function to return
> COHERENCY_FABRIC_TYPE_NONE when is_smp() is false, or the appropriate
> type of the coherency fabric in the other case.
> 
> Thanks to this, the I/O coherency fabric will no longer be used at all
> in !CONFIG_SMP configurations. It will continue to be used in
> CONFIG_SMP configurations on Armada XP, Armada 375 and Armada 38x
> (which are multiple cores processors), but will no longer be used on
> Armada 370 (which is a single core processor).
> 
> In the process, it simplifies the implementation of the
> coherency_type() function, and adds a missing call to of_node_put().
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Fixes: e60304f8cb7bb545e79fe62d9b9762460c254ec2 ("arm: mvebu: Add hardware I/O Coherency support")
> Cc: <stable@vger.kernel.org> # v3.8+
> ---
>  arch/arm/mach-mvebu/coherency.c | 37 +++++++++++++++++++++++--------------
>  1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm/mach-mvebu/coherency.c b/arch/arm/mach-mvebu/coherency.c
> index 2bdc323..abf4535 100644
> --- a/arch/arm/mach-mvebu/coherency.c
> +++ b/arch/arm/mach-mvebu/coherency.c
> @@ -361,25 +361,34 @@ static int coherency_type(void)
>  {
>  	struct device_node *np;
>  	const struct of_device_id *match;
> +	int type;
>  
> -	np = of_find_matching_node_and_match(NULL, of_coherency_table, &match);
> -	if (np) {
> -		int type = (int) match->data;
> +	/*
> +	 * The coherency fabric is needed:
> +	 * - For coherency between processors on Armada XP, so only
> +	 *   when SMP is enabled.
> +	 * - For coherency between the processor and I/O devices, but
> +	 *   this coherency requires many pre-requisites (write
> +	 *   allocate cache policy, shareable pages, SMP bit set) that
> +	 *   are only meant in SMP situations.
> +	 *
> +	 * Note that this means that on Armada 370, there is currently
> +	 * no way to use hardware I/O coherency, because even when
> +	 * CONFIG_SMP is enabled, is_smp() returns false due to the
> +	 * Armada 370 being a single-core processor.

Could we extend this a bit to let future readers know what needs to
change to re-enable support on the 370?

thx,

Jason.

> +	 */
> +	if (!is_smp())
> +		return COHERENCY_FABRIC_TYPE_NONE;
>  
> -		/* Armada 370/XP coherency works in both UP and SMP */
> -		if (type == COHERENCY_FABRIC_TYPE_ARMADA_370_XP)
> -			return type;
> +	np = of_find_matching_node_and_match(NULL, of_coherency_table, &match);
> +	if (!np)
> +		return COHERENCY_FABRIC_TYPE_NONE;
>  
> -		/* Armada 375 coherency works only on SMP */
> -		else if (type == COHERENCY_FABRIC_TYPE_ARMADA_375 && is_smp())
> -			return type;
> +	type = (int) match->data;
>  
> -		/* Armada 380 coherency works only on SMP */
> -		else if (type == COHERENCY_FABRIC_TYPE_ARMADA_380 && is_smp())
> -			return type;
> -	}
> +	of_node_put(np);
>  
> -	return COHERENCY_FABRIC_TYPE_NONE;
> +	return type;
>  }
>  
>  int coherency_available(void)
> -- 
> 2.0.0
> 

[PATCHv2 3/3] ARM: mvebu: add missing of_node_put() call in coherency.c

[Thomas Petazzoni]

There is a missing of_node_put() to decrement the device_node
reference counter after a of_find_matching_node() in coherency_init().

Fixes: 501f928e0097 ("ARM: mvebu: add a coherency_available() call")
Cc: <stable@vger.kernel.org> # v3.16+
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/mach-mvebu/coherency.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/mach-mvebu/coherency.c b/arch/arm/mach-mvebu/coherency.c
index abf4535..ee44965 100644
--- a/arch/arm/mach-mvebu/coherency.c
+++ b/arch/arm/mach-mvebu/coherency.c
@@ -409,6 +409,8 @@ int __init coherency_init(void)
 		 type == COHERENCY_FABRIC_TYPE_ARMADA_380)
 		armada_375_380_coherency_init(np);
 
+	of_node_put(np);
+
 	return 0;
 }
 
-- 
2.0.0

[PATCHv2 3/3] ARM: mvebu: add missing of_node_put() call in coherency.c

[Gregory CLEMENT]

Hi Thomas,
On 27/10/2014 16:32, Thomas Petazzoni wrote:
> There is a missing of_node_put() to decrement the device_node
> reference counter after a of_find_matching_node() in coherency_init().

Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>


Thanks,

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCHv2 3/3] ARM: mvebu: add missing of_node_put() call in coherency.c

[Jason Cooper]

On Mon, Oct 27, 2014 at 04:32:35PM +0100, Thomas Petazzoni wrote:
> There is a missing of_node_put() to decrement the device_node
> reference counter after a of_find_matching_node() in coherency_init().
> 
> Fixes: 501f928e0097 ("ARM: mvebu: add a coherency_available() call")
> Cc: <stable@vger.kernel.org> # v3.16+
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  arch/arm/mach-mvebu/coherency.c | 2 ++
>  1 file changed, 2 insertions(+)

Applied to mvebu/fixes with Gregory's Ack.  Thanks for doing the Fixes
and stable tags!

thx,

Jason.