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

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

[Heiko Stuebner]

Hi John,

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-eS4NqCHxEME@public.gmane.org>
> Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> ---

[...]

> @@ -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

And it still requires the 50ms default sleep to work properly. But it seems I 
was able to find some interesting things when testing the individual parts of 
your patch. The port that is affected is a host-only port, so I can also get

[    3.862440] dwc2 101c0000.usb: dwc2_force_dr_mode() to mode 1
{custom debug in dwc2_force_dr_mode}
[    3.868223] dwc2 101c0000.usb: dwc2_force_mode() no OTG controller
{custom debug in dwc2_force_mode at if (!dwc2_hw_is_otg) }

I remember that I also did my previous tests on the host-only ports (since the 
otg ones are often also used as power-supply) but sadly I only have remote 
access to my boards this week, so cannot change the cabling to actually try 
with a real otg dwc2.


Heiko

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

[Stefan Wahren]

Hi Heiko,

> Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> hat am 11. September 2016 um 23:19
> geschrieben:
> 
> 
> Hi John,
> 
> 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-eS4NqCHxEME@public.gmane.org>
> > Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> > ---
> 
> [...]
> 
> > @@ -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?

Regards
Stefan

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

[Heiko Stuebner]

Hi Stefan,

Am Montag, 12. September 2016, 07:20:44 CEST schrieb Stefan Wahren:
> > Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> 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-eS4NqCHxEME@public.gmane.org>
> > > Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> > > ---
> > 
> > [...]
> > 
> > > @@ -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.

Checking my rk3288-veyron-pinky, I see my usb-ethernet coming up correctly and 
if I'm not mistaken (some sort of blindness when reading dmesg), there the 
adapter is connected to the real otg dwc2 on there.

So right now it really seems to be limited to host-only dwc2 controllers, but 
I guess I should double check that once I can switch cables around again.


Heiko

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

[Stefan Wahren]

Hi Heiko,

> Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> 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-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> 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-eS4NqCHxEME@public.gmane.org>
> > > > Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> > > > ---
> > > 
> > > [...]
> > > 
> > > > @@ -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?

Stefan

> 
> Checking my rk3288-veyron-pinky, I see my usb-ethernet coming up correctly and
> 
> if I'm not mistaken (some sort of blindness when reading dmesg), there the 
> adapter is connected to the real otg dwc2 on there.
> 
> So right now it really seems to be limited to host-only dwc2 controllers, but 
> I guess I should double check that once I can switch cables around again.
> 
> 
> Heiko

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

[Heiko Stübner]

Am Dienstag, 13. September 2016, 20:07:25 schrieb Stefan Wahren:
> Hi Heiko,
> 
> > Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> 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-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> 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-eS4NqCHxEME@public.gmane.org>
> > > > > Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> > > > > ---
> > > > 
> > > > [...]
> > > > 
> > > > > @@ -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.


Heiko

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

[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'll also try to get my hands on a rockchip platform. I think that
will help the testing along.

Regards,
John

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

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

[Heiko Stübner]

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-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> 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-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> 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-eS4NqCHxEME@public.gmane.org>
> >>>>>> Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> >>>>>> ---
> >>>>> 
> >>>>> [...]
> >>>>> 
> >>>>>> @@ -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.


Heiko

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

[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.

Regards,
John

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

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

[Stefan Wahren]

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

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

[John Youn]

On 9/15/2016 12:08 AM, Stefan Wahren wrote:
> 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.
> 

Yeah, we'll have to fix in -rc. Either revert the last patch or add
the msleep.

Regards,
John

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

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

[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-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> 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-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> 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-eS4NqCHxEME@public.gmane.org>
>>>>>>>> Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
>>>>>>>> ---
>>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>>> @@ -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.
> 
> 

Hi Heiko,

Let's add the delay just as you have it here.

I'm not sure how the delay comes into play in host-only hardware
though. I will see if the RTL engineers know why that might be needed.

Regards,
John


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html