Re: [PATCH v2] clk/axs10x: introduce AXS10X pll driver

[Vlad Zakharov]

Hi Stephen,

On Tue, 2017-04-04 at 18:35 -0700, Stephen Boyd wrote:
> > +ÂÂÂÂÂ.pll_table = (struct pll_of_table []){
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂ{
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ.prate = 27000000,
> 
> Can this be another clk in the framework instead of hardcoding
> the parent rate?

In fact there is another clk in the framework that represents this parent clock. But this field is needed to get
appropriate pll_cfg_table as it depends on parent clock frequency. Below in pll_cfg_get function we are searching for
the correct table comparing .parent_node field with real hardware parent clock frequency:
---------------------------------->8------------------------------------
for (i = 0; pll_table[i].prate != 0; i++)
  if (pll_table[i].prate == prate)
    return pll_table[i].pll_cfg_table;
---------------------------------->8------------------------------------

> 
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ.pll_cfg_table = (struct pll_cfg []){
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ{ 25200000, 1, 84, 90 },
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ{ 50000000, 1, 100, 54 },
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ{ 74250000, 1, 44, 16 },
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ{ },
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ},
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂ},
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂ/* Used as list limiter */
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂ{ },
> 
> There's only ever one, so I'm confused why we're making a list.

By this patch we only add support of core arc pll and pgu pll and today they are clocked by the only parent clocks
introduced here. But other plls on axs10x may be driven by different or configurable clocks, so in such cases we will
have more than one entry in this list. And we are going to add more supported plls to this driver in the nearest future.

> > +
> > +ÂÂÂÂÂclk = clk_register(NULL, &pll_clk->hw);
> > +ÂÂÂÂÂif (IS_ERR(clk)) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂpr_err("failed to register %s clock (%ld)\n",
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂnode->name, PTR_ERR(clk));
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂkfree(pll_clk);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂreturn;
> > +ÂÂÂÂÂ}
> > +
> > +ÂÂÂÂÂof_clk_add_provider(node, of_clk_src_simple_get, clk);
> 
> Can you please use the clk_hw based provider and clk registration
> functions?

Sure. Could you be so kind to explain what is the difference between hw and non-hw based provider and clk registration
functions please? In which cases they are preferred?Â

> 
> > +}
> > +
> > +CLK_OF_DECLARE(axs10x_pll_clock, "snps,axs10x-arc-pll-clock", of_pll_clk_setup);
> 
> Does this need to be CLK_OF_DECLARE_DRIVER? I mean does the
> driver need to probe and also have this of declare happen? Is the
> PLL special and needs to be used for the timers?

It is special and is used for the timers, so we have to CLK_OF_DECLARE it. On the other hand similar pll is used to
drive PGU clock frequency and other subsystems and so we add usual probe func.

-- 
Best regards,
Vlad Zakharov <vzakhar@xxxxxxxxxxxx>