From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 1/2] drm/i915: Get CZ clock for VLV Date: Tue, 5 Aug 2014 18:40:37 +0300 Message-ID: <20140805154037.GO4193@intel.com> References: <1407172445-5730-1-git-send-email-vandana.kannan@intel.com> <20140805130958.GL4193@intel.com> <53E0F7CB.509@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 627D76E21B for ; Tue, 5 Aug 2014 08:41:00 -0700 (PDT) Content-Disposition: inline In-Reply-To: <53E0F7CB.509@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Vandana Kannan Cc: "intel-gfx@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org On Tue, Aug 05, 2014 at 08:57:07PM +0530, Vandana Kannan wrote: > On Aug-05-2014 6:39 PM, Ville Syrj=E4l=E4 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 > >> --- > >> 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/i9= 15_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/i9= 15_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/i9= 15/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(s= truct 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 =3D dev->dev_private; > >> + int vco =3D valleyview_get_vco(dev_priv); > >> + u32 val; > >> + int divider; > >> + > >> + mutex_lock(&dev_priv->dpio_lock); > >> + val =3D vlv_cck_read(dev_priv, CCK_CZ_CONTROL); > >> + mutex_unlock(&dev_priv->dpio_lock); > >> + > >> + divider =3D val & DISPLAY_FREQUENCY_VALUES; > >> + > >> + WARN((val & DISPLAY_FREQUENCY_STATUS) !=3D > >> + (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, "cdcl= k"); > > } > > = > 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@lists.freedesktop.org > >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > = -- = Ville Syrj=E4l=E4 Intel OTC