| Message ID | 20240819-i3c_fix-v3-9-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:03 -0400: > According to I3C spec ver 1.1, 09-Jun-2021, section 5.1.2.5: > > The I3C Controller shall hold SCL Low while the Bus is in I3C/I2C Transfer, low transfer > ACK/NACK Phase. But maximum stall time is 100us. We have to disable irq and and/or (I'm not sure) the IRQs > schedule during whole I3C transaction, otherwise, I3C bus timeout will prevnet scheduling during the whole the may timeout. > happen if any irq or schedule happen during transaction. > > Replace mutex with spinlock_saveirq() to make sure finish whole i3c wrong name to avoid stalling SCL... > transaction without stall SCL more than 100us. > > Signed-off-by: Frank Li <Frank.Li@nxp.com> Yes, 100us is low, and that's why I initially did my best to enforce auto ack/nack. We cannot make sure this limit will not be crossed, and when it's the case, we need to handle the consequences. > --- > drivers/i3c/master/svc-i3c-master.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c > index 161ccd824443b..fbb6cef405577 100644 > --- a/drivers/i3c/master/svc-i3c-master.c > +++ b/drivers/i3c/master/svc-i3c-master.c > @@ -432,7 +432,16 @@ static void svc_i3c_master_ibi_work(struct work_struct *work) > u32 status, val; > int ret; > > - mutex_lock(&master->lock); Don't you still need this lock for other concurrency reasons? > + /* > + * According to I3C spec ver 1.1, 09-Jun-2021, section 5.1.2.5: > + * > + * The I3C Controller shall hold SCL Low while the Bus is in I3C/I2C Transfer, ACK/NACK > + * Phase. But maximum stall time is 100us. We have to disable irq and schedule during whole > + * I3C transaction, otherwise, I3C bus timeout will happen if any irq or schedule happen > + * between transaction.. > + */ > + guard(spinlock_irqsave)(&master->xferqueue.lock); > + > /* > * IBIWON may be set before SVC_I3C_MCTRL_REQUEST_AUTO_IBI, causing > * readl_relaxed_poll_timeout() to return immediately. Consequently, > @@ -452,7 +461,7 @@ static void svc_i3c_master_ibi_work(struct work_struct *work) > master->regs + SVC_I3C_MCTRL); > > /* Wait for IBIWON, should take approximately 100us */ > - ret = readl_relaxed_poll_timeout(master->regs + SVC_I3C_MSTATUS, val, > + ret = readl_relaxed_poll_timeout_atomic(master->regs + SVC_I3C_MSTATUS, val, > SVC_I3C_MSTATUS_IBIWON(val), 0, 1000); This means you lock one CPU for 100us doing nothing every time you send a frame, that's not possible. Actually the delay was already very small (could have been set to ~10 maybe) but this is not possible. > if (ret) { > dev_err(master->dev, "Timeout when polling for IBIWON\n"); > @@ -525,7 +534,6 @@ static void svc_i3c_master_ibi_work(struct work_struct *work) > > reenable_ibis: > svc_i3c_master_enable_interrupts(master, SVC_I3C_MINT_SLVSTART); > - mutex_unlock(&master->lock); > } > > static irqreturn_t svc_i3c_master_irq_handler(int irq, void *dev_id) > Thanks, Miquèl
On Fri, Aug 23, 2024 at 06:19:27PM +0200, Miquel Raynal wrote: > Hi Frank, > > Frank.Li@nxp.com wrote on Mon, 19 Aug 2024 12:02:03 -0400: > > > According to I3C spec ver 1.1, 09-Jun-2021, section 5.1.2.5: > > > > The I3C Controller shall hold SCL Low while the Bus is in I3C/I2C Transfer, > > low transfer > > > ACK/NACK Phase. But maximum stall time is 100us. We have to disable irq and > > and/or (I'm not sure) the IRQs > > > schedule during whole I3C transaction, otherwise, I3C bus timeout will > > prevnet scheduling during the whole the may > timeout. > > > happen if any irq or schedule happen during transaction. > > > > Replace mutex with spinlock_saveirq() to make sure finish whole i3c > > wrong name to avoid stalling SCL... > > > transaction without stall SCL more than 100us. > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > Yes, 100us is low, and that's why I initially did my best to enforce > auto ack/nack. We cannot make sure this limit will not be crossed, and > when it's the case, we need to handle the consequences. Only IBI use auto ack/nack. hardware can't handle it for HJ and CR, which have to manually send out. > > > --- > > drivers/i3c/master/svc-i3c-master.c | 14 +++++++++++--- > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c > > index 161ccd824443b..fbb6cef405577 100644 > > --- a/drivers/i3c/master/svc-i3c-master.c > > +++ b/drivers/i3c/master/svc-i3c-master.c > > @@ -432,7 +432,16 @@ static void svc_i3c_master_ibi_work(struct work_struct *work) > > u32 status, val; > > int ret; > > > > - mutex_lock(&master->lock); > > Don't you still need this lock for other concurrency reasons? > > > + /* > > + * According to I3C spec ver 1.1, 09-Jun-2021, section 5.1.2.5: > > + * > > + * The I3C Controller shall hold SCL Low while the Bus is in I3C/I2C Transfer, ACK/NACK > > + * Phase. But maximum stall time is 100us. We have to disable irq and schedule during whole > > + * I3C transaction, otherwise, I3C bus timeout will happen if any irq or schedule happen > > + * between transaction.. > > + */ > > + guard(spinlock_irqsave)(&master->xferqueue.lock); > > + > > /* > > * IBIWON may be set before SVC_I3C_MCTRL_REQUEST_AUTO_IBI, causing > > * readl_relaxed_poll_timeout() to return immediately. Consequently, > > @@ -452,7 +461,7 @@ static void svc_i3c_master_ibi_work(struct work_struct *work) > > master->regs + SVC_I3C_MCTRL); > > > > /* Wait for IBIWON, should take approximately 100us */ > > - ret = readl_relaxed_poll_timeout(master->regs + SVC_I3C_MSTATUS, val, > > + ret = readl_relaxed_poll_timeout_atomic(master->regs + SVC_I3C_MSTATUS, val, > > SVC_I3C_MSTATUS_IBIWON(val), 0, 1000); > > This means you lock one CPU for 100us doing nothing every time you send > a frame, that's not possible. Actually the delay was already very small > (could have been set to ~10 maybe) but this is not possible. It happen only at error case. Most time should wait for only 9th SCL. I think original comment is wrong. Hardware set IBIWON at 8th SCL. Frank > > > if (ret) { > > dev_err(master->dev, "Timeout when polling for IBIWON\n"); > > @@ -525,7 +534,6 @@ static void svc_i3c_master_ibi_work(struct work_struct *work) > > > > reenable_ibis: > > svc_i3c_master_enable_interrupts(master, SVC_I3C_MINT_SLVSTART); > > - mutex_unlock(&master->lock); > > } > > > > static irqreturn_t svc_i3c_master_irq_handler(int irq, void *dev_id) > > > > > Thanks, > Miquèl
diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c index 161ccd824443b..fbb6cef405577 100644 --- a/drivers/i3c/master/svc-i3c-master.c +++ b/drivers/i3c/master/svc-i3c-master.c @@ -432,7 +432,16 @@ static void svc_i3c_master_ibi_work(struct work_struct *work) u32 status, val; int ret; - mutex_lock(&master->lock); + /* + * According to I3C spec ver 1.1, 09-Jun-2021, section 5.1.2.5: + * + * The I3C Controller shall hold SCL Low while the Bus is in I3C/I2C Transfer, ACK/NACK + * Phase. But maximum stall time is 100us. We have to disable irq and schedule during whole + * I3C transaction, otherwise, I3C bus timeout will happen if any irq or schedule happen + * between transaction.. + */ + guard(spinlock_irqsave)(&master->xferqueue.lock); + /* * IBIWON may be set before SVC_I3C_MCTRL_REQUEST_AUTO_IBI, causing * readl_relaxed_poll_timeout() to return immediately. Consequently, @@ -452,7 +461,7 @@ static void svc_i3c_master_ibi_work(struct work_struct *work) master->regs + SVC_I3C_MCTRL); /* Wait for IBIWON, should take approximately 100us */ - ret = readl_relaxed_poll_timeout(master->regs + SVC_I3C_MSTATUS, val, + ret = readl_relaxed_poll_timeout_atomic(master->regs + SVC_I3C_MSTATUS, val, SVC_I3C_MSTATUS_IBIWON(val), 0, 1000); if (ret) { dev_err(master->dev, "Timeout when polling for IBIWON\n"); @@ -525,7 +534,6 @@ static void svc_i3c_master_ibi_work(struct work_struct *work) reenable_ibis: svc_i3c_master_enable_interrupts(master, SVC_I3C_MINT_SLVSTART); - mutex_unlock(&master->lock); } static irqreturn_t svc_i3c_master_irq_handler(int irq, void *dev_id)
According to I3C spec ver 1.1, 09-Jun-2021, section 5.1.2.5: The I3C Controller shall hold SCL Low while the Bus is in I3C/I2C Transfer, ACK/NACK Phase. But maximum stall time is 100us. We have to disable irq and schedule during whole I3C transaction, otherwise, I3C bus timeout will happen if any irq or schedule happen during transaction. Replace mutex with spinlock_saveirq() to make sure finish whole i3c transaction without stall SCL more than 100us. Signed-off-by: Frank Li <Frank.Li@nxp.com> --- drivers/i3c/master/svc-i3c-master.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)