Received: by 2002:ad5:474a:0:0:0:0:0 with SMTP id i10csp801107imu; Tue, 20 Nov 2018 07:10:53 -0800 (PST) X-Google-Smtp-Source: AFSGD/ViGxStszCCVhNPJyMCgJiN4rDzUc//mocFTdb6xfis7fAWssf2o5RUJOnKDqJ5lJXDVE5z X-Received: by 2002:a65:5bc4:: with SMTP id o4mr2263092pgr.426.1542726653522; Tue, 20 Nov 2018 07:10:53 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1542726653; cv=none; d=google.com; s=arc-20160816; b=U4O0HqrV9R5OdjNai7v/H1AcAwJNV5HBs/Ck0t2/aj0Dewkwh652uUDpm/xKW1zRA3 sH9+u4D5UKKyamRgX3kH1XwQQSj8NQofKVUmLbBpMv0SwhGEC1xfUloSU6cyODNuEZhO +THioY6Ki6SmGhCuk9zuj5+hmhtMYPJ8SY9MNGmi9dSQXqw88ik8b+FZT3Lqn0H1/573 Xmci9VF1yiX/tOl+xNNyNAsB2jvb4iCv51W0tjvnVXWrOEWIsUstvMbTNSpdlycNNzOm PcrD7kFZDUL9vvsWYJPveL235B//haTg0W7wnhD8pmWbEWjYa+8UUSKm9eWOqOmS685D u9yQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject; bh=qOwKg9xq9HBs1hUz3Uew2gYbk789GHHVlHNSK12BU5A=; b=Cc+7Fm6ysAcEefwZX4vnAZvcy4eg8tg7ubtffMtSF1ND0VLtBYektGPfcbk0kQ8fjJ 86vbw2YhnBMo4EB0rOBcMpcd2Wlgx7jRWXpSaniX/X/0J6XGZsT/5bYBK6nCUvnIcp5Q 35pJoxyQ/HqNG0lvFCLdpB75B0VndwNXDndVLyug6wZsH3RJMNlTzXjloAM8oa/1YECG Q1E7Jf8qZyyuO6VFKvT2Y/Q1LwVKyRIwiTe2t9d8Ikto7SJWZPTFHxpNRd2soFfOH7gM PrdmozMy4XNzmTFaxM8TmDT7P5qGYWHpj3RzcVV3c+xiKUCQi0GBwii6DkKPOeqMcpiM 3ZSw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id f11-v6si41379034pgi.378.2018.11.20.07.10.37; Tue, 20 Nov 2018 07:10:53 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728251AbeKUBIU (ORCPT + 99 others); Tue, 20 Nov 2018 20:08:20 -0500 Received: from mga14.intel.com ([192.55.52.115]:43015 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726040AbeKUBIU (ORCPT ); Tue, 20 Nov 2018 20:08:20 -0500 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 20 Nov 2018 06:38:52 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,257,1539673200"; d="scan'208";a="92611581" Received: from mattu-haswell.fi.intel.com (HELO [10.237.72.164]) ([10.237.72.164]) by orsmga006.jf.intel.com with ESMTP; 20 Nov 2018 06:38:50 -0800 Subject: Re: [PATCH] xhci: workaround CSS timeout on AMD SNPS 3.0 xHC. To: Kai Heng Feng , "Singh, Sandeep" Cc: "mathias.nyman@intel.com" , "gregkh@linuxfoundation.org" , "linux-usb@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "S-k, Shyam-sundar" , "Shah, Nehal-bakulchandra" References: <1542356426-10299-1-git-send-email-Sandeep.Singh@amd.com> <0B58593B-1CE2-49EB-9F13-A2BC449AB8E8@canonical.com> From: Mathias Nyman Message-ID: <63e848ea-3694-fef1-fab0-8c17000a3581@linux.intel.com> Date: Tue, 20 Nov 2018 16:42:34 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <0B58593B-1CE2-49EB-9F13-A2BC449AB8E8@canonical.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 16.11.2018 10:35, Kai Heng Feng wrote: > Hi Sandeep, > >> On Nov 16, 2018, at 16:21, Singh, Sandeep wrote: >> >> From: Sandeep Singh >> >> Occasionally AMD SNPS 3.0 xHC does not respond to >> CSS when set, also it does not flag anything on SRE and HCE >> to point the internal xHC errors on USBSTS register. This stalls >> the entire system wide suspend and there is no point in stalling >> just because of xHC CSS is not responding. >> >> To work around this problem, if the xHC does not flag >> anything on SRE and HCE, we can skip the CSS >> timeout and allow the system to continue the suspend. Once the >> system resume happens we can internally reset the controller >> using XHCI_RESET_ON_RESUME quirk. > > What happens to the connected and suspended USB devices? > Do USB devices lose remote wakeup functionality when this happens? > >> >> Signed-off-by: Shyam Sundar S K >> Signed-off-by: Sandeep Singh >> cc: Nehal Shah >> --- >> drivers/usb/host/xhci-pci.c | 4 ++++ >> drivers/usb/host/xhci.c | 25 +++++++++++++++++++++++++ >> drivers/usb/host/xhci.h | 1 + >> 3 files changed, 30 insertions(+) >> >> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c >> index 01c5705..72493c4 100644 >> --- a/drivers/usb/host/xhci-pci.c >> +++ b/drivers/usb/host/xhci-pci.c >> @@ -139,6 +139,10 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci) >> pdev->device == 0x43bb)) >> xhci->quirks |= XHCI_SUSPEND_DELAY; >> >> + if (pdev->vendor == PCI_VENDOR_ID_AMD && >> + (pdev->device == 0x15e0 || pdev->device == 0x15e1)) >> + xhci->quirks |= XHCI_SNPS_BROKEN_SUSPEND; >> + >> if (pdev->vendor == PCI_VENDOR_ID_AMD) >> xhci->quirks |= XHCI_TRUST_TX_LENGTH; >> >> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c >> index 0420eef..965b503 100644 >> --- a/drivers/usb/host/xhci.c >> +++ b/drivers/usb/host/xhci.c >> @@ -970,6 +970,7 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup) >> unsigned int delay = XHCI_MAX_HALT_USEC; >> struct usb_hcd *hcd = xhci_to_hcd(xhci); >> u32 command; >> + u32 res; >> >> if (!hcd->state) >> return 0; >> @@ -1025,10 +1026,32 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup) >> writel(command, &xhci->op_regs->command); >> if (xhci_handshake(&xhci->op_regs->status, >> STS_SAVE, 0, 10 * 1000)) { >> + if (xhci->quirks & XHCI_SNPS_BROKEN_SUSPEND) { >> + /* >> + * AMD SNPS xHC 3.0 occasionally does not clear the >> + * SSS bit of USBSTS and when driver tries to poll >> + * to see if the xHC clears BIT(8) which never happens >> + * and driver assumes that controller is not responding >> + * and times out. To workaround this, its good to check >> + * if SRE and HCE bits are not set (as per xhci >> + * Section 5.4.2) and bypass the timeout. >> + */ >> + >> + res = readl(&xhci->op_regs->status); >> + if (res & STS_SAVE) { >> + if (((res & STS_SRE) == 0) && >> + ((res & STS_HCE) == 0)) { >> + xhci->quirks |= XHCI_RESET_ON_RESUME; Better to use some other way or variable, after this change quirks would become dynamic, and depend on each other. >> + goto complete_suspend; >> + } >> + } > > Maybe merge the two “ifs”? There are no other conditions to handle. >> Kai-Heng Or drop the if (res & STS_SAVE) check completely. Only reason we are here is because STS_SAVE is still set. I think the goto statement is not needed either, how about something like if (BROKEN_SUSPEND_QUIRK && !(SRE || HCE)) set some reset on resume flag else unlock return -ETIMEDOUT > >> + } >> + >> xhci_warn(xhci, "WARN: xHC save state timeout\n"); >> spin_unlock_irq(&xhci->lock); >> return -ETIMEDOUT; >> } >> + complete_suspend: >> spin_unlock_irq(&xhci->lock); >> >> /* >> @@ -1213,6 +1236,8 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated) >> usb_hcd_poll_rh_status(xhci->shared_hcd); >> set_bit(HCD_FLAG_POLL_RH, &hcd->flags); >> usb_hcd_poll_rh_status(hcd); >> + if (xhci->quirks & XHCI_SNPS_BROKEN_SUSPEND) >> + xhci->quirks &= ~XHCI_RESET_ON_RESUME; Again, I don't think its a good idea to create this kind of quirk dependency, what about if a future controller needs both SNPS_BROKEN_SUSPEND and always a RESET_ON_RESUME? -Mathias >> >> return retval; >> } >> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h >> index bf0b369..eb99782 100644 >> --- a/drivers/usb/host/xhci.h >> +++ b/drivers/usb/host/xhci.h >> @@ -1849,6 +1849,7 @@ struct xhci_hcd { >> #define XHCI_INTEL_USB_ROLE_SW BIT_ULL(31) >> #define XHCI_ZERO_64B_REGS BIT_ULL(32) >> #define XHCI_DEFAULT_PM_RUNTIME_ALLOW BIT_ULL(33) >> +#define XHCI_SNPS_BROKEN_SUSPEND BIT_ULL(34) >> >> unsigned int num_active_eps; >> unsigned int limit_active_eps; >> -- >> 2.7.4 >> >