diff mbox series

[1/1] i3c: Add fallback method for GETMXDS CCC

Message ID 20230911082456.23239-2-joshua.yeong@starfivetech.com (mailing list archive)
State Superseded
Headers show
Series Fallback mechanism for GETMXDS CCC | expand

Commit Message

Joshua Yeong Sept. 11, 2023, 8:24 a.m. UTC 
Some I3C hardware will report error when incorrect length is received from
device. GETMXDS CCC are availble in 2 formats; without turnaround time (format
1) and with turnaround time (format 2). There is no mechanics to determine which
format is supported by device. In case sending GETMXDS CCC format 2 resulted
failure, try sending GETMXDS CCC format 1 instead.

Signed-off-by: Joshua Yeong <joshua.yeong@starfivetech.com>
---
 drivers/i3c/master.c    | 33 ++++++++++++++++++++++++++++-----
 include/linux/i3c/ccc.h | 17 +++++++++++++++--
 2 files changed, 43 insertions(+), 7 deletions(-)

Comments

Miquel Raynal Sept. 12, 2023, 11:08 a.m. UTC | #1 
Hi Joshua,

joshua.yeong@starfivetech.com wrote on Mon, 11 Sep 2023 16:24:56 +0800:

> Some I3C hardware will report error when incorrect length is received from
> device. GETMXDS CCC are availble in 2 formats; without turnaround time (format
> 1) and with turnaround time (format 2). There is no mechanics to determine which
> format is supported by device. In case sending GETMXDS CCC format 2 resulted
> failure, try sending GETMXDS CCC format 1 instead.
> 
> Signed-off-by: Joshua Yeong <joshua.yeong@starfivetech.com>
> ---
>  drivers/i3c/master.c    | 33 ++++++++++++++++++++++++++++-----
>  include/linux/i3c/ccc.h | 17 +++++++++++++++--
>  2 files changed, 43 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 87283e4a4607..084f64bef155 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1088,10 +1088,37 @@ static int i3c_master_getmxds_locked(struct i3c_master_controller *master,
>  				     struct i3c_device_info *info)
>  {
>  	struct i3c_ccc_getmxds *getmaxds;
> +	struct i3c_ccc_getmxds_turnaround *getmaxds_ta;
>  	struct i3c_ccc_cmd_dest dest;
>  	struct i3c_ccc_cmd cmd;
>  	int ret;
>  
> +	getmaxds_ta = i3c_ccc_cmd_dest_init(&dest, info->dyn_addr,
> +					 sizeof(*getmaxds_ta));
> +	if (!getmaxds_ta)
> +		return -ENOMEM;
> +
> +	i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETMXDS, &dest, 1);
> +	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
> +	if (ret) {
> +		goto alternative;
> +	}
> +
> +	if (dest.payload.len != 2 && dest.payload.len != 5) {

Can len be 2 here?

> +		ret = -EIO;
> +		goto out;
> +	}
> +
> +	info->max_read_ds = getmaxds_ta->maxrd;
> +	info->max_write_ds = getmaxds_ta->maxwr;
> +	if (dest.payload.len == 5)
> +		info->max_read_turnaround = getmaxds_ta->maxrdturn[0] |
> +					    ((u32)getmaxds_ta->maxrdturn[1] << 8) |
> +					    ((u32)getmaxds_ta->maxrdturn[2] << 16);

Don't you want to avoid the "alternative" if it worked?

> +
> +alternative:

I would expect a comment somewhere to explain the subtlety.

> +	i3c_ccc_cmd_dest_cleanup(&dest);
> +
>  	getmaxds = i3c_ccc_cmd_dest_init(&dest, info->dyn_addr,
>  					 sizeof(*getmaxds));
>  	if (!getmaxds)
> @@ -1102,17 +1129,13 @@ static int i3c_master_getmxds_locked(struct i3c_master_controller *master,
>  	if (ret)
>  		goto out;
>  
> -	if (dest.payload.len != 2 && dest.payload.len != 5) {
> +	if (dest.payload.len != 2) {
>  		ret = -EIO;
>  		goto out;
>  	}
>  
>  	info->max_read_ds = getmaxds->maxrd;
>  	info->max_write_ds = getmaxds->maxwr;
> -	if (dest.payload.len == 5)
> -		info->max_read_turnaround = getmaxds->maxrdturn[0] |
> -					    ((u32)getmaxds->maxrdturn[1] << 8) |
> -					    ((u32)getmaxds->maxrdturn[2] << 16);
>  
>  out:
>  	i3c_ccc_cmd_dest_cleanup(&dest);
> diff --git a/include/linux/i3c/ccc.h b/include/linux/i3c/ccc.h
> index ad59a4ae60d1..50ed41d4d5a1 100644
> --- a/include/linux/i3c/ccc.h
> +++ b/include/linux/i3c/ccc.h
> @@ -269,14 +269,27 @@ enum i3c_tsco {
>  #define I3C_CCC_MAX_SDR_FSCL(x)		((x) & I3C_CCC_MAX_SDR_FSCL_MASK)
>  
>  /**
> - * struct i3c_ccc_getmxds - payload passed to GETMXDS CCC
> + * struct i3c_ccc_getmxds - payload passed to GETMXDS CCC without turnaround
> + * (format 1)
> + *
> + * @maxwr: write limitations
> + * @maxrd: read limitations
> + */
> +struct i3c_ccc_getmxds {
> +	u8 maxwr;
> +	u8 maxrd;
> +} __packed;
> +
> +/**
> + * struct i3c_ccc_getmxds_ta - payload passed to GETMXDS CCC with turnaround
> + * (format 2)
>   *
>   * @maxwr: write limitations
>   * @maxrd: read limitations
>   * @maxrdturn: maximum read turn-around expressed micro-seconds and
>   *	       little-endian formatted
>   */
> -struct i3c_ccc_getmxds {
> +struct i3c_ccc_getmxds_turnaround {
>  	u8 maxwr;
>  	u8 maxrd;
>  	u8 maxrdturn[3];


Thanks,
Miquèl
Joshua Yeong Sept. 13, 2023, 5:58 a.m. UTC | #2 
Hi Miquel,


On 12-Sep-23 7:08 PM, Miquel Raynal wrote:
> Hi Joshua,
>
> joshua.yeong@starfivetech.com wrote on Mon, 11 Sep 2023 16:24:56 +0800:
>
>> Some I3C hardware will report error when incorrect length is received from
>> device. GETMXDS CCC are availble in 2 formats; without turnaround time (format
>> 1) and with turnaround time (format 2). There is no mechanics to determine which
>> format is supported by device. In case sending GETMXDS CCC format 2 resulted
>> failure, try sending GETMXDS CCC format 1 instead.
>>
>> Signed-off-by: Joshua Yeong <joshua.yeong@starfivetech.com>
>> ---
>>   drivers/i3c/master.c    | 33 ++++++++++++++++++++++++++++-----
>>   include/linux/i3c/ccc.h | 17 +++++++++++++++--
>>   2 files changed, 43 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
>> index 87283e4a4607..084f64bef155 100644
>> --- a/drivers/i3c/master.c
>> +++ b/drivers/i3c/master.c
>> @@ -1088,10 +1088,37 @@ static int i3c_master_getmxds_locked(struct i3c_master_controller *master,
>>   				     struct i3c_device_info *info)
>>   {
>>   	struct i3c_ccc_getmxds *getmaxds;
>> +	struct i3c_ccc_getmxds_turnaround *getmaxds_ta;
>>   	struct i3c_ccc_cmd_dest dest;
>>   	struct i3c_ccc_cmd cmd;
>>   	int ret;
>>   
>> +	getmaxds_ta = i3c_ccc_cmd_dest_init(&dest, info->dyn_addr,
>> +					 sizeof(*getmaxds_ta));
>> +	if (!getmaxds_ta)
>> +		return -ENOMEM;
>> +
>> +	i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETMXDS, &dest, 1);
>> +	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
>> +	if (ret) {
>> +		goto alternative;
>> +	}
>> +
>> +	if (dest.payload.len != 2 && dest.payload.len != 5) {
> Can len be 2 here?

I do not know if other IP allows lower payload length. The Cadence IP 
will trigger an error in case the GET CCC length is mismatch. May need 
Vitor/Conor to confirm this behavior in Synopsys/Silvaco.

>> +		ret = -EIO;
>> +		goto out;
>> +	}
>> +
>> +	info->max_read_ds = getmaxds_ta->maxrd;
>> +	info->max_write_ds = getmaxds_ta->maxwr;
>> +	if (dest.payload.len == 5)
>> +		info->max_read_turnaround = getmaxds_ta->maxrdturn[0] |
>> +					    ((u32)getmaxds_ta->maxrdturn[1] << 8) |
>> +					    ((u32)getmaxds_ta->maxrdturn[2] << 16);
> Don't you want to avoid the "alternative" if it worked?
Thank you, will fix this.
>
>> +
>> +alternative:
> I would expect a comment somewhere to explain the subtlety.
Will add comments to explain this code.
>
>> +	i3c_ccc_cmd_dest_cleanup(&dest);
>> +
>>   	getmaxds = i3c_ccc_cmd_dest_init(&dest, info->dyn_addr,
>>   					 sizeof(*getmaxds));
>>   	if (!getmaxds)
>> @@ -1102,17 +1129,13 @@ static int i3c_master_getmxds_locked(struct i3c_master_controller *master,
>>   	if (ret)
>>   		goto out;
>>   
>> -	if (dest.payload.len != 2 && dest.payload.len != 5) {
>> +	if (dest.payload.len != 2) {
>>   		ret = -EIO;
>>   		goto out;
>>   	}
>>   
>>   	info->max_read_ds = getmaxds->maxrd;
>>   	info->max_write_ds = getmaxds->maxwr;
>> -	if (dest.payload.len == 5)
>> -		info->max_read_turnaround = getmaxds->maxrdturn[0] |
>> -					    ((u32)getmaxds->maxrdturn[1] << 8) |
>> -					    ((u32)getmaxds->maxrdturn[2] << 16);
>>   
>>   out:
>>   	i3c_ccc_cmd_dest_cleanup(&dest);
>> diff --git a/include/linux/i3c/ccc.h b/include/linux/i3c/ccc.h
>> index ad59a4ae60d1..50ed41d4d5a1 100644
>> --- a/include/linux/i3c/ccc.h
>> +++ b/include/linux/i3c/ccc.h
>> @@ -269,14 +269,27 @@ enum i3c_tsco {
>>   #define I3C_CCC_MAX_SDR_FSCL(x)		((x) & I3C_CCC_MAX_SDR_FSCL_MASK)
>>   
>>   /**
>> - * struct i3c_ccc_getmxds - payload passed to GETMXDS CCC
>> + * struct i3c_ccc_getmxds - payload passed to GETMXDS CCC without turnaround
>> + * (format 1)
>> + *
>> + * @maxwr: write limitations
>> + * @maxrd: read limitations
>> + */
>> +struct i3c_ccc_getmxds {
>> +	u8 maxwr;
>> +	u8 maxrd;
>> +} __packed;
>> +
>> +/**
>> + * struct i3c_ccc_getmxds_ta - payload passed to GETMXDS CCC with turnaround
>> + * (format 2)
>>    *
>>    * @maxwr: write limitations
>>    * @maxrd: read limitations
>>    * @maxrdturn: maximum read turn-around expressed micro-seconds and
>>    *	       little-endian formatted
>>    */
>> -struct i3c_ccc_getmxds {
>> +struct i3c_ccc_getmxds_turnaround {
>>   	u8 maxwr;
>>   	u8 maxrd;
>>   	u8 maxrdturn[3];
>
> Thanks,
> Miquèl
diff mbox series

Patch

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 87283e4a4607..084f64bef155 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -1088,10 +1088,37 @@  static int i3c_master_getmxds_locked(struct i3c_master_controller *master,
 				     struct i3c_device_info *info)
 {
 	struct i3c_ccc_getmxds *getmaxds;
+	struct i3c_ccc_getmxds_turnaround *getmaxds_ta;
 	struct i3c_ccc_cmd_dest dest;
 	struct i3c_ccc_cmd cmd;
 	int ret;
 
+	getmaxds_ta = i3c_ccc_cmd_dest_init(&dest, info->dyn_addr,
+					 sizeof(*getmaxds_ta));
+	if (!getmaxds_ta)
+		return -ENOMEM;
+
+	i3c_ccc_cmd_init(&cmd, true, I3C_CCC_GETMXDS, &dest, 1);
+	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
+	if (ret) {
+		goto alternative;
+	}
+
+	if (dest.payload.len != 2 && dest.payload.len != 5) {
+		ret = -EIO;
+		goto out;
+	}
+
+	info->max_read_ds = getmaxds_ta->maxrd;
+	info->max_write_ds = getmaxds_ta->maxwr;
+	if (dest.payload.len == 5)
+		info->max_read_turnaround = getmaxds_ta->maxrdturn[0] |
+					    ((u32)getmaxds_ta->maxrdturn[1] << 8) |
+					    ((u32)getmaxds_ta->maxrdturn[2] << 16);
+
+alternative:
+	i3c_ccc_cmd_dest_cleanup(&dest);
+
 	getmaxds = i3c_ccc_cmd_dest_init(&dest, info->dyn_addr,
 					 sizeof(*getmaxds));
 	if (!getmaxds)
@@ -1102,17 +1129,13 @@  static int i3c_master_getmxds_locked(struct i3c_master_controller *master,
 	if (ret)
 		goto out;
 
-	if (dest.payload.len != 2 && dest.payload.len != 5) {
+	if (dest.payload.len != 2) {
 		ret = -EIO;
 		goto out;
 	}
 
 	info->max_read_ds = getmaxds->maxrd;
 	info->max_write_ds = getmaxds->maxwr;
-	if (dest.payload.len == 5)
-		info->max_read_turnaround = getmaxds->maxrdturn[0] |
-					    ((u32)getmaxds->maxrdturn[1] << 8) |
-					    ((u32)getmaxds->maxrdturn[2] << 16);
 
 out:
 	i3c_ccc_cmd_dest_cleanup(&dest);
diff --git a/include/linux/i3c/ccc.h b/include/linux/i3c/ccc.h
index ad59a4ae60d1..50ed41d4d5a1 100644
--- a/include/linux/i3c/ccc.h
+++ b/include/linux/i3c/ccc.h
@@ -269,14 +269,27 @@  enum i3c_tsco {
 #define I3C_CCC_MAX_SDR_FSCL(x)		((x) & I3C_CCC_MAX_SDR_FSCL_MASK)
 
 /**
- * struct i3c_ccc_getmxds - payload passed to GETMXDS CCC
+ * struct i3c_ccc_getmxds - payload passed to GETMXDS CCC without turnaround
+ * (format 1)
+ *
+ * @maxwr: write limitations
+ * @maxrd: read limitations
+ */
+struct i3c_ccc_getmxds {
+	u8 maxwr;
+	u8 maxrd;
+} __packed;
+
+/**
+ * struct i3c_ccc_getmxds_ta - payload passed to GETMXDS CCC with turnaround
+ * (format 2)
  *
  * @maxwr: write limitations
  * @maxrd: read limitations
  * @maxrdturn: maximum read turn-around expressed micro-seconds and
  *	       little-endian formatted
  */
-struct i3c_ccc_getmxds {
+struct i3c_ccc_getmxds_turnaround {
 	u8 maxwr;
 	u8 maxrd;
 	u8 maxrdturn[3];