[U-Boot] [PATCH] arm: am33xx: Make pin multiplexing functions optional

[Felix Brack]

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 <fb at 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