[PATCH v2 0/9] Simplify the UFS driver initialization code

[PATCH v2 0/9] Simplify the UFS driver initialization code

[Bart Van Assche]

Hi Martin,

This patch series addresses the following issues in the UFS driver
initialization code:
* The legacy and MCQ scsi_add_host() calls occur in different functions. This
  patch series reduces the number of scsi_add_host() calls from two to one.
* Two functions have a boolean 'init_dev_params' argument. This patch series
  removes that argument from both functions by splitting functions and by
  pushing some function calls from caller into callee.

Please consider this patch series for the next merge window.

Thanks,

Bart.

Changes compared to v1:
 - Fixed a compiler warning reported by the kernel build robot.
 - Improved patch descriptions.

Bart Van Assche (9):
  ufs: core: Introduce ufshcd_add_scsi_host()
  ufs: core: Introduce ufshcd_activate_link()
  ufs: core: Introduce ufshcd_post_device_init()
  ufs: core: Call ufshcd_add_scsi_host() later
  ufs: core: Move the ufshcd_device_init() call
  ufs: core: Move the ufshcd_device_init(hba, true) call
  ufs: core: Expand the ufshcd_device_init(hba, true) call
  ufs: core: Move the MCQ scsi_add_host() call
  ufs: core: Remove the second argument of ufshcd_device_init()

 drivers/ufs/core/ufshcd.c | 269 ++++++++++++++++++++++----------------
 1 file changed, 154 insertions(+), 115 deletions(-)

[PATCH v2 1/9] ufs: core: Introduce ufshcd_add_scsi_host()

[Bart Van Assche]

Move the code for adding a SCSI host and also the code for managing
TMF tags from ufshcd_init() into a new function called
ufshcd_add_scsi_host(). No functionality has been changed.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 84 ++++++++++++++++++++++++---------------
 1 file changed, 53 insertions(+), 31 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 0dd26059f5d7..d29e469c3873 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -10381,6 +10381,56 @@ static const struct blk_mq_ops ufshcd_tmf_ops = {
 	.queue_rq = ufshcd_queue_tmf,
 };
 
+static int ufshcd_add_scsi_host(struct ufs_hba *hba)
+{
+	int err;
+
+	if (!is_mcq_supported(hba)) {
+		err = scsi_add_host(hba->host, hba->dev);
+		if (err) {
+			dev_err(hba->dev, "scsi_add_host failed\n");
+			return err;
+		}
+		hba->scsi_host_added = true;
+	}
+
+	hba->tmf_tag_set = (struct blk_mq_tag_set) {
+		.nr_hw_queues	= 1,
+		.queue_depth	= hba->nutmrs,
+		.ops		= &ufshcd_tmf_ops,
+		.flags		= BLK_MQ_F_NO_SCHED,
+	};
+	err = blk_mq_alloc_tag_set(&hba->tmf_tag_set);
+	if (err < 0)
+		goto remove_scsi_host;
+	hba->tmf_queue = blk_mq_alloc_queue(&hba->tmf_tag_set, NULL, NULL);
+	if (IS_ERR(hba->tmf_queue)) {
+		err = PTR_ERR(hba->tmf_queue);
+		goto free_tmf_tag_set;
+	}
+	hba->tmf_rqs = devm_kcalloc(hba->dev, hba->nutmrs,
+				    sizeof(*hba->tmf_rqs), GFP_KERNEL);
+	if (!hba->tmf_rqs) {
+		err = -ENOMEM;
+		goto free_tmf_queue;
+	}
+
+	return 0;
+
+free_tmf_queue:
+	blk_mq_destroy_queue(hba->tmf_queue);
+	blk_put_queue(hba->tmf_queue);
+
+free_tmf_tag_set:
+	blk_mq_free_tag_set(&hba->tmf_tag_set);
+
+remove_scsi_host:
+	if (hba->scsi_host_added)
+		scsi_remove_host(hba->host);
+
+	return err;
+}
+
 /**
  * ufshcd_init - Driver initialization routine
  * @hba: per-adapter instance
@@ -10514,35 +10564,9 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 		hba->is_irq_enabled = true;
 	}
 
-	if (!is_mcq_supported(hba)) {
-		err = scsi_add_host(host, hba->dev);
-		if (err) {
-			dev_err(hba->dev, "scsi_add_host failed\n");
-			goto out_disable;
-		}
-		hba->scsi_host_added = true;
-	}
-
-	hba->tmf_tag_set = (struct blk_mq_tag_set) {
-		.nr_hw_queues	= 1,
-		.queue_depth	= hba->nutmrs,
-		.ops		= &ufshcd_tmf_ops,
-		.flags		= BLK_MQ_F_NO_SCHED,
-	};
-	err = blk_mq_alloc_tag_set(&hba->tmf_tag_set);
-	if (err < 0)
-		goto out_remove_scsi_host;
-	hba->tmf_queue = blk_mq_alloc_queue(&hba->tmf_tag_set, NULL, NULL);
-	if (IS_ERR(hba->tmf_queue)) {
-		err = PTR_ERR(hba->tmf_queue);
-		goto free_tmf_tag_set;
-	}
-	hba->tmf_rqs = devm_kcalloc(hba->dev, hba->nutmrs,
-				    sizeof(*hba->tmf_rqs), GFP_KERNEL);
-	if (!hba->tmf_rqs) {
-		err = -ENOMEM;
-		goto free_tmf_queue;
-	}
+	err = ufshcd_add_scsi_host(hba);
+	if (err)
+		goto out_disable;
 
 	/* Reset the attached device */
 	ufshcd_device_reset(hba);
@@ -10600,9 +10624,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 free_tmf_queue:
 	blk_mq_destroy_queue(hba->tmf_queue);
 	blk_put_queue(hba->tmf_queue);
-free_tmf_tag_set:
 	blk_mq_free_tag_set(&hba->tmf_tag_set);
-out_remove_scsi_host:
 	if (hba->scsi_host_added)
 		scsi_remove_host(hba->host);
 out_disable:

[PATCH v2 2/9] ufs: core: Introduce ufshcd_activate_link()

[Bart Van Assche]

Prepare for introducing a second caller by moving the code for
activating the link between UFS controller and device into a new
function. No functionality has been changed.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index d29e469c3873..04d94bf5cc2d 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8733,10 +8733,9 @@ static void ufshcd_config_mcq(struct ufs_hba *hba)
 		 hba->nutrs);
 }
 
-static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
+static int ufshcd_activate_link(struct ufs_hba *hba)
 {
 	int ret;
-	struct Scsi_Host *host = hba->host;
 
 	hba->ufshcd_state = UFSHCD_STATE_RESET;
 
@@ -8753,6 +8752,18 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
 	/* UniPro link is active now */
 	ufshcd_set_link_active(hba);
 
+	return 0;
+}
+
+static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
+{
+	struct Scsi_Host *host = hba->host;
+	int ret;
+
+	ret = ufshcd_activate_link(hba);
+	if (ret)
+		return ret;
+
 	/* Reconfigure MCQ upon reset */
 	if (hba->mcq_enabled && !init_dev_params) {
 		ufshcd_config_mcq(hba);

[PATCH v2 3/9] ufs: core: Introduce ufshcd_post_device_init()

[Bart Van Assche]

Prepare for introducing a second caller by moving more code from
ufshcd_device_init() into a new function.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 64 ++++++++++++++++++++++-----------------
 1 file changed, 37 insertions(+), 27 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 04d94bf5cc2d..dcc417d7e19c 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8755,6 +8755,42 @@ static int ufshcd_activate_link(struct ufs_hba *hba)
 	return 0;
 }
 
+static int ufshcd_post_device_init(struct ufs_hba *hba)
+{
+	int ret;
+
+	ufshcd_tune_unipro_params(hba);
+
+	/* UFS device is also active now */
+	ufshcd_set_ufs_dev_active(hba);
+	ufshcd_force_reset_auto_bkops(hba);
+
+	ufshcd_set_timestamp_attr(hba);
+	schedule_delayed_work(&hba->ufs_rtc_update_work,
+			      msecs_to_jiffies(UFS_RTC_UPDATE_INTERVAL_MS));
+
+	if (!hba->max_pwr_info.is_valid)
+		return 0;
+
+	/* Gear up to HS gear. */
+
+	/*
+	 * Set the right value to bRefClkFreq before attempting to
+	 * switch to HS gears.
+	 */
+	if (hba->dev_ref_clk_freq != REF_CLK_FREQ_INVAL)
+		ufshcd_set_dev_ref_clk(hba);
+	ret = ufshcd_config_pwr_mode(hba, &hba->max_pwr_info.info);
+	if (ret) {
+		dev_err(hba->dev,
+			"%s: Failed setting power mode, err = %d\n",
+			__func__, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
 static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
 {
 	struct Scsi_Host *host = hba->host;
@@ -8813,33 +8849,7 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
 		}
 	}
 
-	ufshcd_tune_unipro_params(hba);
-
-	/* UFS device is also active now */
-	ufshcd_set_ufs_dev_active(hba);
-	ufshcd_force_reset_auto_bkops(hba);
-
-	ufshcd_set_timestamp_attr(hba);
-	schedule_delayed_work(&hba->ufs_rtc_update_work,
-			      msecs_to_jiffies(UFS_RTC_UPDATE_INTERVAL_MS));
-
-	/* Gear up to HS gear if supported */
-	if (hba->max_pwr_info.is_valid) {
-		/*
-		 * Set the right value to bRefClkFreq before attempting to
-		 * switch to HS gears.
-		 */
-		if (hba->dev_ref_clk_freq != REF_CLK_FREQ_INVAL)
-			ufshcd_set_dev_ref_clk(hba);
-		ret = ufshcd_config_pwr_mode(hba, &hba->max_pwr_info.info);
-		if (ret) {
-			dev_err(hba->dev, "%s: Failed setting power mode, err = %d\n",
-					__func__, ret);
-			return ret;
-		}
-	}
-
-	return 0;
+	return ufshcd_post_device_init(hba);
 }
 
 /**

[PATCH v2 4/9] ufs: core: Call ufshcd_add_scsi_host() later

[Bart Van Assche]

Call ufshcd_add_scsi_host() after host controller initialization has
completed. This is possible because no code between the old and new
ufshcd_add_scsi_host() call site depends on the scsi_add_host() call.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index dcc417d7e19c..b513ef46d848 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -10585,10 +10585,6 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 		hba->is_irq_enabled = true;
 	}
 
-	err = ufshcd_add_scsi_host(hba);
-	if (err)
-		goto out_disable;
-
 	/* Reset the attached device */
 	ufshcd_device_reset(hba);
 
@@ -10600,7 +10596,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 		dev_err(hba->dev, "Host controller enable failed\n");
 		ufshcd_print_evt_hist(hba);
 		ufshcd_print_host_state(hba);
-		goto free_tmf_queue;
+		goto out_disable;
 	}
 
 	/*
@@ -10635,6 +10631,10 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	 */
 	ufshcd_set_ufs_dev_active(hba);
 
+	err = ufshcd_add_scsi_host(hba);
+	if (err)
+		goto out_disable;
+
 	async_schedule(ufshcd_async_scan, hba);
 	ufs_sysfs_add_nodes(hba->dev);
 
@@ -10642,12 +10642,6 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	ufshcd_pm_qos_init(hba);
 	return 0;
 
-free_tmf_queue:
-	blk_mq_destroy_queue(hba->tmf_queue);
-	blk_put_queue(hba->tmf_queue);
-	blk_mq_free_tag_set(&hba->tmf_tag_set);
-	if (hba->scsi_host_added)
-		scsi_remove_host(hba->host);
 out_disable:
 	hba->is_irq_enabled = false;
 	ufshcd_hba_exit(hba);

[PATCH v2 5/9] ufs: core: Move the ufshcd_device_init() call

[Bart Van Assche]

Move the ufshcd_device_init() call to the ufshcd_probe_hba() callers and
remove the 'init_dev_params' argument of ufshcd_probe_hba(). This change
refactors the code without modifying the behavior of the UFSHCI driver.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index b513ef46d848..5c8751672bc7 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -298,7 +298,8 @@ static int ufshcd_reset_and_restore(struct ufs_hba *hba);
 static int ufshcd_eh_host_reset_handler(struct scsi_cmnd *cmd);
 static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag);
 static void ufshcd_hba_exit(struct ufs_hba *hba);
-static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params);
+static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params);
+static int ufshcd_probe_hba(struct ufs_hba *hba);
 static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on);
 static inline void ufshcd_add_delay_before_dme_cmd(struct ufs_hba *hba);
 static int ufshcd_host_reset_and_restore(struct ufs_hba *hba);
@@ -7717,8 +7718,11 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
 	err = ufshcd_hba_enable(hba);
 
 	/* Establish the link again and restore the device */
-	if (!err)
-		err = ufshcd_probe_hba(hba, false);
+	if (!err) {
+		err = ufshcd_device_init(hba, /*init_dev_params=*/false);
+		if (!err)
+			err = ufshcd_probe_hba(hba);
+	}
 
 	if (err)
 		dev_err(hba->dev, "%s: Host init failed %d\n", __func__, err);
@@ -8855,21 +8859,16 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
 /**
  * ufshcd_probe_hba - probe hba to detect device and initialize it
  * @hba: per-adapter instance
- * @init_dev_params: whether or not to call ufshcd_device_params_init().
  *
  * Execute link-startup and verify device initialization
  *
  * Return: 0 upon success; < 0 upon failure.
  */
-static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
+static int ufshcd_probe_hba(struct ufs_hba *hba)
 {
 	ktime_t start = ktime_get();
 	unsigned long flags;
-	int ret;
-
-	ret = ufshcd_device_init(hba, init_dev_params);
-	if (ret)
-		goto out;
+	int ret = 0;
 
 	if (!hba->pm_op_in_progress &&
 	    (hba->quirks & UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH)) {
@@ -8887,7 +8886,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
 		}
 
 		/* Reinit the device */
-		ret = ufshcd_device_init(hba, init_dev_params);
+		ret = ufshcd_device_init(hba, /*init_dev_params=*/false);
 		if (ret)
 			goto out;
 	}
@@ -8935,7 +8934,9 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
 
 	down(&hba->host_sem);
 	/* Initialize hba, detect and initialize UFS device */
-	ret = ufshcd_probe_hba(hba, true);
+	ret = ufshcd_device_init(hba, /*init_dev_params=*/true);
+	if (ret == 0)
+		ret = ufshcd_probe_hba(hba);
 	up(&hba->host_sem);
 	if (ret)
 		goto out;

[PATCH v2 6/9] ufs: core: Move the ufshcd_device_init(hba, true) call

[Bart Van Assche]

Move the ufshcd_device_init(hba, true) call from ufshcd_async_scan()
into ufshcd_init(). This patch prepares for moving both scsi_add_host()
calls into ufshcd_add_scsi_host(). Calling ufshcd_device_init() from
ufshcd_init() without holding hba->host_sem is safe because
hba->host_sem serializes core code with sysfs callback code and because
the ufshcd_device_init() is moved before the scsi_add_host() call.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 5c8751672bc7..0fdf19889191 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8933,10 +8933,7 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
 	int ret;
 
 	down(&hba->host_sem);
-	/* Initialize hba, detect and initialize UFS device */
-	ret = ufshcd_device_init(hba, /*init_dev_params=*/true);
-	if (ret == 0)
-		ret = ufshcd_probe_hba(hba);
+	ret = ufshcd_probe_hba(hba);
 	up(&hba->host_sem);
 	if (ret)
 		goto out;
@@ -10632,6 +10629,11 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	 */
 	ufshcd_set_ufs_dev_active(hba);
 
+	/* Initialize hba, detect and initialize UFS device */
+	err = ufshcd_device_init(hba, /*init_dev_params=*/true);
+	if (err)
+		goto out_disable;
+
 	err = ufshcd_add_scsi_host(hba);
 	if (err)
 		goto out_disable;

[PATCH v2 7/9] ufs: core: Expand the ufshcd_device_init(hba, true) call

[Bart Van Assche]

Expand the ufshcd_device_init(hba, true) call and remove all code that
depends on init_dev_params == false.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 44 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 42 insertions(+), 2 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 0fdf19889191..6a3873991d2a 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -10629,8 +10629,48 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	 */
 	ufshcd_set_ufs_dev_active(hba);
 
-	/* Initialize hba, detect and initialize UFS device */
-	err = ufshcd_device_init(hba, /*init_dev_params=*/true);
+	err = ufshcd_activate_link(hba);
+	if (err)
+		goto out_disable;
+
+	/* Verify device initialization by sending NOP OUT UPIU. */
+	err = ufshcd_verify_dev_init(hba);
+	if (err)
+		goto out_disable;
+
+	/* Initiate UFS initialization and waiting for completion. */
+	err = ufshcd_complete_dev_init(hba);
+	if (err)
+		goto out_disable;
+
+	/*
+	 * Initialize UFS device parameters used by driver, these
+	 * parameters are associated with UFS descriptors.
+	 */
+	err = ufshcd_device_params_init(hba);
+	if (err)
+		goto out_disable;
+	if (is_mcq_supported(hba)) {
+		ufshcd_mcq_enable(hba);
+		err = ufshcd_alloc_mcq(hba);
+		if (!err) {
+			ufshcd_config_mcq(hba);
+		} else {
+			/* Continue with SDB mode */
+			ufshcd_mcq_disable(hba);
+			use_mcq_mode = false;
+			dev_err(hba->dev, "MCQ mode is disabled, err=%d\n",
+				err);
+		}
+		err = scsi_add_host(host, hba->dev);
+		if (err) {
+			dev_err(hba->dev, "scsi_add_host failed\n");
+			goto out_disable;
+		}
+		hba->scsi_host_added = true;
+	}
+
+	err = ufshcd_post_device_init(hba);
 	if (err)
 		goto out_disable;
 

[PATCH v2 8/9] ufs: core: Move the MCQ scsi_add_host() call

[Bart Van Assche]

Whether or not MCQ is used, call scsi_add_host from ufshcd_add_scsi_host().
For MCQ this patch swaps the order of the scsi_add_host() and
ufshcd_post_device_init() calls. This patch also prepares for moving
both scsi_add_host() calls into ufshcd_add_scsi_host().

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 43 ++++++++++++++++-----------------------
 1 file changed, 18 insertions(+), 25 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 6a3873991d2a..b9aaa3c55d17 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -10404,15 +10404,27 @@ static int ufshcd_add_scsi_host(struct ufs_hba *hba)
 {
 	int err;
 
-	if (!is_mcq_supported(hba)) {
-		err = scsi_add_host(hba->host, hba->dev);
-		if (err) {
-			dev_err(hba->dev, "scsi_add_host failed\n");
-			return err;
+	if (is_mcq_supported(hba)) {
+		ufshcd_mcq_enable(hba);
+		err = ufshcd_alloc_mcq(hba);
+		if (!err) {
+			ufshcd_config_mcq(hba);
+		} else {
+			/* Continue with SDB mode */
+			ufshcd_mcq_disable(hba);
+			use_mcq_mode = false;
+			dev_err(hba->dev, "MCQ mode is disabled, err=%d\n",
+				err);
 		}
-		hba->scsi_host_added = true;
 	}
 
+	err = scsi_add_host(hba->host, hba->dev);
+	if (err) {
+		dev_err(hba->dev, "scsi_add_host failed\n");
+		return err;
+	}
+	hba->scsi_host_added = true;
+
 	hba->tmf_tag_set = (struct blk_mq_tag_set) {
 		.nr_hw_queues	= 1,
 		.queue_depth	= hba->nutmrs,
@@ -10650,25 +10662,6 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	err = ufshcd_device_params_init(hba);
 	if (err)
 		goto out_disable;
-	if (is_mcq_supported(hba)) {
-		ufshcd_mcq_enable(hba);
-		err = ufshcd_alloc_mcq(hba);
-		if (!err) {
-			ufshcd_config_mcq(hba);
-		} else {
-			/* Continue with SDB mode */
-			ufshcd_mcq_disable(hba);
-			use_mcq_mode = false;
-			dev_err(hba->dev, "MCQ mode is disabled, err=%d\n",
-				err);
-		}
-		err = scsi_add_host(host, hba->dev);
-		if (err) {
-			dev_err(hba->dev, "scsi_add_host failed\n");
-			goto out_disable;
-		}
-		hba->scsi_host_added = true;
-	}
 
 	err = ufshcd_post_device_init(hba);
 	if (err)

[PATCH v2 9/9] ufs: core: Remove the second argument of ufshcd_device_init()

[Bart Van Assche]

Both ufshcd_device_init() callers pass 'false' as second argument. Hence,
remove that second argument.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 44 +++++----------------------------------
 1 file changed, 5 insertions(+), 39 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index b9aaa3c55d17..9091e5939fac 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -298,7 +298,7 @@ static int ufshcd_reset_and_restore(struct ufs_hba *hba);
 static int ufshcd_eh_host_reset_handler(struct scsi_cmnd *cmd);
 static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag);
 static void ufshcd_hba_exit(struct ufs_hba *hba);
-static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params);
+static int ufshcd_device_init(struct ufs_hba *hba);
 static int ufshcd_probe_hba(struct ufs_hba *hba);
 static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on);
 static inline void ufshcd_add_delay_before_dme_cmd(struct ufs_hba *hba);
@@ -7719,7 +7719,7 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
 
 	/* Establish the link again and restore the device */
 	if (!err) {
-		err = ufshcd_device_init(hba, /*init_dev_params=*/false);
+		err = ufshcd_device_init(hba);
 		if (!err)
 			err = ufshcd_probe_hba(hba);
 	}
@@ -8795,9 +8795,8 @@ static int ufshcd_post_device_init(struct ufs_hba *hba)
 	return 0;
 }
 
-static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
+static int ufshcd_device_init(struct ufs_hba *hba)
 {
-	struct Scsi_Host *host = hba->host;
 	int ret;
 
 	ret = ufshcd_activate_link(hba);
@@ -8805,7 +8804,7 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
 		return ret;
 
 	/* Reconfigure MCQ upon reset */
-	if (hba->mcq_enabled && !init_dev_params) {
+	if (hba->mcq_enabled) {
 		ufshcd_config_mcq(hba);
 		ufshcd_mcq_enable(hba);
 	}
@@ -8820,39 +8819,6 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
 	if (ret)
 		return ret;
 
-	/*
-	 * Initialize UFS device parameters used by driver, these
-	 * parameters are associated with UFS descriptors.
-	 */
-	if (init_dev_params) {
-		ret = ufshcd_device_params_init(hba);
-		if (ret)
-			return ret;
-		if (is_mcq_supported(hba) && !hba->scsi_host_added) {
-			ufshcd_mcq_enable(hba);
-			ret = ufshcd_alloc_mcq(hba);
-			if (!ret) {
-				ufshcd_config_mcq(hba);
-			} else {
-				/* Continue with SDB mode */
-				ufshcd_mcq_disable(hba);
-				use_mcq_mode = false;
-				dev_err(hba->dev, "MCQ mode is disabled, err=%d\n",
-					 ret);
-			}
-			ret = scsi_add_host(host, hba->dev);
-			if (ret) {
-				dev_err(hba->dev, "scsi_add_host failed\n");
-				return ret;
-			}
-			hba->scsi_host_added = true;
-		} else if (is_mcq_supported(hba)) {
-			/* UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH is set */
-			ufshcd_config_mcq(hba);
-			ufshcd_mcq_enable(hba);
-		}
-	}
-
 	return ufshcd_post_device_init(hba);
 }
 
@@ -8886,7 +8852,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
 		}
 
 		/* Reinit the device */
-		ret = ufshcd_device_init(hba, /*init_dev_params=*/false);
+		ret = ufshcd_device_init(hba);
 		if (ret)
 			goto out;
 	}

RE: [PATCH v2 1/9] ufs: core: Introduce ufshcd_add_scsi_host()

[Avri Altman]

> Move the code for adding a SCSI host and also the code for managing TMF tags
> from ufshcd_init() into a new function called ufshcd_add_scsi_host(). No
> functionality has been changed.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Avri Altman <avri.altman@wdc.com>

RE: [PATCH v2 2/9] ufs: core: Introduce ufshcd_activate_link()

[Avri Altman]

> Prepare for introducing a second caller by moving the code for activating the link
> between UFS controller and device into a new function. No functionality has been
> changed.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Avri Altman <avri.altman@wdc.com>

RE: [PATCH v2 3/9] ufs: core: Introduce ufshcd_post_device_init()

[Avri Altman]

> Prepare for introducing a second caller by moving more code from
> ufshcd_device_init() into a new function.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Avri Altman <avri.altman@wdc.com>

Re: [PATCH v2 1/9] ufs: core: Introduce ufshcd_add_scsi_host()

[Manivannan Sadhasivam]

On Thu, Aug 22, 2024 at 02:36:02PM -0700, Bart Van Assche wrote:
> Move the code for adding a SCSI host and also the code for managing
> TMF tags from ufshcd_init() into a new function called
> ufshcd_add_scsi_host(). No functionality has been changed.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/ufs/core/ufshcd.c | 84 ++++++++++++++++++++++++---------------
>  1 file changed, 53 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 0dd26059f5d7..d29e469c3873 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -10381,6 +10381,56 @@ static const struct blk_mq_ops ufshcd_tmf_ops = {
>  	.queue_rq = ufshcd_queue_tmf,
>  };
>  
> +static int ufshcd_add_scsi_host(struct ufs_hba *hba)
> +{
> +	int err;
> +
> +	if (!is_mcq_supported(hba)) {
> +		err = scsi_add_host(hba->host, hba->dev);
> +		if (err) {
> +			dev_err(hba->dev, "scsi_add_host failed\n");
> +			return err;
> +		}
> +		hba->scsi_host_added = true;
> +	}
> +
> +	hba->tmf_tag_set = (struct blk_mq_tag_set) {
> +		.nr_hw_queues	= 1,
> +		.queue_depth	= hba->nutmrs,
> +		.ops		= &ufshcd_tmf_ops,
> +		.flags		= BLK_MQ_F_NO_SCHED,
> +	};
> +	err = blk_mq_alloc_tag_set(&hba->tmf_tag_set);
> +	if (err < 0)
> +		goto remove_scsi_host;
> +	hba->tmf_queue = blk_mq_alloc_queue(&hba->tmf_tag_set, NULL, NULL);
> +	if (IS_ERR(hba->tmf_queue)) {
> +		err = PTR_ERR(hba->tmf_queue);
> +		goto free_tmf_tag_set;
> +	}
> +	hba->tmf_rqs = devm_kcalloc(hba->dev, hba->nutmrs,
> +				    sizeof(*hba->tmf_rqs), GFP_KERNEL);
> +	if (!hba->tmf_rqs) {
> +		err = -ENOMEM;
> +		goto free_tmf_queue;
> +	}
> +
> +	return 0;
> +
> +free_tmf_queue:
> +	blk_mq_destroy_queue(hba->tmf_queue);
> +	blk_put_queue(hba->tmf_queue);
> +
> +free_tmf_tag_set:
> +	blk_mq_free_tag_set(&hba->tmf_tag_set);
> +
> +remove_scsi_host:
> +	if (hba->scsi_host_added)
> +		scsi_remove_host(hba->host);
> +
> +	return err;
> +}
> +
>  /**
>   * ufshcd_init - Driver initialization routine
>   * @hba: per-adapter instance
> @@ -10514,35 +10564,9 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
>  		hba->is_irq_enabled = true;
>  	}
>  
> -	if (!is_mcq_supported(hba)) {

I guess this series is based on v6.11-rc1, because starting from -rc2 we have
lsdb check here.

- Mani

> -		err = scsi_add_host(host, hba->dev);
> -		if (err) {
> -			dev_err(hba->dev, "scsi_add_host failed\n");
> -			goto out_disable;
> -		}
> -		hba->scsi_host_added = true;
> -	}
> -
> -	hba->tmf_tag_set = (struct blk_mq_tag_set) {
> -		.nr_hw_queues	= 1,
> -		.queue_depth	= hba->nutmrs,
> -		.ops		= &ufshcd_tmf_ops,
> -		.flags		= BLK_MQ_F_NO_SCHED,
> -	};
> -	err = blk_mq_alloc_tag_set(&hba->tmf_tag_set);
> -	if (err < 0)
> -		goto out_remove_scsi_host;
> -	hba->tmf_queue = blk_mq_alloc_queue(&hba->tmf_tag_set, NULL, NULL);
> -	if (IS_ERR(hba->tmf_queue)) {
> -		err = PTR_ERR(hba->tmf_queue);
> -		goto free_tmf_tag_set;
> -	}
> -	hba->tmf_rqs = devm_kcalloc(hba->dev, hba->nutmrs,
> -				    sizeof(*hba->tmf_rqs), GFP_KERNEL);
> -	if (!hba->tmf_rqs) {
> -		err = -ENOMEM;
> -		goto free_tmf_queue;
> -	}
> +	err = ufshcd_add_scsi_host(hba);
> +	if (err)
> +		goto out_disable;
>  
>  	/* Reset the attached device */
>  	ufshcd_device_reset(hba);
> @@ -10600,9 +10624,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
>  free_tmf_queue:
>  	blk_mq_destroy_queue(hba->tmf_queue);
>  	blk_put_queue(hba->tmf_queue);
> -free_tmf_tag_set:
>  	blk_mq_free_tag_set(&hba->tmf_tag_set);
> -out_remove_scsi_host:
>  	if (hba->scsi_host_added)
>  		scsi_remove_host(hba->host);
>  out_disable:

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 2/9] ufs: core: Introduce ufshcd_activate_link()

[Manivannan Sadhasivam]

On Thu, Aug 22, 2024 at 02:36:03PM -0700, Bart Van Assche wrote:
> Prepare for introducing a second caller by moving the code for
> activating the link between UFS controller and device into a new
> function. No functionality has been changed.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  drivers/ufs/core/ufshcd.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index d29e469c3873..04d94bf5cc2d 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8733,10 +8733,9 @@ static void ufshcd_config_mcq(struct ufs_hba *hba)
>  		 hba->nutrs);
>  }
>  
> -static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
> +static int ufshcd_activate_link(struct ufs_hba *hba)
>  {
>  	int ret;
> -	struct Scsi_Host *host = hba->host;
>  
>  	hba->ufshcd_state = UFSHCD_STATE_RESET;
>  
> @@ -8753,6 +8752,18 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
>  	/* UniPro link is active now */
>  	ufshcd_set_link_active(hba);
>  
> +	return 0;
> +}
> +
> +static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
> +{
> +	struct Scsi_Host *host = hba->host;
> +	int ret;
> +
> +	ret = ufshcd_activate_link(hba);
> +	if (ret)
> +		return ret;
> +
>  	/* Reconfigure MCQ upon reset */
>  	if (hba->mcq_enabled && !init_dev_params) {
>  		ufshcd_config_mcq(hba);

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 3/9] ufs: core: Introduce ufshcd_post_device_init()

[Manivannan Sadhasivam]

On Thu, Aug 22, 2024 at 02:36:04PM -0700, Bart Van Assche wrote:
> Prepare for introducing a second caller by moving more code from
> ufshcd_device_init() into a new function.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

One nitpick below.

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

> ---
>  drivers/ufs/core/ufshcd.c | 64 ++++++++++++++++++++++-----------------
>  1 file changed, 37 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 04d94bf5cc2d..dcc417d7e19c 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8755,6 +8755,42 @@ static int ufshcd_activate_link(struct ufs_hba *hba)
>  	return 0;
>  }
>  
> +static int ufshcd_post_device_init(struct ufs_hba *hba)
> +{
> +	int ret;
> +
> +	ufshcd_tune_unipro_params(hba);
> +
> +	/* UFS device is also active now */
> +	ufshcd_set_ufs_dev_active(hba);
> +	ufshcd_force_reset_auto_bkops(hba);
> +
> +	ufshcd_set_timestamp_attr(hba);
> +	schedule_delayed_work(&hba->ufs_rtc_update_work,
> +			      msecs_to_jiffies(UFS_RTC_UPDATE_INTERVAL_MS));
> +
> +	if (!hba->max_pwr_info.is_valid)
> +		return 0;
> +
> +	/* Gear up to HS gear. */

Maybe this comment now belongs above ufshcd_config_pwr_mode()?

- Mani

> +
> +	/*
> +	 * Set the right value to bRefClkFreq before attempting to
> +	 * switch to HS gears.
> +	 */
> +	if (hba->dev_ref_clk_freq != REF_CLK_FREQ_INVAL)
> +		ufshcd_set_dev_ref_clk(hba);
> +	ret = ufshcd_config_pwr_mode(hba, &hba->max_pwr_info.info);
> +	if (ret) {
> +		dev_err(hba->dev,
> +			"%s: Failed setting power mode, err = %d\n",
> +			__func__, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
>  {
>  	struct Scsi_Host *host = hba->host;
> @@ -8813,33 +8849,7 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
>  		}
>  	}
>  
> -	ufshcd_tune_unipro_params(hba);
> -
> -	/* UFS device is also active now */
> -	ufshcd_set_ufs_dev_active(hba);
> -	ufshcd_force_reset_auto_bkops(hba);
> -
> -	ufshcd_set_timestamp_attr(hba);
> -	schedule_delayed_work(&hba->ufs_rtc_update_work,
> -			      msecs_to_jiffies(UFS_RTC_UPDATE_INTERVAL_MS));
> -
> -	/* Gear up to HS gear if supported */
> -	if (hba->max_pwr_info.is_valid) {
> -		/*
> -		 * Set the right value to bRefClkFreq before attempting to
> -		 * switch to HS gears.
> -		 */
> -		if (hba->dev_ref_clk_freq != REF_CLK_FREQ_INVAL)
> -			ufshcd_set_dev_ref_clk(hba);
> -		ret = ufshcd_config_pwr_mode(hba, &hba->max_pwr_info.info);
> -		if (ret) {
> -			dev_err(hba->dev, "%s: Failed setting power mode, err = %d\n",
> -					__func__, ret);
> -			return ret;
> -		}
> -	}
> -
> -	return 0;
> +	return ufshcd_post_device_init(hba);
>  }
>  
>  /**

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 4/9] ufs: core: Call ufshcd_add_scsi_host() later

[Manivannan Sadhasivam]

On Thu, Aug 22, 2024 at 02:36:05PM -0700, Bart Van Assche wrote:
> Call ufshcd_add_scsi_host() after host controller initialization has
> completed. This is possible because no code between the old and new
> ufshcd_add_scsi_host() call site depends on the scsi_add_host() call.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  drivers/ufs/core/ufshcd.c | 16 +++++-----------
>  1 file changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index dcc417d7e19c..b513ef46d848 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -10585,10 +10585,6 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
>  		hba->is_irq_enabled = true;
>  	}
>  
> -	err = ufshcd_add_scsi_host(hba);
> -	if (err)
> -		goto out_disable;
> -
>  	/* Reset the attached device */
>  	ufshcd_device_reset(hba);
>  
> @@ -10600,7 +10596,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
>  		dev_err(hba->dev, "Host controller enable failed\n");
>  		ufshcd_print_evt_hist(hba);
>  		ufshcd_print_host_state(hba);
> -		goto free_tmf_queue;
> +		goto out_disable;
>  	}
>  
>  	/*
> @@ -10635,6 +10631,10 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
>  	 */
>  	ufshcd_set_ufs_dev_active(hba);
>  
> +	err = ufshcd_add_scsi_host(hba);
> +	if (err)
> +		goto out_disable;
> +
>  	async_schedule(ufshcd_async_scan, hba);
>  	ufs_sysfs_add_nodes(hba->dev);
>  
> @@ -10642,12 +10642,6 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
>  	ufshcd_pm_qos_init(hba);
>  	return 0;
>  
> -free_tmf_queue:
> -	blk_mq_destroy_queue(hba->tmf_queue);
> -	blk_put_queue(hba->tmf_queue);
> -	blk_mq_free_tag_set(&hba->tmf_tag_set);
> -	if (hba->scsi_host_added)
> -		scsi_remove_host(hba->host);
>  out_disable:
>  	hba->is_irq_enabled = false;
>  	ufshcd_hba_exit(hba);

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 5/9] ufs: core: Move the ufshcd_device_init() call

[Manivannan Sadhasivam]

On Thu, Aug 22, 2024 at 02:36:06PM -0700, Bart Van Assche wrote:
> Move the ufshcd_device_init() call to the ufshcd_probe_hba() callers and
> remove the 'init_dev_params' argument of ufshcd_probe_hba(). This change
> refactors the code without modifying the behavior of the UFSHCI driver.
> 

I don't see an explanation on why this refactoring is necessary though.
Especially when you move it to callers.

- Mani

> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/ufs/core/ufshcd.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index b513ef46d848..5c8751672bc7 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -298,7 +298,8 @@ static int ufshcd_reset_and_restore(struct ufs_hba *hba);
>  static int ufshcd_eh_host_reset_handler(struct scsi_cmnd *cmd);
>  static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag);
>  static void ufshcd_hba_exit(struct ufs_hba *hba);
> -static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params);
> +static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params);
> +static int ufshcd_probe_hba(struct ufs_hba *hba);
>  static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on);
>  static inline void ufshcd_add_delay_before_dme_cmd(struct ufs_hba *hba);
>  static int ufshcd_host_reset_and_restore(struct ufs_hba *hba);
> @@ -7717,8 +7718,11 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
>  	err = ufshcd_hba_enable(hba);
>  
>  	/* Establish the link again and restore the device */
> -	if (!err)
> -		err = ufshcd_probe_hba(hba, false);
> +	if (!err) {
> +		err = ufshcd_device_init(hba, /*init_dev_params=*/false);
> +		if (!err)
> +			err = ufshcd_probe_hba(hba);
> +	}
>  
>  	if (err)
>  		dev_err(hba->dev, "%s: Host init failed %d\n", __func__, err);
> @@ -8855,21 +8859,16 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
>  /**
>   * ufshcd_probe_hba - probe hba to detect device and initialize it
>   * @hba: per-adapter instance
> - * @init_dev_params: whether or not to call ufshcd_device_params_init().
>   *
>   * Execute link-startup and verify device initialization
>   *
>   * Return: 0 upon success; < 0 upon failure.
>   */
> -static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
> +static int ufshcd_probe_hba(struct ufs_hba *hba)
>  {
>  	ktime_t start = ktime_get();
>  	unsigned long flags;
> -	int ret;
> -
> -	ret = ufshcd_device_init(hba, init_dev_params);
> -	if (ret)
> -		goto out;
> +	int ret = 0;
>  
>  	if (!hba->pm_op_in_progress &&
>  	    (hba->quirks & UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH)) {
> @@ -8887,7 +8886,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
>  		}
>  
>  		/* Reinit the device */
> -		ret = ufshcd_device_init(hba, init_dev_params);
> +		ret = ufshcd_device_init(hba, /*init_dev_params=*/false);
>  		if (ret)
>  			goto out;
>  	}
> @@ -8935,7 +8934,9 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
>  
>  	down(&hba->host_sem);
>  	/* Initialize hba, detect and initialize UFS device */
> -	ret = ufshcd_probe_hba(hba, true);
> +	ret = ufshcd_device_init(hba, /*init_dev_params=*/true);
> +	if (ret == 0)
> +		ret = ufshcd_probe_hba(hba);
>  	up(&hba->host_sem);
>  	if (ret)
>  		goto out;

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 6/9] ufs: core: Move the ufshcd_device_init(hba, true) call

[Manivannan Sadhasivam]

On Thu, Aug 22, 2024 at 02:36:07PM -0700, Bart Van Assche wrote:
> Move the ufshcd_device_init(hba, true) call from ufshcd_async_scan()
> into ufshcd_init(). This patch prepares for moving both scsi_add_host()
> calls into ufshcd_add_scsi_host(). Calling ufshcd_device_init() from
> ufshcd_init() without holding hba->host_sem is safe because
> hba->host_sem serializes core code with sysfs callback code and because
> the ufshcd_device_init() is moved before the scsi_add_host() call.

I think this last sentence is not complete.

> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

One nitpick below.

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

> ---
>  drivers/ufs/core/ufshcd.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 5c8751672bc7..0fdf19889191 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8933,10 +8933,7 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
>  	int ret;
>  
>  	down(&hba->host_sem);
> -	/* Initialize hba, detect and initialize UFS device */
> -	ret = ufshcd_device_init(hba, /*init_dev_params=*/true);
> -	if (ret == 0)
> -		ret = ufshcd_probe_hba(hba);
> +	ret = ufshcd_probe_hba(hba);
>  	up(&hba->host_sem);
>  	if (ret)
>  		goto out;
> @@ -10632,6 +10629,11 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
>  	 */
>  	ufshcd_set_ufs_dev_active(hba);
>  
> +	/* Initialize hba, detect and initialize UFS device */
> +	err = ufshcd_device_init(hba, /*init_dev_params=*/true);

I think this inline comment is not really needed. It is rather decreasing code
readability.

- Mani

> +	if (err)
> +		goto out_disable;
> +
>  	err = ufshcd_add_scsi_host(hba);
>  	if (err)
>  		goto out_disable;

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 7/9] ufs: core: Expand the ufshcd_device_init(hba, true) call

[Manivannan Sadhasivam]

On Thu, Aug 22, 2024 at 02:36:08PM -0700, Bart Van Assche wrote:
> Expand the ufshcd_device_init(hba, true) call and remove all code that
> depends on init_dev_params == false.
> 

Again, no justification is provided on why the expansion is necessary.

- Mani

> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/ufs/core/ufshcd.c | 44 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 0fdf19889191..6a3873991d2a 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -10629,8 +10629,48 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
>  	 */
>  	ufshcd_set_ufs_dev_active(hba);
>  
> -	/* Initialize hba, detect and initialize UFS device */
> -	err = ufshcd_device_init(hba, /*init_dev_params=*/true);
> +	err = ufshcd_activate_link(hba);
> +	if (err)
> +		goto out_disable;
> +
> +	/* Verify device initialization by sending NOP OUT UPIU. */
> +	err = ufshcd_verify_dev_init(hba);
> +	if (err)
> +		goto out_disable;
> +
> +	/* Initiate UFS initialization and waiting for completion. */
> +	err = ufshcd_complete_dev_init(hba);
> +	if (err)
> +		goto out_disable;
> +
> +	/*
> +	 * Initialize UFS device parameters used by driver, these
> +	 * parameters are associated with UFS descriptors.
> +	 */
> +	err = ufshcd_device_params_init(hba);
> +	if (err)
> +		goto out_disable;
> +	if (is_mcq_supported(hba)) {
> +		ufshcd_mcq_enable(hba);
> +		err = ufshcd_alloc_mcq(hba);
> +		if (!err) {
> +			ufshcd_config_mcq(hba);
> +		} else {
> +			/* Continue with SDB mode */
> +			ufshcd_mcq_disable(hba);
> +			use_mcq_mode = false;
> +			dev_err(hba->dev, "MCQ mode is disabled, err=%d\n",
> +				err);
> +		}
> +		err = scsi_add_host(host, hba->dev);
> +		if (err) {
> +			dev_err(hba->dev, "scsi_add_host failed\n");
> +			goto out_disable;
> +		}
> +		hba->scsi_host_added = true;
> +	}
> +
> +	err = ufshcd_post_device_init(hba);
>  	if (err)
>  		goto out_disable;
>  

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 8/9] ufs: core: Move the MCQ scsi_add_host() call

[Manivannan Sadhasivam]

On Thu, Aug 22, 2024 at 02:36:09PM -0700, Bart Van Assche wrote:
> Whether or not MCQ is used, call scsi_add_host from ufshcd_add_scsi_host().
> For MCQ this patch swaps the order of the scsi_add_host() and
> ufshcd_post_device_init() calls. This patch also prepares for moving
> both scsi_add_host() calls into ufshcd_add_scsi_host().
> 

This needs changes due to the addition of lsdb check that skips scsi_add_host().

- Mani

> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/ufs/core/ufshcd.c | 43 ++++++++++++++++-----------------------
>  1 file changed, 18 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 6a3873991d2a..b9aaa3c55d17 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -10404,15 +10404,27 @@ static int ufshcd_add_scsi_host(struct ufs_hba *hba)
>  {
>  	int err;
>  
> -	if (!is_mcq_supported(hba)) {
> -		err = scsi_add_host(hba->host, hba->dev);
> -		if (err) {
> -			dev_err(hba->dev, "scsi_add_host failed\n");
> -			return err;
> +	if (is_mcq_supported(hba)) {
> +		ufshcd_mcq_enable(hba);
> +		err = ufshcd_alloc_mcq(hba);
> +		if (!err) {
> +			ufshcd_config_mcq(hba);
> +		} else {
> +			/* Continue with SDB mode */
> +			ufshcd_mcq_disable(hba);
> +			use_mcq_mode = false;
> +			dev_err(hba->dev, "MCQ mode is disabled, err=%d\n",
> +				err);
>  		}
> -		hba->scsi_host_added = true;
>  	}
>  
> +	err = scsi_add_host(hba->host, hba->dev);
> +	if (err) {
> +		dev_err(hba->dev, "scsi_add_host failed\n");
> +		return err;
> +	}
> +	hba->scsi_host_added = true;
> +
>  	hba->tmf_tag_set = (struct blk_mq_tag_set) {
>  		.nr_hw_queues	= 1,
>  		.queue_depth	= hba->nutmrs,
> @@ -10650,25 +10662,6 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
>  	err = ufshcd_device_params_init(hba);
>  	if (err)
>  		goto out_disable;
> -	if (is_mcq_supported(hba)) {
> -		ufshcd_mcq_enable(hba);
> -		err = ufshcd_alloc_mcq(hba);
> -		if (!err) {
> -			ufshcd_config_mcq(hba);
> -		} else {
> -			/* Continue with SDB mode */
> -			ufshcd_mcq_disable(hba);
> -			use_mcq_mode = false;
> -			dev_err(hba->dev, "MCQ mode is disabled, err=%d\n",
> -				err);
> -		}
> -		err = scsi_add_host(host, hba->dev);
> -		if (err) {
> -			dev_err(hba->dev, "scsi_add_host failed\n");
> -			goto out_disable;
> -		}
> -		hba->scsi_host_added = true;
> -	}
>  
>  	err = ufshcd_post_device_init(hba);
>  	if (err)

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 1/9] ufs: core: Introduce ufshcd_add_scsi_host()

[Bart Van Assche]

On 8/25/24 1:10 AM, Manivannan Sadhasivam wrote:
> On Thu, Aug 22, 2024 at 02:36:02PM -0700, Bart Van Assche wrote:
>>   /**
>>    * ufshcd_init - Driver initialization routine
>>    * @hba: per-adapter instance
>> @@ -10514,35 +10564,9 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
>>   		hba->is_irq_enabled = true;
>>   	}
>>   
>> -	if (!is_mcq_supported(hba)) {
> 
> I guess this series is based on v6.11-rc1, because starting from -rc2 we have
> lsdb check here.

Hi Manivannan,

My patch series is based on Martin's for-next branch. The
UFSHCD_QUIRK_BROKEN_LSDBS_CAP support has been queued on Martin's
fixes branch. I think this is why the lsdb check is not visible
above. I will analyze whether it is possible to rework this patch
series such that Linus won't have to solve a complicated merge
conflict during the next merge window.

Thanks,

Bart.

Re: [PATCH v2 3/9] ufs: core: Introduce ufshcd_post_device_init()

[Bart Van Assche]

On 8/25/24 1:16 AM, Manivannan Sadhasivam wrote:
> On Thu, Aug 22, 2024 at 02:36:04PM -0700, Bart Van Assche wrote:
>> +static int ufshcd_post_device_init(struct ufs_hba *hba)
>> +{
>> +	int ret;
>> +
>> +	ufshcd_tune_unipro_params(hba);
>> +
>> +	/* UFS device is also active now */
>> +	ufshcd_set_ufs_dev_active(hba);
>> +	ufshcd_force_reset_auto_bkops(hba);
>> +
>> +	ufshcd_set_timestamp_attr(hba);
>> +	schedule_delayed_work(&hba->ufs_rtc_update_work,
>> +			      msecs_to_jiffies(UFS_RTC_UPDATE_INTERVAL_MS));
>> +
>> +	if (!hba->max_pwr_info.is_valid)
>> +		return 0;
>> +
>> +	/* Gear up to HS gear. */
> 
> Maybe this comment now belongs above ufshcd_config_pwr_mode()?

Hi Manivannan,

Thanks for the feedback. I will move that comment.

Bart.

Re: [PATCH v2 5/9] ufs: core: Move the ufshcd_device_init() call

[Bart Van Assche]

On 8/25/24 1:22 AM, Manivannan Sadhasivam wrote:
> On Thu, Aug 22, 2024 at 02:36:06PM -0700, Bart Van Assche wrote:
>> Move the ufshcd_device_init() call to the ufshcd_probe_hba() callers and
>> remove the 'init_dev_params' argument of ufshcd_probe_hba(). This change
>> refactors the code without modifying the behavior of the UFSHCI driver.
>>
> 
> I don't see an explanation on why this refactoring is necessary though.
> Especially when you move it to callers.

Hi Mani,

I will add the following in the patch description: "This change prepares
for moving one ufshcd_device_init() call into ufshcd_init()."

Thanks,

Bart.

Re: [PATCH v2 6/9] ufs: core: Move the ufshcd_device_init(hba, true) call

[Bart Van Assche]

On 8/25/24 1:33 AM, Manivannan Sadhasivam wrote:
> On Thu, Aug 22, 2024 at 02:36:07PM -0700, Bart Van Assche wrote:
>> Move the ufshcd_device_init(hba, true) call from ufshcd_async_scan()
>> into ufshcd_init(). This patch prepares for moving both scsi_add_host()
>> calls into ufshcd_add_scsi_host(). Calling ufshcd_device_init() from
>> ufshcd_init() without holding hba->host_sem is safe because
>> hba->host_sem serializes core code with sysfs callback code and because
>> the ufshcd_device_init() is moved before the scsi_add_host() call.
> 
> I think this last sentence is not complete.

It is complete but your feedback means to me that it is hard to
understand. I will reword it.

>> @@ -10632,6 +10629,11 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
>>   	 */
>>   	ufshcd_set_ufs_dev_active(hba);
>>   
>> +	/* Initialize hba, detect and initialize UFS device */
>> +	err = ufshcd_device_init(hba, /*init_dev_params=*/true);
> 
> I think this inline comment is not really needed. It is rather decreasing code
> readability.

I will remove it.

Thanks,

Bart.