mmc: add wrappers for MMC block_{read, write, erase}

Message ID 1401315346-30231-1-git-send-email-srae@broadcom.com
State New
Headers show

Commit Message

Steve Rae May 28, 2014, 10:15 p.m.
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

Comments

Jaehoon Chung May 29, 2014, 5:47 a.m. | #1
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?

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

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

> +	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"?

> +	struct mmc *mmc = find_mmc_device(dev);
> +
> +	if (switch_part(mmc, dev, part_num, part_num))
> +		return -1;
At here, return -1?

> +	rc = mmc->block_dev.block_erase(dev, start, blkcnt);
> +	switch_part(mmc, dev, part_num, mmc->part_num);
> +
> +	return rc;
> +}
> 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
>
Jaehoon Chung May 29, 2014, 7:03 a.m. | #2
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?

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

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

> +	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"?

> +	struct mmc *mmc = find_mmc_device(dev);
> +
> +	if (switch_part(mmc, dev, part_num, part_num))
> +		return -1;
At here, return -1?

Best Regards,
Jaehoon Chung
> +	rc = mmc->block_dev.block_erase(dev, start, blkcnt);
> +	switch_part(mmc, dev, part_num, mmc->part_num);
> +
> +	return rc;
> +}
> 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
>
Stephen Warren May 29, 2014, 4:25 p.m. | #3
On 05/28/2014 04:15 PM, 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.

This feels wrong; why wouldn't mmc_get_dev() return a block_dev_desc_t
containing block_read/block_write functions that do the HW partition
switching. That way, this is all completely hidden, and all client code
only knows about block devices, rather than having to know about
MMC-specific mmc_block_read/write/erase_hwpart() functions.
Steve Rae May 29, 2014, 5:24 p.m. | #4
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
>>
>
Steve Rae May 29, 2014, 5:58 p.m. | #5
Hi, Stephen

On 14-05-29 09:25 AM, Stephen Warren wrote:
> On 05/28/2014 04:15 PM, 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.
>
> This feels wrong; why wouldn't mmc_get_dev() return a block_dev_desc_t
> containing block_read/block_write functions that do the HW partition
> switching. That way, this is all completely hidden, and all client code
> only knows about block devices, rather than having to know about
> MMC-specific mmc_block_read/write/erase_hwpart() functions.
>
This goes back to the initial discussion on this mailing list (which was 
never resolved):
   http://lists.denx.de/pipermail/u-boot/2014-April/178171.html
This issue is that the three callback functions defined in 
'block_desc_t' do not accept the "partition number" as an argument.
It was suggested that we could overwrite those functions; but the 
"partition number" still needs to be passed in by:
(1) overloading the "int dev_num" argument, or
(2) adding another argument to the callback functions
I assumed that neither of these was acceptable, so I have proposed these 
wrappers...

Thanks, Steve
Stephen Warren May 29, 2014, 6:51 p.m. | #6
On 05/29/2014 11:58 AM, Steve Rae wrote:
> Hi, Stephen
> 
> On 14-05-29 09:25 AM, Stephen Warren wrote:
>> On 05/28/2014 04:15 PM, 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.
>>
>> This feels wrong; why wouldn't mmc_get_dev() return a block_dev_desc_t
>> containing block_read/block_write functions that do the HW partition
>> switching. That way, this is all completely hidden, and all client code
>> only knows about block devices, rather than having to know about
>> MMC-specific mmc_block_read/write/erase_hwpart() functions.
>>
> This goes back to the initial discussion on this mailing list (which was
> never resolved):
>   http://lists.denx.de/pipermail/u-boot/2014-April/178171.html
> This issue is that the three callback functions defined in
> 'block_desc_t' do not accept the "partition number" as an argument.
> It was suggested that we could overwrite those functions; but the
> "partition number" still needs to be passed in by:
> (1) overloading the "int dev_num" argument, or
> (2) adding another argument to the callback functions
> I assumed that neither of these was acceptable, so I have proposed these
> wrappers...

Can't the data simply be stored in the block_desc_t itself?
Steve Rae May 29, 2014, 7:44 p.m. | #7
On 14-05-29 11:51 AM, Stephen Warren wrote:
> On 05/29/2014 11:58 AM, Steve Rae wrote:
>> Hi, Stephen
>>
>> On 14-05-29 09:25 AM, Stephen Warren wrote:
>>> On 05/28/2014 04:15 PM, 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.
>>>
>>> This feels wrong; why wouldn't mmc_get_dev() return a block_dev_desc_t
>>> containing block_read/block_write functions that do the HW partition
>>> switching. That way, this is all completely hidden, and all client code
>>> only knows about block devices, rather than having to know about
>>> MMC-specific mmc_block_read/write/erase_hwpart() functions.
>>>
>> This goes back to the initial discussion on this mailing list (which was
>> never resolved):
>>    http://lists.denx.de/pipermail/u-boot/2014-April/178171.html
>> This issue is that the three callback functions defined in
>> 'block_desc_t' do not accept the "partition number" as an argument.
>> It was suggested that we could overwrite those functions; but the
>> "partition number" still needs to be passed in by:
>> (1) overloading the "int dev_num" argument, or
>> (2) adding another argument to the callback functions
>> I assumed that neither of these was acceptable, so I have proposed these
>> wrappers...
>
> Can't the data simply be stored in the block_desc_t itself?
>
If I understand this suggestion, are you proposing:
- add an "unsigned int specified_hw_part" to the block_desc_t

Then the usage would become:
mmc->block_dev.specified_hw_part = 1;	  /* specify Boot1 partition */
mmc->block_dev.block_read(0, 0, 1, buf);  /* read first block (from 
Boot1 partition) */
mmc->block_dev.specified_hw_part = 0;	  /* specify User partition */
mmc->block_dev.block_read(0, 0, 1, buf);  /* read first block (from User 
partition) */

I don't think this is a good idea...
Please clarify!
Thanks, Steve
Stephen Warren May 29, 2014, 8:30 p.m. | #8
On 05/29/2014 01:44 PM, Steve Rae wrote:
> 
> 
> On 14-05-29 11:51 AM, Stephen Warren wrote:
>> On 05/29/2014 11:58 AM, Steve Rae wrote:
>>> Hi, Stephen
>>>
>>> On 14-05-29 09:25 AM, Stephen Warren wrote:
>>>> On 05/28/2014 04:15 PM, 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.
>>>>
>>>> This feels wrong; why wouldn't mmc_get_dev() return a block_dev_desc_t
>>>> containing block_read/block_write functions that do the HW partition
>>>> switching. That way, this is all completely hidden, and all client code
>>>> only knows about block devices, rather than having to know about
>>>> MMC-specific mmc_block_read/write/erase_hwpart() functions.
>>>>
>>> This goes back to the initial discussion on this mailing list (which was
>>> never resolved):
>>>    http://lists.denx.de/pipermail/u-boot/2014-April/178171.html
>>> This issue is that the three callback functions defined in
>>> 'block_desc_t' do not accept the "partition number" as an argument.
>>> It was suggested that we could overwrite those functions; but the
>>> "partition number" still needs to be passed in by:
>>> (1) overloading the "int dev_num" argument, or
>>> (2) adding another argument to the callback functions
>>> I assumed that neither of these was acceptable, so I have proposed these
>>> wrappers...
>>
>> Can't the data simply be stored in the block_desc_t itself?
>
> If I understand this suggestion, are you proposing:
> - add an "unsigned int specified_hw_part" to the block_desc_t

Yes.

> Then the usage would become:
> mmc->block_dev.specified_hw_part = 1;      /* specify Boot1 partition */

The only code that would need to assign that field is
disk/part.c:get_dev() or something called from it. that is the function
that's responsible for looking up or creating the block_dev_desc_t
"handle" for a user-specified storage device, so it's exactly the place
for this kind of object "constructor" code to execute.

> mmc->block_dev.block_read(0, 0, 1, buf);  /* read first block (from
> Boot1 partition) */

Yes.

> mmc->block_dev.specified_hw_part = 0;      /* specify User partition */
> mmc->block_dev.block_read(0, 0, 1, buf);  /* read first block (from User
> partition) */
> 
> I don't think this is a good idea...

Oh, but it is:-)
Steve Rae May 29, 2014, 10:03 p.m. | #9
On 14-05-29 01:30 PM, Stephen Warren wrote:
> On 05/29/2014 01:44 PM, Steve Rae wrote:
>>
>>
>> On 14-05-29 11:51 AM, Stephen Warren wrote:
>>> On 05/29/2014 11:58 AM, Steve Rae wrote:
>>>> Hi, Stephen
>>>>
>>>> On 14-05-29 09:25 AM, Stephen Warren wrote:
>>>>> On 05/28/2014 04:15 PM, 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.
>>>>>
>>>>> This feels wrong; why wouldn't mmc_get_dev() return a block_dev_desc_t
>>>>> containing block_read/block_write functions that do the HW partition
>>>>> switching. That way, this is all completely hidden, and all client code
>>>>> only knows about block devices, rather than having to know about
>>>>> MMC-specific mmc_block_read/write/erase_hwpart() functions.
>>>>>
>>>> This goes back to the initial discussion on this mailing list (which was
>>>> never resolved):
>>>>     http://lists.denx.de/pipermail/u-boot/2014-April/178171.html
>>>> This issue is that the three callback functions defined in
>>>> 'block_desc_t' do not accept the "partition number" as an argument.
>>>> It was suggested that we could overwrite those functions; but the
>>>> "partition number" still needs to be passed in by:
>>>> (1) overloading the "int dev_num" argument, or
>>>> (2) adding another argument to the callback functions
>>>> I assumed that neither of these was acceptable, so I have proposed these
>>>> wrappers...
>>>
>>> Can't the data simply be stored in the block_desc_t itself?
>>
>> If I understand this suggestion, are you proposing:
>> - add an "unsigned int specified_hw_part" to the block_desc_t
>
> Yes.
>
>> Then the usage would become:
>> mmc->block_dev.specified_hw_part = 1;      /* specify Boot1 partition */
>
> The only code that would need to assign that field is
> disk/part.c:get_dev() or something called from it. that is the function
> that's responsible for looking up or creating the block_dev_desc_t
> "handle" for a user-specified storage device, so it's exactly the place
> for this kind of object "constructor" code to execute.
>
Sorry, but now I am totally confused...
Doesn't the "block_dev_desc_t" contain the "device" information (not the 
"partition" information)?
Isn't it only created once (effectively the first time "mmc_init" is 
called on that device)?
So when I'm doing a block_read from the Boot1 partition, followed by a 
block_read from the User partition, I don't expect to see a 
"constructor" being executed (from a get_dev() or anything else...)

>> mmc->block_dev.block_read(0, 0, 1, buf);  /* read first block (from
>> Boot1 partition) */
>
> Yes.
>
>> mmc->block_dev.specified_hw_part = 0;      /* specify User partition */
>> mmc->block_dev.block_read(0, 0, 1, buf);  /* read first block (from User
>> partition) */
>>
>> I don't think this is a good idea...
>
> Oh, but it is:-)
>
Stephen Warren May 30, 2014, 3:58 p.m. | #10
On 05/29/2014 04:03 PM, Steve Rae wrote:
> 
> 
> On 14-05-29 01:30 PM, Stephen Warren wrote:
>> On 05/29/2014 01:44 PM, Steve Rae wrote:
>>>
>>>
>>> On 14-05-29 11:51 AM, Stephen Warren wrote:
>>>> On 05/29/2014 11:58 AM, Steve Rae wrote:
>>>>> Hi, Stephen
>>>>>
>>>>> On 14-05-29 09:25 AM, Stephen Warren wrote:
>>>>>> On 05/28/2014 04:15 PM, 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.
>>>>>>
>>>>>> This feels wrong; why wouldn't mmc_get_dev() return a
>>>>>> block_dev_desc_t
>>>>>> containing block_read/block_write functions that do the HW partition
>>>>>> switching. That way, this is all completely hidden, and all client
>>>>>> code
>>>>>> only knows about block devices, rather than having to know about
>>>>>> MMC-specific mmc_block_read/write/erase_hwpart() functions.
>>>>>>
>>>>> This goes back to the initial discussion on this mailing list
>>>>> (which was
>>>>> never resolved):
>>>>>     http://lists.denx.de/pipermail/u-boot/2014-April/178171.html
>>>>> This issue is that the three callback functions defined in
>>>>> 'block_desc_t' do not accept the "partition number" as an argument.
>>>>> It was suggested that we could overwrite those functions; but the
>>>>> "partition number" still needs to be passed in by:
>>>>> (1) overloading the "int dev_num" argument, or
>>>>> (2) adding another argument to the callback functions
>>>>> I assumed that neither of these was acceptable, so I have proposed
>>>>> these
>>>>> wrappers...
>>>>
>>>> Can't the data simply be stored in the block_desc_t itself?
>>>
>>> If I understand this suggestion, are you proposing:
>>> - add an "unsigned int specified_hw_part" to the block_desc_t
>>
>> Yes.
>>
>>> Then the usage would become:
>>> mmc->block_dev.specified_hw_part = 1;      /* specify Boot1 partition */
>>
>> The only code that would need to assign that field is
>> disk/part.c:get_dev() or something called from it. that is the function
>> that's responsible for looking up or creating the block_dev_desc_t
>> "handle" for a user-specified storage device, so it's exactly the place
>> for this kind of object "constructor" code to execute.
>>
> Sorry, but now I am totally confused...
> Doesn't the "block_dev_desc_t" contain the "device" information (not the
> "partition" information)?

The eMMC HW partitions are separate block devices. So,
get_device_and_partition() returns a block device that represents one of:

a) eMMC "boot0" HW partition
b) eMMC "boot1" HW partition
c) eMMC "main data/user area" HW partition

These HW partitions are entirely separate from (MBR/GPT) SW partitions,
even though both are referred to as "partitions". That's why I call the
former "HW partitions" rather than "partitions".

> Isn't it only created once (effectively the first time "mmc_init" is
> called on that device)?

The block_dev_desc_t initialization/creation does call mmc_init, yes...

> So when I'm doing a block_read from the Boot1 partition, followed by a
> block_read from the User partition, I don't expect to see a
> "constructor" being executed (from a get_dev() or anything else...)

Most U-Boot commands take a single device name (e.g. "mmc 0") and act
just on that. If you want to do something on different block devices,
you'd need to run separate commands, or perhaps have one command take a
list of devices, and initialize each one in turn. What code are you
looking at that handles multiple devices sequentially under program
control rather than user command control?
Steve Rae May 30, 2014, 4:56 p.m. | #11
On 14-05-30 08:58 AM, Stephen Warren wrote:
> On 05/29/2014 04:03 PM, Steve Rae wrote:
>>
>>
>> On 14-05-29 01:30 PM, Stephen Warren wrote:
>>> On 05/29/2014 01:44 PM, Steve Rae wrote:
>>>>
>>>>
>>>> On 14-05-29 11:51 AM, Stephen Warren wrote:
>>>>> On 05/29/2014 11:58 AM, Steve Rae wrote:
>>>>>> Hi, Stephen
>>>>>>
>>>>>> On 14-05-29 09:25 AM, Stephen Warren wrote:
>>>>>>> On 05/28/2014 04:15 PM, 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.
>>>>>>>
>>>>>>> This feels wrong; why wouldn't mmc_get_dev() return a
>>>>>>> block_dev_desc_t
>>>>>>> containing block_read/block_write functions that do the HW partition
>>>>>>> switching. That way, this is all completely hidden, and all client
>>>>>>> code
>>>>>>> only knows about block devices, rather than having to know about
>>>>>>> MMC-specific mmc_block_read/write/erase_hwpart() functions.
>>>>>>>
>>>>>> This goes back to the initial discussion on this mailing list
>>>>>> (which was
>>>>>> never resolved):
>>>>>>      http://lists.denx.de/pipermail/u-boot/2014-April/178171.html
>>>>>> This issue is that the three callback functions defined in
>>>>>> 'block_desc_t' do not accept the "partition number" as an argument.
>>>>>> It was suggested that we could overwrite those functions; but the
>>>>>> "partition number" still needs to be passed in by:
>>>>>> (1) overloading the "int dev_num" argument, or
>>>>>> (2) adding another argument to the callback functions
>>>>>> I assumed that neither of these was acceptable, so I have proposed
>>>>>> these
>>>>>> wrappers...
>>>>>
>>>>> Can't the data simply be stored in the block_desc_t itself?
>>>>
>>>> If I understand this suggestion, are you proposing:
>>>> - add an "unsigned int specified_hw_part" to the block_desc_t
>>>
>>> Yes.
>>>
>>>> Then the usage would become:
>>>> mmc->block_dev.specified_hw_part = 1;      /* specify Boot1 partition */
>>>
>>> The only code that would need to assign that field is
>>> disk/part.c:get_dev() or something called from it. that is the function
>>> that's responsible for looking up or creating the block_dev_desc_t
>>> "handle" for a user-specified storage device, so it's exactly the place
>>> for this kind of object "constructor" code to execute.
>>>
>> Sorry, but now I am totally confused...
>> Doesn't the "block_dev_desc_t" contain the "device" information (not the
>> "partition" information)?
>
> The eMMC HW partitions are separate block devices. So,
> get_device_and_partition() returns a block device that represents one of:
>
> a) eMMC "boot0" HW partition
> b) eMMC "boot1" HW partition
> c) eMMC "main data/user area" HW partition
>
> These HW partitions are entirely separate from (MBR/GPT) SW partitions,
> even though both are referred to as "partitions". That's why I call the
> former "HW partitions" rather than "partitions".
>
Agree -- and sometimes called "physical partitions"

>> Isn't it only created once (effectively the first time "mmc_init" is
>> called on that device)?
>
> The block_dev_desc_t initialization/creation does call mmc_init, yes...
>
>> So when I'm doing a block_read from the Boot1 partition, followed by a
>> block_read from the User partition, I don't expect to see a
>> "constructor" being executed (from a get_dev() or anything else...)
>
> Most U-Boot commands take a single device name (e.g. "mmc 0") and act
> just on that. If you want to do something on different block devices,
> you'd need to run separate commands, or perhaps have one command take a
> list of devices, and initialize each one in turn.
Agree - and can switch to different HW partitions with the existing "mmc 
dev 0 1" command

  What code are you
> looking at that handles multiple devices sequentially under program
> control rather than user command control?
>
Cannot go into too much detail here (yet) -- but imagine the situation 
where:
- lookup the GPT partition name (in User, Boot1, Boot2)
- do a block_write to the desired location...

So after discussing with a colleague, we would propose the following. 
Does this implement what you were proposing?:


Usage (example):

mmc->part_num_next_block_op = 1;          /* specify Boot1 partition */
mmc->block_dev.block_read(0, 0, 1, buf);  /* read first block from Boot1 
partition */
mmc->part_num_next_block_op = 0;          /* specify User partition */
mmc->block_dev.block_read(0, 0, 1, buf);  /* read first block from User 
partition */

Implementation:
(1) The mmc->part_num_next_block_op needs to be initialized to -1.
(2) Each existing mmc_{bread,bwrite,berase} function needs modification:
if 0 <= mmc->part_num_next_block_op && mmc->part_num != 
mmc->part_num_next_block_op
     switch
     if switch failed
         mmc->part_num_next_block_op = -1
         return error
[... the original code ...]
if 0 <= mmc->part_num_next_block_op && mmc->part_num != 
mmc->part_num_next_block_op
     switch_back
     if switch_back failed
         mmc->part_num = mmc->part_num_next_block_op
     mmc->part_num_next_block_op = -1


Many Thanks, Steve
Stephen Warren May 30, 2014, 5:07 p.m. | #12
On 05/30/2014 10:56 AM, Steve Rae wrote:
> On 14-05-30 08:58 AM, Stephen Warren wrote:
...
>> What code are you
>> looking at that handles multiple devices sequentially under program
>> control rather than user command control?
>
> Cannot go into too much detail here (yet) -- but imagine the situation
> where:
> - lookup the GPT partition name (in User, Boot1, Boot2)
> - do a block_write to the desired location...

So this is all to support some non-upstream code that you can't discuss?
That doesn't sound good...

> So after discussing with a colleague, we would propose the following.
> Does this implement what you were proposing?:
> 
> 
> Usage (example):
> 
> mmc->part_num_next_block_op = 1;          /* specify Boot1 partition */
> mmc->block_dev.block_read(0, 0, 1, buf);  /* read first block from Boot1
> partition */
> mmc->part_num_next_block_op = 0;          /* specify User partition */
> mmc->block_dev.block_read(0, 0, 1, buf);  /* read first block from User
> partition */

No. I would propose:

get_device("mmc", "0.1", &bdev_boot1);
bdev_boot1->block_read(...);
get_device("mmc", "0.0", &bdev_user);
bdev_user->block_read(...);

That way, nothing needs to change in block_dev_desc_t, get_device(), etc.
Steve Rae May 30, 2014, 6:39 p.m. | #13
On 14-05-30 10:07 AM, Stephen Warren wrote:
> On 05/30/2014 10:56 AM, Steve Rae wrote:
>> On 14-05-30 08:58 AM, Stephen Warren wrote:
> ...
>>> What code are you
>>> looking at that handles multiple devices sequentially under program
>>> control rather than user command control?
>>
>> Cannot go into too much detail here (yet) -- but imagine the situation
>> where:
>> - lookup the GPT partition name (in User, Boot1, Boot2)
>> - do a block_write to the desired location...
>
> So this is all to support some non-upstream code that you can't discuss?
> That doesn't sound good...
>
>> So after discussing with a colleague, we would propose the following.
>> Does this implement what you were proposing?:
>>
>>
>> Usage (example):
>>
>> mmc->part_num_next_block_op = 1;          /* specify Boot1 partition */
>> mmc->block_dev.block_read(0, 0, 1, buf);  /* read first block from Boot1
>> partition */
>> mmc->part_num_next_block_op = 0;          /* specify User partition */
>> mmc->block_dev.block_read(0, 0, 1, buf);  /* read first block from User
>> partition */
>
> No. I would propose:
>
> get_device("mmc", "0.1", &bdev_boot1);
> bdev_boot1->block_read(...);
> get_device("mmc", "0.0", &bdev_user);
> bdev_user->block_read(...);
>
> That way, nothing needs to change in block_dev_desc_t, get_device(), etc.
>
OK -- I can make this work...
Therefore, abandoning this change.
Many Thanks, Steve
Pantelis Antoniou June 2, 2014, 6:42 a.m. | #14
Hi Steve,

I wanted the discussion to settle a bit before I reply to this series.
On May 29, 2014, at 1: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>
> ---

[snip]

> /**
>  * Start device initialization and return immediately; it does not block on
>  * polling OCR (operation condition register) status.  Then you should call
> -- 
> 1.8.5
> 

The thing IMO should be modeled in the same way that block devices work in
Linux.

TBH I'm not very fond of the way block devices/partitions and the block_ops
are intermixed in block_dev_t. This part of code could use some refactoring
to make it operate more like a regular linux block device (with each partition
being it's own block device), but I don't know if we have enough votes to change
it ATM.

I'm willing to pick up any MMC related patches to get something workable (Stephen's
approach is fine), but I also want to ask if there's any interest in fixing up the 
block dev mess.

Regards

-- Pantelis
Stephen Warren June 2, 2014, 4:30 p.m. | #15
On 06/02/2014 12:42 AM, Pantelis Antoniou wrote:
> Hi Steve,
> 
> I wanted the discussion to settle a bit before I reply to this series.
> On May 29, 2014, at 1: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>
>> ---
> 
> [snip]
> 
>> /**
>>  * Start device initialization and return immediately; it does not block on
>>  * polling OCR (operation condition register) status.  Then you should call
>> -- 
>> 1.8.5
>>
> 
> The thing IMO should be modeled in the same way that block devices work in
> Linux.
> 
> TBH I'm not very fond of the way block devices/partitions and the block_ops
> are intermixed in block_dev_t. This part of code could use some refactoring
> to make it operate more like a regular linux block device (with each partition
> being it's own block device), but I don't know if we have enough votes to change
> it ATM.

Refactoring that would make sense to me. That way, any client code could
just pass the user's command-line (or whatever) parameters to some
lookup function, which could return something that accesses whatever the
user wants, without the code that accesses the data caring whether it's
a complete block device, a complete HW partition, or a SW partition
within one of those. Of course, I guess that's already the case, it's
just that the information is split across block_dev_desc_t and
disk_partition_t, when it doesn't really need to be.

Patch

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;
+	rc = mmc->block_dev.block_read(dev, start, blkcnt, buffer);
+	switch_part(mmc, dev, part_num, mmc->part_num);
+
+	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;
+	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;
+	struct mmc *mmc = find_mmc_device(dev);
+
+	if (switch_part(mmc, dev, part_num, part_num))
+		return -1;
+	rc = mmc->block_dev.block_erase(dev, start, blkcnt);
+	switch_part(mmc, dev, part_num, mmc->part_num);
+
+	return rc;
+}
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