arm1136: cosmetic: Remove double test on CONFIG_SYS_DCACHE_OFF

Remove a redundant '#ifndef CONFIG_SYS_DCACHE_OFF' nested in the same #ifndef.

Signed-off-by Beno?t Th?baudeau <[email protected]>

Cc Albert Aribaud <[email protected]>

---
 .../arch/arm/cpu/arm1136/cpu.c                     |    2 --
 1 file changed, 2 deletions(-)

arch/arm/cpu/arm1136/cpu.c +0 -0

index b98e3d9..1136c1d 100644

Hi Beno?t,

Quoted 15 lines · Beno?t Th?baudeau

Remove a redundant '#ifndef CONFIG_SYS_DCACHE_OFF' nested in the same #ifndef.

Signed-off-by Beno?t Th?baudeau <[email protected]>

Cc Albert Aribaud <[email protected]>

---
 .../arch/arm/cpu/arm1136/cpu.c                     |    2 --
 1 file changed, 2 deletions(-)

(quoted diff) +0 -0

u-boot-4d3c95f/arch/arm/cpu/arm1136/cpu.c
index b98e3d9..1136c1d 100644

arch/arm/cpu/arm1136/cpu.c void enable_caches(void) · line 152

Quoted 9 lines

arch/arm/cpu/arm1136/cpu.c +0 -2

@@ -146,9 +146,7 @@ void enable_caches(void)
 #ifndef CONFIG_SYS_ICACHE_OFF
 	icache_enable();
 #endif
-#ifndef CONFIG_SYS_DCACHE_OFF
 	dcache_enable();
-#endif
 }
 
 #else /* #ifndef CONFIG_SYS_DCACHE_OFF */

I'll NAK this one because:

1) obviously the big #ifndef CONFIG_SYS_DCACHE_OFF / #else /#endif is there to provide either working D$ functions or empty ones;

2) enable_caches() exists only in the "then" branch, not at all in the
"else" branch, which makes it a surprising exception;

3) enable_caches() is the only function in the if/then/else acting on I$
as well as D$;

... so I suspect it did not actually belong in the big if/then/else in
the first place and should not be modified but moved after the #endif.

Please provide a V2 patch, I promise it'll get processed faster than this V1.

Hi Albert,

Quoted 39 lines · Albert ARIBAUD

Hi Beno?t,

Quoted 15 lines · Beno?t Th?baudeau

Remove a redundant '#ifndef CONFIG_SYS_DCACHE_OFF' nested in the same #ifndef.

Signed-off-by Beno?t Th?baudeau <[email protected]>

Cc Albert Aribaud <[email protected]>

---
 .../arch/arm/cpu/arm1136/cpu.c                     |    2 --
 1 file changed, 2 deletions(-)

(quoted diff) +0 -0

u-boot-4d3c95f/arch/arm/cpu/arm1136/cpu.c
index b98e3d9..1136c1d 100644

arch/arm/cpu/arm1136/cpu.c void enable_caches(void) · line 152

Quoted 9 lines

arch/arm/cpu/arm1136/cpu.c +0 -2

@@ -146,9 +146,7 @@ void enable_caches(void)
 #ifndef CONFIG_SYS_ICACHE_OFF
 	icache_enable();
 #endif
-#ifndef CONFIG_SYS_DCACHE_OFF
 	dcache_enable();
-#endif
 }
 
 #else /* #ifndef CONFIG_SYS_DCACHE_OFF */

I'll NAK this one because:

1) obviously the big #ifndef CONFIG_SYS_DCACHE_OFF / #else /#endif is there to provide either working D$ functions or empty ones;

2) enable_caches() exists only in the "then" branch, not at all in the
"else" branch, which makes it a surprising exception;

Quoted 8 lines

3) enable_caches() is the only function in the if/then/else acting on I$
as well as D$;

... so I suspect it did not actually belong in the big if/then/else in
the first place and should not be modified but moved after the #endif.

I agree, simply because with the current code, enable_caches() does not enable icache if CONFIG_SYS_ICACHE_OFF is not defined but CONFIG_SYS_DCACHE_OFF is.

But is it enough to move it? We could indeed move it after the #endif, but also change it to:

---
#if !defined(CONFIG_SYS_ICACHE_OFF) || !defined(CONFIG_SYS_DCACHE_OFF) void enable_caches(void)
{

#ifndef CONFIG_SYS_ICACHE_OFF
	icache_enable();
#endif
#ifndef CONFIG_SYS_DCACHE_OFF
	dcache_enable();

#endif
}
#endif
---

In that way, the default __enable_caches() from cache.c (outputting "WARNING: Caches not enabled\n") would be linked if both CONFIG_SYS_ICACHE_OFF and CONFIG_SYS_DCACHE_OFF are defined.

Do you agree?

Quoted 2 lines

Please provide a V2 patch, I promise it'll get processed faster than this V1.

OK ;) , I'll do that after your answer to the question above.

Best regards,
Beno?t

Hi Beno?t,

Quoted 81 lines · Beno?t Th?baudeau

Hi Albert,

Quoted 48 lines · Albert ARIBAUD

Hi Beno?t,

Quoted 17 lines · Beno?t Th?baudeau

Remove a redundant '#ifndef CONFIG_SYS_DCACHE_OFF' nested in the
same #ifndef.

Signed-off-by: Beno?t Th?baudeau
<[email protected]>
Cc: Albert Aribaud <[email protected]>

---
 .../arch/arm/cpu/arm1136/cpu.c                     |    2 --
 1 file changed, 2 deletions(-)

(quoted diff) +0 -0

u-boot-4d3c95f/arch/arm/cpu/arm1136/cpu.c
index b98e3d9..1136c1d 100644

arch/arm/cpu/arm1136/cpu.c void enable_caches(void) · line 152

Quoted 9 lines

arch/arm/cpu/arm1136/cpu.c +0 -2

@@ -146,9 +146,7 @@ void enable_caches(void)
 #ifndef CONFIG_SYS_ICACHE_OFF
 	icache_enable();
 #endif
-#ifndef CONFIG_SYS_DCACHE_OFF
 	dcache_enable();
-#endif
 }
 
 #else /* #ifndef CONFIG_SYS_DCACHE_OFF */

I'll NAK this one because:

1) obviously the big #ifndef CONFIG_SYS_DCACHE_OFF / #else /#endif is
there to provide either working D$ functions or empty ones;

2) enable_caches() exists only in the "then" branch, not at all in
the
"else" branch, which makes it a surprising exception;

3) enable_caches() is the only function in the if/then/else acting on
I$
as well as D$;

Quoted 5 lines

... so I suspect it did not actually belong in the big if/then/else
in
the first place and should not be modified but moved after the #endif.

I agree, simply because with the current code, enable_caches() does not enable
icache if CONFIG_SYS_ICACHE_OFF is not defined but CONFIG_SYS_DCACHE_OFF is.

But is it enough to move it? We could indeed move it after the #endif, but also
change it to:

---
#if !defined(CONFIG_SYS_ICACHE_OFF) ||
!defined(CONFIG_SYS_DCACHE_OFF)
void enable_caches(void)
{

#ifndef CONFIG_SYS_ICACHE_OFF
	icache_enable();
#endif
#ifndef CONFIG_SYS_DCACHE_OFF
	dcache_enable();

#endif
}
#endif
---

Quoted 6 lines

In that way, the default __enable_caches() from cache.c (outputting "WARNING: Caches not enabled\n") would be linked if both CONFIG_SYS_ICACHE_OFF
and CONFIG_SYS_DCACHE_OFF are defined.

Do you agree?

When CONFIG_SYS_DCACHE_OFF, dcache functions are not compiled out, they
are compiled in but empty. IOW, even with caches off, the API remains callable albeit useless. This is done so that client code does not have to test CONFIG_SYS_DCACHE_OFF before deciding to call the API or not.

Therefore, for consistency, enable_caches() should be defined and empty
even when both CONFIG_SYS_DCACHE_OFF and CONFIG_SYS_ICACHE_OFF are defined, which is not the case in the example above due to the enclosing #if/#endif.

Hi Albert,

Quoted 100 lines · Albert ARIBAUD

Hi Beno?t,

Quoted 81 lines · Beno?t Th?baudeau

Hi Albert,

Quoted 48 lines · Albert ARIBAUD

Hi Beno?t,

Quoted 17 lines · Beno?t Th?baudeau

Remove a redundant '#ifndef CONFIG_SYS_DCACHE_OFF' nested in the
same #ifndef.

Signed-off-by: Beno?t Th?baudeau
<[email protected]>
Cc: Albert Aribaud <[email protected]>

---
 .../arch/arm/cpu/arm1136/cpu.c                     |    2 --
 1 file changed, 2 deletions(-)

(quoted diff) +0 -0

u-boot-4d3c95f/arch/arm/cpu/arm1136/cpu.c
index b98e3d9..1136c1d 100644

arch/arm/cpu/arm1136/cpu.c void enable_caches(void) · line 152

Quoted 9 lines

arch/arm/cpu/arm1136/cpu.c +0 -2

@@ -146,9 +146,7 @@ void enable_caches(void)
 #ifndef CONFIG_SYS_ICACHE_OFF
 	icache_enable();
 #endif
-#ifndef CONFIG_SYS_DCACHE_OFF
 	dcache_enable();
-#endif
 }
 
 #else /* #ifndef CONFIG_SYS_DCACHE_OFF */

I'll NAK this one because:

1) obviously the big #ifndef CONFIG_SYS_DCACHE_OFF / #else /#endif is
there to provide either working D$ functions or empty ones;

2) enable_caches() exists only in the "then" branch, not at all in
the
"else" branch, which makes it a surprising exception;

3) enable_caches() is the only function in the if/then/else acting on
I$
as well as D$;

Quoted 5 lines

... so I suspect it did not actually belong in the big if/then/else
in
the first place and should not be modified but moved after the #endif.

I agree, simply because with the current code, enable_caches() does not enable
icache if CONFIG_SYS_ICACHE_OFF is not defined but CONFIG_SYS_DCACHE_OFF is.

But is it enough to move it? We could indeed move it after the #endif, but also
change it to:

---
#if !defined(CONFIG_SYS_ICACHE_OFF) ||
!defined(CONFIG_SYS_DCACHE_OFF)
void enable_caches(void)
{

#ifndef CONFIG_SYS_ICACHE_OFF
	icache_enable();
#endif
#ifndef CONFIG_SYS_DCACHE_OFF
	dcache_enable();

#endif
}
#endif
---

Quoted 6 lines

In that way, the default __enable_caches() from cache.c (outputting "WARNING: Caches not enabled\n") would be linked if both CONFIG_SYS_ICACHE_OFF
and CONFIG_SYS_DCACHE_OFF are defined.

Do you agree?

When CONFIG_SYS_DCACHE_OFF, dcache functions are not compiled out, they
are compiled in but empty. IOW, even with caches off, the API remains callable albeit useless. This is done so that client code does not have to test CONFIG_SYS_DCACHE_OFF before deciding to call the API or not.

Quoted 5 lines

Therefore, for consistency, enable_caches() should be defined and empty
even when both CONFIG_SYS_DCACHE_OFF and CONFIG_SYS_ICACHE_OFF are defined, which is not the case in the example above due to the enclosing #if/#endif.

In the example, enable_caches() is still defined if both CONFIG_SYS_DCACHE_OFF and CONFIG_SYS_ICACHE_OFF are defined. See arch/arm/lib/cache.c:
---
/*
* Default implementation of enable_caches()
* Real implementation should be in platform code
*/
void __enable_caches(void)

{
	puts("WARNING: Caches not enabled\n");
}
void enable_caches(void)
	__attribute__((weak, alias("__enable_caches")));

---

But you're right, it's not empty in this case. Is it that you want to remove this message in this case?

This default implementation is used in the same way for several other ARM targets.

Best regards,
Beno?t

enable_caches() did not enable icache if CONFIG_SYS_ICACHE_OFF was not defined but CONFIG_SYS_DCACHE_OFF was.

Signed-off-by Beno?t Th?baudeau <[email protected]>

Cc Albert Aribaud <[email protected]>

---
This patch supersedes http://patchwork.ozlabs.org/patch/177263/ .

Changes for v2:
- Move enable_caches() outside of the main #if in order to fix this icache issue rather than doing only #if cosmetic cleanup.

 .../arch/arm/cpu/arm1136/cpu.c                     |   22 +++++++++++---------
 1 file changed, 12 insertions(+), 10 deletions(-)

arch/arm/cpu/arm1136/cpu.c +2 -10

index b98e3d9..32a4c24 100644
@@ -141,16 +141,6 @@ void flush_cache(unsigned long start, unsigned long size)
 	flush_dcache_range(start, start + size);
 }
 
-void enable_caches(void)
-{
-#ifndef CONFIG_SYS_ICACHE_OFF
-	icache_enable();
-#endif
-#ifndef CONFIG_SYS_DCACHE_OFF
-	dcache_enable();
-#endif
-}
-
 #else /* #ifndef CONFIG_SYS_DCACHE_OFF */
 void invalidate_dcache_all(void)
 {

@@ -172,3 +162,15 @@ void flush_cache(unsigned long start, unsigned long size)
 {
 }
 #endif /* #ifndef CONFIG_SYS_DCACHE_OFF */
+
+#if !defined(CONFIG_SYS_ICACHE_OFF) || !defined(CONFIG_SYS_DCACHE_OFF)

Quoted 54 lines · Beno?t Th?baudeau

enable_caches() did not enable icache if CONFIG_SYS_ICACHE_OFF was not defined
but CONFIG_SYS_DCACHE_OFF was.

Signed-off-by Beno?t Th?baudeau <[email protected]>

Cc Albert Aribaud <[email protected]>

---
This patch supersedes http://patchwork.ozlabs.org/patch/177263/ .

Changes for v2:
- Move enable_caches() outside of the main #if in order to fix this icache
issue rather than doing only #if cosmetic cleanup.

 .../arch/arm/cpu/arm1136/cpu.c                     |   22
 +++++++++++---------
 1 file changed, 12 insertions(+), 10 deletions(-)

(quoted diff) +0 -0

u-boot-arm-1dfc916/arch/arm/cpu/arm1136/cpu.c
index b98e3d9..32a4c24 100644

arch/arm/cpu/arm1136/cpu.c +0 -10

@@ -141,16 +141,6 @@ void flush_cache(unsigned long start, unsigned
 long size)
 	flush_dcache_range(start, start + size);
 }
 
-void enable_caches(void)
-{
-#ifndef CONFIG_SYS_ICACHE_OFF
-	icache_enable();
-#endif
-#ifndef CONFIG_SYS_DCACHE_OFF
-	dcache_enable();
-#endif
-}
-
 #else /* #ifndef CONFIG_SYS_DCACHE_OFF */
 void invalidate_dcache_all(void)

{

arch/arm/cpu/arm1136/cpu.c +2 -0

@@ -172,3 +162,15 @@ void flush_cache(unsigned long start, unsigned
 long size)
 {
 }
 #endif /* #ifndef CONFIG_SYS_DCACHE_OFF */
+
+#if !defined(CONFIG_SYS_ICACHE_OFF) ||

!defined(CONFIG_SYS_DCACHE_OFF)

arch/arm/cpu/arm1136/cpu.c +5 -0

+void enable_caches(void)
+{
+#ifndef CONFIG_SYS_ICACHE_OFF
+	icache_enable();
+#endif

arch/arm/cpu/arm1136/cpu.c void flush_cache(unsigned long start, unsigned long size) · line 176

Quoted 6 lines

arch/arm/cpu/arm1136/cpu.c +5 -0

+#ifndef CONFIG_SYS_DCACHE_OFF
+	dcache_enable();
+#endif
+}
+#endif

Applied to u-boot-arm/next, thanks!

Hi Albert,

On Friday, October 5, 2012 8:23:14 PM, Albert ARIBAUD wrote;

Quoted 57 lines

Quoted 54 lines · Beno?t Th?baudeau

enable_caches() did not enable icache if CONFIG_SYS_ICACHE_OFF was not defined
but CONFIG_SYS_DCACHE_OFF was.

Signed-off-by Beno?t Th?baudeau <[email protected]>

Cc Albert Aribaud <[email protected]>

---
This patch supersedes http://patchwork.ozlabs.org/patch/177263/ .

Changes for v2:
- Move enable_caches() outside of the main #if in order to fix this icache
issue rather than doing only #if cosmetic cleanup.

 .../arch/arm/cpu/arm1136/cpu.c                     |   22
 +++++++++++---------
 1 file changed, 12 insertions(+), 10 deletions(-)

(quoted diff) +0 -0

u-boot-arm-1dfc916/arch/arm/cpu/arm1136/cpu.c
index b98e3d9..32a4c24 100644

arch/arm/cpu/arm1136/cpu.c +0 -10

@@ -141,16 +141,6 @@ void flush_cache(unsigned long start, unsigned
 long size)
 	flush_dcache_range(start, start + size);
 }
 
-void enable_caches(void)
-{
-#ifndef CONFIG_SYS_ICACHE_OFF
-	icache_enable();
-#endif
-#ifndef CONFIG_SYS_DCACHE_OFF
-	dcache_enable();
-#endif
-}
-
 #else /* #ifndef CONFIG_SYS_DCACHE_OFF */
 void invalidate_dcache_all(void)

{

arch/arm/cpu/arm1136/cpu.c +2 -0

@@ -172,3 +162,15 @@ void flush_cache(unsigned long start, unsigned
 long size)
 {
 }
 #endif /* #ifndef CONFIG_SYS_DCACHE_OFF */
+
+#if !defined(CONFIG_SYS_ICACHE_OFF) ||

!defined(CONFIG_SYS_DCACHE_OFF)

arch/arm/cpu/arm1136/cpu.c +5 -0

+void enable_caches(void)
+{
+#ifndef CONFIG_SYS_ICACHE_OFF
+	icache_enable();
+#endif

Quoted 8 lines

arch/arm/cpu/arm1136/cpu.c void flush_cache(unsigned long start, unsigned long size) · line 176

Quoted 6 lines

arch/arm/cpu/arm1136/cpu.c +5 -0

+#ifndef CONFIG_SYS_DCACHE_OFF
+	dcache_enable();
+#endif
+}
+#endif

Applied to u-boot-arm/next, thanks!

I know that's true (I had seen it applied), but it has vanished since then. Now, I can't find it in u-boot-arm/next nor in u-boot-arm/master. This is perhaps related to the rebase that you had to do at some point.

Best regards,
Beno?t