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

[Marek Vasut]

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.

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

-- 
Best regards,
Marek Vasut