| Message ID | 20250122100214.489749-4-quic_ziqichen@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Support Multi-frequency scale for UFS | expand |
On Wed, 2025-01-22 at 18:02 +0800, Ziqi Chen wrote: > From: Can Guo <quic_cang@quicinc.com> > > Add a vops to map UFS host controller clock frequencies to the > maximum > supported UFS high speed gear speeds. During clock scaling, we can > map the > target clock frequency, demanded by devfreq, to the maximum supported > gear > speed, so that devfreq can scale the gear to the highest gear speed > supported at the target clock frequency, instead of just scaling > up/down > the gear between the min and max gear speeds. > > Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com> > Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com> > Signed-off-by: Can Guo <quic_cang@quicinc.com> Reviewed-by: Bean Huo <beanhuo@micron.com>
On 1/22/25 2:02 AM, Ziqi Chen wrote: > From: Can Guo <quic_cang@quicinc.com> > > Add a vops to map UFS host controller clock frequencies to the maximum Above and in the subject, please change "vops" into "vop" (variant operation). > struct ufs_hba_variant_ops { > const char *name; > @@ -387,6 +388,8 @@ struct ufs_hba_variant_ops { > unsigned long *ocqs); > int (*config_esi)(struct ufs_hba *hba); > void (*config_scsi_dev)(struct scsi_device *sdev); > + int (*freq_to_gear_speed)(struct ufs_hba *hba, unsigned long freq, > + u32 *gear); > }; Please keep the indentation consistent. Thanks, Bart.
On 1/22/25 2:02 AM, Ziqi Chen wrote: > +static inline int ufshcd_vops_freq_to_gear_speed(struct ufs_hba *hba, > + unsigned long freq, > + u32 *gear) > +{ > + if (hba->vops && hba->vops->freq_to_gear_speed) > + return hba->vops->freq_to_gear_speed(hba, freq, gear); > + > + return -EOPNOTSUPP; > +} Please remove "vops_" from the function name. I don't think this part of the function name is useful. Additionally, please return the gear value as the function result and remove the "u32 *gear" argument. Thanks, Bart.
Hi Bart, Thanks for you review~ On 1/23/2025 2:22 AM, Bart Van Assche wrote: > On 1/22/25 2:02 AM, Ziqi Chen wrote: >> From: Can Guo <quic_cang@quicinc.com> >> >> Add a vops to map UFS host controller clock frequencies to the maximum > > Above and in the subject, please change "vops" into "vop" (variant > operation). > OK , will update it in next version. >> struct ufs_hba_variant_ops { >> const char *name; >> @@ -387,6 +388,8 @@ struct ufs_hba_variant_ops { >> unsigned long *ocqs); >> int (*config_esi)(struct ufs_hba *hba); >> void (*config_scsi_dev)(struct scsi_device *sdev); >> + int (*freq_to_gear_speed)(struct ufs_hba *hba, unsigned long freq, >> + u32 *gear); >> }; > > Please keep the indentation consistent. Sure, thanks~ > > Thanks, > > Bart. >
On 1/23/2025 2:30 AM, Bart Van Assche wrote: > On 1/22/25 2:02 AM, Ziqi Chen wrote: >> +static inline int ufshcd_vops_freq_to_gear_speed(struct ufs_hba *hba, >> + unsigned long freq, >> + u32 *gear) >> +{ >> + if (hba->vops && hba->vops->freq_to_gear_speed) >> + return hba->vops->freq_to_gear_speed(hba, freq, gear); >> + >> + return -EOPNOTSUPP; >> +} > > Please remove "vops_" from the function name. I don't think this part of > the function name is useful. Additionally, please return the gear value > as the function result and remove the "u32 *gear" argument. > > Thanks, > > Bart. Hi Bart, Thanks for your review~ In ufshcd-priv.h , the function name of all vop wrapping APIs have the same prefix "ufshcd_vops", I need to use the same format as them. As for return the gear value as the function result. In our original design, we also return gear result for this function, but finally we want to use return value to indicate the status , e.g,, if vendor doesn't implement this vop, we return -EOPNOTSUPP , if there is no matched gear to the freq , we return -EINVAL. Although we didn't check the return value in this series, we still want to preserve this extensibility in case this function be used to other where in the future. -Ziqi
On 1/22/25 11:40 PM, Ziqi Chen wrote: > In ufshcd-priv.h , the function name of all vop wrapping APIs have the > same prefix "ufshcd_vops", I need to use the same format as them. That sounds fair to me. > As for return the gear value as the function result. In our original > design, we also return gear result for this function, but finally we > want to use return value to indicate the status , e.g,, if vendor > doesn't implement this vop, we return -EOPNOTSUPP , if there is no > matched gear to the freq , we return -EINVAL. Although we didn't check > the return value in this series, we still want to preserve this > extensibility in case this function be used to other where in the future. There are many functions in the Linux kernel that either return a negative error code or a positive value in case of success. Regarding future extensibility, we can't know how this function will evolve in the future. This is not an argument to keep the approach of separate error codes (return value) and gear values (gear argument). Thanks, Bart.
On 1/24/2025 1:49 AM, Bart Van Assche wrote: > On 1/22/25 11:40 PM, Ziqi Chen wrote: >> In ufshcd-priv.h , the function name of all vop wrapping APIs have the >> same prefix "ufshcd_vops", I need to use the same format as them. > > That sounds fair to me. > >> As for return the gear value as the function result. In our original >> design, we also return gear result for this function, but finally we >> want to use return value to indicate the status , e.g,, if vendor >> doesn't implement this vop, we return -EOPNOTSUPP , if there is no >> matched gear to the freq , we return -EINVAL. Although we didn't check >> the return value in this series, we still want to preserve this >> extensibility in case this function be used to other where in the future. > > There are many functions in the Linux kernel that either return a > negative error code or a positive value in case of success. Regarding > future extensibility, we can't know how this function will evolve in the > future. This is not an argument to keep the approach of separate error > codes (return value) and gear values (gear argument). > > Thanks, > > Bart. > OK , Bart, let me improve it and please review again in my next version. -Ziqi
diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h index 0549b65f71ed..a8f05fc6e830 100644 --- a/drivers/ufs/core/ufshcd-priv.h +++ b/drivers/ufs/core/ufshcd-priv.h @@ -277,6 +277,16 @@ static inline int ufshcd_mcq_vops_config_esi(struct ufs_hba *hba) return -EOPNOTSUPP; } +static inline int ufshcd_vops_freq_to_gear_speed(struct ufs_hba *hba, + unsigned long freq, + u32 *gear) +{ + if (hba->vops && hba->vops->freq_to_gear_speed) + return hba->vops->freq_to_gear_speed(hba, freq, gear); + + return -EOPNOTSUPP; +} + extern const struct ufs_pm_lvl_states ufs_pm_lvl_states[]; /** diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h index a4dac897a169..8c7c497d63d3 100644 --- a/include/ufs/ufshcd.h +++ b/include/ufs/ufshcd.h @@ -336,6 +336,7 @@ struct ufs_pwr_mode_info { * @get_outstanding_cqs: called to get outstanding completion queues * @config_esi: called to config Event Specific Interrupt * @config_scsi_dev: called to configure SCSI device parameters + * @freq_to_gear_speed: called to map clock frequency to the max supported gear speed */ struct ufs_hba_variant_ops { const char *name; @@ -387,6 +388,8 @@ struct ufs_hba_variant_ops { unsigned long *ocqs); int (*config_esi)(struct ufs_hba *hba); void (*config_scsi_dev)(struct scsi_device *sdev); + int (*freq_to_gear_speed)(struct ufs_hba *hba, unsigned long freq, + u32 *gear); }; /* clock gating state */