diff mbox series

[v4,12/37] PM / devfreq: tegra20: Use MC timings for building OPP table

Message ID 20200609131404.17523-13-digetx@gmail.com
State New
Headers show
Series [v4,01/37] clk: Export clk_hw_reparent() | expand

Commit Message

Dmitry Osipenko June 9, 2020, 1:13 p.m. UTC
The clk_round_rate() won't be usable for building OPP table once
interconnect support will be added to the EMC driver because that CLK API
function limits the rounded rate based on the clk rate that is imposed by
active clk-users, and thus, the rounding won't work as expected if
interconnect will set the minimum EMC clock rate before devfreq driver is
loaded. The struct tegra_mc contains memory timings which could be used by
the devfreq driver for building up OPP table instead of rounding clock
rate, this patch implements this idea.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/devfreq/tegra20-devfreq.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

Comments

Chanwoo Choi July 2, 2020, 4:18 a.m. UTC | #1
Hi Dmitry,

On 6/9/20 10:13 PM, Dmitry Osipenko wrote:
> The clk_round_rate() won't be usable for building OPP table once
> interconnect support will be added to the EMC driver because that CLK API
> function limits the rounded rate based on the clk rate that is imposed by
> active clk-users, and thus, the rounding won't work as expected if
> interconnect will set the minimum EMC clock rate before devfreq driver is
> loaded. The struct tegra_mc contains memory timings which could be used by
> the devfreq driver for building up OPP table instead of rounding clock
> rate, this patch implements this idea.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/devfreq/tegra20-devfreq.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/devfreq/tegra20-devfreq.c b/drivers/devfreq/tegra20-devfreq.c
> index 6469dc69c5e0..bf504ca4dea2 100644
> --- a/drivers/devfreq/tegra20-devfreq.c
> +++ b/drivers/devfreq/tegra20-devfreq.c
> @@ -123,8 +123,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  {
>  	struct tegra_devfreq *tegra;
>  	struct tegra_mc *mc;
> -	unsigned long max_rate;
> -	unsigned long rate;
> +	unsigned int i;
>  	int err;
>  
>  	mc = tegra_get_memory_controller();
> @@ -151,12 +150,17 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>  
>  	tegra->regs = mc->regs;
>  
> -	max_rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
> -
> -	for (rate = 0; rate <= max_rate; rate++) {
> -		rate = clk_round_rate(tegra->emc_clock, rate);
> +	if (!mc->num_timings) {

Could you explain what is meaning of 'num_timing?
Also, why add the opp entry in case of mc->num_timings is zero?

> +		err = dev_pm_opp_add(&pdev->dev,
> +				     clk_get_rate(tegra->emc_clock), 0);
> +		if (err) {
> +			dev_err(&pdev->dev, "failed to add OPP: %d\n", err);
> +			return err;
> +		}
> +	}
>  
> -		err = dev_pm_opp_add(&pdev->dev, rate, 0);
> +	for (i = 0; i < mc->num_timings; i++) {
> +		err = dev_pm_opp_add(&pdev->dev, mc->timings[i].rate, 0);
>  		if (err) {
>  			dev_err(&pdev->dev, "failed to add opp: %d\n", err);
>  			goto remove_opps;
>
Dmitry Osipenko July 2, 2020, 5:07 a.m. UTC | #2
02.07.2020 07:18, Chanwoo Choi пишет:
> Hi Dmitry,
> 
> On 6/9/20 10:13 PM, Dmitry Osipenko wrote:
>> The clk_round_rate() won't be usable for building OPP table once
>> interconnect support will be added to the EMC driver because that CLK API
>> function limits the rounded rate based on the clk rate that is imposed by
>> active clk-users, and thus, the rounding won't work as expected if
>> interconnect will set the minimum EMC clock rate before devfreq driver is
>> loaded. The struct tegra_mc contains memory timings which could be used by
>> the devfreq driver for building up OPP table instead of rounding clock
>> rate, this patch implements this idea.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/devfreq/tegra20-devfreq.c | 18 +++++++++++-------
>>  1 file changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/devfreq/tegra20-devfreq.c b/drivers/devfreq/tegra20-devfreq.c
>> index 6469dc69c5e0..bf504ca4dea2 100644
>> --- a/drivers/devfreq/tegra20-devfreq.c
>> +++ b/drivers/devfreq/tegra20-devfreq.c
>> @@ -123,8 +123,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>>  {
>>  	struct tegra_devfreq *tegra;
>>  	struct tegra_mc *mc;
>> -	unsigned long max_rate;
>> -	unsigned long rate;
>> +	unsigned int i;
>>  	int err;
>>  
>>  	mc = tegra_get_memory_controller();
>> @@ -151,12 +150,17 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>>  
>>  	tegra->regs = mc->regs;
>>  
>> -	max_rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
>> -
>> -	for (rate = 0; rate <= max_rate; rate++) {
>> -		rate = clk_round_rate(tegra->emc_clock, rate);
>> +	if (!mc->num_timings) {
> 
> Could you explain what is meaning of 'num_timing?

The num_timings is the number of memory timings defined in a
device-tree. One timing configuration per memory clock rate.

> Also, why add the opp entry in case of mc->num_timings is zero?

Timings may be not defined in some device-trees at all and in this case
memory always running on a fixed clock rate.

The devfreq driver won't be practically useful if mc->num_timings is
zero since memory frequency can't be changed, but anyways we'd want to
load the devfreq driver in order to prevent confusion about why it's not
loaded.

For example, you may ask somebody to show contents of
/sys/class/devfreq/tegra20-devfreq/trans_stat and the person says to you
that this file doesn't exist, now you'll have to figure out what
happened to the devfreq driver.
Chanwoo Choi July 2, 2020, 5:30 a.m. UTC | #3
On 7/2/20 2:07 PM, Dmitry Osipenko wrote:
> 02.07.2020 07:18, Chanwoo Choi пишет:
>> Hi Dmitry,
>>
>> On 6/9/20 10:13 PM, Dmitry Osipenko wrote:
>>> The clk_round_rate() won't be usable for building OPP table once
>>> interconnect support will be added to the EMC driver because that CLK API
>>> function limits the rounded rate based on the clk rate that is imposed by
>>> active clk-users, and thus, the rounding won't work as expected if
>>> interconnect will set the minimum EMC clock rate before devfreq driver is
>>> loaded. The struct tegra_mc contains memory timings which could be used by
>>> the devfreq driver for building up OPP table instead of rounding clock
>>> rate, this patch implements this idea.
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>> ---
>>>  drivers/devfreq/tegra20-devfreq.c | 18 +++++++++++-------
>>>  1 file changed, 11 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/tegra20-devfreq.c b/drivers/devfreq/tegra20-devfreq.c
>>> index 6469dc69c5e0..bf504ca4dea2 100644
>>> --- a/drivers/devfreq/tegra20-devfreq.c
>>> +++ b/drivers/devfreq/tegra20-devfreq.c
>>> @@ -123,8 +123,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>>>  {
>>>  	struct tegra_devfreq *tegra;
>>>  	struct tegra_mc *mc;
>>> -	unsigned long max_rate;
>>> -	unsigned long rate;
>>> +	unsigned int i;
>>>  	int err;
>>>  
>>>  	mc = tegra_get_memory_controller();
>>> @@ -151,12 +150,17 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>>>  
>>>  	tegra->regs = mc->regs;
>>>  
>>> -	max_rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
>>> -
>>> -	for (rate = 0; rate <= max_rate; rate++) {
>>> -		rate = clk_round_rate(tegra->emc_clock, rate);
>>> +	if (!mc->num_timings) {
>>
>> Could you explain what is meaning of 'num_timing?
> 
> The num_timings is the number of memory timings defined in a
> device-tree. One timing configuration per memory clock rate.

OK. I understand.

> 
>> Also, why add the opp entry in case of mc->num_timings is zero?
> 
> Timings may be not defined in some device-trees at all and in this case
> memory always running on a fixed clock rate.

You mean that 'timings' information is optional?

> 
> The devfreq driver won't be practically useful if mc->num_timings is
> zero since memory frequency can't be changed, but anyways we'd want to
> load the devfreq driver in order to prevent confusion about why it's not
> loaded.
> 
> For example, you may ask somebody to show contents of
> /sys/class/devfreq/tegra20-devfreq/trans_stat and the person says to you
> that this file doesn't exist, now you'll have to figure out what
> happened to the devfreq driver.

I understand why add OPP entry point when timing is not defined on DT.
But, actually, I think that you better to change 'timings' info is mandatory
instead of optional. Because the devfreq driver is for DVFS
and the driver supporting DVFS have to have the frequency information
like OPP.

Or, 
If you want to keep 'timing' is optional on DT,
I recommend that you add one timing data to tegra mc driver
when DT doesn't include the any timing information
I think that is it more clear.
Dmitry Osipenko July 2, 2020, 5:43 a.m. UTC | #4
02.07.2020 08:30, Chanwoo Choi пишет:
> On 7/2/20 2:07 PM, Dmitry Osipenko wrote:
>> 02.07.2020 07:18, Chanwoo Choi пишет:
>>> Hi Dmitry,
>>>
>>> On 6/9/20 10:13 PM, Dmitry Osipenko wrote:
>>>> The clk_round_rate() won't be usable for building OPP table once
>>>> interconnect support will be added to the EMC driver because that CLK API
>>>> function limits the rounded rate based on the clk rate that is imposed by
>>>> active clk-users, and thus, the rounding won't work as expected if
>>>> interconnect will set the minimum EMC clock rate before devfreq driver is
>>>> loaded. The struct tegra_mc contains memory timings which could be used by
>>>> the devfreq driver for building up OPP table instead of rounding clock
>>>> rate, this patch implements this idea.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>> ---
>>>>  drivers/devfreq/tegra20-devfreq.c | 18 +++++++++++-------
>>>>  1 file changed, 11 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/devfreq/tegra20-devfreq.c b/drivers/devfreq/tegra20-devfreq.c
>>>> index 6469dc69c5e0..bf504ca4dea2 100644
>>>> --- a/drivers/devfreq/tegra20-devfreq.c
>>>> +++ b/drivers/devfreq/tegra20-devfreq.c
>>>> @@ -123,8 +123,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>>>>  {
>>>>  	struct tegra_devfreq *tegra;
>>>>  	struct tegra_mc *mc;
>>>> -	unsigned long max_rate;
>>>> -	unsigned long rate;
>>>> +	unsigned int i;
>>>>  	int err;
>>>>  
>>>>  	mc = tegra_get_memory_controller();
>>>> @@ -151,12 +150,17 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>>>>  
>>>>  	tegra->regs = mc->regs;
>>>>  
>>>> -	max_rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
>>>> -
>>>> -	for (rate = 0; rate <= max_rate; rate++) {
>>>> -		rate = clk_round_rate(tegra->emc_clock, rate);
>>>> +	if (!mc->num_timings) {
>>>
>>> Could you explain what is meaning of 'num_timing?
>>
>> The num_timings is the number of memory timings defined in a
>> device-tree. One timing configuration per memory clock rate.
> 
> OK. I understand.
> 
>>
>>> Also, why add the opp entry in case of mc->num_timings is zero?
>>
>> Timings may be not defined in some device-trees at all and in this case
>> memory always running on a fixed clock rate.
> 
> You mean that 'timings' information is optional?

Yes

Actually, looks like I missed to properly test this case where timings
are missing in DT and it shouldn't work with the current code. I'll fix
it in the next version.

>>
>> The devfreq driver won't be practically useful if mc->num_timings is
>> zero since memory frequency can't be changed, but anyways we'd want to
>> load the devfreq driver in order to prevent confusion about why it's not
>> loaded.
>>
>> For example, you may ask somebody to show contents of
>> /sys/class/devfreq/tegra20-devfreq/trans_stat and the person says to you
>> that this file doesn't exist, now you'll have to figure out what
>> happened to the devfreq driver.
> 
> I understand why add OPP entry point when timing is not defined on DT.
> But, actually, I think that you better to change 'timings' info is mandatory
> instead of optional. Because the devfreq driver is for DVFS
> and the driver supporting DVFS have to have the frequency information
> like OPP.
> 
> Or, 
> If you want to keep 'timing' is optional on DT,
> I recommend that you add one timing data to tegra mc driver
> when DT doesn't include the any timing information
> I think that is it more clear.

Okay, I'll move it into the MC driver in the next version.

Thank you for the review!
Dmitry Osipenko July 2, 2020, 5:53 a.m. UTC | #5
02.07.2020 08:43, Dmitry Osipenko пишет:
> 02.07.2020 08:30, Chanwoo Choi пишет:
>> On 7/2/20 2:07 PM, Dmitry Osipenko wrote:
>>> 02.07.2020 07:18, Chanwoo Choi пишет:
>>>> Hi Dmitry,
>>>>
>>>> On 6/9/20 10:13 PM, Dmitry Osipenko wrote:
>>>>> The clk_round_rate() won't be usable for building OPP table once
>>>>> interconnect support will be added to the EMC driver because that CLK API
>>>>> function limits the rounded rate based on the clk rate that is imposed by
>>>>> active clk-users, and thus, the rounding won't work as expected if
>>>>> interconnect will set the minimum EMC clock rate before devfreq driver is
>>>>> loaded. The struct tegra_mc contains memory timings which could be used by
>>>>> the devfreq driver for building up OPP table instead of rounding clock
>>>>> rate, this patch implements this idea.
>>>>>
>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>> ---
>>>>>  drivers/devfreq/tegra20-devfreq.c | 18 +++++++++++-------
>>>>>  1 file changed, 11 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/devfreq/tegra20-devfreq.c b/drivers/devfreq/tegra20-devfreq.c
>>>>> index 6469dc69c5e0..bf504ca4dea2 100644
>>>>> --- a/drivers/devfreq/tegra20-devfreq.c
>>>>> +++ b/drivers/devfreq/tegra20-devfreq.c
>>>>> @@ -123,8 +123,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>>>>>  {
>>>>>  	struct tegra_devfreq *tegra;
>>>>>  	struct tegra_mc *mc;
>>>>> -	unsigned long max_rate;
>>>>> -	unsigned long rate;
>>>>> +	unsigned int i;
>>>>>  	int err;
>>>>>  
>>>>>  	mc = tegra_get_memory_controller();
>>>>> @@ -151,12 +150,17 @@ static int tegra_devfreq_probe(struct platform_device *pdev)
>>>>>  
>>>>>  	tegra->regs = mc->regs;
>>>>>  
>>>>> -	max_rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
>>>>> -
>>>>> -	for (rate = 0; rate <= max_rate; rate++) {
>>>>> -		rate = clk_round_rate(tegra->emc_clock, rate);
>>>>> +	if (!mc->num_timings) {
>>>>
>>>> Could you explain what is meaning of 'num_timing?
>>>
>>> The num_timings is the number of memory timings defined in a
>>> device-tree. One timing configuration per memory clock rate.
>>
>> OK. I understand.
>>
>>>
>>>> Also, why add the opp entry in case of mc->num_timings is zero?
>>>
>>> Timings may be not defined in some device-trees at all and in this case
>>> memory always running on a fixed clock rate.
>>
>> You mean that 'timings' information is optional?
> 
> Yes
> 
> Actually, looks like I missed to properly test this case where timings
> are missing in DT and it shouldn't work with the current code. I'll fix
> it in the next version.
> 
>>>
>>> The devfreq driver won't be practically useful if mc->num_timings is
>>> zero since memory frequency can't be changed, but anyways we'd want to
>>> load the devfreq driver in order to prevent confusion about why it's not
>>> loaded.
>>>
>>> For example, you may ask somebody to show contents of
>>> /sys/class/devfreq/tegra20-devfreq/trans_stat and the person says to you
>>> that this file doesn't exist, now you'll have to figure out what
>>> happened to the devfreq driver.
>>
>> I understand why add OPP entry point when timing is not defined on DT.
>> But, actually, I think that you better to change 'timings' info is mandatory
>> instead of optional. Because the devfreq driver is for DVFS
>> and the driver supporting DVFS have to have the frequency information
>> like OPP.

That's what I initially did by bailing out from driver's probe if
timings info is missing, until ran into a situation described above :)

>> Or, 
>> If you want to keep 'timing' is optional on DT,
>> I recommend that you add one timing data to tegra mc driver
>> when DT doesn't include the any timing information
>> I think that is it more clear.
> 
> Okay, I'll move it into the MC driver in the next version.
> 
> Thank you for the review!
>
diff mbox series

Patch

diff --git a/drivers/devfreq/tegra20-devfreq.c b/drivers/devfreq/tegra20-devfreq.c
index 6469dc69c5e0..bf504ca4dea2 100644
--- a/drivers/devfreq/tegra20-devfreq.c
+++ b/drivers/devfreq/tegra20-devfreq.c
@@ -123,8 +123,7 @@  static int tegra_devfreq_probe(struct platform_device *pdev)
 {
 	struct tegra_devfreq *tegra;
 	struct tegra_mc *mc;
-	unsigned long max_rate;
-	unsigned long rate;
+	unsigned int i;
 	int err;
 
 	mc = tegra_get_memory_controller();
@@ -151,12 +150,17 @@  static int tegra_devfreq_probe(struct platform_device *pdev)
 
 	tegra->regs = mc->regs;
 
-	max_rate = clk_round_rate(tegra->emc_clock, ULONG_MAX);
-
-	for (rate = 0; rate <= max_rate; rate++) {
-		rate = clk_round_rate(tegra->emc_clock, rate);
+	if (!mc->num_timings) {
+		err = dev_pm_opp_add(&pdev->dev,
+				     clk_get_rate(tegra->emc_clock), 0);
+		if (err) {
+			dev_err(&pdev->dev, "failed to add OPP: %d\n", err);
+			return err;
+		}
+	}
 
-		err = dev_pm_opp_add(&pdev->dev, rate, 0);
+	for (i = 0; i < mc->num_timings; i++) {
+		err = dev_pm_opp_add(&pdev->dev, mc->timings[i].rate, 0);
 		if (err) {
 			dev_err(&pdev->dev, "failed to add opp: %d\n", err);
 			goto remove_opps;