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.
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
...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.
-Doug
