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

[Kever Yang]

On 2024/1/25 06:55, Chris Morgan wrote:
> 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?

This is no reasonable, it should be able to reboot to maskrom mode any 
time we want, because

the vendor kernel also do this and the function works.

>
>> 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.
Yes, you are right about this, and I think rockchip vendor code is using 
SD card as  the first
priority in the SPL instead of "same-as SPL", and make the SPL small and 
simple enough
to involve less bug, so that stable enough and no need to update.
This patch is OK when the check for key press entry is in the board 
level instead of platform common code.

Thanks,
- Kever
>   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;
>>>>>     }