Re: [PATCH v3 13/17] i3c: renesas: Update HW registers after SW computations are done

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Division by zero when calculating i2c_total_ticks in I3C_BUS_MODE_PURE if the I2C SCL rate is not provided.
- [High] Silent truncation of EXTBR register fields causes invalid timing configurations when tick counts exceed 255.
--

commit 379eeb8380aac5b75810596989edff2cb2084d3b
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

i3c: renesas: Update HW registers after SW computations are done

renesas_i3c_bus_init() performs a number of computations and software
cache updates, interleaving them with hardware register writes. While
this works today, it makes it harder to minimize the time the controller
must remain powered when runtime PM is introduced.

Perform all software computations and cache updates first, then update
the hardware registers. This prepares for future runtime PM support.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index dab643cd88598..38ce722881da4 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -560,10 +560,6 @@ static int renesas_i3c_bus_init(struct i3c_master_controller *m)
>  	if (!i3c->rate)
>  		return -EINVAL;
>  
> -	ret = renesas_i3c_reset(i3c);
> -	if (ret)
> -		return ret;
> -
>  	i2c_total_ticks = DIV_ROUND_UP(i3c->rate, bus->scl_rate.i2c);

[Severity: High]
This is a pre-existing issue, but could this cause a division by zero panic
in renesas_i3c_bus_init()?

If the I3C bus operates in I3C_BUS_MODE_PURE and the i2c-scl-hz property
is missing from the device tree, bus->scl_rate.i2c remains 0 because the core
function i3c_bus_set_mode() does not apply a fallback value in pure mode.
Will this lead to a division by zero during the probe phase?

>  	i3c_total_ticks = DIV_ROUND_UP(i3c->rate, bus->scl_rate.i3c);

[ ... ]

> @@ -614,27 +610,31 @@ static int renesas_i3c_bus_init(struct i3c_master_controller *m)
>  			STDBR_SBRHO(double_SBR, od_high_ticks) |
>  			STDBR_SBRLP(pp_low_ticks) |
>  			STDBR_SBRHP(pp_high_ticks);
> -	renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
>  
>  	/* Extended Bit Rate setting */
>  	i3c->extbr = EXTBR_EBRLO(od_low_ticks) | EXTBR_EBRHO(od_high_ticks) |

[Severity: High]
This isn't a bug introduced by this patch, but could this silently truncate
the EXTBR register fields?

The driver supports od_low_ticks and od_high_ticks up to 510 by checking
(od_low_ticks / 2) > 0xFF. For the STDBR registers, double_SBR is set and
used to right-shift the values. However, EXTBR_EBRLO and EXTBR_EBRHO directly
apply an 8-bit mask (e.g. FIELD_PREP(GENMASK(7, 0), x)) without shifting.

If the adjusted ticks fall between 256 and 510, does this misconfigure the
hardware with drastically shortened or zero-length Extended Bit Rate periods?

>  		     EXTBR_EBRLP(pp_low_ticks) | EXTBR_EBRHP(pp_high_ticks);
> -	renesas_writel(i3c->regs, EXTBR, i3c->extbr);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608201543.804902-1-claudiu.beznea@kernel.org?part=13

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c