On Fri, Sep 06, 2013 at 06:24:43PM +0800, Huang Rui wrote:
> The following patch is required to resolve remote wake issues with
> certain devices.
>
> Issue description:
> If the remote wake is issued from the device in a specific timing
> condition while the system is entering sleep state then it may cause
> system to auto wake on subsequent sleep cycle.
>
> Root cause:
> Host controller rebroadcasts the Resume signal > 100 µseconds after
> receiving the original resume event from the device. For proper
> function, some devices may require the rebroadcast of resume event
> within the USB spec of 100µS.
>
> Workaroud:
> 1. Filter the special platforms, then judge of all the usb devices are
> mouse or not. And get out the port id which attached a mouse with Pixart
> controller.
> 2. Then reset the port which attached issue device during system resume
> from S3.
Why are you doing this in the xHCI driver? You're resetting a device
from the host controller driver without letting the USB core know about
it? That seems like a bad idea.
You don't even time the reset, or clear the reset change bit when the
reset completes. You're expecting the USB core to come along and clear
the reset change bit later, which I'm not sure if it will if it didn't
issue the reset.
Why aren't we making the USB core reset the device through the normal
code paths? Why don't you add new API to the USB host controller driver
to see if a host has this new quirk, and have the USB core do the reset?
A simpler solution would be to just set the XHCI_RESET_ON_RESUME quirk,
so that the host controller gets reset on resume from S3 and the devices
are reset and re-enumerated. I like that better than resetting a device
behind the USB core's back.
> [Q] Why the special devices are only mice? Would high speed devices
> such as 3G modem or USB Bluetooth adapter trigger this issue?
> - Current this sensitivity is only confined to devices that use Pixart
> controllers. This controller is designed for use with LS mouse
> devices only. We have not observed any other devices failing. There
> may be a small risk for other devices also but this patch (reset
> device in resume phase) will cover the cases if required.
>
> [Q] Shouldn’t the resume signal be sent within 100 us for every
> device?
> - The Host controller may not send the resume signal within 100us,
> this our host controller specification change. This is why we
> require the patch to prevent side effects on certain known devices.
>
> [Q] Why would clicking mouse INTENSELY to wake the system up trigger
> this issue?
> - This behavior is specific to the devices that use Pixart controller.
> It is timing dependent on when the resume event is triggered during
> the sleep state.
>
> [Q] Is it a host controller issue or mouse?
> - It is the host controller behavior during resume that triggers the
> device incorrect behavior on the next resume.
>
> This patch is only for xHCI driver and the quirk will be also added
> into OHCI driver too.
>
> Signed-off-by: Huang Rui <ray.huang@xxxxxxx>
> ---
> drivers/usb/host/xhci-pci.c | 4 ++++
> drivers/usb/host/xhci.c | 57 +++++++++++++++++++++++++++++++++++++++++++++
> drivers/usb/host/xhci.h | 35 ++++++++++++++--------------
> 3 files changed, 79 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index c2d4950..cc2ff7e 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -90,6 +90,10 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
> /* AMD PLL quirk */
> if (pdev->vendor == PCI_VENDOR_ID_AMD && usb_amd_find_chipset_info())
> xhci->quirks |= XHCI_AMD_PLL_FIX;
> +
> + if (pdev->vendor == PCI_VENDOR_ID_AMD && usb_amd_remote_wakeup_quirk())
> + xhci->quirks |= XHCI_AMD_REMOTE_WAKEUP_QUIRK;
> +
> if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
> xhci->quirks |= XHCI_LPM_SUPPORT;
> xhci->quirks |= XHCI_INTEL_HOST;
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 49b6edb..bf749b0 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -831,6 +831,60 @@ static void xhci_clear_command_ring(struct xhci_hcd *xhci)
> }
>
> /*
> + * Reset port which attached with an issue device.
> + *
> + * If the remote wake is issued from the device in a specific timing
> + * condition while the system is entering sleep state then it may
> + * cause system to auto wake on subsequent sleep cycle.
> + *
> + * Host controller rebroadcasts the Resume signal > 100 µseconds after
> + * receiving the original resume event from the device. For proper
> + * function, some devices may require the rebroadcast of resume event
> + * within the USB spec of 100µS.
> + *
> + * Without this quirk, some usb mouse controller would react
> + * differently to this unexpected event from some AMD host controller
> + * and will result in the mouse to assert a resume event on the
> + * subsequent S3 sleep even if the user does not initiate the wake
> + * event by clicking on the mouse. So it should reset the port which
> + * attached with issue mouse during S3 reusme phase.
> + */
> +static int xhci_reset_port_by_amd_remote_wakeup(struct xhci_hcd *xhci)
> +{
> + __le32 __iomem *addr, *base_addr;
> + u32 temp;
> + struct usb_device *hdev, *child;
> + unsigned long flags;
> + int port1, raw_port;
> +
> + xhci_dbg(xhci, "%s, reset port attached with issue device\n",
> + __func__);
> +
> + base_addr = &xhci->op_regs->port_status_base;
> +
> + hdev = hcd_to_bus(xhci->main_hcd)->root_hub;
> +
> + usb_hub_for_each_child(hdev, port1, child) {
> + raw_port = xhci_find_raw_port_number(xhci->main_hcd,
> + port1);
> + addr = (raw_port - 1) * NUM_PORT_REGS + base_addr;
> +
> + temp = xhci_readl(xhci, addr);
> +
> + if (is_issue_device_for_amd_quirk(child)) {
> + xhci_dbg(xhci, "Attached issue device on port%d.\n",
> + port1);
> + spin_lock_irqsave(&xhci->lock, flags);
> + temp |= PORT_RESET;
> + xhci_writel(xhci, temp, addr);
> + spin_unlock_irqrestore(&xhci->lock, flags);
> + }
> + }
> +
> + return 0;
> +}
> +
> +/*
> * Stop HC (not bus-specific)
> *
> * This is called when the machine transition into S3/S4 mode.
> @@ -1043,6 +1097,9 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
> if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) && !comp_timer_running)
> compliance_mode_recovery_timer_init(xhci);
>
> + if (xhci->quirks & XHCI_AMD_REMOTE_WAKEUP_QUIRK)
> + xhci_reset_port_by_amd_remote_wakeup(xhci);
> +
> /* Re-enable port polling. */
> xhci_dbg(xhci, "%s: starting port polling.\n", __func__);
> set_bit(HCD_FLAG_POLL_RH, &hcd->flags);
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 46aa148..ee72b1a 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1512,11 +1512,11 @@ struct xhci_hcd {
> /* Statistics */
> int error_bitmask;
> unsigned int quirks;
> -#define XHCI_LINK_TRB_QUIRK (1 << 0)
> -#define XHCI_RESET_EP_QUIRK (1 << 1)
> -#define XHCI_NEC_HOST (1 << 2)
> -#define XHCI_AMD_PLL_FIX (1 << 3)
> -#define XHCI_SPURIOUS_SUCCESS (1 << 4)
> +#define XHCI_LINK_TRB_QUIRK (1 << 0)
> +#define XHCI_RESET_EP_QUIRK (1 << 1)
> +#define XHCI_NEC_HOST (1 << 2)
> +#define XHCI_AMD_PLL_FIX (1 << 3)
> +#define XHCI_SPURIOUS_SUCCESS (1 << 4)
> /*
> * Certain Intel host controllers have a limit to the number of endpoint
> * contexts they can handle. Ideally, they would signal that they can't handle
> @@ -1526,18 +1526,19 @@ struct xhci_hcd {
> * commands, reset device commands, disable slot commands, and address device
> * commands.
> */
> -#define XHCI_EP_LIMIT_QUIRK (1 << 5)
> -#define XHCI_BROKEN_MSI (1 << 6)
> -#define XHCI_RESET_ON_RESUME (1 << 7)
> -#define XHCI_SW_BW_CHECKING (1 << 8)
> -#define XHCI_AMD_0x96_HOST (1 << 9)
> -#define XHCI_TRUST_TX_LENGTH (1 << 10)
> -#define XHCI_LPM_SUPPORT (1 << 11)
> -#define XHCI_INTEL_HOST (1 << 12)
> -#define XHCI_SPURIOUS_REBOOT (1 << 13)
> -#define XHCI_COMP_MODE_QUIRK (1 << 14)
> -#define XHCI_AVOID_BEI (1 << 15)
> -#define XHCI_PLAT (1 << 16)
> +#define XHCI_EP_LIMIT_QUIRK (1 << 5)
> +#define XHCI_BROKEN_MSI (1 << 6)
> +#define XHCI_RESET_ON_RESUME (1 << 7)
> +#define XHCI_SW_BW_CHECKING (1 << 8)
> +#define XHCI_AMD_0x96_HOST (1 << 9)
> +#define XHCI_TRUST_TX_LENGTH (1 << 10)
> +#define XHCI_LPM_SUPPORT (1 << 11)
> +#define XHCI_INTEL_HOST (1 << 12)
> +#define XHCI_SPURIOUS_REBOOT (1 << 13)
> +#define XHCI_COMP_MODE_QUIRK (1 << 14)
> +#define XHCI_AVOID_BEI (1 << 15)
> +#define XHCI_PLAT (1 << 16)
> +#define XHCI_AMD_REMOTE_WAKEUP_QUIRK (1 << 17)
> unsigned int num_active_eps;
> unsigned int limit_active_eps;
> /* There are two roothubs to keep track of bus suspend info for */
> --
> 1.7.11.7
>
>
--
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
