Re: [RFC 1/3] system-power: Add system power and restart framework

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2953 bytes --]

On Mon, Jan 30, 2017 at 10:53:01PM +0100, Pavel Machek wrote:
> Hi!
> 
> 
> > +struct system_power_chip;
> > +
> > +struct system_power_ops {
> > +	int (*restart)(struct system_power_chip *chip, enum reboot_mode mode,
> > +		       char *cmd);
> > +	int (*power_off_prepare)(struct system_power_chip *chip);
> > +	int (*power_off)(struct system_power_chip *chip);
> > +};
> > +
> > +struct system_power_chip {
> > +	const struct system_power_ops *ops;
> > +	struct list_head list;
> > +	struct device *dev;
> > +};
> 
> Is it useful to have two structures? AFAICT one would do.

Yeah, one structure works fine. I was drawing inspiration from other
subsystems that have a separate structure for these. I've merged the
operations into the struct system_power_chip now because that gives
us some more flexiblity, for example in cases where a chip can be a
power controller and a reset controller, but sometimes we may want
it to be only one of them.

> Do we always have struct device * to work with? IMO we have nothing
> suitable for example in the ACPI case. Would void * be more suitable?

The struct device * was meant to be purely optional, but working with
the code some more today and doing some more conversions, I've resorted
to adding a separate field (const char *name) that takes precedence. So
if a chip specifies both a .dev and .name field, then .name will be the
user visible string, otherwise dev_name(.dev) will be used in messages.

> Could you convert someting (acpi?) to the new framework as
> demonstration?

I had originally only converted architecture code to call into system
power instead of the notifier chain and added a driver for a chip that
I want to get this to work on. I've now converted a couple of other
drivers from drivers/power/reset as well as ACPI. I've also added a
very rudimentary prioritization mechanism that I've validated on the
specific setup that I'm working on.

On the Jetson TX1 that I'm testing this on, the SoC has a way of
resetting itself. This has the advantage that some of the registers are
kept intact over the reset, and this in turn is used to control early
boot, so that specific recovery modes can be used. However, the board
has to be powered off using the PMIC (via I2C). The patches achieve this
by splitting up restart and power off into two steps, prepare and
restart/power-off, as well as levels to prioritize. On Jetson TX1 the
PMIC will be higher priority than the SoC (determined by the level) and
therefore be able to override the SoC restart mechanism if we want to.
If we don't we simply instruct the MAX77620 driver not to register the
restart callback, in which case the SoC implementation will be used.

I've uploaded all of it to a branch on github:

	https://github.com/thierryreding/linux/tree/system-power

It's rather lengthy, so I'm not sure if it makes sense to send to the
lists right away.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]