diff mbox

[v9,4/6] ARM: Exynos: switch to using generic cpufreq driver for Exynos4210/5250/5420

Message ID 7hbnrc6px3.fsf@paris.lan (mailing list archive)
State New, archived
Headers show

Commit Message

Kevin Hilman Aug. 22, 2014, 11:54 p.m. UTC
Tomasz Figa <tomasz.figa@gmail.com> writes:

> Kukjin,
>
> On 31.07.2014 20:32, Kukjin Kim wrote:
>> On 07/30/14 17:07, Thomas Abraham wrote:
>>> The new CPU clock type allows the use of generic CPUfreq drivers. So for
>>> Exynos4210/5250, switch to using generic cpufreq driver. For Exynos5420,
>>> which did not have CPUfreq driver support, enable the use of generic
>>> CPUfreq driver.
>>>
>>> Suggested-by: Tomasz Figa<t.figa@samsung.com>
>>> Cc: Kukjin Kim<kgene.kim@samsung.com>
>> 
>> Looks good to me,
>> 
>> Acked-by: Kukjin Kim <kgene.kim@samsung.com>
>> 
>> BTW, who will handle this series? I hope see this series in 3.17.
>
> This series consists mostly of clock changes and it likely depends on
> patches already in my for-next, so I would be inclined toward taking it
> through samsung-clk tree. 

So has this series been picked up anywhere?  I don't see it in your
samsung-clk tree, nor in Kukjin's for-next.

Also, I'm curious whether or how this is has been tested on big.LITTLE
SoCs.  

I'm trying it on the 5800/Chromebook2 and it's not terribly stable.  I'm
testing along with CPUidle, so there may be some untested interactions
there as it seems a bit more stable without CPUidle enabled.

I'd love to hear from anyone else that's testing CPUidle and CPUfreq
together big.LITTLE 5420/5800, with or without the switcher.

Also, the patch below[2] is needed for 5800.

FWIW, I have a temporary branch[1] based on the v3.17-rc branch of the
exynos-reference tree where I've added the DT patch needed for CPUidle,
this series (and it's dependencies) which is what I'm using for testing.

Kevin

[1] git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux.git wip/exynos/integ
[2]
From 72ee00246c0fbdcf5dbb0bf910b8a427da4ac002 Mon Sep 17 00:00:00 2001
From: Kevin Hilman <khilman@linaro.org>
Date: Fri, 22 Aug 2014 16:04:11 -0700
Subject: [PATCH] ARM: Exynos: use generic cpufreq driver for Exynos5800

As a derivative of the 5420, the 5800 SoC should use the generic
big.LITTLE driver for Exynos5800 as well.

Signed-off-by: Kevin Hilman <khilman@linaro.org>
---
 arch/arm/mach-exynos/exynos.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Tomasz Figa Aug. 23, 2014, 12:02 a.m. UTC | #1
Hi Kevin,

Thanks for taking a look at this.

On 23.08.2014 01:54, Kevin Hilman wrote:
> Tomasz Figa <tomasz.figa@gmail.com> writes:
> 
>> Kukjin,
>>
>> On 31.07.2014 20:32, Kukjin Kim wrote:
>>> On 07/30/14 17:07, Thomas Abraham wrote:
>>>> The new CPU clock type allows the use of generic CPUfreq drivers. So for
>>>> Exynos4210/5250, switch to using generic cpufreq driver. For Exynos5420,
>>>> which did not have CPUfreq driver support, enable the use of generic
>>>> CPUfreq driver.
>>>>
>>>> Suggested-by: Tomasz Figa<t.figa@samsung.com>
>>>> Cc: Kukjin Kim<kgene.kim@samsung.com>
>>>
>>> Looks good to me,
>>>
>>> Acked-by: Kukjin Kim <kgene.kim@samsung.com>
>>>
>>> BTW, who will handle this series? I hope see this series in 3.17.
>>
>> This series consists mostly of clock changes and it likely depends on
>> patches already in my for-next, so I would be inclined toward taking it
>> through samsung-clk tree. 
> 
> So has this series been picked up anywhere?  I don't see it in your
> samsung-clk tree, nor in Kukjin's for-next.

No, it has not. In general it was already too late in the release cycle
when the last version was posted.

I had a plan to take it through clock tree with Kukjin's and Viresh's
cooperation, but now as you say it...

> 
> Also, I'm curious whether or how this is has been tested on big.LITTLE
> SoCs.  
> 
> I'm trying it on the 5800/Chromebook2 and it's not terribly stable.  I'm
> testing along with CPUidle, so there may be some untested interactions
> there as it seems a bit more stable without CPUidle enabled.
> 
> I'd love to hear from anyone else that's testing CPUidle and CPUfreq
> together big.LITTLE 5420/5800, with or without the switcher.

I'd definitely like to see a clarification on this issues, before this
series hits mainline or at least its parts related to affected SoCs.
Also I'd like to hear some confirmation from Samsung Poland R&D Center
guys (on CC), whether this code works stable on their target boards
(Universal C210, Trats, Trats2).

> 
> Also, the patch below[2] is needed for 5800.
> 
> FWIW, I have a temporary branch[1] based on the v3.17-rc branch of the
> exynos-reference tree where I've added the DT patch needed for CPUidle,
> this series (and it's dependencies) which is what I'm using for testing.

The patch looks fine to me (well, it's trivial :)), thanks.

Best regards,
Tomasz
Lukasz Majewski Aug. 25, 2014, 6:53 a.m. UTC | #2
Hi Tomasz,

> Hi Kevin,
> 
> Thanks for taking a look at this.
> 
> On 23.08.2014 01:54, Kevin Hilman wrote:
> > Tomasz Figa <tomasz.figa@gmail.com> writes:
> > 
> >> Kukjin,
> >>
> >> On 31.07.2014 20:32, Kukjin Kim wrote:
> >>> On 07/30/14 17:07, Thomas Abraham wrote:
> >>>> The new CPU clock type allows the use of generic CPUfreq
> >>>> drivers. So for Exynos4210/5250, switch to using generic cpufreq
> >>>> driver. For Exynos5420, which did not have CPUfreq driver
> >>>> support, enable the use of generic CPUfreq driver.
> >>>>
> >>>> Suggested-by: Tomasz Figa<t.figa@samsung.com>
> >>>> Cc: Kukjin Kim<kgene.kim@samsung.com>
> >>>
> >>> Looks good to me,
> >>>
> >>> Acked-by: Kukjin Kim <kgene.kim@samsung.com>
> >>>
> >>> BTW, who will handle this series? I hope see this series in 3.17.
> >>
> >> This series consists mostly of clock changes and it likely depends
> >> on patches already in my for-next, so I would be inclined toward
> >> taking it through samsung-clk tree. 
> > 
> > So has this series been picked up anywhere?  I don't see it in your
> > samsung-clk tree, nor in Kukjin's for-next.
> 
> No, it has not. In general it was already too late in the release
> cycle when the last version was posted.
> 
> I had a plan to take it through clock tree with Kukjin's and Viresh's
> cooperation, but now as you say it...
> 
> > 
> > Also, I'm curious whether or how this is has been tested on
> > big.LITTLE SoCs.  
> > 
> > I'm trying it on the 5800/Chromebook2 and it's not terribly
> > stable.  I'm testing along with CPUidle, so there may be some
> > untested interactions there as it seems a bit more stable without
> > CPUidle enabled.
> > 
> > I'd love to hear from anyone else that's testing CPUidle and CPUfreq
> > together big.LITTLE 5420/5800, with or without the switcher.
> 
> I'd definitely like to see a clarification on this issues, before this
> series hits mainline or at least its parts related to affected SoCs.

It is a huge step forward - to be honest it is a serious rework of
cpufreq subsystem for Exynos SoCs.

> Also I'd like to hear some confirmation from Samsung Poland R&D Center
> guys (on CC), whether this code works stable on their target boards
> (Universal C210, Trats, Trats2).
> 

Since we have missed the merge window with this code, I can declare
that I will provide code, which means that I will do the cleanup for
excluded from this series Exynos4 SoCs, to test the cpufreq-cpu0.

However, I'm concerned with Exynos4412, which supports BOOST. It might
not be trivial to provide support for it.

I think, that we shall not drop behind any functionality during clean
up.

> > 
> > Also, the patch below[2] is needed for 5800.
> > 
> > FWIW, I have a temporary branch[1] based on the v3.17-rc branch of
> > the exynos-reference tree where I've added the DT patch needed for
> > CPUidle, this series (and it's dependencies) which is what I'm
> > using for testing.
> 
> The patch looks fine to me (well, it's trivial :)), thanks.
> 
> Best regards,
> Tomasz
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Sjoerd Simons Aug. 25, 2014, 8:11 a.m. UTC | #3
Hey,

On Fri, 2014-08-22 at 16:54 -0700, Kevin Hilman wrote:
> Tomasz Figa <tomasz.figa@gmail.com> writes:
> 
> > Kukjin,
> >
> > On 31.07.2014 20:32, Kukjin Kim wrote:
> >> On 07/30/14 17:07, Thomas Abraham wrote:
> >>> The new CPU clock type allows the use of generic CPUfreq drivers. So for
> >>> Exynos4210/5250, switch to using generic cpufreq driver. For Exynos5420,
> >>> which did not have CPUfreq driver support, enable the use of generic
> >>> CPUfreq driver.
> >>>
> >>> Suggested-by: Tomasz Figa<t.figa@samsung.com>
> >>> Cc: Kukjin Kim<kgene.kim@samsung.com>
> >> 
> >> Looks good to me,
> >> 
> >> Acked-by: Kukjin Kim <kgene.kim@samsung.com>
> >> 
> >> BTW, who will handle this series? I hope see this series in 3.17.
> >
> > This series consists mostly of clock changes and it likely depends on
> > patches already in my for-next, so I would be inclined toward taking it
> > through samsung-clk tree. 
> 
> So has this series been picked up anywhere?  I don't see it in your
> samsung-clk tree, nor in Kukjin's for-next.
> 
> Also, I'm curious whether or how this is has been tested on big.LITTLE
> SoCs.  
> 
> I'm trying it on the 5800/Chromebook2 and it's not terribly stable.  I'm
> testing along with CPUidle, so there may be some untested interactions
> there as it seems a bit more stable without CPUidle enabled.
> 
> I'd love to hear from anyone else that's testing CPUidle and CPUfreq
> together big.LITTLE 5420/5800, with or without the switcher.
> 
> Also, the patch below[2] is needed for 5800.

For reference, I had the same patch in a kernel tree we recently used
for a demo on the chromebook 2 13" (Exynos 5800). We didn't see any
stability issues due to this without CPUidle (using the ondemand
govenor). The kernel we ended up using had CONFIG_BL_SWITCHER disabled,
but i don't remember seeing stability issues when i did a testrun with
that enabled.
Chander Kashyap Aug. 25, 2014, 12:15 p.m. UTC | #4
Hi Kevin, Tomasz,

On Sat, Aug 23, 2014 at 5:32 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi Kevin,
>
> Thanks for taking a look at this.
>
> On 23.08.2014 01:54, Kevin Hilman wrote:
>> Tomasz Figa <tomasz.figa@gmail.com> writes:
>>
>>> Kukjin,
>>>
>>> On 31.07.2014 20:32, Kukjin Kim wrote:
>>>> On 07/30/14 17:07, Thomas Abraham wrote:
>>>>> The new CPU clock type allows the use of generic CPUfreq drivers. So for
>>>>> Exynos4210/5250, switch to using generic cpufreq driver. For Exynos5420,
>>>>> which did not have CPUfreq driver support, enable the use of generic
>>>>> CPUfreq driver.
>>>>>
>>>>> Suggested-by: Tomasz Figa<t.figa@samsung.com>
>>>>> Cc: Kukjin Kim<kgene.kim@samsung.com>
>>>>
>>>> Looks good to me,
>>>>
>>>> Acked-by: Kukjin Kim <kgene.kim@samsung.com>
>>>>
>>>> BTW, who will handle this series? I hope see this series in 3.17.
>>>
>>> This series consists mostly of clock changes and it likely depends on
>>> patches already in my for-next, so I would be inclined toward taking it
>>> through samsung-clk tree.
>>
>> So has this series been picked up anywhere?  I don't see it in your
>> samsung-clk tree, nor in Kukjin's for-next.
>
> No, it has not. In general it was already too late in the release cycle
> when the last version was posted.
>
> I had a plan to take it through clock tree with Kukjin's and Viresh's
> cooperation, but now as you say it...
>
>>
>> Also, I'm curious whether or how this is has been tested on big.LITTLE
>> SoCs.
>>
>> I'm trying it on the 5800/Chromebook2 and it's not terribly stable.  I'm
>> testing along with CPUidle, so there may be some untested interactions
>> there as it seems a bit more stable without CPUidle enabled.
>>
>> I'd love to hear from anyone else that's testing CPUidle and CPUfreq
>> together big.LITTLE 5420/5800, with or without the switcher.

I have tested this patch series on SMDK5420 with cpuidle (with and
without b.L switcher enabled).

As of now voltage scaling support is not there in generic big-little
cpufreq driver (arm_big_little.c).
Hence need to tie arm and kfc voltages to highest level for testing.

Without this change stability issues are there, but with this change
everything is stable.

>
> I'd definitely like to see a clarification on this issues, before this
> series hits mainline or at least its parts related to affected SoCs.
> Also I'd like to hear some confirmation from Samsung Poland R&D Center
> guys (on CC), whether this code works stable on their target boards
> (Universal C210, Trats, Trats2).
>
>>
>> Also, the patch below[2] is needed for 5800.
>>
>> FWIW, I have a temporary branch[1] based on the v3.17-rc branch of the
>> exynos-reference tree where I've added the DT patch needed for CPUidle,
>> this series (and it's dependencies) which is what I'm using for testing.
>
> The patch looks fine to me (well, it's trivial :)), thanks.
>
> Best regards,
> Tomasz
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

regards,
Chander
Kevin Hilman Aug. 25, 2014, 3:32 p.m. UTC | #5
Hi Chander,

Chander Kashyap <k.chander@samsung.com> writes:

[...]

>>> I'm trying it on the 5800/Chromebook2 and it's not terribly stable.  I'm
>>> testing along with CPUidle, so there may be some untested interactions
>>> there as it seems a bit more stable without CPUidle enabled.
>>>
>>> I'd love to hear from anyone else that's testing CPUidle and CPUfreq
>>> together big.LITTLE 5420/5800, with or without the switcher.
>
> I have tested this patch series on SMDK5420 with cpuidle (with and
> without b.L switcher enabled).
>
> As of now voltage scaling support is not there in generic big-little
> cpufreq driver (arm_big_little.c).
> Hence need to tie arm and kfc voltages to highest level for testing.

> Without this change stability issues are there, but with this change
> everything is stable.

Can you clarify how you're setting the voltages to ensure stability?

Tomasz, I didn't mean to suggest this isn't ready for mainline.  For the
5420/5800 it seems cpufreq support is a new feature, so this isn't a
regression against previous (mainline) behavior.  Maybe the big.LITTLE
cpufreq support should've been separated out from the cleanup since it's
more of a new feature, but that's up to you.

Kevin
Tomasz Figa Aug. 25, 2014, 3:56 p.m. UTC | #6
On 25.08.2014 17:32, Kevin Hilman wrote:
> Hi Chander,
> 
> Chander Kashyap <k.chander@samsung.com> writes:
> 
> [...]
> 
>>>> I'm trying it on the 5800/Chromebook2 and it's not terribly stable.  I'm
>>>> testing along with CPUidle, so there may be some untested interactions
>>>> there as it seems a bit more stable without CPUidle enabled.
>>>>
>>>> I'd love to hear from anyone else that's testing CPUidle and CPUfreq
>>>> together big.LITTLE 5420/5800, with or without the switcher.
>>
>> I have tested this patch series on SMDK5420 with cpuidle (with and
>> without b.L switcher enabled).
>>
>> As of now voltage scaling support is not there in generic big-little
>> cpufreq driver (arm_big_little.c).
>> Hence need to tie arm and kfc voltages to highest level for testing.
> 
>> Without this change stability issues are there, but with this change
>> everything is stable.
> 
> Can you clarify how you're setting the voltages to ensure stability?
> 
> Tomasz, I didn't mean to suggest this isn't ready for mainline.

I haven't said that either. I'd just like to know in what state this
series is in case of those SoCs. However, if there are stability issues
on them, there is also a chance that the same is true for other boards.

Anyway, we're early in releasy cycle, so probably we could get better
test coverage with this series in linux-next.

Kukjin, Viresh, how would you want to proceed with merging it? It
touches mach-exynos, cpufreq and samsung-clk, so it is non-trivial to
merge. However as far as I can see the cpufreq-related changes are just
a number of full file deletes and minor Makefile/Kconfig updates. It
will be more difficult with mach-exynos changes, as they are more likely
to produce conflict.

The only solution that comes to my mind is that I first apply patches 1
and 2, create a stable branch for Kukjin, then he applies patches 3, 4,
and 5 and creates a stable branch for me, on top of which I apply patch 6.

Best regards,
Tomasz
Viresh Kumar Aug. 26, 2014, 4:54 a.m. UTC | #7
On 25 August 2014 21:26, Tomasz Figa <t.figa@samsung.com> wrote:
> Kukjin, Viresh, how would you want to proceed with merging it? It

Me and Rafael has agreed earlier that Kukjin can take these through
ARM-Soc tree..
diff mbox

Patch

diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c
index 8923d37c3e85..debe50bf736a 100644
--- a/arch/arm/mach-exynos/exynos.c
+++ b/arch/arm/mach-exynos/exynos.c
@@ -283,6 +283,7 @@  static void __init exynos_init_irq(void)
 
 static const struct of_device_id exynos_cpufreq_matches[] = {
 	{ .compatible = "samsung,exynos5420", .data = "arm-bL-cpufreq-dt" },
+	{ .compatible = "samsung,exynos5800", .data = "arm-bL-cpufreq-dt" },
 	{ .compatible = "samsung,exynos5250", .data = "cpufreq-cpu0" },
 	{ .compatible = "samsung,exynos4210", .data = "cpufreq-cpu0" },
 	{ .compatible = "samsung,exynos5440", .data = "exynos5440-cpufreq" },