[PATCH v2 4/6] power: pmic: sunxi: add AXP717 SPL support

[Andre Przywara]

On Sun, 14 Jul 2024 20:20:44 +1200
"Ryan Walklin" <ryan at testtoast.com> wrote:

Hi Ryan,

> On Sat, 13 Jul 2024, at 4:53 AM, Andre Przywara wrote:
> 
> >  #define AXP209_I2C_ADDR			0x34
> > +#define AXP717_I2C_ADDR			0x34
> > 
> >  #define AXP305_I2C_ADDR			0x36
> >  #define AXP313_I2C_ADDR			0x36
> > @@ -36,6 +37,8 @@ static int pmic_i2c_address(void)
> >  		return AXP305_I2C_ADDR;
> >  	if (IS_ENABLED(CONFIG_AXP313_POWER))
> >  		return AXP313_I2C_ADDR;
> > +	if (IS_ENABLED(CONFIG_AXP717_POWER))
> > +		return AXP717_I2C_ADDR;
> > 
> >  	/* Other AXP2xx and AXP8xx variants */
> >  	return AXP209_I2C_ADDR;  
> 
> Small point, but is this check strictly necessary, given that the AXP717 uses the same I2C address as the AXP209? If CONFIG_AXP717_POWER is not checked here, this logic will still fall through to the default AXP209 address, which will also be correct for the 717.

I consider the fact that the AXP209 and the AXP717 use the same I2C
address a sheer coincidence, so would like to keep the code readable
and maintainable. This "IS_ENABLED(AXPyyy) return AXPyyy_I2C_ADDR"
patterns seems to be easy to understand and copy, IMHO. If you are
concerned about generated code size: this whole function is optimised
away, and is replaced with a single "mov w0, #0x34" assembly
instruction, just before the respective i2c function calls. This is due
to the fact that those "if" statements have compile-time constant
input, so the compiler knows the final result already, and just
replaces the function call with this constant load. That's a neat
feature of IS_ENABLED(), and a pattern we are exercising in U-Boot for
years, to keep the code both readable, but also minimal.

Thanks for the Tested-by: tag!

Cheers,
Andre

> 
> > diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> > index ed86f1df5dc..9928069c0eb 100644
> > --- a/board/sunxi/board.c
> > +++ b/board/sunxi/board.c
> > @@ -562,7 +562,7 @@ void sunxi_board_init(void)
> >  #if defined CONFIG_AXP152_POWER || defined CONFIG_AXP209_POWER || \
> >  	defined CONFIG_AXP221_POWER || defined CONFIG_AXP305_POWER || \
> >  	defined CONFIG_AXP809_POWER || defined CONFIG_AXP818_POWER || \
> > -	defined CONFIG_AXP313_POWER
> > +	defined CONFIG_AXP313_POWER || defined CONFIG_AXP717_POWER
> >  	power_failed = axp_init();
> > 
> >  	if (IS_ENABLED(CONFIG_AXP_DISABLE_BOOT_ON_POWERON) && !power_failed) {
> > diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> > index 33b8bc1214d..5556a22cf69 100644
> > --- a/drivers/power/Kconfig
> > +++ b/drivers/power/Kconfig
> > @@ -109,6 +109,13 @@ config AXP313_POWER
> >  	Select this to enable support for the AXP313 PMIC found on some
> >  	H616 boards.
> > 
> > +config AXP717_POWER
> > +	bool "axp717 pmic support"
> > +	select AXP_PMIC_BUS
> > +	select CMD_POWEROFF
> > +	---help---
> > +	Select this to enable support for the AXP717 PMIC found on some boards.
> > +
> >  config AXP809_POWER
> >  	bool "axp809 pmic support"
> >  	depends on MACH_SUN9I
> > @@ -151,10 +158,11 @@ config AXP_DCDC1_VOLT
> > 
> >  config AXP_DCDC2_VOLT
> >  	int "axp pmic dcdc2 voltage"
> > -	depends on AXP152_POWER || AXP209_POWER || AXP221_POWER || 
> > AXP809_POWER || AXP818_POWER || AXP313_POWER
> > +	depends on AXP152_POWER || AXP209_POWER || AXP221_POWER || 
> > AXP809_POWER || AXP818_POWER || AXP313_POWER || AXP717_POWER
> >  	default 900 if AXP818_POWER
> >  	default 1400 if AXP152_POWER || AXP209_POWER
> >  	default 1000 if AXP313_POWER
> > +	default 1000 if AXP717_POWER
> >  	default 1200 if MACH_SUN6I
> >  	default 1100 if MACH_SUN8I
> >  	default 0 if MACH_SUN9I
> > @@ -167,11 +175,11 @@ config AXP_DCDC2_VOLT
> >  	On A80 boards dcdc2 powers the GPU and can be left off.
> >  	On A83T boards dcdc2 is used for VDD-CPUA(cluster 0) and should be 
> > 0.9V.
> >  	On R40 boards dcdc2 is VDD-CPU and should be 1.1V
> > -	On boards using the AXP313 it's often VDD-CPU.
> > +	On boards using the AXP313 or AXP717 it's often VDD-CPU.
> > 
> >  config AXP_DCDC3_VOLT
> >  	int "axp pmic dcdc3 voltage"
> > -	depends on AXP152_POWER || AXP209_POWER || AXP221_POWER || 
> > AXP809_POWER || AXP818_POWER || AXP313_POWER
> > +	depends on AXP152_POWER || AXP209_POWER || AXP221_POWER || 
> > AXP809_POWER || AXP818_POWER || AXP313_POWER || AXP717_POWER
> >  	default 900 if AXP809_POWER || AXP818_POWER
> >  	default 1500 if AXP152_POWER
> >  	default 1250 if AXP209_POWER
> > @@ -188,7 +196,8 @@ config AXP_DCDC3_VOLT
> >  	On A80 boards dcdc3 is used for VDD-CPUA(cluster 0) and should be 
> > 0.9V.
> >  	On A83T boards dcdc3 is used for VDD-CPUB(cluster 1) and should be 
> > 0.9V.
> >  	On R40 boards dcdc3 is VDD-SYS and VDD-GPU and should be 1.1V.
> > -	On boards using the AXP313 it's often VDD-DRAM and should be 1.1V for 
> > LPDDR4.
> > +	On boards using the AXP313 or AXP717 it's often VDD-DRAM and should
> > +	be 1.1V for LPDDR4.
> > 
> >  config AXP_DCDC4_VOLT
> >  	int "axp pmic dcdc4 voltage"
> > diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> > index 0f39459dec4..6df23a81c29 100644
> > --- a/drivers/power/Makefile
> > +++ b/drivers/power/Makefile
> > @@ -14,6 +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)	+= axp_spl.o
> >  obj-$(CONFIG_AXP809_POWER)	+= axp809.o
> >  obj-$(CONFIG_AXP818_POWER)	+= axp818.o
> >  endif
> > -- 
> > 2.25.1  
> 
> Tested with RG35XX-H and RG35XX-Plus boards (Allwinner H700 with AXP717). Confirmed both DCDC2 and DCDC3 are set correctly in SPL, allowing successful DRAM init and boot.
> 
> Tested-by: Ryan Walklin <ryan at testtoast.com>
> 
> Regards,
> 
> Ryan
>