* [PATCH v1] i3c: master: svc: Use manual response for IBI events @ 2025-08-19 9:08 Stanley Chu 2025-08-26 16:17 ` Frank Li 0 siblings, 1 reply; 4+ messages in thread From: Stanley Chu @ 2025-08-19 9:08 UTC (permalink / raw) To: frank.li, miquel.raynal, alexandre.belloni, linux-i3c Cc: linux-kernel, tomer.maimon, kwliu, yschu From: Stanley Chu <yschu@nuvoton.com> Using IBIRESP_AUTO causes the hardware to ACK IBI requests even when the target is not in the device list. The svc_i3c_master_nack_ibi() has no effect in such case. AutoIBI has another issue that the controller doesn't quit AutoIBI state after an IBIWON polling timeout. The following sequence is the case: 1. Target pulls SDA low 2. SLVSTART interrupt triggers the IBI ISR 3. Target releases SDA 4. Driver writes AutoIBI request to MCTRL 5. SDA is high, so AutoIBI process does not start 6. IBIWON polling times out 7. Controller state is AutoIBI and doesn't accept EmitStop request Emitting broadcast address with IBIRESP_MANUAL avoids both issues. Signed-off-by: Stanley Chu <yschu@nuvoton.com> --- drivers/i3c/master/svc-i3c-master.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c index 701ae165b25b..17644e041b44 100644 --- a/drivers/i3c/master/svc-i3c-master.c +++ b/drivers/i3c/master/svc-i3c-master.c @@ -517,9 +517,10 @@ static void svc_i3c_master_ibi_isr(struct svc_i3c_master *master) */ writel(SVC_I3C_MINT_IBIWON, master->regs + SVC_I3C_MSTATUS); - /* Acknowledge the incoming interrupt with the AUTOIBI mechanism */ - writel(SVC_I3C_MCTRL_REQUEST_AUTO_IBI | - SVC_I3C_MCTRL_IBIRESP_AUTO, + /* Emit broadcast address for arbitration */ + writel(SVC_I3C_MCTRL_REQUEST_START_ADDR | + SVC_I3C_MCTRL_IBIRESP_MANUAL | + SVC_I3C_MCTRL_ADDR(I3C_BROADCAST_ADDR), master->regs + SVC_I3C_MCTRL); /* Wait for IBIWON, should take approximately 100us */ @@ -539,10 +540,15 @@ static void svc_i3c_master_ibi_isr(struct svc_i3c_master *master) switch (ibitype) { case SVC_I3C_MSTATUS_IBITYPE_IBI: dev = svc_i3c_master_dev_from_addr(master, ibiaddr); - if (!dev || !is_events_enabled(master, SVC_I3C_EVENT_IBI)) + if (!dev || !is_events_enabled(master, SVC_I3C_EVENT_IBI)) { svc_i3c_master_nack_ibi(master); - else + } else { + if (dev->info.bcr & I3C_BCR_IBI_PAYLOAD) + svc_i3c_master_ack_ibi(master, true); + else + svc_i3c_master_ack_ibi(master, false); svc_i3c_master_handle_ibi(master, dev); + } break; case SVC_I3C_MSTATUS_IBITYPE_HOT_JOIN: if (is_events_enabled(master, SVC_I3C_EVENT_HOTJOIN)) -- 2.34.1 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1] i3c: master: svc: Use manual response for IBI events 2025-08-19 9:08 [PATCH v1] i3c: master: svc: Use manual response for IBI events Stanley Chu @ 2025-08-26 16:17 ` Frank Li 2025-08-27 0:54 ` Stanley Chu 0 siblings, 1 reply; 4+ messages in thread From: Frank Li @ 2025-08-26 16:17 UTC (permalink / raw) To: Stanley Chu Cc: miquel.raynal, alexandre.belloni, linux-i3c, linux-kernel, tomer.maimon, kwliu, yschu On Tue, Aug 19, 2025 at 05:08:31PM +0800, Stanley Chu wrote: > From: Stanley Chu <yschu@nuvoton.com> > > Using IBIRESP_AUTO causes the hardware to ACK IBI requests even when the > target is not in the device list. The svc_i3c_master_nack_ibi() has no > effect in such case. how this case can happen since default target's IBI is disabled. Need host send IBI enable command to target devices before target pull SDA lower. > > AutoIBI has another issue that the controller doesn't quit AutoIBI state > after an IBIWON polling timeout. The following sequence is the case: > 1. Target pulls SDA low > 2. SLVSTART interrupt triggers the IBI ISR > 3. Target releases SDA > 4. Driver writes AutoIBI request to MCTRL > 5. SDA is high, so AutoIBI process does not start > 6. IBIWON polling times out > 7. Controller state is AutoIBI and doesn't accept EmitStop request > > Emitting broadcast address with IBIRESP_MANUAL avoids both issues. > > Signed-off-by: Stanley Chu <yschu@nuvoton.com> > --- > drivers/i3c/master/svc-i3c-master.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c > index 701ae165b25b..17644e041b44 100644 > --- a/drivers/i3c/master/svc-i3c-master.c > +++ b/drivers/i3c/master/svc-i3c-master.c > @@ -517,9 +517,10 @@ static void svc_i3c_master_ibi_isr(struct svc_i3c_master *master) > */ > writel(SVC_I3C_MINT_IBIWON, master->regs + SVC_I3C_MSTATUS); > > - /* Acknowledge the incoming interrupt with the AUTOIBI mechanism */ > - writel(SVC_I3C_MCTRL_REQUEST_AUTO_IBI | > - SVC_I3C_MCTRL_IBIRESP_AUTO, > + /* Emit broadcast address for arbitration */ Need comments here why Request_AutoIBI not work. I think it is IP design defect to make REQUEST_AUTO_IBI useless. Only concern here is that svc_i3c_master_ack_ibi() need send in 100us by i3c spec requirement. Frank > + writel(SVC_I3C_MCTRL_REQUEST_START_ADDR | > + SVC_I3C_MCTRL_IBIRESP_MANUAL | > + SVC_I3C_MCTRL_ADDR(I3C_BROADCAST_ADDR), > master->regs + SVC_I3C_MCTRL); > > /* Wait for IBIWON, should take approximately 100us */ > @@ -539,10 +540,15 @@ static void svc_i3c_master_ibi_isr(struct svc_i3c_master *master) > switch (ibitype) { > case SVC_I3C_MSTATUS_IBITYPE_IBI: > dev = svc_i3c_master_dev_from_addr(master, ibiaddr); > - if (!dev || !is_events_enabled(master, SVC_I3C_EVENT_IBI)) > + if (!dev || !is_events_enabled(master, SVC_I3C_EVENT_IBI)) { > svc_i3c_master_nack_ibi(master); > - else > + } else { > + if (dev->info.bcr & I3C_BCR_IBI_PAYLOAD) > + svc_i3c_master_ack_ibi(master, true); > + else > + svc_i3c_master_ack_ibi(master, false); > svc_i3c_master_handle_ibi(master, dev); > + } > break; > case SVC_I3C_MSTATUS_IBITYPE_HOT_JOIN: > if (is_events_enabled(master, SVC_I3C_EVENT_HOTJOIN)) > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1] i3c: master: svc: Use manual response for IBI events 2025-08-26 16:17 ` Frank Li @ 2025-08-27 0:54 ` Stanley Chu 2025-08-27 16:16 ` Frank Li 0 siblings, 1 reply; 4+ messages in thread From: Stanley Chu @ 2025-08-27 0:54 UTC (permalink / raw) To: Frank Li Cc: miquel.raynal, alexandre.belloni, linux-i3c, linux-kernel, tomer.maimon, kwliu, yschu On Wed, Aug 27, 2025 at 12:17 AM Frank Li <Frank.li@nxp.com> wrote: > > On Tue, Aug 19, 2025 at 05:08:31PM +0800, Stanley Chu wrote: > > From: Stanley Chu <yschu@nuvoton.com> > > > > Using IBIRESP_AUTO causes the hardware to ACK IBI requests even when the > > target is not in the device list. The svc_i3c_master_nack_ibi() has no > > effect in such case. > > how this case can happen since default target's IBI is disabled. Need host > send IBI enable command to target devices before target pull SDA lower. > Hi Frank, This is to explain that the original code svc_i3c_master_nack_ibi() will never take effect when using AUTOIBI with IBIRESP_AUTO rule. /* Handle the critical responses to IBI's */ switch (ibitype) { case SVC_I3C_MSTATUS_IBITYPE_IBI: dev = svc_i3c_master_dev_from_addr(master, ibiaddr); if (!dev || !is_events_enabled(master, SVC_I3C_EVENT_IBI)) { svc_i3c_master_nack_ibi(master); } > > > > AutoIBI has another issue that the controller doesn't quit AutoIBI state > > after an IBIWON polling timeout. The following sequence is the case: > > 1. Target pulls SDA low > > 2. SLVSTART interrupt triggers the IBI ISR > > 3. Target releases SDA > > 4. Driver writes AutoIBI request to MCTRL > > 5. SDA is high, so AutoIBI process does not start > > 6. IBIWON polling times out > > 7. Controller state is AutoIBI and doesn't accept EmitStop request > > > > Emitting broadcast address with IBIRESP_MANUAL avoids both issues. > > > > Signed-off-by: Stanley Chu <yschu@nuvoton.com> > > --- > > drivers/i3c/master/svc-i3c-master.c | 16 +++++++++++----- > > 1 file changed, 11 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c > > index 701ae165b25b..17644e041b44 100644 > > --- a/drivers/i3c/master/svc-i3c-master.c > > +++ b/drivers/i3c/master/svc-i3c-master.c > > @@ -517,9 +517,10 @@ static void svc_i3c_master_ibi_isr(struct svc_i3c_master *master) > > */ > > writel(SVC_I3C_MINT_IBIWON, master->regs + SVC_I3C_MSTATUS); > > > > - /* Acknowledge the incoming interrupt with the AUTOIBI mechanism */ > > - writel(SVC_I3C_MCTRL_REQUEST_AUTO_IBI | > > - SVC_I3C_MCTRL_IBIRESP_AUTO, > > + /* Emit broadcast address for arbitration */ > > Need comments here why Request_AutoIBI not work. I think it is IP design > defect to make REQUEST_AUTO_IBI useless. It is not an IP defect. Per spec, AutoIBI emits a START with address 7'h7E when a slave pulls SDA low. A SDA glitch (high->low->high) can cause the controller to remain in AutoIBI state. 1. SDA high->low: trigger IBI isr to execute 2. SDA low->high 3. IBI isr writes an AutoIBI request, the controller will not emit a START because SDA is not low. 4. Finally IBIWON polling times out, the controller remains in AutoIBI state. > > Only concern here is that svc_i3c_master_ack_ibi() need send in 100us by > i3c spec requirement. The function svc_i3c_master_ack_ibi() executes promptly in the IRQ handler, taking much less than 100 us. --- Thanks. Stanley > > Frank > > > + writel(SVC_I3C_MCTRL_REQUEST_START_ADDR | > > + SVC_I3C_MCTRL_IBIRESP_MANUAL | > > + SVC_I3C_MCTRL_ADDR(I3C_BROADCAST_ADDR), > > master->regs + SVC_I3C_MCTRL); > > > > /* Wait for IBIWON, should take approximately 100us */ > > @@ -539,10 +540,15 @@ static void svc_i3c_master_ibi_isr(struct svc_i3c_master *master) > > switch (ibitype) { > > case SVC_I3C_MSTATUS_IBITYPE_IBI: > > dev = svc_i3c_master_dev_from_addr(master, ibiaddr); > > - if (!dev || !is_events_enabled(master, SVC_I3C_EVENT_IBI)) > > + if (!dev || !is_events_enabled(master, SVC_I3C_EVENT_IBI)) { > > svc_i3c_master_nack_ibi(master); > > - else > > + } else { > > + if (dev->info.bcr & I3C_BCR_IBI_PAYLOAD) > > + svc_i3c_master_ack_ibi(master, true); > > + else > > + svc_i3c_master_ack_ibi(master, false); > > svc_i3c_master_handle_ibi(master, dev); > > + } > > break; > > case SVC_I3C_MSTATUS_IBITYPE_HOT_JOIN: > > if (is_events_enabled(master, SVC_I3C_EVENT_HOTJOIN)) > > -- > > 2.34.1 > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1] i3c: master: svc: Use manual response for IBI events 2025-08-27 0:54 ` Stanley Chu @ 2025-08-27 16:16 ` Frank Li 0 siblings, 0 replies; 4+ messages in thread From: Frank Li @ 2025-08-27 16:16 UTC (permalink / raw) To: Stanley Chu Cc: miquel.raynal, alexandre.belloni, linux-i3c, linux-kernel, tomer.maimon, kwliu, yschu On Wed, Aug 27, 2025 at 08:54:03AM +0800, Stanley Chu wrote: > On Wed, Aug 27, 2025 at 12:17 AM Frank Li <Frank.li@nxp.com> wrote: > > > > On Tue, Aug 19, 2025 at 05:08:31PM +0800, Stanley Chu wrote: > > > From: Stanley Chu <yschu@nuvoton.com> > > > > > > Using IBIRESP_AUTO causes the hardware to ACK IBI requests even when the > > > target is not in the device list. The svc_i3c_master_nack_ibi() has no > > > effect in such case. > > > > how this case can happen since default target's IBI is disabled. Need host > > send IBI enable command to target devices before target pull SDA lower. > > > Hi Frank, > > This is to explain that the original code svc_i3c_master_nack_ibi() > will never take effect when using > AUTOIBI with IBIRESP_AUTO rule. > /* Handle the critical responses to IBI's */ > switch (ibitype) { > case SVC_I3C_MSTATUS_IBITYPE_IBI: > dev = svc_i3c_master_dev_from_addr(master, ibiaddr); > if (!dev || !is_events_enabled(master, SVC_I3C_EVENT_IBI)) { > svc_i3c_master_nack_ibi(master); > } Okay, i3c target may stay in IBI enable state if host reboot. what's bad thing happen if host ACK IBI? other IPs IBI auto handled by hardware, I think there are not complete logic in these IP. > > > > > > > AutoIBI has another issue that the controller doesn't quit AutoIBI state > > > after an IBIWON polling timeout. The following sequence is the case: > > > 1. Target pulls SDA low > > > 2. SLVSTART interrupt triggers the IBI ISR > > > 3. Target releases SDA > > > 4. Driver writes AutoIBI request to MCTRL > > > 5. SDA is high, so AutoIBI process does not start > > > 6. IBIWON polling times out > > > 7. Controller state is AutoIBI and doesn't accept EmitStop request > > > > > > Emitting broadcast address with IBIRESP_MANUAL avoids both issues. > > > > > > Signed-off-by: Stanley Chu <yschu@nuvoton.com> > > > --- > > > drivers/i3c/master/svc-i3c-master.c | 16 +++++++++++----- > > > 1 file changed, 11 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c > > > index 701ae165b25b..17644e041b44 100644 > > > --- a/drivers/i3c/master/svc-i3c-master.c > > > +++ b/drivers/i3c/master/svc-i3c-master.c > > > @@ -517,9 +517,10 @@ static void svc_i3c_master_ibi_isr(struct svc_i3c_master *master) > > > */ > > > writel(SVC_I3C_MINT_IBIWON, master->regs + SVC_I3C_MSTATUS); > > > > > > - /* Acknowledge the incoming interrupt with the AUTOIBI mechanism */ > > > - writel(SVC_I3C_MCTRL_REQUEST_AUTO_IBI | > > > - SVC_I3C_MCTRL_IBIRESP_AUTO, > > > + /* Emit broadcast address for arbitration */ > > > > Need comments here why Request_AutoIBI not work. I think it is IP design > > defect to make REQUEST_AUTO_IBI useless. > It is not an IP defect. Per spec, AutoIBI emits a START with address > 7'h7E when a slave pulls SDA low. > A SDA glitch (high->low->high) can cause the controller to remain in > AutoIBI state. > 1. SDA high->low: trigger IBI isr to execute > 2. SDA low->high > 3. IBI isr writes an AutoIBI request, the controller will not emit a > START because SDA is not low. This is overall design problem, not IP implement problem. AUTO_IBI wait SDA low is wrong. Normally case, target may release SDA after pull down SDA low for some times to avoid lock bus. after your patch, you will find no place will not use SVC_I3C_MCTRL_REQUEST_AUTO_IBI at all. So SVC_I3C_MCTRL_REQUEST_AUTO_IBI became useless. Add comments here why not use SVC_I3C_MCTRL_REQUEST_AUTO_IBI to help developer know this problem because SVC_I3C_MCTRL_REQUEST_AUTO_IBI look like should be used in here from IP spec. > 4. Finally IBIWON polling times out, the controller remains in AutoIBI state. > > > > > Only concern here is that svc_i3c_master_ack_ibi() need send in 100us by > > i3c spec requirement. > The function svc_i3c_master_ack_ibi() executes promptly in the IRQ > handler, taking much less than 100 us. Overall I am agree on what your change, but clearify some situations. Manual handle IBI is not good idea overall. Core may run some EL2/EL3 codes. for example, improve SVC_I3C_IBIRULES_ADDR and let SVC_I3C_MCTRL_IBIRESP_AUTO to check if target address is known list. Thank you find this problem and provide solution. I think just add comments in /* Emit broadcast address for arbitration */ to show why have to use SVC_I3C_MCTRL_REQUEST_START_ADDR. Frank > > --- > Thanks. > Stanley > > > > Frank > > > > > + writel(SVC_I3C_MCTRL_REQUEST_START_ADDR | > > > + SVC_I3C_MCTRL_IBIRESP_MANUAL | > > > + SVC_I3C_MCTRL_ADDR(I3C_BROADCAST_ADDR), > > > master->regs + SVC_I3C_MCTRL); > > > > > > /* Wait for IBIWON, should take approximately 100us */ > > > @@ -539,10 +540,15 @@ static void svc_i3c_master_ibi_isr(struct svc_i3c_master *master) > > > switch (ibitype) { > > > case SVC_I3C_MSTATUS_IBITYPE_IBI: > > > dev = svc_i3c_master_dev_from_addr(master, ibiaddr); > > > - if (!dev || !is_events_enabled(master, SVC_I3C_EVENT_IBI)) > > > + if (!dev || !is_events_enabled(master, SVC_I3C_EVENT_IBI)) { > > > svc_i3c_master_nack_ibi(master); > > > - else > > > + } else { > > > + if (dev->info.bcr & I3C_BCR_IBI_PAYLOAD) > > > + svc_i3c_master_ack_ibi(master, true); > > > + else > > > + svc_i3c_master_ack_ibi(master, false); > > > svc_i3c_master_handle_ibi(master, dev); > > > + } > > > break; > > > case SVC_I3C_MSTATUS_IBITYPE_HOT_JOIN: > > > if (is_events_enabled(master, SVC_I3C_EVENT_HOTJOIN)) > > > -- > > > 2.34.1 > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-08-27 16:16 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-08-19 9:08 [PATCH v1] i3c: master: svc: Use manual response for IBI events Stanley Chu 2025-08-26 16:17 ` Frank Li 2025-08-27 0:54 ` Stanley Chu 2025-08-27 16:16 ` Frank Li
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®