[PATCH v1 0/2] Bluetooth: btnxpuart: Add GPIO mechanism to

[PATCH v1 0/2] Bluetooth: btnxpuart: Add GPIO mechanism to

[Neeraj Sanjay Kale]

This patch series introduces a new optional device tree property
h2c-ps-gpio.

If this property is defined, the BTNXPUART driver uses this GPIO to put
the BT controller into sleep or wake it up.

The vendor command parameters HCI_NXP_WAKEUP_METHOD are configured to
use GPIO method instead of UART-break signal, to inform the chip about
this new mechanism.

Once power save feature is enabled, the driver puts the chip into power
save state by driving the GPIO high, and wakes it up by driving the GPIO
low.

Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>

Neeraj Sanjay Kale (2):
  dt-bindings: net: bluetooth: nxp: Add support for power save feature
    using GPIO
  Bluetooth: btnxpuart: Add GPIO support to power save feature

 .../net/bluetooth/nxp,88w8987-bt.yaml         |  7 ++++
 drivers/bluetooth/btnxpuart.c                 | 36 +++++++++++++++++--
 2 files changed, 41 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH v1 1/2] dt-bindings: net: bluetooth: nxp: Add support for power save feature using GPIO

[Neeraj Sanjay Kale]

This adds a new optional device tree property called h2c-ps-gpio.

If this property is defined, the driver will use this GPIO for driving chip
into sleep/wakeup state.

Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
---
 .../devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml  | 7 +++++++
 1 file changed, 7 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..e4eeee9bed68 100644
--- a/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml
+++ b/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml
@@ -34,6 +34,11 @@ properties:
   firmware-name:
     maxItems: 1
 
+  h2c-ps-gpio:
+    maxItems: 1
+    description:
+      Host-To-Chip power save mechanism is driven by this GPIO.
+
 required:
   - compatible
 
@@ -41,10 +46,12 @@ 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";
+            h2c-ps-gpio = <&gpio 11 GPIO_ACTIVE_HIGH>;
         };
     };
-- 
2.25.1

Re: [PATCH v1 1/2] dt-bindings: net: bluetooth: nxp: Add support for power save feature using GPIO

[Krzysztof Kozlowski]

On 01/10/2024 19:40, Neeraj Sanjay Kale wrote:
> This adds a new optional device tree property called h2c-ps-gpio.
> 
> If this property is defined, the driver will use this GPIO for driving chip
> into sleep/wakeup state.

You described the desired Linux feature or behavior, not the actual
hardware. The bindings are about the latter, so instead you need to
rephrase this to match actual hardware
capabilities/features/configuration etc.

> 
> Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
> ---
>  .../devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml  | 7 +++++++
>  1 file changed, 7 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..e4eeee9bed68 100644
> --- a/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml
> +++ b/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml
> @@ -34,6 +34,11 @@ properties:
>    firmware-name:
>      maxItems: 1
>  
> +  h2c-ps-gpio:
> +    maxItems: 1
> +    description:
> +      Host-To-Chip power save mechanism is driven by this GPIO.

Which pin is it?

> +



Best regards,
Krzysztof

Re: [PATCH v1 1/2] dt-bindings: net: bluetooth: nxp: Add support for power save feature using GPIO

[Rob Herring]

On Tue, Oct 01, 2024 at 11:10:20PM +0530, Neeraj Sanjay Kale wrote:
> This adds a new optional device tree property called h2c-ps-gpio.
> 
> If this property is defined, the driver will use this GPIO for driving chip
> into sleep/wakeup state.
> 
> Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
> ---
>  .../devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml  | 7 +++++++
>  1 file changed, 7 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..e4eeee9bed68 100644
> --- a/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml
> +++ b/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml
> @@ -34,6 +34,11 @@ properties:
>    firmware-name:
>      maxItems: 1
>  
> +  h2c-ps-gpio:

'-gpio' is deprecated. Use '-gpios'.

> +    maxItems: 1
> +    description:
> +      Host-To-Chip power save mechanism is driven by this GPIO.
> +
>  required:
>    - compatible
>  
> @@ -41,10 +46,12 @@ 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";
> +            h2c-ps-gpio = <&gpio 11 GPIO_ACTIVE_HIGH>;
>          };
>      };
> -- 
> 2.25.1
> 

[PATCH v1 2/2] Bluetooth: btnxpuart: Add GPIO support to power save feature

[Neeraj Sanjay Kale]

This adds support for driving the chip into sleep or wakeup with a GPIO.

If the device tree property h2c-ps-gpio is defined, the driver utilizes
this GPIO for controlling the chip's power save state, else it  uses the
default UART-break method.

Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
---
 drivers/bluetooth/btnxpuart.c | 36 +++++++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
index 2b8a07c745c9..327c86a0329c 100644
--- a/drivers/bluetooth/btnxpuart.c
+++ b/drivers/bluetooth/btnxpuart.c
@@ -16,6 +16,7 @@
 #include <linux/crc8.h>
 #include <linux/crc32.h>
 #include <linux/string_helpers.h>
+#include <linux/gpio/consumer.h>
 
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
@@ -82,6 +83,7 @@
 #define WAKEUP_METHOD_BREAK     1
 #define WAKEUP_METHOD_EXT_BREAK 2
 #define WAKEUP_METHOD_RTS       3
+#define WAKEUP_METHOD_GPIO      4
 #define WAKEUP_METHOD_INVALID   0xff
 
 /* power save mode status */
@@ -135,6 +137,7 @@ struct ps_data {
 	bool  driver_sent_cmd;
 	u16   h2c_ps_interval;
 	u16   c2h_ps_interval;
+	struct gpio_desc *h2c_ps_gpio;
 	struct hci_dev *hdev;
 	struct work_struct work;
 	struct timer_list ps_timer;
@@ -365,7 +368,7 @@ static void ps_control(struct hci_dev *hdev, u8 ps_state)
 {
 	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
 	struct ps_data *psdata = &nxpdev->psdata;
-	int status;
+	int status = 0;
 
 	if (psdata->ps_state == ps_state ||
 	    !test_bit(BTNXPUART_SERDEV_OPEN, &nxpdev->tx_state))
@@ -373,6 +376,12 @@ static void ps_control(struct hci_dev *hdev, u8 ps_state)
 
 	mutex_lock(&psdata->ps_lock);
 	switch (psdata->cur_h2c_wakeupmode) {
+	case WAKEUP_METHOD_GPIO:
+		if (ps_state == PS_STATE_AWAKE)
+			gpiod_set_value_cansleep(psdata->h2c_ps_gpio, 0);
+		else
+			gpiod_set_value_cansleep(psdata->h2c_ps_gpio, 1);
+		break;
 	case WAKEUP_METHOD_DTR:
 		if (ps_state == PS_STATE_AWAKE)
 			status = serdev_device_set_tiocm(nxpdev->serdev, TIOCM_DTR, 0);
@@ -425,8 +434,13 @@ static void ps_timeout_func(struct timer_list *t)
 static void ps_setup(struct hci_dev *hdev)
 {
 	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
+	struct serdev_device *serdev = nxpdev->serdev;
 	struct ps_data *psdata = &nxpdev->psdata;
 
+	psdata->h2c_ps_gpio = devm_gpiod_get(&serdev->dev, "h2c-ps", GPIOD_OUT_LOW);
+	if (IS_ERR(psdata->h2c_ps_gpio))
+		psdata->h2c_wakeup_gpio = 0xff;
+
 	psdata->hdev = hdev;
 	INIT_WORK(&psdata->work, ps_work_func);
 	mutex_init(&psdata->ps_lock);
@@ -516,6 +530,9 @@ static int send_wakeup_method_cmd(struct hci_dev *hdev, void *data)
 	pcmd.c2h_wakeupmode = psdata->c2h_wakeupmode;
 	pcmd.c2h_wakeup_gpio = psdata->c2h_wakeup_gpio;
 	switch (psdata->h2c_wakeupmode) {
+	case WAKEUP_METHOD_GPIO:
+		pcmd.h2c_wakeupmode = BT_CTRL_WAKEUP_METHOD_GPIO;
+		break;
 	case WAKEUP_METHOD_DTR:
 		pcmd.h2c_wakeupmode = BT_CTRL_WAKEUP_METHOD_DSR;
 		break;
@@ -550,6 +567,7 @@ static void ps_init(struct hci_dev *hdev)
 {
 	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
 	struct ps_data *psdata = &nxpdev->psdata;
+	u8 default_h2c_wakeup_mode = DEFAULT_H2C_WAKEUP_MODE;
 
 	serdev_device_set_tiocm(nxpdev->serdev, 0, TIOCM_RTS);
 	usleep_range(5000, 10000);
@@ -561,8 +579,19 @@ static void ps_init(struct hci_dev *hdev)
 	psdata->c2h_wakeup_gpio = 0xff;
 
 	psdata->cur_h2c_wakeupmode = WAKEUP_METHOD_INVALID;
+	if (!IS_ERR(psdata->h2c_ps_gpio))
+		default_h2c_wakeup_mode = WAKEUP_METHOD_GPIO;
+
 	psdata->h2c_ps_interval = PS_DEFAULT_TIMEOUT_PERIOD_MS;
-	switch (DEFAULT_H2C_WAKEUP_MODE) {
+
+	switch (default_h2c_wakeup_mode) {
+	case WAKEUP_METHOD_GPIO:
+		psdata->h2c_wakeupmode = WAKEUP_METHOD_GPIO;
+		gpiod_set_value_cansleep(psdata->h2c_ps_gpio, 1);
+		usleep_range(5000, 10000);
+		gpiod_set_value_cansleep(psdata->h2c_ps_gpio, 0);
+		usleep_range(5000, 10000);
+		break;
 	case WAKEUP_METHOD_DTR:
 		psdata->h2c_wakeupmode = WAKEUP_METHOD_DTR;
 		serdev_device_set_tiocm(nxpdev->serdev, 0, TIOCM_DTR);
@@ -1279,6 +1308,9 @@ static int nxp_enqueue(struct hci_dev *hdev, struct sk_buff *skb)
 				psdata->c2h_wakeup_gpio = wakeup_parm.c2h_wakeup_gpio;
 				psdata->h2c_wakeup_gpio = wakeup_parm.h2c_wakeup_gpio;
 				switch (wakeup_parm.h2c_wakeupmode) {
+				case BT_CTRL_WAKEUP_METHOD_GPIO:
+					psdata->h2c_wakeupmode = WAKEUP_METHOD_GPIO;
+					break;
 				case BT_CTRL_WAKEUP_METHOD_DSR:
 					psdata->h2c_wakeupmode = WAKEUP_METHOD_DTR;
 					break;
-- 
2.25.1

RE: [PATCH v1 2/2] Bluetooth: btnxpuart: Add GPIO support to power save feature

[Shenwei Wang]

> -----Original Message-----
> From: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
> Sent: Tuesday, October 1, 2024 12:40 PM
> To: marcel@holtmann.org; luiz.dentz@gmail.com; robh@kernel.org;
> krzk+dt@kernel.org; conor+dt@kernel.org
> Cc: linux-bluetooth@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; Amitkumar Karwar <amitkumar.karwar@nxp.com>;
> Rohit Fule <rohit.fule@nxp.com>; Neeraj Sanjay Kale
> <neeraj.sanjaykale@nxp.com>; Sherry Sun <sherry.sun@nxp.com>; Luke Wang
> <ziniu.wang_1@nxp.com>; Bough Chen <haibo.chen@nxp.com>; LnxRevLi
> <LnxRevLi@nxp.com>
> Subject: [PATCH v1 2/2] Bluetooth: btnxpuart: Add GPIO support to power save
> feature
> 
> This adds support for driving the chip into sleep or wakeup with a GPIO.
> 
> If the device tree property h2c-ps-gpio is defined, the driver utilizes this GPIO for
> controlling the chip's power save state, else it  uses the default UART-break
> method.
> 
> Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
> ---
>  drivers/bluetooth/btnxpuart.c | 36 +++++++++++++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
> 
>  	switch (psdata->h2c_wakeupmode) {
> +	case WAKEUP_METHOD_GPIO:
> +		pcmd.h2c_wakeupmode = BT_CTRL_WAKEUP_METHOD_GPIO;
> +		break;
>  	case WAKEUP_METHOD_DTR:
>  		pcmd.h2c_wakeupmode = BT_CTRL_WAKEUP_METHOD_DSR;
>  		break;
>  	psdata->h2c_ps_interval = PS_DEFAULT_TIMEOUT_PERIOD_MS;
> -	switch (DEFAULT_H2C_WAKEUP_MODE) {
> +
> +	switch (default_h2c_wakeup_mode) {
> +	case WAKEUP_METHOD_GPIO:
> +		psdata->h2c_wakeupmode = WAKEUP_METHOD_GPIO;
> +		gpiod_set_value_cansleep(psdata->h2c_ps_gpio, 1);
> +		usleep_range(5000, 10000);
> +		gpiod_set_value_cansleep(psdata->h2c_ps_gpio, 0);
> +		usleep_range(5000, 10000);
> +		break;

Based on the above GPIO operation sequences, it indicates that the target device likely responds to a 
falling edge (transition from high to low) as its wakeup trigger, is it?

In the cover letter, you mentioned " the driver puts the chip into power save state by driving the GPIO high, 
and wakes it up by driving the GPIO low". Seems the expected behavior is a level trigger.

This appears to be a discrepancy between the code implementation and the description in the cover letter 
regarding the wakeup mechanism. Can you please clarify it?

Thanks,
Shenwei

>  	case WAKEUP_METHOD_DTR:
>  		psdata->h2c_wakeupmode = WAKEUP_METHOD_DTR;
>  		serdev_device_set_tiocm(nxpdev->serdev, 0, TIOCM_DTR); @@
> -1279,6 +1308,9 @@ static int nxp_enqueue(struct hci_dev *hdev, struct sk_buff
> *skb)
>  				psdata->c2h_wakeup_gpio =
> wakeup_parm.c2h_wakeup_gpio;
>  				psdata->h2c_wakeup_gpio =
> wakeup_parm.h2c_wakeup_gpio;
>  				switch (wakeup_parm.h2c_wakeupmode) {
> +				case BT_CTRL_WAKEUP_METHOD_GPIO:
> +					psdata->h2c_wakeupmode =
> WAKEUP_METHOD_GPIO;
> +					break;
>  				case BT_CTRL_WAKEUP_METHOD_DSR:
>  					psdata->h2c_wakeupmode =
> WAKEUP_METHOD_DTR;
>  					break;
> --
> 2.25.1

RE: [PATCH v1 2/2] Bluetooth: btnxpuart: Add GPIO support to power save feature

[Neeraj Sanjay Kale]

Hi Shenwei,

Thank you for the review.

> >
> > This adds support for driving the chip into sleep or wakeup with a GPIO.
> >
> > If the device tree property h2c-ps-gpio is defined, the driver
> > utilizes this GPIO for controlling the chip's power save state, else
> > it  uses the default UART-break method.
> >
> > Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
> > ---
> >  drivers/bluetooth/btnxpuart.c | 36
> > +++++++++++++++++++++++++++++++++--
> >  1 file changed, 34 insertions(+), 2 deletions(-)
> >
> >  	switch (psdata->h2c_wakeupmode) {
> > +	case WAKEUP_METHOD_GPIO:
> > +		pcmd.h2c_wakeupmode =
> BT_CTRL_WAKEUP_METHOD_GPIO;
> > +		break;
> >  	case WAKEUP_METHOD_DTR:
> >  		pcmd.h2c_wakeupmode = BT_CTRL_WAKEUP_METHOD_DSR;
> >  		break;
> >  	psdata->h2c_ps_interval = PS_DEFAULT_TIMEOUT_PERIOD_MS;
> > -	switch (DEFAULT_H2C_WAKEUP_MODE) {
> > +
> > +	switch (default_h2c_wakeup_mode) {
> > +	case WAKEUP_METHOD_GPIO:
> > +		psdata->h2c_wakeupmode = WAKEUP_METHOD_GPIO;
> > +		gpiod_set_value_cansleep(psdata->h2c_ps_gpio, 1);
> > +		usleep_range(5000, 10000);
> > +		gpiod_set_value_cansleep(psdata->h2c_ps_gpio, 0);
> > +		usleep_range(5000, 10000);
> > +		break;
> 
> Based on the above GPIO operation sequences, it indicates that the target
> device likely responds to a falling edge (transition from high to low) as its
> wakeup trigger, is it?
> 
> In the cover letter, you mentioned " the driver puts the chip into power save
> state by driving the GPIO high, and wakes it up by driving the GPIO low".
> Seems the expected behavior is a level trigger.
> 
> This appears to be a discrepancy between the code implementation and the
> description in the cover letter regarding the wakeup mechanism. Can you
> please clarify it?
> 
> Thanks,
> Shenwei

The expected behavior is level trigger.
The piece of code you are referring to is from power save init, where we are setting the initial value of GPIO as HIGH.
However, if the FW is already present and running, with unknown power save state, a GPIO toggle ensures the chip wakes up, and FW and driver are in sync.

Thanks,
Neeraj

RE: [PATCH v1 2/2] Bluetooth: btnxpuart: Add GPIO support to power save feature

[Shenwei Wang]

> -----Original Message-----
> From: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
> Sent: Thursday, October 3, 2024 10:38 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>; marcel@holtmann.org;
> luiz.dentz@gmail.com; robh@kernel.org; krzk+dt@kernel.org;
> conor+dt@kernel.org
> Cc: linux-bluetooth@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; Amitkumar Karwar <amitkumar.karwar@nxp.com>;
> Rohit Fule <rohit.fule@nxp.com>; Sherry Sun <sherry.sun@nxp.com>; Luke Wang
> <ziniu.wang_1@nxp.com>; Bough Chen <haibo.chen@nxp.com>; LnxRevLi
> <LnxRevLi@nxp.com>
> Subject: RE: [PATCH v1 2/2] Bluetooth: btnxpuart: Add GPIO support to power
> save feature
> 
> The expected behavior is level trigger.
> The piece of code you are referring to is from power save init, where we are
> setting the initial value of GPIO as HIGH.
> However, if the FW is already present and running, with unknown power save
> state, a GPIO toggle ensures the chip wakes up, and FW and driver are in sync.
> 

If the module is already in a power-save state, waking it up only to immediately 
return it to power-save seems unnecessary. A more efficient approach would be 
to simply set the GPIO to LOW. This action should transition the module into a 
power-save state regardless of its previous condition.

Regards,
Shenwei

> Thanks,
> Neeraj

RE: [PATCH v1 2/2] Bluetooth: btnxpuart: Add GPIO support to power save feature

[Neeraj Sanjay Kale]

Hi Shenwei,

> >
> > The expected behavior is level trigger.
> > The piece of code you are referring to is from power save init, where
> > we are setting the initial value of GPIO as HIGH.
> > However, if the FW is already present and running, with unknown power
> > save state, a GPIO toggle ensures the chip wakes up, and FW and driver are
> in sync.
> >
> 
> If the module is already in a power-save state, waking it up only to
> immediately return it to power-save seems unnecessary. A more efficient
> approach would be to simply set the GPIO to LOW. This action should
> transition the module into a power-save state regardless of its previous
> condition.
> 
Yes, it seems more efficient. Let me make the change and test.
Ps_init() is expected to make the GPIO low to wake up the chip.
Will include the change in v3 patch.

Correction to my previous reply: 
> > The piece of code you are referring to is from power save init, where
> > we are setting the initial value of GPIO as "LOW".

Thanks,
Neeraj