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.bur...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
