Message-ID: <04fc202a-ac1c-44c9-ba67-8809e45ef940@kernel.org>
Date: Wed, 25 Sep 2024 09:10:13 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Kalle Valo <kvalo@...nel.org>
Cc: Bartosz Golaszewski <brgl@...ev.pl>,
 "David S . Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Jeff Johnson <jjohnson@...nel.org>,
 linux-wireless@...r.kernel.org, netdev@...r.kernel.org,
 devicetree@...r.kernel.org, ath11k@...ts.infradead.org,
 linux-kernel@...r.kernel.org,
 Bartosz Golaszewski <bartosz.golaszewski@...aro.org>,
 Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs
 of the ath11k on WCN6855

On 25/09/2024 07:58, Kalle Valo wrote:
> Krzysztof Kozlowski <krzk@...nel.org> writes:
> 
>> On 20/09/2024 08:45, Kalle Valo wrote:
>>
>>> Krzysztof Kozlowski <krzk@...nel.org> writes:
>>>
>>>> On 19/09/2024 09:48, Kalle Valo wrote:
>>>>> Krzysztof Kozlowski <krzk@...nel.org> writes:
>>>>>
>>>>>> On 14/08/2024 10:23, Bartosz Golaszewski wrote:
>>>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
>>>>>>>
>>>>>>> Describe the inputs from the PMU of the ath11k module on WCN6855.
>>>>>>>
>>>>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@...aro.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).

That's a ACPI system, so ACPI and complex drivers handle this.

> 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

On DT platform they work by coincidence or "work mostly". Ppwer
sequencing work fixes it.

> time trusting your spec and believing it's the ultimate authority. To me
> if reality and spec doesn't match, reality wins.

You miss concepts of DT. Really.

You can skip in DT many regulator supplies, which are enabled by default
(e.g. by bootloader). And what? Everything will work fine even if Linux
kernel requires them. Driver will probe fine, hardware "will work".

Still the regulators *are required* by DT rules and we have been telling
you this so many times already.

That's not a difference case here.

> 
>>> 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.

For me, there could be different WCN6885 chips, flavors or PCI cards.
The binding speaks only about given PCI device.

Best regards,
Krzysztof

Powered by blists - more mailing lists