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

[Tom Rini]

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 <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?

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: <http://lists.denx.de/pipermail/u-boot/attachments/20170912/ea87ba4c/attachment.sig>