Re: [PATCH 07/11] drm/i915: Store rawclk_freq in dev_priv

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

On Mon, 30 Nov 2015, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Generalize rawclk handling by storing it in dev_priv.
>
> Presumably our hrawclk readout works at least for CTG and ELK
> since we've been using it for DP AUX on those platforms. There
> are no real docs anymore after configdb vanished, so the only
> reference is the public CTG GMCH spec. What bits are listed in
> that doc match our code. The ELK GMCH spec have no relevant
> details unfortunately.
>
> The PNV situation is less clear. Starting from
> commit aa17cdb4f836 ("drm/i915: initialize backlight max from VBT")
> we assume that the CTG/ELK hrawclk readout works for PNV as well.
> At least the results *seem* reasonable for one PNV machine (Lenovo
> Ideapad S10-3t). Sadly the PNV GMCH spec doesn't have the goods on
> the relevant register either.
>
> So let's keep assuming it works for PNV,ELK,CTG and read it out on
> those platforms. G33 also has hrawclk according to some notes
> in BSpec, but we don't actually need it for anything, so let's not
> even try to read it out there.

Looks sensible but moderately scary, did not review properly yet,
however a few nitpicks below.

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  1 +
>  drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++--------------
>  drivers/gpu/drm/i915/intel_dp.c      | 16 +++++------
>  drivers/gpu/drm/i915/intel_drv.h     |  2 --
>  drivers/gpu/drm/i915/intel_panel.c   | 24 ++++++++--------
>  5 files changed, 53 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9ab3e25ddf38..64facd410037 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1781,6 +1781,7 @@ struct drm_i915_private {
>  	unsigned int skl_boot_cdclk;
>  	unsigned int cdclk_freq, max_cdclk_freq;
>  	unsigned int max_dotclk_freq;
> +	unsigned int rawclk_freq;
>  	unsigned int hpll_freq;
>  	unsigned int czclk_freq;
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 84395c8c9dce..84a1359ecba5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -168,49 +168,61 @@ static int vlv_get_cck_clock_hpll(struct drm_i915_private *dev_priv,
>  	return DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, divider + 1);
>  }
>  
> -int
> -intel_pch_rawclk(struct drm_device *dev)
> +static int
> +intel_pch_rawclk(struct drm_i915_private *dev_priv)

I guess intel_ prefix would not be needed anymore. *shrug*.

>  {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -
> -	WARN_ON(!HAS_PCH_SPLIT(dev));
> +	return (I915_READ(PCH_RAWCLK_FREQ) & RAWCLK_FREQ_MASK) * 1000;
> +}
>  
> -	return I915_READ(PCH_RAWCLK_FREQ) & RAWCLK_FREQ_MASK;
> +static int
> +intel_vlv_hrawclk(struct drm_i915_private *dev_priv)
> +{
> +	return 200000;
>  }
>  
> -/* hrawclock is 1/4 the FSB frequency */
> -int intel_hrawclk(struct drm_device *dev)
> +static int
> +intel_g4x_hrawclk(struct drm_i915_private *dev_priv)
>  {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	uint32_t clkcfg;
>  
> -	/* There is no CLKCFG reg in Valleyview. VLV hrawclk is 200 MHz */
> -	if (IS_VALLEYVIEW(dev))
> -		return 200;
> -
> +	/* hrawclock is 1/4 the FSB frequency */
>  	clkcfg = I915_READ(CLKCFG);
>  	switch (clkcfg & CLKCFG_FSB_MASK) {
>  	case CLKCFG_FSB_400:
> -		return 100;
> +		return 100000;
>  	case CLKCFG_FSB_533:
> -		return 133;
> +		return 133333;
>  	case CLKCFG_FSB_667:
> -		return 166;
> +		return 166667;
>  	case CLKCFG_FSB_800:
> -		return 200;
> +		return 200000;
>  	case CLKCFG_FSB_1067:
> -		return 266;
> +		return 266667;
>  	case CLKCFG_FSB_1333:
> -		return 333;
> +		return 333333;
>  	/* these two are just a guess; one of them might be right */
>  	case CLKCFG_FSB_1600:
>  	case CLKCFG_FSB_1600_ALT:
> -		return 400;
> +		return 400000;
>  	default:
> -		return 133;
> +		return 133333;
>  	}
>  }
>  
> +static void intel_update_rawclk(struct drm_i915_private *dev_priv)
> +{
> +	if (HAS_PCH_SPLIT(dev_priv))
> +		dev_priv->rawclk_freq = intel_pch_rawclk(dev_priv);
> +	else if (IS_VALLEYVIEW(dev_priv))
> +		dev_priv->rawclk_freq = intel_vlv_hrawclk(dev_priv);
> +	else if (IS_G4X(dev_priv) || IS_PINEVIEW(dev_priv))
> +		dev_priv->rawclk_freq = intel_g4x_hrawclk(dev_priv);
> +	else
> +		return; /* no rawclk on other platforms, or no need to know it */
> +
> +	DRM_DEBUG_DRIVER("rawclk rate: %d kHz\n", dev_priv->rawclk_freq);
> +}
> +
>  static void intel_update_czclk(struct drm_i915_private *dev_priv)
>  {
>  	if (!IS_VALLEYVIEW(dev_priv))
> @@ -15145,6 +15157,7 @@ void intel_modeset_init(struct drm_device *dev)
>  	}
>  
>  	intel_update_czclk(dev_priv);
> +	intel_update_rawclk(dev_priv);
>  	intel_update_cdclk(dev);
>  
>  	intel_shared_dpll_init(dev);
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f335c92b4fa7..2a9b5710ee83 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -675,13 +675,13 @@ intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
>  static uint32_t i9xx_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
>  {
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> -	struct drm_device *dev = intel_dig_port->base.base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
>  
>  	/*
>  	 * The clock divider is based off the hrawclk, and would like to run at
>  	 * 2MHz.  So, take the hrawclk value and divide by 2 and use that
>  	 */

The comment needs updating. Possibly elsewhere too outside of patch
context.

> -	return index ? 0 : DIV_ROUND_CLOSEST(intel_hrawclk(dev), 2);
> +	return index ? 0 : DIV_ROUND_CLOSEST(dev_priv->rawclk_freq, 2000);
>  }
>  
>  static uint32_t ilk_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
> @@ -693,12 +693,10 @@ static uint32_t ilk_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
>  	if (index)
>  		return 0;
>  
> -	if (intel_dig_port->port == PORT_A) {
> +	if (intel_dig_port->port == PORT_A)
>  		return DIV_ROUND_CLOSEST(dev_priv->cdclk_freq, 2000);
> -
> -	} else {
> -		return DIV_ROUND_CLOSEST(intel_pch_rawclk(dev), 2);
> -	}
> +	else
> +		return DIV_ROUND_CLOSEST(dev_priv->rawclk_freq, 2000);
>  }
>  
>  static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
> @@ -719,7 +717,7 @@ static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
>  		default: return 0;
>  		}
>  	} else  {
> -		return index ? 0 : DIV_ROUND_CLOSEST(intel_pch_rawclk(dev), 2);
> +		return index ? 0 : DIV_ROUND_CLOSEST(dev_priv->rawclk_freq, 2000);
>  	}
>  }
>  
> @@ -5237,7 +5235,7 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	u32 pp_on, pp_off, pp_div, port_sel = 0;
> -	int div = HAS_PCH_SPLIT(dev) ? intel_pch_rawclk(dev) : intel_hrawclk(dev);
> +	int div = dev_priv->rawclk_freq / 1000;
>  	i915_reg_t pp_on_reg, pp_off_reg, pp_div_reg, pp_ctrl_reg;
>  	enum port port = dp_to_dig_port(intel_dp)->port;
>  	const struct edp_power_seq *seq = &intel_dp->pps_delays;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1ffd8d5c3235..54b65951622d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1050,8 +1050,6 @@ void i915_audio_component_cleanup(struct drm_i915_private *dev_priv);
>  /* intel_display.c */
>  extern const struct drm_plane_funcs intel_plane_funcs;
>  bool intel_has_pending_fb_unpin(struct drm_device *dev);
> -int intel_pch_rawclk(struct drm_device *dev);
> -int intel_hrawclk(struct drm_device *dev);
>  void intel_mark_busy(struct drm_device *dev);
>  void intel_mark_idle(struct drm_device *dev);
>  void intel_crtc_restore_mode(struct drm_crtc *crtc);
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index 891a587225e2..b8c78d885311 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1314,8 +1314,8 @@ static u32 lpt_hz_to_pwm(struct intel_connector *connector, u32 pwm_freq_hz)
>   */
>  static u32 pch_hz_to_pwm(struct intel_connector *connector, u32 pwm_freq_hz)
>  {
> -	struct drm_device *dev = connector->base.dev;
> -	int clock = MHz(intel_pch_rawclk(dev));
> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> +	int clock = 1000 * dev_priv->rawclk_freq;
>  
>  	return clock / (pwm_freq_hz * 128);

Could use:

	return KHz(dev_priv->rawclk_freq) / (pwm_freq_hz * 128);

Same thing all around.

Side note, maybe these should also round closest (follow-up).


>  }
> @@ -1330,12 +1330,11 @@ static u32 pch_hz_to_pwm(struct intel_connector *connector, u32 pwm_freq_hz)
>   */
>  static u32 i9xx_hz_to_pwm(struct intel_connector *connector, u32 pwm_freq_hz)
>  {
> -	struct drm_device *dev = connector->base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>  	int clock;
>  
> -	if (IS_PINEVIEW(dev))
> -		clock = MHz(intel_hrawclk(dev));
> +	if (IS_PINEVIEW(dev_priv))
> +		clock = 1000 * dev_priv->rawclk_freq;
>  	else
>  		clock = 1000 * dev_priv->cdclk_freq;
>  
> @@ -1354,7 +1353,7 @@ static u32 i965_hz_to_pwm(struct intel_connector *connector, u32 pwm_freq_hz)
>  	int clock;
>  
>  	if (IS_G4X(dev_priv))
> -		clock = MHz(intel_hrawclk(dev));
> +		clock = 1000 * dev_priv->rawclk_freq;
>  	else
>  		clock = 1000 * dev_priv->cdclk_freq;
>  
> @@ -1368,18 +1367,17 @@ static u32 i965_hz_to_pwm(struct intel_connector *connector, u32 pwm_freq_hz)
>   */
>  static u32 vlv_hz_to_pwm(struct intel_connector *connector, u32 pwm_freq_hz)
>  {
> -	struct drm_device *dev = connector->base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	int clock;
> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>  
>  	if ((I915_READ(CBR1_VLV) & CBR_PWM_CLOCK_MUX_SELECT) == 0) {
> -		if (IS_CHERRYVIEW(dev))
> +		if (IS_CHERRYVIEW(dev_priv))
>  			return KHz(19200) / (pwm_freq_hz * 16);
>  		else
>  			return MHz(25) / (pwm_freq_hz * 16);
>  	} else {
> -		clock = intel_hrawclk(dev);
> -		return MHz(clock) / (pwm_freq_hz * 128);
> +		int clock = 1000 * dev_priv->rawclk_freq;
> +
> +		return clock / (pwm_freq_hz * 128);
>  	}
>  }

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx