Re: [PATCH 1/3] PM / devfreq: Actually support providing freq_table
On Mon 23 Apr 23:09 PDT 2018, MyungJoo Ham 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().
> >
> >> 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.
>
> Could you please check if the bug suffering you gets resolved by
> replacing 0 with ULONG_MAX in the function find_available_max_freq?
>
> -max_freq = 0;
> +max_freq = ULONG_MAX;
>
> Even if you are not using OPP, these functions should provide somewhat
> "compatible" values.
>
I also need to make set_freq_table() handle the case where there is no
opp table and change a min_freq of 0 from being treated as an error.
With this I think we're back at supporting using devfreq without
specifying any available frequencies. I am however uncertain if this
should be considered valid use of devfreq.
Regards,
Bjorn
Re: [PATCH 1/3] PM / devfreq: Actually support providing freq_table
On Tue 24 Apr 00:26 PDT 2018, Chanwoo Choi wrote:
> 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.
>
Thanks for the clarification, I see some possibilities for improving
this but it makes sense.
> And, devfreq-cooling device uses the freq_table directly because
> released MALi driver from ARM initializes the freq_table list
> directly.
>
Forgive me if I misunderstand this, but isn't this exactly what I'm
trying to do in patch 3? Which stopped working back in v4.15-rc1, with
the introduction of f1d981eaecf8 ("PM / devfreq: Use the available
min/max frequency").
> I have no any objection for refactoring. Just I'm sharing the issue
> and current status.
>
Thanks for sharing the current status and helping me understand how to
properly use devfreq.
Regards,
Bjorn
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: Re: [PATCH 1/3] PM / devfreq: Actually support providing freq_table
>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().
>
>> 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.
Could you please check if the bug suffering you gets resolved by
replacing 0 with ULONG_MAX in the function find_available_max_freq?
-max_freq = 0;
+max_freq = ULONG_MAX;
Even if you are not using OPP, these functions should provide somewhat
"compatible" values.
Cheers,
MyungJoo
>
>Thanks,
>Bjorn
>
Re: [PATCH 1/3] PM / devfreq: Actually support providing freq_table
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().
> 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.
Thanks,
Bjorn
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 1/3] PM / devfreq: Actually support providing freq_table
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];
}
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];
}
/**
--
2.16.2
