On Thursday 13 of June 2013 12:32:05 Yadwinder Singh Brar wrote:
> On Thu, Jun 13, 2013 at 3:32 AM, Andrew Bresticker
>
> <abrestic@xxxxxxxxxxxx> wrote:
> > Doug,
> >
> >>> Hmm, if done properly, it could simplify PLL registration in SoC
> >>> clock
> >>> initialization code a lot.
> >>>
> >>> I'm not sure if this is really the best solution (feel free to
> >>> suggest
> >>> anything better), but we could put PLLs in an array, like other
> >>> clocks,
> >>> e.g.
> >>>
> >>> ... exynos4210_pll_clks[] = {
> >>>
> >>> CLK_PLL45XX(...),
> >>> CLK_PLL45XX(...),
> >>> CLK_PLL46XX(...),
> >>> CLK_PLL46XX(...),
> >>>
> >>> };
> >>>
> >>> and then just call a helper like
> >>>
> >>> samsung_clk_register_pll(exynos4210_pll_clks,
> >>>
> >>> ARRAY_SIZE(exynos4210_pll_clks));
> >>
> >> Something like that looks like what I was thinking. I'd have to see
> >> it actually coded up to see if there's something I'm missing that
> >> would prevent us from doing that, but I don't see anything.
> >
> > The only issue I see with this is that we may only want to register a
> > rate table with a PLL only if fin_pll is running at a certain rate.
> > On 5250 and 5420, for example, we have EPLL and VPLL rate tables that
> > should only be registered if fin_pll is 24Mhz. We may have to
> > register those separately, but this approach seems fine otherwise.
>
> As Andrew Bresticker said, we will face problem with different table,
> and it will give some pain while handling such cases but I think
> overall code may look better.
>
> Similar thoughts were their in my mind also, but i didn't want to
> disturb this series :).
Yes, I was thinking the same as well, but now that Exynos5420 doesn't
follow the 0x100 register spacing, we have a problem :) .
> Anyways, I think we can do it now only rather going for incremental
> patches after this series.
> I was thinking to make samsung_clk_register_pllxxxx itself little
> generic instead
> of using helper, as we are almost duplicating code for most PLLs.
>
> A rough picture in my mind was,
> After implementing generic samung_clk_register_pll(), code may look
> like below. Its just an idea, please feel free to correct it.
> Later we can factor out other common clk.ops for PLLs also.
>
> this diff is over this series.
> Assuming a generic samung_clk_register_pll() is their(which i think is
> not impossible)
> 8<----------------------------------------------------------------------
> ----
>
> --- a/drivers/clk/samsung/clk-exynos5250.c
> +++ b/drivers/clk/samsung/clk-exynos5250.c
> @@ -493,6 +493,20 @@ static __initdata struct samsung_pll_rate_table
> epll_24mhz_tbl[] = {
> PLL_36XX_RATE(32768000, 131, 3, 5, 4719),
> };
>
> +struct __initdata samsung_pll_init_data samsung_plls[] = {
> + PLL(pll_3500, "fout_apll", "fin_pll", APLL_LOCK, APLL_CON0,
> NULL), + PLL(pll_3500, "fout_mpll", "fin_pll", MPLL_LOCK,
> MPLL_CON0, NULL), + PLL(pll_3500, "fout_bpll",
> "fin_pll",BPLL_LOCK, BPLL_CON0, NULL), + PLL(pll_3500,
> "fout_gpll", "fin_pll",GPLL_LOCK, GPLL_CON0, NULL), +
> PLL(pll_3500, "fout_cpll", "fin_pll",BPLL_LOCK, CPLL_CON0, NULL), +};
> +
> +struct __initdata samsung_pll_init_data epll_init_data =
> + PLL(pll_3600, "fout_epll", "fin_pll", EPLL_LOCK, EPLL_CON0,
> NULL); +
> +struct __initdata samsung_pll_init_data vpll_init_data =
> + PLL(pll_3600, "fout_epll", "fin_pll", VPLL_LOCK, VPLL_CON0,
> NULL); +
This is mostly what I had in my mind. In addition I think I might have a
solution for rate tables:
If we create another array
struct samsung_pll_rate_table *rate_tables_24mhz[] = {
apll_rate_table_24mhz,
mpll_rate_table_24mhz, /* can be NULL as well, if no
support for rate change */
epll_rate_table_24mhz,
vpll_rate_table_24mhz,
/* ... */
};
which lists rate tables for given input frequency. This relies on making
rate tables end with a sentinel, to remove the need of passing array
sizes.
> /* register exynox5250 clocks */
> void __init exynos5250_clk_init(struct device_node *np)
> {
> @@ -519,44 +533,22 @@ void __init exynos5250_clk_init(struct device_node
> *np) samsung_clk_register_mux(exynos5250_pll_pmux_clks,
> ARRAY_SIZE(exynos5250_pll_pmux_clks));
>
> - fin_pll_rate = _get_rate("fin_pll");
> + samsung_clk_register_pll(samsung_plls,
> ARRAY_SIZE(samsung_plls)); +
...and then pass it here like:
if (fin_pll_rate == 24 * MHZ) {
samsung_clk_register_pll(samsung_plls,
ARRAY_SIZE(samsung_plls), rate_tables_24mhz);
} else {
/* warning or whatever */
samsung_clk_register_pll(samsung_plls,
ARRAY_SIZE(samsung_plls), NULL);
}
This way we could specify different rate tables depending on input
frequency for a whole group of PLLs.
The only thing I don't like here is having two separate arrays that need
to have the same sizes. Feel free to improve (or discard) this idea,
though.
Best regards,
Tomasz
> vpllsrc = __clk_lookup("mout_vpllsrc");
> if (vpllsrc)
> mout_vpllsrc_rate = clk_get_rate(vpllsrc);
>
> - apll = samsung_clk_register_pll35xx("fout_apll", "fin_pll",
> - reg_base, NULL, 0);
> - mpll = samsung_clk_register_pll35xx("fout_mpll", "fin_pll",
> - reg_base + 0x4000, NULL, 0);
> - bpll = samsung_clk_register_pll35xx("fout_bpll", "fin_pll",
> - reg_base + 0x20010, NULL, 0);
> - gpll = samsung_clk_register_pll35xx("fout_gpll", "fin_pll",
> - reg_base + 0x10050, NULL, 0);
> - cpll = samsung_clk_register_pll35xx("fout_cpll", "fin_pll",
> - reg_base + 0x10020, NULL, 0);
> -
> + fin_pll_rate = _get_rate("fin_pll");
> if (fin_pll_rate == (24 * MHZ)) {
> - epll = samsung_clk_register_pll36xx("fout_epll",
> "fin_pll", - reg_base + 0x10030,
> epll_24mhz_tbl, -
> ARRAY_SIZE(epll_24mhz_tbl));
> - } else {
> - pr_warn("%s: valid epll rate_table missing for\n"
> - "parent fin_pll:%lu hz\n", __func__,
> fin_pll_rate); - epll =
> samsung_clk_register_pll36xx("fout_epll", "fin_pll", -
> reg_base + 0x10030, NULL, 0);
> + epll_init_data.rate_table = epll_24mhz_tb;
> }
> + samsung_clk_register_pll(&fout_epll_data, 1);
>
> if (mout_vpllsrc_rate == (24 * MHZ)) {
> - vpll = samsung_clk_register_pll36xx("fout_vpll",
> "mout_vpllsrc" - , reg_base + 0x10040,
> vpll_24mhz_tbl,
> - ARRAY_SIZE(vpll_24mhz_tbl));
> - } else {
> - pr_warn("%s: valid vpll rate_table missing for\n"
> - "parent mout_vpllsrc_rate:%lu hz\n", __func__,
> - mout_vpllsrc_rate);
> - samsung_clk_register_pll36xx("fout_vpll",
> "mout_vpllsrc", - reg_base + 0x10040, NULL, 0);
> + vpll_init_data.rate_table = epll_24mhz_tb;
> }
> + samsung_clk_register_pll(&fout_epll_data, 1);
> samsung_clk_register_fixed_rate(exynos5250_fixed_rate_clks,
> ARRAY_SIZE(exynos5250_fixed_rate_clks));
> diff --git a/drivers/clk/samsung/clk-pll.h
> b/drivers/clk/samsung/clk-pll.h index 4780e6c..3b02dc5 100644
> --- a/drivers/clk/samsung/clk-pll.h
> +++ b/drivers/clk/samsung/clk-pll.h
> @@ -39,6 +39,39 @@ struct samsung_pll_rate_table {
> unsigned int kdiv;
> };
>
> +#define PLL(_type, _name, _pname, _lock_reg, _con_reg, _rate_table)
> \ + {
> \ + .type = _type,
> \ + .name = _name,
> \ + .parent_name = _pname,
> \ + .flags = CLK_GET_RATE_NOCACHE,
> \ + .rate_table = _rate_table,
> \ + .con_reg = _con_reg,
> \ + .lock_reg = _lock_reg,
> \ + }
> +
> +enum samsung_pll_type {
> + pll_3500,
> + pll_45xx,
> + pll_2550,
> + pll_3600,
> + pll_46xx,
> + pll_2660,
> +};
> +
> +
> +struct samsung_pll_init_data {
> + enum samsung_pll_type type;
> + struct samsung_pll_rate_table *rate_table;
> + const char *name;
> + const char *parent_name;
> + unsigned long flags;
> + const void __iomem *con_reg;
> + const void __iomem *lock_reg;
> +};
> +
> +extern int __init samsung_clk_register_pll(struct
> samsung_pll_init_data *data, + unsigned
> int nr_pll);
> +
>
> Regards,
> Yadwinder
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
