Re: [PATCH 12/12] rtc: rzn1: Add support for Renesas RZ/T2H and RZ/N2H SoCs

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing validation allows RZ/T2H variants to fall back to SUBU mode, which accesses physically non-existent registers, potentially causing an SError or Data Abort.

Pre-existing issues:
- [High] If the optional `xtal` clock is omitted in the device tree, `rtc->sync_time` remains 0, leading to infinite tight-polling in `readl_poll_timeout`.
--

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

rtc: rzn1: Add support for Renesas RZ/T2H and RZ/N2H SoCs

Add a new compatible string "renesas,r9a09g077-rtc" to the OF match table
to support the RTC IP variant found on the RZ/T2H and RZ/N2H SoCs. These
newer SoCs integrate a closely related variant of the RZ/N1 RTC IP but lack
the RTCA0SUBU and RTCA0TCR registers.

> diff --git a/drivers/rtc/rtc-rzn1.c b/drivers/rtc/rtc-rzn1.c
> index 9f9cf9882fc49..dfff8dc8c3212 100644
> --- a/drivers/rtc/rtc-rzn1.c
> +++ b/drivers/rtc/rtc-rzn1.c
> @@ -597,6 +597,7 @@ static int rzn1_rtc_resume(struct device *dev)
>  static DEFINE_SIMPLE_DEV_PM_OPS(rzn1_rtc_pm_ops, rzn1_rtc_suspend, rzn1_rtc_resume);
>  
>  static const struct of_device_id rzn1_rtc_of_match[] = {
> +	{ .compatible	= "renesas,r9a09g077-rtc" },

[Severity: High]
The commit message notes that the new RZ/T2H variants lack the RTCA0SUBU
register and rely on rzn1_rtc_ops_scmp configurations. However, is it safe to
rely on the clock configuration to select between SCMP and SUBU operations?

In rzn1_rtc_probe(), the fallback to SUBU mode is based solely on the xtal
clock:

drivers/rtc/rtc-rzn1.c:rzn1_rtc_probe() {
	if (xtal) {
		rate = clk_get_rate(xtal);
		...
		if (rate != 32768)
			scmp_val = RZN1_RTC_CTL0_SLSB_SCMP;
		...
	}

	if (scmp_val) {
		writel(rate - 1, rtc->base + RZN1_RTC_SCMP);
		rtc->rtcdev->ops = &rzn1_rtc_ops_scmp;
	} else {
		rtc->rtcdev->ops = &rzn1_rtc_ops_subu;
	}
}

If the device tree misconfigures the clock (e.g., rate is exactly 32768) or
omits the optional xtal clock entirely, scmp_val will be 0. The driver will
dynamically assign rzn1_rtc_ops_subu as the ops.

Subsequent operations like rzn1_rtc_read_offset() will directly access the
missing register:

drivers/rtc/rtc-rzn1.c:rzn1_rtc_read_offset() {
	val = readl(rtc->base + RZN1_RTC_SUBU);
}

Could this cause an unhandled SError or Data Abort when accessing the
physically non-existent SUBU register on RZ/T2H? Should the driver explicitly
enforce the use of SCMP operations for this compatible string regardless of the
clock configuration?


[Severity: High]
This is a pre-existing issue, but if the optional xtal clock is omitted in the
device tree, rtc->sync_time remains 0 since it is initialized by devm_kzalloc()
and the calculation block is skipped.

Later in rzn1_rtc_probe():

	/* Wait 2-4 RTC_PCLK clock cycles for the disabled controller to stop */
	ret = readl_poll_timeout(rtc->base + RZN1_RTC_CTL0, val,
				 !(val & RZN1_RTC_CTL0_CEST), rtc->sync_time,
				 rtc->sync_time * 2);

Passing a timeout of 0 to readl_poll_timeout() instructs it to never time out.
Combined with a sleep_us of 0, could this result in an infinite tight loop
if the hardware condition !(val & RZN1_RTC_CTL0_CEST) is never met during
probe?

>  	{ .compatible	= "renesas,rzn1-rtc" },
>  	{},
>  };

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