* Re: [PATCH] iio: adc: stm32-adc: fix runtime autosuspend delay when slow polling [not found] <1579854369-7972-1-git-send-email-fabrice.gasnier@st.com> @ 2020-02-02 15:33 ` Jonathan Cameron 2020-02-12 14:02 ` Fabrice Gasnier 0 siblings, 1 reply; 3+ messages in thread From: Jonathan Cameron @ 2020-02-02 15:33 UTC (permalink / raw) To: Fabrice Gasnier Cc: linux-arm-kernel, linux-kernel, mcoquelin.stm32, alexandre.torgue, olivier.moysan, linux-iio, lars, knaack.h, pmeerw, linux-stm32, Rafael J. Wysocki, linux-pm On Fri, 24 Jan 2020 09:26:09 +0100 Fabrice Gasnier <fabrice.gasnier@st.com> wrote: > When the ADC is runtime suspended and starting a conversion, the stm32-adc > driver calls pm_runtime_get_sync() that gets cascaded to the parent > (e.g. runtime resume of stm32-adc-core driver). This also kicks the > autosuspend delay (e.g. 2s) of the parent. > Once the ADC is active, calling pm_runtime_get_sync() again (upon a new > capture) won't kick the autosuspend delay for the parent (stm32-adc-core > driver) as already active. > > Currently, this makes the stm32-adc-core driver go in suspend state > every 2s when doing slow polling. As an example, doing a capture, e.g. > cat in_voltageY_raw at a 0.2s rate, the auto suspend delay for the parent > isn't refreshed. Once it expires, the parent immediately falls into > runtime suspended state, in between two captures, as soon as the child > driver falls into runtime suspend state: > - e.g. after 2s, + child calls pm_runtime_put_autosuspend() + 100ms > autosuspend delay of the child. > - stm32-adc-core switches off regulators, clocks and so on. > - They get switched on back again 100ms later in this example (at 2.2s). > > So, add an explicit call to pm_runtime_mark_last_busy() for the parent > driver (stm32-adc-core), synchronously with the child driver (stm32-adc), > to avoid this. > > Fixes: 9bdbb1139ca1 ("iio: adc: stm32-adc: add power management support") > > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> Hi Fabrice, Whilst this will clearly work, it seems like a somewhat adhoc solution. Power management specialists (cc'd): Is this what we should be doing, or have Fabrice and I both missed something that we should be doing here? Thanks, Jonathan > --- > drivers/iio/adc/stm32-adc.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c > index 3b291d7..670157e 100644 > --- a/drivers/iio/adc/stm32-adc.c > +++ b/drivers/iio/adc/stm32-adc.c > @@ -1157,6 +1157,7 @@ static int stm32_adc_single_conv(struct iio_dev *indio_dev, > > stm32_adc_conv_irq_disable(adc); > > + pm_runtime_mark_last_busy(dev->parent); > pm_runtime_mark_last_busy(dev); > pm_runtime_put_autosuspend(dev); > > @@ -1278,6 +1279,7 @@ static int stm32_adc_update_scan_mode(struct iio_dev *indio_dev, > adc->num_conv = bitmap_weight(scan_mask, indio_dev->masklength); > > ret = stm32_adc_conf_scan_seq(indio_dev, scan_mask); > + pm_runtime_mark_last_busy(dev->parent); > pm_runtime_mark_last_busy(dev); > pm_runtime_put_autosuspend(dev); > > @@ -1329,6 +1331,7 @@ static int stm32_adc_debugfs_reg_access(struct iio_dev *indio_dev, > else > *readval = stm32_adc_readl(adc, reg); > > + pm_runtime_mark_last_busy(dev->parent); > pm_runtime_mark_last_busy(dev); > pm_runtime_put_autosuspend(dev); > > @@ -1451,6 +1454,7 @@ static int __stm32_adc_buffer_postenable(struct iio_dev *indio_dev) > err_clr_trig: > stm32_adc_set_trig(indio_dev, NULL); > err_pm_put: > + pm_runtime_mark_last_busy(dev->parent); > pm_runtime_mark_last_busy(dev); > pm_runtime_put_autosuspend(dev); > > @@ -1487,6 +1491,7 @@ static void __stm32_adc_buffer_predisable(struct iio_dev *indio_dev) > if (stm32_adc_set_trig(indio_dev, NULL)) > dev_err(&indio_dev->dev, "Can't clear trigger\n"); > > + pm_runtime_mark_last_busy(dev->parent); > pm_runtime_mark_last_busy(dev); > pm_runtime_put_autosuspend(dev); > } > @@ -1874,6 +1879,7 @@ static int stm32_adc_probe(struct platform_device *pdev) > goto err_hw_stop; > } > > + pm_runtime_mark_last_busy(dev->parent); > pm_runtime_mark_last_busy(dev); > pm_runtime_put_autosuspend(dev); > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] iio: adc: stm32-adc: fix runtime autosuspend delay when slow polling 2020-02-02 15:33 ` [PATCH] iio: adc: stm32-adc: fix runtime autosuspend delay when slow polling Jonathan Cameron @ 2020-02-12 14:02 ` Fabrice Gasnier 2020-03-06 8:22 ` Fabrice Gasnier 0 siblings, 1 reply; 3+ messages in thread From: Fabrice Gasnier @ 2020-02-12 14:02 UTC (permalink / raw) To: Jonathan Cameron, Rafael J. Wysocki Cc: linux-arm-kernel, linux-kernel, mcoquelin.stm32, alexandre.torgue, olivier.moysan, linux-iio, lars, knaack.h, pmeerw, linux-stm32, linux-pm, ulf.hansson, khilman On 2/2/20 4:33 PM, Jonathan Cameron wrote: > On Fri, 24 Jan 2020 09:26:09 +0100 > Fabrice Gasnier <fabrice.gasnier@st.com> wrote: > >> When the ADC is runtime suspended and starting a conversion, the stm32-adc >> driver calls pm_runtime_get_sync() that gets cascaded to the parent >> (e.g. runtime resume of stm32-adc-core driver). This also kicks the >> autosuspend delay (e.g. 2s) of the parent. >> Once the ADC is active, calling pm_runtime_get_sync() again (upon a new >> capture) won't kick the autosuspend delay for the parent (stm32-adc-core >> driver) as already active. >> >> Currently, this makes the stm32-adc-core driver go in suspend state >> every 2s when doing slow polling. As an example, doing a capture, e.g. >> cat in_voltageY_raw at a 0.2s rate, the auto suspend delay for the parent >> isn't refreshed. Once it expires, the parent immediately falls into >> runtime suspended state, in between two captures, as soon as the child >> driver falls into runtime suspend state: >> - e.g. after 2s, + child calls pm_runtime_put_autosuspend() + 100ms >> autosuspend delay of the child. >> - stm32-adc-core switches off regulators, clocks and so on. >> - They get switched on back again 100ms later in this example (at 2.2s). >> >> So, add an explicit call to pm_runtime_mark_last_busy() for the parent >> driver (stm32-adc-core), synchronously with the child driver (stm32-adc), >> to avoid this. >> >> Fixes: 9bdbb1139ca1 ("iio: adc: stm32-adc: add power management support") >> >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> > > Hi Fabrice, > > Whilst this will clearly work, it seems like a somewhat adhoc solution. > Power management specialists (cc'd): Is this what we should be doing, or > have Fabrice and I both missed something that we should be doing here? Hi all, PM specialists, As per my understanding, pm_runtime_mark_last_busy() doesn't cascade to the parent device: - in pm_runtime.h: static inline void pm_runtime_mark_last_busy(struct device *dev) { WRITE_ONCE(dev->power.last_busy, ktime_get_mono_fast_ns()); } STM32 ADC driver uses a model with an autosupsend delay for - a parent driver to handle common resources, registers etc. - child drivers for each ADC. So the question is on how to fix the behavior I described: 1: Child activity with "short" autosuspend_delay 2: Parent activity with "longer" autosuspend_delay _ _ _ _ _ _ _ _ 1: _| |___| |___| |___| |___| |_..._| |___| |___| |_... v v v v v v v v v v ... v v v v v v | | | | | +- pm_runtime_mark_last_busy() | | | +- pm_runtime_put_autosuspend() v | | | | +--- pm_runtime_get_sync() | v | | | +---> expires after autosuspend_delay | | | | | v v v _______________________________________ ___... 2: _| ... |___| Glitches on parent dev near autosuspend_delay ^ - does the child driver needs to "kick" parent driver with pm_runtime_mark_last_busy(), as proposed in current patch ? - or is it something that should be done by PM runtime core routines ? e.g. make pm_runtime_mark_last_busy() recursive or something else ? Please advise Best regards, Fabrice > > Thanks, > > Jonathan > >> --- >> drivers/iio/adc/stm32-adc.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c >> index 3b291d7..670157e 100644 >> --- a/drivers/iio/adc/stm32-adc.c >> +++ b/drivers/iio/adc/stm32-adc.c >> @@ -1157,6 +1157,7 @@ static int stm32_adc_single_conv(struct iio_dev *indio_dev, >> >> stm32_adc_conv_irq_disable(adc); >> >> + pm_runtime_mark_last_busy(dev->parent); >> pm_runtime_mark_last_busy(dev); >> pm_runtime_put_autosuspend(dev); >> >> @@ -1278,6 +1279,7 @@ static int stm32_adc_update_scan_mode(struct iio_dev *indio_dev, >> adc->num_conv = bitmap_weight(scan_mask, indio_dev->masklength); >> >> ret = stm32_adc_conf_scan_seq(indio_dev, scan_mask); >> + pm_runtime_mark_last_busy(dev->parent); >> pm_runtime_mark_last_busy(dev); >> pm_runtime_put_autosuspend(dev); >> >> @@ -1329,6 +1331,7 @@ static int stm32_adc_debugfs_reg_access(struct iio_dev *indio_dev, >> else >> *readval = stm32_adc_readl(adc, reg); >> >> + pm_runtime_mark_last_busy(dev->parent); >> pm_runtime_mark_last_busy(dev); >> pm_runtime_put_autosuspend(dev); >> >> @@ -1451,6 +1454,7 @@ static int __stm32_adc_buffer_postenable(struct iio_dev *indio_dev) >> err_clr_trig: >> stm32_adc_set_trig(indio_dev, NULL); >> err_pm_put: >> + pm_runtime_mark_last_busy(dev->parent); >> pm_runtime_mark_last_busy(dev); >> pm_runtime_put_autosuspend(dev); >> >> @@ -1487,6 +1491,7 @@ static void __stm32_adc_buffer_predisable(struct iio_dev *indio_dev) >> if (stm32_adc_set_trig(indio_dev, NULL)) >> dev_err(&indio_dev->dev, "Can't clear trigger\n"); >> >> + pm_runtime_mark_last_busy(dev->parent); >> pm_runtime_mark_last_busy(dev); >> pm_runtime_put_autosuspend(dev); >> } >> @@ -1874,6 +1879,7 @@ static int stm32_adc_probe(struct platform_device *pdev) >> goto err_hw_stop; >> } >> >> + pm_runtime_mark_last_busy(dev->parent); >> pm_runtime_mark_last_busy(dev); >> pm_runtime_put_autosuspend(dev); >> > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] iio: adc: stm32-adc: fix runtime autosuspend delay when slow polling 2020-02-12 14:02 ` Fabrice Gasnier @ 2020-03-06 8:22 ` Fabrice Gasnier 0 siblings, 0 replies; 3+ messages in thread From: Fabrice Gasnier @ 2020-03-06 8:22 UTC (permalink / raw) To: Jonathan Cameron, Rafael J. Wysocki, ulf.hansson Cc: linux-arm-kernel, linux-kernel, mcoquelin.stm32, alexandre.torgue, olivier.moysan, linux-iio, lars, knaack.h, pmeerw, linux-stm32, linux-pm, khilman, Vincent Guittot, Benjamin GAIGNARD On 2/12/20 3:02 PM, Fabrice Gasnier wrote: > On 2/2/20 4:33 PM, Jonathan Cameron wrote: >> On Fri, 24 Jan 2020 09:26:09 +0100 >> Fabrice Gasnier <fabrice.gasnier@st.com> wrote: >> >>> When the ADC is runtime suspended and starting a conversion, the stm32-adc >>> driver calls pm_runtime_get_sync() that gets cascaded to the parent >>> (e.g. runtime resume of stm32-adc-core driver). This also kicks the >>> autosuspend delay (e.g. 2s) of the parent. >>> Once the ADC is active, calling pm_runtime_get_sync() again (upon a new >>> capture) won't kick the autosuspend delay for the parent (stm32-adc-core >>> driver) as already active. >>> >>> Currently, this makes the stm32-adc-core driver go in suspend state >>> every 2s when doing slow polling. As an example, doing a capture, e.g. >>> cat in_voltageY_raw at a 0.2s rate, the auto suspend delay for the parent >>> isn't refreshed. Once it expires, the parent immediately falls into >>> runtime suspended state, in between two captures, as soon as the child >>> driver falls into runtime suspend state: >>> - e.g. after 2s, + child calls pm_runtime_put_autosuspend() + 100ms >>> autosuspend delay of the child. >>> - stm32-adc-core switches off regulators, clocks and so on. >>> - They get switched on back again 100ms later in this example (at 2.2s). >>> >>> So, add an explicit call to pm_runtime_mark_last_busy() for the parent >>> driver (stm32-adc-core), synchronously with the child driver (stm32-adc), >>> to avoid this. >>> >>> Fixes: 9bdbb1139ca1 ("iio: adc: stm32-adc: add power management support") >>> >>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> >> >> Hi Fabrice, >> >> Whilst this will clearly work, it seems like a somewhat adhoc solution. >> Power management specialists (cc'd): Is this what we should be doing, or >> have Fabrice and I both missed something that we should be doing here? > > Hi all, PM specialists, > > As per my understanding, pm_runtime_mark_last_busy() doesn't cascade to > the parent device: > > - in pm_runtime.h: > static inline void pm_runtime_mark_last_busy(struct device *dev) > { > WRITE_ONCE(dev->power.last_busy, ktime_get_mono_fast_ns()); > } Hi all, PM specialists, Gentle reminder on this topic. There maybe several solutions to address this. Not sure what would be an acceptable / preferred fix ? Just to summarize: --- A - Current post: I'm wondering if this is suitable to call pm_runtime_mark_last_busy() every time, for the parent device from child driver ? --- B - Is it suitable to update the PM runtime, so it's recursive, to refresh "last_busy" for the parent(s) ? e.g. something like: static inline void pm_runtime_mark_last_busy(struct device *dev) { WRITE_ONCE(dev->power.last_busy, ktime_get_mono_fast_ns()); + if (dev->parent) + pm_runtime_mark_last_busy(dev->parent); } --- C - I find few drivers that rely on the rmp_idle() callback in the parent to call pm_runtime_mark_last_busy(), like: -> dwc3_runtime_idle() -> gb_bundle_idle() -> that may lead in the stm32-adc-core driver to add: +static int stm32_adc_core_runtime_idle(struct device *dev) +{ + pm_runtime_mark_last_busy(dev); + + return 0; +} static const struct dev_pm_ops stm32_adc_core_pm_ops = { SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume) SET_RUNTIME_PM_OPS(stm32_adc_core_runtime_suspend, stm32_adc_core_runtime_resume, - NULL) + stm32_adc_core_runtime_idle) }; I've test all these solutions. Would you have some recommendations ? Please advise, Best regards, Fabrice > > STM32 ADC driver uses a model with an autosupsend delay for > - a parent driver to handle common resources, registers etc. > - child drivers for each ADC. > > So the question is on how to fix the behavior I described: > 1: Child activity with "short" autosuspend_delay > 2: Parent activity with "longer" autosuspend_delay > _ _ _ _ _ _ _ _ > 1: _| |___| |___| |___| |___| |_..._| |___| |___| |_... > > v v v v v v v v v v ... v v v v v v > | | | | > | +- pm_runtime_mark_last_busy() | | > | +- pm_runtime_put_autosuspend() v | > | | | > +--- pm_runtime_get_sync() | v > | | | > +---> expires after autosuspend_delay | | > | | | > v v v > _______________________________________ ___... > 2: _| ... |___| > > Glitches on parent dev near autosuspend_delay ^ > > - does the child driver needs to "kick" parent driver with > pm_runtime_mark_last_busy(), as proposed in current patch ? > > - or is it something that should be done by PM runtime core routines ? > e.g. make pm_runtime_mark_last_busy() recursive or something else ? > > Please advise > Best regards, > Fabrice > >> >> Thanks, >> >> Jonathan >> >>> --- >>> drivers/iio/adc/stm32-adc.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c >>> index 3b291d7..670157e 100644 >>> --- a/drivers/iio/adc/stm32-adc.c >>> +++ b/drivers/iio/adc/stm32-adc.c >>> @@ -1157,6 +1157,7 @@ static int stm32_adc_single_conv(struct iio_dev *indio_dev, >>> >>> stm32_adc_conv_irq_disable(adc); >>> >>> + pm_runtime_mark_last_busy(dev->parent); >>> pm_runtime_mark_last_busy(dev); >>> pm_runtime_put_autosuspend(dev); >>> >>> @@ -1278,6 +1279,7 @@ static int stm32_adc_update_scan_mode(struct iio_dev *indio_dev, >>> adc->num_conv = bitmap_weight(scan_mask, indio_dev->masklength); >>> >>> ret = stm32_adc_conf_scan_seq(indio_dev, scan_mask); >>> + pm_runtime_mark_last_busy(dev->parent); >>> pm_runtime_mark_last_busy(dev); >>> pm_runtime_put_autosuspend(dev); >>> >>> @@ -1329,6 +1331,7 @@ static int stm32_adc_debugfs_reg_access(struct iio_dev *indio_dev, >>> else >>> *readval = stm32_adc_readl(adc, reg); >>> >>> + pm_runtime_mark_last_busy(dev->parent); >>> pm_runtime_mark_last_busy(dev); >>> pm_runtime_put_autosuspend(dev); >>> >>> @@ -1451,6 +1454,7 @@ static int __stm32_adc_buffer_postenable(struct iio_dev *indio_dev) >>> err_clr_trig: >>> stm32_adc_set_trig(indio_dev, NULL); >>> err_pm_put: >>> + pm_runtime_mark_last_busy(dev->parent); >>> pm_runtime_mark_last_busy(dev); >>> pm_runtime_put_autosuspend(dev); >>> >>> @@ -1487,6 +1491,7 @@ static void __stm32_adc_buffer_predisable(struct iio_dev *indio_dev) >>> if (stm32_adc_set_trig(indio_dev, NULL)) >>> dev_err(&indio_dev->dev, "Can't clear trigger\n"); >>> >>> + pm_runtime_mark_last_busy(dev->parent); >>> pm_runtime_mark_last_busy(dev); >>> pm_runtime_put_autosuspend(dev); >>> } >>> @@ -1874,6 +1879,7 @@ static int stm32_adc_probe(struct platform_device *pdev) >>> goto err_hw_stop; >>> } >>> >>> + pm_runtime_mark_last_busy(dev->parent); >>> pm_runtime_mark_last_busy(dev); >>> pm_runtime_put_autosuspend(dev); >>> >> ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-03-06 8:23 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1579854369-7972-1-git-send-email-fabrice.gasnier@st.com>
2020-02-02 15:33 ` [PATCH] iio: adc: stm32-adc: fix runtime autosuspend delay when slow polling Jonathan Cameron
2020-02-12 14:02 ` Fabrice Gasnier
2020-03-06 8:22 ` Fabrice Gasnier
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).