Hi Andre,
On 1/11/23 17:08, Andre Przywara wrote:
> On Mon, 28 Nov 2022 00:47:55 -0600
> Samuel Holland <sam...@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 <sam...@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.
Yes, the license is because I took the code from TF-A. I am fine with
dual-licensing as well.
>> +/*
>> + * Copyright (c) 2017-2019, ARM Limited and Contributors. All rights
>> reserved.
>> + * Copyright (c) 2018-2022 Samuel Holland <sam...@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.
I will add a comment above this block.
>> + } 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.
These parameters match the range given for VDC5OUT elsewhere in the
datasheet. Also, DCDC5 is intended for DRAM, so it would need to support
at least 1.8V for DDR2.
>> + { "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.
I will fix this in v2.
> 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.
Do you have any thoughts about the best way to minimize the code/data
size (if needed) for where this driver is used in SPL, like on H616? Of
course, optimization could come later as well.
Regards,
Samuel
> 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,
>> +};
>
