Message-ID: <20170129200239.x2rwx23xjj57flus@earth>
Date:   Sun, 29 Jan 2017 21:02:39 +0100
From:   Sebastian Reichel <sre@...nel.org>
To:     Thierry Reding <thierry.reding@...il.com>
Cc:     Guenter Roeck <linux@...ck-us.net>,
        Laxman Dewangan <ldewangan@...dia.com>,
        Martin Michlmayr <tbm@...ius.com>, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] power: reset: Add MAX77620 support

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

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists