Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933266AbdDEQGV (ORCPT ); Wed, 5 Apr 2017 12:06:21 -0400 Received: from smtprelay.synopsys.com ([198.182.60.111]:42981 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933074AbdDEQGR (ORCPT ); Wed, 5 Apr 2017 12:06:17 -0400 From: Vlad Zakharov To: "sboyd@codeaurora.org" CC: "mark.rutland@arm.com" , "linux-kernel@vger.kernel.org" , "Jose.Abreu@synopsys.com" , "mturquette@baylibre.com" , "devicetree@vger.kernel.org" , "linux-clk@vger.kernel.org" , "linux-snps-arc@lists.infradead.org" Subject: Re: [PATCH v2] clk/axs10x: introduce AXS10X pll driver Thread-Topic: [PATCH v2] clk/axs10x: introduce AXS10X pll driver Thread-Index: AQHSjEP84oiIqPaSbUyCNi/neN7GXaG2H7yAgADzSQA= Date: Wed, 5 Apr 2017 16:06:11 +0000 Message-ID: <1491408370.9650.24.camel@synopsys.com> References: <1487682670-4164-1-git-send-email-vzakhar@synopsys.com> <20170405013525.GJ18246@codeaurora.org> In-Reply-To: <20170405013525.GJ18246@codeaurora.org> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.121.8.100] Content-Type: text/plain; charset="utf-8" Content-ID: MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id v35G732O023936 Content-Length: 3168 Lines: 68 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