[Intel-gfx] [PATCH 1/2] drm/i915: Get CZ clock for VLV

[Ville Syrjälä]

On Tue, Aug 05, 2014 at 08:57:07PM +0530, Vandana Kannan wrote:
> On Aug-05-2014 6:39 PM, Ville Syrjälä wrote:
> > On Mon, Aug 04, 2014 at 10:44:04PM +0530, Vandana Kannan wrote:
> >> CZ clock is related to data flow from memory to display plane. This is
> >> required for comparison with CD clock before programming PFI credits.
> >>
> >> Signed-off-by: Vandana Kannan <vandana.kannan at intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_drv.h      |  2 ++
> >>  drivers/gpu/drm/i915/i915_reg.h      |  1 +
> >>  drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++++++++++++
> >>  3 files changed, 23 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> index 174a294..baeb56f 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -2764,6 +2764,8 @@ void vlv_flisdsi_write(struct drm_i915_private *dev_priv, u32 reg, u32 val);
> >>  int vlv_gpu_freq(struct drm_i915_private *dev_priv, int val);
> >>  int vlv_freq_opcode(struct drm_i915_private *dev_priv, int val);
> >>  
> >> +int valleyview_get_cz_clock_speed(struct drm_device *dev);
> >> +
> >>  #define FORCEWAKE_RENDER	(1 << 0)
> >>  #define FORCEWAKE_MEDIA		(1 << 1)
> >>  #define FORCEWAKE_ALL		(FORCEWAKE_RENDER | FORCEWAKE_MEDIA)
> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >> index fe5c276..1b8f095 100644
> >> --- a/drivers/gpu/drm/i915/i915_reg.h
> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> @@ -601,6 +601,7 @@ enum punit_power_well {
> >>  #define  DISPLAY_FREQUENCY_STATUS		(0x1f << 8)
> >>  #define  DISPLAY_FREQUENCY_STATUS_SHIFT		8
> >>  #define  DISPLAY_FREQUENCY_VALUES		(0x1f << 0)
> >> +#define CCK_CZ_CONTROL				0x62
> > 
> > Please move the reg define to just above DISPLAY_CLOCK_CONTROL to keep
> > things in neat order.
> > 
> Ok, will make this change..
> 
> > Also it might be a good idea to rename the DISPLAY_TRUNK_* and
> > DISPLAY_FREQUENCY_* bits to CCK_... instead of DISPLAY_... to make
> > it clear they apply to all CCK clock control registers.
> > 
> Will submit this renaming part as a separate patch..
> >>  
> >>  /**
> >>   * DOC: DPIO
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 99eb7ca..56a8090 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -5278,6 +5278,26 @@ static int valleyview_get_display_clock_speed(struct drm_device *dev)
> >>  	return DIV_ROUND_CLOSEST(vco << 1, divider + 1);
> >>  }
> >>  
> >> +int valleyview_get_cz_clock_speed(struct drm_device *dev)
> >> +{
> >> +	struct drm_i915_private *dev_priv = dev->dev_private;
> >> +	int vco = valleyview_get_vco(dev_priv);
> >> +	u32 val;
> >> +	int divider;
> >> +
> >> +	mutex_lock(&dev_priv->dpio_lock);
> >> +	val = vlv_cck_read(dev_priv, CCK_CZ_CONTROL);
> >> +	mutex_unlock(&dev_priv->dpio_lock);
> >> +
> >> +	divider = val & DISPLAY_FREQUENCY_VALUES;
> >> +
> >> +	WARN((val & DISPLAY_FREQUENCY_STATUS) !=
> >> +	     (divider << DISPLAY_FREQUENCY_STATUS_SHIFT),
> >> +	     "czclk change in progress\n");
> >> +
> >> +	return DIV_ROUND_CLOSEST(vco << 1, divider + 1);
> >> +}
> > 
> > We might want to refactor this a bit. Eg.:
> > 
> > static int valleyview_get_cck_clock_speed(u32 reg, const char *name)
> > {
> >   ... do the cck read etc. from CCK clock control register 'reg'
> > }
> > 
> > static int valleyview_get_cz_clock_speed()
> > {
> > 	return valleyview_get_cck_clock_speed(CCK_CZ_CLOCK_CONTROL, "czclk");
> > }
> > 
> > static int valleyview_get_display_clock_speed()
> > {
> > 	...
> > 	return valleyview_get_cck_clock_speed(CCK_DISPLAY_CLOCK_CONTROL, "cdclk");
> > }
> > 
> As part of the above refactoring, how about having an enum to specify
> CDCLK or CZCLK instead of char *?

We had such a thing in intel_i2c.c until I killed it here:
drm/i915: Kill duplicated cdclk readout code from i2c

Personally I don't see much point in having it since we'd just have
these few users. But if you want to add it I don't mind too much either.

> 
> - Vandana
> >> +
> >>  static int i945_get_display_clock_speed(struct drm_device *dev)
> >>  {
> >>  	return 400000;
> >> -- 
> >> 2.0.1
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx at lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 

-- 
Ville Syrjälä
Intel OTC