[PATCH 2/2] i3c: add driver for Renesas I3C controller

[Wolfram Sang]

Hi Frank,

thank you for the review!

> Suggest commit message:
> 
> i3c: master: Add basic driver for Renesas RZ/G3S and G3E SoCs
> 
> Add a basic I3C master driver for the I3C controller found in Renesas
> RZ/G3S and G3E SoCs. Support I3C pure busses (tested with two targets) and
> mixed busses (two I3C devices plus various I2C targets). DAA and
> communication with temperature sensors worked reliably at various speeds
> 
> Missing features such as IBI, HotJoin, and target mode will be added
> incrementally.

Ok, I wil use this message. Thanks for providing it.

> > +F:	drivers/i3c/master/renesas-i3c.c
> 
> Add i3c mail list?

It is inherited from the I3C subsystem entry.

> > +#define STDBR			0x74
> > +#define  STDBR_SBRLO(cond, x)	FIELD_PREP(GENMASK(7, 0), (x) >> (cond))
> > +#define  STDBR_SBRHO(cond, x)	FIELD_PREP(GENMASK(15, 8), (x) >> (cond))
> 
> I think pass down od_low_ticks instead of cond will be easy to understand.
> 
> STDBR_SBRLO(l, x) FIELD_PREP(GENMASK(7, 0), (x) >> ((l) > 0xFF > 1: 0)

Well, I think the fact that you got it wrong is indicating that it is
not so easy to understand :) It should be:

STDBR_SBRLO(l, x) FIELD_PREP(GENMASK(7, 0), (x) >> ((l) > 0xFF ? 1: 0)

I also think it won't gain us much. We still need the 'double_SBR'
variable to set a specific bit at the same time we use the macro.
Unless you want a dedicated macro for STDBR_DSBRPO as well.

> But still strange,  l > 0xFF, you just shift right just 1 bits.

Yes.

> what's happen if l is 0x3ff.

The driver bails out:

+	if ((od_low_ticks / 2) > 0xFF || pp_low_ticks > 0x3F) {
+		dev_err(&m->dev, "invalid speed (i2c-scl = %lu Hz, i3c-scl = %lu Hz). Too slow.\n",
+			(unsigned long)bus->scl_rate.i2c, (unsigned long)bus->scl_rate.i3c);
+		ret = -EINVAL;
+		return ret;
+	}

Oh, the last two lines can be merged into one...

> 
> > +#define  STDBR_SBRLP(x)		FIELD_PREP(GENMASK(21, 16), x)
> > +#define  STDBR_SBRHP(x)		FIELD_PREP(GENMASK(29, 24), x)
> > +#define  STDBR_DSBRPO		BIT(31)
> > +
> > +#define EXTBR			0x78
> > +#define  EXTBR_EBRLO(x)		FIELD_PREP(GENMASK(7, 0), x)
> > +#define  EXTBR_EBRHO(x)		FIELD_PREP(GENMASK(15, 8), x)
> > +#define  EXTBR_EBRLP(x)		FIELD_PREP(GENMASK(21, 16), x)
> > +#define  EXTBR_EBRHP(x)		FIELD_PREP(GENMASK(29, 24), x)
> 
> did this pass check_patch.sh? I remember need (x)

Yes, it passed.

> maybe run check_patch.sh --strict

I used --strict. Does it complain on your side?

> > +static void i3c_reg_update_bit(void __iomem *base, u32 reg,
> > +			       u32 mask, u32 val)
> > +{
> > +	i3c_reg_update(base + reg, mask, val);
> > +}
> 
> It is similor to regmap. If you use mmios' regmap, needn't above help
> functions.

Is this a suggestion or a requirement? I'd like to keep it this way.

...

> > +static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
> > +					struct i2c_msg *i2c_xfers,
> > +					int i2c_nxfers)
> > +{
> > +	struct i3c_master_controller *m = i2c_dev_get_master(dev);
> > +	struct renesas_i3c *i3c = to_renesas_i3c(m);
> > +	struct renesas_i3c_xfer *xfer;
> > +	struct renesas_i3c_cmd *cmd;
> > +	u8 start_bit = CNDCTL_STCND;
> > +	int ret, i;
> > +
> > +	if (!i2c_nxfers)
> > +		return 0;
> > +
> > +	renesas_i3c_bus_enable(m, false);
> > +
> > +	xfer = renesas_i3c_alloc_xfer(i3c, 1);
> > +	if (!xfer)
> > +		return -ENOMEM;
> > +
> > +	init_completion(&xfer->comp);
> > +	xfer->is_i2c_xfer = true;
> > +	cmd = xfer->cmds;
> > +
> > +	if (!(i3c_reg_read(i3c->regs, BCST) & BCST_BFREF)) {
> > +		cmd->err = -EBUSY;
> > +		goto out;
> > +	}
> > +
> > +	i3c_reg_write(i3c->regs, BST, 0);
> > +
> > +	renesas_i3c_enqueue_xfer(i3c, xfer);
> 
> You can use refer mutex GUARD define to pair renesas_i3c_enqueue_xfer()
> and renesas_i3c_dequeue_xfer().

Well, looking again, I won't need this. There is no 'goto out' after
enqueueing. So, the label is wrongly placed. Might be an argument to
remove it...

> 
> > +
> > +	for (i = 0; i < i2c_nxfers; i++) {
> > +		cmd->i2c_bytes_left = I2C_INIT_MSG;
> > +		cmd->i2c_buf = i2c_xfers[i].buf;
> > +		cmd->msg = &i2c_xfers[i];
> > +		cmd->i2c_is_last = (i == i2c_nxfers - 1);
> > +
> > +		i3c_reg_set_bit(i3c->regs, BIE, BIE_NACKDIE);
> > +		i3c_reg_set_bit(i3c->regs, NTIE, NTIE_TDBEIE0);
> > +		i3c_reg_set_bit(i3c->regs, BIE, BIE_STCNDDIE);
> > +
> > +		/* Issue Start condition */
> > +		i3c_reg_set_bit(i3c->regs, CNDCTL, start_bit);
> > +
> > +		i3c_reg_set_bit(i3c->regs, NTSTE, NTSTE_TDBEE0);
> > +
> > +		wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
> > +
> > +		if (cmd->err)
> > +			break;
> > +
> > +		start_bit = CNDCTL_SRCND;
> > +	}
> > +out:
> > +	renesas_i3c_dequeue_xfer(i3c, xfer);
> > +	ret = cmd->err;
> > +	kfree(xfer);
> 
> struct renesas_i3c_xfer *xfer __free(kfree) = renesas_i3c_alloc_xfer(i3c, 1);

... by doing this.

> > +		/* Clear the Transmit Buffer Empty status flag. */
> > +		i3c_reg_clear_bit(i3c->regs, BST, BST_TENDF);
> 
> Are you sure you can clear FIFO empty status? Generally it is read only.

Yes. The datasheet says so. If you want to double check, page 1715 of
the document which link I provided last time.

Thanks for the input, I will work on this now...

   Wolfram