[PATCH 1/2] bluetooth: btnxpuart: Support for controller wakeup gpio config

[PATCH 1/2] bluetooth: btnxpuart: Support for controller wakeup gpio config

[Loic Poulain]

When using the out-of-band WAKE_IN and WAKE_OUT pins, we have to tell
the firmware which pins to use (from controller point of view). This
allows to report remote wakeup support when WAKE_OUT(c2h) is configured.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/bluetooth/btnxpuart.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
index aa5ec1d444a9..6fbb8daf6f05 100644
--- a/drivers/bluetooth/btnxpuart.c
+++ b/drivers/bluetooth/btnxpuart.c
@@ -540,9 +540,11 @@ 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;
+	pcmd.h2c_wakeup_gpio = 0xff;
 	switch (psdata->h2c_wakeupmode) {
 	case WAKEUP_METHOD_GPIO:
 		pcmd.h2c_wakeupmode = BT_CTRL_WAKEUP_METHOD_GPIO;
+		pcmd.h2c_wakeup_gpio = psdata->h2c_wakeup_gpio;
 		break;
 	case WAKEUP_METHOD_DTR:
 		pcmd.h2c_wakeupmode = BT_CTRL_WAKEUP_METHOD_DSR;
@@ -552,7 +554,6 @@ static int send_wakeup_method_cmd(struct hci_dev *hdev, void *data)
 		pcmd.h2c_wakeupmode = BT_CTRL_WAKEUP_METHOD_BREAK;
 		break;
 	}
-	pcmd.h2c_wakeup_gpio = 0xff;
 
 	skb = nxp_drv_send_cmd(hdev, HCI_NXP_WAKEUP_METHOD, sizeof(pcmd), &pcmd);
 	if (IS_ERR(skb)) {
@@ -616,6 +617,13 @@ static void ps_init(struct hci_dev *hdev)
 		break;
 	}
 
+	if (!device_property_read_u8(&nxpdev->serdev->dev, "nxp,wakein-pin",
+				     &psdata->h2c_wakeup_gpio))
+		psdata->h2c_wakeupmode = WAKEUP_METHOD_GPIO;
+	if (!device_property_read_u8(&nxpdev->serdev->dev, "nxp,wakeout-pin",
+				     &psdata->c2h_wakeup_gpio))
+		psdata->c2h_wakeupmode = BT_HOST_WAKEUP_METHOD_GPIO;
+
 	psdata->cur_psmode = PS_MODE_DISABLE;
 	psdata->target_ps_mode = DEFAULT_PS_MODE;
 
@@ -1266,6 +1274,17 @@ static int nxp_shutdown(struct hci_dev *hdev)
 	return 0;
 }
 
+static bool nxp_wakeup(struct hci_dev *hdev)
+{
+	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
+	struct ps_data *psdata = &nxpdev->psdata;
+
+	if (psdata->c2h_wakeupmode != BT_HOST_WAKEUP_METHOD_NONE)
+		return true;
+
+	return false;
+}
+
 static int btnxpuart_queue_skb(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
@@ -1546,6 +1565,7 @@ static int nxp_serdev_probe(struct serdev_device *serdev)
 	hdev->send  = nxp_enqueue;
 	hdev->hw_error = nxp_hw_err;
 	hdev->shutdown = nxp_shutdown;
+	hdev->wakeup = nxp_wakeup;
 	SET_HCIDEV_DEV(hdev, &serdev->dev);
 
 	if (hci_register_dev(hdev) < 0) {
-- 
2.34.1

[PATCH 2/2] dt-bindings: net: bluetooth: nxp: Add wakeup pin properties

[Loic Poulain]

NXP bluetooth controller may have GPIO pins used and routed for `WAKE_IN`
and `WAKE_OUT`, such pin info must be known so that the driver is can
configure the controller's firmware accordingly.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 .../bindings/net/bluetooth/nxp,88w8987-bt.yaml       | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml b/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml
index 0a2d7baf5db3..04f55fac42ce 100644
--- a/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml
+++ b/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml
@@ -40,6 +40,16 @@ properties:
       Host-To-Chip power save mechanism is driven by this GPIO
       connected to BT_WAKE_IN pin of the NXP chipset.
 
+  nxp,wakein-pin:
+    $ref: /schemas/types.yaml#/definitions/uint8
+    description:
+      The GPIO number of the NXP chipset used for BT_WAKE_IN.
+
+  nxp,wakeout-pin:
+    $ref: /schemas/types.yaml#/definitions/uint8
+    description:
+      The GPIO number of the NXP chipset used for BT_WAKE_OUT.
+
 required:
   - compatible
 
@@ -54,5 +64,7 @@ examples:
             fw-init-baudrate = <3000000>;
             firmware-name = "uartuart8987_bt_v0.bin";
             device-wakeup-gpios = <&gpio 11 GPIO_ACTIVE_HIGH>;
+            nxp,wakein-pin = /bits/ 8 <18>;
+            nxp,wakeout-pin = /bits/ 8 <19>;
         };
     };
-- 
2.34.1

RE: [1/2] bluetooth: btnxpuart: Support for controller wakeup gpio config

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 3556 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=934718

---Test result---

Test Summary:
CheckPatch                    PENDING   0.23 seconds
GitLint                       PENDING   0.21 seconds
SubjectPrefix                 FAIL      0.57 seconds
BuildKernel                   PASS      28.20 seconds
CheckAllWarning               PASS      30.23 seconds
CheckSparse                   PASS      30.60 seconds
BuildKernel32                 PASS      24.28 seconds
TestRunnerSetup               PASS      430.65 seconds
TestRunner_l2cap-tester       PASS      20.59 seconds
TestRunner_iso-tester         FAIL      142.23 seconds
TestRunner_bnep-tester        PASS      4.85 seconds
TestRunner_mgmt-tester        FAIL      129.41 seconds
TestRunner_rfcomm-tester      PASS      7.85 seconds
TestRunner_sco-tester         PASS      9.59 seconds
TestRunner_ioctl-tester       PASS      8.27 seconds
TestRunner_mesh-tester        PASS      6.29 seconds
TestRunner_smp-tester         PASS      7.18 seconds
TestRunner_userchan-tester    PASS      5.03 seconds
IncrementalBuild              PENDING   0.72 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: SubjectPrefix - FAIL
Desc: Check subject contains "Bluetooth" prefix
Output:
"Bluetooth: " prefix is not specified in the subject
"Bluetooth: " prefix is not specified in the subject
##############################
Test: TestRunner_iso-tester - FAIL
Desc: Run iso-tester with test-runner
Output:
Total: 125, Passed: 109 (87.2%), Failed: 12, Not Run: 4

Failed Test Cases
ISO Defer Connect2 CIG 0x01 - Success                Timed out    2.679 seconds
ISO Connected2 Suspend - Success                     Timed out    2.751 seconds
ISO AC 6(ii) - Success                               Timed out    1.900 seconds
ISO AC 7(ii) - Success                               Timed out    2.502 seconds
ISO AC 8(ii) - Success                               Timed out    2.516 seconds
ISO AC 9(ii) - Success                               Timed out    2.502 seconds
ISO AC 11(ii) - Success                              Timed out    2.500 seconds
ISO AC 1 + 2 - Success                               Timed out    1.971 seconds
ISO AC 1 + 2 CIG 0x01/0x02 - Success                 Timed out    2.003 seconds
ISO Reconnect AC 6(i) - Success                      Timed out    2.021 seconds
ISO Reconnect AC 6(ii) - Success                     Timed out    1.994 seconds
ISO AC 6(ii) CIS 0xEF/auto - Success                 Timed out    2.002 seconds
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 490, Passed: 482 (98.4%), Failed: 4, Not Run: 4

Failed Test Cases
LL Privacy - Set Flags 1 (Add to RL)                 Failed       0.147 seconds
LL Privacy - Set Flags 2 (Enable RL)                 Failed       0.150 seconds
LL Privacy - Unpair 1                                Timed out    2.183 seconds
LL Privacy - Unpair 2 (Remove from AL)               Failed       2.546 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

[PATCH 1/2] bluetooth: btnxpuart: Support for controller wakeup gpio config

[Neeraj Sanjay Kale]

Hi Loic,

Thank you for your patch. Just a few suggestions below:
 
> @@ -616,6 +617,13 @@ static void ps_init(struct hci_dev *hdev)
>                 break;
>         }
> 
> +       if (!device_property_read_u8(&nxpdev->serdev->dev, "nxp,wakein-pin",
> +                                    &psdata->h2c_wakeup_gpio))
> +               psdata->h2c_wakeupmode = WAKEUP_METHOD_GPIO;
> +       if (!device_property_read_u8(&nxpdev->serdev->dev, "nxp,wakeout-
> pin",
> +                                    &psdata->c2h_wakeup_gpio))
> +               psdata->c2h_wakeupmode = BT_HOST_WAKEUP_METHOD_GPIO;
> +
>         psdata->cur_psmode = PS_MODE_DISABLE;
>         psdata->target_ps_mode = DEFAULT_PS_MODE;
> 
Please move device_property_read for "nxp,wakein-pin" to ps_setup(), after "device-wakeup" is read.

I think we should not set h2c_wakeupmode as WAKEUP_METHOD_GPIO based on "nxp,wakein-pin" alone.

In existing code, we are setting default_h2c_wakeup_mode to WAKEUP_METHOD_GPIO if "device-wakeup" is defined in DT, and psdata->h2c_wakeup_gpio = 0xff. WAKE_IN pin is not read.
In this case the FW considers default GPIO as WAKE_IN pin (as per datasheet), which is a valid scenario.

But this logic will fail if we specify only "nxp,wakein-pin", without "device-wakeup" in DT.
Hence, I recommend something as follows in ps_setup():
- if (!psdata->h2c_ps_gpio)
+ if (!psdata->h2c_ps_gpio || device_property_read_u8(&nxpdev->serdev->dev, "nxp,wakein-pin", &psdata->h2c_wakeup_gpio))
        psdata->h2c_wakeup_gpio = 0xff;

For "nxp,wakeout-pin", I have yet to submit patch for "host-wakeup-gpios". I can move "nxp,wakeout-pin" later if required.

Thanks,
Neeraj

Re: [PATCH 1/2] bluetooth: btnxpuart: Support for controller wakeup gpio config

[Loic Poulain]

On Mon, 17 Feb 2025 at 14:53, Neeraj Sanjay Kale
<neeraj.sanjaykale@nxp.com> wrote:
> Hi Loic,
>
> Thank you for your patch. Just a few suggestions below:
>
> > @@ -616,6 +617,13 @@ static void ps_init(struct hci_dev *hdev)
> >                 break;
> >         }
> >
> > +       if (!device_property_read_u8(&nxpdev->serdev->dev, "nxp,wakein-pin",
> > +                                    &psdata->h2c_wakeup_gpio))
> > +               psdata->h2c_wakeupmode = WAKEUP_METHOD_GPIO;
> > +       if (!device_property_read_u8(&nxpdev->serdev->dev, "nxp,wakeout-
> > pin",
> > +                                    &psdata->c2h_wakeup_gpio))
> > +               psdata->c2h_wakeupmode = BT_HOST_WAKEUP_METHOD_GPIO;
> > +
> >         psdata->cur_psmode = PS_MODE_DISABLE;
> >         psdata->target_ps_mode = DEFAULT_PS_MODE;
> >
> Please move device_property_read for "nxp,wakein-pin" to ps_setup(), after "device-wakeup" is read.

Ok, but then I'll need to move all the default value handling from
ps_setup() into ps_init() as well.

>
> I think we should not set h2c_wakeupmode as WAKEUP_METHOD_GPIO based on "nxp,wakein-pin" alone.
>
> In existing code, we are setting default_h2c_wakeup_mode to WAKEUP_METHOD_GPIO if "device-wakeup" is defined in DT, and psdata->h2c_wakeup_gpio = 0xff. WAKE_IN pin is not read.
> In this case the FW considers default GPIO as WAKE_IN pin (as per datasheet), which is a valid scenario.
>
> But this logic will fail if we specify only "nxp,wakein-pin", without "device-wakeup" in DT.
> Hence, I recommend something as follows in ps_setup():
> - if (!psdata->h2c_ps_gpio)
> + if (!psdata->h2c_ps_gpio || device_property_read_u8(&nxpdev->serdev->dev, "nxp,wakein-pin", &psdata->h2c_wakeup_gpio))
>         psdata->h2c_wakeup_gpio = 0xff;

Ok, will do, look like we should print an explicit error then, as it
would be a clear devicetree misconfiguration?

> For "nxp,wakeout-pin", I have yet to submit patch for "host-wakeup-gpios". I can move "nxp,wakeout-pin" later if required.

It may not be necessary, I've submitted an other PR for handling
simple dedicated wakeup interrupts at serdev core level instead of
having to re-implement it in each driver:
https://www.spinics.net/lists/linux-serial/msg66060.html

With that, all we would need is adding the wakeup interrupt in the devicetree:
```
        interrupt-parent = <&gpio4>;
        interrupts = <8 IRQ_TYPE_EDGE_FALLING>;
        interrupt-names = "wakeup";
        wakeup-source;
```

Regards,
Loic

Re: [PATCH 1/2] bluetooth: btnxpuart: Support for controller wakeup gpio config

[Neeraj Sanjay Kale]

Hi Loic,

> On Mon, 17 Feb 2025 at 14:53, Neeraj Sanjay Kale
> <neeraj.sanjaykale@nxp.com> wrote:
> > Hi Loic,
> >
> > Thank you for your patch. Just a few suggestions below:
> >
> > > @@ -616,6 +617,13 @@ static void ps_init(struct hci_dev *hdev)
> > >                 break;
> > >         }
> > >
> > > +       if (!device_property_read_u8(&nxpdev->serdev->dev, "nxp,wakein-
> pin",
> > > +                                    &psdata->h2c_wakeup_gpio))
> > > +               psdata->h2c_wakeupmode = WAKEUP_METHOD_GPIO;
> > > +       if (!device_property_read_u8(&nxpdev->serdev->dev,
> > > + "nxp,wakeout-
> > > pin",
> > > +                                    &psdata->c2h_wakeup_gpio))
> > > +               psdata->c2h_wakeupmode =
> BT_HOST_WAKEUP_METHOD_GPIO;
> > > +
> > >         psdata->cur_psmode = PS_MODE_DISABLE;
> > >         psdata->target_ps_mode = DEFAULT_PS_MODE;
> > >
> > Please move device_property_read for "nxp,wakein-pin" to ps_setup(), after
> "device-wakeup" is read.
>
> Ok, but then I'll need to move all the default value handling from
> ps_setup() into ps_init() as well.
I don't think that would be needed. Simply using the example code I mentioned below should suffice.

To re-iterate, if "device-wakeup-gpios" is defined, we use WAKEUP_METHOD_GPIO, and if "nxp,wakein-pin" is present, use it, else simply use 0xff.

But if "device-wakeup-gpios" is absent, use default WAKEUP_METHOD_BREAK.

>
> >
> > I think we should not set h2c_wakeupmode as WAKEUP_METHOD_GPIO
> based on "nxp,wakein-pin" alone.
> >
> > In existing code, we are setting default_h2c_wakeup_mode to
> WAKEUP_METHOD_GPIO if "device-wakeup" is defined in DT, and psdata-
> >h2c_wakeup_gpio = 0xff. WAKE_IN pin is not read.
> > In this case the FW considers default GPIO as WAKE_IN pin (as per
> datasheet), which is a valid scenario.
> >
> > But this logic will fail if we specify only "nxp,wakein-pin", without "device-
> wakeup" in DT.
> > Hence, I recommend something as follows in ps_setup():
> > - if (!psdata->h2c_ps_gpio)
> > + if (!psdata->h2c_ps_gpio ||
> > + device_property_read_u8(&nxpdev->serdev->dev, "nxp,wakein-pin",
> > + &psdata->h2c_wakeup_gpio))
> >         psdata->h2c_wakeup_gpio = 0xff;
>
> Ok, will do, look like we should print an explicit error then, as it would be a
> clear devicetree misconfiguration?
Yes, an error print for "nxp,wakein-pin", without "device-wakeup-gpios" would be helpful. Thanks!

>
> > For "nxp,wakeout-pin", I have yet to submit patch for "host-wakeup-gpios".
> I can move "nxp,wakeout-pin" later if required.
>
> It may not be necessary, I've submitted an other PR for handling simple
> dedicated wakeup interrupts at serdev core level instead of having to re-
> implement it in each driver:
> https://www.s/
> pinics.net%2Flists%2Flinux-
> serial%2Fmsg66060.html&data=05%7C02%7Cneeraj.sanjaykale%40nxp.com%
> 7C979e9f5f906b438d707e08dd4f61b6e9%7C686ea1d3bc2b4c6fa92cd99c5c30
> 1635%7C0%7C0%7C638754003307634680%7CUnknown%7CTWFpbGZsb3d8e
> yJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoi
> TWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=h%2B9yWxHbWkimN
> 7oHOM0ZU9Cgi1OK86NLGKP7Hw%2B6vRs%3D&reserved=0
>
> With that, all we would need is adding the wakeup interrupt in the devicetree:
> ```
>         interrupt-parent = <&gpio4>;
>         interrupts = <8 IRQ_TYPE_EDGE_FALLING>;
>         interrupt-names = "wakeup";
>         wakeup-source;
> ```
Yes, this was my initial approach, which works fine. But I think using "host-wakeup-gpios" would be a cleaner approach.
Driver will simply use the gpiod_to_irq() API to get an IRQ handler.
```
        compatible = "nxp,88w8987-bt";
        host-wakeup-gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>;
```
Please do let me know if this method has any drawbacks.

Thanks,
Neeraj

Re: [PATCH 1/2] bluetooth: btnxpuart: Support for controller wakeup gpio config

[Loic Poulain]

On Mon, 17 Feb 2025 at 16:41, Neeraj Sanjay Kale
<neeraj.sanjaykale@nxp.com> wrote:
>
> Hi Loic,
>
> > On Mon, 17 Feb 2025 at 14:53, Neeraj Sanjay Kale
> > <neeraj.sanjaykale@nxp.com> wrote:
> > > Hi Loic,
> > >
> > > Thank you for your patch. Just a few suggestions below:
> > >
> > > > @@ -616,6 +617,13 @@ static void ps_init(struct hci_dev *hdev)
> > > >                 break;
> > > >         }
> > > >
> > > > +       if (!device_property_read_u8(&nxpdev->serdev->dev, "nxp,wakein-
> > pin",
> > > > +                                    &psdata->h2c_wakeup_gpio))
> > > > +               psdata->h2c_wakeupmode = WAKEUP_METHOD_GPIO;
> > > > +       if (!device_property_read_u8(&nxpdev->serdev->dev,
> > > > + "nxp,wakeout-
> > > > pin",
> > > > +                                    &psdata->c2h_wakeup_gpio))
> > > > +               psdata->c2h_wakeupmode =
> > BT_HOST_WAKEUP_METHOD_GPIO;
> > > > +
> > > >         psdata->cur_psmode = PS_MODE_DISABLE;
> > > >         psdata->target_ps_mode = DEFAULT_PS_MODE;
> > > >
> > > Please move device_property_read for "nxp,wakein-pin" to ps_setup(), after
> > "device-wakeup" is read.
> >
> > Ok, but then I'll need to move all the default value handling from
> > ps_setup() into ps_init() as well.
> I don't think that would be needed. Simply using the example code I mentioned below should suffice.
>
> To re-iterate, if "device-wakeup-gpios" is defined, we use WAKEUP_METHOD_GPIO, and if "nxp,wakein-pin" is present, use it, else simply use 0xff.
>
> But if "device-wakeup-gpios" is absent, use default WAKEUP_METHOD_BREAK.
>
> >
> > >
> > > I think we should not set h2c_wakeupmode as WAKEUP_METHOD_GPIO
> > based on "nxp,wakein-pin" alone.
> > >
> > > In existing code, we are setting default_h2c_wakeup_mode to
> > WAKEUP_METHOD_GPIO if "device-wakeup" is defined in DT, and psdata-
> > >h2c_wakeup_gpio = 0xff. WAKE_IN pin is not read.
> > > In this case the FW considers default GPIO as WAKE_IN pin (as per
> > datasheet), which is a valid scenario.
> > >
> > > But this logic will fail if we specify only "nxp,wakein-pin", without "device-
> > wakeup" in DT.
> > > Hence, I recommend something as follows in ps_setup():
> > > - if (!psdata->h2c_ps_gpio)
> > > + if (!psdata->h2c_ps_gpio ||
> > > + device_property_read_u8(&nxpdev->serdev->dev, "nxp,wakein-pin",
> > > + &psdata->h2c_wakeup_gpio))
> > >         psdata->h2c_wakeup_gpio = 0xff;
> >
> > Ok, will do, look like we should print an explicit error then, as it would be a
> > clear devicetree misconfiguration?
> Yes, an error print for "nxp,wakein-pin", without "device-wakeup-gpios" would be helpful. Thanks!
>
> >
> > > For "nxp,wakeout-pin", I have yet to submit patch for "host-wakeup-gpios".
> > I can move "nxp,wakeout-pin" later if required.
> >
> > It may not be necessary, I've submitted an other PR for handling simple
> > dedicated wakeup interrupts at serdev core level instead of having to re-
> > implement it in each driver:
> > https://www.s/
> > pinics.net%2Flists%2Flinux-
> > serial%2Fmsg66060.html&data=05%7C02%7Cneeraj.sanjaykale%40nxp.com%
> > 7C979e9f5f906b438d707e08dd4f61b6e9%7C686ea1d3bc2b4c6fa92cd99c5c30
> > 1635%7C0%7C0%7C638754003307634680%7CUnknown%7CTWFpbGZsb3d8e
> > yJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoi
> > TWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=h%2B9yWxHbWkimN
> > 7oHOM0ZU9Cgi1OK86NLGKP7Hw%2B6vRs%3D&reserved=0
> >
> > With that, all we would need is adding the wakeup interrupt in the devicetree:
> > ```
> >         interrupt-parent = <&gpio4>;
> >         interrupts = <8 IRQ_TYPE_EDGE_FALLING>;
> >         interrupt-names = "wakeup";
> >         wakeup-source;
> > ```
> Yes, this was my initial approach, which works fine. But I think using "host-wakeup-gpios" would be a cleaner approach.
> Driver will simply use the gpiod_to_irq() API to get an IRQ handler.
> ```
>         compatible = "nxp,88w8987-bt";
>         host-wakeup-gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>;
> ```
> Please do let me know if this method has any drawbacks.

Two points:
- Why bother with a GPIO if we can directly pass an interrupt (and if
actually don't care about gpio framework usage)
- Why re-implementing it in the driver if the dedicated interrupt can
be handled at the generic layer (serdev bus/core)
Would be good if we can stick with
`Documentation/devicetree/bindings/power/wakeup-source.txt`.

Regards,
Loic

Re: [PATCH 1/2] bluetooth: btnxpuart: Support for controller wakeup gpio config

[Loic Poulain]

On Mon, 17 Feb 2025 at 14:53, Neeraj Sanjay Kale
<neeraj.sanjaykale@nxp.com> wrote:
>
>
> Hi Loic,
>
> Thank you for your patch. Just a few suggestions below:
>
> > @@ -616,6 +617,13 @@ static void ps_init(struct hci_dev *hdev)
> >                 break;
> >         }
> >
> > +       if (!device_property_read_u8(&nxpdev->serdev->dev, "nxp,wakein-pin",
> > +                                    &psdata->h2c_wakeup_gpio))
> > +               psdata->h2c_wakeupmode = WAKEUP_METHOD_GPIO;
> > +       if (!device_property_read_u8(&nxpdev->serdev->dev, "nxp,wakeout-
> > pin",
> > +                                    &psdata->c2h_wakeup_gpio))
> > +               psdata->c2h_wakeupmode = BT_HOST_WAKEUP_METHOD_GPIO;
> > +
> >         psdata->cur_psmode = PS_MODE_DISABLE;
> >         psdata->target_ps_mode = DEFAULT_PS_MODE;
> >
> Please move device_property_read for "nxp,wakein-pin" to ps_setup(), after "device-wakeup" is read.
>
> I think we should not set h2c_wakeupmode as WAKEUP_METHOD_GPIO based on "nxp,wakein-pin" alone.
>
> In existing code, we are setting default_h2c_wakeup_mode to WAKEUP_METHOD_GPIO if "device-wakeup" is defined in DT, and psdata->h2c_wakeup_gpio = 0xff. WAKE_IN pin is not read.
> In this case the FW considers default GPIO as WAKE_IN pin (as per datasheet), which is a valid scenario.
>
> But this logic will fail if we specify only "nxp,wakein-pin", without "device-wakeup" in DT.
> Hence, I recommend something as follows in ps_setup():
> - if (!psdata->h2c_ps_gpio)
> + if (!psdata->h2c_ps_gpio || device_property_read_u8(&nxpdev->serdev->dev, "nxp,wakein-pin", &psdata->h2c_wakeup_gpio))
>         psdata->h2c_wakeup_gpio = 0xff;

I don't get it, can you clarify when we should default to 0xff value
for h2c_wakeup_gpio? and what it means for the firmware.
Before the above change, we apply 0xff if there is **no**
device-wakeup gpio, so if wakeup mode is not GPIO.
After the above change, we apply 0xff if there is no device-wakeup
gpio but also if there is no wakein-pin (whether there is a
device-wakeup gpio or not)...

Regards,
Loic

Re: [PATCH 1/2] bluetooth: btnxpuart: Support for controller wakeup gpio config

[Neeraj Sanjay Kale]

Hi Loic,

> >
> > > @@ -616,6 +617,13 @@ static void ps_init(struct hci_dev *hdev)
> > >                 break;
> > >         }
> > >
> > > +       if (!device_property_read_u8(&nxpdev->serdev->dev, "nxp,wakein-
> pin",
> > > +                                    &psdata->h2c_wakeup_gpio))
> > > +               psdata->h2c_wakeupmode = WAKEUP_METHOD_GPIO;
> > > +       if (!device_property_read_u8(&nxpdev->serdev->dev,
> > > + "nxp,wakeout-
> > > pin",
> > > +                                    &psdata->c2h_wakeup_gpio))
> > > +               psdata->c2h_wakeupmode =
> BT_HOST_WAKEUP_METHOD_GPIO;
> > > +
> > >         psdata->cur_psmode = PS_MODE_DISABLE;
> > >         psdata->target_ps_mode = DEFAULT_PS_MODE;
> > >
> > Please move device_property_read for "nxp,wakein-pin" to ps_setup(), after
> "device-wakeup" is read.
> >
> > I think we should not set h2c_wakeupmode as WAKEUP_METHOD_GPIO
> based on "nxp,wakein-pin" alone.
> >
> > In existing code, we are setting default_h2c_wakeup_mode to
> WAKEUP_METHOD_GPIO if "device-wakeup" is defined in DT, and psdata-
> >h2c_wakeup_gpio = 0xff. WAKE_IN pin is not read.
> > In this case the FW considers default GPIO as WAKE_IN pin (as per
> datasheet), which is a valid scenario.
> >
> > But this logic will fail if we specify only "nxp,wakein-pin", without "device-
> wakeup" in DT.
> > Hence, I recommend something as follows in ps_setup():
> > - if (!psdata->h2c_ps_gpio)
> > + if (!psdata->h2c_ps_gpio ||
> > + device_property_read_u8(&nxpdev->serdev->dev, "nxp,wakein-pin",
> > + &psdata->h2c_wakeup_gpio))
> >         psdata->h2c_wakeup_gpio = 0xff;
> 
> I don't get it, can you clarify when we should default to 0xff value for
> h2c_wakeup_gpio? and what it means for the firmware.
> Before the above change, we apply 0xff if there is **no** device-wakeup
> gpio, so if wakeup mode is not GPIO.
> After the above change, we apply 0xff if there is no device-wakeup gpio but
> also if there is no wakein-pin (whether there is a device-wakeup gpio or
> not)...
> 
In send_wakeup_method_cmd(), we always set pcmd.h2c_wakeup_gpio = 0xff, no matter if the wakeup method is BREAK or GPIO.
This was done on-purpose to allow FW to use default (hardcoded)WAKE_IN pin, specific to the chip, for GPIO wake-up method.
User can get the WAKE_IN pin info from the chip's datasheet. Pretty straightforward right?

With your patch, send_wakeup_method_cmd() sets pcmd.h2c_wakeup_gpio = psdata->h2c_wakeup_gpio.
The GPIO based device wakeup functionality works with very limited number of pins, which again varies from one chip to another.
So not adding wakein-pin was intentional (based on internal discussion) to avoid any sort of confusion and maintain consistency between datasheet and FW default pins.

We would want this behaviour to be continued in this patch, such that if "device-wakeup-gpios" is defined **without** "wakein-pin", the pcmd. h2c_wakeup_gpio parameter = psdata->h2c_wakeup_gpio  would still be 0xff in send_wakeup_method_cmd().

Hope this clarifies everything.

Thanks,
Neeraj