Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Kalle Valo <kvalo@kernel.org>]

Krzysztof Kozlowski <krzk@kernel.org> writes:

> On 20/09/2024 08:45, Kalle Valo wrote:
>
>> Krzysztof Kozlowski <krzk@kernel.org> writes:
>> 
>>> On 19/09/2024 09:48, Kalle Valo wrote:
>>>> Krzysztof Kozlowski <krzk@kernel.org> writes:
>>>>
>>>>> On 14/08/2024 10:23, Bartosz Golaszewski wrote:
>>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>>
>>>>>> Describe the inputs from the PMU of the ath11k module on WCN6855.
>>>>>>
>>>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>> ---
>>>>>> v1 -> v2:
>>>>>> - update the example
>>>>>
>>>>> I don't understand why this patch is no being picked up. The code
>>>>> correct represents the piece of hardware. The supplies should be
>>>>> required, because this one particular device - the one described in this
>>>>> binding - cannot work without them.
>>>>
>>>> I have already explained the situation. With supplies changed to
>>>> optional I'm happy take the patch.
>>>
>>> You did not provide any relevant argument to this case. Your concerns
>>> described quite different case and are no applicable to DT based platforms.
>> 
>> Ok, I'll try to explain my concerns one more time. I'll try to be
>> thorough so will be a longer mail.
>> 
>> In ath11k we have board files, it's basically board/product specific
>> calibration data which is combined with the calibration data from chip's
>> OTP. Choosing the correct board file is essential as otherwise the
>> performance can be bad or the device doesn't work at all.
>> 
>> The board files are stored in board-2.bin file in /lib/firmware. ath11k
>> chooses the correct board file based on the information provided by the
>> ath11k firmware and then transfers the board file to firmware. From
>> board-2.bin the correct board file is search based on strings like this:
>> 
>> bus=pci,vendor=17cb,device=1103,subsystem-vendor=105b,subsystem-device=e0ca,qmi-chip-id=2,qmi-board-id=255
>> bus=pci,vendor=17cb,device=1103,subsystem-vendor=105b,subsystem-device=e0ca,qmi-chip-id=2,qmi-board-id=255,variant=HO_BNM
>> 
>> But the firmware does not always provide unique enough information for
>> choosing the correct board file and that's why we added the variant
>> property (the second example above). This variant property gives us the
>> means to name the board files uniquely and not have any conflicts. In
>> x86 systems we retrieve it from SMBIOS and in DT systems using
>> qcom,ath11k-calibration-variant property.
>> 
>> If WCN6855 supplies are marked as required, it means that we cannot use
>> qcom,ath11k-calibration-variant DT property anymore with WCN6855 M.2
>> boards. So if we have devices which don't provide unique information
>
> No, it does not mean that.
>
>> then for those devices it's impossible to automatically to choose the
>> correct board file.
>
> Anyway, only this device must be fully described, because you cannot
> have pci17cb,1103 without these supplies. It's just electrically not
> possible, according to our investigation.

Yeah, you have been telling that all along. But on the contrary I have
WCN6855 (pci17cb,1103) M.2 board which I installed to my NUC and they
haven't needed any supplies (unless BIOS does something automatically).
Also I have QCA6390 and WCN7850 M.2 boards, both which you claim needs
the supplies, and they just work out-of-box as well. So I have a hard
time trusting your spec and believing it's the ultimate authority. To me
if reality and spec doesn't match, reality wins.

>> So based on this, to me the correct solution here is to make the
>> supplies optional so that qcom,ath11k-calibration-variant DT property
>> can continue to be used with WCN6855 M.2 boards.
>
> WCN6855 can still do whatever they want. They are not restricted, not
> limited. pci17cb,1103 must provide suppplies, because pci17cb,1103
> cannot work without them.

Claiming that WCN6885 can still do whatever they want is confusing to me
because WCN6855 is pci17cb,1103, there are no other ids. See
ath11k/pci.c:

#define WCN6855_DEVICE_ID		0x1103

{ PCI_VDEVICE(QCOM, WCN6855_DEVICE_ID) },

But this discussion is going circles and honestly is waste of time. I
don't think the patch is right but I'll apply it anyway, let's deal with
the problems if/when they come up.

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

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