Hi Joshua,
[email protected] 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 <[email protected]>
> ---
> 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
Hi Miquel,
On 12-Sep-23 7:08 PM, Miquel Raynal wrote:
> Hi Joshua,
>
> [email protected] 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 <[email protected]>
>> ---
>> 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