[PATCH v1 1/3] scsi: ufshcd: Update the set frequency to devfreq

[PATCH v1 1/3] scsi: ufshcd: Update the set frequency to devfreq

[Asutosh Das]

Currently, the frequency that devfreq provides the
driver to set always leads the clocks to be scaled up.
Hence, round the clock-rate to the nearest frequency
before deciding to scale.

Also update the devfreq statistics of current frequency.

Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 2a2a63b..4607bc6 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1187,6 +1187,9 @@ static int ufshcd_devfreq_target(struct device *dev,
 	if (!ufshcd_is_clkscaling_supported(hba))
 		return -EINVAL;
 
+	clki = list_first_entry(&hba->clk_list_head, struct ufs_clk_info, list);
+	/* Override with the closest supported frequency */
+	*freq = (unsigned long) clk_round_rate(clki->clk, *freq);
 	spin_lock_irqsave(hba->host->host_lock, irq_flags);
 	if (ufshcd_eh_in_progress(hba)) {
 		spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
@@ -1201,8 +1204,13 @@ static int ufshcd_devfreq_target(struct device *dev,
 		goto out;
 	}
 
-	clki = list_first_entry(&hba->clk_list_head, struct ufs_clk_info, list);
+	/* Decide based on the rounded-off frequency and update */
 	scale_up = (*freq == clki->max_freq) ? true : false;
+	if (scale_up)
+		*freq = clki->max_freq;
+	else
+		*freq = clki->min_freq;
+	/* Update the frequency */
 	if (!ufshcd_is_devfreq_scaling_required(hba, scale_up)) {
 		spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
 		ret = 0;
@@ -1250,6 +1258,8 @@ static int ufshcd_devfreq_get_dev_status(struct device *dev,
 	struct ufs_hba *hba = dev_get_drvdata(dev);
 	struct ufs_clk_scaling *scaling = &hba->clk_scaling;
 	unsigned long flags;
+	struct list_head *clk_list = &hba->clk_list_head;
+	struct ufs_clk_info *clki;
 
 	if (!ufshcd_is_clkscaling_supported(hba))
 		return -EINVAL;
@@ -1260,6 +1270,8 @@ static int ufshcd_devfreq_get_dev_status(struct device *dev,
 	if (!scaling->window_start_t)
 		goto start_window;
 
+	clki = list_first_entry(clk_list, struct ufs_clk_info, list);
+	stat->current_frequency = clki->curr_freq;
 	if (scaling->is_busy_started)
 		scaling->tot_busy_t += ktime_to_us(ktime_sub(ktime_get(),
 					scaling->busy_start_t));
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v1 2/3] scsi: ufshcd: Let vendor override devfreq parameters

[Asutosh Das]

Vendor drivers may have a need to update the polling interval
and thresholds.
Provide a vops for vendor drivers to use.

Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 11 ++++++++++-
 drivers/scsi/ufs/ufshcd.h | 12 ++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 4607bc6..7643ef5 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1300,6 +1300,13 @@ static struct devfreq_dev_profile ufs_devfreq_profile = {
 	.get_dev_status	= ufshcd_devfreq_get_dev_status,
 };
 
+#if IS_ENABLED(CONFIG_DEVFREQ_GOV_SIMPLE_ONDEMAND)
+static struct devfreq_simple_ondemand_data ufs_ondemand_data;
+static void *gov_data = &ufs_ondemand_data;
+#else
+static void *gov_data = NULL;
+#endif
+
 static int ufshcd_devfreq_init(struct ufs_hba *hba)
 {
 	struct list_head *clk_list = &hba->clk_list_head;
@@ -1315,10 +1322,12 @@ static int ufshcd_devfreq_init(struct ufs_hba *hba)
 	dev_pm_opp_add(hba->dev, clki->min_freq, 0);
 	dev_pm_opp_add(hba->dev, clki->max_freq, 0);
 
+	ufshcd_vops_config_scaling_param(hba, &ufs_devfreq_profile,
+					 gov_data);
 	devfreq = devfreq_add_device(hba->dev,
 			&ufs_devfreq_profile,
 			DEVFREQ_GOV_SIMPLE_ONDEMAND,
-			NULL);
+			gov_data);
 	if (IS_ERR(devfreq)) {
 		ret = PTR_ERR(devfreq);
 		dev_err(hba->dev, "Unable to register with devfreq %d\n", ret);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index d45a044..e8e8ee2 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -56,6 +56,7 @@
 #include <linux/completion.h>
 #include <linux/regulator/consumer.h>
 #include <linux/bitfield.h>
+#include <linux/devfreq.h>
 #include "unipro.h"
 
 #include <asm/irq.h>
@@ -327,6 +328,9 @@ struct ufs_hba_variant_ops {
 	void	(*dbg_register_dump)(struct ufs_hba *hba);
 	int	(*phy_initialization)(struct ufs_hba *);
 	void	(*device_reset)(struct ufs_hba *hba);
+	void	(*config_scaling_param)(struct ufs_hba *hba,
+					struct devfreq_dev_profile *profile,
+					void *data);
 };
 
 /* clock gating state  */
@@ -1083,6 +1087,14 @@ static inline void ufshcd_vops_device_reset(struct ufs_hba *hba)
 	}
 }
 
+static inline void ufshcd_vops_config_scaling_param(struct ufs_hba *hba,
+						    struct devfreq_dev_profile
+						    *profile, void *data)
+{
+	if (hba->vops && hba->vops->config_scaling_param)
+		hba->vops->config_scaling_param(hba, profile, data);
+}
+
 extern struct ufs_pm_lvl_states ufs_pm_lvl_states[];
 
 /*
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v1 3/3] scsi: ufs-qcom: Override devfreq parameters

[Asutosh Das]

Override devfreq parameters for power-performance
trade-off.

Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
---
 drivers/scsi/ufs/ufs-qcom.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
index 6115ac6..0fb17e2 100644
--- a/drivers/scsi/ufs/ufs-qcom.c
+++ b/drivers/scsi/ufs/ufs-qcom.c
@@ -10,6 +10,7 @@
 #include <linux/phy/phy.h>
 #include <linux/gpio/consumer.h>
 #include <linux/reset-controller.h>
+#include <linux/devfreq.h>
 
 #include "ufshcd.h"
 #include "ufshcd-pltfrm.h"
@@ -1689,6 +1690,21 @@ static void ufs_qcom_device_reset(struct ufs_hba *hba)
 	usleep_range(10, 15);
 }
 
+static void ufs_qcom_config_scaling_param(struct ufs_hba *hba,
+					  struct devfreq_dev_profile *p,
+					  void *data)
+{
+	static struct devfreq_simple_ondemand_data *d;
+
+	if (!data)
+		return;
+
+	d = (struct devfreq_simple_ondemand_data *)data;
+	p->polling_ms = 60;
+	d->upthreshold = 70;
+	d->downdifferential = 65;
+}
+
 /**
  * struct ufs_hba_qcom_vops - UFS QCOM specific variant operations
  *
@@ -1710,6 +1726,7 @@ static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
 	.resume			= ufs_qcom_resume,
 	.dbg_register_dump	= ufs_qcom_dump_dbg_regs,
 	.device_reset		= ufs_qcom_device_reset,
+	.config_scaling_param = ufs_qcom_config_scaling_param,
 };
 
 /**
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

RE: [PATCH v1 3/3] scsi: ufs-qcom: Override devfreq parameters

[Avri Altman]

> 
> Override devfreq parameters for power-performance
> trade-off.
> 
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
Reviewed-by: Avri Altman <avri.altman@wdc.com>

RE: [PATCH v1 2/3] scsi: ufshcd: Let vendor override devfreq parameters

[Avri Altman]

> 
> 
> Vendor drivers may have a need to update the polling interval
> and thresholds.
> Provide a vops for vendor drivers to use.
> 
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 11 ++++++++++-
>  drivers/scsi/ufs/ufshcd.h | 12 ++++++++++++
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 4607bc6..7643ef5 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1300,6 +1300,13 @@ static struct devfreq_dev_profile
> ufs_devfreq_profile = {
>         .get_dev_status = ufshcd_devfreq_get_dev_status,
>  };
> 
> +#if IS_ENABLED(CONFIG_DEVFREQ_GOV_SIMPLE_ONDEMAND)
> +static struct devfreq_simple_ondemand_data ufs_ondemand_data;
Might want to set default values here?


>  #include <asm/irq.h>
> @@ -327,6 +328,9 @@ struct ufs_hba_variant_ops {
>         void    (*dbg_register_dump)(struct ufs_hba *hba);
>         int     (*phy_initialization)(struct ufs_hba *);
>         void    (*device_reset)(struct ufs_hba *hba);
> +       void    (*config_scaling_param)(struct ufs_hba *hba,
> +                                       struct devfreq_dev_profile *profile,
This is an excellent idea, because not only it allows you to edit the polling interval,
But you might want to replace the target and status handlers one day. 

> +                                       void *data);
>  };


Thanks,
Avri

RE: [PATCH v1 1/3] scsi: ufshcd: Update the set frequency to devfreq

[Avri Altman]

> 
> Currently, the frequency that devfreq provides the
> driver to set always leads the clocks to be scaled up.
> Hence, round the clock-rate to the nearest frequency
> before deciding to scale.
> 
> Also update the devfreq statistics of current frequency.
> 
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 2a2a63b..4607bc6 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1187,6 +1187,9 @@ static int ufshcd_devfreq_target(struct device
> *dev,
>         if (!ufshcd_is_clkscaling_supported(hba))
>                 return -EINVAL;
> 
> +       clki = list_first_entry(&hba->clk_list_head, struct ufs_clk_info, list);
> +       /* Override with the closest supported frequency */
> +       *freq = (unsigned long) clk_round_rate(clki->clk, *freq);
>         spin_lock_irqsave(hba->host->host_lock, irq_flags);
Please remind me what the spin lock is protecting here?

>         if (ufshcd_eh_in_progress(hba)) {
>                 spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
> @@ -1201,8 +1204,13 @@ static int ufshcd_devfreq_target(struct device
> *dev,
>                 goto out;
>         }
> 
> -       clki = list_first_entry(&hba->clk_list_head, struct ufs_clk_info, list);
> +       /* Decide based on the rounded-off frequency and update */
>         scale_up = (*freq == clki->max_freq) ? true : false;
> +       if (scale_up)
> +               *freq = clki->max_freq;
This was already established 2 lines above ?

> +       else
> +               *freq = clki->min_freq;
> +       /* Update the frequency */
>         if (!ufshcd_is_devfreq_scaling_required(hba, scale_up)) {
>                 spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
>                 ret = 0;
> @@ -1250,6 +1258,8 @@ static int ufshcd_devfreq_get_dev_status(struct
> device *dev,
>         struct ufs_hba *hba = dev_get_drvdata(dev);
>         struct ufs_clk_scaling *scaling = &hba->clk_scaling;
>         unsigned long flags;
> +       struct list_head *clk_list = &hba->clk_list_head;
> +       struct ufs_clk_info *clki;
> 
>         if (!ufshcd_is_clkscaling_supported(hba))
>                 return -EINVAL;
> @@ -1260,6 +1270,8 @@ static int ufshcd_devfreq_get_dev_status(struct
> device *dev,
>         if (!scaling->window_start_t)
>                 goto start_window;
> 
> +       clki = list_first_entry(clk_list, struct ufs_clk_info, list);
> +       stat->current_frequency = clki->curr_freq;
Is this a bug fix?
devfreq_simple_ondemand_func is trying to establish the busy period,
but also uses the frequency in its calculation - which I wasn't able to understand how.
Can you add a short comment why updating current_frequency is needed?


Thanks,
Avri

Re: [PATCH v1 1/3] scsi: ufshcd: Update the set frequency to devfreq

[Asutosh Das (asd)]

On 3/25/2020 6:11 AM, Avri Altman wrote:
>>
>> Currently, the frequency that devfreq provides the
>> driver to set always leads the clocks to be scaled up.
>> Hence, round the clock-rate to the nearest frequency
>> before deciding to scale.
>>
>> Also update the devfreq statistics of current frequency.
>>
>> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
>> ---
>>   drivers/scsi/ufs/ufshcd.c | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 2a2a63b..4607bc6 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -1187,6 +1187,9 @@ static int ufshcd_devfreq_target(struct device
>> *dev,
>>          if (!ufshcd_is_clkscaling_supported(hba))
>>                  return -EINVAL;
>>
>> +       clki = list_first_entry(&hba->clk_list_head, struct ufs_clk_info, list);
>> +       /* Override with the closest supported frequency */
>> +       *freq = (unsigned long) clk_round_rate(clki->clk, *freq);
>>          spin_lock_irqsave(hba->host->host_lock, irq_flags);
> Please remind me what the spin lock is protecting here?

Hmmm ... Nothing comes to my mind. I blamed it but it's a part of a 
bigger change.

> 
>>          if (ufshcd_eh_in_progress(hba)) {
>>                  spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
>> @@ -1201,8 +1204,13 @@ static int ufshcd_devfreq_target(struct device
>> *dev,
>>                  goto out;
>>          }
>>
>> -       clki = list_first_entry(&hba->clk_list_head, struct ufs_clk_info, list);
>> +       /* Decide based on the rounded-off frequency and update */
>>          scale_up = (*freq == clki->max_freq) ? true : false;
>> +       if (scale_up)
>> +               *freq = clki->max_freq;
> This was already established 2 lines above ?
Good point - I'll change it.

> 
>> +       else
>> +               *freq = clki->min_freq;
>> +       /* Update the frequency */
>>          if (!ufshcd_is_devfreq_scaling_required(hba, scale_up)) {
>>                  spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
>>                  ret = 0;
>> @@ -1250,6 +1258,8 @@ static int ufshcd_devfreq_get_dev_status(struct
>> device *dev,
>>          struct ufs_hba *hba = dev_get_drvdata(dev);
>>          struct ufs_clk_scaling *scaling = &hba->clk_scaling;
>>          unsigned long flags;
>> +       struct list_head *clk_list = &hba->clk_list_head;
>> +       struct ufs_clk_info *clki;
>>
>>          if (!ufshcd_is_clkscaling_supported(hba))
>>                  return -EINVAL;
>> @@ -1260,6 +1270,8 @@ static int ufshcd_devfreq_get_dev_status(struct
>> device *dev,
>>          if (!scaling->window_start_t)
>>                  goto start_window;
>>
>> +       clki = list_first_entry(clk_list, struct ufs_clk_info, list);
>> +       stat->current_frequency = clki->curr_freq;
> Is this a bug fix? > devfreq_simple_ondemand_func is trying to establish the busy period,
> but also uses the frequency in its calculation - which I wasn't able to understand how.
> Can you add a short comment why updating current_frequency is needed?
>
Sure - I'll add a comment. If stat->current_frequency is not updated, 
the governor would always ask to set the max freq because the initial 
frequency was unknown to it. Reference - devfreq_simple_ondemand_func(...)

> 
> Thanks,
> Avri
> 

Thanks,
-asd

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