Re: [PATCH 2/5] usb: gadget: Add function wakeup support

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

On 8/11/2022, Thinh Nguyen wrote:
> On 8/11/2022, Elson Serrao wrote:
>>
>>
>> On 8/9/2022 6:08 PM, Thinh Nguyen wrote:
> 
> <snip>
> 
> 
>>> To summarize the points:
>>>
>>> 1) The host only arms function remote wakeup if the device is capable of
>>> remote wakeup (check USB_CONFIG_ATT_WAKEUP in bmAttributes and hardware
>>> capability)
>>>
>>> 2) If the device is in suspend, the device can do remote wakeup (through
>>> LFPS handshake) if the function is armed for remote wakeup (through
>>> SET_FEATURE(FUNC_SUSPEND)).
>>>
>>> 3) If the link transitions to U0 after the device triggering a remote
>>> wakeup, the device will also send device notification function wake for
>>> all the interfaces armed with remote wakeup.
>>>
>>> 4) If the device is not in suspend, the device can send device
>>> notification function wake if it's in U0.
>>>
>>>
>>> Now, remote wakeup and function wake device notification are 2 separate
>>> operations. We have the usb_gadget_ops->wakeup() for remote wakeup. I
>>> suggested to maybe add usb_gadget_ops->send_wakeup_notification(gadget,
>>> intf_id) for the device notification. What you did was combining both
>>> operations in usb_gadget_ops->func_wakeup(). That may only work for
>>> point 4) (assuming you fix the U0 check), but not point 3).
>>
>> Thank you for your feedback and summary. I will rename func_wakeup to
>> send_wakeup_notification to better align with the approach. The reason I
>> have combined remote_wakeup and function wake notification in
>> usb_gadget_ops->func_wakeup() is because since the implementation is at
>> function/composite level it has no knowledge on the link state. So I
>> have delegated that task to controller driver to handle the notification
>> accordingly. That is do a LFPS handshake first if the device is
>> suspended and then send notification (explained below). But we can
>> definitely separate this by adding an additional flag in the composite
>> layer to set the link state based on the gadget suspend callback called
>> when U3 SUSPEND interrupt is received. Let me know if you feel
>> separating the two is a better approach.
>>
> 
> The reason I think we need to separate it is because of point 3. As I
> note earlier, the spec states that "If remote wake event occurs in
> multiple functions, each function shall send a Function Wake Notification."
> 
> But if there's no remote wake event, and the host brought the device up
> instead, then the function suspend state is retained.
> 
> If we separate these 2 operations, the caller can check whether the
> operation went through properly. For example, if the wakeup() is
> initiated properly, but the function wake device notification didn't go
> through. We would only need to resend the device notification rather
> than initiate remote wakeup again.

If we don't have to send device notification for other interfaces, we
can combine the operations here as you did.

BR,
Thinh