diff mbox series

[net,v2] net: wwan: t7xx: add PM_AUTOSUSPEND_MS_BY_DW5933E for Dell DW5933e

Message ID 20240930031624.2116592-1-wojackbb@gmail.com (mailing list archive)
State New, archived
Headers show
Series [net,v2] net: wwan: t7xx: add PM_AUTOSUSPEND_MS_BY_DW5933E for Dell DW5933e | expand

Commit Message

吳逼逼 Sept. 30, 2024, 3:16 a.m. UTC 
From: Jack Wu <wojackbb@gmail.com>

Because optimizing the power consumption of Dell DW5933e,
Add a new auto suspend time for Dell DW5933e.

Signed-off-by: Jack Wu <wojackbb@gmail.com>
---
 drivers/net/wwan/t7xx/t7xx_pci.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

Comments

Andrew Lunn Sept. 30, 2024, 7:06 p.m. UTC | #1 
On Mon, Sep 30, 2024 at 11:16:24AM +0800, wojackbb@gmail.com wrote:
> From: Jack Wu <wojackbb@gmail.com>
> 
> Because optimizing the power consumption of Dell DW5933e,
> Add a new auto suspend time for Dell DW5933e.

Please don't send new versions of a patch within 24 hours.

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html#netdev-faq

You commit message is not very good. What makes this machine special?
What is wrong with the current code? Do you have any benchmark data to
show the improvement?

     Andrew
Andrew Lunn Oct. 15, 2024, 12:08 p.m. UTC | #2 
On Tue, Oct 15, 2024 at 10:48:15AM +0800, 吳逼逼 wrote:
> Hi Andrew,
> 
> We have a power test that uses a small script to loop through the power_state
> of Dell DW5933e.(/sys/bus/pci/devices/..../power_state)
> 
> We expect that PCIE can enter the D3 state when Host don;t have data packet
> transmission,
> but the experimental result of the small script test is that it is only in the
> D3 state about 5% of the time.
> 
> We analyze logs to found that Dell DW5933e occasionally sends signal change
> notifications, and ModemManager occasionally captures Modem status.
> Although these situations are not very frequent,
> However, since the default auto suspend time is 20 seconds, the chance of PCIE
> being able to enter the D3 state is very small.
> 
> After we changed auto suspend to 5 seconds, PCIE have 50% of the time was in D3
> state, which met Dell's requirements.

So you answered some of my questions. But missed:

What makes this machine special?

It is maybe because this machine occasionally sends signal change
notifications? There are modem status changes?

Have you compared this to other machines with the same hardware? Do
they do the same? Or is this Dell special? If it is special, why is it
special?

	Andrew
吳逼逼 Oct. 22, 2024, 3:16 a.m. UTC | #3 
Andrew Lunn <andrew@lunn.ch> 於 2024年10月15日 週二 下午8:09寫道：

>
> On Tue, Oct 15, 2024 at 10:48:15AM +0800, 吳逼逼 wrote:
> > Hi Andrew,
> >
> > We have a power test that uses a small script to loop through the power_state
> > of Dell DW5933e.(/sys/bus/pci/devices/..../power_state)
> >
> > We expect that PCIE can enter the D3 state when Host don;t have data packet
> > transmission,
> > but the experimental result of the small script test is that it is only in the
> > D3 state about 5% of the time.
> >
> > We analyze logs to found that Dell DW5933e occasionally sends signal change
> > notifications, and ModemManager occasionally captures Modem status.
> > Although these situations are not very frequent,
> > However, since the default auto suspend time is 20 seconds, the chance of PCIE
> > being able to enter the D3 state is very small.
> >
> > After we changed auto suspend to 5 seconds, PCIE have 50% of the time was in D3
> > state, which met Dell's requirements.
>
> So you answered some of my questions. But missed:
>
> What makes this machine special?
>
> It is maybe because this machine occasionally sends signal change
> notifications? There are modem status changes?
>
> Have you compared this to other machines with the same hardware? Do
> they do the same? Or is this Dell special? If it is special, why is it
> special?
>
>         Andrew

Hi Andrew,

The chip of Fibocom FM350 is MTK T7XX,
It is the same chip as our device.

We tested the Fibocom FM350 and It had the same issue as our device.
The following tests use the same environment and steps:
a. Make data call to connect Internet
b. No data is transferred to the Internet and wait one minute.
c. use test script to capture and count power_state until one minute.

Result:
1. When autosuspend_delay_ms is 20000,
Our device's d3_cold time is 0%
Fibocom FM350's d3_cold time 0%

2. When autosuspend_delay_ms is 5000,
Our device's d3_cold time is 80%
Fibocom FM350's d3_cold time 60%

So this problem is a common problem.
Should I remove PM_AUTOSUSPEND_MS_BY_DW5933E,
and modify PM_AUTOSUSPEND_MS to 5000 at my patch?

Thanks.
Andrew Lunn Oct. 22, 2024, 1:41 p.m. UTC | #4 
> Hi Andrew,
> 
> The chip of Fibocom FM350 is MTK T7XX,
> It is the same chip as our device.
> 
> We tested the Fibocom FM350 and It had the same issue as our device.
> The following tests use the same environment and steps:
> a. Make data call to connect Internet
> b. No data is transferred to the Internet and wait one minute.
> c. use test script to capture and count power_state until one minute.
> 
> Result:
> 1. When autosuspend_delay_ms is 20000,
> Our device's d3_cold time is 0%
> Fibocom FM350's d3_cold time 0%
> 
> 2. When autosuspend_delay_ms is 5000,
> Our device's d3_cold time is 80%
> Fibocom FM350's d3_cold time 60%
> 
> So this problem is a common problem.
> Should I remove PM_AUTOSUSPEND_MS_BY_DW5933E,
> and modify PM_AUTOSUSPEND_MS to 5000 at my patch?

A sample of two is not great, but does suggest there is nothing
special about the DW5933E, and all users can benefit from this change.

Please do change PM_AUTOSUSPEND_MS.

       Andrew
diff mbox series

Patch

diff --git a/drivers/net/wwan/t7xx/t7xx_pci.c b/drivers/net/wwan/t7xx/t7xx_pci.c
index e556e5bd49ab..ec567153ea6e 100644
--- a/drivers/net/wwan/t7xx/t7xx_pci.c
+++ b/drivers/net/wwan/t7xx/t7xx_pci.c
@@ -49,6 +49,7 @@ 
 #define PM_SLEEP_DIS_TIMEOUT_MS		20
 #define PM_ACK_TIMEOUT_MS		1500
 #define PM_AUTOSUSPEND_MS		20000
+#define PM_AUTOSUSPEND_MS_BY_DW5933E 5000
 #define PM_RESOURCE_POLL_TIMEOUT_US	10000
 #define PM_RESOURCE_POLL_STEP_US	100
 
@@ -174,7 +175,7 @@  static int t7xx_wait_pm_config(struct t7xx_pci_dev *t7xx_dev)
 	return ret;
 }
 
-static int t7xx_pci_pm_init(struct t7xx_pci_dev *t7xx_dev)
+static int t7xx_pci_pm_init(struct t7xx_pci_dev *t7xx_dev, int pm_autosuspend_ms)
 {
 	struct pci_dev *pdev = t7xx_dev->pdev;
 
@@ -191,7 +192,7 @@  static int t7xx_pci_pm_init(struct t7xx_pci_dev *t7xx_dev)
 				DPM_FLAG_NO_DIRECT_COMPLETE);
 
 	iowrite32(T7XX_L1_BIT(0), IREG_BASE(t7xx_dev) + DISABLE_ASPM_LOWPWR);
-	pm_runtime_set_autosuspend_delay(&pdev->dev, PM_AUTOSUSPEND_MS);
+	pm_runtime_set_autosuspend_delay(&pdev->dev, pm_autosuspend_ms);
 	pm_runtime_use_autosuspend(&pdev->dev);
 
 	return 0;
@@ -824,7 +825,13 @@  static int t7xx_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	IREG_BASE(t7xx_dev) = pcim_iomap_table(pdev)[T7XX_PCI_IREG_BASE];
 	t7xx_dev->base_addr.pcie_ext_reg_base = pcim_iomap_table(pdev)[T7XX_PCI_EREG_BASE];
 
-	ret = t7xx_pci_pm_init(t7xx_dev);
+	if (id->vendor == 0x14c0 && id->device == 0x4d75) {
+		/* Dell DW5933e */
+		ret = t7xx_pci_pm_init(t7xx_dev, PM_AUTOSUSPEND_MS_BY_DW5933E);
+	} else {
+		/* Other devices */
+		ret = t7xx_pci_pm_init(t7xx_dev, PM_AUTOSUSPEND_MS);
+	}
 	if (ret)
 		return ret;