Hi Sylwester,
On 10/24/2014 09:03 PM, Sylwester Nawrocki wrote:
> On 24/10/14 13:07, Chanwoo Choi wrote:
>> This patch adds the new clock driver of Exynos4415 SoC based on Cortex-A9
>> using common clock framework. The CMU (Clock Management Unit) of Exynos4415
>> controls PLLs(Phase Locked Loops) and generates system clocks for CPU, buses
>> and function clocks for individual IPs.
>>
>> Cc: Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>
>> Cc: Tomasz Figa <tomasz.figa@xxxxxxxxx>
>> Signed-off-by: Chanwoo Choi <cw00.choi@xxxxxxxxxxx>
>> Signed-off-by: Tomasz Figa <tomasz.figa@xxxxxxxxx>
>> Signed-off-by: Seung-Woo Kim <sw0312.kim@xxxxxxxxxxx>
>> Acked-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
>
> Thanks for the update, there are still couple issues pointed out
> by checkpatch.pl unfortunately, please see below.
> Please fix the errors, I added also some more comments inline below.
> In future please put DT binding documentation patch first in the
> series, before the actual driver patch.
>
> WARNING: kfree(NULL) is safe this check is probably not required
> #252: FILE: drivers/clk/samsung/clk-exynos4415.c:252:
> + if (clk_regs)
> + kfree(clk_regs);
>
> ERROR: space required after that ',' (ctx:VxV)
> #423: FILE: drivers/clk/samsung/clk-exynos4415.c:423:
> + 0,4),
> ^
>
> WARNING: line over 80 characters
> #726: FILE: drivers/clk/samsung/clk-exynos4415.c:726:
> + "div_pxlasync_csis0_fimc", GATE_SCLK_CAM, 10, CLK_SET_RATE_PARENT, 0),
>
> WARNING: line over 80 characters
> #817: FILE: drivers/clk/samsung/clk-exynos4415.c:817:
> + GATE(CLK_SMMUFIMC_LITE2, "smmufimc_lite2", "div_aclk_160", GATE_IP_CAM, 22,
>
> WARNING: line over 80 characters
> #875: FILE: drivers/clk/samsung/clk-exynos4415.c:875:
> + GATE(CLK_USBDEVICE, "usbdevice", "div_aclk_200", GATE_IP_FSYS, 13, 0, 0),
>
> ERROR: space prohibited after that open parenthesis '('
> #920: FILE: drivers/clk/samsung/clk-exynos4415.c:920:
> + PLL_35XX_RATE( 960000000, 320, 4, 1),
>
> ERROR: space prohibited after that open parenthesis '('
> #921: FILE: drivers/clk/samsung/clk-exynos4415.c:921:
> + PLL_35XX_RATE( 900000000, 300, 4, 1),
>
> ERROR: space prohibited after that open parenthesis '('
> #922: FILE: drivers/clk/samsung/clk-exynos4415.c:922:
> + PLL_35XX_RATE( 850000000, 425, 6, 1),
>
> ERROR: space prohibited after that open parenthesis '('
> #923: FILE: drivers/clk/samsung/clk-exynos4415.c:923:
> + PLL_35XX_RATE( 800000000, 200, 3, 1),
>
> ERROR: space prohibited after that open parenthesis '('
> #924: FILE: drivers/clk/samsung/clk-exynos4415.c:924:
> + PLL_35XX_RATE( 700000000, 175, 3, 1),
>
> ERROR: space prohibited after that open parenthesis '('
> #925: FILE: drivers/clk/samsung/clk-exynos4415.c:925:
> + PLL_35XX_RATE( 667000000, 667, 12, 1),
>
> ERROR: space prohibited after that open parenthesis '('
> #926: FILE: drivers/clk/samsung/clk-exynos4415.c:926:
> + PLL_35XX_RATE( 600000000, 400, 4, 2),
>
> ERROR: space prohibited after that open parenthesis '('
> #927: FILE: drivers/clk/samsung/clk-exynos4415.c:927:
> + PLL_35XX_RATE( 550000000, 275, 3, 2),
>
> ERROR: space prohibited after that open parenthesis '('
> #928: FILE: drivers/clk/samsung/clk-exynos4415.c:928:
> + PLL_35XX_RATE( 533000000, 533, 6, 2),
>
> ERROR: space prohibited after that open parenthesis '('
> #929: FILE: drivers/clk/samsung/clk-exynos4415.c:929:
> + PLL_35XX_RATE( 520000000, 260, 3, 2),
>
> ERROR: space prohibited after that open parenthesis '('
> #930: FILE: drivers/clk/samsung/clk-exynos4415.c:930:
> + PLL_35XX_RATE( 500000000, 250, 3, 2),
>
> ERROR: space prohibited after that open parenthesis '('
> #931: FILE: drivers/clk/samsung/clk-exynos4415.c:931:
> + PLL_35XX_RATE( 440000000, 220, 3, 2),
>
> ERROR: space prohibited after that open parenthesis '('
> #932: FILE: drivers/clk/samsung/clk-exynos4415.c:932:
> + PLL_35XX_RATE( 400000000, 200, 3, 2),
>
> ERROR: space prohibited after that open parenthesis '('
> #933: FILE: drivers/clk/samsung/clk-exynos4415.c:933:
> + PLL_35XX_RATE( 350000000, 175, 3, 2),
>
> ERROR: space prohibited after that open parenthesis '('
> #934: FILE: drivers/clk/samsung/clk-exynos4415.c:934:
> + PLL_35XX_RATE( 300000000, 300, 3, 3),
> #935: FILE: drivers/clk/samsung/clk-exynos4415.c:935:
> + PLL_35XX_RATE( 266000000, 266, 3, 3),
>
> ERROR: space prohibited after that open parenthesis '('
> #936: FILE: drivers/clk/samsung/clk-exynos4415.c:936:
> + PLL_35XX_RATE( 200000000, 200, 3, 3),
>
> ERROR: space prohibited after that open parenthesis '('
> #937: FILE: drivers/clk/samsung/clk-exynos4415.c:937:
> + PLL_35XX_RATE( 160000000, 160, 3, 3),
>
> ERROR: space prohibited after that open parenthesis '('
> #938: FILE: drivers/clk/samsung/clk-exynos4415.c:938:
> + PLL_35XX_RATE( 100000000, 200, 3, 4),
>
> ERROR: space prohibited after that open parenthesis '('
> #948: FILE: drivers/clk/samsung/clk-exynos4415.c:948:
> + PLL_36XX_RATE( 96000000, 128, 2, 4, 0),
>
> ERROR: space prohibited after that open parenthesis '('
> #949: FILE: drivers/clk/samsung/clk-exynos4415.c:949:
> + PLL_36XX_RATE( 84000000, 112, 2, 4, 0),
>
> ERROR: space prohibited after that open parenthesis '('
> #950: FILE: drivers/clk/samsung/clk-exynos4415.c:950:
> + PLL_36XX_RATE( 80750011, 107, 2, 4, 43691),
>
> ERROR: space prohibited after that open parenthesis '('
> #951: FILE: drivers/clk/samsung/clk-exynos4415.c:951:
> + PLL_36XX_RATE( 73728004, 98, 2, 4, 19923),
>
> ERROR: space prohibited after that open parenthesis '('
> #952: FILE: drivers/clk/samsung/clk-exynos4415.c:952:
> + PLL_36XX_RATE( 67987602, 271, 3, 5, 62285),
>
> ERROR: space prohibited after that open parenthesis '('
> #953: FILE: drivers/clk/samsung/clk-exynos4415.c:953:
> + PLL_36XX_RATE( 65911004, 175, 2, 5, 49982),
>
> ERROR: space prohibited after that open parenthesis '('
> #954: FILE: drivers/clk/samsung/clk-exynos4415.c:954:
> + PLL_36XX_RATE( 50000000, 200, 3, 5, 0),
>
> ERROR: space prohibited after that open parenthesis '('
> #955: FILE: drivers/clk/samsung/clk-exynos4415.c:955:
> + PLL_36XX_RATE( 49152003, 131, 2, 5, 4719),
>
> ERROR: space prohibited after that open parenthesis '('
> #956: FILE: drivers/clk/samsung/clk-exynos4415.c:956:
> + PLL_36XX_RATE( 48000000, 128, 2, 5, 0),
>
> ERROR: space prohibited after that open parenthesis '('
> #957: FILE: drivers/clk/samsung/clk-exynos4415.c:957:
> + PLL_36XX_RATE( 45250000, 181, 3, 5, 0),
>
> total: 30 errors, 4 warnings, 1133 lines checked
>
> drivers/clk/samsung/clk-exynos4415.c has style problems, please review.
>
>> ---
>> drivers/clk/samsung/Makefile | 1 +
>> drivers/clk/samsung/clk-exynos4415.c | 1133 ++++++++++++++++++++++++++++++++
>> include/dt-bindings/clock/exynos4415.h | 360 ++++++++++
>> 3 files changed, 1494 insertions(+)
>> create mode 100644 drivers/clk/samsung/clk-exynos4415.c
>> create mode 100644 include/dt-bindings/clock/exynos4415.h
>
>> diff --git a/drivers/clk/samsung/clk-exynos4415.c b/drivers/clk/samsung/clk-exynos4415.c
>> new file mode 100644
>> index 0000000..a4b6211
>> --- /dev/null
>> +++ b/drivers/clk/samsung/clk-exynos4415.c
>> @@ -0,0 +1,1133 @@
>
>> +static struct samsung_clk_reg_dump *clk_regs;
>> +static struct samsung_clk_provider *ctx;
>> +
>> +static unsigned long cmu_clk_regs[] __initdata = {
>
>> +};
>> +
>> +static int exynos_clk_suspend(void)
>
> Please prefix such function (and above variable) names with exynos4415,
> as is done for other SoC drivers.
ok.
>
>> +{
>> + samsung_clk_save(ctx->reg_base, clk_regs, ARRAY_SIZE(cmu_clk_regs));
>> +
>> + return 0;
>> +}
>> +
>> +static void exynos_clk_resume(void)
>
> exynos4415_clk_resume ?
ok.
>
>> +{
>> + samsung_clk_restore(ctx->reg_base, clk_regs, ARRAY_SIZE(cmu_clk_regs));
>> +}
>> +
>> +static struct syscore_ops exynos_clk_syscore_ops = {
>
> exynos4415_clk_syscore_ops ?
ok.
>
>> + .suspend = exynos_clk_suspend,
>> + .resume = exynos_clk_resume,
>> +};
>> +
>> +static void exynos_clk_sleep_init(void)
>
> exynos4415_clk_sleep_init ?
>
>> +{
>> + clk_regs = samsung_clk_alloc_reg_dump(cmu_clk_regs,
>> + ARRAY_SIZE(cmu_clk_regs));
>> + if (!clk_regs) {
>> + pr_warn("%s: Failed to allocate sleep save data\n", __func__);
>> + goto err;
>
> You could just return here.
ok.
>
>> + }
>> +
>> + register_syscore_ops(&exynos_clk_syscore_ops);
>> +
>> + return;
>> +err:
>> + if (clk_regs)
>> + kfree(clk_regs);
>
> kfree() can be removed (and the condition could never be true anyway).
You're right. I'll remove it.
>
>> +}
>> +#else
>> +static inline void exynos_clk_sleep_init(void) { }
>
> exynos4415_clk_sleep_init ?
ok.
>
>> +#endif
>
> How about prefixing the table names below with "exynos4415", rather than
> "samsung" ?
'struct samsung_fixed_factor_clock' is common for Exynos SoC.
Do you means that add 'exynos4415' prefix as following:
- fixed_factor_clks -> exynos4415_fixed_factor_clks
>
>> +static struct samsung_fixed_factor_clock fixed_factor_clks[] __initdata = {
>
>> +};
>> +
>> +static struct samsung_fixed_rate_clock fixed_rate_clks[] __initdata = {
>> + FRATE(CLK_SCLK_HDMIPHY, "sclk_hdmiphy", NULL, CLK_IS_ROOT, 27000000),
>> +};
>> +
>> +static struct samsung_mux_clock mux_clks[] __initdata = {
>
>> +};
>> +
>> +static struct samsung_div_clock div_clks[] __initdata = {
>
>> +};
>> +
>> +static struct samsung_gate_clock gate_clks[] __initdata = {
Do you want to change structure naming as following?
fixed_factor_clks -> exynos4415_fixed_factor_clks
fixed_rate_clks -> exynos4415_fixed_rate_clks
mux_clks -> exynos4415_mux_clks
div_clks -> exynos4415_div_clks
gate_clks -> exynos4415_gate_clks
>
>> +};
>> +
>> +/*
>> + * APLL & MPLL & BPLL & ISP_PLL & DISP_PLL & G3D_PLL
>> + */
>> +static struct samsung_pll_rate_table exynos4415_pll_rates[] = {
>> + PLL_35XX_RATE(1600000000, 400, 3, 1),
>> + PLL_35XX_RATE(1500000000, 250, 2, 1),
>> + PLL_35XX_RATE(1400000000, 175, 3, 0),
>> + PLL_35XX_RATE(1300000000, 325, 3, 1),
>> + PLL_35XX_RATE(1200000000, 400, 4, 1),
>> + PLL_35XX_RATE(1100000000, 275, 3, 1),
>> + PLL_35XX_RATE(1066000000, 533, 6, 1),
>> + PLL_35XX_RATE(1000000000, 250, 3, 1),
>> + PLL_35XX_RATE( 960000000, 320, 4, 1),
>> + PLL_35XX_RATE( 900000000, 300, 4, 1),
>> + PLL_35XX_RATE( 850000000, 425, 6, 1),
>> + PLL_35XX_RATE( 800000000, 200, 3, 1),
>> + PLL_35XX_RATE( 700000000, 175, 3, 1),
>> + PLL_35XX_RATE( 667000000, 667, 12, 1),
>> + PLL_35XX_RATE( 600000000, 400, 4, 2),
>> + PLL_35XX_RATE( 550000000, 275, 3, 2),
>> + PLL_35XX_RATE( 533000000, 533, 6, 2),
>> + PLL_35XX_RATE( 520000000, 260, 3, 2),
>> + PLL_35XX_RATE( 500000000, 250, 3, 2),
>> + PLL_35XX_RATE( 440000000, 220, 3, 2),
>> + PLL_35XX_RATE( 400000000, 200, 3, 2),
>> + PLL_35XX_RATE( 350000000, 175, 3, 2),
>> + PLL_35XX_RATE( 300000000, 300, 3, 3),
>> + PLL_35XX_RATE( 266000000, 266, 3, 3),
>> + PLL_35XX_RATE( 200000000, 200, 3, 3),
>> + PLL_35XX_RATE( 160000000, 160, 3, 3),
>> + PLL_35XX_RATE( 100000000, 200, 3, 4),
>> + { /* sentinel */ }
>> +};
>> +
>> +/* EPLL */
>> +static struct samsung_pll_rate_table exynos4415_epll_rates[] = {
>> + PLL_36XX_RATE(800000000, 200, 3, 1, 0),
>> + PLL_36XX_RATE(288000000, 96, 2, 2, 0),
>> + PLL_36XX_RATE(192000000, 128, 2, 3, 0),
>> + PLL_36XX_RATE(144000000, 96, 2, 3, 0),
>> + PLL_36XX_RATE( 96000000, 128, 2, 4, 0),
>> + PLL_36XX_RATE( 84000000, 112, 2, 4, 0),
>> + PLL_36XX_RATE( 80750011, 107, 2, 4, 43691),
>> + PLL_36XX_RATE( 73728004, 98, 2, 4, 19923),
>> + PLL_36XX_RATE( 67987602, 271, 3, 5, 62285),
>> + PLL_36XX_RATE( 65911004, 175, 2, 5, 49982),
>> + PLL_36XX_RATE( 50000000, 200, 3, 5, 0),
>> + PLL_36XX_RATE( 49152003, 131, 2, 5, 4719),
>> + PLL_36XX_RATE( 48000000, 128, 2, 5, 0),
>> + PLL_36XX_RATE( 45250000, 181, 3, 5, 0),
>> + { /* sentinel */ }
>> +};
>> +
>> +static struct samsung_pll_clock plls[nr_plls] __initdata = {
>
>> +};
>> +
>> +static void __init exynos4415_cmu_init(struct device_node *np)
>> +{
>
>> +}
>> +CLK_OF_DECLARE(exynos4415_cmu, "samsung,exynos4415-cmu", exynos4415_cmu_init);
>> +
>> +/*
>> + * CMU DMC
>> + */
>> +
>
>> +#ifdef CONFIG_PM_SLEEP
>
> Similarly for dmc, can we have function, table and below two static
> variable names prefixed with exynos4415, like in exynos3250 driver ?
ok.
>
>> +static struct samsung_clk_reg_dump *dmc_clk_regs;
>> +static struct samsung_clk_provider *dmc_ctx;
>> +
>> +static unsigned long dmc_save_list[] __initdata = {
>
>> +};
>> +
>> +static int dmc_clk_suspend(void)
>> +{
>> + samsung_clk_save(dmc_ctx->reg_base,
>> + dmc_clk_regs, ARRAY_SIZE(dmc_save_list));
>> + return 0;
>> +}
>> +
>> +static void dmc_clk_resume(void)
>> +{
>> + samsung_clk_restore(dmc_ctx->reg_base,
>> + dmc_clk_regs, ARRAY_SIZE(dmc_save_list));
>> +}
>> +
>> +static struct syscore_ops dmc_clk_syscore_ops = {
>> + .suspend = dmc_clk_suspend,
>> + .resume = dmc_clk_resume,
>> +};
>> +
>> +static void dmc_clk_sleep_init(void)
>> +{
>
>> +}
>> +#else
>> +static inline void dmc_clk_sleep_init(void) { }
>> +#endif /* CONFIG_PM_SLEEP */
>> +
>
>> +static struct samsung_mux_clock dmc_mux_clks[] __initdata = {
>
>> +};
>> +
>> +static struct samsung_div_clock dmc_div_clks[] __initdata = {
>
>> +};
>> +
>> +static struct samsung_pll_clock dmc_plls[nr_dmc_plls] __initdata = {
>
>> +};
Best Regards,
Chanwoo Choi
--
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
