Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1626564 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] clk/axs10x: introduce AXS10X pll driver "sboyd@codeaurora.org" <sboyd@codeaurora.org> - 2017-04-19 19:00 +0200
    Re: [PATCH v2] clk/axs10x: introduce AXS10X pll driver Vlad Zakharov <Vladislav.Zakharov@synopsys.com> - 2017-04-20 17:20 +0200

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

On 04/05, Vlad Zakharov wrote:
> 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------------------------------------

When is that done though? During round_rate and recalc_rate the
parent frequency is passed into the function, so it should be
possible to use that if the tree is properly expressed.

> 
> > 
> > > +                     .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.

Ok.

> 
> > > +
> > > +     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? 
> 

We're trying to split the consumer and provider APIs along struct
clk_hw and struct clk respectively. If we can have drivers only
registers clk_hw pointers and never get back anything but an
error code, then we can force consumers to always go through the
clk_get() family of APIs. Then we can easily tell who is a
provider, who is a consumer, and who is a provider + a consumer.
Right now this isn't always clear cut because clk_hw has access
to struct clk, and also clk_register() returns a clk pointer, but
it doesn't really get used by anything in a provider driver,
unless provider drivers are doing something with the consumer
API.

> > 
> > > +}
> > > +
> > > +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.
> 

Presumably we'll have different compatible strings for the
different PLLs then? CLK_OF_DECLARE() will make it so that the
device node that matches never gets a ->probe() from a
platform_driver called on it. If you want it to be called twice,
then you need to use CLK_OF_DECLARE_DRIVER() instead.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [next] | [standalone]

#1627585

Hi Stephen,

On Wed, 2017-04-19 at 09:49 -0700, sboyd@codeaurora.org wrote:
> On 04/05, Vlad Zakharov wrote:
> > 
> > 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------------------------------------
> 
> When is that done though? During round_rate and recalc_rate the
> parent frequency is passed into the function, so it should be
> possible to use that if the tree is properly expressed.
> 

I think that we haven't understood each other correctly.
Anyway I have checked out our hardware documentations and find out that in fact for today's version of hardware we don't
have situations when such plls can be driven by different parent clock. So this approach is not required at the moment.
I am going to simplify my driver so it will reflect current version of hardware and if anything changes I will better
provide a new patch.

> > 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? 
> > 
> 
> We're trying to split the consumer and provider APIs along struct
> clk_hw and struct clk respectively. If we can have drivers only
> registers clk_hw pointers and never get back anything but an
> error code, then we can force consumers to always go through the
> clk_get() family of APIs. Then we can easily tell who is a
> provider, who is a consumer, and who is a provider + a consumer.
> Right now this isn't always clear cut because clk_hw has access
> to struct clk, and also clk_register() returns a clk pointer, but
> it doesn't really get used by anything in a provider driver,
> unless provider drivers are doing something with the consumer
> API.

I am sorry for my foolish questions, but as I understand I should use hw-based functions for clk providers but it is not
still clear enough for me when should I use non-hw functions? Does any drivers need struct clk when they are initiating
or probing themselves? And what clk consumer can be used for?

> 
> > 
> > > 
> > > 
> > > > 
> > > > +}
> > > > +
> > > > +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.
> > 
> 
> Presumably we'll have different compatible strings for the
> different PLLs then? CLK_OF_DECLARE() will make it so that the
> device node that matches never gets a ->probe() from a
> platform_driver called on it. If you want it to be called twice,
> then you need to use CLK_OF_DECLARE_DRIVER() instead.
> 

In fact we don't need it to be called twice. And as you can see I do practically the same things in setup and probe
functions. So maybe I can somehow avoid this code duplication? 
I mean that this driver is going either to be used for the timers or as a simple platform driver for clocking some
peripheras. So for the first case I have written a setup function to CLK_OF_DECLARE my driver and for the second case
I've written usual probe function. Maybe I am mistaken and it is not the right way?

Thank you for you answers. 

-- 
Best regards,
Vlad Zakharov <vzakhar@synopsys.com>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web