Re: [PATCH v2] i3c: master: dw-i3c-master: fix OD timing for first broadcast

On Tue, Jul 21, 2026 at 03:47:50AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issu=
e(s) to consider:
> - [Medium] The calculation for `od_hcnt` can silently overflow its 8-bit =
capacity if the core clock rate exceeds ~1.28 GHz, resulting in an inapprop=
riately fast open-drain speed.
> - [Low] Using DIV_ROUND_UP() with a 64-bit numerator or divisor causes a =
linker error (__udivdi3 undefined) on 32-bit platforms.
> --
>
> --- Patch [1]: [PATCH v2] i3c: master: dw-i3c-master: fix OD timing for f=
irst broadcast ---
> commit b82f29a59ae98bd18cbb544ded9a3bf8f8d60fc6
> Author: Tze Yee Ng <[email protected]>
>
> i3c: master: dw-i3c-master: fix OD timing for first broadcast
>
> This commit implements ->set_speed() to switch open-drain timing for the
> first broadcast address according to the I3C specification. It caches the
> normal OD register value during init and uses a separate od_hcnt for the
> slow path so SDR extended timing remains derived from the normal PP hcnt.
>
> > diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw=
-i3c-master.c
> > index 84ffcb1893389..077f2bf4b0b3f 100644
> > --- a/drivers/i3c/master/dw-i3c-master.c
> > +++ b/drivers/i3c/master/dw-i3c-master.c
>
> [ ... ]
>
> > @@ -1513,6 +1519,48 @@ static irqreturn_t dw_i3c_master_irq_handler(int=
 irq, void *dev_id)
> >  	return IRQ_HANDLED;
> >  }
> >
> > +static int dw_i3c_master_set_speed(struct i3c_master_controller *m,
> > +				   enum i3c_open_drain_speed speed)
> > +{
> > +	struct dw_i3c_master *master =3D to_dw_i3c_master(m);
> > +	unsigned long core_rate;
> > +	u32 scl_timing;
> > +	u8 od_hcnt, lcnt;
> > +
> > +	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(master->dev, pm);
> > +	if (PM_RUNTIME_ACQUIRE_ERR(&pm))
> > +		return -ENXIO;
> > +
> > +	switch (speed) {
> > +	case I3C_OPEN_DRAIN_SLOW_SPEED:
> > +		core_rate =3D clk_get_rate(master->core_clk);
> > +		if (!core_rate)
> > +			return -EINVAL;
> > +
> > +		lcnt =3D SCL_I3C_TIMING_LCNT(master->i3c_od_timing_normal);
> > +		od_hcnt =3D DIV_ROUND_UP((u64)I3C_BUS_THIGH_INIT_OD_MIN_NS * core_ra=
te,
> > +				       1000000000ULL) - 1;
>
> [Severity: Low]
> Will this cause a build failure on 32-bit platforms?
>
> Using DIV_ROUND_UP() with a 64-bit divisor (1000000000ULL) will likely
> result in a linker error for __udivdi3 on 32-bit architectures like ARM32
> or i386. Should this use DIV_ROUND_UP_ULL() instead?
>

make sense,

> [Severity: Medium]
> Can od_hcnt silently overflow here?
>
> Because od_hcnt is declared as a u8 at the top of the function, if the
> core clock rate exceeds ~1.28 GHz, the calculation will evaluate to a
> value greater than 255.

less possible, but suggest fix it.

Frank

>
> This would truncate when assigned to od_hcnt, bypassing the minimum 200ns
> requirement and potentially causing spike filters on devices to drop the
> broadcast.
>
> > +		if (od_hcnt < SCL_I3C_TIMING_CNT_MIN)
> > +			od_hcnt =3D SCL_I3C_TIMING_CNT_MIN;
> > +		scl_timing =3D SCL_I3C_TIMING_HCNT(od_hcnt) |
> > +			     SCL_I3C_TIMING_LCNT(lcnt);
> > +		writel(scl_timing, master->regs + SCL_I3C_OD_TIMING);
> > +		master->i3c_od_timing =3D scl_timing;
> > +		break;
>
> --
> Sashiko AI review =B7 https://sashiko.dev/#/patchset/20260721033159.11789=
[email protected]?part=3D1
>
> --
> linux-i3c mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-i3c

-- =

linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c