| Message ID | 20170112161507.23774-1-thierry.reding@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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"); -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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
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
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
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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
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
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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");