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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 20/09/2024 23:02, Jeff Johnson wrote:
> On 9/20/2024 1:22 AM, Bartosz Golaszewski wrote:
>> On Fri, 20 Sep 2024 08:45:56 +0200, Kalle Valo <kvalo@kernel.org> said:
>>> 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.
>>>
>>
>> No issues here.
>>
>>> 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
>>> then for those devices it's impossible to automatically to choose the
>>> correct board file.
>>>
>>
>> What you're really trying to say is: we cannot use the following snippet of
>> DTS anymore:
>>
>> 	&pcie4_port0 {
>> 		wifi@0 {
>> 			compatible = "pci17cb,1103";
>> 			reg = <0x10000 0x0 0x0 0x0 0x0>;
>>
>> 			qcom,ath11k-calibration-variant = "LE_X13S";
>> 		};
>> 	};
>>
>> First: it's not true. We are not allowed to break existing device-tree sources
>> and a change to the schema has no power to do so anyway. You will however no
>> longer be able to upstream just this as it will not pass make dtbs_check
>> anymore.
>>
>> Second: this bit is incomplete even if the WCN6855 package is on a detachable
>> M.2 card. When a DT property is defined as optional in schema, it doesn't
>> mean: "the driver will work fine without it". It means: "the *hardware* does
>> not actually need it to function". That's a huge difference. DTS is not a
>> configuration file for your convenience.
>>
>>> 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.
>>>
>>
>> No, this is the convenient solution. The *correct* solution is to say how the
>> ath11k inside the WCN6855 package is really supplied. The dt-bindings should
>> define the correct representation, not the convenient one.
>>
>> Let me give you an analogy: we don't really need to have always-on, fixed
>> regulators in DTS. The drivers don't really need them. We do it for
>> completeness of the HW description.
> 
> Again, since I'm a DT n00b:
> Just to make sure I understand, you are saying that with this change any
> existing .dts/.dtb files will still work with an updated driver, so the new
> properties are not required to be populated on existing devices.

Did you folks rejected patches acked by DT maintainers on basis of not
understanding DT at all?

> 
> However a new driver with support for these properties will utilize them when
> they are present, and the current ath11k .dts files will need to be updated to

It is not related to drivers at all. Nothing in this thread is related
to drivers.

Can we entirely drop the drivers from the discussion?

> include these properties for pci17cb,1103, i.e. the following needs updating:
> arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> &pcie4_port0 {
> 	wifi@0 {
> 		compatible = "pci17cb,1103";
> 		reg = <0x10000 0x0 0x0 0x0 0x0>;
> 
> 		qcom,ath11k-calibration-variant = "LE_X13S";
> 	};
> };
> 
> 

Best regards,
Krzysztof