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

[Paul Burton]

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,

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-*.

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

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.

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.

Thanks,
    Paul

> 
>>  }
>>  
>>  int print_cpuinfo(void)
>>
> 
>