[PATCH] usb: pci-quirks: add XHCI_COMP_MODE_QUIRK for disabling amd xhci D3cold

[PATCH] usb: pci-quirks: add XHCI_COMP_MODE_QUIRK for disabling amd xhci D3cold

[Prike Liang]

During xhci suspend some AMD USB host will lose port status change events and
need to have the registers polled during D3, so now just avoid D3cold.

Signed-off-by: Prike Liang <Prike.Liang@amd.com>
---
 drivers/usb/host/xhci-pci.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 3feaafe..bff817a 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -170,6 +170,11 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
 		(pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_1)))
 		xhci->quirks |= XHCI_U2_DISABLE_WAKE;
 
+	if (pdev->vendor == PCI_VENDOR_ID_AMD &&
+		pdev->device == 0x1639) {
+		xhci->quirks |= XHCI_COMP_MODE_QUIRK;
+	}
+
 	if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
 		xhci->quirks |= XHCI_LPM_SUPPORT;
 		xhci->quirks |= XHCI_INTEL_HOST;
-- 
2.7.4

RE: [PATCH] usb: pci-quirks: add XHCI_COMP_MODE_QUIRK for disabling amd xhci D3cold

[Liang, Prike]

[AMD Public Use]

Ping ... please help review.

Thanks,
Prike
> -----Original Message-----
> From: Liang, Prike <Prike.Liang@amd.com>
> Sent: Tuesday, January 19, 2021 9:49 AM
> To: linux-usb@vger.kernel.org
> Cc: mathias.nyman@intel.com; Liang, Prike <Prike.Liang@amd.com>
> Subject: [PATCH] usb: pci-quirks: add XHCI_COMP_MODE_QUIRK for
> disabling amd xhci D3cold
>
> During xhci suspend some AMD USB host will lose port status change events
> and need to have the registers polled during D3, so now just avoid D3cold.
>
> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> ---
>  drivers/usb/host/xhci-pci.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c index
> 3feaafe..bff817a 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -170,6 +170,11 @@ static void xhci_pci_quirks(struct device *dev, struct
> xhci_hcd *xhci)
>  (pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_1)))
>  xhci->quirks |= XHCI_U2_DISABLE_WAKE;
>
> +if (pdev->vendor == PCI_VENDOR_ID_AMD &&
> +pdev->device == 0x1639) {
> +xhci->quirks |= XHCI_COMP_MODE_QUIRK;
> +}
> +
>  if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
>  xhci->quirks |= XHCI_LPM_SUPPORT;
>  xhci->quirks |= XHCI_INTEL_HOST;
> --
> 2.7.4

Re: [PATCH] usb: pci-quirks: add XHCI_COMP_MODE_QUIRK for disabling amd xhci D3cold

[Greg KH]

On Tue, Jan 19, 2021 at 09:48:44AM +0800, Prike Liang wrote:
> During xhci suspend some AMD USB host will lose port status change events and
> need to have the registers polled during D3, so now just avoid D3cold.
> 
> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> ---
>  drivers/usb/host/xhci-pci.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 3feaafe..bff817a 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -170,6 +170,11 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
>  		(pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_1)))
>  		xhci->quirks |= XHCI_U2_DISABLE_WAKE;
>  
> +	if (pdev->vendor == PCI_VENDOR_ID_AMD &&
> +		pdev->device == 0x1639) {
> +		xhci->quirks |= XHCI_COMP_MODE_QUIRK;
> +	}

Why not add this check to the
xhci_compliance_mode_recovery_timer_quirk_check() function instead,
where all other systems that need this quirk are tested for?

thanks,

greg k-h

Re: [PATCH] usb: pci-quirks: add XHCI_COMP_MODE_QUIRK for disabling amd xhci D3cold

[Greg KH]

On Wed, Jan 27, 2021 at 12:33:09PM +0100, Greg KH wrote:
> On Tue, Jan 19, 2021 at 09:48:44AM +0800, Prike Liang wrote:
> > During xhci suspend some AMD USB host will lose port status change events and
> > need to have the registers polled during D3, so now just avoid D3cold.
> > 
> > Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> > ---
> >  drivers/usb/host/xhci-pci.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> > index 3feaafe..bff817a 100644
> > --- a/drivers/usb/host/xhci-pci.c
> > +++ b/drivers/usb/host/xhci-pci.c
> > @@ -170,6 +170,11 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
> >  		(pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_1)))
> >  		xhci->quirks |= XHCI_U2_DISABLE_WAKE;
> >  
> > +	if (pdev->vendor == PCI_VENDOR_ID_AMD &&
> > +		pdev->device == 0x1639) {
> > +		xhci->quirks |= XHCI_COMP_MODE_QUIRK;
> > +	}
> 
> Why not add this check to the
> xhci_compliance_mode_recovery_timer_quirk_check() function instead,
> where all other systems that need this quirk are tested for?

Ah, you don't have a pci device at that point in time.  But, why aren't
you making the same calls that the caller of that function does when
setting this quirk here?  Isn't that also required?

thanks,

greg k-h

RE: [PATCH] usb: pci-quirks: add XHCI_COMP_MODE_QUIRK for disabling amd xhci D3cold

[Liang, Prike]

[AMD Official Use Only - Internal Distribution Only]

>
> On Wed, Jan 27, 2021 at 12:33:09PM +0100, Greg KH wrote:
> > On Tue, Jan 19, 2021 at 09:48:44AM +0800, Prike Liang wrote:
> > > During xhci suspend some AMD USB host will lose port status change
> > > events and need to have the registers polled during D3, so now just avoid
> D3cold.
> > >
> > > Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> > > ---
> > >  drivers/usb/host/xhci-pci.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/usb/host/xhci-pci.c
> > > b/drivers/usb/host/xhci-pci.c index 3feaafe..bff817a 100644
> > > --- a/drivers/usb/host/xhci-pci.c
> > > +++ b/drivers/usb/host/xhci-pci.c
> > > @@ -170,6 +170,11 @@ static void xhci_pci_quirks(struct device *dev,
> struct xhci_hcd *xhci)
> > >  (pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_1)))
> > >  xhci->quirks |= XHCI_U2_DISABLE_WAKE;
> > >
> > > +if (pdev->vendor == PCI_VENDOR_ID_AMD &&
> > > +pdev->device == 0x1639) {
> > > +xhci->quirks |= XHCI_COMP_MODE_QUIRK;
> > > +}
> >
> > Why not add this check to the
> > xhci_compliance_mode_recovery_timer_quirk_check() function instead,
> > where all other systems that need this quirk are tested for?
>
> Ah, you don't have a pci device at that point in time.  But, why aren't you
> making the same calls that the caller of that function does when setting this
> quirk here?  Isn't that also required?
>
[Prike]  Thanks comment. This XHCI host port status lost issue was seen on the s2idle wake up period and poll the CNR bit failed in xhci_resume() that eventually result in the XHCI device resume failed. However, this issue may not totally caused by the entry of XHCI compliance mode and seems only partially enable the COMP flag with disable XHCI D3cold can work around this failed case. Regards to this issue maybe we needn't the COMP quirk and only need disable the D3cold for some specific XHCI device during processing xhci_pci_suspend().

> thanks,
>
> greg k-h

Re: [PATCH] usb: pci-quirks: add XHCI_COMP_MODE_QUIRK for disabling amd xhci D3cold

[Greg KH]

On Mon, Feb 01, 2021 at 02:08:08PM +0000, Liang, Prike wrote:
> [AMD Official Use Only - Internal Distribution Only]

Not very internal :)

> > On Wed, Jan 27, 2021 at 12:33:09PM +0100, Greg KH wrote:
> > > On Tue, Jan 19, 2021 at 09:48:44AM +0800, Prike Liang wrote:
> > > > During xhci suspend some AMD USB host will lose port status change
> > > > events and need to have the registers polled during D3, so now just avoid
> > D3cold.
> > > >
> > > > Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> > > > ---
> > > >  drivers/usb/host/xhci-pci.c | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/drivers/usb/host/xhci-pci.c
> > > > b/drivers/usb/host/xhci-pci.c index 3feaafe..bff817a 100644
> > > > --- a/drivers/usb/host/xhci-pci.c
> > > > +++ b/drivers/usb/host/xhci-pci.c
> > > > @@ -170,6 +170,11 @@ static void xhci_pci_quirks(struct device *dev,
> > struct xhci_hcd *xhci)
> > > >  (pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_1)))
> > > >  xhci->quirks |= XHCI_U2_DISABLE_WAKE;
> > > >
> > > > +if (pdev->vendor == PCI_VENDOR_ID_AMD &&
> > > > +pdev->device == 0x1639) {
> > > > +xhci->quirks |= XHCI_COMP_MODE_QUIRK;
> > > > +}
> > >
> > > Why not add this check to the
> > > xhci_compliance_mode_recovery_timer_quirk_check() function instead,
> > > where all other systems that need this quirk are tested for?
> >
> > Ah, you don't have a pci device at that point in time.  But, why aren't you
> > making the same calls that the caller of that function does when setting this
> > quirk here?  Isn't that also required?
> >
> [Prike]  Thanks comment. This XHCI host port status lost issue was seen on the s2idle wake up period and poll the CNR bit failed in xhci_resume() that eventually result in the XHCI device resume failed. However, this issue may not totally caused by the entry of XHCI compliance mode and seems only partially enable the COMP flag with disable XHCI D3cold can work around this failed case. Regards to this issue maybe we needn't the COMP quirk and only need disable the D3cold for some specific XHCI device during processing xhci_pci_suspend().

Ok, so what does this mean for us exactly?

Can you please properly test the hardware and let us know what needs to
be done?

thanks,

greg k-h

Re: [PATCH] usb: pci-quirks: add XHCI_COMP_MODE_QUIRK for disabling amd xhci D3cold

[Mathias Nyman]

On 1.2.2021 16.08, Liang, Prike wrote:
> [AMD Official Use Only - Internal Distribution Only]
> 
>>
>> On Wed, Jan 27, 2021 at 12:33:09PM +0100, Greg KH wrote:
>>> On Tue, Jan 19, 2021 at 09:48:44AM +0800, Prike Liang wrote:
>>>> During xhci suspend some AMD USB host will lose port status change
>>>> events and need to have the registers polled during D3, so now just avoid
>> D3cold.
>>>>
>>>> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
>>>> ---
>>>>  drivers/usb/host/xhci-pci.c | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/host/xhci-pci.c
>>>> b/drivers/usb/host/xhci-pci.c index 3feaafe..bff817a 100644
>>>> --- a/drivers/usb/host/xhci-pci.c
>>>> +++ b/drivers/usb/host/xhci-pci.c
>>>> @@ -170,6 +170,11 @@ static void xhci_pci_quirks(struct device *dev,
>> struct xhci_hcd *xhci)
>>>>  (pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_1)))
>>>>  xhci->quirks |= XHCI_U2_DISABLE_WAKE;
>>>>
>>>> +if (pdev->vendor == PCI_VENDOR_ID_AMD &&
>>>> +pdev->device == 0x1639) {
>>>> +xhci->quirks |= XHCI_COMP_MODE_QUIRK;
>>>> +}
>>>
>>> Why not add this check to the
>>> xhci_compliance_mode_recovery_timer_quirk_check() function instead,
>>> where all other systems that need this quirk are tested for?
>>
>> Ah, you don't have a pci device at that point in time.  But, why aren't you
>> making the same calls that the caller of that function does when setting this
>> quirk here?  Isn't that also required?
>>
> [Prike]  Thanks comment. This XHCI host port status lost issue was seen on the s2idle wake up period and poll the CNR bit failed in xhci_resume() that eventually result in the XHCI device resume failed. However, this issue may not totally caused by the entry of XHCI compliance mode and seems only partially enable the COMP flag with disable XHCI D3cold can work around this failed case. Regards to this issue maybe we needn't the COMP quirk and only need disable the D3cold for some specific XHCI device during processing xhci_pci_suspend().

The XHCI_COMP_MODE_QUIRK is for cases where ports suddenly go to compliance mode.
For those we start a timer that polls each port every 2 seconds and checks for 
compliance mode, and recovers from it. Timer is not running while xhci is suspended.
Can you see any port ending up in compliance mode?

The 'Controller Not Ready' (CNR) bit not being cleared by host in resume 
sounds like the actual problem.

-Mathias

RE: [PATCH] usb: pci-quirks: add XHCI_COMP_MODE_QUIRK for disabling amd xhci D3cold

[Liang, Prike]

[AMD Public Use]

>
> On 1.2.2021 16.08, Liang, Prike wrote:
> > [AMD Official Use Only - Internal Distribution Only]
> >
> >>
> >> On Wed, Jan 27, 2021 at 12:33:09PM +0100, Greg KH wrote:
> >>> On Tue, Jan 19, 2021 at 09:48:44AM +0800, Prike Liang wrote:
> >>>> During xhci suspend some AMD USB host will lose port status change
> >>>> events and need to have the registers polled during D3, so now just
> >>>> avoid
> >> D3cold.
> >>>>
> >>>> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> >>>> ---
> >>>>  drivers/usb/host/xhci-pci.c | 5 +++++
> >>>>  1 file changed, 5 insertions(+)
> >>>>
> >>>> diff --git a/drivers/usb/host/xhci-pci.c
> >>>> b/drivers/usb/host/xhci-pci.c index 3feaafe..bff817a 100644
> >>>> --- a/drivers/usb/host/xhci-pci.c
> >>>> +++ b/drivers/usb/host/xhci-pci.c
> >>>> @@ -170,6 +170,11 @@ static void xhci_pci_quirks(struct device
> >>>> *dev,
> >> struct xhci_hcd *xhci)
> >>>>  (pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_1)))
> >>>>  xhci->quirks |= XHCI_U2_DISABLE_WAKE;
> >>>>
> >>>> +if (pdev->vendor == PCI_VENDOR_ID_AMD &&
> >>>> +pdev->device == 0x1639) {
> >>>> +xhci->quirks |= XHCI_COMP_MODE_QUIRK;
> >>>> +}
> >>>
> >>> Why not add this check to the
> >>> xhci_compliance_mode_recovery_timer_quirk_check() function instead,
> >>> where all other systems that need this quirk are tested for?
> >>
> >> Ah, you don't have a pci device at that point in time.  But, why
> >> aren't you making the same calls that the caller of that function
> >> does when setting this quirk here?  Isn't that also required?
> >>
> > [Prike]  Thanks comment. This XHCI host port status lost issue was seen on
> the s2idle wake up period and poll the CNR bit failed in xhci_resume() that
> eventually result in the XHCI device resume failed. However, this issue may
> not totally caused by the entry of XHCI compliance mode and seems only
> partially enable the COMP flag with disable XHCI D3cold can work around this
> failed case. Regards to this issue maybe we needn't the COMP quirk and only
> need disable the D3cold for some specific XHCI device during processing
> xhci_pci_suspend().
>
> The XHCI_COMP_MODE_QUIRK is for cases where ports suddenly go to
> compliance mode.
> For those we start a timer that polls each port every 2 seconds and checks for
> compliance mode, and recovers from it. Timer is not running while xhci is
> suspended.
> Can you see any port ending up in compliance mode?
>
> The 'Controller Not Ready' (CNR) bit not being cleared by host in resume
> sounds like the actual problem.
>
> -Mathias
[Prike] Thanks help clarify the compliance quirk. Double confirm that the XHCI device not enter the compliance mode and the PLS field value was 5. So this issue seems not caused by the Compliance Mode entering, but by D3cold entering while in s2idle loop.  Meanwhile, PMFW/XHCI HW guys confirm that in the s0ix(s2idle) AMD solution requires the XHCI USB enter D3hot. Therefore, as to this issue how about add a new quirk flag for handling this?

Thanks,
Prike