Add basic HDR mode support for the svs I3C master driver.
Only support for private transfers and does not support sending CCC
commands in HDR mode.
Key differences:
- HDR uses commands (0x00-0x7F for write, 0x80-0xFF for read) to
distinguish transfer direction.
- HDR read/write commands must be written to FIFO before issuing the I3C
address command. The hardware automatically sends the standard CCC command
to enter HDR mode.
- HDR exit pattern must be sent instead of send a stop after transfer
completion.
- Read/write data size must be an even number.
Co-developed-by: Carlos Song <carlos.song@nxp.com>
Signed-off-by: Carlos Song <carlos.song@nxp.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change in v2
- support HDR DDR write
- rdterm unit is byte, not words (RM is wrong).
---
drivers/i3c/master/svc-i3c-master.c | 95 +++++++++++++++++++++++++++++--------
1 file changed, 76 insertions(+), 19 deletions(-)
diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 701ae165b25b7991360f3a862b34cc1870a9a2ba..3885edea90db4c943a5f13ec4a291ed15cf9decb 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -40,11 +40,13 @@
#define SVC_I3C_MCTRL_REQUEST_NONE 0
#define SVC_I3C_MCTRL_REQUEST_START_ADDR 1
#define SVC_I3C_MCTRL_REQUEST_STOP 2
+#define SVC_I3C_MCTRL_REQUEST_FORCE_EXIT 6
#define SVC_I3C_MCTRL_REQUEST_IBI_ACKNACK 3
#define SVC_I3C_MCTRL_REQUEST_PROC_DAA 4
#define SVC_I3C_MCTRL_REQUEST_AUTO_IBI 7
#define SVC_I3C_MCTRL_TYPE_I3C 0
#define SVC_I3C_MCTRL_TYPE_I2C BIT(4)
+#define SVC_I3C_MCTRL_TYPE_DDR BIT(5)
#define SVC_I3C_MCTRL_IBIRESP_AUTO 0
#define SVC_I3C_MCTRL_IBIRESP_ACK_WITHOUT_BYTE 0
#define SVC_I3C_MCTRL_IBIRESP_ACK_WITH_BYTE BIT(7)
@@ -95,6 +97,7 @@
#define SVC_I3C_MINTMASKED 0x098
#define SVC_I3C_MERRWARN 0x09C
#define SVC_I3C_MERRWARN_NACK BIT(2)
+#define SVC_I3C_MERRWARN_CRC BIT(10)
#define SVC_I3C_MERRWARN_TIMEOUT BIT(20)
#define SVC_I3C_MDMACTRL 0x0A0
#define SVC_I3C_MDATACTRL 0x0AC
@@ -165,7 +168,7 @@
struct svc_i3c_cmd {
u8 addr;
- bool rnw;
+ u8 rnw;
u8 *in;
const void *out;
unsigned int len;
@@ -383,6 +386,21 @@ svc_i3c_master_dev_from_addr(struct svc_i3c_master *master,
return master->descs[i];
}
+static bool svc_is_read(u8 rnw_cmd, u32 type)
+{
+ return (type == SVC_I3C_MCTRL_TYPE_DDR) ? !!(rnw_cmd & 0x80) : rnw_cmd;
+}
+
+static void svc_i3c_master_emit_force_exit(struct svc_i3c_master *master)
+{
+ u32 reg = 0;
+
+ writel(SVC_I3C_MCTRL_REQUEST_FORCE_EXIT, master->regs + SVC_I3C_MCTRL);
+ readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
+ SVC_I3C_MSTATUS_MCTRLDONE(reg), 0, 1000);
+ udelay(1);
+}
+
static void svc_i3c_master_emit_stop(struct svc_i3c_master *master)
{
writel(SVC_I3C_MCTRL_REQUEST_STOP, master->regs + SVC_I3C_MCTRL);
@@ -1272,7 +1290,7 @@ static int svc_i3c_master_write(struct svc_i3c_master *master,
}
static int svc_i3c_master_xfer(struct svc_i3c_master *master,
- bool rnw, unsigned int xfer_type, u8 addr,
+ u8 rnw, unsigned int xfer_type, u8 addr,
u8 *in, const u8 *out, unsigned int xfer_len,
unsigned int *actual_len, bool continued, bool repeat_start)
{
@@ -1283,12 +1301,22 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
/* clean SVC_I3C_MINT_IBIWON w1c bits */
writel(SVC_I3C_MINT_IBIWON, master->regs + SVC_I3C_MSTATUS);
+ if (xfer_type == SVC_I3C_MCTRL_TYPE_DDR) {
+ /* DDR command need prefill into FIFO */
+ writel(rnw, master->regs + SVC_I3C_MWDATAB);
+ if (!svc_is_read(rnw, xfer_type)) {
+ /* write data also need prefill into FIFO */
+ ret = svc_i3c_master_write(master, out, xfer_len);
+ if (ret)
+ goto emit_stop;
+ }
+ }
while (retry--) {
writel(SVC_I3C_MCTRL_REQUEST_START_ADDR |
xfer_type |
SVC_I3C_MCTRL_IBIRESP_NACK |
- SVC_I3C_MCTRL_DIR(rnw) |
+ SVC_I3C_MCTRL_DIR(svc_is_read(rnw, xfer_type)) |
SVC_I3C_MCTRL_ADDR(addr) |
SVC_I3C_MCTRL_RDTERM(*actual_len),
master->regs + SVC_I3C_MCTRL);
@@ -1373,15 +1401,14 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
break;
}
}
-
- if (rnw)
+ if (svc_is_read(rnw, xfer_type))
ret = svc_i3c_master_read(master, in, xfer_len);
- else
+ else if (xfer_type != SVC_I3C_MCTRL_TYPE_DDR)
ret = svc_i3c_master_write(master, out, xfer_len);
if (ret < 0)
goto emit_stop;
- if (rnw)
+ if (svc_is_read(rnw, xfer_type))
*actual_len = ret;
ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
@@ -1389,10 +1416,19 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
if (ret)
goto emit_stop;
+ if (xfer_type == SVC_I3C_MCTRL_TYPE_DDR &&
+ (readl(master->regs + SVC_I3C_MERRWARN) & SVC_I3C_MERRWARN_CRC)) {
+ ret = -ENXIO;
+ goto emit_stop;
+ }
+
writel(SVC_I3C_MINT_COMPLETE, master->regs + SVC_I3C_MSTATUS);
if (!continued) {
- svc_i3c_master_emit_stop(master);
+ if (xfer_type != SVC_I3C_MCTRL_TYPE_DDR)
+ svc_i3c_master_emit_stop(master);
+ else
+ svc_i3c_master_emit_force_exit(master);
/* Wait idle if stop is sent. */
readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
@@ -1402,7 +1438,11 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
return 0;
emit_stop:
- svc_i3c_master_emit_stop(master);
+ if (xfer_type != SVC_I3C_MCTRL_TYPE_DDR)
+ svc_i3c_master_emit_stop(master);
+ else
+ svc_i3c_master_emit_force_exit(master);
+
svc_i3c_master_clear_merrwarn(master);
svc_i3c_master_flush_fifo(master);
@@ -1449,6 +1489,11 @@ static void svc_i3c_master_dequeue_xfer(struct svc_i3c_master *master,
spin_unlock_irqrestore(&master->xferqueue.lock, flags);
}
+static int mode_to_type(enum i3c_hdr_mode mode)
+{
+ return (mode == I3C_SDR) ? SVC_I3C_MCTRL_TYPE_I3C : SVC_I3C_MCTRL_TYPE_DDR;
+}
+
static void svc_i3c_master_start_xfer_locked(struct svc_i3c_master *master)
{
struct svc_i3c_xfer *xfer = master->xferqueue.cur;
@@ -1638,9 +1683,8 @@ static int svc_i3c_master_send_ccc_cmd(struct i3c_master_controller *m,
return ret;
}
-static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
- struct i3c_priv_xfer *xfers,
- int nxfers)
+static int svc_i3c_master_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_priv_xfer *xfers,
+ int nxfers, enum i3c_hdr_mode mode)
{
struct i3c_master_controller *m = i3c_dev_get_master(dev);
struct svc_i3c_master *master = to_svc_i3c_master(m);
@@ -1648,22 +1692,33 @@ static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
struct svc_i3c_xfer *xfer;
int ret, i;
+ if (mode != I3C_SDR) {
+ /*
+ * Only support data size less than FIFO SIZE when use DDR mode.
+ * First entry is cmd in FIFO, so actual available FIFO for data
+ * is SVC_I3C_FIFO_SIZE - 2 since DDR only support even length.
+ */
+ for (i = 0; i < nxfers; i++)
+ if (xfers[i].len > SVC_I3C_FIFO_SIZE - 2)
+ return -EINVAL;
+ }
+
xfer = svc_i3c_master_alloc_xfer(master, nxfers);
if (!xfer)
return -ENOMEM;
- xfer->type = SVC_I3C_MCTRL_TYPE_I3C;
+ xfer->type = mode_to_type(mode);
for (i = 0; i < nxfers; i++) {
+ u8 rnw_cmd = (mode == I3C_SDR) ? xfers[i].rnw : xfers[i].cmd;
struct svc_i3c_cmd *cmd = &xfer->cmds[i];
-
cmd->xfer = &xfers[i];
cmd->addr = master->addrs[data->index];
- cmd->rnw = xfers[i].rnw;
- cmd->in = xfers[i].rnw ? xfers[i].data.in : NULL;
- cmd->out = xfers[i].rnw ? NULL : xfers[i].data.out;
+ cmd->rnw = rnw_cmd;
+ cmd->in = svc_is_read(rnw_cmd, mode_to_type(mode)) ? xfers[i].data.in : NULL;
+ cmd->out = svc_is_read(rnw_cmd, mode_to_type(mode)) ? NULL : xfers[i].data.out;
cmd->len = xfers[i].len;
- cmd->actual_len = xfers[i].rnw ? xfers[i].len : 0;
+ cmd->actual_len = svc_is_read(rnw_cmd, mode_to_type(mode)) ? xfers[i].len : 0;
cmd->continued = (i + 1) < nxfers;
}
@@ -1858,7 +1913,7 @@ static const struct i3c_master_controller_ops svc_i3c_master_ops = {
.do_daa = svc_i3c_master_do_daa,
.supports_ccc_cmd = svc_i3c_master_supports_ccc_cmd,
.send_ccc_cmd = svc_i3c_master_send_ccc_cmd,
- .priv_xfers = svc_i3c_master_priv_xfers,
+ .i3c_xfers = svc_i3c_master_i3c_xfers,
.i2c_xfers = svc_i3c_master_i2c_xfers,
.request_ibi = svc_i3c_master_request_ibi,
.free_ibi = svc_i3c_master_free_ibi,
@@ -1947,6 +2002,8 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
svc_i3c_master_reset(master);
+ master->base.mode_mask = BIT(I3C_SDR) | BIT(I3C_HDR_DDR);
+
/* Register the master */
ret = i3c_master_register(&master->base, &pdev->dev,
&svc_i3c_master_ops, false);
--
2.34.1
Hi Frank, > struct svc_i3c_cmd { > u8 addr; > - bool rnw; > + u8 rnw; You used a union in the core, which makes sense I believe. I guess you should do it here as well? > u8 *in; > const void *out; > unsigned int len; > @@ -383,6 +386,21 @@ svc_i3c_master_dev_from_addr(struct svc_i3c_master *master, > return master->descs[i]; > } Maybe we should: - First change the type of the command and make use of the helper to derive the fact that it is a read then - Introduce HDR support. Because there seems to be a lot of changes which are induced by this internal API change and not related to HDR introduction at all. > > +static bool svc_is_read(u8 rnw_cmd, u32 type) Can we name this helper svc_cmd_is_read() ? > +{ > + return (type == SVC_I3C_MCTRL_TYPE_DDR) ? !!(rnw_cmd & 0x80) : rnw_cmd; > +} > + > +static void svc_i3c_master_emit_force_exit(struct svc_i3c_master *master) > +{ > + u32 reg = 0; > + > + writel(SVC_I3C_MCTRL_REQUEST_FORCE_EXIT, master->regs + SVC_I3C_MCTRL); > + readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg, > + SVC_I3C_MSTATUS_MCTRLDONE(reg), 0, 1000); > + udelay(1); > +} > + > static void svc_i3c_master_emit_stop(struct svc_i3c_master *master) > { > writel(SVC_I3C_MCTRL_REQUEST_STOP, master->regs + SVC_I3C_MCTRL); > @@ -1272,7 +1290,7 @@ static int svc_i3c_master_write(struct svc_i3c_master *master, > } > > static int svc_i3c_master_xfer(struct svc_i3c_master *master, > - bool rnw, unsigned int xfer_type, u8 addr, > + u8 rnw, unsigned int xfer_type, u8 addr, > u8 *in, const u8 *out, unsigned int xfer_len, > unsigned int *actual_len, bool continued, bool repeat_start) > { > @@ -1283,12 +1301,22 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master, > /* clean SVC_I3C_MINT_IBIWON w1c bits */ > writel(SVC_I3C_MINT_IBIWON, master->regs + SVC_I3C_MSTATUS); > > + if (xfer_type == SVC_I3C_MCTRL_TYPE_DDR) { > + /* DDR command need prefill into FIFO */ > + writel(rnw, master->regs + SVC_I3C_MWDATAB); > + if (!svc_is_read(rnw, xfer_type)) { > + /* write data also need prefill into FIFO */ > + ret = svc_i3c_master_write(master, out, xfer_len); > + if (ret) > + goto emit_stop; > + } > + } > > while (retry--) { > writel(SVC_I3C_MCTRL_REQUEST_START_ADDR | > xfer_type | > SVC_I3C_MCTRL_IBIRESP_NACK | > - SVC_I3C_MCTRL_DIR(rnw) | > + SVC_I3C_MCTRL_DIR(svc_is_read(rnw, xfer_type)) | > SVC_I3C_MCTRL_ADDR(addr) | > SVC_I3C_MCTRL_RDTERM(*actual_len), > master->regs + SVC_I3C_MCTRL); > @@ -1373,15 +1401,14 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master, > break; > } > } > - > - if (rnw) > + if (svc_is_read(rnw, xfer_type)) > ret = svc_i3c_master_read(master, in, xfer_len); > - else > + else if (xfer_type != SVC_I3C_MCTRL_TYPE_DDR) > ret = svc_i3c_master_write(master, out, xfer_len); > if (ret < 0) > goto emit_stop; > > - if (rnw) > + if (svc_is_read(rnw, xfer_type)) > *actual_len = ret; > > ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg, > @@ -1389,10 +1416,19 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master, > if (ret) > goto emit_stop; > > + if (xfer_type == SVC_I3C_MCTRL_TYPE_DDR && > + (readl(master->regs + SVC_I3C_MERRWARN) & SVC_I3C_MERRWARN_CRC)) { > + ret = -ENXIO; > + goto emit_stop; > + } > + > writel(SVC_I3C_MINT_COMPLETE, master->regs + SVC_I3C_MSTATUS); > > if (!continued) { > - svc_i3c_master_emit_stop(master); > + if (xfer_type != SVC_I3C_MCTRL_TYPE_DDR) > + svc_i3c_master_emit_stop(master); > + else > + svc_i3c_master_emit_force_exit(master); > > /* Wait idle if stop is sent. */ > readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg, > @@ -1402,7 +1438,11 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master, > return 0; > > emit_stop: > - svc_i3c_master_emit_stop(master); > + if (xfer_type != SVC_I3C_MCTRL_TYPE_DDR) > + svc_i3c_master_emit_stop(master); > + else > + svc_i3c_master_emit_force_exit(master); > + > svc_i3c_master_clear_merrwarn(master); > svc_i3c_master_flush_fifo(master); > > @@ -1449,6 +1489,11 @@ static void svc_i3c_master_dequeue_xfer(struct svc_i3c_master *master, > spin_unlock_irqrestore(&master->xferqueue.lock, flags); > } > > +static int mode_to_type(enum i3c_hdr_mode mode) Maybe "i3c_mode_to_svc_type()"? > +{ > + return (mode == I3C_SDR) ? SVC_I3C_MCTRL_TYPE_I3C : SVC_I3C_MCTRL_TYPE_DDR; > +} > + > static void svc_i3c_master_start_xfer_locked(struct svc_i3c_master *master) > { > struct svc_i3c_xfer *xfer = master->xferqueue.cur; > @@ -1638,9 +1683,8 @@ static int svc_i3c_master_send_ccc_cmd(struct i3c_master_controller *m, > return ret; > } > > -static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev, > - struct i3c_priv_xfer *xfers, > - int nxfers) > +static int svc_i3c_master_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_priv_xfer *xfers, > + int nxfers, enum i3c_hdr_mode mode) > { > struct i3c_master_controller *m = i3c_dev_get_master(dev); > struct svc_i3c_master *master = to_svc_i3c_master(m); > @@ -1648,22 +1692,33 @@ static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev, > struct svc_i3c_xfer *xfer; > int ret, i; > > + if (mode != I3C_SDR) { > + /* > + * Only support data size less than FIFO SIZE when use > DDR mode. when using DDR mode. > + * First entry is cmd in FIFO, so actual available FIFO for data > + * is SVC_I3C_FIFO_SIZE - 2 since DDR only support even > length. supports > + */ > + for (i = 0; i < nxfers; i++) > + if (xfers[i].len > SVC_I3C_FIFO_SIZE - 2) > + return -EINVAL; > + } > + > xfer = svc_i3c_master_alloc_xfer(master, nxfers); > if (!xfer) > return -ENOMEM; > > - xfer->type = SVC_I3C_MCTRL_TYPE_I3C; > + xfer->type = mode_to_type(mode); > > for (i = 0; i < nxfers; i++) { > + u8 rnw_cmd = (mode == I3C_SDR) ? xfers[i].rnw : xfers[i].cmd; > struct svc_i3c_cmd *cmd = &xfer->cmds[i]; > - Spurious change? > cmd->xfer = &xfers[i]; > cmd->addr = master->addrs[data->index]; > - cmd->rnw = xfers[i].rnw; > - cmd->in = xfers[i].rnw ? xfers[i].data.in : NULL; > - cmd->out = xfers[i].rnw ? NULL : xfers[i].data.out; > + cmd->rnw = rnw_cmd; > + cmd->in = svc_is_read(rnw_cmd, mode_to_type(mode)) ? xfers[i].data.in : NULL; > + cmd->out = svc_is_read(rnw_cmd, mode_to_type(mode)) ? NULL : xfers[i].data.out; > cmd->len = xfers[i].len; > - cmd->actual_len = xfers[i].rnw ? xfers[i].len : 0; > + cmd->actual_len = svc_is_read(rnw_cmd, mode_to_type(mode)) ? xfers[i].len : 0; > cmd->continued = (i + 1) < nxfers; > } > > @@ -1858,7 +1913,7 @@ static const struct i3c_master_controller_ops svc_i3c_master_ops = { > .do_daa = svc_i3c_master_do_daa, > .supports_ccc_cmd = svc_i3c_master_supports_ccc_cmd, > .send_ccc_cmd = svc_i3c_master_send_ccc_cmd, > - .priv_xfers = svc_i3c_master_priv_xfers, > + .i3c_xfers = svc_i3c_master_i3c_xfers, Didn't you change this name in patch 1? If you kept both naming, it's fine, otherwise you must do the switch in patch 1 as well to make sure you don't break the build in the middle of the series. > .i2c_xfers = svc_i3c_master_i2c_xfers, > .request_ibi = svc_i3c_master_request_ibi, > .free_ibi = svc_i3c_master_free_ibi, > @@ -1947,6 +2002,8 @@ static int svc_i3c_master_probe(struct platform_device *pdev) > > svc_i3c_master_reset(master); > > + master->base.mode_mask = BIT(I3C_SDR) | BIT(I3C_HDR_DDR); > + > /* Register the master */ > ret = i3c_master_register(&master->base, &pdev->dev, > &svc_i3c_master_ops, false); Thanks, Miquèl
On Mon, Sep 29, 2025 at 05:38:03PM +0200, Miquel Raynal wrote: > Hi Frank, > ... > > @@ -1858,7 +1913,7 @@ static const struct i3c_master_controller_ops svc_i3c_master_ops = { > > .do_daa = svc_i3c_master_do_daa, > > .supports_ccc_cmd = svc_i3c_master_supports_ccc_cmd, > > .send_ccc_cmd = svc_i3c_master_send_ccc_cmd, > > - .priv_xfers = svc_i3c_master_priv_xfers, > > + .i3c_xfers = svc_i3c_master_i3c_xfers, > > Didn't you change this name in patch 1? If you kept both naming, it's > fine, otherwise you must do the switch in patch 1 as well to make sure > you don't break the build in the middle of the series. We keep both API for while. After first patch apply, I change all drivers to i3c_xfers, then remove old .priv_xfers. Frank > > > .i2c_xfers = svc_i3c_master_i2c_xfers, > > .request_ibi = svc_i3c_master_request_ibi, > > .free_ibi = svc_i3c_master_free_ibi, > > @@ -1947,6 +2002,8 @@ static int svc_i3c_master_probe(struct platform_device *pdev) > > > > svc_i3c_master_reset(master); > > > > + master->base.mode_mask = BIT(I3C_SDR) | BIT(I3C_HDR_DDR); > > + > > /* Register the master */ > > ret = i3c_master_register(&master->base, &pdev->dev, > > &svc_i3c_master_ops, false); > > Thanks, > Miquèl
On 29/09/2025 at 11:55:31 -04, Frank Li <Frank.li@nxp.com> wrote: > On Mon, Sep 29, 2025 at 05:38:03PM +0200, Miquel Raynal wrote: >> Hi Frank, >> > ... > >> > @@ -1858,7 +1913,7 @@ static const struct i3c_master_controller_ops svc_i3c_master_ops = { >> > .do_daa = svc_i3c_master_do_daa, >> > .supports_ccc_cmd = svc_i3c_master_supports_ccc_cmd, >> > .send_ccc_cmd = svc_i3c_master_send_ccc_cmd, >> > - .priv_xfers = svc_i3c_master_priv_xfers, >> > + .i3c_xfers = svc_i3c_master_i3c_xfers, >> >> Didn't you change this name in patch 1? If you kept both naming, it's >> fine, otherwise you must do the switch in patch 1 as well to make sure >> you don't break the build in the middle of the series. > > We keep both API for while. After first patch apply, I change all drivers > to i3c_xfers, then remove old .priv_xfers. Fine. Maybe mark it deprecated with a comment in the core? (I haven't checked).
© 2016 - 2025 Red Hat, Inc.