Re: [PATCH v2 2/4] i3c: master: svc: Add basic HDR mode support

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Frank,

>  struct svc_i3c_cmd {
>  	u8 addr;
> -	bool rnw;
> +	u8 rnw;

You used a union in the core, which makes sense I believe. I guess you
should do it here as well?


>  	u8 *in;
>  	const void *out;
>  	unsigned int len;
> @@ -383,6 +386,21 @@ svc_i3c_master_dev_from_addr(struct svc_i3c_master *master,
>  	return master->descs[i];
>  }

Maybe we should:
- First change the type of the command and make use of the helper to
derive the fact that it is a read
then
- Introduce HDR support.

Because there seems to be a lot of changes which are induced by this
internal API change and not related to HDR introduction at all.

>  
> +static bool svc_is_read(u8 rnw_cmd, u32 type)

Can we name this helper svc_cmd_is_read() ?

> +{
> +	return (type == SVC_I3C_MCTRL_TYPE_DDR) ? !!(rnw_cmd & 0x80) : rnw_cmd;
> +}
> +
> +static void svc_i3c_master_emit_force_exit(struct svc_i3c_master *master)
> +{
> +	u32 reg = 0;
> +
> +	writel(SVC_I3C_MCTRL_REQUEST_FORCE_EXIT, master->regs + SVC_I3C_MCTRL);
> +	readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> +			   SVC_I3C_MSTATUS_MCTRLDONE(reg), 0, 1000);
> +	udelay(1);
> +}
> +
>  static void svc_i3c_master_emit_stop(struct svc_i3c_master *master)
>  {
>  	writel(SVC_I3C_MCTRL_REQUEST_STOP, master->regs + SVC_I3C_MCTRL);
> @@ -1272,7 +1290,7 @@ static int svc_i3c_master_write(struct svc_i3c_master *master,
>  }
>  
>  static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> -			       bool rnw, unsigned int xfer_type, u8 addr,
> +			       u8 rnw, unsigned int xfer_type, u8 addr,
>  			       u8 *in, const u8 *out, unsigned int xfer_len,
>  			       unsigned int *actual_len, bool continued, bool repeat_start)
>  {
> @@ -1283,12 +1301,22 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
>  	/* clean SVC_I3C_MINT_IBIWON w1c bits */
>  	writel(SVC_I3C_MINT_IBIWON, master->regs + SVC_I3C_MSTATUS);
>  
> +	if (xfer_type == SVC_I3C_MCTRL_TYPE_DDR) {
> +		/* DDR command need prefill into FIFO */
> +		writel(rnw, master->regs + SVC_I3C_MWDATAB);
> +		if (!svc_is_read(rnw, xfer_type)) {
> +			/* write data also need prefill into FIFO */
> +			ret = svc_i3c_master_write(master, out, xfer_len);
> +		if (ret)
> +			goto emit_stop;
> +		}
> +	}
>  
>  	while (retry--) {
>  		writel(SVC_I3C_MCTRL_REQUEST_START_ADDR |
>  		       xfer_type |
>  		       SVC_I3C_MCTRL_IBIRESP_NACK |
> -		       SVC_I3C_MCTRL_DIR(rnw) |
> +		       SVC_I3C_MCTRL_DIR(svc_is_read(rnw, xfer_type)) |
>  		       SVC_I3C_MCTRL_ADDR(addr) |
>  		       SVC_I3C_MCTRL_RDTERM(*actual_len),
>  		       master->regs + SVC_I3C_MCTRL);
> @@ -1373,15 +1401,14 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
>  			break;
>  		}
>  	}
> -
> -	if (rnw)
> +	if (svc_is_read(rnw, xfer_type))
>  		ret = svc_i3c_master_read(master, in, xfer_len);
> -	else
> +	else if (xfer_type != SVC_I3C_MCTRL_TYPE_DDR)
>  		ret = svc_i3c_master_write(master, out, xfer_len);
>  	if (ret < 0)
>  		goto emit_stop;
>  
> -	if (rnw)
> +	if (svc_is_read(rnw, xfer_type))
>  		*actual_len = ret;
>  
>  	ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> @@ -1389,10 +1416,19 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
>  	if (ret)
>  		goto emit_stop;
>  
> +	if (xfer_type == SVC_I3C_MCTRL_TYPE_DDR &&
> +	    (readl(master->regs + SVC_I3C_MERRWARN) & SVC_I3C_MERRWARN_CRC)) {
> +		ret = -ENXIO;
> +		goto emit_stop;
> +	}
> +
>  	writel(SVC_I3C_MINT_COMPLETE, master->regs + SVC_I3C_MSTATUS);
>  
>  	if (!continued) {
> -		svc_i3c_master_emit_stop(master);
> +		if (xfer_type != SVC_I3C_MCTRL_TYPE_DDR)
> +			svc_i3c_master_emit_stop(master);
> +		else
> +			svc_i3c_master_emit_force_exit(master);
>  
>  		/* Wait idle if stop is sent. */
>  		readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> @@ -1402,7 +1438,11 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
>  	return 0;
>  
>  emit_stop:
> -	svc_i3c_master_emit_stop(master);
> +	if (xfer_type != SVC_I3C_MCTRL_TYPE_DDR)
> +		svc_i3c_master_emit_stop(master);
> +	else
> +		svc_i3c_master_emit_force_exit(master);
> +
>  	svc_i3c_master_clear_merrwarn(master);
>  	svc_i3c_master_flush_fifo(master);
>  
> @@ -1449,6 +1489,11 @@ static void svc_i3c_master_dequeue_xfer(struct svc_i3c_master *master,
>  	spin_unlock_irqrestore(&master->xferqueue.lock, flags);
>  }
>  
> +static int mode_to_type(enum i3c_hdr_mode mode)

Maybe "i3c_mode_to_svc_type()"?
> +{
> +	return (mode == I3C_SDR) ? SVC_I3C_MCTRL_TYPE_I3C : SVC_I3C_MCTRL_TYPE_DDR;
> +}
> +
>  static void svc_i3c_master_start_xfer_locked(struct svc_i3c_master *master)
>  {
>  	struct svc_i3c_xfer *xfer = master->xferqueue.cur;
> @@ -1638,9 +1683,8 @@ static int svc_i3c_master_send_ccc_cmd(struct i3c_master_controller *m,
>  	return ret;
>  }
>  
> -static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
> -				     struct i3c_priv_xfer *xfers,
> -				     int nxfers)
> +static int svc_i3c_master_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_priv_xfer *xfers,
> +				    int nxfers, enum i3c_hdr_mode mode)
>  {
>  	struct i3c_master_controller *m = i3c_dev_get_master(dev);
>  	struct svc_i3c_master *master = to_svc_i3c_master(m);
> @@ -1648,22 +1692,33 @@ static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
>  	struct svc_i3c_xfer *xfer;
>  	int ret, i;
>  
> +	if (mode != I3C_SDR) {
> +		/*
> +		 * Only support data size less than FIFO SIZE when use
> DDR mode.

when using DDR mode.

> +		 * First entry is cmd in FIFO, so actual available FIFO for data
> +		 * is SVC_I3C_FIFO_SIZE - 2 since DDR only support even
> length.

supports

> +		 */
> +		for (i = 0; i < nxfers; i++)
> +			if (xfers[i].len > SVC_I3C_FIFO_SIZE - 2)
> +				return -EINVAL;
> +	}
> +
>  	xfer = svc_i3c_master_alloc_xfer(master, nxfers);
>  	if (!xfer)
>  		return -ENOMEM;
>  
> -	xfer->type = SVC_I3C_MCTRL_TYPE_I3C;
> +	xfer->type = mode_to_type(mode);
>  
>  	for (i = 0; i < nxfers; i++) {
> +		u8 rnw_cmd = (mode == I3C_SDR) ? xfers[i].rnw : xfers[i].cmd;
>  		struct svc_i3c_cmd *cmd = &xfer->cmds[i];
> -

Spurious change?

>  		cmd->xfer = &xfers[i];
>  		cmd->addr = master->addrs[data->index];
> -		cmd->rnw = xfers[i].rnw;
> -		cmd->in = xfers[i].rnw ? xfers[i].data.in : NULL;
> -		cmd->out = xfers[i].rnw ? NULL : xfers[i].data.out;
> +		cmd->rnw = rnw_cmd;
> +		cmd->in = svc_is_read(rnw_cmd, mode_to_type(mode)) ? xfers[i].data.in : NULL;
> +		cmd->out = svc_is_read(rnw_cmd, mode_to_type(mode)) ? NULL : xfers[i].data.out;
>  		cmd->len = xfers[i].len;
> -		cmd->actual_len = xfers[i].rnw ? xfers[i].len : 0;
> +		cmd->actual_len = svc_is_read(rnw_cmd, mode_to_type(mode)) ? xfers[i].len : 0;
>  		cmd->continued = (i + 1) < nxfers;
>  	}
>  
> @@ -1858,7 +1913,7 @@ static const struct i3c_master_controller_ops svc_i3c_master_ops = {
>  	.do_daa = svc_i3c_master_do_daa,
>  	.supports_ccc_cmd = svc_i3c_master_supports_ccc_cmd,
>  	.send_ccc_cmd = svc_i3c_master_send_ccc_cmd,
> -	.priv_xfers = svc_i3c_master_priv_xfers,
> +	.i3c_xfers = svc_i3c_master_i3c_xfers,

Didn't you change this name in patch 1? If you kept both naming, it's
fine, otherwise you must do the switch in patch 1 as well to make sure
you don't break the build in the middle of the series.

>  	.i2c_xfers = svc_i3c_master_i2c_xfers,
>  	.request_ibi = svc_i3c_master_request_ibi,
>  	.free_ibi = svc_i3c_master_free_ibi,
> @@ -1947,6 +2002,8 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
>  
>  	svc_i3c_master_reset(master);
>  
> +	master->base.mode_mask = BIT(I3C_SDR) | BIT(I3C_HDR_DDR);
> +
>  	/* Register the master */
>  	ret = i3c_master_register(&master->base, &pdev->dev,
>  				  &svc_i3c_master_ops, false);

Thanks,
Miquèl