diff mbox series

realtek: do not hardcode restart handler and enable gpio-restart

Message ID 20211005133525.31113-1-fercerpav@gmail.com
State Changes Requested
Delegated to: Petr Štetiar
Headers show
Series realtek: do not hardcode restart handler and enable gpio-restart | expand

Commit Message

Paul Fertser Oct. 5, 2021, 1:35 p.m. UTC 
Most boards supported by this target have a dedicated GPIO line
connected to SoC's /RESET to allow full clean reliable reboot.

Use regular kernel notifier methods for default restart routines to
allow higher-priority handlers (such as gpio-restart) to override it.

Signed-off-by: Paul Fertser <fercerpav@gmail.com>
---
 target/linux/realtek/config-5.10              |  1 +
 .../files-5.10/arch/mips/rtl838x/setup.c      | 75 ++++++++++++-------
 2 files changed, 48 insertions(+), 28 deletions(-)

Comments

Sander Vanheule Oct. 5, 2021, 8:59 p.m. UTC | #1 
Hi Paul,

On Tue, 2021-10-05 at 16:35 +0300, Paul Fertser wrote:
> Most boards supported by this target have a dedicated GPIO line
> connected to SoC's /RESET to allow full clean reliable reboot.
> 
> Use regular kernel notifier methods for default restart routines to
> allow higher-priority handlers (such as gpio-restart) to override it.
> 
> Signed-off-by: Paul Fertser <fercerpav@gmail.com>

This looks like a good improvement to the current restart handling, I think. However, I
also suspect that this logic should actually go into in a separate reset driver, and a
clock driver to handle the PLL. The reset/PLL registers are in the switch-core register
space and we don't have any mainline drivers there yet, so I don't know how we're supposed
to access the jumble of functionality there. As far as I'm concerned though, this can all
stay together in one place for now.

One alternative could be to create some extra driver(s) in OpenWrt already, to get rid of
setup.c (and prom.c at some later point) and work towards the upstream base for
RTL838x/RTL839x. Anyone else with an opinion on this?

I also see that you're removing the _machine_halt function, while this isn't mentioned in
the commit message. I had already removed _machine_halt in f4b687d1f053 ("realtek: use
kernel defined halt"), but apparently it quietly got reintroduced in 8faffa00cb6b
("realtek: add support for the RTL9300 timer").

> ---
>  target/linux/realtek/config-5.10              |  1 +
>  .../files-5.10/arch/mips/rtl838x/setup.c      | 75 ++++++++++++-------
>  2 files changed, 48 insertions(+), 28 deletions(-)
> 

[...]

>  
>  static void __init rtl838x_setup(void)
>  {
> -       pr_info("Registering _machine_restart\n");
> -       _machine_restart = rtl838x_restart;
> -       _machine_halt = rtl838x_halt;
> +       pr_info("Registering rtl838x_restart_notify\n");

I don't think it's very useful to log this, not at the INFO level anyway. Maybe just drop
this, and the ones for the other SoC families.

Best,
Sander
Birger Koblitz Oct. 5, 2021, 9:22 p.m. UTC | #2 
Hi,

On 05/10/2021 22:59, Sander Vanheule wrote:
> One alternative could be to create some extra driver(s) in OpenWrt already, to get rid of
> setup.c (and prom.c at some later point) and work towards the upstream base for
> RTL838x/RTL839x. Anyone else with an opinion on this?
That's a good idea. But I think that one should also already take the work by brainslayer into
account which adds a lot to the initial SoC configuration:
https://github.com/BrainSlayer/pie/tree/pie-5.10-rtl9313/target/linux/realtek/files-5.10/arch/mips/rtl838x
What we put in now should already anticipate the use of SMP (from 839x onwards), and for the 931x the GIC as the new
Interrupt controller, large memory support and the GIC timer.

> 
> I also see that you're removing the _machine_halt function, while this isn't mentioned in
> the commit message. I had already removed _machine_halt in f4b687d1f053 ("realtek: use
> kernel defined halt"), but apparently it quietly got reintroduced in 8faffa00cb6b
> ("realtek: add support for the RTL9300 timer")Sh**, my bad.

Birger
diff mbox series

Patch

diff --git a/target/linux/realtek/config-5.10 b/target/linux/realtek/config-5.10
index 1b050fc08140..84281df316fd 100644
--- a/target/linux/realtek/config-5.10
+++ b/target/linux/realtek/config-5.10
@@ -156,6 +156,7 @@  CONFIG_PHYLIB=y
 CONFIG_PHYLINK=y
 CONFIG_PINCTRL=y
 CONFIG_POWER_RESET=y
+CONFIG_POWER_RESET_GPIO_RESTART=y
 CONFIG_POWER_RESET_SYSCON=y
 CONFIG_RATIONAL=y
 CONFIG_REALTEK_PHY=y
diff --git a/target/linux/realtek/files-5.10/arch/mips/rtl838x/setup.c b/target/linux/realtek/files-5.10/arch/mips/rtl838x/setup.c
index 752a85643728..a2de760321de 100644
--- a/target/linux/realtek/files-5.10/arch/mips/rtl838x/setup.c
+++ b/target/linux/realtek/files-5.10/arch/mips/rtl838x/setup.c
@@ -9,6 +9,7 @@ 
  *
  */
 
+#include <linux/reboot.h>
 #include <linux/console.h>
 #include <linux/init.h>
 #include <linux/clkdev.h>
@@ -29,12 +30,20 @@ 
 
 extern struct rtl83xx_soc_info soc_info;
 
+static struct notifier_block realtek_restart_handler = {
+        .priority = 128,
+};
+
 u32 pll_reset_value;
 
-static void rtl838x_restart(char *command)
+static int rtl838x_restart_notify(struct notifier_block *this,
+				  unsigned long mode, void *cmd)
 {
 	u32 pll = sw_r32(RTL838X_PLL_CML_CTRL);
 
+	if (mode != SYS_RESTART)
+		return NOTIFY_DONE;
+
 	pr_info("System restart.\n");
 	pr_info("PLL control register: %x, applying reset value %x\n",
 		pll, pll_reset_value);
@@ -45,35 +54,48 @@  static void rtl838x_restart(char *command)
 
 	/* Reset Global Control1 Register */
 	sw_w32(1, RTL838X_RST_GLB_CTRL_1);
+
+	return NOTIFY_DONE;
 }
 
-static void rtl839x_restart(char *command)
+static int rtl839x_restart_notify(struct notifier_block *this,
+				  unsigned long mode, void *cmd)
 {
 	/* SoC reset vector (in flash memory): on RTL839x platform preferred way to reset */
 	void (*f)(void) = (void *) 0xbfc00000;
 
+	if (mode != SYS_RESTART)
+		return NOTIFY_DONE;
+
 	pr_info("System restart.\n");
 	/* Reset SoC */
 	sw_w32(0xFFFFFFFF, RTL839X_RST_GLB_CTRL);
 	/* and call reset vector */
 	f();
-	/* If this fails, halt the CPU */
-	while
-		(1);
+
+	return NOTIFY_DONE;
 }
 
-static void rtl930x_restart(char *command)
+static int rtl930x_restart_notify(struct notifier_block *this,
+				  unsigned long mode, void *cmd)
 {
+	if (mode != SYS_RESTART)
+		return NOTIFY_DONE;
+
 	pr_info("System restart.\n");
 	sw_w32(0x1, RTL930X_RST_GLB_CTRL_0);
-	while
-		(1);
+
+	return NOTIFY_DONE;
 }
 
-static void rtl931x_restart(char *command)
+static int rtl931x_restart_notify(struct notifier_block *this,
+				  unsigned long mode, void *cmd)
 {
 	u32 v;
 
+	if (mode != SYS_RESTART)
+		return NOTIFY_DONE;
+
 	pr_info("System restart.\n");
 	sw_w32(1, RTL931X_RST_GLB_CTRL);
 	v = sw_r32(RTL931X_RST_GLB_CTRL);
@@ -82,20 +104,14 @@  static void rtl931x_restart(char *command)
 	sw_w32(v, RTL931X_RST_GLB_CTRL);
 	msleep(15);
 	sw_w32(0x101, RTL931X_RST_GLB_CTRL);
-}
 
-static void rtl838x_halt(void)
-{
-	pr_info("System halted.\n");
-	while
-		(1);
+	return NOTIFY_DONE;
 }
 
 static void __init rtl838x_setup(void)
 {
-	pr_info("Registering _machine_restart\n");
-	_machine_restart = rtl838x_restart;
-	_machine_halt = rtl838x_halt;
+	pr_info("Registering rtl838x_restart_notify\n");
+	realtek_restart_handler.notifier_call = rtl838x_restart_notify;
 
 	/* This PLL value needs to be restored before a reset and will then be
 	 * preserved over a SoC reset. A wrong value prevents the SoC from
@@ -109,9 +125,8 @@  static void __init rtl838x_setup(void)
 
 static void __init rtl839x_setup(void)
 {
-	pr_info("Registering _machine_restart\n");
-	_machine_restart = rtl839x_restart;
-	_machine_halt = rtl838x_halt;
+	pr_info("Registering rtl839x_restart_notify\n");
+	realtek_restart_handler.notifier_call = rtl839x_restart_notify;
 
 	/* Setup System LED. Bit 14 of RTL839X_LED_GLB_CTRL then allows to toggle it */
 	sw_w32_mask(0, 3 << 15, RTL839X_LED_GLB_CTRL);
@@ -119,9 +134,8 @@  static void __init rtl839x_setup(void)
 
 static void __init rtl930x_setup(void)
 {
-	pr_info("Registering _machine_restart\n");
-	_machine_restart = rtl930x_restart;
-	_machine_halt = rtl838x_halt;
+	pr_info("Registering rtl930x_restart_notify\n");
+	realtek_restart_handler.notifier_call = rtl930x_restart_notify;
 
 	if (soc_info.id == 0x9302)
 		sw_w32_mask(0, 3 << 13, RTL9302_LED_GLB_CTRL);
@@ -131,18 +145,17 @@  static void __init rtl930x_setup(void)
 
 static void __init rtl931x_setup(void)
 {
-	pr_info("Registering _machine_restart\n");
-	_machine_restart = rtl931x_restart;
-	_machine_halt = rtl838x_halt;
+	pr_info("Registering rtl931x_restart_notify\n");
+	realtek_restart_handler.notifier_call = rtl931x_restart_notify;
 	sw_w32_mask(0, 3 << 12, RTL931X_LED_GLB_CTRL);
 }
 
 void __init plat_mem_setup(void)
 {
 	void *dtb;
+	int ret;
 
 	set_io_port_base(KSEG1);
-	_machine_restart = rtl838x_restart;
 
 	if (fw_passed_dtb) /* UHI interface */
 		dtb = (void *)fw_passed_dtb;
@@ -171,6 +184,12 @@  void __init plat_mem_setup(void)
 		rtl931x_setup();
 		break;
 	}
+
+	ret = register_restart_handler(&realtek_restart_handler);
+	if (ret) {
+		pr_err("%s: cannot register restart handler, %d\n",
+		       __func__, ret);
+	}
 }
 
 void __init plat_time_init(void)