[U-Boot,v2] mmc: don't support write & erase for SPL builds

For SPL builds this is just dead code since we'll only need to read.
Eliminating it results in a significant size reduction for the SPL
binary, which may be critical for certain platforms where the binary
size is highly constrained.

Signed-off-by: Paul Burton <paul.burton@imgtec.com>
---
Changes in v2:
  - Move the mmc_bwrite & mmc_berase functions to a new mmc_write.c
    file which is only compiled for non-SPL builds, as per a request
    from Pantelis Antoniou. This requires that a few formerly static
    functions in mmc.c be accessible to the new file, so they are
    declared in a new mmc_private.h header along with the write &
    erase functions. For what it's worth I prefered v1, but hey ho.
---
 drivers/mmc/Makefile      |   2 +
 drivers/mmc/mmc.c         | 186 +--------------------------------------------
 drivers/mmc/mmc_private.h |  45 +++++++++++
 drivers/mmc/mmc_write.c   | 189 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 240 insertions(+), 182 deletions(-)
 create mode 100644 drivers/mmc/mmc_private.h
 create mode 100644 drivers/mmc/mmc_write.c

Hi Paul,

On Sep 6, 2013, at 4:43 PM, Paul Burton wrote:

> For SPL builds this is just dead code since we'll only need to read.
> Eliminating it results in a significant size reduction for the SPL
> binary, which may be critical for certain platforms where the binary
> size is highly constrained.
> 
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> ---
> Changes in v2:
>  - Move the mmc_bwrite & mmc_berase functions to a new mmc_write.c
>    file which is only compiled for non-SPL builds, as per a request
>    from Pantelis Antoniou. This requires that a few formerly static
>    functions in mmc.c be accessible to the new file, so they are
>    declared in a new mmc_private.h header along with the write &
>    erase functions. For what it's worth I prefered v1, but hey ho.
> ---
> drivers/mmc/Makefile      |   2 +
> drivers/mmc/mmc.c         | 186 +--------------------------------------------
> drivers/mmc/mmc_private.h |  45 +++++++++++
> drivers/mmc/mmc_write.c   | 189 ++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 240 insertions(+), 182 deletions(-)
> create mode 100644 drivers/mmc/mmc_private.h
> create mode 100644 drivers/mmc/mmc_write.c
> 
> diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile
> index bedf833..06280d1 100644
> --- a/drivers/mmc/Makefile
> +++ b/drivers/mmc/Makefile
> @@ -34,6 +34,8 @@ COBJS-$(CONFIG_EXYNOS_DWMMC) += exynos_dw_mmc.o
> COBJS-$(CONFIG_ZYNQ_SDHCI) += zynq_sdhci.o
> ifdef CONFIG_SPL_BUILD
> COBJS-$(CONFIG_SPL_MMC_BOOT) += fsl_esdhc_spl.o
> +else
> +COBJS-$(CONFIG_GENERIC_MMC) += mmc_write.o
> endif
> 
> COBJS	:= $(COBJS-y)
> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> index 30a985b..666f77b 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -15,6 +15,7 @@
> #include <malloc.h>
> #include <linux/list.h>
> #include <div64.h>
> +#include "mmc_private.h"
> 
> /* Set block count limit because of 16 bit register limit on some hardware*/
> #ifndef CONFIG_SYS_MMC_MAX_BLK_COUNT
> @@ -52,8 +53,7 @@ int __board_mmc_getcd(struct mmc *mmc) {
> int board_mmc_getcd(struct mmc *mmc)__attribute__((weak,
> 	alias("__board_mmc_getcd")));
> 
> -static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
> -			struct mmc_data *data)
> +int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
> {
> 	struct mmc_data backup;
> 	int ret;
> @@ -114,7 +114,7 @@ static int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
> 	return ret;
> }
> 
> -static int mmc_send_status(struct mmc *mmc, int timeout)
> +int mmc_send_status(struct mmc *mmc, int timeout)
> {
> 	struct mmc_cmd cmd;
> 	int err, retries = 5;
> @@ -162,7 +162,7 @@ static int mmc_send_status(struct mmc *mmc, int timeout)
> 	return 0;
> }
> 
> -static int mmc_set_blocklen(struct mmc *mmc, int len)
> +int mmc_set_blocklen(struct mmc *mmc, int len)
> {
> 	struct mmc_cmd cmd;
> 
> @@ -192,184 +192,6 @@ struct mmc *find_mmc_device(int dev_num)
> 	return NULL;
> }
> 
> -static ulong mmc_erase_t(struct mmc *mmc, ulong start, lbaint_t blkcnt)
> -{
> -	struct mmc_cmd cmd;
> -	ulong end;
> -	int err, start_cmd, end_cmd;
> -
> -	if (mmc->high_capacity)
> -		end = start + blkcnt - 1;
> -	else {
> -		end = (start + blkcnt - 1) * mmc->write_bl_len;
> -		start *= mmc->write_bl_len;
> -	}
> -
> -	if (IS_SD(mmc)) {
> -		start_cmd = SD_CMD_ERASE_WR_BLK_START;
> -		end_cmd = SD_CMD_ERASE_WR_BLK_END;
> -	} else {
> -		start_cmd = MMC_CMD_ERASE_GROUP_START;
> -		end_cmd = MMC_CMD_ERASE_GROUP_END;
> -	}
> -
> -	cmd.cmdidx = start_cmd;
> -	cmd.cmdarg = start;
> -	cmd.resp_type = MMC_RSP_R1;
> -
> -	err = mmc_send_cmd(mmc, &cmd, NULL);
> -	if (err)
> -		goto err_out;
> -
> -	cmd.cmdidx = end_cmd;
> -	cmd.cmdarg = end;
> -
> -	err = mmc_send_cmd(mmc, &cmd, NULL);
> -	if (err)
> -		goto err_out;
> -
> -	cmd.cmdidx = MMC_CMD_ERASE;
> -	cmd.cmdarg = SECURE_ERASE;
> -	cmd.resp_type = MMC_RSP_R1b;
> -
> -	err = mmc_send_cmd(mmc, &cmd, NULL);
> -	if (err)
> -		goto err_out;
> -
> -	return 0;
> -
> -err_out:
> -#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> -	puts("mmc erase failed\n");
> -#endif
> -	return err;
> -}
> -
> -static unsigned long
> -mmc_berase(int dev_num, lbaint_t start, lbaint_t blkcnt)
> -{
> -	int err = 0;
> -	struct mmc *mmc = find_mmc_device(dev_num);
> -	lbaint_t blk = 0, blk_r = 0;
> -	int timeout = 1000;
> -
> -	if (!mmc)
> -		return -1;
> -
> -#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> -	if ((start % mmc->erase_grp_size) || (blkcnt % mmc->erase_grp_size))
> -		printf("\n\nCaution! Your devices Erase group is 0x%x\n"
> -		       "The erase range would be change to "
> -		       "0x" LBAF "~0x" LBAF "\n\n",
> -		       mmc->erase_grp_size, start & ~(mmc->erase_grp_size - 1),
> -		       ((start + blkcnt + mmc->erase_grp_size)
> -		       & ~(mmc->erase_grp_size - 1)) - 1);
> -#endif
> -
> -	while (blk < blkcnt) {
> -		blk_r = ((blkcnt - blk) > mmc->erase_grp_size) ?
> -			mmc->erase_grp_size : (blkcnt - blk);
> -		err = mmc_erase_t(mmc, start + blk, blk_r);
> -		if (err)
> -			break;
> -
> -		blk += blk_r;
> -
> -		/* Waiting for the ready status */
> -		if (mmc_send_status(mmc, timeout))
> -			return 0;
> -	}
> -
> -	return blk;
> -}
> -
> -static ulong
> -mmc_write_blocks(struct mmc *mmc, lbaint_t start, lbaint_t blkcnt, const void*src)
> -{
> -	struct mmc_cmd cmd;
> -	struct mmc_data data;
> -	int timeout = 1000;
> -
> -	if ((start + blkcnt) > mmc->block_dev.lba) {
> -#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> -		printf("MMC: block number 0x" LBAF " exceeds max(0x" LBAF ")\n",
> -			start + blkcnt, mmc->block_dev.lba);
> -#endif
> -		return 0;
> -	}
> -
> -	if (blkcnt == 0)
> -		return 0;
> -	else if (blkcnt == 1)
> -		cmd.cmdidx = MMC_CMD_WRITE_SINGLE_BLOCK;
> -	else
> -		cmd.cmdidx = MMC_CMD_WRITE_MULTIPLE_BLOCK;
> -
> -	if (mmc->high_capacity)
> -		cmd.cmdarg = start;
> -	else
> -		cmd.cmdarg = start * mmc->write_bl_len;
> -
> -	cmd.resp_type = MMC_RSP_R1;
> -
> -	data.src = src;
> -	data.blocks = blkcnt;
> -	data.blocksize = mmc->write_bl_len;
> -	data.flags = MMC_DATA_WRITE;
> -
> -	if (mmc_send_cmd(mmc, &cmd, &data)) {
> -#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> -		printf("mmc write failed\n");
> -#endif
> -		return 0;
> -	}
> -
> -	/* SPI multiblock writes terminate using a special
> -	 * token, not a STOP_TRANSMISSION request.
> -	 */
> -	if (!mmc_host_is_spi(mmc) && blkcnt > 1) {
> -		cmd.cmdidx = MMC_CMD_STOP_TRANSMISSION;
> -		cmd.cmdarg = 0;
> -		cmd.resp_type = MMC_RSP_R1b;
> -		if (mmc_send_cmd(mmc, &cmd, NULL)) {
> -#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> -			printf("mmc fail to send stop cmd\n");
> -#endif
> -			return 0;
> -		}
> -	}
> -
> -	/* Waiting for the ready status */
> -	if (mmc_send_status(mmc, timeout))
> -		return 0;
> -
> -	return blkcnt;
> -}
> -
> -static ulong
> -mmc_bwrite(int dev_num, lbaint_t start, lbaint_t blkcnt, const void*src)
> -{
> -	lbaint_t cur, blocks_todo = blkcnt;
> -
> -	struct mmc *mmc = find_mmc_device(dev_num);
> -	if (!mmc)
> -		return 0;
> -
> -	if (mmc_set_blocklen(mmc, mmc->write_bl_len))
> -		return 0;
> -
> -	do {
> -		cur = (blocks_todo > mmc->b_max) ?  mmc->b_max : blocks_todo;
> -		if(mmc_write_blocks(mmc, start, cur, src) != cur)
> -			return 0;
> -		blocks_todo -= cur;
> -		start += cur;
> -		src += cur * mmc->write_bl_len;
> -	} while (blocks_todo > 0);
> -
> -	return blkcnt;
> -}
> -
> static int mmc_read_blocks(struct mmc *mmc, void *dst, lbaint_t start,
> 			   lbaint_t blkcnt)
> {
> diff --git a/drivers/mmc/mmc_private.h b/drivers/mmc/mmc_private.h
> new file mode 100644
> index 0000000..16dcf9f
> --- /dev/null
> +++ b/drivers/mmc/mmc_private.h
> @@ -0,0 +1,45 @@
> +/*
> + * Copyright 2008,2010 Freescale Semiconductor, Inc
> + * Andy Fleming
> + *
> + * Based (loosely) on the Linux code
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#ifndef _MMC_PRIVATE_H_
> +#define _MMC_PRIVATE_H_
> +
> +#include <mmc.h>
> +
> +extern int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
> +			struct mmc_data *data);
> +extern int mmc_send_status(struct mmc *mmc, int timeout);
> +extern int mmc_set_blocklen(struct mmc *mmc, int len);
> +
> +#ifndef CONFIG_SPL_BUILD
> +
> +extern unsigned long mmc_berase(int dev_num, lbaint_t start, lbaint_t blkcnt);
> +
> +extern ulong mmc_bwrite(int dev_num, lbaint_t start, lbaint_t blkcnt,
> +		const void *src);
> +
> +#else /* CONFIG_SPL_BUILD */
> +
> +/* SPL will never write or erase, declare dummies to reduce code size. */
> +
> +static inline unsigned long mmc_berase(int dev_num, lbaint_t start,
> +		lbaint_t blkcnt)
> +{
> +	return 0;
> +}
> +
> +static inline ulong mmc_bwrite(int dev_num, lbaint_t start, lbaint_t blkcnt,
> +		const void *src)
> +{
> +	return 0;
> +}
> +
> +#endif /* CONFIG_SPL_BUILD */
> +
> +#endif /* _MMC_PRIVATE_H_ */
> diff --git a/drivers/mmc/mmc_write.c b/drivers/mmc/mmc_write.c
> new file mode 100644
> index 0000000..dde5cf2
> --- /dev/null
> +++ b/drivers/mmc/mmc_write.c
> @@ -0,0 +1,189 @@
> +/*
> + * Copyright 2008, Freescale Semiconductor, Inc
> + * Andy Fleming
> + *
> + * Based vaguely on the Linux code
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#include <config.h>
> +#include <common.h>
> +#include <part.h>
> +#include "mmc_private.h"
> +
> +static ulong mmc_erase_t(struct mmc *mmc, ulong start, lbaint_t blkcnt)
> +{
> +	struct mmc_cmd cmd;
> +	ulong end;
> +	int err, start_cmd, end_cmd;
> +
> +	if (mmc->high_capacity) {
> +		end = start + blkcnt - 1;
> +	} else {
> +		end = (start + blkcnt - 1) * mmc->write_bl_len;
> +		start *= mmc->write_bl_len;
> +	}
> +
> +	if (IS_SD(mmc)) {
> +		start_cmd = SD_CMD_ERASE_WR_BLK_START;
> +		end_cmd = SD_CMD_ERASE_WR_BLK_END;
> +	} else {
> +		start_cmd = MMC_CMD_ERASE_GROUP_START;
> +		end_cmd = MMC_CMD_ERASE_GROUP_END;
> +	}
> +
> +	cmd.cmdidx = start_cmd;
> +	cmd.cmdarg = start;
> +	cmd.resp_type = MMC_RSP_R1;
> +
> +	err = mmc_send_cmd(mmc, &cmd, NULL);
> +	if (err)
> +		goto err_out;
> +
> +	cmd.cmdidx = end_cmd;
> +	cmd.cmdarg = end;
> +
> +	err = mmc_send_cmd(mmc, &cmd, NULL);
> +	if (err)
> +		goto err_out;
> +
> +	cmd.cmdidx = MMC_CMD_ERASE;
> +	cmd.cmdarg = SECURE_ERASE;
> +	cmd.resp_type = MMC_RSP_R1b;
> +
> +	err = mmc_send_cmd(mmc, &cmd, NULL);
> +	if (err)
> +		goto err_out;
> +
> +	return 0;
> +
> +err_out:
> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> +	puts("mmc erase failed\n");
> +#endif
> +	return err;
> +}
> +
> +unsigned long mmc_berase(int dev_num, lbaint_t start, lbaint_t blkcnt)
> +{
> +	int err = 0;
> +	struct mmc *mmc = find_mmc_device(dev_num);
> +	lbaint_t blk = 0, blk_r = 0;
> +	int timeout = 1000;
> +
> +	if (!mmc)
> +		return -1;
> +
> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> +	if ((start % mmc->erase_grp_size) || (blkcnt % mmc->erase_grp_size))
> +		printf("\n\nCaution! Your devices Erase group is 0x%x\n"
> +		       "The erase range would be change to "
> +		       "0x" LBAF "~0x" LBAF "\n\n",
> +		       mmc->erase_grp_size, start & ~(mmc->erase_grp_size - 1),
> +		       ((start + blkcnt + mmc->erase_grp_size)
> +		       & ~(mmc->erase_grp_size - 1)) - 1);
> +#endif
> +
> +	while (blk < blkcnt) {
> +		blk_r = ((blkcnt - blk) > mmc->erase_grp_size) ?
> +			mmc->erase_grp_size : (blkcnt - blk);
> +		err = mmc_erase_t(mmc, start + blk, blk_r);
> +		if (err)
> +			break;
> +
> +		blk += blk_r;
> +
> +		/* Waiting for the ready status */
> +		if (mmc_send_status(mmc, timeout))
> +			return 0;
> +	}
> +
> +	return blk;
> +}
> +
> +static ulong mmc_write_blocks(struct mmc *mmc, lbaint_t start,
> +		lbaint_t blkcnt, const void *src)
> +{
> +	struct mmc_cmd cmd;
> +	struct mmc_data data;
> +	int timeout = 1000;
> +
> +	if ((start + blkcnt) > mmc->block_dev.lba) {
> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> +		printf("MMC: block number 0x" LBAF " exceeds max(0x" LBAF ")\n",
> +		       start + blkcnt, mmc->block_dev.lba);
> +#endif
> +		return 0;
> +	}
> +
> +	if (blkcnt == 0)
> +		return 0;
> +	else if (blkcnt == 1)
> +		cmd.cmdidx = MMC_CMD_WRITE_SINGLE_BLOCK;
> +	else
> +		cmd.cmdidx = MMC_CMD_WRITE_MULTIPLE_BLOCK;
> +
> +	if (mmc->high_capacity)
> +		cmd.cmdarg = start;
> +	else
> +		cmd.cmdarg = start * mmc->write_bl_len;
> +
> +	cmd.resp_type = MMC_RSP_R1;
> +
> +	data.src = src;
> +	data.blocks = blkcnt;
> +	data.blocksize = mmc->write_bl_len;
> +	data.flags = MMC_DATA_WRITE;
> +
> +	if (mmc_send_cmd(mmc, &cmd, &data)) {
> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> +		printf("mmc write failed\n");
> +#endif
> +		return 0;
> +	}
> +
> +	/* SPI multiblock writes terminate using a special
> +	 * token, not a STOP_TRANSMISSION request.
> +	 */
> +	if (!mmc_host_is_spi(mmc) && blkcnt > 1) {
> +		cmd.cmdidx = MMC_CMD_STOP_TRANSMISSION;
> +		cmd.cmdarg = 0;
> +		cmd.resp_type = MMC_RSP_R1b;
> +		if (mmc_send_cmd(mmc, &cmd, NULL)) {
> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> +			printf("mmc fail to send stop cmd\n");
> +#endif
> +			return 0;
> +		}
> +	}
> +
> +	/* Waiting for the ready status */
> +	if (mmc_send_status(mmc, timeout))
> +		return 0;
> +
> +	return blkcnt;
> +}
> +
> +ulong mmc_bwrite(int dev_num, lbaint_t start, lbaint_t blkcnt, const void *src)
> +{
> +	lbaint_t cur, blocks_todo = blkcnt;
> +
> +	struct mmc *mmc = find_mmc_device(dev_num);
> +	if (!mmc)
> +		return 0;
> +
> +	if (mmc_set_blocklen(mmc, mmc->write_bl_len))
> +		return 0;
> +
> +	do {
> +		cur = (blocks_todo > mmc->b_max) ?  mmc->b_max : blocks_todo;
> +		if (mmc_write_blocks(mmc, start, cur, src) != cur)
> +			return 0;
> +		blocks_todo -= cur;
> +		start += cur;
> +		src += cur * mmc->write_bl_len;
> +	} while (blocks_todo > 0);
> +
> +	return blkcnt;
> +}
> -- 
> 1.8.4
> 
> 

Acked-by: Pantelis Antoniou <panto@antoniou-consulting.com>

Looks good to me; let's hear what other people say too.

Tom?

Regards

-- Pantelis

Dear Paul Burton,

On 06.09.13 15:43, Paul Burton wrote:
> For SPL builds this is just dead code since we'll only need to read.
> Eliminating it results in a significant size reduction for the SPL
> binary, which may be critical for certain platforms where the binary
> size is highly constrained.
> 
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> ---
> Changes in v2:
>   - Move the mmc_bwrite & mmc_berase functions to a new mmc_write.c
>     file which is only compiled for non-SPL builds, as per a request
>     from Pantelis Antoniou. This requires that a few formerly static
>     functions in mmc.c be accessible to the new file, so they are
>     declared in a new mmc_private.h header along with the write &
>     erase functions. For what it's worth I prefered v1, but hey ho.
> ---
>  drivers/mmc/Makefile      |   2 +
>  drivers/mmc/mmc.c         | 186 +--------------------------------------------
>  drivers/mmc/mmc_private.h |  45 +++++++++++
>  drivers/mmc/mmc_write.c   | 189 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 240 insertions(+), 182 deletions(-)
>  create mode 100644 drivers/mmc/mmc_private.h
>  create mode 100644 drivers/mmc/mmc_write.c

<snip>

> diff --git a/drivers/mmc/mmc_write.c b/drivers/mmc/mmc_write.c
> new file mode 100644
> index 0000000..dde5cf2
> --- /dev/null
> +++ b/drivers/mmc/mmc_write.c
> @@ -0,0 +1,189 @@
> +/*
> + * Copyright 2008, Freescale Semiconductor, Inc
> + * Andy Fleming
> + *
> + * Based vaguely on the Linux code
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#include <config.h>
> +#include <common.h>
> +#include <part.h>
> +#include "mmc_private.h"
> +
> +static ulong mmc_erase_t(struct mmc *mmc, ulong start, lbaint_t blkcnt)
> +{
> +	struct mmc_cmd cmd;
> +	ulong end;
> +	int err, start_cmd, end_cmd;
> +
> +	if (mmc->high_capacity) {
> +		end = start + blkcnt - 1;
> +	} else {
> +		end = (start + blkcnt - 1) * mmc->write_bl_len;
> +		start *= mmc->write_bl_len;
> +	}
> +
> +	if (IS_SD(mmc)) {
> +		start_cmd = SD_CMD_ERASE_WR_BLK_START;
> +		end_cmd = SD_CMD_ERASE_WR_BLK_END;
> +	} else {
> +		start_cmd = MMC_CMD_ERASE_GROUP_START;
> +		end_cmd = MMC_CMD_ERASE_GROUP_END;
> +	}
> +
> +	cmd.cmdidx = start_cmd;
> +	cmd.cmdarg = start;
> +	cmd.resp_type = MMC_RSP_R1;
> +
> +	err = mmc_send_cmd(mmc, &cmd, NULL);
> +	if (err)
> +		goto err_out;
> +
> +	cmd.cmdidx = end_cmd;
> +	cmd.cmdarg = end;
> +
> +	err = mmc_send_cmd(mmc, &cmd, NULL);
> +	if (err)
> +		goto err_out;
> +
> +	cmd.cmdidx = MMC_CMD_ERASE;
> +	cmd.cmdarg = SECURE_ERASE;
> +	cmd.resp_type = MMC_RSP_R1b;
> +
> +	err = mmc_send_cmd(mmc, &cmd, NULL);
> +	if (err)
> +		goto err_out;
> +
> +	return 0;
> +
> +err_out:
> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> +	puts("mmc erase failed\n");
> +#endif

this conditional compile in of puts/printf for SPL are no longer
required, I'd prefere to remove them globally in mmc_write.c.

> +	return err;
> +}

Rest of this patch looks good to me.

Best regards

Andreas Bießmann

On Sun 08 Sep 2013 09:48:20 BST, Andreas Bießmann wrote:
>
> Dear Paul Burton,
>
> On 06.09.13 15:43, Paul Burton wrote:
>>
>> For SPL builds this is just dead code since we'll only need to read.
>> Eliminating it results in a significant size reduction for the SPL
>> binary, which may be critical for certain platforms where the binary
>> size is highly constrained.
>>
>> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
>> ---
>> Changes in v2:
>> - Move the mmc_bwrite & mmc_berase functions to a new mmc_write.c
>> file which is only compiled for non-SPL builds, as per a request
>> from Pantelis Antoniou. This requires that a few formerly static
>> functions in mmc.c be accessible to the new file, so they are
>> declared in a new mmc_private.h header along with the write &
>> erase functions. For what it's worth I prefered v1, but hey ho.
>> ---
>> drivers/mmc/Makefile | 2 +
>> drivers/mmc/mmc.c | 186 +--------------------------------------------
>> drivers/mmc/mmc_private.h | 45 +++++++++++
>> drivers/mmc/mmc_write.c | 189 
>> ++++++++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 240 insertions(+), 182 deletions(-)
>> create mode 100644 drivers/mmc/mmc_private.h
>> create mode 100644 drivers/mmc/mmc_write.c
>
>
> <snip>
>
>>
>> diff --git a/drivers/mmc/mmc_write.c b/drivers/mmc/mmc_write.c
>> new file mode 100644
>> index 0000000..dde5cf2
>> --- /dev/null
>> +++ b/drivers/mmc/mmc_write.c
>> @@ -0,0 +1,189 @@
>> +/*
>> + * Copyright 2008, Freescale Semiconductor, Inc
>> + * Andy Fleming
>> + *
>> + * Based vaguely on the Linux code
>> + *
>> + * SPDX-License-Identifier: GPL-2.0+
>> + */
>> +
>> +#include <config.h>
>> +#include <common.h>
>> +#include <part.h>
>> +#include "mmc_private.h"
>> +
>> +static ulong mmc_erase_t(struct mmc *mmc, ulong start, lbaint_t blkcnt)
>> +{
>> + struct mmc_cmd cmd;
>> + ulong end;
>> + int err, start_cmd, end_cmd;
>> +
>> + if (mmc->high_capacity) {
>> + end = start + blkcnt - 1;
>> + } else {
>> + end = (start + blkcnt - 1) * mmc->write_bl_len;
>> + start *= mmc->write_bl_len;
>> + }
>> +
>> + if (IS_SD(mmc)) {
>> + start_cmd = SD_CMD_ERASE_WR_BLK_START;
>> + end_cmd = SD_CMD_ERASE_WR_BLK_END;
>> + } else {
>> + start_cmd = MMC_CMD_ERASE_GROUP_START;
>> + end_cmd = MMC_CMD_ERASE_GROUP_END;
>> + }
>> +
>> + cmd.cmdidx = start_cmd;
>> + cmd.cmdarg = start;
>> + cmd.resp_type = MMC_RSP_R1;
>> +
>> + err = mmc_send_cmd(mmc, &cmd, NULL);
>> + if (err)
>> + goto err_out;
>> +
>> + cmd.cmdidx = end_cmd;
>> + cmd.cmdarg = end;
>> +
>> + err = mmc_send_cmd(mmc, &cmd, NULL);
>> + if (err)
>> + goto err_out;
>> +
>> + cmd.cmdidx = MMC_CMD_ERASE;
>> + cmd.cmdarg = SECURE_ERASE;
>> + cmd.resp_type = MMC_RSP_R1b;
>> +
>> + err = mmc_send_cmd(mmc, &cmd, NULL);
>> + if (err)
>> + goto err_out;
>> +
>> + return 0;
>> +
>> +err_out:
>> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
>> + puts("mmc erase failed\n");
>> +#endif
>
>
> this conditional compile in of puts/printf for SPL are no longer
> required, I'd prefere to remove them globally in mmc_write.c.
Ah, yes good point, I'll remove that.
>
>>
>> + return err;
>> +}
>
>
> Rest of this patch looks good to me.
>
> Best regards
>
> Andreas Bießmann
Thanks for looking at it!

Paul

Hi Paul,

On Sep 9, 2013, at 11:14 AM, Paul Burton wrote:

> On Sun 08 Sep 2013 09:48:20 BST, Andreas Bießmann wrote:
>> 
>> Dear Paul Burton,
>> 
>> On 06.09.13 15:43, Paul Burton wrote:
>>> 
>>> For SPL builds this is just dead code since we'll only need to read.
>>> Eliminating it results in a significant size reduction for the SPL
>>> binary, which may be critical for certain platforms where the binary
>>> size is highly constrained.
>>> 
>>> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
>>> ---
>>> Changes in v2:
>>> - Move the mmc_bwrite & mmc_berase functions to a new mmc_write.c
>>> file which is only compiled for non-SPL builds, as per a request
>>> from Pantelis Antoniou. This requires that a few formerly static
>>> functions in mmc.c be accessible to the new file, so they are
>>> declared in a new mmc_private.h header along with the write &
>>> erase functions. For what it's worth I prefered v1, but hey ho.
>>> ---
>>> drivers/mmc/Makefile | 2 +
>>> drivers/mmc/mmc.c | 186 +--------------------------------------------
>>> drivers/mmc/mmc_private.h | 45 +++++++++++
>>> drivers/mmc/mmc_write.c | 189 ++++++++++++++++++++++++++++++++++++++++++++++
>>> 4 files changed, 240 insertions(+), 182 deletions(-)
>>> create mode 100644 drivers/mmc/mmc_private.h
>>> create mode 100644 drivers/mmc/mmc_write.c
>> 
>> 
>> <snip>
>> 
>>> 
>>> diff --git a/drivers/mmc/mmc_write.c b/drivers/mmc/mmc_write.c
>>> new file mode 100644
>>> index 0000000..dde5cf2
>>> --- /dev/null
>>> +++ b/drivers/mmc/mmc_write.c
>>> @@ -0,0 +1,189 @@
>>> +/*
>>> + * Copyright 2008, Freescale Semiconductor, Inc
>>> + * Andy Fleming
>>> + *
>>> + * Based vaguely on the Linux code
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0+
>>> + */
>>> +
>>> +#include <config.h>
>>> +#include <common.h>
>>> +#include <part.h>
>>> +#include "mmc_private.h"
>>> +
>>> +static ulong mmc_erase_t(struct mmc *mmc, ulong start, lbaint_t blkcnt)
>>> +{
>>> + struct mmc_cmd cmd;
>>> + ulong end;
>>> + int err, start_cmd, end_cmd;
>>> +
>>> + if (mmc->high_capacity) {
>>> + end = start + blkcnt - 1;
>>> + } else {
>>> + end = (start + blkcnt - 1) * mmc->write_bl_len;
>>> + start *= mmc->write_bl_len;
>>> + }
>>> +
>>> + if (IS_SD(mmc)) {
>>> + start_cmd = SD_CMD_ERASE_WR_BLK_START;
>>> + end_cmd = SD_CMD_ERASE_WR_BLK_END;
>>> + } else {
>>> + start_cmd = MMC_CMD_ERASE_GROUP_START;
>>> + end_cmd = MMC_CMD_ERASE_GROUP_END;
>>> + }
>>> +
>>> + cmd.cmdidx = start_cmd;
>>> + cmd.cmdarg = start;
>>> + cmd.resp_type = MMC_RSP_R1;
>>> +
>>> + err = mmc_send_cmd(mmc, &cmd, NULL);
>>> + if (err)
>>> + goto err_out;
>>> +
>>> + cmd.cmdidx = end_cmd;
>>> + cmd.cmdarg = end;
>>> +
>>> + err = mmc_send_cmd(mmc, &cmd, NULL);
>>> + if (err)
>>> + goto err_out;
>>> +
>>> + cmd.cmdidx = MMC_CMD_ERASE;
>>> + cmd.cmdarg = SECURE_ERASE;
>>> + cmd.resp_type = MMC_RSP_R1b;
>>> +
>>> + err = mmc_send_cmd(mmc, &cmd, NULL);
>>> + if (err)
>>> + goto err_out;
>>> +
>>> + return 0;
>>> +
>>> +err_out:
>>> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
>>> + puts("mmc erase failed\n");
>>> +#endif
>> 
>> 
>> this conditional compile in of puts/printf for SPL are no longer
>> required, I'd prefere to remove them globally in mmc_write.c.
> Ah, yes good point, I'll remove that.
>> 
>>> 
>>> + return err;
>>> +}
>> 
>> 
>> Rest of this patch looks good to me.
>> 
>> Best regards
>> 
>> Andreas Bießmann
> Thanks for looking at it!
> 
> Paul
> 

Seem good to me too. I'll give it a spin later this week and make sure nothing breaks.

Regards

-- Pantelis