Re: [PATCH v4 5/5] regulator: qcom: labibb: Add SC interrupt handling
On Wed, Jun 17, 2020 at 05:36:43PM +0530, Sumit Semwal wrote:
> On Tue, 2 Jun 2020 at 17:52, Mark Brown wrote:
> > On Tue, Jun 02, 2020 at 03:39:24PM +0530, Sumit Semwal wrote:
> > > +
> > > + ret = regulator_enable_regmap(rdev);
> > > + if (ret >= 0)
> > > + reg->enabled = true;
> > Can we not read the register we just wrote to here?
> As I mentioned in the other patch, it seems there is a (noticeable)
> delay in getting the value to reflect in this register for IBB.
This sounds like it may not actually have finished enabling fully?
> Also, from the notes from the downstream driver (also copied below),
> it seems like during short circuit there is another protection system
> that can cause the registers to be cleared, hence the need to track
> the current state in software.
If the regulator has been disabled underneath us in a way that means it
won't come back the driver should be reflecting that in the status it
reports.
> > > + * Check if the regulator is enabled in the driver but
> > > + * disabled in hardware, this means a SC fault had happened
> > > + * and SCP handling is completed by PBS.
> > > + */
> > > + if (!in_sc_err) {
> > > +
> > > + reg = labibb_reg->base + REG_LABIBB_ENABLE_CTL;
> > > +
> > > + ret = regmap_read_poll_timeout(labibb_reg->regmap,
> > > + reg, val,
> > > + !(val & LABIBB_CONTROL_ENABLE),
> > > + POLLING_SCP_DONE_INTERVAL_US,
> > > + POLLING_SCP_TIMEOUT);
> > Why do we need a timeout here?
> IMHO, This seems to be the time required by the PBS to actually
> disable the regulator? If the PBS is not able to disable the
> regulator, then it points to a more serious problem?
> I'm sorry, that's just my understanding based on the downstream driver
> :/ - not much input is available from the QC teams about it.
So it might generate an interrupt but then take a long time to take the
actions associated with the interrupt that allow us to tell what the
interrupt was about? That doesn't seem great. Do you know if this code
has ever been exercised, the error handling code appears unusually
involved here? Normally errors don't routinely occur in production.
> > > + NULL);
> > > + regulator_unlock(labibb_reg->rdev);
> > > + }
> > > + return IRQ_HANDLED;
> > This returns IRQ_HANDLED even if we didn't detect an interrupt source...
> > Especially given the need to check to see if the regulator was turned
> > off by the hardware it seems like there must be some false positives.
> Right - I'm not sure what else can I do here.
Only return IRQ_HANDLED if we actually managed to figure out an error to
report?
signature.asc
Description: PGP signature
Re: [PATCH v4 5/5] regulator: qcom: labibb: Add SC interrupt handling
Hi Mark,
On Tue, 2 Jun 2020 at 17:52, Mark Brown wrote:
>
> On Tue, Jun 02, 2020 at 03:39:24PM +0530, Sumit Semwal wrote:
>
> > static int qcom_labibb_regulator_enable(struct regulator_dev *rdev)
> > {
> > - return regulator_enable_regmap(rdev);
> > + int ret;
> > + struct labibb_regulator *reg = rdev_get_drvdata(rdev);
> > +
> > + ret = regulator_enable_regmap(rdev);
> > + if (ret >= 0)
> > + reg->enabled = true;
>
> Can we not read the register we just wrote to here?
As I mentioned in the other patch, it seems there is a (noticeable)
delay in getting the value to reflect in this register for IBB.
Also, from the notes from the downstream driver (also copied below),
it seems like during short circuit there is another protection system
that can cause the registers to be cleared, hence the need to track
the current state in software.
>
> > + /*
> > + * The SC(short circuit) fault would trigger PBS(Portable Batch
> > + * System) to disable regulators for protection. This would
> > + * cause the SC_DETECT status being cleared so that it's not
> > + * able to get the SC fault status.
> > + * Check if the regulator is enabled in the driver but
> > + * disabled in hardware, this means a SC fault had happened
> > + * and SCP handling is completed by PBS.
> > + */
> > + if (!in_sc_err) {
> > +
> > + reg = labibb_reg->base + REG_LABIBB_ENABLE_CTL;
> > +
> > + ret = regmap_read_poll_timeout(labibb_reg->regmap,
> > + reg, val,
> > + !(val & LABIBB_CONTROL_ENABLE),
> > + POLLING_SCP_DONE_INTERVAL_US,
> > + POLLING_SCP_TIMEOUT);
>
> Why do we need a timeout here?
IMHO, This seems to be the time required by the PBS to actually
disable the regulator? If the PBS is not able to disable the
regulator, then it points to a more serious problem?
I'm sorry, that's just my understanding based on the downstream driver
:/ - not much input is available from the QC teams about it.
>
> > + NULL);
> > + regulator_unlock(labibb_reg->rdev);
> > + }
> > + return IRQ_HANDLED;
>
> This returns IRQ_HANDLED even if we didn't detect an interrupt source...
> Especially given the need to check to see if the regulator was turned
> off by the hardware it seems like there must be some false positives.
Right - I'm not sure what else can I do here.
>
> > + } else {
> > + ret = devm_request_threaded_irq(reg->dev,
> > + sc_irq,
> > + NULL, labibb_sc_err_handler,
> > + IRQF_ONESHOT,
> > + "sc-err", reg);
>
> This looks like we're requesting the interrupt before we register the
> regulator which means the interrupt might fire without the regulator
> being there. The order of registration should be reversed.
Agreed, and will update in the next version.
Best,
Sumit.
Re: [PATCH v4 5/5] regulator: qcom: labibb: Add SC interrupt handling
On Tue, Jun 02, 2020 at 03:39:24PM +0530, Sumit Semwal wrote:
> static int qcom_labibb_regulator_enable(struct regulator_dev *rdev)
> {
> - return regulator_enable_regmap(rdev);
> + int ret;
> + struct labibb_regulator *reg = rdev_get_drvdata(rdev);
> +
> + ret = regulator_enable_regmap(rdev);
> + if (ret >= 0)
> + reg->enabled = true;
Can we not read the register we just wrote to here?
> + /*
> + * The SC(short circuit) fault would trigger PBS(Portable Batch
> + * System) to disable regulators for protection. This would
> + * cause the SC_DETECT status being cleared so that it's not
> + * able to get the SC fault status.
> + * Check if the regulator is enabled in the driver but
> + * disabled in hardware, this means a SC fault had happened
> + * and SCP handling is completed by PBS.
> + */
> + if (!in_sc_err) {
> +
> + reg = labibb_reg->base + REG_LABIBB_ENABLE_CTL;
> +
> + ret = regmap_read_poll_timeout(labibb_reg->regmap,
> + reg, val,
> + !(val & LABIBB_CONTROL_ENABLE),
> + POLLING_SCP_DONE_INTERVAL_US,
> + POLLING_SCP_TIMEOUT);
Why do we need a timeout here?
> + NULL);
> + regulator_unlock(labibb_reg->rdev);
> + }
> + return IRQ_HANDLED;
This returns IRQ_HANDLED even if we didn't detect an interrupt source...
Especially given the need to check to see if the regulator was turned
off by the hardware it seems like there must be some false positives.
> + } else {
> + ret = devm_request_threaded_irq(reg->dev,
> + sc_irq,
> + NULL, labibb_sc_err_handler,
> + IRQF_ONESHOT,
> + "sc-err", reg);
This looks like we're requesting the interrupt before we register the
regulator which means the interrupt might fire without the regulator
being there. The order of registration should be reversed.
signature.asc
Description: PGP signature
[PATCH v4 5/5] regulator: qcom: labibb: Add SC interrupt handling
From: Nisha Kumari
Add Short circuit interrupt handling and recovery for the lab and ibb
regulators on qcom platforms.
The client panel drivers need to register for REGULATOR_EVENT_OVER_CURRENT
notification which will be triggered on short circuit. They should
try to enable the regulator once, and if it doesn't get enabled,
handle shutting down the panel accordingly.
Signed-off-by: Nisha Kumari
Signed-off-by: Sumit Semwal
[sumits: updated to rework to use regmap_read_poll_timeout, handle it per
regulator, add REGULATOR_EVENT_OVER_CURRENT handling and
notification to clients and other cleanup]
--
v2: sumits: reworked handling to user regmap_read_poll_timeout, and handle it
per-regulator instead of clearing both lab and ibb errors on either irq
triggering. Also added REGULATOR_EVENT_OVER_CURRENT handling and
notification to clients.
v3: sumits: updated as per review comments of v2: removed spurious check for
irq in handler and some unused variables; inlined some of the code,
omitted IRQF_TRIGGER_RISING as it's coming from DT.
v4: sumits: updated 'int vreg_enabled' to 'boot enabled', made sc_irq a local
var
and other review comments from v3.
---
drivers/regulator/qcom-labibb-regulator.c | 102 +-
1 file changed, 99 insertions(+), 3 deletions(-)
diff --git a/drivers/regulator/qcom-labibb-regulator.c
b/drivers/regulator/qcom-labibb-regulator.c
index 33b764ac69d1..bca0308b26dd 100644
--- a/drivers/regulator/qcom-labibb-regulator.c
+++ b/drivers/regulator/qcom-labibb-regulator.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-only
// Copyright (c) 2020, The Linux Foundation. All rights reserved.
+#include
#include
#include
#include
@@ -18,6 +19,7 @@
#define REG_LABIBB_ENABLE_CTL 0x46
#define LABIBB_STATUS1_VREG_OK_BIT BIT(7)
#define LABIBB_CONTROL_ENABLE BIT(7)
+#define LABIBB_STATUS1_SC_DETECT_BIT BIT(6)
#define LAB_ENABLE_CTL_MASKBIT(7)
#define IBB_ENABLE_CTL_MASK(BIT(7) | BIT(6))
@@ -27,12 +29,16 @@
#define IBB_ENABLE_TIME(LABIBB_OFF_ON_DELAY * 10)
#define LABIBB_POLL_ENABLED_TIME 1000
+#define POLLING_SCP_DONE_INTERVAL_US 5000
+#define POLLING_SCP_TIMEOUT16000
+
struct labibb_regulator {
struct regulator_desc desc;
struct device *dev;
struct regmap *regmap;
struct regulator_dev*rdev;
u16 base;
+ boolenabled;
u8 type;
};
@@ -59,12 +65,26 @@ static int qcom_labibb_regulator_is_enabled(struct
regulator_dev *rdev)
static int qcom_labibb_regulator_enable(struct regulator_dev *rdev)
{
- return regulator_enable_regmap(rdev);
+ int ret;
+ struct labibb_regulator *reg = rdev_get_drvdata(rdev);
+
+ ret = regulator_enable_regmap(rdev);
+ if (ret >= 0)
+ reg->enabled = true;
+
+ return ret;
}
static int qcom_labibb_regulator_disable(struct regulator_dev *rdev)
{
- return regulator_disable_regmap(rdev);
+ int ret = 0;
+ struct labibb_regulator *reg = rdev_get_drvdata(rdev);
+
+ ret = regulator_disable_regmap(rdev);
+ if (ret >= 0)
+ reg->enabled = false;
+
+ return ret;
}
static struct regulator_ops qcom_labibb_ops = {
@@ -73,12 +93,70 @@ static struct regulator_ops qcom_labibb_ops = {
.is_enabled = qcom_labibb_regulator_is_enabled,
};
+static irqreturn_t labibb_sc_err_handler(int irq, void *_reg)
+{
+ int ret;
+ u16 reg;
+ unsigned int val;
+ struct labibb_regulator *labibb_reg = _reg;
+ bool in_sc_err, scp_done = false;
+
+ ret = regmap_read(labibb_reg->regmap,
+ labibb_reg->base + REG_LABIBB_STATUS1, );
+ if (ret < 0) {
+ dev_err(labibb_reg->dev, "sc_err_irq: Read failed, ret=%d\n",
+ ret);
+ return IRQ_HANDLED;
+ }
+
+ dev_dbg(labibb_reg->dev, "%s SC error triggered! STATUS1 = %d\n",
+ labibb_reg->desc.name, val);
+
+ in_sc_err = !!(val & LABIBB_STATUS1_SC_DETECT_BIT);
+
+ /*
+* The SC(short circuit) fault would trigger PBS(Portable Batch
+* System) to disable regulators for protection. This would
+* cause the SC_DETECT status being cleared so that it's not
+* able to get the SC fault status.
+* Check if the regulator is enabled in the driver but
+* disabled in hardware, this means a SC fault had happened
+* and SCP handling is completed by PBS.
+*/
+ if (!in_sc_err) {
+
+ reg = labibb_reg->base + REG_LABIBB_ENABLE_CTL;
+
+ ret = regmap_read_poll_timeout(labibb_reg->regmap,
+ reg, val,
+
