Hi,
On Fri, Apr 19, 2019 at 1:05 PM Artur Petrosyan
<Arthur.Petrosyan@xxxxxxxxxxxx> wrote:
>
> - In dwc2_port_suspend() function added waiting for the
> HPRT0.PrtSusp register field to be set.
>
> - In _dwc2_hcd_suspend() function added checking of
> "hsotg->flags.b.port_connect_status" port connection
> status if port connection status is 0 then skipping
> power saving (entering partial power down mode).
> Because if there is no device connected there would
> be no need to enter partial power down mode.
>
> - Added "hsotg->bus_suspended = true" beceuse after
s/beceuse/because
> entering partial power down in host mode the
> bus_suspended flag must be set.
The above statement sounds to me like trying to win an argument by
saying "I'm right because I'm right." Can you give more details about
why "bus_suspended" must be set? See below also.
> Signed-off-by: Artur Petrosyan <arturp@xxxxxxxxxxxx>
> ---
> drivers/usb/dwc2/hcd.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index dd82fa516f3f..1d18258b5ff8 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -3479,6 +3479,10 @@ static void dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex)
> hprt0 |= HPRT0_SUSP;
> dwc2_writel(hsotg, hprt0, HPRT0);
>
> + /* Wait for the HPRT0.PrtSusp register field to be set */
> + if (dwc2_hsotg_wait_bit_set(hsotg, HPRT0, HPRT0_SUSP, 3000))
> + dev_warn(hsotg->dev, "Suspend wasn't generated\n");
> +
> hsotg->bus_suspended = true;
>
> /*
> @@ -4488,7 +4492,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
> if (hsotg->op_state == OTG_STATE_B_PERIPHERAL)
> goto unlock;
>
> - if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL)
> + if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL ||
> + hsotg->flags.b.port_connect_status == 0)
> goto skip_power_saving;
>
> /*
> @@ -4514,6 +4519,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
> goto skip_power_saving;
> }
>
> + hsotg->bus_suspended = true;
> +
I'm a bit unsure about this. Specifically I note that
_dwc2_hcd_resume() has a special case "if (hsotg->bus_suspended)".
Does that now become dead code?
...I talk about this a bit more in my review of ("usb: dwc2: Add
enter/exit hibernation from system issued suspend/resume")
-Doug
