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.
>
>
> ...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.
>
> -Doug
>
--
Regards,
Artur
