| Message ID | 20240819-i3c_fix-v3-10-7d69f7b0a05e@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i3c: master: some fix and improvemnt for hotjoin | expand |
Hi Frank, Frank.Li@nxp.com wrote on Mon, 19 Aug 2024 12:02:04 -0400: > Wait for the controller to complete emitting ACK/NACK, otherwise the next > command may be omitted by the hardware. > > Add command done check at svc_i3c_master_nack(ack)_ibi() and change return a "command done" check in the reutnr type > type to int to indicate wait done timeout. flag possible timeouts. > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- > drivers/i3c/master/svc-i3c-master.c | 31 +++++++++++++++++++++++++------ > 1 file changed, 25 insertions(+), 6 deletions(-) > > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c > index fbb6cef405577..2010495906eb3 100644 > --- a/drivers/i3c/master/svc-i3c-master.c > +++ b/drivers/i3c/master/svc-i3c-master.c > @@ -384,10 +384,12 @@ static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master, > return 0; > } > > -static void svc_i3c_master_ack_ibi(struct svc_i3c_master *master, > +static int svc_i3c_master_ack_ibi(struct svc_i3c_master *master, > bool mandatory_byte) > { > unsigned int ibi_ack_nack; > + int ret; > + u32 reg; > > ibi_ack_nack = SVC_I3C_MCTRL_REQUEST_IBI_ACKNACK; > if (mandatory_byte) > @@ -396,18 +398,31 @@ static void svc_i3c_master_ack_ibi(struct svc_i3c_master *master, > ibi_ack_nack |= SVC_I3C_MCTRL_IBIRESP_ACK_WITHOUT_BYTE; > > writel(ibi_ack_nack, master->regs + SVC_I3C_MCTRL); > + > + ret = readl_poll_timeout_atomic(master->regs + SVC_I3C_MSTATUS, reg, > + SVC_I3C_MSTATUS_MCTRLDONE(reg), 1, 1000); Still concerned about the _atomic. > + return ret; return readl... > + > } Otherwise LGTM Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com> Thanks, Miquèl
On Fri, Aug 23, 2024 at 06:22:40PM +0200, Miquel Raynal wrote: > Hi Frank, > > Frank.Li@nxp.com wrote on Mon, 19 Aug 2024 12:02:04 -0400: > > > Wait for the controller to complete emitting ACK/NACK, otherwise the next > > command may be omitted by the hardware. > > > > Add command done check at svc_i3c_master_nack(ack)_ibi() and change return > > a "command done" check in > > the reutnr type > > > type to int to indicate wait done timeout. > > flag possible timeouts. > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > --- > > drivers/i3c/master/svc-i3c-master.c | 31 +++++++++++++++++++++++++------ > > 1 file changed, 25 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c > > index fbb6cef405577..2010495906eb3 100644 > > --- a/drivers/i3c/master/svc-i3c-master.c > > +++ b/drivers/i3c/master/svc-i3c-master.c > > @@ -384,10 +384,12 @@ static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master, > > return 0; > > } > > > > -static void svc_i3c_master_ack_ibi(struct svc_i3c_master *master, > > +static int svc_i3c_master_ack_ibi(struct svc_i3c_master *master, > > bool mandatory_byte) > > { > > unsigned int ibi_ack_nack; > > + int ret; > > + u32 reg; > > > > ibi_ack_nack = SVC_I3C_MCTRL_REQUEST_IBI_ACKNACK; > > if (mandatory_byte) > > @@ -396,18 +398,31 @@ static void svc_i3c_master_ack_ibi(struct svc_i3c_master *master, > > ibi_ack_nack |= SVC_I3C_MCTRL_IBIRESP_ACK_WITHOUT_BYTE; > > > > writel(ibi_ack_nack, master->regs + SVC_I3C_MCTRL); > > + > > + ret = readl_poll_timeout_atomic(master->regs + SVC_I3C_MSTATUS, reg, > > + SVC_I3C_MSTATUS_MCTRLDONE(reg), 1, 1000); > > Still concerned about the _atomic. It may be call in atomic context. Because hardware design limition, i3c transact have been in irq disable context to avoid 100us timeout issue. Frank > > > + return ret; > > return readl... > > > + > > } > > Otherwise LGTM > > Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com> > > Thanks, > Miquèl
diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c index fbb6cef405577..2010495906eb3 100644 --- a/drivers/i3c/master/svc-i3c-master.c +++ b/drivers/i3c/master/svc-i3c-master.c @@ -384,10 +384,12 @@ static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master, return 0; } -static void svc_i3c_master_ack_ibi(struct svc_i3c_master *master, +static int svc_i3c_master_ack_ibi(struct svc_i3c_master *master, bool mandatory_byte) { unsigned int ibi_ack_nack; + int ret; + u32 reg; ibi_ack_nack = SVC_I3C_MCTRL_REQUEST_IBI_ACKNACK; if (mandatory_byte) @@ -396,18 +398,31 @@ static void svc_i3c_master_ack_ibi(struct svc_i3c_master *master, ibi_ack_nack |= SVC_I3C_MCTRL_IBIRESP_ACK_WITHOUT_BYTE; writel(ibi_ack_nack, master->regs + SVC_I3C_MCTRL); + + ret = readl_poll_timeout_atomic(master->regs + SVC_I3C_MSTATUS, reg, + SVC_I3C_MSTATUS_MCTRLDONE(reg), 1, 1000); + return ret; + } -static void svc_i3c_master_nack_ibi(struct svc_i3c_master *master) +static int svc_i3c_master_nack_ibi(struct svc_i3c_master *master) { + int ret; + u32 reg; + writel(SVC_I3C_MCTRL_REQUEST_IBI_ACKNACK | SVC_I3C_MCTRL_IBIRESP_NACK, master->regs + SVC_I3C_MCTRL); + + ret = readl_poll_timeout_atomic(master->regs + SVC_I3C_MSTATUS, reg, + SVC_I3C_MSTATUS_MCTRLDONE(reg), 1, 1000); + return ret; } static int svc_i3c_master_handle_ibi_won(struct svc_i3c_master *master, u32 mstatus) { u32 ibitype; + int ret = 0; ibitype = SVC_I3C_MSTATUS_IBITYPE(mstatus); @@ -417,10 +432,10 @@ static int svc_i3c_master_handle_ibi_won(struct svc_i3c_master *master, u32 msta switch (ibitype) { case SVC_I3C_MSTATUS_IBITYPE_HOT_JOIN: case SVC_I3C_MSTATUS_IBITYPE_MASTER_REQUEST: - svc_i3c_master_nack_ibi(master); + ret = svc_i3c_master_nack_ibi(master); } - return 0; + return ret; } static void svc_i3c_master_ibi_work(struct work_struct *work) @@ -871,7 +886,9 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master, if (ret) break; } else if (SVC_I3C_MSTATUS_IBIWON(reg)) { - svc_i3c_master_handle_ibi_won(master, reg); + ret = svc_i3c_master_handle_ibi_won(master, reg); + if (ret) + break; continue; } else if (SVC_I3C_MSTATUS_MCTRLDONE(reg)) { if (SVC_I3C_MSTATUS_STATE_IDLE(reg) && @@ -1145,7 +1162,9 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master, * start. */ if (SVC_I3C_MSTATUS_IBIWON(reg)) { - svc_i3c_master_handle_ibi_won(master, reg); + ret = svc_i3c_master_handle_ibi_won(master, reg); + if (ret) + goto emit_stop; continue; }
Wait for the controller to complete emitting ACK/NACK, otherwise the next command may be omitted by the hardware. Add command done check at svc_i3c_master_nack(ack)_ibi() and change return type to int to indicate wait done timeout. Signed-off-by: Frank Li <Frank.Li@nxp.com> --- drivers/i3c/master/svc-i3c-master.c | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-)