On Tue, 14 Dec 2021 13:48:31 +0100 Stefan Roese <[email protected]> wrote:
> On 12/14/21 13:45, Marek Behún wrote: > > On Tue, 14 Dec 2021 12:12:34 +0100 > > Pali Rohár <[email protected]> wrote: > > > >> On Tuesday 14 December 2021 10:45:15 Marek Behún wrote: > >>> On Tue, 14 Dec 2021 10:36:00 +0100 > >>> Pali Rohár <[email protected]> wrote: > >>> > >>>> On Friday 26 November 2021 15:37:37 Marek Behún wrote: > >>>>> @@ -340,17 +333,17 @@ void board_init_f(ulong dummy) > >>>>> timer_init(); > >>>>> > >>>>> /* Armada 375 does not support SerDes and DDR3 init yet */ > >>>>> -#if !defined(CONFIG_ARMADA_375) > >>>>> - /* First init the serdes PHY's */ > >>>>> - serdes_phy_config(); > >>>>> - > >>>>> - /* Setup DDR */ > >>>>> - ret = ddr3_init(); > >>>>> - if (ret) { > >>>>> - debug("ddr3_init() failed: %d\n", ret); > >>>>> - hang(); > >>>>> + if (!IS_ENABLED(CONFIG_ARMADA_375)) { > >>>>> + /* First init the serdes PHY's */ > >>>>> + serdes_phy_config(); > >>>>> + > >>>>> + /* Setup DDR */ > >>>>> + ret = ddr3_init(); > >>>>> + if (ret) { > >>>>> + debug("ddr3_init() failed: %d\n", ret); > >>>>> + hang(); > >>>>> + } > >>>>> } > >>>>> -#endif > >>>> > >>>> As written in comment above there is no SerDes and DDR3 support for > >>>> Armada 375 and therefore there is no serdes_phy_config() or ddr3_init() > >>>> function. So this code needs not to be compiled at all and usage of > >>>> #ifdef is correct here. > >>> > >>> #ifdefs are almost never correct in C-files, for the parts of the code > >>> they guard isn't put through syntactic analysis, and can therefore > >>> contain bugs which we are not warned about. > >>> > >>> Using if (IS_ENABLED()) almost never producess a different binary, > >>> because the code is optimized away. > >>> > >>> Marek > >> > >> There is no function serdes_phy_config() for Armada 375, so if you put > >> it outside of #ifdef you will get compile error. > > > > The function is always declared in > > arch/arm/mach-mvebu/include/mach/cpu.h > > regardless of architecture. > > > > Thus an error will be raised only when linking, and the compliation was > > done with -O0, which I don't think anyone does. > > > > Anyway, if we want to support -O0, this can and should be solved via > > defining serdes_phy_config() as empty static inline function in the > > cpu.h header, guarded by #ifdef. In header files #ifdefs are allowed, > > in this manner: > > #if X > > declare function > > #else > > define that function as empty static inline > > #endif > > > > So if you think we should support -O0, I can do this. > > > > But the #ifdefs should really go away from real C code, that is the way > > both Linux and U-Boot are trying to go for the last couple of years, if > > I understand it correctly. > > Yes, the #ifdef's really should be avoided if possible. So *if* your > patch above does not generate any build issues, then I don't see any > problems to include it. I personally don't think that we need to support > -O0 builds. db-88f6720_defconfig builds without issue (armada 375). And it builds the relevant file, spl/arch/arm/mach-mvebu/spl.o. Marek
