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

[Joshua Yeong]

Hi Miquel,


On 12-Sep-23 7:08 PM, Miquel Raynal wrote:
> Hi Joshua,
>
> joshua.yeong@xxxxxxxxxxxxxxxx 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@xxxxxxxxxxxxxxxx>
> > ---
> >   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