Re: [PATCH v2 10/10] 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:
- [High] The driver does not enforce SCMP mode for the new RZ/T2H compatible, allowing an unsafe fallback to SUBU mode that accesses the unimplemented RTCA0SUBU register.
- [Medium] The driver clears the entire `RZN1_RTC_CTL1` register to disable interrupts, which inadvertently destroys the state of the RZ/T2H-specific 1 Hz output control bit.
--

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

rtc: rzn1: Add support for Renesas RZ/T2H and RZ/N2H SoCs
    
Add support for the RTC block found on the Renesas RZ/T2H (R9A09G077)
and RZ/N2H (R9A09G087) SoCs by matching the new
renesas,r9a09g077-rtc compatible string.

>     These SoCs integrate a closely related variant of the RZ/N1 RTC IP.
>     Unlike the RZ/N1 implementation, the RZ/T2H and RZ/N2H variants lack
>     the RTCA0SUBU register. However, this register is not accessed when
>     operating in SCMP mode using the existing rzn1_rtc_ops_scmp
>     configuration, allowing the current driver infrastructure to be reused
>     without modification.

[Severity: High]
Does the current driver infrastructure guarantee SCMP mode is always used?

If the xtal clock is missing (since devm_clk_get_optional() is used) or
if its rate is exactly 32768 Hz, scmp_val remains 0:

drivers/rtc/rtc-rzn1.c:rzn1_rtc_probe() {
	...
	xtal = devm_clk_get_optional(dev, "xtal");
	if (IS_ERR(xtal)) {
		...
	} else if (xtal) {
		rate = clk_get_rate(xtal);
		...
		if (rate != 32768)
			scmp_val = RZN1_RTC_CTL0_SLSB_SCMP;
	}
	...

This causes the driver to fall back to the SUBU ops:

	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 this happens, the SUBU ops will access the missing RTCA0SUBU register,
which can trigger a synchronous external abort. Should the driver use OF match
data to identify variants lacking the SUBU register and explicitly prevent
assigning rzn1_rtc_ops_subu?

>     The RZ/T2H RTC variant also supports a 1 Hz output signal on the
>     RTCAT1HZ pin, controlled by the RTCA0CTL1[RTCA01HZE] bit. This bit is
>     marked as reserved in the RZ/N1 hardware manual, making RZ/T2H a
>     distinct RTC variant despite its overall compatibility with the RZ/N1
>     implementation.

[Severity: Medium]
Will this 1 Hz output signal be inadvertently disabled during probe and removal?

During probe and removal, the driver unconditionally clears the entire
RZN1_RTC_CTL1 register to disable interrupts:

drivers/rtc/rtc-rzn1.c:rzn1_rtc_probe() {
	...
	/* Disable all interrupts */
	writel(0, rtc->base + RZN1_RTC_CTL1);
	...

If the bootloader or firmware previously configured the 1 Hz output signal,
won't this unconditional write to 0 destroy that state? Could the driver
perform a read-modify-write operation to clear only the interrupt enable bits
instead?

> diff --git a/drivers/rtc/rtc-rzn1.c b/drivers/rtc/rtc-rzn1.c
> index e77b7e7bcd5bd..26a5cde41c6b2 100644
> --- a/drivers/rtc/rtc-rzn1.c
> +++ b/drivers/rtc/rtc-rzn1.c
> @@ -518,6 +518,7 @@ static void rzn1_rtc_remove(struct platform_device *pdev)
>  }
>  
>  static const struct of_device_id rzn1_rtc_of_match[] = {
> +	{ .compatible	= "renesas,r9a09g077-rtc" },
>  	{ .compatible	= "renesas,rzn1-rtc" },
>  	{},
>  };

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