| Message ID | 20180523160020.15291-2-digetx@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On 23-05-18, 19:00, Dmitry Osipenko wrote: > PLL_C is running at 600MHz which is significantly higher than the 216MHz > of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is > running on that PLL. Let's use PLL_C as intermediate clock source, making > CPU snappier a tad during of the frequency transition. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++---- > 1 file changed, 21 insertions(+), 4 deletions(-) > > diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c > index 3ad6bded6efc..4bf5ba7da40b 100644 > --- a/drivers/cpufreq/tegra20-cpufreq.c > +++ b/drivers/cpufreq/tegra20-cpufreq.c > @@ -25,12 +25,13 @@ > #include <linux/types.h> > > #define PLL_P_FREQ 216000 > +#define PLL_C_FREQ 600000 > > static struct cpufreq_frequency_table freq_table[] = { > { .frequency = 216000 }, > { .frequency = 312000 }, > { .frequency = 456000 }, > - { .frequency = 608000 }, > + { .frequency = 600000 }, > { .frequency = 760000 }, > { .frequency = 816000 }, > { .frequency = 912000 }, > @@ -44,6 +45,7 @@ struct tegra20_cpufreq { > struct clk *cpu_clk; > struct clk *pll_x_clk; > struct clk *pll_p_clk; > + struct clk *pll_c_clk; > bool pll_x_prepared; > }; > > @@ -58,7 +60,10 @@ static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy, > if (index == 0 || policy->cur == PLL_P_FREQ) > return 0; > > - return PLL_P_FREQ; > + if (index == 3 || policy->cur == PLL_C_FREQ) > + return 0; So we can choose between two different intermediate frequencies ? And I didn't like the way magic number 3 is used here. Its prone to errors and we better use a macro or something else here. Like instead of doing index == 3, what about freq_table[index].freq == PLL_C_FREQ ? Same for the previous patch as well.
On 24.05.2018 07:30, Viresh Kumar wrote: > On 23-05-18, 19:00, Dmitry Osipenko wrote: >> PLL_C is running at 600MHz which is significantly higher than the 216MHz >> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is >> running on that PLL. Let's use PLL_C as intermediate clock source, making >> CPU snappier a tad during of the frequency transition. >> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++---- >> 1 file changed, 21 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c >> index 3ad6bded6efc..4bf5ba7da40b 100644 >> --- a/drivers/cpufreq/tegra20-cpufreq.c >> +++ b/drivers/cpufreq/tegra20-cpufreq.c >> @@ -25,12 +25,13 @@ >> #include <linux/types.h> >> >> #define PLL_P_FREQ 216000 >> +#define PLL_C_FREQ 600000 >> >> static struct cpufreq_frequency_table freq_table[] = { >> { .frequency = 216000 }, >> { .frequency = 312000 }, >> { .frequency = 456000 }, >> - { .frequency = 608000 }, >> + { .frequency = 600000 }, >> { .frequency = 760000 }, >> { .frequency = 816000 }, >> { .frequency = 912000 }, >> @@ -44,6 +45,7 @@ struct tegra20_cpufreq { >> struct clk *cpu_clk; >> struct clk *pll_x_clk; >> struct clk *pll_p_clk; >> + struct clk *pll_c_clk; >> bool pll_x_prepared; >> }; >> >> @@ -58,7 +60,10 @@ static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy, >> if (index == 0 || policy->cur == PLL_P_FREQ) >> return 0; >> >> - return PLL_P_FREQ; >> + if (index == 3 || policy->cur == PLL_C_FREQ) >> + return 0; > > So we can choose between two different intermediate frequencies ? And > I didn't like the way magic number 3 is used here. Its prone to errors > and we better use a macro or something else here. > > Like instead of doing index == 3, what about freq_table[index].freq == > PLL_C_FREQ ? Same for the previous patch as well. The frequency is determined by the parent clock of CCLK (CPU clock), we can choose between different parents for the CCLK. PLL_C as PLL_P and PLL_X are among the available parents for the CCLK to choose from and there some others. I don't mind to use freq_table[index].freq, though I'd like to keep compiled assembly minimal where possible. Hence the freq_table should be made constant to tell compiler that it doesn't need to emit data fetches for the table values and could embed the constants into the code where appropriate. Could we constify the "struct cpufreq_frequency_table" within the cpufreq core? Seems nothing prevents this (I already tried to constify - there are no obstacles), unless some cpufreq driver would try to modify policy->freq_table->... within the cpufreq callback implementation.
On Thu, May 24, 2018 at 7:37 AM, Dmitry Osipenko <digetx@gmail.com> wrote: > On 24.05.2018 07:30, Viresh Kumar wrote: >> On 23-05-18, 19:00, Dmitry Osipenko wrote: >>> PLL_C is running at 600MHz which is significantly higher than the 216MHz >>> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is >>> running on that PLL. Let's use PLL_C as intermediate clock source, making >>> CPU snappier a tad during of the frequency transition. >>> >>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>> --- >>> drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++---- >>> 1 file changed, 21 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c >>> index 3ad6bded6efc..4bf5ba7da40b 100644 >>> --- a/drivers/cpufreq/tegra20-cpufreq.c >>> +++ b/drivers/cpufreq/tegra20-cpufreq.c >>> @@ -25,12 +25,13 @@ >>> #include <linux/types.h> >>> >>> #define PLL_P_FREQ 216000 >>> +#define PLL_C_FREQ 600000 >>> >>> static struct cpufreq_frequency_table freq_table[] = { >>> { .frequency = 216000 }, >>> { .frequency = 312000 }, >>> { .frequency = 456000 }, >>> - { .frequency = 608000 }, >>> + { .frequency = 600000 }, >>> { .frequency = 760000 }, >>> { .frequency = 816000 }, >>> { .frequency = 912000 }, >>> @@ -44,6 +45,7 @@ struct tegra20_cpufreq { >>> struct clk *cpu_clk; >>> struct clk *pll_x_clk; >>> struct clk *pll_p_clk; >>> + struct clk *pll_c_clk; >>> bool pll_x_prepared; >>> }; >>> >>> @@ -58,7 +60,10 @@ static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy, >>> if (index == 0 || policy->cur == PLL_P_FREQ) >>> return 0; >>> >>> - return PLL_P_FREQ; >>> + if (index == 3 || policy->cur == PLL_C_FREQ) >>> + return 0; >> >> So we can choose between two different intermediate frequencies ? And >> I didn't like the way magic number 3 is used here. Its prone to errors >> and we better use a macro or something else here. >> >> Like instead of doing index == 3, what about freq_table[index].freq == >> PLL_C_FREQ ? Same for the previous patch as well. > > The frequency is determined by the parent clock of CCLK (CPU clock), we can > choose between different parents for the CCLK. PLL_C as PLL_P and PLL_X are > among the available parents for the CCLK to choose from and there some others. > > I don't mind to use freq_table[index].freq, though I'd like to keep compiled > assembly minimal where possible. Hence the freq_table should be made constant to > tell compiler that it doesn't need to emit data fetches for the table values and > could embed the constants into the code where appropriate. > > Could we constify the "struct cpufreq_frequency_table" within the cpufreq core? > Seems nothing prevents this (I already tried to constify - there are no > obstacles), unless some cpufreq driver would try to modify > policy->freq_table->... within the cpufreq callback implementation. Some drivers generate frequency tables out of external data unavailable at compile time, like ACPI tables. But if you know it for the fact that the core doesn't modify the frequency table, you could pass a constant table from the driver to it, can't you?
On Wed, May 23, 2018 at 07:00:20PM +0300, Dmitry Osipenko wrote: > PLL_C is running at 600MHz which is significantly higher than the 216MHz > of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is > running on that PLL. Let's use PLL_C as intermediate clock source, making > CPU snappier a tad during of the frequency transition. > pll_c isn't necessarily 600Mhz when used as a source for the second display head. Peter. > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++---- > 1 file changed, 21 insertions(+), 4 deletions(-) > > diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c > index 3ad6bded6efc..4bf5ba7da40b 100644 > --- a/drivers/cpufreq/tegra20-cpufreq.c > +++ b/drivers/cpufreq/tegra20-cpufreq.c > @@ -25,12 +25,13 @@ > #include <linux/types.h> > > #define PLL_P_FREQ 216000 > +#define PLL_C_FREQ 600000 > > static struct cpufreq_frequency_table freq_table[] = { > { .frequency = 216000 }, > { .frequency = 312000 }, > { .frequency = 456000 }, > - { .frequency = 608000 }, > + { .frequency = 600000 }, > { .frequency = 760000 }, > { .frequency = 816000 }, > { .frequency = 912000 }, > @@ -44,6 +45,7 @@ struct tegra20_cpufreq { > struct clk *cpu_clk; > struct clk *pll_x_clk; > struct clk *pll_p_clk; > + struct clk *pll_c_clk; > bool pll_x_prepared; > }; > > @@ -58,7 +60,10 @@ static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy, > if (index == 0 || policy->cur == PLL_P_FREQ) > return 0; > > - return PLL_P_FREQ; > + if (index == 3 || policy->cur == PLL_C_FREQ) > + return 0; > + > + return PLL_C_FREQ; > } > > static int tegra_target_intermediate(struct cpufreq_policy *policy, > @@ -79,7 +84,7 @@ static int tegra_target_intermediate(struct cpufreq_policy *policy, > */ > clk_prepare_enable(cpufreq->pll_x_clk); > > - ret = clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_p_clk); > + ret = clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_c_clk); > if (ret) > clk_disable_unprepare(cpufreq->pll_x_clk); > else > @@ -101,6 +106,9 @@ static int tegra_target(struct cpufreq_policy *policy, unsigned int index) > if (index == 0) > return clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_p_clk); > > + if (index == 3) > + return clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_c_clk); > + > ret = clk_set_rate(cpufreq->pll_x_clk, rate * 1000); > /* Restore to earlier frequency on error, i.e. pll_x */ > if (ret) > @@ -174,6 +182,12 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev) > goto put_pll_x; > } > > + cpufreq->pll_c_clk = clk_get_sys(NULL, "pll_c"); > + if (IS_ERR(cpufreq->pll_c_clk)) { > + err = PTR_ERR(cpufreq->pll_c_clk); > + goto put_pll_p; > + } > + > cpufreq->dev = &pdev->dev; > cpufreq->driver.get = cpufreq_generic_get; > cpufreq->driver.attr = cpufreq_generic_attr; > @@ -190,12 +204,14 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev) > > err = cpufreq_register_driver(&cpufreq->driver); > if (err) > - goto put_pll_p; > + goto put_pll_c; > > platform_set_drvdata(pdev, cpufreq); > > return 0; > > +put_pll_c: > + clk_put(cpufreq->pll_c_clk); > put_pll_p: > clk_put(cpufreq->pll_p_clk); > put_pll_x: > @@ -212,6 +228,7 @@ static int tegra20_cpufreq_remove(struct platform_device *pdev) > > cpufreq_unregister_driver(&cpufreq->driver); > > + clk_put(cpufreq->pll_c_clk); > clk_put(cpufreq->pll_p_clk); > clk_put(cpufreq->pll_x_clk); > clk_put(cpufreq->cpu_clk); > -- > 2.17.0 >
On 24.05.2018 11:01, Rafael J. Wysocki wrote: > On Thu, May 24, 2018 at 7:37 AM, Dmitry Osipenko <digetx@gmail.com> wrote: >> On 24.05.2018 07:30, Viresh Kumar wrote: >>> On 23-05-18, 19:00, Dmitry Osipenko wrote: >>>> PLL_C is running at 600MHz which is significantly higher than the 216MHz >>>> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is >>>> running on that PLL. Let's use PLL_C as intermediate clock source, making >>>> CPU snappier a tad during of the frequency transition. >>>> >>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>>> --- >>>> drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++---- >>>> 1 file changed, 21 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c >>>> index 3ad6bded6efc..4bf5ba7da40b 100644 >>>> --- a/drivers/cpufreq/tegra20-cpufreq.c >>>> +++ b/drivers/cpufreq/tegra20-cpufreq.c >>>> @@ -25,12 +25,13 @@ >>>> #include <linux/types.h> >>>> >>>> #define PLL_P_FREQ 216000 >>>> +#define PLL_C_FREQ 600000 >>>> >>>> static struct cpufreq_frequency_table freq_table[] = { >>>> { .frequency = 216000 }, >>>> { .frequency = 312000 }, >>>> { .frequency = 456000 }, >>>> - { .frequency = 608000 }, >>>> + { .frequency = 600000 }, >>>> { .frequency = 760000 }, >>>> { .frequency = 816000 }, >>>> { .frequency = 912000 }, >>>> @@ -44,6 +45,7 @@ struct tegra20_cpufreq { >>>> struct clk *cpu_clk; >>>> struct clk *pll_x_clk; >>>> struct clk *pll_p_clk; >>>> + struct clk *pll_c_clk; >>>> bool pll_x_prepared; >>>> }; >>>> >>>> @@ -58,7 +60,10 @@ static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy, >>>> if (index == 0 || policy->cur == PLL_P_FREQ) >>>> return 0; >>>> >>>> - return PLL_P_FREQ; >>>> + if (index == 3 || policy->cur == PLL_C_FREQ) >>>> + return 0; >>> >>> So we can choose between two different intermediate frequencies ? And >>> I didn't like the way magic number 3 is used here. Its prone to errors >>> and we better use a macro or something else here. >>> >>> Like instead of doing index == 3, what about freq_table[index].freq == >>> PLL_C_FREQ ? Same for the previous patch as well. >> >> The frequency is determined by the parent clock of CCLK (CPU clock), we can >> choose between different parents for the CCLK. PLL_C as PLL_P and PLL_X are >> among the available parents for the CCLK to choose from and there some others. >> >> I don't mind to use freq_table[index].freq, though I'd like to keep compiled >> assembly minimal where possible. Hence the freq_table should be made constant to >> tell compiler that it doesn't need to emit data fetches for the table values and >> could embed the constants into the code where appropriate. >> >> Could we constify the "struct cpufreq_frequency_table" within the cpufreq core? >> Seems nothing prevents this (I already tried to constify - there are no >> obstacles), unless some cpufreq driver would try to modify >> policy->freq_table->... within the cpufreq callback implementation. > > Some drivers generate frequency tables out of external data > unavailable at compile time, like ACPI tables. Instead of making the table constant itself (with its values), seems we can just make the policy->freq_table pointer constant. I'll try to make a patch for that, adjusting the pointers in cpufreq core and the drivers. This works for the acpi-cpufreq at least. > But if you know it for the fact that the core doesn't modify the > frequency table, you could pass a constant table from the driver to > it, can't you? > Yes, but that will require to explicitly silencing the compiler warning about const -> non-const pointer conversion (if you're meaning this pointer conversion), which generally should be avoided.
On 24.05.2018 13:04, Peter De Schrijver wrote: > On Wed, May 23, 2018 at 07:00:20PM +0300, Dmitry Osipenko wrote: >> PLL_C is running at 600MHz which is significantly higher than the 216MHz >> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is >> running on that PLL. Let's use PLL_C as intermediate clock source, making >> CPU snappier a tad during of the frequency transition. >> > > pll_c isn't necessarily 600Mhz when used as a source for the second display > head. Hmm, indeed. Even if PLL_C rate will be adjusted, it will be higher than the PLL_P.. won't it? That's likely to be good enough. Do you know if any of the available CCLK parents has a glitch-less rate switching? I.e. CPU won't hang on the rate switch. There is other possible 600MHz source, the PLL_M. Can we use it? This one also may become dynamic if we'll consider implementing the memory scaling, but the memory frequency probably will fit the transition role pretty well.
On Thu, May 24, 2018 at 03:49:22PM +0300, Dmitry Osipenko wrote: > On 24.05.2018 13:04, Peter De Schrijver wrote: > > On Wed, May 23, 2018 at 07:00:20PM +0300, Dmitry Osipenko wrote: > >> PLL_C is running at 600MHz which is significantly higher than the 216MHz > >> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is > >> running on that PLL. Let's use PLL_C as intermediate clock source, making > >> CPU snappier a tad during of the frequency transition. > >> > > > > pll_c isn't necessarily 600Mhz when used as a source for the second display > > head. > > Hmm, indeed. > > Even if PLL_C rate will be adjusted, it will be higher than the PLL_P.. won't > it? That's likely to be good enough. > Yes. I think it can be always higher than pll_p, but that assumes the display driver will always program the highest possible rate for pll_c for a given mode and then program the display divider to divide it down to the required rate. > Do you know if any of the available CCLK parents has a glitch-less rate > switching? I.e. CPU won't hang on the rate switch. > Tegra20 doesn't have dynamic ramp PLLs no. So you always have to switch to a backup clock source before changing the rate of pll_x. > There is other possible 600MHz source, the PLL_M. Can we use it? This one also > may become dynamic if we'll consider implementing the memory scaling, but the > memory frequency probably will fit the transition role pretty well. I think this should work, but as you mention it may very well be lower than pll_p if Tegra20 EMC scaling is re-introduced. I think that's why historically this was never done. Peter.
On Thu, May 24, 2018 at 2:28 PM, Dmitry Osipenko <digetx@gmail.com> wrote: > On 24.05.2018 11:01, Rafael J. Wysocki wrote: >> On Thu, May 24, 2018 at 7:37 AM, Dmitry Osipenko <digetx@gmail.com> wrote: >>> On 24.05.2018 07:30, Viresh Kumar wrote: >>>> On 23-05-18, 19:00, Dmitry Osipenko wrote: >>>>> PLL_C is running at 600MHz which is significantly higher than the 216MHz >>>>> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is >>>>> running on that PLL. Let's use PLL_C as intermediate clock source, making >>>>> CPU snappier a tad during of the frequency transition. >>>>> >>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>>>> --- >>>>> drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++---- >>>>> 1 file changed, 21 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c >>>>> index 3ad6bded6efc..4bf5ba7da40b 100644 >>>>> --- a/drivers/cpufreq/tegra20-cpufreq.c >>>>> +++ b/drivers/cpufreq/tegra20-cpufreq.c >>>>> @@ -25,12 +25,13 @@ >>>>> #include <linux/types.h> >>>>> >>>>> #define PLL_P_FREQ 216000 >>>>> +#define PLL_C_FREQ 600000 >>>>> >>>>> static struct cpufreq_frequency_table freq_table[] = { >>>>> { .frequency = 216000 }, >>>>> { .frequency = 312000 }, >>>>> { .frequency = 456000 }, >>>>> - { .frequency = 608000 }, >>>>> + { .frequency = 600000 }, >>>>> { .frequency = 760000 }, >>>>> { .frequency = 816000 }, >>>>> { .frequency = 912000 }, >>>>> @@ -44,6 +45,7 @@ struct tegra20_cpufreq { >>>>> struct clk *cpu_clk; >>>>> struct clk *pll_x_clk; >>>>> struct clk *pll_p_clk; >>>>> + struct clk *pll_c_clk; >>>>> bool pll_x_prepared; >>>>> }; >>>>> >>>>> @@ -58,7 +60,10 @@ static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy, >>>>> if (index == 0 || policy->cur == PLL_P_FREQ) >>>>> return 0; >>>>> >>>>> - return PLL_P_FREQ; >>>>> + if (index == 3 || policy->cur == PLL_C_FREQ) >>>>> + return 0; >>>> >>>> So we can choose between two different intermediate frequencies ? And >>>> I didn't like the way magic number 3 is used here. Its prone to errors >>>> and we better use a macro or something else here. >>>> >>>> Like instead of doing index == 3, what about freq_table[index].freq == >>>> PLL_C_FREQ ? Same for the previous patch as well. >>> >>> The frequency is determined by the parent clock of CCLK (CPU clock), we can >>> choose between different parents for the CCLK. PLL_C as PLL_P and PLL_X are >>> among the available parents for the CCLK to choose from and there some others. >>> >>> I don't mind to use freq_table[index].freq, though I'd like to keep compiled >>> assembly minimal where possible. Hence the freq_table should be made constant to >>> tell compiler that it doesn't need to emit data fetches for the table values and >>> could embed the constants into the code where appropriate. >>> >>> Could we constify the "struct cpufreq_frequency_table" within the cpufreq core? >>> Seems nothing prevents this (I already tried to constify - there are no >>> obstacles), unless some cpufreq driver would try to modify >>> policy->freq_table->... within the cpufreq callback implementation. >> >> Some drivers generate frequency tables out of external data >> unavailable at compile time, like ACPI tables. > > Instead of making the table constant itself (with its values), seems we can just > make the policy->freq_table pointer constant. I'll try to make a patch for that, > adjusting the pointers in cpufreq core and the drivers. This works for the > acpi-cpufreq at least. Honestly, messing up with the whole subsystem in order to avoid an explicit pointer case doesn't sound right to me. > >> But if you know it for the fact that the core doesn't modify the >> frequency table, you could pass a constant table from the driver to >> it, can't you? >> > > Yes, but that will require to explicitly silencing the compiler warning about > const -> non-const pointer conversion (if you're meaning this pointer > conversion), which generally should be avoided. Why?
On 25.05.2018 09:32, Peter De Schrijver wrote: > On Thu, May 24, 2018 at 03:49:22PM +0300, Dmitry Osipenko wrote: >> On 24.05.2018 13:04, Peter De Schrijver wrote: >>> On Wed, May 23, 2018 at 07:00:20PM +0300, Dmitry Osipenko wrote: >>>> PLL_C is running at 600MHz which is significantly higher than the 216MHz >>>> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is >>>> running on that PLL. Let's use PLL_C as intermediate clock source, making >>>> CPU snappier a tad during of the frequency transition. >>>> >>> >>> pll_c isn't necessarily 600Mhz when used as a source for the second display >>> head. >> >> Hmm, indeed. >> >> Even if PLL_C rate will be adjusted, it will be higher than the PLL_P.. won't >> it? That's likely to be good enough. >> > > Yes. I think it can be always higher than pll_p, but that assumes the display > driver will always program the highest possible rate for pll_c for a given mode > and then program the display divider to divide it down to the required rate. > >> Do you know if any of the available CCLK parents has a glitch-less rate >> switching? I.e. CPU won't hang on the rate switch. >> > > Tegra20 doesn't have dynamic ramp PLLs no. So you always have to switch to a > backup clock source before changing the rate of pll_x. > >> There is other possible 600MHz source, the PLL_M. Can we use it? This one also >> may become dynamic if we'll consider implementing the memory scaling, but the >> memory frequency probably will fit the transition role pretty well. > > I think this should work, but as you mention it may very well be lower than > pll_p if Tegra20 EMC scaling is re-introduced. I think that's why historically > this was never done. > > Peter. > Okay, thank you very much for the input. Let's put on hold these patches then. Peter Geis just made a cpufreq driver for the Tegra30 and turned out it's just a dozen lines of code that we'll have to add to the tegra20-cpufreq driver to support Tegra30. The tegra20-cpufreq driver code suits very well for the Tegra30 integration, use of PLL_C for the intermediate scaling will make that integration a bit messy. I'll return to considering of the different transition-PLL variant after adding the Tegra30 support.
On Fri, May 25, 2018 at 10:14 AM, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Thu, May 24, 2018 at 2:28 PM, Dmitry Osipenko <digetx@gmail.com> wrote: >> On 24.05.2018 11:01, Rafael J. Wysocki wrote: >>> On Thu, May 24, 2018 at 7:37 AM, Dmitry Osipenko <digetx@gmail.com> wrote: >>>> On 24.05.2018 07:30, Viresh Kumar wrote: >>>>> On 23-05-18, 19:00, Dmitry Osipenko wrote: >>>>>> PLL_C is running at 600MHz which is significantly higher than the 216MHz >>>>>> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is >>>>>> running on that PLL. Let's use PLL_C as intermediate clock source, making >>>>>> CPU snappier a tad during of the frequency transition. >>>>>> >>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>>>>> --- >>>>>> drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++---- >>>>>> 1 file changed, 21 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c >>>>>> index 3ad6bded6efc..4bf5ba7da40b 100644 >>>>>> --- a/drivers/cpufreq/tegra20-cpufreq.c >>>>>> +++ b/drivers/cpufreq/tegra20-cpufreq.c >>>>>> @@ -25,12 +25,13 @@ >>>>>> #include <linux/types.h> >>>>>> >>>>>> #define PLL_P_FREQ 216000 >>>>>> +#define PLL_C_FREQ 600000 >>>>>> >>>>>> static struct cpufreq_frequency_table freq_table[] = { >>>>>> { .frequency = 216000 }, >>>>>> { .frequency = 312000 }, >>>>>> { .frequency = 456000 }, >>>>>> - { .frequency = 608000 }, >>>>>> + { .frequency = 600000 }, >>>>>> { .frequency = 760000 }, >>>>>> { .frequency = 816000 }, >>>>>> { .frequency = 912000 }, >>>>>> @@ -44,6 +45,7 @@ struct tegra20_cpufreq { >>>>>> struct clk *cpu_clk; >>>>>> struct clk *pll_x_clk; >>>>>> struct clk *pll_p_clk; >>>>>> + struct clk *pll_c_clk; >>>>>> bool pll_x_prepared; >>>>>> }; >>>>>> >>>>>> @@ -58,7 +60,10 @@ static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy, >>>>>> if (index == 0 || policy->cur == PLL_P_FREQ) >>>>>> return 0; >>>>>> >>>>>> - return PLL_P_FREQ; >>>>>> + if (index == 3 || policy->cur == PLL_C_FREQ) >>>>>> + return 0; >>>>> >>>>> So we can choose between two different intermediate frequencies ? And >>>>> I didn't like the way magic number 3 is used here. Its prone to errors >>>>> and we better use a macro or something else here. >>>>> >>>>> Like instead of doing index == 3, what about freq_table[index].freq == >>>>> PLL_C_FREQ ? Same for the previous patch as well. >>>> >>>> The frequency is determined by the parent clock of CCLK (CPU clock), we can >>>> choose between different parents for the CCLK. PLL_C as PLL_P and PLL_X are >>>> among the available parents for the CCLK to choose from and there some others. >>>> >>>> I don't mind to use freq_table[index].freq, though I'd like to keep compiled >>>> assembly minimal where possible. Hence the freq_table should be made constant to >>>> tell compiler that it doesn't need to emit data fetches for the table values and >>>> could embed the constants into the code where appropriate. >>>> >>>> Could we constify the "struct cpufreq_frequency_table" within the cpufreq core? >>>> Seems nothing prevents this (I already tried to constify - there are no >>>> obstacles), unless some cpufreq driver would try to modify >>>> policy->freq_table->... within the cpufreq callback implementation. >>> >>> Some drivers generate frequency tables out of external data >>> unavailable at compile time, like ACPI tables. >> >> Instead of making the table constant itself (with its values), seems we can just >> make the policy->freq_table pointer constant. I'll try to make a patch for that, >> adjusting the pointers in cpufreq core and the drivers. This works for the >> acpi-cpufreq at least. > > Honestly, messing up with the whole subsystem in order to avoid an > explicit pointer case doesn't sound right to me. Actually, on a second thought I agree that it is better to do it as you suggested: make the policy->freq_table pointer constant everywhere. Sorry for the noise.
On 25.05.2018 11:36, Rafael J. Wysocki wrote: > On Fri, May 25, 2018 at 10:14 AM, Rafael J. Wysocki <rafael@kernel.org> wrote: >> On Thu, May 24, 2018 at 2:28 PM, Dmitry Osipenko <digetx@gmail.com> wrote: >>> On 24.05.2018 11:01, Rafael J. Wysocki wrote: >>>> On Thu, May 24, 2018 at 7:37 AM, Dmitry Osipenko <digetx@gmail.com> wrote: >>>>> On 24.05.2018 07:30, Viresh Kumar wrote: >>>>>> On 23-05-18, 19:00, Dmitry Osipenko wrote: >>>>>>> PLL_C is running at 600MHz which is significantly higher than the 216MHz >>>>>>> of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is >>>>>>> running on that PLL. Let's use PLL_C as intermediate clock source, making >>>>>>> CPU snappier a tad during of the frequency transition. >>>>>>> >>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>>>>>> --- >>>>>>> drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++---- >>>>>>> 1 file changed, 21 insertions(+), 4 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c >>>>>>> index 3ad6bded6efc..4bf5ba7da40b 100644 >>>>>>> --- a/drivers/cpufreq/tegra20-cpufreq.c >>>>>>> +++ b/drivers/cpufreq/tegra20-cpufreq.c >>>>>>> @@ -25,12 +25,13 @@ >>>>>>> #include <linux/types.h> >>>>>>> >>>>>>> #define PLL_P_FREQ 216000 >>>>>>> +#define PLL_C_FREQ 600000 >>>>>>> >>>>>>> static struct cpufreq_frequency_table freq_table[] = { >>>>>>> { .frequency = 216000 }, >>>>>>> { .frequency = 312000 }, >>>>>>> { .frequency = 456000 }, >>>>>>> - { .frequency = 608000 }, >>>>>>> + { .frequency = 600000 }, >>>>>>> { .frequency = 760000 }, >>>>>>> { .frequency = 816000 }, >>>>>>> { .frequency = 912000 }, >>>>>>> @@ -44,6 +45,7 @@ struct tegra20_cpufreq { >>>>>>> struct clk *cpu_clk; >>>>>>> struct clk *pll_x_clk; >>>>>>> struct clk *pll_p_clk; >>>>>>> + struct clk *pll_c_clk; >>>>>>> bool pll_x_prepared; >>>>>>> }; >>>>>>> >>>>>>> @@ -58,7 +60,10 @@ static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy, >>>>>>> if (index == 0 || policy->cur == PLL_P_FREQ) >>>>>>> return 0; >>>>>>> >>>>>>> - return PLL_P_FREQ; >>>>>>> + if (index == 3 || policy->cur == PLL_C_FREQ) >>>>>>> + return 0; >>>>>> >>>>>> So we can choose between two different intermediate frequencies ? And >>>>>> I didn't like the way magic number 3 is used here. Its prone to errors >>>>>> and we better use a macro or something else here. >>>>>> >>>>>> Like instead of doing index == 3, what about freq_table[index].freq == >>>>>> PLL_C_FREQ ? Same for the previous patch as well. >>>>> >>>>> The frequency is determined by the parent clock of CCLK (CPU clock), we can >>>>> choose between different parents for the CCLK. PLL_C as PLL_P and PLL_X are >>>>> among the available parents for the CCLK to choose from and there some others. >>>>> >>>>> I don't mind to use freq_table[index].freq, though I'd like to keep compiled >>>>> assembly minimal where possible. Hence the freq_table should be made constant to >>>>> tell compiler that it doesn't need to emit data fetches for the table values and >>>>> could embed the constants into the code where appropriate. >>>>> >>>>> Could we constify the "struct cpufreq_frequency_table" within the cpufreq core? >>>>> Seems nothing prevents this (I already tried to constify - there are no >>>>> obstacles), unless some cpufreq driver would try to modify >>>>> policy->freq_table->... within the cpufreq callback implementation. >>>> >>>> Some drivers generate frequency tables out of external data >>>> unavailable at compile time, like ACPI tables. >>> >>> Instead of making the table constant itself (with its values), seems we can just >>> make the policy->freq_table pointer constant. I'll try to make a patch for that, >>> adjusting the pointers in cpufreq core and the drivers. This works for the >>> acpi-cpufreq at least. >> >> Honestly, messing up with the whole subsystem in order to avoid an >> explicit pointer case doesn't sound right to me. > > Actually, on a second thought I agree that it is better to do it as > you suggested: make the policy->freq_table pointer constant > everywhere. > > Sorry for the noise. No worries. As I wrote in the reply to the other patch, the Tegra30 support is now on the way. These changes will collide a tad with the support integration, so I'll return to re-considering the changes made in this patchset after Tegra30 support will land. Thank you very much for your reviews and suggestions, I'll take them into account in the next iteration.
diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c index 3ad6bded6efc..4bf5ba7da40b 100644 --- a/drivers/cpufreq/tegra20-cpufreq.c +++ b/drivers/cpufreq/tegra20-cpufreq.c @@ -25,12 +25,13 @@ #include <linux/types.h> #define PLL_P_FREQ 216000 +#define PLL_C_FREQ 600000 static struct cpufreq_frequency_table freq_table[] = { { .frequency = 216000 }, { .frequency = 312000 }, { .frequency = 456000 }, - { .frequency = 608000 }, + { .frequency = 600000 }, { .frequency = 760000 }, { .frequency = 816000 }, { .frequency = 912000 }, @@ -44,6 +45,7 @@ struct tegra20_cpufreq { struct clk *cpu_clk; struct clk *pll_x_clk; struct clk *pll_p_clk; + struct clk *pll_c_clk; bool pll_x_prepared; }; @@ -58,7 +60,10 @@ static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy, if (index == 0 || policy->cur == PLL_P_FREQ) return 0; - return PLL_P_FREQ; + if (index == 3 || policy->cur == PLL_C_FREQ) + return 0; + + return PLL_C_FREQ; } static int tegra_target_intermediate(struct cpufreq_policy *policy, @@ -79,7 +84,7 @@ static int tegra_target_intermediate(struct cpufreq_policy *policy, */ clk_prepare_enable(cpufreq->pll_x_clk); - ret = clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_p_clk); + ret = clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_c_clk); if (ret) clk_disable_unprepare(cpufreq->pll_x_clk); else @@ -101,6 +106,9 @@ static int tegra_target(struct cpufreq_policy *policy, unsigned int index) if (index == 0) return clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_p_clk); + if (index == 3) + return clk_set_parent(cpufreq->cpu_clk, cpufreq->pll_c_clk); + ret = clk_set_rate(cpufreq->pll_x_clk, rate * 1000); /* Restore to earlier frequency on error, i.e. pll_x */ if (ret) @@ -174,6 +182,12 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev) goto put_pll_x; } + cpufreq->pll_c_clk = clk_get_sys(NULL, "pll_c"); + if (IS_ERR(cpufreq->pll_c_clk)) { + err = PTR_ERR(cpufreq->pll_c_clk); + goto put_pll_p; + } + cpufreq->dev = &pdev->dev; cpufreq->driver.get = cpufreq_generic_get; cpufreq->driver.attr = cpufreq_generic_attr; @@ -190,12 +204,14 @@ static int tegra20_cpufreq_probe(struct platform_device *pdev) err = cpufreq_register_driver(&cpufreq->driver); if (err) - goto put_pll_p; + goto put_pll_c; platform_set_drvdata(pdev, cpufreq); return 0; +put_pll_c: + clk_put(cpufreq->pll_c_clk); put_pll_p: clk_put(cpufreq->pll_p_clk); put_pll_x: @@ -212,6 +228,7 @@ static int tegra20_cpufreq_remove(struct platform_device *pdev) cpufreq_unregister_driver(&cpufreq->driver); + clk_put(cpufreq->pll_c_clk); clk_put(cpufreq->pll_p_clk); clk_put(cpufreq->pll_x_clk); clk_put(cpufreq->cpu_clk);
PLL_C is running at 600MHz which is significantly higher than the 216MHz of the PLL_P and it is known that PLL_C is always-ON because AHB BUS is running on that PLL. Let's use PLL_C as intermediate clock source, making CPU snappier a tad during of the frequency transition. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/cpufreq/tegra20-cpufreq.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-)