| Message ID | 20200602100924.26256-1-sumit.semwal@linaro.org |
|---|---|
| Headers | show |
| Series | Qualcomm labibb regulator driver | expand |
On Tue, Jun 02, 2020 at 03:39:20PM +0530, Sumit Semwal wrote: > + > + if (time_remaining <= 0) { > + rdev_err(rdev, "Enabled check failed.\n"); > + return -ETIMEDOUT; s/failed/timed out/ > + * @poll_enabled_time: Maximum time (in uS) to poll if the regulator is > + * actually enabled, after enable() call > + * This comment needs updating to reflect the new implementation.
Hi Mark, On Tue, 2 Jun 2020 at 17:52, Mark Brown <broonie@kernel.org> 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.
On Wed, Jun 17, 2020 at 05:36:43PM +0530, Sumit Semwal wrote: > On Tue, 2 Jun 2020 at 17:52, Mark Brown <broonie@kernel.org> 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?
On Tue 02 Jun 03:09 PDT 2020, Sumit Semwal wrote: > Some regulators might need to verify that they have indeed been enabled > after the enable() call is made and enable_time delay has passed. > > This is implemented by repeatedly checking is_enabled() upto > poll_enabled_time, waiting for the already calculated enable delay in > each iteration. > > Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org> > -- > v2: Address review comments, including swapping enable_time and poll_enabled_time. > --- > drivers/regulator/core.c | 58 +++++++++++++++++++++++++++++++- > include/linux/regulator/driver.h | 5 +++ > 2 files changed, 62 insertions(+), 1 deletion(-) > > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c > index 7486f6e4e613..d9ab888da95f 100644 > --- a/drivers/regulator/core.c > +++ b/drivers/regulator/core.c > @@ -2347,6 +2347,32 @@ static void _regulator_enable_delay(unsigned int delay) > udelay(us); > } > > +/* _regulator_check_status_enabled Please make all your kerneldoc follow: https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation > + * > + * returns: > + * 1 if status shows regulator is in enabled state > + * 0 if not enabled state > + * else, error value as received from ops->get_status() > + */ > +static inline int _regulator_check_status_enabled(struct regulator_dev *rdev) > +{ > + int ret = rdev->desc->ops->get_status(rdev); > + > + if (ret < 0) { > + rdev_info(rdev, "get_status returned error: %d\n", ret); > + return ret; > + } > + > + switch (ret) { > + case REGULATOR_STATUS_OFF: > + case REGULATOR_STATUS_ERROR: > + case REGULATOR_STATUS_UNDEFINED: > + return 0; > + default: > + return 1; > + } > +} > + > static int _regulator_do_enable(struct regulator_dev *rdev) > { > int ret, delay; > @@ -2407,7 +2433,37 @@ static int _regulator_do_enable(struct regulator_dev *rdev) > * together. */ > trace_regulator_enable_delay(rdev_get_name(rdev)); > > - _regulator_enable_delay(delay); > + /* If poll_enabled_time is set, poll upto the delay calculated > + * above, delaying poll_enabled_time uS to check if the regulator > + * actually got enabled. > + * If the regulator isn't enabled after enable_delay has > + * expired, return -ETIMEDOUT. > + */ > + if (rdev->desc->poll_enabled_time) { > + unsigned int time_remaining = delay; > + > + while (time_remaining > 0) { > + _regulator_enable_delay(rdev->desc->poll_enabled_time); > + > + if (rdev->desc->ops->get_status) { > + ret = _regulator_check_status_enabled(rdev); > + if (ret < 0) > + return ret; > + else if (ret) > + break; > + } else if (rdev->desc->ops->is_enabled(rdev)) > + break; > + > + time_remaining -= rdev->desc->poll_enabled_time; > + } > + > + if (time_remaining <= 0) { > + rdev_err(rdev, "Enabled check failed.\n"); > + return -ETIMEDOUT; > + } > + } else { > + _regulator_enable_delay(delay); > + } > > trace_regulator_enable_complete(rdev_get_name(rdev)); > > diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h > index 29d920516e0b..bb50e943010f 100644 > --- a/include/linux/regulator/driver.h > +++ b/include/linux/regulator/driver.h > @@ -322,6 +322,9 @@ enum regulator_type { > * @enable_time: Time taken for initial enable of regulator (in uS). > * @off_on_delay: guard time (in uS), before re-enabling a regulator > * > + * @poll_enabled_time: Maximum time (in uS) to poll if the regulator is > + * actually enabled, after enable() call I read this as "how long should we stay in the poll loop". I think it would be better describes as something like "The polling interval to use while checking that the regulator was actually enabled". Regards, Bjorn > + * > * @of_map_mode: Maps a hardware mode defined in a DeviceTree to a standard mode > */ > struct regulator_desc { > @@ -389,6 +392,8 @@ struct regulator_desc { > > unsigned int off_on_delay; > > + unsigned int poll_enabled_time; > + > unsigned int (*of_map_mode)(unsigned int mode); > }; > > -- > 2.26.2 >