* [PATCH 00/10] mmc: add SD/eMMC erase support
@ 2024-07-30 7:19 Ahmad Fatoum
2024-07-30 7:19 ` [PATCH 01/10] fs: give erase() a new erase_type parameter Ahmad Fatoum
` (9 more replies)
0 siblings, 10 replies; 29+ messages in thread
From: Ahmad Fatoum @ 2024-07-30 7:19 UTC (permalink / raw)
To: barebox
A common optimization for flashing is to skip gaps between partitions
and only flash actual data. The problem with that is that data from
the previous installation may persist and confuse later software, e.g.
an existing barebox state partition not contained in the image may
survive, when the expectation is that a new state is created.
eMMCs can support three different commands for erasing data:
- Erase: This has the widest support, but works on only on the level
of erase groups that may span multiple write blocks.
The region reads as either '0' or '1' afterwards.
- Trim: New in eMMC v4.4. This erases write blocks.
The region reads as either '0' or '1' afterwards.
- Discard: New in eMMC v4.5. This discards write blocks. It's up to the
card what action if any is taken.
All or part of the data may remain accessible.
All of these don't enforce a actual physical NAND erase operation.
In case erasure does happen, it may happen later at a convenient time.
All of them, except for discard guarantee that a fixed pattern (either
all zeroes or all ones) will be read back after the erase operation
concludes. Therefore let's use them in barebox to implement the erase
command.
The behavior of the erase command will be similar to the Linux
blkdiscard -z command when the erase byte value is all-zeroes. In Linux
blkdiscard -z is emulated with zero writes if the erased byte is 0xFF,
but for barebox, the erase operation won't care whether it writes 0x00
or 0xFF.
Note that this is considerably slower than need be, because we don't
erase multiple erase groups at once. Doing so could run into the
send_cmd timeout of the host hardware or its drivers. The correct
workaround is to calculate the duration of the erase operation and split
up the erases, so we always stay below a specific erase operation
duration. There's a comment that explains this and implementing it is
left as future exercise.
Ahmad Fatoum (10):
fs: give erase() a new erase_type parameter
block: factor out chunk_flush helper
block: allow block devices to implement the cdev erase operation
mci: turn bool members into bitfield in struct mci
mci: describe more command structure in mci.h
mci: core: use CONFIG_MCI_WRITE, not CONFIG_BLOCK_WRITE
mci: add support for discarding write blocks
commands: sync: add new command to flush cached writes
mci: core: remove reference to SD Card from common mci_card_probe
commands: blkstats: add command to print block device statistics
arch/arm/mach-imx/imx-bbu-external-nand.c | 2 +-
arch/arm/mach-imx/imx-bbu-internal.c | 4 +-
arch/arm/mach-omap/am33xx_bbu_spi_mlo.c | 2 +-
commands/Kconfig | 25 ++
commands/Makefile | 2 +
commands/blkstats.c | 60 ++++
commands/flash.c | 2 +-
commands/nandtest.c | 2 +-
commands/sync.c | 42 +++
common/Kconfig | 3 +
common/bbu.c | 2 +-
common/block.c | 123 +++++--
common/environment.c | 7 +-
common/fastboot.c | 2 +-
common/state/backend_bucket_circular.c | 2 +-
drivers/mci/Kconfig | 5 +
drivers/mci/mci-core.c | 405 ++++++++++++++++++++--
drivers/misc/storage-by-uuid.c | 3 +
drivers/usb/gadget/function/dfu.c | 4 +-
fs/devfs-core.c | 7 +-
fs/devfs.c | 5 +-
fs/fs.c | 4 +-
include/block.h | 11 +
include/driver.h | 13 +
include/fs.h | 26 +-
include/mci.h | 75 +++-
lib/libfile.c | 2 +-
27 files changed, 767 insertions(+), 73 deletions(-)
create mode 100644 commands/blkstats.c
create mode 100644 commands/sync.c
--
2.39.2
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 01/10] fs: give erase() a new erase_type parameter
2024-07-30 7:19 [PATCH 00/10] mmc: add SD/eMMC erase support Ahmad Fatoum
@ 2024-07-30 7:19 ` Ahmad Fatoum
2024-07-30 8:31 ` Sascha Hauer
2024-07-30 7:19 ` [PATCH 02/10] block: factor out chunk_flush helper Ahmad Fatoum
` (8 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Ahmad Fatoum @ 2024-07-30 7:19 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
Many managed flashes provide an erase operation, which need not be used
for regular writes, but is provided nonetheless for efficient clearing
of large regions of storage to a fixed bit pattern. The erase type allows
users to specify _why_ the erase operation is done, so the driver or
the core code can choose the most optimal operation.
Existing code is annotated wither either ERASE_TO_WRITE for code that is
immediately followed by a write and ERASE_TO_CLEAR for the standalone
erase operations of the shell and fastboot erase command.
Use of an enum allows the future addition of more types, e.g.:
ERASE_TO_ALL_ZERO: to efficiently implement android sparse all-ones
(like util-linux blkdiscard --zeroout)
ERASE_TO_ALL_ONE: to efficiently implement android sparse all-zeroes
ERASE_TO_DISCARD: as alternative to ERASE_TO_CLEAR, when unwritten
regions are indeed allowed to have arbitrary content
(like util-linux blkdiscard without argument)
ERASE_TO_SECURE: erase data in a manner appropriate for wiping secrets
(like util-linux blkdiscard --secure)
We don't introduce any functional change here yet, as no device yet sets
DEVFS_WRITE_AUTOERASE. This will follow in a separate commit.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
arch/arm/mach-imx/imx-bbu-external-nand.c | 2 +-
arch/arm/mach-imx/imx-bbu-internal.c | 4 ++--
arch/arm/mach-omap/am33xx_bbu_spi_mlo.c | 2 +-
commands/flash.c | 2 +-
commands/nandtest.c | 2 +-
common/bbu.c | 2 +-
common/environment.c | 7 ++++--
common/fastboot.c | 2 +-
common/state/backend_bucket_circular.c | 2 +-
drivers/misc/storage-by-uuid.c | 3 +++
drivers/usb/gadget/function/dfu.c | 4 ++--
fs/devfs-core.c | 7 +++++-
fs/devfs.c | 5 ++++-
fs/fs.c | 4 ++--
include/driver.h | 13 ++++++++++++
include/fs.h | 26 +++++++++++++++++++++--
lib/libfile.c | 2 +-
17 files changed, 69 insertions(+), 20 deletions(-)
diff --git a/arch/arm/mach-imx/imx-bbu-external-nand.c b/arch/arm/mach-imx/imx-bbu-external-nand.c
index 7523008cdb5a..72c2271ff37a 100644
--- a/arch/arm/mach-imx/imx-bbu-external-nand.c
+++ b/arch/arm/mach-imx/imx-bbu-external-nand.c
@@ -153,7 +153,7 @@ static int imx_bbu_external_nand_update(struct bbu_handler *handler, struct bbu_
debug("writing %d bytes at 0x%08llx\n", now, nand_offset);
- ret = erase(fd, blocksize, nand_offset);
+ ret = erase(fd, blocksize, nand_offset, ERASE_TO_WRITE);
if (ret)
goto out;
diff --git a/arch/arm/mach-imx/imx-bbu-internal.c b/arch/arm/mach-imx/imx-bbu-internal.c
index 8cdaab5c1676..4190d60d18dd 100644
--- a/arch/arm/mach-imx/imx-bbu-internal.c
+++ b/arch/arm/mach-imx/imx-bbu-internal.c
@@ -108,7 +108,7 @@ static int imx_bbu_write_device(struct imx_internal_bbu_handler *imx_handler,
if (imx_bbu_erase_required(imx_handler)) {
pr_debug("%s: erasing %s from 0x%08x to 0x%08x\n", __func__,
devicefile, offset, image_len);
- ret = erase(fd, image_len, offset);
+ ret = erase(fd, image_len, offset, ERASE_TO_WRITE);
if (ret) {
pr_err("erasing %s failed with %s\n", devicefile,
strerror(-ret));
@@ -340,7 +340,7 @@ static int imx_bbu_internal_v2_write_nand_dbbt(struct imx_internal_bbu_handler *
pr_debug("writing %d bytes at 0x%08llx\n", now, offset);
- ret = erase(fd, blocksize, offset);
+ ret = erase(fd, blocksize, offset, ERASE_TO_WRITE);
if (ret)
goto out;
diff --git a/arch/arm/mach-omap/am33xx_bbu_spi_mlo.c b/arch/arm/mach-omap/am33xx_bbu_spi_mlo.c
index 2c58c9ae690e..951c3f8ce099 100644
--- a/arch/arm/mach-omap/am33xx_bbu_spi_mlo.c
+++ b/arch/arm/mach-omap/am33xx_bbu_spi_mlo.c
@@ -71,7 +71,7 @@ static int spi_nor_mlo_handler(struct bbu_handler *handler,
goto out;
}
- ret = erase(dstfd, ERASE_SIZE_ALL, 0);
+ ret = erase(dstfd, ERASE_SIZE_ALL, 0, ERASE_TO_WRITE);
if (ret < 0) {
printf("could not erase %s: %m", data->devicefile);
goto out1;
diff --git a/commands/flash.c b/commands/flash.c
index 5b7060aeadfb..de3bf65e5059 100644
--- a/commands/flash.c
+++ b/commands/flash.c
@@ -43,7 +43,7 @@ static int do_flerase(int argc, char *argv[])
goto out;
}
- if (erase(fd, size, start)) {
+ if (erase(fd, size, start, ERASE_TO_CLEAR)) {
perror("erase");
ret = 1;
}
diff --git a/commands/nandtest.c b/commands/nandtest.c
index ba7c87a32eea..bc138646a460 100644
--- a/commands/nandtest.c
+++ b/commands/nandtest.c
@@ -160,7 +160,7 @@ static int erase_and_write(loff_t ofs, unsigned char *data,
er.start = ofs;
er.length = meminfo.erasesize;
- ret = erase(fd, er.length, er.start);
+ ret = erase(fd, er.length, er.start, ERASE_TO_WRITE);
if (ret < 0) {
perror("\nerase");
printf("Could't not erase flash at 0x%08llx length 0x%08llx.\n",
diff --git a/common/bbu.c b/common/bbu.c
index f71747aa2f6d..9ea8a1a3f247 100644
--- a/common/bbu.c
+++ b/common/bbu.c
@@ -453,7 +453,7 @@ int bbu_std_file_handler(struct bbu_handler *handler,
goto err_close;
}
- ret = erase(fd, data->len, 0);
+ ret = erase(fd, data->len, 0, ERASE_TO_WRITE);
if (ret && ret != -ENOSYS) {
printf("erasing %s failed with %s\n", data->devicefile,
strerror(-ret));
diff --git a/common/environment.c b/common/environment.c
index 39cad0c16a77..dbf4c2f52365 100644
--- a/common/environment.c
+++ b/common/environment.c
@@ -152,7 +152,10 @@ static inline int protect(int fd, size_t count, unsigned long offset, int prot)
return 0;
}
-static inline int erase(int fd, size_t count, unsigned long offset)
+enum erase_type {
+ ERASE_TO_WRITE
+};
+static inline int erase(int fd, size_t count, unsigned long offset, enum erase_type type)
{
return 0;
}
@@ -380,7 +383,7 @@ int envfs_save(const char *filename, const char *dirname, unsigned flags)
goto out;
}
- ret = erase(envfd, ERASE_SIZE_ALL, 0);
+ ret = erase(envfd, ERASE_SIZE_ALL, 0, ERASE_TO_WRITE);
/* ENOSYS and EOPNOTSUPP aren't errors here, many devices don't need it */
if (ret && errno != ENOSYS && errno != EOPNOTSUPP) {
diff --git a/common/fastboot.c b/common/fastboot.c
index f8a01dea7a65..d283fc8fc098 100644
--- a/common/fastboot.c
+++ b/common/fastboot.c
@@ -789,7 +789,7 @@ static void cb_erase(struct fastboot *fb, const char *cmd)
if (fd < 0)
fastboot_tx_print(fb, FASTBOOT_MSG_FAIL, strerror(-fd));
- ret = erase(fd, ERASE_SIZE_ALL, 0);
+ ret = erase(fd, ERASE_SIZE_ALL, 0, ERASE_TO_CLEAR);
close(fd);
diff --git a/common/state/backend_bucket_circular.c b/common/state/backend_bucket_circular.c
index 2ac5bf6a8218..6b5873aa9af1 100644
--- a/common/state/backend_bucket_circular.c
+++ b/common/state/backend_bucket_circular.c
@@ -220,7 +220,7 @@ static int state_mtd_peb_write(struct state_backend_storage_bucket_circular *cir
static int state_mtd_peb_erase(struct state_backend_storage_bucket_circular *circ)
{
return erase(circ->fd, circ->mtd->erasesize,
- (off_t)circ->eraseblock * circ->mtd->erasesize);
+ (off_t)circ->eraseblock * circ->mtd->erasesize, ERASE_TO_WRITE);
}
#endif
diff --git a/drivers/misc/storage-by-uuid.c b/drivers/misc/storage-by-uuid.c
index db15b64d7dcf..8b8fd901685e 100644
--- a/drivers/misc/storage-by-uuid.c
+++ b/drivers/misc/storage-by-uuid.c
@@ -126,6 +126,9 @@ static void storage_by_uuid_add_partitions(struct sbu *sbu, struct cdev *rcdev)
sbu->cdev.dev = sbu->dev;
sbu->cdev.priv = sbu;
+ if (rcdev->flags & DEVFS_WRITE_AUTOERASE)
+ sbu->cdev.flags |= DEVFS_WRITE_AUTOERASE;
+
ret = devfs_create(&sbu->cdev);
if (ret) {
dev_err(sbu->dev, "Failed to create cdev: %s\n", strerror(-ret));
diff --git a/drivers/usb/gadget/function/dfu.c b/drivers/usb/gadget/function/dfu.c
index a41ff22dcebc..208e0cd23c29 100644
--- a/drivers/usb/gadget/function/dfu.c
+++ b/drivers/usb/gadget/function/dfu.c
@@ -208,7 +208,7 @@ static void dfu_do_write(struct dfu_work *dw)
if (prog_erase && (dfu_written + wlen) > dfu_erased) {
size = roundup(wlen, dfu_mtdinfo.erasesize);
- ret = erase(dfufd, size, dfu_erased);
+ ret = erase(dfufd, size, dfu_erased, ERASE_TO_WRITE);
dfu_erased += size;
if (ret && ret != -ENOSYS) {
perror("erase");
@@ -333,7 +333,7 @@ static void dfu_do_copy(struct dfu_work *dw)
return;
}
- ret = erase(fd, ERASE_SIZE_ALL, 0);
+ ret = erase_flash(fd, ERASE_SIZE_ALL, 0);
close(fd);
if (ret && ret != -ENOSYS) {
perror("erase");
diff --git a/fs/devfs-core.c b/fs/devfs-core.c
index ed445fbd4712..2715193c6956 100644
--- a/fs/devfs-core.c
+++ b/fs/devfs-core.c
@@ -509,6 +509,7 @@ static struct cdev *__devfs_add_partition(struct cdev *cdev,
loff_t _end = end ? *end : 0;
static struct cdev *new;
struct cdev *overlap;
+ unsigned inherited_flags = 0;
if (cdev_by_name(partinfo->name))
return ERR_PTR(-EEXIST);
@@ -551,11 +552,14 @@ static struct cdev *__devfs_add_partition(struct cdev *cdev,
return overlap;
}
+ /* Filter flags that we want to pass along to children */
+ inherited_flags |= cdev->flags & DEVFS_WRITE_AUTOERASE;
+
if (IS_ENABLED(CONFIG_MTD) && cdev->mtd) {
struct mtd_info *mtd;
mtd = mtd_add_partition(cdev->mtd, offset, size,
- partinfo->flags, partinfo->name);
+ partinfo->flags | inherited_flags, partinfo->name);
if (IS_ERR(mtd))
return (void *)mtd;
@@ -571,6 +575,7 @@ static struct cdev *__devfs_add_partition(struct cdev *cdev,
new->priv = cdev->priv;
new->size = size;
new->offset = cdev->offset + offset;
+ new->flags = inherited_flags;
new->dev = cdev->dev;
new->master = cdev;
diff --git a/fs/devfs.c b/fs/devfs.c
index 9dbfa91b1d9f..5feb76cfef86 100644
--- a/fs/devfs.c
+++ b/fs/devfs.c
@@ -61,13 +61,16 @@ static int devfs_lseek(struct device *_dev, FILE *f, loff_t pos)
}
static int devfs_erase(struct device *_dev, FILE *f, loff_t count,
- loff_t offset)
+ loff_t offset, enum erase_type type)
{
struct cdev *cdev = f->priv;
if (cdev->flags & DEVFS_PARTITION_READONLY)
return -EPERM;
+ if ((type == ERASE_TO_WRITE) && (cdev->flags & DEVFS_WRITE_AUTOERASE))
+ return 0;
+
if (count + offset > cdev->size)
count = cdev->size - offset;
diff --git a/fs/fs.c b/fs/fs.c
index 656434f67ffa..5e7e05f4990a 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -600,7 +600,7 @@ loff_t lseek(int fd, loff_t offset, int whence)
}
EXPORT_SYMBOL(lseek);
-int erase(int fd, loff_t count, loff_t offset)
+int erase(int fd, loff_t count, loff_t offset, enum erase_type type)
{
struct fs_driver *fsdrv;
FILE *f = fd_to_file(fd, false);
@@ -621,7 +621,7 @@ int erase(int fd, loff_t count, loff_t offset)
assert_command_context();
if (fsdrv->erase)
- ret = fsdrv->erase(&f->fsdev->dev, f, count, offset);
+ ret = fsdrv->erase(&f->fsdev->dev, f, count, offset, type);
else
ret = -ENOSYS;
diff --git a/include/driver.h b/include/driver.h
index 2b4db2221a6c..c4067d531cc7 100644
--- a/include/driver.h
+++ b/include/driver.h
@@ -562,6 +562,19 @@ extern struct list_head cdev_list;
#define DEVFS_PARTITION_BOOTABLE_LEGACY (1U << 11)
#define DEVFS_PARTITION_BOOTABLE_ESP (1U << 12)
#define DEVFS_PARTITION_FOR_FIXUP (1U << 13)
+#define DEVFS_WRITE_AUTOERASE (1U << 14)
+
+/**
+ * cdev_write_requires_erase - Check whether writes must be done to erased blocks
+ * @cdev: pointer to cdev structure
+ *
+ * This function checks if the erase operation is required, so a subsequent
+ * write operation can write all bits.
+ */
+static inline bool cdev_write_requires_erase(const struct cdev *cdev)
+{
+ return !(cdev->flags & DEVFS_WRITE_AUTOERASE);
+}
static inline bool cdev_is_mbr_partitioned(const struct cdev *master)
{
diff --git a/include/fs.h b/include/fs.h
index f87c4c40273f..f5950a052923 100644
--- a/include/fs.h
+++ b/include/fs.h
@@ -10,6 +10,7 @@
#include <sys/stat.h>
#include <driver.h>
#include <filetype.h>
+#include <linux/bits.h>
#include <linux/fs.h>
#include <linux/string.h>
#include <linux/limits.h>
@@ -38,6 +39,27 @@ typedef struct filep {
#define FS_DRIVER_NO_DEV 1
+/**
+ * enum erase_type - Type of erase operation
+ * @ERASE_TO_WRITE: Conduct low-level erase operation to prepare for a write
+ * to all or part of the erased regions. This is required
+ * for raw flashes, but can be omitted by flashes behind
+ * a FTL that autoerases on write (e.g. eMMCs)
+ * @ERASE_TO_CLEAR: Force an erase of the region. When read afterwards,
+ * A fixed pattern should result, usually either all-zeroes
+ * or all-ones depending on storage technology.
+ *
+ * Many managed flashes provide an erase operation, which need not be used
+ * for regular writes, but is provided nonetheless for efficient clearing
+ * of large regions of storage to a fixed bit pattern. The erase type allows
+ * users to specify _why_ the erase operation is done, so the driver or
+ * the core code can choose the most optimal operation.
+ */
+enum erase_type {
+ ERASE_TO_WRITE,
+ ERASE_TO_CLEAR
+};
+
struct fs_driver {
int (*probe) (struct device *dev);
@@ -58,7 +80,7 @@ struct fs_driver {
int (*ioctl)(struct device *dev, FILE *f, unsigned int request, void *buf);
int (*erase)(struct device *dev, FILE *f, loff_t count,
- loff_t offset);
+ loff_t offset, enum erase_type type);
int (*protect)(struct device *dev, FILE *f, size_t count,
loff_t offset, int prot);
int (*discard_range)(struct device *dev, FILE *f, loff_t count,
@@ -127,7 +149,7 @@ int umount_by_cdev(struct cdev *cdev);
/* not-so-standard functions */
#define ERASE_SIZE_ALL ((loff_t) - 1)
-int erase(int fd, loff_t count, loff_t offset);
+int erase(int fd, loff_t count, loff_t offset, enum erase_type type);
int protect(int fd, size_t count, loff_t offset, int prot);
int discard_range(int fd, loff_t count, loff_t offset);
int protect_file(const char *file, int prot);
diff --git a/lib/libfile.c b/lib/libfile.c
index a34e011f4f7c..80d4591dd6e5 100644
--- a/lib/libfile.c
+++ b/lib/libfile.c
@@ -412,7 +412,7 @@ int write_file_flash(const char *filename, const void *buf, size_t size)
if (fd < 0)
return fd;
- ret = erase(fd, size, 0);
+ ret = erase(fd, size, 0, ERASE_TO_WRITE);
if (ret < 0)
goto out_close;
--
2.39.2
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 02/10] block: factor out chunk_flush helper
2024-07-30 7:19 [PATCH 00/10] mmc: add SD/eMMC erase support Ahmad Fatoum
2024-07-30 7:19 ` [PATCH 01/10] fs: give erase() a new erase_type parameter Ahmad Fatoum
@ 2024-07-30 7:19 ` Ahmad Fatoum
2024-07-30 7:19 ` [PATCH 03/10] block: allow block devices to implement the cdev erase operation Ahmad Fatoum
` (7 subsequent siblings)
9 siblings, 0 replies; 29+ messages in thread
From: Ahmad Fatoum @ 2024-07-30 7:19 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
We have two places where we flush chunks: when explicitly flushing
all buffered blocks and when there are no idle blocks.
A third place will be added soon for the new erasure callback, so
prepare for that by factoring out the helper.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
common/block.c | 42 ++++++++++++++++++++++++------------------
1 file changed, 24 insertions(+), 18 deletions(-)
diff --git a/common/block.c b/common/block.c
index c7ca4fb4034a..f55da775a797 100644
--- a/common/block.c
+++ b/common/block.c
@@ -31,6 +31,24 @@ static int writebuffer_io_len(struct block_device *blk, struct chunk *chunk)
return min_t(blkcnt_t, blk->rdbufsize, blk->num_blocks - chunk->block_start);
}
+static int chunk_flush(struct block_device *blk, struct chunk *chunk)
+{
+ int ret;
+
+ if (!chunk->dirty)
+ return 0;
+
+ ret = blk->ops->write(blk, chunk->data,
+ chunk->block_start,
+ writebuffer_io_len(blk, chunk));
+ if (ret < 0)
+ return ret;
+
+ chunk->dirty = 0;
+
+ return 0;
+}
+
/*
* Write all dirty chunks back to the device
*/
@@ -43,15 +61,9 @@ static int writebuffer_flush(struct block_device *blk)
return 0;
list_for_each_entry(chunk, &blk->buffered_blocks, list) {
- if (chunk->dirty) {
- ret = blk->ops->write(blk, chunk->data,
- chunk->block_start,
- writebuffer_io_len(blk, chunk));
- if (ret < 0)
- return ret;
-
- chunk->dirty = 0;
- }
+ ret = chunk_flush(blk, chunk);
+ if (ret < 0)
+ return ret;
}
if (blk->ops->flush)
@@ -112,15 +124,9 @@ static struct chunk *get_chunk(struct block_device *blk)
if (list_empty(&blk->idle_blocks)) {
/* use last entry which is the most unused */
chunk = list_last_entry(&blk->buffered_blocks, struct chunk, list);
- if (chunk->dirty) {
- ret = blk->ops->write(blk, chunk->data,
- chunk->block_start,
- writebuffer_io_len(blk, chunk));
- if (ret < 0)
- return ERR_PTR(ret);
-
- chunk->dirty = 0;
- }
+ ret = chunk_flush(blk, chunk);
+ if (ret < 0)
+ return ERR_PTR(ret);
} else {
chunk = list_first_entry(&blk->idle_blocks, struct chunk, list);
}
--
2.39.2
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 03/10] block: allow block devices to implement the cdev erase operation
2024-07-30 7:19 [PATCH 00/10] mmc: add SD/eMMC erase support Ahmad Fatoum
2024-07-30 7:19 ` [PATCH 01/10] fs: give erase() a new erase_type parameter Ahmad Fatoum
2024-07-30 7:19 ` [PATCH 02/10] block: factor out chunk_flush helper Ahmad Fatoum
@ 2024-07-30 7:19 ` Ahmad Fatoum
2024-07-30 8:55 ` Sascha Hauer
2024-07-30 7:19 ` [PATCH 04/10] mci: turn bool members into bitfield in struct mci Ahmad Fatoum
` (6 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Ahmad Fatoum @ 2024-07-30 7:19 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
We mainly implement cdev erase for raw flashes. Many block devices, like
NVMe, SSD or SD/MMC are also flash-based and expose a mechanism or
multiple to trigger an erase on hardware or flash translation layer
level without an accompanied write.
To make it possible to use this functionality elsewhere in barebox,
let's have the common block device layer pass through the erase
operation, so it may be implemented in a device-specific manner.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
common/block.c | 43 +++++++++++++++++++++++++++++++++++++++++++
include/block.h | 1 +
2 files changed, 44 insertions(+)
diff --git a/common/block.c b/common/block.c
index f55da775a797..b43fcbe6927b 100644
--- a/common/block.c
+++ b/common/block.c
@@ -380,10 +380,45 @@ static int block_op_discard_range(struct cdev *cdev, loff_t count, loff_t offset
return 0;
}
+static bool region_overlap(loff_t starta, loff_t lena,
+ loff_t startb, loff_t lenb)
+{
+ if (starta + lena <= startb)
+ return 0;
+ if (startb + lenb <= starta)
+ return 0;
+ return 1;
+}
+
+static __maybe_unused int block_op_erase(struct cdev *cdev, loff_t count, loff_t offset)
+{
+ struct block_device *blk = cdev->priv;
+ struct chunk *chunk, *tmp;
+ int ret;
+
+ if (!blk->ops->erase)
+ return -EOPNOTSUPP;
+
+ count >>= blk->blockbits;
+ offset >>= blk->blockbits;
+
+ list_for_each_entry_safe(chunk, tmp, &blk->buffered_blocks, list) {
+ if (region_overlap(offset, count, chunk->block_start, blk->rdbufsize)) {
+ ret = chunk_flush(blk, chunk);
+ if (ret < 0)
+ return ret;
+ list_move(&chunk->list, &blk->idle_blocks);
+ }
+ }
+
+ return blk->ops->erase(blk, offset, count);
+}
+
static struct cdev_operations block_ops = {
.read = block_op_read,
#ifdef CONFIG_BLOCK_WRITE
.write = block_op_write,
+ .erase = block_op_erase,
#endif
.close = block_op_close,
.flush = block_op_flush,
@@ -429,6 +464,14 @@ int blockdevice_register(struct block_device *blk)
list_add_tail(&chunk->list, &blk->idle_blocks);
}
+ /* TODO: We currently set this to ignore ERASE_TO_FLASH, but it could
+ * be useful to propagate the enum erase_type down into the erase
+ * callbacks and then map ERASE_TO_FLASH here to a discard operation.
+ * Before doing that though, we need to optimize the hardware drivers
+ * (currently eMMC) to erase bigger regions at once
+ */
+ blk->cdev.flags |= DEVFS_WRITE_AUTOERASE;
+
ret = devfs_create(&blk->cdev);
if (ret)
return ret;
diff --git a/include/block.h b/include/block.h
index a0f226c7649f..eb319b953d32 100644
--- a/include/block.h
+++ b/include/block.h
@@ -12,6 +12,7 @@ struct file_list;
struct block_device_ops {
int (*read)(struct block_device *, void *buf, sector_t block, blkcnt_t num_blocks);
int (*write)(struct block_device *, const void *buf, sector_t block, blkcnt_t num_blocks);
+ int (*erase)(struct block_device *blk, sector_t block, blkcnt_t num_blocks);
int (*flush)(struct block_device *);
};
--
2.39.2
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 04/10] mci: turn bool members into bitfield in struct mci
2024-07-30 7:19 [PATCH 00/10] mmc: add SD/eMMC erase support Ahmad Fatoum
` (2 preceding siblings ...)
2024-07-30 7:19 ` [PATCH 03/10] block: allow block devices to implement the cdev erase operation Ahmad Fatoum
@ 2024-07-30 7:19 ` Ahmad Fatoum
2024-07-30 9:06 ` Sascha Hauer
2024-07-30 7:19 ` [PATCH 05/10] mci: describe more command structure in mci.h Ahmad Fatoum
` (5 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Ahmad Fatoum @ 2024-07-30 7:19 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
In preparation for adding more boolean flags, turn the current two bools
into two bits sharing the same u8-sized member.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
include/mci.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/mci.h b/include/mci.h
index 773aed896c96..4f1ff9dc57c8 100644
--- a/include/mci.h
+++ b/include/mci.h
@@ -568,8 +568,8 @@ struct mci {
unsigned csd[4]; /**< card's "card specific data register" */
unsigned cid[4]; /**< card's "card identification register" */
unsigned short rca; /**< relative card address */
- bool sdio; /**< card is a SDIO card */
- bool high_capacity; /**< high capacity card is connected (OCR -> OCR_HCS) */
+ u8 sdio:1; /**< card is a SDIO card */
+ u8 high_capacity:1; /**< high capacity card is connected (OCR -> OCR_HCS) */
unsigned tran_speed; /**< Maximum transfer speed */
/** currently used data block length for read accesses */
unsigned read_bl_len;
--
2.39.2
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 05/10] mci: describe more command structure in mci.h
2024-07-30 7:19 [PATCH 00/10] mmc: add SD/eMMC erase support Ahmad Fatoum
` (3 preceding siblings ...)
2024-07-30 7:19 ` [PATCH 04/10] mci: turn bool members into bitfield in struct mci Ahmad Fatoum
@ 2024-07-30 7:19 ` Ahmad Fatoum
2024-07-30 9:25 ` Yann Sionneau
2024-07-30 7:19 ` [PATCH 06/10] mci: core: use CONFIG_MCI_WRITE, not CONFIG_BLOCK_WRITE Ahmad Fatoum
` (4 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Ahmad Fatoum @ 2024-07-30 7:19 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
This introduces no functional change, but adds a number of definitions
that will come in handy when adding erase support in a later commit.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
include/mci.h | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 60 insertions(+), 1 deletion(-)
diff --git a/include/mci.h b/include/mci.h
index 4f1ff9dc57c8..610040937ee5 100644
--- a/include/mci.h
+++ b/include/mci.h
@@ -14,6 +14,7 @@
#define _MCI_H_
#include <linux/list.h>
+#include <linux/bits.h>
#include <block.h>
#include <fs.h>
#include <regulator.h>
@@ -28,6 +29,7 @@
#define SD_VERSION_1_0 (SD_VERSION_SD | 0x100)
#define SD_VERSION_1_10 (SD_VERSION_SD | 0x1a0)
#define SD_VERSION_2 (SD_VERSION_SD | 0x200)
+#define SD_VERSION_3 (SD_VERSION_SD | 0x300)
/* Firmware revisions for MMC cards */
#define MMC_VERSION_MMC 0x10000
@@ -59,7 +61,8 @@
/* Mask of all caps for bus width */
#define MMC_CAP_BIT_DATA_MASK (MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA)
-#define SD_DATA_4BIT 0x00040000
+#define SD_DATA_4BIT BIT(18)
+#define SD_DATA_STAT_AFTER_ERASE BIT(23)
#define IS_SD(x) (x->version & SD_VERSION_SD)
@@ -90,11 +93,21 @@
#define MMC_CMD_SPI_READ_OCR 58
#define MMC_CMD_SPI_CRC_ON_OFF 59
+ /* class 5 */
+#define MMC_ERASE_GROUP_START 35 /* ac [31:0] data addr R1 */
+#define MMC_ERASE_GROUP_END 36 /* ac [31:0] data addr R1 */
+#define MMC_ERASE 38 /* ac R1b */
+
#define SD_CMD_SEND_RELATIVE_ADDR 3
#define SD_CMD_SWITCH_FUNC 6
#define SD_CMD_SEND_IF_COND 8
+ /* class 5 */
+#define SD_ERASE_WR_BLK_START 32 /* ac [31:0] data addr R1 */
+#define SD_ERASE_WR_BLK_END 33 /* ac [31:0] data addr R1 */
+
#define SD_CMD_APP_SET_BUS_WIDTH 6
+#define SD_CMD_APP_SD_STATUS 13
#define SD_CMD_APP_SEND_OP_COND 41
#define SD_CMD_APP_SEND_SCR 51
@@ -110,6 +123,35 @@
/** card's response in its OCR if it is a high capacity card */
#define OCR_HCS 0x40000000
+/*
+ * Card Command Classes (CCC)
+ */
+#define CCC_BASIC (1<<0) /* (0) Basic protocol functions */
+ /* (CMD0,1,2,3,4,7,9,10,12,13,15) */
+ /* (and for SPI, CMD58,59) */
+#define CCC_STREAM_READ (1<<1) /* (1) Stream read commands */
+ /* (CMD11) */
+#define CCC_BLOCK_READ (1<<2) /* (2) Block read commands */
+ /* (CMD16,17,18) */
+#define CCC_STREAM_WRITE (1<<3) /* (3) Stream write commands */
+ /* (CMD20) */
+#define CCC_BLOCK_WRITE (1<<4) /* (4) Block write commands */
+ /* (CMD16,24,25,26,27) */
+#define CCC_ERASE (1<<5) /* (5) Ability to erase blocks */
+ /* (CMD32,33,34,35,36,37,38,39) */
+#define CCC_WRITE_PROT (1<<6) /* (6) Able to write protect blocks */
+ /* (CMD28,29,30) */
+#define CCC_LOCK_CARD (1<<7) /* (7) Able to lock down card */
+ /* (CMD16,CMD42) */
+#define CCC_APP_SPEC (1<<8) /* (8) Application specific */
+ /* (CMD55,56,57,ACMD*) */
+#define CCC_IO_MODE (1<<9) /* (9) I/O mode */
+ /* (CMD5,39,40,52,53) */
+#define CCC_SWITCH (1<<10) /* (10) High speed switch */
+ /* (CMD6,34,35,36,37,50) */
+ /* (11) Reserved */
+ /* (CMD?) */
+
#define MMC_VDD_165_195 0x00000080 /* VDD voltage 1.65 - 1.95 */
#define MMC_VDD_20_21 0x00000100 /* VDD voltage 2.0 ~ 2.1 */
#define MMC_VDD_21_22 0x00000200 /* VDD voltage 2.1 ~ 2.2 */
@@ -139,6 +181,21 @@
#define SD_SWITCH_CHECK 0
#define SD_SWITCH_SWITCH 1
+/*
+ * Erase/trim/discard
+ */
+#define MMC_ERASE_ARG 0x00000000
+#define MMC_SECURE_ERASE_ARG 0x80000000
+#define MMC_TRIM_ARG 0x00000001
+#define MMC_DISCARD_ARG 0x00000003
+#define MMC_SECURE_TRIM1_ARG 0x80000001
+#define MMC_SECURE_TRIM2_ARG 0x80008000
+#define MMC_SECURE_ARGS 0x80000000
+#define MMC_TRIM_OR_DISCARD_ARGS 0x00008003
+
+#define SD_ERASE_ARG 0x00000000
+#define SD_DISCARD_ARG 0x00000001
+
/*
* EXT_CSD fields
*/
@@ -338,6 +395,8 @@
#define EXT_CSD_TIMING_HS400 3 /* HS400 */
#define EXT_CSD_DRV_STR_SHIFT 4 /* Driver Strength shift */
+#define EXT_CSD_SEC_FEATURE_TRIM_EN (1 << 4) /* Support secure & insecure trim */
+
#define R1_ILLEGAL_COMMAND (1 << 22)
#define R1_STATUS(x) (x & 0xFFF9A000)
#define R1_CURRENT_STATE(x) ((x & 0x00001E00) >> 9) /* sx, b (4 bits) */
--
2.39.2
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 06/10] mci: core: use CONFIG_MCI_WRITE, not CONFIG_BLOCK_WRITE
2024-07-30 7:19 [PATCH 00/10] mmc: add SD/eMMC erase support Ahmad Fatoum
` (4 preceding siblings ...)
2024-07-30 7:19 ` [PATCH 05/10] mci: describe more command structure in mci.h Ahmad Fatoum
@ 2024-07-30 7:19 ` Ahmad Fatoum
2024-07-30 9:18 ` Sascha Hauer
2024-07-30 7:19 ` [PATCH 07/10] mci: add support for discarding write blocks Ahmad Fatoum
` (3 subsequent siblings)
9 siblings, 1 reply; 29+ messages in thread
From: Ahmad Fatoum @ 2024-07-30 7:19 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
There's a more specific CONFIG_MCI_WRITE that's so far only used to
remove write support for in the Atmel MCI driver. We should use the same
symbol also to remove support in the MCI core instead of relying on its
parent CONFIG_BLOCK_WRITE option.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
drivers/mci/mci-core.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
index f6f8a6adabb9..3a5fb0330700 100644
--- a/drivers/mci/mci-core.c
+++ b/drivers/mci/mci-core.c
@@ -1801,8 +1801,8 @@ static int mci_blk_part_switch(struct mci_part *part)
*
* This routine expects the buffer has the correct size to read all data!
*/
-static int __maybe_unused mci_sd_write(struct block_device *blk,
- const void *buffer, sector_t block, blkcnt_t num_blocks)
+static int mci_sd_write(struct block_device *blk,
+ const void *buffer, sector_t block, blkcnt_t num_blocks)
{
struct mci_part *part = container_of(blk, struct mci_part, blk);
struct mci *mci = part->mci;
@@ -2179,9 +2179,7 @@ static int mci_check_if_already_initialized(struct mci *mci)
static struct block_device_ops mci_ops = {
.read = mci_sd_read,
-#ifdef CONFIG_BLOCK_WRITE
- .write = mci_sd_write,
-#endif
+ .write = IS_ENABLED(CONFIG_MCI_WRITE) ? mci_sd_write : NULL,
};
static int mci_set_boot(struct param_d *param, void *priv)
--
2.39.2
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 07/10] mci: add support for discarding write blocks
2024-07-30 7:19 [PATCH 00/10] mmc: add SD/eMMC erase support Ahmad Fatoum
` (5 preceding siblings ...)
2024-07-30 7:19 ` [PATCH 06/10] mci: core: use CONFIG_MCI_WRITE, not CONFIG_BLOCK_WRITE Ahmad Fatoum
@ 2024-07-30 7:19 ` Ahmad Fatoum
2024-07-30 9:23 ` Yann Sionneau
2024-07-30 10:05 ` Sascha Hauer
2024-07-30 7:19 ` [PATCH 08/10] commands: sync: add new command to flush cached writes Ahmad Fatoum
` (2 subsequent siblings)
9 siblings, 2 replies; 29+ messages in thread
From: Ahmad Fatoum @ 2024-07-30 7:19 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
From: Ahmad Fatoum <ahmad@a3f.at>
A common optimization for flashing is to skip gaps between partitions
and only flash actual data. The problem with that is that data from
the previous installation may persist and confuse later software, e.g.
an existing barebox state partition not contained in the image may
survive, when the expectation is that a new state is created.
eMMCs can support three different commands for erasing data:
- Erase: This has the widest support, but works on only on the level
of erase groups that may span multiple write blocks.
The region reads as either '0' or '1' afterwards.
- Trim: New in eMMC v4.4. This erases write blocks.
The region reads as either '0' or '1' afterwards.
- Discard: New in eMMC v4.5. This discards write blocks. It's up to the
card what action if any is taken.
All or part of the data may remain accessible.
All of these don't enforce a actual physical NAND erase operation.
In case erasure does happen, it may happen later at a convenient time.
All of them, except for discard guarantee that a fixed pattern (either
all zeroes or all ones) will be read back after the erase operation
concludes. Therefore let's use them in barebox to implement the erase
command.
The behavior of the erase command will be similar to the Linux
blkdiscard -z command when the erase byte value is all-zeroes. In Linux
blkdiscard -z is emulated with zero writes if the erased byte is 0xFF,
but for barebox, the erase operation won't care whether it writes 0x00
or 0xFF.
Note that this is considerably slower than need be, because we don't
erase multiple erase groups at once. Doing so could run into the
send_cmd timeout of the host hardware or its drivers. The correct
workaround is to calculate the duration of the erase operation and split
up the erases, so we always stay below a specific erase operation
duration. There's a comment that explains this and implementing it is
left as future exercise.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
drivers/mci/Kconfig | 5 +
drivers/mci/mci-core.c | 393 ++++++++++++++++++++++++++++++++++++++---
include/mci.h | 10 ++
3 files changed, 387 insertions(+), 21 deletions(-)
diff --git a/drivers/mci/Kconfig b/drivers/mci/Kconfig
index 1e8c85643b9a..f760614725fa 100644
--- a/drivers/mci/Kconfig
+++ b/drivers/mci/Kconfig
@@ -40,6 +40,11 @@ config MCI_WRITE
default y
select DISK_WRITE
+config MCI_ERASE
+ bool "Support erasing MCI cards"
+ depends on MCI_WRITE
+ default y
+
config MCI_MMC_BOOT_PARTITIONS
bool "support MMC boot partitions"
help
diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
index 3a5fb0330700..200b23ffb604 100644
--- a/drivers/mci/mci-core.c
+++ b/drivers/mci/mci-core.c
@@ -20,6 +20,7 @@
#include <disks.h>
#include <of.h>
#include <linux/err.h>
+#include <linux/log2.h>
#include <linux/sizes.h>
#include <dma.h>
@@ -168,6 +169,32 @@ static int mci_send_status(struct mci *mci, unsigned int *status)
return ret;
}
+static int mci_app_sd_status(struct mci *mci, __be32 *ssr)
+{
+ int err;
+ struct mci_cmd cmd;
+ struct mci_data data;
+
+ cmd.cmdidx = MMC_CMD_APP_CMD;
+ cmd.resp_type = MMC_RSP_R1;
+ cmd.cmdarg = mci->rca << 16;
+
+ err = mci_send_cmd_retry(mci, &cmd, NULL, 4);
+ if (err)
+ return err;
+
+ cmd.cmdidx = SD_CMD_APP_SD_STATUS;
+ cmd.resp_type = MMC_RSP_R1;
+ cmd.cmdarg = 0;
+
+ data.dest = (u8 *)ssr;
+ data.blocksize = 64;
+ data.blocks = 1;
+ data.flags = MMC_DATA_READ;
+
+ return mci_send_cmd_retry(mci, &cmd, &data, 3);
+}
+
static int mmc_switch_status_error(struct mci_host *host, u32 status)
{
if (mmc_host_is_spi(host)) {
@@ -286,6 +313,56 @@ static int mci_block_write(struct mci *mci, const void *src, int blocknum,
return ret;
}
+/**
+ * Erase one or several blocks of data to the card
+ * @param mci_dev MCI instance
+ * @param from Block number to start erasing from
+ * @param to inclusive last block number to erase to
+ * @return Transaction status (0 on success)
+ */
+static int mci_block_erase(struct mci *card, unsigned int from,
+ unsigned int to, unsigned int arg)
+{
+ struct mci_cmd cmd = {};
+ int err;
+
+ if (!card->high_capacity) {
+ from *= card->write_bl_len;
+ to *= card->write_bl_len;
+ }
+
+ cmd.cmdidx = IS_SD(card) ? SD_ERASE_WR_BLK_START : MMC_ERASE_GROUP_START;
+ cmd.cmdarg = from;
+ cmd.resp_type = MMC_RSP_R1;
+ err = mci_send_cmd(card, &cmd, NULL);
+ if (err)
+ goto err_out;
+
+ memset(&cmd, 0, sizeof(struct mci_cmd));
+ cmd.cmdidx = IS_SD(card) ? SD_ERASE_WR_BLK_END : MMC_ERASE_GROUP_END;
+ cmd.cmdarg = to;
+ cmd.resp_type = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
+ err = mci_send_cmd(card, &cmd, NULL);
+ if (err)
+ goto err_out;
+
+ memset(&cmd, 0, sizeof(struct mci_cmd));
+ cmd.cmdidx = MMC_ERASE;
+ cmd.cmdarg = arg;
+ cmd.resp_type = MMC_RSP_R1b;
+
+ err = mci_send_cmd(card, &cmd, NULL);
+ if (err)
+ goto err_out;
+
+ return 0;
+
+err_out:
+ dev_err(&card->dev, "erase cmd %d error %d, status %#x\n",
+ cmd.cmdidx, err, cmd.response[0]);
+ return -EIO;
+}
+
/**
* Read one or several block(s) of data from the card
* @param mci MCI instance
@@ -773,6 +850,49 @@ static int sd_switch(struct mci *mci, unsigned mode, unsigned group,
return mci_send_cmd(mci, &cmd, &data);
}
+static int sd_read_ssr(struct mci *mci)
+{
+ static const unsigned int sd_au_size[] = {
+ 0, SZ_16K / 512, SZ_32K / 512,
+ SZ_64K / 512, SZ_128K / 512, SZ_256K / 512,
+ SZ_512K / 512, SZ_1M / 512, SZ_2M / 512,
+ SZ_4M / 512, SZ_8M / 512, (SZ_8M + SZ_4M) / 512,
+ SZ_16M / 512, (SZ_16M + SZ_8M) / 512, SZ_32M / 512,
+ SZ_64M / 512,
+ };
+ __be32 *ssr;
+ int err;
+ unsigned int au, eo, et, es;
+
+ if (!IS_ENABLED(CONFIG_MCI_ERASE))
+ return -ENOSYS;
+
+ ssr = dma_alloc(64);
+
+ err = mci_app_sd_status(mci, ssr);
+ if (err)
+ goto out;
+
+ au = (be32_to_cpu(ssr[2]) >> 12) & 0xF;
+ if ((au <= 9) || (mci->version == SD_VERSION_3)) {
+ mci->ssr.au = sd_au_size[au];
+ es = (be32_to_cpu(ssr[3]) >> 24) & 0xFF;
+ es |= (be32_to_cpu(ssr[2]) & 0xFF) << 8;
+ et = (be32_to_cpu(ssr[3]) >> 18) & 0x3F;
+ if (es && et) {
+ eo = (be32_to_cpu(ssr[3]) >> 16) & 0x3;
+ mci->ssr.erase_timeout = (et * 1000) / es;
+ mci->ssr.erase_offset = eo * 1000;
+ }
+ } else {
+ dev_dbg(&mci->dev, "invalid allocation unit size.\n");
+ }
+
+out:
+ dma_free(ssr);
+ return err;
+}
+
/**
* Change transfer frequency for an SD card
* @param mci MCI instance
@@ -845,6 +965,11 @@ static int sd_change_freq(struct mci *mci)
if (mci->scr[0] & SD_DATA_4BIT)
mci->card_caps |= MMC_CAP_4_BIT_DATA;
+ if (mci->scr[0] & SD_DATA_STAT_AFTER_ERASE)
+ mci->erased_byte = 0xFF;
+ else
+ mci->erased_byte = 0x0;
+
/* Version 1.0 doesn't support switching */
if (mci->version == SD_VERSION_1_0)
return 0;
@@ -1083,6 +1208,47 @@ static void mci_extract_block_lengths_from_csd(struct mci *mci)
mci->write_bl_len, mci->read_bl_len);
}
+/**
+ * Extract erase group size
+ * @param mci MCI instance
+ */
+static void mci_extract_erase_group_size(struct mci *mci)
+{
+ if (!IS_ENABLED(CONFIG_MCI_ERASE) ||
+ !(UNSTUFF_BITS(mci->csd, 84, 12) & CCC_ERASE))
+ return;
+
+
+ if (IS_SD(mci) && UNSTUFF_BITS(mci->csd, 126, 2) != 0) {
+ /* For SD with csd_struct v1, erase group is always one sector */
+ mci->erase_grp_size = 1;
+ } else {
+ if (mci->ext_csd[EXT_CSD_ERASE_GROUP_DEF] & 0x01) {
+ /* Read out group size from ext_csd */
+ mci->erase_grp_size = mci->ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE] * 1024;
+ } else {
+ /* Calculate the group size from the csd value. */
+ int erase_gsz, erase_gmul;
+
+ erase_gsz = (mci->csd[2] & 0x00007c00) >> 10;
+ erase_gmul = (mci->csd[2] & 0x000003e0) >> 5;
+ mci->erase_grp_size = (erase_gsz + 1)
+ * (erase_gmul + 1);
+ }
+
+ if (mci->ext_csd[EXT_CSD_ERASED_MEM_CONT])
+ mci->erased_byte = 0xFF;
+ else
+ mci->erased_byte = 0x0;
+
+ if (mci->ext_csd[EXT_CSD_SEC_FEATURE_SUPPORT] & EXT_CSD_SEC_FEATURE_TRIM_EN)
+ mci->can_trim = true;
+ }
+
+ dev_dbg(&mci->dev, "Erase group is %u sector(s). Trim %ssupported\n",
+ mci->erase_grp_size, mci->can_trim ? "" : "not ");
+}
+
/**
* Extract card's capacitiy from the CSD
* @param mci MCI instance
@@ -1229,6 +1395,10 @@ static int mci_startup_sd(struct mci *mci)
mci_set_clock(mci, mci->tran_speed);
+ err = sd_read_ssr(mci);
+ if (err)
+ dev_dbg(&mci->dev, "unable to read ssr: %pe\n", ERR_PTR(err));
+
return 0;
}
@@ -1700,6 +1870,7 @@ static int mci_startup(struct mci *mci)
dev_info(&mci->dev, "detected %s card version %s\n", IS_SD(mci) ? "SD" : "MMC",
mci_version_string(mci));
mci_extract_card_capacity_from_csd(mci);
+ mci_extract_erase_group_size(mci);
if (IS_SD(mci))
err = mci_startup_sd(mci);
@@ -1791,6 +1962,189 @@ static int mci_blk_part_switch(struct mci_part *part)
/* ------------------ attach to the blocklayer --------------------------- */
+static int mci_sd_check_write(struct mci *mci, const char *op,
+ sector_t block, blkcnt_t num_blocks)
+{
+ struct mci_host *host = mci->host;
+
+ if (!host->disable_wp &&
+ host->ops.card_write_protected && host->ops.card_write_protected(host)) {
+ dev_err(&mci->dev, "card write protected\n");
+ return -EPERM;
+ }
+
+ dev_dbg(&mci->dev, "%s: %s %llu block(s), starting at %llu\n",
+ __func__, op, num_blocks, block);
+
+ if (mci->write_bl_len != SECTOR_SIZE) {
+ dev_dbg(&mci->dev, "MMC/SD block size is not %d bytes (its %u bytes instead)\n",
+ SECTOR_SIZE, mci->read_bl_len);
+ return -EINVAL;
+ }
+
+ /* size of the block number field in the MMC/SD command is 32 bit only */
+ if (block > MAX_BUFFER_NUMBER) {
+ dev_dbg(&mci->dev, "Cannot handle block number %llu. Too large!\n", block);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static unsigned int mmc_align_erase_size(struct mci *card,
+ sector_t *from,
+ sector_t *to,
+ blkcnt_t nr)
+{
+ unsigned int from_new = *from, to_new, nr_new = nr, rem;
+
+ /*
+ * When the 'card->erase_size' is power of 2, we can use round_up/down()
+ * to align the erase size efficiently.
+ */
+ if (is_power_of_2(card->erase_grp_size)) {
+ unsigned int temp = from_new;
+
+ from_new = round_up(temp, card->erase_grp_size);
+ rem = from_new - temp;
+
+ if (nr_new > rem)
+ nr_new -= rem;
+ else
+ return 0;
+
+ nr_new = round_down(nr_new, card->erase_grp_size);
+ } else {
+ rem = from_new % card->erase_grp_size;
+ if (rem) {
+ rem = card->erase_grp_size - rem;
+ from_new += rem;
+ if (nr_new > rem)
+ nr_new -= rem;
+ else
+ return 0;
+ }
+
+ rem = nr_new % card->erase_grp_size;
+ if (rem)
+ nr_new -= rem;
+ }
+
+ if (nr_new == 0)
+ return 0;
+
+ to_new = from_new + nr_new;
+
+ if (*to != to_new || *from != from_new)
+ dev_warn(&card->dev, "Erase range changed to [0x%x-0x%x] because of %u sector erase group\n",
+ from_new, to_new, card->erase_grp_size);
+
+ *to = to_new;
+ *from = from_new;
+
+ return nr_new;
+}
+
+/**
+ * Erase a memory region
+ * @param blk All info about the block device we need
+ * @param block first block to erase
+ * @param num_blocks Number of blocks to erase
+ * @return 0 on success, anything else on failure
+ *
+ */
+static int mci_sd_erase(struct block_device *blk, sector_t from,
+ blkcnt_t blkcnt)
+{
+ struct mci_part *part = container_of(blk, struct mci_part, blk);
+ struct mci *mci = part->mci;
+ sector_t i = 0;
+ unsigned arg;
+ sector_t to = from + blkcnt;
+ int rc;
+
+ mci_blk_part_switch(part);
+
+ rc = mci_sd_check_write(mci, "Erase", from, blkcnt);
+ if (rc)
+ return rc;
+
+ if (!mci->erase_grp_size)
+ return -EOPNOTSUPP;
+
+ if (mci->can_trim) {
+ arg = MMC_TRIM_ARG;
+ } else {
+ /* We don't use discard, as it doesn't guarantee a fixed value */
+ arg = MMC_ERASE_ARG;
+ blkcnt = mmc_align_erase_size(mci, &from, &to, blkcnt);
+ }
+
+ if (blkcnt == 0)
+ return 0;
+
+ if (to <= from)
+ return -EINVAL;
+
+ /* 'from' and 'to' are inclusive */
+ to -= 1;
+
+ while (i < blkcnt) {
+ sector_t blk_r;
+
+ /* TODO: While it's possible to clear many erase groups at once
+ * and it greatly improves throughput, drivers need adjustment:
+ *
+ * Many drivers hardcode a maximal wait time before aborting
+ * the wait for R1b and returning -ETIMEDOUT. With long
+ * erases/trims, we are bound to run into this timeout, so for now
+ * we just split into suifficiently small erases that are unlikely
+ * to trigger the time.
+ *
+ * What Linux does and what we should be doing in barebox is:
+ *
+ * - add a struct mci_cmd::busy_timeout member that drivers should
+ * use instead of hardcoding their own timeout delay. The busy
+ * timeout length can be calculated by the MCI core after
+ * consulting the appropriate CSD/EXT_CSD/SSR registers.
+ *
+ * - add a struct mci_host::max_busy_timeout member, where drivers
+ * can indicate the maximum timeout they are able to support.
+ * The MCI core will never set a busy_timeout that exceeds this
+ * value.
+ *
+ * Example Samsung eMMC 8GTF4:
+ *
+ * time erase /dev/mmc2.part_of_512m # 1024 trims
+ * time: 2849ms
+ *
+ * time erase /dev/mmc2.part_of_512m # single trim
+ * time: 56ms
+ */
+
+ if (IS_SD(mci) && mci->ssr.au) {
+ blk_r = ((blkcnt - i) > mci->ssr.au) ?
+ mci->ssr.au : (blkcnt - i);
+ } else {
+ blk_r = ((blkcnt - i) > mci->erase_grp_size) ?
+ mci->erase_grp_size : (blkcnt - i);
+ }
+
+ rc = mci_block_erase(mci, from, to, arg);
+ if (rc)
+ break;
+
+ /* Waiting for the ready status */
+ rc = mci_poll_until_ready(mci, 1000 /* ms */);
+ if (rc)
+ break;
+
+ i += blk_r;
+ }
+
+ return i == blkcnt ? 0 : rc;
+}
+
/**
* Write a chunk of sectors to media
* @param blk All info about the block device we need
@@ -1806,7 +2160,6 @@ static int mci_sd_write(struct block_device *blk,
{
struct mci_part *part = container_of(blk, struct mci_part, blk);
struct mci *mci = part->mci;
- struct mci_host *host = mci->host;
int rc;
blkcnt_t max_req_block = num_blocks;
blkcnt_t write_block;
@@ -1816,26 +2169,9 @@ static int mci_sd_write(struct block_device *blk,
mci_blk_part_switch(part);
- if (!host->disable_wp &&
- host->ops.card_write_protected && host->ops.card_write_protected(host)) {
- dev_err(&mci->dev, "card write protected\n");
- return -EPERM;
- }
-
- dev_dbg(&mci->dev, "%s: Write %llu block(s), starting at %llu\n",
- __func__, num_blocks, block);
-
- if (mci->write_bl_len != SECTOR_SIZE) {
- dev_dbg(&mci->dev, "MMC/SD block size is not %d bytes (its %u bytes instead)\n",
- SECTOR_SIZE, mci->read_bl_len);
- return -EINVAL;
- }
-
- /* size of the block number field in the MMC/SD command is 32 bit only */
- if (block > MAX_BUFFER_NUMBER) {
- dev_dbg(&mci->dev, "Cannot handle block number %llu. Too large!\n", block);
- return -EINVAL;
- }
+ rc = mci_sd_check_write(mci, "Write", block, num_blocks);
+ if (rc)
+ return rc;
while (num_blocks) {
write_block = min(num_blocks, max_req_block);
@@ -2123,6 +2459,20 @@ static void mci_info(struct device *dev)
return;
printf(" Version: %s\n", mci_version_string(mci));
printf(" Capacity: %u MiB\n", (unsigned)(mci->capacity >> 20));
+ if (CONFIG_MCI_ERASE) {
+ printf(" Erase support:");
+ if (mci->can_trim)
+ printf(" trim");
+ if (mci->erase_grp_size) {
+ printf(" erase(%u sector%s%s)", mci->ssr.au ?: mci->erase_grp_size,
+ mci->erase_grp_size > 1 ? "s" : "",
+ mci->ssr.au ? " AU" : "");
+ }
+ if (mci->can_trim || mci->erase_grp_size)
+ printf(", erase value: 0x%02x\n", mci->erased_byte);
+ else
+ printf(" none\n");
+ }
if (mci->high_capacity)
printf(" High capacity card\n");
@@ -2180,6 +2530,7 @@ static int mci_check_if_already_initialized(struct mci *mci)
static struct block_device_ops mci_ops = {
.read = mci_sd_read,
.write = IS_ENABLED(CONFIG_MCI_WRITE) ? mci_sd_write : NULL,
+ .erase = IS_ENABLED(CONFIG_MCI_ERASE) ? mci_sd_erase : NULL,
};
static int mci_set_boot(struct param_d *param, void *priv)
diff --git a/include/mci.h b/include/mci.h
index 610040937ee5..3bf1455a401c 100644
--- a/include/mci.h
+++ b/include/mci.h
@@ -616,6 +616,12 @@ struct mci_part {
#define MMC_BLK_DATA_AREA_RPMB (1<<3)
};
+struct sd_ssr {
+ unsigned int au; /* In sectors */
+ unsigned int erase_timeout; /* In milliseconds */
+ unsigned int erase_offset; /* In milliseconds */
+};
+
/** MMC/SD and interface instance information */
struct mci {
struct mci_host *host; /**< the host for this card */
@@ -629,16 +635,20 @@ struct mci {
unsigned short rca; /**< relative card address */
u8 sdio:1; /**< card is a SDIO card */
u8 high_capacity:1; /**< high capacity card is connected (OCR -> OCR_HCS) */
+ u8 can_trim:1; /**< high capacity card is connected (OCR -> OCR_HCS) */
+ u8 erased_byte;
unsigned tran_speed; /**< Maximum transfer speed */
/** currently used data block length for read accesses */
unsigned read_bl_len;
/** currently used data block length for write accesses */
unsigned write_bl_len;
+ unsigned erase_grp_size;
uint64_t capacity; /**< Card's data capacity in bytes */
int ready_for_use; /** true if already probed */
int dsr_imp; /**< DSR implementation state from CSD */
u8 *ext_csd;
int probe;
+ struct sd_ssr ssr;
int bootpart;
int boot_ack_enable;
--
2.39.2
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 08/10] commands: sync: add new command to flush cached writes
2024-07-30 7:19 [PATCH 00/10] mmc: add SD/eMMC erase support Ahmad Fatoum
` (6 preceding siblings ...)
2024-07-30 7:19 ` [PATCH 07/10] mci: add support for discarding write blocks Ahmad Fatoum
@ 2024-07-30 7:19 ` Ahmad Fatoum
2024-07-30 10:08 ` Sascha Hauer
2024-07-30 7:19 ` [PATCH 09/10] mci: core: remove reference to SD Card from common mci_card_probe Ahmad Fatoum
2024-07-30 7:19 ` [PATCH 10/10] commands: blkstats: add command to print block device statistics Ahmad Fatoum
9 siblings, 1 reply; 29+ messages in thread
From: Ahmad Fatoum @ 2024-07-30 7:19 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
In barebox, the dirty block cache for a given device is flushed when
closing all file descriptors to it, a cached block needs to be evicted
due to pressure or when barebox shuts down.
There are valid uses to do it explicitly however:
- When benchmarking write speed to a block device
- Before forced resets, because a regular reset would run shutdown
code that should be omitted.
Add a sync command that does this. Unlike the userspace variant, we
don't both to implement a argument-less sync that's global. Users are
supposed to flush the files they are actually interested in, e.g.:
sync /dev/mmc2.0
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
commands/Kconfig | 14 ++++++++++++++
commands/Makefile | 1 +
commands/sync.c | 42 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 57 insertions(+)
create mode 100644 commands/sync.c
diff --git a/commands/Kconfig b/commands/Kconfig
index 5b512f1bbac7..a8b7037618cc 100644
--- a/commands/Kconfig
+++ b/commands/Kconfig
@@ -1871,6 +1871,20 @@ config CMD_DETECT
-e bail out if one device fails to detect
-a detect all devices
+config CMD_SYNC
+ tristate
+ prompt "sync"
+ help
+ sync - flush cached writes
+
+ Usage: sync DEVICE
+
+ Synchronize cached writes to persistent storage of DEVICE
+ immediately instead of waiting for block device to be closed,
+ cache eviction or the regular barebox exit.
+
+ This command is mainly useful for benchmarking.
+
config CMD_FLASH
tristate
prompt "erase, protect and unprotect"
diff --git a/commands/Makefile b/commands/Makefile
index 4ca7ba7eb609..a9dbead4389f 100644
--- a/commands/Makefile
+++ b/commands/Makefile
@@ -41,6 +41,7 @@ obj-$(CONFIG_CMD_REGINFO) += reginfo.o
obj-$(CONFIG_CMD_CRC) += crc.o
obj-$(CONFIG_CMD_CLEAR) += clear.o
obj-$(CONFIG_CMD_TEST) += test.o
+obj-$(CONFIG_CMD_SYNC) += sync.o
obj-$(CONFIG_CMD_FLASH) += flash.o
obj-$(CONFIG_CMD_MEMINFO) += meminfo.o
obj-$(CONFIG_CMD_TIMEOUT) += timeout.o
diff --git a/commands/sync.c b/commands/sync.c
new file mode 100644
index 000000000000..6add67cd0876
--- /dev/null
+++ b/commands/sync.c
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/* sync - flushing support */
+
+#include <common.h>
+#include <command.h>
+#include <fcntl.h>
+#include <fs.h>
+
+static int do_sync(int argc, char *argv[])
+{
+ int ret, fd;
+
+ if (argc != 2)
+ return COMMAND_ERROR_USAGE;
+
+ fd = open(argv[1], O_WRONLY);
+ if (fd < 0) {
+ printf("open %s: %m\n", argv[1]);
+ return 1;
+ }
+
+ ret = flush(fd);
+
+ close(fd);
+
+ return ret;
+}
+
+BAREBOX_CMD_HELP_START(sync)
+BAREBOX_CMD_HELP_TEXT("Synchronize cached writes to persistent storage of DEVICE")
+BAREBOX_CMD_HELP_TEXT("immediately instead of waiting for block device to be closed,")
+BAREBOX_CMD_HELP_TEXT("cache eviction or the regular barebox exit.")
+BAREBOX_CMD_HELP_END
+
+BAREBOX_CMD_START(sync)
+ .cmd = do_sync,
+ BAREBOX_CMD_DESC("flush cached writes")
+ BAREBOX_CMD_OPTS("DEVICE")
+ BAREBOX_CMD_GROUP(CMD_GRP_HWMANIP)
+ BAREBOX_CMD_HELP(cmd_sync_help)
+BAREBOX_CMD_END
--
2.39.2
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 09/10] mci: core: remove reference to SD Card from common mci_card_probe
2024-07-30 7:19 [PATCH 00/10] mmc: add SD/eMMC erase support Ahmad Fatoum
` (7 preceding siblings ...)
2024-07-30 7:19 ` [PATCH 08/10] commands: sync: add new command to flush cached writes Ahmad Fatoum
@ 2024-07-30 7:19 ` Ahmad Fatoum
2024-07-30 10:09 ` Sascha Hauer
2024-07-30 7:19 ` [PATCH 10/10] commands: blkstats: add command to print block device statistics Ahmad Fatoum
9 siblings, 1 reply; 29+ messages in thread
From: Ahmad Fatoum @ 2024-07-30 7:19 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
mci_card_probe is called for both SD cards and MMC, so reporting that
an SD Card was successfully added is misleading.
There is an info message that reports whether it's a MMC or SD card
already, so just remove the SD from this last debug print.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
drivers/mci/mci-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
index 200b23ffb604..c7e3c4b6976d 100644
--- a/drivers/mci/mci-core.c
+++ b/drivers/mci/mci-core.c
@@ -2784,7 +2784,7 @@ static int mci_card_probe(struct mci *mci)
mci_parse_cid(mci);
- dev_dbg(&mci->dev, "SD Card successfully added\n");
+ dev_dbg(&mci->dev, "Card successfully added\n");
on_error:
if (rc != 0) {
--
2.39.2
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 10/10] commands: blkstats: add command to print block device statistics
2024-07-30 7:19 [PATCH 00/10] mmc: add SD/eMMC erase support Ahmad Fatoum
` (8 preceding siblings ...)
2024-07-30 7:19 ` [PATCH 09/10] mci: core: remove reference to SD Card from common mci_card_probe Ahmad Fatoum
@ 2024-07-30 7:19 ` Ahmad Fatoum
9 siblings, 0 replies; 29+ messages in thread
From: Ahmad Fatoum @ 2024-07-30 7:19 UTC (permalink / raw)
To: barebox; +Cc: Ahmad Fatoum
To test proper operations of block device operations, add a command that
prints how many sectors were read/written/erased for a device so far.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
commands/Kconfig | 11 +++++++++
commands/Makefile | 1 +
commands/blkstats.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
common/Kconfig | 3 +++
common/block.c | 46 ++++++++++++++++++++++++++++------
include/block.h | 10 ++++++++
6 files changed, 123 insertions(+), 8 deletions(-)
create mode 100644 commands/blkstats.c
diff --git a/commands/Kconfig b/commands/Kconfig
index a8b7037618cc..64e834d95a8f 100644
--- a/commands/Kconfig
+++ b/commands/Kconfig
@@ -255,6 +255,17 @@ config CMD_REGINFO
help
Print register information.
+config CMD_BLKSTATS
+ bool
+ depends on BLOCK
+ select BLOCK_STATS
+ prompt "blkstats command"
+ help
+ The blkstats displays statistics about a block devices' number of
+ sectors read, written and erased. This should only be needed for
+ development. Saying y here will start to collect these statistics
+ and enable a command for querying them.
+
config CMD_REGULATOR
bool
depends on REGULATOR
diff --git a/commands/Makefile b/commands/Makefile
index a9dbead4389f..ff5d713ca72c 100644
--- a/commands/Makefile
+++ b/commands/Makefile
@@ -125,6 +125,7 @@ obj-$(CONFIG_CMD_DRVINFO) += drvinfo.o
obj-$(CONFIG_CMD_READF) += readf.o
obj-$(CONFIG_CMD_MENUTREE) += menutree.o
obj-$(CONFIG_CMD_2048) += 2048.o
+obj-$(CONFIG_CMD_BLKSTATS) += blkstats.o
obj-$(CONFIG_CMD_REGULATOR) += regulator.o
obj-$(CONFIG_CMD_PM_DOMAIN) += pm_domain.o
obj-$(CONFIG_CMD_LSPCI) += lspci.o
diff --git a/commands/blkstats.c b/commands/blkstats.c
new file mode 100644
index 000000000000..69bed8b40e95
--- /dev/null
+++ b/commands/blkstats.c
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <common.h>
+#include <command.h>
+#include <block.h>
+#include <getopt.h>
+#include <fs.h>
+
+static int do_blkstats(int argc, char *argv[])
+{
+ struct block_device *blk;
+ const char *name;
+ int opt;
+
+ while ((opt = getopt(argc, argv, "l")) > 0) {
+ switch (opt) {
+ case 'l':
+ for_each_block_device(blk) {
+ printf("%s\n", blk->cdev.name);
+ }
+ default:
+ return COMMAND_ERROR_USAGE;
+ }
+ }
+
+ argv += optind;
+ argc -= optind;
+
+ name = argv[0];
+
+ for_each_block_device(blk) {
+ struct block_device_stats *stats;
+
+ if (name && strcmp(name, blk->cdev.name))
+ continue;
+
+ stats = &blk->stats;
+
+ printf("%-10s %10s %10s %10s\n", "Device", "Read", "Write", "Erase");
+ printf("%-10s %10llu %10llu %10llu\n", blk->cdev.name,
+ stats->read_sectors, stats->write_sectors, stats->erase_sectors);
+ }
+
+ return 0;
+}
+
+BAREBOX_CMD_HELP_START(blkstats)
+BAREBOX_CMD_HELP_TEXT("Display a block device's number of read, written and erased sectors")
+BAREBOX_CMD_HELP_TEXT("")
+BAREBOX_CMD_HELP_TEXT("Options:")
+BAREBOX_CMD_HELP_OPT("-l", "list all currently registered block devices")
+BAREBOX_CMD_HELP_END
+
+BAREBOX_CMD_START(blkstats)
+ .cmd = do_blkstats,
+ BAREBOX_CMD_DESC("display block layer statistics")
+ BAREBOX_CMD_OPTS("[-l] [DEVICE]")
+ BAREBOX_CMD_GROUP(CMD_GRP_INFO)
+ BAREBOX_CMD_HELP(cmd_blkstats_help)
+BAREBOX_CMD_END
diff --git a/common/Kconfig b/common/Kconfig
index fea26262da86..4500feb66c92 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -41,6 +41,9 @@ config BLOCK
config BLOCK_WRITE
bool
+config BLOCK_STATS
+ bool
+
config FILETYPE
bool
diff --git a/common/block.c b/common/block.c
index b43fcbe6927b..92124a415cd9 100644
--- a/common/block.c
+++ b/common/block.c
@@ -31,19 +31,40 @@ static int writebuffer_io_len(struct block_device *blk, struct chunk *chunk)
return min_t(blkcnt_t, blk->rdbufsize, blk->num_blocks - chunk->block_start);
}
+#ifdef CONFIG_BLOCK_STATS
+static void blk_stats_record_read(struct block_device *blk, blkcnt_t count)
+{
+ blk->stats.read_sectors += count;
+}
+static void blk_stats_record_write(struct block_device *blk, blkcnt_t count)
+{
+ blk->stats.write_sectors += count;
+}
+static void blk_stats_record_erase(struct block_device *blk, blkcnt_t count)
+{
+ blk->stats.erase_sectors += count;
+}
+#else
+static void blk_stats_record_read(struct block_device *blk, blkcnt_t count) { }
+static void blk_stats_record_write(struct block_device *blk, blkcnt_t count) { }
+static void blk_stats_record_erase(struct block_device *blk, blkcnt_t count) { }
+#endif
+
static int chunk_flush(struct block_device *blk, struct chunk *chunk)
{
+ size_t len;
int ret;
if (!chunk->dirty)
return 0;
- ret = blk->ops->write(blk, chunk->data,
- chunk->block_start,
- writebuffer_io_len(blk, chunk));
+ len = writebuffer_io_len(blk, chunk);
+ ret = blk->ops->write(blk, chunk->data, chunk->block_start, len);
if (ret < 0)
return ret;
+ blk_stats_record_write(blk, len);
+
chunk->dirty = 0;
return 0;
@@ -144,6 +165,7 @@ static struct chunk *get_chunk(struct block_device *blk)
static int block_cache(struct block_device *blk, sector_t block)
{
struct chunk *chunk;
+ size_t len;
int ret;
chunk = get_chunk(blk);
@@ -155,20 +177,22 @@ static int block_cache(struct block_device *blk, sector_t block)
dev_dbg(blk->dev, "%s: %llu to %d\n", __func__, chunk->block_start,
chunk->num);
+ len = writebuffer_io_len(blk, chunk);
if (chunk->block_start * BLOCKSIZE(blk) >= blk->discard_start &&
- chunk->block_start * BLOCKSIZE(blk) + writebuffer_io_len(blk, chunk)
+ chunk->block_start * BLOCKSIZE(blk) + len
<= blk->discard_start + blk->discard_size) {
- memset(chunk->data, 0, writebuffer_io_len(blk, chunk));
+ memset(chunk->data, 0, len);
list_add(&chunk->list, &blk->buffered_blocks);
return 0;
}
- ret = blk->ops->read(blk, chunk->data, chunk->block_start,
- writebuffer_io_len(blk, chunk));
+ ret = blk->ops->read(blk, chunk->data, chunk->block_start, len);
if (ret) {
list_add_tail(&chunk->list, &blk->idle_blocks);
return ret;
}
+
+ blk_stats_record_read(blk, len);
list_add(&chunk->list, &blk->buffered_blocks);
return 0;
@@ -411,7 +435,13 @@ static __maybe_unused int block_op_erase(struct cdev *cdev, loff_t count, loff_t
}
}
- return blk->ops->erase(blk, offset, count);
+ ret = blk->ops->erase(blk, offset, count);
+ if (ret)
+ return ret;
+
+ blk_stats_record_erase(blk, count);
+
+ return 0;
}
static struct cdev_operations block_ops = {
diff --git a/include/block.h b/include/block.h
index eb319b953d32..b57d99a3fc08 100644
--- a/include/block.h
+++ b/include/block.h
@@ -31,6 +31,12 @@ enum blk_type {
const char *blk_type_str(enum blk_type);
+struct block_device_stats {
+ blkcnt_t read_sectors;
+ blkcnt_t write_sectors;
+ blkcnt_t erase_sectors;
+};
+
struct block_device {
struct device *dev;
struct list_head list;
@@ -50,6 +56,10 @@ struct block_device {
struct cdev cdev;
bool need_reparse;
+
+#ifdef CONFIG_BLOCK_STATS
+ struct block_device_stats stats;
+#endif
};
#define BLOCKSIZE(blk) (1u << (blk)->blockbits)
--
2.39.2
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 01/10] fs: give erase() a new erase_type parameter
2024-07-30 7:19 ` [PATCH 01/10] fs: give erase() a new erase_type parameter Ahmad Fatoum
@ 2024-07-30 8:31 ` Sascha Hauer
2024-07-30 8:32 ` Ahmad Fatoum
0 siblings, 1 reply; 29+ messages in thread
From: Sascha Hauer @ 2024-07-30 8:31 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On Tue, Jul 30, 2024 at 09:19:20AM +0200, Ahmad Fatoum wrote:
> Many managed flashes provide an erase operation, which need not be used
> for regular writes, but is provided nonetheless for efficient clearing
> of large regions of storage to a fixed bit pattern. The erase type allows
> users to specify _why_ the erase operation is done, so the driver or
> the core code can choose the most optimal operation.
>
> Existing code is annotated wither either ERASE_TO_WRITE for code that is
s/wither/with/
> immediately followed by a write and ERASE_TO_CLEAR for the standalone
> erase operations of the shell and fastboot erase command.
>
> Use of an enum allows the future addition of more types, e.g.:
>
> ERASE_TO_ALL_ZERO: to efficiently implement android sparse all-ones
> (like util-linux blkdiscard --zeroout)
> ERASE_TO_ALL_ONE: to efficiently implement android sparse all-zeroes
I suggest using ERASE_TO_ALL_ONE to implement sparse all-ones ;)
Will fix both of these while applying in case there's nothing else in
this series.
Sascha
> ERASE_TO_DISCARD: as alternative to ERASE_TO_CLEAR, when unwritten
> regions are indeed allowed to have arbitrary content
> (like util-linux blkdiscard without argument)
> ERASE_TO_SECURE: erase data in a manner appropriate for wiping secrets
> (like util-linux blkdiscard --secure)
>
> We don't introduce any functional change here yet, as no device yet sets
> DEVFS_WRITE_AUTOERASE. This will follow in a separate commit.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> arch/arm/mach-imx/imx-bbu-external-nand.c | 2 +-
> arch/arm/mach-imx/imx-bbu-internal.c | 4 ++--
> arch/arm/mach-omap/am33xx_bbu_spi_mlo.c | 2 +-
> commands/flash.c | 2 +-
> commands/nandtest.c | 2 +-
> common/bbu.c | 2 +-
> common/environment.c | 7 ++++--
> common/fastboot.c | 2 +-
> common/state/backend_bucket_circular.c | 2 +-
> drivers/misc/storage-by-uuid.c | 3 +++
> drivers/usb/gadget/function/dfu.c | 4 ++--
> fs/devfs-core.c | 7 +++++-
> fs/devfs.c | 5 ++++-
> fs/fs.c | 4 ++--
> include/driver.h | 13 ++++++++++++
> include/fs.h | 26 +++++++++++++++++++++--
> lib/libfile.c | 2 +-
> 17 files changed, 69 insertions(+), 20 deletions(-)
>
> diff --git a/arch/arm/mach-imx/imx-bbu-external-nand.c b/arch/arm/mach-imx/imx-bbu-external-nand.c
> index 7523008cdb5a..72c2271ff37a 100644
> --- a/arch/arm/mach-imx/imx-bbu-external-nand.c
> +++ b/arch/arm/mach-imx/imx-bbu-external-nand.c
> @@ -153,7 +153,7 @@ static int imx_bbu_external_nand_update(struct bbu_handler *handler, struct bbu_
>
> debug("writing %d bytes at 0x%08llx\n", now, nand_offset);
>
> - ret = erase(fd, blocksize, nand_offset);
> + ret = erase(fd, blocksize, nand_offset, ERASE_TO_WRITE);
> if (ret)
> goto out;
>
> diff --git a/arch/arm/mach-imx/imx-bbu-internal.c b/arch/arm/mach-imx/imx-bbu-internal.c
> index 8cdaab5c1676..4190d60d18dd 100644
> --- a/arch/arm/mach-imx/imx-bbu-internal.c
> +++ b/arch/arm/mach-imx/imx-bbu-internal.c
> @@ -108,7 +108,7 @@ static int imx_bbu_write_device(struct imx_internal_bbu_handler *imx_handler,
> if (imx_bbu_erase_required(imx_handler)) {
> pr_debug("%s: erasing %s from 0x%08x to 0x%08x\n", __func__,
> devicefile, offset, image_len);
> - ret = erase(fd, image_len, offset);
> + ret = erase(fd, image_len, offset, ERASE_TO_WRITE);
> if (ret) {
> pr_err("erasing %s failed with %s\n", devicefile,
> strerror(-ret));
> @@ -340,7 +340,7 @@ static int imx_bbu_internal_v2_write_nand_dbbt(struct imx_internal_bbu_handler *
>
> pr_debug("writing %d bytes at 0x%08llx\n", now, offset);
>
> - ret = erase(fd, blocksize, offset);
> + ret = erase(fd, blocksize, offset, ERASE_TO_WRITE);
> if (ret)
> goto out;
>
> diff --git a/arch/arm/mach-omap/am33xx_bbu_spi_mlo.c b/arch/arm/mach-omap/am33xx_bbu_spi_mlo.c
> index 2c58c9ae690e..951c3f8ce099 100644
> --- a/arch/arm/mach-omap/am33xx_bbu_spi_mlo.c
> +++ b/arch/arm/mach-omap/am33xx_bbu_spi_mlo.c
> @@ -71,7 +71,7 @@ static int spi_nor_mlo_handler(struct bbu_handler *handler,
> goto out;
> }
>
> - ret = erase(dstfd, ERASE_SIZE_ALL, 0);
> + ret = erase(dstfd, ERASE_SIZE_ALL, 0, ERASE_TO_WRITE);
> if (ret < 0) {
> printf("could not erase %s: %m", data->devicefile);
> goto out1;
> diff --git a/commands/flash.c b/commands/flash.c
> index 5b7060aeadfb..de3bf65e5059 100644
> --- a/commands/flash.c
> +++ b/commands/flash.c
> @@ -43,7 +43,7 @@ static int do_flerase(int argc, char *argv[])
> goto out;
> }
>
> - if (erase(fd, size, start)) {
> + if (erase(fd, size, start, ERASE_TO_CLEAR)) {
> perror("erase");
> ret = 1;
> }
> diff --git a/commands/nandtest.c b/commands/nandtest.c
> index ba7c87a32eea..bc138646a460 100644
> --- a/commands/nandtest.c
> +++ b/commands/nandtest.c
> @@ -160,7 +160,7 @@ static int erase_and_write(loff_t ofs, unsigned char *data,
> er.start = ofs;
> er.length = meminfo.erasesize;
>
> - ret = erase(fd, er.length, er.start);
> + ret = erase(fd, er.length, er.start, ERASE_TO_WRITE);
> if (ret < 0) {
> perror("\nerase");
> printf("Could't not erase flash at 0x%08llx length 0x%08llx.\n",
> diff --git a/common/bbu.c b/common/bbu.c
> index f71747aa2f6d..9ea8a1a3f247 100644
> --- a/common/bbu.c
> +++ b/common/bbu.c
> @@ -453,7 +453,7 @@ int bbu_std_file_handler(struct bbu_handler *handler,
> goto err_close;
> }
>
> - ret = erase(fd, data->len, 0);
> + ret = erase(fd, data->len, 0, ERASE_TO_WRITE);
> if (ret && ret != -ENOSYS) {
> printf("erasing %s failed with %s\n", data->devicefile,
> strerror(-ret));
> diff --git a/common/environment.c b/common/environment.c
> index 39cad0c16a77..dbf4c2f52365 100644
> --- a/common/environment.c
> +++ b/common/environment.c
> @@ -152,7 +152,10 @@ static inline int protect(int fd, size_t count, unsigned long offset, int prot)
> return 0;
> }
>
> -static inline int erase(int fd, size_t count, unsigned long offset)
> +enum erase_type {
> + ERASE_TO_WRITE
> +};
> +static inline int erase(int fd, size_t count, unsigned long offset, enum erase_type type)
> {
> return 0;
> }
> @@ -380,7 +383,7 @@ int envfs_save(const char *filename, const char *dirname, unsigned flags)
> goto out;
> }
>
> - ret = erase(envfd, ERASE_SIZE_ALL, 0);
> + ret = erase(envfd, ERASE_SIZE_ALL, 0, ERASE_TO_WRITE);
>
> /* ENOSYS and EOPNOTSUPP aren't errors here, many devices don't need it */
> if (ret && errno != ENOSYS && errno != EOPNOTSUPP) {
> diff --git a/common/fastboot.c b/common/fastboot.c
> index f8a01dea7a65..d283fc8fc098 100644
> --- a/common/fastboot.c
> +++ b/common/fastboot.c
> @@ -789,7 +789,7 @@ static void cb_erase(struct fastboot *fb, const char *cmd)
> if (fd < 0)
> fastboot_tx_print(fb, FASTBOOT_MSG_FAIL, strerror(-fd));
>
> - ret = erase(fd, ERASE_SIZE_ALL, 0);
> + ret = erase(fd, ERASE_SIZE_ALL, 0, ERASE_TO_CLEAR);
>
> close(fd);
>
> diff --git a/common/state/backend_bucket_circular.c b/common/state/backend_bucket_circular.c
> index 2ac5bf6a8218..6b5873aa9af1 100644
> --- a/common/state/backend_bucket_circular.c
> +++ b/common/state/backend_bucket_circular.c
> @@ -220,7 +220,7 @@ static int state_mtd_peb_write(struct state_backend_storage_bucket_circular *cir
> static int state_mtd_peb_erase(struct state_backend_storage_bucket_circular *circ)
> {
> return erase(circ->fd, circ->mtd->erasesize,
> - (off_t)circ->eraseblock * circ->mtd->erasesize);
> + (off_t)circ->eraseblock * circ->mtd->erasesize, ERASE_TO_WRITE);
> }
> #endif
>
> diff --git a/drivers/misc/storage-by-uuid.c b/drivers/misc/storage-by-uuid.c
> index db15b64d7dcf..8b8fd901685e 100644
> --- a/drivers/misc/storage-by-uuid.c
> +++ b/drivers/misc/storage-by-uuid.c
> @@ -126,6 +126,9 @@ static void storage_by_uuid_add_partitions(struct sbu *sbu, struct cdev *rcdev)
> sbu->cdev.dev = sbu->dev;
> sbu->cdev.priv = sbu;
>
> + if (rcdev->flags & DEVFS_WRITE_AUTOERASE)
> + sbu->cdev.flags |= DEVFS_WRITE_AUTOERASE;
> +
> ret = devfs_create(&sbu->cdev);
> if (ret) {
> dev_err(sbu->dev, "Failed to create cdev: %s\n", strerror(-ret));
> diff --git a/drivers/usb/gadget/function/dfu.c b/drivers/usb/gadget/function/dfu.c
> index a41ff22dcebc..208e0cd23c29 100644
> --- a/drivers/usb/gadget/function/dfu.c
> +++ b/drivers/usb/gadget/function/dfu.c
> @@ -208,7 +208,7 @@ static void dfu_do_write(struct dfu_work *dw)
>
> if (prog_erase && (dfu_written + wlen) > dfu_erased) {
> size = roundup(wlen, dfu_mtdinfo.erasesize);
> - ret = erase(dfufd, size, dfu_erased);
> + ret = erase(dfufd, size, dfu_erased, ERASE_TO_WRITE);
> dfu_erased += size;
> if (ret && ret != -ENOSYS) {
> perror("erase");
> @@ -333,7 +333,7 @@ static void dfu_do_copy(struct dfu_work *dw)
> return;
> }
>
> - ret = erase(fd, ERASE_SIZE_ALL, 0);
> + ret = erase_flash(fd, ERASE_SIZE_ALL, 0);
> close(fd);
> if (ret && ret != -ENOSYS) {
> perror("erase");
> diff --git a/fs/devfs-core.c b/fs/devfs-core.c
> index ed445fbd4712..2715193c6956 100644
> --- a/fs/devfs-core.c
> +++ b/fs/devfs-core.c
> @@ -509,6 +509,7 @@ static struct cdev *__devfs_add_partition(struct cdev *cdev,
> loff_t _end = end ? *end : 0;
> static struct cdev *new;
> struct cdev *overlap;
> + unsigned inherited_flags = 0;
>
> if (cdev_by_name(partinfo->name))
> return ERR_PTR(-EEXIST);
> @@ -551,11 +552,14 @@ static struct cdev *__devfs_add_partition(struct cdev *cdev,
> return overlap;
> }
>
> + /* Filter flags that we want to pass along to children */
> + inherited_flags |= cdev->flags & DEVFS_WRITE_AUTOERASE;
> +
> if (IS_ENABLED(CONFIG_MTD) && cdev->mtd) {
> struct mtd_info *mtd;
>
> mtd = mtd_add_partition(cdev->mtd, offset, size,
> - partinfo->flags, partinfo->name);
> + partinfo->flags | inherited_flags, partinfo->name);
> if (IS_ERR(mtd))
> return (void *)mtd;
>
> @@ -571,6 +575,7 @@ static struct cdev *__devfs_add_partition(struct cdev *cdev,
> new->priv = cdev->priv;
> new->size = size;
> new->offset = cdev->offset + offset;
> + new->flags = inherited_flags;
>
> new->dev = cdev->dev;
> new->master = cdev;
> diff --git a/fs/devfs.c b/fs/devfs.c
> index 9dbfa91b1d9f..5feb76cfef86 100644
> --- a/fs/devfs.c
> +++ b/fs/devfs.c
> @@ -61,13 +61,16 @@ static int devfs_lseek(struct device *_dev, FILE *f, loff_t pos)
> }
>
> static int devfs_erase(struct device *_dev, FILE *f, loff_t count,
> - loff_t offset)
> + loff_t offset, enum erase_type type)
> {
> struct cdev *cdev = f->priv;
>
> if (cdev->flags & DEVFS_PARTITION_READONLY)
> return -EPERM;
>
> + if ((type == ERASE_TO_WRITE) && (cdev->flags & DEVFS_WRITE_AUTOERASE))
> + return 0;
> +
> if (count + offset > cdev->size)
> count = cdev->size - offset;
>
> diff --git a/fs/fs.c b/fs/fs.c
> index 656434f67ffa..5e7e05f4990a 100644
> --- a/fs/fs.c
> +++ b/fs/fs.c
> @@ -600,7 +600,7 @@ loff_t lseek(int fd, loff_t offset, int whence)
> }
> EXPORT_SYMBOL(lseek);
>
> -int erase(int fd, loff_t count, loff_t offset)
> +int erase(int fd, loff_t count, loff_t offset, enum erase_type type)
> {
> struct fs_driver *fsdrv;
> FILE *f = fd_to_file(fd, false);
> @@ -621,7 +621,7 @@ int erase(int fd, loff_t count, loff_t offset)
> assert_command_context();
>
> if (fsdrv->erase)
> - ret = fsdrv->erase(&f->fsdev->dev, f, count, offset);
> + ret = fsdrv->erase(&f->fsdev->dev, f, count, offset, type);
> else
> ret = -ENOSYS;
>
> diff --git a/include/driver.h b/include/driver.h
> index 2b4db2221a6c..c4067d531cc7 100644
> --- a/include/driver.h
> +++ b/include/driver.h
> @@ -562,6 +562,19 @@ extern struct list_head cdev_list;
> #define DEVFS_PARTITION_BOOTABLE_LEGACY (1U << 11)
> #define DEVFS_PARTITION_BOOTABLE_ESP (1U << 12)
> #define DEVFS_PARTITION_FOR_FIXUP (1U << 13)
> +#define DEVFS_WRITE_AUTOERASE (1U << 14)
> +
> +/**
> + * cdev_write_requires_erase - Check whether writes must be done to erased blocks
> + * @cdev: pointer to cdev structure
> + *
> + * This function checks if the erase operation is required, so a subsequent
> + * write operation can write all bits.
> + */
> +static inline bool cdev_write_requires_erase(const struct cdev *cdev)
> +{
> + return !(cdev->flags & DEVFS_WRITE_AUTOERASE);
> +}
>
> static inline bool cdev_is_mbr_partitioned(const struct cdev *master)
> {
> diff --git a/include/fs.h b/include/fs.h
> index f87c4c40273f..f5950a052923 100644
> --- a/include/fs.h
> +++ b/include/fs.h
> @@ -10,6 +10,7 @@
> #include <sys/stat.h>
> #include <driver.h>
> #include <filetype.h>
> +#include <linux/bits.h>
> #include <linux/fs.h>
> #include <linux/string.h>
> #include <linux/limits.h>
> @@ -38,6 +39,27 @@ typedef struct filep {
>
> #define FS_DRIVER_NO_DEV 1
>
> +/**
> + * enum erase_type - Type of erase operation
> + * @ERASE_TO_WRITE: Conduct low-level erase operation to prepare for a write
> + * to all or part of the erased regions. This is required
> + * for raw flashes, but can be omitted by flashes behind
> + * a FTL that autoerases on write (e.g. eMMCs)
> + * @ERASE_TO_CLEAR: Force an erase of the region. When read afterwards,
> + * A fixed pattern should result, usually either all-zeroes
> + * or all-ones depending on storage technology.
> + *
> + * Many managed flashes provide an erase operation, which need not be used
> + * for regular writes, but is provided nonetheless for efficient clearing
> + * of large regions of storage to a fixed bit pattern. The erase type allows
> + * users to specify _why_ the erase operation is done, so the driver or
> + * the core code can choose the most optimal operation.
> + */
> +enum erase_type {
> + ERASE_TO_WRITE,
> + ERASE_TO_CLEAR
> +};
> +
> struct fs_driver {
> int (*probe) (struct device *dev);
>
> @@ -58,7 +80,7 @@ struct fs_driver {
>
> int (*ioctl)(struct device *dev, FILE *f, unsigned int request, void *buf);
> int (*erase)(struct device *dev, FILE *f, loff_t count,
> - loff_t offset);
> + loff_t offset, enum erase_type type);
> int (*protect)(struct device *dev, FILE *f, size_t count,
> loff_t offset, int prot);
> int (*discard_range)(struct device *dev, FILE *f, loff_t count,
> @@ -127,7 +149,7 @@ int umount_by_cdev(struct cdev *cdev);
>
> /* not-so-standard functions */
> #define ERASE_SIZE_ALL ((loff_t) - 1)
> -int erase(int fd, loff_t count, loff_t offset);
> +int erase(int fd, loff_t count, loff_t offset, enum erase_type type);
> int protect(int fd, size_t count, loff_t offset, int prot);
> int discard_range(int fd, loff_t count, loff_t offset);
> int protect_file(const char *file, int prot);
> diff --git a/lib/libfile.c b/lib/libfile.c
> index a34e011f4f7c..80d4591dd6e5 100644
> --- a/lib/libfile.c
> +++ b/lib/libfile.c
> @@ -412,7 +412,7 @@ int write_file_flash(const char *filename, const void *buf, size_t size)
> if (fd < 0)
> return fd;
>
> - ret = erase(fd, size, 0);
> + ret = erase(fd, size, 0, ERASE_TO_WRITE);
> if (ret < 0)
> goto out_close;
>
> --
> 2.39.2
>
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 01/10] fs: give erase() a new erase_type parameter
2024-07-30 8:31 ` Sascha Hauer
@ 2024-07-30 8:32 ` Ahmad Fatoum
0 siblings, 0 replies; 29+ messages in thread
From: Ahmad Fatoum @ 2024-07-30 8:32 UTC (permalink / raw)
To: Sascha Hauer; +Cc: barebox
On 7/30/24 10:31, Sascha Hauer wrote:
> On Tue, Jul 30, 2024 at 09:19:20AM +0200, Ahmad Fatoum wrote:
>> Many managed flashes provide an erase operation, which need not be used
>> for regular writes, but is provided nonetheless for efficient clearing
>> of large regions of storage to a fixed bit pattern. The erase type allows
>> users to specify _why_ the erase operation is done, so the driver or
>> the core code can choose the most optimal operation.
>>
>> Existing code is annotated wither either ERASE_TO_WRITE for code that is
>
> s/wither/with/
>
>> immediately followed by a write and ERASE_TO_CLEAR for the standalone
>> erase operations of the shell and fastboot erase command.
>>
>> Use of an enum allows the future addition of more types, e.g.:
>>
>> ERASE_TO_ALL_ZERO: to efficiently implement android sparse all-ones
>> (like util-linux blkdiscard --zeroout)
>> ERASE_TO_ALL_ONE: to efficiently implement android sparse all-zeroes
>
> I suggest using ERASE_TO_ALL_ONE to implement sparse all-ones ;)
>
> Will fix both of these while applying in case there's nothing else in
> this series.
Thanks! :)
>
> Sascha
>
>> ERASE_TO_DISCARD: as alternative to ERASE_TO_CLEAR, when unwritten
>> regions are indeed allowed to have arbitrary content
>> (like util-linux blkdiscard without argument)
>> ERASE_TO_SECURE: erase data in a manner appropriate for wiping secrets
>> (like util-linux blkdiscard --secure)
>>
>> We don't introduce any functional change here yet, as no device yet sets
>> DEVFS_WRITE_AUTOERASE. This will follow in a separate commit.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>> arch/arm/mach-imx/imx-bbu-external-nand.c | 2 +-
>> arch/arm/mach-imx/imx-bbu-internal.c | 4 ++--
>> arch/arm/mach-omap/am33xx_bbu_spi_mlo.c | 2 +-
>> commands/flash.c | 2 +-
>> commands/nandtest.c | 2 +-
>> common/bbu.c | 2 +-
>> common/environment.c | 7 ++++--
>> common/fastboot.c | 2 +-
>> common/state/backend_bucket_circular.c | 2 +-
>> drivers/misc/storage-by-uuid.c | 3 +++
>> drivers/usb/gadget/function/dfu.c | 4 ++--
>> fs/devfs-core.c | 7 +++++-
>> fs/devfs.c | 5 ++++-
>> fs/fs.c | 4 ++--
>> include/driver.h | 13 ++++++++++++
>> include/fs.h | 26 +++++++++++++++++++++--
>> lib/libfile.c | 2 +-
>> 17 files changed, 69 insertions(+), 20 deletions(-)
>>
>> diff --git a/arch/arm/mach-imx/imx-bbu-external-nand.c b/arch/arm/mach-imx/imx-bbu-external-nand.c
>> index 7523008cdb5a..72c2271ff37a 100644
>> --- a/arch/arm/mach-imx/imx-bbu-external-nand.c
>> +++ b/arch/arm/mach-imx/imx-bbu-external-nand.c
>> @@ -153,7 +153,7 @@ static int imx_bbu_external_nand_update(struct bbu_handler *handler, struct bbu_
>>
>> debug("writing %d bytes at 0x%08llx\n", now, nand_offset);
>>
>> - ret = erase(fd, blocksize, nand_offset);
>> + ret = erase(fd, blocksize, nand_offset, ERASE_TO_WRITE);
>> if (ret)
>> goto out;
>>
>> diff --git a/arch/arm/mach-imx/imx-bbu-internal.c b/arch/arm/mach-imx/imx-bbu-internal.c
>> index 8cdaab5c1676..4190d60d18dd 100644
>> --- a/arch/arm/mach-imx/imx-bbu-internal.c
>> +++ b/arch/arm/mach-imx/imx-bbu-internal.c
>> @@ -108,7 +108,7 @@ static int imx_bbu_write_device(struct imx_internal_bbu_handler *imx_handler,
>> if (imx_bbu_erase_required(imx_handler)) {
>> pr_debug("%s: erasing %s from 0x%08x to 0x%08x\n", __func__,
>> devicefile, offset, image_len);
>> - ret = erase(fd, image_len, offset);
>> + ret = erase(fd, image_len, offset, ERASE_TO_WRITE);
>> if (ret) {
>> pr_err("erasing %s failed with %s\n", devicefile,
>> strerror(-ret));
>> @@ -340,7 +340,7 @@ static int imx_bbu_internal_v2_write_nand_dbbt(struct imx_internal_bbu_handler *
>>
>> pr_debug("writing %d bytes at 0x%08llx\n", now, offset);
>>
>> - ret = erase(fd, blocksize, offset);
>> + ret = erase(fd, blocksize, offset, ERASE_TO_WRITE);
>> if (ret)
>> goto out;
>>
>> diff --git a/arch/arm/mach-omap/am33xx_bbu_spi_mlo.c b/arch/arm/mach-omap/am33xx_bbu_spi_mlo.c
>> index 2c58c9ae690e..951c3f8ce099 100644
>> --- a/arch/arm/mach-omap/am33xx_bbu_spi_mlo.c
>> +++ b/arch/arm/mach-omap/am33xx_bbu_spi_mlo.c
>> @@ -71,7 +71,7 @@ static int spi_nor_mlo_handler(struct bbu_handler *handler,
>> goto out;
>> }
>>
>> - ret = erase(dstfd, ERASE_SIZE_ALL, 0);
>> + ret = erase(dstfd, ERASE_SIZE_ALL, 0, ERASE_TO_WRITE);
>> if (ret < 0) {
>> printf("could not erase %s: %m", data->devicefile);
>> goto out1;
>> diff --git a/commands/flash.c b/commands/flash.c
>> index 5b7060aeadfb..de3bf65e5059 100644
>> --- a/commands/flash.c
>> +++ b/commands/flash.c
>> @@ -43,7 +43,7 @@ static int do_flerase(int argc, char *argv[])
>> goto out;
>> }
>>
>> - if (erase(fd, size, start)) {
>> + if (erase(fd, size, start, ERASE_TO_CLEAR)) {
>> perror("erase");
>> ret = 1;
>> }
>> diff --git a/commands/nandtest.c b/commands/nandtest.c
>> index ba7c87a32eea..bc138646a460 100644
>> --- a/commands/nandtest.c
>> +++ b/commands/nandtest.c
>> @@ -160,7 +160,7 @@ static int erase_and_write(loff_t ofs, unsigned char *data,
>> er.start = ofs;
>> er.length = meminfo.erasesize;
>>
>> - ret = erase(fd, er.length, er.start);
>> + ret = erase(fd, er.length, er.start, ERASE_TO_WRITE);
>> if (ret < 0) {
>> perror("\nerase");
>> printf("Could't not erase flash at 0x%08llx length 0x%08llx.\n",
>> diff --git a/common/bbu.c b/common/bbu.c
>> index f71747aa2f6d..9ea8a1a3f247 100644
>> --- a/common/bbu.c
>> +++ b/common/bbu.c
>> @@ -453,7 +453,7 @@ int bbu_std_file_handler(struct bbu_handler *handler,
>> goto err_close;
>> }
>>
>> - ret = erase(fd, data->len, 0);
>> + ret = erase(fd, data->len, 0, ERASE_TO_WRITE);
>> if (ret && ret != -ENOSYS) {
>> printf("erasing %s failed with %s\n", data->devicefile,
>> strerror(-ret));
>> diff --git a/common/environment.c b/common/environment.c
>> index 39cad0c16a77..dbf4c2f52365 100644
>> --- a/common/environment.c
>> +++ b/common/environment.c
>> @@ -152,7 +152,10 @@ static inline int protect(int fd, size_t count, unsigned long offset, int prot)
>> return 0;
>> }
>>
>> -static inline int erase(int fd, size_t count, unsigned long offset)
>> +enum erase_type {
>> + ERASE_TO_WRITE
>> +};
>> +static inline int erase(int fd, size_t count, unsigned long offset, enum erase_type type)
>> {
>> return 0;
>> }
>> @@ -380,7 +383,7 @@ int envfs_save(const char *filename, const char *dirname, unsigned flags)
>> goto out;
>> }
>>
>> - ret = erase(envfd, ERASE_SIZE_ALL, 0);
>> + ret = erase(envfd, ERASE_SIZE_ALL, 0, ERASE_TO_WRITE);
>>
>> /* ENOSYS and EOPNOTSUPP aren't errors here, many devices don't need it */
>> if (ret && errno != ENOSYS && errno != EOPNOTSUPP) {
>> diff --git a/common/fastboot.c b/common/fastboot.c
>> index f8a01dea7a65..d283fc8fc098 100644
>> --- a/common/fastboot.c
>> +++ b/common/fastboot.c
>> @@ -789,7 +789,7 @@ static void cb_erase(struct fastboot *fb, const char *cmd)
>> if (fd < 0)
>> fastboot_tx_print(fb, FASTBOOT_MSG_FAIL, strerror(-fd));
>>
>> - ret = erase(fd, ERASE_SIZE_ALL, 0);
>> + ret = erase(fd, ERASE_SIZE_ALL, 0, ERASE_TO_CLEAR);
>>
>> close(fd);
>>
>> diff --git a/common/state/backend_bucket_circular.c b/common/state/backend_bucket_circular.c
>> index 2ac5bf6a8218..6b5873aa9af1 100644
>> --- a/common/state/backend_bucket_circular.c
>> +++ b/common/state/backend_bucket_circular.c
>> @@ -220,7 +220,7 @@ static int state_mtd_peb_write(struct state_backend_storage_bucket_circular *cir
>> static int state_mtd_peb_erase(struct state_backend_storage_bucket_circular *circ)
>> {
>> return erase(circ->fd, circ->mtd->erasesize,
>> - (off_t)circ->eraseblock * circ->mtd->erasesize);
>> + (off_t)circ->eraseblock * circ->mtd->erasesize, ERASE_TO_WRITE);
>> }
>> #endif
>>
>> diff --git a/drivers/misc/storage-by-uuid.c b/drivers/misc/storage-by-uuid.c
>> index db15b64d7dcf..8b8fd901685e 100644
>> --- a/drivers/misc/storage-by-uuid.c
>> +++ b/drivers/misc/storage-by-uuid.c
>> @@ -126,6 +126,9 @@ static void storage_by_uuid_add_partitions(struct sbu *sbu, struct cdev *rcdev)
>> sbu->cdev.dev = sbu->dev;
>> sbu->cdev.priv = sbu;
>>
>> + if (rcdev->flags & DEVFS_WRITE_AUTOERASE)
>> + sbu->cdev.flags |= DEVFS_WRITE_AUTOERASE;
>> +
>> ret = devfs_create(&sbu->cdev);
>> if (ret) {
>> dev_err(sbu->dev, "Failed to create cdev: %s\n", strerror(-ret));
>> diff --git a/drivers/usb/gadget/function/dfu.c b/drivers/usb/gadget/function/dfu.c
>> index a41ff22dcebc..208e0cd23c29 100644
>> --- a/drivers/usb/gadget/function/dfu.c
>> +++ b/drivers/usb/gadget/function/dfu.c
>> @@ -208,7 +208,7 @@ static void dfu_do_write(struct dfu_work *dw)
>>
>> if (prog_erase && (dfu_written + wlen) > dfu_erased) {
>> size = roundup(wlen, dfu_mtdinfo.erasesize);
>> - ret = erase(dfufd, size, dfu_erased);
>> + ret = erase(dfufd, size, dfu_erased, ERASE_TO_WRITE);
>> dfu_erased += size;
>> if (ret && ret != -ENOSYS) {
>> perror("erase");
>> @@ -333,7 +333,7 @@ static void dfu_do_copy(struct dfu_work *dw)
>> return;
>> }
>>
>> - ret = erase(fd, ERASE_SIZE_ALL, 0);
>> + ret = erase_flash(fd, ERASE_SIZE_ALL, 0);
>> close(fd);
>> if (ret && ret != -ENOSYS) {
>> perror("erase");
>> diff --git a/fs/devfs-core.c b/fs/devfs-core.c
>> index ed445fbd4712..2715193c6956 100644
>> --- a/fs/devfs-core.c
>> +++ b/fs/devfs-core.c
>> @@ -509,6 +509,7 @@ static struct cdev *__devfs_add_partition(struct cdev *cdev,
>> loff_t _end = end ? *end : 0;
>> static struct cdev *new;
>> struct cdev *overlap;
>> + unsigned inherited_flags = 0;
>>
>> if (cdev_by_name(partinfo->name))
>> return ERR_PTR(-EEXIST);
>> @@ -551,11 +552,14 @@ static struct cdev *__devfs_add_partition(struct cdev *cdev,
>> return overlap;
>> }
>>
>> + /* Filter flags that we want to pass along to children */
>> + inherited_flags |= cdev->flags & DEVFS_WRITE_AUTOERASE;
>> +
>> if (IS_ENABLED(CONFIG_MTD) && cdev->mtd) {
>> struct mtd_info *mtd;
>>
>> mtd = mtd_add_partition(cdev->mtd, offset, size,
>> - partinfo->flags, partinfo->name);
>> + partinfo->flags | inherited_flags, partinfo->name);
>> if (IS_ERR(mtd))
>> return (void *)mtd;
>>
>> @@ -571,6 +575,7 @@ static struct cdev *__devfs_add_partition(struct cdev *cdev,
>> new->priv = cdev->priv;
>> new->size = size;
>> new->offset = cdev->offset + offset;
>> + new->flags = inherited_flags;
>>
>> new->dev = cdev->dev;
>> new->master = cdev;
>> diff --git a/fs/devfs.c b/fs/devfs.c
>> index 9dbfa91b1d9f..5feb76cfef86 100644
>> --- a/fs/devfs.c
>> +++ b/fs/devfs.c
>> @@ -61,13 +61,16 @@ static int devfs_lseek(struct device *_dev, FILE *f, loff_t pos)
>> }
>>
>> static int devfs_erase(struct device *_dev, FILE *f, loff_t count,
>> - loff_t offset)
>> + loff_t offset, enum erase_type type)
>> {
>> struct cdev *cdev = f->priv;
>>
>> if (cdev->flags & DEVFS_PARTITION_READONLY)
>> return -EPERM;
>>
>> + if ((type == ERASE_TO_WRITE) && (cdev->flags & DEVFS_WRITE_AUTOERASE))
>> + return 0;
>> +
>> if (count + offset > cdev->size)
>> count = cdev->size - offset;
>>
>> diff --git a/fs/fs.c b/fs/fs.c
>> index 656434f67ffa..5e7e05f4990a 100644
>> --- a/fs/fs.c
>> +++ b/fs/fs.c
>> @@ -600,7 +600,7 @@ loff_t lseek(int fd, loff_t offset, int whence)
>> }
>> EXPORT_SYMBOL(lseek);
>>
>> -int erase(int fd, loff_t count, loff_t offset)
>> +int erase(int fd, loff_t count, loff_t offset, enum erase_type type)
>> {
>> struct fs_driver *fsdrv;
>> FILE *f = fd_to_file(fd, false);
>> @@ -621,7 +621,7 @@ int erase(int fd, loff_t count, loff_t offset)
>> assert_command_context();
>>
>> if (fsdrv->erase)
>> - ret = fsdrv->erase(&f->fsdev->dev, f, count, offset);
>> + ret = fsdrv->erase(&f->fsdev->dev, f, count, offset, type);
>> else
>> ret = -ENOSYS;
>>
>> diff --git a/include/driver.h b/include/driver.h
>> index 2b4db2221a6c..c4067d531cc7 100644
>> --- a/include/driver.h
>> +++ b/include/driver.h
>> @@ -562,6 +562,19 @@ extern struct list_head cdev_list;
>> #define DEVFS_PARTITION_BOOTABLE_LEGACY (1U << 11)
>> #define DEVFS_PARTITION_BOOTABLE_ESP (1U << 12)
>> #define DEVFS_PARTITION_FOR_FIXUP (1U << 13)
>> +#define DEVFS_WRITE_AUTOERASE (1U << 14)
>> +
>> +/**
>> + * cdev_write_requires_erase - Check whether writes must be done to erased blocks
>> + * @cdev: pointer to cdev structure
>> + *
>> + * This function checks if the erase operation is required, so a subsequent
>> + * write operation can write all bits.
>> + */
>> +static inline bool cdev_write_requires_erase(const struct cdev *cdev)
>> +{
>> + return !(cdev->flags & DEVFS_WRITE_AUTOERASE);
>> +}
>>
>> static inline bool cdev_is_mbr_partitioned(const struct cdev *master)
>> {
>> diff --git a/include/fs.h b/include/fs.h
>> index f87c4c40273f..f5950a052923 100644
>> --- a/include/fs.h
>> +++ b/include/fs.h
>> @@ -10,6 +10,7 @@
>> #include <sys/stat.h>
>> #include <driver.h>
>> #include <filetype.h>
>> +#include <linux/bits.h>
>> #include <linux/fs.h>
>> #include <linux/string.h>
>> #include <linux/limits.h>
>> @@ -38,6 +39,27 @@ typedef struct filep {
>>
>> #define FS_DRIVER_NO_DEV 1
>>
>> +/**
>> + * enum erase_type - Type of erase operation
>> + * @ERASE_TO_WRITE: Conduct low-level erase operation to prepare for a write
>> + * to all or part of the erased regions. This is required
>> + * for raw flashes, but can be omitted by flashes behind
>> + * a FTL that autoerases on write (e.g. eMMCs)
>> + * @ERASE_TO_CLEAR: Force an erase of the region. When read afterwards,
>> + * A fixed pattern should result, usually either all-zeroes
>> + * or all-ones depending on storage technology.
>> + *
>> + * Many managed flashes provide an erase operation, which need not be used
>> + * for regular writes, but is provided nonetheless for efficient clearing
>> + * of large regions of storage to a fixed bit pattern. The erase type allows
>> + * users to specify _why_ the erase operation is done, so the driver or
>> + * the core code can choose the most optimal operation.
>> + */
>> +enum erase_type {
>> + ERASE_TO_WRITE,
>> + ERASE_TO_CLEAR
>> +};
>> +
>> struct fs_driver {
>> int (*probe) (struct device *dev);
>>
>> @@ -58,7 +80,7 @@ struct fs_driver {
>>
>> int (*ioctl)(struct device *dev, FILE *f, unsigned int request, void *buf);
>> int (*erase)(struct device *dev, FILE *f, loff_t count,
>> - loff_t offset);
>> + loff_t offset, enum erase_type type);
>> int (*protect)(struct device *dev, FILE *f, size_t count,
>> loff_t offset, int prot);
>> int (*discard_range)(struct device *dev, FILE *f, loff_t count,
>> @@ -127,7 +149,7 @@ int umount_by_cdev(struct cdev *cdev);
>>
>> /* not-so-standard functions */
>> #define ERASE_SIZE_ALL ((loff_t) - 1)
>> -int erase(int fd, loff_t count, loff_t offset);
>> +int erase(int fd, loff_t count, loff_t offset, enum erase_type type);
>> int protect(int fd, size_t count, loff_t offset, int prot);
>> int discard_range(int fd, loff_t count, loff_t offset);
>> int protect_file(const char *file, int prot);
>> diff --git a/lib/libfile.c b/lib/libfile.c
>> index a34e011f4f7c..80d4591dd6e5 100644
>> --- a/lib/libfile.c
>> +++ b/lib/libfile.c
>> @@ -412,7 +412,7 @@ int write_file_flash(const char *filename, const void *buf, size_t size)
>> if (fd < 0)
>> return fd;
>>
>> - ret = erase(fd, size, 0);
>> + ret = erase(fd, size, 0, ERASE_TO_WRITE);
>> if (ret < 0)
>> goto out_close;
>>
>> --
>> 2.39.2
>>
>>
>>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 03/10] block: allow block devices to implement the cdev erase operation
2024-07-30 7:19 ` [PATCH 03/10] block: allow block devices to implement the cdev erase operation Ahmad Fatoum
@ 2024-07-30 8:55 ` Sascha Hauer
2024-07-30 11:10 ` Ahmad Fatoum
0 siblings, 1 reply; 29+ messages in thread
From: Sascha Hauer @ 2024-07-30 8:55 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On Tue, Jul 30, 2024 at 09:19:22AM +0200, Ahmad Fatoum wrote:
> We mainly implement cdev erase for raw flashes. Many block devices, like
> NVMe, SSD or SD/MMC are also flash-based and expose a mechanism or
> multiple to trigger an erase on hardware or flash translation layer
> level without an accompanied write.
>
> To make it possible to use this functionality elsewhere in barebox,
> let's have the common block device layer pass through the erase
> operation, so it may be implemented in a device-specific manner.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> common/block.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> include/block.h | 1 +
> 2 files changed, 44 insertions(+)
>
> diff --git a/common/block.c b/common/block.c
> index f55da775a797..b43fcbe6927b 100644
> --- a/common/block.c
> +++ b/common/block.c
> @@ -380,10 +380,45 @@ static int block_op_discard_range(struct cdev *cdev, loff_t count, loff_t offset
> return 0;
> }
>
> +static bool region_overlap(loff_t starta, loff_t lena,
> + loff_t startb, loff_t lenb)
> +{
> + if (starta + lena <= startb)
> + return 0;
> + if (startb + lenb <= starta)
> + return 0;
> + return 1;
> +}
We already have this function in fs/devfs-core.c. Time to export it?
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 04/10] mci: turn bool members into bitfield in struct mci
2024-07-30 7:19 ` [PATCH 04/10] mci: turn bool members into bitfield in struct mci Ahmad Fatoum
@ 2024-07-30 9:06 ` Sascha Hauer
2024-07-30 11:10 ` Ahmad Fatoum
0 siblings, 1 reply; 29+ messages in thread
From: Sascha Hauer @ 2024-07-30 9:06 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On Tue, Jul 30, 2024 at 09:19:23AM +0200, Ahmad Fatoum wrote:
> In preparation for adding more boolean flags, turn the current two bools
> into two bits sharing the same u8-sized member.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> include/mci.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/mci.h b/include/mci.h
> index 773aed896c96..4f1ff9dc57c8 100644
> --- a/include/mci.h
> +++ b/include/mci.h
> @@ -568,8 +568,8 @@ struct mci {
> unsigned csd[4]; /**< card's "card specific data register" */
> unsigned cid[4]; /**< card's "card identification register" */
> unsigned short rca; /**< relative card address */
> - bool sdio; /**< card is a SDIO card */
> - bool high_capacity; /**< high capacity card is connected (OCR -> OCR_HCS) */
> + u8 sdio:1; /**< card is a SDIO card */
> + u8 high_capacity:1; /**< high capacity card is connected (OCR -> OCR_HCS) */
This doesn't seem to be necessary. They share the same byte in either
way. From a quick test:
struct foo {
u16 a;
bool b;
bool c;
u32 d;
};
struct bar {
u16 a;
u8 b:1;
u8 c:1;
u32 d;
};
printf("%d %d\n", sizeof(struct foo), sizeof(struct bar));
The result is 8 for both structs.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 06/10] mci: core: use CONFIG_MCI_WRITE, not CONFIG_BLOCK_WRITE
2024-07-30 7:19 ` [PATCH 06/10] mci: core: use CONFIG_MCI_WRITE, not CONFIG_BLOCK_WRITE Ahmad Fatoum
@ 2024-07-30 9:18 ` Sascha Hauer
2024-07-30 11:08 ` Ahmad Fatoum
0 siblings, 1 reply; 29+ messages in thread
From: Sascha Hauer @ 2024-07-30 9:18 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On Tue, Jul 30, 2024 at 09:19:25AM +0200, Ahmad Fatoum wrote:
> There's a more specific CONFIG_MCI_WRITE that's so far only used to
> remove write support for in the Atmel MCI driver. We should use the same
> symbol also to remove support in the MCI core instead of relying on its
> parent CONFIG_BLOCK_WRITE option.
Currently CONFIG_MCI_WRITE has no relation to CONFIG_BLOCK_WRITE.
Having CONFIG_MCI_WRITE enabled and CONFIG_BLOCK_WRITE disabled doesn't
make sense. Shouldn't CONFIG_MCI_WRITE depend on CONFIG_BLOCK_WRITE?
Also having CONFIG_BLOCK_WRITE enabled and CONFIG_MCI_WRITE disabled
allows you to support writing to block devices that are not MCI devices.
Given that the vast majority of block devices are actually MCI devices
this seems rather exotic.
Maybe we should drop CONFIG_MCI_WRITE and replace it with
CONFIG_BLOCK_WRITE, or just have
config MCI_WRITE
bool
default y if BLOCK_WRITE
Sascha
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> drivers/mci/mci-core.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
> index f6f8a6adabb9..3a5fb0330700 100644
> --- a/drivers/mci/mci-core.c
> +++ b/drivers/mci/mci-core.c
> @@ -1801,8 +1801,8 @@ static int mci_blk_part_switch(struct mci_part *part)
> *
> * This routine expects the buffer has the correct size to read all data!
> */
> -static int __maybe_unused mci_sd_write(struct block_device *blk,
> - const void *buffer, sector_t block, blkcnt_t num_blocks)
> +static int mci_sd_write(struct block_device *blk,
> + const void *buffer, sector_t block, blkcnt_t num_blocks)
> {
> struct mci_part *part = container_of(blk, struct mci_part, blk);
> struct mci *mci = part->mci;
> @@ -2179,9 +2179,7 @@ static int mci_check_if_already_initialized(struct mci *mci)
>
> static struct block_device_ops mci_ops = {
> .read = mci_sd_read,
> -#ifdef CONFIG_BLOCK_WRITE
> - .write = mci_sd_write,
> -#endif
> + .write = IS_ENABLED(CONFIG_MCI_WRITE) ? mci_sd_write : NULL,
> };
>
> static int mci_set_boot(struct param_d *param, void *priv)
> --
> 2.39.2
>
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 07/10] mci: add support for discarding write blocks
2024-07-30 7:19 ` [PATCH 07/10] mci: add support for discarding write blocks Ahmad Fatoum
@ 2024-07-30 9:23 ` Yann Sionneau
2024-07-30 11:14 ` Ahmad Fatoum
2024-07-30 10:05 ` Sascha Hauer
1 sibling, 1 reply; 29+ messages in thread
From: Yann Sionneau @ 2024-07-30 9:23 UTC (permalink / raw)
To: barebox
Hello Ahmad,
Le 7/30/24 à 09:19, Ahmad Fatoum a écrit :
> From: Ahmad Fatoum <ahmad@a3f.at>
>
> A common optimization for flashing is to skip gaps between partitions
> and only flash actual data. The problem with that is that data from
> the previous installation may persist and confuse later software, e.g.
> an existing barebox state partition not contained in the image may
> survive, when the expectation is that a new state is created.
>
> eMMCs can support three different commands for erasing data:
>
> - Erase: This has the widest support, but works on only on the level
> of erase groups that may span multiple write blocks.
> The region reads as either '0' or '1' afterwards.
>
> - Trim: New in eMMC v4.4. This erases write blocks.
> The region reads as either '0' or '1' afterwards.
>
> - Discard: New in eMMC v4.5. This discards write blocks. It's up to the
> card what action if any is taken.
> All or part of the data may remain accessible.
>
> All of these don't enforce a actual physical NAND erase operation.
> In case erasure does happen, it may happen later at a convenient time.
>
> All of them, except for discard guarantee that a fixed pattern (either
> all zeroes or all ones) will be read back after the erase operation
> concludes. Therefore let's use them in barebox to implement the erase
> command.
>
> The behavior of the erase command will be similar to the Linux
> blkdiscard -z command when the erase byte value is all-zeroes. In Linux
> blkdiscard -z is emulated with zero writes if the erased byte is 0xFF,
> but for barebox, the erase operation won't care whether it writes 0x00
> or 0xFF.
>
> Note that this is considerably slower than need be, because we don't
> erase multiple erase groups at once. Doing so could run into the
> send_cmd timeout of the host hardware or its drivers. The correct
> workaround is to calculate the duration of the erase operation and split
> up the erases, so we always stay below a specific erase operation
> duration. There's a comment that explains this and implementing it is
> left as future exercise.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> drivers/mci/Kconfig | 5 +
> drivers/mci/mci-core.c | 393 ++++++++++++++++++++++++++++++++++++++---
> include/mci.h | 10 ++
> 3 files changed, 387 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/mci/Kconfig b/drivers/mci/Kconfig
> index 1e8c85643b9a..f760614725fa 100644
> --- a/drivers/mci/Kconfig
> +++ b/drivers/mci/Kconfig
> @@ -40,6 +40,11 @@ config MCI_WRITE
> default y
> select DISK_WRITE
>
> +config MCI_ERASE
> + bool "Support erasing MCI cards"
> + depends on MCI_WRITE
> + default y
> +
> config MCI_MMC_BOOT_PARTITIONS
> bool "support MMC boot partitions"
> help
> diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
> index 3a5fb0330700..200b23ffb604 100644
> --- a/drivers/mci/mci-core.c
> +++ b/drivers/mci/mci-core.c
> @@ -20,6 +20,7 @@
> #include <disks.h>
> #include <of.h>
> #include <linux/err.h>
> +#include <linux/log2.h>
> #include <linux/sizes.h>
> #include <dma.h>
>
> @@ -168,6 +169,32 @@ static int mci_send_status(struct mci *mci, unsigned int *status)
> return ret;
> }
>
> +static int mci_app_sd_status(struct mci *mci, __be32 *ssr)
> +{
> + int err;
> + struct mci_cmd cmd;
> + struct mci_data data;
> +
> + cmd.cmdidx = MMC_CMD_APP_CMD;
> + cmd.resp_type = MMC_RSP_R1;
> + cmd.cmdarg = mci->rca << 16;
> +
> + err = mci_send_cmd_retry(mci, &cmd, NULL, 4);
> + if (err)
> + return err;
> +
> + cmd.cmdidx = SD_CMD_APP_SD_STATUS;
> + cmd.resp_type = MMC_RSP_R1;
> + cmd.cmdarg = 0;
> +
> + data.dest = (u8 *)ssr;
> + data.blocksize = 64;
> + data.blocks = 1;
> + data.flags = MMC_DATA_READ;
> +
> + return mci_send_cmd_retry(mci, &cmd, &data, 3);
> +}
> +
> static int mmc_switch_status_error(struct mci_host *host, u32 status)
> {
> if (mmc_host_is_spi(host)) {
> @@ -286,6 +313,56 @@ static int mci_block_write(struct mci *mci, const void *src, int blocknum,
> return ret;
> }
>
> +/**
> + * Erase one or several blocks of data to the card
> + * @param mci_dev MCI instance
> + * @param from Block number to start erasing from
> + * @param to inclusive last block number to erase to
> + * @return Transaction status (0 on success)
> + */
> +static int mci_block_erase(struct mci *card, unsigned int from,
> + unsigned int to, unsigned int arg)
> +{
> + struct mci_cmd cmd = {};
> + int err;
> +
> + if (!card->high_capacity) {
> + from *= card->write_bl_len;
> + to *= card->write_bl_len;
> + }
> +
> + cmd.cmdidx = IS_SD(card) ? SD_ERASE_WR_BLK_START : MMC_ERASE_GROUP_START;
> + cmd.cmdarg = from;
> + cmd.resp_type = MMC_RSP_R1;
> + err = mci_send_cmd(card, &cmd, NULL);
> + if (err)
> + goto err_out;
> +
> + memset(&cmd, 0, sizeof(struct mci_cmd));
> + cmd.cmdidx = IS_SD(card) ? SD_ERASE_WR_BLK_END : MMC_ERASE_GROUP_END;
> + cmd.cmdarg = to;
> + cmd.resp_type = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
> + err = mci_send_cmd(card, &cmd, NULL);
> + if (err)
> + goto err_out;
> +
> + memset(&cmd, 0, sizeof(struct mci_cmd));
> + cmd.cmdidx = MMC_ERASE;
> + cmd.cmdarg = arg;
> + cmd.resp_type = MMC_RSP_R1b;
> +
> + err = mci_send_cmd(card, &cmd, NULL);
> + if (err)
> + goto err_out;
> +
> + return 0;
> +
> +err_out:
> + dev_err(&card->dev, "erase cmd %d error %d, status %#x\n",
> + cmd.cmdidx, err, cmd.response[0]);
> + return -EIO;
> +}
> +
> /**
> * Read one or several block(s) of data from the card
> * @param mci MCI instance
> @@ -773,6 +850,49 @@ static int sd_switch(struct mci *mci, unsigned mode, unsigned group,
> return mci_send_cmd(mci, &cmd, &data);
> }
>
> +static int sd_read_ssr(struct mci *mci)
> +{
> + static const unsigned int sd_au_size[] = {
> + 0, SZ_16K / 512, SZ_32K / 512,
> + SZ_64K / 512, SZ_128K / 512, SZ_256K / 512,
> + SZ_512K / 512, SZ_1M / 512, SZ_2M / 512,
> + SZ_4M / 512, SZ_8M / 512, (SZ_8M + SZ_4M) / 512,
> + SZ_16M / 512, (SZ_16M + SZ_8M) / 512, SZ_32M / 512,
> + SZ_64M / 512,
> + };
> + __be32 *ssr;
> + int err;
> + unsigned int au, eo, et, es;
> +
> + if (!IS_ENABLED(CONFIG_MCI_ERASE))
> + return -ENOSYS;
> +
> + ssr = dma_alloc(64);
> +
> + err = mci_app_sd_status(mci, ssr);
> + if (err)
> + goto out;
> +
> + au = (be32_to_cpu(ssr[2]) >> 12) & 0xF;
> + if ((au <= 9) || (mci->version == SD_VERSION_3)) {
> + mci->ssr.au = sd_au_size[au];
> + es = (be32_to_cpu(ssr[3]) >> 24) & 0xFF;
> + es |= (be32_to_cpu(ssr[2]) & 0xFF) << 8;
> + et = (be32_to_cpu(ssr[3]) >> 18) & 0x3F;
> + if (es && et) {
> + eo = (be32_to_cpu(ssr[3]) >> 16) & 0x3;
> + mci->ssr.erase_timeout = (et * 1000) / es;
> + mci->ssr.erase_offset = eo * 1000;
> + }
> + } else {
> + dev_dbg(&mci->dev, "invalid allocation unit size.\n");
> + }
> +
> +out:
> + dma_free(ssr);
> + return err;
> +}
> +
> /**
> * Change transfer frequency for an SD card
> * @param mci MCI instance
> @@ -845,6 +965,11 @@ static int sd_change_freq(struct mci *mci)
> if (mci->scr[0] & SD_DATA_4BIT)
> mci->card_caps |= MMC_CAP_4_BIT_DATA;
>
> + if (mci->scr[0] & SD_DATA_STAT_AFTER_ERASE)
> + mci->erased_byte = 0xFF;
> + else
> + mci->erased_byte = 0x0;
> +
> /* Version 1.0 doesn't support switching */
> if (mci->version == SD_VERSION_1_0)
> return 0;
> @@ -1083,6 +1208,47 @@ static void mci_extract_block_lengths_from_csd(struct mci *mci)
> mci->write_bl_len, mci->read_bl_len);
> }
>
> +/**
> + * Extract erase group size
> + * @param mci MCI instance
> + */
> +static void mci_extract_erase_group_size(struct mci *mci)
> +{
> + if (!IS_ENABLED(CONFIG_MCI_ERASE) ||
> + !(UNSTUFF_BITS(mci->csd, 84, 12) & CCC_ERASE))
> + return;
> +
> +
> + if (IS_SD(mci) && UNSTUFF_BITS(mci->csd, 126, 2) != 0) {
> + /* For SD with csd_struct v1, erase group is always one sector */
> + mci->erase_grp_size = 1;
> + } else {
> + if (mci->ext_csd[EXT_CSD_ERASE_GROUP_DEF] & 0x01) {
> + /* Read out group size from ext_csd */
> + mci->erase_grp_size = mci->ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE] * 1024;
> + } else {
> + /* Calculate the group size from the csd value. */
> + int erase_gsz, erase_gmul;
> +
> + erase_gsz = (mci->csd[2] & 0x00007c00) >> 10;
> + erase_gmul = (mci->csd[2] & 0x000003e0) >> 5;
> + mci->erase_grp_size = (erase_gsz + 1)
> + * (erase_gmul + 1);
> + }
> +
> + if (mci->ext_csd[EXT_CSD_ERASED_MEM_CONT])
> + mci->erased_byte = 0xFF;
> + else
> + mci->erased_byte = 0x0;
> +
> + if (mci->ext_csd[EXT_CSD_SEC_FEATURE_SUPPORT] & EXT_CSD_SEC_FEATURE_TRIM_EN)
> + mci->can_trim = true;
> + }
> +
> + dev_dbg(&mci->dev, "Erase group is %u sector(s). Trim %ssupported\n",
> + mci->erase_grp_size, mci->can_trim ? "" : "not ");
> +}
> +
> /**
> * Extract card's capacitiy from the CSD
> * @param mci MCI instance
> @@ -1229,6 +1395,10 @@ static int mci_startup_sd(struct mci *mci)
>
> mci_set_clock(mci, mci->tran_speed);
>
> + err = sd_read_ssr(mci);
> + if (err)
> + dev_dbg(&mci->dev, "unable to read ssr: %pe\n", ERR_PTR(err));
> +
> return 0;
> }
>
> @@ -1700,6 +1870,7 @@ static int mci_startup(struct mci *mci)
> dev_info(&mci->dev, "detected %s card version %s\n", IS_SD(mci) ? "SD" : "MMC",
> mci_version_string(mci));
> mci_extract_card_capacity_from_csd(mci);
> + mci_extract_erase_group_size(mci);
>
> if (IS_SD(mci))
> err = mci_startup_sd(mci);
> @@ -1791,6 +1962,189 @@ static int mci_blk_part_switch(struct mci_part *part)
>
> /* ------------------ attach to the blocklayer --------------------------- */
>
> +static int mci_sd_check_write(struct mci *mci, const char *op,
> + sector_t block, blkcnt_t num_blocks)
> +{
> + struct mci_host *host = mci->host;
> +
> + if (!host->disable_wp &&
> + host->ops.card_write_protected && host->ops.card_write_protected(host)) {
> + dev_err(&mci->dev, "card write protected\n");
> + return -EPERM;
> + }
> +
> + dev_dbg(&mci->dev, "%s: %s %llu block(s), starting at %llu\n",
> + __func__, op, num_blocks, block);
> +
> + if (mci->write_bl_len != SECTOR_SIZE) {
> + dev_dbg(&mci->dev, "MMC/SD block size is not %d bytes (its %u bytes instead)\n",
> + SECTOR_SIZE, mci->read_bl_len);
> + return -EINVAL;
> + }
> +
> + /* size of the block number field in the MMC/SD command is 32 bit only */
> + if (block > MAX_BUFFER_NUMBER) {
> + dev_dbg(&mci->dev, "Cannot handle block number %llu. Too large!\n", block);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static unsigned int mmc_align_erase_size(struct mci *card,
> + sector_t *from,
> + sector_t *to,
> + blkcnt_t nr)
> +{
> + unsigned int from_new = *from, to_new, nr_new = nr, rem;
> +
> + /*
> + * When the 'card->erase_size' is power of 2, we can use round_up/down()
> + * to align the erase size efficiently.
> + */
> + if (is_power_of_2(card->erase_grp_size)) {
> + unsigned int temp = from_new;
> +
> + from_new = round_up(temp, card->erase_grp_size);
> + rem = from_new - temp;
> +
> + if (nr_new > rem)
> + nr_new -= rem;
> + else
> + return 0;
> +
> + nr_new = round_down(nr_new, card->erase_grp_size);
> + } else {
> + rem = from_new % card->erase_grp_size;
> + if (rem) {
> + rem = card->erase_grp_size - rem;
> + from_new += rem;
> + if (nr_new > rem)
> + nr_new -= rem;
> + else
> + return 0;
> + }
> +
> + rem = nr_new % card->erase_grp_size;
> + if (rem)
> + nr_new -= rem;
> + }
> +
> + if (nr_new == 0)
> + return 0;
> +
> + to_new = from_new + nr_new;
> +
> + if (*to != to_new || *from != from_new)
> + dev_warn(&card->dev, "Erase range changed to [0x%x-0x%x] because of %u sector erase group\n",
> + from_new, to_new, card->erase_grp_size);
> +
> + *to = to_new;
> + *from = from_new;
> +
> + return nr_new;
> +}
> +
> +/**
> + * Erase a memory region
> + * @param blk All info about the block device we need
> + * @param block first block to erase
> + * @param num_blocks Number of blocks to erase
> + * @return 0 on success, anything else on failure
> + *
> + */
> +static int mci_sd_erase(struct block_device *blk, sector_t from,
> + blkcnt_t blkcnt)
> +{
> + struct mci_part *part = container_of(blk, struct mci_part, blk);
> + struct mci *mci = part->mci;
> + sector_t i = 0;
> + unsigned arg;
> + sector_t to = from + blkcnt;
> + int rc;
> +
> + mci_blk_part_switch(part);
> +
> + rc = mci_sd_check_write(mci, "Erase", from, blkcnt);
> + if (rc)
> + return rc;
> +
> + if (!mci->erase_grp_size)
> + return -EOPNOTSUPP;
> +
> + if (mci->can_trim) {
> + arg = MMC_TRIM_ARG;
> + } else {
> + /* We don't use discard, as it doesn't guarantee a fixed value */
> + arg = MMC_ERASE_ARG;
> + blkcnt = mmc_align_erase_size(mci, &from, &to, blkcnt);
> + }
> +
> + if (blkcnt == 0)
> + return 0;
> +
> + if (to <= from)
> + return -EINVAL;
> +
> + /* 'from' and 'to' are inclusive */
> + to -= 1;
> +
> + while (i < blkcnt) {
> + sector_t blk_r;
> +
> + /* TODO: While it's possible to clear many erase groups at once
> + * and it greatly improves throughput, drivers need adjustment:
> + *
> + * Many drivers hardcode a maximal wait time before aborting
> + * the wait for R1b and returning -ETIMEDOUT. With long
> + * erases/trims, we are bound to run into this timeout, so for now
> + * we just split into suifficiently small erases that are unlikely
suifficiently -> sufficiently
> + * to trigger the time.
time -> timeout?
> + *
> + * What Linux does and what we should be doing in barebox is:
> + *
> + * - add a struct mci_cmd::busy_timeout member that drivers should
> + * use instead of hardcoding their own timeout delay. The busy
> + * timeout length can be calculated by the MCI core after
> + * consulting the appropriate CSD/EXT_CSD/SSR registers.
> + *
> + * - add a struct mci_host::max_busy_timeout member, where drivers
> + * can indicate the maximum timeout they are able to support.
> + * The MCI core will never set a busy_timeout that exceeds this
> + * value.
> + *
> + * Example Samsung eMMC 8GTF4:
> + *
> + * time erase /dev/mmc2.part_of_512m # 1024 trims
> + * time: 2849ms
> + *
> + * time erase /dev/mmc2.part_of_512m # single trim
> + * time: 56ms
> + */
> +
> + if (IS_SD(mci) && mci->ssr.au) {
> + blk_r = ((blkcnt - i) > mci->ssr.au) ?
> + mci->ssr.au : (blkcnt - i);
> + } else {
> + blk_r = ((blkcnt - i) > mci->erase_grp_size) ?
> + mci->erase_grp_size : (blkcnt - i);
> + }
> +
> + rc = mci_block_erase(mci, from, to, arg);
> + if (rc)
> + break;
> +
> + /* Waiting for the ready status */
> + rc = mci_poll_until_ready(mci, 1000 /* ms */);
> + if (rc)
> + break;
> +
> + i += blk_r;
> + }
> +
> + return i == blkcnt ? 0 : rc;
> +}
> +
> /**
> * Write a chunk of sectors to media
> * @param blk All info about the block device we need
> @@ -1806,7 +2160,6 @@ static int mci_sd_write(struct block_device *blk,
> {
> struct mci_part *part = container_of(blk, struct mci_part, blk);
> struct mci *mci = part->mci;
> - struct mci_host *host = mci->host;
> int rc;
> blkcnt_t max_req_block = num_blocks;
> blkcnt_t write_block;
> @@ -1816,26 +2169,9 @@ static int mci_sd_write(struct block_device *blk,
>
> mci_blk_part_switch(part);
>
> - if (!host->disable_wp &&
> - host->ops.card_write_protected && host->ops.card_write_protected(host)) {
> - dev_err(&mci->dev, "card write protected\n");
> - return -EPERM;
> - }
> -
> - dev_dbg(&mci->dev, "%s: Write %llu block(s), starting at %llu\n",
> - __func__, num_blocks, block);
> -
> - if (mci->write_bl_len != SECTOR_SIZE) {
> - dev_dbg(&mci->dev, "MMC/SD block size is not %d bytes (its %u bytes instead)\n",
> - SECTOR_SIZE, mci->read_bl_len);
> - return -EINVAL;
> - }
> -
> - /* size of the block number field in the MMC/SD command is 32 bit only */
> - if (block > MAX_BUFFER_NUMBER) {
> - dev_dbg(&mci->dev, "Cannot handle block number %llu. Too large!\n", block);
> - return -EINVAL;
> - }
> + rc = mci_sd_check_write(mci, "Write", block, num_blocks);
> + if (rc)
> + return rc;
>
> while (num_blocks) {
> write_block = min(num_blocks, max_req_block);
> @@ -2123,6 +2459,20 @@ static void mci_info(struct device *dev)
> return;
> printf(" Version: %s\n", mci_version_string(mci));
> printf(" Capacity: %u MiB\n", (unsigned)(mci->capacity >> 20));
> + if (CONFIG_MCI_ERASE) {
> + printf(" Erase support:");
> + if (mci->can_trim)
> + printf(" trim");
> + if (mci->erase_grp_size) {
> + printf(" erase(%u sector%s%s)", mci->ssr.au ?: mci->erase_grp_size,
> + mci->erase_grp_size > 1 ? "s" : "",
> + mci->ssr.au ? " AU" : "");
> + }
> + if (mci->can_trim || mci->erase_grp_size)
> + printf(", erase value: 0x%02x\n", mci->erased_byte);
> + else
> + printf(" none\n");
> + }
>
> if (mci->high_capacity)
> printf(" High capacity card\n");
> @@ -2180,6 +2530,7 @@ static int mci_check_if_already_initialized(struct mci *mci)
> static struct block_device_ops mci_ops = {
> .read = mci_sd_read,
> .write = IS_ENABLED(CONFIG_MCI_WRITE) ? mci_sd_write : NULL,
> + .erase = IS_ENABLED(CONFIG_MCI_ERASE) ? mci_sd_erase : NULL,
> };
>
> static int mci_set_boot(struct param_d *param, void *priv)
> diff --git a/include/mci.h b/include/mci.h
> index 610040937ee5..3bf1455a401c 100644
> --- a/include/mci.h
> +++ b/include/mci.h
> @@ -616,6 +616,12 @@ struct mci_part {
> #define MMC_BLK_DATA_AREA_RPMB (1<<3)
> };
>
> +struct sd_ssr {
> + unsigned int au; /* In sectors */
> + unsigned int erase_timeout; /* In milliseconds */
> + unsigned int erase_offset; /* In milliseconds */
> +};
> +
> /** MMC/SD and interface instance information */
> struct mci {
> struct mci_host *host; /**< the host for this card */
> @@ -629,16 +635,20 @@ struct mci {
> unsigned short rca; /**< relative card address */
> u8 sdio:1; /**< card is a SDIO card */
> u8 high_capacity:1; /**< high capacity card is connected (OCR -> OCR_HCS) */
> + u8 can_trim:1; /**< high capacity card is connected (OCR -> OCR_HCS) */
> + u8 erased_byte;
> unsigned tran_speed; /**< Maximum transfer speed */
> /** currently used data block length for read accesses */
> unsigned read_bl_len;
> /** currently used data block length for write accesses */
> unsigned write_bl_len;
> + unsigned erase_grp_size;
> uint64_t capacity; /**< Card's data capacity in bytes */
> int ready_for_use; /** true if already probed */
> int dsr_imp; /**< DSR implementation state from CSD */
> u8 *ext_csd;
> int probe;
> + struct sd_ssr ssr;
> int bootpart;
> int boot_ack_enable;
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 05/10] mci: describe more command structure in mci.h
2024-07-30 7:19 ` [PATCH 05/10] mci: describe more command structure in mci.h Ahmad Fatoum
@ 2024-07-30 9:25 ` Yann Sionneau
2024-07-30 11:07 ` Ahmad Fatoum
0 siblings, 1 reply; 29+ messages in thread
From: Yann Sionneau @ 2024-07-30 9:25 UTC (permalink / raw)
To: barebox
Hello Ahmad,
Le 7/30/24 à 09:19, Ahmad Fatoum a écrit :
> This introduces no functional change, but adds a number of definitions
> that will come in handy when adding erase support in a later commit.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> include/mci.h | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 60 insertions(+), 1 deletion(-)
>
> diff --git a/include/mci.h b/include/mci.h
> index 4f1ff9dc57c8..610040937ee5 100644
> --- a/include/mci.h
> +++ b/include/mci.h
>
> +/*
> + * Card Command Classes (CCC)
> + */
> +#define CCC_BASIC (1<<0) /* (0) Basic protocol functions */
> + /* (CMD0,1,2,3,4,7,9,10,12,13,15) */
> + /* (and for SPI, CMD58,59) */
> +#define CCC_STREAM_READ (1<<1) /* (1) Stream read commands */
> + /* (CMD11) */
> +#define CCC_BLOCK_READ (1<<2) /* (2) Block read commands */
> + /* (CMD16,17,18) */
> +#define CCC_STREAM_WRITE (1<<3) /* (3) Stream write commands */
> + /* (CMD20) */
> +#define CCC_BLOCK_WRITE (1<<4) /* (4) Block write commands */
> + /* (CMD16,24,25,26,27) */
> +#define CCC_ERASE (1<<5) /* (5) Ability to erase blocks */
> + /* (CMD32,33,34,35,36,37,38,39) */
> +#define CCC_WRITE_PROT (1<<6) /* (6) Able to write protect blocks */
> + /* (CMD28,29,30) */
> +#define CCC_LOCK_CARD (1<<7) /* (7) Able to lock down card */
> + /* (CMD16,CMD42) */
> +#define CCC_APP_SPEC (1<<8) /* (8) Application specific */
> + /* (CMD55,56,57,ACMD*) */
> +#define CCC_IO_MODE (1<<9) /* (9) I/O mode */
> + /* (CMD5,39,40,52,53) */
> +#define CCC_SWITCH (1<<10) /* (10) High speed switch */
> + /* (CMD6,34,35,36,37,50) */
> + /* (11) Reserved */
> + /* (CMD?) */
> +
Why not use BIT(xx)? Maybe this is copy paste from Linux and you want to
keep it easy to synchronize?
--
Yann
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 07/10] mci: add support for discarding write blocks
2024-07-30 7:19 ` [PATCH 07/10] mci: add support for discarding write blocks Ahmad Fatoum
2024-07-30 9:23 ` Yann Sionneau
@ 2024-07-30 10:05 ` Sascha Hauer
2024-07-30 11:17 ` Ahmad Fatoum
1 sibling, 1 reply; 29+ messages in thread
From: Sascha Hauer @ 2024-07-30 10:05 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On Tue, Jul 30, 2024 at 09:19:26AM +0200, Ahmad Fatoum wrote:
> /**
> * Read one or several block(s) of data from the card
> * @param mci MCI instance
> @@ -773,6 +850,49 @@ static int sd_switch(struct mci *mci, unsigned mode, unsigned group,
> return mci_send_cmd(mci, &cmd, &data);
> }
>
> +static int sd_read_ssr(struct mci *mci)
> +{
> + static const unsigned int sd_au_size[] = {
> + 0, SZ_16K / 512, SZ_32K / 512,
> + SZ_64K / 512, SZ_128K / 512, SZ_256K / 512,
> + SZ_512K / 512, SZ_1M / 512, SZ_2M / 512,
> + SZ_4M / 512, SZ_8M / 512, (SZ_8M + SZ_4M) / 512,
> + SZ_16M / 512, (SZ_16M + SZ_8M) / 512, SZ_32M / 512,
> + SZ_64M / 512,
> + };
> + __be32 *ssr;
> + int err;
> + unsigned int au, eo, et, es;
> +
> + if (!IS_ENABLED(CONFIG_MCI_ERASE))
> + return -ENOSYS;
I think we settled on using -EOPNOTSUPP in this case.
> +static unsigned int mmc_align_erase_size(struct mci *card,
> + sector_t *from,
> + sector_t *to,
> + blkcnt_t nr)
> +{
> + unsigned int from_new = *from, to_new, nr_new = nr, rem;
> +
> + /*
> + * When the 'card->erase_size' is power of 2, we can use round_up/down()
> + * to align the erase size efficiently.
> + */
> + if (is_power_of_2(card->erase_grp_size)) {
> + unsigned int temp = from_new;
> +
> + from_new = round_up(temp, card->erase_grp_size);
> + rem = from_new - temp;
> +
> + if (nr_new > rem)
> + nr_new -= rem;
> + else
> + return 0;
> +
> + nr_new = round_down(nr_new, card->erase_grp_size);
> + } else {
> + rem = from_new % card->erase_grp_size;
> + if (rem) {
> + rem = card->erase_grp_size - rem;
> + from_new += rem;
> + if (nr_new > rem)
> + nr_new -= rem;
> + else
> + return 0;
> + }
> +
> + rem = nr_new % card->erase_grp_size;
> + if (rem)
> + nr_new -= rem;
> + }
> +
> + if (nr_new == 0)
> + return 0;
> +
> + to_new = from_new + nr_new;
> +
> + if (*to != to_new || *from != from_new)
> + dev_warn(&card->dev, "Erase range changed to [0x%x-0x%x] because of %u sector erase group\n",
> + from_new, to_new, card->erase_grp_size);
> +
> + *to = to_new;
> + *from = from_new;
> +
> + return nr_new;
> +}
> +
> +/**
> + * Erase a memory region
> + * @param blk All info about the block device we need
> + * @param block first block to erase
> + * @param num_blocks Number of blocks to erase
> + * @return 0 on success, anything else on failure
> + *
> + */
> +static int mci_sd_erase(struct block_device *blk, sector_t from,
> + blkcnt_t blkcnt)
> +{
> + struct mci_part *part = container_of(blk, struct mci_part, blk);
> + struct mci *mci = part->mci;
> + sector_t i = 0;
> + unsigned arg;
> + sector_t to = from + blkcnt;
> + int rc;
> +
> + mci_blk_part_switch(part);
> +
> + rc = mci_sd_check_write(mci, "Erase", from, blkcnt);
> + if (rc)
> + return rc;
> +
> + if (!mci->erase_grp_size)
> + return -EOPNOTSUPP;
> +
> + if (mci->can_trim) {
> + arg = MMC_TRIM_ARG;
> + } else {
> + /* We don't use discard, as it doesn't guarantee a fixed value */
> + arg = MMC_ERASE_ARG;
> + blkcnt = mmc_align_erase_size(mci, &from, &to, blkcnt);
> + }
> +
> + if (blkcnt == 0)
> + return 0;
> +
> + if (to <= from)
> + return -EINVAL;
When mmc_align_erase_size() is not called then we cannot arrive here
as we already returned in the if (blkcnt == 0) check above.
When mmc_align_erase_size() is called and this test triggers then it
only reveals a bug in mmc_align_erase_size().
I think this test should go away.
> +
> + /* 'from' and 'to' are inclusive */
> + to -= 1;
> +
> + while (i < blkcnt) {
> + sector_t blk_r;
> +
> + /* TODO: While it's possible to clear many erase groups at once
> + * and it greatly improves throughput, drivers need adjustment:
> + *
> + * Many drivers hardcode a maximal wait time before aborting
> + * the wait for R1b and returning -ETIMEDOUT. With long
> + * erases/trims, we are bound to run into this timeout, so for now
> + * we just split into suifficiently small erases that are unlikely
> + * to trigger the time.
> + *
> + * What Linux does and what we should be doing in barebox is:
> + *
> + * - add a struct mci_cmd::busy_timeout member that drivers should
> + * use instead of hardcoding their own timeout delay. The busy
> + * timeout length can be calculated by the MCI core after
> + * consulting the appropriate CSD/EXT_CSD/SSR registers.
> + *
> + * - add a struct mci_host::max_busy_timeout member, where drivers
> + * can indicate the maximum timeout they are able to support.
> + * The MCI core will never set a busy_timeout that exceeds this
> + * value.
> + *
> + * Example Samsung eMMC 8GTF4:
> + *
> + * time erase /dev/mmc2.part_of_512m # 1024 trims
> + * time: 2849ms
> + *
> + * time erase /dev/mmc2.part_of_512m # single trim
> + * time: 56ms
> + */
> +
> + if (IS_SD(mci) && mci->ssr.au) {
> + blk_r = ((blkcnt - i) > mci->ssr.au) ?
> + mci->ssr.au : (blkcnt - i);
> + } else {
> + blk_r = ((blkcnt - i) > mci->erase_grp_size) ?
> + mci->erase_grp_size : (blkcnt - i);
> + }
> +
> + rc = mci_block_erase(mci, from, to, arg);
You say you split up the whole erase into sufficiently small erases, but
'from' and 'to' are never changed in this loop and you seem to erase
the whole area multiple times.
> + if (rc)
> + break;
> +
> + /* Waiting for the ready status */
> + rc = mci_poll_until_ready(mci, 1000 /* ms */);
> + if (rc)
> + break;
> +
> + i += blk_r;
> + }
> +
> + return i == blkcnt ? 0 : rc;
> +}
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 08/10] commands: sync: add new command to flush cached writes
2024-07-30 7:19 ` [PATCH 08/10] commands: sync: add new command to flush cached writes Ahmad Fatoum
@ 2024-07-30 10:08 ` Sascha Hauer
0 siblings, 0 replies; 29+ messages in thread
From: Sascha Hauer @ 2024-07-30 10:08 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On Tue, Jul 30, 2024 at 09:19:27AM +0200, Ahmad Fatoum wrote:
> In barebox, the dirty block cache for a given device is flushed when
> closing all file descriptors to it, a cached block needs to be evicted
> due to pressure or when barebox shuts down.
>
> There are valid uses to do it explicitly however:
>
> - When benchmarking write speed to a block device
> - Before forced resets, because a regular reset would run shutdown
> code that should be omitted.
>
> Add a sync command that does this. Unlike the userspace variant, we
> don't both to implement a argument-less sync that's global. Users are
s/both/bother/
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 09/10] mci: core: remove reference to SD Card from common mci_card_probe
2024-07-30 7:19 ` [PATCH 09/10] mci: core: remove reference to SD Card from common mci_card_probe Ahmad Fatoum
@ 2024-07-30 10:09 ` Sascha Hauer
0 siblings, 0 replies; 29+ messages in thread
From: Sascha Hauer @ 2024-07-30 10:09 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On Tue, Jul 30, 2024 at 09:19:28AM +0200, Ahmad Fatoum wrote:
> mci_card_probe is called for both SD cards and MMC, so reporting that
> an SD Card was successfully added is misleading.
>
> There is an info message that reports whether it's a MMC or SD card
> already, so just remove the SD from this last debug print.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> drivers/mci/mci-core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
You sent this as an extra patch already (which I applied). You can drop
this from the next round.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 05/10] mci: describe more command structure in mci.h
2024-07-30 9:25 ` Yann Sionneau
@ 2024-07-30 11:07 ` Ahmad Fatoum
0 siblings, 0 replies; 29+ messages in thread
From: Ahmad Fatoum @ 2024-07-30 11:07 UTC (permalink / raw)
To: Yann Sionneau, barebox
On 30.07.24 11:25, Yann Sionneau wrote:
> Hello Ahmad,
>
> Le 7/30/24 à 09:19, Ahmad Fatoum a écrit :
>> This introduces no functional change, but adds a number of definitions
>> that will come in handy when adding erase support in a later commit.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>> include/mci.h | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 60 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/mci.h b/include/mci.h
>> index 4f1ff9dc57c8..610040937ee5 100644
>> --- a/include/mci.h
>> +++ b/include/mci.h
>>
>> +/*
>> + * Card Command Classes (CCC)
>> + */
>> +#define CCC_BASIC (1<<0) /* (0) Basic protocol functions */
>> + /* (CMD0,1,2,3,4,7,9,10,12,13,15) */
>> + /* (and for SPI, CMD58,59) */
>> +#define CCC_STREAM_READ (1<<1) /* (1) Stream read commands */
>> + /* (CMD11) */
>> +#define CCC_BLOCK_READ (1<<2) /* (2) Block read commands */
>> + /* (CMD16,17,18) */
>> +#define CCC_STREAM_WRITE (1<<3) /* (3) Stream write commands */
>> + /* (CMD20) */
>> +#define CCC_BLOCK_WRITE (1<<4) /* (4) Block write commands */
>> + /* (CMD16,24,25,26,27) */
>> +#define CCC_ERASE (1<<5) /* (5) Ability to erase blocks */
>> + /* (CMD32,33,34,35,36,37,38,39) */
>> +#define CCC_WRITE_PROT (1<<6) /* (6) Able to write protect blocks */
>> + /* (CMD28,29,30) */
>> +#define CCC_LOCK_CARD (1<<7) /* (7) Able to lock down card */
>> + /* (CMD16,CMD42) */
>> +#define CCC_APP_SPEC (1<<8) /* (8) Application specific */
>> + /* (CMD55,56,57,ACMD*) */
>> +#define CCC_IO_MODE (1<<9) /* (9) I/O mode */
>> + /* (CMD5,39,40,52,53) */
>> +#define CCC_SWITCH (1<<10) /* (10) High speed switch */
>> + /* (CMD6,34,35,36,37,50) */
>> + /* (11) Reserved */
>> + /* (CMD?) */
>> +
>
> Why not use BIT(xx)? Maybe this is copy paste from Linux and you want to keep it easy to synchronize?
Exactly.
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 06/10] mci: core: use CONFIG_MCI_WRITE, not CONFIG_BLOCK_WRITE
2024-07-30 9:18 ` Sascha Hauer
@ 2024-07-30 11:08 ` Ahmad Fatoum
2024-07-31 7:19 ` Ahmad Fatoum
0 siblings, 1 reply; 29+ messages in thread
From: Ahmad Fatoum @ 2024-07-30 11:08 UTC (permalink / raw)
To: Sascha Hauer; +Cc: barebox
Hello Sascha,
On 30.07.24 11:18, Sascha Hauer wrote:
> On Tue, Jul 30, 2024 at 09:19:25AM +0200, Ahmad Fatoum wrote:
>> There's a more specific CONFIG_MCI_WRITE that's so far only used to
>> remove write support for in the Atmel MCI driver. We should use the same
>> symbol also to remove support in the MCI core instead of relying on its
>> parent CONFIG_BLOCK_WRITE option.
>
> Currently CONFIG_MCI_WRITE has no relation to CONFIG_BLOCK_WRITE.
> Having CONFIG_MCI_WRITE enabled and CONFIG_BLOCK_WRITE disabled doesn't
> make sense. Shouldn't CONFIG_MCI_WRITE depend on CONFIG_BLOCK_WRITE?
>
> Also having CONFIG_BLOCK_WRITE enabled and CONFIG_MCI_WRITE disabled
> allows you to support writing to block devices that are not MCI devices.
> Given that the vast majority of block devices are actually MCI devices
> this seems rather exotic.
>
> Maybe we should drop CONFIG_MCI_WRITE and replace it with
> CONFIG_BLOCK_WRITE, or just have
>
> config MCI_WRITE
> bool
> default y if BLOCK_WRITE
This works for me. I can do this for v2.
>
> Sascha
>
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>> drivers/mci/mci-core.c | 8 +++-----
>> 1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
>> index f6f8a6adabb9..3a5fb0330700 100644
>> --- a/drivers/mci/mci-core.c
>> +++ b/drivers/mci/mci-core.c
>> @@ -1801,8 +1801,8 @@ static int mci_blk_part_switch(struct mci_part *part)
>> *
>> * This routine expects the buffer has the correct size to read all data!
>> */
>> -static int __maybe_unused mci_sd_write(struct block_device *blk,
>> - const void *buffer, sector_t block, blkcnt_t num_blocks)
>> +static int mci_sd_write(struct block_device *blk,
>> + const void *buffer, sector_t block, blkcnt_t num_blocks)
>> {
>> struct mci_part *part = container_of(blk, struct mci_part, blk);
>> struct mci *mci = part->mci;
>> @@ -2179,9 +2179,7 @@ static int mci_check_if_already_initialized(struct mci *mci)
>>
>> static struct block_device_ops mci_ops = {
>> .read = mci_sd_read,
>> -#ifdef CONFIG_BLOCK_WRITE
>> - .write = mci_sd_write,
>> -#endif
>> + .write = IS_ENABLED(CONFIG_MCI_WRITE) ? mci_sd_write : NULL,
>> };
>>
>> static int mci_set_boot(struct param_d *param, void *priv)
>> --
>> 2.39.2
>>
>>
>>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 03/10] block: allow block devices to implement the cdev erase operation
2024-07-30 8:55 ` Sascha Hauer
@ 2024-07-30 11:10 ` Ahmad Fatoum
2024-07-30 11:21 ` Sascha Hauer
0 siblings, 1 reply; 29+ messages in thread
From: Ahmad Fatoum @ 2024-07-30 11:10 UTC (permalink / raw)
To: Sascha Hauer; +Cc: barebox
On 30.07.24 10:55, Sascha Hauer wrote:
> On Tue, Jul 30, 2024 at 09:19:22AM +0200, Ahmad Fatoum wrote:
>> We mainly implement cdev erase for raw flashes. Many block devices, like
>> NVMe, SSD or SD/MMC are also flash-based and expose a mechanism or
>> multiple to trigger an erase on hardware or flash translation layer
>> level without an accompanied write.
>>
>> To make it possible to use this functionality elsewhere in barebox,
>> let's have the common block device layer pass through the erase
>> operation, so it may be implemented in a device-specific manner.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>> common/block.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>> include/block.h | 1 +
>> 2 files changed, 44 insertions(+)
>>
>> diff --git a/common/block.c b/common/block.c
>> index f55da775a797..b43fcbe6927b 100644
>> --- a/common/block.c
>> +++ b/common/block.c
>> @@ -380,10 +380,45 @@ static int block_op_discard_range(struct cdev *cdev, loff_t count, loff_t offset
>> return 0;
>> }
>>
>> +static bool region_overlap(loff_t starta, loff_t lena,
>> + loff_t startb, loff_t lenb)
>> +{
>> + if (starta + lena <= startb)
>> + return 0;
>> + if (startb + lenb <= starta)
>> + return 0;
>> + return 1;
>> +}
>
> We already have this function in fs/devfs-core.c. Time to export it?
This used to be exported, but the export was removed in commit
04e2aa516ed5 ("common.h: move and rename lregion_overlap()").
I can add it back if you prefer.
>
> Sascha
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 04/10] mci: turn bool members into bitfield in struct mci
2024-07-30 9:06 ` Sascha Hauer
@ 2024-07-30 11:10 ` Ahmad Fatoum
0 siblings, 0 replies; 29+ messages in thread
From: Ahmad Fatoum @ 2024-07-30 11:10 UTC (permalink / raw)
To: Sascha Hauer; +Cc: barebox
On 30.07.24 11:06, Sascha Hauer wrote:
> On Tue, Jul 30, 2024 at 09:19:23AM +0200, Ahmad Fatoum wrote:
>> In preparation for adding more boolean flags, turn the current two bools
>> into two bits sharing the same u8-sized member.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>> include/mci.h | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/mci.h b/include/mci.h
>> index 773aed896c96..4f1ff9dc57c8 100644
>> --- a/include/mci.h
>> +++ b/include/mci.h
>> @@ -568,8 +568,8 @@ struct mci {
>> unsigned csd[4]; /**< card's "card specific data register" */
>> unsigned cid[4]; /**< card's "card identification register" */
>> unsigned short rca; /**< relative card address */
>> - bool sdio; /**< card is a SDIO card */
>> - bool high_capacity; /**< high capacity card is connected (OCR -> OCR_HCS) */
>> + u8 sdio:1; /**< card is a SDIO card */
>> + u8 high_capacity:1; /**< high capacity card is connected (OCR -> OCR_HCS) */
>
> This doesn't seem to be necessary. They share the same byte in either
> way. From a quick test:
They don't share the same byte, but the size didn't change because of
padding. More members are added into the padding space in subsequent
commits, which would affect structure size.
>
> struct foo {
> u16 a;
> bool b;
> bool c;
> u32 d;
> };
>
> struct bar {
> u16 a;
> u8 b:1;
> u8 c:1;
> u32 d;
> };
>
> printf("%d %d\n", sizeof(struct foo), sizeof(struct bar));
>
> The result is 8 for both structs.
>
> Sascha
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 07/10] mci: add support for discarding write blocks
2024-07-30 9:23 ` Yann Sionneau
@ 2024-07-30 11:14 ` Ahmad Fatoum
0 siblings, 0 replies; 29+ messages in thread
From: Ahmad Fatoum @ 2024-07-30 11:14 UTC (permalink / raw)
To: Yann Sionneau, barebox
Hello Yann,
On 30.07.24 11:23, Yann Sionneau wrote:
> Hello Ahmad,
Thanks for the review. Can you please trim your replies in longer mails,
so it's easier to find the review?
>
> Le 7/30/24 à 09:19, Ahmad Fatoum a écrit :
>> From: Ahmad Fatoum <ahmad@a3f.at>
>> + while (i < blkcnt) {
>> + sector_t blk_r;
>> +
>> + /* TODO: While it's possible to clear many erase groups at once
>> + * and it greatly improves throughput, drivers need adjustment:
>> + *
>> + * Many drivers hardcode a maximal wait time before aborting
>> + * the wait for R1b and returning -ETIMEDOUT. With long
>> + * erases/trims, we are bound to run into this timeout, so for now
>> + * we just split into suifficiently small erases that are unlikely
>
> suifficiently -> sufficiently
will fix.
>
>> + * to trigger the time.
> time -> timeout?
Yes.
Will fix for v2.
Thanks,
Ahmad
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 07/10] mci: add support for discarding write blocks
2024-07-30 10:05 ` Sascha Hauer
@ 2024-07-30 11:17 ` Ahmad Fatoum
0 siblings, 0 replies; 29+ messages in thread
From: Ahmad Fatoum @ 2024-07-30 11:17 UTC (permalink / raw)
To: Sascha Hauer; +Cc: barebox
On 30.07.24 12:05, Sascha Hauer wrote:
> On Tue, Jul 30, 2024 at 09:19:26AM +0200, Ahmad Fatoum wrote:
>> + __be32 *ssr;
>> + int err;
>> + unsigned int au, eo, et, es;
>> +
>> + if (!IS_ENABLED(CONFIG_MCI_ERASE))
>> + return -ENOSYS;
>
> I think we settled on using -EOPNOTSUPP in this case.
I like -ENOSYS, because it indicates that supper is merely missing instead of not
being available in the first place.
>> + if (mci->can_trim) {
>> + arg = MMC_TRIM_ARG;
>> + } else {
>> + /* We don't use discard, as it doesn't guarantee a fixed value */
>> + arg = MMC_ERASE_ARG;
>> + blkcnt = mmc_align_erase_size(mci, &from, &to, blkcnt);
>> + }
>> +
>> + if (blkcnt == 0)
>> + return 0;
>> +
>> + if (to <= from)
>> + return -EINVAL;
>
> When mmc_align_erase_size() is not called then we cannot arrive here
> as we already returned in the if (blkcnt == 0) check above.
> When mmc_align_erase_size() is called and this test triggers then it
> only reveals a bug in mmc_align_erase_size().
>
> I think this test should go away.
>> + while (i < blkcnt) {
>> + sector_t blk_r;
>> +
>> + /* TODO: While it's possible to clear many erase groups at once
>> + * and it greatly improves throughput, drivers need adjustment:
>> + *
>> + * Many drivers hardcode a maximal wait time before aborting
>> + * the wait for R1b and returning -ETIMEDOUT. With long
>> + * erases/trims, we are bound to run into this timeout, so for now
>> + * we just split into suifficiently small erases that are unlikely
>> + * to trigger the time.
>> + *
>> + * What Linux does and what we should be doing in barebox is:
>> + *
>> + * - add a struct mci_cmd::busy_timeout member that drivers should
>> + * use instead of hardcoding their own timeout delay. The busy
>> + * timeout length can be calculated by the MCI core after
>> + * consulting the appropriate CSD/EXT_CSD/SSR registers.
>> + *
>> + * - add a struct mci_host::max_busy_timeout member, where drivers
>> + * can indicate the maximum timeout they are able to support.
>> + * The MCI core will never set a busy_timeout that exceeds this
>> + * value.
>> + *
>> + * Example Samsung eMMC 8GTF4:
>> + *
>> + * time erase /dev/mmc2.part_of_512m # 1024 trims
>> + * time: 2849ms
>> + *
>> + * time erase /dev/mmc2.part_of_512m # single trim
>> + * time: 56ms
>> + */
>> +
>> + if (IS_SD(mci) && mci->ssr.au) {
>> + blk_r = ((blkcnt - i) > mci->ssr.au) ?
>> + mci->ssr.au : (blkcnt - i);
>> + } else {
>> + blk_r = ((blkcnt - i) > mci->erase_grp_size) ?
>> + mci->erase_grp_size : (blkcnt - i);
>> + }
>> +
>> + rc = mci_block_erase(mci, from, to, arg);
>
> You say you split up the whole erase into sufficiently small erases, but
> 'from' and 'to' are never changed in this loop and you seem to erase
> the whole area multiple times.
Ouch. Will revisit.
Thanks,
Ahmad
>
>> + if (rc)
>> + break;
>> +
>> + /* Waiting for the ready status */
>> + rc = mci_poll_until_ready(mci, 1000 /* ms */);
>> + if (rc)
>> + break;
>> +
>> + i += blk_r;
>> + }
>> +
>> + return i == blkcnt ? 0 : rc;
>> +}
>
> Sascha
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 03/10] block: allow block devices to implement the cdev erase operation
2024-07-30 11:10 ` Ahmad Fatoum
@ 2024-07-30 11:21 ` Sascha Hauer
0 siblings, 0 replies; 29+ messages in thread
From: Sascha Hauer @ 2024-07-30 11:21 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
On Tue, Jul 30, 2024 at 01:10:05PM +0200, Ahmad Fatoum wrote:
> On 30.07.24 10:55, Sascha Hauer wrote:
> > On Tue, Jul 30, 2024 at 09:19:22AM +0200, Ahmad Fatoum wrote:
> >> We mainly implement cdev erase for raw flashes. Many block devices, like
> >> NVMe, SSD or SD/MMC are also flash-based and expose a mechanism or
> >> multiple to trigger an erase on hardware or flash translation layer
> >> level without an accompanied write.
> >>
> >> To make it possible to use this functionality elsewhere in barebox,
> >> let's have the common block device layer pass through the erase
> >> operation, so it may be implemented in a device-specific manner.
> >>
> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> >> ---
> >> common/block.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> >> include/block.h | 1 +
> >> 2 files changed, 44 insertions(+)
> >>
> >> diff --git a/common/block.c b/common/block.c
> >> index f55da775a797..b43fcbe6927b 100644
> >> --- a/common/block.c
> >> +++ b/common/block.c
> >> @@ -380,10 +380,45 @@ static int block_op_discard_range(struct cdev *cdev, loff_t count, loff_t offset
> >> return 0;
> >> }
> >>
> >> +static bool region_overlap(loff_t starta, loff_t lena,
> >> + loff_t startb, loff_t lenb)
> >> +{
> >> + if (starta + lena <= startb)
> >> + return 0;
> >> + if (startb + lenb <= starta)
> >> + return 0;
> >> + return 1;
> >> +}
> >
> > We already have this function in fs/devfs-core.c. Time to export it?
>
> This used to be exported, but the export was removed in commit
> 04e2aa516ed5 ("common.h: move and rename lregion_overlap()").
>
> I can add it back if you prefer.
I think now that we have two users it makes sense to add it back.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 06/10] mci: core: use CONFIG_MCI_WRITE, not CONFIG_BLOCK_WRITE
2024-07-30 11:08 ` Ahmad Fatoum
@ 2024-07-31 7:19 ` Ahmad Fatoum
0 siblings, 0 replies; 29+ messages in thread
From: Ahmad Fatoum @ 2024-07-31 7:19 UTC (permalink / raw)
To: Sascha Hauer; +Cc: barebox
Hello again,
On 30.07.24 13:08, Ahmad Fatoum wrote:
> Hello Sascha,
>
> On 30.07.24 11:18, Sascha Hauer wrote:
>> On Tue, Jul 30, 2024 at 09:19:25AM +0200, Ahmad Fatoum wrote:
>>> There's a more specific CONFIG_MCI_WRITE that's so far only used to
>>> remove write support for in the Atmel MCI driver. We should use the same
>>> symbol also to remove support in the MCI core instead of relying on its
>>> parent CONFIG_BLOCK_WRITE option.
>>
>> Currently CONFIG_MCI_WRITE has no relation to CONFIG_BLOCK_WRITE.
>> Having CONFIG_MCI_WRITE enabled and CONFIG_BLOCK_WRITE disabled doesn't
>> make sense. Shouldn't CONFIG_MCI_WRITE depend on CONFIG_BLOCK_WRITE?
>>
>> Also having CONFIG_BLOCK_WRITE enabled and CONFIG_MCI_WRITE disabled
>> allows you to support writing to block devices that are not MCI devices.
>> Given that the vast majority of block devices are actually MCI devices
>> this seems rather exotic.
>>
>> Maybe we should drop CONFIG_MCI_WRITE and replace it with
>> CONFIG_BLOCK_WRITE, or just have
>>
>> config MCI_WRITE
>> bool
>> default y if BLOCK_WRITE
>
> This works for me. I can do this for v2.
I looked into this and MCI_WRITE selects DISK_WRITE, which selects
BLOCK_WRITE. BLOCK_WRITE has no prompt. I'd prefer to keep this as-is.
>
>
>>
>> Sascha
>>
>>>
>>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>>> ---
>>> drivers/mci/mci-core.c | 8 +++-----
>>> 1 file changed, 3 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
>>> index f6f8a6adabb9..3a5fb0330700 100644
>>> --- a/drivers/mci/mci-core.c
>>> +++ b/drivers/mci/mci-core.c
>>> @@ -1801,8 +1801,8 @@ static int mci_blk_part_switch(struct mci_part *part)
>>> *
>>> * This routine expects the buffer has the correct size to read all data!
>>> */
>>> -static int __maybe_unused mci_sd_write(struct block_device *blk,
>>> - const void *buffer, sector_t block, blkcnt_t num_blocks)
>>> +static int mci_sd_write(struct block_device *blk,
>>> + const void *buffer, sector_t block, blkcnt_t num_blocks)
>>> {
>>> struct mci_part *part = container_of(blk, struct mci_part, blk);
>>> struct mci *mci = part->mci;
>>> @@ -2179,9 +2179,7 @@ static int mci_check_if_already_initialized(struct mci *mci)
>>>
>>> static struct block_device_ops mci_ops = {
>>> .read = mci_sd_read,
>>> -#ifdef CONFIG_BLOCK_WRITE
>>> - .write = mci_sd_write,
>>> -#endif
>>> + .write = IS_ENABLED(CONFIG_MCI_WRITE) ? mci_sd_write : NULL,
>>> };
>>>
>>> static int mci_set_boot(struct param_d *param, void *priv)
>>> --
>>> 2.39.2
>>>
>>>
>>>
>>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2024-07-31 7:20 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-07-30 7:19 [PATCH 00/10] mmc: add SD/eMMC erase support Ahmad Fatoum
2024-07-30 7:19 ` [PATCH 01/10] fs: give erase() a new erase_type parameter Ahmad Fatoum
2024-07-30 8:31 ` Sascha Hauer
2024-07-30 8:32 ` Ahmad Fatoum
2024-07-30 7:19 ` [PATCH 02/10] block: factor out chunk_flush helper Ahmad Fatoum
2024-07-30 7:19 ` [PATCH 03/10] block: allow block devices to implement the cdev erase operation Ahmad Fatoum
2024-07-30 8:55 ` Sascha Hauer
2024-07-30 11:10 ` Ahmad Fatoum
2024-07-30 11:21 ` Sascha Hauer
2024-07-30 7:19 ` [PATCH 04/10] mci: turn bool members into bitfield in struct mci Ahmad Fatoum
2024-07-30 9:06 ` Sascha Hauer
2024-07-30 11:10 ` Ahmad Fatoum
2024-07-30 7:19 ` [PATCH 05/10] mci: describe more command structure in mci.h Ahmad Fatoum
2024-07-30 9:25 ` Yann Sionneau
2024-07-30 11:07 ` Ahmad Fatoum
2024-07-30 7:19 ` [PATCH 06/10] mci: core: use CONFIG_MCI_WRITE, not CONFIG_BLOCK_WRITE Ahmad Fatoum
2024-07-30 9:18 ` Sascha Hauer
2024-07-30 11:08 ` Ahmad Fatoum
2024-07-31 7:19 ` Ahmad Fatoum
2024-07-30 7:19 ` [PATCH 07/10] mci: add support for discarding write blocks Ahmad Fatoum
2024-07-30 9:23 ` Yann Sionneau
2024-07-30 11:14 ` Ahmad Fatoum
2024-07-30 10:05 ` Sascha Hauer
2024-07-30 11:17 ` Ahmad Fatoum
2024-07-30 7:19 ` [PATCH 08/10] commands: sync: add new command to flush cached writes Ahmad Fatoum
2024-07-30 10:08 ` Sascha Hauer
2024-07-30 7:19 ` [PATCH 09/10] mci: core: remove reference to SD Card from common mci_card_probe Ahmad Fatoum
2024-07-30 10:09 ` Sascha Hauer
2024-07-30 7:19 ` [PATCH 10/10] commands: blkstats: add command to print block device statistics Ahmad Fatoum
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox