* [PATCH v1 2/3] scsi: ufshcd: Let vendor override devfreq parameters
2020-03-25 0:07 [PATCH v1 1/3] scsi: ufshcd: Update the set frequency to devfreq Asutosh Das
@ 2020-03-25 0:07 ` Asutosh Das
2020-03-25 13:11 ` Avri Altman
2020-03-25 0:07 ` [PATCH v1 3/3] scsi: ufs-qcom: Override " Asutosh Das
2020-03-25 13:11 ` [PATCH v1 1/3] scsi: ufshcd: Update the set frequency to devfreq Avri Altman
2 siblings, 1 reply; 7+ messages in thread
From: Asutosh Das @ 2020-03-25 0:07 UTC (permalink / raw)
To: cang, martin.petersen, linux-scsi
Cc: Asutosh Das, linux-arm-msm, Alim Akhtar, Avri Altman,
James E.J. Bottomley, Stanley Chu, Bean Huo, Bart Van Assche,
Venkat Gopalakrishnan, Tomas Winkler, Bjorn Andersson, open list
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.
^ permalink raw reply related [flat|nested] 7+ messages in thread* RE: [PATCH v1 2/3] scsi: ufshcd: Let vendor override devfreq parameters
2020-03-25 0:07 ` [PATCH v1 2/3] scsi: ufshcd: Let vendor override devfreq parameters Asutosh Das
@ 2020-03-25 13:11 ` Avri Altman
0 siblings, 0 replies; 7+ messages in thread
From: Avri Altman @ 2020-03-25 13:11 UTC (permalink / raw)
To: Asutosh Das, cang, martin.petersen, linux-scsi
Cc: linux-arm-msm, Alim Akhtar, James E.J. Bottomley, Stanley Chu,
Bean Huo, Bart Van Assche, Venkat Gopalakrishnan, Tomas Winkler,
Bjorn Andersson, open list
>
>
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v1 3/3] scsi: ufs-qcom: Override devfreq parameters
2020-03-25 0:07 [PATCH v1 1/3] scsi: ufshcd: Update the set frequency to devfreq Asutosh Das
2020-03-25 0:07 ` [PATCH v1 2/3] scsi: ufshcd: Let vendor override devfreq parameters Asutosh Das
@ 2020-03-25 0:07 ` Asutosh Das
2020-03-25 13:11 ` Avri Altman
2020-03-25 13:11 ` [PATCH v1 1/3] scsi: ufshcd: Update the set frequency to devfreq Avri Altman
2 siblings, 1 reply; 7+ messages in thread
From: Asutosh Das @ 2020-03-25 0:07 UTC (permalink / raw)
To: cang, martin.petersen, linux-scsi
Cc: Asutosh Das, linux-arm-msm, Andy Gross, Bjorn Andersson,
Alim Akhtar, Avri Altman, James E.J. Bottomley, open list
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.
^ permalink raw reply related [flat|nested] 7+ messages in thread* RE: [PATCH v1 1/3] scsi: ufshcd: Update the set frequency to devfreq
2020-03-25 0:07 [PATCH v1 1/3] scsi: ufshcd: Update the set frequency to devfreq Asutosh Das
2020-03-25 0:07 ` [PATCH v1 2/3] scsi: ufshcd: Let vendor override devfreq parameters Asutosh Das
2020-03-25 0:07 ` [PATCH v1 3/3] scsi: ufs-qcom: Override " Asutosh Das
@ 2020-03-25 13:11 ` Avri Altman
2020-03-25 16:32 ` Asutosh Das (asd)
2 siblings, 1 reply; 7+ messages in thread
From: Avri Altman @ 2020-03-25 13:11 UTC (permalink / raw)
To: Asutosh Das, cang, martin.petersen, linux-scsi
Cc: linux-arm-msm, Alim Akhtar, James E.J. Bottomley, Stanley Chu,
Bean Huo, Bart Van Assche, Venkat Gopalakrishnan, Tomas Winkler,
open list
>
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v1 1/3] scsi: ufshcd: Update the set frequency to devfreq
2020-03-25 13:11 ` [PATCH v1 1/3] scsi: ufshcd: Update the set frequency to devfreq Avri Altman
@ 2020-03-25 16:32 ` Asutosh Das (asd)
0 siblings, 0 replies; 7+ messages in thread
From: Asutosh Das (asd) @ 2020-03-25 16:32 UTC (permalink / raw)
To: Avri Altman, cang, martin.petersen, linux-scsi
Cc: linux-arm-msm, Alim Akhtar, James E.J. Bottomley, Stanley Chu,
Bean Huo, Bart Van Assche, Venkat Gopalakrishnan, Tomas Winkler,
open list
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
^ permalink raw reply [flat|nested] 7+ messages in thread