Message-ID:
 <DB9PR04MB8429CF700571FE42C997FB9C924D2@DB9PR04MB8429.eurprd04.prod.outlook.com>
Date: Wed, 23 Oct 2024 14:16:26 +0000
From: Sherry Sun <sherry.sun@....com>
To: POPESCU Catalin <catalin.popescu@...ca-geosystems.com>, Marco Felsch
	<m.felsch@...gutronix.de>
CC: 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
> 
> 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.
> 

Powered by blists - more mailing lists