On Thu, Feb 27, 2025 at 12:23:18PM -0500, Frank Li wrote:
> On Thu, Feb 27, 2025 at 02:01:29PM +0800, Stanley Chu wrote:
> > From: Stanley Chu <yschu@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > ---
> > drivers/i3c/master/svc-i3c-master.c | 52 ++++++++++++++++++++++++-----
> > 1 file changed, 43 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> > index 9143a079de53..a0c6d8053a25 100644
> > --- a/drivers/i3c/master/svc-i3c-master.c
> > +++ b/drivers/i3c/master/svc-i3c-master.c
> > @@ -114,6 +114,7 @@
> > #define SVC_I3C_MWDATAHE 0x0BC
> > #define SVC_I3C_MRDATAB 0x0C0
> > #define SVC_I3C_MRDATAH 0x0C8
> > +#define SVC_I3C_MWDATAB1 0x0CC
> > #define SVC_I3C_MWMSG_SDR 0x0D0
> > #define SVC_I3C_MRMSG_SDR 0x0D4
> > #define SVC_I3C_MWMSG_DDR 0x0D8
> > @@ -940,7 +941,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;
> >
> > @@ -983,6 +984,25 @@ 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,
> > + * prefilling the dynamic address can reduce SCL clock stalls
> > + * and also fix the SVC_I3C_QUIRK_FIFO_EMPTY quirk.
> > + *
> > + * 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, which is significantly
> > + * faster compared to the 64 SCL clock cycles.
> > + */
> > + 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.
> > @@ -1061,21 +1081,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;
> > }
> >
> > @@ -1272,6 +1287,24 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> > SVC_I3C_MCTRL_RDTERM(*actual_len),
> > master->regs + SVC_I3C_MCTRL);
> >
> > + /*
> > + * The entire transaction can consist of multiple write transfers.
> > + * Prefilling before EmitStartAddr causes the data to be emitted
> > + * immediately, becoming part of the previous transfer.
> > + * The only way to work around this hardware issue is to let the
> > + * FIFO start filling as soon as possible after EmitStartAddr.
> > + */
> > + if (svc_has_quirk(master, SVC_I3C_QUIRK_FIFO_EMPTY) && !rnw && xfer_len) {
> > + u32 end = xfer_len > SVC_I3C_FIFO_SIZE ? 0 : BIT(8);
>
> Define BIT(8) as macro
>
> Let me find a time at next week to do simple test for this.
I run basic test, it is fine!
Reviewed-by: Frank Li <Frank.Li@xxxxxxx>
>
> > + u32 len = min_t(u32, xfer_len, SVC_I3C_FIFO_SIZE);
> > +
> > + writesb(master->regs + SVC_I3C_MWDATAB1, out, len - 1);
> > + /* Mark END bit if this is the last byte */
> > + writel(out[len - 1] | end, master->regs + SVC_I3C_MWDATAB);
> > + xfer_len -= len;
> > + out += len;
> > + }
> > +
> > ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> > SVC_I3C_MSTATUS_MCTRLDONE(reg), 0, 1000);
> > if (ret)
> > @@ -1360,6 +1393,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
> >
