[PATCH v4 00/11] fix HDCP CTS fail items and add MCCS support

[PATCH v4 00/11] fix HDCP CTS fail items and add MCCS support

[Hermes Wu]

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

This is a v4 patch-set.

There are lots of failure items while running HDCP CTS using UNIGRAF
DPR-100.
In Order to fix those failures, HDCP flow needs to be changed.

The DisplayPort AUX protocol supports I2C transport.
In Order to support MCCS via the aux channel, the aux-i2c operation is
added.

v3 ->v4:
	-split changes into patches.

v2- > v3:
	-split aux read  KSV function to a patch.
	-[1/3] new in v3
	-[2/3] add description of patch

v1 -> v2 :
	- ignored.

Link: https://lore.kernel.org/all/20240923094826.13471-2-Hermes.Wu@ite.com.tw/
Link: https://lore.kernel.org/all/20240923094826.13471-3-Hermes.Wu@ite.com.tw/
Link: https://lore.kernel.org/all/20240923094826.13471-4-Hermes.Wu@ite.com.tw/


Hermes Wu (11):
  drm/bridge: it6505: change aux max fifo size
  drm/bridge: it6505: improve aux operation for edid read
  drm/bridge: it6505: add aux operation for HDCP ksv list read
  drm/bridge: it6505: fix aux command write to aux operaction register
  drm/bridge: it6505: increase supports of HDCP repeater ksv devices
  drm/bridge: it6505: fix HDCP Bstatus check.
  drm/bridge: it6505: fix HDCP encription not enable when R0 ready
  drm/bridge: it6505: fix HDCP KSV list did not read correctly.
  drm/bridge: it6505: fix HDCP CTS compare V matching without retry
  drm/bridge: it6505: fix HDCP CTS ksv wait timer
  drm/bridge: it6505: Add aux i2c functionality

 drivers/gpu/drm/bridge/ite-it6505.c | 313 +++++++++++++++++++++++-----
 1 file changed, 266 insertions(+), 47 deletions(-)

-- 
2.34.1

[PATCH v4 01/11] drm/bridge: it6505: change aux max fifo size

[Hermes Wu]

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

The hardware aux fifo is 16 byte

Change definition of AUX_FIFO_MAX_SIZE to 16



Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 drivers/gpu/drm/bridge/ite-it6505.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index f372c05360f2..28a8043229d3 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
-- 
2.34.1

Re: [PATCH v4 01/11] drm/bridge: it6505: change aux max fifo size

[Dmitry Baryshkov]

On Thu, Sep 26, 2024 at 03:47:51PM GMT, Hermes Wu wrote:
> From: Hermes Wu <Hermes.wu@ite.com.tw>
> 
> The hardware aux fifo is 16 byte

Nit: AUX, FIFO

> 
> Change definition of AUX_FIFO_MAX_SIZE to 16
> 
> 
> 

Nit: no need for so many empty lines.


Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


> Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
> ---
>  drivers/gpu/drm/bridge/ite-it6505.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index f372c05360f2..28a8043229d3 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
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

[PATCH v4 02/11] drm/bridge: it6505: fix aux operation for edid read

[Hermes Wu]

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

The EDID read operation can reach the maximum size of the AUX FIFO buffer.

Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 drivers/gpu/drm/bridge/ite-it6505.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index 28a8043229d3..b451d3c2ac1d 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -1078,8 +1078,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)
+			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 +1091,7 @@ static ssize_t it6505_aux_do_transfer(struct it6505 *it6505,
 			goto aux_op_err;
 		}
 
+		i += request_size;
 		ret += ret_size;
 	}
 
-- 
2.34.1

Re: [PATCH v4 02/11] drm/bridge: it6505: fix aux operation for edid read

[Dmitry Baryshkov]

On Thu, Sep 26, 2024 at 03:47:52PM GMT, Hermes Wu wrote:
> From: Hermes Wu <Hermes.wu@ite.com.tw>
> 
> The EDID read operation can reach the maximum size of the AUX FIFO buffer.

And? Commit message should describe why the change is necessary and what
is being done. Just providing a statement is not enough.

> 
> Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
> ---
>  drivers/gpu/drm/bridge/ite-it6505.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index 28a8043229d3..b451d3c2ac1d 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -1078,8 +1078,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)
> +			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 +1091,7 @@ static ssize_t it6505_aux_do_transfer(struct it6505 *it6505,
>  			goto aux_op_err;
>  		}
>  
> +		i += request_size;
>  		ret += ret_size;
>  	}
>  
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

RE: [PATCH v4 02/11] drm/bridge: it6505: fix aux operation for edid read

[Hermes.Wu]

>On Thu, Sep 26, 2024 at 03:47:52PM GMT, Hermes Wu wrote:
>> From: Hermes Wu <Hermes.wu@ite.com.tw>
>> 
>> The EDID read operation can reach the maximum size of the AUX FIFO buffer.
>
>And? Commit message should describe why the change is necessary and what is being done. Just providing a statement is not enough.
>

The original AUX operation treat all reads by using data registers will be limited at 4 bytes.
AUX operation command CMD_AUX_I2C_EDID_READ using AUX FIFO is capable of reads data by 16 bytes each time.
It can improve speed of read EDID.


>> 
>> Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
>> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
>> ---
>>  drivers/gpu/drm/bridge/ite-it6505.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c 
>> b/drivers/gpu/drm/bridge/ite-it6505.c
>> index 28a8043229d3..b451d3c2ac1d 100644
>> --- a/drivers/gpu/drm/bridge/ite-it6505.c
>> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
>> @@ -1078,8 +1078,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)
>> +			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 +1091,7 @@ static ssize_t it6505_aux_do_transfer(struct it6505 *it6505,
>>  			goto aux_op_err;
>>  		}
>>  
>> +		i += request_size;
>>  		ret += ret_size;
>>  	}
>>  
>> --
>> 2.34.1
>> 
>
>-- 
>With best wishes
>Dmitry
>

BR,
Hermes

Re: [PATCH v4 02/11] drm/bridge: it6505: fix aux operation for edid read

[Dmitry Baryshkov]

On Thu, 26 Sept 2024 at 10:04, <Hermes.Wu@ite.com.tw> wrote:
>
> >On Thu, Sep 26, 2024 at 03:47:52PM GMT, Hermes Wu wrote:
> >> From: Hermes Wu <Hermes.wu@ite.com.tw>
> >>
> >> The EDID read operation can reach the maximum size of the AUX FIFO buffer.
> >
> >And? Commit message should describe why the change is necessary and what is being done. Just providing a statement is not enough.
> >
>
> The original AUX operation treat all reads by using data registers will be limited at 4 bytes.
> AUX operation command CMD_AUX_I2C_EDID_READ using AUX FIFO is capable of reads data by 16 bytes each time.
> It can improve speed of read EDID.

improves, rather than "can improve". Please add this to the commit message.

>
>
> >>
> >> Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
> >> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
> >> ---
> >>  drivers/gpu/drm/bridge/ite-it6505.c | 8 ++++++--
> >>  1 file changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c
> >> b/drivers/gpu/drm/bridge/ite-it6505.c
> >> index 28a8043229d3..b451d3c2ac1d 100644
> >> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> >> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> >> @@ -1078,8 +1078,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)
> >> +                    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 +1091,7 @@ static ssize_t it6505_aux_do_transfer(struct it6505 *it6505,
> >>                      goto aux_op_err;
> >>              }
> >>
> >> +            i += request_size;
> >>              ret += ret_size;
> >>      }
> >>
> >> --
> >> 2.34.1
> >>
> >
> >--
> >With best wishes
> >Dmitry
> >
>
> BR,
> Hermes
>


-- 
With best wishes
Dmitry

Re: [PATCH v4 02/11] drm/bridge: it6505: fix aux operation for edid read

[AngeloGioacchino Del Regno]

Il 26/09/24 09:47, Hermes Wu ha scritto:
> From: Hermes Wu <Hermes.wu@ite.com.tw>
> 
> The EDID read operation can reach the maximum size of the AUX FIFO buffer.
> 
> Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
> ---
>   drivers/gpu/drm/bridge/ite-it6505.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index 28a8043229d3..b451d3c2ac1d 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -1078,8 +1078,11 @@ static ssize_t it6505_aux_do_transfer(struct it6505 *it6505,
>   	int i, ret_size, ret = 0, request_size;

int fifo_max_sz = (cmd == CMD_AUX_I2C_EDID_READ) ?
		  AUX_FIFO_MAX_SIZE : 4;

which later becomes

int fifo_max_sz = (cmd == CMD_AUX_I2C_EDID_READ || cmd == CMD_AUX_GET_KSV_LIST) ?
		  AUX_FIFO_MAX_SIZE : 4;

otherwise you can do it "the long way"...

if (cmd == .....)
	fifo_max_sz = AUX_FIFO_MAX_SIZE;
else
	fifo_max_sz = 4;

The point is, cmd won't change during iterations, so it's useless to put that if
branch inside of that loop below.

>   
>   	mutex_lock(&it6505->aux_lock);
> -	for (i = 0; i < size; i += 4) {
> -		request_size = min((int)size - i, 4);

		request_size = min_t(int, (int)size - i, fifo_max_sz);

P.S.: Also, I'd consider changing this to a do-while instead...

Cheers,
Angelo

> +	for (i = 0; i < size; ) {
> +		if (cmd == CMD_AUX_I2C_EDID_READ)
> +			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 +1091,7 @@ static ssize_t it6505_aux_do_transfer(struct it6505 *it6505,
>   			goto aux_op_err;
>   		}
>   
> +		i += request_size;
>   		ret += ret_size;
>   	}
>   

[PATCH v4 03/11] drm/bridge: it6505: add aux operation for HDCP ksv list read

[Hermes Wu]

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

Add aux operaction command which supports read DPCD KSV FIFO
with aux fifo.


Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 drivers/gpu/drm/bridge/ite-it6505.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index b451d3c2ac1d..0583abdca75f 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -324,6 +324,9 @@ 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,
 };
 
 enum aux_cmd_reply {
@@ -965,7 +968,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 */
@@ -1030,7 +1034,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 +1059,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,
@@ -1079,7 +1083,7 @@ static ssize_t it6505_aux_do_transfer(struct it6505 *it6505,
 
 	mutex_lock(&it6505->aux_lock);
 	for (i = 0; i < size; ) {
-		if (cmd == CMD_AUX_I2C_EDID_READ)
+		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);
-- 
2.34.1

Re: [PATCH v4 03/11] drm/bridge: it6505: add aux operation for HDCP ksv list read

[Dmitry Baryshkov]

On Thu, Sep 26, 2024 at 03:47:53PM GMT, Hermes Wu wrote:
> From: Hermes Wu <Hermes.wu@ite.com.tw>
> 
> Add aux operaction command which supports read DPCD KSV FIFO
> with aux fifo.

Nit: AUX, FIFO. Please be consistent in your commit messages.

> 
> 
> Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")

Why is this considered to be a fix? From the commit message it sounds
like a new feature.

LGTM otherwise

> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
> ---
>  drivers/gpu/drm/bridge/ite-it6505.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index b451d3c2ac1d..0583abdca75f 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -324,6 +324,9 @@ 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,
>  };
>  
>  enum aux_cmd_reply {
> @@ -965,7 +968,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 */
> @@ -1030,7 +1034,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 +1059,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,
> @@ -1079,7 +1083,7 @@ static ssize_t it6505_aux_do_transfer(struct it6505 *it6505,
>  
>  	mutex_lock(&it6505->aux_lock);
>  	for (i = 0; i < size; ) {
> -		if (cmd == CMD_AUX_I2C_EDID_READ)
> +		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);
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

RE: [PATCH v4 03/11] drm/bridge: it6505: add aux operation for HDCP ksv list read

[Hermes.Wu]

>On Thu, Sep 26, 2024 at 03:47:53PM GMT, Hermes Wu wrote:
>> From: Hermes Wu <Hermes.wu@ite.com.tw>
>> 
>> Add aux operaction command which supports read DPCD KSV FIFO with aux 
>> fifo.
>
>Nit: AUX, FIFO. Please be consistent in your commit messages.
>
>> 
>> 
>> Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
>
>Why is this considered to be a fix? From the commit message it sounds like a new feature.

It will be a necessary change for HDCP reads KSV FIFO.


>
>LGTM otherwise
>
>> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
>> ---
>>  drivers/gpu/drm/bridge/ite-it6505.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c 
>> b/drivers/gpu/drm/bridge/ite-it6505.c
>> index b451d3c2ac1d..0583abdca75f 100644
>> --- a/drivers/gpu/drm/bridge/ite-it6505.c
>> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
>> @@ -324,6 +324,9 @@ 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,
>>  };
>>  
>>  enum aux_cmd_reply {
>> @@ -965,7 +968,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 */ @@ -1030,7 +1034,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 +1059,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, @@ -1079,7 +1083,7 @@ static 
>> ssize_t it6505_aux_do_transfer(struct it6505 *it6505,
>>  
>>  	mutex_lock(&it6505->aux_lock);
>>  	for (i = 0; i < size; ) {
>> -		if (cmd == CMD_AUX_I2C_EDID_READ)
>> +		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);
>> --
>> 2.34.1
>> 
>
>-- 
>With best wishes
>Dmitry
>

BR,
Hermes

Re: [PATCH v4 03/11] drm/bridge: it6505: add aux operation for HDCP ksv list read

[Dmitry Baryshkov]

On Thu, 26 Sept 2024 at 11:46, <Hermes.Wu@ite.com.tw> wrote:
>
> >On Thu, Sep 26, 2024 at 03:47:53PM GMT, Hermes Wu wrote:
> >> From: Hermes Wu <Hermes.wu@ite.com.tw>
> >>
> >> Add aux operaction command which supports read DPCD KSV FIFO with aux
> >> fifo.
> >
> >Nit: AUX, FIFO. Please be consistent in your commit messages.
> >
> >>
> >>
> >> Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
> >
> >Why is this considered to be a fix? From the commit message it sounds like a new feature.
>
> It will be a necessary change for HDCP reads KSV FIFO.

First of all, it should be a part of the commit message, because the
patch itself doesn't fix an issue.
Judging by the amount and the intrusivity of the patches, I'd say that
all KSV / HDCP-related patches constitute a new development, rather
than a bugfix.

>
>
> >
> >LGTM otherwise
> >
> >> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
> >> ---
> >>  drivers/gpu/drm/bridge/ite-it6505.c | 12 ++++++++----
> >>  1 file changed, 8 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c
> >> b/drivers/gpu/drm/bridge/ite-it6505.c
> >> index b451d3c2ac1d..0583abdca75f 100644
> >> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> >> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> >> @@ -324,6 +324,9 @@ 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,
> >>  };
> >>
> >>  enum aux_cmd_reply {
> >> @@ -965,7 +968,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 */ @@ -1030,7 +1034,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 +1059,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, @@ -1079,7 +1083,7 @@ static
> >> ssize_t it6505_aux_do_transfer(struct it6505 *it6505,
> >>
> >>      mutex_lock(&it6505->aux_lock);
> >>      for (i = 0; i < size; ) {
> >> -            if (cmd == CMD_AUX_I2C_EDID_READ)
> >> +            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);
> >> --
> >> 2.34.1
> >>
> >
> >--
> >With best wishes
> >Dmitry
> >
>
> BR,
> Hermes
>


-- 
With best wishes
Dmitry

[PATCH v4 04/11] drm/bridge: it6505: fix aux command write to aux operaction register

[Hermes Wu]

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

The aux control register command is 4 bits LSB only, adding a MACRO to get
correct operaction command.

Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
---
 drivers/gpu/drm/bridge/ite-it6505.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index 0583abdca75f..d1f5220e04a6 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -329,6 +329,8 @@ enum aux_cmd_type {
 	CMD_AUX_GET_KSV_LIST = 0x10,
 };
 
+#define GET_AUX_CONTROL_CODE(cmd) ((cmd) & 0x0F)
+
 enum aux_cmd_reply {
 	REPLY_ACK,
 	REPLY_NACK,
@@ -1000,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)
-- 
2.34.1

Re: [PATCH v4 04/11] drm/bridge: it6505: fix aux command write to aux operaction register

[Dmitry Baryshkov]

On Thu, Sep 26, 2024 at 03:47:54PM GMT, Hermes Wu wrote:
> From: Hermes Wu <Hermes.wu@ite.com.tw>
> 
> The aux control register command is 4 bits LSB only, adding a MACRO to get
> correct operaction command.

Nit: AUX, add (not adding), macro.

What happens if the driver doesn't limit the field? Let me guess, the
KSV reading command is 0x10 (it should have been a part of the commit
message, BTW), so it overrides some other bits? In such a case this
either should be a part of the previous commit, or, at least, come
before it.

> Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
> ---
>  drivers/gpu/drm/bridge/ite-it6505.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index 0583abdca75f..d1f5220e04a6 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -329,6 +329,8 @@ enum aux_cmd_type {
>  	CMD_AUX_GET_KSV_LIST = 0x10,
>  };
>  
> +#define GET_AUX_CONTROL_CODE(cmd) ((cmd) & 0x0F)
> +
>  enum aux_cmd_reply {
>  	REPLY_ACK,
>  	REPLY_NACK,
> @@ -1000,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)
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

RE: [PATCH v4 04/11] drm/bridge: it6505: fix aux command write to aux operaction register

[Hermes.Wu]

>On Thu, Sep 26, 2024 at 03:47:54PM GMT, Hermes Wu wrote:
>> From: Hermes Wu <Hermes.wu@ite.com.tw>
>> 
>> The aux control register command is 4 bits LSB only, adding a MACRO to 
>> get correct operaction command.
>
>Nit: AUX, add (not adding), macro.
>
>What happens if the driver doesn't limit the field? Let me guess, the KSV reading command is 0x10 (it should have been a part of the commit message, BTW), so it overrides some other bits? In such a case this either should be a part of the previous commit, or, at least, come before it.
>

Nothing Happens.

The AUX control command at control register REG_AUX_CMD_REQ is 4 bits LSB only, and b[7:4] is ready only.
AUX FIFO access cannot reach all DPCD area, only KSV FIFO at DPCD(0x6802C).
The commend define use [7:4] to extend original AUX_NATIVE_READ(0) as AUX_NATIVE_READ with AUX FIFO

It should be a part of previous commit.

>> Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
>> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
>> ---
>>  drivers/gpu/drm/bridge/ite-it6505.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c 
>> b/drivers/gpu/drm/bridge/ite-it6505.c
>> index 0583abdca75f..d1f5220e04a6 100644
>> --- a/drivers/gpu/drm/bridge/ite-it6505.c
>> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
>> @@ -329,6 +329,8 @@ enum aux_cmd_type {
>>  	CMD_AUX_GET_KSV_LIST = 0x10,
>>  };
>>  
>> +#define GET_AUX_CONTROL_CODE(cmd) ((cmd) & 0x0F)
>> +
>>  enum aux_cmd_reply {
>>  	REPLY_ACK,
>>  	REPLY_NACK,
>> @@ -1000,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)
>> --
>> 2.34.1
>> 
>
>-- 
>With best wishes
>Dmitry
>

BR,
Hermes

Re: [PATCH v4 04/11] drm/bridge: it6505: fix aux command write to aux operaction register

[AngeloGioacchino Del Regno]

Il 26/09/24 09:47, Hermes Wu ha scritto:
> From: Hermes Wu <Hermes.wu@ite.com.tw>
> 
> The aux control register command is 4 bits LSB only, adding a MACRO to get
> correct operaction command.
> 
> Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
> ---
>   drivers/gpu/drm/bridge/ite-it6505.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index 0583abdca75f..d1f5220e04a6 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -329,6 +329,8 @@ enum aux_cmd_type {
>   	CMD_AUX_GET_KSV_LIST = 0x10,
>   };
>   
> +#define GET_AUX_CONTROL_CODE(cmd) ((cmd) & 0x0F)

Just (cmd & 0xf) as the leading zero is meaningless.

As a out-of-scope consideration, though, this driver would really benefit from
a conversion to use bitfield macros... would be a nice cleanup.

Cheers,
Angelo

> +
>   enum aux_cmd_reply {
>   	REPLY_ACK,
>   	REPLY_NACK,
> @@ -1000,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)