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

[Nitin Rawat <quic_nitirawa@quicinc.com>]

On 9/1/2023 9:36 PM, Bjorn Andersson wrote:
> 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.
Sure..Will update the commit text to describe the problem we are solving
in next patchset.

> 
>>
>> Co-developed-by: Naveen Kumar Goud Arepalli <quic_narepall@quicinc.com>
>> Signed-off-by: Naveen Kumar Goud Arepalli <quic_narepall@quicinc.com>
>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
>> ---
>>   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.
Sure..Will take care of this in next patchset.

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

> 
> 
> You're changing the prototype of the function, but you're not updating
> the kernel-doc above, please correct this.
I'll update the kernel doc in next patchset.

> 
>> +				 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.
- Sure..Will correct the code identation.


> 
>> +	    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.
Sure..Will take care of this in next patchset.

> 
> 
> 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.
Sure...Would send separate patch to include hibernation change.


> 
> Regards,
> Bjorn

Thanks,
Nitin