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 <f...@ltec.ch> >>>> --- >>>> >>>> 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? > Thanks! > -- Felix _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
