[PATCH v1] i3c: master: svc: Fix npcm845 FIFO_EMPTY quirk

[PATCH v1] i3c: master: svc: Fix npcm845 FIFO_EMPTY quirk

[Stanley Chu]

From: Stanley Chu <yschu@nuvoton.com>

Prefilling in private write transfers is only necessary when the FIFO
is empty. Otherwise, if the transfer is NACKed due to IBIWON and retries
continue, data may be prefilled again but could be lost because the FIFO
is not empty.

Fixes: 4008a74e0f9b ("i3c: master: svc: Fix npcm845 FIFO empty issue")
Signed-off-by: Stanley Chu <yschu@nuvoton.com>
---
 drivers/i3c/master/svc-i3c-master.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 7e1a7cb94b43..34b6e125b18a 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -104,6 +104,7 @@
 #define   SVC_I3C_MDATACTRL_TXTRIG_FIFO_NOT_FULL GENMASK(5, 4)
 #define   SVC_I3C_MDATACTRL_RXTRIG_FIFO_NOT_EMPTY 0
 #define   SVC_I3C_MDATACTRL_RXCOUNT(x) FIELD_GET(GENMASK(28, 24), (x))
+#define   SVC_I3C_MDATACTRL_TXCOUNT(x) FIELD_GET(GENMASK(20, 16), (x))
 #define   SVC_I3C_MDATACTRL_TXFULL BIT(30)
 #define   SVC_I3C_MDATACTRL_RXEMPTY BIT(31)
 
@@ -280,6 +281,13 @@ static inline bool is_events_enabled(struct svc_i3c_master *master, u32 mask)
 	return !!(master->enabled_events & mask);
 }
 
+static inline bool svc_i3c_master_tx_empty(struct svc_i3c_master *master)
+{
+	u32 reg = readl(master->regs + SVC_I3C_MDATACTRL);
+
+	return (SVC_I3C_MDATACTRL_TXCOUNT(reg) == 0);
+}
+
 static bool svc_i3c_master_error(struct svc_i3c_master *master)
 {
 	u32 mstatus, merrwarn;
@@ -1303,7 +1311,8 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 		 * The only way to work around this hardware issue is to let the
 		 * FIFO start filling as soon as possible after EmitStartAddr.
 		 */
-		if (svc_has_quirk(master, SVC_I3C_QUIRK_FIFO_EMPTY) && !rnw && xfer_len) {
+		if (svc_has_quirk(master, SVC_I3C_QUIRK_FIFO_EMPTY) && !rnw && xfer_len &&
+		    svc_i3c_master_tx_empty(master)) {
 			u32 end = xfer_len > SVC_I3C_FIFO_SIZE ? 0 : SVC_I3C_MWDATAB_END;
 			u32 len = min_t(u32, xfer_len, SVC_I3C_FIFO_SIZE);
 
-- 
2.34.1


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v1] i3c: master: svc: Fix npcm845 FIFO_EMPTY quirk

[Frank Li]

On Fri, Jul 25, 2025 at 03:03:10PM +0800, Stanley Chu wrote:
> From: Stanley Chu <yschu@nuvoton.com>
>
> Prefilling in private write transfers is only necessary when the FIFO
> is empty. Otherwise, if the transfer is NACKed due to IBIWON and retries
> continue, data may be prefilled again but could be lost because the FIFO
> is not empty.

why "maybe prefilled", please use certain words.

>
> Fixes: 4008a74e0f9b ("i3c: master: svc: Fix npcm845 FIFO empty issue")
> Signed-off-by: Stanley Chu <yschu@nuvoton.com>
> ---
>  drivers/i3c/master/svc-i3c-master.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index 7e1a7cb94b43..34b6e125b18a 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -104,6 +104,7 @@
>  #define   SVC_I3C_MDATACTRL_TXTRIG_FIFO_NOT_FULL GENMASK(5, 4)
>  #define   SVC_I3C_MDATACTRL_RXTRIG_FIFO_NOT_EMPTY 0
>  #define   SVC_I3C_MDATACTRL_RXCOUNT(x) FIELD_GET(GENMASK(28, 24), (x))
> +#define   SVC_I3C_MDATACTRL_TXCOUNT(x) FIELD_GET(GENMASK(20, 16), (x))
>  #define   SVC_I3C_MDATACTRL_TXFULL BIT(30)
>  #define   SVC_I3C_MDATACTRL_RXEMPTY BIT(31)
>
> @@ -280,6 +281,13 @@ static inline bool is_events_enabled(struct svc_i3c_master *master, u32 mask)
>  	return !!(master->enabled_events & mask);
>  }
>
> +static inline bool svc_i3c_master_tx_empty(struct svc_i3c_master *master)
> +{
> +	u32 reg = readl(master->regs + SVC_I3C_MDATACTRL);
> +
> +	return (SVC_I3C_MDATACTRL_TXCOUNT(reg) == 0);
> +}
> +
>  static bool svc_i3c_master_error(struct svc_i3c_master *master)
>  {
>  	u32 mstatus, merrwarn;
> @@ -1303,7 +1311,8 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
>  		 * The only way to work around this hardware issue is to let the
>  		 * FIFO start filling as soon as possible after EmitStartAddr.
>  		 */
> -		if (svc_has_quirk(master, SVC_I3C_QUIRK_FIFO_EMPTY) && !rnw && xfer_len) {
> +		if (svc_has_quirk(master, SVC_I3C_QUIRK_FIFO_EMPTY) && !rnw && xfer_len &&
> +		    svc_i3c_master_tx_empty(master)) {
>  			u32 end = xfer_len > SVC_I3C_FIFO_SIZE ? 0 : SVC_I3C_MWDATAB_END;
>  			u32 len = min_t(u32, xfer_len, SVC_I3C_FIFO_SIZE);

if prefill to FIFO SIZE each time, such as

replace SVC_I3C_FIFO_SIZE with

SVC_I3C_FIFO_SIZE - SVC_I3C_MDATACTRL_TXCOUNT(readl(master->regs + SVC_I3C_MDATACTRL)).

Frank
>
> --
> 2.34.1
>

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v1] i3c: master: svc: Fix npcm845 FIFO_EMPTY quirk

[Stanley Chu]

Hi Frank,

On Fri, Jul 25, 2025 at 9:45 PM Frank Li <Frank.li@nxp.com> wrote:
>
> On Fri, Jul 25, 2025 at 03:03:10PM +0800, Stanley Chu wrote:
> > From: Stanley Chu <yschu@nuvoton.com>
> >
> > Prefilling in private write transfers is only necessary when the FIFO
> > is empty. Otherwise, if the transfer is NACKed due to IBIWON and retries
> > continue, data may be prefilled again but could be lost because the FIFO
> > is not empty.
>
> why "maybe prefilled", please use certain words.
The original code didn't consider the retry case, data lost happen in
the retry case only when total write bytes is larger than FIFO size,
not always happen.
In the example of writing 17 bytes (FIFO size is 16), the last byte is lost.
1. Emit S+Addr/W
2. Write 16 bytes to FIFO (FIFO is full)
3. NACKed due to IBIWON
4. (retry) Emit Sr+Addr/W
5. Write last byte to FIFO (data lost)

I just need to prefill FIFO in the beginning to make sure FIFO is not
empty when HW starts transmitting.
So, this patch adds a condition that prefill FIFO only when FIFO is empty.

>
> >
> > Fixes: 4008a74e0f9b ("i3c: master: svc: Fix npcm845 FIFO empty issue")
> > Signed-off-by: Stanley Chu <yschu@nuvoton.com>
> > ---
> >  drivers/i3c/master/svc-i3c-master.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> > index 7e1a7cb94b43..34b6e125b18a 100644
> > --- a/drivers/i3c/master/svc-i3c-master.c
> > +++ b/drivers/i3c/master/svc-i3c-master.c
> > @@ -104,6 +104,7 @@
> >  #define   SVC_I3C_MDATACTRL_TXTRIG_FIFO_NOT_FULL GENMASK(5, 4)
> >  #define   SVC_I3C_MDATACTRL_RXTRIG_FIFO_NOT_EMPTY 0
> >  #define   SVC_I3C_MDATACTRL_RXCOUNT(x) FIELD_GET(GENMASK(28, 24), (x))
> > +#define   SVC_I3C_MDATACTRL_TXCOUNT(x) FIELD_GET(GENMASK(20, 16), (x))
> >  #define   SVC_I3C_MDATACTRL_TXFULL BIT(30)
> >  #define   SVC_I3C_MDATACTRL_RXEMPTY BIT(31)
> >
> > @@ -280,6 +281,13 @@ static inline bool is_events_enabled(struct svc_i3c_master *master, u32 mask)
> >       return !!(master->enabled_events & mask);
> >  }
> >
> > +static inline bool svc_i3c_master_tx_empty(struct svc_i3c_master *master)
> > +{
> > +     u32 reg = readl(master->regs + SVC_I3C_MDATACTRL);
> > +
> > +     return (SVC_I3C_MDATACTRL_TXCOUNT(reg) == 0);
> > +}
> > +
> >  static bool svc_i3c_master_error(struct svc_i3c_master *master)
> >  {
> >       u32 mstatus, merrwarn;
> > @@ -1303,7 +1311,8 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> >                * The only way to work around this hardware issue is to let the
> >                * FIFO start filling as soon as possible after EmitStartAddr.
> >                */
> > -             if (svc_has_quirk(master, SVC_I3C_QUIRK_FIFO_EMPTY) && !rnw && xfer_len) {
> > +             if (svc_has_quirk(master, SVC_I3C_QUIRK_FIFO_EMPTY) && !rnw && xfer_len &&
> > +                 svc_i3c_master_tx_empty(master)) {
> >                       u32 end = xfer_len > SVC_I3C_FIFO_SIZE ? 0 : SVC_I3C_MWDATAB_END;
> >                       u32 len = min_t(u32, xfer_len, SVC_I3C_FIFO_SIZE);
>
> if prefill to FIFO SIZE each time, such as
>
> replace SVC_I3C_FIFO_SIZE with
>
> SVC_I3C_FIFO_SIZE - SVC_I3C_MDATACTRL_TXCOUNT(readl(master->regs + SVC_I3C_MDATACTRL)).
>
The free space is SVC_I3C_FIFO_SIZE when FIFO is empty, no need to
check TX count again.

--
Stanley

> Frank
> >
> > --
> > 2.34.1
> >

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v1] i3c: master: svc: Fix npcm845 FIFO_EMPTY quirk

[Frank Li]

On Fri, Jul 25, 2025 at 10:50:34PM +0800, Stanley Chu wrote:
> Hi Frank,
>
> On Fri, Jul 25, 2025 at 9:45 PM Frank Li <Frank.li@nxp.com> wrote:
> >
> > On Fri, Jul 25, 2025 at 03:03:10PM +0800, Stanley Chu wrote:
> > > From: Stanley Chu <yschu@nuvoton.com>
> > >
> > > Prefilling in private write transfers is only necessary when the FIFO
> > > is empty. Otherwise, if the transfer is NACKed due to IBIWON and retries
> > > continue, data may be prefilled again but could be lost because the FIFO
> > > is not empty.
> >
> > why "maybe prefilled", please use certain words.
> The original code didn't consider the retry case, data lost happen in
> the retry case only when total write bytes is larger than FIFO size,
> not always happen.
> In the example of writing 17 bytes (FIFO size is 16), the last byte is lost.
> 1. Emit S+Addr/W
> 2. Write 16 bytes to FIFO (FIFO is full)
> 3. NACKed due to IBIWON
> 4. (retry) Emit Sr+Addr/W
> 5. Write last byte to FIFO (data lost)

You need descript clearly at commit message about this.

>
> I just need to prefill FIFO in the beginning to make sure FIFO is not
> empty when HW starts transmitting.
> So, this patch adds a condition that prefill FIFO only when FIFO is empty.
>
> >
> > >
> > > Fixes: 4008a74e0f9b ("i3c: master: svc: Fix npcm845 FIFO empty issue")
> > > Signed-off-by: Stanley Chu <yschu@nuvoton.com>
> > > ---
> > >  drivers/i3c/master/svc-i3c-master.c | 11 ++++++++++-
> > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> > > index 7e1a7cb94b43..34b6e125b18a 100644
> > > --- a/drivers/i3c/master/svc-i3c-master.c
> > > +++ b/drivers/i3c/master/svc-i3c-master.c
> > > @@ -104,6 +104,7 @@
> > >  #define   SVC_I3C_MDATACTRL_TXTRIG_FIFO_NOT_FULL GENMASK(5, 4)
> > >  #define   SVC_I3C_MDATACTRL_RXTRIG_FIFO_NOT_EMPTY 0
> > >  #define   SVC_I3C_MDATACTRL_RXCOUNT(x) FIELD_GET(GENMASK(28, 24), (x))
> > > +#define   SVC_I3C_MDATACTRL_TXCOUNT(x) FIELD_GET(GENMASK(20, 16), (x))
> > >  #define   SVC_I3C_MDATACTRL_TXFULL BIT(30)
> > >  #define   SVC_I3C_MDATACTRL_RXEMPTY BIT(31)
> > >
> > > @@ -280,6 +281,13 @@ static inline bool is_events_enabled(struct svc_i3c_master *master, u32 mask)
> > >       return !!(master->enabled_events & mask);
> > >  }
> > >
> > > +static inline bool svc_i3c_master_tx_empty(struct svc_i3c_master *master)
> > > +{
> > > +     u32 reg = readl(master->regs + SVC_I3C_MDATACTRL);
> > > +
> > > +     return (SVC_I3C_MDATACTRL_TXCOUNT(reg) == 0);
> > > +}
> > > +
> > >  static bool svc_i3c_master_error(struct svc_i3c_master *master)
> > >  {
> > >       u32 mstatus, merrwarn;
> > > @@ -1303,7 +1311,8 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> > >                * The only way to work around this hardware issue is to let the
> > >                * FIFO start filling as soon as possible after EmitStartAddr.
> > >                */
> > > -             if (svc_has_quirk(master, SVC_I3C_QUIRK_FIFO_EMPTY) && !rnw && xfer_len) {
> > > +             if (svc_has_quirk(master, SVC_I3C_QUIRK_FIFO_EMPTY) && !rnw && xfer_len &&
> > > +                 svc_i3c_master_tx_empty(master)) {
> > >                       u32 end = xfer_len > SVC_I3C_FIFO_SIZE ? 0 : SVC_I3C_MWDATAB_END;
> > >                       u32 len = min_t(u32, xfer_len, SVC_I3C_FIFO_SIZE);
> >
> > if prefill to FIFO SIZE each time, such as
> >
> > replace SVC_I3C_FIFO_SIZE with
> >
> > SVC_I3C_FIFO_SIZE - SVC_I3C_MDATACTRL_TXCOUNT(readl(master->regs + SVC_I3C_MDATACTRL)).
> >
> The free space is SVC_I3C_FIFO_SIZE when FIFO is empty, no need to
> check TX count again.

My means is that if use above logic, needn't check helper function
svc_i3c_master_tx_empty(master).

Does this method work?

Frank

>
> --
> Stanley
>
> > Frank
> > >
> > > --
> > > 2.34.1
> > >
>
> --
> linux-i3c mailing list
> linux-i3c@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-i3c

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v1] i3c: master: svc: Fix npcm845 FIFO_EMPTY quirk

[Stanley Chu]

On Fri, Jul 25, 2025 at 11:13 PM Frank Li <Frank.li@nxp.com> wrote:
>
> On Fri, Jul 25, 2025 at 10:50:34PM +0800, Stanley Chu wrote:
> > Hi Frank,
> >
> > On Fri, Jul 25, 2025 at 9:45 PM Frank Li <Frank.li@nxp.com> wrote:
> > >
> > > On Fri, Jul 25, 2025 at 03:03:10PM +0800, Stanley Chu wrote:
> > > > From: Stanley Chu <yschu@nuvoton.com>
> > > >
> > > > Prefilling in private write transfers is only necessary when the FIFO
> > > > is empty. Otherwise, if the transfer is NACKed due to IBIWON and retries
> > > > continue, data may be prefilled again but could be lost because the FIFO
> > > > is not empty.
> > >
> > > why "maybe prefilled", please use certain words.
> > The original code didn't consider the retry case, data lost happen in
> > the retry case only when total write bytes is larger than FIFO size,
> > not always happen.
> > In the example of writing 17 bytes (FIFO size is 16), the last byte is lost.
> > 1. Emit S+Addr/W
> > 2. Write 16 bytes to FIFO (FIFO is full)
> > 3. NACKed due to IBIWON
> > 4. (retry) Emit Sr+Addr/W
> > 5. Write last byte to FIFO (data lost)
>
> You need descript clearly at commit message about this.
>
> >
> > I just need to prefill FIFO in the beginning to make sure FIFO is not
> > empty when HW starts transmitting.
> > So, this patch adds a condition that prefill FIFO only when FIFO is empty.
> >
> > >
> > > >
> > > > Fixes: 4008a74e0f9b ("i3c: master: svc: Fix npcm845 FIFO empty issue")
> > > > Signed-off-by: Stanley Chu <yschu@nuvoton.com>
> > > > ---
> > > >  drivers/i3c/master/svc-i3c-master.c | 11 ++++++++++-
> > > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> > > > index 7e1a7cb94b43..34b6e125b18a 100644
> > > > --- a/drivers/i3c/master/svc-i3c-master.c
> > > > +++ b/drivers/i3c/master/svc-i3c-master.c
> > > > @@ -104,6 +104,7 @@
> > > >  #define   SVC_I3C_MDATACTRL_TXTRIG_FIFO_NOT_FULL GENMASK(5, 4)
> > > >  #define   SVC_I3C_MDATACTRL_RXTRIG_FIFO_NOT_EMPTY 0
> > > >  #define   SVC_I3C_MDATACTRL_RXCOUNT(x) FIELD_GET(GENMASK(28, 24), (x))
> > > > +#define   SVC_I3C_MDATACTRL_TXCOUNT(x) FIELD_GET(GENMASK(20, 16), (x))
> > > >  #define   SVC_I3C_MDATACTRL_TXFULL BIT(30)
> > > >  #define   SVC_I3C_MDATACTRL_RXEMPTY BIT(31)
> > > >
> > > > @@ -280,6 +281,13 @@ static inline bool is_events_enabled(struct svc_i3c_master *master, u32 mask)
> > > >       return !!(master->enabled_events & mask);
> > > >  }
> > > >
> > > > +static inline bool svc_i3c_master_tx_empty(struct svc_i3c_master *master)
> > > > +{
> > > > +     u32 reg = readl(master->regs + SVC_I3C_MDATACTRL);
> > > > +
> > > > +     return (SVC_I3C_MDATACTRL_TXCOUNT(reg) == 0);
> > > > +}
> > > > +
> > > >  static bool svc_i3c_master_error(struct svc_i3c_master *master)
> > > >  {
> > > >       u32 mstatus, merrwarn;
> > > > @@ -1303,7 +1311,8 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> > > >                * The only way to work around this hardware issue is to let the
> > > >                * FIFO start filling as soon as possible after EmitStartAddr.
> > > >                */
> > > > -             if (svc_has_quirk(master, SVC_I3C_QUIRK_FIFO_EMPTY) && !rnw && xfer_len) {
> > > > +             if (svc_has_quirk(master, SVC_I3C_QUIRK_FIFO_EMPTY) && !rnw && xfer_len &&
> > > > +                 svc_i3c_master_tx_empty(master)) {
> > > >                       u32 end = xfer_len > SVC_I3C_FIFO_SIZE ? 0 : SVC_I3C_MWDATAB_END;
> > > >                       u32 len = min_t(u32, xfer_len, SVC_I3C_FIFO_SIZE);
> > >
> > > if prefill to FIFO SIZE each time, such as
> > >
> > > replace SVC_I3C_FIFO_SIZE with
> > >
> > > SVC_I3C_FIFO_SIZE - SVC_I3C_MDATACTRL_TXCOUNT(readl(master->regs + SVC_I3C_MDATACTRL)).
> > >
> > The free space is SVC_I3C_FIFO_SIZE when FIFO is empty, no need to
> > check TX count again.
>
> My means is that if use above logic, needn't check helper function
> svc_i3c_master_tx_empty(master).
>
> Does this method work?
>
Thanks Frank,
This method is another option that also can work for this issue.
I will submit version 2.

> Frank
>
> >
> > --
> > Stanley
> >
> > > Frank
> > > >
> > > > --
> > > > 2.34.1
> > > >
> >
> > --
> > linux-i3c mailing list
> > linux-i3c@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-i3c

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c