On Thu, 04 May 2017, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxx>
>
> Cc: stable@xxxxxxxxxxxxxxx
> Cc: Tomi Sarvela <tomi.p.sarvela@xxxxxxxxx>
> Reported-by: Tomi Sarvela <tomi.p.sarvela@xxxxxxxxx>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100926
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> 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