| Message ID | 7hbnrc6px3.fsf@paris.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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.
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
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
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
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 --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" },