| Message ID | 20241001174021.522254-3-neeraj.sanjaykale@nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Bluetooth: btnxpuart: Add GPIO mechanism to | expand |
> -----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
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
> -----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
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
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;
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(-)