diff mbox

[v3,5/5] regulator: tps65090: Make FETs more reliable by adding retries

Message ID 1397689950-1568-6-git-send-email-dianders@chromium.org (mailing list archive)
State New, archived
Headers show

Commit Message

Doug Anderson April 16, 2014, 11:12 p.m. UTC
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@chromium.org>
Signed-off-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Michael Spang <spang@chromium.org>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
Changes in v3:
- Fixed kernel-doc notation for return

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 | 155 +++++++++++++++++++++++++++++----
 1 file changed, 140 insertions(+), 15 deletions(-)

Comments

Mark Brown April 18, 2014, 5:43 p.m. UTC | #1
On Wed, Apr 16, 2014 at 04:12:29PM -0700, Doug Anderson 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

This is basically fine but you said it depends on one of the previous
patches which you didn't CC me on so I've no idea what's going on
there?
Doug Anderson April 18, 2014, 6:05 p.m. UTC | #2
Mark,

On Fri, Apr 18, 2014 at 10:43 AM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Apr 16, 2014 at 04:12:29PM -0700, Doug Anderson 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
>
> This is basically fine but you said it depends on one of the previous
> patches which you didn't CC me on so I've no idea what's going on
> there?

Sorry about that.  :(

Lee has Acked the caching patch.  Lee: what's the best way for you and
Mark to coordinate there?  Should he apply the caching patch with your
Ack?

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lee Jones April 22, 2014, 7:47 a.m. UTC | #3
> Mark,
> 
> On Fri, Apr 18, 2014 at 10:43 AM, Mark Brown <broonie@kernel.org> wrote:
> > On Wed, Apr 16, 2014 at 04:12:29PM -0700, Doug Anderson 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
> >
> > This is basically fine but you said it depends on one of the previous
> > patches which you didn't CC me on so I've no idea what's going on
> > there?
> 
> Sorry about that.  :(
> 
> Lee has Acked the caching patch.  Lee: what's the best way for you and
> Mark to coordinate there?  Should he apply the caching patch with your
> Ack?

If there are cross-subsystem dependencies I prefer to use immutable
branches to eliminate any change of merge conflicts in -next or the
next merge window. I'm happy to either create on with Mark's Acks, or
receive a pull-request from Mark with mine applied.

Up to Mark.
Mark Brown April 22, 2014, 11:02 a.m. UTC | #4
On Tue, Apr 22, 2014 at 08:47:09AM +0100, Lee Jones wrote:

> If there are cross-subsystem dependencies I prefer to use immutable
> branches to eliminate any change of merge conflicts in -next or the
> next merge window. I'm happy to either create on with Mark's Acks, or
> receive a pull-request from Mark with mine applied.

> Up to Mark.

Well, Doug didn't send me the MFD patch so I can't do anything with it
anyway.
Lee Jones April 22, 2014, 3:07 p.m. UTC | #5
> > If there are cross-subsystem dependencies I prefer to use immutable
> > branches to eliminate any change of merge conflicts in -next or the
> > next merge window. I'm happy to either create on with Mark's Acks, or
> > receive a pull-request from Mark with mine applied.
> 
> > Up to Mark.
> 
> Well, Doug didn't send me the MFD patch so I can't do anything with it
> anyway.

Ah!

Doug,
  what exactly are the dependencies within the set?
Doug Anderson April 22, 2014, 3:28 p.m. UTC | #6
Hi,

On Tue, Apr 22, 2014 at 8:07 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> > If there are cross-subsystem dependencies I prefer to use immutable
>> > branches to eliminate any change of merge conflicts in -next or the
>> > next merge window. I'm happy to either create on with Mark's Acks, or
>> > receive a pull-request from Mark with mine applied.
>>
>> > Up to Mark.
>>
>> Well, Doug didn't send me the MFD patch so I can't do anything with it
>> anyway.
>
> Ah!
>
> Doug,
>   what exactly are the dependencies within the set?

I will repost the patch shortly with Mark on the CC list.  Status of
the patches in this series:

- Patch #1 (mfd no irq) has no dependencies, though patch #2 won't
  work without it.

https://patchwork.kernel.org/patch/4004441/

Acked by you.  Not landed anywhere that I'm aware of.

--

- Patch #2 (charger polling) can be applied without patch #1 but won't
  actually make charger polling work without it.

https://patchwork.kernel.org/patch/4004461/

Not acked or applied anywhere.

---

- Patch #3 (caching) can be applied before retries patch but not
  after.

https://patchwork.kernel.org/patch/4033361/

This is the patch we need.  I just resent with Mark on the CC list
(including your ack).

---

- Patch #4 (overcurrent wait time) can be applied before retries patch
  but not after (just due to merge conflicts, no other reason).

https://patchwork.kernel.org/patch/4004471/

I believe Mark has already applied.

---

- Patch #5 (retries) absolutely requires patch #3 (caching).

https://patchwork.kernel.org/patch/4004521/
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/regulator/tps65090-regulator.c b/drivers/regulator/tps65090-regulator.c
index ca04e9f..2057e2e 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
+
+#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 */
 
@@ -80,40 +87,158 @@  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)