| Message ID | CAKew6eVUGzLweLsQkYx-VGxbzWjUbROk0_63djSsYyH17tbGvg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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@chromium.org> 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
On Thu, Jun 13, 2013 at 3:00 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > 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@chromium.org> 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. > I think we may also have to make assumption that entries in the arrays rate_tables_24mhz[] and samsung_plls[] should be in same order in both arrays, and which may not be fair assumption, otherwise we have to use some mechanism to identify which rate_table is for which PLL, which will increase code and complexity. Am I missed something or you are thinking something else? Any thoughts from Doug or others ? >> /* 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. > I think code lines or complexity will be same. Also all PLLs may not have same parent. > 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. > Sorry, I didn't get your point. You are talking about which two arrays? Regards, Yadwinder > 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
On Friday 14 of June 2013 00:05:31 Yadwinder Singh Brar wrote: > On Thu, Jun 13, 2013 at 3:00 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > > 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@chromium.org> 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. > > I think we may also have to make assumption that entries in the arrays > rate_tables_24mhz[] and samsung_plls[] should be in same order in > both arrays, and which may not be fair assumption, otherwise we > have to use some mechanism to identify which rate_table is for which > PLL, which will increase code and complexity. > Am I missed something or you are thinking something else? Yes, this is exactly what I thought. The order and size of rate_tables_24mhz[] would have to be the same as of samsung_plls[], which shouldn't be a problem technically, but adds another responsibility to the person who defines them. > Any thoughts from Doug or others ? > > >> /* 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. > > I think code lines or complexity will be same. > Also all PLLs may not have same parent. Most of them usually do. Anyway, they can be grouped by the parent, e.g. all that have fin_pll as input can use one set of arrays and other can have their own set. > > 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. > > Sorry, I didn't get your point. You are talking about which two arrays? I mean that the code would need to assume that samsung_plls[] and rate_tables_24mhz[] have the same sizes (and orders), which is not a perfect solution, but I can't think of anything better at the moment. Best regards, Tomasz > Regards, > Yadwinder > > > 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
On Fri, Jun 14, 2013 at 12:13 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > On Friday 14 of June 2013 00:05:31 Yadwinder Singh Brar wrote: >> On Thu, Jun 13, 2013 at 3:00 PM, Tomasz Figa <tomasz.figa@gmail.com> > wrote: >> > 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@chromium.org> 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. >> >> I think we may also have to make assumption that entries in the arrays >> rate_tables_24mhz[] and samsung_plls[] should be in same order in >> both arrays, and which may not be fair assumption, otherwise we >> have to use some mechanism to identify which rate_table is for which >> PLL, which will increase code and complexity. >> Am I missed something or you are thinking something else? > > Yes, this is exactly what I thought. The order and size of > rate_tables_24mhz[] would have to be the same as of samsung_plls[], which > shouldn't be a problem technically, but adds another responsibility to the > person who defines them. > OK. but I think this is not a fair assumption. >> Any thoughts from Doug or others ? >> >> >> /* 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. >> >> I think code lines or complexity will be same. >> Also all PLLs may not have same parent. > > Most of them usually do. Anyway, they can be grouped by the parent, e.g. > all that have fin_pll as input can use one set of arrays and other can > have their own set. > Yes, but number of if() statements to handle them and overall lines in file, will be same(rather more) as compare to existing approach. >> > 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. >> >> Sorry, I didn't get your point. You are talking about which two arrays? > > I mean that the code would need to assume that > > samsung_plls[] and rate_tables_24mhz[] > > have the same sizes (and orders), which is not a perfect solution, but I > can't think of anything better at the moment. > Yes, that's what in my mind also, we will need an extra array of pointers to rate_tables.. Also I can't see any considerable advantage of this approach over existing(proposed) approach. So at the moment, If anybody don't have any problem, I would like to adopt the existing(simpler) approach. Regards, Yadwinder. > Best regards, > Tomasz > >> Regards, >> Yadwinder >> >> > 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
--- 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); + /* 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)); + 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