Re: [PATCH 1/2] drm/i915: Get CZ clock for VLV

[Vandana Kannan <vandana.kannan@intel.com>]

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@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 *?

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