diff mbox series

[4/4] board: rockchip: Add early ADC button detect for RGxx3

Message ID 20240205185855.21508-5-macroalpha82@gmail.com
State Accepted
Commit 41a60d0e5cef54a59596a58940fa7c9cf071034b
Delegated to: Kever Yang
Headers show
Series Add New Devices for RGxx3 | expand

Commit Message

Chris Morgan Feb. 5, 2024, 6:58 p.m. UTC 
From: Chris Morgan <macromorgan@hotmail.com>

Add ADC button detect for early SPL stage for RGxx3 device. This is
important because on at least the RG353P and RG353V a clk pin is not
exposed that would allow us to take the eMMC out of the boot path.

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

Comments

Kever Yang Feb. 6, 2024, 2:53 a.m. UTC | #1 
On 2024/2/6 02:58, Chris Morgan wrote:
> From: Chris Morgan <macromorgan@hotmail.com>
>
> Add ADC button detect for early SPL stage for RGxx3 device. This is
> important because on at least the RG353P and RG353V a clk pin is not
> exposed that would allow us to take the eMMC out of the boot path.

This is another version of below patch, right?

https://patchwork.ozlabs.org/project/uboot/patch/20240102154654.191055-5-macroalpha82@gmail.com/


Compare to this one, I would prefer previous version :(


Thanks,

- Kever

>
> 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 5c57b902d1..099eea60c3 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>
> @@ -19,6 +21,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
> @@ -32,6 +36,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 {
> @@ -157,12 +169,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),
Chris Morgan Feb. 6, 2024, 3:29 a.m. UTC | #2 
On Tue, Feb 06, 2024 at 10:53:13AM +0800, Kever Yang wrote:
> 
> On 2024/2/6 02:58, Chris Morgan wrote:
> > From: Chris Morgan <macromorgan@hotmail.com>
> > 
> > Add ADC button detect for early SPL stage for RGxx3 device. This is
> > important because on at least the RG353P and RG353V a clk pin is not
> > exposed that would allow us to take the eMMC out of the boot path.
> 
> This is another version of below patch, right?
> 
> https://patchwork.ozlabs.org/project/uboot/patch/20240102154654.191055-5-macroalpha82@gmail.com/
> 
> 
> Compare to this one, I would prefer previous version :(

It is another version of the patch. I did (redid) this in response to
comments on that patch series.

I basically face a unique set of constraints, and am trying to solve
for it. Those constraints are 1) I have a few boards, namely the
Anbernic RG353P and RG353V that do not have exposed pads that would
allow me to remove the eMMC from the boot path, 2) The SDMMC slot
present is after the eMMC in the boot path, and 3) after the binary
A-TF loads the reset to usb loader fails to work properly and simply
resets the board (but not to maskrom as needed/requested).

Your past comments suggested it would not be needed/useful to have this
done in a board generic way, which I understand; so I'm simply doing
it again in a board specific way. Note that I will shortly be
resubmitting another U-Boot request for the Powkiddy X55 which also
needs this; I'm not aware of any other boards currently that will
though.

So I guess my questions that would answer the path forward are
1) should this be done in a board specific way (like so) or should I
go back to the board generic method. 2) Will the binary A-TF be fixed
to allow me to reboot to maskrom mode or is that by design?

Note I've tested this also with the pending A-TF source at
trustedfirmware.org. Sadly I need SCMI clk support before I can replace
the binary A-TF with the open source one.

Thank you,
Chris

> 
> 
> Thanks,
> 
> - Kever
> 
> > 
> > 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 5c57b902d1..099eea60c3 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>
> > @@ -19,6 +21,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
> > @@ -32,6 +36,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 {
> > @@ -157,12 +169,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),
Kever Yang March 14, 2024, 6:52 a.m. UTC | #3 
On 2024/2/6 02:58, Chris Morgan wrote:
> From: Chris Morgan <macromorgan@hotmail.com>
>
> Add ADC button detect for early SPL stage for RGxx3 device. This is
> important because on at least the RG353P and RG353V a clk pin is not
> exposed that would allow us to take the eMMC out of the boot path.
>
> 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 | 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 5c57b902d1..099eea60c3 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>
> @@ -19,6 +21,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
> @@ -32,6 +36,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 {
> @@ -157,12 +169,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),
diff mbox series

Patch

diff --git a/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c b/board/anbernic/rgxx3_rk3566/rgxx3-rk3566.c
index 5c57b902d1..099eea60c3 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>
@@ -19,6 +21,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
@@ -32,6 +36,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 {
@@ -157,12 +169,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),