[U-Boot] [PATCH] mmc: add wrappers for MMC block_{read, write, erase}

[Steve Rae <srae@broadcom.com>]

Hi, Jaehoon

On 14-05-29 12:03 AM, Jaehoon Chung wrote:
> Hi, Steve.
>
> On 05/29/2014 07:15 AM, Steve Rae wrote:
>> Each wrapper function:
>> - switches to the specified physical partition, then
>> - performs the original function, and then
>> - switches back to the original physical partition
>> where the physical partition (aka HW partition) is
>>    0=User, 1=Boot1, 2=Boot2, etc.
>>
>> Signed-off-by: Steve Rae <srae@broadcom.com>
>> ---
>> based on a discussion: http://lists.denx.de/pipermail/u-boot/2014-April/178171.html
>> The original calling code is (for example):
>>    mmc->block_dev.block_read(dev_num, start, blkcnt, buffer)
>> Therefore, these wrappers use the following naming convention:
>>    mmc_block_read_hwpart(dev_num, part_num, start, blkcnt, buffer)
>> "hwpart" comes from: Stephen Warren <swarren@nvidia.com>
>>
>>   drivers/mmc/Makefile     |  1 +
>>   drivers/mmc/mmc_hwpart.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/mmc.h            | 10 +++++++
>>   3 files changed, 86 insertions(+)
>>   create mode 100644 drivers/mmc/mmc_hwpart.c
>>
>> diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile
>> index 4c6ab9e..04f87f9 100644
>> --- a/drivers/mmc/Makefile
>> +++ b/drivers/mmc/Makefile
>> @@ -11,6 +11,7 @@ obj-$(CONFIG_FSL_ESDHC) += fsl_esdhc.o
>>   obj-$(CONFIG_FTSDC010) += ftsdc010_mci.o
>>   obj-$(CONFIG_FTSDC021) += ftsdc021_sdhci.o
>>   obj-$(CONFIG_GENERIC_MMC) += mmc.o
>> +obj-$(CONFIG_GENERIC_MMC) += mmc_hwpart.o
>>   obj-$(CONFIG_GENERIC_ATMEL_MCI) += gen_atmel_mci.o
>>   obj-$(CONFIG_MMC_SPI) += mmc_spi.o
>>   obj-$(CONFIG_ARM_PL180_MMCI) += arm_pl180_mmci.o
>> diff --git a/drivers/mmc/mmc_hwpart.c b/drivers/mmc/mmc_hwpart.c
>> new file mode 100644
>> index 0000000..1c29f8f
>> --- /dev/null
>> +++ b/drivers/mmc/mmc_hwpart.c
>> @@ -0,0 +1,75 @@
>> +/*
>> + * Copyright 2014 Broadcom Corporation.
>> + *
>> + * SPDX-License-Identifier:	GPL-2.0+
>> + */
>> +
>> +#include <mmc.h>
>> +
>> +static int switch_part(struct mmc *mmc,
>> +		       int dev,
>> +		       unsigned int chk_part_num,
>> +		       unsigned int part_num)
>> +{
>> +	if (!mmc)
>> +		return -1;
>> +
>> +	if (mmc->part_num != chk_part_num) {
>> +		if (mmc_switch_part(dev, part_num)) {
>> +			printf("MMC partition switch to %d failed [dev=%d]\n",
>> +			       part_num, dev);
>> +			return -1;
>> +		}
>> +	}
>> +	return 0;
>> +}
>> +
>> +unsigned long mmc_block_read_hwpart(int dev,
>> +				    unsigned int part_num,
>> +				    lbaint_t start,
>> +				    lbaint_t blkcnt,
>> +				    void *buffer)
>> +{
>> +	unsigned long rc = 0;
>> +	struct mmc *mmc = find_mmc_device(dev);
>> +
>> +	if (switch_part(mmc, dev, part_num, part_num))
>> +		return 0;
> return 0 is right?
>
The original function returns 0 on error (and blkcnt on success)

>> +	rc = mmc->block_dev.block_read(dev, start, blkcnt, buffer);
>> +	switch_part(mmc, dev, part_num, mmc->part_num);
>
> Didn't need to check whether switched partition or not? And if block_read is failed?
>
The calling function needs to handle the situation where block_read 
failed...
If switching back fails (after the previous switching to was successful, 
then there is not much we can do. Except that we should update 
"mmc->part-num" to point to the current partition - I'll add that for v2 
(and in the next functions too)

>> +
>> +	return rc;
>> +}
>> +
>> +unsigned long mmc_block_write_hwpart(int dev,
>> +				     unsigned int part_num,
>> +				     lbaint_t start,
>> +				     lbaint_t blkcnt,
>> +				     const void *buffer)
>> +{
>> +	unsigned long rc = 0;
>> +	struct mmc *mmc = find_mmc_device(dev);
>> +
>> +	if (switch_part(mmc, dev, part_num, part_num))
>> +		return 0;
>
> ditto..
>
The original function returns 0 on error (and blkcnt on success)

>> +	rc = mmc->block_dev.block_write(dev, start, blkcnt, buffer);
>> +	switch_part(mmc, dev, part_num, mmc->part_num);
>> +
>> +	return rc;
>> +}
>> +
>> +unsigned long mmc_block_erase_hwpart(int dev,
>> +				     unsigned int part_num,
>> +				     lbaint_t start,
>> +				     lbaint_t blkcnt)
>> +{
>> +	unsigned long rc = -1;
> Why did you assign to "-1"?
>
this is the default error condition -- but it is not needed to be 
initialized in this function (or the others) - will fix in v2

>> +	struct mmc *mmc = find_mmc_device(dev);
>> +/blk_r

>> +	if (switch_part(mmc, dev, part_num, part_num))
>> +		return -1;
> At here, return -1?
>
The original function returns -1 on error (0 on timeout, otherwise, 
number of blocks erased)

> Best Regards,
> Jaehoon Chung
>> +	rc = mmc->block_dev.block_erase(dev, start, blkcnt);
>> +	switch_part(mmc, dev, part_num, mmc->part_num);
>> +
>> +	return rc;otherwise
>> +}
>> diff --git a/include/mmc.h b/include/mmc.h
>> index a3a100b..4871c08 100644
>> --- a/include/mmc.h
>> +++ b/include/mmc.h
>> @@ -347,6 +347,16 @@ int mmc_rpmb_read(struct mmc *mmc, void *addr, unsigned short blk,
>>   		  unsigned short cnt, unsigned char *key);
>>   int mmc_rpmb_write(struct mmc *mmc, void *addr, unsigned short blk,
>>   		   unsigned short cnt, unsigned char *key);
>> +/* Functions to read/write/erase from the specified HW partition */
>> +unsigned long mmc_block_read_hwpart(int dev, unsigned int part_num,
>> +				    lbaint_t start, lbaint_t blkcnt,
>> +				    void *buffer);
>> +unsigned long mmc_block_write_hwpart(int dev, unsigned int part_num,
>> +				     lbaint_t start, lbaint_t blkcnt,
>> +				     const void *buffer);
>> +
>> +unsigned long mmc_block_erase_hwpart(int dev, unsigned int part_num,
>> +				     lbaint_t start, lbaint_t blkcnt);
>>   /**
>>    * Start device initialization and return immediately; it does not block on
>>    * polling OCR (operation condition register) status.  Then you should call
>>
>