[U-Boot] [PATCH 08/12] sunxi: Add axp209 pmic support

[Hans de Goede]

Hi,

On 05/31/2014 07:08 PM, Ian Campbell wrote:
> On Fri, 2014-05-30 at 11:06 +0200, Hans de Goede wrote:
>> From: Henrik Nordstrom <henrik at henriknordstrom.net>
>>
>> Add support for the x-powers axp209 pmic which is found on most A10, A13 and
>> A20 boards.
>>
>> While changing the boards.cfg lines for the Cubietruck, add myself as board
>> maintainer for the Cubietruck.
>
> I don't know much about these PMIC things, so just some general
> comments/questions.
>
>>   #include <asm/arch/clock.h>
>>   #include <asm/arch/dram.h>
>>   #include <asm/arch/gpio.h>
>> @@ -116,12 +119,35 @@ void i2c_init_board(void)
>>   #ifdef CONFIG_SPL_BUILD
>>   void sunxi_board_init(void)
>>   {
>> +	int power_failed = 0;
>
> bool?

I would prefer to keep this as an int, doing |= on a bool just
feels wrong.

>
>>   	unsigned long ramsize;
>>
>> +#ifdef CONFIG_AXP209_POWER
>> +	power_failed |= axp209_init();
>> +	power_failed |= axp209_set_dcdc2(1400);
>> +	power_failed |= axp209_set_dcdc3(1250);
>> +	power_failed |= axp209_set_ldo2(3000);
>> +	power_failed |= axp209_set_ldo3(2800);
>> +	power_failed |= axp209_set_ldo4(2800);
>
> I take it that it is safe to keep poking at things after one of them
> fails?

Yes.

>
>> +#endif
>> +
>>   	printf("DRAM:");
>>   	ramsize = sunxi_dram_init();
>>   	printf(" %lu MiB\n", ramsize >> 20);
>>   	if (!ramsize)
>>   		hang();
>> +
>> +	/*
>> +	 * Only clock up the CPU to full speed if we are reasonably
>> +	 * assured it's being powered with suitable core voltage
>> +	 */
>> +	if (!power_failed)
>> +#ifdef CONFIG_SUN7I
>> +		clock_set_pll1(912000000);
>> +#else
>> +		clock_set_pll1(1008000000);
>
> #define CLK_FULL_SPEED in the relevant sun?i-config.h-es?

Good idea, done.

>
>> +#endif
>> +	else
>> +		printf("Failed to set core voltage! Can't set CPU frequency\n");
>>   }
>>   #endif
>> diff --git a/drivers/power/axp209.c b/drivers/power/axp209.c
>> new file mode 100644
>> index 0000000..a3f9d52
>> --- /dev/null
>> +++ b/drivers/power/axp209.c
>> @@ -0,0 +1,198 @@
>> +/*
>> + * (C) Copyright 2012
>> + * Henrik Nordstrom <henrik at henriknordstrom.net>
>> + *
>> + * SPDX-License-Identifier:	GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <i2c.h>
>> +#include <axp209.h>
>> +
>> +enum axp209_reg {
>> +	AXP209_POWER_STATUS = 0x00,
>> +	AXP209_CHIP_VERSION = 0x03,
>> +	AXP209_DCDC2_VOLTAGE = 0x23,
>> +	AXP209_DCDC3_VOLTAGE = 0x27,
>> +	AXP209_LDO24_VOLTAGE = 0x28,
>> +	AXP209_LDO3_VOLTAGE = 0x29,
>> +	AXP209_IRQ_STATUS3 = 0x4a,
>> +	AXP209_IRQ_STATUS5 = 0x4c,
>> +	AXP209_SHUTDOWN = 0x32,
>> +};
>> +
>> +#define AXP209_POWER_STATUS_ON_BY_DC	(1<<0)
>> +
>> +#define AXP209_IRQ3_PEK_SHORT		(1<<1)
>> +#define AXP209_IRQ3_PEK_LONG		(1<<0)
>> +
>> +#define AXP209_IRQ5_PEK_UP		(1<<6)
>> +#define AXP209_IRQ5_PEK_DOWN		(1<<5)
>
> Only IRQ5 is used?

True, I've dropped the IRQ3 defines.

>
>> +
>> +int axp209_write(enum axp209_reg reg, u8 val)
>> +{
>> +	return i2c_write(0x34, reg, 1, &val, 1);
>> +}
>> +
>> +int axp209_read(enum axp209_reg reg, u8 *val)
>> +{
>> +	return i2c_read(0x34, reg, 1, val, 1);
>> +}
>> +
>> +int axp209_set_dcdc2(int mvolt)
>> +{
>> +	int cfg = (mvolt - 700) / 25;
>> +	int rc;
>> +	u8 current;
>> +
>> +	if (cfg < 0)
>> +		cfg = 0;
>
> Can we make mvolt and cfg unsigned? (I suppose you'd then need a range
> check on mvolt before the subtraction, so perhaps it doesn't save much?)

I don't think making them unsigned is helpful.
>
>> +	if (cfg > (1 << 6) - 1)
>> +		cfg = (1 << 6) - 1;
>
> #define AXP209_DCDC2_MAX?
>
>> +int axp209_set_dcdc3(int mvolt)
>> +{
>> +	int cfg = (mvolt - 700) / 25;
>> +	u8 reg;
>> +	int rc;
>> +
>> +	if (cfg < 0)
>> +		cfg = 0;
>> +	if (cfg > (1 << 7) - 1)
>> +		cfg = (1 << 7) - 1;
>
> Some two comments as for DCDC2 (which I won't bother making for
> subsequent clocks).
>
> perhaps cfg = clamp(cfg, 0, 1<<7-1)? Or even cfg = mvolt_to_cfg(mvolt,
> 700, 25, 0, 1<<7-1)?

I've added an mvolt_to_cfg function and used that everywhere.

>
>> +	rc = axp209_write(AXP209_DCDC3_VOLTAGE, cfg);
>> +	rc |= axp209_read(AXP209_DCDC3_VOLTAGE, &reg);
>
> Is read safe if the write failed? Since the reg is never used I assume
> it's just some sort of sync? Or do you need to confirm the contents
> "took"? If not you could remove reg and readback into cfg.

The read is useless, good catch, I've dropped it.

>> +	rc = axp209_read(AXP209_LDO24_VOLTAGE, &reg);
>> +	if (rc)
>> +		return rc;
>> +
>> +	reg = (reg & 0x0f) | (cfg << 4);
>
> Do we know what these magic numbers mean?
>
> (ah, you have a comment on the use of the lower 4 bits, but not the
> upper 4 used here...)

Added a comment.

>> +	rc = axp209_write(AXP209_LDO24_VOLTAGE, reg);
>> +	if (rc)
>> +		return rc;
>> +
>> +	return 0;
>> +}
>> +
>> +int axp209_set_ldo3(int mvolt)
>> +{
>> +	int cfg = (mvolt - 700) / 25;
>> +
>> +	if (cfg < 0)
>> +		cfg = 0;
>> +	if (cfg > 127)
>> +		cfg = 127;
>> +	if (mvolt == -1)
>> +		cfg = 0x80;	/* detemined by LDO3IN pin */
>
> "determined"

Fixed.

>
>> +
>> +	return axp209_write(AXP209_LDO3_VOLTAGE, cfg);
>> +}
>> +
>> +int axp209_set_ldo4(int mvolt)
>> +{
>> +	int cfg, rc;
>> +	static const int vindex[] = {
>> +		1250, 1300, 1400, 1500, 1600, 1700, 1800, 1900, 2000, 2500,
>> +		2700, 2800, 3000, 3100, 3200, 3300
>> +	};
>> +	u8 reg;
>> +
>> +	/* Translate mvolt to register cfg value, requested <= selected */
>> +	for (cfg = 15; vindex[cfg] > mvolt && cfg > 0; cfg--);
>> +
>> +	rc = axp209_read(AXP209_LDO24_VOLTAGE, &reg);
>> +	if (rc)
>> +		return rc;
>> +
>> +	/* LDO4 configuration is in lower 4 bits */
>> +	reg = (reg & 0xf0) | (cfg << 0);
>> +	rc = axp209_write(AXP209_LDO24_VOLTAGE, reg);
>> +	if (rc)
>> +		return rc;
>> +
>> +	return 0;
>> +}
>> +
>> +void axp209_poweroff(void)
>> +{
>> +	u8 val;
>> +
>> +	if (axp209_read(AXP209_SHUTDOWN, &val) != 0)
>> +		return;
>> +
>> +	val |= 1 << 7;
>
> #define?

Fixed.

Regards,

Hans