Re: [PATCH v4 5/8] scsi: ufs: core: Enable multi-level gear scaling

[Ziqi Chen <quic_ziqichen@quicinc.com>]

On 2/11/2025 5:28 PM, Peter Wang (王信友) wrote:
> On Mon, 2025-02-10 at 18:02 +0800, Ziqi Chen wrote:
>>
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>
>>
>> From: Can Guo <quic_cang@quicinc.com>
>>
>> With OPP V2 enabled, devfreq can scale clocks amongst multiple
>> frequency
>> plans. However, the gear speed is only toggled between min and max
>> during
>> clock scaling. Enable multi-level gear scaling by mapping clock
>> frequencies
>> to gear speeds, so that when devfreq scales clock frequencies we can
>> put
>> the UFS link at the appropriate gear speeds accordingly.
>>
>> Signed-off-by: Can Guo <quic_cang@quicinc.com>
>> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>> Reviewed-by: Bean Huo <beanhuo@micron.com>
>> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
>> Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
>>
>> v1 -> v2:
>> Rename the lable "do_pmc" to "config_pwr_mode".
>>
>> v2 -> v3:
>> Use assignment instead memcpy() in function ufshcd_scale_gear().
>>
>> v3 -> v4:
>> Typo fixed for commit message.
>> ---
>>   drivers/ufs/core/ufshcd.c | 51 +++++++++++++++++++++++++++++++------
>> --
>>   1 file changed, 41 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>> index 8d295cc827cc..ebab897080a6 100644
>> --- a/drivers/ufs/core/ufshcd.c
>> +++ b/drivers/ufs/core/ufshcd.c
>> @@ -1308,16 +1308,26 @@ static int
>> ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba,
>>   /**
>>    * ufshcd_scale_gear - scale up/down UFS gear
>>    * @hba: per adapter instance
>> + * @target_gear: target gear to scale to
>>    * @scale_up: True for scaling up gear and false for scaling down
>>    *
>>    * Return: 0 for success; -EBUSY if scaling can't happen at this
>> time;
>>    * non-zero for any other errors.
>>    */
>> -static int ufshcd_scale_gear(struct ufs_hba *hba, bool scale_up)
>> +static int ufshcd_scale_gear(struct ufs_hba *hba, u32 target_gear,
>> bool scale_up)
>>   {
>>          int ret = 0;
>>          struct ufs_pa_layer_attr new_pwr_info;
>>
>> +       if (target_gear) {
>> +               new_pwr_info = hba->pwr_info;
>> +               new_pwr_info.gear_tx = target_gear;
>> +               new_pwr_info.gear_rx = target_gear;
>> +
>> +               goto config_pwr_mode;
>> +       }
>> +
>> +       /* Legacy gear scaling, in case vops_freq_to_gear_speed() is
>> not implemented */
>>          if (scale_up) {
>>                  memcpy(&new_pwr_info, &hba-
>>> clk_scaling.saved_pwr_info,
>>                         sizeof(struct ufs_pa_layer_attr));
>> @@ -1338,6 +1348,7 @@ static int ufshcd_scale_gear(struct ufs_hba
>> *hba, bool scale_up)
>>                  }
>>          }
>>
>> +config_pwr_mode:
>>          /* check if the power mode needs to be changed or not? */
>>          ret = ufshcd_config_pwr_mode(hba, &new_pwr_info);
>>          if (ret)
>> @@ -1408,15 +1419,26 @@ static void
>> ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, int err, bool sc
>>   static int ufshcd_devfreq_scale(struct ufs_hba *hba, unsigned long
>> freq,
>>                                  bool scale_up)
>>   {
>> +       u32 old_gear = hba->pwr_info.gear_rx;
>> +       int new_gear = 0;
>>          int ret = 0;
>>
>> +       new_gear = ufshcd_vops_freq_to_gear_speed(hba, freq);
>> +       if (new_gear < 0)
>> +               /*
>> +                * return negative value means that the
>> vops_freq_to_gear_speed() is not
>> +                * implemented or didn't find matched gear speed,
>> assign '0' to new_gear
>> +                * to switch to legacy gear scaling sequence in
>> ufshcd_scale_gear().
>> +                */
>> +               new_gear = 0;
>> +
>>
> 
> Hi Ziqi,
> 
> I think remove help function is better.
> No need change new_gear type when use.
> The readability is higher, and no need add that large amount comments.
> 
>         u32_new_gear = 0;
>         if (hba->vops && hba->vops->freq_to_gear_speed)
>                 new_gear = hba->vops->freq_to_gear_speed(hba, freq);
> 
> 
> Thanks.
> Peter
> 
> 
Hi Peter,

Thanks, Peter.
Frankly, I also think this way has low readability. However, keep the 
u32 type for new_gear is OK to me. But this vop would lose the ability 
to indicate the error types. All types of error can only return "0".

However, we don't need to deal with various types of errors up to now, I 
can submit a new version to change back the new_gear and vop return 
value type to u32 and make correspondingly change in patch 3/8 and 4/8.

-Ziqi

> 
>>          ret = ufshcd_clock_scaling_prepare(hba, 1 * USEC_PER_SEC);
>>          if (ret)
>>                  return ret;
>>
>>          /* scale down the gear before scaling down clocks */
>>          if (!scale_up) {
>> -               ret = ufshcd_scale_gear(hba, false);
>> +               ret = ufshcd_scale_gear(hba, (u32)new_gear, false);
>>                  if (ret)
>>                          goto out_unprepare;
>>          }
>> @@ -1424,13 +1446,13 @@ static int ufshcd_devfreq_scale(struct
>> ufs_hba *hba, unsigned long freq,
>>          ret = ufshcd_scale_clks(hba, freq, scale_up);
>>          if (ret) {
>>                  if (!scale_up)
>> -                       ufshcd_scale_gear(hba, true);
>> +                       ufshcd_scale_gear(hba, old_gear, true);
>>                  goto out_unprepare;
>>          }
>>
>>          /* scale up the gear after scaling up clocks */
>>          if (scale_up) {
>> -               ret = ufshcd_scale_gear(hba, true);
>> +               ret = ufshcd_scale_gear(hba, (u32)new_gear, true);
>>                  if (ret) {
>>                          ufshcd_scale_clks(hba, hba->devfreq-
>>> previous_freq,
>>                                            false);
>> @@ -1723,6 +1745,8 @@ static ssize_t
>> ufshcd_clkscale_enable_store(struct device *dev,
>>                  struct device_attribute *attr, const char *buf,
>> size_t count)
>>   {
>>          struct ufs_hba *hba = dev_get_drvdata(dev);
>> +       struct ufs_clk_info *clki;
>> +       unsigned long freq;
>>          u32 value;
>>          int err = 0;
>>
>> @@ -1746,14 +1770,21 @@ static ssize_t
>> ufshcd_clkscale_enable_store(struct device *dev,
>>
>>          if (value) {
>>                  ufshcd_resume_clkscaling(hba);
>> -       } else {
>> -               ufshcd_suspend_clkscaling(hba);
>> -               err = ufshcd_devfreq_scale(hba, ULONG_MAX, true);
>> -               if (err)
>> -                       dev_err(hba->dev, "%s: failed to scale clocks
>> up %d\n",
>> -                                       __func__, err);
>> +               goto out_rel;
>>          }
>>
>> +       clki = list_first_entry(&hba->clk_list_head, struct
>> ufs_clk_info, list);
>> +       freq = clki->max_freq;
>> +
>> +       ufshcd_suspend_clkscaling(hba);
>> +       err = ufshcd_devfreq_scale(hba, freq, true);
>> +       if (err)
>> +               dev_err(hba->dev, "%s: failed to scale clocks up
>> %d\n",
>> +                               __func__, err);
>> +       else
>> +               hba->clk_scaling.target_freq = freq;
>> +
>> +out_rel:
>>          ufshcd_release(hba);
>>          ufshcd_rpm_put_sync(hba);
>>   out:
>> --
>> 2.34.1
>>
>