diff mbox

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

Message ID 20170130171506.3527-2-thierry.reding@gmail.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Thierry Reding Jan. 30, 2017, 5:15 p.m. UTC 
From: Thierry Reding <treding@nvidia.com>

This adds a very simple framework that allows drivers to register system
power and restart controllers. The goal of this framework is to replace
the current notifier based mechanism for restart handlers and the power
off equivalent that is the global pm_power_off() function.

Both of these approaches currently lack any means of locking against
concurrently registering handlers or formal definitions on what proper
priorities are to order handlers.

The system-power framework attempts to remedy this by adding a system
power chip object that drivers can embed in their driver-specific data.
A chip contains a description of capabilities that the framework uses
to determine a good sequence of handlers to use for restart and power
off.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/base/Makefile        |   3 +-
 drivers/base/system-power.c  | 110 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/system-power.h |  38 +++++++++++++++
 3 files changed, 150 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/system-power.c
 create mode 100644 include/linux/system-power.h

Comments

Pavel Machek Jan. 30, 2017, 9:53 p.m. UTC | #1 
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.

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?

Could you convert someting (acpi?) to the new framework as
demonstration?
								Pavel
Thierry Reding Jan. 31, 2017, 5:46 p.m. UTC | #2 
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
Pavel Machek Feb. 1, 2017, 11:13 a.m. UTC | #3 
On Tue 2017-01-31 18:46:58, Thierry Reding wrote:
> 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.

Thanks!

> > 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.

It is easier to review on lists, but no reasons to do it now.
									Pavel
diff mbox

Patch

diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index f2816f6ff76a..eef165221d9d 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -4,7 +4,8 @@  obj-y			:= component.o core.o bus.o dd.o syscore.o \
 			   driver.o class.o platform.o \
 			   cpu.o firmware.o init.o map.o devres.o \
 			   attribute_container.o transport_class.o \
-			   topology.o container.o property.o cacheinfo.o
+			   topology.o container.o property.o cacheinfo.o \
+			   system-power.o
 obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
 obj-$(CONFIG_DMA_CMA) += dma-contiguous.o
 obj-y			+= power/
diff --git a/drivers/base/system-power.c b/drivers/base/system-power.c
new file mode 100644
index 000000000000..96c0cb457933
--- /dev/null
+++ b/drivers/base/system-power.c
@@ -0,0 +1,110 @@ 
+/*
+ * Copyright (c) 2017 NVIDIA Corporation
+ *
+ * This file is released under the GPL v2
+ */
+
+#define pr_fmt(fmt) "system-power: " fmt
+
+#include <linux/system-power.h>
+
+static DEFINE_MUTEX(system_power_lock);
+static LIST_HEAD(system_power_chips);
+
+int system_power_chip_add(struct system_power_chip *chip)
+{
+	if (!chip->ops || (!chip->ops->restart && !chip->ops->power_off)) {
+		WARN(1, pr_fmt("must implement restart or power off\n"));
+		return -EINVAL;
+	}
+
+	mutex_lock(&system_power_lock);
+
+	INIT_LIST_HEAD(&chip->list);
+	list_add_tail(&chip->list, &system_power_chips);
+
+	mutex_unlock(&system_power_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(system_power_chip_add);
+
+int system_power_chip_remove(struct system_power_chip *chip)
+{
+	mutex_lock(&system_power_lock);
+
+	list_del_init(&chip->list);
+
+	mutex_unlock(&system_power_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(system_power_chip_remove);
+
+bool system_can_power_off(void)
+{
+	/* XXX for backwards compatibility */
+	return pm_power_off != NULL;
+}
+
+int system_restart(char *cmd)
+{
+	struct system_power_chip *chip;
+	int err;
+
+	mutex_lock(&system_power_lock);
+
+	list_for_each_entry(chip, &system_power_chips, list) {
+		if (!chip->ops->restart)
+			continue;
+
+		pr_debug("trying to restart using %ps\n", chip);
+
+		err = chip->ops->restart(chip, reboot_mode, cmd);
+		if (err < 0)
+			dev_warn(chip->dev, "failed to restart: %d\n", err);
+	}
+
+	mutex_unlock(&system_power_lock);
+
+	/* XXX for backwards compatibility */
+	do_kernel_restart(cmd);
+
+	return 0;
+}
+
+int system_power_off_prepare(void)
+{
+	/* XXX for backwards compatibility */
+	if (pm_power_off_prepare)
+		pm_power_off_prepare();
+
+	return 0;
+}
+
+int system_power_off(void)
+{
+	struct system_power_chip *chip;
+	int err;
+
+	mutex_lock(&system_power_lock);
+
+	list_for_each_entry(chip, &system_power_chips, list) {
+		if (!chip->ops->power_off)
+			continue;
+
+		pr_debug("trying to power off using %ps\n", chip);
+
+		err = chip->ops->power_off(chip);
+		if (err < 0)
+			dev_warn(chip->dev, "failed to power off: %d\n", err);
+	}
+
+	mutex_unlock(&system_power_lock);
+
+	/* XXX for backwards compatibility */
+	if (pm_power_off)
+		pm_power_off();
+
+	return 0;
+}
diff --git a/include/linux/system-power.h b/include/linux/system-power.h
new file mode 100644
index 000000000000..f709c14c1552
--- /dev/null
+++ b/include/linux/system-power.h
@@ -0,0 +1,38 @@ 
+/*
+ * Copyright (c) 2017 NVIDIA Corporation
+ *
+ * This file is released under the GPL v2
+ */
+
+#ifndef SYSTEM_POWER_H
+#define SYSTEM_POWER_H
+
+#include <linux/device.h>
+#include <linux/list.h>
+#include <linux/reboot.h>
+
+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;
+};
+
+int system_power_chip_add(struct system_power_chip *chip);
+int system_power_chip_remove(struct system_power_chip *chip);
+
+bool system_can_power_off(void);
+
+int system_restart(char *cmd);
+int system_power_off_prepare(void);
+int system_power_off(void);
+
+#endif