[PATCH 0/4] power: axp: Move poweroff to DM_PMIC/SYSRESET driver
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(-)
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>
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>
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
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
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
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
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
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; }
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;
}
participants (2)
-
Andre Przywara -
Samuel Holland