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

[Rahul Sharma]

HI Pankaj,

On 7 March 2014 19:26, Pankaj Dubey <pankaj.dubey at samsung.com> wrote:
> Hi Rahul,
>
> On Thu, Mar 6, 2014 at 10:45 PM, Rahul Sharma <rahul.sharma at samsung.com> wrote:
>> Add support for exynos5260 clocks in clock driver.
>>
>> Signed-off-by: Rahul Sharma <rahul.sharma at samsung.com>
>> Signed-off-by: Pankaj Dubey <pankaj.dubey at samsung.com>
>
> 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.

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

> 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 Sharma

>
> Thanks,
> Pankaj Dubey