diff mbox series

[4/4] regulator: adding interrupt handling in labibb regulator

Message ID 1560337252-27193-5-git-send-email-nishakumari@codeaurora.org (mailing list archive)
State New, archived
Headers show
Series Add labibb regulator support for LCD display mode | expand

Commit Message

Nisha Kumari June 12, 2019, 11 a.m. UTC 
This patch adds short circuit interrupt handling and recovery.

Signed-off-by: Nisha Kumari <nishakumari@codeaurora.org>
---
 drivers/regulator/qcom-labibb-regulator.c | 161 ++++++++++++++++++++++++++++++
 1 file changed, 161 insertions(+)

Comments

Mark Brown June 13, 2019, 5:27 p.m. UTC | #1 
On Wed, Jun 12, 2019 at 04:30:52PM +0530, Nisha Kumari wrote:

> +static void labibb_sc_err_recovery_work(void *_labibb)
> +{
> +	int ret;
> +	struct qcom_labibb *labibb = (struct qcom_labibb *)_labibb;
> +
> +	labibb->ibb_vreg.vreg_enabled = 0;
> +	labibb->lab_vreg.vreg_enabled = 0;
> +
> +	ret = qcom_ibb_regulator_enable(labibb->lab_vreg.rdev);

The driver should *never* enable the regulator itself, it should only do
this if the core told it to.

> +	/*
> +	 * 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 LAB/IBB regulators are enabled in the driver but
> +	 * disabled in hardware, this means a SC fault had happened
> +	 * and SCP handling is completed by PBS.
> +	 */

Let the core worry about this, the driver should just report the problem
to the core like all other devices do (and this driver doesn't...).
Nisha Kumari June 18, 2019, 6:23 a.m. UTC | #2 
On 6/13/2019 10:57 PM, Mark Brown wrote:
> On Wed, Jun 12, 2019 at 04:30:52PM +0530, Nisha Kumari wrote:
>
>> +static void labibb_sc_err_recovery_work(void *_labibb)
>> +{
>> +	int ret;
>> +	struct qcom_labibb *labibb = (struct qcom_labibb *)_labibb;
>> +
>> +	labibb->ibb_vreg.vreg_enabled = 0;
>> +	labibb->lab_vreg.vreg_enabled = 0;
>> +
>> +	ret = qcom_ibb_regulator_enable(labibb->lab_vreg.rdev);
> The driver should *never* enable the regulator itself, it should only do
> this if the core told it to.
Ok, I will change it
>
>> +	/*
>> +	 * 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 LAB/IBB regulators are enabled in the driver but
>> +	 * disabled in hardware, this means a SC fault had happened
>> +	 * and SCP handling is completed by PBS.
>> +	 */
> Let the core worry about this, the driver should just report the problem
> to the core like all other devices do (and this driver doesn't...).

Ok


Thanks,

Nisha
Sumit Semwal April 28, 2020, 5:16 a.m. UTC | #3 
Hello Mark,

I am looking to address review comments and push v2 of this series (we
need it for pixel3 and poco phones' mainline efforts): I have a query
on your review comment below:

On Thu, 13 Jun 2019 at 22:57, Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Jun 12, 2019 at 04:30:52PM +0530, Nisha Kumari wrote:
>
> > +static void labibb_sc_err_recovery_work(void *_labibb)
> > +{
> > +     int ret;
> > +     struct qcom_labibb *labibb = (struct qcom_labibb *)_labibb;
> > +
> > +     labibb->ibb_vreg.vreg_enabled = 0;
> > +     labibb->lab_vreg.vreg_enabled = 0;
> > +
> > +     ret = qcom_ibb_regulator_enable(labibb->lab_vreg.rdev);
>
> The driver should *never* enable the regulator itself, it should only do
> this if the core told it to.
>
> > +     /*
> > +      * 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 LAB/IBB regulators are enabled in the driver but
> > +      * disabled in hardware, this means a SC fault had happened
> > +      * and SCP handling is completed by PBS.
> > +      */
>
> Let the core worry about this, the driver should just report the problem
> to the core like all other devices do (and this driver doesn't...).

I (and Bjorn too) looked to find the api that allows us to do this
short circuit reporting and recovery in the core, but couldn't find
anything except REGULATOR_ERROR_OVER_CURRENT which also looks like
it's used only once in the code.

I am sure I'm missing something, maybe you could please help me find it?

Best,
Sumit.
Mark Brown April 28, 2020, 11:09 a.m. UTC | #4 
On Tue, Apr 28, 2020 at 10:46:52AM +0530, Sumit Semwal wrote:
> On Thu, 13 Jun 2019 at 22:57, Mark Brown <broonie@kernel.org> wrote:

> > > +     /*
> > > +      * 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 LAB/IBB regulators are enabled in the driver but
> > > +      * disabled in hardware, this means a SC fault had happened
> > > +      * and SCP handling is completed by PBS.
> > > +      */

> > Let the core worry about this, the driver should just report the problem
> > to the core like all other devices do (and this driver doesn't...).

> I (and Bjorn too) looked to find the api that allows us to do this
> short circuit reporting and recovery in the core, but couldn't find
> anything except REGULATOR_ERROR_OVER_CURRENT which also looks like
> it's used only once in the code.

A short circuit will generate excessive current (and detection of a
short circuit is usually current based) so using the same notification
should be fine.  If you're concerned about this feel free to add a
specific notification, and add any handling you need in response to that
notification.  You certainly shouldn't be just reenabling the regulators
in your driver.

Mostly AFAICT people are fairly happy with the autonomous response of
the hardware to these issues, it's not like they're expected to happen
in normal operation or be recoverable.
diff mbox series

Patch

diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
index 0c68883..04fc9512 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
 // Copyright (c) 2019, The Linux Foundation. All rights reserved.
 
+#include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/of_irq.h>
 #include <linux/platform_device.h>
@@ -16,22 +17,28 @@ 
 #define REG_LAB_ENABLE_CTL		0x46
 #define LAB_STATUS1_VREG_OK_BIT		BIT(7)
 #define LAB_ENABLE_CTL_EN		BIT(7)
+#define LAB_STATUS1_SC_DETECT_BIT	BIT(6)
 
 #define REG_IBB_STATUS1			0x08
 #define REG_IBB_ENABLE_CTL		0x46
 #define IBB_STATUS1_VREG_OK_BIT		BIT(7)
 #define IBB_ENABLE_CTL_MASK		(BIT(7) | BIT(6))
 #define IBB_CONTROL_ENABLE		BIT(7)
+#define IBB_STATUS1_SC_DETECT_BIT	BIT(6)
 
 #define POWER_DELAY			8000
+#define POLLING_SCP_DONE_COUNT		2
+#define POLLING_SCP_DONE_INTERVAL_MS	5
 
 struct lab_regulator {
 	struct regulator_dev		*rdev;
+	int				lab_sc_irq;
 	int				vreg_enabled;
 };
 
 struct ibb_regulator {
 	struct regulator_dev		*rdev;
+	int				ibb_sc_irq;
 	int				vreg_enabled;
 };
 
@@ -288,6 +295,112 @@  static int qcom_ibb_regulator_is_enabled(struct regulator_dev *rdev)
 	.owner = THIS_MODULE,
 };
 
+static void labibb_sc_err_recovery_work(void *_labibb)
+{
+	int ret;
+	struct qcom_labibb *labibb = (struct qcom_labibb *)_labibb;
+
+	labibb->ibb_vreg.vreg_enabled = 0;
+	labibb->lab_vreg.vreg_enabled = 0;
+
+	ret = qcom_ibb_regulator_enable(labibb->lab_vreg.rdev);
+	if (ret < 0) {
+		dev_err(labibb->dev,
+			"Interrupt recovery not possible as IBB enable failed");
+		return;
+	}
+
+	ret = qcom_lab_regulator_enable(labibb->ibb_vreg.rdev);
+	if (ret < 0) {
+		dev_err(labibb->dev,
+			"Interrupt recovery not possible as LAB enable failed");
+		ret = qcom_ibb_regulator_disable(labibb->lab_vreg.rdev);
+		if (ret < 0)
+			dev_err(labibb->dev, "IBB disable failed");
+		return;
+	}
+	dev_info(labibb->dev, "Interrupt recovery done");
+}
+
+static irqreturn_t labibb_sc_err_handler(int irq, void *_labibb)
+{
+	int ret, count;
+	u16 reg;
+	u8 sc_err_mask, val;
+	char *str;
+	struct qcom_labibb *labibb = (struct qcom_labibb *)_labibb;
+	bool in_sc_err, lab_en, ibb_en, scp_done = false;
+
+	if (irq == labibb->lab_vreg.lab_sc_irq) {
+		reg = labibb->lab_base + REG_LAB_STATUS1;
+		sc_err_mask = LAB_STATUS1_SC_DETECT_BIT;
+		str = "LAB";
+	} else if (irq == labibb->ibb_vreg.ibb_sc_irq) {
+		reg = labibb->ibb_base + REG_IBB_STATUS1;
+		sc_err_mask = IBB_STATUS1_SC_DETECT_BIT;
+		str = "IBB";
+	} else {
+		return IRQ_HANDLED;
+	}
+
+	ret = qcom_labibb_read(labibb, reg, &val, 1);
+	if (ret < 0) {
+		dev_err(labibb->dev, "Read failed, ret=%d\n", ret);
+		return IRQ_HANDLED;
+	}
+	dev_dbg(labibb->dev, "%s SC error triggered! %s_STATUS1 = %d\n",
+		str, str, val);
+
+	in_sc_err = !!(val & sc_err_mask);
+
+	/*
+	 * 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 LAB/IBB regulators are 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) {
+		count = POLLING_SCP_DONE_COUNT;
+		do {
+			reg = labibb->lab_base + REG_LAB_ENABLE_CTL;
+			ret = qcom_labibb_read(labibb, reg, &val, 1);
+			if (ret < 0) {
+				dev_err(labibb->dev,
+					"Read failed, ret=%d\n", ret);
+				return IRQ_HANDLED;
+			}
+			lab_en = !!(val & LAB_ENABLE_CTL_EN);
+
+			reg = labibb->ibb_base + REG_IBB_ENABLE_CTL;
+			ret = qcom_labibb_read(labibb, reg, &val, 1);
+			if (ret < 0) {
+				dev_err(labibb->dev,
+					"Read failed, ret=%d\n", ret);
+				return IRQ_HANDLED;
+			}
+			ibb_en = !!(val & IBB_CONTROL_ENABLE);
+			if (lab_en || ibb_en)
+				msleep(POLLING_SCP_DONE_INTERVAL_MS);
+			else
+				break;
+		} while ((lab_en || ibb_en) && count--);
+
+		if (labibb->lab_vreg.vreg_enabled &&
+		    labibb->ibb_vreg.vreg_enabled && !lab_en && !ibb_en) {
+			dev_dbg(labibb->dev, "LAB/IBB has been disabled by SCP\n");
+			scp_done = true;
+		}
+	}
+
+	if (in_sc_err || scp_done)
+		labibb_sc_err_recovery_work(labibb);
+
+	return IRQ_HANDLED;
+}
+
 static int register_lab_regulator(struct qcom_labibb *labibb,
 				  struct device_node *of_node)
 {
@@ -295,6 +408,20 @@  static int register_lab_regulator(struct qcom_labibb *labibb,
 	struct regulator_init_data *init_data;
 	struct regulator_config cfg = {};
 
+	(labibb->lab_vreg.lab_sc_irq > 0) {
+		ret = devm_request_threaded_irq(labibb->dev,
+						labibb->lab_vreg.lab_sc_irq,
+						NULL, labibb_sc_err_handler,
+						IRQF_ONESHOT |
+						IRQF_TRIGGER_RISING,
+						"lab-sc-err", labibb);
+		if (ret) {
+			dev_err(labibb->dev, "Failed to register 'lab-sc-err' irq ret=%d\n",
+				ret);
+			return ret;
+		}
+	}
+
 	cfg.dev = labibb->dev;
 	cfg.driver_data = labibb;
 	cfg.of_node = of_node;
@@ -326,6 +453,20 @@  static int register_ibb_regulator(struct qcom_labibb *labibb,
 	struct regulator_init_data *init_data;
 	struct regulator_config cfg = {};
 
+	if (labibb->ibb_vreg.ibb_sc_irq > 0) {
+		ret = devm_request_threaded_irq(labibb->dev,
+						labibb->ibb_vreg.ibb_sc_irq,
+						NULL, labibb_sc_err_handler,
+						IRQF_ONESHOT |
+						IRQF_TRIGGER_RISING,
+						"ibb-sc-err", labibb);
+		if (ret) {
+			dev_err(labibb->dev, "Failed to register 'ibb-sc-err' irq ret=%d\n",
+				ret);
+			return ret;
+		}
+	}
+
 	cfg.dev = labibb->dev;
 	cfg.driver_data = labibb;
 	cfg.of_node = of_node;
@@ -390,6 +531,16 @@  static int qcom_labibb_regulator_probe(struct platform_device *pdev)
 		switch (type) {
 		case QCOM_LAB_TYPE:
 			labibb->lab_base = base;
+
+			labibb->lab_vreg.lab_sc_irq = -EINVAL;
+			ret = of_irq_get_byname(child, "lab-sc-err");
+			if (ret < 0)
+				dev_dbg(labibb->dev,
+					"Unable to get lab-sc-err, ret = %d\n",
+					ret);
+			else
+				labibb->lab_vreg.lab_sc_irq = ret;
+
 			ret = register_lab_regulator(labibb, child);
 			if (ret < 0) {
 				dev_err(labibb->dev,
@@ -400,6 +551,16 @@  static int qcom_labibb_regulator_probe(struct platform_device *pdev)
 
 		case QCOM_IBB_TYPE:
 			labibb->ibb_base = base;
+
+			labibb->ibb_vreg.ibb_sc_irq = -EINVAL;
+			ret = of_irq_get_byname(child, "ibb-sc-err");
+			if (ret < 0)
+				dev_dbg(labibb->dev,
+					"Unable to get ibb-sc-err, ret = %d\n",
+					ret);
+			else
+				labibb->ibb_vreg.ibb_sc_irq = ret;
+
 			ret = register_ibb_regulator(labibb, child);
 			if (ret < 0) {
 				dev_err(labibb->dev,