Re: [PATCH v2 5/7] regulator: qcom-labibb: Implement short-circuit and over-current IRQs
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 --- drivers/regulator/qcom-labibb-regulator.c | 447 +- 1 file changed, 444 insertions(+), 3 deletions(-) diff --git a/drivers/regulator/qcom-labibb-regulator.c
Re: [PATCH v2 5/7] regulator: qcom-labibb: Implement short-circuit and over-current IRQs
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
>
> ---
> 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_SET0x15
> +#define REG_LABIBB_INT_EN_CLR0x16
> + #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_MASKGENMASK(5, 0)
>
> #define REG_LABIBB_ENABLE_CTL0x46
> -#define LABIBB_STATUS1_VREG_OK_BIT BIT(7)
> -#define LABIBB_CONTROL_ENABLEBIT(7)
> + #define LABIBB_CONTROL_ENABLE BIT(7)
>
> #define REG_LABIBB_PD_CTL0x47
> #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_limitsuA_limits;
> + struct delayed_work ocp_recovery_work;
