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