[Intel-gfx] [PATCH] drm/i915: Fix rawclk readout for g4x

Fri May 5 10:33:56 UTC 2017

On Fri, May 05, 2017 at 09:48:08AM +0300, Jani Nikula wrote:
> On Thu, 04 May 2017, ville.syrjala at linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >
> > Turns out our skills in decoding the CLKCFG register weren't good
> > enough. On this particular elk the answer we got was 400 MHz when
> > in reality the clock was running at 266 MHz, which then caused us
> > to program a bogus AUX clock divider that caused all AUX communication
> > to fail.
> >
> > Sadly the docs are now in bit heaven, so the fix will have to be based
> > on empirical evidence. Using another elk machine I was able to frob
> > the FSB frequency from the BIOS and see how it affects the CLKCFG
> > register. The machine seesm to use a frequency of 266 MHz by default,
> > and fortunately it still boot even with the 50% CPU overclock that
> > we get when we bump the FSB up to 400 MHz.
> >
> > It turns out the actual FSB frequency and the register have no real
> > link whatsoever. The register value is based on some straps or something,
> > but fortunately those too can be configured from the BIOS on this board,
> > although it doesn't seem to respect the settings 100%. In the end I was
> > able to derive the following relationship:
> >
> > BIOS FSB / strap | CLKCFG
> > -------------------------
> > 200              | 0x2
> > 266              | 0x0
> > 333              | 0x4
> > 400              | 0x4
> >
> > So only the 200 and 400 MHz cases actually match how we're currently
> > decoding that register. But as the comment next to some of the defines
> > says, we have been just guessing anyway.
> >
> > So let's fix things up so that at least the 266 MHz case will work
> > correctly as that is actually the setting used by both the buggy
> > machine and my test machine.
> >
> > The fact that 333 and 400 MHz BIOS settings result in the same register
> > value is a little disappointing, as that means we can't tell them apart.
> > However, according to the gmch datasheet for both elk and ctg 400 Mhz is
> > not even a supported FSB frequency, so I'm going to make the assumption
> > that we should decode it as 333 MHz instead.
> 
> If you look at b11248df4c0d ("drm/i915: Add CLKCFG register definition")
> and 7662c8bd6545 ("drm/i915: add FIFO watermark support"), the original
> values seem to be on the guesswork side as well...

Some of them do match what's in the gmch datasheets for some platforms.
But sadly all the gmch datasheet are very incomplete, and some lack this
register entirely.

I would prefer to find a way to read this out from some actual PLL or
something, because these strap values might not have anything to do with
reality if the user has adjusted the FSB manually via the BIOS. But
yeah, -ENODOCS so there's not much we can do.

> 
> As such can't review, but
> 
> Acked-by: Jani Nikula <jani.nikula at intel.com>
> 
> >
> > Cc: stable at vger.kernel.org
> > Cc: Tomi Sarvela <tomi.p.sarvela at intel.com>
> > Reported-by: Tomi Sarvela <tomi.p.sarvela at intel.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100926
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h    | 10 +++++++---
> >  drivers/gpu/drm/i915/intel_cdclk.c |  6 ++----
> >  2 files changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index ee8170cda93e..524fdfda9d45 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3059,10 +3059,14 @@ enum skl_disp_power_wells {
> >  #define CLKCFG_FSB_667					(3 << 0)	/* hrawclk 166 */
> >  #define CLKCFG_FSB_800					(2 << 0)	/* hrawclk 200 */
> >  #define CLKCFG_FSB_1067					(6 << 0)	/* hrawclk 266 */
> > +#define CLKCFG_FSB_1067_ALT				(0 << 0)	/* hrawclk 266 */
> >  #define CLKCFG_FSB_1333					(7 << 0)	/* hrawclk 333 */
> > -/* Note, below two are guess */
> > -#define CLKCFG_FSB_1600					(4 << 0)	/* hrawclk 400 */
> > -#define CLKCFG_FSB_1600_ALT				(0 << 0)	/* hrawclk 400 */
> > +/*
> > + * Note that on at least on ELK the below value is reported for both
> > + * 333 and 400 MHz BIOS FSB setting, but given that the gmch datasheet
> > + * lists only 200/266/333 MHz FSB as supported let's decode it as 333 MHz.
> > + */
> > +#define CLKCFG_FSB_1333_ALT				(4 << 0)	/* hrawclk 333 */
> >  #define CLKCFG_FSB_MASK					(7 << 0)
> >  #define CLKCFG_MEM_533					(1 << 4)
> >  #define CLKCFG_MEM_667					(2 << 4)
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> > index 763010f8ad89..29792972d55d 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -1808,13 +1808,11 @@ static int g4x_hrawclk(struct drm_i915_private *dev_priv)
> >  	case CLKCFG_FSB_800:
> >  		return 200000;
> >  	case CLKCFG_FSB_1067:
> > +	case CLKCFG_FSB_1067_ALT:
> >  		return 266667;
> >  	case CLKCFG_FSB_1333:
> > +	case CLKCFG_FSB_1333_ALT:
> >  		return 333333;
> > -	/* these two are just a guess; one of them might be right */
> > -	case CLKCFG_FSB_1600:
> > -	case CLKCFG_FSB_1600_ALT:
> > -		return 400000;
> >  	default:
> >  		return 133333;
> >  	}
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC