Re: Re: [PATCH v4 2/3] devfreq: Add suspend and resume apis

Re: Re: [PATCH v4 2/3] devfreq: Add suspend and resume apis

[MyungJoo Ham]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=euc-kr, Size: 6064 bytes --]

> On 8 October 2012 03:31, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Thursday 04 of October 2012 14:58:33 Rajagopal Venkat wrote:
> >> Add devfreq suspend/resume apis for devfreq users. This patch
> >> supports suspend and resume of devfreq load monitoring, required
> >> for devices which can idle.
> >>
> >> Signed-off-by: Rajagopal Venkat <rajagopal.venkat@linaro.org>
> >> Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> >
> > Well, I wonder if this may be tied in to the runtime PM framework, so that,
> > for example, pm_runtime_suspend() will automatically suspend devfreq on
> > success (and the runtime resume of the device will resume devfreq)?
> 
> That's a good idea. If you agree, we can handle this as separate patch on
> top this patchset.

I guess implementing the idea may be not so obvious; thus, seperating
the patchset would be appropriate.

When we implement the idea, we may need to implement at the
pm_runtime core. Because devfreq->dev is a child of the target dev, not
a parent, runtime-pm event of the target dev does not automatically
invoke a cascaded action on the devfreq->dev.

Maybe either
1) capability to allow a child to monitor the power state of a parent
(I remember Inki Dae once suggested to add notifiers at runtime-pm, but
it seems to be not merged)
or
2) letting runtime-pm be aware of devfreq
(doesn't feel alright with this??)
is required?



> 
> >
> > Rafael
> >
> >
> >> ---
> >>  drivers/devfreq/devfreq.c                 | 28 ++++++++++++++++++++++++++++
> >>  drivers/devfreq/governor.h                |  2 ++
> >>  drivers/devfreq/governor_simpleondemand.c |  9 +++++++++
> >>  include/linux/devfreq.h                   | 12 ++++++++++++
> >>  4 files changed, 51 insertions(+)
> >>
> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> >> index 41a4099..63e075e 100644
> >> --- a/drivers/devfreq/devfreq.c
> >> +++ b/drivers/devfreq/devfreq.c
> >> @@ -428,6 +428,34 @@ int devfreq_remove_device(struct devfreq *devfreq)
> >>  }
> >>  EXPORT_SYMBOL(devfreq_remove_device);
> >>
> >> +/**
> >> + * devfreq_suspend_device() - Suspend devfreq of a device.
> >> + * @devfreq  the devfreq instance to be suspended
> >> + */
> >> +int devfreq_suspend_device(struct devfreq *devfreq)
> >> +{
> >> +     if (!devfreq)
> >> +             return -EINVAL;
> >> +
> >> +     return devfreq->governor->event_handler(devfreq,
> >> +                             DEVFREQ_GOV_SUSPEND, NULL);
> >> +}
> >> +EXPORT_SYMBOL(devfreq_suspend_device);
> >> +
> >> +/**
> >> + * devfreq_resume_device() - Resume devfreq of a device.
> >> + * @devfreq  the devfreq instance to be resumed
> >> + */
> >> +int devfreq_resume_device(struct devfreq *devfreq)
> >> +{
> >> +     if (!devfreq)
> >> +             return -EINVAL;
> >> +
> >> +     return devfreq->governor->event_handler(devfreq,
> >> +                             DEVFREQ_GOV_RESUME, NULL);
> >> +}
> >> +EXPORT_SYMBOL(devfreq_resume_device);
> >> +
> >>  static ssize_t show_governor(struct device *dev,
> >>                            struct device_attribute *attr, char *buf)
> >>  {
> >> diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
> >> index bb3aff3..26432ac 100644
> >> --- a/drivers/devfreq/governor.h
> >> +++ b/drivers/devfreq/governor.h
> >> @@ -22,6 +22,8 @@
> >>  #define DEVFREQ_GOV_START                    0x1
> >>  #define DEVFREQ_GOV_STOP                     0x2
> >>  #define DEVFREQ_GOV_INTERVAL                 0x3
> >> +#define DEVFREQ_GOV_SUSPEND                  0x4
> >> +#define DEVFREQ_GOV_RESUME                   0x5
> >>
> >>  /* Caution: devfreq->lock must be locked before calling update_devfreq */
> >>  extern int update_devfreq(struct devfreq *devfreq);
> >> diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c
> >> index cf94218..a8ba78c 100644
> >> --- a/drivers/devfreq/governor_simpleondemand.c
> >> +++ b/drivers/devfreq/governor_simpleondemand.c
> >> @@ -104,6 +104,15 @@ int devfreq_simple_ondemand_handler(struct devfreq *devfreq,
> >>       case DEVFREQ_GOV_INTERVAL:
> >>               devfreq_interval_update(devfreq, (unsigned int *)data);
> >>               break;
> >> +
> >> +     case DEVFREQ_GOV_SUSPEND:
> >> +             devfreq_monitor_suspend(devfreq);
> >> +             break;
> >> +
> >> +     case DEVFREQ_GOV_RESUME:
> >> +             devfreq_monitor_resume(devfreq);
> >> +             break;
> >> +
> >>       default:
> >>               break;
> >>       }
> >> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> >> index 9cdffde..ee243a3 100644
> >> --- a/include/linux/devfreq.h
> >> +++ b/include/linux/devfreq.h
> >> @@ -158,6 +158,8 @@ extern struct devfreq *devfreq_add_device(struct device *dev,
> >>                                 const struct devfreq_governor *governor,
> >>                                 void *data);
> >>  extern int devfreq_remove_device(struct devfreq *devfreq);
> >> +extern int devfreq_suspend_device(struct devfreq *devfreq);
> >> +extern int devfreq_resume_device(struct devfreq *devfreq);
> >>
> >>  /* Helper functions for devfreq user device driver with OPP. */
> >>  extern struct opp *devfreq_recommended_opp(struct device *dev,
> >> @@ -211,6 +213,16 @@ static int devfreq_remove_device(struct devfreq *devfreq)
> >>       return 0;
> >>  }
> >>
> >> +static int devfreq_suspend_device(struct devfreq *devfreq)
> >> +{
> >> +     return 0;
> >> +}
> >> +
> >> +static int devfreq_resume_device(struct devfreq *devfreq)
> >> +{
> >> +     return 0;
> >> +}
> >> +
> >>  static struct opp *devfreq_recommended_opp(struct device *dev,
> >>                                          unsigned long *freq, u32 flags)
> >>  {
> >>
> > --
> > I speak only for myself.
> > Rafael J. Wysocki, Intel Open Source Technology Center.
> 
> 
> 
> -- 
> Regards,
> Rajagopal
> 
> 
> 
>        
>   
>          
> 
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: Re: [PATCH v4 2/3] devfreq: Add suspend and resume apis

[MyungJoo Ham]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=euc-kr, Size: 2915 bytes --]

> On Monday 08 of October 2012 10:48:24 MyungJoo Ham wrote:
> > > On 8 October 2012 03:31, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > > On Thursday 04 of October 2012 14:58:33 Rajagopal Venkat wrote:
> > > >> Add devfreq suspend/resume apis for devfreq users. This patch
> > > >> supports suspend and resume of devfreq load monitoring, required
> > > >> for devices which can idle.
> > > >>
> > > >> Signed-off-by: Rajagopal Venkat <rajagopal.venkat@linaro.org>
> > > >> Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> > > >
> > > > Well, I wonder if this may be tied in to the runtime PM framework, so that,
> > > > for example, pm_runtime_suspend() will automatically suspend devfreq on
> > > > success (and the runtime resume of the device will resume devfreq)?
> > > 
> > > That's a good idea. If you agree, we can handle this as separate patch on
> > > top this patchset.
> 
> Sure.
> 
> > I guess implementing the idea may be not so obvious; thus, seperating
> > the patchset would be appropriate.
> > 
> > When we implement the idea, we may need to implement at the
> > pm_runtime core. Because devfreq->dev is a child of the target dev, not
> > a parent, runtime-pm event of the target dev does not automatically
> > invoke a cascaded action on the devfreq->dev.
> 
> I'm not exactly sure what you mean, care to explain?

When a device "p" creates devfreq, devfreq->dev->parent = p.
And, devfreq's functions need to react to the "p"'s runtime-pm events.

However, when "p"'s runtime-pm-suspend is being invoked,
devfreq->dev's runtime-pm-suspend won't be automatically invoked.

Thus, we will need a mechanism that invokes devfreq->dev's runtime-pm
callbacks when p's runtime-pm is invoked. (at the runtime-pm core)
Or
A mechanism that one can notify others (including its children) when
the one's runtime-pm is invoked. (probably at the runtime-pm core, too)

Without such support, it appears that the current implementation
(calling runtime-pm suspend/resume equivalent devfreq functions
manually at device drivers) seems to be inevitable.



Anyway, if devfreq->dev is a parent of "p", runtime-pm core will
do the required task automatically; however, it isn't and I don't
think it'd be appropriate.

> 
> > Maybe either
> > 1) capability to allow a child to monitor the power state of a parent
> > (I remember Inki Dae once suggested to add notifiers at runtime-pm, but
> > it seems to be not merged)
> > or
> > 2) letting runtime-pm be aware of devfreq
> > (doesn't feel alright with this??)
> > is required?
> 
> I'm not a big fan of notifiers, so I'd prefer to avoid using them, if
> possible.
> 
> Thanks,
> Rafael
> 
> 
> -- 
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
> 

Cheers,
MyungJoo
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v4 2/3] devfreq: Add suspend and resume apis

[Rafael J. Wysocki]

On Tuesday 16 of October 2012 06:40:19 MyungJoo Ham wrote:
> > On Monday 08 of October 2012 10:48:24 MyungJoo Ham wrote:
> > > > On 8 October 2012 03:31, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > > > On Thursday 04 of October 2012 14:58:33 Rajagopal Venkat wrote:
> > > > >> Add devfreq suspend/resume apis for devfreq users. This patch
> > > > >> supports suspend and resume of devfreq load monitoring, required
> > > > >> for devices which can idle.
> > > > >>
> > > > >> Signed-off-by: Rajagopal Venkat <rajagopal.venkat@linaro.org>
> > > > >> Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> > > > >
> > > > > Well, I wonder if this may be tied in to the runtime PM framework, so that,
> > > > > for example, pm_runtime_suspend() will automatically suspend devfreq on
> > > > > success (and the runtime resume of the device will resume devfreq)?
> > > > 
> > > > That's a good idea. If you agree, we can handle this as separate patch on
> > > > top this patchset.
> > 
> > Sure.
> > 
> > > I guess implementing the idea may be not so obvious; thus, seperating
> > > the patchset would be appropriate.
> > > 
> > > When we implement the idea, we may need to implement at the
> > > pm_runtime core. Because devfreq->dev is a child of the target dev, not
> > > a parent, runtime-pm event of the target dev does not automatically
> > > invoke a cascaded action on the devfreq->dev.
> > 
> > I'm not exactly sure what you mean, care to explain?
> 
> When a device "p" creates devfreq, devfreq->dev->parent = p.

This is wrong.  It shouldn't have been designed this way in the first place
and it was my mistake to merge it, apparently.

> And, devfreq's functions need to react to the "p"'s runtime-pm events.
>
> However, when "p"'s runtime-pm-suspend is being invoked,
> devfreq->dev's runtime-pm-suspend won't be automatically invoked.

Well, the parent should not be able to suspend at all before the children
have suspended and your design doesn't seem to match that principle.

> Thus, we will need a mechanism that invokes devfreq->dev's runtime-pm
> callbacks when p's runtime-pm is invoked. (at the runtime-pm core)

No, this would be going backwards.

> Or
> A mechanism that one can notify others (including its children) when
> the one's runtime-pm is invoked. (probably at the runtime-pm core, too)

You don't appear to understand how the runtime PM framework works.

There's no need to notify a child about the parent's runtime suspend,
because normally the parent won't be suspended before all of its children
have been suspended.

> Without such support, it appears that the current implementation
> (calling runtime-pm suspend/resume equivalent devfreq functions
> manually at device drivers) seems to be inevitable.

Which only means that the devfreq design in incorrect, so please fix it.

> Anyway, if devfreq->dev is a parent of "p", runtime-pm core will
> do the required task automatically; however, it isn't and I don't
> think it'd be appropriate.

No, it wouldn't.  Actually, I'm not sure what you need the dev in struct
devfreq for at all.

As for finding the given device's devfreq structure, for now we can just
walk the devfreq_list.  To avoid doing that for devices that don't have a
struct devfreq attached to them, we can add a devfreq_in_use flag in
struct dev_pm_info.  That may even help some functions in devfreq.c as far
as I can say.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.