From:   Kalle Valo <kvalo@kernel.org>
To:     Manikanta Pubbisetty <quic_mpubbise@quicinc.com>
Cc:     <ath11k@lists.infradead.org>, <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH v4 3/3] ath11k: Enable low power mode when WLAN is not active
References: <20230203060128.19625-1-quic_mpubbise@quicinc.com>
        <20230203060128.19625-4-quic_mpubbise@quicinc.com>
Date:   Fri, 24 Feb 2023 16:50:17 +0200
In-Reply-To: <20230203060128.19625-4-quic_mpubbise@quicinc.com> (Manikanta
        Pubbisetty's message of "Fri, 3 Feb 2023 11:31:28 +0530")
Message-ID: <87cz5zruc6.fsf@kernel.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Manikanta Pubbisetty <quic_mpubbise@quicinc.com> writes:

> Currently, WLAN chip is powered once during driver probe and is kept
> ON (powered) always even when WLAN is not active; keeping the chip
> powered ON all the time will consume extra power which is not
> desirable for a battery operated device. Same is the case with non-WoW
> suspend, chip will never be put into low power mode when the system is
> suspended resulting in higher battery drain.
>
> As per the recommendation, sending a PDEV suspend WMI command followed
> by a QMI MODE OFF command will cease all WLAN activity and put the device
> in low power mode. When WLAN interfaces are brought up, sending a QMI
> MISSION MODE command would be sufficient to bring the chip out of low
> power. This is a better approach than doing hif_power_down()/hif_power_up()
> for every WiFi ON/OFF sequence since the turnaround time for entry/exit of
> low power mode is much less. Overhead is just the time taken for sending
> QMI MODE OFF & QMI MISSION MODE commands instead of going through the
> entire chip boot & QMI init sequence.
>
> Currently the changes are applicable only for WCN6750. This can be
> extended to other targets with a future patch.
>
> Tested-on: WCN6750 hw1.0 AHB WLAN.MSL.1.0.1-00887-QCAMSLSWPLZ-1
> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.16
>
> Signed-off-by: Manikanta Pubbisetty <quic_mpubbise@quicinc.com>

[...]

> +static int ath11k_ahb_core_start_ipq8074(struct ath11k_base *ab)
> +{
> +	/* TODO: Currently initializing the hardware/firmware only
> +	 * during hardware recovery. Support to shutdown/turn-on
> +	 * the hardware during Wi-Fi OFF/ON will be added later.
> +	 */
> +	if (!test_bit(ATH11K_FLAG_RECOVERY, &ab->dev_flags))
> +		return 0;
> +
> +	return ath11k_core_start_device(ab);
> +}
> +
> +static void ath11k_ahb_core_stop_ipq8074(struct ath11k_base *ab)
> +{
> +	/* TODO: Currently stopping the hardware/firmware only
> +	 * during driver unload. Support to shutdown/turn-on
> +	 * the hardware during Wi-Fi OFF/ON will be added later.
> +	 */
> +	if (!test_bit(ATH11K_FLAG_UNREGISTERING, &ab->dev_flags))
> +		return;
> +
> +	return ath11k_core_stop_device(ab);
> +}

Please clarify what Wi-Fi OFF/ON exactly means on these two comments,
it's not clear for me.

Also I want to mention that I suspect eventually we have to always power
off the firmware during suspend to get hibernation working:

https://bugzilla.kernel.org/show_bug.cgi?id=214649

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches