Re: [PATCH 3/3] usb: dwc2: drd: restore role and overrides upon resume

[Minas Harutyunyan <Minas.Harutyunyan@synopsys.com>]

On 12/6/2021 7:56 PM, Fabrice Gasnier wrote:
> Override enable bits may not be restored when going to low power mode
> (e.g. when in DWC2_POWER_DOWN_PARAM_NONE).
> These bits are set when probing/initializing drd (role switch). Restore
> them upon resume from low power mode (in case these have been lost).
> 
> To achieve this, the last known role is restored upon resume. And the
> override enable bits are always set when configuring aval, bval and vbval.
> 
> When resuming, forcing the role should be done only once, or this can cause
> port changes in HOST mode for instance.
> So, only restore FORCEDEVMODE/FORCEHOSTMODE when role_sw is unused
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Acked-by: Minas Harutyunyan <Minas.Harutyunyan@synopsys.com>

> ---
>   drivers/usb/dwc2/drd.c      | 38 ++++++++++++++++++++++++++++++++++++--
>   drivers/usb/dwc2/platform.c | 10 ++++++----
>   2 files changed, 42 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/drd.c b/drivers/usb/dwc2/drd.c
> index 4f453ec..1b39c47 100644
> --- a/drivers/usb/dwc2/drd.c
> +++ b/drivers/usb/dwc2/drd.c
> @@ -13,6 +13,10 @@
>   #include <linux/usb/role.h>
>   #include "core.h"
>   
> +#define dwc2_ovr_gotgctl(gotgctl) \
> +	((gotgctl) |= GOTGCTL_BVALOEN | GOTGCTL_AVALOEN | GOTGCTL_VBVALOEN | \
> +	 GOTGCTL_DBNCE_FLTR_BYPASS)
> +
>   static void dwc2_ovr_init(struct dwc2_hsotg *hsotg)
>   {
>   	unsigned long flags;
> @@ -21,8 +25,7 @@ static void dwc2_ovr_init(struct dwc2_hsotg *hsotg)
>   	spin_lock_irqsave(&hsotg->lock, flags);
>   
>   	gotgctl = dwc2_readl(hsotg, GOTGCTL);
> -	gotgctl |= GOTGCTL_BVALOEN | GOTGCTL_AVALOEN | GOTGCTL_VBVALOEN;
> -	gotgctl |= GOTGCTL_DBNCE_FLTR_BYPASS;
> +	dwc2_ovr_gotgctl(gotgctl);
>   	gotgctl &= ~(GOTGCTL_BVALOVAL | GOTGCTL_AVALOVAL | GOTGCTL_VBVALOVAL);
>   	if (hsotg->role_sw_default_mode == USB_DR_MODE_HOST)
>   		gotgctl |= GOTGCTL_AVALOVAL | GOTGCTL_VBVALOVAL;
> @@ -44,6 +47,9 @@ static int dwc2_ovr_avalid(struct dwc2_hsotg *hsotg, bool valid)
>   	    (!valid && !(gotgctl & GOTGCTL_ASESVLD)))
>   		return -EALREADY;
>   
> +	/* Always enable overrides to handle the resume case */
> +	dwc2_ovr_gotgctl(gotgctl);
> +
>   	gotgctl &= ~GOTGCTL_BVALOVAL;
>   	if (valid)
>   		gotgctl |= GOTGCTL_AVALOVAL | GOTGCTL_VBVALOVAL;
> @@ -63,6 +69,9 @@ static int dwc2_ovr_bvalid(struct dwc2_hsotg *hsotg, bool valid)
>   	    (!valid && !(gotgctl & GOTGCTL_BSESVLD)))
>   		return -EALREADY;
>   
> +	/* Always enable overrides to handle the resume case */
> +	dwc2_ovr_gotgctl(gotgctl);
> +
>   	gotgctl &= ~GOTGCTL_AVALOVAL;
>   	if (valid)
>   		gotgctl |= GOTGCTL_BVALOVAL | GOTGCTL_VBVALOVAL;
> @@ -196,6 +205,31 @@ void dwc2_drd_suspend(struct dwc2_hsotg *hsotg)
>   void dwc2_drd_resume(struct dwc2_hsotg *hsotg)
>   {
>   	u32 gintsts, gintmsk;
> +	enum usb_role role;
> +
> +	if (hsotg->role_sw) {
> +		/* get last known role (as the get ops isn't implemented by this driver) */
> +		role = usb_role_switch_get_role(hsotg->role_sw);
> +
> +		if (role == USB_ROLE_NONE) {
> +			if (hsotg->role_sw_default_mode == USB_DR_MODE_HOST)
> +				role = USB_ROLE_HOST;
> +			else if (hsotg->role_sw_default_mode == USB_DR_MODE_PERIPHERAL)
> +				role = USB_ROLE_DEVICE;
> +		}
> +
> +		/* restore last role that may have been lost */
> +		if (role == USB_ROLE_HOST)
> +			dwc2_ovr_avalid(hsotg, true);
> +		else if (role == USB_ROLE_DEVICE)
> +			dwc2_ovr_bvalid(hsotg, true);
> +
> +		dwc2_force_mode(hsotg, role == USB_ROLE_HOST);
> +
> +		dev_dbg(hsotg->dev, "resuming %s-session valid\n",
> +			role == USB_ROLE_NONE ? "No" :
> +			role == USB_ROLE_HOST ? "A" : "B");
> +	}
>   
>   	if (hsotg->role_sw && !hsotg->params.external_id_pin_ctl) {
>   		gintsts = dwc2_readl(hsotg, GINTSTS);
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index c8f18f3..e6a7fc0 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -748,10 +748,12 @@ static int __maybe_unused dwc2_resume(struct device *dev)
>   		spin_unlock_irqrestore(&dwc2->lock, flags);
>   	}
>   
> -	/* Need to restore FORCEDEVMODE/FORCEHOSTMODE */
> -	dwc2_force_dr_mode(dwc2);
> -
> -	dwc2_drd_resume(dwc2);
> +	if (!dwc2->role_sw) {
> +		/* Need to restore FORCEDEVMODE/FORCEHOSTMODE */
> +		dwc2_force_dr_mode(dwc2);
> +	} else {
> +		dwc2_drd_resume(dwc2);
> +	}
>   
>   	if (dwc2_is_device_mode(dwc2))
>   		ret = dwc2_hsotg_resume(dwc2);