From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from galahad.ideasonboard.com ([185.26.127.97]:37578 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752259AbdLMJJP (ORCPT ); Wed, 13 Dec 2017 04:09:15 -0500 From: Laurent Pinchart To: Kuninori Morimoto Cc: David Airlie , dri-devel@lists.freedesktop.org, linux-renesas-soc@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] drm: rcar-du: calculate DPLLCR to be more small jitter Date: Wed, 13 Dec 2017 11:09:18 +0200 Message-ID: <4708231.IEM7dkWZuQ@avalon> In-Reply-To: <87374oz9f9.wl%kuninori.morimoto.gx@renesas.com> References: <87374oz9f9.wl%kuninori.morimoto.gx@renesas.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-renesas-soc-owner@vger.kernel.org List-ID: Hello Morimoto-san, Thank you for the patch. On Wednesday, 6 December 2017 08:05:38 EET Kuninori Morimoto wrote: > From: Kuninori Morimoto > > In general, PLL has VCO (= Voltage controlled oscillator), > one of the very important electronic feature called as "jitter" > is related to this VCO. > In academic generalism, VCO should be maximum to be more small jitter. > In high frequency clock, jitter will be large impact. > Thus, selecting Hi VCO is general theory. > > fin fvco fout fclkout > in --> [1/M] --> |PD| -> [LPF] -> [VCO] -> [1/P] -+-> [1/FDPLL] -> out > +-> | | | > | | > +-----------------[1/N]<-------------+ > > fclkout = fvco / P / FDPLL -- (1) > > In PD, it will loop until fin/M = fvco/P/N > > fvco = fin * P * N / M -- (2) > > (1) + (2) indicates, fclkout = fin * N / M / FDPLL > In this device, N = (n + 1), M = (m + 1), P = 2, thus > > fclkout = fin * (n + 1) / (m + 1) / FDPLL > > This is the datasheet formula. > One note here is that it should be 2000 < fvco < 4096MHz > To be smaller jitter, fvco should be maximum, > in other words, N as large as possible, M as small as possible driver > should select. Here, basically M=1. > This patch do it. > > Reported-by: HIROSHI INOSE > Signed-off-by: Kuninori Morimoto > --- > v1 -> v2 > > - tidyup typo on git-log "fout" -> "fclkout" > - tidyup for loop terminate condition 40 -> 38 for n > > drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 36 ++++++++++++++++++++++++++++-- > 1 file changed, 34 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index b492063..57479c9 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > @@ -125,8 +125,40 @@ static void rcar_du_dpll_divider(struct rcar_du_crtc > *rcrtc, unsigned int m; > unsigned int n; > > - for (n = 39; n < 120; n++) { > - for (m = 0; m < 4; m++) { > + /* > + * fin fvco fout fclkout > + * in --> [1/M] --> |PD| -> [LPF] -> [VCO] -> [1/P] -+-> [1/FDPLL] -> out > + * +-> | | | > + * | | > + * +-----------------[1/N]<-------------+ > + * > + * fclkout = fvco / P / FDPLL -- (1) > + * > + * fin/M = fvco/P/N > + * > + * fvco = fin * P * N / M -- (2) > + * > + * (1) + (2) indicates > + * > + * fclkout = fin * N / M / FDPLL > + * > + * NOTES > + * N = (n + 1), M = (m + 1), P = 2 > + * 2000 < fvco < 4096Mhz Are you sure that the fvco constraint is really 2kHz, and not 2GHz ? 2kHz - 4GHz would be a surprisingly large range. > + * Basically M=1 Why is this ? > + * To be small jitter, > + * N : as large as possible > + * M : as small as possible > + */ > + for (m = 0; m < 4; m++) { > + for (n = 119; n > 38; n--) { > + unsigned long long fvco = input * 2 * (n + 1) / (m + 1); This code runs for Gen3 only, so unsigned long would be enough. The rest of the function already relies on native support for 64-bit calculation. If you wanted to run this on a 32-bit CPU, you would likely need to do_div() for the division, and convert input to u64 to avoid integer overflows, otherwise the calculation will be performed on 32-bit before a final conversion to 64-bit. > + if ((fvco < 2000) || > + (fvco > 4096000000ll)) No need for the inner parentheses, and you can write both conditions on a single line. Furthemore 4096 MHz will fit in a 32-bit number, so there's no need for the ll. > + continue; > + I think you can then drop the output >= 4000000000 check inside the inner fdpll loop, as the output frequency can't be higher than 4GHz if the VCO frequency isn't. > for (fdpll = 1; fdpll < 32; fdpll++) { > unsigned long output; The output frequency on the line below can be calculated with output = fvco / 2 / (fdpll + 1) to avoid the multiplication by (n + 1) and division by (m + 1). If we wanted to optimize even more we could compute and operatate on fout instead of fvco, that would remove the * 2 and / 2. This patch seems to be a good first step in case of multiple possible exact frequency matches. However, when the PLL can't achieve an exact match, we might still end up with a high M value when a lower value could produce an output frequency close enough to the desired value. I wonder if this function should also take a frequency tolerance as an input parameter, and compute the M, N and FDPLL values that will produce an output frequency within the tolerance with M as small as possible. This can be done as a separate patch. And while we're discussing PLL calculation, the three nested loops will run a total of 10044 iterations :-/ That's a lot, and should be optimized if possible. With the outer loop operating on N an easy optimization would have been to compute fin * N in a local variable to avoid redoing the multiplication for every value of M, but that's not possible anymore with the outer loop operating on M. -- Regards, Laurent Pinchart From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH v2] drm: rcar-du: calculate DPLLCR to be more small jitter Date: Wed, 13 Dec 2017 11:09:18 +0200 Message-ID: <4708231.IEM7dkWZuQ@avalon> References: <87374oz9f9.wl%kuninori.morimoto.gx@renesas.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from galahad.ideasonboard.com (galahad.ideasonboard.com [IPv6:2001:4b98:dc2:45:216:3eff:febb:480d]) by gabe.freedesktop.org (Postfix) with ESMTPS id CD68C6E3DB for ; Wed, 13 Dec 2017 09:09:15 +0000 (UTC) In-Reply-To: <87374oz9f9.wl%kuninori.morimoto.gx@renesas.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Kuninori Morimoto Cc: David Airlie , linux-renesas-soc@vger.kernel.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org SGVsbG8gTW9yaW1vdG8tc2FuLAoKVGhhbmsgeW91IGZvciB0aGUgcGF0Y2guCgpPbiBXZWRuZXNk YXksIDYgRGVjZW1iZXIgMjAxNyAwODowNTozOCBFRVQgS3VuaW5vcmkgTW9yaW1vdG8gd3JvdGU6 Cj4gRnJvbTogS3VuaW5vcmkgTW9yaW1vdG8gPGt1bmlub3JpLm1vcmltb3RvLmd4QHJlbmVzYXMu Y29tPgo+IAo+IEluIGdlbmVyYWwsIFBMTCBoYXMgVkNPICg9IFZvbHRhZ2UgY29udHJvbGxlZCBv c2NpbGxhdG9yKSwKPiBvbmUgb2YgdGhlIHZlcnkgaW1wb3J0YW50IGVsZWN0cm9uaWMgZmVhdHVy ZSBjYWxsZWQgYXMgImppdHRlciIKPiBpcyByZWxhdGVkIHRvIHRoaXMgVkNPLgo+IEluIGFjYWRl bWljIGdlbmVyYWxpc20sIFZDTyBzaG91bGQgYmUgbWF4aW11bSB0byBiZSBtb3JlIHNtYWxsIGpp dHRlci4KPiBJbiBoaWdoIGZyZXF1ZW5jeSBjbG9jaywgaml0dGVyIHdpbGwgYmUgbGFyZ2UgaW1w YWN0Lgo+IFRodXMsIHNlbGVjdGluZyBIaSBWQ08gaXMgZ2VuZXJhbCB0aGVvcnkuCj4gCj4gICAg ZmluICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgZnZjbyAgICAgICAgZm91dCAgICAg IGZjbGtvdXQKPiBpbiAtLT4gWzEvTV0gLS0+IHxQRHwgLT4gW0xQRl0gLT4gW1ZDT10gLT4gWzEv UF0gLSstPiBbMS9GRFBMTF0gLT4gb3V0Cj4gICAgICAgICAgICAgICstPiB8ICB8ICAgICAgICAg ICAgICAgICAgICAgICAgICAgICB8Cj4gICAgICAgICAgICAgIHwgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICB8Cj4gICAgICAgICAgICAgICstLS0tLS0tLS0tLS0tLS0tLVsxL05d PC0tLS0tLS0tLS0tLS0rCj4gCj4gCWZjbGtvdXQgPSBmdmNvIC8gUCAvIEZEUExMIC0tICgxKQo+ IAo+IEluIFBELCBpdCB3aWxsIGxvb3AgdW50aWwgZmluL00gPSBmdmNvL1AvTgo+IAo+IAlmdmNv ID0gZmluICogUCAqICBOIC8gTSAtLSAoMikKPiAKPiAoMSkgKyAoMikgaW5kaWNhdGVzLCBmY2xr b3V0ID0gZmluICogTiAvIE0gLyBGRFBMTAo+IEluIHRoaXMgZGV2aWNlLCBOID0gKG4gKyAxKSwg TSA9IChtICsgMSksIFAgPSAyLCB0aHVzCj4gCj4gCWZjbGtvdXQgPSBmaW4gKiAobiArIDEpIC8g KG0gKyAxKSAvIEZEUExMCj4gCj4gVGhpcyBpcyB0aGUgZGF0YXNoZWV0IGZvcm11bGEuCj4gT25l IG5vdGUgaGVyZSBpcyB0aGF0IGl0IHNob3VsZCBiZSAyMDAwIDwgZnZjbyA8IDQwOTZNSHoKPiBU byBiZSBzbWFsbGVyIGppdHRlciwgZnZjbyBzaG91bGQgYmUgbWF4aW11bSwKPiBpbiBvdGhlciB3 b3JkcywgTiBhcyBsYXJnZSBhcyBwb3NzaWJsZSwgTSBhcyBzbWFsbCBhcyBwb3NzaWJsZSBkcml2 ZXIKPiBzaG91bGQgc2VsZWN0LiBIZXJlLCBiYXNpY2FsbHkgTT0xLgo+IFRoaXMgcGF0Y2ggZG8g aXQuCj4gCj4gUmVwb3J0ZWQtYnk6IEhJUk9TSEkgSU5PU0UgPGhpcm9zaGkuaW5vc2UucmJAcmVu ZXNhcy5jb20+Cj4gU2lnbmVkLW9mZi1ieTogS3VuaW5vcmkgTW9yaW1vdG8gPGt1bmlub3JpLm1v cmltb3RvLmd4QHJlbmVzYXMuY29tPgo+IC0tLQo+IHYxIC0+IHYyCj4gCj4gIC0gdGlkeXVwIHR5 cG8gb24gZ2l0LWxvZyAiZm91dCIgLT4gImZjbGtvdXQiCj4gIC0gdGlkeXVwIGZvciBsb29wIHRl cm1pbmF0ZSBjb25kaXRpb24gNDAgLT4gMzggZm9yIG4KPiAKPiAgZHJpdmVycy9ncHUvZHJtL3Jj YXItZHUvcmNhcl9kdV9jcnRjLmMgfCAzNiArKysrKysrKysrKysrKysrKysrKysrKysrKysrLS0K PiAgMSBmaWxlIGNoYW5nZWQsIDM0IGluc2VydGlvbnMoKyksIDIgZGVsZXRpb25zKC0pCj4gCj4g ZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9yY2FyLWR1L3JjYXJfZHVfY3J0Yy5jCj4gYi9k cml2ZXJzL2dwdS9kcm0vcmNhci1kdS9yY2FyX2R1X2NydGMuYyBpbmRleCBiNDkyMDYzLi41NzQ3 OWM5IDEwMDY0NAo+IC0tLSBhL2RyaXZlcnMvZ3B1L2RybS9yY2FyLWR1L3JjYXJfZHVfY3J0Yy5j Cj4gKysrIGIvZHJpdmVycy9ncHUvZHJtL3JjYXItZHUvcmNhcl9kdV9jcnRjLmMKPiBAQCAtMTI1 LDggKzEyNSw0MCBAQCBzdGF0aWMgdm9pZCByY2FyX2R1X2RwbGxfZGl2aWRlcihzdHJ1Y3QgcmNh cl9kdV9jcnRjCj4gKnJjcnRjLCB1bnNpZ25lZCBpbnQgbTsKPiAgCXVuc2lnbmVkIGludCBuOwo+ IAo+IC0JZm9yIChuID0gMzk7IG4gPCAxMjA7IG4rKykgewo+IC0JCWZvciAobSA9IDA7IG0gPCA0 OyBtKyspIHsKPiArCS8qCj4gKwkgKiAgIGZpbiAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgIGZ2Y28gICAgICAgIGZvdXQgICAgICAgZmNsa291dAo+ICsJICogaW4gLS0+IFsxL01dIC0t PiB8UER8IC0+IFtMUEZdIC0+IFtWQ09dIC0+IFsxL1BdIC0rLT4gWzEvRkRQTExdIC0+IG91dAo+ ICsJICogICAgICAgICAgICAgICstPiB8ICB8ICAgICAgICAgICAgICAgICAgICAgICAgICAgICB8 Cj4gKwkgKiAgICAgICAgICAgICAgfCAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg IHwKPiArCSAqICAgICAgICAgICAgICArLS0tLS0tLS0tLS0tLS0tLS1bMS9OXTwtLS0tLS0tLS0t LS0tKwo+ICsJICoKPiArCSAqCWZjbGtvdXQgPSBmdmNvIC8gUCAvIEZEUExMIC0tICgxKQo+ICsJ ICoKPiArCSAqIGZpbi9NID0gZnZjby9QL04KPiArCSAqCj4gKwkgKglmdmNvID0gZmluICogUCAq ICBOIC8gTSAtLSAoMikKPiArCSAqCj4gKwkgKiAoMSkgKyAoMikgaW5kaWNhdGVzCj4gKwkgKgo+ ICsJICoJZmNsa291dCA9IGZpbiAqIE4gLyBNIC8gRkRQTEwKPiArCSAqCj4gKwkgKiBOT1RFUwo+ ICsJICoJTiA9IChuICsgMSksIE0gPSAobSArIDEpLCBQID0gMgo+ICsJICoJMjAwMCA8IGZ2Y28g PCA0MDk2TWh6CgpBcmUgeW91IHN1cmUgdGhhdCB0aGUgZnZjbyBjb25zdHJhaW50IGlzIHJlYWxs eSAya0h6LCBhbmQgbm90IDJHSHogPyAya0h6IC0gCjRHSHogd291bGQgYmUgYSBzdXJwcmlzaW5n bHkgbGFyZ2UgcmFuZ2UuCgo+ICsJICoJQmFzaWNhbGx5IE09MQoKV2h5IGlzIHRoaXMgPwoKPiAr CSAqIFRvIGJlIHNtYWxsIGppdHRlciwKPiArCSAqIE4gOiBhcyBsYXJnZSBhcyBwb3NzaWJsZQo+ ICsJICogTSA6IGFzIHNtYWxsIGFzIHBvc3NpYmxlCj4gKwkgKi8KPiArCWZvciAobSA9IDA7IG0g PCA0OyBtKyspIHsKPiArCQlmb3IgKG4gPSAxMTk7IG4gPiAzODsgbi0tKSB7Cj4gKwkJCXVuc2ln bmVkIGxvbmcgbG9uZyBmdmNvID0gaW5wdXQgKiAyICogKG4gKyAxKSAvIChtICsgMSk7CgpUaGlz IGNvZGUgcnVucyBmb3IgR2VuMyBvbmx5LCBzbyB1bnNpZ25lZCBsb25nIHdvdWxkIGJlIGVub3Vn aC4gVGhlIHJlc3Qgb2YgCnRoZSBmdW5jdGlvbiBhbHJlYWR5IHJlbGllcyBvbiBuYXRpdmUgc3Vw cG9ydCBmb3IgNjQtYml0IGNhbGN1bGF0aW9uLiBJZiB5b3UgCndhbnRlZCB0byBydW4gdGhpcyBv biBhIDMyLWJpdCBDUFUsIHlvdSB3b3VsZCBsaWtlbHkgbmVlZCB0byBkb19kaXYoKSBmb3IgdGhl IApkaXZpc2lvbiwgYW5kIGNvbnZlcnQgaW5wdXQgdG8gdTY0IHRvIGF2b2lkIGludGVnZXIgb3Zl cmZsb3dzLCBvdGhlcndpc2UgdGhlIApjYWxjdWxhdGlvbiB3aWxsIGJlIHBlcmZvcm1lZCBvbiAz Mi1iaXQgYmVmb3JlIGEgZmluYWwgY29udmVyc2lvbiB0byA2NC1iaXQuCgo+ICsJCQlpZiAoKGZ2 Y28gPCAyMDAwKSB8fAo+ICsJCQkgICAgKGZ2Y28gPiA0MDk2MDAwMDAwbGwpKQoKTm8gbmVlZCBm b3IgdGhlIGlubmVyIHBhcmVudGhlc2VzLCBhbmQgeW91IGNhbiB3cml0ZSBib3RoIGNvbmRpdGlv bnMgb24gYSAKc2luZ2xlIGxpbmUuIEZ1cnRoZW1vcmUgNDA5NiBNSHogd2lsbCBmaXQgaW4gYSAz Mi1iaXQgbnVtYmVyLCBzbyB0aGVyZSdzIG5vIApuZWVkIGZvciB0aGUgbGwuCgo+ICsJCQkJY29u dGludWU7Cj4gKwoKSSB0aGluayB5b3UgY2FuIHRoZW4gZHJvcCB0aGUgb3V0cHV0ID49IDQwMDAw MDAwMDAgY2hlY2sgaW5zaWRlIHRoZSBpbm5lciAKZmRwbGwgbG9vcCwgYXMgdGhlIG91dHB1dCBm cmVxdWVuY3kgY2FuJ3QgYmUgaGlnaGVyIHRoYW4gNEdIeiBpZiB0aGUgVkNPIApmcmVxdWVuY3kg aXNuJ3QuCgo+ICAJCQlmb3IgKGZkcGxsID0gMTsgZmRwbGwgPCAzMjsgZmRwbGwrKykgewo+ICAJ CQkJdW5zaWduZWQgbG9uZyBvdXRwdXQ7CgpUaGUgb3V0cHV0IGZyZXF1ZW5jeSBvbiB0aGUgbGlu ZSBiZWxvdyBjYW4gYmUgY2FsY3VsYXRlZCB3aXRoCgoJb3V0cHV0ID0gZnZjbyAvIDIgLyAoZmRw bGwgKyAxKQoKdG8gYXZvaWQgdGhlIG11bHRpcGxpY2F0aW9uIGJ5IChuICsgMSkgYW5kIGRpdmlz aW9uIGJ5IChtICsgMSkuCgpJZiB3ZSB3YW50ZWQgdG8gb3B0aW1pemUgZXZlbiBtb3JlIHdlIGNv dWxkIGNvbXB1dGUgYW5kIG9wZXJhdGF0ZSBvbiBmb3V0IAppbnN0ZWFkIG9mIGZ2Y28sIHRoYXQg d291bGQgcmVtb3ZlIHRoZSAqIDIgYW5kIC8gMi4KClRoaXMgcGF0Y2ggc2VlbXMgdG8gYmUgYSBn b29kIGZpcnN0IHN0ZXAgaW4gY2FzZSBvZiBtdWx0aXBsZSBwb3NzaWJsZSBleGFjdCAKZnJlcXVl bmN5IG1hdGNoZXMuIEhvd2V2ZXIsIHdoZW4gdGhlIFBMTCBjYW4ndCBhY2hpZXZlIGFuIGV4YWN0 IG1hdGNoLCB3ZSAKbWlnaHQgc3RpbGwgZW5kIHVwIHdpdGggYSBoaWdoIE0gdmFsdWUgd2hlbiBh IGxvd2VyIHZhbHVlIGNvdWxkIHByb2R1Y2UgYW4gCm91dHB1dCBmcmVxdWVuY3kgY2xvc2UgZW5v dWdoIHRvIHRoZSBkZXNpcmVkIHZhbHVlLiBJIHdvbmRlciBpZiB0aGlzIGZ1bmN0aW9uIApzaG91 bGQgYWxzbyB0YWtlIGEgZnJlcXVlbmN5IHRvbGVyYW5jZSBhcyBhbiBpbnB1dCBwYXJhbWV0ZXIs IGFuZCBjb21wdXRlIHRoZSAKTSwgTiBhbmQgRkRQTEwgdmFsdWVzIHRoYXQgd2lsbCBwcm9kdWNl IGFuIG91dHB1dCBmcmVxdWVuY3kgd2l0aGluIHRoZSAKdG9sZXJhbmNlIHdpdGggTSBhcyBzbWFs bCBhcyBwb3NzaWJsZS4gVGhpcyBjYW4gYmUgZG9uZSBhcyBhIHNlcGFyYXRlIHBhdGNoLgoKQW5k IHdoaWxlIHdlJ3JlIGRpc2N1c3NpbmcgUExMIGNhbGN1bGF0aW9uLCB0aGUgdGhyZWUgbmVzdGVk IGxvb3BzIHdpbGwgcnVuIGEgCnRvdGFsIG9mIDEwMDQ0IGl0ZXJhdGlvbnMgOi0vIFRoYXQncyBh IGxvdCwgYW5kIHNob3VsZCBiZSBvcHRpbWl6ZWQgaWYgCnBvc3NpYmxlLiBXaXRoIHRoZSBvdXRl ciBsb29wIG9wZXJhdGluZyBvbiBOIGFuIGVhc3kgb3B0aW1pemF0aW9uIHdvdWxkIGhhdmUgCmJl ZW4gdG8gY29tcHV0ZSBmaW4gKiBOIGluIGEgbG9jYWwgdmFyaWFibGUgdG8gYXZvaWQgcmVkb2lu ZyB0aGUgCm11bHRpcGxpY2F0aW9uIGZvciBldmVyeSB2YWx1ZSBvZiBNLCBidXQgdGhhdCdzIG5v dCBwb3NzaWJsZSBhbnltb3JlIHdpdGggdGhlIApvdXRlciBsb29wIG9wZXJhdGluZyBvbiBNLgoK LS0gClJlZ2FyZHMsCgpMYXVyZW50IFBpbmNoYXJ0CgpfX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBs aXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1h bi9saXN0aW5mby9kcmktZGV2ZWwK