[PATCH 0/4] power: axp: Move poweroff to DM_PMIC/SYSRESET driver

[PATCH 0/4] power: axp: Move poweroff to DM_PMIC/SYSRESET driver

[Samuel Holland]

This series replaces the do_poweroff() implementation from the non-DM
AXP PMIC drivers with a sysreset uclass device, if sysreset is providing
its own do_poweroff() definition.

This depends on the following series, which adds the AXP DM_PMIC driver:
https://patchwork.ozlabs.org/project/uboot/list/?series=259089

Congratulations for making it this far! :) If you apply this series,
along with all of the other sysreset and watchdog series I sent this
weekend, and further enable three Kconfig options:
  SYSRESET
  SYSRESET_WATCHDOG
  SYSRESET_WATCHDOG_AUTO
you will have removed exactly one (1) call to pmic_bus_write() from
U-Boot, hopefully without breaking anything else. Once all of these
series land, I will send a patch enabling those options, and we will be
one quarter of the way to removing pmic_bus from U-Boot proper.

Note that because SYSRESET is disabled by default, each of the other
series I sent can be applied independently.


Samuel Holland (4):
  include: axp_pmic: Add missing header guard definition
  include: axp_pmic: Include headers for all variants
  power: axp: Avoid do_poweroff conflict with sysreset
  power: pmic: axp: Implement poweroff via sysreset

 drivers/power/axp152.c     |  2 ++
 drivers/power/axp209.c     |  2 ++
 drivers/power/axp221.c     |  2 ++
 drivers/power/axp305.c     |  2 +-
 drivers/power/axp809.c     |  2 ++
 drivers/power/axp818.c     |  2 ++
 drivers/power/pmic/Kconfig |  2 ++
 drivers/power/pmic/axp.c   | 36 ++++++++++++++++++++++++++++++++++++
 include/axp152.h           |  2 ++
 include/axp209.h           |  2 ++
 include/axp221.h           |  2 ++
 include/axp809.h           |  2 ++
 include/axp818.h           |  2 ++
 include/axp_pmic.h         | 13 +------------
 14 files changed, 60 insertions(+), 13 deletions(-)

-- 
2.31.1

[PATCH 1/4] include: axp_pmic: Add missing header guard definition

[Samuel Holland]

This header attempted to avoid multiple inclusion using a header guard.
But the preprocessor symbol was never defined, so the guard had no
effect. Fix this by defining the symbol.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 include/axp_pmic.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/axp_pmic.h b/include/axp_pmic.h
index 46a017d2efa..2eab18937bc 100644
--- a/include/axp_pmic.h
+++ b/include/axp_pmic.h
@@ -5,6 +5,7 @@
  * X-Powers AX Power Management IC support header
  */
 #ifndef _AXP_PMIC_H_
+#define _AXP_PMIC_H_
 
 #include <stdbool.h>
 
-- 
2.31.1

[PATCH 2/4] include: axp_pmic: Include headers for all variants

[Samuel Holland]

A single DM-based driver should be able to support some feature for
several PMIC variants where the interface is the same. For example,
all PMIC variants use the same register bit to trigger poweroff.

However, currently only definitions for a single PMIC are available at
a time. This requires drivers to use #ifdefs and different indentifiers
for each variant they support.

Let's simplify this by making register definitions for all variants
available from the header. Then no preprocessor conditions are needed;
the driver can use the register definition from any variant that
supports the relevant feature.

An exception is the GPIO-related definitions, which do not use unique
identifiers. So for now, keep them like before. They will be cleaned up
along with the GPIO driver.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 include/axp152.h   |  2 ++
 include/axp209.h   |  2 ++
 include/axp221.h   |  2 ++
 include/axp809.h   |  2 ++
 include/axp818.h   |  2 ++
 include/axp_pmic.h | 12 ------------
 6 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/include/axp152.h b/include/axp152.h
index c81f172502c..10d845fec42 100644
--- a/include/axp152.h
+++ b/include/axp152.h
@@ -15,6 +15,7 @@ enum axp152_reg {
 #define AXP152_POWEROFF			(1 << 7)
 
 /* For axp_gpio.c */
+#ifdef CONFIG_AXP152_POWER
 #define AXP_GPIO0_CTRL			0x90
 #define AXP_GPIO1_CTRL			0x91
 #define AXP_GPIO2_CTRL			0x92
@@ -24,3 +25,4 @@ enum axp152_reg {
 #define AXP_GPIO_CTRL_INPUT			0x02 /* Input */
 #define AXP_GPIO_STATE			0x97
 #define AXP_GPIO_STATE_OFFSET			0
+#endif
diff --git a/include/axp209.h b/include/axp209.h
index f4f1b2fe56d..30399a8d621 100644
--- a/include/axp209.h
+++ b/include/axp209.h
@@ -74,6 +74,7 @@ enum axp209_reg {
 #define AXP209_POWEROFF			BIT(7)
 
 /* For axp_gpio.c */
+#ifdef CONFIG_AXP209_POWER
 #define AXP_POWER_STATUS		0x00
 #define AXP_POWER_STATUS_VBUS_PRESENT	BIT(5)
 #define AXP_GPIO0_CTRL			0x90
@@ -84,3 +85,4 @@ enum axp209_reg {
 #define AXP_GPIO_CTRL_INPUT		0x02 /* Input */
 #define AXP_GPIO_STATE			0x94
 #define AXP_GPIO_STATE_OFFSET		4
+#endif
diff --git a/include/axp221.h b/include/axp221.h
index caffb910f4b..a02e9b4f645 100644
--- a/include/axp221.h
+++ b/include/axp221.h
@@ -50,6 +50,7 @@
 #define AXP221_SID		0x20
 
 /* For axp_gpio.c */
+#ifdef CONFIG_AXP221_POWER
 #define AXP_POWER_STATUS		0x00
 #define AXP_POWER_STATUS_VBUS_PRESENT		(1 << 5)
 #define AXP_VBUS_IPSOUT			0x30
@@ -63,3 +64,4 @@
 #define AXP_GPIO_CTRL_INPUT			0x02 /* Input */
 #define AXP_GPIO_STATE			0x94
 #define AXP_GPIO_STATE_OFFSET			0
+#endif
diff --git a/include/axp809.h b/include/axp809.h
index 86b29253306..430dbef622b 100644
--- a/include/axp809.h
+++ b/include/axp809.h
@@ -44,6 +44,7 @@
 #define AXP809_SHUTDOWN_POWEROFF	(1 << 7)
 
 /* For axp_gpio.c */
+#ifdef CONFIG_AXP809_POWER
 #define AXP_POWER_STATUS		0x00
 #define AXP_POWER_STATUS_VBUS_PRESENT		(1 << 5)
 #define AXP_VBUS_IPSOUT			0x30
@@ -57,3 +58,4 @@
 #define AXP_GPIO_CTRL_INPUT		0x02 /* Input */
 #define AXP_GPIO_STATE			0x94
 #define AXP_GPIO_STATE_OFFSET		0
+#endif
diff --git a/include/axp818.h b/include/axp818.h
index b16fe0b1527..8bac6b67ca2 100644
--- a/include/axp818.h
+++ b/include/axp818.h
@@ -58,6 +58,7 @@
 #define AXP818_SHUTDOWN_POWEROFF	(1 << 7)
 
 /* For axp_gpio.c */
+#ifdef CONFIG_AXP818_POWER
 #define AXP_POWER_STATUS		0x00
 #define AXP_POWER_STATUS_VBUS_PRESENT		(1 << 5)
 #define AXP_VBUS_IPSOUT			0x30
@@ -71,3 +72,4 @@
 #define AXP_GPIO_CTRL_INPUT		0x02 /* Input */
 #define AXP_GPIO_STATE			0x94
 #define AXP_GPIO_STATE_OFFSET		0
+#endif
diff --git a/include/axp_pmic.h b/include/axp_pmic.h
index 2eab18937bc..01ebba63479 100644
--- a/include/axp_pmic.h
+++ b/include/axp_pmic.h
@@ -9,24 +9,12 @@
 
 #include <stdbool.h>
 
-#ifdef CONFIG_AXP152_POWER
 #include <axp152.h>
-#endif
-#ifdef CONFIG_AXP209_POWER
 #include <axp209.h>
-#endif
-#ifdef CONFIG_AXP221_POWER
 #include <axp221.h>
-#endif
-#ifdef CONFIG_AXP305_POWER
 #include <axp305.h>
-#endif
-#ifdef CONFIG_AXP809_POWER
 #include <axp809.h>
-#endif
-#ifdef CONFIG_AXP818_POWER
 #include <axp818.h>
-#endif
 
 #define AXP_PMIC_MODE_REG		0x3e
 #define AXP_PMIC_MODE_I2C		0x00
-- 
2.31.1

[PATCH 3/4] power: axp: Avoid do_poweroff conflict with sysreset

[Samuel Holland]

The sysreset uclass has an option to provide the do_poweroff() function.
When that option is enabled, the AXP power drivers should not provide
their own definition.

For the AXP305, which is paired with 64-bit systems where TF-A provides
PSCI, there is another possible conflict with the PSCI firmware driver.
This driver can be enabled even if CONFIG_PSCI_RESET is disabled, so
make sure to use the right symbol in the condition.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 drivers/power/axp152.c | 2 ++
 drivers/power/axp209.c | 2 ++
 drivers/power/axp221.c | 2 ++
 drivers/power/axp305.c | 2 +-
 drivers/power/axp809.c | 2 ++
 drivers/power/axp818.c | 2 ++
 6 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/power/axp152.c b/drivers/power/axp152.c
index d6e36125c12..a93987c1538 100644
--- a/drivers/power/axp152.c
+++ b/drivers/power/axp152.c
@@ -79,6 +79,7 @@ int axp_init(void)
 	return 0;
 }
 
+#if !IS_ENABLED(CONFIG_SYSRESET_CMD_POWEROFF)
 int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
 	pmic_bus_write(AXP152_SHUTDOWN, AXP152_POWEROFF);
@@ -89,3 +90,4 @@ int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	/* not reached */
 	return 0;
 }
+#endif
diff --git a/drivers/power/axp209.c b/drivers/power/axp209.c
index ade531940b9..3447b9f0113 100644
--- a/drivers/power/axp209.c
+++ b/drivers/power/axp209.c
@@ -230,6 +230,7 @@ int axp_init(void)
 	return 0;
 }
 
+#if !IS_ENABLED(CONFIG_SYSRESET_CMD_POWEROFF)
 int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
 	pmic_bus_write(AXP209_SHUTDOWN, AXP209_POWEROFF);
@@ -240,3 +241,4 @@ int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	/* not reached */
 	return 0;
 }
+#endif
diff --git a/drivers/power/axp221.c b/drivers/power/axp221.c
index 3446fe7365d..d251c314b98 100644
--- a/drivers/power/axp221.c
+++ b/drivers/power/axp221.c
@@ -264,6 +264,7 @@ int axp_get_sid(unsigned int *sid)
 	return 0;
 }
 
+#if !IS_ENABLED(CONFIG_SYSRESET_CMD_POWEROFF)
 int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
 	pmic_bus_write(AXP221_SHUTDOWN, AXP221_SHUTDOWN_POWEROFF);
@@ -274,3 +275,4 @@ int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	/* not reached */
 	return 0;
 }
+#endif
diff --git a/drivers/power/axp305.c b/drivers/power/axp305.c
index 0191e4d427e..049ef07f746 100644
--- a/drivers/power/axp305.c
+++ b/drivers/power/axp305.c
@@ -69,7 +69,7 @@ int axp_init(void)
 	return ret;
 }
 
-#ifndef CONFIG_PSCI_RESET
+#if !CONFIG_IS_ENABLED(ARM_PSCI_FW) && !IS_ENABLED(CONFIG_SYSRESET_CMD_POWEROFF)
 int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
 	pmic_bus_write(AXP305_SHUTDOWN, AXP305_POWEROFF);
diff --git a/drivers/power/axp809.c b/drivers/power/axp809.c
index 6323492b66d..49584e502fe 100644
--- a/drivers/power/axp809.c
+++ b/drivers/power/axp809.c
@@ -220,6 +220,7 @@ int axp_init(void)
 	return pmic_bus_init();
 }
 
+#if !IS_ENABLED(CONFIG_SYSRESET_CMD_POWEROFF)
 int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
 	pmic_bus_write(AXP809_SHUTDOWN, AXP809_SHUTDOWN_POWEROFF);
@@ -230,3 +231,4 @@ int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	/* not reached */
 	return 0;
 }
+#endif
diff --git a/drivers/power/axp818.c b/drivers/power/axp818.c
index 0531707c8aa..0960786f4a7 100644
--- a/drivers/power/axp818.c
+++ b/drivers/power/axp818.c
@@ -256,6 +256,7 @@ int axp_init(void)
 	return 0;
 }
 
+#if !IS_ENABLED(CONFIG_SYSRESET_CMD_POWEROFF)
 int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
 	pmic_bus_write(AXP818_SHUTDOWN, AXP818_SHUTDOWN_POWEROFF);
@@ -266,3 +267,4 @@ int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	/* not reached */
 	return 0;
 }
+#endif
-- 
2.31.1

[PATCH 4/4] power: pmic: axp: Implement poweroff via sysreset

[Samuel Holland]

The AXP PMICs have the ability to power off the system. The existing
code for this is duplicated for each PMIC variant, and uses the legacy
non-DM "pmic_bus" interface. When SYSRESET is enabled, this can all be
replaced with a sysreset device using the DM_PMIC interface.

Since the trigger bit is the same on all PMIC variants, use the register
definitions from the oldest supported PMIC.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 drivers/power/pmic/Kconfig |  2 ++
 drivers/power/pmic/axp.c   | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/drivers/power/pmic/Kconfig b/drivers/power/pmic/Kconfig
index 6a3e82e1be7..983fe135157 100644
--- a/drivers/power/pmic/Kconfig
+++ b/drivers/power/pmic/Kconfig
@@ -60,6 +60,8 @@ config PMIC_ACT8846
 config PMIC_AXP
 	bool "Enable Driver Model for X-Powers AXP PMICs"
 	depends on DM_I2C
+	select SYSRESET_CMD_POWEROFF if SYSRESET && CMD_POWEROFF
+	imply CMD_POWEROFF if SYSRESET
 	help
 	  This config enables driver-model PMIC uclass features for
 	  X-Powers AXP152, AXP2xx, and AXP8xx PMICs.
diff --git a/drivers/power/pmic/axp.c b/drivers/power/pmic/axp.c
index 7720e1afd9b..0f2b24a8b5f 100644
--- a/drivers/power/pmic/axp.c
+++ b/drivers/power/pmic/axp.c
@@ -1,8 +1,37 @@
 // SPDX-License-Identifier: GPL-2.0+
 
+#include <axp_pmic.h>
 #include <dm.h>
+#include <dm/lists.h>
 #include <i2c.h>
 #include <power/pmic.h>
+#include <sysreset.h>
+
+#if CONFIG_IS_ENABLED(SYSRESET)
+static int axp_sysreset_request(struct udevice *dev, enum sysreset_t type)
+{
+	int ret;
+
+	if (type != SYSRESET_POWER_OFF)
+		return -EPROTONOSUPPORT;
+
+	ret = pmic_clrsetbits(dev->parent, AXP152_SHUTDOWN, 0, AXP152_POWEROFF);
+	if (ret < 0)
+		return ret;
+
+	return -EINPROGRESS;
+}
+
+static struct sysreset_ops axp_sysreset_ops = {
+	.request	= axp_sysreset_request,
+};
+
+U_BOOT_DRIVER(axp_sysreset) = {
+	.name		= "axp_sysreset",
+	.id		= UCLASS_SYSRESET,
+	.ops		= &axp_sysreset_ops,
+};
+#endif
 
 static int axp_pmic_reg_count(struct udevice *dev)
 {
@@ -24,6 +53,13 @@ static int axp_pmic_bind(struct udevice *dev)
 	if (ret)
 		return ret;
 
+	if (CONFIG_IS_ENABLED(SYSRESET)) {
+		ret = device_bind_driver_to_node(dev, "axp_sysreset", "axp_sysreset",
+						 dev_ofnode(dev), NULL);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
-- 
2.31.1

Re: [PATCH 1/4] include: axp_pmic: Add missing header guard definition

[Andre Przywara]

On Sun, 22 Aug 2021 18:18:03 -0500
Samuel Holland <samuel@sholland.org> wrote:

> This header attempted to avoid multiple inclusion using a header guard.
> But the preprocessor symbol was never defined, so the guard had no
> effect. Fix this by defining the symbol.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
> 
>  include/axp_pmic.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/axp_pmic.h b/include/axp_pmic.h
> index 46a017d2efa..2eab18937bc 100644
> --- a/include/axp_pmic.h
> +++ b/include/axp_pmic.h
> @@ -5,6 +5,7 @@
>   * X-Powers AX Power Management IC support header
>   */
>  #ifndef _AXP_PMIC_H_
> +#define _AXP_PMIC_H_
>  
>  #include <stdbool.h>
>  

Re: [PATCH 2/4] include: axp_pmic: Include headers for all variants

[Andre Przywara]

On Sun, 22 Aug 2021 18:18:04 -0500
Samuel Holland <samuel@sholland.org> wrote:

> A single DM-based driver should be able to support some feature for
> several PMIC variants where the interface is the same. For example,
> all PMIC variants use the same register bit to trigger poweroff.
> 
> However, currently only definitions for a single PMIC are available at
> a time. This requires drivers to use #ifdefs and different indentifiers
> for each variant they support.
> 
> Let's simplify this by making register definitions for all variants
> available from the header. Then no preprocessor conditions are needed;
> the driver can use the register definition from any variant that
> supports the relevant feature.
> 
> An exception is the GPIO-related definitions, which do not use unique
> identifiers. So for now, keep them like before. They will be cleaned up
> along with the GPIO driver.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
> 
>  include/axp152.h   |  2 ++
>  include/axp209.h   |  2 ++
>  include/axp221.h   |  2 ++
>  include/axp809.h   |  2 ++
>  include/axp818.h   |  2 ++
>  include/axp_pmic.h | 12 ------------
>  6 files changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/include/axp152.h b/include/axp152.h
> index c81f172502c..10d845fec42 100644
> --- a/include/axp152.h
> +++ b/include/axp152.h
> @@ -15,6 +15,7 @@ enum axp152_reg {
>  #define AXP152_POWEROFF			(1 << 7)
>  
>  /* For axp_gpio.c */
> +#ifdef CONFIG_AXP152_POWER
>  #define AXP_GPIO0_CTRL			0x90
>  #define AXP_GPIO1_CTRL			0x91
>  #define AXP_GPIO2_CTRL			0x92
> @@ -24,3 +25,4 @@ enum axp152_reg {
>  #define AXP_GPIO_CTRL_INPUT			0x02 /* Input */
>  #define AXP_GPIO_STATE			0x97
>  #define AXP_GPIO_STATE_OFFSET			0
> +#endif
> diff --git a/include/axp209.h b/include/axp209.h
> index f4f1b2fe56d..30399a8d621 100644
> --- a/include/axp209.h
> +++ b/include/axp209.h
> @@ -74,6 +74,7 @@ enum axp209_reg {
>  #define AXP209_POWEROFF			BIT(7)
>  
>  /* For axp_gpio.c */
> +#ifdef CONFIG_AXP209_POWER
>  #define AXP_POWER_STATUS		0x00
>  #define AXP_POWER_STATUS_VBUS_PRESENT	BIT(5)
>  #define AXP_GPIO0_CTRL			0x90
> @@ -84,3 +85,4 @@ enum axp209_reg {
>  #define AXP_GPIO_CTRL_INPUT		0x02 /* Input */
>  #define AXP_GPIO_STATE			0x94
>  #define AXP_GPIO_STATE_OFFSET		4
> +#endif
> diff --git a/include/axp221.h b/include/axp221.h
> index caffb910f4b..a02e9b4f645 100644
> --- a/include/axp221.h
> +++ b/include/axp221.h
> @@ -50,6 +50,7 @@
>  #define AXP221_SID		0x20
>  
>  /* For axp_gpio.c */
> +#ifdef CONFIG_AXP221_POWER
>  #define AXP_POWER_STATUS		0x00
>  #define AXP_POWER_STATUS_VBUS_PRESENT		(1 << 5)
>  #define AXP_VBUS_IPSOUT			0x30
> @@ -63,3 +64,4 @@
>  #define AXP_GPIO_CTRL_INPUT			0x02 /* Input */
>  #define AXP_GPIO_STATE			0x94
>  #define AXP_GPIO_STATE_OFFSET			0
> +#endif
> diff --git a/include/axp809.h b/include/axp809.h
> index 86b29253306..430dbef622b 100644
> --- a/include/axp809.h
> +++ b/include/axp809.h
> @@ -44,6 +44,7 @@
>  #define AXP809_SHUTDOWN_POWEROFF	(1 << 7)
>  
>  /* For axp_gpio.c */
> +#ifdef CONFIG_AXP809_POWER
>  #define AXP_POWER_STATUS		0x00
>  #define AXP_POWER_STATUS_VBUS_PRESENT		(1 << 5)
>  #define AXP_VBUS_IPSOUT			0x30
> @@ -57,3 +58,4 @@
>  #define AXP_GPIO_CTRL_INPUT		0x02 /* Input */
>  #define AXP_GPIO_STATE			0x94
>  #define AXP_GPIO_STATE_OFFSET		0
> +#endif
> diff --git a/include/axp818.h b/include/axp818.h
> index b16fe0b1527..8bac6b67ca2 100644
> --- a/include/axp818.h
> +++ b/include/axp818.h
> @@ -58,6 +58,7 @@
>  #define AXP818_SHUTDOWN_POWEROFF	(1 << 7)
>  
>  /* For axp_gpio.c */
> +#ifdef CONFIG_AXP818_POWER
>  #define AXP_POWER_STATUS		0x00
>  #define AXP_POWER_STATUS_VBUS_PRESENT		(1 << 5)
>  #define AXP_VBUS_IPSOUT			0x30
> @@ -71,3 +72,4 @@
>  #define AXP_GPIO_CTRL_INPUT		0x02 /* Input */
>  #define AXP_GPIO_STATE			0x94
>  #define AXP_GPIO_STATE_OFFSET		0
> +#endif
> diff --git a/include/axp_pmic.h b/include/axp_pmic.h
> index 2eab18937bc..01ebba63479 100644
> --- a/include/axp_pmic.h
> +++ b/include/axp_pmic.h
> @@ -9,24 +9,12 @@
>  
>  #include <stdbool.h>
>  
> -#ifdef CONFIG_AXP152_POWER
>  #include <axp152.h>
> -#endif
> -#ifdef CONFIG_AXP209_POWER
>  #include <axp209.h>
> -#endif
> -#ifdef CONFIG_AXP221_POWER
>  #include <axp221.h>
> -#endif
> -#ifdef CONFIG_AXP305_POWER
>  #include <axp305.h>
> -#endif
> -#ifdef CONFIG_AXP809_POWER
>  #include <axp809.h>
> -#endif
> -#ifdef CONFIG_AXP818_POWER
>  #include <axp818.h>
> -#endif
>  
>  #define AXP_PMIC_MODE_REG		0x3e
>  #define AXP_PMIC_MODE_I2C		0x00

Re: [PATCH 3/4] power: axp: Avoid do_poweroff conflict with sysreset

[Andre Przywara]

On Sun, 22 Aug 2021 18:18:05 -0500
Samuel Holland <samuel@sholland.org> wrote:

Hi Samuel,

> The sysreset uclass has an option to provide the do_poweroff() function.
> When that option is enabled, the AXP power drivers should not provide
> their own definition.
> 
> For the AXP305, which is paired with 64-bit systems where TF-A provides
> PSCI, there is another possible conflict with the PSCI firmware driver.
> This driver can be enabled even if CONFIG_PSCI_RESET is disabled, so
> make sure to use the right symbol in the condition.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>

Not a big fan of adding #ifdef's, but it looks like these
implementations of do_poweroff() can eventually be removed, when
SYSRESET works everywhere? Or is there a reason we cannot select
SYSRESET later on, and would still need do_poweroff from those AXP
drivers directly?

Anyway:
Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
> 
>  drivers/power/axp152.c | 2 ++
>  drivers/power/axp209.c | 2 ++
>  drivers/power/axp221.c | 2 ++
>  drivers/power/axp305.c | 2 +-
>  drivers/power/axp809.c | 2 ++
>  drivers/power/axp818.c | 2 ++
>  6 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/axp152.c b/drivers/power/axp152.c
> index d6e36125c12..a93987c1538 100644
> --- a/drivers/power/axp152.c
> +++ b/drivers/power/axp152.c
> @@ -79,6 +79,7 @@ int axp_init(void)
>  	return 0;
>  }
>  
> +#if !IS_ENABLED(CONFIG_SYSRESET_CMD_POWEROFF)
>  int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>  {
>  	pmic_bus_write(AXP152_SHUTDOWN, AXP152_POWEROFF);
> @@ -89,3 +90,4 @@ int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>  	/* not reached */
>  	return 0;
>  }
> +#endif
> diff --git a/drivers/power/axp209.c b/drivers/power/axp209.c
> index ade531940b9..3447b9f0113 100644
> --- a/drivers/power/axp209.c
> +++ b/drivers/power/axp209.c
> @@ -230,6 +230,7 @@ int axp_init(void)
>  	return 0;
>  }
>  
> +#if !IS_ENABLED(CONFIG_SYSRESET_CMD_POWEROFF)
>  int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>  {
>  	pmic_bus_write(AXP209_SHUTDOWN, AXP209_POWEROFF);
> @@ -240,3 +241,4 @@ int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>  	/* not reached */
>  	return 0;
>  }
> +#endif
> diff --git a/drivers/power/axp221.c b/drivers/power/axp221.c
> index 3446fe7365d..d251c314b98 100644
> --- a/drivers/power/axp221.c
> +++ b/drivers/power/axp221.c
> @@ -264,6 +264,7 @@ int axp_get_sid(unsigned int *sid)
>  	return 0;
>  }
>  
> +#if !IS_ENABLED(CONFIG_SYSRESET_CMD_POWEROFF)
>  int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>  {
>  	pmic_bus_write(AXP221_SHUTDOWN, AXP221_SHUTDOWN_POWEROFF);
> @@ -274,3 +275,4 @@ int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>  	/* not reached */
>  	return 0;
>  }
> +#endif
> diff --git a/drivers/power/axp305.c b/drivers/power/axp305.c
> index 0191e4d427e..049ef07f746 100644
> --- a/drivers/power/axp305.c
> +++ b/drivers/power/axp305.c
> @@ -69,7 +69,7 @@ int axp_init(void)
>  	return ret;
>  }
>  
> -#ifndef CONFIG_PSCI_RESET
> +#if !CONFIG_IS_ENABLED(ARM_PSCI_FW) && !IS_ENABLED(CONFIG_SYSRESET_CMD_POWEROFF)
>  int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>  {
>  	pmic_bus_write(AXP305_SHUTDOWN, AXP305_POWEROFF);
> diff --git a/drivers/power/axp809.c b/drivers/power/axp809.c
> index 6323492b66d..49584e502fe 100644
> --- a/drivers/power/axp809.c
> +++ b/drivers/power/axp809.c
> @@ -220,6 +220,7 @@ int axp_init(void)
>  	return pmic_bus_init();
>  }
>  
> +#if !IS_ENABLED(CONFIG_SYSRESET_CMD_POWEROFF)
>  int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>  {
>  	pmic_bus_write(AXP809_SHUTDOWN, AXP809_SHUTDOWN_POWEROFF);
> @@ -230,3 +231,4 @@ int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>  	/* not reached */
>  	return 0;
>  }
> +#endif
> diff --git a/drivers/power/axp818.c b/drivers/power/axp818.c
> index 0531707c8aa..0960786f4a7 100644
> --- a/drivers/power/axp818.c
> +++ b/drivers/power/axp818.c
> @@ -256,6 +256,7 @@ int axp_init(void)
>  	return 0;
>  }
>  
> +#if !IS_ENABLED(CONFIG_SYSRESET_CMD_POWEROFF)
>  int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>  {
>  	pmic_bus_write(AXP818_SHUTDOWN, AXP818_SHUTDOWN_POWEROFF);
> @@ -266,3 +267,4 @@ int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>  	/* not reached */
>  	return 0;
>  }
> +#endif

Re: [PATCH 4/4] power: pmic: axp: Implement poweroff via sysreset

[Andre Przywara]

On Sun, 22 Aug 2021 18:18:06 -0500
Samuel Holland <samuel@sholland.org> wrote:

Hi Samuel,

> The AXP PMICs have the ability to power off the system. The existing
> code for this is duplicated for each PMIC variant, and uses the legacy
> non-DM "pmic_bus" interface. When SYSRESET is enabled, this can all be
> replaced with a sysreset device using the DM_PMIC interface.
> 
> Since the trigger bit is the same on all PMIC variants, use the register
> definitions from the oldest supported PMIC.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>

This looks alright, but doesn't apply anymore, since you removed
axp_pmic_bind() in v2 of the DM_I2C series.

Can you rebase this series (or just 4/4) on top of master?
I will take the first three patches anyway, they apply cleanly.

Thanks,
Andre


> ---
> 
>  drivers/power/pmic/Kconfig |  2 ++
>  drivers/power/pmic/axp.c   | 36 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/drivers/power/pmic/Kconfig b/drivers/power/pmic/Kconfig
> index 6a3e82e1be7..983fe135157 100644
> --- a/drivers/power/pmic/Kconfig
> +++ b/drivers/power/pmic/Kconfig
> @@ -60,6 +60,8 @@ config PMIC_ACT8846
>  config PMIC_AXP
>  	bool "Enable Driver Model for X-Powers AXP PMICs"
>  	depends on DM_I2C
> +	select SYSRESET_CMD_POWEROFF if SYSRESET && CMD_POWEROFF
> +	imply CMD_POWEROFF if SYSRESET
>  	help
>  	  This config enables driver-model PMIC uclass features for
>  	  X-Powers AXP152, AXP2xx, and AXP8xx PMICs.
> diff --git a/drivers/power/pmic/axp.c b/drivers/power/pmic/axp.c
> index 7720e1afd9b..0f2b24a8b5f 100644
> --- a/drivers/power/pmic/axp.c
> +++ b/drivers/power/pmic/axp.c
> @@ -1,8 +1,37 @@
>  // SPDX-License-Identifier: GPL-2.0+
>  
> +#include <axp_pmic.h>
>  #include <dm.h>
> +#include <dm/lists.h>
>  #include <i2c.h>
>  #include <power/pmic.h>
> +#include <sysreset.h>
> +
> +#if CONFIG_IS_ENABLED(SYSRESET)
> +static int axp_sysreset_request(struct udevice *dev, enum sysreset_t type)
> +{
> +	int ret;
> +
> +	if (type != SYSRESET_POWER_OFF)
> +		return -EPROTONOSUPPORT;
> +
> +	ret = pmic_clrsetbits(dev->parent, AXP152_SHUTDOWN, 0, AXP152_POWEROFF);
> +	if (ret < 0)
> +		return ret;
> +
> +	return -EINPROGRESS;
> +}
> +
> +static struct sysreset_ops axp_sysreset_ops = {
> +	.request	= axp_sysreset_request,
> +};
> +
> +U_BOOT_DRIVER(axp_sysreset) = {
> +	.name		= "axp_sysreset",
> +	.id		= UCLASS_SYSRESET,
> +	.ops		= &axp_sysreset_ops,
> +};
> +#endif
>  
>  static int axp_pmic_reg_count(struct udevice *dev)
>  {
> @@ -24,6 +53,13 @@ static int axp_pmic_bind(struct udevice *dev)
>  	if (ret)
>  		return ret;
>  
> +	if (CONFIG_IS_ENABLED(SYSRESET)) {
> +		ret = device_bind_driver_to_node(dev, "axp_sysreset", "axp_sysreset",
> +						 dev_ofnode(dev), NULL);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }
>  

Re: [PATCH 3/4] power: axp: Avoid do_poweroff conflict with sysreset

[Samuel Holland]

On 10/24/21 7:38 PM, Andre Przywara wrote:
> On Sun, 22 Aug 2021 18:18:05 -0500
> Samuel Holland <samuel@sholland.org> wrote:
> 
> Hi Samuel,
> 
>> The sysreset uclass has an option to provide the do_poweroff() function.
>> When that option is enabled, the AXP power drivers should not provide
>> their own definition.
>>
>> For the AXP305, which is paired with 64-bit systems where TF-A provides
>> PSCI, there is another possible conflict with the PSCI firmware driver.
>> This driver can be enabled even if CONFIG_PSCI_RESET is disabled, so
>> make sure to use the right symbol in the condition.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
> 
> Not a big fan of adding #ifdef's, but it looks like these
> implementations of do_poweroff() can eventually be removed, when
> SYSRESET works everywhere? Or is there a reason we cannot select
> SYSRESET later on, and would still need do_poweroff from those AXP
> drivers directly?

Yes, the intention is to remove these function as soon as SYSRESET is enabled
everywhere. We should be able to do that once the watchdog driver is hooked up.

I will send a rebased patch 4.

Regards,
Samuel

> Anyway:
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> 
> Cheers,
> Andre
> 
>> ---
>>
>>  drivers/power/axp152.c | 2 ++
>>  drivers/power/axp209.c | 2 ++
>>  drivers/power/axp221.c | 2 ++
>>  drivers/power/axp305.c | 2 +-
>>  drivers/power/axp809.c | 2 ++
>>  drivers/power/axp818.c | 2 ++
>>  6 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/power/axp152.c b/drivers/power/axp152.c
>> index d6e36125c12..a93987c1538 100644
>> --- a/drivers/power/axp152.c
>> +++ b/drivers/power/axp152.c
>> @@ -79,6 +79,7 @@ int axp_init(void)
>>  	return 0;
>>  }
>>  
>> +#if !IS_ENABLED(CONFIG_SYSRESET_CMD_POWEROFF)
>>  int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>>  {
>>  	pmic_bus_write(AXP152_SHUTDOWN, AXP152_POWEROFF);
>> @@ -89,3 +90,4 @@ int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>>  	/* not reached */
>>  	return 0;
>>  }
>> +#endif
>> diff --git a/drivers/power/axp209.c b/drivers/power/axp209.c
>> index ade531940b9..3447b9f0113 100644
>> --- a/drivers/power/axp209.c
>> +++ b/drivers/power/axp209.c
>> @@ -230,6 +230,7 @@ int axp_init(void)
>>  	return 0;
>>  }
>>  
>> +#if !IS_ENABLED(CONFIG_SYSRESET_CMD_POWEROFF)
>>  int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>>  {
>>  	pmic_bus_write(AXP209_SHUTDOWN, AXP209_POWEROFF);
>> @@ -240,3 +241,4 @@ int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>>  	/* not reached */
>>  	return 0;
>>  }
>> +#endif
>> diff --git a/drivers/power/axp221.c b/drivers/power/axp221.c
>> index 3446fe7365d..d251c314b98 100644
>> --- a/drivers/power/axp221.c
>> +++ b/drivers/power/axp221.c
>> @@ -264,6 +264,7 @@ int axp_get_sid(unsigned int *sid)
>>  	return 0;
>>  }
>>  
>> +#if !IS_ENABLED(CONFIG_SYSRESET_CMD_POWEROFF)
>>  int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>>  {
>>  	pmic_bus_write(AXP221_SHUTDOWN, AXP221_SHUTDOWN_POWEROFF);
>> @@ -274,3 +275,4 @@ int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>>  	/* not reached */
>>  	return 0;
>>  }
>> +#endif
>> diff --git a/drivers/power/axp305.c b/drivers/power/axp305.c
>> index 0191e4d427e..049ef07f746 100644
>> --- a/drivers/power/axp305.c
>> +++ b/drivers/power/axp305.c
>> @@ -69,7 +69,7 @@ int axp_init(void)
>>  	return ret;
>>  }
>>  
>> -#ifndef CONFIG_PSCI_RESET
>> +#if !CONFIG_IS_ENABLED(ARM_PSCI_FW) && !IS_ENABLED(CONFIG_SYSRESET_CMD_POWEROFF)
>>  int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>>  {
>>  	pmic_bus_write(AXP305_SHUTDOWN, AXP305_POWEROFF);
>> diff --git a/drivers/power/axp809.c b/drivers/power/axp809.c
>> index 6323492b66d..49584e502fe 100644
>> --- a/drivers/power/axp809.c
>> +++ b/drivers/power/axp809.c
>> @@ -220,6 +220,7 @@ int axp_init(void)
>>  	return pmic_bus_init();
>>  }
>>  
>> +#if !IS_ENABLED(CONFIG_SYSRESET_CMD_POWEROFF)
>>  int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>>  {
>>  	pmic_bus_write(AXP809_SHUTDOWN, AXP809_SHUTDOWN_POWEROFF);
>> @@ -230,3 +231,4 @@ int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>>  	/* not reached */
>>  	return 0;
>>  }
>> +#endif
>> diff --git a/drivers/power/axp818.c b/drivers/power/axp818.c
>> index 0531707c8aa..0960786f4a7 100644
>> --- a/drivers/power/axp818.c
>> +++ b/drivers/power/axp818.c
>> @@ -256,6 +256,7 @@ int axp_init(void)
>>  	return 0;
>>  }
>>  
>> +#if !IS_ENABLED(CONFIG_SYSRESET_CMD_POWEROFF)
>>  int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>>  {
>>  	pmic_bus_write(AXP818_SHUTDOWN, AXP818_SHUTDOWN_POWEROFF);
>> @@ -266,3 +267,4 @@ int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>>  	/* not reached */
>>  	return 0;
>>  }
>> +#endif
>