[PATCH 0/2] Add the I3C HDR modes

[PATCH 0/2] Add the I3C HDR modes

[Przemyslaw Gaj]

This patch series adds support for HDR modes to the I3C
subsystem and Cadence I3C master driver.

Przemyslaw Gaj (2):
  i3c: Add support for HDR modes.
  i3c: master: cdns: Add support for HDR-DDR mode

 drivers/i3c/device.c                 |  37 +++++++
 drivers/i3c/internals.h              |   3 +
 drivers/i3c/master.c                 |  31 ++++++
 drivers/i3c/master/i3c-master-cdns.c | 195 ++++++++++++++++++++++++++++++++++-
 include/linux/i3c/device.h           |  29 ++++++
 include/linux/i3c/master.h           |   7 ++
 6 files changed, 300 insertions(+), 2 deletions(-)

-- 
2.4.5


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

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

[Przemyslaw Gaj]

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;
+	}
+
+	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,
-- 
2.4.5


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

[PATCH 2/2] i3c: master: cdns: Add support for HDR-DDR mode

[Przemyslaw Gaj]

Cadence I3C master controller HDR-DDR mode support.

This feature was originally created by Boris Brezillon
<boris.brezillon@bootlin.com>. I made some changes/fixes.

Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
---
 drivers/i3c/master/i3c-master-cdns.c | 195 ++++++++++++++++++++++++++++++++++-
 1 file changed, 193 insertions(+), 2 deletions(-)

diff --git a/drivers/i3c/master/i3c-master-cdns.c b/drivers/i3c/master/i3c-master-cdns.c
index a33f3a6..b1a97be 100644
--- a/drivers/i3c/master/i3c-master-cdns.c
+++ b/drivers/i3c/master/i3c-master-cdns.c
@@ -571,7 +571,7 @@ static void cdns_i3c_master_end_xfer_locked(struct cdns_i3c_master *master,
 	     !(status0 & MST_STATUS0_CMDR_EMP);
 	     status0 = readl(master->regs + MST_STATUS0)) {
 		struct cdns_i3c_cmd *cmd;
-		u32 cmdr, rx_len, id;
+		u32 cmdr, rx_len, id, xfer_bytes;
 
 		cmdr = readl(master->regs + CMDR);
 		id = CMDR_CMDID(cmdr);
@@ -581,7 +581,11 @@ static void cdns_i3c_master_end_xfer_locked(struct cdns_i3c_master *master,
 			continue;
 
 		cmd = &xfer->cmds[CMDR_CMDID(cmdr)];
-		rx_len = min_t(u32, CMDR_XFER_BYTES(cmdr), cmd->rx_len);
+		xfer_bytes = CMDR_XFER_BYTES(cmdr);
+		if(cmd->cmd0 & CMD0_FIFO_IS_DDR)
+			xfer_bytes = xfer_bytes * 4;
+		rx_len = min_t(u32, xfer_bytes, cmd->rx_len);
+
 		cdns_i3c_master_rd_from_rx_fifo(master, cmd->rx_buf, rx_len);
 		cmd->error = CMDR_ERROR(cmdr);
 	}
@@ -893,6 +897,192 @@ static int cdns_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
 	return ret;
 }
 
+#define I3C_DDR_FIRST_DATA_WORD_PREAMBLE	0x2
+#define I3C_DDR_DATA_WORD_PREAMBLE		0x3
+
+#define I3C_DDR_PREAMBLE(p)			((p) << 18)
+
+static u32 prepare_ddr_word(u16 payload)
+{
+	u32 ret;
+	u16 pb;
+
+	ret = (u32)payload << 2;
+
+	/* Calculate parity. */
+	pb = (payload >> 15) ^ (payload >> 13) ^ (payload >> 11) ^
+	     (payload >> 9) ^ (payload >> 7) ^ (payload >> 5) ^
+	     (payload >> 3) ^ (payload >> 1);
+	ret |= (pb & 1) << 1;
+	pb = (payload >> 14) ^ (payload >> 12) ^ (payload >> 10) ^
+	     (payload >> 8) ^ (payload >> 6) ^ (payload >> 4) ^
+	     (payload >> 2) ^ payload ^ 1;
+	ret |= (pb & 1);
+
+	return ret;
+}
+
+static u32 prepare_ddr_data_word(u16 data, bool first)
+{
+	return prepare_ddr_word(data) |
+	       I3C_DDR_PREAMBLE(first ?
+			        I3C_DDR_FIRST_DATA_WORD_PREAMBLE :
+				I3C_DDR_DATA_WORD_PREAMBLE);
+}
+
+#define I3C_DDR_READ_CMD	BIT(15)
+
+static u32 prepare_ddr_cmd_word(u16 cmd)
+{
+	return prepare_ddr_word(cmd) | I3C_DDR_PREAMBLE(1);
+}
+
+static u32 prepare_ddr_crc_word(u8 crc5)
+{
+	return (((u32)crc5 & 0x1f) << 9) | (0xc << 14) |
+	       I3C_DDR_PREAMBLE(1);
+}
+
+static u32 prepare_ddr_parity_bit(u32 cmdword)
+{
+	u16 pb;
+
+	pb = (cmdword >> 14) ^ (cmdword >> 12) ^ (cmdword >> 10) ^
+	     (cmdword >> 8) ^ (cmdword >> 6) ^ (cmdword >> 4) ^
+	     (cmdword >> 2);
+
+	if (pb & 1)
+		cmdword |= BIT(0);
+
+	return cmdword;
+}
+
+static u8 update_crc5(u8 crc5, u16 word)
+{
+	u8 crc0;
+	int i;
+
+	/*
+	 * crc0 = next_data_bit ^ crc[4]
+	 *                1         2            3       4
+	 * crc[4:0] = { crc[3:2], crc[1]^crc0, crc[0], crc0 }
+	 */
+	for (i = 15; i >= 0; --i) {
+		crc0 = ((word >> i) ^ (crc5 >> 4)) & 0x1;
+		crc5 = ((crc5 << 1) & 0x1a) |
+		       (((crc5 >> 1) ^ crc0) << 2) |
+		       crc0;
+	}
+
+	return crc5 & 0x1f;
+}
+
+static int cdns_i3c_master_send_hdr_cmd(struct i3c_dev_desc *dev,
+					const struct i3c_hdr_cmd *cmds,
+					int ncmds)
+{
+	struct i3c_master_controller *m = i3c_dev_get_master(dev);
+	struct cdns_i3c_master *master = to_cdns_i3c_master(m);
+	int ret, i, ntxwords = 1, nrxwords = 0;
+	struct cdns_i3c_xfer *xfer;
+	struct cdns_i3c_cmd *ccmd;
+	u16 cmdword, datain;
+	u32 checkword, word;
+	u32 *buf = NULL;
+	u8 crc5;
+
+	if (ncmds < 1)
+		return 0;
+
+	if (ncmds > 1 || cmds[0].ndatawords > CMD0_FIFO_PL_LEN_MAX)
+		return -ENOTSUPP;
+
+	if (cmds[0].mode != I3C_HDR_DDR)
+		return -ENOTSUPP;
+
+	cmdword = ((u16)cmds[0].code << 8) | (dev->info.dyn_addr << 1);
+	if (cmdword & I3C_DDR_READ_CMD)
+		nrxwords += cmds[0].ndatawords + 1;
+	else
+		ntxwords += cmds[0].ndatawords + 1;
+
+	if (ntxwords > master->caps.txfifodepth ||
+	    nrxwords > master->caps.rxfifodepth)
+		return -ENOTSUPP;
+
+	buf = kzalloc((nrxwords + ntxwords) * sizeof(*buf), GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	xfer = cdns_i3c_master_alloc_xfer(master, 2);
+	if (!xfer) {
+		ret = -ENOMEM;
+		goto out_free_buf;
+	}
+
+	ccmd = &xfer->cmds[0];
+	ccmd->cmd1 = CMD1_FIFO_CCC(I3C_CCC_ENTHDR(0));
+	ccmd->cmd0 = CMD0_FIFO_IS_CCC;
+
+	ccmd = &xfer->cmds[1];
+
+	if (cmdword & I3C_DDR_READ_CMD)
+		cmdword = prepare_ddr_parity_bit(cmdword);
+
+	ccmd->tx_len = ntxwords * sizeof(u32);
+	ccmd->tx_buf = buf;
+	ccmd->rx_len = nrxwords * sizeof(u32);
+	ccmd->rx_buf = buf + ntxwords;
+
+	buf[0] = prepare_ddr_cmd_word(cmdword);
+	crc5 = update_crc5(0x1f, cmdword);
+	for (i = 0; i < ntxwords - 2; i++) {
+		crc5 = update_crc5(crc5, cmds[0].data.out[i]);
+		buf[i + 1] = prepare_ddr_data_word(cmds[0].data.out[i], !i);
+	}
+
+	if(!(cmdword & I3C_DDR_READ_CMD))
+		buf[ntxwords-1] = prepare_ddr_crc_word(crc5);
+
+	ccmd->cmd0 = CMD0_FIFO_IS_DDR | CMD0_FIFO_PL_LEN(ntxwords);
+
+	cdns_i3c_master_queue_xfer(master, xfer);
+	if (!wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000)))
+		cdns_i3c_master_unqueue_xfer(master, xfer);
+
+	ret = xfer->ret;
+
+	if (!xfer->ret && nrxwords) {
+		for (i = 0; i < nrxwords - 1; i++) {
+			word = (((u32 *)ccmd->rx_buf)[i] & GENMASK(19, 0));
+			datain = (word >> 2) & GENMASK(15, 0);
+			checkword = prepare_ddr_data_word(datain, !i);
+
+			if (checkword != word) {
+				ret = -EIO;
+				break;
+			}
+
+			crc5 = update_crc5(crc5, datain);
+			cmds[0].data.in[i] = datain;
+		}
+
+		word = (((u32 *)ccmd->rx_buf)[i] & GENMASK(19, 9));
+		datain = (word >> 2) & GENMASK(15, 0);
+		checkword = prepare_ddr_crc_word(crc5);
+
+		if (checkword != word)
+			ret = -EIO;
+	}
+
+	cdns_i3c_master_free_xfer(xfer);
+
+out_free_buf:
+	kfree(buf);
+
+	return ret;
+}
+
 static int cdns_i3c_master_i2c_xfers(struct i2c_dev_desc *dev,
 				     const struct i2c_msg *xfers, int nxfers)
 {
@@ -1714,6 +1904,7 @@ static const struct i3c_master_controller_ops cdns_i3c_master_ops = {
 	.supports_ccc_cmd = cdns_i3c_master_supports_ccc_cmd,
 	.send_ccc_cmd = cdns_i3c_master_send_ccc_cmd,
 	.priv_xfers = cdns_i3c_master_priv_xfers,
+	.send_hdr_cmds = cdns_i3c_master_send_hdr_cmd,
 	.i2c_xfers = cdns_i3c_master_i2c_xfers,
 	.i2c_funcs = cdns_i3c_master_i2c_funcs,
 	.enable_ibi = cdns_i3c_master_enable_ibi,
-- 
2.4.5


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

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

[Boris Brezillon]

On Thu, 13 Dec 2018 12:18:31 +0000
Przemyslaw Gaj <pgaj@cadence.com> 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>.

You should keep the orginal author and it's SoB and then add your own
SoB.

> 
> 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;
> +	}
> +
> +	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,


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

Re: [PATCH 2/2] i3c: master: cdns: Add support for HDR-DDR mode

[Boris Brezillon]

On Thu, 13 Dec 2018 12:18:32 +0000
Przemyslaw Gaj <pgaj@cadence.com> wrote:

> Cadence I3C master controller HDR-DDR mode support.
> 
> This feature was originally created by Boris Brezillon
> <boris.brezillon@bootlin.com>. I made some changes/fixes.

Same here.

> 
> Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
> ---
>  drivers/i3c/master/i3c-master-cdns.c | 195 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 193 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i3c/master/i3c-master-cdns.c b/drivers/i3c/master/i3c-master-cdns.c
> index a33f3a6..b1a97be 100644
> --- a/drivers/i3c/master/i3c-master-cdns.c
> +++ b/drivers/i3c/master/i3c-master-cdns.c
> @@ -571,7 +571,7 @@ static void cdns_i3c_master_end_xfer_locked(struct cdns_i3c_master *master,
>  	     !(status0 & MST_STATUS0_CMDR_EMP);
>  	     status0 = readl(master->regs + MST_STATUS0)) {
>  		struct cdns_i3c_cmd *cmd;
> -		u32 cmdr, rx_len, id;
> +		u32 cmdr, rx_len, id, xfer_bytes;
>  
>  		cmdr = readl(master->regs + CMDR);
>  		id = CMDR_CMDID(cmdr);
> @@ -581,7 +581,11 @@ static void cdns_i3c_master_end_xfer_locked(struct cdns_i3c_master *master,
>  			continue;
>  
>  		cmd = &xfer->cmds[CMDR_CMDID(cmdr)];
> -		rx_len = min_t(u32, CMDR_XFER_BYTES(cmdr), cmd->rx_len);
> +		xfer_bytes = CMDR_XFER_BYTES(cmdr);
> +		if(cmd->cmd0 & CMD0_FIFO_IS_DDR)
> +			xfer_bytes = xfer_bytes * 4;
> +		rx_len = min_t(u32, xfer_bytes, cmd->rx_len);
> +
>  		cdns_i3c_master_rd_from_rx_fifo(master, cmd->rx_buf, rx_len);
>  		cmd->error = CMDR_ERROR(cmdr);
>  	}
> @@ -893,6 +897,192 @@ static int cdns_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
>  	return ret;
>  }
>  
> +#define I3C_DDR_FIRST_DATA_WORD_PREAMBLE	0x2
> +#define I3C_DDR_DATA_WORD_PREAMBLE		0x3
> +
> +#define I3C_DDR_PREAMBLE(p)			((p) << 18)
> +
> +static u32 prepare_ddr_word(u16 payload)
> +{
> +	u32 ret;
> +	u16 pb;
> +
> +	ret = (u32)payload << 2;
> +
> +	/* Calculate parity. */
> +	pb = (payload >> 15) ^ (payload >> 13) ^ (payload >> 11) ^
> +	     (payload >> 9) ^ (payload >> 7) ^ (payload >> 5) ^
> +	     (payload >> 3) ^ (payload >> 1);
> +	ret |= (pb & 1) << 1;
> +	pb = (payload >> 14) ^ (payload >> 12) ^ (payload >> 10) ^
> +	     (payload >> 8) ^ (payload >> 6) ^ (payload >> 4) ^
> +	     (payload >> 2) ^ payload ^ 1;
> +	ret |= (pb & 1);
> +
> +	return ret;
> +}
> +
> +static u32 prepare_ddr_data_word(u16 data, bool first)
> +{
> +	return prepare_ddr_word(data) |
> +	       I3C_DDR_PREAMBLE(first ?
> +			        I3C_DDR_FIRST_DATA_WORD_PREAMBLE :
> +				I3C_DDR_DATA_WORD_PREAMBLE);
> +}
> +
> +#define I3C_DDR_READ_CMD	BIT(15)
> +
> +static u32 prepare_ddr_cmd_word(u16 cmd)
> +{
> +	return prepare_ddr_word(cmd) | I3C_DDR_PREAMBLE(1);
> +}
> +
> +static u32 prepare_ddr_crc_word(u8 crc5)
> +{
> +	return (((u32)crc5 & 0x1f) << 9) | (0xc << 14) |
> +	       I3C_DDR_PREAMBLE(1);
> +}
> +
> +static u32 prepare_ddr_parity_bit(u32 cmdword)
> +{
> +	u16 pb;
> +
> +	pb = (cmdword >> 14) ^ (cmdword >> 12) ^ (cmdword >> 10) ^
> +	     (cmdword >> 8) ^ (cmdword >> 6) ^ (cmdword >> 4) ^
> +	     (cmdword >> 2);
> +
> +	if (pb & 1)
> +		cmdword |= BIT(0);
> +
> +	return cmdword;
> +}
> +
> +static u8 update_crc5(u8 crc5, u16 word)
> +{
> +	u8 crc0;
> +	int i;
> +
> +	/*
> +	 * crc0 = next_data_bit ^ crc[4]
> +	 *                1         2            3       4
> +	 * crc[4:0] = { crc[3:2], crc[1]^crc0, crc[0], crc0 }
> +	 */
> +	for (i = 15; i >= 0; --i) {
> +		crc0 = ((word >> i) ^ (crc5 >> 4)) & 0x1;
> +		crc5 = ((crc5 << 1) & 0x1a) |
> +		       (((crc5 >> 1) ^ crc0) << 2) |
> +		       crc0;
> +	}
> +
> +	return crc5 & 0x1f;
> +}
> +
> +static int cdns_i3c_master_send_hdr_cmd(struct i3c_dev_desc *dev,
> +					const struct i3c_hdr_cmd *cmds,
> +					int ncmds)
> +{
> +	struct i3c_master_controller *m = i3c_dev_get_master(dev);
> +	struct cdns_i3c_master *master = to_cdns_i3c_master(m);
> +	int ret, i, ntxwords = 1, nrxwords = 0;
> +	struct cdns_i3c_xfer *xfer;
> +	struct cdns_i3c_cmd *ccmd;
> +	u16 cmdword, datain;
> +	u32 checkword, word;
> +	u32 *buf = NULL;
> +	u8 crc5;
> +
> +	if (ncmds < 1)
> +		return 0;
> +
> +	if (ncmds > 1 || cmds[0].ndatawords > CMD0_FIFO_PL_LEN_MAX)
> +		return -ENOTSUPP;
> +
> +	if (cmds[0].mode != I3C_HDR_DDR)
> +		return -ENOTSUPP;
> +
> +	cmdword = ((u16)cmds[0].code << 8) | (dev->info.dyn_addr << 1);
> +	if (cmdword & I3C_DDR_READ_CMD)
> +		nrxwords += cmds[0].ndatawords + 1;
> +	else
> +		ntxwords += cmds[0].ndatawords + 1;
> +
> +	if (ntxwords > master->caps.txfifodepth ||
> +	    nrxwords > master->caps.rxfifodepth)
> +		return -ENOTSUPP;
> +
> +	buf = kzalloc((nrxwords + ntxwords) * sizeof(*buf), GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	xfer = cdns_i3c_master_alloc_xfer(master, 2);
> +	if (!xfer) {
> +		ret = -ENOMEM;
> +		goto out_free_buf;
> +	}
> +
> +	ccmd = &xfer->cmds[0];
> +	ccmd->cmd1 = CMD1_FIFO_CCC(I3C_CCC_ENTHDR(0));
> +	ccmd->cmd0 = CMD0_FIFO_IS_CCC;
> +
> +	ccmd = &xfer->cmds[1];
> +
> +	if (cmdword & I3C_DDR_READ_CMD)
> +		cmdword = prepare_ddr_parity_bit(cmdword);
> +
> +	ccmd->tx_len = ntxwords * sizeof(u32);
> +	ccmd->tx_buf = buf;
> +	ccmd->rx_len = nrxwords * sizeof(u32);
> +	ccmd->rx_buf = buf + ntxwords;
> +
> +	buf[0] = prepare_ddr_cmd_word(cmdword);
> +	crc5 = update_crc5(0x1f, cmdword);
> +	for (i = 0; i < ntxwords - 2; i++) {
> +		crc5 = update_crc5(crc5, cmds[0].data.out[i]);
> +		buf[i + 1] = prepare_ddr_data_word(cmds[0].data.out[i], !i);
> +	}
> +
> +	if(!(cmdword & I3C_DDR_READ_CMD))
> +		buf[ntxwords-1] = prepare_ddr_crc_word(crc5);
> +
> +	ccmd->cmd0 = CMD0_FIFO_IS_DDR | CMD0_FIFO_PL_LEN(ntxwords);
> +
> +	cdns_i3c_master_queue_xfer(master, xfer);
> +	if (!wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000)))
> +		cdns_i3c_master_unqueue_xfer(master, xfer);
> +
> +	ret = xfer->ret;
> +
> +	if (!xfer->ret && nrxwords) {
> +		for (i = 0; i < nrxwords - 1; i++) {
> +			word = (((u32 *)ccmd->rx_buf)[i] & GENMASK(19, 0));
> +			datain = (word >> 2) & GENMASK(15, 0);
> +			checkword = prepare_ddr_data_word(datain, !i);
> +
> +			if (checkword != word) {
> +				ret = -EIO;
> +				break;
> +			}
> +
> +			crc5 = update_crc5(crc5, datain);
> +			cmds[0].data.in[i] = datain;
> +		}
> +
> +		word = (((u32 *)ccmd->rx_buf)[i] & GENMASK(19, 9));
> +		datain = (word >> 2) & GENMASK(15, 0);
> +		checkword = prepare_ddr_crc_word(crc5);
> +
> +		if (checkword != word)
> +			ret = -EIO;
> +	}
> +
> +	cdns_i3c_master_free_xfer(xfer);
> +
> +out_free_buf:
> +	kfree(buf);
> +
> +	return ret;
> +}
> +
>  static int cdns_i3c_master_i2c_xfers(struct i2c_dev_desc *dev,
>  				     const struct i2c_msg *xfers, int nxfers)
>  {
> @@ -1714,6 +1904,7 @@ static const struct i3c_master_controller_ops cdns_i3c_master_ops = {
>  	.supports_ccc_cmd = cdns_i3c_master_supports_ccc_cmd,
>  	.send_ccc_cmd = cdns_i3c_master_send_ccc_cmd,
>  	.priv_xfers = cdns_i3c_master_priv_xfers,
> +	.send_hdr_cmds = cdns_i3c_master_send_hdr_cmd,
>  	.i2c_xfers = cdns_i3c_master_i2c_xfers,
>  	.i2c_funcs = cdns_i3c_master_i2c_funcs,
>  	.enable_ibi = cdns_i3c_master_enable_ibi,


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

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

[vitor]

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

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

[Boris Brezillon]

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

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

[Przemyslaw Gaj]

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

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

[vitor]

On 22/02/19 15:02, Przemyslaw Gaj wrote:
>>>>  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.

Sorry, I meant all HDR frame.


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

@Boris: I remember you don't want to expose CCC commands, I think at least for now we shouldn't expose too.

Do you agree?

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


Best regards,
Vitor Soares

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

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

[Boris Brezillon]

Hi Vitor

On Fri, 22 Feb 2019 17:41:34 +0000
vitor <vitor.soares@synopsys.com> wrote:

> On 22/02/19 15:02, Przemyslaw Gaj wrote:
> >>>>  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.  
> 
> Sorry, I meant all HDR frame.

Then the proposed interface should work just fine, since we pass an
array of HDR commands to execute.

> 
> 
> >> 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;
> >> 	};
> >> }  
> 
> @Boris: I remember you don't want to expose CCC commands, I think at least for now we shouldn't expose too.
> 
> Do you agree?

Well, if we want to have a generic interface and not force all drivers
to implement one hook per transfer type, we should also have CCC
commands in this enum. We can still make sure the user does not pass
CCC commands before forwarding the request to the controller.

Anyway, as for all the other changes proposed so far, I'd like to see a
real use case for this SDR/HDR[/CCC] mix. Do you have a device spec
describing such a sequence?

Regards,

Boris

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

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

[vitor]

On 25/02/19 08:56, Boris Brezillon wrote:
> Hi Vitor
>
> On Fri, 22 Feb 2019 17:41:34 +0000
> vitor <vitor.soares@synopsys.com> wrote:
>
>> On 22/02/19 15:02, Przemyslaw Gaj wrote:
>>>>>>  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.  
>> Sorry, I meant all HDR frame.
> Then the proposed interface should work just fine, since we pass an
> array of HDR commands to execute.

I think it is not clear yet. When I referred <HDR command> I meant all HDR frame (ENTHDRx, HDR command, HDR CRC).

>>>> 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;
>>>> 	};
>>>> }  
>> @Boris: I remember you don't want to expose CCC commands, I think at least for now we shouldn't expose too.
>>
>> Do you agree?
> Well, if we want to have a generic interface and not force all drivers
> to implement one hook per transfer type, we should also have CCC
> commands in this enum. We can still make sure the user does not pass
> CCC commands before forwarding the request to the controller.

I'm ok with this. Still there is some CCC commands that might be expose for devices and we can decide it later.

> Anyway, as for all the other changes proposed so far, I'd like to see a
> real use case for this SDR/HDR[/CCC] mix. Do you have a device spec
> describing such a sequence?

From my knowledge there is no devices HDR capable on the market thus I cannot argue with real use case.

But per i3c spec v1.0 there is no mention that HDR frame shall begin with a START condition (not REPEATED START).
So, the question is why do not support it instead of limiting it?

We don't know the implementation of the device and some of them maybe need the previous transfer (in SDR) to do the next one (in HDR).

and supporting it we are cover more use cases.
> Regards,
>
> Boris

Best regards,
Vitor Soares

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

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

[Boris Brezillon]

On Mon, 25 Feb 2019 12:56:35 +0000
vitor <vitor.soares@synopsys.com> wrote:

> On 25/02/19 08:56, Boris Brezillon wrote:
> > Hi Vitor
> >
> > On Fri, 22 Feb 2019 17:41:34 +0000
> > vitor <vitor.soares@synopsys.com> wrote:
> >  
> >> On 22/02/19 15:02, Przemyslaw Gaj wrote:  
> >>>>>>  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.    
> >> Sorry, I meant all HDR frame.  
> > Then the proposed interface should work just fine, since we pass an
> > array of HDR commands to execute.  
> 
> I think it is not clear yet. When I referred <HDR command> I meant all HDR frame (ENTHDRx, HDR command, HDR CRC).

ENTHDRX and CRC is abstracted away. It's the master controller driver
responsibility to enter HDR mode (using a CCC command), generate CRCs
(either in HW or in SW) and add a HDRREPEAT after each HDR frame if
needed (if we have more than one HDR command to send).

> 
> >>>> 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;
> >>>> 	};
> >>>> }    
> >> @Boris: I remember you don't want to expose CCC commands, I think at least for now we shouldn't expose too.
> >>
> >> Do you agree?  
> > Well, if we want to have a generic interface and not force all drivers
> > to implement one hook per transfer type, we should also have CCC
> > commands in this enum. We can still make sure the user does not pass
> > CCC commands before forwarding the request to the controller.  
> 
> I'm ok with this. Still there is some CCC commands that might be expose for devices and we can decide it later.
> 
> > Anyway, as for all the other changes proposed so far, I'd like to see a
> > real use case for this SDR/HDR[/CCC] mix. Do you have a device spec
> > describing such a sequence?  
> 
> From my knowledge there is no devices HDR capable on the market thus I cannot argue with real use case.
> 
> But per i3c spec v1.0 there is no mention that HDR frame shall begin with a START condition (not REPEATED START).
> So, the question is why do not support it instead of limiting it?

Well, all depends on the extra complexity to support this case. If it's
simple enough, I won't complain.

> 
> We don't know the implementation of the device and some of them maybe need the previous transfer (in SDR) to do the next one (in HDR).

Exactly, we don't know, so maybe we should just wait before introducing
HDR mode...

> 
> and supporting it we are cover more use cases.

Are we even sure all controllers will be able to chain things like
that? For instance, in it's non-DMA mode, the Cadence controller is
limited by the FIFO size, which means you anyway won't be able to
chain too much frames using a REPEATED START. So, any device relying on
REPEATED START for atomic sequences is unlikely to work with all master
controllers.

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

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

[vitor]

On 25/02/19 13:10, Boris Brezillon wrote:
>>>>>> 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;
>>>>>> 	};
>>>>>> }    
>>>> @Boris: I remember you don't want to expose CCC commands, I think at least for now we shouldn't expose too.
>>>>
>>>> Do you agree?  
>>> Well, if we want to have a generic interface and not force all drivers
>>> to implement one hook per transfer type, we should also have CCC
>>> commands in this enum. We can still make sure the user does not pass
>>> CCC commands before forwarding the request to the controller.  
>> I'm ok with this. Still there is some CCC commands that might be expose for devices and we can decide it later.
>>
>>> Anyway, as for all the other changes proposed so far, I'd like to see a
>>> real use case for this SDR/HDR[/CCC] mix. Do you have a device spec
>>> describing such a sequence?  
>> From my knowledge there is no devices HDR capable on the market thus I cannot argue with real use case.
>>
>> But per i3c spec v1.0 there is no mention that HDR frame shall begin with a START condition (not REPEATED START).
>> So, the question is why do not support it instead of limiting it?
> Well, all depends on the extra complexity to support this case. If it's
> simple enough, I won't complain.
>
>> We don't know the implementation of the device and some of them maybe need the previous transfer (in SDR) to do the next one (in HDR).
> Exactly, we don't know, so maybe we should just wait before introducing
> HDR mode...
>
>> and supporting it we are cover more use cases.
> Are we even sure all controllers will be able to chain things like
> that? 

Why not? for the controllers shouldn't be a problem. The problem can be if it doesn't support.

> For instance, in it's non-DMA mode, the Cadence controller is
> limited by the FIFO size, which means you anyway won't be able to
> chain too much frames using a REPEATED START. So, any device relying on
> REPEATED START for atomic sequences is unlikely to work with all master
> controllers.

IMO this isn't up to us to decide, we just need to provide support and try to cover the most possible use cases.
Right now we only have 2 masters to support this kind of change and I just see some effort to support the CCC commands.

I would like to test this scenario, what do you think that can add extra complexity?


Best regards,
Vitor Soares

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

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

[Boris Brezillon]

Hi Vitor,

On Mon, 25 Feb 2019 16:45:21 +0000
vitor <vitor.soares@synopsys.com> wrote:

> On 25/02/19 13:10, Boris Brezillon wrote:
> >>>>>> something more generic than what we currently have.
> >>>>>>
> >>>>>> Something like
> >>>>>>
> >>>>>> enum i3c_xfer_type {
> >>>>>> 	I3C_CCC_XFER,
> >>>>>> 	I3C_SDR_XFER,
> >>>>>> 	I3C_HDR_XFER,

Forgot
		I3C_I2C_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;

and 

			struct i2c_msg i2c;

Since you want to make that fully generic.

> >>>>>> 	};
> >>>>>> }      
> >>>> @Boris: I remember you don't want to expose CCC commands, I think at least for now we shouldn't expose too.
> >>>>
> >>>> Do you agree?    
> >>> Well, if we want to have a generic interface and not force all drivers
> >>> to implement one hook per transfer type, we should also have CCC
> >>> commands in this enum. We can still make sure the user does not pass
> >>> CCC commands before forwarding the request to the controller.    
> >> I'm ok with this. Still there is some CCC commands that might be expose for devices and we can decide it later.
> >>  
> >>> Anyway, as for all the other changes proposed so far, I'd like to see a
> >>> real use case for this SDR/HDR[/CCC] mix. Do you have a device spec
> >>> describing such a sequence?    
> >> From my knowledge there is no devices HDR capable on the market thus I cannot argue with real use case.
> >>
> >> But per i3c spec v1.0 there is no mention that HDR frame shall begin with a START condition (not REPEATED START).
> >> So, the question is why do not support it instead of limiting it?  
> > Well, all depends on the extra complexity to support this case. If it's
> > simple enough, I won't complain.
> >  
> >> We don't know the implementation of the device and some of them maybe need the previous transfer (in SDR) to do the next one (in HDR).  
> > Exactly, we don't know, so maybe we should just wait before introducing
> > HDR mode...
> >  
> >> and supporting it we are cover more use cases.  
> > Are we even sure all controllers will be able to chain things like
> > that?   
> 
> Why not? for the controllers shouldn't be a problem. The problem can be if it doesn't support.

I don't know, maybe because some HW will be too limited to support
chained requests. I'm speculating, just like you are :-P.

> 
> > For instance, in it's non-DMA mode, the Cadence controller is
> > limited by the FIFO size, which means you anyway won't be able to
> > chain too much frames using a REPEATED START. So, any device relying on
> > REPEATED START for atomic sequences is unlikely to work with all master
> > controllers.  
> 
> IMO this isn't up to us to decide, we just need to provide support and try to cover the most possible use cases.

And yet, you take a decision based on your own speculations. You might
possibly be right, but until we start seeing devices that support HDR
modes that's not something we can be sure of.

> Right now we only have 2 masters to support this kind of change and I just see some effort to support the CCC commands.

Support CCC commands? We already support CCC commands. What do you mean?

> 
> I would like to test this scenario, what do you think that can add extra complexity?

Go ahead and post an implementation, we'll see how it looks like.
BTW, you have quite a few things on your TODO list already, but none of
these have led to patches being posted. Maybe you should focus on one
topic and finish it instead of constantly bringing new things you say
you plan to work on.

Regards,

Boris

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