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

[vitor <vitor.soares@synopsys.com>]

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>


Best regards,
Vitor Soares


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