| Message ID | 20241001-svc-i3c-hj-v5-4-480ab8aed849@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | I3C: master: svc: collect all patches to improve hotjoin stability | expand |
Hi Frank, Frank.Li@nxp.com wrote on Tue, 01 Oct 2024 12:02:53 -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 ACK/NACK Phase of > I3C/I2C transfer. But maximum stall time is 100us. The IRQs have to be > disabled to prevent schedule during the whole I3C transaction, otherwise, > the I3C bus timeout may happen if any irq or schedule happen during > transaction. > > Replace mutex with spin_lock_irqsave() to avoid stalling SCL more than > 100us. > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- > Change from v3-v4 > - improve commit message > - needn't mutex here, other place already use spin_lock_saveirq to protent > i3c transfer. > --- > 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 5df0ec02d73ce..1ee6ce186195c 100644 > --- a/drivers/i3c/master/svc-i3c-master.c > +++ b/drivers/i3c/master/svc-i3c-master.c > @@ -436,7 +436,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 ACK/NACK Phase of I3C/I2C > + * transfer. But maximum stall time is 100us. The IRQs have to be disabled to prevent > + * schedule during the whole I3C transaction, otherwise, the I3C bus timeout may happen if > + * any irq or schedule happen during 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, > @@ -456,7 +465,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 we now are holding a spinlock and expect this to happen within 100us, then I guess the timeout should be reduced? > if (ret) { > dev_err(master->dev, "Timeout when polling for IBIWON\n"); > @@ -529,7 +538,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) > Otherwise, Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com> Thanks, Miquèl
On Tue, Oct 01, 2024 at 10:15:09PM +0200, Miquel Raynal wrote: > Hi Frank, > > Frank.Li@nxp.com wrote on Tue, 01 Oct 2024 12:02:53 -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 ACK/NACK Phase of > > I3C/I2C transfer. But maximum stall time is 100us. The IRQs have to be > > disabled to prevent schedule during the whole I3C transaction, otherwise, > > the I3C bus timeout may happen if any irq or schedule happen during > > transaction. > > > > Replace mutex with spin_lock_irqsave() to avoid stalling SCL more than > > 100us. > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > --- > > Change from v3-v4 > > - improve commit message > > - needn't mutex here, other place already use spin_lock_saveirq to protent > > i3c transfer. > > --- > > 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 5df0ec02d73ce..1ee6ce186195c 100644 > > --- a/drivers/i3c/master/svc-i3c-master.c > > +++ b/drivers/i3c/master/svc-i3c-master.c > > @@ -436,7 +436,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 ACK/NACK Phase of I3C/I2C > > + * transfer. But maximum stall time is 100us. The IRQs have to be disabled to prevent > > + * schedule during the whole I3C transaction, otherwise, the I3C bus timeout may happen if > > + * any irq or schedule happen during 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, > > @@ -456,7 +465,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 we now are holding a spinlock and expect this to happen within > 100us, then I guess the timeout should be reduced? yes, 100 should be enough for timeout. Normal it should be set at 9th SCL. Frank > > > if (ret) { > > dev_err(master->dev, "Timeout when polling for IBIWON\n"); > > @@ -529,7 +538,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) > > > > Otherwise, > > Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > Thanks, > Miquèl > > -- > linux-i3c mailing list > linux-i3c@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-i3c
diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c index 5df0ec02d73ce..1ee6ce186195c 100644 --- a/drivers/i3c/master/svc-i3c-master.c +++ b/drivers/i3c/master/svc-i3c-master.c @@ -436,7 +436,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 ACK/NACK Phase of I3C/I2C + * transfer. But maximum stall time is 100us. The IRQs have to be disabled to prevent + * schedule during the whole I3C transaction, otherwise, the I3C bus timeout may happen if + * any irq or schedule happen during 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, @@ -456,7 +465,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"); @@ -529,7 +538,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 ACK/NACK Phase of I3C/I2C transfer. But maximum stall time is 100us. The IRQs have to be disabled to prevent schedule during the whole I3C transaction, otherwise, the I3C bus timeout may happen if any irq or schedule happen during transaction. Replace mutex with spin_lock_irqsave() to avoid stalling SCL more than 100us. Signed-off-by: Frank Li <Frank.Li@nxp.com> --- Change from v3-v4 - improve commit message - needn't mutex here, other place already use spin_lock_saveirq to protent i3c transfer. --- drivers/i3c/master/svc-i3c-master.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)