| Message ID | 1504185377-461-1-git-send-email-fb@ltec.ch |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot] arm: am33xx: Make pin multiplexing functions optional | expand |
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@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!
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@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"? 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.
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@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. > 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!
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@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! >
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@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!
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
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@ltec.ch> --- arch/arm/include/asm/arch-am33xx/sys_proto.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)