[PATCH v3 0/3] drm/bridge: it6505: update dp aux funxtion

[PATCH v3 0/3] drm/bridge: it6505: update dp aux funxtion

[Hermes Wu]

From: Hermes Wu <Hermes.wu@ite.com.tw>

Changes in v3
 -Seperate AUX fifo access to a commit
 -Add discriptions about HDCP CTS
 -remove non used aux i2c reply definitions

Hermes Wu (3):
  drm/bridge: it6505: fix AUX read use aux fifo
  drm/bridge: it6505: HDCP CTS fail on repeater items
  drm/bridge: it6505: Add MCCS support

 drivers/gpu/drm/bridge/ite-it6505.c | 311 +++++++++++++++++++++++-----
 1 file changed, 263 insertions(+), 48 deletions(-)

-- 
2.34.1

[PATCH v3 1/3] drm/bridge: it6505: fix AUX read use aux fifo

[Hermes Wu]

From: Hermes Wu <Hermes.wu@ite.com.tw>

Changes in v3:
 -New in v3


it6505 AUX FIFO mode only 16 byte.
AUX FIFO mode only supports EDID read and DPCD KSV FIFO area.

Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 drivers/gpu/drm/bridge/ite-it6505.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index 87b8545fccc0..d8b40ad890bf 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -300,7 +300,7 @@
 #define MAX_CR_LEVEL 0x03
 #define MAX_EQ_LEVEL 0x03
 #define AUX_WAIT_TIMEOUT_MS 15
-#define AUX_FIFO_MAX_SIZE 32
+#define AUX_FIFO_MAX_SIZE 16
 #define PIXEL_CLK_DELAY 1
 #define PIXEL_CLK_INVERSE 0
 #define ADJUST_PHASE_THRESHOLD 80000
@@ -324,8 +324,13 @@ enum aux_cmd_type {
 	CMD_AUX_NATIVE_READ = 0x0,
 	CMD_AUX_NATIVE_WRITE = 0x5,
 	CMD_AUX_I2C_EDID_READ = 0xB,
+
+	/* KSV list read using AUX native read with FIFO */
+	CMD_AUX_GET_KSV_LIST = 0x10,
 };
 
+#define GET_AUX_CONTROL_CODE(cmd) ((cmd) & 0x0F)
+
 enum aux_cmd_reply {
 	REPLY_ACK,
 	REPLY_NACK,
@@ -965,7 +970,8 @@ static ssize_t it6505_aux_operation(struct it6505 *it6505,
 	it6505_set_bits(it6505, REG_AUX_CTRL, AUX_USER_MODE, AUX_USER_MODE);
 
 aux_op_start:
-	if (cmd == CMD_AUX_I2C_EDID_READ) {
+	/* HW AUX FIFO supports only EDID and DCPD KSV FIFO aread */
+	if (cmd == CMD_AUX_I2C_EDID_READ || cmd == CMD_AUX_GET_KSV_LIST) {
 		/* AUX EDID FIFO has max length of AUX_FIFO_MAX_SIZE bytes. */
 		size = min_t(size_t, size, AUX_FIFO_MAX_SIZE);
 		/* Enable AUX FIFO read back and clear FIFO */
@@ -996,7 +1002,7 @@ static ssize_t it6505_aux_operation(struct it6505 *it6505,
 				  size);
 
 	/* Aux Fire */
-	it6505_write(it6505, REG_AUX_CMD_REQ, cmd);
+	it6505_write(it6505, REG_AUX_CMD_REQ, GET_AUX_CONTROL_CODE(cmd));
 
 	ret = it6505_aux_wait(it6505);
 	if (ret < 0)
@@ -1030,7 +1036,7 @@ static ssize_t it6505_aux_operation(struct it6505 *it6505,
 		goto aux_op_start;
 	}
 
-	if (cmd == CMD_AUX_I2C_EDID_READ) {
+	if (cmd == CMD_AUX_I2C_EDID_READ || cmd == CMD_AUX_GET_KSV_LIST) {
 		for (i = 0; i < size; i++) {
 			ret = it6505_read(it6505, REG_AUX_DATA_FIFO);
 			if (ret < 0)
@@ -1055,7 +1061,7 @@ static ssize_t it6505_aux_operation(struct it6505 *it6505,
 	ret = i;
 
 aux_op_err:
-	if (cmd == CMD_AUX_I2C_EDID_READ) {
+	if (cmd == CMD_AUX_I2C_EDID_READ || cmd == CMD_AUX_GET_KSV_LIST) {
 		/* clear AUX FIFO */
 		it6505_set_bits(it6505, REG_AUX_CTRL,
 				AUX_EN_FIFO_READ | CLR_EDID_FIFO,
@@ -1078,8 +1084,11 @@ static ssize_t it6505_aux_do_transfer(struct it6505 *it6505,
 	int i, ret_size, ret = 0, request_size;
 
 	mutex_lock(&it6505->aux_lock);
-	for (i = 0; i < size; i += 4) {
-		request_size = min((int)size - i, 4);
+	for (i = 0; i < size; ) {
+		if (cmd == CMD_AUX_I2C_EDID_READ || cmd == CMD_AUX_GET_KSV_LIST)
+			request_size = min_t(int, (int)size - i, AUX_FIFO_MAX_SIZE);
+		else
+			request_size = min_t(int, (int)size - i, 4);
 		ret_size = it6505_aux_operation(it6505, cmd, address + i,
 						buffer + i, request_size,
 						reply);
@@ -1088,6 +1097,7 @@ static ssize_t it6505_aux_do_transfer(struct it6505 *it6505,
 			goto aux_op_err;
 		}
 
+		i += request_size;
 		ret += ret_size;
 	}
 
@@ -2257,7 +2267,6 @@ static void it6505_link_training_work(struct work_struct *work)
 		it6505->auto_train_retry--;
 		it6505_dump(it6505);
 	}
-
 }
 
 static void it6505_plugged_status_to_codec(struct it6505 *it6505)
-- 
2.34.1

Re: [PATCH v3 1/3] drm/bridge: it6505: fix AUX read use aux fifo

[Dmitry Baryshkov]

On Mon, Sep 23, 2024 at 05:48:26PM GMT, Hermes Wu wrote:
> From: Hermes Wu <Hermes.wu@ite.com.tw>
> 
> Changes in v3:
>  -New in v3
> 
> 
> it6505 AUX FIFO mode only 16 byte.
> AUX FIFO mode only supports EDID read and DPCD KSV FIFO area.

ENOTREADABLE. It should be a text, not a set of phrases.
Also changelog comes afterwards.

> 
> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
> ---
>  drivers/gpu/drm/bridge/ite-it6505.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index 87b8545fccc0..d8b40ad890bf 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -300,7 +300,7 @@
>  #define MAX_CR_LEVEL 0x03
>  #define MAX_EQ_LEVEL 0x03
>  #define AUX_WAIT_TIMEOUT_MS 15
> -#define AUX_FIFO_MAX_SIZE 32
> +#define AUX_FIFO_MAX_SIZE 16
>  #define PIXEL_CLK_DELAY 1
>  #define PIXEL_CLK_INVERSE 0
>  #define ADJUST_PHASE_THRESHOLD 80000
> @@ -324,8 +324,13 @@ enum aux_cmd_type {
>  	CMD_AUX_NATIVE_READ = 0x0,
>  	CMD_AUX_NATIVE_WRITE = 0x5,
>  	CMD_AUX_I2C_EDID_READ = 0xB,
> +
> +	/* KSV list read using AUX native read with FIFO */
> +	CMD_AUX_GET_KSV_LIST = 0x10,

Don't mix two changes in a single patch. There should be one patch
fixing FIFO_MAX_SIDE (and then it should have Fixes tag) and another
patch adding CMD_AUX_GET_KSV_LIST. I keep on pointing to
Documenation/process/submitting-patches.rst, which you didn't seem to
have read. Please do it first. Ask any questions if you don't understand
something.

>  };
>  
> +#define GET_AUX_CONTROL_CODE(cmd) ((cmd) & 0x0F)
> +
>  enum aux_cmd_reply {
>  	REPLY_ACK,
>  	REPLY_NACK,
> @@ -965,7 +970,8 @@ static ssize_t it6505_aux_operation(struct it6505 *it6505,
>  	it6505_set_bits(it6505, REG_AUX_CTRL, AUX_USER_MODE, AUX_USER_MODE);
>  
>  aux_op_start:
> -	if (cmd == CMD_AUX_I2C_EDID_READ) {
> +	/* HW AUX FIFO supports only EDID and DCPD KSV FIFO aread */
> +	if (cmd == CMD_AUX_I2C_EDID_READ || cmd == CMD_AUX_GET_KSV_LIST) {
>  		/* AUX EDID FIFO has max length of AUX_FIFO_MAX_SIZE bytes. */
>  		size = min_t(size_t, size, AUX_FIFO_MAX_SIZE);
>  		/* Enable AUX FIFO read back and clear FIFO */
> @@ -996,7 +1002,7 @@ static ssize_t it6505_aux_operation(struct it6505 *it6505,
>  				  size);
>  
>  	/* Aux Fire */
> -	it6505_write(it6505, REG_AUX_CMD_REQ, cmd);
> +	it6505_write(it6505, REG_AUX_CMD_REQ, GET_AUX_CONTROL_CODE(cmd));

Looks like a separate fix.

>  
>  	ret = it6505_aux_wait(it6505);
>  	if (ret < 0)
> @@ -1030,7 +1036,7 @@ static ssize_t it6505_aux_operation(struct it6505 *it6505,
>  		goto aux_op_start;
>  	}
>  
> -	if (cmd == CMD_AUX_I2C_EDID_READ) {
> +	if (cmd == CMD_AUX_I2C_EDID_READ || cmd == CMD_AUX_GET_KSV_LIST) {
>  		for (i = 0; i < size; i++) {
>  			ret = it6505_read(it6505, REG_AUX_DATA_FIFO);
>  			if (ret < 0)
> @@ -1055,7 +1061,7 @@ static ssize_t it6505_aux_operation(struct it6505 *it6505,
>  	ret = i;
>  
>  aux_op_err:
> -	if (cmd == CMD_AUX_I2C_EDID_READ) {
> +	if (cmd == CMD_AUX_I2C_EDID_READ || cmd == CMD_AUX_GET_KSV_LIST) {
>  		/* clear AUX FIFO */
>  		it6505_set_bits(it6505, REG_AUX_CTRL,
>  				AUX_EN_FIFO_READ | CLR_EDID_FIFO,
> @@ -1078,8 +1084,11 @@ static ssize_t it6505_aux_do_transfer(struct it6505 *it6505,
>  	int i, ret_size, ret = 0, request_size;
>  
>  	mutex_lock(&it6505->aux_lock);
> -	for (i = 0; i < size; i += 4) {
> -		request_size = min((int)size - i, 4);
> +	for (i = 0; i < size; ) {
> +		if (cmd == CMD_AUX_I2C_EDID_READ || cmd == CMD_AUX_GET_KSV_LIST)
> +			request_size = min_t(int, (int)size - i, AUX_FIFO_MAX_SIZE);
> +		else
> +			request_size = min_t(int, (int)size - i, 4);

And this one is also separate.

>  		ret_size = it6505_aux_operation(it6505, cmd, address + i,
>  						buffer + i, request_size,
>  						reply);
> @@ -1088,6 +1097,7 @@ static ssize_t it6505_aux_do_transfer(struct it6505 *it6505,
>  			goto aux_op_err;
>  		}
>  
> +		i += request_size;
>  		ret += ret_size;
>  	}
>  
> @@ -2257,7 +2267,6 @@ static void it6505_link_training_work(struct work_struct *work)
>  		it6505->auto_train_retry--;
>  		it6505_dump(it6505);
>  	}
> -

And this is just a noise. Leave it as it is until somebody has to touch
these lines.

>  }
>  
>  static void it6505_plugged_status_to_codec(struct it6505 *it6505)
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

[PATCH v3 2/3] drm/bridge: it6505: HDCP CTS fail on repeater items

[Hermes Wu]

From: Hermes Wu <Hermes.wu@ite.com.tw>

Changes in v3:
 -add detials about fail item and changes.


Fix HDCP CTS fail items on UNIGRAF DRP-100

DUT must Support 127 devices.
DUT must check BSTATUS when recive CP_IRQ.
DUT must enable encription when R0' is ready.
DUT must retry V' check 3 times.
it6505 must read DRP-100 KSV FIFO by FIFO mode.
it6505 should restart HDCP within 5s if KSV not ready.

Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 drivers/gpu/drm/bridge/ite-it6505.c | 112 ++++++++++++++++++----------
 1 file changed, 74 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index d8b40ad890bf..156440c6517e 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -296,7 +296,7 @@
 #define MAX_LANE_COUNT 4
 #define MAX_LINK_RATE HBR
 #define AUTO_TRAIN_RETRY 3
-#define MAX_HDCP_DOWN_STREAM_COUNT 10
+#define MAX_HDCP_DOWN_STREAM_COUNT 127
 #define MAX_CR_LEVEL 0x03
 #define MAX_EQ_LEVEL 0x03
 #define AUX_WAIT_TIMEOUT_MS 15
@@ -1188,6 +1188,35 @@ static int it6505_get_edid_block(void *data, u8 *buf, unsigned int block,
 	return 0;
 }
 
+static int it6505_get_ksvlist(struct it6505 *it6505, u8 *buf, size_t len)
+{
+	int i, request_size, ret;
+	struct device *dev = it6505->dev;
+	enum aux_cmd_reply reply;
+
+	for (i = 0; i < len; ) {
+		request_size = min_t(int, (int)len - i, 15);
+
+		ret = it6505_aux_do_transfer(it6505, CMD_AUX_GET_KSV_LIST,
+					     DP_AUX_HDCP_KSV_FIFO,
+					     buf + i, request_size, &reply);
+
+		DRM_DEV_DEBUG_DRIVER(dev, "request_size = %d, ret =%d", request_size, ret);
+		if (ret < 0)
+			return ret;
+
+		i += request_size;
+	}
+
+	DRM_DEV_DEBUG_DRIVER(dev, "ksv read cnt = %d down_stream_cnt=%d ", i, i / 5);
+
+	for (i = 0 ; i < len; i += 5)
+		DRM_DEV_DEBUG_DRIVER(dev, "ksv[%d] = %02X%02X%02X%02X%02X",
+				     i / 5, buf[i], buf[i + 1], buf[i + 2], buf[i + 3], buf[i + 4]);
+
+	return len;
+}
+
 static void it6505_variable_config(struct it6505 *it6505)
 {
 	it6505->link_rate_bw_code = HBR;
@@ -1969,7 +1998,7 @@ static int it6505_setup_sha1_input(struct it6505 *it6505, u8 *sha1_input)
 {
 	struct device *dev = it6505->dev;
 	u8 binfo[2];
-	int down_stream_count, i, err, msg_count = 0;
+	int down_stream_count, err, msg_count = 0;
 
 	err = it6505_get_dpcd(it6505, DP_AUX_HDCP_BINFO, binfo,
 			      ARRAY_SIZE(binfo));
@@ -1994,18 +2023,11 @@ static int it6505_setup_sha1_input(struct it6505 *it6505, u8 *sha1_input)
 			down_stream_count);
 		return 0;
 	}
-
-	for (i = 0; i < down_stream_count; i++) {
-		err = it6505_get_dpcd(it6505, DP_AUX_HDCP_KSV_FIFO +
-				      (i % 3) * DRM_HDCP_KSV_LEN,
-				      sha1_input + msg_count,
-				      DRM_HDCP_KSV_LEN);
-
+	err =  it6505_get_ksvlist(it6505, sha1_input, down_stream_count * 5);
 		if (err < 0)
 			return err;
 
-		msg_count += 5;
-	}
+	msg_count += down_stream_count * 5;
 
 	it6505->hdcp_down_stream_count = down_stream_count;
 	sha1_input[msg_count++] = binfo[0];
@@ -2033,7 +2055,7 @@ static bool it6505_hdcp_part2_ksvlist_check(struct it6505 *it6505)
 {
 	struct device *dev = it6505->dev;
 	u8 av[5][4], bv[5][4];
-	int i, err;
+	int i, err, retry;
 
 	i = it6505_setup_sha1_input(it6505, it6505->sha1_input);
 	if (i <= 0) {
@@ -2042,22 +2064,28 @@ static bool it6505_hdcp_part2_ksvlist_check(struct it6505 *it6505)
 	}
 
 	it6505_sha1_digest(it6505, it6505->sha1_input, i, (u8 *)av);
+	/*1B-05 V' must retry 3 times */
+	for (retry = 0; retry < 3; retry++) {
+		err = it6505_get_dpcd(it6505, DP_AUX_HDCP_V_PRIME(0), (u8 *)bv,
+				      sizeof(bv));
+
+		if (err < 0) {
+			dev_err(dev, "Read V' value Fail %d", retry);
+			continue;
+		}
 
-	err = it6505_get_dpcd(it6505, DP_AUX_HDCP_V_PRIME(0), (u8 *)bv,
-			      sizeof(bv));
+		for (i = 0; i < 5; i++) {
+			if (bv[i][3] != av[i][0] || bv[i][2] != av[i][1] ||
+			    av[i][1] != av[i][2] || bv[i][0] != av[i][3])
+				break;
 
-	if (err < 0) {
-		dev_err(dev, "Read V' value Fail");
-		return false;
+			DRM_DEV_DEBUG_DRIVER(dev, "V' all match!! %d, %d", retry, i);
+			return true;
+		}
 	}
 
-	for (i = 0; i < 5; i++)
-		if (bv[i][3] != av[i][0] || bv[i][2] != av[i][1] ||
-		    bv[i][1] != av[i][2] || bv[i][0] != av[i][3])
-			return false;
-
-	DRM_DEV_DEBUG_DRIVER(dev, "V' all match!!");
-	return true;
+	DRM_DEV_DEBUG_DRIVER(dev, "V' NOT match!! %d", retry);
+	return false;
 }
 
 static void it6505_hdcp_wait_ksv_list(struct work_struct *work)
@@ -2065,7 +2093,8 @@ static void it6505_hdcp_wait_ksv_list(struct work_struct *work)
 	struct it6505 *it6505 = container_of(work, struct it6505,
 					     hdcp_wait_ksv_list);
 	struct device *dev = it6505->dev;
-	unsigned int timeout = 5000;
+	/* 1B-04 fail, wait to long to Stop encription(5s->2s). */
+	unsigned int timeout = 2000;
 	u8 bstatus = 0;
 	bool ksv_list_check;
 
@@ -2085,21 +2114,18 @@ static void it6505_hdcp_wait_ksv_list(struct work_struct *work)
 
 	if (timeout == 0) {
 		DRM_DEV_DEBUG_DRIVER(dev, "timeout and ksv list wait failed");
-		goto timeout;
+		goto hdcp_ksvlist_fail;
 	}
 
 	ksv_list_check = it6505_hdcp_part2_ksvlist_check(it6505);
 	DRM_DEV_DEBUG_DRIVER(dev, "ksv list ready, ksv list check %s",
 			     ksv_list_check ? "pass" : "fail");
-	if (ksv_list_check) {
-		it6505_set_bits(it6505, REG_HDCP_TRIGGER,
-				HDCP_TRIGGER_KSV_DONE, HDCP_TRIGGER_KSV_DONE);
+
+	if (ksv_list_check)
 		return;
-	}
-timeout:
-	it6505_set_bits(it6505, REG_HDCP_TRIGGER,
-			HDCP_TRIGGER_KSV_DONE | HDCP_TRIGGER_KSV_FAIL,
-			HDCP_TRIGGER_KSV_DONE | HDCP_TRIGGER_KSV_FAIL);
+
+hdcp_ksvlist_fail:
+	it6505_start_hdcp(it6505);
 }
 
 static void it6505_hdcp_work(struct work_struct *work)
@@ -2321,14 +2347,20 @@ static int it6505_process_hpd_irq(struct it6505 *it6505)
 	DRM_DEV_DEBUG_DRIVER(dev, "dp_irq_vector = 0x%02x", dp_irq_vector);
 
 	if (dp_irq_vector & DP_CP_IRQ) {
-		it6505_set_bits(it6505, REG_HDCP_TRIGGER, HDCP_TRIGGER_CPIRQ,
-				HDCP_TRIGGER_CPIRQ);
-
 		bstatus = it6505_dpcd_read(it6505, DP_AUX_HDCP_BSTATUS);
 		if (bstatus < 0)
 			return bstatus;
 
 		DRM_DEV_DEBUG_DRIVER(dev, "Bstatus = 0x%02x", bstatus);
+
+		/*1B-02 ignore when bstatus is 0 */
+		if (bstatus & DP_BSTATUS_R0_PRIME_READY &&
+		    it6505->hdcp_status == HDCP_AUTH_GOING)
+			it6505_set_bits(it6505, REG_HDCP_TRIGGER, HDCP_TRIGGER_CPIRQ,
+					HDCP_TRIGGER_CPIRQ);
+		else if (bstatus & (DP_BSTATUS_REAUTH_REQ | DP_BSTATUS_LINK_FAILURE) &&
+			 it6505->hdcp_status == HDCP_AUTH_DONE)
+			it6505_start_hdcp(it6505);
 	}
 
 	ret = drm_dp_dpcd_read_link_status(&it6505->aux, link_status);
@@ -2465,7 +2497,11 @@ static void it6505_irq_hdcp_ksv_check(struct it6505 *it6505)
 {
 	struct device *dev = it6505->dev;
 
-	DRM_DEV_DEBUG_DRIVER(dev, "HDCP event Interrupt");
+	DRM_DEV_DEBUG_DRIVER(dev, "HDCP repeater R0 event Interrupt");
+	/* 1B01 HDCP encription should start when R0 is ready*/
+	it6505_set_bits(it6505, REG_HDCP_TRIGGER,
+			HDCP_TRIGGER_KSV_DONE, HDCP_TRIGGER_KSV_DONE);
+
 	schedule_work(&it6505->hdcp_wait_ksv_list);
 }
 
-- 
2.34.1

Re: [PATCH v3 2/3] drm/bridge: it6505: HDCP CTS fail on repeater items

[Dmitry Baryshkov]

On Mon, Sep 23, 2024 at 05:48:28PM GMT, Hermes Wu wrote:
> From: Hermes Wu <Hermes.wu@ite.com.tw>
> 
> Changes in v3:
>  -add detials about fail item and changes.
> 
> 
> Fix HDCP CTS fail items on UNIGRAF DRP-100
> 
> DUT must Support 127 devices.
> DUT must check BSTATUS when recive CP_IRQ.
> DUT must enable encription when R0' is ready.
> DUT must retry V' check 3 times.
> it6505 must read DRP-100 KSV FIFO by FIFO mode.
> it6505 should restart HDCP within 5s if KSV not ready.

Still not readable.

English text, please. Split the patch to fix one issue at a time.
Describe the _reason_ for the change. Annotate fixes with Fixes tags.

> 
> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
> ---
>  drivers/gpu/drm/bridge/ite-it6505.c | 112 ++++++++++++++++++----------
>  1 file changed, 74 insertions(+), 38 deletions(-)

-- 
With best wishes
Dmitry

RE: [PATCH v3 2/3] drm/bridge: it6505: HDCP CTS fail on repeater items

[Hermes.Wu]

>On Mon, Sep 23, 2024 at 05:48:28PM GMT, Hermes Wu wrote:
>> From: Hermes Wu <Hermes.wu@ite.com.tw>
>> 
>> Changes in v3:
>>  -add detials about fail item and changes.
>> 
>> 
>> Fix HDCP CTS fail items on UNIGRAF DRP-100
>> 
>> DUT must Support 127 devices.
>> DUT must check BSTATUS when receive CP_IRQ.
>> DUT must enable encryption when R0' is ready.
>> DUT must retry V' check 3 times.
>> it6505 must read DRP-100 KSV FIFO by FIFO mode.
>> it6505 should restart HDCP within 5s if KSV not ready.
>
>Still not readable.
>
>English text, please. Split the patch to fix one issue at a time.
>Describe the _reason_ for the change. Annotate fixes with Fixes tags.
>

with fixes tag include drm/bridge like this ?  => "Fixes: drm/bridge: it6505: HDCP CTS fail 1B-xx"

About the reason about bug fixes. 

for example, the 1B-01 device count.
will this readable?

" When connect to HDCP repeater, it6505 must support 127 downstream devices. "

And this will be only one change in a patch?

>> 
>> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
>> ---
>>  drivers/gpu/drm/bridge/ite-it6505.c | 112 ++++++++++++++++++----------
>>  1 file changed, 74 insertions(+), 38 deletions(-)
>
>-- 
>With best wishes
>Dmitry

BR,
Hermes

Re: [PATCH v3 2/3] drm/bridge: it6505: HDCP CTS fail on repeater items

[Dmitry Baryshkov]

On Mon, Sep 23, 2024 at 10:45:49AM GMT, Hermes.Wu@ite.com.tw wrote:
> >On Mon, Sep 23, 2024 at 05:48:28PM GMT, Hermes Wu wrote:
> >> From: Hermes Wu <Hermes.wu@ite.com.tw>
> >> 
> >> Changes in v3:
> >>  -add detials about fail item and changes.
> >> 
> >> 
> >> Fix HDCP CTS fail items on UNIGRAF DRP-100
> >> 
> >> DUT must Support 127 devices.
> >> DUT must check BSTATUS when receive CP_IRQ.
> >> DUT must enable encryption when R0' is ready.
> >> DUT must retry V' check 3 times.
> >> it6505 must read DRP-100 KSV FIFO by FIFO mode.
> >> it6505 should restart HDCP within 5s if KSV not ready.
> >
> >Still not readable.
> >
> >English text, please. Split the patch to fix one issue at a time.
> >Describe the _reason_ for the change. Annotate fixes with Fixes tags.
> >
> 
> with fixes tag include drm/bridge like this ?  => "Fixes: drm/bridge: it6505: HDCP CTS fail 1B-xx"

No. Please read the document that I have been pointing you to. It
describes all the tags and procedures.

> 
> About the reason about bug fixes. 
> 
> for example, the 1B-01 device count.
> will this readable?
> 
> " When connect to HDCP repeater, it6505 must support 127 downstream devices. "
> 
> And this will be only one change in a patch?

Let me repeat the phrase that you have quoted few lines above. "Split
the patch to fix one issue at a time." So, no, this will not be the only
change in the patch.

> 
> >> 
> >> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
> >> ---
> >>  drivers/gpu/drm/bridge/ite-it6505.c | 112 ++++++++++++++++++----------
> >>  1 file changed, 74 insertions(+), 38 deletions(-)
> >
> >-- 
> >With best wishes
> >Dmitry
> 
> BR,
> Hermes

-- 
With best wishes
Dmitry

RE: [PATCH v3 2/3] drm/bridge: it6505: HDCP CTS fail on repeater items

[Hermes.Wu]

>On Mon, Sep 23, 2024 at 10:45:49AM GMT, Hermes.Wu@ite.com.tw wrote:
>> >On Mon, Sep 23, 2024 at 05:48:28PM GMT, Hermes Wu wrote:
>> >> From: Hermes Wu <Hermes.wu@ite.com.tw>
>> >> 
>> >> Changes in v3:
>> >>  -add detials about fail item and changes.
>> >> 
>> >> 
>> >> Fix HDCP CTS fail items on UNIGRAF DRP-100
>> >> 
>> >> DUT must Support 127 devices.
>> >> DUT must check BSTATUS when receive CP_IRQ.
>> >> DUT must enable encryption when R0' is ready.
>> >> DUT must retry V' check 3 times.
>> >> it6505 must read DRP-100 KSV FIFO by FIFO mode.
>> >> it6505 should restart HDCP within 5s if KSV not ready.
>> >
>> >Still not readable.
>> >
>> >English text, please. Split the patch to fix one issue at a time.
>> >Describe the _reason_ for the change. Annotate fixes with Fixes tags.
>> >
>> 
>> with fixes tag include drm/bridge like this ?  => "Fixes: drm/bridge: it6505: HDCP CTS fail 1B-xx"
>
>No. Please read the document that I have been pointing you to. It describes all the tags and procedures.
>
>> 
>> About the reason about bug fixes. 
>> 
>> for example, the 1B-01 device count.
>> will this readable?
>> 
>> " When connect to HDCP repeater, it6505 must support 127 downstream devices. "
>> 
>> And this will be only one change in a patch?
>
>Let me repeat the phrase that you have quoted few lines above. "Split the patch to fix one issue at a time." So, no, this will not be the only change in the patch.
>

The HDCP CTS include serval items, I should split each failure item fixes into different patch?


>> 
>> >> 
>> >> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
>> >> ---
>> >>  drivers/gpu/drm/bridge/ite-it6505.c | 112 
>> >> ++++++++++++++++++----------
>> >>  1 file changed, 74 insertions(+), 38 deletions(-)
>> >
>> >--
>> >With best wishes
>> >Dmitry
>> 
>> BR,
>> Hermes
>
>--
>With best wishes
>Dmitry

Re: [PATCH v3 2/3] drm/bridge: it6505: HDCP CTS fail on repeater items

[Pin-yen Lin]

On Tue, Sep 24, 2024 at 10:57 AM <Hermes.Wu@ite.com.tw> wrote:
>
> >On Mon, Sep 23, 2024 at 10:45:49AM GMT, Hermes.Wu@ite.com.tw wrote:
> >> >On Mon, Sep 23, 2024 at 05:48:28PM GMT, Hermes Wu wrote:
> >> >> From: Hermes Wu <Hermes.wu@ite.com.tw>
> >> >>
> >> >> Changes in v3:
> >> >>  -add detials about fail item and changes.
> >> >>
> >> >>
> >> >> Fix HDCP CTS fail items on UNIGRAF DRP-100
> >> >>
> >> >> DUT must Support 127 devices.
> >> >> DUT must check BSTATUS when receive CP_IRQ.
> >> >> DUT must enable encryption when R0' is ready.
> >> >> DUT must retry V' check 3 times.
> >> >> it6505 must read DRP-100 KSV FIFO by FIFO mode.
> >> >> it6505 should restart HDCP within 5s if KSV not ready.
> >> >
> >> >Still not readable.
> >> >
> >> >English text, please. Split the patch to fix one issue at a time.
> >> >Describe the _reason_ for the change. Annotate fixes with Fixes tags.
> >> >
> >>
> >> with fixes tag include drm/bridge like this ?  => "Fixes: drm/bridge: it6505: HDCP CTS fail 1B-xx"
> >
> >No. Please read the document that I have been pointing you to. It describes all the tags and procedures.
> >
> >>
> >> About the reason about bug fixes.
> >>
> >> for example, the 1B-01 device count.
> >> will this readable?
> >>
> >> " When connect to HDCP repeater, it6505 must support 127 downstream devices. "
> >>
> >> And this will be only one change in a patch?
> >
> >Let me repeat the phrase that you have quoted few lines above. "Split the patch to fix one issue at a time." So, no, this will not be the only change in the patch.
> >
>
> The HDCP CTS include serval items, I should split each failure item fixes into different patch?

Yes, please. You can mention in the cover letter that those patches
are fixing HDCP CTS failures, but please fix one issue at a time and
explain what it fixes in the commit message.
>
>
> >>
> >> >>
> >> >> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
> >> >> ---
> >> >>  drivers/gpu/drm/bridge/ite-it6505.c | 112
> >> >> ++++++++++++++++++----------
> >> >>  1 file changed, 74 insertions(+), 38 deletions(-)
> >> >
> >> >--
> >> >With best wishes
> >> >Dmitry
> >>
> >> BR,
> >> Hermes
> >
> >--
> >With best wishes
> >Dmitry

Regards,
Pin-yen

Re: [PATCH v3 2/3] drm/bridge: it6505: HDCP CTS fail on repeater items

[kernel test robot]

Hi Hermes,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm/drm-next]
[also build test WARNING on drm-exynos/exynos-drm-next drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-misc/drm-misc-next drm-tip/drm-tip linus/master v6.11 next-20240923]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Hermes-Wu/drm-bridge-it6505-fix-AUX-read-use-aux-fifo/20240923-175041
base:   git://anongit.freedesktop.org/drm/drm drm-next
patch link:    https://lore.kernel.org/r/20240923094826.13471-3-Hermes.Wu%40ite.com.tw
patch subject: [PATCH v3 2/3] drm/bridge: it6505: HDCP CTS fail on repeater items
config: nios2-randconfig-r073-20240924 (https://download.01.org/0day-ci/archive/20240924/202409241856.OWgR1x3Y-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 14.1.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409241856.OWgR1x3Y-lkp@intel.com/

smatch warnings:
drivers/gpu/drm/bridge/ite-it6505.c:2027 it6505_setup_sha1_input() warn: inconsistent indenting

vim +2027 drivers/gpu/drm/bridge/ite-it6505.c

b5c84a9edcd418 Allen Chen  2022-01-14  1996  
b5c84a9edcd418 Allen Chen  2022-01-14  1997  static int it6505_setup_sha1_input(struct it6505 *it6505, u8 *sha1_input)
b5c84a9edcd418 Allen Chen  2022-01-14  1998  {
d65feac281ab47 Pin-yen Lin 2023-07-18  1999  	struct device *dev = it6505->dev;
b5c84a9edcd418 Allen Chen  2022-01-14  2000  	u8 binfo[2];
9665163df60e16 Hermes Wu   2024-09-23  2001  	int down_stream_count, err, msg_count = 0;
b5c84a9edcd418 Allen Chen  2022-01-14  2002  
b5c84a9edcd418 Allen Chen  2022-01-14  2003  	err = it6505_get_dpcd(it6505, DP_AUX_HDCP_BINFO, binfo,
b5c84a9edcd418 Allen Chen  2022-01-14  2004  			      ARRAY_SIZE(binfo));
b5c84a9edcd418 Allen Chen  2022-01-14  2005  
b5c84a9edcd418 Allen Chen  2022-01-14  2006  	if (err < 0) {
b5c84a9edcd418 Allen Chen  2022-01-14  2007  		dev_err(dev, "Read binfo value Fail");
b5c84a9edcd418 Allen Chen  2022-01-14  2008  		return err;
b5c84a9edcd418 Allen Chen  2022-01-14  2009  	}
b5c84a9edcd418 Allen Chen  2022-01-14  2010  
b5c84a9edcd418 Allen Chen  2022-01-14  2011  	down_stream_count = binfo[0] & 0x7F;
b5c84a9edcd418 Allen Chen  2022-01-14  2012  	DRM_DEV_DEBUG_DRIVER(dev, "binfo:0x%*ph", (int)ARRAY_SIZE(binfo),
b5c84a9edcd418 Allen Chen  2022-01-14  2013  			     binfo);
b5c84a9edcd418 Allen Chen  2022-01-14  2014  
b5c84a9edcd418 Allen Chen  2022-01-14  2015  	if ((binfo[0] & BIT(7)) || (binfo[1] & BIT(3))) {
b5c84a9edcd418 Allen Chen  2022-01-14  2016  		dev_err(dev, "HDCP max cascade device exceed");
b5c84a9edcd418 Allen Chen  2022-01-14  2017  		return 0;
b5c84a9edcd418 Allen Chen  2022-01-14  2018  	}
b5c84a9edcd418 Allen Chen  2022-01-14  2019  
b5c84a9edcd418 Allen Chen  2022-01-14  2020  	if (!down_stream_count ||
b5c84a9edcd418 Allen Chen  2022-01-14  2021  	    down_stream_count > MAX_HDCP_DOWN_STREAM_COUNT) {
b5c84a9edcd418 Allen Chen  2022-01-14  2022  		dev_err(dev, "HDCP down stream count Error %d",
b5c84a9edcd418 Allen Chen  2022-01-14  2023  			down_stream_count);
b5c84a9edcd418 Allen Chen  2022-01-14  2024  		return 0;
b5c84a9edcd418 Allen Chen  2022-01-14  2025  	}
9665163df60e16 Hermes Wu   2024-09-23  2026  	err =  it6505_get_ksvlist(it6505, sha1_input, down_stream_count * 5);
b5c84a9edcd418 Allen Chen  2022-01-14 @2027  		if (err < 0)
b5c84a9edcd418 Allen Chen  2022-01-14  2028  			return err;
b5c84a9edcd418 Allen Chen  2022-01-14  2029  
9665163df60e16 Hermes Wu   2024-09-23  2030  	msg_count += down_stream_count * 5;
b5c84a9edcd418 Allen Chen  2022-01-14  2031  
b5c84a9edcd418 Allen Chen  2022-01-14  2032  	it6505->hdcp_down_stream_count = down_stream_count;
b5c84a9edcd418 Allen Chen  2022-01-14  2033  	sha1_input[msg_count++] = binfo[0];
b5c84a9edcd418 Allen Chen  2022-01-14  2034  	sha1_input[msg_count++] = binfo[1];
b5c84a9edcd418 Allen Chen  2022-01-14  2035  
b5c84a9edcd418 Allen Chen  2022-01-14  2036  	it6505_set_bits(it6505, REG_HDCP_CTRL2, HDCP_EN_M0_READ,
b5c84a9edcd418 Allen Chen  2022-01-14  2037  			HDCP_EN_M0_READ);
b5c84a9edcd418 Allen Chen  2022-01-14  2038  
b5c84a9edcd418 Allen Chen  2022-01-14  2039  	err = regmap_bulk_read(it6505->regmap, REG_M0_0_7,
b5c84a9edcd418 Allen Chen  2022-01-14  2040  			       sha1_input + msg_count, 8);
b5c84a9edcd418 Allen Chen  2022-01-14  2041  
b5c84a9edcd418 Allen Chen  2022-01-14  2042  	it6505_set_bits(it6505, REG_HDCP_CTRL2, HDCP_EN_M0_READ, 0x00);
b5c84a9edcd418 Allen Chen  2022-01-14  2043  
b5c84a9edcd418 Allen Chen  2022-01-14  2044  	if (err < 0) {
b5c84a9edcd418 Allen Chen  2022-01-14  2045  		dev_err(dev, " Warning, Read M value Fail");
b5c84a9edcd418 Allen Chen  2022-01-14  2046  		return err;
b5c84a9edcd418 Allen Chen  2022-01-14  2047  	}
b5c84a9edcd418 Allen Chen  2022-01-14  2048  
b5c84a9edcd418 Allen Chen  2022-01-14  2049  	msg_count += 8;
b5c84a9edcd418 Allen Chen  2022-01-14  2050  
b5c84a9edcd418 Allen Chen  2022-01-14  2051  	return msg_count;
b5c84a9edcd418 Allen Chen  2022-01-14  2052  }
b5c84a9edcd418 Allen Chen  2022-01-14  2053  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v3 3/3] drm/bridge: it6505: Add MCCS support

[Hermes Wu]

From: Hermes Wu <Hermes.wu@ite.com.tw>

Changes in v3:
 -remove non used definition for aux i2x cmd reply

Add Aux-I2C functionality to support MCCS.

Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 drivers/gpu/drm/bridge/ite-it6505.c | 174 +++++++++++++++++++++++++++-
 1 file changed, 172 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index 156440c6517e..5aedc5570739 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -125,6 +125,9 @@
 #define REG_AUX_ADR_16_19 0x26
 #define REG_AUX_OUT_DATA0 0x27
 
+#define REG_AUX_I2C_ADR 0x25
+#define REG_AUX_I2C_OP 0x26
+
 #define REG_AUX_CMD_REQ 0x2B
 #define AUX_BUSY BIT(5)
 
@@ -266,6 +269,19 @@
 #define REG_SSC_CTRL1 0x189
 #define REG_SSC_CTRL2 0x18A
 
+#define REG_AUX_USER_CTRL 0x190
+#define EN_USER_AUX BIT(0)
+#define USER_AUX_DONE BIT(1)
+#define AUX_EVENT BIT(4)
+
+#define REG_AUX_USER_DATA_REC 0x191
+#define M_AUX_IN_REC   0xF0
+#define M_AUX_OUT_REC  0x0F
+
+#define REG_AUX_USER_TXB 0x190
+#define REG_AUX_USER_REPLY 0x19A
+#define REG_AUX_USER_RXB(n) (n + 0x19B)
+
 #define RBR DP_LINK_BW_1_62
 #define HBR DP_LINK_BW_2_7
 #define HBR2 DP_LINK_BW_5_4
@@ -301,6 +317,8 @@
 #define MAX_EQ_LEVEL 0x03
 #define AUX_WAIT_TIMEOUT_MS 15
 #define AUX_FIFO_MAX_SIZE 16
+#define AUX_I2C_MAX_SIZE 4
+#define AUX_I2C_DEFER_RETRY 4
 #define PIXEL_CLK_DELAY 1
 #define PIXEL_CLK_INVERSE 0
 #define ADJUST_PHASE_THRESHOLD 80000
@@ -323,7 +341,12 @@
 enum aux_cmd_type {
 	CMD_AUX_NATIVE_READ = 0x0,
 	CMD_AUX_NATIVE_WRITE = 0x5,
+	CMD_AUX_GI2C_ADR = 0x08,
+	CMD_AUX_GI2C_READ = 0x09,
+	CMD_AUX_GI2C_WRITE = 0x0A,
 	CMD_AUX_I2C_EDID_READ = 0xB,
+	CMD_AUX_I2C_READ = 0x0D,
+	CMD_AUX_I2C_WRITE = 0x0C,
 
 	/* KSV list read using AUX native read with FIFO */
 	CMD_AUX_GET_KSV_LIST = 0x10,
@@ -1106,6 +1129,154 @@ static ssize_t it6505_aux_do_transfer(struct it6505 *it6505,
 	return ret;
 }
 
+static int it6505_aux_i2c_wait(struct it6505 *it6505, u8 *reply)
+{
+	int err = 0;
+	unsigned long timeout;
+	struct device *dev = it6505->dev;
+
+	timeout = jiffies + msecs_to_jiffies(AUX_WAIT_TIMEOUT_MS) + 1;
+
+	do {
+		if (it6505_read(it6505, REG_AUX_USER_CTRL) & AUX_EVENT)
+			break;
+		if (time_after(jiffies, timeout)) {
+			dev_err(dev, "Timed out waiting AUX I2C, BUSY = %X\n",
+				it6505_aux_op_finished(it6505));
+			err = -ETIMEDOUT;
+			goto end_aux_i2c_wait;
+		}
+		usleep_range(300, 800);
+	} while (!it6505_aux_op_finished(it6505));
+
+	if (!reply)
+		goto end_aux_i2c_wait;
+
+	*reply = it6505_read(it6505, REG_AUX_USER_REPLY) >> 4;
+
+	if (*reply == 0)
+		goto end_aux_i2c_wait;
+
+	if ((*reply == DP_AUX_NATIVE_REPLY_DEFER) ||
+	    (*reply == DP_AUX_I2C_REPLY_DEFER))
+		err = -EBUSY;
+	else if ((*reply == DP_AUX_NATIVE_REPLY_NACK) ||
+		 (*reply == DP_AUX_I2C_REPLY_NACK))
+		err = -ENXIO;
+
+end_aux_i2c_wait:
+	it6505_set_bits(it6505, REG_AUX_USER_CTRL, USER_AUX_DONE, USER_AUX_DONE);
+	return err;
+}
+
+static int it6505_aux_i2c_readb(struct it6505 *it6505, u8 *buf, size_t size, u8 *reply)
+{
+	int ret, i;
+	int retry = 0;
+
+	for (retry = 0; retry < AUX_I2C_DEFER_RETRY; retry++) {
+		it6505_write(it6505, REG_AUX_CMD_REQ, CMD_AUX_GI2C_READ);
+		ret = it6505_aux_i2c_wait(it6505, reply);
+		if ((*reply == DP_AUX_NATIVE_REPLY_DEFER) ||
+		    (*reply == DP_AUX_I2C_REPLY_DEFER))
+			continue;
+		if (ret >= 0)
+			break;
+	}
+
+	for (i = 0; i < size; i++)
+		buf[i] =  (u8)it6505_read(it6505, REG_AUX_USER_RXB(0 + i));
+
+	return size;
+}
+
+static int it6505_aux_i2c_writeb(struct it6505 *it6505, u8 *buf, size_t size, u8 *reply)
+{
+	int i, ret;
+	int retry = 0;
+
+	for (i = 0; i < size; i++)
+		it6505_write(it6505, REG_AUX_OUT_DATA0 + i, buf[i]);
+
+	for (retry = 0; retry < AUX_I2C_DEFER_RETRY; retry++) {
+		it6505_write(it6505, REG_AUX_CMD_REQ, CMD_AUX_GI2C_WRITE);
+		ret = it6505_aux_i2c_wait(it6505, reply);
+		if ((*reply == DP_AUX_NATIVE_REPLY_DEFER) ||
+		    (*reply == DP_AUX_I2C_REPLY_DEFER))
+			continue;
+		if (ret >= 0)
+			break;
+	}
+	return size;
+}
+
+static ssize_t it6505_aux_i2c_operation(struct it6505 *it6505,
+					struct drm_dp_aux_msg *msg)
+{
+	int ret;
+	ssize_t request_size, data_cnt = 0;
+	u8 *buffer = msg->buffer;
+
+	/* set AUX user mode */
+	it6505_set_bits(it6505, REG_AUX_CTRL,
+			AUX_USER_MODE | AUX_NO_SEGMENT_WR, AUX_USER_MODE);
+	it6505_set_bits(it6505, REG_AUX_USER_CTRL, EN_USER_AUX, EN_USER_AUX);
+	/* clear AUX FIFO */
+	it6505_set_bits(it6505, REG_AUX_CTRL,
+			AUX_EN_FIFO_READ | CLR_EDID_FIFO,
+			AUX_EN_FIFO_READ | CLR_EDID_FIFO);
+
+	it6505_set_bits(it6505, REG_AUX_CTRL,
+			AUX_EN_FIFO_READ | CLR_EDID_FIFO, 0x00);
+
+	it6505_write(it6505, REG_AUX_ADR_0_7, 0x00);
+	it6505_write(it6505, REG_AUX_I2C_ADR, msg->address << 1);
+
+	if (msg->size == 0) {
+		/* IIC Start/STOP dummy write */
+		it6505_write(it6505, REG_AUX_I2C_OP, msg->request);
+		it6505_write(it6505, REG_AUX_CMD_REQ, CMD_AUX_GI2C_ADR);
+		ret = it6505_aux_i2c_wait(it6505, &msg->reply);
+		goto end_aux_i2c_transfer;
+	}
+
+	/* IIC data transfer */
+	for (data_cnt = 0; data_cnt < msg->size; ) {
+		request_size = min_t(ssize_t, msg->size - data_cnt, AUX_I2C_MAX_SIZE);
+		it6505_write(it6505, REG_AUX_I2C_OP,
+			     msg->request | ((request_size - 1) << 4));
+		if ((msg->request & DP_AUX_I2C_READ) == DP_AUX_I2C_READ)
+			ret = it6505_aux_i2c_readb(it6505, &buffer[data_cnt],
+						   request_size, &msg->reply);
+		else
+			ret = it6505_aux_i2c_writeb(it6505, &buffer[data_cnt],
+						    request_size, &msg->reply);
+
+		if (ret < 0)
+			goto end_aux_i2c_transfer;
+
+		data_cnt += request_size;
+	}
+	ret = data_cnt;
+end_aux_i2c_transfer:
+
+	it6505_set_bits(it6505, REG_AUX_USER_CTRL, EN_USER_AUX, 0);
+	it6505_set_bits(it6505, REG_AUX_CTRL, AUX_USER_MODE, 0);
+	return ret;
+}
+
+static ssize_t it6505_aux_i2c_transfer(struct drm_dp_aux *aux,
+				       struct drm_dp_aux_msg *msg)
+{
+	struct it6505 *it6505 = container_of(aux, struct it6505, aux);
+	int ret;
+
+	mutex_lock(&it6505->aux_lock);
+	ret = it6505_aux_i2c_operation(it6505, msg);
+	mutex_unlock(&it6505->aux_lock);
+	return ret;
+}
+
 static ssize_t it6505_aux_transfer(struct drm_dp_aux *aux,
 				   struct drm_dp_aux_msg *msg)
 {
@@ -1115,9 +1286,8 @@ static ssize_t it6505_aux_transfer(struct drm_dp_aux *aux,
 	int ret;
 	enum aux_cmd_reply reply;
 
-	/* IT6505 doesn't support arbitrary I2C read / write. */
 	if (is_i2c)
-		return -EINVAL;
+		return it6505_aux_i2c_transfer(aux, msg);
 
 	switch (msg->request) {
 	case DP_AUX_NATIVE_READ:
-- 
2.34.1

Re: [PATCH v3 3/3] drm/bridge: it6505: Add MCCS support

[Dmitry Baryshkov]

On Mon, Sep 23, 2024 at 05:48:29PM GMT, Hermes Wu wrote:
> From: Hermes Wu <Hermes.wu@ite.com.tw>
> 
> Changes in v3:
>  -remove non used definition for aux i2x cmd reply
> 
> Add Aux-I2C functionality to support MCCS.
> 
> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
> ---
>  drivers/gpu/drm/bridge/ite-it6505.c | 174 +++++++++++++++++++++++++++-
>  1 file changed, 172 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index 156440c6517e..5aedc5570739 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -125,6 +125,9 @@
>  #define REG_AUX_ADR_16_19 0x26
>  #define REG_AUX_OUT_DATA0 0x27
>  
> +#define REG_AUX_I2C_ADR 0x25
> +#define REG_AUX_I2C_OP 0x26
> +

Are these registers CMD-specific? Because I see that you already have
defines for 0x25 and 0x26.

>  #define REG_AUX_CMD_REQ 0x2B
>  #define AUX_BUSY BIT(5)
>  
> @@ -266,6 +269,19 @@
>  #define REG_SSC_CTRL1 0x189
>  #define REG_SSC_CTRL2 0x18A
>  
> +#define REG_AUX_USER_CTRL 0x190
> +#define EN_USER_AUX BIT(0)
> +#define USER_AUX_DONE BIT(1)
> +#define AUX_EVENT BIT(4)
> +
> +#define REG_AUX_USER_DATA_REC 0x191
> +#define M_AUX_IN_REC   0xF0
> +#define M_AUX_OUT_REC  0x0F
> +
> +#define REG_AUX_USER_TXB 0x190

And two defines for 0x190 too.

> +#define REG_AUX_USER_REPLY 0x19A
> +#define REG_AUX_USER_RXB(n) (n + 0x19B)
> +
>  #define RBR DP_LINK_BW_1_62
>  #define HBR DP_LINK_BW_2_7
>  #define HBR2 DP_LINK_BW_5_4
> @@ -301,6 +317,8 @@
>  #define MAX_EQ_LEVEL 0x03
>  #define AUX_WAIT_TIMEOUT_MS 15
>  #define AUX_FIFO_MAX_SIZE 16
> +#define AUX_I2C_MAX_SIZE 4
> +#define AUX_I2C_DEFER_RETRY 4
>  #define PIXEL_CLK_DELAY 1
>  #define PIXEL_CLK_INVERSE 0
>  #define ADJUST_PHASE_THRESHOLD 80000
> @@ -323,7 +341,12 @@
>  enum aux_cmd_type {
>  	CMD_AUX_NATIVE_READ = 0x0,
>  	CMD_AUX_NATIVE_WRITE = 0x5,
> +	CMD_AUX_GI2C_ADR = 0x08,
> +	CMD_AUX_GI2C_READ = 0x09,
> +	CMD_AUX_GI2C_WRITE = 0x0A,
>  	CMD_AUX_I2C_EDID_READ = 0xB,
> +	CMD_AUX_I2C_READ = 0x0D,
> +	CMD_AUX_I2C_WRITE = 0x0C,
>  
>  	/* KSV list read using AUX native read with FIFO */
>  	CMD_AUX_GET_KSV_LIST = 0x10,
> @@ -1106,6 +1129,154 @@ static ssize_t it6505_aux_do_transfer(struct it6505 *it6505,
>  	return ret;
>  }
>  
> +static int it6505_aux_i2c_wait(struct it6505 *it6505, u8 *reply)
> +{
> +	int err = 0;

Skip assignment here.

> +	unsigned long timeout;
> +	struct device *dev = it6505->dev;
> +
> +	timeout = jiffies + msecs_to_jiffies(AUX_WAIT_TIMEOUT_MS) + 1;
> +
> +	do {
> +		if (it6505_read(it6505, REG_AUX_USER_CTRL) & AUX_EVENT)
> +			break;
> +		if (time_after(jiffies, timeout)) {
> +			dev_err(dev, "Timed out waiting AUX I2C, BUSY = %X\n",
> +				it6505_aux_op_finished(it6505));
> +			err = -ETIMEDOUT;
> +			goto end_aux_i2c_wait;
> +		}
> +		usleep_range(300, 800);
> +	} while (!it6505_aux_op_finished(it6505));
> +
> +	if (!reply)
> +		goto end_aux_i2c_wait;
> +
> +	*reply = it6505_read(it6505, REG_AUX_USER_REPLY) >> 4;
> +
> +	if (*reply == 0)
> +		goto end_aux_i2c_wait;

assign err = 0 here, so that it's obvious that it's a successfull case.

> +
> +	if ((*reply == DP_AUX_NATIVE_REPLY_DEFER) ||
> +	    (*reply == DP_AUX_I2C_REPLY_DEFER))
> +		err = -EBUSY;
> +	else if ((*reply == DP_AUX_NATIVE_REPLY_NACK) ||
> +		 (*reply == DP_AUX_I2C_REPLY_NACK))
> +		err = -ENXIO;
> +
> +end_aux_i2c_wait:
> +	it6505_set_bits(it6505, REG_AUX_USER_CTRL, USER_AUX_DONE, USER_AUX_DONE);
> +	return err;
> +}
> +
> +static int it6505_aux_i2c_readb(struct it6505 *it6505, u8 *buf, size_t size, u8 *reply)
> +{
> +	int ret, i;
> +	int retry = 0;

Skip the init

> +
> +	for (retry = 0; retry < AUX_I2C_DEFER_RETRY; retry++) {
> +		it6505_write(it6505, REG_AUX_CMD_REQ, CMD_AUX_GI2C_READ);

empty line

> +		ret = it6505_aux_i2c_wait(it6505, reply);
> +		if ((*reply == DP_AUX_NATIVE_REPLY_DEFER) ||
> +		    (*reply == DP_AUX_I2C_REPLY_DEFER))

These two lines keep on being repeated over and over. Please consider
defining a helper function.

> +			continue;
> +		if (ret >= 0)
> +			break;
> +	}
> +
> +	for (i = 0; i < size; i++)
> +		buf[i] =  (u8)it6505_read(it6505, REG_AUX_USER_RXB(0 + i));

Single space, drop type conversion.

> +
> +	return size;
> +}
> +
> +static int it6505_aux_i2c_writeb(struct it6505 *it6505, u8 *buf, size_t size, u8 *reply)
> +{
> +	int i, ret;
> +	int retry = 0;
> +
> +	for (i = 0; i < size; i++)
> +		it6505_write(it6505, REG_AUX_OUT_DATA0 + i, buf[i]);
> +
> +	for (retry = 0; retry < AUX_I2C_DEFER_RETRY; retry++) {
> +		it6505_write(it6505, REG_AUX_CMD_REQ, CMD_AUX_GI2C_WRITE);

empty line

> +		ret = it6505_aux_i2c_wait(it6505, reply);
> +		if ((*reply == DP_AUX_NATIVE_REPLY_DEFER) ||
> +		    (*reply == DP_AUX_I2C_REPLY_DEFER))
> +			continue;
> +		if (ret >= 0)
> +			break;
> +	}
> +	return size;
> +}
> +
> +static ssize_t it6505_aux_i2c_operation(struct it6505 *it6505,
> +					struct drm_dp_aux_msg *msg)
> +{
> +	int ret;
> +	ssize_t request_size, data_cnt = 0;
> +	u8 *buffer = msg->buffer;
> +
> +	/* set AUX user mode */
> +	it6505_set_bits(it6505, REG_AUX_CTRL,
> +			AUX_USER_MODE | AUX_NO_SEGMENT_WR, AUX_USER_MODE);
> +	it6505_set_bits(it6505, REG_AUX_USER_CTRL, EN_USER_AUX, EN_USER_AUX);
> +	/* clear AUX FIFO */
> +	it6505_set_bits(it6505, REG_AUX_CTRL,
> +			AUX_EN_FIFO_READ | CLR_EDID_FIFO,
> +			AUX_EN_FIFO_READ | CLR_EDID_FIFO);
> +
> +	it6505_set_bits(it6505, REG_AUX_CTRL,
> +			AUX_EN_FIFO_READ | CLR_EDID_FIFO, 0x00);
> +
> +	it6505_write(it6505, REG_AUX_ADR_0_7, 0x00);
> +	it6505_write(it6505, REG_AUX_I2C_ADR, msg->address << 1);
> +
> +	if (msg->size == 0) {
> +		/* IIC Start/STOP dummy write */
> +		it6505_write(it6505, REG_AUX_I2C_OP, msg->request);
> +		it6505_write(it6505, REG_AUX_CMD_REQ, CMD_AUX_GI2C_ADR);
> +		ret = it6505_aux_i2c_wait(it6505, &msg->reply);
> +		goto end_aux_i2c_transfer;
> +	}
> +
> +	/* IIC data transfer */
> +	for (data_cnt = 0; data_cnt < msg->size; ) {
> +		request_size = min_t(ssize_t, msg->size - data_cnt, AUX_I2C_MAX_SIZE);
> +		it6505_write(it6505, REG_AUX_I2C_OP,
> +			     msg->request | ((request_size - 1) << 4));
> +		if ((msg->request & DP_AUX_I2C_READ) == DP_AUX_I2C_READ)
> +			ret = it6505_aux_i2c_readb(it6505, &buffer[data_cnt],
> +						   request_size, &msg->reply);
> +		else
> +			ret = it6505_aux_i2c_writeb(it6505, &buffer[data_cnt],
> +						    request_size, &msg->reply);
> +
> +		if (ret < 0)
> +			goto end_aux_i2c_transfer;
> +
> +		data_cnt += request_size;
> +	}
> +	ret = data_cnt;
> +end_aux_i2c_transfer:
> +
> +	it6505_set_bits(it6505, REG_AUX_USER_CTRL, EN_USER_AUX, 0);
> +	it6505_set_bits(it6505, REG_AUX_CTRL, AUX_USER_MODE, 0);
> +	return ret;
> +}
> +
> +static ssize_t it6505_aux_i2c_transfer(struct drm_dp_aux *aux,
> +				       struct drm_dp_aux_msg *msg)
> +{
> +	struct it6505 *it6505 = container_of(aux, struct it6505, aux);
> +	int ret;
> +
> +	mutex_lock(&it6505->aux_lock);
> +	ret = it6505_aux_i2c_operation(it6505, msg);
> +	mutex_unlock(&it6505->aux_lock);

Is it enough to protect from other commands sending data in parallel?
Also as much as I don't like it, in this case using guard() from
cleanup.h will remove a need for a separte function.

> +	return ret;
> +}
> +
>  static ssize_t it6505_aux_transfer(struct drm_dp_aux *aux,
>  				   struct drm_dp_aux_msg *msg)
>  {
> @@ -1115,9 +1286,8 @@ static ssize_t it6505_aux_transfer(struct drm_dp_aux *aux,
>  	int ret;
>  	enum aux_cmd_reply reply;
>  
> -	/* IT6505 doesn't support arbitrary I2C read / write. */
>  	if (is_i2c)
> -		return -EINVAL;
> +		return it6505_aux_i2c_transfer(aux, msg);
>  
>  	switch (msg->request) {
>  	case DP_AUX_NATIVE_READ:
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

RE: [PATCH v3 3/3] drm/bridge: it6505: Add MCCS support

[Hermes.Wu]

>On Mon, Sep 23, 2024 at 05:48:29PM GMT, Hermes Wu wrote:
>> From: Hermes Wu <Hermes.wu@ite.com.tw>
>> 
>> Changes in v3:
>>  -remove non used definition for aux i2x cmd reply
>> 
>> Add Aux-I2C functionality to support MCCS.
>> 
>> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
>> ---
>>  drivers/gpu/drm/bridge/ite-it6505.c | 174 
>> +++++++++++++++++++++++++++-
>>  1 file changed, 172 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c 
>> b/drivers/gpu/drm/bridge/ite-it6505.c
>> index 156440c6517e..5aedc5570739 100644
>> --- a/drivers/gpu/drm/bridge/ite-it6505.c
>> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
>> @@ -125,6 +125,9 @@
>>  #define REG_AUX_ADR_16_19 0x26
>>  #define REG_AUX_OUT_DATA0 0x27
>>  
>> +#define REG_AUX_I2C_ADR 0x25
>> +#define REG_AUX_I2C_OP 0x26
>> +
>
>Are these registers CMD-specific? Because I see that you already have defines for 0x25 and 0x26.
>

The AUX packet i2c into aux transfer frames,
and I think it's easier to understand how it6505_aux_i2c_operation() packet i2c request into aux frame.

>>  #define REG_AUX_CMD_REQ 0x2B
>>  #define AUX_BUSY BIT(5)
>>  
>> @@ -266,6 +269,19 @@
>>  #define REG_SSC_CTRL1 0x189
>>  #define REG_SSC_CTRL2 0x18A
>>  
>> +#define REG_AUX_USER_CTRL 0x190
>> +#define EN_USER_AUX BIT(0)
>> +#define USER_AUX_DONE BIT(1)
>> +#define AUX_EVENT BIT(4)
>> +
>> +#define REG_AUX_USER_DATA_REC 0x191
>> +#define M_AUX_IN_REC   0xF0
>> +#define M_AUX_OUT_REC  0x0F
>> +
>> +#define REG_AUX_USER_TXB 0x190
>
>And two defines for 0x190 too.
>
>> +#define REG_AUX_USER_REPLY 0x19A
>> +#define REG_AUX_USER_RXB(n) (n + 0x19B)
>> +
>>  #define RBR DP_LINK_BW_1_62
>>  #define HBR DP_LINK_BW_2_7
>>  #define HBR2 DP_LINK_BW_5_4
>> @@ -301,6 +317,8 @@
>>  #define MAX_EQ_LEVEL 0x03
>>  #define AUX_WAIT_TIMEOUT_MS 15
>>  #define AUX_FIFO_MAX_SIZE 16
>> +#define AUX_I2C_MAX_SIZE 4
>> +#define AUX_I2C_DEFER_RETRY 4
>>  #define PIXEL_CLK_DELAY 1
>>  #define PIXEL_CLK_INVERSE 0
>>  #define ADJUST_PHASE_THRESHOLD 80000
>> @@ -323,7 +341,12 @@
>>  enum aux_cmd_type {
>>  	CMD_AUX_NATIVE_READ = 0x0,
>>  	CMD_AUX_NATIVE_WRITE = 0x5,
>> +	CMD_AUX_GI2C_ADR = 0x08,
>> +	CMD_AUX_GI2C_READ = 0x09,
>> +	CMD_AUX_GI2C_WRITE = 0x0A,
>>  	CMD_AUX_I2C_EDID_READ = 0xB,
>> +	CMD_AUX_I2C_READ = 0x0D,
>> +	CMD_AUX_I2C_WRITE = 0x0C,
>>  
>>  	/* KSV list read using AUX native read with FIFO */
>>  	CMD_AUX_GET_KSV_LIST = 0x10,
>> @@ -1106,6 +1129,154 @@ static ssize_t it6505_aux_do_transfer(struct it6505 *it6505,
>>  	return ret;
>>  }
>>  
>> +static int it6505_aux_i2c_wait(struct it6505 *it6505, u8 *reply) {
>> +	int err = 0;
>
>Skip assignment here.
>
>> +	unsigned long timeout;
>> +	struct device *dev = it6505->dev;
>> +
>> +	timeout = jiffies + msecs_to_jiffies(AUX_WAIT_TIMEOUT_MS) + 1;
>> +
>> +	do {
>> +		if (it6505_read(it6505, REG_AUX_USER_CTRL) & AUX_EVENT)
>> +			break;
>> +		if (time_after(jiffies, timeout)) {
>> +			dev_err(dev, "Timed out waiting AUX I2C, BUSY = %X\n",
>> +				it6505_aux_op_finished(it6505));
>> +			err = -ETIMEDOUT;
>> +			goto end_aux_i2c_wait;
>> +		}
>> +		usleep_range(300, 800);
>> +	} while (!it6505_aux_op_finished(it6505));
>> +
>> +	if (!reply)
>> +		goto end_aux_i2c_wait;
>> +
>> +	*reply = it6505_read(it6505, REG_AUX_USER_REPLY) >> 4;
>> +
>> +	if (*reply == 0)
>> +		goto end_aux_i2c_wait;
>
>assign err = 0 here, so that it's obvious that it's a successfull case.
>
>> +
>> +	if ((*reply == DP_AUX_NATIVE_REPLY_DEFER) ||
>> +	    (*reply == DP_AUX_I2C_REPLY_DEFER))
>> +		err = -EBUSY;
>> +	else if ((*reply == DP_AUX_NATIVE_REPLY_NACK) ||
>> +		 (*reply == DP_AUX_I2C_REPLY_NACK))
>> +		err = -ENXIO;
>> +
>> +end_aux_i2c_wait:
>> +	it6505_set_bits(it6505, REG_AUX_USER_CTRL, USER_AUX_DONE, USER_AUX_DONE);
>> +	return err;
>> +}
>> +
>> +static int it6505_aux_i2c_readb(struct it6505 *it6505, u8 *buf, 
>> +size_t size, u8 *reply) {
>> +	int ret, i;
>> +	int retry = 0;
>
>Skip the init
>
>> +
>> +	for (retry = 0; retry < AUX_I2C_DEFER_RETRY; retry++) {
>> +		it6505_write(it6505, REG_AUX_CMD_REQ, CMD_AUX_GI2C_READ);
>
>empty line
>
>> +		ret = it6505_aux_i2c_wait(it6505, reply);
>> +		if ((*reply == DP_AUX_NATIVE_REPLY_DEFER) ||
>> +		    (*reply == DP_AUX_I2C_REPLY_DEFER))
>
>These two lines keep on being repeated over and over. Please consider defining a helper function.
>
>> +			continue;
>> +		if (ret >= 0)
>> +			break;
>> +	}
>> +
>> +	for (i = 0; i < size; i++)
>> +		buf[i] =  (u8)it6505_read(it6505, REG_AUX_USER_RXB(0 + i));
>
>Single space, drop type conversion.
>
>> +
>> +	return size;
>> +}
>> +
>> +static int it6505_aux_i2c_writeb(struct it6505 *it6505, u8 *buf, 
>> +size_t size, u8 *reply) {
>> +	int i, ret;
>> +	int retry = 0;
>> +
>> +	for (i = 0; i < size; i++)
>> +		it6505_write(it6505, REG_AUX_OUT_DATA0 + i, buf[i]);
>> +
>> +	for (retry = 0; retry < AUX_I2C_DEFER_RETRY; retry++) {
>> +		it6505_write(it6505, REG_AUX_CMD_REQ, CMD_AUX_GI2C_WRITE);
>
>empty line
>
>> +		ret = it6505_aux_i2c_wait(it6505, reply);
>> +		if ((*reply == DP_AUX_NATIVE_REPLY_DEFER) ||
>> +		    (*reply == DP_AUX_I2C_REPLY_DEFER))
>> +			continue;
>> +		if (ret >= 0)
>> +			break;
>> +	}
>> +	return size;
>> +}
>> +
>> +static ssize_t it6505_aux_i2c_operation(struct it6505 *it6505,
>> +					struct drm_dp_aux_msg *msg)
>> +{
>> +	int ret;
>> +	ssize_t request_size, data_cnt = 0;
>> +	u8 *buffer = msg->buffer;
>> +
>> +	/* set AUX user mode */
>> +	it6505_set_bits(it6505, REG_AUX_CTRL,
>> +			AUX_USER_MODE | AUX_NO_SEGMENT_WR, AUX_USER_MODE);
>> +	it6505_set_bits(it6505, REG_AUX_USER_CTRL, EN_USER_AUX, EN_USER_AUX);
>> +	/* clear AUX FIFO */
>> +	it6505_set_bits(it6505, REG_AUX_CTRL,
>> +			AUX_EN_FIFO_READ | CLR_EDID_FIFO,
>> +			AUX_EN_FIFO_READ | CLR_EDID_FIFO);
>> +
>> +	it6505_set_bits(it6505, REG_AUX_CTRL,
>> +			AUX_EN_FIFO_READ | CLR_EDID_FIFO, 0x00);
>> +
>> +	it6505_write(it6505, REG_AUX_ADR_0_7, 0x00);
>> +	it6505_write(it6505, REG_AUX_I2C_ADR, msg->address << 1);
>> +
>> +	if (msg->size == 0) {
>> +		/* IIC Start/STOP dummy write */
>> +		it6505_write(it6505, REG_AUX_I2C_OP, msg->request);
>> +		it6505_write(it6505, REG_AUX_CMD_REQ, CMD_AUX_GI2C_ADR);
>> +		ret = it6505_aux_i2c_wait(it6505, &msg->reply);
>> +		goto end_aux_i2c_transfer;
>> +	}
>> +
>> +	/* IIC data transfer */
>> +	for (data_cnt = 0; data_cnt < msg->size; ) {
>> +		request_size = min_t(ssize_t, msg->size - data_cnt, AUX_I2C_MAX_SIZE);
>> +		it6505_write(it6505, REG_AUX_I2C_OP,
>> +			     msg->request | ((request_size - 1) << 4));
>> +		if ((msg->request & DP_AUX_I2C_READ) == DP_AUX_I2C_READ)
>> +			ret = it6505_aux_i2c_readb(it6505, &buffer[data_cnt],
>> +						   request_size, &msg->reply);
>> +		else
>> +			ret = it6505_aux_i2c_writeb(it6505, &buffer[data_cnt],
>> +						    request_size, &msg->reply);
>> +
>> +		if (ret < 0)
>> +			goto end_aux_i2c_transfer;
>> +
>> +		data_cnt += request_size;
>> +	}
>> +	ret = data_cnt;
>> +end_aux_i2c_transfer:
>> +
>> +	it6505_set_bits(it6505, REG_AUX_USER_CTRL, EN_USER_AUX, 0);
>> +	it6505_set_bits(it6505, REG_AUX_CTRL, AUX_USER_MODE, 0);
>> +	return ret;
>> +}
>> +
>> +static ssize_t it6505_aux_i2c_transfer(struct drm_dp_aux *aux,
>> +				       struct drm_dp_aux_msg *msg) {
>> +	struct it6505 *it6505 = container_of(aux, struct it6505, aux);
>> +	int ret;
>> +
>> +	mutex_lock(&it6505->aux_lock);
>> +	ret = it6505_aux_i2c_operation(it6505, msg);
>> +	mutex_unlock(&it6505->aux_lock);
>
>Is it enough to protect from other commands sending data in parallel?
>Also as much as I don't like it, in this case using guard() from cleanup.h will remove a need for a separte function.
>
>> +	return ret;
>> +}
>> +
>>  static ssize_t it6505_aux_transfer(struct drm_dp_aux *aux,
>>  				   struct drm_dp_aux_msg *msg)
>>  {
>> @@ -1115,9 +1286,8 @@ static ssize_t it6505_aux_transfer(struct drm_dp_aux *aux,
>>  	int ret;
>>  	enum aux_cmd_reply reply;
>>  
>> -	/* IT6505 doesn't support arbitrary I2C read / write. */
>>  	if (is_i2c)
>> -		return -EINVAL;
>> +		return it6505_aux_i2c_transfer(aux, msg);
>>  
>>  	switch (msg->request) {
>>  	case DP_AUX_NATIVE_READ:
>> --
>> 2.34.1
>> 
>
>-- 
>With best wishes
>Dmitry
>

BR,
Hermes

Re: [PATCH v3 3/3] drm/bridge: it6505: Add MCCS support

[Dmitry Baryshkov]

On Tue, Sep 24, 2024 at 03:52:34AM GMT, Hermes.Wu@ite.com.tw wrote:
> >On Mon, Sep 23, 2024 at 05:48:29PM GMT, Hermes Wu wrote:
> >> From: Hermes Wu <Hermes.wu@ite.com.tw>
> >> 
> >> Changes in v3:
> >>  -remove non used definition for aux i2x cmd reply
> >> 
> >> Add Aux-I2C functionality to support MCCS.
> >> 
> >> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
> >> ---
> >>  drivers/gpu/drm/bridge/ite-it6505.c | 174 
> >> +++++++++++++++++++++++++++-
> >>  1 file changed, 172 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c 
> >> b/drivers/gpu/drm/bridge/ite-it6505.c
> >> index 156440c6517e..5aedc5570739 100644
> >> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> >> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> >> @@ -125,6 +125,9 @@
> >>  #define REG_AUX_ADR_16_19 0x26
> >>  #define REG_AUX_OUT_DATA0 0x27
> >>  
> >> +#define REG_AUX_I2C_ADR 0x25
> >> +#define REG_AUX_I2C_OP 0x26
> >> +
> >
> >Are these registers CMD-specific? Because I see that you already have defines for 0x25 and 0x26.
> >
> 
> The AUX packet i2c into aux transfer frames,
> and I think it's easier to understand how it6505_aux_i2c_operation() packet i2c request into aux frame.

I'm really sorry, but I don't think I can parse this or how this answers
my question. If for the user I2C a part of the register space gets
repurposed, please comment that before the defines (and maybe separate
such defines so that it's obvious to anybody reading the driver).

> 
> >>  #define REG_AUX_CMD_REQ 0x2B
> >>  #define AUX_BUSY BIT(5)
> >>  
> >> @@ -266,6 +269,19 @@
> >>  #define REG_SSC_CTRL1 0x189
> >>  #define REG_SSC_CTRL2 0x18A
> >>  
> >> +#define REG_AUX_USER_CTRL 0x190
> >> +#define EN_USER_AUX BIT(0)
> >> +#define USER_AUX_DONE BIT(1)
> >> +#define AUX_EVENT BIT(4)
> >> +
> >> +#define REG_AUX_USER_DATA_REC 0x191
> >> +#define M_AUX_IN_REC   0xF0
> >> +#define M_AUX_OUT_REC  0x0F
> >> +
> >> +#define REG_AUX_USER_TXB 0x190
> >
> >And two defines for 0x190 too.
> >
> >> +#define REG_AUX_USER_REPLY 0x19A
> >> +#define REG_AUX_USER_RXB(n) (n + 0x19B)
> >> +
> >>  #define RBR DP_LINK_BW_1_62
> >>  #define HBR DP_LINK_BW_2_7
> >>  #define HBR2 DP_LINK_BW_5_4

-- 
With best wishes
Dmitry