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

[Ian Campbell]

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?

>  	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?

> +#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?

> +#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?

> +
> +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?)

> +	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)?

> +	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.

> +	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...)

> +	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"

> +
> +	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?

> +
> +	if (axp209_write(AXP209_SHUTDOWN, val) != 0)
> +		return;
> +
> +	udelay(10000);		/* wait for power to drain */
> +}

Ian.