Re: [PATCH v4 3/5] i3c: master: svc: Fix npcm845 FIFO empty issue
From: Stanley Chu <hidden>
Date: 2025-02-26 03:26:38
Also in:
linux-i3c, lkml
On Wed, Feb 26, 2025 at 12:32 AM Frank Li [off-list ref] wrote:
On Tue, Feb 25, 2025 at 05:28:48PM +0800, Stanley Chu wrote:quoted
On Tue, Feb 25, 2025 at 12:32 AM Frank Li [off-list ref] wrote:quoted
On Mon, Feb 24, 2025 at 04:39:06PM +0800, Stanley Chu wrote:quoted
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? FrankIdeally, 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.quoted
quoted
quoted
/* * 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? FrankThe 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 emittedI 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.
quoted
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
quoted
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++; } }quoted
quoted
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