[PATCH V4 3/7] rockchip: boot_mode: Allow rockchip_dnl_key_pressed() in SPL

[Chris Morgan]

On Fri, Jan 19, 2024 at 05:01:38PM +0800, Kever Yang wrote:
> Hi Chris,
> 
> On 2024/1/18 23:06, Chris Morgan wrote:
> > On Thu, Jan 18, 2024 at 03:20:52PM +0800, Kever Yang wrote:
> > > Hi Chris,
> > > 
> > > On 2024/1/2 23:46, Chris Morgan wrote:
> > > > From: Chris Morgan<macromorgan at hotmail.com>
> > > > 
> > > > Update the rockchip_dnl_key_pressed() so that it can run in
> > > > SPL. Also change the ADC channel to a define that can be
> > > > overridden by a board specific option.
> > > I should ask this earlier.
> > > 
> > > Why you have to enable the download key in SPL?
> > > 
> > > This works for a long time in U-Boot proper, both mainline and downstream
> > > vendor U-Boot,
> > > 
> > > no one is detect this download key in SPL.
> > > 
> > > In theory the SPL can implement all the feature which is available in U-Boot
> > > proper,
> > > 
> > > but it would be better to make the SPL focus on the job it should be done
> > > and keep as small as possible.
> > > 
> > I wanted this code to run as soon as possible in the event of a bad
> > write. I have some devices which lack a way to bypass the eMMC in the
> > boot path; the fear is that if an SPL stage is written with a good
> > header but a bad payload (or if the A-TF does something wrong) the
> > device will become unbootable. If we can't do it this way for
> > all Rockchip boards that's fine, but I REALLY want to at least do
> > it this way for the Powkiddy X55 board and the Anbernic RGxx3 board.
> 
> That's why rockchip platform has two download mode, loader mode(in U-Boot)
> 
> and maskRom mode, any error happen before U-Boot should get into maksRom
> mode.
> 
> Most of user/developer only use loader mode because the modules before
> U-Boot should
> 
> be more stable and have very little chance to update, for the module owners
> before U-Boot,
> 
> they are the users of maskRom mode.
> 
> If you enable this feature in SPL, the error still have chance to happen
> during dram init to SPL key detect.
> 
> Does these two board has no way to get into eMMC event with hardware rework?
> any hardware
> 
> signal of eMMC CMD/CLK/DATA is OK.

A few do not. The error is less likely to occur between RAM init and
SPL key detect, but yes it's even possible at this stage.

>From my testing using the latest BL31 file from the
rockchip-linux/rkbin github repository I was unable to reboot into
maskrom mode after A-TF loaded, the device would just straight
reboot. Do you know if A-TF does something that makes reboot
into maskrom mode no longer work?

> 
> And another way to bypass eMMC is to use SDcard which has higher priority
> then eMMC in BootRom.
> 

I see in the TRM under section 1.2 that the eMMC is higher in the boot
path than SDMMC, and that matches my experience as well. So this
wouldn't be a solution sadly.

> For SPL itself debug, I would suggest to RESET into bootrom download mode if
> any error happen
> 
> and a reset will need.
> 
> I pick up patches other than 2,3,4 in this patchset for now.
> 
> > While I have you, I have a related question I hope you can help with.
> > Is there any way to utilize the boot partitions of an eMMC device? I
> > don't know if the BROM of an RK3568 or RK3588 is capable, but I'd love
> > to have the SPL and U-Boot stages written to the eMMC boot partitions
> > instead of the user partition if I can.
> 
> No, Rockchip SoCs does not use the boot partitions of eMMC, only use the
> user data partition.

That's unfortunate, but I appreciate the feedback.

Thank you,
Chris

> 
> 
> Thanks,
> 
> - Kever
> 
> > 
> > Thank you.
> > 
> > > Thanks,
> > > 
> > > - Kever
> > > 
> > > > Signed-off-by: Chris Morgan<macromorgan at hotmail.com>
> > > > ---
> > > >    arch/arm/mach-rockchip/Makefile    |  4 ++--
> > > >    arch/arm/mach-rockchip/boot_mode.c | 11 ++++++++++-
> > > >    2 files changed, 12 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/arch/arm/mach-rockchip/Makefile b/arch/arm/mach-rockchip/Makefile
> > > > index 1dc92066bb..ff089ae949 100644
> > > > --- a/arch/arm/mach-rockchip/Makefile
> > > > +++ b/arch/arm/mach-rockchip/Makefile
> > > > @@ -15,13 +15,13 @@ obj-tpl-$(CONFIG_ROCKCHIP_PX30) += px30-board-tpl.o
> > > >    obj-spl-$(CONFIG_ROCKCHIP_RK3036) += rk3036-board-spl.o
> > > > -ifeq ($(CONFIG_SPL_BUILD)$(CONFIG_TPL_BUILD),)
> > > > -
> > > >    # Always include boot_mode.o, as we bypass it (i.e. turn it off)
> > > >    # inside of boot_mode.c when CONFIG_ROCKCHIP_BOOT_MODE_REG is 0.  This way,
> > > >    # we can have the preprocessor correctly recognise both 0x0 and 0
> > > >    # meaning "turn it off".
> > > >    obj-y += boot_mode.o
> > > > +
> > > > +ifeq ($(CONFIG_SPL_BUILD)$(CONFIG_TPL_BUILD),)
> > > >    obj-$(CONFIG_ROCKCHIP_COMMON_BOARD) += board.o
> > > >    obj-$(CONFIG_MISC_INIT_R) += misc.o
> > > >    endif
> > > > diff --git a/arch/arm/mach-rockchip/boot_mode.c b/arch/arm/mach-rockchip/boot_mode.c
> > > > index eb8f65ae4e..d2308768be 100644
> > > > --- a/arch/arm/mach-rockchip/boot_mode.c
> > > > +++ b/arch/arm/mach-rockchip/boot_mode.c
> > > > @@ -38,6 +38,10 @@ void set_back_to_bootrom_dnl_flag(void)
> > > >    #define KEY_DOWN_MIN_VAL	0
> > > >    #define KEY_DOWN_MAX_VAL	30
> > > > +#ifndef RK_DNL_ADC_CHAN
> > > > +#define RK_DNL_ADC_CHAN		1
> > > > +#endif
> > > > +
> > > >    __weak int rockchip_dnl_key_pressed(void)
> > > >    {
> > > >    	unsigned int val;
> > > > @@ -52,7 +56,8 @@ __weak int rockchip_dnl_key_pressed(void)
> > > >    	ret = -ENODEV;
> > > >    	uclass_foreach_dev(dev, uc) {
> > > >    		if (!strncmp(dev->name, "saradc", 6)) {
> > > > -			ret = adc_channel_single_shot(dev->name, 1, &val);
> > > > +			ret = adc_channel_single_shot(dev->name,
> > > > +						      RK_DNL_ADC_CHAN, &val);
> > > >    			break;
> > > >    		}
> > > >    	}
> > > > @@ -73,11 +78,13 @@ __weak int rockchip_dnl_key_pressed(void)
> > > >    void rockchip_dnl_mode_check(void)
> > > >    {
> > > > +#if CONFIG_IS_ENABLED(ADC)
> > > >    	if (rockchip_dnl_key_pressed()) {
> > > >    		printf("download key pressed, entering download mode...");
> > > >    		set_back_to_bootrom_dnl_flag();
> > > >    		do_reset(NULL, 0, 0, NULL);
> > > >    	}
> > > > +#endif
> > > >    }
> > > >    int setup_boot_mode(void)
> > > > @@ -90,6 +97,7 @@ int setup_boot_mode(void)
> > > >    	boot_mode = readl(reg);
> > > >    	debug("%s: boot mode 0x%08x\n", __func__, boot_mode);
> > > > +#if !defined(CONFIG_SPL_BUILD) && !defined(CONFIG_TPL_BUILD)
> > > >    	/* Clear boot mode */
> > > >    	writel(BOOT_NORMAL, reg);
> > > > @@ -103,6 +111,7 @@ int setup_boot_mode(void)
> > > >    		env_set("preboot", "setenv preboot; ums mmc 0");
> > > >    		break;
> > > >    	}
> > > > +#endif
> > > >    	return 0;
> > > >    }