Re: [PATCH 4/9] drm/i915: fixup g4x pll limits

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jun 12, 2013 at 12:07:39AM +0100, Damien Lespiau wrote:
> On Tue, May 21, 2013 at 09:54:54PM +0200, Daniel Vetter wrote:
> > Again the same confusion that our code expects m1/m2 in register values.
> > This time around with the added fun that many of the existing values
> > have been all off by a bit in different directions. Hence extract a
> > common #define.
> > 
> > Note that n limits differ between lvds and other outputs. Strangely they've
> > all been correct already.
> > 
> > v2: Rebased on top of the DP pll rework, which makes it even more
> > obvious that we can do this ...
> 
> I'm a bit confused by this one:
> 
>   - I don't seem to find the special LVDS limits for n
>   - Those limits are gated by a IS_G4X(). IS_G4X() is true for eaglelake
>     and cantiga. The docs single out Cantiga and we seem to need a
>     different set of limits for that platform (compared to the rest of
>     gen4) platforms?
>   - The limits for n are 3-6 or 3-5 in this code but I read 3-8 and 5-6
>     (cantiga)

I've looked at the mess around n values and decided to run the other
direction. My understanding is that the min/max in the xml are just the
limits of the table and not the hw (i.e. with the given clock ranges and
precision all the most precise clock settings happen to only use that
range).

For the actual values they seem to agree with my docs here with the
exception of TV-out. I have no idea what's going on there really, but at
least this patch here shouldn't break stuff. Otoh native TV-out seems to
have it's own pll and also pipe timing registers, so maybe it simply
doesn't matter.

>   - m doesn't seem to match what I have, it seems like it could be the
>     cantiga values (except that m is in "formula space" so we don't
>     substract 2)

Yeah, m is in formula space, since we compare that limit with what
i9xx_dpll_compute_m does.

>   - m1 and m2 seem to match what I have for cantiga, are we supposed to
>     have those limits for eaglelake as well?

Afaik eaglelake as the desktop version of cantiga has all the same stuff
(minus a few mobile-only power saving features). The spreadsheet I have
here is even called eaglelake_cantiga_something.xls.

> Well, the only conclusion is that I must be reading the wrong docs, or?

I guess I didn't really help :(
-Daniel

> 
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 20 ++++++++------------
> >  1 file changed, 8 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index ea8eb0c..cb54131 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -136,14 +136,16 @@ static const intel_limit_t intel_limits_i9xx_lvds = {
> >  		.p2_slow = 14, .p2_fast = 7 },
> >  };
> >  
> > +#define G4X_DPLL_M_LIMITS \
> > +	.m = { .min = 104, .max = 138 },\
> > +	.m1 = { .min = 15, .max = 21 },\
> > +	.m2 = { .min = 3, .max = 11 },
> >  
> >  static const intel_limit_t intel_limits_g4x_sdvo = {
> >  	.dot = { .min = 25000, .max = 270000 },
> >  	.vco = { .min = 1750000, .max = 3500000},
> >  	.n = { .min = 1, .max = 4 },
> > -	.m = { .min = 104, .max = 138 },
> > -	.m1 = { .min = 17, .max = 23 },
> > -	.m2 = { .min = 5, .max = 11 },
> > +	G4X_DPLL_M_LIMITS
> >  	.p = { .min = 10, .max = 30 },
> >  	.p1 = { .min = 1, .max = 3},
> >  	.p2 = { .dot_limit = 270000,
> > @@ -156,9 +158,7 @@ static const intel_limit_t intel_limits_g4x_hdmi = {
> >  	.dot = { .min = 22000, .max = 400000 },
> >  	.vco = { .min = 1750000, .max = 3500000},
> >  	.n = { .min = 1, .max = 4 },
> > -	.m = { .min = 104, .max = 138 },
> > -	.m1 = { .min = 16, .max = 23 },
> > -	.m2 = { .min = 5, .max = 11 },
> > +	G4X_DPLL_M_LIMITS
> >  	.p = { .min = 5, .max = 80 },
> >  	.p1 = { .min = 1, .max = 8},
> >  	.p2 = { .dot_limit = 165000,
> > @@ -169,9 +169,7 @@ static const intel_limit_t intel_limits_g4x_single_channel_lvds = {
> >  	.dot = { .min = 20000, .max = 115000 },
> >  	.vco = { .min = 1750000, .max = 3500000 },
> >  	.n = { .min = 1, .max = 3 },
> > -	.m = { .min = 104, .max = 138 },
> > -	.m1 = { .min = 17, .max = 23 },
> > -	.m2 = { .min = 5, .max = 11 },
> > +	G4X_DPLL_M_LIMITS
> >  	.p = { .min = 28, .max = 112 },
> >  	.p1 = { .min = 2, .max = 8 },
> >  	.p2 = { .dot_limit = 0,
> > @@ -183,9 +181,7 @@ static const intel_limit_t intel_limits_g4x_dual_channel_lvds = {
> >  	.dot = { .min = 80000, .max = 224000 },
> >  	.vco = { .min = 1750000, .max = 3500000 },
> >  	.n = { .min = 1, .max = 3 },
> > -	.m = { .min = 104, .max = 138 },
> > -	.m1 = { .min = 17, .max = 23 },
> > -	.m2 = { .min = 5, .max = 11 },
> > +	G4X_DPLL_M_LIMITS
> >  	.p = { .min = 14, .max = 42 },
> >  	.p1 = { .min = 2, .max = 6 },
> >  	.p2 = { .dot_limit = 0,
> > -- 
> > 1.7.11.7
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch