[RFC PATCH v2 0/5] Add generic macros for declaring various CPU structs

[RFC PATCH v2 0/5] Add generic macros for declaring various CPU structs

[Dave Martin]

Based on previous discussions, this series provides an alternative
macro for defining CPU-specific structures compactly.

I don't get to have so much fun with macros in this version, but
it's more straightforward and actually allows the declarations to
be collapsed down further by taking advantage of common naming
conventions.

In this RFC, only the arch names, processor_functions and
cache_functions are turned into macros, to show how this could
work.  This could be straightforwardly extended to cover tlb_fns,
but proc_info is more complex and may require a bit more thought.

A couple of files are ported to use the macros as an example.

These patches are for illustration only and **untested** (although
I have build-tested omap2plus_defconfig).

Unresolved issues:

 * The architecture/CPU name strings were previously split out into
   separate places, with the CPU name in .text and the other
   strings in .rodata.  This looked anomalous, so now a common
   mergeable string section .rodata.str is used instead.  This can
   be changed if needed.

 * For consistency, I've renamed the arch/CPU name string labels.
   If that is seen as unnecessary churn, it can be undone.

 * It's unclear whether it's better to switch sections inside the
   macros or outside.  Currently I switch inside, but the previous
   section is always restored using .popsection at the end of the
   macro to avoid confusion.  I'm assuming that there will never
   be a need to put any of this data in a custom section: if that's
   a bad assumption, we could move the section directives back
   outside the macros.

Dave Martin (5):
  ARM: mm: Add generic proc/arch struct definition macros
  ARM: proc-v7: Use new generic struct definition macros
  ARM: cache-v7: Use new generic struct definition macros
  ARM: proc-v6: Use new generic struct definition macros
  ARM: cache-v6: Use new generic struct definition macros

 arch/arm/mm/cache-v6.S    |   16 +--------
 arch/arm/mm/cache-v7.S    |   16 +--------
 arch/arm/mm/proc-macros.S |   79 +++++++++++++++++++++++++++++++++++++++++++++
 arch/arm/mm/proc-v6.S     |   43 +++---------------------
 arch/arm/mm/proc-v7.S     |   46 ++++---------------------
 5 files changed, 94 insertions(+), 106 deletions(-)

-- 
1.7.4.1

[RFC PATCH v2 1/5] ARM: mm: Add generic proc/arch struct definition macros

[Dave Martin]

This patch adds some generic macros to reduce boilerplate when
declaring certain common structures in arch/arm/mm/*.S

Thanks to Russell King for outlining what the
define_processor_functions macro could look like.

Signed-off-by: Dave Martin <dave.martin@linaro.org>
---
 arch/arm/mm/proc-macros.S |   79 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 79 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mm/proc-macros.S b/arch/arm/mm/proc-macros.S
index 34261f9..3c5ffbb 100644
--- a/arch/arm/mm/proc-macros.S
+++ b/arch/arm/mm/proc-macros.S
@@ -4,6 +4,7 @@
  *  VMA_VM_FLAGS
  *  VM_EXEC
  */
+#include <linux/init.h>
 #include <asm/asm-offsets.h>
 #include <asm/thread_info.h>
 
@@ -254,3 +255,81 @@
 	mcr	p15, 0, r0, c7, c10, 1		@ clean L1 D line
 	mcr	p15, 0, ip, c7, c10, 4		@ data write barrier
 	.endm
+
+.macro define_processor_functions name:req, dabort:req, pabort:req, nommu=0, suspend=0
+	.pushsection .text
+
+	__INITDATA
+
+	.type	\name\()_processor_functions, #object
+
+ENTRY(\name\()_processor_functions)
+	.word	\dabort\()_abort
+	.word	\pabort\()_pabort
+	.word	cpu_\name\()_proc_init
+	.word	cpu_\name\()_proc_fin
+	.word	cpu_\name\()_reset
+	.word	cpu_\name\()_do_idle
+	.word	cpu_\name\()_dcache_clean_area
+	.word	cpu_\name\()_switch_mm
+
+	.if \nommu
+	.word	0
+	.else
+	.word	cpu_\name\()_set_pte_ext
+	.endif
+
+	.if \suspend
+	.word	cpu_\name\()_suspend_size
+	.word	cpu_\name\()_do_suspend
+	.word	cpu_\name\()_do_resume
+	.else
+	.word	0
+	.word	0
+	.word	0
+	.endif
+
+	.size	\name\()_processor_functions, . - \name\()_processor_functions
+
+	.popsection
+.endm
+
+.macro define_proc_names name:req, arch:req, elf:req, cpu:req
+	.pushsection .rodata.str, "aMS", 1
+
+	.type \name\()_arch_name, #object
+\name\()_arch_name: .asciz "\arch"
+	.size \name\()_arch_name, . - \name\()_arch_name
+
+	.type \name\()_elf_name, #object
+\name\()_elf_name: .asciz "\elf"
+	.size \name\()_elf_name, . - \name\()_elf_name
+
+	.type \name\()_cpu_name, #object
+\name\()_cpu_name: .asciz "\cpu"
+	.size \name\()_cpu_name, . - \name\()_cpu_name
+
+	.popsection
+.endm
+
+.macro define_cache_functions name:req
+	.pushsection .text
+
+	__INITDATA
+
+	.type	\name\()_cache_fns, #object
+ENTRY(\name\()_cache_fns)
+	.long	\name\()_flush_icache_all
+	.long	\name\()_flush_kern_cache_all
+	.long	\name\()_flush_user_cache_all
+	.long	\name\()_flush_user_cache_range
+	.long	\name\()_coherent_kern_range
+	.long	\name\()_coherent_user_range
+	.long	\name\()_flush_kern_dcache_area
+	.long	\name\()_dma_map_area
+	.long	\name\()_dma_unmap_area
+	.long	\name\()_dma_flush_range
+	.size	\name\()_cache_fns, . - \name\()_cache_fns
+
+	.popsection
+.endm
-- 
1.7.4.1

[RFC PATCH v2 1/5] ARM: mm: Add generic proc/arch struct definition macros

[Nicolas Pitre]

On Tue, 14 Jun 2011, Dave Martin wrote:

> This patch adds some generic macros to reduce boilerplate when
> declaring certain common structures in arch/arm/mm/*.S
> 
> Thanks to Russell King for outlining what the
> define_processor_functions macro could look like.
> 
> Signed-off-by: Dave Martin <dave.martin@linaro.org>
> ---
>  arch/arm/mm/proc-macros.S |   79 +++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 79 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mm/proc-macros.S b/arch/arm/mm/proc-macros.S
> index 34261f9..3c5ffbb 100644
> --- a/arch/arm/mm/proc-macros.S
> +++ b/arch/arm/mm/proc-macros.S
> @@ -254,3 +255,81 @@
>  	mcr	p15, 0, r0, c7, c10, 1		@ clean L1 D line
>  	mcr	p15, 0, ip, c7, c10, 4		@ data write barrier
>  	.endm
> +
> +.macro define_processor_functions name:req, dabort:req, pabort:req, nommu=0, suspend=0
> +	.pushsection .text
> +
> +	__INITDATA

Was this __INITDATA right after a .pushsection intentional?  One might 
be confused about the effective section after this.

The __INITDATA tag is actually doing a

	.section	".init.data","aw",%progbits

Maybe this could be used as argument to .pushsection instead of .text 
(which in this case should probably have been .rodata otherwise anyway).


Nicolas

[RFC PATCH v2 1/5] ARM: mm: Add generic proc/arch struct definition macros

[Dave Martin]

On Wed, Jun 15, 2011 at 07:57:13PM -0400, Nicolas Pitre wrote:
> On Tue, 14 Jun 2011, Dave Martin wrote:
> 
> > This patch adds some generic macros to reduce boilerplate when
> > declaring certain common structures in arch/arm/mm/*.S
> > 
> > Thanks to Russell King for outlining what the
> > define_processor_functions macro could look like.
> > 
> > Signed-off-by: Dave Martin <dave.martin@linaro.org>
> > ---
> >  arch/arm/mm/proc-macros.S |   79 +++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 79 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/arm/mm/proc-macros.S b/arch/arm/mm/proc-macros.S
> > index 34261f9..3c5ffbb 100644
> > --- a/arch/arm/mm/proc-macros.S
> > +++ b/arch/arm/mm/proc-macros.S
> > @@ -254,3 +255,81 @@
> >  	mcr	p15, 0, r0, c7, c10, 1		@ clean L1 D line
> >  	mcr	p15, 0, ip, c7, c10, 4		@ data write barrier
> >  	.endm
> > +
> > +.macro define_processor_functions name:req, dabort:req, pabort:req, nommu=0, suspend=0
> > +	.pushsection .text
> > +
> > +	__INITDATA
> 
> Was this __INITDATA right after a .pushsection intentional?  One might 
> be confused about the effective section after this.

The .pushsection is needed in order to save the previous state of the
section stack so that it can be restored later with .popsection.

.pushsection syntactically requires an argument, though in this case it's
not useful, so I just specify .text since this should be harmless.

This is kind of unclear, so at the very least it could benefit from a
comment.

> 
> The __INITDATA tag is actually doing a
> 
> 	.section	".init.data","aw",%progbits
> 
> Maybe this could be used as argument to .pushsection instead of .text 
> (which in this case should probably have been .rodata otherwise anyway).

The problem is that __INITDATA conflates the information about the section
with the action of actually switching to the section.  Nowhere are the
section name and flags available by themselves.  Since the point
of a macro is to avoid copy-paste, I prefer not to manually copy-paste
this stuff into a .pushsection directive in a random assembler file...

Perhaps we should instead use

	__INITDATA

	/* ... */

	.previous
	.endm

I tend to avoid ".previous" because I consider it somewhat broken:
in the presence of macros, the section restored by .previous is
indeterminate because a called macro may switch sections and hence
change what .previous will refer to.  There is no way to restore what
.previous points at after switching sections.

In this case, we don't call any nested macro, so .previous _should_
refer to the right thing.  We mess up .previous for the caller, but
then we always mess that up when switching sections anyway.  Nothing
in arch/arm/mm/*.S uses .previous, so we shouldn't get any problems
resulting from this, in this particular case.

Alternatively, we could propose a change <linux/init.h> to split the
__INITDATA macro up, and define the related macros in a sane way:

#define __INITDATA_NAME .init.data
#define __INITDATA_FLAGS "aw",%progbtis
#define __initdata __section(__INITDATA_NAME)
#define __INITDATA .section __stringify(__INITDATA_NAME),__INITDATA_FLAGS

Any thoughts on that?

Cheers
---Dave

[RFC PATCH v2 1/5] ARM: mm: Add generic proc/arch struct definition macros

[Nicolas Pitre]

On Thu, 16 Jun 2011, Dave Martin wrote:

> Alternatively, we could propose a change <linux/init.h> to split the
> __INITDATA macro up, and define the related macros in a sane way:
> 
> #define __INITDATA_NAME .init.data
> #define __INITDATA_FLAGS "aw",%progbtis
> #define __initdata __section(__INITDATA_NAME)
> #define __INITDATA .section __stringify(__INITDATA_NAME),__INITDATA_FLAGS
> 
> Any thoughts on that?

I do like it.


Nicolas

[RFC PATCH v2 1/5] ARM: mm: Add generic proc/arch struct definition macros

[Dave Martin]

On Sun, Jun 19, 2011 at 11:13:46PM -0400, Nicolas Pitre wrote:
> On Thu, 16 Jun 2011, Dave Martin wrote:
> 
> > Alternatively, we could propose a change <linux/init.h> to split the
> > __INITDATA macro up, and define the related macros in a sane way:
> > 
> > #define __INITDATA_NAME .init.data
> > #define __INITDATA_FLAGS "aw",%progbtis
> > #define __initdata __section(__INITDATA_NAME)
> > #define __INITDATA .section __stringify(__INITDATA_NAME),__INITDATA_FLAGS
> > 
> > Any thoughts on that?
> 
> I do like it.

I think that all this data (the cache, tlb and processor funcs tables, and
the name strings) should theoretically go in __INITRODATA, since it is not
written and is only referenced via .proc.info.init so is not findable after
init data has been discarded anyway.  This data is all copied by setup.c
during startup, AFAICT.

However, the current section assignment situation is somewhat
inconsistent.

 * processor_functions, cache_fns: __INITDATA (writable)
 * tlb_fns: various (tlb-v3.S has __INITDATA; tlb-v7.S has __INIT)
 * name strings: .rodata (not initdata)


I don't really understand how this stuff interacts with CPU hotplug however;
maybe this stuff should all be in __CPUINITRODATA instead.


Since I will likely patch all the proc-*.S and cache-*.S files, I will
try to avoid any potential breakage in the initial series; so I'll keep
the section directives out of the macros so that we can prove in an
automated way that there is zero change the generated binaries.

This still brings much of the benefits.

Moving the section directives in the macros could possibly occur as a
separate follow-up.

Cheers
---Dave

[RFC PATCH v2 2/5] ARM: proc-v7: Use new generic struct definition macros

[Dave Martin]

Signed-off-by: Dave Martin <dave.martin@linaro.org>
---
 arch/arm/mm/proc-v7.S |   46 ++++++++--------------------------------------
 1 files changed, 8 insertions(+), 38 deletions(-)

diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
index b3b566e..c5cf6ea 100644
--- a/arch/arm/mm/proc-v7.S
+++ b/arch/arm/mm/proc-v7.S
@@ -171,8 +171,6 @@ ENTRY(cpu_v7_set_pte_ext)
 	mov	pc, lr
 ENDPROC(cpu_v7_set_pte_ext)
 
-cpu_v7_name:
-	.ascii	"ARMv7 Processor"
 	.align
 
 	/*
@@ -403,36 +401,8 @@ v7_crval:
 __v7_setup_stack:
 	.space	4 * 11				@ 11 registers
 
-	__INITDATA
-
-	.type	v7_processor_functions, #object
-ENTRY(v7_processor_functions)
-	.word	v7_early_abort
-	.word	v7_pabort
-	.word	cpu_v7_proc_init
-	.word	cpu_v7_proc_fin
-	.word	cpu_v7_reset
-	.word	cpu_v7_do_idle
-	.word	cpu_v7_dcache_clean_area
-	.word	cpu_v7_switch_mm
-	.word	cpu_v7_set_pte_ext
-	.word	0
-	.word	0
-	.word	0
-	.size	v7_processor_functions, . - v7_processor_functions
-
-	.section ".rodata"
-
-	.type	cpu_arch_name, #object
-cpu_arch_name:
-	.asciz	"armv7"
-	.size	cpu_arch_name, . - cpu_arch_name
-
-	.type	cpu_elf_name, #object
-cpu_elf_name:
-	.asciz	"v7"
-	.size	cpu_elf_name, . - cpu_elf_name
-	.align
+	define_processor_functions v7, dabort=v7_early, pabort=v7
+	define_proc_names v7, "armv7", "v7", "ARMv7 Processor"
 
 	.section ".proc.info.init", #alloc, #execinstr
 
@@ -455,10 +425,10 @@ __v7_ca9mp_proc_info:
 		PMD_SECT_AP_WRITE | \
 		PMD_SECT_AP_READ
 	W(b)	__v7_ca9mp_setup
-	.long	cpu_arch_name
-	.long	cpu_elf_name
+	.long	v7_arch_name
+	.long	v7_elf_name
 	.long	HWCAP_SWP|HWCAP_HALF|HWCAP_THUMB|HWCAP_FAST_MULT|HWCAP_EDSP|HWCAP_TLS
-	.long	cpu_v7_name
+	.long	v7_cpu_name
 	.long	v7_processor_functions
 	.long	v7wbi_tlb_fns
 	.long	v6_user_fns
@@ -487,10 +457,10 @@ __v7_proc_info:
 		PMD_SECT_AP_WRITE | \
 		PMD_SECT_AP_READ
 	W(b)	__v7_setup
-	.long	cpu_arch_name
-	.long	cpu_elf_name
+	.long	v7_arch_name
+	.long	v7_elf_name
 	.long	HWCAP_SWP|HWCAP_HALF|HWCAP_THUMB|HWCAP_FAST_MULT|HWCAP_EDSP|HWCAP_TLS
-	.long	cpu_v7_name
+	.long	v7_cpu_name
 	.long	v7_processor_functions
 	.long	v7wbi_tlb_fns
 	.long	v6_user_fns
-- 
1.7.4.1

[RFC PATCH v2 2/5] ARM: proc-v7: Use new generic struct definition macros

[Will Deacon]

On Tue, Jun 14, 2011 at 11:58:22AM +0100, Dave Martin wrote:
> diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
> index b3b566e..c5cf6ea 100644
> --- a/arch/arm/mm/proc-v7.S
> +++ b/arch/arm/mm/proc-v7.S
> @@ -171,8 +171,6 @@ ENTRY(cpu_v7_set_pte_ext)
>  	mov	pc, lr
>  ENDPROC(cpu_v7_set_pte_ext)
>  
> -cpu_v7_name:
> -	.ascii	"ARMv7 Processor"
>  	.align
>  
>  	/*
> @@ -403,36 +401,8 @@ v7_crval:
>  __v7_setup_stack:
>  	.space	4 * 11				@ 11 registers
>  
> -	__INITDATA
> -
> -	.type	v7_processor_functions, #object
> -ENTRY(v7_processor_functions)
> -	.word	v7_early_abort
> -	.word	v7_pabort
> -	.word	cpu_v7_proc_init
> -	.word	cpu_v7_proc_fin
> -	.word	cpu_v7_reset
> -	.word	cpu_v7_do_idle
> -	.word	cpu_v7_dcache_clean_area
> -	.word	cpu_v7_switch_mm
> -	.word	cpu_v7_set_pte_ext
> -	.word	0
> -	.word	0
> -	.word	0
> -	.size	v7_processor_functions, . - v7_processor_functions
> -
> -	.section ".rodata"
> -
> -	.type	cpu_arch_name, #object
> -cpu_arch_name:
> -	.asciz	"armv7"
> -	.size	cpu_arch_name, . - cpu_arch_name
> -
> -	.type	cpu_elf_name, #object
> -cpu_elf_name:
> -	.asciz	"v7"
> -	.size	cpu_elf_name, . - cpu_elf_name
> -	.align
> +	define_processor_functions v7, dabort=v7_early, pabort=v7
> +	define_proc_names v7, "armv7", "v7", "ARMv7 Processor"

It's not required, but it might be nice to use named arguments here anyway
since v7, "armv7", "v7" and "ARMv7 Processor" are all quite similar and it
would be easier to read if it's explicitly stated what's what.

Will

[RFC PATCH v2 3/5] ARM: cache-v7: Use new generic struct definition macros

[Dave Martin]

Signed-off-by: Dave Martin <dave.martin@linaro.org>
---
 arch/arm/mm/cache-v7.S |   16 +---------------
 1 files changed, 1 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mm/cache-v7.S b/arch/arm/mm/cache-v7.S
index d32f02b..be4e94b 100644
--- a/arch/arm/mm/cache-v7.S
+++ b/arch/arm/mm/cache-v7.S
@@ -323,18 +323,4 @@ ENTRY(v7_dma_unmap_area)
 	mov	pc, lr
 ENDPROC(v7_dma_unmap_area)
 
-	__INITDATA
-
-	.type	v7_cache_fns, #object
-ENTRY(v7_cache_fns)
-	.long	v7_flush_icache_all
-	.long	v7_flush_kern_cache_all
-	.long	v7_flush_user_cache_all
-	.long	v7_flush_user_cache_range
-	.long	v7_coherent_kern_range
-	.long	v7_coherent_user_range
-	.long	v7_flush_kern_dcache_area
-	.long	v7_dma_map_area
-	.long	v7_dma_unmap_area
-	.long	v7_dma_flush_range
-	.size	v7_cache_fns, . - v7_cache_fns
+	define_cache_functions v7
-- 
1.7.4.1

[RFC PATCH v2 4/5] ARM: proc-v6: Use new generic struct definition macros

[Dave Martin]

Signed-off-by: Dave Martin <dave.martin@linaro.org>
---
 arch/arm/mm/proc-v6.S |   43 +++++--------------------------------------
 1 files changed, 5 insertions(+), 38 deletions(-)

diff --git a/arch/arm/mm/proc-v6.S b/arch/arm/mm/proc-v6.S
index 1d2b845..d7ae4b7 100644
--- a/arch/arm/mm/proc-v6.S
+++ b/arch/arm/mm/proc-v6.S
@@ -170,11 +170,6 @@ cpu_resume_l1_flags:
 #endif
 
 
-	.type	cpu_v6_name, #object
-cpu_v6_name:
-	.asciz	"ARMv6-compatible processor"
-	.size	cpu_v6_name, . - cpu_v6_name
-
 	.align
 
 	__CPUINIT
@@ -237,36 +232,8 @@ __v6_setup:
 v6_crval:
 	crval	clear=0x01e0fb7f, mmuset=0x00c0387d, ucset=0x00c0187c
 
-	__INITDATA
-
-	.type	v6_processor_functions, #object
-ENTRY(v6_processor_functions)
-	.word	v6_early_abort
-	.word	v6_pabort
-	.word	cpu_v6_proc_init
-	.word	cpu_v6_proc_fin
-	.word	cpu_v6_reset
-	.word	cpu_v6_do_idle
-	.word	cpu_v6_dcache_clean_area
-	.word	cpu_v6_switch_mm
-	.word	cpu_v6_set_pte_ext
-	.word	cpu_v6_suspend_size
-	.word	cpu_v6_do_suspend
-	.word	cpu_v6_do_resume
-	.size	v6_processor_functions, . - v6_processor_functions
-
-	.section ".rodata"
-
-	.type	cpu_arch_name, #object
-cpu_arch_name:
-	.asciz	"armv6"
-	.size	cpu_arch_name, . - cpu_arch_name
-
-	.type	cpu_elf_name, #object
-cpu_elf_name:
-	.asciz	"v6"
-	.size	cpu_elf_name, . - cpu_elf_name
-	.align
+	define_processor_functions v6, dabort=v6_early, pabort=v6, suspend=1
+	define_proc_names v6, "armv6", "v6", "ARMv6-compatible processor"
 
 	.section ".proc.info.init", #alloc, #execinstr
 
@@ -292,11 +259,11 @@ __v6_proc_info:
 		PMD_SECT_AP_WRITE | \
 		PMD_SECT_AP_READ
 	b	__v6_setup
-	.long	cpu_arch_name
-	.long	cpu_elf_name
+	.long	v6_arch_name
+	.long	v6_elf_name
 	/* See also feat_v6_fixup() for HWCAP_TLS */
 	.long	HWCAP_SWP|HWCAP_HALF|HWCAP_THUMB|HWCAP_FAST_MULT|HWCAP_EDSP|HWCAP_JAVA|HWCAP_TLS
-	.long	cpu_v6_name
+	.long	v6_cpu_name
 	.long	v6_processor_functions
 	.long	v6wbi_tlb_fns
 	.long	v6_user_fns
-- 
1.7.4.1

[RFC PATCH v2 5/5] ARM: cache-v6: Use new generic struct definition macros

[Dave Martin]

Signed-off-by: Dave Martin <dave.martin@linaro.org>
---
 arch/arm/mm/cache-v6.S |   16 +---------------
 1 files changed, 1 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mm/cache-v6.S b/arch/arm/mm/cache-v6.S
index 73b4a8b..93d3bf8 100644
--- a/arch/arm/mm/cache-v6.S
+++ b/arch/arm/mm/cache-v6.S
@@ -328,18 +328,4 @@ ENTRY(v6_dma_unmap_area)
 	mov	pc, lr
 ENDPROC(v6_dma_unmap_area)
 
-	__INITDATA
-
-	.type	v6_cache_fns, #object
-ENTRY(v6_cache_fns)
-	.long	v6_flush_icache_all
-	.long	v6_flush_kern_cache_all
-	.long	v6_flush_user_cache_all
-	.long	v6_flush_user_cache_range
-	.long	v6_coherent_kern_range
-	.long	v6_coherent_user_range
-	.long	v6_flush_kern_dcache_area
-	.long	v6_dma_map_area
-	.long	v6_dma_unmap_area
-	.long	v6_dma_flush_range
-	.size	v6_cache_fns, . - v6_cache_fns
+	define_cache_functions v6
-- 
1.7.4.1

[RFC PATCH v2 0/5] Add generic macros for declaring various CPU structs

[Will Deacon]

Hi Dave,

On Tue, Jun 14, 2011 at 11:58:20AM +0100, Dave Martin wrote:
> Based on previous discussions, this series provides an alternative
> macro for defining CPU-specific structures compactly.
> 
> I don't get to have so much fun with macros in this version, but
> it's more straightforward and actually allows the declarations to
> be collapsed down further by taking advantage of common naming
> conventions.
> 
> In this RFC, only the arch names, processor_functions and
> cache_functions are turned into macros, to show how this could
> work.  This could be straightforwardly extended to cover tlb_fns,
> but proc_info is more complex and may require a bit more thought.

In general, this looks good to me and I'm happy to rebase my A5/A15 core
patches on top of this series.

>  * For consistency, I've renamed the arch/CPU name string labels.
>    If that is seen as unnecessary churn, it can be undone.

I don't see the win here, so let's leave the names like they are to avoid
unnecessary conflicts with other patches dealing with proc_info structs.

Will

[RFC PATCH v2 0/5] Add generic macros for declaring various CPU structs

[Russell King - ARM Linux]

On Thu, Jun 16, 2011 at 11:12:41AM +0100, Will Deacon wrote:
> >  * For consistency, I've renamed the arch/CPU name string labels.
> >    If that is seen as unnecessary churn, it can be undone.
> 
> I don't see the win here, so let's leave the names like they are to avoid
> unnecessary conflicts with other patches dealing with proc_info structs.

One of the issues is that there are proc-* files which use the same
strings for several entries, for example, proc-xscale.S.

In general, the ELF name and arch name should be the same across all,
just the CPU name should differ.

[RFC PATCH v2 0/5] Add generic macros for declaring various CPU structs

[Dave Martin]

On Thu, Jun 16, 2011 at 11:15:36AM +0100, Russell King - ARM Linux wrote:
> On Thu, Jun 16, 2011 at 11:12:41AM +0100, Will Deacon wrote:
> > >  * For consistency, I've renamed the arch/CPU name string labels.
> > >    If that is seen as unnecessary churn, it can be undone.
> > 
> > I don't see the win here, so let's leave the names like they are to avoid
> > unnecessary conflicts with other patches dealing with proc_info structs.
> 
> One of the issues is that there are proc-* files which use the same
> strings for several entries, for example, proc-xscale.S.
> 
> In general, the ELF name and arch name should be the same across all,
> just the CPU name should differ.

Hmmm, that explains why those strings were declared a bit differently.

A few of possible solutions to this:

1) Keep my macros as they are, but use a couple of #defines to make sure
	that arch_name and cpu_name are the same everywhere in a given
	proc-*.S file. Each cpu will get its own symbol for each of these
	strings, but the strings will be merged by the linker and so won't
	be duplicated in the kernel image.

2) Split define_proc_names into two macros, say, define_arch_names and
	define_cpu_name.  define_arch_names would be used once in the
	file.

3) Define a generic macro to define a string.  That would help automate
	string declarations, but really it would no longer be arch-
	specific and could live outside arch/arm.

4) Get rid of the define_proc_names macro altogether, and declare those
	things in the existing way.

Any view on these approaches?

Cheers
---Dave

[RFC PATCH v2 0/5] Add generic macros for declaring various CPU structs

[Dave Martin]

On Thu, Jun 16, 2011 at 11:12:41AM +0100, Will Deacon wrote:
> Hi Dave,
> 
> On Tue, Jun 14, 2011 at 11:58:20AM +0100, Dave Martin wrote:
> > Based on previous discussions, this series provides an alternative
> > macro for defining CPU-specific structures compactly.
> > 
> > I don't get to have so much fun with macros in this version, but
> > it's more straightforward and actually allows the declarations to
> > be collapsed down further by taking advantage of common naming
> > conventions.
> > 
> > In this RFC, only the arch names, processor_functions and
> > cache_functions are turned into macros, to show how this could
> > work.  This could be straightforwardly extended to cover tlb_fns,
> > but proc_info is more complex and may require a bit more thought.
> 
> In general, this looks good to me and I'm happy to rebase my A5/A15 core
> patches on top of this series.
> 
> >  * For consistency, I've renamed the arch/CPU name string labels.
> >    If that is seen as unnecessary churn, it can be undone.
> 
> I don't see the win here, so let's leave the names like they are to avoid
> unnecessary conflicts with other patches dealing with proc_info structs.

Agreed:

Having thought about this, and taking into account Russell's comments
about problems with this approach in proc-*.S files declaring multiple
CPUs, we should revert to not renaming any of those labels.

I'll follow up with updated patches after Russell has given his view,
but assume for now that the proc_info struct contents themselves
won't need to change.

Cheers
---Dave