All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC 0/5] restart cleanups
@ 2011-11-01 16:07 Russell King - ARM Linux
  2011-11-01 16:08 ` [PATCH 1/5] ARM: restart: remove argument to setup_mm_for_reboot() Russell King - ARM Linux
                   ` (8 more replies)
  0 siblings, 9 replies; 32+ messages in thread
From: Russell King - ARM Linux @ 2011-11-01 16:07 UTC (permalink / raw)
  To: linux-arm-kernel

This series cleans up some of the restart code for ARM.  It doesn't go
as far as Will's patch set (or even anywhere close to it) mainly because
of the poor state of the various implementations in various platforms.

Effectively, the various platforms themselves stand in the way of optimal
cleanup of this code.

We already have a function pointer to allow the restart to be hooked at
runtime - it's called arm_pm_restart().

What a lot of platforms use this for is to intercept the restart call,
do something, and then continue on by calling arm_machine_restart().

One platform (mioa701) in particular thinks its a good idea to call
functions like gpio_free() and free_irq() from here.  This is soo far
from reality its untrue; the restart paths can be called from _any_
context including IRQ context.  This alone effectively stands in the
way of moving the IRQ disables before the call to arm_pm_restart().

Other platforms use arm_pm_restart() to intercept the restart, and
force the mode to 'g', 'h' or 's'.  As the mode already defaults to
'h' I don't understand why people felt forcing it in that case was
necessary.  As for the 's' case, this can be done by merely specifying
".soft_reboot = 1" in the machine record.  We can solve the 'g' case
by changing .soft_reboot to be a char "restart_mode" argument which
sets the default restart mode.

Although I have a patch (not published as part of this series) which
makes that change, I've found it less than useful for getting rid of
some of the intercept crap because of other games platforms play in
there (like writing magic values to registers.)

Essentially, we want to move to a state where arm_pm_restart() is _the_
hook for platforms to insert their restart code into, with the default
function being a no-op.  All platforms which want to have working
restart support then have to insert their restart code into this hook.

So, this patch set takes us a little way there by cleaning up some of
the restart code:

1. remove the useless argument to setup_mm_for_reboot()
2. remove the silly indirection in AT91 code and do this like other
   platforms do.
3. remove any local_irq_disable() calls inside arch_reset() because
   this is already the case.
4. factor out the code which fiddles with the MMU tables.
5. provide (and use) a function for soft rebooting which includes the
   setup of the MMU tables and call cpu_reset() - and remove the MMU
   setup from the code paths before arch_reset() is called.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 1/5] ARM: restart: remove argument to setup_mm_for_reboot()
  2011-11-01 16:07 [RFC 0/5] restart cleanups Russell King - ARM Linux
@ 2011-11-01 16:08 ` Russell King - ARM Linux
  2011-11-01 22:27   ` Ryan Mallon
  2011-11-01 16:08 ` [PATCH 2/5] ARM: restart: get rid of needless at91_arch_reset additional indirection Russell King - ARM Linux
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 32+ messages in thread
From: Russell King - ARM Linux @ 2011-11-01 16:08 UTC (permalink / raw)
  To: linux-arm-kernel

setup_mm_for_reboot() doesn't make use of its argument, so remove it.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/kernel/machine_kexec.c |    4 ++--
 arch/arm/kernel/process.c       |    4 ++--
 arch/arm/mm/idmap.c             |    2 +-
 arch/arm/mm/nommu.c             |    2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c
index c1b4463..6fcdfc4 100644
--- a/arch/arm/kernel/machine_kexec.c
+++ b/arch/arm/kernel/machine_kexec.c
@@ -16,7 +16,7 @@
 extern const unsigned char relocate_new_kernel[];
 extern const unsigned int relocate_new_kernel_size;
 
-extern void setup_mm_for_reboot(char mode);
+extern void setup_mm_for_reboot(void);
 
 extern unsigned long kexec_start_address;
 extern unsigned long kexec_indirection_page;
@@ -114,7 +114,7 @@ void machine_kexec(struct kimage *image)
 		kexec_reinit();
 	local_irq_disable();
 	local_fiq_disable();
-	setup_mm_for_reboot(0); /* mode is not used, so just pass 0*/
+	setup_mm_for_reboot(); /* mode is not used, so just pass 0*/
 	flush_cache_all();
 	outer_flush_all();
 	outer_disable();
diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index fd08140..62946c0 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -57,7 +57,7 @@ static const char *isa_modes[] = {
   "ARM" , "Thumb" , "Jazelle", "ThumbEE"
 };
 
-extern void setup_mm_for_reboot(char mode);
+extern void setup_mm_for_reboot(void);
 
 static volatile int hlt_counter;
 
@@ -103,7 +103,7 @@ void arm_machine_restart(char mode, const char *cmd)
 	 * we may need it to insert some 1:1 mappings so that
 	 * soft boot works.
 	 */
-	setup_mm_for_reboot(mode);
+	setup_mm_for_reboot();
 
 	/* Clean and invalidate caches */
 	flush_cache_all();
diff --git a/arch/arm/mm/idmap.c b/arch/arm/mm/idmap.c
index 2be9139..296ad2e 100644
--- a/arch/arm/mm/idmap.c
+++ b/arch/arm/mm/idmap.c
@@ -78,7 +78,7 @@ void identity_mapping_del(pgd_t *pgd, unsigned long addr, unsigned long end)
  * the user-mode pages.  This will then ensure that we have predictable
  * results when turning the mmu off
  */
-void setup_mm_for_reboot(char mode)
+void setup_mm_for_reboot(void)
 {
 	/*
 	 * We need to access to user-mode page tables here. For kernel threads
diff --git a/arch/arm/mm/nommu.c b/arch/arm/mm/nommu.c
index 941a98c..8841751 100644
--- a/arch/arm/mm/nommu.c
+++ b/arch/arm/mm/nommu.c
@@ -43,7 +43,7 @@ void __init paging_init(struct machine_desc *mdesc)
 /*
  * We don't need to do anything here for nommu machines.
  */
-void setup_mm_for_reboot(char mode)
+void setup_mm_for_reboot(void)
 {
 }
 
-- 
1.7.4.4

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH 2/5] ARM: restart: get rid of needless at91_arch_reset additional indirection
  2011-11-01 16:07 [RFC 0/5] restart cleanups Russell King - ARM Linux
  2011-11-01 16:08 ` [PATCH 1/5] ARM: restart: remove argument to setup_mm_for_reboot() Russell King - ARM Linux
@ 2011-11-01 16:08 ` Russell King - ARM Linux
  2011-11-01 16:09 ` [PATCH 3/5] ARM: restart: remove local_irq_disable() from within arch_reset() Russell King - ARM Linux
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 32+ messages in thread
From: Russell King - ARM Linux @ 2011-11-01 16:08 UTC (permalink / raw)
  To: linux-arm-kernel

We don't need an additional level of indirection here, we can just
turn arch_reset into a function pointer directly and assign the
appropriate function (with appropriate arguments to it) just like
other platforms do.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/mach-at91/at91cap9.c            |    4 ++--
 arch/arm/mach-at91/at91rm9200.c          |    4 ++--
 arch/arm/mach-at91/at91sam9260.c         |    2 +-
 arch/arm/mach-at91/at91sam9261.c         |    2 +-
 arch/arm/mach-at91/at91sam9263.c         |    2 +-
 arch/arm/mach-at91/at91sam9g45.c         |    4 ++--
 arch/arm/mach-at91/at91sam9rl.c          |    2 +-
 arch/arm/mach-at91/generic.h             |    2 +-
 arch/arm/mach-at91/include/mach/system.h |    9 +--------
 9 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/arch/arm/mach-at91/at91cap9.c b/arch/arm/mach-at91/at91cap9.c
index bfc6844..d45a877 100644
--- a/arch/arm/mach-at91/at91cap9.c
+++ b/arch/arm/mach-at91/at91cap9.c
@@ -311,7 +311,7 @@ static struct at91_gpio_bank at91cap9_gpio[] = {
 	}
 };
 
-static void at91cap9_reset(void)
+static void at91cap9_reset(char mode, const char *cmd)
 {
 	at91_sys_write(AT91_RSTC_CR, AT91_RSTC_KEY | AT91_RSTC_PROCRST | AT91_RSTC_PERRST);
 }
@@ -333,7 +333,7 @@ static void __init at91cap9_map_io(void)
 
 static void __init at91cap9_initialize(void)
 {
-	at91_arch_reset = at91cap9_reset;
+	arch_reset = at91cap9_reset;
 	pm_power_off = at91cap9_poweroff;
 	at91_extern_irq = (1 << AT91CAP9_ID_IRQ0) | (1 << AT91CAP9_ID_IRQ1);
 
diff --git a/arch/arm/mach-at91/at91rm9200.c b/arch/arm/mach-at91/at91rm9200.c
index f73302d..15161a2 100644
--- a/arch/arm/mach-at91/at91rm9200.c
+++ b/arch/arm/mach-at91/at91rm9200.c
@@ -286,7 +286,7 @@ static struct at91_gpio_bank at91rm9200_gpio[] = {
 	}
 };
 
-static void at91rm9200_reset(void)
+static void at91rm9200_reset(char mode, const char *cmd)
 {
 	/*
 	 * Perform a hardware reset with the use of the Watchdog timer.
@@ -307,7 +307,7 @@ static void __init at91rm9200_map_io(void)
 
 static void __init at91rm9200_initialize(void)
 {
-	at91_arch_reset = at91rm9200_reset;
+	arch_reset = at91rm9200_reset;
 	at91_extern_irq = (1 << AT91RM9200_ID_IRQ0) | (1 << AT91RM9200_ID_IRQ1)
 			| (1 << AT91RM9200_ID_IRQ2) | (1 << AT91RM9200_ID_IRQ3)
 			| (1 << AT91RM9200_ID_IRQ4) | (1 << AT91RM9200_ID_IRQ5)
diff --git a/arch/arm/mach-at91/at91sam9260.c b/arch/arm/mach-at91/at91sam9260.c
index cb397be..2301336 100644
--- a/arch/arm/mach-at91/at91sam9260.c
+++ b/arch/arm/mach-at91/at91sam9260.c
@@ -317,7 +317,7 @@ static void __init at91sam9260_map_io(void)
 
 static void __init at91sam9260_initialize(void)
 {
-	at91_arch_reset = at91sam9_alt_reset;
+	arch_reset = at91sam9_alt_reset;
 	pm_power_off = at91sam9260_poweroff;
 	at91_extern_irq = (1 << AT91SAM9260_ID_IRQ0) | (1 << AT91SAM9260_ID_IRQ1)
 			| (1 << AT91SAM9260_ID_IRQ2);
diff --git a/arch/arm/mach-at91/at91sam9261.c b/arch/arm/mach-at91/at91sam9261.c
index 6c8e3b5..771134b 100644
--- a/arch/arm/mach-at91/at91sam9261.c
+++ b/arch/arm/mach-at91/at91sam9261.c
@@ -286,7 +286,7 @@ static void __init at91sam9261_map_io(void)
 
 static void __init at91sam9261_initialize(void)
 {
-	at91_arch_reset = at91sam9_alt_reset;
+	arch_reset = at91sam9_alt_reset;
 	pm_power_off = at91sam9261_poweroff;
 	at91_extern_irq = (1 << AT91SAM9261_ID_IRQ0) | (1 << AT91SAM9261_ID_IRQ1)
 			| (1 << AT91SAM9261_ID_IRQ2);
diff --git a/arch/arm/mach-at91/at91sam9263.c b/arch/arm/mach-at91/at91sam9263.c
index 044f3c9..733abdd 100644
--- a/arch/arm/mach-at91/at91sam9263.c
+++ b/arch/arm/mach-at91/at91sam9263.c
@@ -303,7 +303,7 @@ static void __init at91sam9263_map_io(void)
 
 static void __init at91sam9263_initialize(void)
 {
-	at91_arch_reset = at91sam9_alt_reset;
+	arch_reset = at91sam9_alt_reset;
 	pm_power_off = at91sam9263_poweroff;
 	at91_extern_irq = (1 << AT91SAM9263_ID_IRQ0) | (1 << AT91SAM9263_ID_IRQ1);
 
diff --git a/arch/arm/mach-at91/at91sam9g45.c b/arch/arm/mach-at91/at91sam9g45.c
index e04c5fb..9a02804 100644
--- a/arch/arm/mach-at91/at91sam9g45.c
+++ b/arch/arm/mach-at91/at91sam9g45.c
@@ -301,7 +301,7 @@ static struct at91_gpio_bank at91sam9g45_gpio[] = {
 	}
 };
 
-static void at91sam9g45_reset(void)
+static void at91sam9g45_reset(char mode, const char *cmd)
 {
 	at91_sys_write(AT91_RSTC_CR, AT91_RSTC_KEY | AT91_RSTC_PROCRST | AT91_RSTC_PERRST);
 }
@@ -323,7 +323,7 @@ static void __init at91sam9g45_map_io(void)
 
 static void __init at91sam9g45_initialize(void)
 {
-	at91_arch_reset = at91sam9g45_reset;
+	arch_reset = at91sam9g45_reset;
 	pm_power_off = at91sam9g45_poweroff;
 	at91_extern_irq = (1 << AT91SAM9G45_ID_IRQ0);
 
diff --git a/arch/arm/mach-at91/at91sam9rl.c b/arch/arm/mach-at91/at91sam9rl.c
index a238105..043d3ac 100644
--- a/arch/arm/mach-at91/at91sam9rl.c
+++ b/arch/arm/mach-at91/at91sam9rl.c
@@ -292,7 +292,7 @@ static void __init at91sam9rl_map_io(void)
 
 static void __init at91sam9rl_initialize(void)
 {
-	at91_arch_reset = at91sam9_alt_reset;
+	arch_reset = at91sam9_alt_reset;
 	pm_power_off = at91sam9rl_poweroff;
 	at91_extern_irq = (1 << AT91SAM9RL_ID_IRQ0);
 
diff --git a/arch/arm/mach-at91/generic.h b/arch/arm/mach-at91/generic.h
index 938b34f..80bc57f 100644
--- a/arch/arm/mach-at91/generic.h
+++ b/arch/arm/mach-at91/generic.h
@@ -57,7 +57,7 @@ extern void at91_irq_suspend(void);
 extern void at91_irq_resume(void);
 
 /* reset */
-extern void at91sam9_alt_reset(void);
+extern void at91sam9_alt_reset(char, const char *);
 
  /* GPIO */
 #define AT91RM9200_PQFP		3	/* AT91RM9200 PQFP package has 3 banks */
diff --git a/arch/arm/mach-at91/include/mach/system.h b/arch/arm/mach-at91/include/mach/system.h
index 36af14b..b141fe0 100644
--- a/arch/arm/mach-at91/include/mach/system.h
+++ b/arch/arm/mach-at91/include/mach/system.h
@@ -47,13 +47,6 @@ static inline void arch_idle(void)
 #endif
 }
 
-void (*at91_arch_reset)(void);
-
-static inline void arch_reset(char mode, const char *cmd)
-{
-	/* call the CPU-specific reset function */
-	if (at91_arch_reset)
-		(at91_arch_reset)();
-}
+void (*arch_reset)(char mode, const char *cmd);
 
 #endif
-- 
1.7.4.4

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH 3/5] ARM: restart: remove local_irq_disable() from within arch_reset()
  2011-11-01 16:07 [RFC 0/5] restart cleanups Russell King - ARM Linux
  2011-11-01 16:08 ` [PATCH 1/5] ARM: restart: remove argument to setup_mm_for_reboot() Russell King - ARM Linux
  2011-11-01 16:08 ` [PATCH 2/5] ARM: restart: get rid of needless at91_arch_reset additional indirection Russell King - ARM Linux
@ 2011-11-01 16:09 ` Russell King - ARM Linux
  2011-11-01 16:36   ` H Hartley Sweeten
  2011-11-01 16:09 ` [PATCH 4/5] ARM: restart: turn reboot setup code into a library function Russell King - ARM Linux
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 32+ messages in thread
From: Russell King - ARM Linux @ 2011-11-01 16:09 UTC (permalink / raw)
  To: linux-arm-kernel

IRQs are already disabled by the time arch_reset() is called, so these
calls to local_irq_disable() instead arch_reset() are redundant.  Remove
them.

The following spatch was used:
@ rule1 @ identifier fn; @@
arch_reset = fn;

@@ identifier rule1.fn, m, c; @@
void fn(char m, const char *c)
{
<... when != local_irq_enable
-local_irq_disable();
...>
}

@@ identifier m, c; @@
arch_reset(char m, const char *c)
{
<... when != local_irq_enable
-local_irq_disable();
...>
}

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/mach-ep93xx/include/mach/system.h  |    2 --
 arch/arm/mach-iop32x/include/mach/system.h  |    2 --
 arch/arm/mach-ixp2000/include/mach/system.h |    2 --
 arch/arm/mach-lpc32xx/include/mach/system.h |    3 ---
 arch/arm/mach-shark/core.c                  |    1 -
 arch/arm/mach-u300/include/mach/system.h    |    1 -
 6 files changed, 0 insertions(+), 11 deletions(-)

diff --git a/arch/arm/mach-ep93xx/include/mach/system.h b/arch/arm/mach-ep93xx/include/mach/system.h
index 6d661fe..bdf6c4f 100644
--- a/arch/arm/mach-ep93xx/include/mach/system.h
+++ b/arch/arm/mach-ep93xx/include/mach/system.h
@@ -11,8 +11,6 @@ static inline void arch_idle(void)
 
 static inline void arch_reset(char mode, const char *cmd)
 {
-	local_irq_disable();
-
 	/*
 	 * Set then clear the SWRST bit to initiate a software reset
 	 */
diff --git a/arch/arm/mach-iop32x/include/mach/system.h b/arch/arm/mach-iop32x/include/mach/system.h
index a4b808f..5987196 100644
--- a/arch/arm/mach-iop32x/include/mach/system.h
+++ b/arch/arm/mach-iop32x/include/mach/system.h
@@ -18,8 +18,6 @@ static inline void arch_idle(void)
 
 static inline void arch_reset(char mode, const char *cmd)
 {
-	local_irq_disable();
-
 	if (machine_is_n2100()) {
 		gpio_line_set(N2100_HARDWARE_RESET, GPIO_LOW);
 		gpio_line_config(N2100_HARDWARE_RESET, GPIO_OUT);
diff --git a/arch/arm/mach-ixp2000/include/mach/system.h b/arch/arm/mach-ixp2000/include/mach/system.h
index de37099..810df7b 100644
--- a/arch/arm/mach-ixp2000/include/mach/system.h
+++ b/arch/arm/mach-ixp2000/include/mach/system.h
@@ -19,8 +19,6 @@ static inline void arch_idle(void)
 
 static inline void arch_reset(char mode, const char *cmd)
 {
-	local_irq_disable();
-
 	/*
 	 * Reset flash banking register so that we are pointing at
 	 * RedBoot bank.
diff --git a/arch/arm/mach-lpc32xx/include/mach/system.h b/arch/arm/mach-lpc32xx/include/mach/system.h
index df3b0de..d47f3b1 100644
--- a/arch/arm/mach-lpc32xx/include/mach/system.h
+++ b/arch/arm/mach-lpc32xx/include/mach/system.h
@@ -33,9 +33,6 @@ static inline void arch_reset(char mode, const char *cmd)
 	case 'h':
 		printk(KERN_CRIT "RESET: Rebooting system\n");
 
-		/* Disable interrupts */
-		local_irq_disable();
-
 		lpc32xx_watchdog_reset();
 		break;
 
diff --git a/arch/arm/mach-shark/core.c b/arch/arm/mach-shark/core.c
index ac2873c..be82b2a 100644
--- a/arch/arm/mach-shark/core.c
+++ b/arch/arm/mach-shark/core.c
@@ -29,7 +29,6 @@
 void arch_reset(char mode, const char *cmd)
 {
         short temp;
-        local_irq_disable();
         /* Reset the Machine via pc[3] of the sequoia chipset */
         outw(0x09,0x24);
         temp=inw(0x26);
diff --git a/arch/arm/mach-u300/include/mach/system.h b/arch/arm/mach-u300/include/mach/system.h
index 8daf136..a000c04 100644
--- a/arch/arm/mach-u300/include/mach/system.h
+++ b/arch/arm/mach-u300/include/mach/system.h
@@ -28,7 +28,6 @@ static void arch_reset(char mode, const char *cmd)
 	case 'h':
 		printk(KERN_CRIT "RESET: shutting down/rebooting system\n");
 		/* Disable interrupts */
-		local_irq_disable();
 #ifdef CONFIG_COH901327_WATCHDOG
 		coh901327_watchdog_reset();
 #endif
-- 
1.7.4.4

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH 4/5] ARM: restart: turn reboot setup code into a library function
  2011-11-01 16:07 [RFC 0/5] restart cleanups Russell King - ARM Linux
                   ` (2 preceding siblings ...)
  2011-11-01 16:09 ` [PATCH 3/5] ARM: restart: remove local_irq_disable() from within arch_reset() Russell King - ARM Linux
@ 2011-11-01 16:09 ` Russell King - ARM Linux
  2011-11-01 16:09 ` [PATCH 5/5] ARM: restart: only perform setup for restart when soft-restarting Russell King - ARM Linux
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 32+ messages in thread
From: Russell King - ARM Linux @ 2011-11-01 16:09 UTC (permalink / raw)
  To: linux-arm-kernel

Move the failure to reboot into machine_restart() to always catch
this condition, and turn the bulk of arm_machine_restart() which is
required for soft-reboot modes into a library call.

This allows everyone to hook into the restart via the same method
via arm_pm_restart() irrespective of what they want, and if they
want the old 'arch_reset' way, call setup_restart() themselves.

Acked-by: Will Deacon <will.deacon@arm.com>
Tested-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/kernel/process.c |   35 +++++++++++++++++++----------------
 1 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 62946c0..db8d836 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -92,12 +92,8 @@ static int __init hlt_setup(char *__unused)
 __setup("nohlt", nohlt_setup);
 __setup("hlt", hlt_setup);
 
-void arm_machine_restart(char mode, const char *cmd)
+static void setup_restart(void)
 {
-	/* Disable interrupts first */
-	local_irq_disable();
-	local_fiq_disable();
-
 	/*
 	 * Tell the mm system that we are going to reboot -
 	 * we may need it to insert some 1:1 mappings so that
@@ -113,19 +109,18 @@ void arm_machine_restart(char mode, const char *cmd)
 
 	/* Push out any further dirty data, and ensure cache is empty */
 	flush_cache_all();
+}
 
-	/*
-	 * Now call the architecture specific reboot code.
-	 */
-	arch_reset(mode, cmd);
+void arm_machine_restart(char mode, const char *cmd)
+{
+	/* Disable interrupts first */
+	local_irq_disable();
+	local_fiq_disable();
 
-	/*
-	 * Whoops - the architecture was unable to reboot.
-	 * Tell the user!
-	 */
-	mdelay(1000);
-	printk("Reboot failed -- System halted\n");
-	while (1);
+	setup_restart();
+
+	/* Now call the architecture specific reboot code. */
+	arch_reset(mode, cmd);
 }
 
 /*
@@ -250,7 +245,15 @@ void machine_power_off(void)
 void machine_restart(char *cmd)
 {
 	machine_shutdown();
+
 	arm_pm_restart(reboot_mode, cmd);
+
+	/* Give a grace period for failure to restart of 1s */
+	mdelay(1000);
+
+	/* Whoops - the platform was unable to reboot. Tell the user! */
+	printk("Reboot failed -- System halted\n");
+	while (1);
 }
 
 void __show_regs(struct pt_regs *regs)
-- 
1.7.4.4

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH 5/5] ARM: restart: only perform setup for restart when soft-restarting
  2011-11-01 16:07 [RFC 0/5] restart cleanups Russell King - ARM Linux
                   ` (3 preceding siblings ...)
  2011-11-01 16:09 ` [PATCH 4/5] ARM: restart: turn reboot setup code into a library function Russell King - ARM Linux
@ 2011-11-01 16:09 ` Russell King - ARM Linux
  2011-11-02 12:12   ` Will Deacon
  2011-11-01 16:38 ` [RFC 0/5] restart cleanups Russell King - ARM Linux
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 32+ messages in thread
From: Russell King - ARM Linux @ 2011-11-01 16:09 UTC (permalink / raw)
  To: linux-arm-kernel

We only need to set the system up for a soft-restart if we're going to
be doing a soft-restart.  Provide a new function (soft_restart()) which
does the setup and final call for this, and make platforms use it.
Eliminate the call to setup_restart() from the default handler.

This means that platforms arch_reset() function is no longer called with
the page tables prepared for a soft-restart, and caches will still be
enabled.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/include/asm/system.h                     |    1 +
 arch/arm/kernel/process.c                         |   10 +++++++---
 arch/arm/mach-clps711x/include/mach/system.h      |    2 +-
 arch/arm/mach-ebsa110/include/mach/system.h       |    2 +-
 arch/arm/mach-footbridge/include/mach/system.h    |    2 +-
 arch/arm/mach-iop32x/include/mach/system.h        |    2 +-
 arch/arm/mach-iop33x/include/mach/system.h        |    2 +-
 arch/arm/mach-ixp4xx/include/mach/system.h        |    2 +-
 arch/arm/mach-ks8695/include/mach/system.h        |    2 +-
 arch/arm/mach-mmp/include/mach/system.h           |    4 ++--
 arch/arm/mach-mxs/system.c                        |    2 +-
 arch/arm/mach-nuc93x/include/mach/system.h        |    2 +-
 arch/arm/mach-pnx4008/include/mach/system.h       |    2 +-
 arch/arm/mach-pxa/reset.c                         |    2 +-
 arch/arm/mach-rpc/include/mach/system.h           |    2 +-
 arch/arm/mach-s3c2410/include/mach/system-reset.h |    4 ++--
 arch/arm/mach-s3c64xx/include/mach/system.h       |    2 +-
 arch/arm/mach-sa1100/include/mach/system.h        |    2 +-
 arch/arm/mach-shmobile/include/mach/system.h      |    2 +-
 arch/arm/mach-w90x900/include/mach/system.h       |    2 +-
 arch/arm/plat-mxc/system.c                        |    2 +-
 arch/arm/plat-spear/include/plat/system.h         |    2 +-
 22 files changed, 30 insertions(+), 25 deletions(-)

diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h
index 984014b..fe7de75 100644
--- a/arch/arm/include/asm/system.h
+++ b/arch/arm/include/asm/system.h
@@ -101,6 +101,7 @@ extern int __pure cpu_architecture(void);
 extern void cpu_init(void);
 
 void arm_machine_restart(char mode, const char *cmd);
+void soft_restart(unsigned long);
 extern void (*arm_pm_restart)(char str, const char *cmd);
 
 #define UDBG_UNDEFINED	(1 << 0)
diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index db8d836..6d1044b 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -111,15 +111,19 @@ static void setup_restart(void)
 	flush_cache_all();
 }
 
+void soft_restart(unsigned long addr)
+{
+	setup_restart();
+	cpu_reset(addr);
+}
+
 void arm_machine_restart(char mode, const char *cmd)
 {
 	/* Disable interrupts first */
 	local_irq_disable();
 	local_fiq_disable();
 
-	setup_restart();
-
-	/* Now call the architecture specific reboot code. */
+	/* Call the architecture specific reboot code. */
 	arch_reset(mode, cmd);
 }
 
diff --git a/arch/arm/mach-clps711x/include/mach/system.h b/arch/arm/mach-clps711x/include/mach/system.h
index f916cd7..6c11993 100644
--- a/arch/arm/mach-clps711x/include/mach/system.h
+++ b/arch/arm/mach-clps711x/include/mach/system.h
@@ -34,7 +34,7 @@ static inline void arch_idle(void)
 
 static inline void arch_reset(char mode, const char *cmd)
 {
-	cpu_reset(0);
+	soft_restart(0);
 }
 
 #endif
diff --git a/arch/arm/mach-ebsa110/include/mach/system.h b/arch/arm/mach-ebsa110/include/mach/system.h
index 9a26245..0d5df72 100644
--- a/arch/arm/mach-ebsa110/include/mach/system.h
+++ b/arch/arm/mach-ebsa110/include/mach/system.h
@@ -34,6 +34,6 @@ static inline void arch_idle(void)
 	asm volatile ("mcr p15, 0, ip, c15, c1, 2" : : : "cc");
 }
 
-#define arch_reset(mode, cmd)	cpu_reset(0x80000000)
+#define arch_reset(mode, cmd)	soft_restart(0x80000000)
 
 #endif
diff --git a/arch/arm/mach-footbridge/include/mach/system.h b/arch/arm/mach-footbridge/include/mach/system.h
index 0b29315..249f895 100644
--- a/arch/arm/mach-footbridge/include/mach/system.h
+++ b/arch/arm/mach-footbridge/include/mach/system.h
@@ -24,7 +24,7 @@ static inline void arch_reset(char mode, const char *cmd)
 		/*
 		 * Jump into the ROM
 		 */
-		cpu_reset(0x41000000);
+		soft_restart(0x41000000);
 	} else {
 		if (machine_is_netwinder()) {
 			/* open up the SuperIO chip
diff --git a/arch/arm/mach-iop32x/include/mach/system.h b/arch/arm/mach-iop32x/include/mach/system.h
index 5987196..b4f83e5 100644
--- a/arch/arm/mach-iop32x/include/mach/system.h
+++ b/arch/arm/mach-iop32x/include/mach/system.h
@@ -28,5 +28,5 @@ static inline void arch_reset(char mode, const char *cmd)
 	*IOP3XX_PCSR = 0x30;
 
 	/* Jump into ROM at address 0 */
-	cpu_reset(0);
+	soft_restart(0);
 }
diff --git a/arch/arm/mach-iop33x/include/mach/system.h b/arch/arm/mach-iop33x/include/mach/system.h
index f192a34..86d1b20 100644
--- a/arch/arm/mach-iop33x/include/mach/system.h
+++ b/arch/arm/mach-iop33x/include/mach/system.h
@@ -19,5 +19,5 @@ static inline void arch_reset(char mode, const char *cmd)
 	*IOP3XX_PCSR = 0x30;
 
 	/* Jump into ROM at address 0 */
-	cpu_reset(0);
+	soft_restart(0);
 }
diff --git a/arch/arm/mach-ixp4xx/include/mach/system.h b/arch/arm/mach-ixp4xx/include/mach/system.h
index 54c0af7..24337d9 100644
--- a/arch/arm/mach-ixp4xx/include/mach/system.h
+++ b/arch/arm/mach-ixp4xx/include/mach/system.h
@@ -26,7 +26,7 @@ static inline void arch_reset(char mode, const char *cmd)
 {
 	if ( 1 && mode == 's') {
 		/* Jump into ROM at address 0 */
-		cpu_reset(0);
+		soft_restart(0);
 	} else {
 		/* Use on-chip reset capability */
 
diff --git a/arch/arm/mach-ks8695/include/mach/system.h b/arch/arm/mach-ks8695/include/mach/system.h
index fb1dda9..ceb19c9 100644
--- a/arch/arm/mach-ks8695/include/mach/system.h
+++ b/arch/arm/mach-ks8695/include/mach/system.h
@@ -32,7 +32,7 @@ static void arch_reset(char mode, const char *cmd)
 	unsigned int reg;
 
 	if (mode == 's')
-		cpu_reset(0);
+		soft_restart(0);
 
 	/* disable timer0 */
 	reg = __raw_readl(KS8695_TMR_VA + KS8695_TMCON);
diff --git a/arch/arm/mach-mmp/include/mach/system.h b/arch/arm/mach-mmp/include/mach/system.h
index 1a8a25e..cb06379 100644
--- a/arch/arm/mach-mmp/include/mach/system.h
+++ b/arch/arm/mach-mmp/include/mach/system.h
@@ -19,8 +19,8 @@ static inline void arch_idle(void)
 static inline void arch_reset(char mode, const char *cmd)
 {
 	if (cpu_is_pxa168())
-		cpu_reset(0xffff0000);
+		soft_restart(0xffff0000);
 	else
-		cpu_reset(0);
+		soft_restart(0);
 }
 #endif /* __ASM_MACH_SYSTEM_H */
diff --git a/arch/arm/mach-mxs/system.c b/arch/arm/mach-mxs/system.c
index 20ec3bd..cab8836 100644
--- a/arch/arm/mach-mxs/system.c
+++ b/arch/arm/mach-mxs/system.c
@@ -53,7 +53,7 @@ void arch_reset(char mode, const char *cmd)
 	mdelay(50);
 
 	/* We'll take a jump through zero as a poor second */
-	cpu_reset(0);
+	soft_restart(0);
 }
 
 static int __init mxs_arch_reset_init(void)
diff --git a/arch/arm/mach-nuc93x/include/mach/system.h b/arch/arm/mach-nuc93x/include/mach/system.h
index d26bd9a..55704e2 100644
--- a/arch/arm/mach-nuc93x/include/mach/system.h
+++ b/arch/arm/mach-nuc93x/include/mach/system.h
@@ -23,6 +23,6 @@ static void arch_idle(void)
 
 static void arch_reset(char mode, const char *cmd)
 {
-	cpu_reset(0);
+	soft_restart(0);
 }
 
diff --git a/arch/arm/mach-pnx4008/include/mach/system.h b/arch/arm/mach-pnx4008/include/mach/system.h
index 5dda2bb..5d6384a 100644
--- a/arch/arm/mach-pnx4008/include/mach/system.h
+++ b/arch/arm/mach-pnx4008/include/mach/system.h
@@ -32,7 +32,7 @@ static void arch_idle(void)
 
 static inline void arch_reset(char mode, const char *cmd)
 {
-	cpu_reset(0);
+	soft_restart(0);
 }
 
 #endif
diff --git a/arch/arm/mach-pxa/reset.c b/arch/arm/mach-pxa/reset.c
index 01e9d64..b8bcda1 100644
--- a/arch/arm/mach-pxa/reset.c
+++ b/arch/arm/mach-pxa/reset.c
@@ -88,7 +88,7 @@ void arch_reset(char mode, const char *cmd)
 	switch (mode) {
 	case 's':
 		/* Jump into ROM at address 0 */
-		cpu_reset(0);
+		soft_restart(0);
 		break;
 	case 'g':
 		do_gpio_reset();
diff --git a/arch/arm/mach-rpc/include/mach/system.h b/arch/arm/mach-rpc/include/mach/system.h
index 45c7b93..a354f4d 100644
--- a/arch/arm/mach-rpc/include/mach/system.h
+++ b/arch/arm/mach-rpc/include/mach/system.h
@@ -23,5 +23,5 @@ static inline void arch_reset(char mode, const char *cmd)
 	/*
 	 * Jump into the ROM
 	 */
-	cpu_reset(0);
+	soft_restart(0);
 }
diff --git a/arch/arm/mach-s3c2410/include/mach/system-reset.h b/arch/arm/mach-s3c2410/include/mach/system-reset.h
index 6faadce..913893d 100644
--- a/arch/arm/mach-s3c2410/include/mach/system-reset.h
+++ b/arch/arm/mach-s3c2410/include/mach/system-reset.h
@@ -19,7 +19,7 @@ static void
 arch_reset(char mode, const char *cmd)
 {
 	if (mode == 's') {
-		cpu_reset(0);
+		soft_restart(0);
 	}
 
 	if (s3c24xx_reset_hook)
@@ -28,5 +28,5 @@ arch_reset(char mode, const char *cmd)
 	arch_wdt_reset();
 
 	/* we'll take a jump through zero as a poor second */
-	cpu_reset(0);
+	soft_restart(0);
 }
diff --git a/arch/arm/mach-s3c64xx/include/mach/system.h b/arch/arm/mach-s3c64xx/include/mach/system.h
index 2e58cb7..d8ca578 100644
--- a/arch/arm/mach-s3c64xx/include/mach/system.h
+++ b/arch/arm/mach-s3c64xx/include/mach/system.h
@@ -24,7 +24,7 @@ static void arch_reset(char mode, const char *cmd)
 		arch_wdt_reset();
 
 	/* if all else fails, or mode was for soft, jump to 0 */
-	cpu_reset(0);
+	soft_restart(0);
 }
 
 #endif /* __ASM_ARCH_IRQ_H */
diff --git a/arch/arm/mach-sa1100/include/mach/system.h b/arch/arm/mach-sa1100/include/mach/system.h
index ba9da9f..345d35b 100644
--- a/arch/arm/mach-sa1100/include/mach/system.h
+++ b/arch/arm/mach-sa1100/include/mach/system.h
@@ -14,7 +14,7 @@ static inline void arch_reset(char mode, const char *cmd)
 {
 	if (mode == 's') {
 		/* Jump into ROM at address 0 */
-		cpu_reset(0);
+		soft_restart(0);
 	} else {
 		/* Use on-chip reset capability */
 		RSRR = RSRR_SWR;
diff --git a/arch/arm/mach-shmobile/include/mach/system.h b/arch/arm/mach-shmobile/include/mach/system.h
index 76a687e..956ac18 100644
--- a/arch/arm/mach-shmobile/include/mach/system.h
+++ b/arch/arm/mach-shmobile/include/mach/system.h
@@ -8,7 +8,7 @@ static inline void arch_idle(void)
 
 static inline void arch_reset(char mode, const char *cmd)
 {
-	cpu_reset(0);
+	soft_restart(0);
 }
 
 #endif
diff --git a/arch/arm/mach-w90x900/include/mach/system.h b/arch/arm/mach-w90x900/include/mach/system.h
index ce228bd..68875a1 100644
--- a/arch/arm/mach-w90x900/include/mach/system.h
+++ b/arch/arm/mach-w90x900/include/mach/system.h
@@ -33,7 +33,7 @@ static void arch_reset(char mode, const char *cmd)
 {
 	if (mode == 's') {
 		/* Jump into ROM at address 0 */
-		cpu_reset(0);
+		soft_restart(0);
 	} else {
 		__raw_writel(WTE | WTRE | WTCLK, WTCR);
 	}
diff --git a/arch/arm/plat-mxc/system.c b/arch/arm/plat-mxc/system.c
index 8024f2a..a1e8f80 100644
--- a/arch/arm/plat-mxc/system.c
+++ b/arch/arm/plat-mxc/system.c
@@ -67,7 +67,7 @@ void arch_reset(char mode, const char *cmd)
 	mdelay(50);
 
 	/* we'll take a jump through zero as a poor second */
-	cpu_reset(0);
+	soft_restart(0);
 }
 
 void mxc_arch_reset_init(void __iomem *base)
diff --git a/arch/arm/plat-spear/include/plat/system.h b/arch/arm/plat-spear/include/plat/system.h
index a235fa0..1171f22 100644
--- a/arch/arm/plat-spear/include/plat/system.h
+++ b/arch/arm/plat-spear/include/plat/system.h
@@ -31,7 +31,7 @@ static inline void arch_reset(char mode, const char *cmd)
 {
 	if (mode == 's') {
 		/* software reset, Jump into ROM@address 0 */
-		cpu_reset(0);
+		soft_restart(0);
 	} else {
 		/* hardware reset, Use on-chip reset capability */
 		sysctl_soft_reset((void __iomem *)VA_SPEAR_SYS_CTRL_BASE);
-- 
1.7.4.4

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH 3/5] ARM: restart: remove local_irq_disable() from within arch_reset()
  2011-11-01 16:09 ` [PATCH 3/5] ARM: restart: remove local_irq_disable() from within arch_reset() Russell King - ARM Linux
@ 2011-11-01 16:36   ` H Hartley Sweeten
  2011-11-01 17:56     ` Russell King - ARM Linux
  0 siblings, 1 reply; 32+ messages in thread
From: H Hartley Sweeten @ 2011-11-01 16:36 UTC (permalink / raw)
  To: linux-arm-kernel

On Tuesday, November 01, 2011 9:09 AM, Russell King wrote:
>
> IRQs are already disabled by the time arch_reset() is called, so these
> calls to local_irq_disable() instead arch_reset() are redundant.  Remove
> them.
>
> The following spatch was used:
> @ rule1 @ identifier fn; @@
> arch_reset = fn;
>
> @@ identifier rule1.fn, m, c; @@
> void fn(char m, const char *c)
> {
> <... when != local_irq_enable
> -local_irq_disable();
> ...>
> }
>
> @@ identifier m, c; @@
> arch_reset(char m, const char *c)
> {
> <... when != local_irq_enable
> -local_irq_disable();
> ...>
> }
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  arch/arm/mach-ep93xx/include/mach/system.h  |    2 --

[...]

> diff --git a/arch/arm/mach-ep93xx/include/mach/system.h b/arch/arm/mach-ep93xx/include/mach/system.h
> index 6d661fe..bdf6c4f 100644
> --- a/arch/arm/mach-ep93xx/include/mach/system.h
> +++ b/arch/arm/mach-ep93xx/include/mach/system.h
> @@ -11,8 +11,6 @@ static inline void arch_idle(void)
>  
>  static inline void arch_reset(char mode, const char *cmd)
>  {
> -	local_irq_disable();
> -
>  	/*
>  	 * Set then clear the SWRST bit to initiate a software reset
>  	 */

For ep93xx.

Acked-by: H Hartley Sweeten <hsweeten@visionengravers.com>

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [RFC 0/5] restart cleanups
  2011-11-01 16:07 [RFC 0/5] restart cleanups Russell King - ARM Linux
                   ` (4 preceding siblings ...)
  2011-11-01 16:09 ` [PATCH 5/5] ARM: restart: only perform setup for restart when soft-restarting Russell King - ARM Linux
@ 2011-11-01 16:38 ` Russell King - ARM Linux
  2011-11-03  9:12   ` Russell King - ARM Linux
  2011-11-01 17:02 ` Russell King - ARM Linux
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 32+ messages in thread
From: Russell King - ARM Linux @ 2011-11-01 16:38 UTC (permalink / raw)
  To: linux-arm-kernel

Can someone please check whether this code:

/* Hook for arm_pm_restart to ensure we execute the reset code
 * with the caches enabled. It seems at least the S3C2440 has a problem
 * resetting if there is bus activity interrupted by the reset.
 */
static void s3c24xx_pm_restart(char mode, const char *cmd)
{
        if (mode != 's') {
                unsigned long flags;

                local_irq_save(flags);
                __cpuc_flush_kern_all();
                __cpuc_flush_user_all();

                arch_reset(mode, cmd);
                local_irq_restore(flags);
        }

        /* fallback, or unhandled */
        arm_machine_restart(mode, cmd);
}

will be broken by any of these patches - and more importantly whether
this code can simply be deleted with these five patches applied.

Thanks.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [RFC 0/5] restart cleanups
  2011-11-01 16:07 [RFC 0/5] restart cleanups Russell King - ARM Linux
                   ` (5 preceding siblings ...)
  2011-11-01 16:38 ` [RFC 0/5] restart cleanups Russell King - ARM Linux
@ 2011-11-01 17:02 ` Russell King - ARM Linux
  2011-11-02 13:49   ` Richard Purdie
  2011-11-01 19:23 ` Will Deacon
  2011-11-02 13:35 ` Nicolas Pitre
  8 siblings, 1 reply; 32+ messages in thread
From: Russell King - ARM Linux @ 2011-11-01 17:02 UTC (permalink / raw)
  To: linux-arm-kernel

(Poodle doesn't have any MAINTAINERS entry, I think Richard Purdie last
touched Poodle in a 'maintainery' way back in 2006.)

poodle does this:

static void poodle_restart(char mode, const char *cmd)
{
        arm_machine_restart('h', cmd);
}

Given that the default value of 'mode' (in the absense of any reboot=
command line argument to the kernel) is 'h', this seems to be a no-op.
Below is the history; I suspect that the reason this first appeared was
because of the need to fiddle with RCSR - that's now gone so I suspect
the entire restart handler can be deleted.

What I don't understand in that case is why 'mode' wasn't simply passed
through in the first patch.

Please confirm.

commit 74617fb6b825ea370ae72565f7543306bc08ef6e
Author: Richard Purdie <rpurdie@rpsys.net>
Date:   Mon Jun 19 19:57:12 2006 +0100

    [ARM] 3593/1: Add reboot and shutdown handlers for Zaurus handhelds

    Patch from Richard Purdie

    Add functionality to allow machine specific reboot handlers on ARM.
    Add machine specific reboot and poweroff handlers for all PXA Zaurus
    models.

diff --git a/arch/arm/mach-pxa/poodle.c b/arch/arm/mach-pxa/poodle.c

@@ -247,10 +249,25 @@ static struct platform_device *devices[] __initdata = {
        &poodle_scoop_device,
 };

+static void poodle_poweroff(void)
+{
+       RCSR = RCSR_HWR | RCSR_WDR | RCSR_SMR | RCSR_GPR;
+       arm_machine_restart('h');
+}
+
+static void poodle_restart(char mode)
+{
+       RCSR = RCSR_HWR | RCSR_WDR | RCSR_SMR | RCSR_GPR;
+       arm_machine_restart('h');
+}
+
 static void __init poodle_init(void)
 {
        int ret = 0;

+       pm_power_off = poodle_poweroff;
+       arm_pm_restart = poodle_restart;

...
commit dc38e2ad53ca27968919dea6d7fa60575782d5a6
Author: Russell King <rmk@dyn-67.arm.linux.org.uk>
Date:   Thu May 8 16:50:39 2008 +0100

    [ARM] pxa: Fix RCSR handling

    Related to d3930614e68bdf83a120d904c039a64e9f75dba1.

    RCSR is only present on PXA2xx CPUs, not on PXA3xx CPUs.  Therefore,
    we should not be unconditionally writing to RCSR from generic code.

    Since we now clear the RCSR status from the SoC specific PXA PM code
    and before reset in the arch_reset() function, the duplication in
    the corgi, poodle, spitz and tosa code can be removed.

diff --git a/arch/arm/mach-pxa/poodle.c b/arch/arm/mach-pxa/poodle.c
index ca5ac19..0b30f25 100644
--- a/arch/arm/mach-pxa/poodle.c
+++ b/arch/arm/mach-pxa/poodle.c
@@ -326,13 +326,11 @@ static struct platform_device *devices[] __initdata = {

 static void poodle_poweroff(void)
 {
-       RCSR = RCSR_HWR | RCSR_WDR | RCSR_SMR | RCSR_GPR;
        arm_machine_restart('h');
 }

 static void poodle_restart(char mode)
 {
-       RCSR = RCSR_HWR | RCSR_WDR | RCSR_SMR | RCSR_GPR;
        arm_machine_restart('h');
 }

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH 3/5] ARM: restart: remove local_irq_disable() from within arch_reset()
  2011-11-01 16:36   ` H Hartley Sweeten
@ 2011-11-01 17:56     ` Russell King - ARM Linux
  0 siblings, 0 replies; 32+ messages in thread
From: Russell King - ARM Linux @ 2011-11-01 17:56 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Nov 01, 2011 at 11:36:14AM -0500, H Hartley Sweeten wrote:
> > diff --git a/arch/arm/mach-ep93xx/include/mach/system.h b/arch/arm/mach-ep93xx/include/mach/system.h
> > index 6d661fe..bdf6c4f 100644
> > --- a/arch/arm/mach-ep93xx/include/mach/system.h
> > +++ b/arch/arm/mach-ep93xx/include/mach/system.h
> > @@ -11,8 +11,6 @@ static inline void arch_idle(void)
> >  
> >  static inline void arch_reset(char mode, const char *cmd)
> >  {
> > -	local_irq_disable();
> > -
> >  	/*
> >  	 * Set then clear the SWRST bit to initiate a software reset
> >  	 */
> 
> For ep93xx.
> 
> Acked-by: H Hartley Sweeten <hsweeten@visionengravers.com>

Thanks.  I noticed that u300 had a comment which should've been deleted,
so I've also done that and removed the spatch from the commit log (as it
no longer reflects the patch.)

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [RFC 0/5] restart cleanups
  2011-11-01 16:07 [RFC 0/5] restart cleanups Russell King - ARM Linux
                   ` (6 preceding siblings ...)
  2011-11-01 17:02 ` Russell King - ARM Linux
@ 2011-11-01 19:23 ` Will Deacon
  2011-11-01 19:41   ` Russell King - ARM Linux
  2011-11-02 13:35 ` Nicolas Pitre
  8 siblings, 1 reply; 32+ messages in thread
From: Will Deacon @ 2011-11-01 19:23 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Russell,

On Tue, Nov 01, 2011 at 04:07:31PM +0000, Russell King - ARM Linux wrote:
> This series cleans up some of the restart code for ARM.  It doesn't go
> as far as Will's patch set (or even anywhere close to it) mainly because
> of the poor state of the various implementations in various platforms.

Thanks for helping out with this!

> Effectively, the various platforms themselves stand in the way of optimal
> cleanup of this code.

Agreed. I'm hoping that people will complain if we break them by accident.

> Other platforms use arm_pm_restart() to intercept the restart, and
> force the mode to 'g', 'h' or 's'.  As the mode already defaults to
> 'h' I don't understand why people felt forcing it in that case was
> necessary.  As for the 's' case, this can be done by merely specifying
> ".soft_reboot = 1" in the machine record.  We can solve the 'g' case
> by changing .soft_reboot to be a char "restart_mode" argument which
> sets the default restart mode.

Actually, I think we can go as far as deleteing the 'g' mode and using the
GPIO reset as a fallback if we fail in the usual 'h' case. This mode is only
used by mach-pxa, so it would be good to get rid of it.

We can always do this in a later patch series if it gets in the way (I have
a series to clean up the reboot modes already).

> So, this patch set takes us a little way there by cleaning up some of
> the restart code:
> 
> 1. remove the useless argument to setup_mm_for_reboot()

I'm definitely in favour of this change, but be aware that I plan to modify
setup_mm_for_reboot in the future to:

  (a) Use a static identity mapping so we don't have to pgd_alloc explicitly
      [I can't see this affecting anything]

  (b) Extend the identity mapping into kernel space, leaving some room for
      the kernel image. If this is all called from soft_reboot, which just
      does the cpu_reset(addr) afterwards, then we're fine. I have code to
      take care of changing stack and switching to the new mapping.

> 5. provide (and use) a function for soft rebooting which includes the
>    setup of the MMU tables and call cpu_reset() - and remove the MMU
>    setup from the code paths before arch_reset() is called.

This function will probably grow to contain the arm_machine_reset function
body that I have for kexec.

If you're happy with the direction I'd like to take this in, then you can
have my Acked-by tags for patches 1 and 5.

Cheers,

Will

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [RFC 0/5] restart cleanups
  2011-11-01 19:23 ` Will Deacon
@ 2011-11-01 19:41   ` Russell King - ARM Linux
  2011-11-01 21:19     ` Will Deacon
  0 siblings, 1 reply; 32+ messages in thread
From: Russell King - ARM Linux @ 2011-11-01 19:41 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Nov 01, 2011 at 07:23:43PM +0000, Will Deacon wrote:
> Hi Russell,
> 
> On Tue, Nov 01, 2011 at 04:07:31PM +0000, Russell King - ARM Linux wrote:
> > Other platforms use arm_pm_restart() to intercept the restart, and
> > force the mode to 'g', 'h' or 's'.  As the mode already defaults to
> > 'h' I don't understand why people felt forcing it in that case was
> > necessary.  As for the 's' case, this can be done by merely specifying
> > ".soft_reboot = 1" in the machine record.  We can solve the 'g' case
> > by changing .soft_reboot to be a char "restart_mode" argument which
> > sets the default restart mode.
> 
> Actually, I think we can go as far as deleteing the 'g' mode and using the
> GPIO reset as a fallback if we fail in the usual 'h' case. This mode is only
> used by mach-pxa, so it would be good to get rid of it.
> 
> We can always do this in a later patch series if it gets in the way (I have
> a series to clean up the reboot modes already).

I think it's useful to distinguish between them especially as there's
platforms in PXA land doing weirdo things with this.  Some platforms
use the GPIO reset thing as a way to shut the system down as well as
reboot it, so it's not purely just about reboot (which is in itself
annoying).

The other thing is that the generic code shouldn't care what value 'mode'
actually is - we just pass it through to the platform handler (via
arm_pm_restart()) and if the platform handler wants to ignore it and
just do the software reboot thing, it uses soft_restart(addr) which
does all the necessary setup for that.

The nice thing about that is we no longer have to worry about calling
arm_pm_restart() with 'h' and a platform doing a soft-reboot instead.

> > So, this patch set takes us a little way there by cleaning up some of
> > the restart code:
> > 
> > 1. remove the useless argument to setup_mm_for_reboot()
> 
> I'm definitely in favour of this change, but be aware that I plan to modify
> setup_mm_for_reboot in the future to:
> 
>   (a) Use a static identity mapping so we don't have to pgd_alloc explicitly
>       [I can't see this affecting anything]
> 
>   (b) Extend the identity mapping into kernel space, leaving some room for
>       the kernel image. If this is all called from soft_reboot, which just
>       does the cpu_reset(addr) afterwards, then we're fine. I have code to
>       take care of changing stack and switching to the new mapping.

Well, bear in mind that (1) + (5) produce something really self-contained.
We no longer fiddle with MMU tables for anything but the 'soft reboot'
path.  That means things like hardware based reset (eg, using watchdogs,
asserting GPIOs, etc) are all dealt with without fiddling with the MMU
and caches.

The side effect of this is that the whole soft-reboot thing is now a
self-contained chunk of code which can deal with setting things up in
order to do what it needs to do without fear of trampling over hardware
mappings.

> > 5. provide (and use) a function for soft rebooting which includes the
> >    setup of the MMU tables and call cpu_reset() - and remove the MMU
> >    setup from the code paths before arch_reset() is called.
> 
> This function will probably grow to contain the arm_machine_reset function
> body that I have for kexec.

arm_machine_restart() has become almost empty - I want to move the
IRQ disables out into machine_restart() and kill off arm_machine_restart()
entirely (at the moment its the 'default' action, which is to call the
old arch_reset() thing.)

Eventually, when we have everyone hooking into arm_pm_restart() instead,
arm_machine_restart() will be unused and meaningless to keep around.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [RFC 0/5] restart cleanups
  2011-11-01 19:41   ` Russell King - ARM Linux
@ 2011-11-01 21:19     ` Will Deacon
  2011-11-01 23:09       ` Russell King - ARM Linux
  0 siblings, 1 reply; 32+ messages in thread
From: Will Deacon @ 2011-11-01 21:19 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Nov 01, 2011 at 07:41:54PM +0000, Russell King - ARM Linux wrote:
> The other thing is that the generic code shouldn't care what value 'mode'
> actually is - we just pass it through to the platform handler (via
> arm_pm_restart()) and if the platform handler wants to ignore it and
> just do the software reboot thing, it uses soft_restart(addr) which
> does all the necessary setup for that.

Ah yes, you're right. I think it might be worth defining 'h' as REBOOT_MODE_HARD
and 's' as REBOOT_MODE_SOFT, but there's no reason to disallow other
combinations giving that the core code no longer has to do anything with
these.

> The nice thing about that is we no longer have to worry about calling
> arm_pm_restart() with 'h' and a platform doing a soft-reboot instead.

Yes, that's truly horrific. It's a miracle soft reboot worked at all from v6
onwards.

> > I'm definitely in favour of this change, but be aware that I plan to modify
> > setup_mm_for_reboot in the future to:
> > 
> >   (a) Use a static identity mapping so we don't have to pgd_alloc explicitly
> >       [I can't see this affecting anything]
> > 
> >   (b) Extend the identity mapping into kernel space, leaving some room for
> >       the kernel image. If this is all called from soft_reboot, which just
> >       does the cpu_reset(addr) afterwards, then we're fine. I have code to
> >       take care of changing stack and switching to the new mapping.
> 
> Well, bear in mind that (1) + (5) produce something really self-contained.
> We no longer fiddle with MMU tables for anything but the 'soft reboot'
> path.  That means things like hardware based reset (eg, using watchdogs,
> asserting GPIOs, etc) are all dealt with without fiddling with the MMU
> and caches.

Yup, so we should be able to add the scary MMU and cache stuff into
soft_reboot. We can even call this function from the kexec code.

> arm_machine_restart() has become almost empty - I want to move the
> IRQ disables out into machine_restart() and kill off arm_machine_restart()
> entirely (at the moment its the 'default' action, which is to call the
> old arch_reset() thing.)
> 
> Eventually, when we have everyone hooking into arm_pm_restart() instead,
> arm_machine_restart() will be unused and meaningless to keep around.

That's where our friend Coccinelle comes in at the end of my patch series.

Will

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 1/5] ARM: restart: remove argument to setup_mm_for_reboot()
  2011-11-01 16:08 ` [PATCH 1/5] ARM: restart: remove argument to setup_mm_for_reboot() Russell King - ARM Linux
@ 2011-11-01 22:27   ` Ryan Mallon
  2011-11-02 14:48     ` Russell King - ARM Linux
  0 siblings, 1 reply; 32+ messages in thread
From: Ryan Mallon @ 2011-11-01 22:27 UTC (permalink / raw)
  To: linux-arm-kernel

On 02/11/11 03:08, Russell King - ARM Linux wrote:

> setup_mm_for_reboot() doesn't make use of its argument, so remove it.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  arch/arm/kernel/machine_kexec.c |    4 ++--
>  arch/arm/kernel/process.c       |    4 ++--
>  arch/arm/mm/idmap.c             |    2 +-
>  arch/arm/mm/nommu.c             |    2 +-
>  4 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c
> index c1b4463..6fcdfc4 100644
> --- a/arch/arm/kernel/machine_kexec.c
> +++ b/arch/arm/kernel/machine_kexec.c
> @@ -16,7 +16,7 @@
>  extern const unsigned char relocate_new_kernel[];
>  extern const unsigned int relocate_new_kernel_size;
>  
> -extern void setup_mm_for_reboot(char mode);
> +extern void setup_mm_for_reboot(void);
>  
>  extern unsigned long kexec_start_address;
>  extern unsigned long kexec_indirection_page;
> @@ -114,7 +114,7 @@ void machine_kexec(struct kimage *image)
>  		kexec_reinit();
>  	local_irq_disable();
>  	local_fiq_disable();
> -	setup_mm_for_reboot(0); /* mode is not used, so just pass 0*/
> +	setup_mm_for_reboot(); /* mode is not used, so just pass 0*/


The comment should get removed now also.

~Ryan

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [RFC 0/5] restart cleanups
  2011-11-01 21:19     ` Will Deacon
@ 2011-11-01 23:09       ` Russell King - ARM Linux
  2011-11-02 21:33         ` Robert Jarzmik
  0 siblings, 1 reply; 32+ messages in thread
From: Russell King - ARM Linux @ 2011-11-01 23:09 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Nov 01, 2011 at 09:19:33PM +0000, Will Deacon wrote:
> Ah yes, you're right. I think it might be worth defining 'h' as
> REBOOT_MODE_HARD and 's' as REBOOT_MODE_SOFT, but there's no reason to
> disallow other combinations giving that the core code no longer has to
> do anything with these.

I don't see any need to define these to anything, especially as we just
take the first character of the reboot= argument.  If we did change it,
it would imply that we'd need to change how we parsed that argument as
well.

> > The nice thing about that is we no longer have to worry about calling
> > arm_pm_restart() with 'h' and a platform doing a soft-reboot instead.
> 
> Yes, that's truly horrific. It's a miracle soft reboot worked at all from v6
> onwards.

The short answer is it doesn't - and it's something that with this new
structure we can fix - we're no longer concerned by whatever the platform
code is doing between the soft restart setup code and actually executing
the soft restart itself.

Thankfully, at the point where we have v6 support, most platforms have
advanced to the point where there's (mostly) always a hardware method
to do a restart, which is much more preferable than trying to do a
soft-reboot (it ensures that all peripheral devices are placed into a
known state, whereas a soft restart doesn't have that guarantee.)

> > arm_machine_restart() has become almost empty - I want to move the
> > IRQ disables out into machine_restart() and kill off arm_machine_restart()
> > entirely (at the moment its the 'default' action, which is to call the
> > old arch_reset() thing.)
> > 
> > Eventually, when we have everyone hooking into arm_pm_restart() instead,
> > arm_machine_restart() will be unused and meaningless to keep around.
> 
> That's where our friend Coccinelle comes in at the end of my patch series.

We do have this problem - mioa701 - which insists on doing things like
kfree, free_irq, gpio_free in this path, which is a bad idea as it can
be called from many different contexts including IRQ context.  That
will appear to work for '/sbin/reboot' but not for things like softdog
or a sysrq-b.  Arguably, mioa701 is broken in this regard.

However, we don't know whether removing the code will then prevent it
rebooting even for a userspace-invoked reboot.  We need feedback from
the platform maintainer for that (added Philipp and Robert in the
hope one of them can answer this - and maybe send a patch to fix it.)

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 5/5] ARM: restart: only perform setup for restart when soft-restarting
  2011-11-01 16:09 ` [PATCH 5/5] ARM: restart: only perform setup for restart when soft-restarting Russell King - ARM Linux
@ 2011-11-02 12:12   ` Will Deacon
  2011-11-02 12:37     ` Russell King - ARM Linux
  0 siblings, 1 reply; 32+ messages in thread
From: Will Deacon @ 2011-11-02 12:12 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Russell,

On Tue, Nov 01, 2011 at 04:09:49PM +0000, Russell King - ARM Linux wrote:
> We only need to set the system up for a soft-restart if we're going to
> be doing a soft-restart.  Provide a new function (soft_restart()) which
> does the setup and final call for this, and make platforms use it.
> Eliminate the call to setup_restart() from the default handler.
> 
> This means that platforms arch_reset() function is no longer called with
> the page tables prepared for a soft-restart, and caches will still be
> enabled.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  arch/arm/include/asm/system.h                     |    1 +
>  arch/arm/kernel/process.c                         |   10 +++++++---
>  arch/arm/mach-clps711x/include/mach/system.h      |    2 +-
>  arch/arm/mach-ebsa110/include/mach/system.h       |    2 +-
>  arch/arm/mach-footbridge/include/mach/system.h    |    2 +-
>  arch/arm/mach-iop32x/include/mach/system.h        |    2 +-
>  arch/arm/mach-iop33x/include/mach/system.h        |    2 +-
>  arch/arm/mach-ixp4xx/include/mach/system.h        |    2 +-
>  arch/arm/mach-ks8695/include/mach/system.h        |    2 +-
>  arch/arm/mach-mmp/include/mach/system.h           |    4 ++--
>  arch/arm/mach-mxs/system.c                        |    2 +-
>  arch/arm/mach-nuc93x/include/mach/system.h        |    2 +-
>  arch/arm/mach-pnx4008/include/mach/system.h       |    2 +-
>  arch/arm/mach-pxa/reset.c                         |    2 +-
>  arch/arm/mach-rpc/include/mach/system.h           |    2 +-
>  arch/arm/mach-s3c2410/include/mach/system-reset.h |    4 ++--
>  arch/arm/mach-s3c64xx/include/mach/system.h       |    2 +-
>  arch/arm/mach-sa1100/include/mach/system.h        |    2 +-
>  arch/arm/mach-shmobile/include/mach/system.h      |    2 +-
>  arch/arm/mach-w90x900/include/mach/system.h       |    2 +-
>  arch/arm/plat-mxc/system.c                        |    2 +-
>  arch/arm/plat-spear/include/plat/system.h         |    2 +-
>  22 files changed, 30 insertions(+), 25 deletions(-)

Which branch is this patch taken against? I'm getting conflicts if I try to
apply it against mainline, next, 3.1 or your devel branch.

Thanks,

Will

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 5/5] ARM: restart: only perform setup for restart when soft-restarting
  2011-11-02 12:12   ` Will Deacon
@ 2011-11-02 12:37     ` Russell King - ARM Linux
  2011-11-02 13:11       ` Will Deacon
  0 siblings, 1 reply; 32+ messages in thread
From: Russell King - ARM Linux @ 2011-11-02 12:37 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Nov 02, 2011 at 12:12:00PM +0000, Will Deacon wrote:
> Hi Russell,
> 
> On Tue, Nov 01, 2011 at 04:09:49PM +0000, Russell King - ARM Linux wrote:
> > We only need to set the system up for a soft-restart if we're going to
> > be doing a soft-restart.  Provide a new function (soft_restart()) which
> > does the setup and final call for this, and make platforms use it.
> > Eliminate the call to setup_restart() from the default handler.
> > 
> > This means that platforms arch_reset() function is no longer called with
> > the page tables prepared for a soft-restart, and caches will still be
> > enabled.
> > 
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > ---
> >  arch/arm/include/asm/system.h                     |    1 +
> >  arch/arm/kernel/process.c                         |   10 +++++++---
> >  arch/arm/mach-clps711x/include/mach/system.h      |    2 +-
> >  arch/arm/mach-ebsa110/include/mach/system.h       |    2 +-
> >  arch/arm/mach-footbridge/include/mach/system.h    |    2 +-
> >  arch/arm/mach-iop32x/include/mach/system.h        |    2 +-
> >  arch/arm/mach-iop33x/include/mach/system.h        |    2 +-
> >  arch/arm/mach-ixp4xx/include/mach/system.h        |    2 +-
> >  arch/arm/mach-ks8695/include/mach/system.h        |    2 +-
> >  arch/arm/mach-mmp/include/mach/system.h           |    4 ++--
> >  arch/arm/mach-mxs/system.c                        |    2 +-
> >  arch/arm/mach-nuc93x/include/mach/system.h        |    2 +-
> >  arch/arm/mach-pnx4008/include/mach/system.h       |    2 +-
> >  arch/arm/mach-pxa/reset.c                         |    2 +-
> >  arch/arm/mach-rpc/include/mach/system.h           |    2 +-
> >  arch/arm/mach-s3c2410/include/mach/system-reset.h |    4 ++--
> >  arch/arm/mach-s3c64xx/include/mach/system.h       |    2 +-
> >  arch/arm/mach-sa1100/include/mach/system.h        |    2 +-
> >  arch/arm/mach-shmobile/include/mach/system.h      |    2 +-
> >  arch/arm/mach-w90x900/include/mach/system.h       |    2 +-
> >  arch/arm/plat-mxc/system.c                        |    2 +-
> >  arch/arm/plat-spear/include/plat/system.h         |    2 +-
> >  22 files changed, 30 insertions(+), 25 deletions(-)
> 
> Which branch is this patch taken against? I'm getting conflicts if I try to
> apply it against mainline, next, 3.1 or your devel branch.

It's at some random point during the merge window at the moment.

It's probably going to be a good idea to wait until after the merge window
closes before tackling this stuff seriously, because things are still
changing quite quickly - eg, we have new platforms merged last night which
wouldn't have been updated with these patches.

I'd suggest not rushing this; we'll have a full 2-3 months before they
can be merged into mainline.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 5/5] ARM: restart: only perform setup for restart when soft-restarting
  2011-11-02 12:37     ` Russell King - ARM Linux
@ 2011-11-02 13:11       ` Will Deacon
  0 siblings, 0 replies; 32+ messages in thread
From: Will Deacon @ 2011-11-02 13:11 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Nov 02, 2011 at 12:37:30PM +0000, Russell King - ARM Linux wrote:
> It's probably going to be a good idea to wait until after the merge window
> closes before tackling this stuff seriously, because things are still
> changing quite quickly - eg, we have new platforms merged last night which
> wouldn't have been updated with these patches.
> 
> I'd suggest not rushing this; we'll have a full 2-3 months before they
> can be merged into mainline.

Sure, suits me - I have plenty of things to be getting on with.

Will

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [RFC 0/5] restart cleanups
  2011-11-01 16:07 [RFC 0/5] restart cleanups Russell King - ARM Linux
                   ` (7 preceding siblings ...)
  2011-11-01 19:23 ` Will Deacon
@ 2011-11-02 13:35 ` Nicolas Pitre
  8 siblings, 0 replies; 32+ messages in thread
From: Nicolas Pitre @ 2011-11-02 13:35 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, 1 Nov 2011, Russell King - ARM Linux wrote:

> This series cleans up some of the restart code for ARM.  It doesn't go
> as far as Will's patch set (or even anywhere close to it) mainly because
> of the poor state of the various implementations in various platforms.
[...]

For the 5 patches:

Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org>


Nicolas

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [RFC 0/5] restart cleanups
  2011-11-01 17:02 ` Russell King - ARM Linux
@ 2011-11-02 13:49   ` Richard Purdie
  2011-11-02 14:49     ` Russell King - ARM Linux
  0 siblings, 1 reply; 32+ messages in thread
From: Richard Purdie @ 2011-11-02 13:49 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, 2011-11-01 at 17:02 +0000, Russell King - ARM Linux wrote:
> (Poodle doesn't have any MAINTAINERS entry, I think Richard Purdie last
> touched Poodle in a 'maintainery' way back in 2006.)
> 
> poodle does this:
> 
> static void poodle_restart(char mode, const char *cmd)
> {
>         arm_machine_restart('h', cmd);
> }
> 
> Given that the default value of 'mode' (in the absense of any reboot=
> command line argument to the kernel) is 'h', this seems to be a no-op.
> Below is the history; I suspect that the reason this first appeared was
> because of the need to fiddle with RCSR - that's now gone so I suspect
> the entire restart handler can be deleted.
> 
> What I don't understand in that case is why 'mode' wasn't simply passed
> through in the first patch.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 1/5] ARM: restart: remove argument to setup_mm_for_reboot()
  2011-11-01 22:27   ` Ryan Mallon
@ 2011-11-02 14:48     ` Russell King - ARM Linux
  0 siblings, 0 replies; 32+ messages in thread
From: Russell King - ARM Linux @ 2011-11-02 14:48 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Nov 02, 2011 at 09:27:03AM +1100, Ryan Mallon wrote:
> The comment should get removed now also.

Thanks, it'll be gone for the next posting of these patches.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [RFC 0/5] restart cleanups
  2011-11-02 13:49   ` Richard Purdie
@ 2011-11-02 14:49     ` Russell King - ARM Linux
  2011-11-04 13:58       ` Andrea Adami
  0 siblings, 1 reply; 32+ messages in thread
From: Russell King - ARM Linux @ 2011-11-02 14:49 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Nov 02, 2011 at 01:49:07PM +0000, Richard Purdie wrote:
> >From what I remember that hardware either always reboots or always
> halts. I think the option was therefore left hardcoded to make it clear
> it wasn't expected to work. Later Zaurii models could do either but
> required some manual poking of registers to make it happen iirc.
> 
> Regardless, you can probably clean this up as you suggest now.

Thanks, I've now committed a change which removes the indirection
entirely from poodle.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [RFC 0/5] restart cleanups
  2011-11-01 23:09       ` Russell King - ARM Linux
@ 2011-11-02 21:33         ` Robert Jarzmik
  2011-11-02 23:31           ` Russell King - ARM Linux
  0 siblings, 1 reply; 32+ messages in thread
From: Robert Jarzmik @ 2011-11-02 21:33 UTC (permalink / raw)
  To: linux-arm-kernel

Russell King - ARM Linux <linux@arm.linux.org.uk> writes:
> However, we don't know whether removing the code will then prevent it
> rebooting even for a userspace-invoked reboot.  We need feedback from
> the platform maintainer for that (added Philipp and Robert in the
> hope one of them can answer this - and maybe send a patch to fix it.)

OK, I tested the serie on mioa701 board, and everything works as before. I
tested it with mioa701.c amended so that no more halt/reboot hooks are used
anymore. The resources freeing is gone as well.

I will submit the following patch [1] to get rid of the hooks to Eric, unless
you want it in your tree.

Cheers.

-- 
Robert

[1]

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [RFC 0/5] restart cleanups
  2011-11-02 21:33         ` Robert Jarzmik
@ 2011-11-02 23:31           ` Russell King - ARM Linux
  0 siblings, 0 replies; 32+ messages in thread
From: Russell King - ARM Linux @ 2011-11-02 23:31 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Nov 02, 2011 at 10:33:00PM +0100, Robert Jarzmik wrote:
> Russell King - ARM Linux <linux@arm.linux.org.uk> writes:
> > However, we don't know whether removing the code will then prevent it
> > rebooting even for a userspace-invoked reboot.  We need feedback from
> > the platform maintainer for that (added Philipp and Robert in the
> > hope one of them can answer this - and maybe send a patch to fix it.)
> 
> OK, I tested the serie on mioa701 board, and everything works as before. I
> tested it with mioa701.c amended so that no more halt/reboot hooks are used
> anymore. The resources freeing is gone as well.
> 
> I will submit the following patch [1] to get rid of the hooks to Eric, unless
> you want it in your tree.

I think I should pick it up because I have a patch changing the
.soft_reboot stuff.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [RFC 0/5] restart cleanups
  2011-11-01 16:38 ` [RFC 0/5] restart cleanups Russell King - ARM Linux
@ 2011-11-03  9:12   ` Russell King - ARM Linux
  2011-11-03  9:24     ` Kukjin Kim
  0 siblings, 1 reply; 32+ messages in thread
From: Russell King - ARM Linux @ 2011-11-03  9:12 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Nov 01, 2011 at 04:38:17PM +0000, Russell King - ARM Linux wrote:
> Can someone please check whether this code:
> 
> /* Hook for arm_pm_restart to ensure we execute the reset code
>  * with the caches enabled. It seems at least the S3C2440 has a problem
>  * resetting if there is bus activity interrupted by the reset.
>  */
> static void s3c24xx_pm_restart(char mode, const char *cmd)
> {
>         if (mode != 's') {
>                 unsigned long flags;
> 
>                 local_irq_save(flags);
>                 __cpuc_flush_kern_all();
>                 __cpuc_flush_user_all();
> 
>                 arch_reset(mode, cmd);
>                 local_irq_restore(flags);
>         }
> 
>         /* fallback, or unhandled */
>         arm_machine_restart(mode, cmd);
> }
> 
> will be broken by any of these patches - and more importantly whether
> this code can simply be deleted with these five patches applied.

Okay, I'm going to delete the above and hope for the best; I'm taking
the lack of reply as meaning that no one in the Samsung community is
that interested in this code.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [RFC 0/5] restart cleanups
  2011-11-03  9:12   ` Russell King - ARM Linux
@ 2011-11-03  9:24     ` Kukjin Kim
  2011-11-03  9:57       ` Russell King - ARM Linux
  0 siblings, 1 reply; 32+ messages in thread
From: Kukjin Kim @ 2011-11-03  9:24 UTC (permalink / raw)
  To: linux-arm-kernel

Russell King - ARM Linux wrote:
> 
> On Tue, Nov 01, 2011 at 04:38:17PM +0000, Russell King - ARM Linux wrote:
> > Can someone please check whether this code:
> >
> > /* Hook for arm_pm_restart to ensure we execute the reset code
> >  * with the caches enabled. It seems at least the S3C2440 has a problem
> >  * resetting if there is bus activity interrupted by the reset.
> >  */
> > static void s3c24xx_pm_restart(char mode, const char *cmd)
> > {
> >         if (mode != 's') {
> >                 unsigned long flags;
> >
> >                 local_irq_save(flags);
> >                 __cpuc_flush_kern_all();
> >                 __cpuc_flush_user_all();
> >
> >                 arch_reset(mode, cmd);
> >                 local_irq_restore(flags);
> >         }
> >
> >         /* fallback, or unhandled */
> >         arm_machine_restart(mode, cmd);
> > }
> >
> > will be broken by any of these patches - and more importantly whether
> > this code can simply be deleted with these five patches applied.
> 
> Okay, I'm going to delete the above and hope for the best; I'm taking
> the lack of reply as meaning that no one in the Samsung community is
> that interested in this code.

Russell,
Sorry for late response.

According to its commit message, it is needed...

[ARM] 4987/1: S3C24XX: Ensure watchdog reset initiated from cached code.

    There seems to be some problem with at-least the S3C2440 and
    bus traffic during an reset. It is unlikely, but still possible
    that the system will hang in such a way that the watchdog cannot
    get the system out of the state it is in.

    Change to making the code that calls the watchdog reset run from
    cached memory so that instruction fetches have quiesced before the
    watchdog fires.

But frankly, I'm not sure now and there is no S3C2440 on my desk :(
Let me check again, if no response on this, please keep going on.

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [RFC 0/5] restart cleanups
  2011-11-03  9:24     ` Kukjin Kim
@ 2011-11-03  9:57       ` Russell King - ARM Linux
  0 siblings, 0 replies; 32+ messages in thread
From: Russell King - ARM Linux @ 2011-11-03  9:57 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Nov 03, 2011 at 06:24:30PM +0900, Kukjin Kim wrote:
> Russell King - ARM Linux wrote:
> > 
> > On Tue, Nov 01, 2011 at 04:38:17PM +0000, Russell King - ARM Linux wrote:
> > > Can someone please check whether this code:
> > >
> > > /* Hook for arm_pm_restart to ensure we execute the reset code
> > >  * with the caches enabled. It seems at least the S3C2440 has a problem
> > >  * resetting if there is bus activity interrupted by the reset.
> > >  */
> > > static void s3c24xx_pm_restart(char mode, const char *cmd)
> > > {
> > >         if (mode != 's') {
> > >                 unsigned long flags;
> > >
> > >                 local_irq_save(flags);
> > >                 __cpuc_flush_kern_all();
> > >                 __cpuc_flush_user_all();
> > >
> > >                 arch_reset(mode, cmd);
> > >                 local_irq_restore(flags);
> > >         }
> > >
> > >         /* fallback, or unhandled */
> > >         arm_machine_restart(mode, cmd);
> > > }
> > >
> > > will be broken by any of these patches - and more importantly whether
> > > this code can simply be deleted with these five patches applied.
> > 
> > Okay, I'm going to delete the above and hope for the best; I'm taking
> > the lack of reply as meaning that no one in the Samsung community is
> > that interested in this code.
> 
> Russell,
> Sorry for late response.
> 
> According to its commit message, it is needed...
> 
> [ARM] 4987/1: S3C24XX: Ensure watchdog reset initiated from cached code.
> 
>     There seems to be some problem with at-least the S3C2440 and
>     bus traffic during an reset. It is unlikely, but still possible
>     that the system will hang in such a way that the watchdog cannot
>     get the system out of the state it is in.
> 
>     Change to making the code that calls the watchdog reset run from
>     cached memory so that instruction fetches have quiesced before the
>     watchdog fires.
> 
> But frankly, I'm not sure now and there is no S3C2440 on my desk :(
> Let me check again, if no response on this, please keep going on.

And now consider it along side the set of patches that I posted as
part of this thread, which have the effect that arch_reset() will
now be called with caches enabled.

Given that, is it still required?

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [RFC 0/5] restart cleanups
  2011-11-02 14:49     ` Russell King - ARM Linux
@ 2011-11-04 13:58       ` Andrea Adami
  2011-11-04 14:01         ` Russell King - ARM Linux
  0 siblings, 1 reply; 32+ messages in thread
From: Andrea Adami @ 2011-11-04 13:58 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Nov 2, 2011 at 3:49 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
>
> On Wed, Nov 02, 2011 at 01:49:07PM +0000, Richard Purdie wrote:
> > >From what I remember that hardware either always reboots or always
> > halts. I think the option was therefore left hardcoded to make it clear
> > it wasn't expected to work. Later Zaurii models could do either but
> > required some manual poking of registers to make it happen iirc.
> >
> > Regardless, you can probably clean this up as you suggest now.
>
> Thanks, I've now committed a change which removes the indirection
> entirely from poodle.
>
Hi, sorry for the late reply

I'm playing with poodle and I can confirm that poweroff does indeed
not work with 2.6.3x kernels.
See this thread:
http://lists.linuxtogo.org/pipermail/zaurus-devel/2011-May/000501.html

I'll be happy to test again and report

Regards

Andrea



> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [RFC 0/5] restart cleanups
  2011-11-04 13:58       ` Andrea Adami
@ 2011-11-04 14:01         ` Russell King - ARM Linux
  2011-11-04 14:22           ` Andrea Adami
  0 siblings, 1 reply; 32+ messages in thread
From: Russell King - ARM Linux @ 2011-11-04 14:01 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Nov 04, 2011 at 02:58:02PM +0100, Andrea Adami wrote:
> On Wed, Nov 2, 2011 at 3:49 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> >
> > On Wed, Nov 02, 2011 at 01:49:07PM +0000, Richard Purdie wrote:
> > > >From what I remember that hardware either always reboots or always
> > > halts. I think the option was therefore left hardcoded to make it clear
> > > it wasn't expected to work. Later Zaurii models could do either but
> > > required some manual poking of registers to make it happen iirc.
> > >
> > > Regardless, you can probably clean this up as you suggest now.
> >
> > Thanks, I've now committed a change which removes the indirection
> > entirely from poodle.
> >
> Hi, sorry for the late reply
> 
> I'm playing with poodle and I can confirm that poweroff does indeed
> not work with 2.6.3x kernels.
> See this thread:
> http://lists.linuxtogo.org/pipermail/zaurus-devel/2011-May/000501.html

Okay, at least we know that restart works with that.

I'm not offering to try and change the poweroff behaviour for the better,
as I know absolutely nothing about the Zaurus platforms.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [RFC 0/5] restart cleanups
  2011-11-04 14:01         ` Russell King - ARM Linux
@ 2011-11-04 14:22           ` Andrea Adami
  0 siblings, 0 replies; 32+ messages in thread
From: Andrea Adami @ 2011-11-04 14:22 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Nov 4, 2011 at 3:01 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Nov 04, 2011 at 02:58:02PM +0100, Andrea Adami wrote:
>> On Wed, Nov 2, 2011 at 3:49 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> >
>> > On Wed, Nov 02, 2011 at 01:49:07PM +0000, Richard Purdie wrote:
>> > > >From what I remember that hardware either always reboots or always
>> > > halts. I think the option was therefore left hardcoded to make it clear
>> > > it wasn't expected to work. Later Zaurii models could do either but
>> > > required some manual poking of registers to make it happen iirc.
>> > >
>> > > Regardless, you can probably clean this up as you suggest now.
>> >
>> > Thanks, I've now committed a change which removes the indirection
>> > entirely from poodle.
>> >
>> Hi, sorry for the late reply
>>
>> I'm playing with poodle and I can confirm that poweroff does indeed
>> not work with 2.6.3x kernels.
>> See this thread:
>> http://lists.linuxtogo.org/pipermail/zaurus-devel/2011-May/000501.html
>
> Okay, at least we know that restart works with that.
Yes, indeed

>
> I'm not offering to try and change the poweroff behaviour for the better,
> as I know absolutely nothing about the Zaurus platforms.
>
I see, np. I'm just puzzled because after a cold reset (pulling
battery out) poodle is acting as if powered off and does not restart
itself.
Apparently we cannot reach this state again after boot.

FYI the modern zaurus (corgi and spitz) do
suspend/resume/poweroff/restart flawlessy so it's only this machine
failing poweroff (and maybe collie, no idea).

Regards

Andrea

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [RFC 0/5] restart cleanups
  2011-11-02  7:46 ` Robert Jarzmik
@ 2011-11-02 10:30   ` Russell King - ARM Linux
  0 siblings, 0 replies; 32+ messages in thread
From: Russell King - ARM Linux @ 2011-11-02 10:30 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Nov 02, 2011 at 08:46:06AM +0100, Robert Jarzmik wrote:
> ----- Mail Original -----
> De: "Russell King - ARM Linux" <linux@arm.linux.org.uk>
> > We do have this problem - mioa701 - which insists on doing things like
> > kfree, free_irq, gpio_free in this path, which is a bad idea as it can
> True. You mean gsm_exit() and bootstrap_exit() I think.
> These were added to free resources in a shutdown. If freeing is pointless,
> in a reboot case, and stale resources don't matter, well, we can kill these
> 2 functions.

Provided you're talking about software state only, then it is pointless
because the state of the system software will be lost on reboot anyway.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [RFC 0/5] restart cleanups
       [not found] <1676495226.4381981320219764753.JavaMail.root@zimbra20-e3.priv.proxad.net>
@ 2011-11-02  7:46 ` Robert Jarzmik
  2011-11-02 10:30   ` Russell King - ARM Linux
  0 siblings, 1 reply; 32+ messages in thread
From: Robert Jarzmik @ 2011-11-02  7:46 UTC (permalink / raw)
  To: linux-arm-kernel

----- Mail Original -----
De: "Russell King - ARM Linux" <linux@arm.linux.org.uk>
> We do have this problem - mioa701 - which insists on doing things like
> kfree, free_irq, gpio_free in this path, which is a bad idea as it can
True. You mean gsm_exit() and bootstrap_exit() I think.
These were added to free resources in a shutdown. If freeing is pointless,
in a reboot case, and stale resources don't matter, well, we can kill these
2 functions.

> However, we don't know whether removing the code will then prevent it
> rebooting even for a userspace-invoked reboot.  We need feedback from
> the platform maintainer for that (added Philipp and Robert in the
> hope one of them can answer this - and maybe send a patch to fix it.)
I'll try to test the patch serie this evening after my daywork,
and give feedback.

The main facts of mioa701 board regarding halting/rebooting:
 - watchdog reset kills RTC time
 - there is no GPIO reset (which would preserve time, sic ...)
 - mioa701 is based on PXA272 (arm v5)

Cheers.

--
Robert

^ permalink raw reply	[flat|nested] 32+ messages in thread

end of thread, other threads:[~2011-11-04 14:22 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-11-01 16:07 [RFC 0/5] restart cleanups Russell King - ARM Linux
2011-11-01 16:08 ` [PATCH 1/5] ARM: restart: remove argument to setup_mm_for_reboot() Russell King - ARM Linux
2011-11-01 22:27   ` Ryan Mallon
2011-11-02 14:48     ` Russell King - ARM Linux
2011-11-01 16:08 ` [PATCH 2/5] ARM: restart: get rid of needless at91_arch_reset additional indirection Russell King - ARM Linux
2011-11-01 16:09 ` [PATCH 3/5] ARM: restart: remove local_irq_disable() from within arch_reset() Russell King - ARM Linux
2011-11-01 16:36   ` H Hartley Sweeten
2011-11-01 17:56     ` Russell King - ARM Linux
2011-11-01 16:09 ` [PATCH 4/5] ARM: restart: turn reboot setup code into a library function Russell King - ARM Linux
2011-11-01 16:09 ` [PATCH 5/5] ARM: restart: only perform setup for restart when soft-restarting Russell King - ARM Linux
2011-11-02 12:12   ` Will Deacon
2011-11-02 12:37     ` Russell King - ARM Linux
2011-11-02 13:11       ` Will Deacon
2011-11-01 16:38 ` [RFC 0/5] restart cleanups Russell King - ARM Linux
2011-11-03  9:12   ` Russell King - ARM Linux
2011-11-03  9:24     ` Kukjin Kim
2011-11-03  9:57       ` Russell King - ARM Linux
2011-11-01 17:02 ` Russell King - ARM Linux
2011-11-02 13:49   ` Richard Purdie
2011-11-02 14:49     ` Russell King - ARM Linux
2011-11-04 13:58       ` Andrea Adami
2011-11-04 14:01         ` Russell King - ARM Linux
2011-11-04 14:22           ` Andrea Adami
2011-11-01 19:23 ` Will Deacon
2011-11-01 19:41   ` Russell King - ARM Linux
2011-11-01 21:19     ` Will Deacon
2011-11-01 23:09       ` Russell King - ARM Linux
2011-11-02 21:33         ` Robert Jarzmik
2011-11-02 23:31           ` Russell King - ARM Linux
2011-11-02 13:35 ` Nicolas Pitre
     [not found] <1676495226.4381981320219764753.JavaMail.root@zimbra20-e3.priv.proxad.net>
2011-11-02  7:46 ` Robert Jarzmik
2011-11-02 10:30   ` Russell King - ARM Linux

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.