| Message ID | 20250116091150.1167739-8-quic_ziqichen@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Support Multi-frequency scale for UFS | expand |
> > From: Can Guo <quic_cang@quicinc.com> > > During clock scaling, Write Booster is toggled on or off based on whether the > clock is scaled up or down. However, with OPP V2 powered multi-level gear > scaling, the gear can be scaled amongst multiple gear speeds, e.g., it may > scale down from G5 to G4, or from G4 to G2. To provide flexibilities, add a > new field for clock scaling such that during clock scaling Write Booster can be > enabled or disabled based on gear speeds but not based on scaling up or > down. > > 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> > --- > drivers/ufs/core/ufshcd.c | 17 ++++++++++++----- > include/ufs/ufshcd.h | 3 +++ > 2 files changed, 15 insertions(+), 5 deletions(-) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index > 721bf9d1a356..31ebf267135b 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -1395,13 +1395,17 @@ static int ufshcd_clock_scaling_prepare(struct > ufs_hba *hba, u64 timeout_us) > return ret; > } > > -static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, int err, bool > scale_up) > +static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, int > +err) > { > up_write(&hba->clk_scaling_lock); > > - /* Enable Write Booster if we have scaled up else disable it */ > - if (ufshcd_enable_wb_if_scaling_up(hba) && !err) > - ufshcd_wb_toggle(hba, scale_up); > + /* Enable Write Booster if current gear requires it else disable it */ > + if (ufshcd_enable_wb_if_scaling_up(hba) && !err) { > + bool wb_en; Can be initialized? > + > + wb_en = hba->pwr_info.gear_rx >= hba->clk_scaling.wb_gear ? true > : false; If (wb != hba->dev_info.wb_enabled) > + ufshcd_wb_toggle(hba, wb_en); > + } Wouldn't it make sense to move the wb toggling to ufshcd_scale_gear ? This way you'll be able to leave the legacy on/off toggling? > > mutex_unlock(&hba->wb_mutex); > > @@ -1456,7 +1460,7 @@ static int ufshcd_devfreq_scale(struct ufs_hba > *hba, unsigned long freq, > } > > out_unprepare: > - ufshcd_clock_scaling_unprepare(hba, ret, scale_up); > + ufshcd_clock_scaling_unprepare(hba, ret); > return ret; > } > > @@ -1816,6 +1820,9 @@ static void ufshcd_init_clk_scaling(struct ufs_hba > *hba) > if (!hba->clk_scaling.min_gear) > hba->clk_scaling.min_gear = UFS_HS_G1; > > + if (!hba->clk_scaling.wb_gear) > + hba->clk_scaling.wb_gear = UFS_HS_G3; So you will toggle wb off on init (pwm) and on sporadic writes. I guess there is no harm done. Thanks, Avri > + > INIT_WORK(&hba->clk_scaling.suspend_work, > ufshcd_clk_scaling_suspend_work); > INIT_WORK(&hba->clk_scaling.resume_work, > diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h index > 8c7c497d63d3..8e6c2eb68011 100644 > --- a/include/ufs/ufshcd.h > +++ b/include/ufs/ufshcd.h > @@ -448,6 +448,8 @@ struct ufs_clk_gating { > * @resume_work: worker to resume devfreq > * @target_freq: frequency requested by devfreq framework > * @min_gear: lowest HS gear to scale down to > + * @wb_gear: enable Write Booster when HS gear scales above or equal to > it, else > + * disable Write Booster > * @is_enabled: tracks if scaling is currently enabled or not, controlled by > * clkscale_enable sysfs node > * @is_allowed: tracks if scaling is currently allowed or not, used to block > @@ -468,6 +470,7 @@ struct ufs_clk_scaling { > struct work_struct resume_work; > unsigned long target_freq; > u32 min_gear; > + u32 wb_gear; > bool is_enabled; > bool is_allowed; > bool is_initialized; > -- > 2.34.1
Hi Avri, Thanks for your comment. On 1/16/2025 9:27 PM, Avri Altman wrote: >> >> From: Can Guo <quic_cang@quicinc.com> >> >> During clock scaling, Write Booster is toggled on or off based on whether the >> clock is scaled up or down. However, with OPP V2 powered multi-level gear >> scaling, the gear can be scaled amongst multiple gear speeds, e.g., it may >> scale down from G5 to G4, or from G4 to G2. To provide flexibilities, add a >> new field for clock scaling such that during clock scaling Write Booster can be >> enabled or disabled based on gear speeds but not based on scaling up or >> down. >> >> 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> >> --- >> drivers/ufs/core/ufshcd.c | 17 ++++++++++++----- >> include/ufs/ufshcd.h | 3 +++ >> 2 files changed, 15 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index >> 721bf9d1a356..31ebf267135b 100644 >> --- a/drivers/ufs/core/ufshcd.c >> +++ b/drivers/ufs/core/ufshcd.c >> @@ -1395,13 +1395,17 @@ static int ufshcd_clock_scaling_prepare(struct >> ufs_hba *hba, u64 timeout_us) >> return ret; >> } >> >> -static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, int err, bool >> scale_up) >> +static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, int >> +err) >> { >> up_write(&hba->clk_scaling_lock); >> >> - /* Enable Write Booster if we have scaled up else disable it */ >> - if (ufshcd_enable_wb_if_scaling_up(hba) && !err) >> - ufshcd_wb_toggle(hba, scale_up); >> + /* Enable Write Booster if current gear requires it else disable it */ >> + if (ufshcd_enable_wb_if_scaling_up(hba) && !err) { >> + bool wb_en; > Can be initialized? > In this code, the wb_en variable does not need to be explicitly initialized. This is because within the conditional statement, wb_en is assigned a value based on the comparison between hba->pwr_info.gear_rx and hba->clk_scaling.wb_gear. Therefore, regardless of the condition, wb_en will be assigned either true or false. However, it is a good practice to ensure that wb_en is correctly assigned in all possible code paths. Thanks for your suggestion, I will initialize it as "False" in next version. >> + >> + wb_en = hba->pwr_info.gear_rx >= hba->clk_scaling.wb_gear ? true >> : false; > > If (wb != hba->dev_info.wb_enabled) >> + ufshcd_wb_toggle(hba, wb_en); >> + } > Wouldn't it make sense to move the wb toggling to ufshcd_scale_gear ? > This way you'll be able to leave the legacy on/off toggling? > We don't need legacy wb on/off any more. Regarding the logic of this series, even if gear scale go to legacy branch , current wb toggle logic can also cover it. for example, for legacy gear scale , it only has 2 possible gear speed mode, scal up to max gear or scale down to G1, we choose G3 as the wb_gear toggle gear can cover legacy WB toggle. Any more , some customer may want the WB keep ON or OFF, they can set the wb_gear to G0 or max_gear to meet their requirement. >> >> mutex_unlock(&hba->wb_mutex); >> >> @@ -1456,7 +1460,7 @@ static int ufshcd_devfreq_scale(struct ufs_hba >> *hba, unsigned long freq, >> } >> >> out_unprepare: >> - ufshcd_clock_scaling_unprepare(hba, ret, scale_up); >> + ufshcd_clock_scaling_unprepare(hba, ret); >> return ret; >> } >> >> @@ -1816,6 +1820,9 @@ static void ufshcd_init_clk_scaling(struct ufs_hba >> *hba) >> if (!hba->clk_scaling.min_gear) >> hba->clk_scaling.min_gear = UFS_HS_G1; >> >> + if (!hba->clk_scaling.wb_gear) >> + hba->clk_scaling.wb_gear = UFS_HS_G3; > So you will toggle wb off on init (pwm) and on sporadic writes. > I guess there is no harm done. > No , it won't toggle wb off on init. As per UFS hba probe sequence, the timing of devfreq init is very late. The WB will be turn ON at the early init and complete whole init sequence with high gear speed mode. Not worry about WB would be turn OFF during init. > Thanks, > Avri -Ziqi > >> + >> INIT_WORK(&hba->clk_scaling.suspend_work, >> ufshcd_clk_scaling_suspend_work); >> INIT_WORK(&hba->clk_scaling.resume_work, >> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h index >> 8c7c497d63d3..8e6c2eb68011 100644 >> --- a/include/ufs/ufshcd.h >> +++ b/include/ufs/ufshcd.h >> @@ -448,6 +448,8 @@ struct ufs_clk_gating { >> * @resume_work: worker to resume devfreq >> * @target_freq: frequency requested by devfreq framework >> * @min_gear: lowest HS gear to scale down to >> + * @wb_gear: enable Write Booster when HS gear scales above or equal to >> it, else >> + * disable Write Booster >> * @is_enabled: tracks if scaling is currently enabled or not, controlled by >> * clkscale_enable sysfs node >> * @is_allowed: tracks if scaling is currently allowed or not, used to block >> @@ -468,6 +470,7 @@ struct ufs_clk_scaling { >> struct work_struct resume_work; >> unsigned long target_freq; >> u32 min_gear; >> + u32 wb_gear; >> bool is_enabled; >> bool is_allowed; >> bool is_initialized; >> -- >> 2.34.1 >
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 721bf9d1a356..31ebf267135b 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -1395,13 +1395,17 @@ static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba, u64 timeout_us) return ret; } -static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, int err, bool scale_up) +static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, int err) { up_write(&hba->clk_scaling_lock); - /* Enable Write Booster if we have scaled up else disable it */ - if (ufshcd_enable_wb_if_scaling_up(hba) && !err) - ufshcd_wb_toggle(hba, scale_up); + /* Enable Write Booster if current gear requires it else disable it */ + if (ufshcd_enable_wb_if_scaling_up(hba) && !err) { + bool wb_en; + + wb_en = hba->pwr_info.gear_rx >= hba->clk_scaling.wb_gear ? true : false; + ufshcd_wb_toggle(hba, wb_en); + } mutex_unlock(&hba->wb_mutex); @@ -1456,7 +1460,7 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, unsigned long freq, } out_unprepare: - ufshcd_clock_scaling_unprepare(hba, ret, scale_up); + ufshcd_clock_scaling_unprepare(hba, ret); return ret; } @@ -1816,6 +1820,9 @@ static void ufshcd_init_clk_scaling(struct ufs_hba *hba) if (!hba->clk_scaling.min_gear) hba->clk_scaling.min_gear = UFS_HS_G1; + if (!hba->clk_scaling.wb_gear) + hba->clk_scaling.wb_gear = UFS_HS_G3; + INIT_WORK(&hba->clk_scaling.suspend_work, ufshcd_clk_scaling_suspend_work); INIT_WORK(&hba->clk_scaling.resume_work, diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h index 8c7c497d63d3..8e6c2eb68011 100644 --- a/include/ufs/ufshcd.h +++ b/include/ufs/ufshcd.h @@ -448,6 +448,8 @@ struct ufs_clk_gating { * @resume_work: worker to resume devfreq * @target_freq: frequency requested by devfreq framework * @min_gear: lowest HS gear to scale down to + * @wb_gear: enable Write Booster when HS gear scales above or equal to it, else + * disable Write Booster * @is_enabled: tracks if scaling is currently enabled or not, controlled by * clkscale_enable sysfs node * @is_allowed: tracks if scaling is currently allowed or not, used to block @@ -468,6 +470,7 @@ struct ufs_clk_scaling { struct work_struct resume_work; unsigned long target_freq; u32 min_gear; + u32 wb_gear; bool is_enabled; bool is_allowed; bool is_initialized;