On 1/11/21 9:02 AM, André Przywara wrote:
> On 03/01/2021 09:26, Jernej Skrabec wrote:
>> This PMIC can be found on H616 boards and it's very similar to AXP805
>> and AXP806.
>>> Signed-off-by: Jernej Skrabec <jernej.skra...@siol.net>
>
> The existing sunxi PMIC code is the typical U-Boot mess, but I don't
> want to block this series on a rework. I put some comments and ideas
> below how to improve the whole "framework".
Agreed. it seems that it needs to make more time about refactoring whole codes.
>
> For this actual patch I checked the bits against the AXP305 datasheet,
> it does what we need.
>
> (reluctantly)
> Reviewed-by: Andre Przywara <andre.przyw...@arm.com>
Reviewed-by: Jaehoon Chung <jh80.ch...@samsung.com>
Best Regards,
Jaehoon Chung
>
> Thanks,
> Andre
>
>> ---
>> arch/arm/mach-sunxi/pmic_bus.c | 6 +++
>> board/sunxi/board.c | 10 +++--
>> drivers/power/Kconfig | 13 +++++-
>> drivers/power/Makefile | 1 +
>> drivers/power/axp305.c | 80 ++++++++++++++++++++++++++++++++++
>> include/axp305.h | 17 ++++++++
>> include/axp_pmic.h | 3 ++
>> 7 files changed, 126 insertions(+), 4 deletions(-)
>> create mode 100644 drivers/power/axp305.c
>> create mode 100644 include/axp305.h
>>
>> diff --git a/arch/arm/mach-sunxi/pmic_bus.c b/arch/arm/mach-sunxi/pmic_bus.c
>> index dea42de833f1..0394ce856448 100644
>> --- a/arch/arm/mach-sunxi/pmic_bus.c
>> +++ b/arch/arm/mach-sunxi/pmic_bus.c
>> @@ -18,6 +18,8 @@
>>
>> #define AXP209_I2C_ADDR 0x34
>>
>> +#define AXP305_I2C_ADDR 0x36
>> +
>> #define AXP221_CHIP_ADDR 0x68
>> #define AXP221_CTRL_ADDR 0x3e
>> #define AXP221_INIT_DATA 0x3e
>> @@ -64,6 +66,8 @@ int pmic_bus_read(u8 reg, u8 *data)
>> return i2c_read(AXP152_I2C_ADDR, reg, 1, data, 1);
>> #elif defined CONFIG_AXP209_POWER
>> return i2c_read(AXP209_I2C_ADDR, reg, 1, data, 1);
>> +#elif defined CONFIG_AXP305_POWER
>> + return i2c_read(AXP305_I2C_ADDR, reg, 1, data, 1);
>> #elif defined CONFIG_AXP221_POWER || defined CONFIG_AXP809_POWER || defined
>> CONFIG_AXP818_POWER
>> # ifdef CONFIG_MACH_SUN6I
>> return p2wi_read(reg, data);
>> @@ -81,6 +85,8 @@ int pmic_bus_write(u8 reg, u8 data)
>> return i2c_write(AXP152_I2C_ADDR, reg, 1, &data, 1);
>> #elif defined CONFIG_AXP209_POWER
>> return i2c_write(AXP209_I2C_ADDR, reg, 1, &data, 1);
>> +#elif defined CONFIG_AXP305_POWER
>> + return i2c_write(AXP305_I2C_ADDR, reg, 1, &data, 1);
>> #elif defined CONFIG_AXP221_POWER || defined CONFIG_AXP809_POWER || defined
>> CONFIG_AXP818_POWER
>> # ifdef CONFIG_MACH_SUN6I
>> return p2wi_write(reg, data);
>> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
>> index 708a27ed78e9..54ff9bc92396 100644
>> --- a/board/sunxi/board.c
>> +++ b/board/sunxi/board.c
>> @@ -634,16 +634,18 @@ void sunxi_board_init(void)
>> #endif
>
> This whole function is really a mess, but we can clean this up later.
> I wonder if we can guard with the actual CONFIG_AXP_*_VOLT symbols
> directly instead of specifying the list of PMICs requiring this rail
> (this list is already in Kconfig).
> Maybe with some macro magic to avoid the repetitions and move the ifdefs
> out of the function?
> power_failed |= axp_set_rail(dcdc1, DCDC1);
> and assemble the function name and CONFIG_ symbol in that macro, plus
> having the guard there?
>
>>
>> #if defined CONFIG_AXP152_POWER || defined CONFIG_AXP209_POWER || \
>> - defined CONFIG_AXP221_POWER || defined CONFIG_AXP809_POWER || \
>> - defined CONFIG_AXP818_POWER
>> + defined CONFIG_AXP221_POWER || defined CONFIG_AXP305_POWER || \
>> + defined CONFIG_AXP809_POWER || defined CONFIG_AXP818_POWER
>> power_failed = axp_init();
>>
>> #if defined CONFIG_AXP221_POWER || defined CONFIG_AXP809_POWER || \
>> defined CONFIG_AXP818_POWER
>> power_failed |= axp_set_dcdc1(CONFIG_AXP_DCDC1_VOLT);
>> #endif
>> +#if !defined(CONFIG_AXP305_POWER)
>> power_failed |= axp_set_dcdc2(CONFIG_AXP_DCDC2_VOLT);
>> power_failed |= axp_set_dcdc3(CONFIG_AXP_DCDC3_VOLT);
>> +#endif
>> #if !defined(CONFIG_AXP209_POWER) && !defined(CONFIG_AXP818_POWER)
>> power_failed |= axp_set_dcdc4(CONFIG_AXP_DCDC4_VOLT);
>> #endif
>> @@ -656,8 +658,10 @@ void sunxi_board_init(void)
>> defined CONFIG_AXP818_POWER
>> power_failed |= axp_set_aldo1(CONFIG_AXP_ALDO1_VOLT);
>> #endif
>> +#if !defined(CONFIG_AXP305_POWER)
>> power_failed |= axp_set_aldo2(CONFIG_AXP_ALDO2_VOLT);
>> -#if !defined(CONFIG_AXP152_POWER)
>> +#endif
>> +#if !defined(CONFIG_AXP152_POWER) && !defined(CONFIG_AXP305_POWER)
>> power_failed |= axp_set_aldo3(CONFIG_AXP_ALDO3_VOLT);
>> #endif
>> #ifdef CONFIG_AXP209_POWER
>
> Verified this hunk by playing CPP and removing all non-applicable calls.
> This ended up in:
> power_failed = axp_init();
> power_failed |= axp_set_dcdc4(CONFIG_AXP_DCDC4_VOLT);
>
> Which looks like the idea behind this patch, but is totally non-obvious
> from the diff.
>
>> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
>> index 02050f6f3569..d17cf2d9112a 100644
>> --- a/drivers/power/Kconfig
>> +++ b/drivers/power/Kconfig
>> @@ -48,6 +48,15 @@ config AXP221_POWER
>> Select this to enable support for the axp221/axp223 pmic found on most
>> A23 and A31 boards.
>>
>> +config AXP305_POWER
>> + bool "axp305 pmic support"
>> + depends on MACH_SUN50I_H616
>> + select AXP_PMIC_BUS
>> + select CMD_POWEROFF
>> + ---help---
>> + Select this to enable support for the axp305 pmic found on most
>> + H616 boards.
>> +
>> config AXP809_POWER
>> bool "axp809 pmic support"
>> depends on MACH_SUN9I
>> @@ -127,11 +136,12 @@ config AXP_DCDC3_VOLT
>>
>> config AXP_DCDC4_VOLT
>> int "axp pmic dcdc4 voltage"
>> - depends on AXP152_POWER || AXP221_POWER || AXP809_POWER || AXP818_POWER
>> + depends on AXP152_POWER || AXP221_POWER || AXP809_POWER || AXP818_POWER
>> || AXP305_POWER
>> default 1250 if AXP152_POWER
>> default 1200 if MACH_SUN6I
>> default 0 if MACH_SUN8I
>> default 900 if MACH_SUN9I
>> + default 1500 if AXP305_POWER
>> ---help---
>> Set the voltage (mV) to program the axp pmic dcdc4 at, set to 0 to
>> disable dcdc4.
>> @@ -140,6 +150,7 @@ config AXP_DCDC4_VOLT
>> On A23 / A33 boards dcdc4 is unused and should be disabled.
>> On A80 boards dcdc4 powers VDD-SYS, HDMI, USB OTG and should be 0.9V.
>> On A83T boards dcdc4 is used for VDD-GPU.
>> + On H616 boards dcdcd is used for VCC-DRAM.
>>
>> config AXP_DCDC5_VOLT
>> int "axp pmic dcdc5 voltage"
>> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
>> index 2dcc7bb99d02..0bef06920a7d 100644
>> --- a/drivers/power/Makefile
>> +++ b/drivers/power/Makefile
>> @@ -6,6 +6,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_AXP809_POWER) += axp809.o
>> obj-$(CONFIG_AXP818_POWER) += axp818.o
>> obj-$(CONFIG_EXYNOS_TMU) += exynos-tmu.o
>> diff --git a/drivers/power/axp305.c b/drivers/power/axp305.c
>> new file mode 100644
>> index 000000000000..f620798bb1d7
>> --- /dev/null
>> +++ b/drivers/power/axp305.c
>> @@ -0,0 +1,80 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * AXP305 driver
>> + *
>> + * (C) Copyright 2020 Jernej Skrabec <jernej.skra...@siol.net>
>> + *
>> + * Based on axp221.c
>
> Given that, I wonder if we can (later) simplify this, ideally we end up
> at something like this:
> const struct axp_regulator axp_regulators[] = {
> {"dcdc1", 1600, 3400, 100, NA, 0x20, 0x10, 0},
> ...
> https://protect2.fireeye.com/v1/url?k=d54639fd-8add0072-d547b2b2-0cc47a31307c-01fd70f5628c90cc&q=1&e=594dac17-d975-498f-b06b-149370c2c809&u=https%3A%2F%2Fgit.trustedfirmware.org%2FTF-A%2Ftrusted-firmware-a.git%2Ftree%2Fdrivers%2Fallwinner%2Faxp%2Faxp803.c
>
> Not sure that can cover all AXP chips, but it's worth a try to remove
> all this boilerplate.
>
> Cheers,
> Andre
>
>
>> + * (C) Copyright 2014 Hans de Goede <hdego...@redhat.com>
>> + * (C) Copyright 2013 Oliver Schinagl <oli...@schinagl.nl>
>> + */
>> +
>> +#include <common.h>
>> +#include <command.h>
>> +#include <errno.h>
>> +#include <asm/arch/pmic_bus.h>
>> +#include <axp_pmic.h>
>> +
>> +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 = 46 + 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;
>> +}
>> +
>> +#ifndef CONFIG_PSCI_RESET
>> +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/include/axp305.h b/include/axp305.h
>> new file mode 100644
>> index 000000000000..225c5040a322
>> --- /dev/null
>> +++ b/include/axp305.h
>> @@ -0,0 +1,17 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ */
>> +/*
>> + * (C) Copyright 2020 Jernej Skrabec <jernej.skra...@siol.net>
>> + */
>> +
>> +enum axp305_reg {
>> + AXP305_CHIP_VERSION = 0x3,
>> + AXP305_OUTPUT_CTRL1 = 0x10,
>> + AXP305_DCDCD_VOLTAGE = 0x15,
>> + AXP305_SHUTDOWN = 0x32,
>> +};
>> +
>> +#define AXP305_CHIP_VERSION_MASK 0xcf
>> +
>> +#define AXP305_OUTPUT_CTRL1_DCDCD_EN (1 << 3)
>> +
>> +#define AXP305_POWEROFF (1 << 7)
>> diff --git a/include/axp_pmic.h b/include/axp_pmic.h
>> index 10091d0bb278..405044c3a32f 100644
>> --- a/include/axp_pmic.h
>> +++ b/include/axp_pmic.h
>> @@ -15,6 +15,9 @@
>> #ifdef CONFIG_AXP221_POWER
>> #include <axp221.h>
>> #endif
>> +#ifdef CONFIG_AXP305_POWER
>> +#include <axp305.h>
>> +#endif
>> #ifdef CONFIG_AXP809_POWER
>> #include <axp809.h>
>> #endif
>>
>
>
