armv7: cache maintenance operations

With D-cache and MMU enabled for ARM in u-boot it becomes imperative to support a minimal set of cache maintenance operations and necessary initializations before enabling MMU.

This series of patches attempt to do the following for armv7:
* Necessary initialization sequence before enabling MMU that includes invalidation of TLB, data caches, branch predictor array etc.
* Framework for supporting SOC specific outer caches in a generic manner (using a structure of function pointers - inspired by the Linux implementation)
* Generic armv7 cache maintenance operations for caches known to the CPU
* Support for ARM PL310 L2 cache controller used in OMAP4
* Cleanup of the cleanup_before_linux() function
* Adapting all armv7 SOCs to use the new framework and removing duplicated code

Testing:
* Extensive testing on OMAP4430SDP and OMAP3430SDP by creating coherency issues and solving them using the maintenance routines
- Eg: memfill a region of memory with a known pattern
- Invalidate the region
- Read back and compare the region with the original pattern
- If match fails it means that invalidate is successful
- Now add a flush call just before the invalidate
- If match succeeds it means that flush was successful
- Outer caches were tested with experiments involving making the function pointers NULL
* Kernel booting on OMAP4430SDP and OMAP3430SDP
Note: v2 has been tested only on OMAP4430SDP

v2:
* Pointer based callback mechanism for outer cache operations changed to a weakly linked functions.
* Change -march=armv7-a back to armv5
* Moved utility macros out of armv7.h
* Added documentation for new CONFIG options.
* Changed implementation of log2n to not use CLZ instruction as armv4 doesn't support this instruction and newly added Tegra2 uses -march=armv4
* Blank line after local variable declarations - fixed globally
* Explicitly added an empty flush_cache() under
#ifdef CONFIG_SYS_NO_DCACHE
* Removed the print inside the weakly linked stub function - __arm_init_before_mmu
* Fixed signature of flush_cache in cache.c
* More descriptive commit message for the PL310 support patch
* C struct for PL310 register accesses
* Fixed white space issues

Aneesh V (10):
arm: make default implementation of cache_flush() weakly linked
armv7: add miscellaneous utility macros
armv7: cache maintenance operations for armv7
armv7: replace CONFIG_L2_OFF with CONFIG_SYS_NO_L2CACHE
armv7: integrate cache maintenance support
arm: minor fixes for cache and mmu handling
armv7: add PL310 support to u-boot
armv7: adapt omap4 to the new cache maintenance framework
armv7: adapt omap3 to the new cache maintenance framework
armv7: adapt s5pc1xx to the new cache maintenance framework

 README                                        |   11 +
 arch/arm/cpu/armv7/Makefile                   |    2 +-
 arch/arm/cpu/armv7/cache_v7.c                 |  390
+++++++++++++++++++++++++
 arch/arm/cpu/armv7/cpu.c                      |   51 ++--
 arch/arm/cpu/armv7/omap3/Makefile             |    1 -
 arch/arm/cpu/armv7/omap3/board.c              |  138 ++++++++--
 arch/arm/cpu/armv7/omap3/cache.S              |  263 -----------------
 arch/arm/cpu/armv7/omap3/lowlevel_init.S      |   32 ++
 arch/arm/cpu/armv7/omap4/lowlevel_init.S      |   18 ++
 arch/arm/cpu/armv7/s5pc1xx/cache.S            |   88 +-----
 arch/arm/cpu/armv7/start.S                    |   18 +-
 arch/arm/include/asm/arch-omap3/omap3.h       |   20 ++
 arch/arm/include/asm/arch-omap3/sys_proto.h   |   10 +-
 arch/arm/include/asm/arch-omap4/sys_proto.h   |    1 -
 arch/arm/include/asm/arch-s5pc1xx/sys_proto.h |    3 -
 arch/arm/include/asm/armv7.h                  |   68 +++++
 arch/arm/include/asm/pl310.h                  |   74 +++++
 arch/arm/include/asm/utils.h                  |   80 +++++
 arch/arm/lib/Makefile                         |    1 +
 arch/arm/lib/board.c                          |    6 +
 arch/arm/lib/cache-cp15.c                     |   16 +-
 arch/arm/lib/cache-pl310.c                    |  116 ++++++++
 arch/arm/lib/cache.c                          |   20 +-
 include/common.h                              |    5 +-
 include/configs/ca9x4_ct_vxp.h                |    2 +-
 include/configs/efikamx.h                     |    2 +-
 include/configs/mx51evk.h                     |    2 +-
 include/configs/mx53evk.h                     |    2 +-
 include/configs/omap4_panda.h                 |    8 +-
 include/configs/omap4_sdp4430.h               |    8 +-
 include/configs/s5pc210_universal.h           |    2 +-
 include/configs/tegra2-common.h               |    2 +-
 include/configs/vision2.h                     |    2 +-
 33 files changed, 1037 insertions(+), 425 deletions(-)
 create mode 100644 arch/arm/cpu/armv7/cache_v7.c
 delete mode 100644 arch/arm/cpu/armv7/omap3/cache.S
 create mode 100644 arch/arm/include/asm/armv7.h
 create mode 100644 arch/arm/include/asm/pl310.h
 create mode 100644 arch/arm/include/asm/utils.h
 create mode 100644 arch/arm/lib/cache-pl310.c

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Quoted 5 lines

With D-cache and MMU enabled for ARM in u-boot it becomes imperative to support a minimal set of cache maintenance operations and necessary initializations before enabling MMU.

Quoted 13 lines

This series of patches attempt to do the following for armv7:
* Necessary initialization sequence before enabling MMU that includes invalidation of TLB, data caches, branch predictor array etc.
* Framework for supporting SOC specific outer caches in a generic manner (using a structure of function pointers - inspired by the Linux implementation)
* Generic armv7 cache maintenance operations for caches known to the CPU
* Support for ARM PL310 L2 cache controller used in OMAP4
* Cleanup of the cleanup_before_linux() function
* Adapting all armv7 SOCs to use the new framework and removing duplicated code

On Tue, Mar 8, 2011 at 5:07 AM, Aneesh V <[email protected] <mailto:[email protected]>> wrote:

With D-cache and MMU enabled for ARM in u-boot it becomes imperative to
support a minimal set of cache maintenance operations and necessary initializations before enabling MMU.

This series of patches attempt to do the following for armv7:
* Necessary initialization sequence before enabling MMU that includes
invalidation of TLB, data caches, branch predictor array etc.
* Framework for supporting SOC specific outer caches in a generic manner
(using a structure of function pointers - inspired by the Linux implementation)
* Generic armv7 cache maintenance operations for caches known to the CPU
* Support for ARM PL310 L2 cache controller used in OMAP4
* Cleanup of the cleanup_before_linux() function
* Adapting all armv7 SOCs to use the new framework and removing duplicated code

Any thoughts on this? What is the status please? Will this patch be applied, or if not, what is left to be done?

Regards,
Simon

add utility macros for:
* bit field operations
* log2n functions

Dear Aneesh V,

Quoted 3 lines · In message <[email protected]> you wrote:

add utility macros for:
* bit field operations
* log2n functions

...

arch/arm/include/asm/utils.h line 36

Quoted 9 lines

arch/arm/include/asm/utils.h new file +9 -0

+/* extract a bit field from a bit vector */
+#define get_bit_field(nr, start, mask)\
+     (((nr) & (mask)) >> (start))
+
+/* Set a field in a bit vector */
+#define set_bit_field(nr, start, mask, val)\
+     do { \
+             (nr) = ((nr) & ~(mask)) | (((val) << (start)) & (mask));\
+     } while (0);

I really dislike such "useful" helpers, because they make the code unreadable. Being nonstandrd, nbody knows what they are doing, so you always will have to look up the implementation before you can continue reading the code. It's a waste of time an resources.

arch/arm/include/asm/utils.h

Quoted 11 lines

arch/arm/include/asm/utils.h new file +10 -0

+/*
+ * Utility macro for read-modify-write of a hardware register
+ *   addr - address of the register
+ *   shift - starting bit position of the field to be modified
+ *   msk - mask for the field
+ *   val - value to be shifted masked and written to the field
+ */
+#define modify_reg_32(addr, shift, msk, val) \
+     do {\
+             writel(((readl(addr) & ~(msk))|(((val) << (shift)) &

(msk))),\

arch/arm/include/asm/utils.h line 49

Quoted 2 lines

arch/arm/include/asm/utils.h new file +2 -0

+                    (addr));\
+     } while (0);

NAK again, for the same reasons.

Note that there are some semi-standardized I/O accessro macros available, at least for some architectures, like clrbits.*(), setbits_(), or clrsetbits().

See for example "arch/arm/include/asm/io.h",
"arch/powerpc/include/asm/io.h" for reference.

Instead of reinventing the wheel (just differently shaped) we should rather try and use a single, standardized set of such helpers.

So please use the existing macros instead of inventing new, non-standard ones.

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de No journaling file system can recover your data if the disk dies.
- Steve Rago in <[email protected]>
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

I believe that this problem is getting worse - e.g. USB on Tegra2 writes various fields of about 20 registers to get things up and running. I find translating SOC datasheet register definitions into C code with shifts and masks to be slow and error-prone work. Also we do need to maintain this code, and it gets reused for new SOC variants, etc. So it is not as if it is written once and then buried and forgotten. There is also a tendency to use 'magic' constants rather than #define values or something with a sensible name, then hopefully add a half-hearted comment. This requires constant return looks at the datasheet to see what bits were chosen.

Quoted 3 lines · Simon Glass

I believe that this problem is getting worse - e.g. USB on Tegra2 writes various fields of about 20 registers to get things up and running. I find translating SOC datasheet register definitions into C code with shifts

and

Quoted 2 lines

masks to be slow and error-prone work. Also we do need to maintain this code, and it gets reused for new SOC variants, etc. So it is not as if it

is

Quoted 1 line

written once and then buried and forgotten. There is also a tendency to

use

Quoted 3 lines

'magic' constants rather than #define values or something with a sensible name, then hopefully add a half-hearted comment. This requires constant return looks at the datasheet to see what bits were chosen.

I have nothing against helper functions like this (although I think they should be inline functions, not macros, to guard against bugs due to side-effects in arguments).

But wouldn't it be even better to define the registers as C structures containing bitfields, so the C compiler can do the error-prone shifting and masking? Well-commented structure definitions for these registers also eliminate the need to refer back to the datasheet when working on the code.

--
Eric Cooper e c c @ c m u . e d u
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

I believe the problem here is the read/modify/write behaviour which is not well-defined in C. One ends up using volatile to ensure the right behaviour, and then this doesn't necessarily work either. Where one write must happen

Perhaps in newer compilers bitfield semantics have been improved?

Being a boot loader, charged with basic hardware initialisation, I believe bitfield access primitives should be well-supported by U-Boot.

Would you consider an RFC patch to add pan-U-Boot bitfield operations? Failing that, how about just for ARM?

Dear Simon Glass,

Quoted 3 lines · In message <[email protected]> you wrote:

Being a boot loader, charged with basic hardware initialisation, I believe bitfield access primitives should be well-supported by U-Boot.

I agree, and they are.

Quoted 3 lines

Would you consider an RFC patch to add pan-U-Boot bitfield operations? Failing that, how about just for ARM?

I fail to understand what you mean. ?We already have such implementations in place. ?I just oobject against adding new, non-standard ones when the existing ones perform the same purpose.

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de I think it's a new feature. Don't tell anyone it was an accident. :-) ?-- Larry Wall on s/foo/bar/eieio in <[email protected]>

There are a few very primitive macros in setbits and clrbits. I would very much like to see at least:

- define a field once in a header in an easy format along with any enums which define allowable values if applicable
- pack and unpack a value into a field: so set a bitfield to 13, for exampl= e
- obtain a mask for a field (i.e. with all bits set)
- equivalent of writel and readl for a bitfield (writel in fact being read/modify/write so perhaps a different name)

Some of these exist, some don't.

Any thoughts? I we get something in there I suspect ARM people will use it at least, and that will reduce the number of ways this gets

done. At present the local bitfield macros are not accessible to even other SOCs, let along other architectures. As this thread shows (and some of the code I have been reviewing recently) there is a demand for something more functional.

Dear Simon Glass,

Quoted 3 lines · In message<[email protected]> you wrote:

There are a few very primitive macros in setbits and clrbits. I would very much like to see at least:

Maybe we can agree to use these existing macros then instead of inventing new ones with the same functionality.

Quoted 8 lines

- define a field once in a header in an easy format along with any enums which define allowable values if applicable
- pack and unpack a value into a field: so set a bitfield to 13, for exampl= e
- obtain a mask for a field (i.e. with all bits set)
- equivalent of writel and readl for a bitfield (writel in fact being read/modify/write so perhaps a different name)

Quoted 1 line

Some of these exist, some don't.

Do you know of examples of such more complex definitions in the Linux kernel code?

In fact I had searched for a macro for similar needs as my set_bit_field() is addressing in Linux Kernel too but didn't find any.

I couldn't find any utility macros/functions for doing something like this.

Please somebody correct me if I am wrong.

Dear Aneesh V,

Quoted 3 lines · In message <[email protected]> you wrote:

In fact I had searched for a macro for similar needs as my set_bit_field() is addressing in Linux Kernel too but didn't find any.

...

Quoted 3 lines

I couldn't find any utility macros/functions for doing something like this.

Quoted 1 line

Please somebody correct me if I am wrong.

I am not aware of such a functionality in the Linux kernel, either.

A quick search did not even show any attempts to introduce such macros to Linux. ?It appears they don't need such a feature ?

Hi Wolfgang,

Quoted 10 lines · Wolfgang Denk

Dear Aneesh V,

Quoted 3 lines · In message <[email protected]> you wrote:

In fact I had searched for a macro for similar needs as my set_bit_field() is addressing in Linux Kernel too but didn't find any.

...

Quoted 3 lines

I couldn't find any utility macros/functions for doing something like this.

Quoted 6 lines

Quoted 1 line

Please somebody correct me if I am wrong.

I am not aware of such a functionality in the Linux kernel, either.

A quick search did not even show any attempts to introduce such macros to Linux. ?It appears they don't need such a feature ?

Wolfgang, what's your final take on this. Can I continue to use the utility macros given that I don't have any 'standard' alternative. For register accesses I shall move to setclrbits*() macros that you had suggested. But for bit operations I do not have any alternatives.

best regards,
Aneesh

Quoted 25 lines · Aneesh

Hi Wolfgang,

Quoted 10 lines · Wolfgang Denk

Dear Aneesh V,

Quoted 3 lines · In message<[email protected]> you wrote:

In fact I had searched for a macro for similar needs as my set_bit_field() is addressing in Linux Kernel too but didn't find any.

...

Quoted 3 lines

I couldn't find any utility macros/functions for doing something like this.

Quoted 6 lines

Quoted 1 line

Please somebody correct me if I am wrong.

I am not aware of such a functionality in the Linux kernel, either.

A quick search did not even show any attempts to introduce such macros to Linux. It appears they don't need such a feature ?

Wolfgang, what's your final take on this. Can I continue to use the utility macros given that I don't have any 'standard' alternative. For register accesses I shall move to setclrbits*() macros that you had suggested. But for bit operations I do not have any alternatives.

Quoted 3 lines

best regards,
Aneesh

Hi Aneesh,

If it helps, I will email a cleaned up bitfield patch to the list tomorrow. I hoping that it will get a warm reception :-)

Dear Aneesh V,

Quoted 3 lines · In message<[email protected]> you wrote:

add utility macros for:
* bit field operations
* log2n functions

...

arch/arm/include/asm/utils.h line 36

Quoted 9 lines

arch/arm/include/asm/utils.h new file +9 -0

+/* extract a bit field from a bit vector */
+#define get_bit_field(nr, start, mask)\
+	(((nr)&  (mask))>>  (start))
+
+/* Set a field in a bit vector */
+#define set_bit_field(nr, start, mask, val)\
+	do { \
+		(nr) = ((nr)&  ~(mask)) | (((val)<<  (start))&  (mask));\
+	} while (0);

I really dislike such "useful" helpers, because they make the code unreadable. Being nonstandrd, nbody knows what they are doing, so you always will have to look up the implementation before you can continue reading the code. It's a waste of time an resources.

arch/arm/include/asm/utils.h line 49

Quoted 12 lines

arch/arm/include/asm/utils.h new file +12 -0

+/*
+ * Utility macro for read-modify-write of a hardware register
+ *	addr - address of the register
+ *	shift - starting bit position of the field to be modified
+ *	msk - mask for the field
+ *	val - value to be shifted masked and written to the field
+ */
+#define modify_reg_32(addr, shift, msk, val) \
+	do {\
+		writel(((readl(addr)&  ~(msk))|(((val)<<  (shift))&  (msk))),\
+		       (addr));\
+	} while (0);

NAK again, for the same reasons.

Note that there are some semi-standardized I/O accessro macros available, at least for some architectures, like clrbits.*(), setbits_(), or clrsetbits().

See for example "arch/arm/include/asm/io.h",
"arch/powerpc/include/asm/io.h" for reference.

Instead of reinventing the wheel (just differently shaped) we should rather try and use a single, standardized set of such helpers.

So please use the existing macros instead of inventing new, non-standard ones.

Best regards,

Wolfgang Denk

Quoted 8 lines · Wolfgang Denk

Dear Aneesh V,

Quoted 3 lines · In message<[email protected]> you wrote:

add utility macros for:
* bit field operations
* log2n functions

...

Quoted 6 lines

Quoted 6 lines

arch/arm/include/asm/utils.h new file +6 -0

+/* extract a bit field from a bit vector */
+#define get_bit_field(nr, start, mask)\
+ (((nr)& (mask))>> (start))
+
+/* Set a field in a bit vector */
+#define set_bit_field(nr, start, mask, val)\

Quoted 9 lines

Quoted 3 lines

+ do { \
+ (nr) = ((nr)& ~(mask)) | (((val)<< (start))& (mask));\
+ } while (0);

I really dislike such "useful" helpers, because they make the code unreadable. Being nonstandrd, nbody knows what they are doing, so you always will have to look up the implementation before you can continue reading the code. It's a waste of time an resources.

Quoted 5 lines

Quoted 5 lines

I really dislike such "useful" helpers, because they make the code unreadable. Being nonstandrd, nbody knows what they are doing, so you always will have to look up the implementation before you can continue reading the code. It's a waste of time an resources.

What if I change the above to something like this(please note there isn't any Linux, U-Boot substitute for this):

arch/arm/include/asm/utils.h new file +11 -0

+/* extract a bit field from a bit vector */
+#define get_bit_field(nr, field)\
+	(((nr) & (field##_MASK)) >> (field##_OFFSET))
+
+/* Set a field in a bit vector */
+#define set_bit_field(nr, field, val)\
+	do { \
+		(nr) = ((nr) & ~(field##_MASK)) | (((val) << (field##_OFFSET))\
+			& (field##_MASK));\
+	} while (0);
+

And use it like this:
assoc = get_bit_field(ccsidr, CCSIDR_ASSOCIATIVITY); set_bit_field(ccsidr, CCSIDR_ASSOCIATIVITY, assoc + 1);

Isn't it more intuitive and almost self-explanatory now.

If you still don't like these as standard generic macros, how about having these macros just for OMAP with these names and use them only for OMAP code?

Please note that Simon's recent work for bitfield operations doesn't help me because the field definitions available to me are in shift/mask format and not in the range format that he uses. I will not be

able to now generate the range format for the hundreds of registers I use.

Dear Aneesh V,

Quoted 21 lines · In message<[email protected]> you wrote:

Quoted 5 lines

Quoted 5 lines

I really dislike such "useful" helpers, because they make the code unreadable. Being nonstandrd, nbody knows what they are doing, so you always will have to look up the implementation before you can continue reading the code. It's a waste of time an resources.

What if I change the above to something like this(please note there isn't any Linux, U-Boot substitute for this):

arch/arm/include/asm/utils.h new file +11 -0

+/* extract a bit field from a bit vector */
+#define get_bit_field(nr, field)\
+	(((nr)&  (field##_MASK))>>  (field##_OFFSET))
+
+/* Set a field in a bit vector */
+#define set_bit_field(nr, field, val)\
+	do { \
+		(nr) = ((nr)&  ~(field##_MASK)) | (((val)<<  (field##_OFFSET))\
+			&  (field##_MASK));\
+	} while (0);
+

Quoted 5 lines

And use it like this:
assoc = get_bit_field(ccsidr, CCSIDR_ASSOCIATIVITY); set_bit_field(ccsidr, CCSIDR_ASSOCIATIVITY, assoc + 1);

Isn't it more intuitive and almost self-explanatory now.

To me it is definitely NOT self-explanatory.

Actually I cannot even understand the argument names, now how it's supposed to be used. I would interpret "nr" as "number"; in the context of a bit field probably a bit number? Wrong guess...

It's highly cryptic and will only work with very special #defines providing definitions of foo_MASK and foo_OFFSET.

It is not clear that you can use this on plain simple memory stored variables only, and that you must not use this on any device registers; yet your example looks as if this were intended to be used on registers - but then we would need proper I/O accessors.

And there are still many cases where you cannot use this because for example evaluating several bits from the same object will cause repeated accesses, which may have side effects (when accessing device registers).

And, last but not least: they are nonstandard. I still don't understand why you cannot use the standard macros that already exist in Linux and in U-Boot, here for example clrbits*(), setbits*() and clrsetbits*() ?

Quoted 4 lines

If you still don't like these as standard generic macros, how about having these macros just for OMAP with these names and use them only for OMAP code?

Would that make it anything better? If it's not good enough for general use, we can still use it for OMAP? Like: oh, it's ony OMAP, so code quality does not matter? I think that's not good reasoning.

As I had mentioned in a previous mail, please note that the above macros are not for the same use-case as clrsetbits*() or friends (I had one macro that did something similar to clrsetbits*() and I intent to remove that in the next revision)

The above macros are for bit-field manipulation in a C integer variable
- nothing more.

So, if I have to write 5 different fields in a register I first write them into a variable and finally call writel() instead of making 5 clrsetbits*() calls.

There aren't any standard routines available for this need in Linux or U-Boot. I think you had agreed on this fact sometime back.

No. It was not about code quality. The question was whether these macros were generic enough to be used as the standard U-boot ones. The key question is how do you represent bit fields. There are different alternatives for this.

a. bit range (say 5:3)
b. shift(3) and field width(3)
c. shift(3) and mask(0x38)

We traditionally use (c) and we have auto-generated defines in this form. So, my macros use this format. I was not sure if other SoCs follow the same approach. That's why I suggested making them OMAP specific if you think (c) is not the standard approach.

Dear Aneesh V,

Quoted 8 lines · In message<[email protected]> you wrote:

As I had mentioned in a previous mail, please note that the above macros are not for the same use-case as clrsetbits*() or friends (I had one macro that did something similar to clrsetbits*() and I intent to remove that in the next revision)

The above macros are for bit-field manipulation in a C integer variable
- nothing more.

Why cannot we use the existing macros?

Quoted 3 lines

So, if I have to write 5 different fields in a register I first write them into a variable and finally call writel() instead of making 5 clrsetbits*() calls.

It does not make much difference to me if you call one macro or another 5 times.

It does mater to me to have several incompatible implementations doing essentially the same thing.

Quoted 3 lines

There aren't any standard routines available for this need in Linux or U-Boot. I think you had agreed on this fact sometime back.

I agree in so far as I am not aware of any such macros in Linux either. But my conclusion is a different one - it boils down to:
Linux is way more complex than U-Boot, so if they don;t need this, we don't need it either.

Quoted 10 lines

No. It was not about code quality. The question was whether these macros were generic enough to be used as the standard U-boot ones. The key question is how do you represent bit fields. There are different alternatives for this.

a. bit range (say 5:3)
b. shift(3) and field width(3)
c. shift(3) and mask(0x38)

d) Value and mask

Quoted 4 lines

We traditionally use (c) and we have auto-generated defines in this form. So, my macros use this format. I was not sure if other SoCs follow the same approach. That's why I suggested making them OMAP specific if you think (c) is not the standard approach.

Actually it does not matter. See my previous message to Simon: you can cover all this with the existing macros, and without adding any significant overhead.

So far, I did not see a single good argument why any new, nonstandard macros would be needed.

I am surprised why Linux doesn't have a solution for this. Perhaps the reason must be the confusion about the representation of a field that we discussed below. I suspect there may be non-standard local implementations in different modules.

Also, as somebody already mentioned, can't we do better than Linux?

Please consider the above example and let me know if I missed any solution using the existing standard macros.

best regards,
Aneesh

Quoted 7 lines

Quoted 4 lines

So, if I have to write 5 different fields in a register I first write them into a variable and finally call writel() instead of making 5 clrsetbits*() calls.

It does not make much difference to me if you call one macro or another 5 times.

No it makes a difference. It's 5 writes to a variable typically in an ARM register + 1 IO access vs 5 IO accesses. It's logically not equivalent.

Further if the 5 values are constants a smart compiler will fold the five writes into one write to the ARM register + 1 IO access, which won't happen if you used 5 clrsetbits*()

Problem: We want to read-modify-write an IO register 'reg' affecting 3 different fields: a, b, and c. The values to be written to the fields are a_val, b_val, and c_val respectively:

Solution 1 - without any macros:

unsigned int r = readl(reg);

r = (r & ~a_mask) | ((a_val << a_shift) & a_mask)
r = (r & ~b_mask) | ((b_val << b_shift) & b_mask)
r = (r & ~c_mask) | ((c_val << c_shift) & c_mask)

writel(r, reg);

Solution2 - with my macros:

unsigned int r = readl(reg);
set_bit_field(r, a, a_val);
set_bit_field(r, b, b_val);
set_bit_field(r, c, c_val);
writel(r, reg);

Solution3 - with clrsetbits*():

clrsetbits_le32(reg, a_mask, a_val << a_shift);
clrsetbits_le32(reg, b_mask, b_val << b_shift);
clrsetbits_le32(reg, c_mask, c_val << c_shift);

Solution 3 is not acceptable to me because it's clearly not equivalent to what I want to do. Writing the register 3 times instead of once may have undesirable side-effects. Even if it worked, it's clearly not efficient.

If you are forcing me to use solution 1, IMHO you are essentially forcing me not to use a sub-routine for a task that is repeated many times in my code, leaving my code to be more error prone and less readable.

You accuse set_bit_field() of being cryptic. I would say the implementation of clrsetbits_le32() is even more cryptic with so many levels of indirection. I think that goes with any sub-routine/API.

You need to read the code/documentation once to know what it does.

Quoted 2 lines

It does mater to me to have several incompatible implementations doing essentially the same thing.

They are not doing the same thing as explained above.

Dear Aneesh V,

Quoted 11 lines · In message<[email protected]> you wrote:

Quoted 7 lines

Quoted 4 lines

So, if I have to write 5 different fields in a register I first write them into a variable and finally call writel() instead of making 5 clrsetbits*() calls.

It does not make much difference to me if you call one macro or another 5 times.

No it makes a difference. It's 5 writes to a variable typically in an ARM register + 1 IO access vs 5 IO accesses. It's logically not equivalent.

You can use in_le32() to read the register in a variable, a number of macros operating on that variable, and then a out_le32() to write it back.

It's logically equivalent.

Quoted 3 lines

Further if the 5 values are constants a smart compiler will fold the five writes into one write to the ARM register + 1 IO access, which won't happen if you used 5 clrsetbits*()

See above.

If you ever want to use your macros on device registers, you MUST use proper memory barriers. This will probably ruin your idea to combine the access into a single write. So you can just work on a variable as suggested before.

Quoted 20 lines

Problem: We want to read-modify-write an IO register 'reg' affecting 3 different fields: a, b, and c. The values to be written to the fields are a_val, b_val, and c_val respectively:

Solution 1 - without any macros:

unsigned int r = readl(reg);

r = (r&  ~a_mask) | ((a_val<<  a_shift)&  a_mask)
r = (r&  ~b_mask) | ((b_val<<  b_shift)&  b_mask)
r = (r&  ~c_mask) | ((c_val<<  c_shift)&  c_mask)

writel(r, reg);

Solution2 - with my macros:

unsigned int r = readl(reg);
set_bit_field(r, a, a_val);
set_bit_field(r, b, b_val);
set_bit_field(r, c, c_val);
writel(r, reg);

Quoted 5 lines

Solution3 - with clrsetbits*():

clrsetbits_le32(reg, a_mask, a_val<<  a_shift);
clrsetbits_le32(reg, b_mask, b_val<<  b_shift);
clrsetbits_le32(reg, c_mask, c_val<<  c_shift);

Solution 4 - with standard macros, and proper defines:

	unsigned int r = in_le32(reg);		/* or readl() if you like */

	clrsetbits_le32(&r, a_mask, a_val);
	clrsetbits_le32(&r, b_mask, b_val);
	clrsetbits_le32(&r, c_mask, c_val);

	out_le32(reg, r);

Actually solution 3 looks best to me.

Quoted 4 lines

Solution 3 is not acceptable to me because it's clearly not equivalent to what I want to do. Writing the register 3 times instead of once may have undesirable side-effects. Even if it worked, it's clearly not efficient.

In case of side effects you can use solution 4.

We should not bother here about wether this is "efficient" or "not efficient". Probably none opf this code is ever time critical, not to the extent of a few additional instructions.

Quoted 4 lines

If you are forcing me to use solution 1, IMHO you are essentially forcing me not to use a sub-routine for a task that is repeated many times in my code, leaving my code to be more error prone and less readable.

I agree that 1 is ugly and error prone, but there is no need to use it.

I repeat: we have a set of powerful macros ready available. Just use them.

I still don't think this is the 'right' solution for my problem. I don't like the fact that clrsetbits_le32() introduces a lot of un-necessary 'volatile's.

Yes, it's about the 'efficiency'. May be it doesn't count in some cases. But, may be it counts in some other cases. Basically, I don't like to sacrifice 'efficiency' unless the cost for achieving it is very

If you still insist, I can use clrsetbits_le32() in the interest of getting this to a closure.

We have a set of powerful macros designed for bit-field accesses in IO egisters.

But, what I am looking for is a set of macros for bit-field operations on C integer variables without the un-necessary overhead of IO register accesses. I am looking for missing APIs in bitops.h not anything from io.h

Dear Aneesh V,

Quoted 4 lines · In message<[email protected]> you wrote:

I still don't think this is the 'right' solution for my problem. I don't like the fact that clrsetbits_le32() introduces a lot of un-necessary 'volatile's.

Well, with this argument you would also have to refuse using readl() and writel() and all other I/O accessor macros. The only place where volatile is used is in the __arch_get*() and __arch_put*() macros, and ther eit is supposed to be ok.

Quoted 3 lines

Yes, it's about the 'efficiency'. May be it doesn't count in some cases. But, may be it counts in some other cases. Basically, I don't like to sacrifice 'efficiency' unless the cost for achieving it is very

Try and show me a single case where you see a measurable difference in performance.

Quoted 2 lines

If you still insist, I can use clrsetbits_le32() in the interest of getting this to a closure.

Please do. Thanks.

As I start re-working on my patches I realize that there is no alternative to get_bit_field(). clrsetbits_le32() works as an alternative for set_bit_field() but I couldn't find anything in io.h that could replace get_bit_field(). The only option I seem to have is to mask and shift directly every time. Is that what you prefer over get_bit_field()?

Dear Aneesh V,

Quoted 7 lines · In message<[email protected]> you wrote:

As I start re-working on my patches I realize that there is no alternative to get_bit_field(). clrsetbits_le32() works as an alternative for set_bit_field() but I couldn't find anything in io.h that could replace get_bit_field(). The only option I seem to have is to mask and shift directly every time. Is that what you prefer over get_bit_field()?

I don't understand this comment.

You should NOT use clrsetbits_*() instead of set_bit_field() - depending on what you want to do, clrbits_*() or setbits_*() are better choices for simple operations.

Also please notice that none of clrbits_*(), setbits_*() or clrsetbits_*() perform any masking or shifting. If you don't want to do this explicitly when using the macro, you can hide it in respective definitions of the mask values. I showed how this could be done in the examples I posted in the thread with Simon.

Yes. I have seen those macros. But more often than not the bit field is more than 1 bit wide and the value to be set is not necessarily all 0's or all 1's. That's why I have to use clrsetbits_*()

The problem I have to deal with is different. get_bit_field() was intended to extract bit fields from an integer. So, the target usage will be something like this(where a, b, and c are bit fields in register my_reg)

u32 my_reg, a_val, b_val, c_val;

u32 my_reg = readl(my_reg_addr);

a_val = get_bit_field(my_reg, a_mask);
b_val = get_bit_field(my_reg, b_mask);
c_val = get_bit_field(my_reg, c_mask);

Do you see an alternative method for doing this using the standard macros?

Dear Aneesh V,

Quoted 4 lines · In message <[email protected]> you wrote:

Yes. I have seen those macros. But more often than not the bit field is more than 1 bit wide and the value to be set is not necessarily all 0's or all 1's. That's why I have to use clrsetbits_*()

I see. ?In such a case (and only then) clrsetbits_*() is indeed the right choice.

Quoted 7 lines

The problem I have to deal with is different. get_bit_field() was intended to extract bit fields from an integer. So, the target usage will be something like this(where a, b, and c are bit fields in register my_reg)

u32 my_reg, a_val, b_val, c_val;

Quoted 8 lines

u32 my_reg = readl(my_reg_addr);

a_val = get_bit_field(my_reg, a_mask);
b_val = get_bit_field(my_reg, b_mask);
c_val = get_bit_field(my_reg, c_mask);

Do you see an alternative method for doing this using the standard macros?

Please see the example given here:

http://article.gmane.org/gmane.comp.boot-loaders.u-boot/101146

Looking closer, the "FIELD_VAL" macro alone will probably not suffice, as you need both shift directions, like that:

? ? ? ?#define FIELD_SHIFT ? ? 16
? ? ? ?#define FIELD_MASK ? ? ?0xF

? ? ? ?#define FIELD_BITS(x) (x << 16)
? ? ? ?#define FIELD_MASK FIELD_BITS(0xF)
? ? ? ?#define FIELD_VAL(x) ((x & FIELD_MASK) >> 16)

The code would then look something like this:

? ? ? ?my_reg = readl(my_reg_addr);

? ? ? ?a_val = A_VAL(my_reg);
? ? ? ?b_val = B_VAL(my_reg);
? ? ? ?c_val = C_VAL(my_reg);

...or similar.

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de "I dislike companies that have a we-are-the-high-priests-of-hardware- so-you'll-like-what-we-give-you attitude. I like commodity markets in which iron-and-silicon hawkers know that they exist to ?provide ?fast toys for software types like me to play with..." ? ?- Eric S. Raymond

Quoted 8 lines

Looking closer, the "FIELD_VAL" macro alone will probably not suffice, as you need both shift directions, like that:

       #define FIELD_SHIFT     16
       #define FIELD_MASK      0xF

Quoted 3 lines

       #define FIELD_BITS(x) (x << 16)
       #define FIELD_MASK FIELD_BITS(0xF)
       #define FIELD_VAL(x) ((x & FIELD_MASK) >> 16)

Hi Wolfgang,

I think you have FIELD_MASK being two meanings: the un-shifted or 'raw' mask, and the shifted mask. So perhaps:

        #define FIELD_SHIFT     16
        #define FIELD_RAWMASK      0xF

When you have a lot of these definitions in a row you have mentally check the bit width of the mask:

Is the above correct, or do fields overlap or not cover fully?

Dear Simon Glass,

Quoted 8 lines · In message <[email protected]> you wrote:

Quoted 8 lines

Looking closer, the "FIELD_VAL" macro alone will probably not suffice, as you need both shift directions, like that:

? ? ? ?#define FIELD_SHIFT ? ? 16
? ? ? ?#define FIELD_MASK ? ? ?0xF

Quoted 8 lines

Quoted 3 lines

? ? ? ?#define FIELD_BITS(x) (x << 16)
? ? ? ?#define FIELD_MASK FIELD_BITS(0xF)
? ? ? ?#define FIELD_VAL(x) ((x & FIELD_MASK) >> 16)

Hi Wolfgang,

I think you have FIELD_MASK being two meanings: the un-shifted or 'raw' mask, and the shifted mask. So perhaps:

My intention was to have only the real mask, i. e. what you need to apply to the register data (in your language the shifted mask).

Quoted 2 lines

? ? ? ? #define FIELD_SHIFT ? ? 16
? ? ? ? #define FIELD_RAWMASK ? ? ?0xF

This is never really needed - you only use it once.

Quoted 2 lines

When you have a lot of these definitions in a row you have mentally check the bit width of the mask:

Yes, this is one of the disadvantages of thinking in bit fields and one reason why I never write code like that.

I mentioned this before - I consider it error prone [like most other "clever" tricks that "make life easy" and allow you to stop thinking].

Quoted 1 line

Is the above correct, or do fields overlap or not cover fully?

All this is possible. ?There may be overlapping definitions, and there may be reserved bits that need special handling.

There may even be bits that cannot simply be re-writeen as we read them, but that must always be written as zeroes or ones.

I never claimed that these "bit field" juggling was a clever idea. It works for some cases, and causes major pita for others.

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de In an organization, each person rises to the level of his own ?incom- petency ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? - The Peter Principle

Dear Aneesh V,

Quoted 4 lines · In message<[email protected]> you wrote:

Yes. I have seen those macros. But more often than not the bit field is more than 1 bit wide and the value to be set is not necessarily all 0's or all 1's. That's why I have to use clrsetbits_*()

I see. In such a case (and only then) clrsetbits_*() is indeed the right choice.

Quoted 7 lines

The problem I have to deal with is different. get_bit_field() was intended to extract bit fields from an integer. So, the target usage will be something like this(where a, b, and c are bit fields in register my_reg)

u32 my_reg, a_val, b_val, c_val;

Quoted 8 lines

u32 my_reg = readl(my_reg_addr);

a_val = get_bit_field(my_reg, a_mask);
b_val = get_bit_field(my_reg, b_mask);
c_val = get_bit_field(my_reg, c_mask);

Do you see an alternative method for doing this using the standard macros?

Please see the example given here:

http://article.gmane.org/gmane.comp.boot-loaders.u-boot/101146

Looking closer, the "FIELD_VAL" macro alone will probably not suffice, as you need both shift directions, like that:

	#define FIELD_SHIFT	16
	#define FIELD_MASK	0xF

	#define FIELD_BITS(x) (x<<  16)
	#define FIELD_MASK FIELD_BITS(0xF)
	#define FIELD_VAL(x) ((x&  FIELD_MASK)>>  16)

The code would then look something like this:

	my_reg = readl(my_reg_addr);

	a_val = A_VAL(my_reg);
	b_val = B_VAL(my_reg);
	c_val = C_VAL(my_reg);

So, the shift information is embedded in this mask and can be extracted by finding the first set bit. But in reality my get_bit_field() function indeed takes both arguments. So it's something like this:

Dear Aneesh V,

Quoted 4 lines · In message<[email protected]> you wrote:

So, the shift information is embedded in this mask and can be extracted by finding the first set bit. But in reality my get_bit_field() function indeed takes both arguments. So it's something like this:

As stated before, I will not accept any bit filed macros like get_bit_field().

Will you accept something like this?

a_val = (reg & a_mask) >> a_shift;

Dear Aneesh V,

Quoted 4 lines · In message <[email protected]> you wrote:

Will you accept something like this?

a_val = (reg & a_mask) >> a_shift;

Yes, of course (that's what seems most natural to me).

And to set the value then you have:

	reg &= ~a_mask;				/* Clear a_val */
	reg |= (a_val << a_shift) & a_mask;	/* Set new a_val */

Dear Graeme Russ,

Quoted 1 line · In message <[email protected]> you wrote:

Quoted 4 lines

And to set the value then you have:

	reg &= ~a_mask;				/* Clear a_val */
	reg |= (a_val << a_shift) & a_mask;	/* Set new a_val */

This could be done using

	clrsetbits_le32(&reg, a_mask, a_val << a_shift);

Dear Graeme Russ,

Quoted 1 line · In message <[email protected]> you wrote:

Quoted 4 lines

And to set the value then you have:

	reg &= ~a_mask;				/* Clear a_val */
	reg |= (a_val << a_shift) & a_mask;	/* Set new a_val */

This could be done using

	clrsetbits_le32(&reg, a_mask, a_val << a_shift);

is equivalent except that, as already pointed out, clrsetbits and friends:

a) Are not portable because only ARM and PPC define them which makes them, by definition, non-standard

b) Each invocation results in a read barrier plus a write barrier

c) If the hardware register is sensitive to partial updates (i.e. requires all bit-fields to be updated in on operation) this requires a read into a local variable, calls to clrsetbits against that variable and finally a write-back - Lots of memory barriers

Dear Graeme Russ,

Quoted 5 lines · In message <[email protected]> you wrote:

is equivalent except that, as already pointed out, clrsetbits and friends:

?a) Are not portable because only ARM and PPC define them which makes ? ? them, by definition, non-standard

They should be added to _any_ arch/<arch>/include/asm/io.h that doesn't use them yet.

Quoted 1 line

?b) Each invocation results in a read barrier plus a write barrier

...which is intentionally (actually mandatory) when accessing I/O memory, and negligable overhead on plain memory.

Quoted 4 lines

?c) If the hardware register is sensitive to partial updates (i.e. requires ? ? all bit-fields to be updated in on operation) this requires a read into ? ? a local variable, calls to clrsetbits against that variable and finally ? ? a write-back - Lots of memory barriers

Wrong. The macro does this automatically. ?There is only a single read and a single write per call.

Quoted 10 lines · Wolfgang Denk

Dear Aneesh V,

Quoted 4 lines · In message<[email protected]> you wrote:

Will you accept something like this?

a_val = (reg& a_mask)>> a_shift;

Yes, of course (that's what seems most natural to me).

Me too - The code is obvious - the desired value is being masked out of a larger composite value and then shifted right to bit 0

And to set the value then you have:

	reg&= ~a_mask;				/* Clear a_val */
	reg |= (a_val<<  a_shift)&  a_mask;	/* Set new a_val */

AND'ing with a_mask is required to prevent accidental clobbering when a_val is out-of-range. May give undesirable results by setting an illegal a_val, but at least you don't clobber unrelated bit fields

Hi Grame,

Quoted 22 lines · Graeme Russ

Quoted 11 lines · Wolfgang Denk

Dear Aneesh V,

Quoted 4 lines · In message<[email protected]> ?you wrote:

Will you accept something like this?

a_val = (reg& ?a_mask)>> ?a_shift;

Yes, of course (that's what seems most natural to me).

Me too - The code is obvious - the desired value is being masked out of a larger composite value and then shifted right to bit 0

And to set the value then you have:

? ? ? ?reg&= ~a_mask; ? ? ? ? ? ? ? ? ? ? ? ? ?/* Clear a_val */ ? ? ? ?reg |= (a_val<< ?a_shift)& ?a_mask; ? ? /* Set new a_val */

Quoted 4 lines

AND'ing with a_mask is required to prevent accidental clobbering when a_val
is out-of-range. May give undesirable results by setting an illegal a_val, but at least you don't clobber unrelated bit fields

These are exactly what my helper functions were doing. Are you suggesting that doing these directly is better than doing them using helper functions?

Now, that being said, I see no reason not to do the following if I had, for example, multiple serial port configuration registers which are all identical:

/* num data bits is stored in bits 2-4 of the serial config register */
#define DATA_BITS_MASK 0x001c
#define DATA_BITS_OFFSET 2

u32 set_serial_data_bits(u32 ser_cfg, u8 data_bits)

{
	ser_cfg &= ~DATA_BITS_MASK;
	ser_cfg |= ((u32)ser_cfg << DATA_BITS_OFFSET) & ?DATA_BITS_MASK;

	return ser_cfg;
}

void serial_init(void)

{
	u32 ser_cfg;

	for (i=0; i<NUM_SERIAL_PORTS; i++) {
		ser_cfg = read_serial_cfg(i);
		ser_cfg = set_serial_data_bits(ser_cfg, 7);
		write_serial_cfg(i, ser_cfg);
	}
}

Dear Graeme Russ,

Quoted 18 lines · In message <[email protected]> you wrote:

Now, that being said, I see no reason not to do the following if I had, for example, multiple serial port configuration registers which are all identical:

/* num data bits is stored in bits 2-4 of the serial config register */
#define DATA_BITS_MASK 0x001c
#define DATA_BITS_OFFSET 2

u32 set_serial_data_bits(u32 ser_cfg, u8 data_bits)

{
	ser_cfg &= ~DATA_BITS_MASK;
	ser_cfg |= ((u32)ser_cfg << DATA_BITS_OFFSET) &  DATA_BITS_MASK;

	return ser_cfg;
}

void serial_init(void)

Quoted 9 lines

{
	u32 ser_cfg;

	for (i=0; i<NUM_SERIAL_PORTS; i++) {
		ser_cfg = read_serial_cfg(i);
		ser_cfg = set_serial_data_bits(ser_cfg, 7);
		write_serial_cfg(i, ser_cfg);
	}
}

One reason for not doing this is that we should not reinvent the wheel again and again, and instead use standard APIs.

I cannot find any such code in U-Boot, so I cannot check, but to me it smells a lot as if this code should rather use clrsetbits_*() and other proper I/O accessors.

- Add a framework for layered cache maintenance
- separate out SOC specific outer cache maintenance from maintenance of caches known to CPU

- Add generic ARMv7 cache maintenance operations that affect all caches known to ARMv7 CPUs. For instance in Cortex-A8 these opertions will affect both L1 and L2 caches. In Cortex-A9 these will affect only L1 cache

- D-cache operations supported:
- Invalidate entire D-cache
- Invalidate D-cache range
- Flush(clean & invalidate) entire D-cache
- Flush D-cache range

arch/arm/cpu/armv7/cache_v7.c new file +13 -0

+void invalidate_dcache_all(void)
+{
+}
+
+void flush_dcache_all(void)
+{
+}
+
+void invalidate_dcache_range(unsigned long start, unsigned long stop)
+{
+}
+
+void flush_dcache_range(unsigned long start, unsigned long stop)

Dear Aneesh V,

Quoted 11 lines · In message<[email protected]> you wrote:

- Add a framework for layered cache maintenance
- separate out SOC specific outer cache maintenance from maintenance of caches known to CPU

- Add generic ARMv7 cache maintenance operations that affect all caches known to ARMv7 CPUs. For instance in Cortex-A8 these opertions will affect both L1 and L2 caches. In Cortex-A9 these will affect only L1 cache

- D-cache operations supported:
- Invalidate entire D-cache

Quoted 1 line

- Invalidate D-cache range

Quoted 1 line

- Flush(clean& invalidate) entire D-cache

Quoted 1 line

- Flush D-cache range

How much of this is actually needed in the context of U-Boot?

...

arch/arm/cpu/armv7/cache_v7.c line 82

Quoted 8 lines

arch/arm/cpu/armv7/cache_v7.c new file +8 -0

+	for (way = num_ways - 1; way>= 0 ; way--)
+		for (set = num_sets - 1; set>= 0; set--) {
+			setway = (level<<  1) | (set<<  log2_line_len) |
+				 (way<<  way_shift);
+			/* Invalidate data/unified cache line by set/way */
+			asm volatile ("	mcr p15, 0, %0, c7, c6, 2"
+					: : "r" (setway));
+		}

Braces needed for multiline for(). Please fix globally.

...

arch/arm/cpu/armv7/cache_v7.c line 144

Quoted 6 lines

arch/arm/cpu/armv7/cache_v7.c new file +6 -0

+	if (operation == ARMV7_DCACHE_INVAL_ALL)
+		v7_inval_dcache_level_setway(level, num_sets, num_ways,
+				      way_shift, log2_line_len);
+	else if (operation == ARMV7_DCACHE_CLEAN_INVAL_ALL)
+		v7_clean_inval_dcache_level_setway(level, num_sets, num_ways,
+						   way_shift, log2_line_len);

Braces needed for multiline statements. Please fix globally.

Review

Quoted 3 lines

(quoted diff) +3 -0

+	for (mva = start; mva<  stop; mva = mva + line_len)
+		/* DCCIMVAC - Clean&  Invalidate data cache by MVA to PoC */
+		asm volatile ("mcr p15, 0, %0, c7, c14, 1" : : "r" (mva));

And again etc. etc.

...

Quoted 18 lines

arch/arm/cpu/armv7/cache_v7.c new file +18 -0

+void invalidate_dcache_all(void)
+{
+}
+
+void flush_dcache_all(void)
+{
+}
+
+void invalidate_dcache_range(unsigned long start, unsigned long stop)
+{
+}
+
+void flush_dcache_range(unsigned long start, unsigned long stop)
+{
+}
+
+void arm_init_before_mmu(void)
+{

arch/arm/cpu/armv7/cache_v7.c line 301

Quoted 5 lines

arch/arm/cpu/armv7/cache_v7.c new file +5 -0

+}
+
+void  flush_cache(unsigned long start, unsigned long size)
+{
+}

Please do not add dead code. I consider it misleading to have functions which promise to perform something, and actually do nothing.

Quoted 2 lines

How much of this is actually needed in the context of U-Boot?

Please see above. As far as I know OMAP doesn't do DMA in U-Boot.

Quoted 2 lines

Please do not add dead code. I consider it misleading to have functions which promise to perform something, and actually do nothing.

These are empty only in the else part of an #ifdef. When cache support is compiled out these need to be empty.

Dear Aneesh V,

Quoted 4 lines · In message<[email protected]> you wrote:

Quoted 2 lines

How much of this is actually needed in the context of U-Boot?

Please see above. As far as I know OMAP doesn't do DMA in U-Boot.

Devices like USB oth Ethernet don't use DMA for data transfers?

Quoted 5 lines

Quoted 2 lines

Please do not add dead code. I consider it misleading to have functions which promise to perform something, and actually do nothing.

These are empty only in the else part of an #ifdef. When cache support is compiled out these need to be empty.

When cache support is compiled out there should be no callers to these functions, so maybe they can disappear?

Dear Aneesh V,

Quoted 4 lines · In message<[email protected]> you wrote:

Quoted 2 lines

How much of this is actually needed in the context of U-Boot?

Please see above. As far as I know OMAP doesn't do DMA in U-Boot.

Devices like USB oth Ethernet don't use DMA for data transfers?

Hi Wolfgang,

Quoted 4 lines · Wolfgang Denk

Dear Aneesh V,

Quoted 1 line · In message<[email protected]> ?you wrote:

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

How much of this is actually needed in the context of U-Boot?

Please see above. As far as I know OMAP doesn't do DMA in U-Boot.

Devices like USB oth Ethernet don't use DMA for data transfers?

Ethernet support is not upstreamed yet. USB doesn't seem to be using DMA on a quick check.

best regards,
Aneesh
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

replace all occurences of CONFIG_L2_OFF with a more appropriate CONFIG_SYS_NO_L2CACHE

CONFIG_SYS_NO_L2CACHE has been chosen to be in line with CONFIG_SYS_NO_ICACHE and CONFIG_SYS_NO_DCACHE

Dear Aneesh V,

Quoted 5 lines · In message<[email protected]> you wrote:

replace all occurences of CONFIG_L2_OFF with a more appropriate CONFIG_SYS_NO_L2CACHE

CONFIG_SYS_NO_L2CACHE has been chosen to be in line with CONFIG_SYS_NO_ICACHE and CONFIG_SYS_NO_DCACHE

sorry, but these are ugly mis-nomers. The situation is not that there is no cache present (which "NO_*CACHE" suggestes), but that the cache is (kept) turned off.

CONFIG_SYS_L2CACHE_OFF is a _much_ better name than CONFIG_SYS_NO_L2CACHE.

Please fix all these names.

Quoted 2 lines

Please fix all these names.

Ok. Including the existing CONFIG_SYS_NO_DCACHE and CONFIG_SYS_NO_ICACHE too, right?

Dear Aneesh V,

Quoted 5 lines · In message<[email protected]> you wrote:

Quoted 2 lines

Please fix all these names.

Ok. Including the existing CONFIG_SYS_NO_DCACHE and CONFIG_SYS_NO_ICACHE too, right?

yes, please. Thanks a lot!

Hi Wolfgang,

Quoted 9 lines · Wolfgang Denk

Dear Aneesh V,

Quoted 5 lines · In message<[email protected]> you wrote:

Quoted 2 lines

Please fix all these names.

Ok. Including the existing CONFIG_SYS_NO_DCACHE and CONFIG_SYS_NO_ICACHE too, right?

Quoted 1 line

yes, please. Thanks a lot!

How about the existing flag CONFIG_SYS_NO_CP15_CACHE

I think this flag should be simply removed. I think CONFIG_SYS_ICACHE_OFF and CONFIG_SYS_DCACHE_OFF together can serve all needs.

- Enable I-cache on bootup
- Enable MMU and D-cache immediately after relocation
- Do necessary initialization before enabling d-cache and MMU

Dear Aneesh V,

Quoted 3 lines · In message<[email protected]> you wrote:

- Enable I-cache on bootup
- Enable MMU and D-cache immediately after relocation
- Do necessary initialization before enabling d-cache and MMU

Would it be possible to do this even _before_ relocation, so to speed up memory accesses during relocation? Of course, proper invalidates/ flushes will be needed before jumping to the RAM address, but I guess this would save a bit of boot times?

Quoted 5 lines

Would it be possible to do this even _before_ relocation, so to speed up memory accesses during relocation? Of course, proper invalidates/ flushes will be needed before jumping to the RAM address, but I guess this would save a bit of boot times?

I intentionally kept it after relocation to avoid un-necessary complexities.

Relocation is a case of self-modifying code. In Harvard architectures like armv7 there will be coherency issues unless we flush the entire D-cache(range based operation may be equally or more expensive) and invalidate the entire I-cache.

So, in effect everything has to be flushed to memory before you jump to the new location. There may be a small advantage because flushing from cache allows for bursting to the DDR where as bursting is not possible when d-cache is disabled. But I think this is not worth the trouble.

Dear Aneesh V,

Quoted 8 lines · In message<[email protected]> you wrote:

Quoted 5 lines

Would it be possible to do this even _before_ relocation, so to speed up memory accesses during relocation? Of course, proper invalidates/ flushes will be needed before jumping to the RAM address, but I guess this would save a bit of boot times?

I intentionally kept it after relocation to avoid un-necessary complexities.

Yes, I can understand this. Do you plean to extend this to include relocation n a later step?

Quoted 4 lines

Relocation is a case of self-modifying code. In Harvard architectures like armv7 there will be coherency issues unless we flush the entire D-cache(range based operation may be equally or more expensive) and invalidate the entire I-cache.

I would not call this self-modifying code.

Regarding the need to flush/invalidate caches when jumping to RAM:
yes, we have to flush the D-cache to make sure all data actually has hit the memory. But there should be no need to invalidate the I-cache as we have never been fetching any instructions from this address range, so there cannot be any incorrect entries in cache.

Or am I missing something?

Quoted 5 lines

So, in effect everything has to be flushed to memory before you jump to the new location. There may be a small advantage because flushing from cache allows for bursting to the DDR where as bursting is not possible when d-cache is disabled. But I think this is not worth the trouble.

Well, all the data copy will also use cached writes, which are much

faster. I think this will result in measurable time differences.

In ARM literature this kind of situations are also referred to as "self- modifying". Here is an excerpt from ARM manual gloassary:

Quoted 1 line

Well, all the data copy will also use cached writes, which are much

You mean, you will do a range-flush on only the .text section's area?

Quoted 1 line

faster. I think this will result in measurable time differences.

Relocation itself was not taking a huge amount of time in my measurement sometime back. It was about 16 ms for OMAP4.

Dear Aneesh V,

Quoted 3 lines · In message<[email protected]> you wrote:

In ARM literature this kind of situations are also referred to as "self- modifying". Here is an excerpt from ARM manual gloassary:

I see. Thanks.

Quoted 3 lines

Quoted 1 line

Well, all the data copy will also use cached writes, which are much

You mean, you will do a range-flush on only the .text section's area?

That should indeed be sufficient.

Quoted 2 lines

Quoted 1 line

faster. I think this will result in measurable time differences.

Quoted 2 lines

Relocation itself was not taking a huge amount of time in my measurement sometime back. It was about 16 ms for OMAP4.

I was especially talking about the loading from external storage, not primarily relocation. This will even be mnore important if we had (much) bigger images - like when loading an OS kernel as second stage payload instead of U-Boot.

Quoted 5 lines

I was especially talking about the loading from external storage, not primarily relocation. This will even be mnore important if we had (much) bigger images - like when loading an OS kernel as second stage payload instead of U-Boot.

Are you talking about enabling D-cache in SPL? Technically, this should

be possible as soon as DRAM is initialized(because internal RAMs typically do not have enough memory for page-tables(16KB) in addition to the SPL itself). But you might want to consider these.

1. We need to setup the page tables, invalidate the cache, enable it and before jumping to kernel flush and disable it again. Is that all worth just for speeding up the loading part? I think one must consider DMA for loading the image instead.

2. What happens if the payload is U-Boot? You still have the problem of flushing. Besides after reaching U-Boot do we setup MMU again or not? If we decide to retain the page-tables setup by SPL then U-Boot and SPL needs to align on the location of page-tables. This is difficult because U-Boot dynamically determines the memory for page-tables where as SPL doesn't have such intelligence. If we decide to do the MMU setup again, then it means setting up the page-tables twice in the bootloaders!