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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <871sjyqd99.wl%kuninori.morimoto.gx@renesas.com>
Date:   Thu, 14 Dec 2017 02:10:27 +0000
From:   Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     David Airlie <airlied@...ux.ie>, dri-devel@...ts.freedesktop.org,
        linux-renesas-soc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] drm: rcar-du: calculate DPLLCR to be more small jitter


Hi Laurent

Thank you for your feedback

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