Re: [PATCH] ath10k: support PCIe enter L1 state

From: Kalle Valo <kvalo@codeaurora.org>
To: "Michał Kazior" <kazikcz@gmail.com>
Cc: wgong@qti.qualcomm.com, briannorris@chromium.org,
	linux-wireless <linux-wireless@vger.kernel.org>,
	ath10k@lists.infradead.org, Wen Gong <wgong@codeaurora.org>
Subject: Re: [PATCH] ath10k: support PCIe enter L1 state
Date: Fri, 08 Feb 2019 15:42:44 +0200	[thread overview]
Message-ID: <87y36q75wr.fsf@kamboji.qca.qualcomm.com> (raw)
In-Reply-To: <CABvG-CVAnwkiKVJik0PdsmRxF62kKv2N+aRKNq=nbopoExLvDA@mail.gmail.com> (=?utf-8?Q?=22Micha=C5=82?= Kazior"'s message of "Fri, 16 Nov 2018 08:56:27 +0100")

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