2019-04-18 09:11:05

by Sean Wang

[permalink] [raw]
Subject: [PATCH v1 0/4] Add a couple of enhancements to btmtksdio

From: Sean Wang <[email protected]>

First three are all minor changes and the final one adds a runtime pm support to
enter a lower power state for decreasing power dissipation when there is idle
traffic in SDIO transport for a while.

Sean Wang (4):
Bluetooth: btmtksdio: Drop newline with bt_dev logging macros
Bluetooth: btmtksdio: Add a bit definition for CHLPCR
Bluetooth: btmtksdio: Fix hdev->stat.byte_rx accumulation
Bluetooth: btmtksdio: Add runtime PM support to SDIO based Bluetooth

drivers/bluetooth/btmtksdio.c | 161 ++++++++++++++++++++++++++++++++--
1 file changed, 153 insertions(+), 8 deletions(-)

--
2.18.0


2019-04-18 09:09:57

by Sean Wang

[permalink] [raw]
Subject: [PATCH v1 3/4] Bluetooth: btmtksdio: Fix hdev->stat.byte_rx accumulation

From: Sean Wang <[email protected]>

Accumulate hdev->stat.byte_rx only for valid packets as btmtkuart doing.

Signed-off-by: Sean Wang <[email protected]>
---
drivers/bluetooth/btmtksdio.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.c
index 9c123a9de673..877c0a831775 100644
--- a/drivers/bluetooth/btmtksdio.c
+++ b/drivers/bluetooth/btmtksdio.c
@@ -391,8 +391,6 @@ static int btmtksdio_rx_packet(struct btmtksdio_dev *bdev, u16 rx_size)
if (err < 0)
goto err_kfree_skb;

- bdev->hdev->stat.byte_rx += rx_size;
-
sdio_hdr = (void *)skb->data;

/* We assume the default error as -EILSEQ simply to make the error path
@@ -457,6 +455,8 @@ static int btmtksdio_rx_packet(struct btmtksdio_dev *bdev, u16 rx_size)
/* Complete frame */
(&pkts[i])->recv(bdev->hdev, skb);

+ bdev->hdev->stat.byte_rx += rx_size;
+
return 0;

err_kfree_skb:
--
2.18.0

2019-04-18 09:09:57

by Sean Wang

[permalink] [raw]
Subject: [PATCH v1 4/4] Bluetooth: btmtksdio: Add runtime PM support to SDIO based Bluetooth

From: Sean Wang <[email protected]>

Add runtime PM support to btmtksdio. With this way, there will be the
benefit of the device entering the more power saving state once it is
been a while data traffic is idle.

Signed-off-by: Sean Wang <[email protected]>
---
drivers/bluetooth/btmtksdio.c | 144 ++++++++++++++++++++++++++++++++++
1 file changed, 144 insertions(+)

diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.c
index 877c0a831775..813338288453 100644
--- a/drivers/bluetooth/btmtksdio.c
+++ b/drivers/bluetooth/btmtksdio.c
@@ -17,6 +17,7 @@
#include <linux/iopoll.h>
#include <linux/kernel.h>
#include <linux/module.h>
+#include <linux/pm_runtime.h>
#include <linux/skbuff.h>

#include <linux/mmc/host.h>
@@ -33,6 +34,10 @@
#define FIRMWARE_MT7663 "mediatek/mt7663pr2h.bin"
#define FIRMWARE_MT7668 "mediatek/mt7668pr2h.bin"

+#define MTKBTSDIO_AUTOSUSPEND_DELAY 8000
+
+static bool enable_autosuspend;
+
struct btmtksdio_data {
const char *fwname;
};
@@ -150,6 +155,7 @@ struct btmtk_hci_wmt_params {
struct btmtksdio_dev {
struct hci_dev *hdev;
struct sdio_func *func;
+ struct device *dev;

struct work_struct tx_work;
unsigned long tx_state;
@@ -301,6 +307,8 @@ static void btmtksdio_tx_work(struct work_struct *work)
struct sk_buff *skb;
int err;

+ pm_runtime_get_sync(bdev->dev);
+
sdio_claim_host(bdev->func);

while ((skb = skb_dequeue(&bdev->txq))) {
@@ -313,6 +321,9 @@ static void btmtksdio_tx_work(struct work_struct *work)
}

sdio_release_host(bdev->func);
+
+ pm_runtime_mark_last_busy(bdev->dev);
+ pm_runtime_put_autosuspend(bdev->dev);
}

static int btmtksdio_recv_event(struct hci_dev *hdev, struct sk_buff *skb)
@@ -471,6 +482,18 @@ static void btmtksdio_interrupt(struct sdio_func *func)
u32 int_status;
u16 rx_size;

+ /* It is required that the host gets ownership from the device before
+ * accessing any register, however, if SDIO host is not being released,
+ * a potential deadlock probably happens in a circular wait between SDIO
+ * IRQ work and PM runtime work. So, we have to explicitly release SDIO
+ * host here and claim again after the PM runtime work is all done.
+ */
+ sdio_release_host(bdev->func);
+
+ pm_runtime_get_sync(bdev->dev);
+
+ sdio_claim_host(bdev->func);
+
/* Disable interrupt */
sdio_writel(func, C_INT_EN_CLR, MTK_REG_CHLPCR, 0);

@@ -507,6 +530,9 @@ static void btmtksdio_interrupt(struct sdio_func *func)

/* Enable interrupt */
sdio_writel(func, C_INT_EN_SET, MTK_REG_CHLPCR, 0);
+
+ pm_runtime_mark_last_busy(bdev->dev);
+ pm_runtime_put_autosuspend(bdev->dev);
}

static int btmtksdio_open(struct hci_dev *hdev)
@@ -815,6 +841,23 @@ static int btmtksdio_setup(struct hci_dev *hdev)
delta = ktime_sub(rettime, calltime);
duration = (unsigned long long)ktime_to_ns(delta) >> 10;

+ pm_runtime_set_autosuspend_delay(bdev->dev,
+ MTKBTSDIO_AUTOSUSPEND_DELAY);
+ pm_runtime_use_autosuspend(bdev->dev);
+
+ err = pm_runtime_set_active(bdev->dev);
+ if (err < 0)
+ return err;
+
+ /* Default forbid runtime auto suspend, that can be allowed by
+ * enable_autosuspend flag or the PM runtime entry under sysfs.
+ */
+ pm_runtime_forbid(bdev->dev);
+ pm_runtime_enable(bdev->dev);
+
+ if (enable_autosuspend)
+ pm_runtime_allow(bdev->dev);
+
bt_dev_info(hdev, "Device setup in %llu usecs", duration);

return 0;
@@ -822,10 +865,16 @@ static int btmtksdio_setup(struct hci_dev *hdev)

static int btmtksdio_shutdown(struct hci_dev *hdev)
{
+ struct btmtksdio_dev *bdev = hci_get_drvdata(hdev);
struct btmtk_hci_wmt_params wmt_params;
u8 param = 0x0;
int err;

+ /* Get back the state to be consistent with the state
+ * in btmtksdio_setup.
+ */
+ pm_runtime_get_sync(bdev->dev);
+
/* Disable the device */
wmt_params.op = MTK_WMT_FUNC_CTRL;
wmt_params.flag = 0;
@@ -839,6 +888,9 @@ static int btmtksdio_shutdown(struct hci_dev *hdev)
return err;
}

+ pm_runtime_put_noidle(bdev->dev);
+ pm_runtime_disable(bdev->dev);
+
return 0;
}

@@ -885,6 +937,7 @@ static int btmtksdio_probe(struct sdio_func *func,
if (!bdev->data)
return -ENODEV;

+ bdev->dev = &func->dev;
bdev->func = func;

INIT_WORK(&bdev->tx_work, btmtksdio_tx_work);
@@ -922,6 +975,25 @@ static int btmtksdio_probe(struct sdio_func *func,

sdio_set_drvdata(func, bdev);

+ /* pm_runtime_enable would be done after the firmware is being
+ * downloaded because the core layer probably already enables
+ * runtime PM for this func such as the case host->caps &
+ * MMC_CAP_POWER_OFF_CARD.
+ */
+ if (pm_runtime_enabled(bdev->dev))
+ pm_runtime_disable(bdev->dev);
+
+ /* As explaination in drivers/mmc/core/sdio_bus.c tells us:
+ * Unbound SDIO functions are always suspended.
+ * During probe, the function is set active and the usage count
+ * is incremented. If the driver supports runtime PM,
+ * it should call pm_runtime_put_noidle() in its probe routine and
+ * pm_runtime_get_noresume() in its remove routine.
+ *
+ * So, put a pm_runtime_put_noidle here !
+ */
+ pm_runtime_put_noidle(bdev->dev);
+
return 0;
}

@@ -933,6 +1005,9 @@ static void btmtksdio_remove(struct sdio_func *func)
if (!bdev)
return;

+ /* Be consistent the state in btmtksdio_probe */
+ pm_runtime_get_noresume(bdev->dev);
+
hdev = bdev->hdev;

sdio_set_drvdata(func, NULL);
@@ -940,15 +1015,84 @@ static void btmtksdio_remove(struct sdio_func *func)
hci_free_dev(hdev);
}

+#ifdef CONFIG_PM
+static int btmtksdio_runtime_suspend(struct device *dev)
+{
+ struct sdio_func *func = dev_to_sdio_func(dev);
+ struct btmtksdio_dev *bdev;
+ u32 status;
+ int err;
+
+ bdev = sdio_get_drvdata(func);
+ if (!bdev)
+ return 0;
+
+ sdio_claim_host(bdev->func);
+
+ sdio_writel(bdev->func, C_FW_OWN_REQ_SET, MTK_REG_CHLPCR, &err);
+ if (err < 0)
+ goto out;
+
+ err = readx_poll_timeout(btmtksdio_drv_own_query, bdev, status,
+ !(status & C_COM_DRV_OWN), 2000, 1000000);
+out:
+ bt_dev_info(bdev->hdev, "status (%d) return ownership to device", err);
+
+ sdio_release_host(bdev->func);
+
+ return err;
+}
+
+static int btmtksdio_runtime_resume(struct device *dev)
+{
+ struct sdio_func *func = dev_to_sdio_func(dev);
+ struct btmtksdio_dev *bdev;
+ u32 status;
+ int err;
+
+ bdev = sdio_get_drvdata(func);
+ if (!bdev)
+ return 0;
+
+ sdio_claim_host(bdev->func);
+
+ sdio_writel(bdev->func, C_FW_OWN_REQ_CLR, MTK_REG_CHLPCR, &err);
+ if (err < 0)
+ goto out;
+
+ err = readx_poll_timeout(btmtksdio_drv_own_query, bdev, status,
+ status & C_COM_DRV_OWN, 2000, 1000000);
+out:
+ bt_dev_info(bdev->hdev, "status (%d) get ownership from device", err);
+
+ sdio_release_host(bdev->func);
+
+ return err;
+}
+
+static UNIVERSAL_DEV_PM_OPS(btmtksdio_pm_ops, btmtksdio_runtime_suspend,
+ btmtksdio_runtime_resume, NULL);
+#define BTMTKSDIO_PM_OPS (&btmtksdio_pm_ops)
+#else /* CONFIG_PM */
+#define BTMTKSDIO_PM_OPS NULL
+#endif /* CONFIG_PM */
+
static struct sdio_driver btmtksdio_driver = {
.name = "btmtksdio",
.probe = btmtksdio_probe,
.remove = btmtksdio_remove,
.id_table = btmtksdio_table,
+ .drv = {
+ .owner = THIS_MODULE,
+ .pm = BTMTKSDIO_PM_OPS,
+ }
};

module_sdio_driver(btmtksdio_driver);

+module_param(enable_autosuspend, bool, 0644);
+MODULE_PARM_DESC(enable_autosuspend, "Enable autosuspend by default");
+
MODULE_AUTHOR("Sean Wang <[email protected]>");
MODULE_DESCRIPTION("MediaTek Bluetooth SDIO driver ver " VERSION);
MODULE_VERSION(VERSION);
--
2.18.0

2019-04-18 09:10:12

by Sean Wang

[permalink] [raw]
Subject: [PATCH v1 2/4] Bluetooth: btmtksdio: Add a bit definition for CHLPCR

From: Sean Wang <[email protected]>

Add a register bit definition about CHLPCR bit 8 because the bit is quite
different in the meaning between reading and writing that bit.

The patch adds a definition particularly for the bit read to avoid the
confusion about using write definition to read the bit.

Signed-off-by: Sean Wang <[email protected]>
---
drivers/bluetooth/btmtksdio.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.c
index 681e3e34977e..9c123a9de673 100644
--- a/drivers/bluetooth/btmtksdio.c
+++ b/drivers/bluetooth/btmtksdio.c
@@ -56,7 +56,8 @@ static const struct sdio_device_id btmtksdio_table[] = {
#define MTK_REG_CHLPCR 0x4 /* W1S */
#define C_INT_EN_SET BIT(0)
#define C_INT_EN_CLR BIT(1)
-#define C_FW_OWN_REQ_SET BIT(8)
+#define C_FW_OWN_REQ_SET BIT(8) /* For write */
+#define C_COM_DRV_OWN BIT(8) /* For read */
#define C_FW_OWN_REQ_CLR BIT(9)

#define MTK_REG_CSDIOCSR 0x8
@@ -526,7 +527,7 @@ static int btmtksdio_open(struct hci_dev *hdev)
goto err_disable_func;

err = readx_poll_timeout(btmtksdio_drv_own_query, bdev, status,
- status & C_FW_OWN_REQ_SET, 2000, 1000000);
+ status & C_COM_DRV_OWN, 2000, 1000000);
if (err < 0) {
bt_dev_err(bdev->hdev, "Cannot get ownership from device");
goto err_disable_func;
@@ -606,7 +607,7 @@ static int btmtksdio_close(struct hci_dev *hdev)
sdio_writel(bdev->func, C_FW_OWN_REQ_SET, MTK_REG_CHLPCR, NULL);

err = readx_poll_timeout(btmtksdio_drv_own_query, bdev, status,
- !(status & C_FW_OWN_REQ_SET), 2000, 1000000);
+ !(status & C_COM_DRV_OWN), 2000, 1000000);
if (err < 0)
bt_dev_err(bdev->hdev, "Cannot return ownership to device");

--
2.18.0

2019-04-18 09:10:28

by Sean Wang

[permalink] [raw]
Subject: [PATCH v1 1/4] Bluetooth: btmtksdio: Drop newline with bt_dev logging macros

From: Sean Wang <[email protected]>

bt_dev logging macros already include a newline at each output
so drop these unnecessary additional newlines in the driver.

Signed-off-by: Sean Wang <[email protected]>
---
drivers/bluetooth/btmtksdio.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.c
index 7d0d1cb93b0e..681e3e34977e 100644
--- a/drivers/bluetooth/btmtksdio.c
+++ b/drivers/bluetooth/btmtksdio.c
@@ -487,15 +487,15 @@ static void btmtksdio_interrupt(struct sdio_func *func)
sdio_writel(func, int_status, MTK_REG_CHISR, NULL);

if (unlikely(!int_status))
- bt_dev_err(bdev->hdev, "CHISR is 0\n");
+ bt_dev_err(bdev->hdev, "CHISR is 0");

if (int_status & FW_OWN_BACK_INT)
- bt_dev_dbg(bdev->hdev, "Get fw own back\n");
+ bt_dev_dbg(bdev->hdev, "Get fw own back");

if (int_status & TX_EMPTY)
schedule_work(&bdev->tx_work);
else if (unlikely(int_status & TX_FIFO_OVERFLOW))
- bt_dev_warn(bdev->hdev, "Tx fifo overflow\n");
+ bt_dev_warn(bdev->hdev, "Tx fifo overflow");

if (int_status & RX_DONE_INT) {
rx_size = (int_status & RX_PKT_LEN) >> 16;
--
2.18.0