Hi,
On 4/29/2019 21:41, Doug Anderson wrote:
> 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.
Yes this is a good idea.
>
> 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:
I have implemented everything based on programming guide and data book.
Core can only support hibernation or partial power down based on the
configuration parameters. There cannot be two modes simultaneously of
power saving only one of them.
The power_down flag is set to DWC2_POWER_DOWN_PARAM_PARTIAL ,
DWC2_POWER_DOWN_PARAM_HIBERNATION or DWC2_POWER_DOWN_PARAM_NONE while
checking the hw parameters. So it just indicates which power down mode
is supporting the core.
>
>
> // 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
If we do so it will show that core doesn't support both hibernation and
partial power down but the core actually supports one of them.
>
> // 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
There is only one mode available at one core hibernation or partial
power down.
>
>
> 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.
Confusion will be when let's say core supports hibernation but you set
power_down to DWC2_POWER_DOWN_PARAM_NONE. This will mean that core
doesn't support no power saving option. But in reality it does and it is
hibernation for instance.
>
>
>>> ...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".
Your vision of the world to set "power_down =
DWC2_POWER_DOWN_PARAM_NONE" will indicate that there is no power saving
mode available on the core.
>
> -Doug
>
--
Regards,
Artur
