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

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

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

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

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

[U-Boot] [PATCH 2/3] common: rework bouncebuf implementation

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

The current bouncebuf API requires all parameters to be passed to both
bounce_buffer_start() and bounce_buffer_stop(). This works fine when
both functions are called from the same place. However, Tegra's MMC
driver splits the data setup and post-processing steps between two
functions, and passing all that state around separately would be painful.
Modify the bouncebuf API to accept a state structure as a parameter, so
that only a single struct needs to be passed to both APIs.

Also, don't modify the data pointer, but rather store the temporary
buffer in this state struct. This also avoids passing the buffer pointer
all over the place. The bouncebuf code ensures that client code can
always use a single buffer pointer in the state structure, irrespective
of whether a bounce buffer actually had to be allocated.

Finally, store the aligned/rounded length in the state structure too, so
that client code doesn't need to duplicate the size alignment/rounding
code, and hence have to guess at the value it was aligned/rounded to.

Update the MXS MMC driver for this change.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 common/bouncebuf.c   |   41 ++++++++++++++++-------------------------
 drivers/mmc/mxsmmc.c |   25 ++++++++++++++-----------
 include/bouncebuf.h  |   36 +++++++++++++++++++++++-------------
 3 files changed, 53 insertions(+), 49 deletions(-)

diff --git a/common/bouncebuf.c b/common/bouncebuf.c
index ffd3c90..210d70d 100644
--- a/common/bouncebuf.c
+++ b/common/bouncebuf.c
@@ -50,44 +50,35 @@ static int addr_aligned(void *data, size_t len)
 	return 1;
 }
 
-int bounce_buffer_start(void **data, size_t len, void **backup, uint8_t flags)
+int bounce_buffer_start(struct bounce_buffer_state *state, void *data,
+			size_t len, uint8_t flags)
 {
-	void *tmp;
-	size_t alen;
+	state->user_buffer = data;
+	state->bounce_buffer = data;
+	state->len = len;
+	state->len_aligned = roundup(len, ARCH_DMA_MINALIGN);
+	state->flags = flags;
 
-	if (addr_aligned(*data, len)) {
-		*backup = NULL;
+	if (addr_aligned(data, len))
 		return 0;
-	}
-
-	alen = roundup(len, ARCH_DMA_MINALIGN);
-	tmp = memalign(ARCH_DMA_MINALIGN, alen);
 
-	if (!tmp)
+	state->bounce_buffer = memalign(ARCH_DMA_MINALIGN, state->len_aligned);
+	if (!state->bounce_buffer)
 		return -ENOMEM;
 
-	if (flags & GEN_BB_READ)
-		memcpy(tmp, *data, len);
-
-	*backup = *data;
-	*data = tmp;
+	if (state->flags & GEN_BB_READ)
+		memcpy(state->bounce_buffer, state->user_buffer, state->len);
 
 	return 0;
 }
 
-int bounce_buffer_stop(void **data, size_t len, void **backup, uint8_t flags)
+int bounce_buffer_stop(struct bounce_buffer_state *state)
 {
-	void *tmp = *data;
-
-	/* The buffer was already aligned, since "backup" is NULL. */
-	if (!*backup)
+	if (state->bounce_buffer == state->user_buffer)
 		return 0;
 
-	if (flags & GEN_BB_WRITE)
-		memcpy(*backup, *data, len);
-
-	*data = *backup;
-	free(tmp);
+	if (state->flags & GEN_BB_WRITE)
+		memcpy(state->user_buffer, state->bounce_buffer, state->len);
 
 	return 0;
 }
diff --git a/drivers/mmc/mxsmmc.c b/drivers/mmc/mxsmmc.c
index 109acbf..d314d7d 100644
--- a/drivers/mmc/mxsmmc.c
+++ b/drivers/mmc/mxsmmc.c
@@ -96,11 +96,11 @@ static int mxsmmc_send_cmd_pio(struct mxsmmc_priv *priv, struct mmc_data *data)
 static int mxsmmc_send_cmd_dma(struct mxsmmc_priv *priv, struct mmc_data *data)
 {
 	uint32_t data_count = data->blocksize * data->blocks;
-	uint32_t cache_data_count = roundup(data_count, ARCH_DMA_MINALIGN);
 	int dmach;
 	struct mxs_dma_desc *desc = priv->desc;
-	void *addr, *backup;
+	void *addr;
 	uint8_t flags;
+	struct bounce_buffer_state bbstate;
 
 	memset(desc, 0, sizeof(struct mxs_dma_desc));
 	desc->address = (dma_addr_t)desc;
@@ -115,19 +115,21 @@ static int mxsmmc_send_cmd_dma(struct mxsmmc_priv *priv, struct mmc_data *data)
 		flags = GEN_BB_READ;
 	}
 
-	bounce_buffer_start(&addr, data_count, &backup, flags);
+	bounce_buffer_start(&bbstate, addr, data_count, flags);
 
-	priv->desc->cmd.address = (dma_addr_t)addr;
+	priv->desc->cmd.address = (dma_addr_t)bbstate.bounce_buffer;
 
 	if (data->flags & MMC_DATA_WRITE) {
 		/* Flush data to DRAM so DMA can pick them up */
-		flush_dcache_range((uint32_t)addr,
-			(uint32_t)(addr) + cache_data_count);
+		flush_dcache_range((uint32_t)bbstate.bounce_buffer,
+					(uint32_t)(bbstate.bounce_buffer) +
+						bbstate.len_aligned);
 	}
 
 	/* Invalidate the area, so no writeback into the RAM races with DMA */
 	invalidate_dcache_range((uint32_t)priv->desc->cmd.address,
-			(uint32_t)(priv->desc->cmd.address + cache_data_count));
+				(uint32_t)(priv->desc->cmd.address +
+					bbstate.len_aligned));
 
 	priv->desc->cmd.data |= MXS_DMA_DESC_IRQ | MXS_DMA_DESC_DEC_SEM |
 				(data_count << MXS_DMA_DESC_BYTES_OFFSET);
@@ -135,17 +137,18 @@ static int mxsmmc_send_cmd_dma(struct mxsmmc_priv *priv, struct mmc_data *data)
 	dmach = MXS_DMA_CHANNEL_AHB_APBH_SSP0 + priv->id;
 	mxs_dma_desc_append(dmach, priv->desc);
 	if (mxs_dma_go(dmach)) {
-		bounce_buffer_stop(&addr, data_count, &backup, flags);
+		bounce_buffer_stop(&bbstate);
 		return COMM_ERR;
 	}
 
 	/* The data arrived into DRAM, invalidate cache over them */
 	if (data->flags & MMC_DATA_READ) {
-		invalidate_dcache_range((uint32_t)addr,
-			(uint32_t)(addr) + cache_data_count);
+		invalidate_dcache_range((uint32_t)bbstate.bounce_buffer,
+					(uint32_t)(bbstate.bounce_buffer) +
+						bbstate.len_aligned);
 	}
 
-	bounce_buffer_stop(&addr, data_count, &backup, flags);
+	bounce_buffer_stop(&bbstate);
 
 	return 0;
 }
diff --git a/include/bouncebuf.h b/include/bouncebuf.h
index 31021c5..205a1ed 100644
--- a/include/bouncebuf.h
+++ b/include/bouncebuf.h
@@ -52,33 +52,43 @@
 #define GEN_BB_RW	(GEN_BB_READ | GEN_BB_WRITE)
 
 #ifdef CONFIG_BOUNCE_BUFFER
+struct bounce_buffer_state {
+	void *user_buffer;
+	void *bounce_buffer;
+	size_t len;
+	size_t len_aligned;
+	uint8_t flags;
+};
+
 /**
  * bounce_buffer_start() -- Start the bounce buffer session
+ * state:	stores state passed between bounce_buffer_{start,stop}
  * data:	pointer to buffer to be aligned
  * len:		length of the buffer
- * backup:	pointer to backup buffer (the original value is stored here if
- *              needed
  * flags:	flags describing the transaction, see above.
  */
-int bounce_buffer_start(void **data, size_t len, void **backup, uint8_t flags);
+int bounce_buffer_start(struct bounce_buffer_state *state, void *data,
+			size_t len, uint8_t flags);
 /**
  * bounce_buffer_stop() -- Finish the bounce buffer session
- * data:	pointer to buffer that was aligned
- * len:		length of the buffer
- * backup:	pointer to backup buffer (the original value is stored here if
- *              needed
- * flags:	flags describing the transaction, see above.
+ * state:	stores state passed between bounce_buffer_{start,stop}
  */
-int bounce_buffer_stop(void **data, size_t len, void **backup, uint8_t flags);
+int bounce_buffer_stop(struct bounce_buffer_state *state);
 #else
-static inline int bounce_buffer_start(void **data, size_t len, void **backup,
-					uint8_t flags)
+struct bounce_buffer_state {
+	void *bounce_buffer;
+	size_t len_aligned;
+};
+
+static inline int bounce_buffer_start(struct bounce_buffer_state *state,
+					void *data, size_t len, uint8_t flags)
 {
+	state->bounce_buffer = data;
+	state->len_aligned = len;
 	return 0;
 }
 
-static inline int bounce_buffer_stop(void **data, size_t len, void **backup,
-					uint8_t flags)
+static inline int bounce_buffer_stop(struct bounce_buffer_state *state)
 {
 	return 0;
 }
-- 
1.7.0.4

[U-Boot] [PATCH 2/3] common: rework bouncebuf implementation

[Simon Glass]

Hi Stephen,

On Mon, Nov 5, 2012 at 3:04 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> From: Stephen Warren <swarren@nvidia.com>
>
> The current bouncebuf API requires all parameters to be passed to both
> bounce_buffer_start() and bounce_buffer_stop(). This works fine when
> both functions are called from the same place. However, Tegra's MMC
> driver splits the data setup and post-processing steps between two
> functions, and passing all that state around separately would be painful.
> Modify the bouncebuf API to accept a state structure as a parameter, so
> that only a single struct needs to be passed to both APIs.
>
> Also, don't modify the data pointer, but rather store the temporary
> buffer in this state struct. This also avoids passing the buffer pointer
> all over the place. The bouncebuf code ensures that client code can
> always use a single buffer pointer in the state structure, irrespective
> of whether a bounce buffer actually had to be allocated.
>
> Finally, store the aligned/rounded length in the state structure too, so
> that client code doesn't need to duplicate the size alignment/rounding
> code, and hence have to guess at the value it was aligned/rounded to.
>
> Update the MXS MMC driver for this change.

I missed this API when it came through. I think your changes improve it a lot.

>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  common/bouncebuf.c   |   41 ++++++++++++++++-------------------------
>  drivers/mmc/mxsmmc.c |   25 ++++++++++++++-----------
>  include/bouncebuf.h  |   36 +++++++++++++++++++++++-------------
>  3 files changed, 53 insertions(+), 49 deletions(-)
>
> diff --git a/common/bouncebuf.c b/common/bouncebuf.c
> index ffd3c90..210d70d 100644
> --- a/common/bouncebuf.c
> +++ b/common/bouncebuf.c
> @@ -50,44 +50,35 @@ static int addr_aligned(void *data, size_t len)
>         return 1;
>  }
>
> -int bounce_buffer_start(void **data, size_t len, void **backup, uint8_t flags)
> +int bounce_buffer_start(struct bounce_buffer_state *state, void *data,
> +                       size_t len, uint8_t flags)
>  {
> -       void *tmp;
> -       size_t alen;
> +       state->user_buffer = data;
> +       state->bounce_buffer = data;
> +       state->len = len;
> +       state->len_aligned = roundup(len, ARCH_DMA_MINALIGN);
> +       state->flags = flags;
>
> -       if (addr_aligned(*data, len)) {
> -               *backup = NULL;
> +       if (addr_aligned(data, len))

Maybe consider checking for data == NULL here, and return 0. This
would allow you to remove your 'if (data)' checks in the tegra driver.
Would need to update function description in the header file though.

>                 return 0;
> -       }
> -
> -       alen = roundup(len, ARCH_DMA_MINALIGN);
> -       tmp = memalign(ARCH_DMA_MINALIGN, alen);
>
> -       if (!tmp)
> +       state->bounce_buffer = memalign(ARCH_DMA_MINALIGN, state->len_aligned);
> +       if (!state->bounce_buffer)
>                 return -ENOMEM;
>
> -       if (flags & GEN_BB_READ)
> -               memcpy(tmp, *data, len);
> -
> -       *backup = *data;
> -       *data = tmp;
> +       if (state->flags & GEN_BB_READ)
> +               memcpy(state->bounce_buffer, state->user_buffer, state->len);

I wonder if the dcache handling could be done here (and in the
memcpy() below), perhaps under the control of a new flag. After the
cache code in both drivers seems very similar.

>
>         return 0;
>  }
>
> -int bounce_buffer_stop(void **data, size_t len, void **backup, uint8_t flags)
> +int bounce_buffer_stop(struct bounce_buffer_state *state)
>  {
> -       void *tmp = *data;
> -
> -       /* The buffer was already aligned, since "backup" is NULL. */
> -       if (!*backup)
> +       if (state->bounce_buffer == state->user_buffer)
>                 return 0;
>
> -       if (flags & GEN_BB_WRITE)
> -               memcpy(*backup, *data, len);
> -
> -       *data = *backup;
> -       free(tmp);

Don't you need to keep the free()?

> +       if (state->flags & GEN_BB_WRITE)
> +               memcpy(state->user_buffer, state->bounce_buffer, state->len);
>
>         return 0;
>  }
> diff --git a/drivers/mmc/mxsmmc.c b/drivers/mmc/mxsmmc.c
> index 109acbf..d314d7d 100644
> --- a/drivers/mmc/mxsmmc.c
> +++ b/drivers/mmc/mxsmmc.c
> @@ -96,11 +96,11 @@ static int mxsmmc_send_cmd_pio(struct mxsmmc_priv *priv, struct mmc_data *data)
>  static int mxsmmc_send_cmd_dma(struct mxsmmc_priv *priv, struct mmc_data *data)
>  {
>         uint32_t data_count = data->blocksize * data->blocks;
> -       uint32_t cache_data_count = roundup(data_count, ARCH_DMA_MINALIGN);
>         int dmach;
>         struct mxs_dma_desc *desc = priv->desc;
> -       void *addr, *backup;
> +       void *addr;
>         uint8_t flags;
> +       struct bounce_buffer_state bbstate;
>
>         memset(desc, 0, sizeof(struct mxs_dma_desc));
>         desc->address = (dma_addr_t)desc;
> @@ -115,19 +115,21 @@ static int mxsmmc_send_cmd_dma(struct mxsmmc_priv *priv, struct mmc_data *data)
>                 flags = GEN_BB_READ;
>         }
>
> -       bounce_buffer_start(&addr, data_count, &backup, flags);
> +       bounce_buffer_start(&bbstate, addr, data_count, flags);
>
> -       priv->desc->cmd.address = (dma_addr_t)addr;
> +       priv->desc->cmd.address = (dma_addr_t)bbstate.bounce_buffer;
>
>         if (data->flags & MMC_DATA_WRITE) {
>                 /* Flush data to DRAM so DMA can pick them up */
> -               flush_dcache_range((uint32_t)addr,
> -                       (uint32_t)(addr) + cache_data_count);
> +               flush_dcache_range((uint32_t)bbstate.bounce_buffer,
> +                                       (uint32_t)(bbstate.bounce_buffer) +
> +                                               bbstate.len_aligned);
>         }
>
>         /* Invalidate the area, so no writeback into the RAM races with DMA */
>         invalidate_dcache_range((uint32_t)priv->desc->cmd.address,
> -                       (uint32_t)(priv->desc->cmd.address + cache_data_count));
> +                               (uint32_t)(priv->desc->cmd.address +
> +                                       bbstate.len_aligned));
>
>         priv->desc->cmd.data |= MXS_DMA_DESC_IRQ | MXS_DMA_DESC_DEC_SEM |
>                                 (data_count << MXS_DMA_DESC_BYTES_OFFSET);
> @@ -135,17 +137,18 @@ static int mxsmmc_send_cmd_dma(struct mxsmmc_priv *priv, struct mmc_data *data)
>         dmach = MXS_DMA_CHANNEL_AHB_APBH_SSP0 + priv->id;
>         mxs_dma_desc_append(dmach, priv->desc);
>         if (mxs_dma_go(dmach)) {
> -               bounce_buffer_stop(&addr, data_count, &backup, flags);
> +               bounce_buffer_stop(&bbstate);
>                 return COMM_ERR;
>         }
>
>         /* The data arrived into DRAM, invalidate cache over them */
>         if (data->flags & MMC_DATA_READ) {
> -               invalidate_dcache_range((uint32_t)addr,
> -                       (uint32_t)(addr) + cache_data_count);
> +               invalidate_dcache_range((uint32_t)bbstate.bounce_buffer,
> +                                       (uint32_t)(bbstate.bounce_buffer) +
> +                                               bbstate.len_aligned);
>         }
>
> -       bounce_buffer_stop(&addr, data_count, &backup, flags);
> +       bounce_buffer_stop(&bbstate);
>
>         return 0;
>  }
> diff --git a/include/bouncebuf.h b/include/bouncebuf.h
> index 31021c5..205a1ed 100644
> --- a/include/bouncebuf.h
> +++ b/include/bouncebuf.h
> @@ -52,33 +52,43 @@
>  #define GEN_BB_RW      (GEN_BB_READ | GEN_BB_WRITE)
>
>  #ifdef CONFIG_BOUNCE_BUFFER
> +struct bounce_buffer_state {
> +       void *user_buffer;
> +       void *bounce_buffer;
> +       size_t len;
> +       size_t len_aligned;
> +       uint8_t flags;

Would struct bounce_buffer be better?

Perhaps add member comments?

> +};
> +
>  /**
>   * bounce_buffer_start() -- Start the bounce buffer session
> + * state:      stores state passed between bounce_buffer_{start,stop}
>   * data:       pointer to buffer to be aligned
>   * len:                length of the buffer
> - * backup:     pointer to backup buffer (the original value is stored here if
> - *              needed
>   * flags:      flags describing the transaction, see above.
>   */
> -int bounce_buffer_start(void **data, size_t len, void **backup, uint8_t flags);
> +int bounce_buffer_start(struct bounce_buffer_state *state, void *data,
> +                       size_t len, uint8_t flags);

I would argue that a uint8_t parameter doesn't make a lot of sense.
Why not just unsigned int?

>  /**
>   * bounce_buffer_stop() -- Finish the bounce buffer session
> - * data:       pointer to buffer that was aligned
> - * len:                length of the buffer
> - * backup:     pointer to backup buffer (the original value is stored here if
> - *              needed
> - * flags:      flags describing the transaction, see above.
> + * state:      stores state passed between bounce_buffer_{start,stop}
>   */
> -int bounce_buffer_stop(void **data, size_t len, void **backup, uint8_t flags);
> +int bounce_buffer_stop(struct bounce_buffer_state *state);
>  #else
> -static inline int bounce_buffer_start(void **data, size_t len, void **backup,
> -                                       uint8_t flags)
> +struct bounce_buffer_state {
> +       void *bounce_buffer;
> +       size_t len_aligned;
> +};
> +
> +static inline int bounce_buffer_start(struct bounce_buffer_state *state,
> +                                       void *data, size_t len, uint8_t flags)
>  {
> +       state->bounce_buffer = data;
> +       state->len_aligned = len;

Why do you need to do this if it is just a null implementation?
Perhaps add a comment.

>         return 0;
>  }
>
> -static inline int bounce_buffer_stop(void **data, size_t len, void **backup,
> -                                       uint8_t flags)
> +static inline int bounce_buffer_stop(struct bounce_buffer_state *state)
>  {
>         return 0;
>  }
> --
> 1.7.0.4
>

Regards,
Simon

[U-Boot] [PATCH 2/3] common: rework bouncebuf implementation

[Stephen Warren]

On 11/05/2012 04:54 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Mon, Nov 5, 2012 at 3:04 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> The current bouncebuf API requires all parameters to be passed to both
>> bounce_buffer_start() and bounce_buffer_stop(). This works fine when
>> both functions are called from the same place. However, Tegra's MMC
>> driver splits the data setup and post-processing steps between two
>> functions, and passing all that state around separately would be painful.
>> Modify the bouncebuf API to accept a state structure as a parameter, so
>> that only a single struct needs to be passed to both APIs.
...
> I wonder if the dcache handling could be done here (and in the
> memcpy() below), perhaps under the control of a new flag. After the
> cache code in both drivers seems very similar.

Yes, that's a good idea. It cross my mind before, but I forgot to follow
up on it and realize that the cache management APIs are actually common,
not CPU-specific.

One question here. The MXS MMC driver contains:

> 	if (data->flags & MMC_DATA_WRITE) {
> 		/* Flush data to DRAM so DMA can pick them up */
> 		flush_dcache_range((uint32_t)bbstate.bounce_buffer,
> 					(uint32_t)(bbstate.bounce_buffer) +
> 						bbstate.len_aligned);
> 	}
> 
> 	/* Invalidate the area, so no writeback into the RAM races with DMA */
> 	invalidate_dcache_range((uint32_t)priv->desc->cmd.address,
> 				(uint32_t)(priv->desc->cmd.address +
> 					bbstate.len_aligned));

It the invalidate_dcache_range() really necessary here? I would assume
that the flush_dcache_range() call marks the cache non-dirty, so there
can no longer be any write-backs to race with the DMA. Certainly, the
Tegra MMC driver doesn't include that invalidate call and appears to
work fine.

I agree with all your comments that I haven't quoted here, and will go
implement them.

>> -static inline int bounce_buffer_start(void **data, size_t len, void **backup,
>> -                                       uint8_t flags)
>> +struct bounce_buffer_state {
>> +       void *bounce_buffer;
>> +       size_t len_aligned;
>> +};
>> +
>> +static inline int bounce_buffer_start(struct bounce_buffer_state *state,
>> +                                       void *data, size_t len, uint8_t flags)
>>  {
>> +       state->bounce_buffer = data;
>> +       state->len_aligned = len;
> 
> Why do you need to do this if it is just a null implementation?
> Perhaps add a comment.

I wondered whether we should simply remove the dummy implementations
completely; it seems that if any MMC driver needs bouncebuf ever, it
always needs it. Hence, there should never be a case where these APIs
are called/referenced, yet CONFIG_BOUNCE_BUFFER is not set. However,
there is such a case right now; sc_sps_1.h enables the MXS MMC driver
but not the bounce buffer. Perhaps I should just send a patch to fix
that, and remove these dummy functions.

Aside from that, the reason these dummy functions need to set fields in
*state right now is that the MXS MMC driver unconditionally accesses
those fields, so they need to exist and contain valid data.

[U-Boot] [PATCH 2/3] common: rework bouncebuf implementation

[Stephen Warren]

On 11/05/2012 04:54 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>
>>
>> The current bouncebuf API requires all parameters to be passed to both
>> bounce_buffer_start() and bounce_buffer_stop(). This works fine when
>> both functions are called from the same place. However, Tegra's MMC
>> driver splits the data setup and post-processing steps between two
>> functions, and passing all that state around separately would be painful.
>> Modify the bouncebuf API to accept a state structure as a parameter, so
>> that only a single struct needs to be passed to both APIs.

>> diff --git a/common/bouncebuf.c b/common/bouncebuf.c

>> -int bounce_buffer_start(void **data, size_t len, void **backup, uint8_t flags)
>> +int bounce_buffer_start(struct bounce_buffer_state *state, void *data,
>> +                       size_t len, uint8_t flags)
>>  {
>> -       void *tmp;
>> -       size_t alen;
>> +       state->user_buffer = data;
>> +       state->bounce_buffer = data;
>> +       state->len = len;
>> +       state->len_aligned = roundup(len, ARCH_DMA_MINALIGN);
>> +       state->flags = flags;
>>
>> -       if (addr_aligned(*data, len)) {
>> -               *backup = NULL;
>> +       if (addr_aligned(data, len))
> 
> Maybe consider checking for data == NULL here, and return 0. This
> would allow you to remove your 'if (data)' checks in the tegra driver.
> Would need to update function description in the header file though.

That doesn't actually work out. The if (data) test in the Tegra driver
is checking whether a non-NULL "struct mmc_data *data" is passed to
Tegra's mmc_send_cmd(). That value isn't directly passed to
bounce_buffer_start(), but rather data->{src,dest}, which doesn't even
exist if (!data).

[U-Boot] [PATCH 3/3] mmc: tegra: use bounce buffer APIs

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Tegra's MMC driver does DMA, and hence needs cache-aligned buffers. In
some cases (e.g. user load commands) this cannot be guaranteed by callers
of the MMC APIs. To solve this, modify the Tegra MMC driver to use the
new bounce_buffer_*() APIs.

Note: Ideally, all U-Boot code will always provide address- and size-
aligned buffers, so a bounce buffer will only ever be needed for user-
supplied buffers (e.g. load commands). Ensuring this removes the need
for performance-sucking bounce buffer memory allocations and memcpy()s.
The one known exception at present is the SCR buffer in sd_change_freq(),
which is only 8 bytes long. Solving this requires enhancing struct
mmc_data to know the difference between buffer size and transferred data
size, or forcing all callers of mmc_send_cmd() to have allocated buffers
using ALLOC_CACHE_ALIGN_BUFFER(), which will true in this case, is not
enforced in any way at present, and so cannot be assumed by the core MMC
code.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/mmc/tegra_mmc.c             |   81 +++++++++++++++++++++++------------
 include/configs/tegra-common-post.h |    4 ++
 2 files changed, 58 insertions(+), 27 deletions(-)

diff --git a/drivers/mmc/tegra_mmc.c b/drivers/mmc/tegra_mmc.c
index 1fd5592..e5d55b0 100644
--- a/drivers/mmc/tegra_mmc.c
+++ b/drivers/mmc/tegra_mmc.c
@@ -26,6 +26,7 @@
 #include <asm/arch-tegra/clk_rst.h>
 #include <asm/arch-tegra/tegra_mmc.h>
 #include <mmc.h>
+#include <bouncebuf.h>
 
 /* support 4 mmc hosts */
 struct mmc mmc_dev[4];
@@ -66,14 +67,34 @@ static void tegra_get_setup(struct mmc_host *host, int dev_index)
 	host->reg = (struct tegra_mmc *)host->base;
 }
 
-static void mmc_prepare_data(struct mmc_host *host, struct mmc_data *data)
+static void mmc_prepare_data(struct mmc_host *host, struct mmc_data *data,
+				struct bounce_buffer_state *bbstate)
 {
 	unsigned char ctrl;
+	void *buf;
+	uint8_t bbflags;
+	size_t len;
+
+	if (data->flags & MMC_DATA_READ) {
+		buf = data->dest;
+		bbflags = GEN_BB_WRITE;
+	} else {
+		buf = (void *)data->src;
+		bbflags = GEN_BB_READ;
+	}
+	len = data->blocks * data->blocksize;
+
+	bounce_buffer_start(bbstate, buf, len, bbflags);
+
+	debug("buf: %08X, data->blocks: %u, data->blocksize: %u\n",
+		(u32)bbstate->bounce_buffer, data->blocks, data->blocksize);
 
-	debug("data->dest: %08X, data->blocks: %u, data->blocksize: %u\n",
-	(u32)data->dest, data->blocks, data->blocksize);
+	if (data->flags & MMC_DATA_WRITE)
+		flush_dcache_range((ulong)bbstate->bounce_buffer,
+					(ulong)bbstate->bounce_buffer +
+						bbstate->len_aligned);
 
-	writel((u32)data->dest, &host->reg->sysad);
+	writel((u32)bbstate->bounce_buffer, &host->reg->sysad);
 	/*
 	 * DMASEL[4:3]
 	 * 00 = Selects SDMA
@@ -114,14 +135,6 @@ static void mmc_set_transfer_mode(struct mmc_host *host, struct mmc_data *data)
 	if (data->flags & MMC_DATA_READ)
 		mode |= TEGRA_MMC_TRNMOD_DATA_XFER_DIR_SEL_READ;
 
-	if (data->flags & MMC_DATA_WRITE) {
-		if ((uintptr_t)data->src & (ARCH_DMA_MINALIGN - 1))
-			printf("Warning: unaligned write to %p may fail\n",
-			       data->src);
-		flush_dcache_range((ulong)data->src, (ulong)data->src +
-			data->blocks * data->blocksize);
-	}
-
 	writew(mode, &host->reg->trnmod);
 }
 
@@ -164,6 +177,9 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
 	int result;
 	unsigned int mask = 0;
 	unsigned int retry = 0x100000;
+	struct bounce_buffer_state bbstate;
+	int ret;
+
 	debug(" mmc_send_cmd called\n");
 
 	result = mmc_wait_inhibit(host, cmd, data, 10 /* ms */);
@@ -172,7 +188,7 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
 		return result;
 
 	if (data)
-		mmc_prepare_data(host, data);
+		mmc_prepare_data(host, data, &bbstate);
 
 	debug("cmd->arg: %08x\n", cmd->cmdarg);
 	writel(cmd->cmdarg, &host->reg->argument);
@@ -180,8 +196,10 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
 	if (data)
 		mmc_set_transfer_mode(host, data);
 
-	if ((cmd->resp_type & MMC_RSP_136) && (cmd->resp_type & MMC_RSP_BUSY))
-		return -1;
+	if ((cmd->resp_type & MMC_RSP_136) && (cmd->resp_type & MMC_RSP_BUSY)) {
+		ret = -1;
+		goto cleanup;
+	}
 
 	/*
 	 * CMDREG
@@ -228,19 +246,22 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
 	if (i == retry) {
 		printf("%s: waiting for status update\n", __func__);
 		writel(mask, &host->reg->norintsts);
-		return TIMEOUT;
+		ret = TIMEOUT;
+		goto cleanup;
 	}
 
 	if (mask & TEGRA_MMC_NORINTSTS_CMD_TIMEOUT) {
 		/* Timeout Error */
 		debug("timeout: %08x cmd %d\n", mask, cmd->cmdidx);
 		writel(mask, &host->reg->norintsts);
-		return TIMEOUT;
+		ret = TIMEOUT;
+		goto cleanup;
 	} else if (mask & TEGRA_MMC_NORINTSTS_ERR_INTERRUPT) {
 		/* Error Interrupt */
 		debug("error: %08x cmd %d\n", mask, cmd->cmdidx);
 		writel(mask, &host->reg->norintsts);
-		return -1;
+		ret = -1;
+		goto cleanup;
 	}
 
 	if (cmd->resp_type & MMC_RSP_PRESENT) {
@@ -269,7 +290,8 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
 			if (i == retry) {
 				printf("%s: card is still busy\n", __func__);
 				writel(mask, &host->reg->norintsts);
-				return TIMEOUT;
+				ret = TIMEOUT;
+				goto cleanup;
 			}
 
 			cmd->response[0] = readl(&host->reg->rspreg0);
@@ -291,7 +313,8 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
 				writel(mask, &host->reg->norintsts);
 				printf("%s: error during transfer: 0x%08x\n",
 						__func__, mask);
-				return -1;
+				ret = -1;
+				goto cleanup;
 			} else if (mask & TEGRA_MMC_NORINTSTS_DMA_INTERRUPT) {
 				/*
 				 * DMA Interrupt, restart the transfer where
@@ -318,22 +341,26 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
 				       readl(&host->reg->norintstsen),
 				       readl(&host->reg->norintsigen),
 				       readl(&host->reg->prnsts));
-				return -1;
+				ret = -1;
+				goto cleanup;
 			}
 		}
 		writel(mask, &host->reg->norintsts);
 		if (data->flags & MMC_DATA_READ) {
-			if ((uintptr_t)data->dest & (ARCH_DMA_MINALIGN - 1))
-				printf("Warning: unaligned read from %p "
-					"may fail\n", data->dest);
-			invalidate_dcache_range((ulong)data->dest,
-				(ulong)data->dest +
-					data->blocks * data->blocksize);
+			invalidate_dcache_range((ulong)bbstate.bounce_buffer,
+						(ulong)bbstate.bounce_buffer +
+							bbstate.len_aligned);
 		}
+		bounce_buffer_stop(&bbstate);
 	}
 
 	udelay(1000);
 	return 0;
+
+cleanup:
+	if (data)
+		bounce_buffer_stop(&bbstate);
+	return ret;
 }
 
 static void mmc_change_clock(struct mmc_host *host, uint clock)
diff --git a/include/configs/tegra-common-post.h b/include/configs/tegra-common-post.h
index ab62e71..2d45933 100644
--- a/include/configs/tegra-common-post.h
+++ b/include/configs/tegra-common-post.h
@@ -251,4 +251,8 @@
 
 #endif /* CONFIG_SPL_BUILD */
 
+#ifdef CONFIG_TEGRA_MMC
+#define CONFIG_BOUNCE_BUFFER
+#endif
+
 #endif /* __TEGRA_COMMON_POST_H */
-- 
1.7.0.4

[U-Boot] [PATCH 3/3] mmc: tegra: use bounce buffer APIs

[Simon Glass]

Hi Stephen,

On Mon, Nov 5, 2012 at 3:04 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> From: Stephen Warren <swarren@nvidia.com>
>
> Tegra's MMC driver does DMA, and hence needs cache-aligned buffers. In
> some cases (e.g. user load commands) this cannot be guaranteed by callers
> of the MMC APIs. To solve this, modify the Tegra MMC driver to use the
> new bounce_buffer_*() APIs.
>
> Note: Ideally, all U-Boot code will always provide address- and size-
> aligned buffers, so a bounce buffer will only ever be needed for user-
> supplied buffers (e.g. load commands). Ensuring this removes the need
> for performance-sucking bounce buffer memory allocations and memcpy()s.
> The one known exception at present is the SCR buffer in sd_change_freq(),
> which is only 8 bytes long. Solving this requires enhancing struct
> mmc_data to know the difference between buffer size and transferred data
> size, or forcing all callers of mmc_send_cmd() to have allocated buffers
> using ALLOC_CACHE_ALIGN_BUFFER(), which will true in this case, is not
> enforced in any way at present, and so cannot be assumed by the core MMC
> code.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  drivers/mmc/tegra_mmc.c             |   81 +++++++++++++++++++++++------------
>  include/configs/tegra-common-post.h |    4 ++
>  2 files changed, 58 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/mmc/tegra_mmc.c b/drivers/mmc/tegra_mmc.c
> index 1fd5592..e5d55b0 100644
> --- a/drivers/mmc/tegra_mmc.c
> +++ b/drivers/mmc/tegra_mmc.c
> @@ -26,6 +26,7 @@
>  #include <asm/arch-tegra/clk_rst.h>
>  #include <asm/arch-tegra/tegra_mmc.h>
>  #include <mmc.h>
> +#include <bouncebuf.h>

The order seems wrong here - I think bouncebuf and mmc should go above
the asm/ ones, and bouncebuf should be first.

>
>  /* support 4 mmc hosts */
>  struct mmc mmc_dev[4];
> @@ -66,14 +67,34 @@ static void tegra_get_setup(struct mmc_host *host, int dev_index)
>         host->reg = (struct tegra_mmc *)host->base;
>  }
>
> -static void mmc_prepare_data(struct mmc_host *host, struct mmc_data *data)
> +static void mmc_prepare_data(struct mmc_host *host, struct mmc_data *data,
> +                               struct bounce_buffer_state *bbstate)
>  {
>         unsigned char ctrl;
> +       void *buf;
> +       uint8_t bbflags;
> +       size_t len;
> +
> +       if (data->flags & MMC_DATA_READ) {
> +               buf = data->dest;
> +               bbflags = GEN_BB_WRITE;
> +       } else {
> +               buf = (void *)data->src;
> +               bbflags = GEN_BB_READ;
> +       }
> +       len = data->blocks * data->blocksize;
> +
> +       bounce_buffer_start(bbstate, buf, len, bbflags);
> +
> +       debug("buf: %08X, data->blocks: %u, data->blocksize: %u\n",
> +               (u32)bbstate->bounce_buffer, data->blocks, data->blocksize);
>
> -       debug("data->dest: %08X, data->blocks: %u, data->blocksize: %u\n",
> -       (u32)data->dest, data->blocks, data->blocksize);
> +       if (data->flags & MMC_DATA_WRITE)
> +               flush_dcache_range((ulong)bbstate->bounce_buffer,
> +                                       (ulong)bbstate->bounce_buffer +
> +                                               bbstate->len_aligned);
>
> -       writel((u32)data->dest, &host->reg->sysad);
> +       writel((u32)bbstate->bounce_buffer, &host->reg->sysad);
>         /*
>          * DMASEL[4:3]
>          * 00 = Selects SDMA
> @@ -114,14 +135,6 @@ static void mmc_set_transfer_mode(struct mmc_host *host, struct mmc_data *data)
>         if (data->flags & MMC_DATA_READ)
>                 mode |= TEGRA_MMC_TRNMOD_DATA_XFER_DIR_SEL_READ;
>
> -       if (data->flags & MMC_DATA_WRITE) {
> -               if ((uintptr_t)data->src & (ARCH_DMA_MINALIGN - 1))
> -                       printf("Warning: unaligned write to %p may fail\n",
> -                              data->src);
> -               flush_dcache_range((ulong)data->src, (ulong)data->src +
> -                       data->blocks * data->blocksize);
> -       }
> -
>         writew(mode, &host->reg->trnmod);
>  }
>
> @@ -164,6 +177,9 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
>         int result;
>         unsigned int mask = 0;
>         unsigned int retry = 0x100000;
> +       struct bounce_buffer_state bbstate;
> +       int ret;
> +
>         debug(" mmc_send_cmd called\n");
>
>         result = mmc_wait_inhibit(host, cmd, data, 10 /* ms */);
> @@ -172,7 +188,7 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
>                 return result;
>
>         if (data)
> -               mmc_prepare_data(host, data);
> +               mmc_prepare_data(host, data, &bbstate);
>
>         debug("cmd->arg: %08x\n", cmd->cmdarg);
>         writel(cmd->cmdarg, &host->reg->argument);
> @@ -180,8 +196,10 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
>         if (data)
>                 mmc_set_transfer_mode(host, data);
>
> -       if ((cmd->resp_type & MMC_RSP_136) && (cmd->resp_type & MMC_RSP_BUSY))
> -               return -1;
> +       if ((cmd->resp_type & MMC_RSP_136) && (cmd->resp_type & MMC_RSP_BUSY)) {
> +               ret = -1;
> +               goto cleanup;
> +       }

You might consider putting this body in a function so you don't need
these two lines everywhere below.

>
>         /*
>          * CMDREG
> @@ -228,19 +246,22 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
>         if (i == retry) {
>                 printf("%s: waiting for status update\n", __func__);
>                 writel(mask, &host->reg->norintsts);
> -               return TIMEOUT;
> +               ret = TIMEOUT;
> +               goto cleanup;
>         }
>
>         if (mask & TEGRA_MMC_NORINTSTS_CMD_TIMEOUT) {
>                 /* Timeout Error */
>                 debug("timeout: %08x cmd %d\n", mask, cmd->cmdidx);
>                 writel(mask, &host->reg->norintsts);
> -               return TIMEOUT;
> +               ret = TIMEOUT;
> +               goto cleanup;
>         } else if (mask & TEGRA_MMC_NORINTSTS_ERR_INTERRUPT) {
>                 /* Error Interrupt */
>                 debug("error: %08x cmd %d\n", mask, cmd->cmdidx);
>                 writel(mask, &host->reg->norintsts);
> -               return -1;
> +               ret = -1;
> +               goto cleanup;
>         }
>
>         if (cmd->resp_type & MMC_RSP_PRESENT) {
> @@ -269,7 +290,8 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
>                         if (i == retry) {
>                                 printf("%s: card is still busy\n", __func__);
>                                 writel(mask, &host->reg->norintsts);
> -                               return TIMEOUT;
> +                               ret = TIMEOUT;
> +                               goto cleanup;
>                         }
>
>                         cmd->response[0] = readl(&host->reg->rspreg0);
> @@ -291,7 +313,8 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
>                                 writel(mask, &host->reg->norintsts);
>                                 printf("%s: error during transfer: 0x%08x\n",
>                                                 __func__, mask);
> -                               return -1;
> +                               ret = -1;
> +                               goto cleanup;
>                         } else if (mask & TEGRA_MMC_NORINTSTS_DMA_INTERRUPT) {
>                                 /*
>                                  * DMA Interrupt, restart the transfer where
> @@ -318,22 +341,26 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
>                                        readl(&host->reg->norintstsen),
>                                        readl(&host->reg->norintsigen),
>                                        readl(&host->reg->prnsts));
> -                               return -1;
> +                               ret = -1;
> +                               goto cleanup;
>                         }
>                 }
>                 writel(mask, &host->reg->norintsts);
>                 if (data->flags & MMC_DATA_READ) {
> -                       if ((uintptr_t)data->dest & (ARCH_DMA_MINALIGN - 1))
> -                               printf("Warning: unaligned read from %p "
> -                                       "may fail\n", data->dest);
> -                       invalidate_dcache_range((ulong)data->dest,
> -                               (ulong)data->dest +
> -                                       data->blocks * data->blocksize);
> +                       invalidate_dcache_range((ulong)bbstate.bounce_buffer,
> +                                               (ulong)bbstate.bounce_buffer +
> +                                                       bbstate.len_aligned);
>                 }
> +               bounce_buffer_stop(&bbstate);
>         }
>
>         udelay(1000);
>         return 0;
> +
> +cleanup:
> +       if (data)
> +               bounce_buffer_stop(&bbstate);
> +       return ret;
>  }
>
>  static void mmc_change_clock(struct mmc_host *host, uint clock)
> diff --git a/include/configs/tegra-common-post.h b/include/configs/tegra-common-post.h
> index ab62e71..2d45933 100644
> --- a/include/configs/tegra-common-post.h
> +++ b/include/configs/tegra-common-post.h
> @@ -251,4 +251,8 @@
>
>  #endif /* CONFIG_SPL_BUILD */
>
> +#ifdef CONFIG_TEGRA_MMC
> +#define CONFIG_BOUNCE_BUFFER
> +#endif

Is there really any harm in just defining this always (say in the
tegra20-common.h)? The functions should be dropped if not used.

> +
>  #endif /* __TEGRA_COMMON_POST_H */
> --
> 1.7.0.4
>

Regards,
Simon

[U-Boot] [PATCH 3/3] mmc: tegra: use bounce buffer APIs

[Stephen Warren]

On 11/05/2012 05:00 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>
>>
>> Tegra's MMC driver does DMA, and hence needs cache-aligned buffers. In
>> some cases (e.g. user load commands) this cannot be guaranteed by callers
>> of the MMC APIs. To solve this, modify the Tegra MMC driver to use the
>> new bounce_buffer_*() APIs.

>> diff --git a/drivers/mmc/tegra_mmc.c b/drivers/mmc/tegra_mmc.c

>>  #include <asm/arch-tegra/clk_rst.h>
>>  #include <asm/arch-tegra/tegra_mmc.h>
>>  #include <mmc.h>
>> +#include <bouncebuf.h>
> 
> The order seems wrong here - I think bouncebuf and mmc should go above
> the asm/ ones, and bouncebuf should be first.

Is there a defined order for header files? I suppose I should try and
read and remember more documentation!

>> @@ -180,8 +196,10 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
>>         if (data)
>>                 mmc_set_transfer_mode(host, data);
>>
>> -       if ((cmd->resp_type & MMC_RSP_136) && (cmd->resp_type & MMC_RSP_BUSY))
>> -               return -1;
>> +       if ((cmd->resp_type & MMC_RSP_136) && (cmd->resp_type & MMC_RSP_BUSY)) {
>> +               ret = -1;
>> +               goto cleanup;
>> +       }
> 
> You might consider putting this body in a function so you don't need
> these two lines everywhere below.

I'm not quite sure how a function would work here; a function can't
really goto. Do you mean a macro? I'd tend to this a macro would
obfuscate the pretty simple code.

Oh, perhaps you mean having a new top-level function that does:

bounce_buffer_start();
calls a function to do all the work
bounce_buffer_stop();

That would certainly simplify the patch.

>> diff --git a/include/configs/tegra-common-post.h b/include/configs/tegra-common-post.h

>> +#ifdef CONFIG_TEGRA_MMC
>> +#define CONFIG_BOUNCE_BUFFER
>> +#endif
> 
> Is there really any harm in just defining this always (say in the
> tegra20-common.h)? The functions should be dropped if not used.

I suppose it'd be fine to always enable this since the linker should
drop the functions when not referenced. Of course, that relies on
bouncebuf.o not having any global side-effects (e.g. registering things
via custom linker segments that are always pulled in). The code above
represents the actual dependency too; hopefully one day U-Boot will
sprout Kconfig, and that logic can be replaced by:

config TEGRA_MMC
    select BOUNCE_BUFFER

[U-Boot] [PATCH 3/3] mmc: tegra: use bounce buffer APIs

[Simon Glass]

Hi Stephen,

On Tue, Nov 6, 2012 at 10:50 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 11/05/2012 05:00 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>
>>>
>>> Tegra's MMC driver does DMA, and hence needs cache-aligned buffers. In
>>> some cases (e.g. user load commands) this cannot be guaranteed by callers
>>> of the MMC APIs. To solve this, modify the Tegra MMC driver to use the
>>> new bounce_buffer_*() APIs.
>
>>> diff --git a/drivers/mmc/tegra_mmc.c b/drivers/mmc/tegra_mmc.c
>
>>>  #include <asm/arch-tegra/clk_rst.h>
>>>  #include <asm/arch-tegra/tegra_mmc.h>
>>>  #include <mmc.h>
>>> +#include <bouncebuf.h>
>>
>> The order seems wrong here - I think bouncebuf and mmc should go above
>> the asm/ ones, and bouncebuf should be first.
>
> Is there a defined order for header files? I suppose I should try and
> read and remember more documentation!

According to Mike, "the order should generally be:
        - stuff in include/
        - stuff in sys/
        - stuff in linux/
        - stuff in asm/
each sub-region can be sorted, but i don't think we should go against the
subdir rule"

>
>>> @@ -180,8 +196,10 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
>>>         if (data)
>>>                 mmc_set_transfer_mode(host, data);
>>>
>>> -       if ((cmd->resp_type & MMC_RSP_136) && (cmd->resp_type & MMC_RSP_BUSY))
>>> -               return -1;
>>> +       if ((cmd->resp_type & MMC_RSP_136) && (cmd->resp_type & MMC_RSP_BUSY)) {
>>> +               ret = -1;
>>> +               goto cleanup;
>>> +       }
>>
>> You might consider putting this body in a function so you don't need
>> these two lines everywhere below.
>
> I'm not quite sure how a function would work here; a function can't
> really goto. Do you mean a macro? I'd tend to this a macro would
> obfuscate the pretty simple code.
>
> Oh, perhaps you mean having a new top-level function that does:
>
> bounce_buffer_start();
> calls a function to do all the work
> bounce_buffer_stop();
>
> That would certainly simplify the patch.

Yes that's what I meant.

>
>>> diff --git a/include/configs/tegra-common-post.h b/include/configs/tegra-common-post.h
>
>>> +#ifdef CONFIG_TEGRA_MMC
>>> +#define CONFIG_BOUNCE_BUFFER
>>> +#endif
>>
>> Is there really any harm in just defining this always (say in the
>> tegra20-common.h)? The functions should be dropped if not used.
>
> I suppose it'd be fine to always enable this since the linker should
> drop the functions when not referenced. Of course, that relies on
> bouncebuf.o not having any global side-effects (e.g. registering things
> via custom linker segments that are always pulled in). The code above
> represents the actual dependency too; hopefully one day U-Boot will
> sprout Kconfig, and that logic can be replaced by:
>
> config TEGRA_MMC
>     select BOUNCE_BUFFER

Yes, maybe it isn't that simple in general, but it seems that it might
be safe with Tegra at least.

Regards,
Simon

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

[Simon Glass]

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

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

[Stephen Warren]

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.

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

[Marek Vasut]

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

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

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

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

[Marek Vasut]

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

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

[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

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

[Marek Vasut]

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

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

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

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

[Marek Vasut]

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,

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

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

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

[Marek Vasut]

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!