[PATCH 00/20] powerpc: Convert power off logic to pm_power_off

[PATCH 00/20] powerpc: Convert power off logic to pm_power_off

[Alexander Graf]

The generic Linux framework to power off the machine is a function pointer
called pm_power_off. The trick about this pointer is that device drivers can
potentially implement it rather than board files.

Today on PowerPC we set pm_power_off to invoke our generic full machine power
off logic which then calls ppc_md.power_off to invoke machine specific power
off.

However, when we want to add a power off GPIO via the "gpio-poweroff" driver,
this card house falls apart. That driver only registers itself if pm_power_off
is NULL to ensure it doesn't override board specific logic. However, since we
always set pm_power_off to the generic power off logic (which will just not
power off the machine if no ppc_md.power_off call is implemented), we can't
implement power off via the generic GPIO power off driver.

To fix this up, let's get rid of the ppc_md.power_off logic and just always use
pm_power_off as was intended. Then individual drivers such as the GPIO power off
driver can implement power off logic via that function pointer.

With this patch set applied and a few patches on top of QEMU that implement a
power off GPIO on the virt e500 machine, I can successfully turn off my virtual
machine after halt.


Alex

Alexander Graf (20):
  powerpc: Support override of pm_power_off
  powerpc/xmon: Support either ppc_md.power_off or pm_power_off
  powerpc/47x: Use pm_power_off rather than ppc_md.power_off
  powerpc/52xx/efika: Use pm_power_off rather than ppc_md.power_off
  powerpc/mpc8349emitx: Use pm_power_off rather than ppc_md.power_off
  powerpc/corenet: Use pm_power_off rather than ppc_md.power_off
  powerpc/85xx/sgy_cts1000: Use pm_power_off rather than
    ppc_md.power_off
  powerpc/celleb: Use pm_power_off rather than ppc_md.power_off
  powerpc/cell/qpace: Use pm_power_off rather than ppc_md.power_off
  powerpc/cell: Use pm_power_off rather than ppc_md.power_off
  powerpc/chrp: Use pm_power_off rather than ppc_md.power_off
  powerpc/6xx/gamecube: Use pm_power_off rather than ppc_md.power_off
  powerpc/6xx/linkstation: Use pm_power_off rather than ppc_md.power_off
  powerpc/6xx/wii: Use pm_power_off rather than ppc_md.power_off
  powerpc/maple: Use pm_power_off rather than ppc_md.power_off
  powerpc/powermac: Use pm_power_off rather than ppc_md.power_off
  powerpc/powernv: Use pm_power_off rather than ppc_md.power_off
  powerpc/ps3: Use pm_power_off rather than ppc_md.power_off
  powerpc/pseries: Use pm_power_off rather than ppc_md.power_off
  powerpc: Remove ppc_md.power_off

 arch/powerpc/include/asm/machdep.h               |   1 -
 arch/powerpc/kernel/setup-common.c               |   6 +-
 arch/powerpc/platforms/44x/ppc476.c              |   2 +-
 arch/powerpc/platforms/52xx/efika.c              |   3 +-
 arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c   |   8 +-
 arch/powerpc/platforms/85xx/corenet_generic.c    |   2 +-
 arch/powerpc/platforms/85xx/sgy_cts1000.c        |   4 +-
 arch/powerpc/platforms/cell/celleb_setup.c       |   4 +-
 arch/powerpc/platforms/cell/qpace_setup.c        |   3 +-
 arch/powerpc/platforms/cell/setup.c              |   2 +-
 arch/powerpc/platforms/chrp/setup.c              |   3 +-
 arch/powerpc/platforms/embedded6xx/gamecube.c    |   3 +-
 arch/powerpc/platforms/embedded6xx/linkstation.c |   3 +-
 arch/powerpc/platforms/embedded6xx/wii.c         |   3 +-
 arch/powerpc/platforms/maple/setup.c             |   4 +-
 arch/powerpc/platforms/powermac/setup.c          | 147 ++++++++++++-----------
 arch/powerpc/platforms/powernv/setup.c           |   4 +-
 arch/powerpc/platforms/ps3/setup.c               |   3 +-
 arch/powerpc/platforms/pseries/setup.c           |  59 ++++-----
 arch/powerpc/sysdev/fsl_soc.c                    |   2 +-
 arch/powerpc/xmon/xmon.c                         |   3 +-
 21 files changed, 139 insertions(+), 130 deletions(-)

-- 
1.8.1.4

[PATCH 01/20] powerpc: Support override of pm_power_off

[Alexander Graf]

The pm_power_off callback is what drivers are supposed to modify when they
implement power off support for the system.

Support a modified callback on powerpc. That way power off support code can
now either override ppc_md.power_off or pm_power_off.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/kernel/setup-common.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
index 1b0e260..5dfcb28 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -140,6 +140,8 @@ void machine_power_off(void)
 	machine_shutdown();
 	if (ppc_md.power_off)
 		ppc_md.power_off();
+	if (pm_power_off != machine_power_off)
+		pm_power_off();
 #ifdef CONFIG_SMP
 	smp_send_stop();
 #endif
-- 
1.8.1.4

[PATCH 02/20] powerpc/xmon: Support either ppc_md.power_off or pm_power_off

[Alexander Graf]

Xmon can manually turn off the machine. We now have 2 code paths for this:

  1) ppc_md.power_off
  2) pm_power_off

This patch allows xmon to support both and makes sure it graciously allows
a path to not be implemented.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/xmon/xmon.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index b988b5a..531f649 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -981,7 +981,10 @@ static void bootcmds(void)
 	else if (cmd == 'h')
 		ppc_md.halt();
 	else if (cmd == 'p')
-		ppc_md.power_off();
+		if (ppc_md.power_off)
+			ppc_md.power_off();
+		if (pm_power_off)
+			pm_power_off();
 }
 
 static int cpu_cmd(void)
-- 
1.8.1.4

[PATCH 03/20] powerpc/47x: Use pm_power_off rather than ppc_md.power_off

[Alexander Graf]

The generic power off callback is pm_power_off. Use that one rather than
the powerpc specific ppc_md.power_off.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/platforms/44x/ppc476.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/44x/ppc476.c b/arch/powerpc/platforms/44x/ppc476.c
index 33986c1..7027015 100644
--- a/arch/powerpc/platforms/44x/ppc476.c
+++ b/arch/powerpc/platforms/44x/ppc476.c
@@ -94,7 +94,7 @@ static int avr_probe(struct i2c_client *client,
 {
 	avr_i2c_client = client;
 	ppc_md.restart = avr_reset_system;
-	ppc_md.power_off = avr_power_off_system;
+	pm_power_off = avr_power_off_system
 	return 0;
 }
 
-- 
1.8.1.4

Re: [PATCH 03/20] powerpc/47x: Use pm_power_off rather than ppc_md.power_off

[Segher Boessenkool]

On Wed, Oct 01, 2014 at 03:27:49PM +0200, Alexander Graf wrote:
> diff --git a/arch/powerpc/platforms/44x/ppc476.c b/arch/powerpc/platforms/44x/ppc476.c
> index 33986c1..7027015 100644
> --- a/arch/powerpc/platforms/44x/ppc476.c
> +++ b/arch/powerpc/platforms/44x/ppc476.c
> @@ -94,7 +94,7 @@ static int avr_probe(struct i2c_client *client,
>  {
>  	avr_i2c_client = client;
>  	ppc_md.restart = avr_reset_system;
> -	ppc_md.power_off = avr_power_off_system;
> +	pm_power_off = avr_power_off_system
>  	return 0;
>  }

That doesn't compile afaics?


Segher

Re: [PATCH 03/20] powerpc/47x: Use pm_power_off rather than ppc_md.power_off

[Alexander Graf]

On 05.10.14 02:39, Segher Boessenkool wrote:
> On Wed, Oct 01, 2014 at 03:27:49PM +0200, Alexander Graf wrote:
>> diff --git a/arch/powerpc/platforms/44x/ppc476.c b/arch/powerpc/platforms/44x/ppc476.c
>> index 33986c1..7027015 100644
>> --- a/arch/powerpc/platforms/44x/ppc476.c
>> +++ b/arch/powerpc/platforms/44x/ppc476.c
>> @@ -94,7 +94,7 @@ static int avr_probe(struct i2c_client *client,
>>  {
>>  	avr_i2c_client = client;
>>  	ppc_md.restart = avr_reset_system;
>> -	ppc_md.power_off = avr_power_off_system;
>> +	pm_power_off = avr_power_off_system
>>  	return 0;
>>  }
> 
> That doesn't compile afaics?

Oops :(. Is there any easy way to test compile all targets?


Alex

[PATCH 04/20] powerpc/52xx/efika: Use pm_power_off rather than ppc_md.power_off

[Alexander Graf]

The generic power off callback is pm_power_off. Use that one rather than
the powerpc specific ppc_md.power_off.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/platforms/52xx/efika.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/52xx/efika.c b/arch/powerpc/platforms/52xx/efika.c
index 3feffde..485a470 100644
--- a/arch/powerpc/platforms/52xx/efika.c
+++ b/arch/powerpc/platforms/52xx/efika.c
@@ -194,6 +194,8 @@ static void __init efika_setup_arch(void)
 	mpc52xx_pm_init();
 #endif
 
+	pm_power_off = rtas_power_off;
+
 	if (ppc_md.progress)
 		ppc_md.progress("Linux/PPC " UTS_RELEASE " running on Efika ;-)\n", 0x0);
 }
@@ -225,7 +227,6 @@ define_machine(efika)
 	.init_IRQ		= mpc52xx_init_irq,
 	.get_irq		= mpc52xx_get_irq,
 	.restart		= rtas_restart,
-	.power_off		= rtas_power_off,
 	.halt			= rtas_halt,
 	.set_rtc_time		= rtas_set_rtc_time,
 	.get_rtc_time		= rtas_get_rtc_time,
-- 
1.8.1.4

[PATCH 05/20] powerpc/mpc8349emitx: Use pm_power_off rather than ppc_md.power_off

[Alexander Graf]

The generic power off callback is pm_power_off. Use that one rather than
the powerpc specific ppc_md.power_off.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c b/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c
index e238b6a..a3b7a1f 100644
--- a/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c
+++ b/arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c
@@ -166,10 +166,10 @@ static int mcu_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	if (ret)
 		goto err;
 
-	/* XXX: this is potentially racy, but there is no lock for ppc_md */
-	if (!ppc_md.power_off) {
+	/* XXX: this is potentially racy, but there is no lock for pm_power_off */
+	if (!pm_power_off) {
 		glob_mcu = mcu;
-		ppc_md.power_off = mcu_power_off;
+		pm_power_off = mcu_power_off;
 		dev_info(&client->dev, "will provide power-off service\n");
 	}
 
@@ -196,7 +196,7 @@ static int mcu_remove(struct i2c_client *client)
 	device_remove_file(&client->dev, &dev_attr_status);
 
 	if (glob_mcu == mcu) {
-		ppc_md.power_off = NULL;
+		pm_power_off = NULL;
 		glob_mcu = NULL;
 	}
 
-- 
1.8.1.4

[PATCH 06/20] powerpc/corenet: Use pm_power_off rather than ppc_md.power_off

[Alexander Graf]

The generic power off callback is pm_power_off. Use that one rather than
the powerpc specific ppc_md.power_off.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/platforms/85xx/corenet_generic.c | 2 +-
 arch/powerpc/sysdev/fsl_soc.c                 | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/85xx/corenet_generic.c b/arch/powerpc/platforms/85xx/corenet_generic.c
index d22dd85..89fd568 100644
--- a/arch/powerpc/platforms/85xx/corenet_generic.c
+++ b/arch/powerpc/platforms/85xx/corenet_generic.c
@@ -156,7 +156,7 @@ static int __init corenet_generic_probe(void)
 
 			ppc_md.get_irq = ehv_pic_get_irq;
 			ppc_md.restart = fsl_hv_restart;
-			ppc_md.power_off = fsl_hv_halt;
+			pm_power_off = fsl_hv_halt;
 			ppc_md.halt = fsl_hv_halt;
 #ifdef CONFIG_SMP
 			/*
diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index ffd1169..1e04568 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -238,7 +238,7 @@ void fsl_hv_restart(char *cmd)
 /*
  * Halt the current partition
  *
- * This function should be assigned to the ppc_md.power_off and ppc_md.halt
+ * This function should be assigned to the pm_power_off and ppc_md.halt
  * function pointers, to shut down the partition when we're running under
  * the Freescale hypervisor.
  */
-- 
1.8.1.4

[PATCH 07/20] powerpc/85xx/sgy_cts1000: Use pm_power_off rather than ppc_md.power_off

[Alexander Graf]

The generic power off callback is pm_power_off. Use that one rather than
the powerpc specific ppc_md.power_off.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/platforms/85xx/sgy_cts1000.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/85xx/sgy_cts1000.c b/arch/powerpc/platforms/85xx/sgy_cts1000.c
index bb75add..33f16bb 100644
--- a/arch/powerpc/platforms/85xx/sgy_cts1000.c
+++ b/arch/powerpc/platforms/85xx/sgy_cts1000.c
@@ -120,7 +120,7 @@ static int gpio_halt_probe(struct platform_device *pdev)
 
 	/* Register our halt function */
 	ppc_md.halt = gpio_halt_cb;
-	ppc_md.power_off = gpio_halt_cb;
+	pm_power_off = gpio_halt_cb;
 
 	printk(KERN_INFO "gpio-halt: registered GPIO %d (%d trigger, %d"
 	       " irq).\n", gpio, trigger, irq);
@@ -137,7 +137,7 @@ static int gpio_halt_remove(struct platform_device *pdev)
 		free_irq(irq, halt_node);
 
 		ppc_md.halt = NULL;
-		ppc_md.power_off = NULL;
+		pm_power_off = NULL;
 
 		gpio_free(gpio);
 
-- 
1.8.1.4

[PATCH 08/20] powerpc/celleb: Use pm_power_off rather than ppc_md.power_off

[Alexander Graf]

The generic power off callback is pm_power_off. Use that one rather than
the powerpc specific ppc_md.power_off.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/platforms/cell/celleb_setup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/cell/celleb_setup.c b/arch/powerpc/platforms/cell/celleb_setup.c
index 1d5a4d8..011cc88 100644
--- a/arch/powerpc/platforms/cell/celleb_setup.c
+++ b/arch/powerpc/platforms/cell/celleb_setup.c
@@ -130,6 +130,7 @@ static void __init celleb_setup_arch_beat(void)
 #endif
 
 	celleb_setup_arch_common();
+	pm_power_off = beat_power_off;
 }
 
 static int __init celleb_probe_beat(void)
@@ -178,6 +179,7 @@ static void __init celleb_setup_arch_native(void)
 	/* XXX: nvram initialization should be added */
 
 	celleb_setup_arch_common();
+	pm_power_off = rtas_power_off;
 }
 
 static int __init celleb_probe_native(void)
@@ -204,7 +206,6 @@ define_machine(celleb_beat) {
 	.setup_arch		= celleb_setup_arch_beat,
 	.show_cpuinfo		= celleb_show_cpuinfo,
 	.restart		= beat_restart,
-	.power_off		= beat_power_off,
 	.halt			= beat_halt,
 	.get_rtc_time		= beat_get_rtc_time,
 	.set_rtc_time		= beat_set_rtc_time,
@@ -230,7 +231,6 @@ define_machine(celleb_native) {
 	.setup_arch		= celleb_setup_arch_native,
 	.show_cpuinfo		= celleb_show_cpuinfo,
 	.restart		= rtas_restart,
-	.power_off		= rtas_power_off,
 	.halt			= rtas_halt,
 	.get_boot_time		= rtas_get_boot_time,
 	.get_rtc_time		= rtas_get_rtc_time,
-- 
1.8.1.4

[PATCH 09/20] powerpc/cell/qpace: Use pm_power_off rather than ppc_md.power_off

[Alexander Graf]

The generic power off callback is pm_power_off. Use that one rather than
the powerpc specific ppc_md.power_off.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/platforms/cell/qpace_setup.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/cell/qpace_setup.c b/arch/powerpc/platforms/cell/qpace_setup.c
index 6e3409d..8488094 100644
--- a/arch/powerpc/platforms/cell/qpace_setup.c
+++ b/arch/powerpc/platforms/cell/qpace_setup.c
@@ -117,6 +117,8 @@ static void __init qpace_setup_arch(void)
 #ifdef CONFIG_DUMMY_CONSOLE
 	conswitchp = &dummy_con;
 #endif
+
+	pm_power_off = rtas_power_off;
 }
 
 static int __init qpace_probe(void)
@@ -137,7 +139,6 @@ define_machine(qpace) {
 	.setup_arch		= qpace_setup_arch,
 	.show_cpuinfo		= qpace_show_cpuinfo,
 	.restart		= rtas_restart,
-	.power_off		= rtas_power_off,
 	.halt			= rtas_halt,
 	.get_boot_time		= rtas_get_boot_time,
 	.get_rtc_time		= rtas_get_rtc_time,
-- 
1.8.1.4

[PATCH 10/20] powerpc/cell: Use pm_power_off rather than ppc_md.power_off

[Alexander Graf]

The generic power off callback is pm_power_off. Use that one rather than
the powerpc specific ppc_md.power_off.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/platforms/cell/setup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/cell/setup.c b/arch/powerpc/platforms/cell/setup.c
index 6ae25fb..b6a27d7 100644
--- a/arch/powerpc/platforms/cell/setup.c
+++ b/arch/powerpc/platforms/cell/setup.c
@@ -248,6 +248,7 @@ static void __init cell_setup_arch(void)
 #endif
 
 	mmio_nvram_init();
+	pm_power_off = rtas_power_off;
 }
 
 static int __init cell_probe(void)
@@ -269,7 +270,6 @@ define_machine(cell) {
 	.setup_arch		= cell_setup_arch,
 	.show_cpuinfo		= cell_show_cpuinfo,
 	.restart		= rtas_restart,
-	.power_off		= rtas_power_off,
 	.halt			= rtas_halt,
 	.get_boot_time		= rtas_get_boot_time,
 	.get_rtc_time		= rtas_get_rtc_time,
-- 
1.8.1.4

[PATCH 11/20] powerpc/chrp: Use pm_power_off rather than ppc_md.power_off

[Alexander Graf]

The generic power off callback is pm_power_off. Use that one rather than
the powerpc specific ppc_md.power_off.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/platforms/chrp/setup.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/chrp/setup.c b/arch/powerpc/platforms/chrp/setup.c
index 7044fd3..f9ad816 100644
--- a/arch/powerpc/platforms/chrp/setup.c
+++ b/arch/powerpc/platforms/chrp/setup.c
@@ -356,6 +356,8 @@ void __init chrp_setup_arch(void)
 
 	pci_create_OF_bus_map();
 
+	pm_power_off = rtas_power_off,
+
 	/*
 	 * Print the banner, then scroll down so boot progress
 	 * can be printed.  -- Cort
@@ -597,7 +599,6 @@ define_machine(chrp) {
 	.show_cpuinfo		= chrp_show_cpuinfo,
 	.init_IRQ		= chrp_init_IRQ,
 	.restart		= rtas_restart,
-	.power_off		= rtas_power_off,
 	.halt			= rtas_halt,
 	.time_init		= chrp_time_init,
 	.set_rtc_time		= chrp_set_rtc_time,
-- 
1.8.1.4

[PATCH 12/20] powerpc/6xx/gamecube: Use pm_power_off rather than ppc_md.power_off

[Alexander Graf]

The generic power off callback is pm_power_off. Use that one rather than
the powerpc specific ppc_md.power_off.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/platforms/embedded6xx/gamecube.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/embedded6xx/gamecube.c b/arch/powerpc/platforms/embedded6xx/gamecube.c
index a138e14..3ee7a8b 100644
--- a/arch/powerpc/platforms/embedded6xx/gamecube.c
+++ b/arch/powerpc/platforms/embedded6xx/gamecube.c
@@ -67,6 +67,8 @@ static int __init gamecube_probe(void)
 	if (!of_flat_dt_is_compatible(dt_root, "nintendo,gamecube"))
 		return 0;
 
+	pm_power_off = gamecube_power_off;
+
 	return 1;
 }
 
@@ -80,7 +82,6 @@ define_machine(gamecube) {
 	.probe			= gamecube_probe,
 	.init_early		= gamecube_init_early,
 	.restart		= gamecube_restart,
-	.power_off		= gamecube_power_off,
 	.halt			= gamecube_halt,
 	.init_IRQ		= flipper_pic_probe,
 	.get_irq		= flipper_pic_get_irq,
-- 
1.8.1.4

[PATCH 13/20] powerpc/6xx/linkstation: Use pm_power_off rather than ppc_md.power_off

[Alexander Graf]

The generic power off callback is pm_power_off. Use that one rather than
the powerpc specific ppc_md.power_off.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/platforms/embedded6xx/linkstation.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/embedded6xx/linkstation.c b/arch/powerpc/platforms/embedded6xx/linkstation.c
index 455e7c08..e397e36 100644
--- a/arch/powerpc/platforms/embedded6xx/linkstation.c
+++ b/arch/powerpc/platforms/embedded6xx/linkstation.c
@@ -70,6 +70,8 @@ static void __init linkstation_setup_arch(void)
 	for_each_compatible_node(np, "pci", "mpc10x-pci")
 		linkstation_add_bridge(np);
 
+	pm_power_off = linkstation_power_off;
+
 	printk(KERN_INFO "BUFFALO Network Attached Storage Series\n");
 	printk(KERN_INFO "(C) 2002-2005 BUFFALO INC.\n");
 }
@@ -158,7 +160,6 @@ define_machine(linkstation){
 	.show_cpuinfo 		= linkstation_show_cpuinfo,
 	.get_irq 		= mpic_get_irq,
 	.restart 		= linkstation_restart,
-	.power_off 		= linkstation_power_off,
 	.halt	 		= linkstation_halt,
 	.calibrate_decr 	= generic_calibrate_decr,
 };
-- 
1.8.1.4

[PATCH 14/20] powerpc/6xx/wii: Use pm_power_off rather than ppc_md.power_off

[Alexander Graf]

The generic power off callback is pm_power_off. Use that one rather than
the powerpc specific ppc_md.power_off.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/platforms/embedded6xx/wii.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/embedded6xx/wii.c b/arch/powerpc/platforms/embedded6xx/wii.c
index 6d8dadf..24d656d 100644
--- a/arch/powerpc/platforms/embedded6xx/wii.c
+++ b/arch/powerpc/platforms/embedded6xx/wii.c
@@ -158,6 +158,8 @@ static void __init wii_setup_arch(void)
 		clrbits32(hw_gpio + HW_GPIO_OUT(0),
 			  HW_GPIO_SLOT_LED | HW_GPIO_SENSOR_BAR);
 	}
+
+	pm_power_off = wii_power_off;
 }
 
 static void wii_restart(char *cmd)
@@ -226,7 +228,6 @@ define_machine(wii) {
 	.init_early		= wii_init_early,
 	.setup_arch		= wii_setup_arch,
 	.restart		= wii_restart,
-	.power_off		= wii_power_off,
 	.halt			= wii_halt,
 	.init_IRQ		= wii_pic_probe,
 	.get_irq		= flipper_pic_get_irq,
-- 
1.8.1.4

[PATCH 15/20] powerpc/maple: Use pm_power_off rather than ppc_md.power_off

[Alexander Graf]

The generic power off callback is pm_power_off. Use that one rather than
the powerpc specific ppc_md.power_off.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/platforms/maple/setup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/maple/setup.c b/arch/powerpc/platforms/maple/setup.c
index cb1b0b3..34a08db 100644
--- a/arch/powerpc/platforms/maple/setup.c
+++ b/arch/powerpc/platforms/maple/setup.c
@@ -169,7 +169,7 @@ static void __init maple_use_rtas_reboot_and_halt_if_present(void)
 	if (rtas_service_present("system-reboot") &&
 	    rtas_service_present("power-off")) {
 		ppc_md.restart = rtas_restart;
-		ppc_md.power_off = rtas_power_off;
+		pm_power_off = rtas_power_off;
 		ppc_md.halt = rtas_halt;
 	}
 }
@@ -189,6 +189,7 @@ void __init maple_setup_arch(void)
 #ifdef CONFIG_DUMMY_CONSOLE
 	conswitchp = &dummy_con;
 #endif
+	pm_power_off = maple_power_off;
 	maple_use_rtas_reboot_and_halt_if_present();
 
 	printk(KERN_DEBUG "Using native/NAP idle loop\n");
@@ -325,7 +326,6 @@ define_machine(maple) {
 	.pci_irq_fixup		= maple_pci_irq_fixup,
 	.pci_get_legacy_ide_irq	= maple_pci_get_legacy_ide_irq,
 	.restart		= maple_restart,
-	.power_off		= maple_power_off,
 	.halt			= maple_halt,
        	.get_boot_time		= maple_get_boot_time,
        	.set_rtc_time		= maple_set_rtc_time,
-- 
1.8.1.4

[PATCH 16/20] powerpc/powermac: Use pm_power_off rather than ppc_md.power_off

[Alexander Graf]

The generic power off callback is pm_power_off. Use that one rather than
the powerpc specific ppc_md.power_off.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/platforms/powermac/setup.c | 147 ++++++++++++++++----------------
 1 file changed, 74 insertions(+), 73 deletions(-)

diff --git a/arch/powerpc/platforms/powermac/setup.c b/arch/powerpc/platforms/powermac/setup.c
index 141f8899..d3b0a87 100644
--- a/arch/powerpc/platforms/powermac/setup.c
+++ b/arch/powerpc/platforms/powermac/setup.c
@@ -274,6 +274,78 @@ static void __init l2cr_init(void)
 }
 #endif
 
+#ifdef CONFIG_ADB_CUDA
+static void cuda_restart(void)
+{
+	struct adb_request req;
+
+	cuda_request(&req, NULL, 2, CUDA_PACKET, CUDA_RESET_SYSTEM);
+	for (;;)
+		cuda_poll();
+}
+
+static void cuda_shutdown(void)
+{
+	struct adb_request req;
+
+	cuda_request(&req, NULL, 2, CUDA_PACKET, CUDA_POWERDOWN);
+	for (;;)
+		cuda_poll();
+}
+
+#else
+#define cuda_restart()
+#define cuda_shutdown()
+#endif
+
+#ifndef CONFIG_ADB_PMU
+#define pmu_restart()
+#define pmu_shutdown()
+#endif
+
+#ifndef CONFIG_PMAC_SMU
+#define smu_restart()
+#define smu_shutdown()
+#endif
+
+static void pmac_restart(char *cmd)
+{
+	switch (sys_ctrler) {
+	case SYS_CTRLER_CUDA:
+		cuda_restart();
+		break;
+	case SYS_CTRLER_PMU:
+		pmu_restart();
+		break;
+	case SYS_CTRLER_SMU:
+		smu_restart();
+		break;
+	default: ;
+	}
+}
+
+static void pmac_power_off(void)
+{
+	switch (sys_ctrler) {
+	case SYS_CTRLER_CUDA:
+		cuda_shutdown();
+		break;
+	case SYS_CTRLER_PMU:
+		pmu_shutdown();
+		break;
+	case SYS_CTRLER_SMU:
+		smu_shutdown();
+		break;
+	default: ;
+	}
+}
+
+static void
+pmac_halt(void)
+{
+	pmac_power_off();
+}
+
 static void __init pmac_setup_arch(void)
 {
 	struct device_node *cpu, *ic;
@@ -341,6 +413,8 @@ static void __init pmac_setup_arch(void)
 		__adb_probe_sync = 1;
 	}
 #endif /* CONFIG_ADB */
+
+	pm_power_off = pmac_power_off;
 }
 
 #ifdef CONFIG_SCSI
@@ -382,78 +456,6 @@ void __init_refok note_bootable_part(dev_t dev, int part, int goodness)
 	current_root_goodness = goodness;
 }
 
-#ifdef CONFIG_ADB_CUDA
-static void cuda_restart(void)
-{
-	struct adb_request req;
-
-	cuda_request(&req, NULL, 2, CUDA_PACKET, CUDA_RESET_SYSTEM);
-	for (;;)
-		cuda_poll();
-}
-
-static void cuda_shutdown(void)
-{
-	struct adb_request req;
-
-	cuda_request(&req, NULL, 2, CUDA_PACKET, CUDA_POWERDOWN);
-	for (;;)
-		cuda_poll();
-}
-
-#else
-#define cuda_restart()
-#define cuda_shutdown()
-#endif
-
-#ifndef CONFIG_ADB_PMU
-#define pmu_restart()
-#define pmu_shutdown()
-#endif
-
-#ifndef CONFIG_PMAC_SMU
-#define smu_restart()
-#define smu_shutdown()
-#endif
-
-static void pmac_restart(char *cmd)
-{
-	switch (sys_ctrler) {
-	case SYS_CTRLER_CUDA:
-		cuda_restart();
-		break;
-	case SYS_CTRLER_PMU:
-		pmu_restart();
-		break;
-	case SYS_CTRLER_SMU:
-		smu_restart();
-		break;
-	default: ;
-	}
-}
-
-static void pmac_power_off(void)
-{
-	switch (sys_ctrler) {
-	case SYS_CTRLER_CUDA:
-		cuda_shutdown();
-		break;
-	case SYS_CTRLER_PMU:
-		pmu_shutdown();
-		break;
-	case SYS_CTRLER_SMU:
-		smu_shutdown();
-		break;
-	default: ;
-	}
-}
-
-static void
-pmac_halt(void)
-{
-	pmac_power_off();
-}
-
 /* 
  * Early initialization.
  */
@@ -663,7 +665,6 @@ define_machine(powermac) {
 	.get_irq		= NULL,	/* changed later */
 	.pci_irq_fixup		= pmac_pci_irq_fixup,
 	.restart		= pmac_restart,
-	.power_off		= pmac_power_off,
 	.halt			= pmac_halt,
 	.time_init		= pmac_time_init,
 	.get_boot_time		= pmac_get_boot_time,
-- 
1.8.1.4

[PATCH 17/20] powerpc/powernv: Use pm_power_off rather than ppc_md.power_off

[Alexander Graf]

The generic power off callback is pm_power_off. Use that one rather than
the powerpc specific ppc_md.power_off.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/platforms/powernv/setup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c
index 5a0e2dc..60d137b 100644
--- a/arch/powerpc/platforms/powernv/setup.c
+++ b/arch/powerpc/platforms/powernv/setup.c
@@ -260,7 +260,7 @@ static void __init pnv_setup_machdep_opal(void)
 	ppc_md.get_rtc_time = opal_get_rtc_time;
 	ppc_md.set_rtc_time = opal_set_rtc_time;
 	ppc_md.restart = pnv_restart;
-	ppc_md.power_off = pnv_power_off;
+	pm_power_off = pnv_power_off;
 	ppc_md.halt = pnv_halt;
 	ppc_md.machine_check_exception = opal_machine_check;
 	ppc_md.mce_check_early_recovery = opal_mce_check_early_recovery;
@@ -277,7 +277,7 @@ static void __init pnv_setup_machdep_rtas(void)
 		ppc_md.set_rtc_time = rtas_set_rtc_time;
 	}
 	ppc_md.restart = rtas_restart;
-	ppc_md.power_off = rtas_power_off;
+	pm_power_off = rtas_power_off;
 	ppc_md.halt = rtas_halt;
 }
 #endif /* CONFIG_PPC_POWERNV_RTAS */
-- 
1.8.1.4

[PATCH 18/20] powerpc/ps3: Use pm_power_off rather than ppc_md.power_off

[Alexander Graf]

The generic power off callback is pm_power_off. Use that one rather than
the powerpc specific ppc_md.power_off.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/platforms/ps3/setup.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/ps3/setup.c b/arch/powerpc/platforms/ps3/setup.c
index 3f509f8..ce3b455 100644
--- a/arch/powerpc/platforms/ps3/setup.c
+++ b/arch/powerpc/platforms/ps3/setup.c
@@ -223,6 +223,8 @@ static void __init ps3_setup_arch(void)
 	ppc_md.power_save = ps3_power_save;
 	ps3_os_area_init();
 
+	pm_power_off = ps3_power_off;
+
 	DBG(" <- %s:%d\n", __func__, __LINE__);
 }
 
@@ -278,7 +280,6 @@ define_machine(ps3) {
 	.calibrate_decr			= ps3_calibrate_decr,
 	.progress			= ps3_progress,
 	.restart			= ps3_restart,
-	.power_off			= ps3_power_off,
 	.halt				= ps3_halt,
 #if defined(CONFIG_KEXEC)
 	.kexec_cpu_down			= ps3_kexec_cpu_down,
-- 
1.8.1.4

[PATCH 19/20] powerpc/pseries: Use pm_power_off rather than ppc_md.power_off

[Alexander Graf]

The generic power off callback is pm_power_off. Use that one rather than
the powerpc specific ppc_md.power_off.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/platforms/pseries/setup.c | 59 +++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 29 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
index e724d31..981e9ee 100644
--- a/arch/powerpc/platforms/pseries/setup.c
+++ b/arch/powerpc/platforms/pseries/setup.c
@@ -460,6 +460,34 @@ static long pseries_little_endian_exceptions(void)
 }
 #endif
 
+/**
+ * pSeries_power_off - tell firmware about how to power off the system.
+ *
+ * This function calls either the power-off rtas token in normal cases
+ * or the ibm,power-off-ups token (if present & requested) in case of
+ * a power failure. If power-off token is used, power on will only be
+ * possible with power button press. If ibm,power-off-ups token is used
+ * it will allow auto poweron after power is restored.
+ */
+static void pSeries_power_off(void)
+{
+	int rc;
+	int rtas_poweroff_ups_token = rtas_token("ibm,power-off-ups");
+
+	if (rtas_flash_term_hook)
+		rtas_flash_term_hook(SYS_POWER_OFF);
+
+	if (rtas_poweron_auto == 0 ||
+		rtas_poweroff_ups_token == RTAS_UNKNOWN_SERVICE) {
+		rc = rtas_call(rtas_token("power-off"), 2, 1, NULL, -1, -1);
+		printk(KERN_INFO "RTAS power-off returned %d\n", rc);
+	} else {
+		rc = rtas_call(rtas_poweroff_ups_token, 0, 1, NULL);
+		printk(KERN_INFO "RTAS ibm,power-off-ups returned %d\n", rc);
+	}
+	for (;;);
+}
+
 static void __init pSeries_setup_arch(void)
 {
 	set_arch_panic_timeout(10, ARCH_PANIC_TIMEOUT);
@@ -504,6 +532,8 @@ static void __init pSeries_setup_arch(void)
 				"%ld\n", rc);
 		}
 	}
+
+	pm_power_off = pSeries_power_off;
 }
 
 static int __init pSeries_init_panel(void)
@@ -754,34 +784,6 @@ static int pSeries_pci_probe_mode(struct pci_bus *bus)
 	return PCI_PROBE_NORMAL;
 }
 
-/**
- * pSeries_power_off - tell firmware about how to power off the system.
- *
- * This function calls either the power-off rtas token in normal cases
- * or the ibm,power-off-ups token (if present & requested) in case of
- * a power failure. If power-off token is used, power on will only be
- * possible with power button press. If ibm,power-off-ups token is used
- * it will allow auto poweron after power is restored.
- */
-static void pSeries_power_off(void)
-{
-	int rc;
-	int rtas_poweroff_ups_token = rtas_token("ibm,power-off-ups");
-
-	if (rtas_flash_term_hook)
-		rtas_flash_term_hook(SYS_POWER_OFF);
-
-	if (rtas_poweron_auto == 0 ||
-		rtas_poweroff_ups_token == RTAS_UNKNOWN_SERVICE) {
-		rc = rtas_call(rtas_token("power-off"), 2, 1, NULL, -1, -1);
-		printk(KERN_INFO "RTAS power-off returned %d\n", rc);
-	} else {
-		rc = rtas_call(rtas_poweroff_ups_token, 0, 1, NULL);
-		printk(KERN_INFO "RTAS ibm,power-off-ups returned %d\n", rc);
-	}
-	for (;;);
-}
-
 #ifndef CONFIG_PCI
 void pSeries_final_fixup(void) { }
 #endif
@@ -796,7 +798,6 @@ define_machine(pseries) {
 	.pcibios_fixup		= pSeries_final_fixup,
 	.pci_probe_mode		= pSeries_pci_probe_mode,
 	.restart		= rtas_restart,
-	.power_off		= pSeries_power_off,
 	.halt			= rtas_halt,
 	.panic			= rtas_os_term,
 	.get_boot_time		= rtas_get_boot_time,
-- 
1.8.1.4

[PATCH 20/20] powerpc: Remove ppc_md.power_off

[Alexander Graf]

Now that we have all implementations of ppc_md.power_off converted to
pm_power_off we can remove the ppc_md variant.

While at it, also set the default for pm_power_off to NULL so that non
machine drivers can implement overrides.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/include/asm/machdep.h | 1 -
 arch/powerpc/kernel/setup-common.c | 6 ++----
 arch/powerpc/xmon/xmon.c           | 2 --
 3 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/include/asm/machdep.h b/arch/powerpc/include/asm/machdep.h
index b125cea..10dc008 100644
--- a/arch/powerpc/include/asm/machdep.h
+++ b/arch/powerpc/include/asm/machdep.h
@@ -144,7 +144,6 @@ struct machdep_calls {
 #endif
 
 	void		(*restart)(char *cmd);
-	void		(*power_off)(void);
 	void		(*halt)(void);
 	void		(*panic)(char *str);
 	void		(*cpu_die)(void);
diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
index 5dfcb28..58a260f 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -138,9 +138,7 @@ void machine_restart(char *cmd)
 void machine_power_off(void)
 {
 	machine_shutdown();
-	if (ppc_md.power_off)
-		ppc_md.power_off();
-	if (pm_power_off != machine_power_off)
+	if (pm_power_off)
 		pm_power_off();
 #ifdef CONFIG_SMP
 	smp_send_stop();
@@ -152,7 +150,7 @@ void machine_power_off(void)
 /* Used by the G5 thermal driver */
 EXPORT_SYMBOL_GPL(machine_power_off);
 
-void (*pm_power_off)(void) = machine_power_off;
+void (*pm_power_off)(void);
 EXPORT_SYMBOL_GPL(pm_power_off);
 
 void machine_halt(void)
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 531f649..506d256 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -981,8 +981,6 @@ static void bootcmds(void)
 	else if (cmd == 'h')
 		ppc_md.halt();
 	else if (cmd == 'p')
-		if (ppc_md.power_off)
-			ppc_md.power_off();
 		if (pm_power_off)
 			pm_power_off();
 }
-- 
1.8.1.4

Re: [PATCH 00/20] powerpc: Convert power off logic to pm_power_off

[Geert Uytterhoeven]

Hi Alex,

On Wed, Oct 1, 2014 at 3:27 PM, Alexander Graf <agraf@suse.de> wrote:
> The generic Linux framework to power off the machine is a function pointer
> called pm_power_off. The trick about this pointer is that device drivers can
> potentially implement it rather than board files.
>
> Today on PowerPC we set pm_power_off to invoke our generic full machine power
> off logic which then calls ppc_md.power_off to invoke machine specific power
> off.
>
> However, when we want to add a power off GPIO via the "gpio-poweroff" driver,
> this card house falls apart. That driver only registers itself if pm_power_off
> is NULL to ensure it doesn't override board specific logic. However, since we
> always set pm_power_off to the generic power off logic (which will just not
> power off the machine if no ppc_md.power_off call is implemented), we can't
> implement power off via the generic GPIO power off driver.
>
> To fix this up, let's get rid of the ppc_md.power_off logic and just always use
> pm_power_off as was intended. Then individual drivers such as the GPIO power off
> driver can implement power off logic via that function pointer.
>
> With this patch set applied and a few patches on top of QEMU that implement a
> power off GPIO on the virt e500 machine, I can successfully turn off my virtual
> machine after halt.

This is touching the same area as last night's
"[RFC PATCH 00/16] kernel: Add support for poweroff handler call chain"
https://lkml.org/lkml/2014/9/30/575

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 00/20] powerpc: Convert power off logic to pm_power_off

[Alexander Graf]

On 01.10.14 16:33, Geert Uytterhoeven wrote:
> Hi Alex,
> 
> On Wed, Oct 1, 2014 at 3:27 PM, Alexander Graf <agraf@suse.de> wrote:
>> The generic Linux framework to power off the machine is a function pointer
>> called pm_power_off. The trick about this pointer is that device drivers can
>> potentially implement it rather than board files.
>>
>> Today on PowerPC we set pm_power_off to invoke our generic full machine power
>> off logic which then calls ppc_md.power_off to invoke machine specific power
>> off.
>>
>> However, when we want to add a power off GPIO via the "gpio-poweroff" driver,
>> this card house falls apart. That driver only registers itself if pm_power_off
>> is NULL to ensure it doesn't override board specific logic. However, since we
>> always set pm_power_off to the generic power off logic (which will just not
>> power off the machine if no ppc_md.power_off call is implemented), we can't
>> implement power off via the generic GPIO power off driver.
>>
>> To fix this up, let's get rid of the ppc_md.power_off logic and just always use
>> pm_power_off as was intended. Then individual drivers such as the GPIO power off
>> driver can implement power off logic via that function pointer.
>>
>> With this patch set applied and a few patches on top of QEMU that implement a
>> power off GPIO on the virt e500 machine, I can successfully turn off my virtual
>> machine after halt.
> 
> This is touching the same area as last night's
> "[RFC PATCH 00/16] kernel: Add support for poweroff handler call chain"
> https://lkml.org/lkml/2014/9/30/575

I agree, and I think your patch set is walking into a reasonable
direction. However, I really think it should convert all users of
pm_power_off - at which point you'll probably get to the same conclusion
that ppc_md.power_off is a bad idea :).

So in a way, this patch set is semantically a prerequisite to the full
conversion you'd probably like to do :).

Also, in your cover letter you describe that some methods power off the
CPU power while others power off the system power. How do you
distinguish between them with a call chain? You probably won't get
around to trigger the system power off callback after the CPU power off
callback ran ;).


Alex

Re: [PATCH 00/20] powerpc: Convert power off logic to pm_power_off

[Geert Uytterhoeven]

On Wed, Oct 1, 2014 at 4:47 PM, Alexander Graf <agraf@suse.de> wrote:
>> This is touching the same area as last night's
>> "[RFC PATCH 00/16] kernel: Add support for poweroff handler call chain"
>> https://lkml.org/lkml/2014/9/30/575
>
> I agree, and I think your patch set is walking into a reasonable
> direction. However, I really think it should convert all users of
> pm_power_off - at which point you'll probably get to the same conclusion
> that ppc_md.power_off is a bad idea :).
>
> So in a way, this patch set is semantically a prerequisite to the full
> conversion you'd probably like to do :).
>
> Also, in your cover letter you describe that some methods power off the
> CPU power while others power off the system power. How do you
> distinguish between them with a call chain? You probably won't get
> around to trigger the system power off callback after the CPU power off
> callback ran ;).

I'll let G=C3=BCnther answer that...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k=
.org

In personal conversations with technical people, I call myself a hacker. Bu=
t
when I'm talking to journalists I just say "programmer" or something like t=
hat.
                                -- Linus Torvalds

Re: [PATCH 00/20] powerpc: Convert power off logic to pm_power_off

[Guenter Roeck]

On Wed, Oct 01, 2014 at 04:47:23PM +0200, Alexander Graf wrote:
> 
> 
> On 01.10.14 16:33, Geert Uytterhoeven wrote:
> > Hi Alex,
> > 
> > On Wed, Oct 1, 2014 at 3:27 PM, Alexander Graf <agraf@suse.de> wrote:
> >> The generic Linux framework to power off the machine is a function pointer
> >> called pm_power_off. The trick about this pointer is that device drivers can
> >> potentially implement it rather than board files.
> >>
> >> Today on PowerPC we set pm_power_off to invoke our generic full machine power
> >> off logic which then calls ppc_md.power_off to invoke machine specific power
> >> off.
> >>
> >> However, when we want to add a power off GPIO via the "gpio-poweroff" driver,
> >> this card house falls apart. That driver only registers itself if pm_power_off
> >> is NULL to ensure it doesn't override board specific logic. However, since we
> >> always set pm_power_off to the generic power off logic (which will just not
> >> power off the machine if no ppc_md.power_off call is implemented), we can't
> >> implement power off via the generic GPIO power off driver.
> >>
> >> To fix this up, let's get rid of the ppc_md.power_off logic and just always use
> >> pm_power_off as was intended. Then individual drivers such as the GPIO power off
> >> driver can implement power off logic via that function pointer.
> >>
> >> With this patch set applied and a few patches on top of QEMU that implement a
> >> power off GPIO on the virt e500 machine, I can successfully turn off my virtual
> >> machine after halt.
> > 
> > This is touching the same area as last night's
> > "[RFC PATCH 00/16] kernel: Add support for poweroff handler call chain"
> > https://lkml.org/lkml/2014/9/30/575
> 
> I agree, and I think your patch set is walking into a reasonable
> direction. However, I really think it should convert all users of
> pm_power_off - at which point you'll probably get to the same conclusion
> that ppc_md.power_off is a bad idea :).
> 
Yes, that would be the ultimate goal.

> So in a way, this patch set is semantically a prerequisite to the full
> conversion you'd probably like to do :).
> 
> Also, in your cover letter you describe that some methods power off the
> CPU power while others power off the system power. How do you
> distinguish between them with a call chain? You probably won't get
> around to trigger the system power off callback after the CPU power off
> callback ran ;).
> 
Those are examples. Don't get hung up on it. I may actually replace the
CPU example with something better in the next version; it is not really
a good example and may get people stuck on "why on earth would anyone want
or need a means to turn off the CPU power" instead of focusing on the problem
the patch set tries to solve.

The basic problem is that there can be different poweroff handlers,
some of which may not be available on some systems, and some may not
be as desirable as others for various reasons. The code registering
those poweroff handlers does not specify the poweroff method, but its 
priority. It would be up to the programmer (hopefully together with
the board designer) to determine which method should have higher priority.
Taking the above example, the callback to turn off CPU power would presumably
be one of last resort, and have a very low priority.

A better example may actually be patch 15/16 of the series. The affected
driver (drivers/power/reset/restart-poweroff.c) does not really power off
the system, but restarts it instead. Obviously that would only be a poweroff
handler of last resort, which should only be executed if no other means
to power off the system is available.

Thanks,
Guenter

Re: [PATCH 00/20] powerpc: Convert power off logic to pm_power_off

[Alexander Graf]

On 01.10.14 17:54, Guenter Roeck wrote:
> On Wed, Oct 01, 2014 at 04:47:23PM +0200, Alexander Graf wrote:
>>
>>
>> On 01.10.14 16:33, Geert Uytterhoeven wrote:
>>> Hi Alex,
>>>
>>> On Wed, Oct 1, 2014 at 3:27 PM, Alexander Graf <agraf@suse.de> wrote:
>>>> The generic Linux framework to power off the machine is a function pointer
>>>> called pm_power_off. The trick about this pointer is that device drivers can
>>>> potentially implement it rather than board files.
>>>>
>>>> Today on PowerPC we set pm_power_off to invoke our generic full machine power
>>>> off logic which then calls ppc_md.power_off to invoke machine specific power
>>>> off.
>>>>
>>>> However, when we want to add a power off GPIO via the "gpio-poweroff" driver,
>>>> this card house falls apart. That driver only registers itself if pm_power_off
>>>> is NULL to ensure it doesn't override board specific logic. However, since we
>>>> always set pm_power_off to the generic power off logic (which will just not
>>>> power off the machine if no ppc_md.power_off call is implemented), we can't
>>>> implement power off via the generic GPIO power off driver.
>>>>
>>>> To fix this up, let's get rid of the ppc_md.power_off logic and just always use
>>>> pm_power_off as was intended. Then individual drivers such as the GPIO power off
>>>> driver can implement power off logic via that function pointer.
>>>>
>>>> With this patch set applied and a few patches on top of QEMU that implement a
>>>> power off GPIO on the virt e500 machine, I can successfully turn off my virtual
>>>> machine after halt.
>>>
>>> This is touching the same area as last night's
>>> "[RFC PATCH 00/16] kernel: Add support for poweroff handler call chain"
>>> https://lkml.org/lkml/2014/9/30/575
>>
>> I agree, and I think your patch set is walking into a reasonable
>> direction. However, I really think it should convert all users of
>> pm_power_off - at which point you'll probably get to the same conclusion
>> that ppc_md.power_off is a bad idea :).
>>
> Yes, that would be the ultimate goal.
> 
>> So in a way, this patch set is semantically a prerequisite to the full
>> conversion you'd probably like to do :).
>>
>> Also, in your cover letter you describe that some methods power off the
>> CPU power while others power off the system power. How do you
>> distinguish between them with a call chain? You probably won't get
>> around to trigger the system power off callback after the CPU power off
>> callback ran ;).
>>
> Those are examples. Don't get hung up on it. I may actually replace the
> CPU example with something better in the next version; it is not really
> a good example and may get people stuck on "why on earth would anyone want
> or need a means to turn off the CPU power" instead of focusing on the problem
> the patch set tries to solve.
> 
> The basic problem is that there can be different poweroff handlers,
> some of which may not be available on some systems, and some may not
> be as desirable as others for various reasons. The code registering
> those poweroff handlers does not specify the poweroff method, but its 
> priority. It would be up to the programmer (hopefully together with
> the board designer) to determine which method should have higher priority.
> Taking the above example, the callback to turn off CPU power would presumably
> be one of last resort, and have a very low priority.
> 
> A better example may actually be patch 15/16 of the series. The affected
> driver (drivers/power/reset/restart-poweroff.c) does not really power off
> the system, but restarts it instead. Obviously that would only be a poweroff
> handler of last resort, which should only be executed if no other means
> to power off the system is available.

Sounds like a good plan :). You probably want to have some global list
of priority numbers like "try this first" or "this is a non-optimal, but
working method" and "only ever do this as last resort".

Maybe you could as a first step convert every user of pm_power_off to
this new framework with a global notifier_block, similar to how
pm_power_off is a global today? Then we can at least get rid of
pm_power_off altogether and move to only notifiers, whereas new
notifiers can come before or after the old machine set implementations.

As a nice bonus this automatically converts every user of pm_power_off()
to instead call the notifier chain.


Alex

Re: [PATCH 00/20] powerpc: Convert power off logic to pm_power_off

[Guenter Roeck]

On 10/01/2014 02:25 PM, Alexander Graf wrote:
>
>
> On 01.10.14 17:54, Guenter Roeck wrote:
>> On Wed, Oct 01, 2014 at 04:47:23PM +0200, Alexander Graf wrote:
>>>
>>>
>>> On 01.10.14 16:33, Geert Uytterhoeven wrote:
>>>> Hi Alex,
>>>>
>>>> On Wed, Oct 1, 2014 at 3:27 PM, Alexander Graf <agraf@suse.de> wrote:
>>>>> The generic Linux framework to power off the machine is a function pointer
>>>>> called pm_power_off. The trick about this pointer is that device drivers can
>>>>> potentially implement it rather than board files.
>>>>>
>>>>> Today on PowerPC we set pm_power_off to invoke our generic full machine power
>>>>> off logic which then calls ppc_md.power_off to invoke machine specific power
>>>>> off.
>>>>>
>>>>> However, when we want to add a power off GPIO via the "gpio-poweroff" driver,
>>>>> this card house falls apart. That driver only registers itself if pm_power_off
>>>>> is NULL to ensure it doesn't override board specific logic. However, since we
>>>>> always set pm_power_off to the generic power off logic (which will just not
>>>>> power off the machine if no ppc_md.power_off call is implemented), we can't
>>>>> implement power off via the generic GPIO power off driver.
>>>>>
>>>>> To fix this up, let's get rid of the ppc_md.power_off logic and just always use
>>>>> pm_power_off as was intended. Then individual drivers such as the GPIO power off
>>>>> driver can implement power off logic via that function pointer.
>>>>>
>>>>> With this patch set applied and a few patches on top of QEMU that implement a
>>>>> power off GPIO on the virt e500 machine, I can successfully turn off my virtual
>>>>> machine after halt.
>>>>
>>>> This is touching the same area as last night's
>>>> "[RFC PATCH 00/16] kernel: Add support for poweroff handler call chain"
>>>> https://lkml.org/lkml/2014/9/30/575
>>>
>>> I agree, and I think your patch set is walking into a reasonable
>>> direction. However, I really think it should convert all users of
>>> pm_power_off - at which point you'll probably get to the same conclusion
>>> that ppc_md.power_off is a bad idea :).
>>>
>> Yes, that would be the ultimate goal.
>>
>>> So in a way, this patch set is semantically a prerequisite to the full
>>> conversion you'd probably like to do :).
>>>
>>> Also, in your cover letter you describe that some methods power off the
>>> CPU power while others power off the system power. How do you
>>> distinguish between them with a call chain? You probably won't get
>>> around to trigger the system power off callback after the CPU power off
>>> callback ran ;).
>>>
>> Those are examples. Don't get hung up on it. I may actually replace the
>> CPU example with something better in the next version; it is not really
>> a good example and may get people stuck on "why on earth would anyone want
>> or need a means to turn off the CPU power" instead of focusing on the problem
>> the patch set tries to solve.
>>
>> The basic problem is that there can be different poweroff handlers,
>> some of which may not be available on some systems, and some may not
>> be as desirable as others for various reasons. The code registering
>> those poweroff handlers does not specify the poweroff method, but its
>> priority. It would be up to the programmer (hopefully together with
>> the board designer) to determine which method should have higher priority.
>> Taking the above example, the callback to turn off CPU power would presumably
>> be one of last resort, and have a very low priority.
>>
>> A better example may actually be patch 15/16 of the series. The affected
>> driver (drivers/power/reset/restart-poweroff.c) does not really power off
>> the system, but restarts it instead. Obviously that would only be a poweroff
>> handler of last resort, which should only be executed if no other means
>> to power off the system is available.
>
> Sounds like a good plan :). You probably want to have some global list
> of priority numbers like "try this first" or "this is a non-optimal, but
> working method" and "only ever do this as last resort".
>
Yes, this is already in the patch set, similar to the restart handler.

> Maybe you could as a first step convert every user of pm_power_off to
> this new framework with a global notifier_block, similar to how
> pm_power_off is a global today? Then we can at least get rid of
> pm_power_off altogether and move to only notifiers, whereas new
> notifiers can come before or after the old machine set implementations.
>
> As a nice bonus this automatically converts every user of pm_power_off()
> to instead call the notifier chain.
>
Interesting idea, but I am not really sure if it would make much of
a difference. I would still have to implement that handler for each
platform. I might as well convert all users directly; at least this would
ensure that all users are converted.

Guenter

Re: [PATCH 00/20] powerpc: Convert power off logic to pm_power_off

[Scott Wood]

On Wed, 2014-10-01 at 15:27 +0200, Alexander Graf wrote:
> The generic Linux framework to power off the machine is a function pointer
> called pm_power_off. The trick about this pointer is that device drivers can
> potentially implement it rather than board files.
> 
> Today on PowerPC we set pm_power_off to invoke our generic full machine power
> off logic which then calls ppc_md.power_off to invoke machine specific power
> off.
> 
> However, when we want to add a power off GPIO via the "gpio-poweroff" driver,
> this card house falls apart. That driver only registers itself if pm_power_off
> is NULL to ensure it doesn't override board specific logic. However, since we
> always set pm_power_off to the generic power off logic (which will just not
> power off the machine if no ppc_md.power_off call is implemented), we can't
> implement power off via the generic GPIO power off driver.
> 
> To fix this up, let's get rid of the ppc_md.power_off logic and just always use
> pm_power_off as was intended. Then individual drivers such as the GPIO power off
> driver can implement power off logic via that function pointer.
> 
> With this patch set applied and a few patches on top of QEMU that implement a
> power off GPIO on the virt e500 machine, I can successfully turn off my virtual
> machine after halt.

Are there any plans to handle restart similarly?

-Scott

Re: [PATCH 00/20] powerpc: Convert power off logic to pm_power_off

[Alexander Graf]

On 02.10.14 00:39, Scott Wood wrote:
> On Wed, 2014-10-01 at 15:27 +0200, Alexander Graf wrote:
>> The generic Linux framework to power off the machine is a function pointer
>> called pm_power_off. The trick about this pointer is that device drivers can
>> potentially implement it rather than board files.
>>
>> Today on PowerPC we set pm_power_off to invoke our generic full machine power
>> off logic which then calls ppc_md.power_off to invoke machine specific power
>> off.
>>
>> However, when we want to add a power off GPIO via the "gpio-poweroff" driver,
>> this card house falls apart. That driver only registers itself if pm_power_off
>> is NULL to ensure it doesn't override board specific logic. However, since we
>> always set pm_power_off to the generic power off logic (which will just not
>> power off the machine if no ppc_md.power_off call is implemented), we can't
>> implement power off via the generic GPIO power off driver.
>>
>> To fix this up, let's get rid of the ppc_md.power_off logic and just always use
>> pm_power_off as was intended. Then individual drivers such as the GPIO power off
>> driver can implement power off logic via that function pointer.
>>
>> With this patch set applied and a few patches on top of QEMU that implement a
>> power off GPIO on the virt e500 machine, I can successfully turn off my virtual
>> machine after halt.
> 
> Are there any plans to handle restart similarly?

I don't see an immediate need for it, as we do have access to reset via
the guts device.

I also don't see any generic driver in Linux that would implement reset
via a gpio controller device, so apparently it's not incredibly common
to implement reset via gpio.


Alex

Re: [PATCH 00/20] powerpc: Convert power off logic to pm_power_off

[Scott Wood]

On Thu, 2014-10-02 at 01:21 +0200, Alexander Graf wrote:
> 
> On 02.10.14 00:39, Scott Wood wrote:
> > On Wed, 2014-10-01 at 15:27 +0200, Alexander Graf wrote:
> >> The generic Linux framework to power off the machine is a function pointer
> >> called pm_power_off. The trick about this pointer is that device drivers can
> >> potentially implement it rather than board files.
> >>
> >> Today on PowerPC we set pm_power_off to invoke our generic full machine power
> >> off logic which then calls ppc_md.power_off to invoke machine specific power
> >> off.
> >>
> >> However, when we want to add a power off GPIO via the "gpio-poweroff" driver,
> >> this card house falls apart. That driver only registers itself if pm_power_off
> >> is NULL to ensure it doesn't override board specific logic. However, since we
> >> always set pm_power_off to the generic power off logic (which will just not
> >> power off the machine if no ppc_md.power_off call is implemented), we can't
> >> implement power off via the generic GPIO power off driver.
> >>
> >> To fix this up, let's get rid of the ppc_md.power_off logic and just always use
> >> pm_power_off as was intended. Then individual drivers such as the GPIO power off
> >> driver can implement power off logic via that function pointer.
> >>
> >> With this patch set applied and a few patches on top of QEMU that implement a
> >> power off GPIO on the virt e500 machine, I can successfully turn off my virtual
> >> machine after halt.
> > 
> > Are there any plans to handle restart similarly?
> 
> I don't see an immediate need for it, as we do have access to reset via
> the guts device.

The guts device is problematic from a hardware description perspective,
as we advertise a bunch of things to the guest that we don't implement.
E.g. the only reason we don't currently have a problem with Linux trying
to use guts to sync the timebase across cores is that by chance we
happen to expose a guts that corresponds to a single-cpu SoC.

> I also don't see any generic driver in Linux that would implement reset
> via a gpio controller device, so apparently it's not incredibly common
> to implement reset via gpio.

There wasn't a generic gpio-power-off driver either until a couple years
ago...  Regardless of whether it's done via gpio, using ppc_md for it is
bad for the same reasons as for power-off.

-Scott

Re: [PATCH 00/20] powerpc: Convert power off logic to pm_power_off

[Alexander Graf]

On 02.10.14 01:28, Scott Wood wrote:
> On Thu, 2014-10-02 at 01:21 +0200, Alexander Graf wrote:
>>
>> On 02.10.14 00:39, Scott Wood wrote:
>>> On Wed, 2014-10-01 at 15:27 +0200, Alexander Graf wrote:
>>>> The generic Linux framework to power off the machine is a function pointer
>>>> called pm_power_off. The trick about this pointer is that device drivers can
>>>> potentially implement it rather than board files.
>>>>
>>>> Today on PowerPC we set pm_power_off to invoke our generic full machine power
>>>> off logic which then calls ppc_md.power_off to invoke machine specific power
>>>> off.
>>>>
>>>> However, when we want to add a power off GPIO via the "gpio-poweroff" driver,
>>>> this card house falls apart. That driver only registers itself if pm_power_off
>>>> is NULL to ensure it doesn't override board specific logic. However, since we
>>>> always set pm_power_off to the generic power off logic (which will just not
>>>> power off the machine if no ppc_md.power_off call is implemented), we can't
>>>> implement power off via the generic GPIO power off driver.
>>>>
>>>> To fix this up, let's get rid of the ppc_md.power_off logic and just always use
>>>> pm_power_off as was intended. Then individual drivers such as the GPIO power off
>>>> driver can implement power off logic via that function pointer.
>>>>
>>>> With this patch set applied and a few patches on top of QEMU that implement a
>>>> power off GPIO on the virt e500 machine, I can successfully turn off my virtual
>>>> machine after halt.
>>>
>>> Are there any plans to handle restart similarly?
>>
>> I don't see an immediate need for it, as we do have access to reset via
>> the guts device.
> 
> The guts device is problematic from a hardware description perspective,
> as we advertise a bunch of things to the guest that we don't implement.
> E.g. the only reason we don't currently have a problem with Linux trying
> to use guts to sync the timebase across cores is that by chance we
> happen to expose a guts that corresponds to a single-cpu SoC.

True, and it is slightly ugly to expose a specific SoC's guts in our
generic pv machine type.

> 
>> I also don't see any generic driver in Linux that would implement reset
>> via a gpio controller device, so apparently it's not incredibly common
>> to implement reset via gpio.
> 
> There wasn't a generic gpio-power-off driver either until a couple years
> ago...  Regardless of whether it's done via gpio, using ppc_md for it is
> bad for the same reasons as for power-off.

I agree. Today, only ARM has a comparable mechanism for drivers to hook
a reset handler into the machine specific reset path.

I'd say let's wait for Guenter to finish the power off rework and then
tackle a generic reset call chain based approach next.


Alex

Re: [PATCH 00/20] powerpc: Convert power off logic to pm_power_off

[Guenter Roeck]

On 10/01/2014 04:44 PM, Alexander Graf wrote:
>
>
> On 02.10.14 01:28, Scott Wood wrote:
>> On Thu, 2014-10-02 at 01:21 +0200, Alexander Graf wrote:
>>>
>>> On 02.10.14 00:39, Scott Wood wrote:
>>>> On Wed, 2014-10-01 at 15:27 +0200, Alexander Graf wrote:
>>>>> The generic Linux framework to power off the machine is a function pointer
>>>>> called pm_power_off. The trick about this pointer is that device drivers can
>>>>> potentially implement it rather than board files.
>>>>>
>>>>> Today on PowerPC we set pm_power_off to invoke our generic full machine power
>>>>> off logic which then calls ppc_md.power_off to invoke machine specific power
>>>>> off.
>>>>>
>>>>> However, when we want to add a power off GPIO via the "gpio-poweroff" driver,
>>>>> this card house falls apart. That driver only registers itself if pm_power_off
>>>>> is NULL to ensure it doesn't override board specific logic. However, since we
>>>>> always set pm_power_off to the generic power off logic (which will just not
>>>>> power off the machine if no ppc_md.power_off call is implemented), we can't
>>>>> implement power off via the generic GPIO power off driver.
>>>>>
>>>>> To fix this up, let's get rid of the ppc_md.power_off logic and just always use
>>>>> pm_power_off as was intended. Then individual drivers such as the GPIO power off
>>>>> driver can implement power off logic via that function pointer.
>>>>>
>>>>> With this patch set applied and a few patches on top of QEMU that implement a
>>>>> power off GPIO on the virt e500 machine, I can successfully turn off my virtual
>>>>> machine after halt.
>>>>
>>>> Are there any plans to handle restart similarly?
>>>
Guess I am missing some context here. Support for a restart handler call chain
will be added in 3.18. Currently its primary goal is to replace arm_pm_restart,
but it would be a one-liner to add support for it to powerpc (see below).

>>> I don't see an immediate need for it, as we do have access to reset via
>>> the guts device.
>>
>> The guts device is problematic from a hardware description perspective,
>> as we advertise a bunch of things to the guest that we don't implement.
>> E.g. the only reason we don't currently have a problem with Linux trying
>> to use guts to sync the timebase across cores is that by chance we
>> happen to expose a guts that corresponds to a single-cpu SoC.
>
> True, and it is slightly ugly to expose a specific SoC's guts in our
> generic pv machine type.
>
>>
>>> I also don't see any generic driver in Linux that would implement reset
>>> via a gpio controller device, so apparently it's not incredibly common
>>> to implement reset via gpio.
>>
Again, I may be missing something. How about drivers/power/reset/gpio-restart.c ?
It would be really simple make it work with powerpc; all you would have to do
is to add a call to do_kernel_restart() to the powerpc machine_restart() function.

>> There wasn't a generic gpio-power-off driver either until a couple years
>> ago...  Regardless of whether it's done via gpio, using ppc_md for it is
>> bad for the same reasons as for power-off.
>
> I agree. Today, only ARM has a comparable mechanism for drivers to hook
> a reset handler into the machine specific reset path.
>
Not anymore ...

> I'd say let's wait for Guenter to finish the power off rework and then
> tackle a generic reset call chain based approach next.
>
Please have a look into the restart handler; the code is in linux-next.
That should probably solve your restart related problems.
A branch on top of v3.17-rc2 is at
https://git.kernel.org/cgit/linux/kernel/git/groeck/linux-staging.git/log/?h=restart-handler

Thanks,
Guenter

Re: [PATCH 00/20] powerpc: Convert power off logic to pm_power_off

[Michael Ellerman]

On Wed, 2014-10-01 at 15:27 +0200, Alexander Graf wrote:
> The generic Linux framework to power off the machine is a function pointer
> called pm_power_off. The trick about this pointer is that device drivers can
> potentially implement it rather than board files.
> 
> Today on PowerPC we set pm_power_off to invoke our generic full machine power
> off logic which then calls ppc_md.power_off to invoke machine specific power
> off.
> 
> To fix this up, let's get rid of the ppc_md.power_off logic and just always use
> pm_power_off as was intended. Then individual drivers such as the GPIO power off
> driver can implement power off logic via that function pointer.

This looks OK to me with one caveat.

In several of the patches you're replacing a static initialisation with a
runtime one, and you're doing the runtime initialisation in xxx_setup_arch().
That's reasonably late, so I'd prefer you did it in xxx_probe().

If you resend with that changed I'll put it in next.

cheers

Re: [PATCH 00/20] powerpc: Convert power off logic to pm_power_off

[Alexander Graf]

On 03.10.14 06:42, Michael Ellerman wrote:
> On Wed, 2014-10-01 at 15:27 +0200, Alexander Graf wrote:
>> The generic Linux framework to power off the machine is a function pointer
>> called pm_power_off. The trick about this pointer is that device drivers can
>> potentially implement it rather than board files.
>>
>> Today on PowerPC we set pm_power_off to invoke our generic full machine power
>> off logic which then calls ppc_md.power_off to invoke machine specific power
>> off.
>>
>> To fix this up, let's get rid of the ppc_md.power_off logic and just always use
>> pm_power_off as was intended. Then individual drivers such as the GPIO power off
>> driver can implement power off logic via that function pointer.
> 
> This looks OK to me with one caveat.
> 
> In several of the patches you're replacing a static initialisation with a
> runtime one, and you're doing the runtime initialisation in xxx_setup_arch().
> That's reasonably late, so I'd prefer you did it in xxx_probe().

Heh, I had it in xxx_probe() originally and then realized that

  a) the power off function is basically a driver. Driver initialization
happens in xxx_setup_arch() and

  b) the maple target already does overwrite its power_off callback in
xxx_setup_arch and

  c) on all targets xxx_probe() is very slim and doesn't do much

but I'll happily change it back to put the bits in xxx_probe() instead.


Alex

Re: [PATCH 00/20] powerpc: Convert power off logic to pm_power_off

[Michael Ellerman]

On Mon, 2014-10-06 at 12:00 +0200, Alexander Graf wrote:
> 
> On 03.10.14 06:42, Michael Ellerman wrote:
> > On Wed, 2014-10-01 at 15:27 +0200, Alexander Graf wrote:
> >> The generic Linux framework to power off the machine is a function pointer
> >> called pm_power_off. The trick about this pointer is that device drivers can
> >> potentially implement it rather than board files.
> >>
> >> Today on PowerPC we set pm_power_off to invoke our generic full machine power
> >> off logic which then calls ppc_md.power_off to invoke machine specific power
> >> off.
> >>
> >> To fix this up, let's get rid of the ppc_md.power_off logic and just always use
> >> pm_power_off as was intended. Then individual drivers such as the GPIO power off
> >> driver can implement power off logic via that function pointer.
> > 
> > This looks OK to me with one caveat.
> > 
> > In several of the patches you're replacing a static initialisation with a
> > runtime one, and you're doing the runtime initialisation in xxx_setup_arch().
> > That's reasonably late, so I'd prefer you did it in xxx_probe().
> 
> Heh, I had it in xxx_probe() originally and then realized that
> 
>   a) the power off function is basically a driver. Driver initialization
> happens in xxx_setup_arch() and
> 
>   b) the maple target already does overwrite its power_off callback in
> xxx_setup_arch and
> 
>   c) on all targets xxx_probe() is very slim and doesn't do much
> 
> but I'll happily change it back to put the bits in xxx_probe() instead.

Thanks.

That way you shouldn't be changing behaviour.

It may still be the case that some power off routines don't actually work until
later, but that's an existing problem. Some power off routines *do* work before
setup_arch(), so they will continue to work.


Also, how does your series interact with Guenter's that removes pm_power_off ?
It seems at the moment they are unaware of each other.

cheers

Re: [PATCH 00/20] powerpc: Convert power off logic to pm_power_off

[Alexander Graf]

On 07.10.14 08:25, Michael Ellerman wrote:
> On Mon, 2014-10-06 at 12:00 +0200, Alexander Graf wrote:
>>
>> On 03.10.14 06:42, Michael Ellerman wrote:
>>> On Wed, 2014-10-01 at 15:27 +0200, Alexander Graf wrote:
>>>> The generic Linux framework to power off the machine is a function pointer
>>>> called pm_power_off. The trick about this pointer is that device drivers can
>>>> potentially implement it rather than board files.
>>>>
>>>> Today on PowerPC we set pm_power_off to invoke our generic full machine power
>>>> off logic which then calls ppc_md.power_off to invoke machine specific power
>>>> off.
>>>>
>>>> To fix this up, let's get rid of the ppc_md.power_off logic and just always use
>>>> pm_power_off as was intended. Then individual drivers such as the GPIO power off
>>>> driver can implement power off logic via that function pointer.
>>>
>>> This looks OK to me with one caveat.
>>>
>>> In several of the patches you're replacing a static initialisation with a
>>> runtime one, and you're doing the runtime initialisation in xxx_setup_arch().
>>> That's reasonably late, so I'd prefer you did it in xxx_probe().
>>
>> Heh, I had it in xxx_probe() originally and then realized that
>>
>>   a) the power off function is basically a driver. Driver initialization
>> happens in xxx_setup_arch() and
>>
>>   b) the maple target already does overwrite its power_off callback in
>> xxx_setup_arch and
>>
>>   c) on all targets xxx_probe() is very slim and doesn't do much
>>
>> but I'll happily change it back to put the bits in xxx_probe() instead.
> 
> Thanks.
> 
> That way you shouldn't be changing behaviour.
> 
> It may still be the case that some power off routines don't actually work until
> later, but that's an existing problem. Some power off routines *do* work before
> setup_arch(), so they will continue to work.

Ok, works for me :). Just wanted to make sure you're aware of the
reasoning why I didn't do it in probe().

> Also, how does your series interact with Guenter's that removes pm_power_off ?
> It seems at the moment they are unaware of each other.

Guenters patches convert users of pm_power_off to his new scheme. We're
not even at that stage at all yet in the powerpc tree. Converting
everything to pm_power_off is basically a first step. His patch set
maintains pm_power_off, so there shouldn't be nasty conflicts.

Once we converted from ppc_md tables to actual code, we can just run a
simple coccinelle patch to convert from pm_power_off to his new scheme
later.


Alex

Re: [PATCH 00/20] powerpc: Convert power off logic to pm_power_off

[Guenter Roeck]

On Tue, Oct 07, 2014 at 01:35:07PM +0200, Alexander Graf wrote:
> 
> 
> On 07.10.14 08:25, Michael Ellerman wrote:
> > On Mon, 2014-10-06 at 12:00 +0200, Alexander Graf wrote:
> >>
> >> On 03.10.14 06:42, Michael Ellerman wrote:
> >>> On Wed, 2014-10-01 at 15:27 +0200, Alexander Graf wrote:
> >>>> The generic Linux framework to power off the machine is a function pointer
> >>>> called pm_power_off. The trick about this pointer is that device drivers can
> >>>> potentially implement it rather than board files.
> >>>>
> >>>> Today on PowerPC we set pm_power_off to invoke our generic full machine power
> >>>> off logic which then calls ppc_md.power_off to invoke machine specific power
> >>>> off.
> >>>>
> >>>> To fix this up, let's get rid of the ppc_md.power_off logic and just always use
> >>>> pm_power_off as was intended. Then individual drivers such as the GPIO power off
> >>>> driver can implement power off logic via that function pointer.
> >>>
> >>> This looks OK to me with one caveat.
> >>>
> >>> In several of the patches you're replacing a static initialisation with a
> >>> runtime one, and you're doing the runtime initialisation in xxx_setup_arch().
> >>> That's reasonably late, so I'd prefer you did it in xxx_probe().
> >>
> >> Heh, I had it in xxx_probe() originally and then realized that
> >>
> >>   a) the power off function is basically a driver. Driver initialization
> >> happens in xxx_setup_arch() and
> >>
> >>   b) the maple target already does overwrite its power_off callback in
> >> xxx_setup_arch and
> >>
> >>   c) on all targets xxx_probe() is very slim and doesn't do much
> >>
> >> but I'll happily change it back to put the bits in xxx_probe() instead.
> > 
> > Thanks.
> > 
> > That way you shouldn't be changing behaviour.
> > 
> > It may still be the case that some power off routines don't actually work until
> > later, but that's an existing problem. Some power off routines *do* work before
> > setup_arch(), so they will continue to work.
> 
> Ok, works for me :). Just wanted to make sure you're aware of the
> reasoning why I didn't do it in probe().
> 
> > Also, how does your series interact with Guenter's that removes pm_power_off ?
> > It seems at the moment they are unaware of each other.
> 
> Guenters patches convert users of pm_power_off to his new scheme. We're
> not even at that stage at all yet in the powerpc tree. Converting
> everything to pm_power_off is basically a first step. His patch set
> maintains pm_power_off, so there shouldn't be nasty conflicts.
> 
Onlly the first m68k patch, though. The very last patch in the series
remvoes pm_power_off.

Guenter

> Once we converted from ppc_md tables to actual code, we can just run a
> simple coccinelle patch to convert from pm_power_off to his new scheme
> later.
> 
> 
> Alex

Re: [PATCH 00/20] powerpc: Convert power off logic to pm_power_off

[Alexander Graf]

On 07.10.14 19:00, Guenter Roeck wrote:
> On Tue, Oct 07, 2014 at 01:35:07PM +0200, Alexander Graf wrote:
>>
>>
>> On 07.10.14 08:25, Michael Ellerman wrote:
>>> On Mon, 2014-10-06 at 12:00 +0200, Alexander Graf wrote:
>>>>
>>>> On 03.10.14 06:42, Michael Ellerman wrote:
>>>>> On Wed, 2014-10-01 at 15:27 +0200, Alexander Graf wrote:
>>>>>> The generic Linux framework to power off the machine is a function pointer
>>>>>> called pm_power_off. The trick about this pointer is that device drivers can
>>>>>> potentially implement it rather than board files.
>>>>>>
>>>>>> Today on PowerPC we set pm_power_off to invoke our generic full machine power
>>>>>> off logic which then calls ppc_md.power_off to invoke machine specific power
>>>>>> off.
>>>>>>
>>>>>> To fix this up, let's get rid of the ppc_md.power_off logic and just always use
>>>>>> pm_power_off as was intended. Then individual drivers such as the GPIO power off
>>>>>> driver can implement power off logic via that function pointer.
>>>>>
>>>>> This looks OK to me with one caveat.
>>>>>
>>>>> In several of the patches you're replacing a static initialisation with a
>>>>> runtime one, and you're doing the runtime initialisation in xxx_setup_arch().
>>>>> That's reasonably late, so I'd prefer you did it in xxx_probe().
>>>>
>>>> Heh, I had it in xxx_probe() originally and then realized that
>>>>
>>>>   a) the power off function is basically a driver. Driver initialization
>>>> happens in xxx_setup_arch() and
>>>>
>>>>   b) the maple target already does overwrite its power_off callback in
>>>> xxx_setup_arch and
>>>>
>>>>   c) on all targets xxx_probe() is very slim and doesn't do much
>>>>
>>>> but I'll happily change it back to put the bits in xxx_probe() instead.
>>>
>>> Thanks.
>>>
>>> That way you shouldn't be changing behaviour.
>>>
>>> It may still be the case that some power off routines don't actually work until
>>> later, but that's an existing problem. Some power off routines *do* work before
>>> setup_arch(), so they will continue to work.
>>
>> Ok, works for me :). Just wanted to make sure you're aware of the
>> reasoning why I didn't do it in probe().
>>
>>> Also, how does your series interact with Guenter's that removes pm_power_off ?
>>> It seems at the moment they are unaware of each other.
>>
>> Guenters patches convert users of pm_power_off to his new scheme. We're
>> not even at that stage at all yet in the powerpc tree. Converting
>> everything to pm_power_off is basically a first step. His patch set
>> maintains pm_power_off, so there shouldn't be nasty conflicts.
>>
> Onlly the first m68k patch, though. The very last patch in the series
> remvoes pm_power_off.

And there go my patch reading skills ... :).

For which window are you targeting this? 3.18 or 3.19? If you're trying
to hit 3.18, I can easily wait with my patch set and base it on top of
yours.


Alex

Re: [PATCH 00/20] powerpc: Convert power off logic to pm_power_off

[Guenter Roeck]

On Tue, Oct 07, 2014 at 07:47:35PM +0200, Alexander Graf wrote:
> 
> 
> On 07.10.14 19:00, Guenter Roeck wrote:
> > On Tue, Oct 07, 2014 at 01:35:07PM +0200, Alexander Graf wrote:
> >>
> >>
> >> On 07.10.14 08:25, Michael Ellerman wrote:
> >>> On Mon, 2014-10-06 at 12:00 +0200, Alexander Graf wrote:
> >>>>
> >>>> On 03.10.14 06:42, Michael Ellerman wrote:
> >>>>> On Wed, 2014-10-01 at 15:27 +0200, Alexander Graf wrote:
> >>>>>> The generic Linux framework to power off the machine is a function pointer
> >>>>>> called pm_power_off. The trick about this pointer is that device drivers can
> >>>>>> potentially implement it rather than board files.
> >>>>>>
> >>>>>> Today on PowerPC we set pm_power_off to invoke our generic full machine power
> >>>>>> off logic which then calls ppc_md.power_off to invoke machine specific power
> >>>>>> off.
> >>>>>>
> >>>>>> To fix this up, let's get rid of the ppc_md.power_off logic and just always use
> >>>>>> pm_power_off as was intended. Then individual drivers such as the GPIO power off
> >>>>>> driver can implement power off logic via that function pointer.
> >>>>>
> >>>>> This looks OK to me with one caveat.
> >>>>>
> >>>>> In several of the patches you're replacing a static initialisation with a
> >>>>> runtime one, and you're doing the runtime initialisation in xxx_setup_arch().
> >>>>> That's reasonably late, so I'd prefer you did it in xxx_probe().
> >>>>
> >>>> Heh, I had it in xxx_probe() originally and then realized that
> >>>>
> >>>>   a) the power off function is basically a driver. Driver initialization
> >>>> happens in xxx_setup_arch() and
> >>>>
> >>>>   b) the maple target already does overwrite its power_off callback in
> >>>> xxx_setup_arch and
> >>>>
> >>>>   c) on all targets xxx_probe() is very slim and doesn't do much
> >>>>
> >>>> but I'll happily change it back to put the bits in xxx_probe() instead.
> >>>
> >>> Thanks.
> >>>
> >>> That way you shouldn't be changing behaviour.
> >>>
> >>> It may still be the case that some power off routines don't actually work until
> >>> later, but that's an existing problem. Some power off routines *do* work before
> >>> setup_arch(), so they will continue to work.
> >>
> >> Ok, works for me :). Just wanted to make sure you're aware of the
> >> reasoning why I didn't do it in probe().
> >>
> >>> Also, how does your series interact with Guenter's that removes pm_power_off ?
> >>> It seems at the moment they are unaware of each other.
> >>
> >> Guenters patches convert users of pm_power_off to his new scheme. We're
> >> not even at that stage at all yet in the powerpc tree. Converting
> >> everything to pm_power_off is basically a first step. His patch set
> >> maintains pm_power_off, so there shouldn't be nasty conflicts.
> >>
> > Onlly the first m68k patch, though. The very last patch in the series
> > remvoes pm_power_off.
> 
> And there go my patch reading skills ... :).
> 
> For which window are you targeting this? 3.18 or 3.19? If you're trying
> to hit 3.18, I can easily wait with my patch set and base it on top of
> yours.
> 
Definitely 3.19; this is way too late for 3.18 and will need some time
to mature in -next.

I can merge with your code once it is ready to go and you can make an
immutable branch available.

Thanks,
Guenter