Re: [PATCH 2/4] clk: renesas: cpg-mssr: Add table-driven MSTP dummy-read delay for LCDC on RZ/T2H

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Integer division truncation in RZT2H_MSTP_READS_TO_US causes insufficient dummy-read delays, violating hardware requirements.
- [Medium] The table-driven MSTP delay lookup logic contradicts its documentation and is structurally brittle, failing to support register-level wildcards or multiple IP blocks per register safely.
--

commit 45819a6f54fdfd758c7050cb72ae4d08ec929f51
Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

clk: renesas: cpg-mssr: Add table-driven MSTP dummy-read delay for LCDC on RZ/T2H

This commit introduces a table-driven mechanism to apply MSTP dummy-read
delays per-IP block on RZ/T2H. It replaces the hardcoded udelay(10) with
a computed delay based on the hardware manual's minimum dummy-read
requirements.

> diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
> index 26ea85cfaa027..19a96ca17d73a 100644
> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
> @@ -96,6 +96,24 @@ static const u16 mstpcr_for_gen4[] = {
>  #define RZT2H_MSTPCR_BLOCK(x)		((x) >> RZT2H_MSTPCR_BLOCK_SHIFT)
>  #define RZT2H_MSTPCR_OFFSET(x)		((x) & RZT2H_MSTPCR_OFFSET_MASK)
>  
> +/* Dummy read counts as specified by the RZ/T2H hardware manual */
> +#define RZT2H_MSTP_DEFAULT_DUMMY_READS	7
> +#define RZT2H_MSTP_LCDC_DUMMY_READS	100
> +
> +/*
> + * Time per dummy read in nanoseconds, derived from the original udelay(10)
> + * which was used to satisfy the 7 dummy-read requirement:
> + * 10000 ns / 7 reads = 1429 ns per read.
> + */
> +#define RZT2H_MSTP_DUMMY_READ_NS	1429
> +#define RZT2H_MSTP_READS_TO_US(n)	(((n) * RZT2H_MSTP_DUMMY_READ_NS) / 1000)

Does this macro truncate the division, resulting in an insufficient delay?

For the LCDC requirement of 100 reads, n = 100 evaluates to
142,900 / 1000 = 142 microseconds.

However, 142 us only covers 99.37 reads at 1429 ns per read.

This falls short of the hardware manual requirement of "at least 100 times"
mentioned in the commit message.

Should this use DIV_ROUND_UP to ensure the minimum delay is always met?

> +#define RZT2H_MSTP_DEFAULT_DELAY_US	RZT2H_MSTP_READS_TO_US(RZT2H_MSTP_DEFAULT_DUMMY_READS)
> +
> +#define RZT2H_MSTPCRM_INDEX		12
> +#define RZT2H_MSTPCRM04_LCDC		4
> +
> +#define RZT2H_MSTP_ANY_BIT		U32_MAX

[ ... ]

> +/**
> + * struct rzt2h_mstp_delay_entry - MSTP dummy-read requirement for RZ/T2H
> + *
> + * @reg: Index into control_regs[]. Exact match.
> + * @bit: MSTP bit position, or RZT2H_MSTP_ANY_BIT for register-level match.
> + * @delay_us: Computed delay in microseconds to satisfy the dummy read requirement.
> + */
> +struct rzt2h_mstp_delay_entry {
> +	u32 reg;
> +	u32 bit;
> +	u32 delay_us;
> +};

[ ... ]

> @@ -253,6 +300,20 @@ static void cpg_rzt2h_mstp_write(struct cpg_mssr_priv *priv, u16 offset, u32 val
>  	writel(value, base + RZT2H_MSTPCR_OFFSET(offset));
>  }
>  
> +static unsigned int cpg_rzt2h_mstp_get_delay_us(u32 reg, u32 bit)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(rzt2h_mstp_delay_table); i++) {
> +		const struct rzt2h_mstp_delay_entry *e = &rzt2h_mstp_delay_table[i];
> +
> +		if (e->reg == reg && (e->bit == bit || bit == RZT2H_MSTP_ANY_BIT))
> +			return e->delay_us;
> +	}
> +
> +	return RZT2H_MSTP_DEFAULT_DELAY_US;
> +}

Is the register-level match fully supported by this logic?

The struct documentation above states e->bit can be RZT2H_MSTP_ANY_BIT for a
register-level match.

However, the condition here checks if the passed bit argument is
RZT2H_MSTP_ANY_BIT, but does not check if e->bit is RZT2H_MSTP_ANY_BIT.

If the table contained an entry with e->bit set to RZT2H_MSTP_ANY_BIT,
it seems it would be ignored when querying for a specific bit.

Additionally, in cpg_mssr_resume_noirq(), the driver queries the delay by
passing RZT2H_MSTP_ANY_BIT.

Because this loop returns on the very first match, if multiple IPs in the
same register have custom requirements in the future, will it return the
delay of the first entry instead of the maximum required delay for the
entire register?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511191910.1945705-1-prabhakar.mahadev-lad.rj@bp.renesas.com?part=2