[U-Boot] [PATCH 1/3] ARmv7: Add a soc_init hook to start.S

[Bill Pringlemeir]

>> On 21 Jan 2015, hdegoede at redhat.com wrote:
>>
>>> On some SoCs / ARMv7 CPU cores we need to do some setup before
>>> enabling the icache, etc. Add a soc_init hook with a weak default
>>> which just calls cpu_init_cp15.
>>>
>>> This way different implementations can be provided to do some extra
>>> work before or after cpu_init_cp15, or completely replacing
>>> cpu_init_cp15.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>>> ---
>>> arch/arm/cpu/armv7/start.S | 18 +++++++++++++++++-
>>> 1 file changed, 17 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
>>> index fdc05b9..9882b20 100644
>>> --- a/arch/arm/cpu/armv7/start.S
>>> +++ b/arch/arm/cpu/armv7/start.S
>>>>> -64,7 +64,7 @@ reset:
>>>
>>> 	/* the mask ROM code should have PLL and others stable */
>>> #ifndef CONFIG_SKIP_LOWLEVEL_INIT
>>> -	bl	cpu_init_cp15
>>> +	bl	soc_init
>>> 	bl	cpu_init_crit
>>> #endif
>>>
>>>>> -102,6 +102,22 @@ ENDPROC(save_boot_params)
>>> /*************************************************************************
>>> * + * void soc_init(void) + * __attribute__((weak)); + * + * Stack
>>> pointer is not yet initialized at this moment + * Don't save
>>> anything to stack even if compiled with -O0 + * +
>>> *************************************************************************/
>>> +ENTRY(soc_init)
>>> + mov r9, lr 
>>> + bl cpu_init_cp15 
>>> + mov pc, r9 @ back to my caller 
>>> +ENDPROC(soc_init)
>>> + .weak soc_init

> On 21-01-15 22:59, Bill Pringlemeir wrote:
>>
>> You could just use a 'tail call' and make this,
>>
>> +ENTRY(soc_init)
>> +	b	cpu_init_cp15
>> +ENDPROC(soc_init)

On 22 Jan 2015, hdegoede at redhat.com wrote:

> True, although I think that actually saving lr to r9 is useful as an
> example for boards who want to override this, and that we can spare
> the 2 extra instructions :)

Yes, it is a good example if you are looking to over-ride the 'soc_init'
and I expect that it was hard to figure out that you needed to save the
lr; I can see why you want to warn others.  Two instructions isn't bad.
However, I looked a the code from a different perspective.  I just want
to see what is going on in a normal boot.  I would look at this code and
think what the heck is this about.  Especially as 'r9' is used for other
purposes.

>> Or even as the code follows just add a duplicate label, so 'soc_init'
>> is a weak version of 'cpu_init_cp15'?  This gives no additional code
>> in a final binary.  I guess it depends on how the generic 'soc_init'
>> might be modified in the future?

> I think that having a separate entry for it is much more clear,
> otherwise the entire symbol will be hard to find / grok for people
> trying to get into the code.

Hmm.  I understood better after looking at the sunxi implementation.
The weak 'soc_init' seemed like I didn't understand something.  The
saving of 'lr' in 'r9' is only needed if you have extra code and is
nothing to do with global data or something.  So, it is not just less
code but I think it is more clear what the default case is doing.

I agree, it is definitely an opinion.  Either way works.

> Albert what do you want to do here?