[PATCH 0/3] Add Additional Boards and Features to RGxx3

[PATCH 0/3] Add Additional Boards and Features to RGxx3

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

The RGxx3 is a pseudo-device for U-Boot that works for every Anbernic
RGxx3 series device on the market. Add support for another series of
very similar devices from Powkiddy.

Chris Morgan (3):
  board: rockchip: Refactor panel auto-detect code
  board: rockchip: Add Maskrom Mode for Anbernic RGxx3
  board: rockchip: Add support for RGB30 and RK2023 to RGxx3

 board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c | 195 ++++++++++++++++-----
 1 file changed, 154 insertions(+), 41 deletions(-)

-- 
2.34.1

[PATCH 1/3] board: rockchip: Refactor panel auto-detect code

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

Make the inability to detect a panel using the auto detection code not
fail the entire boot process. This means that if the panel ID cannot
be read we don't set an environment variable for the panel, and if an
environment variable for the panel is not set we don't attempt to
update the compatible string. Changes to the code also ensure that
when there are multiple compatible strings required for the panel
we use them both, which solves some issues that will pop up soon
for the Linux driver.

Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
---
 board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c | 115 +++++++++++++--------
 1 file changed, 74 insertions(+), 41 deletions(-)

diff --git a/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c b/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
index 3f1a42d184..3d0c614623 100644
--- a/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
+++ b/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
@@ -40,6 +40,7 @@ struct rg3xx_model {
 	const char *board;
 	const char *board_name;
 	const char *fdtfile;
+	const bool detect_panel;
 };
 
 enum rgxx3_device_id {
@@ -54,52 +55,67 @@ enum rgxx3_device_id {
 
 static const struct rg3xx_model rg3xx_model_details[] = {
 	[RG353M] = {
-		517, /* Observed average from device */
-		"rk3566-anbernic-rg353m",
-		"RG353M",
-		DTB_DIR "rk3566-anbernic-rg353p.dtb", /* Identical devices */
+		.adc_value = 517, /* Observed average from device */
+		.board = "rk3566-anbernic-rg353m",
+		.board_name = "RG353M",
+		/* Device is identical to RG353P. */
+		.fdtfile = DTB_DIR "rk3566-anbernic-rg353p.dtb",
+		.detect_panel = 1,
 	},
 	[RG353P] = {
-		860, /* Documented value of 860 */
-		"rk3566-anbernic-rg353p",
-		"RG353P",
-		DTB_DIR "rk3566-anbernic-rg353p.dtb",
+		.adc_value = 860, /* Documented value of 860 */
+		.board = "rk3566-anbernic-rg353p",
+		.board_name = "RG353P",
+		.fdtfile = DTB_DIR "rk3566-anbernic-rg353p.dtb",
+		.detect_panel = 1,
 	},
 	[RG353V] = {
-		695, /* Observed average from device */
-		"rk3566-anbernic-rg353v",
-		"RG353V",
-		DTB_DIR "rk3566-anbernic-rg353v.dtb",
+		.adc_value = 695, /* Observed average from device */
+		.board = "rk3566-anbernic-rg353v",
+		.board_name = "RG353V",
+		.fdtfile = DTB_DIR "rk3566-anbernic-rg353v.dtb",
+		.detect_panel = 1,
 	},
 	[RG503] = {
-		1023, /* Observed average from device */
-		"rk3566-anbernic-rg503",
-		"RG503",
-		DTB_DIR "rk3566-anbernic-rg503.dtb",
+		.adc_value = 1023, /* Observed average from device */
+		.board = "rk3566-anbernic-rg503",
+		.board_name = "RG503",
+		.fdtfile = DTB_DIR "rk3566-anbernic-rg503.dtb",
+		.detect_panel = 0,
 	},
 	/* Devices with duplicate ADC value */
 	[RG353PS] = {
-		860, /* Observed average from device */
-		"rk3566-anbernic-rg353ps",
-		"RG353PS",
-		DTB_DIR "rk3566-anbernic-rg353ps.dtb",
+		.adc_value = 860, /* Observed average from device */
+		.board = "rk3566-anbernic-rg353ps",
+		.board_name = "RG353PS",
+		.fdtfile = DTB_DIR "rk3566-anbernic-rg353ps.dtb",
+		.detect_panel = 1,
 	},
 	[RG353VS] = {
-		695, /* Gathered from second hand information */
-		"rk3566-anbernic-rg353vs",
-		"RG353VS",
-		DTB_DIR "rk3566-anbernic-rg353vs.dtb",
+		.adc_value = 695, /* Gathered from second hand information */
+		.board = "rk3566-anbernic-rg353vs",
+		.board_name = "RG353VS",
+		.fdtfile = DTB_DIR "rk3566-anbernic-rg353vs.dtb",
+		.detect_panel = 1,
 	},
 };
 
 struct rg353_panel {
 	const u16 id;
-	const char *panel_compat;
+	const char *panel_compat[2];
 };
 
 static const struct rg353_panel rg353_panel_details[] = {
-	{ .id = 0x3052, .panel_compat = "newvision,nv3051d"},
-	{ .id = 0x3821, .panel_compat = "anbernic,rg353v-panel-v2"},
+	{
+		.id = 0x3052,
+		.panel_compat[0] = "anbernic,rg353p-panel",
+		.panel_compat[1] = "newvision,nv3051d",
+	},
+	{
+		.id = 0x3821,
+		.panel_compat[0] = "anbernic,rg353v-panel-v2",
+		.panel_compat[1] = NULL,
+	},
 };
 
 /*
@@ -298,11 +314,10 @@ int rgxx3_detect_display(void)
 	if (!panel) {
 		printf("Unable to identify panel_id %x\n",
 		       (panel_id[0] << 8) | panel_id[1]);
-		env_set("panel", "unknown");
 		return -EINVAL;
 	}
 
-	env_set("panel", panel->panel_compat);
+	env_set("panel", panel->panel_compat[0]);
 
 	return 0;
 }
@@ -367,13 +382,14 @@ int rgxx3_detect_device(void)
 		rg3xx_model_details[board_id].board_name);
 	env_set("fdtfile", rg3xx_model_details[board_id].fdtfile);
 
-	/* Detect the panel type for any device that isn't a 503. */
-	if (board_id == RG503)
+	/* Skip panel detection for when it is not needed. */
+	if (!rg3xx_model_details[board_id].detect_panel)
 		return 0;
 
+	/* Warn but don't fail for errors in auto-detection of the panel. */
 	ret = rgxx3_detect_display();
 	if (ret)
-		return ret;
+		printf("Failed to detect panel type\n");
 
 	return 0;
 }
@@ -400,7 +416,8 @@ int rk_board_late_init(void)
 
 int ft_board_setup(void *blob, struct bd_info *bd)
 {
-	int node, ret;
+	const struct rg353_panel *panel = NULL;
+	int node, ret, i;
 	char *env;
 
 	/* No fixups necessary for the RG503 */
@@ -414,6 +431,12 @@ int ft_board_setup(void *blob, struct bd_info *bd)
 			    rg3xx_model_details[RG353M].board_name,
 			    sizeof(rg3xx_model_details[RG353M].board_name));
 
+	env = env_get("panel");
+	if (!env) {
+		printf("Can't get panel env\n");
+		return 0;
+	}
+
 	/*
 	 * Check if the environment variable doesn't equal the panel.
 	 * If it doesn't, update the devicetree to the correct panel.
@@ -424,12 +447,6 @@ int ft_board_setup(void *blob, struct bd_info *bd)
 		return -ENODEV;
 	}
 
-	env = env_get("panel");
-	if (!env) {
-		printf("Can't get panel env\n");
-		return -ENODEV;
-	}
-
 	ret = fdt_node_check_compatible(blob, node, env);
 	if (ret < 0)
 		return -ENODEV;
@@ -438,8 +455,24 @@ int ft_board_setup(void *blob, struct bd_info *bd)
 	if (!ret)
 		return 0;
 
-	do_fixup_by_path_string(blob, "/dsi@fe060000/panel@0",
-				"compatible", env);
+	/* Panels don't match, search by first compatible value. */
+	for (i = 0; i < ARRAY_SIZE(rg353_panel_details); i++) {
+		if (!strcmp(env, rg353_panel_details[i].panel_compat[0])) {
+			panel = &rg353_panel_details[i];
+			break;
+		}
+	}
+
+	if (!panel) {
+		printf("Unable to identify panel by compat string\n");
+		return -ENODEV;
+	}
+
+	/* Set the compatible with the auto-detected values */
+	fdt_setprop_string(blob, node, "compatible", panel->panel_compat[0]);
+	if (panel->panel_compat[1])
+		fdt_appendprop_string(blob, node, "compatible",
+				      panel->panel_compat[1]);
 
 	return 0;
 }
-- 
2.34.1

[PATCH 2/3] board: rockchip: Add Maskrom Mode for Anbernic RGxx3

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

Add support for users to enter maskrom mode by holding the function
button when they power up the device.

Since the device has soldered eMMC and sometimes does not expose a clk
pin on the mainboard there is a small chance that a user who flashes a
bad bootloader may not be able to recover if the headers themselves
are valid. As a result this check is done during spl_early_init() to
ensure that it runs as early as possible, and it does so by directly
manipulating the ADC hardware in lieu of loading the ADC driver.

Ideally, once we have an open source TPL stage we can move this to
the TPL stage, so it will run even earlier.

Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
---
 board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c | 64 ++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c b/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
index 3d0c614623..a93b11cd47 100644
--- a/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
+++ b/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
@@ -6,12 +6,14 @@
 #include <abuf.h>
 #include <adc.h>
 #include <asm/io.h>
+#include <command.h>
 #include <display.h>
 #include <dm.h>
 #include <dm/lists.h>
 #include <env.h>
 #include <fdt_support.h>
 #include <linux/delay.h>
+#include <linux/iopoll.h>
 #include <mipi_dsi.h>
 #include <mmc.h>
 #include <panel.h>
@@ -20,6 +22,8 @@
 #include <stdlib.h>
 #include <video_bridge.h>
 
+#define BOOT_BROM_DOWNLOAD	0xef08a53c
+
 #define GPIO0_BASE		0xfdd60000
 #define GPIO4_BASE		0xfe770000
 #define GPIO_SWPORT_DR_L	0x0000
@@ -33,6 +37,14 @@
 
 #define GPIO_WRITEMASK(bits)	((bits) << 16)
 
+#define SARADC_BASE		0xfe720000
+#define SARADC_DATA		0x0000
+#define SARADC_STAS		0x0004
+#define SARADC_ADC_STATUS	BIT(0)
+#define SARADC_CTRL		0x0008
+#define SARADC_INPUT_SRC_MSK	0x7
+#define SARADC_POWER_CTRL	BIT(3)
+
 #define DTB_DIR			"rockchip/"
 
 struct rg3xx_model {
@@ -118,12 +130,64 @@ static const struct rg353_panel rg353_panel_details[] = {
 	},
 };
 
+/*
+ * The device has internal eMMC, and while some devices have an exposed
+ * clk pin you can ground to force a bypass not all devices do. As a
+ * result it may be possible for some devices to become a perma-brick
+ * if a corrupted TPL or SPL stage with a valid header is flashed to
+ * the internal eMMC. Add functionality to read ADC channel 0 (the func
+ * button) as early as possible in the boot process to provide some
+ * protection against this. If we ever get an open TPL stage, we should
+ * consider moving this function there.
+ */
+void read_func_button(void)
+{
+	int ret;
+	u32 reg;
+
+	/* Turn off SARADC to reset it. */
+	writel(0, (SARADC_BASE + SARADC_CTRL));
+
+	/* Enable channel 0 and power on SARADC. */
+	writel(((0 & SARADC_INPUT_SRC_MSK) | SARADC_POWER_CTRL),
+	       (SARADC_BASE + SARADC_CTRL));
+
+	/*
+	 * Wait for data to be ready. Use timeout of 20000us from
+	 * rockchip_saradc driver.
+	 */
+	ret = readl_poll_timeout((SARADC_BASE + SARADC_STAS), reg,
+				 !(reg & SARADC_ADC_STATUS), 20000);
+	if (ret) {
+		printf("ADC Timeout");
+		return;
+	}
+
+	/* Read the data from the SARADC. */
+	reg = readl((SARADC_BASE + SARADC_DATA));
+
+	/* Turn the SARADC back off so it's ready to be used again. */
+	writel(0, (SARADC_BASE + SARADC_CTRL));
+
+	/*
+	 * If the value is less than 30 the button is being pressed.
+	 * Reset the device back into Rockchip download mode.
+	 */
+	if (reg <= 30) {
+		printf("download key pressed, entering download mode...");
+		writel(BOOT_BROM_DOWNLOAD, CONFIG_ROCKCHIP_BOOT_MODE_REG);
+		do_reset(NULL, 0, 0, NULL);
+	}
+};
+
 /*
  * Start LED very early so user knows device is on. Set color
  * to red.
  */
 void spl_board_init(void)
 {
+	read_func_button();
+
 	/* Set GPIO0_C5, GPIO0_C6, and GPIO0_C7 to output. */
 	writel(GPIO_WRITEMASK(GPIO_C7 | GPIO_C6 | GPIO_C5) | \
 	       (GPIO_C7 | GPIO_C6 | GPIO_C5),
-- 
2.34.1

[PATCH 3/3] board: rockchip: Add support for RGB30 and RK2023 to RGxx3

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

Add support for the Powkiddy RK2023 and RGB30 to the Anbernic RGxx3.
While these devices are manufactured by Powkiddy instead of Anbernic,
the hardware is so similar they can all use the same bootloader.

Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
---
 board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c b/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
index a93b11cd47..dae9dc87c2 100644
--- a/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
+++ b/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
@@ -60,6 +60,8 @@ enum rgxx3_device_id {
 	RG353P,
 	RG353V,
 	RG503,
+	RGB30,
+	RK2023,
 	/* Devices with duplicate ADC value */
 	RG353PS,
 	RG353VS,
@@ -95,6 +97,20 @@ static const struct rg3xx_model rg3xx_model_details[] = {
 		.fdtfile = DTB_DIR "rk3566-anbernic-rg503.dtb",
 		.detect_panel = 0,
 	},
+	[RGB30] = {
+		.adc_value = 383, /* Gathered from second hand information */
+		.board = "rk3566-powkiddy-rgb30",
+		.board_name = "RGB30",
+		.fdtfile = DTB_DIR "rk3566-powkiddy-rgb30.dtb",
+		.detect_panel = 0,
+	},
+	[RK2023] = {
+		.adc_value = 635, /* Observed average from device */
+		.board = "rk3566-powkiddy-rk2023",
+		.board_name = "RK2023",
+		.fdtfile = DTB_DIR "rk3566-powkiddy-rk2023.dtb",
+		.detect_panel = 0,
+	},
 	/* Devices with duplicate ADC value */
 	[RG353PS] = {
 		.adc_value = 860, /* Observed average from device */
-- 
2.34.1

Re: [PATCH 3/3] board: rockchip: Add support for RGB30 and RK2023 to RGxx3

[Kever Yang]

On 2023/10/18 02:24, Chris Morgan wrote:
> From: Chris Morgan <macromorgan@hotmail.com>
>
> Add support for the Powkiddy RK2023 and RGB30 to the Anbernic RGxx3.
> While these devices are manufactured by Powkiddy instead of Anbernic,
> the hardware is so similar they can all use the same bootloader.
>
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever

> ---
>   board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
>
> diff --git a/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c b/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
> index a93b11cd47..dae9dc87c2 100644
> --- a/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
> +++ b/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
> @@ -60,6 +60,8 @@ enum rgxx3_device_id {
>   	RG353P,
>   	RG353V,
>   	RG503,
> +	RGB30,
> +	RK2023,
>   	/* Devices with duplicate ADC value */
>   	RG353PS,
>   	RG353VS,
> @@ -95,6 +97,20 @@ static const struct rg3xx_model rg3xx_model_details[] = {
>   		.fdtfile = DTB_DIR "rk3566-anbernic-rg503.dtb",
>   		.detect_panel = 0,
>   	},
> +	[RGB30] = {
> +		.adc_value = 383, /* Gathered from second hand information */
> +		.board = "rk3566-powkiddy-rgb30",
> +		.board_name = "RGB30",
> +		.fdtfile = DTB_DIR "rk3566-powkiddy-rgb30.dtb",
> +		.detect_panel = 0,
> +	},
> +	[RK2023] = {
> +		.adc_value = 635, /* Observed average from device */
> +		.board = "rk3566-powkiddy-rk2023",
> +		.board_name = "RK2023",
> +		.fdtfile = DTB_DIR "rk3566-powkiddy-rk2023.dtb",
> +		.detect_panel = 0,
> +	},
>   	/* Devices with duplicate ADC value */
>   	[RG353PS] = {
>   		.adc_value = 860, /* Observed average from device */

Re: [PATCH 2/3] board: rockchip: Add Maskrom Mode for Anbernic RGxx3

[Kever Yang]

Hi Chris,

On 2023/10/18 02:24, Chris Morgan wrote:
> From: Chris Morgan <macromorgan@hotmail.com>
>
> Add support for users to enter maskrom mode by holding the function
> button when they power up the device.
>
> Since the device has soldered eMMC and sometimes does not expose a clk
> pin on the mainboard

This board already available, so does this clk pin available?

Not only clk, other cmd/data pin is also OK to make the soc get into 
maskrom mode.

> there is a small chance that a user who flashes a
> bad bootloader may not be able to recover if the headers themselves
> are valid. As a result this check is done during spl_early_init() to
> ensure that it runs as early as possible, and it does so by directly
> manipulating the ADC hardware in lieu of loading the ADC driver.

This key seems like a "recovery" adc key instead of "maskrom" key, right?

I think we don't need to add this patch if the hardware does not really 
have a "maskrom" key.

Basically the "maskrom" key should not depends on any software.


Thanks,

- Kever

>
> Ideally, once we have an open source TPL stage we can move this to
> the TPL stage, so it will run even earlier.
>
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> ---
>   board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c | 64 ++++++++++++++++++++++
>   1 file changed, 64 insertions(+)
>
> diff --git a/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c b/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
> index 3d0c614623..a93b11cd47 100644
> --- a/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
> +++ b/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
> @@ -6,12 +6,14 @@
>   #include <abuf.h>
>   #include <adc.h>
>   #include <asm/io.h>
> +#include <command.h>
>   #include <display.h>
>   #include <dm.h>
>   #include <dm/lists.h>
>   #include <env.h>
>   #include <fdt_support.h>
>   #include <linux/delay.h>
> +#include <linux/iopoll.h>
>   #include <mipi_dsi.h>
>   #include <mmc.h>
>   #include <panel.h>
> @@ -20,6 +22,8 @@
>   #include <stdlib.h>
>   #include <video_bridge.h>
>   
> +#define BOOT_BROM_DOWNLOAD	0xef08a53c
> +
>   #define GPIO0_BASE		0xfdd60000
>   #define GPIO4_BASE		0xfe770000
>   #define GPIO_SWPORT_DR_L	0x0000
> @@ -33,6 +37,14 @@
>   
>   #define GPIO_WRITEMASK(bits)	((bits) << 16)
>   
> +#define SARADC_BASE		0xfe720000
> +#define SARADC_DATA		0x0000
> +#define SARADC_STAS		0x0004
> +#define SARADC_ADC_STATUS	BIT(0)
> +#define SARADC_CTRL		0x0008
> +#define SARADC_INPUT_SRC_MSK	0x7
> +#define SARADC_POWER_CTRL	BIT(3)
> +
>   #define DTB_DIR			"rockchip/"
>   
>   struct rg3xx_model {
> @@ -118,12 +130,64 @@ static const struct rg353_panel rg353_panel_details[] = {
>   	},
>   };
>   
> +/*
> + * The device has internal eMMC, and while some devices have an exposed
> + * clk pin you can ground to force a bypass not all devices do. As a
> + * result it may be possible for some devices to become a perma-brick
> + * if a corrupted TPL or SPL stage with a valid header is flashed to
> + * the internal eMMC. Add functionality to read ADC channel 0 (the func
> + * button) as early as possible in the boot process to provide some
> + * protection against this. If we ever get an open TPL stage, we should
> + * consider moving this function there.
> + */
> +void read_func_button(void)
> +{
> +	int ret;
> +	u32 reg;
> +
> +	/* Turn off SARADC to reset it. */
> +	writel(0, (SARADC_BASE + SARADC_CTRL));
> +
> +	/* Enable channel 0 and power on SARADC. */
> +	writel(((0 & SARADC_INPUT_SRC_MSK) | SARADC_POWER_CTRL),
> +	       (SARADC_BASE + SARADC_CTRL));
> +
> +	/*
> +	 * Wait for data to be ready. Use timeout of 20000us from
> +	 * rockchip_saradc driver.
> +	 */
> +	ret = readl_poll_timeout((SARADC_BASE + SARADC_STAS), reg,
> +				 !(reg & SARADC_ADC_STATUS), 20000);
> +	if (ret) {
> +		printf("ADC Timeout");
> +		return;
> +	}
> +
> +	/* Read the data from the SARADC. */
> +	reg = readl((SARADC_BASE + SARADC_DATA));
> +
> +	/* Turn the SARADC back off so it's ready to be used again. */
> +	writel(0, (SARADC_BASE + SARADC_CTRL));
> +
> +	/*
> +	 * If the value is less than 30 the button is being pressed.
> +	 * Reset the device back into Rockchip download mode.
> +	 */
> +	if (reg <= 30) {
> +		printf("download key pressed, entering download mode...");
> +		writel(BOOT_BROM_DOWNLOAD, CONFIG_ROCKCHIP_BOOT_MODE_REG);
> +		do_reset(NULL, 0, 0, NULL);
> +	}
> +};
> +
>   /*
>    * Start LED very early so user knows device is on. Set color
>    * to red.
>    */
>   void spl_board_init(void)
>   {
> +	read_func_button();
> +
>   	/* Set GPIO0_C5, GPIO0_C6, and GPIO0_C7 to output. */
>   	writel(GPIO_WRITEMASK(GPIO_C7 | GPIO_C6 | GPIO_C5) | \
>   	       (GPIO_C7 | GPIO_C6 | GPIO_C5),

Re: [PATCH 2/3] board: rockchip: Add Maskrom Mode for Anbernic RGxx3

[Chris Morgan]

On Mon, Oct 23, 2023 at 06:10:07PM +0800, Kever Yang wrote:
> Hi Chris,
> 
> On 2023/10/18 02:24, Chris Morgan wrote:
> > From: Chris Morgan <macromorgan@hotmail.com>
> > 
> > Add support for users to enter maskrom mode by holding the function
> > button when they power up the device.
> > 
> > Since the device has soldered eMMC and sometimes does not expose a clk
> > pin on the mainboard
> 
> This board already available, so does this clk pin available?
> 
> Not only clk, other cmd/data pin is also OK to make the soc get into maskrom
> mode.

No, as near as I can tell some boards (such as the 353P and 353V) don't
expose a clk, cmd, or data pin. The 353M does expose a clk pin though.

> 
> > there is a small chance that a user who flashes a
> > bad bootloader may not be able to recover if the headers themselves
> > are valid. As a result this check is done during spl_early_init() to
> > ensure that it runs as early as possible, and it does so by directly
> > manipulating the ADC hardware in lieu of loading the ADC driver.
> 
> This key seems like a "recovery" adc key instead of "maskrom" key, right?
> 
> I think we don't need to add this patch if the hardware does not really have
> a "maskrom" key.
> 
> Basically the "maskrom" key should not depends on any software.

Okay, so should I rename it as a "recovery" key? Basically it's just
a button hooked up to ADC channel 0. I'm trying to make as simple a
failsafe as possible, because for the boards without the clk/cmd pads
I want to make it easy to debrick in the event of something like a
bad U-Boot stage or a bad A-TF stage.

Thank you.

> 
> 
> Thanks,
> 
> - Kever
> 
> > 
> > Ideally, once we have an open source TPL stage we can move this to
> > the TPL stage, so it will run even earlier.
> > 
> > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> > ---
> >   board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c | 64 ++++++++++++++++++++++
> >   1 file changed, 64 insertions(+)
> > 
> > diff --git a/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c b/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
> > index 3d0c614623..a93b11cd47 100644
> > --- a/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
> > +++ b/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
> > @@ -6,12 +6,14 @@
> >   #include <abuf.h>
> >   #include <adc.h>
> >   #include <asm/io.h>
> > +#include <command.h>
> >   #include <display.h>
> >   #include <dm.h>
> >   #include <dm/lists.h>
> >   #include <env.h>
> >   #include <fdt_support.h>
> >   #include <linux/delay.h>
> > +#include <linux/iopoll.h>
> >   #include <mipi_dsi.h>
> >   #include <mmc.h>
> >   #include <panel.h>
> > @@ -20,6 +22,8 @@
> >   #include <stdlib.h>
> >   #include <video_bridge.h>
> > +#define BOOT_BROM_DOWNLOAD	0xef08a53c
> > +
> >   #define GPIO0_BASE		0xfdd60000
> >   #define GPIO4_BASE		0xfe770000
> >   #define GPIO_SWPORT_DR_L	0x0000
> > @@ -33,6 +37,14 @@
> >   #define GPIO_WRITEMASK(bits)	((bits) << 16)
> > +#define SARADC_BASE		0xfe720000
> > +#define SARADC_DATA		0x0000
> > +#define SARADC_STAS		0x0004
> > +#define SARADC_ADC_STATUS	BIT(0)
> > +#define SARADC_CTRL		0x0008
> > +#define SARADC_INPUT_SRC_MSK	0x7
> > +#define SARADC_POWER_CTRL	BIT(3)
> > +
> >   #define DTB_DIR			"rockchip/"
> >   struct rg3xx_model {
> > @@ -118,12 +130,64 @@ static const struct rg353_panel rg353_panel_details[] = {
> >   	},
> >   };
> > +/*
> > + * The device has internal eMMC, and while some devices have an exposed
> > + * clk pin you can ground to force a bypass not all devices do. As a
> > + * result it may be possible for some devices to become a perma-brick
> > + * if a corrupted TPL or SPL stage with a valid header is flashed to
> > + * the internal eMMC. Add functionality to read ADC channel 0 (the func
> > + * button) as early as possible in the boot process to provide some
> > + * protection against this. If we ever get an open TPL stage, we should
> > + * consider moving this function there.
> > + */
> > +void read_func_button(void)
> > +{
> > +	int ret;
> > +	u32 reg;
> > +
> > +	/* Turn off SARADC to reset it. */
> > +	writel(0, (SARADC_BASE + SARADC_CTRL));
> > +
> > +	/* Enable channel 0 and power on SARADC. */
> > +	writel(((0 & SARADC_INPUT_SRC_MSK) | SARADC_POWER_CTRL),
> > +	       (SARADC_BASE + SARADC_CTRL));
> > +
> > +	/*
> > +	 * Wait for data to be ready. Use timeout of 20000us from
> > +	 * rockchip_saradc driver.
> > +	 */
> > +	ret = readl_poll_timeout((SARADC_BASE + SARADC_STAS), reg,
> > +				 !(reg & SARADC_ADC_STATUS), 20000);
> > +	if (ret) {
> > +		printf("ADC Timeout");
> > +		return;
> > +	}
> > +
> > +	/* Read the data from the SARADC. */
> > +	reg = readl((SARADC_BASE + SARADC_DATA));
> > +
> > +	/* Turn the SARADC back off so it's ready to be used again. */
> > +	writel(0, (SARADC_BASE + SARADC_CTRL));
> > +
> > +	/*
> > +	 * If the value is less than 30 the button is being pressed.
> > +	 * Reset the device back into Rockchip download mode.
> > +	 */
> > +	if (reg <= 30) {
> > +		printf("download key pressed, entering download mode...");
> > +		writel(BOOT_BROM_DOWNLOAD, CONFIG_ROCKCHIP_BOOT_MODE_REG);
> > +		do_reset(NULL, 0, 0, NULL);
> > +	}
> > +};
> > +
> >   /*
> >    * Start LED very early so user knows device is on. Set color
> >    * to red.
> >    */
> >   void spl_board_init(void)
> >   {
> > +	read_func_button();
> > +
> >   	/* Set GPIO0_C5, GPIO0_C6, and GPIO0_C7 to output. */
> >   	writel(GPIO_WRITEMASK(GPIO_C7 | GPIO_C6 | GPIO_C5) | \
> >   	       (GPIO_C7 | GPIO_C6 | GPIO_C5),

Re: [PATCH 2/3] board: rockchip: Add Maskrom Mode for Anbernic RGxx3

[Jonas Karlman]

Hi Chris,

On 2023-10-23 17:18, Chris Morgan wrote:
> On Mon, Oct 23, 2023 at 06:10:07PM +0800, Kever Yang wrote:
>> Hi Chris,
>>
>> On 2023/10/18 02:24, Chris Morgan wrote:
>>> From: Chris Morgan <macromorgan@hotmail.com>
>>>
>>> Add support for users to enter maskrom mode by holding the function
>>> button when they power up the device.
>>>
>>> Since the device has soldered eMMC and sometimes does not expose a clk
>>> pin on the mainboard
>>
>> This board already available, so does this clk pin available?
>>
>> Not only clk, other cmd/data pin is also OK to make the soc get into maskrom
>> mode.
> 
> No, as near as I can tell some boards (such as the 353P and 353V) don't
> expose a clk, cmd, or data pin. The 353M does expose a clk pin though.
> 
>>
>>> there is a small chance that a user who flashes a
>>> bad bootloader may not be able to recover if the headers themselves
>>> are valid. As a result this check is done during spl_early_init() to
>>> ensure that it runs as early as possible, and it does so by directly
>>> manipulating the ADC hardware in lieu of loading the ADC driver.
>>
>> This key seems like a "recovery" adc key instead of "maskrom" key, right?
>>
>> I think we don't need to add this patch if the hardware does not really have
>> a "maskrom" key.
>>
>> Basically the "maskrom" key should not depends on any software.
> 
> Okay, so should I rename it as a "recovery" key? Basically it's just
> a button hooked up to ADC channel 0. I'm trying to make as simple a
> failsafe as possible, because for the boards without the clk/cmd pads
> I want to make it easy to debrick in the event of something like a
> bad U-Boot stage or a bad A-TF stage.

Isn't this very similar to what arch/arm/mach-rockchip/boot_mode.c and
rockchip_dnl_key_pressed() already does, or is supposed to do?

Maybe this existing code can be changed/fixed to work earlier in SPL and
possible also read recovery button information from DT or Kconfig to
work with more rockchip boards?

Would be nice if a hang/panic in SPL make rk devices reset into maskrom
usb mode. Maybe even the watchdog can be of some use here.

Btw, you could probably also enable CONFIG_SPL_FIT_SIGNATURE for added
protection against starting corrupt/bitrot FIT images. To speed up boot
after adding a sha256 checksum test, you can use following rfc/patch:

[RFC] rockchip: spl: Enable caches to speed up checksum validation
https://patchwork.ozlabs.org/patch/1802303/

Regards,
Jonas

> 
> Thank you.
> 
>>
>>
>> Thanks,
>>
>> - Kever
>>
>>>
>>> Ideally, once we have an open source TPL stage we can move this to
>>> the TPL stage, so it will run even earlier.
>>>
>>> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
>>> ---
>>>   board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c | 64 ++++++++++++++++++++++
>>>   1 file changed, 64 insertions(+)
>>>
>>> diff --git a/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c b/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
>>> index 3d0c614623..a93b11cd47 100644
>>> --- a/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
>>> +++ b/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
>>> @@ -6,12 +6,14 @@
>>>   #include <abuf.h>
>>>   #include <adc.h>
>>>   #include <asm/io.h>
>>> +#include <command.h>
>>>   #include <display.h>
>>>   #include <dm.h>
>>>   #include <dm/lists.h>
>>>   #include <env.h>
>>>   #include <fdt_support.h>
>>>   #include <linux/delay.h>
>>> +#include <linux/iopoll.h>
>>>   #include <mipi_dsi.h>
>>>   #include <mmc.h>
>>>   #include <panel.h>
>>> @@ -20,6 +22,8 @@
>>>   #include <stdlib.h>
>>>   #include <video_bridge.h>
>>> +#define BOOT_BROM_DOWNLOAD	0xef08a53c
>>> +
>>>   #define GPIO0_BASE		0xfdd60000
>>>   #define GPIO4_BASE		0xfe770000
>>>   #define GPIO_SWPORT_DR_L	0x0000
>>> @@ -33,6 +37,14 @@
>>>   #define GPIO_WRITEMASK(bits)	((bits) << 16)
>>> +#define SARADC_BASE		0xfe720000
>>> +#define SARADC_DATA		0x0000
>>> +#define SARADC_STAS		0x0004
>>> +#define SARADC_ADC_STATUS	BIT(0)
>>> +#define SARADC_CTRL		0x0008
>>> +#define SARADC_INPUT_SRC_MSK	0x7
>>> +#define SARADC_POWER_CTRL	BIT(3)
>>> +
>>>   #define DTB_DIR			"rockchip/"
>>>   struct rg3xx_model {
>>> @@ -118,12 +130,64 @@ static const struct rg353_panel rg353_panel_details[] = {
>>>   	},
>>>   };
>>> +/*
>>> + * The device has internal eMMC, and while some devices have an exposed
>>> + * clk pin you can ground to force a bypass not all devices do. As a
>>> + * result it may be possible for some devices to become a perma-brick
>>> + * if a corrupted TPL or SPL stage with a valid header is flashed to
>>> + * the internal eMMC. Add functionality to read ADC channel 0 (the func
>>> + * button) as early as possible in the boot process to provide some
>>> + * protection against this. If we ever get an open TPL stage, we should
>>> + * consider moving this function there.
>>> + */
>>> +void read_func_button(void)
>>> +{
>>> +	int ret;
>>> +	u32 reg;
>>> +
>>> +	/* Turn off SARADC to reset it. */
>>> +	writel(0, (SARADC_BASE + SARADC_CTRL));
>>> +
>>> +	/* Enable channel 0 and power on SARADC. */
>>> +	writel(((0 & SARADC_INPUT_SRC_MSK) | SARADC_POWER_CTRL),
>>> +	       (SARADC_BASE + SARADC_CTRL));
>>> +
>>> +	/*
>>> +	 * Wait for data to be ready. Use timeout of 20000us from
>>> +	 * rockchip_saradc driver.
>>> +	 */
>>> +	ret = readl_poll_timeout((SARADC_BASE + SARADC_STAS), reg,
>>> +				 !(reg & SARADC_ADC_STATUS), 20000);
>>> +	if (ret) {
>>> +		printf("ADC Timeout");
>>> +		return;
>>> +	}
>>> +
>>> +	/* Read the data from the SARADC. */
>>> +	reg = readl((SARADC_BASE + SARADC_DATA));
>>> +
>>> +	/* Turn the SARADC back off so it's ready to be used again. */
>>> +	writel(0, (SARADC_BASE + SARADC_CTRL));
>>> +
>>> +	/*
>>> +	 * If the value is less than 30 the button is being pressed.
>>> +	 * Reset the device back into Rockchip download mode.
>>> +	 */
>>> +	if (reg <= 30) {
>>> +		printf("download key pressed, entering download mode...");
>>> +		writel(BOOT_BROM_DOWNLOAD, CONFIG_ROCKCHIP_BOOT_MODE_REG);
>>> +		do_reset(NULL, 0, 0, NULL);
>>> +	}
>>> +};
>>> +
>>>   /*
>>>    * Start LED very early so user knows device is on. Set color
>>>    * to red.
>>>    */
>>>   void spl_board_init(void)
>>>   {
>>> +	read_func_button();
>>> +
>>>   	/* Set GPIO0_C5, GPIO0_C6, and GPIO0_C7 to output. */
>>>   	writel(GPIO_WRITEMASK(GPIO_C7 | GPIO_C6 | GPIO_C5) | \
>>>   	       (GPIO_C7 | GPIO_C6 | GPIO_C5),

Re: [PATCH 2/3] board: rockchip: Add Maskrom Mode for Anbernic RGxx3

[Chris Morgan]

On Mon, Oct 23, 2023 at 07:12:14PM +0200, Jonas Karlman wrote:
> Hi Chris,
> 
> On 2023-10-23 17:18, Chris Morgan wrote:
> > On Mon, Oct 23, 2023 at 06:10:07PM +0800, Kever Yang wrote:
> >> Hi Chris,
> >>
> >> On 2023/10/18 02:24, Chris Morgan wrote:
> >>> From: Chris Morgan <macromorgan@hotmail.com>
> >>>
> >>> Add support for users to enter maskrom mode by holding the function
> >>> button when they power up the device.
> >>>
> >>> Since the device has soldered eMMC and sometimes does not expose a clk
> >>> pin on the mainboard
> >>
> >> This board already available, so does this clk pin available?
> >>
> >> Not only clk, other cmd/data pin is also OK to make the soc get into maskrom
> >> mode.
> > 
> > No, as near as I can tell some boards (such as the 353P and 353V) don't
> > expose a clk, cmd, or data pin. The 353M does expose a clk pin though.
> > 
> >>
> >>> there is a small chance that a user who flashes a
> >>> bad bootloader may not be able to recover if the headers themselves
> >>> are valid. As a result this check is done during spl_early_init() to
> >>> ensure that it runs as early as possible, and it does so by directly
> >>> manipulating the ADC hardware in lieu of loading the ADC driver.
> >>
> >> This key seems like a "recovery" adc key instead of "maskrom" key, right?
> >>
> >> I think we don't need to add this patch if the hardware does not really have
> >> a "maskrom" key.
> >>
> >> Basically the "maskrom" key should not depends on any software.
> > 
> > Okay, so should I rename it as a "recovery" key? Basically it's just
> > a button hooked up to ADC channel 0. I'm trying to make as simple a
> > failsafe as possible, because for the boards without the clk/cmd pads
> > I want to make it easy to debrick in the event of something like a
> > bad U-Boot stage or a bad A-TF stage.
> 
> Isn't this very similar to what arch/arm/mach-rockchip/boot_mode.c and
> rockchip_dnl_key_pressed() already does, or is supposed to do?
> 
> Maybe this existing code can be changed/fixed to work earlier in SPL and
> possible also read recovery button information from DT or Kconfig to
> work with more rockchip boards?
> 
> Would be nice if a hang/panic in SPL make rk devices reset into maskrom
> usb mode. Maybe even the watchdog can be of some use here.
> 
> Btw, you could probably also enable CONFIG_SPL_FIT_SIGNATURE for added
> protection against starting corrupt/bitrot FIT images. To speed up boot
> after adding a sha256 checksum test, you can use following rfc/patch:
> 
> [RFC] rockchip: spl: Enable caches to speed up checksum validation
> https://patchwork.ozlabs.org/patch/1802303/
> 
> Regards,
> Jonas

Thank you, yes I will look into that. That said even if we detect a
corrupt SPL stage we're still stuck unless we add the additional
reset stuff like you mention.

Yes, this is more or less identical to the rockchip_dnl_key_pressed()
function with 2 distinct differences; one this doesn't require the ADC
driver since we're manipulating the controller directly (this makes it
easier to support in SPL or maybe even one day TPL), and two the
existing function has hard-coded checks for channel 1 whereas the board
here uses channel 0 for the function button. The comments for the
existing function suggest that it's meant to be "generic" and boards
are welcome to override with their own function, which is what I'm
doing though at an earlier stage in the boot process.

Thank you,
Chris

> 
> > 
> > Thank you.
> > 
> >>
> >>
> >> Thanks,
> >>
> >> - Kever
> >>
> >>>
> >>> Ideally, once we have an open source TPL stage we can move this to
> >>> the TPL stage, so it will run even earlier.
> >>>
> >>> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> >>> ---
> >>>   board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c | 64 ++++++++++++++++++++++
> >>>   1 file changed, 64 insertions(+)
> >>>
> >>> diff --git a/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c b/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
> >>> index 3d0c614623..a93b11cd47 100644
> >>> --- a/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
> >>> +++ b/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
> >>> @@ -6,12 +6,14 @@
> >>>   #include <abuf.h>
> >>>   #include <adc.h>
> >>>   #include <asm/io.h>
> >>> +#include <command.h>
> >>>   #include <display.h>
> >>>   #include <dm.h>
> >>>   #include <dm/lists.h>
> >>>   #include <env.h>
> >>>   #include <fdt_support.h>
> >>>   #include <linux/delay.h>
> >>> +#include <linux/iopoll.h>
> >>>   #include <mipi_dsi.h>
> >>>   #include <mmc.h>
> >>>   #include <panel.h>
> >>> @@ -20,6 +22,8 @@
> >>>   #include <stdlib.h>
> >>>   #include <video_bridge.h>
> >>> +#define BOOT_BROM_DOWNLOAD	0xef08a53c
> >>> +
> >>>   #define GPIO0_BASE		0xfdd60000
> >>>   #define GPIO4_BASE		0xfe770000
> >>>   #define GPIO_SWPORT_DR_L	0x0000
> >>> @@ -33,6 +37,14 @@
> >>>   #define GPIO_WRITEMASK(bits)	((bits) << 16)
> >>> +#define SARADC_BASE		0xfe720000
> >>> +#define SARADC_DATA		0x0000
> >>> +#define SARADC_STAS		0x0004
> >>> +#define SARADC_ADC_STATUS	BIT(0)
> >>> +#define SARADC_CTRL		0x0008
> >>> +#define SARADC_INPUT_SRC_MSK	0x7
> >>> +#define SARADC_POWER_CTRL	BIT(3)
> >>> +
> >>>   #define DTB_DIR			"rockchip/"
> >>>   struct rg3xx_model {
> >>> @@ -118,12 +130,64 @@ static const struct rg353_panel rg353_panel_details[] = {
> >>>   	},
> >>>   };
> >>> +/*
> >>> + * The device has internal eMMC, and while some devices have an exposed
> >>> + * clk pin you can ground to force a bypass not all devices do. As a
> >>> + * result it may be possible for some devices to become a perma-brick
> >>> + * if a corrupted TPL or SPL stage with a valid header is flashed to
> >>> + * the internal eMMC. Add functionality to read ADC channel 0 (the func
> >>> + * button) as early as possible in the boot process to provide some
> >>> + * protection against this. If we ever get an open TPL stage, we should
> >>> + * consider moving this function there.
> >>> + */
> >>> +void read_func_button(void)
> >>> +{
> >>> +	int ret;
> >>> +	u32 reg;
> >>> +
> >>> +	/* Turn off SARADC to reset it. */
> >>> +	writel(0, (SARADC_BASE + SARADC_CTRL));
> >>> +
> >>> +	/* Enable channel 0 and power on SARADC. */
> >>> +	writel(((0 & SARADC_INPUT_SRC_MSK) | SARADC_POWER_CTRL),
> >>> +	       (SARADC_BASE + SARADC_CTRL));
> >>> +
> >>> +	/*
> >>> +	 * Wait for data to be ready. Use timeout of 20000us from
> >>> +	 * rockchip_saradc driver.
> >>> +	 */
> >>> +	ret = readl_poll_timeout((SARADC_BASE + SARADC_STAS), reg,
> >>> +				 !(reg & SARADC_ADC_STATUS), 20000);
> >>> +	if (ret) {
> >>> +		printf("ADC Timeout");
> >>> +		return;
> >>> +	}
> >>> +
> >>> +	/* Read the data from the SARADC. */
> >>> +	reg = readl((SARADC_BASE + SARADC_DATA));
> >>> +
> >>> +	/* Turn the SARADC back off so it's ready to be used again. */
> >>> +	writel(0, (SARADC_BASE + SARADC_CTRL));
> >>> +
> >>> +	/*
> >>> +	 * If the value is less than 30 the button is being pressed.
> >>> +	 * Reset the device back into Rockchip download mode.
> >>> +	 */
> >>> +	if (reg <= 30) {
> >>> +		printf("download key pressed, entering download mode...");
> >>> +		writel(BOOT_BROM_DOWNLOAD, CONFIG_ROCKCHIP_BOOT_MODE_REG);
> >>> +		do_reset(NULL, 0, 0, NULL);
> >>> +	}
> >>> +};
> >>> +
> >>>   /*
> >>>    * Start LED very early so user knows device is on. Set color
> >>>    * to red.
> >>>    */
> >>>   void spl_board_init(void)
> >>>   {
> >>> +	read_func_button();
> >>> +
> >>>   	/* Set GPIO0_C5, GPIO0_C6, and GPIO0_C7 to output. */
> >>>   	writel(GPIO_WRITEMASK(GPIO_C7 | GPIO_C6 | GPIO_C5) | \
> >>>   	       (GPIO_C7 | GPIO_C6 | GPIO_C5),
> 

Re: [PATCH 2/3] board: rockchip: Add Maskrom Mode for Anbernic RGxx3

[Chris Morgan]

On Wed, Oct 25, 2023 at 02:15:57PM -0500, Chris Morgan wrote:
> On Mon, Oct 23, 2023 at 07:12:14PM +0200, Jonas Karlman wrote:
> > Hi Chris,
> > 
> > On 2023-10-23 17:18, Chris Morgan wrote:
> > > On Mon, Oct 23, 2023 at 06:10:07PM +0800, Kever Yang wrote:
> > >> Hi Chris,
> > >>
> > >> On 2023/10/18 02:24, Chris Morgan wrote:
> > >>> From: Chris Morgan <macromorgan@hotmail.com>
> > >>>
> > >>> Add support for users to enter maskrom mode by holding the function
> > >>> button when they power up the device.
> > >>>
> > >>> Since the device has soldered eMMC and sometimes does not expose a clk
> > >>> pin on the mainboard
> > >>
> > >> This board already available, so does this clk pin available?
> > >>
> > >> Not only clk, other cmd/data pin is also OK to make the soc get into maskrom
> > >> mode.
> > > 
> > > No, as near as I can tell some boards (such as the 353P and 353V) don't
> > > expose a clk, cmd, or data pin. The 353M does expose a clk pin though.
> > > 
> > >>
> > >>> there is a small chance that a user who flashes a
> > >>> bad bootloader may not be able to recover if the headers themselves
> > >>> are valid. As a result this check is done during spl_early_init() to
> > >>> ensure that it runs as early as possible, and it does so by directly
> > >>> manipulating the ADC hardware in lieu of loading the ADC driver.
> > >>
> > >> This key seems like a "recovery" adc key instead of "maskrom" key, right?
> > >>
> > >> I think we don't need to add this patch if the hardware does not really have
> > >> a "maskrom" key.
> > >>
> > >> Basically the "maskrom" key should not depends on any software.
> > > 
> > > Okay, so should I rename it as a "recovery" key? Basically it's just
> > > a button hooked up to ADC channel 0. I'm trying to make as simple a
> > > failsafe as possible, because for the boards without the clk/cmd pads
> > > I want to make it easy to debrick in the event of something like a
> > > bad U-Boot stage or a bad A-TF stage.
> > 
> > Isn't this very similar to what arch/arm/mach-rockchip/boot_mode.c and
> > rockchip_dnl_key_pressed() already does, or is supposed to do?
> > 
> > Maybe this existing code can be changed/fixed to work earlier in SPL and
> > possible also read recovery button information from DT or Kconfig to
> > work with more rockchip boards?
> > 
> > Would be nice if a hang/panic in SPL make rk devices reset into maskrom
> > usb mode. Maybe even the watchdog can be of some use here.
> > 
> > Btw, you could probably also enable CONFIG_SPL_FIT_SIGNATURE for added
> > protection against starting corrupt/bitrot FIT images. To speed up boot
> > after adding a sha256 checksum test, you can use following rfc/patch:
> > 
> > [RFC] rockchip: spl: Enable caches to speed up checksum validation
> > https://patchwork.ozlabs.org/patch/1802303/
> > 
> > Regards,
> > Jonas
> 
> Thank you, yes I will look into that. That said even if we detect a
> corrupt SPL stage we're still stuck unless we add the additional
> reset stuff like you mention.
> 
> Yes, this is more or less identical to the rockchip_dnl_key_pressed()
> function with 2 distinct differences; one this doesn't require the ADC
> driver since we're manipulating the controller directly (this makes it
> easier to support in SPL or maybe even one day TPL), and two the
> existing function has hard-coded checks for channel 1 whereas the board
> here uses channel 0 for the function button. The comments for the
> existing function suggest that it's meant to be "generic" and boards
> are welcome to override with their own function, which is what I'm
> doing though at an earlier stage in the boot process.
> 
> Thank you,
> Chris

Just wanted to dig this one back up. If I refer to this as "recovery"
instead of "maskrom", are we okay to go?

Thank you.

> 
> > 
> > > 
> > > Thank you.
> > > 
> > >>
> > >>
> > >> Thanks,
> > >>
> > >> - Kever
> > >>
> > >>>
> > >>> Ideally, once we have an open source TPL stage we can move this to
> > >>> the TPL stage, so it will run even earlier.
> > >>>
> > >>> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> > >>> ---
> > >>>   board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c | 64 ++++++++++++++++++++++
> > >>>   1 file changed, 64 insertions(+)
> > >>>
> > >>> diff --git a/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c b/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
> > >>> index 3d0c614623..a93b11cd47 100644
> > >>> --- a/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
> > >>> +++ b/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
> > >>> @@ -6,12 +6,14 @@
> > >>>   #include <abuf.h>
> > >>>   #include <adc.h>
> > >>>   #include <asm/io.h>
> > >>> +#include <command.h>
> > >>>   #include <display.h>
> > >>>   #include <dm.h>
> > >>>   #include <dm/lists.h>
> > >>>   #include <env.h>
> > >>>   #include <fdt_support.h>
> > >>>   #include <linux/delay.h>
> > >>> +#include <linux/iopoll.h>
> > >>>   #include <mipi_dsi.h>
> > >>>   #include <mmc.h>
> > >>>   #include <panel.h>
> > >>> @@ -20,6 +22,8 @@
> > >>>   #include <stdlib.h>
> > >>>   #include <video_bridge.h>
> > >>> +#define BOOT_BROM_DOWNLOAD	0xef08a53c
> > >>> +
> > >>>   #define GPIO0_BASE		0xfdd60000
> > >>>   #define GPIO4_BASE		0xfe770000
> > >>>   #define GPIO_SWPORT_DR_L	0x0000
> > >>> @@ -33,6 +37,14 @@
> > >>>   #define GPIO_WRITEMASK(bits)	((bits) << 16)
> > >>> +#define SARADC_BASE		0xfe720000
> > >>> +#define SARADC_DATA		0x0000
> > >>> +#define SARADC_STAS		0x0004
> > >>> +#define SARADC_ADC_STATUS	BIT(0)
> > >>> +#define SARADC_CTRL		0x0008
> > >>> +#define SARADC_INPUT_SRC_MSK	0x7
> > >>> +#define SARADC_POWER_CTRL	BIT(3)
> > >>> +
> > >>>   #define DTB_DIR			"rockchip/"
> > >>>   struct rg3xx_model {
> > >>> @@ -118,12 +130,64 @@ static const struct rg353_panel rg353_panel_details[] = {
> > >>>   	},
> > >>>   };
> > >>> +/*
> > >>> + * The device has internal eMMC, and while some devices have an exposed
> > >>> + * clk pin you can ground to force a bypass not all devices do. As a
> > >>> + * result it may be possible for some devices to become a perma-brick
> > >>> + * if a corrupted TPL or SPL stage with a valid header is flashed to
> > >>> + * the internal eMMC. Add functionality to read ADC channel 0 (the func
> > >>> + * button) as early as possible in the boot process to provide some
> > >>> + * protection against this. If we ever get an open TPL stage, we should
> > >>> + * consider moving this function there.
> > >>> + */
> > >>> +void read_func_button(void)
> > >>> +{
> > >>> +	int ret;
> > >>> +	u32 reg;
> > >>> +
> > >>> +	/* Turn off SARADC to reset it. */
> > >>> +	writel(0, (SARADC_BASE + SARADC_CTRL));
> > >>> +
> > >>> +	/* Enable channel 0 and power on SARADC. */
> > >>> +	writel(((0 & SARADC_INPUT_SRC_MSK) | SARADC_POWER_CTRL),
> > >>> +	       (SARADC_BASE + SARADC_CTRL));
> > >>> +
> > >>> +	/*
> > >>> +	 * Wait for data to be ready. Use timeout of 20000us from
> > >>> +	 * rockchip_saradc driver.
> > >>> +	 */
> > >>> +	ret = readl_poll_timeout((SARADC_BASE + SARADC_STAS), reg,
> > >>> +				 !(reg & SARADC_ADC_STATUS), 20000);
> > >>> +	if (ret) {
> > >>> +		printf("ADC Timeout");
> > >>> +		return;
> > >>> +	}
> > >>> +
> > >>> +	/* Read the data from the SARADC. */
> > >>> +	reg = readl((SARADC_BASE + SARADC_DATA));
> > >>> +
> > >>> +	/* Turn the SARADC back off so it's ready to be used again. */
> > >>> +	writel(0, (SARADC_BASE + SARADC_CTRL));
> > >>> +
> > >>> +	/*
> > >>> +	 * If the value is less than 30 the button is being pressed.
> > >>> +	 * Reset the device back into Rockchip download mode.
> > >>> +	 */
> > >>> +	if (reg <= 30) {
> > >>> +		printf("download key pressed, entering download mode...");
> > >>> +		writel(BOOT_BROM_DOWNLOAD, CONFIG_ROCKCHIP_BOOT_MODE_REG);
> > >>> +		do_reset(NULL, 0, 0, NULL);
> > >>> +	}
> > >>> +};
> > >>> +
> > >>>   /*
> > >>>    * Start LED very early so user knows device is on. Set color
> > >>>    * to red.
> > >>>    */
> > >>>   void spl_board_init(void)
> > >>>   {
> > >>> +	read_func_button();
> > >>> +
> > >>>   	/* Set GPIO0_C5, GPIO0_C6, and GPIO0_C7 to output. */
> > >>>   	writel(GPIO_WRITEMASK(GPIO_C7 | GPIO_C6 | GPIO_C5) | \
> > >>>   	       (GPIO_C7 | GPIO_C6 | GPIO_C5),
> >