Hi Tomasz, Thanks for the review. On Fri, May 2, 2014 at 10:53 PM, Tomasz Figa <tomasz.figa@xxxxxxxxx> wrote: > Hi Arun, Alim, > > > On 02.05.2014 15:03, Arun Kumar K wrote: >> >> From: Alim Akhtar <alim.akhtar@xxxxxxxxxxx> >> >> Exynos5800 clock structure is mostly similar to 5420 with only >> a small delta changes. So the 5420 clock file is re-used for >> 5800 also. The common clocks for both are seggreagated and few >> clocks which are different for both are separately initialized. > > > Let's start with a general comment to this patch first. Have you consulted > this with Shaik and Rahul (on CC) who are currently doing a quite heavy > lifting of this driver to make sure that your work don't cause conflicts? > Yes I am aware of that series and I can post another version which is based on that series if that is almost ready to be taken. > Also please see some more specific comments inline. > > >> >> Signed-off-by: Alim Akhtar <alim.akhtar@xxxxxxxxxxx> >> Signed-off-by: Arun Kumar K <arun.kk@xxxxxxxxxxx> >> --- >> .../devicetree/bindings/clock/exynos5420-clock.txt | 3 +- >> drivers/clk/samsung/clk-exynos5420.c | 192 >> +++++++++++++++----- >> include/dt-bindings/clock/exynos5420.h | 1 + >> 3 files changed, 150 insertions(+), 46 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/clock/exynos5420-clock.txt >> b/Documentation/devicetree/bindings/clock/exynos5420-clock.txt >> index ca88c97..d54f42c 100644 >> --- a/Documentation/devicetree/bindings/clock/exynos5420-clock.txt >> +++ b/Documentation/devicetree/bindings/clock/exynos5420-clock.txt >> @@ -1,12 +1,13 @@ >> * Samsung Exynos5420 Clock Controller >> >> The Exynos5420 clock controller generates and supplies clock to various >> -controllers within the Exynos5420 SoC. >> +controllers within the Exynos5420 SoC and for the Exynos5800 SoC. >> >> Required Properties: >> >> - compatible: should be one of the following. >> - "samsung,exynos5420-clock" - controller compatible with Exynos5420 >> SoC. >> + - "samsung,exynos5800-clock" - controller compatible with Exynos5800 >> SoC. >> >> - reg: physical base address of the controller and length of memory >> mapped >> region. >> diff --git a/drivers/clk/samsung/clk-exynos5420.c >> b/drivers/clk/samsung/clk-exynos5420.c >> index 60b2681..0543cb7 100644 >> --- a/drivers/clk/samsung/clk-exynos5420.c >> +++ b/drivers/clk/samsung/clk-exynos5420.c >> @@ -51,6 +51,7 @@ >> #define SRC_TOP5 0x10214 >> #define SRC_TOP6 0x10218 >> #define SRC_TOP7 0x1021c >> +#define SRC_TOP9 0x10224 /* 5800 specific */ >> #define SRC_DISP10 0x1022c >> #define SRC_MAU 0x10240 >> #define SRC_FSYS 0x10244 >> @@ -59,6 +60,7 @@ >> #define SRC_TOP10 0x10280 >> #define SRC_TOP11 0x10284 >> #define SRC_TOP12 0x10288 >> +#define SRC_TOP13 0x1028c /* 5800 specific */ >> #define SRC_MASK_DISP10 0x1032c >> #define SRC_MASK_FSYS 0x10340 >> #define SRC_MASK_PERIC0 0x10350 >> @@ -66,6 +68,7 @@ >> #define DIV_TOP0 0x10500 >> #define DIV_TOP1 0x10504 >> #define DIV_TOP2 0x10508 >> +#define DIV_TOP9 0x10524 /* 5800 specific */ >> #define DIV_DISP10 0x1052c >> #define DIV_MAU 0x10544 >> #define DIV_FSYS0 0x10548 >> @@ -102,8 +105,14 @@ >> #define SRC_KFC 0x28200 >> #define DIV_KFC0 0x28500 >> >> +/* Exynos5x SoC type */ >> +enum exynos5x_soc { >> + EXYNOS5420, >> + EXYNOS5800, >> +}; >> + >> /* list of PLLs */ >> -enum exynos5420_plls { >> +enum exynos5x_plls { >> apll, cpll, dpll, epll, rpll, ipll, spll, vpll, mpll, >> bpll, kpll, >> nr_plls /* number of PLLs */ >> @@ -112,13 +121,13 @@ enum exynos5420_plls { >> static void __iomem *reg_base; >> >> #ifdef CONFIG_PM_SLEEP >> -static struct samsung_clk_reg_dump *exynos5420_save; >> +static struct samsung_clk_reg_dump *exynos5x_save; >> >> /* >> * list of controller registers to be saved and restored during a >> * suspend/resume cycle. >> */ >> -static unsigned long exynos5420_clk_regs[] __initdata = { >> +static unsigned long exynos5x_clk_regs[] __initdata = { >> SRC_CPU, >> DIV_CPU0, >> DIV_CPU1, >> @@ -182,16 +191,16 @@ static unsigned long exynos5420_clk_regs[] >> __initdata = { >> >> static int exynos5420_clk_suspend(void) >> { >> - samsung_clk_save(reg_base, exynos5420_save, >> - ARRAY_SIZE(exynos5420_clk_regs)); >> + samsung_clk_save(reg_base, exynos5x_save, >> + ARRAY_SIZE(exynos5x_clk_regs)); > > > Don't you need to handle save and restore of 5800-specific registers as > well? You can see Exynos4 clock driver for an example of handling such > setup. > Ok will make the change. > >> >> return 0; >> } >> >> static void exynos5420_clk_resume(void) >> { >> - samsung_clk_restore(reg_base, exynos5420_save, >> - ARRAY_SIZE(exynos5420_clk_regs)); >> + samsung_clk_restore(reg_base, exynos5x_save, >> + ARRAY_SIZE(exynos5x_clk_regs)); > > > Ditto. > > >> } >> >> static struct syscore_ops exynos5420_clk_syscore_ops = { >> @@ -201,9 +210,9 @@ static struct syscore_ops exynos5420_clk_syscore_ops = >> { >> >> static void exynos5420_clk_sleep_init(void) >> { >> - exynos5420_save = samsung_clk_alloc_reg_dump(exynos5420_clk_regs, >> - ARRAY_SIZE(exynos5420_clk_regs)); >> - if (!exynos5420_save) { >> + exynos5x_save = samsung_clk_alloc_reg_dump(exynos5x_clk_regs, >> + ARRAY_SIZE(exynos5x_clk_regs)); >> + if (!exynos5x_save) { >> pr_warn("%s: failed to allocate sleep save data, no sleep >> support!\n", >> __func__); >> return; >> @@ -296,13 +305,29 @@ PNAME(hdmi_p) = { "dout_hdmi_pixel", >> "sclk_hdmiphy" }; >> PNAME(maudio0_p) = { "fin_pll", "maudio_clk", "sclk_dpll", >> "sclk_mpll", >> "sclk_spll", "sclk_ipll", "sclk_epll", >> "sclk_rpll" }; >> >> +/* List of parents specific to exynos5800 */ >> +PNAME(mout_epll2_5800_p) = { "mout_sclk_epll", "ffactor_dout_epll2" >> }; >> +PNAME(mout_group1_5800_p) = { "mout_sclk_cpll", "mout_sclk_dpll", >> + "mout_sclk_mpll", "ffactor_dout_spll2" }; >> +PNAME(mout_group3_5800_p) = { "mout_sclk_cpll", "mout_sclk_dpll", >> + "mout_sclk_mpll", >> "ffactor_dout_spll2", >> + "mout_epll2" }; >> +PNAME(mout_group5_5800_p) = { "mout_sclk_cpll", "mout_sclk_dpll", >> + "mout_sclk_mpll", "mout_sclk_spll" >> }; >> +PNAME(mout_group6_5800_p) = { "mout_sclk_ipll", "mout_sclk_dpll", >> + "mout_sclk_mpll", "ffactor_dout_spll2" }; >> +PNAME(mout_group8_5800_p) = { "dout_aclk432_scaler", "dout_sclk_sw" >> }; >> +PNAME(mout_group9_5800_p) = { "dout_osc_div", >> "mout_sw_aclk432_scaler" }; >> + >> /* fixed rate clocks generated outside the soc */ >> -static struct samsung_fixed_rate_clock exynos5420_fixed_rate_ext_clks[] >> __initdata = { >> +static struct >> +samsung_fixed_rate_clock exynos5x_fixed_rate_ext_clks[] __initdata = { > > > This is kind of strange way of wrapping long lines. I'd say that at least > struct should be in the same line as samsung_fixed_rate_clock, so that could > be at least kind of readable. > Ok will do that. > >> FRATE(CLK_FIN_PLL, "fin_pll", NULL, CLK_IS_ROOT, 0), >> }; >> >> /* fixed rate clocks generated inside the soc */ >> -static struct samsung_fixed_rate_clock exynos5420_fixed_rate_clks[] >> __initdata = { >> +static struct >> +samsung_fixed_rate_clock exynos5x_fixed_rate_clks[] __initdata = { > > > Ditto. > > >> FRATE(CLK_SCLK_HDMIPHY, "sclk_hdmiphy", NULL, CLK_IS_ROOT, >> 24000000), >> FRATE(0, "sclk_pwi", NULL, CLK_IS_ROOT, 24000000), >> FRATE(0, "sclk_usbh20", NULL, CLK_IS_ROOT, 48000000), >> @@ -310,39 +335,88 @@ static struct samsung_fixed_rate_clock >> exynos5420_fixed_rate_clks[] __initdata = >> FRATE(0, "sclk_usbh20_scan_clk", NULL, CLK_IS_ROOT, 480000000), >> }; >> >> -static struct samsung_fixed_factor_clock exynos5420_fixed_factor_clks[] >> __initdata = { >> +static struct >> +samsung_fixed_factor_clock exynos5x_fixed_factor_clks[] __initdata = { > > > Ditto (and the same for other numerous instances of this below). > > >> FFACTOR(0, "sclk_hsic_12m", "fin_pll", 1, 2, 0), >> }; >> >> -static struct samsung_mux_clock exynos5420_mux_clks[] __initdata = { >> - MUX(0, "mout_mspll_kfc", mspll_cpu_p, SRC_TOP7, 8, 2), >> - MUX(0, "mout_mspll_cpu", mspll_cpu_p, SRC_TOP7, 12, 2), >> - MUX(0, "mout_apll", apll_p, SRC_CPU, 0, 1), >> - MUX(0, "mout_cpu", cpu_p, SRC_CPU, 16, 1), >> - MUX(0, "mout_kpll", kpll_p, SRC_KFC, 0, 1), >> - MUX(0, "mout_cpu_kfc", kfc_p, SRC_KFC, 16, 1), >> +static struct >> +samsung_fixed_factor_clock exynos5800_fixed_factor_clks[] __initdata = { >> + FFACTOR(0, "ffactor_dout_epll2", "mout_sclk_epll", 1, 2, 0), >> + FFACTOR(0, "ffactor_dout_spll2", "mout_sclk_spll", 1, 2, 0), > > > I don't think you need the "ffactor_" prefix in the name. I assume > "dout_epll2" and "dout_spll2" are the names listed in the datasheet. Is that > correct? > Yes. I will drop the ffactor_ prefix. > Best regards, > Tomasz -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html