> -----Original Message----- > From: dianders@xxxxxxxxxx [mailto:dianders@xxxxxxxxxx] On Behalf Of > Doug Anderson > Sent: Thursday, September 17, 2015 9:42 PM > To: Kaukab, Yousaf > Cc: linux-usb@xxxxxxxxxxxxxxx; Felipe Balbi; John Youn; Herrero, Gregory; Heiko > Stübner; r.baldyga@xxxxxxxxxxx; Dinh Nguyen; Zhangfei Gao; > sergei.shtylyov@xxxxxxxxxxxxxxxxxx; david.a.cohen@xxxxxxxxxxxxxxx > Subject: Re: [PATCH v1 02/32] usb: dwc2: host: create a function to handle > port_resume > > Hi, > > On Wed, Sep 9, 2015 at 3:20 AM, Mian Yousaf Kaukab > <yousaf.kaukab@xxxxxxxxx> wrote: > > From: Gregory Herrero <gregory.herrero@xxxxxxxxx> > > > > port resume sequence may be used in different places. Create a > > function to handle it. Moreover, make hprt0 read-modify-write atomic. > > > > Signed-off-by: Gregory Herrero <gregory.herrero@xxxxxxxxx> > > Signed-off-by: Mian Yousaf Kaukab <yousaf.kaukab@xxxxxxxxx> > > Tested-by: Robert Baldyga <r.baldyga@xxxxxxxxxxx> > > --- > > drivers/usb/dwc2/hcd.c | 30 ++++++++++++++++++++++-------- > > 1 file changed, 22 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index > > 007a3d5..0cef770 100644 > > --- a/drivers/usb/dwc2/hcd.c > > +++ b/drivers/usb/dwc2/hcd.c > > @@ -1484,6 +1484,27 @@ static void dwc2_port_suspend(struct dwc2_hsotg > *hsotg, u16 windex) > > } > > } > > > > +/* Must NOT be called with interrupt disabled or spinlock held */ > > +static void dwc2_port_resume(struct dwc2_hsotg *hsotg) { > > + unsigned long flags; > > + u32 hprt0; > > + > > + spin_lock_irqsave(&hsotg->lock, flags); > > + hprt0 = dwc2_read_hprt0(hsotg); > > + hprt0 |= HPRT0_RES; > > + hprt0 &= ~HPRT0_SUSP; > > + writel(hprt0, hsotg->regs + HPRT0); > > Just from a completely "what changed" point of view, you have made a non- > obvious behavior change here. Previously the code only masked out > HPRT0_SUSP in the 2nd write, not the first. Was that a bug in the old code, or > does it just not matter? It doesn't matter. I will add this to the change log anyway. HPRT0_SUSP is "read, write-set, and self-clear (R_WS_SC)" Application can only set it and writing 0 has no effect. Hardware should clear it when we set HPRT0_RES. So it doesn't matter if it is set in the first write. But since we are clearing it, it's better to clear it for both writes. So that there is no chance hardware can see this as another assertion of this bit. > > > + spin_unlock_irqrestore(&hsotg->lock, flags); > > + > > + msleep(USB_RESUME_TIMEOUT); > > + > > + spin_lock_irqsave(&hsotg->lock, flags); > > + hprt0 &= ~HPRT0_RES; > > + writel(hprt0, hsotg->regs + HPRT0); > > Just curious: since you released and re-grabbed the lock, shouldn't you re-read > HPRT0? Agree we should. I will update the patch. > > Also: one thing that was added in my local tree here was to make things > parallel to dwc2_port_suspend() and update "hsotg->lx_state" > here. I'm not sure if it was added it because there as a real issue or if it just > seemed better... > > > + spin_unlock_irqrestore(&hsotg->lock, flags); } > > + > > /* Handles hub class-specific requests */ static int > > dwc2_hcd_hub_control(struct dwc2_hsotg *hsotg, u16 typereq, > > u16 wvalue, u16 windex, char *buf, u16 > > wlength) @@ -1532,14 +1553,7 @@ static int dwc2_hcd_hub_control(struct > dwc2_hsotg *hsotg, u16 typereq, > > writel(0, hsotg->regs + PCGCTL); > > usleep_range(20000, 40000); > > In my local tree we already had a dwc2_port_resume() function and it included > the two lines above. I presume you didn't include them in your patch because > future callers of dwc2_port_resume() don't want those lines. That's fine, but I > figured I'd mention it... dwc2_exit_hibernation() is taking care of it for the other caller. It's better to move this to dwc2_port_resume() to keep it balanced with dwc2_port_suspend(). I will update " usb: dwc2: host: enter hibernation during bus suspend" to add the core_params->hibernation check to this new part as well. > > > > > - hprt0 = dwc2_read_hprt0(hsotg); > > - hprt0 |= HPRT0_RES; > > - writel(hprt0, hsotg->regs + HPRT0); > > - hprt0 &= ~HPRT0_SUSP; > > - msleep(USB_RESUME_TIMEOUT); > > - > > - hprt0 &= ~HPRT0_RES; > > - writel(hprt0, hsotg->regs + HPRT0); > > + dwc2_port_resume(hsotg); > > break; > > > > case USB_PORT_FEAT_POWER: > > -Doug BR, Yousaf ��.n��������+%������w��{.n�����{���)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
