Re: [PATCH V6 5/6] scsi: ufs: qcom: Refactor ufs_qcom_cfg_timers function.

[Bjorn Andersson]

On Fri, Sep 01, 2023 at 05:13:35PM +0530, Nitin Rawat wrote:
> a)Configure SYS1CLK_1US_REG for clock scaleup pre ops.
> b)Move ufs_qcom_cfg_timers from clk scaling post change ops
> to clk scaling pre change ops to align with the HPG.
> c)Introduce a new argument is_pre_scale_up for ufs_qcom_cfg_timers
> to configure sys_1us timer for max freq as per HPG.

You forgot to describe the problem you're trying to solve. This is just
a summary of the changes done in the code.

> 
> Co-developed-by: Naveen Kumar Goud Arepalli <quic_narepall@xxxxxxxxxxx>
> Signed-off-by: Naveen Kumar Goud Arepalli <quic_narepall@xxxxxxxxxxx>
> Signed-off-by: Nitin Rawat <quic_nitirawa@xxxxxxxxxxx>
> ---
>  drivers/ufs/host/ufs-qcom.c | 63 +++++++++++++++++++++++++++----------
>  1 file changed, 46 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index d670fcc27ffb..c251c98a74f0 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -532,7 +532,8 @@ static int ufs_qcom_hce_enable_notify(struct ufs_hba *hba,
>   * Return: zero for success and non-zero in case of a failure.
>   */
>  static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
> -			       u32 hs, u32 rate, bool update_link_startup_timer)

Again, it doesn't seem like this variable name change has any functional
impact, so please don't change it.

PS. You're very welcome to send separate cleanup patches just making the
code easier to read, but please do so separately.


You're changing the prototype of the function, but you're not updating
the kernel-doc above, please correct this.

> +				 u32 hs, u32 rate, bool link_startup,
> +				 bool is_pre_scale_up)
>  {
>  	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>  	struct ufs_clk_info *clki;
> @@ -563,11 +564,16 @@ static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
>  	/*
>  	 * The Qunipro controller does not use following registers:
>  	 * SYS1CLK_1US_REG, TX_SYMBOL_CLK_1US_REG, CLK_NS_REG &
> -	 * UFS_REG_PA_LINK_STARTUP_TIMER
> -	 * But UTP controller uses SYS1CLK_1US_REG register for Interrupt
> -	 * Aggregation logic.
> -	*/
> -	if (ufs_qcom_cap_qunipro(host) && !ufshcd_is_intr_aggr_allowed(hba))
> +	 * UFS_REG_PA_LINK_STARTUP_TIMER.
> +	 * However UTP controller uses SYS1CLK_1US_REG register for Interrupt
> +	 * Aggregation logic and Auto hibern8 logic.
> +	 * It is mandatory to write SYS1CLK_1US_REG register on UFS host
> +	 * controller V4.0.0 onwards.
> +	 */
> +	if (ufs_qcom_cap_qunipro(host) &&
> +	    !(ufshcd_is_intr_aggr_allowed(hba) ||
> +	    ufshcd_is_auto_hibern8_supported(hba) ||

This line is indented to the start of the condition, but it's actually
part of the !() expression starting on the line above. Please indent it
to align with the ufshcd_is_intr_aggr_allowed() to make this obvious.

> +	    host->hw_ver.major >= 4))

I think that this condition is added so that the return will only happen
on hw_ver < 4. But if it's "significant", why is it hidden in the inner
expression and not part of the outer expression.

  cap && !(aggr || h8 || ver >= 4)

vs

  ver < 4 && cap && !(aggr || h8)

The latter prunes the option space much faster than the latter.


Second, there are two changes to this condition in this one patch; one
affects hw_ver >= 4, and the other affects any previous target
supporting hibernation.

Please separate these changes, to facilitate debugging any effects of
the hibernation change.

Regards,
Bjorn