[PATCH 2/3] power: regulator: Add a driver for AXP PMIC regulators

Andre Przywara andre.przywara at arm.com
Thu Jan 12 00:08:48 CET 2023


On Mon, 28 Nov 2022 00:47:55 -0600
Samuel Holland <samuel at sholland.org> wrote:

Hi Samuel,

> This driver handles most voltage regulators found in X-Powers AXP PMICs.
> It is based on, and intended to replace, the regulator driver in TF-A.

Many thanks for putting this together! That's much appreciated.

> AXP PMIC regulators can be divided into 6 categories:
>  - Switches without voltage control => fully supported.
>  - Single linear range => fully supported.
>  - Two linear ranges, "step" and "2 * step" => fully supported.
>  - Two linear ranges, "step" and "5 * step" => only the first range is
>    supported. No boards are known to use the second range.
>  - Non-linear voltage values => fully supported.
>  - LDOs shared with GPIO pins => not supported.

Quite an impressive feature list!

So I first scratched my head about the ffs and shift dance, but then
realised that some of those older PMICs stuffed two voltage controls in
one register. Neat solution for that!

I compared the tables against all datasheets, just found one bug (plus
apparently one documentation bug).
And one question about the license, see below.

Otherwise I am ready to take that driver now, just need to test it on
some boards.

> 
> Signed-off-by: Samuel Holland <samuel at sholland.org>
> ---
> 
>  drivers/power/regulator/Kconfig         |  14 ++
>  drivers/power/regulator/Makefile        |   1 +
>  drivers/power/regulator/axp_regulator.c | 308 ++++++++++++++++++++++++
>  3 files changed, 323 insertions(+)
>  create mode 100644 drivers/power/regulator/axp_regulator.c
> 
> diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig
> index c519e066ef0..de776556ffe 100644
> --- a/drivers/power/regulator/Kconfig
> +++ b/drivers/power/regulator/Kconfig
> @@ -43,6 +43,20 @@ config REGULATOR_AS3722
>  	  but does not yet support change voltages. Currently this must be
>  	  done using direct register writes to the PMIC.
>  
> +config REGULATOR_AXP
> +	bool "Enable driver for X-Powers AXP PMIC regulators"
> +	depends on DM_REGULATOR && PMIC_AXP
> +	help
> +	  Enable support for the regulators (DCDCs, LDOs) in the
> +	  X-Powers AXP152, AXP2xx, and AXP8xx PMICs.
> +
> +config SPL_REGULATOR_AXP
> +	bool "Enable driver for X-Powers AXP PMIC regulators in SPL"
> +	depends on SPL_DM_REGULATOR && SPL_PMIC_AXP
> +	help
> +	  Enable support in SPL for the regulators (DCDCs, LDOs) in the
> +	  X-Powers AXP152, AXP2xx, and AXP8xx PMICs.
> +
>  config DM_REGULATOR_BD71837
>  	bool "Enable Driver Model for ROHM BD71837/BD71847 regulators"
>  	depends on DM_REGULATOR && DM_PMIC_BD71837
> diff --git a/drivers/power/regulator/Makefile b/drivers/power/regulator/Makefile
> index bc736068bca..eb06b85bcd9 100644
> --- a/drivers/power/regulator/Makefile
> +++ b/drivers/power/regulator/Makefile
> @@ -7,6 +7,7 @@
>  obj-$(CONFIG_$(SPL_)DM_REGULATOR) += regulator-uclass.o
>  obj-$(CONFIG_REGULATOR_ACT8846) += act8846.o
>  obj-$(CONFIG_REGULATOR_AS3722)	+= as3722_regulator.o
> +obj-$(CONFIG_$(SPL_)REGULATOR_AXP) += axp_regulator.o
>  obj-$(CONFIG_$(SPL_)DM_REGULATOR_DA9063) += da9063.o
>  obj-$(CONFIG_DM_REGULATOR_MAX77686) += max77686.o
>  obj-$(CONFIG_$(SPL_)DM_PMIC_PFUZE100) += pfuze100.o
> diff --git a/drivers/power/regulator/axp_regulator.c b/drivers/power/regulator/axp_regulator.c
> new file mode 100644
> index 00000000000..7af3cccd043
> --- /dev/null
> +++ b/drivers/power/regulator/axp_regulator.c
> @@ -0,0 +1,308 @@
> +// SPDX-License-Identifier: BSD-3-Clause

So just BSD-3 looks a bit odd. I guess this comes because you copied
some code (actually not that much?) from TF-A? Looking at the history
there, it was just you and me contributing. I see one small patch from
someone else, but you didn't copy that code.
So I am fine with dual licensing this as "GPL-2.0 OR BSD-3-Clause". If
you agree as well, I would appreciate if you would change the license.

> +/*
> + * Copyright (c) 2017-2019, ARM Limited and Contributors. All rights reserved.
> + * Copyright (c) 2018-2022 Samuel Holland <samuel at sholland.org>
> + */
> +
> +#include <axp_pmic.h>
> +#include <dm.h>
> +#include <errno.h>
> +#include <dm/device-internal.h>
> +#include <power/pmic.h>
> +#include <power/regulator.h>
> +
> +#define NA 0xff
> +
> +struct axp_regulator_plat {
> +	const char	*name;
> +	u8		enable_reg;
> +	u8		enable_mask;
> +	u8		volt_reg;
> +	u8		volt_mask;
> +	u16		min_mV;
> +	u16		max_mV;
> +	u8		step_mV;
> +	u8		split;
> +	const u16	*table;
> +};
> +
> +static int axp_regulator_get_value(struct udevice *dev)
> +{
> +	const struct axp_regulator_plat *plat = dev_get_plat(dev);
> +	int mV, sel;
> +
> +	if (plat->volt_reg == NA)
> +		return -EINVAL;
> +
> +	sel = pmic_reg_read(dev->parent, plat->volt_reg);
> +	if (sel < 0)
> +		return sel;
> +
> +	sel &= plat->volt_mask;
> +	sel >>= ffs(plat->volt_mask) - 1;
> +
> +	if (plat->table) {
> +		mV = plat->table[sel];
> +	} else {
> +		if (sel > plat->split)
> +			sel = plat->split + (sel - plat->split) * 2;
> +		mV = plat->min_mV + sel * plat->step_mV;
> +	}
> +
> +	return mV * 1000;
> +}
> +
> +static int axp_regulator_set_value(struct udevice *dev, int uV)
> +{
> +	const struct axp_regulator_plat *plat = dev_get_plat(dev);
> +	int mV = uV / 1000;
> +	uint sel, shift;
> +
> +	if (plat->volt_reg == NA)
> +		return -EINVAL;
> +	if (mV < plat->min_mV || mV > plat->max_mV)
> +		return -EINVAL;
> +
> +	shift = ffs(plat->volt_mask) - 1;
> +
> +	if (plat->table) {
> +		sel = plat->volt_mask >> shift;
> +		while (sel && plat->table[sel] > mV)
> +			sel--;

I wonder if it deserves some documentation/comment somewhere that
tables need to span the whole bit range, as you scan from the end.

> +	} else {
> +		sel = (mV - plat->min_mV) / plat->step_mV;
> +		if (sel > plat->split)
> +			sel = plat->split + (sel - plat->split) / 2;
> +	}
> +
> +	return pmic_clrsetbits(dev->parent, plat->volt_reg,
> +			       plat->volt_mask, sel << shift);
> +}
> +
> +static int axp_regulator_get_enable(struct udevice *dev)
> +{
> +	const struct axp_regulator_plat *plat = dev_get_plat(dev);
> +	int reg;
> +
> +	reg = pmic_reg_read(dev->parent, plat->enable_reg);
> +	if (reg < 0)
> +		return reg;
> +
> +	return (reg & plat->enable_mask) == plat->enable_mask;
> +}
> +
> +static int axp_regulator_set_enable(struct udevice *dev, bool enable)
> +{
> +	const struct axp_regulator_plat *plat = dev_get_plat(dev);
> +
> +	return pmic_clrsetbits(dev->parent, plat->enable_reg,
> +			       plat->enable_mask,
> +			       enable ? plat->enable_mask : 0);
> +}
> +
> +static const struct dm_regulator_ops axp_regulator_ops = {
> +	.get_value		= axp_regulator_get_value,
> +	.set_value		= axp_regulator_set_value,
> +	.get_enable		= axp_regulator_get_enable,
> +	.set_enable		= axp_regulator_set_enable,
> +};
> +
> +static const u16 axp152_dcdc1_table[] = {
> +	1700, 1800, 1900, 2000, 2100, 2400, 2500, 2600,
> +	2700, 2800, 3000, 3100, 3200, 3300, 3400, 3500,
> +};
> +
> +static const u16 axp152_aldo12_table[] = {
> +	1200, 1300, 1400, 1500, 1600, 1700, 1800, 1900,
> +	2000, 2500, 2700, 2800, 3000, 3100, 3200, 3300,
> +};
> +
> +static const u16 axp152_ldo0_table[] = {
> +	5000, 3300, 2800, 2500,
> +};
> +
> +static const struct axp_regulator_plat axp152_regulators[] = {
> +	{ "dcdc1", 0x12, BIT(7), 0x26, 0x0f, .table = axp152_dcdc1_table },
> +	{ "dcdc2", 0x12, BIT(6), 0x23, 0x3f,  700, 2275,  25, NA },
> +	{ "dcdc3", 0x12, BIT(5), 0x27, 0x3f,  700, 3500,  50, NA },
> +	{ "dcdc4", 0x12, BIT(4), 0x2b, 0x7f,  700, 3500,  25, NA },
> +	{ "aldo1", 0x12, BIT(3), 0x28, 0xf0, .table = axp152_aldo12_table },
> +	{ "aldo2", 0x12, BIT(2), 0x28, 0x0f, .table = axp152_aldo12_table },
> +	{ "dldo1", 0x12, BIT(1), 0x29, 0x1f,  700, 3500, 100, NA },
> +	{ "dldo2", 0x12, BIT(0), 0x2a, 0x1f,  700, 3500, 100, NA },
> +	{ "ldo0",  0x15, BIT(7), 0x15, 0x30, .table = axp152_ldo0_table },
> +	{ }
> +};
> +
> +static const u16 axp20x_ldo4_table[] = {
> +	1250, 1300, 1400, 1500, 1600, 1700, 1800, 1900,
> +	2000, 2500, 2700, 2800, 3000, 3100, 3200, 3300,
> +};
> +
> +static const struct axp_regulator_plat axp20x_regulators[] = {
> +	{ "dcdc2", 0x12, BIT(4), 0x23, 0x3f,  700, 2275,  25, NA },
> +	{ "dcdc3", 0x12, BIT(1), 0x27, 0x7f,  700, 3500,  25, NA },
> +	{ "ldo2",  0x12, BIT(2), 0x28, 0xf0, 1800, 3300, 100, NA },
> +	{ "ldo3",  0x12, BIT(6), 0x29, 0x7f,  700, 2275,  25, NA },
> +	{ "ldo4",  0x12, BIT(3), 0x28, 0x0f, .table = axp20x_ldo4_table },
> +	{ }
> +};
> +
> +static const struct axp_regulator_plat axp22x_regulators[] = {
> +	{"dc5ldo", 0x10, BIT(0), 0x1c, 0x07,  700, 1400, 100, NA },
> +	{ "dcdc1", 0x10, BIT(1), 0x21, 0x1f, 1600, 3400, 100, NA },
> +	{ "dcdc2", 0x10, BIT(2), 0x22, 0x3f,  600, 1540,  20, NA },
> +	{ "dcdc3", 0x10, BIT(3), 0x23, 0x3f,  600, 1860,  20, NA },
> +	{ "dcdc4", 0x10, BIT(4), 0x24, 0x3f,  600, 1540,  20, NA },
> +	{ "dcdc5", 0x10, BIT(5), 0x25, 0x1f, 1000, 2550,  50, NA },

The AXP221 datasheet lists this as 0.6->1.54mV @ 20mV/step. However
there are not enough bits to encode the whole range, and the Linux
driver also uses the above parameters. So I am inclined to believe in a
manual copy&paste incident (from DCDC4), and the above entry being
correct.

> +	{ "aldo1", 0x10, BIT(6), 0x28, 0x1f,  700, 3300, 100, NA },
> +	{ "aldo2", 0x10, BIT(7), 0x29, 0x1f,  700, 3300, 100, NA },
> +	{ "aldo3", 0x13, BIT(0), 0x2a, 0x1f,  700, 3300, 100, NA },

According to the AXP221 datasheet and the Linux driver, the enable bit
is bit 7 in reg 13h.

The rest looks fine, just from reading through the code. As mentioned,
I still need to test it, but I am quite hopeful we can take this.

Thanks again!
Andre


> +	{ "dldo1", 0x12, BIT(3), 0x15, 0x1f,  700, 3300, 100, NA },
> +	{ "dldo2", 0x12, BIT(4), 0x16, 0x1f,  700, 3300, 100, NA },
> +	{ "dldo3", 0x12, BIT(5), 0x17, 0x1f,  700, 3300, 100, NA },
> +	{ "dldo4", 0x12, BIT(6), 0x18, 0x1f,  700, 3300, 100, NA },
> +	{ "eldo1", 0x12, BIT(0), 0x19, 0x1f,  700, 3300, 100, NA },
> +	{ "eldo2", 0x12, BIT(1), 0x1a, 0x1f,  700, 3300, 100, NA },
> +	{ "eldo3", 0x12, BIT(2), 0x1b, 0x1f,  700, 3300, 100, NA },
> +	{ "dc1sw", 0x12, BIT(7),   NA,   NA,   NA,   NA,  NA, NA },
> +	{ }
> +};
> +
> +static const struct axp_regulator_plat axp803_regulators[] = {
> +	{ "dcdc1", 0x10, BIT(0), 0x20, 0x1f, 1600, 3400, 100, NA },
> +	{ "dcdc2", 0x10, BIT(1), 0x21, 0x7f,  500, 1300,  10, 70 },
> +	{ "dcdc3", 0x10, BIT(2), 0x22, 0x7f,  500, 1300,  10, 70 },
> +	{ "dcdc4", 0x10, BIT(3), 0x23, 0x7f,  500, 1300,  10, 70 },
> +	{ "dcdc5", 0x10, BIT(4), 0x24, 0x7f,  800, 1840,  10, 32 },
> +	{ "dcdc6", 0x10, BIT(5), 0x25, 0x7f,  600, 1520,  10, 50 },
> +	{ "aldo1", 0x13, BIT(5), 0x28, 0x1f,  700, 3300, 100, NA },
> +	{ "aldo2", 0x13, BIT(6), 0x29, 0x1f,  700, 3300, 100, NA },
> +	{ "aldo3", 0x13, BIT(7), 0x2a, 0x1f,  700, 3300, 100, NA },
> +	{ "dldo1", 0x12, BIT(3), 0x15, 0x1f,  700, 3300, 100, NA },
> +	{ "dldo2", 0x12, BIT(4), 0x16, 0x1f,  700, 4200, 100, 27 },
> +	{ "dldo3", 0x12, BIT(5), 0x17, 0x1f,  700, 3300, 100, NA },
> +	{ "dldo4", 0x12, BIT(6), 0x18, 0x1f,  700, 3300, 100, NA },
> +	{ "eldo1", 0x12, BIT(0), 0x19, 0x1f,  700, 1900,  50, NA },
> +	{ "eldo2", 0x12, BIT(1), 0x1a, 0x1f,  700, 1900,  50, NA },
> +	{ "eldo3", 0x12, BIT(2), 0x1b, 0x1f,  700, 1900,  50, NA },
> +	{ "fldo1", 0x13, BIT(2), 0x1c, 0x0f,  700, 1450,  50, NA },
> +	{ "fldo2", 0x13, BIT(3), 0x1d, 0x0f,  700, 1450,  50, NA },
> +	{ "dc1sw", 0x12, BIT(7),   NA,   NA,   NA,   NA,  NA, NA },
> +	{ }
> +};
> +
> +/*
> + * The "dcdcd" split changes the step size by a factor of 5, not 2;
> + * disallow values above the split to maintain accuracy.
> + */
> +static const struct axp_regulator_plat axp806_regulators[] = {
> +	{ "dcdca", 0x10, BIT(0), 0x12, 0x7f,  600, 1520,  10, 50 },
> +	{ "dcdcb", 0x10, BIT(1), 0x13, 0x1f, 1000, 2550,  50, NA },
> +	{ "dcdcc", 0x10, BIT(2), 0x14, 0x7f,  600, 1520,  10, 50 },
> +	{ "dcdcd", 0x10, BIT(3), 0x15, 0x3f,  600, 1500,  20, NA },
> +	{ "dcdce", 0x10, BIT(4), 0x16, 0x1f, 1100, 3400, 100, NA },
> +	{ "aldo1", 0x10, BIT(5), 0x17, 0x1f,  700, 3300, 100, NA },
> +	{ "aldo2", 0x10, BIT(6), 0x18, 0x1f,  700, 3300, 100, NA },
> +	{ "aldo3", 0x10, BIT(7), 0x19, 0x1f,  700, 3300, 100, NA },
> +	{ "bldo1", 0x11, BIT(0), 0x20, 0x0f,  700, 1900, 100, NA },
> +	{ "bldo2", 0x11, BIT(1), 0x21, 0x0f,  700, 1900, 100, NA },
> +	{ "bldo3", 0x11, BIT(2), 0x22, 0x0f,  700, 1900, 100, NA },
> +	{ "bldo4", 0x11, BIT(3), 0x23, 0x0f,  700, 1900, 100, NA },
> +	{ "cldo1", 0x11, BIT(4), 0x24, 0x1f,  700, 3300, 100, NA },
> +	{ "cldo2", 0x11, BIT(5), 0x25, 0x1f,  700, 4200, 100, 27 },
> +	{ "cldo3", 0x11, BIT(6), 0x26, 0x1f,  700, 3300, 100, NA },
> +	{ "sw",    0x11, BIT(7),   NA,   NA,   NA,   NA,  NA, NA },
> +	{ }
> +};
> +
> +/*
> + * The "dcdc4" split changes the step size by a factor of 5, not 2;
> + * disallow values above the split to maintain accuracy.
> + */
> +static const struct axp_regulator_plat axp809_regulators[] = {
> +	{"dc5ldo", 0x10, BIT(0), 0x1c, 0x07,  700, 1400, 100, NA },
> +	{ "dcdc1", 0x10, BIT(1), 0x21, 0x1f, 1600, 3400, 100, NA },
> +	{ "dcdc2", 0x10, BIT(2), 0x22, 0x3f,  600, 1540,  20, NA },
> +	{ "dcdc3", 0x10, BIT(3), 0x23, 0x3f,  600, 1860,  20, NA },
> +	{ "dcdc4", 0x10, BIT(4), 0x24, 0x3f,  600, 1540,  20, NA },
> +	{ "dcdc5", 0x10, BIT(5), 0x25, 0x1f, 1000, 2550,  50, NA },
> +	{ "aldo1", 0x10, BIT(6), 0x28, 0x1f,  700, 3300, 100, NA },
> +	{ "aldo2", 0x10, BIT(7), 0x29, 0x1f,  700, 3300, 100, NA },
> +	{ "aldo3", 0x12, BIT(5), 0x2a, 0x1f,  700, 3300, 100, NA },
> +	{ "dldo1", 0x12, BIT(3), 0x15, 0x1f,  700, 3300, 100, NA },
> +	{ "dldo2", 0x12, BIT(4), 0x16, 0x1f,  700, 3300, 100, NA },
> +	{ "eldo1", 0x12, BIT(0), 0x19, 0x1f,  700, 3300, 100, NA },
> +	{ "eldo2", 0x12, BIT(1), 0x1a, 0x1f,  700, 3300, 100, NA },
> +	{ "eldo3", 0x12, BIT(2), 0x1b, 0x1f,  700, 3300, 100, NA },
> +	{ "sw",    0x12, BIT(6),   NA,   NA,   NA,   NA,  NA, NA },
> +	{ "dc1sw", 0x12, BIT(7),   NA,   NA,   NA,   NA,  NA, NA },
> +	{ }
> +};
> +
> +static const struct axp_regulator_plat axp813_regulators[] = {
> +	{ "dcdc1", 0x10, BIT(0), 0x20, 0x1f, 1600, 3400, 100, NA },
> +	{ "dcdc2", 0x10, BIT(1), 0x21, 0x7f,  500, 1300,  10, 70 },
> +	{ "dcdc3", 0x10, BIT(2), 0x22, 0x7f,  500, 1300,  10, 70 },
> +	{ "dcdc4", 0x10, BIT(3), 0x23, 0x7f,  500, 1300,  10, 70 },
> +	{ "dcdc5", 0x10, BIT(4), 0x24, 0x7f,  800, 1840,  10, 32 },
> +	{ "dcdc6", 0x10, BIT(5), 0x25, 0x7f,  600, 1520,  10, 50 },
> +	{ "dcdc7", 0x10, BIT(6), 0x26, 0x7f,  600, 1520,  10, 50 },
> +	{ "aldo1", 0x13, BIT(5), 0x28, 0x1f,  700, 3300, 100, NA },
> +	{ "aldo2", 0x13, BIT(6), 0x29, 0x1f,  700, 3300, 100, NA },
> +	{ "aldo3", 0x13, BIT(7), 0x2a, 0x1f,  700, 3300, 100, NA },
> +	{ "dldo1", 0x12, BIT(3), 0x15, 0x1f,  700, 3300, 100, NA },
> +	{ "dldo2", 0x12, BIT(4), 0x16, 0x1f,  700, 4200, 100, 27 },
> +	{ "dldo3", 0x12, BIT(5), 0x17, 0x1f,  700, 3300, 100, NA },
> +	{ "dldo4", 0x12, BIT(6), 0x18, 0x1f,  700, 3300, 100, NA },
> +	{ "eldo1", 0x12, BIT(0), 0x19, 0x1f,  700, 1900,  50, NA },
> +	{ "eldo2", 0x12, BIT(1), 0x1a, 0x1f,  700, 1900,  50, NA },
> +	{ "eldo3", 0x12, BIT(2), 0x1b, 0x1f,  700, 1900,  50, NA },
> +	{ "fldo1", 0x13, BIT(2), 0x1c, 0x0f,  700, 1450,  50, NA },
> +	{ "fldo2", 0x13, BIT(3), 0x1d, 0x0f,  700, 1450,  50, NA },
> +	{ "fldo3", 0x13, BIT(4),   NA,   NA,   NA,   NA,  NA, NA },
> +	{ }
> +};
> +
> +static const struct axp_regulator_plat *const axp_regulators[] = {
> +	[AXP152_ID]	= axp152_regulators,
> +	[AXP202_ID]	= axp20x_regulators,
> +	[AXP209_ID]	= axp20x_regulators,
> +	[AXP221_ID]	= axp22x_regulators,
> +	[AXP223_ID]	= axp22x_regulators,
> +	[AXP803_ID]	= axp803_regulators,
> +	[AXP806_ID]	= axp806_regulators,
> +	[AXP809_ID]	= axp809_regulators,
> +	[AXP813_ID]	= axp813_regulators,
> +};
> +
> +static int axp_regulator_bind(struct udevice *dev)
> +{
> +	struct dm_regulator_uclass_plat *uc_plat = dev_get_uclass_plat(dev);
> +	ulong id = dev_get_driver_data(dev->parent);
> +	const struct axp_regulator_plat *plat;
> +
> +	for (plat = axp_regulators[id]; plat && plat->name; plat++)
> +		if (!strcmp(plat->name, dev->name))
> +			break;
> +	if (!plat || !plat->name)
> +		return -ENODEV;
> +
> +	dev_set_plat(dev, (void *)plat);
> +
> +	if (plat->volt_reg == NA)
> +		uc_plat->type = REGULATOR_TYPE_FIXED;
> +	else if (!strncmp(plat->name, "dcdc", strlen("dcdc")))
> +		uc_plat->type = REGULATOR_TYPE_BUCK;
> +	else
> +		uc_plat->type = REGULATOR_TYPE_LDO;
> +
> +	return 0;
> +}
> +
> +U_BOOT_DRIVER(axp_regulator) = {
> +	.name		= "axp_regulator",
> +	.id		= UCLASS_REGULATOR,
> +	.bind		= axp_regulator_bind,
> +	.ops		= &axp_regulator_ops,
> +};



More information about the U-Boot mailing list