Re: [PATCH v2 5/5] regulator: tps65090: Make FETs more reliable by adding retries

From: Simon Glass
Date: Wed Apr 16 2014 - 16:50:41 EST

Next message: Andrew Morton: "Re: [PATCH] Dead code removal in madvise_willneed"
Previous message: Tejun Heo: "Re: [PATCH V2 1/2] ata: ahci: append new hflag AHCI_HFLAG_NO_FBS"
In reply to: Doug Anderson: "[PATCH v2 5/5] regulator: tps65090: Make FETs more reliable by adding retries"
Next in thread: Doug Anderson: "Re: [PATCH v2 5/5] regulator: tps65090: Make FETs more reliable by adding retries"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Doug, (take 2)

On 16 April 2014 12:25, Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
> An issue was discovered with tps65090 where sometimes the FETs
> wouldn't actually turn on when requested (they would report
> overcurrent). The most problematic FET was the one used for the LCD
> backlight on the Samsung ARM Chromebook (FET1). Problems were
> especially prevalent when the device was plugged in to AC power (when
> the backlight voltage was higher).
>
> Mitigate the problem by adding retries on the enables of the FETs,
> which works around the problem fairly effectively.
>
> Signed-off-by: Doug Anderson <dianders@xxxxxxxxxxxx>
> Signed-off-by: Simon Glass <sjg@xxxxxxxxxxxx>
> Signed-off-by: Michael Spang <spang@xxxxxxxxxxxx>
> Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
> ---
> Changes in v2:
> - Separated the overcurrent and retries changes into two patches.
> - No longer open code fet_is_enabled().
> - Fixed tps6090 typo.
> - For loop => "while true".
> - Removed a set of braces.
>
> drivers/regulator/tps65090-regulator.c | 153 +++++++++++++++++++++++++++++----
> 1 file changed, 138 insertions(+), 15 deletions(-)

Reviewed-by: Simon Glass <sjg@xxxxxxxxxxxx>

(see minor comment below)

>
> diff --git a/drivers/regulator/tps65090-regulator.c b/drivers/regulator/tps65090-regulator.c
> index ca13a1a..c37ffb72 100644
> --- a/drivers/regulator/tps65090-regulator.c
> +++ b/drivers/regulator/tps65090-regulator.c
> @@ -17,6 +17,7 @@
> */
>
> #include <linux/module.h>
> +#include <linux/delay.h>
> #include <linux/init.h>
> #include <linux/gpio.h>
> #include <linux/of_gpio.h>
> @@ -28,7 +29,13 @@
> #include <linux/regulator/of_regulator.h>
> #include <linux/mfd/tps65090.h>
>
> +#define MAX_CTRL_READ_TRIES 5
> +#define MAX_FET_ENABLE_TRIES 1000

Gosh that is a lot of tries - should we maybe give up sooner?

> +
> +#define CTRL_EN_BIT 0 /* Regulator enable bit, active high */
> #define CTRL_WT_BIT 2 /* Regulator wait time 0 bit */
> +#define CTRL_PG_BIT 4 /* Regulator power good bit, 1=good */
> +#define CTRL_TO_BIT 7 /* Regulator timeout bit, 1=wait */
>
> #define MAX_OVERCURRENT_WAIT 3 /* Overcurrent wait must be <= this */
>
> @@ -79,40 +86,156 @@ static int tps65090_reg_set_overcurrent_wait(struct tps65090_regulator *ri,
> return ret;
> }
>
> -static struct regulator_ops tps65090_reg_contol_ops = {
> +/**
> + * tps65090_try_enable_fet - Try to enable a FET
> + *
> + * @rdev: Regulator device
> + * @return 0 if ok, -ENOTRECOVERABLE if the FET power good bit did not get set,
> + * or some other -ve value if another error occurred (e.g. i2c error)
> + */
> +static int tps65090_try_enable_fet(struct regulator_dev *rdev)
> +{
> + unsigned int control;
> + int ret, i;
> +
> + ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
> + rdev->desc->enable_mask,
> + rdev->desc->enable_mask);
> + if (ret < 0) {
> + dev_err(&rdev->dev, "Error in updating reg %#x\n",
> + rdev->desc->enable_reg);
> + return ret;
> + }
> +
> + for (i = 0; i < MAX_CTRL_READ_TRIES; i++) {
> + ret = regmap_read(rdev->regmap, rdev->desc->enable_reg,
> + &control);
> + if (ret < 0)
> + return ret;
> +
> + if (!(control & BIT(CTRL_TO_BIT)))
> + break;
> +
> + usleep_range(1000, 1500);
> + }
> + if (!(control & BIT(CTRL_PG_BIT)))
> + return -ENOTRECOVERABLE;
> +
> + return 0;
> +}
> +
> +/**
> + * tps65090_fet_enable - Enable a FET, trying a few times if it fails
> + *
> + * Some versions of the tps65090 have issues when turning on the FETs.
> + * This function goes through several steps to ensure the best chance of the
> + * FET going on. Specifically:
> + * - We'll make sure that we bump the "overcurrent wait" to the maximum, which
> + * increases the chances that we'll turn on properly.
> + * - We'll retry turning the FET on multiple times (turning off in between).
> + *
> + * @rdev: Regulator device
> + * @return 0 if ok, non-zero if it fails.
> + */
> +static int tps65090_fet_enable(struct regulator_dev *rdev)
> +{
> + int ret, tries;
> +
> + /*
> + * Try enabling multiple times until we succeed since sometimes the
> + * first try times out.
> + */
> + tries = 0;
> + while (true) {
> + ret = tps65090_try_enable_fet(rdev);
> + if (!ret)
> + break;
> + if (ret != -ENOTRECOVERABLE || tries == MAX_FET_ENABLE_TRIES)
> + goto err;
> +
> + /* Try turning the FET off (and then on again) */
> + ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
> + rdev->desc->enable_mask, 0);
> + if (ret)
> + goto err;
> +
> + tries++;
> + }
> +
> + if (tries)
> + dev_warn(&rdev->dev, "reg %#x enable ok after %d tries\n",
> + rdev->desc->enable_reg, tries);
> +
> + return 0;
> +err:
> + dev_warn(&rdev->dev, "reg %#x enable failed\n", rdev->desc->enable_reg);
> + WARN_ON(1);
> +
> + return ret;
> +}
> +
> +static struct regulator_ops tps65090_reg_control_ops = {
> .enable = regulator_enable_regmap,
> .disable = regulator_disable_regmap,
> .is_enabled = regulator_is_enabled_regmap,
> };
>
> +static struct regulator_ops tps65090_fet_control_ops = {
> + .enable = tps65090_fet_enable,
> + .disable = regulator_disable_regmap,
> + .is_enabled = regulator_is_enabled_regmap,
> +};
> +
> static struct regulator_ops tps65090_ldo_ops = {
> };
>
> -#define tps65090_REG_DESC(_id, _sname, _en_reg, _ops) \
> +#define tps65090_REG_DESC(_id, _sname, _en_reg, _en_bits, _ops) \
> { \
> .name = "TPS65090_RAILS"#_id, \
> .supply_name = _sname, \
> .id = TPS65090_REGULATOR_##_id, \
> .ops = &_ops, \
> .enable_reg = _en_reg, \
> - .enable_mask = BIT(0), \
> + .enable_val = _en_bits, \
> + .enable_mask = _en_bits, \
> .type = REGULATOR_VOLTAGE, \
> .owner = THIS_MODULE, \
> }
>
> static struct regulator_desc tps65090_regulator_desc[] = {
> - tps65090_REG_DESC(DCDC1, "vsys1", 0x0C, tps65090_reg_contol_ops),
> - tps65090_REG_DESC(DCDC2, "vsys2", 0x0D, tps65090_reg_contol_ops),
> - tps65090_REG_DESC(DCDC3, "vsys3", 0x0E, tps65090_reg_contol_ops),
> - tps65090_REG_DESC(FET1, "infet1", 0x0F, tps65090_reg_contol_ops),
> - tps65090_REG_DESC(FET2, "infet2", 0x10, tps65090_reg_contol_ops),
> - tps65090_REG_DESC(FET3, "infet3", 0x11, tps65090_reg_contol_ops),
> - tps65090_REG_DESC(FET4, "infet4", 0x12, tps65090_reg_contol_ops),
> - tps65090_REG_DESC(FET5, "infet5", 0x13, tps65090_reg_contol_ops),
> - tps65090_REG_DESC(FET6, "infet6", 0x14, tps65090_reg_contol_ops),
> - tps65090_REG_DESC(FET7, "infet7", 0x15, tps65090_reg_contol_ops),
> - tps65090_REG_DESC(LDO1, "vsys-l1", 0, tps65090_ldo_ops),
> - tps65090_REG_DESC(LDO2, "vsys-l2", 0, tps65090_ldo_ops),
> + tps65090_REG_DESC(DCDC1, "vsys1", 0x0C, BIT(CTRL_EN_BIT),
> + tps65090_reg_control_ops),
> + tps65090_REG_DESC(DCDC2, "vsys2", 0x0D, BIT(CTRL_EN_BIT),
> + tps65090_reg_control_ops),
> + tps65090_REG_DESC(DCDC3, "vsys3", 0x0E, BIT(CTRL_EN_BIT),
> + tps65090_reg_control_ops),
> +
> + tps65090_REG_DESC(FET1, "infet1", 0x0F,
> + BIT(CTRL_EN_BIT) | BIT(CTRL_PG_BIT),
> + tps65090_fet_control_ops),
> + tps65090_REG_DESC(FET2, "infet2", 0x10,
> + BIT(CTRL_EN_BIT) | BIT(CTRL_PG_BIT),
> + tps65090_fet_control_ops),
> + tps65090_REG_DESC(FET3, "infet3", 0x11,
> + BIT(CTRL_EN_BIT) | BIT(CTRL_PG_BIT),
> + tps65090_fet_control_ops),
> + tps65090_REG_DESC(FET4, "infet4", 0x12,
> + BIT(CTRL_EN_BIT) | BIT(CTRL_PG_BIT),
> + tps65090_fet_control_ops),
> + tps65090_REG_DESC(FET5, "infet5", 0x13,
> + BIT(CTRL_EN_BIT) | BIT(CTRL_PG_BIT),
> + tps65090_fet_control_ops),
> + tps65090_REG_DESC(FET6, "infet6", 0x14,
> + BIT(CTRL_EN_BIT) | BIT(CTRL_PG_BIT),
> + tps65090_fet_control_ops),
> + tps65090_REG_DESC(FET7, "infet7", 0x15,
> + BIT(CTRL_EN_BIT) | BIT(CTRL_PG_BIT),
> + tps65090_fet_control_ops),
> +
> + tps65090_REG_DESC(LDO1, "vsys-l1", 0, 0,
> + tps65090_ldo_ops),
> + tps65090_REG_DESC(LDO2, "vsys-l2", 0, 0,
> + tps65090_ldo_ops),
> };
>
> static inline bool is_dcdc(int id)
> --
> 1.9.1.423.g4596e3a
>

Regards,
Simon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Andrew Morton: "Re: [PATCH] Dead code removal in madvise_willneed"
Previous message: Tejun Heo: "Re: [PATCH V2 1/2] ata: ahci: append new hflag AHCI_HFLAG_NO_FBS"
In reply to: Doug Anderson: "[PATCH v2 5/5] regulator: tps65090: Make FETs more reliable by adding retries"
Next in thread: Doug Anderson: "Re: [PATCH v2 5/5] regulator: tps65090: Make FETs more reliable by adding retries"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]