diff mbox

drm/i915: Add haswell_pcode_write function

Message ID 1415237564-89631-1-git-send-email-Tom.O'Rourke@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tom.O'Rourke@intel.com Nov. 6, 2014, 1:32 a.m. UTC
From: Tom O'Rourke <Tom.O'Rourke@intel.com>

Based on sandybridge_pcode_write, haswell_pcode_write has an
additional field for address control.

Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |    1 +
 drivers/gpu/drm/i915/i915_reg.h |    1 +
 drivers/gpu/drm/i915/intel_pm.c |    9 +++++++--
 3 files changed, 9 insertions(+), 2 deletions(-)

Comments

Ville Syrjala Nov. 6, 2014, 8:28 a.m. UTC | #1
On Wed, Nov 05, 2014 at 05:32:44PM -0800, Tom.O'Rourke@intel.com wrote:
> From: Tom O'Rourke <Tom.O'Rourke@intel.com>
> 
> Based on sandybridge_pcode_write, haswell_pcode_write has an
> additional field for address control.

It's already there in snb.

Do you have an actual use case for this? If so I wonder if we should
just change the mbox parameter to u32 and allow the caller to specify
it all?

> 
> Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |    1 +
>  drivers/gpu/drm/i915/i915_reg.h |    1 +
>  drivers/gpu/drm/i915/intel_pm.c |    9 +++++++--
>  3 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0f00e58..fd8b550 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2950,6 +2950,7 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine);
>  void assert_force_wake_inactive(struct drm_i915_private *dev_priv);
>  
>  int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val);
> +int haswell_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val, u32 control);
>  int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val);
>  
>  /* intel_sideband.c */
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 6fbfdec..b674050 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6011,6 +6011,7 @@ enum punit_power_well {
>  #define   GEN6_DECODE_RC6_VID(vids)		(((vids) * 5) + 245)
>  #define   DISPLAY_IPS_CONTROL			0x19
>  #define	  HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL	0x1A
> +#define   HSW_PCODE_ADDR_CNTL(cntl)		((cntl << 8) & 0x1fffff00)
>  #define GEN6_PCODE_DATA				0x138128
>  #define   GEN6_PCODE_FREQ_IA_RATIO_SHIFT	8
>  #define   GEN6_PCODE_FREQ_RING_RATIO_SHIFT	16
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 1244ff8..9c47bc8 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -7277,7 +7277,7 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val)
>  	return 0;
>  }
>  
> -int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> +int haswell_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val, u32 control)
>  {
>  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
>  
> @@ -7287,7 +7287,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
>  	}
>  
>  	I915_WRITE(GEN6_PCODE_DATA, val);
> -	I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox);
> +	I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox | HSW_PCODE_ADDR_CNTL(control));
>  
>  	if (wait_for((I915_READ(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) == 0,
>  		     500)) {
> @@ -7300,6 +7300,11 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
>  	return 0;
>  }
>  
> +int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> +{
> +	return haswell_pcode_write(dev_priv, mbox, val, 0);
> +}
> +
>  static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
>  {
>  	int div;
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Tom.O'Rourke@intel.com Nov. 6, 2014, 5:35 p.m. UTC | #2
On Thu, Nov 06, 2014 at 10:28:53AM +0200, Ville Syrjälä wrote:
> On Wed, Nov 05, 2014 at 05:32:44PM -0800, Tom.O'Rourke@intel.com wrote:
> > From: Tom O'Rourke <Tom.O'Rourke@intel.com>
> > 
> > Based on sandybridge_pcode_write, haswell_pcode_write has an
> > additional field for address control.
> 
> It's already there in snb.
If the address control field is already there for SNB,
then I would prefer to modify sandybridge_pcode_write
instead of adding a haswell_pcode_write.

I based this patch on the Haswell documentation for
PCU_CR_GTDRIVER_MAILBOX_INTERFACE_0_2_0_GTTMMADR on
page 101 of
https://01.org/linuxgraphics/sites/default/files/documentation/intel-gfx-prm-osrc-hsw-pcie-config-registers.pdf

Can you point me to the SNB documentation for the
address control field?   I do not have documentation
for address control for earlier than Haswell.

> 
> Do you have an actual use case for this? If so I wonder if we should
> just change the mbox parameter to u32 and allow the caller to specify
> it all?
> 
No, I do not have an actual use case for this yet.
I am working on something that will use it but that
is not ready yet.

Yes, we could change mbox to u32 and shift the work
to the caller.  The caller would need to combine the
u8 mailbox command with the address control field.
I do not think that would be better (or worse) but
I can make that change if others want it.

> > 
> > Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |    1 +
> >  drivers/gpu/drm/i915/i915_reg.h |    1 +
> >  drivers/gpu/drm/i915/intel_pm.c |    9 +++++++--
> >  3 files changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 0f00e58..fd8b550 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2950,6 +2950,7 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine);
> >  void assert_force_wake_inactive(struct drm_i915_private *dev_priv);
> >  
> >  int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val);
> > +int haswell_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val, u32 control);
> >  int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val);
> >  
> >  /* intel_sideband.c */
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 6fbfdec..b674050 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6011,6 +6011,7 @@ enum punit_power_well {
> >  #define   GEN6_DECODE_RC6_VID(vids)		(((vids) * 5) + 245)
> >  #define   DISPLAY_IPS_CONTROL			0x19
> >  #define	  HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL	0x1A
> > +#define   HSW_PCODE_ADDR_CNTL(cntl)		((cntl << 8) & 0x1fffff00)
> >  #define GEN6_PCODE_DATA				0x138128
> >  #define   GEN6_PCODE_FREQ_IA_RATIO_SHIFT	8
> >  #define   GEN6_PCODE_FREQ_RING_RATIO_SHIFT	16
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 1244ff8..9c47bc8 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -7277,7 +7277,7 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val)
> >  	return 0;
> >  }
> >  
> > -int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> > +int haswell_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val, u32 control)
> >  {
> >  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> >  
> > @@ -7287,7 +7287,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> >  	}
> >  
> >  	I915_WRITE(GEN6_PCODE_DATA, val);
> > -	I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox);
> > +	I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox | HSW_PCODE_ADDR_CNTL(control));
> >  
> >  	if (wait_for((I915_READ(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) == 0,
> >  		     500)) {
> > @@ -7300,6 +7300,11 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> >  	return 0;
> >  }
> >  
> > +int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> > +{
> > +	return haswell_pcode_write(dev_priv, mbox, val, 0);
> > +}
> > +
> >  static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
> >  {
> >  	int div;
> > -- 
> > 1.7.9.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
Ville Syrjala Nov. 6, 2014, 6:18 p.m. UTC | #3
On Thu, Nov 06, 2014 at 09:35:48AM -0800, O'Rourke, Tom wrote:
> On Thu, Nov 06, 2014 at 10:28:53AM +0200, Ville Syrjälä wrote:
> > On Wed, Nov 05, 2014 at 05:32:44PM -0800, Tom.O'Rourke@intel.com wrote:
> > > From: Tom O'Rourke <Tom.O'Rourke@intel.com>
> > > 
> > > Based on sandybridge_pcode_write, haswell_pcode_write has an
> > > additional field for address control.
> > 
> > It's already there in snb.
> If the address control field is already there for SNB,
> then I would prefer to modify sandybridge_pcode_write
> instead of adding a haswell_pcode_write.

Agreed.

> 
> I based this patch on the Haswell documentation for
> PCU_CR_GTDRIVER_MAILBOX_INTERFACE_0_2_0_GTTMMADR on
> page 101 of
> https://01.org/linuxgraphics/sites/default/files/documentation/intel-gfx-prm-osrc-hsw-pcie-config-registers.pdf
> 
> Can you point me to the SNB documentation for the
> address control field?   I do not have documentation
> for address control for earlier than Haswell.

I found it in BSpec. Not sure it can be found in the PRMs.

> 
> > 
> > Do you have an actual use case for this? If so I wonder if we should
> > just change the mbox parameter to u32 and allow the caller to specify
> > it all?
> > 
> No, I do not have an actual use case for this yet.
> I am working on something that will use it but that
> is not ready yet.
> 
> Yes, we could change mbox to u32 and shift the work
> to the caller.  The caller would need to combine the
> u8 mailbox command with the address control field.
> I do not think that would be better (or worse) but
> I can make that change if others want it.

Well, I don't really mind either way. So unless anyone else objects to
one of the approaches feel free to pick either one.

> 
> > > 
> > > Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h |    1 +
> > >  drivers/gpu/drm/i915/i915_reg.h |    1 +
> > >  drivers/gpu/drm/i915/intel_pm.c |    9 +++++++--
> > >  3 files changed, 9 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 0f00e58..fd8b550 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -2950,6 +2950,7 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine);
> > >  void assert_force_wake_inactive(struct drm_i915_private *dev_priv);
> > >  
> > >  int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val);
> > > +int haswell_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val, u32 control);
> > >  int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val);
> > >  
> > >  /* intel_sideband.c */
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 6fbfdec..b674050 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -6011,6 +6011,7 @@ enum punit_power_well {
> > >  #define   GEN6_DECODE_RC6_VID(vids)		(((vids) * 5) + 245)
> > >  #define   DISPLAY_IPS_CONTROL			0x19
> > >  #define	  HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL	0x1A
> > > +#define   HSW_PCODE_ADDR_CNTL(cntl)		((cntl << 8) & 0x1fffff00)

I don't think we need the mask here. We generally don't have it, with some
exceptions like the csc stuff I did recently which deals in signed values,
and so needs it to avoid the sign bits from clobbering everything above.

But I have been dreaming occasionally about having such macros defined
in a way that would allow us to verify that we don't overflow anything
by accident. Eg. something like this:

#if CONFIG_I915_CHECK_REGISTERS
#define _BITFIELD(value,offset,size) ({ WARN_ON((value) & ~((1 << (size)) - 1)); ((value) << (shift)) })
#else
#define _BITFIELD(value,offset,size) ((value) << (shift))
#endif

#define PCODE_ADDR_CNTL(x) _BITFIELD((x), 8, 21)

But converting everything would be a big task, especially as we don't
have such macros for everything. Sometimes we just have a shift value,
and sometimes not even that and we calculate a shift using some case
specific magic, and sometimes the shift isn't strictly needed (eg.
it's 0, or just happens to match naturally the data we stuff in there
like in the case of page aligned addresses).

> > >  #define GEN6_PCODE_DATA				0x138128
> > >  #define   GEN6_PCODE_FREQ_IA_RATIO_SHIFT	8
> > >  #define   GEN6_PCODE_FREQ_RING_RATIO_SHIFT	16
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 1244ff8..9c47bc8 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -7277,7 +7277,7 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val)
> > >  	return 0;
> > >  }
> > >  
> > > -int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> > > +int haswell_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val, u32 control)
> > >  {
> > >  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> > >  
> > > @@ -7287,7 +7287,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> > >  	}
> > >  
> > >  	I915_WRITE(GEN6_PCODE_DATA, val);
> > > -	I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox);
> > > +	I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox | HSW_PCODE_ADDR_CNTL(control));
> > >  
> > >  	if (wait_for((I915_READ(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) == 0,
> > >  		     500)) {
> > > @@ -7300,6 +7300,11 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> > >  	return 0;
> > >  }
> > >  
> > > +int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> > > +{
> > > +	return haswell_pcode_write(dev_priv, mbox, val, 0);
> > > +}
> > > +
> > >  static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
> > >  {
> > >  	int div;
> > > -- 
> > > 1.7.9.5
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
Tom.O'Rourke@intel.com Nov. 6, 2014, 8:21 p.m. UTC | #4
On Thu, Nov 06, 2014 at 08:18:46PM +0200, Ville Syrjälä wrote:
> On Thu, Nov 06, 2014 at 09:35:48AM -0800, O'Rourke, Tom wrote:
> > On Thu, Nov 06, 2014 at 10:28:53AM +0200, Ville Syrjälä wrote:
> > > On Wed, Nov 05, 2014 at 05:32:44PM -0800, Tom.O'Rourke@intel.com wrote:
> > > > From: Tom O'Rourke <Tom.O'Rourke@intel.com>
> > > > 
> > > > Based on sandybridge_pcode_write, haswell_pcode_write has an
> > > > additional field for address control.
> > > 
> > > It's already there in snb.
> > If the address control field is already there for SNB,
> > then I would prefer to modify sandybridge_pcode_write
> > instead of adding a haswell_pcode_write.
> 
> Agreed.
> 
> > 
> > I based this patch on the Haswell documentation for
> > PCU_CR_GTDRIVER_MAILBOX_INTERFACE_0_2_0_GTTMMADR on
> > page 101 of
> > https://01.org/linuxgraphics/sites/default/files/documentation/intel-gfx-prm-osrc-hsw-pcie-config-registers.pdf
> > 
> > Can you point me to the SNB documentation for the
> > address control field?   I do not have documentation
> > for address control for earlier than Haswell.
> 
> I found it in BSpec. Not sure it can be found in the PRMs.
>
Thank you for the pointer.  I will rewrite the patch.
This current patch should be abandoned.

> > 
> > > 
> > > Do you have an actual use case for this? If so I wonder if we should
> > > just change the mbox parameter to u32 and allow the caller to specify
> > > it all?
> > > 
> > No, I do not have an actual use case for this yet.
> > I am working on something that will use it but that
> > is not ready yet.
> > 
> > Yes, we could change mbox to u32 and shift the work
> > to the caller.  The caller would need to combine the
> > u8 mailbox command with the address control field.
> > I do not think that would be better (or worse) but
> > I can make that change if others want it.
> 
> Well, I don't really mind either way. So unless anyone else objects to
> one of the approaches feel free to pick either one.
> 
> > 
> > > > 
> > > > Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.h |    1 +
> > > >  drivers/gpu/drm/i915/i915_reg.h |    1 +
> > > >  drivers/gpu/drm/i915/intel_pm.c |    9 +++++++--
> > > >  3 files changed, 9 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 0f00e58..fd8b550 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -2950,6 +2950,7 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine);
> > > >  void assert_force_wake_inactive(struct drm_i915_private *dev_priv);
> > > >  
> > > >  int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val);
> > > > +int haswell_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val, u32 control);
> > > >  int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val);
> > > >  
> > > >  /* intel_sideband.c */
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > index 6fbfdec..b674050 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -6011,6 +6011,7 @@ enum punit_power_well {
> > > >  #define   GEN6_DECODE_RC6_VID(vids)		(((vids) * 5) + 245)
> > > >  #define   DISPLAY_IPS_CONTROL			0x19
> > > >  #define	  HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL	0x1A
> > > > +#define   HSW_PCODE_ADDR_CNTL(cntl)		((cntl << 8) & 0x1fffff00)
> 
> I don't think we need the mask here. We generally don't have it, with some
> exceptions like the csc stuff I did recently which deals in signed values,
> and so needs it to avoid the sign bits from clobbering everything above.
> 
> But I have been dreaming occasionally about having such macros defined
> in a way that would allow us to verify that we don't overflow anything
> by accident. Eg. something like this:
> 
> #if CONFIG_I915_CHECK_REGISTERS
> #define _BITFIELD(value,offset,size) ({ WARN_ON((value) & ~((1 << (size)) - 1)); ((value) << (shift)) })
> #else
> #define _BITFIELD(value,offset,size) ((value) << (shift))
> #endif
> 
> #define PCODE_ADDR_CNTL(x) _BITFIELD((x), 8, 21)
> 
> But converting everything would be a big task, especially as we don't
> have such macros for everything. Sometimes we just have a shift value,
> and sometimes not even that and we calculate a shift using some case
> specific magic, and sometimes the shift isn't strictly needed (eg.
> it's 0, or just happens to match naturally the data we stuff in there
> like in the case of page aligned addresses).
> 
I like the CONFIG_I915_CHECK_REGISTERS idea.  Until we
have something like that, I would like to keep the mask.
In addition to adding a little safety, the mask makes it
clear in the code how big the field should be.

> > > >  #define GEN6_PCODE_DATA				0x138128
> > > >  #define   GEN6_PCODE_FREQ_IA_RATIO_SHIFT	8
> > > >  #define   GEN6_PCODE_FREQ_RING_RATIO_SHIFT	16
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > index 1244ff8..9c47bc8 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -7277,7 +7277,7 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> > > > +int haswell_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val, u32 control)
> > > >  {
> > > >  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> > > >  
> > > > @@ -7287,7 +7287,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> > > >  	}
> > > >  
> > > >  	I915_WRITE(GEN6_PCODE_DATA, val);
> > > > -	I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox);
> > > > +	I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox | HSW_PCODE_ADDR_CNTL(control));
> > > >  
> > > >  	if (wait_for((I915_READ(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) == 0,
> > > >  		     500)) {
> > > > @@ -7300,6 +7300,11 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
> > > > +{
> > > > +	return haswell_pcode_write(dev_priv, mbox, val, 0);
> > > > +}
> > > > +
> > > >  static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
> > > >  {
> > > >  	int div;
> > > > -- 
> > > > 1.7.9.5
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel OTC
> 
> -- 
> Ville Syrjälä
> Intel OTC
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0f00e58..fd8b550 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2950,6 +2950,7 @@  void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine);
 void assert_force_wake_inactive(struct drm_i915_private *dev_priv);
 
 int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val);
+int haswell_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val, u32 control);
 int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val);
 
 /* intel_sideband.c */
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 6fbfdec..b674050 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6011,6 +6011,7 @@  enum punit_power_well {
 #define   GEN6_DECODE_RC6_VID(vids)		(((vids) * 5) + 245)
 #define   DISPLAY_IPS_CONTROL			0x19
 #define	  HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL	0x1A
+#define   HSW_PCODE_ADDR_CNTL(cntl)		((cntl << 8) & 0x1fffff00)
 #define GEN6_PCODE_DATA				0x138128
 #define   GEN6_PCODE_FREQ_IA_RATIO_SHIFT	8
 #define   GEN6_PCODE_FREQ_RING_RATIO_SHIFT	16
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1244ff8..9c47bc8 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7277,7 +7277,7 @@  int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val)
 	return 0;
 }
 
-int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
+int haswell_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val, u32 control)
 {
 	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
 
@@ -7287,7 +7287,7 @@  int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
 	}
 
 	I915_WRITE(GEN6_PCODE_DATA, val);
-	I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox);
+	I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox | HSW_PCODE_ADDR_CNTL(control));
 
 	if (wait_for((I915_READ(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) == 0,
 		     500)) {
@@ -7300,6 +7300,11 @@  int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
 	return 0;
 }
 
+int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u8 mbox, u32 val)
+{
+	return haswell_pcode_write(dev_priv, mbox, val, 0);
+}
+
 static int byt_gpu_freq(struct drm_i915_private *dev_priv, int val)
 {
 	int div;