Quoting Krzysztof Kozlowski (2015-06-20 03:01:12) > W dniu 19.06.2015 o 23:53, Michael Turquette pisze: > > Quoting Bartlomiej Zolnierkiewicz (2015-06-19 05:35:23) > >> On Friday, June 19, 2015 01:19:06 PM Bartlomiej Zolnierkiewicz wrote: > >>> > >>> Hi, > >>> > >>> On Thursday, June 18, 2015 12:58:46 PM Michael Turquette wrote: > >>>> Quoting Sylwester Nawrocki (2015-05-13 07:13:13) > >>>>> On 03/04/15 18:43, Bartlomiej Zolnierkiewicz wrote: > >>>>>> This flag is needed to fix the issue with wrong dividers being setup > >>>>>> by Common Clock Framework when using the new Exynos cpu clock support. > >>>>>> > >>>>>> The issue happens because clk_core_set_rate_nolock() calls > >>>>>> clk_calc_new_rates(clk, rate) before both pre/post clock notifiers have > >>>>>> a chance to run. In case of Exynos cpu clock support pre/post clock > >>>>>> notifiers are registered for mout_apll clock which is a parent of armclk > >>>>>> cpu clock and dividers are modified in both pre and post clock notifier. > >>>>>> This results in wrong dividers values being later programmed by > >>>>>> clk_change_rate(top). To workaround the problem CLK_RECALC_NEW_RATES > >>>>>> flag is added and it is set for mout_apll clock later so the correct > >>>>>> divider values are re-calculated after both pre and post clock notifiers > >>>>>> had run. > >>>>>> > >>>>>> For example when using "performance" governor on Exynos4210 Origen board > >>>>>> the cpufreq-dt driver requests to change the frequency from 1000MHz to > >>>>>> 1200MHz and after the change state of the relevant clocks is following: > >>>>>> > >>>>>> Without use of CLK_GET_RATE_NOCACHE flag: > >>>>>> > >>>>>> fout_apll rate: 1200000000 > >>>>>> fout_apll_div_2 rate: 600000000 > >>>>>> mout_clkout_cpu rate: 600000000 > >>>>>> div_clkout_cpu rate: 600000000 > >>>>>> clkout_cpu rate: 600000000 > >>>>>> mout_apll rate: 1200000000 > >>>>>> armclk rate: 1200000000 > >>>>>> mout_hpm rate: 1200000000 > >>>>>> div_copy rate: 300000000 > >>>>>> div_hpm rate: 300000000 > >>>>>> mout_core rate: 1200000000 > >>>>>> div_core rate: 1200000000 > >>>>>> div_core2 rate: 1200000000 > >>>>>> arm_clk_div_2 rate: 600000000 > >>>>>> div_corem0 rate: 300000000 > >>>>>> div_corem1 rate: 150000000 > >>>>>> div_periph rate: 300000000 > >>>>>> div_atb rate: 300000000 > >>>>>> div_pclk_dbg rate: 150000000 > >>>>>> sclk_apll rate: 1200000000 > >>>>>> sclk_apll_div_2 rate: 600000000 > >>>>>> > >>>>>> > >>>>>> With use of CLK_GET_RATE_NOCACHE flag: > >>>>>> > >>>>>> fout_apll rate: 1200000000 > >>>>>> fout_apll_div_2 rate: 600000000 > >>>>>> mout_clkout_cpu rate: 600000000 > >>>>>> div_clkout_cpu rate: 600000000 > >>>>>> clkout_cpu rate: 600000000 > >>>>>> mout_apll rate: 1200000000 > >>>>>> armclk rate: 1200000000 > >>>>>> mout_hpm rate: 1200000000 > >>>>>> div_copy rate: 200000000 > >>>>>> div_hpm rate: 200000000 > >>>>>> mout_core rate: 1200000000 > >>>>>> div_core rate: 1200000000 > >>>>>> div_core2 rate: 1200000000 > >>>>>> arm_clk_div_2 rate: 600000000 > >>>>>> div_corem0 rate: 300000000 > >>>>>> div_corem1 rate: 150000000 > >>>>>> div_periph rate: 300000000 > >>>>>> div_atb rate: 240000000 > >>>>>> div_pclk_dbg rate: 120000000 > >>>>>> sclk_apll rate: 150000000 > >>>>>> sclk_apll_div_2 rate: 75000000 > >>>>>> > >>>>>> Without this change cpufreq-dt driver showed ~10 mA larger energy > >>>>>> consumption when compared to cpufreq-exynos one when "performance" > >>>>>> cpufreq governor was used on Exynos4210 SoC based Origen board. > >>>>>> > >>>>>> This issue was probably meant to be workarounded by use of > >>>>>> CLK_GET_RATE_NOCACHE and CLK_DIVIDER_READ_ONLY clock flags in > >>>>>> the original Exynos cpu clock patchset (in "[PATCH v12 6/6] clk: > >>>>>> samsung: remove unused clock aliases and update clock flags" patch) > >>>>>> but usage of these flags is not sufficient to fix the issue observed. > >>>>>> > >>>>>> Cc: Thomas Abraham <thomas.ab@xxxxxxxxxxx> > >>>>>> Cc: Tomasz Figa <tomasz.figa@xxxxxxxxx> > >>>>>> Cc: Mike Turquette <mturquette@xxxxxxxxxx> > >>>>>> Cc: Javier Martinez Canillas <javier.martinez@xxxxxxxxxxxxxxx> > >>>>>> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx> > >>>>>> --- > >>>>>> drivers/clk/clk.c | 3 +++ > >>>>>> include/linux/clk-provider.h | 1 + > >>>>>> 2 files changed, 4 insertions(+) > >>>>>> > >>>>>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > >>>>>> index f85c8e2..97cc73e 100644 > >>>>>> --- a/drivers/clk/clk.c > >>>>>> +++ b/drivers/clk/clk.c > >>>>>> @@ -1771,6 +1771,9 @@ static void clk_change_rate(struct clk_core *clk) > >>>>>> if (clk->notifier_count && old_rate != clk->rate) > >>>>>> __clk_notify(clk, POST_RATE_CHANGE, old_rate, clk->rate); > >>>>>> > >>>>>> + if (clk->flags & CLK_RECALC_NEW_RATES) > >>>>>> + (void)clk_calc_new_rates(clk, clk->new_rate); > >>>>>> + > >>>>>> /* > >>>>>> * Use safe iteration, as change_rate can actually swap parents > >>>>>> * for certain clock types. > >>>>>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > >>>>>> index 28abf1b..8d1aebe 100644 > >>>>>> --- a/include/linux/clk-provider.h > >>>>>> +++ b/include/linux/clk-provider.h > >>>>>> @@ -31,6 +31,7 @@ > >>>>>> #define CLK_GET_RATE_NOCACHE BIT(6) /* do not use the cached clk rate */ > >>>>>> #define CLK_SET_RATE_NO_REPARENT BIT(7) /* don't re-parent on rate change */ > >>>>>> #define CLK_GET_ACCURACY_NOCACHE BIT(8) /* do not use the cached clk accuracy */ > >>>>>> +#define CLK_RECALC_NEW_RATES BIT(9) /* recalc rates after notifications */ > >>>>> > >>>>> Mike, Stephen, what do you think about this? I'm rather resistant to > >>>>> this new flag approach, it looks like a hack. I don't seem to have better > >>>>> ideas to address the missing rate recalculation issue though. > >>>> > >>>> I also do not like it. The root of the problem is the use of clk > >>>> notifiers to change clk rates. This is also a hack and it points towards > >>>> some missing infrastructure in the clock framework. > >>> > >>> I'm very surprised by this. Clock changes using clock notifiers in > >>> Thomas' original patchset were Acked by you: > >>> > >>> "[PATCH v12 1/6] clk: samsung: add infrastructure to register cpu clocks" > >>> https://www.marc.info/?l=linux-arm-kernel&m=141657613203808&w=2 > >>> > >>> I've only fixed issues present within the original code (this 4 lines > >>> workaround/hack change to clock subsystem is a result of this), I have > >>> not changed it fundamentally. > >> > >> Moreover similar changes for rockchip SoCs (which were explicitly based > >> on Thomas' patches as noted in the code!) have already been merged in > >> 3.18: > >> > >> http://lkml.iu.edu/hypermail/linux/kernel/1410.1/02644.html > >> > >> and are available in commit f6fba5f6967dbc062a7c138d67e2314220f5dd04 > >> ("clk: rockchip: add new clock-type for the cpuclk"). > >> > >> I understand that my findings have uncovered some clock subsystem > >> deficiencies which resulted in afterthought about fundamental design > >> of cpu clocks but I have a feeling that our patches are now being > >> unjustly punished for making these issues public. > >> > >> I agree that current patches are not perfect (especially this patch) > >> but they are good enough IMO. Please also understand that there were > >> some serious work put into validating and reviewing them. > > > > You know what? You're right. > > > > I don't really like this cpu-clock code (and similarly I don't like > > Tegra's EMC code which requires access to clk_hw_reparent). But I slept > > on this issue overnight and it doesn't seem right for me to hold back > > these patches when the better solution is currently vaporware (I have > > some code but it's far from ready). > > > > It occurs to me that the best decision I can take is to merge it now and > > then force you guys to switch over when the new infrastructure is > > available. That is more reasonable than delaying the patches getting > > pulled. > > > > So how to merge it? Viresh has given his Ack and is OK for the cpufreq > > changes to go through another tree. I can take the whole thing with > > Kukjin's Ack on the ARM dts patch, or we can set up an immutable > > branch. > > I already acked the DTS [0] patch and I'm fine with it. However there > would be some conflicts if this went through tree different than > Samsung. There are many Exynos4 DTS changes in the queue for 4.2. > > Are there any objections for picking the DTS patch to Samsung tree? That's fine with me. I don't foresee any build problems if I take patches 1-3 and 5-6. So I'll take those and you'll take #4, sound good? Regards, Mike > > Best regards, > Krzysztof > > [0] https://lkml.org/lkml/2015/5/7/999 > > > > > If you really want to make my life easy you can send a pull request for > > these patches (and the other Exynos cpufreq/cpu-clock series). > > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
 |