[PATCH 0/2] cpufreq: exynos: Fixes for v3.12

[PATCH 0/2] cpufreq: exynos: Fixes for v3.12

[Lukasz Majewski]

Attached commits provide cpufreq regression fixes for Trats and Trats2
Exynos4 boards.
Since v3.12 Exynos4 uses common clock framework for clock manipulation.
Those patches restore correct output for cpuinfo_cur_freq [1] sysfs
attribute.
Without them, the [1] provides default frequency (800 MHz) set at driver
initialization.

Lukasz Majewski (2):
  cpufreq: exynos4x12: Use the common clock framework to set APLL clock
    rate
  cpufreq: exynos4210: Use the common clock framework to set APLL clock
    rate

 drivers/cpufreq/exynos4210-cpufreq.c |   20 ++++----------------
 drivers/cpufreq/exynos4x12-cpufreq.c |   23 ++++-------------------
 2 files changed, 8 insertions(+), 35 deletions(-)

-- 
1.7.10.4

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

[Lukasz Majewski]

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 <l.majewski@samsung.com>
Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Reviewed-by: Tomasz Figa <t.figa@samsung.com>
---
 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

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

[Sachin Kamat]

Hi Lukasz,

On 25 September 2013 16:52, Lukasz Majewski <l.majewski@samsung.com> 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.

-- 
With warm regards,
Sachin

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

[Lukasz Majewski]

Hi Sachin,

> Hi Lukasz,
> 
> On 25 September 2013 16:52, Lukasz Majewski <l.majewski@samsung.com>
> 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.
> 



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

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

[Yadwinder Singh Brar]

Hi Lukasz,

On Wed, Sep 25, 2013 at 4:52 PM, Lukasz Majewski <l.majewski@samsung.com> 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 <l.majewski@samsung.com>
> Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Reviewed-by: Tomasz Figa <t.figa@samsung.com>
> ---
>  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Tomasz Figa]

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 <l.majewski@samsung.com> 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

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

[Yadwinder Singh Brar]

Hi Tomasz,

>> On Wed, Sep 25, 2013 at 4:52 PM, Lukasz Majewski <l.majewski@samsung.com> 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

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

[Lukasz Majewski]

Hi Yadwinder,

> Hi Tomasz,
> 
> >> On Wed, Sep 25, 2013 at 4:52 PM, Lukasz Majewski
> >> <l.majewski@samsung.com> 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



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[PATCH 2/2] cpufreq: exynos4210: Use the common clock framework to set APLL clock rate

[Lukasz Majewski]

In the exynos4210_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:
- Exynos4210 - Trats board (linux 3.12-rc1)

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
---
 drivers/cpufreq/exynos4210-cpufreq.c |   20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/cpufreq/exynos4210-cpufreq.c b/drivers/cpufreq/exynos4210-cpufreq.c
index add7fbe..363c658 100644
--- a/drivers/cpufreq/exynos4210-cpufreq.c
+++ b/drivers/cpufreq/exynos4210-cpufreq.c
@@ -81,9 +81,9 @@ static void exynos4210_set_clkdiv(unsigned int div_index)
 
 static void exynos4210_set_apll(unsigned int index)
 {
-	unsigned int tmp;
+	unsigned int tmp, freq = apll_freq_4210[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 {
@@ -92,21 +92,9 @@ static void exynos4210_set_apll(unsigned int index)
 		tmp &= 0x7;
 	} while (tmp != 0x2);
 
-	/* 2. Set APLL Lock time */
-	__raw_writel(EXYNOS4_APLL_LOCKTIME, EXYNOS4_APLL_LOCK);
-
-	/* 3. Change PLL PMS values */
-	tmp = __raw_readl(EXYNOS4_APLL_CON0);
-	tmp &= ~((0x3ff << 16) | (0x3f << 8) | (0x7 << 0));
-	tmp |= apll_freq_4210[index].mps;
-	__raw_writel(tmp, EXYNOS4_APLL_CON0);
-
-	/* 4. wait_lock_time */
-	do {
-		tmp = __raw_readl(EXYNOS4_APLL_CON0);
-	} while (!(tmp & (0x1 << EXYNOS4_APLLCON0_LOCKED_SHIFT)));
+	clk_set_rate(mout_apll, freq * 1000);
 
-	/* 5. MUX_CORE_SEL = APLL */
+	/* MUX_CORE_SEL = APLL */
 	clk_set_parent(moutcore, mout_apll);
 
 	do {
-- 
1.7.10.4

[PATCH v2 0/2] cpufreq: exynos: Fixes for v3.12

[Lukasz Majewski]

Attached commits provide cpufreq regression fixes for Trats and Trats2
Exynos4 boards.
Since v3.12 Exynos4 uses common clock framework for clock manipulation.
Those patches restore correct output for cpuinfo_cur_freq [1] sysfs
attribute.
Without them, the [1] provides default frequency (800 MHz) set at driver
initialization.


Lukasz Majewski (2):
  cpufreq: exynos4x12: Use the common clock framework to set APLL clock
    rate
  cpufreq: exynos4210: Use the common clock framework to set APLL clock
    rate

 drivers/cpufreq/exynos4210-cpufreq.c |   67 ++++-----------------------------
 drivers/cpufreq/exynos4x12-cpufreq.c |   69 ++++------------------------------
 2 files changed, 16 insertions(+), 120 deletions(-)

-- 
1.7.10.4

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

[Lukasz Majewski]

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.

Also direct manipulation with PLL's S parameter has been removed. It is
already done at PLL35xx code.

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

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Reviewed-by: Tomasz Figa <t.figa@samsung.com>

Changes for v2:
- Remove PLL's S parameter setting via registers. It is now done with PLL35xx
  code.
- Remove exynos4x12_pms_change() function.

Change-Id: I6c32d6fd7f634ff8d07cbaa0ebe0e21614f9f3c9
---
 drivers/cpufreq/exynos4x12-cpufreq.c |   69 ++++------------------------------
 1 file changed, 8 insertions(+), 61 deletions(-)

diff --git a/drivers/cpufreq/exynos4x12-cpufreq.c b/drivers/cpufreq/exynos4x12-cpufreq.c
index 08b7477..8683304 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 {
@@ -167,52 +152,15 @@ static void exynos4x12_set_apll(unsigned int index)
 	} while (tmp != (0x1 << EXYNOS4_CLKSRC_CPU_MUXCORE_SHIFT));
 }
 
-static bool exynos4x12_pms_change(unsigned int old_index, unsigned int new_index)
-{
-	unsigned int old_pm = apll_freq_4x12[old_index].mps >> 8;
-	unsigned int new_pm = apll_freq_4x12[new_index].mps >> 8;
-
-	return (old_pm == new_pm) ? 0 : 1;
-}
-
 static void exynos4x12_set_frequency(unsigned int old_index,
 				  unsigned int new_index)
 {
-	unsigned int tmp;
-
 	if (old_index > new_index) {
-		if (!exynos4x12_pms_change(old_index, new_index)) {
-			/* 1. Change the system clock divider values */
-			exynos4x12_set_clkdiv(new_index);
-			/* 2. Change just s value in apll m,p,s value */
-			tmp = __raw_readl(EXYNOS4_APLL_CON0);
-			tmp &= ~(0x7 << 0);
-			tmp |= apll_freq_4x12[new_index].mps & 0x7;
-			__raw_writel(tmp, EXYNOS4_APLL_CON0);
-
-		} else {
-			/* Clock Configuration Procedure */
-			/* 1. Change the system clock divider values */
-			exynos4x12_set_clkdiv(new_index);
-			/* 2. Change the apll m,p,s value */
-			exynos4x12_set_apll(new_index);
-		}
+		exynos4x12_set_clkdiv(new_index);
+		exynos4x12_set_apll(new_index);
 	} else if (old_index < new_index) {
-		if (!exynos4x12_pms_change(old_index, new_index)) {
-			/* 1. Change just s value in apll m,p,s value */
-			tmp = __raw_readl(EXYNOS4_APLL_CON0);
-			tmp &= ~(0x7 << 0);
-			tmp |= apll_freq_4x12[new_index].mps & 0x7;
-			__raw_writel(tmp, EXYNOS4_APLL_CON0);
-			/* 2. Change the system clock divider values */
-			exynos4x12_set_clkdiv(new_index);
-		} else {
-			/* Clock Configuration Procedure */
-			/* 1. Change the apll m,p,s value */
-			exynos4x12_set_apll(new_index);
-			/* 2. Change the system clock divider values */
-			exynos4x12_set_clkdiv(new_index);
-		}
+		exynos4x12_set_apll(new_index);
+		exynos4x12_set_clkdiv(new_index);
 	}
 }
 
@@ -250,7 +198,6 @@ int exynos4x12_cpufreq_init(struct exynos_dvfs_info *info)
 	info->volt_table = exynos4x12_volt_table;
 	info->freq_table = exynos4x12_freq_table;
 	info->set_freq = exynos4x12_set_frequency;
-	info->need_apll_change = exynos4x12_pms_change;
 
 	return 0;
 
-- 
1.7.10.4

[PATCH v2 2/2] cpufreq: exynos4210: Use the common clock framework to set APLL clock rate

[Lukasz Majewski]

In the exynos4210_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.

Also direct manipulation with PLL's S parameter has been removed. It is
already done at PLL35xx code.

Tested at:
- Exynos4210 - Trats board (linux 3.12-rc4)

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>

Changes for v2:
- Remove PLL's S parameter setting via registers. It is now done with PLL35xx
  code.
- Remove exynos4210_pms_change() function

Change-Id: Ie5fb3c7946ba77b6f3d5e91af72eef2fd06770c1
---
 drivers/cpufreq/exynos4210-cpufreq.c |   67 ++++------------------------------
 1 file changed, 8 insertions(+), 59 deletions(-)

diff --git a/drivers/cpufreq/exynos4210-cpufreq.c b/drivers/cpufreq/exynos4210-cpufreq.c
index add7fbe..f2c7506 100644
--- a/drivers/cpufreq/exynos4210-cpufreq.c
+++ b/drivers/cpufreq/exynos4210-cpufreq.c
@@ -81,9 +81,9 @@ static void exynos4210_set_clkdiv(unsigned int div_index)
 
 static void exynos4210_set_apll(unsigned int index)
 {
-	unsigned int tmp;
+	unsigned int tmp, freq = apll_freq_4210[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 {
@@ -92,21 +92,9 @@ static void exynos4210_set_apll(unsigned int index)
 		tmp &= 0x7;
 	} while (tmp != 0x2);
 
-	/* 2. Set APLL Lock time */
-	__raw_writel(EXYNOS4_APLL_LOCKTIME, EXYNOS4_APLL_LOCK);
-
-	/* 3. Change PLL PMS values */
-	tmp = __raw_readl(EXYNOS4_APLL_CON0);
-	tmp &= ~((0x3ff << 16) | (0x3f << 8) | (0x7 << 0));
-	tmp |= apll_freq_4210[index].mps;
-	__raw_writel(tmp, EXYNOS4_APLL_CON0);
+	clk_set_rate(mout_apll, freq * 1000);
 
-	/* 4. wait_lock_time */
-	do {
-		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 {
@@ -115,53 +103,15 @@ static void exynos4210_set_apll(unsigned int index)
 	} while (tmp != (0x1 << EXYNOS4_CLKSRC_CPU_MUXCORE_SHIFT));
 }
 
-static bool exynos4210_pms_change(unsigned int old_index, unsigned int new_index)
-{
-	unsigned int old_pm = apll_freq_4210[old_index].mps >> 8;
-	unsigned int new_pm = apll_freq_4210[new_index].mps >> 8;
-
-	return (old_pm == new_pm) ? 0 : 1;
-}
-
 static void exynos4210_set_frequency(unsigned int old_index,
 				     unsigned int new_index)
 {
-	unsigned int tmp;
-
 	if (old_index > new_index) {
-		if (!exynos4210_pms_change(old_index, new_index)) {
-			/* 1. Change the system clock divider values */
-			exynos4210_set_clkdiv(new_index);
-
-			/* 2. Change just s value in apll m,p,s value */
-			tmp = __raw_readl(EXYNOS4_APLL_CON0);
-			tmp &= ~(0x7 << 0);
-			tmp |= apll_freq_4210[new_index].mps & 0x7;
-			__raw_writel(tmp, EXYNOS4_APLL_CON0);
-		} else {
-			/* Clock Configuration Procedure */
-			/* 1. Change the system clock divider values */
-			exynos4210_set_clkdiv(new_index);
-			/* 2. Change the apll m,p,s value */
-			exynos4210_set_apll(new_index);
-		}
+		exynos4210_set_clkdiv(new_index);
+		exynos4210_set_apll(new_index);
 	} else if (old_index < new_index) {
-		if (!exynos4210_pms_change(old_index, new_index)) {
-			/* 1. Change just s value in apll m,p,s value */
-			tmp = __raw_readl(EXYNOS4_APLL_CON0);
-			tmp &= ~(0x7 << 0);
-			tmp |= apll_freq_4210[new_index].mps & 0x7;
-			__raw_writel(tmp, EXYNOS4_APLL_CON0);
-
-			/* 2. Change the system clock divider values */
-			exynos4210_set_clkdiv(new_index);
-		} else {
-			/* Clock Configuration Procedure */
-			/* 1. Change the apll m,p,s value */
-			exynos4210_set_apll(new_index);
-			/* 2. Change the system clock divider values */
-			exynos4210_set_clkdiv(new_index);
-		}
+		exynos4210_set_apll(new_index);
+		exynos4210_set_clkdiv(new_index);
 	}
 }
 
@@ -194,7 +144,6 @@ int exynos4210_cpufreq_init(struct exynos_dvfs_info *info)
 	info->volt_table = exynos4210_volt_table;
 	info->freq_table = exynos4210_freq_table;
 	info->set_freq = exynos4210_set_frequency;
-	info->need_apll_change = exynos4210_pms_change;
 
 	return 0;
 
-- 
1.7.10.4

Re: [PATCH v2 2/2] cpufreq: exynos4210: Use the common clock framework to set APLL clock rate

[Rafael J. Wysocki]

On Wednesday, October 09, 2013 02:08:43 PM Lukasz Majewski wrote:
> In the exynos4210_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.
> 
> Also direct manipulation with PLL's S parameter has been removed. It is
> already done at PLL35xx code.
> 
> Tested at:
> - Exynos4210 - Trats board (linux 3.12-rc4)
> 
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>

I need an ACK or Reviewed-by from someone in the CC list here.

Thanks!

> Changes for v2:
> - Remove PLL's S parameter setting via registers. It is now done with PLL35xx
>   code.
> - Remove exynos4210_pms_change() function
> 
> Change-Id: Ie5fb3c7946ba77b6f3d5e91af72eef2fd06770c1
> ---
>  drivers/cpufreq/exynos4210-cpufreq.c |   67 ++++------------------------------
>  1 file changed, 8 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/cpufreq/exynos4210-cpufreq.c b/drivers/cpufreq/exynos4210-cpufreq.c
> index add7fbe..f2c7506 100644
> --- a/drivers/cpufreq/exynos4210-cpufreq.c
> +++ b/drivers/cpufreq/exynos4210-cpufreq.c
> @@ -81,9 +81,9 @@ static void exynos4210_set_clkdiv(unsigned int div_index)
>  
>  static void exynos4210_set_apll(unsigned int index)
>  {
> -	unsigned int tmp;
> +	unsigned int tmp, freq = apll_freq_4210[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 {
> @@ -92,21 +92,9 @@ static void exynos4210_set_apll(unsigned int index)
>  		tmp &= 0x7;
>  	} while (tmp != 0x2);
>  
> -	/* 2. Set APLL Lock time */
> -	__raw_writel(EXYNOS4_APLL_LOCKTIME, EXYNOS4_APLL_LOCK);
> -
> -	/* 3. Change PLL PMS values */
> -	tmp = __raw_readl(EXYNOS4_APLL_CON0);
> -	tmp &= ~((0x3ff << 16) | (0x3f << 8) | (0x7 << 0));
> -	tmp |= apll_freq_4210[index].mps;
> -	__raw_writel(tmp, EXYNOS4_APLL_CON0);
> +	clk_set_rate(mout_apll, freq * 1000);
>  
> -	/* 4. wait_lock_time */
> -	do {
> -		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 {
> @@ -115,53 +103,15 @@ static void exynos4210_set_apll(unsigned int index)
>  	} while (tmp != (0x1 << EXYNOS4_CLKSRC_CPU_MUXCORE_SHIFT));
>  }
>  
> -static bool exynos4210_pms_change(unsigned int old_index, unsigned int new_index)
> -{
> -	unsigned int old_pm = apll_freq_4210[old_index].mps >> 8;
> -	unsigned int new_pm = apll_freq_4210[new_index].mps >> 8;
> -
> -	return (old_pm == new_pm) ? 0 : 1;
> -}
> -
>  static void exynos4210_set_frequency(unsigned int old_index,
>  				     unsigned int new_index)
>  {
> -	unsigned int tmp;
> -
>  	if (old_index > new_index) {
> -		if (!exynos4210_pms_change(old_index, new_index)) {
> -			/* 1. Change the system clock divider values */
> -			exynos4210_set_clkdiv(new_index);
> -
> -			/* 2. Change just s value in apll m,p,s value */
> -			tmp = __raw_readl(EXYNOS4_APLL_CON0);
> -			tmp &= ~(0x7 << 0);
> -			tmp |= apll_freq_4210[new_index].mps & 0x7;
> -			__raw_writel(tmp, EXYNOS4_APLL_CON0);
> -		} else {
> -			/* Clock Configuration Procedure */
> -			/* 1. Change the system clock divider values */
> -			exynos4210_set_clkdiv(new_index);
> -			/* 2. Change the apll m,p,s value */
> -			exynos4210_set_apll(new_index);
> -		}
> +		exynos4210_set_clkdiv(new_index);
> +		exynos4210_set_apll(new_index);
>  	} else if (old_index < new_index) {
> -		if (!exynos4210_pms_change(old_index, new_index)) {
> -			/* 1. Change just s value in apll m,p,s value */
> -			tmp = __raw_readl(EXYNOS4_APLL_CON0);
> -			tmp &= ~(0x7 << 0);
> -			tmp |= apll_freq_4210[new_index].mps & 0x7;
> -			__raw_writel(tmp, EXYNOS4_APLL_CON0);
> -
> -			/* 2. Change the system clock divider values */
> -			exynos4210_set_clkdiv(new_index);
> -		} else {
> -			/* Clock Configuration Procedure */
> -			/* 1. Change the apll m,p,s value */
> -			exynos4210_set_apll(new_index);
> -			/* 2. Change the system clock divider values */
> -			exynos4210_set_clkdiv(new_index);
> -		}
> +		exynos4210_set_apll(new_index);
> +		exynos4210_set_clkdiv(new_index);
>  	}
>  }
>  
> @@ -194,7 +144,6 @@ int exynos4210_cpufreq_init(struct exynos_dvfs_info *info)
>  	info->volt_table = exynos4210_volt_table;
>  	info->freq_table = exynos4210_freq_table;
>  	info->set_freq = exynos4210_set_frequency;
> -	info->need_apll_change = exynos4210_pms_change;
>  
>  	return 0;
>  
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v2 0/2] cpufreq: exynos: Fixes for v3.12

[Yadwinder Singh Brar]

On Wed, Oct 9, 2013 at 5:38 PM, Lukasz Majewski <l.majewski@samsung.com> wrote:
> Attached commits provide cpufreq regression fixes for Trats and Trats2
> Exynos4 boards.
> Since v3.12 Exynos4 uses common clock framework for clock manipulation.
> Those patches restore correct output for cpuinfo_cur_freq [1] sysfs
> attribute.
> Without them, the [1] provides default frequency (800 MHz) set at driver
> initialization.
>
>
> Lukasz Majewski (2):
>   cpufreq: exynos4x12: Use the common clock framework to set APLL clock
>     rate
>   cpufreq: exynos4210: Use the common clock framework to set APLL clock
>     rate
>

Just as a fix for 3.12, these patches looks OK to me.

Reviewed-by: Yadwinder Singh Brar <yadi.brar@samsung.com>


Regards,
Yadwinder

>  drivers/cpufreq/exynos4210-cpufreq.c |   67 ++++-----------------------------
>  drivers/cpufreq/exynos4x12-cpufreq.c |   69 ++++------------------------------
>  2 files changed, 16 insertions(+), 120 deletions(-)
>
> --
> 1.7.10.4
>

Re: [PATCH v2 0/2] cpufreq: exynos: Fixes for v3.12

[Rafael J. Wysocki]

On Wednesday, October 09, 2013 02:08:41 PM Lukasz Majewski wrote:
> Attached commits provide cpufreq regression fixes for Trats and Trats2
> Exynos4 boards.
> Since v3.12 Exynos4 uses common clock framework for clock manipulation.
> Those patches restore correct output for cpuinfo_cur_freq [1] sysfs
> attribute.
> Without them, the [1] provides default frequency (800 MHz) set at driver
> initialization.
> 
> 
> Lukasz Majewski (2):
>   cpufreq: exynos4x12: Use the common clock framework to set APLL clock
>     rate
>   cpufreq: exynos4210: Use the common clock framework to set APLL clock
>     rate
> 
>  drivers/cpufreq/exynos4210-cpufreq.c |   67 ++++-----------------------------
>  drivers/cpufreq/exynos4x12-cpufreq.c |   69 ++++------------------------------
>  2 files changed, 16 insertions(+), 120 deletions(-)

Can you please resend [1/2], it got lost somewhere (at least I can't see it
now)?


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v2 0/2] cpufreq: exynos: Fixes for v3.12

[Rafael J. Wysocki]

On Friday, October 11, 2013 01:22:57 PM Rafael J. Wysocki wrote:
> On Wednesday, October 09, 2013 02:08:41 PM Lukasz Majewski wrote:
> > Attached commits provide cpufreq regression fixes for Trats and Trats2
> > Exynos4 boards.
> > Since v3.12 Exynos4 uses common clock framework for clock manipulation.
> > Those patches restore correct output for cpuinfo_cur_freq [1] sysfs
> > attribute.
> > Without them, the [1] provides default frequency (800 MHz) set at driver
> > initialization.
> > 
> > 
> > Lukasz Majewski (2):
> >   cpufreq: exynos4x12: Use the common clock framework to set APLL clock
> >     rate
> >   cpufreq: exynos4210: Use the common clock framework to set APLL clock
> >     rate
> > 
> >  drivers/cpufreq/exynos4210-cpufreq.c |   67 ++++-----------------------------
> >  drivers/cpufreq/exynos4x12-cpufreq.c |   69 ++++------------------------------
> >  2 files changed, 16 insertions(+), 120 deletions(-)
> 
> Can you please resend [1/2], it got lost somewhere (at least I can't see it
> now)?

Scratch this, it's there in patchwork.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v2 0/2] cpufreq: exynos: Fixes for v3.12

[Lukasz Majewski]

Hi Rafael,

> On Friday, October 11, 2013 01:22:57 PM Rafael J. Wysocki wrote:
> > On Wednesday, October 09, 2013 02:08:41 PM Lukasz Majewski wrote:
> > > Attached commits provide cpufreq regression fixes for Trats and
> > > Trats2 Exynos4 boards.
> > > Since v3.12 Exynos4 uses common clock framework for clock
> > > manipulation. Those patches restore correct output for
> > > cpuinfo_cur_freq [1] sysfs attribute.
> > > Without them, the [1] provides default frequency (800 MHz) set at
> > > driver initialization.
> > > 
> > > 
> > > Lukasz Majewski (2):
> > >   cpufreq: exynos4x12: Use the common clock framework to set APLL
> > > clock rate
> > >   cpufreq: exynos4210: Use the common clock framework to set APLL
> > > clock rate
> > > 
> > >  drivers/cpufreq/exynos4210-cpufreq.c |   67
> > > ++++-----------------------------
> > > drivers/cpufreq/exynos4x12-cpufreq.c |   69
> > > ++++------------------------------ 2 files changed, 16
> > > insertions(+), 120 deletions(-)
> > 
> > Can you please resend [1/2], it got lost somewhere (at least I
> > can't see it now)?
> 
> Scratch this, it's there in patchwork.
> 

Rafael, do you need any more help with those fixes? 

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH v2 0/2] cpufreq: exynos: Fixes for v3.12

[Rafael J. Wysocki]

On Monday, October 14, 2013 07:55:46 AM Lukasz Majewski wrote:
> Hi Rafael,
> 
> > On Friday, October 11, 2013 01:22:57 PM Rafael J. Wysocki wrote:
> > > On Wednesday, October 09, 2013 02:08:41 PM Lukasz Majewski wrote:
> > > > Attached commits provide cpufreq regression fixes for Trats and
> > > > Trats2 Exynos4 boards.
> > > > Since v3.12 Exynos4 uses common clock framework for clock
> > > > manipulation. Those patches restore correct output for
> > > > cpuinfo_cur_freq [1] sysfs attribute.
> > > > Without them, the [1] provides default frequency (800 MHz) set at
> > > > driver initialization.
> > > > 
> > > > 
> > > > Lukasz Majewski (2):
> > > >   cpufreq: exynos4x12: Use the common clock framework to set APLL
> > > > clock rate
> > > >   cpufreq: exynos4210: Use the common clock framework to set APLL
> > > > clock rate
> > > > 
> > > >  drivers/cpufreq/exynos4210-cpufreq.c |   67
> > > > ++++-----------------------------
> > > > drivers/cpufreq/exynos4x12-cpufreq.c |   69
> > > > ++++------------------------------ 2 files changed, 16
> > > > insertions(+), 120 deletions(-)
> > > 
> > > Can you please resend [1/2], it got lost somewhere (at least I
> > > can't see it now)?
> > 
> > Scratch this, it's there in patchwork.
> > 
> 
> Rafael, do you need any more help with those fixes? 

Well, if I do, you'll know that. :-)

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.