On Fri, Jun 14, 2013 at 12:13 AM, Tomasz Figa <tomasz.figa@xxxxxxxxx> 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@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.
>
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
--
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
