These three patches bring tps65090 up to speed with what's currently
in the Chromium OS kernel 3.8 tree and running on the Samsung ARM
Chromebook. Changes were tested atop the current linux tree
(v3.15-rc1). FET retries were tested on a machine with a known flaky
tps65090. Since display isn't working on pure upstream, I augmented
the code to turn FET1 (vcd_led) on/off 500 times at bootup. When
testing I included <https://patchwork.kernel.org/patch/3980731/> to
make sure tps65090 was in exynos5250-snow's device tree.
Doug Anderson (3):
mfd: tps65090: Allow charger module to be used when no irq
mfd: tps65090: Stop caching registers
regulator: tps65090: Make FETs more reliable
.../devicetree/bindings/regulator/tps65090.txt | 4 +
drivers/mfd/tps65090.c | 24 ++-
drivers/power/tps65090-charger.c | 76 ++++++--
drivers/regulator/tps65090-regulator.c | 197 +++++++++++++++++++--
include/linux/mfd/tps65090.h | 5 +
5 files changed, 264 insertions(+), 42 deletions(-)
--
1.9.1.423.g4596e3a
An issue was discovered with tps65090 where sometimes the FETs
wouldn't actually turn on when requested. The most problematic FET
was the one use for the LCD backlight on the Samsung ARM Chromebook
(FET1). Problems were especially prevalent when the device was
plugged in to AC power, making the backlight voltage higher.
Mitigate the problem by:
* Allow setting the overcurrent wait time so devices with this problem
can set it to the max.
* Add retry logic on enables of FETs.
Signed-off-by: Doug Anderson <[email protected]>
Signed-off-by: Simon Glass <[email protected]>
Signed-off-by: Michael Spang <[email protected]>
Signed-off-by: Sean Paul <[email protected]>
---
.../devicetree/bindings/regulator/tps65090.txt | 4 +
drivers/regulator/tps65090-regulator.c | 197 +++++++++++++++++++--
include/linux/mfd/tps65090.h | 5 +
3 files changed, 194 insertions(+), 12 deletions(-)
diff --git a/Documentation/devicetree/bindings/regulator/tps65090.txt b/Documentation/devicetree/bindings/regulator/tps65090.txt
index 313a60b..34098023 100644
--- a/Documentation/devicetree/bindings/regulator/tps65090.txt
+++ b/Documentation/devicetree/bindings/regulator/tps65090.txt
@@ -21,6 +21,10 @@ Optional properties:
number should be provided. If it is externally controlled and no GPIO
entry then driver will just configure this rails as external control
and will not provide any enable/disable APIs.
+- ti,overcurrent-wait: This is applicable to FET registers, which have a
+ poorly defined "overcurrent wait" field. If this property is present it
+ should be between 0 - 3. If this property isn't present we won't touch the
+ "overcurrent wait" field and we'll leave it to the BIOS/EC to deal with.
Each regulator is defined using the standard binding for regulators.
diff --git a/drivers/regulator/tps65090-regulator.c b/drivers/regulator/tps65090-regulator.c
index 2e92ef6..e8d1c62 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,21 +29,186 @@
#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 less than this */
+
+/**
+ * struct tps65090_regulator - Per-regulator data for a tps65090 regulator
+ *
+ * @dev: Pointer to our device.
+ * @desc: The struct regulator_desc for the regulator.
+ * @rdev: The struct regulator_dev for the regulator.
+ * @overcurrent_wait_valid: True if overcurrent_wait is valid.
+ * @overcurrent_wait: For FETs, the value to put in the WTFET bitfield.
+ */
+
struct tps65090_regulator {
struct device *dev;
struct regulator_desc *desc;
struct regulator_dev *rdev;
+ bool overcurrent_wait_valid;
+ int overcurrent_wait;
};
static struct regulator_ops tps65090_ext_control_ops = {
};
-static struct regulator_ops tps65090_reg_contol_ops = {
+/**
+ * tps65090_fet_is_enabled - Tell if a fet is enabled
+ *
+ * @rdev: Regulator device
+ * @return true if the fet is enabled and its power is good; false otherwise.
+ */
+static int tps65090_fet_is_enabled(struct regulator_dev *rdev)
+{
+ unsigned int control;
+ unsigned int expected = rdev->desc->enable_mask | BIT(CTRL_PG_BIT);
+ int ret;
+
+ ret = regmap_read(rdev->regmap, rdev->desc->enable_reg, &control);
+ if (ret != 0)
+ return ret;
+
+ return (control & expected) == expected;
+}
+
+/**
+ * tps65090_reg_set_overcurrent_wait - Setup overcurrent wait
+ *
+ * This will set the overcurrent wait time based on what's in the regulator
+ * info.
+ *
+ * @rdev: Regulator device
+ * @return 0 if no error, non-zero if there was an error writing the register.
+ */
+static int tps65090_reg_set_overcurrent_wait(struct regulator_dev *rdev)
+{
+ struct tps65090_regulator *ri = rdev_get_drvdata(rdev);
+ int ret;
+
+ if (!ri->overcurrent_wait_valid)
+ return 0;
+
+ ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
+ MAX_OVERCURRENT_WAIT << CTRL_WT_BIT,
+ ri->overcurrent_wait << CTRL_WT_BIT);
+ if (ret) {
+ dev_err(&rdev->dev, "Error updating overcurrent wait %#x\n",
+ rdev->desc->enable_reg);
+ }
+
+ return ret;
+}
+
+/**
+ * tps6090_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 tps6090_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;
+
+ ret = tps65090_reg_set_overcurrent_wait(rdev);
+ if (ret)
+ goto err;
+
+ /*
+ * Try enabling multiple times until we succeed since sometimes the
+ * first try times out.
+ */
+ for (tries = 0; ; tries++) {
+ ret = tps6090_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;
+ }
+
+ 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 = tps65090_fet_is_enabled,
+};
+
static struct regulator_ops tps65090_ldo_ops = {
};
@@ -53,22 +219,22 @@ static struct regulator_ops tps65090_ldo_ops = {
.id = TPS65090_REGULATOR_##_id, \
.ops = &_ops, \
.enable_reg = _en_reg, \
- .enable_mask = BIT(0), \
+ .enable_mask = BIT(CTRL_EN_BIT), \
.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(DCDC1, "vsys1", 0x0C, tps65090_reg_control_ops),
+ tps65090_REG_DESC(DCDC2, "vsys2", 0x0D, tps65090_reg_control_ops),
+ tps65090_REG_DESC(DCDC3, "vsys3", 0x0E, tps65090_reg_control_ops),
+ tps65090_REG_DESC(FET1, "infet1", 0x0F, tps65090_fet_control_ops),
+ tps65090_REG_DESC(FET2, "infet2", 0x10, tps65090_fet_control_ops),
+ tps65090_REG_DESC(FET3, "infet3", 0x11, tps65090_fet_control_ops),
+ tps65090_REG_DESC(FET4, "infet4", 0x12, tps65090_fet_control_ops),
+ tps65090_REG_DESC(FET5, "infet5", 0x13, tps65090_fet_control_ops),
+ tps65090_REG_DESC(FET6, "infet6", 0x14, tps65090_fet_control_ops),
+ tps65090_REG_DESC(FET7, "infet7", 0x15, tps65090_fet_control_ops),
tps65090_REG_DESC(LDO1, "vsys-l1", 0, tps65090_ldo_ops),
tps65090_REG_DESC(LDO2, "vsys-l2", 0, tps65090_ldo_ops),
};
@@ -209,6 +375,11 @@ static struct tps65090_platform_data *tps65090_parse_dt_reg_data(
rpdata->gpio = of_get_named_gpio(np,
"dcdc-ext-control-gpios", 0);
+ if (of_property_read_u32(tps65090_matches[idx].of_node,
+ "ti,overcurrent-wait",
+ &rpdata->overcurrent_wait) == 0)
+ rpdata->overcurrent_wait_valid = true;
+
tps65090_pdata->reg_pdata[idx] = rpdata;
}
return tps65090_pdata;
@@ -258,6 +429,8 @@ static int tps65090_regulator_probe(struct platform_device *pdev)
ri = &pmic[num];
ri->dev = &pdev->dev;
ri->desc = &tps65090_regulator_desc[num];
+ ri->overcurrent_wait_valid = tps_pdata->overcurrent_wait_valid;
+ ri->overcurrent_wait = tps_pdata->overcurrent_wait;
/*
* TPS5090 DCDC support the control from external digital input.
diff --git a/include/linux/mfd/tps65090.h b/include/linux/mfd/tps65090.h
index 3f43069..f25adfa 100644
--- a/include/linux/mfd/tps65090.h
+++ b/include/linux/mfd/tps65090.h
@@ -78,11 +78,16 @@ struct tps65090 {
* DCDC1, DCDC2 and DCDC3.
* @gpio: Gpio number if external control is enabled and controlled through
* gpio.
+ * @overcurrent_wait_valid: True if the overcurrent_wait should be applied.
+ * @overcurrent_wait: Value to set as the overcurrent wait time. This is the
+ * actual bitfield value, not a time in ms (valid value are 0 - 3).
*/
struct tps65090_regulator_plat_data {
struct regulator_init_data *reg_init_data;
bool enable_ext_control;
int gpio;
+ bool overcurrent_wait_valid;
+ int overcurrent_wait;
};
struct tps65090_platform_data {
--
1.9.1.423.g4596e3a
On the ARM Chromebook tps65090 has two masters: the AP (the main
processor running linux) and the EC (the embedded controller). The AP
is allowed to mess with FETs but the EC is in charge of charge control.
The tps65090 interupt line is routed to both the AP and the EC, which
can cause quite a headache. Having two people adjusting masks and
acking interrupts is a recipe for disaster.
In the shipping kernel we had a hack to have the AP pay attention to
the IRQ but not to ack it. It also wasn't supposed to configure the
IRQ in any way. That hack allowed us to detect when the device was
charging without messing with the EC's state.
The current tps65090 infrastructure makes the above difficult, and it
was a bit of a hack to begin with. Rather than uglify the driver to
support it, just extend the driver's existing notion of "no irq" to
the charger. This makes the charger code poll every 2 seconds for AC
detect, which is sufficient.
Signed-off-by: Doug Anderson <[email protected]>
---
drivers/mfd/tps65090.c | 14 ++++++--
drivers/power/tps65090-charger.c | 76 +++++++++++++++++++++++++++++++---------
2 files changed, 70 insertions(+), 20 deletions(-)
diff --git a/drivers/mfd/tps65090.c b/drivers/mfd/tps65090.c
index ba1a25d..c3cddb4 100644
--- a/drivers/mfd/tps65090.c
+++ b/drivers/mfd/tps65090.c
@@ -64,11 +64,16 @@ static struct resource charger_resources[] = {
}
};
-static const struct mfd_cell tps65090s[] = {
- {
+enum tps65090_cells {
+ PMIC = 0,
+ CHARGER = 1,
+};
+
+static struct mfd_cell tps65090s[] = {
+ [PMIC] = {
.name = "tps65090-pmic",
},
- {
+ [CHARGER] = {
.name = "tps65090-charger",
.num_resources = ARRAY_SIZE(charger_resources),
.resources = &charger_resources[0],
@@ -211,6 +216,9 @@ static int tps65090_i2c_probe(struct i2c_client *client,
"IRQ init failed with err: %d\n", ret);
return ret;
}
+ } else {
+ /* Don't tell children they have an IRQ that'll never fire */
+ tps65090s[CHARGER].num_resources = 0;
}
ret = mfd_add_devices(tps65090->dev, -1, tps65090s,
diff --git a/drivers/power/tps65090-charger.c b/drivers/power/tps65090-charger.c
index 8fc9d6d..cc26c12 100644
--- a/drivers/power/tps65090-charger.c
+++ b/drivers/power/tps65090-charger.c
@@ -17,9 +17,11 @@
*/
#include <linux/delay.h>
#include <linux/err.h>
+#include <linux/freezer.h>
#include <linux/init.h>
#include <linux/interrupt.h>
#include <linux/kernel.h>
+#include <linux/kthread.h>
#include <linux/module.h>
#include <linux/of_device.h>
#include <linux/platform_device.h>
@@ -43,11 +45,15 @@
#define TPS65090_VACG BIT(1)
#define TPS65090_NOITERM BIT(5)
+#define POLL_INTERVAL (HZ * 2) /* Used when no irq */
+
struct tps65090_charger {
struct device *dev;
int ac_online;
int prev_ac_online;
int irq;
+ struct task_struct *poll_task;
+ bool passive_mode;
struct power_supply ac;
struct tps65090_platform_data *pdata;
};
@@ -60,6 +66,9 @@ static int tps65090_low_chrg_current(struct tps65090_charger *charger)
{
int ret;
+ if (charger->passive_mode)
+ return 0;
+
ret = tps65090_write(charger->dev->parent, TPS65090_REG_CG_CTRL5,
TPS65090_NOITERM);
if (ret < 0) {
@@ -75,6 +84,9 @@ static int tps65090_enable_charging(struct tps65090_charger *charger)
int ret;
uint8_t ctrl0 = 0;
+ if (charger->passive_mode)
+ return 0;
+
ret = tps65090_read(charger->dev->parent, TPS65090_REG_CG_CTRL0,
&ctrl0);
if (ret < 0) {
@@ -98,6 +110,9 @@ static int tps65090_config_charger(struct tps65090_charger *charger)
uint8_t intrmask = 0;
int ret;
+ if (charger->passive_mode)
+ return 0;
+
if (charger->pdata->enable_low_current_chrg) {
ret = tps65090_low_chrg_current(charger);
if (ret < 0) {
@@ -175,10 +190,14 @@ static irqreturn_t tps65090_charger_isr(int irq, void *dev_id)
}
/* Clear interrupts. */
- ret = tps65090_write(charger->dev->parent, TPS65090_REG_INTR_STS, 0x00);
- if (ret < 0) {
- dev_err(charger->dev, "%s(): Error in writing reg 0x%x\n",
+ if (!charger->passive_mode) {
+ ret = tps65090_write(charger->dev->parent,
+ TPS65090_REG_INTR_STS, 0x00);
+ if (ret < 0) {
+ dev_err(charger->dev,
+ "%s(): Error in writing reg 0x%x\n",
__func__, TPS65090_REG_INTR_STS);
+ }
}
if (charger->prev_ac_online != charger->ac_online)
@@ -209,6 +228,18 @@ static struct tps65090_platform_data *
}
+static int tps65090_charger_poll_task(void *data)
+{
+ set_freezable();
+
+ while (!kthread_should_stop()) {
+ schedule_timeout_interruptible(POLL_INTERVAL);
+ try_to_freeze();
+ tps65090_charger_isr(-1, data);
+ }
+ return 0;
+}
+
static int tps65090_charger_probe(struct platform_device *pdev)
{
struct tps65090_charger *cdata;
@@ -255,22 +286,10 @@ static int tps65090_charger_probe(struct platform_device *pdev)
}
irq = platform_get_irq(pdev, 0);
- if (irq <= 0) {
- dev_warn(&pdev->dev, "Unable to get charger irq = %d\n", irq);
- ret = irq;
- goto fail_unregister_supply;
- }
-
+ if (irq < 0)
+ irq = NO_IRQ;
cdata->irq = irq;
- ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
- tps65090_charger_isr, 0, "tps65090-charger", cdata);
- if (ret) {
- dev_err(cdata->dev, "Unable to register irq %d err %d\n", irq,
- ret);
- goto fail_unregister_supply;
- }
-
ret = tps65090_config_charger(cdata);
if (ret < 0) {
dev_err(&pdev->dev, "charger config failed, err %d\n", ret);
@@ -296,6 +315,27 @@ static int tps65090_charger_probe(struct platform_device *pdev)
power_supply_changed(&cdata->ac);
}
+ if (irq != NO_IRQ) {
+ ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+ tps65090_charger_isr, 0, "tps65090-charger", cdata);
+ if (ret) {
+ dev_err(cdata->dev,
+ "Unable to register irq %d err %d\n", irq,
+ ret);
+ goto fail_unregister_supply;
+ }
+ } else {
+ cdata->poll_task = kthread_run(tps65090_charger_poll_task,
+ cdata, "ktps65090charger");
+ cdata->passive_mode = true;
+ if (IS_ERR(cdata->poll_task)) {
+ ret = PTR_ERR(cdata->poll_task);
+ dev_err(cdata->dev,
+ "Unable to run kthread err %d\n", ret);
+ goto fail_unregister_supply;
+ }
+ }
+
return 0;
fail_unregister_supply:
@@ -308,6 +348,8 @@ static int tps65090_charger_remove(struct platform_device *pdev)
{
struct tps65090_charger *cdata = platform_get_drvdata(pdev);
+ if (cdata->irq == NO_IRQ)
+ kthread_stop(cdata->poll_task);
power_supply_unregister(&cdata->ac);
return 0;
--
1.9.1.423.g4596e3a
Nearly all of the registers in tps65090 combine control bits and
status bits. Turn off caching of registers so that we can read status
bits reliably.
NOTE: the IRQnMASK and CG_CTRLn registers are the exception and could
be cached. If we find that we spend a lot of time reading those we
can turn on cache for just those registers.
Signed-off-by: Doug Anderson <[email protected]>
---
drivers/mfd/tps65090.c | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/drivers/mfd/tps65090.c b/drivers/mfd/tps65090.c
index c3cddb4..4cfdd07 100644
--- a/drivers/mfd/tps65090.c
+++ b/drivers/mfd/tps65090.c
@@ -149,21 +149,11 @@ static struct regmap_irq_chip tps65090_irq_chip = {
.mask_invert = true,
};
-static bool is_volatile_reg(struct device *dev, unsigned int reg)
-{
- if ((reg == TPS65090_INT_STS) || (reg == TPS65090_INT_STS2))
- return true;
- else
- return false;
-}
-
static const struct regmap_config tps65090_regmap_config = {
.reg_bits = 8,
.val_bits = 8,
.max_register = TOTAL_NUM_REG,
.num_reg_defaults_raw = TOTAL_NUM_REG,
- .cache_type = REGCACHE_RBTREE,
- .volatile_reg = is_volatile_reg,
};
#ifdef CONFIG_OF
--
1.9.1.423.g4596e3a
On Tue, Apr 15, 2014 at 01:14:36PM -0700, Doug Anderson wrote:
> Mitigate the problem by:
> * Allow setting the overcurrent wait time so devices with this problem
> can set it to the max.
> * Add retry logic on enables of FETs.
This is two changes, should really be two patches.
> +- ti,overcurrent-wait: This is applicable to FET registers, which have a
> + poorly defined "overcurrent wait" field. If this property is present it
> + should be between 0 - 3. If this property isn't present we won't touch the
> + "overcurrent wait" field and we'll leave it to the BIOS/EC to deal with.
I take it this is the raw value to write to the register?
> +static int tps65090_fet_is_enabled(struct regulator_dev *rdev)
> +{
> + unsigned int control;
> + unsigned int expected = rdev->desc->enable_mask | BIT(CTRL_PG_BIT);
> + int ret;
> +
> + ret = regmap_read(rdev->regmap, rdev->desc->enable_reg, &control);
> + if (ret != 0)
> + return ret;
> +
> + return (control & expected) == expected;
> +}
No need to open code this, regulator_is_enabled_regmap() can check for
any value in a bitfield.
> +static int tps6090_try_enable_fet(struct regulator_dev *rdev)
Why is this called tps6090_try_enable_fet(), looks like a missing 5?
> + /*
> + * Try enabling multiple times until we succeed since sometimes the
> + * first try times out.
> + */
> + for (tries = 0; ; tries++) {
> + ret = tps6090_try_enable_fet(rdev);
> + if (!ret)
> + break;
> + if (ret != -ENOTRECOVERABLE || tries == MAX_FET_ENABLE_TRIES)
> + goto err;
Make this a do { } while so we don't have the exit condition missing in
the for loop please, it's doing the right thing but it's not as obvious
as it could be.
> + if (tries) {
> + dev_warn(&rdev->dev, "reg %#x enable ok after %d tries\n",
> + rdev->desc->enable_reg, tries);
> + }
No need for braces here, and I guess that ought to be retries rather
than tries (though that is pedantry).
> On the ARM Chromebook tps65090 has two masters: the AP (the main
> processor running linux) and the EC (the embedded controller). The AP
> is allowed to mess with FETs but the EC is in charge of charge control.
>
> The tps65090 interupt line is routed to both the AP and the EC, which
> can cause quite a headache. Having two people adjusting masks and
> acking interrupts is a recipe for disaster.
>
> In the shipping kernel we had a hack to have the AP pay attention to
> the IRQ but not to ack it. It also wasn't supposed to configure the
> IRQ in any way. That hack allowed us to detect when the device was
> charging without messing with the EC's state.
>
> The current tps65090 infrastructure makes the above difficult, and it
> was a bit of a hack to begin with. Rather than uglify the driver to
> support it, just extend the driver's existing notion of "no irq" to
> the charger. This makes the charger code poll every 2 seconds for AC
> detect, which is sufficient.
>
> Signed-off-by: Doug Anderson <[email protected]>
> ---
> drivers/mfd/tps65090.c | 14 ++++++--
> drivers/power/tps65090-charger.c | 76 +++++++++++++++++++++++++++++++---------
> 2 files changed, 70 insertions(+), 20 deletions(-)
For the MFD part:
Acked-by: Lee Jones <[email protected]>
Anton,
If you are okay with this patch I'd be happy to create an immutable
branch for you to pull from?
Doug,
What is the relationship (dependencies) between this and the other
patches in the set?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
> Nearly all of the registers in tps65090 combine control bits and
> status bits. Turn off caching of registers so that we can read status
> bits reliably.
>
> NOTE: the IRQnMASK and CG_CTRLn registers are the exception and could
> be cached. If we find that we spend a lot of time reading those we
> can turn on cache for just those registers.
>
> Signed-off-by: Doug Anderson <[email protected]>
> ---
> drivers/mfd/tps65090.c | 10 ----------
> 1 file changed, 10 deletions(-)
>
> diff --git a/drivers/mfd/tps65090.c b/drivers/mfd/tps65090.c
> index c3cddb4..4cfdd07 100644
> --- a/drivers/mfd/tps65090.c
> +++ b/drivers/mfd/tps65090.c
> @@ -149,21 +149,11 @@ static struct regmap_irq_chip tps65090_irq_chip = {
> .mask_invert = true,
> };
>
> -static bool is_volatile_reg(struct device *dev, unsigned int reg)
> -{
> - if ((reg == TPS65090_INT_STS) || (reg == TPS65090_INT_STS2))
> - return true;
> - else
> - return false;
> -}
> -
I don't know enough about Regmap internals to know what this actually
affects in real terms.
Mark,
Does this change seem sane to you?
> static const struct regmap_config tps65090_regmap_config = {
> .reg_bits = 8,
> .val_bits = 8,
> .max_register = TOTAL_NUM_REG,
> .num_reg_defaults_raw = TOTAL_NUM_REG,
> - .cache_type = REGCACHE_RBTREE,
> - .volatile_reg = is_volatile_reg,
> };
>
> #ifdef CONFIG_OF
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, Apr 16, 2014 at 10:59:22AM +0100, Lee Jones wrote:
> > NOTE: the IRQnMASK and CG_CTRLn registers are the exception and could
> > be cached. If we find that we spend a lot of time reading those we
> > can turn on cache for just those registers.
> > -static bool is_volatile_reg(struct device *dev, unsigned int reg)
> > -{
> > - if ((reg == TPS65090_INT_STS) || (reg == TPS65090_INT_STS2))
> > - return true;
> > - else
> > - return false;
> > -}
> > -
> I don't know enough about Regmap internals to know what this actually
> affects in real terms.
> Mark,
> Does this change seem sane to you?
It does what it says, it stops us caching stuff. It would seem better
to do what the changelog suggests above and keep caching the registers
that can be cached - especially the interrupt masks, they should get
read in the interrupt path and that tends to be a bit latency sensitive.
Lee
On Wed, Apr 16, 2014 at 2:52 AM, Lee Jones <[email protected]> wrote:
>> On the ARM Chromebook tps65090 has two masters: the AP (the main
>> processor running linux) and the EC (the embedded controller). The AP
>> is allowed to mess with FETs but the EC is in charge of charge control.
>>
>> The tps65090 interupt line is routed to both the AP and the EC, which
>> can cause quite a headache. Having two people adjusting masks and
>> acking interrupts is a recipe for disaster.
>>
>> In the shipping kernel we had a hack to have the AP pay attention to
>> the IRQ but not to ack it. It also wasn't supposed to configure the
>> IRQ in any way. That hack allowed us to detect when the device was
>> charging without messing with the EC's state.
>>
>> The current tps65090 infrastructure makes the above difficult, and it
>> was a bit of a hack to begin with. Rather than uglify the driver to
>> support it, just extend the driver's existing notion of "no irq" to
>> the charger. This makes the charger code poll every 2 seconds for AC
>> detect, which is sufficient.
>>
>> Signed-off-by: Doug Anderson <[email protected]>
>> ---
>> drivers/mfd/tps65090.c | 14 ++++++--
>> drivers/power/tps65090-charger.c | 76 +++++++++++++++++++++++++++++++---------
>> 2 files changed, 70 insertions(+), 20 deletions(-)
>
> For the MFD part:
> Acked-by: Lee Jones <[email protected]>
>
> Anton,
> If you are okay with this patch I'd be happy to create an immutable
> branch for you to pull from?
>
> Doug,
> What is the relationship (dependencies) between this and the other
> patches in the set?
This patch can be applied irrespective of other others in the series.
-Doug
> >> On the ARM Chromebook tps65090 has two masters: the AP (the main
> >> processor running linux) and the EC (the embedded controller). The AP
> >> is allowed to mess with FETs but the EC is in charge of charge control.
> >>
> >> The tps65090 interupt line is routed to both the AP and the EC, which
> >> can cause quite a headache. Having two people adjusting masks and
> >> acking interrupts is a recipe for disaster.
> >>
> >> In the shipping kernel we had a hack to have the AP pay attention to
> >> the IRQ but not to ack it. It also wasn't supposed to configure the
> >> IRQ in any way. That hack allowed us to detect when the device was
> >> charging without messing with the EC's state.
> >>
> >> The current tps65090 infrastructure makes the above difficult, and it
> >> was a bit of a hack to begin with. Rather than uglify the driver to
> >> support it, just extend the driver's existing notion of "no irq" to
> >> the charger. This makes the charger code poll every 2 seconds for AC
> >> detect, which is sufficient.
> >>
> >> Signed-off-by: Doug Anderson <[email protected]>
> >> ---
> >> drivers/mfd/tps65090.c | 14 ++++++--
> >> drivers/power/tps65090-charger.c | 76 +++++++++++++++++++++++++++++++---------
> >> 2 files changed, 70 insertions(+), 20 deletions(-)
> >
> > For the MFD part:
> > Acked-by: Lee Jones <[email protected]>
> >
> > Anton,
> > If you are okay with this patch I'd be happy to create an immutable
> > branch for you to pull from?
> >
> > Doug,
> > What is the relationship (dependencies) between this and the other
> > patches in the set?
>
> This patch can be applied irrespective of other others in the series.
What about the files in the patch? Could you make two separate patches
from this one patch and it would still compile okay? I'm _guessing_
the answer is yes?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Lee,
On Wed, Apr 16, 2014 at 9:26 AM, Lee Jones <[email protected]> wrote:
>> >> On the ARM Chromebook tps65090 has two masters: the AP (the main
>> >> processor running linux) and the EC (the embedded controller). The AP
>> >> is allowed to mess with FETs but the EC is in charge of charge control.
>> >>
>> >> The tps65090 interupt line is routed to both the AP and the EC, which
>> >> can cause quite a headache. Having two people adjusting masks and
>> >> acking interrupts is a recipe for disaster.
>> >>
>> >> In the shipping kernel we had a hack to have the AP pay attention to
>> >> the IRQ but not to ack it. It also wasn't supposed to configure the
>> >> IRQ in any way. That hack allowed us to detect when the device was
>> >> charging without messing with the EC's state.
>> >>
>> >> The current tps65090 infrastructure makes the above difficult, and it
>> >> was a bit of a hack to begin with. Rather than uglify the driver to
>> >> support it, just extend the driver's existing notion of "no irq" to
>> >> the charger. This makes the charger code poll every 2 seconds for AC
>> >> detect, which is sufficient.
>> >>
>> >> Signed-off-by: Doug Anderson <[email protected]>
>> >> ---
>> >> drivers/mfd/tps65090.c | 14 ++++++--
>> >> drivers/power/tps65090-charger.c | 76 +++++++++++++++++++++++++++++++---------
>> >> 2 files changed, 70 insertions(+), 20 deletions(-)
>> >
>> > For the MFD part:
>> > Acked-by: Lee Jones <[email protected]>
>> >
>> > Anton,
>> > If you are okay with this patch I'd be happy to create an immutable
>> > branch for you to pull from?
>> >
>> > Doug,
>> > What is the relationship (dependencies) between this and the other
>> > patches in the set?
>>
>> This patch can be applied irrespective of other others in the series.
>
> What about the files in the patch? Could you make two separate patches
> from this one patch and it would still compile okay? I'm _guessing_
> the answer is yes?
Yes, they'll compile and even boot on their own. I just tested it.
If I put only the MFD part in, then at boot I see:
tps65090-charger tps65090-charger: Unable to get charger irq = -6
...but otherwise the system functions.
If I put only the charger part in, then at boot:
tps65090-charger tps65090-charger: Unable to register irq 1 err -22
tps65090-charger: probe of tps65090-charger failed with error -22
...so you need both patches in order to make things function, but they
can be applied separately. I'll assume it will make your life easier
if I split this into two patches so I'll do that.
-Doug
On the ARM Chromebook tps65090 has two masters: the AP (the main
processor running linux) and the EC (the embedded controller). The AP
is allowed to mess with FETs but the EC is in charge of charge control.
The tps65090 interupt line is routed to both the AP and the EC, which
can cause quite a headache. Having two people adjusting masks and
acking interrupts is a recipe for disaster.
In the shipping kernel we had a hack to have the AP pay attention to
the IRQ but not to ack it. It also wasn't supposed to configure the
IRQ in any way. That hack allowed us to detect when the device was
charging without messing with the EC's state.
The current tps65090 infrastructure makes the above difficult, and it
was a bit of a hack to begin with. Rather than uglify the driver to
support it, just extend the driver's existing notion of "no irq" to
the charger. This makes the charger code poll every 2 seconds for AC
detect, which is sufficient.
For proper functioning, requires (mfd: tps65090: Don't tell child
devices we have an IRQ if we don't). If we don't have that patch
we'll simply fail to probe on devices without an interrupt (just like
we did before this patch).
Signed-off-by: Doug Anderson <[email protected]>
---
Changes in v2:
- Split noirq (polling mode) changes into MFD and charger
drivers/power/tps65090-charger.c | 76 +++++++++++++++++++++++++++++++---------
1 file changed, 59 insertions(+), 17 deletions(-)
diff --git a/drivers/power/tps65090-charger.c b/drivers/power/tps65090-charger.c
index 8fc9d6d..cc26c12 100644
--- a/drivers/power/tps65090-charger.c
+++ b/drivers/power/tps65090-charger.c
@@ -17,9 +17,11 @@
*/
#include <linux/delay.h>
#include <linux/err.h>
+#include <linux/freezer.h>
#include <linux/init.h>
#include <linux/interrupt.h>
#include <linux/kernel.h>
+#include <linux/kthread.h>
#include <linux/module.h>
#include <linux/of_device.h>
#include <linux/platform_device.h>
@@ -43,11 +45,15 @@
#define TPS65090_VACG BIT(1)
#define TPS65090_NOITERM BIT(5)
+#define POLL_INTERVAL (HZ * 2) /* Used when no irq */
+
struct tps65090_charger {
struct device *dev;
int ac_online;
int prev_ac_online;
int irq;
+ struct task_struct *poll_task;
+ bool passive_mode;
struct power_supply ac;
struct tps65090_platform_data *pdata;
};
@@ -60,6 +66,9 @@ static int tps65090_low_chrg_current(struct tps65090_charger *charger)
{
int ret;
+ if (charger->passive_mode)
+ return 0;
+
ret = tps65090_write(charger->dev->parent, TPS65090_REG_CG_CTRL5,
TPS65090_NOITERM);
if (ret < 0) {
@@ -75,6 +84,9 @@ static int tps65090_enable_charging(struct tps65090_charger *charger)
int ret;
uint8_t ctrl0 = 0;
+ if (charger->passive_mode)
+ return 0;
+
ret = tps65090_read(charger->dev->parent, TPS65090_REG_CG_CTRL0,
&ctrl0);
if (ret < 0) {
@@ -98,6 +110,9 @@ static int tps65090_config_charger(struct tps65090_charger *charger)
uint8_t intrmask = 0;
int ret;
+ if (charger->passive_mode)
+ return 0;
+
if (charger->pdata->enable_low_current_chrg) {
ret = tps65090_low_chrg_current(charger);
if (ret < 0) {
@@ -175,10 +190,14 @@ static irqreturn_t tps65090_charger_isr(int irq, void *dev_id)
}
/* Clear interrupts. */
- ret = tps65090_write(charger->dev->parent, TPS65090_REG_INTR_STS, 0x00);
- if (ret < 0) {
- dev_err(charger->dev, "%s(): Error in writing reg 0x%x\n",
+ if (!charger->passive_mode) {
+ ret = tps65090_write(charger->dev->parent,
+ TPS65090_REG_INTR_STS, 0x00);
+ if (ret < 0) {
+ dev_err(charger->dev,
+ "%s(): Error in writing reg 0x%x\n",
__func__, TPS65090_REG_INTR_STS);
+ }
}
if (charger->prev_ac_online != charger->ac_online)
@@ -209,6 +228,18 @@ static struct tps65090_platform_data *
}
+static int tps65090_charger_poll_task(void *data)
+{
+ set_freezable();
+
+ while (!kthread_should_stop()) {
+ schedule_timeout_interruptible(POLL_INTERVAL);
+ try_to_freeze();
+ tps65090_charger_isr(-1, data);
+ }
+ return 0;
+}
+
static int tps65090_charger_probe(struct platform_device *pdev)
{
struct tps65090_charger *cdata;
@@ -255,22 +286,10 @@ static int tps65090_charger_probe(struct platform_device *pdev)
}
irq = platform_get_irq(pdev, 0);
- if (irq <= 0) {
- dev_warn(&pdev->dev, "Unable to get charger irq = %d\n", irq);
- ret = irq;
- goto fail_unregister_supply;
- }
-
+ if (irq < 0)
+ irq = NO_IRQ;
cdata->irq = irq;
- ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
- tps65090_charger_isr, 0, "tps65090-charger", cdata);
- if (ret) {
- dev_err(cdata->dev, "Unable to register irq %d err %d\n", irq,
- ret);
- goto fail_unregister_supply;
- }
-
ret = tps65090_config_charger(cdata);
if (ret < 0) {
dev_err(&pdev->dev, "charger config failed, err %d\n", ret);
@@ -296,6 +315,27 @@ static int tps65090_charger_probe(struct platform_device *pdev)
power_supply_changed(&cdata->ac);
}
+ if (irq != NO_IRQ) {
+ ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+ tps65090_charger_isr, 0, "tps65090-charger", cdata);
+ if (ret) {
+ dev_err(cdata->dev,
+ "Unable to register irq %d err %d\n", irq,
+ ret);
+ goto fail_unregister_supply;
+ }
+ } else {
+ cdata->poll_task = kthread_run(tps65090_charger_poll_task,
+ cdata, "ktps65090charger");
+ cdata->passive_mode = true;
+ if (IS_ERR(cdata->poll_task)) {
+ ret = PTR_ERR(cdata->poll_task);
+ dev_err(cdata->dev,
+ "Unable to run kthread err %d\n", ret);
+ goto fail_unregister_supply;
+ }
+ }
+
return 0;
fail_unregister_supply:
@@ -308,6 +348,8 @@ static int tps65090_charger_remove(struct platform_device *pdev)
{
struct tps65090_charger *cdata = platform_get_drvdata(pdev);
+ if (cdata->irq == NO_IRQ)
+ kthread_stop(cdata->poll_task);
power_supply_unregister(&cdata->ac);
return 0;
--
1.9.1.423.g4596e3a
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 <[email protected]>
Signed-off-by: Simon Glass <[email protected]>
Signed-off-by: Michael Spang <[email protected]>
Signed-off-by: Sean Paul <[email protected]>
---
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(-)
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
+
+#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
Mark,
On Wed, Apr 16, 2014 at 3:13 AM, Mark Brown <[email protected]> wrote:
> On Wed, Apr 16, 2014 at 10:59:22AM +0100, Lee Jones wrote:
>
>> > NOTE: the IRQnMASK and CG_CTRLn registers are the exception and could
>> > be cached. If we find that we spend a lot of time reading those we
>> > can turn on cache for just those registers.
>
>> > -static bool is_volatile_reg(struct device *dev, unsigned int reg)
>> > -{
>> > - if ((reg == TPS65090_INT_STS) || (reg == TPS65090_INT_STS2))
>> > - return true;
>> > - else
>> > - return false;
>> > -}
>> > -
>
>> I don't know enough about Regmap internals to know what this actually
>> affects in real terms.
>
>> Mark,
>> Does this change seem sane to you?
>
> It does what it says, it stops us caching stuff. It would seem better
> to do what the changelog suggests above and keep caching the registers
> that can be cached - especially the interrupt masks, they should get
> read in the interrupt path and that tends to be a bit latency sensitive.
Done
The tps65090 regulator allows you to specify how long you want it to
wait before detecting an overcurrent condition. Allow specifying that
through the device tree (or through platform data).
Signed-off-by: Doug Anderson <[email protected]>
Signed-off-by: Simon Glass <[email protected]>
Signed-off-by: Michael Spang <[email protected]>
Signed-off-by: Sean Paul <[email protected]>
---
Changes in v2:
- Separated the overcurrent and retries changes into two patches.
- Now set overcurrent at probe time since it doesn't change.
.../devicetree/bindings/regulator/tps65090.txt | 4 ++
drivers/regulator/tps65090-regulator.c | 55 ++++++++++++++++++++++
include/linux/mfd/tps65090.h | 5 ++
3 files changed, 64 insertions(+)
diff --git a/Documentation/devicetree/bindings/regulator/tps65090.txt b/Documentation/devicetree/bindings/regulator/tps65090.txt
index 313a60b..34098023 100644
--- a/Documentation/devicetree/bindings/regulator/tps65090.txt
+++ b/Documentation/devicetree/bindings/regulator/tps65090.txt
@@ -21,6 +21,10 @@ Optional properties:
number should be provided. If it is externally controlled and no GPIO
entry then driver will just configure this rails as external control
and will not provide any enable/disable APIs.
+- ti,overcurrent-wait: This is applicable to FET registers, which have a
+ poorly defined "overcurrent wait" field. If this property is present it
+ should be between 0 - 3. If this property isn't present we won't touch the
+ "overcurrent wait" field and we'll leave it to the BIOS/EC to deal with.
Each regulator is defined using the standard binding for regulators.
diff --git a/drivers/regulator/tps65090-regulator.c b/drivers/regulator/tps65090-regulator.c
index 2e92ef6..ca13a1a 100644
--- a/drivers/regulator/tps65090-regulator.c
+++ b/drivers/regulator/tps65090-regulator.c
@@ -28,15 +28,57 @@
#include <linux/regulator/of_regulator.h>
#include <linux/mfd/tps65090.h>
+#define CTRL_WT_BIT 2 /* Regulator wait time 0 bit */
+
+#define MAX_OVERCURRENT_WAIT 3 /* Overcurrent wait must be <= this */
+
+/**
+ * struct tps65090_regulator - Per-regulator data for a tps65090 regulator
+ *
+ * @dev: Pointer to our device.
+ * @desc: The struct regulator_desc for the regulator.
+ * @rdev: The struct regulator_dev for the regulator.
+ * @overcurrent_wait_valid: True if overcurrent_wait is valid.
+ * @overcurrent_wait: For FETs, the value to put in the WTFET bitfield.
+ */
+
struct tps65090_regulator {
struct device *dev;
struct regulator_desc *desc;
struct regulator_dev *rdev;
+ bool overcurrent_wait_valid;
+ int overcurrent_wait;
};
static struct regulator_ops tps65090_ext_control_ops = {
};
+/**
+ * tps65090_reg_set_overcurrent_wait - Setup overcurrent wait
+ *
+ * This will set the overcurrent wait time based on what's in the regulator
+ * info.
+ *
+ * @ri: Overall regulator data
+ * @rdev: Regulator device
+ * @return 0 if no error, non-zero if there was an error writing the register.
+ */
+static int tps65090_reg_set_overcurrent_wait(struct tps65090_regulator *ri,
+ struct regulator_dev *rdev)
+{
+ int ret;
+
+ ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
+ MAX_OVERCURRENT_WAIT << CTRL_WT_BIT,
+ ri->overcurrent_wait << CTRL_WT_BIT);
+ if (ret) {
+ dev_err(&rdev->dev, "Error updating overcurrent wait %#x\n",
+ rdev->desc->enable_reg);
+ }
+
+ return ret;
+}
+
static struct regulator_ops tps65090_reg_contol_ops = {
.enable = regulator_enable_regmap,
.disable = regulator_disable_regmap,
@@ -209,6 +251,11 @@ static struct tps65090_platform_data *tps65090_parse_dt_reg_data(
rpdata->gpio = of_get_named_gpio(np,
"dcdc-ext-control-gpios", 0);
+ if (of_property_read_u32(tps65090_matches[idx].of_node,
+ "ti,overcurrent-wait",
+ &rpdata->overcurrent_wait) == 0)
+ rpdata->overcurrent_wait_valid = true;
+
tps65090_pdata->reg_pdata[idx] = rpdata;
}
return tps65090_pdata;
@@ -258,6 +305,8 @@ static int tps65090_regulator_probe(struct platform_device *pdev)
ri = &pmic[num];
ri->dev = &pdev->dev;
ri->desc = &tps65090_regulator_desc[num];
+ ri->overcurrent_wait_valid = tps_pdata->overcurrent_wait_valid;
+ ri->overcurrent_wait = tps_pdata->overcurrent_wait;
/*
* TPS5090 DCDC support the control from external digital input.
@@ -299,6 +348,12 @@ static int tps65090_regulator_probe(struct platform_device *pdev)
}
ri->rdev = rdev;
+ if (ri->overcurrent_wait_valid) {
+ ret = tps65090_reg_set_overcurrent_wait(ri, rdev);
+ if (ret < 0)
+ return ret;
+ }
+
/* Enable external control if it is require */
if (tps_pdata && is_dcdc(num) && tps_pdata->reg_init_data &&
tps_pdata->enable_ext_control) {
diff --git a/include/linux/mfd/tps65090.h b/include/linux/mfd/tps65090.h
index 45f0f9d..0bf2708 100644
--- a/include/linux/mfd/tps65090.h
+++ b/include/linux/mfd/tps65090.h
@@ -92,11 +92,16 @@ struct tps65090 {
* DCDC1, DCDC2 and DCDC3.
* @gpio: Gpio number if external control is enabled and controlled through
* gpio.
+ * @overcurrent_wait_valid: True if the overcurrent_wait should be applied.
+ * @overcurrent_wait: Value to set as the overcurrent wait time. This is the
+ * actual bitfield value, not a time in ms (valid value are 0 - 3).
*/
struct tps65090_regulator_plat_data {
struct regulator_init_data *reg_init_data;
bool enable_ext_control;
int gpio;
+ bool overcurrent_wait_valid;
+ int overcurrent_wait;
};
struct tps65090_platform_data {
--
1.9.1.423.g4596e3a
Mark,
On Tue, Apr 15, 2014 at 3:52 PM, Mark Brown <[email protected]> wrote:
> On Tue, Apr 15, 2014 at 01:14:36PM -0700, Doug Anderson wrote:
>
>> Mitigate the problem by:
>> * Allow setting the overcurrent wait time so devices with this problem
>> can set it to the max.
>> * Add retry logic on enables of FETs.
>
> This is two changes, should really be two patches.
OK, sure.
>> +- ti,overcurrent-wait: This is applicable to FET registers, which have a
>> + poorly defined "overcurrent wait" field. If this property is present it
>> + should be between 0 - 3. If this property isn't present we won't touch the
>> + "overcurrent wait" field and we'll leave it to the BIOS/EC to deal with.
>
> I take it this is the raw value to write to the register?
Yes.
>> +static int tps65090_fet_is_enabled(struct regulator_dev *rdev)
>> +{
>> + unsigned int control;
>> + unsigned int expected = rdev->desc->enable_mask | BIT(CTRL_PG_BIT);
>> + int ret;
>> +
>> + ret = regmap_read(rdev->regmap, rdev->desc->enable_reg, &control);
>> + if (ret != 0)
>> + return ret;
>> +
>> + return (control & expected) == expected;
>> +}
>
> No need to open code this, regulator_is_enabled_regmap() can check for
> any value in a bitfield.
The overall problem was that we were checking a different bit than we
were setting. We set "enabled" to turn things on and then we check
"power good".
I can avoid the open coding by doing something that's slightly a hack.
I'll put that in V2 and you can tell me if you like it better. I'll
set "enable_mask" and "enable_val" to include both bits. The "power
good" is read only so it won't hurt to set it. ...and it doesn't hurt
to check the enabled bit in addition to the power good bit.
>> +static int tps6090_try_enable_fet(struct regulator_dev *rdev)
>
> Why is this called tps6090_try_enable_fet(), looks like a missing 5?
typo. fixed.
>
>> + /*
>> + * Try enabling multiple times until we succeed since sometimes the
>> + * first try times out.
>> + */
>> + for (tries = 0; ; tries++) {
>> + ret = tps6090_try_enable_fet(rdev);
>> + if (!ret)
>> + break;
>> + if (ret != -ENOTRECOVERABLE || tries == MAX_FET_ENABLE_TRIES)
>> + goto err;
>
> Make this a do { } while so we don't have the exit condition missing in
> the for loop please, it's doing the right thing but it's not as obvious
> as it could be.
It's not quite a "do { } while" since the break is in the middle, but
happy to change to a "while (true)". Hope that's OK.
>
>> + if (tries) {
>> + dev_warn(&rdev->dev, "reg %#x enable ok after %d tries\n",
>> + rdev->desc->enable_reg, tries);
>> + }
>
> No need for braces here, and I guess that ought to be retries rather
> than tries (though that is pedantry).
LOL. I've been yelled at for the opposite. ;) There's at least
someone out there who thinks that we should have braces if you've got
a single statement like this that wraps to two lines, but I can't
remember who.
In any case, fixed.
-Doug
These five patches bring tps65090 up to speed with what's currently
in the Chromium OS kernel 3.8 tree and running on the Samsung ARM
Chromebook. Changes were tested atop the current linux tree
(v3.15-rc1). FET retries were tested on a machine with a known flaky
tps65090. Since display isn't working on pure upstream, I augmented
the code to turn FET1 (vcd_led) on/off 500 times at bootup. When
testing I included <https://patchwork.kernel.org/patch/3980731/> to
make sure tps65090 was in exynos5250-snow's device tree.
Dependencies:
- Patch #1 (mfd no irq) has no dependencies, though patch #2 won't
work without it.
- Patch #2 (charger polling) can be applied without patch #1 but won't
actually make charger polling work without it.
- Patch #3 (caching) can be applied before retries patch but not
after.
- Patch #4 (overcurrent wait time) can be applied before retries patch
but not after (just due to merge conflicts, no other reason).
- Patch #5 (retries) absolutely requires patch #3 (caching).
Changes in v2:
- Split noirq (polling mode) changes into MFD and charger
- Leave cache on for the registers that can be cached.
- Move register offsets to mfd header file.
- Separated the overcurrent and retries changes into two patches.
- Now set overcurrent at probe time since it doesn't change.
- 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.
Doug Anderson (5):
mfd: tps65090: Don't tell child devices we have an IRQ if we don't
charger: tps65090: Allow charger module to be used when no irq
mfd: tps65090: Stop caching most registers
regulator: tps65090: Allow setting the overcurrent wait time
regulator: tps65090: Make FETs more reliable by adding retries
.../devicetree/bindings/regulator/tps65090.txt | 4 +
drivers/mfd/tps65090.c | 41 ++--
drivers/power/tps65090-charger.c | 87 ++++++---
drivers/regulator/tps65090-regulator.c | 208 +++++++++++++++++++--
include/linux/mfd/tps65090.h | 19 ++
5 files changed, 300 insertions(+), 59 deletions(-)
--
1.9.1.423.g4596e3a
Nearly all of the registers in tps65090 combine control bits and
status bits. Turn off caching of all registers except the select few
that can be cached.
In order to avoid adding more duplicate #defines, we also move some
register offset definitions to the mfd driver (and resolve
inconsistent names).
Signed-off-by: Doug Anderson <[email protected]>
---
Changes in v2:
- Leave cache on for the registers that can be cached.
- Move register offsets to mfd header file.
drivers/mfd/tps65090.c | 27 ++++++++++++++-------------
drivers/power/tps65090-charger.c | 11 -----------
include/linux/mfd/tps65090.h | 14 ++++++++++++++
3 files changed, 28 insertions(+), 24 deletions(-)
diff --git a/drivers/mfd/tps65090.c b/drivers/mfd/tps65090.c
index c3cddb4..1c3e6e2 100644
--- a/drivers/mfd/tps65090.c
+++ b/drivers/mfd/tps65090.c
@@ -32,14 +32,6 @@
#define NUM_INT_REG 2
#define TOTAL_NUM_REG 0x18
-/* interrupt status registers */
-#define TPS65090_INT_STS 0x0
-#define TPS65090_INT_STS2 0x1
-
-/* interrupt mask registers */
-#define TPS65090_INT_MSK 0x2
-#define TPS65090_INT_MSK2 0x3
-
#define TPS65090_INT1_MASK_VAC_STATUS_CHANGE 1
#define TPS65090_INT1_MASK_VSYS_STATUS_CHANGE 2
#define TPS65090_INT1_MASK_BAT_STATUS_CHANGE 3
@@ -144,17 +136,26 @@ static struct regmap_irq_chip tps65090_irq_chip = {
.irqs = tps65090_irqs,
.num_irqs = ARRAY_SIZE(tps65090_irqs),
.num_regs = NUM_INT_REG,
- .status_base = TPS65090_INT_STS,
- .mask_base = TPS65090_INT_MSK,
+ .status_base = TPS65090_REG_INTR_STS,
+ .mask_base = TPS65090_REG_INTR_MASK,
.mask_invert = true,
};
static bool is_volatile_reg(struct device *dev, unsigned int reg)
{
- if ((reg == TPS65090_INT_STS) || (reg == TPS65090_INT_STS2))
- return true;
- else
+ /* Nearly all registers have status bits mixed in, except a few */
+ switch (reg) {
+ case TPS65090_REG_INTR_MASK:
+ case TPS65090_REG_INTR_MASK2:
+ case TPS65090_REG_CG_CTRL0:
+ case TPS65090_REG_CG_CTRL1:
+ case TPS65090_REG_CG_CTRL2:
+ case TPS65090_REG_CG_CTRL3:
+ case TPS65090_REG_CG_CTRL4:
+ case TPS65090_REG_CG_CTRL5:
return false;
+ }
+ return true;
}
static const struct regmap_config tps65090_regmap_config = {
diff --git a/drivers/power/tps65090-charger.c b/drivers/power/tps65090-charger.c
index cc26c12..31a3ba4 100644
--- a/drivers/power/tps65090-charger.c
+++ b/drivers/power/tps65090-charger.c
@@ -30,17 +30,6 @@
#include <linux/mfd/tps65090.h>
-#define TPS65090_REG_INTR_STS 0x00
-#define TPS65090_REG_INTR_MASK 0x02
-#define TPS65090_REG_CG_CTRL0 0x04
-#define TPS65090_REG_CG_CTRL1 0x05
-#define TPS65090_REG_CG_CTRL2 0x06
-#define TPS65090_REG_CG_CTRL3 0x07
-#define TPS65090_REG_CG_CTRL4 0x08
-#define TPS65090_REG_CG_CTRL5 0x09
-#define TPS65090_REG_CG_STATUS1 0x0a
-#define TPS65090_REG_CG_STATUS2 0x0b
-
#define TPS65090_CHARGER_ENABLE BIT(0)
#define TPS65090_VACG BIT(1)
#define TPS65090_NOITERM BIT(5)
diff --git a/include/linux/mfd/tps65090.h b/include/linux/mfd/tps65090.h
index 3f43069..45f0f9d 100644
--- a/include/linux/mfd/tps65090.h
+++ b/include/linux/mfd/tps65090.h
@@ -64,6 +64,20 @@ enum {
TPS65090_REGULATOR_MAX,
};
+/* Register addresses */
+#define TPS65090_REG_INTR_STS 0x00
+#define TPS65090_REG_INTR_STS2 0x01
+#define TPS65090_REG_INTR_MASK 0x02
+#define TPS65090_REG_INTR_MASK2 0x03
+#define TPS65090_REG_CG_CTRL0 0x04
+#define TPS65090_REG_CG_CTRL1 0x05
+#define TPS65090_REG_CG_CTRL2 0x06
+#define TPS65090_REG_CG_CTRL3 0x07
+#define TPS65090_REG_CG_CTRL4 0x08
+#define TPS65090_REG_CG_CTRL5 0x09
+#define TPS65090_REG_CG_STATUS1 0x0a
+#define TPS65090_REG_CG_STATUS2 0x0b
+
struct tps65090 {
struct device *dev;
struct regmap *rmap;
--
1.9.1.423.g4596e3a
If we weren't given an interrupt we shouldn't tell child devices (like
the tps65090 charger) that they have an interrupt. This is needed so
that we can support polling mode in the tps65090 charger driver.
See also (charger: tps65090: Allow charger module to be used when no
irq).
Signed-off-by: Doug Anderson <[email protected]>
Acked-by: Lee Jones <[email protected]>
---
Changes in v2:
- Split noirq (polling mode) changes into MFD and charger
drivers/mfd/tps65090.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/mfd/tps65090.c b/drivers/mfd/tps65090.c
index ba1a25d..c3cddb4 100644
--- a/drivers/mfd/tps65090.c
+++ b/drivers/mfd/tps65090.c
@@ -64,11 +64,16 @@ static struct resource charger_resources[] = {
}
};
-static const struct mfd_cell tps65090s[] = {
- {
+enum tps65090_cells {
+ PMIC = 0,
+ CHARGER = 1,
+};
+
+static struct mfd_cell tps65090s[] = {
+ [PMIC] = {
.name = "tps65090-pmic",
},
- {
+ [CHARGER] = {
.name = "tps65090-charger",
.num_resources = ARRAY_SIZE(charger_resources),
.resources = &charger_resources[0],
@@ -211,6 +216,9 @@ static int tps65090_i2c_probe(struct i2c_client *client,
"IRQ init failed with err: %d\n", ret);
return ret;
}
+ } else {
+ /* Don't tell children they have an IRQ that'll never fire */
+ tps65090s[CHARGER].num_resources = 0;
}
ret = mfd_add_devices(tps65090->dev, -1, tps65090s,
--
1.9.1.423.g4596e3a
> >> >> On the ARM Chromebook tps65090 has two masters: the AP (the main
> >> >> processor running linux) and the EC (the embedded controller). The AP
> >> >> is allowed to mess with FETs but the EC is in charge of charge control.
> >> >>
> >> >> The tps65090 interupt line is routed to both the AP and the EC, which
> >> >> can cause quite a headache. Having two people adjusting masks and
> >> >> acking interrupts is a recipe for disaster.
> >> >>
> >> >> In the shipping kernel we had a hack to have the AP pay attention to
> >> >> the IRQ but not to ack it. It also wasn't supposed to configure the
> >> >> IRQ in any way. That hack allowed us to detect when the device was
> >> >> charging without messing with the EC's state.
> >> >>
> >> >> The current tps65090 infrastructure makes the above difficult, and it
> >> >> was a bit of a hack to begin with. Rather than uglify the driver to
> >> >> support it, just extend the driver's existing notion of "no irq" to
> >> >> the charger. This makes the charger code poll every 2 seconds for AC
> >> >> detect, which is sufficient.
> >> >>
> >> >> Signed-off-by: Doug Anderson <[email protected]>
> >> >> ---
> >> >> drivers/mfd/tps65090.c | 14 ++++++--
> >> >> drivers/power/tps65090-charger.c | 76 +++++++++++++++++++++++++++++++---------
> >> >> 2 files changed, 70 insertions(+), 20 deletions(-)
> >> >
> >> > For the MFD part:
> >> > Acked-by: Lee Jones <[email protected]>
> >> >
> >> > Anton,
> >> > If you are okay with this patch I'd be happy to create an immutable
> >> > branch for you to pull from?
> >> >
> >> > Doug,
> >> > What is the relationship (dependencies) between this and the other
> >> > patches in the set?
> >>
> >> This patch can be applied irrespective of other others in the series.
> >
> > What about the files in the patch? Could you make two separate patches
> > from this one patch and it would still compile okay? I'm _guessing_
> > the answer is yes?
>
> Yes, they'll compile and even boot on their own. I just tested it.
>
> If I put only the MFD part in, then at boot I see:
> tps65090-charger tps65090-charger: Unable to get charger irq = -6
> ...but otherwise the system functions.
>
> If I put only the charger part in, then at boot:
> tps65090-charger tps65090-charger: Unable to register irq 1 err -22
> tps65090-charger: probe of tps65090-charger failed with error -22
>
> ...so you need both patches in order to make things function, but they
> can be applied separately. I'll assume it will make your life easier
> if I split this into two patches so I'll do that.
Yes please.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On 04/16/2014 11:25 AM, Doug Anderson wrote:
> diff --git a/drivers/regulator/tps65090-regulator.c b/drivers/regulator/tps65090-regulator.c
> index 2e92ef6..ca13a1a 100644
> --- a/drivers/regulator/tps65090-regulator.c
> +++ b/drivers/regulator/tps65090-regulator.c
> @@ -28,15 +28,57 @@
> +/**
> + * tps65090_reg_set_overcurrent_wait - Setup overcurrent wait
> + *
> + * This will set the overcurrent wait time based on what's in the regulator
> + * info.
> + *
> + * @ri: Overall regulator data
> + * @rdev: Regulator device
> + * @return 0 if no error, non-zero if there was an error writing the register.
kernel-doc notation here should be:
* Return: 0 if no error, non-zero if there was an error writing the register.
> + */
> +static int tps65090_reg_set_overcurrent_wait(struct tps65090_regulator *ri,
> + struct regulator_dev *rdev)
> +{
--
~Randy
Hi Doug, (take 2)
On 16 April 2014 12:25, Doug Anderson <[email protected]> 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 <[email protected]>
> Signed-off-by: Simon Glass <[email protected]>
> Signed-off-by: Michael Spang <[email protected]>
> Signed-off-by: Sean Paul <[email protected]>
> ---
> 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 <[email protected]>
(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
On Wed, Apr 16, 2014 at 11:25:24AM -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
Please don't send new patches as replies in the middle of threads, it
makes it confusing trying to work out which versions of things should be
applied.
Simon,
On Wed, Apr 16, 2014 at 1:50 PM, Simon Glass <[email protected]> wrote:
>> +#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?
That's actually a squash of a recent patch. See
<https://chromium-review.googlesource.com/189239>. I've specifically
seen at least one case on my device where it needed 888 retries at
bootup!
...on my really old Chromebook, it seems to get into a bad state if it
sits on my desk for a long time. After I use it a bit it rarely needs
more than 10 retries.
-Doug
Mark,
On Wed, Apr 16, 2014 at 1:51 PM, Mark Brown <[email protected]> wrote:
> On Wed, Apr 16, 2014 at 11:25:24AM -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
>
> Please don't send new patches as replies in the middle of threads, it
> makes it confusing trying to work out which versions of things should be
> applied.
I'm a little confused about what I did wrong. Can you give more details?
* V1 had 3 patches plus a cover letter.
* I was asked to split two patches, so V2 has 5 patches plus a cover letter.
* My v2 series was all "in reply to" the v1 cover letter, which I
thought was best practice.
* All of my v2 patches were marked with v2 and included changes
between v1 and v2.
* Everyone was CCed on the cover letter. Only appropriate people were
CCed on the individual patches (as per get_maintainer, automated by
patman).
* All patches were resent at v2.
If I had to answer your question, I'd say that you should now
completely ignore v1 and look at v2.
-Doug
On Wed, Apr 16, 2014 at 02:34:47PM -0700, Doug Anderson wrote:
> On Wed, Apr 16, 2014 at 1:51 PM, Mark Brown <[email protected]> wrote:
> > Please don't send new patches as replies in the middle of threads, it
> > makes it confusing trying to work out which versions of things should be
> > applied.
> I'm a little confused about what I did wrong. Can you give more details?
I'm seeing a reply which looks like it was sent as a followup to Randy's
comment, although now I look at everything together I think that's due
to you sending your new thread in reply to the old thread (that can also
be a problem due to threading either burying the new mail or putting
things in odd places) and my mailer trying to tie the one mail from your
first series that I'd not deleted into the thread.
Mark,
On Wed, Apr 16, 2014 at 2:54 PM, Mark Brown <[email protected]> wrote:
> On Wed, Apr 16, 2014 at 02:34:47PM -0700, Doug Anderson wrote:
>> On Wed, Apr 16, 2014 at 1:51 PM, Mark Brown <[email protected]> wrote:
>
>> > Please don't send new patches as replies in the middle of threads, it
>> > makes it confusing trying to work out which versions of things should be
>> > applied.
>
>> I'm a little confused about what I did wrong. Can you give more details?
>
> I'm seeing a reply which looks like it was sent as a followup to Randy's
> comment, although now I look at everything together I think that's due
> to you sending your new thread in reply to the old thread (that can also
> be a problem due to threading either burying the new mail or putting
> things in odd places) and my mailer trying to tie the one mail from your
> first series that I'd not deleted into the thread.
OK, I'm about to send my v3 of the thread taking Randy's comments
about kernel-doc into account. I'll explicitly not mark it as
"in-reply-to" the previous thread and hope that solves the problems
you were seeing.
-Doug
Randy,
On Wed, Apr 16, 2014 at 1:33 PM, Randy Dunlap <[email protected]> wrote:
> On 04/16/2014 11:25 AM, Doug Anderson wrote:
>> diff --git a/drivers/regulator/tps65090-regulator.c b/drivers/regulator/tps65090-regulator.c
>> index 2e92ef6..ca13a1a 100644
>> --- a/drivers/regulator/tps65090-regulator.c
>> +++ b/drivers/regulator/tps65090-regulator.c
>> @@ -28,15 +28,57 @@
>
>> +/**
>> + * tps65090_reg_set_overcurrent_wait - Setup overcurrent wait
>> + *
>> + * This will set the overcurrent wait time based on what's in the regulator
>> + * info.
>> + *
>> + * @ri: Overall regulator data
>> + * @rdev: Regulator device
>> + * @return 0 if no error, non-zero if there was an error writing the register.
>
> kernel-doc notation here should be:
>
> * Return: 0 if no error, non-zero if there was an error writing the register.
Done in v3.
Hi Doug,
On 16 April 2014 15:25, Doug Anderson <[email protected]> wrote:
> Simon,
>
> On Wed, Apr 16, 2014 at 1:50 PM, Simon Glass <[email protected]> wrote:
>>> +#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?
>
> That's actually a squash of a recent patch. See
> <https://chromium-review.googlesource.com/189239>. I've specifically
> seen at least one case on my device where it needed 888 retries at
> bootup!
>
> ...on my really old Chromebook, it seems to get into a bad state if it
> sits on my desk for a long time. After I use it a bit it rarely needs
> more than 10 retries.
Try to be kinder to your hardware?
Anyway, fair enough, if you've seen 888 then we need to deal with that case.
Regards,
Simon