| Message ID | 20250224083908.1880383-4-yschu@nuvoton.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for Nuvoton npcm845 i3c controller | expand |
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 >
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 > >
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
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
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
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; }