Re: [PATCH 3/7] drm/i915/chv: Enable RPS (Turbo) for Cherryview

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

On Mon, May 26, 2014 at 07:24:21PM +0530, Deepak S wrote:
> 
> On Monday 26 May 2014 07:00 PM, Mika Kuoppala wrote:
> > Hi Deepak,
> >
> > deepak.s@linux.intel.com writes:
> >
> >> From: Deepak S <deepak.s@linux.intel.com>
> >>
> >> v2: Disable media turbo and Add DOWN_IDLE_AVG support (Ville)
> >>
> >> v3: Mass rename of the dev_priv->rps variables in upstream.
> >>
> >> v4: Rebase against latest code. (Deepak)
> >>
> >> v5: Rebase against latest nightly code. (Deepak)
> >>
> >> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> ---
> >>   drivers/gpu/drm/i915/i915_drv.h       |  1 +
> >>   drivers/gpu/drm/i915/i915_reg.h       | 10 ++++
> >>   drivers/gpu/drm/i915/intel_pm.c       | 95 ++++++++++++++++++++++++++++++++++-
> >>   drivers/gpu/drm/i915/intel_sideband.c | 14 ++++++
> >>   4 files changed, 119 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> index 0412b12..5f0e338 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -2621,6 +2621,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> >>   u32 vlv_punit_read(struct drm_i915_private *dev_priv, u8 addr);
> >>   void vlv_punit_write(struct drm_i915_private *dev_priv, u8 addr, u32 val);
> >>   u32 vlv_nc_read(struct drm_i915_private *dev_priv, u8 addr);
> >> +u32 chv_nc_read(struct drm_i915_private *dev_priv, u8 addr);
> >>   u32 vlv_gpio_nc_read(struct drm_i915_private *dev_priv, u32 reg);
> >>   void vlv_gpio_nc_write(struct drm_i915_private *dev_priv, u32 reg, u32 val);
> >>   u32 vlv_cck_read(struct drm_i915_private *dev_priv, u32 reg);
> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >> index c1f36a5..37f4b12 100644
> >> --- a/drivers/gpu/drm/i915/i915_reg.h
> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> @@ -487,6 +487,7 @@
> >>   #define VLV_IOSF_DATA				(VLV_DISPLAY_BASE + 0x2104)
> >>   #define VLV_IOSF_ADDR				(VLV_DISPLAY_BASE + 0x2108)
> >>   
> >> +#define   CHV_IOSF_PORT_NC			0x04
> > Use IOSF_PORT_PUNIT instead of defining this?
> 
> Yes, Agreed, I will address this
> 
> >>   /* See configdb bunit SB addr map */
> >>   #define BUNIT_REG_BISOC				0x11
> >>
> >> @@ -529,6 +530,14 @@ enum punit_power_well {
> >>   #define PUNIT_FUSE_BUS2				0xf6 /* bits 47:40 */
> >>   #define PUNIT_FUSE_BUS1				0xf5 /* bits 55:48 */
> >>   
> >> +#define CHV_IOSF_NC_FB_GFX_FREQ_FUSE		0xdb
> >> +#define CHV_FB_GFX_MAX_FREQ_FUSE_SHIFT		16
> >> +#define CHV_FB_GFX_MAX_FREQ_FUSE_MASK		0xff
> >> +
> >> +#define CHV_IOSF_NC_FB_GFX_RPE_FUSE		0xdf
> >> +#define CHV_FB_RPE_FREQ_SHIFT			8
> >> +#define CHV_FB_RPE_FREQ_MASK			0xff
> >> +
> > These seem to be also part of punit space so I would prefer:
> > PUNIT_REG_GPU_STATUS                    0xdb
> >    PUNIT_GPU_STATUS_MAX_FREQ_SHIFT	16
> >    PUNIT_GPU_STATUS_MAX_FREQ_MASK	0xff
> > PUNIT_REG_GPU_DUTYCYCLE                 0xdf
> >
> > etc...
> 
> I can change. Q? don't we want to identify the register with CHV?
> 
> >>   #define IOSF_NC_FB_GFX_FREQ_FUSE		0x1c
> >>   #define   FB_GFX_MAX_FREQ_FUSE_SHIFT		3
> >>   #define   FB_GFX_MAX_FREQ_FUSE_MASK		0x000007f8
> >> @@ -933,6 +942,7 @@ enum punit_power_well {
> >>   #define   SANDYBRIDGE_FENCE_PITCH_SHIFT	32
> >>   #define   GEN7_FENCE_MAX_PITCH_VAL	0x0800
> >>   
> >> +
> >>   /* control register for cpu gtt access */
> >>   #define TILECTL				0x101000
> >>   #define   TILECTL_SWZCTL			(1 << 0)
> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >> index 1816c52..08dcdc5 100644
> >> --- a/drivers/gpu/drm/i915/intel_pm.c
> >> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >> @@ -3731,6 +3731,38 @@ void gen6_update_ring_freq(struct drm_device *dev)
> >>   	mutex_unlock(&dev_priv->rps.hw_lock);
> >>   }
> >>   
> >> +int cherryview_rps_max_freq(struct drm_i915_private *dev_priv)
> >> +{
> >> +	u32 val, rp0;
> >> +
> >> +	val = chv_nc_read(dev_priv, CHV_IOSF_NC_FB_GFX_FREQ_FUSE);
> >> +
> > I didn't find any reason we couldn't use vlv_punit_read().
> 
> I am adding separate function to be inline with VLV. If needed we can modify both VLV and CHV
> I would prefer to keep distinguish between fuse and punit read.

If the register is in the punit you should use the punit funcs. If
there's something special about those registers just add a comment
which explains it.

The whole nc unit seems to have disappeared in CHV, so it's rather
confusing when you see NC being mentioned and then you go digging
through the docs and can't find anything like it.

-- 
Ville Syrjälä
Intel OTC