| Message ID | 87o9mwridk.wl%kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hello Morimoto-san, On Monday, 18 December 2017 02:35:56 EET Kuninori Morimoto wrote: > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > 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, FDPLL = (fdpll + 1). > > fclkout = fin * (n + 1) / (m + 1) / (fdpll + 1) > > This is the datasheet formula. > One note here is that it should be 2kHz < 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 <hiroshi.inose.rb@renesas.com> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > v3 -> v4 > > - 2000 -> 2kHz > > drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 58 ++++++++++++++++++++++++++++--- > 1 file changed, 54 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 6820461f..574854a 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > @@ -125,13 +125,63 @@ 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) > + * FDPLL : (fdpll + 1) > + * P : 2 > + * 2kHz < fvco < 4096MHz > + * > + * To be small jitter, Nitpicking, I would write this "to minimize the jitter". > + * N : as large as possible > + * M : as small as possible > + */ > + for (m = 0; m < 4; m++) { > + for (n = 119; n > 38; n--) { > + /* > + * NOTE: > + * > + * This code is assuming "used" from 64bit CPU only, > + * not from 32bit CPU. But both can compile correctly Nitpicking again, I would write this "This code only runs on 64-bit architectures, the unsigned long type can thus be used for 64-bit computation. It will still compile without any warning on 32-bit architectures." > + */ > + > + /* > + * fvco = fin * P * N / M > + * fclkout = fin * N / M / FDPLL > + * > + * To avoid duplicate calculation, let's use below > + * > + * finnm = fin * N / M This is called fout in your diagram above, I would use the same name here. > + * fvco = finnm * P > + * fclkout = finnm / FDPLL > + */ > + unsigned long finnm = input * (n + 1) / (m + 1); > + unsigned long fvco = finnm * 2; > + > + if (fvco < 2000 || fvco > 4096 * 1000 * 1000U) > + continue; How about if (fvco < 1000 || fvco > 2048 * 1000 * 1000) to avoid computing the intermediate fvco variable ? If you agree with these small changes there's no need to resubmit the patch, I'll modify it when applying, and Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > for (fdpll = 1; fdpll < 32; fdpll++) { > unsigned long output; > > - output = input * (n + 1) / (m + 1) > - / (fdpll + 1); > + output = finnm / (fdpll + 1); > if (output >= 400 * 1000 * 1000) > continue;
Hi Laurent Thank you for your feedback > > + * To be small jitter, > > Nitpicking, I would write this "to minimize the jitter". (snip) > > + * This code is assuming "used" from 64bit CPU only, > > + * not from 32bit CPU. But both can compile correctly > > Nitpicking again, I would write this "This code only runs on 64-bit > architectures, the unsigned long type can thus be used for 64-bit computation. > It will still compile without any warning on 32-bit architectures." I will follow your English ;) > > + /* > > + * fvco = fin * P * N / M > > + * fclkout = fin * N / M / FDPLL > > + * > > + * To avoid duplicate calculation, let's use below > > + * > > + * finnm = fin * N / M > > This is called fout in your diagram above, I would use the same name here. Oops indeed. I didn't notice > > + unsigned long finnm = input * (n + 1) / (m + 1); > > + unsigned long fvco = finnm * 2; > > + > > + if (fvco < 2000 || fvco > 4096 * 1000 * 1000U) > > + continue; > > How about > > if (fvco < 1000 || fvco > 2048 * 1000 * 1000) > > to avoid computing the intermediate fvco variable ? I think you want to say - if (fvco < 1000 || fvco > 2048 * 1000 * 1000) + if (fout < 1000 || fout > 2048 * 1000 * 1000) Actually I notcied about this, but I thought it makes user confuse. Thus, I kept original number. I'm happy if compiler can adjust it automatically, if not, I have no objection to modify it but we want to have such comment ? Because above comment/explain mentions about "fvco", not "fout". > If you agree with these small changes there's no need to resubmit the patch, > I'll modify it when applying, and > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thank you for your help Best regards --- Kuninori Morimoto
Hi Morimoto-san, On Monday, 18 December 2017 10:38:19 EET Kuninori Morimoto wrote: > Hi Laurent > > Thank you for your feedback > > >> + * To be small jitter, > > > > Nitpicking, I would write this "to minimize the jitter". > > (snip) > > >> + * This code is assuming "used" from 64bit CPU only, > >> + * not from 32bit CPU. But both can compile correctly > > > > Nitpicking again, I would write this "This code only runs on 64-bit > > architectures, the unsigned long type can thus be used for 64-bit > > computation. It will still compile without any warning on 32-bit > > architectures." > > I will follow your English ;) > > >> + /* > >> + * fvco = fin * P * N / M > >> + * fclkout = fin * N / M / FDPLL > >> + * > >> + * To avoid duplicate calculation, let's use below > >> + * > >> + * finnm = fin * N / M > > > > This is called fout in your diagram above, I would use the same name here. > > Oops indeed. I didn't notice > > >> + unsigned long finnm = input * (n + 1) / (m + 1); > >> + unsigned long fvco = finnm * 2; > >> + > >> + if (fvco < 2000 || fvco > 4096 * 1000 * 1000U) > >> + continue; > > > > How about > > > > if (fvco < 1000 || fvco > 2048 * 1000 * 1000) > > > > to avoid computing the intermediate fvco variable ? > > I think you want to say > > - if (fvco < 1000 || fvco > 2048 * 1000 * 1000) > + if (fout < 1000 || fout > 2048 * 1000 * 1000) Yes, sorry, that's what I meant. > Actually I notcied about this, but I thought it makes > user confuse. Thus, I kept original number. > > I'm happy if compiler can adjust it automatically, > if not, I have no objection to modify it but we want to have such comment ? > Because above comment/explain mentions about "fvco", not "fout". Sure, I'll add a comment, it's a good point. > > If you agree with these small changes there's no need to resubmit the > > patch, I'll modify it when applying, and > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Thank you for your help Thank you for the code :-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 6820461f..574854a 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -125,13 +125,63 @@ 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) + * FDPLL : (fdpll + 1) + * P : 2 + * 2kHz < fvco < 4096MHz + * + * 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--) { + /* + * NOTE: + * + * This code is assuming "used" from 64bit CPU only, + * not from 32bit CPU. But both can compile correctly + */ + + /* + * fvco = fin * P * N / M + * fclkout = fin * N / M / FDPLL + * + * To avoid duplicate calculation, let's use below + * + * finnm = fin * N / M + * fvco = finnm * P + * fclkout = finnm / FDPLL + */ + unsigned long finnm = input * (n + 1) / (m + 1); + unsigned long fvco = finnm * 2; + + if (fvco < 2000 || fvco > 4096 * 1000 * 1000U) + continue; + for (fdpll = 1; fdpll < 32; fdpll++) { unsigned long output; - output = input * (n + 1) / (m + 1) - / (fdpll + 1); + output = finnm / (fdpll + 1); if (output >= 400 * 1000 * 1000) continue;