Hi,
On Fri, Apr 27, 2018 at 03:00:05PM +0200, Daniel Glöckner wrote:
> It has been observed that writing 0xF2 to the power register while it
> reads as 0xF4 results in the register having the value 0xF0, i.e. clearing
> RESUME and setting SUSPENDM in one go does not work. It might also violate
> the USB spec to transition directly from resume to suspend, especially
> when not taking T_DRSMDN into account. But this is what happens when a
> remote wakeup occurs between SetPortFeature USB_PORT_FEAT_SUSPEND on the
> root hub and musb_bus_suspend being called.
>
> This commit returns -EBUSY when musb_bus_suspend is called while remote
> wakeup is signalled and thus avoids to reset the RESUME bit. Remember that
> resume can be signalled only when the bus was already suspended, so it is
> safe to skip the following steps even when musb_hub_control ignores the
what do you mean by "skip the following steps"?
> error returned by musb_port_suspend.
>
> The resume part of musb_port_suspend is modified to also accept a pending
> remote wakeup, to bring it to the end after T_DRSMDN has passed.
Can you please explain more here? I am not sure when musb_port_suspend()
is involved in resume by remote wakeup, and what case is a 'pending'
remote wakeup?
>
> Signed-off-by: Daniel Glöckner <dg@xxxxxxxxx>
> ---
> drivers/usb/musb/musb_host.c | 5 ++++-
> drivers/usb/musb/musb_host.h | 7 +++++--
> drivers/usb/musb/musb_virthub.c | 27 ++++++++++++++++-----------
> 3 files changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> index 3a8451a..d05cb03 100644
> --- a/drivers/usb/musb/musb_host.c
> +++ b/drivers/usb/musb/musb_host.c
> @@ -2522,8 +2522,11 @@ static int musb_bus_suspend(struct usb_hcd *hcd)
> {
> struct musb *musb = hcd_to_musb(hcd);
> u8 devctl;
> + int ret;
>
> - musb_port_suspend(musb, true);
> + ret = musb_port_suspend(musb, true);
> + if (ret)
> + return ret;
>
> if (!is_host_active(musb))
> return 0;
> diff --git a/drivers/usb/musb/musb_host.h b/drivers/usb/musb/musb_host.h
> index 72392bb..2999845 100644
> --- a/drivers/usb/musb/musb_host.h
> +++ b/drivers/usb/musb/musb_host.h
> @@ -67,7 +67,7 @@ extern void musb_host_rx(struct musb *, u8);
> extern void musb_root_disconnect(struct musb *musb);
> extern void musb_host_resume_root_hub(struct musb *musb);
> extern void musb_host_poke_root_hub(struct musb *musb);
> -extern void musb_port_suspend(struct musb *musb, bool do_suspend);
> +extern int musb_port_suspend(struct musb *musb, bool do_suspend);
> extern void musb_port_reset(struct musb *musb, bool do_reset);
> extern void musb_host_finish_resume(struct work_struct *work);
> #else
> @@ -99,7 +99,10 @@ static inline void musb_root_disconnect(struct musb *musb) {}
> static inline void musb_host_resume_root_hub(struct musb *musb) {}
> static inline void musb_host_poll_rh_status(struct musb *musb) {}
> static inline void musb_host_poke_root_hub(struct musb *musb) {}
> -static inline void musb_port_suspend(struct musb *musb, bool do_suspend) {}
> +static inline int musb_port_suspend(struct musb *musb, bool do_suspend)
> +{
> + return 0;
> +}
> static inline void musb_port_reset(struct musb *musb, bool do_reset) {}
> static inline void musb_host_finish_resume(struct work_struct *work) {}
> #endif
> diff --git a/drivers/usb/musb/musb_virthub.c b/drivers/usb/musb/musb_virthub.c
> index 5165d2b..41b44ce 100644
> --- a/drivers/usb/musb/musb_virthub.c
> +++ b/drivers/usb/musb/musb_virthub.c
> @@ -48,14 +48,14 @@ void musb_host_finish_resume(struct work_struct *work)
> spin_unlock_irqrestore(&musb->lock, flags);
> }
>
> -void musb_port_suspend(struct musb *musb, bool do_suspend)
> +int musb_port_suspend(struct musb *musb, bool do_suspend)
> {
> struct usb_otg *otg = musb->xceiv->otg;
> u8 power;
> void __iomem *mbase = musb->mregs;
>
> if (!is_host_active(musb))
> - return;
> + return 0;
>
> /* NOTE: this doesn't necessarily put PHY into low power mode,
> * turning off its clock; that's a function of PHY integration and
> @@ -66,16 +66,20 @@ void musb_port_suspend(struct musb *musb, bool do_suspend)
> if (do_suspend) {
> int retries = 10000;
>
> - power &= ~MUSB_POWER_RESUME;
> - power |= MUSB_POWER_SUSPENDM;
> - musb_writeb(mbase, MUSB_POWER, power);
> + if (power & MUSB_POWER_RESUME)
> + return -EBUSY;
>
> - /* Needed for OPT A tests */
> - power = musb_readb(mbase, MUSB_POWER);
> - while (power & MUSB_POWER_SUSPENDM) {
> + if (!(power & MUSB_POWER_SUSPENDM)) {
> + power |= MUSB_POWER_SUSPENDM;
> + musb_writeb(mbase, MUSB_POWER, power);
> +
> + /* Needed for OPT A tests */
> power = musb_readb(mbase, MUSB_POWER);
> - if (retries-- < 1)
> - break;
> + while (power & MUSB_POWER_SUSPENDM) {
> + power = musb_readb(mbase, MUSB_POWER);
> + if (retries-- < 1)
> + break;
> + }
> }
>
> musb_dbg(musb, "Root port suspended, power %02x", power);
> @@ -100,7 +104,7 @@ void musb_port_suspend(struct musb *musb, bool do_suspend)
> musb_dbg(musb, "bogus rh suspend? %s",
> usb_otg_state_string(musb->xceiv->otg->state));
> }
> - } else if (power & MUSB_POWER_SUSPENDM) {
> + } else if (power & (MUSB_POWER_SUSPENDM | MUSB_POWER_RESUME)) {
> power &= ~MUSB_POWER_SUSPENDM;
> power |= MUSB_POWER_RESUME;
> musb_writeb(mbase, MUSB_POWER, power);
> @@ -111,6 +115,7 @@ void musb_port_suspend(struct musb *musb, bool do_suspend)
> schedule_delayed_work(&musb->finish_resume_work,
> msecs_to_jiffies(USB_RESUME_TIMEOUT));
> }
> + return 0;
> }
>
> void musb_port_reset(struct musb *musb, bool do_reset)
Regards,
-Bin.
--
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
