diff mbox

[U-Boot,1/3] common: add ifdefs around bouncebuf.c body

Message ID 1352156642-7975-1-git-send-email-swarren@wwwdotorg.org
State Superseded
Headers show

Commit Message

Stephen Warren Nov. 5, 2012, 11:04 p.m. UTC
From: Stephen Warren <swarren@nvidia.com>

If a U-Boot config file enables CONFIG_BOUNCE_BUFFER only for the main
U-Boot build and not for the SPL, then config.mk will contain
CONFIG_BOUNCE_BUFFER=y, so common/Makefile will build bouncebuf.c for
both the SPL and main U-Boot, but config.h won't set CONFIG_BOUNCE_BUFFER
for the SPL, so bouncebuf.h will provide static inline functions, which
will conflict with the compiled bouncebuf.c. Solve this by guarding the
body of bouncebuf.c with the ifdef to avoid conflicts.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
This series is based on u-boot/master. I've CC'd the MMC and Tegra
maintainers since they'll presumably need to ack the changes in order for
these patches to all be applied in one place. Marek seems to be the main
MXS MMC maintainer as far as I can tell.

 common/bouncebuf.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

Comments

Simon Glass Nov. 5, 2012, 11:47 p.m. UTC | #1
Hi Stephen,

On Mon, Nov 5, 2012 at 3:04 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> From: Stephen Warren <swarren@nvidia.com>
>
> If a U-Boot config file enables CONFIG_BOUNCE_BUFFER only for the main
> U-Boot build and not for the SPL, then config.mk will contain
> CONFIG_BOUNCE_BUFFER=y, so common/Makefile will build bouncebuf.c for
> both the SPL and main U-Boot, but config.h won't set CONFIG_BOUNCE_BUFFER
> for the SPL, so bouncebuf.h will provide static inline functions, which
> will conflict with the compiled bouncebuf.c. Solve this by guarding the
> body of bouncebuf.c with the ifdef to avoid conflicts.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

Acked-by: Simon Glass <sjg@chromium.org>

This seems like a problem that might come up in other areas. I wonder
if SPL should have its own autoconf.mk?

> ---
> This series is based on u-boot/master. I've CC'd the MMC and Tegra
> maintainers since they'll presumably need to ack the changes in order for
> these patches to all be applied in one place. Marek seems to be the main
> MXS MMC maintainer as far as I can tell.
>
>  common/bouncebuf.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/common/bouncebuf.c b/common/bouncebuf.c
> index 4f827f8..ffd3c90 100644
> --- a/common/bouncebuf.c
> +++ b/common/bouncebuf.c
> @@ -27,6 +27,7 @@
>  #include <errno.h>
>  #include <bouncebuf.h>
>
> +#ifdef CONFIG_BOUNCE_BUFFER
>  static int addr_aligned(void *data, size_t len)
>  {
>         const ulong align_mask = ARCH_DMA_MINALIGN - 1;
> @@ -90,3 +91,4 @@ int bounce_buffer_stop(void **data, size_t len, void **backup, uint8_t flags)
>
>         return 0;
>  }
> +#endif
> --
> 1.7.0.4
>

Regards,
Simon
Marek Vasut Nov. 6, 2012, 12:54 a.m. UTC | #2
Dear Stephen Warren,

> From: Stephen Warren <swarren@nvidia.com>
> 
> If a U-Boot config file enables CONFIG_BOUNCE_BUFFER only for the main
> U-Boot build and not for the SPL, then config.mk will contain
> CONFIG_BOUNCE_BUFFER=y, so common/Makefile will build bouncebuf.c for
> both the SPL and main U-Boot, but config.h won't set CONFIG_BOUNCE_BUFFER
> for the SPL, so bouncebuf.h will provide static inline functions, which
> will conflict with the compiled bouncebuf.c. Solve this by guarding the
> body of bouncebuf.c with the ifdef to avoid conflicts.

Uh, don't you want the bounce buffer not compiled in for SPL? Then maybe add 
CONFIG_SPL_BOUNCE_BUFFER to force BB to be compiled into SPL or something ...

> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> This series is based on u-boot/master. I've CC'd the MMC and Tegra
> maintainers since they'll presumably need to ack the changes in order for
> these patches to all be applied in one place. Marek seems to be the main
> MXS MMC maintainer as far as I can tell.

What did I get myself into ... ;-)

>  common/bouncebuf.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/common/bouncebuf.c b/common/bouncebuf.c
> index 4f827f8..ffd3c90 100644
> --- a/common/bouncebuf.c
> +++ b/common/bouncebuf.c
> @@ -27,6 +27,7 @@
>  #include <errno.h>
>  #include <bouncebuf.h>
> 
> +#ifdef CONFIG_BOUNCE_BUFFER
>  static int addr_aligned(void *data, size_t len)
>  {
>  	const ulong align_mask = ARCH_DMA_MINALIGN - 1;
> @@ -90,3 +91,4 @@ int bounce_buffer_stop(void **data, size_t len, void
> **backup, uint8_t flags)
> 
>  	return 0;
>  }
> +#endif

Best regards,
Marek Vasut
Stephen Warren Nov. 6, 2012, 6:04 p.m. UTC | #3
On 11/05/2012 04:47 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Mon, Nov 5, 2012 at 3:04 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> If a U-Boot config file enables CONFIG_BOUNCE_BUFFER only for the main
>> U-Boot build and not for the SPL, then config.mk will contain
>> CONFIG_BOUNCE_BUFFER=y, so common/Makefile will build bouncebuf.c for
>> both the SPL and main U-Boot, but config.h won't set CONFIG_BOUNCE_BUFFER
>> for the SPL, so bouncebuf.h will provide static inline functions, which
>> will conflict with the compiled bouncebuf.c. Solve this by guarding the
>> body of bouncebuf.c with the ifdef to avoid conflicts.
>>
>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> 
> Acked-by: Simon Glass <sjg@chromium.org>
> 
> This seems like a problem that might come up in other areas. I wonder
> if SPL should have its own autoconf.mk?

That might be a good idea. Is the config.h separate for SPL-vs-non-SPL?
Perhaps it doesn't need to be because it's simply always evaluated at
each individual compile time, whereas perhaps autoconf.mk is generated
once rather than evaluated? As you can tell, I have not looked into this
or most aspects of U-Boot's build system, so I have no idea how feasible
fixing the build system for this would be.
Stephen Warren Nov. 6, 2012, 6:07 p.m. UTC | #4
On 11/05/2012 05:54 PM, Marek Vasut wrote:
> Dear Stephen Warren,
> 
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> If a U-Boot config file enables CONFIG_BOUNCE_BUFFER only for the main
>> U-Boot build and not for the SPL, then config.mk will contain
>> CONFIG_BOUNCE_BUFFER=y, so common/Makefile will build bouncebuf.c for
>> both the SPL and main U-Boot, but config.h won't set CONFIG_BOUNCE_BUFFER
>> for the SPL, so bouncebuf.h will provide static inline functions, which
>> will conflict with the compiled bouncebuf.c. Solve this by guarding the
>> body of bouncebuf.c with the ifdef to avoid conflicts.
> 
> Uh, don't you want the bounce buffer not compiled in for SPL? Then maybe add 
> CONFIG_SPL_BOUNCE_BUFFER to force BB to be compiled into SPL or something ...

Not compiling bouncebuf.c for SPL would solve this too. I have no idea
what build system contortions would be required to do this though. Do
you think the build system should be fixed first rather than taking this
series/patch?

I guess we shouldn't need a separate CONFIG_SPL_BOUNCE_BUFFER option
though; we should rather simply set CONFIG_SPL_BOUNCE_BUFFER
appropriately for SPL and non-SPL, and have everything key off that one
variable, right?
Marek Vasut Nov. 6, 2012, 10:43 p.m. UTC | #5
Dear Stephen Warren,

> On 11/05/2012 05:54 PM, Marek Vasut wrote:
> > Dear Stephen Warren,
> > 
> >> From: Stephen Warren <swarren@nvidia.com>
> >> 
> >> If a U-Boot config file enables CONFIG_BOUNCE_BUFFER only for the main
> >> U-Boot build and not for the SPL, then config.mk will contain
> >> CONFIG_BOUNCE_BUFFER=y, so common/Makefile will build bouncebuf.c for
> >> both the SPL and main U-Boot, but config.h won't set
> >> CONFIG_BOUNCE_BUFFER for the SPL, so bouncebuf.h will provide static
> >> inline functions, which will conflict with the compiled bouncebuf.c.
> >> Solve this by guarding the body of bouncebuf.c with the ifdef to avoid
> >> conflicts.
> > 
> > Uh, don't you want the bounce buffer not compiled in for SPL? Then maybe
> > add CONFIG_SPL_BOUNCE_BUFFER to force BB to be compiled into SPL or
> > something ...
> 
> Not compiling bouncebuf.c for SPL would solve this too. I have no idea
> what build system contortions would be required to do this though. Do
> you think the build system should be fixed first rather than taking this
> series/patch?
> 
> I guess we shouldn't need a separate CONFIG_SPL_BOUNCE_BUFFER option
> though; we should rather simply set CONFIG_SPL_BOUNCE_BUFFER
> appropriately for SPL and non-SPL, and have everything key off that one
> variable, right?

How will you be able to configure it separately for spl and non-spl ?

Best regards,
Marek Vasut
Stephen Warren Nov. 6, 2012, 10:49 p.m. UTC | #6
On 11/06/2012 03:43 PM, Marek Vasut wrote:
> Dear Stephen Warren,
> 
>> On 11/05/2012 05:54 PM, Marek Vasut wrote:
>>> Dear Stephen Warren,
>>>
>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>
>>>> If a U-Boot config file enables CONFIG_BOUNCE_BUFFER only for the main
>>>> U-Boot build and not for the SPL, then config.mk will contain
>>>> CONFIG_BOUNCE_BUFFER=y, so common/Makefile will build bouncebuf.c for
>>>> both the SPL and main U-Boot, but config.h won't set
>>>> CONFIG_BOUNCE_BUFFER for the SPL, so bouncebuf.h will provide static
>>>> inline functions, which will conflict with the compiled bouncebuf.c.
>>>> Solve this by guarding the body of bouncebuf.c with the ifdef to avoid
>>>> conflicts.
>>>
>>> Uh, don't you want the bounce buffer not compiled in for SPL? Then maybe
>>> add CONFIG_SPL_BOUNCE_BUFFER to force BB to be compiled into SPL or
>>> something ...
>>
>> Not compiling bouncebuf.c for SPL would solve this too. I have no idea
>> what build system contortions would be required to do this though. Do
>> you think the build system should be fixed first rather than taking this
>> series/patch?
>>
>> I guess we shouldn't need a separate CONFIG_SPL_BOUNCE_BUFFER option
>> though; we should rather simply set CONFIG_SPL_BOUNCE_BUFFER
>> appropriately for SPL and non-SPL, and have everything key off that one
>> variable, right?
> 
> How will you be able to configure it separately for spl and non-spl ?

For example,

include/configs/trimslice.h contains:

/* SD/MMC */
#define CONFIG_MMC
#define CONFIG_GENERIC_MMC
#define CONFIG_TEGRA_MMC
#define CONFIG_CMD_MMC

However, we don't use MMC in our SPL, but don't want to pollute every
Tegra board file with ifdefs for SPL, so
include/configs/tegra-common-post.h (which is included at the end of
trimslice.h) contains:

#ifdef CONFIG_SPL_BUILD
...
/* remove MMC support */
#ifdef CONFIG_MMC
#undef CONFIG_MMC
#endif
#ifdef CONFIG_GENERIC_MMC
#undef CONFIG_GENERIC_MMC
#endif
#ifdef CONFIG_TEGRA_MMC
#undef CONFIG_TEGRA_MMC
#endif
#ifdef CONFIG_CMD_MMC
#undef CONFIG_CMD_MMC
#endif
...
#endif

And in the V1 patch I proposed to tegra-common-post.h, I added the
following at the end:

#ifdef CONFIG_TEGRA_MMC
#define CONFIG_BOUNCE_BUFFER
#endif
Marek Vasut Nov. 6, 2012, 10:57 p.m. UTC | #7
Dear Stephen Warren,

> On 11/06/2012 03:43 PM, Marek Vasut wrote:
> > Dear Stephen Warren,
> > 
> >> On 11/05/2012 05:54 PM, Marek Vasut wrote:
> >>> Dear Stephen Warren,
> >>> 
> >>>> From: Stephen Warren <swarren@nvidia.com>
> >>>> 
> >>>> If a U-Boot config file enables CONFIG_BOUNCE_BUFFER only for the main
> >>>> U-Boot build and not for the SPL, then config.mk will contain
> >>>> CONFIG_BOUNCE_BUFFER=y, so common/Makefile will build bouncebuf.c for
> >>>> both the SPL and main U-Boot, but config.h won't set
> >>>> CONFIG_BOUNCE_BUFFER for the SPL, so bouncebuf.h will provide static
> >>>> inline functions, which will conflict with the compiled bouncebuf.c.
> >>>> Solve this by guarding the body of bouncebuf.c with the ifdef to avoid
> >>>> conflicts.
> >>> 
> >>> Uh, don't you want the bounce buffer not compiled in for SPL? Then
> >>> maybe add CONFIG_SPL_BOUNCE_BUFFER to force BB to be compiled into SPL
> >>> or something ...
> >> 
> >> Not compiling bouncebuf.c for SPL would solve this too. I have no idea
> >> what build system contortions would be required to do this though. Do
> >> you think the build system should be fixed first rather than taking this
> >> series/patch?
> >> 
> >> I guess we shouldn't need a separate CONFIG_SPL_BOUNCE_BUFFER option
> >> though; we should rather simply set CONFIG_SPL_BOUNCE_BUFFER
> >> appropriately for SPL and non-SPL, and have everything key off that one
> >> variable, right?
> > 
> > How will you be able to configure it separately for spl and non-spl ?
> 
> For example,
> 
> include/configs/trimslice.h contains:
> 
> /* SD/MMC */
> #define CONFIG_MMC
> #define CONFIG_GENERIC_MMC
> #define CONFIG_TEGRA_MMC
> #define CONFIG_CMD_MMC
> 
> However, we don't use MMC in our SPL, but don't want to pollute every
> Tegra board file with ifdefs for SPL, so
> include/configs/tegra-common-post.h (which is included at the end of
> trimslice.h) contains:
> 
> #ifdef CONFIG_SPL_BUILD
> ...
> /* remove MMC support */
> #ifdef CONFIG_MMC
> #undef CONFIG_MMC
> #endif
> #ifdef CONFIG_GENERIC_MMC
> #undef CONFIG_GENERIC_MMC
> #endif
> #ifdef CONFIG_TEGRA_MMC
> #undef CONFIG_TEGRA_MMC
> #endif
> #ifdef CONFIG_CMD_MMC
> #undef CONFIG_CMD_MMC
> #endif
> ...
> #endif
> 
> And in the V1 patch I proposed to tegra-common-post.h, I added the
> following at the end:
> 
> #ifdef CONFIG_TEGRA_MMC
> #define CONFIG_BOUNCE_BUFFER
> #endif

Yet, this doesn't solve the problem with SPL ... since for SPL, you'd have to do 
ifdef CONFIG_SPL, no?

Best regards,
Marek Vasut
Stephen Warren Nov. 6, 2012, 11:13 p.m. UTC | #8
On 11/06/2012 03:57 PM, Marek Vasut wrote:
> Dear Stephen Warren,
> 
>> On 11/06/2012 03:43 PM, Marek Vasut wrote:
>>> Dear Stephen Warren,
>>>
>>>> On 11/05/2012 05:54 PM, Marek Vasut wrote:
>>>>> Dear Stephen Warren,
>>>>>
>>>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>>>
>>>>>> If a U-Boot config file enables CONFIG_BOUNCE_BUFFER only for the main
>>>>>> U-Boot build and not for the SPL, then config.mk will contain
>>>>>> CONFIG_BOUNCE_BUFFER=y, so common/Makefile will build bouncebuf.c for
>>>>>> both the SPL and main U-Boot, but config.h won't set
>>>>>> CONFIG_BOUNCE_BUFFER for the SPL, so bouncebuf.h will provide static
>>>>>> inline functions, which will conflict with the compiled bouncebuf.c.
>>>>>> Solve this by guarding the body of bouncebuf.c with the ifdef to avoid
>>>>>> conflicts.
>>>>>
>>>>> Uh, don't you want the bounce buffer not compiled in for SPL? Then
>>>>> maybe add CONFIG_SPL_BOUNCE_BUFFER to force BB to be compiled into SPL
>>>>> or something ...
>>>>
>>>> Not compiling bouncebuf.c for SPL would solve this too. I have no idea
>>>> what build system contortions would be required to do this though. Do
>>>> you think the build system should be fixed first rather than taking this
>>>> series/patch?
>>>>
>>>> I guess we shouldn't need a separate CONFIG_SPL_BOUNCE_BUFFER option
>>>> though; we should rather simply set CONFIG_SPL_BOUNCE_BUFFER
>>>> appropriately for SPL and non-SPL, and have everything key off that one
>>>> variable, right?
>>>
>>> How will you be able to configure it separately for spl and non-spl ?
>>
>> For example,
>>
>> include/configs/trimslice.h contains:
>>
>> /* SD/MMC */
>> #define CONFIG_MMC
>> #define CONFIG_GENERIC_MMC
>> #define CONFIG_TEGRA_MMC
>> #define CONFIG_CMD_MMC
>>
>> However, we don't use MMC in our SPL, but don't want to pollute every
>> Tegra board file with ifdefs for SPL, so
>> include/configs/tegra-common-post.h (which is included at the end of
>> trimslice.h) contains:
>>
>> #ifdef CONFIG_SPL_BUILD
>> ...
>> /* remove MMC support */
>> #ifdef CONFIG_MMC
>> #undef CONFIG_MMC
>> #endif
>> #ifdef CONFIG_GENERIC_MMC
>> #undef CONFIG_GENERIC_MMC
>> #endif
>> #ifdef CONFIG_TEGRA_MMC
>> #undef CONFIG_TEGRA_MMC
>> #endif
>> #ifdef CONFIG_CMD_MMC
>> #undef CONFIG_CMD_MMC
>> #endif
>> ...
>> #endif
>>
>> And in the V1 patch I proposed to tegra-common-post.h, I added the
>> following at the end:
>>
>> #ifdef CONFIG_TEGRA_MMC
>> #define CONFIG_BOUNCE_BUFFER
>> #endif
> 
> Yet, this doesn't solve the problem with SPL ... since for SPL, you'd have to do 
> ifdef CONFIG_SPL, no?

Sorry, what problem with the SPL is this not solving?
Marek Vasut Nov. 7, 2012, 1:21 p.m. UTC | #9
Dear Stephen Warren,

> On 11/06/2012 03:57 PM, Marek Vasut wrote:
> > Dear Stephen Warren,
> > 
> >> On 11/06/2012 03:43 PM, Marek Vasut wrote:
> >>> Dear Stephen Warren,
> >>> 
> >>>> On 11/05/2012 05:54 PM, Marek Vasut wrote:
> >>>>> Dear Stephen Warren,
> >>>>> 
> >>>>>> From: Stephen Warren <swarren@nvidia.com>
> >>>>>> 
> >>>>>> If a U-Boot config file enables CONFIG_BOUNCE_BUFFER only for the
> >>>>>> main U-Boot build and not for the SPL, then config.mk will contain
> >>>>>> CONFIG_BOUNCE_BUFFER=y, so common/Makefile will build bouncebuf.c
> >>>>>> for both the SPL and main U-Boot, but config.h won't set
> >>>>>> CONFIG_BOUNCE_BUFFER for the SPL, so bouncebuf.h will provide static
> >>>>>> inline functions, which will conflict with the compiled bouncebuf.c.
> >>>>>> Solve this by guarding the body of bouncebuf.c with the ifdef to
> >>>>>> avoid conflicts.
> >>>>> 
> >>>>> Uh, don't you want the bounce buffer not compiled in for SPL? Then
> >>>>> maybe add CONFIG_SPL_BOUNCE_BUFFER to force BB to be compiled into
> >>>>> SPL or something ...
> >>>> 
> >>>> Not compiling bouncebuf.c for SPL would solve this too. I have no idea
> >>>> what build system contortions would be required to do this though. Do
> >>>> you think the build system should be fixed first rather than taking
> >>>> this series/patch?
> >>>> 
> >>>> I guess we shouldn't need a separate CONFIG_SPL_BOUNCE_BUFFER option
> >>>> though; we should rather simply set CONFIG_SPL_BOUNCE_BUFFER
> >>>> appropriately for SPL and non-SPL, and have everything key off that
> >>>> one variable, right?
> >>> 
> >>> How will you be able to configure it separately for spl and non-spl ?
> >> 
> >> For example,
> >> 
> >> include/configs/trimslice.h contains:
> >> 
> >> /* SD/MMC */
> >> #define CONFIG_MMC
> >> #define CONFIG_GENERIC_MMC
> >> #define CONFIG_TEGRA_MMC
> >> #define CONFIG_CMD_MMC
> >> 
> >> However, we don't use MMC in our SPL, but don't want to pollute every
> >> Tegra board file with ifdefs for SPL, so
> >> include/configs/tegra-common-post.h (which is included at the end of
> >> trimslice.h) contains:
> >> 
> >> #ifdef CONFIG_SPL_BUILD
> >> ...
> >> /* remove MMC support */
> >> #ifdef CONFIG_MMC
> >> #undef CONFIG_MMC
> >> #endif
> >> #ifdef CONFIG_GENERIC_MMC
> >> #undef CONFIG_GENERIC_MMC
> >> #endif
> >> #ifdef CONFIG_TEGRA_MMC
> >> #undef CONFIG_TEGRA_MMC
> >> #endif
> >> #ifdef CONFIG_CMD_MMC
> >> #undef CONFIG_CMD_MMC
> >> #endif
> >> ...
> >> #endif
> >> 
> >> And in the V1 patch I proposed to tegra-common-post.h, I added the
> >> following at the end:
> >> 
> >> #ifdef CONFIG_TEGRA_MMC
> >> #define CONFIG_BOUNCE_BUFFER
> >> #endif
> > 
> > Yet, this doesn't solve the problem with SPL ... since for SPL, you'd
> > have to do ifdef CONFIG_SPL, no?
> 
> Sorry, what problem with the SPL is this not solving?

I think I was tired when replying (sorry, the conference is really heavy on me). 
I though you wanted to disable BB only for SPL, but now I see BB being enabled 
depends on tegra mmc.

btw. shouldn't --gc-sections remove BB code if it's not used at all?

Best regards,
Stephen Warren Nov. 7, 2012, 5 p.m. UTC | #10
On 11/07/2012 06:21 AM, Marek Vasut wrote:
> Dear Stephen Warren,
> 
>> On 11/06/2012 03:57 PM, Marek Vasut wrote:
>>> Dear Stephen Warren,
>>>
>>>> On 11/06/2012 03:43 PM, Marek Vasut wrote:
>>>>> Dear Stephen Warren,
>>>>>
>>>>>> On 11/05/2012 05:54 PM, Marek Vasut wrote:
>>>>>>> Dear Stephen Warren,
>>>>>>>
>>>>>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>>>>>
>>>>>>>> If a U-Boot config file enables CONFIG_BOUNCE_BUFFER only for the
>>>>>>>> main U-Boot build and not for the SPL, then config.mk will contain
>>>>>>>> CONFIG_BOUNCE_BUFFER=y, so common/Makefile will build bouncebuf.c
>>>>>>>> for both the SPL and main U-Boot, but config.h won't set
>>>>>>>> CONFIG_BOUNCE_BUFFER for the SPL, so bouncebuf.h will provide static
>>>>>>>> inline functions, which will conflict with the compiled bouncebuf.c.
>>>>>>>> Solve this by guarding the body of bouncebuf.c with the ifdef to
>>>>>>>> avoid conflicts.
>>>>>>>
>>>>>>> Uh, don't you want the bounce buffer not compiled in for SPL? Then
>>>>>>> maybe add CONFIG_SPL_BOUNCE_BUFFER to force BB to be compiled into
>>>>>>> SPL or something ...
>>>>>>
>>>>>> Not compiling bouncebuf.c for SPL would solve this too. I have no idea
>>>>>> what build system contortions would be required to do this though. Do
>>>>>> you think the build system should be fixed first rather than taking
>>>>>> this series/patch?
>>>>>>
>>>>>> I guess we shouldn't need a separate CONFIG_SPL_BOUNCE_BUFFER option
>>>>>> though; we should rather simply set CONFIG_SPL_BOUNCE_BUFFER
>>>>>> appropriately for SPL and non-SPL, and have everything key off that
>>>>>> one variable, right?
>>>>>
>>>>> How will you be able to configure it separately for spl and non-spl ?
>>>>
>>>> For example,
>>>>
>>>> include/configs/trimslice.h contains:
>>>>
>>>> /* SD/MMC */
>>>> #define CONFIG_MMC
>>>> #define CONFIG_GENERIC_MMC
>>>> #define CONFIG_TEGRA_MMC
>>>> #define CONFIG_CMD_MMC
>>>>
>>>> However, we don't use MMC in our SPL, but don't want to pollute every
>>>> Tegra board file with ifdefs for SPL, so
>>>> include/configs/tegra-common-post.h (which is included at the end of
>>>> trimslice.h) contains:
>>>>
>>>> #ifdef CONFIG_SPL_BUILD
>>>> ...
>>>> /* remove MMC support */
>>>> #ifdef CONFIG_MMC
>>>> #undef CONFIG_MMC
>>>> #endif
>>>> #ifdef CONFIG_GENERIC_MMC
>>>> #undef CONFIG_GENERIC_MMC
>>>> #endif
>>>> #ifdef CONFIG_TEGRA_MMC
>>>> #undef CONFIG_TEGRA_MMC
>>>> #endif
>>>> #ifdef CONFIG_CMD_MMC
>>>> #undef CONFIG_CMD_MMC
>>>> #endif
>>>> ...
>>>> #endif
>>>>
>>>> And in the V1 patch I proposed to tegra-common-post.h, I added the
>>>> following at the end:
>>>>
>>>> #ifdef CONFIG_TEGRA_MMC
>>>> #define CONFIG_BOUNCE_BUFFER
>>>> #endif
>>>
>>> Yet, this doesn't solve the problem with SPL ... since for SPL, you'd
>>> have to do ifdef CONFIG_SPL, no?
>>
>> Sorry, what problem with the SPL is this not solving?
> 
> I think I was tired when replying (sorry, the conference is really heavy on me). 
> I though you wanted to disable BB only for SPL, but now I see BB being enabled 
> depends on tegra mmc.
> 
> btw. shouldn't --gc-sections remove BB code if it's not used at all?

Yes, I assume so. In v2 of the patch series I have simply enabled
CONFIG_BOUNCE_BUFFER unconditionally on Tegra, which removes the need
for any SPL-specific changes.

Before enabling it:

Configuring for trimslice board...
   text	   data	    bss	    dec	    hex	filename
 245625	   9304	 274968	 529897	  815e9	./u-boot
  14451	    208	     72	  14731	   398b	./spl/u-boot-spl

After enabling it:

Configuring for trimslice board...
   text	   data	    bss	    dec	    hex	filename
 245742	   9304	 274964	 530010	  8165a	./u-boot
  14451	    208	     72	  14731	   398b	./spl/u-boot-spl

SPL didn't change since, so this seems to be working fine.
Marek Vasut Nov. 8, 2012, 1:20 a.m. UTC | #11
Dear Stephen Warren,

> On 11/07/2012 06:21 AM, Marek Vasut wrote:
> > Dear Stephen Warren,
> > 
> >> On 11/06/2012 03:57 PM, Marek Vasut wrote:
> >>> Dear Stephen Warren,
> >>> 
> >>>> On 11/06/2012 03:43 PM, Marek Vasut wrote:
> >>>>> Dear Stephen Warren,
> >>>>> 
> >>>>>> On 11/05/2012 05:54 PM, Marek Vasut wrote:
> >>>>>>> Dear Stephen Warren,
> >>>>>>> 
> >>>>>>>> From: Stephen Warren <swarren@nvidia.com>
> >>>>>>>> 
> >>>>>>>> If a U-Boot config file enables CONFIG_BOUNCE_BUFFER only for the
> >>>>>>>> main U-Boot build and not for the SPL, then config.mk will contain
> >>>>>>>> CONFIG_BOUNCE_BUFFER=y, so common/Makefile will build bouncebuf.c
> >>>>>>>> for both the SPL and main U-Boot, but config.h won't set
> >>>>>>>> CONFIG_BOUNCE_BUFFER for the SPL, so bouncebuf.h will provide
> >>>>>>>> static inline functions, which will conflict with the compiled
> >>>>>>>> bouncebuf.c. Solve this by guarding the body of bouncebuf.c with
> >>>>>>>> the ifdef to avoid conflicts.
> >>>>>>> 
> >>>>>>> Uh, don't you want the bounce buffer not compiled in for SPL? Then
> >>>>>>> maybe add CONFIG_SPL_BOUNCE_BUFFER to force BB to be compiled into
> >>>>>>> SPL or something ...
> >>>>>> 
> >>>>>> Not compiling bouncebuf.c for SPL would solve this too. I have no
> >>>>>> idea what build system contortions would be required to do this
> >>>>>> though. Do you think the build system should be fixed first rather
> >>>>>> than taking this series/patch?
> >>>>>> 
> >>>>>> I guess we shouldn't need a separate CONFIG_SPL_BOUNCE_BUFFER option
> >>>>>> though; we should rather simply set CONFIG_SPL_BOUNCE_BUFFER
> >>>>>> appropriately for SPL and non-SPL, and have everything key off that
> >>>>>> one variable, right?
> >>>>> 
> >>>>> How will you be able to configure it separately for spl and non-spl ?
> >>>> 
> >>>> For example,
> >>>> 
> >>>> include/configs/trimslice.h contains:
> >>>> 
> >>>> /* SD/MMC */
> >>>> #define CONFIG_MMC
> >>>> #define CONFIG_GENERIC_MMC
> >>>> #define CONFIG_TEGRA_MMC
> >>>> #define CONFIG_CMD_MMC
> >>>> 
> >>>> However, we don't use MMC in our SPL, but don't want to pollute every
> >>>> Tegra board file with ifdefs for SPL, so
> >>>> include/configs/tegra-common-post.h (which is included at the end of
> >>>> trimslice.h) contains:
> >>>> 
> >>>> #ifdef CONFIG_SPL_BUILD
> >>>> ...
> >>>> /* remove MMC support */
> >>>> #ifdef CONFIG_MMC
> >>>> #undef CONFIG_MMC
> >>>> #endif
> >>>> #ifdef CONFIG_GENERIC_MMC
> >>>> #undef CONFIG_GENERIC_MMC
> >>>> #endif
> >>>> #ifdef CONFIG_TEGRA_MMC
> >>>> #undef CONFIG_TEGRA_MMC
> >>>> #endif
> >>>> #ifdef CONFIG_CMD_MMC
> >>>> #undef CONFIG_CMD_MMC
> >>>> #endif
> >>>> ...
> >>>> #endif
> >>>> 
> >>>> And in the V1 patch I proposed to tegra-common-post.h, I added the
> >>>> following at the end:
> >>>> 
> >>>> #ifdef CONFIG_TEGRA_MMC
> >>>> #define CONFIG_BOUNCE_BUFFER
> >>>> #endif
> >>> 
> >>> Yet, this doesn't solve the problem with SPL ... since for SPL, you'd
> >>> have to do ifdef CONFIG_SPL, no?
> >> 
> >> Sorry, what problem with the SPL is this not solving?
> > 
> > I think I was tired when replying (sorry, the conference is really heavy
> > on me). I though you wanted to disable BB only for SPL, but now I see BB
> > being enabled depends on tegra mmc.
> > 
> > btw. shouldn't --gc-sections remove BB code if it's not used at all?
> 
> Yes, I assume so. In v2 of the patch series I have simply enabled
> CONFIG_BOUNCE_BUFFER unconditionally on Tegra, which removes the need
> for any SPL-specific changes.
> 
> Before enabling it:
> 
> Configuring for trimslice board...
>    text	   data	    bss	    dec	    hex	filename
>  245625	   9304	 274968	 529897	  815e9	./u-boot
>   14451	    208	     72	  14731	   398b	./spl/u-boot-spl
> 
> After enabling it:
> 
> Configuring for trimslice board...
>    text	   data	    bss	    dec	    hex	filename
>  245742	   9304	 274964	 530010	  8165a	./u-boot
>   14451	    208	     72	  14731	   398b	./spl/u-boot-spl
> 
> SPL didn't change since, so this seems to be working fine.

Good!
diff mbox

Patch

diff --git a/common/bouncebuf.c b/common/bouncebuf.c
index 4f827f8..ffd3c90 100644
--- a/common/bouncebuf.c
+++ b/common/bouncebuf.c
@@ -27,6 +27,7 @@ 
 #include <errno.h>
 #include <bouncebuf.h>
 
+#ifdef CONFIG_BOUNCE_BUFFER
 static int addr_aligned(void *data, size_t len)
 {
 	const ulong align_mask = ARCH_DMA_MINALIGN - 1;
@@ -90,3 +91,4 @@  int bounce_buffer_stop(void **data, size_t len, void **backup, uint8_t flags)
 
 	return 0;
 }
+#endif