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