Message-ID: <28322f07-de6b-81e0-38c5-c856d5ce2dce@quicinc.com>
Date:   Thu, 9 Feb 2023 17:36:23 -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 v3 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: <1675710806-9735-1-git-send-email-quic_eserrao@quicinc.com>
 <1675710806-9735-3-git-send-email-quic_eserrao@quicinc.com>
 <20230207004833.tuy52evcgk7sfa4q@synopsys.com>
 <9bcf323a-5660-da47-dd6c-b218f7f26a45@quicinc.com>
 <20230208011017.47o7m3xt63lbih3m@synopsys.com>
 <cd143a99-f781-fd32-cad2-3df8f8c0d617@quicinc.com>
 <20230208021127.syauhdtpbyyncixr@synopsys.com>
From:   Elson Serrao <quic_eserrao@quicinc.com>
In-Reply-To: <20230208021127.syauhdtpbyyncixr@synopsys.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2/7/2023 6:11 PM, Thinh Nguyen wrote:
> On Tue, Feb 07, 2023, Elson Serrao wrote:
>>
>>
>> On 2/7/2023 5:10 PM, Thinh Nguyen wrote:
>>> On Tue, Feb 07, 2023, Elson Serrao wrote:
>>>> On 2/6/2023 4:48 PM, Thinh Nguyen wrote:
>>>>>> +static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
>>>>>>     {
>>>>>>     	int			retries;
>>>>>> @@ -2296,9 +2309,14 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
>>>>>>     	link_state = DWC3_DSTS_USBLNKST(reg);
>>>>>>     	switch (link_state) {
>>>>>> +	case DWC3_LINK_STATE_U3:	/* in HS, means SUSPEND */
>>>>>
>>>>> It's also possible to do remote wakeup in L1 for highspeed.
>>>>>
>>>>
>>>> The rw_configured flag here is in context of triggering remote wakeup from
>>>> bus suspend only.
>>>>
>>>> The remote wakeup setting for l1 in HighSpeed is controlled through LPM
>>>> token and overrides/ignores the config desc bmAttributes wakeup bit.
>>>>
>>>> Section 4.1 of USB2_LinkPowerMangement_ECN[final] spec "The host system sets the Remote Wake Flag parameter in this request to
>>>> enable or disable the addressed device
>>>> for remote wake from L1. The value of this flag will temporarily (while in
>>>> L1) override the current setting of the
>>>> Remote Wake feature settable by the standard Set/ClearFeature() commands
>>>> defined in Universal Serial Bus Specification, revision 2.0, Chapter 9."
>>>>
>>>> Please let me know if I am missing something.
>>>>
>>>
>>> It overrides the setting of the SetFeature request, not the device
>>> configuration.
>>>
>>> The rw_configured reflects the user configuration. Whether the host
>>> tries to enable the remote wakeup through SetFeature request or LPM
>>> token, the device should operate within the user configuration
>>> limitation.
>>>
>>> If the configuration indicates that it doesn't support remote wakeup, we
>>> should prevent unexpected behavior from the device. For simplicity, we
>>> can just return failure to wakeup for all states.
>>>
>>> Thanks,
>>> Thinh
>>
>> L1 entry/exit is HW controlled and the remote wakeup is conditional.(Section
>> 7.1/Table7.2 of dwc3 data book). Even though we block it from
>> SW the l1 exit will still happen from HW point of view.
>>
>> To correlate the user configuration with LPM token, I experimented by
>> disabling the wakeup bit in the bmAtrributes, but I still see remote wakeup
>> bit being set in the LPM token. From the observation it seems like there is
> 
> That's because the linux xhci driver enables remote wakeup bit in its
> port without regard for the device configuration.
> 
>> no correlation between the wakeup bit in the bmAtrributes and the wakeup bit
>> in the LPM token.
>>
> 
> The host can bring the device out of L1, that's probably what you saw.
> The controller doesn't initiate remote wakeup by itself.
> 
> Thanks,
> Thinh

Actually it seems the controller is initiating a remote wakeup by itself 
to exit from l1 when we send a STARTTRANSFER command. I did below 
experiment when the device was in HighSpeed

1.) Enabled l1.
2.) Disabled the remote wakeup software path (i.e avoid calling 
__gadget_wakeup() if link is in l1 in the gadget_ep_cmd() path).
3.) Sent an IN packet when the link was in l1.

 From the lecroy logs it looks like the controller initiated a remote 
wakeup and sent the data.

Below are the events and the corresponding lecroy snippet
1.)Packet(55551) ------------> LPM token from Windows Host PC.

2.) Link in l1 for 2.445 secs

3. ) Send a ping data from device to host

4. )Packet(55554) ----------------> Resume

5.) IN data


Packet#
_______|_______________________________________________________________________
Transaction(26584) H(S) EXT(0x0F) LPM(0xC3) ADDR(11) ENDP(0) BESL(150 us)
_______| Link State(0x1) Rem Wake(0x1) ACK(0x4B) Time Stamp(27 . 204 671 
632)
_______|_______________________________________________________________________Ch0 

Packet(55550) Dir H(S) EXT(0x0F) ADDR(11) ENDP(0) CRC5(0x04) Pkt Len(8)
_______| Duration(133.330 ns) Idle(200.660 ns) Time Stamp(27 . 204 671 632)
_______|_______________________________________________________________________Ch0 

Packet(55551) Dir H(S) LPM(0xC3) BESL(150 us) Link State(0x1)
_______| Rem Wake(0x1) Rsvd(0x0) CRC5(0x04) Pkt Len(8) Duration(133.330 ns)
_______| Idle(182.660 ns) Time Stamp(27 . 204 671 966)
_______|_______________________________________________________________________Ch0 

Packet(55552) Dir H(S) ACK(0x4B) Pkt Len(6) Duration(100.000 ns)
_______| Idle( 11.450 us) Time Stamp(27 . 204 672 282)
_______|_______________________________________________________________________Ch0 

Packet(55553) Dir(?) Full Speed J (Suspend)( 2.445 sec)
_______| Time Stamp(27 . 204 683 832)
_______|_______________________________________________________________________Ch0 

Packet(55554) Dir(?) Full Speed K (Resume?)( 95.168 us) Time(165.134 us)
_______| Time Stamp(29 . 649 644 482)
_______|_______________________________________________________________________
Transfer(67) H(S) Bulk(IN) ADDR(11) ENDP(1) Bytes Transferred(142)
_______| Time(309.366 us) Time Stamp(29 . 649 809 616)
_______|_______________________________________________________________________
Transfer(68) H(S) Bulk(OUT) ADDR(11) ENDP(1) Bytes Transferred(142)
_______| Time(520.050 us) Time Stamp(29 . 650 118 982)
_______|_______________________________________________________________________
Transaction(26655) H(S) EXT(0x0F) LPM(0xC3) ADDR(11) ENDP(0) BESL(150 us)
_______| Link State(0x1) Rem Wake(0x1) ACK(0x4B) Time( 12.168 us)
_______| Time Stamp(29 . 650 639 032)
_______|_______________________________________________________________________

If software was solely responsible for waking up from l1, then there 
should be no reason why the host would exit l1 in this scenario.
I tried with different ping intervals and saw that the duration for 
which the link was in l1 correlates with the ping interval.

Thanks
Elson