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, ®map_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-...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-i3c
