Message-ID: <1b8864e5-0ec7-49c4-932a-89cfbaeacc9f@leica-geosystems.com>
Date: Wed, 23 Oct 2024 14:38:01 +0000
From: POPESCU Catalin <catalin.popescu@...ca-geosystems.com>
To: Sherry Sun <sherry.sun@....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

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

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/20241017131957.1171323-1-catalin.popescu@leica-geosystems.com/

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

Powered by blists - more mailing lists