From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steve Rae Date: Thu, 29 May 2014 10:24:08 -0700 Subject: [U-Boot] [PATCH] mmc: add wrappers for MMC block_{read, write, erase} In-Reply-To: <5386DBC2.2040705@samsung.com> References: <1401315346-30231-1-git-send-email-srae@broadcom.com> <5386DBC2.2040705@samsung.com> Message-ID: <53876D38.6090204@broadcom.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >> --- >> 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 >> >> 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 >> + >> +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 >> >