Re: [PATCH v5 02/17] i3c: renesas: Restore STDBR and EXTBR registers on resume

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

Pre-existing issues:
- [High] EXTBR timing registers are silently truncated when SCL low/high ticks exceed 255.
- [High] Hardware resets are unconditionally deasserted before clocks are enabled during system resume.
--

--- Patch [2]: [PATCH v5 02/17] i3c: renesas: Restore STDBR and EXTBR registers on resume ---
commit 33917325ffb6281761a5f99c8b41bb972ad950bd
Author: Claudiu Beznea <[email protected]>

i3c: renesas: Restore STDBR and EXTBR registers on resume

This commit adds an extbr field to the renesas_i3c struct to cache the
Extended Bit Rate configuration during initialization. It then restores
this value, along with the Standard Bit Rate register, during the device
resume sequence to retain the controller's state after power loss.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index 38b8428f464c3..cd9928649c7f7 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c

[ ... ]

> @@ -622,10 +623,9 @@ static int renesas_i3c_bus_init(struct i3c_master_controller *m)
>  	renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
>  
>  	/* Extended Bit Rate setting */
> -	renesas_writel(i3c->regs, EXTBR, EXTBR_EBRLO(od_low_ticks) |
> -					   EXTBR_EBRHO(od_high_ticks) |
> -					   EXTBR_EBRLP(pp_low_ticks) |
> -					   EXTBR_EBRHP(pp_high_ticks));
> +	i3c->extbr = EXTBR_EBRLO(od_low_ticks) | EXTBR_EBRHO(od_high_ticks) |
> +		     EXTBR_EBRLP(pp_low_ticks) | EXTBR_EBRHP(pp_high_ticks);
> +	renesas_writel(i3c->regs, EXTBR, i3c->extbr);

[Severity: High]
This isn't a bug introduced by this patch, but caching this value here means
we are storing a corrupted register value to restore on system resume.

When the calculated od_low_ticks or od_high_ticks exceed 255 (which sets
the double_SBR bit for STDBR), does EXTBR_EBRLO silently truncate the
upper bits?

Unlike STDBR_SBRLO which shifts the value down by double_SBR before
masking, EXTBR_EBRLO passes the value directly to
FIELD_PREP(GENMASK(7, 0), x).

Since od_low_ticks can be up to 510, this would drop the high bit and
result in malformed Extended Bit Rate (EXTBR) timings.

>  
>  	renesas_writel(i3c->regs, REFCKCTL, REFCKCTL_IREFCKS(cks));
>  	i3c->refclk_div = cks;

[ ... ]

> @@ -1468,6 +1468,8 @@ static int renesas_i3c_resume_noirq(struct device *dev)
>  		goto err_tresetn;
>  
>  	/* Re-store I3C registers value. */
> +	renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
> +	renesas_writel(i3c->regs, EXTBR, i3c->extbr);

[Severity: High]
This is a pre-existing issue, but since it is modifying register restores
during system resume, are we doing this in the correct order with respect
to clocks?

Looking slightly earlier in renesas_i3c_resume_noirq():

renesas_i3c_resume_noirq() {
        ...
        ret = reset_control_deassert(i3c->presetn);
        if (ret)
                return ret;

        ret = reset_control_deassert(i3c->tresetn);
        if (ret)
                goto err_presetn;

        ret = clk_bulk_enable(i3c->num_clks, i3c->clks);
        ...
}

If the hardware resets are synchronous, could deasserting them while
clocks are disabled leave the controller in an undefined state or prevent
proper recovery during resume?

>  	renesas_writel(i3c->regs, REFCKCTL,
>  		       REFCKCTL_IREFCKS(i3c->refclk_div));
>  	renesas_writel(i3c->regs, MSDVAD, MSDVAD_MDYADV |

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

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