[PATCH v2 00/17] scsi: ufs: qcom: Code cleanups

[PATCH v2 00/17] scsi: ufs: qcom: Code cleanups

[Manivannan Sadhasivam]

Hello,

This series has code some cleanups to the Qcom UFS driver. No functional
change. In this version, I've removed code supporting legacy controllers
ver < 2.0, as the respective platforms were never supported in upstream.

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

- Mani

Changes in v2:

* Collected review tags
* Fixed the comments from Andrew
* Added a few more patches, most notably one removing the code for old
  controllers (ver < v2.0)

Manivannan Sadhasivam (17):
  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()
  scsi: ufs: qcom: Simplify ufs_qcom_{assert/deassert}_reset
  scsi: ufs: qcom: Remove support for host controllers older than v2.0
  scsi: ufs: qcom: Use ufshcd_rmwl() where applicable
  scsi: ufs: qcom: Remove unused definitions

 drivers/ufs/core/ufshcd.c   |   6 +-
 drivers/ufs/host/ufs-qcom.c | 377 +++++-------------------------------
 drivers/ufs/host/ufs-qcom.h |  52 +----
 include/ufs/ufshcd.h        |   2 +
 4 files changed, 66 insertions(+), 371 deletions(-)

-- 
2.25.1

[PATCH v2 01/17] 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.

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
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

[PATCH v2 02/17] 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")
Reviewed-by: Abel Vesa <abel.vesa@linaro.org>
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

[PATCH v2 03/17] 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")
Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
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

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

[Manivannan Sadhasivam]

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

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
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

[PATCH v2 05/17] 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.

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
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 v2 05/17] scsi: ufs: qcom: Remove the warning message when core_reset is not available

[Nitin Rawat]

On 12/8/2023 12:28 PM, Manivannan Sadhasivam wrote:
> core_reset is optional, so there is no need to warn the user if it is not
> available.
> 
> Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
> 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);


Hi Mani,

I think core reset is not frequent. It happen during only probe ,error 
handler.

core reset is needed in kernel to cleanup UFS phy and controller 
configuration before UFS HLOS operation starts as per HPG.

Having existing warn print can be used to to debug or atleast know 
core_reset is missed in device tree to give indication complete reset 
hasn't been done and we could still be operating in bootloader 
configuration.


Regards,
Nitin

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

[Manivannan Sadhasivam]

On Fri, Dec 08, 2023 at 02:55:21PM +0530, Nitin Rawat wrote:
> 
> 
> On 12/8/2023 12:28 PM, Manivannan Sadhasivam wrote:
> > core_reset is optional, so there is no need to warn the user if it is not
> > available.
> > 
> > Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
> > 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);
> 
> 
> Hi Mani,
> 
> I think core reset is not frequent. It happen during only probe ,error
> handler.
> 
> core reset is needed in kernel to cleanup UFS phy and controller
> configuration before UFS HLOS operation starts as per HPG.
> 

This sounds like core reset is not an optional property but a required one. I
just checked the upstream DT files for all SoCs, and looks like pretty much all
of them support core reset.

Only MSM8996 doesn't have the reset property, but the reset is available in GCC.
So we should be able to use it in dtsi.

I also skimmed through the HPG and looks like core reset is not optional. Please
confirm.

- Mani

> Having existing warn print can be used to to debug or atleast know
> core_reset is missed in device tree to give indication complete reset hasn't
> been done and we could still be operating in bootloader configuration.
> 
> 
> Regards,
> Nitin
> 

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

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

[Nitin Rawat]

On 12/8/2023 3:58 PM, Manivannan Sadhasivam wrote:
> On Fri, Dec 08, 2023 at 02:55:21PM +0530, Nitin Rawat wrote:
>>
>>
>> On 12/8/2023 12:28 PM, Manivannan Sadhasivam wrote:
>>> core_reset is optional, so there is no need to warn the user if it is not
>>> available.
>>>
>>> Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
>>> 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);
>>
>>
>> Hi Mani,
>>
>> I think core reset is not frequent. It happen during only probe ,error
>> handler.
>>
>> core reset is needed in kernel to cleanup UFS phy and controller
>> configuration before UFS HLOS operation starts as per HPG.
>>
> 
> This sounds like core reset is not an optional property but a required one. I
> just checked the upstream DT files for all SoCs, and looks like pretty much all
> of them support core reset.
> 
> Only MSM8996 doesn't have the reset property, but the reset is available in GCC.
> So we should be able to use it in dtsi.
> 
> I also skimmed through the HPG and looks like core reset is not optional. Please
> confirm.
> 
> - Mani


Hi Mani,

Yes Core_reset is part of HPG sequence and is needed.

Regards,
Nitin


> 
>> Having existing warn print can be used to to debug or atleast know
>> core_reset is missed in device tree to give indication complete reset hasn't
>> been done and we could still be operating in bootloader configuration.
>>
>>
>> Regards,
>> Nitin
>>
> 

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

[Nitin Rawat]

On 12/8/2023 6:59 PM, Nitin Rawat wrote:
> 
> 
> On 12/8/2023 3:58 PM, Manivannan Sadhasivam wrote:
>> On Fri, Dec 08, 2023 at 02:55:21PM +0530, Nitin Rawat wrote:
>>>
>>>
>>> On 12/8/2023 12:28 PM, Manivannan Sadhasivam wrote:
>>>> core_reset is optional, so there is no need to warn the user if it 
>>>> is not
>>>> available.
>>>>
>>>> Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
>>>> 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);
>>>
>>>
>>> Hi Mani,
>>>
>>> I think core reset is not frequent. It happen during only probe ,error
>>> handler.
>>>
>>> core reset is needed in kernel to cleanup UFS phy and controller
>>> configuration before UFS HLOS operation starts as per HPG.
>>>
>>
>> This sounds like core reset is not an optional property but a required 
>> one. I
>> just checked the upstream DT files for all SoCs, and looks like pretty 
>> much all
>> of them support core reset.
>>
>> Only MSM8996 doesn't have the reset property, but the reset is 
>> available in GCC.
>> So we should be able to use it in dtsi.
>>
>> I also skimmed through the HPG and looks like core reset is not 
>> optional. Please
>> confirm.
>>
>> - Mani
> 
> 
> Hi Mani,
> 
> Yes Core_reset is part of HPG sequence and is needed.
> 
> Regards,
> Nitin


Hi Mani,

I see this patch series is merged . So planning to keep the warn message
based on above discussion.

Regards,
Nitin
> 
> 
>>
>>> Having existing warn print can be used to to debug or atleast know
>>> core_reset is missed in device tree to give indication complete reset 
>>> hasn't
>>> been done and we could still be operating in bootloader configuration.
>>>
>>>
>>> Regards,
>>> Nitin
>>>
>>
> 

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

[Manivannan Sadhasivam]

On Thu, Dec 14, 2023 at 12:43:02PM +0530, Nitin Rawat wrote:
> 
> 
> On 12/8/2023 6:59 PM, Nitin Rawat wrote:
> > 
> > 
> > On 12/8/2023 3:58 PM, Manivannan Sadhasivam wrote:
> > > On Fri, Dec 08, 2023 at 02:55:21PM +0530, Nitin Rawat wrote:
> > > > 
> > > > 
> > > > On 12/8/2023 12:28 PM, Manivannan Sadhasivam wrote:
> > > > > core_reset is optional, so there is no need to warn the user
> > > > > if it is not
> > > > > available.
> > > > > 
> > > > > Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
> > > > > 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);
> > > > 
> > > > 
> > > > Hi Mani,
> > > > 
> > > > I think core reset is not frequent. It happen during only probe ,error
> > > > handler.
> > > > 
> > > > core reset is needed in kernel to cleanup UFS phy and controller
> > > > configuration before UFS HLOS operation starts as per HPG.
> > > > 
> > > 
> > > This sounds like core reset is not an optional property but a
> > > required one. I
> > > just checked the upstream DT files for all SoCs, and looks like
> > > pretty much all
> > > of them support core reset.
> > > 
> > > Only MSM8996 doesn't have the reset property, but the reset is
> > > available in GCC.
> > > So we should be able to use it in dtsi.
> > > 
> > > I also skimmed through the HPG and looks like core reset is not
> > > optional. Please
> > > confirm.
> > > 
> > > - Mani
> > 
> > 
> > Hi Mani,
> > 
> > Yes Core_reset is part of HPG sequence and is needed.
> > 
> > Regards,
> > Nitin
> 
> 
> Hi Mani,
> 
> I see this patch series is merged . So planning to keep the warn message
> based on above discussion.
> 

No, you should not add that warning back. As per our discussion, we concluded
that core_reset is not optional, so devm_reset_control_get_exclusive() should be
used to acquire the reset.

If the reset property is not present in DT, then ufs_qcom_init() will fail. This
also means that we should fix the binding and DT of SoCs missing the reset
property.

- Mani

> Regards,
> Nitin
> > 
> > 
> > > 
> > > > Having existing warn print can be used to to debug or atleast know
> > > > core_reset is missed in device tree to give indication complete
> > > > reset hasn't
> > > > been done and we could still be operating in bootloader configuration.
> > > > 
> > > > 
> > > > Regards,
> > > > Nitin
> > > > 
> > > 
> > 

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

[PATCH v2 06/17] 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")
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
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

[PATCH v2 07/17] 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.

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/ufs/host/ufs-qcom.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 604273a22afd..365a61dbf7ea 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -317,9 +317,11 @@ static int ufs_qcom_host_reset(struct ufs_hba *hba)
 	usleep_range(200, 210);
 
 	ret = reset_control_deassert(host->core_reset);
-	if (ret)
+	if (ret) {
 		dev_err(hba->dev, "%s: core_reset deassert failed, err = %d\n",
 				 __func__, ret);
+		return ret;
+	}
 
 	usleep_range(1000, 1100);
 
@@ -359,8 +361,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

[PATCH v2 08/17] 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().

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
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 365a61dbf7ea..b141dd2a9346 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -417,7 +417,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

[PATCH v2 09/17] 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 b141dd2a9346..05a9a25bc34c 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1109,10 +1109,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 v2 09/17] scsi: ufs: qcom: Remove redundant error print for devm_kzalloc() failure

[Nitin Rawat]

On 12/8/2023 12:28 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 b141dd2a9346..05a9a25bc34c 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -1109,10 +1109,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 v2 10/17] 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".

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
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 05a9a25bc34c..ae9fafa49ded 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1148,9 +1148,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 v2 11/17] 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.

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/ufs/host/ufs-qcom.c | 5 -----
 drivers/ufs/host/ufs-qcom.h | 1 -
 2 files changed, 6 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index ae9fafa49ded..18ea41f5bae9 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);
 
@@ -1194,9 +1192,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)
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index e2df4c528a2a..53db424a0bcb 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -10,7 +10,6 @@
 #include <soc/qcom/ice.h>
 #include <ufs/ufshcd.h>
 
-#define MAX_UFS_QCOM_HOSTS	1
 #define MAX_U32                 (~(u32)0)
 #define MPHY_TX_FSM_STATE       0x41
 #define TX_FSM_HIBERN8          0x1
-- 
2.25.1

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

[Manivannan Sadhasivam]

Sort includes alphabetically.

Reviewed-by: Nitin Rawat <quic_nitirawa@quicinc.com>
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 18ea41f5bae9..7fbd35a3eb81 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

[PATCH v2 13/17] 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'.

Reviewed-by: Nitin Rawat <quic_nitirawa@quicinc.com>
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 7fbd35a3eb81..9b3d6d3609c9 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1298,7 +1298,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

[PATCH v2 14/17] scsi: ufs: qcom: Simplify ufs_qcom_{assert/deassert}_reset

[Manivannan Sadhasivam]

In both the functions, UFS_PHY_SOFT_RESET contains the mask of the reset
bit. So this can be passed directly as the value to be written for
asserting the reset. For deasserting, 0 can be passed.

This gets rid of the FIELD_PREP() inside these functions and also
UFS_PHY_RESET_{ENABLE/DISABLE} definitions.

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

diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index 53db424a0bcb..a109d3359db4 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -92,9 +92,6 @@ enum {
 #define TEST_BUS_SEL		GENMASK(22, 19)
 #define UFS_REG_TEST_BUS_EN	BIT(30)
 
-#define UFS_PHY_RESET_ENABLE	1
-#define UFS_PHY_RESET_DISABLE	0
-
 /* bit definitions for REG_UFS_CFG2 register */
 #define UAWM_HW_CGC_EN		BIT(0)
 #define UARM_HW_CGC_EN		BIT(1)
@@ -157,8 +154,7 @@ ufs_qcom_get_controller_revision(struct ufs_hba *hba,
 
 static inline void ufs_qcom_assert_reset(struct ufs_hba *hba)
 {
-	ufshcd_rmwl(hba, UFS_PHY_SOFT_RESET, FIELD_PREP(UFS_PHY_SOFT_RESET, UFS_PHY_RESET_ENABLE),
-		    REG_UFS_CFG1);
+	ufshcd_rmwl(hba, UFS_PHY_SOFT_RESET, UFS_PHY_SOFT_RESET, REG_UFS_CFG1);
 
 	/*
 	 * Make sure assertion of ufs phy reset is written to
@@ -169,8 +165,7 @@ static inline void ufs_qcom_assert_reset(struct ufs_hba *hba)
 
 static inline void ufs_qcom_deassert_reset(struct ufs_hba *hba)
 {
-	ufshcd_rmwl(hba, UFS_PHY_SOFT_RESET, FIELD_PREP(UFS_PHY_SOFT_RESET, UFS_PHY_RESET_DISABLE),
-		    REG_UFS_CFG1);
+	ufshcd_rmwl(hba, UFS_PHY_SOFT_RESET, 0, REG_UFS_CFG1);
 
 	/*
 	 * Make sure de-assertion of ufs phy reset is written to
-- 
2.25.1

Re: [PATCH v2 14/17] scsi: ufs: qcom: Simplify ufs_qcom_{assert/deassert}_reset

[Andrew Halaney]

On Fri, Dec 08, 2023 at 12:28:59PM +0530, Manivannan Sadhasivam wrote:
> In both the functions, UFS_PHY_SOFT_RESET contains the mask of the reset
> bit. So this can be passed directly as the value to be written for
> asserting the reset. For deasserting, 0 can be passed.
> 
> This gets rid of the FIELD_PREP() inside these functions and also
> UFS_PHY_RESET_{ENABLE/DISABLE} definitions.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

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

> ---
>  drivers/ufs/host/ufs-qcom.h | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
> index 53db424a0bcb..a109d3359db4 100644
> --- a/drivers/ufs/host/ufs-qcom.h
> +++ b/drivers/ufs/host/ufs-qcom.h
> @@ -92,9 +92,6 @@ enum {
>  #define TEST_BUS_SEL		GENMASK(22, 19)
>  #define UFS_REG_TEST_BUS_EN	BIT(30)
>  
> -#define UFS_PHY_RESET_ENABLE	1
> -#define UFS_PHY_RESET_DISABLE	0
> -
>  /* bit definitions for REG_UFS_CFG2 register */
>  #define UAWM_HW_CGC_EN		BIT(0)
>  #define UARM_HW_CGC_EN		BIT(1)
> @@ -157,8 +154,7 @@ ufs_qcom_get_controller_revision(struct ufs_hba *hba,
>  
>  static inline void ufs_qcom_assert_reset(struct ufs_hba *hba)
>  {
> -	ufshcd_rmwl(hba, UFS_PHY_SOFT_RESET, FIELD_PREP(UFS_PHY_SOFT_RESET, UFS_PHY_RESET_ENABLE),
> -		    REG_UFS_CFG1);
> +	ufshcd_rmwl(hba, UFS_PHY_SOFT_RESET, UFS_PHY_SOFT_RESET, REG_UFS_CFG1);
>  
>  	/*
>  	 * Make sure assertion of ufs phy reset is written to
> @@ -169,8 +165,7 @@ static inline void ufs_qcom_assert_reset(struct ufs_hba *hba)
>  
>  static inline void ufs_qcom_deassert_reset(struct ufs_hba *hba)
>  {
> -	ufshcd_rmwl(hba, UFS_PHY_SOFT_RESET, FIELD_PREP(UFS_PHY_SOFT_RESET, UFS_PHY_RESET_DISABLE),
> -		    REG_UFS_CFG1);
> +	ufshcd_rmwl(hba, UFS_PHY_SOFT_RESET, 0, REG_UFS_CFG1);
>  
>  	/*
>  	 * Make sure de-assertion of ufs phy reset is written to
> -- 
> 2.25.1
> 

[PATCH v2 15/17] scsi: ufs: qcom: Remove support for host controllers older than v2.0

[Manivannan Sadhasivam]

The legacy platforms making use of host controllers older than version 2.0
are not supported in upstream. So there is no need to carry code to support
them.

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

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 9b3d6d3609c9..26aa8904c823 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -274,9 +274,7 @@ static int ufs_qcom_check_hibern8(struct ufs_hba *hba)
 
 static void ufs_qcom_select_unipro_mode(struct ufs_qcom_host *host)
 {
-	ufshcd_rmwl(host->hba, QUNIPRO_SEL,
-		   ufs_qcom_cap_qunipro(host) ? QUNIPRO_SEL : 0,
-		   REG_UFS_CFG1);
+	ufshcd_rmwl(host->hba, QUNIPRO_SEL, QUNIPRO_SEL, REG_UFS_CFG1);
 
 	if (host->hw_ver.major >= 0x05)
 		ufshcd_rmwl(host->hba, QUNIPRO_G4_SEL, 0, REG_UFS_CFG0);
@@ -333,18 +331,8 @@ static u32 ufs_qcom_get_hs_gear(struct ufs_hba *hba)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 
-	if (host->hw_ver.major == 0x1) {
-		/*
-		 * HS-G3 operations may not reliably work on legacy QCOM
-		 * UFS host controller hardware even though capability
-		 * exchange during link startup phase may end up
-		 * negotiating maximum supported gear as G3.
-		 * Hence downgrade the maximum supported gear to HS-G2.
-		 */
-		return UFS_HS_G2;
-	} else if (host->hw_ver.major >= 0x4) {
+	if (host->hw_ver.major >= 0x4)
 		return UFS_QCOM_MAX_GEAR(ufshcd_readl(hba, REG_UFS_PARAM0));
-	}
 
 	/* Default is HS-G3 */
 	return UFS_HS_G3;
@@ -457,41 +445,16 @@ static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 	struct ufs_clk_info *clki;
-	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;
 
-	static u32 pwm_fr_table[][2] = {
-		{UFS_PWM_G1, 0x1},
-		{UFS_PWM_G2, 0x1},
-		{UFS_PWM_G3, 0x1},
-		{UFS_PWM_G4, 0x1},
-	};
-
-	static u32 hs_fr_table_rA[][2] = {
-		{UFS_HS_G1, 0x1F},
-		{UFS_HS_G2, 0x3e},
-		{UFS_HS_G3, 0x7D},
-	};
-
-	static u32 hs_fr_table_rB[][2] = {
-		{UFS_HS_G1, 0x24},
-		{UFS_HS_G2, 0x49},
-		{UFS_HS_G3, 0x92},
-	};
-
 	/*
-	 * The Qunipro controller does not use following registers:
-	 * SYS1CLK_1US_REG, TX_SYMBOL_CLK_1US_REG, CLK_NS_REG &
-	 * UFS_REG_PA_LINK_STARTUP_TIMER.
-	 * However UTP controller uses SYS1CLK_1US_REG register for Interrupt
+	 * UTP controller uses SYS1CLK_1US_REG register for Interrupt
 	 * Aggregation logic.
 	 * It is mandatory to write SYS1CLK_1US_REG register on UFS host
 	 * controller V4.0.0 onwards.
 	 */
-	if (host->hw_ver.major < 4 && ufs_qcom_cap_qunipro(host) &&
-	    !ufshcd_is_intr_aggr_allowed(hba))
+	if (host->hw_ver.major < 4 && !ufshcd_is_intr_aggr_allowed(hba))
 		return 0;
 
 	if (gear == 0) {
@@ -524,79 +487,6 @@ static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
 		mb();
 	}
 
-	if (ufs_qcom_cap_qunipro(host))
-		return 0;
-
-	core_clk_period_in_ns = NSEC_PER_SEC / core_clk_rate;
-	core_clk_period_in_ns <<= OFFSET_CLK_NS_REG;
-	core_clk_period_in_ns &= MASK_CLK_NS_REG;
-
-	switch (hs) {
-	case FASTAUTO_MODE:
-	case FAST_MODE:
-		if (rate == PA_HS_MODE_A) {
-			if (gear > ARRAY_SIZE(hs_fr_table_rA)) {
-				dev_err(hba->dev,
-					"%s: index %d exceeds table size %zu\n",
-					__func__, gear,
-					ARRAY_SIZE(hs_fr_table_rA));
-				return -EINVAL;
-			}
-			tx_clk_cycles_per_us = hs_fr_table_rA[gear-1][1];
-		} else if (rate == PA_HS_MODE_B) {
-			if (gear > ARRAY_SIZE(hs_fr_table_rB)) {
-				dev_err(hba->dev,
-					"%s: index %d exceeds table size %zu\n",
-					__func__, gear,
-					ARRAY_SIZE(hs_fr_table_rB));
-				return -EINVAL;
-			}
-			tx_clk_cycles_per_us = hs_fr_table_rB[gear-1][1];
-		} else {
-			dev_err(hba->dev, "%s: invalid rate = %d\n",
-				__func__, rate);
-			return -EINVAL;
-		}
-		break;
-	case SLOWAUTO_MODE:
-	case SLOW_MODE:
-		if (gear > ARRAY_SIZE(pwm_fr_table)) {
-			dev_err(hba->dev,
-					"%s: index %d exceeds table size %zu\n",
-					__func__, gear,
-					ARRAY_SIZE(pwm_fr_table));
-			return -EINVAL;
-		}
-		tx_clk_cycles_per_us = pwm_fr_table[gear-1][1];
-		break;
-	case UNCHANGED:
-	default:
-		dev_err(hba->dev, "%s: invalid mode = %d\n", __func__, hs);
-		return -EINVAL;
-	}
-
-	if (ufshcd_readl(hba, REG_UFS_TX_SYMBOL_CLK_NS_US) !=
-	    (core_clk_period_in_ns | tx_clk_cycles_per_us)) {
-		/* this register 2 fields shall be written at once */
-		ufshcd_writel(hba, core_clk_period_in_ns | tx_clk_cycles_per_us,
-			      REG_UFS_TX_SYMBOL_CLK_NS_US);
-		/*
-		 * make sure above write gets applied before we return from
-		 * this function.
-		 */
-		mb();
-	}
-
-	if (update_link_startup_timer && host->hw_ver.major != 0x5) {
-		ufshcd_writel(hba, ((core_clk_rate / MSEC_PER_SEC) * 100),
-			      REG_UFS_CFG0);
-		/*
-		 * make sure that this configuration is applied before
-		 * we return
-		 */
-		mb();
-	}
-
 	return 0;
 }
 
@@ -604,7 +494,6 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
 					enum ufs_notify_change_status status)
 {
 	int err = 0;
-	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 
 	switch (status) {
 	case PRE_CHANGE:
@@ -615,11 +504,9 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
 			return -EINVAL;
 		}
 
-		if (ufs_qcom_cap_qunipro(host)) {
-			err = ufs_qcom_set_core_clk_ctrl(hba, true);
-			if (err)
-				dev_err(hba->dev, "cfg core clk ctrl failed\n");
-		}
+		err = ufs_qcom_set_core_clk_ctrl(hba, true);
+		if (err)
+			dev_err(hba->dev, "cfg core clk ctrl failed\n");
 		/*
 		 * Some UFS devices (and may be host) have issues if LCC is
 		 * enabled. So we are setting PA_Local_TX_LCC_Enable to 0
@@ -918,12 +805,7 @@ static int ufs_qcom_apply_dev_quirks(struct ufs_hba *hba)
 
 static u32 ufs_qcom_get_ufs_hci_version(struct ufs_hba *hba)
 {
-	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
-
-	if (host->hw_ver.major == 0x1)
-		return ufshci_version(1, 1);
-	else
-		return ufshci_version(2, 0);
+	return ufshci_version(2, 0);
 }
 
 /**
@@ -939,46 +821,21 @@ static void ufs_qcom_advertise_quirks(struct ufs_hba *hba)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 
-	if (host->hw_ver.major == 0x01) {
-		hba->quirks |= UFSHCD_QUIRK_DELAY_BEFORE_DME_CMDS
-			    | UFSHCD_QUIRK_BROKEN_PA_RXHSUNTERMCAP
-			    | UFSHCD_QUIRK_DME_PEER_ACCESS_AUTO_MODE;
-
-		if (host->hw_ver.minor == 0x0001 && host->hw_ver.step == 0x0001)
-			hba->quirks |= UFSHCD_QUIRK_BROKEN_INTR_AGGR;
-
-		hba->quirks |= UFSHCD_QUIRK_BROKEN_LCC;
-	}
-
-	if (host->hw_ver.major == 0x2) {
+	if (host->hw_ver.major == 0x2)
 		hba->quirks |= UFSHCD_QUIRK_BROKEN_UFS_HCI_VERSION;
 
-		if (!ufs_qcom_cap_qunipro(host))
-			/* Legacy UniPro mode still need following quirks */
-			hba->quirks |= (UFSHCD_QUIRK_DELAY_BEFORE_DME_CMDS
-				| UFSHCD_QUIRK_DME_PEER_ACCESS_AUTO_MODE
-				| UFSHCD_QUIRK_BROKEN_PA_RXHSUNTERMCAP);
-	}
-
 	if (host->hw_ver.major > 0x3)
 		hba->quirks |= UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH;
 }
 
 static void ufs_qcom_set_caps(struct ufs_hba *hba)
 {
-	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
-
 	hba->caps |= UFSHCD_CAP_CLK_GATING | UFSHCD_CAP_HIBERN8_WITH_CLK_GATING;
 	hba->caps |= UFSHCD_CAP_CLK_SCALING | UFSHCD_CAP_WB_WITH_CLK_SCALING;
 	hba->caps |= UFSHCD_CAP_AUTO_BKOPS_SUSPEND;
 	hba->caps |= UFSHCD_CAP_WB_EN;
 	hba->caps |= UFSHCD_CAP_AGGR_POWER_COLLAPSE;
 	hba->caps |= UFSHCD_CAP_RPM_AUTOSUSPEND;
-
-	if (host->hw_ver.major >= 0x2) {
-		host->caps = UFS_QCOM_CAP_QUNIPRO |
-			     UFS_QCOM_CAP_RETAIN_SEC_CFG_AFTER_PWR_COLLAPSE;
-	}
 }
 
 /**
@@ -1101,9 +958,7 @@ static int ufs_qcom_init(struct ufs_hba *hba)
 {
 	int err;
 	struct device *dev = hba->dev;
-	struct platform_device *pdev = to_platform_device(dev);
 	struct ufs_qcom_host *host;
-	struct resource *res;
 	struct ufs_clk_info *clki;
 
 	host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
@@ -1154,25 +1009,8 @@ static int ufs_qcom_init(struct ufs_hba *hba)
 	ufs_qcom_get_controller_revision(hba, &host->hw_ver.major,
 		&host->hw_ver.minor, &host->hw_ver.step);
 
-	/*
-	 * for newer controllers, device reference clock control bit has
-	 * moved inside UFS controller register address space itself.
-	 */
-	if (host->hw_ver.major >= 0x02) {
-		host->dev_ref_clk_ctrl_mmio = hba->mmio_base + REG_UFS_CFG1;
-		host->dev_ref_clk_en_mask = BIT(26);
-	} else {
-		/* "dev_ref_clk_ctrl_mem" is optional resource */
-		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
-						   "dev_ref_clk_ctrl_mem");
-		if (res) {
-			host->dev_ref_clk_ctrl_mmio =
-					devm_ioremap_resource(dev, res);
-			if (IS_ERR(host->dev_ref_clk_ctrl_mmio))
-				host->dev_ref_clk_ctrl_mmio = NULL;
-			host->dev_ref_clk_en_mask = BIT(5);
-		}
-	}
+	host->dev_ref_clk_ctrl_mmio = hba->mmio_base + REG_UFS_CFG1;
+	host->dev_ref_clk_en_mask = BIT(26);
 
 	list_for_each_entry(clki, &hba->clk_list_head, list) {
 		if (!strcmp(clki->name, "core_clk_unipro"))
@@ -1351,9 +1189,6 @@ static int ufs_qcom_clk_scale_up_pre_change(struct ufs_hba *hba)
 	struct ufs_pa_layer_attr *attr = &host->dev_req_params;
 	int ret;
 
-	if (!ufs_qcom_cap_qunipro(host))
-		return 0;
-
 	ret = ufs_qcom_cfg_timers(hba, attr->gear_rx, attr->pwr_rx,
 				  attr->hs_rate, false, true);
 	if (ret) {
@@ -1371,13 +1206,9 @@ static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba)
 
 static int ufs_qcom_clk_scale_down_pre_change(struct ufs_hba *hba)
 {
-	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 	int err;
 	u32 core_clk_ctrl_reg;
 
-	if (!ufs_qcom_cap_qunipro(host))
-		return 0;
-
 	err = ufshcd_dme_get(hba,
 			    UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL),
 			    &core_clk_ctrl_reg);
@@ -1396,11 +1227,6 @@ static int ufs_qcom_clk_scale_down_pre_change(struct ufs_hba *hba)
 
 static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba)
 {
-	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
-
-	if (!ufs_qcom_cap_qunipro(host))
-		return 0;
-
 	/* set unipro core clock attributes and clear clock divider */
 	return ufs_qcom_set_core_clk_ctrl(hba, false);
 }
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index a109d3359db4..385480499e71 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -114,13 +114,6 @@ enum {
 				 DFC_HW_CGC_EN | TRLUT_HW_CGC_EN |\
 				 TMRLUT_HW_CGC_EN | OCSC_HW_CGC_EN)
 
-/* bit offset */
-#define OFFSET_CLK_NS_REG		0xa
-
-/* bit masks */
-#define MASK_TX_SYMBOL_CLK_1US_REG	GENMASK(9, 0)
-#define MASK_CLK_NS_REG			GENMASK(23, 10)
-
 /* QUniPro Vendor specific attributes */
 #define PA_VS_CONFIG_REG1	0x9000
 #define DME_VS_CORE_CLK_CTRL	0xD002
@@ -189,21 +182,6 @@ struct ufs_qcom_testbus {
 struct gpio_desc;
 
 struct ufs_qcom_host {
-	/*
-	 * Set this capability if host controller supports the QUniPro mode
-	 * and if driver wants the Host controller to operate in QUniPro mode.
-	 * Note: By default this capability will be kept enabled if host
-	 * controller supports the QUniPro mode.
-	 */
-	#define UFS_QCOM_CAP_QUNIPRO	0x1
-
-	/*
-	 * Set this capability if host controller can retain the secure
-	 * configuration even after UFS controller core power collapse.
-	 */
-	#define UFS_QCOM_CAP_RETAIN_SEC_CFG_AFTER_PWR_COLLAPSE	0x2
-	u32 caps;
-
 	struct phy *generic_phy;
 	struct ufs_hba *hba;
 	struct ufs_pa_layer_attr dev_req_params;
@@ -253,9 +231,4 @@ ufs_qcom_get_debug_reg_offset(struct ufs_qcom_host *host, u32 reg)
 
 int ufs_qcom_testbus_config(struct ufs_qcom_host *host);
 
-static inline bool ufs_qcom_cap_qunipro(struct ufs_qcom_host *host)
-{
-	return host->caps & UFS_QCOM_CAP_QUNIPRO;
-}
-
 #endif /* UFS_QCOM_H_ */
-- 
2.25.1

[PATCH v2 16/17] scsi: ufs: qcom: Use ufshcd_rmwl() where applicable

[Manivannan Sadhasivam]

Instead of using both ufshcd_readl() and ufshcd_writel() to read/modify/
write a register, let's make use of the existing helper.

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

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 26aa8904c823..549a08645391 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -387,9 +387,8 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
  */
 static void ufs_qcom_enable_hw_clk_gating(struct ufs_hba *hba)
 {
-	ufshcd_writel(hba,
-		ufshcd_readl(hba, REG_UFS_CFG2) | REG_UFS_CFG2_CGC_EN_ALL,
-		REG_UFS_CFG2);
+	ufshcd_rmwl(hba, REG_UFS_CFG2_CGC_EN_ALL, REG_UFS_CFG2_CGC_EN_ALL,
+		    REG_UFS_CFG2);
 
 	/* Ensure that HW clock gating is enabled before next operations */
 	mb();
@@ -1689,11 +1688,8 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba)
 		platform_msi_domain_free_irqs(hba->dev);
 	} else {
 		if (host->hw_ver.major == 6 && host->hw_ver.minor == 0 &&
-		    host->hw_ver.step == 0) {
-			ufshcd_writel(hba,
-				      ufshcd_readl(hba, REG_UFS_CFG3) | 0x1F000,
-				      REG_UFS_CFG3);
-		}
+		    host->hw_ver.step == 0)
+			ufshcd_rmwl(hba, ESI_VEC_MASK, 0x1f00, REG_UFS_CFG3);
 		ufshcd_mcq_enable_esi(hba);
 	}
 
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index 385480499e71..2ce63a1c7f2f 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -102,6 +102,9 @@ enum {
 #define TMRLUT_HW_CGC_EN	BIT(6)
 #define OCSC_HW_CGC_EN		BIT(7)
 
+/* bit definitions for REG_UFS_CFG3 register */
+#define ESI_VEC_MASK		GENMASK(22, 12)
+
 /* bit definitions for REG_UFS_PARAM0 */
 #define MAX_HS_GEAR_MASK	GENMASK(6, 4)
 #define UFS_QCOM_MAX_GEAR(x)	FIELD_GET(MAX_HS_GEAR_MASK, (x))
-- 
2.25.1

Re: [PATCH v2 16/17] scsi: ufs: qcom: Use ufshcd_rmwl() where applicable

[Andrew Halaney]

On Fri, Dec 08, 2023 at 12:29:01PM +0530, Manivannan Sadhasivam wrote:
> Instead of using both ufshcd_readl() and ufshcd_writel() to read/modify/
> write a register, let's make use of the existing helper.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/ufs/host/ufs-qcom.c | 12 ++++--------
>  drivers/ufs/host/ufs-qcom.h |  3 +++
>  2 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 26aa8904c823..549a08645391 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -387,9 +387,8 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>   */
>  static void ufs_qcom_enable_hw_clk_gating(struct ufs_hba *hba)
>  {
> -	ufshcd_writel(hba,
> -		ufshcd_readl(hba, REG_UFS_CFG2) | REG_UFS_CFG2_CGC_EN_ALL,
> -		REG_UFS_CFG2);
> +	ufshcd_rmwl(hba, REG_UFS_CFG2_CGC_EN_ALL, REG_UFS_CFG2_CGC_EN_ALL,
> +		    REG_UFS_CFG2);
>  
>  	/* Ensure that HW clock gating is enabled before next operations */
>  	mb();
> @@ -1689,11 +1688,8 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba)
>  		platform_msi_domain_free_irqs(hba->dev);
>  	} else {
>  		if (host->hw_ver.major == 6 && host->hw_ver.minor == 0 &&
> -		    host->hw_ver.step == 0) {
> -			ufshcd_writel(hba,
> -				      ufshcd_readl(hba, REG_UFS_CFG3) | 0x1F000,
> -				      REG_UFS_CFG3);
> -		}
> +		    host->hw_ver.step == 0)
> +			ufshcd_rmwl(hba, ESI_VEC_MASK, 0x1f00, REG_UFS_CFG3);

Is this right? I feel like you accidentally just shifted what was written
prior >> 4 bits.

Before: 0x1f000
After:  0x01f00

>  		ufshcd_mcq_enable_esi(hba);
>  	}
>  
> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
> index 385480499e71..2ce63a1c7f2f 100644
> --- a/drivers/ufs/host/ufs-qcom.h
> +++ b/drivers/ufs/host/ufs-qcom.h
> @@ -102,6 +102,9 @@ enum {
>  #define TMRLUT_HW_CGC_EN	BIT(6)
>  #define OCSC_HW_CGC_EN		BIT(7)
>  
> +/* bit definitions for REG_UFS_CFG3 register */
> +#define ESI_VEC_MASK		GENMASK(22, 12)
> +

I'll leave it to someone with the docs to review this field. It at least
contains the bits set above, fwiw. It would be neat to see that
converted to using the field + a FIELD_PREP to set the bits IMO, but I
honestly don't have a lot of experience using those APIs so feel free to
reject that.

>  /* bit definitions for REG_UFS_PARAM0 */
>  #define MAX_HS_GEAR_MASK	GENMASK(6, 4)
>  #define UFS_QCOM_MAX_GEAR(x)	FIELD_GET(MAX_HS_GEAR_MASK, (x))
> -- 
> 2.25.1
> 

Re: [PATCH v2 16/17] scsi: ufs: qcom: Use ufshcd_rmwl() where applicable

[Manivannan Sadhasivam]

On Fri, Dec 08, 2023 at 12:02:03PM -0600, Andrew Halaney wrote:
> On Fri, Dec 08, 2023 at 12:29:01PM +0530, Manivannan Sadhasivam wrote:
> > Instead of using both ufshcd_readl() and ufshcd_writel() to read/modify/
> > write a register, let's make use of the existing helper.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/ufs/host/ufs-qcom.c | 12 ++++--------
> >  drivers/ufs/host/ufs-qcom.h |  3 +++
> >  2 files changed, 7 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> > index 26aa8904c823..549a08645391 100644
> > --- a/drivers/ufs/host/ufs-qcom.c
> > +++ b/drivers/ufs/host/ufs-qcom.c
> > @@ -387,9 +387,8 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
> >   */
> >  static void ufs_qcom_enable_hw_clk_gating(struct ufs_hba *hba)
> >  {
> > -	ufshcd_writel(hba,
> > -		ufshcd_readl(hba, REG_UFS_CFG2) | REG_UFS_CFG2_CGC_EN_ALL,
> > -		REG_UFS_CFG2);
> > +	ufshcd_rmwl(hba, REG_UFS_CFG2_CGC_EN_ALL, REG_UFS_CFG2_CGC_EN_ALL,
> > +		    REG_UFS_CFG2);
> >  
> >  	/* Ensure that HW clock gating is enabled before next operations */
> >  	mb();
> > @@ -1689,11 +1688,8 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba)
> >  		platform_msi_domain_free_irqs(hba->dev);
> >  	} else {
> >  		if (host->hw_ver.major == 6 && host->hw_ver.minor == 0 &&
> > -		    host->hw_ver.step == 0) {
> > -			ufshcd_writel(hba,
> > -				      ufshcd_readl(hba, REG_UFS_CFG3) | 0x1F000,
> > -				      REG_UFS_CFG3);
> > -		}
> > +		    host->hw_ver.step == 0)
> > +			ufshcd_rmwl(hba, ESI_VEC_MASK, 0x1f00, REG_UFS_CFG3);
> 
> Is this right? I feel like you accidentally just shifted what was written
> prior >> 4 bits.
> 
> Before: 0x1f000
> After:  0x01f00
> 

Doh! you are right, I accidentally missed one 0. Since the series is already
merged, I will send a follow up patch.

> >  		ufshcd_mcq_enable_esi(hba);
> >  	}
> >  
> > diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
> > index 385480499e71..2ce63a1c7f2f 100644
> > --- a/drivers/ufs/host/ufs-qcom.h
> > +++ b/drivers/ufs/host/ufs-qcom.h
> > @@ -102,6 +102,9 @@ enum {
> >  #define TMRLUT_HW_CGC_EN	BIT(6)
> >  #define OCSC_HW_CGC_EN		BIT(7)
> >  
> > +/* bit definitions for REG_UFS_CFG3 register */
> > +#define ESI_VEC_MASK		GENMASK(22, 12)
> > +
> 
> I'll leave it to someone with the docs to review this field. It at least
> contains the bits set above, fwiw. It would be neat to see that
> converted to using the field + a FIELD_PREP to set the bits IMO, but I
> honestly don't have a lot of experience using those APIs so feel free to
> reject that.
> 

Fair point. The reason for hardcoding the value in this patch was I didn't get
information on how the value works. But now I got that info, so will address
this in the follow up patch I mentioned above.

- Mani

> >  /* bit definitions for REG_UFS_PARAM0 */
> >  #define MAX_HS_GEAR_MASK	GENMASK(6, 4)
> >  #define UFS_QCOM_MAX_GEAR(x)	FIELD_GET(MAX_HS_GEAR_MASK, (x))
> > -- 
> > 2.25.1
> > 
> 

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

[PATCH v2 17/17] scsi: ufs: qcom: Remove unused definitions

[Manivannan Sadhasivam]

Remove unused definitions.

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

diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index 2ce63a1c7f2f..cdceeb795e70 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -10,22 +10,16 @@
 #include <soc/qcom/ice.h>
 #include <ufs/ufshcd.h>
 
-#define MAX_U32                 (~(u32)0)
 #define MPHY_TX_FSM_STATE       0x41
 #define TX_FSM_HIBERN8          0x1
 #define HBRN8_POLL_TOUT_MS      100
 #define DEFAULT_CLK_RATE_HZ     1000000
-#define BUS_VECTOR_NAME_LEN     32
 #define MAX_SUPP_MAC		64
 
 #define UFS_HW_VER_MAJOR_MASK	GENMASK(31, 28)
 #define UFS_HW_VER_MINOR_MASK	GENMASK(27, 16)
 #define UFS_HW_VER_STEP_MASK	GENMASK(15, 0)
 
-/* vendor specific pre-defined parameters */
-#define SLOW 1
-#define FAST 2
-
 #define UFS_QCOM_LIMIT_HS_RATE		PA_HS_MODE_B
 
 /* QCOM UFS host controller vendor specific registers */
-- 
2.25.1

Re: [PATCH v2 17/17] scsi: ufs: qcom: Remove unused definitions

[Andrew Halaney]

On Fri, Dec 08, 2023 at 12:29:02PM +0530, Manivannan Sadhasivam wrote:
> Remove unused definitions.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

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

> ---
>  drivers/ufs/host/ufs-qcom.h | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
> index 2ce63a1c7f2f..cdceeb795e70 100644
> --- a/drivers/ufs/host/ufs-qcom.h
> +++ b/drivers/ufs/host/ufs-qcom.h
> @@ -10,22 +10,16 @@
>  #include <soc/qcom/ice.h>
>  #include <ufs/ufshcd.h>
>  
> -#define MAX_U32                 (~(u32)0)
>  #define MPHY_TX_FSM_STATE       0x41
>  #define TX_FSM_HIBERN8          0x1
>  #define HBRN8_POLL_TOUT_MS      100
>  #define DEFAULT_CLK_RATE_HZ     1000000
> -#define BUS_VECTOR_NAME_LEN     32
>  #define MAX_SUPP_MAC		64
>  
>  #define UFS_HW_VER_MAJOR_MASK	GENMASK(31, 28)
>  #define UFS_HW_VER_MINOR_MASK	GENMASK(27, 16)
>  #define UFS_HW_VER_STEP_MASK	GENMASK(15, 0)
>  
> -/* vendor specific pre-defined parameters */
> -#define SLOW 1
> -#define FAST 2
> -
>  #define UFS_QCOM_LIMIT_HS_RATE		PA_HS_MODE_B
>  
>  /* QCOM UFS host controller vendor specific registers */
> -- 
> 2.25.1
> 

Re: [PATCH v2 00/17] scsi: ufs: qcom: Code cleanups

[Andrew Halaney]

On Fri, Dec 08, 2023 at 12:28:45PM +0530, Manivannan Sadhasivam wrote:
> Hello,
> 
> This series has code some cleanups to the Qcom UFS driver. No functional
> change. In this version, I've removed code supporting legacy controllers
> ver < 2.0, as the respective platforms were never supported in upstream.
> 
> Tested on: RB5 development board based on Qcom SM8250 SoC.
> 
> - Mani
> 
> Changes in v2:
> 
> * Collected review tags
> * Fixed the comments from Andrew
> * Added a few more patches, most notably one removing the code for old
>   controllers (ver < v2.0)
> 

I took this for a spin on sa8775p-ride when developing another patch
today with no issues. Certainly doesn't hit all the cases here, but:

Tested-by: Andrew Halaney <ahalaney@redhat.com> # sa8775p-ride

> Manivannan Sadhasivam (17):
>   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()
>   scsi: ufs: qcom: Simplify ufs_qcom_{assert/deassert}_reset
>   scsi: ufs: qcom: Remove support for host controllers older than v2.0
>   scsi: ufs: qcom: Use ufshcd_rmwl() where applicable
>   scsi: ufs: qcom: Remove unused definitions
> 
>  drivers/ufs/core/ufshcd.c   |   6 +-
>  drivers/ufs/host/ufs-qcom.c | 377 +++++-------------------------------
>  drivers/ufs/host/ufs-qcom.h |  52 +----
>  include/ufs/ufshcd.h        |   2 +
>  4 files changed, 66 insertions(+), 371 deletions(-)
> 
> -- 
> 2.25.1
> 

Re: [PATCH v2 00/17] scsi: ufs: qcom: Code cleanups

[Konrad Dybcio]

On 8.12.2023 07:58, Manivannan Sadhasivam wrote:
> Hello,
> 
> This series has code some cleanups to the Qcom UFS driver. No functional
> change. In this version, I've removed code supporting legacy controllers
> ver < 2.0, as the respective platforms were never supported in upstream.
> 
> Tested on: RB5 development board based on Qcom SM8250 SoC.
> 
> - Mani
> 
> Changes in v2:
> 
> * Collected review tags
> * Fixed the comments from Andrew
> * Added a few more patches, most notably one removing the code for old
>   controllers (ver < v2.0)
FWIW i found this snipped from a downstream commit from 2014:

8084 : 1.1.1
8994v1 : 1.2.0
8994v2 : 1.3.0

I'm yet to see any 8994 production device utilizing UFS (it wasn't
very good or affordable in 2014/15 IIRC), so I think it's gtg.

Konrad

Re: [PATCH v2 00/17] scsi: ufs: qcom: Code cleanups

[Manivannan Sadhasivam]

On Sat, Dec 09, 2023 at 06:42:31PM +0100, Konrad Dybcio wrote:
> On 8.12.2023 07:58, Manivannan Sadhasivam wrote:
> > Hello,
> > 
> > This series has code some cleanups to the Qcom UFS driver. No functional
> > change. In this version, I've removed code supporting legacy controllers
> > ver < 2.0, as the respective platforms were never supported in upstream.
> > 
> > Tested on: RB5 development board based on Qcom SM8250 SoC.
> > 
> > - Mani
> > 
> > Changes in v2:
> > 
> > * Collected review tags
> > * Fixed the comments from Andrew
> > * Added a few more patches, most notably one removing the code for old
> >   controllers (ver < v2.0)
> FWIW i found this snipped from a downstream commit from 2014:
> 
> 8084 : 1.1.1
> 8994v1 : 1.2.0
> 8994v2 : 1.3.0
> 
> I'm yet to see any 8994 production device utilizing UFS (it wasn't
> very good or affordable in 2014/15 IIRC), so I think it's gtg.
> 

Thanks for digging! I was told that SoCs based on UFS 1.x controllers were not
widely used in production, though I don't know why.

- Mani

> Konrad

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

Re: [PATCH v2 00/17] scsi: ufs: qcom: Code cleanups

[Martin K. Petersen]

Manivannan,

> This series has code some cleanups to the Qcom UFS driver. No functional
> change. In this version, I've removed code supporting legacy controllers
> ver < 2.0, as the respective platforms were never supported in upstream.
>
> Tested on: RB5 development board based on Qcom SM8250 SoC.

Applied to 6.8/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2 00/17] scsi: ufs: qcom: Code cleanups

[Manivannan Sadhasivam]

On Wed, Dec 13, 2023 at 11:10:41PM -0500, Martin K. Petersen wrote:
> 
> Manivannan,
> 
> > This series has code some cleanups to the Qcom UFS driver. No functional
> > change. In this version, I've removed code supporting legacy controllers
> > ver < 2.0, as the respective platforms were never supported in upstream.
> >
> > Tested on: RB5 development board based on Qcom SM8250 SoC.
> 
> Applied to 6.8/scsi-staging, thanks!
> 

Thanks Martin! Andrew spotted an issue on patch 16/17 and I'm going to submit a
patch fixing that. Please either squash it or apply it separately at your own
convenience.

- Mani

> -- 
> Martin K. Petersen	Oracle Linux Engineering

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

Re: [PATCH v2 00/17] scsi: ufs: qcom: Code cleanups

[Martin K. Petersen]

On Fri, 08 Dec 2023 12:28:45 +0530, Manivannan Sadhasivam wrote:

> This series has code some cleanups to the Qcom UFS driver. No functional
> change. In this version, I've removed code supporting legacy controllers
> ver < 2.0, as the respective platforms were never supported in upstream.
> 
> Tested on: RB5 development board based on Qcom SM8250 SoC.
> 
> - Mani
> 
> [...]

Applied to 6.8/scsi-queue, thanks!

[01/17] scsi: ufs: qcom: Use clk_bulk APIs for managing lane clocks
        https://git.kernel.org/mkp/scsi/c/9caef8568831
[02/17] scsi: ufs: qcom: Fix the return value of ufs_qcom_ice_program_key()
        https://git.kernel.org/mkp/scsi/c/3bf7ab4ac30c
[03/17] scsi: ufs: qcom: Fix the return value when platform_get_resource_byname() fails
        https://git.kernel.org/mkp/scsi/c/3a747c5cf9b6
[04/17] scsi: ufs: qcom: Remove superfluous variable assignments
        https://git.kernel.org/mkp/scsi/c/1f165c87ec3e
[05/17] scsi: ufs: qcom: Remove the warning message when core_reset is not available
        https://git.kernel.org/mkp/scsi/c/d42d368647da
[06/17] scsi: ufs: qcom: Export ufshcd_{enable/disable}_irq helpers and make use of them
        https://git.kernel.org/mkp/scsi/c/0ae7a02726bc
[07/17] scsi: ufs: qcom: Fail ufs_qcom_power_up_sequence() when core_reset fails
        https://git.kernel.org/mkp/scsi/c/d11954711499
[08/17] scsi: ufs: qcom: Check the return value of ufs_qcom_power_up_sequence()
        https://git.kernel.org/mkp/scsi/c/e430c0e08957
[09/17] scsi: ufs: qcom: Remove redundant error print for devm_kzalloc() failure
        https://git.kernel.org/mkp/scsi/c/8291652ed8a2
[10/17] scsi: ufs: qcom: Use dev_err_probe() to simplify error handling of devm_gpiod_get_optional()
        https://git.kernel.org/mkp/scsi/c/c7afadacc180
[11/17] scsi: ufs: qcom: Remove unused ufs_qcom_hosts struct array
        https://git.kernel.org/mkp/scsi/c/e7458beab809
[12/17] scsi: ufs: qcom: Sort includes alphabetically
        https://git.kernel.org/mkp/scsi/c/be2e06c81a31
[13/17] scsi: ufs: qcom: Initialize cycles_in_1us variable in ufs_qcom_set_core_clk_ctrl()
        https://git.kernel.org/mkp/scsi/c/3b60f4564ff5
[14/17] scsi: ufs: qcom: Simplify ufs_qcom_{assert/deassert}_reset
        https://git.kernel.org/mkp/scsi/c/6b481af25ec0
[15/17] scsi: ufs: qcom: Remove support for host controllers older than v2.0
        https://git.kernel.org/mkp/scsi/c/104cd58d9af8
[16/17] scsi: ufs: qcom: Use ufshcd_rmwl() where applicable
        https://git.kernel.org/mkp/scsi/c/0e9f4375db1c
[17/17] scsi: ufs: qcom: Remove unused definitions
        https://git.kernel.org/mkp/scsi/c/cac50d04fffe

-- 
Martin K. Petersen	Oracle Linux Engineering