From: Vlad Zakharov <Vladislav.Zakharov@synopsys.com>
To: "sboyd@codeaurora.org" <sboyd@codeaurora.org>
Cc: "mark.rutland@arm.com" <mark.rutland@arm.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"Jose.Abreu@synopsys.com" <Jose.Abreu@synopsys.com>,
"mturquette@baylibre.com" <mturquette@baylibre.com>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>,
"linux-snps-arc@lists.infradead.org"
<linux-snps-arc@lists.infradead.org>
Subject: Re: [PATCH v2] clk/axs10x: introduce AXS10X pll driver
Date: Wed, 5 Apr 2017 16:06:11 +0000 [thread overview]
Message-ID: <1491408370.9650.24.camel@synopsys.com> (raw)
In-Reply-To: <20170405013525.GJ18246@codeaurora.org>
SGkgU3RlcGhlbiwNCg0KT24gVHVlLCAyMDE3LTA0LTA0IGF0IDE4OjM1IC0wNzAwLCBTdGVwaGVu
IEJveWQgd3JvdGU6DQo+ID4gK8KgwqDCoMKgwqAucGxsX3RhYmxlID0gKHN0cnVjdCBwbGxfb2Zf
dGFibGUgW10pew0KPiA+ICvCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoHsNCj4gPiArwqDCoMKg
wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgLnByYXRlID0gMjcwMDAwMDAsDQo+
IA0KPiBDYW4gdGhpcyBiZSBhbm90aGVyIGNsayBpbiB0aGUgZnJhbWV3b3JrIGluc3RlYWQgb2Yg
aGFyZGNvZGluZw0KPiB0aGUgcGFyZW50IHJhdGU/DQoNCkluIGZhY3QgdGhlcmUgaXMgYW5vdGhl
ciBjbGsgaW4gdGhlIGZyYW1ld29yayB0aGF0IHJlcHJlc2VudHMgdGhpcyBwYXJlbnQgY2xvY2su
IEJ1dCB0aGlzIGZpZWxkIGlzIG5lZWRlZCB0byBnZXQNCmFwcHJvcHJpYXRlIHBsbF9jZmdfdGFi
bGUgYXMgaXQgZGVwZW5kcyBvbiBwYXJlbnQgY2xvY2sgZnJlcXVlbmN5LiBCZWxvdyBpbiBwbGxf
Y2ZnX2dldCBmdW5jdGlvbiB3ZSBhcmUgc2VhcmNoaW5nIGZvcg0KdGhlIGNvcnJlY3QgdGFibGUg
Y29tcGFyaW5nIC5wYXJlbnRfbm9kZSBmaWVsZCB3aXRoIHJlYWwgaGFyZHdhcmUgcGFyZW50IGNs
b2NrIGZyZXF1ZW5jeToNCi0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0+OC0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQ0KZm9yIChpID0gMDsgcGxsX3RhYmxlW2ld
LnByYXRlICE9IDA7IGkrKykNCsKgIMKgIGlmIChwbGxfdGFibGVbaV0ucHJhdGUgPT0gcHJhdGUp
DQrCoCDCoCDCoCDCoCByZXR1cm4gcGxsX3RhYmxlW2ldLnBsbF9jZmdfdGFibGU7DQotLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tPjgtLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0NCg0KPiANCj4gPiArwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg
wqDCoMKgLnBsbF9jZmdfdGFibGUgPSAoc3RydWN0IHBsbF9jZmcgW10pew0KPiA+ICvCoMKgwqDC
oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgeyAyNTIw
MDAwMCwgMSwgODQsIDkwIH0sDQo+ID4gK8KgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg
wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqB7IDUwMDAwMDAwLCAxLCAxMDAsIDU0IH0sDQo+ID4g
K8KgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg
wqB7IDc0MjUwMDAwLCAxLCA0NCwgMTYgfSwNCj4gPiArwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg
wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoHsgfSwNCj4gPiArwqDCoMKgwqDCoMKg
wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgfSwNCj4gPiArwqDCoMKgwqDCoMKgwqDCoMKg
wqDCoMKgwqB9LA0KPiA+ICvCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoC8qIFVzZWQgYXMgbGlz
dCBsaW1pdGVyICovDQo+ID4gK8KgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgeyB9LA0KPiANCj4g
VGhlcmUncyBvbmx5IGV2ZXIgb25lLCBzbyBJJ20gY29uZnVzZWQgd2h5IHdlJ3JlIG1ha2luZyBh
IGxpc3QuDQoNCkJ5IHRoaXMgcGF0Y2ggd2Ugb25seSBhZGQgc3VwcG9ydCBvZiBjb3JlIGFyYyBw
bGwgYW5kIHBndSBwbGwgYW5kIHRvZGF5IHRoZXkgYXJlIGNsb2NrZWQgYnkgdGhlIG9ubHkgcGFy
ZW50IGNsb2Nrcw0KaW50cm9kdWNlZCBoZXJlLiBCdXQgb3RoZXIgcGxscyBvbiBheHMxMHggbWF5
IGJlIGRyaXZlbiBieSBkaWZmZXJlbnQgb3IgY29uZmlndXJhYmxlIGNsb2Nrcywgc28gaW4gc3Vj
aCBjYXNlcyB3ZSB3aWxsDQpoYXZlIG1vcmUgdGhhbiBvbmUgZW50cnkgaW4gdGhpcyBsaXN0LiBB
bmQgd2UgYXJlIGdvaW5nIHRvIGFkZCBtb3JlIHN1cHBvcnRlZCBwbGxzIHRvIHRoaXMgZHJpdmVy
IGluIHRoZSBuZWFyZXN0IGZ1dHVyZS4NCg0KPiA+ICsNCj4gPiArwqDCoMKgwqDCoGNsayA9IGNs
a19yZWdpc3RlcihOVUxMLCAmcGxsX2Nsay0+aHcpOw0KPiA+ICvCoMKgwqDCoMKgaWYgKElTX0VS
UihjbGspKSB7DQo+ID4gK8KgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgcHJfZXJyKCJmYWlsZWQg
dG8gcmVnaXN0ZXIgJXMgY2xvY2sgKCVsZClcbiIsDQo+ID4gK8KgwqDCoMKgwqDCoMKgwqDCoMKg
wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqBub2RlLT5uYW1lLCBQVFJfRVJS
KGNsaykpOw0KPiA+ICvCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoGtmcmVlKHBsbF9jbGspOw0K
PiA+ICvCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoHJldHVybjsNCj4gPiArwqDCoMKgwqDCoH0N
Cj4gPiArDQo+ID4gK8KgwqDCoMKgwqBvZl9jbGtfYWRkX3Byb3ZpZGVyKG5vZGUsIG9mX2Nsa19z
cmNfc2ltcGxlX2dldCwgY2xrKTsNCj4gDQo+IENhbiB5b3UgcGxlYXNlIHVzZSB0aGUgY2xrX2h3
IGJhc2VkIHByb3ZpZGVyIGFuZCBjbGsgcmVnaXN0cmF0aW9uDQo+IGZ1bmN0aW9ucz8NCg0KU3Vy
ZS4gQ291bGQgeW91IGJlIHNvIGtpbmQgdG8gZXhwbGFpbiB3aGF0IGlzIHRoZSBkaWZmZXJlbmNl
IGJldHdlZW4gaHcgYW5kIG5vbi1odyBiYXNlZCBwcm92aWRlciBhbmQgY2xrIHJlZ2lzdHJhdGlv
bg0KZnVuY3Rpb25zIHBsZWFzZT8gSW4gd2hpY2ggY2FzZXMgdGhleSBhcmUgcHJlZmVycmVkP8Kg
DQoNCj4gDQo+ID4gK30NCj4gPiArDQo+ID4gK0NMS19PRl9ERUNMQVJFKGF4czEweF9wbGxfY2xv
Y2ssICJzbnBzLGF4czEweC1hcmMtcGxsLWNsb2NrIiwgb2ZfcGxsX2Nsa19zZXR1cCk7DQo+IA0K
PiBEb2VzIHRoaXMgbmVlZCB0byBiZSBDTEtfT0ZfREVDTEFSRV9EUklWRVI/IEkgbWVhbiBkb2Vz
IHRoZQ0KPiBkcml2ZXIgbmVlZCB0byBwcm9iZSBhbmQgYWxzbyBoYXZlIHRoaXMgb2YgZGVjbGFy
ZSBoYXBwZW4/IElzIHRoZQ0KPiBQTEwgc3BlY2lhbCBhbmQgbmVlZHMgdG8gYmUgdXNlZCBmb3Ig
dGhlIHRpbWVycz8NCg0KSXQgaXMgc3BlY2lhbCBhbmQgaXMgdXNlZCBmb3IgdGhlIHRpbWVycywg
c28gd2UgaGF2ZSB0byBDTEtfT0ZfREVDTEFSRSBpdC4gT24gdGhlIG90aGVyIGhhbmQgc2ltaWxh
ciBwbGwgaXMgdXNlZCB0bw0KZHJpdmUgUEdVIGNsb2NrIGZyZXF1ZW5jeSBhbmQgb3RoZXIgc3Vi
c3lzdGVtcyBhbmQgc28gd2UgYWRkIHVzdWFsIHByb2JlIGZ1bmMuDQoNCi0tIA0KQmVzdCByZWdh
cmRzLA0KVmxhZCBaYWtoYXJvdiA8dnpha2hhckBzeW5vcHN5cy5jb20+
WARNING: multiple messages have this Message-ID (diff)
From: Vladislav.Zakharov@synopsys.com (Vlad Zakharov)
To: linux-snps-arc@lists.infradead.org
Subject: [PATCH v2] clk/axs10x: introduce AXS10X pll driver
Date: Wed, 5 Apr 2017 16:06:11 +0000 [thread overview]
Message-ID: <1491408370.9650.24.camel@synopsys.com> (raw)
In-Reply-To: <20170405013525.GJ18246@codeaurora.org>
Hi Stephen,
On Tue, 2017-04-04@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 at synopsys.com>
WARNING: multiple messages have this Message-ID (diff)
From: Vlad Zakharov <Vladislav.Zakharov@synopsys.com>
To: "sboyd@codeaurora.org" <sboyd@codeaurora.org>
Cc: "mark.rutland@arm.com" <mark.rutland@arm.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"Jose.Abreu@synopsys.com" <Jose.Abreu@synopsys.com>,
"mturquette@baylibre.com" <mturquette@baylibre.com>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>,
"linux-snps-arc@lists.infradead.org"
<linux-snps-arc@lists.infradead.org>
Subject: Re: [PATCH v2] clk/axs10x: introduce AXS10X pll driver
Date: Wed, 5 Apr 2017 16:06:11 +0000 [thread overview]
Message-ID: <1491408370.9650.24.camel@synopsys.com> (raw)
In-Reply-To: <20170405013525.GJ18246@codeaurora.org>
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@synopsys.com>
next prev parent reply other threads:[~2017-04-05 16:06 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-02-21 13:11 [PATCH v2] clk/axs10x: introduce AXS10X pll driver Vlad Zakharov
2017-02-21 13:11 ` Vlad Zakharov
2017-03-03 13:18 ` Vlad Zakharov
2017-03-03 13:18 ` Vlad Zakharov
2017-03-03 13:18 ` Vlad Zakharov
2017-03-03 13:18 ` Vlad Zakharov
2017-03-03 23:50 ` Stephen Boyd
2017-03-03 23:50 ` Stephen Boyd
2017-03-03 23:50 ` Stephen Boyd
2017-03-29 11:20 ` Vlad Zakharov
2017-03-29 11:20 ` Vlad Zakharov
2017-03-29 11:20 ` Vlad Zakharov
2017-03-29 11:20 ` Vlad Zakharov
2017-04-03 10:54 ` Jose Abreu
2017-04-03 10:54 ` Jose Abreu
2017-04-03 10:54 ` Jose Abreu
2017-04-05 1:35 ` Stephen Boyd
2017-04-05 1:35 ` Stephen Boyd
2017-04-05 1:35 ` Stephen Boyd
2017-04-05 16:06 ` Vlad Zakharov [this message]
2017-04-05 16:06 ` Vlad Zakharov
2017-04-05 16:06 ` Vlad Zakharov
2017-04-19 16:49 ` sboyd
2017-04-19 16:49 ` sboyd
2017-04-20 15:13 ` Vlad Zakharov
2017-04-20 15:13 ` Vlad Zakharov
2017-04-20 15:13 ` Vlad Zakharov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1491408370.9650.24.camel@synopsys.com \
--to=vladislav.zakharov@synopsys.com \
--cc=Jose.Abreu@synopsys.com \
--cc=devicetree@vger.kernel.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-snps-arc@lists.infradead.org \
--cc=mark.rutland@arm.com \
--cc=mturquette@baylibre.com \
--cc=sboyd@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.