On 2024-03-19 16:59, Jonas Karlman wrote:
On 2024-03-19 10:44, Dragan Simic wrote:
On 2024-03-15 18:34, Jonas Karlman wrote:
diff --git a/arch/arm/mach-rockchip/spl.c b/arch/arm/mach-rockchip/spl.c index 1586a093fc37..27e996b504e7 100644 --- a/arch/arm/mach-rockchip/spl.c +++ b/arch/arm/mach-rockchip/spl.c @@ -32,9 +32,17 @@ __weak const char * const boot_devices[BROM_LAST_BOOTSOURCE + 1] = {
const char *board_spl_was_booted_from(void) {
- u32 bootdevice_brom_id = readl(BROM_BOOTSOURCE_ID_ADDR);
static u32 bootdevice_brom_id; const char *bootdevice_ofpath = NULL;
if (!bootdevice_brom_id)
bootdevice_brom_id = readl(BROM_BOOTSOURCE_ID_ADDR);if (!bootdevice_brom_id) {
debug("%s: unknown brom_bootdevice_id %x\n",__func__, bootdevice_brom_id);return NULL;}
Maybe it would be better to execute readl(BROM_BOOTSOURCE_ID_ADDR) only once, i.e. to have something like this instead:
+ static u32 bootdevice_brom_id = -1; + if (bootdevice_brom_id == -1) { + bootdevice_brom_id = readl(BROM_BOOTSOURCE_ID_ADDR); + if (!bootdevice_brom_id) + debug("%s: unknown brom_bootdevice_id %x\n", + __func__, bootdevice_brom_id); + } + + if (!bootdevice_brom_id) /* fail on subsequent tries */ + return NULL; +The logic behind such an approach would be to try only once and fail on subsequent (re)tries. That way, it would also serve as some kind of a runtime canary test, because the first try should succeed, which may prove useful in the field.
If we initialize the static variable to a value it will be stored in the .data section and takes up binary size. With a static variable like in this patch this is instead stored in .bss section and gets initialized to 0 by runtime. 0 is also not a valid boot source id value and the reset value for the reg.
Is it mandatory that this static variable ends up in the .bss section?
I do not see any point in making the code more complex than it has to be. The reg will contain a known boot source id, 1 - 10, set by BROM or something has gone wrong and the value is not usable any way.
As I already described, making the code more complex would intentionally introduce a failure point, which would be triggered in case obtaining valid value from readl() fails the first time. Something like that is usually known as a canary, which I'm sure you already know about.