| Message ID | 7920088e6620bd0b7739657c2bd8b703c2b43bb9.1544614809.git.hminas@synopsys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: dwc2: Reset device address on EnumDone | expand |
Hi Felipe, On 12/12/2018 3:43 PM, Minas Harutyunyan wrote: > Initially resetting device address was done in USB RESET interrupt > handler. In case, when power saving mode enabled (hibernation) USB > RESET interrupt handled in dwc2_handle_gpwrdn_intr() and then it > not seen in dwc2_hsotg_irq() handler. This is why reset device > address to zero moved from USB RESET handler to EnumDone handler. > > Signed-off-by: Minas Harutyunyan <hminas@synopsys.com> > --- > drivers/usb/dwc2/gadget.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > index 68ad75a7460d..7f922f19f8e1 100644 > --- a/drivers/usb/dwc2/gadget.c > +++ b/drivers/usb/dwc2/gadget.c > @@ -3072,6 +3072,9 @@ static void dwc2_hsotg_irq_enumdone(struct dwc2_hsotg *hsotg) > > dev_dbg(hsotg->dev, "EnumDone (DSTS=0x%08x)\n", dsts); > > + /* Reset device address to zero */ > + dwc2_clear_bit(hsotg, DCFG, DCFG_DEVADDR_MASK); > + > /* > * note, since we're limited by the size of transfer on EP0, and > * it seems IN transfers must be a even number of packets we do > @@ -3614,9 +3617,6 @@ static irqreturn_t dwc2_hsotg_irq(int irq, void *pw) > /* Report disconnection if it is not already done. */ > dwc2_hsotg_disconnect(hsotg); > > - /* Reset device address to zero */ > - dwc2_clear_bit(hsotg, DCFG, DCFG_DEVADDR_MASK); > - > if (usb_status & GOTGCTL_BSESVLD && connected) > dwc2_hsotg_core_init_disconnected(hsotg, true); > } > This patch not seen yet in your testing/fixes or next. Any reason for delay or you missed it? Thanks, Minas
Hi Felipe, On 1/21/2019 11:13 AM, Minas Harutyunyan wrote: > Hi Felipe, > > On 12/12/2018 3:43 PM, Minas Harutyunyan wrote: >> Initially resetting device address was done in USB RESET interrupt >> handler. In case, when power saving mode enabled (hibernation) USB >> RESET interrupt handled in dwc2_handle_gpwrdn_intr() and then it >> not seen in dwc2_hsotg_irq() handler. This is why reset device >> address to zero moved from USB RESET handler to EnumDone handler. >> >> Signed-off-by: Minas Harutyunyan <hminas@synopsys.com> >> --- >> drivers/usb/dwc2/gadget.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c >> index 68ad75a7460d..7f922f19f8e1 100644 >> --- a/drivers/usb/dwc2/gadget.c >> +++ b/drivers/usb/dwc2/gadget.c >> @@ -3072,6 +3072,9 @@ static void dwc2_hsotg_irq_enumdone(struct dwc2_hsotg *hsotg) >> >> dev_dbg(hsotg->dev, "EnumDone (DSTS=0x%08x)\n", dsts); >> >> + /* Reset device address to zero */ >> + dwc2_clear_bit(hsotg, DCFG, DCFG_DEVADDR_MASK); >> + >> /* >> * note, since we're limited by the size of transfer on EP0, and >> * it seems IN transfers must be a even number of packets we do >> @@ -3614,9 +3617,6 @@ static irqreturn_t dwc2_hsotg_irq(int irq, void *pw) >> /* Report disconnection if it is not already done. */ >> dwc2_hsotg_disconnect(hsotg); >> >> - /* Reset device address to zero */ >> - dwc2_clear_bit(hsotg, DCFG, DCFG_DEVADDR_MASK); >> - >> if (usb_status & GOTGCTL_BSESVLD && connected) >> dwc2_hsotg_core_init_disconnected(hsotg, true); >> } >> > > This patch not seen yet in your testing/fixes or next. Any reason for > delay or you missed it? > > Thanks, > Minas > > Not seen yet. Ping again. Thanks, Minas
Hi, Minas Harutyunyan <minas.harutyunyan@synopsys.com> writes: > Hi Felipe, > > On 1/21/2019 11:13 AM, Minas Harutyunyan wrote: >> Hi Felipe, >> >> On 12/12/2018 3:43 PM, Minas Harutyunyan wrote: >>> Initially resetting device address was done in USB RESET interrupt >>> handler. In case, when power saving mode enabled (hibernation) USB >>> RESET interrupt handled in dwc2_handle_gpwrdn_intr() and then it >>> not seen in dwc2_hsotg_irq() handler. This is why reset device >>> address to zero moved from USB RESET handler to EnumDone handler. >>> >>> Signed-off-by: Minas Harutyunyan <hminas@synopsys.com> >>> --- >>> drivers/usb/dwc2/gadget.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c >>> index 68ad75a7460d..7f922f19f8e1 100644 >>> --- a/drivers/usb/dwc2/gadget.c >>> +++ b/drivers/usb/dwc2/gadget.c >>> @@ -3072,6 +3072,9 @@ static void dwc2_hsotg_irq_enumdone(struct dwc2_hsotg *hsotg) >>> >>> dev_dbg(hsotg->dev, "EnumDone (DSTS=0x%08x)\n", dsts); >>> >>> + /* Reset device address to zero */ >>> + dwc2_clear_bit(hsotg, DCFG, DCFG_DEVADDR_MASK); >>> + >>> /* >>> * note, since we're limited by the size of transfer on EP0, and >>> * it seems IN transfers must be a even number of packets we do >>> @@ -3614,9 +3617,6 @@ static irqreturn_t dwc2_hsotg_irq(int irq, void *pw) >>> /* Report disconnection if it is not already done. */ >>> dwc2_hsotg_disconnect(hsotg); >>> >>> - /* Reset device address to zero */ >>> - dwc2_clear_bit(hsotg, DCFG, DCFG_DEVADDR_MASK); >>> - >>> if (usb_status & GOTGCTL_BSESVLD && connected) >>> dwc2_hsotg_core_init_disconnected(hsotg, true); >>> } >>> >> >> This patch not seen yet in your testing/fixes or next. Any reason for >> delay or you missed it? >> >> Thanks, >> Minas >> >> > Not seen yet. Ping again. I don't see any indication that this is a bug fix that needs to go during -rc cycle, so I was gonna queue it for next merge window. Frankly, moving address reset to enumdone sounds like a bad idea. It looks to me that you're moving the problem from one place to another because of hibernation. I would, rather, suggest that you review your interrupt handler and make sure it's compliant with your documentation. Why is RESET handled by HIBERNATION handler, for example? Anyway, I'm gonna wait for your reply before doing anything with this patch.
Hi Felipe, On 2/6/2019 10:44 AM, Felipe Balbi wrote: > > Hi, > > Minas Harutyunyan <minas.harutyunyan@synopsys.com> writes: >> Hi Felipe, >> >> On 1/21/2019 11:13 AM, Minas Harutyunyan wrote: >>> Hi Felipe, >>> >>> On 12/12/2018 3:43 PM, Minas Harutyunyan wrote: >>>> Initially resetting device address was done in USB RESET interrupt >>>> handler. In case, when power saving mode enabled (hibernation) USB >>>> RESET interrupt handled in dwc2_handle_gpwrdn_intr() and then it >>>> not seen in dwc2_hsotg_irq() handler. This is why reset device >>>> address to zero moved from USB RESET handler to EnumDone handler. >>>> >>>> Signed-off-by: Minas Harutyunyan <hminas@synopsys.com> >>>> --- >>>> drivers/usb/dwc2/gadget.c | 6 +++--- >>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c >>>> index 68ad75a7460d..7f922f19f8e1 100644 >>>> --- a/drivers/usb/dwc2/gadget.c >>>> +++ b/drivers/usb/dwc2/gadget.c >>>> @@ -3072,6 +3072,9 @@ static void dwc2_hsotg_irq_enumdone(struct dwc2_hsotg *hsotg) >>>> >>>> dev_dbg(hsotg->dev, "EnumDone (DSTS=0x%08x)\n", dsts); >>>> >>>> + /* Reset device address to zero */ >>>> + dwc2_clear_bit(hsotg, DCFG, DCFG_DEVADDR_MASK); >>>> + >>>> /* >>>> * note, since we're limited by the size of transfer on EP0, and >>>> * it seems IN transfers must be a even number of packets we do >>>> @@ -3614,9 +3617,6 @@ static irqreturn_t dwc2_hsotg_irq(int irq, void *pw) >>>> /* Report disconnection if it is not already done. */ >>>> dwc2_hsotg_disconnect(hsotg); >>>> >>>> - /* Reset device address to zero */ >>>> - dwc2_clear_bit(hsotg, DCFG, DCFG_DEVADDR_MASK); >>>> - >>>> if (usb_status & GOTGCTL_BSESVLD && connected) >>>> dwc2_hsotg_core_init_disconnected(hsotg, true); >>>> } >>>> >>> >>> This patch not seen yet in your testing/fixes or next. Any reason for >>> delay or you missed it? >>> >>> Thanks, >>> Minas >>> >>> >> Not seen yet. Ping again. > > I don't see any indication that this is a bug fix that needs to go > during -rc cycle, so I was gonna queue it for next merge window. > > Frankly, moving address reset to enumdone sounds like a bad idea. It > looks to me that you're moving the problem from one place to another > because of hibernation. > > I would, rather, suggest that you review your interrupt handler and make > sure it's compliant with your documentation. Why is RESET handled by > HIBERNATION handler, for example? Yes, you are right. I'll add clearing device address in hibernation handler, in case of exiting from hibernation triggered by USB Reset. Please ignore this patch. > > Anyway, I'm gonna wait for your reply before doing anything with this > patch. >
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index 68ad75a7460d..7f922f19f8e1 100644 --- a/drivers/usb/dwc2/gadget.c +++ b/drivers/usb/dwc2/gadget.c @@ -3072,6 +3072,9 @@ static void dwc2_hsotg_irq_enumdone(struct dwc2_hsotg *hsotg) dev_dbg(hsotg->dev, "EnumDone (DSTS=0x%08x)\n", dsts); + /* Reset device address to zero */ + dwc2_clear_bit(hsotg, DCFG, DCFG_DEVADDR_MASK); + /* * note, since we're limited by the size of transfer on EP0, and * it seems IN transfers must be a even number of packets we do @@ -3614,9 +3617,6 @@ static irqreturn_t dwc2_hsotg_irq(int irq, void *pw) /* Report disconnection if it is not already done. */ dwc2_hsotg_disconnect(hsotg); - /* Reset device address to zero */ - dwc2_clear_bit(hsotg, DCFG, DCFG_DEVADDR_MASK); - if (usb_status & GOTGCTL_BSESVLD && connected) dwc2_hsotg_core_init_disconnected(hsotg, true); }
Initially resetting device address was done in USB RESET interrupt handler. In case, when power saving mode enabled (hibernation) USB RESET interrupt handled in dwc2_handle_gpwrdn_intr() and then it not seen in dwc2_hsotg_irq() handler. This is why reset device address to zero moved from USB RESET handler to EnumDone handler. Signed-off-by: Minas Harutyunyan <hminas@synopsys.com> --- drivers/usb/dwc2/gadget.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)