* [PATCH] xhci: workaround for S3 issue on AMD SNPS 3.0 xHC
@ 2020-08-31 6:52 Nehal Bakulchandra Shah
2020-09-16 8:06 ` Greg KH
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Nehal Bakulchandra Shah @ 2020-08-31 6:52 UTC (permalink / raw)
To: mathias.nyman, gregkh, linux-usb, linux-kernel, Sandeep.Singh, yuanmei
Cc: Nehal Bakulchandra Shah
From: Nehal Bakulchandra Shah <Nehal-Bakulchandra.shah@amd.com>
On some platform of AMD, S3 fails with HCE and SRE errors.To fix this,
sparse controller enable bit has to be disabled.
Signed-off-by: Nehal Bakulchandra Shah <Nehal-Bakulchandra.shah@amd.com>
---
drivers/usb/host/xhci-pci.c | 12 ++++++++++++
drivers/usb/host/xhci.h | 1 +
2 files changed, 13 insertions(+)
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 3feaafebfe58..865a16e6c1ed 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -160,6 +160,9 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
(pdev->device == 0x15e0 || pdev->device == 0x15e1))
xhci->quirks |= XHCI_SNPS_BROKEN_SUSPEND;
+ if (pdev->vendor == PCI_VENDOR_ID_AMD && pdev->device == 0x15e5)
+ xhci->quirks |= XHCI_DISABLE_SPARSE;
+
if (pdev->vendor == PCI_VENDOR_ID_AMD)
xhci->quirks |= XHCI_TRUST_TX_LENGTH;
@@ -371,6 +374,15 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
/* USB 2.0 roothub is stored in the PCI device now. */
hcd = dev_get_drvdata(&dev->dev);
xhci = hcd_to_xhci(hcd);
+
+ if (xhci->quirks & XHCI_DISABLE_SPARSE) {
+ u32 reg;
+
+ reg = readl(hcd->regs + 0xC12C);
+ reg &= ~BIT(17);
+ writel(reg, hcd->regs + 0xC12C);
+ }
+
xhci->shared_hcd = usb_create_shared_hcd(&xhci_pci_hc_driver, &dev->dev,
pci_name(dev), hcd);
if (!xhci->shared_hcd) {
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index ea1754f185a2..ea966d70f1ee 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1874,6 +1874,7 @@ struct xhci_hcd {
#define XHCI_RESET_PLL_ON_DISCONNECT BIT_ULL(34)
#define XHCI_SNPS_BROKEN_SUSPEND BIT_ULL(35)
#define XHCI_RENESAS_FW_QUIRK BIT_ULL(36)
+#define XHCI_DISABLE_SPARSE BIT_ULL(37)
unsigned int num_active_eps;
unsigned int limit_active_eps;
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] xhci: workaround for S3 issue on AMD SNPS 3.0 xHC
2020-08-31 6:52 [PATCH] xhci: workaround for S3 issue on AMD SNPS 3.0 xHC Nehal Bakulchandra Shah
@ 2020-09-16 8:06 ` Greg KH
2020-09-16 12:47 ` Mathias Nyman
[not found] ` <1163986b-075f-8d5e-13dc-7141cb25e484@amd.com>
2020-09-16 12:30 ` Mathias Nyman
2 siblings, 1 reply; 5+ messages in thread
From: Greg KH @ 2020-09-16 8:06 UTC (permalink / raw)
To: Nehal Bakulchandra Shah
Cc: mathias.nyman, linux-usb, linux-kernel, Sandeep.Singh, yuanmei
On Mon, Aug 31, 2020 at 06:52:46AM +0000, Nehal Bakulchandra Shah wrote:
> From: Nehal Bakulchandra Shah <Nehal-Bakulchandra.shah@amd.com>
>
> On some platform of AMD, S3 fails with HCE and SRE errors.To fix this,
> sparse controller enable bit has to be disabled.
>
> Signed-off-by: Nehal Bakulchandra Shah <Nehal-Bakulchandra.shah@amd.com>
> ---
> drivers/usb/host/xhci-pci.c | 12 ++++++++++++
> drivers/usb/host/xhci.h | 1 +
> 2 files changed, 13 insertions(+)
>
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 3feaafebfe58..865a16e6c1ed 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -160,6 +160,9 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
> (pdev->device == 0x15e0 || pdev->device == 0x15e1))
> xhci->quirks |= XHCI_SNPS_BROKEN_SUSPEND;
>
> + if (pdev->vendor == PCI_VENDOR_ID_AMD && pdev->device == 0x15e5)
> + xhci->quirks |= XHCI_DISABLE_SPARSE;
> +
> if (pdev->vendor == PCI_VENDOR_ID_AMD)
> xhci->quirks |= XHCI_TRUST_TX_LENGTH;
>
> @@ -371,6 +374,15 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
> /* USB 2.0 roothub is stored in the PCI device now. */
> hcd = dev_get_drvdata(&dev->dev);
> xhci = hcd_to_xhci(hcd);
> +
> + if (xhci->quirks & XHCI_DISABLE_SPARSE) {
> + u32 reg;
> +
> + reg = readl(hcd->regs + 0xC12C);
> + reg &= ~BIT(17);
Odd spacing :(
Anyway, what is magic bit 17? Shouldn't that be documented somewhere?
And you forgot to handle endian issues here, right?
> + writel(reg, hcd->regs + 0xC12C);
Same for this magic address, shouldn't you document that here please?
And is this the proper place to be testing for quirks and applying them?
Why not do the above in the xhci_pci_quirks() call?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xhci: workaround for S3 issue on AMD SNPS 3.0 xHC
[not found] ` <1163986b-075f-8d5e-13dc-7141cb25e484@amd.com>
@ 2020-09-16 8:06 ` Greg KH
0 siblings, 0 replies; 5+ messages in thread
From: Greg KH @ 2020-09-16 8:06 UTC (permalink / raw)
To: Singh, Sandeep
Cc: Nehal Bakulchandra Shah, mathias.nyman, linux-usb, linux-kernel,
Sandeep.Singh, yuanmei
On Wed, Sep 16, 2020 at 12:28:40AM +0530, Singh, Sandeep wrote:
> On 8/31/2020 12:22 PM, Nehal Bakulchandra Shah wrote:
> > From: Nehal Bakulchandra Shah <Nehal-Bakulchandra.shah@amd.com>
> >
> > On some platform of AMD, S3 fails with HCE and SRE errors.To fix this,
> > sparse controller enable bit has to be disabled.
> >
> > Signed-off-by: Nehal Bakulchandra Shah <Nehal-Bakulchandra.shah@amd.com>
> > ---
> > drivers/usb/host/xhci-pci.c | 12 ++++++++++++
> > drivers/usb/host/xhci.h | 1 +
> > 2 files changed, 13 insertions(+)
> >
> > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> > index 3feaafebfe58..865a16e6c1ed 100644
> > --- a/drivers/usb/host/xhci-pci.c
> > +++ b/drivers/usb/host/xhci-pci.c
> > @@ -160,6 +160,9 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
> > (pdev->device == 0x15e0 || pdev->device == 0x15e1))
> > xhci->quirks |= XHCI_SNPS_BROKEN_SUSPEND;
> > + if (pdev->vendor == PCI_VENDOR_ID_AMD && pdev->device == 0x15e5)
> > + xhci->quirks |= XHCI_DISABLE_SPARSE;
> > +
> > if (pdev->vendor == PCI_VENDOR_ID_AMD)
> > xhci->quirks |= XHCI_TRUST_TX_LENGTH;
> > @@ -371,6 +374,15 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
> > /* USB 2.0 roothub is stored in the PCI device now. */
> > hcd = dev_get_drvdata(&dev->dev);
> > xhci = hcd_to_xhci(hcd);
> > +
> > + if (xhci->quirks & XHCI_DISABLE_SPARSE) {
> > + u32 reg;
> > +
> > + reg = readl(hcd->regs + 0xC12C);
> > + reg &= ~BIT(17);
> > + writel(reg, hcd->regs + 0xC12C);
> > + }
> > +
> > xhci->shared_hcd = usb_create_shared_hcd(&xhci_pci_hc_driver, &dev->dev,
> > pci_name(dev), hcd);
> > if (!xhci->shared_hcd) {
> > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> > index ea1754f185a2..ea966d70f1ee 100644
> > --- a/drivers/usb/host/xhci.h
> > +++ b/drivers/usb/host/xhci.h
> > @@ -1874,6 +1874,7 @@ struct xhci_hcd {
> > #define XHCI_RESET_PLL_ON_DISCONNECT BIT_ULL(34)
> > #define XHCI_SNPS_BROKEN_SUSPEND BIT_ULL(35)
> > #define XHCI_RENESAS_FW_QUIRK BIT_ULL(36)
> > +#define XHCI_DISABLE_SPARSE BIT_ULL(37)
> > unsigned int num_active_eps;
> > unsigned int limit_active_eps;
>
> Any feedback on this patch?
Do you agree with it? If so, can you review it and say so please?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xhci: workaround for S3 issue on AMD SNPS 3.0 xHC
2020-08-31 6:52 [PATCH] xhci: workaround for S3 issue on AMD SNPS 3.0 xHC Nehal Bakulchandra Shah
2020-09-16 8:06 ` Greg KH
[not found] ` <1163986b-075f-8d5e-13dc-7141cb25e484@amd.com>
@ 2020-09-16 12:30 ` Mathias Nyman
2 siblings, 0 replies; 5+ messages in thread
From: Mathias Nyman @ 2020-09-16 12:30 UTC (permalink / raw)
To: Nehal Bakulchandra Shah, gregkh, linux-usb, linux-kernel,
Sandeep.Singh, yuanmei
On 31.8.2020 9.52, Nehal Bakulchandra Shah wrote:
> From: Nehal Bakulchandra Shah <Nehal-Bakulchandra.shah@amd.com>
>
> On some platform of AMD, S3 fails with HCE and SRE errors.To fix this,
> sparse controller enable bit has to be disabled.
>
> Signed-off-by: Nehal Bakulchandra Shah <Nehal-Bakulchandra.shah@amd.com>
> ---
> drivers/usb/host/xhci-pci.c | 12 ++++++++++++
> drivers/usb/host/xhci.h | 1 +
> 2 files changed, 13 insertions(+)
>
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 3feaafebfe58..865a16e6c1ed 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -160,6 +160,9 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
> (pdev->device == 0x15e0 || pdev->device == 0x15e1))
> xhci->quirks |= XHCI_SNPS_BROKEN_SUSPEND;
>
> + if (pdev->vendor == PCI_VENDOR_ID_AMD && pdev->device == 0x15e5)
> + xhci->quirks |= XHCI_DISABLE_SPARSE;
> +
> if (pdev->vendor == PCI_VENDOR_ID_AMD)
> xhci->quirks |= XHCI_TRUST_TX_LENGTH;
>
> @@ -371,6 +374,15 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
> /* USB 2.0 roothub is stored in the PCI device now. */
> hcd = dev_get_drvdata(&dev->dev);
> xhci = hcd_to_xhci(hcd);
> +
> + if (xhci->quirks & XHCI_DISABLE_SPARSE) {
> + u32 reg;
> +
> + reg = readl(hcd->regs + 0xC12C);
> + reg &= ~BIT(17);
> + writel(reg, hcd->regs + 0xC12C);
> + }
> +
Is disabling the bit once in probe enough?
xHC will be reset after hibernation as well, does this bit need to be cleared after that?
Also consider turning this into a separate function with a proper description,
see xhci_pme_quirk() for example. Avoids cluttering probe.
Actually if this bit only needs to be cleared once then that function could be called from xhci_pci_setup()
-Mathias
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xhci: workaround for S3 issue on AMD SNPS 3.0 xHC
2020-09-16 8:06 ` Greg KH
@ 2020-09-16 12:47 ` Mathias Nyman
0 siblings, 0 replies; 5+ messages in thread
From: Mathias Nyman @ 2020-09-16 12:47 UTC (permalink / raw)
To: Greg KH, Nehal Bakulchandra Shah
Cc: linux-usb, linux-kernel, Sandeep.Singh, yuanmei
On 16.9.2020 11.06, Greg KH wrote:
> On Mon, Aug 31, 2020 at 06:52:46AM +0000, Nehal Bakulchandra Shah wrote:
>> From: Nehal Bakulchandra Shah <Nehal-Bakulchandra.shah@amd.com>
>>
>> On some platform of AMD, S3 fails with HCE and SRE errors.To fix this,
>> sparse controller enable bit has to be disabled.
>>
>> Signed-off-by: Nehal Bakulchandra Shah <Nehal-Bakulchandra.shah@amd.com>
>> ---
>> drivers/usb/host/xhci-pci.c | 12 ++++++++++++
>> drivers/usb/host/xhci.h | 1 +
>> 2 files changed, 13 insertions(+)
>>
>> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
>> @@ -371,6 +374,15 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>> /* USB 2.0 roothub is stored in the PCI device now. */
>> hcd = dev_get_drvdata(&dev->dev);
>> xhci = hcd_to_xhci(hcd);
>> +
>> + if (xhci->quirks & XHCI_DISABLE_SPARSE) {
>> + u32 reg;
>> +
>> + reg = readl(hcd->regs + 0xC12C);
>> + reg &= ~BIT(17);
>
> And is this the proper place to be testing for quirks and applying them?
> Why not do the above in the xhci_pci_quirks() call?
xhci_pci_quirks() is a confusing name, it actually only sets quirk flags based on PCI
vendor/device.
Anyway, point is still valid, this level of bitops in probe isn't very nice.
Turn it into a separate function, and call it from xhci_pci_setup(), or if flag
needs to be cleared more often, and is related to S3 problems then possibly from xhci_pci_suspend()
-Mathias
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-09-16 20:37 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-31 6:52 [PATCH] xhci: workaround for S3 issue on AMD SNPS 3.0 xHC Nehal Bakulchandra Shah
2020-09-16 8:06 ` Greg KH
2020-09-16 12:47 ` Mathias Nyman
[not found] ` <1163986b-075f-8d5e-13dc-7141cb25e484@amd.com>
2020-09-16 8:06 ` Greg KH
2020-09-16 12:30 ` Mathias Nyman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).