From: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
To: "sboyd@codeaurora.org" <sboyd@codeaurora.org>
Cc: "Eugeniy.Paltsev@synopsys.com" <Eugeniy.Paltsev@synopsys.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"mark.rutland@arm.com" <mark.rutland@arm.com>,
"mturquette@baylibre.com" <mturquette@baylibre.com>,
"robh+dt@kernel.org" <robh+dt@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] ARC: clk: introduce HSDKv1 pll driver
Date: Wed, 9 Aug 2017 16:43:49 +0000 [thread overview]
Message-ID: <1502297028.2586.4.camel@synopsys.com> (raw)
In-Reply-To: <20170804015351.GW2146@codeaurora.org>
SGkgU3RlcGhlbizCoA0KdGhhbmtzIGZvciByZXNwb25kLCBteSBjb21tZW50cyBhcmUgaW5saW5l
ZCBiZWxvdy4NCg0KT24gVGh1LCAyMDE3LTA4LTAzIGF0IDE4OjUzIC0wNzAwLCBTdGVwaGVuIEJv
eWQgd3JvdGU6DQo+IE9uIDA3LzE0LCBFdWdlbml5IFBhbHRzZXYgd3JvdGU6DQoNCj4gWy4uLl0N
Cj4gPiArCWRldl9kYmcoY2xrLT5kZXYsICJ3cml0ZSBjb25maWd1cmFyaW9uOiAweCV4IiwgdmFs
KTsNCj7CoA0KPiBPciBqdXN0IHVzZSAlI3ggdG8gYWRkIHRoZSAweCBwYXJ0Lg0KDQpUaGFua3Ms
IEkgZG9uJ3Qga25vdyBhYm91dCB0aGlzIG9wdGlvbi4NCj7CoA0KPiBbLi4uXQ0KPg0KPiA+ICsJ
LyogaW5wdXQgZGl2aWRlciA9IHJlZy5pZGl2ICsgMSAqLw0KPiA+ICsJaWRpdiA9IDEgKyAoKHZh
bCAmIENHVV9QTExfQ1RSTF9JRElWX01BU0spID4+DQo+ID4gQ0dVX1BMTF9DVFJMX0lESVZfU0hJ
RlQpOw0KPiA+ICsJLyogZmIgZGl2aWRlciA9IDIqKHJlZy5mYmRpdiArIDEpICovDQo+ID4gKwlm
YmRpdiA9IDIgKiAoMSArICgodmFsICYgQ0dVX1BMTF9DVFJMX0ZCRElWX01BU0spID4+DQo+ID4g
Q0dVX1BMTF9DVFJMX0ZCRElWX1NISUZUKSk7DQo+ID4gKwkvKiBvdXRwdXQgZGl2aWRlciA9IDJe
KHJlZy5vZGl2KSAqLw0KPiA+ICsJb2RpdiA9IDEgPDwgKCh2YWwgJiBDR1VfUExMX0NUUkxfT0RJ
Vl9NQVNLKSA+Pg0KPiA+IENHVV9QTExfQ1RSTF9PRElWX1NISUZUKTsNCj4NCj4gTWF5YmUganVz
dCBkcm9wIHRoZXNlIGNvbW1lbnRzLiBUaGV5J3JlIGp1c3QgcmVwZWF0aW5nIHRoZSBjb2RlLg0K
DQpBY3R1YWxseSBJIHdvdWxkIHByZWZlciB0byBrZWVwIHRoZW0sIGFzICIyXihyZWcub2Rpdiki
IGlzIG1vcmXCoA0KaHVtYW4tcmVhZGFibGUgdGhlbg0KIjEgPDwgKCh2YWwgJiBDR1VfUExMX0NU
UkxfT0RJVl9NQVNLKSA+PiBDR1VfUExMX0NUUkxfT0RJVl9TSElGVCkiDQoNCj4gPiArDQo+ID4g
KwlyYXRlID0gKHU2NClwYXJlbnRfcmF0ZSAqIGZiZGl2Ow0KPiA+ICsJZG9fZGl2KHJhdGUsIGlk
aXYgKiBvZGl2KTsNCj4gPiArDQo+ID4gKwlyZXR1cm4gcmF0ZTsNCj4gPiArfQ0KPiA+ICsNCj4g
PiArc3RhdGljIGxvbmcgaHNka19wbGxfcm91bmRfcmF0ZShzdHJ1Y3QgY2xrX2h3ICpodywgdW5z
aWduZWQgbG9uZw0KPiA+IHJhdGUsDQo+ID4gKwkJCQl1bnNpZ25lZCBsb25nICpwcmF0ZSkNCj4g
PiArew0KPiA+ICsJaW50IGk7DQo+ID4gKwlsb25nIGJlc3RfcmF0ZTsNCj4gPiArCXN0cnVjdCBo
c2RrX3BsbF9jbGsgKmNsayA9IHRvX2hzZGtfcGxsX2Nsayhodyk7DQo+ID4gKwljb25zdCBzdHJ1
Y3QgaHNka19wbGxfY2ZnICpwbGxfY2ZnID0gY2xrLT5wbGxfY2ZnOw0KPiA+ICsNCj4gPiArCWlm
IChwbGxfY2ZnWzBdLnJhdGUgPT0gMCkNCj4gPiArCQlyZXR1cm4gLUVJTlZBTDsNCj7CoA0KPiBU
aGlzIGhhcHBlbnM/DQoNCk9ubHkgaWYgd2UgYWRkIGJhZCBoc2RrX3BsbF9jZmcgdGFibGUuIEJ1
dCBpdCBpcyBxdWl0ZSBjb2xkIGNvZGUgLSB3ZQ0KY2hhbmdlIHBsbCBjb25maWd1cmF0aW9uIHF1
aXRlIHJhcmUsIHNvIG1heWJlIGl0J3MgYmV0dGVyIHRvIGtlZXAgdGhpcw0KYXNzZXJ0Pw0KDQo+
ID4gKw0KPiA+ICsJYmVzdF9yYXRlID0gcGxsX2NmZ1swXS5yYXRlOw0KPiA+ICsNCj4gPiArCWZv
ciAoaSA9IDE7IHBsbF9jZmdbaV0ucmF0ZSAhPSAwOyBpKyspIHsNCj4gPiArCQlpZiAoYWJzKHJh
dGUgLSBwbGxfY2ZnW2ldLnJhdGUpIDwgYWJzKHJhdGUgLQ0KPiA+IGJlc3RfcmF0ZSkpDQo+wqAN
Cj4gQWxyaWdodCwgcmF0ZSBpcyB1bnNpZ25lZCBsb25nLCBhbmQgYmVzdF9yYXRlIGlzIGxvbmcu
IGFicygpIGRvZXMNCj4gdGhlIHJpZ2h0IHRoaW5nIGhlcmUsIGJ1dCBpdCBtYWtlcyBtZSBoYXZl
IHRvIHRoaW5rIHR3aWNlIGlmDQo+IGJlc3RfcmF0ZSBjYW4gYmUgbmVnYXRpdmUgYW5kIHRoZW4g
dGhpcyBpcyBhIGxhcmdlciBudW1iZXIsIG5vdCBhDQo+IHNtYWxsZXIgb25lLiBDYW4gd2UgbWFr
ZSBiZXN0X3JhdGUgdW5zaWduZWQgbG9uZyBpbiB0aGlzDQo+IGZ1bmN0aW9uPw0KDQpZZXMsIHdl
IGNhbi4NCkFueXdheSBpdCdzIGEgYml0IHN0cmFuZ2Ugd2hhdCByYXRlIGlzIHVuc2lnbmVkIGxv
bmcgYW5kIHJvdW5kX3JhdGUNCnJldHVybiB2YWx1ZSBpcyBsb25nLg0KLS3CoA0KwqBFdWdlbml5
IFBhbHRzZXY=
WARNING: multiple messages have this Message-ID (diff)
From: Eugeniy.Paltsev@synopsys.com (Eugeniy Paltsev)
To: linux-snps-arc@lists.infradead.org
Subject: [PATCH] ARC: clk: introduce HSDKv1 pll driver
Date: Wed, 9 Aug 2017 16:43:49 +0000 [thread overview]
Message-ID: <1502297028.2586.4.camel@synopsys.com> (raw)
In-Reply-To: <20170804015351.GW2146@codeaurora.org>
Hi Stephen,?
thanks for respond, my comments are inlined below.
On Thu, 2017-08-03@18:53 -0700, Stephen Boyd wrote:
> On 07/14, Eugeniy Paltsev wrote:
> [...]
> > + dev_dbg(clk->dev, "write configurarion: 0x%x", val);
>?
> Or just use %#x to add the 0x part.
Thanks, I don't know about this option.
>?
> [...]
>
> > + /* input divider = reg.idiv + 1 */
> > + idiv = 1 + ((val & CGU_PLL_CTRL_IDIV_MASK) >>
> > CGU_PLL_CTRL_IDIV_SHIFT);
> > + /* fb divider = 2*(reg.fbdiv + 1) */
> > + fbdiv = 2 * (1 + ((val & CGU_PLL_CTRL_FBDIV_MASK) >>
> > CGU_PLL_CTRL_FBDIV_SHIFT));
> > + /* output divider = 2^(reg.odiv) */
> > + odiv = 1 << ((val & CGU_PLL_CTRL_ODIV_MASK) >>
> > CGU_PLL_CTRL_ODIV_SHIFT);
>
> Maybe just drop these comments. They're just repeating the code.
Actually I would prefer to keep them, as "2^(reg.odiv)" is more?
human-readable then
"1 << ((val & CGU_PLL_CTRL_ODIV_MASK) >> CGU_PLL_CTRL_ODIV_SHIFT)"
> > +
> > + rate = (u64)parent_rate * fbdiv;
> > + do_div(rate, idiv * odiv);
> > +
> > + return rate;
> > +}
> > +
> > +static long hsdk_pll_round_rate(struct clk_hw *hw, unsigned long
> > rate,
> > + unsigned long *prate)
> > +{
> > + int i;
> > + long best_rate;
> > + struct hsdk_pll_clk *clk = to_hsdk_pll_clk(hw);
> > + const struct hsdk_pll_cfg *pll_cfg = clk->pll_cfg;
> > +
> > + if (pll_cfg[0].rate == 0)
> > + return -EINVAL;
>?
> This happens?
Only if we add bad hsdk_pll_cfg table. But it is quite cold code - we
change pll configuration quite rare, so maybe it's better to keep this
assert?
> > +
> > + best_rate = pll_cfg[0].rate;
> > +
> > + for (i = 1; pll_cfg[i].rate != 0; i++) {
> > + if (abs(rate - pll_cfg[i].rate) < abs(rate -
> > best_rate))
>?
> Alright, rate is unsigned long, and best_rate is long. abs() does
> the right thing here, but it makes me have to think twice if
> best_rate can be negative and then this is a larger number, not a
> smaller one. Can we make best_rate unsigned long in this
> function?
Yes, we can.
Anyway it's a bit strange what rate is unsigned long and round_rate
return value is long.
--?
?Eugeniy Paltsev
WARNING: multiple messages have this Message-ID (diff)
From: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
To: "sboyd@codeaurora.org" <sboyd@codeaurora.org>
Cc: "Eugeniy.Paltsev@synopsys.com" <Eugeniy.Paltsev@synopsys.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"mark.rutland@arm.com" <mark.rutland@arm.com>,
"mturquette@baylibre.com" <mturquette@baylibre.com>,
"robh+dt@kernel.org" <robh+dt@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] ARC: clk: introduce HSDKv1 pll driver
Date: Wed, 9 Aug 2017 16:43:49 +0000 [thread overview]
Message-ID: <1502297028.2586.4.camel@synopsys.com> (raw)
In-Reply-To: <20170804015351.GW2146@codeaurora.org>
Hi Stephen,
thanks for respond, my comments are inlined below.
On Thu, 2017-08-03 at 18:53 -0700, Stephen Boyd wrote:
> On 07/14, Eugeniy Paltsev wrote:
> [...]
> > + dev_dbg(clk->dev, "write configurarion: 0x%x", val);
>
> Or just use %#x to add the 0x part.
Thanks, I don't know about this option.
>
> [...]
>
> > + /* input divider = reg.idiv + 1 */
> > + idiv = 1 + ((val & CGU_PLL_CTRL_IDIV_MASK) >>
> > CGU_PLL_CTRL_IDIV_SHIFT);
> > + /* fb divider = 2*(reg.fbdiv + 1) */
> > + fbdiv = 2 * (1 + ((val & CGU_PLL_CTRL_FBDIV_MASK) >>
> > CGU_PLL_CTRL_FBDIV_SHIFT));
> > + /* output divider = 2^(reg.odiv) */
> > + odiv = 1 << ((val & CGU_PLL_CTRL_ODIV_MASK) >>
> > CGU_PLL_CTRL_ODIV_SHIFT);
>
> Maybe just drop these comments. They're just repeating the code.
Actually I would prefer to keep them, as "2^(reg.odiv)" is more
human-readable then
"1 << ((val & CGU_PLL_CTRL_ODIV_MASK) >> CGU_PLL_CTRL_ODIV_SHIFT)"
> > +
> > + rate = (u64)parent_rate * fbdiv;
> > + do_div(rate, idiv * odiv);
> > +
> > + return rate;
> > +}
> > +
> > +static long hsdk_pll_round_rate(struct clk_hw *hw, unsigned long
> > rate,
> > + unsigned long *prate)
> > +{
> > + int i;
> > + long best_rate;
> > + struct hsdk_pll_clk *clk = to_hsdk_pll_clk(hw);
> > + const struct hsdk_pll_cfg *pll_cfg = clk->pll_cfg;
> > +
> > + if (pll_cfg[0].rate == 0)
> > + return -EINVAL;
>
> This happens?
Only if we add bad hsdk_pll_cfg table. But it is quite cold code - we
change pll configuration quite rare, so maybe it's better to keep this
assert?
> > +
> > + best_rate = pll_cfg[0].rate;
> > +
> > + for (i = 1; pll_cfg[i].rate != 0; i++) {
> > + if (abs(rate - pll_cfg[i].rate) < abs(rate -
> > best_rate))
>
> Alright, rate is unsigned long, and best_rate is long. abs() does
> the right thing here, but it makes me have to think twice if
> best_rate can be negative and then this is a larger number, not a
> smaller one. Can we make best_rate unsigned long in this
> function?
Yes, we can.
Anyway it's a bit strange what rate is unsigned long and round_rate
return value is long.
--
Eugeniy Paltsev
next prev parent reply other threads:[~2017-08-09 16:43 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-14 15:31 [PATCH] ARC: clk: introduce HSDKv1 pll driver Eugeniy Paltsev
2017-07-14 15:31 ` Eugeniy Paltsev
2017-07-27 7:50 ` Vineet Gupta
2017-07-27 7:50 ` Vineet Gupta
2017-07-28 1:24 ` Stephen Boyd
2017-07-28 1:24 ` Stephen Boyd
2017-08-04 1:53 ` Stephen Boyd
2017-08-04 1:53 ` Stephen Boyd
2017-08-09 16:43 ` Eugeniy Paltsev [this message]
2017-08-09 16:43 ` Eugeniy Paltsev
2017-08-09 16:43 ` Eugeniy Paltsev
2017-08-09 17:22 ` sboyd
2017-08-09 17:22 ` sboyd
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=1502297028.2586.4.camel@synopsys.com \
--to=eugeniy.paltsev@synopsys.com \
--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=robh+dt@kernel.org \
--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.