[PATCH 0/6] power: pmic: sunxi: consolidate AXP SPL drivers
Hi,
this is the first series in an attempt to clean up the X-Powers AXP PMIC drivers used by the SPL for sunxi boards. So far we have a separate driver file for each AXP variant, but the code was largely the same, just differing by the regulator ranges.
This adds a new generic driver, which reads the regulator description from an array of structs. This is similar to how the DM AXP driver used for U-Boot proper works, but is simplified, since we won't need the full feature set for the SPL, and we want to keep the code size small.
To help bisect-ability, and to simplify review, each of the simpler AXP drivers covered by this approach is handled in a separate patch. We just convert the AXP313, AXP305, AXP717 for now, and on the way add support for the AXP707, just because it's now very easy, and we will need it soon enough. The other AXP SPL drivers are more complex, and support more regulators, but my hunch is that we really just need the DC/DC converters in the SPL. However I need to prove and test this, so I will convert the other AXP chips later.
Please have a look and comment whether the approach in general is a good idea.
Cheers, Andre
Andre Przywara (6): power: pmic: sunxi: only build AXP drivers for SPL power: pmic: sunxi: introduce generic SPL AXP DC/DC driver power: pmic: sunxi: replace AXP717 SPL driver power: pmic: sunxi: use generic AXP SPL driver for AXP313 power: pmic: sunxi: use generic AXP SPL driver for AXP305 power: pmic: sunxi: add AXP707 support
drivers/power/Makefile | 8 +- drivers/power/axp305.c | 82 ------------------ drivers/power/axp313.c | 133 ----------------------------- drivers/power/axp717.c | 92 -------------------- drivers/power/axp_spl.c | 184 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 189 insertions(+), 310 deletions(-) delete mode 100644 drivers/power/axp305.c delete mode 100644 drivers/power/axp313.c delete mode 100644 drivers/power/axp717.c create mode 100644 drivers/power/axp_spl.c
The axp<xxx>.c drivers are only used for the SPL, for U-Boot proper we have a separate, DM compliant driver. Mask the build instructions with CONFIG_SPL_BUILD, to avoid them being build for U-Boot proper as well.
Signed-off-by: Andre Przywara andre.przywara@arm.com --- drivers/power/Makefile | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/power/Makefile b/drivers/power/Makefile index 41ebb494fff..9d1f4c65262 100644 --- a/drivers/power/Makefile +++ b/drivers/power/Makefile @@ -8,6 +8,7 @@ obj-$(CONFIG_$(SPL_TPL_)POWER_DOMAIN) += domain/ obj-y += pmic/ obj-y += regulator/
+ifdef CONFIG_SPL_BUILD obj-$(CONFIG_AXP152_POWER) += axp152.o obj-$(CONFIG_AXP209_POWER) += axp209.o obj-$(CONFIG_AXP221_POWER) += axp221.o @@ -16,6 +17,7 @@ obj-$(CONFIG_AXP313_POWER) += axp313.o obj-$(CONFIG_AXP717_POWER) += axp717.o obj-$(CONFIG_AXP809_POWER) += axp809.o obj-$(CONFIG_AXP818_POWER) += axp818.o +endif obj-$(CONFIG_EXYNOS_TMU) += exynos-tmu.o obj-$(CONFIG_SY8106A_POWER) += sy8106a.o obj-$(CONFIG_TPS6586X_POWER) += tps6586x.o
So far we had a separate driver file for each AXP PMIC chip that we need to support in the SPL. The code in there was largely similar, but differed in many details.
Based on the idea of the DM AXP driver, introduce a data structure to describe each regulator in a compact way. This is a simplified version of the struct used in the DM driver, as we don't need to support the full voltage range and not every regulator in the SPL. For now we only support the DC/DC buck converters, since that's what we need the SPL to configure, mostly. Also we get rid of the regulator name, and hardcode the regulator number by its position in the array (first is DCDC1, second is DCDC2, etc). We also drop support for the value table, we ideally won't need that for the subset of regulators required. At the end each regulator is described by a 10 bytes struct, so we avoid blowing up the SPL footprint, but still can use generic code.
Each chip is supposed to be described separately, and protected by ifdef's, to only build in the regulators needed for a particular board. We also describe the bits to help identifying the AXP chip, and the shutdown details in that section.
Add a generic driver, that exports axp_set_dcdc<x>() functions to set up the buck converters. For now this just contains the bits for the (new) AXP717, but it's not wired up anywhere yet.
Signed-off-by: Andre Przywara andre.przywara@arm.com --- drivers/power/axp_spl.c | 141 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 141 insertions(+) create mode 100644 drivers/power/axp_spl.c
diff --git a/drivers/power/axp_spl.c b/drivers/power/axp_spl.c new file mode 100644 index 00000000000..36eb6bd0b2a --- /dev/null +++ b/drivers/power/axp_spl.c @@ -0,0 +1,141 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * AXP PMIC SPL driver + * (C) Copyright 2024 Arm Ltd. + */ + +#include <errno.h> +#include <asm/arch/pmic_bus.h> +#include <axp_pmic.h> + +struct axp_reg_desc_spl { + u8 enable_reg; + u8 enable_mask; + u8 volt_reg; + u8 volt_mask; + u16 min_mV; + u16 max_mV; + u8 step_mV; + u8 split; +}; + +#define NA 0xff + +#if defined(CONFIG_AXP717_POWER) /* AXP717 */ + +static const struct axp_reg_desc_spl axp_spl_dcdc_regulators[] = { + { 0x80, BIT(0), 0x83, 0x7f, 500, 1540, 10, 70 }, + { 0x80, BIT(1), 0x84, 0x7f, 500, 1540, 10, 70 }, + { 0x80, BIT(2), 0x85, 0x7f, 500, 1840, 10, 70 }, +}; +#define AXP_CHIP_VERSION 0x0 +#define AXP_CHIP_VERSION_MASK 0x0 +#define AXP_CHIP_ID 0x0 +#define AXP_SHUTDOWN_REG 0x27 +#define AXP_SHUTDOWN_MASK BIT(0) + +#else + + #error "Please define the regulator registers in axp_spl_regulators[]." + +#endif + +static u8 axp_mvolt_to_cfg(int mvolt, const struct axp_reg_desc_spl *reg) +{ + if (mvolt < reg->min_mV) + mvolt = reg->min_mV; + else if (mvolt > reg->max_mV) + mvolt = reg->max_mV; + + mvolt -= reg->min_mV; + + /* voltage in the first range ? */ + if (mvolt <= reg->split * reg->step_mV) + return mvolt / reg->step_mV; + + mvolt -= reg->split * reg->step_mV; + + return reg->split + mvolt / (reg->step_mV * 2); +} + +static int axp_set_dcdc(int dcdc_num, unsigned int mvolt) +{ + const struct axp_reg_desc_spl *reg; + int ret; + + if (dcdc_num < 1 || dcdc_num > ARRAY_SIZE(axp_spl_dcdc_regulators)) + return -EINVAL; + + reg = &axp_spl_dcdc_regulators[dcdc_num - 1]; + + + if (mvolt == 0) + return pmic_bus_clrbits(reg->enable_reg, reg->enable_mask); + + ret = pmic_bus_write(reg->volt_reg, axp_mvolt_to_cfg(mvolt, reg)); + if (ret) + return ret; + + return pmic_bus_setbits(reg->enable_reg, reg->enable_mask); +} + +int axp_set_dcdc1(unsigned int mvolt) +{ + return axp_set_dcdc(1, mvolt); +} + +int axp_set_dcdc2(unsigned int mvolt) +{ + return axp_set_dcdc(2, mvolt); +} + +int axp_set_dcdc3(unsigned int mvolt) +{ + return axp_set_dcdc(3, mvolt); +} + +int axp_set_dcdc4(unsigned int mvolt) +{ + return axp_set_dcdc(4, mvolt); +} + +int axp_set_dcdc5(unsigned int mvolt) +{ + return axp_set_dcdc(5, mvolt); +} + +int axp_init(void) +{ + int ret = pmic_bus_init(); + + if (ret) + return ret; + + if (AXP_CHIP_VERSION_MASK) { + u8 axp_chip_id; + + ret = pmic_bus_read(AXP_CHIP_VERSION, &axp_chip_id); + if (ret) + return ret; + + if ((axp_chip_id & AXP_CHIP_VERSION_MASK) != AXP_CHIP_ID) { + debug("unknown PMIC: 0x%x\n", axp_chip_id); + return -EINVAL; + } + } + + return 0; +} + +#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_setbits(AXP_SHUTDOWN_REG, AXP_SHUTDOWN_MASK); + + /* infinite loop during shutdown */ + while (1) {} + + /* not reached */ + return 0; +} +#endif
We now have a generic AXP SPL driver implementation, that already covers the DC/DC converters of the AXP717 PMIC.
Remove the old, dedicated driver and switch to the new generic driver. This should not introduce any change in behaviour.
Signed-off-by: Andre Przywara andre.przywara@arm.com --- drivers/power/Makefile | 2 +- drivers/power/axp717.c | 92 ------------------------------------------ 2 files changed, 1 insertion(+), 93 deletions(-) delete mode 100644 drivers/power/axp717.c
diff --git a/drivers/power/Makefile b/drivers/power/Makefile index 9d1f4c65262..6df23a81c29 100644 --- a/drivers/power/Makefile +++ b/drivers/power/Makefile @@ -14,7 +14,7 @@ obj-$(CONFIG_AXP209_POWER) += axp209.o obj-$(CONFIG_AXP221_POWER) += axp221.o obj-$(CONFIG_AXP305_POWER) += axp305.o obj-$(CONFIG_AXP313_POWER) += axp313.o -obj-$(CONFIG_AXP717_POWER) += axp717.o +obj-$(CONFIG_AXP717_POWER) += axp_spl.o obj-$(CONFIG_AXP809_POWER) += axp809.o obj-$(CONFIG_AXP818_POWER) += axp818.o endif diff --git a/drivers/power/axp717.c b/drivers/power/axp717.c deleted file mode 100644 index 7c77c09ea8f..00000000000 --- a/drivers/power/axp717.c +++ /dev/null @@ -1,92 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0+ -/* - * AXP717 SPL driver - * (C) Copyright 2024 Arm Ltd. - */ - -#include <common.h> -#include <command.h> -#include <errno.h> -#include <asm/arch/pmic_bus.h> -#include <axp_pmic.h> - -enum axp717_reg { - AXP717_CHIP_VERSION = 0x3, - AXP717_SHUTDOWN = 0x27, - AXP717_OUTPUT_CTRL1 = 0x80, - AXP717_DCDC1_VOLTAGE = 0x83, -}; - -#define AXP717_CHIP_VERSION_MASK 0xc8 -#define AXP717_DCDC_1220MV_OFFSET 71 -#define AXP717_POWEROFF (1U << 0) -#define DCDC_DVM_ENABLE (1U << 7) - -static u8 axp_mvolt_to_cfg(int mvolt, int min, int max, int div) -{ - if (mvolt < min) - mvolt = min; - else if (mvolt > max) - mvolt = max; - - return (mvolt - min) / div; -} - -static int axp_set_dcdc(int dcdc_num, unsigned int mvolt) -{ - int ret; - u8 cfg = DCDC_DVM_ENABLE; - - if (dcdc_num < 1 || dcdc_num > 3) - return -EINVAL; - - if (mvolt >= 1220) - cfg |= AXP717_DCDC_1220MV_OFFSET + - axp_mvolt_to_cfg(mvolt, 1220, - dcdc_num == 3 ? 1840 : 1540, 20); - else - cfg |= axp_mvolt_to_cfg(mvolt, 500, 1200, 10); - - if (mvolt == 0) - return pmic_bus_clrbits(AXP717_OUTPUT_CTRL1, - 1U << (dcdc_num -1)); - - ret = pmic_bus_write(AXP717_DCDC1_VOLTAGE + dcdc_num - 1, cfg); - if (ret) - return ret; - - return pmic_bus_setbits(AXP717_OUTPUT_CTRL1, 1U << (dcdc_num - 1)); -} - -int axp_set_dcdc1(unsigned int mvolt) -{ - return axp_set_dcdc(1, mvolt); -} - -int axp_set_dcdc2(unsigned int mvolt) -{ - return axp_set_dcdc(2, mvolt); -} - -int axp_set_dcdc3(unsigned int mvolt) -{ - return axp_set_dcdc(3, mvolt); -} - -int axp_init(void) -{ - return pmic_bus_init(); -} - -#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_setbits(AXP717_SHUTDOWN, AXP717_POWEROFF); - - /* infinite loop during shutdown */ - while (1) {} - - /* not reached */ - return 0; -} -#endif
On Thu, May 16, 2024 at 12:48:36AM +0100, Andre Przywara wrote:
We now have a generic AXP SPL driver implementation, that already covers the DC/DC converters of the AXP717 PMIC.
Remove the old, dedicated driver and switch to the new generic driver. This should not introduce any change in behaviour.
Signed-off-by: Andre Przywara andre.przywara@arm.com
drivers/power/Makefile | 2 +- drivers/power/axp717.c | 92 ------------------------------------------ 2 files changed, 1 insertion(+), 93 deletions(-) delete mode 100644 drivers/power/axp717.c
diff --git a/drivers/power/Makefile b/drivers/power/Makefile index 9d1f4c65262..6df23a81c29 100644 --- a/drivers/power/Makefile +++ b/drivers/power/Makefile @@ -14,7 +14,7 @@ obj-$(CONFIG_AXP209_POWER) += axp209.o obj-$(CONFIG_AXP221_POWER) += axp221.o obj-$(CONFIG_AXP305_POWER) += axp305.o obj-$(CONFIG_AXP313_POWER) += axp313.o -obj-$(CONFIG_AXP717_POWER) += axp717.o +obj-$(CONFIG_AXP717_POWER) += axp_spl.o obj-$(CONFIG_AXP809_POWER) += axp809.o obj-$(CONFIG_AXP818_POWER) += axp818.o endif diff --git a/drivers/power/axp717.c b/drivers/power/axp717.c deleted file mode 100644 index 7c77c09ea8f..00000000000 --- a/drivers/power/axp717.c +++ /dev/null @@ -1,92 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0+ -/*
- AXP717 SPL driver
- (C) Copyright 2024 Arm Ltd.
- */
-#include <common.h> -#include <command.h> -#include <errno.h> -#include <asm/arch/pmic_bus.h> -#include <axp_pmic.h>
-enum axp717_reg {
- AXP717_CHIP_VERSION = 0x3,
- AXP717_SHUTDOWN = 0x27,
- AXP717_OUTPUT_CTRL1 = 0x80,
- AXP717_DCDC1_VOLTAGE = 0x83,
-};
-#define AXP717_CHIP_VERSION_MASK 0xc8 -#define AXP717_DCDC_1220MV_OFFSET 71 -#define AXP717_POWEROFF (1U << 0) -#define DCDC_DVM_ENABLE (1U << 7)
-static u8 axp_mvolt_to_cfg(int mvolt, int min, int max, int div) -{
- if (mvolt < min)
mvolt = min;- else if (mvolt > max)
mvolt = max;- return (mvolt - min) / div;
-}
-static int axp_set_dcdc(int dcdc_num, unsigned int mvolt) -{
- int ret;
- u8 cfg = DCDC_DVM_ENABLE;
- if (dcdc_num < 1 || dcdc_num > 3)
return -EINVAL;- if (mvolt >= 1220)
cfg |= AXP717_DCDC_1220MV_OFFSET +axp_mvolt_to_cfg(mvolt, 1220,dcdc_num == 3 ? 1840 : 1540, 20);- else
cfg |= axp_mvolt_to_cfg(mvolt, 500, 1200, 10);- if (mvolt == 0)
return pmic_bus_clrbits(AXP717_OUTPUT_CTRL1,1U << (dcdc_num -1));- ret = pmic_bus_write(AXP717_DCDC1_VOLTAGE + dcdc_num - 1, cfg);
- if (ret)
return ret;- return pmic_bus_setbits(AXP717_OUTPUT_CTRL1, 1U << (dcdc_num - 1));
-}
-int axp_set_dcdc1(unsigned int mvolt) -{
- return axp_set_dcdc(1, mvolt);
-}
-int axp_set_dcdc2(unsigned int mvolt) -{
- return axp_set_dcdc(2, mvolt);
-}
-int axp_set_dcdc3(unsigned int mvolt) -{
- return axp_set_dcdc(3, mvolt);
-}
-int axp_init(void) -{
- return pmic_bus_init();
-}
-#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_setbits(AXP717_SHUTDOWN, AXP717_POWEROFF);
- /* infinite loop during shutdown */
- while (1) {}
- /* not reached */
- return 0;
-}
-#endif
2.35.8
I've been using this for the past week or so on my RG35XX-H for which I've been testing some code and it has worked well for me.
Tested-by: Chris Morgan macromorgan@hotmail.com
The generic AXP SPL driver implementation can cover all regulators we need for the AXP313.
Add the descriptions for the three DC/DC regulators of the AXP313, and enable that when CONFIG_AXP313_POWER is enabled. Also remove the old driver, and switch the Makefile to include the new, generic version.
Signed-off-by: Andre Przywara andre.przywara@arm.com --- drivers/power/Makefile | 2 +- drivers/power/axp313.c | 133 ---------------------------------------- drivers/power/axp_spl.c | 13 ++++ 3 files changed, 14 insertions(+), 134 deletions(-) delete mode 100644 drivers/power/axp313.c
diff --git a/drivers/power/Makefile b/drivers/power/Makefile index 6df23a81c29..30e0daf0621 100644 --- a/drivers/power/Makefile +++ b/drivers/power/Makefile @@ -13,7 +13,7 @@ obj-$(CONFIG_AXP152_POWER) += axp152.o obj-$(CONFIG_AXP209_POWER) += axp209.o obj-$(CONFIG_AXP221_POWER) += axp221.o obj-$(CONFIG_AXP305_POWER) += axp305.o -obj-$(CONFIG_AXP313_POWER) += axp313.o +obj-$(CONFIG_AXP313_POWER) += axp_spl.o obj-$(CONFIG_AXP717_POWER) += axp_spl.o obj-$(CONFIG_AXP809_POWER) += axp809.o obj-$(CONFIG_AXP818_POWER) += axp818.o diff --git a/drivers/power/axp313.c b/drivers/power/axp313.c deleted file mode 100644 index 09ecb5b1ec2..00000000000 --- a/drivers/power/axp313.c +++ /dev/null @@ -1,133 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0+ -/* - * AXP313(a) driver - * - * (C) Copyright 2023 Arm Ltd. - * - * Based on axp305.c - * (C) Copyright 2020 Jernej Skrabec jernej.skrabec@siol.net - * (C) Copyright 2014 Hans de Goede hdegoede@redhat.com - * (C) Copyright 2013 Oliver Schinagl oliver@schinagl.nl - */ - -#include <command.h> -#include <errno.h> -#include <asm/arch/pmic_bus.h> -#include <axp_pmic.h> - -enum axp313_reg { - AXP313_CHIP_VERSION = 0x03, - AXP313_OUTPUT_CTRL = 0x10, - AXP313_DCDC1_CTRL = 0x13, - AXP313_SHUTDOWN = 0x1a, -}; - -#define AXP313_CHIP_VERSION_MASK 0xcf -#define AXP313_CHIP_VERSION_AXP1530 0x48 -#define AXP313_CHIP_VERSION_AXP313A 0x4b -#define AXP313_CHIP_VERSION_AXP313B 0x4c - -#define AXP313_DCDC_SPLIT_OFFSET 71 -#define AXP313_DCDC_SPLIT_MVOLT 1200 - -#define AXP313_POWEROFF BIT(7) - -static u8 mvolt_to_cfg(int mvolt, int min, int max, int div) -{ - if (mvolt < min) - mvolt = min; - else if (mvolt > max) - mvolt = max; - - return (mvolt - min) / div; -} - -static int axp_set_dcdc(int dcdc_num, unsigned int mvolt) -{ - int ret; - u8 cfg, enable_mask = 1U << (dcdc_num - 1); - int volt_reg = AXP313_DCDC1_CTRL + dcdc_num - 1; - int max_mV; - - switch (dcdc_num) { - case 1: - case 2: - max_mV = 1540; - break; - case 3: - /* - * The manual defines a different split point, but tests - * show that it's the same 1200mV as for DCDC1/2. - */ - max_mV = 1840; - break; - default: - return -EINVAL; - } - - if (mvolt > AXP313_DCDC_SPLIT_MVOLT) - cfg = AXP313_DCDC_SPLIT_OFFSET + mvolt_to_cfg(mvolt, - AXP313_DCDC_SPLIT_MVOLT + 20, max_mV, 20); - else - cfg = mvolt_to_cfg(mvolt, 500, AXP313_DCDC_SPLIT_MVOLT, 10); - - if (mvolt == 0) - return pmic_bus_clrbits(AXP313_OUTPUT_CTRL, enable_mask); - - debug("DCDC%d: writing 0x%x to reg 0x%x\n", dcdc_num, cfg, volt_reg); - ret = pmic_bus_write(volt_reg, cfg); - if (ret) - return ret; - - return pmic_bus_setbits(AXP313_OUTPUT_CTRL, enable_mask); -} - -int axp_set_dcdc2(unsigned int mvolt) -{ - return axp_set_dcdc(2, mvolt); -} - -int axp_set_dcdc3(unsigned int mvolt) -{ - return axp_set_dcdc(3, mvolt); -} - -int axp_init(void) -{ - u8 axp_chip_id; - int ret; - - ret = pmic_bus_init(); - if (ret) - return ret; - - ret = pmic_bus_read(AXP313_CHIP_VERSION, &axp_chip_id); - if (ret) - return ret; - - axp_chip_id &= AXP313_CHIP_VERSION_MASK; - switch (axp_chip_id) { - case AXP313_CHIP_VERSION_AXP1530: - case AXP313_CHIP_VERSION_AXP313A: - case AXP313_CHIP_VERSION_AXP313B: - break; - default: - debug("unknown PMIC: 0x%x\n", axp_chip_id); - return -EINVAL; - } - - return ret; -} - -#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(AXP313_SHUTDOWN, AXP313_POWEROFF); - - /* infinite loop during shutdown */ - while (1) {} - - /* not reached */ - return 0; -} -#endif diff --git a/drivers/power/axp_spl.c b/drivers/power/axp_spl.c index 36eb6bd0b2a..f72d106315e 100644 --- a/drivers/power/axp_spl.c +++ b/drivers/power/axp_spl.c @@ -34,6 +34,19 @@ static const struct axp_reg_desc_spl axp_spl_dcdc_regulators[] = { #define AXP_SHUTDOWN_REG 0x27 #define AXP_SHUTDOWN_MASK BIT(0)
+#elif defined(CONFIG_AXP313_POWER) /* AXP313 */ + +static const struct axp_reg_desc_spl axp_spl_dcdc_regulators[] = { + { 0x10, BIT(0), 0x13, 0x7f, 500, 1540, 10, 70 }, + { 0x10, BIT(1), 0x14, 0x7f, 500, 1540, 10, 70 }, + { 0x10, BIT(2), 0x15, 0x7f, 500, 1840, 10, 70 }, +}; +#define AXP_CHIP_VERSION 0x3 +#define AXP_CHIP_VERSION_MASK 0xc8 +#define AXP_CHIP_ID 0x48 +#define AXP_SHUTDOWN_REG 0x1a +#define AXP_SHUTDOWN_MASK BIT(7) + #else
#error "Please define the regulator registers in axp_spl_regulators[]."
The generic AXP SPL driver implementation can cover all regulators we need for the AXP305.
Add the descriptions for four of the six DC/DC regulators of the AXP305, and enable that when CONFIG_AXP305_POWER is enabled. We won't need DCDC2 and DCDC3, but by using the position in the array for the index we keep the code cleaner. Also remove the old driver, and switch the Makefile to include the new, generic driver.
Signed-off-by: Andre Przywara andre.przywara@arm.com --- drivers/power/Makefile | 2 +- drivers/power/axp305.c | 82 ----------------------------------------- drivers/power/axp_spl.c | 15 ++++++++ 3 files changed, 16 insertions(+), 83 deletions(-) delete mode 100644 drivers/power/axp305.c
diff --git a/drivers/power/Makefile b/drivers/power/Makefile index 30e0daf0621..8d36f87ed32 100644 --- a/drivers/power/Makefile +++ b/drivers/power/Makefile @@ -12,7 +12,7 @@ ifdef CONFIG_SPL_BUILD obj-$(CONFIG_AXP152_POWER) += axp152.o obj-$(CONFIG_AXP209_POWER) += axp209.o obj-$(CONFIG_AXP221_POWER) += axp221.o -obj-$(CONFIG_AXP305_POWER) += axp305.o +obj-$(CONFIG_AXP305_POWER) += axp_spl.o obj-$(CONFIG_AXP313_POWER) += axp_spl.o obj-$(CONFIG_AXP717_POWER) += axp_spl.o obj-$(CONFIG_AXP809_POWER) += axp809.o diff --git a/drivers/power/axp305.c b/drivers/power/axp305.c deleted file mode 100644 index 0312ad9af76..00000000000 --- a/drivers/power/axp305.c +++ /dev/null @@ -1,82 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0+ -/* - * AXP305 driver - * - * (C) Copyright 2020 Jernej Skrabec jernej.skrabec@siol.net - * - * Based on axp221.c - * (C) Copyright 2014 Hans de Goede hdegoede@redhat.com - * (C) Copyright 2013 Oliver Schinagl oliver@schinagl.nl - */ - -#include <command.h> -#include <errno.h> -#include <asm/arch/pmic_bus.h> -#include <axp_pmic.h> - -#define AXP305_DCDC4_1600MV_OFFSET 46 - -static u8 axp305_mvolt_to_cfg(int mvolt, int min, int max, int div) -{ - if (mvolt < min) - mvolt = min; - else if (mvolt > max) - mvolt = max; - - return (mvolt - min) / div; -} - -int axp_set_dcdc4(unsigned int mvolt) -{ - int ret; - u8 cfg; - - if (mvolt >= 1600) - cfg = AXP305_DCDC4_1600MV_OFFSET + - axp305_mvolt_to_cfg(mvolt, 1600, 3300, 100); - else - cfg = axp305_mvolt_to_cfg(mvolt, 600, 1500, 20); - - if (mvolt == 0) - return pmic_bus_clrbits(AXP305_OUTPUT_CTRL1, - AXP305_OUTPUT_CTRL1_DCDCD_EN); - - ret = pmic_bus_write(AXP305_DCDCD_VOLTAGE, cfg); - if (ret) - return ret; - - return pmic_bus_setbits(AXP305_OUTPUT_CTRL1, - AXP305_OUTPUT_CTRL1_DCDCD_EN); -} - -int axp_init(void) -{ - u8 axp_chip_id; - int ret; - - ret = pmic_bus_init(); - if (ret) - return ret; - - ret = pmic_bus_read(AXP305_CHIP_VERSION, &axp_chip_id); - if (ret) - return ret; - - if ((axp_chip_id & AXP305_CHIP_VERSION_MASK) != 0x40) - return -ENODEV; - - return ret; -} - -#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); - - /* infinite loop during shutdown */ - while (1) {} - - /* not reached */ - return 0; -} -#endif diff --git a/drivers/power/axp_spl.c b/drivers/power/axp_spl.c index f72d106315e..e38895c5c7d 100644 --- a/drivers/power/axp_spl.c +++ b/drivers/power/axp_spl.c @@ -47,6 +47,21 @@ static const struct axp_reg_desc_spl axp_spl_dcdc_regulators[] = { #define AXP_SHUTDOWN_REG 0x1a #define AXP_SHUTDOWN_MASK BIT(7)
+#elif defined(CONFIG_AXP305_POWER) /* AXP305 */ + +static const struct axp_reg_desc_spl axp_spl_dcdc_regulators[] = { + { 0x10, BIT(0), 0x12, 0x7f, 600, 1520, 10, 50 }, + { 0x10, BIT(1), 0x13, 0x1f, 1000, 2550, 50, NA }, + { 0x10, BIT(2), 0x14, 0x7f, 600, 1520, 10, 50 }, + { 0x10, BIT(3), 0x15, 0x3f, 600, 1500, 20, NA }, + { 0x10, BIT(4), 0x16, 0x1f, 1100, 3400, 100, NA }, +}; +#define AXP_CHIP_VERSION 0x3 +#define AXP_CHIP_VERSION_MASK 0xcf +#define AXP_CHIP_ID 0x40 +#define AXP_SHUTDOWN_REG 0x32 +#define AXP_SHUTDOWN_MASK BIT(7) + #else
#error "Please define the regulator registers in axp_spl_regulators[]."
The X-Powers AXP707 is a PMIC with some buck converters and a larger number of LDOs, alongside some charging and USB circuitry.
Add the descriptions for the five DC/DC regulators that we will need, and enable that when CONFIG_AXP707_POWER is enabled. We won't need DCDC2 till DCDC4, but by using the position in the array for the index we keep the code cleaner.
Signed-off-by: Andre Przywara andre.przywara@arm.com --- drivers/power/axp_spl.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/drivers/power/axp_spl.c b/drivers/power/axp_spl.c index e38895c5c7d..8c9698612b5 100644 --- a/drivers/power/axp_spl.c +++ b/drivers/power/axp_spl.c @@ -62,6 +62,21 @@ static const struct axp_reg_desc_spl axp_spl_dcdc_regulators[] = { #define AXP_SHUTDOWN_REG 0x32 #define AXP_SHUTDOWN_MASK BIT(7)
+#elif defined(CONFIG_AXP707_POWER) /* AXP707 */ + +static const struct axp_reg_desc_spl axp_spl_dcdc_regulators[] = { + { 0x10, BIT(0), 0x20, 0x1f, 1600, 3400, 100, NA }, + { 0x10, BIT(1), 0x21, 0x7f, 500, 1300, 10, 70 }, + { 0x10, BIT(2), 0x22, 0x7f, 500, 1300, 10, 70 }, + { 0x10, BIT(3), 0x23, 0x7f, 500, 1300, 10, 70 }, + { 0x10, BIT(4), 0x24, 0x7f, 800, 1840, 10, 32 }, +}; +#define AXP_CHIP_VERSION 0x3 +#define AXP_CHIP_VERSION_MASK 0xcf +#define AXP_CHIP_ID 0x81 +#define AXP_SHUTDOWN_REG 0x32 +#define AXP_SHUTDOWN_MASK BIT(7) + #else
#error "Please define the regulator registers in axp_spl_regulators[]."
Hi Andre,
this is the first series in an attempt to clean up the X-Powers AXP PMIC drivers used by the SPL for sunxi boards. So far we have a separate driver file for each AXP variant, but the code was largely the same, just differing by the regulator ranges.
This adds a new generic driver, which reads the regulator description from an array of structs. This is similar to how the DM AXP driver used for U-Boot proper works, but is simplified, since we won't need the full feature set for the SPL, and we want to keep the code size small.
Overall seems a reasonable approach, would it make sense to put the regulator descriptions in a file that could be shared between this and the DM driver so the information isn't duplicated and hence may diverge over time with things like copy/paste errors?
Peter
To help bisect-ability, and to simplify review, each of the simpler AXP drivers covered by this approach is handled in a separate patch. We just convert the AXP313, AXP305, AXP717 for now, and on the way add support for the AXP707, just because it's now very easy, and we will need it soon enough. The other AXP SPL drivers are more complex, and support more regulators, but my hunch is that we really just need the DC/DC converters in the SPL. However I need to prove and test this, so I will convert the other AXP chips later.
Please have a look and comment whether the approach in general is a good idea.
Cheers, Andre
Andre Przywara (6): power: pmic: sunxi: only build AXP drivers for SPL power: pmic: sunxi: introduce generic SPL AXP DC/DC driver power: pmic: sunxi: replace AXP717 SPL driver power: pmic: sunxi: use generic AXP SPL driver for AXP313 power: pmic: sunxi: use generic AXP SPL driver for AXP305 power: pmic: sunxi: add AXP707 support
drivers/power/Makefile | 8 +- drivers/power/axp305.c | 82 ------------------ drivers/power/axp313.c | 133 ----------------------------- drivers/power/axp717.c | 92 -------------------- drivers/power/axp_spl.c | 184 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 189 insertions(+), 310 deletions(-) delete mode 100644 drivers/power/axp305.c delete mode 100644 drivers/power/axp313.c delete mode 100644 drivers/power/axp717.c create mode 100644 drivers/power/axp_spl.c
-- 2.35.8
On Mon, 20 May 2024 13:48:41 +0100 Peter Robinson pbrobinson@gmail.com wrote:
Hi Peter,
thanks for having a look!
Hi Andre,
this is the first series in an attempt to clean up the X-Powers AXP PMIC drivers used by the SPL for sunxi boards. So far we have a separate driver file for each AXP variant, but the code was largely the same, just differing by the regulator ranges.
This adds a new generic driver, which reads the regulator description from an array of structs. This is similar to how the DM AXP driver used for U-Boot proper works, but is simplified, since we won't need the full feature set for the SPL, and we want to keep the code size small.
Overall seems a reasonable approach, would it make sense to put the regulator descriptions in a file that could be shared between this and the DM driver so the information isn't duplicated and hence may diverge over time with things like copy/paste errors?
Yes, I thought about it, but it gets nasty: - The SPL should only have the regulator descriptions for the *one* selected AXP chip, whereas the DM driver can afford to describe all regulators and select the right ones via the compatible at runtime. This is to conserve memory for the SPL. - The SPL should only be interested in the DC/DC buck converters, we don't really need any LDOs. Again saves memory. - Each regulator entry for the SPL should be as small as possible, we don't want and need the regulator name, for instance, or the table pointer. Again to save memory.
Currently the array for the three AXP313 regulators in the SPL is exactly 30 bytes long; in the DM driver, just for the small AXP313: 192 bytes. The difference is even more pronounced for the larger AXPs.
I am hopeful there might be some macro magic to express this, like: static const struct axp_regulator_plat axp313_regulators[] = { SPL( "dcdc1", 0x10, BIT(0), 0x13, 0x7f, 500, 1540, 10, 70 ) SPL( "dcdc2", 0x10, BIT(1), 0x14, 0x7f, 500, 1540, 10, 70 ) SPL( "dcdc3", 0x10, BIT(2), 0x15, 0x7f, 500, 1840, 10, 70 ) PROPER( "aldo1", 0x10, BIT(3), 0x16, 0x1f, 500, 3500, 100, NA ) PROPER( "dldo1", 0x10, BIT(4), 0x17, 0x1f, 500, 3500, 100, NA ) {} }; But we still need something to avoid the array altogether when another AXP is selected, which might require #ifdefs. So it might be possible, but the devil is probably in the details.
For now I just wanted to avoid adding more AXP driver files to the drivers/power directory, so I consider this a future optimisation. Happy to take patches :-D
Cheers, Andre
Peter
To help bisect-ability, and to simplify review, each of the simpler AXP drivers covered by this approach is handled in a separate patch. We just convert the AXP313, AXP305, AXP717 for now, and on the way add support for the AXP707, just because it's now very easy, and we will need it soon enough. The other AXP SPL drivers are more complex, and support more regulators, but my hunch is that we really just need the DC/DC converters in the SPL. However I need to prove and test this, so I will convert the other AXP chips later.
Please have a look and comment whether the approach in general is a good idea.
Cheers, Andre
Andre Przywara (6): power: pmic: sunxi: only build AXP drivers for SPL power: pmic: sunxi: introduce generic SPL AXP DC/DC driver power: pmic: sunxi: replace AXP717 SPL driver power: pmic: sunxi: use generic AXP SPL driver for AXP313 power: pmic: sunxi: use generic AXP SPL driver for AXP305 power: pmic: sunxi: add AXP707 support
drivers/power/Makefile | 8 +- drivers/power/axp305.c | 82 ------------------ drivers/power/axp313.c | 133 ----------------------------- drivers/power/axp717.c | 92 -------------------- drivers/power/axp_spl.c | 184 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 189 insertions(+), 310 deletions(-) delete mode 100644 drivers/power/axp305.c delete mode 100644 drivers/power/axp313.c delete mode 100644 drivers/power/axp717.c create mode 100644 drivers/power/axp_spl.c
-- 2.35.8
On Mon, 20 May 2024 at 15:18, Andre Przywara andre.przywara@arm.com wrote:
On Mon, 20 May 2024 13:48:41 +0100 Peter Robinson pbrobinson@gmail.com wrote:
Hi Peter,
thanks for having a look!
Hi Andre,
this is the first series in an attempt to clean up the X-Powers AXP PMIC drivers used by the SPL for sunxi boards. So far we have a separate driver file for each AXP variant, but the code was largely the same, just differing by the regulator ranges.
This adds a new generic driver, which reads the regulator description from an array of structs. This is similar to how the DM AXP driver used for U-Boot proper works, but is simplified, since we won't need the full feature set for the SPL, and we want to keep the code size small.
Overall seems a reasonable approach, would it make sense to put the regulator descriptions in a file that could be shared between this and the DM driver so the information isn't duplicated and hence may diverge over time with things like copy/paste errors?
Yes, I thought about it, but it gets nasty:
- The SPL should only have the regulator descriptions for the *one*
selected AXP chip, whereas the DM driver can afford to describe all regulators and select the right ones via the compatible at runtime. This is to conserve memory for the SPL.
- The SPL should only be interested in the DC/DC buck converters, we don't
really need any LDOs. Again saves memory.
- Each regulator entry for the SPL should be as small as possible, we don't
want and need the regulator name, for instance, or the table pointer. Again to save memory.
Currently the array for the three AXP313 regulators in the SPL is exactly 30 bytes long; in the DM driver, just for the small AXP313: 192 bytes. The difference is even more pronounced for the larger AXPs.
I am hopeful there might be some macro magic to express this, like: static const struct axp_regulator_plat axp313_regulators[] = { SPL( "dcdc1", 0x10, BIT(0), 0x13, 0x7f, 500, 1540, 10, 70 ) SPL( "dcdc2", 0x10, BIT(1), 0x14, 0x7f, 500, 1540, 10, 70 ) SPL( "dcdc3", 0x10, BIT(2), 0x15, 0x7f, 500, 1840, 10, 70 ) PROPER( "aldo1", 0x10, BIT(3), 0x16, 0x1f, 500, 3500, 100, NA ) PROPER( "dldo1", 0x10, BIT(4), 0x17, 0x1f, 500, 3500, 100, NA ) {} }; But we still need something to avoid the array altogether when another AXP is selected, which might require #ifdefs. So it might be possible, but the devil is probably in the details.
For now I just wanted to avoid adding more AXP driver files to the drivers/power directory, so I consider this a future optimisation.
That all makes sense, thanks for the great explanation!
Happy to take patches :-D
Cheers, Andre
Peter
To help bisect-ability, and to simplify review, each of the simpler AXP drivers covered by this approach is handled in a separate patch. We just convert the AXP313, AXP305, AXP717 for now, and on the way add support for the AXP707, just because it's now very easy, and we will need it soon enough. The other AXP SPL drivers are more complex, and support more regulators, but my hunch is that we really just need the DC/DC converters in the SPL. However I need to prove and test this, so I will convert the other AXP chips later.
Please have a look and comment whether the approach in general is a good idea.
Cheers, Andre
Andre Przywara (6): power: pmic: sunxi: only build AXP drivers for SPL power: pmic: sunxi: introduce generic SPL AXP DC/DC driver power: pmic: sunxi: replace AXP717 SPL driver power: pmic: sunxi: use generic AXP SPL driver for AXP313 power: pmic: sunxi: use generic AXP SPL driver for AXP305 power: pmic: sunxi: add AXP707 support
drivers/power/Makefile | 8 +- drivers/power/axp305.c | 82 ------------------ drivers/power/axp313.c | 133 ----------------------------- drivers/power/axp717.c | 92 -------------------- drivers/power/axp_spl.c | 184 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 189 insertions(+), 310 deletions(-) delete mode 100644 drivers/power/axp305.c delete mode 100644 drivers/power/axp313.c delete mode 100644 drivers/power/axp717.c create mode 100644 drivers/power/axp_spl.c
-- 2.35.8
participants (3)
-
Andre Przywara -
Chris Morgan -
Peter Robinson