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

Commit Message

POPESCU Catalin Oct. 4, 2024, 11:35 a.m. UTC

Add support for chip power supply and chip reset/powerdown.

Signed-off-by: Catalin Popescu <[e-mail]>
---
 .../bindings/net/bluetooth/nxp,88w8987-bt.yaml        | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Conor Dooley Oct. 4, 2024, 3:24 p.m. UTC | #1

On Fri, Oct 04, 2024 at 01:35:56PM +0200, Catalin Popescu wrote:
> Add support for chip power supply and chip reset/powerdown.
> 
> Signed-off-by: Catalin Popescu <[e-mail]>

Acked-by: Conor Dooley <[e-mail]>

Sherry Sun Oct. 6, 2024, 8:49 a.m. UTC | #2

> -----Original Message-----
> From: Catalin Popescu <[e-mail]>
> Sent: Friday, October 4, 2024 7:36 PM
> To: Amitkumar Karwar <[e-mail]>; Neeraj Sanjay Kale
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Cc: [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail]; bsp-
> [e-mail]; Catalin Popescu
> <[e-mail]>
> Subject: [PATCH 1/2] dt-bindings: net: bluetooth: nxp: add support for supply
> and reset
> 
> Add support for chip power supply and chip reset/powerdown.
> 
> Signed-off-by: Catalin Popescu <[e-mail]>
> ---
>  .../bindings/net/bluetooth/nxp,88w8987-bt.yaml        | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-
> bt.yaml b/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-
> bt.yaml
> index 37a65badb448..8520b3812bd2 100644
> --- a/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-
> bt.yaml
> +++ b/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-
> bt.yaml
> @@ -34,6 +34,14 @@ properties:
>    firmware-name:
>      maxItems: 1
> 
> +  vcc-supply:
> +    description:
> +      phandle of the regulator that provides the supply voltage.
> +
> +  reset-gpios:
> +    description:
> +      Chip powerdown/reset signal (PDn).
> +

Hi Catalin,

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.
Taking the M.2 NXP WIFI/BT module as an example, pin56(W_DISABLE1) is connected to the WIFI/BT chip PDn pin, we has already controlled this pin in the corresponding PCIe/SDIO controller dts nodes.
It is not clear to me what exactly pins for vcc-supply and reset-gpios you describing here. Can you help understand the corresponding pins on M.2 interface as an example? Thanks.

Best Regards
Sherry


>  required:
>    - compatible
> 
> @@ -41,10 +49,13 @@ additionalProperties: false
> 
>  examples:
>    - |
> +    #include <dt-bindings/gpio/gpio.h>
>      serial {
>          bluetooth {
>              compatible = "nxp,88w8987-bt";
>              fw-init-baudrate = <3000000>;
>              firmware-name = "uartuart8987_bt_v0.bin";
> +            vcc-supply = <&nxp_iw612_supply>;
> +            reset-gpios = <&gpioctrl 2 GPIO_ACTIVE_LOW>;
>          };
>      };
> --
> 2.34.1
>

Krzysztof Kozlowski Oct. 6, 2024, 11:33 a.m. UTC | #3

On 06/10/2024 10:49, Sherry Sun wrote:
> 
> 
>> -----Original Message-----
>> From: Catalin Popescu <[e-mail]>
>> Sent: Friday, October 4, 2024 7:36 PM
>> To: Amitkumar Karwar <[e-mail]>; Neeraj Sanjay Kale
>> <[e-mail]>; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]
>> Cc: [e-mail]; [e-mail]; linux-
>> [e-mail]; [e-mail]; bsp-
>> [e-mail]; Catalin Popescu
>> <[e-mail]>
>> Subject: [PATCH 1/2] dt-bindings: net: bluetooth: nxp: add support for supply
>> and reset
>>
>> Add support for chip power supply and chip reset/powerdown.
>>
>> Signed-off-by: Catalin Popescu <[e-mail]>
>> ---
>>  .../bindings/net/bluetooth/nxp,88w8987-bt.yaml        | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-
>> bt.yaml b/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-
>> bt.yaml
>> index 37a65badb448..8520b3812bd2 100644
>> --- a/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-
>> bt.yaml
>> +++ b/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-
>> bt.yaml
>> @@ -34,6 +34,14 @@ properties:
>>    firmware-name:
>>      maxItems: 1
>>
>> +  vcc-supply:
>> +    description:
>> +      phandle of the regulator that provides the supply voltage.
>> +
>> +  reset-gpios:
>> +    description:
>> +      Chip powerdown/reset signal (PDn).
>> +
> 
> Hi Catalin,
> 
> 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.
> Taking the M.2 NXP WIFI/BT module as an example, pin56(W_DISABLE1) is connected to the WIFI/BT chip PDn pin, we has already controlled this pin in the corresponding PCIe/SDIO controller dts nodes.
> It is not clear to me what exactly pins for vcc-supply and reset-gpios you describing here. Can you help understand the corresponding pins on M.2 interface as an example? Thanks.

Please wrap your replies.

It seems you need power sequencing just like Bartosz did for Qualcomm WCN.


Best regards,
Krzysztof

POPESCU Catalin Oct. 7, 2024, 12:58 p.m. UTC | #4

On 06/10/2024 13:33, Krzysztof Kozlowski wrote:
> [Some people who received this message don't often get email from [e-mail]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>
>
> On 06/10/2024 10:49, Sherry Sun wrote:
>>
>>> -----Original Message-----
>>> From: Catalin Popescu <[e-mail]>
>>> Sent: Friday, October 4, 2024 7:36 PM
>>> To: Amitkumar Karwar <[e-mail]>; Neeraj Sanjay Kale
>>> <[e-mail]>; [e-mail];
>>> [e-mail]; [e-mail]; [e-mail];
>>> [e-mail]; [e-mail]
>>> Cc: [e-mail]; [e-mail]; linux-
>>> [e-mail]; [e-mail]; bsp-
>>> [e-mail]; Catalin Popescu
>>> <[e-mail]>
>>> Subject: [PATCH 1/2] dt-bindings: net: bluetooth: nxp: add support for supply
>>> and reset
>>>
>>> Add support for chip power supply and chip reset/powerdown.
>>>
>>> Signed-off-by: Catalin Popescu <[e-mail]>
>>> ---
>>>   .../bindings/net/bluetooth/nxp,88w8987-bt.yaml        | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-
>>> bt.yaml b/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-
>>> bt.yaml
>>> index 37a65badb448..8520b3812bd2 100644
>>> --- a/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-
>>> bt.yaml
>>> +++ b/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-
>>> bt.yaml
>>> @@ -34,6 +34,14 @@ properties:
>>>     firmware-name:
>>>       maxItems: 1
>>>
>>> +  vcc-supply:
>>> +    description:
>>> +      phandle of the regulator that provides the supply voltage.
>>> +
>>> +  reset-gpios:
>>> +    description:
>>> +      Chip powerdown/reset signal (PDn).
>>> +
>> Hi Catalin,
>>
>> 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.
>> Taking the M.2 NXP WIFI/BT module as an example, pin56(W_DISABLE1) is connected to the WIFI/BT chip PDn pin, we has already controlled this pin in the corresponding PCIe/SDIO controller dts nodes.
>> It is not clear to me what exactly pins for vcc-supply and reset-gpios you describing here. Can you help understand the corresponding pins on M.2 interface as an example? Thanks.

Hi Sherry,

Regulators and reset controls being refcounted, we can then implement 
powerup sequence in both bluetooth/wlan drivers and have the drivers 
operate independently. This way bluetooth driver would has no dependance 
on the wlan driver for :

- its power supply

- its reset pin (PDn)

- its firmware (being downloaded as part of the combo firmware)

For the wlan driver we use mmc power sequence to drive the chip reset 
pin and there's another patchset that adds support for reset control 
into the mmc pwrseq simple driver.

> Please wrap your replies.
>
> It seems you need power sequencing just like Bartosz did for Qualcomm WCN.

Hi Krzysztof,

I'm not familiar with power sequencing, but looks like way more 
complicated than reset controls. So, why power sequencing is recommended 
here ? Is it b/c a supply is involved ?

>
> Best regards,
> Krzysztof
>

Krzysztof Kozlowski Oct. 7, 2024, 2:54 p.m. UTC | #5

On 07/10/2024 14:58, POPESCU Catalin wrote:
>>>>
>>>> +  vcc-supply:
>>>> +    description:
>>>> +      phandle of the regulator that provides the supply voltage.
>>>> +
>>>> +  reset-gpios:
>>>> +    description:
>>>> +      Chip powerdown/reset signal (PDn).
>>>> +
>>> Hi Catalin,
>>>
>>> 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.
>>> Taking the M.2 NXP WIFI/BT module as an example, pin56(W_DISABLE1) is connected to the WIFI/BT chip PDn pin, we has already controlled this pin in the corresponding PCIe/SDIO controller dts nodes.
>>> It is not clear to me what exactly pins for vcc-supply and reset-gpios you describing here. Can you help understand the corresponding pins on M.2 interface as an example? Thanks.
> 
> Hi Sherry,
> 
> Regulators and reset controls being refcounted, we can then implement 
> powerup sequence in both bluetooth/wlan drivers and have the drivers 
> operate independently. This way bluetooth driver would has no dependance 
> on the wlan driver for :
> 
> - its power supply
> 
> - its reset pin (PDn)
> 
> - its firmware (being downloaded as part of the combo firmware)
> 
> For the wlan driver we use mmc power sequence to drive the chip reset 
> pin and there's another patchset that adds support for reset control 
> into the mmc pwrseq simple driver.
> 
>> Please wrap your replies.
>>
>> It seems you need power sequencing just like Bartosz did for Qualcomm WCN.
> 
> Hi Krzysztof,
> 
> I'm not familiar with power sequencing, but looks like way more 
> complicated than reset controls. So, why power sequencing is recommended 
> here ? Is it b/c a supply is involved ?

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.

Best regards,
Krzysztof

Marco Felsch Oct. 21, 2024, 6:41 a.m. UTC | #6

On 24-10-07, Krzysztof Kozlowski wrote:
> On 07/10/2024 14:58, POPESCU Catalin wrote:
> >>>>
> >>>> +  vcc-supply:
> >>>> +    description:
> >>>> +      phandle of the regulator that provides the supply voltage.
> >>>> +
> >>>> +  reset-gpios:
> >>>> +    description:
> >>>> +      Chip powerdown/reset signal (PDn).
> >>>> +
> >>> Hi Catalin,
> >>>
> >>> 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.
> >>> Taking the M.2 NXP WIFI/BT module as an example, pin56(W_DISABLE1) is connected to the WIFI/BT chip PDn pin, we has already controlled this pin in the corresponding PCIe/SDIO controller dts nodes.
> >>> It is not clear to me what exactly pins for vcc-supply and reset-gpios you describing here. Can you help understand the corresponding pins on M.2 interface as an example? Thanks.
> > 
> > Hi Sherry,
> > 
> > Regulators and reset controls being refcounted, we can then implement 
> > powerup sequence in both bluetooth/wlan drivers and have the drivers 
> > operate independently. This way bluetooth driver would has no dependance 
> > on the wlan driver for :
> > 
> > - its power supply
> > 
> > - its reset pin (PDn)
> > 
> > - its firmware (being downloaded as part of the combo firmware)
> > 
> > For the wlan driver we use mmc power sequence to drive the chip reset 
> > pin and there's another patchset that adds support for reset control 
> > into the mmc pwrseq simple driver.
> > 
> >> Please wrap your replies.
> >>
> >> It seems you need power sequencing just like Bartosz did for Qualcomm WCN.
> > 
> > Hi Krzysztof,
> > 
> > I'm not familiar with power sequencing, but looks like way more 
> > complicated than reset controls. So, why power sequencing is recommended 
> > here ? Is it b/c a supply is involved ?
> 
> 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.

Regards,
  Marco

> Best regards,
> Krzysztof
> 
>

Krzysztof Kozlowski Oct. 21, 2024, 7:50 a.m. UTC | #7

On 21/10/2024 08:41, Marco Felsch wrote:
> On 24-10-07, Krzysztof Kozlowski wrote:
>> On 07/10/2024 14:58, POPESCU Catalin wrote:
>>>>>>
>>>>>> +  vcc-supply:
>>>>>> +    description:
>>>>>> +      phandle of the regulator that provides the supply voltage.
>>>>>> +
>>>>>> +  reset-gpios:
>>>>>> +    description:
>>>>>> +      Chip powerdown/reset signal (PDn).
>>>>>> +
>>>>> Hi Catalin,
>>>>>
>>>>> 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.
>>>>> Taking the M.2 NXP WIFI/BT module as an example, pin56(W_DISABLE1) is connected to the WIFI/BT chip PDn pin, we has already controlled this pin in the corresponding PCIe/SDIO controller dts nodes.
>>>>> It is not clear to me what exactly pins for vcc-supply and reset-gpios you describing here. Can you help understand the corresponding pins on M.2 interface as an example? Thanks.
>>>
>>> Hi Sherry,
>>>
>>> Regulators and reset controls being refcounted, we can then implement 
>>> powerup sequence in both bluetooth/wlan drivers and have the drivers 
>>> operate independently. This way bluetooth driver would has no dependance 
>>> on the wlan driver for :
>>>
>>> - its power supply
>>>
>>> - its reset pin (PDn)
>>>
>>> - its firmware (being downloaded as part of the combo firmware)
>>>
>>> For the wlan driver we use mmc power sequence to drive the chip reset 
>>> pin and there's another patchset that adds support for reset control 
>>> into the mmc pwrseq simple driver.
>>>
>>>> Please wrap your replies.
>>>>
>>>> It seems you need power sequencing just like Bartosz did for Qualcomm WCN.
>>>
>>> Hi Krzysztof,
>>>
>>> I'm not familiar with power sequencing, but looks like way more 
>>> complicated than reset controls. So, why power sequencing is recommended 
>>> here ? Is it b/c a supply is involved ?
>>
>> 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.

Best regards,
Krzysztof

Marco Felsch Oct. 21, 2024, 10:25 a.m. UTC | #8

On 24-10-21, Krzysztof Kozlowski wrote:
> On 21/10/2024 08:41, Marco Felsch wrote:
> > On 24-10-07, Krzysztof Kozlowski wrote:
> >> On 07/10/2024 14:58, POPESCU Catalin wrote:
> >>>>>>
> >>>>>> +  vcc-supply:
> >>>>>> +    description:
> >>>>>> +      phandle of the regulator that provides the supply voltage.
> >>>>>> +
> >>>>>> +  reset-gpios:
> >>>>>> +    description:
> >>>>>> +      Chip powerdown/reset signal (PDn).
> >>>>>> +
> >>>>> Hi Catalin,
> >>>>>
> >>>>> 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.
> >>>>> Taking the M.2 NXP WIFI/BT module as an example, pin56(W_DISABLE1) is connected to the WIFI/BT chip PDn pin, we has already controlled this pin in the corresponding PCIe/SDIO controller dts nodes.
> >>>>> It is not clear to me what exactly pins for vcc-supply and reset-gpios you describing here. Can you help understand the corresponding pins on M.2 interface as an example? Thanks.
> >>>
> >>> Hi Sherry,
> >>>
> >>> Regulators and reset controls being refcounted, we can then implement 
> >>> powerup sequence in both bluetooth/wlan drivers and have the drivers 
> >>> operate independently. This way bluetooth driver would has no dependance 
> >>> on the wlan driver for :
> >>>
> >>> - its power supply
> >>>
> >>> - its reset pin (PDn)
> >>>
> >>> - its firmware (being downloaded as part of the combo firmware)
> >>>
> >>> For the wlan driver we use mmc power sequence to drive the chip reset 
> >>> pin and there's another patchset that adds support for reset control 
> >>> into the mmc pwrseq simple driver.
> >>>
> >>>> Please wrap your replies.
> >>>>
> >>>> It seems you need power sequencing just like Bartosz did for Qualcomm WCN.
> >>>
> >>> Hi Krzysztof,
> >>>
> >>> I'm not familiar with power sequencing, but looks like way more 
> >>> complicated than reset controls. So, why power sequencing is recommended 
> >>> here ? Is it b/c a supply is involved ?
> >>
> >> 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.

Regards,
  Marco

Krzysztof Kozlowski Oct. 22, 2024, 5:11 a.m. UTC | #9

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:
>>>> On 07/10/2024 14:58, POPESCU Catalin wrote:
>>>>>>>>
>>>>>>>> +  vcc-supply:
>>>>>>>> +    description:
>>>>>>>> +      phandle of the regulator that provides the supply voltage.
>>>>>>>> +
>>>>>>>> +  reset-gpios:
>>>>>>>> +    description:
>>>>>>>> +      Chip powerdown/reset signal (PDn).
>>>>>>>> +
>>>>>>> Hi Catalin,
>>>>>>>
>>>>>>> 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.
>>>>>>> Taking the M.2 NXP WIFI/BT module as an example, pin56(W_DISABLE1) is connected to the WIFI/BT chip PDn pin, we has already controlled this pin in the corresponding PCIe/SDIO controller dts nodes.
>>>>>>> It is not clear to me what exactly pins for vcc-supply and reset-gpios you describing here. Can you help understand the corresponding pins on M.2 interface as an example? Thanks.
>>>>>
>>>>> Hi Sherry,
>>>>>
>>>>> Regulators and reset controls being refcounted, we can then implement 
>>>>> powerup sequence in both bluetooth/wlan drivers and have the drivers 
>>>>> operate independently. This way bluetooth driver would has no dependance 
>>>>> on the wlan driver for :
>>>>>
>>>>> - its power supply
>>>>>
>>>>> - its reset pin (PDn)
>>>>>
>>>>> - its firmware (being downloaded as part of the combo firmware)
>>>>>
>>>>> For the wlan driver we use mmc power sequence to drive the chip reset 
>>>>> pin and there's another patchset that adds support for reset control 
>>>>> into the mmc pwrseq simple driver.
>>>>>
>>>>>> Please wrap your replies.
>>>>>>
>>>>>> It seems you need power sequencing just like Bartosz did for Qualcomm WCN.
>>>>>
>>>>> Hi Krzysztof,
>>>>>
>>>>> I'm not familiar with power sequencing, but looks like way more 
>>>>> complicated than reset controls. So, why power sequencing is recommended 
>>>>> here ? Is it b/c a supply is involved ?
>>>>
>>>> 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.

Best regards,
Krzysztof

Sherry Sun Oct. 22, 2024, 5:13 a.m. UTC | #10

> -----Original Message-----
> From: Marco Felsch <[e-mail]>
> Sent: Monday, October 21, 2024 6:26 PM
> To: Krzysztof Kozlowski <[e-mail]>
> Cc: POPESCU Catalin <[e-mail]>; Sherry Sun
> <[e-mail]>; Amitkumar Karwar <[e-mail]>;
> Neeraj Sanjay Kale <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail]; linux-
> [e-mail]; GEO-CHHER-bsp-development <bsp-
> [e-mail]>
> Subject: Re: [PATCH 1/2] dt-bindings: net: bluetooth: nxp: add support for
> supply and reset
> 
> On 24-10-21, Krzysztof Kozlowski wrote:
> > On 21/10/2024 08:41, Marco Felsch wrote:
> > > On 24-10-07, Krzysztof Kozlowski wrote:
> > >> On 07/10/2024 14:58, POPESCU Catalin wrote:
> > >>>>>>
> > >>>>>> +  vcc-supply:
> > >>>>>> +    description:
> > >>>>>> +      phandle of the regulator that provides the supply voltage.
> > >>>>>> +
> > >>>>>> +  reset-gpios:
> > >>>>>> +    description:
> > >>>>>> +      Chip powerdown/reset signal (PDn).
> > >>>>>> +
> > >>>>> Hi Catalin,
> > >>>>>
> > >>>>> 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.
> > >>>>> Taking the M.2 NXP WIFI/BT module as an example,
> pin56(W_DISABLE1) is connected to the WIFI/BT chip PDn pin, we has already
> controlled this pin in the corresponding PCIe/SDIO controller dts nodes.
> > >>>>> It is not clear to me what exactly pins for vcc-supply and reset-gpios
> you describing here. Can you help understand the corresponding pins on M.2
> interface as an example? Thanks.
> > >>>
> > >>> Hi Sherry,
> > >>>
> > >>> Regulators and reset controls being refcounted, we can then
> > >>> implement powerup sequence in both bluetooth/wlan drivers and have
> > >>> the drivers operate independently. This way bluetooth driver would
> > >>> has no dependance on the wlan driver for :
> > >>>
> > >>> - its power supply
> > >>>
> > >>> - its reset pin (PDn)
> > >>>
> > >>> - its firmware (being downloaded as part of the combo firmware)
> > >>>
> > >>> For the wlan driver we use mmc power sequence to drive the chip
> > >>> reset pin and there's another patchset that adds support for reset
> > >>> control into the mmc pwrseq simple driver.
> > >>>
> > >>>> Please wrap your replies.
> > >>>>
> > >>>> It seems you need power sequencing just like Bartosz did for
> Qualcomm WCN.
> > >>>
> > >>> Hi Krzysztof,
> > >>>
> > >>> I'm not familiar with power sequencing, but looks like way more
> > >>> complicated than reset controls. So, why power sequencing is
> > >>> recommended here ? Is it b/c a supply is involved ?
> > >>
> > >> 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
> 

Actually we should use vcc-supply to control the PDn pin, this is the power supply for NXP wifi/BT.

Best Regards
Sherry

Marco Felsch Oct. 22, 2024, 7:12 a.m. UTC | #11

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

bt {
	reset-gpios = <&gpio 4 0>;
	vcc-supply = <&supply>;
};

to

bt {
	vcc-supply = <&pmu_supply>;
};

or:

bt {
	pmu = <&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.

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.

Regards,
  Marco

> Best regards,
> Krzysztof
> 
>

Marco Felsch Oct. 22, 2024, 7:23 a.m. UTC | #12

On 24-10-22, Sherry Sun wrote:
> > On 24-10-21, Krzysztof Kozlowski wrote:
> > > On 21/10/2024 08:41, Marco Felsch wrote:
> > > > On 24-10-07, Krzysztof Kozlowski wrote:
> > > >> On 07/10/2024 14:58, POPESCU Catalin wrote:
> > > >>>>>>
> > > >>>>>> +  vcc-supply:
> > > >>>>>> +    description:
> > > >>>>>> +      phandle of the regulator that provides the supply voltage.
> > > >>>>>> +
> > > >>>>>> +  reset-gpios:
> > > >>>>>> +    description:
> > > >>>>>> +      Chip powerdown/reset signal (PDn).
> > > >>>>>> +
> > > >>>>> Hi Catalin,
> > > >>>>>
> > > >>>>> 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.
> > > >>>>> Taking the M.2 NXP WIFI/BT module as an example,
> > pin56(W_DISABLE1) is connected to the WIFI/BT chip PDn pin, we has already
> > controlled this pin in the corresponding PCIe/SDIO controller dts nodes.
> > > >>>>> It is not clear to me what exactly pins for vcc-supply and reset-gpios
> > you describing here. Can you help understand the corresponding pins on M.2
> > interface as an example? Thanks.
> > > >>>
> > > >>> Hi Sherry,
> > > >>>
> > > >>> Regulators and reset controls being refcounted, we can then
> > > >>> implement powerup sequence in both bluetooth/wlan drivers and have
> > > >>> the drivers operate independently. This way bluetooth driver would
> > > >>> has no dependance on the wlan driver for :
> > > >>>
> > > >>> - its power supply
> > > >>>
> > > >>> - its reset pin (PDn)
> > > >>>
> > > >>> - its firmware (being downloaded as part of the combo firmware)
> > > >>>
> > > >>> For the wlan driver we use mmc power sequence to drive the chip
> > > >>> reset pin and there's another patchset that adds support for reset
> > > >>> control into the mmc pwrseq simple driver.
> > > >>>
> > > >>>> Please wrap your replies.
> > > >>>>
> > > >>>> It seems you need power sequencing just like Bartosz did for
> > Qualcomm WCN.
> > > >>>
> > > >>> Hi Krzysztof,
> > > >>>
> > > >>> I'm not familiar with power sequencing, but looks like way more
> > > >>> complicated than reset controls. So, why power sequencing is
> > > >>> recommended here ? Is it b/c a supply is involved ?
> > > >>
> > > >> 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
> 
> Actually we should use vcc-supply to control the PDn pin, this is the
> power supply for NXP wifi/BT.

Please don't since this is regular pin on the wlan/bt device not the
regulator. People often do that for GPIOs if the driver is missing the
support to pull the reset/pdn/enable gpio but the enable-gpio on the
regulator is to enable the regulator and _not_ the bt/wlan device.

Therefore the implementation Catalin provided is the correct one.

Regards,
  Marco

Krzysztof Kozlowski Oct. 22, 2024, 7:30 a.m. UTC | #13

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.

> };
> 
> or:
> 
> bt {
> 	pmu = <&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.

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

Best regards,
Krzysztof

Krzysztof Kozlowski Oct. 22, 2024, 7:32 a.m. UTC | #14

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.
> 
>> };
>>
>> 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.
> 
>>
>> 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.
> 
> Best regards,
> Krzysztof
> 

Best regards,
Krzysztof

Sherry Sun Oct. 22, 2024, 8:09 a.m. UTC | #15

> -----Original Message-----
> From: Marco Felsch <[e-mail]>
> Sent: Tuesday, October 22, 2024 3:23 PM
> To: Sherry Sun <[e-mail]>
> Cc: POPESCU Catalin <[e-mail]>; Amitkumar
> Karwar <[e-mail]>; Neeraj Sanjay Kale
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail]; linux-
> [e-mail]; GEO-CHHER-bsp-development <bsp-
> [e-mail]>; Krzysztof Kozlowski
> <[e-mail]>
> Subject: Re: [PATCH 1/2] dt-bindings: net: bluetooth: nxp: add support for
> supply and reset
> 
> On 24-10-22, Sherry Sun wrote:
> > > On 24-10-21, Krzysztof Kozlowski wrote:
> > > > On 21/10/2024 08:41, Marco Felsch wrote:
> > > > > On 24-10-07, Krzysztof Kozlowski wrote:
> > > > >> On 07/10/2024 14:58, POPESCU Catalin wrote:
> > > > >>>>>>
> > > > >>>>>> +  vcc-supply:
> > > > >>>>>> +    description:
> > > > >>>>>> +      phandle of the regulator that provides the supply voltage.
> > > > >>>>>> +
> > > > >>>>>> +  reset-gpios:
> > > > >>>>>> +    description:
> > > > >>>>>> +      Chip powerdown/reset signal (PDn).
> > > > >>>>>> +
> > > > >>>>> Hi Catalin,
> > > > >>>>>
> > > > >>>>> 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.
> > > > >>>>> Taking the M.2 NXP WIFI/BT module as an example,
> > > pin56(W_DISABLE1) is connected to the WIFI/BT chip PDn pin, we has
> > > already controlled this pin in the corresponding PCIe/SDIO controller dts
> nodes.
> > > > >>>>> It is not clear to me what exactly pins for vcc-supply and
> > > > >>>>> reset-gpios
> > > you describing here. Can you help understand the corresponding pins
> > > on M.2 interface as an example? Thanks.
> > > > >>>
> > > > >>> Hi Sherry,
> > > > >>>
> > > > >>> Regulators and reset controls being refcounted, we can then
> > > > >>> implement powerup sequence in both bluetooth/wlan drivers and
> > > > >>> have the drivers operate independently. This way bluetooth
> > > > >>> driver would has no dependance on the wlan driver for :
> > > > >>>
> > > > >>> - its power supply
> > > > >>>
> > > > >>> - its reset pin (PDn)
> > > > >>>
> > > > >>> - its firmware (being downloaded as part of the combo
> > > > >>> firmware)
> > > > >>>
> > > > >>> For the wlan driver we use mmc power sequence to drive the
> > > > >>> chip reset pin and there's another patchset that adds support
> > > > >>> for reset control into the mmc pwrseq simple driver.
> > > > >>>
> > > > >>>> Please wrap your replies.
> > > > >>>>
> > > > >>>> It seems you need power sequencing just like Bartosz did for
> > > Qualcomm WCN.
> > > > >>>
> > > > >>> Hi Krzysztof,
> > > > >>>
> > > > >>> I'm not familiar with power sequencing, but looks like way
> > > > >>> more complicated than reset controls. So, why power sequencing
> > > > >>> is recommended here ? Is it b/c a supply is involved ?
> > > > >>
> > > > >> 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
> >
> > Actually we should use vcc-supply to control the PDn pin, this is the
> > power supply for NXP wifi/BT.
> 
> Please don't since this is regular pin on the wlan/bt device not the regulator.
> People often do that for GPIOs if the driver is missing the support to pull the
> reset/pdn/enable gpio but the enable-gpio on the regulator is to enable the
> regulator and _not_ the bt/wlan device.
> 
> Therefore the implementation Catalin provided is the correct one.
> 

For NXP wifi/BT, the PDn is the only power control pin, no specific regulator, per my understanding,
it is a common way to configure this pin as the vcc-supply for the wifi interface(SDIO or PCIe).

reg_usdhc3_vmmc: regulator-usdhc3 {
         compatible = "regulator-fixed";
         regulator-name = "WLAN_EN";
         regulator-min-microvolt = <3300000>;
         regulator-max-microvolt = <3300000>;
         gpio = <&pcal6524 20 GPIO_ACTIVE_HIGH>;
         enable-active-high;
};

&usdhc3 {
...
      vmmc-supply = <&reg_usdhc3_vmmc>;
...
}

Best Regards
Sherry

Marco Felsch Oct. 22, 2024, 8:13 a.m. UTC | #16

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")

> >> };
> >>
> >> 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?

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

Regards,
  Marco

POPESCU Catalin Oct. 22, 2024, 8:21 a.m. UTC | #17

On 22/10/2024 10:09, Sherry Sun wrote:
> [收到此邮件的某些人通常不会收到来自 [e-mail] 的电子邮件。请访问 https://aka.ms/LearnAboutSenderIdentification，以了解为什么这一点很重要。]
>
> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>
>
>> -----Original Message-----
>> From: Marco Felsch <[e-mail]>
>> Sent: Tuesday, October 22, 2024 3:23 PM
>> To: Sherry Sun <[e-mail]>
>> Cc: POPESCU Catalin <[e-mail]>; Amitkumar
>> Karwar <[e-mail]>; Neeraj Sanjay Kale
>> <[e-mail]>; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; linux-
>> [e-mail]; [e-mail]; linux-
>> [e-mail]; GEO-CHHER-bsp-development <bsp-
>> [e-mail]>; Krzysztof Kozlowski
>> <[e-mail]>
>> Subject: Re: [PATCH 1/2] dt-bindings: net: bluetooth: nxp: add support for
>> supply and reset
>>
>> On 24-10-22, Sherry Sun wrote:
>>>> On 24-10-21, Krzysztof Kozlowski wrote:
>>>>> On 21/10/2024 08:41, Marco Felsch wrote:
>>>>>> On 24-10-07, Krzysztof Kozlowski wrote:
>>>>>>> On 07/10/2024 14:58, POPESCU Catalin wrote:
>>>>>>>>>>> +  vcc-supply:
>>>>>>>>>>> +    description:
>>>>>>>>>>> +      phandle of the regulator that provides the supply voltage.
>>>>>>>>>>> +
>>>>>>>>>>> +  reset-gpios:
>>>>>>>>>>> +    description:
>>>>>>>>>>> +      Chip powerdown/reset signal (PDn).
>>>>>>>>>>> +
>>>>>>>>>> Hi Catalin,
>>>>>>>>>>
>>>>>>>>>> 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.
>>>>>>>>>> Taking the M.2 NXP WIFI/BT module as an example,
>>>> pin56(W_DISABLE1) is connected to the WIFI/BT chip PDn pin, we has
>>>> already controlled this pin in the corresponding PCIe/SDIO controller dts
>> nodes.
>>>>>>>>>> It is not clear to me what exactly pins for vcc-supply and
>>>>>>>>>> reset-gpios
>>>> you describing here. Can you help understand the corresponding pins
>>>> on M.2 interface as an example? Thanks.
>>>>>>>> Hi Sherry,
>>>>>>>>
>>>>>>>> Regulators and reset controls being refcounted, we can then
>>>>>>>> implement powerup sequence in both bluetooth/wlan drivers and
>>>>>>>> have the drivers operate independently. This way bluetooth
>>>>>>>> driver would has no dependance on the wlan driver for :
>>>>>>>>
>>>>>>>> - its power supply
>>>>>>>>
>>>>>>>> - its reset pin (PDn)
>>>>>>>>
>>>>>>>> - its firmware (being downloaded as part of the combo
>>>>>>>> firmware)
>>>>>>>>
>>>>>>>> For the wlan driver we use mmc power sequence to drive the
>>>>>>>> chip reset pin and there's another patchset that adds support
>>>>>>>> for reset control into the mmc pwrseq simple driver.
>>>>>>>>
>>>>>>>>> Please wrap your replies.
>>>>>>>>>
>>>>>>>>> It seems you need power sequencing just like Bartosz did for
>>>> Qualcomm WCN.
>>>>>>>> Hi Krzysztof,
>>>>>>>>
>>>>>>>> I'm not familiar with power sequencing, but looks like way
>>>>>>>> more complicated than reset controls. So, why power sequencing
>>>>>>>> is recommended here ? Is it b/c a supply is involved ?
>>>>>>> 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
>>> Actually we should use vcc-supply to control the PDn pin, this is the
>>> power supply for NXP wifi/BT.
>> Please don't since this is regular pin on the wlan/bt device not the regulator.
>> People often do that for GPIOs if the driver is missing the support to pull the
>> reset/pdn/enable gpio but the enable-gpio on the regulator is to enable the
>> regulator and _not_ the bt/wlan device.
>>
>> Therefore the implementation Catalin provided is the correct one.
>>
> For NXP wifi/BT, the PDn is the only power control pin, no specific regulator, per my understanding,
> it is a common way to configure this pin as the vcc-supply for the wifi interface(SDIO or PCIe).
>
> reg_usdhc3_vmmc: regulator-usdhc3 {
>           compatible = "regulator-fixed";
>           regulator-name = "WLAN_EN";
>           regulator-min-microvolt = <3300000>;
>           regulator-max-microvolt = <3300000>;
>           gpio = <&pcal6524 20 GPIO_ACTIVE_HIGH>;
>           enable-active-high;
> };
>
> &usdhc3 {
> ...
>        vmmc-supply = <&reg_usdhc3_vmmc>;
> ...
> }
>
> Best Regards
> Sherry

Hi Sherry,

I'm sorry but  I have to disagree. I checked again block diagrams for 
IW611, IW612, IW416 which are available on NXP website and they clearly 
differentiate between power supply(s) and power-down. Power-down is 
actually a reset and should be treated as such in the DT, not as a 
supply regulator.

BR,

Catalin

Marco Felsch Oct. 22, 2024, 8:22 a.m. UTC | #18

On 24-10-22, Sherry Sun wrote:
> 
> 
> > -----Original Message-----
> > From: Marco Felsch <[e-mail]>
> > Sent: Tuesday, October 22, 2024 3:23 PM
> > To: Sherry Sun <[e-mail]>
> > Cc: POPESCU Catalin <[e-mail]>; Amitkumar
> > Karwar <[e-mail]>; Neeraj Sanjay Kale
> > <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; linux-
> > [e-mail]; [e-mail]; linux-
> > [e-mail]; GEO-CHHER-bsp-development <bsp-
> > [e-mail]>; Krzysztof Kozlowski
> > <[e-mail]>
> > Subject: Re: [PATCH 1/2] dt-bindings: net: bluetooth: nxp: add support for
> > supply and reset
> > 
> > On 24-10-22, Sherry Sun wrote:
> > > > On 24-10-21, Krzysztof Kozlowski wrote:
> > > > > On 21/10/2024 08:41, Marco Felsch wrote:
> > > > > > On 24-10-07, Krzysztof Kozlowski wrote:
> > > > > >> On 07/10/2024 14:58, POPESCU Catalin wrote:
> > > > > >>>>>>
> > > > > >>>>>> +  vcc-supply:
> > > > > >>>>>> +    description:
> > > > > >>>>>> +      phandle of the regulator that provides the supply voltage.
> > > > > >>>>>> +
> > > > > >>>>>> +  reset-gpios:
> > > > > >>>>>> +    description:
> > > > > >>>>>> +      Chip powerdown/reset signal (PDn).
> > > > > >>>>>> +
> > > > > >>>>> Hi Catalin,
> > > > > >>>>>
> > > > > >>>>> 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.
> > > > > >>>>> Taking the M.2 NXP WIFI/BT module as an example,
> > > > pin56(W_DISABLE1) is connected to the WIFI/BT chip PDn pin, we has
> > > > already controlled this pin in the corresponding PCIe/SDIO controller dts
> > nodes.
> > > > > >>>>> It is not clear to me what exactly pins for vcc-supply and
> > > > > >>>>> reset-gpios
> > > > you describing here. Can you help understand the corresponding pins
> > > > on M.2 interface as an example? Thanks.
> > > > > >>>
> > > > > >>> Hi Sherry,
> > > > > >>>
> > > > > >>> Regulators and reset controls being refcounted, we can then
> > > > > >>> implement powerup sequence in both bluetooth/wlan drivers and
> > > > > >>> have the drivers operate independently. This way bluetooth
> > > > > >>> driver would has no dependance on the wlan driver for :
> > > > > >>>
> > > > > >>> - its power supply
> > > > > >>>
> > > > > >>> - its reset pin (PDn)
> > > > > >>>
> > > > > >>> - its firmware (being downloaded as part of the combo
> > > > > >>> firmware)
> > > > > >>>
> > > > > >>> For the wlan driver we use mmc power sequence to drive the
> > > > > >>> chip reset pin and there's another patchset that adds support
> > > > > >>> for reset control into the mmc pwrseq simple driver.
> > > > > >>>
> > > > > >>>> Please wrap your replies.
> > > > > >>>>
> > > > > >>>> It seems you need power sequencing just like Bartosz did for
> > > > Qualcomm WCN.
> > > > > >>>
> > > > > >>> Hi Krzysztof,
> > > > > >>>
> > > > > >>> I'm not familiar with power sequencing, but looks like way
> > > > > >>> more complicated than reset controls. So, why power sequencing
> > > > > >>> is recommended here ? Is it b/c a supply is involved ?
> > > > > >>
> > > > > >> 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
> > >
> > > Actually we should use vcc-supply to control the PDn pin, this is the
> > > power supply for NXP wifi/BT.
> > 
> > Please don't since this is regular pin on the wlan/bt device not the regulator.
> > People often do that for GPIOs if the driver is missing the support to pull the
> > reset/pdn/enable gpio but the enable-gpio on the regulator is to enable the
> > regulator and _not_ the bt/wlan device.
> > 
> > Therefore the implementation Catalin provided is the correct one.
> > 
> 
> For NXP wifi/BT, the PDn is the only power control pin, no specific
> regulator, per my understanding, it is a common way to configure this
> pin as the vcc-supply for the wifi interface(SDIO or PCIe).

NACK. Each active external chip needs power, this is supplied via an
supply-rail and this is what vcc/vdd/va/vdio/v***-supply are used for.

The PDn is a digital input signal which tells the chip to go into
power-down/reset mode or not.

> reg_usdhc3_vmmc: regulator-usdhc3 {
>          compatible = "regulator-fixed";
>          regulator-name = "WLAN_EN";
>          regulator-min-microvolt = <3300000>;
>          regulator-max-microvolt = <3300000>;
>          gpio = <&pcal6524 20 GPIO_ACTIVE_HIGH>;
>          enable-active-high;
> };

This is what I meant previously, you do use a regualtor device for
switching the PDn signal. This is not correct, albeit a lot of people
are doing this because they don't want to adapt the driver. The 'gpio'
within this regualtor should enable/disable this particular physical
regualtor.

Regards,
  Marco

> &usdhc3 {
> ...
>       vmmc-supply = <&reg_usdhc3_vmmc>;
> ...
> }
> 
> Best Regards
> Sherry
>

Krzysztof Kozlowski Oct. 22, 2024, 8:23 a.m. UTC | #19

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

Sherry Sun Oct. 22, 2024, 2:24 p.m. UTC | #20

> -----Original Message-----
> From: Marco Felsch <[e-mail]>
> Sent: Tuesday, October 22, 2024 4:23 PM
> To: Sherry Sun <[e-mail]>
> Cc: POPESCU Catalin <[e-mail]>; Amitkumar
> Karwar <[e-mail]>; Neeraj Sanjay Kale
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail]; linux-
> [e-mail]; GEO-CHHER-bsp-development <bsp-
> [e-mail]>; Krzysztof Kozlowski
> <[e-mail]>
> Subject: Re: [PATCH 1/2] dt-bindings: net: bluetooth: nxp: add support for
> supply and reset
> 
> On 24-10-22, Sherry Sun wrote:
> >
> >
> > > -----Original Message-----
> > > From: Marco Felsch <[e-mail]>
> > > Sent: Tuesday, October 22, 2024 3:23 PM
> > > To: Sherry Sun <[e-mail]>
> > > Cc: POPESCU Catalin <[e-mail]>;
> > > Amitkumar Karwar <[e-mail]>; Neeraj Sanjay Kale
> > > <[e-mail]>; [e-mail];
> > > [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; linux-
> > > [e-mail]; [e-mail]; linux-
> > > [e-mail]; GEO-CHHER-bsp-development <bsp-
> > > [e-mail]>; Krzysztof Kozlowski
> > > <[e-mail]>
> > > Subject: Re: [PATCH 1/2] dt-bindings: net: bluetooth: nxp: add
> > > support for supply and reset
> > >
> > > On 24-10-22, Sherry Sun wrote:
> > > > > On 24-10-21, Krzysztof Kozlowski wrote:
> > > > > > On 21/10/2024 08:41, Marco Felsch wrote:
> > > > > > > On 24-10-07, Krzysztof Kozlowski wrote:
> > > > > > >> On 07/10/2024 14:58, POPESCU Catalin wrote:
> > > > > > >>>>>>
> > > > > > >>>>>> +  vcc-supply:
> > > > > > >>>>>> +    description:
> > > > > > >>>>>> +      phandle of the regulator that provides the supply
> voltage.
> > > > > > >>>>>> +
> > > > > > >>>>>> +  reset-gpios:
> > > > > > >>>>>> +    description:
> > > > > > >>>>>> +      Chip powerdown/reset signal (PDn).
> > > > > > >>>>>> +
> > > > > > >>>>> Hi Catalin,
> > > > > > >>>>>
> > > > > > >>>>> 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.
> > > > > > >>>>> Taking the M.2 NXP WIFI/BT module as an example,
> > > > > pin56(W_DISABLE1) is connected to the WIFI/BT chip PDn pin, we
> > > > > has already controlled this pin in the corresponding PCIe/SDIO
> > > > > controller dts
> > > nodes.
> > > > > > >>>>> It is not clear to me what exactly pins for vcc-supply
> > > > > > >>>>> and reset-gpios
> > > > > you describing here. Can you help understand the corresponding
> > > > > pins on M.2 interface as an example? Thanks.
> > > > > > >>>
> > > > > > >>> Hi Sherry,
> > > > > > >>>
> > > > > > >>> Regulators and reset controls being refcounted, we can
> > > > > > >>> then implement powerup sequence in both bluetooth/wlan
> > > > > > >>> drivers and have the drivers operate independently. This
> > > > > > >>> way bluetooth driver would has no dependance on the wlan
> driver for :
> > > > > > >>>
> > > > > > >>> - its power supply
> > > > > > >>>
> > > > > > >>> - its reset pin (PDn)
> > > > > > >>>
> > > > > > >>> - its firmware (being downloaded as part of the combo
> > > > > > >>> firmware)
> > > > > > >>>
> > > > > > >>> For the wlan driver we use mmc power sequence to drive the
> > > > > > >>> chip reset pin and there's another patchset that adds
> > > > > > >>> support for reset control into the mmc pwrseq simple driver.
> > > > > > >>>
> > > > > > >>>> Please wrap your replies.
> > > > > > >>>>
> > > > > > >>>> It seems you need power sequencing just like Bartosz did
> > > > > > >>>> for
> > > > > Qualcomm WCN.
> > > > > > >>>
> > > > > > >>> Hi Krzysztof,
> > > > > > >>>
> > > > > > >>> I'm not familiar with power sequencing, but looks like way
> > > > > > >>> more complicated than reset controls. So, why power
> > > > > > >>> sequencing is recommended here ? Is it b/c a supply is
> involved ?
> > > > > > >>
> > > > > > >> 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
> > > >
> > > > Actually we should use vcc-supply to control the PDn pin, this is
> > > > the power supply for NXP wifi/BT.
> > >
> > > Please don't since this is regular pin on the wlan/bt device not the
> regulator.
> > > People often do that for GPIOs if the driver is missing the support
> > > to pull the reset/pdn/enable gpio but the enable-gpio on the
> > > regulator is to enable the regulator and _not_ the bt/wlan device.
> > >
> > > Therefore the implementation Catalin provided is the correct one.
> > >
> >
> > For NXP wifi/BT, the PDn is the only power control pin, no specific
> > regulator, per my understanding, it is a common way to configure this
> > pin as the vcc-supply for the wifi interface(SDIO or PCIe).
> 
> NACK. Each active external chip needs power, this is supplied via an supply-
> rail and this is what vcc/vdd/va/vdio/v***-supply are used for.
> 
> The PDn is a digital input signal which tells the chip to go into power-
> down/reset mode or not.
> 
> > reg_usdhc3_vmmc: regulator-usdhc3 {
> >          compatible = "regulator-fixed";
> >          regulator-name = "WLAN_EN";
> >          regulator-min-microvolt = <3300000>;
> >          regulator-max-microvolt = <3300000>;
> >          gpio = <&pcal6524 20 GPIO_ACTIVE_HIGH>;
> >          enable-active-high;
> > };
> 
> This is what I meant previously, you do use a regualtor device for switching
> the PDn signal. This is not correct, albeit a lot of people are doing this
> because they don't want to adapt the driver. The 'gpio'
> within this regualtor should enable/disable this particular physical regualtor.
> 

Sorry I see it differently. I checked the datasheet of NXP wifi chip(taking IW612
as an example), the PDn pin is not the BT reset pin, we usually take it as the
PMIC_EN/WL_REG_ON pin to control the whole chip power supply.

I think the reset-gpio added here should control the IND_RST_BT pin
(Independent software reset for Bluetooth), similar for the 
IND_RST_WL pin(Independent software reset for Wi-Fi).

Best Regards
Sherry

POPESCU Catalin Oct. 22, 2024, 3:49 p.m. UTC | #21

On 22/10/2024 16:24, Sherry Sun wrote:
> [收到此邮件的某些人通常不会收到来自 [e-mail] 的电子邮件。请访问 https://aka.ms/LearnAboutSenderIdentification，以了解为什么这一点很重要。]
>
> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>
>
>> -----Original Message-----
>> From: Marco Felsch <[e-mail]>
>> Sent: Tuesday, October 22, 2024 4:23 PM
>> To: Sherry Sun <[e-mail]>
>> Cc: POPESCU Catalin <[e-mail]>; Amitkumar
>> Karwar <[e-mail]>; Neeraj Sanjay Kale
>> <[e-mail]>; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; linux-
>> [e-mail]; [e-mail]; linux-
>> [e-mail]; GEO-CHHER-bsp-development <bsp-
>> [e-mail]>; Krzysztof Kozlowski
>> <[e-mail]>
>> Subject: Re: [PATCH 1/2] dt-bindings: net: bluetooth: nxp: add support for
>> supply and reset
>>
>> On 24-10-22, Sherry Sun wrote:
>>>
>>>> -----Original Message-----
>>>> From: Marco Felsch <[e-mail]>
>>>> Sent: Tuesday, October 22, 2024 3:23 PM
>>>> To: Sherry Sun <[e-mail]>
>>>> Cc: POPESCU Catalin <[e-mail]>;
>>>> Amitkumar Karwar <[e-mail]>; Neeraj Sanjay Kale
>>>> <[e-mail]>; [e-mail];
>>>> [e-mail]; [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail]; linux-
>>>> [e-mail]; [e-mail]; linux-
>>>> [e-mail]; GEO-CHHER-bsp-development <bsp-
>>>> [e-mail]>; Krzysztof Kozlowski
>>>> <[e-mail]>
>>>> Subject: Re: [PATCH 1/2] dt-bindings: net: bluetooth: nxp: add
>>>> support for supply and reset
>>>>
>>>> On 24-10-22, Sherry Sun wrote:
>>>>>> On 24-10-21, Krzysztof Kozlowski wrote:
>>>>>>> On 21/10/2024 08:41, Marco Felsch wrote:
>>>>>>>> On 24-10-07, Krzysztof Kozlowski wrote:
>>>>>>>>> On 07/10/2024 14:58, POPESCU Catalin wrote:
>>>>>>>>>>>>> +  vcc-supply:
>>>>>>>>>>>>> +    description:
>>>>>>>>>>>>> +      phandle of the regulator that provides the supply
>> voltage.
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +  reset-gpios:
>>>>>>>>>>>>> +    description:
>>>>>>>>>>>>> +      Chip powerdown/reset signal (PDn).
>>>>>>>>>>>>> +
>>>>>>>>>>>> Hi Catalin,
>>>>>>>>>>>>
>>>>>>>>>>>> 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.
>>>>>>>>>>>> Taking the M.2 NXP WIFI/BT module as an example,
>>>>>> pin56(W_DISABLE1) is connected to the WIFI/BT chip PDn pin, we
>>>>>> has already controlled this pin in the corresponding PCIe/SDIO
>>>>>> controller dts
>>>> nodes.
>>>>>>>>>>>> It is not clear to me what exactly pins for vcc-supply
>>>>>>>>>>>> and reset-gpios
>>>>>> you describing here. Can you help understand the corresponding
>>>>>> pins on M.2 interface as an example? Thanks.
>>>>>>>>>> Hi Sherry,
>>>>>>>>>>
>>>>>>>>>> Regulators and reset controls being refcounted, we can
>>>>>>>>>> then implement powerup sequence in both bluetooth/wlan
>>>>>>>>>> drivers and have the drivers operate independently. This
>>>>>>>>>> way bluetooth driver would has no dependance on the wlan
>> driver for :
>>>>>>>>>> - its power supply
>>>>>>>>>>
>>>>>>>>>> - its reset pin (PDn)
>>>>>>>>>>
>>>>>>>>>> - its firmware (being downloaded as part of the combo
>>>>>>>>>> firmware)
>>>>>>>>>>
>>>>>>>>>> For the wlan driver we use mmc power sequence to drive the
>>>>>>>>>> chip reset pin and there's another patchset that adds
>>>>>>>>>> support for reset control into the mmc pwrseq simple driver.
>>>>>>>>>>
>>>>>>>>>>> Please wrap your replies.
>>>>>>>>>>>
>>>>>>>>>>> It seems you need power sequencing just like Bartosz did
>>>>>>>>>>> for
>>>>>> Qualcomm WCN.
>>>>>>>>>> Hi Krzysztof,
>>>>>>>>>>
>>>>>>>>>> I'm not familiar with power sequencing, but looks like way
>>>>>>>>>> more complicated than reset controls. So, why power
>>>>>>>>>> sequencing is recommended here ? Is it b/c a supply is
>> involved ?
>>>>>>>>> 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
>>>>> Actually we should use vcc-supply to control the PDn pin, this is
>>>>> the power supply for NXP wifi/BT.
>>>> Please don't since this is regular pin on the wlan/bt device not the
>> regulator.
>>>> People often do that for GPIOs if the driver is missing the support
>>>> to pull the reset/pdn/enable gpio but the enable-gpio on the
>>>> regulator is to enable the regulator and _not_ the bt/wlan device.
>>>>
>>>> Therefore the implementation Catalin provided is the correct one.
>>>>
>>> For NXP wifi/BT, the PDn is the only power control pin, no specific
>>> regulator, per my understanding, it is a common way to configure this
>>> pin as the vcc-supply for the wifi interface(SDIO or PCIe).
>> NACK. Each active external chip needs power, this is supplied via an supply-
>> rail and this is what vcc/vdd/va/vdio/v***-supply are used for.
>>
>> The PDn is a digital input signal which tells the chip to go into power-
>> down/reset mode or not.
>>
>>> reg_usdhc3_vmmc: regulator-usdhc3 {
>>>           compatible = "regulator-fixed";
>>>           regulator-name = "WLAN_EN";
>>>           regulator-min-microvolt = <3300000>;
>>>           regulator-max-microvolt = <3300000>;
>>>           gpio = <&pcal6524 20 GPIO_ACTIVE_HIGH>;
>>>           enable-active-high;
>>> };
>> This is what I meant previously, you do use a regualtor device for switching
>> the PDn signal. This is not correct, albeit a lot of people are doing this
>> because they don't want to adapt the driver. The 'gpio'
>> within this regualtor should enable/disable this particular physical regualtor.
>>
> Sorry I see it differently. I checked the datasheet of NXP wifi chip(taking IW612
> as an example), the PDn pin is not the BT reset pin, we usually take it as the
> PMIC_EN/WL_REG_ON pin to control the whole chip power supply.
>
> I think the reset-gpio added here should control the IND_RST_BT pin
> (Independent software reset for Bluetooth), similar for the
> IND_RST_WL pin(Independent software reset for Wi-Fi).
>
> Best Regards
> Sherry

PDn is not the BT reset :

- PDn is a chip reset and is shared b/w BT and WIFI : hence, it needs to 
be managed as a reset control

- BT reset is specific to BT and can be handled as a simple gpio as it's 
not being shared with other driver (e.g WIFI)

I've only added support for power-supply and PDn.

BT specific reset has been ignored so far as it's optional software 
reset and it can be left open if not needed in the design.

Sherry Sun Oct. 23, 2024, 2:16 p.m. UTC | #22

> >
> >> -----Original Message-----
> >> From: Marco Felsch <[e-mail]>
> >> Sent: Tuesday, October 22, 2024 4:23 PM
> >> To: Sherry Sun <[e-mail]>
> >> Cc: POPESCU Catalin <[e-mail]>;
> Amitkumar
> >> Karwar <[e-mail]>; Neeraj Sanjay Kale
> >> <[e-mail]>; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; linux-
> >> [e-mail]; [e-mail]; linux-
> >> [e-mail]; GEO-CHHER-bsp-development <bsp-
> >> [e-mail]>; Krzysztof Kozlowski
> >> <[e-mail]>
> >> Subject: Re: [PATCH 1/2] dt-bindings: net: bluetooth: nxp: add
> >> support for supply and reset
> >>
> >> On 24-10-22, Sherry Sun wrote:
> >>>
> >>>> -----Original Message-----
> >>>> From: Marco Felsch <[e-mail]>
> >>>> Sent: Tuesday, October 22, 2024 3:23 PM
> >>>> To: Sherry Sun <[e-mail]>
> >>>> Cc: POPESCU Catalin <[e-mail]>;
> >>>> Amitkumar Karwar <[e-mail]>; Neeraj Sanjay Kale
> >>>> <[e-mail]>; [e-mail];
> >>>> [e-mail]; [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail]; linux-
> >>>> [e-mail]; [e-mail]; linux-
> >>>> [e-mail]; GEO-CHHER-bsp-development <bsp-
> >>>> [e-mail]>; Krzysztof Kozlowski
> >>>> <[e-mail]>
> >>>> Subject: Re: [PATCH 1/2] dt-bindings: net: bluetooth: nxp: add
> >>>> support for supply and reset
> >>>>
> >>>> On 24-10-22, Sherry Sun wrote:
> >>>>>> On 24-10-21, Krzysztof Kozlowski wrote:
> >>>>>>> On 21/10/2024 08:41, Marco Felsch wrote:
> >>>>>>>> On 24-10-07, Krzysztof Kozlowski wrote:
> >>>>>>>>> On 07/10/2024 14:58, POPESCU Catalin wrote:
> >>>>>>>>>>>>> +  vcc-supply:
> >>>>>>>>>>>>> +    description:
> >>>>>>>>>>>>> +      phandle of the regulator that provides the supply
> >> voltage.
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> +  reset-gpios:
> >>>>>>>>>>>>> +    description:
> >>>>>>>>>>>>> +      Chip powerdown/reset signal (PDn).
> >>>>>>>>>>>>> +
> >>>>>>>>>>>> Hi Catalin,
> >>>>>>>>>>>>
> >>>>>>>>>>>> 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.
> >>>>>>>>>>>> Taking the M.2 NXP WIFI/BT module as an example,
> >>>>>> pin56(W_DISABLE1) is connected to the WIFI/BT chip PDn pin, we
> >>>>>> has already controlled this pin in the corresponding PCIe/SDIO
> >>>>>> controller dts
> >>>> nodes.
> >>>>>>>>>>>> It is not clear to me what exactly pins for vcc-supply and
> >>>>>>>>>>>> reset-gpios
> >>>>>> you describing here. Can you help understand the corresponding
> >>>>>> pins on M.2 interface as an example? Thanks.
> >>>>>>>>>> Hi Sherry,
> >>>>>>>>>>
> >>>>>>>>>> Regulators and reset controls being refcounted, we can then
> >>>>>>>>>> implement powerup sequence in both bluetooth/wlan drivers
> and
> >>>>>>>>>> have the drivers operate independently. This way bluetooth
> >>>>>>>>>> driver would has no dependance on the wlan
> >> driver for :
> >>>>>>>>>> - its power supply
> >>>>>>>>>>
> >>>>>>>>>> - its reset pin (PDn)
> >>>>>>>>>>
> >>>>>>>>>> - its firmware (being downloaded as part of the combo
> >>>>>>>>>> firmware)
> >>>>>>>>>>
> >>>>>>>>>> For the wlan driver we use mmc power sequence to drive the
> >>>>>>>>>> chip reset pin and there's another patchset that adds support
> >>>>>>>>>> for reset control into the mmc pwrseq simple driver.
> >>>>>>>>>>
> >>>>>>>>>>> Please wrap your replies.
> >>>>>>>>>>>
> >>>>>>>>>>> It seems you need power sequencing just like Bartosz did for
> >>>>>> Qualcomm WCN.
> >>>>>>>>>> Hi Krzysztof,
> >>>>>>>>>>
> >>>>>>>>>> I'm not familiar with power sequencing, but looks like way
> >>>>>>>>>> more complicated than reset controls. So, why power
> >>>>>>>>>> sequencing is recommended here ? Is it b/c a supply is
> >> involved ?
> >>>>>>>>> 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
> >>>>> Actually we should use vcc-supply to control the PDn pin, this is
> >>>>> the power supply for NXP wifi/BT.
> >>>> Please don't since this is regular pin on the wlan/bt device not
> >>>> the
> >> regulator.
> >>>> People often do that for GPIOs if the driver is missing the support
> >>>> to pull the reset/pdn/enable gpio but the enable-gpio on the
> >>>> regulator is to enable the regulator and _not_ the bt/wlan device.
> >>>>
> >>>> Therefore the implementation Catalin provided is the correct one.
> >>>>
> >>> For NXP wifi/BT, the PDn is the only power control pin, no specific
> >>> regulator, per my understanding, it is a common way to configure
> >>> this pin as the vcc-supply for the wifi interface(SDIO or PCIe).
> >> NACK. Each active external chip needs power, this is supplied via an
> >> supply- rail and this is what vcc/vdd/va/vdio/v***-supply are used for.
> >>
> >> The PDn is a digital input signal which tells the chip to go into
> >> power- down/reset mode or not.
> >>
> >>> reg_usdhc3_vmmc: regulator-usdhc3 {
> >>>           compatible = "regulator-fixed";
> >>>           regulator-name = "WLAN_EN";
> >>>           regulator-min-microvolt = <3300000>;
> >>>           regulator-max-microvolt = <3300000>;
> >>>           gpio = <&pcal6524 20 GPIO_ACTIVE_HIGH>;
> >>>           enable-active-high;
> >>> };
> >> This is what I meant previously, you do use a regualtor device for
> >> switching the PDn signal. This is not correct, albeit a lot of people
> >> are doing this because they don't want to adapt the driver. The 'gpio'
> >> within this regualtor should enable/disable this particular physical
> regualtor.
> >>
> > Sorry I see it differently. I checked the datasheet of NXP wifi
> > chip(taking IW612 as an example), the PDn pin is not the BT reset pin,
> > we usually take it as the PMIC_EN/WL_REG_ON pin to control the whole
> chip power supply.
> >
> > I think the reset-gpio added here should control the IND_RST_BT pin
> > (Independent software reset for Bluetooth), similar for the IND_RST_WL
> > pin(Independent software reset for Wi-Fi).
> >
> > Best Regards
> > Sherry
> 
> PDn is not the BT reset :
> 
> - PDn is a chip reset and is shared b/w BT and WIFI : hence, it needs to be
> managed as a reset control
> 

Ok, so can you please also send out the corresponding wifi driver changes
for the reset control for reference? Otherwise I suppose the wifi part will
be powered off unexpectedly if unload the BT driver with your patch.

Best Regards
Sherry

> - BT reset is specific to BT and can be handled as a simple gpio as it's not
> being shared with other driver (e.g WIFI)
> 
> I've only added support for power-supply and PDn.
> 
> BT specific reset has been ignored so far as it's optional software reset and it
> can be left open if not needed in the design.
>

POPESCU Catalin Oct. 23, 2024, 2:38 p.m. UTC | #23

On 23/10/2024 16:16, Sherry Sun wrote:
> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>
>
>>>> -----Original Message-----
>>>> From: Marco Felsch <[e-mail]>
>>>> Sent: Tuesday, October 22, 2024 4:23 PM
>>>> To: Sherry Sun <[e-mail]>
>>>> Cc: POPESCU Catalin <[e-mail]>;
>> Amitkumar
>>>> Karwar <[e-mail]>; Neeraj Sanjay Kale
>>>> <[e-mail]>; [e-mail];
>>>> [e-mail]; [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail]; linux-
>>>> [e-mail]; [e-mail]; linux-
>>>> [e-mail]; GEO-CHHER-bsp-development <bsp-
>>>> [e-mail]>; Krzysztof Kozlowski
>>>> <[e-mail]>
>>>> Subject: Re: [PATCH 1/2] dt-bindings: net: bluetooth: nxp: add
>>>> support for supply and reset
>>>>
>>>> On 24-10-22, Sherry Sun wrote:
>>>>>> -----Original Message-----
>>>>>> From: Marco Felsch <[e-mail]>
>>>>>> Sent: Tuesday, October 22, 2024 3:23 PM
>>>>>> To: Sherry Sun <[e-mail]>
>>>>>> Cc: POPESCU Catalin <[e-mail]>;
>>>>>> Amitkumar Karwar <[e-mail]>; Neeraj Sanjay Kale
>>>>>> <[e-mail]>; [e-mail];
>>>>>> [e-mail]; [e-mail]; [e-mail];
>>>>>> [e-mail]; [e-mail]; linux-
>>>>>> [e-mail]; [e-mail]; linux-
>>>>>> [e-mail]; GEO-CHHER-bsp-development <bsp-
>>>>>> [e-mail]>; Krzysztof Kozlowski
>>>>>> <[e-mail]>
>>>>>> Subject: Re: [PATCH 1/2] dt-bindings: net: bluetooth: nxp: add
>>>>>> support for supply and reset
>>>>>>
>>>>>> On 24-10-22, Sherry Sun wrote:
>>>>>>>> On 24-10-21, Krzysztof Kozlowski wrote:
>>>>>>>>> On 21/10/2024 08:41, Marco Felsch wrote:
>>>>>>>>>> On 24-10-07, Krzysztof Kozlowski wrote:
>>>>>>>>>>> On 07/10/2024 14:58, POPESCU Catalin wrote:
>>>>>>>>>>>>>>> +  vcc-supply:
>>>>>>>>>>>>>>> +    description:
>>>>>>>>>>>>>>> +      phandle of the regulator that provides the supply
>>>> voltage.
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +  reset-gpios:
>>>>>>>>>>>>>>> +    description:
>>>>>>>>>>>>>>> +      Chip powerdown/reset signal (PDn).
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> Hi Catalin,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 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.
>>>>>>>>>>>>>> Taking the M.2 NXP WIFI/BT module as an example,
>>>>>>>> pin56(W_DISABLE1) is connected to the WIFI/BT chip PDn pin, we
>>>>>>>> has already controlled this pin in the corresponding PCIe/SDIO
>>>>>>>> controller dts
>>>>>> nodes.
>>>>>>>>>>>>>> It is not clear to me what exactly pins for vcc-supply and
>>>>>>>>>>>>>> reset-gpios
>>>>>>>> you describing here. Can you help understand the corresponding
>>>>>>>> pins on M.2 interface as an example? Thanks.
>>>>>>>>>>>> Hi Sherry,
>>>>>>>>>>>>
>>>>>>>>>>>> Regulators and reset controls being refcounted, we can then
>>>>>>>>>>>> implement powerup sequence in both bluetooth/wlan drivers
>> and
>>>>>>>>>>>> have the drivers operate independently. This way bluetooth
>>>>>>>>>>>> driver would has no dependance on the wlan
>>>> driver for :
>>>>>>>>>>>> - its power supply
>>>>>>>>>>>>
>>>>>>>>>>>> - its reset pin (PDn)
>>>>>>>>>>>>
>>>>>>>>>>>> - its firmware (being downloaded as part of the combo
>>>>>>>>>>>> firmware)
>>>>>>>>>>>>
>>>>>>>>>>>> For the wlan driver we use mmc power sequence to drive the
>>>>>>>>>>>> chip reset pin and there's another patchset that adds support
>>>>>>>>>>>> for reset control into the mmc pwrseq simple driver.
>>>>>>>>>>>>
>>>>>>>>>>>>> Please wrap your replies.
>>>>>>>>>>>>>
>>>>>>>>>>>>> It seems you need power sequencing just like Bartosz did for
>>>>>>>> Qualcomm WCN.
>>>>>>>>>>>> Hi Krzysztof,
>>>>>>>>>>>>
>>>>>>>>>>>> I'm not familiar with power sequencing, but looks like way
>>>>>>>>>>>> more complicated than reset controls. So, why power
>>>>>>>>>>>> sequencing is recommended here ? Is it b/c a supply is
>>>> involved ?
>>>>>>>>>>> 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
>>>>>>> Actually we should use vcc-supply to control the PDn pin, this is
>>>>>>> the power supply for NXP wifi/BT.
>>>>>> Please don't since this is regular pin on the wlan/bt device not
>>>>>> the
>>>> regulator.
>>>>>> People often do that for GPIOs if the driver is missing the support
>>>>>> to pull the reset/pdn/enable gpio but the enable-gpio on the
>>>>>> regulator is to enable the regulator and _not_ the bt/wlan device.
>>>>>>
>>>>>> Therefore the implementation Catalin provided is the correct one.
>>>>>>
>>>>> For NXP wifi/BT, the PDn is the only power control pin, no specific
>>>>> regulator, per my understanding, it is a common way to configure
>>>>> this pin as the vcc-supply for the wifi interface(SDIO or PCIe).
>>>> NACK. Each active external chip needs power, this is supplied via an
>>>> supply- rail and this is what vcc/vdd/va/vdio/v***-supply are used for.
>>>>
>>>> The PDn is a digital input signal which tells the chip to go into
>>>> power- down/reset mode or not.
>>>>
>>>>> reg_usdhc3_vmmc: regulator-usdhc3 {
>>>>>            compatible = "regulator-fixed";
>>>>>            regulator-name = "WLAN_EN";
>>>>>            regulator-min-microvolt = <3300000>;
>>>>>            regulator-max-microvolt = <3300000>;
>>>>>            gpio = <&pcal6524 20 GPIO_ACTIVE_HIGH>;
>>>>>            enable-active-high;
>>>>> };
>>>> This is what I meant previously, you do use a regualtor device for
>>>> switching the PDn signal. This is not correct, albeit a lot of people
>>>> are doing this because they don't want to adapt the driver. The 'gpio'
>>>> within this regualtor should enable/disable this particular physical
>> regualtor.
>>> Sorry I see it differently. I checked the datasheet of NXP wifi
>>> chip(taking IW612 as an example), the PDn pin is not the BT reset pin,
>>> we usually take it as the PMIC_EN/WL_REG_ON pin to control the whole
>> chip power supply.
>>> I think the reset-gpio added here should control the IND_RST_BT pin
>>> (Independent software reset for Bluetooth), similar for the IND_RST_WL
>>> pin(Independent software reset for Wi-Fi).
>>>
>>> Best Regards
>>> Sherry
>> PDn is not the BT reset :
>>
>> - PDn is a chip reset and is shared b/w BT and WIFI : hence, it needs to be
>> managed as a reset control
>>
> Ok, so can you please also send out the corresponding wifi driver changes
> for the reset control for reference? Otherwise I suppose the wifi part will
> be powered off unexpectedly if unload the BT driver with your patch.
>
> Best Regards
> Sherry

We use the NXP downstream driver mwifiex which doesn't have support for 
regulator or PDn.

However, regulator is already supported by the MMC core (vmmc-supply).

For PDn, we use mmc pwrseq simple driver that has been patched to add 
support for reset-control.

Please check : 
https://lore.kernel.org/all/[e-mail]/

BR,

>
>> - BT reset is specific to BT and can be handled as a simple gpio as it's not
>> being shared with other driver (e.g WIFI)
>>
>> I've only added support for power-supply and PDn.
>>
>> BT specific reset has been ignored so far as it's optional software reset and it
>> can be left open if not needed in the design.
>>

Sherry Sun Oct. 28, 2024, 4:27 a.m. UTC | #24

> -----Original Message-----
> From: POPESCU Catalin <[e-mail]>
> Sent: Wednesday, October 23, 2024 10:38 PM
> To: Sherry Sun <[e-mail]>; Marco Felsch
> <[e-mail]>
> Cc: Amitkumar Karwar <[e-mail]>; Neeraj Sanjay Kale
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail]; linux-
> [e-mail]; GEO-CHHER-bsp-development <bsp-
> [e-mail]>; Krzysztof Kozlowski
> <[e-mail]>
> Subject: Re: [PATCH 1/2] dt-bindings: net: bluetooth: nxp: add support for
> supply and reset
>
> On 23/10/2024 16:16, Sherry Sun wrote:
> > This email is not from Hexagon's Office 365 instance. Please be careful while
> clicking links, opening attachments, or replying to this email.
> >
> >
> >>>> -----Original Message-----
> >>>> From: Marco Felsch <[e-mail]>
> >>>> Sent: Tuesday, October 22, 2024 4:23 PM
> >>>> To: Sherry Sun <[e-mail]>
> >>>> Cc: POPESCU Catalin <[e-mail]>;
> >> Amitkumar
> >>>> Karwar <[e-mail]>; Neeraj Sanjay Kale
> >>>> <[e-mail]>; [e-mail];
> >>>> [e-mail]; [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail]; linux-
> >>>> [e-mail]; [e-mail]; linux-
> >>>> [e-mail]; GEO-CHHER-bsp-development <bsp-
> >>>> [e-mail]>; Krzysztof Kozlowski
> >>>> <[e-mail]>
> >>>> Subject: Re: [PATCH 1/2] dt-bindings: net: bluetooth: nxp: add
> >>>> support for supply and reset
> >>>>
> >>>> On 24-10-22, Sherry Sun wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: Marco Felsch <[e-mail]>
> >>>>>> Sent: Tuesday, October 22, 2024 3:23 PM
> >>>>>> To: Sherry Sun <[e-mail]>
> >>>>>> Cc: POPESCU Catalin <[e-mail]>;
> >>>>>> Amitkumar Karwar <[e-mail]>; Neeraj Sanjay
> Kale
> >>>>>> <[e-mail]>; [e-mail];
> >>>>>> [e-mail]; [e-mail]; [e-mail];
> >>>>>> [e-mail]; [e-mail]; linux-
> >>>>>> [e-mail]; [e-mail]; linux-
> >>>>>> [e-mail]; GEO-CHHER-bsp-development <bsp-
> >>>>>> [e-mail]>; Krzysztof Kozlowski
> >>>>>> <[e-mail]>
> >>>>>> Subject: Re: [PATCH 1/2] dt-bindings: net: bluetooth: nxp: add
> >>>>>> support for supply and reset
> >>>>>>
> >>>>>> On 24-10-22, Sherry Sun wrote:
> >>>>>>>> On 24-10-21, Krzysztof Kozlowski wrote:
> >>>>>>>>> On 21/10/2024 08:41, Marco Felsch wrote:
> >>>>>>>>>> On 24-10-07, Krzysztof Kozlowski wrote:
> >>>>>>>>>>> On 07/10/2024 14:58, POPESCU Catalin wrote:
> >>>>>>>>>>>>>>> +  vcc-supply:
> >>>>>>>>>>>>>>> +    description:
> >>>>>>>>>>>>>>> +      phandle of the regulator that provides the supply
> >>>> voltage.
> >>>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>> +  reset-gpios:
> >>>>>>>>>>>>>>> +    description:
> >>>>>>>>>>>>>>> +      Chip powerdown/reset signal (PDn).
> >>>>>>>>>>>>>>> +
> >>>>>>>>>>>>>> Hi Catalin,
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> 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.
> >>>>>>>>>>>>>> Taking the M.2 NXP WIFI/BT module as an example,
> >>>>>>>> pin56(W_DISABLE1) is connected to the WIFI/BT chip PDn pin, we
> >>>>>>>> has already controlled this pin in the corresponding PCIe/SDIO
> >>>>>>>> controller dts
> >>>>>> nodes.
> >>>>>>>>>>>>>> It is not clear to me what exactly pins for vcc-supply
> >>>>>>>>>>>>>> and reset-gpios
> >>>>>>>> you describing here. Can you help understand the corresponding
> >>>>>>>> pins on M.2 interface as an example? Thanks.
> >>>>>>>>>>>> Hi Sherry,
> >>>>>>>>>>>>
> >>>>>>>>>>>> Regulators and reset controls being refcounted, we can then
> >>>>>>>>>>>> implement powerup sequence in both bluetooth/wlan drivers
> >> and
> >>>>>>>>>>>> have the drivers operate independently. This way bluetooth
> >>>>>>>>>>>> driver would has no dependance on the wlan
> >>>> driver for :
> >>>>>>>>>>>> - its power supply
> >>>>>>>>>>>>
> >>>>>>>>>>>> - its reset pin (PDn)
> >>>>>>>>>>>>
> >>>>>>>>>>>> - its firmware (being downloaded as part of the combo
> >>>>>>>>>>>> firmware)
> >>>>>>>>>>>>
> >>>>>>>>>>>> For the wlan driver we use mmc power sequence to drive the
> >>>>>>>>>>>> chip reset pin and there's another patchset that adds
> >>>>>>>>>>>> support for reset control into the mmc pwrseq simple driver.
> >>>>>>>>>>>>
> >>>>>>>>>>>>> Please wrap your replies.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> It seems you need power sequencing just like Bartosz did
> >>>>>>>>>>>>> for
> >>>>>>>> Qualcomm WCN.
> >>>>>>>>>>>> Hi Krzysztof,
> >>>>>>>>>>>>
> >>>>>>>>>>>> I'm not familiar with power sequencing, but looks like way
> >>>>>>>>>>>> more complicated than reset controls. So, why power
> >>>>>>>>>>>> sequencing is recommended here ? Is it b/c a supply is
> >>>> involved ?
> >>>>>>>>>>> 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
> >>>>>>> Actually we should use vcc-supply to control the PDn pin, this
> >>>>>>> is the power supply for NXP wifi/BT.
> >>>>>> Please don't since this is regular pin on the wlan/bt device not
> >>>>>> the
> >>>> regulator.
> >>>>>> People often do that for GPIOs if the driver is missing the
> >>>>>> support to pull the reset/pdn/enable gpio but the enable-gpio on
> >>>>>> the regulator is to enable the regulator and _not_ the bt/wlan device.
> >>>>>>
> >>>>>> Therefore the implementation Catalin provided is the correct one.
> >>>>>>
> >>>>> For NXP wifi/BT, the PDn is the only power control pin, no
> >>>>> specific regulator, per my understanding, it is a common way to
> >>>>> configure this pin as the vcc-supply for the wifi interface(SDIO or PCIe).
> >>>> NACK. Each active external chip needs power, this is supplied via
> >>>> an
> >>>> supply- rail and this is what vcc/vdd/va/vdio/v***-supply are used for.
> >>>>
> >>>> The PDn is a digital input signal which tells the chip to go into
> >>>> power- down/reset mode or not.
> >>>>
> >>>>> reg_usdhc3_vmmc: regulator-usdhc3 {
> >>>>>            compatible = "regulator-fixed";
> >>>>>            regulator-name = "WLAN_EN";
> >>>>>            regulator-min-microvolt = <3300000>;
> >>>>>            regulator-max-microvolt = <3300000>;
> >>>>>            gpio = <&pcal6524 20 GPIO_ACTIVE_HIGH>;
> >>>>>            enable-active-high;
> >>>>> };
> >>>> This is what I meant previously, you do use a regualtor device for
> >>>> switching the PDn signal. This is not correct, albeit a lot of
> >>>> people are doing this because they don't want to adapt the driver. The
> 'gpio'
> >>>> within this regualtor should enable/disable this particular
> >>>> physical
> >> regualtor.
> >>> Sorry I see it differently. I checked the datasheet of NXP wifi
> >>> chip(taking IW612 as an example), the PDn pin is not the BT reset
> >>> pin, we usually take it as the PMIC_EN/WL_REG_ON pin to control the
> >>> whole
> >> chip power supply.
> >>> I think the reset-gpio added here should control the IND_RST_BT pin
> >>> (Independent software reset for Bluetooth), similar for the
> >>> IND_RST_WL pin(Independent software reset for Wi-Fi).
> >>>
> >>> Best Regards
> >>> Sherry
> >> PDn is not the BT reset :
> >>
> >> - PDn is a chip reset and is shared b/w BT and WIFI : hence, it needs
> >> to be managed as a reset control
> >>
> > Ok, so can you please also send out the corresponding wifi driver
> > changes for the reset control for reference? Otherwise I suppose the
> > wifi part will be powered off unexpectedly if unload the BT driver with your
> patch.
> >
> > Best Regards
> > Sherry
>
> We use the NXP downstream driver mwifiex which doesn't have support for
> regulator or PDn.
>
> However, regulator is already supported by the MMC core (vmmc-supply).
>
> For PDn, we use mmc pwrseq simple driver that has been patched to add
> support for reset-control.
>
> Please check :
> https://lore.ke/
> rnel.org%2Fall%2F20241017131957.1171323-1-catalin.popescu%40leica-
> geosystems.com%2F&data=05%7C02%7Csherry.sun%40nxp.com%7C89459f5
> c23724f1bf16308dcf3704ce0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%
> 7C0%7C638652910876819305%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4
> wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7
> C%7C&sdata=6Kz7Fh1e%2F8iRZtTmb%2BHEWDeaTsG0D9tBa4TQjotZJwY%3D
> &reserved=0
>

Ok, thanks, the mmc change looks good for me, so there is no problem with the NXP SDIO wifi.
But how do you plan to handle the NXP PCIe wifi? We also need to make sure the BT patch won't break the PCIe wifi function.

Best Regards
Sherry

Marco Felsch Oct. 28, 2024, 9 a.m. UTC | #25

Hi,

On 24-10-28, Sherry Sun wrote:
> 
> > From: POPESCU Catalin <[e-mail]>
> >
> > We use the NXP downstream driver mwifiex which doesn't have support for
> > regulator or PDn.
> >
> > However, regulator is already supported by the MMC core (vmmc-supply).
> >
> > For PDn, we use mmc pwrseq simple driver that has been patched to add
> > support for reset-control.
> 
> Ok, thanks, the mmc change looks good for me, so there is no problem
> with the NXP SDIO wifi.
>
> But how do you plan to handle the NXP PCIe wifi? We also need to make
> sure the BT patch won't break the PCIe wifi function.

Can you please elaborate how this could break the PCIe use-case?

Regards,
  Marco

Sherry Sun Oct. 28, 2024, 9:59 a.m. UTC | #26

> -----Original Message-----
> From: Marco Felsch <[e-mail]>
> Sent: Monday, October 28, 2024 5:00 PM
> To: Sherry Sun <[e-mail]>
> Cc: POPESCU Catalin <[e-mail]>; Amitkumar
> Karwar <[e-mail]>; Neeraj Sanjay Kale
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail]; linux-
> [e-mail]; GEO-CHHER-bsp-development <bsp-
> [e-mail]>; Krzysztof Kozlowski
> <[e-mail]>
> Subject: Re: [PATCH 1/2] dt-bindings: net: bluetooth: nxp: add support for
> supply and reset
> 
> Hi,
> 
> On 24-10-28, Sherry Sun wrote:
> >
> > > From: POPESCU Catalin <[e-mail]>
> > >
> > > We use the NXP downstream driver mwifiex which doesn't have support
> > > for regulator or PDn.
> > >
> > > However, regulator is already supported by the MMC core (vmmc-supply).
> > >
> > > For PDn, we use mmc pwrseq simple driver that has been patched to
> > > add support for reset-control.
> >
> > Ok, thanks, the mmc change looks good for me, so there is no problem
> > with the NXP SDIO wifi.
> >
> > But how do you plan to handle the NXP PCIe wifi? We also need to make
> > sure the BT patch won't break the PCIe wifi function.
> 
> Can you please elaborate how this could break the PCIe use-case?

Similar to the SDIO wifi, if no corresponding reset control for the PDn pin in PCIe wifi driver, the wifi part will be unexpectedly powered off when removing the BT driver.

Best Regards
Sherry

Marco Felsch Oct. 28, 2024, 11:51 a.m. UTC | #27

On 24-10-28, Sherry Sun wrote:
> 
> > From: Marco Felsch <[e-mail]>
> > 
> > Hi,
> > 
> > On 24-10-28, Sherry Sun wrote:
> > >
> > > > From: POPESCU Catalin <[e-mail]>
> > > >
> > > > We use the NXP downstream driver mwifiex which doesn't have support
> > > > for regulator or PDn.
> > > >
> > > > However, regulator is already supported by the MMC core (vmmc-supply).
> > > >
> > > > For PDn, we use mmc pwrseq simple driver that has been patched to
> > > > add support for reset-control.
> > >
> > > Ok, thanks, the mmc change looks good for me, so there is no problem
> > > with the NXP SDIO wifi.
> > >
> > > But how do you plan to handle the NXP PCIe wifi? We also need to make
> > > sure the BT patch won't break the PCIe wifi function.
> > 
> > Can you please elaborate how this could break the PCIe use-case?
> 
> Similar to the SDIO wifi, if no corresponding reset control for the
> PDn pin in PCIe wifi driver, the wifi part will be unexpectedly
> powered off when removing the BT driver.

Nope it's not that easy for PCIe case since the phy + link layer
handling is much more complex compared to the MMC case. For the PCIe
case the intial handling is very strict according to the PCIe spec and
we can't handle the BT device independently.

_BUT_ this patch doesn't cause any regression for the PCIe use-case
since the support added by Catalin is optional which means that the user
don't have to use these options.

To sum up:

WLAN (PCIe) used + BT (UART) used -> no independent handling
                                     possible. BT depends on WLAN.

WLAN (PCIe) not used + BT (UART) used -> This patchset allow us to
                                         handle BT. Without the patchset
					 this is not possible.

WLAN (SDIO) + BT (UART) -> This patchset and the mmc-power-seq patchset
                           allow us to handle WLAN and BT independently
			   regardless if BT or WLAN is used or not.

Regards,
  Marco

Sherry Sun Oct. 28, 2024, 1:16 p.m. UTC | #28

> -----Original Message-----
> From: Marco Felsch <[e-mail]>
> Sent: Monday, October 28, 2024 7:52 PM
> To: Sherry Sun <[e-mail]>
> Cc: POPESCU Catalin <[e-mail]>; Amitkumar
> Karwar <[e-mail]>; Neeraj Sanjay Kale
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail]; linux-
> [e-mail]; GEO-CHHER-bsp-development <bsp-
> [e-mail]>; Krzysztof Kozlowski
> <[e-mail]>
> Subject: Re: [PATCH 1/2] dt-bindings: net: bluetooth: nxp: add support for
> supply and reset
> 
> On 24-10-28, Sherry Sun wrote:
> >
> > > From: Marco Felsch <[e-mail]>
> > >
> > > Hi,
> > >
> > > On 24-10-28, Sherry Sun wrote:
> > > >
> > > > > From: POPESCU Catalin <[e-mail]>
> > > > >
> > > > > We use the NXP downstream driver mwifiex which doesn't have
> > > > > support for regulator or PDn.
> > > > >
> > > > > However, regulator is already supported by the MMC core (vmmc-
> supply).
> > > > >
> > > > > For PDn, we use mmc pwrseq simple driver that has been patched
> > > > > to add support for reset-control.
> > > >
> > > > Ok, thanks, the mmc change looks good for me, so there is no
> > > > problem with the NXP SDIO wifi.
> > > >
> > > > But how do you plan to handle the NXP PCIe wifi? We also need to
> > > > make sure the BT patch won't break the PCIe wifi function.
> > >
> > > Can you please elaborate how this could break the PCIe use-case?
> >
> > Similar to the SDIO wifi, if no corresponding reset control for the
> > PDn pin in PCIe wifi driver, the wifi part will be unexpectedly
> > powered off when removing the BT driver.
> 
> Nope it's not that easy for PCIe case since the phy + link layer handling is
> much more complex compared to the MMC case. For the PCIe case the intial
> handling is very strict according to the PCIe spec and we can't handle the BT
> device independently.
> 
> _BUT_ this patch doesn't cause any regression for the PCIe use-case since the
> support added by Catalin is optional which means that the user don't have to
> use these options.
> 
> To sum up:
> 
> WLAN (PCIe) used + BT (UART) used -> no independent handling
>                                      possible. BT depends on WLAN.
> 
> WLAN (PCIe) not used + BT (UART) used -> This patchset allow us to
>                                          handle BT. Without the patchset
> 					 this is not possible.
> 
> WLAN (SDIO) + BT (UART) -> This patchset and the mmc-power-seq patchset
>                            allow us to handle WLAN and BT independently
> 			   regardless if BT or WLAN is used or not.
> 

If we add the reset-gpios property in the BT dts node when using the SDIO wifi chip, my concern is for some host platforms, taking i.MX95-19x19-EVK as an example, it supports both SDIO and PCIe interface wifi chip through the M.2 connector, when customers want to plug in the PCIe wifi chip, they have to remove the reset-gpios in the BT dts node to avoid the PCIe WLAN been affected by BT, right?

And it looks strange that we can only add the reset-gpios BT property to the hosts that only support SDIO WLAN, we hope there is a solution for the PCIe WLAN too.

Best Regards
Sherry

Marco Felsch Oct. 28, 2024, 3 p.m. UTC | #29

On 24-10-28, Sherry Sun wrote:
> 
> > From: Marco Felsch <[e-mail]>
> > 
> > On 24-10-28, Sherry Sun wrote:
> > >
> > > > From: Marco Felsch <[e-mail]>
> > > >
> > > > Hi,
> > > >
> > > > On 24-10-28, Sherry Sun wrote:
> > > > >
> > > > > > From: POPESCU Catalin <[e-mail]>
> > > > > >
> > > > > > We use the NXP downstream driver mwifiex which doesn't have
> > > > > > support for regulator or PDn.
> > > > > >
> > > > > > However, regulator is already supported by the MMC core (vmmc-
> > supply).
> > > > > >
> > > > > > For PDn, we use mmc pwrseq simple driver that has been patched
> > > > > > to add support for reset-control.
> > > > >
> > > > > Ok, thanks, the mmc change looks good for me, so there is no
> > > > > problem with the NXP SDIO wifi.
> > > > >
> > > > > But how do you plan to handle the NXP PCIe wifi? We also need to
> > > > > make sure the BT patch won't break the PCIe wifi function.
> > > >
> > > > Can you please elaborate how this could break the PCIe use-case?
> > >
> > > Similar to the SDIO wifi, if no corresponding reset control for the
> > > PDn pin in PCIe wifi driver, the wifi part will be unexpectedly
> > > powered off when removing the BT driver.
> > 
> > Nope it's not that easy for PCIe case since the phy + link layer handling is
> > much more complex compared to the MMC case. For the PCIe case the intial
> > handling is very strict according to the PCIe spec and we can't handle the BT
> > device independently.
> > 
> > _BUT_ this patch doesn't cause any regression for the PCIe use-case since the
> > support added by Catalin is optional which means that the user don't have to
> > use these options.
> > 
> > To sum up:
> > 
> > WLAN (PCIe) used + BT (UART) used -> no independent handling
> >                                      possible. BT depends on WLAN.
> > 
> > WLAN (PCIe) not used + BT (UART) used -> This patchset allow us to
> >                                          handle BT. Without the patchset
> > 					 this is not possible.
> > 
> > WLAN (SDIO) + BT (UART) -> This patchset and the mmc-power-seq patchset
> >                            allow us to handle WLAN and BT independently
> > 			   regardless if BT or WLAN is used or not.
> 
> If we add the reset-gpios property in the BT dts node when using the
> SDIO wifi chip, my concern is for some host platforms, taking
> i.MX95-19x19-EVK as an example, it supports both SDIO and PCIe
> interface wifi chip through the M.2 connector, when customers want to
> plug in the PCIe wifi chip, they have to remove the reset-gpios in the
> BT dts node to avoid the PCIe WLAN been affected by BT, right?

I don't know the i.MX95-19x19-EVK platform since it is not upstream. If
you want to support both:

> > WLAN (PCIe) used + BT (UART) used -> no independent handling
> >                                      possible. BT depends on WLAN.

and

> > WLAN (SDIO) + BT (UART) -> This patchset and the mmc-power-seq patchset
> >                            allow us to handle WLAN and BT independently
> > 			   regardless if BT or WLAN is used or not.

you need to stick with the dependent handling which is no problem once
this patchset get applied if your system support hot-plug. If hot-plug
is not possible you could consider unsing overlays.

However, this patchset does _NOT_ cause any regression neither for the
MMC nor the PCIe use-case, and you don't have to touch your DTS files. It
would be an improvement for platforms (not speaking of NXP EVK
platforms) which utilize the MMC+UART interfaces only.

> And it looks strange that we can only add the reset-gpios BT property
> to the hosts that only support SDIO WLAN, we hope there is a solution
> for the PCIe WLAN too.

"We hope there is a solution" <-- This is not how upstream work.

Also as said: The WLAN PCIe interface must/should be compatible with the
PCIe Spec. There is no way that we can handle both devices
independent since the PCIe spec specifies the power-up-sequence very
strict.

If for example, we do handle it independent and the BT part brings the
device out-of-reset while the PCIe bus is not yet ready, the device's
WLAN PCIe subsystem may get confused.

There are two solution NXP could provide:

 - The PCIe WLAN/BT devices exposes all devices WLAN + BT via PCIe, this
   would eliminate the UART part.
 - All new WLAN/BT devices do have a separate hw reset line for each
   radio the device supports.

Regards,
  Marco

Marco Felsch Nov. 18, 2024, 10:17 p.m. UTC | #30

Hi,

gentle ping on this discussion since I'm still convinced that this the
correct approach to add the reset mechanism and handle the power.

Regards,
  Marco

On 24-10-28, Marco Felsch wrote:
> On 24-10-28, Sherry Sun wrote:
> > 
> > > From: Marco Felsch <[e-mail]>
> > > 
> > > On 24-10-28, Sherry Sun wrote:
> > > >
> > > > > From: Marco Felsch <[e-mail]>
> > > > >
> > > > > Hi,
> > > > >
> > > > > On 24-10-28, Sherry Sun wrote:
> > > > > >
> > > > > > > From: POPESCU Catalin <[e-mail]>
> > > > > > >
> > > > > > > We use the NXP downstream driver mwifiex which doesn't have
> > > > > > > support for regulator or PDn.
> > > > > > >
> > > > > > > However, regulator is already supported by the MMC core (vmmc-
> > > supply).
> > > > > > >
> > > > > > > For PDn, we use mmc pwrseq simple driver that has been patched
> > > > > > > to add support for reset-control.
> > > > > >
> > > > > > Ok, thanks, the mmc change looks good for me, so there is no
> > > > > > problem with the NXP SDIO wifi.
> > > > > >
> > > > > > But how do you plan to handle the NXP PCIe wifi? We also need to
> > > > > > make sure the BT patch won't break the PCIe wifi function.
> > > > >
> > > > > Can you please elaborate how this could break the PCIe use-case?
> > > >
> > > > Similar to the SDIO wifi, if no corresponding reset control for the
> > > > PDn pin in PCIe wifi driver, the wifi part will be unexpectedly
> > > > powered off when removing the BT driver.
> > > 
> > > Nope it's not that easy for PCIe case since the phy + link layer handling is
> > > much more complex compared to the MMC case. For the PCIe case the intial
> > > handling is very strict according to the PCIe spec and we can't handle the BT
> > > device independently.
> > > 
> > > _BUT_ this patch doesn't cause any regression for the PCIe use-case since the
> > > support added by Catalin is optional which means that the user don't have to
> > > use these options.
> > > 
> > > To sum up:
> > > 
> > > WLAN (PCIe) used + BT (UART) used -> no independent handling
> > >                                      possible. BT depends on WLAN.
> > > 
> > > WLAN (PCIe) not used + BT (UART) used -> This patchset allow us to
> > >                                          handle BT. Without the patchset
> > > 					 this is not possible.
> > > 
> > > WLAN (SDIO) + BT (UART) -> This patchset and the mmc-power-seq patchset
> > >                            allow us to handle WLAN and BT independently
> > > 			   regardless if BT or WLAN is used or not.
> > 
> > If we add the reset-gpios property in the BT dts node when using the
> > SDIO wifi chip, my concern is for some host platforms, taking
> > i.MX95-19x19-EVK as an example, it supports both SDIO and PCIe
> > interface wifi chip through the M.2 connector, when customers want to
> > plug in the PCIe wifi chip, they have to remove the reset-gpios in the
> > BT dts node to avoid the PCIe WLAN been affected by BT, right?
> 
> I don't know the i.MX95-19x19-EVK platform since it is not upstream. If
> you want to support both:
> 
> > > WLAN (PCIe) used + BT (UART) used -> no independent handling
> > >                                      possible. BT depends on WLAN.
> 
> and
> 
> > > WLAN (SDIO) + BT (UART) -> This patchset and the mmc-power-seq patchset
> > >                            allow us to handle WLAN and BT independently
> > > 			   regardless if BT or WLAN is used or not.
> 
> you need to stick with the dependent handling which is no problem once
> this patchset get applied if your system support hot-plug. If hot-plug
> is not possible you could consider unsing overlays.
> 
> However, this patchset does _NOT_ cause any regression neither for the
> MMC nor the PCIe use-case, and you don't have to touch your DTS files. It
> would be an improvement for platforms (not speaking of NXP EVK
> platforms) which utilize the MMC+UART interfaces only.
> 
> > And it looks strange that we can only add the reset-gpios BT property
> > to the hosts that only support SDIO WLAN, we hope there is a solution
> > for the PCIe WLAN too.
> 
> "We hope there is a solution" <-- This is not how upstream work.
> 
> Also as said: The WLAN PCIe interface must/should be compatible with the
> PCIe Spec. There is no way that we can handle both devices
> independent since the PCIe spec specifies the power-up-sequence very
> strict.
> 
> If for example, we do handle it independent and the BT part brings the
> device out-of-reset while the PCIe bus is not yet ready, the device's
> WLAN PCIe subsystem may get confused.
> 
> There are two solution NXP could provide:
> 
>  - The PCIe WLAN/BT devices exposes all devices WLAN + BT via PCIe, this
>    would eliminate the UART part.
>  - All new WLAN/BT devices do have a separate hw reset line for each
>    radio the device supports.
> 
> Regards,
>   Marco

Sherry Sun Nov. 19, 2024, 10:09 a.m. UTC | #31

> -----Original Message-----
> From: Marco Felsch <[e-mail]>
> Sent: Tuesday, November 19, 2024 6:18 AM
> To: Sherry Sun <[e-mail]>
> Cc: POPESCU Catalin <[e-mail]>; Amitkumar
> Karwar <[e-mail]>; Neeraj Sanjay Kale
> <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; GEO-CHHER-bsp-
> development <[e-mail]>; Krzysztof
> Kozlowski <[e-mail]>
> Subject: Re: [PATCH 1/2] dt-bindings: net: bluetooth: nxp: add support for supply
> and reset
> 
> Hi,
> 
> gentle ping on this discussion since I'm still convinced that this the correct
> approach to add the reset mechanism and handle the power.

Hi Marco,

Sorry for the late reply. After internal discussion, we still have some confusion regarding this new feature.
This patch do improve the independent handling of wifi/BT, but with the controlling granularity segmentation, many different wifi/BT use cases need to be considered.
For the case -- WLAN (SDIO) not used + BT (UART) used:
The ideal behavior of BT should be reset and the standalone BT FW should be re-downloaded when unloading and re-loading the BT driver.
However, due to the regulator control and PDn reset control are bound to the SDIO bus instead of the WLAN device, the SDIO bus may be ready after kernel boot up. Although the WLAN is not used(WLAN driver is not loaded and WLAN FW is not downloaded), the corresponding regulator count and PDn reset count are both incremented by 1 through MMC pwrseq. Then with the BT driver remove & re-probe, the PDn reset cannot truly reset the BT chip due to the count been +1 by MMC pwrseq. So the BT will not reset and BT FW won't be re-downloaded when re-loading the BT driver, right?

Best Regards
Sherry
> 
> Regards,
>   Marco
> 
> On 24-10-28, Marco Felsch wrote:
> > On 24-10-28, Sherry Sun wrote:
> > >
> > > > From: Marco Felsch <[e-mail]>
> > > >
> > > > On 24-10-28, Sherry Sun wrote:
> > > > >
> > > > > > From: Marco Felsch <[e-mail]>
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > On 24-10-28, Sherry Sun wrote:
> > > > > > >
> > > > > > > > From: POPESCU Catalin
> > > > > > > > <[e-mail]>
> > > > > > > >
> > > > > > > > We use the NXP downstream driver mwifiex which doesn't
> > > > > > > > have support for regulator or PDn.
> > > > > > > >
> > > > > > > > However, regulator is already supported by the MMC core
> > > > > > > > (vmmc-
> > > > supply).
> > > > > > > >
> > > > > > > > For PDn, we use mmc pwrseq simple driver that has been
> > > > > > > > patched to add support for reset-control.
> > > > > > >
> > > > > > > Ok, thanks, the mmc change looks good for me, so there is no
> > > > > > > problem with the NXP SDIO wifi.
> > > > > > >
> > > > > > > But how do you plan to handle the NXP PCIe wifi? We also
> > > > > > > need to make sure the BT patch won't break the PCIe wifi function.
> > > > > >
> > > > > > Can you please elaborate how this could break the PCIe use-case?
> > > > >
> > > > > Similar to the SDIO wifi, if no corresponding reset control for
> > > > > the PDn pin in PCIe wifi driver, the wifi part will be
> > > > > unexpectedly powered off when removing the BT driver.
> > > >
> > > > Nope it's not that easy for PCIe case since the phy + link layer
> > > > handling is much more complex compared to the MMC case. For the
> > > > PCIe case the intial handling is very strict according to the PCIe
> > > > spec and we can't handle the BT device independently.
> > > >
> > > > _BUT_ this patch doesn't cause any regression for the PCIe
> > > > use-case since the support added by Catalin is optional which
> > > > means that the user don't have to use these options.
> > > >
> > > > To sum up:
> > > >
> > > > WLAN (PCIe) used + BT (UART) used -> no independent handling
> > > >                                      possible. BT depends on WLAN.
> > > >
> > > > WLAN (PCIe) not used + BT (UART) used -> This patchset allow us to
> > > >                                          handle BT. Without the patchset
> > > > 					 this is not possible.
> > > >
> > > > WLAN (SDIO) + BT (UART) -> This patchset and the mmc-power-seq
> patchset
> > > >                            allow us to handle WLAN and BT independently
> > > > 			   regardless if BT or WLAN is used or not.
> > >
> > > If we add the reset-gpios property in the BT dts node when using the
> > > SDIO wifi chip, my concern is for some host platforms, taking
> > > i.MX95-19x19-EVK as an example, it supports both SDIO and PCIe
> > > interface wifi chip through the M.2 connector, when customers want
> > > to plug in the PCIe wifi chip, they have to remove the reset-gpios
> > > in the BT dts node to avoid the PCIe WLAN been affected by BT, right?
> >
> > I don't know the i.MX95-19x19-EVK platform since it is not upstream.
> > If you want to support both:
> >
> > > > WLAN (PCIe) used + BT (UART) used -> no independent handling
> > > >                                      possible. BT depends on WLAN.
> >
> > and
> >
> > > > WLAN (SDIO) + BT (UART) -> This patchset and the mmc-power-seq
> patchset
> > > >                            allow us to handle WLAN and BT independently
> > > > 			   regardless if BT or WLAN is used or not.
> >
> > you need to stick with the dependent handling which is no problem once
> > this patchset get applied if your system support hot-plug. If hot-plug
> > is not possible you could consider unsing overlays.
> >
> > However, this patchset does _NOT_ cause any regression neither for the
> > MMC nor the PCIe use-case, and you don't have to touch your DTS files.
> > It would be an improvement for platforms (not speaking of NXP EVK
> > platforms) which utilize the MMC+UART interfaces only.
> >
> > > And it looks strange that we can only add the reset-gpios BT
> > > property to the hosts that only support SDIO WLAN, we hope there is
> > > a solution for the PCIe WLAN too.
> >
> > "We hope there is a solution" <-- This is not how upstream work.
> >
> > Also as said: The WLAN PCIe interface must/should be compatible with
> > the PCIe Spec. There is no way that we can handle both devices
> > independent since the PCIe spec specifies the power-up-sequence very
> > strict.
> >
> > If for example, we do handle it independent and the BT part brings the
> > device out-of-reset while the PCIe bus is not yet ready, the device's
> > WLAN PCIe subsystem may get confused.
> >
> > There are two solution NXP could provide:
> >
> >  - The PCIe WLAN/BT devices exposes all devices WLAN + BT via PCIe, this
> >    would eliminate the UART part.
> >  - All new WLAN/BT devices do have a separate hw reset line for each
> >    radio the device supports.
> >
> > Regards,
> >   Marco

Marco Felsch Nov. 19, 2024, 12:51 p.m. UTC | #32

On 24-11-19, Sherry Sun wrote:
> 
> > -----Original Message-----
> > From: Marco Felsch <[e-mail]>
> > 
> > Hi,
> > 
> > gentle ping on this discussion since I'm still convinced that this the correct
> > approach to add the reset mechanism and handle the power.
> 
> Hi Marco,
> 
> Sorry for the late reply. After internal discussion, we still have
> some confusion regarding this new feature.
> This patch do improve the independent handling of wifi/BT, but with
> the controlling granularity segmentation, many different wifi/BT use
> cases need to be considered.

Sure!

> For the case -- WLAN (SDIO) not used + BT (UART) used:
>
> The ideal behavior of BT should be reset and the standalone BT FW
> should be re-downloaded when unloading and re-loading the BT driver.

To make it clear, I assumed that it's clear that independent
(sub-)device handling require independent firmware (fw) files, which can
be the case. NXP already supplies independent FW files for bt and wifi.
We just need to ensure that the drivers are using these.

That said the bt driver already checks if the fw has to be downloaded.

> However, due to the regulator control and PDn reset control are bound
> to the SDIO bus instead of the WLAN device, the SDIO bus may be ready
> after kernel boot up.

Right, but this is a separate discussion not belonging to these driver
changes. Also it's the common chicken-egg issue. You need to power the
bus and release the device-reset before you can check which device is
connected and to check if there would be a proper driver.

> Although the WLAN is not used(WLAN driver is not loaded and WLAN FW is
> not downloaded), the corresponding regulator count and PDn reset count
> are both incremented by 1 through MMC pwrseq. Then with the BT driver
> remove & re-probe, the PDn reset cannot truly reset the BT chip due to
> the count been +1 by MMC pwrseq.  So the BT will not reset and BT FW
> won't be re-downloaded when re-loading the BT driver, right?

You're aware that the btnxpuart.c driver already has the support for an
independent software based reset? Not sure what this sw-reset does, due
to the lack of missing documentation, but this is the only option to
over-come your above mentioned use-case.

I have to ask, is this really a use-case for someone? Either your device
supports both: WLAN and BT or only one of WLAN/BT. If it would be only
BT or WLAN you just don't need the specify the other one within your
devicetree.

Furthermore, this patchset does not break any current use-case you/NXP
has. You still can use the combined fw version and still can use the not
so user friendly user-space dependency of: "wlan driver _must_ be
loaded" before the "bt driver _can_ be loaded" by just not using the
split power handling. For use-space which wants to use the split version
because there is no such dependecy.

Regards,
  Marco

Sherry Sun Nov. 20, 2024, 3:14 a.m. UTC | #33

> -----Original Message-----
> From: Marco Felsch <[e-mail]>
> Sent: Tuesday, November 19, 2024 8:52 PM
> To: Sherry Sun <[e-mail]>
> Cc: Bough Chen <[e-mail]>; POPESCU Catalin
> <[e-mail]>; Amitkumar Karwar
> <[e-mail]>; Neeraj Sanjay Kale
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail]; linux-
> [e-mail]; GEO-CHHER-bsp-development <bsp-
> [e-mail]>; Krzysztof Kozlowski
> <[e-mail]>; Shenwei Wang <[e-mail]>; Jun Li
> <[e-mail]>
> Subject: ttyRe: [PATCH 1/2] dt-bindings: net: bluetooth: nxp: add support for
> supply and reset
> 
> On 24-11-19, Sherry Sun wrote:
> >
> > > -----Original Message-----
> > > From: Marco Felsch <[e-mail]>
> > >
> > > Hi,
> > >
> > > gentle ping on this discussion since I'm still convinced that this
> > > the correct approach to add the reset mechanism and handle the power.
> >
> > Hi Marco,
> >
> > Sorry for the late reply. After internal discussion, we still have
> > some confusion regarding this new feature.
> > This patch do improve the independent handling of wifi/BT, but with
> > the controlling granularity segmentation, many different wifi/BT use
> > cases need to be considered.
> 
> Sure!
> 
> > For the case -- WLAN (SDIO) not used + BT (UART) used:
> >
> > The ideal behavior of BT should be reset and the standalone BT FW
> > should be re-downloaded when unloading and re-loading the BT driver.
> 
> To make it clear, I assumed that it's clear that independent (sub-)device
> handling require independent firmware (fw) files, which can be the case. NXP
> already supplies independent FW files for bt and wifi.
> We just need to ensure that the drivers are using these.
> 
> That said the bt driver already checks if the fw has to be downloaded.
> 
> > However, due to the regulator control and PDn reset control are bound
> > to the SDIO bus instead of the WLAN device, the SDIO bus may be ready
> > after kernel boot up.
> 
> Right, but this is a separate discussion not belonging to these driver changes.
> Also it's the common chicken-egg issue. You need to power the bus and
> release the device-reset before you can check which device is connected and
> to check if there would be a proper driver.
> 
> > Although the WLAN is not used(WLAN driver is not loaded and WLAN FW is
> > not downloaded), the corresponding regulator count and PDn reset count
> > are both incremented by 1 through MMC pwrseq. Then with the BT driver
> > remove & re-probe, the PDn reset cannot truly reset the BT chip due to
> > the count been +1 by MMC pwrseq.  So the BT will not reset and BT FW
> > won't be re-downloaded when re-loading the BT driver, right?
> 
> You're aware that the btnxpuart.c driver already has the support for an
> independent software based reset? Not sure what this sw-reset does, due to
> the lack of missing documentation, but this is the only option to over-come
> your above mentioned use-case.
> 
> I have to ask, is this really a use-case for someone? Either your device
> supports both: WLAN and BT or only one of WLAN/BT. If it would be only BT
> or WLAN you just don't need the specify the other one within your
> devicetree.

Hi Marco, 
I am not sure if this is a real use case for customers, we just thought of this possibility during internal discussions.
Currently our actual use case is to use both wlan & bt at the same time. Thanks for the clarification, now I am okay with this patch set.

Best Regards
Sherry

Neeraj Sanjay Kale Nov. 20, 2024, 4:44 a.m. UTC | #34

Hi Marco, Catalin,

Thank you for the patch.

> >
> > On 24-11-19, Sherry Sun wrote:
> > >
> > > > -----Original Message-----
> > > > From: Marco Felsch <[e-mail]>
> > > >
> > > > Hi,
> > > >
> > > > gentle ping on this discussion since I'm still convinced that this
> > > > the correct approach to add the reset mechanism and handle the
> power.
> > >
> > > Hi Marco,
> > >
> > > Sorry for the late reply. After internal discussion, we still have
> > > some confusion regarding this new feature.
> > > This patch do improve the independent handling of wifi/BT, but with
> > > the controlling granularity segmentation, many different wifi/BT use
> > > cases need to be considered.
> >
> > Sure!
> >
> > > For the case -- WLAN (SDIO) not used + BT (UART) used:
> > >
> > > The ideal behavior of BT should be reset and the standalone BT FW
> > > should be re-downloaded when unloading and re-loading the BT driver.
> >
> > To make it clear, I assumed that it's clear that independent
> > (sub-)device handling require independent firmware (fw) files, which
> > can be the case. NXP already supplies independent FW files for bt and wifi.
> > We just need to ensure that the drivers are using these.
> >
> > That said the bt driver already checks if the fw has to be downloaded.
> >
> > > However, due to the regulator control and PDn reset control are
> > > bound to the SDIO bus instead of the WLAN device, the SDIO bus may
> > > be ready after kernel boot up.
> >
> > Right, but this is a separate discussion not belonging to these driver
> changes.
> > Also it's the common chicken-egg issue. You need to power the bus and
> > release the device-reset before you can check which device is
> > connected and to check if there would be a proper driver.
> >
> > > Although the WLAN is not used(WLAN driver is not loaded and WLAN FW
> > > is not downloaded), the corresponding regulator count and PDn reset
> > > count are both incremented by 1 through MMC pwrseq. Then with the BT
> > > driver remove & re-probe, the PDn reset cannot truly reset the BT
> > > chip due to the count been +1 by MMC pwrseq.  So the BT will not
> > > reset and BT FW won't be re-downloaded when re-loading the BT driver,
> right?
> >
> > You're aware that the btnxpuart.c driver already has the support for
> > an independent software based reset? Not sure what this sw-reset does,
> > due to the lack of missing documentation, but this is the only option
> > to over-come your above mentioned use-case.
> >
> > I have to ask, is this really a use-case for someone? Either your
> > device supports both: WLAN and BT or only one of WLAN/BT. If it would
> > be only BT or WLAN you just don't need the specify the other one
> > within your devicetree.
> 
> Hi Marco,
> I am not sure if this is a real use case for customers, we just thought of this
> possibility during internal discussions.
> Currently our actual use case is to use both wlan & bt at the same time.
> Thanks for the clarification, now I am okay with this patch set.
> 
> Best Regards
> Sherry

I think the patch series is an improvement. It looks good to me.

Thanks,
Neeraj

Patch

diff --git a/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml b/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml
index 37a65badb448..8520b3812bd2 100644
--- a/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml
+++ b/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml
@@ -34,6 +34,14 @@  properties:
   firmware-name:
     maxItems: 1
 
+  vcc-supply:
+    description:
+      phandle of the regulator that provides the supply voltage.
+
+  reset-gpios:
+    description:
+      Chip powerdown/reset signal (PDn).
+
 required:
   - compatible
 
@@ -41,10 +49,13 @@  additionalProperties: false
 
 examples:
   - |
+    #include <dt-bindings/gpio/gpio.h>
     serial {
         bluetooth {
             compatible = "nxp,88w8987-bt";
             fw-init-baudrate = <3000000>;
             firmware-name = "uartuart8987_bt_v0.bin";
+            vcc-supply = <&nxp_iw612_supply>;
+            reset-gpios = <&gpioctrl 2 GPIO_ACTIVE_LOW>;
         };
     };