Re: [PATCH v11 3/3] rtc: sophgo: cv18xx: Add restart handler

From: Alexandre Belloni <alexandre.belloni@bootlin.com>
To: Alexander Sverdlin <alexander.sverdlin@gmail.com>
Cc: devicetree@vger.kernel.org, linux-riscv@lists.infradead.org,
	linux-rtc@vger.kernel.org, Inochi Amaoto <inochiama@gmail.com>,
	robh@kernel.org, krzysztof.kozlowski+dt@linaro.org,
	conor+dt@kernel.org, unicorn_wang@outlook.com,
	inochiama@outlook.com, paul.walmsley@sifive.com,
	palmer@dabbelt.com, aou@eecs.berkeley.edu, dlan@gentoo.org,
	linux-kernel@vger.kernel.org,
	Jingbao Qiu <qiujingbao.dlmu@gmail.com>,
	Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Subject: Re: [PATCH v11 3/3] rtc: sophgo: cv18xx: Add restart handler
Date: Fri, 14 Feb 2025 00:00:17 +0100	[thread overview]
Message-ID: <20250213230017eaa89034@mail.local> (raw)
In-Reply-To: <20250213215655.2311793-4-alexander.sverdlin@gmail.com>

On 13/02/2025 22:56:47+0100, Alexander Sverdlin wrote:
> Currently provided ATF firmware for ARM64 Sophgo SoCs is binary-only.
> To make Linux independent from this binary firmware, provide restart
> handler triggering COLD or WARM reboot directly via RTC HW block as
> alternative to PSCI interface.
> 
> Handler priority SYS_OFF_PRIO_DEFAULT of 0 is desired, because if PSCI
> is indeed present, its priority of 129 will override the new one. On RiskV
> side the behaviour is unchanged as well because SBI SRST handler has higher
> priority as well (192).
> 
> Tested on SG2000 in ARM64 mode.
> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> ---
> v11:
> - patch introduced;
> 
>  drivers/rtc/rtc-cv1800.c | 51 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-cv1800.c b/drivers/rtc/rtc-cv1800.c
> index 5c30f7388983..586d7195f1f4 100644
> --- a/drivers/rtc/rtc-cv1800.c
> +++ b/drivers/rtc/rtc-cv1800.c
> @@ -9,17 +9,23 @@
>  #include <linux/irq.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/notifier.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> +#include <linux/reboot.h>
>  #include <linux/regmap.h>
>  #include <linux/rtc.h>
>  
> +#define CTRL_UNLOCKKEY         0x0004
> +#define CTRL0                  0x0008
>  #define SEC_PULSE_GEN          0x1004
>  #define ALARM_TIME             0x1008
>  #define ALARM_ENABLE           0x100C
>  #define SET_SEC_CNTR_VAL       0x1010
>  #define SET_SEC_CNTR_TRIG      0x1014
>  #define SEC_CNTR_VAL           0x1018
> +#define EN_PWR_CYC_REQ         0x10C8
> +#define EN_WARM_RST_REQ        0x10CC
>  
>  /*
>   * When in VDDBKUP domain, this MACRO register
> @@ -28,6 +34,10 @@
>  #define MACRO_RO_T             0x14A8
>  #define MACRO_RG_SET_T         0x1498
>  
> +/* CTRL0 bits */
> +#define REQ_PWR_CYC            BIT(3)
> +#define REQ_WARM_RST           BIT(4)
> +
>  #define ALARM_ENABLE_MASK      BIT(0)
>  #define SEL_SEC_PULSE          BIT(31)
>  
> @@ -160,6 +170,29 @@ static const struct rtc_class_ops cv1800_rtc_ops = {
>  	.alarm_irq_enable = cv1800_rtc_alarm_irq_enable,
>  };
>  
> +static int cv1800_restart_handler(struct sys_off_data *data)
> +{
> +	struct cv1800_rtc_priv *info = data->cb_data;
> +	u32 reg_en = EN_WARM_RST_REQ;
> +	u32 request = 0xFFFF0800;
> +
> +	if (data->mode == REBOOT_COLD) {
> +		reg_en = EN_PWR_CYC_REQ;
> +		request |= REQ_PWR_CYC;
> +	} else {
> +		request |= REQ_WARM_RST;
> +	}
> +
> +	/* Enable reset request */
> +	regmap_write(info->rtc_map, reg_en, 1);
> +	/* Enable CTRL0 register access */
> +	regmap_write(info->rtc_map, CTRL_UNLOCKKEY, 0xAB18);
> +	/* Request reset */
> +	regmap_write(info->rtc_map, CTRL0, request);
> +
> +	return NOTIFY_DONE;
> +}
> +
>  static int cv1800_rtc_probe(struct platform_device *pdev)
>  {
>  	struct cv1800_rtc_priv *rtc;
> @@ -205,7 +238,23 @@ static int cv1800_rtc_probe(struct platform_device *pdev)
>  		return dev_err_probe(&pdev->dev, ret,
>  				     "cannot register interrupt handler\n");
>  
> -	return devm_rtc_register_device(rtc->rtc_dev);
> +	ret = devm_rtc_register_device(rtc->rtc_dev);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "cannot register RTC\n");

This message is useless, there is no failure path without a message in
devm_rtc_register_device.

> +
> +	/*
> +	 * SYS_OFF_PRIO_DEFAULT of 0 is desired here because both RiscV SBI SRST
> +	 * (prio 192) and ARM PSCI (prio 129) will override this one; this
> +	 * handler shall serve as a fallback in cases where firmware is not
> +	 * present.
> +	 */
> +	ret = devm_register_restart_handler(&pdev->dev, cv1800_restart_handler,
> +					    rtc);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "cannot register restart handler\n");

Probe must never fail after devm_rtc_register_device.

> +
> +	return 0;
>  }
>  
>  static const struct of_device_id cv1800_dt_ids[] = {
> -- 
> 2.48.1
> 

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