Message-ID:
 <DB9PR04MB84292445D0FEDB8211ED52C3924C2@DB9PR04MB8429.eurprd04.prod.outlook.com>
Date: Tue, 22 Oct 2024 14:24:17 +0000
From: Sherry Sun <sherry.sun@....com>
To: Marco Felsch <m.felsch@...gutronix.de>
CC: POPESCU Catalin <catalin.popescu@...ca-geosystems.com>, Amitkumar Karwar
	<amitkumar.karwar@....com>, Neeraj Sanjay Kale <neeraj.sanjaykale@....com>,
	"marcel@...tmann.org" <marcel@...tmann.org>, "luiz.dentz@...il.com"
	<luiz.dentz@...il.com>, "robh@...nel.org" <robh@...nel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "conor+dt@...nel.org"
	<conor+dt@...nel.org>, "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
	"linux-bluetooth@...r.kernel.org" <linux-bluetooth@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	GEO-CHHER-bsp-development <bsp-development.geo@...ca-geosystems.com>,
	Krzysztof Kozlowski <krzk@...nel.org>
Subject: RE: [PATCH 1/2] dt-bindings: net: bluetooth: nxp: add support for
 supply and reset



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

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

Powered by blists - more mailing lists