Hi,
On Mon, Apr 29, 2019 at 4:30 AM Artur Petrosyan
<Arthur.Petrosyan@xxxxxxxxxxxx> wrote:
>
> Hi,
>
> On 4/27/2019 00:46, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Apr 19, 2019 at 11:53 AM Artur Petrosyan
> > <Arthur.Petrosyan@xxxxxxxxxxxx> wrote:
> >>
> >> - Added a default param "power_saving" to enable or
> >> disable hibernation or partial power down features.
> >>
> >> - Printed hibernation param in hw_params_show and
> >> power_saving param in params_show.
> >>
> >> Signed-off-by: Artur Petrosyan <arturp@xxxxxxxxxxxx>
> >> Signed-off-by: Minas Harutyunyan <hminas@xxxxxxxxxxxx>
> >> ---
> >> drivers/usb/dwc2/core.h | 3 +++
> >> drivers/usb/dwc2/debugfs.c | 2 ++
> >> drivers/usb/dwc2/params.c | 19 +++++++++++++------
> >> 3 files changed, 18 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> >> index 30bab8463c96..9221933ab64e 100644
> >> --- a/drivers/usb/dwc2/core.h
> >> +++ b/drivers/usb/dwc2/core.h
> >> @@ -373,6 +373,8 @@ enum dwc2_ep0_state {
> >> * case.
> >> * 0 - No (default)
> >> * 1 - Yes
> >> + * @power_saving: Specifies if power saving is enabled or not. If it is
> >> + * enabled power_down functionality will be enabled.
> >> * @power_down: Specifies whether the controller support power_down.
> >> * If power_down is enabled, the controller will enter
> >> * power_down in both peripheral and host mode when
> >
> > Why are you adding a new parameter? power_saving should be exactly
> > the same as "power_down != DWC2_POWER_DOWN_PARAM_NONE". Just use that
> > anywhere you need it.
> Customers should have a parameter using which they will disable entire
> power saving hibernation and Partial Power Down support.
>
> power_down is used to see which power saving mode we got
> (hibernation/partial power down).
>
> >
> > Having two parameters like you're doing is just asking for them to get
> > out of sync. ...and, in fact, I think they will get out of sync. On
> > rk3288, for instance:
> >
> > -> dwc2_set_default_params()
> > ---> power_saving = true
> > ---> dwc2_set_param_power_down()
> > -----> power_down = DWC2_POWER_DOWN_PARAM_PARTIAL
> > -> set_params(), which is actually dwc2_set_rk_params()
> > ---> power_down = 0
> Setting power_down = 0 is a wrong and old option of disabling power
> saving feature because if we set power_down = 0 then it shows that there
> is no support for any power saving mode. That is why this patch is
> introduced to provide an easier way of disabling power saving modes.
If setting "power_down = 0" is wrong then please update your patch to
remove all the mainline code that sets power_down to 0. Presumably
this means you'd want to convert that code over to using "power_saving
= False". Perhaps then I can see your vision of how this works more
clearly.
NOTE: I'm curious how you envision what someone would do if they had a
core that supported hibernation but they only wanted to enable partial
power down. I guess then they'd have to set "power_saving = True" and
then "power_down = DWC2_POWER_DOWN_PARAM_PARTIAL"? I guess your
vision of the world is:
// Example 1: Core supports power savings but we want disabled
// (no code since this is the default)
// Example 2: Pick the best power saving available
params->power_saving = True
// Example 3: Supports hibernation, but we only want partial:
params->power_saving = True
params->power_down = DWC2_POWER_DOWN_PARAM_PARTIAL
My vision of the world is:
// Example 1: Core supports power savings but we want disabled
params->power_down = DWC2_POWER_DOWN_PARAM_NONE
// Example 2: Pick the best power saving available
// (no code since this is the default)
// Example 3: Supports hibernation, but we only want partial:
params->power_down = DWC2_POWER_DOWN_PARAM_PARTIAL
I like that in my vision of the world "pick the best" is the default
(though I suppose we need to fix the driver so it actually works) and
that there's only one variable so you don't have extra confusion.
> > ...so at the end of dwc2_init_params() you will have power_saving =
> > true but power_down set to DWC2_POWER_DOWN_PARAM_NONE. That seems
> > bad. ...and, in fact:
> >
> > # grep '^power' /sys/kernel/debug/*.usb/params
> > /sys/kernel/debug/ff540000.usb/params:power_saving : 1
> > /sys/kernel/debug/ff540000.usb/params:power_down : 0
> > /sys/kernel/debug/ff580000.usb/params:power_saving : 1
> > /sys/kernel/debug/ff580000.usb/params:power_down : 0
> >
> >
> > ...so while you could fix all of the various set_params() functions,
> > it seems better to just drop this patch since I don't think it buys
> > anything.
> I don't think we should drop this patch. Because, it is introducing the
> correct way of disabling power saving (hibernation/partial power down
> modes). Explanation is listed above.
I personally see no benefit still. It's just as clear to me for
someone to set "power_down = DWC2_POWER_DOWN_PARAM_NONE" as it is to
set "power_savings = False".
-Doug
