Hi Rajat,
On Thu, Jan 12, 2017 at 10:01:06AM -0800, Rajat Jain wrote:
> Some onboard BT chips (e.g. Marvell 8997) contain a wakeup pin that
> can be connected to a gpio on the CPU side, and can be used to wakeup
> the host out-of-band. This can be useful in situations where the
> in-band wakeup is not possible or not preferable (e.g. the in-band
> wakeup may require the USB host controller to remain active, and
> hence consuming more system power during system sleep).
>
> The oob gpio interrupt to be used for wakeup on the CPU side, is
> read from the device tree node, (using standard interrupt descriptors).
> A devcie tree binding document is also added for the driver. The
> compatible string is in compliance with
> Documentation/devicetree/bindings/usb/usb-device.txt
>
> Signed-off-by: Rajat Jain <rajatja@xxxxxxxxxx>
> Reviewed-by: Brian Norris <briannorris@xxxxxxxxxxxx>
> Acked-by: Rob Herring <robh@xxxxxxxxxx>
> ---
> v5: Move the call to pm_wakeup_event() to the begining of irq handler.
> v4: Move the set_bit(BTUSB_OOB_WAKE_DISABLED,..) call to the beginning of
> btusb_config_oob_wake()
> v3: Add Brian's "Reviewed-by"
> v2: * Use interrupt-names ("wakeup") instead of assuming first interrupt.
> * Leave it on device tree to specify IRQ flags (level /edge triggered)
> * Mark the device as non wakeable on exit.
>
> Documentation/devicetree/bindings/net/btusb.txt | 40 ++++++++++++
> drivers/bluetooth/btusb.c | 85 +++++++++++++++++++++++++
> 2 files changed, 125 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/btusb.txt
>
> diff --git a/Documentation/devicetree/bindings/net/btusb.txt b/Documentation/devicetree/bindings/net/btusb.txt
> new file mode 100644
> index 000000000000..2c0355c85972
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/btusb.txt
> @@ -0,0 +1,40 @@
> +Generic Bluetooth controller over USB (btusb driver)
> +---------------------------------------------------
> +
> +Required properties:
> +
> + - compatible : should comply with the format "usbVID,PID" specified in
> + Documentation/devicetree/bindings/usb/usb-device.txt
> + At the time of writing, the only OF supported devices
> + (more may be added later) are:
> +
> + "usb1286,204e" (Marvell 8997)
> +
> +Optional properties:
> +
> + - interrupt-parent: phandle of the parent interrupt controller
> + - interrupt-names: (see below)
> + - interrupts : The interrupt specified by the name "wakeup" is the interrupt
> + that shall be used for out-of-band wake-on-bt. Driver will
> + request this interrupt for wakeup. During system suspend, the
> + irq will be enabled so that the bluetooth chip can wakeup host
> + platform out of band. During system resume, the irq will be
> + disabled to make sure unnecessary interrupt is not received.
> +
> +Example:
> +
> +Following example uses irq pin number 3 of gpio0 for out of band wake-on-bt:
> +
> +&usb_host1_ehci {
> + status = "okay";
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + mvl_bt1: bt@1 {
> + compatible = "usb1286,204e";
> + reg = <1>;
> + interrupt-parent = <&gpio0>;
> + interrupt-name = "wakeup";
> + interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
> + };
> +};
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index ce22cefceed1..0a777bb407b1 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -24,6 +24,8 @@
> #include <linux/module.h>
> #include <linux/usb.h>
> #include <linux/firmware.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> #include <asm/unaligned.h>
>
> #include <net/bluetooth/bluetooth.h>
> @@ -369,6 +371,7 @@ static const struct usb_device_id blacklist_table[] = {
> #define BTUSB_BOOTING 9
> #define BTUSB_RESET_RESUME 10
> #define BTUSB_DIAG_RUNNING 11
> +#define BTUSB_OOB_WAKE_DISABLED 12
>
> struct btusb_data {
> struct hci_dev *hdev;
> @@ -416,6 +419,8 @@ struct btusb_data {
> int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
>
> int (*setup_on_usb)(struct hci_dev *hdev);
> +
> + int oob_wake_irq; /* irq for out-of-band wake-on-bt */
> };
>
> static inline void btusb_free_frags(struct btusb_data *data)
> @@ -2728,6 +2733,66 @@ static int btusb_bcm_set_diag(struct hci_dev *hdev, bool enable)
> }
> #endif
>
> +#ifdef CONFIG_PM
> +static irqreturn_t btusb_oob_wake_handler(int irq, void *priv)
> +{
> + struct btusb_data *data = priv;
> +
> + pm_wakeup_event(&data->udev->dev, 0);
> +
> + /* Disable only if not already disabled (keep it balanced) */
> + if (!test_and_set_bit(BTUSB_OOB_WAKE_DISABLED, &data->flags)) {
> + disable_irq_nosync(irq);
> + disable_irq_wake(irq);
> + }
> + return IRQ_HANDLED;
> +}
> +
> +static const struct of_device_id btusb_match_table[] = {
> + { .compatible = "usb1286,204e" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, btusb_match_table);
> +
> +/* Use an oob wakeup pin? */
> +static int btusb_config_oob_wake(struct hci_dev *hdev)
> +{
> + struct btusb_data *data = hci_get_drvdata(hdev);
> + struct device *dev = &data->udev->dev;
> + int irq, ret;
> +
> + set_bit(BTUSB_OOB_WAKE_DISABLED, &data->flags);
> +
> + if (!of_match_device(btusb_match_table, dev))
> + return 0;
> +
> + /* Move on if no IRQ specified */
> + irq = of_irq_get_byname(dev->of_node, "wakeup");
> + if (irq <= 0) {
> + bt_dev_dbg(hdev, "%s: no OOB Wakeup IRQ in DT", __func__);
> + return 0;
> + }
> +
> + ret = devm_request_irq(&hdev->dev, irq, btusb_oob_wake_handler,
> + 0, "OOB Wake-on-BT", data);
> + if (ret) {
> + bt_dev_err(hdev, "%s: IRQ request failed", __func__);
> + return ret;
> + }
> +
> + ret = device_init_wakeup(dev, true);
> + if (ret) {
> + bt_dev_err(hdev, "%s: failed to init_wakeup\n", __func__);
bt_dev_err() includes the newlines for you, but you'd added an extra one
here (but not above).
> + return ret;
> + }
> +
> + data->oob_wake_irq = irq;
> + disable_irq(irq);
> + bt_dev_info(hdev, "OOB Wake-on-BT configured at IRQ %u\n", irq);
And here.
> + return 0;
> +}
> +#endif
> +
> static int btusb_probe(struct usb_interface *intf,
> const struct usb_device_id *id)
> {
> @@ -2849,6 +2914,11 @@ static int btusb_probe(struct usb_interface *intf,
> hdev->send = btusb_send_frame;
> hdev->notify = btusb_notify;
>
> +#ifdef CONFIG_PM
> + err = btusb_config_oob_wake(hdev);
> + if (err)
> + goto out_free_dev;
> +#endif
> if (id->driver_info & BTUSB_CW6622)
> set_bit(HCI_QUIRK_BROKEN_STORED_LINK_KEY, &hdev->quirks);
>
> @@ -3061,6 +3131,9 @@ static void btusb_disconnect(struct usb_interface *intf)
> usb_driver_release_interface(&btusb_driver, data->isoc);
> }
>
> + if (data->oob_wake_irq)
> + device_init_wakeup(&data->udev->dev, false);
> +
> hci_free_dev(hdev);
> }
>
> @@ -3089,6 +3162,12 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
> btusb_stop_traffic(data);
> usb_kill_anchored_urbs(&data->tx_anchor);
>
> + if (data->oob_wake_irq && device_may_wakeup(&data->udev->dev)) {
> + clear_bit(BTUSB_OOB_WAKE_DISABLED, &data->flags);
> + enable_irq_wake(data->oob_wake_irq);
> + enable_irq(data->oob_wake_irq);
> + }
> +
> /* Optionally request a device reset on resume, but only when
> * wakeups are disabled. If wakeups are enabled we assume the
> * device will stay powered up throughout suspend.
> @@ -3126,6 +3205,12 @@ static int btusb_resume(struct usb_interface *intf)
> if (--data->suspend_count)
> return 0;
>
> + /* Disable only if not already disabled (keep it balanced) */
> + if (!test_and_set_bit(BTUSB_OOB_WAKE_DISABLED, &data->flags)) {
As I mentioned elsewhere, the negative form (i.e., DISABLED instead of
ENABLED) seems a little backward to me. It has the small effect of
meaning that the default behavior is actually to pretend that OOB wake
was enabled, and so if somehow btusb_config_oob_wake() wasn't called
(e.g., if CONFIG_PM is not enabled, or if the code gets refactored) this
then btusb_resume() will behave badly.
Now, this doesn't create an immediate problem (btusb_resume() should
never be called if !CONFIG_PM), but it does suggest that maybe it would
be better for the default (0) value to mean "disabled".
Brian
> + disable_irq(data->oob_wake_irq);
> + disable_irq_wake(data->oob_wake_irq);
> + }
> +
> if (!test_bit(HCI_RUNNING, &hdev->flags))
> goto done;
>
> --
> 2.11.0.390.gc69c2f50cf-goog
>
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
