| Message ID | 6145714.tWcMvDakYx@diego (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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. > > 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
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);