MIPS cache cleanups

Move details of the L1 cache line sizes & total sizes into Kconfig, defaulting to 0 & using 0 to indicate that the value should be autodetected.

Signed-off-by Paul Burton <[email protected]>

---

 arch/mips/Kconfig                | 12 ++++++++++++
 arch/mips/lib/cache.c            |  2 +-
 arch/mips/lib/cache_init.S       |  6 +++---
 board/dbau1x00/Kconfig           |  9 +++++++++
 board/micronas/vct/Kconfig       |  9 +++++++++
 board/pb1x00/Kconfig             |  9 +++++++++
 board/qca/ap121/Kconfig          |  9 +++++++++
 board/qca/ap143/Kconfig          |  9 +++++++++
 board/qemu-mips/Kconfig          |  9 +++++++++
 board/tplink/wdr4300/Kconfig     |  9 +++++++++
 include/configs/ap121.h          |  5 -----
 include/configs/ap143.h          |  5 -----
 include/configs/dbau1x00.h       |  7 -------
 include/configs/pb1x00.h         |  6 ------
 include/configs/qemu-mips.h      |  7 -------
 include/configs/qemu-mips64.h    |  7 -------
 include/configs/tplink_wdr4300.h |  5 -----
 include/configs/vct.h            |  7 -------
 18 files changed, 79 insertions(+), 53 deletions(-)

arch/mips/Kconfig +2 -0

index abaeaf0..13f1164 100644
@@ -243,6 +243,18 @@ config SWAP_IO_SPACE
 config SYS_MIPS_CACHE_INIT_RAM_LOAD
 	bool
 
+config SYS_DCACHE_SIZE
+	hex

arch/mips/lib/cache.c +0 -0

 static inline unsigned long icache_line_size(void)
 {

arch/mips/lib/cache_init.S +3 -3

index 08b7c3a..e4a44ff 100644
@@ -99,14 +99,14 @@
  *
  */
 LEAF(mips_cache_reset)
-#ifdef CONFIG_SYS_ICACHE_SIZE
+#if CONFIG_SYS_ICACHE_SIZE != 0
 	li	t2, CONFIG_SYS_ICACHE_SIZE
 	li	t8, CONFIG_SYS_CACHELINE_SIZE
 #else
 	l1_info	t2, t8, MIPS_CONF1_IA_SHF
 #endif
 
-#ifdef CONFIG_SYS_DCACHE_SIZE
+#if CONFIG_SYS_DCACHE_SIZE != 0
 	li	t3, CONFIG_SYS_DCACHE_SIZE
 	li	t9, CONFIG_SYS_CACHELINE_SIZE
 #else

@@ -116,7 +116,7 @@ LEAF(mips_cache_reset)
 #ifdef CONFIG_SYS_MIPS_CACHE_INIT_RAM_LOAD
 
 	/* Determine the largest L1 cache size */
-#if defined(CONFIG_SYS_ICACHE_SIZE) && defined(CONFIG_SYS_DCACHE_SIZE)
+#if (CONFIG_SYS_ICACHE_SIZE != 0) && (CONFIG_SYS_DCACHE_SIZE != 0)
 #if CONFIG_SYS_ICACHE_SIZE > CONFIG_SYS_DCACHE_SIZE
 	li	v0, CONFIG_SYS_ICACHE_SIZE
 #else

board/dbau1x00/Kconfig +9 -0

index 342ec59..1715a28 100644
@@ -12,6 +12,15 @@ config SYS_CONFIG_NAME
 config SYS_TEXT_BASE
 	default 0xbfc00000
 
+config SYS_DCACHE_SIZE
+	default 16384
+
+config SYS_ICACHE_SIZE
+	default 16384
+
+config SYS_CACHELINE_SIZE
+	default 32
+
 menu "dbau1x00 board options"
 
 choice

board/micronas/vct/Kconfig +9 -0

index 535a77b..5bb6f03 100644
@@ -12,6 +12,15 @@ config SYS_CONFIG_NAME
 config SYS_TEXT_BASE
 	default 0x87000000
 
+config SYS_DCACHE_SIZE
+	default 16384
+
+config SYS_ICACHE_SIZE
+	default 16384
+
+config SYS_CACHELINE_SIZE
+	default 32
+
 menu "vct board options"
 
 choice

board/pb1x00/Kconfig +9 -0

index 236a410..27b2ef0 100644
@@ -12,4 +12,13 @@ config SYS_CONFIG_NAME
 config SYS_TEXT_BASE
 	default 0x83800000
 
+config SYS_DCACHE_SIZE
+	default 16384
+
+config SYS_ICACHE_SIZE
+	default 16384
+
+config SYS_CACHELINE_SIZE
+	default 32
+
 endif

board/qca/ap121/Kconfig +9 -0

index c3ecc8f..1ace0e5 100644
@@ -12,4 +12,13 @@ config SYS_CONFIG_NAME
 config SYS_TEXT_BASE
 	default 0x9f000000
 
+config SYS_DCACHE_SIZE
+	default 0x8000
+
+config SYS_ICACHE_SIZE
+	default 0x10000
+
+config SYS_CACHELINE_SIZE
+	default 32
+
 endif

board/qca/ap143/Kconfig +9 -0

index 5ea5d6f..ac73782 100644
@@ -12,4 +12,13 @@ config SYS_CONFIG_NAME
 config SYS_TEXT_BASE
 	default 0x9f000000
 
+config SYS_DCACHE_SIZE
+	default 0x8000
+
+config SYS_ICACHE_SIZE
+	default 0x10000
+
+config SYS_CACHELINE_SIZE
+	default 32
+
 endif

board/qemu-mips/Kconfig +9 -0

index 3de1f44..66957e7 100644
@@ -11,4 +11,13 @@ config SYS_TEXT_BASE
 	default 0xbfc00000 if 32BIT
 	default 0xffffffffbfc00000 if 64BIT
 
+config SYS_DCACHE_SIZE
+	default 16384
+
+config SYS_ICACHE_SIZE
+	default 16384
+
+config SYS_CACHELINE_SIZE
+	default 32
+
 endif

board/tplink/wdr4300/Kconfig +9 -0

index 65785bd..bbec2e5 100644
@@ -15,4 +15,13 @@ config SYS_CONFIG_NAME
 config SYS_TEXT_BASE
 	default 0xa1000000
 
+config SYS_DCACHE_SIZE
+	default 0x8000
+
+config SYS_ICACHE_SIZE
+	default 0x10000
+
+config SYS_CACHELINE_SIZE
+	default 32
+
 endif

include/configs/ap121.h +0 -5

index 6f69f31..f069d50 100644
@@ -15,11 +15,6 @@
 #define CONFIG_SYS_MHZ                  200
 #define CONFIG_SYS_MIPS_TIMER_FREQ      (CONFIG_SYS_MHZ * 1000000)
 
-/* Cache Configuration */
-#define CONFIG_SYS_DCACHE_SIZE          0x8000
-#define CONFIG_SYS_ICACHE_SIZE          0x10000
-#define CONFIG_SYS_CACHELINE_SIZE       32
-
 #define CONFIG_SYS_MONITOR_BASE         CONFIG_SYS_TEXT_BASE
 
 #define CONFIG_SYS_MALLOC_LEN           0x40000

include/configs/ap143.h +0 -5

index f907c02..e45f743 100644
@@ -15,11 +15,6 @@
 #define CONFIG_SYS_MHZ                  325
 #define CONFIG_SYS_MIPS_TIMER_FREQ      (CONFIG_SYS_MHZ * 1000000)
 
-/* Cache Configuration */
-#define CONFIG_SYS_DCACHE_SIZE          0x8000
-#define CONFIG_SYS_ICACHE_SIZE          0x10000
-#define CONFIG_SYS_CACHELINE_SIZE       32
-
 #define CONFIG_SYS_MONITOR_BASE         CONFIG_SYS_TEXT_BASE
 
 #define CONFIG_SYS_MALLOC_LEN           0x40000

include/configs/dbau1x00.h +0 -7

index 68d9e36..68ff025 100644
@@ -202,11 +202,4 @@
 #define CONFIG_SYS_ATA_ALT_OFFSET      0x0100
 #endif /* CONFIG_DBAU1550 */
 
-/*-----------------------------------------------------------------------
- * Cache Configuration
- */
-#define CONFIG_SYS_DCACHE_SIZE		16384
-#define CONFIG_SYS_ICACHE_SIZE		16384
-#define CONFIG_SYS_CACHELINE_SIZE	32
-
 #endif	/* __CONFIG_H */

include/configs/pb1x00.h +0 -6

index 869768a..b907419 100644
@@ -144,12 +144,6 @@
 #define CONFIG_SYS_ATA_ALT_OFFSET      0x0100
 
 #endif
-/*-----------------------------------------------------------------------
- * Cache Configuration
- */
-#define CONFIG_SYS_DCACHE_SIZE		16384
-#define CONFIG_SYS_ICACHE_SIZE		16384
-#define CONFIG_SYS_CACHELINE_SIZE	32
 
 /*
  * BOOTP options

include/configs/qemu-mips.h +0 -7

index 246ee01..f58fc4c 100644
@@ -132,11 +132,4 @@
 
 #define CONFIG_LZMA
 
-/*-----------------------------------------------------------------------
- * Cache Configuration
- */
-#define CONFIG_SYS_DCACHE_SIZE		16384
-#define CONFIG_SYS_ICACHE_SIZE		16384
-#define CONFIG_SYS_CACHELINE_SIZE	32
-
 #endif /* __CONFIG_H */

include/configs/qemu-mips64.h +0 -7

index 60a3a71..2190d16 100644
@@ -132,11 +132,4 @@
 
 #define CONFIG_LZMA
 
-/*-----------------------------------------------------------------------
- * Cache Configuration
- */
-#define CONFIG_SYS_DCACHE_SIZE		16384
-#define CONFIG_SYS_ICACHE_SIZE		16384
-#define CONFIG_SYS_CACHELINE_SIZE	32
-
 #endif /* __CONFIG_H */

include/configs/tplink_wdr4300.h +0 -5

index 09a69fe..6273711 100644
@@ -15,11 +15,6 @@
 #define CONFIG_SYS_MHZ			280
 #define CONFIG_SYS_MIPS_TIMER_FREQ	(CONFIG_SYS_MHZ * 1000000)
 
-/* Cache Configuration */
-#define CONFIG_SYS_DCACHE_SIZE		0x8000
-#define CONFIG_SYS_ICACHE_SIZE		0x10000
-#define CONFIG_SYS_CACHELINE_SIZE	32
-
 #define CONFIG_SYS_MONITOR_BASE		CONFIG_SYS_TEXT_BASE
 
 #define CONFIG_SYS_MALLOC_LEN		0x40000

include/configs/vct.h +0 -7

index 68eb089..cc5e354 100644
@@ -204,13 +204,6 @@
 #endif /* CONFIG_VCT_ONENAND */
 
 /*
- * Cache Configuration
- */
-#define CONFIG_SYS_DCACHE_SIZE		16384
-#define CONFIG_SYS_ICACHE_SIZE		16384
-#define CONFIG_SYS_CACHELINE_SIZE	32
-
-/*
  * I2C/EEPROM
  */
 #define CONFIG_SYS_I2C

Quoted 8 lines

arch/mips/lib/cache.c +0 -0

index 7482005..7695325 100644
@@ -9,7 +9,7 @@
 #include <asm/cacheops.h>
 #include <asm/mipsregs.h>
 

arch/mips/lib/cache.c line 12

Quoted 2 lines

arch/mips/lib/cache.c +1 -1

-#ifdef CONFIG_SYS_CACHELINE_SIZE
+#if CONFIG_SYS_CACHELINE_SIZE != 0

Wouldn't it make more sense to introduce something like CONFIG_HAVE_CACHE_SUPPORT instead , so you don't need this #if CONFIG_FOO != 0 construct all over the place ?

Cool stuff otherwise, thanks!

Quoted 8 lines · Marek Vasut

Quoted 8 lines

arch/mips/lib/cache.c +0 -0

index 7482005..7695325 100644
@@ -9,7 +9,7 @@
 #include <asm/cacheops.h>
 #include <asm/mipsregs.h>
 

Quoted 6 lines

arch/mips/lib/cache.c line 12

Quoted 2 lines

arch/mips/lib/cache.c +1 -1

-#ifdef CONFIG_SYS_CACHELINE_SIZE
+#if CONFIG_SYS_CACHELINE_SIZE != 0

Wouldn't it make more sense to introduce something like CONFIG_HAVE_CACHE_SUPPORT instead , so you don't need this #if CONFIG_FOO != 0 construct all over the place ?

Hi Marek,

It's not about whether cache support is present (we always build cache support), it's about whether we are hardcoding the sizes of the caches or detecting them at runtime. The latter is especially useful on FPGA-based platforms like Malta where the CPU (& caches) can change. I suppose I could add something like CONFIG_SYS_CACHE_SIZE_AUTO, but I still think it would be cleanest to have that default to a value based upon whether a board has set a non-zero size for any of the caches.

Quoted 1 line

Cool stuff otherwise, thanks!

Thanks,
Paul

Quoted 18 lines · Paul Burton

Quoted 8 lines · Marek Vasut

Quoted 8 lines

arch/mips/lib/cache.c +0 -0

index 7482005..7695325 100644
@@ -9,7 +9,7 @@
 #include <asm/cacheops.h>
 #include <asm/mipsregs.h>
 

Quoted 6 lines

arch/mips/lib/cache.c line 12

Quoted 2 lines

arch/mips/lib/cache.c +1 -1

-#ifdef CONFIG_SYS_CACHELINE_SIZE
+#if CONFIG_SYS_CACHELINE_SIZE != 0

Wouldn't it make more sense to introduce something like CONFIG_HAVE_CACHE_SUPPORT instead , so you don't need this #if CONFIG_FOO != 0 construct all over the place ?

Hi Marek,

Quoted 7 lines

It's not about whether cache support is present (we always build cache support), it's about whether we are hardcoding the sizes of the caches or detecting them at runtime. The latter is especially useful on FPGA-based platforms like Malta where the CPU (& caches) can change. I suppose I could add something like CONFIG_SYS_CACHE_SIZE_AUTO, but I still think it would be cleanest to have that default to a value based upon whether a board has set a non-zero size for any of the caches.

Auto-detecting cacheline size is cool, but that'd need much more work. Just grep through drivers/ for CONFIG_SYS_CACHELINE_SIZE and you'll see a lot of code depends on non-zero value in it. Setting it to zero will just bite you at some point in a nasty way.

Quoted 27 lines · Marek Vasut

Quoted 25 lines · Paul Burton

Quoted 8 lines · Marek Vasut

Quoted 8 lines

arch/mips/lib/cache.c +0 -0

index 7482005..7695325 100644
@@ -9,7 +9,7 @@
 #include <asm/cacheops.h>
 #include <asm/mipsregs.h>
 

Quoted 6 lines

arch/mips/lib/cache.c line 12

Quoted 2 lines

arch/mips/lib/cache.c +1 -1

-#ifdef CONFIG_SYS_CACHELINE_SIZE
+#if CONFIG_SYS_CACHELINE_SIZE != 0

Wouldn't it make more sense to introduce something like CONFIG_HAVE_CACHE_SUPPORT instead , so you don't need this #if CONFIG_FOO != 0 construct all over the place ?

Hi Marek,

It's not about whether cache support is present (we always build cache support), it's about whether we are hardcoding the sizes of the caches or detecting them at runtime. The latter is especially useful on FPGA-based platforms like Malta where the CPU (& caches) can change. I suppose I could add something like CONFIG_SYS_CACHE_SIZE_AUTO, but I still think it would be cleanest to have that default to a value based upon whether a board has set a non-zero size for any of the caches.

Quoted 4 lines

Auto-detecting cacheline size is cool, but that'd need much more work. Just grep through drivers/ for CONFIG_SYS_CACHELINE_SIZE and you'll see a lot of code depends on non-zero value in it. Setting it to zero will just bite you at some point in a nasty way.

Hi Marek,

We're already using the cache size auto-detection on Malta, and on 2 other FPGA-based boards internally. I've submitted v2 which preserves CONFIG_SYS_CACHELINE_SIZE as a synonym of ARCH_DMA_MINALIGN for the drivers that are still using it.

Thanks,
Paul

Quoted 1 line

Hi Marek,

Hi!

Quoted 4 lines

We're already using the cache size auto-detection on Malta, and on 2 other FPGA-based boards internally. I've submitted v2 which preserves CONFIG_SYS_CACHELINE_SIZE as a synonym of ARCH_DMA_MINALIGN for the drivers that are still using it.

That's a good workaround for now. Would you be interested in fixing this runtime cache configuration properly ?

Off-topic: Is malta that mipsfpga or is that something else ? Can I synthesise that mipsfpga into some altera FPGA ? If so, which one is a good pick ?

Quoted 3 lines · Marek Vasut

Quoted 1 line

Hi Marek,

Hi!

Hi again Marek :)

Quoted 7 lines

Quoted 4 lines

We're already using the cache size auto-detection on Malta, and on 2 other FPGA-based boards internally. I've submitted v2 which preserves CONFIG_SYS_CACHELINE_SIZE as a synonym of ARCH_DMA_MINALIGN for the drivers that are still using it.

That's a good workaround for now. Would you be interested in fixing this runtime cache configuration properly ?

We use the runtime cache size detection on Malta, on SEAD3 & on Boston. The latter 2 aren't upstream yet of course, but all 3 are FPGA-based platforms used to develop, test & showcase new CPUs. As such the CPU (& by association the cache) can change just by flashing a new bitfile to the board, and it's very convenient for a single build of U-Boot to work regardless of the bitfile in use.

It all works fine so long as drivers are well behaved, and nothing on any of those platforms uses CONFIG_SYS_CACHELINE_SIZE. I'm not sure there's anything to fix really apart from drivers should possibly move to use ARCH_DMA_MINALIGN, but I wouldn't feel comfortable doing a global replacement as I don't have a way to test many boards for other architectures.

Quoted 3 lines

Off-topic: Is malta that mipsfpga or is that something else ? Can I synthesise that mipsfpga into some altera FPGA ? If so, which one is a good pick ?

I don't know much about the MIPSfpga project to be honest, but I can forward your question to someone who does.

Thanks,
Paul

Quoted 28 lines · Marek Vasut

Quoted 27 lines · Paul Burton

Quoted 25 lines · Marek Vasut

Quoted 8 lines

Quoted 8 lines

arch/mips/lib/cache.c +0 -0

index 7482005..7695325 100644
@@ -9,7 +9,7 @@
 #include <asm/cacheops.h>
 #include <asm/mipsregs.h>
 

Quoted 6 lines

arch/mips/lib/cache.c line 12

Quoted 2 lines

arch/mips/lib/cache.c +1 -1

-#ifdef CONFIG_SYS_CACHELINE_SIZE
+#if CONFIG_SYS_CACHELINE_SIZE != 0

Wouldn't it make more sense to introduce something like CONFIG_HAVE_CACHE_SUPPORT instead , so you don't need this #if CONFIG_FOO != 0 construct all over the place ?

Hi Marek,

It's not about whether cache support is present (we always build cache support), it's about whether we are hardcoding the sizes of the caches or detecting them at runtime. The latter is especially useful on FPGA-based platforms like Malta where the CPU (& caches) can change. I suppose I could add something like CONFIG_SYS_CACHE_SIZE_AUTO, but I still think it would be cleanest to have that default to a value based upon whether a board has set a non-zero size for any of the caches.

Quoted 6 lines

Quoted 4 lines

Auto-detecting cacheline size is cool, but that'd need much more work. Just grep through drivers/ for CONFIG_SYS_CACHELINE_SIZE and you'll see a lot of code depends on non-zero value in it. Setting it to zero will just bite you at some point in a nasty way.

Hi Marek,

Hi!

Quoted 4 lines

We're already using the cache size auto-detection on Malta, and on 2 other FPGA-based boards internally. I've submitted v2 which preserves CONFIG_SYS_CACHELINE_SIZE as a synonym of ARCH_DMA_MINALIGN for the drivers that are still using it.

That's a good workaround for now. Would you be interested in fixing this runtime cache configuration properly ?

Am 27.05.2016 um 16:40 schrieb Marek Vasut:

Quoted 43 lines · Paul Burton

Quoted 28 lines · Marek Vasut

Quoted 27 lines · Paul Burton

Quoted 25 lines · Marek Vasut

Quoted 8 lines

Quoted 8 lines

arch/mips/lib/cache.c +0 -0

index 7482005..7695325 100644
@@ -9,7 +9,7 @@
 #include <asm/cacheops.h>
 #include <asm/mipsregs.h>
 

Quoted 6 lines

arch/mips/lib/cache.c line 12

Quoted 2 lines

arch/mips/lib/cache.c +1 -1

-#ifdef CONFIG_SYS_CACHELINE_SIZE
+#if CONFIG_SYS_CACHELINE_SIZE != 0

Wouldn't it make more sense to introduce something like CONFIG_HAVE_CACHE_SUPPORT instead , so you don't need this #if CONFIG_FOO != 0 construct all over the place ?

Hi Marek,

It's not about whether cache support is present (we always build cache support), it's about whether we are hardcoding the sizes of the caches or detecting them at runtime. The latter is especially useful on FPGA-based platforms like Malta where the CPU (& caches) can change. I suppose I could add something like CONFIG_SYS_CACHE_SIZE_AUTO, but I still think it would be cleanest to have that default to a value based upon whether a board has set a non-zero size for any of the caches.

Quoted 6 lines

Quoted 4 lines

Auto-detecting cacheline size is cool, but that'd need much more work. Just grep through drivers/ for CONFIG_SYS_CACHELINE_SIZE and you'll see a lot of code depends on non-zero value in it. Setting it to zero will just bite you at some point in a nasty way.

Hi Marek,

Hi!

Quoted 4 lines

We're already using the cache size auto-detection on Malta, and on 2 other FPGA-based boards internally. I've submitted v2 which preserves CONFIG_SYS_CACHELINE_SIZE as a synonym of ARCH_DMA_MINALIGN for the drivers that are still using it.

Quoted 3 lines

That's a good workaround for now. Would you be interested in fixing this runtime cache configuration properly ?

there is no runtime configuration for ARCH_DMA_MINALIGN. This value is defined at compile-time by Kconfig option MIPS_L1_CACHE_SHIFT. This is similar to what Linux is doing.

Off-topic: Is malta that mipsfpga or is that something else ? Can I synthesise that mipsfpga into some altera FPGA ? If so, which one is a good pick ?

Move details of the L1 cache line sizes & total sizes into Kconfig, defaulting to 0 & using 0 to indicate that the value should be autodetected.

Signed-off-by Paul Burton <[email protected]>

---

 arch/mips/Kconfig                | 12 ++++++++++++
 arch/mips/lib/cache.c            |  2 +-
 arch/mips/lib/cache_init.S       |  6 +++---
 board/dbau1x00/Kconfig           |  9 +++++++++
 board/micronas/vct/Kconfig       |  9 +++++++++
 board/pb1x00/Kconfig             |  9 +++++++++
 board/qca/ap121/Kconfig          |  9 +++++++++
 board/qca/ap143/Kconfig          |  9 +++++++++
 board/qemu-mips/Kconfig          |  9 +++++++++
 board/tplink/wdr4300/Kconfig     |  9 +++++++++
 include/configs/ap121.h          |  5 -----

 include/configs/ap143.h          |  5 -----
 include/configs/dbau1x00.h       |  7 -------
 include/configs/pb1x00.h         |  6 ------
 include/configs/qemu-mips.h      |  7 -------
 include/configs/qemu-mips64.h    |  7 -------
 include/configs/tplink_wdr4300.h |  5 -----
 include/configs/vct.h            |  7 -------
 18 files changed, 79 insertions(+), 53 deletions(-)

Allow L1 Icache & L1 Dcache line size to be specified separately, since there's no architectural mandate that they be the same. The [id]cache_line_size functions are tidied up to take advantage of the fact that the Kconfig entries are always present to simply check them for zero rather than needing to #ifdef on their presence.

Signed-off-by Paul Burton <[email protected]>

---

 arch/mips/Kconfig            |  6 +++++-
 arch/mips/lib/cache.c        | 22 +++++++---------------
 arch/mips/lib/cache_init.S   |  4 ++--
 board/dbau1x00/Kconfig       |  5 ++++-
 board/micronas/vct/Kconfig   |  5 ++++-
 board/pb1x00/Kconfig         |  5 ++++-
 board/qca/ap121/Kconfig      |  5 ++++-
 board/qca/ap143/Kconfig      |  5 ++++-
 board/qemu-mips/Kconfig      |  5 ++++-
 board/tplink/wdr4300/Kconfig |  5 ++++-
 10 files changed, 42 insertions(+), 25 deletions(-)

arch/mips/Kconfig +0 -0

index 13f1164..8af8799 100644
@@ -247,11 +247,15 @@ config SYS_DCACHE_SIZE
 	hex
 	default 0
 

arch/mips/Kconfig +0 -0

 	hex
 	default 0
 

arch/mips/lib/cache.c +7 -15

index 7695325..2bb91c6 100644
@@ -9,23 +9,13 @@
 #include <asm/cacheops.h>
 #include <asm/mipsregs.h>
 
-#if CONFIG_SYS_CACHELINE_SIZE != 0
-
 static inline unsigned long icache_line_size(void)
 {
-	return CONFIG_SYS_CACHELINE_SIZE;
-}
-
-static inline unsigned long dcache_line_size(void)
-{
-	return CONFIG_SYS_CACHELINE_SIZE;
-}
+	unsigned long conf1, il;
 
-#else /* !CONFIG_SYS_CACHELINE_SIZE */
+	if (CONFIG_SYS_ICACHE_LINE_SIZE != 0)
+		return CONFIG_SYS_ICACHE_LINE_SIZE;
 
-static inline unsigned long icache_line_size(void)
-{
-	unsigned long conf1, il;
 	conf1 = read_c0_config1();
 	il = (conf1 & MIPS_CONF1_IL) >> MIPS_CONF1_IL_SHF;
 	if (!il)

@@ -36,6 +26,10 @@ static inline unsigned long icache_line_size(void)
 static inline unsigned long dcache_line_size(void)
 {
 	unsigned long conf1, dl;
+
+	if (CONFIG_SYS_DCACHE_LINE_SIZE != 0)
+		return CONFIG_SYS_DCACHE_LINE_SIZE;
+
 	conf1 = read_c0_config1();
 	dl = (conf1 & MIPS_CONF1_DL) >> MIPS_CONF1_DL_SHF;
 	if (!dl)

@@ -43,8 +37,6 @@ static inline unsigned long dcache_line_size(void)
 	return 2 << dl;
 }
 
-#endif /* !CONFIG_SYS_CACHELINE_SIZE */
-
 void flush_cache(ulong start_addr, ulong size)
 {
 	unsigned long ilsize = icache_line_size();

arch/mips/lib/cache_init.S +2 -2

index e4a44ff..41cb3a6 100644
@@ -101,14 +101,14 @@
 LEAF(mips_cache_reset)
 #if CONFIG_SYS_ICACHE_SIZE != 0
 	li	t2, CONFIG_SYS_ICACHE_SIZE
-	li	t8, CONFIG_SYS_CACHELINE_SIZE
+	li	t8, CONFIG_SYS_ICACHE_LINE_SIZE
 #else
 	l1_info	t2, t8, MIPS_CONF1_IA_SHF
 #endif
 
 #if CONFIG_SYS_DCACHE_SIZE != 0
 	li	t3, CONFIG_SYS_DCACHE_SIZE
-	li	t9, CONFIG_SYS_CACHELINE_SIZE
+	li	t9, CONFIG_SYS_DCACHE_LINE_SIZE
 #else
 	l1_info	t3, t9, MIPS_CONF1_DA_SHF
 #endif

board/dbau1x00/Kconfig +4 -1

index 1715a28..448176d 100644
@@ -15,10 +15,13 @@ config SYS_TEXT_BASE
 config SYS_DCACHE_SIZE
 	default 16384
 
+config SYS_DCACHE_LINE_SIZE
+	default 32
+
 config SYS_ICACHE_SIZE
 	default 16384
 
-config SYS_CACHELINE_SIZE
+config SYS_ICACHE_LINE_SIZE
 	default 32
 
 menu "dbau1x00 board options"

board/micronas/vct/Kconfig +4 -1

index 5bb6f03..df7c029 100644
@@ -15,10 +15,13 @@ config SYS_TEXT_BASE
 config SYS_DCACHE_SIZE
 	default 16384
 
+config SYS_DCACHE_LINE_SIZE
+	default 32
+
 config SYS_ICACHE_SIZE
 	default 16384
 
-config SYS_CACHELINE_SIZE
+config SYS_ICACHE_LINE_SIZE
 	default 32
 
 menu "vct board options"

board/pb1x00/Kconfig +4 -1

index 27b2ef0..ef8905d 100644
@@ -15,10 +15,13 @@ config SYS_TEXT_BASE
 config SYS_DCACHE_SIZE
 	default 16384
 
+config SYS_DCACHE_LINE_SIZE
+	default 32
+
 config SYS_ICACHE_SIZE
 	default 16384
 
-config SYS_CACHELINE_SIZE
+config SYS_ICACHE_LINE_SIZE
 	default 32
 
 endif

board/qca/ap121/Kconfig +4 -1

index 1ace0e5..09a06d8 100644
@@ -15,10 +15,13 @@ config SYS_TEXT_BASE
 config SYS_DCACHE_SIZE
 	default 0x8000
 
+config SYS_DCACHE_LINE_SIZE
+	default 32
+
 config SYS_ICACHE_SIZE
 	default 0x10000
 
-config SYS_CACHELINE_SIZE
+config SYS_ICACHE_LINE_SIZE
 	default 32
 
 endif

board/qca/ap143/Kconfig +4 -1

index ac73782..f3d658d 100644
@@ -15,10 +15,13 @@ config SYS_TEXT_BASE
 config SYS_DCACHE_SIZE
 	default 0x8000
 
+config SYS_DCACHE_LINE_SIZE
+	default 32
+
 config SYS_ICACHE_SIZE
 	default 0x10000
 
-config SYS_CACHELINE_SIZE
+config SYS_ICACHE_LINE_SIZE
 	default 32
 
 endif

board/qemu-mips/Kconfig +4 -1

index 66957e7..e696a12 100644
@@ -14,10 +14,13 @@ config SYS_TEXT_BASE
 config SYS_DCACHE_SIZE
 	default 16384
 
+config SYS_DCACHE_LINE_SIZE
+	default 32
+
 config SYS_ICACHE_SIZE
 	default 16384
 
-config SYS_CACHELINE_SIZE
+config SYS_ICACHE_LINE_SIZE
 	default 32
 
 endif

board/tplink/wdr4300/Kconfig +4 -1

index bbec2e5..86afd76 100644
@@ -18,10 +18,13 @@ config SYS_TEXT_BASE
 config SYS_DCACHE_SIZE
 	default 0x8000
 
+config SYS_DCACHE_LINE_SIZE
+	default 32
+
 config SYS_ICACHE_SIZE
 	default 0x10000
 
-config SYS_CACHELINE_SIZE
+config SYS_ICACHE_LINE_SIZE
 	default 32
 
 endif

Quoted 37 lines

Allow L1 Icache & L1 Dcache line size to be specified separately, since there's no architectural mandate that they be the same. The [id]cache_line_size functions are tidied up to take advantage of the fact that the Kconfig entries are always present to simply check them for zero rather than needing to #ifdef on their presence.

Signed-off-by Paul Burton <[email protected]>

---

 arch/mips/Kconfig            |  6 +++++-
 arch/mips/lib/cache.c        | 22 +++++++---------------
 arch/mips/lib/cache_init.S   |  4 ++--
 board/dbau1x00/Kconfig       |  5 ++++-
 board/micronas/vct/Kconfig   |  5 ++++-
 board/pb1x00/Kconfig         |  5 ++++-
 board/qca/ap121/Kconfig      |  5 ++++-
 board/qca/ap143/Kconfig      |  5 ++++-
 board/qemu-mips/Kconfig      |  5 ++++-
 board/tplink/wdr4300/Kconfig |  5 ++++-
 10 files changed, 42 insertions(+), 25 deletions(-)

arch/mips/Kconfig +4 -0

index 13f1164..8af8799 100644
@@ -247,11 +247,15 @@ config SYS_DCACHE_SIZE
 	hex
 	default 0
 
+config SYS_DCACHE_LINE_SIZE
+	hex
+	default 0
+
 config SYS_ICACHE_SIZE
 	hex
 	default 0
 

arch/mips/Kconfig config SYS_DCACHE_SIZE · line 258

Quoted 2 lines

arch/mips/Kconfig +1 -1

-config SYS_CACHELINE_SIZE
+config SYS_ICACHE_LINE_SIZE

Be careful here, CONFIG_SYS_CACHELINE_SIZE is established all over the u-boot, so this might cause build breakage. You should keep some compatibility CONFIG_SYS_CACHELINE_SIZE entry and set it to default to SYS_DCACHE_LINE_SIZE , since it's mostly used for DMA anyway.

Allow L1 Icache & L1 Dcache line size to be specified separately, since there's no architectural mandate that they be the same. The [id]cache_line_size functions are tidied up to take advantage of the fact that the Kconfig entries are always present to simply check them for zero rather than needing to #ifdef on their presence.

Signed-off-by Paul Burton <[email protected]>

---

 arch/mips/Kconfig            |  6 +++++-
 arch/mips/lib/cache.c        | 22 +++++++---------------
 arch/mips/lib/cache_init.S   |  4 ++--
 board/dbau1x00/Kconfig       |  5 ++++-
 board/micronas/vct/Kconfig   |  5 ++++-
 board/pb1x00/Kconfig         |  5 ++++-
 board/qca/ap121/Kconfig      |  5 ++++-
 board/qca/ap143/Kconfig      |  5 ++++-
 board/qemu-mips/Kconfig      |  5 ++++-
 board/tplink/wdr4300/Kconfig |  5 ++++-
 10 files changed, 42 insertions(+), 25 deletions(-)

The various cache maintenance routines perform a number of loops over cache lines. Rather than duplicate the code for performing such loops, abstract it out into a new cache_loop macro which performs an arbitrary number of cache ops on a range of addresses. This reduces duplication in the existing L1 cache maintenance code & will allow for not adding further duplication when introducing L2 cache support.

Signed-off-by Paul Burton <[email protected]>

---

 arch/mips/lib/cache.c | 59 ++++++++++++++++-----------------------------------
 1 file changed, 18 insertions(+), 41 deletions(-)

arch/mips/lib/cache.c +0 -0

index 2bb91c6..320335c 100644

arch/mips/lib/cache.c +14 -41

+	unsigned int i;							\
+									\
+	for (; addr <= aend; addr += lsize) {				\
+		for (i = 0; i < ARRAY_SIZE(cache_ops); i++)		\
+			mips_cache(cache_ops[i], addr);			\
+	}								\
+} while (0)
+
 void flush_cache(ulong start_addr, ulong size)
 {
 	unsigned long ilsize = icache_line_size();
 	unsigned long dlsize = dcache_line_size();
-	const void *addr, *aend;
 
 	/* aend will be miscalculated when size is zero, so we return here */
 	if (size == 0)
 		return;
 
-	addr = (const void *)(start_addr & ~(dlsize - 1));
-	aend = (const void *)((start_addr + size - 1) & ~(dlsize - 1));
-
 	if (ilsize == dlsize) {
 		/* flush I-cache & D-cache simultaneously */
-		while (1) {
-			mips_cache(HIT_WRITEBACK_INV_D, addr);
-			mips_cache(HIT_INVALIDATE_I, addr);
-			if (addr == aend)
-				break;
-			addr += dlsize;
-		}
+		cache_loop(start_addr, start_addr + size, ilsize,
+			   HIT_WRITEBACK_INV_D, HIT_INVALIDATE_I);
 		return;
 	}
 
 	/* flush D-cache */
-	while (1) {
-		mips_cache(HIT_WRITEBACK_INV_D, addr);
-		if (addr == aend)
-			break;
-		addr += dlsize;
-	}
+	cache_loop(start_addr, start_addr + size, dlsize, HIT_WRITEBACK_INV_D);
 
 	/* flush I-cache */
-	addr = (const void *)(start_addr & ~(ilsize - 1));
-	aend = (const void *)((start_addr + size - 1) & ~(ilsize - 1));
-	while (1) {
-		mips_cache(HIT_INVALIDATE_I, addr);
-		if (addr == aend)
-			break;
-		addr += ilsize;
-	}
+	cache_loop(start_addr, start_addr + size, ilsize, HIT_INVALIDATE_I);
 }
 
 void flush_dcache_range(ulong start_addr, ulong stop)
 {
 	unsigned long lsize = dcache_line_size();
-	const void *addr = (const void *)(start_addr & ~(lsize - 1));
-	const void *aend = (const void *)((stop - 1) & ~(lsize - 1));
 
 	/* aend will be miscalculated when size is zero, so we return here */
 	if (start_addr == stop)
 		return;
 
-	while (1) {
-		mips_cache(HIT_WRITEBACK_INV_D, addr);
-		if (addr == aend)
-			break;
-		addr += lsize;
-	}
+	cache_loop(start_addr, stop, lsize, HIT_WRITEBACK_INV_D);
 }
 
 void invalidate_dcache_range(ulong start_addr, ulong stop)
 {
 	unsigned long lsize = dcache_line_size();
-	const void *addr = (const void *)(start_addr & ~(lsize - 1));
-	const void *aend = (const void *)((stop - 1) & ~(lsize - 1));
 
 	/* aend will be miscalculated when size is zero, so we return here */
 	if (start_addr == stop)
 		return;
 
-	while (1) {
-		mips_cache(HIT_INVALIDATE_D, addr);
-		if (addr == aend)
-			break;
-		addr += lsize;
-	}
+	cache_loop(start_addr, stop, lsize, HIT_INVALIDATE_I);
 }

Quoted 22 lines · Paul Burton

The various cache maintenance routines perform a number of loops over cache lines. Rather than duplicate the code for performing such loops, abstract it out into a new cache_loop macro which performs an arbitrary number of cache ops on a range of addresses. This reduces duplication in the existing L1 cache maintenance code & will allow for not adding further duplication when introducing L2 cache support.

Signed-off-by Paul Burton <[email protected]>

---

 arch/mips/lib/cache.c | 59 ++++++++++++++++-----------------------------------
 1 file changed, 18 insertions(+), 41 deletions(-)

arch/mips/lib/cache.c +0 -0

index 2bb91c6..320335c 100644
@@ -37,82 +37,59 @@ static inline unsigned long dcache_line_size(void)
 	return 2 << dl;
 }
 

arch/mips/lib/cache.c static inline unsigned long dcache_line_size(void) · line 43

Quoted 4 lines

arch/mips/lib/cache.c +4 -0

+#define cache_loop(start, end, lsize, ops...) do {			\
+	const void *addr = (const void *)(start & ~(lsize - 1));	\
+	const void *aend = (const void *)((end - 1) & ~(lsize - 1));	\
+	const unsigned int cache_ops[] = { ops };			\

Can't you turn this into a function instead and pass some flags to select the ops instead ?

Quoted 95 lines

arch/mips/lib/cache.c +14 -41

+	unsigned int i;							\
+									\
+	for (; addr <= aend; addr += lsize) {				\
+		for (i = 0; i < ARRAY_SIZE(cache_ops); i++)		\
+			mips_cache(cache_ops[i], addr);			\
+	}								\
+} while (0)
+
 void flush_cache(ulong start_addr, ulong size)
 {
 	unsigned long ilsize = icache_line_size();
 	unsigned long dlsize = dcache_line_size();
-	const void *addr, *aend;
 
 	/* aend will be miscalculated when size is zero, so we return here */
 	if (size == 0)
 		return;
 
-	addr = (const void *)(start_addr & ~(dlsize - 1));
-	aend = (const void *)((start_addr + size - 1) & ~(dlsize - 1));
-
 	if (ilsize == dlsize) {
 		/* flush I-cache & D-cache simultaneously */
-		while (1) {
-			mips_cache(HIT_WRITEBACK_INV_D, addr);
-			mips_cache(HIT_INVALIDATE_I, addr);
-			if (addr == aend)
-				break;
-			addr += dlsize;
-		}
+		cache_loop(start_addr, start_addr + size, ilsize,
+			   HIT_WRITEBACK_INV_D, HIT_INVALIDATE_I);
 		return;
 	}
 
 	/* flush D-cache */
-	while (1) {
-		mips_cache(HIT_WRITEBACK_INV_D, addr);
-		if (addr == aend)
-			break;
-		addr += dlsize;
-	}
+	cache_loop(start_addr, start_addr + size, dlsize, HIT_WRITEBACK_INV_D);
 
 	/* flush I-cache */
-	addr = (const void *)(start_addr & ~(ilsize - 1));
-	aend = (const void *)((start_addr + size - 1) & ~(ilsize - 1));
-	while (1) {
-		mips_cache(HIT_INVALIDATE_I, addr);
-		if (addr == aend)
-			break;
-		addr += ilsize;
-	}
+	cache_loop(start_addr, start_addr + size, ilsize, HIT_INVALIDATE_I);
 }
 
 void flush_dcache_range(ulong start_addr, ulong stop)
 {
 	unsigned long lsize = dcache_line_size();
-	const void *addr = (const void *)(start_addr & ~(lsize - 1));
-	const void *aend = (const void *)((stop - 1) & ~(lsize - 1));
 
 	/* aend will be miscalculated when size is zero, so we return here */
 	if (start_addr == stop)
 		return;
 
-	while (1) {
-		mips_cache(HIT_WRITEBACK_INV_D, addr);
-		if (addr == aend)
-			break;
-		addr += lsize;
-	}
+	cache_loop(start_addr, stop, lsize, HIT_WRITEBACK_INV_D);
 }
 
 void invalidate_dcache_range(ulong start_addr, ulong stop)
 {
 	unsigned long lsize = dcache_line_size();
-	const void *addr = (const void *)(start_addr & ~(lsize - 1));
-	const void *aend = (const void *)((stop - 1) & ~(lsize - 1));
 
 	/* aend will be miscalculated when size is zero, so we return here */
 	if (start_addr == stop)
 		return;
 
-	while (1) {
-		mips_cache(HIT_INVALIDATE_D, addr);
-		if (addr == aend)
-			break;
-		addr += lsize;
-	}
+	cache_loop(start_addr, stop, lsize, HIT_INVALIDATE_I);
 }

Signature · 2 lines

Best regards,
Marek Vasut

Quoted 23 lines · Marek Vasut

Quoted 22 lines · Paul Burton

The various cache maintenance routines perform a number of loops over cache lines. Rather than duplicate the code for performing such loops, abstract it out into a new cache_loop macro which performs an arbitrary number of cache ops on a range of addresses. This reduces duplication in the existing L1 cache maintenance code & will allow for not adding further duplication when introducing L2 cache support.

Signed-off-by Paul Burton <[email protected]>

---

 arch/mips/lib/cache.c | 59 ++++++++++++++++-----------------------------------
 1 file changed, 18 insertions(+), 41 deletions(-)

arch/mips/lib/cache.c +0 -0

index 2bb91c6..320335c 100644
@@ -37,82 +37,59 @@ static inline unsigned long dcache_line_size(void)
 	return 2 << dl;
 }
 

Quoted 7 lines

arch/mips/lib/cache.c static inline unsigned long dcache_line_size(void) · line 43

Quoted 4 lines

arch/mips/lib/cache.c +4 -0

+#define cache_loop(start, end, lsize, ops...) do {			\
+	const void *addr = (const void *)(start & ~(lsize - 1));	\
+	const void *aend = (const void *)((end - 1) & ~(lsize - 1));	\
+	const unsigned int cache_ops[] = { ops };			\

Can't you turn this into a function instead and pass some flags to select the ops instead ?

Hi Marek,

The problem is that then both that function & its callers would need to know about the types of cache ops, and there'd need to be some mapping from flags to the actual cache op values (of which there'll be a couple more once L2 support is added).

I think it's much simpler & cleaner to
just go with the macro in this case - it keeps knowledge of which cache ops to use with the callers, and it ends up generating the same code as before (ie. the inner loop gets unrolled to just the relevant cache instructions).

Thanks,
Paul

Quoted 100 lines

Quoted 95 lines

arch/mips/lib/cache.c +14 -41

+	unsigned int i;							\
+									\
+	for (; addr <= aend; addr += lsize) {				\
+		for (i = 0; i < ARRAY_SIZE(cache_ops); i++)		\
+			mips_cache(cache_ops[i], addr);			\
+	}								\
+} while (0)
+
 void flush_cache(ulong start_addr, ulong size)
 {
 	unsigned long ilsize = icache_line_size();
 	unsigned long dlsize = dcache_line_size();
-	const void *addr, *aend;
 
 	/* aend will be miscalculated when size is zero, so we return here */
 	if (size == 0)
 		return;
 
-	addr = (const void *)(start_addr & ~(dlsize - 1));
-	aend = (const void *)((start_addr + size - 1) & ~(dlsize - 1));
-
 	if (ilsize == dlsize) {
 		/* flush I-cache & D-cache simultaneously */
-		while (1) {
-			mips_cache(HIT_WRITEBACK_INV_D, addr);
-			mips_cache(HIT_INVALIDATE_I, addr);
-			if (addr == aend)
-				break;
-			addr += dlsize;
-		}
+		cache_loop(start_addr, start_addr + size, ilsize,
+			   HIT_WRITEBACK_INV_D, HIT_INVALIDATE_I);
 		return;
 	}
 
 	/* flush D-cache */
-	while (1) {
-		mips_cache(HIT_WRITEBACK_INV_D, addr);
-		if (addr == aend)
-			break;
-		addr += dlsize;
-	}
+	cache_loop(start_addr, start_addr + size, dlsize, HIT_WRITEBACK_INV_D);
 
 	/* flush I-cache */
-	addr = (const void *)(start_addr & ~(ilsize - 1));
-	aend = (const void *)((start_addr + size - 1) & ~(ilsize - 1));
-	while (1) {
-		mips_cache(HIT_INVALIDATE_I, addr);
-		if (addr == aend)
-			break;
-		addr += ilsize;
-	}
+	cache_loop(start_addr, start_addr + size, ilsize, HIT_INVALIDATE_I);
 }
 
 void flush_dcache_range(ulong start_addr, ulong stop)
 {
 	unsigned long lsize = dcache_line_size();
-	const void *addr = (const void *)(start_addr & ~(lsize - 1));
-	const void *aend = (const void *)((stop - 1) & ~(lsize - 1));
 
 	/* aend will be miscalculated when size is zero, so we return here */
 	if (start_addr == stop)
 		return;
 
-	while (1) {
-		mips_cache(HIT_WRITEBACK_INV_D, addr);
-		if (addr == aend)
-			break;
-		addr += lsize;
-	}
+	cache_loop(start_addr, stop, lsize, HIT_WRITEBACK_INV_D);
 }
 
 void invalidate_dcache_range(ulong start_addr, ulong stop)
 {
 	unsigned long lsize = dcache_line_size();
-	const void *addr = (const void *)(start_addr & ~(lsize - 1));
-	const void *aend = (const void *)((stop - 1) & ~(lsize - 1));
 
 	/* aend will be miscalculated when size is zero, so we return here */
 	if (start_addr == stop)
 		return;
 
-	while (1) {
-		mips_cache(HIT_INVALIDATE_D, addr);
-		if (addr == aend)
-			break;
-		addr += lsize;
-	}
+	cache_loop(start_addr, stop, lsize, HIT_INVALIDATE_I);
 }

Signature · 2 lines

Best regards,
Marek Vasut

Quoted 4 lines

The problem is that then both that function & its callers would need to know about the types of cache ops, and there'd need to be some mapping from flags to the actual cache op values (of which there'll be a couple more once L2 support is added).

But the callers already know which ops they must invoke, right ? You can just have enum {} for the bit definitions. It's all local to this file, so I don't see a problem.

Quoted 5 lines

I think it's much simpler & cleaner to
just go with the macro in this case - it keeps knowledge of which cache ops to use with the callers, and it ends up generating the same code as before (ie. the inner loop gets unrolled to just the relevant cache instructions).

I would expect GCC to take care of inlining the code, but you can always use the always_inline directive if that's your concern. Even cleaner way might be to use CONFIG_CC_OPTIMIZE_LIBS_FOR_SPEED and compile the cache file with -O2 .

Quoted 8 lines · Marek Vasut

Quoted 4 lines

The problem is that then both that function & its callers would need to know about the types of cache ops, and there'd need to be some mapping from flags to the actual cache op values (of which there'll be a couple more once L2 support is added).

But the callers already know which ops they must invoke, right ? You can just have enum {} for the bit definitions. It's all local to this file, so I don't see a problem.

It's all local to the file, but it's still an extra level of abstraction that I don't see the value of. It just means that both callers & callee have to know about various cache ops, and I don't see the point in duplicating that knowledge. I think it's much cleaner to keep that knowledge in one place - the caller.

Quoted 10 lines

Quoted 5 lines

I think it's much simpler & cleaner to
just go with the macro in this case - it keeps knowledge of which cache ops to use with the callers, and it ends up generating the same code as before (ie. the inner loop gets unrolled to just the relevant cache instructions).

I would expect GCC to take care of inlining the code, but you can always use the always_inline directive if that's your concern. Even cleaner way might be to use CONFIG_CC_OPTIMIZE_LIBS_FOR_SPEED and compile the cache file with -O2 .

It may well inline it, remove the flags indirection & generate the same code - I haven't tried it to find out. Making the code less clear with that abstraction, even if the compiler will come along & clean it back up, doesn't make much sense to me as someone who has to read the code.

I understand the preference for functions over macros, but I think this is a case where the macro has strengths that a function cannot provide (ignoring perhaps varargs which would be even more messy).

Thanks,
Paul