| Message ID | 20210113194214.522238-6-angelogioacchino.delregno@somainline.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Really implement Qualcomm LAB/IBB regulators | expand |
On Wed 13 Jan 13:42 CST 2021, AngeloGioacchino Del Regno wrote: > Short-Circuit Protection (SCP) and Over-Current Protection (OCP) are > very important for regulators like LAB and IBB, which are designed to > provide from very small to relatively big amounts of current to the > device (normally, a display). > > Now that this regulator supports both voltage setting and current > limiting in this driver, to me it looked like being somehow essential > to provide support for SCP and OCP, for two reasons: > 1. SCP is a drastic measure to prevent damaging "more" hardware in > the worst situations, if any was damaged, preventing potentially > drastic issues; > 2. OCP is a great way to protect the hardware that we're powering > through these regulators as if anything bad happens, the HW will > draw more current than expected: in this case, the OCP interrupt > will fire and the regulators will be immediately shut down, > preventing hardware damage in many cases. So when the OCP fires it stops the regulator? Is it automatically enabled by the re-enabling of the OCP interrupt (if so please mention this in a comment or so in the code), or does it simply signal the client driver and it will have to re-enable the regulator? > > Both interrupts were successfully tested in a "sort-of" controlled > manner, with the following methodology: > > Short-Circuit Protection (SCP): > 1. Set LAB/IBB to 4.6/-1.4V, current limit 200mA/50mA; > 2. Connect a 10 KOhm resistor to LAB/IBB by poking the right traces > on a FxTec Pro1 smartphone for a very brief time (in short words, > "just a rapid touch with flying wires"); > 3. The Short-Circuit protection trips: IRQ raises, regulators get > cut. Recovery OK, test repeated without rebooting, OK. > > Over-Current Protection (OCP): > 1. Set LAB/IBB to the expected voltage to power up the display of > a Sony Xperia XZ Premium smartphone (Sharp LS055D1SX04), set > current limit to LAB 200mA, IBB 50mA (the values that this > display unit needs are 200/800mA); > 2. Boot the kernel: OCP fires. Recovery never happens because > the selected current limit is too low, but that's expected. > Test OK. > > 3. Set LAB/IBB to the expected current limits for XZ Premium > (LAB 200mA, IBB 800mA), but lower than expected voltage, > specifically LAB 5.4V, IBB -5.6V (instead of 5.6, -5.8V); > 4. Boot the kernel: OCP fires. Recovery never happens because > the selected voltage (still in the working range limits) > is producing a current draw of more than 200mA on LAB. > Test OK. > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> > --- > drivers/regulator/qcom-labibb-regulator.c | 447 +++++++++++++++++++++- > 1 file changed, 444 insertions(+), 3 deletions(-) > > diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c > index 38ab1eba1c59..38763625241e 100644 > --- a/drivers/regulator/qcom-labibb-regulator.c > +++ b/drivers/regulator/qcom-labibb-regulator.c > @@ -17,8 +17,20 @@ > > #define PMI8998_LAB_REG_BASE 0xde00 > #define PMI8998_IBB_REG_BASE 0xdc00 > +#define PMI8998_IBB_LAB_REG_OFFSET 0x200 > > #define REG_LABIBB_STATUS1 0x08 > + #define LABIBB_STATUS1_SC_BIT BIT(6) > + #define LABIBB_STATUS1_VREG_OK_BIT BIT(7) > + > +#define REG_LABIBB_INT_SET_TYPE 0x11 > +#define REG_LABIBB_INT_POLARITY_HIGH 0x12 > +#define REG_LABIBB_INT_POLARITY_LOW 0x13 > +#define REG_LABIBB_INT_LATCHED_CLR 0x14 > +#define REG_LABIBB_INT_EN_SET 0x15 > +#define REG_LABIBB_INT_EN_CLR 0x16 > + #define LABIBB_INT_VREG_OK BIT(0) > + #define LABIBB_INT_VREG_TYPE_LEVEL 0 > > #define REG_LABIBB_VOLTAGE 0x41 > #define LABIBB_VOLTAGE_OVERRIDE_EN BIT(7) > @@ -26,8 +38,7 @@ > #define IBB_VOLTAGE_SET_MASK GENMASK(5, 0) > > #define REG_LABIBB_ENABLE_CTL 0x46 > -#define LABIBB_STATUS1_VREG_OK_BIT BIT(7) > -#define LABIBB_CONTROL_ENABLE BIT(7) > + #define LABIBB_CONTROL_ENABLE BIT(7) > > #define REG_LABIBB_PD_CTL 0x47 > #define LAB_PD_CTL_MASK GENMASK(1, 0) > @@ -56,6 +67,11 @@ > #define LAB_ENABLE_TIME (LABIBB_OFF_ON_DELAY * 2) > #define IBB_ENABLE_TIME (LABIBB_OFF_ON_DELAY * 10) > #define LABIBB_POLL_ENABLED_TIME 1000 > +#define OCP_RECOVERY_INTERVAL_MS 500 > +#define SC_RECOVERY_INTERVAL_MS 250 > +#define LABIBB_MAX_OCP_COUNT 4 > +#define LABIBB_MAX_SC_COUNT 3 > +#define LABIBB_MAX_FATAL_COUNT 2 > > struct labibb_current_limits { > u32 uA_min; > @@ -69,10 +85,17 @@ struct labibb_regulator { > struct regmap *regmap; > struct regulator_dev *rdev; > struct labibb_current_limits uA_limits; > + struct delayed_work ocp_recovery_work; > + struct delayed_work sc_recovery_work; > u16 base; > u8 type; > u8 dischg_sel; > u8 soft_start_sel; > + int sc_irq; > + int sc_count; > + int ocp_irq; > + int ocp_irq_count; > + int fatal_count; > }; > > struct labibb_regulator_data { > @@ -82,6 +105,379 @@ struct labibb_regulator_data { > const struct regulator_desc *desc; > }; > > +static int qcom_labibb_ocp_hw_enable(struct regulator_dev *rdev) > +{ > + struct labibb_regulator *vreg = rdev_get_drvdata(rdev); > + int ret; > + > + /* Clear irq latch status to avoid spurious event */ > + ret = regmap_update_bits(rdev->regmap, > + vreg->base + REG_LABIBB_INT_LATCHED_CLR, > + LABIBB_INT_VREG_OK, 1); Either the clear register reads as 0, making the read-modify-write pointless, or there are 1s in there which we inadvertently will use to clear unrelated interrupts with. So a regmap_write() seems more appropriate. > + if (ret) > + return ret; > + > + /* Enable OCP HW interrupt */ > + return regmap_update_bits(rdev->regmap, > + vreg->base + REG_LABIBB_INT_EN_SET, > + LABIBB_INT_VREG_OK, 1); > +} > + > +static int qcom_labibb_ocp_hw_disable(struct regulator_dev *rdev) > +{ > + struct labibb_regulator *vreg = rdev_get_drvdata(rdev); > + > + return regmap_update_bits(rdev->regmap, > + vreg->base + REG_LABIBB_INT_EN_CLR, > + LABIBB_INT_VREG_OK, 1); > +} > + > +/* Please add another '*' to complete the /** and () to the function name, if you intend for this to be valid kerneldoc. > + * qcom_labibb_check_ocp_status - Check the Over-Current Protection status > + * @rdev: Regulator device rdev != vreg > + * > + * This function checks the STATUS1 register for the VREG_OK bit: if it is > + * set, then there is no Over-Current event. > + * > + * Returns: Zero if there is no over-current, 1 if in over-current or > + * negative number for error > + */ > +static int qcom_labibb_check_ocp_status(struct labibb_regulator *vreg) > +{ > + u32 cur_status; > + int ret; > + > + ret = regmap_read(vreg->rdev->regmap, vreg->base + REG_LABIBB_STATUS1, > + &cur_status); > + if (ret) > + return ret; > + > + return !(cur_status & LABIBB_STATUS1_VREG_OK_BIT); > +} > + > +static void qcom_labibb_ocp_recovery_worker(struct work_struct *work) > +{ > + struct labibb_regulator *vreg; > + const struct regulator_ops *ops; > + int ret; > + > + vreg = container_of(work, struct labibb_regulator, > + ocp_recovery_work.work); > + ops = vreg->rdev->desc->ops; > + > + if (vreg->ocp_irq_count >= LABIBB_MAX_OCP_COUNT) { > + /* > + * If we tried to disable the regulator multiple times but > + * we kept failing, there's only one last hope to save our > + * hardware from the death: raise a kernel bug, reboot and > + * hope that the bootloader kindly saves us. This, though > + * is done only as paranoid checking, because failing the > + * regmap write to disable the vreg is almost impossible, > + * since we got here after multiple regmap R/W. > + */ > + BUG_ON(vreg->fatal_count > LABIBB_MAX_FATAL_COUNT); > + dev_err(&vreg->rdev->dev, "LABIBB: CRITICAL: Disabling regulator\n"); > + > + /* Disable the regulator immediately to avoid damage */ > + ret = ops->disable(vreg->rdev); > + if (ret) { > + vreg->fatal_count++; > + goto reschedule; > + } > + enable_irq(vreg->ocp_irq); > + vreg->fatal_count = 0; > + return; > + } > + > + ret = qcom_labibb_check_ocp_status(vreg); > + if (ret != 0) { > + vreg->ocp_irq_count++; > + goto reschedule; > + } > + > + ret = qcom_labibb_ocp_hw_enable(vreg->rdev); > + if (ret) { > + /* We cannot trust it without OCP enabled. */ > + dev_err(vreg->dev, "Cannot enable OCP IRQ\n"); > + vreg->ocp_irq_count++; > + goto reschedule; > + } > + > + enable_irq(vreg->ocp_irq); > + /* Everything went fine: reset the OCP count! */ > + vreg->ocp_irq_count = 0; > + return; > + > +reschedule: > + mod_delayed_work(system_wq, &vreg->ocp_recovery_work, > + msecs_to_jiffies(OCP_RECOVERY_INTERVAL_MS)); > +} > + > +static irqreturn_t qcom_labibb_ocp_isr(int irq, void *chip) > +{ > + struct labibb_regulator *vreg = chip; > + const struct regulator_ops *ops = vreg->rdev->desc->ops; > + int ret; > + > + /* If the regulator is not enabled, this is a fake event */ > + if (!ops->is_enabled(vreg->rdev)) > + return 0; > + > + /* If we tried to recover for too many times it's not getting better */ > + if (vreg->ocp_irq_count > LABIBB_MAX_OCP_COUNT) > + return IRQ_NONE; > + > + /* > + * If we (unlikely) can't read this register, to prevent hardware > + * damage at all costs, we assume that the overcurrent event was > + * real; Moreover, if the status register is not signaling OCP, > + * it was a spurious event, so it's all ok. > + */ > + ret = qcom_labibb_check_ocp_status(vreg); > + if (ret == 0) { > + vreg->ocp_irq_count = 0; > + goto end; > + } > + vreg->ocp_irq_count++; > + > + /* > + * Disable the interrupt temporarily, or it will fire continuously; > + * we will re-enable it in the recovery worker function. > + */ > + disable_irq(irq); > + > + /* Warn the user for overcurrent */ > + dev_warn(vreg->dev, "Over-Current interrupt fired!\n"); > + > + /* Disable the interrupt to avoid hogging */ > + ret = qcom_labibb_ocp_hw_disable(vreg->rdev); > + if (ret) > + goto end; > + > + /* Signal overcurrent event to drivers */ > + regulator_notifier_call_chain(vreg->rdev, > + REGULATOR_EVENT_OVER_CURRENT, NULL); > + > +end: > + /* Schedule the recovery work */ > + schedule_delayed_work(&vreg->ocp_recovery_work, > + msecs_to_jiffies(OCP_RECOVERY_INTERVAL_MS)); > + if (ret) > + return IRQ_NONE; > + > + return IRQ_HANDLED; > +} > + > +static int qcom_labibb_set_ocp(struct regulator_dev *rdev) > +{ > + struct labibb_regulator *vreg = rdev_get_drvdata(rdev); > + char *ocp_irq_name; > + u32 irq_flags = IRQF_ONESHOT; > + int irq_trig_low, ret; > + > + /* If there is no OCP interrupt, there's nothing to set */ > + if (vreg->ocp_irq <= 0) > + return -EINVAL; > + > + ocp_irq_name = devm_kasprintf(vreg->dev, GFP_KERNEL, "%s-over-current", > + vreg->desc.name); > + if (!ocp_irq_name) > + return -ENOMEM; > + > + /* IRQ polarities - LAB: trigger-low, IBB: trigger-high */ > + switch (vreg->type) { > + case QCOM_LAB_TYPE: > + irq_flags |= IRQF_TRIGGER_LOW; > + irq_trig_low = 1; > + break; > + case QCOM_IBB_TYPE: > + irq_flags |= IRQF_TRIGGER_HIGH; > + irq_trig_low = 0; > + break; > + default: > + return -EINVAL; > + } > + > + /* Activate OCP HW level interrupt */ > + ret = regmap_update_bits(rdev->regmap, > + vreg->base + REG_LABIBB_INT_SET_TYPE, > + LABIBB_INT_VREG_OK, > + LABIBB_INT_VREG_TYPE_LEVEL); > + if (ret) > + return ret; > + > + /* Set OCP interrupt polarity */ > + ret = regmap_update_bits(rdev->regmap, > + vreg->base + REG_LABIBB_INT_POLARITY_HIGH, > + LABIBB_INT_VREG_OK, !irq_trig_low); > + if (ret) > + return ret; > + ret = regmap_update_bits(rdev->regmap, > + vreg->base + REG_LABIBB_INT_POLARITY_LOW, > + LABIBB_INT_VREG_OK, irq_trig_low); > + if (ret) > + return ret; > + > + ret = qcom_labibb_ocp_hw_enable(rdev); > + if (ret) > + return ret; > + > + return devm_request_threaded_irq(vreg->dev, vreg->ocp_irq, NULL, > + qcom_labibb_ocp_isr, irq_flags, > + ocp_irq_name, vreg); > +} > + > +/* > + * qcom_labibb_check_sc_status - Check the Short Circuit Protection status > + * @rdev: Regulator device > + * > + * This function checks the STATUS1 register on both LAB and IBB regulators > + * for the ShortCircuit bit: if it is set on *any* of them, then we have > + * experienced a short-circuit event. > + * > + * Returns: Zero if there is no short-circuit, 1 if in short-circuit or > + * negative number for error > + */ > +static int qcom_labibb_check_sc_status(struct labibb_regulator *vreg) > +{ > + u32 ibb_status, ibb_reg, lab_status, lab_reg; > + int ret; > + > + /* We have to work on both regulators due to PBS... */ > + lab_reg = ibb_reg = vreg->base; > + if (vreg->type == QCOM_LAB_TYPE) > + ibb_reg -= PMI8998_IBB_LAB_REG_OFFSET; Minus? The register comes before the base address? > + else > + lab_reg += PMI8998_IBB_LAB_REG_OFFSET; > + > + ret = regmap_read(vreg->rdev->regmap, lab_reg, &lab_status); > + if (ret) > + return ret; > + ret = regmap_read(vreg->rdev->regmap, ibb_reg, &ibb_status); > + if (ret) > + return ret; > + > + return !!(lab_status & LABIBB_STATUS1_SC_BIT) || > + !!(ibb_status & LABIBB_STATUS1_SC_BIT); > +} > + > +static void qcom_labibb_sc_recovery_worker(struct work_struct *work) > +{ > + struct labibb_regulator *vreg; > + const struct regulator_ops *ops; > + u32 lab_reg, ibb_reg, temp, val; > + bool pbs_cut = false; > + int i, sc, ret; > + > + vreg = container_of(work, struct labibb_regulator, > + sc_recovery_work.work); > + ops = vreg->rdev->desc->ops; > + > + /* > + * If we tried to check the regulator status multiple times but we > + * kept failing, then just bail out, as the Portable Batch System > + * (PBS) will disable the vregs for us, preventing hardware damage. > + */ > + if (vreg->fatal_count > LABIBB_MAX_FATAL_COUNT) > + return; > + > + /* Too many short-circuit events. Throw in the towel. */ > + if (vreg->sc_count > LABIBB_MAX_SC_COUNT) > + return; > + > + /* > + * The Portable Batch System (PBS) automatically disables LAB > + * and IBB when a short-circuit event is detected, so we have to > + * check and work on both of them at the same time. > + */ > + lab_reg = ibb_reg = vreg->base; > + if (vreg->type == QCOM_LAB_TYPE) > + ibb_reg -= PMI8998_IBB_LAB_REG_OFFSET; > + else > + lab_reg += PMI8998_IBB_LAB_REG_OFFSET; > + > + sc = qcom_labibb_check_sc_status(vreg); > + if (sc) > + goto reschedule; > + > + for (i = 0; i < LABIBB_MAX_SC_COUNT; i++) { > + ret = regmap_read(vreg->regmap, lab_reg, &temp); > + if (ret) { > + vreg->fatal_count++; > + goto reschedule; > + } > + val = temp; > + > + ret = regmap_read(vreg->regmap, ibb_reg, &temp); > + if (ret) { > + vreg->fatal_count++; > + goto reschedule; > + } > + val &= temp; Perhaps using two variables, with suitable names makes this more maintainable? > + > + if (val & LABIBB_CONTROL_ENABLE) { Flip this around and do pbs_cut = true; break; in the conditional, to not end the loop with a conditional continue and an unconditional break. > + usleep_range(5000, 6000); > + continue; > + } > + pbs_cut = true; > + break; > + } > + if (pbs_cut) > + goto reschedule; > + > + /* > + * If we have reached this point, we either had a spurious SC IRQ > + * or we have successfully recovered from the SC condition, which Wouldn't it make sense to put the "recovered from the SC condition" before the "spurious" part in this sentence - just seems like this is the scenario we're most likely looking for. Regards, Bjorn > + * means that we can re-enable the regulators, if they have ever > + * been disabled by the PBS. > + */ > + ret = ops->enable(vreg->rdev); > + if (ret) > + goto reschedule; > + > + /* Everything went fine: reset the OCP count! */ > + vreg->sc_count = 0; > + enable_irq(vreg->sc_irq); > + return; > + > +reschedule: > + /* > + * Now that we have done basic handling of the short-circuit, > + * reschedule this worker in the regular system workqueue, as > + * taking action is not truly urgent anymore. > + */ > + vreg->sc_count++; > + mod_delayed_work(system_wq, &vreg->sc_recovery_work, > + msecs_to_jiffies(SC_RECOVERY_INTERVAL_MS)); > +} > + > +static irqreturn_t qcom_labibb_sc_isr(int irq, void *chip) > +{ > + struct labibb_regulator *vreg = chip; > + > + if (vreg->sc_count > LABIBB_MAX_SC_COUNT) > + return IRQ_NONE; > + > + /* Warn the user for short circuit */ > + dev_warn(vreg->dev, "Short-Circuit interrupt fired!\n"); > + > + /* > + * Disable the interrupt temporarily, or it will fire continuously; > + * we will re-enable it in the recovery worker function. > + */ > + disable_irq(irq); > + > + /* Signal out of regulation event to drivers */ > + regulator_notifier_call_chain(vreg->rdev, > + REGULATOR_EVENT_REGULATION_OUT, NULL); > + > + /* Schedule the short-circuit handling as high-priority work */ > + mod_delayed_work(system_highpri_wq, &vreg->sc_recovery_work, > + msecs_to_jiffies(SC_RECOVERY_INTERVAL_MS)); > + return IRQ_HANDLED; > +} > + > + > static int qcom_labibb_set_current_limit(struct regulator_dev *rdev, > int min_uA, int max_uA) > { > @@ -210,6 +606,7 @@ static const struct regulator_ops qcom_labibb_ops = { > .set_current_limit = qcom_labibb_set_current_limit, > .get_current_limit = qcom_labibb_get_current_limit, > .set_soft_start = qcom_labibb_set_soft_start, > + .set_over_current_protection = qcom_labibb_set_ocp, > }; > > static const struct regulator_desc pmi8998_lab_desc = { > @@ -291,7 +688,7 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev) > struct labibb_regulator *vreg; > struct device *dev = &pdev->dev; > struct regulator_config cfg = {}; > - > + struct device_node *reg_node; > const struct of_device_id *match; > const struct labibb_regulator_data *reg_data; > struct regmap *reg_regmap; > @@ -309,6 +706,8 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev) > return -ENODEV; > > for (reg_data = match->data; reg_data->name; reg_data++) { > + char *sc_irq_name; > + int irq = 0; > > /* Validate if the type of regulator is indeed > * what's mentioned in DT. > @@ -331,10 +730,44 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev) > if (!vreg) > return -ENOMEM; > > + sc_irq_name = devm_kasprintf(dev, GFP_KERNEL, > + "%s-short-circuit", > + reg_data->name); > + if (!sc_irq_name) > + return -ENOMEM; > + > + reg_node = of_get_child_by_name(pdev->dev.of_node, > + reg_data->name); > + if (!reg_node) > + return -EINVAL; > + > + /* The Short Circuit interrupt is critical */ > + irq = of_irq_get_byname(reg_node, "sc-err"); > + if (irq <= 0) { > + if (irq == 0) > + irq = -EINVAL; > + > + return dev_err_probe(vreg->dev, irq, > + "Short-circuit irq not found.\n"); > + } > + vreg->sc_irq = irq; > + > + /* OverCurrent Protection IRQ is optional */ > + irq = of_irq_get_byname(reg_node, "ocp"); > + vreg->ocp_irq = irq; > + vreg->ocp_irq_count = 0; > + of_node_put(reg_node); > + > vreg->regmap = reg_regmap; > vreg->dev = dev; > vreg->base = reg_data->base; > vreg->type = reg_data->type; > + INIT_DELAYED_WORK(&vreg->sc_recovery_work, > + qcom_labibb_sc_recovery_worker); > + > + if (vreg->ocp_irq > 0) > + INIT_DELAYED_WORK(&vreg->ocp_recovery_work, > + qcom_labibb_ocp_recovery_worker); > > switch (vreg->type) { > case QCOM_LAB_TYPE: > @@ -369,6 +802,14 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev) > reg_data->name, ret); > return PTR_ERR(vreg->rdev); > } > + > + ret = devm_request_threaded_irq(vreg->dev, vreg->sc_irq, NULL, > + qcom_labibb_sc_isr, > + IRQF_ONESHOT | > + IRQF_TRIGGER_RISING, > + sc_irq_name, vreg); > + if (ret) > + return ret; > } > > return 0; > -- > 2.29.2 >
Il 15/01/21 06:31, Bjorn Andersson ha scritto: > On Wed 13 Jan 13:42 CST 2021, AngeloGioacchino Del Regno wrote: > >> Short-Circuit Protection (SCP) and Over-Current Protection (OCP) are >> very important for regulators like LAB and IBB, which are designed to >> provide from very small to relatively big amounts of current to the >> device (normally, a display). >> >> Now that this regulator supports both voltage setting and current >> limiting in this driver, to me it looked like being somehow essential >> to provide support for SCP and OCP, for two reasons: >> 1. SCP is a drastic measure to prevent damaging "more" hardware in >> the worst situations, if any was damaged, preventing potentially >> drastic issues; >> 2. OCP is a great way to protect the hardware that we're powering >> through these regulators as if anything bad happens, the HW will >> draw more current than expected: in this case, the OCP interrupt >> will fire and the regulators will be immediately shut down, >> preventing hardware damage in many cases. > > So when the OCP fires it stops the regulator? Is it automatically > enabled by the re-enabling of the OCP interrupt (if so please mention > this in a comment or so in the code), or does it simply signal the > client driver and it will have to re-enable the regulator? > I am sorry if my comments in the driver are not clear enough but, before giving you an answer, let me explain my comment logic: In function qcom_labibb_ocp_isr(): /* Signal overcurrent event to drivers */ /* Schedule the recovery work */ In function qcom_labibb_ocp_recovery_worker(): if (vreg->ocp_irq_count >= LABIBB_MAX_OCP_COUNT) /* * If we tried to disable the regulator multiple times but * we kept failing, there's only one last hope to save our * hardware from the death: raise a kernel bug, reboot and * hope that the bootloader kindly saves us. This, though * is done only as paranoid checking, because failing the * regmap write to disable the vreg is almost impossible, * since we got here after multiple regmap R/W. */ ret = qcom_labibb_ocp_hw_enable(vreg->rdev); if (ret) /* We cannot trust it without OCP enabled. */ (hence, reschedule worker) /* Everything went fine: reset the OCP count! */ vreg->ocp_irq_count = 0; So, schematically: 1. Signals overcurrent event to the driver that is attached; the driver will have to take action on its own - it's not our problem, for now; 2. Schedules a recovery worker, which waits until we stop reading "OUCH! OVERCURRENT!" from the hardware and reschedules itself for 4 times 3. Did the driver take action? Did the hardware just stabilize its current draw for whatever reason? Now it may be our problem. 3a. (Yes!) Great, we're done, everything is fine; 3b. (No! The driver didn't care, or the hardware is really at fault!) Force disabling the regulator to protect HW; 3b-alt. We couldn't disable the regulator, something is seriously wrong, we are frying the HW. PRESS THE RED BUTTON! Panic and pray that the bootloader saves us and that it's not too late. We really can't do anything more than that. Is the flow clear now? How would you improve the comments in this commit, if necessary? >> >> Both interrupts were successfully tested in a "sort-of" controlled >> manner, with the following methodology: >> >> Short-Circuit Protection (SCP): >> 1. Set LAB/IBB to 4.6/-1.4V, current limit 200mA/50mA; >> 2. Connect a 10 KOhm resistor to LAB/IBB by poking the right traces >> on a FxTec Pro1 smartphone for a very brief time (in short words, >> "just a rapid touch with flying wires"); >> 3. The Short-Circuit protection trips: IRQ raises, regulators get >> cut. Recovery OK, test repeated without rebooting, OK. >> >> Over-Current Protection (OCP): >> 1. Set LAB/IBB to the expected voltage to power up the display of >> a Sony Xperia XZ Premium smartphone (Sharp LS055D1SX04), set >> current limit to LAB 200mA, IBB 50mA (the values that this >> display unit needs are 200/800mA); >> 2. Boot the kernel: OCP fires. Recovery never happens because >> the selected current limit is too low, but that's expected. >> Test OK. >> >> 3. Set LAB/IBB to the expected current limits for XZ Premium >> (LAB 200mA, IBB 800mA), but lower than expected voltage, >> specifically LAB 5.4V, IBB -5.6V (instead of 5.6, -5.8V); >> 4. Boot the kernel: OCP fires. Recovery never happens because >> the selected voltage (still in the working range limits) >> is producing a current draw of more than 200mA on LAB. >> Test OK. >> >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> >> --- >> drivers/regulator/qcom-labibb-regulator.c | 447 +++++++++++++++++++++- >> 1 file changed, 444 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c >> index 38ab1eba1c59..38763625241e 100644 >> --- a/drivers/regulator/qcom-labibb-regulator.c >> +++ b/drivers/regulator/qcom-labibb-regulator.c >> @@ -17,8 +17,20 @@ >> >> #define PMI8998_LAB_REG_BASE 0xde00 >> #define PMI8998_IBB_REG_BASE 0xdc00 >> +#define PMI8998_IBB_LAB_REG_OFFSET 0x200 >> >> #define REG_LABIBB_STATUS1 0x08 >> + #define LABIBB_STATUS1_SC_BIT BIT(6) >> + #define LABIBB_STATUS1_VREG_OK_BIT BIT(7) >> + >> +#define REG_LABIBB_INT_SET_TYPE 0x11 >> +#define REG_LABIBB_INT_POLARITY_HIGH 0x12 >> +#define REG_LABIBB_INT_POLARITY_LOW 0x13 >> +#define REG_LABIBB_INT_LATCHED_CLR 0x14 >> +#define REG_LABIBB_INT_EN_SET 0x15 >> +#define REG_LABIBB_INT_EN_CLR 0x16 >> + #define LABIBB_INT_VREG_OK BIT(0) >> + #define LABIBB_INT_VREG_TYPE_LEVEL 0 >> >> #define REG_LABIBB_VOLTAGE 0x41 >> #define LABIBB_VOLTAGE_OVERRIDE_EN BIT(7) >> @@ -26,8 +38,7 @@ >> #define IBB_VOLTAGE_SET_MASK GENMASK(5, 0) >> >> #define REG_LABIBB_ENABLE_CTL 0x46 >> -#define LABIBB_STATUS1_VREG_OK_BIT BIT(7) >> -#define LABIBB_CONTROL_ENABLE BIT(7) >> + #define LABIBB_CONTROL_ENABLE BIT(7) >> >> #define REG_LABIBB_PD_CTL 0x47 >> #define LAB_PD_CTL_MASK GENMASK(1, 0) >> @@ -56,6 +67,11 @@ >> #define LAB_ENABLE_TIME (LABIBB_OFF_ON_DELAY * 2) >> #define IBB_ENABLE_TIME (LABIBB_OFF_ON_DELAY * 10) >> #define LABIBB_POLL_ENABLED_TIME 1000 >> +#define OCP_RECOVERY_INTERVAL_MS 500 >> +#define SC_RECOVERY_INTERVAL_MS 250 >> +#define LABIBB_MAX_OCP_COUNT 4 >> +#define LABIBB_MAX_SC_COUNT 3 >> +#define LABIBB_MAX_FATAL_COUNT 2 >> >> struct labibb_current_limits { >> u32 uA_min; >> @@ -69,10 +85,17 @@ struct labibb_regulator { >> struct regmap *regmap; >> struct regulator_dev *rdev; >> struct labibb_current_limits uA_limits; >> + struct delayed_work ocp_recovery_work; >> + struct delayed_work sc_recovery_work; >> u16 base; >> u8 type; >> u8 dischg_sel; >> u8 soft_start_sel; >> + int sc_irq; >> + int sc_count; >> + int ocp_irq; >> + int ocp_irq_count; >> + int fatal_count; >> }; >> >> struct labibb_regulator_data { >> @@ -82,6 +105,379 @@ struct labibb_regulator_data { >> const struct regulator_desc *desc; >> }; >> >> +static int qcom_labibb_ocp_hw_enable(struct regulator_dev *rdev) >> +{ >> + struct labibb_regulator *vreg = rdev_get_drvdata(rdev); >> + int ret; >> + >> + /* Clear irq latch status to avoid spurious event */ >> + ret = regmap_update_bits(rdev->regmap, >> + vreg->base + REG_LABIBB_INT_LATCHED_CLR, >> + LABIBB_INT_VREG_OK, 1); > > Either the clear register reads as 0, making the read-modify-write > pointless, or there are 1s in there which we inadvertently will use to > clear unrelated interrupts with. > > So a regmap_write() seems more appropriate. > I'm not sure about what the other bits are for in that register, that's why I chose to use regmap_update_bits... If you are sure that I can safely use regmap_write here, then I will, so since it is safety protections that we're talking about, I feel the need to have a 100% sure answer on this, hence I ask: Are you sure that it is safe to use regmap_write instead, here? (P.S.: You should know, I don't want to seem rude in any way, it's just a paranoid concern about safety) >> + if (ret) >> + return ret; >> + >> + /* Enable OCP HW interrupt */ >> + return regmap_update_bits(rdev->regmap, >> + vreg->base + REG_LABIBB_INT_EN_SET, >> + LABIBB_INT_VREG_OK, 1); >> +} >> + >> +static int qcom_labibb_ocp_hw_disable(struct regulator_dev *rdev) >> +{ >> + struct labibb_regulator *vreg = rdev_get_drvdata(rdev); >> + >> + return regmap_update_bits(rdev->regmap, >> + vreg->base + REG_LABIBB_INT_EN_CLR, >> + LABIBB_INT_VREG_OK, 1); >> +} >> + >> +/* > > Please add another '*' to complete the /** and () to the function name, > if you intend for this to be valid kerneldoc. > Oh, thank you, that was a typo. Yes I wanted to write /**. >> + * qcom_labibb_check_ocp_status - Check the Over-Current Protection status >> + * @rdev: Regulator device > > rdev != vreg > Of course, @vreg: Main driver structure, not unexistant rdev. Big oops! >> + * >> + * This function checks the STATUS1 register for the VREG_OK bit: if it is >> + * set, then there is no Over-Current event. >> + * >> + * Returns: Zero if there is no over-current, 1 if in over-current or >> + * negative number for error >> + */ >> +static int qcom_labibb_check_ocp_status(struct labibb_regulator *vreg) >> +{ >> + u32 cur_status; >> + int ret; >> + >> + ret = regmap_read(vreg->rdev->regmap, vreg->base + REG_LABIBB_STATUS1, >> + &cur_status); >> + if (ret) >> + return ret; >> + >> + return !(cur_status & LABIBB_STATUS1_VREG_OK_BIT); >> +} >> + >> +static void qcom_labibb_ocp_recovery_worker(struct work_struct *work) >> +{ >> + struct labibb_regulator *vreg; >> + const struct regulator_ops *ops; >> + int ret; >> + >> + vreg = container_of(work, struct labibb_regulator, >> + ocp_recovery_work.work); >> + ops = vreg->rdev->desc->ops; >> + >> + if (vreg->ocp_irq_count >= LABIBB_MAX_OCP_COUNT) { >> + /* >> + * If we tried to disable the regulator multiple times but >> + * we kept failing, there's only one last hope to save our >> + * hardware from the death: raise a kernel bug, reboot and >> + * hope that the bootloader kindly saves us. This, though >> + * is done only as paranoid checking, because failing the >> + * regmap write to disable the vreg is almost impossible, >> + * since we got here after multiple regmap R/W. >> + */ >> + BUG_ON(vreg->fatal_count > LABIBB_MAX_FATAL_COUNT); >> + dev_err(&vreg->rdev->dev, "LABIBB: CRITICAL: Disabling regulator\n"); >> + >> + /* Disable the regulator immediately to avoid damage */ >> + ret = ops->disable(vreg->rdev); >> + if (ret) { >> + vreg->fatal_count++; >> + goto reschedule; >> + } >> + enable_irq(vreg->ocp_irq); >> + vreg->fatal_count = 0; >> + return; >> + } >> + >> + ret = qcom_labibb_check_ocp_status(vreg); >> + if (ret != 0) { >> + vreg->ocp_irq_count++; >> + goto reschedule; >> + } >> + >> + ret = qcom_labibb_ocp_hw_enable(vreg->rdev); >> + if (ret) { >> + /* We cannot trust it without OCP enabled. */ >> + dev_err(vreg->dev, "Cannot enable OCP IRQ\n"); >> + vreg->ocp_irq_count++; >> + goto reschedule; >> + } >> + >> + enable_irq(vreg->ocp_irq); >> + /* Everything went fine: reset the OCP count! */ >> + vreg->ocp_irq_count = 0; >> + return; >> + >> +reschedule: >> + mod_delayed_work(system_wq, &vreg->ocp_recovery_work, >> + msecs_to_jiffies(OCP_RECOVERY_INTERVAL_MS)); >> +} >> + >> +static irqreturn_t qcom_labibb_ocp_isr(int irq, void *chip) >> +{ >> + struct labibb_regulator *vreg = chip; >> + const struct regulator_ops *ops = vreg->rdev->desc->ops; >> + int ret; >> + >> + /* If the regulator is not enabled, this is a fake event */ >> + if (!ops->is_enabled(vreg->rdev)) >> + return 0; >> + >> + /* If we tried to recover for too many times it's not getting better */ >> + if (vreg->ocp_irq_count > LABIBB_MAX_OCP_COUNT) >> + return IRQ_NONE; >> + >> + /* >> + * If we (unlikely) can't read this register, to prevent hardware >> + * damage at all costs, we assume that the overcurrent event was >> + * real; Moreover, if the status register is not signaling OCP, >> + * it was a spurious event, so it's all ok. >> + */ >> + ret = qcom_labibb_check_ocp_status(vreg); >> + if (ret == 0) { >> + vreg->ocp_irq_count = 0; >> + goto end; >> + } >> + vreg->ocp_irq_count++; >> + >> + /* >> + * Disable the interrupt temporarily, or it will fire continuously; >> + * we will re-enable it in the recovery worker function. >> + */ >> + disable_irq(irq); >> + >> + /* Warn the user for overcurrent */ >> + dev_warn(vreg->dev, "Over-Current interrupt fired!\n"); >> + >> + /* Disable the interrupt to avoid hogging */ >> + ret = qcom_labibb_ocp_hw_disable(vreg->rdev); >> + if (ret) >> + goto end; >> + >> + /* Signal overcurrent event to drivers */ >> + regulator_notifier_call_chain(vreg->rdev, >> + REGULATOR_EVENT_OVER_CURRENT, NULL); >> + >> +end: >> + /* Schedule the recovery work */ >> + schedule_delayed_work(&vreg->ocp_recovery_work, >> + msecs_to_jiffies(OCP_RECOVERY_INTERVAL_MS)); >> + if (ret) >> + return IRQ_NONE; >> + >> + return IRQ_HANDLED; >> +} >> + >> +static int qcom_labibb_set_ocp(struct regulator_dev *rdev) >> +{ >> + struct labibb_regulator *vreg = rdev_get_drvdata(rdev); >> + char *ocp_irq_name; >> + u32 irq_flags = IRQF_ONESHOT; >> + int irq_trig_low, ret; >> + >> + /* If there is no OCP interrupt, there's nothing to set */ >> + if (vreg->ocp_irq <= 0) >> + return -EINVAL; >> + >> + ocp_irq_name = devm_kasprintf(vreg->dev, GFP_KERNEL, "%s-over-current", >> + vreg->desc.name); >> + if (!ocp_irq_name) >> + return -ENOMEM; >> + >> + /* IRQ polarities - LAB: trigger-low, IBB: trigger-high */ >> + switch (vreg->type) { >> + case QCOM_LAB_TYPE: >> + irq_flags |= IRQF_TRIGGER_LOW; >> + irq_trig_low = 1; >> + break; >> + case QCOM_IBB_TYPE: >> + irq_flags |= IRQF_TRIGGER_HIGH; >> + irq_trig_low = 0; >> + break; >> + default: >> + return -EINVAL; >> + } >> + >> + /* Activate OCP HW level interrupt */ >> + ret = regmap_update_bits(rdev->regmap, >> + vreg->base + REG_LABIBB_INT_SET_TYPE, >> + LABIBB_INT_VREG_OK, >> + LABIBB_INT_VREG_TYPE_LEVEL); >> + if (ret) >> + return ret; >> + >> + /* Set OCP interrupt polarity */ >> + ret = regmap_update_bits(rdev->regmap, >> + vreg->base + REG_LABIBB_INT_POLARITY_HIGH, >> + LABIBB_INT_VREG_OK, !irq_trig_low); >> + if (ret) >> + return ret; >> + ret = regmap_update_bits(rdev->regmap, >> + vreg->base + REG_LABIBB_INT_POLARITY_LOW, >> + LABIBB_INT_VREG_OK, irq_trig_low); >> + if (ret) >> + return ret; >> + >> + ret = qcom_labibb_ocp_hw_enable(rdev); >> + if (ret) >> + return ret; >> + >> + return devm_request_threaded_irq(vreg->dev, vreg->ocp_irq, NULL, >> + qcom_labibb_ocp_isr, irq_flags, >> + ocp_irq_name, vreg); >> +} >> + >> +/* >> + * qcom_labibb_check_sc_status - Check the Short Circuit Protection status >> + * @rdev: Regulator device >> + * >> + * This function checks the STATUS1 register on both LAB and IBB regulators >> + * for the ShortCircuit bit: if it is set on *any* of them, then we have >> + * experienced a short-circuit event. >> + * >> + * Returns: Zero if there is no short-circuit, 1 if in short-circuit or >> + * negative number for error >> + */ >> +static int qcom_labibb_check_sc_status(struct labibb_regulator *vreg) >> +{ >> + u32 ibb_status, ibb_reg, lab_status, lab_reg; >> + int ret; >> + >> + /* We have to work on both regulators due to PBS... */ >> + lab_reg = ibb_reg = vreg->base; >> + if (vreg->type == QCOM_LAB_TYPE) >> + ibb_reg -= PMI8998_IBB_LAB_REG_OFFSET; > > Minus? The register comes before the base address? > #define PMI8998_LAB_REG_BASE 0xde00 #define PMI8998_IBB_REG_BASE 0xdc00 #define PMI8998_IBB_LAB_REG_OFFSET 0x200 Yeah, it actually does.. should I write this differently? I did this to avoid stunts to get the regmap base of the other regulator by storing both bases in both instances or other worst solutions. In case, have you got any idea that's cleaner than this? If so, please tell... Maybe it's stupid-level simple and my brain doesn't want to cooperate... >> + else >> + lab_reg += PMI8998_IBB_LAB_REG_OFFSET; >> + >> + ret = regmap_read(vreg->rdev->regmap, lab_reg, &lab_status); >> + if (ret) >> + return ret; >> + ret = regmap_read(vreg->rdev->regmap, ibb_reg, &ibb_status); >> + if (ret) >> + return ret; >> + >> + return !!(lab_status & LABIBB_STATUS1_SC_BIT) || >> + !!(ibb_status & LABIBB_STATUS1_SC_BIT); >> +} >> + >> +static void qcom_labibb_sc_recovery_worker(struct work_struct *work) >> +{ >> + struct labibb_regulator *vreg; >> + const struct regulator_ops *ops; >> + u32 lab_reg, ibb_reg, temp, val; >> + bool pbs_cut = false; >> + int i, sc, ret; >> + >> + vreg = container_of(work, struct labibb_regulator, >> + sc_recovery_work.work); >> + ops = vreg->rdev->desc->ops; >> + >> + /* >> + * If we tried to check the regulator status multiple times but we >> + * kept failing, then just bail out, as the Portable Batch System >> + * (PBS) will disable the vregs for us, preventing hardware damage. >> + */ >> + if (vreg->fatal_count > LABIBB_MAX_FATAL_COUNT) >> + return; >> + >> + /* Too many short-circuit events. Throw in the towel. */ >> + if (vreg->sc_count > LABIBB_MAX_SC_COUNT) >> + return; >> + >> + /* >> + * The Portable Batch System (PBS) automatically disables LAB >> + * and IBB when a short-circuit event is detected, so we have to >> + * check and work on both of them at the same time. >> + */ >> + lab_reg = ibb_reg = vreg->base; >> + if (vreg->type == QCOM_LAB_TYPE) >> + ibb_reg -= PMI8998_IBB_LAB_REG_OFFSET; >> + else >> + lab_reg += PMI8998_IBB_LAB_REG_OFFSET; >> + >> + sc = qcom_labibb_check_sc_status(vreg); >> + if (sc) >> + goto reschedule; >> + >> + for (i = 0; i < LABIBB_MAX_SC_COUNT; i++) { >> + ret = regmap_read(vreg->regmap, lab_reg, &temp); >> + if (ret) { >> + vreg->fatal_count++; >> + goto reschedule; >> + } >> + val = temp; >> + >> + ret = regmap_read(vreg->regmap, ibb_reg, &temp); >> + if (ret) { >> + vreg->fatal_count++; >> + goto reschedule; >> + } >> + val &= temp; > > Perhaps using two variables, with suitable names makes this more > maintainable? > Do you mean like this? (error checking omitted to be short) regmap_read(..., &lab_val) regmap_read(..., &ibb_val) val = lab_val & ibb_val; >> + >> + if (val & LABIBB_CONTROL_ENABLE) { > > Flip this around and do pbs_cut = true; break; in the conditional, to > not end the loop with a conditional continue and an unconditional break. > Yeah. Indeed. It makes no sense to have a loop if I appear to unconditionally break it. I don't know what I was thinking. Though, fun story: that's my brain's convoluted logic. >> + usleep_range(5000, 6000); >> + continue; >> + } >> + pbs_cut = true; >> + break; >> + } >> + if (pbs_cut) >> + goto reschedule; >> + >> + /* >> + * If we have reached this point, we either had a spurious SC IRQ >> + * or we have successfully recovered from the SC condition, which > > Wouldn't it make sense to put the "recovered from the SC condition" > before the "spurious" part in this sentence - just seems like this is > the scenario we're most likely looking for. > Brain convoluted logic strikes again. I totally agree with you: let's do it like you suggested. > Regards, > Bjorn > >> + * means that we can re-enable the regulators, if they have ever >> + * been disabled by the PBS. >> + */ >> + ret = ops->enable(vreg->rdev); >> + if (ret) >> + goto reschedule; >> + >> + /* Everything went fine: reset the OCP count! */ >> + vreg->sc_count = 0; >> + enable_irq(vreg->sc_irq); >> + return; >> + >> +reschedule: >> + /* >> + * Now that we have done basic handling of the short-circuit, >> + * reschedule this worker in the regular system workqueue, as >> + * taking action is not truly urgent anymore. >> + */ >> + vreg->sc_count++; >> + mod_delayed_work(system_wq, &vreg->sc_recovery_work, >> + msecs_to_jiffies(SC_RECOVERY_INTERVAL_MS)); >> +} >> + >> +static irqreturn_t qcom_labibb_sc_isr(int irq, void *chip) >> +{ >> + struct labibb_regulator *vreg = chip; >> + >> + if (vreg->sc_count > LABIBB_MAX_SC_COUNT) >> + return IRQ_NONE; >> + >> + /* Warn the user for short circuit */ >> + dev_warn(vreg->dev, "Short-Circuit interrupt fired!\n"); >> + >> + /* >> + * Disable the interrupt temporarily, or it will fire continuously; >> + * we will re-enable it in the recovery worker function. >> + */ >> + disable_irq(irq); >> + >> + /* Signal out of regulation event to drivers */ >> + regulator_notifier_call_chain(vreg->rdev, >> + REGULATOR_EVENT_REGULATION_OUT, NULL); >> + >> + /* Schedule the short-circuit handling as high-priority work */ >> + mod_delayed_work(system_highpri_wq, &vreg->sc_recovery_work, >> + msecs_to_jiffies(SC_RECOVERY_INTERVAL_MS)); >> + return IRQ_HANDLED; >> +} >> + >> + >> static int qcom_labibb_set_current_limit(struct regulator_dev *rdev, >> int min_uA, int max_uA) >> { >> @@ -210,6 +606,7 @@ static const struct regulator_ops qcom_labibb_ops = { >> .set_current_limit = qcom_labibb_set_current_limit, >> .get_current_limit = qcom_labibb_get_current_limit, >> .set_soft_start = qcom_labibb_set_soft_start, >> + .set_over_current_protection = qcom_labibb_set_ocp, >> }; >> >> static const struct regulator_desc pmi8998_lab_desc = { >> @@ -291,7 +688,7 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev) >> struct labibb_regulator *vreg; >> struct device *dev = &pdev->dev; >> struct regulator_config cfg = {}; >> - >> + struct device_node *reg_node; >> const struct of_device_id *match; >> const struct labibb_regulator_data *reg_data; >> struct regmap *reg_regmap; >> @@ -309,6 +706,8 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev) >> return -ENODEV; >> >> for (reg_data = match->data; reg_data->name; reg_data++) { >> + char *sc_irq_name; >> + int irq = 0; >> >> /* Validate if the type of regulator is indeed >> * what's mentioned in DT. >> @@ -331,10 +730,44 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev) >> if (!vreg) >> return -ENOMEM; >> >> + sc_irq_name = devm_kasprintf(dev, GFP_KERNEL, >> + "%s-short-circuit", >> + reg_data->name); >> + if (!sc_irq_name) >> + return -ENOMEM; >> + >> + reg_node = of_get_child_by_name(pdev->dev.of_node, >> + reg_data->name); >> + if (!reg_node) >> + return -EINVAL; >> + >> + /* The Short Circuit interrupt is critical */ >> + irq = of_irq_get_byname(reg_node, "sc-err"); >> + if (irq <= 0) { >> + if (irq == 0) >> + irq = -EINVAL; >> + >> + return dev_err_probe(vreg->dev, irq, >> + "Short-circuit irq not found.\n"); >> + } >> + vreg->sc_irq = irq; >> + >> + /* OverCurrent Protection IRQ is optional */ >> + irq = of_irq_get_byname(reg_node, "ocp"); >> + vreg->ocp_irq = irq; >> + vreg->ocp_irq_count = 0; >> + of_node_put(reg_node); >> + >> vreg->regmap = reg_regmap; >> vreg->dev = dev; >> vreg->base = reg_data->base; >> vreg->type = reg_data->type; >> + INIT_DELAYED_WORK(&vreg->sc_recovery_work, >> + qcom_labibb_sc_recovery_worker); >> + >> + if (vreg->ocp_irq > 0) >> + INIT_DELAYED_WORK(&vreg->ocp_recovery_work, >> + qcom_labibb_ocp_recovery_worker); >> >> switch (vreg->type) { >> case QCOM_LAB_TYPE: >> @@ -369,6 +802,14 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev) >> reg_data->name, ret); >> return PTR_ERR(vreg->rdev); >> } >> + >> + ret = devm_request_threaded_irq(vreg->dev, vreg->sc_irq, NULL, >> + qcom_labibb_sc_isr, >> + IRQF_ONESHOT | >> + IRQF_TRIGGER_RISING, >> + sc_irq_name, vreg); >> + if (ret) >> + return ret; >> } >> >> return 0; >> -- >> 2.29.2 >>
diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c index 38ab1eba1c59..38763625241e 100644 --- a/drivers/regulator/qcom-labibb-regulator.c +++ b/drivers/regulator/qcom-labibb-regulator.c @@ -17,8 +17,20 @@ #define PMI8998_LAB_REG_BASE 0xde00 #define PMI8998_IBB_REG_BASE 0xdc00 +#define PMI8998_IBB_LAB_REG_OFFSET 0x200 #define REG_LABIBB_STATUS1 0x08 + #define LABIBB_STATUS1_SC_BIT BIT(6) + #define LABIBB_STATUS1_VREG_OK_BIT BIT(7) + +#define REG_LABIBB_INT_SET_TYPE 0x11 +#define REG_LABIBB_INT_POLARITY_HIGH 0x12 +#define REG_LABIBB_INT_POLARITY_LOW 0x13 +#define REG_LABIBB_INT_LATCHED_CLR 0x14 +#define REG_LABIBB_INT_EN_SET 0x15 +#define REG_LABIBB_INT_EN_CLR 0x16 + #define LABIBB_INT_VREG_OK BIT(0) + #define LABIBB_INT_VREG_TYPE_LEVEL 0 #define REG_LABIBB_VOLTAGE 0x41 #define LABIBB_VOLTAGE_OVERRIDE_EN BIT(7) @@ -26,8 +38,7 @@ #define IBB_VOLTAGE_SET_MASK GENMASK(5, 0) #define REG_LABIBB_ENABLE_CTL 0x46 -#define LABIBB_STATUS1_VREG_OK_BIT BIT(7) -#define LABIBB_CONTROL_ENABLE BIT(7) + #define LABIBB_CONTROL_ENABLE BIT(7) #define REG_LABIBB_PD_CTL 0x47 #define LAB_PD_CTL_MASK GENMASK(1, 0) @@ -56,6 +67,11 @@ #define LAB_ENABLE_TIME (LABIBB_OFF_ON_DELAY * 2) #define IBB_ENABLE_TIME (LABIBB_OFF_ON_DELAY * 10) #define LABIBB_POLL_ENABLED_TIME 1000 +#define OCP_RECOVERY_INTERVAL_MS 500 +#define SC_RECOVERY_INTERVAL_MS 250 +#define LABIBB_MAX_OCP_COUNT 4 +#define LABIBB_MAX_SC_COUNT 3 +#define LABIBB_MAX_FATAL_COUNT 2 struct labibb_current_limits { u32 uA_min; @@ -69,10 +85,17 @@ struct labibb_regulator { struct regmap *regmap; struct regulator_dev *rdev; struct labibb_current_limits uA_limits; + struct delayed_work ocp_recovery_work; + struct delayed_work sc_recovery_work; u16 base; u8 type; u8 dischg_sel; u8 soft_start_sel; + int sc_irq; + int sc_count; + int ocp_irq; + int ocp_irq_count; + int fatal_count; }; struct labibb_regulator_data { @@ -82,6 +105,379 @@ struct labibb_regulator_data { const struct regulator_desc *desc; }; +static int qcom_labibb_ocp_hw_enable(struct regulator_dev *rdev) +{ + struct labibb_regulator *vreg = rdev_get_drvdata(rdev); + int ret; + + /* Clear irq latch status to avoid spurious event */ + ret = regmap_update_bits(rdev->regmap, + vreg->base + REG_LABIBB_INT_LATCHED_CLR, + LABIBB_INT_VREG_OK, 1); + if (ret) + return ret; + + /* Enable OCP HW interrupt */ + return regmap_update_bits(rdev->regmap, + vreg->base + REG_LABIBB_INT_EN_SET, + LABIBB_INT_VREG_OK, 1); +} + +static int qcom_labibb_ocp_hw_disable(struct regulator_dev *rdev) +{ + struct labibb_regulator *vreg = rdev_get_drvdata(rdev); + + return regmap_update_bits(rdev->regmap, + vreg->base + REG_LABIBB_INT_EN_CLR, + LABIBB_INT_VREG_OK, 1); +} + +/* + * qcom_labibb_check_ocp_status - Check the Over-Current Protection status + * @rdev: Regulator device + * + * This function checks the STATUS1 register for the VREG_OK bit: if it is + * set, then there is no Over-Current event. + * + * Returns: Zero if there is no over-current, 1 if in over-current or + * negative number for error + */ +static int qcom_labibb_check_ocp_status(struct labibb_regulator *vreg) +{ + u32 cur_status; + int ret; + + ret = regmap_read(vreg->rdev->regmap, vreg->base + REG_LABIBB_STATUS1, + &cur_status); + if (ret) + return ret; + + return !(cur_status & LABIBB_STATUS1_VREG_OK_BIT); +} + +static void qcom_labibb_ocp_recovery_worker(struct work_struct *work) +{ + struct labibb_regulator *vreg; + const struct regulator_ops *ops; + int ret; + + vreg = container_of(work, struct labibb_regulator, + ocp_recovery_work.work); + ops = vreg->rdev->desc->ops; + + if (vreg->ocp_irq_count >= LABIBB_MAX_OCP_COUNT) { + /* + * If we tried to disable the regulator multiple times but + * we kept failing, there's only one last hope to save our + * hardware from the death: raise a kernel bug, reboot and + * hope that the bootloader kindly saves us. This, though + * is done only as paranoid checking, because failing the + * regmap write to disable the vreg is almost impossible, + * since we got here after multiple regmap R/W. + */ + BUG_ON(vreg->fatal_count > LABIBB_MAX_FATAL_COUNT); + dev_err(&vreg->rdev->dev, "LABIBB: CRITICAL: Disabling regulator\n"); + + /* Disable the regulator immediately to avoid damage */ + ret = ops->disable(vreg->rdev); + if (ret) { + vreg->fatal_count++; + goto reschedule; + } + enable_irq(vreg->ocp_irq); + vreg->fatal_count = 0; + return; + } + + ret = qcom_labibb_check_ocp_status(vreg); + if (ret != 0) { + vreg->ocp_irq_count++; + goto reschedule; + } + + ret = qcom_labibb_ocp_hw_enable(vreg->rdev); + if (ret) { + /* We cannot trust it without OCP enabled. */ + dev_err(vreg->dev, "Cannot enable OCP IRQ\n"); + vreg->ocp_irq_count++; + goto reschedule; + } + + enable_irq(vreg->ocp_irq); + /* Everything went fine: reset the OCP count! */ + vreg->ocp_irq_count = 0; + return; + +reschedule: + mod_delayed_work(system_wq, &vreg->ocp_recovery_work, + msecs_to_jiffies(OCP_RECOVERY_INTERVAL_MS)); +} + +static irqreturn_t qcom_labibb_ocp_isr(int irq, void *chip) +{ + struct labibb_regulator *vreg = chip; + const struct regulator_ops *ops = vreg->rdev->desc->ops; + int ret; + + /* If the regulator is not enabled, this is a fake event */ + if (!ops->is_enabled(vreg->rdev)) + return 0; + + /* If we tried to recover for too many times it's not getting better */ + if (vreg->ocp_irq_count > LABIBB_MAX_OCP_COUNT) + return IRQ_NONE; + + /* + * If we (unlikely) can't read this register, to prevent hardware + * damage at all costs, we assume that the overcurrent event was + * real; Moreover, if the status register is not signaling OCP, + * it was a spurious event, so it's all ok. + */ + ret = qcom_labibb_check_ocp_status(vreg); + if (ret == 0) { + vreg->ocp_irq_count = 0; + goto end; + } + vreg->ocp_irq_count++; + + /* + * Disable the interrupt temporarily, or it will fire continuously; + * we will re-enable it in the recovery worker function. + */ + disable_irq(irq); + + /* Warn the user for overcurrent */ + dev_warn(vreg->dev, "Over-Current interrupt fired!\n"); + + /* Disable the interrupt to avoid hogging */ + ret = qcom_labibb_ocp_hw_disable(vreg->rdev); + if (ret) + goto end; + + /* Signal overcurrent event to drivers */ + regulator_notifier_call_chain(vreg->rdev, + REGULATOR_EVENT_OVER_CURRENT, NULL); + +end: + /* Schedule the recovery work */ + schedule_delayed_work(&vreg->ocp_recovery_work, + msecs_to_jiffies(OCP_RECOVERY_INTERVAL_MS)); + if (ret) + return IRQ_NONE; + + return IRQ_HANDLED; +} + +static int qcom_labibb_set_ocp(struct regulator_dev *rdev) +{ + struct labibb_regulator *vreg = rdev_get_drvdata(rdev); + char *ocp_irq_name; + u32 irq_flags = IRQF_ONESHOT; + int irq_trig_low, ret; + + /* If there is no OCP interrupt, there's nothing to set */ + if (vreg->ocp_irq <= 0) + return -EINVAL; + + ocp_irq_name = devm_kasprintf(vreg->dev, GFP_KERNEL, "%s-over-current", + vreg->desc.name); + if (!ocp_irq_name) + return -ENOMEM; + + /* IRQ polarities - LAB: trigger-low, IBB: trigger-high */ + switch (vreg->type) { + case QCOM_LAB_TYPE: + irq_flags |= IRQF_TRIGGER_LOW; + irq_trig_low = 1; + break; + case QCOM_IBB_TYPE: + irq_flags |= IRQF_TRIGGER_HIGH; + irq_trig_low = 0; + break; + default: + return -EINVAL; + } + + /* Activate OCP HW level interrupt */ + ret = regmap_update_bits(rdev->regmap, + vreg->base + REG_LABIBB_INT_SET_TYPE, + LABIBB_INT_VREG_OK, + LABIBB_INT_VREG_TYPE_LEVEL); + if (ret) + return ret; + + /* Set OCP interrupt polarity */ + ret = regmap_update_bits(rdev->regmap, + vreg->base + REG_LABIBB_INT_POLARITY_HIGH, + LABIBB_INT_VREG_OK, !irq_trig_low); + if (ret) + return ret; + ret = regmap_update_bits(rdev->regmap, + vreg->base + REG_LABIBB_INT_POLARITY_LOW, + LABIBB_INT_VREG_OK, irq_trig_low); + if (ret) + return ret; + + ret = qcom_labibb_ocp_hw_enable(rdev); + if (ret) + return ret; + + return devm_request_threaded_irq(vreg->dev, vreg->ocp_irq, NULL, + qcom_labibb_ocp_isr, irq_flags, + ocp_irq_name, vreg); +} + +/* + * qcom_labibb_check_sc_status - Check the Short Circuit Protection status + * @rdev: Regulator device + * + * This function checks the STATUS1 register on both LAB and IBB regulators + * for the ShortCircuit bit: if it is set on *any* of them, then we have + * experienced a short-circuit event. + * + * Returns: Zero if there is no short-circuit, 1 if in short-circuit or + * negative number for error + */ +static int qcom_labibb_check_sc_status(struct labibb_regulator *vreg) +{ + u32 ibb_status, ibb_reg, lab_status, lab_reg; + int ret; + + /* We have to work on both regulators due to PBS... */ + lab_reg = ibb_reg = vreg->base; + if (vreg->type == QCOM_LAB_TYPE) + ibb_reg -= PMI8998_IBB_LAB_REG_OFFSET; + else + lab_reg += PMI8998_IBB_LAB_REG_OFFSET; + + ret = regmap_read(vreg->rdev->regmap, lab_reg, &lab_status); + if (ret) + return ret; + ret = regmap_read(vreg->rdev->regmap, ibb_reg, &ibb_status); + if (ret) + return ret; + + return !!(lab_status & LABIBB_STATUS1_SC_BIT) || + !!(ibb_status & LABIBB_STATUS1_SC_BIT); +} + +static void qcom_labibb_sc_recovery_worker(struct work_struct *work) +{ + struct labibb_regulator *vreg; + const struct regulator_ops *ops; + u32 lab_reg, ibb_reg, temp, val; + bool pbs_cut = false; + int i, sc, ret; + + vreg = container_of(work, struct labibb_regulator, + sc_recovery_work.work); + ops = vreg->rdev->desc->ops; + + /* + * If we tried to check the regulator status multiple times but we + * kept failing, then just bail out, as the Portable Batch System + * (PBS) will disable the vregs for us, preventing hardware damage. + */ + if (vreg->fatal_count > LABIBB_MAX_FATAL_COUNT) + return; + + /* Too many short-circuit events. Throw in the towel. */ + if (vreg->sc_count > LABIBB_MAX_SC_COUNT) + return; + + /* + * The Portable Batch System (PBS) automatically disables LAB + * and IBB when a short-circuit event is detected, so we have to + * check and work on both of them at the same time. + */ + lab_reg = ibb_reg = vreg->base; + if (vreg->type == QCOM_LAB_TYPE) + ibb_reg -= PMI8998_IBB_LAB_REG_OFFSET; + else + lab_reg += PMI8998_IBB_LAB_REG_OFFSET; + + sc = qcom_labibb_check_sc_status(vreg); + if (sc) + goto reschedule; + + for (i = 0; i < LABIBB_MAX_SC_COUNT; i++) { + ret = regmap_read(vreg->regmap, lab_reg, &temp); + if (ret) { + vreg->fatal_count++; + goto reschedule; + } + val = temp; + + ret = regmap_read(vreg->regmap, ibb_reg, &temp); + if (ret) { + vreg->fatal_count++; + goto reschedule; + } + val &= temp; + + if (val & LABIBB_CONTROL_ENABLE) { + usleep_range(5000, 6000); + continue; + } + pbs_cut = true; + break; + } + if (pbs_cut) + goto reschedule; + + /* + * If we have reached this point, we either had a spurious SC IRQ + * or we have successfully recovered from the SC condition, which + * means that we can re-enable the regulators, if they have ever + * been disabled by the PBS. + */ + ret = ops->enable(vreg->rdev); + if (ret) + goto reschedule; + + /* Everything went fine: reset the OCP count! */ + vreg->sc_count = 0; + enable_irq(vreg->sc_irq); + return; + +reschedule: + /* + * Now that we have done basic handling of the short-circuit, + * reschedule this worker in the regular system workqueue, as + * taking action is not truly urgent anymore. + */ + vreg->sc_count++; + mod_delayed_work(system_wq, &vreg->sc_recovery_work, + msecs_to_jiffies(SC_RECOVERY_INTERVAL_MS)); +} + +static irqreturn_t qcom_labibb_sc_isr(int irq, void *chip) +{ + struct labibb_regulator *vreg = chip; + + if (vreg->sc_count > LABIBB_MAX_SC_COUNT) + return IRQ_NONE; + + /* Warn the user for short circuit */ + dev_warn(vreg->dev, "Short-Circuit interrupt fired!\n"); + + /* + * Disable the interrupt temporarily, or it will fire continuously; + * we will re-enable it in the recovery worker function. + */ + disable_irq(irq); + + /* Signal out of regulation event to drivers */ + regulator_notifier_call_chain(vreg->rdev, + REGULATOR_EVENT_REGULATION_OUT, NULL); + + /* Schedule the short-circuit handling as high-priority work */ + mod_delayed_work(system_highpri_wq, &vreg->sc_recovery_work, + msecs_to_jiffies(SC_RECOVERY_INTERVAL_MS)); + return IRQ_HANDLED; +} + + static int qcom_labibb_set_current_limit(struct regulator_dev *rdev, int min_uA, int max_uA) { @@ -210,6 +606,7 @@ static const struct regulator_ops qcom_labibb_ops = { .set_current_limit = qcom_labibb_set_current_limit, .get_current_limit = qcom_labibb_get_current_limit, .set_soft_start = qcom_labibb_set_soft_start, + .set_over_current_protection = qcom_labibb_set_ocp, }; static const struct regulator_desc pmi8998_lab_desc = { @@ -291,7 +688,7 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev) struct labibb_regulator *vreg; struct device *dev = &pdev->dev; struct regulator_config cfg = {}; - + struct device_node *reg_node; const struct of_device_id *match; const struct labibb_regulator_data *reg_data; struct regmap *reg_regmap; @@ -309,6 +706,8 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev) return -ENODEV; for (reg_data = match->data; reg_data->name; reg_data++) { + char *sc_irq_name; + int irq = 0; /* Validate if the type of regulator is indeed * what's mentioned in DT. @@ -331,10 +730,44 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev) if (!vreg) return -ENOMEM; + sc_irq_name = devm_kasprintf(dev, GFP_KERNEL, + "%s-short-circuit", + reg_data->name); + if (!sc_irq_name) + return -ENOMEM; + + reg_node = of_get_child_by_name(pdev->dev.of_node, + reg_data->name); + if (!reg_node) + return -EINVAL; + + /* The Short Circuit interrupt is critical */ + irq = of_irq_get_byname(reg_node, "sc-err"); + if (irq <= 0) { + if (irq == 0) + irq = -EINVAL; + + return dev_err_probe(vreg->dev, irq, + "Short-circuit irq not found.\n"); + } + vreg->sc_irq = irq; + + /* OverCurrent Protection IRQ is optional */ + irq = of_irq_get_byname(reg_node, "ocp"); + vreg->ocp_irq = irq; + vreg->ocp_irq_count = 0; + of_node_put(reg_node); + vreg->regmap = reg_regmap; vreg->dev = dev; vreg->base = reg_data->base; vreg->type = reg_data->type; + INIT_DELAYED_WORK(&vreg->sc_recovery_work, + qcom_labibb_sc_recovery_worker); + + if (vreg->ocp_irq > 0) + INIT_DELAYED_WORK(&vreg->ocp_recovery_work, + qcom_labibb_ocp_recovery_worker); switch (vreg->type) { case QCOM_LAB_TYPE: @@ -369,6 +802,14 @@ static int qcom_labibb_regulator_probe(struct platform_device *pdev) reg_data->name, ret); return PTR_ERR(vreg->rdev); } + + ret = devm_request_threaded_irq(vreg->dev, vreg->sc_irq, NULL, + qcom_labibb_sc_isr, + IRQF_ONESHOT | + IRQF_TRIGGER_RISING, + sc_irq_name, vreg); + if (ret) + return ret; } return 0;
Short-Circuit Protection (SCP) and Over-Current Protection (OCP) are very important for regulators like LAB and IBB, which are designed to provide from very small to relatively big amounts of current to the device (normally, a display). Now that this regulator supports both voltage setting and current limiting in this driver, to me it looked like being somehow essential to provide support for SCP and OCP, for two reasons: 1. SCP is a drastic measure to prevent damaging "more" hardware in the worst situations, if any was damaged, preventing potentially drastic issues; 2. OCP is a great way to protect the hardware that we're powering through these regulators as if anything bad happens, the HW will draw more current than expected: in this case, the OCP interrupt will fire and the regulators will be immediately shut down, preventing hardware damage in many cases. Both interrupts were successfully tested in a "sort-of" controlled manner, with the following methodology: Short-Circuit Protection (SCP): 1. Set LAB/IBB to 4.6/-1.4V, current limit 200mA/50mA; 2. Connect a 10 KOhm resistor to LAB/IBB by poking the right traces on a FxTec Pro1 smartphone for a very brief time (in short words, "just a rapid touch with flying wires"); 3. The Short-Circuit protection trips: IRQ raises, regulators get cut. Recovery OK, test repeated without rebooting, OK. Over-Current Protection (OCP): 1. Set LAB/IBB to the expected voltage to power up the display of a Sony Xperia XZ Premium smartphone (Sharp LS055D1SX04), set current limit to LAB 200mA, IBB 50mA (the values that this display unit needs are 200/800mA); 2. Boot the kernel: OCP fires. Recovery never happens because the selected current limit is too low, but that's expected. Test OK. 3. Set LAB/IBB to the expected current limits for XZ Premium (LAB 200mA, IBB 800mA), but lower than expected voltage, specifically LAB 5.4V, IBB -5.6V (instead of 5.6, -5.8V); 4. Boot the kernel: OCP fires. Recovery never happens because the selected voltage (still in the working range limits) is producing a current draw of more than 200mA on LAB. Test OK. Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> --- drivers/regulator/qcom-labibb-regulator.c | 447 +++++++++++++++++++++- 1 file changed, 444 insertions(+), 3 deletions(-)