Patchwork [U-Boot,3/5] mmc: don't call *printf or puts when SPL & !CONFIG_SPL_LIBCOMMON_SUPPORT

login
register
mail settings
Submitter Paul Burton
Date Sept. 4, 2013, 3:12 p.m.
Message ID <1378307547-16984-4-git-send-email-paul.burton@imgtec.com>
Download mbox | patch
Permalink /patch/272651/
State Accepted
Delegated to: Pantelis Antoniou
Headers show

Comments

Paul Burton - Sept. 4, 2013, 3:12 p.m.
If we don't have CONFIG_SPL_LIBCOMMON_SUPPORT defined then stdio
& *printf functions are unavailable & calling them will cause a link
failure.

Signed-off-by: Paul Burton <paul.burton@imgtec.com>
---
 drivers/mmc/mmc.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)
Pantelis Antoniou - Sept. 6, 2013, 12:48 p.m.
Hi Paul

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

> If we don't have CONFIG_SPL_LIBCOMMON_SUPPORT defined then stdio
> & *printf functions are unavailable & calling them will cause a link
> failure.
> 

> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> ---
> drivers/mmc/mmc.c | 36 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 36 insertions(+)
> 
> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> index 5502675..30a985b 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -135,8 +135,10 @@ static int mmc_send_status(struct mmc *mmc, int timeout)
> 			     MMC_STATE_PRG)
> 				break;
> 			else if (cmd.response[0] & MMC_STATUS_MASK) {
> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> 				printf("Status Error: 0x%08X\n",
> 					cmd.response[0]);
> +#endif
> 				return COMM_ERR;
> 			}
> 		} else if (--retries < 0)
> @@ -151,7 +153,9 @@ static int mmc_send_status(struct mmc *mmc, int timeout)
> 	printf("CURR STATE:%d\n", status);
> #endif
> 	if (timeout <= 0) {
> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> 		printf("Timeout waiting card ready\n");
> +#endif
> 		return TIMEOUT;
> 	}
> 
> @@ -181,7 +185,9 @@ struct mmc *find_mmc_device(int dev_num)
> 			return m;
> 	}
> 
> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> 	printf("MMC Device %d not found\n", dev_num);
> +#endif
> 
> 	return NULL;
> }
> @@ -233,7 +239,9 @@ static ulong mmc_erase_t(struct mmc *mmc, ulong start, lbaint_t blkcnt)
> 	return 0;
> 
> err_out:
> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> 	puts("mmc erase failed\n");
> +#endif
> 	return err;
> }
> 
> @@ -248,6 +256,7 @@ mmc_berase(int dev_num, lbaint_t start, lbaint_t blkcnt)
> 	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 "
> @@ -255,6 +264,7 @@ mmc_berase(int dev_num, lbaint_t start, lbaint_t blkcnt)
> 		       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) ?
> @@ -281,8 +291,10 @@ mmc_write_blocks(struct mmc *mmc, lbaint_t start, lbaint_t blkcnt, const void*sr
> 	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;
> 	}
> 
> @@ -306,7 +318,9 @@ mmc_write_blocks(struct mmc *mmc, lbaint_t start, lbaint_t blkcnt, const void*sr
> 	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;
> 	}
> 
> @@ -318,7 +332,9 @@ mmc_write_blocks(struct mmc *mmc, lbaint_t start, lbaint_t blkcnt, const void*sr
> 		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;
> 		}
> 	}
> @@ -385,7 +401,9 @@ static int mmc_read_blocks(struct mmc *mmc, void *dst, lbaint_t start,
> 		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;
> 		}
> 	}
> @@ -405,8 +423,10 @@ static ulong mmc_bread(int dev_num, lbaint_t start, lbaint_t blkcnt, void *dst)
> 		return 0;
> 
> 	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;
> 	}

The idea is sound, but I don't like peppering the source with #ifdefs here.

Why not create a varargs orintf macro and use it instead

i.e.


#if !defined(CONFIG_SPL_BUILD_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
#define mmc_printf(...) ...
#else
#define mmc_printf(...) /* nothing */
#endif


> 
> @@ -1268,6 +1288,7 @@ static int mmc_startup(struct mmc *mmc)
> 	mmc->block_dev.blksz = mmc->read_bl_len;
> 	mmc->block_dev.log2blksz = LOG2(mmc->block_dev.blksz);
> 	mmc->block_dev.lba = lldiv(mmc->capacity, mmc->read_bl_len);
> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> 	sprintf(mmc->block_dev.vendor, "Man %06x Snr %04x%04x",
> 		mmc->cid[0] >> 24, (mmc->cid[2] & 0xffff),
> 		(mmc->cid[3] >> 16) & 0xffff);
> @@ -1277,6 +1298,11 @@ static int mmc_startup(struct mmc *mmc)
> 		(mmc->cid[2] >> 24) & 0xff);
> 	sprintf(mmc->block_dev.revision, "%d.%d", (mmc->cid[2] >> 20) & 0xf,
> 		(mmc->cid[2] >> 16) & 0xf);
> +#else
> +	mmc->block_dev.vendor[0] = 0;
> +	mmc->block_dev.product[0] = 0;
> +	mmc->block_dev.revision[0] = 0;
> +#endif

^^^ 
What goes on here?

> #if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBDISK_SUPPORT)
> 	init_part(&mmc->block_dev);
> #endif
> @@ -1343,7 +1369,9 @@ int mmc_start_init(struct mmc *mmc)
> 
> 	if (mmc_getcd(mmc) == 0) {
> 		mmc->has_init = 0;
> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> 		printf("MMC: no card present\n");
> +#endif
> 		return NO_CARD_ERR;
> 	}
> 
> @@ -1378,7 +1406,9 @@ int mmc_start_init(struct mmc *mmc)
> 		err = mmc_send_op_cond(mmc);
> 
> 		if (err && err != IN_PROGRESS) {
> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> 			printf("Card did not respond to voltage select!\n");
> +#endif
> 			return UNUSABLE_ERR;
> 		}
> 	}
> @@ -1434,6 +1464,8 @@ static int __def_mmc_init(bd_t *bis)
> int cpu_mmc_init(bd_t *bis) __attribute__((weak, alias("__def_mmc_init")));
> int board_mmc_init(bd_t *bis) __attribute__((weak, alias("__def_mmc_init")));
> 
> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> +
> void print_mmc_devices(char separator)
> {
> 	struct mmc *m;
> @@ -1451,6 +1483,10 @@ void print_mmc_devices(char separator)
> 	printf("\n");
> }
> 
> +#else
> +void print_mmc_devices(char separator) { }
> +#endif
> +
> int get_mmc_num(void)
> {
> 	return cur_dev_num;
> -- 
> 1.8.3.4
> 
> 

CCing Tom Rini on this.

Regards

-- Pantelis
Paul Burton - Sept. 6, 2013, 12:51 p.m.
On 06/09/13 13:48, Pantelis Antoniou wrote:
> Hi Paul
>
> On Sep 4, 2013, at 6:12 PM, Paul Burton wrote:
>
>> If we don't have CONFIG_SPL_LIBCOMMON_SUPPORT defined then stdio
>> & *printf functions are unavailable & calling them will cause a link
>> failure.
>>
>> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
>> ---
>> drivers/mmc/mmc.c | 36 ++++++++++++++++++++++++++++++++++++
>> 1 file changed, 36 insertions(+)
>>
>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
>> index 5502675..30a985b 100644
>> --- a/drivers/mmc/mmc.c
>> +++ b/drivers/mmc/mmc.c
>> @@ -135,8 +135,10 @@ static int mmc_send_status(struct mmc *mmc, int timeout)
>> 			     MMC_STATE_PRG)
>> 				break;
>> 			else if (cmd.response[0] & MMC_STATUS_MASK) {
>> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
>> 				printf("Status Error: 0x%08X\n",
>> 					cmd.response[0]);
>> +#endif
>> 				return COMM_ERR;
>> 			}
>> 		} else if (--retries < 0)
>> @@ -151,7 +153,9 @@ static int mmc_send_status(struct mmc *mmc, int timeout)
>> 	printf("CURR STATE:%d\n", status);
>> #endif
>> 	if (timeout <= 0) {
>> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
>> 		printf("Timeout waiting card ready\n");
>> +#endif
>> 		return TIMEOUT;
>> 	}
>>
>> @@ -181,7 +185,9 @@ struct mmc *find_mmc_device(int dev_num)
>> 			return m;
>> 	}
>>
>> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
>> 	printf("MMC Device %d not found\n", dev_num);
>> +#endif
>>
>> 	return NULL;
>> }
>> @@ -233,7 +239,9 @@ static ulong mmc_erase_t(struct mmc *mmc, ulong start, lbaint_t blkcnt)
>> 	return 0;
>>
>> err_out:
>> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
>> 	puts("mmc erase failed\n");
>> +#endif
>> 	return err;
>> }
>>
>> @@ -248,6 +256,7 @@ mmc_berase(int dev_num, lbaint_t start, lbaint_t blkcnt)
>> 	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 "
>> @@ -255,6 +264,7 @@ mmc_berase(int dev_num, lbaint_t start, lbaint_t blkcnt)
>> 		       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) ?
>> @@ -281,8 +291,10 @@ mmc_write_blocks(struct mmc *mmc, lbaint_t start, lbaint_t blkcnt, const void*sr
>> 	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;
>> 	}
>>
>> @@ -306,7 +318,9 @@ mmc_write_blocks(struct mmc *mmc, lbaint_t start, lbaint_t blkcnt, const void*sr
>> 	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;
>> 	}
>>
>> @@ -318,7 +332,9 @@ mmc_write_blocks(struct mmc *mmc, lbaint_t start, lbaint_t blkcnt, const void*sr
>> 		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;
>> 		}
>> 	}
>> @@ -385,7 +401,9 @@ static int mmc_read_blocks(struct mmc *mmc, void *dst, lbaint_t start,
>> 		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;
>> 		}
>> 	}
>> @@ -405,8 +423,10 @@ static ulong mmc_bread(int dev_num, lbaint_t start, lbaint_t blkcnt, void *dst)
>> 		return 0;
>>
>> 	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;
>> 	}
> The idea is sound, but I don't like peppering the source with #ifdefs here.
>
> Why not create a varargs orintf macro and use it instead
>
> i.e.
>
>
> #if !defined(CONFIG_SPL_BUILD_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
> #define mmc_printf(...) ...
> #else
> #define mmc_printf(...) /* nothing */
> #endif
That would work too, I'm fine with changing to that if people prefer it.


>> @@ -1268,6 +1288,7 @@ static int mmc_startup(struct mmc *mmc)
>> 	mmc->block_dev.blksz = mmc->read_bl_len;
>> 	mmc->block_dev.log2blksz = LOG2(mmc->block_dev.blksz);
>> 	mmc->block_dev.lba = lldiv(mmc->capacity, mmc->read_bl_len);
>> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
>> 	sprintf(mmc->block_dev.vendor, "Man %06x Snr %04x%04x",
>> 		mmc->cid[0] >> 24, (mmc->cid[2] & 0xffff),
>> 		(mmc->cid[3] >> 16) & 0xffff);
>> @@ -1277,6 +1298,11 @@ static int mmc_startup(struct mmc *mmc)
>> 		(mmc->cid[2] >> 24) & 0xff);
>> 	sprintf(mmc->block_dev.revision, "%d.%d", (mmc->cid[2] >> 20) & 0xf,
>> 		(mmc->cid[2] >> 16) & 0xf);
>> +#else
>> +	mmc->block_dev.vendor[0] = 0;
>> +	mmc->block_dev.product[0] = 0;
>> +	mmc->block_dev.revision[0] = 0;
>> +#endif
> ^^^
> What goes on here?
Thanks for pointing that out, perhaps I should comment on it in the 
source. Basically it needs to avoid sprintf, and since there's no 
possibility that the information will ever be displayed in this case 
(and it has no other uses than display) I figured it would be fine to 
just have empty strings.

>> #if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBDISK_SUPPORT)
>> 	init_part(&mmc->block_dev);
>> #endif
>> @@ -1343,7 +1369,9 @@ int mmc_start_init(struct mmc *mmc)
>>
>> 	if (mmc_getcd(mmc) == 0) {
>> 		mmc->has_init = 0;
>> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
>> 		printf("MMC: no card present\n");
>> +#endif
>> 		return NO_CARD_ERR;
>> 	}
>>
>> @@ -1378,7 +1406,9 @@ int mmc_start_init(struct mmc *mmc)
>> 		err = mmc_send_op_cond(mmc);
>>
>> 		if (err && err != IN_PROGRESS) {
>> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
>> 			printf("Card did not respond to voltage select!\n");
>> +#endif
>> 			return UNUSABLE_ERR;
>> 		}
>> 	}
>> @@ -1434,6 +1464,8 @@ static int __def_mmc_init(bd_t *bis)
>> int cpu_mmc_init(bd_t *bis) __attribute__((weak, alias("__def_mmc_init")));
>> int board_mmc_init(bd_t *bis) __attribute__((weak, alias("__def_mmc_init")));
>>
>> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
>> +
>> void print_mmc_devices(char separator)
>> {
>> 	struct mmc *m;
>> @@ -1451,6 +1483,10 @@ void print_mmc_devices(char separator)
>> 	printf("\n");
>> }
>>
>> +#else
>> +void print_mmc_devices(char separator) { }
>> +#endif
>> +
>> int get_mmc_num(void)
>> {
>> 	return cur_dev_num;
>> -- 
>> 1.8.3.4
>>
>>
> CCing Tom Rini on this.
>
> Regards
>
> -- Pantelis
>

Thanks,
     Paul
Pantelis Antoniou - Sept. 6, 2013, 12:55 p.m.
Hi Paul

On Sep 6, 2013, at 3:51 PM, Paul Burton wrote:

> 
> On 06/09/13 13:48, Pantelis Antoniou wrote:
>> Hi Paul
>> 
>> On Sep 4, 2013, at 6:12 PM, Paul Burton wrote:
>> 
>>> If we don't have CONFIG_SPL_LIBCOMMON_SUPPORT defined then stdio
>>> & *printf functions are unavailable & calling them will cause a link
>>> failure.
>>> 
>>> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
>>> ---
>>> drivers/mmc/mmc.c | 36 ++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 36 insertions(+)
>>> 
>>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
>>> index 5502675..30a985b 100644
>>> --- a/drivers/mmc/mmc.c
>>> +++ b/drivers/mmc/mmc.c
>>> @@ -135,8 +135,10 @@ static int mmc_send_status(struct mmc *mmc, int timeout)
>>> 			     MMC_STATE_PRG)
>>> 				break;
>>> 			else if (cmd.response[0] & MMC_STATUS_MASK) {
>>> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
>>> 				printf("Status Error: 0x%08X\n",
>>> 					cmd.response[0]);
>>> +#endif
>>> 				return COMM_ERR;
>>> 			}
>>> 		} else if (--retries < 0)
>>> @@ -151,7 +153,9 @@ static int mmc_send_status(struct mmc *mmc, int timeout)
>>> 	printf("CURR STATE:%d\n", status);
>>> #endif
>>> 	if (timeout <= 0) {
>>> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
>>> 		printf("Timeout waiting card ready\n");
>>> +#endif
>>> 		return TIMEOUT;
>>> 	}
>>> 
>>> @@ -181,7 +185,9 @@ struct mmc *find_mmc_device(int dev_num)
>>> 			return m;
>>> 	}
>>> 
>>> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
>>> 	printf("MMC Device %d not found\n", dev_num);
>>> +#endif
>>> 
>>> 	return NULL;
>>> }
>>> @@ -233,7 +239,9 @@ static ulong mmc_erase_t(struct mmc *mmc, ulong start, lbaint_t blkcnt)
>>> 	return 0;
>>> 
>>> err_out:
>>> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
>>> 	puts("mmc erase failed\n");
>>> +#endif
>>> 	return err;
>>> }
>>> 
>>> @@ -248,6 +256,7 @@ mmc_berase(int dev_num, lbaint_t start, lbaint_t blkcnt)
>>> 	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 "
>>> @@ -255,6 +264,7 @@ mmc_berase(int dev_num, lbaint_t start, lbaint_t blkcnt)
>>> 		       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) ?
>>> @@ -281,8 +291,10 @@ mmc_write_blocks(struct mmc *mmc, lbaint_t start, lbaint_t blkcnt, const void*sr
>>> 	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;
>>> 	}
>>> 
>>> @@ -306,7 +318,9 @@ mmc_write_blocks(struct mmc *mmc, lbaint_t start, lbaint_t blkcnt, const void*sr
>>> 	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;
>>> 	}
>>> 
>>> @@ -318,7 +332,9 @@ mmc_write_blocks(struct mmc *mmc, lbaint_t start, lbaint_t blkcnt, const void*sr
>>> 		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;
>>> 		}
>>> 	}
>>> @@ -385,7 +401,9 @@ static int mmc_read_blocks(struct mmc *mmc, void *dst, lbaint_t start,
>>> 		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;
>>> 		}
>>> 	}
>>> @@ -405,8 +423,10 @@ static ulong mmc_bread(int dev_num, lbaint_t start, lbaint_t blkcnt, void *dst)
>>> 		return 0;
>>> 
>>> 	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;
>>> 	}
>> The idea is sound, but I don't like peppering the source with #ifdefs here.
>> 
>> Why not create a varargs orintf macro and use it instead
>> 
>> i.e.
>> 
>> 
>> #if !defined(CONFIG_SPL_BUILD_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
>> #define mmc_printf(...) ...
>> #else
>> #define mmc_printf(...) /* nothing */
>> #endif
> That would work too, I'm fine with changing to that if people prefer it.
> 
> 

Speaking of which this isn't really mmc specific, more like SPL specific; perhaps what needs to
be done is have by default printf defined to empty on SPL and have an foo_printf that always outputs.

Tom, what do you think?

>>> @@ -1268,6 +1288,7 @@ static int mmc_startup(struct mmc *mmc)
>>> 	mmc->block_dev.blksz = mmc->read_bl_len;
>>> 	mmc->block_dev.log2blksz = LOG2(mmc->block_dev.blksz);
>>> 	mmc->block_dev.lba = lldiv(mmc->capacity, mmc->read_bl_len);
>>> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
>>> 	sprintf(mmc->block_dev.vendor, "Man %06x Snr %04x%04x",
>>> 		mmc->cid[0] >> 24, (mmc->cid[2] & 0xffff),
>>> 		(mmc->cid[3] >> 16) & 0xffff);
>>> @@ -1277,6 +1298,11 @@ static int mmc_startup(struct mmc *mmc)
>>> 		(mmc->cid[2] >> 24) & 0xff);
>>> 	sprintf(mmc->block_dev.revision, "%d.%d", (mmc->cid[2] >> 20) & 0xf,
>>> 		(mmc->cid[2] >> 16) & 0xf);
>>> +#else
>>> +	mmc->block_dev.vendor[0] = 0;
>>> +	mmc->block_dev.product[0] = 0;
>>> +	mmc->block_dev.revision[0] = 0;
>>> +#endif
>> ^^^
>> What goes on here?
> Thanks for pointing that out, perhaps I should comment on it in the source. Basically it needs to avoid sprintf, and since there's no possibility that the information will ever be displayed in this case (and it has no other uses than display) I figured it would be fine to just have empty strings.
> 

Drop this please then, and put it in another patch if need be. I still don't like it.

>>> #if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBDISK_SUPPORT)
>>> 	init_part(&mmc->block_dev);
>>> #endif
>>> @@ -1343,7 +1369,9 @@ int mmc_start_init(struct mmc *mmc)
>>> 
>>> 	if (mmc_getcd(mmc) == 0) {
>>> 		mmc->has_init = 0;
>>> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
>>> 		printf("MMC: no card present\n");
>>> +#endif
>>> 		return NO_CARD_ERR;
>>> 	}
>>> 
>>> @@ -1378,7 +1406,9 @@ int mmc_start_init(struct mmc *mmc)
>>> 		err = mmc_send_op_cond(mmc);
>>> 
>>> 		if (err && err != IN_PROGRESS) {
>>> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
>>> 			printf("Card did not respond to voltage select!\n");
>>> +#endif
>>> 			return UNUSABLE_ERR;
>>> 		}
>>> 	}
>>> @@ -1434,6 +1464,8 @@ static int __def_mmc_init(bd_t *bis)
>>> int cpu_mmc_init(bd_t *bis) __attribute__((weak, alias("__def_mmc_init")));
>>> int board_mmc_init(bd_t *bis) __attribute__((weak, alias("__def_mmc_init")));
>>> 
>>> +#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
>>> +
>>> void print_mmc_devices(char separator)
>>> {
>>> 	struct mmc *m;
>>> @@ -1451,6 +1483,10 @@ void print_mmc_devices(char separator)
>>> 	printf("\n");
>>> }
>>> 
>>> +#else
>>> +void print_mmc_devices(char separator) { }
>>> +#endif
>>> +
>>> int get_mmc_num(void)
>>> {
>>> 	return cur_dev_num;
>>> -- 
>>> 1.8.3.4
>>> 
>>> 
>> CCing Tom Rini on this.
>> 
>> Regards
>> 
>> -- Pantelis
>> 
> 
> Thanks,
>    Paul
> 

Regards

-- Pantelis

Patch

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 5502675..30a985b 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -135,8 +135,10 @@  static int mmc_send_status(struct mmc *mmc, int timeout)
 			     MMC_STATE_PRG)
 				break;
 			else if (cmd.response[0] & MMC_STATUS_MASK) {
+#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
 				printf("Status Error: 0x%08X\n",
 					cmd.response[0]);
+#endif
 				return COMM_ERR;
 			}
 		} else if (--retries < 0)
@@ -151,7 +153,9 @@  static int mmc_send_status(struct mmc *mmc, int timeout)
 	printf("CURR STATE:%d\n", status);
 #endif
 	if (timeout <= 0) {
+#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
 		printf("Timeout waiting card ready\n");
+#endif
 		return TIMEOUT;
 	}
 
@@ -181,7 +185,9 @@  struct mmc *find_mmc_device(int dev_num)
 			return m;
 	}
 
+#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
 	printf("MMC Device %d not found\n", dev_num);
+#endif
 
 	return NULL;
 }
@@ -233,7 +239,9 @@  static ulong mmc_erase_t(struct mmc *mmc, ulong start, lbaint_t blkcnt)
 	return 0;
 
 err_out:
+#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
 	puts("mmc erase failed\n");
+#endif
 	return err;
 }
 
@@ -248,6 +256,7 @@  mmc_berase(int dev_num, lbaint_t start, lbaint_t blkcnt)
 	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 "
@@ -255,6 +264,7 @@  mmc_berase(int dev_num, lbaint_t start, lbaint_t blkcnt)
 		       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) ?
@@ -281,8 +291,10 @@  mmc_write_blocks(struct mmc *mmc, lbaint_t start, lbaint_t blkcnt, const void*sr
 	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;
 	}
 
@@ -306,7 +318,9 @@  mmc_write_blocks(struct mmc *mmc, lbaint_t start, lbaint_t blkcnt, const void*sr
 	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;
 	}
 
@@ -318,7 +332,9 @@  mmc_write_blocks(struct mmc *mmc, lbaint_t start, lbaint_t blkcnt, const void*sr
 		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;
 		}
 	}
@@ -385,7 +401,9 @@  static int mmc_read_blocks(struct mmc *mmc, void *dst, lbaint_t start,
 		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;
 		}
 	}
@@ -405,8 +423,10 @@  static ulong mmc_bread(int dev_num, lbaint_t start, lbaint_t blkcnt, void *dst)
 		return 0;
 
 	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;
 	}
 
@@ -1268,6 +1288,7 @@  static int mmc_startup(struct mmc *mmc)
 	mmc->block_dev.blksz = mmc->read_bl_len;
 	mmc->block_dev.log2blksz = LOG2(mmc->block_dev.blksz);
 	mmc->block_dev.lba = lldiv(mmc->capacity, mmc->read_bl_len);
+#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
 	sprintf(mmc->block_dev.vendor, "Man %06x Snr %04x%04x",
 		mmc->cid[0] >> 24, (mmc->cid[2] & 0xffff),
 		(mmc->cid[3] >> 16) & 0xffff);
@@ -1277,6 +1298,11 @@  static int mmc_startup(struct mmc *mmc)
 		(mmc->cid[2] >> 24) & 0xff);
 	sprintf(mmc->block_dev.revision, "%d.%d", (mmc->cid[2] >> 20) & 0xf,
 		(mmc->cid[2] >> 16) & 0xf);
+#else
+	mmc->block_dev.vendor[0] = 0;
+	mmc->block_dev.product[0] = 0;
+	mmc->block_dev.revision[0] = 0;
+#endif
 #if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBDISK_SUPPORT)
 	init_part(&mmc->block_dev);
 #endif
@@ -1343,7 +1369,9 @@  int mmc_start_init(struct mmc *mmc)
 
 	if (mmc_getcd(mmc) == 0) {
 		mmc->has_init = 0;
+#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
 		printf("MMC: no card present\n");
+#endif
 		return NO_CARD_ERR;
 	}
 
@@ -1378,7 +1406,9 @@  int mmc_start_init(struct mmc *mmc)
 		err = mmc_send_op_cond(mmc);
 
 		if (err && err != IN_PROGRESS) {
+#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
 			printf("Card did not respond to voltage select!\n");
+#endif
 			return UNUSABLE_ERR;
 		}
 	}
@@ -1434,6 +1464,8 @@  static int __def_mmc_init(bd_t *bis)
 int cpu_mmc_init(bd_t *bis) __attribute__((weak, alias("__def_mmc_init")));
 int board_mmc_init(bd_t *bis) __attribute__((weak, alias("__def_mmc_init")));
 
+#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
+
 void print_mmc_devices(char separator)
 {
 	struct mmc *m;
@@ -1451,6 +1483,10 @@  void print_mmc_devices(char separator)
 	printf("\n");
 }
 
+#else
+void print_mmc_devices(char separator) { }
+#endif
+
 int get_mmc_num(void)
 {
 	return cur_dev_num;