[Intel-gfx] [PATCH] drm/i915: Get correct display clock on 945gm

[Daniel Vetter]

On Fri, Jan 27, 2017 at 09:51:50PM +0200, Ville Syrjälä wrote:
> On Fri, Jan 27, 2017 at 06:24:25PM +0100, Arthur Heymans wrote:
> > Ville Syrjälä <ville.syrjala at linux.intel.com> writes:
> > 
> > >> 
> > >> > Now if someone could figure out where to dig up the DDR and FSB clocks
> > >> > we could also fix up the 190 vs. 200 MHz case...
> > >> >
> > >> >> +		default:
> > >> >> +		case GC_DISPLAY_CLOCK_190_200_MHZ:
> > >> >> +			return 200000;
> > >> >> +		}
> > >> >> +	}
> > >> >> +}
> > >> >> +
> > >> 
> > >> Hmm that seems to be 915gm specific (always 200 on 945gm). According to
> > >> "Mobile Intel® 915/910 Express Chipset: Datasheet", the only fsb/dram
> > >> combo that has 190MHz is FSB: 533MHz, DDR333. All the other supported
> > >> combos have 200Mhz set by that configuration.
> > >
> > > Yeah. Unfortunately I couldn't find a solid source of FSB/DDR clock
> > > information in the spec, apart from the actual strap pins but I can't
> > > see any register that would reflect those. So I guess we'll just leave
> > > it the way it is.
> > 
> > Hmm "Mobile Intel® 915GM/PM/GME/GMS and 910GML/GMLE Express Chipset
> > Specification Update" seems to sketch an even more complicated scenario,
> > where it even depends on the GMCH subtype...
> 
> Oh, I didn't even have that particular document here. Have it now, and
> indeed it shows even more variation.
> 
> > 
> > FSB and also DRAM freq can be probably be fetched from respectively
> > [2:0] and [6:4] of CLKCFG, MCHBAR(0xC00) but not documented.
> 
> I managed to dig up one more doc where the 6:4 bits are listed as
> 1 = 333
> 2 = 400
> 3 = 533
> 
> so those seem to agree with the 945gm version, with the 333 case
> added. The FSB speed I didn't manage to find in any 915 doc
> unfortunately. I suppose we could assume it matches 945gm with
> the extra 0=400 case added.
> 
> Apparently there's also CLKCFG bit 16 on 915gm which seems to be
> telling me something about the ddr speed and voltage:
> 0 = 333,400 (533 @ 1.5v)
> 1 = 533 @ 1.05V
> 
> so that could potentially solve part the voltage detection. Sadly only
> for DDR 533, if it's even correct.
> 
> So I see a couple of options we could do here:
> a) ignore all of it and just go with your original change of for the
>    320 vs. 333 case
> b) ignore most of these details and just pick either the 190/213 or
>    200/222 as our presumed clocks
> c) just use the fsb/ddr info and again just assume one or the other
>    for the cases where the voltage matters
> d) just assume the 945 DFT_STRAP1 detection of the voltage works on 915
>    and deal with every case. We wouldn't that far off the mark even if
>    we got it wrong so I don't see a huge problem with just guessing
> e) something I didn't think of?
> 
> I'll let you figure it out ;)

Isn't the defensive thing to go with the lower clock? We might get a
regression report, but chances are slim, and if we do get it we'll have a
guinea-pig to test stuff on :-) So option a)
-Daniel

> 
> > On 945GM I think the voltage strap is on DFT_STRAP1 bit20, MCHBAR(0xE08)
> > (also not really documented but this is how it is done in Coreboot).
> > It might be the same on 915GM...
> > 
> > Given that getting the 915GM display clock correctly probably ends up
> > being quite different from 945GM it might make sense to have 2 separate
> > functions for that?
> > 
> > Kind regards
> > -- 
> > Arthur Heymans
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch