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

Commit Message

Ziqi Chen Dec. 22, 2020, 1:49 p.m. UTC

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.

Cc: Kiwoong Kim <[e-mail]>
Cc: Stanley Chu <[e-mail]>
Signed-off-by: Ziqi Chen <[e-mail]>
---
 drivers/scsi/ufs/ufs-mediatek.c | 32 ++++++++++++++++----------------
 drivers/scsi/ufs/ufs-qcom.c     | 24 +++++++++++++++---------
 drivers/scsi/ufs/ufshcd.c       | 36 +++++++++++++++++++++++++-----------
 drivers/scsi/ufs/ufshcd.h       |  8 ++++----
 4 files changed, 60 insertions(+), 40 deletions(-)

Comments

Stanley Chu Dec. 23, 2020, 9:33 a.m. UTC | #1

On Tue, 2020-12-22 at 21:49 +0800, Ziqi Chen wrote:
> 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.
> 
> Cc: Kiwoong Kim <[e-mail]>
> Cc: Stanley Chu <[e-mail]>
> Signed-off-by: Ziqi Chen <[e-mail]>

Thanks for this fix including ufs-mediatek change as well.

Reviewed-by: Stanley Chu <[e-mail]>
Tested-by: Stanley Chu <[e-mail]>

Can Guo Dec. 23, 2020, 12:17 p.m. UTC | #2

On 2020-12-22 21:49, Ziqi Chen wrote:
> 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.
> 
> Cc: Kiwoong Kim <[e-mail]>
> Cc: Stanley Chu <[e-mail]>
> Signed-off-by: Ziqi Chen <[e-mail]>
> ---
>  drivers/scsi/ufs/ufs-mediatek.c | 32 ++++++++++++++++----------------
>  drivers/scsi/ufs/ufs-qcom.c     | 24 +++++++++++++++---------
>  drivers/scsi/ufs/ufshcd.c       | 36 
> +++++++++++++++++++++++++-----------
>  drivers/scsi/ufs/ufshcd.h       |  8 ++++----
>  4 files changed, 60 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs-mediatek.c 
> b/drivers/scsi/ufs/ufs-mediatek.c
> index 80618af..072f4db 100644
> --- a/drivers/scsi/ufs/ufs-mediatek.c
> +++ b/drivers/scsi/ufs/ufs-mediatek.c
> @@ -841,27 +841,27 @@ static int ufs_mtk_link_startup_notify(struct
> ufs_hba *hba,
>  	return ret;
>  }
> 
> -static int ufs_mtk_device_reset(struct ufs_hba *hba)
> +static int ufs_mtk_device_reset(struct ufs_hba *hba, bool asserted)
>  {
>  	struct arm_smccc_res res;
> 
> -	ufs_mtk_device_reset_ctrl(0, res);
> +	if (asserted) {
> +		ufs_mtk_device_reset_ctrl(0, res);
> 
> -	/*
> -	 * The reset signal is active low. UFS devices shall detect
> -	 * more than or equal to 1us of positive or negative RST_n
> -	 * pulse width.
> -	 *
> -	 * To be on safe side, keep the reset low for at least 10us.
> -	 */
> -	usleep_range(10, 15);
> -
> -	ufs_mtk_device_reset_ctrl(1, res);
> -
> -	/* Some devices may need time to respond to rst_n */
> -	usleep_range(10000, 15000);
> +		/*
> +		 * The reset signal is active low. UFS devices shall detect
> +		 * more than or equal to 1us of positive or negative RST_n
> +		 * pulse width.
> +		 *
> +		 * To be on safe side, keep the reset low for at least 10us.
> +		 */
> +		usleep_range(10, 15);
> +	} else {
> +		ufs_mtk_device_reset_ctrl(1, res);
> 
> -	dev_info(hba->dev, "device reset done\n");
> +		/* Some devices may need time to respond to rst_n */
> +		usleep_range(10000, 15000);
> +	}
> 
>  	return 0;
>  }
> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> index 2206b1e..fed10e5 100644
> --- a/drivers/scsi/ufs/ufs-qcom.c
> +++ b/drivers/scsi/ufs/ufs-qcom.c
> @@ -1406,10 +1406,11 @@ static void ufs_qcom_dump_dbg_regs(struct 
> ufs_hba *hba)
>  /**
>   * ufs_qcom_device_reset() - toggle the (optional) device reset line
>   * @hba: per-adapter instance
> + * @asserted: assert or deassert device reset line
>   *
>   * Toggles the (optional) reset line to reset the attached device.
>   */
> -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);
> +	}
> 
>  	return 0;
>  }
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index e221add..f2daac2 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -585,7 +585,13 @@ static void ufshcd_device_reset(struct ufs_hba 
> *hba)
>  {
>  	int err;
> 
> -	err = ufshcd_vops_device_reset(hba);
> +	err = ufshcd_vops_device_reset(hba, true);
> +	if (err) {
> +		dev_err(hba->dev, "asserting device reset failed: %d\n", err);
> +		return;
> +	}
> +
> +	err = ufshcd_vops_device_reset(hba, false);
> 
>  	if (!err) {
>  		ufshcd_set_ufs_dev_active(hba);
> @@ -593,7 +599,11 @@ static void ufshcd_device_reset(struct ufs_hba 
> *hba)
>  			hba->wb_enabled = false;
>  			hba->wb_buf_flush_enabled = false;
>  		}
> +		dev_dbg(hba->dev, "device reset done\n");
> +	} else {
> +		dev_err(hba->dev, "deasserting device reset failed: %d\n", err);
>  	}
> +
>  	if (err != -EOPNOTSUPP)
>  		ufshcd_update_evt_hist(hba, UFS_EVT_DEV_RESET, err);
>  }
> @@ -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;
>  }

Reviewed-by: Can Guo <[e-mail]>

Avri Altman Dec. 23, 2020, 8:45 p.m. UTC | #3

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 <[e-mail]>
> Cc: Stanley Chu <[e-mail]>
> Signed-off-by: Ziqi Chen <[e-mail]>


> -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? 

> +       }
> 
>         return 0;
>  }

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

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

Ziqi Chen Dec. 24, 2020, 3:35 p.m. UTC | #4

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 <[e-mail]>
>> Cc: Stanley Chu <[e-mail]>
>> Signed-off-by: Ziqi Chen <[e-mail]>
> 
> 
>> -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

Bjorn Andersson Dec. 28, 2020, 5:55 p.m. UTC | #5

On Tue 22 Dec 07:49 CST 2020, Ziqi Chen wrote:

> 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.
> 
> Cc: Kiwoong Kim <[e-mail]>
> Cc: Stanley Chu <[e-mail]>
> Signed-off-by: Ziqi Chen <[e-mail]>
> ---
>  drivers/scsi/ufs/ufs-mediatek.c | 32 ++++++++++++++++----------------
>  drivers/scsi/ufs/ufs-qcom.c     | 24 +++++++++++++++---------
>  drivers/scsi/ufs/ufshcd.c       | 36 +++++++++++++++++++++++++-----------
>  drivers/scsi/ufs/ufshcd.h       |  8 ++++----
>  4 files changed, 60 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs-mediatek.c b/drivers/scsi/ufs/ufs-mediatek.c
> index 80618af..072f4db 100644
> --- a/drivers/scsi/ufs/ufs-mediatek.c
> +++ b/drivers/scsi/ufs/ufs-mediatek.c
> @@ -841,27 +841,27 @@ static int ufs_mtk_link_startup_notify(struct ufs_hba *hba,
>  	return ret;
>  }
>  
> -static int ufs_mtk_device_reset(struct ufs_hba *hba)
> +static int ufs_mtk_device_reset(struct ufs_hba *hba, bool asserted)
>  {
>  	struct arm_smccc_res res;
>  
> -	ufs_mtk_device_reset_ctrl(0, res);
> +	if (asserted) {
> +		ufs_mtk_device_reset_ctrl(0, res);
>  
> -	/*
> -	 * The reset signal is active low. UFS devices shall detect
> -	 * more than or equal to 1us of positive or negative RST_n
> -	 * pulse width.
> -	 *
> -	 * To be on safe side, keep the reset low for at least 10us.
> -	 */
> -	usleep_range(10, 15);
> -
> -	ufs_mtk_device_reset_ctrl(1, res);
> -
> -	/* Some devices may need time to respond to rst_n */
> -	usleep_range(10000, 15000);
> +		/*
> +		 * The reset signal is active low. UFS devices shall detect
> +		 * more than or equal to 1us of positive or negative RST_n
> +		 * pulse width.
> +		 *
> +		 * To be on safe side, keep the reset low for at least 10us.
> +		 */
> +		usleep_range(10, 15);

I see no point in allowing vendors to "tweak" the 1us->10us adjustment.
The specification says 1us and we all agree that 10us gives us good
enough slack. I.e. this is common code.

> +	} else {
> +		ufs_mtk_device_reset_ctrl(1, res);
>  
> -	dev_info(hba->dev, "device reset done\n");
> +		/* Some devices may need time to respond to rst_n */
> +		usleep_range(10000, 15000);

The comment in both the Qualcomm and Mediatek drivers claim that this is
sleep relates to the UFS device (not host), so why should it be
different?

What happens if I take the device that Mediatek see a need for a 10ms
delay and hook that up to a Qualcomm host? This really should go in the
common code.



As such I really would prefer to see these delays in the common code!
You really shouldn't write code based on a speculation that one day
there might come someone who need other values - when that day come we
can just change the code, and if it never comes we're better off with
the cleaner implementation.

Regards,
Bjorn

> +	}
>  
>  	return 0;
>  }
> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> index 2206b1e..fed10e5 100644
> --- a/drivers/scsi/ufs/ufs-qcom.c
> +++ b/drivers/scsi/ufs/ufs-qcom.c
> @@ -1406,10 +1406,11 @@ static void ufs_qcom_dump_dbg_regs(struct ufs_hba *hba)
>  /**
>   * ufs_qcom_device_reset() - toggle the (optional) device reset line
>   * @hba: per-adapter instance
> + * @asserted: assert or deassert device reset line
>   *
>   * Toggles the (optional) reset line to reset the attached device.
>   */
> -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);
> +	}
>  
>  	return 0;
>  }
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index e221add..f2daac2 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -585,7 +585,13 @@ static void ufshcd_device_reset(struct ufs_hba *hba)
>  {
>  	int err;
>  
> -	err = ufshcd_vops_device_reset(hba);
> +	err = ufshcd_vops_device_reset(hba, true);
> +	if (err) {
> +		dev_err(hba->dev, "asserting device reset failed: %d\n", err);
> +		return;
> +	}
> +
> +	err = ufshcd_vops_device_reset(hba, false);
>  
>  	if (!err) {
>  		ufshcd_set_ufs_dev_active(hba);
> @@ -593,7 +599,11 @@ static void ufshcd_device_reset(struct ufs_hba *hba)
>  			hba->wb_enabled = false;
>  			hba->wb_buf_flush_enabled = false;
>  		}
> +		dev_dbg(hba->dev, "device reset done\n");
> +	} else {
> +		dev_err(hba->dev, "deasserting device reset failed: %d\n", err);
>  	}
> +
>  	if (err != -EOPNOTSUPP)
>  		ufshcd_update_evt_hist(hba, UFS_EVT_DEV_RESET, err);
>  }
> @@ -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
>

Can Guo Dec. 29, 2020, 1:18 a.m. UTC | #6

On 2020-12-29 01:55, Bjorn Andersson wrote:
> On Tue 22 Dec 07:49 CST 2020, Ziqi Chen wrote:
> 
>> 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.
>> 
>> Cc: Kiwoong Kim <[e-mail]>
>> Cc: Stanley Chu <[e-mail]>
>> Signed-off-by: Ziqi Chen <[e-mail]>
>> ---
>>  drivers/scsi/ufs/ufs-mediatek.c | 32 ++++++++++++++++----------------
>>  drivers/scsi/ufs/ufs-qcom.c     | 24 +++++++++++++++---------
>>  drivers/scsi/ufs/ufshcd.c       | 36 
>> +++++++++++++++++++++++++-----------
>>  drivers/scsi/ufs/ufshcd.h       |  8 ++++----
>>  4 files changed, 60 insertions(+), 40 deletions(-)
>> 
>> diff --git a/drivers/scsi/ufs/ufs-mediatek.c 
>> b/drivers/scsi/ufs/ufs-mediatek.c
>> index 80618af..072f4db 100644
>> --- a/drivers/scsi/ufs/ufs-mediatek.c
>> +++ b/drivers/scsi/ufs/ufs-mediatek.c
>> @@ -841,27 +841,27 @@ static int ufs_mtk_link_startup_notify(struct 
>> ufs_hba *hba,
>>  	return ret;
>>  }
>> 
>> -static int ufs_mtk_device_reset(struct ufs_hba *hba)
>> +static int ufs_mtk_device_reset(struct ufs_hba *hba, bool asserted)
>>  {
>>  	struct arm_smccc_res res;
>> 
>> -	ufs_mtk_device_reset_ctrl(0, res);
>> +	if (asserted) {
>> +		ufs_mtk_device_reset_ctrl(0, res);
>> 
>> -	/*
>> -	 * The reset signal is active low. UFS devices shall detect
>> -	 * more than or equal to 1us of positive or negative RST_n
>> -	 * pulse width.
>> -	 *
>> -	 * To be on safe side, keep the reset low for at least 10us.
>> -	 */
>> -	usleep_range(10, 15);
>> -
>> -	ufs_mtk_device_reset_ctrl(1, res);
>> -
>> -	/* Some devices may need time to respond to rst_n */
>> -	usleep_range(10000, 15000);
>> +		/*
>> +		 * The reset signal is active low. UFS devices shall detect
>> +		 * more than or equal to 1us of positive or negative RST_n
>> +		 * pulse width.
>> +		 *
>> +		 * To be on safe side, keep the reset low for at least 10us.
>> +		 */
>> +		usleep_range(10, 15);
> 
> I see no point in allowing vendors to "tweak" the 1us->10us adjustment.
> The specification says 1us and we all agree that 10us gives us good
> enough slack. I.e. this is common code.

Hi Bjron,

We tried, but Samsung fellows wanted 5us. We couldn't get a agreement
on this delay in short term, so we chose to leave it in vops.

> 
>> +	} else {
>> +		ufs_mtk_device_reset_ctrl(1, res);
>> 
>> -	dev_info(hba->dev, "device reset done\n");
>> +		/* Some devices may need time to respond to rst_n */
>> +		usleep_range(10000, 15000);
> 
> The comment in both the Qualcomm and Mediatek drivers claim that this 
> is
> sleep relates to the UFS device (not host), so why should it be
> different?
> 
> What happens if I take the device that Mediatek see a need for a 10ms
> delay and hook that up to a Qualcomm host? This really should go in the
> common code.
> 

Agree, but Qualcomm host didn't have any problems with 10us yet, so if 
we put
the 10ms delay to common code, Qualcomm host would suffer longer delay 
when
device reset happens - both bootup and resume(xpm_lvl = 5/6) latency 
would
be increased.

Regards,
Can Guo.

> 
> 
> As such I really would prefer to see these delays in the common code!
> You really shouldn't write code based on a speculation that one day
> there might come someone who need other values - when that day come we
> can just change the code, and if it never comes we're better off with
> the cleaner implementation.
> 
> Regards,
> Bjorn
> 
>> +	}
>> 
>>  	return 0;
>>  }
>> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
>> index 2206b1e..fed10e5 100644
>> --- a/drivers/scsi/ufs/ufs-qcom.c
>> +++ b/drivers/scsi/ufs/ufs-qcom.c
>> @@ -1406,10 +1406,11 @@ static void ufs_qcom_dump_dbg_regs(struct 
>> ufs_hba *hba)
>>  /**
>>   * ufs_qcom_device_reset() - toggle the (optional) device reset line
>>   * @hba: per-adapter instance
>> + * @asserted: assert or deassert device reset line
>>   *
>>   * Toggles the (optional) reset line to reset the attached device.
>>   */
>> -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);
>> +	}
>> 
>>  	return 0;
>>  }
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index e221add..f2daac2 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -585,7 +585,13 @@ static void ufshcd_device_reset(struct ufs_hba 
>> *hba)
>>  {
>>  	int err;
>> 
>> -	err = ufshcd_vops_device_reset(hba);
>> +	err = ufshcd_vops_device_reset(hba, true);
>> +	if (err) {
>> +		dev_err(hba->dev, "asserting device reset failed: %d\n", err);
>> +		return;
>> +	}
>> +
>> +	err = ufshcd_vops_device_reset(hba, false);
>> 
>>  	if (!err) {
>>  		ufshcd_set_ufs_dev_active(hba);
>> @@ -593,7 +599,11 @@ static void ufshcd_device_reset(struct ufs_hba 
>> *hba)
>>  			hba->wb_enabled = false;
>>  			hba->wb_buf_flush_enabled = false;
>>  		}
>> +		dev_dbg(hba->dev, "device reset done\n");
>> +	} else {
>> +		dev_err(hba->dev, "deasserting device reset failed: %d\n", err);
>>  	}
>> +
>>  	if (err != -EOPNOTSUPP)
>>  		ufshcd_update_evt_hist(hba, UFS_EVT_DEV_RESET, err);
>>  }
>> @@ -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
>>

Can Guo Dec. 29, 2020, 1:48 a.m. UTC | #7

On 2020-12-29 09:18, Can Guo wrote:
> On 2020-12-29 01:55, Bjorn Andersson wrote:
>> On Tue 22 Dec 07:49 CST 2020, Ziqi Chen wrote:
>> 
>>> 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.
>>> 
>>> Cc: Kiwoong Kim <[e-mail]>
>>> Cc: Stanley Chu <[e-mail]>
>>> Signed-off-by: Ziqi Chen <[e-mail]>
>>> ---
>>>  drivers/scsi/ufs/ufs-mediatek.c | 32 
>>> ++++++++++++++++----------------
>>>  drivers/scsi/ufs/ufs-qcom.c     | 24 +++++++++++++++---------
>>>  drivers/scsi/ufs/ufshcd.c       | 36 
>>> +++++++++++++++++++++++++-----------
>>>  drivers/scsi/ufs/ufshcd.h       |  8 ++++----
>>>  4 files changed, 60 insertions(+), 40 deletions(-)
>>> 
>>> diff --git a/drivers/scsi/ufs/ufs-mediatek.c 
>>> b/drivers/scsi/ufs/ufs-mediatek.c
>>> index 80618af..072f4db 100644
>>> --- a/drivers/scsi/ufs/ufs-mediatek.c
>>> +++ b/drivers/scsi/ufs/ufs-mediatek.c
>>> @@ -841,27 +841,27 @@ static int ufs_mtk_link_startup_notify(struct 
>>> ufs_hba *hba,
>>>  	return ret;
>>>  }
>>> 
>>> -static int ufs_mtk_device_reset(struct ufs_hba *hba)
>>> +static int ufs_mtk_device_reset(struct ufs_hba *hba, bool asserted)
>>>  {
>>>  	struct arm_smccc_res res;
>>> 
>>> -	ufs_mtk_device_reset_ctrl(0, res);
>>> +	if (asserted) {
>>> +		ufs_mtk_device_reset_ctrl(0, res);
>>> 
>>> -	/*
>>> -	 * The reset signal is active low. UFS devices shall detect
>>> -	 * more than or equal to 1us of positive or negative RST_n
>>> -	 * pulse width.
>>> -	 *
>>> -	 * To be on safe side, keep the reset low for at least 10us.
>>> -	 */
>>> -	usleep_range(10, 15);
>>> -
>>> -	ufs_mtk_device_reset_ctrl(1, res);
>>> -
>>> -	/* Some devices may need time to respond to rst_n */
>>> -	usleep_range(10000, 15000);
>>> +		/*
>>> +		 * The reset signal is active low. UFS devices shall detect
>>> +		 * more than or equal to 1us of positive or negative RST_n
>>> +		 * pulse width.
>>> +		 *
>>> +		 * To be on safe side, keep the reset low for at least 10us.
>>> +		 */
>>> +		usleep_range(10, 15);
>> 
>> I see no point in allowing vendors to "tweak" the 1us->10us 
>> adjustment.
>> The specification says 1us and we all agree that 10us gives us good
>> enough slack. I.e. this is common code.
> 
> Hi Bjron,
> 
> We tried, but Samsung fellows wanted 5us. We couldn't get a agreement
> on this delay in short term, so we chose to leave it in vops.
> 
>> 
>>> +	} else {
>>> +		ufs_mtk_device_reset_ctrl(1, res);
>>> 
>>> -	dev_info(hba->dev, "device reset done\n");
>>> +		/* Some devices may need time to respond to rst_n */
>>> +		usleep_range(10000, 15000);
>> 
>> The comment in both the Qualcomm and Mediatek drivers claim that this 
>> is
>> sleep relates to the UFS device (not host), so why should it be
>> different?
>> 
>> What happens if I take the device that Mediatek see a need for a 10ms
>> delay and hook that up to a Qualcomm host? This really should go in 
>> the
>> common code.
>> 
> 
> Agree, but Qualcomm host didn't have any problems with 10us yet, so if 
> we put
> the 10ms delay to common code, Qualcomm host would suffer longer delay 
> when
> device reset happens - both bootup and resume(xpm_lvl = 5/6) latency 
> would
> be increased.
> 
> Regards,
> Can Guo.
> 

Besides, currently this device reset vops is only registered by 
ufs-qcom.c
and ufs-mediatek.c, meaning any delays that we put in the common code 
are not
necessary for those who do not have this vops registered, i.e 
ufs-exynos.c,
ufs-hisi.c.

Regards,
Can Guo.

>> 
>> 
>> As such I really would prefer to see these delays in the common code!
>> You really shouldn't write code based on a speculation that one day
>> there might come someone who need other values - when that day come we
>> can just change the code, and if it never comes we're better off with
>> the cleaner implementation.
>> 
>> Regards,
>> Bjorn
>> 
>>> +	}
>>> 
>>>  	return 0;
>>>  }
>>> diff --git a/drivers/scsi/ufs/ufs-qcom.c 
>>> b/drivers/scsi/ufs/ufs-qcom.c
>>> index 2206b1e..fed10e5 100644
>>> --- a/drivers/scsi/ufs/ufs-qcom.c
>>> +++ b/drivers/scsi/ufs/ufs-qcom.c
>>> @@ -1406,10 +1406,11 @@ static void ufs_qcom_dump_dbg_regs(struct 
>>> ufs_hba *hba)
>>>  /**
>>>   * ufs_qcom_device_reset() - toggle the (optional) device reset line
>>>   * @hba: per-adapter instance
>>> + * @asserted: assert or deassert device reset line
>>>   *
>>>   * Toggles the (optional) reset line to reset the attached device.
>>>   */
>>> -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);
>>> +	}
>>> 
>>>  	return 0;
>>>  }
>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>> index e221add..f2daac2 100644
>>> --- a/drivers/scsi/ufs/ufshcd.c
>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>> @@ -585,7 +585,13 @@ static void ufshcd_device_reset(struct ufs_hba 
>>> *hba)
>>>  {
>>>  	int err;
>>> 
>>> -	err = ufshcd_vops_device_reset(hba);
>>> +	err = ufshcd_vops_device_reset(hba, true);
>>> +	if (err) {
>>> +		dev_err(hba->dev, "asserting device reset failed: %d\n", err);
>>> +		return;
>>> +	}
>>> +
>>> +	err = ufshcd_vops_device_reset(hba, false);
>>> 
>>>  	if (!err) {
>>>  		ufshcd_set_ufs_dev_active(hba);
>>> @@ -593,7 +599,11 @@ static void ufshcd_device_reset(struct ufs_hba 
>>> *hba)
>>>  			hba->wb_enabled = false;
>>>  			hba->wb_buf_flush_enabled = false;
>>>  		}
>>> +		dev_dbg(hba->dev, "device reset done\n");
>>> +	} else {
>>> +		dev_err(hba->dev, "deasserting device reset failed: %d\n", err);
>>>  	}
>>> +
>>>  	if (err != -EOPNOTSUPP)
>>>  		ufshcd_update_evt_hist(hba, UFS_EVT_DEV_RESET, err);
>>>  }
>>> @@ -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
>>>

Adrian Hunter Jan. 4, 2021, 9:15 a.m. UTC | #8

On 22/12/20 3:49 pm, Ziqi Chen wrote:
> 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.

This patch assumes the power is controlled by voltage regulators, but for us
it is controlled by firmware (ACPI), so it is not correct to change RST_n
for all host controllers as you are doing.

Also we might need to use a firmware interface for device reset, in which
case the 'asserted' value doe not make sense.

Can we leave the device reset callback alone, and instead introduce a new
variant operation for setting RST_n to match voltage regulator power changes?

Bjorn Andersson Jan. 4, 2021, 6:55 p.m. UTC | #9

On Mon 04 Jan 03:15 CST 2021, Adrian Hunter wrote:

> On 22/12/20 3:49 pm, Ziqi Chen wrote:
> > 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.
> 
> This patch assumes the power is controlled by voltage regulators, but for us
> it is controlled by firmware (ACPI), so it is not correct to change RST_n
> for all host controllers as you are doing.
> 
> Also we might need to use a firmware interface for device reset, in which
> case the 'asserted' value doe not make sense.
> 

Are you saying that the entire flip-flop-the-reset is a single firmware
operation in your case? If you look at the Mediatek driver, the
implementation of ufs_mtk_device_reset_ctrl() is a jump to firmware.


But perhaps "asserted" isn't the appropriate English word for saying
"the reset is in the resetting state"?

I just wanted to avoid the use of "high"/"lo" as if you look at the
Mediatek code they pass the expected line-level to the firmware, while
in the Qualcomm code we pass the logical state to the GPIO code which is
setup up as "active low" and thereby flip the meaning before hitting the
pad.

> Can we leave the device reset callback alone, and instead introduce a new
> variant operation for setting RST_n to match voltage regulator power changes?

Wouldn't this new function just have to look like the proposed patches?
In which case for existing platforms we'd have both?

How would you implement this, or would you simply skip implementing
this?

Regards,
Bjorn

Bjorn Andersson Jan. 4, 2021, 6:57 p.m. UTC | #10

On Mon 28 Dec 19:18 CST 2020, Can Guo wrote:

> On 2020-12-29 01:55, Bjorn Andersson wrote:
> > On Tue 22 Dec 07:49 CST 2020, Ziqi Chen wrote:
> > 
> > > 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.
> > > 
> > > Cc: Kiwoong Kim <[e-mail]>
> > > Cc: Stanley Chu <[e-mail]>
> > > Signed-off-by: Ziqi Chen <[e-mail]>
> > > ---
> > >  drivers/scsi/ufs/ufs-mediatek.c | 32 ++++++++++++++++----------------
> > >  drivers/scsi/ufs/ufs-qcom.c     | 24 +++++++++++++++---------
> > >  drivers/scsi/ufs/ufshcd.c       | 36
> > > +++++++++++++++++++++++++-----------
> > >  drivers/scsi/ufs/ufshcd.h       |  8 ++++----
> > >  4 files changed, 60 insertions(+), 40 deletions(-)
> > > 
> > > diff --git a/drivers/scsi/ufs/ufs-mediatek.c
> > > b/drivers/scsi/ufs/ufs-mediatek.c
> > > index 80618af..072f4db 100644
> > > --- a/drivers/scsi/ufs/ufs-mediatek.c
> > > +++ b/drivers/scsi/ufs/ufs-mediatek.c
> > > @@ -841,27 +841,27 @@ static int ufs_mtk_link_startup_notify(struct
> > > ufs_hba *hba,
> > >  	return ret;
> > >  }
> > > 
> > > -static int ufs_mtk_device_reset(struct ufs_hba *hba)
> > > +static int ufs_mtk_device_reset(struct ufs_hba *hba, bool asserted)
> > >  {
> > >  	struct arm_smccc_res res;
> > > 
> > > -	ufs_mtk_device_reset_ctrl(0, res);
> > > +	if (asserted) {
> > > +		ufs_mtk_device_reset_ctrl(0, res);
> > > 
> > > -	/*
> > > -	 * The reset signal is active low. UFS devices shall detect
> > > -	 * more than or equal to 1us of positive or negative RST_n
> > > -	 * pulse width.
> > > -	 *
> > > -	 * To be on safe side, keep the reset low for at least 10us.
> > > -	 */
> > > -	usleep_range(10, 15);
> > > -
> > > -	ufs_mtk_device_reset_ctrl(1, res);
> > > -
> > > -	/* Some devices may need time to respond to rst_n */
> > > -	usleep_range(10000, 15000);
> > > +		/*
> > > +		 * The reset signal is active low. UFS devices shall detect
> > > +		 * more than or equal to 1us of positive or negative RST_n
> > > +		 * pulse width.
> > > +		 *
> > > +		 * To be on safe side, keep the reset low for at least 10us.
> > > +		 */
> > > +		usleep_range(10, 15);
> > 
> > I see no point in allowing vendors to "tweak" the 1us->10us adjustment.
> > The specification says 1us and we all agree that 10us gives us good
> > enough slack. I.e. this is common code.
> 
> Hi Bjron,
> 
> We tried, but Samsung fellows wanted 5us. We couldn't get a agreement
> on this delay in short term, so we chose to leave it in vops.
> 

I'm not able to find the code you're referring to.

> > 
> > > +	} else {
> > > +		ufs_mtk_device_reset_ctrl(1, res);
> > > 
> > > -	dev_info(hba->dev, "device reset done\n");
> > > +		/* Some devices may need time to respond to rst_n */
> > > +		usleep_range(10000, 15000);
> > 
> > The comment in both the Qualcomm and Mediatek drivers claim that this is
> > sleep relates to the UFS device (not host), so why should it be
> > different?
> > 
> > What happens if I take the device that Mediatek see a need for a 10ms
> > delay and hook that up to a Qualcomm host? This really should go in the
> > common code.
> > 
> 
> Agree, but Qualcomm host didn't have any problems with 10us yet, so if we
> put
> the 10ms delay to common code, Qualcomm host would suffer longer delay when
> device reset happens - both bootup and resume(xpm_lvl = 5/6) latency would
> be increased.
> 

Okay, for the resume case I accept that this is a measurable difference.
I still believe this is a property of the device and not the platform
though.

Regards,
Bjorn

> Regards,
> Can Guo.
> 
> > 
> > 
> > As such I really would prefer to see these delays in the common code!
> > You really shouldn't write code based on a speculation that one day
> > there might come someone who need other values - when that day come we
> > can just change the code, and if it never comes we're better off with
> > the cleaner implementation.
> > 
> > Regards,
> > Bjorn
> > 
> > > +	}
> > > 
> > >  	return 0;
> > >  }
> > > diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> > > index 2206b1e..fed10e5 100644
> > > --- a/drivers/scsi/ufs/ufs-qcom.c
> > > +++ b/drivers/scsi/ufs/ufs-qcom.c
> > > @@ -1406,10 +1406,11 @@ static void ufs_qcom_dump_dbg_regs(struct
> > > ufs_hba *hba)
> > >  /**
> > >   * ufs_qcom_device_reset() - toggle the (optional) device reset line
> > >   * @hba: per-adapter instance
> > > + * @asserted: assert or deassert device reset line
> > >   *
> > >   * Toggles the (optional) reset line to reset the attached device.
> > >   */
> > > -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);
> > > +	}
> > > 
> > >  	return 0;
> > >  }
> > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > > index e221add..f2daac2 100644
> > > --- a/drivers/scsi/ufs/ufshcd.c
> > > +++ b/drivers/scsi/ufs/ufshcd.c
> > > @@ -585,7 +585,13 @@ static void ufshcd_device_reset(struct ufs_hba
> > > *hba)
> > >  {
> > >  	int err;
> > > 
> > > -	err = ufshcd_vops_device_reset(hba);
> > > +	err = ufshcd_vops_device_reset(hba, true);
> > > +	if (err) {
> > > +		dev_err(hba->dev, "asserting device reset failed: %d\n", err);
> > > +		return;
> > > +	}
> > > +
> > > +	err = ufshcd_vops_device_reset(hba, false);
> > > 
> > >  	if (!err) {
> > >  		ufshcd_set_ufs_dev_active(hba);
> > > @@ -593,7 +599,11 @@ static void ufshcd_device_reset(struct ufs_hba
> > > *hba)
> > >  			hba->wb_enabled = false;
> > >  			hba->wb_buf_flush_enabled = false;
> > >  		}
> > > +		dev_dbg(hba->dev, "device reset done\n");
> > > +	} else {
> > > +		dev_err(hba->dev, "deasserting device reset failed: %d\n", err);
> > >  	}
> > > +
> > >  	if (err != -EOPNOTSUPP)
> > >  		ufshcd_update_evt_hist(hba, UFS_EVT_DEV_RESET, err);
> > >  }
> > > @@ -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
> > >

Bjorn Andersson Jan. 4, 2021, 6:59 p.m. UTC | #11

On Mon 28 Dec 19:48 CST 2020, Can Guo wrote:

> On 2020-12-29 09:18, Can Guo wrote:
> > On 2020-12-29 01:55, Bjorn Andersson wrote:
> > > On Tue 22 Dec 07:49 CST 2020, Ziqi Chen wrote:
> > > 
> > > > 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.
> > > > 
> > > > Cc: Kiwoong Kim <[e-mail]>
> > > > Cc: Stanley Chu <[e-mail]>
> > > > Signed-off-by: Ziqi Chen <[e-mail]>
> > > > ---
> > > >  drivers/scsi/ufs/ufs-mediatek.c | 32
> > > > ++++++++++++++++----------------
> > > >  drivers/scsi/ufs/ufs-qcom.c     | 24 +++++++++++++++---------
> > > >  drivers/scsi/ufs/ufshcd.c       | 36
> > > > +++++++++++++++++++++++++-----------
> > > >  drivers/scsi/ufs/ufshcd.h       |  8 ++++----
> > > >  4 files changed, 60 insertions(+), 40 deletions(-)
> > > > 
> > > > diff --git a/drivers/scsi/ufs/ufs-mediatek.c
> > > > b/drivers/scsi/ufs/ufs-mediatek.c
> > > > index 80618af..072f4db 100644
> > > > --- a/drivers/scsi/ufs/ufs-mediatek.c
> > > > +++ b/drivers/scsi/ufs/ufs-mediatek.c
> > > > @@ -841,27 +841,27 @@ static int
> > > > ufs_mtk_link_startup_notify(struct ufs_hba *hba,
> > > >  	return ret;
> > > >  }
> > > > 
> > > > -static int ufs_mtk_device_reset(struct ufs_hba *hba)
> > > > +static int ufs_mtk_device_reset(struct ufs_hba *hba, bool asserted)
> > > >  {
> > > >  	struct arm_smccc_res res;
> > > > 
> > > > -	ufs_mtk_device_reset_ctrl(0, res);
> > > > +	if (asserted) {
> > > > +		ufs_mtk_device_reset_ctrl(0, res);
> > > > 
> > > > -	/*
> > > > -	 * The reset signal is active low. UFS devices shall detect
> > > > -	 * more than or equal to 1us of positive or negative RST_n
> > > > -	 * pulse width.
> > > > -	 *
> > > > -	 * To be on safe side, keep the reset low for at least 10us.
> > > > -	 */
> > > > -	usleep_range(10, 15);
> > > > -
> > > > -	ufs_mtk_device_reset_ctrl(1, res);
> > > > -
> > > > -	/* Some devices may need time to respond to rst_n */
> > > > -	usleep_range(10000, 15000);
> > > > +		/*
> > > > +		 * The reset signal is active low. UFS devices shall detect
> > > > +		 * more than or equal to 1us of positive or negative RST_n
> > > > +		 * pulse width.
> > > > +		 *
> > > > +		 * To be on safe side, keep the reset low for at least 10us.
> > > > +		 */
> > > > +		usleep_range(10, 15);
> > > 
> > > I see no point in allowing vendors to "tweak" the 1us->10us
> > > adjustment.
> > > The specification says 1us and we all agree that 10us gives us good
> > > enough slack. I.e. this is common code.
> > 
> > Hi Bjron,
> > 
> > We tried, but Samsung fellows wanted 5us. We couldn't get a agreement
> > on this delay in short term, so we chose to leave it in vops.
> > 
> > > 
> > > > +	} else {
> > > > +		ufs_mtk_device_reset_ctrl(1, res);
> > > > 
> > > > -	dev_info(hba->dev, "device reset done\n");
> > > > +		/* Some devices may need time to respond to rst_n */
> > > > +		usleep_range(10000, 15000);
> > > 
> > > The comment in both the Qualcomm and Mediatek drivers claim that
> > > this is
> > > sleep relates to the UFS device (not host), so why should it be
> > > different?
> > > 
> > > What happens if I take the device that Mediatek see a need for a 10ms
> > > delay and hook that up to a Qualcomm host? This really should go in
> > > the
> > > common code.
> > > 
> > 
> > Agree, but Qualcomm host didn't have any problems with 10us yet, so if
> > we put
> > the 10ms delay to common code, Qualcomm host would suffer longer delay
> > when
> > device reset happens - both bootup and resume(xpm_lvl = 5/6) latency
> > would
> > be increased.
> > 
> > Regards,
> > Can Guo.
> > 
> 
> Besides, currently this device reset vops is only registered by ufs-qcom.c
> and ufs-mediatek.c, meaning any delays that we put in the common code are
> not
> necessary for those who do not have this vops registered, i.e ufs-exynos.c,
> ufs-hisi.c.
> 

Surely we can detect this in the common code and only sleep if the vops
is implemented - and successfully deasserted the reset.

Regards,
Bjorn

Can Guo Jan. 5, 2021, 1:39 a.m. UTC | #12

On 2021-01-05 02:57, Bjorn Andersson wrote:
> On Mon 28 Dec 19:18 CST 2020, Can Guo wrote:
> 
>> On 2020-12-29 01:55, Bjorn Andersson wrote:
>> > On Tue 22 Dec 07:49 CST 2020, Ziqi Chen wrote:
>> >
>> > > 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.
>> > >
>> > > Cc: Kiwoong Kim <[e-mail]>
>> > > Cc: Stanley Chu <[e-mail]>
>> > > Signed-off-by: Ziqi Chen <[e-mail]>
>> > > ---
>> > >  drivers/scsi/ufs/ufs-mediatek.c | 32 ++++++++++++++++----------------
>> > >  drivers/scsi/ufs/ufs-qcom.c     | 24 +++++++++++++++---------
>> > >  drivers/scsi/ufs/ufshcd.c       | 36
>> > > +++++++++++++++++++++++++-----------
>> > >  drivers/scsi/ufs/ufshcd.h       |  8 ++++----
>> > >  4 files changed, 60 insertions(+), 40 deletions(-)
>> > >
>> > > diff --git a/drivers/scsi/ufs/ufs-mediatek.c
>> > > b/drivers/scsi/ufs/ufs-mediatek.c
>> > > index 80618af..072f4db 100644
>> > > --- a/drivers/scsi/ufs/ufs-mediatek.c
>> > > +++ b/drivers/scsi/ufs/ufs-mediatek.c
>> > > @@ -841,27 +841,27 @@ static int ufs_mtk_link_startup_notify(struct
>> > > ufs_hba *hba,
>> > >  	return ret;
>> > >  }
>> > >
>> > > -static int ufs_mtk_device_reset(struct ufs_hba *hba)
>> > > +static int ufs_mtk_device_reset(struct ufs_hba *hba, bool asserted)
>> > >  {
>> > >  	struct arm_smccc_res res;
>> > >
>> > > -	ufs_mtk_device_reset_ctrl(0, res);
>> > > +	if (asserted) {
>> > > +		ufs_mtk_device_reset_ctrl(0, res);
>> > >
>> > > -	/*
>> > > -	 * The reset signal is active low. UFS devices shall detect
>> > > -	 * more than or equal to 1us of positive or negative RST_n
>> > > -	 * pulse width.
>> > > -	 *
>> > > -	 * To be on safe side, keep the reset low for at least 10us.
>> > > -	 */
>> > > -	usleep_range(10, 15);
>> > > -
>> > > -	ufs_mtk_device_reset_ctrl(1, res);
>> > > -
>> > > -	/* Some devices may need time to respond to rst_n */
>> > > -	usleep_range(10000, 15000);
>> > > +		/*
>> > > +		 * The reset signal is active low. UFS devices shall detect
>> > > +		 * more than or equal to 1us of positive or negative RST_n
>> > > +		 * pulse width.
>> > > +		 *
>> > > +		 * To be on safe side, keep the reset low for at least 10us.
>> > > +		 */
>> > > +		usleep_range(10, 15);
>> >
>> > I see no point in allowing vendors to "tweak" the 1us->10us adjustment.
>> > The specification says 1us and we all agree that 10us gives us good
>> > enough slack. I.e. this is common code.
>> 
>> Hi Bjron,
>> 
>> We tried, but Samsung fellows wanted 5us. We couldn't get a agreement
>> on this delay in short term, so we chose to leave it in vops.
>> 
> 
> I'm not able to find the code you're referring to.

static void exynos_ufs_dev_hw_reset(struct ufs_hba *hba)
{
         struct exynos_ufs *ufs = ufshcd_get_variant(hba);

         hci_writel(ufs, 0 << 0, HCI_GPIO_OUT);
         udelay(5);
         hci_writel(ufs, 1 << 0, HCI_GPIO_OUT);
}

> 
>> >
>> > > +	} else {
>> > > +		ufs_mtk_device_reset_ctrl(1, res);
>> > >
>> > > -	dev_info(hba->dev, "device reset done\n");
>> > > +		/* Some devices may need time to respond to rst_n */
>> > > +		usleep_range(10000, 15000);
>> >
>> > The comment in both the Qualcomm and Mediatek drivers claim that this is
>> > sleep relates to the UFS device (not host), so why should it be
>> > different?
>> >
>> > What happens if I take the device that Mediatek see a need for a 10ms
>> > delay and hook that up to a Qualcomm host? This really should go in the
>> > common code.
>> >
>> 
>> Agree, but Qualcomm host didn't have any problems with 10us yet, so if 
>> we
>> put
>> the 10ms delay to common code, Qualcomm host would suffer longer delay 
>> when
>> device reset happens - both bootup and resume(xpm_lvl = 5/6) latency 
>> would
>> be increased.
>> 
> 
> Okay, for the resume case I accept that this is a measurable 
> difference.
> I still believe this is a property of the device and not the platform
> though.
> 

True, I also wanted to make the codes simpler and universal for 
everyone...

> Regards,
> Bjorn
> 
>> Regards,
>> Can Guo.
>> 
>> >
>> >
>> > As such I really would prefer to see these delays in the common code!
>> > You really shouldn't write code based on a speculation that one day
>> > there might come someone who need other values - when that day come we
>> > can just change the code, and if it never comes we're better off with
>> > the cleaner implementation.
>> >
>> > Regards,
>> > Bjorn
>> >
>> > > +	}
>> > >
>> > >  	return 0;
>> > >  }
>> > > diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
>> > > index 2206b1e..fed10e5 100644
>> > > --- a/drivers/scsi/ufs/ufs-qcom.c
>> > > +++ b/drivers/scsi/ufs/ufs-qcom.c
>> > > @@ -1406,10 +1406,11 @@ static void ufs_qcom_dump_dbg_regs(struct
>> > > ufs_hba *hba)
>> > >  /**
>> > >   * ufs_qcom_device_reset() - toggle the (optional) device reset line
>> > >   * @hba: per-adapter instance
>> > > + * @asserted: assert or deassert device reset line
>> > >   *
>> > >   * Toggles the (optional) reset line to reset the attached device.
>> > >   */
>> > > -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);
>> > > +	}
>> > >
>> > >  	return 0;
>> > >  }
>> > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> > > index e221add..f2daac2 100644
>> > > --- a/drivers/scsi/ufs/ufshcd.c
>> > > +++ b/drivers/scsi/ufs/ufshcd.c
>> > > @@ -585,7 +585,13 @@ static void ufshcd_device_reset(struct ufs_hba
>> > > *hba)
>> > >  {
>> > >  	int err;
>> > >
>> > > -	err = ufshcd_vops_device_reset(hba);
>> > > +	err = ufshcd_vops_device_reset(hba, true);
>> > > +	if (err) {
>> > > +		dev_err(hba->dev, "asserting device reset failed: %d\n", err);
>> > > +		return;
>> > > +	}
>> > > +
>> > > +	err = ufshcd_vops_device_reset(hba, false);
>> > >
>> > >  	if (!err) {
>> > >  		ufshcd_set_ufs_dev_active(hba);
>> > > @@ -593,7 +599,11 @@ static void ufshcd_device_reset(struct ufs_hba
>> > > *hba)
>> > >  			hba->wb_enabled = false;
>> > >  			hba->wb_buf_flush_enabled = false;
>> > >  		}
>> > > +		dev_dbg(hba->dev, "device reset done\n");
>> > > +	} else {
>> > > +		dev_err(hba->dev, "deasserting device reset failed: %d\n", err);
>> > >  	}
>> > > +
>> > >  	if (err != -EOPNOTSUPP)
>> > >  		ufshcd_update_evt_hist(hba, UFS_EVT_DEV_RESET, err);
>> > >  }
>> > > @@ -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
>> > >

Adrian Hunter Jan. 5, 2021, 7:16 a.m. UTC | #13

On 4/01/21 8:55 pm, Bjorn Andersson wrote:
> On Mon 04 Jan 03:15 CST 2021, Adrian Hunter wrote:
> 
>> On 22/12/20 3:49 pm, Ziqi Chen wrote:
>>> 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.
>>
>> This patch assumes the power is controlled by voltage regulators, but for us
>> it is controlled by firmware (ACPI), so it is not correct to change RST_n
>> for all host controllers as you are doing.
>>
>> Also we might need to use a firmware interface for device reset, in which
>> case the 'asserted' value doe not make sense.
>>
> 
> Are you saying that the entire flip-flop-the-reset is a single firmware
> operation in your case?

Yes

>                         If you look at the Mediatek driver, the
> implementation of ufs_mtk_device_reset_ctrl() is a jump to firmware.
> 
> 
> But perhaps "asserted" isn't the appropriate English word for saying
> "the reset is in the resetting state"?
> 
> I just wanted to avoid the use of "high"/"lo" as if you look at the
> Mediatek code they pass the expected line-level to the firmware, while
> in the Qualcomm code we pass the logical state to the GPIO code which is
> setup up as "active low" and thereby flip the meaning before hitting the
> pad.
> 
>> Can we leave the device reset callback alone, and instead introduce a new
>> variant operation for setting RST_n to match voltage regulator power changes?
> 
> Wouldn't this new function just have to look like the proposed patches?
> In which case for existing platforms we'd have both?
> 
> How would you implement this, or would you simply skip implementing
> this?

Functionally, doing a device reset is not the same as adjusting signal
levels to meet power up/off ramp requirements.  However, the issue is that
we do not use regulators, so the power is not necessarily being changed at
those points, and we definitely do not want to reset instead of entering
DeepSleep for example.

Off the top of my head, I imagine something like a callback called
ufshcd_vops_prepare_power_ramp(hba, bool on) which is called only if
hba->vreg_info->vcc is not NULL.

Can Guo Jan. 5, 2021, 7:28 a.m. UTC | #14

On 2021-01-05 15:16, Adrian Hunter wrote:
> On 4/01/21 8:55 pm, Bjorn Andersson wrote:
>> On Mon 04 Jan 03:15 CST 2021, Adrian Hunter wrote:
>> 
>>> On 22/12/20 3:49 pm, Ziqi Chen wrote:
>>>> 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.
>>> 
>>> This patch assumes the power is controlled by voltage regulators, but 
>>> for us
>>> it is controlled by firmware (ACPI), so it is not correct to change 
>>> RST_n
>>> for all host controllers as you are doing.
>>> 
>>> Also we might need to use a firmware interface for device reset, in 
>>> which
>>> case the 'asserted' value doe not make sense.
>>> 
>> 
>> Are you saying that the entire flip-flop-the-reset is a single 
>> firmware
>> operation in your case?
> 
> Yes
> 
>>                         If you look at the Mediatek driver, the
>> implementation of ufs_mtk_device_reset_ctrl() is a jump to firmware.
>> 
>> 
>> But perhaps "asserted" isn't the appropriate English word for saying
>> "the reset is in the resetting state"?
>> 
>> I just wanted to avoid the use of "high"/"lo" as if you look at the
>> Mediatek code they pass the expected line-level to the firmware, while
>> in the Qualcomm code we pass the logical state to the GPIO code which 
>> is
>> setup up as "active low" and thereby flip the meaning before hitting 
>> the
>> pad.
>> 
>>> Can we leave the device reset callback alone, and instead introduce a 
>>> new
>>> variant operation for setting RST_n to match voltage regulator power 
>>> changes?
>> 
>> Wouldn't this new function just have to look like the proposed 
>> patches?
>> In which case for existing platforms we'd have both?
>> 
>> How would you implement this, or would you simply skip implementing
>> this?
> 
> Functionally, doing a device reset is not the same as adjusting signal
> levels to meet power up/off ramp requirements.  However, the issue is 
> that
> we do not use regulators, so the power is not necessarily being changed 
> at
> those points, and we definitely do not want to reset instead of 
> entering
> DeepSleep for example.
> 
> Off the top of my head, I imagine something like a callback called
> ufshcd_vops_prepare_power_ramp(hba, bool on) which is called only if
> hba->vreg_info->vcc is not NULL.

Hi Adrian,

I don't see you have the vops device_reset() implemented anywhere in
current code base, how is this change impacting you? Do I miss anything
or are you planning to push a change which implements device_reset() 
soon?

Thanks,
Can Guo.

Adrian Hunter Jan. 5, 2021, 7:33 a.m. UTC | #15

On 5/01/21 9:28 am, Can Guo wrote:
> On 2021-01-05 15:16, Adrian Hunter wrote:
>> On 4/01/21 8:55 pm, Bjorn Andersson wrote:
>>> On Mon 04 Jan 03:15 CST 2021, Adrian Hunter wrote:
>>>
>>>> On 22/12/20 3:49 pm, Ziqi Chen wrote:
>>>>> 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.
>>>>
>>>> This patch assumes the power is controlled by voltage regulators, but
>>>> for us
>>>> it is controlled by firmware (ACPI), so it is not correct to change RST_n
>>>> for all host controllers as you are doing.
>>>>
>>>> Also we might need to use a firmware interface for device reset, in which
>>>> case the 'asserted' value doe not make sense.
>>>>
>>>
>>> Are you saying that the entire flip-flop-the-reset is a single firmware
>>> operation in your case?
>>
>> Yes
>>
>>>                         If you look at the Mediatek driver, the
>>> implementation of ufs_mtk_device_reset_ctrl() is a jump to firmware.
>>>
>>>
>>> But perhaps "asserted" isn't the appropriate English word for saying
>>> "the reset is in the resetting state"?
>>>
>>> I just wanted to avoid the use of "high"/"lo" as if you look at the
>>> Mediatek code they pass the expected line-level to the firmware, while
>>> in the Qualcomm code we pass the logical state to the GPIO code which is
>>> setup up as "active low" and thereby flip the meaning before hitting the
>>> pad.
>>>
>>>> Can we leave the device reset callback alone, and instead introduce a new
>>>> variant operation for setting RST_n to match voltage regulator power
>>>> changes?
>>>
>>> Wouldn't this new function just have to look like the proposed patches?
>>> In which case for existing platforms we'd have both?
>>>
>>> How would you implement this, or would you simply skip implementing
>>> this?
>>
>> Functionally, doing a device reset is not the same as adjusting signal
>> levels to meet power up/off ramp requirements.  However, the issue is that
>> we do not use regulators, so the power is not necessarily being changed at
>> those points, and we definitely do not want to reset instead of entering
>> DeepSleep for example.
>>
>> Off the top of my head, I imagine something like a callback called
>> ufshcd_vops_prepare_power_ramp(hba, bool on) which is called only if
>> hba->vreg_info->vcc is not NULL.
> 
> Hi Adrian,
> 
> I don't see you have the vops device_reset() implemented anywhere in
> current code base, how is this change impacting you? Do I miss anything
> or are you planning to push a change which implements device_reset() soon?

At some point, yes.

Can Guo Jan. 5, 2021, 10:06 a.m. UTC | #16

On 2021-01-05 15:33, Adrian Hunter wrote:
> On 5/01/21 9:28 am, Can Guo wrote:
>> On 2021-01-05 15:16, Adrian Hunter wrote:
>>> On 4/01/21 8:55 pm, Bjorn Andersson wrote:
>>>> On Mon 04 Jan 03:15 CST 2021, Adrian Hunter wrote:
>>>> 
>>>>> On 22/12/20 3:49 pm, Ziqi Chen wrote:
>>>>>> 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.
>>>>> 
>>>>> This patch assumes the power is controlled by voltage regulators, 
>>>>> but
>>>>> for us
>>>>> it is controlled by firmware (ACPI), so it is not correct to change 
>>>>> RST_n
>>>>> for all host controllers as you are doing.
>>>>> 
>>>>> Also we might need to use a firmware interface for device reset, in 
>>>>> which
>>>>> case the 'asserted' value doe not make sense.
>>>>> 
>>>> 
>>>> Are you saying that the entire flip-flop-the-reset is a single 
>>>> firmware
>>>> operation in your case?
>>> 
>>> Yes
>>> 
>>>>                         If you look at the Mediatek driver, the
>>>> implementation of ufs_mtk_device_reset_ctrl() is a jump to firmware.
>>>> 
>>>> 
>>>> But perhaps "asserted" isn't the appropriate English word for saying
>>>> "the reset is in the resetting state"?
>>>> 
>>>> I just wanted to avoid the use of "high"/"lo" as if you look at the
>>>> Mediatek code they pass the expected line-level to the firmware, 
>>>> while
>>>> in the Qualcomm code we pass the logical state to the GPIO code 
>>>> which is
>>>> setup up as "active low" and thereby flip the meaning before hitting 
>>>> the
>>>> pad.
>>>> 
>>>>> Can we leave the device reset callback alone, and instead introduce 
>>>>> a new
>>>>> variant operation for setting RST_n to match voltage regulator 
>>>>> power
>>>>> changes?
>>>> 
>>>> Wouldn't this new function just have to look like the proposed 
>>>> patches?
>>>> In which case for existing platforms we'd have both?
>>>> 
>>>> How would you implement this, or would you simply skip implementing
>>>> this?
>>> 
>>> Functionally, doing a device reset is not the same as adjusting 
>>> signal
>>> levels to meet power up/off ramp requirements.  However, the issue is 
>>> that
>>> we do not use regulators, so the power is not necessarily being 
>>> changed at
>>> those points, and we definitely do not want to reset instead of 
>>> entering
>>> DeepSleep for example.
>>> 
>>> Off the top of my head, I imagine something like a callback called
>>> ufshcd_vops_prepare_power_ramp(hba, bool on) which is called only if
>>> hba->vreg_info->vcc is not NULL.
>> 
>> Hi Adrian,
>> 
>> I don't see you have the vops device_reset() implemented anywhere in
>> current code base, how is this change impacting you? Do I miss 
>> anything
>> or are you planning to push a change which implements device_reset() 
>> soon?
> 
> At some point, yes.

OK, then we don't even have to add a new vops, just go back to version 
#1 to
use ufshcd_vops_suspend() to control the device_reset. We took the hard 
way
because we wanted to fix it for all users.

Patch

diff --git a/drivers/scsi/ufs/ufs-mediatek.c b/drivers/scsi/ufs/ufs-mediatek.c
index 80618af..072f4db 100644
--- a/drivers/scsi/ufs/ufs-mediatek.c
+++ b/drivers/scsi/ufs/ufs-mediatek.c
@@ -841,27 +841,27 @@  static int ufs_mtk_link_startup_notify(struct ufs_hba *hba,
 	return ret;
 }
 
-static int ufs_mtk_device_reset(struct ufs_hba *hba)
+static int ufs_mtk_device_reset(struct ufs_hba *hba, bool asserted)
 {
 	struct arm_smccc_res res;
 
-	ufs_mtk_device_reset_ctrl(0, res);
+	if (asserted) {
+		ufs_mtk_device_reset_ctrl(0, res);
 
-	/*
-	 * The reset signal is active low. UFS devices shall detect
-	 * more than or equal to 1us of positive or negative RST_n
-	 * pulse width.
-	 *
-	 * To be on safe side, keep the reset low for at least 10us.
-	 */
-	usleep_range(10, 15);
-
-	ufs_mtk_device_reset_ctrl(1, res);
-
-	/* Some devices may need time to respond to rst_n */
-	usleep_range(10000, 15000);
+		/*
+		 * The reset signal is active low. UFS devices shall detect
+		 * more than or equal to 1us of positive or negative RST_n
+		 * pulse width.
+		 *
+		 * To be on safe side, keep the reset low for at least 10us.
+		 */
+		usleep_range(10, 15);
+	} else {
+		ufs_mtk_device_reset_ctrl(1, res);
 
-	dev_info(hba->dev, "device reset done\n");
+		/* Some devices may need time to respond to rst_n */
+		usleep_range(10000, 15000);
+	}
 
 	return 0;
 }
diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
index 2206b1e..fed10e5 100644
--- a/drivers/scsi/ufs/ufs-qcom.c
+++ b/drivers/scsi/ufs/ufs-qcom.c
@@ -1406,10 +1406,11 @@  static void ufs_qcom_dump_dbg_regs(struct ufs_hba *hba)
 /**
  * ufs_qcom_device_reset() - toggle the (optional) device reset line
  * @hba: per-adapter instance
+ * @asserted: assert or deassert device reset line
  *
  * Toggles the (optional) reset line to reset the attached device.
  */
-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);
+	}
 
 	return 0;
 }
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index e221add..f2daac2 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -585,7 +585,13 @@  static void ufshcd_device_reset(struct ufs_hba *hba)
 {
 	int err;
 
-	err = ufshcd_vops_device_reset(hba);
+	err = ufshcd_vops_device_reset(hba, true);
+	if (err) {
+		dev_err(hba->dev, "asserting device reset failed: %d\n", err);
+		return;
+	}
+
+	err = ufshcd_vops_device_reset(hba, false);
 
 	if (!err) {
 		ufshcd_set_ufs_dev_active(hba);
@@ -593,7 +599,11 @@  static void ufshcd_device_reset(struct ufs_hba *hba)
 			hba->wb_enabled = false;
 			hba->wb_buf_flush_enabled = false;
 		}
+		dev_dbg(hba->dev, "device reset done\n");
+	} else {
+		dev_err(hba->dev, "deasserting device reset failed: %d\n", err);
 	}
+
 	if (err != -EOPNOTSUPP)
 		ufshcd_update_evt_hist(hba, UFS_EVT_DEV_RESET, err);
 }
@@ -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;
 }