Linux-Wireless Archive on lore.kernel.org
 help / Atom feed
* [PATCH] ath10k: support PCIe enter L1 state
@ 2018-11-14  2:50 Wen Gong
  2018-11-15  0:28 ` Brian Norris
  0 siblings, 1 reply; 8+ messages in thread
From: Wen Gong @ 2018-11-14  2:50 UTC (permalink / raw)
  To: ath10k; +Cc: linux-wireless

QCA6174A/QCA9377 PCIe chips support PCIe L1 and L1SS, and indicate the
L1/L1SS capabilities in PCI configuration space. Currently ath10k driver
write target PCIe config flags to disallow HW enter into L1, this leads
some HW modules are still powered up even when both system PCIe RC and
QCA6174A/QCA9377 endpoint decides to enter into L1 or L1SS.

This cause ~12 mA power drain of bottom power consumption for all scenarios.
Fix this issue by removing the drive code to write PCIe config flags.

Tested with QCA6174 PCI with firmware
WLAN.RM.4.4.1-00109-QCARMSWPZ-1, but this will also affect QCA9377 PCI.
It's not a regression with new firmware releases.

Signed-off-by: Wen Gong <wgong@codeaurora.org>
---
 drivers/net/wireless/ath/ath10k/pci.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index af2cf55..549da7a 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -2301,7 +2301,6 @@ int ath10k_pci_init_config(struct ath10k *ar)
 	u32 pcie_state_targ_addr = 0;
 	u32 pipe_cfg_targ_addr = 0;
 	u32 svc_to_pipe_map = 0;
-	u32 pcie_config_flags = 0;
 	u32 ealloc_value;
 	u32 ealloc_targ_addr;
 	u32 flag2_value;
@@ -2374,26 +2373,6 @@ int ath10k_pci_init_config(struct ath10k *ar)
 		return ret;
 	}
 
-	ret = ath10k_pci_diag_read32(ar, (pcie_state_targ_addr +
-					  offsetof(struct pcie_state,
-						   config_flags)),
-				     &pcie_config_flags);
-	if (ret != 0) {
-		ath10k_err(ar, "Failed to get pcie config_flags: %d\n", ret);
-		return ret;
-	}
-
-	pcie_config_flags &= ~PCIE_CONFIG_FLAG_ENABLE_L1;
-
-	ret = ath10k_pci_diag_write32(ar, (pcie_state_targ_addr +
-					   offsetof(struct pcie_state,
-						    config_flags)),
-				      pcie_config_flags);
-	if (ret != 0) {
-		ath10k_err(ar, "Failed to write pcie config_flags: %d\n", ret);
-		return ret;
-	}
-
 	/* configure early allocation */
 	ealloc_targ_addr = host_interest_item_address(HI_ITEM(hi_early_alloc));
 
-- 
1.9.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ath10k: support PCIe enter L1 state
  2018-11-14  2:50 [PATCH] ath10k: support PCIe enter L1 state Wen Gong
@ 2018-11-15  0:28 ` Brian Norris
  2018-11-15  6:38   ` Wen Gong
  0 siblings, 1 reply; 8+ messages in thread
From: Brian Norris @ 2018-11-15  0:28 UTC (permalink / raw)
  To: Wen Gong; +Cc: ath10k, linux-wireless

Hi Wen,

On Wed, Nov 14, 2018 at 10:50:48AM +0800, Wen Gong wrote:
> QCA6174A/QCA9377 PCIe chips support PCIe L1 and L1SS, and indicate the
> L1/L1SS capabilities in PCI configuration space. Currently ath10k driver
> write target PCIe config flags to disallow HW enter into L1, this leads
> some HW modules are still powered up even when both system PCIe RC and
> QCA6174A/QCA9377 endpoint decides to enter into L1 or L1SS.
> 
> This cause ~12 mA power drain of bottom power consumption for all scenarios.
> Fix this issue by removing the drive code to write PCIe config flags.
> 
> Tested with QCA6174 PCI with firmware
> WLAN.RM.4.4.1-00109-QCARMSWPZ-1, but this will also affect QCA9377 PCI.
> It's not a regression with new firmware releases.
> 
> Signed-off-by: Wen Gong <wgong@codeaurora.org>

Is there some reason L1 was disabled in the first place? Was it known to
be unreliable?

Brian

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE:  [PATCH] ath10k: support PCIe enter L1 state
  2018-11-15  0:28 ` Brian Norris
@ 2018-11-15  6:38   ` Wen Gong
  2018-11-15 18:43     ` Brian Norris
  0 siblings, 1 reply; 8+ messages in thread
From: Wen Gong @ 2018-11-15  6:38 UTC (permalink / raw)
  To: Brian Norris, Wen Gong; +Cc: linux-wireless, ath10k

> -----Original Message-----
> From: ath10k <ath10k-bounces@lists.infradead.org> On Behalf Of Brian
> Norris
> Sent: Thursday, November 15, 2018 8:29 AM
> To: Wen Gong <wgong@codeaurora.org>
> Cc: linux-wireless@vger.kernel.org; ath10k@lists.infradead.org
> Subject: [EXTERNAL] Re: [PATCH] ath10k: support PCIe enter L1 state
> 
> Hi Wen,
> 
> >
> > Signed-off-by: Wen Gong <wgong@codeaurora.org>
> 
> Is there some reason L1 was disabled in the first place? Wa
s it known to be
> unreliable?
> 
Hi Brian,
It is a BUG for power, and it is not considered this BUG before.
So this change will fix the bug.

> Brian
> 
> _______________________________________________
> ath10k mailing list
> ath10k@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/ath10k

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ath10k: support PCIe enter L1 state
  2018-11-15  6:38   ` Wen Gong
@ 2018-11-15 18:43     ` Brian Norris
  2018-11-16  7:00       ` Kalle Valo
  0 siblings, 1 reply; 8+ messages in thread
From: Brian Norris @ 2018-11-15 18:43 UTC (permalink / raw)
  To: Wen Gong; +Cc: Wen Gong, linux-wireless, ath10k

Hi,

On Thu, Nov 15, 2018 at 06:38:25AM +0000, Wen Gong wrote:
> > -----Original Message-----
> > From: ath10k <ath10k-bounces@lists.infradead.org> On Behalf Of Brian Norris
> > 
> > Is there some reason L1 was disabled in the first place? Was it known to be
> > unreliable?
>
> Hi Brian,
> It is a BUG for power, and it is not considered this BUG before.
> So this change will fix the bug.

I understand that the existing behavior is suboptimal for power, but on
the other hand, code that goes out of its way to *clear* the L1 flag
doesn't just pop up out of nowhere. Somebody clearly wrote that! If it
just meant "we didn't verify L1 at first", then maybe it's fine to
enable it unconditionally and see what happens, but if it meant "we
tried L1 on some old chip XXXX and it caused problems", then it would be
nice to know what those problems were.

Or maybe that is hard to figure out, given there's no public git history
tracking the original code, and we just need to try it out.

Anyway, I'm giving it a try here, but I just wanted to ask :)

Thanks,
Brian

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ath10k: support PCIe enter L1 state
  2018-11-15 18:43     ` Brian Norris
@ 2018-11-16  7:00       ` Kalle Valo
  2018-11-16  7:56         ` Michał Kazior
  0 siblings, 1 reply; 8+ messages in thread
From: Kalle Valo @ 2018-11-16  7:00 UTC (permalink / raw)
  To: Brian Norris; +Cc: Wen Gong, linux-wireless\, ath10k\, Wen Gong

Brian Norris <briannorris@chromium.org> writes:
> On Thu, Nov 15, 2018 at 06:38:25AM +0000, Wen Gong wrote:
>> > 
>> > Is there some reason L1 was disabled in the first place? Was it known to be
>> > unreliable?
>>
>> Hi Brian,
>> It is a BUG for power, and it is not considered this BUG before.
>> So this change will fix the bug.
>
> I understand that the existing behavior is suboptimal for power, but on
> the other hand, code that goes out of its way to *clear* the L1 flag
> doesn't just pop up out of nowhere. Somebody clearly wrote that! If it
> just meant "we didn't verify L1 at first", then maybe it's fine to
> enable it unconditionally and see what happens, but if it meant "we
> tried L1 on some old chip XXXX and it caused problems", then it would be
> nice to know what those problems were.
>
> Or maybe that is hard to figure out, given there's no public git history
> tracking the original code, and we just need to try it out.

Yeah, it seems L1 was disabled already on the first ath10k commit:

5e3dd157d7e70 (Kalle Valo                2013-06-12 20:52:10 +0300 2404)        pcie_config_flags &= ~PCIE_CONFIG_FLAG_ENABLE_L1;

I don't remember anymore why but my guess is that the proprietary driver
at the time didn't support it with QCA998X. Or maybe QCA988X doesn't
even support L1? Michal, do you remember?

Related to QCA988X supporting L1 state also the commit log is misleading
as it only talks QCA6174/QCA9377, and has been only tested on QCA6174,
but the actual change enables L1 on _all_ PCI and AHB devices. So this
patch needs a lot more testing so that we have confidence that no
existing setups break.

-- 
Kalle Valo

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ath10k: support PCIe enter L1 state
  2018-11-16  7:00       ` Kalle Valo
@ 2018-11-16  7:56         ` Michał Kazior
  2019-02-08 13:42           ` Kalle Valo
  0 siblings, 1 reply; 8+ messages in thread
From: Michał Kazior @ 2018-11-16  7:56 UTC (permalink / raw)
  To: kvalo; +Cc: briannorris, linux-wireless, wgong, ath10k, Wen Gong

On Fri, 16 Nov 2018 at 08:00, Kalle Valo <kvalo@codeaurora.org> wrote:
>
> Brian Norris <briannorris@chromium.org> writes:
> > On Thu, Nov 15, 2018 at 06:38:25AM +0000, Wen Gong wrote:
> >> >
> >> > Is there some reason L1 was disabled in the first place? Was it known to be
> >> > unreliable?
> >>
> >> Hi Brian,
> >> It is a BUG for power, and it is not considered this BUG before.
> >> So this change will fix the bug.
> >
> > I understand that the existing behavior is suboptimal for power, but on
> > the other hand, code that goes out of its way to *clear* the L1 flag
> > doesn't just pop up out of nowhere. Somebody clearly wrote that! If it
> > just meant "we didn't verify L1 at first", then maybe it's fine to
> > enable it unconditionally and see what happens, but if it meant "we
> > tried L1 on some old chip XXXX and it caused problems", then it would be
> > nice to know what those problems were.
> >
> > Or maybe that is hard to figure out, given there's no public git history
> > tracking the original code, and we just need to try it out.
>
> Yeah, it seems L1 was disabled already on the first ath10k commit:
>
> 5e3dd157d7e70 (Kalle Valo                2013-06-12 20:52:10 +0300 2404)        pcie_config_flags &= ~PCIE_CONFIG_FLAG_ENABLE_L1;
>
> I don't remember anymore why but my guess is that the proprietary driver
> at the time didn't support it with QCA998X. Or maybe QCA988X doesn't
> even support L1? Michal, do you remember?

Proprietary driver has it ifdef-ed to enable/disable.

Older driver enabled it only for some station-only target/product so
by default QCA988X would build with L1 flag masked out. It made sense
to be conservative and change as little as possible to avoid random
bugs and breakage - so the logic was inherited minus the build-time
ifdef.

However 10.4 driver seems to enable it unconditionally. I'm not sure
if this depends on target firmware in any way or if some other host
driver or bus settings need to be pre-set before L1 can be expected to
work reliably.

I guess there's no way other than testing it out.


Michał

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ath10k: support PCIe enter L1 state
  2018-11-16  7:56         ` Michał Kazior
@ 2019-02-08 13:42           ` Kalle Valo
  2019-02-08 17:05             ` Brian Norris
  0 siblings, 1 reply; 8+ messages in thread
From: Kalle Valo @ 2019-02-08 13:42 UTC (permalink / raw)
  To: Michał Kazior; +Cc: wgong, briannorris, linux-wireless, ath10k, Wen Gong

Michał Kazior <kazikcz@gmail.com> writes:

> On Fri, 16 Nov 2018 at 08:00, Kalle Valo <kvalo@codeaurora.org> wrote:
>>
>> Brian Norris <briannorris@chromium.org> writes:
>> > On Thu, Nov 15, 2018 at 06:38:25AM +0000, Wen Gong wrote:
>> >> >
>> >> > Is there some reason L1 was disabled in the first place? Was it known to be
>> >> > unreliable?
>> >>
>> >> Hi Brian,
>> >> It is a BUG for power, and it is not considered this BUG before.
>> >> So this change will fix the bug.
>> >
>> > I understand that the existing behavior is suboptimal for power, but on
>> > the other hand, code that goes out of its way to *clear* the L1 flag
>> > doesn't just pop up out of nowhere. Somebody clearly wrote that! If it
>> > just meant "we didn't verify L1 at first", then maybe it's fine to
>> > enable it unconditionally and see what happens, but if it meant "we
>> > tried L1 on some old chip XXXX and it caused problems", then it would be
>> > nice to know what those problems were.
>> >
>> > Or maybe that is hard to figure out, given there's no public git history
>> > tracking the original code, and we just need to try it out.
>>
>> Yeah, it seems L1 was disabled already on the first ath10k commit:
>>
>> 5e3dd157d7e70 (Kalle Valo 2013-06-12 20:52:10 +0300 2404)
>> pcie_config_flags &= ~PCIE_CONFIG_FLAG_ENABLE_L1;
>>
>> I don't remember anymore why but my guess is that the proprietary driver
>> at the time didn't support it with QCA998X. Or maybe QCA988X doesn't
>> even support L1? Michal, do you remember?
>
> Proprietary driver has it ifdef-ed to enable/disable.
>
> Older driver enabled it only for some station-only target/product so
> by default QCA988X would build with L1 flag masked out. It made sense
> to be conservative and change as little as possible to avoid random
> bugs and breakage - so the logic was inherited minus the build-time
> ifdef.
>
> However 10.4 driver seems to enable it unconditionally. I'm not sure
> if this depends on target firmware in any way or if some other host
> driver or bus settings need to be pre-set before L1 can be expected to
> work reliably.
>
> I guess there's no way other than testing it out.

No replies from anyone (including Wen) for 3 months about testing this
patch on anything else than QCA6174. So I'll drop this now, please
resubmit once test coverage is better.

-- 
Kalle Valo

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ath10k: support PCIe enter L1 state
  2019-02-08 13:42           ` Kalle Valo
@ 2019-02-08 17:05             ` Brian Norris
  0 siblings, 0 replies; 8+ messages in thread
From: Brian Norris @ 2019-02-08 17:05 UTC (permalink / raw)
  To: Kalle Valo; +Cc: Michał Kazior, Wen Gong, linux-wireless, ath10k, Wen Gong

On Fri, Feb 8, 2019 at 5:42 AM Kalle Valo <kvalo@codeaurora.org> wrote:
> No replies from anyone (including Wen) for 3 months about testing this
> patch on anything else than QCA6174. So I'll drop this now, please
> resubmit once test coverage is better.

I know this isn't exactly what you're asking for, but FWIW we've been
using this since late November on all our QCA6174 products. No issues
seen as far as I know, and we have seen some power savings.

Regards,
Brian

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-14  2:50 [PATCH] ath10k: support PCIe enter L1 state Wen Gong
2018-11-15  0:28 ` Brian Norris
2018-11-15  6:38   ` Wen Gong
2018-11-15 18:43     ` Brian Norris
2018-11-16  7:00       ` Kalle Valo
2018-11-16  7:56         ` Michał Kazior
2019-02-08 13:42           ` Kalle Valo
2019-02-08 17:05             ` Brian Norris

Linux-Wireless Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-wireless/0 linux-wireless/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-wireless linux-wireless/ https://lore.kernel.org/linux-wireless \
		linux-wireless@vger.kernel.org linux-wireless@archiver.kernel.org
	public-inbox-index linux-wireless


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-wireless


AGPL code for this site: git clone https://public-inbox.org/ public-inbox