Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <f541c943-bdf9-f6bd-1826-0aa5a81088ef@quicinc.com>
Date:   Thu, 19 Jan 2023 12:03:19 -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 v2 4/5] usb: dwc3: Add function suspend and function
 wakeup support
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: <1673992507-7823-1-git-send-email-quic_eserrao@quicinc.com>
 <1673992507-7823-5-git-send-email-quic_eserrao@quicinc.com>
 <20230119021838.luuiwe5n6npf3kaj@synopsys.com>
From:   Elson Serrao <quic_eserrao@quicinc.com>
In-Reply-To: <20230119021838.luuiwe5n6npf3kaj@synopsys.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 1/18/2023 6:18 PM, Thinh Nguyen wrote:
> On Tue, Jan 17, 2023, Elson Roy Serrao wrote:
>> USB host sends function suspend and function resume notifications to
>> the interface through SET_FEATURE/CLEAR_FEATURE setup packets.
>> Add support to handle these packets by delegating the requests to
>> composite layer. Also add support to handle function wake notification
>> requests to exit from function suspend state.
>>
>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>> ---
>>   drivers/usb/dwc3/core.h   |  1 +
>>   drivers/usb/dwc3/debug.h  |  2 ++
>>   drivers/usb/dwc3/ep0.c    | 12 ++++--------
>>   drivers/usb/dwc3/gadget.c | 36 +++++++++++++++++++++++++++++++++++-
>>   4 files changed, 42 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 53ded08..5cda645 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -526,6 +526,7 @@
>>   #define DWC3_DGCMD_SET_ENDPOINT_NRDY	0x0c
>>   #define DWC3_DGCMD_SET_ENDPOINT_PRIME	0x0d
>>   #define DWC3_DGCMD_RUN_SOC_BUS_LOOPBACK	0x10
>> +#define DWC3_DGCMD_DEV_NOTIFICATION	0x07
>>   
>>   #define DWC3_DGCMD_STATUS(n)		(((n) >> 12) & 0x0F)
>>   #define DWC3_DGCMD_CMDACT		BIT(10)
>> diff --git a/drivers/usb/dwc3/debug.h b/drivers/usb/dwc3/debug.h
>> index 48b44b8..0897d9d 100644
>> --- a/drivers/usb/dwc3/debug.h
>> +++ b/drivers/usb/dwc3/debug.h
>> @@ -72,6 +72,8 @@ dwc3_gadget_generic_cmd_string(u8 cmd)
>>   		return "Set Endpoint Prime";
>>   	case DWC3_DGCMD_RUN_SOC_BUS_LOOPBACK:
>>   		return "Run SoC Bus Loopback Test";
>> +	case DWC3_DGCMD_DEV_NOTIFICATION:
>> +		return "Device Notification";
>>   	default:
>>   		return "UNKNOWN";
>>   	}
>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>> index 0c1203d..e05c2b9 100644
>> --- a/drivers/usb/dwc3/ep0.c
>> +++ b/drivers/usb/dwc3/ep0.c
>> @@ -30,6 +30,8 @@
>>   static void __dwc3_ep0_do_control_status(struct dwc3 *dwc, struct dwc3_ep *dep);
>>   static void __dwc3_ep0_do_control_data(struct dwc3 *dwc,
>>   		struct dwc3_ep *dep, struct dwc3_request *req);
>> +static int dwc3_ep0_delegate_req(struct dwc3 *dwc,
>> +				 struct usb_ctrlrequest *ctrl);
>>   
>>   static void dwc3_ep0_prepare_one_trb(struct dwc3_ep *dep,
>>   		dma_addr_t buf_dma, u32 len, u32 type, bool chain)
>> @@ -367,7 +369,7 @@ static int dwc3_ep0_handle_status(struct dwc3 *dwc,
>>   		 * Function Remote Wake Capable	D0
>>   		 * Function Remote Wakeup	D1
>>   		 */
>> -		break;
>> +		return dwc3_ep0_delegate_req(dwc, ctrl);
>>   
>>   	case USB_RECIP_ENDPOINT:
>>   		dep = dwc3_wIndex_to_dep(dwc, ctrl->wIndex);
>> @@ -514,13 +516,7 @@ static int dwc3_ep0_handle_intf(struct dwc3 *dwc,
>>   
>>   	switch (wValue) {
>>   	case USB_INTRF_FUNC_SUSPEND:
>> -		/*
>> -		 * REVISIT: Ideally we would enable some low power mode here,
>> -		 * however it's unclear what we should be doing here.
>> -		 *
>> -		 * For now, we're not doing anything, just making sure we return
>> -		 * 0 so USB Command Verifier tests pass without any errors.
>> -		 */
>> +		ret = dwc3_ep0_delegate_req(dwc, ctrl);
>>   		break;
>>   	default:
>>   		ret = -EINVAL;
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index db4b438..1c958c4 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2346,7 +2346,7 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
>>   		return 0;
>>   
>>   	/* poll until Link State changes to ON */
>> -	retries = 20000;
>> +	retries = 30000;
> 
> This seems like a fix that needs to be splitted to a separate change.
> read_poll_timeout_atomic() would be useful here (as a separate/unrelated
> change for status polling).
> 
Done.

>>   
>>   	while (retries--) {
>>   		reg = dwc3_readl(dwc->regs, DWC3_DSTS);
>> @@ -2383,6 +2383,39 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
>>   	return ret;
>>   }
>>   
>> +static void dwc3_resume_gadget(struct dwc3 *dwc);
>> +
>> +static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int interface_id)
>> +{
>> +	struct  dwc3		*dwc = gadget_to_dwc(g);
>> +	unsigned long		flags;
>> +	int			ret;
>> +
>> +	spin_lock_irqsave(&dwc->lock, flags);
>> +	/*
>> +	 * If the link is in LPM, first bring the link to U0
> 
> Reword LMP -> low power.
> 
Sure.

>> +	 * before triggering function remote wakeup.
>> +	 */
>> +	if (dwc->link_state == DWC3_LINK_STATE_U3) {
>> +		ret = __dwc3_gadget_wakeup(dwc, false);
>> +		if (ret) {
>> +			spin_unlock_irqrestore(&dwc->lock, flags);
>> +			return -EINVAL;
>> +		}
>> +		dwc3_resume_gadget(dwc);
>> +		dwc->link_state = DWC3_LINK_STATE_U0;
>> +	}
>> +
>> +	ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
>> +					       0x1 | (interface_id << 4));
> 
> Create a macro for 0x1. Something like DWC3_DGCMDPAR_DN_FUNC_WAKE. Also,
> create an macro for interface selection. Maybe
> DWC3_DGCMDPAR_INTF_SEL(interface_id).
> 
Sure.

>> +	if (ret)
>> +		dev_err(dwc->dev, "function remote wakeup failed, ret:%d\n", ret);
>> +
>> +	spin_unlock_irqrestore(&dwc->lock, flags);
>> +
>> +	return ret;
>> +}
>> +
>>   static int dwc3_gadget_set_selfpowered(struct usb_gadget *g,
>>   		int is_selfpowered)
>>   {
>> @@ -3012,6 +3045,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,
>> +	.func_wakeup		= dwc3_gadget_func_wakeup,
>>   	.set_selfpowered	= dwc3_gadget_set_selfpowered,
>>   	.pullup			= dwc3_gadget_pullup,
>>   	.udc_start		= dwc3_gadget_start,
>> -- 
>> 2.7.4
>>
> 
> Thanks,
> Thinh