[PATCH v4 0/5] Add support for Nuvoton npcm845 i3c controller

[PATCH v4 0/5] Add support for Nuvoton npcm845 i3c controller

[Stanley Chu]

This patchset adds support for the Nuvoton npcm845
Board Management controller (BMC) SoC family.

The Nuvoton npcm845 uses the same Silvico IP but an older version.
This patchset adds fixes for the npcm845 specific hardware issues.

--
v4:
 - Fix kernel test robot build warning.
 - Add SVC_I3C_QUIRK_DAA_CORRUPT fix
--
v3:
 - Add more description in dt-binging commit message
 - Add the svc_i3c_drvdata structure in struct svc_i3c_master
 - Improve the do_daa
---
v2:
 - Add a new compatible string in dt-binding doc.
 - Add driver data for npcm845 to address the quirks.
 - Modify svc_i3c_master_write to be reused by SVC_I3C_QUIRK_FIFO_EMPTY fix.
 - Fix typo of SVC_I3C_QUIRK_FALSE_SLVSTART fix.
 - Remove the code changes in svc_i3c_master_do_daa_locked, will add it in
   another patch series for common improvement.
---

Stanley Chu (5):
  dt-bindings: i3c: silvaco: Add npcm845 compatible string
  i3c: master: svc: Add support for Nuvoton npcm845 i3c
  i3c: master: svc: Fix npcm845 FIFO empty issue
  i3c: master: svc: Fix npcm845 invalid slvstart event
  i3c: master: svc: Fix npcm845 DAA process corruption

 .../bindings/i3c/silvaco,i3c-master.yaml      |   4 +-
 drivers/i3c/master/svc-i3c-master.c           | 115 ++++++++++++++++--
 2 files changed, 105 insertions(+), 14 deletions(-)

-- 
2.34.1


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

[PATCH v4 1/5] dt-bindings: i3c: silvaco: Add npcm845 compatible string

[Stanley Chu]

From: Stanley Chu <yschu@nuvoton.com>

Nuvoton npcm845 SoC uses the same Silvico IP but an older version.
Need to add a new compatible string to distinguish between different
hardware versions.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Stanley Chu <yschu@nuvoton.com>
---
 Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml b/Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml
index c56ff77677f1..4fbdcdac0aee 100644
--- a/Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml
+++ b/Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml
@@ -14,7 +14,9 @@ allOf:
 
 properties:
   compatible:
-    const: silvaco,i3c-master-v1
+    enum:
+      - nuvoton,npcm845-i3c
+      - silvaco,i3c-master-v1
 
   reg:
     maxItems: 1
-- 
2.34.1


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

[PATCH v4 2/5] i3c: master: svc: Add support for Nuvoton npcm845 i3c

[Stanley Chu]

From: Stanley Chu <yschu@nuvoton.com>

Nuvoton npcm845 SoC uses the same Silvico IP but an older version.
Add quirks to address the npcm845 specific issues.

Signed-off-by: Stanley Chu <yschu@nuvoton.com>
---
 drivers/i3c/master/svc-i3c-master.c | 56 +++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index d6057d8c7dec..8834f87a4767 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -32,6 +32,7 @@
 #define   SVC_I3C_MCONFIG_ODBAUD(x) FIELD_PREP(GENMASK(23, 16), (x))
 #define   SVC_I3C_MCONFIG_ODHPP(x) FIELD_PREP(BIT(24), (x))
 #define   SVC_I3C_MCONFIG_SKEW(x) FIELD_PREP(GENMASK(27, 25), (x))
+#define   SVC_I3C_MCONFIG_SKEW_MASK GENMASK(27, 25)
 #define   SVC_I3C_MCONFIG_I2CBAUD(x) FIELD_PREP(GENMASK(31, 28), (x))
 
 #define SVC_I3C_MCTRL        0x084
@@ -133,6 +134,32 @@
 #define SVC_I3C_EVENT_IBI	GENMASK(7, 0)
 #define SVC_I3C_EVENT_HOTJOIN	BIT(31)
 
+/*
+ * SVC_I3C_QUIRK_FIFO_EMPTY:
+ * I3C HW stalls the write transfer if the transmit FIFO becomes empty,
+ * when new data is written to FIFO, I3C HW resumes the transfer but
+ * the first transmitted data bit may have the wrong value.
+ * Workaround:
+ * Fill the FIFO in advance to prevent FIFO from becoming empty.
+ */
+#define SVC_I3C_QUIRK_FIFO_EMPTY	BIT(0)
+/*
+ * SVC_I3C_QUIRK_FLASE_SLVSTART:
+ * I3C HW may generate an invalid SlvStart event when emitting a STOP.
+ * If it is a true SlvStart, the MSTATUS state is SLVREQ.
+ */
+#define SVC_I3C_QUIRK_FALSE_SLVSTART	BIT(1)
+/*
+ * SVC_I3C_QUIRK_DAA_CORRUPT:
+ * When MCONFIG.SKEW=0 and MCONFIG.ODHPP=0, the ENTDAA transaction gets
+ * corrupted and results in a no repeated-start condition at the end of
+ * address assignment.
+ * Workaround:
+ * Set MCONFIG.SKEW to 1 before initiating the DAA process. After the DAA
+ * process is completed, return MCONFIG.SKEW to its previous value.
+ */
+#define SVC_I3C_QUIRK_DAA_CORRUPT	BIT(2)
+
 struct svc_i3c_cmd {
 	u8 addr;
 	bool rnw;
@@ -158,6 +185,10 @@ struct svc_i3c_regs_save {
 	u32 mdynaddr;
 };
 
+struct svc_i3c_drvdata {
+	u32 quirks;
+};
+
 /**
  * struct svc_i3c_master - Silvaco I3C Master structure
  * @base: I3C master controller
@@ -183,6 +214,7 @@ struct svc_i3c_regs_save {
  * @ibi.tbq_slot: To be queued IBI slot
  * @ibi.lock: IBI lock
  * @lock: Transfer lock, protect between IBI work thread and callbacks from master
+ * @drvdata: Driver data
  * @enabled_events: Bit masks for enable events (IBI, HotJoin).
  * @mctrl_config: Configuration value in SVC_I3C_MCTRL for setting speed back.
  */
@@ -214,6 +246,7 @@ struct svc_i3c_master {
 		spinlock_t lock;
 	} ibi;
 	struct mutex lock;
+	const struct svc_i3c_drvdata *drvdata;
 	u32 enabled_events;
 	u32 mctrl_config;
 };
@@ -230,6 +263,27 @@ struct svc_i3c_i2c_dev_data {
 	struct i3c_generic_ibi_pool *ibi_pool;
 };
 
+const struct svc_i3c_drvdata npcm845_drvdata = {
+	.quirks = SVC_I3C_QUIRK_FIFO_EMPTY | SVC_I3C_QUIRK_FALSE_SLVSTART
+		| SVC_I3C_QUIRK_DAA_CORRUPT,
+};
+
+static bool svc_has_quirk(struct svc_i3c_master *master, u32 quirk)
+{
+	if (!master->drvdata)
+		return false;
+
+	if ((master->drvdata->quirks & quirk) == SVC_I3C_QUIRK_DAA_CORRUPT) {
+		if (master->mctrl_config &
+		    (SVC_I3C_MCONFIG_SKEW_MASK | SVC_I3C_MCONFIG_ODHPP(1)))
+			return false;
+		else
+			return true;
+	}
+
+	return (master->drvdata->quirks & quirk);
+}
+
 static inline bool is_events_enabled(struct svc_i3c_master *master, u32 mask)
 {
 	return !!(master->enabled_events & mask);
@@ -1868,6 +1922,7 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
 	}
 
 	platform_set_drvdata(pdev, master);
+	master->drvdata = of_device_get_match_data(dev);
 
 	pm_runtime_set_autosuspend_delay(&pdev->dev, SVC_I3C_PM_TIMEOUT_MS);
 	pm_runtime_use_autosuspend(&pdev->dev);
@@ -1960,6 +2015,7 @@ static const struct dev_pm_ops svc_i3c_pm_ops = {
 
 static const struct of_device_id svc_i3c_master_of_match_tbl[] = {
 	{ .compatible = "silvaco,i3c-master-v1"},
+	{ .compatible = "nuvoton,npcm845-i3c", .data = &npcm845_drvdata },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, svc_i3c_master_of_match_tbl);
-- 
2.34.1


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

[PATCH v4 3/5] i3c: master: svc: Fix npcm845 FIFO empty issue

[Stanley Chu]

From: Stanley Chu <yschu@nuvoton.com>

I3C HW stalls the write transfer if the transmit FIFO becomes empty,
when new data is written to FIFO, I3C HW resumes the transfer but the
first transmitted data bit may have the wrong value.
Fill the FIFO in advance to prevent FIFO from becoming empty.

Signed-off-by: Stanley Chu <yschu@nuvoton.com>
---
 drivers/i3c/master/svc-i3c-master.c | 44 ++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 13 deletions(-)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 8834f87a4767..07506ae0f914 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -942,7 +942,7 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
 					u8 *addrs, unsigned int *count)
 {
 	u64 prov_id[SVC_I3C_MAX_DEVS] = {}, nacking_prov_id = 0;
-	unsigned int dev_nb = 0, last_addr = 0;
+	unsigned int dev_nb = 0, last_addr = 0, dyn_addr;
 	u32 reg;
 	int ret, i;
 
@@ -985,6 +985,17 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
 		if (SVC_I3C_MSTATUS_RXPEND(reg)) {
 			u8 data[6];
 
+			/*
+			 * One slave sends its ID to request for address assignment,
+			 * pre-filling the dynamic address can reduce SCL clock stalls
+			 * and also fix the SVC_I3C_QUIRK_FIFO_EMPTY quirk.
+			 */
+			dyn_addr = i3c_master_get_free_addr(&master->base, last_addr + 1);
+			if (dyn_addr < 0)
+				return -ENOSPC;
+
+			writel(dyn_addr, master->regs + SVC_I3C_MWDATAB);
+
 			/*
 			 * We only care about the 48-bit provisioned ID yet to
 			 * be sure a device does not nack an address twice.
@@ -1063,21 +1074,16 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
 		if (ret)
 			break;
 
-		/* Give the slave device a suitable dynamic address */
-		ret = i3c_master_get_free_addr(&master->base, last_addr + 1);
-		if (ret < 0)
-			break;
-
-		addrs[dev_nb] = ret;
+		addrs[dev_nb] = dyn_addr;
 		dev_dbg(master->dev, "DAA: device %d assigned to 0x%02x\n",
 			dev_nb, addrs[dev_nb]);
-
-		writel(addrs[dev_nb], master->regs + SVC_I3C_MWDATAB);
 		last_addr = addrs[dev_nb++];
 	}
 
 	/* Need manual issue STOP except for Complete condition */
 	svc_i3c_master_emit_stop(master);
+	svc_i3c_master_flush_fifo(master);
+
 	return ret;
 }
 
@@ -1225,8 +1231,8 @@ static int svc_i3c_master_read(struct svc_i3c_master *master,
 	return offset;
 }
 
-static int svc_i3c_master_write(struct svc_i3c_master *master,
-				const u8 *out, unsigned int len)
+static int svc_i3c_master_write(struct svc_i3c_master *master, const u8 *out,
+				unsigned int len, bool last)
 {
 	int offset = 0, ret;
 	u32 mdctrl;
@@ -1243,7 +1249,7 @@ static int svc_i3c_master_write(struct svc_i3c_master *master,
 		 * The last byte to be sent over the bus must either have the
 		 * "end" bit set or be written in MWDATABE.
 		 */
-		if (likely(offset < (len - 1)))
+		if (likely(offset < (len - 1)) || !last)
 			writel(out[offset++], master->regs + SVC_I3C_MWDATAB);
 		else
 			writel(out[offset++], master->regs + SVC_I3C_MWDATABE);
@@ -1274,6 +1280,17 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 		       SVC_I3C_MCTRL_RDTERM(*actual_len),
 		       master->regs + SVC_I3C_MCTRL);
 
+		if (svc_has_quirk(master, SVC_I3C_QUIRK_FIFO_EMPTY) && !rnw && xfer_len) {
+			u32 len = min_t(u32, xfer_len, SVC_I3C_FIFO_SIZE);
+
+			ret = svc_i3c_master_write(master, out, len,
+						   xfer_len <= SVC_I3C_FIFO_SIZE);
+			if (ret < 0)
+				goto emit_stop;
+			xfer_len -= len;
+			out += len;
+		}
+
 		ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
 				 SVC_I3C_MSTATUS_MCTRLDONE(reg), 0, 1000);
 		if (ret)
@@ -1335,7 +1352,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 	if (rnw)
 		ret = svc_i3c_master_read(master, in, xfer_len);
 	else
-		ret = svc_i3c_master_write(master, out, xfer_len);
+		ret = svc_i3c_master_write(master, out, xfer_len, true);
 	if (ret < 0)
 		goto emit_stop;
 
@@ -1362,6 +1379,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 emit_stop:
 	svc_i3c_master_emit_stop(master);
 	svc_i3c_master_clear_merrwarn(master);
+	svc_i3c_master_flush_fifo(master);
 
 	return ret;
 }
-- 
2.34.1


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

[PATCH v4 4/5] i3c: master: svc: Fix npcm845 invalid slvstart event

[Stanley Chu]

From: Stanley Chu <yschu@nuvoton.com>

I3C HW may generate an invalid SlvStart event when emitting a STOP.
If it is a true SlvStart, the MSTATUS state is SLVREQ. Check the
MSTATUS state to ignore the false event.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Stanley Chu <yschu@nuvoton.com>
---
 drivers/i3c/master/svc-i3c-master.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 07506ae0f914..4eb4b8888725 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -59,6 +59,7 @@
 #define SVC_I3C_MSTATUS      0x088
 #define   SVC_I3C_MSTATUS_STATE(x) FIELD_GET(GENMASK(2, 0), (x))
 #define   SVC_I3C_MSTATUS_STATE_DAA(x) (SVC_I3C_MSTATUS_STATE(x) == 5)
+#define   SVC_I3C_MSTATUS_STATE_SLVREQ(x) (SVC_I3C_MSTATUS_STATE(x) == 1)
 #define   SVC_I3C_MSTATUS_STATE_IDLE(x) (SVC_I3C_MSTATUS_STATE(x) == 0)
 #define   SVC_I3C_MSTATUS_BETWEEN(x) FIELD_GET(BIT(4), (x))
 #define   SVC_I3C_MSTATUS_NACKED(x) FIELD_GET(BIT(5), (x))
@@ -618,6 +619,11 @@ static irqreturn_t svc_i3c_master_irq_handler(int irq, void *dev_id)
 	/* Clear the interrupt status */
 	writel(SVC_I3C_MINT_SLVSTART, master->regs + SVC_I3C_MSTATUS);
 
+	/* Ignore the false event */
+	if (svc_has_quirk(master, SVC_I3C_QUIRK_FALSE_SLVSTART) &&
+	    !SVC_I3C_MSTATUS_STATE_SLVREQ(active))
+		return IRQ_HANDLED;
+
 	svc_i3c_master_disable_interrupts(master);
 
 	/* Handle the interrupt in a non atomic context */
-- 
2.34.1


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

[PATCH v4 5/5] i3c: master: svc: Fix npcm845 DAA process corruption

[Stanley Chu]

From: Stanley Chu <yschu@nuvoton.com>

When MCONFIG.SKEW=0 and MCONFIG.ODHPP=0, the ENTDAA transaction gets
corrupted and results in a no repeated-start condition at the end of
address assignment.

Workaround: Set MCONFIG.SKEW to 1 before initiating the DAA process.
After the DAA process is completed, return MCONFIG.SKEW to its previous
value.

Signed-off-by: Stanley Chu <yschu@nuvoton.com>
---
 drivers/i3c/master/svc-i3c-master.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 4eb4b8888725..403d625e999e 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -1162,7 +1162,16 @@ static int svc_i3c_master_do_daa(struct i3c_master_controller *m)
 	}
 
 	spin_lock_irqsave(&master->xferqueue.lock, flags);
+
+	if (svc_has_quirk(master, SVC_I3C_QUIRK_DAA_CORRUPT))
+		writel(master->mctrl_config | SVC_I3C_MCONFIG_SKEW(1),
+		       master->regs + SVC_I3C_MCONFIG);
+
 	ret = svc_i3c_master_do_daa_locked(master, addrs, &dev_nb);
+
+	if (svc_has_quirk(master, SVC_I3C_QUIRK_DAA_CORRUPT))
+		writel(master->mctrl_config, master->regs + SVC_I3C_MCONFIG);
+
 	spin_unlock_irqrestore(&master->xferqueue.lock, flags);
 
 	svc_i3c_master_clear_merrwarn(master);
-- 
2.34.1


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

Re: [PATCH v4 1/5] dt-bindings: i3c: silvaco: Add npcm845 compatible string

[Frank Li]

On Mon, Feb 24, 2025 at 04:39:04PM +0800, Stanley Chu wrote:
> From: Stanley Chu <yschu@nuvoton.com>
>
> Nuvoton npcm845 SoC uses the same Silvico IP but an older version.
> Need to add a new compatible string to distinguish between different
> hardware versions.
>
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Stanley Chu <yschu@nuvoton.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml b/Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml
> index c56ff77677f1..4fbdcdac0aee 100644
> --- a/Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml
> +++ b/Documentation/devicetree/bindings/i3c/silvaco,i3c-master.yaml
> @@ -14,7 +14,9 @@ allOf:
>
>  properties:
>    compatible:
> -    const: silvaco,i3c-master-v1
> +    enum:
> +      - nuvoton,npcm845-i3c
> +      - silvaco,i3c-master-v1
>
>    reg:
>      maxItems: 1
> --
> 2.34.1
>

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

Re: [PATCH v4 2/5] i3c: master: svc: Add support for Nuvoton npcm845 i3c

[Frank Li]

On Mon, Feb 24, 2025 at 04:39:05PM +0800, Stanley Chu wrote:
> From: Stanley Chu <yschu@nuvoton.com>
>
> Nuvoton npcm845 SoC uses the same Silvico IP but an older version.
> Add quirks to address the npcm845 specific issues.

It'd better list issue here.

>
> Signed-off-by: Stanley Chu <yschu@nuvoton.com>
> ---
>  drivers/i3c/master/svc-i3c-master.c | 56 +++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
>
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index d6057d8c7dec..8834f87a4767 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -32,6 +32,7 @@
>  #define   SVC_I3C_MCONFIG_ODBAUD(x) FIELD_PREP(GENMASK(23, 16), (x))
>  #define   SVC_I3C_MCONFIG_ODHPP(x) FIELD_PREP(BIT(24), (x))
>  #define   SVC_I3C_MCONFIG_SKEW(x) FIELD_PREP(GENMASK(27, 25), (x))
> +#define   SVC_I3C_MCONFIG_SKEW_MASK GENMASK(27, 25)
>  #define   SVC_I3C_MCONFIG_I2CBAUD(x) FIELD_PREP(GENMASK(31, 28), (x))
>
>  #define SVC_I3C_MCTRL        0x084
> @@ -133,6 +134,32 @@
>  #define SVC_I3C_EVENT_IBI	GENMASK(7, 0)
>  #define SVC_I3C_EVENT_HOTJOIN	BIT(31)
>
> +/*
> + * SVC_I3C_QUIRK_FIFO_EMPTY:
> + * I3C HW stalls the write transfer if the transmit FIFO becomes empty,
> + * when new data is written to FIFO, I3C HW resumes the transfer but
> + * the first transmitted data bit may have the wrong value.
> + * Workaround:
> + * Fill the FIFO in advance to prevent FIFO from becoming empty.
> + */
> +#define SVC_I3C_QUIRK_FIFO_EMPTY	BIT(0)
> +/*
> + * SVC_I3C_QUIRK_FLASE_SLVSTART:
> + * I3C HW may generate an invalid SlvStart event when emitting a STOP.
> + * If it is a true SlvStart, the MSTATUS state is SLVREQ.
> + */
> +#define SVC_I3C_QUIRK_FALSE_SLVSTART	BIT(1)
> +/*
> + * SVC_I3C_QUIRK_DAA_CORRUPT:
> + * When MCONFIG.SKEW=0 and MCONFIG.ODHPP=0, the ENTDAA transaction gets
> + * corrupted and results in a no repeated-start condition at the end of
> + * address assignment.
> + * Workaround:
> + * Set MCONFIG.SKEW to 1 before initiating the DAA process. After the DAA
> + * process is completed, return MCONFIG.SKEW to its previous value.
> + */
> +#define SVC_I3C_QUIRK_DAA_CORRUPT	BIT(2)
> +
>  struct svc_i3c_cmd {
>  	u8 addr;
>  	bool rnw;
> @@ -158,6 +185,10 @@ struct svc_i3c_regs_save {
>  	u32 mdynaddr;
>  };
>
> +struct svc_i3c_drvdata {
> +	u32 quirks;
> +};
> +
>  /**
>   * struct svc_i3c_master - Silvaco I3C Master structure
>   * @base: I3C master controller
> @@ -183,6 +214,7 @@ struct svc_i3c_regs_save {
>   * @ibi.tbq_slot: To be queued IBI slot
>   * @ibi.lock: IBI lock
>   * @lock: Transfer lock, protect between IBI work thread and callbacks from master
> + * @drvdata: Driver data
>   * @enabled_events: Bit masks for enable events (IBI, HotJoin).
>   * @mctrl_config: Configuration value in SVC_I3C_MCTRL for setting speed back.
>   */
> @@ -214,6 +246,7 @@ struct svc_i3c_master {
>  		spinlock_t lock;
>  	} ibi;
>  	struct mutex lock;
> +	const struct svc_i3c_drvdata *drvdata;
>  	u32 enabled_events;
>  	u32 mctrl_config;
>  };
> @@ -230,6 +263,27 @@ struct svc_i3c_i2c_dev_data {
>  	struct i3c_generic_ibi_pool *ibi_pool;
>  };
>
> +const struct svc_i3c_drvdata npcm845_drvdata = {
> +	.quirks = SVC_I3C_QUIRK_FIFO_EMPTY | SVC_I3C_QUIRK_FALSE_SLVSTART
> +		| SVC_I3C_QUIRK_DAA_CORRUPT,
> +};
> +
> +static bool svc_has_quirk(struct svc_i3c_master *master, u32 quirk)
> +{
> +	if (!master->drvdata)
> +		return false;

You's better add drvdata for exist one to keep consistent for all compatibe
string.

> +
> +	if ((master->drvdata->quirks & quirk) == SVC_I3C_QUIRK_DAA_CORRUPT) {
> +		if (master->mctrl_config &
> +		    (SVC_I3C_MCONFIG_SKEW_MASK | SVC_I3C_MCONFIG_ODHPP(1)))
> +			return false;

This helper function should only check quirk, you addtional check for
mctrl_config, which exceed this function scope.  Or you add new helper
funciton check daa_corrupt only, such as  svs_has_daa_corrupt() ?

> +		else
> +			return true;
> +	}
> +
> +	return (master->drvdata->quirks & quirk);
> +}
> +
>  static inline bool is_events_enabled(struct svc_i3c_master *master, u32 mask)
>  {
>  	return !!(master->enabled_events & mask);
> @@ -1868,6 +1922,7 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
>  	}
>
>  	platform_set_drvdata(pdev, master);
> +	master->drvdata = of_device_get_match_data(dev);
>
>  	pm_runtime_set_autosuspend_delay(&pdev->dev, SVC_I3C_PM_TIMEOUT_MS);
>  	pm_runtime_use_autosuspend(&pdev->dev);
> @@ -1960,6 +2015,7 @@ static const struct dev_pm_ops svc_i3c_pm_ops = {
>
>  static const struct of_device_id svc_i3c_master_of_match_tbl[] = {
>  	{ .compatible = "silvaco,i3c-master-v1"},

silcaco_default_drvdata = {};
	.compatible = &silcaco_default_drvdata;


> +	{ .compatible = "nuvoton,npcm845-i3c", .data = &npcm845_drvdata },
>  	{ /* sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, svc_i3c_master_of_match_tbl);
> --
> 2.34.1
>

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

Re: [PATCH v4 3/5] i3c: master: svc: Fix npcm845 FIFO empty issue

[Frank Li]

On Mon, Feb 24, 2025 at 04:39:06PM +0800, Stanley Chu wrote:
> From: Stanley Chu <yschu@nuvoton.com>
>
> I3C HW stalls the write transfer if the transmit FIFO becomes empty,
> when new data is written to FIFO, I3C HW resumes the transfer but the
> first transmitted data bit may have the wrong value.
> Fill the FIFO in advance to prevent FIFO from becoming empty.
>
> Signed-off-by: Stanley Chu <yschu@nuvoton.com>
> ---
>  drivers/i3c/master/svc-i3c-master.c | 44 ++++++++++++++++++++---------
>  1 file changed, 31 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index 8834f87a4767..07506ae0f914 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -942,7 +942,7 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
>  					u8 *addrs, unsigned int *count)
>  {
>  	u64 prov_id[SVC_I3C_MAX_DEVS] = {}, nacking_prov_id = 0;
> -	unsigned int dev_nb = 0, last_addr = 0;
> +	unsigned int dev_nb = 0, last_addr = 0, dyn_addr;
>  	u32 reg;
>  	int ret, i;
>
> @@ -985,6 +985,17 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
>  		if (SVC_I3C_MSTATUS_RXPEND(reg)) {
>  			u8 data[6];
>
> +			/*
> +			 * One slave sends its ID to request for address assignment,
> +			 * pre-filling the dynamic address can reduce SCL clock stalls
> +			 * and also fix the SVC_I3C_QUIRK_FIFO_EMPTY quirk.
> +			 */
> +			dyn_addr = i3c_master_get_free_addr(&master->base, last_addr + 1);
> +			if (dyn_addr < 0)
> +				return -ENOSPC;
> +
> +			writel(dyn_addr, master->regs + SVC_I3C_MWDATAB);
> +

Although there is 64 clock time after issue do_daa, it is still better if
prefill dyn_addr before sent do daa command?

If add a debug message before i3c_master_get_free_addr(), does it trigger
hardware issue?

Frank

>  			/*
>  			 * We only care about the 48-bit provisioned ID yet to
>  			 * be sure a device does not nack an address twice.
> @@ -1063,21 +1074,16 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
>  		if (ret)
>  			break;
>
> -		/* Give the slave device a suitable dynamic address */
> -		ret = i3c_master_get_free_addr(&master->base, last_addr + 1);
> -		if (ret < 0)
> -			break;
> -
> -		addrs[dev_nb] = ret;
> +		addrs[dev_nb] = dyn_addr;
>  		dev_dbg(master->dev, "DAA: device %d assigned to 0x%02x\n",
>  			dev_nb, addrs[dev_nb]);
> -
> -		writel(addrs[dev_nb], master->regs + SVC_I3C_MWDATAB);
>  		last_addr = addrs[dev_nb++];
>  	}
>
>  	/* Need manual issue STOP except for Complete condition */
>  	svc_i3c_master_emit_stop(master);
> +	svc_i3c_master_flush_fifo(master);
> +
>  	return ret;
>  }
>
> @@ -1225,8 +1231,8 @@ static int svc_i3c_master_read(struct svc_i3c_master *master,
>  	return offset;
>  }
>
> -static int svc_i3c_master_write(struct svc_i3c_master *master,
> -				const u8 *out, unsigned int len)
> +static int svc_i3c_master_write(struct svc_i3c_master *master, const u8 *out,
> +				unsigned int len, bool last)
>  {
>  	int offset = 0, ret;
>  	u32 mdctrl;
> @@ -1243,7 +1249,7 @@ static int svc_i3c_master_write(struct svc_i3c_master *master,
>  		 * The last byte to be sent over the bus must either have the
>  		 * "end" bit set or be written in MWDATABE.
>  		 */
> -		if (likely(offset < (len - 1)))
> +		if (likely(offset < (len - 1)) || !last)
>  			writel(out[offset++], master->regs + SVC_I3C_MWDATAB);
>  		else
>  			writel(out[offset++], master->regs + SVC_I3C_MWDATABE);
> @@ -1274,6 +1280,17 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
>  		       SVC_I3C_MCTRL_RDTERM(*actual_len),
>  		       master->regs + SVC_I3C_MCTRL);
>
> +		if (svc_has_quirk(master, SVC_I3C_QUIRK_FIFO_EMPTY) && !rnw && xfer_len) {
> +			u32 len = min_t(u32, xfer_len, SVC_I3C_FIFO_SIZE);
> +
> +			ret = svc_i3c_master_write(master, out, len,
> +						   xfer_len <= SVC_I3C_FIFO_SIZE);
> +			if (ret < 0)
> +				goto emit_stop;
> +			xfer_len -= len;
> +			out += len;
> +		}
> +

The same here, you prefill data after issue sent out address, timing still
tight, only 9 SCL clock time. should it prefill before issue address?

Frank

>  		ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
>  				 SVC_I3C_MSTATUS_MCTRLDONE(reg), 0, 1000);
>  		if (ret)
> @@ -1335,7 +1352,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
>  	if (rnw)
>  		ret = svc_i3c_master_read(master, in, xfer_len);
>  	else
> -		ret = svc_i3c_master_write(master, out, xfer_len);
> +		ret = svc_i3c_master_write(master, out, xfer_len, true);
>  	if (ret < 0)
>  		goto emit_stop;
>
> @@ -1362,6 +1379,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
>  emit_stop:
>  	svc_i3c_master_emit_stop(master);
>  	svc_i3c_master_clear_merrwarn(master);
> +	svc_i3c_master_flush_fifo(master);
>
>  	return ret;
>  }
> --
> 2.34.1
>

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

Re: [PATCH v4 5/5] i3c: master: svc: Fix npcm845 DAA process corruption

[Frank Li]

On Mon, Feb 24, 2025 at 04:39:08PM +0800, Stanley Chu wrote:
> From: Stanley Chu <yschu@nuvoton.com>
>
> When MCONFIG.SKEW=0 and MCONFIG.ODHPP=0, the ENTDAA transaction gets
> corrupted and results in a no repeated-start condition at the end of
> address assignment.

If there are errata doc, provide a link will be appeciated.

>
> Workaround: Set MCONFIG.SKEW to 1 before initiating the DAA process.
> After the DAA process is completed, return MCONFIG.SKEW to its previous
> value.
>
> Signed-off-by: Stanley Chu <yschu@nuvoton.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/i3c/master/svc-i3c-master.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index 4eb4b8888725..403d625e999e 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -1162,7 +1162,16 @@ static int svc_i3c_master_do_daa(struct i3c_master_controller *m)
>  	}
>
>  	spin_lock_irqsave(&master->xferqueue.lock, flags);
> +
> +	if (svc_has_quirk(master, SVC_I3C_QUIRK_DAA_CORRUPT))
> +		writel(master->mctrl_config | SVC_I3C_MCONFIG_SKEW(1),
> +		       master->regs + SVC_I3C_MCONFIG);
> +
>  	ret = svc_i3c_master_do_daa_locked(master, addrs, &dev_nb);
> +
> +	if (svc_has_quirk(master, SVC_I3C_QUIRK_DAA_CORRUPT))
> +		writel(master->mctrl_config, master->regs + SVC_I3C_MCONFIG);
> +
>  	spin_unlock_irqrestore(&master->xferqueue.lock, flags);
>
>  	svc_i3c_master_clear_merrwarn(master);
> --
> 2.34.1
>

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

Re: [PATCH v4 3/5] i3c: master: svc: Fix npcm845 FIFO empty issue

[Stanley Chu]

On Tue, Feb 25, 2025 at 12:32 AM Frank Li <Frank.li@nxp.com> wrote:
>
> On Mon, Feb 24, 2025 at 04:39:06PM +0800, Stanley Chu wrote:
> > From: Stanley Chu <yschu@nuvoton.com>
> >
> > I3C HW stalls the write transfer if the transmit FIFO becomes empty,
> > when new data is written to FIFO, I3C HW resumes the transfer but the
> > first transmitted data bit may have the wrong value.
> > Fill the FIFO in advance to prevent FIFO from becoming empty.
> >
> > Signed-off-by: Stanley Chu <yschu@nuvoton.com>
> > ---
> >  drivers/i3c/master/svc-i3c-master.c | 44 ++++++++++++++++++++---------
> >  1 file changed, 31 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> > index 8834f87a4767..07506ae0f914 100644
> > --- a/drivers/i3c/master/svc-i3c-master.c
> > +++ b/drivers/i3c/master/svc-i3c-master.c
> > @@ -942,7 +942,7 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
> >                                       u8 *addrs, unsigned int *count)
> >  {
> >       u64 prov_id[SVC_I3C_MAX_DEVS] = {}, nacking_prov_id = 0;
> > -     unsigned int dev_nb = 0, last_addr = 0;
> > +     unsigned int dev_nb = 0, last_addr = 0, dyn_addr;
> >       u32 reg;
> >       int ret, i;
> >
> > @@ -985,6 +985,17 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
> >               if (SVC_I3C_MSTATUS_RXPEND(reg)) {
> >                       u8 data[6];
> >
> > +                     /*
> > +                      * One slave sends its ID to request for address assignment,
> > +                      * pre-filling the dynamic address can reduce SCL clock stalls
> > +                      * and also fix the SVC_I3C_QUIRK_FIFO_EMPTY quirk.
> > +                      */
> > +                     dyn_addr = i3c_master_get_free_addr(&master->base, last_addr + 1);
> > +                     if (dyn_addr < 0)
> > +                             return -ENOSPC;
> > +
> > +                     writel(dyn_addr, master->regs + SVC_I3C_MWDATAB);
> > +
>
> Although there is 64 clock time after issue do_daa, it is still better if
> prefill dyn_addr before sent do daa command?
>
> If add a debug message before i3c_master_get_free_addr(), does it trigger
> hardware issue?
>
> Frank

Ideally, prefilling before the processDAA command is better. However,
it requires an additional check to write the dyn_addr at the right time
because the driver needs to write the processDAA command twice for one
assignment

Prefilling here is safe and efficient because the FIFO starts filling
within a few hundred nanoseconds on the npcm845, which is significantly
faster compared to the 64 SCL clock cycles.


>
> >                       /*
> >                        * We only care about the 48-bit provisioned ID yet to
> >                        * be sure a device does not nack an address twice.
> > @@ -1063,21 +1074,16 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
> >               if (ret)
> >                       break;
> >
> > -             /* Give the slave device a suitable dynamic address */
> > -             ret = i3c_master_get_free_addr(&master->base, last_addr + 1);
> > -             if (ret < 0)
> > -                     break;
> > -
> > -             addrs[dev_nb] = ret;
> > +             addrs[dev_nb] = dyn_addr;
> >               dev_dbg(master->dev, "DAA: device %d assigned to 0x%02x\n",
> >                       dev_nb, addrs[dev_nb]);
> > -
> > -             writel(addrs[dev_nb], master->regs + SVC_I3C_MWDATAB);
> >               last_addr = addrs[dev_nb++];
> >       }
> >
> >       /* Need manual issue STOP except for Complete condition */
> >       svc_i3c_master_emit_stop(master);
> > +     svc_i3c_master_flush_fifo(master);
> > +
> >       return ret;
> >  }
> >
> > @@ -1225,8 +1231,8 @@ static int svc_i3c_master_read(struct svc_i3c_master *master,
> >       return offset;
> >  }
> >
> > -static int svc_i3c_master_write(struct svc_i3c_master *master,
> > -                             const u8 *out, unsigned int len)
> > +static int svc_i3c_master_write(struct svc_i3c_master *master, const u8 *out,
> > +                             unsigned int len, bool last)
> >  {
> >       int offset = 0, ret;
> >       u32 mdctrl;
> > @@ -1243,7 +1249,7 @@ static int svc_i3c_master_write(struct svc_i3c_master *master,
> >                * The last byte to be sent over the bus must either have the
> >                * "end" bit set or be written in MWDATABE.
> >                */
> > -             if (likely(offset < (len - 1)))
> > +             if (likely(offset < (len - 1)) || !last)
> >                       writel(out[offset++], master->regs + SVC_I3C_MWDATAB);
> >               else
> >                       writel(out[offset++], master->regs + SVC_I3C_MWDATABE);
> > @@ -1274,6 +1280,17 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> >                      SVC_I3C_MCTRL_RDTERM(*actual_len),
> >                      master->regs + SVC_I3C_MCTRL);
> >
> > +             if (svc_has_quirk(master, SVC_I3C_QUIRK_FIFO_EMPTY) && !rnw && xfer_len) {
> > +                     u32 len = min_t(u32, xfer_len, SVC_I3C_FIFO_SIZE);
> > +
> > +                     ret = svc_i3c_master_write(master, out, len,
> > +                                                xfer_len <= SVC_I3C_FIFO_SIZE);
> > +                     if (ret < 0)
> > +                             goto emit_stop;
> > +                     xfer_len -= len;
> > +                     out += len;
> > +             }
> > +
>
> The same here, you prefill data after issue sent out address, timing still
> tight, only 9 SCL clock time. should it prefill before issue address?
>
> Frank

The entire transaction can consist of multiple read and write
transfers. In the case
of S+7E/W+Sr+dyn_addr/W+data+P, If the data is prefilled before Sr, it
will be emitted
immediately and become part of the previous transfer.

It is not a problem to fill FIFO here, the reason is the same as above.
I will also modify the code as below to make it efficient and keep
svc_i3c_master_write unchanged.

                if (svc_has_quirk(master, SVC_I3C_QUIRK_FIFO_EMPTY) &&
!rnw && xfer_len) {
                        u32 len = min_t(u32, xfer_len, SVC_I3C_FIFO_SIZE);

                        while (len--) {
                                if (xfer_len == 1)
                                        writel(out[0], master->regs +
SVC_I3C_MWDATABE);
                                else
                                        writel(out[0], master->regs +
SVC_I3C_MWDATAB);
                                xfer_len--;
                                out++;
                        }
                }


>
> >               ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> >                                SVC_I3C_MSTATUS_MCTRLDONE(reg), 0, 1000);
> >               if (ret)
> > @@ -1335,7 +1352,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> >       if (rnw)
> >               ret = svc_i3c_master_read(master, in, xfer_len);
> >       else
> > -             ret = svc_i3c_master_write(master, out, xfer_len);
> > +             ret = svc_i3c_master_write(master, out, xfer_len, true);
> >       if (ret < 0)
> >               goto emit_stop;
> >
> > @@ -1362,6 +1379,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> >  emit_stop:
> >       svc_i3c_master_emit_stop(master);
> >       svc_i3c_master_clear_merrwarn(master);
> > +     svc_i3c_master_flush_fifo(master);
> >
> >       return ret;
> >  }
> > --
> > 2.34.1
> >

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

Re: [PATCH v4 3/5] i3c: master: svc: Fix npcm845 FIFO empty issue

[Frank Li]

On Tue, Feb 25, 2025 at 05:28:48PM +0800, Stanley Chu wrote:
> On Tue, Feb 25, 2025 at 12:32 AM Frank Li <Frank.li@nxp.com> wrote:
> >
> > On Mon, Feb 24, 2025 at 04:39:06PM +0800, Stanley Chu wrote:
> > > From: Stanley Chu <yschu@nuvoton.com>
> > >
> > > I3C HW stalls the write transfer if the transmit FIFO becomes empty,
> > > when new data is written to FIFO, I3C HW resumes the transfer but the
> > > first transmitted data bit may have the wrong value.
> > > Fill the FIFO in advance to prevent FIFO from becoming empty.
> > >
> > > Signed-off-by: Stanley Chu <yschu@nuvoton.com>
> > > ---
> > >  drivers/i3c/master/svc-i3c-master.c | 44 ++++++++++++++++++++---------
> > >  1 file changed, 31 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> > > index 8834f87a4767..07506ae0f914 100644
> > > --- a/drivers/i3c/master/svc-i3c-master.c
> > > +++ b/drivers/i3c/master/svc-i3c-master.c
> > > @@ -942,7 +942,7 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
> > >                                       u8 *addrs, unsigned int *count)
> > >  {
> > >       u64 prov_id[SVC_I3C_MAX_DEVS] = {}, nacking_prov_id = 0;
> > > -     unsigned int dev_nb = 0, last_addr = 0;
> > > +     unsigned int dev_nb = 0, last_addr = 0, dyn_addr;
> > >       u32 reg;
> > >       int ret, i;
> > >
> > > @@ -985,6 +985,17 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
> > >               if (SVC_I3C_MSTATUS_RXPEND(reg)) {
> > >                       u8 data[6];
> > >
> > > +                     /*
> > > +                      * One slave sends its ID to request for address assignment,
> > > +                      * pre-filling the dynamic address can reduce SCL clock stalls
> > > +                      * and also fix the SVC_I3C_QUIRK_FIFO_EMPTY quirk.
> > > +                      */
> > > +                     dyn_addr = i3c_master_get_free_addr(&master->base, last_addr + 1);
> > > +                     if (dyn_addr < 0)
> > > +                             return -ENOSPC;
> > > +
> > > +                     writel(dyn_addr, master->regs + SVC_I3C_MWDATAB);
> > > +
> >
> > Although there is 64 clock time after issue do_daa, it is still better if
> > prefill dyn_addr before sent do daa command?
> >
> > If add a debug message before i3c_master_get_free_addr(), does it trigger
> > hardware issue?
> >
> > Frank
>
> Ideally, prefilling before the processDAA command is better. However,
> it requires an additional check to write the dyn_addr at the right time
> because the driver needs to write the processDAA command twice for one
> assignment
>
> Prefilling here is safe and efficient because the FIFO starts filling
> within a few hundred nanoseconds on the npcm845, which is significantly
> faster compared to the 64 SCL clock cycles.

Okay, please this to comments.

>
>
> >
> > >                       /*
> > >                        * We only care about the 48-bit provisioned ID yet to
> > >                        * be sure a device does not nack an address twice.
> > > @@ -1063,21 +1074,16 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
> > >               if (ret)
> > >                       break;
> > >
> > > -             /* Give the slave device a suitable dynamic address */
> > > -             ret = i3c_master_get_free_addr(&master->base, last_addr + 1);
> > > -             if (ret < 0)
> > > -                     break;
> > > -
> > > -             addrs[dev_nb] = ret;
> > > +             addrs[dev_nb] = dyn_addr;
> > >               dev_dbg(master->dev, "DAA: device %d assigned to 0x%02x\n",
> > >                       dev_nb, addrs[dev_nb]);
> > > -
> > > -             writel(addrs[dev_nb], master->regs + SVC_I3C_MWDATAB);
> > >               last_addr = addrs[dev_nb++];
> > >       }
> > >
> > >       /* Need manual issue STOP except for Complete condition */
> > >       svc_i3c_master_emit_stop(master);
> > > +     svc_i3c_master_flush_fifo(master);
> > > +
> > >       return ret;
> > >  }
> > >
> > > @@ -1225,8 +1231,8 @@ static int svc_i3c_master_read(struct svc_i3c_master *master,
> > >       return offset;
> > >  }
> > >
> > > -static int svc_i3c_master_write(struct svc_i3c_master *master,
> > > -                             const u8 *out, unsigned int len)
> > > +static int svc_i3c_master_write(struct svc_i3c_master *master, const u8 *out,
> > > +                             unsigned int len, bool last)
> > >  {
> > >       int offset = 0, ret;
> > >       u32 mdctrl;
> > > @@ -1243,7 +1249,7 @@ static int svc_i3c_master_write(struct svc_i3c_master *master,
> > >                * The last byte to be sent over the bus must either have the
> > >                * "end" bit set or be written in MWDATABE.
> > >                */
> > > -             if (likely(offset < (len - 1)))
> > > +             if (likely(offset < (len - 1)) || !last)
> > >                       writel(out[offset++], master->regs + SVC_I3C_MWDATAB);
> > >               else
> > >                       writel(out[offset++], master->regs + SVC_I3C_MWDATABE);
> > > @@ -1274,6 +1280,17 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> > >                      SVC_I3C_MCTRL_RDTERM(*actual_len),
> > >                      master->regs + SVC_I3C_MCTRL);
> > >
> > > +             if (svc_has_quirk(master, SVC_I3C_QUIRK_FIFO_EMPTY) && !rnw && xfer_len) {
> > > +                     u32 len = min_t(u32, xfer_len, SVC_I3C_FIFO_SIZE);
> > > +
> > > +                     ret = svc_i3c_master_write(master, out, len,
> > > +                                                xfer_len <= SVC_I3C_FIFO_SIZE);
> > > +                     if (ret < 0)
> > > +                             goto emit_stop;
> > > +                     xfer_len -= len;
> > > +                     out += len;
> > > +             }
> > > +
> >
> > The same here, you prefill data after issue sent out address, timing still
> > tight, only 9 SCL clock time. should it prefill before issue address?
> >
> > Frank
>
> The entire transaction can consist of multiple read and write
> transfers. In the case
> of S+7E/W+Sr+dyn_addr/W+data+P, If the data is prefilled before Sr, it
> will be emitted

I think S+7E/W should be xfer[0]
        Sr+dyn_addr/W + data + P should be xfer[1]

this function only handle one xfer each call. xfer[0]'s size is 0, no
pre fill data.

Only have prefill data at xfer[1].

> immediately and become part of the previous transfer.
>
> It is not a problem to fill FIFO here, the reason is the same as above.
> I will also modify the code as below to make it efficient and keep
> svc_i3c_master_write unchanged.

no issue to modify svc_i3c_master_write(). I consider prefill data before
actually.

This hardware is not prefect.  Although it aleady in spin lock, it may run
some secuity firmware in secuity domain.  There are 100us timeout. If a
hypervisor manage firmware interrupt transfer, one timeout may happen.

If prefill data before send address,  it was safe at least for lenght less
than FIFO case.

Frank

>
>                 if (svc_has_quirk(master, SVC_I3C_QUIRK_FIFO_EMPTY) &&
> !rnw && xfer_len) {
>                         u32 len = min_t(u32, xfer_len, SVC_I3C_FIFO_SIZE);
>
>                         while (len--) {
>                                 if (xfer_len == 1)
>                                         writel(out[0], master->regs +
> SVC_I3C_MWDATABE);
>                                 else
>                                         writel(out[0], master->regs +
> SVC_I3C_MWDATAB);
>                                 xfer_len--;
>                                 out++;
>                         }
>                 }
>
>
> >
> > >               ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> > >                                SVC_I3C_MSTATUS_MCTRLDONE(reg), 0, 1000);
> > >               if (ret)
> > > @@ -1335,7 +1352,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> > >       if (rnw)
> > >               ret = svc_i3c_master_read(master, in, xfer_len);
> > >       else
> > > -             ret = svc_i3c_master_write(master, out, xfer_len);
> > > +             ret = svc_i3c_master_write(master, out, xfer_len, true);
> > >       if (ret < 0)
> > >               goto emit_stop;
> > >
> > > @@ -1362,6 +1379,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> > >  emit_stop:
> > >       svc_i3c_master_emit_stop(master);
> > >       svc_i3c_master_clear_merrwarn(master);
> > > +     svc_i3c_master_flush_fifo(master);
> > >
> > >       return ret;
> > >  }
> > > --
> > > 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 v4 3/5] i3c: master: svc: Fix npcm845 FIFO empty issue

[Stanley Chu]

On Wed, Feb 26, 2025 at 12:32 AM Frank Li <Frank.li@nxp.com> wrote:
>
> On Tue, Feb 25, 2025 at 05:28:48PM +0800, Stanley Chu wrote:
> > On Tue, Feb 25, 2025 at 12:32 AM Frank Li <Frank.li@nxp.com> wrote:
> > >
> > > On Mon, Feb 24, 2025 at 04:39:06PM +0800, Stanley Chu wrote:
> > > > From: Stanley Chu <yschu@nuvoton.com>
> > > >
> > > > I3C HW stalls the write transfer if the transmit FIFO becomes empty,
> > > > when new data is written to FIFO, I3C HW resumes the transfer but the
> > > > first transmitted data bit may have the wrong value.
> > > > Fill the FIFO in advance to prevent FIFO from becoming empty.
> > > >
> > > > Signed-off-by: Stanley Chu <yschu@nuvoton.com>
> > > > ---
> > > >  drivers/i3c/master/svc-i3c-master.c | 44 ++++++++++++++++++++---------
> > > >  1 file changed, 31 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> > > > index 8834f87a4767..07506ae0f914 100644
> > > > --- a/drivers/i3c/master/svc-i3c-master.c
> > > > +++ b/drivers/i3c/master/svc-i3c-master.c
> > > > @@ -942,7 +942,7 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
> > > >                                       u8 *addrs, unsigned int *count)
> > > >  {
> > > >       u64 prov_id[SVC_I3C_MAX_DEVS] = {}, nacking_prov_id = 0;
> > > > -     unsigned int dev_nb = 0, last_addr = 0;
> > > > +     unsigned int dev_nb = 0, last_addr = 0, dyn_addr;
> > > >       u32 reg;
> > > >       int ret, i;
> > > >
> > > > @@ -985,6 +985,17 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
> > > >               if (SVC_I3C_MSTATUS_RXPEND(reg)) {
> > > >                       u8 data[6];
> > > >
> > > > +                     /*
> > > > +                      * One slave sends its ID to request for address assignment,
> > > > +                      * pre-filling the dynamic address can reduce SCL clock stalls
> > > > +                      * and also fix the SVC_I3C_QUIRK_FIFO_EMPTY quirk.
> > > > +                      */
> > > > +                     dyn_addr = i3c_master_get_free_addr(&master->base, last_addr + 1);
> > > > +                     if (dyn_addr < 0)
> > > > +                             return -ENOSPC;
> > > > +
> > > > +                     writel(dyn_addr, master->regs + SVC_I3C_MWDATAB);
> > > > +
> > >
> > > Although there is 64 clock time after issue do_daa, it is still better if
> > > prefill dyn_addr before sent do daa command?
> > >
> > > If add a debug message before i3c_master_get_free_addr(), does it trigger
> > > hardware issue?
> > >
> > > Frank
> >
> > Ideally, prefilling before the processDAA command is better. However,
> > it requires an additional check to write the dyn_addr at the right time
> > because the driver needs to write the processDAA command twice for one
> > assignment
> >
> > Prefilling here is safe and efficient because the FIFO starts filling
> > within a few hundred nanoseconds on the npcm845, which is significantly
> > faster compared to the 64 SCL clock cycles.
>
> Okay, please this to comments.
>
> >
> >
> > >
> > > >                       /*
> > > >                        * We only care about the 48-bit provisioned ID yet to
> > > >                        * be sure a device does not nack an address twice.
> > > > @@ -1063,21 +1074,16 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
> > > >               if (ret)
> > > >                       break;
> > > >
> > > > -             /* Give the slave device a suitable dynamic address */
> > > > -             ret = i3c_master_get_free_addr(&master->base, last_addr + 1);
> > > > -             if (ret < 0)
> > > > -                     break;
> > > > -
> > > > -             addrs[dev_nb] = ret;
> > > > +             addrs[dev_nb] = dyn_addr;
> > > >               dev_dbg(master->dev, "DAA: device %d assigned to 0x%02x\n",
> > > >                       dev_nb, addrs[dev_nb]);
> > > > -
> > > > -             writel(addrs[dev_nb], master->regs + SVC_I3C_MWDATAB);
> > > >               last_addr = addrs[dev_nb++];
> > > >       }
> > > >
> > > >       /* Need manual issue STOP except for Complete condition */
> > > >       svc_i3c_master_emit_stop(master);
> > > > +     svc_i3c_master_flush_fifo(master);
> > > > +
> > > >       return ret;
> > > >  }
> > > >
> > > > @@ -1225,8 +1231,8 @@ static int svc_i3c_master_read(struct svc_i3c_master *master,
> > > >       return offset;
> > > >  }
> > > >
> > > > -static int svc_i3c_master_write(struct svc_i3c_master *master,
> > > > -                             const u8 *out, unsigned int len)
> > > > +static int svc_i3c_master_write(struct svc_i3c_master *master, const u8 *out,
> > > > +                             unsigned int len, bool last)
> > > >  {
> > > >       int offset = 0, ret;
> > > >       u32 mdctrl;
> > > > @@ -1243,7 +1249,7 @@ static int svc_i3c_master_write(struct svc_i3c_master *master,
> > > >                * The last byte to be sent over the bus must either have the
> > > >                * "end" bit set or be written in MWDATABE.
> > > >                */
> > > > -             if (likely(offset < (len - 1)))
> > > > +             if (likely(offset < (len - 1)) || !last)
> > > >                       writel(out[offset++], master->regs + SVC_I3C_MWDATAB);
> > > >               else
> > > >                       writel(out[offset++], master->regs + SVC_I3C_MWDATABE);
> > > > @@ -1274,6 +1280,17 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> > > >                      SVC_I3C_MCTRL_RDTERM(*actual_len),
> > > >                      master->regs + SVC_I3C_MCTRL);
> > > >
> > > > +             if (svc_has_quirk(master, SVC_I3C_QUIRK_FIFO_EMPTY) && !rnw && xfer_len) {
> > > > +                     u32 len = min_t(u32, xfer_len, SVC_I3C_FIFO_SIZE);
> > > > +
> > > > +                     ret = svc_i3c_master_write(master, out, len,
> > > > +                                                xfer_len <= SVC_I3C_FIFO_SIZE);
> > > > +                     if (ret < 0)
> > > > +                             goto emit_stop;
> > > > +                     xfer_len -= len;
> > > > +                     out += len;
> > > > +             }
> > > > +
> > >
> > > The same here, you prefill data after issue sent out address, timing still
> > > tight, only 9 SCL clock time. should it prefill before issue address?
> > >
> > > Frank
> >
> > The entire transaction can consist of multiple read and write
> > transfers. In the case
> > of S+7E/W+Sr+dyn_addr/W+data+P, If the data is prefilled before Sr, it
> > will be emitted
>
> I think S+7E/W should be xfer[0]
>         Sr+dyn_addr/W + data + P should be xfer[1]
>
> this function only handle one xfer each call. xfer[0]'s size is 0, no
> pre fill data.
>
> Only have prefill data at xfer[1].
>

Hi Frank,

Please check the example, a transaction contains 2 transfers.
xfer[0].addr=7E/W, no data
xfer[1].addr=dyn_addr/W, with data

send xfer[0]:
1. Emit Start+7E/W
2. Wait for MCTRLDONE
send xfer[1]:
3. Prefill xfer[1].data  // data is emitted immediately
4. Emit RepeatedStart+dyn_addr/W
5. Wait for MCTRLDONE
6. Write remaining data to FIFO if needed
7. Emit STOP

Part of xfer[1].data is emitted in xfer[0], which is the problem of prefill
before EmitStartAddr. This is the reason I moved step 3 after step 4.


> > immediately and become part of the previous transfer.
> >
> > It is not a problem to fill FIFO here, the reason is the same as above.
> > I will also modify the code as below to make it efficient and keep
> > svc_i3c_master_write unchanged.
>
> no issue to modify svc_i3c_master_write(). I consider prefill data before
> actually.
>
> This hardware is not prefect.  Although it aleady in spin lock, it may run
> some secuity firmware in secuity domain.  There are 100us timeout. If a
> hypervisor manage firmware interrupt transfer, one timeout may happen.
>
> If prefill data before send address,  it was safe at least for lenght less
> than FIFO case.
>
> Frank
>

Since prefill before EmitStartAddr has the problem I mentioned above, the FIFO
should start filling as soon as possible to work around the hardware issue.
This is why I prefer writing MWDATAB here instead of using svc_i3c_master_write,
as it can save some polling time.

The hardware-specific quirk is added to avoid affecting other platforms that
do not have this issue.

Stanley


> >
> >                 if (svc_has_quirk(master, SVC_I3C_QUIRK_FIFO_EMPTY) &&
> > !rnw && xfer_len) {
> >                         u32 len = min_t(u32, xfer_len, SVC_I3C_FIFO_SIZE);
> >
> >                         while (len--) {
> >                                 if (xfer_len == 1)
> >                                         writel(out[0], master->regs +
> > SVC_I3C_MWDATABE);
> >                                 else
> >                                         writel(out[0], master->regs +
> > SVC_I3C_MWDATAB);
> >                                 xfer_len--;
> >                                 out++;
> >                         }
> >                 }
> >
> >
> > >
> > > >               ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> > > >                                SVC_I3C_MSTATUS_MCTRLDONE(reg), 0, 1000);
> > > >               if (ret)
> > > > @@ -1335,7 +1352,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> > > >       if (rnw)
> > > >               ret = svc_i3c_master_read(master, in, xfer_len);
> > > >       else
> > > > -             ret = svc_i3c_master_write(master, out, xfer_len);
> > > > +             ret = svc_i3c_master_write(master, out, xfer_len, true);
> > > >       if (ret < 0)
> > > >               goto emit_stop;
> > > >
> > > > @@ -1362,6 +1379,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> > > >  emit_stop:
> > > >       svc_i3c_master_emit_stop(master);
> > > >       svc_i3c_master_clear_merrwarn(master);
> > > > +     svc_i3c_master_flush_fifo(master);
> > > >
> > > >       return ret;
> > > >  }
> > > > --
> > > > 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 v4 3/5] i3c: master: svc: Fix npcm845 FIFO empty issue

[Frank Li]

On Wed, Feb 26, 2025 at 11:26:24AM +0800, Stanley Chu wrote:
> On Wed, Feb 26, 2025 at 12:32 AM Frank Li <Frank.li@nxp.com> wrote:
> >
> > On Tue, Feb 25, 2025 at 05:28:48PM +0800, Stanley Chu wrote:
> > > On Tue, Feb 25, 2025 at 12:32 AM Frank Li <Frank.li@nxp.com> wrote:
> > > >
> > > > On Mon, Feb 24, 2025 at 04:39:06PM +0800, Stanley Chu wrote:
> > > > > From: Stanley Chu <yschu@nuvoton.com>
> > > > >
> > > > > I3C HW stalls the write transfer if the transmit FIFO becomes empty,
> > > > > when new data is written to FIFO, I3C HW resumes the transfer but the
> > > > > first transmitted data bit may have the wrong value.
> > > > > Fill the FIFO in advance to prevent FIFO from becoming empty.
> > > > >
> > > > > Signed-off-by: Stanley Chu <yschu@nuvoton.com>
> > > > > ---
> > > > >  drivers/i3c/master/svc-i3c-master.c | 44 ++++++++++++++++++++---------
> > > > >  1 file changed, 31 insertions(+), 13 deletions(-)
> > > > >
> > > > > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> > > > > index 8834f87a4767..07506ae0f914 100644
> > > > > --- a/drivers/i3c/master/svc-i3c-master.c
> > > > > +++ b/drivers/i3c/master/svc-i3c-master.c
> > > > > @@ -942,7 +942,7 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
> > > > >                                       u8 *addrs, unsigned int *count)
> > > > >  {
> > > > >       u64 prov_id[SVC_I3C_MAX_DEVS] = {}, nacking_prov_id = 0;
> > > > > -     unsigned int dev_nb = 0, last_addr = 0;
> > > > > +     unsigned int dev_nb = 0, last_addr = 0, dyn_addr;
> > > > >       u32 reg;
> > > > >       int ret, i;
> > > > >
> > > > > @@ -985,6 +985,17 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
> > > > >               if (SVC_I3C_MSTATUS_RXPEND(reg)) {
> > > > >                       u8 data[6];
> > > > >
> > > > > +                     /*
> > > > > +                      * One slave sends its ID to request for address assignment,
> > > > > +                      * pre-filling the dynamic address can reduce SCL clock stalls
> > > > > +                      * and also fix the SVC_I3C_QUIRK_FIFO_EMPTY quirk.
> > > > > +                      */
> > > > > +                     dyn_addr = i3c_master_get_free_addr(&master->base, last_addr + 1);
> > > > > +                     if (dyn_addr < 0)
> > > > > +                             return -ENOSPC;
> > > > > +
> > > > > +                     writel(dyn_addr, master->regs + SVC_I3C_MWDATAB);
> > > > > +
> > > >
> > > > Although there is 64 clock time after issue do_daa, it is still better if
> > > > prefill dyn_addr before sent do daa command?
> > > >
> > > > If add a debug message before i3c_master_get_free_addr(), does it trigger
> > > > hardware issue?
> > > >
> > > > Frank
> > >
> > > Ideally, prefilling before the processDAA command is better. However,
> > > it requires an additional check to write the dyn_addr at the right time
> > > because the driver needs to write the processDAA command twice for one
> > > assignment
> > >
> > > Prefilling here is safe and efficient because the FIFO starts filling
> > > within a few hundred nanoseconds on the npcm845, which is significantly
> > > faster compared to the 64 SCL clock cycles.
> >
> > Okay, please this to comments.
> >
> > >
> > >
> > > >
> > > > >                       /*
> > > > >                        * We only care about the 48-bit provisioned ID yet to
> > > > >                        * be sure a device does not nack an address twice.
> > > > > @@ -1063,21 +1074,16 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
> > > > >               if (ret)
> > > > >                       break;
> > > > >
> > > > > -             /* Give the slave device a suitable dynamic address */
> > > > > -             ret = i3c_master_get_free_addr(&master->base, last_addr + 1);
> > > > > -             if (ret < 0)
> > > > > -                     break;
> > > > > -
> > > > > -             addrs[dev_nb] = ret;
> > > > > +             addrs[dev_nb] = dyn_addr;
> > > > >               dev_dbg(master->dev, "DAA: device %d assigned to 0x%02x\n",
> > > > >                       dev_nb, addrs[dev_nb]);
> > > > > -
> > > > > -             writel(addrs[dev_nb], master->regs + SVC_I3C_MWDATAB);
> > > > >               last_addr = addrs[dev_nb++];
> > > > >       }
> > > > >
> > > > >       /* Need manual issue STOP except for Complete condition */
> > > > >       svc_i3c_master_emit_stop(master);
> > > > > +     svc_i3c_master_flush_fifo(master);
> > > > > +
> > > > >       return ret;
> > > > >  }
> > > > >
> > > > > @@ -1225,8 +1231,8 @@ static int svc_i3c_master_read(struct svc_i3c_master *master,
> > > > >       return offset;
> > > > >  }
> > > > >
> > > > > -static int svc_i3c_master_write(struct svc_i3c_master *master,
> > > > > -                             const u8 *out, unsigned int len)
> > > > > +static int svc_i3c_master_write(struct svc_i3c_master *master, const u8 *out,
> > > > > +                             unsigned int len, bool last)
> > > > >  {
> > > > >       int offset = 0, ret;
> > > > >       u32 mdctrl;
> > > > > @@ -1243,7 +1249,7 @@ static int svc_i3c_master_write(struct svc_i3c_master *master,
> > > > >                * The last byte to be sent over the bus must either have the
> > > > >                * "end" bit set or be written in MWDATABE.
> > > > >                */
> > > > > -             if (likely(offset < (len - 1)))
> > > > > +             if (likely(offset < (len - 1)) || !last)
> > > > >                       writel(out[offset++], master->regs + SVC_I3C_MWDATAB);
> > > > >               else
> > > > >                       writel(out[offset++], master->regs + SVC_I3C_MWDATABE);
> > > > > @@ -1274,6 +1280,17 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> > > > >                      SVC_I3C_MCTRL_RDTERM(*actual_len),
> > > > >                      master->regs + SVC_I3C_MCTRL);
> > > > >
> > > > > +             if (svc_has_quirk(master, SVC_I3C_QUIRK_FIFO_EMPTY) && !rnw && xfer_len) {
> > > > > +                     u32 len = min_t(u32, xfer_len, SVC_I3C_FIFO_SIZE);
> > > > > +
> > > > > +                     ret = svc_i3c_master_write(master, out, len,
> > > > > +                                                xfer_len <= SVC_I3C_FIFO_SIZE);
> > > > > +                     if (ret < 0)
> > > > > +                             goto emit_stop;
> > > > > +                     xfer_len -= len;
> > > > > +                     out += len;
> > > > > +             }
> > > > > +
> > > >
> > > > The same here, you prefill data after issue sent out address, timing still
> > > > tight, only 9 SCL clock time. should it prefill before issue address?
> > > >
> > > > Frank
> > >
> > > The entire transaction can consist of multiple read and write
> > > transfers. In the case
> > > of S+7E/W+Sr+dyn_addr/W+data+P, If the data is prefilled before Sr, it
> > > will be emitted
> >
> > I think S+7E/W should be xfer[0]
> >         Sr+dyn_addr/W + data + P should be xfer[1]
> >
> > this function only handle one xfer each call. xfer[0]'s size is 0, no
> > pre fill data.
> >
> > Only have prefill data at xfer[1].
> >
>
> Hi Frank,
>
> Please check the example, a transaction contains 2 transfers.
> xfer[0].addr=7E/W, no data
> xfer[1].addr=dyn_addr/W, with data
>
> send xfer[0]:
> 1. Emit Start+7E/W
> 2. Wait for MCTRLDONE
> send xfer[1]:
> 3. Prefill xfer[1].data  // data is emitted immediately

Yes, you are right.

> 4. Emit RepeatedStart+dyn_addr/W
> 5. Wait for MCTRLDONE
> 6. Write remaining data to FIFO if needed
> 7. Emit STOP
>
> Part of xfer[1].data is emitted in xfer[0], which is the problem of prefill
> before EmitStartAddr. This is the reason I moved step 3 after step 4.

Need comments to show the reason.

>
>
> > > immediately and become part of the previous transfer.
> > >
> > > It is not a problem to fill FIFO here, the reason is the same as above.
> > > I will also modify the code as below to make it efficient and keep
> > > svc_i3c_master_write unchanged.
> >
> > no issue to modify svc_i3c_master_write(). I consider prefill data before
> > actually.
> >
> > This hardware is not prefect.  Although it aleady in spin lock, it may run
> > some secuity firmware in secuity domain.  There are 100us timeout. If a
> > hypervisor manage firmware interrupt transfer, one timeout may happen.
> >
> > If prefill data before send address,  it was safe at least for lenght less
> > than FIFO case.
> >
> > Frank
> >
>
> Since prefill before EmitStartAddr has the problem I mentioned above, the FIFO
> should start filling as soon as possible to work around the hardware issue.
> This is why I prefer writing MWDATAB here instead of using svc_i3c_master_write,
> as it can save some polling time.

svc_i3c_master_xfer() is too long and not easy to read. You can create
new helper function/macro for it if you like.

>
> The hardware-specific quirk is added to avoid affecting other platforms that
> do not have this issue.
>
> Stanley
>
>
> > >
> > >                 if (svc_has_quirk(master, SVC_I3C_QUIRK_FIFO_EMPTY) &&
> > > !rnw && xfer_len) {
> > >                         u32 len = min_t(u32, xfer_len, SVC_I3C_FIFO_SIZE);
> > >
> > >                         while (len--) {
> > >                                 if (xfer_len == 1)
> > >                                         writel(out[0], master->regs +
> > > SVC_I3C_MWDATABE);
> > >                                 else
> > >                                         writel(out[0], master->regs +
> > > SVC_I3C_MWDATAB);

Or
#define SVC_I3C_MWD(last) ((last) ? SVC_I3C_MWDATABE : SVC_I3C_MWDATAB)

	writel(out[0], master->regs + SVC_I3C_MWD(xfer_len == 1))

Frank
> > >                                 xfer_len--;
> > >                                 out++;
> > >                         }
> > >                 }
> > >
> > >
> > > >
> > > > >               ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> > > > >                                SVC_I3C_MSTATUS_MCTRLDONE(reg), 0, 1000);
> > > > >               if (ret)
> > > > > @@ -1335,7 +1352,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> > > > >       if (rnw)
> > > > >               ret = svc_i3c_master_read(master, in, xfer_len);
> > > > >       else
> > > > > -             ret = svc_i3c_master_write(master, out, xfer_len);
> > > > > +             ret = svc_i3c_master_write(master, out, xfer_len, true);
> > > > >       if (ret < 0)
> > > > >               goto emit_stop;
> > > > >
> > > > > @@ -1362,6 +1379,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> > > > >  emit_stop:
> > > > >       svc_i3c_master_emit_stop(master);
> > > > >       svc_i3c_master_clear_merrwarn(master);
> > > > > +     svc_i3c_master_flush_fifo(master);
> > > > >
> > > > >       return ret;
> > > > >  }
> > > > > --
> > > > > 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