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

[Marc Gonzalez <mgonzalez@freebox.fr>]

On 04/03/2024 20:34, Conor Dooley wrote:

> On Mon, Mar 04, 2024 at 05:21:37PM +0100, Marc Gonzalez wrote:
>
>> On 29/02/2024 19:40, Conor Dooley wrote:
>>
>>> On Wed, Feb 28, 2024 at 06:37:08PM +0200, Kalle Valo wrote:
>>>
>>>> Marc Gonzalez wrote:
>>>>
>>>>> 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.
>>>
>>> I dunno, if the firmware affects the functionality of the hardware in a
>>> way that cannot be detected from the operating system at runtime how
>>> else is it supposed to deal with that?
>>> The devicetree is supposed to describe hardware, yes, but at a certain
>>> point the line between firmware and hardware is invisible :)
>>> Not describing software is mostly about not using it to determine
>>> software policy in the operating system.
>>
>> Recording here what was discussed a few days ago on IRC:
>>
>> If all msm8998 boards are affected, then it /might/ make sense
>> to work around the issue for ALL msm8998 boards:
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/qmi.c b/drivers/net/wireless/ath/ath10k/qmi.c
>> index 0776e79b25f3a..9da06da518fb6 100644
>> --- a/drivers/net/wireless/ath/ath10k/qmi.c
>> +++ b/drivers/net/wireless/ath/ath10k/qmi.c
>> @@ -1076,6 +1076,9 @@ int ath10k_qmi_init(struct ath10k *ar, u32 msa_size)
>>  	qmi->ar = ar;
>>  	ar_snoc->qmi = qmi;
>>  
>> +	if (of_device_is_compatible(of_root, "qcom,msm8998")
>> +		qmi->no_point_in_waiting_for_msa_ready_indicator = true;
>> +
>>  	if (of_property_read_bool(dev->of_node, "qcom,msa-fixed-perm"))
>>  		qmi->msa_fixed_perm = true;
>>  
>>
>> Thus, anyone porting an msm8998 board to mainline would automatically
>> get the work-around, without having to hunt down the feature bit,
>> and tweak the FW files.
> 
> How come the root node comes into this, don't you have a soc-specific
> compatible for the integration on this SoC?
> (I am assuming that this is not the SDIO variant, given then it'd not be
> fixed to this particular implementation)

I see only
"qcom,ipq4019-wifi"
"qcom,wcn3990-wifi"
"qcom,wcn6750-wifi"

arch/arm64/boot/dts/qcom/msm8998.dtsi uses "qcom,wcn3990-wifi"
(which is also used for 9 other SoCs).

IIUC, you're saying there probably should have been a generic
compatible AND a board-specific compatible, like for ufshc:

$ git grep qcom,ufshc arch/arm64/boot/dts/qcom
arch/arm64/boot/dts/qcom/msm8996.dtsi:                  compatible = "qcom,msm8996-ufshc", "qcom,ufshc",
arch/arm64/boot/dts/qcom/msm8998.dtsi:                  compatible = "qcom,msm8998-ufshc", "qcom,ufshc", "jedec,ufs-2.0";
arch/arm64/boot/dts/qcom/sa8775p.dtsi:                  compatible = "qcom,sa8775p-ufshc", "qcom,ufshc", "jedec,ufs-2.0";
arch/arm64/boot/dts/qcom/sc7280.dtsi:                   compatible = "qcom,sc7280-ufshc", "qcom,ufshc",
arch/arm64/boot/dts/qcom/sc8180x.dtsi:                  compatible = "qcom,sc8180x-ufshc", "qcom,ufshc",
arch/arm64/boot/dts/qcom/sc8280xp.dtsi:                 compatible = "qcom,sc8280xp-ufshc", "qcom,ufshc",
arch/arm64/boot/dts/qcom/sc8280xp.dtsi:                 compatible = "qcom,sc8280xp-ufshc", "qcom,ufshc",
arch/arm64/boot/dts/qcom/sdm845.dtsi:                   compatible = "qcom,sdm845-ufshc", "qcom,ufshc",
arch/arm64/boot/dts/qcom/sm6115.dtsi:                   compatible = "qcom,sm6115-ufshc", "qcom,ufshc", "jedec,ufs-2.0";
arch/arm64/boot/dts/qcom/sm6125.dtsi:                   compatible = "qcom,sm6125-ufshc", "qcom,ufshc", "jedec,ufs-2.0";
arch/arm64/boot/dts/qcom/sm6350.dtsi:                   compatible = "qcom,sm6350-ufshc", "qcom,ufshc",
arch/arm64/boot/dts/qcom/sm8150.dtsi:                   compatible = "qcom,sm8150-ufshc", "qcom,ufshc",
arch/arm64/boot/dts/qcom/sm8250.dtsi:                   compatible = "qcom,sm8250-ufshc", "qcom,ufshc",
arch/arm64/boot/dts/qcom/sm8350.dtsi:                   compatible = "qcom,sm8350-ufshc", "qcom,ufshc",
arch/arm64/boot/dts/qcom/sm8450.dtsi:                   compatible = "qcom,sm8450-ufshc", "qcom,ufshc",
arch/arm64/boot/dts/qcom/sm8550.dtsi:                   compatible = "qcom,sm8550-ufshc", "qcom,ufshc",
arch/arm64/boot/dts/qcom/sm8650.dtsi:                   compatible = "qcom,sm8650-ufshc", "qcom,ufshc", "jedec,ufs-2.0";


If all msm8998-based device trees had (for example)
a qcom,msm8998-wcn3990-wifi compatible, we could have
matched that to apply a quirk to all msm8998 boards.

Did I understand correctly?

(In the toy code I provided, I matched the of_root because
there is no SoC-specific compatible for the device itself.)

-- 
Regards