[PATCH V3 0/2] scsi: ufs: Add support to control UFS device background operations

[PATCH V3 0/2] scsi: ufs: Add support to control UFS device background operations

[Sujit Reddy Thumma]

Add host assisted background operations for UFS device and runtime PM
helpers for ufshcd platform and pci glue drivers. The background operations
are disabled during runtime resume and enabled when the device is idle and
runtime suspended.

These patches depends on:
[PATCH V3 1/2] scsi: ufs: Add support for sending NOP OUT UPIU
[PATCH V3 2/2] scsi: ufs: Set fDeviceInit flag to initiate device initialization

Changes from v2:
	- Enable auto bkops by default explicitly during bootup so that we
	  may not assume it as enabled after reset.
	- Enable runtime PM support for contexts that are not part of SCSI,
	  so that the host is not suspended while running other contexts
	  like bkops exception handling.
Changes from v1:
	- Minor cleanup and rebase
	- Forced enable of auto bkops during initialization to make sure device
	  and driver state are matched.

Sujit Reddy Thumma (2):
  scsi: ufs: Add support for host assisted background operations
  scsi: ufs: Add runtime PM support for UFS host controller driver

 drivers/scsi/ufs/ufs.h           |   25 +++-
 drivers/scsi/ufs/ufshcd-pci.c    |   65 +++++++-
 drivers/scsi/ufs/ufshcd-pltfrm.c |   51 ++++++-
 drivers/scsi/ufs/ufshcd.c        |  346 ++++++++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufshcd.h        |   10 +
 5 files changed, 489 insertions(+), 8 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

[PATCH V3 1/2] scsi: ufs: Add support for host assisted background operations

[Sujit Reddy Thumma]

Background operations in the UFS device can be disabled by
the host to reduce the response latency of transfer requests.
Add support for enabling/disabling the background operations
during runtime suspend/resume of the device.

If the device is in critical need of BKOPS it will raise an
URGENT_BKOPS exception which should be handled by the host to
make sure the device performs as expected.

During bootup, the BKOPS is enabled in the device by default.
The disable of BKOPS is supported only when the driver supports
runtime suspend/resume operations as the runtime PM framework
provides a way to determine the device idleness and hence BKOPS
can be managed effectively. During runtime resume the BKOPS is
disabled to reduce latency and during runtime suspend the BKOPS
is enabled to allow device to carry out idle time BKOPS.

In some cases where the BKOPS is disabled during runtime resume
and due to continuous data transfers the runtime suspend is not
triggered, the BKOPS is enabled when the device raises a level-2
exception (outstanding operations - performance impact).

Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
---
 drivers/scsi/ufs/ufs.h    |   25 ++++-
 drivers/scsi/ufs/ufshcd.c |  338 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufshcd.h |   10 ++
 3 files changed, 372 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index db5bde4..549a652 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -107,7 +107,29 @@ enum {
 
 /* Flag idn for Query Requests*/
 enum flag_idn {
-	QUERY_FLAG_IDN_FDEVICEINIT = 0x01,
+	QUERY_FLAG_IDN_FDEVICEINIT	= 0x01,
+	QUERY_FLAG_IDN_BKOPS_EN		= 0x04,
+};
+
+/* Attribute idn for Query requests */
+enum attr_idn {
+	QUERY_ATTR_IDN_BKOPS_STATUS	= 0x05,
+	QUERY_ATTR_IDN_EE_CONTROL	= 0x0D,
+	QUERY_ATTR_IDN_EE_STATUS	= 0x0E,
+};
+
+/* Exception event mask values */
+enum {
+	MASK_EE_STATUS		= 0xFFFF,
+	MASK_EE_URGENT_BKOPS	= (1 << 2),
+};
+
+/* Background operation status */
+enum {
+	BKOPS_STATUS_NO_OP               = 0x0,
+	BKOPS_STATUS_NON_CRITICAL        = 0x1,
+	BKOPS_STATUS_PERF_IMPACT         = 0x2,
+	BKOPS_STATUS_CRITICAL            = 0x3,
 };
 
 /* UTP QUERY Transaction Specific Fields OpCode */
@@ -156,6 +178,7 @@ enum {
 	MASK_TASK_RESPONSE	= 0xFF00,
 	MASK_RSP_UPIU_RESULT	= 0xFFFF,
 	MASK_QUERY_DATA_SEG_LEN	= 0xFFFF,
+	MASK_RSP_EXCEPTION_EVENT = 0x10000,
 };
 
 /* Task management service response */
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 96ccb28..a25de66 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -268,6 +268,21 @@ ufshcd_get_rsp_upiu_result(struct utp_upiu_rsp *ucd_rsp_ptr)
 }
 
 /**
+ * ufshcd_is_exception_event - Check if the device raised an exception event
+ * @ucd_rsp_ptr: pointer to response UPIU
+ *
+ * The function checks if the device raised an exception event indicated in
+ * the Device Information field of response UPIU.
+ *
+ * Returns true if exception is raised, false otherwise.
+ */
+static inline bool ufshcd_is_exception_event(struct utp_upiu_rsp *ucd_rsp_ptr)
+{
+	return be32_to_cpu(ucd_rsp_ptr->header.dword_2) &
+			MASK_RSP_EXCEPTION_EVENT ? true : false;
+}
+
+/**
  * ufshcd_config_int_aggr - Configure interrupt aggregation values.
  *		Currently there is no use case where we want to configure
  *		interrupt aggregation dynamically. So to configure interrupt
@@ -1174,6 +1189,86 @@ out_no_mem:
 }
 
 /**
+ * ufshcd_query_attr - Helper function for composing attribute requests
+ * hba: per-adapter instance
+ * opcode: attribute opcode
+ * idn: attribute idn to access
+ * index: index field
+ * selector: selector field
+ * attr_val: the attribute value after the query request completes
+ *
+ * Returns 0 for success, non-zero in case of failure
+*/
+int ufshcd_query_attr(struct ufs_hba *hba, enum query_opcode opcode,
+			enum attr_idn idn, u8 index, u8 selector, u32 *attr_val)
+{
+	struct ufs_query_req *query;
+	struct ufs_query_res *response;
+	int err = -ENOMEM;
+
+	if (!attr_val) {
+		dev_err(hba->dev, "%s: attribute value required for write request\n",
+				__func__);
+		err = -EINVAL;
+		goto out;
+	}
+
+	query = kzalloc(sizeof(struct ufs_query_req), GFP_KERNEL);
+	if (!query) {
+		dev_err(hba->dev,
+			"%s: Failed allocating ufs_query_req instance\n",
+			__func__);
+		goto out;
+	}
+
+	response = kzalloc(sizeof(struct ufs_query_res), GFP_KERNEL);
+	if (!response) {
+		dev_err(hba->dev,
+			"%s: Failed allocating ufs_query_res instance\n",
+			__func__);
+		goto out_free_query;
+	}
+
+	switch (opcode) {
+	case UPIU_QUERY_OPCODE_WRITE_ATTR:
+		query->query_func = UPIU_QUERY_FUNC_STANDARD_WRITE_REQUEST;
+		query->upiu_req.value = *attr_val;
+		break;
+	case UPIU_QUERY_OPCODE_READ_ATTR:
+		query->query_func = UPIU_QUERY_FUNC_STANDARD_READ_REQUEST;
+		break;
+	default:
+		dev_err(hba->dev, "%s: Expected query attr opcode but got = 0x%.2x\n",
+				__func__, opcode);
+		err = -EINVAL;
+		goto out_free;
+	}
+
+	query->upiu_req.opcode = opcode;
+	query->upiu_req.idn = idn;
+	query->upiu_req.index = index;
+	query->upiu_req.selector = selector;
+
+	/* Send query request */
+	err = ufshcd_send_query_request(hba, query, NULL, response);
+
+	if (err) {
+		dev_err(hba->dev, "%s: opcode 0x%.2x for idn %d failed, err = %d\n",
+				__func__, opcode, idn, err);
+		goto out_free;
+	}
+
+	*attr_val = response->upiu_res.value;
+
+out_free:
+	kfree(response);
+out_free_query:
+	kfree(query);
+out:
+	return err;
+}
+
+/**
  * ufshcd_memory_alloc - allocate memory for host memory space data structures
  * @hba: per adapter instance
  *
@@ -1842,6 +1937,9 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 			 */
 			scsi_status = result & MASK_SCSI_STATUS;
 			result = ufshcd_scsi_cmd_status(lrbp, scsi_status);
+
+			if (ufshcd_is_exception_event(lrbp->ucd_rsp_ptr))
+				schedule_work(&hba->eeh_work);
 			break;
 		case UPIU_TRANSACTION_REJECT_UPIU:
 			/* TODO: handle Reject UPIU Response */
@@ -1942,6 +2040,215 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
 }
 
 /**
+ * ufshcd_disable_ee - disable exception event
+ * @hba: per-adapter instance
+ * @mask: exception event to disable
+ *
+ * Disables exception event in the device so that the EVENT_ALERT
+ * bit is not set.
+ *
+ * Returns zero on success, non-zero error value on failure.
+ */
+static int ufshcd_disable_ee(struct ufs_hba *hba, u16 mask)
+{
+	int err = 0;
+	u32 val;
+
+	if (!(hba->ee_ctrl_mask & mask))
+		goto out;
+
+	val = hba->ee_ctrl_mask & ~mask;
+	val &= 0xFFFF; /* 2 bytes */
+	err = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
+			QUERY_ATTR_IDN_EE_CONTROL, 0, 0, &val);
+	if (!err)
+		hba->ee_ctrl_mask &= ~mask;
+out:
+	return err;
+}
+
+/**
+ * ufshcd_enable_ee - enable exception event
+ * @hba: per-adapter instance
+ * @mask: exception event to enable
+ *
+ * Enable corresponding exception event in the device to allow
+ * device to alert host in critical scenarios.
+ *
+ * Returns zero on success, non-zero error value on failure.
+ */
+static int ufshcd_enable_ee(struct ufs_hba *hba, u16 mask)
+{
+	int err = 0;
+	u32 val;
+
+	if (hba->ee_ctrl_mask & mask)
+		goto out;
+
+	val = hba->ee_ctrl_mask | mask;
+	val &= 0xFFFF; /* 2 bytes */
+	err = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
+			QUERY_ATTR_IDN_EE_CONTROL, 0, 0, &val);
+	if (!err)
+		hba->ee_ctrl_mask |= mask;
+out:
+	return err;
+}
+
+/**
+ * ufshcd_enable_auto_bkops - Allow device managed BKOPS
+ * @hba: per-adapter instance
+ *
+ * Allow device to manage background operations on its own. Enabling
+ * this might lead to inconsistent latencies during normal data transfers
+ * as the device is allowed to manage its own way of handling background
+ * operations.
+ *
+ * Returns zero on success, non-zero on failure.
+ */
+static int ufshcd_enable_auto_bkops(struct ufs_hba *hba)
+{
+	int err = 0;
+
+	if (hba->auto_bkops_enabled)
+		goto out;
+
+	err = ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_SET_FLAG,
+			QUERY_FLAG_IDN_BKOPS_EN, NULL);
+	if (err) {
+		dev_err(hba->dev, "%s: failed to enable bkops %d\n",
+				__func__, err);
+		goto out;
+	}
+
+	hba->auto_bkops_enabled = true;
+
+	/* No need of URGENT_BKOPS exception from the device */
+	err = ufshcd_disable_ee(hba, MASK_EE_URGENT_BKOPS);
+	if (err)
+		dev_err(hba->dev, "%s: failed to disable exception event %d\n",
+				__func__, err);
+out:
+	return err;
+}
+
+/**
+ * ufshcd_disable_auto_bkops - block device in doing background operations
+ * @hba: per-adapter instance
+ *
+ * Disabling background operations improves command response latency but
+ * has drawback of device moving into critical state where the device is
+ * not-operable. Make sure to call ufshcd_enable_auto_bkops() whenever the
+ * host is idle so that BKOPS are managed effectively without any negative
+ * impacts.
+ *
+ * Returns zero on success, non-zero on failure.
+ */
+static int ufshcd_disable_auto_bkops(struct ufs_hba *hba)
+{
+	int err = 0;
+
+	if (!hba->auto_bkops_enabled)
+		goto out;
+
+	/*
+	 * If host assisted BKOPs is to be enabled, make sure
+	 * urgent bkops exception is allowed.
+	 */
+	err = ufshcd_enable_ee(hba, MASK_EE_URGENT_BKOPS);
+	if (err) {
+		dev_err(hba->dev, "%s: failed to enable exception event %d\n",
+				__func__, err);
+		goto out;
+	}
+
+	err = ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_CLEAR_FLAG,
+			QUERY_FLAG_IDN_BKOPS_EN, NULL);
+	if (err) {
+		dev_err(hba->dev, "%s: failed to disable bkops %d\n",
+				__func__, err);
+		ufshcd_disable_ee(hba, MASK_EE_URGENT_BKOPS);
+		goto out;
+	}
+
+	hba->auto_bkops_enabled = false;
+out:
+	return err;
+}
+
+static inline int ufshcd_get_bkops_status(struct ufs_hba *hba, u32 *status)
+{
+	return ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+			QUERY_ATTR_IDN_BKOPS_STATUS, 0, 0, status);
+}
+
+/**
+ * ufshcd_urgent_bkops - handle urgent bkops exception event
+ * @hba: per-adapter instance
+ *
+ * Enable fBackgroundOpsEn flag in the device to permit background
+ * operations.
+ */
+static int ufshcd_urgent_bkops(struct ufs_hba *hba)
+{
+	int err;
+	u32 status = 0;
+
+	err = ufshcd_get_bkops_status(hba, &status);
+	if (err) {
+		dev_err(hba->dev, "%s: failed to get BKOPS status %d\n",
+				__func__, err);
+		goto out;
+	}
+
+	status = status & 0xF;
+
+	/* handle only if status indicates performance impact or critical */
+	if (status >= BKOPS_STATUS_PERF_IMPACT)
+		err = ufshcd_enable_auto_bkops(hba);
+out:
+	return err;
+}
+
+static inline int ufshcd_get_ee_status(struct ufs_hba *hba, u32 *status)
+{
+	return ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+			QUERY_ATTR_IDN_EE_STATUS, 0, 0, status);
+}
+
+/**
+ * ufshcd_exception_event_handler - handle exceptions raised by device
+ * @work: pointer to work data
+ *
+ * Read bExceptionEventStatus attribute from the device and handle the
+ * exception event accordingly.
+ */
+static void ufshcd_exception_event_handler(struct work_struct *work)
+{
+	struct ufs_hba *hba;
+	int err;
+	u32 status = 0;
+	hba = container_of(work, struct ufs_hba, eeh_work);
+
+	err = ufshcd_get_ee_status(hba, &status);
+	if (err) {
+		dev_err(hba->dev, "%s: failed to get exception status %d\n",
+				__func__, err);
+		goto out;
+	}
+
+	status &= hba->ee_ctrl_mask;
+	if (status & MASK_EE_URGENT_BKOPS) {
+		err = ufshcd_urgent_bkops(hba);
+		if (err)
+			dev_err(hba->dev, "%s: failed to handle urgent bkops %d\n",
+					__func__, err);
+	}
+out:
+	return;
+}
+
+/**
  * ufshcd_fatal_err_handler - handle fatal errors
  * @hba: per adapter instance
  */
@@ -2252,6 +2559,8 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
 	if (ret)
 		goto out;
 
+	hba->auto_bkops_enabled = false;
+	ufshcd_enable_auto_bkops(hba);
 	scsi_scan_host(hba->host);
 out:
 	return;
@@ -2316,6 +2625,34 @@ int ufshcd_resume(struct ufs_hba *hba)
 }
 EXPORT_SYMBOL_GPL(ufshcd_resume);
 
+int ufshcd_runtime_suspend(struct ufs_hba *hba)
+{
+	if (!hba)
+		return 0;
+
+	/*
+	 * The device is idle with no requests in the queue,
+	 * allow background operations.
+	 */
+	return ufshcd_enable_auto_bkops(hba);
+}
+EXPORT_SYMBOL(ufshcd_runtime_suspend);
+
+int ufshcd_runtime_resume(struct ufs_hba *hba)
+{
+	if (!hba)
+		return 0;
+
+	return ufshcd_disable_auto_bkops(hba);
+}
+EXPORT_SYMBOL(ufshcd_runtime_resume);
+
+int ufshcd_runtime_idle(struct ufs_hba *hba)
+{
+	return 0;
+}
+EXPORT_SYMBOL(ufshcd_runtime_idle);
+
 /**
  * ufshcd_remove - de-allocate SCSI host and host memory space
  *		data structure memory
@@ -2406,6 +2743,7 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
 
 	/* Initialize work queues */
 	INIT_WORK(&hba->feh_workq, ufshcd_fatal_err_handler);
+	INIT_WORK(&hba->eeh_work, ufshcd_exception_event_handler);
 
 	/* Initialize UIC command mutex */
 	mutex_init(&hba->uic_cmd_mutex);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index c6aeb6d..6c9bd35 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -178,9 +178,12 @@ struct ufs_dev_cmd {
  * @tm_condition: condition variable for task management
  * @ufshcd_state: UFSHCD states
  * @intr_mask: Interrupt Mask Bits
+ * @ee_ctrl_mask: Exception event control mask
  * @feh_workq: Work queue for fatal controller error handling
+ * @eeh_work: Worker to handle exception events
  * @errors: HBA errors
  * @dev_cmd: ufs device management command information
+ * @auto_bkops_enabled: to track whether bkops is enabled in device
  */
 struct ufs_hba {
 	void __iomem *mmio_base;
@@ -218,15 +221,19 @@ struct ufs_hba {
 
 	u32 ufshcd_state;
 	u32 intr_mask;
+	u16 ee_ctrl_mask;
 
 	/* Work Queues */
 	struct work_struct feh_workq;
+	struct work_struct eeh_work;
 
 	/* HBA Errors */
 	u32 errors;
 
 	/* Device management request data */
 	struct ufs_dev_cmd dev_cmd;
+
+	bool auto_bkops_enabled;
 };
 
 #define ufshcd_writel(hba, val, reg)	\
@@ -247,4 +254,7 @@ static inline void ufshcd_hba_stop(struct ufs_hba *hba)
 	ufshcd_writel(hba, CONTROLLER_DISABLE,  REG_CONTROLLER_ENABLE);
 }
 
+extern int ufshcd_runtime_suspend(struct ufs_hba *hba);
+extern int ufshcd_runtime_resume(struct ufs_hba *hba);
+extern int ufshcd_runtime_idle(struct ufs_hba *hba);
 #endif /* End of Header */
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

[PATCH V3 2/2] scsi: ufs: Add runtime PM support for UFS host controller driver

[Sujit Reddy Thumma]

Add runtime PM helpers to suspend/resume UFS controller at runtime.
Enable runtime PM by default for pci and platform drivers as the
initialized hardware can suspend if it is not used after bootup.

Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd-pci.c    |   65 ++++++++++++++++++++++++++++++++++---
 drivers/scsi/ufs/ufshcd-pltfrm.c |   51 +++++++++++++++++++++++++++++-
 drivers/scsi/ufs/ufshcd.c        |    8 +++++
 3 files changed, 117 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd-pci.c b/drivers/scsi/ufs/ufshcd-pci.c
index 48be39a..7bd8faa 100644
--- a/drivers/scsi/ufs/ufshcd-pci.c
+++ b/drivers/scsi/ufs/ufshcd-pci.c
@@ -35,6 +35,7 @@
 
 #include "ufshcd.h"
 #include <linux/pci.h>
+#include <linux/pm_runtime.h>
 
 #ifdef CONFIG_PM
 /**
@@ -44,7 +45,7 @@
  *
  * Returns -ENOSYS
  */
-static int ufshcd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
+static int ufshcd_pci_suspend(struct device *dev)
 {
 	/*
 	 * TODO:
@@ -61,7 +62,7 @@ static int ufshcd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
  *
  * Returns -ENOSYS
  */
-static int ufshcd_pci_resume(struct pci_dev *pdev)
+static int ufshcd_pci_resume(struct device *dev)
 {
 	/*
 	 * TODO:
@@ -71,8 +72,48 @@ static int ufshcd_pci_resume(struct pci_dev *pdev)
 
 	return -ENOSYS;
 }
+#else
+#define ufshcd_pci_suspend	NULL
+#define ufshcd_pci_resume	NULL
 #endif /* CONFIG_PM */
 
+#ifdef CONFIG_PM_RUNTIME
+static int ufshcd_pci_runtime_suspend(struct device *dev)
+{
+	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
+	struct ufs_hba *hba = pci_get_drvdata(pdev);
+
+	if (!hba)
+		return 0;
+
+	return ufshcd_runtime_suspend(hba);
+}
+static int ufshcd_pci_runtime_resume(struct device *dev)
+{
+	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
+	struct ufs_hba *hba = pci_get_drvdata(pdev);
+
+	if (!hba)
+		return 0;
+
+	return ufshcd_runtime_resume(hba);
+}
+static int ufshcd_pci_runtime_idle(struct device *dev)
+{
+	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
+	struct ufs_hba *hba = pci_get_drvdata(pdev);
+
+	if (!hba)
+		return 0;
+
+	return ufshcd_runtime_idle(hba);
+}
+#else /* !CONFIG_PM_RUNTIME */
+#define ufshcd_pci_runtime_suspend	NULL
+#define ufshcd_pci_runtime_resume	NULL
+#define ufshcd_pci_runtime_idle	NULL
+#endif /* CONFIG_PM_RUNTIME */
+
 /**
  * ufshcd_pci_shutdown - main function to put the controller in reset state
  * @pdev: pointer to PCI device handle
@@ -91,6 +132,9 @@ static void ufshcd_pci_remove(struct pci_dev *pdev)
 {
 	struct ufs_hba *hba = pci_get_drvdata(pdev);
 
+	pm_runtime_forbid(&pdev->dev);
+	pm_runtime_get_noresume(&pdev->dev);
+
 	disable_irq(pdev->irq);
 	ufshcd_remove(hba);
 	pci_release_regions(pdev);
@@ -168,6 +212,8 @@ ufshcd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	}
 
 	pci_set_drvdata(pdev, hba);
+	pm_runtime_put_noidle(&pdev->dev);
+	pm_runtime_allow(&pdev->dev);
 
 	return 0;
 
@@ -182,6 +228,14 @@ out_error:
 	return err;
 }
 
+static const struct dev_pm_ops ufshcd_pci_pm_ops = {
+	.suspend	= ufshcd_pci_suspend,
+	.resume		= ufshcd_pci_resume,
+	.runtime_suspend = ufshcd_pci_runtime_suspend,
+	.runtime_resume  = ufshcd_pci_runtime_resume,
+	.runtime_idle    = ufshcd_pci_runtime_idle,
+};
+
 static DEFINE_PCI_DEVICE_TABLE(ufshcd_pci_tbl) = {
 	{ PCI_VENDOR_ID_SAMSUNG, 0xC00C, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
 	{ }	/* terminate list */
@@ -195,10 +249,9 @@ static struct pci_driver ufshcd_pci_driver = {
 	.probe = ufshcd_pci_probe,
 	.remove = ufshcd_pci_remove,
 	.shutdown = ufshcd_pci_shutdown,
-#ifdef CONFIG_PM
-	.suspend = ufshcd_pci_suspend,
-	.resume = ufshcd_pci_resume,
-#endif
+	.driver = {
+		.pm = &ufshcd_pci_pm_ops
+	},
 };
 
 module_pci_driver(ufshcd_pci_driver);
diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
index c42db40..b1f2605 100644
--- a/drivers/scsi/ufs/ufshcd-pltfrm.c
+++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
@@ -34,6 +34,7 @@
  */
 
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 
 #include "ufshcd.h"
 
@@ -87,6 +88,43 @@ static int ufshcd_pltfrm_resume(struct device *dev)
 #define ufshcd_pltfrm_resume	NULL
 #endif
 
+#ifdef CONFIG_PM_RUNTIME
+static int ufshcd_pltfrm_runtime_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct ufs_hba *hba =  platform_get_drvdata(pdev);
+
+	if (!hba)
+		return 0;
+
+	return ufshcd_runtime_suspend(hba);
+}
+static int ufshcd_pltfrm_runtime_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct ufs_hba *hba =  platform_get_drvdata(pdev);
+
+	if (!hba)
+		return 0;
+
+	return ufshcd_runtime_resume(hba);
+}
+static int ufshcd_pltfrm_runtime_idle(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct ufs_hba *hba =  platform_get_drvdata(pdev);
+
+	if (!hba)
+		return 0;
+
+	return ufshcd_runtime_idle(hba);
+}
+#else /* !CONFIG_PM_RUNTIME */
+#define ufshcd_pltfrm_runtime_suspend	NULL
+#define ufshcd_pltfrm_runtime_resume	NULL
+#define ufshcd_pltfrm_runtime_idle	NULL
+#endif /* CONFIG_PM_RUNTIME */
+
 /**
  * ufshcd_pltfrm_probe - probe routine of the driver
  * @pdev: pointer to Platform device handle
@@ -122,14 +160,20 @@ static int ufshcd_pltfrm_probe(struct platform_device *pdev)
 		goto out;
 	}
 
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
 	err = ufshcd_init(dev, &hba, mmio_base, irq);
 	if (err) {
 		dev_err(dev, "Intialization failed\n");
-		goto out;
+		goto out_disable_rpm;
 	}
 
 	platform_set_drvdata(pdev, hba);
 
+out_disable_rpm:
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_set_suspended(&pdev->dev);
 out:
 	return err;
 }
@@ -144,6 +188,8 @@ static int ufshcd_pltfrm_remove(struct platform_device *pdev)
 {
 	struct ufs_hba *hba =  platform_get_drvdata(pdev);
 
+	pm_runtime_get_sync(&(pdev)->dev);
+
 	disable_irq(hba->irq);
 	ufshcd_remove(hba);
 	return 0;
@@ -157,6 +203,9 @@ static const struct of_device_id ufs_of_match[] = {
 static const struct dev_pm_ops ufshcd_dev_pm_ops = {
 	.suspend	= ufshcd_pltfrm_suspend,
 	.resume		= ufshcd_pltfrm_resume,
+	.runtime_suspend = ufshcd_pltfrm_runtime_suspend,
+	.runtime_resume  = ufshcd_pltfrm_runtime_resume,
+	.runtime_idle    = ufshcd_pltfrm_runtime_idle,
 };
 
 static struct platform_driver ufshcd_pltfrm_driver = {
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index a25de66..af7d01d 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2230,6 +2230,7 @@ static void ufshcd_exception_event_handler(struct work_struct *work)
 	u32 status = 0;
 	hba = container_of(work, struct ufs_hba, eeh_work);
 
+	pm_runtime_get_sync(hba->dev);
 	err = ufshcd_get_ee_status(hba, &status);
 	if (err) {
 		dev_err(hba->dev, "%s: failed to get exception status %d\n",
@@ -2245,6 +2246,7 @@ static void ufshcd_exception_event_handler(struct work_struct *work)
 					__func__, err);
 	}
 out:
+	pm_runtime_put_sync(hba->dev);
 	return;
 }
 
@@ -2257,9 +2259,11 @@ static void ufshcd_fatal_err_handler(struct work_struct *work)
 	struct ufs_hba *hba;
 	hba = container_of(work, struct ufs_hba, feh_workq);
 
+	pm_runtime_get_sync(hba->dev);
 	/* check if reset is already in progress */
 	if (hba->ufshcd_state != UFSHCD_STATE_RESET)
 		ufshcd_do_reset(hba);
+	pm_runtime_put_sync(hba->dev);
 }
 
 /**
@@ -2562,6 +2566,7 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
 	hba->auto_bkops_enabled = false;
 	ufshcd_enable_auto_bkops(hba);
 	scsi_scan_host(hba->host);
+	pm_runtime_put_sync(hba->dev);
 out:
 	return;
 }
@@ -2783,6 +2788,9 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
 
 	*hba_handle = hba;
 
+	/* Hold auto suspend until async scan completes */
+	pm_runtime_get_sync(dev);
+
 	async_schedule(ufshcd_async_scan, hba);
 
 	return 0;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH V3 1/2] scsi: ufs: Add support for host assisted background operations

[merez]

Tested-by: Maya Erez <merez@codeaurora.org>

> Background operations in the UFS device can be disabled by
> the host to reduce the response latency of transfer requests.
> Add support for enabling/disabling the background operations
> during runtime suspend/resume of the device.
>
> If the device is in critical need of BKOPS it will raise an
> URGENT_BKOPS exception which should be handled by the host to
> make sure the device performs as expected.
>
> During bootup, the BKOPS is enabled in the device by default.
> The disable of BKOPS is supported only when the driver supports
> runtime suspend/resume operations as the runtime PM framework
> provides a way to determine the device idleness and hence BKOPS
> can be managed effectively. During runtime resume the BKOPS is
> disabled to reduce latency and during runtime suspend the BKOPS
> is enabled to allow device to carry out idle time BKOPS.
>
> In some cases where the BKOPS is disabled during runtime resume
> and due to continuous data transfers the runtime suspend is not
> triggered, the BKOPS is enabled when the device raises a level-2
> exception (outstanding operations - performance impact).
>
> Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufs.h    |   25 ++++-
>  drivers/scsi/ufs/ufshcd.c |  338
> +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/scsi/ufs/ufshcd.h |   10 ++
>  3 files changed, 372 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index db5bde4..549a652 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -107,7 +107,29 @@ enum {
>
>  /* Flag idn for Query Requests*/
>  enum flag_idn {
> -	QUERY_FLAG_IDN_FDEVICEINIT = 0x01,
> +	QUERY_FLAG_IDN_FDEVICEINIT	= 0x01,
> +	QUERY_FLAG_IDN_BKOPS_EN		= 0x04,
> +};
> +
> +/* Attribute idn for Query requests */
> +enum attr_idn {
> +	QUERY_ATTR_IDN_BKOPS_STATUS	= 0x05,
> +	QUERY_ATTR_IDN_EE_CONTROL	= 0x0D,
> +	QUERY_ATTR_IDN_EE_STATUS	= 0x0E,
> +};
> +
> +/* Exception event mask values */
> +enum {
> +	MASK_EE_STATUS		= 0xFFFF,
> +	MASK_EE_URGENT_BKOPS	= (1 << 2),
> +};
> +
> +/* Background operation status */
> +enum {
> +	BKOPS_STATUS_NO_OP               = 0x0,
> +	BKOPS_STATUS_NON_CRITICAL        = 0x1,
> +	BKOPS_STATUS_PERF_IMPACT         = 0x2,
> +	BKOPS_STATUS_CRITICAL            = 0x3,
>  };
>
>  /* UTP QUERY Transaction Specific Fields OpCode */
> @@ -156,6 +178,7 @@ enum {
>  	MASK_TASK_RESPONSE	= 0xFF00,
>  	MASK_RSP_UPIU_RESULT	= 0xFFFF,
>  	MASK_QUERY_DATA_SEG_LEN	= 0xFFFF,
> +	MASK_RSP_EXCEPTION_EVENT = 0x10000,
>  };
>
>  /* Task management service response */
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 96ccb28..a25de66 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -268,6 +268,21 @@ ufshcd_get_rsp_upiu_result(struct utp_upiu_rsp
> *ucd_rsp_ptr)
>  }
>
>  /**
> + * ufshcd_is_exception_event - Check if the device raised an exception
> event
> + * @ucd_rsp_ptr: pointer to response UPIU
> + *
> + * The function checks if the device raised an exception event indicated
> in
> + * the Device Information field of response UPIU.
> + *
> + * Returns true if exception is raised, false otherwise.
> + */
> +static inline bool ufshcd_is_exception_event(struct utp_upiu_rsp
> *ucd_rsp_ptr)
> +{
> +	return be32_to_cpu(ucd_rsp_ptr->header.dword_2) &
> +			MASK_RSP_EXCEPTION_EVENT ? true : false;
> +}
> +
> +/**
>   * ufshcd_config_int_aggr - Configure interrupt aggregation values.
>   *		Currently there is no use case where we want to configure
>   *		interrupt aggregation dynamically. So to configure interrupt
> @@ -1174,6 +1189,86 @@ out_no_mem:
>  }
>
>  /**
> + * ufshcd_query_attr - Helper function for composing attribute requests
> + * hba: per-adapter instance
> + * opcode: attribute opcode
> + * idn: attribute idn to access
> + * index: index field
> + * selector: selector field
> + * attr_val: the attribute value after the query request completes
> + *
> + * Returns 0 for success, non-zero in case of failure
> +*/
> +int ufshcd_query_attr(struct ufs_hba *hba, enum query_opcode opcode,
> +			enum attr_idn idn, u8 index, u8 selector, u32 *attr_val)
> +{
> +	struct ufs_query_req *query;
> +	struct ufs_query_res *response;
> +	int err = -ENOMEM;
> +
> +	if (!attr_val) {
> +		dev_err(hba->dev, "%s: attribute value required for write request\n",
> +				__func__);
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
> +	query = kzalloc(sizeof(struct ufs_query_req), GFP_KERNEL);
> +	if (!query) {
> +		dev_err(hba->dev,
> +			"%s: Failed allocating ufs_query_req instance\n",
> +			__func__);
> +		goto out;
> +	}
> +
> +	response = kzalloc(sizeof(struct ufs_query_res), GFP_KERNEL);
> +	if (!response) {
> +		dev_err(hba->dev,
> +			"%s: Failed allocating ufs_query_res instance\n",
> +			__func__);
> +		goto out_free_query;
> +	}
> +
> +	switch (opcode) {
> +	case UPIU_QUERY_OPCODE_WRITE_ATTR:
> +		query->query_func = UPIU_QUERY_FUNC_STANDARD_WRITE_REQUEST;
> +		query->upiu_req.value = *attr_val;
> +		break;
> +	case UPIU_QUERY_OPCODE_READ_ATTR:
> +		query->query_func = UPIU_QUERY_FUNC_STANDARD_READ_REQUEST;
> +		break;
> +	default:
> +		dev_err(hba->dev, "%s: Expected query attr opcode but got = 0x%.2x\n",
> +				__func__, opcode);
> +		err = -EINVAL;
> +		goto out_free;
> +	}
> +
> +	query->upiu_req.opcode = opcode;
> +	query->upiu_req.idn = idn;
> +	query->upiu_req.index = index;
> +	query->upiu_req.selector = selector;
> +
> +	/* Send query request */
> +	err = ufshcd_send_query_request(hba, query, NULL, response);
> +
> +	if (err) {
> +		dev_err(hba->dev, "%s: opcode 0x%.2x for idn %d failed, err = %d\n",
> +				__func__, opcode, idn, err);
> +		goto out_free;
> +	}
> +
> +	*attr_val = response->upiu_res.value;
> +
> +out_free:
> +	kfree(response);
> +out_free_query:
> +	kfree(query);
> +out:
> +	return err;
> +}
> +
> +/**
>   * ufshcd_memory_alloc - allocate memory for host memory space data
> structures
>   * @hba: per adapter instance
>   *
> @@ -1842,6 +1937,9 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba,
> struct ufshcd_lrb *lrbp)
>  			 */
>  			scsi_status = result & MASK_SCSI_STATUS;
>  			result = ufshcd_scsi_cmd_status(lrbp, scsi_status);
> +
> +			if (ufshcd_is_exception_event(lrbp->ucd_rsp_ptr))
> +				schedule_work(&hba->eeh_work);
>  			break;
>  		case UPIU_TRANSACTION_REJECT_UPIU:
>  			/* TODO: handle Reject UPIU Response */
> @@ -1942,6 +2040,215 @@ static void ufshcd_transfer_req_compl(struct
> ufs_hba *hba)
>  }
>
>  /**
> + * ufshcd_disable_ee - disable exception event
> + * @hba: per-adapter instance
> + * @mask: exception event to disable
> + *
> + * Disables exception event in the device so that the EVENT_ALERT
> + * bit is not set.
> + *
> + * Returns zero on success, non-zero error value on failure.
> + */
> +static int ufshcd_disable_ee(struct ufs_hba *hba, u16 mask)
> +{
> +	int err = 0;
> +	u32 val;
> +
> +	if (!(hba->ee_ctrl_mask & mask))
> +		goto out;
> +
> +	val = hba->ee_ctrl_mask & ~mask;
> +	val &= 0xFFFF; /* 2 bytes */
> +	err = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> +			QUERY_ATTR_IDN_EE_CONTROL, 0, 0, &val);
> +	if (!err)
> +		hba->ee_ctrl_mask &= ~mask;
> +out:
> +	return err;
> +}
> +
> +/**
> + * ufshcd_enable_ee - enable exception event
> + * @hba: per-adapter instance
> + * @mask: exception event to enable
> + *
> + * Enable corresponding exception event in the device to allow
> + * device to alert host in critical scenarios.
> + *
> + * Returns zero on success, non-zero error value on failure.
> + */
> +static int ufshcd_enable_ee(struct ufs_hba *hba, u16 mask)
> +{
> +	int err = 0;
> +	u32 val;
> +
> +	if (hba->ee_ctrl_mask & mask)
> +		goto out;
> +
> +	val = hba->ee_ctrl_mask | mask;
> +	val &= 0xFFFF; /* 2 bytes */
> +	err = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> +			QUERY_ATTR_IDN_EE_CONTROL, 0, 0, &val);
> +	if (!err)
> +		hba->ee_ctrl_mask |= mask;
> +out:
> +	return err;
> +}
> +
> +/**
> + * ufshcd_enable_auto_bkops - Allow device managed BKOPS
> + * @hba: per-adapter instance
> + *
> + * Allow device to manage background operations on its own. Enabling
> + * this might lead to inconsistent latencies during normal data transfers
> + * as the device is allowed to manage its own way of handling background
> + * operations.
> + *
> + * Returns zero on success, non-zero on failure.
> + */
> +static int ufshcd_enable_auto_bkops(struct ufs_hba *hba)
> +{
> +	int err = 0;
> +
> +	if (hba->auto_bkops_enabled)
> +		goto out;
> +
> +	err = ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_SET_FLAG,
> +			QUERY_FLAG_IDN_BKOPS_EN, NULL);
> +	if (err) {
> +		dev_err(hba->dev, "%s: failed to enable bkops %d\n",
> +				__func__, err);
> +		goto out;
> +	}
> +
> +	hba->auto_bkops_enabled = true;
> +
> +	/* No need of URGENT_BKOPS exception from the device */
> +	err = ufshcd_disable_ee(hba, MASK_EE_URGENT_BKOPS);
> +	if (err)
> +		dev_err(hba->dev, "%s: failed to disable exception event %d\n",
> +				__func__, err);
> +out:
> +	return err;
> +}
> +
> +/**
> + * ufshcd_disable_auto_bkops - block device in doing background
> operations
> + * @hba: per-adapter instance
> + *
> + * Disabling background operations improves command response latency but
> + * has drawback of device moving into critical state where the device is
> + * not-operable. Make sure to call ufshcd_enable_auto_bkops() whenever
> the
> + * host is idle so that BKOPS are managed effectively without any
> negative
> + * impacts.
> + *
> + * Returns zero on success, non-zero on failure.
> + */
> +static int ufshcd_disable_auto_bkops(struct ufs_hba *hba)
> +{
> +	int err = 0;
> +
> +	if (!hba->auto_bkops_enabled)
> +		goto out;
> +
> +	/*
> +	 * If host assisted BKOPs is to be enabled, make sure
> +	 * urgent bkops exception is allowed.
> +	 */
> +	err = ufshcd_enable_ee(hba, MASK_EE_URGENT_BKOPS);
> +	if (err) {
> +		dev_err(hba->dev, "%s: failed to enable exception event %d\n",
> +				__func__, err);
> +		goto out;
> +	}
> +
> +	err = ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_CLEAR_FLAG,
> +			QUERY_FLAG_IDN_BKOPS_EN, NULL);
> +	if (err) {
> +		dev_err(hba->dev, "%s: failed to disable bkops %d\n",
> +				__func__, err);
> +		ufshcd_disable_ee(hba, MASK_EE_URGENT_BKOPS);
> +		goto out;
> +	}
> +
> +	hba->auto_bkops_enabled = false;
> +out:
> +	return err;
> +}
> +
> +static inline int ufshcd_get_bkops_status(struct ufs_hba *hba, u32
> *status)
> +{
> +	return ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> +			QUERY_ATTR_IDN_BKOPS_STATUS, 0, 0, status);
> +}
> +
> +/**
> + * ufshcd_urgent_bkops - handle urgent bkops exception event
> + * @hba: per-adapter instance
> + *
> + * Enable fBackgroundOpsEn flag in the device to permit background
> + * operations.
> + */
> +static int ufshcd_urgent_bkops(struct ufs_hba *hba)
> +{
> +	int err;
> +	u32 status = 0;
> +
> +	err = ufshcd_get_bkops_status(hba, &status);
> +	if (err) {
> +		dev_err(hba->dev, "%s: failed to get BKOPS status %d\n",
> +				__func__, err);
> +		goto out;
> +	}
> +
> +	status = status & 0xF;
> +
> +	/* handle only if status indicates performance impact or critical */
> +	if (status >= BKOPS_STATUS_PERF_IMPACT)
> +		err = ufshcd_enable_auto_bkops(hba);
> +out:
> +	return err;
> +}
> +
> +static inline int ufshcd_get_ee_status(struct ufs_hba *hba, u32 *status)
> +{
> +	return ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> +			QUERY_ATTR_IDN_EE_STATUS, 0, 0, status);
> +}
> +
> +/**
> + * ufshcd_exception_event_handler - handle exceptions raised by device
> + * @work: pointer to work data
> + *
> + * Read bExceptionEventStatus attribute from the device and handle the
> + * exception event accordingly.
> + */
> +static void ufshcd_exception_event_handler(struct work_struct *work)
> +{
> +	struct ufs_hba *hba;
> +	int err;
> +	u32 status = 0;
> +	hba = container_of(work, struct ufs_hba, eeh_work);
> +
> +	err = ufshcd_get_ee_status(hba, &status);
> +	if (err) {
> +		dev_err(hba->dev, "%s: failed to get exception status %d\n",
> +				__func__, err);
> +		goto out;
> +	}
> +
> +	status &= hba->ee_ctrl_mask;
> +	if (status & MASK_EE_URGENT_BKOPS) {
> +		err = ufshcd_urgent_bkops(hba);
> +		if (err)
> +			dev_err(hba->dev, "%s: failed to handle urgent bkops %d\n",
> +					__func__, err);
> +	}
> +out:
> +	return;
> +}
> +
> +/**
>   * ufshcd_fatal_err_handler - handle fatal errors
>   * @hba: per adapter instance
>   */
> @@ -2252,6 +2559,8 @@ static void ufshcd_async_scan(void *data,
> async_cookie_t cookie)
>  	if (ret)
>  		goto out;
>
> +	hba->auto_bkops_enabled = false;
> +	ufshcd_enable_auto_bkops(hba);
>  	scsi_scan_host(hba->host);
>  out:
>  	return;
> @@ -2316,6 +2625,34 @@ int ufshcd_resume(struct ufs_hba *hba)
>  }
>  EXPORT_SYMBOL_GPL(ufshcd_resume);
>
> +int ufshcd_runtime_suspend(struct ufs_hba *hba)
> +{
> +	if (!hba)
> +		return 0;
> +
> +	/*
> +	 * The device is idle with no requests in the queue,
> +	 * allow background operations.
> +	 */
> +	return ufshcd_enable_auto_bkops(hba);
> +}
> +EXPORT_SYMBOL(ufshcd_runtime_suspend);
> +
> +int ufshcd_runtime_resume(struct ufs_hba *hba)
> +{
> +	if (!hba)
> +		return 0;
> +
> +	return ufshcd_disable_auto_bkops(hba);
> +}
> +EXPORT_SYMBOL(ufshcd_runtime_resume);
> +
> +int ufshcd_runtime_idle(struct ufs_hba *hba)
> +{
> +	return 0;
> +}
> +EXPORT_SYMBOL(ufshcd_runtime_idle);
> +
>  /**
>   * ufshcd_remove - de-allocate SCSI host and host memory space
>   *		data structure memory
> @@ -2406,6 +2743,7 @@ int ufshcd_init(struct device *dev, struct ufs_hba
> **hba_handle,
>
>  	/* Initialize work queues */
>  	INIT_WORK(&hba->feh_workq, ufshcd_fatal_err_handler);
> +	INIT_WORK(&hba->eeh_work, ufshcd_exception_event_handler);
>
>  	/* Initialize UIC command mutex */
>  	mutex_init(&hba->uic_cmd_mutex);
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index c6aeb6d..6c9bd35 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -178,9 +178,12 @@ struct ufs_dev_cmd {
>   * @tm_condition: condition variable for task management
>   * @ufshcd_state: UFSHCD states
>   * @intr_mask: Interrupt Mask Bits
> + * @ee_ctrl_mask: Exception event control mask
>   * @feh_workq: Work queue for fatal controller error handling
> + * @eeh_work: Worker to handle exception events
>   * @errors: HBA errors
>   * @dev_cmd: ufs device management command information
> + * @auto_bkops_enabled: to track whether bkops is enabled in device
>   */
>  struct ufs_hba {
>  	void __iomem *mmio_base;
> @@ -218,15 +221,19 @@ struct ufs_hba {
>
>  	u32 ufshcd_state;
>  	u32 intr_mask;
> +	u16 ee_ctrl_mask;
>
>  	/* Work Queues */
>  	struct work_struct feh_workq;
> +	struct work_struct eeh_work;
>
>  	/* HBA Errors */
>  	u32 errors;
>
>  	/* Device management request data */
>  	struct ufs_dev_cmd dev_cmd;
> +
> +	bool auto_bkops_enabled;
>  };
>
>  #define ufshcd_writel(hba, val, reg)	\
> @@ -247,4 +254,7 @@ static inline void ufshcd_hba_stop(struct ufs_hba
> *hba)
>  	ufshcd_writel(hba, CONTROLLER_DISABLE,  REG_CONTROLLER_ENABLE);
>  }
>
> +extern int ufshcd_runtime_suspend(struct ufs_hba *hba);
> +extern int ufshcd_runtime_resume(struct ufs_hba *hba);
> +extern int ufshcd_runtime_idle(struct ufs_hba *hba);
>  #endif /* End of Header */
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
Maya Erez
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH V3 2/2] scsi: ufs: Add runtime PM support for UFS host controller driver

[merez]

Tested-by: Maya Erez <merez@codeaurora.org>

> Add runtime PM helpers to suspend/resume UFS controller at runtime.
> Enable runtime PM by default for pci and platform drivers as the
> initialized hardware can suspend if it is not used after bootup.
>
> Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufshcd-pci.c    |   65
> ++++++++++++++++++++++++++++++++++---
>  drivers/scsi/ufs/ufshcd-pltfrm.c |   51 +++++++++++++++++++++++++++++-
>  drivers/scsi/ufs/ufshcd.c        |    8 +++++
>  3 files changed, 117 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd-pci.c b/drivers/scsi/ufs/ufshcd-pci.c
> index 48be39a..7bd8faa 100644
> --- a/drivers/scsi/ufs/ufshcd-pci.c
> +++ b/drivers/scsi/ufs/ufshcd-pci.c
> @@ -35,6 +35,7 @@
>
>  #include "ufshcd.h"
>  #include <linux/pci.h>
> +#include <linux/pm_runtime.h>
>
>  #ifdef CONFIG_PM
>  /**
> @@ -44,7 +45,7 @@
>   *
>   * Returns -ENOSYS
>   */
> -static int ufshcd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
> +static int ufshcd_pci_suspend(struct device *dev)
>  {
>  	/*
>  	 * TODO:
> @@ -61,7 +62,7 @@ static int ufshcd_pci_suspend(struct pci_dev *pdev,
> pm_message_t state)
>   *
>   * Returns -ENOSYS
>   */
> -static int ufshcd_pci_resume(struct pci_dev *pdev)
> +static int ufshcd_pci_resume(struct device *dev)
>  {
>  	/*
>  	 * TODO:
> @@ -71,8 +72,48 @@ static int ufshcd_pci_resume(struct pci_dev *pdev)
>
>  	return -ENOSYS;
>  }
> +#else
> +#define ufshcd_pci_suspend	NULL
> +#define ufshcd_pci_resume	NULL
>  #endif /* CONFIG_PM */
>
> +#ifdef CONFIG_PM_RUNTIME
> +static int ufshcd_pci_runtime_suspend(struct device *dev)
> +{
> +	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
> +	struct ufs_hba *hba = pci_get_drvdata(pdev);
> +
> +	if (!hba)
> +		return 0;
> +
> +	return ufshcd_runtime_suspend(hba);
> +}
> +static int ufshcd_pci_runtime_resume(struct device *dev)
> +{
> +	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
> +	struct ufs_hba *hba = pci_get_drvdata(pdev);
> +
> +	if (!hba)
> +		return 0;
> +
> +	return ufshcd_runtime_resume(hba);
> +}
> +static int ufshcd_pci_runtime_idle(struct device *dev)
> +{
> +	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
> +	struct ufs_hba *hba = pci_get_drvdata(pdev);
> +
> +	if (!hba)
> +		return 0;
> +
> +	return ufshcd_runtime_idle(hba);
> +}
> +#else /* !CONFIG_PM_RUNTIME */
> +#define ufshcd_pci_runtime_suspend	NULL
> +#define ufshcd_pci_runtime_resume	NULL
> +#define ufshcd_pci_runtime_idle	NULL
> +#endif /* CONFIG_PM_RUNTIME */
> +
>  /**
>   * ufshcd_pci_shutdown - main function to put the controller in reset
> state
>   * @pdev: pointer to PCI device handle
> @@ -91,6 +132,9 @@ static void ufshcd_pci_remove(struct pci_dev *pdev)
>  {
>  	struct ufs_hba *hba = pci_get_drvdata(pdev);
>
> +	pm_runtime_forbid(&pdev->dev);
> +	pm_runtime_get_noresume(&pdev->dev);
> +
>  	disable_irq(pdev->irq);
>  	ufshcd_remove(hba);
>  	pci_release_regions(pdev);
> @@ -168,6 +212,8 @@ ufshcd_pci_probe(struct pci_dev *pdev, const struct
> pci_device_id *id)
>  	}
>
>  	pci_set_drvdata(pdev, hba);
> +	pm_runtime_put_noidle(&pdev->dev);
> +	pm_runtime_allow(&pdev->dev);
>
>  	return 0;
>
> @@ -182,6 +228,14 @@ out_error:
>  	return err;
>  }
>
> +static const struct dev_pm_ops ufshcd_pci_pm_ops = {
> +	.suspend	= ufshcd_pci_suspend,
> +	.resume		= ufshcd_pci_resume,
> +	.runtime_suspend = ufshcd_pci_runtime_suspend,
> +	.runtime_resume  = ufshcd_pci_runtime_resume,
> +	.runtime_idle    = ufshcd_pci_runtime_idle,
> +};
> +
>  static DEFINE_PCI_DEVICE_TABLE(ufshcd_pci_tbl) = {
>  	{ PCI_VENDOR_ID_SAMSUNG, 0xC00C, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
>  	{ }	/* terminate list */
> @@ -195,10 +249,9 @@ static struct pci_driver ufshcd_pci_driver = {
>  	.probe = ufshcd_pci_probe,
>  	.remove = ufshcd_pci_remove,
>  	.shutdown = ufshcd_pci_shutdown,
> -#ifdef CONFIG_PM
> -	.suspend = ufshcd_pci_suspend,
> -	.resume = ufshcd_pci_resume,
> -#endif
> +	.driver = {
> +		.pm = &ufshcd_pci_pm_ops
> +	},
>  };
>
>  module_pci_driver(ufshcd_pci_driver);
> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c
> b/drivers/scsi/ufs/ufshcd-pltfrm.c
> index c42db40..b1f2605 100644
> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c
> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
> @@ -34,6 +34,7 @@
>   */
>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>
>  #include "ufshcd.h"
>
> @@ -87,6 +88,43 @@ static int ufshcd_pltfrm_resume(struct device *dev)
>  #define ufshcd_pltfrm_resume	NULL
>  #endif
>
> +#ifdef CONFIG_PM_RUNTIME
> +static int ufshcd_pltfrm_runtime_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct ufs_hba *hba =  platform_get_drvdata(pdev);
> +
> +	if (!hba)
> +		return 0;
> +
> +	return ufshcd_runtime_suspend(hba);
> +}
> +static int ufshcd_pltfrm_runtime_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct ufs_hba *hba =  platform_get_drvdata(pdev);
> +
> +	if (!hba)
> +		return 0;
> +
> +	return ufshcd_runtime_resume(hba);
> +}
> +static int ufshcd_pltfrm_runtime_idle(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct ufs_hba *hba =  platform_get_drvdata(pdev);
> +
> +	if (!hba)
> +		return 0;
> +
> +	return ufshcd_runtime_idle(hba);
> +}
> +#else /* !CONFIG_PM_RUNTIME */
> +#define ufshcd_pltfrm_runtime_suspend	NULL
> +#define ufshcd_pltfrm_runtime_resume	NULL
> +#define ufshcd_pltfrm_runtime_idle	NULL
> +#endif /* CONFIG_PM_RUNTIME */
> +
>  /**
>   * ufshcd_pltfrm_probe - probe routine of the driver
>   * @pdev: pointer to Platform device handle
> @@ -122,14 +160,20 @@ static int ufshcd_pltfrm_probe(struct
> platform_device *pdev)
>  		goto out;
>  	}
>
> +	pm_runtime_set_active(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
> +
>  	err = ufshcd_init(dev, &hba, mmio_base, irq);
>  	if (err) {
>  		dev_err(dev, "Intialization failed\n");
> -		goto out;
> +		goto out_disable_rpm;
>  	}
>
>  	platform_set_drvdata(pdev, hba);
>
> +out_disable_rpm:
> +	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_set_suspended(&pdev->dev);
>  out:
>  	return err;
>  }
> @@ -144,6 +188,8 @@ static int ufshcd_pltfrm_remove(struct platform_device
> *pdev)
>  {
>  	struct ufs_hba *hba =  platform_get_drvdata(pdev);
>
> +	pm_runtime_get_sync(&(pdev)->dev);
> +
>  	disable_irq(hba->irq);
>  	ufshcd_remove(hba);
>  	return 0;
> @@ -157,6 +203,9 @@ static const struct of_device_id ufs_of_match[] = {
>  static const struct dev_pm_ops ufshcd_dev_pm_ops = {
>  	.suspend	= ufshcd_pltfrm_suspend,
>  	.resume		= ufshcd_pltfrm_resume,
> +	.runtime_suspend = ufshcd_pltfrm_runtime_suspend,
> +	.runtime_resume  = ufshcd_pltfrm_runtime_resume,
> +	.runtime_idle    = ufshcd_pltfrm_runtime_idle,
>  };
>
>  static struct platform_driver ufshcd_pltfrm_driver = {
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index a25de66..af7d01d 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2230,6 +2230,7 @@ static void ufshcd_exception_event_handler(struct
> work_struct *work)
>  	u32 status = 0;
>  	hba = container_of(work, struct ufs_hba, eeh_work);
>
> +	pm_runtime_get_sync(hba->dev);
>  	err = ufshcd_get_ee_status(hba, &status);
>  	if (err) {
>  		dev_err(hba->dev, "%s: failed to get exception status %d\n",
> @@ -2245,6 +2246,7 @@ static void ufshcd_exception_event_handler(struct
> work_struct *work)
>  					__func__, err);
>  	}
>  out:
> +	pm_runtime_put_sync(hba->dev);
>  	return;
>  }
>
> @@ -2257,9 +2259,11 @@ static void ufshcd_fatal_err_handler(struct
> work_struct *work)
>  	struct ufs_hba *hba;
>  	hba = container_of(work, struct ufs_hba, feh_workq);
>
> +	pm_runtime_get_sync(hba->dev);
>  	/* check if reset is already in progress */
>  	if (hba->ufshcd_state != UFSHCD_STATE_RESET)
>  		ufshcd_do_reset(hba);
> +	pm_runtime_put_sync(hba->dev);
>  }
>
>  /**
> @@ -2562,6 +2566,7 @@ static void ufshcd_async_scan(void *data,
> async_cookie_t cookie)
>  	hba->auto_bkops_enabled = false;
>  	ufshcd_enable_auto_bkops(hba);
>  	scsi_scan_host(hba->host);
> +	pm_runtime_put_sync(hba->dev);
>  out:
>  	return;
>  }
> @@ -2783,6 +2788,9 @@ int ufshcd_init(struct device *dev, struct ufs_hba
> **hba_handle,
>
>  	*hba_handle = hba;
>
> +	/* Hold auto suspend until async scan completes */
> +	pm_runtime_get_sync(dev);
> +
>  	async_schedule(ufshcd_async_scan, hba);
>
>  	return 0;
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
Maya Erez
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

RE: [PATCH V3 1/2] scsi: ufs: Add support for host assisted background operations

[Seungwon Jeon]

I'm not sure that BKOPS with runtime-pm associates.
Do you think it's helpful for power management?
How about hibernation scheme for runtime-pm?
I'm testing and I can introduce soon.

On Tue, July 09, 2013, Sujit Reddy Thumma wrote:
> Background operations in the UFS device can be disabled by
> the host to reduce the response latency of transfer requests.
> Add support for enabling/disabling the background operations
> during runtime suspend/resume of the device.
> 
> If the device is in critical need of BKOPS it will raise an
> URGENT_BKOPS exception which should be handled by the host to
> make sure the device performs as expected.
> 
> During bootup, the BKOPS is enabled in the device by default.
> The disable of BKOPS is supported only when the driver supports
> runtime suspend/resume operations as the runtime PM framework
> provides a way to determine the device idleness and hence BKOPS
> can be managed effectively. During runtime resume the BKOPS is
> disabled to reduce latency and during runtime suspend the BKOPS
> is enabled to allow device to carry out idle time BKOPS.
> 
> In some cases where the BKOPS is disabled during runtime resume
> and due to continuous data transfers the runtime suspend is not
> triggered, the BKOPS is enabled when the device raises a level-2
> exception (outstanding operations - performance impact).
> 
> Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufs.h    |   25 ++++-
>  drivers/scsi/ufs/ufshcd.c |  338 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/scsi/ufs/ufshcd.h |   10 ++
>  3 files changed, 372 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index db5bde4..549a652 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -107,7 +107,29 @@ enum {
> 
>  /* Flag idn for Query Requests*/
>  enum flag_idn {
> -	QUERY_FLAG_IDN_FDEVICEINIT = 0x01,
> +	QUERY_FLAG_IDN_FDEVICEINIT	= 0x01,
> +	QUERY_FLAG_IDN_BKOPS_EN		= 0x04,
> +};
> +
> +/* Attribute idn for Query requests */
> +enum attr_idn {
> +	QUERY_ATTR_IDN_BKOPS_STATUS	= 0x05,
> +	QUERY_ATTR_IDN_EE_CONTROL	= 0x0D,
> +	QUERY_ATTR_IDN_EE_STATUS	= 0x0E,
> +};
> +
> +/* Exception event mask values */
> +enum {
> +	MASK_EE_STATUS		= 0xFFFF,
> +	MASK_EE_URGENT_BKOPS	= (1 << 2),
> +};
> +
> +/* Background operation status */
> +enum {
> +	BKOPS_STATUS_NO_OP               = 0x0,
> +	BKOPS_STATUS_NON_CRITICAL        = 0x1,
> +	BKOPS_STATUS_PERF_IMPACT         = 0x2,
> +	BKOPS_STATUS_CRITICAL            = 0x3,
>  };
> 
>  /* UTP QUERY Transaction Specific Fields OpCode */
> @@ -156,6 +178,7 @@ enum {
>  	MASK_TASK_RESPONSE	= 0xFF00,
>  	MASK_RSP_UPIU_RESULT	= 0xFFFF,
>  	MASK_QUERY_DATA_SEG_LEN	= 0xFFFF,
> +	MASK_RSP_EXCEPTION_EVENT = 0x10000,
>  };
> 
>  /* Task management service response */
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 96ccb28..a25de66 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -268,6 +268,21 @@ ufshcd_get_rsp_upiu_result(struct utp_upiu_rsp *ucd_rsp_ptr)
>  }
> 
>  /**
> + * ufshcd_is_exception_event - Check if the device raised an exception event
> + * @ucd_rsp_ptr: pointer to response UPIU
> + *
> + * The function checks if the device raised an exception event indicated in
> + * the Device Information field of response UPIU.
> + *
> + * Returns true if exception is raised, false otherwise.
> + */
> +static inline bool ufshcd_is_exception_event(struct utp_upiu_rsp *ucd_rsp_ptr)
> +{
> +	return be32_to_cpu(ucd_rsp_ptr->header.dword_2) &
> +			MASK_RSP_EXCEPTION_EVENT ? true : false;
> +}
> +
> +/**
>   * ufshcd_config_int_aggr - Configure interrupt aggregation values.
>   *		Currently there is no use case where we want to configure
>   *		interrupt aggregation dynamically. So to configure interrupt
> @@ -1174,6 +1189,86 @@ out_no_mem:
>  }
> 
>  /**
> + * ufshcd_query_attr - Helper function for composing attribute requests
> + * hba: per-adapter instance
> + * opcode: attribute opcode
> + * idn: attribute idn to access
> + * index: index field
> + * selector: selector field
> + * attr_val: the attribute value after the query request completes
> + *
> + * Returns 0 for success, non-zero in case of failure
> +*/
> +int ufshcd_query_attr(struct ufs_hba *hba, enum query_opcode opcode,
> +			enum attr_idn idn, u8 index, u8 selector, u32 *attr_val)
> +{
> +	struct ufs_query_req *query;
> +	struct ufs_query_res *response;
> +	int err = -ENOMEM;
> +
> +	if (!attr_val) {
> +		dev_err(hba->dev, "%s: attribute value required for write request\n",
It's trivial, but message is only focused on write.
attr_val is also needed in case read request.

Thanks,
Seungwon Jeon

> +				__func__);
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
> +	query = kzalloc(sizeof(struct ufs_query_req), GFP_KERNEL);
> +	if (!query) {
> +		dev_err(hba->dev,
> +			"%s: Failed allocating ufs_query_req instance\n",
> +			__func__);
> +		goto out;
> +	}
> +
> +	response = kzalloc(sizeof(struct ufs_query_res), GFP_KERNEL);
> +	if (!response) {
> +		dev_err(hba->dev,
> +			"%s: Failed allocating ufs_query_res instance\n",
> +			__func__);
> +		goto out_free_query;
> +	}
> +
> +	switch (opcode) {
> +	case UPIU_QUERY_OPCODE_WRITE_ATTR:
> +		query->query_func = UPIU_QUERY_FUNC_STANDARD_WRITE_REQUEST;
> +		query->upiu_req.value = *attr_val;
> +		break;
> +	case UPIU_QUERY_OPCODE_READ_ATTR:
> +		query->query_func = UPIU_QUERY_FUNC_STANDARD_READ_REQUEST;
> +		break;
> +	default:
> +		dev_err(hba->dev, "%s: Expected query attr opcode but got = 0x%.2x\n",
> +				__func__, opcode);
> +		err = -EINVAL;
> +		goto out_free;
> +	}
> +
> +	query->upiu_req.opcode = opcode;
> +	query->upiu_req.idn = idn;
> +	query->upiu_req.index = index;
> +	query->upiu_req.selector = selector;
> +
> +	/* Send query request */
> +	err = ufshcd_send_query_request(hba, query, NULL, response);
> +
> +	if (err) {
> +		dev_err(hba->dev, "%s: opcode 0x%.2x for idn %d failed, err = %d\n",
> +				__func__, opcode, idn, err);
> +		goto out_free;
> +	}
> +
> +	*attr_val = response->upiu_res.value;
> +
> +out_free:
> +	kfree(response);
> +out_free_query:
> +	kfree(query);
> +out:
> +	return err;
> +}
> +
> +/**
>   * ufshcd_memory_alloc - allocate memory for host memory space data structures
>   * @hba: per adapter instance
>   *
> @@ -1842,6 +1937,9 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
>  			 */
>  			scsi_status = result & MASK_SCSI_STATUS;
>  			result = ufshcd_scsi_cmd_status(lrbp, scsi_status);
> +
> +			if (ufshcd_is_exception_event(lrbp->ucd_rsp_ptr))
> +				schedule_work(&hba->eeh_work);
>  			break;
>  		case UPIU_TRANSACTION_REJECT_UPIU:
>  			/* TODO: handle Reject UPIU Response */
> @@ -1942,6 +2040,215 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
>  }
> 
>  /**
> + * ufshcd_disable_ee - disable exception event
> + * @hba: per-adapter instance
> + * @mask: exception event to disable
> + *
> + * Disables exception event in the device so that the EVENT_ALERT
> + * bit is not set.
> + *
> + * Returns zero on success, non-zero error value on failure.
> + */
> +static int ufshcd_disable_ee(struct ufs_hba *hba, u16 mask)
> +{
> +	int err = 0;
> +	u32 val;
> +
> +	if (!(hba->ee_ctrl_mask & mask))
> +		goto out;
> +
> +	val = hba->ee_ctrl_mask & ~mask;
> +	val &= 0xFFFF; /* 2 bytes */
> +	err = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> +			QUERY_ATTR_IDN_EE_CONTROL, 0, 0, &val);
> +	if (!err)
> +		hba->ee_ctrl_mask &= ~mask;
> +out:
> +	return err;
> +}
> +
> +/**
> + * ufshcd_enable_ee - enable exception event
> + * @hba: per-adapter instance
> + * @mask: exception event to enable
> + *
> + * Enable corresponding exception event in the device to allow
> + * device to alert host in critical scenarios.
> + *
> + * Returns zero on success, non-zero error value on failure.
> + */
> +static int ufshcd_enable_ee(struct ufs_hba *hba, u16 mask)
> +{
> +	int err = 0;
> +	u32 val;
> +
> +	if (hba->ee_ctrl_mask & mask)
> +		goto out;
> +
> +	val = hba->ee_ctrl_mask | mask;
> +	val &= 0xFFFF; /* 2 bytes */
> +	err = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
> +			QUERY_ATTR_IDN_EE_CONTROL, 0, 0, &val);
> +	if (!err)
> +		hba->ee_ctrl_mask |= mask;
> +out:
> +	return err;
> +}
> +
> +/**
> + * ufshcd_enable_auto_bkops - Allow device managed BKOPS
> + * @hba: per-adapter instance
> + *
> + * Allow device to manage background operations on its own. Enabling
> + * this might lead to inconsistent latencies during normal data transfers
> + * as the device is allowed to manage its own way of handling background
> + * operations.
> + *
> + * Returns zero on success, non-zero on failure.
> + */
> +static int ufshcd_enable_auto_bkops(struct ufs_hba *hba)
> +{
> +	int err = 0;
> +
> +	if (hba->auto_bkops_enabled)
> +		goto out;
> +
> +	err = ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_SET_FLAG,
> +			QUERY_FLAG_IDN_BKOPS_EN, NULL);
> +	if (err) {
> +		dev_err(hba->dev, "%s: failed to enable bkops %d\n",
> +				__func__, err);
> +		goto out;
> +	}
> +
> +	hba->auto_bkops_enabled = true;
> +
> +	/* No need of URGENT_BKOPS exception from the device */
> +	err = ufshcd_disable_ee(hba, MASK_EE_URGENT_BKOPS);
> +	if (err)
> +		dev_err(hba->dev, "%s: failed to disable exception event %d\n",
> +				__func__, err);
> +out:
> +	return err;
> +}
> +
> +/**
> + * ufshcd_disable_auto_bkops - block device in doing background operations
> + * @hba: per-adapter instance
> + *
> + * Disabling background operations improves command response latency but
> + * has drawback of device moving into critical state where the device is
> + * not-operable. Make sure to call ufshcd_enable_auto_bkops() whenever the
> + * host is idle so that BKOPS are managed effectively without any negative
> + * impacts.
> + *
> + * Returns zero on success, non-zero on failure.
> + */
> +static int ufshcd_disable_auto_bkops(struct ufs_hba *hba)
> +{
> +	int err = 0;
> +
> +	if (!hba->auto_bkops_enabled)
> +		goto out;
> +
> +	/*
> +	 * If host assisted BKOPs is to be enabled, make sure
> +	 * urgent bkops exception is allowed.
> +	 */
> +	err = ufshcd_enable_ee(hba, MASK_EE_URGENT_BKOPS);
> +	if (err) {
> +		dev_err(hba->dev, "%s: failed to enable exception event %d\n",
> +				__func__, err);
> +		goto out;
> +	}
> +
> +	err = ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_CLEAR_FLAG,
> +			QUERY_FLAG_IDN_BKOPS_EN, NULL);
> +	if (err) {
> +		dev_err(hba->dev, "%s: failed to disable bkops %d\n",
> +				__func__, err);
> +		ufshcd_disable_ee(hba, MASK_EE_URGENT_BKOPS);
> +		goto out;
> +	}
> +
> +	hba->auto_bkops_enabled = false;
> +out:
> +	return err;
> +}
> +
> +static inline int ufshcd_get_bkops_status(struct ufs_hba *hba, u32 *status)
> +{
> +	return ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> +			QUERY_ATTR_IDN_BKOPS_STATUS, 0, 0, status);
> +}
> +
> +/**
> + * ufshcd_urgent_bkops - handle urgent bkops exception event
> + * @hba: per-adapter instance
> + *
> + * Enable fBackgroundOpsEn flag in the device to permit background
> + * operations.
> + */
> +static int ufshcd_urgent_bkops(struct ufs_hba *hba)
> +{
> +	int err;
> +	u32 status = 0;
> +
> +	err = ufshcd_get_bkops_status(hba, &status);
> +	if (err) {
> +		dev_err(hba->dev, "%s: failed to get BKOPS status %d\n",
> +				__func__, err);
> +		goto out;
> +	}
> +
> +	status = status & 0xF;
> +
> +	/* handle only if status indicates performance impact or critical */
> +	if (status >= BKOPS_STATUS_PERF_IMPACT)
> +		err = ufshcd_enable_auto_bkops(hba);
> +out:
> +	return err;
> +}
> +
> +static inline int ufshcd_get_ee_status(struct ufs_hba *hba, u32 *status)
> +{
> +	return ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> +			QUERY_ATTR_IDN_EE_STATUS, 0, 0, status);
> +}
> +
> +/**
> + * ufshcd_exception_event_handler - handle exceptions raised by device
> + * @work: pointer to work data
> + *
> + * Read bExceptionEventStatus attribute from the device and handle the
> + * exception event accordingly.
> + */
> +static void ufshcd_exception_event_handler(struct work_struct *work)
> +{
> +	struct ufs_hba *hba;
> +	int err;
> +	u32 status = 0;
> +	hba = container_of(work, struct ufs_hba, eeh_work);
> +
> +	err = ufshcd_get_ee_status(hba, &status);
> +	if (err) {
> +		dev_err(hba->dev, "%s: failed to get exception status %d\n",
> +				__func__, err);
> +		goto out;
> +	}
> +
> +	status &= hba->ee_ctrl_mask;
> +	if (status & MASK_EE_URGENT_BKOPS) {
> +		err = ufshcd_urgent_bkops(hba);
> +		if (err)
> +			dev_err(hba->dev, "%s: failed to handle urgent bkops %d\n",
> +					__func__, err);
> +	}
> +out:
> +	return;
> +}
> +
> +/**
>   * ufshcd_fatal_err_handler - handle fatal errors
>   * @hba: per adapter instance
>   */
> @@ -2252,6 +2559,8 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
>  	if (ret)
>  		goto out;
> 
> +	hba->auto_bkops_enabled = false;
> +	ufshcd_enable_auto_bkops(hba);
>  	scsi_scan_host(hba->host);
>  out:
>  	return;
> @@ -2316,6 +2625,34 @@ int ufshcd_resume(struct ufs_hba *hba)
>  }
>  EXPORT_SYMBOL_GPL(ufshcd_resume);
> 
> +int ufshcd_runtime_suspend(struct ufs_hba *hba)
> +{
> +	if (!hba)
> +		return 0;
> +
> +	/*
> +	 * The device is idle with no requests in the queue,
> +	 * allow background operations.
> +	 */
> +	return ufshcd_enable_auto_bkops(hba);
> +}
> +EXPORT_SYMBOL(ufshcd_runtime_suspend);
> +
> +int ufshcd_runtime_resume(struct ufs_hba *hba)
> +{
> +	if (!hba)
> +		return 0;
> +
> +	return ufshcd_disable_auto_bkops(hba);
> +}
> +EXPORT_SYMBOL(ufshcd_runtime_resume);
> +
> +int ufshcd_runtime_idle(struct ufs_hba *hba)
> +{
> +	return 0;
> +}
> +EXPORT_SYMBOL(ufshcd_runtime_idle);
> +
>  /**
>   * ufshcd_remove - de-allocate SCSI host and host memory space
>   *		data structure memory
> @@ -2406,6 +2743,7 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
> 
>  	/* Initialize work queues */
>  	INIT_WORK(&hba->feh_workq, ufshcd_fatal_err_handler);
> +	INIT_WORK(&hba->eeh_work, ufshcd_exception_event_handler);
> 
>  	/* Initialize UIC command mutex */
>  	mutex_init(&hba->uic_cmd_mutex);
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index c6aeb6d..6c9bd35 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -178,9 +178,12 @@ struct ufs_dev_cmd {
>   * @tm_condition: condition variable for task management
>   * @ufshcd_state: UFSHCD states
>   * @intr_mask: Interrupt Mask Bits
> + * @ee_ctrl_mask: Exception event control mask
>   * @feh_workq: Work queue for fatal controller error handling
> + * @eeh_work: Worker to handle exception events
>   * @errors: HBA errors
>   * @dev_cmd: ufs device management command information
> + * @auto_bkops_enabled: to track whether bkops is enabled in device
>   */
>  struct ufs_hba {
>  	void __iomem *mmio_base;
> @@ -218,15 +221,19 @@ struct ufs_hba {
> 
>  	u32 ufshcd_state;
>  	u32 intr_mask;
> +	u16 ee_ctrl_mask;
> 
>  	/* Work Queues */
>  	struct work_struct feh_workq;
> +	struct work_struct eeh_work;
> 
>  	/* HBA Errors */
>  	u32 errors;
> 
>  	/* Device management request data */
>  	struct ufs_dev_cmd dev_cmd;
> +
> +	bool auto_bkops_enabled;
>  };
> 
>  #define ufshcd_writel(hba, val, reg)	\
> @@ -247,4 +254,7 @@ static inline void ufshcd_hba_stop(struct ufs_hba *hba)
>  	ufshcd_writel(hba, CONTROLLER_DISABLE,  REG_CONTROLLER_ENABLE);
>  }
> 
> +extern int ufshcd_runtime_suspend(struct ufs_hba *hba);
> +extern int ufshcd_runtime_resume(struct ufs_hba *hba);
> +extern int ufshcd_runtime_idle(struct ufs_hba *hba);
>  #endif /* End of Header */
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH V3 2/2] scsi: ufs: Add runtime PM support for UFS host controller driver

[Seungwon Jeon]

On Tue, July 09, 2013, Sujit Reddy Thumma wrote:
> Add runtime PM helpers to suspend/resume UFS controller at runtime.
> Enable runtime PM by default for pci and platform drivers as the
> initialized hardware can suspend if it is not used after bootup.
> 
> Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufshcd-pci.c    |   65 ++++++++++++++++++++++++++++++++++---
>  drivers/scsi/ufs/ufshcd-pltfrm.c |   51 +++++++++++++++++++++++++++++-
>  drivers/scsi/ufs/ufshcd.c        |    8 +++++
>  3 files changed, 117 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd-pci.c b/drivers/scsi/ufs/ufshcd-pci.c
> index 48be39a..7bd8faa 100644
> --- a/drivers/scsi/ufs/ufshcd-pci.c
> +++ b/drivers/scsi/ufs/ufshcd-pci.c
> @@ -35,6 +35,7 @@
> 
>  #include "ufshcd.h"
>  #include <linux/pci.h>
> +#include <linux/pm_runtime.h>
> 
>  #ifdef CONFIG_PM
>  /**
> @@ -44,7 +45,7 @@
>   *
>   * Returns -ENOSYS
>   */
> -static int ufshcd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
> +static int ufshcd_pci_suspend(struct device *dev)
>  {
>  	/*
>  	 * TODO:
> @@ -61,7 +62,7 @@ static int ufshcd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
>   *
>   * Returns -ENOSYS
>   */
> -static int ufshcd_pci_resume(struct pci_dev *pdev)
> +static int ufshcd_pci_resume(struct device *dev)
>  {
>  	/*
>  	 * TODO:
> @@ -71,8 +72,48 @@ static int ufshcd_pci_resume(struct pci_dev *pdev)
> 
>  	return -ENOSYS;
>  }
> +#else
> +#define ufshcd_pci_suspend	NULL
> +#define ufshcd_pci_resume	NULL
>  #endif /* CONFIG_PM */
> 
> +#ifdef CONFIG_PM_RUNTIME
> +static int ufshcd_pci_runtime_suspend(struct device *dev)
> +{
> +	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
> +	struct ufs_hba *hba = pci_get_drvdata(pdev);
Can be replaced:
	struct ufs_hba *hba = dev_get_drvdata(dev);

> +
> +	if (!hba)
> +		return 0;
> +
> +	return ufshcd_runtime_suspend(hba);
> +}
> +static int ufshcd_pci_runtime_resume(struct device *dev)
> +{
> +	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
> +	struct ufs_hba *hba = pci_get_drvdata(pdev);
Same as above.


> +
> +	if (!hba)
> +		return 0;
> +
> +	return ufshcd_runtime_resume(hba);
> +}
> +static int ufshcd_pci_runtime_idle(struct device *dev)
> +{
> +	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
> +	struct ufs_hba *hba = pci_get_drvdata(pdev);
Same as above.

> +
> +	if (!hba)
> +		return 0;
> +
> +	return ufshcd_runtime_idle(hba);
> +}
> +#else /* !CONFIG_PM_RUNTIME */
> +#define ufshcd_pci_runtime_suspend	NULL
> +#define ufshcd_pci_runtime_resume	NULL
> +#define ufshcd_pci_runtime_idle	NULL
> +#endif /* CONFIG_PM_RUNTIME */
> +
>  /**
>   * ufshcd_pci_shutdown - main function to put the controller in reset state
>   * @pdev: pointer to PCI device handle
> @@ -91,6 +132,9 @@ static void ufshcd_pci_remove(struct pci_dev *pdev)
>  {
>  	struct ufs_hba *hba = pci_get_drvdata(pdev);
> 
> +	pm_runtime_forbid(&pdev->dev);
> +	pm_runtime_get_noresume(&pdev->dev);
> +
>  	disable_irq(pdev->irq);
>  	ufshcd_remove(hba);
>  	pci_release_regions(pdev);
> @@ -168,6 +212,8 @@ ufshcd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	}
> 
>  	pci_set_drvdata(pdev, hba);
> +	pm_runtime_put_noidle(&pdev->dev);
> +	pm_runtime_allow(&pdev->dev);
> 
>  	return 0;
> 
> @@ -182,6 +228,14 @@ out_error:
>  	return err;
>  }
> 
> +static const struct dev_pm_ops ufshcd_pci_pm_ops = {
> +	.suspend	= ufshcd_pci_suspend,
> +	.resume		= ufshcd_pci_resume,
> +	.runtime_suspend = ufshcd_pci_runtime_suspend,
> +	.runtime_resume  = ufshcd_pci_runtime_resume,
> +	.runtime_idle    = ufshcd_pci_runtime_idle,
> +};
> +
>  static DEFINE_PCI_DEVICE_TABLE(ufshcd_pci_tbl) = {
>  	{ PCI_VENDOR_ID_SAMSUNG, 0xC00C, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
>  	{ }	/* terminate list */
> @@ -195,10 +249,9 @@ static struct pci_driver ufshcd_pci_driver = {
>  	.probe = ufshcd_pci_probe,
>  	.remove = ufshcd_pci_remove,
>  	.shutdown = ufshcd_pci_shutdown,
> -#ifdef CONFIG_PM
> -	.suspend = ufshcd_pci_suspend,
> -	.resume = ufshcd_pci_resume,
> -#endif
> +	.driver = {
> +		.pm = &ufshcd_pci_pm_ops
> +	},
>  };
> 
>  module_pci_driver(ufshcd_pci_driver);
> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
> index c42db40..b1f2605 100644
> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c
> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
> @@ -34,6 +34,7 @@
>   */
> 
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> 
>  #include "ufshcd.h"
> 
> @@ -87,6 +88,43 @@ static int ufshcd_pltfrm_resume(struct device *dev)
>  #define ufshcd_pltfrm_resume	NULL
>  #endif
> 
> +#ifdef CONFIG_PM_RUNTIME
> +static int ufshcd_pltfrm_runtime_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct ufs_hba *hba =  platform_get_drvdata(pdev);
Same as above.

> +
> +	if (!hba)
> +		return 0;
> +
> +	return ufshcd_runtime_suspend(hba);
> +}
> +static int ufshcd_pltfrm_runtime_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct ufs_hba *hba =  platform_get_drvdata(pdev);
Same as above.

> +
> +	if (!hba)
> +		return 0;
> +
> +	return ufshcd_runtime_resume(hba);
> +}
> +static int ufshcd_pltfrm_runtime_idle(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct ufs_hba *hba =  platform_get_drvdata(pdev);
Same as above.

> +
> +	if (!hba)
> +		return 0;
> +
> +	return ufshcd_runtime_idle(hba);
> +}
> +#else /* !CONFIG_PM_RUNTIME */
> +#define ufshcd_pltfrm_runtime_suspend	NULL
> +#define ufshcd_pltfrm_runtime_resume	NULL
> +#define ufshcd_pltfrm_runtime_idle	NULL
> +#endif /* CONFIG_PM_RUNTIME */
> +
>  /**
>   * ufshcd_pltfrm_probe - probe routine of the driver
>   * @pdev: pointer to Platform device handle
> @@ -122,14 +160,20 @@ static int ufshcd_pltfrm_probe(struct platform_device *pdev)
>  		goto out;
>  	}
> 
> +	pm_runtime_set_active(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
> +
>  	err = ufshcd_init(dev, &hba, mmio_base, irq);
>  	if (err) {
>  		dev_err(dev, "Intialization failed\n");
> -		goto out;
> +		goto out_disable_rpm;
>  	}
> 
>  	platform_set_drvdata(pdev, hba);
> 
> +out_disable_rpm:
> +	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_set_suspended(&pdev->dev);
>  out:
>  	return err;
>  }
> @@ -144,6 +188,8 @@ static int ufshcd_pltfrm_remove(struct platform_device *pdev)
>  {
>  	struct ufs_hba *hba =  platform_get_drvdata(pdev);
> 
> +	pm_runtime_get_sync(&(pdev)->dev);
> +
>  	disable_irq(hba->irq);
>  	ufshcd_remove(hba);
>  	return 0;
> @@ -157,6 +203,9 @@ static const struct of_device_id ufs_of_match[] = {
>  static const struct dev_pm_ops ufshcd_dev_pm_ops = {
>  	.suspend	= ufshcd_pltfrm_suspend,
>  	.resume		= ufshcd_pltfrm_resume,
> +	.runtime_suspend = ufshcd_pltfrm_runtime_suspend,
> +	.runtime_resume  = ufshcd_pltfrm_runtime_resume,
> +	.runtime_idle    = ufshcd_pltfrm_runtime_idle,
>  };
> 
>  static struct platform_driver ufshcd_pltfrm_driver = {
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index a25de66..af7d01d 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2230,6 +2230,7 @@ static void ufshcd_exception_event_handler(struct work_struct *work)
>  	u32 status = 0;
>  	hba = container_of(work, struct ufs_hba, eeh_work);
> 
> +	pm_runtime_get_sync(hba->dev);
>  	err = ufshcd_get_ee_status(hba, &status);
>  	if (err) {
>  		dev_err(hba->dev, "%s: failed to get exception status %d\n",
> @@ -2245,6 +2246,7 @@ static void ufshcd_exception_event_handler(struct work_struct *work)
>  					__func__, err);
>  	}
>  out:
> +	pm_runtime_put_sync(hba->dev);
In ufshcd_urgent_bkops, ufshcd_enable_auto_bkops will be executed.

>  	return;
>  }
> 
> @@ -2257,9 +2259,11 @@ static void ufshcd_fatal_err_handler(struct work_struct *work)
>  	struct ufs_hba *hba;
>  	hba = container_of(work, struct ufs_hba, feh_workq);
> 
> +	pm_runtime_get_sync(hba->dev);
>  	/* check if reset is already in progress */
>  	if (hba->ufshcd_state != UFSHCD_STATE_RESET)
>  		ufshcd_do_reset(hba);
> +	pm_runtime_put_sync(hba->dev);
>  }
> 
>  /**
> @@ -2562,6 +2566,7 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
>  	hba->auto_bkops_enabled = false;
In the end of 'ufshcd_init', pm_runtime_get_sync is called, hence auto_bkops_enabled may has false.
'auto_bkops_enabled = false' is needed?

>  	ufshcd_enable_auto_bkops(hba);
Should 'ufshcd_enable_auto_bkops' be done here?

>  	scsi_scan_host(hba->host);
> +	pm_runtime_put_sync(hba->dev);
Practically, after pm_runtime_put_sync, ufshcd_enable_auto_bkops will be executed.
Then, it's duplication of ufshcd_enable_auto_bkops?
ufshcd_enable_auto_bkops is already done above.

And actual routine of scsi_scan_host performs asynchronously.
That means it's not completed. So, pm_runtime_put_sync looks like improper here.

>  out:
>  	return;
>  }
> @@ -2783,6 +2788,9 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
> 
>  	*hba_handle = hba;
> 
> +	/* Hold auto suspend until async scan completes */
What's meaning of comment?

Thanks,
Seungwon Jeon

> +	pm_runtime_get_sync(dev);
> +
>  	async_schedule(ufshcd_async_scan, hba);
> 
>  	return 0;
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 1/2] scsi: ufs: Add support for host assisted background operations

[Sujit Reddy Thumma]

On 7/10/2013 7:01 PM, Seungwon Jeon wrote:
> I'm not sure that BKOPS with runtime-pm associates.
> Do you think it's helpful for power management?
> How about hibernation scheme for runtime-pm?
> I'm testing and I can introduce soon.

Well, I am thinking on following approach when we introduce
power management.

ufshcd_runtime_suspend() {
	if (bkops_status >= NON_CRITICAL) { /* 0x1 */
		ufshcd_enable_auto_bkops();
		hibernate(); /* only the link and the device
			should be able to cary out bkops */
	} else {
		hibernate(); /* Link and the device for more savings */
	}
}

Let me know if this is okay.

>
> On Tue, July 09, 2013, Sujit Reddy Thumma wrote:
>> Background operations in the UFS device can be disabled by
>> the host to reduce the response latency of transfer requests.
>> Add support for enabling/disabling the background operations
>> during runtime suspend/resume of the device.
>>
>> If the device is in critical need of BKOPS it will raise an
>> URGENT_BKOPS exception which should be handled by the host to
>> make sure the device performs as expected.
>>
>> During bootup, the BKOPS is enabled in the device by default.
>> The disable of BKOPS is supported only when the driver supports
>> runtime suspend/resume operations as the runtime PM framework
>> provides a way to determine the device idleness and hence BKOPS
>> can be managed effectively. During runtime resume the BKOPS is
>> disabled to reduce latency and during runtime suspend the BKOPS
>> is enabled to allow device to carry out idle time BKOPS.
>>
>> In some cases where the BKOPS is disabled during runtime resume
>> and due to continuous data transfers the runtime suspend is not
>> triggered, the BKOPS is enabled when the device raises a level-2
>> exception (outstanding operations - performance impact).
>>
>> Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
>> ---
>>   drivers/scsi/ufs/ufs.h    |   25 ++++-
>>   drivers/scsi/ufs/ufshcd.c |  338 +++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/scsi/ufs/ufshcd.h |   10 ++
>>   3 files changed, 372 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
>> index db5bde4..549a652 100644
>> --- a/drivers/scsi/ufs/ufs.h
>> +++ b/drivers/scsi/ufs/ufs.h
>> @@ -107,7 +107,29 @@ enum {
>>
>>   /* Flag idn for Query Requests*/
>>   enum flag_idn {
>> -	QUERY_FLAG_IDN_FDEVICEINIT = 0x01,
>> +	QUERY_FLAG_IDN_FDEVICEINIT	= 0x01,
>> +	QUERY_FLAG_IDN_BKOPS_EN		= 0x04,
>> +};
>> +
>> +/* Attribute idn for Query requests */
>> +enum attr_idn {
>> +	QUERY_ATTR_IDN_BKOPS_STATUS	= 0x05,
>> +	QUERY_ATTR_IDN_EE_CONTROL	= 0x0D,
>> +	QUERY_ATTR_IDN_EE_STATUS	= 0x0E,
>> +};
>> +
>> +/* Exception event mask values */
>> +enum {
>> +	MASK_EE_STATUS		= 0xFFFF,
>> +	MASK_EE_URGENT_BKOPS	= (1 << 2),
>> +};
>> +
>> +/* Background operation status */
>> +enum {
>> +	BKOPS_STATUS_NO_OP               = 0x0,
>> +	BKOPS_STATUS_NON_CRITICAL        = 0x1,
>> +	BKOPS_STATUS_PERF_IMPACT         = 0x2,
>> +	BKOPS_STATUS_CRITICAL            = 0x3,
>>   };
>>
>>   /* UTP QUERY Transaction Specific Fields OpCode */
>> @@ -156,6 +178,7 @@ enum {
>>   	MASK_TASK_RESPONSE	= 0xFF00,
>>   	MASK_RSP_UPIU_RESULT	= 0xFFFF,
>>   	MASK_QUERY_DATA_SEG_LEN	= 0xFFFF,
>> +	MASK_RSP_EXCEPTION_EVENT = 0x10000,
>>   };
>>
>>   /* Task management service response */
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 96ccb28..a25de66 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -268,6 +268,21 @@ ufshcd_get_rsp_upiu_result(struct utp_upiu_rsp *ucd_rsp_ptr)
>>   }
>>
>>   /**
>> + * ufshcd_is_exception_event - Check if the device raised an exception event
>> + * @ucd_rsp_ptr: pointer to response UPIU
>> + *
>> + * The function checks if the device raised an exception event indicated in
>> + * the Device Information field of response UPIU.
>> + *
>> + * Returns true if exception is raised, false otherwise.
>> + */
>> +static inline bool ufshcd_is_exception_event(struct utp_upiu_rsp *ucd_rsp_ptr)
>> +{
>> +	return be32_to_cpu(ucd_rsp_ptr->header.dword_2) &
>> +			MASK_RSP_EXCEPTION_EVENT ? true : false;
>> +}
>> +
>> +/**
>>    * ufshcd_config_int_aggr - Configure interrupt aggregation values.
>>    *		Currently there is no use case where we want to configure
>>    *		interrupt aggregation dynamically. So to configure interrupt
>> @@ -1174,6 +1189,86 @@ out_no_mem:
>>   }
>>
>>   /**
>> + * ufshcd_query_attr - Helper function for composing attribute requests
>> + * hba: per-adapter instance
>> + * opcode: attribute opcode
>> + * idn: attribute idn to access
>> + * index: index field
>> + * selector: selector field
>> + * attr_val: the attribute value after the query request completes
>> + *
>> + * Returns 0 for success, non-zero in case of failure
>> +*/
>> +int ufshcd_query_attr(struct ufs_hba *hba, enum query_opcode opcode,
>> +			enum attr_idn idn, u8 index, u8 selector, u32 *attr_val)
>> +{
>> +	struct ufs_query_req *query;
>> +	struct ufs_query_res *response;
>> +	int err = -ENOMEM;
>> +
>> +	if (!attr_val) {
>> +		dev_err(hba->dev, "%s: attribute value required for write request\n",
> It's trivial, but message is only focused on write.
> attr_val is also needed in case read request.

That's right.

>
> Thanks,
> Seungwon Jeon
>
>> +				__func__);
>> +		err = -EINVAL;
>> +		goto out;
>> +	}
>> +
>> +	query = kzalloc(sizeof(struct ufs_query_req), GFP_KERNEL);
>> +	if (!query) {
>> +		dev_err(hba->dev,
>> +			"%s: Failed allocating ufs_query_req instance\n",
>> +			__func__);
>> +		goto out;
>> +	}
>> +
>> +	response = kzalloc(sizeof(struct ufs_query_res), GFP_KERNEL);
>> +	if (!response) {
>> +		dev_err(hba->dev,
>> +			"%s: Failed allocating ufs_query_res instance\n",
>> +			__func__);
>> +		goto out_free_query;
>> +	}
>> +
>> +	switch (opcode) {
>> +	case UPIU_QUERY_OPCODE_WRITE_ATTR:
>> +		query->query_func = UPIU_QUERY_FUNC_STANDARD_WRITE_REQUEST;
>> +		query->upiu_req.value = *attr_val;
>> +		break;
>> +	case UPIU_QUERY_OPCODE_READ_ATTR:
>> +		query->query_func = UPIU_QUERY_FUNC_STANDARD_READ_REQUEST;
>> +		break;
>> +	default:
>> +		dev_err(hba->dev, "%s: Expected query attr opcode but got = 0x%.2x\n",
>> +				__func__, opcode);
>> +		err = -EINVAL;
>> +		goto out_free;
>> +	}
>> +
>> +	query->upiu_req.opcode = opcode;
>> +	query->upiu_req.idn = idn;
>> +	query->upiu_req.index = index;
>> +	query->upiu_req.selector = selector;
>> +
>> +	/* Send query request */
>> +	err = ufshcd_send_query_request(hba, query, NULL, response);
>> +
>> +	if (err) {
>> +		dev_err(hba->dev, "%s: opcode 0x%.2x for idn %d failed, err = %d\n",
>> +				__func__, opcode, idn, err);
>> +		goto out_free;
>> +	}
>> +
>> +	*attr_val = response->upiu_res.value;
>> +
>> +out_free:
>> +	kfree(response);
>> +out_free_query:
>> +	kfree(query);
>> +out:
>> +	return err;
>> +}
>> +
>> +/**
>>    * ufshcd_memory_alloc - allocate memory for host memory space data structures
>>    * @hba: per adapter instance
>>    *
>> @@ -1842,6 +1937,9 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
>>   			 */
>>   			scsi_status = result & MASK_SCSI_STATUS;
>>   			result = ufshcd_scsi_cmd_status(lrbp, scsi_status);
>> +
>> +			if (ufshcd_is_exception_event(lrbp->ucd_rsp_ptr))
>> +				schedule_work(&hba->eeh_work);
>>   			break;
>>   		case UPIU_TRANSACTION_REJECT_UPIU:
>>   			/* TODO: handle Reject UPIU Response */
>> @@ -1942,6 +2040,215 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
>>   }
>>
>>   /**
>> + * ufshcd_disable_ee - disable exception event
>> + * @hba: per-adapter instance
>> + * @mask: exception event to disable
>> + *
>> + * Disables exception event in the device so that the EVENT_ALERT
>> + * bit is not set.
>> + *
>> + * Returns zero on success, non-zero error value on failure.
>> + */
>> +static int ufshcd_disable_ee(struct ufs_hba *hba, u16 mask)
>> +{
>> +	int err = 0;
>> +	u32 val;
>> +
>> +	if (!(hba->ee_ctrl_mask & mask))
>> +		goto out;
>> +
>> +	val = hba->ee_ctrl_mask & ~mask;
>> +	val &= 0xFFFF; /* 2 bytes */
>> +	err = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
>> +			QUERY_ATTR_IDN_EE_CONTROL, 0, 0, &val);
>> +	if (!err)
>> +		hba->ee_ctrl_mask &= ~mask;
>> +out:
>> +	return err;
>> +}
>> +
>> +/**
>> + * ufshcd_enable_ee - enable exception event
>> + * @hba: per-adapter instance
>> + * @mask: exception event to enable
>> + *
>> + * Enable corresponding exception event in the device to allow
>> + * device to alert host in critical scenarios.
>> + *
>> + * Returns zero on success, non-zero error value on failure.
>> + */
>> +static int ufshcd_enable_ee(struct ufs_hba *hba, u16 mask)
>> +{
>> +	int err = 0;
>> +	u32 val;
>> +
>> +	if (hba->ee_ctrl_mask & mask)
>> +		goto out;
>> +
>> +	val = hba->ee_ctrl_mask | mask;
>> +	val &= 0xFFFF; /* 2 bytes */
>> +	err = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
>> +			QUERY_ATTR_IDN_EE_CONTROL, 0, 0, &val);
>> +	if (!err)
>> +		hba->ee_ctrl_mask |= mask;
>> +out:
>> +	return err;
>> +}
>> +
>> +/**
>> + * ufshcd_enable_auto_bkops - Allow device managed BKOPS
>> + * @hba: per-adapter instance
>> + *
>> + * Allow device to manage background operations on its own. Enabling
>> + * this might lead to inconsistent latencies during normal data transfers
>> + * as the device is allowed to manage its own way of handling background
>> + * operations.
>> + *
>> + * Returns zero on success, non-zero on failure.
>> + */
>> +static int ufshcd_enable_auto_bkops(struct ufs_hba *hba)
>> +{
>> +	int err = 0;
>> +
>> +	if (hba->auto_bkops_enabled)
>> +		goto out;
>> +
>> +	err = ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_SET_FLAG,
>> +			QUERY_FLAG_IDN_BKOPS_EN, NULL);
>> +	if (err) {
>> +		dev_err(hba->dev, "%s: failed to enable bkops %d\n",
>> +				__func__, err);
>> +		goto out;
>> +	}
>> +
>> +	hba->auto_bkops_enabled = true;
>> +
>> +	/* No need of URGENT_BKOPS exception from the device */
>> +	err = ufshcd_disable_ee(hba, MASK_EE_URGENT_BKOPS);
>> +	if (err)
>> +		dev_err(hba->dev, "%s: failed to disable exception event %d\n",
>> +				__func__, err);
>> +out:
>> +	return err;
>> +}
>> +
>> +/**
>> + * ufshcd_disable_auto_bkops - block device in doing background operations
>> + * @hba: per-adapter instance
>> + *
>> + * Disabling background operations improves command response latency but
>> + * has drawback of device moving into critical state where the device is
>> + * not-operable. Make sure to call ufshcd_enable_auto_bkops() whenever the
>> + * host is idle so that BKOPS are managed effectively without any negative
>> + * impacts.
>> + *
>> + * Returns zero on success, non-zero on failure.
>> + */
>> +static int ufshcd_disable_auto_bkops(struct ufs_hba *hba)
>> +{
>> +	int err = 0;
>> +
>> +	if (!hba->auto_bkops_enabled)
>> +		goto out;
>> +
>> +	/*
>> +	 * If host assisted BKOPs is to be enabled, make sure
>> +	 * urgent bkops exception is allowed.
>> +	 */
>> +	err = ufshcd_enable_ee(hba, MASK_EE_URGENT_BKOPS);
>> +	if (err) {
>> +		dev_err(hba->dev, "%s: failed to enable exception event %d\n",
>> +				__func__, err);
>> +		goto out;
>> +	}
>> +
>> +	err = ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_CLEAR_FLAG,
>> +			QUERY_FLAG_IDN_BKOPS_EN, NULL);
>> +	if (err) {
>> +		dev_err(hba->dev, "%s: failed to disable bkops %d\n",
>> +				__func__, err);
>> +		ufshcd_disable_ee(hba, MASK_EE_URGENT_BKOPS);
>> +		goto out;
>> +	}
>> +
>> +	hba->auto_bkops_enabled = false;
>> +out:
>> +	return err;
>> +}
>> +
>> +static inline int ufshcd_get_bkops_status(struct ufs_hba *hba, u32 *status)
>> +{
>> +	return ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
>> +			QUERY_ATTR_IDN_BKOPS_STATUS, 0, 0, status);
>> +}
>> +
>> +/**
>> + * ufshcd_urgent_bkops - handle urgent bkops exception event
>> + * @hba: per-adapter instance
>> + *
>> + * Enable fBackgroundOpsEn flag in the device to permit background
>> + * operations.
>> + */
>> +static int ufshcd_urgent_bkops(struct ufs_hba *hba)
>> +{
>> +	int err;
>> +	u32 status = 0;
>> +
>> +	err = ufshcd_get_bkops_status(hba, &status);
>> +	if (err) {
>> +		dev_err(hba->dev, "%s: failed to get BKOPS status %d\n",
>> +				__func__, err);
>> +		goto out;
>> +	}
>> +
>> +	status = status & 0xF;
>> +
>> +	/* handle only if status indicates performance impact or critical */
>> +	if (status >= BKOPS_STATUS_PERF_IMPACT)
>> +		err = ufshcd_enable_auto_bkops(hba);
>> +out:
>> +	return err;
>> +}
>> +
>> +static inline int ufshcd_get_ee_status(struct ufs_hba *hba, u32 *status)
>> +{
>> +	return ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
>> +			QUERY_ATTR_IDN_EE_STATUS, 0, 0, status);
>> +}
>> +
>> +/**
>> + * ufshcd_exception_event_handler - handle exceptions raised by device
>> + * @work: pointer to work data
>> + *
>> + * Read bExceptionEventStatus attribute from the device and handle the
>> + * exception event accordingly.
>> + */
>> +static void ufshcd_exception_event_handler(struct work_struct *work)
>> +{
>> +	struct ufs_hba *hba;
>> +	int err;
>> +	u32 status = 0;
>> +	hba = container_of(work, struct ufs_hba, eeh_work);
>> +
>> +	err = ufshcd_get_ee_status(hba, &status);
>> +	if (err) {
>> +		dev_err(hba->dev, "%s: failed to get exception status %d\n",
>> +				__func__, err);
>> +		goto out;
>> +	}
>> +
>> +	status &= hba->ee_ctrl_mask;
>> +	if (status & MASK_EE_URGENT_BKOPS) {
>> +		err = ufshcd_urgent_bkops(hba);
>> +		if (err)
>> +			dev_err(hba->dev, "%s: failed to handle urgent bkops %d\n",
>> +					__func__, err);
>> +	}
>> +out:
>> +	return;
>> +}
>> +
>> +/**
>>    * ufshcd_fatal_err_handler - handle fatal errors
>>    * @hba: per adapter instance
>>    */
>> @@ -2252,6 +2559,8 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
>>   	if (ret)
>>   		goto out;
>>
>> +	hba->auto_bkops_enabled = false;
>> +	ufshcd_enable_auto_bkops(hba);
>>   	scsi_scan_host(hba->host);
>>   out:
>>   	return;
>> @@ -2316,6 +2625,34 @@ int ufshcd_resume(struct ufs_hba *hba)
>>   }
>>   EXPORT_SYMBOL_GPL(ufshcd_resume);
>>
>> +int ufshcd_runtime_suspend(struct ufs_hba *hba)
>> +{
>> +	if (!hba)
>> +		return 0;
>> +
>> +	/*
>> +	 * The device is idle with no requests in the queue,
>> +	 * allow background operations.
>> +	 */
>> +	return ufshcd_enable_auto_bkops(hba);
>> +}
>> +EXPORT_SYMBOL(ufshcd_runtime_suspend);
>> +
>> +int ufshcd_runtime_resume(struct ufs_hba *hba)
>> +{
>> +	if (!hba)
>> +		return 0;
>> +
>> +	return ufshcd_disable_auto_bkops(hba);
>> +}
>> +EXPORT_SYMBOL(ufshcd_runtime_resume);
>> +
>> +int ufshcd_runtime_idle(struct ufs_hba *hba)
>> +{
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(ufshcd_runtime_idle);
>> +
>>   /**
>>    * ufshcd_remove - de-allocate SCSI host and host memory space
>>    *		data structure memory
>> @@ -2406,6 +2743,7 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
>>
>>   	/* Initialize work queues */
>>   	INIT_WORK(&hba->feh_workq, ufshcd_fatal_err_handler);
>> +	INIT_WORK(&hba->eeh_work, ufshcd_exception_event_handler);
>>
>>   	/* Initialize UIC command mutex */
>>   	mutex_init(&hba->uic_cmd_mutex);
>> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
>> index c6aeb6d..6c9bd35 100644
>> --- a/drivers/scsi/ufs/ufshcd.h
>> +++ b/drivers/scsi/ufs/ufshcd.h
>> @@ -178,9 +178,12 @@ struct ufs_dev_cmd {
>>    * @tm_condition: condition variable for task management
>>    * @ufshcd_state: UFSHCD states
>>    * @intr_mask: Interrupt Mask Bits
>> + * @ee_ctrl_mask: Exception event control mask
>>    * @feh_workq: Work queue for fatal controller error handling
>> + * @eeh_work: Worker to handle exception events
>>    * @errors: HBA errors
>>    * @dev_cmd: ufs device management command information
>> + * @auto_bkops_enabled: to track whether bkops is enabled in device
>>    */
>>   struct ufs_hba {
>>   	void __iomem *mmio_base;
>> @@ -218,15 +221,19 @@ struct ufs_hba {
>>
>>   	u32 ufshcd_state;
>>   	u32 intr_mask;
>> +	u16 ee_ctrl_mask;
>>
>>   	/* Work Queues */
>>   	struct work_struct feh_workq;
>> +	struct work_struct eeh_work;
>>
>>   	/* HBA Errors */
>>   	u32 errors;
>>
>>   	/* Device management request data */
>>   	struct ufs_dev_cmd dev_cmd;
>> +
>> +	bool auto_bkops_enabled;
>>   };
>>
>>   #define ufshcd_writel(hba, val, reg)	\
>> @@ -247,4 +254,7 @@ static inline void ufshcd_hba_stop(struct ufs_hba *hba)
>>   	ufshcd_writel(hba, CONTROLLER_DISABLE,  REG_CONTROLLER_ENABLE);
>>   }
>>
>> +extern int ufshcd_runtime_suspend(struct ufs_hba *hba);
>> +extern int ufshcd_runtime_resume(struct ufs_hba *hba);
>> +extern int ufshcd_runtime_idle(struct ufs_hba *hba);
>>   #endif /* End of Header */

-- 
Regards,
Sujit

Re: [PATCH V3 2/2] scsi: ufs: Add runtime PM support for UFS host controller driver

[Sujit Reddy Thumma]

On 7/10/2013 7:01 PM, Seungwon Jeon wrote:
> On Tue, July 09, 2013, Sujit Reddy Thumma wrote:
>> Add runtime PM helpers to suspend/resume UFS controller at runtime.
>> Enable runtime PM by default for pci and platform drivers as the
>> initialized hardware can suspend if it is not used after bootup.
>>
>> Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
>> ---
>>   drivers/scsi/ufs/ufshcd-pci.c    |   65 ++++++++++++++++++++++++++++++++++---
>>   drivers/scsi/ufs/ufshcd-pltfrm.c |   51 +++++++++++++++++++++++++++++-
>>   drivers/scsi/ufs/ufshcd.c        |    8 +++++
>>   3 files changed, 117 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd-pci.c b/drivers/scsi/ufs/ufshcd-pci.c
>> index 48be39a..7bd8faa 100644
>> --- a/drivers/scsi/ufs/ufshcd-pci.c
>> +++ b/drivers/scsi/ufs/ufshcd-pci.c
>> @@ -35,6 +35,7 @@
>>
>>   #include "ufshcd.h"
>>   #include <linux/pci.h>
>> +#include <linux/pm_runtime.h>
>>
>>   #ifdef CONFIG_PM
>>   /**
>> @@ -44,7 +45,7 @@
>>    *
>>    * Returns -ENOSYS
>>    */
>> -static int ufshcd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
>> +static int ufshcd_pci_suspend(struct device *dev)
>>   {
>>   	/*
>>   	 * TODO:
>> @@ -61,7 +62,7 @@ static int ufshcd_pci_suspend(struct pci_dev *pdev, pm_message_t state)
>>    *
>>    * Returns -ENOSYS
>>    */
>> -static int ufshcd_pci_resume(struct pci_dev *pdev)
>> +static int ufshcd_pci_resume(struct device *dev)
>>   {
>>   	/*
>>   	 * TODO:
>> @@ -71,8 +72,48 @@ static int ufshcd_pci_resume(struct pci_dev *pdev)
>>
>>   	return -ENOSYS;
>>   }
>> +#else
>> +#define ufshcd_pci_suspend	NULL
>> +#define ufshcd_pci_resume	NULL
>>   #endif /* CONFIG_PM */
>>
>> +#ifdef CONFIG_PM_RUNTIME
>> +static int ufshcd_pci_runtime_suspend(struct device *dev)
>> +{
>> +	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
>> +	struct ufs_hba *hba = pci_get_drvdata(pdev);
> Can be replaced:
> 	struct ufs_hba *hba = dev_get_drvdata(dev);

Yes.

>
>> +
>> +	if (!hba)
>> +		return 0;
>> +
>> +	return ufshcd_runtime_suspend(hba);
>> +}
>> +static int ufshcd_pci_runtime_resume(struct device *dev)
>> +{
>> +	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
>> +	struct ufs_hba *hba = pci_get_drvdata(pdev);
> Same as above.

Okay.

>
>
>> +
>> +	if (!hba)
>> +		return 0;
>> +
>> +	return ufshcd_runtime_resume(hba);
>> +}
>> +static int ufshcd_pci_runtime_idle(struct device *dev)
>> +{
>> +	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
>> +	struct ufs_hba *hba = pci_get_drvdata(pdev);
> Same as above.

Okay.
>
>> +
>> +	if (!hba)
>> +		return 0;
>> +
>> +	return ufshcd_runtime_idle(hba);
>> +}
>> +#else /* !CONFIG_PM_RUNTIME */
>> +#define ufshcd_pci_runtime_suspend	NULL
>> +#define ufshcd_pci_runtime_resume	NULL
>> +#define ufshcd_pci_runtime_idle	NULL
>> +#endif /* CONFIG_PM_RUNTIME */
>> +
>>   /**
>>    * ufshcd_pci_shutdown - main function to put the controller in reset state
>>    * @pdev: pointer to PCI device handle
>> @@ -91,6 +132,9 @@ static void ufshcd_pci_remove(struct pci_dev *pdev)
>>   {
>>   	struct ufs_hba *hba = pci_get_drvdata(pdev);
>>
>> +	pm_runtime_forbid(&pdev->dev);
>> +	pm_runtime_get_noresume(&pdev->dev);
>> +
>>   	disable_irq(pdev->irq);
>>   	ufshcd_remove(hba);
>>   	pci_release_regions(pdev);
>> @@ -168,6 +212,8 @@ ufshcd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>   	}
>>
>>   	pci_set_drvdata(pdev, hba);
>> +	pm_runtime_put_noidle(&pdev->dev);
>> +	pm_runtime_allow(&pdev->dev);
>>
>>   	return 0;
>>
>> @@ -182,6 +228,14 @@ out_error:
>>   	return err;
>>   }
>>
>> +static const struct dev_pm_ops ufshcd_pci_pm_ops = {
>> +	.suspend	= ufshcd_pci_suspend,
>> +	.resume		= ufshcd_pci_resume,
>> +	.runtime_suspend = ufshcd_pci_runtime_suspend,
>> +	.runtime_resume  = ufshcd_pci_runtime_resume,
>> +	.runtime_idle    = ufshcd_pci_runtime_idle,
>> +};
>> +
>>   static DEFINE_PCI_DEVICE_TABLE(ufshcd_pci_tbl) = {
>>   	{ PCI_VENDOR_ID_SAMSUNG, 0xC00C, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
>>   	{ }	/* terminate list */
>> @@ -195,10 +249,9 @@ static struct pci_driver ufshcd_pci_driver = {
>>   	.probe = ufshcd_pci_probe,
>>   	.remove = ufshcd_pci_remove,
>>   	.shutdown = ufshcd_pci_shutdown,
>> -#ifdef CONFIG_PM
>> -	.suspend = ufshcd_pci_suspend,
>> -	.resume = ufshcd_pci_resume,
>> -#endif
>> +	.driver = {
>> +		.pm = &ufshcd_pci_pm_ops
>> +	},
>>   };
>>
>>   module_pci_driver(ufshcd_pci_driver);
>> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
>> index c42db40..b1f2605 100644
>> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c
>> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
>> @@ -34,6 +34,7 @@
>>    */
>>
>>   #include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>>
>>   #include "ufshcd.h"
>>
>> @@ -87,6 +88,43 @@ static int ufshcd_pltfrm_resume(struct device *dev)
>>   #define ufshcd_pltfrm_resume	NULL
>>   #endif
>>
>> +#ifdef CONFIG_PM_RUNTIME
>> +static int ufshcd_pltfrm_runtime_suspend(struct device *dev)
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct ufs_hba *hba =  platform_get_drvdata(pdev);
> Same as above.
Okay.

>
>> +
>> +	if (!hba)
>> +		return 0;
>> +
>> +	return ufshcd_runtime_suspend(hba);
>> +}
>> +static int ufshcd_pltfrm_runtime_resume(struct device *dev)
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct ufs_hba *hba =  platform_get_drvdata(pdev);
> Same as above.

Okay.

>
>> +
>> +	if (!hba)
>> +		return 0;
>> +
>> +	return ufshcd_runtime_resume(hba);
>> +}
>> +static int ufshcd_pltfrm_runtime_idle(struct device *dev)
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct ufs_hba *hba =  platform_get_drvdata(pdev);
> Same as above.
Okay.
>
>> +
>> +	if (!hba)
>> +		return 0;
>> +
>> +	return ufshcd_runtime_idle(hba);
>> +}
>> +#else /* !CONFIG_PM_RUNTIME */
>> +#define ufshcd_pltfrm_runtime_suspend	NULL
>> +#define ufshcd_pltfrm_runtime_resume	NULL
>> +#define ufshcd_pltfrm_runtime_idle	NULL
>> +#endif /* CONFIG_PM_RUNTIME */
>> +
>>   /**
>>    * ufshcd_pltfrm_probe - probe routine of the driver
>>    * @pdev: pointer to Platform device handle
>> @@ -122,14 +160,20 @@ static int ufshcd_pltfrm_probe(struct platform_device *pdev)
>>   		goto out;
>>   	}
>>
>> +	pm_runtime_set_active(&pdev->dev);
>> +	pm_runtime_enable(&pdev->dev);
>> +
>>   	err = ufshcd_init(dev, &hba, mmio_base, irq);
>>   	if (err) {
>>   		dev_err(dev, "Intialization failed\n");
>> -		goto out;
>> +		goto out_disable_rpm;
>>   	}
>>
>>   	platform_set_drvdata(pdev, hba);
>>
>> +out_disable_rpm:
>> +	pm_runtime_disable(&pdev->dev);
>> +	pm_runtime_set_suspended(&pdev->dev);
>>   out:
>>   	return err;
>>   }
>> @@ -144,6 +188,8 @@ static int ufshcd_pltfrm_remove(struct platform_device *pdev)
>>   {
>>   	struct ufs_hba *hba =  platform_get_drvdata(pdev);
>>
>> +	pm_runtime_get_sync(&(pdev)->dev);
>> +
>>   	disable_irq(hba->irq);
>>   	ufshcd_remove(hba);
>>   	return 0;
>> @@ -157,6 +203,9 @@ static const struct of_device_id ufs_of_match[] = {
>>   static const struct dev_pm_ops ufshcd_dev_pm_ops = {
>>   	.suspend	= ufshcd_pltfrm_suspend,
>>   	.resume		= ufshcd_pltfrm_resume,
>> +	.runtime_suspend = ufshcd_pltfrm_runtime_suspend,
>> +	.runtime_resume  = ufshcd_pltfrm_runtime_resume,
>> +	.runtime_idle    = ufshcd_pltfrm_runtime_idle,
>>   };
>>
>>   static struct platform_driver ufshcd_pltfrm_driver = {
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index a25de66..af7d01d 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -2230,6 +2230,7 @@ static void ufshcd_exception_event_handler(struct work_struct *work)
>>   	u32 status = 0;
>>   	hba = container_of(work, struct ufs_hba, eeh_work);
>>
>> +	pm_runtime_get_sync(hba->dev);
>>   	err = ufshcd_get_ee_status(hba, &status);
>>   	if (err) {
>>   		dev_err(hba->dev, "%s: failed to get exception status %d\n",
>> @@ -2245,6 +2246,7 @@ static void ufshcd_exception_event_handler(struct work_struct *work)
>>   					__func__, err);
>>   	}
>>   out:
>> +	pm_runtime_put_sync(hba->dev);
> In ufshcd_urgent_bkops, ufshcd_enable_auto_bkops will be executed.

That should be still fine. We have necessary checks in
ufshcd_enable_auto_bkops() to skip enabling if already enabled.


>
>>   	return;
>>   }
>>
>> @@ -2257,9 +2259,11 @@ static void ufshcd_fatal_err_handler(struct work_struct *work)
>>   	struct ufs_hba *hba;
>>   	hba = container_of(work, struct ufs_hba, feh_workq);
>>
>> +	pm_runtime_get_sync(hba->dev);
>>   	/* check if reset is already in progress */
>>   	if (hba->ufshcd_state != UFSHCD_STATE_RESET)
>>   		ufshcd_do_reset(hba);
>> +	pm_runtime_put_sync(hba->dev);
>>   }
>>
>>   /**
>> @@ -2562,6 +2566,7 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
>>   	hba->auto_bkops_enabled = false;
> In the end of 'ufshcd_init', pm_runtime_get_sync is called, hence auto_bkops_enabled may has false.
> 'auto_bkops_enabled = false' is needed?

The device may undergo reset before this, and so the internal device
flag may be reset to default (which is auto bkops enabled). The two
steps here - auto_bkops_enabled = false and  ufshcd_enabled_auto_bkops()
ensure that s/w variables are aligned with h/w state.

>
>>   	ufshcd_enable_auto_bkops(hba);
> Should 'ufshcd_enable_auto_bkops' be done here?

Not really needed. But just to make sure that bootloader didn't mess
up with it and the s/w variables are still pointing that auto_bkops is
disabled.

>
>>   	scsi_scan_host(hba->host);
>> +	pm_runtime_put_sync(hba->dev);
> Practically, after pm_runtime_put_sync, ufshcd_enable_auto_bkops will be executed.
> Then, it's duplication of ufshcd_enable_auto_bkops?
> ufshcd_enable_auto_bkops is already done above.

ufshcd_enable_auto_bkops() has necessary checks to make sure that we
are not re-enabling if already enabled.

One thing to note here is that pm_runtime_get_sync() and put_sync()
are independent of the bkops status. The runtime PM make sures that
after device idle'd suspend is triggered. What we do in suspend routine
is not of a concern. Moreover, pm_runtime_get_sync() calls are just
dummy calls when CONFIG_RUNTIME_PM is disabled so we can't assume
anything with that and make the sequence complex.

>
> And actual routine of scsi_scan_host performs asynchronously.
> That means it's not completed. So, pm_runtime_put_sync looks like improper here.

After this operation, the device is sure to respond to any command
including those required for bkops. So put_sync is proper here.
If there is any new command issued to the controller as part
of scsi_scan_host() and the host is already suspended it will be
resumed, we are not breaking the parent-child relation ship so any new
device when added as a child triggers runtime resume for parent (if
suspended).

>
>>   out:
>>   	return;
>>   }
>> @@ -2783,6 +2788,9 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
>>
>>   	*hba_handle = hba;
>>
>> +	/* Hold auto suspend until async scan completes */
> What's meaning of comment?

We haven't yet initialized the device so runtime PM suspned/resume may
fail. Until async scan is scheduled we take PM reference count to make
sure device is not suspended inbetween.


>
> Thanks,
> Seungwon Jeon
>
>> +	pm_runtime_get_sync(dev);
>> +
>>   	async_schedule(ufshcd_async_scan, hba);
>>
>>   	return 0;
>> --
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
>> of Code Aurora Forum, hosted by The Linux Foundation.
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-- 
Regards,
Sujit

RE: [PATCH V3 1/2] scsi: ufs: Add support for host assisted background operations

[Seungwon Jeon]

On Thu, July 11, 2013, Sujit Reddy Thumma wrote:
> On 7/10/2013 7:01 PM, Seungwon Jeon wrote:
> > I'm not sure that BKOPS with runtime-pm associates.
> > Do you think it's helpful for power management?
> > How about hibernation scheme for runtime-pm?
> > I'm testing and I can introduce soon.
> 
> Well, I am thinking on following approach when we introduce
> power management.
> 
> ufshcd_runtime_suspend() {
> 	if (bkops_status >= NON_CRITICAL) { /* 0x1 */
> 		ufshcd_enable_auto_bkops();
> 		hibernate(); /* only the link and the device
> 			should be able to cary out bkops */
> 	} else {
> 		hibernate(); /* Link and the device for more savings */
> 	}
> }
> 
> Let me know if this is okay.
I still consider whether BKOPS is proper behavior with runtime-pm or not.
How about this scenario? It seems more simple.
If we concern a response latency of transfer requests, BKOPS can be disabled by default.
And then BKOPS can be enabled whenever device requests in some exception.
If you have any idea, let me know.

Thanks,
Seungwon Jeon

Re: [PATCH V3 1/2] scsi: ufs: Add support for host assisted background operations

[Sujit Reddy Thumma]

>>> > >I'm not sure that BKOPS with runtime-pm associates.
>>> > >Do you think it's helpful for power management?
>>> > >How about hibernation scheme for runtime-pm?
>>> > >I'm testing and I can introduce soon.
>> >
>> >Well, I am thinking on following approach when we introduce
>> >power management.
>> >
>> >ufshcd_runtime_suspend() {
>> >	if (bkops_status >= NON_CRITICAL) { /* 0x1 */
>> >		ufshcd_enable_auto_bkops();
>> >		hibernate(); /* only the link and the device
>> >			should be able to cary out bkops */
>> >	} else {
>> >		hibernate(); /* Link and the device for more savings */
>> >	}
>> >}
>> >
>> >Let me know if this is okay.
> I still consider whether BKOPS is proper behavior with runtime-pm or not.

The BKOPS is something that host allows the card to carry out
when the host knows it is idle and not expecting back to back requests.
Runtime PM idle is the only way to know whether the device is
idle (unless we want to reinvent the wheel to detect the idleness of
host and trigger bkops). There was a discussion on this in MMC mailing
list as well, and folks have agreed to move idle time bkops to runtime
PM (http://thread.gmane.org/gmane.linux.kernel.mmc/19444/)

> How about this scenario? It seems more simple.
> If we concern a response latency of transfer requests, BKOPS can be disabled by default.
> And then BKOPS can be enabled whenever device requests in some exception.
> If you have any idea, let me know.

Exceptions are raised only when the device is in critical need for
bkops. Also the spec. recommends, host should ensure that the device
doesn't go into such states.

With your suggestion, when we disable bkops, the exception is raised and 
we enable bkops after which there is no way to disable it again?

-- 
Regards,
Sujit

RE: [PATCH V3 1/2] scsi: ufs: Add support for host assisted background operations

[Seungwon Jeon]

On Thu, July 18, 2013, Sujit Reddy Thumma wrote:
> >>> > >I'm not sure that BKOPS with runtime-pm associates.
> >>> > >Do you think it's helpful for power management?
> >>> > >How about hibernation scheme for runtime-pm?
> >>> > >I'm testing and I can introduce soon.
> >> >
> >> >Well, I am thinking on following approach when we introduce
> >> >power management.
> >> >
> >> >ufshcd_runtime_suspend() {
> >> >	if (bkops_status >= NON_CRITICAL) { /* 0x1 */
> >> >		ufshcd_enable_auto_bkops();
> >> >		hibernate(); /* only the link and the device
> >> >			should be able to cary out bkops */
> >> >	} else {
> >> >		hibernate(); /* Link and the device for more savings */
> >> >	}
> >> >}
> >> >
> >> >Let me know if this is okay.
> > I still consider whether BKOPS is proper behavior with runtime-pm or not.
> 
> The BKOPS is something that host allows the card to carry out
> when the host knows it is idle and not expecting back to back requests.
> Runtime PM idle is the only way to know whether the device is
> idle (unless we want to reinvent the wheel to detect the idleness of
> host and trigger bkops). There was a discussion on this in MMC mailing
> list as well, and folks have agreed to move idle time bkops to runtime
> PM (http://thread.gmane.org/gmane.linux.kernel.mmc/19444/)
It looks like different.
eMMC cannot execute BKOPS itself unlike UFS.
That's the way eMMC's host should trigger the BKOPS manually.

> 
> > How about this scenario? It seems more simple.
	> > If we concern a response latency of transfer requests, BKOPS can be disabled by default.
> > And then BKOPS can be enabled whenever device requests in some exception.
> > If you have any idea, let me know.
> 
> Exceptions are raised only when the device is in critical need for
> bkops. Also the spec. recommends, host should ensure that the device
> doesn't go into such states.
> 
> With your suggestion, when we disable bkops, the exception is raised and
> we enable bkops after which there is no way to disable it again?
Yes, it's difficult to find proper time. 
Maybe, BKOPS can be disabled when request comes up.

Thanks,
Seungwon Jeon
> 
> --
> Regards,
> Sujit
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 1/2] scsi: ufs: Add support for host assisted background operations

[Sujit Reddy Thumma]

>>>>>>> I'm not sure that BKOPS with runtime-pm associates.
>>>>>>> Do you think it's helpful for power management?
>>>>>>> How about hibernation scheme for runtime-pm?
>>>>>>> I'm testing and I can introduce soon.
>>>>>
>>>>> Well, I am thinking on following approach when we introduce
>>>>> power management.
>>>>>
>>>>> ufshcd_runtime_suspend() {
>>>>> 	if (bkops_status >= NON_CRITICAL) { /* 0x1 */
>>>>> 		ufshcd_enable_auto_bkops();
>>>>> 		hibernate(); /* only the link and the device
>>>>> 			should be able to cary out bkops */
>>>>> 	} else {
>>>>> 		hibernate(); /* Link and the device for more savings */
>>>>> 	}
>>>>> }
>>>>>
>>>>> Let me know if this is okay.
>>> I still consider whether BKOPS is proper behavior with runtime-pm or not.
>>
>> The BKOPS is something that host allows the card to carry out
>> when the host knows it is idle and not expecting back to back requests.
>> Runtime PM idle is the only way to know whether the device is
>> idle (unless we want to reinvent the wheel to detect the idleness of
>> host and trigger bkops). There was a discussion on this in MMC mailing
>> list as well, and folks have agreed to move idle time bkops to runtime
>> PM (http://thread.gmane.org/gmane.linux.kernel.mmc/19444/)
> It looks like different.
> eMMC cannot execute BKOPS itself unlike UFS.
> That's the way eMMC's host should trigger the BKOPS manually.
>

I guess it is not much of a difference for UFS as far as we concern
only about idle time bkops. In UFS one can still disallow the device to
not carry out BKOPS and hence the case where UFS device also cannot
execute BKOPS itself and a idle timer is needed to allow BKOPS
sporadically so that device doesn't go into URGENT_BKOPS state.

>>
>>> How about this scenario? It seems more simple.
> 	> > If we concern a response latency of transfer requests, BKOPS can be disabled by default.
>>> And then BKOPS can be enabled whenever device requests in some exception.
>>> If you have any idea, let me know.
>>
>> Exceptions are raised only when the device is in critical need for
>> bkops. Also the spec. recommends, host should ensure that the device
>> doesn't go into such states.
>>
>> With your suggestion, when we disable bkops, the exception is raised and
>> we enable bkops after which there is no way to disable it again?
> Yes, it's difficult to find proper time.
> Maybe, BKOPS can be disabled when request comes up.
>
In cases where there are back-to-back heavy data write requests then it
is not proper to enable/disable BKOPS as soon as the new request comes
up. It may happen that the card will then move from PERFORMANCE_IMPACT
state to CRITICAL and ultimately start failing the requests. The point
is that we should never get into state where we need URGENT_BKOPS.

-- 
Regards,
Sujit