From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Rini Date: Tue, 12 Sep 2017 22:56:45 -0400 Subject: [U-Boot] [PATCH] arm: am33xx: Make pin multiplexing functions optional In-Reply-To: <90e7d653-c994-bc87-cf24-324f1ddb1b72@ltec.ch> References: <1504185377-461-1-git-send-email-fb@ltec.ch> <20170901152128.GA17058@bill-the-cat> <20170907151428.GS17058@bill-the-cat> <90e7d653-c994-bc87-cf24-324f1ddb1b72@ltec.ch> Message-ID: <20170913025645.GY4474@bill-the-cat> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Tue, Sep 12, 2017 at 02:05:01PM +0200, Felix Brack wrote: > > > On 07.09.2017 17:14, Tom Rini wrote: > > On Wed, Sep 06, 2017 at 04:57:52PM +0200, Felix Brack wrote: > >> On 01.09.2017 17:21, Tom Rini wrote: > >>> On Thu, Aug 31, 2017 at 03:16:17PM +0200, Felix Brack wrote: > >>> > >>>> Boards using the single-register-pin-controller can configure all > >>>> pins by means of the device tree. This renders the implementation of > >>>> the two functions set_uart_mux_conf and set_mux_conf_regs obsolete. > >>>> Using the weak attribute for these two function declarations allows > >>>> the omission of the respective definitions. > >>>> > >>>> Signed-off-by: Felix Brack > >>>> --- > >>>> > >>>> arch/arm/include/asm/arch-am33xx/sys_proto.h | 4 ++-- > >>>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/arch/arm/include/asm/arch-am33xx/sys_proto.h b/arch/arm/include/asm/arch-am33xx/sys_proto.h > >>>> index 4e78aaf..e31c25c 100644 > >>>> --- a/arch/arm/include/asm/arch-am33xx/sys_proto.h > >>>> +++ b/arch/arm/include/asm/arch-am33xx/sys_proto.h > >>>> @@ -31,8 +31,8 @@ void enable_gpmc_cs_config(const u32 *gpmc_config, const struct gpmc_cs *cs, u32 > >>>> u32 size); > >>>> int omap_nand_switch_ecc(uint32_t, uint32_t); > >>>> > >>>> -void set_uart_mux_conf(void); > >>>> -void set_mux_conf_regs(void); > >>>> +__weak void set_uart_mux_conf(void); > >>>> +__weak void set_mux_conf_regs(void); > >>>> void sdram_init(void); > >>>> u32 wait_on_value(u32, u32, void *, u32); > >>>> #ifdef CONFIG_NOR_BOOT > >>> > >>> This seems wrong in a few ways. First, this (and the matching ones for > >>> omap3, etc) should be full of externs instead and in that case __weak > >>> makes no sense. Then, on the technical side, what you're describing > >>> isn't quite true in that you're likely relying on having the ROM to have > >>> setup the 'normal' UART still for you, as it so often does, I believe. > >>> Or in the case I'm wrong, then yes, you do get UART to work once we have > >>> parsed the DT, but the "usual" case is that we want UART and thus > >>> printf/etc to be available asap in case of errors, so it's still not a > >>> recommended way to go. Thanks! > >>> > >> > >> Hi Tom, > >> > >> Could you please explain what you mean by "... should be full of externs > >> instead and in that case __weak makes no sense"? > > > > I mean it's a header file. We should only be declaring function > > prototypes, and thus using 'extern' here. It's not the right place to > > have an inline weak function. > > > > Reflecting once more on my patch and after some tests I must admit that > the patch is based on a fundamental misunderstanding of the attribute > 'weak' that could even lead to segmentation faults :$. > The patch, as such, should therefore be discarded. However I will retain > the idea behind the patch (see below). > > >> The pins of the UART I use are not configured by the ROM, it is the pin > >> controller driver configuring them. > >> You are of course right in that UART output is not available before the > >> pin controller driver has executed correctly. In my case the UART is > >> available in 'spl_board_init()'. I know that many things can go wrong > >> before this and therefore configuring UART pins in 'set_uart_mux_conf()' > >> while using 'CONFIG_DEBUG_UART' is fine. In this context the word > >> 'obsolete' may be wrong. The idea behind the patch is to not force the > >> implementation of those two (potentially empty) functions. > > > > I'm open to discussing making these functions be not required but then > > the weak empty function should be under arch/arm/mach-omap2/ somewhere. > > How about adding a 'weak', default, empty implementation of these two > functions to arch/arm/mach-omap2/am33xx/mux.c? Sounds good, thanks! -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: