[U-Boot] [PATCH 02/10] power: Add support for TPS65090 PMU chip.

Lukasz Majewski l.majewski at samsung.com
Mon Mar 31 16:33:18 CEST 2014


Hi Simon,

> Hi Lukasz,
> 
> On 27 March 2014 11:59, Lukasz Majewski <l.majewski at samsung.com>
> wrote: Hi Simon,
> 
> > From: Tom Wai-Hong Tam <waihong at chromium.org>
> >
> > This adds driver support for the TPS65090 PMU. Support includes
> > hooking into the pmic infrastructure  so that the pmic commands
> > can be used on the console. The TPS65090 supports the following
> > functionality:
> >
> > - fet enable/disable/querying
> > - getting and setting of charge state
> >
> > Even though it is connected to the pmic infrastructure it does
> > not hook into the pmic charging charging infrastructure.
> 
> Can I ask what was the problem with adding support for "pmic bat
> [state|charge]" command on this PMIC?
> 
> Was the framework unfriendly or there was no need to do that?
> 
> It's not great. I spent a bit of time trying again. It think the
> issues are:
> 
> - no device tree support
> - no comments in the pmic.h file so it's hard do know what everything
> is for

You are right here - the lack of DT support is the main problem here.

As Tom Rini pointed out - it is problematic in this framework to
support more than one instance of the same PMIC device.

I must confess that I've overlooked this use case.

> - the battery charging command should really be common, not specific
> to each driver

I agree. We can try to discuss one solution suitable for Exynos4 and 5.

> 
> I did actually create a battery system in the Chromium source tree a
> while back, but never sent it upstream, partly because the pmic stuff
> was there and I was not sure how to get it into that framework.
> 
> I think maybe if someone can comment the pmic.h file then I could
> have another try. But it would be a separate series to this one,
> which is focussed on the LCD, not the battery. 

For a quick reference please consider Trats and Trats2. If you need any
more help, then please write an e-mail to me.

> 
> >
> > Signed-off-by: Tom Wai-Hong Tam <waihong at chromium.org>
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > Signed-off-by: Hatim Ali <hatim.rv at samsung.com>
> > Signed-off-by: Katie Roberts-Hoffman <katierh at chromium.org>
> > Signed-off-by: Rong Chang <rongchang at chromium.org>
> > Signed-off-by: Sean Paul <seanpaul at chromium.org>
> > Signed-off-by: Vincent Palatin <vpalatin at chromium.org>
> > Signed-off-by: Aaron Durbin <adurbin at chromium.org>
> > ---
> >
> >  doc/device-tree-bindings/power/tps65090.txt |  21 ++
> >  drivers/power/pmic/Makefile                 |   1 +
> >  drivers/power/pmic/pmic_tps65090.c          | 296
> > ++++++++++++++++++++++++++++
> > include/fdtdec.h                            |   1 +
> > include/power/tps65090_pmic.h               |  83 ++++++++
> > lib/fdtdec.c                                |   1 + 6 files changed,
> > 403 insertions(+) create mode 100644
> > doc/device-tree-bindings/power/tps65090.txt create mode 100644
> > drivers/power/pmic/pmic_tps65090.c create mode 100644
> > include/power/tps65090_pmic.h
> >
> > diff --git a/doc/device-tree-bindings/power/tps65090.txt
> > b/doc/device-tree-bindings/power/tps65090.txt new file mode 100644
> > index 0000000..6a8a884
> > --- /dev/null
> > +++ b/doc/device-tree-bindings/power/tps65090.txt
> > @@ -0,0 +1,21 @@
> > +TPSchrome binding
> > +=================
> > +
> > +The device tree node which describes the operation of the Texas
> > Instrument +TPS65090 power regulator chip is as follows:
> > +
> > +Required properties :
> > +- compatible : "ti,tps65090"
> > +- reg : slave address on the i2c bus
> > +
> > +The tps65090 node should appear as a subnode of the i2c bus that
> > connects it. +
> > +Example
> > +=======
> > +
> > +       i2c at 12ca0000 {
> > +               power-regulator at 48 {
> > +                       compatible = "ti,tps65090"
> > +                       reg = <0x48>;
> > +               };
> > +       };
> 
> Those bindings look very different from the one at Linux kernel for
> this device. Therefore I assume that there was justification to not
> stick to the linux kernel DT format.
> 
> Yes they pre-date the kernel. But now that the kernel has support I
> will pull those in. For the parts we use it is the same. 

That would be great, thanks.

> 
> > diff --git a/drivers/power/pmic/Makefile
> > b/drivers/power/pmic/Makefile index 0b45ffa..7ed55e6 100644
> > --- a/drivers/power/pmic/Makefile
> > +++ b/drivers/power/pmic/Makefile
> > @@ -9,5 +9,6 @@ obj-$(CONFIG_POWER_MAX8998) += pmic_max8998.o
> >  obj-$(CONFIG_POWER_MAX8997) += pmic_max8997.o
> >  obj-$(CONFIG_POWER_MUIC_MAX8997) += muic_max8997.o
> >  obj-$(CONFIG_POWER_MAX77686) += pmic_max77686.o
> > +obj-$(CONFIG_POWER_TPS65090) += pmic_tps65090.o
> >  obj-$(CONFIG_POWER_TPS65217) += pmic_tps65217.o
> >  obj-$(CONFIG_POWER_TPS65910) += pmic_tps65910.o
> > diff --git a/drivers/power/pmic/pmic_tps65090.c
> > b/drivers/power/pmic/pmic_tps65090.c new file mode 100644
> > index 0000000..ef9f911
> > --- /dev/null
> > +++ b/drivers/power/pmic/pmic_tps65090.c
> > @@ -0,0 +1,296 @@
> > +/*
> > + * Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
> > + * Use of this source code is governed by a BSD-style license that
> > can be
> > + * found in the LICENSE file.
> > + *
> > + * Alternatively, this software may be distributed under the terms
> > of the
> > + * GNU General Public License ("GPL") version 2 as published by the
> > Free
> > + * Software Foundation.
> 
> Would it be possible to tune this license description to be similar to
> SPDX-License-Identifier:        GPL-2.0+|BSD
> 
> Yes, will do.
>  
> 
> > + */
> > +
> > +#include <common.h>
> > +#include <errno.h>
> > +#include <fdtdec.h>
> > +#include <i2c.h>
> > +#include <power/pmic.h>
> > +#include <power/tps65090_pmic.h>
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +#define TPS65090_NAME "TPS65090_PMIC"
> > +
> > +/* TPS65090 register addresses */
> > +enum {
> > +       REG_CG_CTRL0 = 4,
> > +       REG_CG_STATUS1 = 0xa,
> > +       REG_FET1_CTRL = 0x0f,
> > +       REG_FET2_CTRL,
> > +       REG_FET3_CTRL,
> > +       REG_FET4_CTRL,
> > +       REG_FET5_CTRL,
> > +       REG_FET6_CTRL,
> > +       REG_FET7_CTRL,
> > +       TPS65090_NUM_REGS,
> > +};
> > +
> > +enum {
> > +       CG_CTRL0_ENC_MASK       = 0x01,
> > +
> > +       MAX_FET_NUM     = 7,
> > +       MAX_CTRL_READ_TRIES = 5,
> > +
> > +       /* TPS65090 FET_CTRL register values */
> > +       FET_CTRL_TOFET          = 1 << 7,  /* Timeout, startup,
> > overload */
> > +       FET_CTRL_PGFET          = 1 << 4,  /* Power good for FET
> > status */
> > +       FET_CTRL_WAIT           = 3 << 2,  /* Overcurrent timeout
> > max */
> > +       FET_CTRL_ADENFET        = 1 << 1,  /* Enable output auto
> > discharge */
> > +       FET_CTRL_ENFET          = 1 << 0,  /* Enable FET */
> > +};
> > +
> > +/**
> > + * Checks for a valid FET number
> > + *
> > + * @param fet_id       FET number to check
> > + * @return 0 if ok, -1 if FET value is out of range
> > + */
> > +static int tps65090_check_fet(unsigned int fet_id)
> > +{
> > +       if (fet_id == 0 || fet_id > MAX_FET_NUM) {
> > +               debug("parameter fet_id is out of range, %u not in 1
> > ~ %u\n",
> > +                     fet_id, MAX_FET_NUM);
> > +               return -1;
> 
> In the recent code (like trats/trats2, bugs fixed at existing power
> infrastructure) I'm encouraging people to use error descriptions from
> <errno.h>, not the -1/-2 values. 
> Can you fix this globally in this patch?
> 
> Yes good idea.
>  
> 
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * Set the power state for a FET
> > + *
> > + * @param pmic         pmic structure for the tps65090
> > + * @param fet_id       Fet number to set (1..MAX_FET_NUM)
> > + * @param set          1 to power on FET, 0 to power off
> > + * @return FET_ERR_COMMS if we got a comms error, FET_ERR_NOT_READY
> > if the
> > + * FET failed to change state. If all is ok, returns 0.
> > + */
> > +static int tps65090_fet_set(struct pmic *pmic, int fet_id, int set)
>                                                               ^^^^
>                                                               maybe
>                                                               bool
> 
> > +{
> > +       int retry;
> > +       u32 reg, value;
> > +
> > +       value = FET_CTRL_ADENFET | FET_CTRL_WAIT;
> > +       if (set)
> > +               value |= FET_CTRL_ENFET;
> > +
> > +       if (pmic_reg_write(pmic, REG_FET1_CTRL + fet_id - 1, value))
> > +               return FET_ERR_COMMS;
> > +       /* Try reading until we get a result */
> > +       for (retry = 0; retry < MAX_CTRL_READ_TRIES; retry++) {
> > +               if (pmic_reg_read(pmic, REG_FET1_CTRL + fet_id - 1,
> > &reg))
> > +                       return FET_ERR_COMMS;
> > +
> > +               /* Check that the fet went into the expected state
> > */
> > +               if (!!(reg & FET_CTRL_PGFET) == set)
> > +                       return 0;
> > +
> > +               /* If we got a timeout, there is no point in waiting
> > longer */
> > +               if (reg & FET_CTRL_TOFET)
> > +                       break;
> > +
> > +               mdelay(1);
> > +       }
> > +
> > +       debug("FET %d: Power good should have set to %d but
> > reg=%#02x\n",
> > +             fet_id, set, reg);
> > +       return FET_ERR_NOT_READY;
> > +}
> > +
> > +int tps65090_fet_enable(unsigned int fet_id)
> > +{
> > +       int loops;
> > +       ulong start;
> > +       struct pmic *pmic;
> > +       int ret = 0;
> > +
> > +       if (tps65090_check_fet(fet_id))
> > +               return -1;
> 
>                 As I've described above - maybe -ENODEV?
> 
> > +
> > +       pmic = pmic_get(TPS65090_NAME);
> > +       if (pmic == NULL)
>              It seems like in the code I tend to use:
>                 if (!pmic)
> 
> OK 
> 
> > +               return -1;
> > +
> > +       start = get_timer(0);
> > +       for (loops = 0;; loops++) {
> > +               ret = tps65090_fet_set(pmic, fet_id, 1);
> > +               if (!ret)
> > +                       break;
> > +
> > +               if (get_timer(start) > 100)
> > +                       break;
> > +
> > +               /* Turn it off and try again until we time out */
> > +               tps65090_fet_set(pmic, fet_id, 0);
> > +       }
> > +
> > +       if (ret) {
> > +               debug("%s: FET%d failed to power on: time=%lums,
> > loops=%d\n",
> > +                     __func__, fet_id, get_timer(start), loops);
> > +       } else if (loops) {
> > +               debug("%s: FET%d powered on after %lums,
> > loops=%d\n",
> > +                     __func__, fet_id, get_timer(start), loops);
> > +       }
> 
> Here the parenthesis can be omitted.
> 
> OK 
> 
> > +       /*
> > +        * Unfortunately, there are some conditions where the power
> > +        * good bit will be 0, but the fet still comes up. One such
> > +        * case occurs with the lcd backlight. We'll just return 0
> > here
> > +        * and assume that the fet will eventually come up.
> > +        */
> > +       if (ret == FET_ERR_NOT_READY)
> > +               ret = 0;
> > +
> > +       return ret;
> > +}
> > +
> > +int tps65090_fet_disable(unsigned int fet_id)
> > +{
> > +       int ret;
> > +       struct pmic *pmic;
> > +
> > +       if (tps65090_check_fet(fet_id))
> > +               return -1;
> > +
> > +       pmic = pmic_get(TPS65090_NAME);
> > +       if (pmic == NULL)
> > +               return -1;
> > +       ret = tps65090_fet_set(pmic, fet_id, 0);
> > +
> > +       return ret;
> > +}
> > +
> > +int tps65090_fet_is_enabled(unsigned int fet_id)
> > +{
> > +       u32 reg;
> > +       int ret;
> > +       struct pmic *pmic;
> > +
> > +       if (tps65090_check_fet(fet_id))
> > +               return -1;
> > +       pmic = pmic_get(TPS65090_NAME);
> > +       if (pmic == NULL)
> > +               return -1;
> > +       ret = pmic_reg_read(pmic, REG_FET1_CTRL + fet_id - 1, &reg);
> > +       if (ret) {
> > +               debug("fail to read FET%u_CTRL register over I2C",
> > fet_id);
> > +               return -2;
> > +       }
> > +
> > +       return reg & FET_CTRL_ENFET;
> > +}
> > +
> > +int tps65090_get_charging(void)
> > +{
> > +       u32 val;
> > +       int ret;
> > +       struct pmic *pmic;
> > +
> > +       pmic = pmic_get(TPS65090_NAME);
> > +       if (pmic == NULL)
> > +               return -1;
> > +       ret = pmic_reg_read(pmic, REG_CG_CTRL0, &val);
> > +       if (ret)
> > +               return ret;
> > +       return val & CG_CTRL0_ENC_MASK ? 1 : 0;
> > +}
> > +
> > +int tps65090_set_charge_enable(int enable)
> 
> This can be easily added to be used at "pmic bat charge" command.
> 
> Please look into the ./drivers/power/mfd/pmic_max77693.c
> 
> Not easily. As mentioned above this is quite a bit of work. For a
> later series I think. 

I'm looking forward for questions :-).

> Regards,
> Simon
> 



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group


More information about the U-Boot mailing list