Re: [U-Boot] [PATCH] mmc: add wrappers for MMC block_{read, write, erase}
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 s...@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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc: add wrappers for MMC block_{read, write, erase}
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 s...@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. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc: add wrappers for MMC block_{read, write, erase}
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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc: add wrappers for MMC block_{read, write, erase}
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 s...@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 swar...@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 000..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,
+
Re: [U-Boot] [PATCH] mmc: add wrappers for MMC block_{read, write, erase}
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:-) ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc: add wrappers for MMC block_{read, write, erase}
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? ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc: add wrappers for MMC block_{read, write, erase}
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:-) ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc: add wrappers for MMC block_{read, write, erase}
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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc: add wrappers for MMC block_{read, write, erase}
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 s...@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 swar...@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 000..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
Re: [U-Boot] [PATCH] mmc: add wrappers for MMC block_{read, write, erase}
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 s...@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 swar...@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 000..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);
+
Re: [U-Boot] [PATCH] mmc: add wrappers for MMC block_{read, write, erase}
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. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc: add wrappers for MMC block_{read, write, erase}
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? ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc: add wrappers for MMC block_{read, write, erase}
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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc: add wrappers for MMC block_{read, write, erase}
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. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] mmc: add wrappers for MMC block_{read, write, erase}
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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] 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.
Signed-off-by: Steve Rae s...@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 swar...@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 000..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
