Re: [PATCH 07/12] drm/i915: extend the fsb_freq initialization to more platforms

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Jun 04, 2024 at 02:46:18PM +0300, Jani Nikula wrote:
> On Thu, 30 May 2024, Jani Nikula <jani.nikula@intel.com> wrote:
> > On Wed, 29 May 2024, Matt Roper <matthew.d.roper@intel.com> 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. ;)
> 
> Ville, do you have any idea about CLKCFG?

Acording to the datasheet PNV only really supports the 667MHz
option, so technically might not even matter. Maybe there's some
way to force it to use a different clock, but at least one "desktop"
PNV I looked at didn't have any knobs in the BIOS for this.

Anyways, based on past experience PNV always looks like a mobile
part, despite potentially being itself considered the "desktop"
variant. That interpretation also matches the existing
pnv_detect_mem_freq() implementation. So from that POV the
patch looks correct to me.

> 
> BR,
> Jani.
> 
> 
> >
> > BR,
> > Jani.
> >
> >
> >>
> >>
> >> Matt
> >>
> >>> 
> >>> FIXME: What should the default or failure mode be when the value is
> >>> unknown?
> >>> 
> >>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >>> ---
> >>>  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

-- 
Ville Syrjälä
Intel