Message-ID: <2024091810261833316e25@mail.local>
Date: Wed, 18 Sep 2024 12:26:18 +0200
From: Alexandre Belloni <alexandre.belloni@...tlin.com>
To: Ciprian Costea <ciprianmarian.costea@....nxp.com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>, linux-rtc@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	NXP S32 Linux Team <s32@....com>,
	Bogdan Hamciuc <bogdan.hamciuc@....com>,
	Bogdan-Gabriel Roman <bogdan-gabriel.roman@....com>,
	Ghennadi Procopciuc <Ghennadi.Procopciuc@....com>
Subject: Re: [PATCH 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support

On 11/09/2024 10:00:26+0300, Ciprian Costea wrote:
> +struct rtc_time_base {
> +	s64 sec;
> +	u64 cycles;
> +	u64 rollovers;

The whole rollovers implementation seems useless as the rollover may
happen when Linux is off.

> +#ifdef CONFIG_PM_SLEEP
> +	struct rtc_time tm;
> +#endif
> +};
> +
> +struct rtc_priv {
> +	struct rtc_device *rdev;
> +	struct device *dev;
> +	u8 __iomem *rtc_base;
> +	struct clk *firc;
> +	struct clk *sirc;
> +	struct clk *ipg;
> +	struct rtc_time_base base;
> +	u64 rtc_hz;
> +	u64 rollovers;
> +	int dt_irq_id;
> +	u8 clk_source;
> +	bool div512;
> +	bool div32;
> +};
> +
> +static u64 cycles_to_sec(u64 hz, u64 cycles)
> +{
> +	return cycles / hz;
> +}
> +
> +/*
> + * Convert a number of seconds to a value suitable for RTCVAL in our clock's
> + * current configuration.
> + * @rtcval: The value to go into RTCVAL[RTCVAL]
> + * Returns: 0 for success, -EINVAL if @seconds push the counter at least
> + *          twice the rollover interval
> + */
> +static int sec_to_rtcval(const struct rtc_priv *priv,
> +			 unsigned long seconds, u32 *rtcval)
> +{
> +	u32 rtccnt, delta_cnt;
> +	u32 target_cnt = 0;
> +
> +	/* For now, support at most one rollover of the counter */
> +	if (!seconds || seconds > cycles_to_sec(priv->rtc_hz, ULONG_MAX))
> +		return -EINVAL;
> +
> +	/*
> +	 * RTCCNT is read-only; we must return a value relative to the
> +	 * current value of the counter (and hope we don't linger around
> +	 * too much before we get to enable the interrupt)
> +	 */
> +	delta_cnt = seconds * priv->rtc_hz;
> +	rtccnt = ioread32(priv->rtc_base + RTCCNT_OFFSET);
> +
> +	if (~rtccnt < delta_cnt)
> +		target_cnt = (delta_cnt - ~rtccnt);
> +	else
> +		target_cnt = rtccnt + delta_cnt;
> +
> +	/*
> +	 * According to RTCVAL register description,
> +	 * its minimum value should be 4.
> +	 */
> +	if (unlikely(target_cnt < 4))
> +		target_cnt = 4;
> +
> +	*rtcval = target_cnt;
> +
> +	return 0;
> +}
> +
> +static irqreturn_t rtc_handler(int irq, void *dev)
> +{
> +	struct rtc_priv *priv = platform_get_drvdata(dev);
> +	u32 status;
> +
> +	status = ioread32(priv->rtc_base + RTCS_OFFSET);
> +	if (status & RTCS_ROVRF) {
> +		if (priv->rollovers == ULONG_MAX)
> +			priv->rollovers = 0;
> +		else
> +			priv->rollovers++;
> +	}
> +
> +	if (status & RTCS_RTCF) {
> +		iowrite32(0x0, priv->rtc_base + RTCVAL_OFFSET);
> +		rtc_update_irq(priv->rdev, 1, RTC_AF);
> +	}
> +
> +	if (status & RTCS_APIF)
> +		rtc_update_irq(priv->rdev, 1, RTC_PF);
> +
> +	iowrite32(status, priv->rtc_base + RTCS_OFFSET);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int get_time_left(struct device *dev, struct rtc_priv *priv,
> +			 u32 *sec)
> +{
> +	u32 rtccnt = ioread32(priv->rtc_base + RTCCNT_OFFSET);
> +	u32 rtcval = ioread32(priv->rtc_base + RTCVAL_OFFSET);
> +
> +	if (rtcval < rtccnt) {
> +		dev_err(dev, "RTC timer expired before entering suspend\n");
> +		return -EIO;
> +	}
> +
> +	*sec = cycles_to_sec(priv->rtc_hz, rtcval - rtccnt);
> +
> +	return 0;
> +}
> +
> +static int s32g_rtc_get_time_or_alrm(struct rtc_priv *priv,
> +				     u32 offset)
> +{
> +	u64 cycles, sec, base_cycles;
> +	u32 counter;
> +
> +	counter = ioread32(priv->rtc_base + offset);
> +	cycles = priv->rollovers * ROLLOVER_VAL + counter;
> +	base_cycles = priv->base.cycles + priv->base.rollovers * ROLLOVER_VAL;
> +
> +	if (cycles < base_cycles)
> +		return -EINVAL;
> +
> +	cycles -= base_cycles;
> +	sec = priv->base.sec + cycles_to_sec(priv->rtc_hz, cycles);

What happens after you reboot?

This doesn't seem to be a proper RTC, unless you have some NVMEM to
store the offset between the current time and the value of the counter.
As-is, the driver is not working.

> +
> +	return sec;
> +}
> +
> +static int s32g_rtc_read_time(struct device *dev,
> +			      struct rtc_time *tm)
> +{
> +	struct rtc_priv *priv = dev_get_drvdata(dev);
> +	u64 sec;
> +
> +	if (!tm)
> +		return -EINVAL;
> +
> +	sec = s32g_rtc_get_time_or_alrm(priv, RTCCNT_OFFSET);
> +	if (sec < 0)
> +		return -EINVAL;
> +
> +	rtc_time64_to_tm(sec, tm);
> +
> +	return 0;
> +}
> +
> +static int s32g_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct rtc_priv *priv = dev_get_drvdata(dev);
> +	u32 rtcc, sec_left;
> +	u64 sec;
> +
> +	if (!alrm)
> +		return -EINVAL;
> +
> +	sec = s32g_rtc_get_time_or_alrm(priv, RTCVAL_OFFSET);
> +	if (sec < 0)
> +		return -EINVAL;
> +
> +	rtc_time64_to_tm(sec, &alrm->time);
> +
> +	rtcc = ioread32(priv->rtc_base + RTCC_OFFSET);
> +	alrm->enabled = sec && (rtcc & RTCC_RTCIE);
> +
> +	alrm->pending = 0;
> +	if (alrm->enabled && !get_time_left(dev, priv, &sec_left))
> +		alrm->pending = !!sec_left;
> +
> +	return 0;
> +}
> +
> +static int s32g_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
> +{
> +	struct rtc_priv *priv = dev_get_drvdata(dev);
> +	u32 rtcc;
> +
> +	if (!priv->dt_irq_id)
> +		return -EIO;
> +
> +	/*
> +	 * RTCIE cannot be deasserted because it will also disable the
> +	 * rollover interrupt.
> +	 */
> +	rtcc = ioread32(priv->rtc_base + RTCC_OFFSET);
> +	if (enabled)
> +		rtcc |= RTCC_RTCIE;
> +
> +	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
> +
> +	return 0;
> +}
> +
> +static int s32g_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct rtc_priv *priv = dev_get_drvdata(dev);
> +	struct rtc_time time_crt;
> +	long long t_crt, t_alrm;
> +	int ret = 0;
> +	u32 rtcval, rtcs;
> +
> +	iowrite32(0x0, priv->rtc_base + RTCVAL_OFFSET);
> +
> +	t_alrm = rtc_tm_to_time64(&alrm->time);
> +
> +	/*
> +	 * Assuming the alarm is being set relative to the same time
> +	 * returned by our s32g_rtc_read_time callback
> +	 */
> +	ret = s32g_rtc_read_time(dev, &time_crt);
> +	if (ret)
> +		return ret;
> +
> +	t_crt = rtc_tm_to_time64(&time_crt);
> +	if (t_alrm <= t_crt) {
> +		dev_warn(dev, "Alarm is set in the past\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = sec_to_rtcval(priv, t_alrm - t_crt, &rtcval);
> +	if (ret) {
> +		dev_warn(dev, "Alarm is set too far in the future\n");
> +		return ret;
> +	}
> +
> +	ret = read_poll_timeout(ioread32, rtcs, !(rtcs & RTCS_INV_RTC),
> +				0, RTC_SYNCH_TIMEOUT, false, priv->rtc_base + RTCS_OFFSET);
> +	if (ret) {
> +		dev_err(dev, "Synchronization failed\n");
> +		return ret;
> +	}
> +
> +	iowrite32(rtcval, priv->rtc_base + RTCVAL_OFFSET);
> +
> +	return 0;
> +}
> +
> +static int s32g_rtc_set_time(struct device *dev,
> +			     struct rtc_time *time)
> +{
> +	struct rtc_priv *priv = dev_get_drvdata(dev);
> +
> +	if (!time)
> +		return -EINVAL;
> +
> +	priv->base.rollovers = priv->rollovers;
> +	priv->base.cycles = ioread32(priv->rtc_base + RTCCNT_OFFSET);
> +	priv->base.sec = rtc_tm_to_time64(time);
> +
> +	return 0;
> +}
> +
> +static const struct rtc_class_ops rtc_ops = {
> +	.read_time = s32g_rtc_read_time,
> +	.set_time = s32g_rtc_set_time,
> +	.read_alarm = s32g_rtc_read_alarm,
> +	.set_alarm = s32g_rtc_set_alarm,
> +	.alarm_irq_enable = s32g_rtc_alarm_irq_enable,
> +};
> +
> +static void rtc_disable(struct rtc_priv *priv)
> +{
> +	u32 rtcc = ioread32(priv->rtc_base + RTCC_OFFSET);
> +
> +	rtcc &= ~RTCC_CNTEN;
> +	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
> +}
> +
> +static void rtc_enable(struct rtc_priv *priv)
> +{
> +	u32 rtcc = ioread32(priv->rtc_base + RTCC_OFFSET);
> +
> +	rtcc |= RTCC_CNTEN;
> +	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
> +}
> +
> +static int rtc_init(struct rtc_priv *priv)
> +{
> +	struct device *dev = priv->dev;
> +	struct clk *sclk;
> +	u32 rtcc = 0;
> +	u32 clksel;
> +	int ret;
> +
> +	ret = clk_prepare_enable(priv->ipg);
> +	if (ret) {
> +		dev_err(dev, "Cannot enable 'ipg' clock, error: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(priv->sirc);
> +	if (ret) {
> +		dev_err(dev, "Cannot enable 'sirc' clock, error: %d\n", ret);
> +		goto disable_ipg_clk;
> +	}
> +
> +	ret = clk_prepare_enable(priv->firc);
> +	if (ret) {
> +		dev_err(dev, "Cannot enable 'firc' clock, error: %d\n", ret);
> +		goto disable_sirc_clk;
> +	}
> +
> +	/* Make sure the RTC ticking is disabled before we configure dividers */
> +	rtc_disable(priv);

At this point, you lost the RTC accuracy, this must not be done on probe
> +
> +	clksel = RTCC_CLKSEL(priv->clk_source);
> +	rtcc |= clksel;
> +
> +	/* Precompute the base frequency of the clock */
> +	switch (clksel) {
> +	case RTCC_CLKSEL(S32G_RTC_SOURCE_SIRC):
> +		sclk = priv->sirc;
> +		break;
> +	case RTCC_CLKSEL(S32G_RTC_SOURCE_FIRC):
> +		sclk = priv->firc;
> +		break;
> +	default:
> +		dev_err(dev, "Invalid clksel value: %u\n", clksel);
> +		ret = -EINVAL;
> +		goto disable_firc_clk;
> +	}
> +
> +	priv->rtc_hz = clk_get_rate(sclk);
> +	if (!priv->rtc_hz) {
> +		dev_err(dev, "Invalid RTC frequency\n");
> +		ret = -EINVAL;
> +		goto disable_firc_clk;
> +	}
> +
> +	/* Adjust frequency if dividers are enabled */
> +	if (priv->div512) {
> +		rtcc |= RTCC_DIV512EN;
> +		priv->rtc_hz /= 512;
> +	}
> +
> +	if (priv->div32) {
> +		rtcc |= RTCC_DIV32EN;
> +		priv->rtc_hz /= 32;
> +	}
> +
> +	rtcc |= RTCC_RTCIE | RTCC_ROVREN;
> +	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
> +
> +	return 0;
> +
> +disable_firc_clk:
> +	clk_disable_unprepare(priv->firc);
> +disable_sirc_clk:
> +	clk_disable_unprepare(priv->sirc);
> +disable_ipg_clk:
> +	clk_disable_unprepare(priv->ipg);
> +	return ret;
> +}
> +
> +static int priv_dts_init(struct rtc_priv *priv)
> +{
> +	struct device *dev = priv->dev;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	u32 clksel = S32G_RTC_SOURCE_SIRC;
> +	/* div512 and div32 */
> +	u32 div[2] = { 0 };
> +	int ret;
> +
> +	priv->sirc = devm_clk_get(dev, "sirc");
> +	if (IS_ERR(priv->sirc)) {
> +		dev_err(dev, "Failed to get 'sirc' clock\n");
> +		return PTR_ERR(priv->sirc);
> +	}
> +
> +	priv->firc = devm_clk_get(dev, "firc");
> +	if (IS_ERR(priv->firc)) {
> +		dev_err(dev, "Failed to get 'firc' clock\n");
> +		return PTR_ERR(priv->firc);
> +	}
> +
> +	priv->ipg = devm_clk_get(dev, "ipg");
> +	if (IS_ERR(priv->ipg)) {
> +		dev_err(dev, "Failed to get 'ipg' clock\n");
> +		return PTR_ERR(priv->ipg);
> +	}
> +
> +	priv->dt_irq_id = platform_get_irq(pdev, 0);
> +	if (priv->dt_irq_id < 0) {
> +		dev_err(dev, "Error reading interrupt # from dts\n");
> +		return priv->dt_irq_id;
> +	}
> +
> +	ret = device_property_read_u32_array(dev, "nxp,dividers", div,
> +					     ARRAY_SIZE(div));
> +	if (ret) {
> +		dev_err(dev, "Error reading dividers configuration, err: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = device_property_read_u32(dev, "nxp,clksel", &clksel);
> +	if (ret) {
> +		dev_err(dev, "Error reading clksel configuration, err: %d\n", ret);
> +		return ret;
> +	}
> +
> +	priv->div512 = !!div[0];
> +	priv->div32 = !!div[1];
> +
> +	switch (clksel) {
> +	case S32G_RTC_SOURCE_SIRC:
> +	case S32G_RTC_SOURCE_FIRC:
> +		priv->clk_source = clksel;
> +		break;
> +	default:
> +		dev_err(dev, "Unsupported clksel: %d\n", clksel);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int rtc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct rtc_priv *priv;
> +	int ret = 0;
> +
> +	priv = devm_kzalloc(dev, sizeof(struct rtc_priv),
> +			    GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->rtc_base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->rtc_base)) {
> +		dev_err(dev, "Failed to map registers\n");
> +		return PTR_ERR(priv->rtc_base);
> +	}
> +
> +	device_init_wakeup(dev, true);
> +	priv->dev = dev;
> +
> +	ret = priv_dts_init(priv);
> +	if (ret)
> +		return ret;
> +
> +	ret = rtc_init(priv);
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, priv);
> +	rtc_enable(priv);
> +
> +	priv->rdev = devm_rtc_device_register(dev, "s32g_rtc",
> +					      &rtc_ops, THIS_MODULE);

This is deprecated, please use devm_rtc_allocate_device and devm_rtc_register_device

> +	if (IS_ERR_OR_NULL(priv->rdev)) {
> +		dev_err(dev, "Error registering RTC device, err: %ld\n",
> +			PTR_ERR(priv->rdev));
> +		ret = PTR_ERR(priv->rdev);
> +		goto disable_rtc;
> +	}
> +
> +	ret = devm_request_irq(dev, priv->dt_irq_id,
> +			       rtc_handler, 0, dev_name(dev), pdev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Request interrupt %d failed, error: %d\n",
> +			priv->dt_irq_id, ret);
> +		goto disable_rtc;

You must not fail after registering the RTC, else your driver will be
opened to a race condition;

> +	}
> +
> +	return 0;
> +
> +disable_rtc:
> +	rtc_disable(priv);
> +	return ret;
> +}
> +
> +static void rtc_remove(struct platform_device *pdev)
> +{
> +	struct rtc_priv *priv = platform_get_drvdata(pdev);
> +
> +	rtc_disable(priv);

Disabling the RTC when shutting down renders the RTC useless.

> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static void enable_api_irq(struct device *dev, unsigned int enabled)
> +{
> +	struct rtc_priv *priv = dev_get_drvdata(dev);
> +	u32 api_irq = RTCC_APIEN | RTCC_APIIE;
> +	u32 rtcc;
> +
> +	rtcc = ioread32(priv->rtc_base + RTCC_OFFSET);
> +	if (enabled)
> +		rtcc |= api_irq;
> +	else
> +		rtcc &= ~api_irq;
> +	iowrite32(rtcc, priv->rtc_base + RTCC_OFFSET);
> +}
> +
> +static int adjust_dividers(u32 sec, struct rtc_priv *priv)
> +{
> +	u64 rtcval_max = U32_MAX;
> +	u64 rtcval;
> +
> +	priv->div32 = 0;
> +	priv->div512 = 0;
> +
> +	rtcval = sec * priv->rtc_hz;
> +	if (rtcval < rtcval_max)
> +		return 0;
> +
> +	if (rtcval / 32 < rtcval_max) {
> +		priv->div32 = 1;
> +		return 0;
> +	}
> +
> +	if (rtcval / 512 < rtcval_max) {
> +		priv->div512 = 1;
> +		return 0;
> +	}
> +
> +	if (rtcval / (512 * 32) < rtcval_max) {
> +		priv->div32 = 1;
> +		priv->div512 = 1;
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int rtc_suspend(struct device *dev)
> +{
> +	struct rtc_priv *init_priv = dev_get_drvdata(dev);
> +	struct rtc_priv priv;
> +	long long base_sec;
> +	int ret = 0;
> +	u32 rtcval;
> +	u32 sec;
> +
> +	if (!device_may_wakeup(dev))
> +		return 0;
> +
> +	/* Save last known timestamp before we switch clocks and reinit RTC */
> +	ret = s32g_rtc_read_time(dev, &priv.base.tm);
> +	if (ret)
> +		return ret;
> +
> +	if (init_priv->clk_source == S32G_RTC_SOURCE_SIRC)
> +		return 0;
> +
> +	/*
> +	 * Use a local copy of the RTC control block to
> +	 * avoid restoring it on resume path.
> +	 */
> +	memcpy(&priv, init_priv, sizeof(priv));
> +
> +	/* Switch to SIRC */
> +	priv.clk_source = S32G_RTC_SOURCE_SIRC;
> +
> +	ret = get_time_left(dev, init_priv, &sec);
> +	if (ret)
> +		return ret;
> +
> +	/* Adjust for the number of seconds we'll be asleep */
> +	base_sec = rtc_tm_to_time64(&init_priv->base.tm);
> +	base_sec += sec;
> +	rtc_time64_to_tm(base_sec, &init_priv->base.tm);
> +
> +	rtc_disable(&priv);
> +
> +	ret = adjust_dividers(sec, &priv);
> +	if (ret) {
> +		dev_err(dev, "Failed to adjust RTC dividers to match a %u seconds delay\n", sec);
> +		return ret;
> +	}
> +
> +	ret = rtc_init(&priv);
> +	if (ret)
> +		return ret;
> +
> +	ret = sec_to_rtcval(&priv, sec, &rtcval);
> +	if (ret) {
> +		dev_warn(dev, "Alarm is too far in the future\n");
> +		return ret;
> +	}

All of this seems super fishy.

> +
> +	s32g_rtc_alarm_irq_enable(dev, 0);
> +	enable_api_irq(dev, 1);
> +	iowrite32(rtcval, priv.rtc_base + APIVAL_OFFSET);
> +	iowrite32(0, priv.rtc_base + RTCVAL_OFFSET);
> +
> +	rtc_enable(&priv);
> +
> +	return ret;
> +}
> +
> +static int rtc_resume(struct device *dev)
> +{
> +	struct rtc_priv *priv = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (!device_may_wakeup(dev))
> +		return 0;
> +
> +	/* Disable wake-up interrupts */
> +	enable_api_irq(dev, 0);
> +
> +	/* Reinitialize the driver using the initial settings */
> +	ret = rtc_init(priv);
> +	if (ret)
> +		return ret;
> +
> +	rtc_enable(priv);
> +
> +	/*
> +	 * Now RTCCNT has just been reset, and is out of sync with priv->base;
> +	 * reapply the saved time settings
> +	 */
> +	return s32g_rtc_set_time(dev, &priv->base.tm);
> +}
> +#endif /* CONFIG_PM_SLEEP */
> +
> +static const struct of_device_id rtc_dt_ids[] = {
> +	{.compatible = "nxp,s32g-rtc" },
> +	{ /* sentinel */ },
> +};
> +
> +static SIMPLE_DEV_PM_OPS(rtc_pm_ops,
> +			 rtc_suspend, rtc_resume);
> +
> +static struct platform_driver rtc_driver = {
> +	.driver		= {
> +		.name			= "s32g-rtc",
> +		.pm				= &rtc_pm_ops,
> +		.of_match_table = of_match_ptr(rtc_dt_ids),
> +	},
> +	.probe		= rtc_probe,
> +	.remove_new	= rtc_remove,
> +};
> +module_platform_driver(rtc_driver);
> +
> +MODULE_AUTHOR("NXP");
> +MODULE_DESCRIPTION("NXP RTC driver for S32G2/S32G3");
> +MODULE_LICENSE("GPL");
> -- 
> 2.45.2
> 
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Powered by blists - more mailing lists