# [PATCH v2] drm: rcar-du: calculate DPLLCR to be more small jitter

Kuninori Morimoto kuninori.morimoto.gx at renesas.com
Thu Dec 14 02:10:27 UTC 2017

```Hi Laurent

> > +	 * 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.

It is 2kHz. This is came from HW team, and indicated
on HW design sheet (?)

> > +	 *	Basically M=1
>
> Why is this ?

This is came from HW team, too.
They are assuming M=1, basically.
But yes confusable, let's remove it from comment.
m is started from 0 (= M=1), no need to explain.

> > +	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.
(snip)
> > +			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.

Yes, but compiled by 32bit too, right ?
Without this "ll", 32bit compiler say

warning: this decimal constant is unsigned only in ISO C90

# anyway, I will add this assumption (= used only by 64bit CPU)
# on comment to avoid future confusion

> 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.

I think code has

if (output >= 400000000)

This is 400MHz, not 4GHz

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

It is nice idea to avoid extra calculation.
I will use this idea, and add extrate comment to avoid confusion

Best regards
---
Kuninori Morimoto
```