| Message ID | 1505937448-13475-1-git-send-email-john.stultz@linaro.org |
|---|---|
| Headers | show |
| Series | dwc2 fixes for edge cases on hikey | expand |
On 09/20/2017 12:57 PM, John Stultz wrote: > So here are a few dwc2 fixes that I've been using with HiKey. > I'm not totally sure these are all ideal, but they avoid edge case > issues that we have been running into with switching between > gadget mode and host mode. > > I'd guess the first two are potentially -stable material, and > the last might be worth sending to -stable too, as its a relatively > simple fix, but to my understanding the UDC state tracking has > always been broken so its not really a regression. But still. > > I'd love to get some feedback on the patches and consideration > to be merged upstream. > > thanks > -john > > Cc: Wei Xu <xuwei5@hisilicon.com> > Cc: Guodong Xu <guodong.xu@linaro.org> > Cc: Amit Pundir <amit.pundir@linaro.org> > Cc: YongQin Liu <yongqin.liu@linaro.org> > Cc: John Youn <johnyoun@synopsys.com> > Cc: Minas Harutyunyan <Minas.Harutyunyan@synopsys.com> > Cc: Douglas Anderson <dianders@chromium.org> > Cc: Chen Yu <chenyu56@huawei.com> > Cc: Felipe Balbi <felipe.balbi@linux.intel.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: linux-usb@vger.kernel.org > > John Stultz (3): > usb: dwc2: Improve gadget state disconnection handling > usb: dwc2: Error out of dwc2_hsotg_ep_disable() if we're in host mode > usb: dwc2: Fix UDC state tracking > > drivers/usb/dwc2/gadget.c | 7 +++++++ > drivers/usb/dwc2/hcd.c | 8 ++++++-- > 2 files changed, 13 insertions(+), 2 deletions(-) > Hi John, I think we have something that fixes these issues. Minas, Could you take a look at this? I was not able to find the patches we talked about. If possible, please post them so that John can try them out. Thanks, John
On 9/30/2017 9:13 PM, John Youn wrote: > On 09/20/2017 12:57 PM, John Stultz wrote: >> So here are a few dwc2 fixes that I've been using with HiKey. >> I'm not totally sure these are all ideal, but they avoid edge case >> issues that we have been running into with switching between >> gadget mode and host mode. >> >> I'd guess the first two are potentially -stable material, and >> the last might be worth sending to -stable too, as its a relatively >> simple fix, but to my understanding the UDC state tracking has >> always been broken so its not really a regression. But still. >> >> I'd love to get some feedback on the patches and consideration >> to be merged upstream. >> >> thanks >> -john >> >> Cc: Wei Xu <xuwei5@hisilicon.com> >> Cc: Guodong Xu <guodong.xu@linaro.org> >> Cc: Amit Pundir <amit.pundir@linaro.org> >> Cc: YongQin Liu <yongqin.liu@linaro.org> >> Cc: John Youn <johnyoun@synopsys.com> >> Cc: Minas Harutyunyan <Minas.Harutyunyan@synopsys.com> >> Cc: Douglas Anderson <dianders@chromium.org> >> Cc: Chen Yu <chenyu56@huawei.com> >> Cc: Felipe Balbi <felipe.balbi@linux.intel.com> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Cc: linux-usb@vger.kernel.org >> >> John Stultz (3): >> usb: dwc2: Improve gadget state disconnection handling >> usb: dwc2: Error out of dwc2_hsotg_ep_disable() if we're in host mode >> usb: dwc2: Fix UDC state tracking >> >> drivers/usb/dwc2/gadget.c | 7 +++++++ >> drivers/usb/dwc2/hcd.c | 8 ++++++-- >> 2 files changed, 13 insertions(+), 2 deletions(-) >> > > Hi John, > > I think we have something that fixes these issues. > > Minas, > > Could you take a look at this? I was not able to find the patches we > talked about. If possible, please post them so that John can try them > out. > > Thanks, > John > Hi John Stultz, Could you please apply patch from Vardan Mikayelyan "usb: dwc2: Fix dwc2_hsotg_core_init_disconnected()" submitted at 02/25/2017 (https://marc.info/?l=linux-usb&m=148801589931039&w=2) instead of your "usb: dwc2: Improve gadget state disconnection handling" and test again failing scenario. Other 2 patches from series "[PATCH 0/3] dwc2 fixes for edge cases on hikey" are Ok. Thanks, Minas
On Tue, Oct 3, 2017 at 2:58 AM, Minas Harutyunyan <Minas.Harutyunyan@synopsys.com> wrote: > > Could you please apply patch from Vardan Mikayelyan "usb: dwc2: Fix > dwc2_hsotg_core_init_disconnected()" submitted at 02/25/2017 > (https://marc.info/?l=linux-usb&m=148801589931039&w=2) instead of your > "usb: dwc2: Improve gadget state disconnection handling" and test again > failing scenario. I may be misunderstanding htings, but I don't believe that patch addresses the same issue I'm trying to fix (I've tested with it, and it doesn't cause any trouble for me, but it also doesn't seem to address the corner-cases I'm hitting). My "Improve gadget state disconnection handling" patch handles the fact that when we switch from B/gadget mode to A/Host mode, we don't always go through a gadget disconnect path. So instead of calling the dwc2_hsotg_disconnect() path initially when switching to gadget mode (to avoid the state complaining that we set it up twice), we should instead be calling dwc2_hsotg_disconnect() when we are setting up the A/host mode. So for example, the follow-on fix to the UDC state won't properly work without my "Improve gadget state disconnection handling" patch, and "cat /sys/class/udc/f72c0000.usb/state" will always return configured (assuming gadget mode was used once) regardless of the gadget state, since we don't actually call dwc2_hsotg_disconnect when the otg plug is removed. > Other 2 patches from series "[PATCH 0/3] dwc2 fixes for edge cases on > hikey" are Ok. Thanks for the review/feedback! thanks -john
On 10/10/2017 1:50 AM, John Stultz wrote: > On Tue, Oct 3, 2017 at 2:58 AM, Minas Harutyunyan > <Minas.Harutyunyan@synopsys.com> wrote: >> >> Could you please apply patch from Vardan Mikayelyan "usb: dwc2: Fix >> dwc2_hsotg_core_init_disconnected()" submitted at 02/25/2017 >> (https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-3Fl-3Dlinux-2Dusb-26m-3D148801589931039-26w-3D2&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=6z9Al9FrHR_ZqbbtSAsD16pvOL2S3XHxQnSzq8kusyI&m=4y4_kSoJQIp-rJkvFNu8yXR67QxLLQrbFkjlyytMUCE&s=3Gmh7tVGk7ncQfBNUjkVdpRa1XX_jf7lWga7kR1O9bQ&e=) instead of your >> "usb: dwc2: Improve gadget state disconnection handling" and test again >> failing scenario. > > I may be misunderstanding htings, but I don't believe that patch > addresses the same issue I'm trying to fix (I've tested with it, and > it doesn't cause any trouble for me, but it also doesn't seem to > address the corner-cases I'm hitting). > > My "Improve gadget state disconnection handling" patch handles the > fact that when we switch from B/gadget mode to A/Host mode, we don't > always go through a gadget disconnect path. > > So instead of calling the dwc2_hsotg_disconnect() path initially when > switching to gadget mode (to avoid the state complaining that we set > it up twice), we should instead be calling dwc2_hsotg_disconnect() > when we are setting up the A/host mode. > > So for example, the follow-on fix to the UDC state won't properly work > without my "Improve gadget state disconnection handling" patch, and > "cat /sys/class/udc/f72c0000.usb/state" will always return configured > (assuming gadget mode was used once) regardless of the gadget state, > since we don't actually call dwc2_hsotg_disconnect when the otg plug > is removed. > >> Other 2 patches from series "[PATCH 0/3] dwc2 fixes for edge cases on >> hikey" are Ok. > > Thanks for the review/feedback! > > thanks > -john > Hi John Stultz, 1. Vardan's patch fixing issue when dwc2 switched from host to device mode. It's allow to make functional device after reconnecting without tracking UDC state. 2. I suppose that your patch "[RESEND x2][PATCH 1/3] usb: dwc2: Improve gadget state disconnection handling" not a good way to set correct UDC state. You added calling device mode functions dwc2_hsotg_disconnect() and dwc2_hsotg_core_init_disconnected() while core in Host mode and as result additional unwanted "mode mismatch" interrupts will be asserted. 3. Function dwc2_conn_id_status_change() called when connector ID status changed. This interrupt asserted only when A-plug connected or disconnected. Connecting/disconnecting B-plug doesn't assert this interrupt. Thanks, Minas
On Thu, Oct 12, 2017 at 12:59 AM, Minas Harutyunyan <Minas.Harutyunyan@synopsys.com> wrote: > > 1. Vardan's patch fixing issue when dwc2 switched from host to device > mode. It's allow to make functional device after reconnecting without > tracking UDC state. While I'm sure Vardan's patch is useful, I worry that its resolving a different issue then what I'm trying to address, as it doesn't seem to help the problems I'm seeing. > 2. I suppose that your patch "[RESEND x2][PATCH 1/3] usb: dwc2: Improve > gadget state disconnection handling" not a good way to set correct UDC > state. You added calling device mode functions dwc2_hsotg_disconnect() > and dwc2_hsotg_core_init_disconnected() while core in Host mode and as > result additional unwanted "mode mismatch" interrupts will be asserted. Apologies, I'm not sure I'm understanding you here. Forgive me if I'm misinterpreting your feedback. So, the "usb: dwc2: Improve gadget state disconnection handling" isn't itself doing the UDC state handling. Personally I see it as improving a previously applied fix (dad3f793f20f - usb: dwc2: Make sure we disconnect the gadget state). So instead of calling dwc2_hsotg_disconnect() in dwc2_conn_id_status_change() when transitioning INTO device/B mode, which was added due to earlier problems with state tracking (as when we unplug the gadget cable, nothing else triggers the hsotg_disconnect code), I'm instead suggesting we call dwc2_hsotg_disconnect() when we transition into host/A mode. This only allows us to do proper UDC state handling later, since we properly run the disconnect code for device when we switch into host mode. If I'm understanding you, you seem to be objecting to this, as calling dwc2_hsotg_disconnect() while we are transitioning to host mode can cause "mode mismatch" interrupts. I've not seen this in practice with this patch, but you know the logic better and it could be possible. Now, I'm of course open to other approaches, but it seems that we need *something* to call dwc2_hsotg_disconnect() when the otg cable is removed (which currently just doesn't happen). The earlier patch calling dwc2_hsotg_disconnect() when we are entering device/B mode avoids the state tracking warnings but, doesn't seem correct (nor does it allow for things like proper UDC state handling). > 3. Function dwc2_conn_id_status_change() called when connector ID status > changed. This interrupt asserted only when A-plug connected or > disconnected. Connecting/disconnecting B-plug doesn't assert this interrupt. Ok. What I'm seeing may be somewhat hardware specific then, as on HiKey, we have a switch that enables a on-board USB hub when the OTG plug is removed. This may be the root of the issue, but I guess I'm at a loss for how things should be handled here. When the b-plug is disconnected, we need to do something to signal to the core that we aren't connected, no? And it seems that your point that the conn_id_status_change logic only runs on the A-plug connect/disconnect mirrors the usage for the B plug (ie: if A is disconnected, we enter B mode, thus if A is connected, shouldn't we disable/disconnect B mode?). I suspect there's something more subtle to your statement though, so if you could expand a bit so I could better understand I'd appreciate it. thanks -john
On 10/12/2017 10:06 PM, John Stultz wrote: > On Thu, Oct 12, 2017 at 12:59 AM, Minas Harutyunyan > <Minas.Harutyunyan@synopsys.com> wrote: >> >> 1. Vardan's patch fixing issue when dwc2 switched from host to device >> mode. It's allow to make functional device after reconnecting without >> tracking UDC state. > > While I'm sure Vardan's patch is useful, I worry that its resolving a > different issue then what I'm trying to address, as it doesn't seem to > help the problems I'm seeing. > >> 2. I suppose that your patch "[RESEND x2][PATCH 1/3] usb: dwc2: Improve >> gadget state disconnection handling" not a good way to set correct UDC >> state. You added calling device mode functions dwc2_hsotg_disconnect() >> and dwc2_hsotg_core_init_disconnected() while core in Host mode and as >> result additional unwanted "mode mismatch" interrupts will be asserted. > > Apologies, I'm not sure I'm understanding you here. Forgive me if I'm > misinterpreting your feedback. > > So, the "usb: dwc2: Improve gadget state disconnection handling" isn't > itself doing the UDC state handling. > > Personally I see it as improving a previously applied fix > (dad3f793f20f - usb: dwc2: Make sure we disconnect the gadget state). > So instead of calling dwc2_hsotg_disconnect() in > dwc2_conn_id_status_change() when transitioning INTO device/B mode, > which was added due to earlier problems with state tracking (as when > we unplug the gadget cable, nothing else triggers the hsotg_disconnect > code), I'm instead suggesting we call dwc2_hsotg_disconnect() when we > transition into host/A mode. > > This only allows us to do proper UDC state handling later, since we > properly run the disconnect code for device when we switch into host > mode. > > If I'm understanding you, you seem to be objecting to this, as calling > dwc2_hsotg_disconnect() while we are transitioning to host mode can > cause "mode mismatch" interrupts. I've not seen this in practice with > this patch, but you know the logic better and it could be possible. > > Now, I'm of course open to other approaches, but it seems that we need > *something* to call dwc2_hsotg_disconnect() when the otg cable is > removed (which currently just doesn't happen). The earlier patch > calling dwc2_hsotg_disconnect() when we are entering device/B mode > avoids the state tracking warnings but, doesn't seem correct (nor does > it allow for things like proper UDC state handling). > >> 3. Function dwc2_conn_id_status_change() called when connector ID status >> changed. This interrupt asserted only when A-plug connected or >> disconnected. Connecting/disconnecting B-plug doesn't assert this interrupt. > > Ok. What I'm seeing may be somewhat hardware specific then, as on > HiKey, we have a switch that enables a on-board USB hub when the OTG > plug is removed. This may be the root of the issue, but I guess I'm > at a loss for how things should be handled here. > > When the b-plug is disconnected, we need to do something to signal to > the core that we aren't connected, no? > > And it seems that your point that the conn_id_status_change logic only > runs on the A-plug connect/disconnect mirrors the usage for the B plug > (ie: if A is disconnected, we enter B mode, thus if A is connected, > shouldn't we disable/disconnect B mode?). I suspect there's something > more subtle to your statement though, so if you could expand a bit so > I could better understand I'd appreciate it. > > thanks > -john > Hi John Stultz, On b-plug disconnect should asserted GOTGINT.SesEndDet interrupt. According previously sent by you register dump (GHWCFG2 = 0x23affc70) your core OTG_MODE=0. Bellow fragment from programming guide on Device disconnect: "7.3Device Disconnection The device session ends when the USB cable is disconnected or if the VBUS is switched off by the Host. The device disconnect flow varies depending on the value of the OTG_MODE configuration parameter. When OTG_MODE = 0,1, or 3 When OTG_MODE is set to 0,1, or 3, the device disconnect flow is as follows: 1. When the USB cable is unplugged or when the VBUS is switched off by the Host, the Device core trigger GINTSTS.OTGInt [bit 2] interrupt bit. 2. When the device application detects GINTSTS.OTGInt interrupt, it checks that the GOTGINT.SesEndDet (Session End Detected) bit is set to 1’b1." So, you should receive and handle "Session End Detected". In function dwc2_handle_otg_intr() on this interrupt (in device mode) calling dwc2_hsotg_disconnect() function. By adding your patch "[PATCH 3/3] usb: dwc2: Fix UDC state tracking" state changed to not attached as required. Thanks, Minas
On 10/17/2017 1:34 AM, John Stultz wrote: > On Mon, Oct 16, 2017 at 1:36 AM, Minas Harutyunyan > <Minas.Harutyunyan@synopsys.com> wrote: >> On b-plug disconnect should asserted GOTGINT.SesEndDet interrupt. >> According previously sent by you register dump (GHWCFG2 = 0x23affc70) >> your core OTG_MODE=0. >> Bellow fragment from programming guide on Device disconnect: >> >> "7.3Device Disconnection >> The device session ends when the USB cable is disconnected or if the >> VBUS is switched off by the Host. The >> device disconnect flow varies depending on the value of the OTG_MODE >> configuration parameter. >> >> When OTG_MODE = 0,1, or 3 >> When OTG_MODE is set to 0,1, or 3, the device disconnect flow is as follows: >> 1. When the USB cable is unplugged or when the VBUS is switched off by >> the Host, the Device core >> trigger GINTSTS.OTGInt [bit 2] interrupt bit. >> 2. When the device application detects GINTSTS.OTGInt interrupt, it >> checks that the >> GOTGINT.SesEndDet (Session End Detected) bit is set to 1’b1." >> >> So, you should receive and handle "Session End Detected". In function >> dwc2_handle_otg_intr() on this interrupt (in device mode) calling >> dwc2_hsotg_disconnect() function. By adding your patch "[PATCH 3/3] usb: >> dwc2: Fix UDC state tracking" state changed to not attached as required. > > > So, on the HiKey board (using 4.14-rc5 + Vardan's patch), I'm not > seeing the GOTGINT_SES_END_DET in dwc2_handle_otg_intr() when I remove > the USB OTG cable. > > In fact, I'm not seeing any calls to dwc2_handle_otg_intr()... which > seems... odd maybe? Any clues as to what might be going wrong then? > > thanks > -john > Hi John Stultz, So, on Hikey board on unplug B connector GOTGINT.SesEndDet interrupt not asserted, instead asserted GINTSTS_CONIDSTSCHNG. Please, confirm. In this case without your patch "[PATCH 1/3] usb: dwc2: Improve gadget state disconnection handling" but by applying your patch "[PATCH 3/3] usb: dwc2: Fix UDC state tracking": 1. On B plug connect UDC state will be set to "configured" 2. On B plug disconnect - "not attached". Is it Ok for you? Meantime, I'll check with HW team why GOTGINT.SesEndDet interrupt not asserted on unplug B connector. Thanks, Minas
On 10/17/2017 12:41 PM, Minas Harutyunyan wrote: > On 10/17/2017 1:34 AM, John Stultz wrote: >> On Mon, Oct 16, 2017 at 1:36 AM, Minas Harutyunyan >> <Minas.Harutyunyan@synopsys.com> wrote: >>> On b-plug disconnect should asserted GOTGINT.SesEndDet interrupt. >>> According previously sent by you register dump (GHWCFG2 = 0x23affc70) >>> your core OTG_MODE=0. >>> Bellow fragment from programming guide on Device disconnect: >>> >>> "7.3Device Disconnection >>> The device session ends when the USB cable is disconnected or if the >>> VBUS is switched off by the Host. The >>> device disconnect flow varies depending on the value of the OTG_MODE >>> configuration parameter. >>> >>> When OTG_MODE = 0,1, or 3 >>> When OTG_MODE is set to 0,1, or 3, the device disconnect flow is as follows: >>> 1. When the USB cable is unplugged or when the VBUS is switched off by >>> the Host, the Device core >>> trigger GINTSTS.OTGInt [bit 2] interrupt bit. >>> 2. When the device application detects GINTSTS.OTGInt interrupt, it >>> checks that the >>> GOTGINT.SesEndDet (Session End Detected) bit is set to 1’b1." >>> >>> So, you should receive and handle "Session End Detected". In function >>> dwc2_handle_otg_intr() on this interrupt (in device mode) calling >>> dwc2_hsotg_disconnect() function. By adding your patch "[PATCH 3/3] usb: >>> dwc2: Fix UDC state tracking" state changed to not attached as required. >> >> >> So, on the HiKey board (using 4.14-rc5 + Vardan's patch), I'm not >> seeing the GOTGINT_SES_END_DET in dwc2_handle_otg_intr() when I remove >> the USB OTG cable. >> >> In fact, I'm not seeing any calls to dwc2_handle_otg_intr()... which >> seems... odd maybe? Any clues as to what might be going wrong then? >> >> thanks >> -john >> > Hi John Stultz, > So, on Hikey board on unplug B connector GOTGINT.SesEndDet interrupt not > asserted, instead asserted GINTSTS_CONIDSTSCHNG. Please, confirm. > > In this case without your patch "[PATCH 1/3] usb: dwc2: Improve gadget > state disconnection handling" but by applying your patch "[PATCH 3/3] > usb: dwc2: Fix UDC state tracking": > 1. On B plug connect UDC state will be set to "configured" > 2. On B plug disconnect - "not attached". > Is it Ok for you? > > Meantime, I'll check with HW team why GOTGINT.SesEndDet interrupt not > asserted on unplug B connector. > > Thanks, > Minas > Hi John Stultz, Could you please apply this patch. Please not apply your patch series "[PATCH 0/3] dwc2 fixes for edge cases on hikey" to check only below patch. If you confirm that this patch fix your issue with "Transaction Error" and " ChHltd set, but reason is unknown" I'll submit to LKML as final patch. Then can be applied your patch series, without patch 1/3 (changing in connector id status change) to correctly set UDC states. diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index f4ef159b538e..7da22152df68 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -331,6 +331,9 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg) usbcfg = dwc2_readl(hsotg->regs + GUSBCFG); usbcfg &= ~(GUSBCFG_HNPCAP | GUSBCFG_SRPCAP); + /* Set HS/FS Timeout Calibration */ + usbcfg |= GUSBCFG_TOUTCAL(7); + switch (hsotg->hw_params.op_mode) { case GHWCFG2_OP_MODE_HNP_SRP_CAPABLE: if (hsotg->params.otg_cap == Thanks, Minas
On Tue, Oct 17, 2017 at 1:41 AM, Minas Harutyunyan <Minas.Harutyunyan@synopsys.com> wrote: > On 10/17/2017 1:34 AM, John Stultz wrote: >> On Mon, Oct 16, 2017 at 1:36 AM, Minas Harutyunyan >> <Minas.Harutyunyan@synopsys.com> wrote: >>> On b-plug disconnect should asserted GOTGINT.SesEndDet interrupt. >>> According previously sent by you register dump (GHWCFG2 = 0x23affc70) >>> your core OTG_MODE=0. >>> Bellow fragment from programming guide on Device disconnect: >>> >>> "7.3Device Disconnection >>> The device session ends when the USB cable is disconnected or if the >>> VBUS is switched off by the Host. The >>> device disconnect flow varies depending on the value of the OTG_MODE >>> configuration parameter. >>> >>> When OTG_MODE = 0,1, or 3 >>> When OTG_MODE is set to 0,1, or 3, the device disconnect flow is as follows: >>> 1. When the USB cable is unplugged or when the VBUS is switched off by >>> the Host, the Device core >>> trigger GINTSTS.OTGInt [bit 2] interrupt bit. >>> 2. When the device application detects GINTSTS.OTGInt interrupt, it >>> checks that the >>> GOTGINT.SesEndDet (Session End Detected) bit is set to 1’b1." >>> >>> So, you should receive and handle "Session End Detected". In function >>> dwc2_handle_otg_intr() on this interrupt (in device mode) calling >>> dwc2_hsotg_disconnect() function. By adding your patch "[PATCH 3/3] usb: >>> dwc2: Fix UDC state tracking" state changed to not attached as required. >> >> >> So, on the HiKey board (using 4.14-rc5 + Vardan's patch), I'm not >> seeing the GOTGINT_SES_END_DET in dwc2_handle_otg_intr() when I remove >> the USB OTG cable. >> >> In fact, I'm not seeing any calls to dwc2_handle_otg_intr()... which >> seems... odd maybe? Any clues as to what might be going wrong then? >> >> thanks >> -john >> > Hi John Stultz, > So, on Hikey board on unplug B connector GOTGINT.SesEndDet interrupt not > asserted, instead asserted GINTSTS_CONIDSTSCHNG. Please, confirm. Correct. On B unplug, I see: dwc2_handle_conn_id_status_change_intr: ++Connector ID Status Change Interrupt++ (Host) And I never see any calls to dwc2_handle_otg_intr(). > In this case without your patch "[PATCH 1/3] usb: dwc2: Improve gadget > state disconnection handling" but by applying your patch "[PATCH 3/3] > usb: dwc2: Fix UDC state tracking": > 1. On B plug connect UDC state will be set to "configured" > 2. On B plug disconnect - "not attached". > Is it Ok for you? So this is what I expect, but I don't see it. Since without my patch, nothing seems to call disconenct when the B plug is disconnected, I still see: # cat /sys/class/udc/f72c0000.usb/state configured thanks -john
On Wed, Oct 18, 2017 at 11:46 PM, Minas Harutyunyan <Minas.Harutyunyan@synopsys.com> wrote: > Could you please apply this patch. Please not apply your patch series > "[PATCH 0/3] dwc2 fixes for edge cases on hikey" to check only below patch. > If you confirm that this patch fix your issue with "Transaction Error" > and " ChHltd set, but reason is unknown" I'll submit to LKML as final patch. > Then can be applied your patch series, without patch 1/3 (changing in > connector id status change) to correctly set UDC states. > > diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index > f4ef159b538e..7da22152df68 100644 > --- a/drivers/usb/dwc2/hcd.c > +++ b/drivers/usb/dwc2/hcd.c > @@ -331,6 +331,9 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg) > usbcfg = dwc2_readl(hsotg->regs + GUSBCFG); > usbcfg &= ~(GUSBCFG_HNPCAP | GUSBCFG_SRPCAP); > > + /* Set HS/FS Timeout Calibration */ > + usbcfg |= GUSBCFG_TOUTCAL(7); > + > switch (hsotg->hw_params.op_mode) { > case GHWCFG2_OP_MODE_HNP_SRP_CAPABLE: > if (hsotg->params.otg_cap == So while using this patch and the earlier one from Vardan, I don't see the "Transaction Error" and " ChHltd set, but reason is unknown" messages, but I do see: [ 272.290459] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently in Host mode [ 272.290476] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently in Host mode [ 272.290491] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently in Host mode [ 272.290507] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently in Host mode [ 272.290522] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently in Host mode [ 272.290538] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently in Host mode [ 272.290554] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently in Host mode [ 272.290570] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently in Host mode [ 272.290585] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently in Host mode [ 272.290687] dwc2 f72c0000.usb: dwc2_hsotg_ep_stop_xfr: timeout DIEPINT.NAKEFF [ 272.290702] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently in Host mode [ 272.290717] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently in Host mode After which the USB eth device stops functioning. So its not really much functional change from without the patch from my perspective. Additionally, I'm still not seeing any disconnect calls when removing the B plug. Re-adding my three patches ontop of this change and Vardan's does fix both the UDC state handling and avoids usb devices from failing when the board is in host mode and the /dev/usb-ffs/adb/ep0 file is closed by adbd. thanks -john
Hi John Stultz, On 10/20/2017 12:06 AM, John Stultz wrote: > On Tue, Oct 17, 2017 at 1:41 AM, Minas Harutyunyan > <Minas.Harutyunyan@synopsys.com> wrote: >> On 10/17/2017 1:34 AM, John Stultz wrote: >>> On Mon, Oct 16, 2017 at 1:36 AM, Minas Harutyunyan >>> <Minas.Harutyunyan@synopsys.com> wrote: >>>> On b-plug disconnect should asserted GOTGINT.SesEndDet interrupt. >>>> According previously sent by you register dump (GHWCFG2 = 0x23affc70) >>>> your core OTG_MODE=0. >>>> Bellow fragment from programming guide on Device disconnect: >>>> >>>> "7.3Device Disconnection >>>> The device session ends when the USB cable is disconnected or if the >>>> VBUS is switched off by the Host. The >>>> device disconnect flow varies depending on the value of the OTG_MODE >>>> configuration parameter. >>>> >>>> When OTG_MODE = 0,1, or 3 >>>> When OTG_MODE is set to 0,1, or 3, the device disconnect flow is as follows: >>>> 1. When the USB cable is unplugged or when the VBUS is switched off by >>>> the Host, the Device core >>>> trigger GINTSTS.OTGInt [bit 2] interrupt bit. >>>> 2. When the device application detects GINTSTS.OTGInt interrupt, it >>>> checks that the >>>> GOTGINT.SesEndDet (Session End Detected) bit is set to 1’b1." >>>> >>>> So, you should receive and handle "Session End Detected". In function >>>> dwc2_handle_otg_intr() on this interrupt (in device mode) calling >>>> dwc2_hsotg_disconnect() function. By adding your patch "[PATCH 3/3] usb: >>>> dwc2: Fix UDC state tracking" state changed to not attached as required. >>> >>> >>> So, on the HiKey board (using 4.14-rc5 + Vardan's patch), I'm not >>> seeing the GOTGINT_SES_END_DET in dwc2_handle_otg_intr() when I remove >>> the USB OTG cable. >>> >>> In fact, I'm not seeing any calls to dwc2_handle_otg_intr()... which >>> seems... odd maybe? Any clues as to what might be going wrong then? >>> >>> thanks >>> -john >>> >> Hi John Stultz, >> So, on Hikey board on unplug B connector GOTGINT.SesEndDet interrupt not >> asserted, instead asserted GINTSTS_CONIDSTSCHNG. Please, confirm. > > Correct. On B unplug, I see: > dwc2_handle_conn_id_status_change_intr: ++Connector ID Status Change > Interrupt++ (Host) I can't understand how on B unplug connector id status changed to host! More probable it's board design issue. Please investigate hikey design. Electrically Connector ID pin status changed when plugged A connector and when unplugged A connector because of in A connector internally ConnID pin connected to ground. In B connector ConnID pin floating. > > And I never see any calls to dwc2_handle_otg_intr(). > >> In this case without your patch "[PATCH 1/3] usb: dwc2: Improve gadget >> state disconnection handling" but by applying your patch "[PATCH 3/3] >> usb: dwc2: Fix UDC state tracking": >> 1. On B plug connect UDC state will be set to "configured" >> 2. On B plug disconnect - "not attached". >> Is it Ok for you? > On our setup I tested your patch 3/3 without patches 1/3 and 2/3 and its changed state correctly. > So this is what I expect, but I don't see it. Since without my patch, > nothing seems to call disconenct when the B plug is disconnected, I > still see: > # cat /sys/class/udc/f72c0000.usb/state > configured > > thanks > -john >
On 10/20/2017 12:20 AM, John Stultz wrote: > On Wed, Oct 18, 2017 at 11:46 PM, Minas Harutyunyan > <Minas.Harutyunyan@synopsys.com> wrote: >> Could you please apply this patch. Please not apply your patch series >> "[PATCH 0/3] dwc2 fixes for edge cases on hikey" to check only below patch. >> If you confirm that this patch fix your issue with "Transaction Error" >> and " ChHltd set, but reason is unknown" I'll submit to LKML as final patch. >> Then can be applied your patch series, without patch 1/3 (changing in >> connector id status change) to correctly set UDC states. >> >> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index >> f4ef159b538e..7da22152df68 100644 >> --- a/drivers/usb/dwc2/hcd.c >> +++ b/drivers/usb/dwc2/hcd.c >> @@ -331,6 +331,9 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg) >> usbcfg = dwc2_readl(hsotg->regs + GUSBCFG); >> usbcfg &= ~(GUSBCFG_HNPCAP | GUSBCFG_SRPCAP); >> >> + /* Set HS/FS Timeout Calibration */ >> + usbcfg |= GUSBCFG_TOUTCAL(7); >> + >> switch (hsotg->hw_params.op_mode) { >> case GHWCFG2_OP_MODE_HNP_SRP_CAPABLE: >> if (hsotg->params.otg_cap == > > > So while using this patch and the earlier one from Vardan, I don't see > the "Transaction Error" > and " ChHltd set, but reason is unknown" messages, but I do see: > > [ 272.290459] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently > in Host mode > [ 272.290476] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently > in Host mode > [ 272.290491] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently > in Host mode > [ 272.290507] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently > in Host mode > [ 272.290522] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently > in Host mode > [ 272.290538] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently > in Host mode > [ 272.290554] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently > in Host mode > [ 272.290570] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently > in Host mode > [ 272.290585] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently > in Host mode > [ 272.290687] dwc2 f72c0000.usb: dwc2_hsotg_ep_stop_xfr: timeout DIEPINT.NAKEFF > [ 272.290702] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently > in Host mode > [ 272.290717] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently > in Host mode > > After which the USB eth device stops functioning. > > So its not really much functional change from without the patch from > my perspective. > > Additionally, I'm still not seeing any disconnect calls when removing > the B plug. > > Re-adding my three patches ontop of this change and Vardan's does fix > both the UDC state handling and avoids usb devices from failing when > the board is in host mode and the /dev/usb-ffs/adb/ep0 file is closed > by adbd. > > > thanks > -john > Hi John Stultz, It's good news that you not see more "Transaction error" and "ChHlt set, but reason is unknown"! To avoid "Mode Mismatch" interrupt please apply your patch 2/3. Additionally I see that in your setup used UTMI+ 8-bit PHY but GUSBCFG.USBTRDTIM set to 5, but should be 9. In device mode in function dwc2_hsotg_core_init_disconnected() GUSBCFG related fields TOUTCAL and USBTRDTIM setting again, but in Host mode these fields are not set at all. This can be reason why using your patch 1/3 you overcome TOUTCAL and USBTRDTIM issue. So, suggesting another patch for USBTRDTIM field bellow: @@ -2311,10 +2314,17 @@ static int dwc2_core_init(struct dwc2_hsotg *hsotg, bool initial_setup) */ static void dwc2_core_host_init(struct dwc2_hsotg *hsotg) { - u32 hcfg, hfir, otgctl; + u32 hcfg, hfir, otgctl, usbcfg, trdtrim; dev_dbg(hsotg->dev, "%s(%p)\n", __func__, hsotg); + /* Set USBTrdTim value */ + usbcfg = dwc2_readl(hsotg->regs + GUSBCFG); + usbcfg &= ~GUSBCFG_USBTRDTIM_MASK; + trdtrim = (hsotg->phyif == GUSBCFG_PHYIF8) ? 9 : 5; + usbcfg |= (trdtrim << GUSBCFG_USBTRDTIM_SHIFT); + dwc2_writel(usbcfg, hsotg->regs + GUSBCFG); + /* Restart the Phy Clock */ dwc2_writel(0, hsotg->regs + PCGCTL); Please apply follow patches: 1. Vardan's patch. 2. Patch for TOUTCAL. 3. Patch for USBTRDTIM (see above). 4. Your patch 2/3 to avoid "Mode Mismatch" interrupts. 5. Your patch 3/3 to set udc state to "not attached". Unfortunately, we haven't hikey board in our lab and can't fully test patches on our side. Thanks, Minas
On 10/20/2017 3:57 PM, Minas Harutyunyan wrote: > On 10/20/2017 12:20 AM, John Stultz wrote: >> On Wed, Oct 18, 2017 at 11:46 PM, Minas Harutyunyan >> <Minas.Harutyunyan@synopsys.com> wrote: >>> Could you please apply this patch. Please not apply your patch series >>> "[PATCH 0/3] dwc2 fixes for edge cases on hikey" to check only below patch. >>> If you confirm that this patch fix your issue with "Transaction Error" >>> and " ChHltd set, but reason is unknown" I'll submit to LKML as final patch. >>> Then can be applied your patch series, without patch 1/3 (changing in >>> connector id status change) to correctly set UDC states. >>> >>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index >>> f4ef159b538e..7da22152df68 100644 >>> --- a/drivers/usb/dwc2/hcd.c >>> +++ b/drivers/usb/dwc2/hcd.c >>> @@ -331,6 +331,9 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg) >>> usbcfg = dwc2_readl(hsotg->regs + GUSBCFG); >>> usbcfg &= ~(GUSBCFG_HNPCAP | GUSBCFG_SRPCAP); >>> >>> + /* Set HS/FS Timeout Calibration */ >>> + usbcfg |= GUSBCFG_TOUTCAL(7); >>> + >>> switch (hsotg->hw_params.op_mode) { >>> case GHWCFG2_OP_MODE_HNP_SRP_CAPABLE: >>> if (hsotg->params.otg_cap == >> >> >> So while using this patch and the earlier one from Vardan, I don't see >> the "Transaction Error" >> and " ChHltd set, but reason is unknown" messages, but I do see: >> >> [ 272.290459] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently >> in Host mode >> [ 272.290476] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently >> in Host mode >> [ 272.290491] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently >> in Host mode >> [ 272.290507] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently >> in Host mode >> [ 272.290522] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently >> in Host mode >> [ 272.290538] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently >> in Host mode >> [ 272.290554] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently >> in Host mode >> [ 272.290570] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently >> in Host mode >> [ 272.290585] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently >> in Host mode >> [ 272.290687] dwc2 f72c0000.usb: dwc2_hsotg_ep_stop_xfr: timeout DIEPINT.NAKEFF >> [ 272.290702] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently >> in Host mode >> [ 272.290717] dwc2 f72c0000.usb: Mode Mismatch Interrupt: currently >> in Host mode >> >> After which the USB eth device stops functioning. >> >> So its not really much functional change from without the patch from >> my perspective. >> >> Additionally, I'm still not seeing any disconnect calls when removing >> the B plug. >> >> Re-adding my three patches ontop of this change and Vardan's does fix >> both the UDC state handling and avoids usb devices from failing when >> the board is in host mode and the /dev/usb-ffs/adb/ep0 file is closed >> by adbd. >> >> >> thanks >> -john >> > > Hi John Stultz, > > It's good news that you not see more "Transaction error" and "ChHlt set, > but reason is unknown"! > > To avoid "Mode Mismatch" interrupt please apply your patch 2/3. > > Additionally I see that in your setup used UTMI+ 8-bit PHY but > GUSBCFG.USBTRDTIM set to 5, but should be 9. > In device mode in function dwc2_hsotg_core_init_disconnected() GUSBCFG > related fields TOUTCAL and USBTRDTIM setting again, but in Host mode > these fields are not set at all. This can be reason why using your patch > 1/3 you overcome TOUTCAL and USBTRDTIM issue. > > So, suggesting another patch for USBTRDTIM field bellow: > > @@ -2311,10 +2314,17 @@ static int dwc2_core_init(struct dwc2_hsotg > *hsotg, bool initial_setup) > */ > static void dwc2_core_host_init(struct dwc2_hsotg *hsotg) > { > - u32 hcfg, hfir, otgctl; > + u32 hcfg, hfir, otgctl, usbcfg, trdtrim; > > dev_dbg(hsotg->dev, "%s(%p)\n", __func__, hsotg); > > + /* Set USBTrdTim value */ > + usbcfg = dwc2_readl(hsotg->regs + GUSBCFG); > + usbcfg &= ~GUSBCFG_USBTRDTIM_MASK; > + trdtrim = (hsotg->phyif == GUSBCFG_PHYIF8) ? 9 : 5; > + usbcfg |= (trdtrim << GUSBCFG_USBTRDTIM_SHIFT); > + dwc2_writel(usbcfg, hsotg->regs + GUSBCFG); > + > /* Restart the Phy Clock */ > dwc2_writel(0, hsotg->regs + PCGCTL); > > > Please apply follow patches: > 1. Vardan's patch. > 2. Patch for TOUTCAL. > 3. Patch for USBTRDTIM (see above). > 4. Your patch 2/3 to avoid "Mode Mismatch" interrupts. > 5. Your patch 3/3 to set udc state to "not attached". > > Unfortunately, we haven't hikey board in our lab and can't fully test > patches on our side. > > > Thanks, > Minas > Hi John Stultz, Could you please verify on your setup follow patches: 1. Vardan's patch. 2. Patch for TOUTCAL&USBTRDTIM programming (new version see below). 4. Your patch 2/3 to avoid "Mode Mismatch" interrupts. 5. Your patch 3/3 to set udc state to "not attached". 6. Your patch 1/3, but remove dwc2_hsotg_core_init_disconnected() function call from Host starting brnch, keep *only* dwc2_hsotg_disconnect() to change UDC state to "not attached". diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index f4ef159b538e..1f65464055b9 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -2311,10 +2311,17 @@ static int dwc2_core_init(struct dwc2_hsotg *hsotg, bool initial_setup) */ static void dwc2_core_host_init(struct dwc2_hsotg *hsotg) { - u32 hcfg, hfir, otgctl; + u32 hcfg, hfir, otgctl, usbcfg, val; dev_dbg(hsotg->dev, "%s(%p)\n", __func__, hsotg); + /* Set HS/FS Timeout Calibration and USBTrdTim */ + usbcfg = dwc2_readl(hsotg->regs + GUSBCFG); + usbcfg &= ~(GUSBCFG_TOUTCAL_MASK | GUSBCFG_USBTRDTIM_MASK); + val = (hsotg->phyif == GUSBCFG_PHYIF8) ? 9 : 5; + usbcfg |= (GUSBCFG_TOUTCAL(7) | (val << GUSBCFG_USBTRDTIM_SHIFT)); + dwc2_writel(usbcfg, hsotg->regs + GUSBCFG); + /* Restart the Phy Clock */ dwc2_writel(0, hsotg->regs + PCGCTL); Thanks, Minas
On Mon, Oct 23, 2017 at 2:19 AM, Minas Harutyunyan <Minas.Harutyunyan@synopsys.com> wrote: > Could you please verify on your setup follow patches: > 1. Vardan's patch. > 2. Patch for TOUTCAL&USBTRDTIM programming (new version see below). > 4. Your patch 2/3 to avoid "Mode Mismatch" interrupts. > 5. Your patch 3/3 to set udc state to "not attached". > 6. Your patch 1/3, but remove dwc2_hsotg_core_init_disconnected() > function call from Host starting brnch, keep *only* > dwc2_hsotg_disconnect() to change UDC state to "not attached". So yes, this set does seem to work ok for me. Though neither Vardan's patch or the TOUTCAL/USBTRDTIM patch seem to have much effect either way (I need to do more testing just to be sure, but for the use cases I've had trouble with they don't seem to do much). I'm happy to rework my earlier patch #1/3 to remove dwc2_hsotg_core_init_disconnected() and resend. thanks -john