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

[Boris Brezillon <bbrezillon@kernel.org>]

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.
> 
> > +
> > +	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>

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);
	...
};

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