Re: [PATCH] drm/i915: Correct the IOSF Dev_FN field for IOSF transfers

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 05 Feb 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Feb 05, 2015 at 05:10:56PM +0530, Shobhit Kumar wrote:
>> As per the specififcation, the SB_DevFn is the PCI_DEVFN of the target
>> device and not the source. So PCI_DEVFN(2,0) is not correct. Further the
>> port ID should be enough to identify devices unless they are MFD. The
>> SB_DevFn was intended to remove ambiguity in case of these MFD devices.
>> 
>> For non MFD devices the recommendation for the target device IP was to
>> ignore these fields, but not all of them followed the recommendation.
>> Some like CCK ignore these fields and hence PCI_DEVFN(2, 0) works and so
>> does PCI_DEVFN(0, 0) as it works for DPIO. The issue came to light because
>> of GPIONC which was not getting programmed correctly with PCI_DEVFN(2, 0).
>> It turned out that this did not follow the recommendation and expected 0
>> in this field.
>> 
>> In general the recommendation is to use SB_DevFn as PCI_DEVFN(0, 0) for
>> all devices except target PCI devices.
>> 
>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>
> Yep, confirmed on my BYT that GPIONC doesn't like the devfn 2.0. On BSW
> GPIONC seems to have become more relaxed and accepts devfn 2.0 as well.
>
> I also tried to poke at all the other sideband units we care about and
> they appear to work just as well with 2.0 and 0.0. But being consistent
> seems like a good idea, so 0.0 it is for everyone.

Shobhit, do you have any idea if there are devices out there that would
really need this, i.e. should we backport this to stable kernels or is
it good enough for next?

BR,
Jani.


>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>> ---
>>  drivers/gpu/drm/i915/intel_sideband.c | 26 +++++++++++++-------------
>>  1 file changed, 13 insertions(+), 13 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
>> index 01d841e..731b10a 100644
>> --- a/drivers/gpu/drm/i915/intel_sideband.c
>> +++ b/drivers/gpu/drm/i915/intel_sideband.c
>> @@ -82,7 +82,7 @@ u32 vlv_punit_read(struct drm_i915_private *dev_priv, u8 addr)
>>  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
>>  
>>  	mutex_lock(&dev_priv->dpio_lock);
>> -	vlv_sideband_rw(dev_priv, PCI_DEVFN(2, 0), IOSF_PORT_PUNIT,
>> +	vlv_sideband_rw(dev_priv, PCI_DEVFN(0, 0), IOSF_PORT_PUNIT,
>>  			SB_CRRDDA_NP, addr, &val);
>>  	mutex_unlock(&dev_priv->dpio_lock);
>>  
>> @@ -94,7 +94,7 @@ void vlv_punit_write(struct drm_i915_private *dev_priv, u8 addr, u32 val)
>>  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
>>  
>>  	mutex_lock(&dev_priv->dpio_lock);
>> -	vlv_sideband_rw(dev_priv, PCI_DEVFN(2, 0), IOSF_PORT_PUNIT,
>> +	vlv_sideband_rw(dev_priv, PCI_DEVFN(0, 0), IOSF_PORT_PUNIT,
>>  			SB_CRWRDA_NP, addr, &val);
>>  	mutex_unlock(&dev_priv->dpio_lock);
>>  }
>> @@ -103,7 +103,7 @@ u32 vlv_bunit_read(struct drm_i915_private *dev_priv, u32 reg)
>>  {
>>  	u32 val = 0;
>>  
>> -	vlv_sideband_rw(dev_priv, PCI_DEVFN(2, 0), IOSF_PORT_BUNIT,
>> +	vlv_sideband_rw(dev_priv, PCI_DEVFN(0, 0), IOSF_PORT_BUNIT,
>>  			SB_CRRDDA_NP, reg, &val);
>>  
>>  	return val;
>> @@ -111,7 +111,7 @@ u32 vlv_bunit_read(struct drm_i915_private *dev_priv, u32 reg)
>>  
>>  void vlv_bunit_write(struct drm_i915_private *dev_priv, u32 reg, u32 val)
>>  {
>> -	vlv_sideband_rw(dev_priv, PCI_DEVFN(2, 0), IOSF_PORT_BUNIT,
>> +	vlv_sideband_rw(dev_priv, PCI_DEVFN(0, 0), IOSF_PORT_BUNIT,
>>  			SB_CRWRDA_NP, reg, &val);
>>  }
>>  
>> @@ -122,7 +122,7 @@ u32 vlv_nc_read(struct drm_i915_private *dev_priv, u8 addr)
>>  	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
>>  
>>  	mutex_lock(&dev_priv->dpio_lock);
>> -	vlv_sideband_rw(dev_priv, PCI_DEVFN(2, 0), IOSF_PORT_NC,
>> +	vlv_sideband_rw(dev_priv, PCI_DEVFN(0, 0), IOSF_PORT_NC,
>>  			SB_CRRDDA_NP, addr, &val);
>>  	mutex_unlock(&dev_priv->dpio_lock);
>>  
>> @@ -132,56 +132,56 @@ u32 vlv_nc_read(struct drm_i915_private *dev_priv, u8 addr)
>>  u32 vlv_gpio_nc_read(struct drm_i915_private *dev_priv, u32 reg)
>>  {
>>  	u32 val = 0;
>> -	vlv_sideband_rw(dev_priv, PCI_DEVFN(2, 0), IOSF_PORT_GPIO_NC,
>> +	vlv_sideband_rw(dev_priv, PCI_DEVFN(0, 0), IOSF_PORT_GPIO_NC,
>>  			SB_CRRDDA_NP, reg, &val);
>>  	return val;
>>  }
>>  
>>  void vlv_gpio_nc_write(struct drm_i915_private *dev_priv, u32 reg, u32 val)
>>  {
>> -	vlv_sideband_rw(dev_priv, PCI_DEVFN(2, 0), IOSF_PORT_GPIO_NC,
>> +	vlv_sideband_rw(dev_priv, PCI_DEVFN(0, 0), IOSF_PORT_GPIO_NC,
>>  			SB_CRWRDA_NP, reg, &val);
>>  }
>>  
>>  u32 vlv_cck_read(struct drm_i915_private *dev_priv, u32 reg)
>>  {
>>  	u32 val = 0;
>> -	vlv_sideband_rw(dev_priv, PCI_DEVFN(2, 0), IOSF_PORT_CCK,
>> +	vlv_sideband_rw(dev_priv, PCI_DEVFN(0, 0), IOSF_PORT_CCK,
>>  			SB_CRRDDA_NP, reg, &val);
>>  	return val;
>>  }
>>  
>>  void vlv_cck_write(struct drm_i915_private *dev_priv, u32 reg, u32 val)
>>  {
>> -	vlv_sideband_rw(dev_priv, PCI_DEVFN(2, 0), IOSF_PORT_CCK,
>> +	vlv_sideband_rw(dev_priv, PCI_DEVFN(0, 0), IOSF_PORT_CCK,
>>  			SB_CRWRDA_NP, reg, &val);
>>  }
>>  
>>  u32 vlv_ccu_read(struct drm_i915_private *dev_priv, u32 reg)
>>  {
>>  	u32 val = 0;
>> -	vlv_sideband_rw(dev_priv, PCI_DEVFN(2, 0), IOSF_PORT_CCU,
>> +	vlv_sideband_rw(dev_priv, PCI_DEVFN(0, 0), IOSF_PORT_CCU,
>>  			SB_CRRDDA_NP, reg, &val);
>>  	return val;
>>  }
>>  
>>  void vlv_ccu_write(struct drm_i915_private *dev_priv, u32 reg, u32 val)
>>  {
>> -	vlv_sideband_rw(dev_priv, PCI_DEVFN(2, 0), IOSF_PORT_CCU,
>> +	vlv_sideband_rw(dev_priv, PCI_DEVFN(0, 0), IOSF_PORT_CCU,
>>  			SB_CRWRDA_NP, reg, &val);
>>  }
>>  
>>  u32 vlv_gps_core_read(struct drm_i915_private *dev_priv, u32 reg)
>>  {
>>  	u32 val = 0;
>> -	vlv_sideband_rw(dev_priv, PCI_DEVFN(2, 0), IOSF_PORT_GPS_CORE,
>> +	vlv_sideband_rw(dev_priv, PCI_DEVFN(0, 0), IOSF_PORT_GPS_CORE,
>>  			SB_CRRDDA_NP, reg, &val);
>>  	return val;
>>  }
>>  
>>  void vlv_gps_core_write(struct drm_i915_private *dev_priv, u32 reg, u32 val)
>>  {
>> -	vlv_sideband_rw(dev_priv, PCI_DEVFN(2, 0), IOSF_PORT_GPS_CORE,
>> +	vlv_sideband_rw(dev_priv, PCI_DEVFN(0, 0), IOSF_PORT_GPS_CORE,
>>  			SB_CRWRDA_NP, reg, &val);
>>  }
>>  
>> -- 
>> 1.9.1
>
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx