| Message ID | 1606202906-14485-2-git-send-email-cang@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Refector ufshcd_setup_clocks() to remove param skip_ref_clk | expand |
On Mon, 2020-11-23 at 23:28 -0800, Can Guo wrote: > +++ b/drivers/scsi/ufs/ufshcd.h > @@ -229,6 +229,8 @@ struct ufs_dev_cmd { > * @max_freq: maximum frequency supported by the clock > * @min_freq: min frequency that can be used for clock scaling > * @curr_freq: indicates the current frequency that it is set to > + * @always_on_while_link_active: indicate that the clk should not be > disabled if > + link is still active > * @enabled: variable to check against multiple enable/disable > */ > struct ufs_clk_info { > @@ -238,6 +240,7 @@ struct ufs_clk_info { > u32 max_freq; > u32 min_freq; > u32 curr_freq; > + bool always_on_while_link_active; Can, using a sentence as a parameter name looks a little bit clumsy to me. The meaning has been explained in the comments section. How about simplify it and in line with other parameters in the structure? Thanks, Bean > bool enabled; > }; >
On 2020-11-25 05:09, Bean Huo wrote: > On Mon, 2020-11-23 at 23:28 -0800, Can Guo wrote: >> +++ b/drivers/scsi/ufs/ufshcd.h >> @@ -229,6 +229,8 @@ struct ufs_dev_cmd { >> * @max_freq: maximum frequency supported by the clock >> * @min_freq: min frequency that can be used for clock scaling >> * @curr_freq: indicates the current frequency that it is set to >> + * @always_on_while_link_active: indicate that the clk should not be >> disabled if >> + link is still active >> * @enabled: variable to check against multiple enable/disable >> */ >> struct ufs_clk_info { >> @@ -238,6 +240,7 @@ struct ufs_clk_info { >> u32 max_freq; >> u32 min_freq; >> u32 curr_freq; >> + bool always_on_while_link_active; > > Can, > using a sentence as a parameter name looks a little bit clumsy to me. > The meaning has been explained in the comments section. How about > simplify it and in line with other parameters in the structure? > Do you have a better name in mind? Thanks, Can Guo. > Thanks, > Bean > >> bool enabled; >> }; >>
On 2020-11-25 08:53, Can Guo wrote: > On 2020-11-25 05:09, Bean Huo wrote: >> On Mon, 2020-11-23 at 23:28 -0800, Can Guo wrote: >>> +++ b/drivers/scsi/ufs/ufshcd.h >>> @@ -229,6 +229,8 @@ struct ufs_dev_cmd { >>> * @max_freq: maximum frequency supported by the clock >>> * @min_freq: min frequency that can be used for clock scaling >>> * @curr_freq: indicates the current frequency that it is set to >>> + * @always_on_while_link_active: indicate that the clk should not be >>> disabled if >>> + link is still active >>> * @enabled: variable to check against multiple enable/disable >>> */ >>> struct ufs_clk_info { >>> @@ -238,6 +240,7 @@ struct ufs_clk_info { >>> u32 max_freq; >>> u32 min_freq; >>> u32 curr_freq; >>> + bool always_on_while_link_active; >> >> Can, >> using a sentence as a parameter name looks a little bit clumsy to me. >> The meaning has been explained in the comments section. How about >> simplify it and in line with other parameters in the structure? >> > > Do you have a better name in mind? > > Thanks, > > Can Guo. > >> Thanks, >> Bean >> >>> bool enabled; >>> }; >>> Looks good to me. The variable name is not a problem. Reviewed-by: Hongwu Su<hongwus@codeaurora.org>
On Wed, 2020-11-25 at 08:53 +0800, Can Guo wrote: > > > + bool always_on_while_link_active; > > > > Can, > > using a sentence as a parameter name looks a little bit clumsy to > > me. > > The meaning has been explained in the comments section. How about > > simplify it and in line with other parameters in the structure? > > > > Do you have a better name in mind? > no specail input in mind, maybe just "bool eternal_on" > Thanks, > > Can Guo.
On 2020-11-25 19:54, Bean Huo wrote: > On Wed, 2020-11-25 at 08:53 +0800, Can Guo wrote: >> > > + bool always_on_while_link_active; >> > >> > Can, >> > using a sentence as a parameter name looks a little bit clumsy to >> > me. >> > The meaning has been explained in the comments section. How about >> > simplify it and in line with other parameters in the structure? >> > >> >> Do you have a better name in mind? >> > no specail input in mind, maybe just "bool eternal_on" It is like plain "always_on", but it cannot tell the whole story. If it is not something crutial, let's just let it go first so long as it does not break the original functionality. What do you say? Thanks, Can Guo. > >> Thanks, >> >> Can Guo.
On Wed, 2020-11-25 at 20:28 +0800, Can Guo wrote: > > On Wed, 2020-11-25 at 08:53 +0800, Can Guo wrote: > > > > > + bool always_on_while_link_active; > > > > > > > > Can, > > > > using a sentence as a parameter name looks a little bit clumsy > > > > to > > > > me. > > > > The meaning has been explained in the comments section. How > > > > about > > > > simplify it and in line with other parameters in the structure? > > > > > > > > > > Do you have a better name in mind? > > > > > > > no specail input in mind, maybe just "bool eternal_on" > > It is like plain "always_on", but it cannot tell the whole story. > If it is not something crutial, let's just let it go first so long > as it does not break the original functionality. What do you say? > > Thanks, > > Can Guo. Can, yes, it is not functional change, but always_on_while_link_active is too fat, and not non-productive way. anyway, Reviewed-by: Bean Huo <beanhuo@micron.com>
Hi Can, "Refector" in title shall be "Refactor"? On Mon, 2020-11-23 at 23:28 -0800, Can Guo wrote: > Remove the param skip_ref_clk from __ufshcd_setup_clocks(), but keep a flag > in struct ufs_clk_info to tell whether a clock can be disabled or not while > the link is active. > > Signed-off-by: Can Guo <cang@codeaurora.org> Otherwise looks good to me. Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
On 2020-11-26 08:58, Stanley Chu wrote: > Hi Can, > > "Refector" in title shall be "Refactor"? > > On Mon, 2020-11-23 at 23:28 -0800, Can Guo wrote: >> Remove the param skip_ref_clk from __ufshcd_setup_clocks(), but keep a >> flag >> in struct ufs_clk_info to tell whether a clock can be disabled or not >> while >> the link is active. >> >> Signed-off-by: Can Guo <cang@codeaurora.org> > > Otherwise looks good to me. > Sorry, will fix it in next version. Thanks, Can Guo. > Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>
On 2020-11-26 03:02, Bean Huo wrote: > On Wed, 2020-11-25 at 20:28 +0800, Can Guo wrote: >> > On Wed, 2020-11-25 at 08:53 +0800, Can Guo wrote: >> > > > > + bool always_on_while_link_active; >> > > > >> > > > Can, >> > > > using a sentence as a parameter name looks a little bit clumsy >> > > > to >> > > > me. >> > > > The meaning has been explained in the comments section. How >> > > > about >> > > > simplify it and in line with other parameters in the structure? >> > > > >> > > >> > > Do you have a better name in mind? >> > > >> > >> > no specail input in mind, maybe just "bool eternal_on" >> >> It is like plain "always_on", but it cannot tell the whole story. >> If it is not something crutial, let's just let it go first so long >> as it does not break the original functionality. What do you say? >> >> Thanks, >> >> Can Guo. > > Can, > > yes, it is not functional change, but always_on_while_link_active is > too fat, and not non-productive way. > anyway, I will change it to keep_link_active in next version. Thank you sir. Regards, Can Guo. > > Reviewed-by: Bean Huo <beanhuo@micron.com>
diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c index 3db0af6..39b1f9b 100644 --- a/drivers/scsi/ufs/ufshcd-pltfrm.c +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c @@ -92,6 +92,8 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba) clki->min_freq = clkfreq[i]; clki->max_freq = clkfreq[i+1]; clki->name = kstrdup(name, GFP_KERNEL); + if (!strcmp(name, "ref_clk")) + clki->always_on_while_link_active = true; dev_dbg(dev, "%s: min %u max %u name %s\n", "freq-table-hz", clki->min_freq, clki->max_freq, clki->name); list_add_tail(&clki->list, &hba->clk_list_head); diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index a7857f6..0802c10 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -221,8 +221,6 @@ static int ufshcd_eh_host_reset_handler(struct scsi_cmnd *cmd); static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag); static void ufshcd_hba_exit(struct ufs_hba *hba); static int ufshcd_probe_hba(struct ufs_hba *hba, bool async); -static int __ufshcd_setup_clocks(struct ufs_hba *hba, bool on, - bool skip_ref_clk); static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on); static int ufshcd_uic_hibern8_enter(struct ufs_hba *hba); static inline void ufshcd_add_delay_before_dme_cmd(struct ufs_hba *hba); @@ -1707,11 +1705,7 @@ static void ufshcd_gate_work(struct work_struct *work) ufshcd_disable_irq(hba); - if (!ufshcd_is_link_active(hba)) - ufshcd_setup_clocks(hba, false); - else - /* If link is active, device ref_clk can't be switched off */ - __ufshcd_setup_clocks(hba, false, true); + ufshcd_setup_clocks(hba, false); /* * In case you are here to cancel this work the gating state @@ -7990,8 +7984,7 @@ static int ufshcd_init_hba_vreg(struct ufs_hba *hba) return 0; } -static int __ufshcd_setup_clocks(struct ufs_hba *hba, bool on, - bool skip_ref_clk) +static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on) { int ret = 0; struct ufs_clk_info *clki; @@ -8009,7 +8002,8 @@ static int __ufshcd_setup_clocks(struct ufs_hba *hba, bool on, list_for_each_entry(clki, head, list) { if (!IS_ERR_OR_NULL(clki->clk)) { - if (skip_ref_clk && !strcmp(clki->name, "ref_clk")) + if (ufshcd_is_link_active(hba) && + clki->always_on_while_link_active) continue; clk_state_changed = on ^ clki->enabled; @@ -8054,11 +8048,6 @@ static int __ufshcd_setup_clocks(struct ufs_hba *hba, bool on, return ret; } -static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on) -{ - return __ufshcd_setup_clocks(hba, on, false); -} - static int ufshcd_init_clocks(struct ufs_hba *hba) { int ret = 0; @@ -8577,11 +8566,7 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) */ ufshcd_disable_irq(hba); - if (!ufshcd_is_link_active(hba)) - ufshcd_setup_clocks(hba, false); - else - /* If link is active, device ref_clk can't be switched off */ - __ufshcd_setup_clocks(hba, false, true); + ufshcd_setup_clocks(hba, false); if (ufshcd_is_clkgating_allowed(hba)) { hba->clk_gating.state = CLKS_OFF; diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index 66e5338..a06bdfb 100644 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@ -229,6 +229,8 @@ struct ufs_dev_cmd { * @max_freq: maximum frequency supported by the clock * @min_freq: min frequency that can be used for clock scaling * @curr_freq: indicates the current frequency that it is set to + * @always_on_while_link_active: indicate that the clk should not be disabled if + link is still active * @enabled: variable to check against multiple enable/disable */ struct ufs_clk_info { @@ -238,6 +240,7 @@ struct ufs_clk_info { u32 max_freq; u32 min_freq; u32 curr_freq; + bool always_on_while_link_active; bool enabled; };
Remove the param skip_ref_clk from __ufshcd_setup_clocks(), but keep a flag in struct ufs_clk_info to tell whether a clock can be disabled or not while the link is active. Signed-off-by: Can Guo <cang@codeaurora.org> --- drivers/scsi/ufs/ufshcd-pltfrm.c | 2 ++ drivers/scsi/ufs/ufshcd.c | 25 +++++-------------------- drivers/scsi/ufs/ufshcd.h | 3 +++ 3 files changed, 10 insertions(+), 20 deletions(-)