Re: [PATCH RFC v4 1/1] scsi: ufs: Fix ufs power down/on specs violation

ziqichen@xxxxxxxxxxxxxx · Thu, 24 Dec 2020 23:35:12 +0800

On 2020-12-24 04:45, Avri Altman wrote:
Hi,

As per specs, e.g, JESD220E chapter 7.2, while powering
off/on the ufs device, RST_N signal and REF_CLK signal
should be between VSS(Ground) and VCCQ/VCCQ2.

To flexibly control device reset line, refactor the function
ufschd_vops_device_reset(sturct ufs_hba *hba) to ufshcd_
vops_device_reset(sturct ufs_hba *hba, bool asserted). The
new parameter "bool asserted" is used to separate device reset
line pulling down from pulling up.
Sorry for my late response.
Please allow few more days to consult internally about this.


Cc: Kiwoong Kim <kwmad.kim@xxxxxxxxxxx>
Cc: Stanley Chu <stanley.chu@xxxxxxxxxxxx>
Signed-off-by: Ziqi Chen <ziqichen@xxxxxxxxxxxxxx>


-static int ufs_qcom_device_reset(struct ufs_hba *hba)
+static int ufs_qcom_device_reset(struct ufs_hba *hba, bool asserted)
 {
        struct ufs_qcom_host *host = ufshcd_get_variant(hba);

@@ -1417,15 +1418,20 @@ static int ufs_qcom_device_reset(struct 
ufs_hba
*hba)
        if (!host->device_reset)
                return -EOPNOTSUPP;

-       /*
-        * The UFS device shall detect reset pulses of 1us, sleep for 
10us to
-        * be on the safe side.
-        */
-       gpiod_set_value_cansleep(host->device_reset, 1);
-       usleep_range(10, 15);
+       if (asserted) {
+               gpiod_set_value_cansleep(host->device_reset, 1);

-       gpiod_set_value_cansleep(host->device_reset, 0);
-       usleep_range(10, 15);
+               /*
+                * The UFS device shall detect reset pulses of 1us, 
sleep for 10us to
+                * be on the safe side.
+                */
+               usleep_range(10, 15);
+       } else {
+               gpiod_set_value_cansleep(host->device_reset, 0);
+
+                /* Some devices may need time to respond to rst_n */
+               usleep_range(10, 15);
Since sleep the same on assert/de-assert can move it outside the
if-else clause?

Hi Avri,

Even though there is same delay on assert/de-assert, they have different 
purposes. The delay
on assert is for JEDEC requirement while the delay on de-assert is for 
some devices requirement.
The latter is just a empirical value and is still controversial among 
SoC vendors. This value
may be changed in the further. So I don't think we should move it 
outside the if-else clause.


+       }

        return 0;
 }

All the below changes, in suspend/resume, deserves some references in
your commit log,
And probably a separate patch.

Below changes adjusted the sequence of CLK, Rst_n, VCC and VCCQ/VCCQ2, 
These requirements
are referred in JEDEC and already quoted in my commit log. We don't need 
more descriptions.

Below changes have also modified common callback interface, we'd better 
keep implementation
and caller changes in one patch.


Best Regards,
Ziqi Chen


Thanks,
Avri

@@ -8686,8 +8696,6 @@ static int ufshcd_suspend(struct ufs_hba *hba,
enum ufs_pm_op pm_op)
        if (ret)
                goto set_dev_active;

-       ufshcd_vreg_set_lpm(hba);
-
 disable_clks:
        /*
         * Call vendor specific suspend callback. As these callbacks 
may access
@@ -8703,6 +8711,9 @@ static int ufshcd_suspend(struct ufs_hba *hba,
enum ufs_pm_op pm_op)
         */
        ufshcd_disable_irq(hba);

+       if (ufshcd_is_link_off(hba))
+               ufshcd_vops_device_reset(hba, true);
+
        ufshcd_setup_clocks(hba, false);

        if (ufshcd_is_clkgating_allowed(hba)) {
@@ -8711,6 +8722,8 @@ static int ufshcd_suspend(struct ufs_hba *hba,
enum ufs_pm_op pm_op)
                                        hba->clk_gating.state);
        }

+       ufshcd_vreg_set_lpm(hba);
+
        /* Put the host controller in low power mode if possible */
        ufshcd_hba_vreg_set_lpm(hba);
        goto out;
@@ -8778,18 +8791,19 @@ static int ufshcd_resume(struct ufs_hba *hba,
enum ufs_pm_op pm_op)
        old_link_state = hba->uic_link_state;

        ufshcd_hba_vreg_set_hpm(hba);
+
+       ret = ufshcd_vreg_set_hpm(hba);
+       if (ret)
+               goto out;
+
        /* Make sure clocks are enabled before accessing controller */
        ret = ufshcd_setup_clocks(hba, true);
        if (ret)
-               goto out;
+               goto disable_vreg;

        /* enable the host irq as host controller would be active soon 
*/
        ufshcd_enable_irq(hba);

-       ret = ufshcd_vreg_set_hpm(hba);
-       if (ret)
-               goto disable_irq_and_vops_clks;
-
        /*
         * Call vendor specific resume callback. As these callbacks 
may access
         * vendor specific host controller register space call them 
when the
@@ -8797,7 +8811,7 @@ static int ufshcd_resume(struct ufs_hba *hba,
enum ufs_pm_op pm_op)
         */
        ret = ufshcd_vops_resume(hba, pm_op);
        if (ret)
-               goto disable_vreg;
+               goto disable_irq_and_vops_clks;

        /* For DeepSleep, the only supported option is to have the 
link off */
        WARN_ON(ufshcd_is_ufs_dev_deepsleep(hba) &&
!ufshcd_is_link_off(hba));
@@ -8864,8 +8878,6 @@ static int ufshcd_resume(struct ufs_hba *hba,
enum ufs_pm_op pm_op)
        ufshcd_link_state_transition(hba, old_link_state, 0);
 vendor_suspend:
        ufshcd_vops_suspend(hba, pm_op);
-disable_vreg:
-       ufshcd_vreg_set_lpm(hba);
 disable_irq_and_vops_clks:
        ufshcd_disable_irq(hba);
        if (hba->clk_scaling.is_allowed)
@@ -8876,6 +8888,8 @@ static int ufshcd_resume(struct ufs_hba *hba,
enum ufs_pm_op pm_op)
                trace_ufshcd_clk_gating(dev_name(hba->dev),
                                        hba->clk_gating.state);
        }
+disable_vreg:
+       ufshcd_vreg_set_lpm(hba);
 out:
        hba->pm_op_in_progress = 0;
        if (ret)

diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 9bb5f0e..d5fbaba 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -319,7 +319,7 @@ struct ufs_pwr_mode_info {
  * @resume: called during host controller PM callback
  * @dbg_register_dump: used to dump controller debug information
  * @phy_initialization: used to initialize phys
- * @device_reset: called to issue a reset pulse on the UFS device
+ * @device_reset: called to assert or deassert device reset line
  * @program_key: program or evict an inline encryption key
  * @event_notify: called to notify important events
  */
@@ -350,7 +350,7 @@ struct ufs_hba_variant_ops {
        int     (*resume)(struct ufs_hba *, enum ufs_pm_op);
        void    (*dbg_register_dump)(struct ufs_hba *hba);
        int     (*phy_initialization)(struct ufs_hba *);
-       int     (*device_reset)(struct ufs_hba *hba);
+       int     (*device_reset)(struct ufs_hba *hba, bool asserted);
        void    (*config_scaling_param)(struct ufs_hba *hba,
                                        struct devfreq_dev_profile 
*profile,
                                        void *data);
@@ -1216,10 +1216,10 @@ static inline void
ufshcd_vops_dbg_register_dump(struct ufs_hba *hba)
                hba->vops->dbg_register_dump(hba);
 }

-static inline int ufshcd_vops_device_reset(struct ufs_hba *hba)
+static inline int ufshcd_vops_device_reset(struct ufs_hba *hba, bool
asserted)
 {
        if (hba->vops && hba->vops->device_reset)
-               return hba->vops->device_reset(hba);
+               return hba->vops->device_reset(hba, asserted);

        return -EOPNOTSUPP;
 }
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
a Linux Foundation Collaborative Project






