[PATCH v4 0/2] drm: rcar-du: calculate DPLLCR to be more small jitter

[PATCH v4 0/2] drm: rcar-du: calculate DPLLCR to be more small jitter

[Kuninori Morimoto]

Hi Laurent, David

These are v4 of DPLLCR patch for rcar-du.
Mainly fixuped 2000 -> 2kHz to unambiguous

Kuninori Morimoto (2):
  drm: rcar-du: use 1000 to avoid misunderstanding in rcar_du_dpll_divider()
  drm: rcar-du: calculate DPLLCR to be more small jitter

 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 60 +++++++++++++++++++++++++++++++---
 1 file changed, 55 insertions(+), 5 deletions(-)

-- 
1.9.1

[PATCH v4 1/2] drm: rcar-du: use 1000 to avoid misunderstanding in rcar_du_dpll_divider()

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

It is difficult to understand its scale if number has many 0s.
This patch uses "* 1000" to avoid it in rcar_du_dpll_divider().

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
v3 -> v4

 - no change

 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 5685d5a..6820461f 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -132,7 +132,7 @@ static void rcar_du_dpll_divider(struct rcar_du_crtc *rcrtc,
 
 				output = input * (n + 1) / (m + 1)
 				       / (fdpll + 1);
-				if (output >= 400000000)
+				if (output >= 400 * 1000 * 1000)
 					continue;
 
 				diff = abs((long)output - (long)target);
-- 
1.9.1

[PATCH v4 2/2] drm: rcar-du: calculate DPLLCR to be more small jitter

[Kuninori Morimoto]

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,
+	 * 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;
 
-- 
1.9.1

Re: [PATCH v4 1/2] drm: rcar-du: use 1000 to avoid misunderstanding in rcar_du_dpll_divider()

[Laurent Pinchart]

Hello Morimoto-san,

Thankk you for the patch.

On Monday, 18 December 2017 02:35:18 EET Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> It is difficult to understand its scale if number has many 0s.
> This patch uses "* 1000" to avoid it in rcar_du_dpll_divider().
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> v3 -> v4
> 
>  - no change
> 
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 5685d5a..6820461f 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -132,7 +132,7 @@ static void rcar_du_dpll_divider(struct rcar_du_crtc
> *rcrtc,
> 
>  				output = input * (n + 1) / (m + 1)
>  				       / (fdpll + 1);
> -				if (output >= 400000000)
> +				if (output >= 400 * 1000 * 1000)
>  					continue;
> 
>  				diff = abs((long)output - (long)target);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 2/2] drm: rcar-du: calculate DPLLCR to be more small jitter

[Laurent Pinchart]

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;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 2/2] drm: rcar-du: calculate DPLLCR to be more small jitter

[Kuninori Morimoto]

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

Re: [PATCH v4 2/2] drm: rcar-du: calculate DPLLCR to be more small jitter

[Laurent Pinchart]

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 :-)

-- 
Regards,

Laurent Pinchart