Hi Lukasz,
Thank you for the review.
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.
Are you taking care of this requirement in your restructuring?
>> +
>> +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.
>> +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
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
