Hi Arun,
> Hi Lukasz,
>
> Thank you for the review.
No problem.
>
>
> On Mon, Dec 9, 2013 at 1:53 PM, Lukasz Majewski
> <l.majewski@xxxxxxxxxxx> wrote:
> > Hi Arun,
> >
> >> From: "Arjun.K.V" <arjun.kv@xxxxxxxxxxx>
> >>
> >> The patch adds cpufreq driver for exynos5420.
> >>
> >> Signed-off-by: Arjun.K.V <arjun.kv@xxxxxxxxxxx>
> >> Signed-off-by: Andrew Bresticker <abrestic@xxxxxxxxxxxx>
> >> Signed-off-by: Arun Kumar K <arun.kk@xxxxxxxxxxx>
> >> ---
> >> drivers/cpufreq/Kconfig.arm | 11 ++
> >> drivers/cpufreq/Makefile | 1 +
> >> drivers/cpufreq/exynos-cpufreq.c | 2 +
> >> drivers/cpufreq/exynos-cpufreq.h | 8 +
> >> drivers/cpufreq/exynos5420-cpufreq.c | 346
> >> ++++++++++++++++++++++++++++++++++ 5 files changed, 368
> >> insertions(+) create mode 100644
> >> drivers/cpufreq/exynos5420-cpufreq.c
> >
> > I think that we shall cleanup "a bit" the cpufreq code for exynos.
> > Now we have [*]:
> > - exynos4x12-cpufreq.c
> > - exynos4210-cpufreq.c
> > - exynos5250-cpufreq.c
> >
> > and you want to add exynos5420-cpufreq.c
> >
> > Those files are pretty similar, so are a very good candidates for
> > cleanup.
> >
>
> Yes thats right. There is a lot of common code now in these files.
>
> > All supported processors (up to exynos5250) allows for frequency
> > setting on all cores in the SoC.
> >
> > Those files [*] can be easily replaced by cpu0-cpufreq.c generic
> > code. Also we can provide frequency, voltage table and ASV if
> > needed via device tree.
> >
> > I did some preliminary work on this for Exynos4412. My plan is to
> > continue when things with BOOST "calm down" :-).
> >
>
> Ok I will wait for your patches then :)
>
> [snip]
>
> >> +static unsigned int exynos5420_volt_table[CPUFREQ_NUM_LEVELS];
> >> +static struct cpufreq_frequency_table exynos5420_freq_table[] = {
> >> + {L0, 2000 * 1000},
> >> + {L1, 1900 * 1000},
> >> + {L2, 1800 * 1000},
> >> + {L3, 1700 * 1000},
> >> + {L4, 1600 * 1000},
> >> + {L5, 1500 * 1000},
> >> + {L6, 1400 * 1000},
> >> + {L7, 1300 * 1000},
> >> + {L8, 1200 * 1000},
> >> + {L9, 1100 * 1000},
> >> + {L10, 1000 * 1000},
> >> + {L11, 900 * 1000},
> >> + {L12, 800 * 1000},
> >> + {L13, 700 * 1000},
> >> + {L14, 600 * 1000},
> >> + {L15, 500 * 1000},
> >> + {L16, 400 * 1000},
> >> + {L17, 300 * 1000},
> >> + {L18, 200 * 1000},
> >> + {0, CPUFREQ_TABLE_END},
> >> +};
> >
> > This shall be provided via device tree.
> >
>
> Ok.
>
> >> +
> >> +static struct cpufreq_clkdiv
> >> exynos5420_clkdiv_table[CPUFREQ_NUM_LEVELS]; +
> >> +static unsigned int clkdiv_cpu0_5420[CPUFREQ_NUM_LEVELS][7] = {
> >> + /*
> >> + * Clock divider values for {CPUD, ATB, PCLK_DBG, APLL,
> >> ARM2}
> >> + */
> >> + { 2, 7, 7, 3, 0 }, /* ARM L0: 2.0GHz */
> >> + { 2, 7, 7, 3, 0 }, /* ARM L1: 1.9GHz */
> >> + { 2, 7, 7, 3, 0 }, /* ARM L2: 1.8GHz */
> >> + { 2, 7, 7, 3, 0 }, /* ARM L3: 1.7GHz */
> >> + { 2, 7, 7, 3, 0 }, /* ARM L4: 1.6GHz */
> >> + { 2, 7, 7, 3, 0 }, /* ARM L5: 1.5GHz */
> >> + { 2, 7, 7, 3, 0 }, /* ARM L6: 1.4GHz */
> >> + { 2, 7, 7, 3, 0 }, /* ARM L7: 1.3GHz */
> >> + { 2, 7, 7, 3, 0 }, /* ARM L8: 1.2GHz */
> >> + { 2, 7, 7, 3, 0 }, /* ARM L9: 1.1GHz */
> >> + { 2, 6, 6, 3, 0 }, /* ARM L10: 1GHz */
> >> + { 2, 6, 6, 3, 0 }, /* ARM L11: 900MHz */
> >> + { 2, 5, 5, 3, 0 }, /* ARM L12: 800MHz */
> >> + { 2, 5, 5, 3, 0 }, /* ARM L13: 700MHz */
> >> + { 2, 4, 4, 3, 0 }, /* ARM L14: 600MHz */
> >> + { 2, 3, 3, 3, 0 }, /* ARM L15: 500MHz */
> >> + { 2, 3, 3, 3, 0 }, /* ARM L16: 400MHz */
> >> + { 2, 3, 3, 3, 0 }, /* ARM L17: 300MHz */
> >> + { 2, 3, 3, 3, 0 }, /* ARM L18: 200MHz */
> >> +};
> >
> > This table is not needed (as similar ones in this patch), since the
> > Common Clock Framework (and more specific the PLL code for Exynos)
> > shall handle this.
> >
>
> Actually these values are not for PLL, but for the dividers.
> If you see below, the PLL rate setting is done through clk_set_rate()
> going via CCF. But I found an issue if the divider values are set via
> clk_set_rate API.
> What I found is, the system goes into freeze if all the divider
> values are not set in one shot. So we cannot call multiple
> clk_set_rate()'s on each divider. Thats why I continued with non-CCF
> way of setting the divider.
I see. I'm not an expert on common clock framework (CCF), but for me
conceptually clock dividers setting shall be handled by CCF.
However, I've poked a bit at CCF. There isn't any out of the box
solutions available.
A "virtual" clock can be defined and inside its implementation we can
atomically set dividers. Another solution would be to hack the current
CCF to provide atomic clock operations
> Are you taking care of this requirement
> in your restructuring?
I would like to avoid any "bare" messing with registers. Probably we
would need to extend the CCF framework.
>
> >> +
> >> +unsigned int clkdiv_cpu1_5420[CPUFREQ_NUM_LEVELS][2] = {
> >> + /* Clock divider values for { copy, HPM } */
> >> + { 7, 7 }, /* ARM L0: 2.0GHz */
> >> + { 7, 7 }, /* ARM L1: 1.9GHz */
> >> + { 7, 7 }, /* ARM L2: 1.8GHz */
> >> + { 7, 7 }, /* ARM L3: 1.7GHz */
> >> + { 7, 7 }, /* ARM L4: 1.6GHz */
> >> + { 7, 7 }, /* ARM L5: 1.5GHz */
> >> + { 7, 7 }, /* ARM L6: 1.4GHz */
> >> + { 7, 7 }, /* ARM L7: 1.3GHz */
> >> + { 7, 7 }, /* ARM L8: 1.2GHz */
> >> + { 7, 7 }, /* ARM L9: 1.1GHz */
> >> + { 7, 7 }, /* ARM L10: 1GHz */
> >> + { 7, 7 }, /* ARM L11: 900MHz */
> >> + { 7, 7 }, /* ARM L12: 800MHz */
> >> + { 7, 7 }, /* ARM L13: 700MHz */
> >> + { 7, 7 }, /* ARM L14: 600MHz */
> >> + { 7, 7 }, /* ARM L15: 500MHz */
> >> + { 7, 7 }, /* ARM L16: 400MHz */
> >> + { 7, 7 }, /* ARM L17: 300MHz */
> >> + { 7, 7 }, /* ARM L18: 200MHz */
> >> +};
> >> +
> >> +/*
> >> + * Default ASV table
> >> + */
> >> +static const unsigned int asv_voltage_5420[CPUFREQ_NUM_LEVELS] = {
> >> + 1300000, /* L0 2000 */
> >> + 1300000, /* L1 1900 */
> >> + 1200000, /* L2 1800 */
> >> + 1200000, /* L3 1700 */
> >> + 1200000, /* L4 1600 */
> >> + 1175000, /* L5 1500 */
> >> + 1150000, /* L6 1400 */
> >> + 1125000, /* L7 1300 */
> >> + 1100000, /* L8 1200 */
> >> + 1075000, /* L9 1100 */
> >> + 1050000, /* L10 1000 */
> >> + 1000000, /* L11 900 */
> >> + 950000, /* L12 800 */
> >> + 925000, /* L13 700 */
> >> + 900000, /* L14 600 */
> >> + 900000, /* L15 500 */
> >> + 900000, /* L16 400 */
> >> + 900000, /* L17 300 */
> >> + 900000, /* L18 200 */
> >> +};
> >> +
> >
> > If I remember correctly, some code for ASV generic framework has
> > been posted recently (by Sachin):
> >
> > http://article.gmane.org/gmane.linux.power-management.general/40453/match=asv+framework
> >
>
> Yes that will be incorporated with ASV support enabled. Even without
> ASV enabled, a default
> table can be provided with safe operating voltages. If ASV is enabled,
> these values wont be used.
I think that generic ASV voltage table shall be provided from device
tree. It shall be parsed on the cpufreq code.
As you pointed out - default values would be overwritten when ASV
framework is enabled.
>
> >> +static void exynos5420_set_clkdiv(unsigned int div_index)
> >> +{
> >> + unsigned int tmp;
> >> +
> >> + /* Change Divider - CPU0 for CMU_CPU */
> >> + tmp = exynos5420_clkdiv_table[div_index].clkdiv;
> >> + __raw_writel(tmp, EXYNOS5_CLKDIV_CPU0);
> >> +
> >> + do {
> >> + cpu_relax();
> >> + tmp = __raw_readl(EXYNOS5_CLKDIV_STATCPU0);
> >> + } while (tmp & EXYNOS5_CLKDIV_STATCPU0_MASK);
> >> + pr_debug("DIV_CPU0[0x%x]\n",
> >> __raw_readl(EXYNOS5_CLKDIV_CPU0)); +
> >> + /* Change Divider - CPU1 for CMU_CPU */
> >> + tmp = exynos5420_clkdiv_table[div_index].clkdiv1;
> >> + __raw_writel(tmp, EXYNOS5_CLKDIV_CPU1);
> >> +
> >> + do {
> >> + cpu_relax();
> >> + tmp = __raw_readl(EXYNOS5_CLKDIV_STATCPU1);
> >> + } while (tmp & EXYNOS5_CLKDIV_STATCPU1_MASK);
> >> + pr_debug("DIV_CPU1[0x%x]\n",
> >> __raw_readl(EXYNOS5_CLKDIV_CPU1)); +}
> >> +
> >
> > Operations on raw registers - behind the Common Clock Framework is
> > asking for a trouble. I had a similar issues. Please refer to
> > appropriate Exynos4412/4210 patches.
> >
> > http://article.gmane.org/gmane.linux.kernel/1575500/match=cpufreq
> >
>
> As mentioned earlier, I couldnt avoid doing it for divider values
> setting.
>
> Regards
> Arun
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
--
Best regards,
Lukasz Majewski
Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
--
To unsubscribe from this list: send the line "unsubscribe cpufreq" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
