Re: [PATCH v1 1/3] mach-sunxi: Add boot device detection for SUNIV

[Jesse Taube <mr.bossman075@gmail.com>]

On 2/10/22 05:57, Andre Przywara wrote:
> On Wed,  9 Feb 2022 23:34:36 -0500
> Jesse Taube <mr.bossman075@gmail.com> wrote:
> 
> Hi Jesse,
> 
> many thanks for sending this, much appreciated!
> 
>> Use Samuel's suggestion of looking at the BootRom's stack
>> to determine the boot device.
> 
> Can you please elaborate here what's going on, for future reference? Like:
> =============
> In contrast to other Allwinner SoCs the F1C100s BROM does not store a boot
> source indicator in the eGON header in SRAM. This leaves us guessing where
> we were exactly booted from, and for instance trying the SD card first,
> even though we booted from SPI flash.
> By inspecting the BROM code and by experimentation, Samuel found that the
> top of the BROM stack contains unique pointers for each of the boot
> sources, which we can use as a boot source indicator.
> 
> Remove the existing board_boot_order kludge and replace it with a proper
> boot source indication function.
> =============
> 
> 
>>
>> Signed-off-by: Jesse Taube <Mr.Bossman075@gmail.com>
>> Suggested-by: Samuel Holland <samuel@sholland.org>
>> ---
>>   arch/arm/include/asm/arch-sunxi/spl.h | 15 ++++++++
>>   arch/arm/mach-sunxi/board.c           | 50 ++++++++++++---------------
>>   2 files changed, 38 insertions(+), 27 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/arch-sunxi/spl.h b/arch/arm/include/asm/arch-sunxi/spl.h
>> index 58cdf806d9..d069091297 100644
>> --- a/arch/arm/include/asm/arch-sunxi/spl.h
>> +++ b/arch/arm/include/asm/arch-sunxi/spl.h
>> @@ -19,8 +19,23 @@
>>   #define SUNXI_BOOTED_FROM_MMC0_HIGH	0x10
>>   #define SUNXI_BOOTED_FROM_MMC2_HIGH	0x12
>>   
>> +/*
>> + * Values taken from the Bootrom's stack used
>> + * to determine where we booted from.
>> + * 0xffff40f8: mmc0
>> + * 0xffff4114: spi0 NAND
>> + * 0xffff4130: spi0 NOR
>> + * 0xffff4150: mmc1
> 
> Those last four lines are redundant, as you say exactly that, in code,
> down here again. Comments are good, speaking code is better.
> 
>> + */
>> +
>> +#define SUNIV_BOOTED_FROM_MMC0	0xffff40f8
>> +#define SUNIV_BOOTED_FROM_NAND	0xffff4114
>> +#define SUNIV_BOOTED_FROM_SPI	0xffff4130
>> +#define SUNIV_BOOTED_FROM_MMC1	0xffff4150
>> +
>>   #define is_boot0_magic(addr)	(memcmp((void *)(addr), BOOT0_MAGIC, 8) == 0)
>>   
>>   uint32_t sunxi_get_boot_device(void);
>> +uint32_t suniv_get_boot_device(void);
>>   
>>   #endif
>> diff --git a/arch/arm/mach-sunxi/board.c b/arch/arm/mach-sunxi/board.c
>> index 57078f7a7b..b0658d583e 100644
>> --- a/arch/arm/mach-sunxi/board.c
>> +++ b/arch/arm/mach-sunxi/board.c
>> @@ -241,6 +241,25 @@ uint32_t sunxi_get_boot_device(void)
>>   	return -1;		/* Never reached */
>>   }
>>   
>> +uint32_t suniv_get_boot_device(void)
> 
> This can be static, right?
> 
>> +{
>> +	/* Get the last function call from BootRom's stack. */
>> +	u32 brom_call = *(u32 *)(fel_stash.sp - 4);You are okay with this I was expecting you to explain a better way that 
i don't know about.
>> +
>> +	switch (brom_call) {
>> +	case SUNIV_BOOTED_FROM_MMC0:
>> +		return BOOT_DEVICE_MMC1;
>> +	case SUNIV_BOOTED_FROM_NAND:
>> +	case SUNIV_BOOTED_FROM_SPI:
>> +		return BOOT_DEVICE_SPI;
>> +	case SUNIV_BOOTED_FROM_MMC1:
>> +		return BOOT_DEVICE_MMC2;
>> +	}
> 
> Don't you need to handle FEL boot here somehow?
Yes but I have no clue what the SP is also wouldn't we have it hang 
anyway. The other changes requested i have fixed, I'm sorry about the 
subpar commit descriptions.

Thanks,
	Jesse
> Cheers,
> Andre
> 
>> +
>> +	printf("Unknown boot source from BROM: 0x%x\n", brom_call);
>> +	return BOOT_DEVICE_MMC1;
>> +}
>> +
>>   #ifdef CONFIG_SPL_BUILD
>>   static u32 sunxi_get_spl_size(void)
>>   {
>> @@ -276,36 +295,13 @@ unsigned long spl_mmc_get_uboot_raw_sector(struct mmc *mmc,
>>   	return sector;
>>   }
>>   
>> -#ifdef CONFIG_MACH_SUNIV
>> -/*
>> - * The suniv BROM does not pass the boot media type to SPL, so we try with the
>> - * boot sequence in BROM: mmc0->spinor->fail.
>> - * TODO: This has the slight chance of being wrong (invalid SPL signature,
>> - * but valid U-Boot legacy image on the SD card), but this should be rare.
>> - * It looks like we can deduce from some BROM state upon entering the SPL
>> - * (registers, SP, or stack itself) where the BROM was coming from and use
>> - * that here.
>> - */
>> -void board_boot_order(u32 *spl_boot_list)
>> -{
>> -	/*
>> -	 * See the comments above in sunxi_get_boot_device() for information
>> -	 * about FEL boot.
>> -	 */
>> -	if (!is_boot0_magic(SPL_ADDR + 4)) {
>> -		spl_boot_list[0] = BOOT_DEVICE_BOARD;
>> -		return;
>> -	}
>> -
>> -	spl_boot_list[0] = BOOT_DEVICE_MMC1;
>> -	spl_boot_list[1] = BOOT_DEVICE_SPI;
>> -}
>> -#else
>>   u32 spl_boot_device(void)
>>   {
>> -	return sunxi_get_boot_device();
>> +	if (IS_ENABLED(CONFIG_MACH_SUNIV))
>> +		return suniv_get_boot_device();
>> +	else
>> +		return sunxi_get_boot_device();
>>   }
>> -#endif
>>   
>>   __weak void sunxi_sram_init(void)
>>   {
>