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

[Marco Felsch <m.felsch@pengutronix.de>]

On 24-10-22, Sherry Sun wrote:
> 
> 
> > -----Original Message-----
> > From: Marco Felsch <m.felsch@pengutronix.de>
> > Sent: Tuesday, October 22, 2024 3:23 PM
> > To: Sherry Sun <sherry.sun@nxp.com>
> > Cc: POPESCU Catalin <catalin.popescu@leica-geosystems.com>; Amitkumar
> > Karwar <amitkumar.karwar@nxp.com>; Neeraj Sanjay Kale
> > <neeraj.sanjaykale@nxp.com>; marcel@holtmann.org;
> > luiz.dentz@gmail.com; robh@kernel.org; krzk+dt@kernel.org;
> > conor+dt@kernel.org; p.zabel@pengutronix.de; linux-
> > bluetooth@vger.kernel.org; devicetree@vger.kernel.org; linux-
> > kernel@vger.kernel.org; GEO-CHHER-bsp-development <bsp-
> > development.geo@leica-geosystems.com>; Krzysztof Kozlowski
> > <krzk@kernel.org>
> > 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
>