[U-Boot] [PATCH 3/9] arm: Move CP15 init out of cpu_init_crit()

[Simon Glass]

Hi Albert,

On Mon, Oct 24, 2011 at 1:04 PM, Albert ARIBAUD
<albert.u.boot at aribaud.net> wrote:
> Le 22/10/2011 18:13, Simon Glass a écrit :
>>
>> Hi Albert,
>>
>> On Sat, Oct 22, 2011 at 12:56 AM, Albert ARIBAUD
>> <albert.u.boot at aribaud.net>  wrote:
>>>
>>> Le 22/10/2011 07:05, Simon Glass a écrit :
>>>
>>>>>> Well I actually haven't moved it! It is just that previously it was
>>>>>> impossible to call cp15_init from anywhere later.
>>>>>
>>>>> It is moved, in that it belongs to low level init... of A9.
>>>>
>>>> OK, I see - you mean moved in order if not in source code file.
>>>
>>> Yes. In the code, though, it belongs to low-level init.
>>
>> Arguably this could go in a library like arch/arm/cpu/armv7/lib/cache.S
>>
>>>
>>>>>> What you say can be done, it would involve some assembler though and
>>>>>> would need to be another CONFIG option. Was trying to avoid adding new
>>>>>> assembler.
>>>>>
>>>>> Low level init is about assembler and, well, low level. :)
>>>>
>>>> Yes but it's yuck. Part of the clean-up is to remove most of the
>>>> assembler - really very little is needed.
>>>
>>> I don't think "yuck" is a valid reasoned argument against assembly
>>> language
>>> :) but I assume what you mainly mean is 'hard to understand and
>>> replaceable
>>> by easy to understand C code'. I agree to the first part, and to the
>>> second
>>> one for the general case, but not for the startup path where, precisely,
>>> we
>>> don't have a working C run-time and most of the understanding requires
>>> knowledge of hardware, not only general programming knowledge. Past that,
>>> C
>>
>> I'm not arguing for the cache init stuff to move to C, just trying to
>> avoid any creeping growth of assembler when C can do it.
>>
>>> is ok -- but then beware: some C++ guy could chime in and contend that C
>>> is
>>> "yuck" by your own standards. :)
>>
>> The C++ guy who finds himself in U-Boot is probably lost :-)
>>
>>>
>>>>> But I don't see why there should be another CONFIG option. IIUC, you
>>>>> should
>>>>> be able to do with only CONFIG_SKIP_LOWLEVEL_INIT: within the low level
>>>>> init
>>>>> code under it, you would do the equivalent of a switch, with one branch
>>>>> for
>>>>> AVM (and DDR init etc) and one branch for A9 (with cp15 init etc).
>>>>
>>>> Yes I can, but I need to be able to call cp15_init. And I can't do
>>>> that because if CONFIG_SKIP_LOWLEVEL_INIT is defined, that code is
>>>> compiled out! So I need to move that cp15_init code outside the
>>>> CONFIG_SKIP_LOWLEVEL_INIT #ifdef. That is all I am trying to do,
>>>> honest!
>>>
>>> I understand, and I think the approach here is wrong, because you *do*
>>> want
>>> low-level init in the A9 case, and thus you should not have
>>> CONFIG_SKIP_LOWLEVEL_INIT set. But you also need the AVP low-level init
>>> to
>>> be empty, and the AVP and A9 must follow the same startup path. And all
>>> this
>>> it not necessarily required for all armv7 platforms, but some of them
>>> will
>>> want lowlevel init, and some not. Considering all this...
>>>
>>> For now, I would opt for a CONFIG_ARCH_GENERIC_LOWLEVEL_INIT option which
>>> start.S would use to compile out the low level init code definition (like
>>> CONFIG_SKIP_LOWLEVEL_INIT does) but not the the calls to it (UNline
>>> CONFIG_SKIP_LOWLEVEL_INIT does). Then you could provide a SoC-specific
>>> version of lowlevel_init.
>>
>> If I understand you correctly, this is the opposite of what I want. It
>> is basically what we have now.
>>
>> All ARMv7 chips are going to turn off MMU, caches, flush TLBs, and the
>> like - this is an architecture feature, not an SOC one. We have
>> conflated ARM arch init with SOC init as you say, but the solution is
>> not to force the SOC to copy code from start.S just so that it can do
>> ARM arch init later.
>
> Yes, all ARMv7 CPUs will turn off caches etc, and thus, they all will want
> to do the cp15 init where it is now. Only the Tegra wants has issues with it
> so far.

Yes

>
>> And I'd argue for LOWLEVEL_INIT being about setting up the memory so
>> we can run code, and in particular relocate it later. The cp15_init
>> stuff should arguably be in arch_cpu_init() (or perhaps a new
>> arch_init()) as the first thing called from board_init_f() in my view.
>
> I disagree that cp15 could be set later: if we don't set it at low level
> init, we cannot be sure whether data caches are off or on, and if on, what
> they contain already.

Well on Tegra the AVP cannot execute this code. There is no danger in
this case since it doesn't have caches, etc. We execute the code as
soon as practical on the A9.

>
>> So how about I create a patch to move the cp15_init() code into
>> arch/arm/cpu/armv7/lib/lowlevel.S or similar so I can call it later?
>
> I don't mind moving it to armv7/lib. I do mind the "later".

OK, but the AVP cannot execute this code, whereas the A9 needs to
execute it on start-up.

We define CONFIG_SKIP_LOWLEVEL_INIT on Tegra, and the mere fact that
this option exists suggests that platforms are permitted to do it. We
don't need to do any memory init, and the CP15 init can be called in
arch_cpu_init() once we establish that we are an A9 and not an AVP.
Does that sound ok?

The only problem I can see with this is if the data cache is on. This
could happen if you load U-Boot with (say) tftp and jump to it. Well
'don't do that!'. We also hope that the D-cache has at least been
flushed or there is nothing we can do. One fix for this (prior to the
refactor you are planning and mentioned in your earlier email) is
perhaps to implement a run-time check for the existence of CP15. Is
that better or worse?

Regards,
Simon

>
>>> In the longer term, I would be more in favor of a weak definition system
>>> with hierarchical ARCH, ISA, SoC, board priority order (yes, I am kind of
>>> fond of it) for lowlevel init -- but that will require looking into asm
>>> weak
>>> symbol handling and may require splitting of the assembly code function
>>> by
>>> function.
>>
>> I think you are trying to avoid the
>>
>> #ifdef CONFIG_ARCH_CPU_INIT
>>    arch_cpu_init,
>> #endif
>> #ifdef ...
>>    some_other_thing
>> #endif
>>
>> in the board_init_f() function table. Yes I agree it would tidy things
>> up, although I wonder if Graeme's initcall thing isn't better again.
>> It has the same confusion level coefficient (only the linker decides
>> what code is included) with a bit more flexibility. Then each arch /
>> board / cpu / soc / squirrel can decide on its own init and put it in
>> its own file. C only please :-)
>
> AFAIU, initcall does not have an override mechanism by which a later module
> can replace an initcall symbol already defined; two modules defining the
> same function name as initcall will just end up causing a linker error.
>
>> Regards,
>> Simon
>>
>>>
>>>> Regards,
>>>> Simon
>>>
>>> Amicalement,
>>> --
>>> Albert.
>
> Amicalement,
> --
> Albert.
>