On 2/27/2024 6:22 PM, Baochen Qiang wrote:
> Now that all infrastructure is in place and ath11k is fixed to handle all the
> corner cases, power down the ath11k firmware during suspend and power it back
> up during resume. This fixes the problem when using hibernation with ath11k PCI
> devices.
>
> For suspend, two conditions needs to be satisfied:
> 1. since MHI channel unprepare would be done in late suspend stage,
> ath11k needs to get all QMI-dependent things done before that stage.
> 2. and because unprepare MHI channels requires a working MHI stack,
> ath11k is not allowed to call mhi_power_down() until that finishes.
> So the original suspend callback is separated into two parts: the first part
> handles all QMI-dependent things in suspend callback; while the second part
> powers down MHI in suspend_late callback. This is valid because kernel calls
> ath11k's suspend callback before all suspend_late callbacks, making the first
> condition happy. And because MHI devices are children of ath11k device
> (ab->dev), kernel guarantees that ath11k's suspend_late callback is called
> after QRTR's suspend_late callback, this satisfies the second condition.
>
> Above analysis also applies to resume process. so the original resume
> callback is separated into two parts: the first part powers up MHI stack
> in resume_early callback, this guarantees MHI stack is working when
> QRTR tries to prepare MHI channels (kernel calls QRTR's resume_early callback
> after ath11k's resume_early callback, due to the child-father relationship);
> the second part waits for the completion of restart, which won't fail now
> since MHI channels are ready for use by QMI.
>
> Another notable change is in power down path, we tell mhi_power_down() to not
> to destroy MHI devices, making it possible for QRTR to help unprepare/prepare
> MHI channels, and finally get us rid of the probe-defer issue when resume.
>
> Also change related code due to interface changes.
>
> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30
>
> Tested-by: Takashi Iwai <tiwai@xxxxxxx>
> Signed-off-by: Kalle Valo <quic_kvalo@xxxxxxxxxxx>
> Signed-off-by: Baochen Qiang <quic_bqiang@xxxxxxxxxxx>
> ---
> drivers/net/wireless/ath/ath11k/ahb.c | 6 +-
> drivers/net/wireless/ath/ath11k/core.c | 105 +++++++++++++++++--------
> drivers/net/wireless/ath/ath11k/core.h | 6 +-
> drivers/net/wireless/ath/ath11k/hif.h | 14 +++-
> drivers/net/wireless/ath/ath11k/mhi.c | 12 ++-
> drivers/net/wireless/ath/ath11k/mhi.h | 5 +-
> drivers/net/wireless/ath/ath11k/pci.c | 44 +++++++++--
> drivers/net/wireless/ath/ath11k/qmi.c | 2 +-
> 8 files changed, 142 insertions(+), 52 deletions(-)
...snip...
> +int ath11k_core_resume_early(struct ath11k_base *ab)
> +{
> + int ret;
> + struct ath11k_pdev *pdev;
> + struct ath11k *ar;
> +
> + if (!ab->hw_params.supports_suspend)
> + return -EOPNOTSUPP;
> +
> + /* so far signle_pdev_only chips have supports_suspend as true
nit: s/signle/single/
> + * and only the first pdev is valid.
> + */
> + pdev = ath11k_core_get_single_pdev(ab);
> + ar = pdev->ar;
> + if (!ar || ar->state != ATH11K_STATE_OFF)
> + return 0;
> +
> + reinit_completion(&ab->restart_completed);
> + ret = ath11k_hif_power_up(ab);
> + if (ret)
> + ath11k_warn(ab, "failed to power up hif during resume: %d\n", ret);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(ath11k_core_resume_early);
>
> int ath11k_core_resume(struct ath11k_base *ab)
> {
> int ret;
> struct ath11k_pdev *pdev;
> struct ath11k *ar;
> + long time_left;
>
> if (!ab->hw_params.supports_suspend)
> return -EOPNOTSUPP;
> @@ -940,29 +990,19 @@ int ath11k_core_resume(struct ath11k_base *ab)
> if (!ar || ar->state != ATH11K_STATE_OFF)
> return 0;
>
> - ret = ath11k_hif_resume(ab);
> - if (ret) {
> - ath11k_warn(ab, "failed to resume hif during resume: %d\n", ret);
> - return ret;
> + time_left = wait_for_completion_timeout(&ab->restart_completed,
> + ATH11K_RESET_TIMEOUT_HZ);
> + if (time_left == 0) {
> + ath11k_warn(ab, "timeout while waiting for restart complete");
> + return -ETIMEDOUT;
> }
>
> - ath11k_hif_ce_irq_enable(ab);
> - ath11k_hif_irq_enable(ab);
these are disabled in suspend_late()
do you need to enable in resume_early()?
or are they expected to be enabled via ath11k_wow_op_resume()?
and if that is the case, why isn't the disable in
ath11k_wow_op_suspend() sufficient? can the disables in suspend_late()
be removed?
just concerned about the lack of symmetry here
/jeff
