[PATCH] drm: bridge: dw-hdmi: support i2c extended read mode

[PATCH] drm: bridge: dw-hdmi: support i2c extended read mode

[Nickey Yang]

"I2C Master Interface Extended Read Mode" implements a segment
pointer-based read operation using the Special Register configuration.

This patch fix https://patchwork.kernel.org/patch/7098101/ mentioned
"The current implementation does not support "I2C Master Interface
Extended Read Mode" to read data addressed by non-zero segment
pointer, this means that if EDID has more than 1 extension blocks"

With this patch,dw-hdmi can read EDID data with 1/2/4 blocks.

Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
---
 drivers/gpu/drm/bridge/dw-hdmi.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 9a9ec27..6ad7b87 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -33,6 +33,7 @@
 #include "dw-hdmi-audio.h"
 
 #define HDMI_EDID_LEN		512
+#define DDC_SEGMENT_ADDR       0x30
 
 #define RGB			0
 #define YCBCR444		1
@@ -111,6 +112,7 @@ struct dw_hdmi_i2c {
 
 	u8			slave_reg;
 	bool			is_regaddr;
+	bool			is_segment;
 };
 
 struct dw_hdmi_phy_data {
@@ -258,8 +260,12 @@ static int dw_hdmi_i2c_read(struct dw_hdmi *hdmi,
 		reinit_completion(&i2c->cmp);
 
 		hdmi_writeb(hdmi, i2c->slave_reg++, HDMI_I2CM_ADDRESS);
-		hdmi_writeb(hdmi, HDMI_I2CM_OPERATION_READ,
-			    HDMI_I2CM_OPERATION);
+		if (i2c->is_segment)
+			hdmi_writeb(hdmi, HDMI_I2CM_OPERATION_READ_EXT,
+				    HDMI_I2CM_OPERATION);
+		else
+			hdmi_writeb(hdmi, HDMI_I2CM_OPERATION_READ,
+				    HDMI_I2CM_OPERATION);
 
 		stat = wait_for_completion_timeout(&i2c->cmp, HZ / 10);
 		if (!stat)
@@ -271,6 +277,7 @@ static int dw_hdmi_i2c_read(struct dw_hdmi *hdmi,
 
 		*buf++ = hdmi_readb(hdmi, HDMI_I2CM_DATAI);
 	}
+	i2c->is_segment = false;
 
 	return 0;
 }
@@ -320,12 +327,6 @@ static int dw_hdmi_i2c_xfer(struct i2c_adapter *adap,
 	dev_dbg(hdmi->dev, "xfer: num: %d, addr: %#x\n", num, addr);
 
 	for (i = 0; i < num; i++) {
-		if (msgs[i].addr != addr) {
-			dev_warn(hdmi->dev,
-				 "unsupported transfer, changed slave address\n");
-			return -EOPNOTSUPP;
-		}
-
 		if (msgs[i].len == 0) {
 			dev_dbg(hdmi->dev,
 				"unsupported transfer %d/%d, no data\n",
@@ -345,15 +346,24 @@ static int dw_hdmi_i2c_xfer(struct i2c_adapter *adap,
 	/* Set slave device register address on transfer */
 	i2c->is_regaddr = false;
 
+	/* Set segment pointer for I2C extended read mode operation */
+	i2c->is_segment = false;
+
 	for (i = 0; i < num; i++) {
 		dev_dbg(hdmi->dev, "xfer: num: %d/%d, len: %d, flags: %#x\n",
 			i + 1, num, msgs[i].len, msgs[i].flags);
-
-		if (msgs[i].flags & I2C_M_RD)
-			ret = dw_hdmi_i2c_read(hdmi, msgs[i].buf, msgs[i].len);
-		else
-			ret = dw_hdmi_i2c_write(hdmi, msgs[i].buf, msgs[i].len);
-
+		if (msgs[i].addr == DDC_SEGMENT_ADDR && msgs[i].len == 1) {
+			i2c->is_segment = true;
+			hdmi_writeb(hdmi, DDC_SEGMENT_ADDR, HDMI_I2CM_SEGADDR);
+			hdmi_writeb(hdmi, *msgs[i].buf, HDMI_I2CM_SEGPTR);
+		} else {
+			if (msgs[i].flags & I2C_M_RD)
+				ret = dw_hdmi_i2c_read(hdmi, msgs[i].buf,
+						       msgs[i].len);
+			else
+				ret = dw_hdmi_i2c_write(hdmi, msgs[i].buf,
+							msgs[i].len);
+		}
 		if (ret < 0)
 			break;
 	}
-- 
1.9.1

Re: [PATCH] drm: bridge: dw-hdmi: support i2c extended read mode

[Jose Abreu]

Hi Nickey,


On 22-02-2017 07:45, Nickey Yang wrote:
> "I2C Master Interface Extended Read Mode" implements a segment
> pointer-based read operation using the Special Register configuration.
>
> This patch fix https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_7098101_&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=yaVFU4TjGY0gVF8El1uKcisy6TPsyCl9uN7Wsis-qhY&m=GO5hTo1pTsXMTs-_YDONdmAGQyk8vf0XXA5s-P77fPo&s=cy1I1t3kDvC0ihSnmEqbDx-PiPvzXaksyM9G240RSmE&e=  mentioned
> "The current implementation does not support "I2C Master Interface
> Extended Read Mode" to read data addressed by non-zero segment
> pointer, this means that if EDID has more than 1 extension blocks"
>
> With this patch,dw-hdmi can read EDID data with 1/2/4 blocks.

Can't read an EDID with 3 extension blocks? Why? (I'm not reading
the spec so I don't know if this is possible to exist. Maybe only
an even number of blocks is allowed).

>
> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
> ---
>  drivers/gpu/drm/bridge/dw-hdmi.c | 38 ++++++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
> index 9a9ec27..6ad7b87 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -33,6 +33,7 @@
>  #include "dw-hdmi-audio.h"
>  
>  #define HDMI_EDID_LEN		512
> +#define DDC_SEGMENT_ADDR       0x30
>  
>  #define RGB			0
>  #define YCBCR444		1
> @@ -111,6 +112,7 @@ struct dw_hdmi_i2c {
>  
>  	u8			slave_reg;
>  	bool			is_regaddr;
> +	bool			is_segment;
>  };
>  
>  struct dw_hdmi_phy_data {
> @@ -258,8 +260,12 @@ static int dw_hdmi_i2c_read(struct dw_hdmi *hdmi,
>  		reinit_completion(&i2c->cmp);
>  
>  		hdmi_writeb(hdmi, i2c->slave_reg++, HDMI_I2CM_ADDRESS);
> -		hdmi_writeb(hdmi, HDMI_I2CM_OPERATION_READ,
> -			    HDMI_I2CM_OPERATION);
> +		if (i2c->is_segment)
> +			hdmi_writeb(hdmi, HDMI_I2CM_OPERATION_READ_EXT,
> +				    HDMI_I2CM_OPERATION);
> +		else
> +			hdmi_writeb(hdmi, HDMI_I2CM_OPERATION_READ,
> +				    HDMI_I2CM_OPERATION);
>  
>  		stat = wait_for_completion_timeout(&i2c->cmp, HZ / 10);
>  		if (!stat)
> @@ -271,6 +277,7 @@ static int dw_hdmi_i2c_read(struct dw_hdmi *hdmi,
>  
>  		*buf++ = hdmi_readb(hdmi, HDMI_I2CM_DATAI);
>  	}
> +	i2c->is_segment = false;
>  
>  	return 0;
>  }
> @@ -320,12 +327,6 @@ static int dw_hdmi_i2c_xfer(struct i2c_adapter *adap,
>  	dev_dbg(hdmi->dev, "xfer: num: %d, addr: %#x\n", num, addr);
>  
>  	for (i = 0; i < num; i++) {
> -		if (msgs[i].addr != addr) {
> -			dev_warn(hdmi->dev,
> -				 "unsupported transfer, changed slave address\n");
> -			return -EOPNOTSUPP;
> -		}
> -
>  		if (msgs[i].len == 0) {
>  			dev_dbg(hdmi->dev,
>  				"unsupported transfer %d/%d, no data\n",
> @@ -345,15 +346,24 @@ static int dw_hdmi_i2c_xfer(struct i2c_adapter *adap,
>  	/* Set slave device register address on transfer */
>  	i2c->is_regaddr = false;
>  
> +	/* Set segment pointer for I2C extended read mode operation */
> +	i2c->is_segment = false;
> +
>  	for (i = 0; i < num; i++) {
>  		dev_dbg(hdmi->dev, "xfer: num: %d/%d, len: %d, flags: %#x\n",
>  			i + 1, num, msgs[i].len, msgs[i].flags);
> -
> -		if (msgs[i].flags & I2C_M_RD)
> -			ret = dw_hdmi_i2c_read(hdmi, msgs[i].buf, msgs[i].len);
> -		else
> -			ret = dw_hdmi_i2c_write(hdmi, msgs[i].buf, msgs[i].len);
> -
> +		if (msgs[i].addr == DDC_SEGMENT_ADDR && msgs[i].len == 1) {
> +			i2c->is_segment = true;
> +			hdmi_writeb(hdmi, DDC_SEGMENT_ADDR, HDMI_I2CM_SEGADDR);
> +			hdmi_writeb(hdmi, *msgs[i].buf, HDMI_I2CM_SEGPTR);

Hmm, this does not match what I have here internally but I may
have something wrong. You tested this using an EDID with how many
extension blocks?

Anyway, this is what I have:
    if(msgs[i].addr == DDC_SEGMENT_ADDR && msgs[i].len == 1) {
        hdmi_writeb(hdmi, msgs[i].buf[0], HDMI_I2CM_SEGADDR);
        [and segptr is 0x0]
    }

I probably may have something wrong but I remember I tested this
a long time ago using an HDMI 2.0 EDID and it worked fine. BUT, I
have special cases for other addresses (0x50 and 0x54), I can't
remember if these are for EDID or for SCDC. I can send you the
code I have if you want.

Best regards,
Jose Miguel Abreu

> +		} else {
> +			if (msgs[i].flags & I2C_M_RD)
> +				ret = dw_hdmi_i2c_read(hdmi, msgs[i].buf,
> +						       msgs[i].len);
> +			else
> +				ret = dw_hdmi_i2c_write(hdmi, msgs[i].buf,
> +							msgs[i].len);
> +		}
>  		if (ret < 0)
>  			break;
>  	}

Re: [PATCH] drm: bridge: dw-hdmi: support i2c extended read mode

[Nickey.Yang]

Hi Jose,


在 2017年02月22日 17:51, Jose Abreu 写道:
> Hi Nickey,
>
>
> On 22-02-2017 07:45, Nickey Yang wrote:
>> "I2C Master Interface Extended Read Mode" implements a segment
>> pointer-based read operation using the Special Register configuration.
>>
>> This patch fix https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_7098101_&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=yaVFU4TjGY0gVF8El1uKcisy6TPsyCl9uN7Wsis-qhY&m=GO5hTo1pTsXMTs-_YDONdmAGQyk8vf0XXA5s-P77fPo&s=cy1I1t3kDvC0ihSnmEqbDx-PiPvzXaksyM9G240RSmE&e=  mentioned
>> "The current implementation does not support "I2C Master Interface
>> Extended Read Mode" to read data addressed by non-zero segment
>> pointer, this means that if EDID has more than 1 extension blocks"
>>
>> With this patch,dw-hdmi can read EDID data with 1/2/4 blocks.
> Can't read an EDID with 3 extension blocks? Why? (I'm not reading
> the spec so I don't know if this is possible to exist. Maybe only
> an even number of blocks is allowed).
>
Read EDID func is drm_do_probe_ddc_edid    (in 
drivers/gpu/drm/drm_edid.c     Line:1215)
when block = 3  means &msgs[3].addr = DDC_SEGMENT_ADDR(0x30) ,but in 
dw_hdmi_i2c_xfer
line 299  msgs[i].addr != addr (0x50),  it will printk "unsupported 
transfer, changed slave address"
and return -EOPNOTSUPP.

in fact,0x30 not means change slave address but means segment address.

>> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
>> ---
>>   drivers/gpu/drm/bridge/dw-hdmi.c | 38 ++++++++++++++++++++++++--------------
>>   1 file changed, 24 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
>> index 9a9ec27..6ad7b87 100644
>> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
>> @@ -33,6 +33,7 @@
>>   #include "dw-hdmi-audio.h"
>>   
>>   #define HDMI_EDID_LEN		512
>> +#define DDC_SEGMENT_ADDR       0x30
>>   
>>   #define RGB			0
>>   #define YCBCR444		1
>> @@ -111,6 +112,7 @@ struct dw_hdmi_i2c {
>>   
>>   	u8			slave_reg;
>>   	bool			is_regaddr;
>> +	bool			is_segment;
>>   };
>>   
>>   struct dw_hdmi_phy_data {
>> @@ -258,8 +260,12 @@ static int dw_hdmi_i2c_read(struct dw_hdmi *hdmi,
>>   		reinit_completion(&i2c->cmp);
>>   
>>   		hdmi_writeb(hdmi, i2c->slave_reg++, HDMI_I2CM_ADDRESS);
>> -		hdmi_writeb(hdmi, HDMI_I2CM_OPERATION_READ,
>> -			    HDMI_I2CM_OPERATION);
>> +		if (i2c->is_segment)
>> +			hdmi_writeb(hdmi, HDMI_I2CM_OPERATION_READ_EXT,
>> +				    HDMI_I2CM_OPERATION);
>> +		else
>> +			hdmi_writeb(hdmi, HDMI_I2CM_OPERATION_READ,
>> +				    HDMI_I2CM_OPERATION);
>>   
>>   		stat = wait_for_completion_timeout(&i2c->cmp, HZ / 10);
>>   		if (!stat)
>> @@ -271,6 +277,7 @@ static int dw_hdmi_i2c_read(struct dw_hdmi *hdmi,
>>   
>>   		*buf++ = hdmi_readb(hdmi, HDMI_I2CM_DATAI);
>>   	}
>> +	i2c->is_segment = false;
>>   
>>   	return 0;
>>   }
>> @@ -320,12 +327,6 @@ static int dw_hdmi_i2c_xfer(struct i2c_adapter *adap,
>>   	dev_dbg(hdmi->dev, "xfer: num: %d, addr: %#x\n", num, addr);
>>   
>>   	for (i = 0; i < num; i++) {
>> -		if (msgs[i].addr != addr) {
>> -			dev_warn(hdmi->dev,
>> -				 "unsupported transfer, changed slave address\n");
>> -			return -EOPNOTSUPP;
>> -		}
>> -
>>   		if (msgs[i].len == 0) {
>>   			dev_dbg(hdmi->dev,
>>   				"unsupported transfer %d/%d, no data\n",
>> @@ -345,15 +346,24 @@ static int dw_hdmi_i2c_xfer(struct i2c_adapter *adap,
>>   	/* Set slave device register address on transfer */
>>   	i2c->is_regaddr = false;
>>   
>> +	/* Set segment pointer for I2C extended read mode operation */
>> +	i2c->is_segment = false;
>> +
>>   	for (i = 0; i < num; i++) {
>>   		dev_dbg(hdmi->dev, "xfer: num: %d/%d, len: %d, flags: %#x\n",
>>   			i + 1, num, msgs[i].len, msgs[i].flags);
>> -
>> -		if (msgs[i].flags & I2C_M_RD)
>> -			ret = dw_hdmi_i2c_read(hdmi, msgs[i].buf, msgs[i].len);
>> -		else
>> -			ret = dw_hdmi_i2c_write(hdmi, msgs[i].buf, msgs[i].len);
>> -
>> +		if (msgs[i].addr == DDC_SEGMENT_ADDR && msgs[i].len == 1) {
>> +			i2c->is_segment = true;
>> +			hdmi_writeb(hdmi, DDC_SEGMENT_ADDR, HDMI_I2CM_SEGADDR);
>> +			hdmi_writeb(hdmi, *msgs[i].buf, HDMI_I2CM_SEGPTR);
> Hmm, this does not match what I have here internally but I may
> have something wrong. You tested this using an EDID with how many
> extension blocks?
>
> Anyway, this is what I have:
>      if(msgs[i].addr == DDC_SEGMENT_ADDR && msgs[i].len == 1) {
>          hdmi_writeb(hdmi, msgs[i].buf[0], HDMI_I2CM_SEGADDR);
>          [and segptr is 0x0]
>      }
>
> I probably may have something wrong but I remember I tested this
> a long time ago using an HDMI 2.0 EDID and it worked fine. BUT, I
> have special cases for other addresses (0x50 and 0x54), I can't
> remember if these are for EDID or for SCDC. I can send you the
> code I have if you want.
>
> Best regards,
> Jose Miguel Abreu
>
I have tested this using an EDID with 0/1/3 extension blocks including HDMI1.4/2.0 EDID.
We pass HDMI CDF certification  successfully with this.
(  Test  Item 7-1:  EDID Related Behavior  2-Block EDID , 4-Block EDID)

0x50  is ddc slave address for read edid.
0x54  is ddc slave address for scdc operation.
0x30 is ddc  segment address.
(segaddr
This register configures the segment address for extended R/W destination and is used for EDID reading
operations, particularly for the Extended Data Read Operation for Enhanced DDC)

Best regards,
Nickey Yang

>> +		} else {
>> +			if (msgs[i].flags & I2C_M_RD)
>> +				ret = dw_hdmi_i2c_read(hdmi, msgs[i].buf,
>> +						       msgs[i].len);
>> +			else
>> +				ret = dw_hdmi_i2c_write(hdmi, msgs[i].buf,
>> +							msgs[i].len);
>> +		}
>>   		if (ret < 0)
>>   			break;
>>   	}
>
>
>

Re: [PATCH] drm: bridge: dw-hdmi: support i2c extended read mode

[Doug Anderson]

Hi,

On Tue, Feb 21, 2017 at 11:45 PM, Nickey Yang
<nickey.yang@rock-chips.com> wrote:
> "I2C Master Interface Extended Read Mode" implements a segment
> pointer-based read operation using the Special Register configuration.
>
> This patch fix https://patchwork.kernel.org/patch/7098101/ mentioned
> "The current implementation does not support "I2C Master Interface
> Extended Read Mode" to read data addressed by non-zero segment
> pointer, this means that if EDID has more than 1 extension blocks"
>
> With this patch,dw-hdmi can read EDID data with 1/2/4 blocks.
>
> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
> ---
>  drivers/gpu/drm/bridge/dw-hdmi.c | 38 ++++++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 14 deletions(-)

I don't have any deep expertise on how the DW HDMI's i2c controller is
supposed to work and the truly correct way to specify the segment
address.  However, this looks fairly sane to me and review feedback I
provided on http://crosreview.com/442308 has been addressed.

I would agree that this appears to handle the types of messages that
the kernel seems to construct, basically noticing the first write to
DDC_SEGMENT_ADDR and stashing the data that the kernel provides in
HDMI_I2CM_SEGPTR.  It then tells dw-hdmi to do a
HDMI_I2CM_OPERATION_READ_EXT instead of HDMI_I2CM_OPERATION_READ.

Anyway, presuming that's the right way to tell dw-hdmi, I'd say:

Reviewed-by: Douglas Anderson <dianders@chromium.org>