arm: am33xx: Make pin multiplexing functions optional

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 <[email protected]>

---

 arch/arm/include/asm/arch-am33xx/sys_proto.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/arm/include/asm/arch-am33xx/sys_proto.h +0 -0

index 4e78aaf..e31c25c 100644

Quoted 17 lines · Felix Brack

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 <[email protected]>

---

 arch/arm/include/asm/arch-am33xx/sys_proto.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/arm/include/asm/arch-am33xx/sys_proto.h +0 -0

index 4e78aaf..e31c25c 100644

arch/arm/include/asm/arch-am33xx/sys_proto.h void enable_gpmc_cs_config(const u32 *gpmc_config, const struct gpmc_cs *cs, u32 · line 38

Quoted 10 lines

arch/arm/include/asm/arch-am33xx/sys_proto.h +2 -2

@@ -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!

Quoted 31 lines · Tom Rini

Quoted 17 lines · Felix Brack

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 <[email protected]>

---

 arch/arm/include/asm/arch-am33xx/sys_proto.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/arm/include/asm/arch-am33xx/sys_proto.h +0 -0

index 4e78aaf..e31c25c 100644

arch/arm/include/asm/arch-am33xx/sys_proto.h void enable_gpmc_cs_config(const u32 *gpmc_config, const struct gpmc_cs *cs, u32 · line 38

Quoted 10 lines

arch/arm/include/asm/arch-am33xx/sys_proto.h +2 -2

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

Quoted 9 lines

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

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.

Quoted 42 lines · Felix Brack

Quoted 40 lines · Tom Rini

Quoted 17 lines · Felix Brack

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 <[email protected]>

---

 arch/arm/include/asm/arch-am33xx/sys_proto.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/arm/include/asm/arch-am33xx/sys_proto.h +0 -0

index 4e78aaf..e31c25c 100644

arch/arm/include/asm/arch-am33xx/sys_proto.h void enable_gpmc_cs_config(const u32 *gpmc_config, const struct gpmc_cs *cs, u32 · line 38

Quoted 10 lines

arch/arm/include/asm/arch-am33xx/sys_proto.h +2 -2

@@ -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!

Quoted 4 lines

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.

Quoted 9 lines

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.

Thanks!

Quoted 43 lines · Tom Rini

Quoted 42 lines · Felix Brack

Quoted 40 lines · Tom Rini

Quoted 17 lines · Felix Brack

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 <[email protected]>

---

 arch/arm/include/asm/arch-am33xx/sys_proto.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/arm/include/asm/arch-am33xx/sys_proto.h +0 -0

index 4e78aaf..e31c25c 100644

arch/arm/include/asm/arch-am33xx/sys_proto.h void enable_gpmc_cs_config(const u32 *gpmc_config, const struct gpmc_cs *cs, u32 · line 38

Quoted 10 lines

arch/arm/include/asm/arch-am33xx/sys_proto.h +2 -2

@@ -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!

Quoted 9 lines

Quoted 4 lines

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).

Quoted 10 lines

Quoted 9 lines

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.

Quoted 2 lines

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?