Messages in this thread | | | Date | Sun, 29 Jan 2017 21:02:39 +0100 | From | Sebastian Reichel <> | 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 [unhandled content-type:application/pgp-signature] | |