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.
