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

[Frank Li]

On Wed, Jul 16, 2025 at 01:53:26PM +0200, Wolfram Sang wrote:
> 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...

Okay, it is not big detail.

>
> >
> > > +#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.
>

Mark brown provide similar feedback at the other code review because only
4 line at probe function needed to avoid duplicate simular function every
where.

for example:

static const struct regmap_config regmap_config = {
			.reg_bits = 32,
			.val_bits = 32,
			.reg_stride = 4,
		};

imx_pcie->iomuxc_gpr = devm_regmap_init_mmio(dev, off, &regmap_config);

If alex think that is okay, I am fine. I just think not neccesary to
duplicate such codes in your driver.

Frank
> ...
>
> > > +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
>
>
> --
> linux-i3c mailing list
> linux-i3c at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-i3c