[PATCH 00/13] scsi: ufs: qcom: Minor code cleanups

[PATCH 00/13] scsi: ufs: qcom: Minor code cleanups

[Manivannan Sadhasivam]

Hello,

This series has some minor code cleanups to the Qcom UFS driver. No functional
change.

Tested on: RB5 development board based on Qcom SM8250 SoC.

- Mani

Manivannan Sadhasivam (13):
  scsi: ufs: qcom: Use clk_bulk APIs for managing lane clocks
  scsi: ufs: qcom: Fix the return value of ufs_qcom_ice_program_key()
  scsi: ufs: qcom: Fix the return value when
    platform_get_resource_byname() fails
  scsi: ufs: qcom: Remove superfluous variable assignments
  scsi: ufs: qcom: Remove the warning message when core_reset is not
    available
  scsi: ufs: qcom: Export ufshcd_{enable/disable}_irq helpers and make
    use of them
  scsi: ufs: qcom: Fail ufs_qcom_power_up_sequence() when core_reset
    fails
  scsi: ufs: qcom: Check the return value of
    ufs_qcom_power_up_sequence()
  scsi: ufs: qcom: Remove redundant error print for devm_kzalloc()
    failure
  scsi: ufs: qcom: Use dev_err_probe() to simplify error handling of
    devm_gpiod_get_optional()
  scsi: ufs: qcom: Remove unused ufs_qcom_hosts struct array
  scsi: ufs: qcom: Sort includes alphabetically
  scsi: ufs: qcom: Initialize cycles_in_1us variable in
    ufs_qcom_set_core_clk_ctrl()

 drivers/ufs/core/ufshcd.c   |   6 +-
 drivers/ufs/host/ufs-qcom.c | 165 ++++++++----------------------------
 drivers/ufs/host/ufs-qcom.h |   6 +-
 include/ufs/ufshcd.h        |   2 +
 4 files changed, 43 insertions(+), 136 deletions(-)

-- 
2.25.1

[PATCH 01/13] scsi: ufs: qcom: Use clk_bulk APIs for managing lane clocks

[Manivannan Sadhasivam]

Lane clock handling can be simplified by using the clk_bulk APIs. So let's
make use of them. This also get's rid of the clock validation in the driver
as kernel should just rely on the firmware (DT/ACPI) to provide the clocks
required for proper functioning.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/ufs/host/ufs-qcom.c | 94 ++-----------------------------------
 drivers/ufs/host/ufs-qcom.h |  6 +--
 2 files changed, 7 insertions(+), 93 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 96cb8b5b4e66..cbb6a696cd97 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -194,52 +194,12 @@ static inline int ufs_qcom_ice_suspend(struct ufs_qcom_host *host)
 }
 #endif
 
-static int ufs_qcom_host_clk_get(struct device *dev,
-		const char *name, struct clk **clk_out, bool optional)
-{
-	struct clk *clk;
-	int err = 0;
-
-	clk = devm_clk_get(dev, name);
-	if (!IS_ERR(clk)) {
-		*clk_out = clk;
-		return 0;
-	}
-
-	err = PTR_ERR(clk);
-
-	if (optional && err == -ENOENT) {
-		*clk_out = NULL;
-		return 0;
-	}
-
-	if (err != -EPROBE_DEFER)
-		dev_err(dev, "failed to get %s err %d\n", name, err);
-
-	return err;
-}
-
-static int ufs_qcom_host_clk_enable(struct device *dev,
-		const char *name, struct clk *clk)
-{
-	int err = 0;
-
-	err = clk_prepare_enable(clk);
-	if (err)
-		dev_err(dev, "%s: %s enable failed %d\n", __func__, name, err);
-
-	return err;
-}
-
 static void ufs_qcom_disable_lane_clks(struct ufs_qcom_host *host)
 {
 	if (!host->is_lane_clks_enabled)
 		return;
 
-	clk_disable_unprepare(host->tx_l1_sync_clk);
-	clk_disable_unprepare(host->tx_l0_sync_clk);
-	clk_disable_unprepare(host->rx_l1_sync_clk);
-	clk_disable_unprepare(host->rx_l0_sync_clk);
+	clk_bulk_disable_unprepare(host->num_clks, host->clks);
 
 	host->is_lane_clks_enabled = false;
 }
@@ -247,43 +207,14 @@ static void ufs_qcom_disable_lane_clks(struct ufs_qcom_host *host)
 static int ufs_qcom_enable_lane_clks(struct ufs_qcom_host *host)
 {
 	int err;
-	struct device *dev = host->hba->dev;
-
-	if (host->is_lane_clks_enabled)
-		return 0;
 
-	err = ufs_qcom_host_clk_enable(dev, "rx_lane0_sync_clk",
-		host->rx_l0_sync_clk);
+	err = clk_bulk_prepare_enable(host->num_clks, host->clks);
 	if (err)
 		return err;
 
-	err = ufs_qcom_host_clk_enable(dev, "tx_lane0_sync_clk",
-		host->tx_l0_sync_clk);
-	if (err)
-		goto disable_rx_l0;
-
-	err = ufs_qcom_host_clk_enable(dev, "rx_lane1_sync_clk",
-			host->rx_l1_sync_clk);
-	if (err)
-		goto disable_tx_l0;
-
-	err = ufs_qcom_host_clk_enable(dev, "tx_lane1_sync_clk",
-			host->tx_l1_sync_clk);
-	if (err)
-		goto disable_rx_l1;
-
 	host->is_lane_clks_enabled = true;
 
 	return 0;
-
-disable_rx_l1:
-	clk_disable_unprepare(host->rx_l1_sync_clk);
-disable_tx_l0:
-	clk_disable_unprepare(host->tx_l0_sync_clk);
-disable_rx_l0:
-	clk_disable_unprepare(host->rx_l0_sync_clk);
-
-	return err;
 }
 
 static int ufs_qcom_init_lane_clks(struct ufs_qcom_host *host)
@@ -294,26 +225,11 @@ static int ufs_qcom_init_lane_clks(struct ufs_qcom_host *host)
 	if (has_acpi_companion(dev))
 		return 0;
 
-	err = ufs_qcom_host_clk_get(dev, "rx_lane0_sync_clk",
-					&host->rx_l0_sync_clk, false);
-	if (err)
-		return err;
-
-	err = ufs_qcom_host_clk_get(dev, "tx_lane0_sync_clk",
-					&host->tx_l0_sync_clk, false);
-	if (err)
+	err = devm_clk_bulk_get_all(dev, &host->clks);
+	if (err <= 0)
 		return err;
 
-	/* In case of single lane per direction, don't read lane1 clocks */
-	if (host->hba->lanes_per_direction > 1) {
-		err = ufs_qcom_host_clk_get(dev, "rx_lane1_sync_clk",
-			&host->rx_l1_sync_clk, false);
-		if (err)
-			return err;
-
-		err = ufs_qcom_host_clk_get(dev, "tx_lane1_sync_clk",
-			&host->tx_l1_sync_clk, true);
-	}
+	host->num_clks = err;
 
 	return 0;
 }
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index 9950a0089475..e2df4c528a2a 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -213,10 +213,8 @@ struct ufs_qcom_host {
 	struct phy *generic_phy;
 	struct ufs_hba *hba;
 	struct ufs_pa_layer_attr dev_req_params;
-	struct clk *rx_l0_sync_clk;
-	struct clk *tx_l0_sync_clk;
-	struct clk *rx_l1_sync_clk;
-	struct clk *tx_l1_sync_clk;
+	struct clk_bulk_data *clks;
+	u32 num_clks;
 	bool is_lane_clks_enabled;
 
 	struct icc_path *icc_ddr;
-- 
2.25.1

Re: [PATCH 01/13] scsi: ufs: qcom: Use clk_bulk APIs for managing lane clocks

[Andrew Halaney]

On Fri, Dec 01, 2023 at 08:44:05PM +0530, Manivannan Sadhasivam wrote:
> Lane clock handling can be simplified by using the clk_bulk APIs. So let's
> make use of them. This also get's rid of the clock validation in the driver
> as kernel should just rely on the firmware (DT/ACPI) to provide the clocks
> required for proper functioning.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>

> ---
>  drivers/ufs/host/ufs-qcom.c | 94 ++-----------------------------------
>  drivers/ufs/host/ufs-qcom.h |  6 +--
>  2 files changed, 7 insertions(+), 93 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 96cb8b5b4e66..cbb6a696cd97 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -194,52 +194,12 @@ static inline int ufs_qcom_ice_suspend(struct ufs_qcom_host *host)
>  }
>  #endif
>  
> -static int ufs_qcom_host_clk_get(struct device *dev,
> -		const char *name, struct clk **clk_out, bool optional)
> -{
> -	struct clk *clk;
> -	int err = 0;
> -
> -	clk = devm_clk_get(dev, name);
> -	if (!IS_ERR(clk)) {
> -		*clk_out = clk;
> -		return 0;
> -	}
> -
> -	err = PTR_ERR(clk);
> -
> -	if (optional && err == -ENOENT) {
> -		*clk_out = NULL;
> -		return 0;
> -	}
> -
> -	if (err != -EPROBE_DEFER)
> -		dev_err(dev, "failed to get %s err %d\n", name, err);
> -
> -	return err;
> -}
> -
> -static int ufs_qcom_host_clk_enable(struct device *dev,
> -		const char *name, struct clk *clk)
> -{
> -	int err = 0;
> -
> -	err = clk_prepare_enable(clk);
> -	if (err)
> -		dev_err(dev, "%s: %s enable failed %d\n", __func__, name, err);
> -
> -	return err;
> -}
> -
>  static void ufs_qcom_disable_lane_clks(struct ufs_qcom_host *host)
>  {
>  	if (!host->is_lane_clks_enabled)
>  		return;
>  
> -	clk_disable_unprepare(host->tx_l1_sync_clk);
> -	clk_disable_unprepare(host->tx_l0_sync_clk);
> -	clk_disable_unprepare(host->rx_l1_sync_clk);
> -	clk_disable_unprepare(host->rx_l0_sync_clk);
> +	clk_bulk_disable_unprepare(host->num_clks, host->clks);
>  
>  	host->is_lane_clks_enabled = false;
>  }
> @@ -247,43 +207,14 @@ static void ufs_qcom_disable_lane_clks(struct ufs_qcom_host *host)
>  static int ufs_qcom_enable_lane_clks(struct ufs_qcom_host *host)
>  {
>  	int err;
> -	struct device *dev = host->hba->dev;
> -
> -	if (host->is_lane_clks_enabled)
> -		return 0;
>  
> -	err = ufs_qcom_host_clk_enable(dev, "rx_lane0_sync_clk",
> -		host->rx_l0_sync_clk);
> +	err = clk_bulk_prepare_enable(host->num_clks, host->clks);
>  	if (err)
>  		return err;
>  
> -	err = ufs_qcom_host_clk_enable(dev, "tx_lane0_sync_clk",
> -		host->tx_l0_sync_clk);
> -	if (err)
> -		goto disable_rx_l0;
> -
> -	err = ufs_qcom_host_clk_enable(dev, "rx_lane1_sync_clk",
> -			host->rx_l1_sync_clk);
> -	if (err)
> -		goto disable_tx_l0;
> -
> -	err = ufs_qcom_host_clk_enable(dev, "tx_lane1_sync_clk",
> -			host->tx_l1_sync_clk);
> -	if (err)
> -		goto disable_rx_l1;
> -
>  	host->is_lane_clks_enabled = true;
>  
>  	return 0;
> -
> -disable_rx_l1:
> -	clk_disable_unprepare(host->rx_l1_sync_clk);
> -disable_tx_l0:
> -	clk_disable_unprepare(host->tx_l0_sync_clk);
> -disable_rx_l0:
> -	clk_disable_unprepare(host->rx_l0_sync_clk);
> -
> -	return err;
>  }
>  
>  static int ufs_qcom_init_lane_clks(struct ufs_qcom_host *host)
> @@ -294,26 +225,11 @@ static int ufs_qcom_init_lane_clks(struct ufs_qcom_host *host)
>  	if (has_acpi_companion(dev))
>  		return 0;
>  
> -	err = ufs_qcom_host_clk_get(dev, "rx_lane0_sync_clk",
> -					&host->rx_l0_sync_clk, false);
> -	if (err)
> -		return err;
> -
> -	err = ufs_qcom_host_clk_get(dev, "tx_lane0_sync_clk",
> -					&host->tx_l0_sync_clk, false);
> -	if (err)
> +	err = devm_clk_bulk_get_all(dev, &host->clks);
> +	if (err <= 0)
>  		return err;
>  
> -	/* In case of single lane per direction, don't read lane1 clocks */
> -	if (host->hba->lanes_per_direction > 1) {
> -		err = ufs_qcom_host_clk_get(dev, "rx_lane1_sync_clk",
> -			&host->rx_l1_sync_clk, false);
> -		if (err)
> -			return err;
> -
> -		err = ufs_qcom_host_clk_get(dev, "tx_lane1_sync_clk",
> -			&host->tx_l1_sync_clk, true);
> -	}
> +	host->num_clks = err;
>  
>  	return 0;
>  }
> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
> index 9950a0089475..e2df4c528a2a 100644
> --- a/drivers/ufs/host/ufs-qcom.h
> +++ b/drivers/ufs/host/ufs-qcom.h
> @@ -213,10 +213,8 @@ struct ufs_qcom_host {
>  	struct phy *generic_phy;
>  	struct ufs_hba *hba;
>  	struct ufs_pa_layer_attr dev_req_params;
> -	struct clk *rx_l0_sync_clk;
> -	struct clk *tx_l0_sync_clk;
> -	struct clk *rx_l1_sync_clk;
> -	struct clk *tx_l1_sync_clk;
> +	struct clk_bulk_data *clks;
> +	u32 num_clks;
>  	bool is_lane_clks_enabled;
>  
>  	struct icc_path *icc_ddr;
> -- 
> 2.25.1
> 
> 

[PATCH 02/13] scsi: ufs: qcom: Fix the return value of ufs_qcom_ice_program_key()

[Manivannan Sadhasivam]

Currently, the function returns -EINVAL if algorithm other than AES-256-XTS
is requested. But the correct error code is -EOPNOTSUPP. Fix it!

Cc: Abel Vesa <abel.vesa@linaro.org>
Fixes: 56541c7c4468 ("scsi: ufs: ufs-qcom: Switch to the new ICE API")
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/ufs/host/ufs-qcom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index cbb6a696cd97..852179e456f2 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -158,7 +158,7 @@ static int ufs_qcom_ice_program_key(struct ufs_hba *hba,
 	cap = hba->crypto_cap_array[cfg->crypto_cap_idx];
 	if (cap.algorithm_id != UFS_CRYPTO_ALG_AES_XTS ||
 	    cap.key_size != UFS_CRYPTO_KEY_SIZE_256)
-		return -EINVAL;
+		return -EOPNOTSUPP;
 
 	if (config_enable)
 		return qcom_ice_program_key(host->ice,
-- 
2.25.1

Re: [PATCH 02/13] scsi: ufs: qcom: Fix the return value of ufs_qcom_ice_program_key()

[Abel Vesa]

On 23-12-01 20:44:06, Manivannan Sadhasivam wrote:
> Currently, the function returns -EINVAL if algorithm other than AES-256-XTS
> is requested. But the correct error code is -EOPNOTSUPP. Fix it!
> 
> Cc: Abel Vesa <abel.vesa@linaro.org>
> Fixes: 56541c7c4468 ("scsi: ufs: ufs-qcom: Switch to the new ICE API")
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

LGTM.

Reviewed-by: Abel Vesa <abel.vesa@linaro.org>

> ---
>  drivers/ufs/host/ufs-qcom.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index cbb6a696cd97..852179e456f2 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -158,7 +158,7 @@ static int ufs_qcom_ice_program_key(struct ufs_hba *hba,
>  	cap = hba->crypto_cap_array[cfg->crypto_cap_idx];
>  	if (cap.algorithm_id != UFS_CRYPTO_ALG_AES_XTS ||
>  	    cap.key_size != UFS_CRYPTO_KEY_SIZE_256)
> -		return -EINVAL;
> +		return -EOPNOTSUPP;
>  
>  	if (config_enable)
>  		return qcom_ice_program_key(host->ice,
> -- 
> 2.25.1
> 

[PATCH 03/13] scsi: ufs: qcom: Fix the return value when platform_get_resource_byname() fails

[Manivannan Sadhasivam]

The return value should be -ENODEV indicating that the resource is not
provided in DT, not -ENOMEM. Fix it!

Fixes: c263b4ef737e ("scsi: ufs: core: mcq: Configure resource regions")
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/ufs/host/ufs-qcom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 852179e456f2..778df0a9c65e 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1701,7 +1701,7 @@ static int ufs_qcom_mcq_config_resource(struct ufs_hba *hba)
 		if (!res->resource) {
 			dev_info(hba->dev, "Resource %s not provided\n", res->name);
 			if (i == RES_UFS)
-				return -ENOMEM;
+				return -ENODEV;
 			continue;
 		} else if (i == RES_UFS) {
 			res_mem = res->resource;
-- 
2.25.1

Re: [PATCH 03/13] scsi: ufs: qcom: Fix the return value when platform_get_resource_byname() fails

[Andrew Halaney]

On Fri, Dec 01, 2023 at 08:44:07PM +0530, Manivannan Sadhasivam wrote:
> The return value should be -ENODEV indicating that the resource is not
> provided in DT, not -ENOMEM. Fix it!
> 
> Fixes: c263b4ef737e ("scsi: ufs: core: mcq: Configure resource regions")
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>

> ---
>  drivers/ufs/host/ufs-qcom.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 852179e456f2..778df0a9c65e 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -1701,7 +1701,7 @@ static int ufs_qcom_mcq_config_resource(struct ufs_hba *hba)
>  		if (!res->resource) {
>  			dev_info(hba->dev, "Resource %s not provided\n", res->name);
>  			if (i == RES_UFS)
> -				return -ENOMEM;
> +				return -ENODEV;
>  			continue;
>  		} else if (i == RES_UFS) {
>  			res_mem = res->resource;
> -- 
> 2.25.1
> 
> 

[PATCH 04/13] scsi: ufs: qcom: Remove superfluous variable assignments

[Manivannan Sadhasivam]

There are many instances where the variable assignments are not needed.
Remove them.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/ufs/host/ufs-qcom.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 778df0a9c65e..dc93b1c5ca74 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -219,7 +219,7 @@ static int ufs_qcom_enable_lane_clks(struct ufs_qcom_host *host)
 
 static int ufs_qcom_init_lane_clks(struct ufs_qcom_host *host)
 {
-	int err = 0;
+	int err;
 	struct device *dev = host->hba->dev;
 
 	if (has_acpi_companion(dev))
@@ -237,7 +237,7 @@ static int ufs_qcom_init_lane_clks(struct ufs_qcom_host *host)
 static int ufs_qcom_check_hibern8(struct ufs_hba *hba)
 {
 	int err;
-	u32 tx_fsm_val = 0;
+	u32 tx_fsm_val;
 	unsigned long timeout = jiffies + msecs_to_jiffies(HBRN8_POLL_TOUT_MS);
 
 	do {
@@ -292,9 +292,9 @@ static void ufs_qcom_select_unipro_mode(struct ufs_qcom_host *host)
  */
 static int ufs_qcom_host_reset(struct ufs_hba *hba)
 {
-	int ret = 0;
+	int ret;
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
-	bool reenable_intr = false;
+	bool reenable_intr;
 
 	if (!host->core_reset) {
 		dev_warn(hba->dev, "%s: reset control not set\n", __func__);
@@ -417,7 +417,7 @@ static int ufs_qcom_hce_enable_notify(struct ufs_hba *hba,
 				      enum ufs_notify_change_status status)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
-	int err = 0;
+	int err;
 
 	switch (status) {
 	case PRE_CHANGE:
@@ -463,7 +463,7 @@ static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
 	u32 core_clk_period_in_ns;
 	u32 tx_clk_cycles_per_us = 0;
 	unsigned long core_clk_rate = 0;
-	u32 core_clk_cycles_per_us = 0;
+	u32 core_clk_cycles_per_us;
 
 	static u32 pwm_fr_table[][2] = {
 		{UFS_PWM_G1, 0x1},
@@ -1418,7 +1418,7 @@ static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba,
 		bool scale_up, enum ufs_notify_change_status status)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
-	int err = 0;
+	int err;
 
 	/* check the host controller state before sending hibern8 cmd */
 	if (!ufshcd_is_hba_active(hba))
@@ -1689,7 +1689,7 @@ static int ufs_qcom_mcq_config_resource(struct ufs_hba *hba)
 	struct platform_device *pdev = to_platform_device(hba->dev);
 	struct ufshcd_res_info *res;
 	struct resource *res_mem, *res_mcq;
-	int i, ret = 0;
+	int i, ret;
 
 	memcpy(hba->res, ufs_res_info, sizeof(ufs_res_info));
 
-- 
2.25.1

Re: [PATCH 04/13] scsi: ufs: qcom: Remove superfluous variable assignments

[Andrew Halaney]

On Fri, Dec 01, 2023 at 08:44:08PM +0530, Manivannan Sadhasivam wrote:
> There are many instances where the variable assignments are not needed.
> Remove them.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>

> ---
>  drivers/ufs/host/ufs-qcom.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 778df0a9c65e..dc93b1c5ca74 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -219,7 +219,7 @@ static int ufs_qcom_enable_lane_clks(struct ufs_qcom_host *host)
>  
>  static int ufs_qcom_init_lane_clks(struct ufs_qcom_host *host)
>  {
> -	int err = 0;
> +	int err;
>  	struct device *dev = host->hba->dev;
>  
>  	if (has_acpi_companion(dev))
> @@ -237,7 +237,7 @@ static int ufs_qcom_init_lane_clks(struct ufs_qcom_host *host)
>  static int ufs_qcom_check_hibern8(struct ufs_hba *hba)
>  {
>  	int err;
> -	u32 tx_fsm_val = 0;
> +	u32 tx_fsm_val;
>  	unsigned long timeout = jiffies + msecs_to_jiffies(HBRN8_POLL_TOUT_MS);
>  
>  	do {
> @@ -292,9 +292,9 @@ static void ufs_qcom_select_unipro_mode(struct ufs_qcom_host *host)
>   */
>  static int ufs_qcom_host_reset(struct ufs_hba *hba)
>  {
> -	int ret = 0;
> +	int ret;
>  	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> -	bool reenable_intr = false;
> +	bool reenable_intr;
>  
>  	if (!host->core_reset) {
>  		dev_warn(hba->dev, "%s: reset control not set\n", __func__);
> @@ -417,7 +417,7 @@ static int ufs_qcom_hce_enable_notify(struct ufs_hba *hba,
>  				      enum ufs_notify_change_status status)
>  {
>  	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> -	int err = 0;
> +	int err;
>  
>  	switch (status) {
>  	case PRE_CHANGE:
> @@ -463,7 +463,7 @@ static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
>  	u32 core_clk_period_in_ns;
>  	u32 tx_clk_cycles_per_us = 0;
>  	unsigned long core_clk_rate = 0;
> -	u32 core_clk_cycles_per_us = 0;
> +	u32 core_clk_cycles_per_us;
>  
>  	static u32 pwm_fr_table[][2] = {
>  		{UFS_PWM_G1, 0x1},
> @@ -1418,7 +1418,7 @@ static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba,
>  		bool scale_up, enum ufs_notify_change_status status)
>  {
>  	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> -	int err = 0;
> +	int err;
>  
>  	/* check the host controller state before sending hibern8 cmd */
>  	if (!ufshcd_is_hba_active(hba))
> @@ -1689,7 +1689,7 @@ static int ufs_qcom_mcq_config_resource(struct ufs_hba *hba)
>  	struct platform_device *pdev = to_platform_device(hba->dev);
>  	struct ufshcd_res_info *res;
>  	struct resource *res_mem, *res_mcq;
> -	int i, ret = 0;
> +	int i, ret;
>  
>  	memcpy(hba->res, ufs_res_info, sizeof(ufs_res_info));
>  
> -- 
> 2.25.1
> 
> 

[PATCH 05/13] scsi: ufs: qcom: Remove the warning message when core_reset is not available

[Manivannan Sadhasivam]

core_reset is optional, so there is no need to warn the user if it is not
available (that too not while doing host reset each time).

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/ufs/host/ufs-qcom.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index dc93b1c5ca74..d474de0739e4 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -296,10 +296,8 @@ static int ufs_qcom_host_reset(struct ufs_hba *hba)
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 	bool reenable_intr;
 
-	if (!host->core_reset) {
-		dev_warn(hba->dev, "%s: reset control not set\n", __func__);
+	if (!host->core_reset)
 		return 0;
-	}
 
 	reenable_intr = hba->is_irq_enabled;
 	disable_irq(hba->irq);
-- 
2.25.1

Re: [PATCH 05/13] scsi: ufs: qcom: Remove the warning message when core_reset is not available

[Andrew Halaney]

On Fri, Dec 01, 2023 at 08:44:09PM +0530, Manivannan Sadhasivam wrote:
> core_reset is optional, so there is no need to warn the user if it is not
> available (that too not while doing host reset each time).

What's the bit in the parenthesis mean here? I'm having a hard time
following. Otherwise, this looks good to me.

> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/ufs/host/ufs-qcom.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index dc93b1c5ca74..d474de0739e4 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -296,10 +296,8 @@ static int ufs_qcom_host_reset(struct ufs_hba *hba)
>  	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>  	bool reenable_intr;
>  
> -	if (!host->core_reset) {
> -		dev_warn(hba->dev, "%s: reset control not set\n", __func__);
> +	if (!host->core_reset)
>  		return 0;
> -	}
>  
>  	reenable_intr = hba->is_irq_enabled;
>  	disable_irq(hba->irq);
> -- 
> 2.25.1
> 
> 

Re: [PATCH 05/13] scsi: ufs: qcom: Remove the warning message when core_reset is not available

[Manivannan Sadhasivam]

On Wed, Dec 06, 2023 at 12:36:41PM -0600, Andrew Halaney wrote:
> On Fri, Dec 01, 2023 at 08:44:09PM +0530, Manivannan Sadhasivam wrote:
> > core_reset is optional, so there is no need to warn the user if it is not
> > available (that too not while doing host reset each time).
> 
> What's the bit in the parenthesis mean here? I'm having a hard time
> following. Otherwise, this looks good to me.
> 

I was just mentioning that the core reset can happen multiple times depending on
the scenario, so it doesn't make sense to print a warning each time if the reset
was not available.

- Mani

> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/ufs/host/ufs-qcom.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> > index dc93b1c5ca74..d474de0739e4 100644
> > --- a/drivers/ufs/host/ufs-qcom.c
> > +++ b/drivers/ufs/host/ufs-qcom.c
> > @@ -296,10 +296,8 @@ static int ufs_qcom_host_reset(struct ufs_hba *hba)
> >  	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> >  	bool reenable_intr;
> >  
> > -	if (!host->core_reset) {
> > -		dev_warn(hba->dev, "%s: reset control not set\n", __func__);
> > +	if (!host->core_reset)
> >  		return 0;
> > -	}
> >  
> >  	reenable_intr = hba->is_irq_enabled;
> >  	disable_irq(hba->irq);
> > -- 
> > 2.25.1
> > 
> > 
> 

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

Re: [PATCH 05/13] scsi: ufs: qcom: Remove the warning message when core_reset is not available

[Andrew Halaney]

On Thu, Dec 07, 2023 at 10:48:35AM +0530, Manivannan Sadhasivam wrote:
> On Wed, Dec 06, 2023 at 12:36:41PM -0600, Andrew Halaney wrote:
> > On Fri, Dec 01, 2023 at 08:44:09PM +0530, Manivannan Sadhasivam wrote:
> > > core_reset is optional, so there is no need to warn the user if it is not
> > > available (that too not while doing host reset each time).
> > 
> > What's the bit in the parenthesis mean here? I'm having a hard time
> > following. Otherwise, this looks good to me.
> > 
> 
> I was just mentioning that the core reset can happen multiple times depending on
> the scenario, so it doesn't make sense to print a warning each time if the reset
> was not available.

Ahh gotcha, maybe be a little more verbose in that part on the next
spin? As is I'm struggling to get that from the commit message's text.

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>

> 
> - Mani
> 
> > > 
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> > >  drivers/ufs/host/ufs-qcom.c | 4 +---
> > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> > > index dc93b1c5ca74..d474de0739e4 100644
> > > --- a/drivers/ufs/host/ufs-qcom.c
> > > +++ b/drivers/ufs/host/ufs-qcom.c
> > > @@ -296,10 +296,8 @@ static int ufs_qcom_host_reset(struct ufs_hba *hba)
> > >  	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> > >  	bool reenable_intr;
> > >  
> > > -	if (!host->core_reset) {
> > > -		dev_warn(hba->dev, "%s: reset control not set\n", __func__);
> > > +	if (!host->core_reset)
> > >  		return 0;
> > > -	}
> > >  
> > >  	reenable_intr = hba->is_irq_enabled;
> > >  	disable_irq(hba->irq);
> > > -- 
> > > 2.25.1
> > > 
> > > 
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்
> 

[PATCH 06/13] scsi: ufs: qcom: Export ufshcd_{enable/disable}_irq helpers and make use of them

[Manivannan Sadhasivam]

Instead of duplicating the enable/disable IRQ part, let's export the
helpers available in ufshcd driver and make use of them. This also fixes
the possible redundant IRQ disable before asserting reset (when IRQ was
already disabled).

Fixes: 4a791574a0cc ("scsi: ufs: ufs-qcom: Disable interrupt in reset path")
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/ufs/core/ufshcd.c   | 6 ++++--
 drivers/ufs/host/ufs-qcom.c | 9 +++------
 include/ufs/ufshcd.h        | 2 ++
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 8b1031fb0a44..671facc73921 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -289,21 +289,23 @@ static void ufshcd_wb_toggle_buf_flush_during_h8(struct ufs_hba *hba,
 static void ufshcd_hba_vreg_set_lpm(struct ufs_hba *hba);
 static void ufshcd_hba_vreg_set_hpm(struct ufs_hba *hba);
 
-static inline void ufshcd_enable_irq(struct ufs_hba *hba)
+void ufshcd_enable_irq(struct ufs_hba *hba)
 {
 	if (!hba->is_irq_enabled) {
 		enable_irq(hba->irq);
 		hba->is_irq_enabled = true;
 	}
 }
+EXPORT_SYMBOL_GPL(ufshcd_enable_irq);
 
-static inline void ufshcd_disable_irq(struct ufs_hba *hba)
+void ufshcd_disable_irq(struct ufs_hba *hba)
 {
 	if (hba->is_irq_enabled) {
 		disable_irq(hba->irq);
 		hba->is_irq_enabled = false;
 	}
 }
+EXPORT_SYMBOL_GPL(ufshcd_disable_irq);
 
 static void ufshcd_configure_wb(struct ufs_hba *hba)
 {
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index d474de0739e4..604273a22afd 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -300,8 +300,7 @@ static int ufs_qcom_host_reset(struct ufs_hba *hba)
 		return 0;
 
 	reenable_intr = hba->is_irq_enabled;
-	disable_irq(hba->irq);
-	hba->is_irq_enabled = false;
+	ufshcd_disable_irq(hba);
 
 	ret = reset_control_assert(host->core_reset);
 	if (ret) {
@@ -324,10 +323,8 @@ static int ufs_qcom_host_reset(struct ufs_hba *hba)
 
 	usleep_range(1000, 1100);
 
-	if (reenable_intr) {
-		enable_irq(hba->irq);
-		hba->is_irq_enabled = true;
-	}
+	if (reenable_intr)
+		ufshcd_enable_irq(hba);
 
 	return 0;
 }
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 7f0b2c5599cd..f1fc16ac6af2 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -1231,6 +1231,8 @@ static inline void ufshcd_rmwl(struct ufs_hba *hba, u32 mask, u32 val, u32 reg)
 	ufshcd_writel(hba, tmp, reg);
 }
 
+void ufshcd_enable_irq(struct ufs_hba *hba);
+void ufshcd_disable_irq(struct ufs_hba *hba);
 int ufshcd_alloc_host(struct device *, struct ufs_hba **);
 void ufshcd_dealloc_host(struct ufs_hba *);
 int ufshcd_hba_enable(struct ufs_hba *hba);
-- 
2.25.1

Re: [PATCH 06/13] scsi: ufs: qcom: Export ufshcd_{enable/disable}_irq helpers and make use of them

[Bart Van Assche]

On 12/1/23 07:14, Manivannan Sadhasivam wrote:
> Instead of duplicating the enable/disable IRQ part, let's export the
> helpers available in ufshcd driver and make use of them. This also fixes
> the possible redundant IRQ disable before asserting reset (when IRQ was
> already disabled).

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

[PATCH 07/13] scsi: ufs: qcom: Fail ufs_qcom_power_up_sequence() when core_reset fails

[Manivannan Sadhasivam]

Even though core_reset is optional, a failure during assert/deassert should
be considered fatal, if core_reset is available. So fail
ufs_qcom_power_up_sequence() if an error happens during reset and also get
rid of the redundant warning as the ufs_qcom_host_reset() function itself
prints error messages.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/ufs/host/ufs-qcom.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 604273a22afd..4948dd732aae 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -359,8 +359,7 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
 	/* Reset UFS Host Controller and PHY */
 	ret = ufs_qcom_host_reset(hba);
 	if (ret)
-		dev_warn(hba->dev, "%s: host reset returned %d\n",
-				  __func__, ret);
+		return ret;
 
 	/* phy initialization - calibrate the phy */
 	ret = phy_init(phy);
-- 
2.25.1

Re: [PATCH 07/13] scsi: ufs: qcom: Fail ufs_qcom_power_up_sequence() when core_reset fails

[Andrew Halaney]

On Fri, Dec 01, 2023 at 08:44:11PM +0530, Manivannan Sadhasivam wrote:
> Even though core_reset is optional, a failure during assert/deassert should
> be considered fatal, if core_reset is available. So fail
> ufs_qcom_power_up_sequence() if an error happens during reset and also get
> rid of the redundant warning as the ufs_qcom_host_reset() function itself
> prints error messages.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
> ---
>  drivers/ufs/host/ufs-qcom.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 604273a22afd..4948dd732aae 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -359,8 +359,7 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>  	/* Reset UFS Host Controller and PHY */
>  	ret = ufs_qcom_host_reset(hba);

I noticed that ufs_qcom_host_reset() doesn't return an error if
reset_control_deassert() fails. Can you address this in the next spin of
the series (I don't think its in the following patches that I glanced
through).

Thanks,
Andrew

>  	if (ret)
> -		dev_warn(hba->dev, "%s: host reset returned %d\n",
> -				  __func__, ret);
> +		return ret;
>  
>  	/* phy initialization - calibrate the phy */
>  	ret = phy_init(phy);
> -- 
> 2.25.1
> 
> 

Re: [PATCH 07/13] scsi: ufs: qcom: Fail ufs_qcom_power_up_sequence() when core_reset fails

[Manivannan Sadhasivam]

On Wed, Dec 06, 2023 at 12:41:28PM -0600, Andrew Halaney wrote:
> On Fri, Dec 01, 2023 at 08:44:11PM +0530, Manivannan Sadhasivam wrote:
> > Even though core_reset is optional, a failure during assert/deassert should
> > be considered fatal, if core_reset is available. So fail
> > ufs_qcom_power_up_sequence() if an error happens during reset and also get
> > rid of the redundant warning as the ufs_qcom_host_reset() function itself
> > prints error messages.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
> > ---
> >  drivers/ufs/host/ufs-qcom.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> > index 604273a22afd..4948dd732aae 100644
> > --- a/drivers/ufs/host/ufs-qcom.c
> > +++ b/drivers/ufs/host/ufs-qcom.c
> > @@ -359,8 +359,7 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
> >  	/* Reset UFS Host Controller and PHY */
> >  	ret = ufs_qcom_host_reset(hba);
> 
> I noticed that ufs_qcom_host_reset() doesn't return an error if
> reset_control_deassert() fails. Can you address this in the next spin of
> the series (I don't think its in the following patches that I glanced
> through).
> 

Right. I'll fix it in next version.

- Mani

> Thanks,
> Andrew
> 
> >  	if (ret)
> > -		dev_warn(hba->dev, "%s: host reset returned %d\n",
> > -				  __func__, ret);
> > +		return ret;
> >  
> >  	/* phy initialization - calibrate the phy */
> >  	ret = phy_init(phy);
> > -- 
> > 2.25.1
> > 
> > 
> 
> 

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

[PATCH 08/13] scsi: ufs: qcom: Check the return value of ufs_qcom_power_up_sequence()

[Manivannan Sadhasivam]

If ufs_qcom_power_up_sequence() fails, then it makes no sense to enable
the lane clocks and continue ufshcd_hba_enable(). So let's check the return
value of ufs_qcom_power_up_sequence().

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/ufs/host/ufs-qcom.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 4948dd732aae..e4dd3777a4d4 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -415,7 +415,10 @@ static int ufs_qcom_hce_enable_notify(struct ufs_hba *hba,
 
 	switch (status) {
 	case PRE_CHANGE:
-		ufs_qcom_power_up_sequence(hba);
+		err = ufs_qcom_power_up_sequence(hba);
+		if (err)
+			return err;
+
 		/*
 		 * The PHY PLL output is the source of tx/rx lane symbol
 		 * clocks, hence, enable the lane clocks only after PHY
-- 
2.25.1

Re: [PATCH 08/13] scsi: ufs: qcom: Check the return value of ufs_qcom_power_up_sequence()

[Andrew Halaney]

On Fri, Dec 01, 2023 at 08:44:12PM +0530, Manivannan Sadhasivam wrote:
> If ufs_qcom_power_up_sequence() fails, then it makes no sense to enable
> the lane clocks and continue ufshcd_hba_enable(). So let's check the return
> value of ufs_qcom_power_up_sequence().
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>

> ---
>  drivers/ufs/host/ufs-qcom.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 4948dd732aae..e4dd3777a4d4 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -415,7 +415,10 @@ static int ufs_qcom_hce_enable_notify(struct ufs_hba *hba,
>  
>  	switch (status) {
>  	case PRE_CHANGE:
> -		ufs_qcom_power_up_sequence(hba);
> +		err = ufs_qcom_power_up_sequence(hba);
> +		if (err)
> +			return err;
> +
>  		/*
>  		 * The PHY PLL output is the source of tx/rx lane symbol
>  		 * clocks, hence, enable the lane clocks only after PHY
> -- 
> 2.25.1
> 
> 

Re: [PATCH 08/13] scsi: ufs: qcom: Check the return value of ufs_qcom_power_up_sequence()

[Nitin Rawat]

On 12/1/2023 8:44 PM, Manivannan Sadhasivam wrote:
> If ufs_qcom_power_up_sequence() fails, then it makes no sense to enable
> the lane clocks and continue ufshcd_hba_enable(). So let's check the return
> value of ufs_qcom_power_up_sequence().
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>   drivers/ufs/host/ufs-qcom.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 4948dd732aae..e4dd3777a4d4 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -415,7 +415,10 @@ static int ufs_qcom_hce_enable_notify(struct ufs_hba *hba,
>   
>   	switch (status) {
>   	case PRE_CHANGE:
> -		ufs_qcom_power_up_sequence(hba);
> +		err = ufs_qcom_power_up_sequence(hba);
> +		if (err)
> +			return err;
> +
>   		/*
>   		 * The PHY PLL output is the source of tx/rx lane symbol
>   		 * clocks, hence, enable the lane clocks only after PHY


Reviewed-by: Nitin Rawat <quic_nitirawa@quicinc.com>

[PATCH 09/13] scsi: ufs: qcom: Remove redundant error print for devm_kzalloc() failure

[Manivannan Sadhasivam]

devm_kzalloc() will itself print the error message on failure. So let's get
rid of the redundant error message in ufs_qcom_init().

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/ufs/host/ufs-qcom.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index e4dd3777a4d4..218d22e1efce 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1107,10 +1107,8 @@ static int ufs_qcom_init(struct ufs_hba *hba)
 	struct ufs_clk_info *clki;
 
 	host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
-	if (!host) {
-		dev_err(dev, "%s: no memory for qcom ufs host\n", __func__);
+	if (!host)
 		return -ENOMEM;
-	}
 
 	/* Make a two way bind between the qcom host and the hba */
 	host->hba = hba;
-- 
2.25.1

Re: [PATCH 09/13] scsi: ufs: qcom: Remove redundant error print for devm_kzalloc() failure

[Nitin Rawat]

On 12/1/2023 8:44 PM, Manivannan Sadhasivam wrote:
> devm_kzalloc() will itself print the error message on failure. So let's get
> rid of the redundant error message in ufs_qcom_init().
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>   drivers/ufs/host/ufs-qcom.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index e4dd3777a4d4..218d22e1efce 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -1107,10 +1107,8 @@ static int ufs_qcom_init(struct ufs_hba *hba)
>   	struct ufs_clk_info *clki;
>   
>   	host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
> -	if (!host) {
> -		dev_err(dev, "%s: no memory for qcom ufs host\n", __func__);
> +	if (!host)
>   		return -ENOMEM;
> -	}
>   
>   	/* Make a two way bind between the qcom host and the hba */
>   	host->hba = hba;

Reviewed-by: Nitin Rawat <quic_nitirawa@quicinc.com>

[PATCH 10/13] scsi: ufs: qcom: Use dev_err_probe() to simplify error handling of devm_gpiod_get_optional()

[Manivannan Sadhasivam]

As done in other places, let's use dev_err_probe() to simplify the error
handling while acquiring the device reset gpio using
devm_gpiod_get_optional().

While at it, let's reword the error message to make it clear that the
failure is due to acquiring "device reset gpio".

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/ufs/host/ufs-qcom.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 218d22e1efce..a86f6620abc8 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1146,9 +1146,8 @@ static int ufs_qcom_init(struct ufs_hba *hba)
 	host->device_reset = devm_gpiod_get_optional(dev, "reset",
 						     GPIOD_OUT_HIGH);
 	if (IS_ERR(host->device_reset)) {
-		err = PTR_ERR(host->device_reset);
-		if (err != -EPROBE_DEFER)
-			dev_err(dev, "failed to acquire reset gpio: %d\n", err);
+		err = dev_err_probe(dev, PTR_ERR(host->device_reset),
+				    "Failed to acquire device reset gpio\n");
 		goto out_variant_clear;
 	}
 
-- 
2.25.1

Re: [PATCH 10/13] scsi: ufs: qcom: Use dev_err_probe() to simplify error handling of devm_gpiod_get_optional()

[Andrew Halaney]

On Fri, Dec 01, 2023 at 08:44:14PM +0530, Manivannan Sadhasivam wrote:
> As done in other places, let's use dev_err_probe() to simplify the error
> handling while acquiring the device reset gpio using
> devm_gpiod_get_optional().
> 
> While at it, let's reword the error message to make it clear that the
> failure is due to acquiring "device reset gpio".
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>

> ---
>  drivers/ufs/host/ufs-qcom.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 218d22e1efce..a86f6620abc8 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -1146,9 +1146,8 @@ static int ufs_qcom_init(struct ufs_hba *hba)
>  	host->device_reset = devm_gpiod_get_optional(dev, "reset",
>  						     GPIOD_OUT_HIGH);
>  	if (IS_ERR(host->device_reset)) {
> -		err = PTR_ERR(host->device_reset);
> -		if (err != -EPROBE_DEFER)
> -			dev_err(dev, "failed to acquire reset gpio: %d\n", err);
> +		err = dev_err_probe(dev, PTR_ERR(host->device_reset),
> +				    "Failed to acquire device reset gpio\n");
>  		goto out_variant_clear;
>  	}
>  
> -- 
> 2.25.1
> 
> 

[PATCH 11/13] scsi: ufs: qcom: Remove unused ufs_qcom_hosts struct array

[Manivannan Sadhasivam]

ufs_qcom_hosts array is assigned, but not used anywhere. So let's remove
it.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/ufs/host/ufs-qcom.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index a86f6620abc8..824c006be093 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -90,8 +90,6 @@ static const struct __ufs_qcom_bw_table {
 	[MODE_MAX][0][0]		    = { 7643136,	307200 },
 };
 
-static struct ufs_qcom_host *ufs_qcom_hosts[MAX_UFS_QCOM_HOSTS];
-
 static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
 static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up);
 
@@ -1192,9 +1190,6 @@ static int ufs_qcom_init(struct ufs_hba *hba)
 
 	ufs_qcom_setup_clocks(hba, true, POST_CHANGE);
 
-	if (hba->dev->id < MAX_UFS_QCOM_HOSTS)
-		ufs_qcom_hosts[hba->dev->id] = host;
-
 	ufs_qcom_get_default_testbus_cfg(host);
 	err = ufs_qcom_testbus_config(host);
 	if (err)
-- 
2.25.1

Re: [PATCH 11/13] scsi: ufs: qcom: Remove unused ufs_qcom_hosts struct array

[Andrew Halaney]

On Fri, Dec 01, 2023 at 08:44:15PM +0530, Manivannan Sadhasivam wrote:
> ufs_qcom_hosts array is assigned, but not used anywhere. So let's remove
> it.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/ufs/host/ufs-qcom.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index a86f6620abc8..824c006be093 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -90,8 +90,6 @@ static const struct __ufs_qcom_bw_table {
>  	[MODE_MAX][0][0]		    = { 7643136,	307200 },
>  };
>  
> -static struct ufs_qcom_host *ufs_qcom_hosts[MAX_UFS_QCOM_HOSTS];
> -

I think we can get rid of MAX_UFS_QCOM_HOSTS as well with this change in
place?

>  static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
>  static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up);
>  
> @@ -1192,9 +1190,6 @@ static int ufs_qcom_init(struct ufs_hba *hba)
>  
>  	ufs_qcom_setup_clocks(hba, true, POST_CHANGE);
>  
> -	if (hba->dev->id < MAX_UFS_QCOM_HOSTS)
> -		ufs_qcom_hosts[hba->dev->id] = host;
> -
>  	ufs_qcom_get_default_testbus_cfg(host);
>  	err = ufs_qcom_testbus_config(host);
>  	if (err)
> -- 
> 2.25.1
> 
> 

Re: [PATCH 11/13] scsi: ufs: qcom: Remove unused ufs_qcom_hosts struct array

[Manivannan Sadhasivam]

On Wed, Dec 06, 2023 at 12:54:43PM -0600, Andrew Halaney wrote:
> On Fri, Dec 01, 2023 at 08:44:15PM +0530, Manivannan Sadhasivam wrote:
> > ufs_qcom_hosts array is assigned, but not used anywhere. So let's remove
> > it.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/ufs/host/ufs-qcom.c | 5 -----
> >  1 file changed, 5 deletions(-)
> > 
> > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> > index a86f6620abc8..824c006be093 100644
> > --- a/drivers/ufs/host/ufs-qcom.c
> > +++ b/drivers/ufs/host/ufs-qcom.c
> > @@ -90,8 +90,6 @@ static const struct __ufs_qcom_bw_table {
> >  	[MODE_MAX][0][0]		    = { 7643136,	307200 },
> >  };
> >  
> > -static struct ufs_qcom_host *ufs_qcom_hosts[MAX_UFS_QCOM_HOSTS];
> > -
> 
> I think we can get rid of MAX_UFS_QCOM_HOSTS as well with this change in
> place?
> 

Yes, thanks for spotting.

- Mani

> >  static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
> >  static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up);
> >  
> > @@ -1192,9 +1190,6 @@ static int ufs_qcom_init(struct ufs_hba *hba)
> >  
> >  	ufs_qcom_setup_clocks(hba, true, POST_CHANGE);
> >  
> > -	if (hba->dev->id < MAX_UFS_QCOM_HOSTS)
> > -		ufs_qcom_hosts[hba->dev->id] = host;
> > -
> >  	ufs_qcom_get_default_testbus_cfg(host);
> >  	err = ufs_qcom_testbus_config(host);
> >  	if (err)
> > -- 
> > 2.25.1
> > 
> > 
> 
> 

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

Re: [PATCH 11/13] scsi: ufs: qcom: Remove unused ufs_qcom_hosts struct array

[Andrew Halaney]

On Thu, Dec 07, 2023 at 11:01:59AM +0530, Manivannan Sadhasivam wrote:
> On Wed, Dec 06, 2023 at 12:54:43PM -0600, Andrew Halaney wrote:
> > On Fri, Dec 01, 2023 at 08:44:15PM +0530, Manivannan Sadhasivam wrote:
> > > ufs_qcom_hosts array is assigned, but not used anywhere. So let's remove
> > > it.
> > > 
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> > >  drivers/ufs/host/ufs-qcom.c | 5 -----
> > >  1 file changed, 5 deletions(-)
> > > 
> > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> > > index a86f6620abc8..824c006be093 100644
> > > --- a/drivers/ufs/host/ufs-qcom.c
> > > +++ b/drivers/ufs/host/ufs-qcom.c
> > > @@ -90,8 +90,6 @@ static const struct __ufs_qcom_bw_table {
> > >  	[MODE_MAX][0][0]		    = { 7643136,	307200 },
> > >  };
> > >  
> > > -static struct ufs_qcom_host *ufs_qcom_hosts[MAX_UFS_QCOM_HOSTS];
> > > -
> > 
> > I think we can get rid of MAX_UFS_QCOM_HOSTS as well with this change in
> > place?
> > 
> 
> Yes, thanks for spotting.

With that in place please add:

    Reviewed-by: Andrew Halaney <ahalaney@redhat.com>

> 
> - Mani
> 
> > >  static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
> > >  static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up);
> > >  
> > > @@ -1192,9 +1190,6 @@ static int ufs_qcom_init(struct ufs_hba *hba)
> > >  
> > >  	ufs_qcom_setup_clocks(hba, true, POST_CHANGE);
> > >  
> > > -	if (hba->dev->id < MAX_UFS_QCOM_HOSTS)
> > > -		ufs_qcom_hosts[hba->dev->id] = host;
> > > -
> > >  	ufs_qcom_get_default_testbus_cfg(host);
> > >  	err = ufs_qcom_testbus_config(host);
> > >  	if (err)
> > > -- 
> > > 2.25.1
> > > 
> > > 
> > 
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்
> 

Re: [PATCH 11/13] scsi: ufs: qcom: Remove unused ufs_qcom_hosts struct array

[Nitin Rawat]

On 12/1/2023 8:44 PM, Manivannan Sadhasivam wrote:
> ufs_qcom_hosts array is assigned, but not used anywhere. So let's remove
> it.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>   drivers/ufs/host/ufs-qcom.c | 5 -----
>   1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index a86f6620abc8..824c006be093 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -90,8 +90,6 @@ static const struct __ufs_qcom_bw_table {
>   	[MODE_MAX][0][0]		    = { 7643136,	307200 },
>   };
>   
> -static struct ufs_qcom_host *ufs_qcom_hosts[MAX_UFS_QCOM_HOSTS];
> -
>   static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
>   static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up);
>   
> @@ -1192,9 +1190,6 @@ static int ufs_qcom_init(struct ufs_hba *hba)
>   
>   	ufs_qcom_setup_clocks(hba, true, POST_CHANGE);
>   
> -	if (hba->dev->id < MAX_UFS_QCOM_HOSTS)
> -		ufs_qcom_hosts[hba->dev->id] = host;
> -
>   	ufs_qcom_get_default_testbus_cfg(host);
>   	err = ufs_qcom_testbus_config(host);
>   	if (err)

Hi Mani,
There is plan to upstream 2nd UFS instance change which may need 
ufs_qcom_hosts. But for now this is unused and can be removed.

Anyways change Looks Good. Hence

Reviewed-by: Nitin Rawat <quic_nitirawa@quicinc.com>


Regards,
Nitin

[PATCH 12/13] scsi: ufs: qcom: Sort includes alphabetically

[Manivannan Sadhasivam]

Sort includes alphabetically.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/ufs/host/ufs-qcom.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 824c006be093..590a2c67cf7d 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -4,26 +4,26 @@
  */
 
 #include <linux/acpi.h>
-#include <linux/time.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/devfreq.h>
+#include <linux/gpio/consumer.h>
 #include <linux/interconnect.h>
 #include <linux/module.h>
 #include <linux/of.h>
-#include <linux/platform_device.h>
 #include <linux/phy/phy.h>
-#include <linux/gpio/consumer.h>
+#include <linux/platform_device.h>
 #include <linux/reset-controller.h>
-#include <linux/devfreq.h>
+#include <linux/time.h>
 
 #include <soc/qcom/ice.h>
 
 #include <ufs/ufshcd.h>
-#include "ufshcd-pltfrm.h"
-#include <ufs/unipro.h>
-#include "ufs-qcom.h"
 #include <ufs/ufshci.h>
 #include <ufs/ufs_quirks.h>
+#include <ufs/unipro.h>
+#include "ufshcd-pltfrm.h"
+#include "ufs-qcom.h"
 
 #define MCQ_QCFGPTR_MASK	GENMASK(7, 0)
 #define MCQ_QCFGPTR_UNIT	0x200
-- 
2.25.1

Re: [PATCH 12/13] scsi: ufs: qcom: Sort includes alphabetically

[Nitin Rawat]

On 12/1/2023 8:44 PM, Manivannan Sadhasivam wrote:
> Sort includes alphabetically.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>   drivers/ufs/host/ufs-qcom.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 824c006be093..590a2c67cf7d 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -4,26 +4,26 @@
>    */
>   
>   #include <linux/acpi.h>
> -#include <linux/time.h>
>   #include <linux/clk.h>
>   #include <linux/delay.h>
> +#include <linux/devfreq.h>
> +#include <linux/gpio/consumer.h>
>   #include <linux/interconnect.h>
>   #include <linux/module.h>
>   #include <linux/of.h>
> -#include <linux/platform_device.h>
>   #include <linux/phy/phy.h>
> -#include <linux/gpio/consumer.h>
> +#include <linux/platform_device.h>
>   #include <linux/reset-controller.h>
> -#include <linux/devfreq.h>
> +#include <linux/time.h>
>   
>   #include <soc/qcom/ice.h>
>   
>   #include <ufs/ufshcd.h>
> -#include "ufshcd-pltfrm.h"
> -#include <ufs/unipro.h>
> -#include "ufs-qcom.h"
>   #include <ufs/ufshci.h>
>   #include <ufs/ufs_quirks.h>
> +#include <ufs/unipro.h>
> +#include "ufshcd-pltfrm.h"
> +#include "ufs-qcom.h"
>   
>   #define MCQ_QCFGPTR_MASK	GENMASK(7, 0)
>   #define MCQ_QCFGPTR_UNIT	0x200

Reviewed-by: Nitin Rawat <quic_nitirawa@quicinc.com>

[PATCH 13/13] scsi: ufs: qcom: Initialize cycles_in_1us variable in ufs_qcom_set_core_clk_ctrl()

[Manivannan Sadhasivam]

In case the "core_clk_unipro" clock is not provided, "cycles_in_1us"
variable will be used as uninitialized. So initialize it with 0.

Issue reported by Smatch tool:

drivers/ufs/host/ufs-qcom.c:1336 ufs_qcom_set_core_clk_ctrl() error: uninitialized symbol 'cycles_in_1us'.
drivers/ufs/host/ufs-qcom.c:1341 ufs_qcom_set_core_clk_ctrl() error: uninitialized symbol 'cycles_in_1us'.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/ufs/host/ufs-qcom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 590a2c67cf7d..208543a62d43 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1296,7 +1296,7 @@ static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up)
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 	struct list_head *head = &hba->clk_list_head;
 	struct ufs_clk_info *clki;
-	u32 cycles_in_1us;
+	u32 cycles_in_1us = 0;
 	u32 core_clk_ctrl_reg;
 	int err;
 
-- 
2.25.1

Re: [PATCH 13/13] scsi: ufs: qcom: Initialize cycles_in_1us variable in ufs_qcom_set_core_clk_ctrl()

[Nitin Rawat]

On 12/1/2023 8:44 PM, Manivannan Sadhasivam wrote:
> In case the "core_clk_unipro" clock is not provided, "cycles_in_1us"
> variable will be used as uninitialized. So initialize it with 0.
> 
> Issue reported by Smatch tool:
> 
> drivers/ufs/host/ufs-qcom.c:1336 ufs_qcom_set_core_clk_ctrl() error: uninitialized symbol 'cycles_in_1us'.
> drivers/ufs/host/ufs-qcom.c:1341 ufs_qcom_set_core_clk_ctrl() error: uninitialized symbol 'cycles_in_1us'.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>   drivers/ufs/host/ufs-qcom.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 590a2c67cf7d..208543a62d43 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -1296,7 +1296,7 @@ static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up)
>   	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>   	struct list_head *head = &hba->clk_list_head;
>   	struct ufs_clk_info *clki;
> -	u32 cycles_in_1us;
> +	u32 cycles_in_1us = 0;
>   	u32 core_clk_ctrl_reg;
>   	int err;
>   

Reviewed-by: Nitin Rawat <quic_nitirawa@quicinc.com>