| Message ID | 20231016153232.2851095-4-Frank.Li@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i3c: master: svc: collection of bugs fixes | expand |
Hi Frank, Frank.Li@nxp.com wrote on Mon, 16 Oct 2023 11:32:29 -0400: > MSTATUS[RXPEND] is only updated after the data transfer cycle started. This > creates an issue when the I3C clock is slow, and the CPU is running fast > enough that MSTATUS[RXPEND] may not be updated when the code reach checking > point. As a result, mandatory data are being missed. can be missed. > Add a wait for MSTATUS[COMPLETE] to ensure that all mandatory data already > in FIFO. is already in the FIFO > > Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver") > Cc: stable@vger.kernel.org > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- > drivers/i3c/master/svc-i3c-master.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c > index 0f57a5f75e39..c252446b2bc5 100644 > --- a/drivers/i3c/master/svc-i3c-master.c > +++ b/drivers/i3c/master/svc-i3c-master.c > @@ -331,6 +331,7 @@ static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master, > struct i3c_ibi_slot *slot; > unsigned int count; > u32 mdatactrl; > + int ret, val; > u8 *buf; > > slot = i3c_generic_ibi_get_free_slot(data->ibi_pool); > @@ -340,6 +341,13 @@ static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master, > slot->len = 0; > buf = slot->data; > > + ret = readl_relaxed_poll_timeout(master->regs + SVC_I3C_MSTATUS, val, > + SVC_I3C_MSTATUS_COMPLETE(val), 0, 1000); Are you sure !MSTATUS_COMPLETE(val) is never a valid condition? Especially with non-mandatory bytes? Also, are you sure of the indentation here? > + if (ret) { > + dev_err(master->dev, "Timeout when polling for COMPLETE\n"); > + return ret; > + } > + > while (SVC_I3C_MSTATUS_RXPEND(readl(master->regs + SVC_I3C_MSTATUS)) && > slot->len < SVC_I3C_FIFO_SIZE) { > mdatactrl = readl(master->regs + SVC_I3C_MDATACTRL); Thanks, Miquèl
On Tue, Oct 17, 2023 at 04:27:07PM +0200, Miquel Raynal wrote: > Hi Frank, > > Frank.Li@nxp.com wrote on Mon, 16 Oct 2023 11:32:29 -0400: > > > MSTATUS[RXPEND] is only updated after the data transfer cycle started. This > > creates an issue when the I3C clock is slow, and the CPU is running fast > > enough that MSTATUS[RXPEND] may not be updated when the code reach checking > > point. As a result, mandatory data are being missed. > > can be missed. > > > Add a wait for MSTATUS[COMPLETE] to ensure that all mandatory data already > > in FIFO. > > is already in the FIFO > > > > > Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver") > > Cc: stable@vger.kernel.org > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > --- > > drivers/i3c/master/svc-i3c-master.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c > > index 0f57a5f75e39..c252446b2bc5 100644 > > --- a/drivers/i3c/master/svc-i3c-master.c > > +++ b/drivers/i3c/master/svc-i3c-master.c > > @@ -331,6 +331,7 @@ static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master, > > struct i3c_ibi_slot *slot; > > unsigned int count; > > u32 mdatactrl; > > + int ret, val; > > u8 *buf; > > > > slot = i3c_generic_ibi_get_free_slot(data->ibi_pool); > > @@ -340,6 +341,13 @@ static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master, > > slot->len = 0; > > buf = slot->data; > > > > + ret = readl_relaxed_poll_timeout(master->regs + SVC_I3C_MSTATUS, val, > > + SVC_I3C_MSTATUS_COMPLETE(val), 0, 1000); > > Are you sure !MSTATUS_COMPLETE(val) is never a valid condition? > Especially with non-mandatory bytes? > > Also, are you sure of the indentation here? If no-mandatory data, it is equal RDTERAM is 0. It takes some times to change my slave code to create such test case. Frank Li > > > + if (ret) { > > + dev_err(master->dev, "Timeout when polling for COMPLETE\n"); > > + return ret; > > + } > > + > > while (SVC_I3C_MSTATUS_RXPEND(readl(master->regs + SVC_I3C_MSTATUS)) && > > slot->len < SVC_I3C_FIFO_SIZE) { > > mdatactrl = readl(master->regs + SVC_I3C_MDATACTRL); > > > Thanks, > Miquèl
diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c index 0f57a5f75e39..c252446b2bc5 100644 --- a/drivers/i3c/master/svc-i3c-master.c +++ b/drivers/i3c/master/svc-i3c-master.c @@ -331,6 +331,7 @@ static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master, struct i3c_ibi_slot *slot; unsigned int count; u32 mdatactrl; + int ret, val; u8 *buf; slot = i3c_generic_ibi_get_free_slot(data->ibi_pool); @@ -340,6 +341,13 @@ static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master, slot->len = 0; buf = slot->data; + ret = readl_relaxed_poll_timeout(master->regs + SVC_I3C_MSTATUS, val, + SVC_I3C_MSTATUS_COMPLETE(val), 0, 1000); + if (ret) { + dev_err(master->dev, "Timeout when polling for COMPLETE\n"); + return ret; + } + while (SVC_I3C_MSTATUS_RXPEND(readl(master->regs + SVC_I3C_MSTATUS)) && slot->len < SVC_I3C_FIFO_SIZE) { mdatactrl = readl(master->regs + SVC_I3C_MDATACTRL);
MSTATUS[RXPEND] is only updated after the data transfer cycle started. This creates an issue when the I3C clock is slow, and the CPU is running fast enough that MSTATUS[RXPEND] may not be updated when the code reach checking point. As a result, mandatory data are being missed. Add a wait for MSTATUS[COMPLETE] to ensure that all mandatory data already in FIFO. Fixes: dd3c52846d59 ("i3c: master: svc: Add Silvaco I3C master driver") Cc: stable@vger.kernel.org Signed-off-by: Frank Li <Frank.Li@nxp.com> --- drivers/i3c/master/svc-i3c-master.c | 8 ++++++++ 1 file changed, 8 insertions(+)