Re: [PATCH 1/2] i3c: Add support for HDR modes.

[Przemyslaw Gaj <pgaj@cadence.com>]

Sorry for the delay. It was on my todo list.

The 02/22/2019 15:52, Boris Brezillon wrote:
> EXTERNAL MAIL
> 
> 
> On Thu, 21 Feb 2019 15:15:57 +0000
> vitor <vitor.soares@synopsys.com> wrote:
> 
> > Hi Przemek,
> > 
> > Sorry for the late response.
> > 
> > On 13/12/18 12:18, Przemyslaw Gaj wrote:
> > > HDR (High Data Rate) modes is an important feature of the I3C protocol
> > > as it allows to get higher throughput than with the SDR (Single Data
> > > Rate) mode.
> > >
> > > Add new controller hooks and extend the I3C device API to expose this
> > > new feature.
> > >
> > > This feature was originally created by Boris Brezillon
> > > <boris.brezillon@bootlin.com>.
> > >
> > > Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
> > > ---
> > >  drivers/i3c/device.c       | 37 +++++++++++++++++++++++++++++++++++++
> > >  drivers/i3c/internals.h    |  3 +++
> > >  drivers/i3c/master.c       | 31 +++++++++++++++++++++++++++++++
> > >  include/linux/i3c/device.h | 29 +++++++++++++++++++++++++++++
> > >  include/linux/i3c/master.h |  7 +++++++
> > >  5 files changed, 107 insertions(+)
> > >
> > > diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c
> > > index 69cc040..97910aa 100644
> > > --- a/drivers/i3c/device.c
> > > +++ b/drivers/i3c/device.c
> > > @@ -51,6 +51,43 @@ int i3c_device_do_priv_xfers(struct i3c_device *dev,
> > >  EXPORT_SYMBOL_GPL(i3c_device_do_priv_xfers);
> > >  
> > >  /**
> > > + * i3c_device_send_hdr_cmds() - send HDR commands to a specific device
> > > + *
> > > + * @dev: device to which these commands should be sent
> > > + * @xfers: array of commands
> > > + * @nxfers: number of commands
> > > + *
> > > + * Send one or several HDR commands to @dev.
> > > + *
> > > + * This function can sleep and thus cannot be called in atomic context.
> > > + *
> > > + * Return: 0 in case of success, a negative error core otherwise.
> > > + */
> > > +int i3c_device_send_hdr_cmds(struct i3c_device *dev,
> > > +			     struct i3c_hdr_cmd *cmds,
> > > +			     int ncmds)
> > > +{
> > > +	int ret, i;
> > > +	enum i3c_hdr_mode mode;
> > > +
> > > +	if (ncmds < 1)
> > > +		return 0;
> > > +
> > > +	mode = cmds[0].mode;
> > > +	for (i = 1; i < ncmds; i++) {
> > > +		if (mode != cmds[i].mode)
> > > +			return -EINVAL;
> > > +	}
> > > +
> > > +	i3c_bus_normaluse_lock(dev->bus);
> > > +	ret = i3c_dev_send_hdr_cmds_locked(dev->desc, cmds, ncmds);
> > > +	i3c_bus_normaluse_unlock(dev->bus);
> > > +
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(i3c_device_send_hdr_cmds);
> > > +
> > > +/**
> > >   * i3c_device_get_info() - get I3C device information
> > >   *
> > >   * @dev: device we want information on
> > > diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
> > > index 86b7b44..46c4de7 100644
> > > --- a/drivers/i3c/internals.h
> > > +++ b/drivers/i3c/internals.h
> > > @@ -18,6 +18,9 @@ void i3c_bus_normaluse_unlock(struct i3c_bus *bus);
> > >  int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
> > >  				 struct i3c_priv_xfer *xfers,
> > >  				 int nxfers);
> > > +int i3c_dev_send_hdr_cmds_locked(struct i3c_dev_desc *dev,
> > > +				 struct i3c_hdr_cmd *cmds,
> > > +				 int ncmds);
> > >  int i3c_dev_disable_ibi_locked(struct i3c_dev_desc *dev);
> > >  int i3c_dev_enable_ibi_locked(struct i3c_dev_desc *dev);
> > >  int i3c_dev_request_ibi_locked(struct i3c_dev_desc *dev,
> > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > > index e98b600..16d6dd5 100644
> > > --- a/drivers/i3c/master.c
> > > +++ b/drivers/i3c/master.c
> > > @@ -2791,6 +2791,37 @@ int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
> > >  	return master->ops->priv_xfers(dev, xfers, nxfers);
> > >  }
> > >  
> > > +int i3c_dev_send_hdr_cmds_locked(struct i3c_dev_desc *dev,
> > > +				 struct i3c_hdr_cmd *cmds,
> > > +				 int ncmds)
> > > +{
> > > +	struct i3c_master_controller *master;
> > > +	int i;
> > > +
> > > +	if (!dev)
> > > +		return -ENOENT;
> > > +
> > > +	master = i3c_dev_get_master(dev);
> > > +	if (!master || !cmds)
> > > +		return -EINVAL;
> > > +
> > > +	if (master->op_mode == I3C_SLAVE_MODE) {
> > > +		if (i3c_master_request_mastership(master))
> > > +			return -EIO;
> > > +	}  
> > 
> > This patch seems to be applied on top of secondary master patch proposal.
> > 
> > I think it is better to remove the secondary master stuffs from here.
> > 

Yes, I'll do that.

> > > +
> > > +	if (!master->ops->send_hdr_cmds)
> > > +		return -ENOTSUPP;
> > > +
> > > +	for (i = 0; i < ncmds; i++) {
> > > +		if (!(master->this->info.hdr_cap & BIT(cmds->mode)))
> > > +			return -ENOTSUPP;
> > > +	}
> > > +
> > > +	return master->ops->send_hdr_cmds(dev, cmds, ncmds);
> > > +}
> > > +
> > > +
> > >  int i3c_dev_disable_ibi_locked(struct i3c_dev_desc *dev)
> > >  {
> > >  	struct i3c_master_controller *master;
> > > diff --git a/include/linux/i3c/device.h b/include/linux/i3c/device.h
> > > index 5ecb055..75a947f 100644
> > > --- a/include/linux/i3c/device.h
> > > +++ b/include/linux/i3c/device.h
> > > @@ -49,6 +49,31 @@ enum i3c_hdr_mode {
> > >  	I3C_HDR_TSL,
> > >  };
> > >  
> > > +#define I3C_HDR_GEN_WRITE_CMD(id)    	(id)
> > > +#define I3C_HDR_VENDOR_WRITE_CMD(id) 	(0x20 + (id))
> > > +#define I3C_HDR_IS_READ_CMD        	BIT(7)
> > > +#define I3C_HDR_GEN_READ_CMD(id)    	(0x80 + (id))
> > > +#define I3C_HDR_VENDOR_READ_CMD(id)    	(0xa0 + (id))
> > > +
> > > +/**
> > > + * struct i3c_hdr_cmd - I3C HDR command
> > > + * @mode: HDR mode selected for this command
> > > + * @code: command opcode. Bit 7 encodes the direction of the data transfer, if
> > > + *      set this is a read, otherwise this is a write
> > > + * @ndatawords: number of data words (a word is 16bits wide) to transfer
> > > + * @data: input/output buffer
> > > + */
> > > +struct i3c_hdr_cmd {
> > > +    enum i3c_hdr_mode mode;
> > > +    u8 code;
> > > +    int ndatawords;
> > > +    union {
> > > +        u16 *in;
> > > +        const u16 *out;
> > > +    } data;
> > > +};
> > > +
> > > +
> > >  /**
> > >   * struct i3c_priv_xfer - I3C SDR private transfer
> > >   * @rnw: encodes the transfer direction. true for a read, false for a write
> > > @@ -289,6 +314,10 @@ int i3c_device_do_priv_xfers(struct i3c_device *dev,
> > >  			     struct i3c_priv_xfer *xfers,
> > >  			     int nxfers);
> > >  
> > > +int i3c_device_send_hdr_cmds(struct i3c_device *dev,
> > > +			     struct i3c_hdr_cmd *cmds,
> > > +			     int ncmds);
> > > +
> > >  void i3c_device_get_info(struct i3c_device *dev, struct i3c_device_info *info);
> > >  
> > >  struct i3c_ibi_payload {
> > > diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> > > index ada956a..fd50473 100644
> > > --- a/include/linux/i3c/master.h
> > > +++ b/include/linux/i3c/master.h
> > > @@ -386,6 +386,10 @@ struct i3c_bus {
> > >   *		  This method is mandatory.
> > >   * @priv_xfers: do one or several private I3C SDR transfers
> > >   *		This method is mandatory.
> > > + * @send_hdr_cmds: send one or several HDR commands. If there is more than one
> > > + *		   command, they should ideally be sent in the same HDR
> > > + *		   transaction.
> > > + *		   This method is optional.
> > >   * @attach_i2c_dev: called every time an I2C device is attached to the bus.
> > >   *		    This is a good place to attach master controller specific
> > >   *		    data to I2C devices.
> > > @@ -457,6 +461,9 @@ struct i3c_master_controller_ops {
> > >  	int (*priv_xfers)(struct i3c_dev_desc *dev,
> > >  			  struct i3c_priv_xfer *xfers,
> > >  			  int nxfers);
> > > +	int (*send_hdr_cmds)(struct i3c_dev_desc *dev,
> > > +			     const struct i3c_hdr_cmd *cmds,
> > > +			     int ncmds);
> > >  	int (*attach_i2c_dev)(struct i2c_dev_desc *dev);
> > >  	void (*detach_i2c_dev)(struct i2c_dev_desc *dev);
> > >  	int (*i2c_xfers)(struct i2c_dev_desc *dev,  
> > 
> > With this approach the controller between a start and stop can only transmit in SDR or HDR.
> > 
> > This is limited for devices that need the following frame:
> >     <Start><SDR xfer><Repeated Start><HDR command><Stop>

Actually, there is repeated start between SDR and ENTHDR. There is no repeated
start between ENTHDR and HDR commands, HDR command has to start immedetly after
ENTHDR. Of course, hdr restart may occur between HDR commands.

> 
> If this is a use case we want to support, then we should probably have
> something more generic than what we currently have.
> 
> Something like
> 
> enum i3c_xfer_type {
> 	I3C_CCC_XFER,
> 	I3C_SDR_XFER,
> 	I3C_HDR_XFER,
> }
> 
> struct i3c_xfer {
> 	enum i3c_xfer_type type;
> 	union {
> 		struct i3c_ccc_cmd ccc;
> 		struct i3c_priv_xfer sdr;
> 		struct i3c_hdr_cmd hdr;
> 	};
> }
> 
> struct i3c_master_controller_ops {
> 	...
> 	int (*i3c_xfers)(struct i3c_dev_desc *dev,
> 			 struct i3c_xfer *xfers,
> 			 int nxfers);
> 	...
> };

I agree. This is something we should think of.

-- 
-- 
Przemyslaw Gaj

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c