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@xxxxxxxxx>
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@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.
>
> 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
--
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
