On 03/08/2014 03:41 AM, Rahul Sharma wrote:
HI Pankaj, On 7 March 2014 19:26, Pankaj Dubey <pankaj.dubey@xxxxxxxxxxx> wrote:Hi Rahul, On Thu, Mar 6, 2014 at 10:45 PM, Rahul Sharma <rahul.sharma@xxxxxxxxxxx> wrote:Add support for exynos5260 clocks in clock driver. Signed-off-by: Rahul Sharma <rahul.sharma@xxxxxxxxxxx> Signed-off-by: Pankaj Dubey <pankaj.dubey@xxxxxxxxxxx>Even though my signed-off-by is present in this patch I can't see my email-id in cc list. Please check why?Yea this is strange. I always use git send. let me check it. I will also add you manually for next version.[snip]+#include "clk-exynos5260.h" +#include "clk.h" +#include "clk-pll.h" + +#include <dt-bindings/clk/exynos5260-clk.h>Better to move "exynos5260-clk.h" from "dt-bindings/clk" to "dt-bindings/clock" as patch for moving all such headers already landed and looks good also.yea correct. I changed this.[snip]+/* + * Applicable for all 2550 Type PLLS for Exynos5260, listed below + * DISP_PLL, EGL_PLL, KFC_PLL, MEM_PLL, + * BUS_PLL, MEDIA_PLL, G3D_PLL. + */ +static const struct samsung_pll_rate_table pll2550_24mhz_tbl[] = {shouldn't we mark rate_table with __initconst?added.
Not only this, but for all const struct samsung_pll_rate_table in this file.
OK. Apart from removing fixed clock registration related code from clk-exynos5260.c and[snip]+ * Applicable for 2650 Type PLL for AUD_PLL. + */ +static const struct samsung_pll_rate_table pll2650_24mhz_tbl[] = {Ditto. [snip]+#else +static void exynos5260_clk_sleep_init(void) {}This will fail to compile if CONFIG_PM_SLEEP is not defined. Keep function signature same.Done.+#endif + +static struct samsung_clk_provider * +__init exynos5260_cmu_register_one(struct device_node *np, + struct exynos5260_cmu_info *cmu) +{ + void __iomem *reg_base; + struct samsung_clk_provider *ctx; + + reg_base = of_iomap(np, 0); + if (!reg_base) + panic("%s: failed to map registers\n", __func__); + + ctx = samsung_clk_init(np, reg_base, cmu->nr_clk_ids); + if (!ctx) + panic("%s: unable to alllocate ctx\n", __func__); + + if (cmu->pll_clks) + samsung_clk_register_pll(ctx, cmu->pll_clks, cmu->nr_pll_clks, + reg_base); + if (cmu->mux_clks) + samsung_clk_register_mux(ctx, cmu->mux_clks, + cmu->nr_mux_clks); + if (cmu->div_clks) + samsung_clk_register_div(ctx, cmu->div_clks, cmu->nr_div_clks); + if (cmu->gate_clks) + samsung_clk_register_gate(ctx, cmu->gate_clks, + cmu->nr_gate_clks); + if (cmu->clk_regs) + exynos5260_clk_sleep_init(reg_base, cmu->clk_regs, + cmu->nr_clk_regs); + + return ctx;As far as I can see only one user of this return "ctx" is only "exynos5260_clk_top_init", other init functions just ignoring this return value. This can be avoided if we register "fin_pll" (as well as all phyclocks as they are also fixed rate clocks) clock via DT. As I have already done it for another ExynosXXX SoC and it worked for me, on the same hand when today I tried this on Exynos5260, I can see it's working well and I can register "fin_pll" as "fixed_clock" via DT and kernel booted without any issues. Late registration of parent clock does not causing any issues and CCF takes care of that.Hmmn... thats why we are able to chain multiple CMUs :). But I consistently see "divide by zero" asserts during mct init. I will send you the logs offline. If I just ensure that "fin_pll" registers before other CMUs, no problem. I am surprised why you are not getting this issue.If required I can send the changes internally or if you are OK I canIf fix is in same file/dt please highlight those here else; better you post a independent fix. Even UART behaves weird in my setup with IGNORE flag removed.
clk-exynos5260.h (remove FIN_PLL macros renumber other CMU_TOP clock ids),
following changes are required in exynos5260 DT file,
so that fixed clocks can be registered via DT and used.
Changes in exynos5260-xyref5260-evt0.dts
clocks {
compatible = "simple-bus";
#address-cells = <1>;
#size-cells = <0>;
- fin_pll: oscillator@0 {
- compatible = "samsung,exynos5260-oscclk";
+ fin_pll: clock-fin-pll {
+ compatible = "fixed-clock";
+ reg = <0>;
+ #clock-cells = <0>;
clock-frequency = <24000000>;
+ clock-output-names = "fin_pll";
+ };
+
Changes in exynos5260.dtsi
mct: mct@100B0000 {
compatible = "samsung,exynos4210-mct";
reg = <0x100B0000 0x1000>;
- clocks = <&clock_top FIN_PLL>, <&clock_peri PERI_CLK_MCT>;
+ clocks = <&fin_pll>, <&clock_peri PERI_CLK_MCT>;
clock-names = "fin_pll", "mct";
With above changes it's working well for me.
Thanks,
Pankaj Dubey
Rest of the changes are pretty trivial. I should be able to handle this.also upload next version of this patch with this fix, along with addressing all other comments . [snip]+struct samsung_fixed_rate_clock fixed_rate_ext_clks[] __initdata = { + FRATE(FIN_PLL, "fin_pll", NULL, CLK_IS_ROOT, 0), +};In this version you removed other fixed clocks (phyclks and ioclks) but I can not see corresponding DT patches where it has been moved. Or am I missing anything here?I will update DT patch set. Those are already dependent on dt based probe for sysram. I will be updating the next week, probably.[snip]+static void __init exynos5260_clk_top_init(struct device_node *np) +{ + struct exynos5260_cmu_info cmu; + struct samsung_clk_provider *ctx; + + memset(&cmu, 0, sizeof(cmu)); + + cmu.pll_clks = top_pll_clks; + cmu.nr_pll_clks = ARRAY_SIZE(top_pll_clks); + cmu.mux_clks = top_mux_clks; + cmu.nr_mux_clks = ARRAY_SIZE(top_mux_clks); + cmu.div_clks = top_div_clks; + cmu.nr_div_clks = ARRAY_SIZE(top_div_clks); + cmu.gate_clks = top_gate_clks; + cmu.nr_gate_clks = ARRAY_SIZE(top_gate_clks); + cmu.nr_clk_ids = TOP_NR_CLK; + cmu.clk_regs = top_clk_regs; + cmu.nr_clk_regs = ARRAY_SIZE(top_clk_regs); + + ctx = exynos5260_cmu_register_one(np, &cmu); + + samsung_clk_of_register_fixed_ext(ctx, fixed_rate_ext_clks, + ARRAY_SIZE(fixed_rate_ext_clks), + ext_clk_match); +} + +CLK_OF_DECLARE(exynos5260_clk_top, "samsung,exynos5260-clock-top", + exynos5260_clk_top_init);Well with this approach we end up adding 14 such exynosxxx_clk_xxx_init functions all of which has similar lines of code. As I know there are many upcoming Exynos SoC which will also have similar multiple clock controllers (in some of them there are upto 25 clock domains, and in that case we will end up writing 25 such init functions) so I have following suggestion where we can have one more structure which will hold all static data and match_table to match compatibility string and return CMU_TYPE which can be mapped to get proper clock_data which can be used in single clock_init function. Following is some sample code which I have implemented and tested on one of Exynos SoC. Please let me know your opinion about this. ============================= static struct exynosxxxx_clock_data exynosxxxx_clk_data[] __initdata = { { .cmu_type = CMU_TYPE_TOP, .mux_clocks = top_mux_clks, .div_clocks = top_div_clks, .pll_clocks = top_pll_clks, .clk_regs = top_clk_regs, .nr_mux_clocks = ARRAY_SIZE(top_mux_clks), .nr_div_clocks = ARRAY_SIZE(top_div_clks), .nr_pll_clocks = ARRAY_SIZE(top_pll_clks), .nr_clk_regs = ARRAY_SIZE(top_clk_regs), .nr_clks = TOP_NR_CLK, }, { .cmu_type = CMU_TYPE_EGL, .mux_clocks = egl_mux_clks, .div_clocks = egl_div_clks, .pll_clocks = egl_pll_clks, .clk_regs = egl_clk_regs, .nr_mux_clocks = ARRAY_SIZE(egl_mux_clks), .nr_div_clocks = ARRAY_SIZE(egl_div_clks), .nr_pll_clocks = ARRAY_SIZE(egl_pll_clks), .nr_clk_regs = ARRAY_SIZE(egl_clk_regs), .nr_clks = EGL_NR_CLK, }, { /* Add similar elements here */ }; static struct of_device_id cmu_subtype_match_table[] = { { .compatible = "exynosxxxx-cmu-top", .data = (void *)CMU_TYPE_TOP, }, { .compatible = "exynosxxx-cmu-peris", .data = (void *)CMU_TYPE_PERIS, }, { /* Add similar elements here */ }; void __init exynosxxx_clk_init(struct device_node *np) { [snip] match = of_match_node(cmu_subtype_match_table, np); if (!match) panic("%s: cmu type (%s) is not supported.\n", __func__, np->name); reg_base = of_iomap(np, 0); if (!reg_base) panic("%s: failed to map registers\n", __func__); cmu_type = (unsigned long) match->data; for (; i < CMU_TYPE_ALL; i++) { clk_data = &exynosxxxx_clk_data[i]; if (cmu_type == clk_data->cmu_type) break; } ctx = samsung_clk_init(np, reg_base, clk_data->nr_clks); if (!ctx) panic("%s: unable to alllocate ctx\n", __func__); if (clk_data->nr_pll_clocks) samsung_clk_register_pll(ctx, clk_data->pll_clocks, clk_data->nr_pll_clocks, reg_base); if (clk_data->nr_mux_clocks) samsung_clk_register_mux(ctx, clk_data->mux_clocks, clk_data->nr_mux_clocks); if (clk_data->nr_div_clocks) samsung_clk_register_div(ctx, clk_data->div_clocks, clk_data->nr_div_clocks); if (clk_data->nr_gate_clocks) samsung_clk_register_gate(ctx, clk_data->gate_clocks, clk_data->nr_gate_clocks); if (cmu->nr_clk_regs) exynosxxx_clk_sleep_init(reg_base, cmu->clk_regs, cmu->nr_clk_regs); [snip] } CLK_OF_DECLARE(cmu_top, "exynosxxxx-cmu-top", exynosxxxx_clk_init); CLK_OF_DECLARE(cmu_egl, "exynosxxxx-cmu-egl", exynosxxxx_clk_init); /* add more clock domain entries here */ =======================================================diff --git a/drivers/clk/samsung/clk-exynos5260.h b/drivers/clk/samsung/clk-exynos5260.h new file mode 100644 index 0000000..7c3717a --- /dev/null +++ b/drivers/clk/samsung/clk-exynos5260.h @@ -0,0 +1,448 @@ +#ifndef __CLK_EXYNOS5260_H +#define __CLK_EXYNOS5260_H +let me reply this along with Tomasz's comment in next message. Regards, Rahul SharmaThanks, Pankaj Dubey
-- Best Regards, Pankaj Dubey -- 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
