[V6,0/6] scsi: ufs: qcom: Align programming sequence as per HW spec

This patch aligns programming sequence as per Qualcomm UFS
hardware specification.

changes from v5:
- Addressed Mani comment to FIELD_PREP and FIELD_FIT.
- Optimised ufs_qcom_set_core_clk_ctrl API.
- Updated commit text for few patches to capture more details.

Changes from v4:
- Addressed bjorn comment to split single patch to multiple patches.

Changes from v3:
-Addressed bjorn comment to update commit msg to capture change details.

Changes from v2:
- Addressed bao comment, removed duplicate clock timer cfg API call

Changes from v1:
- Addressed bao comment, removed wrapper function
- Tab alignment

Nitin Rawat (6):
  scsi: ufs: qcom: Align mask for core_clk_1us_cycles
  scsi: ufs: qcom: Configure PA_VS_CORE_CLK_40NS_CYCLES
  scsi: ufs: qcom: Add multiple frequency support for unipro clk
    attributes
  scsi: ufs: qcom: Align unipro clk attributes configuration as per HPG
  scsi: ufs: qcom: Refactor ufs_qcom_cfg_timers function.
  scsi: ufs: qcom: Configure clk HW division based on scaling
    conditions.

 drivers/ufs/host/ufs-qcom.c | 240 ++++++++++++++++++++++++++----------
 drivers/ufs/host/ufs-qcom.h |  17 ++-
 2 files changed, 193 insertions(+), 64 deletions(-)

--
2.17.1

On 1.09.2023 13:43, Nitin Rawat wrote:
> This patch aligns programming sequence as per Qualcomm UFS
> hardware specification.
> 
> changes from v5:
> - Addressed Mani comment to FIELD_PREP and FIELD_FIT.
> - Optimised ufs_qcom_set_core_clk_ctrl API.
> - Updated commit text for few patches to capture more details.
> 
Any reason I received this twice?

Konrad

On Fri, Sep 01, 2023 at 05:13:30PM +0530, Nitin Rawat wrote:
> This patch aligns programming sequence as per Qualcomm UFS
> hardware specification.

reading this series, it is difficult for me to understand as a user of
the driver if this should have any noticeable effect.

Some of the patches mention that there is no functional change, some
only say align with the HPG but change programming sequence, frequency,
etc if I understand correctly on a quick glance.

I think being a bit verbose in some of the patches with respect to
explaining the effect of the patch (or lack of a noticeable effect)
would be a beneficial improvement to this series if there's another
version.

I agree that aligning with the HPG instead of doing some undefined
sequence is a good idea, I'm just reading some of the changes and
thinking "I have no idea if this is going to fix something (no Fixes:
tag but it almost sounds like one), will this improve something, or will this
just change the programming sequence to a known and recommended
sequence?".

Thanks for the patches,
Andrew

On Fri, Sep 01, 2023 at 09:56:05AM -0500, Andrew Halaney wrote:
> On Fri, Sep 01, 2023 at 05:13:30PM +0530, Nitin Rawat wrote:
> > This patch aligns programming sequence as per Qualcomm UFS
> > hardware specification.
> 
> reading this series, it is difficult for me to understand as a user of
> the driver if this should have any noticeable effect.
> 
> Some of the patches mention that there is no functional change, some
> only say align with the HPG but change programming sequence, frequency,
> etc if I understand correctly on a quick glance.
> 
> I think being a bit verbose in some of the patches with respect to
> explaining the effect of the patch (or lack of a noticeable effect)
> would be a beneficial improvement to this series if there's another
> version.
> 
> I agree that aligning with the HPG instead of doing some undefined
> sequence is a good idea, I'm just reading some of the changes and
> thinking "I have no idea if this is going to fix something (no Fixes:
> tag but it almost sounds like one), will this improve something, or will this
> just change the programming sequence to a known and recommended
> sequence?".
> 

Very valid feedback, Andrew.

Correct or not, there are a fair amount of users out there who runs the
current implementation. Changes to that should be described in a way
that doesn't depend on inside-knowledge.

Regards,
Bjorn

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@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.

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

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