Hello Morimoto-san,
On Monday, 18 December 2017 02:35:56 EET Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto...@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...@renesas.com>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto...@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.pinch...@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;
--
Regards,
Laurent Pinchart
