* [PATCH v2 0/5] devfreq: handle suspend/resume [not found] <CGME20181203143127eucas1p2604fc066139a32fdffe996212b42b40e@eucas1p2.samsung.com> @ 2018-12-03 14:31 ` Lukasz Luba [not found] ` <CGME20181203143129eucas1p2955b6becc60ee57110cbc52f6e4f60c5@eucas1p2.samsung.com> ` (4 more replies) 0 siblings, 5 replies; 21+ messages in thread From: Lukasz Luba @ 2018-12-03 14:31 UTC (permalink / raw) To: linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, devicetree Cc: tjakobi, myungjoo.ham, kyungmin.park, cw00.choi, rjw, len.brown, pavel, gregkh, keescook, anton, ccross, tony.luck, robh+dt, mark.rutland, kgene, krzk, m.szyprowski, b.zolnierkie, Lukasz Luba Hi all, This v2 patch set aims to address the issue with devfreq devices' frequency during suspend/resume. It extends suspend/resume by calls to Devfreq framework. In the devfreq framework there is a small refactoring to avoid code duplication in changging frequency (patch 1) and there are extensions for suspending devices. The suspending device has now chance to set proper state when the system is going for suspend. This phase is the right place to set needed frequences for the next resume process. It has been tested on Odroid u3 with Exynos 4412. The patch set draws on Tobias Jakobi's work posted ~2 years ago, who tried to solve issue with devfreq device's frequency during suspend/resume. During the discussion on LKML some corner cases and comments appeared related to the design. This patch set address them keeping in mind suggestions from Chanwoo Choi. Tobias's paches: https://www.spinics.net/lists/linux-samsung-soc/msg56602.html Changes: v2: - refactored patchset and merget patch 1 and 3 as suggested by Chanwoo Choi, - changed devfreq_{susped|resume}_device functions, - added doxygen information for new entres in 'struct devfreq', - devfreq_set_target skipped one argument, now resume_freq is set inside, - minor changes addresing comments from maintainers regarding the style, Regards, Lukasz Luba Lukasz Luba (5): devfreq: refactor set_target frequency function devfreq: add support for suspend/resume of a devfreq device devfreq: add devfreq_suspend/resume() functions drivers: power: suspend: call devfreq suspend/resume arm: dts: exynos4: opp-suspend in DMC and leftbus arch/arm/boot/dts/exynos4210.dtsi | 2 + arch/arm/boot/dts/exynos4412.dtsi | 2 + drivers/base/power/main.c | 3 + drivers/devfreq/devfreq.c | 155 +++++++++++++++++++++++++++++--------- include/linux/devfreq.h | 13 ++++ 5 files changed, 141 insertions(+), 34 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 21+ messages in thread
[parent not found: <CGME20181203143129eucas1p2955b6becc60ee57110cbc52f6e4f60c5@eucas1p2.samsung.com>]
* [PATCH v2 1/5] devfreq: refactor set_target frequency function [not found] ` <CGME20181203143129eucas1p2955b6becc60ee57110cbc52f6e4f60c5@eucas1p2.samsung.com> @ 2018-12-03 14:31 ` Lukasz Luba 2018-12-04 4:39 ` Chanwoo Choi 0 siblings, 1 reply; 21+ messages in thread From: Lukasz Luba @ 2018-12-03 14:31 UTC (permalink / raw) To: linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, devicetree Cc: tjakobi, myungjoo.ham, kyungmin.park, cw00.choi, rjw, len.brown, pavel, gregkh, keescook, anton, ccross, tony.luck, robh+dt, mark.rutland, kgene, krzk, m.szyprowski, b.zolnierkie, Lukasz Luba The refactoring is needed for the new client in devfreq: suspend. To avoid code duplication, move it to the new local function devfreq_set_target. The patch is based on earlier work by Tobias Jakobi. Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> --- drivers/devfreq/devfreq.c | 62 +++++++++++++++++++++++++++-------------------- 1 file changed, 36 insertions(+), 26 deletions(-) diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 1414130..a9fd61b 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -285,6 +285,40 @@ static int devfreq_notify_transition(struct devfreq *devfreq, return 0; } +static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq, + u32 flags) +{ + struct devfreq_freqs freqs; + unsigned long cur_freq; + int err = 0; + + if (devfreq->profile->get_cur_freq) + devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq); + else + cur_freq = devfreq->previous_freq; + + freqs.old = cur_freq; + freqs.new = new_freq; + devfreq_notify_transition(devfreq, &freqs, DEVFREQ_PRECHANGE); + + err = devfreq->profile->target(devfreq->dev.parent, &new_freq, flags); + if (err) { + freqs.new = cur_freq; + devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE); + return err; + } + + freqs.new = new_freq; + devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE); + + if (devfreq_update_status(devfreq, new_freq)) + dev_err(&devfreq->dev, + "Couldn't update frequency transition information.\n"); + + devfreq->previous_freq = new_freq; + return err; +} + /* Load monitoring helper functions for governors use */ /** @@ -296,8 +330,7 @@ static int devfreq_notify_transition(struct devfreq *devfreq, */ int update_devfreq(struct devfreq *devfreq) { - struct devfreq_freqs freqs; - unsigned long freq, cur_freq, min_freq, max_freq; + unsigned long freq, min_freq, max_freq; int err = 0; u32 flags = 0; @@ -333,31 +366,8 @@ int update_devfreq(struct devfreq *devfreq) flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */ } - if (devfreq->profile->get_cur_freq) - devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq); - else - cur_freq = devfreq->previous_freq; - - freqs.old = cur_freq; - freqs.new = freq; - devfreq_notify_transition(devfreq, &freqs, DEVFREQ_PRECHANGE); + return devfreq_set_target(devfreq, freq, flags); - err = devfreq->profile->target(devfreq->dev.parent, &freq, flags); - if (err) { - freqs.new = cur_freq; - devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE); - return err; - } - - freqs.new = freq; - devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE); - - if (devfreq_update_status(devfreq, freq)) - dev_err(&devfreq->dev, - "Couldn't update frequency transition information.\n"); - - devfreq->previous_freq = freq; - return err; } EXPORT_SYMBOL(update_devfreq); -- 2.7.4 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/5] devfreq: refactor set_target frequency function 2018-12-03 14:31 ` [PATCH v2 1/5] devfreq: refactor set_target frequency function Lukasz Luba @ 2018-12-04 4:39 ` Chanwoo Choi 2018-12-04 5:43 ` Chanwoo Choi 2018-12-04 9:36 ` Lukasz Luba 0 siblings, 2 replies; 21+ messages in thread From: Chanwoo Choi @ 2018-12-04 4:39 UTC (permalink / raw) To: Lukasz Luba, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, devicetree Cc: tjakobi, myungjoo.ham, kyungmin.park, rjw, len.brown, pavel, gregkh, keescook, anton, ccross, tony.luck, robh+dt, mark.rutland, kgene, krzk, m.szyprowski, b.zolnierkie Hi Lukasz, On 2018년 12월 03일 23:31, Lukasz Luba wrote: > The refactoring is needed for the new client in devfreq: suspend. > To avoid code duplication, move it to the new local function > devfreq_set_target. > > The patch is based on earlier work by Tobias Jakobi. As I already commented, Please remove it. You already mentioned it on cover-letter. If you want to contain the contribution history of Tobias, you might better to add 'Signed-off-by' or others. > > Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> > Suggested-by: Chanwoo Choi <cw00.choi@samsung.com> > Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> > --- > drivers/devfreq/devfreq.c | 62 +++++++++++++++++++++++++++-------------------- > 1 file changed, 36 insertions(+), 26 deletions(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 1414130..a9fd61b 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -285,6 +285,40 @@ static int devfreq_notify_transition(struct devfreq *devfreq, > return 0; > } > > +static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq, > + u32 flags) > +{ > + struct devfreq_freqs freqs; > + unsigned long cur_freq; > + int err = 0; > + > + if (devfreq->profile->get_cur_freq) > + devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq); > + else > + cur_freq = devfreq->previous_freq; > + > + freqs.old = cur_freq; > + freqs.new = new_freq; > + devfreq_notify_transition(devfreq, &freqs, DEVFREQ_PRECHANGE); > + > + err = devfreq->profile->target(devfreq->dev.parent, &new_freq, flags); > + if (err) { > + freqs.new = cur_freq; > + devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE); > + return err; > + } > + > + freqs.new = new_freq; > + devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE); > + > + if (devfreq_update_status(devfreq, new_freq)) > + dev_err(&devfreq->dev, > + "Couldn't update frequency transition information.\n"); > + > + devfreq->previous_freq = new_freq; > + return err; > +} > + > /* Load monitoring helper functions for governors use */ > > /** > @@ -296,8 +330,7 @@ static int devfreq_notify_transition(struct devfreq *devfreq, > */ > int update_devfreq(struct devfreq *devfreq) > { > - struct devfreq_freqs freqs; > - unsigned long freq, cur_freq, min_freq, max_freq; > + unsigned long freq, min_freq, max_freq; > int err = 0; > u32 flags = 0; > > @@ -333,31 +366,8 @@ int update_devfreq(struct devfreq *devfreq) > flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */ > } > > - if (devfreq->profile->get_cur_freq) > - devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq); > - else > - cur_freq = devfreq->previous_freq; > - > - freqs.old = cur_freq; > - freqs.new = freq; > - devfreq_notify_transition(devfreq, &freqs, DEVFREQ_PRECHANGE); > + return devfreq_set_target(devfreq, freq, flags); > > - err = devfreq->profile->target(devfreq->dev.parent, &freq, flags); > - if (err) { > - freqs.new = cur_freq; > - devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE); > - return err; > - } > - > - freqs.new = freq; > - devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE); > - > - if (devfreq_update_status(devfreq, freq)) > - dev_err(&devfreq->dev, > - "Couldn't update frequency transition information.\n"); > - > - devfreq->previous_freq = freq; > - return err; > } > EXPORT_SYMBOL(update_devfreq); > > -- Best Regards, Chanwoo Choi Samsung Electronics ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/5] devfreq: refactor set_target frequency function 2018-12-04 4:39 ` Chanwoo Choi @ 2018-12-04 5:43 ` Chanwoo Choi 2018-12-04 9:36 ` Lukasz Luba 1 sibling, 0 replies; 21+ messages in thread From: Chanwoo Choi @ 2018-12-04 5:43 UTC (permalink / raw) To: Lukasz Luba, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, devicetree Cc: tjakobi, myungjoo.ham, kyungmin.park, rjw, len.brown, pavel, gregkh, keescook, anton, ccross, tony.luck, robh+dt, mark.rutland, kgene, krzk, m.szyprowski, b.zolnierkie Hi, On 2018년 12월 04일 13:39, Chanwoo Choi wrote: > Hi Lukasz, > > On 2018년 12월 03일 23:31, Lukasz Luba wrote: >> The refactoring is needed for the new client in devfreq: suspend. >> To avoid code duplication, move it to the new local function >> devfreq_set_target. >> >> The patch is based on earlier work by Tobias Jakobi. > > As I already commented, Please remove it. You already mentioned it on cover-letter. > If you want to contain the contribution history of Tobias, you might better > to add 'Signed-off-by' or others. If you will fix it, feel free to add my tag: Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> > >> >> Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com> >> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> >> --- >> drivers/devfreq/devfreq.c | 62 +++++++++++++++++++++++++++-------------------- >> 1 file changed, 36 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >> index 1414130..a9fd61b 100644 >> --- a/drivers/devfreq/devfreq.c >> +++ b/drivers/devfreq/devfreq.c >> @@ -285,6 +285,40 @@ static int devfreq_notify_transition(struct devfreq *devfreq, >> return 0; >> } >> >> +static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq, >> + u32 flags) >> +{ >> + struct devfreq_freqs freqs; >> + unsigned long cur_freq; >> + int err = 0; >> + >> + if (devfreq->profile->get_cur_freq) >> + devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq); >> + else >> + cur_freq = devfreq->previous_freq; >> + >> + freqs.old = cur_freq; >> + freqs.new = new_freq; >> + devfreq_notify_transition(devfreq, &freqs, DEVFREQ_PRECHANGE); >> + >> + err = devfreq->profile->target(devfreq->dev.parent, &new_freq, flags); >> + if (err) { >> + freqs.new = cur_freq; >> + devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE); >> + return err; >> + } >> + >> + freqs.new = new_freq; >> + devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE); >> + >> + if (devfreq_update_status(devfreq, new_freq)) >> + dev_err(&devfreq->dev, >> + "Couldn't update frequency transition information.\n"); >> + >> + devfreq->previous_freq = new_freq; >> + return err; >> +} >> + >> /* Load monitoring helper functions for governors use */ >> >> /** >> @@ -296,8 +330,7 @@ static int devfreq_notify_transition(struct devfreq *devfreq, >> */ >> int update_devfreq(struct devfreq *devfreq) >> { >> - struct devfreq_freqs freqs; >> - unsigned long freq, cur_freq, min_freq, max_freq; >> + unsigned long freq, min_freq, max_freq; >> int err = 0; >> u32 flags = 0; >> >> @@ -333,31 +366,8 @@ int update_devfreq(struct devfreq *devfreq) >> flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */ >> } >> >> - if (devfreq->profile->get_cur_freq) >> - devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq); >> - else >> - cur_freq = devfreq->previous_freq; >> - >> - freqs.old = cur_freq; >> - freqs.new = freq; >> - devfreq_notify_transition(devfreq, &freqs, DEVFREQ_PRECHANGE); >> + return devfreq_set_target(devfreq, freq, flags); >> >> - err = devfreq->profile->target(devfreq->dev.parent, &freq, flags); >> - if (err) { >> - freqs.new = cur_freq; >> - devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE); >> - return err; >> - } >> - >> - freqs.new = freq; >> - devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE); >> - >> - if (devfreq_update_status(devfreq, freq)) >> - dev_err(&devfreq->dev, >> - "Couldn't update frequency transition information.\n"); >> - >> - devfreq->previous_freq = freq; >> - return err; >> } >> EXPORT_SYMBOL(update_devfreq); >> >> > > -- Best Regards, Chanwoo Choi Samsung Electronics ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/5] devfreq: refactor set_target frequency function 2018-12-04 4:39 ` Chanwoo Choi 2018-12-04 5:43 ` Chanwoo Choi @ 2018-12-04 9:36 ` Lukasz Luba 1 sibling, 0 replies; 21+ messages in thread From: Lukasz Luba @ 2018-12-04 9:36 UTC (permalink / raw) To: Chanwoo Choi, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, devicetree Cc: tjakobi, myungjoo.ham, kyungmin.park, rjw, len.brown, pavel, gregkh, keescook, anton, ccross, tony.luck, robh+dt, mark.rutland, kgene, krzk, m.szyprowski, b.zolnierkie Hi Chanwoo, On 12/4/18 5:39 AM, Chanwoo Choi wrote: > Hi Lukasz, > > On 2018년 12월 03일 23:31, Lukasz Luba wrote: >> The refactoring is needed for the new client in devfreq: suspend. >> To avoid code duplication, move it to the new local function >> devfreq_set_target. >> >> The patch is based on earlier work by Tobias Jakobi. > > As I already commented, Please remove it. You already mentioned it on cover-letter. > If you want to contain the contribution history of Tobias, you might better > to add 'Signed-off-by' or others. OK Regards, Lukasz > >> >> Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com> >> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> >> --- >> drivers/devfreq/devfreq.c | 62 +++++++++++++++++++++++++++-------------------- >> 1 file changed, 36 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >> index 1414130..a9fd61b 100644 >> --- a/drivers/devfreq/devfreq.c >> +++ b/drivers/devfreq/devfreq.c >> @@ -285,6 +285,40 @@ static int devfreq_notify_transition(struct devfreq *devfreq, >> return 0; >> } >> >> +static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq, >> + u32 flags) >> +{ >> + struct devfreq_freqs freqs; >> + unsigned long cur_freq; >> + int err = 0; >> + >> + if (devfreq->profile->get_cur_freq) >> + devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq); >> + else >> + cur_freq = devfreq->previous_freq; >> + >> + freqs.old = cur_freq; >> + freqs.new = new_freq; >> + devfreq_notify_transition(devfreq, &freqs, DEVFREQ_PRECHANGE); >> + >> + err = devfreq->profile->target(devfreq->dev.parent, &new_freq, flags); >> + if (err) { >> + freqs.new = cur_freq; >> + devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE); >> + return err; >> + } >> + >> + freqs.new = new_freq; >> + devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE); >> + >> + if (devfreq_update_status(devfreq, new_freq)) >> + dev_err(&devfreq->dev, >> + "Couldn't update frequency transition information.\n"); >> + >> + devfreq->previous_freq = new_freq; >> + return err; >> +} >> + >> /* Load monitoring helper functions for governors use */ >> >> /** >> @@ -296,8 +330,7 @@ static int devfreq_notify_transition(struct devfreq *devfreq, >> */ >> int update_devfreq(struct devfreq *devfreq) >> { >> - struct devfreq_freqs freqs; >> - unsigned long freq, cur_freq, min_freq, max_freq; >> + unsigned long freq, min_freq, max_freq; >> int err = 0; >> u32 flags = 0; >> >> @@ -333,31 +366,8 @@ int update_devfreq(struct devfreq *devfreq) >> flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */ >> } >> >> - if (devfreq->profile->get_cur_freq) >> - devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq); >> - else >> - cur_freq = devfreq->previous_freq; >> - >> - freqs.old = cur_freq; >> - freqs.new = freq; >> - devfreq_notify_transition(devfreq, &freqs, DEVFREQ_PRECHANGE); >> + return devfreq_set_target(devfreq, freq, flags); >> >> - err = devfreq->profile->target(devfreq->dev.parent, &freq, flags); >> - if (err) { >> - freqs.new = cur_freq; >> - devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE); >> - return err; >> - } >> - >> - freqs.new = freq; >> - devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE); >> - >> - if (devfreq_update_status(devfreq, freq)) >> - dev_err(&devfreq->dev, >> - "Couldn't update frequency transition information.\n"); >> - >> - devfreq->previous_freq = freq; >> - return err; >> } >> EXPORT_SYMBOL(update_devfreq); >> >> > > ^ permalink raw reply [flat|nested] 21+ messages in thread
[parent not found: <CGME20181203143131eucas1p217f22ac6d19682a54a57658a06980914@eucas1p2.samsung.com>]
* [PATCH v2 2/5] devfreq: add support for suspend/resume of a devfreq device [not found] ` <CGME20181203143131eucas1p217f22ac6d19682a54a57658a06980914@eucas1p2.samsung.com> @ 2018-12-03 14:31 ` Lukasz Luba 2018-12-04 5:36 ` Chanwoo Choi 0 siblings, 1 reply; 21+ messages in thread From: Lukasz Luba @ 2018-12-03 14:31 UTC (permalink / raw) To: linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, devicetree Cc: tjakobi, myungjoo.ham, kyungmin.park, cw00.choi, rjw, len.brown, pavel, gregkh, keescook, anton, ccross, tony.luck, robh+dt, mark.rutland, kgene, krzk, m.szyprowski, b.zolnierkie, Lukasz Luba The patch prepares devfreq device for handling suspend/resume functionality. The new fields will store needed information during this process. Devfreq framework handles opp-suspend DT entry and there is no need of modyfications in the drivers code. It uses atomic variables to make sure no race condition affects the process. The patch is based on earlier work by Tobias Jakobi. Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> --- drivers/devfreq/devfreq.c | 51 +++++++++++++++++++++++++++++++++++++++-------- include/linux/devfreq.h | 7 +++++++ 2 files changed, 50 insertions(+), 8 deletions(-) diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index a9fd61b..36bed24 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -316,6 +316,10 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq, "Couldn't update frequency transition information.\n"); devfreq->previous_freq = new_freq; + + if (devfreq->suspend_freq) + devfreq->resume_freq = cur_freq; + return err; } @@ -667,6 +671,9 @@ struct devfreq *devfreq_add_device(struct device *dev, } devfreq->max_freq = devfreq->scaling_max_freq; + devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev); + atomic_set(&devfreq->suspend_count, 0); + dev_set_name(&devfreq->dev, "devfreq%d", atomic_inc_return(&devfreq_no)); err = device_register(&devfreq->dev); @@ -867,14 +874,28 @@ EXPORT_SYMBOL(devm_devfreq_remove_device); */ int devfreq_suspend_device(struct devfreq *devfreq) { + int ret; + if (!devfreq) return -EINVAL; - if (!devfreq->governor) - return 0; + if (devfreq->governor) { + ret = devfreq->governor->event_handler(devfreq, + DEVFREQ_GOV_SUSPEND, NULL); + if (ret) + return ret; + } + + if (devfreq->suspend_freq) { + if (atomic_inc_return(&devfreq->suspend_count) > 1) + return 0; + + ret = devfreq_set_target(devfreq, devfreq->suspend_freq, 0); + if (ret) + return ret; + } - return devfreq->governor->event_handler(devfreq, - DEVFREQ_GOV_SUSPEND, NULL); + return 0; } EXPORT_SYMBOL(devfreq_suspend_device); @@ -888,14 +909,28 @@ EXPORT_SYMBOL(devfreq_suspend_device); */ int devfreq_resume_device(struct devfreq *devfreq) { + int ret; + if (!devfreq) return -EINVAL; - if (!devfreq->governor) - return 0; + if (devfreq->resume_freq) { + if (atomic_dec_return(&devfreq->suspend_count) >= 1) + return 0; - return devfreq->governor->event_handler(devfreq, - DEVFREQ_GOV_RESUME, NULL); + ret = devfreq_set_target(devfreq, devfreq->resume_freq, 0); + if (ret) + return ret; + } + + if (devfreq->governor) { + ret = devfreq->governor->event_handler(devfreq, + DEVFREQ_GOV_RESUME, NULL); + if (ret) + return ret; + } + + return 0; } EXPORT_SYMBOL(devfreq_resume_device); diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h index e4963b0..d985199 100644 --- a/include/linux/devfreq.h +++ b/include/linux/devfreq.h @@ -131,6 +131,9 @@ struct devfreq_dev_profile { * @scaling_min_freq: Limit minimum frequency requested by OPP interface * @scaling_max_freq: Limit maximum frequency requested by OPP interface * @stop_polling: devfreq polling status of a device. + * @suspend_freq: frequency of a device set during suspend phase. + * @resume_freq: frequency of a device set in resume phase. + * @suspend_count: suspend requests counter for a device. * @total_trans: Number of devfreq transitions * @trans_table: Statistics of devfreq transitions * @time_in_state: Statistics of devfreq states @@ -167,6 +170,10 @@ struct devfreq { unsigned long scaling_max_freq; bool stop_polling; + unsigned long suspend_freq; + unsigned long resume_freq; + atomic_t suspend_count; + /* information for device frequency transition */ unsigned int total_trans; unsigned int *trans_table; -- 2.7.4 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/5] devfreq: add support for suspend/resume of a devfreq device 2018-12-03 14:31 ` [PATCH v2 2/5] devfreq: add support for suspend/resume of a devfreq device Lukasz Luba @ 2018-12-04 5:36 ` Chanwoo Choi 2018-12-04 5:43 ` Chanwoo Choi ` (2 more replies) 0 siblings, 3 replies; 21+ messages in thread From: Chanwoo Choi @ 2018-12-04 5:36 UTC (permalink / raw) To: Lukasz Luba, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, devicetree Cc: tjakobi, myungjoo.ham, kyungmin.park, rjw, len.brown, pavel, gregkh, keescook, anton, ccross, tony.luck, robh+dt, mark.rutland, kgene, krzk, m.szyprowski, b.zolnierkie Hi Lukasz, Looks good to me. But, I add the some comments. If you will fix it, feel free to add my tag: Reviewed-by: Chanwoo choi <cw00.choi@samsung.com> On 2018년 12월 03일 23:31, Lukasz Luba wrote: > The patch prepares devfreq device for handling suspend/resume > functionality. The new fields will store needed information during this nitpick. Remove unneeded space. There are two spaces between '.' and 'The new'. > process. Devfreq framework handles opp-suspend DT entry and there is no ditto. > need of modyfications in the drivers code. It uses atomic variables to ditto. > make sure no race condition affects the process. > > The patch is based on earlier work by Tobias Jakobi. Please remove it from each patch description. > > Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> > Suggested-by: Chanwoo Choi <cw00.choi@samsung.com> > Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> > --- > drivers/devfreq/devfreq.c | 51 +++++++++++++++++++++++++++++++++++++++-------- > include/linux/devfreq.h | 7 +++++++ > 2 files changed, 50 insertions(+), 8 deletions(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index a9fd61b..36bed24 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -316,6 +316,10 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq, > "Couldn't update frequency transition information.\n"); > > devfreq->previous_freq = new_freq; > + > + if (devfreq->suspend_freq) > + devfreq->resume_freq = cur_freq; > + > return err; > } > > @@ -667,6 +671,9 @@ struct devfreq *devfreq_add_device(struct device *dev, > } > devfreq->max_freq = devfreq->scaling_max_freq; > > + devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev); > + atomic_set(&devfreq->suspend_count, 0); > + > dev_set_name(&devfreq->dev, "devfreq%d", > atomic_inc_return(&devfreq_no)); > err = device_register(&devfreq->dev); > @@ -867,14 +874,28 @@ EXPORT_SYMBOL(devm_devfreq_remove_device); > */ > int devfreq_suspend_device(struct devfreq *devfreq) > { > + int ret; > + > if (!devfreq) > return -EINVAL; > > - if (!devfreq->governor) > - return 0; > + if (devfreq->governor) { > + ret = devfreq->governor->event_handler(devfreq, > + DEVFREQ_GOV_SUSPEND, NULL); > + if (ret) > + return ret; > + } > + > + if (devfreq->suspend_freq) { > + if (atomic_inc_return(&devfreq->suspend_count) > 1) > + return 0; > + > + ret = devfreq_set_target(devfreq, devfreq->suspend_freq, 0); > + if (ret) > + return ret; > + } > > - return devfreq->governor->event_handler(devfreq, > - DEVFREQ_GOV_SUSPEND, NULL); > + return 0; > } > EXPORT_SYMBOL(devfreq_suspend_device); > > @@ -888,14 +909,28 @@ EXPORT_SYMBOL(devfreq_suspend_device); > */ > int devfreq_resume_device(struct devfreq *devfreq) > { > + int ret; > + > if (!devfreq) > return -EINVAL; > > - if (!devfreq->governor) > - return 0; > + if (devfreq->resume_freq) { > + if (atomic_dec_return(&devfreq->suspend_count) >= 1) > + return 0; > > - return devfreq->governor->event_handler(devfreq, > - DEVFREQ_GOV_RESUME, NULL); > + ret = devfreq_set_target(devfreq, devfreq->resume_freq, 0); > + if (ret) > + return ret; > + } > + > + if (devfreq->governor) { > + ret = devfreq->governor->event_handler(devfreq, > + DEVFREQ_GOV_RESUME, NULL); > + if (ret) > + return ret; > + } > + > + return 0; > } > EXPORT_SYMBOL(devfreq_resume_device); > > diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h > index e4963b0..d985199 100644 > --- a/include/linux/devfreq.h > +++ b/include/linux/devfreq.h > @@ -131,6 +131,9 @@ struct devfreq_dev_profile { > * @scaling_min_freq: Limit minimum frequency requested by OPP interface > * @scaling_max_freq: Limit maximum frequency requested by OPP interface > * @stop_polling: devfreq polling status of a device. > + * @suspend_freq: frequency of a device set during suspend phase. > + * @resume_freq: frequency of a device set in resume phase. > + * @suspend_count: suspend requests counter for a device. > * @total_trans: Number of devfreq transitions > * @trans_table: Statistics of devfreq transitions > * @time_in_state: Statistics of devfreq states > @@ -167,6 +170,10 @@ struct devfreq { > unsigned long scaling_max_freq; > bool stop_polling; > > + unsigned long suspend_freq; > + unsigned long resume_freq; > + atomic_t suspend_count; > + > /* information for device frequency transition */ > unsigned int total_trans; > unsigned int *trans_table; > -- Best Regards, Chanwoo Choi Samsung Electronics ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/5] devfreq: add support for suspend/resume of a devfreq device 2018-12-04 5:36 ` Chanwoo Choi @ 2018-12-04 5:43 ` Chanwoo Choi 2018-12-04 6:10 ` Chanwoo Choi 2018-12-04 9:39 ` Lukasz Luba 2018-12-09 9:00 ` Pavel Machek 2 siblings, 1 reply; 21+ messages in thread From: Chanwoo Choi @ 2018-12-04 5:43 UTC (permalink / raw) To: Lukasz Luba, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, devicetree Cc: tjakobi, myungjoo.ham, kyungmin.park, rjw, len.brown, pavel, gregkh, keescook, anton, ccross, tony.luck, robh+dt, mark.rutland, kgene, krzk, m.szyprowski, b.zolnierkie Hi, On 2018년 12월 04일 14:36, Chanwoo Choi wrote: > Hi Lukasz, > > Looks good to me. But, I add the some comments. > If you will fix it, feel free to add my tag: > Reviewed-by: Chanwoo choi <cw00.choi@samsung.com> Sorry. Fix typo 'choi' to 'Choi' as following. Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> > > On 2018년 12월 03일 23:31, Lukasz Luba wrote: >> The patch prepares devfreq device for handling suspend/resume >> functionality. The new fields will store needed information during this > > nitpick. Remove unneeded space. There are two spaces between '.' and 'The new'. > >> process. Devfreq framework handles opp-suspend DT entry and there is no > > ditto. > >> need of modyfications in the drivers code. It uses atomic variables to > > ditto. > >> make sure no race condition affects the process. >> >> The patch is based on earlier work by Tobias Jakobi. > > Please remove it from each patch description. > >> >> Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com> >> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> >> --- >> drivers/devfreq/devfreq.c | 51 +++++++++++++++++++++++++++++++++++++++-------- >> include/linux/devfreq.h | 7 +++++++ >> 2 files changed, 50 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >> index a9fd61b..36bed24 100644 >> --- a/drivers/devfreq/devfreq.c >> +++ b/drivers/devfreq/devfreq.c >> @@ -316,6 +316,10 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq, >> "Couldn't update frequency transition information.\n"); >> >> devfreq->previous_freq = new_freq; >> + >> + if (devfreq->suspend_freq) >> + devfreq->resume_freq = cur_freq; >> + >> return err; >> } >> >> @@ -667,6 +671,9 @@ struct devfreq *devfreq_add_device(struct device *dev, >> } >> devfreq->max_freq = devfreq->scaling_max_freq; >> >> + devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev); >> + atomic_set(&devfreq->suspend_count, 0); >> + >> dev_set_name(&devfreq->dev, "devfreq%d", >> atomic_inc_return(&devfreq_no)); >> err = device_register(&devfreq->dev); >> @@ -867,14 +874,28 @@ EXPORT_SYMBOL(devm_devfreq_remove_device); >> */ >> int devfreq_suspend_device(struct devfreq *devfreq) >> { >> + int ret; >> + >> if (!devfreq) >> return -EINVAL; >> >> - if (!devfreq->governor) >> - return 0; >> + if (devfreq->governor) { >> + ret = devfreq->governor->event_handler(devfreq, >> + DEVFREQ_GOV_SUSPEND, NULL); >> + if (ret) >> + return ret; >> + } >> + >> + if (devfreq->suspend_freq) { >> + if (atomic_inc_return(&devfreq->suspend_count) > 1) >> + return 0; >> + >> + ret = devfreq_set_target(devfreq, devfreq->suspend_freq, 0); >> + if (ret) >> + return ret; >> + } >> >> - return devfreq->governor->event_handler(devfreq, >> - DEVFREQ_GOV_SUSPEND, NULL); >> + return 0; >> } >> EXPORT_SYMBOL(devfreq_suspend_device); >> >> @@ -888,14 +909,28 @@ EXPORT_SYMBOL(devfreq_suspend_device); >> */ >> int devfreq_resume_device(struct devfreq *devfreq) >> { >> + int ret; >> + >> if (!devfreq) >> return -EINVAL; >> >> - if (!devfreq->governor) >> - return 0; >> + if (devfreq->resume_freq) { >> + if (atomic_dec_return(&devfreq->suspend_count) >= 1) >> + return 0; >> >> - return devfreq->governor->event_handler(devfreq, >> - DEVFREQ_GOV_RESUME, NULL); >> + ret = devfreq_set_target(devfreq, devfreq->resume_freq, 0); >> + if (ret) >> + return ret; >> + } >> + >> + if (devfreq->governor) { >> + ret = devfreq->governor->event_handler(devfreq, >> + DEVFREQ_GOV_RESUME, NULL); >> + if (ret) >> + return ret; >> + } >> + >> + return 0; >> } >> EXPORT_SYMBOL(devfreq_resume_device); >> >> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h >> index e4963b0..d985199 100644 >> --- a/include/linux/devfreq.h >> +++ b/include/linux/devfreq.h >> @@ -131,6 +131,9 @@ struct devfreq_dev_profile { >> * @scaling_min_freq: Limit minimum frequency requested by OPP interface >> * @scaling_max_freq: Limit maximum frequency requested by OPP interface >> * @stop_polling: devfreq polling status of a device. >> + * @suspend_freq: frequency of a device set during suspend phase. >> + * @resume_freq: frequency of a device set in resume phase. >> + * @suspend_count: suspend requests counter for a device. >> * @total_trans: Number of devfreq transitions >> * @trans_table: Statistics of devfreq transitions >> * @time_in_state: Statistics of devfreq states >> @@ -167,6 +170,10 @@ struct devfreq { >> unsigned long scaling_max_freq; >> bool stop_polling; >> >> + unsigned long suspend_freq; >> + unsigned long resume_freq; >> + atomic_t suspend_count; >> + >> /* information for device frequency transition */ >> unsigned int total_trans; >> unsigned int *trans_table; >> > > -- Best Regards, Chanwoo Choi Samsung Electronics ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/5] devfreq: add support for suspend/resume of a devfreq device 2018-12-04 5:43 ` Chanwoo Choi @ 2018-12-04 6:10 ` Chanwoo Choi 2018-12-04 9:53 ` Lukasz Luba 0 siblings, 1 reply; 21+ messages in thread From: Chanwoo Choi @ 2018-12-04 6:10 UTC (permalink / raw) To: Lukasz Luba, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, devicetree Cc: tjakobi, myungjoo.ham, kyungmin.park, rjw, len.brown, pavel, gregkh, keescook, anton, ccross, tony.luck, robh+dt, mark.rutland, kgene, krzk, m.szyprowski, b.zolnierkie Hi Lukasz, I add the comment about 'suspend_count'. On 2018년 12월 04일 14:43, Chanwoo Choi wrote: > Hi, > > On 2018년 12월 04일 14:36, Chanwoo Choi wrote: >> Hi Lukasz, >> >> Looks good to me. But, I add the some comments. >> If you will fix it, feel free to add my tag: >> Reviewed-by: Chanwoo choi <cw00.choi@samsung.com> > > Sorry. Fix typo 'choi' to 'Choi' as following. > Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> > >> >> On 2018년 12월 03일 23:31, Lukasz Luba wrote: >>> The patch prepares devfreq device for handling suspend/resume >>> functionality. The new fields will store needed information during this >> >> nitpick. Remove unneeded space. There are two spaces between '.' and 'The new'. >> >>> process. Devfreq framework handles opp-suspend DT entry and there is no >> >> ditto. >> >>> need of modyfications in the drivers code. It uses atomic variables to >> >> ditto. >> >>> make sure no race condition affects the process. >>> >>> The patch is based on earlier work by Tobias Jakobi. >> >> Please remove it from each patch description. >> >>> >>> Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >>> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com> >>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> >>> --- >>> drivers/devfreq/devfreq.c | 51 +++++++++++++++++++++++++++++++++++++++-------- >>> include/linux/devfreq.h | 7 +++++++ >>> 2 files changed, 50 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>> index a9fd61b..36bed24 100644 >>> --- a/drivers/devfreq/devfreq.c >>> +++ b/drivers/devfreq/devfreq.c >>> @@ -316,6 +316,10 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq, >>> "Couldn't update frequency transition information.\n"); >>> >>> devfreq->previous_freq = new_freq; >>> + >>> + if (devfreq->suspend_freq) >>> + devfreq->resume_freq = cur_freq; >>> + >>> return err; >>> } >>> >>> @@ -667,6 +671,9 @@ struct devfreq *devfreq_add_device(struct device *dev, >>> } >>> devfreq->max_freq = devfreq->scaling_max_freq; >>> >>> + devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev); >>> + atomic_set(&devfreq->suspend_count, 0); >>> + >>> dev_set_name(&devfreq->dev, "devfreq%d", >>> atomic_inc_return(&devfreq_no)); >>> err = device_register(&devfreq->dev); >>> @@ -867,14 +874,28 @@ EXPORT_SYMBOL(devm_devfreq_remove_device); >>> */ >>> int devfreq_suspend_device(struct devfreq *devfreq) >>> { >>> + int ret; >>> + >>> if (!devfreq) >>> return -EINVAL; >>> >>> - if (!devfreq->governor) >>> - return 0; >>> + if (devfreq->governor) { >>> + ret = devfreq->governor->event_handler(devfreq, >>> + DEVFREQ_GOV_SUSPEND, NULL); >>> + if (ret) >>> + return ret; >>> + } >>> + >>> + if (devfreq->suspend_freq) { >>> + if (atomic_inc_return(&devfreq->suspend_count) > 1) >>> + return 0; >>> + >>> + ret = devfreq_set_target(devfreq, devfreq->suspend_freq, 0); >>> + if (ret) >>> + return ret; >>> + } In this patch, if some users call 'devfreq_suspend_device' twice, 'devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_SUSPEND, NULL)' is called twice but devfreq_set_target() is called only one. I knew that it is no problem for operation. But, I think that you better to use 'suspend_count' as the reference count of devfreq_suspend/resume_device(). But, if you use 'suspend_count' in order to check whether this devfreq is suspended or not, we can reduce the unneeded redundant call when calling it twice. clock and regulator used the 'reference count' method in order to remove the redundant call. >>> >>> - return devfreq->governor->event_handler(devfreq, >>> - DEVFREQ_GOV_SUSPEND, NULL); >>> + return 0; >>> } >>> EXPORT_SYMBOL(devfreq_suspend_device); >>> >>> @@ -888,14 +909,28 @@ EXPORT_SYMBOL(devfreq_suspend_device); >>> */ >>> int devfreq_resume_device(struct devfreq *devfreq) >>> { >>> + int ret; >>> + >>> if (!devfreq) >>> return -EINVAL; >>> >>> - if (!devfreq->governor) >>> - return 0; >>> + if (devfreq->resume_freq) { >>> + if (atomic_dec_return(&devfreq->suspend_count) >= 1) >>> + return 0; ditto. >>> >>> - return devfreq->governor->event_handler(devfreq, >>> - DEVFREQ_GOV_RESUME, NULL); >>> + ret = devfreq_set_target(devfreq, devfreq->resume_freq, 0); >>> + if (ret) >>> + return ret; >>> + } >>> + >>> + if (devfreq->governor) { >>> + ret = devfreq->governor->event_handler(devfreq, >>> + DEVFREQ_GOV_RESUME, NULL); >>> + if (ret) >>> + return ret; >>> + } >>> + >>> + return 0; >>> } >>> EXPORT_SYMBOL(devfreq_resume_device); >>> >>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h >>> index e4963b0..d985199 100644 >>> --- a/include/linux/devfreq.h >>> +++ b/include/linux/devfreq.h >>> @@ -131,6 +131,9 @@ struct devfreq_dev_profile { >>> * @scaling_min_freq: Limit minimum frequency requested by OPP interface >>> * @scaling_max_freq: Limit maximum frequency requested by OPP interface >>> * @stop_polling: devfreq polling status of a device. >>> + * @suspend_freq: frequency of a device set during suspend phase. >>> + * @resume_freq: frequency of a device set in resume phase. >>> + * @suspend_count: suspend requests counter for a device. >>> * @total_trans: Number of devfreq transitions >>> * @trans_table: Statistics of devfreq transitions >>> * @time_in_state: Statistics of devfreq states >>> @@ -167,6 +170,10 @@ struct devfreq { >>> unsigned long scaling_max_freq; >>> bool stop_polling; >>> >>> + unsigned long suspend_freq; >>> + unsigned long resume_freq; >>> + atomic_t suspend_count; >>> + >>> /* information for device frequency transition */ >>> unsigned int total_trans; >>> unsigned int *trans_table; >>> >> >> > > -- Best Regards, Chanwoo Choi Samsung Electronics ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/5] devfreq: add support for suspend/resume of a devfreq device 2018-12-04 6:10 ` Chanwoo Choi @ 2018-12-04 9:53 ` Lukasz Luba 2018-12-05 0:09 ` Chanwoo Choi 0 siblings, 1 reply; 21+ messages in thread From: Lukasz Luba @ 2018-12-04 9:53 UTC (permalink / raw) To: Chanwoo Choi, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, devicetree Cc: tjakobi, myungjoo.ham, kyungmin.park, rjw, len.brown, pavel, gregkh, keescook, anton, ccross, tony.luck, robh+dt, mark.rutland, kgene, krzk, m.szyprowski, b.zolnierkie Hi Chanwoo, On 12/4/18 7:10 AM, Chanwoo Choi wrote: > Hi Lukasz, > > I add the comment about 'suspend_count'. > > On 2018년 12월 04일 14:43, Chanwoo Choi wrote: >> Hi, >> >> On 2018년 12월 04일 14:36, Chanwoo Choi wrote: >>> Hi Lukasz, >>> >>> Looks good to me. But, I add the some comments. >>> If you will fix it, feel free to add my tag: >>> Reviewed-by: Chanwoo choi <cw00.choi@samsung.com> >> >> Sorry. Fix typo 'choi' to 'Choi' as following. >> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> >> >>> >>> On 2018년 12월 03일 23:31, Lukasz Luba wrote: >>>> The patch prepares devfreq device for handling suspend/resume >>>> functionality. The new fields will store needed information during this >>> >>> nitpick. Remove unneeded space. There are two spaces between '.' and 'The new'. >>> >>>> process. Devfreq framework handles opp-suspend DT entry and there is no >>> >>> ditto. >>> >>>> need of modyfications in the drivers code. It uses atomic variables to >>> >>> ditto. >>> >>>> make sure no race condition affects the process. >>>> >>>> The patch is based on earlier work by Tobias Jakobi. >>> >>> Please remove it from each patch description. >>> >>>> >>>> Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >>>> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com> >>>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> >>>> --- >>>> drivers/devfreq/devfreq.c | 51 +++++++++++++++++++++++++++++++++++++++-------- >>>> include/linux/devfreq.h | 7 +++++++ >>>> 2 files changed, 50 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>>> index a9fd61b..36bed24 100644 >>>> --- a/drivers/devfreq/devfreq.c >>>> +++ b/drivers/devfreq/devfreq.c >>>> @@ -316,6 +316,10 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq, >>>> "Couldn't update frequency transition information.\n"); >>>> >>>> devfreq->previous_freq = new_freq; >>>> + >>>> + if (devfreq->suspend_freq) >>>> + devfreq->resume_freq = cur_freq; >>>> + >>>> return err; >>>> } >>>> >>>> @@ -667,6 +671,9 @@ struct devfreq *devfreq_add_device(struct device *dev, >>>> } >>>> devfreq->max_freq = devfreq->scaling_max_freq; >>>> >>>> + devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev); >>>> + atomic_set(&devfreq->suspend_count, 0); >>>> + >>>> dev_set_name(&devfreq->dev, "devfreq%d", >>>> atomic_inc_return(&devfreq_no)); >>>> err = device_register(&devfreq->dev); >>>> @@ -867,14 +874,28 @@ EXPORT_SYMBOL(devm_devfreq_remove_device); >>>> */ >>>> int devfreq_suspend_device(struct devfreq *devfreq) >>>> { >>>> + int ret; >>>> + >>>> if (!devfreq) >>>> return -EINVAL; >>>> >>>> - if (!devfreq->governor) >>>> - return 0; >>>> + if (devfreq->governor) { >>>> + ret = devfreq->governor->event_handler(devfreq, >>>> + DEVFREQ_GOV_SUSPEND, NULL); >>>> + if (ret) >>>> + return ret; >>>> + } >>>> + >>>> + if (devfreq->suspend_freq) { >>>> + if (atomic_inc_return(&devfreq->suspend_count) > 1) >>>> + return 0; >>>> + >>>> + ret = devfreq_set_target(devfreq, devfreq->suspend_freq, 0); >>>> + if (ret) >>>> + return ret; >>>> + } > > In this patch, if some users call 'devfreq_suspend_device' twice, > 'devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_SUSPEND, NULL)' > is called twice but devfreq_set_target() is called only one. > I knew that it is no problem for operation. > > But, > I think that you better to use 'suspend_count' as the reference count > of devfreq_suspend/resume_device(). But, if you use 'suspend_count' > in order to check whether this devfreq is suspended or not, > we can reduce the unneeded redundant call when calling it twice. > > clock and regulator used the 'reference count' method in order to > remove the redundant call. I think I've got the point. I will move the atomic check just after the !devfreq check. Something like the code bellow is what you would like to see? ---8<----- if (!devfreq) return -EINVAL; if (atomic_inc_return(&devfreq->suspend_count) > 1) return0; ---->8------- > > >>>> >>>> - return devfreq->governor->event_handler(devfreq, >>>> - DEVFREQ_GOV_SUSPEND, NULL); >>>> + return 0; >>>> } >>>> EXPORT_SYMBOL(devfreq_suspend_device); >>>> >>>> @@ -888,14 +909,28 @@ EXPORT_SYMBOL(devfreq_suspend_device); >>>> */ >>>> int devfreq_resume_device(struct devfreq *devfreq) >>>> { >>>> + int ret; >>>> + >>>> if (!devfreq) >>>> return -EINVAL; >>>> >>>> - if (!devfreq->governor) >>>> - return 0; >>>> + if (devfreq->resume_freq) { >>>> + if (atomic_dec_return(&devfreq->suspend_count) >= 1) >>>> + return 0; > > ditto. Same approach here: ---8<----- if (!devfreq) return -EINVAL; if (atomic_dec_return(&devfreq->suspend_count) >= 1) return 0; ---->8------- Regards, Lukasz > >>>> >>>> - return devfreq->governor->event_handler(devfreq, >>>> - DEVFREQ_GOV_RESUME, NULL); >>>> + ret = devfreq_set_target(devfreq, devfreq->resume_freq, 0); >>>> + if (ret) >>>> + return ret; >>>> + } >>>> + >>>> + if (devfreq->governor) { >>>> + ret = devfreq->governor->event_handler(devfreq, >>>> + DEVFREQ_GOV_RESUME, NULL); >>>> + if (ret) >>>> + return ret; >>>> + } >>>> + >>>> + return 0; >>>> } >>>> EXPORT_SYMBOL(devfreq_resume_device); >>>> >>>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h >>>> index e4963b0..d985199 100644 >>>> --- a/include/linux/devfreq.h >>>> +++ b/include/linux/devfreq.h >>>> @@ -131,6 +131,9 @@ struct devfreq_dev_profile { >>>> * @scaling_min_freq: Limit minimum frequency requested by OPP interface >>>> * @scaling_max_freq: Limit maximum frequency requested by OPP interface >>>> * @stop_polling: devfreq polling status of a device. >>>> + * @suspend_freq: frequency of a device set during suspend phase. >>>> + * @resume_freq: frequency of a device set in resume phase. >>>> + * @suspend_count: suspend requests counter for a device. >>>> * @total_trans: Number of devfreq transitions >>>> * @trans_table: Statistics of devfreq transitions >>>> * @time_in_state: Statistics of devfreq states >>>> @@ -167,6 +170,10 @@ struct devfreq { >>>> unsigned long scaling_max_freq; >>>> bool stop_polling; >>>> >>>> + unsigned long suspend_freq; >>>> + unsigned long resume_freq; >>>> + atomic_t suspend_count; >>>> + >>>> /* information for device frequency transition */ >>>> unsigned int total_trans; >>>> unsigned int *trans_table; >>>> >>> >>> >> >> > > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/5] devfreq: add support for suspend/resume of a devfreq device 2018-12-04 9:53 ` Lukasz Luba @ 2018-12-05 0:09 ` Chanwoo Choi 2018-12-05 11:07 ` Lukasz Luba 0 siblings, 1 reply; 21+ messages in thread From: Chanwoo Choi @ 2018-12-05 0:09 UTC (permalink / raw) To: Lukasz Luba, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, devicetree Cc: tjakobi, myungjoo.ham, kyungmin.park, rjw, len.brown, pavel, gregkh, keescook, anton, ccross, tony.luck, robh+dt, mark.rutland, kgene, krzk, m.szyprowski, b.zolnierkie Hi Lukasz, On 2018년 12월 04일 18:53, Lukasz Luba wrote: > Hi Chanwoo, > > On 12/4/18 7:10 AM, Chanwoo Choi wrote: >> Hi Lukasz, >> >> I add the comment about 'suspend_count'. >> >> On 2018년 12월 04일 14:43, Chanwoo Choi wrote: >>> Hi, >>> >>> On 2018년 12월 04일 14:36, Chanwoo Choi wrote: >>>> Hi Lukasz, >>>> >>>> Looks good to me. But, I add the some comments. >>>> If you will fix it, feel free to add my tag: >>>> Reviewed-by: Chanwoo choi <cw00.choi@samsung.com> >>> >>> Sorry. Fix typo 'choi' to 'Choi' as following. >>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> >>> >>>> >>>> On 2018년 12월 03일 23:31, Lukasz Luba wrote: >>>>> The patch prepares devfreq device for handling suspend/resume >>>>> functionality. The new fields will store needed information during this >>>> >>>> nitpick. Remove unneeded space. There are two spaces between '.' and 'The new'. >>>> >>>>> process. Devfreq framework handles opp-suspend DT entry and there is no >>>> >>>> ditto. >>>> >>>>> need of modyfications in the drivers code. It uses atomic variables to >>>> >>>> ditto. >>>> >>>>> make sure no race condition affects the process. >>>>> >>>>> The patch is based on earlier work by Tobias Jakobi. >>>> >>>> Please remove it from each patch description. >>>> >>>>> >>>>> Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >>>>> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com> >>>>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> >>>>> --- >>>>> drivers/devfreq/devfreq.c | 51 +++++++++++++++++++++++++++++++++++++++-------- >>>>> include/linux/devfreq.h | 7 +++++++ >>>>> 2 files changed, 50 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>>>> index a9fd61b..36bed24 100644 >>>>> --- a/drivers/devfreq/devfreq.c >>>>> +++ b/drivers/devfreq/devfreq.c >>>>> @@ -316,6 +316,10 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq, >>>>> "Couldn't update frequency transition information.\n"); >>>>> >>>>> devfreq->previous_freq = new_freq; >>>>> + >>>>> + if (devfreq->suspend_freq) >>>>> + devfreq->resume_freq = cur_freq; >>>>> + >>>>> return err; >>>>> } >>>>> >>>>> @@ -667,6 +671,9 @@ struct devfreq *devfreq_add_device(struct device *dev, >>>>> } >>>>> devfreq->max_freq = devfreq->scaling_max_freq; >>>>> >>>>> + devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev); >>>>> + atomic_set(&devfreq->suspend_count, 0); >>>>> + >>>>> dev_set_name(&devfreq->dev, "devfreq%d", >>>>> atomic_inc_return(&devfreq_no)); >>>>> err = device_register(&devfreq->dev); >>>>> @@ -867,14 +874,28 @@ EXPORT_SYMBOL(devm_devfreq_remove_device); >>>>> */ >>>>> int devfreq_suspend_device(struct devfreq *devfreq) >>>>> { >>>>> + int ret; >>>>> + >>>>> if (!devfreq) >>>>> return -EINVAL; >>>>> >>>>> - if (!devfreq->governor) >>>>> - return 0; >>>>> + if (devfreq->governor) { >>>>> + ret = devfreq->governor->event_handler(devfreq, >>>>> + DEVFREQ_GOV_SUSPEND, NULL); >>>>> + if (ret) >>>>> + return ret; >>>>> + } >>>>> + >>>>> + if (devfreq->suspend_freq) { >>>>> + if (atomic_inc_return(&devfreq->suspend_count) > 1) >>>>> + return 0; >>>>> + >>>>> + ret = devfreq_set_target(devfreq, devfreq->suspend_freq, 0); >>>>> + if (ret) >>>>> + return ret; >>>>> + } >> >> In this patch, if some users call 'devfreq_suspend_device' twice, >> 'devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_SUSPEND, NULL)' >> is called twice but devfreq_set_target() is called only one. >> I knew that it is no problem for operation. >> >> But, >> I think that you better to use 'suspend_count' as the reference count >> of devfreq_suspend/resume_device(). But, if you use 'suspend_count' >> in order to check whether this devfreq is suspended or not, >> we can reduce the unneeded redundant call when calling it twice. >> >> clock and regulator used the 'reference count' method in order to >> remove the redundant call. > > I think I've got the point. I will move the atomic check just > after the !devfreq check. Something like the code bellow is what you > would like to see? > ---8<----- > if (!devfreq) > return -EINVAL; > if (atomic_inc_return(&devfreq->suspend_count) > 1) > return0; Looks good to me. I agree. > > ---->8------- >> >> >>>>> >>>>> - return devfreq->governor->event_handler(devfreq, >>>>> - DEVFREQ_GOV_SUSPEND, NULL); >>>>> + return 0; >>>>> } >>>>> EXPORT_SYMBOL(devfreq_suspend_device); >>>>> >>>>> @@ -888,14 +909,28 @@ EXPORT_SYMBOL(devfreq_suspend_device); >>>>> */ >>>>> int devfreq_resume_device(struct devfreq *devfreq) >>>>> { >>>>> + int ret; >>>>> + >>>>> if (!devfreq) >>>>> return -EINVAL; >>>>> >>>>> - if (!devfreq->governor) >>>>> - return 0; >>>>> + if (devfreq->resume_freq) { >>>>> + if (atomic_dec_return(&devfreq->suspend_count) >= 1) >>>>> + return 0; >> >> ditto. > Same approach here: > ---8<----- > if (!devfreq) > return -EINVAL; > if (atomic_dec_return(&devfreq->suspend_count) >= 1) > return 0; Looks good to me. > ---->8------- > > Regards, > Lukasz >> >>>>> >>>>> - return devfreq->governor->event_handler(devfreq, >>>>> - DEVFREQ_GOV_RESUME, NULL); >>>>> + ret = devfreq_set_target(devfreq, devfreq->resume_freq, 0); >>>>> + if (ret) >>>>> + return ret; >>>>> + } >>>>> + >>>>> + if (devfreq->governor) { >>>>> + ret = devfreq->governor->event_handler(devfreq, >>>>> + DEVFREQ_GOV_RESUME, NULL); >>>>> + if (ret) >>>>> + return ret; >>>>> + } >>>>> + >>>>> + return 0; >>>>> } >>>>> EXPORT_SYMBOL(devfreq_resume_device); >>>>> >>>>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h >>>>> index e4963b0..d985199 100644 >>>>> --- a/include/linux/devfreq.h >>>>> +++ b/include/linux/devfreq.h >>>>> @@ -131,6 +131,9 @@ struct devfreq_dev_profile { >>>>> * @scaling_min_freq: Limit minimum frequency requested by OPP interface >>>>> * @scaling_max_freq: Limit maximum frequency requested by OPP interface >>>>> * @stop_polling: devfreq polling status of a device. >>>>> + * @suspend_freq: frequency of a device set during suspend phase. >>>>> + * @resume_freq: frequency of a device set in resume phase. >>>>> + * @suspend_count: suspend requests counter for a device. >>>>> * @total_trans: Number of devfreq transitions >>>>> * @trans_table: Statistics of devfreq transitions >>>>> * @time_in_state: Statistics of devfreq states >>>>> @@ -167,6 +170,10 @@ struct devfreq { >>>>> unsigned long scaling_max_freq; >>>>> bool stop_polling; >>>>> >>>>> + unsigned long suspend_freq; >>>>> + unsigned long resume_freq; >>>>> + atomic_t suspend_count; >>>>> + >>>>> /* information for device frequency transition */ >>>>> unsigned int total_trans; >>>>> unsigned int *trans_table; >>>>> >>>> >>>> >>> >>> >> >> > > -- Best Regards, Chanwoo Choi Samsung Electronics ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/5] devfreq: add support for suspend/resume of a devfreq device 2018-12-05 0:09 ` Chanwoo Choi @ 2018-12-05 11:07 ` Lukasz Luba 0 siblings, 0 replies; 21+ messages in thread From: Lukasz Luba @ 2018-12-05 11:07 UTC (permalink / raw) To: Chanwoo Choi, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, devicetree Cc: tjakobi, myungjoo.ham, kyungmin.park, rjw, len.brown, pavel, gregkh, keescook, anton, ccross, tony.luck, robh+dt, mark.rutland, kgene, krzk, m.szyprowski, b.zolnierkie Hi Chanwoo, On 12/5/18 1:09 AM, Chanwoo Choi wrote: > Hi Lukasz, > > On 2018년 12월 04일 18:53, Lukasz Luba wrote: >> Hi Chanwoo, >> >> On 12/4/18 7:10 AM, Chanwoo Choi wrote: >>> Hi Lukasz, >>> >>> I add the comment about 'suspend_count'. >>> >>> On 2018년 12월 04일 14:43, Chanwoo Choi wrote: >>>> Hi, >>>> >>>> On 2018년 12월 04일 14:36, Chanwoo Choi wrote: >>>>> Hi Lukasz, >>>>> >>>>> Looks good to me. But, I add the some comments. >>>>> If you will fix it, feel free to add my tag: >>>>> Reviewed-by: Chanwoo choi <cw00.choi@samsung.com> >>>> >>>> Sorry. Fix typo 'choi' to 'Choi' as following. >>>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> >>>> >>>>> >>>>> On 2018년 12월 03일 23:31, Lukasz Luba wrote: >>>>>> The patch prepares devfreq device for handling suspend/resume >>>>>> functionality. The new fields will store needed information during this >>>>> >>>>> nitpick. Remove unneeded space. There are two spaces between '.' and 'The new'. >>>>> >>>>>> process. Devfreq framework handles opp-suspend DT entry and there is no >>>>> >>>>> ditto. >>>>> >>>>>> need of modyfications in the drivers code. It uses atomic variables to >>>>> >>>>> ditto. >>>>> >>>>>> make sure no race condition affects the process. >>>>>> >>>>>> The patch is based on earlier work by Tobias Jakobi. >>>>> >>>>> Please remove it from each patch description. >>>>> >>>>>> >>>>>> Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >>>>>> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com> >>>>>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> >>>>>> --- >>>>>> drivers/devfreq/devfreq.c | 51 +++++++++++++++++++++++++++++++++++++++-------- >>>>>> include/linux/devfreq.h | 7 +++++++ >>>>>> 2 files changed, 50 insertions(+), 8 deletions(-) >>>>>> >>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>>>>> index a9fd61b..36bed24 100644 >>>>>> --- a/drivers/devfreq/devfreq.c >>>>>> +++ b/drivers/devfreq/devfreq.c >>>>>> @@ -316,6 +316,10 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq, >>>>>> "Couldn't update frequency transition information.\n"); >>>>>> >>>>>> devfreq->previous_freq = new_freq; >>>>>> + >>>>>> + if (devfreq->suspend_freq) >>>>>> + devfreq->resume_freq = cur_freq; >>>>>> + >>>>>> return err; >>>>>> } >>>>>> >>>>>> @@ -667,6 +671,9 @@ struct devfreq *devfreq_add_device(struct device *dev, >>>>>> } >>>>>> devfreq->max_freq = devfreq->scaling_max_freq; >>>>>> >>>>>> + devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev); >>>>>> + atomic_set(&devfreq->suspend_count, 0); >>>>>> + >>>>>> dev_set_name(&devfreq->dev, "devfreq%d", >>>>>> atomic_inc_return(&devfreq_no)); >>>>>> err = device_register(&devfreq->dev); >>>>>> @@ -867,14 +874,28 @@ EXPORT_SYMBOL(devm_devfreq_remove_device); >>>>>> */ >>>>>> int devfreq_suspend_device(struct devfreq *devfreq) >>>>>> { >>>>>> + int ret; >>>>>> + >>>>>> if (!devfreq) >>>>>> return -EINVAL; >>>>>> >>>>>> - if (!devfreq->governor) >>>>>> - return 0; >>>>>> + if (devfreq->governor) { >>>>>> + ret = devfreq->governor->event_handler(devfreq, >>>>>> + DEVFREQ_GOV_SUSPEND, NULL); >>>>>> + if (ret) >>>>>> + return ret; >>>>>> + } >>>>>> + >>>>>> + if (devfreq->suspend_freq) { >>>>>> + if (atomic_inc_return(&devfreq->suspend_count) > 1) >>>>>> + return 0; >>>>>> + >>>>>> + ret = devfreq_set_target(devfreq, devfreq->suspend_freq, 0); >>>>>> + if (ret) >>>>>> + return ret; >>>>>> + } >>> >>> In this patch, if some users call 'devfreq_suspend_device' twice, >>> 'devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_SUSPEND, NULL)' >>> is called twice but devfreq_set_target() is called only one. >>> I knew that it is no problem for operation. >>> >>> But, >>> I think that you better to use 'suspend_count' as the reference count >>> of devfreq_suspend/resume_device(). But, if you use 'suspend_count' >>> in order to check whether this devfreq is suspended or not, >>> we can reduce the unneeded redundant call when calling it twice. >>> >>> clock and regulator used the 'reference count' method in order to >>> remove the redundant call. >> >> I think I've got the point. I will move the atomic check just >> after the !devfreq check. Something like the code bellow is what you >> would like to see? >> ---8<----- >> if (!devfreq) >> return -EINVAL; >> if (atomic_inc_return(&devfreq->suspend_count) > 1) >> return0; > > Looks good to me. I agree. OK > >> >> ---->8------- >>> >>> >>>>>> >>>>>> - return devfreq->governor->event_handler(devfreq, >>>>>> - DEVFREQ_GOV_SUSPEND, NULL); >>>>>> + return 0; >>>>>> } >>>>>> EXPORT_SYMBOL(devfreq_suspend_device); >>>>>> >>>>>> @@ -888,14 +909,28 @@ EXPORT_SYMBOL(devfreq_suspend_device); >>>>>> */ >>>>>> int devfreq_resume_device(struct devfreq *devfreq) >>>>>> { >>>>>> + int ret; >>>>>> + >>>>>> if (!devfreq) >>>>>> return -EINVAL; >>>>>> >>>>>> - if (!devfreq->governor) >>>>>> - return 0; >>>>>> + if (devfreq->resume_freq) { >>>>>> + if (atomic_dec_return(&devfreq->suspend_count) >= 1) >>>>>> + return 0; >>> >>> ditto. >> Same approach here: >> ---8<----- >> if (!devfreq) >> return -EINVAL; >> if (atomic_dec_return(&devfreq->suspend_count) >= 1) >> return 0; > > Looks good to me. Thank you. The patch set v3 is going to be sent. Regards, Lukasz > >> ---->8------- >> >> Regards, >> Lukasz >>> >>>>>> >>>>>> - return devfreq->governor->event_handler(devfreq, >>>>>> - DEVFREQ_GOV_RESUME, NULL); >>>>>> + ret = devfreq_set_target(devfreq, devfreq->resume_freq, 0); >>>>>> + if (ret) >>>>>> + return ret; >>>>>> + } >>>>>> + >>>>>> + if (devfreq->governor) { >>>>>> + ret = devfreq->governor->event_handler(devfreq, >>>>>> + DEVFREQ_GOV_RESUME, NULL); >>>>>> + if (ret) >>>>>> + return ret; >>>>>> + } >>>>>> + >>>>>> + return 0; >>>>>> } >>>>>> EXPORT_SYMBOL(devfreq_resume_device); >>>>>> >>>>>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h >>>>>> index e4963b0..d985199 100644 >>>>>> --- a/include/linux/devfreq.h >>>>>> +++ b/include/linux/devfreq.h >>>>>> @@ -131,6 +131,9 @@ struct devfreq_dev_profile { >>>>>> * @scaling_min_freq: Limit minimum frequency requested by OPP interface >>>>>> * @scaling_max_freq: Limit maximum frequency requested by OPP interface >>>>>> * @stop_polling: devfreq polling status of a device. >>>>>> + * @suspend_freq: frequency of a device set during suspend phase. >>>>>> + * @resume_freq: frequency of a device set in resume phase. >>>>>> + * @suspend_count: suspend requests counter for a device. >>>>>> * @total_trans: Number of devfreq transitions >>>>>> * @trans_table: Statistics of devfreq transitions >>>>>> * @time_in_state: Statistics of devfreq states >>>>>> @@ -167,6 +170,10 @@ struct devfreq { >>>>>> unsigned long scaling_max_freq; >>>>>> bool stop_polling; >>>>>> >>>>>> + unsigned long suspend_freq; >>>>>> + unsigned long resume_freq; >>>>>> + atomic_t suspend_count; >>>>>> + >>>>>> /* information for device frequency transition */ >>>>>> unsigned int total_trans; >>>>>> unsigned int *trans_table; >>>>>> >>>>> >>>>> >>>> >>>> >>> >>> >> >> > > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/5] devfreq: add support for suspend/resume of a devfreq device 2018-12-04 5:36 ` Chanwoo Choi 2018-12-04 5:43 ` Chanwoo Choi @ 2018-12-04 9:39 ` Lukasz Luba 2018-12-09 9:00 ` Pavel Machek 2 siblings, 0 replies; 21+ messages in thread From: Lukasz Luba @ 2018-12-04 9:39 UTC (permalink / raw) To: Chanwoo Choi, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, devicetree Cc: tjakobi, myungjoo.ham, kyungmin.park, rjw, len.brown, pavel, gregkh, keescook, anton, ccross, tony.luck, robh+dt, mark.rutland, kgene, krzk, m.szyprowski, b.zolnierkie Hi Chanwoo, On 12/4/18 6:36 AM, Chanwoo Choi wrote: > Hi Lukasz, > > Looks good to me. But, I add the some comments. > If you will fix it, feel free to add my tag: > Reviewed-by: Chanwoo choi <cw00.choi@samsung.com> > > On 2018년 12월 03일 23:31, Lukasz Luba wrote: >> The patch prepares devfreq device for handling suspend/resume >> functionality. The new fields will store needed information during this > > nitpick. Remove unneeded space. There are two spaces between '.' and 'The new'. > >> process. Devfreq framework handles opp-suspend DT entry and there is no > > ditto. > >> need of modyfications in the drivers code. It uses atomic variables to > > ditto. > >> make sure no race condition affects the process. >> >> The patch is based on earlier work by Tobias Jakobi. > > Please remove it from each patch description. > Comments addressed in next v3 patch set. Thank you. Regards, Lukasz >> >> Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com> >> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> >> --- >> drivers/devfreq/devfreq.c | 51 +++++++++++++++++++++++++++++++++++++++-------- >> include/linux/devfreq.h | 7 +++++++ >> 2 files changed, 50 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >> index a9fd61b..36bed24 100644 >> --- a/drivers/devfreq/devfreq.c >> +++ b/drivers/devfreq/devfreq.c >> @@ -316,6 +316,10 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq, >> "Couldn't update frequency transition information.\n"); >> >> devfreq->previous_freq = new_freq; >> + >> + if (devfreq->suspend_freq) >> + devfreq->resume_freq = cur_freq; >> + >> return err; >> } >> >> @@ -667,6 +671,9 @@ struct devfreq *devfreq_add_device(struct device *dev, >> } >> devfreq->max_freq = devfreq->scaling_max_freq; >> >> + devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev); >> + atomic_set(&devfreq->suspend_count, 0); >> + >> dev_set_name(&devfreq->dev, "devfreq%d", >> atomic_inc_return(&devfreq_no)); >> err = device_register(&devfreq->dev); >> @@ -867,14 +874,28 @@ EXPORT_SYMBOL(devm_devfreq_remove_device); >> */ >> int devfreq_suspend_device(struct devfreq *devfreq) >> { >> + int ret; >> + >> if (!devfreq) >> return -EINVAL; >> >> - if (!devfreq->governor) >> - return 0; >> + if (devfreq->governor) { >> + ret = devfreq->governor->event_handler(devfreq, >> + DEVFREQ_GOV_SUSPEND, NULL); >> + if (ret) >> + return ret; >> + } >> + >> + if (devfreq->suspend_freq) { >> + if (atomic_inc_return(&devfreq->suspend_count) > 1) >> + return 0; >> + >> + ret = devfreq_set_target(devfreq, devfreq->suspend_freq, 0); >> + if (ret) >> + return ret; >> + } >> >> - return devfreq->governor->event_handler(devfreq, >> - DEVFREQ_GOV_SUSPEND, NULL); >> + return 0; >> } >> EXPORT_SYMBOL(devfreq_suspend_device); >> >> @@ -888,14 +909,28 @@ EXPORT_SYMBOL(devfreq_suspend_device); >> */ >> int devfreq_resume_device(struct devfreq *devfreq) >> { >> + int ret; >> + >> if (!devfreq) >> return -EINVAL; >> >> - if (!devfreq->governor) >> - return 0; >> + if (devfreq->resume_freq) { >> + if (atomic_dec_return(&devfreq->suspend_count) >= 1) >> + return 0; >> >> - return devfreq->governor->event_handler(devfreq, >> - DEVFREQ_GOV_RESUME, NULL); >> + ret = devfreq_set_target(devfreq, devfreq->resume_freq, 0); >> + if (ret) >> + return ret; >> + } >> + >> + if (devfreq->governor) { >> + ret = devfreq->governor->event_handler(devfreq, >> + DEVFREQ_GOV_RESUME, NULL); >> + if (ret) >> + return ret; >> + } >> + >> + return 0; >> } >> EXPORT_SYMBOL(devfreq_resume_device); >> >> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h >> index e4963b0..d985199 100644 >> --- a/include/linux/devfreq.h >> +++ b/include/linux/devfreq.h >> @@ -131,6 +131,9 @@ struct devfreq_dev_profile { >> * @scaling_min_freq: Limit minimum frequency requested by OPP interface >> * @scaling_max_freq: Limit maximum frequency requested by OPP interface >> * @stop_polling: devfreq polling status of a device. >> + * @suspend_freq: frequency of a device set during suspend phase. >> + * @resume_freq: frequency of a device set in resume phase. >> + * @suspend_count: suspend requests counter for a device. >> * @total_trans: Number of devfreq transitions >> * @trans_table: Statistics of devfreq transitions >> * @time_in_state: Statistics of devfreq states >> @@ -167,6 +170,10 @@ struct devfreq { >> unsigned long scaling_max_freq; >> bool stop_polling; >> >> + unsigned long suspend_freq; >> + unsigned long resume_freq; >> + atomic_t suspend_count; >> + >> /* information for device frequency transition */ >> unsigned int total_trans; >> unsigned int *trans_table; >> > > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/5] devfreq: add support for suspend/resume of a devfreq device 2018-12-04 5:36 ` Chanwoo Choi 2018-12-04 5:43 ` Chanwoo Choi 2018-12-04 9:39 ` Lukasz Luba @ 2018-12-09 9:00 ` Pavel Machek 2 siblings, 0 replies; 21+ messages in thread From: Pavel Machek @ 2018-12-09 9:00 UTC (permalink / raw) To: Chanwoo Choi Cc: Lukasz Luba, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, devicetree, tjakobi, myungjoo.ham, kyungmin.park, rjw, len.brown, gregkh, keescook, anton, ccross, tony.luck, robh+dt, mark.rutland, kgene, krzk, m.szyprowski, b.zolnierkie [-- Attachment #1: Type: text/plain, Size: 956 bytes --] On Tue 2018-12-04 14:36:11, Chanwoo Choi wrote: > Hi Lukasz, > > Looks good to me. But, I add the some comments. > If you will fix it, feel free to add my tag: > Reviewed-by: Chanwoo choi <cw00.choi@samsung.com> > > On 2018년 12월 03일 23:31, Lukasz Luba wrote: > > The patch prepares devfreq device for handling suspend/resume > > functionality. The new fields will store needed information during this > > nitpick. Remove unneeded space. There are two spaces between '.' and 'The new'. Yes. And that's because two spaces are okay at that place. https://www.writersdigest.com/online-editor/how-many-spaces-after-a-period Please don't cause unneeded noise on the list with trivial comments. If the patch is okay, apply the patch, no need to find trivial nit everywhere. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
[parent not found: <CGME20181203143132eucas1p128c029a7c7461e1127924a08e4a71811@eucas1p1.samsung.com>]
* [PATCH v2 3/5] devfreq: add devfreq_suspend/resume() functions [not found] ` <CGME20181203143132eucas1p128c029a7c7461e1127924a08e4a71811@eucas1p1.samsung.com> @ 2018-12-03 14:31 ` Lukasz Luba 2018-12-04 6:19 ` Chanwoo Choi 0 siblings, 1 reply; 21+ messages in thread From: Lukasz Luba @ 2018-12-03 14:31 UTC (permalink / raw) To: linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, devicetree Cc: tjakobi, myungjoo.ham, kyungmin.park, cw00.choi, rjw, len.brown, pavel, gregkh, keescook, anton, ccross, tony.luck, robh+dt, mark.rutland, kgene, krzk, m.szyprowski, b.zolnierkie, Lukasz Luba This patch adds implementation for global suspend/resume for devfreq framework. System suspend will next use these functions. The patch is based on earlier work by Tobias Jakobi. Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> --- drivers/devfreq/devfreq.c | 42 ++++++++++++++++++++++++++++++++++++++++++ include/linux/devfreq.h | 6 ++++++ 2 files changed, 48 insertions(+) diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 36bed24..7d60423 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -935,6 +935,48 @@ int devfreq_resume_device(struct devfreq *devfreq) EXPORT_SYMBOL(devfreq_resume_device); /** + * devfreq_suspend() - Suspend devfreq governors and devices + * + * Called during system wide Suspend/Hibernate cycles for suspending governors + * and devices preserving the state for resume. On some platforms the devfreq + * device must have precise state (frequency) after resume in order to provide + * fully operating setup. + */ +void devfreq_suspend(void) +{ + struct devfreq *devfreq; + int ret; + + mutex_lock(&devfreq_list_lock); + list_for_each_entry(devfreq, &devfreq_list, node) { + ret = devfreq_suspend_device(devfreq); + if (ret) + dev_warn(&devfreq->dev, "device suspend failed\n"); + } + mutex_unlock(&devfreq_list_lock); +} + +/** + * devfreq_resume() - Resume devfreq governors and devices + * + * Called during system wide Suspend/Hibernate cycle for resuming governors and + * devices that are suspended with devfreq_suspend(). + */ +void devfreq_resume(void) +{ + struct devfreq *devfreq; + int ret; + + mutex_lock(&devfreq_list_lock); + list_for_each_entry(devfreq, &devfreq_list, node) { + ret = devfreq_resume_device(devfreq); + if (ret) + dev_warn(&devfreq->dev, "device resume failed\n"); + } + mutex_unlock(&devfreq_list_lock); +} + +/** * devfreq_add_governor() - Add devfreq governor * @governor: the devfreq governor to be added */ diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h index d985199..fbffa74 100644 --- a/include/linux/devfreq.h +++ b/include/linux/devfreq.h @@ -205,6 +205,9 @@ extern void devm_devfreq_remove_device(struct device *dev, extern int devfreq_suspend_device(struct devfreq *devfreq); extern int devfreq_resume_device(struct devfreq *devfreq); +extern void devfreq_suspend(void); +extern void devfreq_resume(void); + /** * update_devfreq() - Reevaluate the device and configure frequency * @devfreq: the devfreq device @@ -331,6 +334,9 @@ static inline int devfreq_resume_device(struct devfreq *devfreq) return 0; } +static inline void devfreq_suspend(void) {} +static inline void devfreq_resume(void) {} + static inline struct dev_pm_opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq, u32 flags) { -- 2.7.4 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/5] devfreq: add devfreq_suspend/resume() functions 2018-12-03 14:31 ` [PATCH v2 3/5] devfreq: add devfreq_suspend/resume() functions Lukasz Luba @ 2018-12-04 6:19 ` Chanwoo Choi 2018-12-04 9:44 ` Lukasz Luba 0 siblings, 1 reply; 21+ messages in thread From: Chanwoo Choi @ 2018-12-04 6:19 UTC (permalink / raw) To: Lukasz Luba, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, devicetree Cc: tjakobi, myungjoo.ham, kyungmin.park, rjw, len.brown, pavel, gregkh, keescook, anton, ccross, tony.luck, robh+dt, mark.rutland, kgene, krzk, m.szyprowski, b.zolnierkie Hi Lukasz, On 2018년 12월 03일 23:31, Lukasz Luba wrote: > This patch adds implementation for global suspend/resume for > devfreq framework. System suspend will next use these functions. > > The patch is based on earlier work by Tobias Jakobi. Please remove it from each patch description. > > Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> > Suggested-by: Chanwoo Choi <cw00.choi@samsung.com> > Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> > --- > drivers/devfreq/devfreq.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > include/linux/devfreq.h | 6 ++++++ > 2 files changed, 48 insertions(+) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 36bed24..7d60423 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -935,6 +935,48 @@ int devfreq_resume_device(struct devfreq *devfreq) > EXPORT_SYMBOL(devfreq_resume_device); > > /** > + * devfreq_suspend() - Suspend devfreq governors and devices > + * > + * Called during system wide Suspend/Hibernate cycles for suspending governors > + * and devices preserving the state for resume. On some platforms the devfreq > + * device must have precise state (frequency) after resume in order to provide > + * fully operating setup. > + */ > +void devfreq_suspend(void) > +{ > + struct devfreq *devfreq; > + int ret; > + > + mutex_lock(&devfreq_list_lock); > + list_for_each_entry(devfreq, &devfreq_list, node) { > + ret = devfreq_suspend_device(devfreq); > + if (ret) > + dev_warn(&devfreq->dev, "device suspend failed\n"); When I checked the cpufreq_suspend(), cpufreq_suspend() prints message as 'err' level. I think that dev_err is more proper than dev_warn. I'm not sure what is more correct log. But, 'devfreq->dev' device has the separate suspend/resume function. So, I think that devfreq_suspend() should print error log containing that it is error by devfreq framework. "device suspend failed" -> "failed to suspend devfreq device" > + } > + mutex_unlock(&devfreq_list_lock); > +} > + > +/** > + * devfreq_resume() - Resume devfreq governors and devices > + * > + * Called during system wide Suspend/Hibernate cycle for resuming governors and > + * devices that are suspended with devfreq_suspend(). > + */ > +void devfreq_resume(void) > +{ > + struct devfreq *devfreq; > + int ret; > + > + mutex_lock(&devfreq_list_lock); > + list_for_each_entry(devfreq, &devfreq_list, node) { > + ret = devfreq_resume_device(devfreq); > + if (ret) > + dev_warn(&devfreq->dev, "device resume failed\n"); ditto. "device resume failed" -> "failed to resume devfreq device" > + } > + mutex_unlock(&devfreq_list_lock); > +} > + > +/** > * devfreq_add_governor() - Add devfreq governor > * @governor: the devfreq governor to be added > */ > diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h > index d985199..fbffa74 100644 > --- a/include/linux/devfreq.h > +++ b/include/linux/devfreq.h > @@ -205,6 +205,9 @@ extern void devm_devfreq_remove_device(struct device *dev, > extern int devfreq_suspend_device(struct devfreq *devfreq); > extern int devfreq_resume_device(struct devfreq *devfreq); > > +extern void devfreq_suspend(void); > +extern void devfreq_resume(void); > + > /** > * update_devfreq() - Reevaluate the device and configure frequency > * @devfreq: the devfreq device > @@ -331,6 +334,9 @@ static inline int devfreq_resume_device(struct devfreq *devfreq) > return 0; > } > > +static inline void devfreq_suspend(void) {} > +static inline void devfreq_resume(void) {} > + > static inline struct dev_pm_opp *devfreq_recommended_opp(struct device *dev, > unsigned long *freq, u32 flags) > { > -- Best Regards, Chanwoo Choi Samsung Electronics ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/5] devfreq: add devfreq_suspend/resume() functions 2018-12-04 6:19 ` Chanwoo Choi @ 2018-12-04 9:44 ` Lukasz Luba 0 siblings, 0 replies; 21+ messages in thread From: Lukasz Luba @ 2018-12-04 9:44 UTC (permalink / raw) To: Chanwoo Choi, linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, devicetree Cc: tjakobi, myungjoo.ham, kyungmin.park, rjw, len.brown, pavel, gregkh, keescook, anton, ccross, tony.luck, robh+dt, mark.rutland, kgene, krzk, m.szyprowski, b.zolnierkie Hi Chanwoo, On 12/4/18 7:19 AM, Chanwoo Choi wrote: > Hi Lukasz, > > On 2018년 12월 03일 23:31, Lukasz Luba wrote: >> This patch adds implementation for global suspend/resume for >> devfreq framework. System suspend will next use these functions. >> >> The patch is based on earlier work by Tobias Jakobi. > > Please remove it from each patch description. OK > >> >> Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com> >> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> >> --- >> drivers/devfreq/devfreq.c | 42 ++++++++++++++++++++++++++++++++++++++++++ >> include/linux/devfreq.h | 6 ++++++ >> 2 files changed, 48 insertions(+) >> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >> index 36bed24..7d60423 100644 >> --- a/drivers/devfreq/devfreq.c >> +++ b/drivers/devfreq/devfreq.c >> @@ -935,6 +935,48 @@ int devfreq_resume_device(struct devfreq *devfreq) >> EXPORT_SYMBOL(devfreq_resume_device); >> >> /** >> + * devfreq_suspend() - Suspend devfreq governors and devices >> + * >> + * Called during system wide Suspend/Hibernate cycles for suspending governors >> + * and devices preserving the state for resume. On some platforms the devfreq >> + * device must have precise state (frequency) after resume in order to provide >> + * fully operating setup. >> + */ >> +void devfreq_suspend(void) >> +{ >> + struct devfreq *devfreq; >> + int ret; >> + >> + mutex_lock(&devfreq_list_lock); >> + list_for_each_entry(devfreq, &devfreq_list, node) { >> + ret = devfreq_suspend_device(devfreq); >> + if (ret) >> + dev_warn(&devfreq->dev, "device suspend failed\n"); > > When I checked the cpufreq_suspend(), cpufreq_suspend() prints message as 'err' level. > I think that dev_err is more proper than dev_warn. > > I'm not sure what is more correct log. > But, 'devfreq->dev' device has the separate suspend/resume function. > So, I think that devfreq_suspend() should print error log containing > that it is error by devfreq framework. > > "device suspend failed" > -> "failed to suspend devfreq device" OK, changed in next v3 patch set. > >> + } >> + mutex_unlock(&devfreq_list_lock); >> +} >> + >> +/** >> + * devfreq_resume() - Resume devfreq governors and devices >> + * >> + * Called during system wide Suspend/Hibernate cycle for resuming governors and >> + * devices that are suspended with devfreq_suspend(). >> + */ >> +void devfreq_resume(void) >> +{ >> + struct devfreq *devfreq; >> + int ret; >> + >> + mutex_lock(&devfreq_list_lock); >> + list_for_each_entry(devfreq, &devfreq_list, node) { >> + ret = devfreq_resume_device(devfreq); >> + if (ret) >> + dev_warn(&devfreq->dev, "device resume failed\n"); > > ditto. > > "device resume failed" > -> "failed to resume devfreq device" > ACK Regards, Lukasz > >> + } >> + mutex_unlock(&devfreq_list_lock); >> +} >> + >> +/** >> * devfreq_add_governor() - Add devfreq governor >> * @governor: the devfreq governor to be added >> */ >> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h >> index d985199..fbffa74 100644 >> --- a/include/linux/devfreq.h >> +++ b/include/linux/devfreq.h >> @@ -205,6 +205,9 @@ extern void devm_devfreq_remove_device(struct device *dev, >> extern int devfreq_suspend_device(struct devfreq *devfreq); >> extern int devfreq_resume_device(struct devfreq *devfreq); >> >> +extern void devfreq_suspend(void); >> +extern void devfreq_resume(void); >> + >> /** >> * update_devfreq() - Reevaluate the device and configure frequency >> * @devfreq: the devfreq device >> @@ -331,6 +334,9 @@ static inline int devfreq_resume_device(struct devfreq *devfreq) >> return 0; >> } >> >> +static inline void devfreq_suspend(void) {} >> +static inline void devfreq_resume(void) {} >> + >> static inline struct dev_pm_opp *devfreq_recommended_opp(struct device *dev, >> unsigned long *freq, u32 flags) >> { >> > ^ permalink raw reply [flat|nested] 21+ messages in thread
[parent not found: <CGME20181203143134eucas1p1edd70b0f4dca115f6f154555d8829427@eucas1p1.samsung.com>]
* [PATCH v2 4/5] drivers: power: suspend: call devfreq suspend/resume [not found] ` <CGME20181203143134eucas1p1edd70b0f4dca115f6f154555d8829427@eucas1p1.samsung.com> @ 2018-12-03 14:31 ` Lukasz Luba 0 siblings, 0 replies; 21+ messages in thread From: Lukasz Luba @ 2018-12-03 14:31 UTC (permalink / raw) To: linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, devicetree Cc: tjakobi, myungjoo.ham, kyungmin.park, cw00.choi, rjw, len.brown, pavel, gregkh, keescook, anton, ccross, tony.luck, robh+dt, mark.rutland, kgene, krzk, m.szyprowski, b.zolnierkie, Lukasz Luba Devfreq framework supports suspend of its devices. Call the the devfreq interface and allow devfreq devices preserve/restore their states during suspend/resume. The patch is based on earlier work by Tobias Jakobi. Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> --- drivers/base/power/main.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c index a690fd4..0992e67 100644 --- a/drivers/base/power/main.c +++ b/drivers/base/power/main.c @@ -32,6 +32,7 @@ #include <trace/events/power.h> #include <linux/cpufreq.h> #include <linux/cpuidle.h> +#include <linux/devfreq.h> #include <linux/timer.h> #include "../base.h" @@ -1078,6 +1079,7 @@ void dpm_resume(pm_message_t state) dpm_show_time(starttime, state, 0, NULL); cpufreq_resume(); + devfreq_resume(); trace_suspend_resume(TPS("dpm_resume"), state.event, false); } @@ -1852,6 +1854,7 @@ int dpm_suspend(pm_message_t state) trace_suspend_resume(TPS("dpm_suspend"), state.event, true); might_sleep(); + devfreq_suspend(); cpufreq_suspend(); mutex_lock(&dpm_list_mtx); -- 2.7.4 ^ permalink raw reply related [flat|nested] 21+ messages in thread
[parent not found: <CGME20181203143135eucas1p165fe6183ae90de7906f9683cb41ff4c1@eucas1p1.samsung.com>]
* [PATCH v2 5/5] arm: dts: exynos4: opp-suspend in DMC and leftbus [not found] ` <CGME20181203143135eucas1p165fe6183ae90de7906f9683cb41ff4c1@eucas1p1.samsung.com> @ 2018-12-03 14:31 ` Lukasz Luba 2018-12-03 17:22 ` Krzysztof Kozlowski 0 siblings, 1 reply; 21+ messages in thread From: Lukasz Luba @ 2018-12-03 14:31 UTC (permalink / raw) To: linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, devicetree Cc: tjakobi, myungjoo.ham, kyungmin.park, cw00.choi, rjw, len.brown, pavel, gregkh, keescook, anton, ccross, tony.luck, robh+dt, mark.rutland, kgene, krzk, m.szyprowski, b.zolnierkie, Lukasz Luba Mark the state for devfreq device while entring suspend/resume process. The patch is based on earlier work by Tobias Jakobi. Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> --- arch/arm/boot/dts/exynos4210.dtsi | 2 ++ arch/arm/boot/dts/exynos4412.dtsi | 2 ++ 2 files changed, 4 insertions(+) diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi index b6091c2..4429b72 100644 --- a/arch/arm/boot/dts/exynos4210.dtsi +++ b/arch/arm/boot/dts/exynos4210.dtsi @@ -298,6 +298,7 @@ opp-400000000 { opp-hz = /bits/ 64 <400000000>; opp-microvolt = <1150000>; + opp-suspend; }; }; @@ -367,6 +368,7 @@ }; opp-200000000 { opp-hz = /bits/ 64 <200000000>; + opp-suspend; }; }; }; diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi index 51f72f0..908c0c4 100644 --- a/arch/arm/boot/dts/exynos4412.dtsi +++ b/arch/arm/boot/dts/exynos4412.dtsi @@ -432,6 +432,7 @@ opp-400000000 { opp-hz = /bits/ 64 <400000000>; opp-microvolt = <1050000>; + opp-suspend; }; }; @@ -520,6 +521,7 @@ opp-200000000 { opp-hz = /bits/ 64 <200000000>; opp-microvolt = <1000000>; + opp-suspend; }; }; -- 2.7.4 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 5/5] arm: dts: exynos4: opp-suspend in DMC and leftbus 2018-12-03 14:31 ` [PATCH v2 5/5] arm: dts: exynos4: opp-suspend in DMC and leftbus Lukasz Luba @ 2018-12-03 17:22 ` Krzysztof Kozlowski 2018-12-03 17:48 ` Lukasz Luba 0 siblings, 1 reply; 21+ messages in thread From: Krzysztof Kozlowski @ 2018-12-03 17:22 UTC (permalink / raw) To: Lukasz Luba Cc: linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, devicetree, tjakobi, myungjoo.ham, kyungmin.park, cw00.choi, rjw, len.brown, pavel, gregkh, keescook, anton, ccross, tony.luck, robh+dt, mark.rutland, kgene, m.szyprowski, b.zolnierkie On Mon, Dec 03, 2018 at 03:31:15PM +0100, Lukasz Luba wrote: > Mark the state for devfreq device while entring suspend/resume process. > > The patch is based on earlier work by Tobias Jakobi. > > Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> > Suggested-by: Chanwoo Choi <cw00.choi@samsung.com> > Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> > Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> > --- > arch/arm/boot/dts/exynos4210.dtsi | 2 ++ > arch/arm/boot/dts/exynos4412.dtsi | 2 ++ > 2 files changed, 4 insertions(+) Thanks, applied with some minor commit msg changes. In general, please take care about title prefix (git log --oneline arch/arm/boot/dts/exynos*) and always explain why you are doing this. You just mentioned "what" but that is pretty obvious by looking at commit contents. The commit msg should answer why these should be marked as opp-suspend and why these values were chosen. The cover letter just briefly describes "issue with devfreq devices' frequency during suspend/resume"... but what issue? Best regards, Krzysztof > > diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi > index b6091c2..4429b72 100644 > --- a/arch/arm/boot/dts/exynos4210.dtsi > +++ b/arch/arm/boot/dts/exynos4210.dtsi > @@ -298,6 +298,7 @@ > opp-400000000 { > opp-hz = /bits/ 64 <400000000>; > opp-microvolt = <1150000>; > + opp-suspend; > }; > }; > > @@ -367,6 +368,7 @@ > }; > opp-200000000 { > opp-hz = /bits/ 64 <200000000>; > + opp-suspend; > }; > }; > }; > diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi > index 51f72f0..908c0c4 100644 > --- a/arch/arm/boot/dts/exynos4412.dtsi > +++ b/arch/arm/boot/dts/exynos4412.dtsi > @@ -432,6 +432,7 @@ > opp-400000000 { > opp-hz = /bits/ 64 <400000000>; > opp-microvolt = <1050000>; > + opp-suspend; > }; > }; > > @@ -520,6 +521,7 @@ > opp-200000000 { > opp-hz = /bits/ 64 <200000000>; > opp-microvolt = <1000000>; > + opp-suspend; > }; > }; > > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 5/5] arm: dts: exynos4: opp-suspend in DMC and leftbus 2018-12-03 17:22 ` Krzysztof Kozlowski @ 2018-12-03 17:48 ` Lukasz Luba 0 siblings, 0 replies; 21+ messages in thread From: Lukasz Luba @ 2018-12-03 17:48 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: linux-arm-kernel, linux-samsung-soc, linux-kernel, linux-pm, devicetree, tjakobi, myungjoo.ham, kyungmin.park, cw00.choi, rjw, len.brown, pavel, gregkh, keescook, anton, ccross, tony.luck, robh+dt, mark.rutland, kgene, m.szyprowski, b.zolnierkie Hi Krzysztof, On 12/3/18 6:22 PM, Krzysztof Kozlowski wrote: > On Mon, Dec 03, 2018 at 03:31:15PM +0100, Lukasz Luba wrote: >> Mark the state for devfreq device while entring suspend/resume process. >> >> The patch is based on earlier work by Tobias Jakobi. >> >> Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com> >> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> >> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> >> --- >> arch/arm/boot/dts/exynos4210.dtsi | 2 ++ >> arch/arm/boot/dts/exynos4412.dtsi | 2 ++ >> 2 files changed, 4 insertions(+) > > Thanks, applied with some minor commit msg changes. In general, please > take care about title prefix (git log --oneline > arch/arm/boot/dts/exynos*) and always explain why you are doing this. Thank you that you have applied and for the hint. > You just mentioned "what" but that is pretty obvious by looking at > commit contents. The commit msg should answer why these should be marked > as opp-suspend and why these values were chosen. > > The cover letter just briefly describes "issue with devfreq devices' frequency > during suspend/resume"... but what issue? In the cover letter there is sentence: 'The suspending device has now chance to set proper state when the system is going for suspend. This phase is the right place to set needed frequences for the next resume process.' Generally speaking, there is a need of setting the right frequency/voltage, because we need that frequency during resume, i.e. for booting CPUs (which are poked earlier during resume than the buses in this design). Regards, Lukasz > > Best regards, > Krzysztof > >> >> diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi >> index b6091c2..4429b72 100644 >> --- a/arch/arm/boot/dts/exynos4210.dtsi >> +++ b/arch/arm/boot/dts/exynos4210.dtsi >> @@ -298,6 +298,7 @@ >> opp-400000000 { >> opp-hz = /bits/ 64 <400000000>; >> opp-microvolt = <1150000>; >> + opp-suspend; >> }; >> }; >> >> @@ -367,6 +368,7 @@ >> }; >> opp-200000000 { >> opp-hz = /bits/ 64 <200000000>; >> + opp-suspend; >> }; >> }; >> }; >> diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi >> index 51f72f0..908c0c4 100644 >> --- a/arch/arm/boot/dts/exynos4412.dtsi >> +++ b/arch/arm/boot/dts/exynos4412.dtsi >> @@ -432,6 +432,7 @@ >> opp-400000000 { >> opp-hz = /bits/ 64 <400000000>; >> opp-microvolt = <1050000>; >> + opp-suspend; >> }; >> }; >> >> @@ -520,6 +521,7 @@ >> opp-200000000 { >> opp-hz = /bits/ 64 <200000000>; >> opp-microvolt = <1000000>; >> + opp-suspend; >> }; >> }; >> >> -- >> 2.7.4 >> > > ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2018-12-09 9:00 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CGME20181203143127eucas1p2604fc066139a32fdffe996212b42b40e@eucas1p2.samsung.com> 2018-12-03 14:31 ` [PATCH v2 0/5] devfreq: handle suspend/resume Lukasz Luba [not found] ` <CGME20181203143129eucas1p2955b6becc60ee57110cbc52f6e4f60c5@eucas1p2.samsung.com> 2018-12-03 14:31 ` [PATCH v2 1/5] devfreq: refactor set_target frequency function Lukasz Luba 2018-12-04 4:39 ` Chanwoo Choi 2018-12-04 5:43 ` Chanwoo Choi 2018-12-04 9:36 ` Lukasz Luba [not found] ` <CGME20181203143131eucas1p217f22ac6d19682a54a57658a06980914@eucas1p2.samsung.com> 2018-12-03 14:31 ` [PATCH v2 2/5] devfreq: add support for suspend/resume of a devfreq device Lukasz Luba 2018-12-04 5:36 ` Chanwoo Choi 2018-12-04 5:43 ` Chanwoo Choi 2018-12-04 6:10 ` Chanwoo Choi 2018-12-04 9:53 ` Lukasz Luba 2018-12-05 0:09 ` Chanwoo Choi 2018-12-05 11:07 ` Lukasz Luba 2018-12-04 9:39 ` Lukasz Luba 2018-12-09 9:00 ` Pavel Machek [not found] ` <CGME20181203143132eucas1p128c029a7c7461e1127924a08e4a71811@eucas1p1.samsung.com> 2018-12-03 14:31 ` [PATCH v2 3/5] devfreq: add devfreq_suspend/resume() functions Lukasz Luba 2018-12-04 6:19 ` Chanwoo Choi 2018-12-04 9:44 ` Lukasz Luba [not found] ` <CGME20181203143134eucas1p1edd70b0f4dca115f6f154555d8829427@eucas1p1.samsung.com> 2018-12-03 14:31 ` [PATCH v2 4/5] drivers: power: suspend: call devfreq suspend/resume Lukasz Luba [not found] ` <CGME20181203143135eucas1p165fe6183ae90de7906f9683cb41ff4c1@eucas1p1.samsung.com> 2018-12-03 14:31 ` [PATCH v2 5/5] arm: dts: exynos4: opp-suspend in DMC and leftbus Lukasz Luba 2018-12-03 17:22 ` Krzysztof Kozlowski 2018-12-03 17:48 ` Lukasz Luba
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).