mmc: add wrappers for MMC block_{read, write, erase}
PATCH 1/1 mmc: add wrappers for MMC block_{read, write, erase}
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.
---
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 <[email protected]>
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
drivers/mmc/Makefile +1 -0
@@ -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
drivers/mmc/mmc_hwpart.c new file +75 -0
75 lines not on this page · raw message
include/mmc.h +10 -0
@@ -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
Signature · 1 line
1.8.5
PATCH 1/1 mmc: add wrappers for MMC block_{read, write, erase}
Hi, Steve.
Quoted 67 lines · Steve Rae
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 <[email protected]>---
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 <[email protected]>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.cdrivers/mmc/Makefile +1
-0@@ -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.odrivers/mmc/mmc_hwpart.c new file +27
-0@@ -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,
drivers/mmc/mmc_hwpart.c line 37
Quoted 10 lines
drivers/mmc/mmc_hwpart.c new file +10
-0+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?
drivers/mmc/mmc_hwpart.c line 39
Quoted 2 lines
drivers/mmc/mmc_hwpart.c new file +2
-0+ 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?
Quoted 5 lines
drivers/mmc/mmc_hwpart.c new file +5
-0+ +return rc; +} + +unsigned long mmc_block_write_hwpart(int dev,
drivers/mmc/mmc_hwpart.c line 54
Quoted 10 lines
drivers/mmc/mmc_hwpart.c new file +10
-0+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..
drivers/mmc/mmc_hwpart.c line 66
Quoted 12 lines
drivers/mmc/mmc_hwpart.c new file +12
-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 ;
Why did you assign to "-1"?
drivers/mmc/mmc_hwpart.c line 70
Quoted 4 lines
drivers/mmc/mmc_hwpart.c new file +4
-0+struct mmc *mmc =find_mmc_device (dev); + +if (switch_part (mmc, dev, part_num, part_num)) +return -1 ;
At here, return -1?
Quoted 27 lines
drivers/mmc/mmc_hwpart.c new file +5
-0+ rc = mmc->block_dev.block_erase (dev, start, blkcnt); +switch_part (mmc, dev, part_num, mmc->part_num); + +return rc; +}include/mmc.h +10
-0@@ -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
PATCH 1/1 mmc: add wrappers for MMC block_{read, write, erase}
In reply to Steve RaeHi, Steve.
Quoted 67 lines · Steve Rae
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 <[email protected]>---
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 <[email protected]>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.cdrivers/mmc/Makefile +1
-0@@ -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.odrivers/mmc/mmc_hwpart.c new file +27
-0@@ -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,
drivers/mmc/mmc_hwpart.c line 37
Quoted 10 lines
drivers/mmc/mmc_hwpart.c new file +10
-0+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?
drivers/mmc/mmc_hwpart.c line 39
Quoted 2 lines
drivers/mmc/mmc_hwpart.c new file +2
-0+ 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?
Quoted 5 lines
drivers/mmc/mmc_hwpart.c new file +5
-0+ +return rc; +} + +unsigned long mmc_block_write_hwpart(int dev,
drivers/mmc/mmc_hwpart.c line 54
Quoted 10 lines
drivers/mmc/mmc_hwpart.c new file +10
-0+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..
drivers/mmc/mmc_hwpart.c line 66
Quoted 12 lines
drivers/mmc/mmc_hwpart.c new file +12
-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 ;
Why did you assign to "-1"?
drivers/mmc/mmc_hwpart.c line 70
Quoted 4 lines
drivers/mmc/mmc_hwpart.c new file +4
-0+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
Quoted 27 lines
drivers/mmc/mmc_hwpart.c new file +5
-0+ rc = mmc->block_dev.block_erase (dev, start, blkcnt); +switch_part (mmc, dev, part_num, mmc->part_num); + +return rc; +}include/mmc.h +10
-0@@ -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
PATCH 1/1 mmc: add wrappers for MMC block_{read, write, erase}
Hi, Jaehoon
Quoted 73 lines · Jaehoon Chung
Hi, Steve.
Quoted 70 lines · Steve Rae
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 <[email protected]>---
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 <[email protected]>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.cdrivers/mmc/Makefile +1
-0@@ -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.odrivers/mmc/mmc_hwpart.c new file +30
-0@@ -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,
Quoted 9 lines
drivers/mmc/mmc_hwpart.c line 37
Quoted 7 lines
drivers/mmc/mmc_hwpart.c new file +7
-0+ 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)
Quoted 5 lines
drivers/mmc/mmc_hwpart.c line 39
Quoted 2 lines
drivers/mmc/mmc_hwpart.c new file +2
-0+ 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_readis 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)
Quoted 9 lines
Quoted 9 lines
drivers/mmc/mmc_hwpart.c new file +9
-0+ +return rc; +} + +unsigned long mmc_block_write_hwpart (int dev, +unsigned int part_num, +lbaint_t start, +lbaint_t blkcnt, +const void *buffer)
Quoted 9 lines
drivers/mmc/mmc_hwpart.c line 37
Quoted 6 lines
drivers/mmc/mmc_hwpart.c new file +6
-0+{ +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)
Quoted 5 lines
Quoted 5 lines
drivers/mmc/mmc_hwpart.c new file +5
-0+ rc = mmc->block_dev.block_write (dev, start, blkcnt, buffer); +switch_part (mmc, dev, part_num, mmc->part_num); + +return rc; +}
Quoted 9 lines
drivers/mmc/mmc_hwpart.c line 66
Quoted 7 lines
drivers/mmc/mmc_hwpart.c new file +7
-0+ +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
Quoted 7 lines
drivers/mmc/mmc_hwpart.c line 70
Quoted 5 lines
drivers/mmc/mmc_hwpart.c new file +4
-0+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)
Quoted 30 lines
Best Regards,
Jaehoon ChungQuoted 27 lines
drivers/mmc/mmc_hwpart.c new file +5
-0+ rc = mmc->block_dev.block_erase (dev, start, blkcnt); +switch_part (mmc, dev, part_num, mmc->part_num); + + return rc;otherwise +}include/mmc.h +10
-0@@ -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
PATCH 1/1 mmc: add wrappers for MMC block_{read, write, erase}
In reply to Steve RaeQuoted 6 lines · Steve Rae
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.
PATCH 1/1 mmc: add wrappers for MMC block_{read, write, erase}
Hi, Stephen
Quoted 8 lines · Stephen Warren
Quoted 6 lines · Steve Rae
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.
Quoted 6 lines
This feels wrong; why wouldn't
mmc_get_dev()return ablock_dev_desc_tcontaining 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
PATCH 1/1 mmc: add wrappers for MMC block_{read, write, erase}
Quoted 17 lines · Steve Rae
Hi, Stephen
Quoted 8 lines · Stephen Warren
Quoted 6 lines · Steve Rae
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.Quoted 6 lines
This feels wrong; why wouldn't
mmc_get_dev()return ablock_dev_desc_tcontaining 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.
Quoted 11 lines
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 "intdev_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?
PATCH 1/1 mmc: add wrappers for MMC block_{read, write, erase}
Quoted 30 lines · Stephen Warren
Quoted 20 lines · Steve Rae
Hi, Stephen
Quoted 8 lines · Stephen Warren
Quoted 6 lines · Steve Rae
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) is0=User, 1=Boot1, 2=Boot2, etc.Quoted 6 lines
This feels wrong; why wouldn't
mmc_get_dev()return ablock_dev_desc_tcontaining 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.htmlQuoted 8 lines
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 "intdev_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...
Quoted 2 lines
Can't the data simply be stored in the
block_desc_titself?
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
PATCH 1/1 mmc: add wrappers for MMC block_{read, write, erase}
Quoted 33 lines · Steve Rae
Quoted 30 lines · Stephen Warren
Quoted 28 lines · Steve Rae
Hi, Stephen
Quoted 8 lines · Stephen Warren
Quoted 6 lines · Steve Rae
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) is0=User, 1=Boot1, 2=Boot2, etc.Quoted 6 lines
This feels wrong; why wouldn't
mmc_get_dev()return ablock_dev_desc_tcontaining 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.htmlThis 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 "intdev_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...
Quoted 4 lines
Quoted 1 line
Can't the data simply be stored in the
block_desc_titself?If I understand this suggestion, are you proposing:
- add an "unsigned intspecified_hw_part" to theblock_desc_t
Yes.
Quoted 2 lines
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.
Quoted 2 lines
mmc->block_dev.block_read(0, 0, 1, buf); /* read first block (from Boot1 partition) */
Yes.
Quoted 5 lines
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:-)
PATCH 1/1 mmc: add wrappers for MMC block_{read, write, erase}
Quoted 41 lines · Stephen Warren
Quoted 33 lines · Steve Rae
Quoted 30 lines · Stephen Warren
Quoted 28 lines · Steve Rae
Hi, Stephen
Quoted 8 lines · Stephen Warren
Quoted 6 lines · Steve Rae
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) is0=User, 1=Boot1, 2=Boot2, etc.Quoted 6 lines
This feels wrong; why wouldn't
mmc_get_dev()return ablock_dev_desc_tcontaining 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.htmlThis 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 "intdev_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...Quoted 4 lines
Quoted 1 line
Can't the data simply be stored in the
block_desc_titself?If I understand this suggestion, are you proposing:
- add an "unsigned intspecified_hw_part" to theblock_desc_tYes.
Quoted 9 lines
Quoted 2 lines
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...)
Quoted 13 lines
Quoted 2 lines
mmc->block_dev.block_read(0, 0, 1, buf); /* read first block (from Boot1 partition) */Yes.
Quoted 5 lines
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:-)
PATCH 1/1 mmc: add wrappers for MMC block_{read, write, erase}
Quoted 57 lines · Steve Rae
Quoted 45 lines · Stephen Warren
Quoted 37 lines · Steve Rae
Quoted 34 lines · Stephen Warren
Quoted 32 lines · Steve Rae
Hi, Stephen
Quoted 8 lines · Stephen Warren
Quoted 6 lines · Steve Rae
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) is0=User, 1=Boot1, 2=Boot2, etc.Quoted 8 lines
This feels wrong; why wouldn't
mmc_get_dev()return ablock_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.htmlThis 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 "intdev_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...Quoted 4 lines
Quoted 1 line
Can't the data simply be stored in the
block_desc_titself?If I understand this suggestion, are you proposing:
- add an "unsigned intspecified_hw_part" to theblock_desc_tYes.
Quoted 9 lines
Quoted 2 lines
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.
Quoted 3 lines
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".
Quoted 2 lines
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...
Quoted 3 lines
So when I'm doing a
block_readfrom the Boot1 partition, followed by ablock_readfrom the User partition, I don't expect to see a "constructor" being executed (from aget_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?
PATCH 1/1 mmc: add wrappers for MMC block_{read, write, erase}
Quoted 65 lines · Stephen Warren
Quoted 57 lines · Steve Rae
Quoted 45 lines · Stephen Warren
Quoted 37 lines · Steve Rae
Quoted 34 lines · Stephen Warren
Quoted 32 lines · Steve Rae
Hi, Stephen
Quoted 8 lines · Stephen Warren
Quoted 6 lines · Steve Rae
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) is0=User, 1=Boot1, 2=Boot2, etc.Quoted 8 lines
This feels wrong; why wouldn't
mmc_get_dev()return ablock_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.htmlThis 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 "intdev_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...Quoted 4 lines
Quoted 1 line
Can't the data simply be stored in the
block_desc_titself?If I understand this suggestion, are you proposing:
- add an "unsigned intspecified_hw_part" to theblock_desc_tYes.
Quoted 9 lines
Quoted 2 lines
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.Quoted 3 lines
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:
Quoted 8 lines
a) eMMC "boot0" HW partition
b) eMMC "boot1" HW partition
c) eMMC "main data/user area" HW partitionThese 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"
Quoted 5 lines
Quoted 2 lines
Isn't it only created once (effectively the first time "
mmc_init" is called on that device)?The
block_dev_desc_tinitialization/creation does callmmc_init, yes...
Quoted 8 lines
Quoted 3 lines
So when I'm doing a
block_readfrom the Boot1 partition, followed by ablock_readfrom the User partition, I don't expect to see a "constructor" being executed (from aget_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
Quoted 3 lines
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 = -1Many Thanks, Steve
PATCH 1/1 mmc: add wrappers for MMC block_{read, write, erase}
Quoted 1 line · Steve Rae
On 14-05-30 08:58 AM, Stephen Warren wrote:
...
Quoted 8 lines
Quoted 3 lines
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 ablock_writeto the desired location...
So this is all to support some non-upstream code that you can't discuss? That doesn't sound good...
Quoted 4 lines
So after discussing with a colleague, we would propose the following. Does this implement what you were proposing?:
Quoted 8 lines
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.
PATCH 1/1 mmc: add wrappers for MMC block_{read, write, erase}
Quoted 28 lines · Stephen Warren
Quoted 1 line · Steve Rae
On 14-05-30 08:58 AM, Stephen Warren wrote:
...
Quoted 8 lines
Quoted 3 lines
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 ablock_writeto the desired location...So this is all to support some non-upstream code that you can't discuss? That doesn't sound good...
Quoted 4 lines
So after discussing with a colleague, we would propose the following. Does this implement what you were proposing?:
Quoted 8 lines
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 */
Quoted 9 lines
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
PATCH 1/1 mmc: add wrappers for MMC block_{read, write, erase}
In reply to Steve RaeHi Steve,
I wanted the discussion to settle a bit before I reply to this series.
Quoted 9 lines · Steve Rae
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 <[email protected]>---
[snip]
Quoted 6 lines
/**
* Start device initialization and return immediately; it does not block on
* polling OCR (operation condition register) status. Then you should callSignature · 1 line
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
PATCH 1/1 mmc: add wrappers for MMC block_{read, write, erase}
Quoted 24 lines · Pantelis Antoniou
Hi Steve,
I wanted the discussion to settle a bit before I reply to this series.
Quoted 9 lines · Steve Rae
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 <[email protected]>---
[snip]
Quoted 6 lines
/**
* Start device initialization and return immediately; it does not block on
* polling OCR (operation condition register) status. Then you should callSignature · 1 line
1.8.5
Quoted 8 lines
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_opsare intermixed inblock_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.