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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 04 May 2017, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@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...

As such can't review, but

Acked-by: Jani Nikula <jani.nikula@intel.com>

>
> Cc: stable@vger.kernel.org
> Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
> Reported-by: Tomi Sarvela <tomi.p.sarvela@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100926
> Signed-off-by: Ville Syrjälä <ville.syrjala@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