[PATCH] power: reset: Add MAX77620 support

[PATCH] power: reset: Add MAX77620 support

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The Maxim MAX77620 PMIC has the ability to power off and restart the
system. Add a driver that supports power off (via pm_power_off()) and
restart (via arm_pm_restart() on 32-bit and 64-bit ARM).

Based on work by Chaitanya Bandi <bandik@nvidia.com>.

Cc: Chaitanya Bandi <bandik@nvidia.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/power/reset/Kconfig             |   6 ++
 drivers/power/reset/Makefile            |   1 +
 drivers/power/reset/max77620-poweroff.c | 146 ++++++++++++++++++++++++++++++++
 3 files changed, 153 insertions(+)
 create mode 100644 drivers/power/reset/max77620-poweroff.c

diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
index abeb77217a21..f0d0c20632f8 100644
--- a/drivers/power/reset/Kconfig
+++ b/drivers/power/reset/Kconfig
@@ -98,6 +98,12 @@ config POWER_RESET_IMX
 	  say N here or disable in dts to make sure pm_power_off never be
 	  overwrote wrongly by this driver.
 
+config POWER_RESET_MAX77620
+	bool "Maxim MAX77620 PMIC power-off driver"
+	depends on MFD_MAX77620
+	help
+	  Power off and restart support for Maxim MAX77620 PMICs.
+
 config POWER_RESET_MSM
 	bool "Qualcomm MSM power-off driver"
 	depends on ARCH_QCOM
diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
index 11dae3b56ff9..74511d2f037a 100644
--- a/drivers/power/reset/Makefile
+++ b/drivers/power/reset/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_POWER_RESET_GPIO) += gpio-poweroff.o
 obj-$(CONFIG_POWER_RESET_GPIO_RESTART) += gpio-restart.o
 obj-$(CONFIG_POWER_RESET_HISI) += hisi-reboot.o
 obj-$(CONFIG_POWER_RESET_IMX) += imx-snvs-poweroff.o
+obj-$(CONFIG_POWER_RESET_MAX77620) += max77620-poweroff.o
 obj-$(CONFIG_POWER_RESET_MSM) += msm-poweroff.o
 obj-$(CONFIG_POWER_RESET_PIIX4_POWEROFF) += piix4-poweroff.o
 obj-$(CONFIG_POWER_RESET_LTC2952) += ltc2952-poweroff.o
diff --git a/drivers/power/reset/max77620-poweroff.c b/drivers/power/reset/max77620-poweroff.c
new file mode 100644
index 000000000000..4f2682d10925
--- /dev/null
+++ b/drivers/power/reset/max77620-poweroff.c
@@ -0,0 +1,146 @@
+/*
+ * Power off driver for Maxim MAX77620 device.
+ *
+ * Copyright (c) 2014-2016, NVIDIA CORPORATION.  All rights reserved.
+ *
+ * Based on work by Chaitanya Bandi <bandik@nvidia.com>.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any kind,
+ * whether express or implied; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ */
+
+#include <linux/errno.h>
+#include <linux/mfd/max77620.h>
+#include <linux/module.h>
+#include <linux/notifier.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reboot.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
+#include <asm/system_misc.h>
+#endif
+
+struct max77620_power {
+	struct regmap *regmap;
+	struct device *dev;
+};
+
+static struct max77620_power *system_power_controller = NULL;
+
+static void max77620_pm_power_off(void)
+{
+	struct max77620_power *power = system_power_controller;
+	unsigned int value;
+	int err;
+
+	if (!power)
+		return;
+
+	/* clear power key interrupts */
+	err = regmap_read(power->regmap, MAX77620_REG_ONOFFIRQ, &value);
+	if (err < 0)
+		dev_err(power->dev, "failed to clear power key interrupts: %d\n", err);
+
+	/* clear RTC interrupts */
+	/*
+	err = regmap_read(power->regmap, MAX77620_REG_RTCINT, &value);
+	if (err < 0)
+		dev_err(power->dev, "failed to clear RTC interrupts: %d\n", err);
+	*/
+
+	/* clear TOP interrupts */
+	err = regmap_read(power->regmap, MAX77620_REG_IRQTOP, &value);
+	if (err < 0)
+		dev_err(power->dev, "failed to clear interrupts: %d\n", err);
+
+	err = regmap_update_bits(power->regmap, MAX77620_REG_ONOFFCNFG2,
+				 MAX77620_ONOFFCNFG2_SFT_RST_WK, 0);
+	if (err < 0)
+		dev_err(power->dev, "failed to clear SFT_RST_WK: %d\n", err);
+
+	err = regmap_update_bits(power->regmap, MAX77620_REG_ONOFFCNFG1,
+				 MAX77620_ONOFFCNFG1_SFT_RST,
+				 MAX77620_ONOFFCNFG1_SFT_RST);
+	if (err < 0)
+		dev_err(power->dev, "failed to set SFT_RST: %d\n", err);
+}
+
+#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
+static void max77620_pm_restart(enum reboot_mode mode, const char *cmd)
+{
+	struct max77620_power *power = system_power_controller;
+	int err;
+
+	if (!power)
+		return;
+
+	err = regmap_update_bits(power->regmap, MAX77620_REG_ONOFFCNFG2,
+				 MAX77620_ONOFFCNFG2_SFT_RST_WK,
+				 MAX77620_ONOFFCNFG2_SFT_RST_WK);
+	if (err < 0)
+		dev_err(power->dev, "failed to set SFT_RST_WK: %d\n", err);
+
+	err = regmap_update_bits(power->regmap, MAX77620_REG_ONOFFCNFG1,
+				 MAX77620_ONOFFCNFG1_SFT_RST,
+				 MAX77620_ONOFFCNFG1_SFT_RST);
+	if (err < 0)
+		dev_err(power->dev, "failed to set SFT_RST: %d\n", err);
+}
+#endif
+
+static int max77620_poweroff_probe(struct platform_device *pdev)
+{
+	struct max77620_chip *max77620 = dev_get_drvdata(pdev->dev.parent);
+	struct device_node *np = pdev->dev.parent->of_node;
+	struct max77620_power *power;
+	unsigned int value;
+	int err;
+
+	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
+
+	return 0;
+}
+
+static struct platform_driver max77620_poweroff_driver = {
+	.driver = {
+		.name = "max77620-power",
+	},
+	.probe = max77620_poweroff_probe,
+};
+module_platform_driver(max77620_poweroff_driver);
+
+MODULE_DESCRIPTION("Maxim MAX77620 PMIC power off and restart driver");
+MODULE_AUTHOR("Thierry Reding <treding@nvidia.com>");
+MODULE_ALIAS("platform:max77620-power");
+MODULE_LICENSE("GPL v2");
-- 
2.11.0

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

[Laxman Dewangan]

On Thursday 12 January 2017 09:45 PM, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> The Maxim MAX77620 PMIC has the ability to power off and restart the
> system. Add a driver that supports power off (via pm_power_off()) and
> restart (via arm_pm_restart() on 32-bit and 64-bit ARM).
>
> Based on work by Chaitanya Bandi <bandik@nvidia.com>.
>
> Cc: Chaitanya Bandi <bandik@nvidia.com>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>   drivers/power/reset/Kconfig             |   6 ++
>   drivers/power/reset/Makefile            |   1 +
>   drivers/power/reset/max77620-poweroff.c | 146 ++++++++++++++++++++++++++++++++
>   3 files changed, 153 insertions(+)
>   create mode 100644 drivers/power/reset/max77620-poweroff.c
>
> diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
> index abeb77217a21..f0d0c20632f8 100644
> --- a/drivers/power/reset/Kconfig
> +++ b/drivers/power/reset/Kconfig
> @@ -98,6 +98,12 @@ config POWER_RESET_IMX
>   	  say N here or disable in dts to make sure pm_power_off never be
>   	  overwrote wrongly by this driver.
>   
> +config POWER_RESET_MAX77620
> +	bool "Maxim MAX77620 PMIC power-off driver"
> +	depends on MFD_MAX77620
> +	help
> +	  Power off and restart support for Maxim MAX77620 PMICs.
> +
>   config POWER_RESET_MSM
>   	bool "Qualcomm MSM power-off driver"
>   	depends on ARCH_QCOM
> diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
> index 11dae3b56ff9..74511d2f037a 100644
> --- a/drivers/power/reset/Makefile
> +++ b/drivers/power/reset/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_POWER_RESET_GPIO) += gpio-poweroff.o
>   obj-$(CONFIG_POWER_RESET_GPIO_RESTART) += gpio-restart.o
>   obj-$(CONFIG_POWER_RESET_HISI) += hisi-reboot.o
>   obj-$(CONFIG_POWER_RESET_IMX) += imx-snvs-poweroff.o
> +obj-$(CONFIG_POWER_RESET_MAX77620) += max77620-poweroff.o
>   obj-$(CONFIG_POWER_RESET_MSM) += msm-poweroff.o
>   obj-$(CONFIG_POWER_RESET_PIIX4_POWEROFF) += piix4-poweroff.o
>   obj-$(CONFIG_POWER_RESET_LTC2952) += ltc2952-poweroff.o
> diff --git a/drivers/power/reset/max77620-poweroff.c b/drivers/power/reset/max77620-poweroff.c
> new file mode 100644
> index 000000000000..4f2682d10925
> --- /dev/null
> +++ b/drivers/power/reset/max77620-poweroff.c
> @@ -0,0 +1,146 @@
> +/*
> + * Power off driver for Maxim MAX77620 device.
> + *
> + * Copyright (c) 2014-2016, NVIDIA CORPORATION.  All rights reserved.

Should we change year here?

> + *
> + * Based on work by Chaitanya Bandi <bandik@nvidia.com>.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any kind,
> + * whether express or implied; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/mfd/max77620.h>
> +#include <linux/module.h>
> +#include <linux/notifier.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/reboot.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
> +#include <asm/system_misc.h>
> +#endif
> +
> +struct max77620_power {
> +	struct regmap *regmap;
> +	struct device *dev;
> +};
> +
> +static struct max77620_power *system_power_controller = NULL;

As this is static and so already initialized as NULL. Hence we may not 
need to explicitly NULL initialization.
> +
> +static void max77620_pm_power_off(void)
> +{
> +	struct max77620_power *power = system_power_controller;
> +	unsigned int value;
> +	int err;
> +
> +	if (!power)
> +		return;
> +
> +	/* clear power key interrupts */
> +	err = regmap_read(power->regmap, MAX77620_REG_ONOFFIRQ, &value);
> +	if (err < 0)
> +		dev_err(power->dev, "failed to clear power key interrupts: %d\n", err);
> +
> +	/* clear RTC interrupts */
> +	/*
> +	err = regmap_read(power->regmap, MAX77620_REG_RTCINT, &value);
> +	if (err < 0)
> +		dev_err(power->dev, "failed to clear RTC interrupts: %d\n", err);
> +	*/
> +
> +	/* clear TOP interrupts */
> +	err = regmap_read(power->regmap, MAX77620_REG_IRQTOP, &value);
> +	if (err < 0)
> +		dev_err(power->dev, "failed to clear interrupts: %d\n", err);
> +
> +	err = regmap_update_bits(power->regmap, MAX77620_REG_ONOFFCNFG2,
> +				 MAX77620_ONOFFCNFG2_SFT_RST_WK, 0);
> +	if (err < 0)
> +		dev_err(power->dev, "failed to clear SFT_RST_WK: %d\n", err);
> +
> +	err = regmap_update_bits(power->regmap, MAX77620_REG_ONOFFCNFG1,
> +				 MAX77620_ONOFFCNFG1_SFT_RST,
> +				 MAX77620_ONOFFCNFG1_SFT_RST);
> +	if (err < 0)
> +		dev_err(power->dev, "failed to set SFT_RST: %d\n", err);
> +}
> +
> +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
> +static void max77620_pm_restart(enum reboot_mode mode, const char *cmd)
> +{
> +	struct max77620_power *power = system_power_controller;
> +	int err;
> +
> +	if (!power)
> +		return;
> +
> +	err = regmap_update_bits(power->regmap, MAX77620_REG_ONOFFCNFG2,
> +				 MAX77620_ONOFFCNFG2_SFT_RST_WK,
> +				 MAX77620_ONOFFCNFG2_SFT_RST_WK);
> +	if (err < 0)
> +		dev_err(power->dev, "failed to set SFT_RST_WK: %d\n", err);
> +
> +	err = regmap_update_bits(power->regmap, MAX77620_REG_ONOFFCNFG1,
> +				 MAX77620_ONOFFCNFG1_SFT_RST,
> +				 MAX77620_ONOFFCNFG1_SFT_RST);
> +	if (err < 0)
> +		dev_err(power->dev, "failed to set SFT_RST: %d\n", err);
> +}
> +#endif
> +
> +static int max77620_poweroff_probe(struct platform_device *pdev)
> +{
> +	struct max77620_chip *max77620 = dev_get_drvdata(pdev->dev.parent);
> +	struct device_node *np = pdev->dev.parent->of_node;
> +	struct max77620_power *power;
> +	unsigned int value;
> +	int err;
> +
> +	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;

We can use the function info->regmap = dev_get_regmap(parent, NULL); to 
get the regmap. This will make this driver independent of max77620 chip 
and extend same for other Maxim Chips.

> +	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;

What if we want to reset via the Tegra and not through PMIC?
> +#endif
> +
> +	return 0;
> +}
> +
> +static struct platform_driver max77620_poweroff_driver = {
> +	.driver = {
> +		.name = "max77620-power",
> +	},
> +	.probe = max77620_poweroff_probe,
> +};
> +module_platform_driver(max77620_poweroff_driver);
> +
> +MODULE_DESCRIPTION("Maxim MAX77620 PMIC power off and restart driver");
> +MODULE_AUTHOR("Thierry Reding <treding@nvidia.com>");
> +MODULE_ALIAS("platform:max77620-power");
> +MODULE_LICENSE("GPL v2");

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

[Laxman Dewangan]

On Thursday 12 January 2017 11:05 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Thu, Jan 12, 2017 at 10:06:24PM +0530, Laxman Dewangan wrote:
>> On Thursday 12 January 2017 09:45 PM, Thierry Reding wrote:
>>>
>>> +	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;
>> What if we want to reset via the Tegra and not through PMIC?
> In that case I assume we either don't have a PMIC, or we should not add
> the system-power-controller device tree property to the PMIC node. In
> the latter case this driver won't install any power off or restart
> callbacks.


We need reset from Tegra and power of from PMIC. Tegra does not support 
power OFF.

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

[Thierry Reding]

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

On Thu, Jan 12, 2017 at 10:06:24PM +0530, Laxman Dewangan wrote:
> 
> On Thursday 12 January 2017 09:45 PM, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > The Maxim MAX77620 PMIC has the ability to power off and restart the
> > system. Add a driver that supports power off (via pm_power_off()) and
> > restart (via arm_pm_restart() on 32-bit and 64-bit ARM).
> > 
> > Based on work by Chaitanya Bandi <bandik@nvidia.com>.
> > 
> > Cc: Chaitanya Bandi <bandik@nvidia.com>
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >   drivers/power/reset/Kconfig             |   6 ++
> >   drivers/power/reset/Makefile            |   1 +
> >   drivers/power/reset/max77620-poweroff.c | 146 ++++++++++++++++++++++++++++++++
> >   3 files changed, 153 insertions(+)
> >   create mode 100644 drivers/power/reset/max77620-poweroff.c
> > 
> > diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
> > index abeb77217a21..f0d0c20632f8 100644
> > --- a/drivers/power/reset/Kconfig
> > +++ b/drivers/power/reset/Kconfig
> > @@ -98,6 +98,12 @@ config POWER_RESET_IMX
> >   	  say N here or disable in dts to make sure pm_power_off never be
> >   	  overwrote wrongly by this driver.
> > +config POWER_RESET_MAX77620
> > +	bool "Maxim MAX77620 PMIC power-off driver"
> > +	depends on MFD_MAX77620
> > +	help
> > +	  Power off and restart support for Maxim MAX77620 PMICs.
> > +
> >   config POWER_RESET_MSM
> >   	bool "Qualcomm MSM power-off driver"
> >   	depends on ARCH_QCOM
> > diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
> > index 11dae3b56ff9..74511d2f037a 100644
> > --- a/drivers/power/reset/Makefile
> > +++ b/drivers/power/reset/Makefile
> > @@ -9,6 +9,7 @@ obj-$(CONFIG_POWER_RESET_GPIO) += gpio-poweroff.o
> >   obj-$(CONFIG_POWER_RESET_GPIO_RESTART) += gpio-restart.o
> >   obj-$(CONFIG_POWER_RESET_HISI) += hisi-reboot.o
> >   obj-$(CONFIG_POWER_RESET_IMX) += imx-snvs-poweroff.o
> > +obj-$(CONFIG_POWER_RESET_MAX77620) += max77620-poweroff.o
> >   obj-$(CONFIG_POWER_RESET_MSM) += msm-poweroff.o
> >   obj-$(CONFIG_POWER_RESET_PIIX4_POWEROFF) += piix4-poweroff.o
> >   obj-$(CONFIG_POWER_RESET_LTC2952) += ltc2952-poweroff.o
> > diff --git a/drivers/power/reset/max77620-poweroff.c b/drivers/power/reset/max77620-poweroff.c
> > new file mode 100644
> > index 000000000000..4f2682d10925
> > --- /dev/null
> > +++ b/drivers/power/reset/max77620-poweroff.c
> > @@ -0,0 +1,146 @@
> > +/*
> > + * Power off driver for Maxim MAX77620 device.
> > + *
> > + * Copyright (c) 2014-2016, NVIDIA CORPORATION.  All rights reserved.
> 
> Should we change year here?

Heh, indeed. It's probably okay to even leave out the range and make
this 2017 because the current driver is fairly far removed from the one
downstream.

> > + *
> > + * Based on work by Chaitanya Bandi <bandik@nvidia.com>.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation version 2.
> > + *
> > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any kind,
> > + * whether express or implied; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + * General Public License for more details.
> > + */
> > +
> > +#include <linux/errno.h>
> > +#include <linux/mfd/max77620.h>
> > +#include <linux/module.h>
> > +#include <linux/notifier.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/reboot.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +
> > +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
> > +#include <asm/system_misc.h>
> > +#endif
> > +
> > +struct max77620_power {
> > +	struct regmap *regmap;
> > +	struct device *dev;
> > +};
> > +
> > +static struct max77620_power *system_power_controller = NULL;
> 
> As this is static and so already initialized as NULL. Hence we may not need
> to explicitly NULL initialization.

Yes, I'll drop the explicit assignment.

> > +static void max77620_pm_power_off(void)
> > +{
> > +	struct max77620_power *power = system_power_controller;
> > +	unsigned int value;
> > +	int err;
> > +
> > +	if (!power)
> > +		return;
> > +
> > +	/* clear power key interrupts */
> > +	err = regmap_read(power->regmap, MAX77620_REG_ONOFFIRQ, &value);
> > +	if (err < 0)
> > +		dev_err(power->dev, "failed to clear power key interrupts: %d\n", err);
> > +
> > +	/* clear RTC interrupts */
> > +	/*
> > +	err = regmap_read(power->regmap, MAX77620_REG_RTCINT, &value);
> > +	if (err < 0)
> > +		dev_err(power->dev, "failed to clear RTC interrupts: %d\n", err);
> > +	*/
> > +
> > +	/* clear TOP interrupts */
> > +	err = regmap_read(power->regmap, MAX77620_REG_IRQTOP, &value);
> > +	if (err < 0)
> > +		dev_err(power->dev, "failed to clear interrupts: %d\n", err);
> > +
> > +	err = regmap_update_bits(power->regmap, MAX77620_REG_ONOFFCNFG2,
> > +				 MAX77620_ONOFFCNFG2_SFT_RST_WK, 0);
> > +	if (err < 0)
> > +		dev_err(power->dev, "failed to clear SFT_RST_WK: %d\n", err);
> > +
> > +	err = regmap_update_bits(power->regmap, MAX77620_REG_ONOFFCNFG1,
> > +				 MAX77620_ONOFFCNFG1_SFT_RST,
> > +				 MAX77620_ONOFFCNFG1_SFT_RST);
> > +	if (err < 0)
> > +		dev_err(power->dev, "failed to set SFT_RST: %d\n", err);
> > +}
> > +
> > +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
> > +static void max77620_pm_restart(enum reboot_mode mode, const char *cmd)
> > +{
> > +	struct max77620_power *power = system_power_controller;
> > +	int err;
> > +
> > +	if (!power)
> > +		return;
> > +
> > +	err = regmap_update_bits(power->regmap, MAX77620_REG_ONOFFCNFG2,
> > +				 MAX77620_ONOFFCNFG2_SFT_RST_WK,
> > +				 MAX77620_ONOFFCNFG2_SFT_RST_WK);
> > +	if (err < 0)
> > +		dev_err(power->dev, "failed to set SFT_RST_WK: %d\n", err);
> > +
> > +	err = regmap_update_bits(power->regmap, MAX77620_REG_ONOFFCNFG1,
> > +				 MAX77620_ONOFFCNFG1_SFT_RST,
> > +				 MAX77620_ONOFFCNFG1_SFT_RST);
> > +	if (err < 0)
> > +		dev_err(power->dev, "failed to set SFT_RST: %d\n", err);
> > +}
> > +#endif
> > +
> > +static int max77620_poweroff_probe(struct platform_device *pdev)
> > +{
> > +	struct max77620_chip *max77620 = dev_get_drvdata(pdev->dev.parent);
> > +	struct device_node *np = pdev->dev.parent->of_node;
> > +	struct max77620_power *power;
> > +	unsigned int value;
> > +	int err;
> > +
> > +	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;
> 
> We can use the function info->regmap = dev_get_regmap(parent, NULL); to get
> the regmap. This will make this driver independent of max77620 chip and
> extend same for other Maxim Chips.

Good point. I'll make that change.

> > +	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;
> 
> What if we want to reset via the Tegra and not through PMIC?

In that case I assume we either don't have a PMIC, or we should not add
the system-power-controller device tree property to the PMIC node. In
the latter case this driver won't install any power off or restart
callbacks.

Thierry

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

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

[Sebastian Reichel]

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

Hi Thierry,

I have a few comments inline.

On Thu, Jan 12, 2017 at 05:15:07PM +0100, Thierry Reding wrote:
> The Maxim MAX77620 PMIC has the ability to power off and restart the
> system. Add a driver that supports power off (via pm_power_off()) and
> restart (via arm_pm_restart() on 32-bit and 64-bit ARM).
> 
> Based on work by Chaitanya Bandi <bandik@nvidia.com>.
> 
> Cc: Chaitanya Bandi <bandik@nvidia.com>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/power/reset/Kconfig             |   6 ++
>  drivers/power/reset/Makefile            |   1 +
>  drivers/power/reset/max77620-poweroff.c | 146 ++++++++++++++++++++++++++++++++
>  3 files changed, 153 insertions(+)
>  create mode 100644 drivers/power/reset/max77620-poweroff.c
> 
> diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
> index abeb77217a21..f0d0c20632f8 100644
> --- a/drivers/power/reset/Kconfig
> +++ b/drivers/power/reset/Kconfig
> @@ -98,6 +98,12 @@ config POWER_RESET_IMX
>  	  say N here or disable in dts to make sure pm_power_off never be
>  	  overwrote wrongly by this driver.
>  
> +config POWER_RESET_MAX77620
> +	bool "Maxim MAX77620 PMIC power-off driver"
> +	depends on MFD_MAX77620
> +	help
> +	  Power off and restart support for Maxim MAX77620 PMICs.
> +
>  config POWER_RESET_MSM
>  	bool "Qualcomm MSM power-off driver"
>  	depends on ARCH_QCOM
> diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
> index 11dae3b56ff9..74511d2f037a 100644
> --- a/drivers/power/reset/Makefile
> +++ b/drivers/power/reset/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_POWER_RESET_GPIO) += gpio-poweroff.o
>  obj-$(CONFIG_POWER_RESET_GPIO_RESTART) += gpio-restart.o
>  obj-$(CONFIG_POWER_RESET_HISI) += hisi-reboot.o
>  obj-$(CONFIG_POWER_RESET_IMX) += imx-snvs-poweroff.o
> +obj-$(CONFIG_POWER_RESET_MAX77620) += max77620-poweroff.o
>  obj-$(CONFIG_POWER_RESET_MSM) += msm-poweroff.o
>  obj-$(CONFIG_POWER_RESET_PIIX4_POWEROFF) += piix4-poweroff.o
>  obj-$(CONFIG_POWER_RESET_LTC2952) += ltc2952-poweroff.o
> diff --git a/drivers/power/reset/max77620-poweroff.c b/drivers/power/reset/max77620-poweroff.c
> new file mode 100644
> index 000000000000..4f2682d10925
> --- /dev/null
> +++ b/drivers/power/reset/max77620-poweroff.c
> @@ -0,0 +1,146 @@
> +/*
> + * Power off driver for Maxim MAX77620 device.
> + *
> + * Copyright (c) 2014-2016, NVIDIA CORPORATION.  All rights reserved.
> + *
> + * Based on work by Chaitanya Bandi <bandik@nvidia.com>.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any kind,
> + * whether express or implied; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/mfd/max77620.h>
> +#include <linux/module.h>
> +#include <linux/notifier.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/reboot.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
> +#include <asm/system_misc.h>
> +#endif
> +
> +struct max77620_power {
> +	struct regmap *regmap;
> +	struct device *dev;
> +};
> +
> +static struct max77620_power *system_power_controller = NULL;
> +
> +static void max77620_pm_power_off(void)
> +{
> +	struct max77620_power *power = system_power_controller;
> +	unsigned int value;
> +	int err;
> +
> +	if (!power)
> +		return;
> +
> +	/* clear power key interrupts */
> +	err = regmap_read(power->regmap, MAX77620_REG_ONOFFIRQ, &value);
> +	if (err < 0)
> +		dev_err(power->dev, "failed to clear power key interrupts: %d\n", err);
> +
> +	/* clear RTC interrupts */
> +	/*
> +	err = regmap_read(power->regmap, MAX77620_REG_RTCINT, &value);
> +	if (err < 0)
> +		dev_err(power->dev, "failed to clear RTC interrupts: %d\n", err);
> +	*/
> +
> +	/* clear TOP interrupts */
> +	err = regmap_read(power->regmap, MAX77620_REG_IRQTOP, &value);
> +	if (err < 0)
> +		dev_err(power->dev, "failed to clear interrupts: %d\n", err);
> +
> +	err = regmap_update_bits(power->regmap, MAX77620_REG_ONOFFCNFG2,
> +				 MAX77620_ONOFFCNFG2_SFT_RST_WK, 0);
> +	if (err < 0)
> +		dev_err(power->dev, "failed to clear SFT_RST_WK: %d\n", err);
> +
> +	err = regmap_update_bits(power->regmap, MAX77620_REG_ONOFFCNFG1,
> +				 MAX77620_ONOFFCNFG1_SFT_RST,
> +				 MAX77620_ONOFFCNFG1_SFT_RST);
> +	if (err < 0)
> +		dev_err(power->dev, "failed to set SFT_RST: %d\n", err);
> +}
> +
> +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
> +static void max77620_pm_restart(enum reboot_mode mode, const char *cmd)
> +{
> +	struct max77620_power *power = system_power_controller;
> +	int err;
> +
> +	if (!power)
> +		return;
> +
> +	err = regmap_update_bits(power->regmap, MAX77620_REG_ONOFFCNFG2,
> +				 MAX77620_ONOFFCNFG2_SFT_RST_WK,
> +				 MAX77620_ONOFFCNFG2_SFT_RST_WK);
> +	if (err < 0)
> +		dev_err(power->dev, "failed to set SFT_RST_WK: %d\n", err);
> +
> +	err = regmap_update_bits(power->regmap, MAX77620_REG_ONOFFCNFG1,
> +				 MAX77620_ONOFFCNFG1_SFT_RST,
> +				 MAX77620_ONOFFCNFG1_SFT_RST);
> +	if (err < 0)
> +		dev_err(power->dev, "failed to set SFT_RST: %d\n", err);
> +}
> +#endif
> +
> +static int max77620_poweroff_probe(struct platform_device *pdev)
> +{
> +	struct max77620_chip *max77620 = dev_get_drvdata(pdev->dev.parent);
> +	struct device_node *np = pdev->dev.parent->of_node;
> +	struct max77620_power *power;
> +	unsigned int value;
> +	int err;

if (system_power_controller)
    return -EINVAL;

> +	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().

> +	return 0;
> +}

This is missing a removal function, which should unregister the
restart handler and do something like this:

if (pm_power_off == max77620_pm_power_off)
    pm_power_off = NULL;

> +static struct platform_driver max77620_poweroff_driver = {
> +	.driver = {
> +		.name = "max77620-power",
> +	},
> +	.probe = max77620_poweroff_probe,
> +};
> +module_platform_driver(max77620_poweroff_driver);
> +
> +MODULE_DESCRIPTION("Maxim MAX77620 PMIC power off and restart driver");
> +MODULE_AUTHOR("Thierry Reding <treding@nvidia.com>");
> +MODULE_ALIAS("platform:max77620-power");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.11.0

-- Sebastian

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

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

[Thierry Reding]

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

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

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

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

[Thierry Reding]

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

On Thu, Jan 12, 2017 at 11:05:05PM +0530, Laxman Dewangan wrote:
> 
> On Thursday 12 January 2017 11:05 PM, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > On Thu, Jan 12, 2017 at 10:06:24PM +0530, Laxman Dewangan wrote:
> > > On Thursday 12 January 2017 09:45 PM, Thierry Reding wrote:
> > > > 
> > > > +	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;
> > > What if we want to reset via the Tegra and not through PMIC?
> > In that case I assume we either don't have a PMIC, or we should not add
> > the system-power-controller device tree property to the PMIC node. In
> > the latter case this driver won't install any power off or restart
> > callbacks.
> 
> 
> We need reset from Tegra and power of from PMIC. Tegra does not support
> power OFF.

Can you explain in more details what exactly the combination is that we
need to support? Why can't the PMIC both be the reset and power off
controller? Are there any advantages to resetting with the SoC rather
than the PMIC?

Thierry

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

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

[Sebastian Reichel]

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

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

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

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

[Guenter Roeck]

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

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

[Thierry Reding]

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

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

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

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

[Sebastian Reichel]

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

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

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

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

[Thierry Reding]

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

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

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

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

[Sebastian Reichel]

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

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

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

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

[Guenter Roeck]

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

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

[Guenter Roeck]

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

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

[Sebastian Reichel]

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

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

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

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

[Guenter Roeck]

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

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

[Sebastian Reichel]

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

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

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