[U-Boot] [PATCH 1/9] MIPS: Add arch-wide arch_cpu_init

[Marek Vasut]

On 09/08/2016 01:21 PM, Paul Burton wrote:
> 
> 
> On 08/09/16 11:47, Marek Vasut wrote:
>> On 09/08/2016 12:36 PM, Paul Burton wrote:
>>> On 08/09/16 11:02, Marek Vasut wrote:
>>>> On 09/07/2016 07:48 PM, Paul Burton wrote:
>>>>> Add an implementation of arch_cpu_init that covers all MIPS boards, in
>>>>> preparation for performing various pieces of initialisation there in
>>>>> later patches.
>>>>>
>>>>> In order to allow the ath79 code to continue performing initialisation
>>>>> at this point in the boot, it's converted to use a new mach_cpu_init
>>>>> function which is called from arch_cpu_init.
>>>>>
>>>>> Signed-off-by: Paul Burton <paul.burton at imgtec.com>
>>>>
>>>> [...]
>>>>
>>>> Hi!
>>>>
>>>>> diff --git a/arch/mips/include/asm/u-boot-mips.h b/arch/mips/include/asm/u-boot-mips.h
>>>>> index 1f527bb..0eaf32b 100644
>>>>> --- a/arch/mips/include/asm/u-boot-mips.h
>>>>> +++ b/arch/mips/include/asm/u-boot-mips.h
>>>>> @@ -5,4 +5,16 @@
>>>>>  #ifndef _U_BOOT_MIPS_H_
>>>>>  #define _U_BOOT_MIPS_H_
>>>>>  
>>>>> +/**
>>>>> + * mach_cpu_init() - Perform SoC/machine-specific CPU initialisation
>>>>> + *
>>>>> + * This is called from arch_cpu_init() to allow for SoC/machine-level code to
>>>>> + * perform any CPU initialisation it may require.
>>>>
>>>> Just call this function from arch_cpu_init() in various places instead
>>>> of replacing arch_cpu_init() with it all over the place. Also rename it
>>>> to arch_cpu_init_common() to make it more obvious what this does .
>>>
>>> Hi Marek,
>>
>> Hi,
>>
>>> That's "backwards" compared with what this code actually does -
>>> arch_cpu_init becomes the common function (ie. is arch-wide as in the
>>> patch subject) and mach_cpu_init is for use by machines/SoCs - ie. code
>>> under arch/mips/mach-*.
>>
>> For machines, we already have board_cpu_init() , for SoCs we have
>> arch_cpu_init() . Reading through the patchset, I understand the
>> purpose, but then the content of mach_cpu_init() looks like some
>> common arch init bit to me.
> 
> Hi Marek,
> 
> The content of the only implementation of mach_cpu_init is some
> ath79-specific chip ID stuff. I'm not sure how you think that's "common
> arch init"?
> 
> Right now without this patch you're right - arch_cpu_init is used by
> SoCs (well, one SoC - ath79). This patch changes that so that
> arch_cpu_init is used by the arch code & mach_cpu_init is used by the
> SoC/machine code under arch/mips/mach-*. That seems like the most
> logical way to handle it to me.

It also introduces discrepancy with ARM and other architectures though.

> I get that arch_cpu_init is also used by SoCs on some other
> architectures (arm & x86 seemingly), but not on all - there's precedent
> for an arch-wide implementation in arc, avr32, blackfin, nios2 or xtensa.

Most of which are dead/unmaintained though.

Seems like this might need some further cleanup/clarification then.

>>
>>>>> + */
>>>>> +#ifdef CONFIG_MACH_CPU_INIT
>>>>> +void mach_cpu_init(void);
>>>>> +#else
>>>>> +static inline void mach_cpu_init(void) {}
>>>>
>>>> Implement this as __weak int and you can nuke the ifdefery .
>>>
>>> I could make it weak, but then we don't let the compiler optimise it out
>>> entirely for builds that don't need it (ie. everything except ath79).
>>
>> Did you try it ?
> 
> I didn't, but the compiler will have to emit the call as it won't be
> known whether there's an implementation of the function until link time
> so it will obviously have a small cost (unless LTO is used). Having said
> that I see the U-Boot wiki seems to indicate that weak is preferred so
> I'm ok with changing it.

btw if you want to go ahead with adding the mach_cpu_init(), you might
want to consider adding it into the board_f.c initcalls , so other
arches can convert to it.

-- 
Best regards,
Marek Vasut