Re: [PATCH v5 3/3] usb: dwc2: Properly account for the force mode delays

[Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>]

Hi John,

[add Felipe and Greg]

Am 14.09.2016 um 23:10 schrieb John Youn:
> On 9/14/2016 7:05 AM, Heiko Stübner wrote:
>> Am Dienstag, 13. September 2016, 12:04:12 schrieb John Youn:
>>> On 9/13/2016 11:39 AM, Heiko Stübner wrote:
>>>> Am Dienstag, 13. September 2016, 20:07:25 schrieb Stefan Wahren:
>>>>> Hi Heiko,
>>>>>
>>>>>> Heiko Stuebner <heiko@sntech.de> hat am 12. September 2016 um 13:05
>>>>>> geschrieben:
>>>>>>
>>>>>>
>>>>>> Hi Stefan,
>>>>>>
>>>>>> Am Montag, 12. September 2016, 07:20:44 CEST schrieb Stefan Wahren:
>>>>>>>> Heiko Stuebner <heiko@sntech.de> hat am 11. September 2016 um 23:19
>>>>>>>> geschrieben:
>>>>>>>>
>>>>>>>> Am Mittwoch, 7. September 2016, 19:39:43 CEST schrieb John Youn:
>>>>>>>>> When a force mode bit is set and the IDDIG debounce filter is
>>>>>>>>> enabled,
>>>>>>>>> there is a delay for the forced mode to take effect. This delay is
>>>>>>>>> due
>>>>>>>>> to the IDDIG debounce filter and is variable depending on the
>>>>>>>>> platform's
>>>>>>>>> PHY clock speed. To account for this delay we can poll for the
>>>>>>>>> expected
>>>>>>>>> mode.
>>>>>>>>>
>>>>>>>>> On a clear force mode, since we don't know what mode to poll for,
>>>>>>>>> delay
>>>>>>>>> for a fixed 100 ms. This is the maximum delay based on the slowest
>>>>>>>>> PHY
>>>>>>>>> clock speed.
>>>>>>>>>
>>>>>>>>> Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
>>>>>>>>> Signed-off-by: John Youn <johnyoun@synopsys.com>
>>>>>>>>> ---
>>>>>>>> [...]
>>>>>>>>
>>>>>>>>> @@ -475,12 +478,6 @@ void dwc2_force_dr_mode(struct dwc2_hsotg
>>>>>>>>> *hsotg)
>>>>>>>>>
>>>>>>>>>   			 __func__, hsotg->dr_mode);
>>>>>>>>>   		
>>>>>>>>>   		break;
>>>>>>>>>   	
>>>>>>>>>   	}
>>>>>>>>>
>>>>>>>>> -
>>>>>>>>> -	/*
>>>>>>>>> -	 * NOTE: This is required for some rockchip soc based
>>>>>>>>> -	 * platforms.
>>>>>>>>> -	 */
>>>>>>>>> -	msleep(50);
>>>>>>>>>
>>>>>>>>>   }
>>>>>>>> sorry for not finding the time to test your subsequent versions, but
>>>>>>>> this
>>>>>>>> still
>>>>>>>> acts up on my Rockchip boards, as I'm still running into errors like
>>>>>>>>
>>>>>>>> 	[    4.875570] usb usb2-port1: connect-debounce failed
>>>>>>> could you please name the relevant DTS file of the affected boards?
>>>>>> So far I've been able to see that on
>>>>>>
>>>>>> rk3188-radxarock
>>>>>> rk3036-kylin
>>>>>>
>>>>>> both on host-only dwc2 controllers.
>>>>> thanks, does patch 1 & 2 already have a negative effect on these
>>>>> controllers?
>>>> nope, patches 1+2 alone are ok and only the msleep(50) going away causes
>>>> problems. I'm back home now, so I'll hopefully have time to check
>>>> host-only
>>>> vs. otg dwc2 controllers tomorrow as well.
>>> Ok, thanks for testing Heiko.
>>>
>>> Given that, let's try to fix this in the next -rc (or before) because
>>> I think the Raspberry Pi needs the other parts of that last patch. We
>>> can just revert the msleep(50) if needed.
>> I could nicely confirm, that only the host-only controller seems affected by
>> this. Same kernel image that fails on the host-only one with the "connect-
>> debounce failed" message seems to work once I switch that over to the  otg
>> controller and the system comes up nicely again.
>>
>> So if we don't find the root cause in the short term, maybe we could at least
>> limit the delay for host-only variants, like
>>
>> ---------------------- 8< -------------------
>> diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
>> index fa9b26b..4c0fa0b 100644
>> --- a/drivers/usb/dwc2/core.c
>> +++ b/drivers/usb/dwc2/core.c
>> @@ -463,9 +463,18 @@ static void dwc2_clear_force_mode(struct dwc2_hsotg *hsotg)
>>    */
>>   void dwc2_force_dr_mode(struct dwc2_hsotg *hsotg)
>>   {
>> +	bool ret;
>> +
>>   	switch (hsotg->dr_mode) {
>>   	case USB_DR_MODE_HOST:
>> -		dwc2_force_mode(hsotg, true);
>> +		ret = dwc2_force_mode(hsotg, true);
>> +		/*
>> +		 * NOTE: This is required for some rockchip soc based
>> +		 * platforms on their host-only dwc2.
>> +		 */
>> +		if (!ret)
>> +			msleep(50);
>> +
>>   		break;
>>   	case USB_DR_MODE_PERIPHERAL:
>>   		dwc2_force_mode(hsotg, false);
>> ---------------------- 8< -------------------
>>
>> I've been running with that change for some time now on both the
>> rk3036-kylin and rk3188-radxarock. Plugged and unplugged different
>> combinations of usb-devices on the usb otg and host ports, so far
>> seems to work.
>>
> Ok great. We'll use that for the -rc if we can't find the root cause.
>
> Then hopefully we can apply the final dropped patch for 4.10.

unfortunately i noticed too late that Felipe and Greg didn't get the 
whole conversation. The series v5 has been merged in Greg's USB tree.

Stefan

>
> Regards,
> John


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip