[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <f6b030cc-6ec7-d522-d797-7b84cc48a66a@quicinc.com>
Date: Mon, 4 Sep 2023 20:18:10 +0530
From: Nitin Rawat <quic_nitirawa@...cinc.com>
To: Bjorn Andersson <quic_bjorande@...cinc.com>
CC: <mani@...nel.org>, <agross@...nel.org>, <andersson@...nel.org>,
<konrad.dybcio@...aro.org>, <jejb@...ux.ibm.com>,
<martin.petersen@...cle.com>, <quic_cang@...cinc.com>,
<quic_nguyenb@...cinc.com>, <linux-scsi@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
"Naveen Kumar Goud Arepalli" <quic_narepall@...cinc.com>
Subject: Re: [PATCH V6 5/6] scsi: ufs: qcom: Refactor ufs_qcom_cfg_timers
function.
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@...cinc.com>
>> Signed-off-by: Naveen Kumar Goud Arepalli <quic_narepall@...cinc.com>
>> Signed-off-by: Nitin Rawat <quic_nitirawa@...cinc.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
Powered by blists - more mailing lists