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?
> + 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?
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...
>
> - 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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
