Re: [PATCH] rockchip: spl: Cache boot source id for later use

[Dragan Simic <dsimic@manjaro.org>]

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.