Re: [PATCH 1/2] dt-bindings: net: bluetooth: nxp: add support for supply and reset

[Krzysztof Kozlowski <krzk@kernel.org>]

On 22/10/2024 10:13, Marco Felsch wrote:
> On 24-10-22, Krzysztof Kozlowski wrote:
>> On 22/10/2024 09:30, Krzysztof Kozlowski wrote:
>>> On 22/10/2024 09:12, Marco Felsch wrote:
>>>> On 24-10-22, Krzysztof Kozlowski wrote:
>>>>> On 21/10/2024 12:25, Marco Felsch wrote:
>>>>>> On 24-10-21, Krzysztof Kozlowski wrote:
>>>>>>> On 21/10/2024 08:41, Marco Felsch wrote:
>>>>>>>> On 24-10-07, Krzysztof Kozlowski wrote:
>>>>
>>>> ...
>>>>
>>>>>>>>> Based on earlier message:
>>>>>>>>>
>>>>>>>>> "For NXP WIFI/BT chip, WIFI and BT share the one PDn pin, which means
>>>>>>>>> that both wifi and BT controller will be powered on and off at the same
>>>>>>>>> time."
>>>>>>>>>
>>>>>>>>> but maybe that's not needed. No clue, I don't know the hardware. But be
>>>>>>>>> carefully what you write in the bindings, because then it will be ABI.
>>>>>>>>
>>>>>>>> We noticed the new power-sequencing infrastructure which is part of 6.11
>>>>>>>> too but I don't think that this patch is wrong. The DT ABI won't break
>>>>>>>> if we switch to the power-sequencing later on since the "reset-gpios"
>>>>>>>> are not marked as required. So it is up to the driver to handle it
>>>>>>>> either via a separate power-sequence driver or via "power-supply" and
>>>>>>>> "reset-gpios" directly.
>>>>>>>
>>>>>>> That's not the point. We expect correct hardware description. If you say
>>>>>>> now it has "reset-gpios" but later say "actually no, because it has
>>>>>>> PMU", I respond: no. Describe the hardware, not current Linux.
>>>>>>
>>>>>> I know that DT abstracts the HW. That said I don't see the problem with
>>>>>> this patch. The HW is abstracted just fine:
>>>>>>
>>>>>> shared PDn          -> reset-gpios
>>>>>> shared power-supply -> vcc-supply
>>>>>>
>>>>>> Right now the DT ABI for the BT part is incomplete since it assume a
>>>>>> running WLAN part or some hog-gpios to pull the device out-of-reset
>>>>>> which is addressed by this patchset.
>>>>>>
>>>>>> Making use of the new power-sequencing fw is a Linux detail and I don't
>>>>>> see why the DT can't be extended later on. We always extend the DT if
>>>>>> something is missing or if we found a better way to handle devices.
>>>>>
>>>>> Sure, although I am not really confident that you understand the
>>>>> implications - you will not be able to switch to proper power-sequencing
>>>>> with above bindings, because it will not be just possible without
>>>>> breaking the ABI or changing hardware description (which you say it is
>>>>> "fine", so complete/done). I am fine with it, just mind the implications.
>>>>
>>>> Sorry can you please share your concerns? I don't get the point yet why
>>>> we do break the DT ABI if we are going from
>>>
>>> Not necessarily breaking ABI, but changing the description.
>>>>
>>>> bt {
>>>> 	reset-gpios = <&gpio 4 0>;
>>>> 	vcc-supply = <&supply>;
>>>> };
>>>>
>>>> to
>>>>
>>>> bt {
>>>> 	vcc-supply = <&pmu_supply>;
>>>
>>> ...because you just removed reset-gpios which is a property of this device.
> 
> An optional property. That beeing said, dropping the *-gpios was the
> solution for the Qualcomm DTS as well:
> 
> bd37ce2eeb84 ("arm64: dts: qcom: qrb5165-rb5: add the Wifi node")

True, the difference is I think that we came with proper PMU model only
recently while above device is supported for few years.

This is not the case here: you can choose now hardware description which
will be both accurate and solve power sequencing issues.

> 
>>>> };
>>>>
>>>> or:
>>>>
>>>> bt {
>>>> 	pmu = <&pmu>;
>>
>> Ah, and I forgot here: this also might not be correct, because if you
>> have PMU, then the PMU consumes VCC, not the Bluetooth. Therefore both
>> of above two solutions might be inaccurate description if you decide to
>> go with PMU.
>>
>>>> };
>>>>
>>>> Of course the driver need to support all 2/3 cases due to backward
>>>> compatibility but from DT pov I don't see any breakage since we already
>>>> need to define the power handling properties (gpio & supply) as
>>>> optional.
>>>
>>> Either existing binding is complete or not. Not half-done.
> 
> As I remember DT ABI must be backward compatible. I understand this as
> followed: In our current use-case the dt-bindings don't describe any
> required hw resource so we need to mark them as optional to be backward
> compatible.
> 
> Regarding your above comment: "complete or not. Not half-done". Do you
> see the current dt-bindings for this particular device as complete or
> not? In other words can we mark the reset-gpios and vcc-supply
> properties as required albeit this would break the DT ABI since all
> current users don't specify it?

It is not about required property. Does this device has reset lines or
not? If yes, then please do not come in 2 years removing it from DTS.
Because this breaks all of DTS users.

> 
>>>> That beeing said I don't see the need for a PMU driver for this WLAN/BT
>>>> combi chip which is way simpler than the Qualcomm one from Bartosz. Also
>>>> there is physically no PMU device which powers the chip unlike the
>>>> Qualcomm one. I'm not sure if you would accept virtual PMU devices.
>>>
>>> Virtual PMU, of course not. I would like to have complete hardware
>>> description, not something which matches your current driver model.
> 
> Okay so PMU is no option and we don't have to consider this idea any
> longer. So reset-gpios and vcc-supply it is :) and I don't expect this
> to change.

Ack.

Best regards,
Krzysztof