Skip to contentSkip to conversation

clk/exynos5260: add clock file for exynos5260

PATCH New Rahul Sharma 4 participants 11 messages Mar 6, 2014 – Mar 10, 2014

Subsystems (3)
Common clk Arm Samsung Exynos Samsung SoC clock drivers
lore
Share & tools
lore

Rahul Sharma

author
#0 lore raw

PATCH v4 5/5 clk/exynos5260: add clock file for exynos5260

Add support for exynos5260 clocks in clock driver.

Signed-off-by Rahul Sharma <[email protected]>
Signed-off-by Pankaj Dubey <[email protected]>
---
 drivers/clk/samsung/Makefile         |    1 +
 drivers/clk/samsung/clk-exynos5260.c | 1890 ++++++++++++++++++++++++++++++++++
 drivers/clk/samsung/clk-exynos5260.h |  448 ++++++++
 3 files changed, 2339 insertions(+)
 create mode 100644 drivers/clk/samsung/clk-exynos5260.c
 create mode 100644 drivers/clk/samsung/clk-exynos5260.h
drivers/clk/samsung/Makefile +1 -0
index 77313e2..56e7d38 100644
@@ -5,6 +5,7 @@
 obj-$(CONFIG_COMMON_CLK)	+= clk.o clk-pll.o
 obj-$(CONFIG_ARCH_EXYNOS4)	+= clk-exynos4.o
 obj-$(CONFIG_SOC_EXYNOS5250)	+= clk-exynos5250.o
+obj-$(CONFIG_SOC_EXYNOS5260)	+= clk-exynos5260.o
 obj-$(CONFIG_SOC_EXYNOS5420)	+= clk-exynos5420.o
 obj-$(CONFIG_SOC_EXYNOS5440)	+= clk-exynos5440.o
 obj-$(CONFIG_ARCH_EXYNOS)	+= clk-exynos-audss.o
drivers/clk/samsung/clk-exynos5260.c new file +1890 -0

1,890 lines not on this page · raw message

drivers/clk/samsung/clk-exynos5260.h new file +448 -0

448 lines not on this page · raw message

Signature · 1 line
1.7.9.5
#1 lore raw

PATCH v4 5/5 clk/exynos5260: add clock file for exynos5260

Hi Rahul,

Quoted 4 lines · Rahul Sharma

Add support for exynos5260 clocks in clock driver.

Signed-off-by Rahul Sharma <[email protected]>
Signed-off-by Pankaj Dubey <[email protected]>

Even though my signed-off-by is present in this patch I can't see my email-id in cc list. Please check why?

[snip]

drivers/clk/samsung/clk-exynos5260.c line 22

Quoted 5 lines
drivers/clk/samsung/clk-exynos5260.c new file +5 -0
+#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.

[snip]

drivers/clk/samsung/clk-exynos5260.c line 59

Quoted 6 lines
drivers/clk/samsung/clk-exynos5260.c new file +6 -0
+/*
+ * 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?

[snip]

Quoted 3 lines

+ * Applicable for 2650 Type PLL for AUD_PLL.
+ */
+static const struct samsung_pll_rate_table pll2650_24mhz_tbl[] = {

Ditto.

[snip]

drivers/clk/samsung/clk-exynos5260.c line 170

Quoted 2 lines
drivers/clk/samsung/clk-exynos5260.c new file +2 -0
+#else
+static void exynos5260_clk_sleep_init(void) {}

This will fail to compile if CONFIG_PM_SLEEP is not defined. Keep function signature same.

Quoted 23 lines
drivers/clk/samsung/clk-exynos5260.c new file +23 -0
+#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);

drivers/clk/samsung/clk-exynos5260.c line 203

Quoted 10 lines
drivers/clk/samsung/clk-exynos5260.c new file +10 -0
+       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. If required I can send the changes internally or if you are OK I can also upload next version of this patch with this fix, along with addressing all other comments .

[snip]

drivers/clk/samsung/clk-exynos5260.c line 1594

Quoted 3 lines
drivers/clk/samsung/clk-exynos5260.c new file +3 -0
+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?

[snip]

Quoted 18 lines
drivers/clk/samsung/clk-exynos5260.c new file +18 -0
+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);

drivers/clk/samsung/clk-exynos5260.c line 1890

Quoted 10 lines
drivers/clk/samsung/clk-exynos5260.c new file +10 -0
+
+       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 */

=======================================================

drivers/clk/samsung/clk-exynos5260.h line 3

Quoted 9 lines
drivers/clk/samsung/clk-exynos5260.h new file +3 -0
new file mode 100644
index 0000000..7c3717a
@@ -0,0 +1,448 @@
+#ifndef __CLK_EXYNOS5260_H
+#define __CLK_EXYNOS5260_H
+

Thanks,
Pankaj Dubey

#2 lore raw

PATCH v4 5/5 clk/exynos5260: add clock file for exynos5260

Hi Pankaj,

Quoted 32 lines · Pankaj Dubey
Quoted 18 lines
drivers/clk/samsung/clk-exynos5260.c new file +18 -0
+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);

drivers/clk/samsung/clk-exynos5260.c line 1890

Quoted 10 lines
drivers/clk/samsung/clk-exynos5260.c new file +10 -0
+
+       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

Quoted 8 lines

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.

Yes, this looks better indeed, however there is still a room for improvement. Please see my comments below.

Quoted 4 lines

=============================

static struct exynosxxxx_clock_data exynosxxxx_clk_data[] __initdata = {

Instead of making this an array, particular elements could be separate structures. This would simplify the code below.

Quoted 26 lines

{
.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 */
};

Quoted 4 lines
static struct of_device_id cmu_subtype_match_table[] = {
         {
                 .compatible = "exynosxxxx-cmu-top",
                 .data   = (void *)CMU_TYPE_TOP,

Here the data would be just a pointer to respective clock data struct defined above.

Quoted 8 lines
         }, {
                 .compatible = "exynosxxx-cmu-peris",
                 .data   = (void *)CMU_TYPE_PERIS,
         }, {
        /* Add similar elements here */
};

void __init exynosxxx_clk_init(struct device_node *np)

Quoted 8 lines
{
          [snip]

         match = of_match_node(cmu_subtype_match_table, np);

         if (!match)
                 panic("%s: cmu type (%s) is not supported.\n", __func__,
                                 np->name);

This can't happen, because this function won't be called for any node with compatible string not declared using CLK_OF_DECLARE().

Quoted 12 lines
         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;
         }

Now clk_data could be taken directly from match->data, without the need to iterate over an array.

Best regards,
Tomasz

#3 lore raw

PATCH v4 5/5 clk/exynos5260: add clock file for exynos5260

Hi Tomasz,

Quoted 111 lines · Tomasz Figa

Hi Pankaj,

Quoted 34 lines · Pankaj Dubey
Quoted 19 lines
drivers/clk/samsung/clk-exynos5260.c new file +18 -0
+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);

drivers/clk/samsung/clk-exynos5260.c line 1890

Quoted 10 lines
drivers/clk/samsung/clk-exynos5260.c new file +10 -0
+
+       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

Quoted 8 lines

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.

Yes, this looks better indeed, however there is still a room for improvement. Please see my comments below.

Quoted 4 lines

=============================

static struct exynosxxxx_clock_data exynosxxxx_clk_data[] __initdata = {

Instead of making this an array, particular elements could be separate structures. This would simplify the code below.

Quoted 26 lines

{
.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 */
};

Quoted 4 lines
static struct of_device_id cmu_subtype_match_table[] = {
         {
                 .compatible = "exynosxxxx-cmu-top",
                 .data   = (void *)CMU_TYPE_TOP,

Here the data would be just a pointer to respective clock data struct defined above.

Quoted 13 lines
         }, {
                 .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);
Quoted 7 lines
Quoted 3 lines
         if (!match)
                 panic("%s: cmu type (%s) is not supported.\n", __func__,
                                 np->name);

This can't happen, because this function won't be called for any node with compatible string not declared using CLK_OF_DECLARE().

Agreed.

Quoted 16 lines
Quoted 14 lines


         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;
         }
Quoted 2 lines

Now clk_data could be taken directly from match->data, without the need to iterate over an array.

Good point. Let me change as per your suggestion and test. And then we will go for this change in next new version of patch. Also please review rest patch also and let us know if anything still can be improved.

Thanks,
Pankaj Dubey

Quoted 9 lines

Best regards,
Tomasz

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

#4 lore raw

PATCH v4 5/5 clk/exynos5260: add clock file for exynos5260

Quoted 141 lines · Pankaj Dubey

Hi Tomasz,

Quoted 111 lines · Tomasz Figa

Hi Pankaj,

Quoted 34 lines · Pankaj Dubey
Quoted 19 lines
drivers/clk/samsung/clk-exynos5260.c new file +18 -0
+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);

drivers/clk/samsung/clk-exynos5260.c line 1890

Quoted 10 lines
drivers/clk/samsung/clk-exynos5260.c new file +10 -0
+
+       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

Quoted 8 lines

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.

Yes, this looks better indeed, however there is still a room for improvement. Please see my comments below.

Quoted 4 lines

=============================

static struct exynosxxxx_clock_data exynosxxxx_clk_data[] __initdata = {

Instead of making this an array, particular elements could be separate structures. This would simplify the code below.

Quoted 26 lines

{
.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 */
};

Quoted 4 lines
static struct of_device_id cmu_subtype_match_table[] = {
          {
                  .compatible = "exynosxxxx-cmu-top",
                  .data   = (void *)CMU_TYPE_TOP,

Here the data would be just a pointer to respective clock data struct defined above.

Quoted 13 lines
          }, {
                  .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);
Quoted 7 lines
Quoted 3 lines
          if (!match)
                  panic("%s: cmu type (%s) is not supported.\n", __func__,
                                  np->name);

This can't happen, because this function won't be called for any node with compatible string not declared using CLK_OF_DECLARE().

Agreed.

Quoted 16 lines
Quoted 14 lines


          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;
          }
Quoted 5 lines
Quoted 2 lines

Now clk_data could be taken directly from match->data, without the need to iterate over an array.

Good point. Let me change as per your suggestion and test. And then we will go for this change in next new version of patch.

OK, thanks.

Quoted 3 lines

Also please
review rest patch also and let us know if anything still can be improved.

I had reviewed version 3 of this series and most of my comments have been addressed. I'm waiting for remaining two to be addressed, but they are covered by your comments for v4.

A link to v3 thread for reference:
http://thread.gmane.org/gmane.linux.kernel.samsung-soc/27249

Best regards,
Tomasz

#5 lore raw

PATCH v4 5/5 clk/exynos5260: add clock file for exynos5260

Hi Tomasz,

Quoted 155 lines · Tomasz Figa
Quoted 154 lines · Pankaj Dubey

Hi Tomasz,

Quoted 124 lines · Tomasz Figa

Hi Pankaj,

Quoted 36 lines · Pankaj Dubey
Quoted 20 lines
drivers/clk/samsung/clk-exynos5260.c new file +18 -0
+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);

drivers/clk/samsung/clk-exynos5260.c line 1890

Quoted 10 lines
drivers/clk/samsung/clk-exynos5260.c new file +10 -0
+
+       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

Quoted 8 lines

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.

Yes, this looks better indeed, however there is still a room for improvement. Please see my comments below.

Quoted 4 lines

=============================

static struct exynosxxxx_clock_data exynosxxxx_clk_data[] __initdata = {

Instead of making this an array, particular elements could be separate structures. This would simplify the code below.

Quoted 26 lines

{
.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 */
};

Quoted 4 lines
static struct of_device_id cmu_subtype_match_table[] = {
          {
                  .compatible = "exynosxxxx-cmu-top",
                  .data   = (void *)CMU_TYPE_TOP,

Here the data would be just a pointer to respective clock data struct defined above.

Quoted 8 lines
          }, {
                  .compatible = "exynosxxx-cmu-peris",
                  .data   = (void *)CMU_TYPE_PERIS,
          }, {
         /* Add similar elements here */
};

void __init exynosxxx_clk_init(struct device_node *np)

Quoted 9 lines
{
           [snip]

          match = of_match_node(cmu_subtype_match_table, np);

          if (!match)
                  panic("%s: cmu type (%s) is not supported.\n",
__func__,
                                  np->name);
Quoted 3 lines

This can't happen, because this function won't be called for any node with
compatible string not declared using CLK_OF_DECLARE().

Agreed.

Quoted 17 lines
Quoted 14 lines


          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;
          }
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines

Now clk_data could be taken directly from match->data, without the need to
iterate over an array.

I am just curious, is it really a worth to replace 14 init functions with 14 same
sized isolated structures. We will also be adding a array of these structure pointers and little overhead to parse this array based on compatible string for each cmu probe.

IMHO, calling correct function meant for the given CMU, looks more straight and crisp. Most of the duplication is already addressed in exynos5260_cmu_register_one.

Please, let me know if we are still looking for this change. It should be easy to
change it that way.

Quoted 8 lines
Quoted 3 lines

Good point. Let me change as per your suggestion and test. And then we will go for this change in next new version of patch.

OK, thanks.

Quoted 8 lines
Quoted 3 lines

Also please
review rest patch also and let us know if anything still can be improved.

I had reviewed version 3 of this series and most of my comments have been addressed. I'm waiting for remaining two to be addressed, but they are covered by your comments for v4.

I will address the fixes for fin_pll and sclk_uart in the next version. Thanks for the
review.

Regards,
Rahul Sharma.

Quoted 6 lines

A link to v3 thread for reference:
http://thread.gmane.org/gmane.linux.kernel.samsung-soc/27249

Best regards,
Tomasz

#6 lore raw

PATCH v4 5/5 clk/exynos5260: add clock file for exynos5260

Hi Rahul,

Quoted 172 lines · Rahul Sharma

Hi Tomasz,

Quoted 142 lines · Tomasz Figa
Quoted 141 lines · Pankaj Dubey

Hi Tomasz,

Quoted 116 lines · Tomasz Figa

Hi Pankaj,

Quoted 34 lines · Pankaj Dubey
Quoted 19 lines
drivers/clk/samsung/clk-exynos5260.c new file +18 -0
+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);

drivers/clk/samsung/clk-exynos5260.c line 1890

Quoted 10 lines
drivers/clk/samsung/clk-exynos5260.c new file +10 -0
+
+       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

Quoted 8 lines

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.

Yes, this looks better indeed, however there is still a room for improvement. Please see my comments below.

Quoted 3 lines

=============================

static struct exynosxxxx_clock_data exynosxxxx_clk_data[] __initdata = {

Instead of making this an array, particular elements could be separate structures. This would simplify the code below.

Quoted 26 lines

{
.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 */
};

Quoted 4 lines
static struct of_device_id cmu_subtype_match_table[] = {
           {
                   .compatible = "exynosxxxx-cmu-top",
                   .data   = (void *)CMU_TYPE_TOP,

Here the data would be just a pointer to respective clock data struct defined above.

Quoted 8 lines
           }, {
                   .compatible = "exynosxxx-cmu-peris",
                   .data   = (void *)CMU_TYPE_PERIS,
           }, {
          /* Add similar elements here */
};

void __init exynosxxx_clk_init(struct device_node *np)

Quoted 9 lines
{
            [snip]

           match = of_match_node(cmu_subtype_match_table, np);

           if (!match)
                   panic("%s: cmu type (%s) is not supported.\n",
__func__,
                                   np->name);
Quoted 3 lines

This can't happen, because this function won't be called for any node with
compatible string not declared using CLK_OF_DECLARE().

Agreed.

Quoted 14 lines
Quoted 12 lines
           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;
           }
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

Now clk_data could be taken directly from match->data, without the need to
iterate over an array.

I am just curious, is it really a worth to replace 14 init functions with 14 same
sized isolated structures. We will also be adding a array of these structure pointers and little overhead to parse this array based on compatible string for each cmu probe.

IMHO, calling correct function meant for the given CMU, looks more straight and crisp. Most of the duplication is already addressed in exynos5260_cmu_register_one.

Please, let me know if we are still looking for this change. It should be easy to
change it that way.

Quoted 10 lines
Quoted 2 lines

Good point. Let me change as per your suggestion and test. And then we will go for this change in next new version of patch.

OK, thanks.

Quoted 3 lines

Also please
review rest patch also and let us know if anything still can be improved.

Quoted 6 lines
Quoted 3 lines

I had reviewed version 3 of this series and most of my comments have been addressed. I'm waiting for remaining two to be addressed, but they are covered by your comments for v4.

I will address the fixes for fin_pll and sclk_uart in the next version. Thanks for the
review.

One update from my end.
Today I verified that even though I removed CLK_IGNORE_UNUSED flag from sclk_uartX I can
see kernel is booting fine and no issues as you mentioned. FYI I am using 3.14_rc1 tag and
applied all patches on top of it. For reference I have mailed all related patches to you.

@Tomasz,

I modified code as per our discussion and addressed all comments including moving fixed clock to DT
and found it's working well, so we can now finalize which way to go ahead. Following is some data between V4 and probable next version, which might help in making decision.

drivers/clk/samsung/exynos5260-clock.c

Before Change
LoC: 1890
size drivers/clk/samsung/clk-exynos5260.o
text data bss dec hex filename
14226 14956 0 29182 71fe
drivers/clk/samsung/clk-exynos5260.o

After Change:
LoC: 1806
size drivers/clk/samsung/clk-exynos5260.o
text data bss dec hex filename
15446 14544 0 29990 7526
drivers/clk/samsung/clk-exynos5260.o

Quoted 9 lines

Regards,
Rahul Sharma.

Quoted 5 lines

A link to v3 thread for reference:
http://thread.gmane.org/gmane.linux.kernel.samsung-soc/27249

Best regards,
Tomasz

Signature · 2 lines
Best Regards,
Pankaj Dubey
#7 lore raw

PATCH v4 5/5 clk/exynos5260: add clock file for exynos5260

Quoted 17 lines · Pankaj Dubey

Hi Rahul,

I modified code as per our discussion and addressed all comments including moving fixed clock to DT
and found it's working well, so we can now finalize which way to go ahead. Following is some data between V4 and probable next version, which might help in making decision.

drivers/clk/samsung/exynos5260-clock.c

Before Change
LoC: 1890
size drivers/clk/samsung/clk-exynos5260.o
text data bss dec hex filename
14226 14956 0 29182 71fe
drivers/clk/samsung/clk-exynos5260.o

Quoted 6 lines

After Change:
LoC: 1806
size drivers/clk/samsung/clk-exynos5260.o
text data bss dec hex filename
15446 14544 0 29990 7526
drivers/clk/samsung/clk-exynos5260.o

Well, somehow I've been under impression that the change would bring more significant results. Here we can see slight reduction in lines of code, but also slight increase of kernel image size. Still, in general, nothing extreme.

In this case I'd probably go with Rahul's original code, as it doesn't require second matching and separate match table, which adds redundant compatible strings used just for this extra matching pass.

I guess this is all just bike shedding, though.

Best regards,
Tomasz

#8 lore raw

PATCH v4 5/5 clk/exynos5260: add clock file for exynos5260

Hi Tomasz,

Quoted 34 lines · Tomasz Figa
Quoted 20 lines · Pankaj Dubey

Hi Rahul,

I modified code as per our discussion and addressed all comments including moving fixed clock to DT
and found it's working well, so we can now finalize which way to go ahead.
Following is some data between V4 and probable next version, which might help in making decision.

drivers/clk/samsung/exynos5260-clock.c

Before Change
LoC: 1890
size drivers/clk/samsung/clk-exynos5260.o
text data bss dec hex filename
14226 14956 0 29182 71fe
drivers/clk/samsung/clk-exynos5260.o

Quoted 6 lines

After Change:
LoC: 1806
size drivers/clk/samsung/clk-exynos5260.o
text data bss dec hex filename
15446 14544 0 29990 7526
drivers/clk/samsung/clk-exynos5260.o

Well, somehow I've been under impression that the change would bring more significant results. Here we can see slight reduction in lines of code, but also slight increase of kernel image size. Still, in general, nothing extreme.

Quoted 5 lines

In this case I'd probably go with Rahul's original code, as it doesn't require second matching and separate match table, which adds redundant compatible strings used just for this extra matching pass.

I guess this is all just bike shedding, though.

yea. I understand. Will proceed as you said.

Regards,
Rahul Sharma

Quoted 3 lines

Best regards,
Tomasz

#9 lore raw

PATCH v4 5/5 clk/exynos5260: add clock file for exynos5260

In reply to Pankaj Dubey

HI Pankaj,

Quoted 10 lines · Pankaj Dubey

Hi Rahul,

Quoted 4 lines · Rahul Sharma

Add support for exynos5260 clocks in clock driver.

Signed-off-by Rahul Sharma <[email protected]>
Signed-off-by Pankaj Dubey <[email protected]>

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.

Quoted 12 lines

[snip]

drivers/clk/samsung/clk-exynos5260.c line 22

Quoted 5 lines
drivers/clk/samsung/clk-exynos5260.c new file +5 -0
+#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.

Quoted 11 lines

[snip]

drivers/clk/samsung/clk-exynos5260.c line 59

Quoted 6 lines
drivers/clk/samsung/clk-exynos5260.c new file +6 -0
+/*
+ * 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.

Quoted 8 lines

[snip]

Quoted 3 lines

+ * Applicable for 2650 Type PLL for AUD_PLL.
+ */
+static const struct samsung_pll_rate_table pll2650_24mhz_tbl[] = {

Ditto.

Quoted 8 lines

[snip]

drivers/clk/samsung/clk-exynos5260.c line 170

Quoted 2 lines
drivers/clk/samsung/clk-exynos5260.c new file +2 -0
+#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.

Quoted 36 lines
Quoted 23 lines
drivers/clk/samsung/clk-exynos5260.c new file +23 -0
+#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);

drivers/clk/samsung/clk-exynos5260.c line 203

Quoted 10 lines
drivers/clk/samsung/clk-exynos5260.c new file +10 -0
+       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

Quoted 8 lines

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.

Quoted 1 line

If required I can send the changes internally or if you are OK I can

If 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.

Rest of the changes are pretty trivial. I should be able to handle this.

Quoted 5 lines

also upload next version of this patch with this fix, along with addressing all other comments .

[snip]

Quoted 7 lines

drivers/clk/samsung/clk-exynos5260.c line 1594

Quoted 3 lines
drivers/clk/samsung/clk-exynos5260.c new file +3 -0
+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.

Quoted 135 lines

[snip]

Quoted 18 lines
drivers/clk/samsung/clk-exynos5260.c new file +18 -0
+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);

drivers/clk/samsung/clk-exynos5260.c line 1890

Quoted 10 lines
drivers/clk/samsung/clk-exynos5260.c new file +10 -0
+
+       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 */

=======================================================

Quoted 10 lines

drivers/clk/samsung/clk-exynos5260.h line 3

Quoted 9 lines
drivers/clk/samsung/clk-exynos5260.h new file +3 -0
new file mode 100644
index 0000000..7c3717a
@@ -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 Sharma

Quoted 3 lines

Thanks,
Pankaj Dubey

#10 lore raw

PATCH v4 5/5 clk/exynos5260: add clock file for exynos5260

Quoted 37 lines · Rahul Sharma

HI Pankaj,

Quoted 9 lines · Pankaj Dubey

Hi Rahul,

Quoted 4 lines · Rahul Sharma

Add support for exynos5260 clocks in clock driver.

Signed-off-by Rahul Sharma <[email protected]>
Signed-off-by Pankaj Dubey <[email protected]>

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.

Quoted 10 lines

[snip]

drivers/clk/samsung/clk-exynos5260.c line 22

Quoted 5 lines
drivers/clk/samsung/clk-exynos5260.c new file +5 -0
+#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.

Quoted 10 lines

[snip]

drivers/clk/samsung/clk-exynos5260.c line 59

Quoted 6 lines
drivers/clk/samsung/clk-exynos5260.c new file +6 -0
+/*
+ * 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?

Quoted 1 line

added.

Not only this, but for all const struct samsung_pll_rate_table in this file.

Quoted 66 lines
Quoted 6 lines

[snip]

Quoted 3 lines

+ * Applicable for 2650 Type PLL for AUD_PLL.
+ */
+static const struct samsung_pll_rate_table pll2650_24mhz_tbl[] = {

Quoted 9 lines

Ditto.

[snip]

drivers/clk/samsung/clk-exynos5260.c line 170

Quoted 2 lines
drivers/clk/samsung/clk-exynos5260.c new file +2 -0
+#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.

Quoted 35 lines
Quoted 23 lines
drivers/clk/samsung/clk-exynos5260.c new file +23 -0
+#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);

drivers/clk/samsung/clk-exynos5260.c line 203

Quoted 10 lines
drivers/clk/samsung/clk-exynos5260.c new file +10 -0
+       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

Quoted 8 lines

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.

Quoted 4 lines
Quoted 1 line

If required I can send the changes internally or if you are OK I can

If 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.

OK. Apart from removing fixed clock registration related code from clk-exynos5260.c and 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

Quoted 166 lines

Rest of the changes are pretty trivial. I should be able to handle this.

Quoted 11 lines

also upload next version of this patch with this fix, along with addressing all other comments .

[snip]

drivers/clk/samsung/clk-exynos5260.c line 1594

Quoted 3 lines
drivers/clk/samsung/clk-exynos5260.c new file +3 -0
+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.

Quoted 133 lines

[snip]

Quoted 18 lines
drivers/clk/samsung/clk-exynos5260.c new file +18 -0
+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);

drivers/clk/samsung/clk-exynos5260.c line 1890

Quoted 10 lines
drivers/clk/samsung/clk-exynos5260.c new file +10 -0
+
+       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 */

=======================================================

Quoted 9 lines

drivers/clk/samsung/clk-exynos5260.h line 3

Quoted 9 lines
drivers/clk/samsung/clk-exynos5260.h new file +3 -0
new file mode 100644
index 0000000..7c3717a
@@ -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 Sharma

Quoted 2 lines

Thanks,
Pankaj Dubey

Signature · 2 lines
Best Regards,
Pankaj Dubey