Re: [PATCH 5/5] Bluetooth: hci_intel: Implement suspend/resume

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Loic,

> Add platform PM suspend/resume callbacks.
> Chip can be suspended/resumed via specific vendor commands which need
> to be acked by the controller.
> Enable the IRQ wake capability in order to let the controller wake the
> host in case of available data.
> 
> Signed-off-by: Loic Poulain <loic.poulain@xxxxxxxxx>
> ---
> drivers/bluetooth/hci_intel.c | 176 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 176 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_intel.c b/drivers/bluetooth/hci_intel.c
> index 99f1307..4fdcb5d 100644
> --- a/drivers/bluetooth/hci_intel.c
> +++ b/drivers/bluetooth/hci_intel.c
> @@ -45,6 +45,10 @@
> #define STATE_FIRMWARE_FAILED	3
> #define STATE_BOOTING		4
> #define STATE_TX_ACTIVE		5
> +#define STATE_SUSPENDED		7
> +#define STATE_SUSPENDING	8
> +#define STATE_RESUMING		9
> +#define STATE_LPM_ENABLED	10

at some point we really need to think about how we can combine USB and UART into a unified code block. So we need to be careful with just stuffing new states in here.

> 
> #define HCI_LPM_PKT 0xf1
> #define HCI_LPM_MAX_SIZE 10
> @@ -57,6 +61,8 @@
> 	.maxlen = HCI_LPM_MAX_SIZE
> 
> #define LPM_OP_TX_NOTIFY 0x00
> +#define LPM_OP_SUSPEND_ACK 0x02
> +#define LPM_OP_RESUME_ACK 0x03
> 
> struct hci_lpm_pkt {
> 	__u8 opcode;
> @@ -735,6 +741,8 @@ done:
> 	}
> 	kfree_skb(skb);
> 
> +	set_bit(STATE_LPM_ENABLED, &intel->flags);
> +

This should have gone in the previous patch introducing LPM support.

> no_lpm:
> 	skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_CMD_TIMEOUT);
> 	if (IS_ERR(skb))
> @@ -812,12 +820,28 @@ static void intel_recv_lpm_notification(struct hci_dev *hdev, int value)
> static int intel_recv_lpm(struct hci_dev *hdev, struct sk_buff *skb)
> {
> 	struct hci_lpm_pkt *lpm = (void *)skb->data;
> +	struct hci_uart *hu = hci_get_drvdata(hdev);
> +	struct intel_data *intel = hu->priv;
> 
> 	switch (lpm->opcode) {
> 	case LPM_OP_TX_NOTIFY:
> 		if (lpm->dlen)
> 			intel_recv_lpm_notification(hdev, lpm->data[0]);
> 		break;
> +	case LPM_OP_SUSPEND_ACK:
> +		set_bit(STATE_SUSPENDED, &intel->flags);
> +		if (test_and_clear_bit(STATE_SUSPENDING, &intel->flags)) {
> +			smp_mb__after_atomic();
> +			wake_up_bit(&intel->flags, STATE_SUSPENDING);
> +		}
> +		break;
> +	case LPM_OP_RESUME_ACK:
> +		clear_bit(STATE_SUSPENDED, &intel->flags);
> +		if (test_and_clear_bit(STATE_RESUMING, &intel->flags)) {
> +			smp_mb__after_atomic();
> +			wake_up_bit(&intel->flags, STATE_SUSPENDING);
> +		}

Is this a typo with RESUMING vs SUSPENDING? Otherwise better call this LPM_TRANSACTION or something that makes clear that we are doing a LPM change. Since we can only have one at a time.

> +		break;
> 	default:
> 		BT_ERR("%s: unknown LPM opcode (%02x)", hdev->name,
> 		       lpm->opcode);
> @@ -895,6 +919,111 @@ static struct sk_buff *intel_dequeue(struct hci_uart *hu)
> 	return skb;
> }
> 
> +static int intel_lpm_suspend(struct hci_uart *hu)
> +{
> +	static const u8 suspend[] = {0x01, 0x01, 0x01};
> +	struct intel_data *intel = hu->priv;
> +	struct sk_buff *skb;
> +	int err;
> +
> +	if (!test_bit(STATE_LPM_ENABLED, &intel->flags) ||
> +	    test_bit(STATE_SUSPENDED, &intel->flags))
> +		return 0;
> +
> +	if (test_bit(STATE_TX_ACTIVE, &intel->flags))
> +		return -EAGAIN;
> +
> +	BT_DBG("%s: suspending", hu->hdev->name);
> +
> +	skb = bt_skb_alloc(sizeof(suspend), GFP_KERNEL);
> +	if (!skb) {
> +		BT_ERR("Failed to allocate memory for suspend packet");
> +		return -ENOMEM;
> +	}
> +
> +	memcpy(skb_put(skb, sizeof(suspend)), suspend, sizeof(suspend));
> +	bt_cb(skb)->pkt_type = HCI_LPM_PKT;
> +
> +	set_bit(STATE_SUSPENDING, &intel->flags);
> +
> +	skb_queue_tail(&intel->txq, skb);
> +	hci_uart_tx_wakeup(hu);
> +
> +	err = wait_on_bit_timeout(&intel->flags, STATE_SUSPENDING,
> +				  TASK_INTERRUPTIBLE,
> +				  msecs_to_jiffies(1000));
> +	if (err == 1) {
> +		BT_ERR("%s: Device suspend interrupted", hu->hdev->name);
> +		clear_bit(STATE_SUSPENDING, &intel->flags);
> +		return -EINTR;
> +	}
> +
> +	if (err) {
> +		BT_ERR("%s: Device suspend timeout", hu->hdev->name);
> +		clear_bit(STATE_SUSPENDING, &intel->flags);
> +		return -ETIMEDOUT;
> +	}
> +
> +	if (!test_bit(STATE_SUSPENDED, &intel->flags)) {
> +		BT_ERR("%s: Device suspend error", hu->hdev->name);
> +		return -EINVAL;
> +	}

This keeps repeating itself. Create a helper function like I did with the booting one.

> +
> +	BT_DBG("%s: suspended", hu->hdev->name);
> +
> +	return 0;
> +}
> +
> +static int intel_lpm_resume(struct hci_uart *hu)
> +{
> +	struct intel_data *intel = hu->priv;
> +	struct sk_buff *skb;
> +	int err;
> +
> +	if (!test_bit(STATE_LPM_ENABLED, &intel->flags) ||
> +	    !test_bit(STATE_SUSPENDED, &intel->flags))
> +		return 0;
> +
> +	BT_DBG("%s: resuming", hu->hdev->name);
> +
> +	skb = bt_skb_alloc(0, GFP_KERNEL);
> +	if (!skb) {
> +		BT_ERR("Failed to allocate memory for resume packet");
> +		return -ENOMEM;
> +	}
> +
> +	bt_cb(skb)->pkt_type = 0xf0;

Introduce a define for this type like you did for the other one.

> +
> +	set_bit(STATE_RESUMING, &intel->flags);
> +
> +	skb_queue_tail(&intel->txq, skb);
> +	hci_uart_tx_wakeup(hu);
> +
> +	err = wait_on_bit_timeout(&intel->flags, STATE_RESUMING,
> +				  TASK_INTERRUPTIBLE,
> +				  msecs_to_jiffies(1000));
> +	if (err == 1) {
> +		BT_ERR("%s: Device resume interrupted", hu->hdev->name);
> +		clear_bit(STATE_RESUMING, &intel->flags);
> +		return -EINTR;
> +	}
> +
> +	if (err) {
> +		BT_ERR("%s: Device resume timeout", hu->hdev->name);
> +		clear_bit(STATE_RESUMING, &intel->flags);
> +		return -ETIMEDOUT;
> +	}
> +
> +	if (test_bit(STATE_SUSPENDED, &intel->flags)) {
> +		BT_ERR("%s: Device resume error", hu->hdev->name);
> +		return -EINVAL;
> +	}
> +
> +	BT_DBG("%s: resumed", hu->hdev->name);
> +
> +	return 0;
> +}
> +
> static const struct hci_uart_proto intel_proto = {
> 	.id		= HCI_UART_INTEL,
> 	.name		= "Intel",
> @@ -1025,6 +1154,52 @@ static int intel_remove(struct platform_device *pdev)
> 	return 0;
> }
> 
> +static int intel_suspend(struct device *dev)
> +{
> +	struct intel_device *idev = dev_get_drvdata(dev);
> +	struct intel_data *intel;
> +
> +	dev_dbg(dev, "intel_suspend\n");
> +
> +	if ((idev->irq < 0) || !device_may_wakeup(dev))
> +		return 0;

No ( ) around idev->irq < 0.

> +
> +	mutex_lock(&intel_data_list_lock);
> +	intel = intel_data_get(idev);
> +	if (intel)
> +		intel_lpm_suspend(intel->hu);
> +	mutex_unlock(&intel_data_list_lock);
> +
> +	enable_irq_wake(idev->irq);
> +
> +	return 0;
> +}
> +
> +static int intel_resume(struct device *dev)
> +{
> +	struct intel_device *idev = dev_get_drvdata(dev);
> +	struct intel_data *intel;
> +
> +	dev_dbg(dev, "intel_resume\n");
> +
> +	if ((idev->irq < 0) || !device_may_wakeup(dev))
> +		return 0;
> +
> +	disable_irq_wake(idev->irq);
> +
> +	mutex_lock(&intel_data_list_lock);
> +	intel = intel_data_get(idev);
> +	if (intel)
> +		intel_lpm_resume(intel->hu);
> +	mutex_unlock(&intel_data_list_lock);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops intel_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(intel_suspend, intel_resume)
> +};
> +

You need #ifdef CONFIG_PM_SLEEP.

> static struct platform_driver intel_driver = {
> 	.probe = intel_probe,
> 	.remove = intel_remove,
> @@ -1032,6 +1207,7 @@ static struct platform_driver intel_driver = {
> 		.name = "hci_intel",
> 		.acpi_match_table = ACPI_PTR(intel_acpi_match),
> 		.owner = THIS_MODULE,
> +		.pm = &intel_pm_ops,
> 	},
> };

Regards

Marcel

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



[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux