[1/2] cpufreq: exynos4x12: Use the common clock framework to set APLL clock rate

Commit Message

Lukasz Majewski Sept. 25, 2013, 11:22 a.m. UTC

In the exynos4x12_set_apll() function, the APLL frequency is set with
direct register manipulation.

Such approach is not allowed in the common clock framework. The frequency
is changed, but the corresponding clock value is not updated. This causes
wrong frequency read from cpufreq's cpuinfo_cur_freq sysfs attribute.

Tested at:
- Exynos4412 - Trats2 board (linux 3.12-rc1)

Signed-off-by: Lukasz Majewski <[e-mail]>
Reviewed-by: Bartlomiej Zolnierkiewicz <[e-mail]>
Reviewed-by: Tomasz Figa <[e-mail]>
---
 drivers/cpufreq/exynos4x12-cpufreq.c |   23 ++++-------------------
 1 file changed, 4 insertions(+), 19 deletions(-)

Comments

Sachin Kamat Sept. 25, 2013, 11:35 a.m. UTC | #1

Hi Lukasz,

On 25 September 2013 16:52, Lukasz Majewski <[e-mail]> wrote:
>
>  static void exynos4x12_set_apll(unsigned int index)
>  {
> -       unsigned int tmp, pdiv;
> +       unsigned int tmp, freq = apll_freq_4x12[index].freq;

nit: It is better to put the 'freq' assignment on a new line.

>
> -       /* 1. MUX_CORE_SEL = MPLL, ARMCLK uses MPLL for lock time */
> +       /* MUX_CORE_SEL = MPLL, ARMCLK uses MPLL for lock time */
>         clk_set_parent(moutcore, mout_mpll);
>
>         do {
> @@ -140,24 +140,9 @@ static void exynos4x12_set_apll(unsigned int index)
>                 tmp &= 0x7;
>         } while (tmp != 0x2);
>
> -       /* 2. Set APLL Lock time */
> -       pdiv = ((apll_freq_4x12[index].mps >> 8) & 0x3f);
> +       clk_set_rate(mout_apll, freq * 1000);

Don't we need to check the return value of this?

Same comments for the second patch too.

Lukasz Majewski Sept. 25, 2013, 1:10 p.m. UTC | #2

Hi Sachin,

> Hi Lukasz,
> 
> On 25 September 2013 16:52, Lukasz Majewski <[e-mail]>
> wrote:
> >
> >  static void exynos4x12_set_apll(unsigned int index)
> >  {
> > -       unsigned int tmp, pdiv;
> > +       unsigned int tmp, freq = apll_freq_4x12[index].freq;
> 
> nit: It is better to put the 'freq' assignment on a new line.

checkpatch.pl wasn't complaining :-).
Also please consider below comment.

> 
> >
> > -       /* 1. MUX_CORE_SEL = MPLL, ARMCLK uses MPLL for lock time */
> > +       /* MUX_CORE_SEL = MPLL, ARMCLK uses MPLL for lock time */
> >         clk_set_parent(moutcore, mout_mpll);
> >
> >         do {
> > @@ -140,24 +140,9 @@ static void exynos4x12_set_apll(unsigned int
> > index) tmp &= 0x7;
> >         } while (tmp != 0x2);
> >
> > -       /* 2. Set APLL Lock time */
> > -       pdiv = ((apll_freq_4x12[index].mps >> 8) & 0x3f);
> > +       clk_set_rate(mout_apll, freq * 1000);
> 
> Don't we need to check the return value of this?

The broken code isn't handling errors now (*_set_apll() function is
defined as void). Since this patch is a regression fix (for v3.12) I
just wanted to change as little as possible to provide a functional fix.

I think that regression fix shall not change much functionality -
therefore the exynosXXXX-cpufreq.c cleanup will be done for next kernel
release.

> 
> Same comments for the second patch too.
>

Yadwinder Singh Brar Sept. 25, 2013, 1:55 p.m. UTC | #3

Hi Lukasz,

On Wed, Sep 25, 2013 at 4:52 PM, Lukasz Majewski <[e-mail]> wrote:
> In the exynos4x12_set_apll() function, the APLL frequency is set with
> direct register manipulation.
>
> Such approach is not allowed in the common clock framework. The frequency
> is changed, but the corresponding clock value is not updated. This causes
> wrong frequency read from cpufreq's cpuinfo_cur_freq sysfs attribute.
>

This patch looks incomplete, leaving the driver in untidy state, perhaps its
doesn't fix the above stated problem completely. what about
if (!exynos4x12_pms_change(old_index, new_index)) becomes true?

IMHO, this driver needs lot more work in addition to this patch to cleanly and
completely move the cpufreq driver to common clock framework.

For fixing this issue urgently, setting CLK_GET_RATE_NOCACHE for apll
in clk driver can also be quicker fix.

Regards,
Yadwinder

> Tested at:
> - Exynos4412 - Trats2 board (linux 3.12-rc1)
>
> Signed-off-by: Lukasz Majewski <[e-mail]>
> Reviewed-by: Bartlomiej Zolnierkiewicz <[e-mail]>
> Reviewed-by: Tomasz Figa <[e-mail]>
> ---
>  drivers/cpufreq/exynos4x12-cpufreq.c |   23 ++++-------------------
>  1 file changed, 4 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/cpufreq/exynos4x12-cpufreq.c b/drivers/cpufreq/exynos4x12-cpufreq.c
> index 08b7477..b2f51c9 100644
> --- a/drivers/cpufreq/exynos4x12-cpufreq.c
> +++ b/drivers/cpufreq/exynos4x12-cpufreq.c
> @@ -128,9 +128,9 @@ static void exynos4x12_set_clkdiv(unsigned int div_index)
>
>  static void exynos4x12_set_apll(unsigned int index)
>  {
> -       unsigned int tmp, pdiv;
> +       unsigned int tmp, freq = apll_freq_4x12[index].freq;
>
> -       /* 1. MUX_CORE_SEL = MPLL, ARMCLK uses MPLL for lock time */
> +       /* MUX_CORE_SEL = MPLL, ARMCLK uses MPLL for lock time */
>         clk_set_parent(moutcore, mout_mpll);
>
>         do {
> @@ -140,24 +140,9 @@ static void exynos4x12_set_apll(unsigned int index)
>                 tmp &= 0x7;
>         } while (tmp != 0x2);
>
> -       /* 2. Set APLL Lock time */
> -       pdiv = ((apll_freq_4x12[index].mps >> 8) & 0x3f);
> +       clk_set_rate(mout_apll, freq * 1000);
>
> -       __raw_writel((pdiv * 250), EXYNOS4_APLL_LOCK);
> -
> -       /* 3. Change PLL PMS values */
> -       tmp = __raw_readl(EXYNOS4_APLL_CON0);
> -       tmp &= ~((0x3ff << 16) | (0x3f << 8) | (0x7 << 0));
> -       tmp |= apll_freq_4x12[index].mps;
> -       __raw_writel(tmp, EXYNOS4_APLL_CON0);
> -
> -       /* 4. wait_lock_time */
> -       do {
> -               cpu_relax();
> -               tmp = __raw_readl(EXYNOS4_APLL_CON0);
> -       } while (!(tmp & (0x1 << EXYNOS4_APLLCON0_LOCKED_SHIFT)));
> -
> -       /* 5. MUX_CORE_SEL = APLL */
> +       /* MUX_CORE_SEL = APLL */
>         clk_set_parent(moutcore, mout_apll);
>
>         do {
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tomasz Figa Sept. 25, 2013, 2:03 p.m. UTC | #4

Hi Yadwinder,

[resending in plain text, as my mail client somehow reset message 
format from plain back to HTML...]

On Wednesday 25 of September 2013 19:25:54 Yadwinder Singh Brar wrote:
> Hi Lukasz,
> 
> On Wed, Sep 25, 2013 at 4:52 PM, Lukasz Majewski <[e-mail]> wrote:
> > In the exynos4x12_set_apll() function, the APLL frequency is set with
> > direct register manipulation.
> >
> > Such approach is not allowed in the common clock framework. The frequency
> > is changed, but the corresponding clock value is not updated. This causes
> > wrong frequency read from cpufreq's cpuinfo_cur_freq sysfs attribute.
> >
> 
> This patch looks incomplete, leaving the driver in untidy state, perhaps its
> doesn't fix the above stated problem completely. what about
> if (!exynos4x12_pms_change(old_index, new_index)) becomes true?
> 
> IMHO, this driver needs lot more work in addition to this patch to cleanly and
> completely move the cpufreq driver to common clock framework.

I agree that the other case needs to be handled as well. Basically the
whole conditional block dependent on exynos4x12_pms_change() can be safely
dropped, because this condition is already handled in PLL driver.

Lukasz is already working on further rework of this driver to clean it up
from legacy code, but this will have to wait for 3.13, as 3.12 is already
in rc stage and only fixes can be accepted for it.

> For fixing this issue urgently, setting CLK_GET_RATE_NOCACHE for apll
> in clk driver can also be quicker fix.

Unfortunately this is not how this flag works. It only makes
clk_get_rate() call ->recalc_rate() operation of the clock instead of
instantly returning cached rate - it doesn't seem to work recursively.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Yadwinder Singh Brar Sept. 26, 2013, 6:14 a.m. UTC | #5

Hi Tomasz,

>> On Wed, Sep 25, 2013 at 4:52 PM, Lukasz Majewski <[e-mail]> wrote:
>> > In the exynos4x12_set_apll() function, the APLL frequency is set with
>> > direct register manipulation.
>> >
>> > Such approach is not allowed in the common clock framework. The frequency
>> > is changed, but the corresponding clock value is not updated. This causes
>> > wrong frequency read from cpufreq's cpuinfo_cur_freq sysfs attribute.
>> >
>>
>> This patch looks incomplete, leaving the driver in untidy state, perhaps its
>> doesn't fix the above stated problem completely. what about
>> if (!exynos4x12_pms_change(old_index, new_index)) becomes true?
>>
>> IMHO, this driver needs lot more work in addition to this patch to cleanly and
>> completely move the cpufreq driver to common clock framework.
>
> I agree that the other case needs to be handled as well. Basically the
> whole conditional block dependent on exynos4x12_pms_change() can be safely
> dropped, because this condition is already handled in PLL driver.
>

Exactly!

> Lukasz is already working on further rework of this driver to clean it up
> from legacy code, but this will have to wait for 3.13, as 3.12 is already
> in rc stage and only fixes can be accepted for it.
>
>> For fixing this issue urgently, setting CLK_GET_RATE_NOCACHE for apll
>> in clk driver can also be quicker fix.
>
> Unfortunately this is not how this flag works. It only makes
> clk_get_rate() call ->recalc_rate() operation of the clock instead of
> instantly returning cached rate - it doesn't seem to work recursively.
>

hmm.. yes it can't help in our case as it recursively walks only the subtree
of clk but in our case we are changing rate of parent.

Regards,
Yadwinder
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Lukasz Majewski Sept. 26, 2013, 9:16 a.m. UTC | #6

Hi Yadwinder,

> Hi Tomasz,
> 
> >> On Wed, Sep 25, 2013 at 4:52 PM, Lukasz Majewski
> >> <[e-mail]> wrote:
> >> > In the exynos4x12_set_apll() function, the APLL frequency is set
> >> > with direct register manipulation.
> >> >
> >> > Such approach is not allowed in the common clock framework. The
> >> > frequency is changed, but the corresponding clock value is not
> >> > updated. This causes wrong frequency read from cpufreq's
> >> > cpuinfo_cur_freq sysfs attribute.
> >> >
> >>
> >> This patch looks incomplete, leaving the driver in untidy state,
> >> perhaps its doesn't fix the above stated problem completely. what
> >> about if (!exynos4x12_pms_change(old_index, new_index)) becomes
> >> true?
> >>
> >> IMHO, this driver needs lot more work in addition to this patch to
> >> cleanly and completely move the cpufreq driver to common clock
> >> framework.
> >
> > I agree that the other case needs to be handled as well. Basically
> > the whole conditional block dependent on exynos4x12_pms_change()
> > can be safely dropped, because this condition is already handled in
> > PLL driver.
> >
> 
> Exactly!

After more corner case testing, I admit that corner cases with PLL s
parameter change are still broken (e.g. 1400000 -> 700000).

I will prepare v2 for it.

If I manage I will push it into -rc. If not those changes will be
included to exynos cpufreq rework. 

Thanks guys for a thorough review.

> 
> > Lukasz is already working on further rework of this driver to clean
> > it up from legacy code, but this will have to wait for 3.13, as
> > 3.12 is already in rc stage and only fixes can be accepted for it.
> >
> >> For fixing this issue urgently, setting CLK_GET_RATE_NOCACHE for
> >> apll in clk driver can also be quicker fix.
> >
> > Unfortunately this is not how this flag works. It only makes
> > clk_get_rate() call ->recalc_rate() operation of the clock instead
> > of instantly returning cached rate - it doesn't seem to work
> > recursively.
> >
> 
> hmm.. yes it can't help in our case as it recursively walks only the
> subtree of clk but in our case we are changing rate of parent.
> 
> Regards,
> Yadwinder

Patch

diff --git a/drivers/cpufreq/exynos4x12-cpufreq.c b/drivers/cpufreq/exynos4x12-cpufreq.c
index 08b7477..b2f51c9 100644
--- a/drivers/cpufreq/exynos4x12-cpufreq.c
+++ b/drivers/cpufreq/exynos4x12-cpufreq.c
@@ -128,9 +128,9 @@  static void exynos4x12_set_clkdiv(unsigned int div_index)
 
 static void exynos4x12_set_apll(unsigned int index)
 {
-	unsigned int tmp, pdiv;
+	unsigned int tmp, freq = apll_freq_4x12[index].freq;
 
-	/* 1. MUX_CORE_SEL = MPLL, ARMCLK uses MPLL for lock time */
+	/* MUX_CORE_SEL = MPLL, ARMCLK uses MPLL for lock time */
 	clk_set_parent(moutcore, mout_mpll);
 
 	do {
@@ -140,24 +140,9 @@  static void exynos4x12_set_apll(unsigned int index)
 		tmp &= 0x7;
 	} while (tmp != 0x2);
 
-	/* 2. Set APLL Lock time */
-	pdiv = ((apll_freq_4x12[index].mps >> 8) & 0x3f);
+	clk_set_rate(mout_apll, freq * 1000);
 
-	__raw_writel((pdiv * 250), EXYNOS4_APLL_LOCK);
-
-	/* 3. Change PLL PMS values */
-	tmp = __raw_readl(EXYNOS4_APLL_CON0);
-	tmp &= ~((0x3ff << 16) | (0x3f << 8) | (0x7 << 0));
-	tmp |= apll_freq_4x12[index].mps;
-	__raw_writel(tmp, EXYNOS4_APLL_CON0);
-
-	/* 4. wait_lock_time */
-	do {
-		cpu_relax();
-		tmp = __raw_readl(EXYNOS4_APLL_CON0);
-	} while (!(tmp & (0x1 << EXYNOS4_APLLCON0_LOCKED_SHIFT)));
-
-	/* 5. MUX_CORE_SEL = APLL */
+	/* MUX_CORE_SEL = APLL */
 	clk_set_parent(moutcore, mout_apll);
 
 	do {