From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 708FAC433EF for ; Thu, 16 Dec 2021 18:16:53 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id B376D82A65; Thu, 16 Dec 2021 19:16:50 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.b="rTt3O2oq"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 618BF801CF; Thu, 16 Dec 2021 19:16:49 +0100 (CET) Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id AF48A801CF for ; Thu, 16 Dec 2021 19:16:45 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=pali@kernel.org Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 4D2D3B825EB; Thu, 16 Dec 2021 18:16:45 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id AF08CC36AE0; Thu, 16 Dec 2021 18:16:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1639678604; bh=q+MKuXdHuoG/CCPbWRPplwtns2no2MEbuFAzZ5+NCRs=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=rTt3O2oqs7h/0WsbYG8WpSQBU7pp09kwaDR+BRjM1ZbMmAQVwxHfJgJt0ZrkrAABu rph8AHeodIoghW7N0Evt874TYg4yCYKLoSDO3kkFeCHssAP4tMmmNVww5qgNNOJ7mP 9EFXO9yzRBSc2tNJazhdxCeTgK6KEKW1lXP3xZzQ2ZCJWvD2I4I7YZzNHpai3xvdnY vXbgj6a7rzuOXOM+lHpVISa80sJU32rqgJLrKM7Ik8ns7reP03hHLwnEqwXlqJaN1e AFh/w6LOT0FuJuomZMyeuWLzGjQ6k2VhWUUjT92RjkCkhextlTZzPK3CqNBM03fGPd gcV/2dGbtFenw== Received: by pali.im (Postfix) id DB61688E; Thu, 16 Dec 2021 19:16:40 +0100 (CET) Date: Thu, 16 Dec 2021 19:16:40 +0100 From: Pali =?utf-8?B?Um9ow6Fy?= To: Marek =?utf-8?B?QmVow7pu?= Cc: Stefan Roese , u-boot@lists.denx.de, Marek =?utf-8?B?QmVow7pu?= Subject: Re: [PATCH u-boot-marvell v2 8/9] arm: mvebu: spl: Use IS_ENABLED() instead of #ifdef where possible Message-ID: <20211216181640.c3aolyfege4675ib@pali> References: <20211126143738.23830-1-kabel@kernel.org> <20211126143738.23830-9-kabel@kernel.org> <20211214093600.e2sy5yrtpz7gvfna@pali> <20211214104515.2ae03d0d@thinkpad> <20211214111234.ie54zex6veoirfam@pali> <20211214134536.2baeb2a0@thinkpad> <6cf901fd-01d7-6461-18e4-b362a37bac7c@denx.de> <20211214140104.7a410847@thinkpad> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20211214140104.7a410847@thinkpad> User-Agent: NeoMutt/20180716 X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.38 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean On Tuesday 14 December 2021 14:01:04 Marek Behún wrote: > On Tue, 14 Dec 2021 13:48:31 +0100 > Stefan Roese wrote: > > > On 12/14/21 13:45, Marek Behún wrote: > > > On Tue, 14 Dec 2021 12:12:34 +0100 > > > Pali Rohár 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 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 -O0 is useful for debugging purposes, it generates more readable assembler code. Anyway, the issue here is that those two functions are not defined and implemented for armada 375 soc. #ifdef is here to selectively do not compile code which is not implemented on armada 375. And this cannot be done by normal if(). The reason that it currently works is just because gcc compiler does not do all checks before doing optimizations and so it currently does generate any errors or warnings. But this is just undefined behavior and like any other undefined behavior it may change in some future version of gcc (or changing compiler to some other). This approach with converting correct #ifdef to if() with undefined behavior just hides the real issue that those two functions are not defined and implemented for all mvebu platforms. Why not rather to define these two functions are empty static inline stubs with big comment that they are missing? I think this is proper solution as it does not depends on undefined behavior of compiler and linker, supports also -O0 and removes that #ifdef in spl.c file.