Re: [PATCH 08/12] rtc: rzn1: Dynamically calculate synchronization delay based on clock rate

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Uninitialized `rtc->sync_time` causing infinite polling loop and missing hardware delays when optional `xtal` clock is absent.
- [Low] Semantic unit mismatch using `NSEC_PER_MSEC` for microsecond duration calculation.
--

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

rtc: rzn1: Dynamically calculate synchronization delay based on clock rate

This commit replaces hardcoded hardware synchronization delays with a
dynamically calculated time window derived from the runtime clock rate.
It stores the calculated clock cycles in the driver private structure to
adjust sleep boundaries dynamically.

> diff --git a/drivers/rtc/rtc-rzn1.c b/drivers/rtc/rtc-rzn1.c
> index 06339adae71f6..bc6af59744e48 100644
> --- a/drivers/rtc/rtc-rzn1.c
> +++ b/drivers/rtc/rtc-rzn1.c
[ ... ]
> @@ -124,8 +125,8 @@ static int rzn1_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  		/* Hold the counter if it was counting up */
>  		writel(RZN1_RTC_CTL2_WAIT, rtc->base + RZN1_RTC_CTL2);
>  
> -		/* Wait for the counter to stop: two 32k clock cycles */
> -		usleep_range(61, 100);
> +		/* Wait for the counter to stop: two RTC_PCLK clock cycles */
> +		usleep_range(rtc->sync_time, rtc->sync_time + 100);

[Severity: High]
If the optional xtal clock is missing, rtc->sync_time remains 0 because the
calculation block in rzn1_rtc_probe() is skipped.

Does this evaluate to usleep_range(0, 100), bypassing the hardware's
required minimum 2-cycle synchronization wait period?

[ ... ]
> @@ -433,17 +434,25 @@ static int rzn1_rtc_probe(struct platform_device *pdev)
>  			ret = -EOPNOTSUPP;
>  			goto dis_runtime_pm;
>  		}
> -
>  		if (rate != 32768)
>  			scmp_val = RZN1_RTC_CTL0_SLSB_SCMP;
> +
> +		/*
> +		 * The internal clock counter operates in synchronization with the
> +		 * RTC_PCLK clock. Calculate the duration of two RTC_PCLK clock
> +		 * cycles in microseconds required for operations to complete.
> +		 */
> +		rtc->sync_time = DIV_ROUND_UP(2 * NSEC_PER_MSEC, rate);

[Severity: Low]
This isn't a bug, but the calculation uses NSEC_PER_MSEC to compute
microseconds. While mathematically equivalent to 1,000,000, could this
use USEC_PER_SEC to clearly reflect a Hz-to-microseconds conversion?

[Severity: High]
The driver retrieves the xtal clock using devm_clk_get_optional() earlier
in rzn1_rtc_probe(). If the clock is missing, the else if (xtal) block is
skipped entirely.

Does this leave rtc->sync_time uninitialized at 0 for the default internal
clock?

> +
>  	}
>  
>  	/* Disable controller during SUBU/SCMP setup */
>  	val = readl(rtc->base + RZN1_RTC_CTL0) & ~RZN1_RTC_CTL0_CE;
>  	writel(val, rtc->base + RZN1_RTC_CTL0);
> -	/* Wait 2-4 32k clock cycles for the disabled controller */
> +	/* 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), 62, 123);
> +				 !(val & RZN1_RTC_CTL0_CEST), rtc->sync_time,
> +				 rtc->sync_time * 2);

[Severity: High]
If rtc->sync_time is 0 from the missing optional clock, the timeout argument
(rtc->sync_time * 2) evaluates to 0.

Since readl_poll_timeout() treats a 0 timeout as explicitly disabling the
timeout check, will this turn the wait into an infinite tight loop if the
hardware fails to respond?

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