Re: [PATCH v1 & v6 1/2] PM/devfreq: add suspend frequency support

[hl]

Hi Chanwoo Choi,


On 2016å11æ24æ 16:16, Chanwoo Choi wrote:
> Hi Lin,
>
> On 2016ë 11ì 24ì 16:34, hl wrote:
> > Hi Chanwoo Choi,
> >
> >      I think the dev_pm_opp_get_suspend_opp() have implement most of
> > the funtion, all we need is just define the node in dts, like following:
> >
> > &dmc_opp_table {
> >      opp06 {
> >          opp-suspend;
> >      };
> > };
> Two approaches use the 'opp-suspend' property.
>
> I think that the method to support suspend-opp have to
> guarantee following conditions:
> - Support the all of devfreq's governors.
    As MyungJoo Ham suggestion, i will set the suspend frequency in 
devfreq_suspend_device(),
which will ingore governor.
> - Devfreq framework have the responsibility to change the
>    frequency/voltage for suspend-opp. If we uses the
>    new devfreq_suspend(), each devfreq device don't care
>    how to support the suspend-opp. Just the developer of each
>    devfreq device need to add 'opp-suspend' propet to OPP entry in DT file.
Why should support change the voltage in devfreq framework, i think it 
shuold be handle in
specific driver, i think the devfreq only handle it can get the right 
frequency, then pass it to
specific driver, i think the voltage should handle in the 
devfreq->profile->target();
> Best Regards,
> Chanwoo Choi
>
> > so i think my way semm more simple.
> >
> > On 2016å11æ24æ 15:10, Chanwoo Choi wrote:
> > > + Tobias Jakobi,
> > >
> > > Hi Lin,
> > >
> > > We need to discuss how to support the suspend-opp of devfreq device.
> > > Now, there are two patch thread for suspend-opp of devfreq.
> > >
> > > The Lin's approach modify the devfreq_suspend_device() to support suspend-opp.
> > > The Tobias's approach[1] add new devfreq_suspend() and then call it on dpm_suspend()
> > > when entering the suspend state.
> > >
> > > [1] [RFC 0/4] PM / devfreq: draft for OPP suspend impl
> > > - https://patchwork.kernel.org/patch/9443323/
> > > - https://patchwork.kernel.org/patch/9443325/
> > > - https://patchwork.kernel.org/patch/9443329/
> > > - https://patchwork.kernel.org/patch/9443331/
> > >
> > > I think we need to discuss it together.
> > >
> > > Regards,
> > > Chanwoo Choi
> > >
> > > On 2016ë 11ì 24ì 15:45, hl wrote:
> > > > Hi MyungJoo Ham,
> > > >
> > > > On 2016å11æ24æ 14:14, MyungJoo Ham wrote:
> > > > > On Thu, Nov 24, 2016 at 11:18 AM, hl <hl@xxxxxxxxxxxxxx> wrote:
> > > > > > Hi MyungJoo Ham,
> > > > > []
> > > > > > > We still need to sync the all status even i call target() in
> > > > > > > devfreq_suspend/resume_device
> > > > > > > directly, so still need update_devfreq() other setp except
> > > > > > > devfreq->governor->get_target_freq(devfreq, &freq);
> > > > > > And i think it better to be governor behaviors, for userspace they may not
> > > > > > want to change
> > > > > > the suspend frequency like other governor, the frequency should decide by
> > > > > > the user, if they
> > > > > > want this function, they should like other governor to rigister a
> > > > > > devfreq_monitor_suspend().
> > > > > > What do you think about my rev6 patch?
> > > > > If I understand the intention correctly, this is for the stability of
> > > > > the device due to the behavior or bootloader/SoC-initializer, which
> > > > > has nothing to do with governors.
> > > > >
> > > > > Even if users are using userspace, as long as they set the custom
> > > > > frequencies lower than the default, they have the possibility of
> > > > > being unstable as ondemand is going to have.
> > > > >
> > > > >
> > > > > To reuse the update_devfreq() code, you may do something like:
> > > > >
> > > > > static int _update_freq(struct devfreq *devfreq, bool is_suspending)
> > > > > {
> > > > >       /* original contents of update_freq with if statement with is_suspending wrapping get_target_freq */
> > > > > }
> > > > > int update_freq(struct devfreq *devfreq)
> > > > > {
> > > > >      return _update_freq(devfreq, false);
> > > > > }
> > > > >
> > > > >
> > > > > There should be other good non-invasive methods that are not governoe-specific as well.
> > > > Thanks for your suggestion, i will update the new version soon.
> > > > > Cheers,
> > > > > MyungJoo
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > _______________________________________________
> > > > > Linux-rockchip mailing list
> > > > > Linux-rockchip@xxxxxxxxxxxxxxxxxxx
> > > > > http://lists.infradead.org/mailman/listinfo/linux-rockchip
> > > > --
> > > > Lin Huang

--
Lin Huang