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

[Frank Li <Frank.li@nxp.com>]

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
>