Re: [PATCH 1/2] dt-bindings: net: wireless: ath10k: add qcom,no-msa-ready-indicator prop

From: Kalle Valo <kvalo@kernel.org>
To: Marc Gonzalez <mgonzalez@freebox.fr>
Cc: Jeff Johnson <quic_jjohnson@quicinc.com>,
	 ath10k <ath10k@lists.infradead.org>,
	 wireless <linux-wireless@vger.kernel.org>,
	DT <devicetree@vger.kernel.org>,
	 Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	 Conor Dooley <conor+dt@kernel.org>,
	 Pierre-Hugues Husson <phhusson@freebox.fr>,
	 Jami Kettunen <jamipkettunen@gmail.com>,
	 Jeffrey Hugo <quic_jhugo@quicinc.com>
Subject: Re: [PATCH 1/2] dt-bindings: net: wireless: ath10k: add qcom,no-msa-ready-indicator prop
Date: Wed, 28 Feb 2024 18:37:08 +0200	[thread overview]
Message-ID: <87wmqoilzf.fsf@kernel.org> (raw)
In-Reply-To: <3392f356-7b19-483d-b9f8-3bd84068fa52@freebox.fr> (Marc Gonzalez's message of "Wed, 28 Feb 2024 17:12:11 +0100")

Marc Gonzalez <mgonzalez@freebox.fr> writes:

> On 28/02/2024 15:03, Kalle Valo wrote:
>
>> Marc Gonzalez writes:
>> 
>>> +  qcom,no-msa-ready-indicator:
>>> +    type: boolean
>>> +    description:
>>> +      The driver waits for this indicator before proceeding,
>>> +      yet some WCNSS firmwares apparently do not send it.
>>> +      On those devices, it seems safe to ignore the indicator,
>>> +      and continue loading the firmware.
>> 
>> This sounds more like a firmware feature, not a hardware feature. What
>> about having a flag in enum ath10k_fw_features in firmware-2.bin?
>
> Are you using the word "feature" as in "it was done purposefully" ?

No, there's no bigger meaning like that. It's more like ath10k has to do
something differently when a certain bit is enabled in the firmware. I
just had to pick a word for the enum and from my limited vocabulary I
chose "feature" :)

> Is enum ath10k_fw_features also supposed to include work-arounds?

Yes, and we already use.

> Sorry, I've grepped over the entire Linux source code,
> and I cannot find where ath10k_fw_features is used,
> other than in ath10k_core_get_fw_feature_str().

Here's one example where in ath10k we use a feature bit as a workaround:

	/* Don't trust error code from otp.bin */
	ATH10K_FW_FEATURE_IGNORE_OTP_RESULT = 7,

        ....

	if (!(skip_otp || test_bit(ATH10K_FW_FEATURE_IGNORE_OTP_RESULT,
				   ar->running_fw->fw_file.fw_features)) &&
	    result != 0) {
		ath10k_err(ar, "otp calibration failed: %d", result);
		return -EINVAL;
	}

BTW for modifying firmware-N.bin files we have a script here:

https://github.com/qca/qca-swiss-army-knife/blob/master/tools/scripts/ath10k/ath10k-fwencoder

> As mentioned in my other reply, there are several msm8998-based
> devices affected by this issue. Is it not appropriate to consider
> a kernel-based work-around?

Sorry, not following you here. But I'll try to answer anyway:

I have understood that Device Tree is supposed to describe hardware, not
software. This is why having this property in DT does not look right
place for this. For example, if the ath10k firmware is fixed then DT
would have to be changed even though nothing changed in hardware. But of
course DT maintainers have the final say.

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

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