diff mbox series

[v3] usb: xhci: add LWP quirk for ensuring uPD720201 into D3 state after S5

Message ID 1636612118-32481-1-git-send-email-zhuyinbo@loongson.cn (mailing list archive)
State New, archived
Headers show
Series [v3] usb: xhci: add LWP quirk for ensuring uPD720201 into D3 state after S5 | expand

Commit Message

Yinbo Zhu Nov. 11, 2021, 6:28 a.m. UTC
After S5, any pci device should into D3 state that if supported, but the
uPD720201 was not and cause OSPM power consumption is more higher that
S5 than S4. Due to that uPD720201 firmware behavior was unknown and the
_PS3 method wasn't implemented in ACPI table which can make device into
D3, I think xhci HCD can add a quirk ensure it into D3 state after S5
that is appropriate and this patch was to add the XHCI_LWP_QURIK and set
PCI_D3hot to uPD720201 pmsc register in xhci_pci_shutdown and
xhci_pci_remove to fix xhci power consumption issue.

Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
---
Change in v3:
		Add D3 set in xhci_pci_remove function.

 drivers/usb/host/xhci-pci.c | 9 +++++++++
 drivers/usb/host/xhci.h     | 1 +
 2 files changed, 10 insertions(+)

Comments

Yinbo Zhu Nov. 22, 2021, 12:25 p.m. UTC | #1
在 2021/11/11 下午2:28, Yinbo Zhu 写道:
> After S5, any pci device should into D3 state that if supported, but the
> uPD720201 was not and cause OSPM power consumption is more higher that
> S5 than S4. Due to that uPD720201 firmware behavior was unknown and the
> _PS3 method wasn't implemented in ACPI table which can make device into
> D3, I think xhci HCD can add a quirk ensure it into D3 state after S5
> that is appropriate and this patch was to add the XHCI_LWP_QURIK and set
> PCI_D3hot to uPD720201 pmsc register in xhci_pci_shutdown and
> xhci_pci_remove to fix xhci power consumption issue.
>
> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
> ---
> Change in v3:
> 		Add D3 set in xhci_pci_remove function.
>
>   drivers/usb/host/xhci-pci.c | 9 +++++++++
>   drivers/usb/host/xhci.h     | 1 +
>   2 files changed, 10 insertions(+)
>
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 2c9f25c..6258a5a 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -265,6 +265,7 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
>   	    pdev->device == 0x0014) {
>   		xhci->quirks |= XHCI_TRUST_TX_LENGTH;
>   		xhci->quirks |= XHCI_ZERO_64B_REGS;
> +		xhci->quirks |= XHCI_LWP_QUIRK;
>   	}
>   	if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
>   	    pdev->device == 0x0015) {
> @@ -466,6 +467,10 @@ static void xhci_pci_remove(struct pci_dev *dev)
>   		pci_set_power_state(dev, PCI_D3hot);
>   
>   	usb_hcd_pci_remove(dev);
> +
> +	/* Workaround for decreasing power consumption after S5 */
> +	if (xhci->quirks & XHCI_LWP_QUIRK)
> +		pci_set_power_state(dev, PCI_D3hot);
>   }
>   
>   #ifdef CONFIG_PM
> @@ -610,6 +615,10 @@ static void xhci_pci_shutdown(struct usb_hcd *hcd)
>   	/* Yet another workaround for spurious wakeups at shutdown with HSW */
>   	if (xhci->quirks & XHCI_SPURIOUS_WAKEUP)
>   		pci_set_power_state(pdev, PCI_D3hot);
> +
> +	/* Workaround for decreasing power consumption after S5 */
> +	if (xhci->quirks & XHCI_LWP_QUIRK)
> +		pci_set_power_state(pdev, PCI_D3hot);
>   }
>   #endif /* CONFIG_PM */
>   
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index dca6181..bcd70d1 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1899,6 +1899,7 @@ struct xhci_hcd {
>   #define XHCI_SG_TRB_CACHE_SIZE_QUIRK	BIT_ULL(39)
>   #define XHCI_NO_SOFT_RETRY	BIT_ULL(40)
>   #define XHCI_BROKEN_D3COLD	BIT_ULL(41)
> +#define XHCI_LWP_QUIRK		BIT_ULL(42)
>   
>   	unsigned int		num_active_eps;
>   	unsigned int		limit_active_eps;

Hi all,


Do you have any advice about my patch, if no any question, please you 
help me merge this patch to upstream.


Thanks,

BRs,

Yinbo Zhu.
Mathias Nyman Nov. 29, 2021, 2:35 p.m. UTC | #2
On 22.11.2021 14.25, zhuyinbo wrote:
> 
> 在 2021/11/11 下午2:28, Yinbo Zhu 写道:
>> After S5, any pci device should into D3 state that if supported, but the
>> uPD720201 was not and cause OSPM power consumption is more higher that
>> S5 than S4. Due to that uPD720201 firmware behavior was unknown and the
>> _PS3 method wasn't implemented in ACPI table which can make device into
>> D3, I think xhci HCD can add a quirk ensure it into D3 state after S5
>> that is appropriate and this patch was to add the XHCI_LWP_QURIK and set
>> PCI_D3hot to uPD720201 pmsc register in xhci_pci_shutdown and
>> xhci_pci_remove to fix xhci power consumption issue.
>>
>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>> ---
>> Change in v3:
>>         Add D3 set in xhci_pci_remove function.
>>
>>   drivers/usb/host/xhci-pci.c | 9 +++++++++
>>   drivers/usb/host/xhci.h     | 1 +
>>   2 files changed, 10 insertions(+)
>>
>> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
>> index 2c9f25c..6258a5a 100644
>> --- a/drivers/usb/host/xhci-pci.c
>> +++ b/drivers/usb/host/xhci-pci.c
>> @@ -265,6 +265,7 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
>>           pdev->device == 0x0014) {
>>           xhci->quirks |= XHCI_TRUST_TX_LENGTH;
>>           xhci->quirks |= XHCI_ZERO_64B_REGS;
>> +        xhci->quirks |= XHCI_LWP_QUIRK;
>>       }
>>       if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
>>           pdev->device == 0x0015) {
>> @@ -466,6 +467,10 @@ static void xhci_pci_remove(struct pci_dev *dev)
>>           pci_set_power_state(dev, PCI_D3hot);
>>         usb_hcd_pci_remove(dev);
>> +
>> +    /* Workaround for decreasing power consumption after S5 */
>> +    if (xhci->quirks & XHCI_LWP_QUIRK)
>> +        pci_set_power_state(dev, PCI_D3hot);
>>   }
>>     #ifdef CONFIG_PM
>> @@ -610,6 +615,10 @@ static void xhci_pci_shutdown(struct usb_hcd *hcd)
>>       /* Yet another workaround for spurious wakeups at shutdown with HSW */
>>       if (xhci->quirks & XHCI_SPURIOUS_WAKEUP)
>>           pci_set_power_state(pdev, PCI_D3hot);
>> +
>> +    /* Workaround for decreasing power consumption after S5 */
>> +    if (xhci->quirks & XHCI_LWP_QUIRK)
>> +        pci_set_power_state(pdev, PCI_D3hot);
>>   }
>>   #endif /* CONFIG_PM */
>>   diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>> index dca6181..bcd70d1 100644
>> --- a/drivers/usb/host/xhci.h
>> +++ b/drivers/usb/host/xhci.h
>> @@ -1899,6 +1899,7 @@ struct xhci_hcd {
>>   #define XHCI_SG_TRB_CACHE_SIZE_QUIRK    BIT_ULL(39)
>>   #define XHCI_NO_SOFT_RETRY    BIT_ULL(40)
>>   #define XHCI_BROKEN_D3COLD    BIT_ULL(41)
>> +#define XHCI_LWP_QUIRK        BIT_ULL(42)
>>         unsigned int        num_active_eps;
>>       unsigned int        limit_active_eps;
> 
> Hi all,
> 
> 
> Do you have any advice about my patch, if no any question, please you help me merge this patch to upstream.
> 

This will set any Renesas uPD720201 PCI controller to D3 at remove or shutdown.
Should this be limited to only those platforms with missing ACPI methods?

Would be better if we could avoid setting PCI D states directly in xhci driver.

Do you know the details why pci_disable_device() called by usb_hcd_pci_shutdown()
is not setting this right D state in this case? 
Is it just the missing _PS3 ACPI method described in the patch, or some other reason?

Is windows working? or is that not relevant in this case?
If not working, and product not yet on the market then fixing the ACPI firmware
would be solve both cases.

Thanks
-Mathias
diff mbox series

Patch

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 2c9f25c..6258a5a 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -265,6 +265,7 @@  static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
 	    pdev->device == 0x0014) {
 		xhci->quirks |= XHCI_TRUST_TX_LENGTH;
 		xhci->quirks |= XHCI_ZERO_64B_REGS;
+		xhci->quirks |= XHCI_LWP_QUIRK;
 	}
 	if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
 	    pdev->device == 0x0015) {
@@ -466,6 +467,10 @@  static void xhci_pci_remove(struct pci_dev *dev)
 		pci_set_power_state(dev, PCI_D3hot);
 
 	usb_hcd_pci_remove(dev);
+
+	/* Workaround for decreasing power consumption after S5 */
+	if (xhci->quirks & XHCI_LWP_QUIRK)
+		pci_set_power_state(dev, PCI_D3hot);
 }
 
 #ifdef CONFIG_PM
@@ -610,6 +615,10 @@  static void xhci_pci_shutdown(struct usb_hcd *hcd)
 	/* Yet another workaround for spurious wakeups at shutdown with HSW */
 	if (xhci->quirks & XHCI_SPURIOUS_WAKEUP)
 		pci_set_power_state(pdev, PCI_D3hot);
+
+	/* Workaround for decreasing power consumption after S5 */
+	if (xhci->quirks & XHCI_LWP_QUIRK)
+		pci_set_power_state(pdev, PCI_D3hot);
 }
 #endif /* CONFIG_PM */
 
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index dca6181..bcd70d1 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1899,6 +1899,7 @@  struct xhci_hcd {
 #define XHCI_SG_TRB_CACHE_SIZE_QUIRK	BIT_ULL(39)
 #define XHCI_NO_SOFT_RETRY	BIT_ULL(40)
 #define XHCI_BROKEN_D3COLD	BIT_ULL(41)
+#define XHCI_LWP_QUIRK		BIT_ULL(42)
 
 	unsigned int		num_active_eps;
 	unsigned int		limit_active_eps;