When i3c controller reads data from slave device, slave device can stop
returning data with an ACK after any byte.
Add this support for svc i3c controller. Otherwise, it will go TIMEOUT
error path when the slave device ends the read operation early.
Signed-off-by: Clark Wang <[email protected]>
Reviewed-by: Jun Li <[email protected]>
---
V4 changes:
- Only use COMPLETE flag in the read function.
- Add a reset before each transfer.
V2/V3: No change.
---
drivers/i3c/master/svc-i3c-master.c | 56 ++++++++++++++++++++---------
1 file changed, 39 insertions(+), 17 deletions(-)
diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 4e69c691253d..e767f307db2d 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -896,27 +896,35 @@ static int svc_i3c_master_do_daa(struct i3c_master_controller *m)
static int svc_i3c_master_read(struct svc_i3c_master *master,
u8 *in, unsigned int len)
{
- int offset = 0, i, ret;
- u32 mdctrl;
+ int offset = 0, i;
+ u32 mdctrl, mstatus;
+ bool completed = false;
+ unsigned int count;
+ unsigned long start = jiffies;
- while (offset < len) {
- unsigned int count;
+ while (!completed) {
+ mstatus = readl(master->regs + SVC_I3C_MSTATUS);
+ if (SVC_I3C_MSTATUS_COMPLETE(mstatus) != 0)
+ completed = true;
- ret = readl_poll_timeout(master->regs + SVC_I3C_MDATACTRL,
- mdctrl,
- !(mdctrl & SVC_I3C_MDATACTRL_RXEMPTY),
- 0, 1000);
- if (ret)
- return ret;
+ if (time_after(jiffies, start + msecs_to_jiffies(1000))) {
+ dev_dbg(master->dev, "<%s> timeout\n", __func__);
+ return -ETIMEDOUT;
+ }
+ mdctrl = readl(master->regs + SVC_I3C_MDATACTRL);
count = SVC_I3C_MDATACTRL_RXCOUNT(mdctrl);
+ if (offset + count > len) {
+ dev_err(master->dev, "<%s> recv len too long!\n",
+ __func__);
+ return -EINVAL;
+ }
for (i = 0; i < count; i++)
in[offset + i] = readl(master->regs + SVC_I3C_MRDATAB);
-
offset += count;
}
- return 0;
+ return offset;
}
static int svc_i3c_master_write(struct svc_i3c_master *master,
@@ -949,7 +957,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 *in, const u8 *out, unsigned int xfer_len,
- unsigned int read_len, bool continued)
+ unsigned int *read_len, bool continued)
{
u32 reg;
int ret;
@@ -959,7 +967,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
SVC_I3C_MCTRL_IBIRESP_NACK |
SVC_I3C_MCTRL_DIR(rnw) |
SVC_I3C_MCTRL_ADDR(addr) |
- SVC_I3C_MCTRL_RDTERM(read_len),
+ SVC_I3C_MCTRL_RDTERM(*read_len),
master->regs + SVC_I3C_MCTRL);
ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
@@ -971,17 +979,25 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
ret = svc_i3c_master_read(master, in, xfer_len);
else
ret = svc_i3c_master_write(master, out, xfer_len);
- if (ret)
+ if (ret < 0)
goto emit_stop;
+ if (rnw)
+ *read_len = ret;
ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
SVC_I3C_MSTATUS_COMPLETE(reg), 0, 1000);
if (ret)
goto emit_stop;
+ writel(SVC_I3C_MINT_COMPLETE, master->regs + SVC_I3C_MSTATUS);
- if (!continued)
+ if (!continued) {
svc_i3c_master_emit_stop(master);
+ /* Wait idle if stop is sent. */
+ readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
+ !SVC_I3C_MSTATUS_STATE(reg), 0, 1000);
+ }
+
return 0;
emit_stop:
@@ -1039,12 +1055,15 @@ static void svc_i3c_master_start_xfer_locked(struct svc_i3c_master *master)
if (!xfer)
return;
+ svc_i3c_master_clear_merrwarn(master);
+ svc_i3c_master_flush_fifo(master);
+
for (i = 0; i < xfer->ncmds; i++) {
struct svc_i3c_cmd *cmd = &xfer->cmds[i];
ret = svc_i3c_master_xfer(master, cmd->rnw, xfer->type,
cmd->addr, cmd->in, cmd->out,
- cmd->len, cmd->read_len,
+ cmd->len, &cmd->read_len,
cmd->continued);
if (ret)
break;
@@ -1173,6 +1192,9 @@ static int svc_i3c_master_send_direct_ccc_cmd(struct svc_i3c_master *master,
if (!wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000)))
svc_i3c_master_dequeue_xfer(master, xfer);
+ if (cmd->read_len != xfer_len)
+ ccc->dests[0].payload.len = cmd->read_len;
+
ret = xfer->ret;
svc_i3c_master_free_xfer(xfer);
--
2.25.1
Hi Clark,
Just a few nitpicks.
Clark Wang <[email protected]> wrote on Mon, 9 Aug 2021 14:36:41
+0800:
> When i3c controller reads data from slave device, slave device can stop
> returning data with an ACK after any byte.
> Add this support for svc i3c controller. Otherwise, it will go TIMEOUT
"it will timeout when the slave..."
> error path when the slave device ends the read operation early.
>
> Signed-off-by: Clark Wang <[email protected]>
> Reviewed-by: Jun Li <[email protected]>
> ---
> V4 changes:
> - Only use COMPLETE flag in the read function.
> - Add a reset before each transfer.
> V2/V3: No change.
> ---
> drivers/i3c/master/svc-i3c-master.c | 56 ++++++++++++++++++++---------
> 1 file changed, 39 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index 4e69c691253d..e767f307db2d 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -896,27 +896,35 @@ static int svc_i3c_master_do_daa(struct i3c_master_controller *m)
> static int svc_i3c_master_read(struct svc_i3c_master *master,
> u8 *in, unsigned int len)
> {
> - int offset = 0, i, ret;
> - u32 mdctrl;
> + int offset = 0, i;
> + u32 mdctrl, mstatus;
> + bool completed = false;
> + unsigned int count;
> + unsigned long start = jiffies;
>
> - while (offset < len) {
> - unsigned int count;
> + while (!completed) {
> + mstatus = readl(master->regs + SVC_I3C_MSTATUS);
> + if (SVC_I3C_MSTATUS_COMPLETE(mstatus) != 0)
> + completed = true;
>
> - ret = readl_poll_timeout(master->regs + SVC_I3C_MDATACTRL,
> - mdctrl,
> - !(mdctrl & SVC_I3C_MDATACTRL_RXEMPTY),
> - 0, 1000);
> - if (ret)
> - return ret;
> + if (time_after(jiffies, start + msecs_to_jiffies(1000))) {
> + dev_dbg(master->dev, "<%s> timeout\n", __func__);
Can you get rid of this <%s>/__func__ parameter and just tell that we
got a read timeout?
> + return -ETIMEDOUT;
> + }
>
> + mdctrl = readl(master->regs + SVC_I3C_MDATACTRL);
> count = SVC_I3C_MDATACTRL_RXCOUNT(mdctrl);
> + if (offset + count > len) {
> + dev_err(master->dev, "<%s> recv len too long!\n",
> + __func__);
> + return -EINVAL;
> + }
And is this something theoretical or a real error that can happen?
If you think this is really needed, please keep the same dev_err()
formatting as in the other places in this driver.
> for (i = 0; i < count; i++)
> in[offset + i] = readl(master->regs + SVC_I3C_MRDATAB);
> -
Please keep that space
> offset += count;
> }
>
> - return 0;
> + return offset;
> }
>
> static int svc_i3c_master_write(struct svc_i3c_master *master,
> @@ -949,7 +957,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 *in, const u8 *out, unsigned int xfer_len,
> - unsigned int read_len, bool continued)
> + unsigned int *read_len, bool continued)
> {
> u32 reg;
> int ret;
> @@ -959,7 +967,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> SVC_I3C_MCTRL_IBIRESP_NACK |
> SVC_I3C_MCTRL_DIR(rnw) |
> SVC_I3C_MCTRL_ADDR(addr) |
> - SVC_I3C_MCTRL_RDTERM(read_len),
> + SVC_I3C_MCTRL_RDTERM(*read_len),
> master->regs + SVC_I3C_MCTRL);
>
> ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> @@ -971,17 +979,25 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> ret = svc_i3c_master_read(master, in, xfer_len);
> else
> ret = svc_i3c_master_write(master, out, xfer_len);
> - if (ret)
> + if (ret < 0)
> goto emit_stop;
Space?
> + if (rnw)
> + *read_len = ret;
>
> ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> SVC_I3C_MSTATUS_COMPLETE(reg), 0, 1000);
> if (ret)
> goto emit_stop;
Space?
> + writel(SVC_I3C_MINT_COMPLETE, master->regs + SVC_I3C_MSTATUS);
>
> - if (!continued)
> + if (!continued) {
> svc_i3c_master_emit_stop(master);
>
> + /* Wait idle if stop is sent. */
> + readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> + !SVC_I3C_MSTATUS_STATE(reg), 0, 1000);
Please use _MSTATUS_STATE_IDLE() for more clarity.
If this wait loop is really needed, better adding it to
_master_emit_stop()? (in another patch)
> + }
> +
> return 0;
>
> emit_stop:
> @@ -1039,12 +1055,15 @@ static void svc_i3c_master_start_xfer_locked(struct svc_i3c_master *master)
> if (!xfer)
> return;
>
> + svc_i3c_master_clear_merrwarn(master);
> + svc_i3c_master_flush_fifo(master);
> +
> for (i = 0; i < xfer->ncmds; i++) {
> struct svc_i3c_cmd *cmd = &xfer->cmds[i];
>
> ret = svc_i3c_master_xfer(master, cmd->rnw, xfer->type,
> cmd->addr, cmd->in, cmd->out,
> - cmd->len, cmd->read_len,
> + cmd->len, &cmd->read_len,
> cmd->continued);
> if (ret)
> break;
> @@ -1173,6 +1192,9 @@ static int svc_i3c_master_send_direct_ccc_cmd(struct svc_i3c_master *master,
> if (!wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000)))
> svc_i3c_master_dequeue_xfer(master, xfer);
>
> + if (cmd->read_len != xfer_len)
> + ccc->dests[0].payload.len = cmd->read_len;
> +
> ret = xfer->ret;
> svc_i3c_master_free_xfer(xfer);
>
Thanks,
Miquèl