scsi: ufs: rework link start-up process

PATCH Superseded Seungwon Jeon 4 participants 14 messages Apr 24, 2013 – May 4, 2013 SCSI

Share & tools

Seungwon Jeon

author

2013-04-24 16:06 UTC #0 lore raw

PATCH 4/5 scsi: ufs: rework link start-up process

Link start-up requires long time with multiphase handshakes between UFS host and device. This affects driver's probe time. This patch let link start-up run asynchronously.
And completion time of uic command is defined to avoid a permanent wait.

Signed-off-by Seungwon Jeon <[email protected]>

---
 drivers/scsi/ufs/ufshcd.c |  114 +++++++++++++++++++++++++++++++++-----------
 drivers/scsi/ufs/ufshcd.h |    6 ++-
 2 files changed, 89 insertions(+), 31 deletions(-)

drivers/scsi/ufs/ufshcd.c +85 ~~-29~~

index efe2256..76ff332 100644
@@ -38,6 +38,7 @@
 #define UFSHCD_ENABLE_INTRS	(UTP_TRANSFER_REQ_COMPL |\
 				 UTP_TASK_REQ_COMPL |\
 				 UFSHCD_ERROR_MASK)
+#define UIC_CMD_TIMEOUT	100
 
 enum {
 	UFSHCD_MAX_CHANNEL	= 0,

@@ -357,13 +358,15 @@ static inline void ufshcd_hba_capabilities(struct ufs_hba *hba)
 }
 
 /**
- * ufshcd_send_uic_command - Send UIC commands to unipro layers
+ * ufshcd_dispatch_uic_cmd - Dispatch UIC commands to unipro layers
  * @hba: per adapter instance
  * @uic_command: UIC command
  */
 static inline void
-ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
+ufshcd_dispatch_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmnd)
 {
+	init_completion(&uic_cmnd->done);
+
 	/* Write Args */
 	ufshcd_writel(hba, REG_UIC_COMMAND_ARG_1, uic_cmnd->argument1);
 	ufshcd_writel(hba, REG_UIC_COMMAND_ARG_2, uic_cmnd->argument2);

@@ -375,6 +378,45 @@ ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
 }
 
 /**
+ * ufshcd_wait_for_uic_cmd - Wait complectioin of UIC command
+ * @hba: per adapter instance
+ * @uic_command: UIC command
+ *
+ * Returns 0 only if success.
+ */
+static int ufshcd_wait_for_uic_cmd(struct ufs_hba *hba)
+{
+	struct uic_command *uic_cmd = &hba->active_uic_cmd;
+	int ret;
+
+	if (wait_for_completion_timeout(&uic_cmd->done,
+					msecs_to_jiffies(UIC_CMD_TIMEOUT)))
+		ret = ufshcd_get_uic_cmd_result(hba);
+	else
+		ret = -ETIMEDOUT;
+
+	return ret;
+}
+
+/**
+ * ufshcd_ready_uic_cmd - Check if controller is ready
+ *                        to accept UIC commands
+ * @hba: per adapter instance
+ * Return true on success, else false
+ */
+static inline bool ufshcd_ready_uic_cmd(struct ufs_hba *hba)
+{
+	if (ufshcd_readl(hba, REG_CONTROLLER_STATUS) & UIC_COMMAND_READY) {
+		return true;
+	} else {
+		dev_err(hba->dev,
+				"Controller not ready"
+				" to accept UIC commands\n");
+		return false;
+	}
+}
+
+/**
  * ufshcd_map_sg - Map scatter-gather list to prdt
  * @lrbp - pointer to local reference block
  *

@@ -735,15 +777,10 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
 {
 	struct uic_command *uic_cmd;
 	unsigned long flags;
+	int ret;
 
-	/* check if controller is ready to accept UIC commands */
-	if (((ufshcd_readl(hba, REG_CONTROLLER_STATUS)) &
-	    UIC_COMMAND_READY) == 0x0) {
-		dev_err(hba->dev,
-			"Controller not ready"
-			" to accept UIC commands\n");
+	if (!ufshcd_ready_uic_cmd(hba))
 		return -EIO;
-	}
 
 	spin_lock_irqsave(hba->host->host_lock, flags);

@@ -754,13 +791,16 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
 	uic_cmd->argument2 = 0;
 	uic_cmd->argument3 = 0;
 
-	/* enable UIC related interrupts */
-	ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
+	/* Dispatching UIC commands to controller */
+	ufshcd_dispatch_uic_cmd(hba, uic_cmd);
 
-	/* sending UIC commands to controller */
-	ufshcd_send_uic_command(hba, uic_cmd);
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
-	return 0;
+
+	ret = ufshcd_wait_for_uic_cmd(hba);
+	if (ret)
+		dev_err(hba->dev, "link startup: error code %d returned\n", ret);
+
+	return ret;
 }
 
 /**

@@ -898,6 +938,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
 	if (ufshcd_hba_enable(hba))
 		return -EIO;
 
+	/* enable UIC related interrupts */
+	ufshcd_enable_intr(hba, UIC_COMMAND_COMPL | UIC_ERROR);
+
 	/* Configure UTRL and UTMRL base address registers */
 	ufshcd_writel(hba, REG_UTP_TRANSFER_REQ_LIST_BASE_L,
 		      lower_32_bits(hba->utrdl_dma_addr));

@@ -909,7 +952,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
 		      upper_32_bits(hba->utmrdl_dma_addr));
 
 	/* Initialize unipro link startup procedure */
-	return ufshcd_dme_link_startup(hba);
+	schedule_work(&hba->link_startup_wq);
+
+	return 0;
 }
 
 /**

@@ -1186,6 +1231,16 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 }
 
 /**
+ * ufshcd_uic_cmd_compl - handle completion of uic command
+ * @hba: per adapter instance
+ */
+static void ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
+{
+	if (intr_status & UIC_COMMAND_COMPL)
+		complete(&hba->active_uic_cmd.done);
+}
+
+/**
  * ufshcd_transfer_req_compl - handle SCSI and query command completion
  * @hba: per adapter instance
  */

@@ -1225,25 +1280,26 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
 }
 
 /**
- * ufshcd_uic_cc_handler - handle UIC command completion
+ * ufshcd_link_startup - link initialization
  * @work: pointer to a work queue structure
- *
- * Returns 0 on success, non-zero value on failure
  */
-static void ufshcd_uic_cc_handler (struct work_struct *work)
+static void ufshcd_link_startup(struct work_struct *work)
 {
 	struct ufs_hba *hba;
+	int ret;
 
-	hba = container_of(work, struct ufs_hba, uic_workq);
+	hba = container_of(work, struct ufs_hba, link_startup_wq);
 
-	if ((hba->active_uic_cmd.command == UIC_CMD_DME_LINK_STARTUP) &&
-	    !(ufshcd_get_uic_cmd_result(hba))) {
+	ret = ufshcd_dme_link_startup(hba);
+	if (ret)
+		goto out;
 
-		if (ufshcd_make_hba_operational(hba))
-			dev_err(hba->dev,
-				"cc: hba not operational state\n");
-		return;
-	}
+	ret = ufshcd_make_hba_operational(hba);
+	if (ret)
+		goto out;
+	return;
+out:
+	dev_err(hba->dev, "link startup failed %d\n", ret);
 }
 
 /**

@@ -1307,7 +1363,7 @@ static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
 		ufshcd_err_handler(hba);
 
 	if (intr_status & UIC_COMMAND_COMPL)
-		schedule_work(&hba->uic_workq);
+		ufshcd_uic_cmd_compl(hba, intr_status);
 
 	if (intr_status & UTP_TASK_REQ_COMPL)
 		ufshcd_tmc_handler(hba);

@@ -1694,7 +1750,7 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
 	init_waitqueue_head(&hba->ufshcd_tm_wait_queue);
 
 	/* Initialize work queues */
-	INIT_WORK(&hba->uic_workq, ufshcd_uic_cc_handler);
+	INIT_WORK(&hba->link_startup_wq, ufshcd_link_startup);
 	INIT_WORK(&hba->feh_workq, ufshcd_fatal_err_handler);
 
 	/* IRQ registration */

drivers/scsi/ufs/ufshcd.h +4 -2

index 87d5a94..2fb4d94 100644
@@ -51,6 +51,7 @@
 #include <linux/bitops.h>
 #include <linux/pm_runtime.h>
 #include <linux/clk.h>
+#include <linux/completion.h>
 
 #include <asm/irq.h>
 #include <asm/byteorder.h>

@@ -83,6 +84,7 @@ struct uic_command {
 	u32 argument3;
 	int cmd_active;
 	int result;
+	struct completion done;
 };
 
 /**

@@ -140,7 +142,7 @@ struct ufshcd_lrb {
  * @tm_condition: condition variable for task management
  * @ufshcd_state: UFSHCD states
  * @intr_mask: Interrupt Mask Bits
- * @uic_workq: Work queue for UIC completion handling
+ * @link_startup_wq: Work queue for link start-up
  * @feh_workq: Work queue for fatal controller error handling
  * @errors: HBA errors
  */

@@ -179,7 +181,7 @@ struct ufs_hba {
 	u32 intr_mask;
 
 	/* Work Queues */
-	struct work_struct uic_workq;
+	struct work_struct link_startup_wq;
 	struct work_struct feh_workq;
 
 	/* HBA Errors */

Signature · 1 line

1.7.0.4

Sujit Reddy Thumma

2013-04-25 05:05 UTC #1 lore raw

Quoted 5 lines · Seungwon Jeon

Link start-up requires long time with multiphase handshakes between UFS host and device. This affects driver's probe time. This patch let link start-up run asynchronously.
And completion time of uic command is defined to avoid a permanent wait.

I have similar patch posted few days back "scsi: ufs: Generalize UFS Interconnect Layer (UIC) command support" which does a bit more (mutex, error handling) than what is done here. Can that be used/improved?

Quoted 257 lines

Signed-off-by Seungwon Jeon <[email protected]>

---
  drivers/scsi/ufs/ufshcd.c |  114 +++++++++++++++++++++++++++++++++-----------
  drivers/scsi/ufs/ufshcd.h |    6 ++-
  2 files changed, 89 insertions(+), 31 deletions(-)

drivers/scsi/ufs/ufshcd.c +85 ~~-29~~

index efe2256..76ff332 100644
@@ -38,6 +38,7 @@
  #define UFSHCD_ENABLE_INTRS	(UTP_TRANSFER_REQ_COMPL |\
  				 UTP_TASK_REQ_COMPL |\
  				 UFSHCD_ERROR_MASK)
+#define UIC_CMD_TIMEOUT	100
 
  enum {
  	UFSHCD_MAX_CHANNEL	= 0,

@@ -357,13 +358,15 @@ static inline void ufshcd_hba_capabilities(struct ufs_hba *hba)
  }
 
  /**
- * ufshcd_send_uic_command - Send UIC commands to unipro layers
+ * ufshcd_dispatch_uic_cmd - Dispatch UIC commands to unipro layers
   * @hba: per adapter instance
   * @uic_command: UIC command
   */
  static inline void
-ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
+ufshcd_dispatch_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmnd)
  {
+	init_completion(&uic_cmnd->done);
+
  	/* Write Args */
  	ufshcd_writel(hba, REG_UIC_COMMAND_ARG_1, uic_cmnd->argument1);
  	ufshcd_writel(hba, REG_UIC_COMMAND_ARG_2, uic_cmnd->argument2);

@@ -375,6 +378,45 @@ ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
  }
 
  /**
+ * ufshcd_wait_for_uic_cmd - Wait complectioin of UIC command
+ * @hba: per adapter instance
+ * @uic_command: UIC command
+ *
+ * Returns 0 only if success.
+ */
+static int ufshcd_wait_for_uic_cmd(struct ufs_hba *hba)
+{
+	struct uic_command *uic_cmd = &hba->active_uic_cmd;
+	int ret;
+
+	if (wait_for_completion_timeout(&uic_cmd->done,
+					msecs_to_jiffies(UIC_CMD_TIMEOUT)))
+		ret = ufshcd_get_uic_cmd_result(hba);
+	else
+		ret = -ETIMEDOUT;
+
+	return ret;
+}
+
+/**
+ * ufshcd_ready_uic_cmd - Check if controller is ready
+ *                        to accept UIC commands
+ * @hba: per adapter instance
+ * Return true on success, else false
+ */
+static inline bool ufshcd_ready_uic_cmd(struct ufs_hba *hba)
+{
+	if (ufshcd_readl(hba, REG_CONTROLLER_STATUS) & UIC_COMMAND_READY) {
+		return true;
+	} else {
+		dev_err(hba->dev,
+				"Controller not ready"
+				" to accept UIC commands\n");
+		return false;
+	}
+}
+
+/**
   * ufshcd_map_sg - Map scatter-gather list to prdt
   * @lrbp - pointer to local reference block
   *

@@ -735,15 +777,10 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
  {
  	struct uic_command *uic_cmd;
  	unsigned long flags;
+	int ret;
 
-	/* check if controller is ready to accept UIC commands */
-	if (((ufshcd_readl(hba, REG_CONTROLLER_STATUS)) &
-	    UIC_COMMAND_READY) == 0x0) {
-		dev_err(hba->dev,
-			"Controller not ready"
-			" to accept UIC commands\n");
+	if (!ufshcd_ready_uic_cmd(hba))
  		return -EIO;
-	}
 
  	spin_lock_irqsave(hba->host->host_lock, flags);

@@ -754,13 +791,16 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
  	uic_cmd->argument2 = 0;
  	uic_cmd->argument3 = 0;
 
-	/* enable UIC related interrupts */
-	ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
+	/* Dispatching UIC commands to controller */
+	ufshcd_dispatch_uic_cmd(hba, uic_cmd);
 
-	/* sending UIC commands to controller */
-	ufshcd_send_uic_command(hba, uic_cmd);
  	spin_unlock_irqrestore(hba->host->host_lock, flags);
-	return 0;
+
+	ret = ufshcd_wait_for_uic_cmd(hba);
+	if (ret)
+		dev_err(hba->dev, "link startup: error code %d returned\n", ret);
+
+	return ret;
  }
 
  /**

@@ -898,6 +938,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
  	if (ufshcd_hba_enable(hba))
  		return -EIO;
 
+	/* enable UIC related interrupts */
+	ufshcd_enable_intr(hba, UIC_COMMAND_COMPL | UIC_ERROR);
+
  	/* Configure UTRL and UTMRL base address registers */
  	ufshcd_writel(hba, REG_UTP_TRANSFER_REQ_LIST_BASE_L,
  		      lower_32_bits(hba->utrdl_dma_addr));

@@ -909,7 +952,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
  		      upper_32_bits(hba->utmrdl_dma_addr));
 
  	/* Initialize unipro link startup procedure */
-	return ufshcd_dme_link_startup(hba);
+	schedule_work(&hba->link_startup_wq);
+
+	return 0;
  }
 
  /**

@@ -1186,6 +1231,16 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
  }
 
  /**
+ * ufshcd_uic_cmd_compl - handle completion of uic command
+ * @hba: per adapter instance
+ */
+static void ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
+{
+	if (intr_status & UIC_COMMAND_COMPL)
+		complete(&hba->active_uic_cmd.done);
+}
+
+/**
   * ufshcd_transfer_req_compl - handle SCSI and query command completion
   * @hba: per adapter instance
   */

@@ -1225,25 +1280,26 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
  }
 
  /**
- * ufshcd_uic_cc_handler - handle UIC command completion
+ * ufshcd_link_startup - link initialization
   * @work: pointer to a work queue structure
- *
- * Returns 0 on success, non-zero value on failure
   */
-static void ufshcd_uic_cc_handler (struct work_struct *work)
+static void ufshcd_link_startup(struct work_struct *work)
  {
  	struct ufs_hba *hba;
+	int ret;
 
-	hba = container_of(work, struct ufs_hba, uic_workq);
+	hba = container_of(work, struct ufs_hba, link_startup_wq);
 
-	if ((hba->active_uic_cmd.command == UIC_CMD_DME_LINK_STARTUP) &&
-	    !(ufshcd_get_uic_cmd_result(hba))) {
+	ret = ufshcd_dme_link_startup(hba);
+	if (ret)
+		goto out;
 
-		if (ufshcd_make_hba_operational(hba))
-			dev_err(hba->dev,
-				"cc: hba not operational state\n");
-		return;
-	}
+	ret = ufshcd_make_hba_operational(hba);
+	if (ret)
+		goto out;
+	return;
+out:
+	dev_err(hba->dev, "link startup failed %d\n", ret);
  }
 
  /**

@@ -1307,7 +1363,7 @@ static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
  		ufshcd_err_handler(hba);
 
  	if (intr_status & UIC_COMMAND_COMPL)
-		schedule_work(&hba->uic_workq);
+		ufshcd_uic_cmd_compl(hba, intr_status);
 
  	if (intr_status & UTP_TASK_REQ_COMPL)
  		ufshcd_tmc_handler(hba);

@@ -1694,7 +1750,7 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
  	init_waitqueue_head(&hba->ufshcd_tm_wait_queue);
 
  	/* Initialize work queues */
-	INIT_WORK(&hba->uic_workq, ufshcd_uic_cc_handler);
+	INIT_WORK(&hba->link_startup_wq, ufshcd_link_startup);
  	INIT_WORK(&hba->feh_workq, ufshcd_fatal_err_handler);
 
  	/* IRQ registration */

drivers/scsi/ufs/ufshcd.h +4 -2

index 87d5a94..2fb4d94 100644
@@ -51,6 +51,7 @@
  #include <linux/bitops.h>
  #include <linux/pm_runtime.h>
  #include <linux/clk.h>
+#include <linux/completion.h>
 
  #include <asm/irq.h>
  #include <asm/byteorder.h>

@@ -83,6 +84,7 @@ struct uic_command {
  	u32 argument3;
  	int cmd_active;
  	int result;
+	struct completion done;
  };
 
  /**

@@ -140,7 +142,7 @@ struct ufshcd_lrb {
   * @tm_condition: condition variable for task management
   * @ufshcd_state: UFSHCD states
   * @intr_mask: Interrupt Mask Bits
- * @uic_workq: Work queue for UIC completion handling
+ * @link_startup_wq: Work queue for link start-up
   * @feh_workq: Work queue for fatal controller error handling
   * @errors: HBA errors
   */

@@ -179,7 +181,7 @@ struct ufs_hba {
  	u32 intr_mask;
 
  	/* Work Queues */
-	struct work_struct uic_workq;
+	struct work_struct link_startup_wq;
  	struct work_struct feh_workq;
 
  	/* HBA Errors */

Signature · 2 lines

Regards,
Sujit

Seungwon Jeon

author

2013-04-26 05:14 UTC #2 lore raw

Quoted 10 lines · Sujit Reddy Thumma

Quoted 5 lines · Seungwon Jeon
Link start-up requires long time with multiphase handshakes between UFS host and device. This affects driver's probe time. This patch let link start-up run asynchronously.
And completion time of uic command is defined to avoid a permanent wait.
I have similar patch posted few days back "scsi: ufs: Generalize UFS Interconnect Layer (UIC) command support" which does a bit more (mutex, error handling) than what is done here. Can that be used/improved?

I completed to check your patch to compare it now.
Though it's just my thought, the patch I sent is more intuitive on the whole. Considering other dme operations which I have introduced, it looks like matched. Of course, you may disagree.
But I think the part of mutex is needed. It's a good point.
In case of error handling, I didn't catch nothing special.
Rather, handling link lost case is not proper.
When ufs host meets link lost status, it should start with dme_reset not retried dme_linkstartup. And it would be good if link start-up procedure is done in separate process, not in driver probe. If it's all right with you, I'd like to update lock mechanism for uic command.
I can add your signed-off. Please let me know your opinion.

Thanks,
Seungwon Jeon

Quoted 266 lines

Quoted 250 lines

Signed-off-by Seungwon Jeon <[email protected]>

---
  drivers/scsi/ufs/ufshcd.c |  114 +++++++++++++++++++++++++++++++++-----------
  drivers/scsi/ufs/ufshcd.h |    6 ++-
  2 files changed, 89 insertions(+), 31 deletions(-)

drivers/scsi/ufs/ufshcd.c +85 ~~-29~~

index efe2256..76ff332 100644
@@ -38,6 +38,7 @@
  #define UFSHCD_ENABLE_INTRS	(UTP_TRANSFER_REQ_COMPL |\
  				 UTP_TASK_REQ_COMPL |\
  				 UFSHCD_ERROR_MASK)
+#define UIC_CMD_TIMEOUT	100
 
  enum {
  	UFSHCD_MAX_CHANNEL	= 0,

@@ -357,13 +358,15 @@ static inline void ufshcd_hba_capabilities(struct ufs_hba *hba)
  }
 
  /**
- * ufshcd_send_uic_command - Send UIC commands to unipro layers
+ * ufshcd_dispatch_uic_cmd - Dispatch UIC commands to unipro layers
   * @hba: per adapter instance
   * @uic_command: UIC command
   */
  static inline void
-ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
+ufshcd_dispatch_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmnd)
  {
+	init_completion(&uic_cmnd->done);
+
  	/* Write Args */
  	ufshcd_writel(hba, REG_UIC_COMMAND_ARG_1, uic_cmnd->argument1);
  	ufshcd_writel(hba, REG_UIC_COMMAND_ARG_2, uic_cmnd->argument2);

@@ -375,6 +378,45 @@ ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
  }
 
  /**
+ * ufshcd_wait_for_uic_cmd - Wait complectioin of UIC command
+ * @hba: per adapter instance
+ * @uic_command: UIC command
+ *
+ * Returns 0 only if success.
+ */
+static int ufshcd_wait_for_uic_cmd(struct ufs_hba *hba)
+{
+	struct uic_command *uic_cmd = &hba->active_uic_cmd;
+	int ret;
+
+	if (wait_for_completion_timeout(&uic_cmd->done,
+					msecs_to_jiffies(UIC_CMD_TIMEOUT)))
+		ret = ufshcd_get_uic_cmd_result(hba);
+	else
+		ret = -ETIMEDOUT;
+
+	return ret;
+}
+
+/**
+ * ufshcd_ready_uic_cmd - Check if controller is ready
+ *                        to accept UIC commands
+ * @hba: per adapter instance
+ * Return true on success, else false
+ */
+static inline bool ufshcd_ready_uic_cmd(struct ufs_hba *hba)
+{
+	if (ufshcd_readl(hba, REG_CONTROLLER_STATUS) & UIC_COMMAND_READY) {
+		return true;
+	} else {
+		dev_err(hba->dev,
+				"Controller not ready"
+				" to accept UIC commands\n");
+		return false;
+	}
+}
+
+/**
   * ufshcd_map_sg - Map scatter-gather list to prdt
   * @lrbp - pointer to local reference block
   *

@@ -735,15 +777,10 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
  {
  	struct uic_command *uic_cmd;
  	unsigned long flags;
+	int ret;
 
-	/* check if controller is ready to accept UIC commands */
-	if (((ufshcd_readl(hba, REG_CONTROLLER_STATUS)) &
-	    UIC_COMMAND_READY) == 0x0) {
-		dev_err(hba->dev,
-			"Controller not ready"
-			" to accept UIC commands\n");
+	if (!ufshcd_ready_uic_cmd(hba))
  		return -EIO;
-	}
 
  	spin_lock_irqsave(hba->host->host_lock, flags);

@@ -754,13 +791,16 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
  	uic_cmd->argument2 = 0;
  	uic_cmd->argument3 = 0;
 
-	/* enable UIC related interrupts */
-	ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
+	/* Dispatching UIC commands to controller */
+	ufshcd_dispatch_uic_cmd(hba, uic_cmd);
 
-	/* sending UIC commands to controller */
-	ufshcd_send_uic_command(hba, uic_cmd);
  	spin_unlock_irqrestore(hba->host->host_lock, flags);
-	return 0;
+
+	ret = ufshcd_wait_for_uic_cmd(hba);
+	if (ret)
+		dev_err(hba->dev, "link startup: error code %d returned\n", ret);
+
+	return ret;
  }
 
  /**

@@ -898,6 +938,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
  	if (ufshcd_hba_enable(hba))
  		return -EIO;
 
+	/* enable UIC related interrupts */
+	ufshcd_enable_intr(hba, UIC_COMMAND_COMPL | UIC_ERROR);
+
  	/* Configure UTRL and UTMRL base address registers */
  	ufshcd_writel(hba, REG_UTP_TRANSFER_REQ_LIST_BASE_L,
  		      lower_32_bits(hba->utrdl_dma_addr));

@@ -909,7 +952,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
  		      upper_32_bits(hba->utmrdl_dma_addr));
 
  	/* Initialize unipro link startup procedure */
-	return ufshcd_dme_link_startup(hba);
+	schedule_work(&hba->link_startup_wq);
+
+	return 0;
  }
 
  /**

@@ -1186,6 +1231,16 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
  }
 
  /**
+ * ufshcd_uic_cmd_compl - handle completion of uic command
+ * @hba: per adapter instance
+ */
+static void ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
+{
+	if (intr_status & UIC_COMMAND_COMPL)
+		complete(&hba->active_uic_cmd.done);
+}
+
+/**
   * ufshcd_transfer_req_compl - handle SCSI and query command completion
   * @hba: per adapter instance
   */

@@ -1225,25 +1280,26 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
  }
 
  /**
- * ufshcd_uic_cc_handler - handle UIC command completion
+ * ufshcd_link_startup - link initialization
   * @work: pointer to a work queue structure
- *
- * Returns 0 on success, non-zero value on failure
   */
-static void ufshcd_uic_cc_handler (struct work_struct *work)
+static void ufshcd_link_startup(struct work_struct *work)
  {
  	struct ufs_hba *hba;
+	int ret;
 
-	hba = container_of(work, struct ufs_hba, uic_workq);
+	hba = container_of(work, struct ufs_hba, link_startup_wq);
 
-	if ((hba->active_uic_cmd.command == UIC_CMD_DME_LINK_STARTUP) &&
-	    !(ufshcd_get_uic_cmd_result(hba))) {
+	ret = ufshcd_dme_link_startup(hba);
+	if (ret)
+		goto out;
 
-		if (ufshcd_make_hba_operational(hba))
-			dev_err(hba->dev,
-				"cc: hba not operational state\n");
-		return;
-	}
+	ret = ufshcd_make_hba_operational(hba);
+	if (ret)
+		goto out;
+	return;
+out:
+	dev_err(hba->dev, "link startup failed %d\n", ret);
  }
 
  /**

@@ -1307,7 +1363,7 @@ static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
  		ufshcd_err_handler(hba);
 
  	if (intr_status & UIC_COMMAND_COMPL)
-		schedule_work(&hba->uic_workq);
+		ufshcd_uic_cmd_compl(hba, intr_status);
 
  	if (intr_status & UTP_TASK_REQ_COMPL)
  		ufshcd_tmc_handler(hba);

@@ -1694,7 +1750,7 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
  	init_waitqueue_head(&hba->ufshcd_tm_wait_queue);
 
  	/* Initialize work queues */
-	INIT_WORK(&hba->uic_workq, ufshcd_uic_cc_handler);
+	INIT_WORK(&hba->link_startup_wq, ufshcd_link_startup);
  	INIT_WORK(&hba->feh_workq, ufshcd_fatal_err_handler);
 
  	/* IRQ registration */

drivers/scsi/ufs/ufshcd.h +3 -1

index 87d5a94..2fb4d94 100644
@@ -51,6 +51,7 @@
  #include <linux/bitops.h>
  #include <linux/pm_runtime.h>
  #include <linux/clk.h>
+#include <linux/completion.h>
 
  #include <asm/irq.h>
  #include <asm/byteorder.h>

@@ -83,6 +84,7 @@ struct uic_command {
  	u32 argument3;
  	int cmd_active;
  	int result;
+	struct completion done;
  };
 
  /**

@@ -140,7 +142,7 @@ struct ufshcd_lrb {
   * @tm_condition: condition variable for task management
   * @ufshcd_state: UFSHCD states
   * @intr_mask: Interrupt Mask Bits
- * @uic_workq: Work queue for UIC completion handling
+ * @link_startup_wq: Work queue for link start-up
   * @feh_workq: Work queue for fatal controller error handling
   * @errors: HBA errors
   */

drivers/scsi/ufs/ufshcd.h struct ufs_hba { · line 187

Quoted 8 lines

drivers/scsi/ufs/ufshcd.h +1 -1

@@ -179,7 +181,7 @@ struct ufs_hba {
  	u32 intr_mask;
 
  	/* Work Queues */
-	struct work_struct uic_workq;
+	struct work_struct link_startup_wq;
  	struct work_struct feh_workq;
 
  	/* HBA Errors */

--
Regards,
Sujit
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Sujit Reddy Thumma

2013-04-29 04:25 UTC #3 lore raw

Quoted 8 lines · Seungwon Jeon

Quoted 7 lines · Sujit Reddy Thumma
Quoted 5 lines · Seungwon Jeon
Link start-up requires long time with multiphase handshakes between UFS host and device. This affects driver's probe time. This patch let link start-up run asynchronously.
And completion time of uic command is defined to avoid a permanent wait.

Quoted 6 lines

Quoted 3 lines
I have similar patch posted few days back "scsi: ufs: Generalize UFS Interconnect Layer (UIC) command support" which does a bit more (mutex, error handling) than what is done here. Can that be used/improved?
I completed to check your patch to compare it now.
Though it's just my thought, the patch I sent is more intuitive on the whole. Considering other dme operations which I have introduced, it looks like matched.

There are lot of code duplications you might want to minimize building a DME command.

Quoted 5 lines

Of course, you may disagree.
But I think the part of mutex is needed. It's a good point.
In case of error handling, I didn't catch nothing special.
Rather, handling link lost case is not proper.
When ufs host meets link lost status, it should start with dme_reset not retried dme_linkstartup.

In section 7.2.1 (Host Controller Initialization) of JESD223A UFS HCI v1.1 specification I find this -

6. Sent DME_LINKSTARTUP command to start the link startup procedure
9. Check value of HCS.DP and make sure that there is a device attached to the Link. If presence of a device is detected, go to step 10; otherwise, resend the DME_LINKSTARTUP command after IS.ULLS has been set to 1 (Go to step 6). IS.ULLS equal 1 indicates that the UFS Device is ready for a link startup.

Going by the spec. just retrying with DME_LINKSTARTUP is correct.

In addition, it doesn't say what happens if IS.ULLS never sets to 1. Probably, the case which never happens.

Quoted 1 line

And it would be good if link start-up procedure is done in separate process, not in driver probe.

True.

Quoted 2 lines

If it's all right with you, I'd like to update lock mechanism for uic command. I can add your signed-off. Please let me know your opinion.

I would like to get a third opinion as both the patches needs modifications.

Some comments below:

Quoted 111 lines

Signed-off-by Seungwon Jeon <[email protected]>

---
   drivers/scsi/ufs/ufshcd.c |  114 +++++++++++++++++++++++++++++++++-----------
   drivers/scsi/ufs/ufshcd.h |    6 ++-
   2 files changed, 89 insertions(+), 31 deletions(-)

drivers/scsi/ufs/ufshcd.c +48 ~~-11~~

index efe2256..76ff332 100644
@@ -38,6 +38,7 @@
   #define UFSHCD_ENABLE_INTRS	(UTP_TRANSFER_REQ_COMPL |\
   				 UTP_TASK_REQ_COMPL |\
   				 UFSHCD_ERROR_MASK)
+#define UIC_CMD_TIMEOUT	100
 
   enum {
   	UFSHCD_MAX_CHANNEL	= 0,

@@ -357,13 +358,15 @@ static inline void ufshcd_hba_capabilities(struct ufs_hba *hba)
   }
 
   /**
- * ufshcd_send_uic_command - Send UIC commands to unipro layers
+ * ufshcd_dispatch_uic_cmd - Dispatch UIC commands to unipro layers
    * @hba: per adapter instance
    * @uic_command: UIC command
    */
   static inline void
-ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
+ufshcd_dispatch_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmnd)
   {
+	init_completion(&uic_cmnd->done);
+
   	/* Write Args */
   	ufshcd_writel(hba, REG_UIC_COMMAND_ARG_1, uic_cmnd->argument1);
   	ufshcd_writel(hba, REG_UIC_COMMAND_ARG_2, uic_cmnd->argument2);

@@ -375,6 +378,45 @@ ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
   }
 
   /**
+ * ufshcd_wait_for_uic_cmd - Wait complectioin of UIC command
+ * @hba: per adapter instance
+ * @uic_command: UIC command
+ *
+ * Returns 0 only if success.
+ */
+static int ufshcd_wait_for_uic_cmd(struct ufs_hba *hba)
+{
+	struct uic_command *uic_cmd = &hba->active_uic_cmd;
+	int ret;
+
+	if (wait_for_completion_timeout(&uic_cmd->done,
+					msecs_to_jiffies(UIC_CMD_TIMEOUT)))
+		ret = ufshcd_get_uic_cmd_result(hba);
+	else
+		ret = -ETIMEDOUT;
+
+	return ret;
+}
+
+/**
+ * ufshcd_ready_uic_cmd - Check if controller is ready
+ *                        to accept UIC commands
+ * @hba: per adapter instance
+ * Return true on success, else false
+ */
+static inline bool ufshcd_ready_uic_cmd(struct ufs_hba *hba)
+{
+	if (ufshcd_readl(hba, REG_CONTROLLER_STATUS) & UIC_COMMAND_READY) {
+		return true;
+	} else {
+		dev_err(hba->dev,
+				"Controller not ready"
+				" to accept UIC commands\n");
+		return false;
+	}
+}
+
+/**
    * ufshcd_map_sg - Map scatter-gather list to prdt
    * @lrbp - pointer to local reference block
    *

@@ -735,15 +777,10 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
   {
   	struct uic_command *uic_cmd;
   	unsigned long flags;
+	int ret;
 
-	/* check if controller is ready to accept UIC commands */
-	if (((ufshcd_readl(hba, REG_CONTROLLER_STATUS)) &
-	    UIC_COMMAND_READY) == 0x0) {
-		dev_err(hba->dev,
-			"Controller not ready"
-			" to accept UIC commands\n");
+	if (!ufshcd_ready_uic_cmd(hba))
   		return -EIO;
-	}
 
   	spin_lock_irqsave(hba->host->host_lock, flags);

@@ -754,13 +791,16 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
   	uic_cmd->argument2 = 0;
   	uic_cmd->argument3 = 0;
 
-	/* enable UIC related interrupts */
-	ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
+	/* Dispatching UIC commands to controller */
+	ufshcd_dispatch_uic_cmd(hba, uic_cmd);

Quoted 6 lines

Quoted 6 lines
drivers/scsi/ufs/ufshcd.c static int ufshcd_dme_link_startup(struct ufs_hba *hba) · line 799
Quoted 6 lines
drivers/scsi/ufs/ufshcd.c +2 -3
-	/* sending UIC commands to controller */
-	ufshcd_send_uic_command(hba, uic_cmd);
   	spin_unlock_irqrestore(hba->host->host_lock, flags);
-	return 0;
+
+	ret = ufshcd_wait_for_uic_cmd(hba);

Error code is incorrect. only -ETIMEDOUT is valid others are just DME errors.

Also, spec. clearly mentions a retry mechanism which means that there could be some timing issues anticipated where the UIC layer cannot respond properly.

Quoted 6 lines

drivers/scsi/ufs/ufshcd.c +4 -0

+	if (ret)
+		dev_err(hba->dev, "link startup: error code %d returned\n", ret);
+
+	return ret;
   }

Quoted 7 lines

Quoted 7 lines
drivers/scsi/ufs/ufshcd.c static int ufshcd_initialize_hba(struct ufs_hba *hba) · line 942
Quoted 7 lines
drivers/scsi/ufs/ufshcd.c +2 -0
   /**
@@ -898,6 +938,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
   	if (ufshcd_hba_enable(hba))
   		return -EIO;
 
+	/* enable UIC related interrupts */
+	ufshcd_enable_intr(hba, UIC_COMMAND_COMPL | UIC_ERROR);

The recovery when UIC_ERROR happens is broken because of re-entrancy to dme_link_startup from ufshcd_fatal_err_handler(). So better handle with timeout than allowing controller to raise a UIC_ERROR until that is fixed?

Quoted 18 lines

drivers/scsi/ufs/ufshcd.c +4 -1

+
   	/* Configure UTRL and UTMRL base address registers */
   	ufshcd_writel(hba, REG_UTP_TRANSFER_REQ_LIST_BASE_L,
   		      lower_32_bits(hba->utrdl_dma_addr));

@@ -909,7 +952,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
   		      upper_32_bits(hba->utmrdl_dma_addr));
 
   	/* Initialize unipro link startup procedure */
-	return ufshcd_dme_link_startup(hba);
+	schedule_work(&hba->link_startup_wq);
+
+	return 0;
   }
 
   /**

@@ -1186,6 +1231,16 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
   }

Quoted 7 lines

Quoted 7 lines
drivers/scsi/ufs/ufshcd.c ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) · line 1239
Quoted 7 lines
drivers/scsi/ufs/ufshcd.c +6 -0
   /**
+ * ufshcd_uic_cmd_compl - handle completion of uic command
+ * @hba: per adapter instance
+ */
+static void ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
+{
+	if (intr_status & UIC_COMMAND_COMPL)

why this redundant check if it is already checked in ufshcd_sl_intr()?

Quoted 53 lines

drivers/scsi/ufs/ufshcd.c +18 ~~-13~~

+		complete(&hba->active_uic_cmd.done);
+}
+
+/**
    * ufshcd_transfer_req_compl - handle SCSI and query command completion
    * @hba: per adapter instance
    */

@@ -1225,25 +1280,26 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
   }
 
   /**
- * ufshcd_uic_cc_handler - handle UIC command completion
+ * ufshcd_link_startup - link initialization
    * @work: pointer to a work queue structure
- *
- * Returns 0 on success, non-zero value on failure
    */
-static void ufshcd_uic_cc_handler (struct work_struct *work)
+static void ufshcd_link_startup(struct work_struct *work)
   {
   	struct ufs_hba *hba;
+	int ret;
 
-	hba = container_of(work, struct ufs_hba, uic_workq);
+	hba = container_of(work, struct ufs_hba, link_startup_wq);
 
-	if ((hba->active_uic_cmd.command == UIC_CMD_DME_LINK_STARTUP) &&
-	    !(ufshcd_get_uic_cmd_result(hba))) {
+	ret = ufshcd_dme_link_startup(hba);
+	if (ret)
+		goto out;
 
-		if (ufshcd_make_hba_operational(hba))
-			dev_err(hba->dev,
-				"cc: hba not operational state\n");
-		return;
-	}
+	ret = ufshcd_make_hba_operational(hba);
+	if (ret)
+		goto out;
+	return;
+out:
+	dev_err(hba->dev, "link startup failed %d\n", ret);
   }
 
   /**

@@ -1307,7 +1363,7 @@ static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
   		ufshcd_err_handler(hba);
 
   	if (intr_status & UIC_COMMAND_COMPL)
-		schedule_work(&hba->uic_workq);
+		ufshcd_uic_cmd_compl(hba, intr_status);

Quoted 8 lines

drivers/scsi/ufs/ufshcd.c int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle, · line 1753

Quoted 8 lines

drivers/scsi/ufs/ufshcd.c +1 -1

   	if (intr_status & UTP_TASK_REQ_COMPL)
   		ufshcd_tmc_handler(hba);

@@ -1694,7 +1750,7 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
   	init_waitqueue_head(&hba->ufshcd_tm_wait_queue);
 
   	/* Initialize work queues */
-	INIT_WORK(&hba->uic_workq, ufshcd_uic_cc_handler);
+	INIT_WORK(&hba->link_startup_wq, ufshcd_link_startup);

Can we use async function calls kernel/async.c instead of having work queues as this is only used during boot up?

Quoted 43 lines

Quoted 42 lines

drivers/scsi/ufs/ufshcd.c +0 -0

   	INIT_WORK(&hba->feh_workq, ufshcd_fatal_err_handler);
 
   	/* IRQ registration */

drivers/scsi/ufs/ufshcd.h +4 -2

index 87d5a94..2fb4d94 100644
@@ -51,6 +51,7 @@
   #include <linux/bitops.h>
   #include <linux/pm_runtime.h>
   #include <linux/clk.h>
+#include <linux/completion.h>
 
   #include <asm/irq.h>
   #include <asm/byteorder.h>

@@ -83,6 +84,7 @@ struct uic_command {
   	u32 argument3;
   	int cmd_active;
   	int result;
+	struct completion done;
   };
 
   /**

@@ -140,7 +142,7 @@ struct ufshcd_lrb {
    * @tm_condition: condition variable for task management
    * @ufshcd_state: UFSHCD states
    * @intr_mask: Interrupt Mask Bits
- * @uic_workq: Work queue for UIC completion handling
+ * @link_startup_wq: Work queue for link start-up
    * @feh_workq: Work queue for fatal controller error handling
    * @errors: HBA errors
    */

@@ -179,7 +181,7 @@ struct ufs_hba {
   	u32 intr_mask;
 
   	/* Work Queues */
-	struct work_struct uic_workq;
+	struct work_struct link_startup_wq;
   	struct work_struct feh_workq;
 
   	/* HBA Errors */

Signature · 2 lines

Regards,
Sujit

Seungwon Jeon

author

2013-04-29 10:24 UTC #4 lore raw

Quoted 28 lines · Sujit Reddy Thumma

Quoted 8 lines · Seungwon Jeon
Quoted 7 lines · Sujit Reddy Thumma
Quoted 5 lines · Seungwon Jeon
Link start-up requires long time with multiphase handshakes between UFS host and device. This affects driver's probe time. This patch let link start-up run asynchronously.
And completion time of uic command is defined to avoid a permanent wait.
Quoted 6 lines
Quoted 3 lines
I have similar patch posted few days back "scsi: ufs: Generalize UFS Interconnect Layer (UIC) command support" which does a bit more (mutex, error handling) than what is done here. Can that be used/improved?
I completed to check your patch to compare it now.
Though it's just my thought, the patch I sent is more intuitive on the whole. Considering other dme operations which I have introduced, it looks like matched.
There are lot of code duplications you might want to minimize building a DME command.
Quoted 5 lines
Of course, you may disagree.
But I think the part of mutex is needed. It's a good point.
In case of error handling, I didn't catch nothing special.
Rather, handling link lost case is not proper.
When ufs host meets link lost status, it should start with dme_reset not retried dme_linkstartup.
In section 7.2.1 (Host Controller Initialization) of JESD223A UFS HCI v1.1 specification I find this -

Quoted 8 lines

6. Sent DME_LINKSTARTUP command to start the link startup procedure
9. Check value of HCS.DP and make sure that there is a device attached to the Link. If presence of a device is detected, go to step 10; otherwise, resend the DME_LINKSTARTUP command after IS.ULLS has been set to 1 (Go to step 6). IS.ULLS equal 1 indicates that the UFS Device is ready for a link startup.
Going by the spec. just retrying with DME_LINKSTARTUP is correct.

Yes, as you quoted above, HCI standard mentions that.
Also, the following is mentioned.
UIC Link Lost Status (ULLS) corresponds to the UniPro DME_LINKLOST.ind
I just referred unipro specification.
When DME_LINKLOST.ind is generated, this affects the Link is put in the LinkLost state. Unipro spec says that DME User must apply a DME_RESET to redo the boot sequence. If there is misunderstood meaning and I have something to miss, we can discuss more. Please let me know.

Quoted 124 lines

In addition, it doesn't say what happens if IS.ULLS never sets to 1. Probably, the case which never happens.

Quoted 1 line

And it would be good if link start-up procedure is done in separate process, not in driver probe.

True.

Quoted 2 lines

If it's all right with you, I'd like to update lock mechanism for uic command. I can add your signed-off. Please let me know your opinion.

I would like to get a third opinion as both the patches needs modifications.

Some comments below:

Quoted 111 lines

Signed-off-by Seungwon Jeon <[email protected]>

---
   drivers/scsi/ufs/ufshcd.c |  114 +++++++++++++++++++++++++++++++++-----------
   drivers/scsi/ufs/ufshcd.h |    6 ++-
   2 files changed, 89 insertions(+), 31 deletions(-)

drivers/scsi/ufs/ufshcd.c +48 ~~-11~~

index efe2256..76ff332 100644
@@ -38,6 +38,7 @@
   #define UFSHCD_ENABLE_INTRS	(UTP_TRANSFER_REQ_COMPL |\
   				 UTP_TASK_REQ_COMPL |\
   				 UFSHCD_ERROR_MASK)
+#define UIC_CMD_TIMEOUT	100
 
   enum {
   	UFSHCD_MAX_CHANNEL	= 0,

@@ -357,13 +358,15 @@ static inline void ufshcd_hba_capabilities(struct ufs_hba *hba)
   }
 
   /**
- * ufshcd_send_uic_command - Send UIC commands to unipro layers
+ * ufshcd_dispatch_uic_cmd - Dispatch UIC commands to unipro layers
    * @hba: per adapter instance
    * @uic_command: UIC command
    */
   static inline void
-ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
+ufshcd_dispatch_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmnd)
   {
+	init_completion(&uic_cmnd->done);
+
   	/* Write Args */
   	ufshcd_writel(hba, REG_UIC_COMMAND_ARG_1, uic_cmnd->argument1);
   	ufshcd_writel(hba, REG_UIC_COMMAND_ARG_2, uic_cmnd->argument2);

@@ -375,6 +378,45 @@ ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
   }
 
   /**
+ * ufshcd_wait_for_uic_cmd - Wait complectioin of UIC command
+ * @hba: per adapter instance
+ * @uic_command: UIC command
+ *
+ * Returns 0 only if success.
+ */
+static int ufshcd_wait_for_uic_cmd(struct ufs_hba *hba)
+{
+	struct uic_command *uic_cmd = &hba->active_uic_cmd;
+	int ret;
+
+	if (wait_for_completion_timeout(&uic_cmd->done,
+					msecs_to_jiffies(UIC_CMD_TIMEOUT)))
+		ret = ufshcd_get_uic_cmd_result(hba);
+	else
+		ret = -ETIMEDOUT;
+
+	return ret;
+}
+
+/**
+ * ufshcd_ready_uic_cmd - Check if controller is ready
+ *                        to accept UIC commands
+ * @hba: per adapter instance
+ * Return true on success, else false
+ */
+static inline bool ufshcd_ready_uic_cmd(struct ufs_hba *hba)
+{
+	if (ufshcd_readl(hba, REG_CONTROLLER_STATUS) & UIC_COMMAND_READY) {
+		return true;
+	} else {
+		dev_err(hba->dev,
+				"Controller not ready"
+				" to accept UIC commands\n");
+		return false;
+	}
+}
+
+/**
    * ufshcd_map_sg - Map scatter-gather list to prdt
    * @lrbp - pointer to local reference block
    *

@@ -735,15 +777,10 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
   {
   	struct uic_command *uic_cmd;
   	unsigned long flags;
+	int ret;
 
-	/* check if controller is ready to accept UIC commands */
-	if (((ufshcd_readl(hba, REG_CONTROLLER_STATUS)) &
-	    UIC_COMMAND_READY) == 0x0) {
-		dev_err(hba->dev,
-			"Controller not ready"
-			" to accept UIC commands\n");
+	if (!ufshcd_ready_uic_cmd(hba))
   		return -EIO;
-	}
 
   	spin_lock_irqsave(hba->host->host_lock, flags);

@@ -754,13 +791,16 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
   	uic_cmd->argument2 = 0;
   	uic_cmd->argument3 = 0;
 
-	/* enable UIC related interrupts */
-	ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
+	/* Dispatching UIC commands to controller */
+	ufshcd_dispatch_uic_cmd(hba, uic_cmd);

Quoted 9 lines

Quoted 6 lines
Quoted 6 lines
drivers/scsi/ufs/ufshcd.c static int ufshcd_dme_link_startup(struct ufs_hba *hba) · line 799
Quoted 6 lines
drivers/scsi/ufs/ufshcd.c +2 -3
-	/* sending UIC commands to controller */
-	ufshcd_send_uic_command(hba, uic_cmd);
   	spin_unlock_irqrestore(hba->host->host_lock, flags);
-	return 0;
+
+	ret = ufshcd_wait_for_uic_cmd(hba);
Error code is incorrect. only -ETIMEDOUT is valid others are just DME errors.

Only success returns '0', other positive value from dme and -ETIMEDOUT mean failure. Error code can be reused purely, not being redefined.
I am seeing that -EINVAL represents from 01h to 07h in your handling. It looks like error's detail is disappear. Exact return might be needed from DME.

Quoted 4 lines

Also, spec. clearly mentions a retry mechanism which means that there could be some timing issues anticipated where the UIC layer cannot respond properly.

Sorry, I didn't catch your meaning fully. Where can I refer to it?

Quoted 13 lines

drivers/scsi/ufs/ufshcd.c +4 -0

+	if (ret)
+		dev_err(hba->dev, "link startup: error code %d returned\n", ret);
+
+	return ret;
   }
 
   /**

@@ -898,6 +938,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
   	if (ufshcd_hba_enable(hba))
   		return -EIO;

Quoted 6 lines

Quoted 2 lines
Quoted 2 lines
drivers/scsi/ufs/ufshcd.c static int ufshcd_initialize_hba(struct ufs_hba *hba) · line 942
Quoted 2 lines
drivers/scsi/ufs/ufshcd.c +2 -0
+	/* enable UIC related interrupts */
+	ufshcd_enable_intr(hba, UIC_COMMAND_COMPL | UIC_ERROR);
The recovery when UIC_ERROR happens is broken because of re-entrancy to dme_link_startup from ufshcd_fatal_err_handler(). So better handle with timeout than allowing controller to raise a UIC_ERROR until that is fixed?

I also recognize error handling should be done further. Ok, I agree with you.

Quoted 19 lines

drivers/scsi/ufs/ufshcd.c +4 -1

+
   	/* Configure UTRL and UTMRL base address registers */
   	ufshcd_writel(hba, REG_UTP_TRANSFER_REQ_LIST_BASE_L,
   		      lower_32_bits(hba->utrdl_dma_addr));

@@ -909,7 +952,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
   		      upper_32_bits(hba->utmrdl_dma_addr));
 
   	/* Initialize unipro link startup procedure */
-	return ufshcd_dme_link_startup(hba);
+	schedule_work(&hba->link_startup_wq);
+
+	return 0;
   }
 
   /**

@@ -1186,6 +1231,16 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
   }

Quoted 9 lines

Quoted 7 lines
Quoted 7 lines
drivers/scsi/ufs/ufshcd.c ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) · line 1239
Quoted 7 lines
drivers/scsi/ufs/ufshcd.c +6 -0
   /**
+ * ufshcd_uic_cmd_compl - handle completion of uic command
+ * @hba: per adapter instance
+ */
+static void ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
+{
+	if (intr_status & UIC_COMMAND_COMPL)
why this redundant check if it is already checked in ufshcd_sl_intr()?

Yes, it's currently not needed.
It will be used to identify several uic command. ([PATCH 5/5] scsi: ufs: add dme operations) Anyway, it's better to be removed here.

Quoted 59 lines

drivers/scsi/ufs/ufshcd.c +18 ~~-13~~

+		complete(&hba->active_uic_cmd.done);
+}
+
+/**
    * ufshcd_transfer_req_compl - handle SCSI and query command completion
    * @hba: per adapter instance
    */

@@ -1225,25 +1280,26 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
   }
 
   /**
- * ufshcd_uic_cc_handler - handle UIC command completion
+ * ufshcd_link_startup - link initialization
    * @work: pointer to a work queue structure
- *
- * Returns 0 on success, non-zero value on failure
    */
-static void ufshcd_uic_cc_handler (struct work_struct *work)
+static void ufshcd_link_startup(struct work_struct *work)
   {
   	struct ufs_hba *hba;
+	int ret;
 
-	hba = container_of(work, struct ufs_hba, uic_workq);
+	hba = container_of(work, struct ufs_hba, link_startup_wq);
 
-	if ((hba->active_uic_cmd.command == UIC_CMD_DME_LINK_STARTUP) &&
-	    !(ufshcd_get_uic_cmd_result(hba))) {
+	ret = ufshcd_dme_link_startup(hba);
+	if (ret)
+		goto out;
 
-		if (ufshcd_make_hba_operational(hba))
-			dev_err(hba->dev,
-				"cc: hba not operational state\n");
-		return;
-	}
+	ret = ufshcd_make_hba_operational(hba);
+	if (ret)
+		goto out;
+	return;
+out:
+	dev_err(hba->dev, "link startup failed %d\n", ret);
   }
 
   /**

@@ -1307,7 +1363,7 @@ static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
   		ufshcd_err_handler(hba);
 
   	if (intr_status & UIC_COMMAND_COMPL)
-		schedule_work(&hba->uic_workq);
+		ufshcd_uic_cmd_compl(hba, intr_status);
 
   	if (intr_status & UTP_TASK_REQ_COMPL)
   		ufshcd_tmc_handler(hba);

@@ -1694,7 +1750,7 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
   	init_waitqueue_head(&hba->ufshcd_tm_wait_queue);

Quoted 6 lines

Quoted 3 lines
Quoted 3 lines
drivers/scsi/ufs/ufshcd.c int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle, · line 1753
Quoted 3 lines
drivers/scsi/ufs/ufshcd.c +1 -1
   	/* Initialize work queues */
-	INIT_WORK(&hba->uic_workq, ufshcd_uic_cc_handler);
+	INIT_WORK(&hba->link_startup_wq, ufshcd_link_startup);
Can we use async function calls kernel/async.c instead of having work queues as this is only used during boot up?

As we know, both probe and resume are sensitive to execution time. I guess link startup procedure will also be activated in driver's resume. Do you have any specific reason for async function?

Thanks,
Seungwon Jeon

Quoted 53 lines

Quoted 36 lines

drivers/scsi/ufs/ufshcd.c +0 -0

   	INIT_WORK(&hba->feh_workq, ufshcd_fatal_err_handler);
 
   	/* IRQ registration */

drivers/scsi/ufs/ufshcd.h +3 -1

index 87d5a94..2fb4d94 100644
@@ -51,6 +51,7 @@
   #include <linux/bitops.h>
   #include <linux/pm_runtime.h>
   #include <linux/clk.h>
+#include <linux/completion.h>
 
   #include <asm/irq.h>
   #include <asm/byteorder.h>

@@ -83,6 +84,7 @@ struct uic_command {
   	u32 argument3;
   	int cmd_active;
   	int result;
+	struct completion done;
   };
 
   /**

@@ -140,7 +142,7 @@ struct ufshcd_lrb {
    * @tm_condition: condition variable for task management
    * @ufshcd_state: UFSHCD states
    * @intr_mask: Interrupt Mask Bits
- * @uic_workq: Work queue for UIC completion handling
+ * @link_startup_wq: Work queue for link start-up
    * @feh_workq: Work queue for fatal controller error handling
    * @errors: HBA errors
    */

@@ -179,7 +181,7 @@ struct ufs_hba {
   	u32 intr_mask;

Quoted 8 lines

Quoted 8 lines
drivers/scsi/ufs/ufshcd.h struct ufs_hba { · line 187
Quoted 7 lines
drivers/scsi/ufs/ufshcd.h +1 -1
   	/* Work Queues */
-	struct work_struct uic_workq;
+	struct work_struct link_startup_wq;
   	struct work_struct feh_workq;
 
   	/* HBA Errors */

Sujit Reddy Thumma

2013-04-29 13:05 UTC #5 lore raw

Quoted 37 lines · Seungwon Jeon

Quoted 28 lines · Sujit Reddy Thumma
Quoted 8 lines · Seungwon Jeon
Quoted 7 lines · Sujit Reddy Thumma
Quoted 5 lines · Seungwon Jeon
Link start-up requires long time with multiphase handshakes between UFS host and device. This affects driver's probe time. This patch let link start-up run asynchronously.
And completion time of uic command is defined to avoid a permanent wait.
Quoted 6 lines
Quoted 3 lines
I have similar patch posted few days back "scsi: ufs: Generalize UFS Interconnect Layer (UIC) command support" which does a bit more (mutex, error handling) than what is done here. Can that be used/improved?
I completed to check your patch to compare it now.
Though it's just my thought, the patch I sent is more intuitive on the whole. Considering other dme operations which I have introduced, it looks like matched.
There are lot of code duplications you might want to minimize building a DME command.
Quoted 5 lines
Of course, you may disagree.
But I think the part of mutex is needed. It's a good point.
In case of error handling, I didn't catch nothing special.
Rather, handling link lost case is not proper.
When ufs host meets link lost status, it should start with dme_reset not retried dme_linkstartup.
In section 7.2.1 (Host Controller Initialization) of JESD223A UFS HCI v1.1 specification I find this -
Quoted 8 lines
6. Sent DME_LINKSTARTUP command to start the link startup procedure
9. Check value of HCS.DP and make sure that there is a device attached to the Link. If presence of a device is detected, go to step 10; otherwise, resend the DME_LINKSTARTUP command after IS.ULLS has been set to 1 (Go to step 6). IS.ULLS equal 1 indicates that the UFS Device is ready for a link startup.
Going by the spec. just retrying with DME_LINKSTARTUP is correct.

Quoted 8 lines

Yes, as you quoted above, HCI standard mentions that.
Also, the following is mentioned.
UIC Link Lost Status (ULLS) corresponds to the UniPro DME_LINKLOST.ind
I just referred unipro specification.
When DME_LINKLOST.ind is generated, this affects the Link is put in the LinkLost state. Unipro spec says that DME User must apply a DME_RESET to redo the boot sequence. If there is misunderstood meaning and I have something to miss, we can discuss more. Please let me know.

Yes, it looks like the two specs. are conflicting each other. I guess we need to take this to Jedec for clarification. Meanwhile, to be on safe side can we add a retry mechanism that does ufshcd_hba_enable() before sending DME_LINKSTARTUP again? This way we can be sure that the DME_RESET and DME_ENABLE is taken care by the host reset itself.

Quoted 134 lines

Quoted 125 lines

In addition, it doesn't say what happens if IS.ULLS never sets to 1. Probably, the case which never happens.

Quoted 1 line

And it would be good if link start-up procedure is done in separate process, not in driver probe.

True.

Quoted 2 lines

If it's all right with you, I'd like to update lock mechanism for uic command. I can add your signed-off. Please let me know your opinion.

I would like to get a third opinion as both the patches needs modifications.

Some comments below:

Quoted 111 lines

Signed-off-by Seungwon Jeon <[email protected]>

---
    drivers/scsi/ufs/ufshcd.c |  114 +++++++++++++++++++++++++++++++++-----------
    drivers/scsi/ufs/ufshcd.h |    6 ++-
    2 files changed, 89 insertions(+), 31 deletions(-)

drivers/scsi/ufs/ufshcd.c +48 ~~-11~~

index efe2256..76ff332 100644
@@ -38,6 +38,7 @@
    #define UFSHCD_ENABLE_INTRS	(UTP_TRANSFER_REQ_COMPL |\
    				 UTP_TASK_REQ_COMPL |\
    				 UFSHCD_ERROR_MASK)
+#define UIC_CMD_TIMEOUT	100
 
    enum {
    	UFSHCD_MAX_CHANNEL	= 0,

@@ -357,13 +358,15 @@ static inline void ufshcd_hba_capabilities(struct ufs_hba *hba)
    }
 
    /**
- * ufshcd_send_uic_command - Send UIC commands to unipro layers
+ * ufshcd_dispatch_uic_cmd - Dispatch UIC commands to unipro layers
     * @hba: per adapter instance
     * @uic_command: UIC command
     */
    static inline void
-ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
+ufshcd_dispatch_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmnd)
    {
+	init_completion(&uic_cmnd->done);
+
    	/* Write Args */
    	ufshcd_writel(hba, REG_UIC_COMMAND_ARG_1, uic_cmnd->argument1);
    	ufshcd_writel(hba, REG_UIC_COMMAND_ARG_2, uic_cmnd->argument2);

@@ -375,6 +378,45 @@ ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
    }
 
    /**
+ * ufshcd_wait_for_uic_cmd - Wait complectioin of UIC command
+ * @hba: per adapter instance
+ * @uic_command: UIC command
+ *
+ * Returns 0 only if success.
+ */
+static int ufshcd_wait_for_uic_cmd(struct ufs_hba *hba)
+{
+	struct uic_command *uic_cmd = &hba->active_uic_cmd;
+	int ret;
+
+	if (wait_for_completion_timeout(&uic_cmd->done,
+					msecs_to_jiffies(UIC_CMD_TIMEOUT)))
+		ret = ufshcd_get_uic_cmd_result(hba);
+	else
+		ret = -ETIMEDOUT;
+
+	return ret;
+}
+
+/**
+ * ufshcd_ready_uic_cmd - Check if controller is ready
+ *                        to accept UIC commands
+ * @hba: per adapter instance
+ * Return true on success, else false
+ */
+static inline bool ufshcd_ready_uic_cmd(struct ufs_hba *hba)
+{
+	if (ufshcd_readl(hba, REG_CONTROLLER_STATUS) & UIC_COMMAND_READY) {
+		return true;
+	} else {
+		dev_err(hba->dev,
+				"Controller not ready"
+				" to accept UIC commands\n");
+		return false;
+	}
+}
+
+/**
     * ufshcd_map_sg - Map scatter-gather list to prdt
     * @lrbp - pointer to local reference block
     *

@@ -735,15 +777,10 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
    {
    	struct uic_command *uic_cmd;
    	unsigned long flags;
+	int ret;
 
-	/* check if controller is ready to accept UIC commands */
-	if (((ufshcd_readl(hba, REG_CONTROLLER_STATUS)) &
-	    UIC_COMMAND_READY) == 0x0) {
-		dev_err(hba->dev,
-			"Controller not ready"
-			" to accept UIC commands\n");
+	if (!ufshcd_ready_uic_cmd(hba))
    		return -EIO;
-	}
 
    	spin_lock_irqsave(hba->host->host_lock, flags);

@@ -754,13 +791,16 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
    	uic_cmd->argument2 = 0;
    	uic_cmd->argument3 = 0;
 
-	/* enable UIC related interrupts */
-	ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
+	/* Dispatching UIC commands to controller */
+	ufshcd_dispatch_uic_cmd(hba, uic_cmd);

Quoted 9 lines

Quoted 6 lines
Quoted 6 lines
drivers/scsi/ufs/ufshcd.c static int ufshcd_dme_link_startup(struct ufs_hba *hba) · line 799
Quoted 6 lines
drivers/scsi/ufs/ufshcd.c +2 -3
-	/* sending UIC commands to controller */
-	ufshcd_send_uic_command(hba, uic_cmd);
    	spin_unlock_irqrestore(hba->host->host_lock, flags);
-	return 0;
+
+	ret = ufshcd_wait_for_uic_cmd(hba);
Error code is incorrect. only -ETIMEDOUT is valid others are just DME errors.

Quoted 4 lines

Only success returns '0', other positive value from dme and -ETIMEDOUT mean failure. Error code can be reused purely, not being redefined.
I am seeing that -EINVAL represents from 01h to 07h in your handling. It looks like error's detail is disappear. Exact return might be needed from DME.

okay.

Quoted 6 lines

Quoted 5 lines
Also, spec. clearly mentions a retry mechanism which means that there could be some timing issues anticipated where the UIC layer cannot respond properly.
Sorry, I didn't catch your meaning fully. Where can I refer to it?

I meant the same retry mechanism mentioned in the section 7.2.1 (Host Controller Initialization) of JESD223A UFS HCI v1.1.

Quoted 120 lines

Quoted 14 lines

drivers/scsi/ufs/ufshcd.c +4 -0

+	if (ret)
+		dev_err(hba->dev, "link startup: error code %d returned\n", ret);
+
+	return ret;
    }
 
    /**

@@ -898,6 +938,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
    	if (ufshcd_hba_enable(hba))
    		return -EIO;

Quoted 6 lines

Quoted 2 lines
Quoted 2 lines
drivers/scsi/ufs/ufshcd.c static int ufshcd_initialize_hba(struct ufs_hba *hba) · line 942
Quoted 2 lines
drivers/scsi/ufs/ufshcd.c +2 -0
+	/* enable UIC related interrupts */
+	ufshcd_enable_intr(hba, UIC_COMMAND_COMPL | UIC_ERROR);
The recovery when UIC_ERROR happens is broken because of re-entrancy to dme_link_startup from ufshcd_fatal_err_handler(). So better handle with timeout than allowing controller to raise a UIC_ERROR until that is fixed?

I also recognize error handling should be done further. Ok, I agree with you.

Quoted 19 lines

drivers/scsi/ufs/ufshcd.c +4 -1

+
    	/* Configure UTRL and UTMRL base address registers */
    	ufshcd_writel(hba, REG_UTP_TRANSFER_REQ_LIST_BASE_L,
    		      lower_32_bits(hba->utrdl_dma_addr));

@@ -909,7 +952,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
    		      upper_32_bits(hba->utmrdl_dma_addr));
 
    	/* Initialize unipro link startup procedure */
-	return ufshcd_dme_link_startup(hba);
+	schedule_work(&hba->link_startup_wq);
+
+	return 0;
    }
 
    /**

@@ -1186,6 +1231,16 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
    }

Quoted 9 lines

Quoted 7 lines
Quoted 7 lines
drivers/scsi/ufs/ufshcd.c ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) · line 1239
Quoted 7 lines
drivers/scsi/ufs/ufshcd.c +6 -0
    /**
+ * ufshcd_uic_cmd_compl - handle completion of uic command
+ * @hba: per adapter instance
+ */
+static void ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
+{
+	if (intr_status & UIC_COMMAND_COMPL)
why this redundant check if it is already checked in ufshcd_sl_intr()?

Yes, it's currently not needed.
It will be used to identify several uic command. ([PATCH 5/5] scsi: ufs: add dme operations) Anyway, it's better to be removed here.

Quoted 59 lines

drivers/scsi/ufs/ufshcd.c +18 ~~-13~~

+		complete(&hba->active_uic_cmd.done);
+}
+
+/**
     * ufshcd_transfer_req_compl - handle SCSI and query command completion
     * @hba: per adapter instance
     */

@@ -1225,25 +1280,26 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
    }
 
    /**
- * ufshcd_uic_cc_handler - handle UIC command completion
+ * ufshcd_link_startup - link initialization
     * @work: pointer to a work queue structure
- *
- * Returns 0 on success, non-zero value on failure
     */
-static void ufshcd_uic_cc_handler (struct work_struct *work)
+static void ufshcd_link_startup(struct work_struct *work)
    {
    	struct ufs_hba *hba;
+	int ret;
 
-	hba = container_of(work, struct ufs_hba, uic_workq);
+	hba = container_of(work, struct ufs_hba, link_startup_wq);
 
-	if ((hba->active_uic_cmd.command == UIC_CMD_DME_LINK_STARTUP) &&
-	    !(ufshcd_get_uic_cmd_result(hba))) {
+	ret = ufshcd_dme_link_startup(hba);
+	if (ret)
+		goto out;
 
-		if (ufshcd_make_hba_operational(hba))
-			dev_err(hba->dev,
-				"cc: hba not operational state\n");
-		return;
-	}
+	ret = ufshcd_make_hba_operational(hba);
+	if (ret)
+		goto out;
+	return;
+out:
+	dev_err(hba->dev, "link startup failed %d\n", ret);
    }
 
    /**

@@ -1307,7 +1363,7 @@ static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
    		ufshcd_err_handler(hba);
 
    	if (intr_status & UIC_COMMAND_COMPL)
-		schedule_work(&hba->uic_workq);
+		ufshcd_uic_cmd_compl(hba, intr_status);
 
    	if (intr_status & UTP_TASK_REQ_COMPL)
    		ufshcd_tmc_handler(hba);

@@ -1694,7 +1750,7 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
    	init_waitqueue_head(&hba->ufshcd_tm_wait_queue);

Quoted 6 lines

Quoted 3 lines
Quoted 3 lines
drivers/scsi/ufs/ufshcd.c int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle, · line 1753
Quoted 3 lines
drivers/scsi/ufs/ufshcd.c +1 -1
    	/* Initialize work queues */
-	INIT_WORK(&hba->uic_workq, ufshcd_uic_cc_handler);
+	INIT_WORK(&hba->link_startup_wq, ufshcd_link_startup);
Can we use async function calls kernel/async.c instead of having work queues as this is only used during boot up?

Quoted 4 lines

As we know, both probe and resume are sensitive to execution time. I guess link startup procedure will also be activated in driver's resume. Do you have any specific reason for async function?

I guess most UFS devices currently are embedded and have rootfs on them. If we use schedule_work there is no synchronization mechanism to check whether the device link startup is completed and device is available for the userspace to mount the partitions. Having async mechanism in place, the prepare_namespace() does wait for such async probes to be completed before mounting the rootfs.

I agree that the resume is sensitive to execute time but during resume you can't schedule link startup work because the fs/block/scsi layer expects the device availability as soon as resume operation is completed. So ufshcd_link_startup() should be called in the resume context itself or implement a synchronization mechanism like blocking scsi layer queuing requests until link startup is completed.

Would following implementation looks better?

ufshcd_async_probe()
{
...
ufshcd_link_startup(hba);
...
}

ufshcd_resume()
{
...
ufshcd_link_startup(hba);
...
}

ufshcd_init()
{
...
async_schedule(ufshcd_async_probe, hba);
...
}

Signature · 2 lines

Regards,
Sujit

Seungwon Jeon

author

2013-04-30 06:33 UTC #6 lore raw

Quoted 47 lines · Sujit Reddy Thumma

Quoted 37 lines · Seungwon Jeon
Quoted 28 lines · Sujit Reddy Thumma
Quoted 8 lines · Seungwon Jeon
Quoted 7 lines · Sujit Reddy Thumma
Quoted 5 lines · Seungwon Jeon
Link start-up requires long time with multiphase handshakes between UFS host and device. This affects driver's probe time. This patch let link start-up run asynchronously.
And completion time of uic command is defined to avoid a permanent wait.
Quoted 6 lines
Quoted 3 lines
I have similar patch posted few days back "scsi: ufs: Generalize UFS Interconnect Layer (UIC) command support" which does a bit more (mutex, error handling) than what is done here. Can that be used/improved?
I completed to check your patch to compare it now.
Though it's just my thought, the patch I sent is more intuitive on the whole. Considering other dme operations which I have introduced, it looks like matched.
There are lot of code duplications you might want to minimize building a DME command.
Quoted 5 lines
Of course, you may disagree.
But I think the part of mutex is needed. It's a good point.
In case of error handling, I didn't catch nothing special.
Rather, handling link lost case is not proper.
When ufs host meets link lost status, it should start with dme_reset not retried dme_linkstartup.
In section 7.2.1 (Host Controller Initialization) of JESD223A UFS HCI v1.1 specification I find this -
Quoted 8 lines
6. Sent DME_LINKSTARTUP command to start the link startup procedure
9. Check value of HCS.DP and make sure that there is a device attached to the Link. If presence of a device is detected, go to step 10; otherwise, resend the DME_LINKSTARTUP command after IS.ULLS has been set to 1 (Go to step 6). IS.ULLS equal 1 indicates that the UFS Device is ready for a link startup.
Going by the spec. just retrying with DME_LINKSTARTUP is correct.
Quoted 8 lines
Yes, as you quoted above, HCI standard mentions that.
Also, the following is mentioned.
UIC Link Lost Status (ULLS) corresponds to the UniPro DME_LINKLOST.ind
I just referred unipro specification.
When DME_LINKLOST.ind is generated, this affects the Link is put in the LinkLost state. Unipro spec says that DME User must apply a DME_RESET to redo the boot sequence. If there is misunderstood meaning and I have something to miss, we can discuss more. Please let me know.

Quoted 5 lines

Yes, it looks like the two specs. are conflicting each other. I guess we need to take this to Jedec for clarification. Meanwhile, to be on safe side can we add a retry mechanism that does ufshcd_hba_enable() before sending DME_LINKSTARTUP again? This way we can be sure that the DME_RESET and DME_ENABLE is taken care by the host reset itself.

Yes, If the latter case is applied, 'ufshcd_hba_enable' will be start entry for retry. Further, IS.ULLS could be handled through the interrupt instead of polling for retry mechanism?

Quoted 276 lines

Quoted 135 lines

Quoted 126 lines

In addition, it doesn't say what happens if IS.ULLS never sets to 1. Probably, the case which never happens.

Quoted 1 line

And it would be good if link start-up procedure is done in separate process, not in driver probe.

True.

Quoted 2 lines

If it's all right with you, I'd like to update lock mechanism for uic command. I can add your signed-off. Please let me know your opinion.

I would like to get a third opinion as both the patches needs modifications.

Some comments below:

Quoted 111 lines

Signed-off-by Seungwon Jeon <[email protected]>

---
    drivers/scsi/ufs/ufshcd.c |  114 +++++++++++++++++++++++++++++++++-----------
    drivers/scsi/ufs/ufshcd.h |    6 ++-
    2 files changed, 89 insertions(+), 31 deletions(-)

drivers/scsi/ufs/ufshcd.c +48 ~~-11~~

index efe2256..76ff332 100644
@@ -38,6 +38,7 @@
    #define UFSHCD_ENABLE_INTRS	(UTP_TRANSFER_REQ_COMPL |\
    				 UTP_TASK_REQ_COMPL |\
    				 UFSHCD_ERROR_MASK)
+#define UIC_CMD_TIMEOUT	100
 
    enum {
    	UFSHCD_MAX_CHANNEL	= 0,

@@ -357,13 +358,15 @@ static inline void ufshcd_hba_capabilities(struct ufs_hba *hba)
    }
 
    /**
- * ufshcd_send_uic_command - Send UIC commands to unipro layers
+ * ufshcd_dispatch_uic_cmd - Dispatch UIC commands to unipro layers
     * @hba: per adapter instance
     * @uic_command: UIC command
     */
    static inline void
-ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
+ufshcd_dispatch_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmnd)
    {
+	init_completion(&uic_cmnd->done);
+
    	/* Write Args */
    	ufshcd_writel(hba, REG_UIC_COMMAND_ARG_1, uic_cmnd->argument1);
    	ufshcd_writel(hba, REG_UIC_COMMAND_ARG_2, uic_cmnd->argument2);

@@ -375,6 +378,45 @@ ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
    }
 
    /**
+ * ufshcd_wait_for_uic_cmd - Wait complectioin of UIC command
+ * @hba: per adapter instance
+ * @uic_command: UIC command
+ *
+ * Returns 0 only if success.
+ */
+static int ufshcd_wait_for_uic_cmd(struct ufs_hba *hba)
+{
+	struct uic_command *uic_cmd = &hba->active_uic_cmd;
+	int ret;
+
+	if (wait_for_completion_timeout(&uic_cmd->done,
+					msecs_to_jiffies(UIC_CMD_TIMEOUT)))
+		ret = ufshcd_get_uic_cmd_result(hba);
+	else
+		ret = -ETIMEDOUT;
+
+	return ret;
+}
+
+/**
+ * ufshcd_ready_uic_cmd - Check if controller is ready
+ *                        to accept UIC commands
+ * @hba: per adapter instance
+ * Return true on success, else false
+ */
+static inline bool ufshcd_ready_uic_cmd(struct ufs_hba *hba)
+{
+	if (ufshcd_readl(hba, REG_CONTROLLER_STATUS) & UIC_COMMAND_READY) {
+		return true;
+	} else {
+		dev_err(hba->dev,
+				"Controller not ready"
+				" to accept UIC commands\n");
+		return false;
+	}
+}
+
+/**
     * ufshcd_map_sg - Map scatter-gather list to prdt
     * @lrbp - pointer to local reference block
     *

@@ -735,15 +777,10 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
    {
    	struct uic_command *uic_cmd;
    	unsigned long flags;
+	int ret;
 
-	/* check if controller is ready to accept UIC commands */
-	if (((ufshcd_readl(hba, REG_CONTROLLER_STATUS)) &
-	    UIC_COMMAND_READY) == 0x0) {
-		dev_err(hba->dev,
-			"Controller not ready"
-			" to accept UIC commands\n");
+	if (!ufshcd_ready_uic_cmd(hba))
    		return -EIO;
-	}
 
    	spin_lock_irqsave(hba->host->host_lock, flags);

@@ -754,13 +791,16 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
    	uic_cmd->argument2 = 0;
    	uic_cmd->argument3 = 0;
 
-	/* enable UIC related interrupts */
-	ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
+	/* Dispatching UIC commands to controller */
+	ufshcd_dispatch_uic_cmd(hba, uic_cmd);

Quoted 9 lines

Quoted 6 lines
Quoted 6 lines
drivers/scsi/ufs/ufshcd.c static int ufshcd_dme_link_startup(struct ufs_hba *hba) · line 799
Quoted 6 lines
drivers/scsi/ufs/ufshcd.c +2 -3
-	/* sending UIC commands to controller */
-	ufshcd_send_uic_command(hba, uic_cmd);
    	spin_unlock_irqrestore(hba->host->host_lock, flags);
-	return 0;
+
+	ret = ufshcd_wait_for_uic_cmd(hba);
Error code is incorrect. only -ETIMEDOUT is valid others are just DME errors.

Quoted 4 lines

Only success returns '0', other positive value from dme and -ETIMEDOUT mean failure. Error code can be reused purely, not being redefined.
I am seeing that -EINVAL represents from 01h to 07h in your handling. It looks like error's detail is disappear. Exact return might be needed from DME.

okay.

Quoted 6 lines

Quoted 5 lines
Also, spec. clearly mentions a retry mechanism which means that there could be some timing issues anticipated where the UIC layer cannot respond properly.
Sorry, I didn't catch your meaning fully. Where can I refer to it?

I meant the same retry mechanism mentioned in the section 7.2.1 (Host Controller Initialization) of JESD223A UFS HCI v1.1.

Quoted 120 lines

Quoted 14 lines

drivers/scsi/ufs/ufshcd.c +4 -0

+	if (ret)
+		dev_err(hba->dev, "link startup: error code %d returned\n", ret);
+
+	return ret;
    }
 
    /**

@@ -898,6 +938,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
    	if (ufshcd_hba_enable(hba))
    		return -EIO;

Quoted 6 lines

Quoted 2 lines
Quoted 2 lines
drivers/scsi/ufs/ufshcd.c static int ufshcd_initialize_hba(struct ufs_hba *hba) · line 942
Quoted 2 lines
drivers/scsi/ufs/ufshcd.c +2 -0
+	/* enable UIC related interrupts */
+	ufshcd_enable_intr(hba, UIC_COMMAND_COMPL | UIC_ERROR);
The recovery when UIC_ERROR happens is broken because of re-entrancy to dme_link_startup from ufshcd_fatal_err_handler(). So better handle with timeout than allowing controller to raise a UIC_ERROR until that is fixed?

I also recognize error handling should be done further. Ok, I agree with you.

Quoted 19 lines

drivers/scsi/ufs/ufshcd.c +4 -1

+
    	/* Configure UTRL and UTMRL base address registers */
    	ufshcd_writel(hba, REG_UTP_TRANSFER_REQ_LIST_BASE_L,
    		      lower_32_bits(hba->utrdl_dma_addr));

@@ -909,7 +952,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
    		      upper_32_bits(hba->utmrdl_dma_addr));
 
    	/* Initialize unipro link startup procedure */
-	return ufshcd_dme_link_startup(hba);
+	schedule_work(&hba->link_startup_wq);
+
+	return 0;
    }
 
    /**

@@ -1186,6 +1231,16 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
    }

Quoted 9 lines

Quoted 7 lines
Quoted 7 lines
drivers/scsi/ufs/ufshcd.c ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) · line 1239
Quoted 7 lines
drivers/scsi/ufs/ufshcd.c +6 -0
    /**
+ * ufshcd_uic_cmd_compl - handle completion of uic command
+ * @hba: per adapter instance
+ */
+static void ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
+{
+	if (intr_status & UIC_COMMAND_COMPL)
why this redundant check if it is already checked in ufshcd_sl_intr()?

Yes, it's currently not needed.
It will be used to identify several uic command. ([PATCH 5/5] scsi: ufs: add dme operations) Anyway, it's better to be removed here.

Quoted 59 lines

drivers/scsi/ufs/ufshcd.c +18 ~~-13~~

+		complete(&hba->active_uic_cmd.done);
+}
+
+/**
     * ufshcd_transfer_req_compl - handle SCSI and query command completion
     * @hba: per adapter instance
     */

@@ -1225,25 +1280,26 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
    }
 
    /**
- * ufshcd_uic_cc_handler - handle UIC command completion
+ * ufshcd_link_startup - link initialization
     * @work: pointer to a work queue structure
- *
- * Returns 0 on success, non-zero value on failure
     */
-static void ufshcd_uic_cc_handler (struct work_struct *work)
+static void ufshcd_link_startup(struct work_struct *work)
    {
    	struct ufs_hba *hba;
+	int ret;
 
-	hba = container_of(work, struct ufs_hba, uic_workq);
+	hba = container_of(work, struct ufs_hba, link_startup_wq);
 
-	if ((hba->active_uic_cmd.command == UIC_CMD_DME_LINK_STARTUP) &&
-	    !(ufshcd_get_uic_cmd_result(hba))) {
+	ret = ufshcd_dme_link_startup(hba);
+	if (ret)
+		goto out;
 
-		if (ufshcd_make_hba_operational(hba))
-			dev_err(hba->dev,
-				"cc: hba not operational state\n");
-		return;
-	}
+	ret = ufshcd_make_hba_operational(hba);
+	if (ret)
+		goto out;
+	return;
+out:
+	dev_err(hba->dev, "link startup failed %d\n", ret);
    }
 
    /**

@@ -1307,7 +1363,7 @@ static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
    		ufshcd_err_handler(hba);
 
    	if (intr_status & UIC_COMMAND_COMPL)
-		schedule_work(&hba->uic_workq);
+		ufshcd_uic_cmd_compl(hba, intr_status);
 
    	if (intr_status & UTP_TASK_REQ_COMPL)
    		ufshcd_tmc_handler(hba);

@@ -1694,7 +1750,7 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
    	init_waitqueue_head(&hba->ufshcd_tm_wait_queue);

Quoted 6 lines

Quoted 3 lines
Quoted 3 lines
drivers/scsi/ufs/ufshcd.c int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle, · line 1753
Quoted 3 lines
drivers/scsi/ufs/ufshcd.c +1 -1
    	/* Initialize work queues */
-	INIT_WORK(&hba->uic_workq, ufshcd_uic_cc_handler);
+	INIT_WORK(&hba->link_startup_wq, ufshcd_link_startup);
Can we use async function calls kernel/async.c instead of having work queues as this is only used during boot up?

Quoted 4 lines

As we know, both probe and resume are sensitive to execution time. I guess link startup procedure will also be activated in driver's resume. Do you have any specific reason for async function?

Quoted 6 lines

I guess most UFS devices currently are embedded and have rootfs on them. If we use schedule_work there is no synchronization mechanism to check whether the device link startup is completed and device is available for the userspace to mount the partitions. Having async mechanism in place, the prepare_namespace() does wait for such async probes to be completed before mounting the rootfs.

I understand your meaning.
If you are considering that, I think 'scsi_scan_host' has a role for that. 'scsi_scan_host' will be the conclusion and be done in async subsystem. 'scsi_scan_host' is actually called, after finishing link startup procedure.

Quoted 7 lines

I agree that the resume is sensitive to execute time but during resume you can't schedule link startup work because the fs/block/scsi layer expects the device availability as soon as resume operation is completed. So ufshcd_link_startup() should be called in the resume context itself or implement a synchronization mechanism like blocking scsi layer queuing requests until link startup is completed.

'scsi_block_requests' and 'scsi_unblock_requests' can be used during suspend/resume. After the link startup is finished and host is ready, 'scsi_unblock_requests' will be called.

Thanks,
Seungwon Jeon

Quoted 33 lines

Would following implementation looks better?
ufshcd_async_probe()
{
...
ufshcd_link_startup(hba);
...
}
ufshcd_resume()
{
...
ufshcd_link_startup(hba);
...
}
ufshcd_init()
{
...
async_schedule(ufshcd_async_probe, hba);
...
}
--
Regards,
Sujit
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Sujit Reddy Thumma

2013-04-30 08:43 UTC #7 lore raw

Quoted 48 lines · Seungwon Jeon

Quoted 47 lines · Sujit Reddy Thumma
Quoted 45 lines · Seungwon Jeon
Quoted 28 lines · Sujit Reddy Thumma
Quoted 8 lines · Seungwon Jeon
Quoted 7 lines · Sujit Reddy Thumma
Quoted 5 lines · Seungwon Jeon
Link start-up requires long time with multiphase handshakes between UFS host and device. This affects driver's probe time. This patch let link start-up run asynchronously.
And completion time of uic command is defined to avoid a permanent wait.
Quoted 6 lines
Quoted 3 lines
I have similar patch posted few days back "scsi: ufs: Generalize UFS Interconnect Layer (UIC) command support" which does a bit more (mutex, error handling) than what is done here. Can that be used/improved?
I completed to check your patch to compare it now.
Though it's just my thought, the patch I sent is more intuitive on the whole. Considering other dme operations which I have introduced, it looks like matched.
There are lot of code duplications you might want to minimize building a DME command.
Quoted 5 lines
Of course, you may disagree.
But I think the part of mutex is needed. It's a good point.
In case of error handling, I didn't catch nothing special.
Rather, handling link lost case is not proper.
When ufs host meets link lost status, it should start with dme_reset not retried dme_linkstartup.
In section 7.2.1 (Host Controller Initialization) of JESD223A UFS HCI v1.1 specification I find this -
Quoted 8 lines
6. Sent DME_LINKSTARTUP command to start the link startup procedure
9. Check value of HCS.DP and make sure that there is a device attached to the Link. If presence of a device is detected, go to step 10; otherwise, resend the DME_LINKSTARTUP command after IS.ULLS has been set to 1 (Go to step 6). IS.ULLS equal 1 indicates that the UFS Device is ready for a link startup.
Going by the spec. just retrying with DME_LINKSTARTUP is correct.
Yes, as you quoted above, HCI standard mentions that.
Also, the following is mentioned.
UIC Link Lost Status (ULLS) corresponds to the UniPro DME_LINKLOST.ind
I just referred unipro specification.
When DME_LINKLOST.ind is generated, this affects the Link is put in the LinkLost state. Unipro spec says that DME User must apply a DME_RESET to redo the boot sequence. If there is misunderstood meaning and I have something to miss, we can discuss more. Please let me know.

Quoted 7 lines

Quoted 5 lines
Yes, it looks like the two specs. are conflicting each other. I guess we need to take this to Jedec for clarification. Meanwhile, to be on safe side can we add a retry mechanism that does ufshcd_hba_enable() before sending DME_LINKSTARTUP again? This way we can be sure that the DME_RESET and DME_ENABLE is taken care by the host reset itself.
Yes, If the latter case is applied, 'ufshcd_hba_enable' will be start entry for retry. Further, IS.ULLS could be handled through the interrupt instead of polling for retry mechanism?

Agree, but the interrupt handling will be tailored for two things - 1) bootup case where scsi_scan_host is not yet called. 2) the case where link lost occurred after a long time after bootup where there is no need to do scsi_scan_host again.

Quoted 283 lines

Quoted 277 lines
Quoted 136 lines
Quoted 127 lines
In addition, it doesn't say what happens if IS.ULLS never sets to 1. Probably, the case which never happens.
Quoted 1 line
And it would be good if link start-up procedure is done in separate process, not in driver probe.
True.
Quoted 2 lines
If it's all right with you, I'd like to update lock mechanism for uic command. I can add your signed-off. Please let me know your opinion.
I would like to get a third opinion as both the patches needs modifications.
Some comments below:
Quoted 111 lines
Quoted 111 lines
Quoted 111 lines
Signed-off-by Seungwon Jeon <[email protected]>
---
     drivers/scsi/ufs/ufshcd.c |  114 +++++++++++++++++++++++++++++++++-----------
     drivers/scsi/ufs/ufshcd.h |    6 ++-
     2 files changed, 89 insertions(+), 31 deletions(-)
drivers/scsi/ufs/ufshcd.c +48 ~~-11~~
index efe2256..76ff332 100644
@@ -38,6 +38,7 @@
     #define UFSHCD_ENABLE_INTRS	(UTP_TRANSFER_REQ_COMPL |\
     				 UTP_TASK_REQ_COMPL |\
     				 UFSHCD_ERROR_MASK)
+#define UIC_CMD_TIMEOUT	100
 
     enum {
     	UFSHCD_MAX_CHANNEL	= 0,
@@ -357,13 +358,15 @@ static inline void ufshcd_hba_capabilities(struct ufs_hba *hba)
     }
 
     /**
- * ufshcd_send_uic_command - Send UIC commands to unipro layers
+ * ufshcd_dispatch_uic_cmd - Dispatch UIC commands to unipro layers
      * @hba: per adapter instance
      * @uic_command: UIC command
      */
     static inline void
-ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
+ufshcd_dispatch_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmnd)
     {
+	init_completion(&uic_cmnd->done);
+
     	/* Write Args */
     	ufshcd_writel(hba, REG_UIC_COMMAND_ARG_1, uic_cmnd->argument1);
     	ufshcd_writel(hba, REG_UIC_COMMAND_ARG_2, uic_cmnd->argument2);
@@ -375,6 +378,45 @@ ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
     }
 
     /**
+ * ufshcd_wait_for_uic_cmd - Wait complectioin of UIC command
+ * @hba: per adapter instance
+ * @uic_command: UIC command
+ *
+ * Returns 0 only if success.
+ */
+static int ufshcd_wait_for_uic_cmd(struct ufs_hba *hba)
+{
+	struct uic_command *uic_cmd = &hba->active_uic_cmd;
+	int ret;
+
+	if (wait_for_completion_timeout(&uic_cmd->done,
+					msecs_to_jiffies(UIC_CMD_TIMEOUT)))
+		ret = ufshcd_get_uic_cmd_result(hba);
+	else
+		ret = -ETIMEDOUT;
+
+	return ret;
+}
+
+/**
+ * ufshcd_ready_uic_cmd - Check if controller is ready
+ *                        to accept UIC commands
+ * @hba: per adapter instance
+ * Return true on success, else false
+ */
+static inline bool ufshcd_ready_uic_cmd(struct ufs_hba *hba)
+{
+	if (ufshcd_readl(hba, REG_CONTROLLER_STATUS) & UIC_COMMAND_READY) {
+		return true;
+	} else {
+		dev_err(hba->dev,
+				"Controller not ready"
+				" to accept UIC commands\n");
+		return false;
+	}
+}
+
+/**
      * ufshcd_map_sg - Map scatter-gather list to prdt
      * @lrbp - pointer to local reference block
      *
@@ -735,15 +777,10 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
     {
     	struct uic_command *uic_cmd;
     	unsigned long flags;
+	int ret;
 
-	/* check if controller is ready to accept UIC commands */
-	if (((ufshcd_readl(hba, REG_CONTROLLER_STATUS)) &
-	    UIC_COMMAND_READY) == 0x0) {
-		dev_err(hba->dev,
-			"Controller not ready"
-			" to accept UIC commands\n");
+	if (!ufshcd_ready_uic_cmd(hba))
     		return -EIO;
-	}
 
     	spin_lock_irqsave(hba->host->host_lock, flags);
 
@@ -754,13 +791,16 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
     	uic_cmd->argument2 = 0;
     	uic_cmd->argument3 = 0;
 
-	/* enable UIC related interrupts */
-	ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
+	/* Dispatching UIC commands to controller */
+	ufshcd_dispatch_uic_cmd(hba, uic_cmd);
 
Quoted 9 lines
Quoted 6 lines
Quoted 6 lines
drivers/scsi/ufs/ufshcd.c static int ufshcd_dme_link_startup(struct ufs_hba *hba) · line 799
Quoted 6 lines
drivers/scsi/ufs/ufshcd.c +2 -3
-	/* sending UIC commands to controller */
-	ufshcd_send_uic_command(hba, uic_cmd);
     	spin_unlock_irqrestore(hba->host->host_lock, flags);
-	return 0;
+
+	ret = ufshcd_wait_for_uic_cmd(hba);
Error code is incorrect. only -ETIMEDOUT is valid others are just DME errors.
Quoted 4 lines
Only success returns '0', other positive value from dme and -ETIMEDOUT mean failure. Error code can be reused purely, not being redefined.
I am seeing that -EINVAL represents from 01h to 07h in your handling. It looks like error's detail is disappear. Exact return might be needed from DME.
okay.
Quoted 6 lines
Quoted 5 lines
Also, spec. clearly mentions a retry mechanism which means that there could be some timing issues anticipated where the UIC layer cannot respond properly.
Sorry, I didn't catch your meaning fully. Where can I refer to it?
I meant the same retry mechanism mentioned in the section 7.2.1 (Host Controller Initialization) of JESD223A UFS HCI v1.1.
Quoted 120 lines
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
drivers/scsi/ufs/ufshcd.c +4 -0
+	if (ret)
+		dev_err(hba->dev, "link startup: error code %d returned\n", ret);
+
+	return ret;
     }
 
     /**
@@ -898,6 +938,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
     	if (ufshcd_hba_enable(hba))
     		return -EIO;
 
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
drivers/scsi/ufs/ufshcd.c static int ufshcd_initialize_hba(struct ufs_hba *hba) · line 942
Quoted 2 lines
drivers/scsi/ufs/ufshcd.c +2 -0
+	/* enable UIC related interrupts */
+	ufshcd_enable_intr(hba, UIC_COMMAND_COMPL | UIC_ERROR);
The recovery when UIC_ERROR happens is broken because of re-entrancy to dme_link_startup from ufshcd_fatal_err_handler(). So better handle with timeout than allowing controller to raise a UIC_ERROR until that is fixed?
I also recognize error handling should be done further. Ok, I agree with you.
Quoted 19 lines
Quoted 19 lines
Quoted 19 lines
Quoted 19 lines
drivers/scsi/ufs/ufshcd.c +4 -1
+
     	/* Configure UTRL and UTMRL base address registers */
     	ufshcd_writel(hba, REG_UTP_TRANSFER_REQ_LIST_BASE_L,
     		      lower_32_bits(hba->utrdl_dma_addr));
@@ -909,7 +952,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
     		      upper_32_bits(hba->utmrdl_dma_addr));
 
     	/* Initialize unipro link startup procedure */
-	return ufshcd_dme_link_startup(hba);
+	schedule_work(&hba->link_startup_wq);
+
+	return 0;
     }
 
     /**
@@ -1186,6 +1231,16 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
     }
 
Quoted 9 lines
Quoted 7 lines
Quoted 7 lines
drivers/scsi/ufs/ufshcd.c ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) · line 1239
Quoted 7 lines
drivers/scsi/ufs/ufshcd.c +6 -0
     /**
+ * ufshcd_uic_cmd_compl - handle completion of uic command
+ * @hba: per adapter instance
+ */
+static void ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
+{
+	if (intr_status & UIC_COMMAND_COMPL)
why this redundant check if it is already checked in ufshcd_sl_intr()?
Yes, it's currently not needed.
It will be used to identify several uic command. ([PATCH 5/5] scsi: ufs: add dme operations) Anyway, it's better to be removed here.
Quoted 59 lines
Quoted 59 lines
Quoted 59 lines
Quoted 59 lines
drivers/scsi/ufs/ufshcd.c +18 ~~-13~~
+		complete(&hba->active_uic_cmd.done);
+}
+
+/**
      * ufshcd_transfer_req_compl - handle SCSI and query command completion
      * @hba: per adapter instance
      */
@@ -1225,25 +1280,26 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
     }
 
     /**
- * ufshcd_uic_cc_handler - handle UIC command completion
+ * ufshcd_link_startup - link initialization
      * @work: pointer to a work queue structure
- *
- * Returns 0 on success, non-zero value on failure
      */
-static void ufshcd_uic_cc_handler (struct work_struct *work)
+static void ufshcd_link_startup(struct work_struct *work)
     {
     	struct ufs_hba *hba;
+	int ret;
 
-	hba = container_of(work, struct ufs_hba, uic_workq);
+	hba = container_of(work, struct ufs_hba, link_startup_wq);
 
-	if ((hba->active_uic_cmd.command == UIC_CMD_DME_LINK_STARTUP) &&
-	    !(ufshcd_get_uic_cmd_result(hba))) {
+	ret = ufshcd_dme_link_startup(hba);
+	if (ret)
+		goto out;
 
-		if (ufshcd_make_hba_operational(hba))
-			dev_err(hba->dev,
-				"cc: hba not operational state\n");
-		return;
-	}
+	ret = ufshcd_make_hba_operational(hba);
+	if (ret)
+		goto out;
+	return;
+out:
+	dev_err(hba->dev, "link startup failed %d\n", ret);
     }
 
     /**
@@ -1307,7 +1363,7 @@ static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
     		ufshcd_err_handler(hba);
 
     	if (intr_status & UIC_COMMAND_COMPL)
-		schedule_work(&hba->uic_workq);
+		ufshcd_uic_cmd_compl(hba, intr_status);
 
     	if (intr_status & UTP_TASK_REQ_COMPL)
     		ufshcd_tmc_handler(hba);
@@ -1694,7 +1750,7 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
     	init_waitqueue_head(&hba->ufshcd_tm_wait_queue);
 
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
drivers/scsi/ufs/ufshcd.c int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle, · line 1753
Quoted 3 lines
drivers/scsi/ufs/ufshcd.c +1 -1
     	/* Initialize work queues */
-	INIT_WORK(&hba->uic_workq, ufshcd_uic_cc_handler);
+	INIT_WORK(&hba->link_startup_wq, ufshcd_link_startup);
Can we use async function calls kernel/async.c instead of having work queues as this is only used during boot up?
Quoted 4 lines
As we know, both probe and resume are sensitive to execution time. I guess link startup procedure will also be activated in driver's resume. Do you have any specific reason for async function?
Quoted 6 lines
I guess most UFS devices currently are embedded and have rootfs on them. If we use schedule_work there is no synchronization mechanism to check whether the device link startup is completed and device is available for the userspace to mount the partitions. Having async mechanism in place, the prepare_namespace() does wait for such async probes to be completed before mounting the rootfs.

Quoted 4 lines

I understand your meaning.
If you are considering that, I think 'scsi_scan_host' has a role for that. 'scsi_scan_host' will be the conclusion and be done in async subsystem. 'scsi_scan_host' is actually called, after finishing link startup procedure.

with this patch the scsi_scan_host() is called after the work is scheduled. Which means that link_startup_wq might be scheduled after prepare_namespace().

Quoted 40 lines

Quoted 8 lines
I agree that the resume is sensitive to execute time but during resume you can't schedule link startup work because the fs/block/scsi layer expects the device availability as soon as resume operation is completed. So ufshcd_link_startup() should be called in the resume context itself or implement a synchronization mechanism like blocking scsi layer queuing requests until link startup is completed.
'scsi_block_requests' and 'scsi_unblock_requests' can be used during suspend/resume. After the link startup is finished and host is ready, 'scsi_unblock_requests' will be called.
Thanks,
Seungwon Jeon
Quoted 27 lines
Would following implementation looks better?
ufshcd_async_probe()
{
...
    ufshcd_link_startup(hba);
...
}
ufshcd_resume()
{
...
ufshcd_link_startup(hba);
...
}
ufshcd_init()
{
...
async_schedule(ufshcd_async_probe, hba);
...
}
--

Signature · 2 lines

Regards,
Sujit

Seungwon Jeon

author

2013-05-02 05:15 UTC #8 lore raw

Quoted 54 lines · Sujit Reddy Thumma

Quoted 53 lines · Seungwon Jeon
Quoted 52 lines · Sujit Reddy Thumma
Quoted 37 lines · Seungwon Jeon
Quoted 28 lines · Sujit Reddy Thumma
Quoted 8 lines · Seungwon Jeon
Quoted 7 lines · Sujit Reddy Thumma
Quoted 5 lines · Seungwon Jeon
Link start-up requires long time with multiphase handshakes between UFS host and device. This affects driver's probe time. This patch let link start-up run asynchronously.
And completion time of uic command is defined to avoid a permanent wait.
Quoted 6 lines
Quoted 3 lines
I have similar patch posted few days back "scsi: ufs: Generalize UFS Interconnect Layer (UIC) command support" which does a bit more (mutex, error handling) than what is done here. Can that be used/improved?
I completed to check your patch to compare it now.
Though it's just my thought, the patch I sent is more intuitive on the whole. Considering other dme operations which I have introduced, it looks like matched.
There are lot of code duplications you might want to minimize building a DME command.
Quoted 5 lines
Of course, you may disagree.
But I think the part of mutex is needed. It's a good point.
In case of error handling, I didn't catch nothing special.
Rather, handling link lost case is not proper.
When ufs host meets link lost status, it should start with dme_reset not retried dme_linkstartup.
In section 7.2.1 (Host Controller Initialization) of JESD223A UFS HCI v1.1 specification I find this -
Quoted 8 lines
6. Sent DME_LINKSTARTUP command to start the link startup procedure
9. Check value of HCS.DP and make sure that there is a device attached to the Link. If presence of a device is detected, go to step 10; otherwise, resend the DME_LINKSTARTUP command after IS.ULLS has been set to 1 (Go to step 6). IS.ULLS equal 1 indicates that the UFS Device is ready for a link startup.
Going by the spec. just retrying with DME_LINKSTARTUP is correct.
Quoted 8 lines
Yes, as you quoted above, HCI standard mentions that.
Also, the following is mentioned.
UIC Link Lost Status (ULLS) corresponds to the UniPro DME_LINKLOST.ind
I just referred unipro specification.
When DME_LINKLOST.ind is generated, this affects the Link is put in the LinkLost state. Unipro spec says that DME User must apply a DME_RESET to redo the boot sequence. If there is misunderstood meaning and I have something to miss, we can discuss more. Please let me know.
Yes, it looks like the two specs. are conflicting each other. I guess we need to take this to Jedec for clarification. Meanwhile, to be on safe side can we add a retry mechanism that does ufshcd_hba_enable() before sending DME_LINKSTARTUP again? This way we can be sure that the DME_RESET and DME_ENABLE is taken care by the host reset itself.

Quoted 7 lines

Quoted 2 lines
Yes, If the latter case is applied, 'ufshcd_hba_enable' will be start entry for retry. Further, IS.ULLS could be handled through the interrupt instead of polling for retry mechanism?
Agree, but the interrupt handling will be tailored for two things - 1) bootup case where scsi_scan_host is not yet called. 2) the case where link lost occurred after a long time after bootup where there is no need to do scsi_scan_host again.

Yes, it could be another patch.

Quoted 284 lines

Quoted 284 lines
Quoted 284 lines
Quoted 137 lines
Quoted 128 lines
In addition, it doesn't say what happens if IS.ULLS never sets to 1. Probably, the case which never happens.
Quoted 1 line
And it would be good if link start-up procedure is done in separate process, not in driver probe.
True.
Quoted 2 lines
If it's all right with you, I'd like to update lock mechanism for uic command. I can add your signed-off. Please let me know your opinion.
I would like to get a third opinion as both the patches needs modifications.
Some comments below:
Quoted 111 lines
Quoted 111 lines
Quoted 111 lines
Signed-off-by Seungwon Jeon <[email protected]>
---
     drivers/scsi/ufs/ufshcd.c |  114 +++++++++++++++++++++++++++++++++-----------
     drivers/scsi/ufs/ufshcd.h |    6 ++-
     2 files changed, 89 insertions(+), 31 deletions(-)
drivers/scsi/ufs/ufshcd.c +48 ~~-11~~
index efe2256..76ff332 100644
@@ -38,6 +38,7 @@
     #define UFSHCD_ENABLE_INTRS	(UTP_TRANSFER_REQ_COMPL |\
     				 UTP_TASK_REQ_COMPL |\
     				 UFSHCD_ERROR_MASK)
+#define UIC_CMD_TIMEOUT	100
 
     enum {
     	UFSHCD_MAX_CHANNEL	= 0,
@@ -357,13 +358,15 @@ static inline void ufshcd_hba_capabilities(struct ufs_hba *hba)
     }
 
     /**
- * ufshcd_send_uic_command - Send UIC commands to unipro layers
+ * ufshcd_dispatch_uic_cmd - Dispatch UIC commands to unipro layers
      * @hba: per adapter instance
      * @uic_command: UIC command
      */
     static inline void
-ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
+ufshcd_dispatch_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmnd)
     {
+	init_completion(&uic_cmnd->done);
+
     	/* Write Args */
     	ufshcd_writel(hba, REG_UIC_COMMAND_ARG_1, uic_cmnd->argument1);
     	ufshcd_writel(hba, REG_UIC_COMMAND_ARG_2, uic_cmnd->argument2);
@@ -375,6 +378,45 @@ ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
     }
 
     /**
+ * ufshcd_wait_for_uic_cmd - Wait complectioin of UIC command
+ * @hba: per adapter instance
+ * @uic_command: UIC command
+ *
+ * Returns 0 only if success.
+ */
+static int ufshcd_wait_for_uic_cmd(struct ufs_hba *hba)
+{
+	struct uic_command *uic_cmd = &hba->active_uic_cmd;
+	int ret;
+
+	if (wait_for_completion_timeout(&uic_cmd->done,
+					msecs_to_jiffies(UIC_CMD_TIMEOUT)))
+		ret = ufshcd_get_uic_cmd_result(hba);
+	else
+		ret = -ETIMEDOUT;
+
+	return ret;
+}
+
+/**
+ * ufshcd_ready_uic_cmd - Check if controller is ready
+ *                        to accept UIC commands
+ * @hba: per adapter instance
+ * Return true on success, else false
+ */
+static inline bool ufshcd_ready_uic_cmd(struct ufs_hba *hba)
+{
+	if (ufshcd_readl(hba, REG_CONTROLLER_STATUS) & UIC_COMMAND_READY) {
+		return true;
+	} else {
+		dev_err(hba->dev,
+				"Controller not ready"
+				" to accept UIC commands\n");
+		return false;
+	}
+}
+
+/**
      * ufshcd_map_sg - Map scatter-gather list to prdt
      * @lrbp - pointer to local reference block
      *
@@ -735,15 +777,10 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
     {
     	struct uic_command *uic_cmd;
     	unsigned long flags;
+	int ret;
 
-	/* check if controller is ready to accept UIC commands */
-	if (((ufshcd_readl(hba, REG_CONTROLLER_STATUS)) &
-	    UIC_COMMAND_READY) == 0x0) {
-		dev_err(hba->dev,
-			"Controller not ready"
-			" to accept UIC commands\n");
+	if (!ufshcd_ready_uic_cmd(hba))
     		return -EIO;
-	}
 
     	spin_lock_irqsave(hba->host->host_lock, flags);
 
@@ -754,13 +791,16 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
     	uic_cmd->argument2 = 0;
     	uic_cmd->argument3 = 0;
 
-	/* enable UIC related interrupts */
-	ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
+	/* Dispatching UIC commands to controller */
+	ufshcd_dispatch_uic_cmd(hba, uic_cmd);
 
Quoted 9 lines
Quoted 6 lines
Quoted 6 lines
drivers/scsi/ufs/ufshcd.c static int ufshcd_dme_link_startup(struct ufs_hba *hba) · line 799
Quoted 6 lines
drivers/scsi/ufs/ufshcd.c +2 -3
-	/* sending UIC commands to controller */
-	ufshcd_send_uic_command(hba, uic_cmd);
     	spin_unlock_irqrestore(hba->host->host_lock, flags);
-	return 0;
+
+	ret = ufshcd_wait_for_uic_cmd(hba);
Error code is incorrect. only -ETIMEDOUT is valid others are just DME errors.
Quoted 4 lines
Only success returns '0', other positive value from dme and -ETIMEDOUT mean failure. Error code can be reused purely, not being redefined.
I am seeing that -EINVAL represents from 01h to 07h in your handling. It looks like error's detail is disappear. Exact return might be needed from DME.
okay.
Quoted 6 lines
Quoted 5 lines
Also, spec. clearly mentions a retry mechanism which means that there could be some timing issues anticipated where the UIC layer cannot respond properly.
Sorry, I didn't catch your meaning fully. Where can I refer to it?
I meant the same retry mechanism mentioned in the section 7.2.1 (Host Controller Initialization) of JESD223A UFS HCI v1.1.
Quoted 120 lines
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
drivers/scsi/ufs/ufshcd.c +4 -0
+	if (ret)
+		dev_err(hba->dev, "link startup: error code %d returned\n", ret);
+
+	return ret;
     }
 
     /**
@@ -898,6 +938,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
     	if (ufshcd_hba_enable(hba))
     		return -EIO;
 
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
drivers/scsi/ufs/ufshcd.c static int ufshcd_initialize_hba(struct ufs_hba *hba) · line 942
Quoted 2 lines
drivers/scsi/ufs/ufshcd.c +2 -0
+	/* enable UIC related interrupts */
+	ufshcd_enable_intr(hba, UIC_COMMAND_COMPL | UIC_ERROR);
The recovery when UIC_ERROR happens is broken because of re-entrancy to dme_link_startup from ufshcd_fatal_err_handler(). So better handle with timeout than allowing controller to raise a UIC_ERROR until that is fixed?
I also recognize error handling should be done further. Ok, I agree with you.
Quoted 19 lines
Quoted 19 lines
Quoted 19 lines
Quoted 19 lines
drivers/scsi/ufs/ufshcd.c +4 -1
+
     	/* Configure UTRL and UTMRL base address registers */
     	ufshcd_writel(hba, REG_UTP_TRANSFER_REQ_LIST_BASE_L,
     		      lower_32_bits(hba->utrdl_dma_addr));
@@ -909,7 +952,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
     		      upper_32_bits(hba->utmrdl_dma_addr));
 
     	/* Initialize unipro link startup procedure */
-	return ufshcd_dme_link_startup(hba);
+	schedule_work(&hba->link_startup_wq);
+
+	return 0;
     }
 
     /**
@@ -1186,6 +1231,16 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
     }
 
Quoted 9 lines
Quoted 7 lines
Quoted 7 lines
drivers/scsi/ufs/ufshcd.c ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) · line 1239
Quoted 7 lines
drivers/scsi/ufs/ufshcd.c +6 -0
     /**
+ * ufshcd_uic_cmd_compl - handle completion of uic command
+ * @hba: per adapter instance
+ */
+static void ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
+{
+	if (intr_status & UIC_COMMAND_COMPL)
why this redundant check if it is already checked in ufshcd_sl_intr()?
Yes, it's currently not needed.
It will be used to identify several uic command. ([PATCH 5/5] scsi: ufs: add dme operations) Anyway, it's better to be removed here.
Quoted 59 lines
Quoted 59 lines
Quoted 59 lines
Quoted 59 lines
drivers/scsi/ufs/ufshcd.c +18 ~~-13~~
+		complete(&hba->active_uic_cmd.done);
+}
+
+/**
      * ufshcd_transfer_req_compl - handle SCSI and query command completion
      * @hba: per adapter instance
      */
@@ -1225,25 +1280,26 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
     }
 
     /**
- * ufshcd_uic_cc_handler - handle UIC command completion
+ * ufshcd_link_startup - link initialization
      * @work: pointer to a work queue structure
- *
- * Returns 0 on success, non-zero value on failure
      */
-static void ufshcd_uic_cc_handler (struct work_struct *work)
+static void ufshcd_link_startup(struct work_struct *work)
     {
     	struct ufs_hba *hba;
+	int ret;
 
-	hba = container_of(work, struct ufs_hba, uic_workq);
+	hba = container_of(work, struct ufs_hba, link_startup_wq);
 
-	if ((hba->active_uic_cmd.command == UIC_CMD_DME_LINK_STARTUP) &&
-	    !(ufshcd_get_uic_cmd_result(hba))) {
+	ret = ufshcd_dme_link_startup(hba);
+	if (ret)
+		goto out;
 
-		if (ufshcd_make_hba_operational(hba))
-			dev_err(hba->dev,
-				"cc: hba not operational state\n");
-		return;
-	}
+	ret = ufshcd_make_hba_operational(hba);
+	if (ret)
+		goto out;
+	return;
+out:
+	dev_err(hba->dev, "link startup failed %d\n", ret);
     }
 
     /**
@@ -1307,7 +1363,7 @@ static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
     		ufshcd_err_handler(hba);
 
     	if (intr_status & UIC_COMMAND_COMPL)
-		schedule_work(&hba->uic_workq);
+		ufshcd_uic_cmd_compl(hba, intr_status);
 
     	if (intr_status & UTP_TASK_REQ_COMPL)
     		ufshcd_tmc_handler(hba);
@@ -1694,7 +1750,7 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
     	init_waitqueue_head(&hba->ufshcd_tm_wait_queue);
 
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
drivers/scsi/ufs/ufshcd.c int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle, · line 1753
Quoted 3 lines
drivers/scsi/ufs/ufshcd.c +1 -1
     	/* Initialize work queues */
-	INIT_WORK(&hba->uic_workq, ufshcd_uic_cc_handler);
+	INIT_WORK(&hba->link_startup_wq, ufshcd_link_startup);
Can we use async function calls kernel/async.c instead of having work queues as this is only used during boot up?
Quoted 4 lines
As we know, both probe and resume are sensitive to execution time. I guess link startup procedure will also be activated in driver's resume. Do you have any specific reason for async function?
I guess most UFS devices currently are embedded and have rootfs on them. If we use schedule_work there is no synchronization mechanism to check whether the device link startup is completed and device is available for the userspace to mount the partitions. Having async mechanism in place, the prepare_namespace() does wait for such async probes to be completed before mounting the rootfs.

Quoted 8 lines

Quoted 4 lines
I understand your meaning.
If you are considering that, I think 'scsi_scan_host' has a role for that. 'scsi_scan_host' will be the conclusion and be done in async subsystem. 'scsi_scan_host' is actually called, after finishing link startup procedure.
with this patch the scsi_scan_host() is called after the work is scheduled. Which means that link_startup_wq might be scheduled after prepare_namespace().

AFAIK, the work is scheduled immediately. So I think we don't need to worry about that. I guess you want to gather all of initialization sequence into async mechanism including link startup. If your intention is for that reason, it's okay to use async_schedule.
Then, we may reuse these in driver's resume.
I should have answered quickly, but I have a day off yesterday.
If you have any opinion, please let me know.

Thanks,
Seungwon Jeon

Quoted 49 lines

Quoted 9 lines
Quoted 9 lines
I agree that the resume is sensitive to execute time but during resume you can't schedule link startup work because the fs/block/scsi layer expects the device availability as soon as resume operation is completed. So ufshcd_link_startup() should be called in the resume context itself or implement a synchronization mechanism like blocking scsi layer queuing requests until link startup is completed.
Quoted 32 lines
'scsi_block_requests' and 'scsi_unblock_requests' can be used during suspend/resume. After the link startup is finished and host is ready, 'scsi_unblock_requests' will be called.
Thanks,
Seungwon Jeon
Quoted 27 lines
Would following implementation looks better?
ufshcd_async_probe()
{
...
    ufshcd_link_startup(hba);
...
}
ufshcd_resume()
{
...
ufshcd_link_startup(hba);
...
}
ufshcd_init()
{
...
async_schedule(ufshcd_async_probe, hba);
...
}
--
--
Regards,
Sujit
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Santosh Y

2013-05-02 07:59 UTC #9 lore raw

Quoted 55 lines · Seungwon Jeon

Quoted 54 lines · Sujit Reddy Thumma
Quoted 53 lines · Seungwon Jeon
Quoted 52 lines · Sujit Reddy Thumma
Quoted 37 lines · Seungwon Jeon
Quoted 28 lines · Sujit Reddy Thumma
Quoted 8 lines · Seungwon Jeon
Quoted 7 lines · Sujit Reddy Thumma
Quoted 5 lines · Seungwon Jeon
Link start-up requires long time with multiphase handshakes between UFS host and device. This affects driver's probe time. This patch let link start-up run asynchronously.
And completion time of uic command is defined to avoid a permanent wait.
Quoted 6 lines
Quoted 3 lines
I have similar patch posted few days back "scsi: ufs: Generalize UFS Interconnect Layer (UIC) command support" which does a bit more (mutex, error handling) than what is done here. Can that be used/improved?
I completed to check your patch to compare it now.
Though it's just my thought, the patch I sent is more intuitive on the whole. Considering other dme operations which I have introduced, it looks like matched.
There are lot of code duplications you might want to minimize building a DME command.
Quoted 5 lines
Of course, you may disagree.
But I think the part of mutex is needed. It's a good point.
In case of error handling, I didn't catch nothing special.
Rather, handling link lost case is not proper.
When ufs host meets link lost status, it should start with dme_reset not retried dme_linkstartup.
In section 7.2.1 (Host Controller Initialization) of JESD223A UFS HCI v1.1 specification I find this -
Quoted 8 lines
6. Sent DME_LINKSTARTUP command to start the link startup procedure
9. Check value of HCS.DP and make sure that there is a device attached to the Link. If presence of a device is detected, go to step 10; otherwise, resend the DME_LINKSTARTUP command after IS.ULLS has been set to 1 (Go to step 6). IS.ULLS equal 1 indicates that the UFS Device is ready for a link startup.
Going by the spec. just retrying with DME_LINKSTARTUP is correct.
Quoted 8 lines
Yes, as you quoted above, HCI standard mentions that.
Also, the following is mentioned.
UIC Link Lost Status (ULLS) corresponds to the UniPro DME_LINKLOST.ind
I just referred unipro specification.
When DME_LINKLOST.ind is generated, this affects the Link is put in the LinkLost state. Unipro spec says that DME User must apply a DME_RESET to redo the boot sequence. If there is misunderstood meaning and I have something to miss, we can discuss more. Please let me know.
Yes, it looks like the two specs. are conflicting each other. I guess we need to take this to Jedec for clarification. Meanwhile, to be on safe side can we add a retry mechanism that does ufshcd_hba_enable() before sending DME_LINKSTARTUP again? This way we can be sure that the DME_RESET and DME_ENABLE is taken care by the host reset itself.

Quoted 19 lines

Quoted 7 lines
Quoted 2 lines
Yes, If the latter case is applied, 'ufshcd_hba_enable' will be start entry for retry. Further, IS.ULLS could be handled through the interrupt instead of polling for retry mechanism?
Agree, but the interrupt handling will be tailored for two things - 1) bootup case where scsi_scan_host is not yet called. 2) the case where link lost occurred after a long time after bootup where there is no need to do scsi_scan_host again.
Yes, it could be another patch.
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
In addition, it doesn't say what happens if IS.ULLS never sets to 1. Probably, the case which never happens.
Quoted 1 line
And it would be good if link start-up procedure is done in separate process, not in driver probe.
True.

Yes, but the probe should be notified of the link-startup status.

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
If it's all right with you, I'd like to update lock mechanism for uic command. I can add your signed-off. Please let me know your opinion.

If you decide to combine the patches, then please do the lock/unlock in send_uic_command.

Quoted 171 lines

Quoted 124 lines

Quoted 115 lines

I would like to get a third opinion as both the patches needs modifications.

Some comments below:

Quoted 111 lines

Signed-off-by Seungwon Jeon <[email protected]>

---
     drivers/scsi/ufs/ufshcd.c |  114 +++++++++++++++++++++++++++++++++-----------
     drivers/scsi/ufs/ufshcd.h |    6 ++-
     2 files changed, 89 insertions(+), 31 deletions(-)

drivers/scsi/ufs/ufshcd.c +48 ~~-11~~

index efe2256..76ff332 100644
@@ -38,6 +38,7 @@
     #define UFSHCD_ENABLE_INTRS      (UTP_TRANSFER_REQ_COMPL |\
                               UTP_TASK_REQ_COMPL |\
                               UFSHCD_ERROR_MASK)
+#define UIC_CMD_TIMEOUT      100
 
     enum {
      UFSHCD_MAX_CHANNEL      = 0,

@@ -357,13 +358,15 @@ static inline void ufshcd_hba_capabilities(struct ufs_hba *hba)
     }
 
     /**
- * ufshcd_send_uic_command - Send UIC commands to unipro layers
+ * ufshcd_dispatch_uic_cmd - Dispatch UIC commands to unipro layers
      * @hba: per adapter instance
      * @uic_command: UIC command
      */
     static inline void
-ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
+ufshcd_dispatch_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmnd)
     {
+     init_completion(&uic_cmnd->done);
+
      /* Write Args */
      ufshcd_writel(hba, REG_UIC_COMMAND_ARG_1, uic_cmnd->argument1);
      ufshcd_writel(hba, REG_UIC_COMMAND_ARG_2, uic_cmnd->argument2);

@@ -375,6 +378,45 @@ ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
     }
 
     /**
+ * ufshcd_wait_for_uic_cmd - Wait complectioin of UIC command
+ * @hba: per adapter instance
+ * @uic_command: UIC command
+ *
+ * Returns 0 only if success.
+ */
+static int ufshcd_wait_for_uic_cmd(struct ufs_hba *hba)
+{
+     struct uic_command *uic_cmd = &hba->active_uic_cmd;
+     int ret;
+
+     if (wait_for_completion_timeout(&uic_cmd->done,
+                                     msecs_to_jiffies(UIC_CMD_TIMEOUT)))
+             ret = ufshcd_get_uic_cmd_result(hba);
+     else
+             ret = -ETIMEDOUT;
+
+     return ret;
+}
+
+/**
+ * ufshcd_ready_uic_cmd - Check if controller is ready
+ *                        to accept UIC commands
+ * @hba: per adapter instance
+ * Return true on success, else false
+ */
+static inline bool ufshcd_ready_uic_cmd(struct ufs_hba *hba)
+{
+     if (ufshcd_readl(hba, REG_CONTROLLER_STATUS) & UIC_COMMAND_READY) {
+             return true;
+     } else {
+             dev_err(hba->dev,
+                             "Controller not ready"
+                             " to accept UIC commands\n");
+             return false;
+     }
+}
+
+/**
      * ufshcd_map_sg - Map scatter-gather list to prdt
      * @lrbp - pointer to local reference block
      *

@@ -735,15 +777,10 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
     {
      struct uic_command *uic_cmd;
      unsigned long flags;
+     int ret;
 
-     /* check if controller is ready to accept UIC commands */
-     if (((ufshcd_readl(hba, REG_CONTROLLER_STATUS)) &
-         UIC_COMMAND_READY) == 0x0) {
-             dev_err(hba->dev,
-                     "Controller not ready"
-                     " to accept UIC commands\n");
+     if (!ufshcd_ready_uic_cmd(hba))
              return -EIO;
-     }
 
      spin_lock_irqsave(hba->host->host_lock, flags);

@@ -754,13 +791,16 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
      uic_cmd->argument2 = 0;
      uic_cmd->argument3 = 0;
 
-     /* enable UIC related interrupts */
-     ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
+     /* Dispatching UIC commands to controller */
+     ufshcd_dispatch_uic_cmd(hba, uic_cmd);

Quoted 9 lines

Quoted 6 lines
Quoted 6 lines
drivers/scsi/ufs/ufshcd.c static int ufshcd_dme_link_startup(struct ufs_hba *hba) · line 799
Quoted 6 lines
drivers/scsi/ufs/ufshcd.c +2 -3
-     /* sending UIC commands to controller */
-     ufshcd_send_uic_command(hba, uic_cmd);
      spin_unlock_irqrestore(hba->host->host_lock, flags);
-     return 0;
+
+     ret = ufshcd_wait_for_uic_cmd(hba);
Error code is incorrect. only -ETIMEDOUT is valid others are just DME errors.

Quoted 4 lines

Only success returns '0', other positive value from dme and -ETIMEDOUT mean failure. Error code can be reused purely, not being redefined.
I am seeing that -EINVAL represents from 01h to 07h in your handling. It looks like error's detail is disappear. Exact return might be needed from DME.

okay.

Quoted 6 lines

Quoted 5 lines
Also, spec. clearly mentions a retry mechanism which means that there could be some timing issues anticipated where the UIC layer cannot respond properly.
Sorry, I didn't catch your meaning fully. Where can I refer to it?

I meant the same retry mechanism mentioned in the section 7.2.1 (Host Controller Initialization) of JESD223A UFS HCI v1.1.

Quoted 31 lines

Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
drivers/scsi/ufs/ufshcd.c +4 -0
+     if (ret)
+             dev_err(hba->dev, "link startup: error code %d returned\n", ret);
+
+     return ret;
     }
 
     /**
@@ -898,6 +938,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
      if (ufshcd_hba_enable(hba))
              return -EIO;
 
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
drivers/scsi/ufs/ufshcd.c static int ufshcd_initialize_hba(struct ufs_hba *hba) · line 942
Quoted 2 lines
drivers/scsi/ufs/ufshcd.c +2 -0
+     /* enable UIC related interrupts */
+     ufshcd_enable_intr(hba, UIC_COMMAND_COMPL | UIC_ERROR);
The recovery when UIC_ERROR happens is broken because of re-entrancy to dme_link_startup from ufshcd_fatal_err_handler(). So better handle with timeout than allowing controller to raise a UIC_ERROR until that is fixed?
I also recognize error handling should be done further. Ok, I agree with you.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/scsi/ufs/ufshcd.c +1 -0
+
      /* Configure UTRL and UTMRL base address registers */
      ufshcd_writel(hba, REG_UTP_TRANSFER_REQ_LIST_BASE_L,
                    lower_32_bits(hba->utrdl_dma_addr));
@@ -909,7 +952,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
                    upper_32_bits(hba->utmrdl_dma_addr));
 

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/scsi/ufs/ufshcd.c static int ufshcd_initialize_hba(struct ufs_hba *hba) · line 958
Quoted 6 lines
drivers/scsi/ufs/ufshcd.c +3 -1
      /* Initialize unipro link startup procedure */
-     return ufshcd_dme_link_startup(hba);
+     schedule_work(&hba->link_startup_wq);
+
+     return 0;
     }

With this approach, always success is returned to the probe irrespective of link-startup result.
Also if the link-startup fails there is no mechanism to retry the procedure. So it is better to combine it with Sujith's patch.

Quoted 161 lines

Quoted 94 lines
Quoted 94 lines
Quoted 94 lines
Quoted 83 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/scsi/ufs/ufshcd.c +0 -0
     /**
@@ -1186,6 +1231,16 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
     }
 
Quoted 9 lines
Quoted 7 lines
Quoted 7 lines
drivers/scsi/ufs/ufshcd.c ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) · line 1239
Quoted 7 lines
drivers/scsi/ufs/ufshcd.c +6 -0
     /**
+ * ufshcd_uic_cmd_compl - handle completion of uic command
+ * @hba: per adapter instance
+ */
+static void ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
+{
+     if (intr_status & UIC_COMMAND_COMPL)
why this redundant check if it is already checked in ufshcd_sl_intr()?
Yes, it's currently not needed.
It will be used to identify several uic command. ([PATCH 5/5] scsi: ufs: add dme operations) Anyway, it's better to be removed here.
Quoted 59 lines
Quoted 59 lines
Quoted 59 lines
Quoted 59 lines
drivers/scsi/ufs/ufshcd.c +18 ~~-13~~
+             complete(&hba->active_uic_cmd.done);
+}
+
+/**
      * ufshcd_transfer_req_compl - handle SCSI and query command completion
      * @hba: per adapter instance
      */
@@ -1225,25 +1280,26 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
     }
 
     /**
- * ufshcd_uic_cc_handler - handle UIC command completion
+ * ufshcd_link_startup - link initialization
      * @work: pointer to a work queue structure
- *
- * Returns 0 on success, non-zero value on failure
      */
-static void ufshcd_uic_cc_handler (struct work_struct *work)
+static void ufshcd_link_startup(struct work_struct *work)
     {
      struct ufs_hba *hba;
+     int ret;
 
-     hba = container_of(work, struct ufs_hba, uic_workq);
+     hba = container_of(work, struct ufs_hba, link_startup_wq);
 
-     if ((hba->active_uic_cmd.command == UIC_CMD_DME_LINK_STARTUP) &&
-         !(ufshcd_get_uic_cmd_result(hba))) {
+     ret = ufshcd_dme_link_startup(hba);
+     if (ret)
+             goto out;
 
-             if (ufshcd_make_hba_operational(hba))
-                     dev_err(hba->dev,
-                             "cc: hba not operational state\n");
-             return;
-     }
+     ret = ufshcd_make_hba_operational(hba);
+     if (ret)
+             goto out;
+     return;
+out:
+     dev_err(hba->dev, "link startup failed %d\n", ret);
     }
 
     /**
@@ -1307,7 +1363,7 @@ static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
              ufshcd_err_handler(hba);
 
      if (intr_status & UIC_COMMAND_COMPL)
-             schedule_work(&hba->uic_workq);
+             ufshcd_uic_cmd_compl(hba, intr_status);
 
      if (intr_status & UTP_TASK_REQ_COMPL)
              ufshcd_tmc_handler(hba);
@@ -1694,7 +1750,7 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
      init_waitqueue_head(&hba->ufshcd_tm_wait_queue);
 
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
drivers/scsi/ufs/ufshcd.c int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle, · line 1753
Quoted 3 lines
drivers/scsi/ufs/ufshcd.c +1 -1
      /* Initialize work queues */
-     INIT_WORK(&hba->uic_workq, ufshcd_uic_cc_handler);
+     INIT_WORK(&hba->link_startup_wq, ufshcd_link_startup);
Can we use async function calls kernel/async.c instead of having work queues as this is only used during boot up?
Quoted 4 lines
As we know, both probe and resume are sensitive to execution time. I guess link startup procedure will also be activated in driver's resume. Do you have any specific reason for async function?
I guess most UFS devices currently are embedded and have rootfs on them. If we use schedule_work there is no synchronization mechanism to check whether the device link startup is completed and device is available for the userspace to mount the partitions. Having async mechanism in place, the prepare_namespace() does wait for such async probes to be completed before mounting the rootfs.
Quoted 8 lines
Quoted 4 lines
I understand your meaning.
If you are considering that, I think 'scsi_scan_host' has a role for that. 'scsi_scan_host' will be the conclusion and be done in async subsystem. 'scsi_scan_host' is actually called, after finishing link startup procedure.
with this patch the scsi_scan_host() is called after the work is scheduled. Which means that link_startup_wq might be scheduled after prepare_namespace().
AFAIK, the work is scheduled immediately. So I think we don't need to worry about that. I guess you want to gather all of initialization sequence into async mechanism including link startup. If your intention is for that reason, it's okay to use async_schedule.
Then, we may reuse these in driver's resume.
I should have answered quickly, but I have a day off yesterday.
If you have any opinion, please let me know.
Thanks,
Seungwon Jeon
Quoted 49 lines
Quoted 9 lines
Quoted 9 lines
I agree that the resume is sensitive to execute time but during resume you can't schedule link startup work because the fs/block/scsi layer expects the device availability as soon as resume operation is completed. So ufshcd_link_startup() should be called in the resume context itself or implement a synchronization mechanism like blocking scsi layer queuing requests until link startup is completed.
Quoted 32 lines
'scsi_block_requests' and 'scsi_unblock_requests' can be used during suspend/resume. After the link startup is finished and host is ready, 'scsi_unblock_requests' will be called.
Thanks,
Seungwon Jeon
Quoted 27 lines
Would following implementation looks better?
ufshcd_async_probe()
{
...
    ufshcd_link_startup(hba);
...
}
ufshcd_resume()
{
...
ufshcd_link_startup(hba);
...
}
ufshcd_init()
{
...
async_schedule(ufshcd_async_probe, hba);
...
}
--
--
Regards,
Sujit
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 1 line

~Santosh

Seungwon Jeon

author

2013-05-02 13:38 UTC #10 lore raw

Quoted 71 lines · Santosh Y

Quoted 21 lines · Seungwon Jeon
Quoted 20 lines · Sujit Reddy Thumma
Quoted 19 lines · Seungwon Jeon
Quoted 18 lines · Sujit Reddy Thumma
Quoted 17 lines · Seungwon Jeon
Quoted 16 lines · Sujit Reddy Thumma
Quoted 14 lines · Seungwon Jeon
Quoted 10 lines · Sujit Reddy Thumma
Quoted 5 lines · Seungwon Jeon
Link start-up requires long time with multiphase handshakes between UFS host and device. This affects driver's probe time. This patch let link start-up run asynchronously.
And completion time of uic command is defined to avoid a permanent wait.
I have similar patch posted few days back "scsi: ufs: Generalize UFS Interconnect Layer (UIC) command support" which does a bit more (mutex, error handling) than what is done here. Can that be used/improved?
I completed to check your patch to compare it now.
Though it's just my thought, the patch I sent is more intuitive on the whole. Considering other dme operations which I have introduced, it looks like matched.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
There are lot of code duplications you might want to minimize building a DME command.
Quoted 5 lines
Of course, you may disagree.
But I think the part of mutex is needed. It's a good point.
In case of error handling, I didn't catch nothing special.
Rather, handling link lost case is not proper.
When ufs host meets link lost status, it should start with dme_reset not retried
dme_linkstartup.
Quoted 40 lines
Quoted 26 lines
Quoted 26 lines
Quoted 26 lines
Quoted 12 lines
Quoted 4 lines
In section 7.2.1 (Host Controller Initialization) of JESD223A UFS HCI v1.1 specification I find this -
Quoted 8 lines
6. Sent DME_LINKSTARTUP command to start the link startup procedure
9. Check value of HCS.DP and make sure that there is a device attached to the Link. If presence of a device is detected, go to step 10; otherwise, resend the DME_LINKSTARTUP command after IS.ULLS has been set to 1 (Go to step 6). IS.ULLS equal 1 indicates that the UFS Device is ready for a link startup.
Going by the spec. just retrying with DME_LINKSTARTUP is correct.
Quoted 8 lines
Yes, as you quoted above, HCI standard mentions that.
Also, the following is mentioned.
UIC Link Lost Status (ULLS) corresponds to the UniPro DME_LINKLOST.ind
I just referred unipro specification.
When DME_LINKLOST.ind is generated, this affects the Link is put in the LinkLost state. Unipro spec says that DME User must apply a DME_RESET to redo the boot sequence. If there is misunderstood meaning and I have something to miss, we can discuss more. Please let me know.
Yes, it looks like the two specs. are conflicting each other. I guess we need to take this to Jedec for clarification. Meanwhile, to be on safe side can we add a retry mechanism that does ufshcd_hba_enable() before sending DME_LINKSTARTUP again? This way we can be sure that the DME_RESET and DME_ENABLE is taken care by the host reset itself.
Quoted 7 lines
Quoted 2 lines
Yes, If the latter case is applied, 'ufshcd_hba_enable' will be start entry for retry. Further, IS.ULLS could be handled through the interrupt instead of polling for retry mechanism?
Agree, but the interrupt handling will be tailored for two things - 1) bootup case where scsi_scan_host is not yet called. 2) the case where link lost occurred after a long time after bootup where there is no need to do scsi_scan_host again.
Yes, it could be another patch.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

Quoted 8 lines

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
In addition, it doesn't say what happens if IS.ULLS never sets to 1. Probably, the case which never happens.
Quoted 1 line
And it would be good if link start-up procedure is done in separate process, not in driver
probe.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
True.
Yes, but the probe should be notified of the link-startup status.

When hci driver's probe does itself, it doesn't need to guarantee to complete link startup. Actually, it is communication for initialization between host and device.
If link startup should be done in probe time, there is no meaning for separate process.

Quoted 7 lines

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
If it's all right with you, I'd like to update lock mechanism for uic command. I can add your signed-off. Please let me know your opinion.
If you decide to combine the patches, then please do the lock/unlock in send_uic_command.

Ok.

Quoted 174 lines

Quoted 29 lines

I would like to get a third opinion as both the patches needs modifications.

Some comments below:

Quoted 23 lines

Signed-off-by Seungwon Jeon <[email protected]>

---
     drivers/scsi/ufs/ufshcd.c |  114 +++++++++++++++++++++++++++++++++-----------
     drivers/scsi/ufs/ufshcd.h |    6 ++-
     2 files changed, 89 insertions(+), 31 deletions(-)

drivers/scsi/ufs/ufshcd.c +1 -0

index efe2256..76ff332 100644
@@ -38,6 +38,7 @@
     #define UFSHCD_ENABLE_INTRS      (UTP_TRANSFER_REQ_COMPL |\
                               UTP_TASK_REQ_COMPL |\
                               UFSHCD_ERROR_MASK)
+#define UIC_CMD_TIMEOUT      100
 
     enum {
      UFSHCD_MAX_CHANNEL      = 0,

@@ -357,13 +358,15 @@ static inline void ufshcd_hba_capabilities(struct ufs_hba *hba)
     }

Quoted 16 lines

Quoted 8 lines

drivers/scsi/ufs/ufshcd.c +1 -2

     /**
- * ufshcd_send_uic_command - Send UIC commands to unipro layers
+ * ufshcd_dispatch_uic_cmd - Dispatch UIC commands to unipro layers
      * @hba: per adapter instance
      * @uic_command: UIC command
      */
     static inline void
-ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/scsi/ufs/ufshcd.c ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command
Quoted 8 lines
drivers/scsi/ufs/ufshcd.c +3 -0
+ufshcd_dispatch_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmnd)
     {
+     init_completion(&uic_cmnd->done);
+
      /* Write Args */
      ufshcd_writel(hba, REG_UIC_COMMAND_ARG_1, uic_cmnd->argument1);
      ufshcd_writel(hba, REG_UIC_COMMAND_ARG_2, uic_cmnd->argument2);
@@ -375,6 +378,45 @@ ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command

*uic_cmnd)

Quoted 128 lines

Quoted 81 lines

Quoted 72 lines

drivers/scsi/ufs/ufshcd.c +43 -9

     }
 
     /**
+ * ufshcd_wait_for_uic_cmd - Wait complectioin of UIC command
+ * @hba: per adapter instance
+ * @uic_command: UIC command
+ *
+ * Returns 0 only if success.
+ */
+static int ufshcd_wait_for_uic_cmd(struct ufs_hba *hba)
+{
+     struct uic_command *uic_cmd = &hba->active_uic_cmd;
+     int ret;
+
+     if (wait_for_completion_timeout(&uic_cmd->done,
+                                     msecs_to_jiffies(UIC_CMD_TIMEOUT)))
+             ret = ufshcd_get_uic_cmd_result(hba);
+     else
+             ret = -ETIMEDOUT;
+
+     return ret;
+}
+
+/**
+ * ufshcd_ready_uic_cmd - Check if controller is ready
+ *                        to accept UIC commands
+ * @hba: per adapter instance
+ * Return true on success, else false
+ */
+static inline bool ufshcd_ready_uic_cmd(struct ufs_hba *hba)
+{
+     if (ufshcd_readl(hba, REG_CONTROLLER_STATUS) & UIC_COMMAND_READY) {
+             return true;
+     } else {
+             dev_err(hba->dev,
+                             "Controller not ready"
+                             " to accept UIC commands\n");
+             return false;
+     }
+}
+
+/**
      * ufshcd_map_sg - Map scatter-gather list to prdt
      * @lrbp - pointer to local reference block
      *

@@ -735,15 +777,10 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
     {
      struct uic_command *uic_cmd;
      unsigned long flags;
+     int ret;
 
-     /* check if controller is ready to accept UIC commands */
-     if (((ufshcd_readl(hba, REG_CONTROLLER_STATUS)) &
-         UIC_COMMAND_READY) == 0x0) {
-             dev_err(hba->dev,
-                     "Controller not ready"
-                     " to accept UIC commands\n");
+     if (!ufshcd_ready_uic_cmd(hba))
              return -EIO;
-     }
 
      spin_lock_irqsave(hba->host->host_lock, flags);

@@ -754,13 +791,16 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
      uic_cmd->argument2 = 0;
      uic_cmd->argument3 = 0;
 
-     /* enable UIC related interrupts */
-     ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
+     /* Dispatching UIC commands to controller */
+     ufshcd_dispatch_uic_cmd(hba, uic_cmd);

Quoted 9 lines

Quoted 6 lines
Quoted 6 lines
drivers/scsi/ufs/ufshcd.c static int ufshcd_dme_link_startup(struct ufs_hba *hba) · line 799
Quoted 6 lines
drivers/scsi/ufs/ufshcd.c +2 -3
-     /* sending UIC commands to controller */
-     ufshcd_send_uic_command(hba, uic_cmd);
      spin_unlock_irqrestore(hba->host->host_lock, flags);
-     return 0;
+
+     ret = ufshcd_wait_for_uic_cmd(hba);
Error code is incorrect. only -ETIMEDOUT is valid others are just DME errors.

Quoted 4 lines

Only success returns '0', other positive value from dme and -ETIMEDOUT mean failure. Error code can be reused purely, not being redefined.
I am seeing that -EINVAL represents from 01h to 07h in your handling. It looks like error's detail is disappear. Exact return might be needed from DME.

okay.

Quoted 6 lines

Quoted 5 lines
Also, spec. clearly mentions a retry mechanism which means that there could be some timing issues anticipated where the UIC layer cannot respond properly.
Sorry, I didn't catch your meaning fully. Where can I refer to it?

I meant the same retry mechanism mentioned in the section 7.2.1 (Host Controller Initialization) of JESD223A UFS HCI v1.1.

Quoted 31 lines

Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
drivers/scsi/ufs/ufshcd.c +4 -0
+     if (ret)
+             dev_err(hba->dev, "link startup: error code %d returned\n", ret);
+
+     return ret;
     }
 
     /**
@@ -898,6 +938,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
      if (ufshcd_hba_enable(hba))
              return -EIO;
 
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
drivers/scsi/ufs/ufshcd.c static int ufshcd_initialize_hba(struct ufs_hba *hba) · line 942
Quoted 2 lines
drivers/scsi/ufs/ufshcd.c +2 -0
+     /* enable UIC related interrupts */
+     ufshcd_enable_intr(hba, UIC_COMMAND_COMPL | UIC_ERROR);
The recovery when UIC_ERROR happens is broken because of re-entrancy to dme_link_startup from ufshcd_fatal_err_handler(). So better handle with timeout than allowing controller to raise a UIC_ERROR until that is fixed?
I also recognize error handling should be done further. Ok, I agree with you.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/scsi/ufs/ufshcd.c +1 -0
+
      /* Configure UTRL and UTMRL base address registers */
      ufshcd_writel(hba, REG_UTP_TRANSFER_REQ_LIST_BASE_L,
                    lower_32_bits(hba->utrdl_dma_addr));
@@ -909,7 +952,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
                    upper_32_bits(hba->utmrdl_dma_addr));
 

Quoted 11 lines

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/scsi/ufs/ufshcd.c static int ufshcd_initialize_hba(struct ufs_hba *hba) · line 958
Quoted 6 lines
drivers/scsi/ufs/ufshcd.c +3 -1
      /* Initialize unipro link startup procedure */
-     return ufshcd_dme_link_startup(hba);
+     schedule_work(&hba->link_startup_wq);
+
+     return 0;
     }
With this approach, always success is returned to the probe irrespective of link-startup result.
Also if the link-startup fails there is no mechanism to retry the procedure. So it is better to combine it with Sujith's patch.

As it has been discussed with Sujith above, retry could be handled via IS.ULLS.

Thanks,
Seungwon Jeon

Quoted 173 lines

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/scsi/ufs/ufshcd.c ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb
Quoted 4 lines
drivers/scsi/ufs/ufshcd.c +0 -0
     /**
@@ -1186,6 +1231,16 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb
*lrbp)
Quoted 96 lines
Quoted 96 lines
Quoted 95 lines
Quoted 85 lines
Quoted 80 lines
Quoted 11 lines
Quoted 9 lines
Quoted 9 lines
drivers/scsi/ufs/ufshcd.c ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb · line 1239
Quoted 9 lines
drivers/scsi/ufs/ufshcd.c +6 -0
     }
 
     /**
+ * ufshcd_uic_cmd_compl - handle completion of uic command
+ * @hba: per adapter instance
+ */
+static void ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
+{
+     if (intr_status & UIC_COMMAND_COMPL)
why this redundant check if it is already checked in ufshcd_sl_intr()?
Yes, it's currently not needed.
It will be used to identify several uic command. ([PATCH 5/5] scsi: ufs: add dme operations) Anyway, it's better to be removed here.
Quoted 59 lines
Quoted 59 lines
Quoted 59 lines
Quoted 59 lines
drivers/scsi/ufs/ufshcd.c +18 ~~-13~~
+             complete(&hba->active_uic_cmd.done);
+}
+
+/**
      * ufshcd_transfer_req_compl - handle SCSI and query command completion
      * @hba: per adapter instance
      */
@@ -1225,25 +1280,26 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
     }
 
     /**
- * ufshcd_uic_cc_handler - handle UIC command completion
+ * ufshcd_link_startup - link initialization
      * @work: pointer to a work queue structure
- *
- * Returns 0 on success, non-zero value on failure
      */
-static void ufshcd_uic_cc_handler (struct work_struct *work)
+static void ufshcd_link_startup(struct work_struct *work)
     {
      struct ufs_hba *hba;
+     int ret;
 
-     hba = container_of(work, struct ufs_hba, uic_workq);
+     hba = container_of(work, struct ufs_hba, link_startup_wq);
 
-     if ((hba->active_uic_cmd.command == UIC_CMD_DME_LINK_STARTUP) &&
-         !(ufshcd_get_uic_cmd_result(hba))) {
+     ret = ufshcd_dme_link_startup(hba);
+     if (ret)
+             goto out;
 
-             if (ufshcd_make_hba_operational(hba))
-                     dev_err(hba->dev,
-                             "cc: hba not operational state\n");
-             return;
-     }
+     ret = ufshcd_make_hba_operational(hba);
+     if (ret)
+             goto out;
+     return;
+out:
+     dev_err(hba->dev, "link startup failed %d\n", ret);
     }
 
     /**
@@ -1307,7 +1363,7 @@ static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
              ufshcd_err_handler(hba);
 
      if (intr_status & UIC_COMMAND_COMPL)
-             schedule_work(&hba->uic_workq);
+             ufshcd_uic_cmd_compl(hba, intr_status);
 
      if (intr_status & UTP_TASK_REQ_COMPL)
              ufshcd_tmc_handler(hba);
@@ -1694,7 +1750,7 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
      init_waitqueue_head(&hba->ufshcd_tm_wait_queue);
 
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
drivers/scsi/ufs/ufshcd.c int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle, · line 1753
Quoted 3 lines
drivers/scsi/ufs/ufshcd.c +1 -1
      /* Initialize work queues */
-     INIT_WORK(&hba->uic_workq, ufshcd_uic_cc_handler);
+     INIT_WORK(&hba->link_startup_wq, ufshcd_link_startup);
Can we use async function calls kernel/async.c instead of having work queues as this is only used during boot up?
Quoted 4 lines
As we know, both probe and resume are sensitive to execution time. I guess link startup procedure will also be activated in driver's resume. Do you have any specific reason for async function?
Quoted 6 lines
I guess most UFS devices currently are embedded and have rootfs on them. If we use schedule_work there is no synchronization mechanism to check whether the device link startup is completed and device is available for the userspace to mount the partitions. Having async mechanism in place, the prepare_namespace() does wait for such async probes to be completed before mounting the rootfs.
I understand your meaning.
If you are considering that, I think 'scsi_scan_host' has a role for that. 'scsi_scan_host' will be the conclusion and be done in async subsystem. 'scsi_scan_host' is actually called, after finishing link startup procedure.
Quoted 5 lines
Quoted 3 lines
with this patch the scsi_scan_host() is called after the work is scheduled. Which means that link_startup_wq might be scheduled after prepare_namespace().
AFAIK, the work is scheduled immediately. So I think we don't need to worry about that. I guess you want to gather all of initialization sequence into async mechanism including link
startup.
Quoted 5 lines
If your intention is for that reason, it's okay to use async_schedule. Then, we may reuse these in driver's resume.
I should have answered quickly, but I have a day off yesterday. If you have any opinion, please let me know.
Quoted 52 lines
Thanks,
Seungwon Jeon
Quoted 49 lines
Quoted 9 lines
Quoted 9 lines
I agree that the resume is sensitive to execute time but during resume you can't schedule link startup work because the fs/block/scsi layer expects the device availability as soon as resume operation is completed. So ufshcd_link_startup() should be called in the resume context itself or implement a synchronization mechanism like blocking scsi layer queuing requests until link startup is completed.
Quoted 32 lines
'scsi_block_requests' and 'scsi_unblock_requests' can be used during suspend/resume. After the link startup is finished and host is ready, 'scsi_unblock_requests' will be called.
Thanks,
Seungwon Jeon
Quoted 27 lines
Would following implementation looks better?
ufshcd_async_probe()
{
...
    ufshcd_link_startup(hba);
...
}
ufshcd_resume()
{
...
ufshcd_link_startup(hba);
...
}
ufshcd_init()
{
...
async_schedule(ufshcd_async_probe, hba);
...
}
--
--
Regards,
Sujit
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
~Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Subhash Jadavani

2013-05-02 11:46 UTC #11 lore raw

In reply to Seungwon Jeon

Few minor comments other than what Sujit/Santosh have already commented.

Quoted 17 lines · Seungwon Jeon

Link start-up requires long time with multiphase handshakes between UFS host and device. This affects driver's probe time. This patch let link start-up run asynchronously.
And completion time of uic command is defined to avoid a permanent wait.
Signed-off-by Seungwon Jeon <[email protected]>
---
  drivers/scsi/ufs/ufshcd.c |  114 +++++++++++++++++++++++++++++++++-----------
  drivers/scsi/ufs/ufshcd.h |    6 ++-
  2 files changed, 89 insertions(+), 31 deletions(-)
drivers/scsi/ufs/ufshcd.c +0 -0
index efe2256..76ff332 100644

drivers/scsi/ufs/ufshcd.c line 41

Quoted 4 lines

drivers/scsi/ufs/ufshcd.c +1 -0

@@ -38,6 +38,7 @@
  #define UFSHCD_ENABLE_INTRS	(UTP_TRANSFER_REQ_COMPL |\
  				 UTP_TASK_REQ_COMPL |\
  				 UFSHCD_ERROR_MASK)
+#define UIC_CMD_TIMEOUT	100

What is the timeout unit here (us/ms/sec)? Please add the same in the comment.

Quoted 42 lines

drivers/scsi/ufs/ufshcd.c +21 -2

  enum {
  	UFSHCD_MAX_CHANNEL	= 0,

@@ -357,13 +358,15 @@ static inline void ufshcd_hba_capabilities(struct ufs_hba *hba)
  }
 
  /**
- * ufshcd_send_uic_command - Send UIC commands to unipro layers
+ * ufshcd_dispatch_uic_cmd - Dispatch UIC commands to unipro layers
   * @hba: per adapter instance
   * @uic_command: UIC command
   */
  static inline void
-ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
+ufshcd_dispatch_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmnd)
  {
+	init_completion(&uic_cmnd->done);
+
  	/* Write Args */
  	ufshcd_writel(hba, REG_UIC_COMMAND_ARG_1, uic_cmnd->argument1);
  	ufshcd_writel(hba, REG_UIC_COMMAND_ARG_2, uic_cmnd->argument2);

@@ -375,6 +378,45 @@ ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
  }
 
  /**
+ * ufshcd_wait_for_uic_cmd - Wait complectioin of UIC command
+ * @hba: per adapter instance
+ * @uic_command: UIC command
+ *
+ * Returns 0 only if success.
+ */
+static int ufshcd_wait_for_uic_cmd(struct ufs_hba *hba)
+{
+	struct uic_command *uic_cmd = &hba->active_uic_cmd;
+	int ret;
+
+	if (wait_for_completion_timeout(&uic_cmd->done,
+					msecs_to_jiffies(UIC_CMD_TIMEOUT)))
+		ret = ufshcd_get_uic_cmd_result(hba);
+	else
+		ret = -ETIMEDOUT;
+

drivers/scsi/ufs/ufshcd.c ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd) · line 407

Quoted 10 lines

drivers/scsi/ufs/ufshcd.c +10 -0

+	return ret;
+}
+
+/**
+ * ufshcd_ready_uic_cmd - Check if controller is ready
+ *                        to accept UIC commands
+ * @hba: per adapter instance
+ * Return true on success, else false
+ */
+static inline bool ufshcd_ready_uic_cmd(struct ufs_hba *hba)

I guess this might sound more readable: s/ufshcd_ready_uic_cmd/ufshcd_ready_for_uic_cmd

drivers/scsi/ufs/ufshcd.c ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd) · line 414

Quoted 7 lines

drivers/scsi/ufs/ufshcd.c +7 -0

+{
+	if (ufshcd_readl(hba, REG_CONTROLLER_STATUS) & UIC_COMMAND_READY) {
+		return true;
+	} else {
+		dev_err(hba->dev,
+				"Controller not ready"
+				" to accept UIC commands\n");

Please have the full string on the same line even if it exceeeds 80 characters limit per line.

Quoted 181 lines

drivers/scsi/ufs/ufshcd.c +46 ~~-27~~

+		return false;
+	}
+}
+
+/**
   * ufshcd_map_sg - Map scatter-gather list to prdt
   * @lrbp - pointer to local reference block
   *

@@ -735,15 +777,10 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
  {
  	struct uic_command *uic_cmd;
  	unsigned long flags;
+	int ret;
 
-	/* check if controller is ready to accept UIC commands */
-	if (((ufshcd_readl(hba, REG_CONTROLLER_STATUS)) &
-	    UIC_COMMAND_READY) == 0x0) {
-		dev_err(hba->dev,
-			"Controller not ready"
-			" to accept UIC commands\n");
+	if (!ufshcd_ready_uic_cmd(hba))
  		return -EIO;
-	}
 
  	spin_lock_irqsave(hba->host->host_lock, flags);

@@ -754,13 +791,16 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
  	uic_cmd->argument2 = 0;
  	uic_cmd->argument3 = 0;
 
-	/* enable UIC related interrupts */
-	ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
+	/* Dispatching UIC commands to controller */
+	ufshcd_dispatch_uic_cmd(hba, uic_cmd);
 
-	/* sending UIC commands to controller */
-	ufshcd_send_uic_command(hba, uic_cmd);
  	spin_unlock_irqrestore(hba->host->host_lock, flags);
-	return 0;
+
+	ret = ufshcd_wait_for_uic_cmd(hba);
+	if (ret)
+		dev_err(hba->dev, "link startup: error code %d returned\n", ret);
+
+	return ret;
  }
 
  /**

@@ -898,6 +938,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
  	if (ufshcd_hba_enable(hba))
  		return -EIO;
 
+	/* enable UIC related interrupts */
+	ufshcd_enable_intr(hba, UIC_COMMAND_COMPL | UIC_ERROR);
+
  	/* Configure UTRL and UTMRL base address registers */
  	ufshcd_writel(hba, REG_UTP_TRANSFER_REQ_LIST_BASE_L,
  		      lower_32_bits(hba->utrdl_dma_addr));

@@ -909,7 +952,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
  		      upper_32_bits(hba->utmrdl_dma_addr));
 
  	/* Initialize unipro link startup procedure */
-	return ufshcd_dme_link_startup(hba);
+	schedule_work(&hba->link_startup_wq);
+
+	return 0;
  }
 
  /**

@@ -1186,6 +1231,16 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
  }
 
  /**
+ * ufshcd_uic_cmd_compl - handle completion of uic command
+ * @hba: per adapter instance
+ */
+static void ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
+{
+	if (intr_status & UIC_COMMAND_COMPL)
+		complete(&hba->active_uic_cmd.done);
+}
+
+/**
   * ufshcd_transfer_req_compl - handle SCSI and query command completion
   * @hba: per adapter instance
   */

@@ -1225,25 +1280,26 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
  }
 
  /**
- * ufshcd_uic_cc_handler - handle UIC command completion
+ * ufshcd_link_startup - link initialization
   * @work: pointer to a work queue structure
- *
- * Returns 0 on success, non-zero value on failure
   */
-static void ufshcd_uic_cc_handler (struct work_struct *work)
+static void ufshcd_link_startup(struct work_struct *work)
  {
  	struct ufs_hba *hba;
+	int ret;
 
-	hba = container_of(work, struct ufs_hba, uic_workq);
+	hba = container_of(work, struct ufs_hba, link_startup_wq);
 
-	if ((hba->active_uic_cmd.command == UIC_CMD_DME_LINK_STARTUP) &&
-	    !(ufshcd_get_uic_cmd_result(hba))) {
+	ret = ufshcd_dme_link_startup(hba);
+	if (ret)
+		goto out;
 
-		if (ufshcd_make_hba_operational(hba))
-			dev_err(hba->dev,
-				"cc: hba not operational state\n");
-		return;
-	}
+	ret = ufshcd_make_hba_operational(hba);
+	if (ret)
+		goto out;
+	return;
+out:
+	dev_err(hba->dev, "link startup failed %d\n", ret);
  }
 
  /**

@@ -1307,7 +1363,7 @@ static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
  		ufshcd_err_handler(hba);
 
  	if (intr_status & UIC_COMMAND_COMPL)
-		schedule_work(&hba->uic_workq);
+		ufshcd_uic_cmd_compl(hba, intr_status);
 
  	if (intr_status & UTP_TASK_REQ_COMPL)
  		ufshcd_tmc_handler(hba);

@@ -1694,7 +1750,7 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
  	init_waitqueue_head(&hba->ufshcd_tm_wait_queue);
 
  	/* Initialize work queues */
-	INIT_WORK(&hba->uic_workq, ufshcd_uic_cc_handler);
+	INIT_WORK(&hba->link_startup_wq, ufshcd_link_startup);
  	INIT_WORK(&hba->feh_workq, ufshcd_fatal_err_handler);
 
  	/* IRQ registration */

drivers/scsi/ufs/ufshcd.h +4 -2

index 87d5a94..2fb4d94 100644
@@ -51,6 +51,7 @@
  #include <linux/bitops.h>
  #include <linux/pm_runtime.h>
  #include <linux/clk.h>
+#include <linux/completion.h>
 
  #include <asm/irq.h>
  #include <asm/byteorder.h>

@@ -83,6 +84,7 @@ struct uic_command {
  	u32 argument3;
  	int cmd_active;
  	int result;
+	struct completion done;
  };
 
  /**

@@ -140,7 +142,7 @@ struct ufshcd_lrb {
   * @tm_condition: condition variable for task management
   * @ufshcd_state: UFSHCD states
   * @intr_mask: Interrupt Mask Bits
- * @uic_workq: Work queue for UIC completion handling
+ * @link_startup_wq: Work queue for link start-up
   * @feh_workq: Work queue for fatal controller error handling
   * @errors: HBA errors
   */

@@ -179,7 +181,7 @@ struct ufs_hba {
  	u32 intr_mask;
 
  	/* Work Queues */
-	struct work_struct uic_workq;
+	struct work_struct link_startup_wq;
  	struct work_struct feh_workq;
 
  	/* HBA Errors */

Seungwon Jeon

author

2013-05-02 13:38 UTC #12 lore raw

Quoted 1 line · Subhash Jadavani

Few minor comments other than what Sujit/Santosh have already commented.

I'll take your comments for next send.

Thanks,
Seungwon Jeon

Quoted 277 lines · Seungwon Jeon

Quoted 17 lines

Link start-up requires long time with multiphase handshakes between UFS host and device. This affects driver's probe time. This patch let link start-up run asynchronously.
And completion time of uic command is defined to avoid a permanent wait.
Signed-off-by Seungwon Jeon <[email protected]>
---
  drivers/scsi/ufs/ufshcd.c |  114 +++++++++++++++++++++++++++++++++-----------
  drivers/scsi/ufs/ufshcd.h |    6 ++-
  2 files changed, 89 insertions(+), 31 deletions(-)
drivers/scsi/ufs/ufshcd.c +0 -0
index efe2256..76ff332 100644

drivers/scsi/ufs/ufshcd.c line 41

Quoted 4 lines

drivers/scsi/ufs/ufshcd.c +1 -0

@@ -38,6 +38,7 @@
  #define UFSHCD_ENABLE_INTRS	(UTP_TRANSFER_REQ_COMPL |\
  				 UTP_TASK_REQ_COMPL |\
  				 UFSHCD_ERROR_MASK)
+#define UIC_CMD_TIMEOUT	100

What is the timeout unit here (us/ms/sec)? Please add the same in the comment.

Quoted 42 lines

drivers/scsi/ufs/ufshcd.c +21 -2

  enum {
  	UFSHCD_MAX_CHANNEL	= 0,

@@ -357,13 +358,15 @@ static inline void ufshcd_hba_capabilities(struct ufs_hba *hba)
  }
 
  /**
- * ufshcd_send_uic_command - Send UIC commands to unipro layers
+ * ufshcd_dispatch_uic_cmd - Dispatch UIC commands to unipro layers
   * @hba: per adapter instance
   * @uic_command: UIC command
   */
  static inline void
-ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
+ufshcd_dispatch_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmnd)
  {
+	init_completion(&uic_cmnd->done);
+
  	/* Write Args */
  	ufshcd_writel(hba, REG_UIC_COMMAND_ARG_1, uic_cmnd->argument1);
  	ufshcd_writel(hba, REG_UIC_COMMAND_ARG_2, uic_cmnd->argument2);

@@ -375,6 +378,45 @@ ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
  }
 
  /**
+ * ufshcd_wait_for_uic_cmd - Wait complectioin of UIC command
+ * @hba: per adapter instance
+ * @uic_command: UIC command
+ *
+ * Returns 0 only if success.
+ */
+static int ufshcd_wait_for_uic_cmd(struct ufs_hba *hba)
+{
+	struct uic_command *uic_cmd = &hba->active_uic_cmd;
+	int ret;
+
+	if (wait_for_completion_timeout(&uic_cmd->done,
+					msecs_to_jiffies(UIC_CMD_TIMEOUT)))
+		ret = ufshcd_get_uic_cmd_result(hba);
+	else
+		ret = -ETIMEDOUT;
+

drivers/scsi/ufs/ufshcd.c ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd) · line 407

Quoted 10 lines

drivers/scsi/ufs/ufshcd.c +10 -0

+	return ret;
+}
+
+/**
+ * ufshcd_ready_uic_cmd - Check if controller is ready
+ *                        to accept UIC commands
+ * @hba: per adapter instance
+ * Return true on success, else false
+ */
+static inline bool ufshcd_ready_uic_cmd(struct ufs_hba *hba)

I guess this might sound more readable:
s/ufshcd_ready_uic_cmd/ufshcd_ready_for_uic_cmd

drivers/scsi/ufs/ufshcd.c ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd) · line 414

Quoted 7 lines

drivers/scsi/ufs/ufshcd.c +7 -0

+{
+	if (ufshcd_readl(hba, REG_CONTROLLER_STATUS) & UIC_COMMAND_READY) {
+		return true;
+	} else {
+		dev_err(hba->dev,
+				"Controller not ready"
+				" to accept UIC commands\n");

Please have the full string on the same line even if it exceeeds 80 characters limit per line.

Quoted 173 lines

drivers/scsi/ufs/ufshcd.c +46 ~~-27~~

+		return false;
+	}
+}
+
+/**
   * ufshcd_map_sg - Map scatter-gather list to prdt
   * @lrbp - pointer to local reference block
   *

@@ -735,15 +777,10 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
  {
  	struct uic_command *uic_cmd;
  	unsigned long flags;
+	int ret;
 
-	/* check if controller is ready to accept UIC commands */
-	if (((ufshcd_readl(hba, REG_CONTROLLER_STATUS)) &
-	    UIC_COMMAND_READY) == 0x0) {
-		dev_err(hba->dev,
-			"Controller not ready"
-			" to accept UIC commands\n");
+	if (!ufshcd_ready_uic_cmd(hba))
  		return -EIO;
-	}
 
  	spin_lock_irqsave(hba->host->host_lock, flags);

@@ -754,13 +791,16 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
  	uic_cmd->argument2 = 0;
  	uic_cmd->argument3 = 0;
 
-	/* enable UIC related interrupts */
-	ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
+	/* Dispatching UIC commands to controller */
+	ufshcd_dispatch_uic_cmd(hba, uic_cmd);
 
-	/* sending UIC commands to controller */
-	ufshcd_send_uic_command(hba, uic_cmd);
  	spin_unlock_irqrestore(hba->host->host_lock, flags);
-	return 0;
+
+	ret = ufshcd_wait_for_uic_cmd(hba);
+	if (ret)
+		dev_err(hba->dev, "link startup: error code %d returned\n", ret);
+
+	return ret;
  }
 
  /**

@@ -898,6 +938,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
  	if (ufshcd_hba_enable(hba))
  		return -EIO;
 
+	/* enable UIC related interrupts */
+	ufshcd_enable_intr(hba, UIC_COMMAND_COMPL | UIC_ERROR);
+
  	/* Configure UTRL and UTMRL base address registers */
  	ufshcd_writel(hba, REG_UTP_TRANSFER_REQ_LIST_BASE_L,
  		      lower_32_bits(hba->utrdl_dma_addr));

@@ -909,7 +952,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
  		      upper_32_bits(hba->utmrdl_dma_addr));
 
  	/* Initialize unipro link startup procedure */
-	return ufshcd_dme_link_startup(hba);
+	schedule_work(&hba->link_startup_wq);
+
+	return 0;
  }
 
  /**

@@ -1186,6 +1231,16 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
  }
 
  /**
+ * ufshcd_uic_cmd_compl - handle completion of uic command
+ * @hba: per adapter instance
+ */
+static void ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
+{
+	if (intr_status & UIC_COMMAND_COMPL)
+		complete(&hba->active_uic_cmd.done);
+}
+
+/**
   * ufshcd_transfer_req_compl - handle SCSI and query command completion
   * @hba: per adapter instance
   */

@@ -1225,25 +1280,26 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
  }
 
  /**
- * ufshcd_uic_cc_handler - handle UIC command completion
+ * ufshcd_link_startup - link initialization
   * @work: pointer to a work queue structure
- *
- * Returns 0 on success, non-zero value on failure
   */
-static void ufshcd_uic_cc_handler (struct work_struct *work)
+static void ufshcd_link_startup(struct work_struct *work)
  {
  	struct ufs_hba *hba;
+	int ret;
 
-	hba = container_of(work, struct ufs_hba, uic_workq);
+	hba = container_of(work, struct ufs_hba, link_startup_wq);
 
-	if ((hba->active_uic_cmd.command == UIC_CMD_DME_LINK_STARTUP) &&
-	    !(ufshcd_get_uic_cmd_result(hba))) {
+	ret = ufshcd_dme_link_startup(hba);
+	if (ret)
+		goto out;
 
-		if (ufshcd_make_hba_operational(hba))
-			dev_err(hba->dev,
-				"cc: hba not operational state\n");
-		return;
-	}
+	ret = ufshcd_make_hba_operational(hba);
+	if (ret)
+		goto out;
+	return;
+out:
+	dev_err(hba->dev, "link startup failed %d\n", ret);
  }
 
  /**

@@ -1307,7 +1363,7 @@ static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
  		ufshcd_err_handler(hba);
 
  	if (intr_status & UIC_COMMAND_COMPL)
-		schedule_work(&hba->uic_workq);
+		ufshcd_uic_cmd_compl(hba, intr_status);
 
  	if (intr_status & UTP_TASK_REQ_COMPL)
  		ufshcd_tmc_handler(hba);

@@ -1694,7 +1750,7 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
  	init_waitqueue_head(&hba->ufshcd_tm_wait_queue);
 
  	/* Initialize work queues */
-	INIT_WORK(&hba->uic_workq, ufshcd_uic_cc_handler);
+	INIT_WORK(&hba->link_startup_wq, ufshcd_link_startup);
  	INIT_WORK(&hba->feh_workq, ufshcd_fatal_err_handler);
 
  	/* IRQ registration */

drivers/scsi/ufs/ufshcd.h +3 -1

index 87d5a94..2fb4d94 100644
@@ -51,6 +51,7 @@
  #include <linux/bitops.h>
  #include <linux/pm_runtime.h>
  #include <linux/clk.h>
+#include <linux/completion.h>
 
  #include <asm/irq.h>
  #include <asm/byteorder.h>

@@ -83,6 +84,7 @@ struct uic_command {
  	u32 argument3;
  	int cmd_active;
  	int result;
+	struct completion done;
  };
 
  /**

@@ -140,7 +142,7 @@ struct ufshcd_lrb {
   * @tm_condition: condition variable for task management
   * @ufshcd_state: UFSHCD states
   * @intr_mask: Interrupt Mask Bits
- * @uic_workq: Work queue for UIC completion handling
+ * @link_startup_wq: Work queue for link start-up
   * @feh_workq: Work queue for fatal controller error handling
   * @errors: HBA errors
   */

drivers/scsi/ufs/ufshcd.h struct ufs_hba { · line 187

Quoted 8 lines

drivers/scsi/ufs/ufshcd.h +1 -1

@@ -179,7 +181,7 @@ struct ufs_hba {
  	u32 intr_mask;
 
  	/* Work Queues */
-	struct work_struct uic_workq;
+	struct work_struct link_startup_wq;
  	struct work_struct feh_workq;
 
  	/* HBA Errors */

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Seungwon Jeon

author

2013-05-04 08:45 UTC #13 lore raw

PATCH v2 4/7 scsi: ufs: fix interrupt status clears

In reply to Seungwon Jeon

There is no need to check the version to clear
the interrupt status. And the order is changed
prior to actual handling.

Signed-off-by Seungwon Jeon <[email protected]>

---
 drivers/scsi/ufs/ufshcd.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

drivers/scsi/ufs/ufshcd.c +1 -4

index bc956e8..442195c 100644
@@ -1591,11 +1591,8 @@ static irqreturn_t ufshcd_intr(int irq, void *__hba)
 	intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
 
 	if (intr_status) {
+		ufshcd_writel(hba, intr_status, REG_INTERRUPT_STATUS);
 		ufshcd_sl_intr(hba, intr_status);
-
-		/* If UFSHCI 1.0 then clear interrupt status register */
-		if (hba->ufs_version == UFSHCI_VERSION_10)
-			ufshcd_writel(hba, intr_status, REG_INTERRUPT_STATUS);
 		retval = IRQ_HANDLED;
 	}
 	spin_unlock(hba->host->host_lock);

Signature · 1 line

1.7.0.4