| Message ID | 1473189119-29458-3-git-send-email-afleming@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Scott Wood |
| Headers | show |
On 09/06/2016 02:12 PM, Andy Fleming wrote: > Boards can implement power and reset functionality over gpio using > these drivers: > drivers/power/reset/gpio-poweroff.c > drivers/power/reset/gpio-restart.c > > While not all corenet boards use gpio for power/reset, this > support can be added without interfering with boards that do not > use this functionality. > > If a board's device tree has the related nodes, they are now probed. > Also, gpio-poweroff uses the global pm_power_off callback to implement > the shutdown. However, pm_power_off was not invoked when the kernel > halted, although that is usually the desired behavior. If the board > provides gpio power and reset support, it is reasonable to assume that > halting should also power down the system, unless it has chosen to > pass those calls on to hypervisor. Halt and poweroff are not the same thing. If userspace requests a poweroff, then kernel_power_off() will call machine_power_off() which will call pm_power_off(). Why do we need anything corenet-specific here? > @@ -127,6 +137,12 @@ static const struct of_device_id of_device_ids[] = { > { > .name = "handles", > }, > + { > + .name = "gpio-poweroff", > + }, > + { > + .name = "gpio-restart", > + }, > {} > }; > I don't see any other platforms doing this. How do the nodes get probed for them? -Scott
On Tuesday, September 6, 2016, Scott Wood <scott.wood@nxp.com> wrote: > On 09/06/2016 02:12 PM, Andy Fleming wrote: > > Boards can implement power and reset functionality over gpio using > > these drivers: > > drivers/power/reset/gpio-poweroff.c > > drivers/power/reset/gpio-restart.c > > > > While not all corenet boards use gpio for power/reset, this > > support can be added without interfering with boards that do not > > use this functionality. > > > > If a board's device tree has the related nodes, they are now probed. > > Also, gpio-poweroff uses the global pm_power_off callback to implement > > the shutdown. However, pm_power_off was not invoked when the kernel > > halted, although that is usually the desired behavior. If the board > > provides gpio power and reset support, it is reasonable to assume that > > halting should also power down the system, unless it has chosen to > > pass those calls on to hypervisor. > > Halt and poweroff are not the same thing. If userspace requests a > poweroff, then kernel_power_off() will call machine_power_off() which > will call pm_power_off(). > > Why do we need anything corenet-specific here? > > We don't, but then the board will halt instead of power off when you type > shutdown -h now. Or if you type poweroff without a high enough run level, > apparently. I'm amenable to removing the halt code, but there are concerns > that this will cause the systems to behave unintentionally as intended, in > that most systems that users interact with shut down when you call shutdown > -h now. There may be scripts that depend on that behavior (or at least > assume it). Perhaps I could add a config option? > CONFIG_TREAT_HALT_AS_POWEROFF? CONFIG_POWEROFF_ON_HALT? > I'll note that there is one other board that is doing the same thing, but > they've implemented their own gpio poweroff driver. See > commit 5611fe48c545a22e62273428ed74c9bfae4a9406. That commit also points > vaguely in the direction of an answer to your second question... > > > @@ -127,6 +137,12 @@ static const struct of_device_id of_device_ids[] = { > > { > > .name = "handles", > > }, > > + { > > + .name = "gpio-poweroff", > > + }, > > + { > > + .name = "gpio-restart", > > + }, > > {} > > }; > > > > I don't see any other platforms doing this. How do the nodes get probed > for them? > The answer is I don't know, but this is a common issue with adding new > devices to the device tree in embedded powerpc. The only other platforms > which have gpio-poweroff nodes in their trees are in arch/arm, and none of > those platforms call the probing function of_platform_bus_probe. I suspect > they either probe every root node, or they somehow construct the match_id. > As noted in the above-referenced commit, putting the nodes under the gpio > bus does not cause them to get probed. This seemed like the best way under > the current corenet code. > > -Scott > >
On 09/10/2016 05:05 PM, Andy Fleming wrote: > > > On Tuesday, September 6, 2016, Scott Wood <scott.wood@nxp.com > <mailto:scott.wood@nxp.com>> wrote: > > On 09/06/2016 02:12 PM, Andy Fleming wrote: > > Boards can implement power and reset functionality over gpio using > > these drivers: > > drivers/power/reset/gpio-poweroff.c > > drivers/power/reset/gpio-restart.c > > > > While not all corenet boards use gpio for power/reset, this > > support can be added without interfering with boards that do not > > use this functionality. > > > > If a board's device tree has the related nodes, they are now probed. > > Also, gpio-poweroff uses the global pm_power_off callback to implement > > the shutdown. However, pm_power_off was not invoked when the kernel > > halted, although that is usually the desired behavior. If the board > > provides gpio power and reset support, it is reasonable to assume that > > halting should also power down the system, unless it has chosen to > > pass those calls on to hypervisor. > > Halt and poweroff are not the same thing. If userspace requests a > poweroff, then kernel_power_off() will call machine_power_off() which > will call pm_power_off(). > > Why do we need anything corenet-specific here? > > > > We don't, but then the board will halt instead of power off when you > type shutdown -h now. Isn't that what it's supposed to do? If you want poweroff then ask for poweroff. > Or if you type poweroff without a high enough > run level, apparently. Hmm? In any case, run levels have nothing to do with the kernel. The kernel implements LINUX_REBOOT_CMD_HALT and LINUX_REBOOT_CMD_POWER_OFF, and they should do what they're advertised to do. > I'm amenable to removing the halt code, but > there are concerns that this will cause the systems to behave > unintentionally as intended, in that most systems that users > interact with shut down when you call shutdown -h now. There may be > scripts that depend on that behavior (or at least assume it). When did the behavior you're seeking ever exist? > I don't see any other platforms doing this. How do the nodes get probed > for them? > > > The answer is I don't know, but this is a common issue with adding > new devices to the device tree in embedded powerpc. The only other > platforms which have gpio-poweroff nodes in their trees are in > arch/arm, and none of those platforms call the probing > function of_platform_bus_probe. I suspect they either probe every > root node, or they somehow construct the match_id. As noted in the > above-referenced commit, putting the nodes under the gpio bus does > not cause them to get probed. This seemed like the best way under > the current corenet code. Well, let's figure out what it is that PPC should be doing to have things work the way it does on ARM. -Scott
> On Sep 12, 2016, at 23:47, Scott Wood <scott.wood@nxp.com> wrote: > >> On 09/10/2016 05:05 PM, Andy Fleming wrote: >> >> >> On Tuesday, September 6, 2016, Scott Wood <scott.wood@nxp.com >> <mailto:scott.wood@nxp.com>> wrote: >> >>> On 09/06/2016 02:12 PM, Andy Fleming wrote: >>> Boards can implement power and reset functionality over gpio using >>> these drivers: >>> drivers/power/reset/gpio-poweroff.c >>> drivers/power/reset/gpio-restart.c >>> >>> While not all corenet boards use gpio for power/reset, this >>> support can be added without interfering with boards that do not >>> use this functionality. >>> >>> If a board's device tree has the related nodes, they are now probed. >>> Also, gpio-poweroff uses the global pm_power_off callback to implement >>> the shutdown. However, pm_power_off was not invoked when the kernel >>> halted, although that is usually the desired behavior. If the board >>> provides gpio power and reset support, it is reasonable to assume that >>> halting should also power down the system, unless it has chosen to >>> pass those calls on to hypervisor. >> >> Halt and poweroff are not the same thing. If userspace requests a >> poweroff, then kernel_power_off() will call machine_power_off() which >> will call pm_power_off(). >> >> Why do we need anything corenet-specific here? >> >> >> >> We don't, but then the board will halt instead of power off when you >> type shutdown -h now. > > Isn't that what it's supposed to do? If you want poweroff then ask for > poweroff. > >> Or if you type poweroff without a high enough >> run level, apparently. > > Hmm? > > In any case, run levels have nothing to do with the kernel. The kernel > implements LINUX_REBOOT_CMD_HALT and LINUX_REBOOT_CMD_POWER_OFF, and > they should do what they're advertised to do. > >> I'm amenable to removing the halt code, but >> there are concerns that this will cause the systems to behave >> unintentionally as intended, in that most systems that users >> interact with shut down when you call shutdown -h now. There may be >> scripts that depend on that behavior (or at least assume it). > > When did the behavior you're seeking ever exist? Well, for one, on that Servergy board. I agree that halt and power off mean and have always meant different things to the kernel. The problem is that most desktop systems, having halted, pass control to the BIOS which--usually--shuts off the power. Am I wrong about this? I've been using shutdown -h now to turn off my Linux systems for nearly 2 decades now, but I admit that I don't do it often, and I tend to stick with whatever works. > >> I don't see any other platforms doing this. How do the nodes get probed >> for them? >> >> >> The answer is I don't know, but this is a common issue with adding >> new devices to the device tree in embedded powerpc. The only other >> platforms which have gpio-poweroff nodes in their trees are in >> arch/arm, and none of those platforms call the probing >> function of_platform_bus_probe. I suspect they either probe every >> root node, or they somehow construct the match_id. As noted in the >> above-referenced commit, putting the nodes under the gpio bus does >> not cause them to get probed. This seemed like the best way under >> the current corenet code. > > Well, let's figure out what it is that PPC should be doing to have > things work the way it does on ARM. For all of the devices? Or just these two? Andy
On 09/15/2016 03:03 AM, Andy Fleming wrote: > I agree that halt and power off mean and have always meant different > things to the kernel. The problem is that most desktop systems, > having halted, pass control to the BIOS which--usually--shuts off the > power. Am I wrong about this? I've been using shutdown -h now to turn > off my Linux systems for nearly 2 decades now, but I admit that I > don't do it often, and I tend to stick with whatever works. From the shutdown man page: -h Equivalent to --poweroff, unless --halt is specified. Again, let's talk in terms of the kernel API rather than quirky userspace commands. FWIW, I've always used "halt -p" and recall the system not powering off on PCs when I use plain "halt", though it's been many years since I've tried. >>> I don't see any other platforms doing this. How do the nodes get probed >>> for them? >>> >>> >>> The answer is I don't know, but this is a common issue with adding >>> new devices to the device tree in embedded powerpc. The only other >>> platforms which have gpio-poweroff nodes in their trees are in >>> arch/arm, and none of those platforms call the probing >>> function of_platform_bus_probe. I suspect they either probe every >>> root node, or they somehow construct the match_id. As noted in the >>> above-referenced commit, putting the nodes under the gpio bus does >>> not cause them to get probed. This seemed like the best way under >>> the current corenet code. >> >> Well, let's figure out what it is that PPC should be doing to have >> things work the way it does on ARM. > > For all of the devices? Or just these two? All of them. If ARM isn't maintaining these annoying lists why should we have to? :-P -Scott
On 09/16/2016 04:05 PM, Scott Wood wrote: >>>> I don't see any other platforms doing this. How do the nodes get probed >>>> for them? >>>> >>>> >>>> The answer is I don't know, but this is a common issue with adding >>>> new devices to the device tree in embedded powerpc. The only other >>>> platforms which have gpio-poweroff nodes in their trees are in >>>> arch/arm, and none of those platforms call the probing >>>> function of_platform_bus_probe. I suspect they either probe every >>>> root node, or they somehow construct the match_id. As noted in the >>>> above-referenced commit, putting the nodes under the gpio bus does >>>> not cause them to get probed. This seemed like the best way under >>>> the current corenet code. >>> >>> Well, let's figure out what it is that PPC should be doing to have >>> things work the way it does on ARM. >> >> For all of the devices? Or just these two? > > All of them. If ARM isn't maintaining these annoying lists why should > we have to? :-P The answer seems to be using of_platform_populate() rather than of_platform_bus_probe(). -Scott
diff --git a/arch/powerpc/platforms/85xx/corenet_generic.c b/arch/powerpc/platforms/85xx/corenet_generic.c index 3a6a84f..17b8ebb 100644 --- a/arch/powerpc/platforms/85xx/corenet_generic.c +++ b/arch/powerpc/platforms/85xx/corenet_generic.c @@ -59,6 +59,16 @@ void __init corenet_gen_pic_init(void) } } +/* If someone has registered a poweroff callback, invoke it */ +static void __noreturn corenet_generic_halt(void) +{ + if (pm_power_off) + pm_power_off(); + + /* Should not return */ + for(;;); +} + /* * Setup the architecture */ @@ -127,6 +137,12 @@ static const struct of_device_id of_device_ids[] = { { .name = "handles", }, + { + .name = "gpio-poweroff", + }, + { + .name = "gpio-restart", + }, {} }; @@ -176,6 +192,8 @@ static int __init corenet_generic_probe(void) extern struct smp_ops_t smp_85xx_ops; #endif + ppc_md.halt = corenet_generic_halt; + if (of_device_compatible_match(of_root, boards)) return 1;
Boards can implement power and reset functionality over gpio using these drivers: drivers/power/reset/gpio-poweroff.c drivers/power/reset/gpio-restart.c While not all corenet boards use gpio for power/reset, this support can be added without interfering with boards that do not use this functionality. If a board's device tree has the related nodes, they are now probed. Also, gpio-poweroff uses the global pm_power_off callback to implement the shutdown. However, pm_power_off was not invoked when the kernel halted, although that is usually the desired behavior. If the board provides gpio power and reset support, it is reasonable to assume that halting should also power down the system, unless it has chosen to pass those calls on to hypervisor. Signed-off-by: Andy Fleming <afleming@gmail.com> --- arch/powerpc/platforms/85xx/corenet_generic.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)