Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1562675 > unrolled thread

Re: [PATCH] power: reset: Add MAX77620 support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] power: reset: Add MAX77620 support Thierry Reding <thierry.reding@gmail.com> - 2017-01-19 13:30 +0100
    Re: [PATCH] power: reset: Add MAX77620 support Sebastian Reichel <sre@kernel.org> - 2017-01-20 00:20 +0100
      Re: [PATCH] power: reset: Add MAX77620 support Guenter Roeck <linux@roeck-us.net> - 2017-01-20 00:30 +0100
        Re: [PATCH] power: reset: Add MAX77620 support Thierry Reding <thierry.reding@gmail.com> - 2017-01-20 09:40 +0100
          Re: [PATCH] power: reset: Add MAX77620 support Guenter Roeck <linux@roeck-us.net> - 2017-01-20 19:00 +0100
          Re: [PATCH] power: reset: Add MAX77620 support Sebastian Reichel <sre@kernel.org> - 2017-01-29 21:20 +0100
            Re: [PATCH] power: reset: Add MAX77620 support Guenter Roeck <linux@roeck-us.net> - 2017-01-29 21:50 +0100
              Re: [PATCH] power: reset: Add MAX77620 support Sebastian Reichel <sre@kernel.org> - 2017-01-30 00:00 +0100
        Re: [PATCH] power: reset: Add MAX77620 support Sebastian Reichel <sre@kernel.org> - 2017-01-20 13:50 +0100
          Re: [PATCH] power: reset: Add MAX77620 support Thierry Reding <thierry.reding@gmail.com> - 2017-01-20 14:30 +0100
            Re: [PATCH] power: reset: Add MAX77620 support Sebastian Reichel <sre@kernel.org> - 2017-01-20 14:50 +0100
            Re: [PATCH] power: reset: Add MAX77620 support Guenter Roeck <linux@roeck-us.net> - 2017-01-20 15:40 +0100

#1562675 — Re: [PATCH] power: reset: Add MAX77620 support

[Multipart message — attachments visible in raw view] — view raw

On Fri, Jan 13, 2017 at 04:44:25AM +0100, Sebastian Reichel wrote:
> On Thu, Jan 12, 2017 at 05:15:07PM +0100, Thierry Reding wrote:
[...]
> > +	if (!of_property_read_bool(np, "system-power-controller"))
> > +		return 0;
> > +
> > +	power = devm_kzalloc(&pdev->dev, sizeof(*power), GFP_KERNEL);
> > +	if (!power)
> > +		return -ENOMEM;
> > +
> > +	power->regmap = max77620->rmap;
> > +	power->dev = &pdev->dev;
> > +
> > +	err = regmap_read(power->regmap, MAX77620_REG_NVERC, &value);
> > +	if (err < 0) {
> > +		dev_err(power->dev, "failed to read event recorder: %d\n", err);
> > +		return err;
> > +	}
> > +
> > +	dev_dbg(&pdev->dev, "event recorder: %#x\n", value);
> > +
> > +	system_power_controller = power;
> > +	pm_power_off = max77620_pm_power_off;
> 
> > +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
> > +	arm_pm_restart = max77620_pm_restart;
> > +#endif
> 
> Please use register_restart_handler() instead. It has support for
> priorities, is not arm specific and properly supports unregistering
> (some handler with lower priority will take over). You can check
> gpio-restart as an example for the API.
> 
> If you have some other arm_pm_restart handler (those are tried first)
> I suggest to convert that to the restart handler framework. Actually
> it may be a good idea to convert all of them and drop arm_pm_restart,
> since there are only 4 left:
> 
> $ git grep "arm_pm_restart ="
> drivers/firmware/psci.c:        arm_pm_restart = psci_sys_reset;
> arch/arm/mach-prima2/rstc.c:    arm_pm_restart = sirfsoc_restart;
> arch/arm/xen/enlighten.c:       arm_pm_restart = xen_restart;
> arch/arm/kernel/setup.c:                arm_pm_restart = mdesc->restart;
> 
> The first 3 should be easy to update and the last one could
> be solved by registering a wrapper function as restart handler,
> which calls mdesc->restart().

I remember this not being the first time that this confuses me. And from
looking around a little it seems like I'm not the only one.

Do you know if there's ever been any attempts to formalize all of this
by adding some sort of framework for this? That way some of the
confusion may be removed and architecture-specific bits could be
eliminated more easily.

Thierry

[toc] | [next] | [standalone]

#1563188

[Multipart message — attachments visible in raw view] — view raw

Hi Thierry,

> > > [...]
> >
> > Please use register_restart_handler() instead. It has support for
> > priorities, is not arm specific and properly supports unregistering
> > (some handler with lower priority will take over). You can check
> > gpio-restart as an example for the API.
> > 
> > If you have some other arm_pm_restart handler (those are tried first)
> > I suggest to convert that to the restart handler framework. Actually
> > it may be a good idea to convert all of them and drop arm_pm_restart,
> > since there are only 4 left:
> > 
> > $ git grep "arm_pm_restart ="
> > drivers/firmware/psci.c:        arm_pm_restart = psci_sys_reset;
> > arch/arm/mach-prima2/rstc.c:    arm_pm_restart = sirfsoc_restart;
> > arch/arm/xen/enlighten.c:       arm_pm_restart = xen_restart;
> > arch/arm/kernel/setup.c:                arm_pm_restart = mdesc->restart;
> > 
> > The first 3 should be easy to update and the last one could
> > be solved by registering a wrapper function as restart handler,
> > which calls mdesc->restart().
> 
> I remember this not being the first time that this confuses me. And from
> looking around a little it seems like I'm not the only one.
> 
> Do you know if there's ever been any attempts to formalize all of this
> by adding some sort of framework for this? That way some of the
> confusion may be removed and architecture-specific bits could be
> eliminated more easily.

We have a framework for restart handlers, which has been written
by Guenter Roeck [0] in 2014. You just have to call register_restart_handler()
during probe and unregister_restart_handler() during removal of
your reboot driver. All reboot drivers in drivers/power/reset use
that framework.

The restart handlers are invoked by calling do_kernel_restart()
based on their configured priority. That function is called by
the architecture's machine_restart() function. It's currently
used by mips, ppc, arm & arm64 as far as I can see. ARM's
implementation looks like this:

if (arm_pm_restart)
	arm_pm_restart()
else
	do_kernel_restart() /* reboot handler framework */

No such thing exists for poweroff. Guenter also wrote something for
that [1], but Linus intervened [2]. Anyways, pm_power_off is at
least architecture independent.

[0] https://lwn.net/Articles/605298/
[1] https://lwn.net/Articles/617345/
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/351369.html

-- Sebastian

[toc] | [prev] | [next] | [standalone]

#1563194

On Fri, Jan 20, 2017 at 12:00:36AM +0100, Sebastian Reichel wrote:
> Hi Thierry,
> 
> > > > [...]
> > >
> > > Please use register_restart_handler() instead. It has support for
> > > priorities, is not arm specific and properly supports unregistering
> > > (some handler with lower priority will take over). You can check
> > > gpio-restart as an example for the API.
> > > 
> > > If you have some other arm_pm_restart handler (those are tried first)
> > > I suggest to convert that to the restart handler framework. Actually
> > > it may be a good idea to convert all of them and drop arm_pm_restart,
> > > since there are only 4 left:
> > > 
> > > $ git grep "arm_pm_restart ="
> > > drivers/firmware/psci.c:        arm_pm_restart = psci_sys_reset;
> > > arch/arm/mach-prima2/rstc.c:    arm_pm_restart = sirfsoc_restart;
> > > arch/arm/xen/enlighten.c:       arm_pm_restart = xen_restart;
> > > arch/arm/kernel/setup.c:                arm_pm_restart = mdesc->restart;
> > > 
> > > The first 3 should be easy to update and the last one could
> > > be solved by registering a wrapper function as restart handler,
> > > which calls mdesc->restart().
> > 
> > I remember this not being the first time that this confuses me. And from
> > looking around a little it seems like I'm not the only one.
> > 
> > Do you know if there's ever been any attempts to formalize all of this
> > by adding some sort of framework for this? That way some of the
> > confusion may be removed and architecture-specific bits could be
> > eliminated more easily.
> 
> We have a framework for restart handlers, which has been written
> by Guenter Roeck [0] in 2014. You just have to call register_restart_handler()
> during probe and unregister_restart_handler() during removal of
> your reboot driver. All reboot drivers in drivers/power/reset use
> that framework.
> 
> The restart handlers are invoked by calling do_kernel_restart()
> based on their configured priority. That function is called by
> the architecture's machine_restart() function. It's currently
> used by mips, ppc, arm & arm64 as far as I can see. ARM's
> implementation looks like this:
> 
> if (arm_pm_restart)
> 	arm_pm_restart()
> else
> 	do_kernel_restart() /* reboot handler framework */
> 
I actually have a set of patches somewhere which transforms the remaining
direct users of arm_pm_restart to use the framework (unless I removed it
from my trees - I don't really recall). I just never got around to submit
it, and then [2] happened and I lost interest.

> No such thing exists for poweroff. Guenter also wrote something for
> that [1], but Linus intervened [2]. Anyways, pm_power_off is at
> least architecture independent.
> 
That was by far the most frustrating kernel development experience
I ever head :-(.

Guenter

> [0] https://lwn.net/Articles/605298/
> [1] https://lwn.net/Articles/617345/
> [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/351369.html
> 
> -- Sebastian

[toc] | [prev] | [next] | [standalone]

#1563383

[Multipart message — attachments visible in raw view] — view raw

On Thu, Jan 19, 2017 at 03:29:34PM -0800, Guenter Roeck wrote:
> On Fri, Jan 20, 2017 at 12:00:36AM +0100, Sebastian Reichel wrote:
> > Hi Thierry,
> > 
> > > > > [...]
> > > >
> > > > Please use register_restart_handler() instead. It has support for
> > > > priorities, is not arm specific and properly supports unregistering
> > > > (some handler with lower priority will take over). You can check
> > > > gpio-restart as an example for the API.
> > > > 
> > > > If you have some other arm_pm_restart handler (those are tried first)
> > > > I suggest to convert that to the restart handler framework. Actually
> > > > it may be a good idea to convert all of them and drop arm_pm_restart,
> > > > since there are only 4 left:
> > > > 
> > > > $ git grep "arm_pm_restart ="
> > > > drivers/firmware/psci.c:        arm_pm_restart = psci_sys_reset;
> > > > arch/arm/mach-prima2/rstc.c:    arm_pm_restart = sirfsoc_restart;
> > > > arch/arm/xen/enlighten.c:       arm_pm_restart = xen_restart;
> > > > arch/arm/kernel/setup.c:                arm_pm_restart = mdesc->restart;
> > > > 
> > > > The first 3 should be easy to update and the last one could
> > > > be solved by registering a wrapper function as restart handler,
> > > > which calls mdesc->restart().
> > > 
> > > I remember this not being the first time that this confuses me. And from
> > > looking around a little it seems like I'm not the only one.
> > > 
> > > Do you know if there's ever been any attempts to formalize all of this
> > > by adding some sort of framework for this? That way some of the
> > > confusion may be removed and architecture-specific bits could be
> > > eliminated more easily.
> > 
> > We have a framework for restart handlers, which has been written
> > by Guenter Roeck [0] in 2014. You just have to call register_restart_handler()
> > during probe and unregister_restart_handler() during removal of
> > your reboot driver. All reboot drivers in drivers/power/reset use
> > that framework.
> > 
> > The restart handlers are invoked by calling do_kernel_restart()
> > based on their configured priority. That function is called by
> > the architecture's machine_restart() function. It's currently
> > used by mips, ppc, arm & arm64 as far as I can see. ARM's
> > implementation looks like this:
> > 
> > if (arm_pm_restart)
> > 	arm_pm_restart()
> > else
> > 	do_kernel_restart() /* reboot handler framework */
> > 
> I actually have a set of patches somewhere which transforms the remaining
> direct users of arm_pm_restart to use the framework (unless I removed it
> from my trees - I don't really recall). I just never got around to submit
> it, and then [2] happened and I lost interest.
> 
> > No such thing exists for poweroff. Guenter also wrote something for
> > that [1], but Linus intervened [2]. Anyways, pm_power_off is at
> > least architecture independent.
> > 
> That was by far the most frustrating kernel development experience
> I ever head :-(.

It was a little difficult to track down all the discussion around this,
but reading through what I could find, I can understand why this would
have been frustrating. You obviously spent an enormous amount of work
only to get it shot down.

I have to say that I have similar concerns to those voiced by Linus and
Rafael. Notifier chains and priority seem too little restrictive for
this kind of functionality, in my opinion. I think those concerns could
be removed if things got formalized using a framework.

Without having spent the amount of time on the topic that you have, the
following straw-man proposal is perhaps a little naive:

	struct system_power_controller;

	struct system_power_ops {
		int (*power_off)(struct system_power_controller *spc);
		int (*restart)(struct system_power_controller *spc,
			       enum reboot_mode mode,
			       const char *cmd);
	};

	struct system_power_controller {
		struct device *dev;
		const struct system_power_ops *ops;
		struct list_head list;
		...
	};

	int system_power_controller_add(struct system_power_controller *spc);
	void system_power_controller_remove(struct system_power_controller *spc);

	int system_power_off(void);
	int system_restart(enum reboot_mode mode, const char *cmd);

The above is based on what other subsystems use. Drivers would embed the
struct system_power_controller in a driver-specific data structure and
use container_of() to get at that from the callbacks.

Controllers can be added and removed to the subsystem. Internally it may
be worth to keep all of the controllers in a list, but only use the most
appropriate ones. The above would need some sort of flag or type list
that drivers can set in addition to operations to indicate what your
proposal had done via priorities. I'm thinking of something along the
lines of:

	enum system_power_controller_type {
		SYSTEM_POWER_CONTROLLER_TYPE_CPU,
		SYSTEM_POWER_CONTROLLER_TYPE_SOC,
		SYSTEM_POWER_CONTROLLER_TYPE_BOARD,
	};

	struct system_power_controller {
		...
		enum system_power_controller_type type;
		...
	};

There's a fairly natural ordering in the above "levels". Obviously the
board-level controller would be highest priority because it controls
power or reset of the complete board. This would likely be implemented
by GPIO-based or PMIC type of drivers. SoC-level controllers would use
mechanisms provided by the SoC in order to power off or reset only the
SoC. This is obviously lower priority than board-level because some of
the components on the board may end up remaining powered on or not
getting reset. But it could be a useful fallback in case a board-level
controller isn't present or fails. Finally there's the CPU-level code
that would most likely be implemented by architecture code. In the most
basic version this could be a WFI kind of instruction, or a busy loop,
or perhaps even a simple call to panic().

One complication might be that there are things like PSCI that have an
architecture-specific implementation (CPU-level) but could actually be
a board-level "controller".

To keep things simple, I think it would be okay to allow only one of
each type of controller in any running system. It's very unlikely that
board designers would devise two different ways of powering off or
restarting a system, while in a similar way an SoC or CPU would only
ever provide one way to do so. Even if theoretically multiple
possibilities exist, I think the board code should pick which ones are
appropriate.

Another missing feature in the above is that sometimes a mechanism
exists to record in persistent storage (registers, memory, ...) what
kind of reset was executed and which boot code will inspect and use to
run different paths during boot. One such example is Tegra where the
PMC has "scratch" registers that don't get reset on reboot. Software
sets some of the bits to enable things like forced-recovery mode or
Android-type recovery booting. I'm sure other similar mechanisms exist
on other SoCs. Upon board-level reset, we would still want to have the
SoC-level reset prep code execute, but not reset the SoC, otherwise
the board-level code wouldn't have a chance of running anymore. This
could possibly be solved by adding more fine-grained operations in the
struct system_power_ops.

One other possible solution that comes to mind is that system power
controllers could be explicitly stacked. This would be complicated to
achieve in code, but I'm thinking it could have interesting use-cases
in device tree based systems, for example. I'm just brainstorming here,
this may not be a good idea at all:

	pmc: pmc@... {
		system-power-controller;
	};

	i2c@... {
		pmic: pmic@... {
			...
			system-power-parent = <&pmc>;
			...
		};
	};

The PMIC would in the above call into PMC in order to reset on a SoC
level before performing the board-level reset. I suppose it could use
a separate callback (->prepare({RESET,POWER})) instead of the usual
->restart() and ->power_off().

Does the above sound at all sane to you, given your broad experience
in this field? Anything I missed that I'd need to consider in a design
for such a framework?

Sebastian, any thoughts from you on the above?

Thierry

[toc] | [prev] | [next] | [standalone]

#1563828

On Fri, Jan 20, 2017 at 09:38:04AM +0100, Thierry Reding wrote:
> > 
> > > No such thing exists for poweroff. Guenter also wrote something for
> > > that [1], but Linus intervened [2]. Anyways, pm_power_off is at
> > > least architecture independent.
> > > 
> > That was by far the most frustrating kernel development experience
> > I ever head :-(.
> 
> It was a little difficult to track down all the discussion around this,
> but reading through what I could find, I can understand why this would
> have been frustrating. You obviously spent an enormous amount of work
> only to get it shot down.
> 
> I have to say that I have similar concerns to those voiced by Linus and
> Rafael. Notifier chains and priority seem too little restrictive for
> this kind of functionality, in my opinion. I think those concerns could
> be removed if things got formalized using a framework.
> 

Exactly the same argument applies to the restart handler. I am just glad
I got it in before the objections came up.

I do understand the concerns, which is why I tried to accommodate them
and come up with another solution. However, it boils down to the following:

- Priorities are needed, no matter how they are called.
- Data structures have to be allocated statically, since the code
  still needs to work if the system is out of memory.
- Data structurs need to be maintained in a prioritized list or similar,
  to be able to handle insertions and removals at will, and to be able
  to determine which function should be called first.

So ultimately, no matter what you do, you'll end up re-implementing
parts of the notifier code.

To me the argument, ultimately, boils down to arguing with Amazon that they
should always use the smallest possible packaging (and not ship flash drives
in such a large box that it is hard to find). Just like Amazon has good reasons
to standardize on a limited number of box sizes, an argument can be made to
have only a limited amount of infrastructure code. If some infrastructure code
is "too little restrictive", but does the job, so be it. That should not be
a reason to have separate and highly specialized infrastructure for everything,
just to make it a 100% fit.

That is, however, my personal opinion. If people want to have and implement
a large amount of highly specialized infrastructure instead of more generic
(and less restrictive) infrastructure, so be it. Just don't expect _me_
to implement it.

> Without having spent the amount of time on the topic that you have, the
> following straw-man proposal is perhaps a little naive:
> 
> 	struct system_power_controller;
> 
> 	struct system_power_ops {
> 		int (*power_off)(struct system_power_controller *spc);
> 		int (*restart)(struct system_power_controller *spc,
> 			       enum reboot_mode mode,
> 			       const char *cmd);
> 	};
> 
> 	struct system_power_controller {
> 		struct device *dev;
> 		const struct system_power_ops *ops;
> 		struct list_head list;
> 		...
> 	};
> 
> 	int system_power_controller_add(struct system_power_controller *spc);
> 	void system_power_controller_remove(struct system_power_controller *spc);
> 
> 	int system_power_off(void);
> 	int system_restart(enum reboot_mode mode, const char *cmd);
> 
It seems to me that restart and power off are inherently independent of each
other. I would personally not try to unify them. Having said that, that is my
personal opinion only. I would for sure not object to anything in this area
(nor get involved - once was enough ;-).

Thanks,
Guenter

[toc] | [prev] | [next] | [standalone]

#1569275

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Fri, Jan 20, 2017 at 09:38:04AM +0100, Thierry Reding wrote:
> On Thu, Jan 19, 2017 at 03:29:34PM -0800, Guenter Roeck wrote:
> > On Fri, Jan 20, 2017 at 12:00:36AM +0100, Sebastian Reichel wrote:
> > > Hi Thierry,
> > > 
> > > > > > [...]
> > > > >
> > > > > Please use register_restart_handler() instead. It has support for
> > > > > priorities, is not arm specific and properly supports unregistering
> > > > > (some handler with lower priority will take over). You can check
> > > > > gpio-restart as an example for the API.
> > > > > 
> > > > > If you have some other arm_pm_restart handler (those are tried first)
> > > > > I suggest to convert that to the restart handler framework. Actually
> > > > > it may be a good idea to convert all of them and drop arm_pm_restart,
> > > > > since there are only 4 left:
> > > > > 
> > > > > $ git grep "arm_pm_restart ="
> > > > > drivers/firmware/psci.c:        arm_pm_restart = psci_sys_reset;
> > > > > arch/arm/mach-prima2/rstc.c:    arm_pm_restart = sirfsoc_restart;
> > > > > arch/arm/xen/enlighten.c:       arm_pm_restart = xen_restart;
> > > > > arch/arm/kernel/setup.c:                arm_pm_restart = mdesc->restart;
> > > > > 
> > > > > The first 3 should be easy to update and the last one could
> > > > > be solved by registering a wrapper function as restart handler,
> > > > > which calls mdesc->restart().
> > > > 
> > > > I remember this not being the first time that this confuses me. And from
> > > > looking around a little it seems like I'm not the only one.
> > > > 
> > > > Do you know if there's ever been any attempts to formalize all of this
> > > > by adding some sort of framework for this? That way some of the
> > > > confusion may be removed and architecture-specific bits could be
> > > > eliminated more easily.
> > > 
> > > We have a framework for restart handlers, which has been written
> > > by Guenter Roeck [0] in 2014. You just have to call register_restart_handler()
> > > during probe and unregister_restart_handler() during removal of
> > > your reboot driver. All reboot drivers in drivers/power/reset use
> > > that framework.
> > > 
> > > The restart handlers are invoked by calling do_kernel_restart()
> > > based on their configured priority. That function is called by
> > > the architecture's machine_restart() function. It's currently
> > > used by mips, ppc, arm & arm64 as far as I can see. ARM's
> > > implementation looks like this:
> > > 
> > > if (arm_pm_restart)
> > > 	arm_pm_restart()
> > > else
> > > 	do_kernel_restart() /* reboot handler framework */
> > > 
> > I actually have a set of patches somewhere which transforms the remaining
> > direct users of arm_pm_restart to use the framework (unless I removed it
> > from my trees - I don't really recall). I just never got around to submit
> > it, and then [2] happened and I lost interest.
> > 
> > > No such thing exists for poweroff. Guenter also wrote something for
> > > that [1], but Linus intervened [2]. Anyways, pm_power_off is at
> > > least architecture independent.
> > > 
> > That was by far the most frustrating kernel development experience
> > I ever head :-(.
> 
> It was a little difficult to track down all the discussion around this,
> but reading through what I could find, I can understand why this would
> have been frustrating. You obviously spent an enormous amount of work
> only to get it shot down.
> 
> I have to say that I have similar concerns to those voiced by Linus and
> Rafael. Notifier chains and priority seem too little restrictive for
> this kind of functionality, in my opinion. I think those concerns could
> be removed if things got formalized using a framework.
> 
> Without having spent the amount of time on the topic that you have, the
> following straw-man proposal is perhaps a little naive:
> 
> 	struct system_power_controller;
> 
> 	struct system_power_ops {
> 		int (*power_off)(struct system_power_controller *spc);
> 		int (*restart)(struct system_power_controller *spc,
> 			       enum reboot_mode mode,
> 			       const char *cmd);
> 	};
> 
> 	struct system_power_controller {
> 		struct device *dev;
> 		const struct system_power_ops *ops;
> 		struct list_head list;
> 		...
> 	};
> 
> 	int system_power_controller_add(struct system_power_controller *spc);
> 	void system_power_controller_remove(struct system_power_controller *spc);
> 
> 	int system_power_off(void);
> 	int system_restart(enum reboot_mode mode, const char *cmd);
> 
> The above is based on what other subsystems use. Drivers would embed the
> struct system_power_controller in a driver-specific data structure and
> use container_of() to get at that from the callbacks.
> 
> Controllers can be added and removed to the subsystem. Internally it may
> be worth to keep all of the controllers in a list, but only use the most
> appropriate ones. The above would need some sort of flag or type list
> that drivers can set in addition to operations to indicate what your
> proposal had done via priorities. I'm thinking of something along the
> lines of:
> 
> 	enum system_power_controller_type {
> 		SYSTEM_POWER_CONTROLLER_TYPE_CPU,
> 		SYSTEM_POWER_CONTROLLER_TYPE_SOC,
> 		SYSTEM_POWER_CONTROLLER_TYPE_BOARD,
> 	};
> 
> 	struct system_power_controller {
> 		...
> 		enum system_power_controller_type type;
> 		...
> 	};
> 
> There's a fairly natural ordering in the above "levels". Obviously the
> board-level controller would be highest priority because it controls
> power or reset of the complete board. This would likely be implemented
> by GPIO-based or PMIC type of drivers. SoC-level controllers would use
> mechanisms provided by the SoC in order to power off or reset only the
> SoC. This is obviously lower priority than board-level because some of
> the components on the board may end up remaining powered on or not
> getting reset. But it could be a useful fallback in case a board-level
> controller isn't present or fails. Finally there's the CPU-level code
> that would most likely be implemented by architecture code. In the most
> basic version this could be a WFI kind of instruction, or a busy loop,
> or perhaps even a simple call to panic().
> 
> One complication might be that there are things like PSCI that have an
> architecture-specific implementation (CPU-level) but could actually be
> a board-level "controller".
> 
> To keep things simple, I think it would be okay to allow only one of
> each type of controller in any running system. It's very unlikely that
> board designers would devise two different ways of powering off or
> restarting a system, while in a similar way an SoC or CPU would only
> ever provide one way to do so. Even if theoretically multiple
> possibilities exist, I think the board code should pick which ones are
> appropriate.

Using that logic we may also advice, that board-code should only
register the board-level reset/poweroff and it's enough to have
a callback again... I wonder if that is really feasible.

> Another missing feature in the above is that sometimes a mechanism
> exists to record in persistent storage (registers, memory, ...) what
> kind of reset was executed and which boot code will inspect and use to
> run different paths during boot. One such example is Tegra where the
> PMC has "scratch" registers that don't get reset on reboot. Software
> sets some of the bits to enable things like forced-recovery mode or
> Android-type recovery booting. I'm sure other similar mechanisms exist
> on other SoCs. Upon board-level reset, we would still want to have the
> SoC-level reset prep code execute, but not reset the SoC, otherwise
> the board-level code wouldn't have a chance of running anymore. This
> could possibly be solved by adding more fine-grained operations in the
> struct system_power_ops.

This kind of hardware is currently supported via
drivers/power/reset/reboot-mode.c

> One other possible solution that comes to mind is that system power
> controllers could be explicitly stacked. This would be complicated to
> achieve in code, but I'm thinking it could have interesting use-cases
> in device tree based systems, for example. I'm just brainstorming here,
> this may not be a good idea at all:
> 
> 	pmc: pmc@... {
> 		system-power-controller;
> 	};
> 
> 	i2c@... {
> 		pmic: pmic@... {
> 			...
> 			system-power-parent = <&pmc>;
> 			...
> 		};
> 	};
> 
> The PMIC would in the above call into PMC in order to reset on a SoC
> level before performing the board-level reset. I suppose it could use
> a separate callback (->prepare({RESET,POWER})) instead of the usual
> ->restart() and ->power_off().

Wouldn't SoC level reset stop the execution so board is never
resetted?

> Does the above sound at all sane to you, given your broad experience
> in this field? Anything I missed that I'd need to consider in a design
> for such a framework?
> 
> Sebastian, any thoughts from you on the above?

I would accept patches implementing the above, but I'm also totally
fine with the notifier chains. IMHO it would be enough to define
something like this:

#define RESTART_PRIORITY_CPU_LEVEL 32
#define RESTART_PRIORITY_SOC_LEVEL 64
#define RESTART_PRIORITY_BOARD_LEVEL 128

-- Sebastian

[toc] | [prev] | [next] | [standalone]

#1569280

On 01/29/2017 12:02 PM, Sebastian Reichel wrote:

>>
>> To keep things simple, I think it would be okay to allow only one of
>> each type of controller in any running system. It's very unlikely that
>> board designers would devise two different ways of powering off or
>> restarting a system, while in a similar way an SoC or CPU would only
>> ever provide one way to do so. Even if theoretically multiple
>> possibilities exist, I think the board code should pick which ones are
>> appropriate.
>
> Using that logic we may also advice, that board-code should only
> register the board-level reset/poweroff and it's enough to have
> a callback again... I wonder if that is really feasible.
>

FWIW, it is also not true. There is a reason why many of the restart
handlers used to have code saying "install restart handler, but only
if none is installed yet". Which of course is racy, and gets more
interesting if the restart handler installed first is unloaded at a
later time, leaving the system with no restart handler. Or both are
unloaded, leaving the system with a pointer to a no longer existing
handler.

One could then argue that anything implementing a restart handler must
not unload. Which results in more restrictions. And drivers loaded
on hardware which don't need it. And more corner cases to deal with.
And more inconsistencies.

In reality, many systems or system variants will have more than one means
to restart it. Yes, board designers do devise multiple ways of powering off
or restarting a system. There may be and likely are valid reasons for doing
so; I would not want to claim or suggest that board designers would design
such hardware without reason. Even "standard" PCs tend to have have more
than one means to reset it. There _was_ a reason for introducing that
framework; I didn't just do it for fun.

However, as I had mentioned before, I am not really interested in this
topic anymore. Just treat this as my final word of caution, or feel free
to ignore it. I hope you'll find a much better solution than mine
to implement "the board code should pick which ones are appropriate".

Thanks,
Guenter

[toc] | [prev] | [next] | [standalone]

#1569303

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Sun, Jan 29, 2017 at 12:47:57PM -0800, Guenter Roeck wrote:
> On 01/29/2017 12:02 PM, Sebastian Reichel wrote:
> 
> > > 
> > > To keep things simple, I think it would be okay to allow only one of
> > > each type of controller in any running system. It's very unlikely that
> > > board designers would devise two different ways of powering off or
> > > restarting a system, while in a similar way an SoC or CPU would only
> > > ever provide one way to do so. Even if theoretically multiple
> > > possibilities exist, I think the board code should pick which ones are
> > > appropriate.
> > 
> > Using that logic we may also advice, that board-code should only
> > register the board-level reset/poweroff and it's enough to have
> > a callback again... I wonder if that is really feasible.
> > 
> 
> FWIW, it is also not true.

It seems this was misunderstood. I do not expect this to work.

> There is a reason why many of the restart handlers used to have
> code saying "install restart handler, but only if none is
> installed yet". Which of course is racy, and gets more interesting
> if the restart handler installed first is unloaded at a later
> time, leaving the system with no restart handler. Or both are
> unloaded, leaving the system with a pointer to a no longer
> existing handler.
>
> One could then argue that anything implementing a restart handler must
> not unload. Which results in more restrictions. And drivers loaded
> on hardware which don't need it. And more corner cases to deal with.
> And more inconsistencies.
> 
> In reality, many systems or system variants will have more than one means
> to restart it. Yes, board designers do devise multiple ways of powering off
> or restarting a system. There may be and likely are valid reasons for doing
> so; I would not want to claim or suggest that board designers would design
> such hardware without reason. Even "standard" PCs tend to have have more
> than one means to reset it. There _was_ a reason for introducing that
> framework; I didn't just do it for fun.
> 
> However, as I had mentioned before, I am not really interested in this
> topic anymore. Just treat this as my final word of caution, or feel free
> to ignore it. I hope you'll find a much better solution than mine
> to implement "the board code should pick which ones are appropriate".

In case I was unclear: I'm fine with the current state of reboot
code using notifier chain and really thankful for the work. IMHO
it improved the status-quo a lot.

However I'm not fine with the current poweroff stuff and if somebody
offers to implement a solution compatible with Linus (and other people,
which disliked the notifier chain approach): Thanks, please do!

-- Sebastian

[toc] | [prev] | [next] | [standalone]

#1563544

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Thu, Jan 19, 2017 at 03:29:34PM -0800, Guenter Roeck wrote:
> On Fri, Jan 20, 2017 at 12:00:36AM +0100, Sebastian Reichel wrote:
> I actually have a set of patches somewhere which transforms the remaining
> direct users of arm_pm_restart to use the framework (unless I removed it
> from my trees - I don't really recall). I just never got around to submit
> it, and then [2] happened and I lost interest.

I found a patchset from you from 2016. I see a few Acks and no
negative feedback. Looks like they only need to be uploaded at
Russel's patch system?

arch/arm/mach-prima2/rstc.c			https://patchwork.kernel.org/patch/8844441/
arch/arm/xen/enlighten.c			https://patchwork.kernel.org/patch/8844481/
drivers/firmware/psci.c				https://patchwork.kernel.org/patch/8844411/
arch/arm/kernel/setup.c				https://patchwork.kernel.org/patch/8844451/
removal of ARM64 arm_pm_restart		https://patchwork.kernel.org/patch/8844471/
removal of ARM32 arm_pm_restart		https://patchwork.kernel.org/patch/8844391/

> > No such thing exists for poweroff. Guenter also wrote something for
> > that [1], but Linus intervened [2]. Anyways, pm_power_off is at
> > least architecture independent.
> > 
> That was by far the most frustrating kernel development experience
> I ever head :-(.

Sorry for the reminder.

-- Sebastian

[toc] | [prev] | [next] | [standalone]

#1563568

[Multipart message — attachments visible in raw view] — view raw

On Fri, Jan 20, 2017 at 01:44:04PM +0100, Sebastian Reichel wrote:
> Hi,
> 
> On Thu, Jan 19, 2017 at 03:29:34PM -0800, Guenter Roeck wrote:
> > On Fri, Jan 20, 2017 at 12:00:36AM +0100, Sebastian Reichel wrote:
> > I actually have a set of patches somewhere which transforms the remaining
> > direct users of arm_pm_restart to use the framework (unless I removed it
> > from my trees - I don't really recall). I just never got around to submit
> > it, and then [2] happened and I lost interest.
> 
> I found a patchset from you from 2016. I see a few Acks and no
> negative feedback. Looks like they only need to be uploaded at
> Russel's patch system?
> 
> arch/arm/mach-prima2/rstc.c			https://patchwork.kernel.org/patch/8844441/
> arch/arm/xen/enlighten.c			https://patchwork.kernel.org/patch/8844481/
> drivers/firmware/psci.c				https://patchwork.kernel.org/patch/8844411/
> arch/arm/kernel/setup.c				https://patchwork.kernel.org/patch/8844451/
> removal of ARM64 arm_pm_restart		https://patchwork.kernel.org/patch/8844471/
> removal of ARM32 arm_pm_restart		https://patchwork.kernel.org/patch/8844391/

Nice, those would be excellent preparatory work for my earlier framework
proposal.

Do you want me to pick those up and run with them a bit?

Thierry

[toc] | [prev] | [next] | [standalone]

#1563595

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Fri, Jan 20, 2017 at 02:11:16PM +0100, Thierry Reding wrote:
> On Fri, Jan 20, 2017 at 01:44:04PM +0100, Sebastian Reichel wrote:
> > Hi,
> > 
> > On Thu, Jan 19, 2017 at 03:29:34PM -0800, Guenter Roeck wrote:
> > > On Fri, Jan 20, 2017 at 12:00:36AM +0100, Sebastian Reichel wrote:
> > > I actually have a set of patches somewhere which transforms the remaining
> > > direct users of arm_pm_restart to use the framework (unless I removed it
> > > from my trees - I don't really recall). I just never got around to submit
> > > it, and then [2] happened and I lost interest.
> > 
> > I found a patchset from you from 2016. I see a few Acks and no
> > negative feedback. Looks like they only need to be uploaded at
> > Russel's patch system?
> > 
> > arch/arm/mach-prima2/rstc.c			https://patchwork.kernel.org/patch/8844441/
> > arch/arm/xen/enlighten.c			https://patchwork.kernel.org/patch/8844481/
> > drivers/firmware/psci.c				https://patchwork.kernel.org/patch/8844411/
> > arch/arm/kernel/setup.c				https://patchwork.kernel.org/patch/8844451/
> > removal of ARM64 arm_pm_restart		https://patchwork.kernel.org/patch/8844471/
> > removal of ARM32 arm_pm_restart		https://patchwork.kernel.org/patch/8844391/
> 
> Nice, those would be excellent preparatory work for my earlier
> framework proposal. Do you want me to pick those up and run with
> them a bit?

They implement what I suggested earlier in this thread, so I think
they should get upstream. If you pick them up, you can add my
Reviewed-By to all patches. After having them upstream, ARM, ARM64,
MIPS and PPC use do_kernel_restart() for reboot.

-- Sebastian

[toc] | [prev] | [next] | [standalone]

#1563663

On 01/20/2017 05:11 AM, Thierry Reding wrote:
> On Fri, Jan 20, 2017 at 01:44:04PM +0100, Sebastian Reichel wrote:
>> Hi,
>>
>> On Thu, Jan 19, 2017 at 03:29:34PM -0800, Guenter Roeck wrote:
>>> On Fri, Jan 20, 2017 at 12:00:36AM +0100, Sebastian Reichel wrote:
>>> I actually have a set of patches somewhere which transforms the remaining
>>> direct users of arm_pm_restart to use the framework (unless I removed it
>>> from my trees - I don't really recall). I just never got around to submit
>>> it, and then [2] happened and I lost interest.
>>
>> I found a patchset from you from 2016. I see a few Acks and no
>> negative feedback. Looks like they only need to be uploaded at
>> Russel's patch system?
>>
>> arch/arm/mach-prima2/rstc.c			https://patchwork.kernel.org/patch/8844441/
>> arch/arm/xen/enlighten.c			https://patchwork.kernel.org/patch/8844481/
>> drivers/firmware/psci.c				https://patchwork.kernel.org/patch/8844411/
>> arch/arm/kernel/setup.c				https://patchwork.kernel.org/patch/8844451/
>> removal of ARM64 arm_pm_restart		https://patchwork.kernel.org/patch/8844471/
>> removal of ARM32 arm_pm_restart		https://patchwork.kernel.org/patch/8844391/
>
> Nice, those would be excellent preparatory work for my earlier framework
> proposal.
>
> Do you want me to pick those up and run with them a bit?
>
> Thierry
>

I didn't recall that I ever submitted them. They are all yours ...

Guenter

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web