Re: [PATCH v5 2/2] drm/i915/gmbus: Enable burst read

[Ramalingam C <ramalingam.c@intel.com>]

On Tuesday 29 May 2018 11:35 PM, Ville Syrjälä wrote:
> On Fri, May 18, 2018 at 02:54:53PM +0530, Ramalingam C wrote:
>> Support for Burst read in HW is added for HDCP2.2 compliance
>> requirement.
>>
>> This patch enables the burst read for all the gmbus read of more than
>> 511Bytes, on capable platforms.
>>
>> v2:
>>    Extra line is removed.
>> v3:
>>    Macro is added for detecting the BURST_READ Support [Jani]
>>    Runtime detection of the need for burst_read [Jani]
>>    Calculation enhancement.
>> v4:
>>    GMBUS0 reg val is passed from caller [ville]
>>    Removed a extra var [ville]
>>    Extra brackets are removed [ville]
>>    Implemented the handling of 512Bytes Burst Read.
>> v5:
>>    Burst read max length is fixed at 767Bytes [Ville]
>>
>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h  |  3 ++
>>   drivers/gpu/drm/i915/i915_reg.h  |  1 +
>>   drivers/gpu/drm/i915/intel_i2c.c | 62 +++++++++++++++++++++++++++++++++-------
>>   3 files changed, 56 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 028691108125..14293fc1a142 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2552,6 +2552,9 @@ intel_info(const struct drm_i915_private *dev_priv)
>>    */
>>   #define HAS_AUX_IRQ(dev_priv)   true
>>   #define HAS_GMBUS_IRQ(dev_priv) (INTEL_GEN(dev_priv) >= 4)
>> +#define HAS_GMBUS_BURST_READ(dev_priv) (INTEL_GEN(dev_priv) >= 10 || \
>> +					IS_GEMINILAKE(dev_priv) || \
>> +					IS_KABYLAKE(dev_priv))
> Note 100% sure about these. The spec say some late stepping SPT has this
> already. But I suppose this KBL+ match means just KBP+?
>
> Hmm. Did I ask this already before? Getting some dejavu here.
Ville,

All product sku of KBL will have the WA. And we are enabling this from 
gen10+ including KBL and GLK for HDCP2.2 requirement.
Thats the reasoning behind this.


>
>>   /* With the 945 and later, Y tiling got adjusted so that it was 32 128-byte
>>    * rows, which changed the alignment requirements and fence programming.
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index ebdf7c9d816e..575d9495f3e2 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -2996,6 +2996,7 @@ enum i915_power_well_id {
>>   #define   GMBUS_RATE_400KHZ	(2<<8) /* reserved on Pineview */
>>   #define   GMBUS_RATE_1MHZ	(3<<8) /* reserved on Pineview */
>>   #define   GMBUS_HOLD_EXT	(1<<7) /* 300ns hold time, rsvd on Pineview */
>> +#define   GMBUS_BYTE_CNT_OVERRIDE (1<<6)
>>   #define   GMBUS_PIN_DISABLED	0
>>   #define   GMBUS_PIN_SSC		1
>>   #define   GMBUS_PIN_VGADDC	2
>> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
>> index 1c0f6b56b209..9e1142a2f81b 100644
>> --- a/drivers/gpu/drm/i915/intel_i2c.c
>> +++ b/drivers/gpu/drm/i915/intel_i2c.c
>> @@ -371,12 +371,30 @@ unsigned int gmbus_max_xfer_size(struct drm_i915_private *dev_priv)
>>   static int
>>   gmbus_xfer_read_chunk(struct drm_i915_private *dev_priv,
>>   		      unsigned short addr, u8 *buf, unsigned int len,
>> -		      u32 gmbus1_index)
>> +		      u32 gmbus0_reg, u32 gmbus1_index)
>>   {
>> +	unsigned int size = len;
>> +	bool burst_read = len > gmbus_max_xfer_size(dev_priv);
>> +	bool extra_byte_added = false;
>> +
>> +	if (burst_read) {
>> +
> Stray newline.
With this newline removed, I will submit the next version with your 
reviewed-by.

Thanks
Ram
>
> Otherwise this looks good to me.
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>
>> +		/*
>> +		 * As per HW Spec, for 512Bytes need to read extra Byte and
>> +		 * Ignore the extra byte read.
>> +		 */
>> +		if (len == 512) {
>> +			extra_byte_added = true;
>> +			len++;
>> +		}
>> +		size = len % 256 + 256;
>> +		I915_WRITE_FW(GMBUS0, gmbus0_reg | GMBUS_BYTE_CNT_OVERRIDE);
>> +	}
>> +
>>   	I915_WRITE_FW(GMBUS1,
>>   		      gmbus1_index |
>>   		      GMBUS_CYCLE_WAIT |
>> -		      (len << GMBUS_BYTE_COUNT_SHIFT) |
>> +		      (size << GMBUS_BYTE_COUNT_SHIFT) |
>>   		      (addr << GMBUS_SLAVE_ADDR_SHIFT) |
>>   		      GMBUS_SLAVE_READ | GMBUS_SW_RDY);
>>   	while (len) {
>> @@ -389,17 +407,34 @@ gmbus_xfer_read_chunk(struct drm_i915_private *dev_priv,
>>   
>>   		val = I915_READ_FW(GMBUS3);
>>   		do {
>> +			if (extra_byte_added && len == 1)
>> +				break;
>> +
>>   			*buf++ = val & 0xff;
>>   			val >>= 8;
>>   		} while (--len && ++loop < 4);
>> +
>> +		if (burst_read && len == size - 4)
>> +			/* Reset the override bit */
>> +			I915_WRITE_FW(GMBUS0, gmbus0_reg);
>>   	}
>>   
>>   	return 0;
>>   }
>>   
>> +/*
>> + * HW spec says that 512Bytes in Burst read need special treatment.
>> + * But it doesn't talk about other multiple of 256Bytes. And couldn't locate
>> + * an I2C slave, which supports such a lengthy burst read too for experiments.
>> + *
>> + * So until things get clarified on HW support, to avoid the burst read length
>> + * in fold of 256Bytes except 512, max burst read length is fixed at 767Bytes.
>> + */
>> +#define INTEL_GMBUS_BURST_READ_MAX_LEN		767U
>> +
>>   static int
>>   gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
>> -		u32 gmbus1_index)
>> +		u32 gmbus0_reg, u32 gmbus1_index)
>>   {
>>   	u8 *buf = msg->buf;
>>   	unsigned int rx_size = msg->len;
>> @@ -407,10 +442,13 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
>>   	int ret;
>>   
>>   	do {
>> -		len = min(rx_size, gmbus_max_xfer_size(dev_priv));
>> +		if (HAS_GMBUS_BURST_READ(dev_priv))
>> +			len = min(rx_size, INTEL_GMBUS_BURST_READ_MAX_LEN);
>> +		else
>> +			len = min(rx_size, gmbus_max_xfer_size(dev_priv));
>>   
>> -		ret = gmbus_xfer_read_chunk(dev_priv, msg->addr,
>> -					    buf, len, gmbus1_index);
>> +		ret = gmbus_xfer_read_chunk(dev_priv, msg->addr, buf, len,
>> +					    gmbus0_reg, gmbus1_index);
>>   		if (ret)
>>   			return ret;
>>   
>> @@ -498,7 +536,8 @@ gmbus_is_index_xfer(struct i2c_msg *msgs, int i, int num)
>>   }
>>   
>>   static int
>> -gmbus_index_xfer(struct drm_i915_private *dev_priv, struct i2c_msg *msgs)
>> +gmbus_index_xfer(struct drm_i915_private *dev_priv, struct i2c_msg *msgs,
>> +		 u32 gmbus0_reg)
>>   {
>>   	u32 gmbus1_index = 0;
>>   	u32 gmbus5 = 0;
>> @@ -516,7 +555,8 @@ gmbus_index_xfer(struct drm_i915_private *dev_priv, struct i2c_msg *msgs)
>>   		I915_WRITE_FW(GMBUS5, gmbus5);
>>   
>>   	if (msgs[1].flags & I2C_M_RD)
>> -		ret = gmbus_xfer_read(dev_priv, &msgs[1], gmbus1_index);
>> +		ret = gmbus_xfer_read(dev_priv, &msgs[1], gmbus0_reg,
>> +				      gmbus1_index);
>>   	else
>>   		ret = gmbus_xfer_write(dev_priv, &msgs[1], gmbus1_index);
>>   
>> @@ -551,10 +591,12 @@ do_gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num,
>>   	for (; i < num; i += inc) {
>>   		inc = 1;
>>   		if (gmbus_is_index_xfer(msgs, i, num)) {
>> -			ret = gmbus_index_xfer(dev_priv, &msgs[i]);
>> +			ret = gmbus_index_xfer(dev_priv, &msgs[i],
>> +					       gmbus0_source | bus->reg0);
>>   			inc = 2; /* an index transmission is two msgs */
>>   		} else if (msgs[i].flags & I2C_M_RD) {
>> -			ret = gmbus_xfer_read(dev_priv, &msgs[i], 0);
>> +			ret = gmbus_xfer_read(dev_priv, &msgs[i],
>> +					      gmbus0_source | bus->reg0, 0);
>>   		} else {
>>   			ret = gmbus_xfer_write(dev_priv, &msgs[i], 0);
>>   		}
>> -- 
>> 2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx