| Message ID | 1401315346-30231-1-git-send-email-srae@broadcom.com |
|---|---|
| State | New |
| Headers | show |
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 >
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 >
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.
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 >> >
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
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?
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
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:-)
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:-) >
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?
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
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.
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
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
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.
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
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