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

[Paul Burton]

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.

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.

> 
>>>> + */
>>>> +#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.

Thanks,
    Paul

> 
>> I'd say that since the #ifdefery here is confined to this one case in
>> the header (which is a pretty common approach) it's ugliness is kept
>> minimal & its cost on binaries & runtime is as low as it can be at zero.
> 
> Even if it costs 4 bytes more, one less ifdef is one good ifdef.
> 
>> Daniel - do you have a preference?
>>
>>>
>>>> +#endif
>>>> +
>>>>  #endif /* _U_BOOT_MIPS_H_ */
>>>> diff --git a/arch/mips/mach-ath79/cpu.c b/arch/mips/mach-ath79/cpu.c
>>>> index 5756a06..a556b73 100644
>>>> --- a/arch/mips/mach-ath79/cpu.c
>>>> +++ b/arch/mips/mach-ath79/cpu.c
>>>> @@ -46,7 +46,7 @@ static const struct ath79_soc_desc desc[] = {
>>>>  	{ATH79_SOC_QCA9561,     "9561", REV_ID_MAJOR_QCA9561,   0},
>>>>  };
>>>>  
>>>> -int arch_cpu_init(void)
>>>> +void mach_cpu_init(void)
>>>
>>> See above.
>>>
>>>>  {
>>>>  	void __iomem *base;
>>>>  	enum ath79_soc_type soc = ATH79_SOC_UNKNOWN;
>>>> @@ -98,7 +98,6 @@ int arch_cpu_init(void)
>>>>  	gd->arch.soc = soc;
>>>>  	gd->arch.rev = rev;
>>>>  	gd->arch.ver = ver;
>>>> -	return 0;
>>>
>>> That's a nope, keep the return value.
>>
>> The implementation always returns zero, so I see no point.
> 
> It does for now, but arch_cpu_init() can fail, so I see a point in
> propagating return values.
>