On Wed, 29 May 2024, Matt Roper <matthew.d.roper@xxxxxxxxx> wrote: > On Tue, May 28, 2024 at 05:24:56PM +0300, Jani Nikula wrote: >> Initialize fsb frequency for more platforms to be able to use it for GT >> clock and rawclk frequency initialization. >> >> Note: There's a discrepancy between existing pnv_fsb_freq() and >> i9xx_hrawclk() regarding CLKCFG interpretation. Presume all PNV is >> mobile. > > Do you just mean we assume PNV always treats CLKCFG the same way mobile > platforms do? Because we have both mobile and non-mobile platforms > defined in the driver (pnv_m_info vs pnv_g_info) and that matches > https://ark.intel.com/content/www/us/en/ark/products/codename/32201/products-formerly-pineview.html > that lists both desktop and mobile. Yeah. The problem is, current code in intel_dram.c and intel_cdclk.c interpret the CLKCFG register differently for desktop PNV. At least one of them is wrong. Basically I just picked one, and secretly hoped Ville would tell me. ;) BR, Jani. > > > Matt > >> >> FIXME: What should the default or failure mode be when the value is >> unknown? >> >> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx> >> --- >> drivers/gpu/drm/i915/soc/intel_dram.c | 54 ++++++++++++++++++++------- >> 1 file changed, 40 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/soc/intel_dram.c b/drivers/gpu/drm/i915/soc/intel_dram.c >> index ace9372244a4..74b5b70e91f9 100644 >> --- a/drivers/gpu/drm/i915/soc/intel_dram.c >> +++ b/drivers/gpu/drm/i915/soc/intel_dram.c >> @@ -142,24 +142,50 @@ static void detect_mem_freq(struct drm_i915_private *i915) >> drm_dbg(&i915->drm, "DDR speed: %d kHz\n", i915->mem_freq); >> } >> >> -static unsigned int pnv_fsb_freq(struct drm_i915_private *i915) >> +static unsigned int i9xx_fsb_freq(struct drm_i915_private *i915) >> { >> u32 fsb; >> >> fsb = intel_uncore_read(&i915->uncore, CLKCFG) & CLKCFG_FSB_MASK; >> >> - switch (fsb) { >> - case CLKCFG_FSB_400: >> - return 400000; >> - case CLKCFG_FSB_533: >> - return 533333; >> - case CLKCFG_FSB_667: >> - return 666667; >> - case CLKCFG_FSB_800: >> - return 800000; >> + if (IS_PINEVIEW(i915) || IS_MOBILE(i915)) { >> + switch (fsb) { >> + case CLKCFG_FSB_400: >> + return 400000; >> + case CLKCFG_FSB_533: >> + return 533333; >> + case CLKCFG_FSB_667: >> + return 666667; >> + case CLKCFG_FSB_800: >> + return 800000; >> + case CLKCFG_FSB_1067: >> + return 1066667; >> + case CLKCFG_FSB_1333: >> + return 1333333; >> + default: >> + MISSING_CASE(fsb); >> + return 1333333; >> + } >> + } else { >> + switch (fsb) { >> + case CLKCFG_FSB_400_ALT: >> + return 400000; >> + case CLKCFG_FSB_533: >> + return 533333; >> + case CLKCFG_FSB_667: >> + return 666667; >> + case CLKCFG_FSB_800: >> + return 800000; >> + case CLKCFG_FSB_1067_ALT: >> + return 1066667; >> + case CLKCFG_FSB_1333_ALT: >> + return 1333333; >> + case CLKCFG_FSB_1600_ALT: >> + return 1600000; >> + default: >> + return 533333; >> + } >> } >> - >> - return 0; >> } >> >> static unsigned int ilk_fsb_freq(struct drm_i915_private *dev_priv) >> @@ -193,8 +219,8 @@ static void detect_fsb_freq(struct drm_i915_private *i915) >> { >> if (GRAPHICS_VER(i915) == 5) >> i915->fsb_freq = ilk_fsb_freq(i915); >> - else if (IS_PINEVIEW(i915)) >> - i915->fsb_freq = pnv_fsb_freq(i915); >> + else if (GRAPHICS_VER(i915) == 3 || GRAPHICS_VER(i915) == 4) >> + i915->fsb_freq = i9xx_fsb_freq(i915); >> >> if (i915->fsb_freq) >> drm_dbg(&i915->drm, "FSB frequency: %d kHz\n", i915->fsb_freq); >> -- >> 2.39.2 >> -- Jani Nikula, Intel
