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 <hen...@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 <hen...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to