Re: [PATCH v2 2/2] scsi: ufs: Use freq table with devfreq
Hi,
On 2018년 05월 05일 07:44, Bjorn Andersson wrote:
> devfreq requires that the client operates on actual frequencies, not
> only 0 and UMAX_INT and as such UFS brok with the introduction of
> f1d981eaecf8 ("PM / devfreq: Use the available min/max frequency").
>
> This patch registers the frequencies of the first clock as opp levels
> and use these to determine if we're trying to step up or down.
>
> Signed-off-by: Bjorn Andersson
> ---
>
> Chances since v1:
> - Register min_freq and max_freq as opp levels.
> - Unregister opp levels on removal, to make e.g. probe defer working
>
> drivers/scsi/ufs/ufshcd.c | 47 +--
> 1 file changed, 40 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 2253f24309ec..257614b889bd 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1168,16 +1168,13 @@ static int ufshcd_devfreq_target(struct device *dev,
> struct ufs_hba *hba = dev_get_drvdata(dev);
> ktime_t start;
> bool scale_up, sched_clk_scaling_suspend_work = false;
> + struct list_head *clk_list = &hba->clk_list_head;
> + struct ufs_clk_info *clki;
> unsigned long irq_flags;
>
> if (!ufshcd_is_clkscaling_supported(hba))
> return -EINVAL;
>
> - if ((*freq > 0) && (*freq < UINT_MAX)) {
> - dev_err(hba->dev, "%s: invalid freq = %lu\n", __func__, *freq);
> - return -EINVAL;
> - }
> -
> spin_lock_irqsave(hba->host->host_lock, irq_flags);
> if (ufshcd_eh_in_progress(hba)) {
> spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
> @@ -1187,7 +1184,13 @@ static int ufshcd_devfreq_target(struct device *dev,
> if (!hba->clk_scaling.active_reqs)
> sched_clk_scaling_suspend_work = true;
>
> - scale_up = (*freq == UINT_MAX) ? true : false;
> + if (list_empty(clk_list)) {
> + spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
> + goto out;
> + }
> +
> + clki = list_first_entry(clk_list, struct ufs_clk_info, list);
> + scale_up = (*freq == clki->max_freq) ? true : false;
> if (!ufshcd_is_devfreq_scaling_required(hba, scale_up)) {
> spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
> ret = 0;
> @@ -1257,16 +1260,29 @@ static struct devfreq_dev_profile ufs_devfreq_profile
> = {
>
> static int ufshcd_devfreq_init(struct ufs_hba *hba)
> {
> + struct list_head *clk_list = &hba->clk_list_head;
> + struct ufs_clk_info *clki;
> struct devfreq *devfreq;
> int ret;
>
> - devfreq = devm_devfreq_add_device(hba->dev,
> + /* Skip devfreq if we don't have any clocks in the list */
> + if (list_empty(clk_list))
> + return 0;
> +
> + clki = list_first_entry(clk_list, struct ufs_clk_info, list);
> + dev_pm_opp_add(hba->dev, clki->min_freq, 0);
> + dev_pm_opp_add(hba->dev, clki->max_freq, 0);
> +
> + devfreq = devfreq_add_device(hba->dev,
> &ufs_devfreq_profile,
> "simple_ondemand",
> NULL);
> if (IS_ERR(devfreq)) {
> ret = PTR_ERR(devfreq);
> dev_err(hba->dev, "Unable to register with devfreq %d\n", ret);
> +
> + dev_pm_opp_remove(hba->dev, clki->min_freq);
> + dev_pm_opp_remove(hba->dev, clki->max_freq);
> return ret;
> }
>
> @@ -1275,6 +1291,22 @@ static int ufshcd_devfreq_init(struct ufs_hba *hba)
> return 0;
> }
>
> +static void ufshcd_devfreq_remove(struct ufs_hba *hba)
> +{
> + struct list_head *clk_list = &hba->clk_list_head;
> + struct ufs_clk_info *clki;
> +
> + if (!hba->devfreq)
> + return;
> +
> + devfreq_remove_device(hba->devfreq);
> + hba->devfreq = NULL;
> +
> + clki = list_first_entry(clk_list, struct ufs_clk_info, list);
> + dev_pm_opp_remove(hba->dev, clki->min_freq);
> + dev_pm_opp_remove(hba->dev, clki->max_freq);
> +}
> +
> static void __ufshcd_suspend_clkscaling(struct ufs_hba *hba)
> {
> unsigned long flags;
> @@ -6966,6 +6998,7 @@ static void ufshcd_hba_exit(struct ufs_hba *hba)
> if (hba->devfreq)
> ufshcd_suspend_clkscaling(hba);
> destroy_workqueue(hba->clk_scaling.workq);
> + ufshcd_devfreq_remove(hba);
> }
> ufshcd_setup_clocks(hba, false);
> ufshcd_setup_hba_vreg(hba, false);
>
For using OPP entries for devfreq:
Reviewed-by: Chanwoo Choi
--
Best Regards,
Chanwoo Choi
Samsung Electronics
Re: [PATCH v2 1/2] scsi: ufs: Extract devfreq registration
Hi,
On 2018년 05월 05일 07:44, Bjorn Andersson wrote:
> Failing to register with devfreq leaves hba->devfreq assigned, which
> causes the error path to dereference the ERR_PTR(). Rather than bolting
> on more conditionals, move the call of devm_devfreq_add_device() into
> it's own function and only update hba->devfreq once it's successfully
> registered.
>
> The subsequent patch builds upon this to make UFS actually work again,
> as it's been broken since f1d981eaecf8 ("PM / devfreq: Use the available
> min/max frequency")
>
> Reviewed-by: Subhash Jadavani
> Signed-off-by: Bjorn Andersson
> ---
>
> Changes since v1:
> - None
>
> drivers/scsi/ufs/ufshcd.c | 31 ++-
> 1 file changed, 22 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 8f22a980b1a7..2253f24309ec 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1255,6 +1255,26 @@ static struct devfreq_dev_profile ufs_devfreq_profile
> = {
> .get_dev_status = ufshcd_devfreq_get_dev_status,
> };
>
> +static int ufshcd_devfreq_init(struct ufs_hba *hba)
> +{
> + struct devfreq *devfreq;
> + int ret;
> +
> + devfreq = devm_devfreq_add_device(hba->dev,
> + &ufs_devfreq_profile,
> + "simple_ondemand",
You better to use 'DEVFREQ_GOV_SIMPLE_ONDEMAND' definition
in include/linux/devfreq.h in order to prevent the some typo.
- aa7c352f9841 ("PM / devfreq: Define the constant governor name")
> + NULL);
> + if (IS_ERR(devfreq)) {
> + ret = PTR_ERR(devfreq);
> + dev_err(hba->dev, "Unable to register with devfreq %d\n", ret);
> + return ret;
> + }
> +
> + hba->devfreq = devfreq;
> +
> + return 0;
> +}
> +
> static void __ufshcd_suspend_clkscaling(struct ufs_hba *hba)
> {
> unsigned long flags;
> @@ -6399,16 +6419,9 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
> sizeof(struct ufs_pa_layer_attr));
> hba->clk_scaling.saved_pwr_info.is_valid = true;
> if (!hba->devfreq) {
> - hba->devfreq = devm_devfreq_add_device(hba->dev,
> - &ufs_devfreq_profile,
> - "simple_ondemand",
> - NULL);
> - if (IS_ERR(hba->devfreq)) {
> - ret = PTR_ERR(hba->devfreq);
> - dev_err(hba->dev, "Unable to register
> with devfreq %d\n",
> - ret);
> + ret = ufshcd_devfreq_init(hba);
> + if (ret)
> goto out;
> - }
> }
> hba->clk_scaling.is_allowed = true;
> }
>
Reviewed-by: Chanwoo Choi
--
Best Regards,
Chanwoo Choi
Samsung Electronics
Re: [PATCH 1/3] PM / devfreq: Actually support providing freq_table
Hi,
On 2018년 04월 24일 14:29, Bjorn Andersson wrote:
> On Mon 23 Apr 19:48 PDT 2018, Chanwoo Choi wrote:
>
>> Hi,
>>
>> On 2018??? 04??? 24??? 09:20, Bjorn Andersson wrote:
>>> The code in devfreq_add_device() handles the case where a freq_table is
>>> passed by the client, but then requests min and max frequences from
>>> the, in this case absent, opp tables.
>>>
>>> Read the min and max frequencies from the frequency table, which has
>>> been built from the opp table if one exists, instead of querying the
>>> opp table.
>>>
>>> Signed-off-by: Bjorn Andersson
>>> ---
>>>
>>> An alternative approach is to clarify in the devfreq code that it's not
>>> possible to pass a freq_table and then in patch 3 create an opp table for
>>> the
>>> device in runtime; although the error handling of this becomes non-trivial.
>>>
>>> Transitioning the UFSHCD to use opp tables directly is hindered by the fact
>>> that the Qualcomm UFS hardware has two different clocks that needs to be
>>> running at different rates, so we would need a way to describe the two
>>> rates in
>>> the opp table. (And would force us to change the DT binding)
>>>
>>> drivers/devfreq/devfreq.c | 22 --
>>> 1 file changed, 4 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index fe2af6aa88fc..086ced50a13d 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -74,30 +74,16 @@ static struct devfreq *find_device_devfreq(struct
>>> device *dev)
>>>
>>> static unsigned long find_available_min_freq(struct devfreq *devfreq)
>>> {
>>> - struct dev_pm_opp *opp;
>>> - unsigned long min_freq = 0;
>>> -
>>> - opp = dev_pm_opp_find_freq_ceil(devfreq->dev.parent, &min_freq);
>>> - if (IS_ERR(opp))
>>> - min_freq = 0;
>>> - else
>>> - dev_pm_opp_put(opp);
>>> + struct devfreq_dev_profile *profile = devfreq->profile;
>>>
>>> - return min_freq;
>>> + return profile->freq_table[0];
>>
>> It is wrong. The thermal framework support the devfreq-cooling device
>> which uses the dev_pm_opp_enable/disable().
>>
>
> Okay, that makes sense. So rather than registering a custom freq_table I
> should register the min and max frequency using dev_pm_opp_add().
Thanks.
>
>> In order to find the correct available min frequency,
>> the devfreq have to use the OPP function instead of using the first entry
>> of the freq_table array.
>>
>
> Based on this there seems to be room for cleaning out the freq_table
> from devfreq, to reduce the confusion. I will review this further.
Actually, devfreq must need to have the freq_table[] array. But, freq_table[]
array should be handled in the devfreq core. Now, the devfreq device drivers can
touch the freq_table. I think it is not good.
There is a reason why we have to maintain the freq_table[] as the internal
variable.
OPP doesn't provide the OPP API which get the all registered frequencies.
If devfreq-cooling device disables the specific frequency by using
dev_pm_oppdisable(),
the user of OPP interface can not get the disabled frequency list.
So, I maintain the freq_table even if using the OPP interface.
And, devfreq-cooling device uses the freq_table directly because released MALi
driver
from ARM initializes the freq_table list directly.
I have no any objection for refactoring. Just I'm sharing the issue and current
status.
>
> Thanks,
> Bjorn
>
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
Re: [PATCH 1/3] PM / devfreq: Actually support providing freq_table
Hi,
On 2018년 04월 24일 09:20, Bjorn Andersson wrote:
> The code in devfreq_add_device() handles the case where a freq_table is
> passed by the client, but then requests min and max frequences from
> the, in this case absent, opp tables.
>
> Read the min and max frequencies from the frequency table, which has
> been built from the opp table if one exists, instead of querying the
> opp table.
>
> Signed-off-by: Bjorn Andersson
> ---
>
> An alternative approach is to clarify in the devfreq code that it's not
> possible to pass a freq_table and then in patch 3 create an opp table for the
> device in runtime; although the error handling of this becomes non-trivial.
>
> Transitioning the UFSHCD to use opp tables directly is hindered by the fact
> that the Qualcomm UFS hardware has two different clocks that needs to be
> running at different rates, so we would need a way to describe the two rates
> in
> the opp table. (And would force us to change the DT binding)
>
> drivers/devfreq/devfreq.c | 22 --
> 1 file changed, 4 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index fe2af6aa88fc..086ced50a13d 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -74,30 +74,16 @@ static struct devfreq *find_device_devfreq(struct device
> *dev)
>
> static unsigned long find_available_min_freq(struct devfreq *devfreq)
> {
> - struct dev_pm_opp *opp;
> - unsigned long min_freq = 0;
> -
> - opp = dev_pm_opp_find_freq_ceil(devfreq->dev.parent, &min_freq);
> - if (IS_ERR(opp))
> - min_freq = 0;
> - else
> - dev_pm_opp_put(opp);
> + struct devfreq_dev_profile *profile = devfreq->profile;
>
> - return min_freq;
> + return profile->freq_table[0];
It is wrong. The thermal framework support the devfreq-cooling device
which uses the dev_pm_opp_enable/disable().
In order to find the correct available min frequency,
the devfreq have to use the OPP function instead of using the first entry
of the freq_table array.
> }
>
> static unsigned long find_available_max_freq(struct devfreq *devfreq)
> {
> - struct dev_pm_opp *opp;
> - unsigned long max_freq = ULONG_MAX;
> -
> - opp = dev_pm_opp_find_freq_floor(devfreq->dev.parent, &max_freq);
> - if (IS_ERR(opp))
> - max_freq = 0;
> - else
> - dev_pm_opp_put(opp);
> + struct devfreq_dev_profile *profile = devfreq->profile;
>
> - return max_freq;
> + return profile->freq_table[profile->max_state - 1];
> }
ditto.
>
> /**
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
[PATCH v2 2/2] scsi: ufs: Use the resource-managed function to add devfreq device
This patch uses the resource-managed to add the devfreq device.
This function will make it easy to handle the devfreq device.
- struct devfreq *devm_devfreq_add_device(struct device *dev,
struct devfreq_dev_profile *profile,
const char *governor_name,
void *data);
Cc: Vinayak Holikatti
Cc: James E.J. Bottomley
Cc: Martin K. Petersen
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Chanwoo Choi
---
drivers/scsi/ufs/ufshcd.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index e491c4bda32f..e8c5ba274830 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6250,8 +6250,6 @@ void ufshcd_remove(struct ufs_hba *hba)
ufshcd_hba_stop(hba, true);
ufshcd_exit_clk_gating(hba);
- if (ufshcd_is_clkscaling_enabled(hba))
- devfreq_remove_device(hba->devfreq);
ufshcd_hba_exit(hba);
}
EXPORT_SYMBOL_GPL(ufshcd_remove);
@@ -6579,7 +6577,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem
*mmio_base, unsigned int irq)
}
if (ufshcd_is_clkscaling_enabled(hba)) {
- hba->devfreq = devfreq_add_device(dev, &ufs_devfreq_profile,
+ hba->devfreq = devm_devfreq_add_device(dev,
&ufs_devfreq_profile,
"simple_ondemand", NULL);
if (IS_ERR(hba->devfreq)) {
dev_err(hba->dev, "Unable to register with devfreq
%ld\n",
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/2] scsi: ufs: Use the resource-managed function to add devfreq device
This patch uses the resource-managed to add the devfreq device.
This function will make it easy to handle the devfreq device.
- struct devfreq *devm_devfreq_add_device(struct device *dev,
struct devfreq_dev_profile *profile,
const char *governor_name,
void *data);
Cc: Vinayak Holikatti
Cc: James E.J. Bottomley
Cc: Martin K. Petersen
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Chanwoo Choi
---
drivers/scsi/ufs/ufshcd.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index e491c4bda32f..e8c5ba274830 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6250,8 +6250,6 @@ void ufshcd_remove(struct ufs_hba *hba)
ufshcd_hba_stop(hba, true);
ufshcd_exit_clk_gating(hba);
- if (ufshcd_is_clkscaling_enabled(hba))
- devfreq_remove_device(hba->devfreq);
ufshcd_hba_exit(hba);
}
EXPORT_SYMBOL_GPL(ufshcd_remove);
@@ -6579,7 +6577,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem
*mmio_base, unsigned int irq)
}
if (ufshcd_is_clkscaling_enabled(hba)) {
- hba->devfreq = devfreq_add_device(dev, &ufs_devfreq_profile,
+ hba->devfreq = devm_devfreq_add_device(dev,
&ufs_devfreq_profile,
"simple_ondemand", NULL);
if (IS_ERR(hba->devfreq)) {
dev_err(hba->dev, "Unable to register with devfreq
%ld\n",
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] scsi: ufs: Use the resource-managed function to add devfreq device
This patch uses the resource-managed to add the devfreq device.
This function will make it easy to handle the devfreq device.
- struct devfreq *devm_devfreq_add_device(struct device *dev,
struct devfreq_dev_profile *profile,
const char *governor_name,
void *data);
Cc: Vinayak Holikatti
Cc: James E.J. Bottomley
Cc: Martin K. Petersen
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Chanwoo Choi
---
drivers/scsi/ufs/ufshcd.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index f08d41a2d70b..e639071fd3a1 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6198,8 +6198,6 @@ void ufshcd_remove(struct ufs_hba *hba)
scsi_host_put(hba->host);
ufshcd_exit_clk_gating(hba);
- if (ufshcd_is_clkscaling_enabled(hba))
- devfreq_remove_device(hba->devfreq);
ufshcd_hba_exit(hba);
}
EXPORT_SYMBOL_GPL(ufshcd_remove);
@@ -6495,7 +6493,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem
*mmio_base, unsigned int irq)
}
if (ufshcd_is_clkscaling_enabled(hba)) {
- hba->devfreq = devfreq_add_device(dev, &ufs_devfreq_profile,
+ hba->devfreq = devm_devfreq_add_device(dev,
&ufs_devfreq_profile,
"simple_ondemand", NULL);
if (IS_ERR(hba->devfreq)) {
dev_err(hba->dev, "Unable to register with devfreq
%ld\n",
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] scsi: ufs: Use the resource-managed function to add devfreq device
This patch uses the resource-managed to add the devfreq device.
This function will make it easy to handle the devfreq device.
- struct devfreq *devm_devfreq_add_device(struct device *dev,
struct devfreq_dev_profile *profile,
const char *governor_name,
void *data);
Cc: Vinayak Holikatti
Cc: James E.J. Bottomley
Cc: Martin K. Petersen
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Chanwoo Choi
---
drivers/scsi/ufs/ufshcd.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index f08d41a2d70b..e639071fd3a1 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6198,8 +6198,6 @@ void ufshcd_remove(struct ufs_hba *hba)
scsi_host_put(hba->host);
ufshcd_exit_clk_gating(hba);
- if (ufshcd_is_clkscaling_enabled(hba))
- devfreq_remove_device(hba->devfreq);
ufshcd_hba_exit(hba);
}
EXPORT_SYMBOL_GPL(ufshcd_remove);
@@ -6495,7 +6493,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem
*mmio_base, unsigned int irq)
}
if (ufshcd_is_clkscaling_enabled(hba)) {
- hba->devfreq = devfreq_add_device(dev, &ufs_devfreq_profile,
+ hba->devfreq = devm_devfreq_add_device(dev,
&ufs_devfreq_profile,
"simple_ondemand", NULL);
if (IS_ERR(hba->devfreq)) {
dev_err(hba->dev, "Unable to register with devfreq
%ld\n",
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
