[PATCH v2 0/2] Refector ufshcd_setup_clocks() to remove param skip_ref_clk

[PATCH v2 0/2] Refector ufshcd_setup_clocks() to remove param skip_ref_clk

[Can Guo]

Allow vendor drivers to decide which clock should be kept on when
clk gating or suspend disables clocks while link is active.

Can Guo (2):
  scsi: ufs: Refector ufshcd_setup_clocks() to remove skip_ref_clk
  scsi: ufs-qcom: Keep core_clk_unipro ON while link is active

 drivers/scsi/ufs/ufs-qcom.c      |  6 ++++++
 drivers/scsi/ufs/ufshcd-pltfrm.c |  2 ++
 drivers/scsi/ufs/ufshcd.c        | 25 +++++--------------------
 drivers/scsi/ufs/ufshcd.h        |  3 +++
 4 files changed, 16 insertions(+), 20 deletions(-)

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 1/2] scsi: ufs: Refector ufshcd_setup_clocks() to remove skip_ref_clk

[Can Guo]

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(-)

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;
 };
 
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v2 1/2] scsi: ufs: Refector ufshcd_setup_clocks() to remove skip_ref_clk

[Bean Huo]

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;
>  };
>  

Re: [PATCH v2 1/2] scsi: ufs: Refector ufshcd_setup_clocks() to remove skip_ref_clk

[Can Guo]

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;
>>  };
>> 

Re: [PATCH v2 1/2] scsi: ufs: Refector ufshcd_setup_clocks() to remove skip_ref_clk

[hongwus]

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>

Re: [PATCH v2 1/2] scsi: ufs: Refector ufshcd_setup_clocks() to remove skip_ref_clk

[Bean Huo]

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.

Re: [PATCH v2 1/2] scsi: ufs: Refector ufshcd_setup_clocks() to remove skip_ref_clk

[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.

Re: [PATCH v2 1/2] scsi: ufs: Refector ufshcd_setup_clocks() to remove skip_ref_clk

[Bean Huo]

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>

Re: [PATCH v2 1/2] scsi: ufs: Refector ufshcd_setup_clocks() to remove skip_ref_clk

[Can Guo]

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>

Re: [PATCH v2 1/2] scsi: ufs: Refector ufshcd_setup_clocks() to remove skip_ref_clk

[Stanley Chu]

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>

Re: [PATCH v2 1/2] scsi: ufs: Refector ufshcd_setup_clocks() to remove skip_ref_clk

[Can Guo]

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>

[PATCH v2 2/2] scsi: ufs-qcom: Keep core_clk_unipro ON while link is active

[Can Guo]

If we want to disable clocks to save power but still keep the link active,
core_clk_unipro, as same as ref_clk, should not be the one being disabled.

Signed-off-by: Can Guo <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufs-qcom.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
index f9d6ef3..70df357 100644
--- a/drivers/scsi/ufs/ufs-qcom.c
+++ b/drivers/scsi/ufs/ufs-qcom.c
@@ -977,6 +977,7 @@ static int ufs_qcom_init(struct ufs_hba *hba)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct ufs_qcom_host *host;
 	struct resource *res;
+	struct ufs_clk_info *clki;
 
 	if (strlen(android_boot_dev) && strcmp(android_boot_dev, dev_name(dev)))
 		return -ENODEV;
@@ -1075,6 +1076,11 @@ static int ufs_qcom_init(struct ufs_hba *hba)
 		}
 	}
 
+	list_for_each_entry(clki, &hba->clk_list_head, list) {
+		if (!strcmp(clki->name, "core_clk_unipro"))
+			clki->always_on_while_link_active = true;
+	}
+
 	err = ufs_qcom_init_lane_clks(host);
 	if (err)
 		goto out_variant_clear;
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v2 2/2] scsi: ufs-qcom: Keep core_clk_unipro ON while link is active

[hongwus]

On 2020-11-24 15:28, Can Guo wrote:
> If we want to disable clocks to save power but still keep the link 
> active,
> core_clk_unipro, as same as ref_clk, should not be the one being 
> disabled.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufs-qcom.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> index f9d6ef3..70df357 100644
> --- a/drivers/scsi/ufs/ufs-qcom.c
> +++ b/drivers/scsi/ufs/ufs-qcom.c
> @@ -977,6 +977,7 @@ static int ufs_qcom_init(struct ufs_hba *hba)
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct ufs_qcom_host *host;
>  	struct resource *res;
> +	struct ufs_clk_info *clki;
> 
>  	if (strlen(android_boot_dev) && strcmp(android_boot_dev, 
> dev_name(dev)))
>  		return -ENODEV;
> @@ -1075,6 +1076,11 @@ static int ufs_qcom_init(struct ufs_hba *hba)
>  		}
>  	}
> 
> +	list_for_each_entry(clki, &hba->clk_list_head, list) {
> +		if (!strcmp(clki->name, "core_clk_unipro"))
> +			clki->always_on_while_link_active = true;
> +	}
> +
>  	err = ufs_qcom_init_lane_clks(host);
>  	if (err)
>  		goto out_variant_clear;

Reviewed-by: Hongwu Su<hongwus@codeaurora.org>