Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: ssreeela=codeaurora.org@mg.codeaurora.org
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Date:   Thu, 15 Oct 2020 17:07:14 +0530
From:   ssreeela@codeaurora.org
To:     Peter Oh <peter.oh@eero.com>
Cc:     ath11k@lists.infradead.org, linux-wireless@vger.kernel.org,
        Govindaraj Saminathan <gsamin@codeaurora.org>
Subject: Re: [PATCH v3] ath11k: cold boot calibration support
In-Reply-To: <b0d42c5e-8aac-cea2-bc6d-3a6341689d05@eero.com>
References: <1600163991-2093-1-git-send-email-ssreeela@codeaurora.org>
 <b0d42c5e-8aac-cea2-bc6d-3a6341689d05@eero.com>
Message-ID: <66888db2618270660f38a0c89226332f@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Precedence: bulk

On 2020-09-18 01:24, Peter Oh wrote:
> On 9/15/20 2:59 AM, Sowmiya Sree Elavalagan wrote:
>> From: Govindaraj Saminathan <gsamin@codeaurora.org>
>> 
>> cold boot calibration is the process to calibrate all the channels
>> during the boot-up to avoid the calibration delay during the
>> channel change.
>> During the boot-up, firmware started with mode “cold_boot_calibration”
>> Firmware calibrate all channels and generate CalDb(DDR).
>> Subsequent WIFI ON will reuse the same CalDb.
> Can you clarify the definition of "WIFI ON"?

WIFI ON here refers to wireless interfaces bring
up(reinitialised) in normal mode

>> Firmware restarted with Mission mode to continue the normal operation.
> 
> Please change the mission mode to normal according to
> ATH11K_FIRMWARE_MODE_NORMAL.

Sure we will change that
> 
> Which code it sets back to normal mode from cold boot mode? I don't see 
> it.
> 

Please check the function ath11k_qmi_fwreset_from_cold_boot where 
firmware is reset.
During restart, firmware mode will be reset to normal.
>> +static int ath11k_qmi_process_coldboot_calibration(struct ath11k_base 
>> *ab)
>> +{
>> +	int timeout;
>> +	int ret;
>> +
>> +	ret = ath11k_qmi_wlanfw_mode_send(ab, 
>> ATH11K_FIRMWARE_MODE_COLD_BOOT);
>> +	if (ret < 0) {
>> +		ath11k_warn(ab, "qmi failed to send wlan fw mode:%d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ath11k_dbg(ab, ATH11K_DBG_QMI, "Coldboot calibration wait 
>> started\n");
>> +
>> +	timeout = wait_event_timeout(ab->qmi.cold_boot_waitq,
>> +				     (ab->qmi.cal_done  == 1),
>> +				     ATH11K_COLD_BOOT_FW_RESET_DELAY);
>> +	if (timeout <= 0) {
>> +		ath11k_warn(ab, "Coldboot Calibration timed out\n");
>> +		return -ETIMEDOUT;
> 
> Does it make ath11k firmware stay in cold boot mode when timeout 
> happens?
> 
> If so, is it worthy to have WLAN usuable due to cold boot calibration 
> failure?
> 
> As I understand, initial calibration on current operation channel will
> be done regardless of cold boot calibration
> 

Agreed, these comments will be addressed in the next patch set.
>> +	}
>> +
>> +	ath11k_dbg(ab, ATH11K_DBG_QMI, "Coldboot calibration wait ended\n");
>> +
>> +	return 0;
>> +}
>> +
>>   static int
>>   ath11k_qmi_driver_event_post(struct ath11k_qmi *qmi,
>>   			     enum ath11k_qmi_event_type type,
>> @@ -2597,9 +2643,16 @@ static void ath11k_qmi_driver_event_work(struct 
>> work_struct *work)
>>   				break;
>>   			}
>>   -			ath11k_core_qmi_firmware_ready(ab);
>> -			ab->qmi.cal_done = 1;
>> -			set_bit(ATH11K_FLAG_REGISTERED, &ab->dev_flags);
>> +			if (ath11k_cold_boot_cal && ab->qmi.cal_done == 0 &&
>> +			    ab->hw_params.cold_boot_calib) {
>> +				ath11k_qmi_process_coldboot_calibration(ab);
>> +			} else {
>> +				clear_bit(ATH11K_FLAG_CRASH_FLUSH,
>> +					  &ab->dev_flags);
>> +				clear_bit(ATH11K_FLAG_RECOVERY, &ab->dev_flags);
> Please split this patch into a separate patch. It's not directly
> related to cold boot.

This resets the flag to commence normal operation when firmware switches 
to normal mode after cold boot calibration.
Once the firmware is ready we clear this flag so that it will not block 
any further communication with firmware.
>> +				ath11k_core_qmi_firmware_ready(ab);
>> +				set_bit(ATH11K_FLAG_REGISTERED, &ab->dev_flags);
>> +			}
>>     			break;
>>   		case ATH11K_QMI_EVENT_COLD_BOOT_CAL_DONE:
> 
> Thanks,
> 
> Peter