| Message ID | 20180424002016.9205-2-bjorn.andersson@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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 <bjorn.andersson@linaro.org> > --- > > 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. > > /** >
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 <bjorn.andersson@linaro.org> > > --- > > > > 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
>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 <bjorn.andersson@linaro.org> >> > --- >> > >> > 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 >
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 <bjorn.andersson@linaro.org> >>> --- >>> >>> 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 > > >
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 <bjorn.andersson@linaro.org> > >>> --- > >>> > >>> 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
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 <bjorn.andersson@linaro.org> > >> > --- > >> > > >> > 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
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]; } /**
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 <bjorn.andersson@linaro.org> --- 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(-)