Re: [PATCH v9 1/2] dt-bindings: net: wireless: qcom,ath11k: describe the ath11k on QCA6390

[Krzysztof Kozlowski]

On 06/06/2024 20:08, Bartosz Golaszewski wrote:
> On Thu, Jun 6, 2024 at 6:16 PM Kalle Valo <kvalo@xxxxxxxxxx> wrote:
>>
>> Bartosz Golaszewski <brgl@xxxxxxxx> writes:
>>
>>> On Thu, Jun 6, 2024 at 4:02 PM Kalle Valo <kvalo@xxxxxxxxxx> wrote:
>>>
>>>>
>>>> Bartosz Golaszewski <brgl@xxxxxxxx> writes:
>>>>
>>>>> On Thu, Jun 6, 2024 at 3:30 PM Kalle Valo <kvalo@xxxxxxxxxx> wrote:
>>>>>
>>>>>>
>>>>>> Bartosz Golaszewski <brgl@xxxxxxxx> writes:
>>>>>>
>>>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
>>>>>>>
>>>>>>> Add a PCI compatible for the ATH11K module on QCA6390 and describe the
>>>>>>> power inputs from the PMU that it consumes.
>>>>>>>
>>>>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
>>>>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> +allOf:
>>>>>>> +  - if:
>>>>>>> +      properties:
>>>>>>> +        compatible:
>>>>>>> +          contains:
>>>>>>> +            const: pci17cb,1101
>>>>>>> +    then:
>>>>>>> +      required:
>>>>>>> +        - vddrfacmn-supply
>>>>>>> +        - vddaon-supply
>>>>>>> +        - vddwlcx-supply
>>>>>>> +        - vddwlmx-supply
>>>>>>> +        - vddrfa0p8-supply
>>>>>>> +        - vddrfa1p2-supply
>>>>>>> +        - vddrfa1p7-supply
>>>>>>> +        - vddpcie0p9-supply
>>>>>>> +        - vddpcie1p8-supply
>>>>>>
>>>>>> Not sure if we discussed this before, but based on this I understand
>>>>>> that there can't be an DT entry for device pci17cb,1101 without all the
>>>>>> supply properties? But there are QCA6390 devices with PCI id 17cb:1101
>>>>>> which do not need these supplies and already work. For example, my Dell
>>>>>> XPS 13 x86 laptop is one. Or anyone who manually installs QCA6390 board
>>>>>> to their PCI slot and some of them might want to use DT, for example
>>>>>> setting qcom,ath11k-calibration-variant.
>>>>>>
>>>>>> This is not a blocker for me, just making sure that we are not breaking
>>>>>> any existing setups.
>>>>>>
>>>>>
>>>>> If they are already powered up without the need for the PCI pwrctl
>>>>> driver to do it, then they will work alright. Bindings don't affect
>>>>> functionality.
>>>>
>>>> Sure, I'm not worried about functionality. I'm worried that if I
>>>> there's, for example, an ARM based setup which uses DT and wants to use
>>>> a similar QCA6390 board that I have, and set
>>>> qcom,ath11k-calibration-variant in DT. In other words, I'm worried if
>>>> you are looking at this only for Snapdragon family of boards?
>>>>
>>>
>>> No, what I'm looking at is the entire QCA6390 package. That means WLAN
>>> *and* Bluetooth *and* the PMU that manages power.
>>
>> I think we are just looking at this from different point of views. You
>> are looking at a datasheet (most likely for a Snapdragon based system)
>> and I'm looking what actual devices there are out in the field.
>>
>>> If you're using the QCA6390 on a device-tree system then you should
>>> probably model at least the WLAN node and the PMU and the problem with
>>> supplies is fixed.
>>
>> But why? If there are boards out there who don't need any of this why
>> would they still need to model all this in DT?
>>
> 
> Because this is what is there? The goal of the device tree is to
> describe the hardware. The fact we didn't describe it before doesn't
> make it correct.

Correct.

Kalle,
All of the devices out there need these supplies, but they are sometimes
provided by generic PCI supply and on-board regulators. Basically your
PCI adapter is not the same as QCA6390 chip on Snapdragon board.


> 
>> Based on the discussions I have heard only Snapdragon systems who
>> require all this configuration you describe. Of course there can be
>> other systems but I have not heard about those.
>>
> 
> DT is not configuration, it is description of actual hardware. It
> doesn't matter if Snapdragon systems are the only ones that actually
> *require* this description to make WLAN/BT functional upstream. The
> chipset would be the same on any PCIe board, it's just that the host
> systems wouldn't need to take care with its power sequence. But for a
> dynamic board like this, you don't need DT.
> 

Correct.

...

> 
>>> If your detachable board "just works" then it must be wired in a way
>>> that enables WLAN the moment it's plugged in but this doesn't happen
>>> over PCI. The chipset has a power input and GPIOs to enable each
>>> module.
>>
>> I don't know how the boards are implemented but it could be so. But from
>> host system point of view it's just a regular PCI device.
>>
> 
> And you don't need DT anyway for this type of devices.

Detechable board, like PCI adapter, derives these supplies from generic
PCI whatever-3.3v through additional regulators. All these supplies are
there - on the board.

> 
>>> Also: I doubt you need DT for your detachable board?
>>
>> Sure, I don't need DT but that's not my point. My point is why require
>> these supplies for _all_ devices having PCI id 17cb:1101 (ie. QCA6390)
>> then clearly there are such devices which don't need it? To me that's
>> bad design and, if I'm understanding correctly, prevents use of
>> qcom,ath11k-calibration-variant property. To me having the supplies
>> optional in DT is more approriate.
>>
> 
> We require them because *they are physically there*.

I understand that for all known DT QCA6390 hardware, the supplies should
be provided thus they should be required. If in the future we have
different design or we represent some pluggable PCI card, then:
1. Probably that PCI card does not need power sequencing, thus no DT
description,
2. If still needs power sequencing, you can always amend bindings and
un-require the supplies.


Best regards,
Krzysztof