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
From: Kuninori Morimoto <[email protected]>
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 <[email protected]>
---
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
From: Kuninori Morimoto <[email protected]>
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 <[email protected]>
Signed-off-by: Kuninori Morimoto <[email protected]>
---
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
Hello Morimoto-san,
Thankk you for the patch.
On Monday, 18 December 2017 02:35:18 EET Kuninori Morimoto wrote:
> From: Kuninori Morimoto <[email protected]>
>
> 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 <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
> ---
> 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
Hello Morimoto-san,
On Monday, 18 December 2017 02:35:56 EET Kuninori Morimoto wrote:
> From: Kuninori Morimoto <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Kuninori Morimoto <[email protected]>
> ---
> 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 <[email protected]>
> 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
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 <[email protected]>
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 <[email protected]>
>
> Thank you for your help
Thank you for the code :-)
--
Regards,
Laurent Pinchart