[PATCH 1/2] [ARM] Introduce 'struct machine_class' for SoC level abstraction

[PATCH 1/2] [ARM] Introduce 'struct machine_class' for SoC level abstraction

[Eric Miao]

From: Eric Miao <eric.y.miao@gmail.com>

For machine class (mostly equivalent to SoC) level abstraction.

The modification to head.S is preliminary, working but possibly
can be made more elegant.

Signed-off-by: Eric Miao <eric.miao@canonical.com>
---
 arch/arm/include/asm/mach/arch.h |   13 +++++++++++++
 arch/arm/kernel/asm-offsets.c    |    4 ++++
 arch/arm/kernel/head-common.S    |    3 +++
 arch/arm/kernel/head.S           |   12 +++++++++---
 arch/arm/kernel/setup.c          |   14 ++++++++++----
 arch/arm/mm/mmu.c                |    5 ++++-
 6 files changed, 43 insertions(+), 8 deletions(-)

diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h
index c59842d..214ca29 100644
--- a/arch/arm/include/asm/mach/arch.h
+++ b/arch/arm/include/asm/mach/arch.h
@@ -14,6 +14,18 @@ struct tag;
 struct meminfo;
 struct sys_timer;
 
+struct machine_class {
+	unsigned int		phys_io;	/* start of physical io	*/
+	unsigned int		io_pg_offst;	/* byte offset for io
+						 * page tabe entry	*/
+	const char		*name;		/* machine class name	*/
+	unsigned long		boot_params;	/* tagged list		*/
+
+	void			(*map_io)(void);/* IO mapping function */
+	void			(*init_irq)(void);
+	struct sys_timer	*timer;
+};
+
 struct machine_desc {
 	/*
 	 * Note! The first four elements are used
@@ -25,6 +37,7 @@ struct machine_desc {
 						 * page tabe entry	*/
 
 	const char		*name;		/* architecture name	*/
+	struct machine_class	*class;		/* machine class */
 	unsigned long		boot_params;	/* tagged list		*/
 
 	unsigned int		video_start;	/* start of video RAM	*/
diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
index 8835115..5c9038a 100644
--- a/arch/arm/kernel/asm-offsets.c
+++ b/arch/arm/kernel/asm-offsets.c
@@ -101,6 +101,10 @@ int main(void)
   DEFINE(MACHINFO_NAME,		offsetof(struct machine_desc, name));
   DEFINE(MACHINFO_PHYSIO,	offsetof(struct machine_desc, phys_io));
   DEFINE(MACHINFO_PGOFFIO,	offsetof(struct machine_desc, io_pg_offst));
+  DEFINE(MACHINFO_CLASS,	offsetof(struct machine_desc, class));
+  BLANK();
+  DEFINE(MACHCLASS_PHYSIO,	offsetof(struct machine_class, phys_io));
+  DEFINE(MACHCLASS_PGOFFIO,	offsetof(struct machine_class, io_pg_offst));
   BLANK();
   DEFINE(PROC_INFO_SZ,		sizeof(struct proc_info_list));
   DEFINE(PROCINFO_INITFUNC,	offsetof(struct proc_info_list, __cpu_flush));
diff --git a/arch/arm/kernel/head-common.S b/arch/arm/kernel/head-common.S
index b9505aa..660756b 100644
--- a/arch/arm/kernel/head-common.S
+++ b/arch/arm/kernel/head-common.S
@@ -207,6 +207,7 @@ ENDPROC(lookup_processor_type)
  * Returns:
  *  r3, r4, r6 corrupted
  *  r5 = mach_info pointer in physical address space
+ *  r7 = machine_class pointer in physical address space
  */
 __lookup_machine_type:
 	adr	r3, 4b
@@ -214,6 +215,8 @@ __lookup_machine_type:
 	sub	r3, r3, r4			@ get offset between virt&phys
 	add	r5, r5, r3			@ convert virt addresses to
 	add	r6, r6, r3			@ physical address space
+	ldr	r7, [r5, #MACHINFO_CLASS]	@ get machine class
+	add	r7, r7, r3
 1:	ldr	r3, [r5, #MACHINFO_TYPE]	@ get machine type
 	teq	r3, r1				@ matches loader number?
 	beq	2f				@ found
diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S
index eb62bf9..06f8834 100644
--- a/arch/arm/kernel/head.S
+++ b/arch/arm/kernel/head.S
@@ -82,8 +82,9 @@ ENTRY(stext)
 	bl	__lookup_processor_type		@ r5=procinfo r9=cpuid
 	movs	r10, r5				@ invalid processor (r5=0)?
 	beq	__error_p			@ yes, error 'p'
-	bl	__lookup_machine_type		@ r5=machinfo
+	bl	__lookup_machine_type		@ r5=machinfo,r7=machine_class
 	movs	r8, r5				@ invalid machine (r5=0)?
+	mov	r11, r7
 	beq	__error_a			@ yes, error 'a'
 	bl	__vet_atags
 	bl	__create_page_tables
@@ -211,6 +212,7 @@ ENDPROC(__turn_mmu_on)
  * r8  = machinfo
  * r9  = cpuid
  * r10 = procinfo
+ * r11 = machine_class
  *
  * Returns:
  *  r0, r3, r6, r7 corrupted
@@ -295,13 +297,17 @@ __create_page_tables:
 	 * This allows debug messages to be output
 	 * via a serial console before paging_init.
 	 */
-	ldr	r3, [r8, #MACHINFO_PGOFFIO]
+	cmp	r11, #0
+	ldrne	r3, [r11, #MACHCLASS_PGOFFIO]
+	ldreq	r3, [r8, #MACHINFO_PGOFFIO]
 	add	r0, r4, r3
 	rsb	r3, r3, #0x4000			@ PTRS_PER_PGD*sizeof(long)
 	cmp	r3, #0x0800			@ limit to 512MB
 	movhi	r3, #0x0800
 	add	r6, r0, r3
-	ldr	r3, [r8, #MACHINFO_PHYSIO]
+	cmp	r11, #0
+	ldrne	r3, [r11, #MACHCLASS_PHYSIO]
+	ldreq	r3, [r8, #MACHINFO_PHYSIO]
 	orr	r3, r3, r7
 1:	str	r3, [r0], #4
 	add	r3, r3, #1 << 20
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 122d999..e5627da 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -667,12 +667,14 @@ void __init setup_arch(char **cmdline_p)
 {
 	struct tag *tags = (struct tag *)&init_tags;
 	struct machine_desc *mdesc;
+	struct machine_class *class;
 	char *from = default_command_line;
 
 	unwind_init();
 
 	setup_processor();
 	mdesc = setup_machine(machine_arch_type);
+	class = mdesc->class;
 	machine_name = mdesc->name;
 
 	if (mdesc->soft_reboot)
@@ -680,8 +682,12 @@ void __init setup_arch(char **cmdline_p)
 
 	if (__atags_pointer)
 		tags = phys_to_virt(__atags_pointer);
-	else if (mdesc->boot_params)
-		tags = phys_to_virt(mdesc->boot_params);
+	else {
+		unsigned long boot_params = class ? class->boot_params :
+						    mdesc->boot_params;
+		if (boot_params)
+			tags = phys_to_virt(boot_params);
+	}
 
 	/*
 	 * If we have the old style parameters, convert them to
@@ -729,8 +735,8 @@ void __init setup_arch(char **cmdline_p)
 	/*
 	 * Set up various architecture-specific pointers
 	 */
-	init_arch_irq = mdesc->init_irq;
-	system_timer = mdesc->timer;
+	init_arch_irq = class ? class->init_irq : mdesc->init_irq;
+	system_timer = class ? class->timer : mdesc->timer;
 	init_machine = mdesc->init_machine;
 
 #ifdef CONFIG_VT
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index 2858941..2af11dc 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -933,6 +933,7 @@ void __init reserve_node_zero(pg_data_t *pgdat)
  */
 static void __init devicemaps_init(struct machine_desc *mdesc)
 {
+	struct machine_class *class = mdesc->class;
 	struct map_desc map;
 	unsigned long addr;
 	void *vectors;
@@ -995,7 +996,9 @@ static void __init devicemaps_init(struct machine_desc *mdesc)
 	/*
 	 * Ask the machine support to map in the statically mapped devices.
 	 */
-	if (mdesc->map_io)
+	if (class && class->map_io)
+		class->map_io();
+	else if (mdesc->map_io)
 		mdesc->map_io();
 
 	/*
-- 
1.7.1

[PATCH 2/2] [ARM] pxa: make use of 'struct machine_class'

[Eric Miao]

From: Eric Miao <eric.y.miao@gmail.com>

Signed-off-by: Eric Miao <eric.miao@canonical.com>
---
 arch/arm/mach-pxa/ezx.c      |   60 ++++-------------------------------------
 arch/arm/mach-pxa/generic.h  |   15 ++++++++++
 arch/arm/mach-pxa/hx4700.c   |   10 +------
 arch/arm/mach-pxa/magician.c |    9 +-----
 arch/arm/mach-pxa/pxa25x.c   |   11 +++++++
 arch/arm/mach-pxa/pxa27x.c   |   11 +++++++
 6 files changed, 46 insertions(+), 70 deletions(-)

diff --git a/arch/arm/mach-pxa/ezx.c b/arch/arm/mach-pxa/ezx.c
index 626c82b..2f4c8f1 100644
--- a/arch/arm/mach-pxa/ezx.c
+++ b/arch/arm/mach-pxa/ezx.c
@@ -795,15 +795,7 @@ static void __init a780_init(void)
 	platform_add_devices(ARRAY_AND_SIZE(a780_devices));
 }
 
-MACHINE_START(EZX_A780, "Motorola EZX A780")
-	.phys_io        = 0x40000000,
-	.io_pg_offst    = (io_p2v(0x40000000) >> 18) & 0xfffc,
-	.boot_params    = 0xa0000100,
-	.map_io         = pxa_map_io,
-	.init_irq       = pxa27x_init_irq,
-	.timer          = &pxa_timer,
-	.init_machine   = a780_init,
-MACHINE_END
+PXA27X_MACHINE(EZX_A780, a780_init, "Motorola EZX A780");
 #endif
 
 #ifdef CONFIG_MACH_EZX_E680
@@ -861,15 +853,7 @@ static void __init e680_init(void)
 	platform_add_devices(ARRAY_AND_SIZE(e680_devices));
 }
 
-MACHINE_START(EZX_E680, "Motorola EZX E680")
-	.phys_io        = 0x40000000,
-	.io_pg_offst    = (io_p2v(0x40000000) >> 18) & 0xfffc,
-	.boot_params    = 0xa0000100,
-	.map_io         = pxa_map_io,
-	.init_irq       = pxa27x_init_irq,
-	.timer          = &pxa_timer,
-	.init_machine   = e680_init,
-MACHINE_END
+PXA27X_MACHINE(EZX_E680, e680_init, "Motorola EZX E680");
 #endif
 
 #ifdef CONFIG_MACH_EZX_A1200
@@ -927,15 +911,7 @@ static void __init a1200_init(void)
 	platform_add_devices(ARRAY_AND_SIZE(a1200_devices));
 }
 
-MACHINE_START(EZX_A1200, "Motorola EZX A1200")
-	.phys_io        = 0x40000000,
-	.io_pg_offst    = (io_p2v(0x40000000) >> 18) & 0xfffc,
-	.boot_params    = 0xa0000100,
-	.map_io         = pxa_map_io,
-	.init_irq       = pxa27x_init_irq,
-	.timer          = &pxa_timer,
-	.init_machine   = a1200_init,
-MACHINE_END
+PXA27X_MACHINE(EZX_A1200, a1200_init, "Motorola EZX A1200");
 #endif
 
 #ifdef CONFIG_MACH_EZX_A910
@@ -1119,15 +1095,7 @@ static void __init a910_init(void)
 	platform_add_devices(ARRAY_AND_SIZE(a910_devices));
 }
 
-MACHINE_START(EZX_A910, "Motorola EZX A910")
-	.phys_io        = 0x40000000,
-	.io_pg_offst    = (io_p2v(0x40000000) >> 18) & 0xfffc,
-	.boot_params    = 0xa0000100,
-	.map_io         = pxa_map_io,
-	.init_irq       = pxa27x_init_irq,
-	.timer          = &pxa_timer,
-	.init_machine   = a910_init,
-MACHINE_END
+PXA27X_MACHINE(EZX_A910, a910_init, "Motorola EZX A910");
 #endif
 
 #ifdef CONFIG_MACH_EZX_E6
@@ -1185,15 +1153,7 @@ static void __init e6_init(void)
 	platform_add_devices(ARRAY_AND_SIZE(e6_devices));
 }
 
-MACHINE_START(EZX_E6, "Motorola EZX E6")
-	.phys_io        = 0x40000000,
-	.io_pg_offst    = (io_p2v(0x40000000) >> 18) & 0xfffc,
-	.boot_params    = 0xa0000100,
-	.map_io         = pxa_map_io,
-	.init_irq       = pxa27x_init_irq,
-	.timer          = &pxa_timer,
-	.init_machine   = e6_init,
-MACHINE_END
+PXA27X_MACHINE(EZX_E6, e6_init, "Motorola EZX E6");
 #endif
 
 #ifdef CONFIG_MACH_EZX_E2
@@ -1225,13 +1185,5 @@ static void __init e2_init(void)
 	platform_add_devices(ARRAY_AND_SIZE(e2_devices));
 }
 
-MACHINE_START(EZX_E2, "Motorola EZX E2")
-	.phys_io        = 0x40000000,
-	.io_pg_offst    = (io_p2v(0x40000000) >> 18) & 0xfffc,
-	.boot_params    = 0xa0000100,
-	.map_io         = pxa_map_io,
-	.init_irq       = pxa27x_init_irq,
-	.timer          = &pxa_timer,
-	.init_machine   = e2_init,
-MACHINE_END
+PXA27X_MACHINE(EZX_E2, e2_init, "Motorola EZX E2");
 #endif
diff --git a/arch/arm/mach-pxa/generic.h b/arch/arm/mach-pxa/generic.h
index 890fb90..6b6d767 100644
--- a/arch/arm/mach-pxa/generic.h
+++ b/arch/arm/mach-pxa/generic.h
@@ -72,3 +72,18 @@ void __init pxa_set_ffuart_info(void *info);
 void __init pxa_set_btuart_info(void *info);
 void __init pxa_set_stuart_info(void *info);
 void __init pxa_set_hwuart_info(void *info);
+
+extern struct machine_class machine_class_pxa25x;
+extern struct machine_class machine_class_pxa27x;
+
+#define PXA25X_MACHINE(_type,_init,_name)		\
+MACHINE_START(_type,_name)				\
+	.class		= &machine_class_pxa25x,	\
+	.init_machine	= _init,			\
+}
+
+#define PXA27X_MACHINE(_type,_init,_name)		\
+MACHINE_START(_type,_name)				\
+	.class		= &machine_class_pxa27x,	\
+	.init_machine	= _init,			\
+}
diff --git a/arch/arm/mach-pxa/hx4700.c b/arch/arm/mach-pxa/hx4700.c
index 848c861..a1b1587 100644
--- a/arch/arm/mach-pxa/hx4700.c
+++ b/arch/arm/mach-pxa/hx4700.c
@@ -869,12 +869,4 @@ static void __init hx4700_init(void)
 	mdelay(10);
 }
 
-MACHINE_START(H4700, "HP iPAQ HX4700")
-	.phys_io      = 0x40000000,
-	.io_pg_offst  = (io_p2v(0x40000000) >> 18) & 0xfffc,
-	.boot_params  = 0xa0000100,
-	.map_io       = pxa_map_io,
-	.init_irq     = pxa27x_init_irq,
-	.init_machine = hx4700_init,
-	.timer        = &pxa_timer,
-MACHINE_END
+PXA27X_MACHINE_START(H4700, hx4700_init, "HP iPAQ HX4700");
diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach-pxa/magician.c
index e81dd0c..6a86f22 100644
--- a/arch/arm/mach-pxa/magician.c
+++ b/arch/arm/mach-pxa/magician.c
@@ -764,11 +764,6 @@ static void __init magician_init(void)
 
 
 MACHINE_START(MAGICIAN, "HTC Magician")
-	.phys_io = 0x40000000,
-	.io_pg_offst = (io_p2v(0x40000000) >> 18) & 0xfffc,
-	.boot_params = 0xa0000100,
-	.map_io = pxa_map_io,
-	.init_irq = pxa27x_init_irq,
-	.init_machine = magician_init,
-	.timer = &pxa_timer,
+	.class		= &machine_class_pxa27x,
+	.init_machine	= magician_init,
 MACHINE_END
diff --git a/arch/arm/mach-pxa/pxa25x.c b/arch/arm/mach-pxa/pxa25x.c
index 0b9ad30..8993df6 100644
--- a/arch/arm/mach-pxa/pxa25x.c
+++ b/arch/arm/mach-pxa/pxa25x.c
@@ -23,6 +23,7 @@
 #include <linux/suspend.h>
 #include <linux/sysdev.h>
 
+#include <asm/mach/arch.h>
 #include <mach/hardware.h>
 #include <mach/irqs.h>
 #include <mach/gpio.h>
@@ -341,6 +342,16 @@ static struct sys_device pxa25x_sysdev[] = {
 	},
 };
 
+struct machine_class machine_class_pxa25x __initdata = {
+	.name		= "PXA25x",
+	.phys_io	= 0x40000000,
+	.io_pg_offst	= (io_p2v(0x40000000) >> 18) & 0xfffc,
+	.boot_params	= 0xa0000100,
+	.timer		= &pxa_timer,
+	.map_io		= pxa_map_io,
+	.init_irq	= pxa25x_init_irq,
+};
+
 static int __init pxa25x_init(void)
 {
 	int i, ret = 0;
diff --git a/arch/arm/mach-pxa/pxa27x.c b/arch/arm/mach-pxa/pxa27x.c
index 0af3617..d64b194 100644
--- a/arch/arm/mach-pxa/pxa27x.c
+++ b/arch/arm/mach-pxa/pxa27x.c
@@ -18,6 +18,7 @@
 #include <linux/platform_device.h>
 #include <linux/sysdev.h>
 
+#include <asm/mach/arch.h>
 #include <mach/hardware.h>
 #include <asm/irq.h>
 #include <mach/irqs.h>
@@ -403,6 +404,16 @@ static struct sys_device pxa27x_sysdev[] = {
 	},
 };
 
+struct machine_class machine_class_pxa27x __initdata = {
+	.name		= "PXA27x",
+	.phys_io	= 0x40000000,
+	.io_pg_offst	= (io_p2v(0x40000000) >> 18) & 0xfffc,
+	.boot_params	= 0xa0000100,
+	.timer		= &pxa_timer,
+	.map_io		= pxa_map_io,
+	.init_irq	= pxa27x_init_irq,
+};
+
 static int __init pxa27x_init(void)
 {
 	int i, ret = 0;
-- 
1.7.1

[PATCH 2/2] [ARM] pxa: make use of 'struct machine_class'

[Antonio Ospite]

On Sun, 20 Jun 2010 22:00:50 +0800
Eric Miao <eric.miao@canonical.com> wrote:

> From: Eric Miao <eric.y.miao@gmail.com>
> 
> Signed-off-by: Eric Miao <eric.miao@canonical.com>
> ---
>  arch/arm/mach-pxa/ezx.c      |   60 ++++-------------------------------------

The ezx part looks OK. A comment about magician below.

[...]
> diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach-pxa/magician.c
> index e81dd0c..6a86f22 100644
> --- a/arch/arm/mach-pxa/magician.c
> +++ b/arch/arm/mach-pxa/magician.c
> @@ -764,11 +764,6 @@ static void __init magician_init(void)
>  
>  
>  MACHINE_START(MAGICIAN, "HTC Magician")
> -	.phys_io = 0x40000000,
> -	.io_pg_offst = (io_p2v(0x40000000) >> 18) & 0xfffc,
> -	.boot_params = 0xa0000100,
> -	.map_io = pxa_map_io,
> -	.init_irq = pxa27x_init_irq,
> -	.init_machine = magician_init,
> -	.timer = &pxa_timer,
> +	.class		= &machine_class_pxa27x,
> +	.init_machine	= magician_init,
>  MACHINE_END

Magician can use PXA27X_MACHINE() as well, can't it?

[...]

Regards,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100620/9cfddfda/attachment.sig>

[PATCH 2/2] [ARM] pxa: make use of 'struct machine_class'

[Eric Miao]

On Mon, Jun 21, 2010 at 12:08 AM, Antonio Ospite
<ospite@studenti.unina.it> wrote:
> On Sun, 20 Jun 2010 22:00:50 +0800
> Eric Miao <eric.miao@canonical.com> wrote:
>
>> From: Eric Miao <eric.y.miao@gmail.com>
>>
>> Signed-off-by: Eric Miao <eric.miao@canonical.com>
>> ---
>> ?arch/arm/mach-pxa/ezx.c ? ? ?| ? 60 ++++-------------------------------------
>
> The ezx part looks OK. A comment about magician below.
>
> [...]
>> diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach-pxa/magician.c
>> index e81dd0c..6a86f22 100644
>> --- a/arch/arm/mach-pxa/magician.c
>> +++ b/arch/arm/mach-pxa/magician.c
>> @@ -764,11 +764,6 @@ static void __init magician_init(void)
>>
>>
>> ?MACHINE_START(MAGICIAN, "HTC Magician")
>> - ? ? .phys_io = 0x40000000,
>> - ? ? .io_pg_offst = (io_p2v(0x40000000) >> 18) & 0xfffc,
>> - ? ? .boot_params = 0xa0000100,
>> - ? ? .map_io = pxa_map_io,
>> - ? ? .init_irq = pxa27x_init_irq,
>> - ? ? .init_machine = magician_init,
>> - ? ? .timer = &pxa_timer,
>> + ? ? .class ? ? ? ? ?= &machine_class_pxa27x,
>> + ? ? .init_machine ? = magician_init,
>> ?MACHINE_END
>
> Magician can use PXA27X_MACHINE() as well, can't it?

Yes, it can. Missed that :-)

[PATCH 1/2] [ARM] Introduce 'struct machine_class' for SoC level abstraction

[Jeremy Kerr]

Hi Eric,

> For machine class (mostly equivalent to SoC) level abstraction.

Could you detail your intended usage of machine_class and how this will fit in 
with the mdesc? Will the mdesc fields override the machine_class fields, or is 
this intended to be a replacement?

Cheers,


Jeremy

[PATCH 1/2] [ARM] Introduce 'struct machine_class' for SoC level abstraction

[Eric Miao]

On Mon, Jun 21, 2010 at 3:27 PM, Jeremy Kerr <jeremy.kerr@canonical.com> wrote:
> Hi Eric,
>
>> For machine class (mostly equivalent to SoC) level abstraction.
>
> Could you detail your intended usage of machine_class and how this will fit in
> with the mdesc? Will the mdesc fields override the machine_class fields, or is
> this intended to be a replacement?
>

Good question, I've missed that part in the commit message. So my idea
is to have machine_class fields to replace machine_desc completely in
the end, but the situation is we need a smooth transition:

1. so for newly added machines with machine_class pointer, the fields in
machine_class will dominate (ignoring the fields in machine_desc)

2. for legacy machine_desc (apparently without machine_class pointer),
the fields within will be used, and these machines are not affected

So my intention of machine_class is really a class of machines, instead of
the class concept in C++. There might be useful situations for derivative,
E.g. some PXA machines have their specific map_io, mainstone:

static struct map_desc mainstone_io_desc[] __initdata = {
  	{	/* CPLD */
		.virtual	=  MST_FPGA_VIRT,
		.pfn		= __phys_to_pfn(MST_FPGA_PHYS),
		.length		= 0x00100000,
		.type		= MT_DEVICE
	}
};

static void __init mainstone_map_io(void)
{
	pxa_map_io();
	iotable_init(mainstone_io_desc, ARRAY_SIZE(mainstone_io_desc));
}

So that from derivative POV, it can be expressed as:

	if (mdesc->class && mdesc->class->map_io)
		mdesc->class->map_io();
	if (mdesc->map_io())
		mdesc->map_io();

But this seems to make things a bit over complicated, so for such cases
I'd prefer:

1. have a separate machine_class for mainstone
2. or move the board specific map_io() separatedly

[PATCH 1/2] [ARM] Introduce 'struct machine_class' for SoC level abstraction

[Russell King - ARM Linux]

On Mon, Jun 21, 2010 at 04:23:22PM +0800, Eric Miao wrote:
> So that from derivative POV, it can be expressed as:
> 
> 	if (mdesc->class && mdesc->class->map_io)
> 		mdesc->class->map_io();
> 	if (mdesc->map_io())
> 		mdesc->map_io();
> 
> But this seems to make things a bit over complicated, so for such cases
> I'd prefer:
> 
> 1. have a separate machine_class for mainstone
> 2. or move the board specific map_io() separatedly

How about making the presence of mdesc->map_io override the class
version?

So if mainstone provides its own map_io() function, it is still
responsible (as is today) for calling the PXA specific map_io().
The same behaviour seems sensible to apply to the other class
functions as well - allow platforms to override the class
version, and leave the replacement platform function responsible
for calling the class if that's what it needs to do.

[PATCH 1/2] [ARM] Introduce 'struct machine_class' for SoC level abstraction

[Eric Miao]

On Mon, Jun 21, 2010 at 5:02 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Jun 21, 2010 at 04:23:22PM +0800, Eric Miao wrote:
>> So that from derivative POV, it can be expressed as:
>>
>> ? ? ? if (mdesc->class && mdesc->class->map_io)
>> ? ? ? ? ? ? ? mdesc->class->map_io();
>> ? ? ? if (mdesc->map_io())
>> ? ? ? ? ? ? ? mdesc->map_io();
>>
>> But this seems to make things a bit over complicated, so for such cases
>> I'd prefer:
>>
>> 1. have a separate machine_class for mainstone
>> 2. or move the board specific map_io() separatedly
>
> How about making the presence of mdesc->map_io override the class
> version?
>
> So if mainstone provides its own map_io() function, it is still
> responsible (as is today) for calling the PXA specific map_io().
> The same behaviour seems sensible to apply to the other class
> functions as well - allow platforms to override the class
> version, and leave the replacement platform function responsible
> for calling the class if that's what it needs to do.
>

Yep, that's a good idea, and actually was as my first version. Yet my
concern is this doesn't easily fit for the class data, i.e., allow the
machine specific data to override the class data like below:

	if (valid_value(mdesc->phys_io))
		use(mdesc->phys_io);
	else
		use(mdesc->class->phys_io);

but since phys_io could be valid for value zero (as if not explicitly
initialized), so we may end up defining like below to allow use of the
default class values:

MACHINE_START(....)
	.phys_io	= INVALID_ADDRESS,
	.io_pg_offst	= INVALID_ADDRESS,
	....
MACHINE_END

This, however, creates some incompatiblity in semantics from the functions.
That's why I chose a much simpler scheme: use the class version if class
is there, otherwise use the machine specific one.

[PATCH 1/2] [ARM] Introduce 'struct machine_class' for SoC level abstraction

[Nicolas Pitre]

On Mon, 21 Jun 2010, Eric Miao wrote:

> On Mon, Jun 21, 2010 at 5:02 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > The same behaviour seems sensible to apply to the other class
> > functions as well - allow platforms to override the class
> > version, and leave the replacement platform function responsible
> > for calling the class if that's what it needs to do.
> >
> 
> Yep, that's a good idea, and actually was as my first version. Yet my
> concern is this doesn't easily fit for the class data, i.e., allow the
> machine specific data to override the class data like below:
> 
> 	if (valid_value(mdesc->phys_io))
> 		use(mdesc->phys_io);
> 	else
> 		use(mdesc->class->phys_io);
> 
> but since phys_io could be valid for value zero (as if not explicitly
> initialized), so we may end up defining like below to allow use of the
> default class values:
> 
> MACHINE_START(....)
> 	.phys_io	= INVALID_ADDRESS,
> 	.io_pg_offst	= INVALID_ADDRESS,
> 	....
> MACHINE_END

No, please don't do that.  This is horribly ugly.

And in fact I would actually be highly surprised if 0 was a sensible 
value to use for either of those fields anyway.  No SOCs I know about 
has its IO at physical address0, and we're even less likely to remap 
them at virtual address 0 either.  So having 0 meaning uninitialized 
makes perfect sense in practice.


Nicolas

[PATCH 1/2] [ARM] Introduce 'struct machine_class' for SoC level abstraction

[Eric Miao]

On Mon, Jun 21, 2010 at 11:38 PM, Nicolas Pitre <nico@fluxnic.net> wrote:
> On Mon, 21 Jun 2010, Eric Miao wrote:
>
>> On Mon, Jun 21, 2010 at 5:02 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > The same behaviour seems sensible to apply to the other class
>> > functions as well - allow platforms to override the class
>> > version, and leave the replacement platform function responsible
>> > for calling the class if that's what it needs to do.
>> >
>>
>> Yep, that's a good idea, and actually was as my first version. Yet my
>> concern is this doesn't easily fit for the class data, i.e., allow the
>> machine specific data to override the class data like below:
>>
>> ? ? ? if (valid_value(mdesc->phys_io))
>> ? ? ? ? ? ? ? use(mdesc->phys_io);
>> ? ? ? else
>> ? ? ? ? ? ? ? use(mdesc->class->phys_io);
>>
>> but since phys_io could be valid for value zero (as if not explicitly
>> initialized), so we may end up defining like below to allow use of the
>> default class values:
>>
>> MACHINE_START(....)
>> ? ? ? .phys_io ? ? ? ?= INVALID_ADDRESS,
>> ? ? ? .io_pg_offst ? ?= INVALID_ADDRESS,
>> ? ? ? ....
>> MACHINE_END
>
> No, please don't do that. ?This is horribly ugly.
>
> And in fact I would actually be highly surprised if 0 was a sensible
> value to use for either of those fields anyway. ?No SOCs I know about
> has its IO at physical address0, and we're even less likely to remap
> them at virtual address 0 either. ?So having 0 meaning uninitialized
> makes perfect sense in practice.
>

Hrm... that makes sense. I'll add some comment to the 'struct machine_class'
and get this patch updated later.