Message-ID: <b76f2d92-a470-3e7a-54cd-c0d099cd4d5c@quicinc.com>
Date:   Tue, 14 Feb 2023 18:08:30 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH v4 2/5] usb: dwc3: Add remote wakeup handling
Content-Language: en-US
To:     Thinh Nguyen <Thinh.Nguyen@synopsys.com>
CC:     "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "balbi@kernel.org" <balbi@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "quic_wcheng@quicinc.com" <quic_wcheng@quicinc.com>,
        "quic_jackp@quicinc.com" <quic_jackp@quicinc.com>
References: <1676316676-28377-1-git-send-email-quic_eserrao@quicinc.com>
 <1676316676-28377-3-git-send-email-quic_eserrao@quicinc.com>
 <20230215010950.ufazmdev7cvtef3n@synopsys.com>
From:   Elson Serrao <quic_eserrao@quicinc.com>
In-Reply-To: <20230215010950.ufazmdev7cvtef3n@synopsys.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2/14/2023 5:09 PM, Thinh Nguyen wrote:
> On Mon, Feb 13, 2023, Elson Roy Serrao wrote:
>> An usb device can initate a remote wakeup and bring the link out of
>> suspend as dictated by the DEVICE_REMOTE_WAKEUP feature selector.
>> Add support to handle this packet and set the remote wakeup capability.
>>
>> Some hosts may take longer time to initiate the resume signaling after
>> device triggers a remote wakeup. So add async support to the wakeup API
>> by enabling link status change events.
>>
>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>> ---
>>   drivers/usb/dwc3/core.h   |  2 ++
>>   drivers/usb/dwc3/ep0.c    |  4 +++
>>   drivers/usb/dwc3/gadget.c | 74 +++++++++++++++++++++++++++++++++++++++++++----
>>   3 files changed, 74 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 582ebd9..cc78236 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -1110,6 +1110,7 @@ struct dwc3_scratchpad_array {
>>    *	3	- Reserved
>>    * @dis_metastability_quirk: set to disable metastability quirk.
>>    * @dis_split_quirk: set to disable split boundary.
>> + * @wakeup_configured: set if the device is configured for remote wakeup.
>>    * @imod_interval: set the interrupt moderation interval in 250ns
>>    *			increments or 0 to disable.
>>    * @max_cfg_eps: current max number of IN eps used across all USB configs.
>> @@ -1327,6 +1328,7 @@ struct dwc3 {
>>   
>>   	unsigned		dis_split_quirk:1;
>>   	unsigned		async_callbacks:1;
>> +	unsigned		wakeup_configured:1;
>>   
>>   	u16			imod_interval;
>>   
>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>> index 61de693..f90fa55 100644
>> --- a/drivers/usb/dwc3/ep0.c
>> +++ b/drivers/usb/dwc3/ep0.c
>> @@ -356,6 +356,9 @@ static int dwc3_ep0_handle_status(struct dwc3 *dwc,
>>   				usb_status |= 1 << USB_DEV_STAT_U1_ENABLED;
>>   			if (reg & DWC3_DCTL_INITU2ENA)
>>   				usb_status |= 1 << USB_DEV_STAT_U2_ENABLED;
>> +		} else {
>> +			usb_status |= dwc->gadget->wakeup_armed <<
>> +					USB_DEVICE_REMOTE_WAKEUP;
>>   		}
>>   
>>   		break;
>> @@ -476,6 +479,7 @@ static int dwc3_ep0_handle_device(struct dwc3 *dwc,
>>   
>>   	switch (wValue) {
>>   	case USB_DEVICE_REMOTE_WAKEUP:
>> +		dwc->gadget->wakeup_armed = set;
>>   		break;
>>   	/*
>>   	 * 9.4.1 says only for SS, in AddressState only for
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 3c63fa9..b7fef5d 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -258,7 +258,7 @@ int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned int cmd,
>>   	return ret;
>>   }
>>   
>> -static int __dwc3_gadget_wakeup(struct dwc3 *dwc);
>> +static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async);
>>   
>>   /**
>>    * dwc3_send_gadget_ep_cmd - issue an endpoint command
>> @@ -325,7 +325,7 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned int cmd,
>>   
>>   			fallthrough;
>>   		case DWC3_LINK_STATE_U3:
>> -			ret = __dwc3_gadget_wakeup(dwc);
>> +			ret = __dwc3_gadget_wakeup(dwc, false);
>>   			dev_WARN_ONCE(dwc->dev, ret, "wakeup failed --> %d\n",
>>   					ret);
>>   			break;
>> @@ -2269,6 +2269,22 @@ static const struct usb_ep_ops dwc3_gadget_ep_ops = {
>>   
>>   /* -------------------------------------------------------------------------- */
>>   
>> +static void dwc3_gadget_enable_linksts_evts(struct dwc3 *dwc, bool set)
>> +{
>> +	u32 reg;
>> +
>> +	if (DWC3_VER_IS_PRIOR(DWC3, 250A))
>> +		return;
>> +
>> +	reg = dwc3_readl(dwc->regs, DWC3_DEVTEN);
>> +	if (set)
>> +		reg |= DWC3_DEVTEN_ULSTCNGEN;
>> +	else
>> +		reg &= ~DWC3_DEVTEN_ULSTCNGEN;
>> +
>> +	dwc3_writel(dwc->regs, DWC3_DEVTEN, reg);
>> +}
>> +
>>   static int dwc3_gadget_get_frame(struct usb_gadget *g)
>>   {
>>   	struct dwc3		*dwc = gadget_to_dwc(g);
>> @@ -2276,7 +2292,7 @@ static int dwc3_gadget_get_frame(struct usb_gadget *g)
>>   	return __dwc3_gadget_get_frame(dwc);
>>   }
>>   
>> -static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
>> +static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
>>   {
>>   	int			retries;
>>   
>> @@ -2307,9 +2323,13 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
>>   		return -EINVAL;
>>   	}
>>   
>> +	if (async)
>> +		dwc3_gadget_enable_linksts_evts(dwc, true);
>> +
>>   	ret = dwc3_gadget_set_link_state(dwc, DWC3_LINK_STATE_RECOV);
>>   	if (ret < 0) {
>>   		dev_err(dwc->dev, "failed to put link in Recovery\n");
>> +		dwc3_gadget_enable_linksts_evts(dwc, false);
>>   		return ret;
>>   	}
>>   
>> @@ -2321,6 +2341,13 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
>>   		dwc3_writel(dwc->regs, DWC3_DCTL, reg);
>>   	}
>>   
>> +	/*
>> +	 * Since link status change events are enabled we will receive
>> +	 * an U0 event when wakeup is successful. So bail out.
>> +	 */
>> +	if (async)
>> +		return 0;
>> +
>>   	/* poll until Link State changes to ON */
>>   	retries = 20000;
>>   
>> @@ -2346,13 +2373,36 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
>>   	unsigned long		flags;
>>   	int			ret;
>>   
>> +	if (!dwc->wakeup_configured) {
>> +		dev_err(dwc->dev, "remote wakeup not configured\n");
>> +		return -EINVAL;
>> +	}
>> +
>>   	spin_lock_irqsave(&dwc->lock, flags);
>> -	ret = __dwc3_gadget_wakeup(dwc);
>> +	if (!dwc->gadget->wakeup_armed) {
>> +		dev_err(dwc->dev, "not armed for remote wakeup\n");
>> +		spin_unlock_irqrestore(&dwc->lock, flags);
>> +		return -EINVAL;
>> +	}
>> +	ret = __dwc3_gadget_wakeup(dwc, true);
>> +
>>   	spin_unlock_irqrestore(&dwc->lock, flags);
>>   
>>   	return ret;
>>   }
>>   
>> +static int dwc3_gadget_set_remote_wakeup(struct usb_gadget *g, int set)
>> +{
>> +	struct dwc3		*dwc = gadget_to_dwc(g);
>> +	unsigned long		flags;
>> +
>> +	spin_lock_irqsave(&dwc->lock, flags);
>> +	dwc->wakeup_configured = !!set;
>> +	spin_unlock_irqrestore(&dwc->lock, flags);
>> +
>> +	return 0;
>> +}
>> +
>>   static int dwc3_gadget_set_selfpowered(struct usb_gadget *g,
>>   		int is_selfpowered)
>>   {
>> @@ -2978,6 +3028,7 @@ static void dwc3_gadget_async_callbacks(struct usb_gadget *g, bool enable)
>>   static const struct usb_gadget_ops dwc3_gadget_ops = {
>>   	.get_frame		= dwc3_gadget_get_frame,
>>   	.wakeup			= dwc3_gadget_wakeup,
>> +	.set_remote_wakeup	= dwc3_gadget_set_remote_wakeup,
>>   	.set_selfpowered	= dwc3_gadget_set_selfpowered,
>>   	.pullup			= dwc3_gadget_pullup,
>>   	.udc_start		= dwc3_gadget_start,
>> @@ -3819,6 +3870,8 @@ static void dwc3_gadget_disconnect_interrupt(struct dwc3 *dwc)
>>   
>>   	dwc->gadget->speed = USB_SPEED_UNKNOWN;
>>   	dwc->setup_packet_pending = false;
>> +	dwc->gadget->wakeup_armed = false;
>> +	dwc3_gadget_enable_linksts_evts(dwc, false);
>>   	usb_gadget_set_state(dwc->gadget, USB_STATE_NOTATTACHED);
>>   
>>   	if (dwc->ep0state != EP0_SETUP_PHASE) {
>> @@ -3912,6 +3965,8 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
>>   	reg &= ~DWC3_DCTL_TSTCTRL_MASK;
>>   	dwc3_gadget_dctl_write_safe(dwc, reg);
>>   	dwc->test_mode = false;
>> +	dwc->gadget->wakeup_armed = false;
>> +	dwc3_gadget_enable_linksts_evts(dwc, false);
>>   	dwc3_clear_stall_all_ep(dwc);
>>   
>>   	/* Reset device address to zero */
>> @@ -4064,7 +4119,7 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc)
>>   	 */
>>   }
>>   
>> -static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc)
>> +static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc, unsigned int evtinfo)
>>   {
>>   	/*
>>   	 * TODO take core out of low power mode when that's
>> @@ -4076,6 +4131,8 @@ static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc)
>>   		dwc->gadget_driver->resume(dwc->gadget);
>>   		spin_lock(&dwc->lock);
>>   	}
>> +
>> +	dwc->link_state = evtinfo & DWC3_LINK_STATE_MASK;
>>   }
>>   
>>   static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
>> @@ -4157,6 +4214,10 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
>>   	}
>>   
>>   	switch (next) {
>> +	case DWC3_LINK_STATE_U0:
>> +		dwc3_gadget_enable_linksts_evts(dwc, false);
>> +		dwc3_resume_gadget(dwc);
> 
> I don't think this should be a problem for dwc_usb3 2.50a and prior. But
> for a device that enables link state change event all the time, it would
> call this every time link transitions to U0. I'm not sure what the side
> effect can be. Can we add a wakeup_armed check here to prevent possible
> regression?
> 
> Thanks,
> Thinh
> 
Sure. I will upload v5 with this modification.

Thanks
Elson
>> +		break;
>>   	case DWC3_LINK_STATE_U1:
>>   		if (dwc->speed == USB_SPEED_SUPER)
>>   			dwc3_suspend_gadget(dwc);
>> @@ -4225,7 +4286,7 @@ static void dwc3_gadget_interrupt(struct dwc3 *dwc,
>>   		dwc3_gadget_conndone_interrupt(dwc);
>>   		break;
>>   	case DWC3_DEVICE_EVENT_WAKEUP:
>> -		dwc3_gadget_wakeup_interrupt(dwc);
>> +		dwc3_gadget_wakeup_interrupt(dwc, event->event_info);
>>   		break;
>>   	case DWC3_DEVICE_EVENT_HIBER_REQ:
>>   		if (dev_WARN_ONCE(dwc->dev, !dwc->has_hibernation,
>> @@ -4485,6 +4546,7 @@ int dwc3_gadget_init(struct dwc3 *dwc)
>>   	dwc->gadget->sg_supported	= true;
>>   	dwc->gadget->name		= "dwc3-gadget";
>>   	dwc->gadget->lpm_capable	= !dwc->usb2_gadget_lpm_disable;
>> +	dwc->gadget->wakeup_capable	= true;
>>   
>>   	/*
>>   	 * FIXME We might be setting max_speed to <SUPER, however versions
>> -- 
>> 2.7.4