[PATCH v1] i3c: master: svc: Use manual response for IBI events

[PATCH v1] i3c: master: svc: Use manual response for IBI events

[Stanley Chu]

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

Re: [PATCH v1] i3c: master: svc: Use manual response for IBI events

[Frank Li]

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
>

Re: [PATCH v1] i3c: master: svc: Use manual response for IBI events

[Stanley Chu]

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
> >

Re: [PATCH v1] i3c: master: svc: Use manual response for IBI events

[Frank Li]

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
> > >