From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Date: Tue, 03 Jun 2014 19:12:29 +0200 Subject: [U-Boot] [PATCH 08/12] sunxi: Add axp209 pmic support In-Reply-To: <1401556087.15871.122.camel@hastur.hellion.org.uk> References: <1401440772-7462-1-git-send-email-hdegoede@redhat.com> <1401440772-7462-9-git-send-email-hdegoede@redhat.com> <1401556087.15871.122.camel@hastur.hellion.org.uk> Message-ID: <538E01FD.8040502@redhat.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >> >> 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 >> #include >> #include >> @@ -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 >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> + >> +#include >> +#include >> +#include >> + >> +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, ®); > > 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, ®); >> + 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, ®); >> + 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