| Message ID | 3hD.ZbWV.1{t85GxWb7T.1crWYj@seznam.cz |
|---|---|
| State | New |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [v3] cmd: mmc: Allow using partition name in mmc erase command | expand |
Hi Tomas, On 9/2/24 8:49 PM, Tomas Paukrt wrote: > The mmc erase command currently requires blk# and cnt parameters > which can be obtained using the part start and part size commands > if the entire partition needs to be erased. > > Simplify the use of the mmc erase command by allowing the partition > name to be specified directly. > > Signed-off-by: Tomas Paukrt <tomaspaukrt@email.cz> > --- > Changes since v1: > - Swapped init_mmc_device call and argc check > - Removed round down info.size to a multiple of erase_grp_size > - Split the syntax into two separate lines > --- > cmd/mmc.c | 17 +++++++++++++---- > doc/usage/cmd/mmc.rst | 6 +++++- > 2 files changed, 18 insertions(+), 5 deletions(-) > > diff --git a/cmd/mmc.c b/cmd/mmc.c > index 7244a90..2fc05f9 100644 > --- a/cmd/mmc.c > +++ b/cmd/mmc.c > @@ -472,18 +472,26 @@ static int do_mmc_erase(struct cmd_tbl *cmdtp, int flag, > int argc, char *const argv[]) > { > struct mmc *mmc; > + struct disk_partition info; > u32 blk, cnt, n; > > - if (argc != 3) > + if (argc < 2 || argc > 3) > return CMD_RET_USAGE; > > - blk = hextoul(argv[1], NULL); > - cnt = hextoul(argv[2], NULL); > - > mmc = init_mmc_device(curr_device, false); > if (!mmc) > return CMD_RET_FAILURE; > > + if (argc == 3) { > + blk = hextoul(argv[1], NULL); > + cnt = hextoul(argv[2], NULL); > + } else if (part_get_info_by_name(mmc_get_blk_desc(mmc), argv[1], &info) >= 0) { > + blk = info.start; > + cnt = info.size; Same question as for v1, are we sure that the partition block size is the same as the MMC erase block size? Is this guaranteed by all standards (all MMC standards, and all partition table standards)? Otherwise I assume we should do some maths here to figure out the MMC start block AND block size based on the partition start block AND block size and to pass the correct information to blk_derase(). I hope they also are guaranteed to be aligned somehow and that a partition block size is a multiple of an MMC erase block size? This maths seems to be done in mmc_berase() (at a quick glance, not sure it's appropriate). Cheers, Quentin > + } else { > + return CMD_RET_FAILURE; > + } > + > printf("\nMMC erase: dev # %d, block # %d, count %d ... ", > curr_device, blk, cnt); > > @@ -1271,6 +1279,7 @@ U_BOOT_CMD( > "mmc swrite addr blk#\n" > #endif > "mmc erase blk# cnt\n" > + "mmc erase partname\n" > "mmc rescan [mode]\n" > "mmc part - lists available partition on current mmc device\n" > "mmc dev [dev] [part] [mode] - show or set current mmc device [partition] and set mode\n" > diff --git a/doc/usage/cmd/mmc.rst b/doc/usage/cmd/mmc.rst > index 5a64400..55391fd 100644 > --- a/doc/usage/cmd/mmc.rst > +++ b/doc/usage/cmd/mmc.rst > @@ -15,6 +15,7 @@ Synopsis > mmc read addr blk# cnt > mmc write addr blk# cnt > mmc erase blk# cnt > + mmc erase partname > mmc rescan [mode] > mmc part > mmc dev [dev] [part] [mode] > @@ -44,12 +45,15 @@ The 'mmc write' command writes raw data to MMC device from memory address with b > cnt > block count > > -The 'mmc erase' command erases *cnt* blocks on the MMC device starting at block *blk#*. > +The 'mmc erase' command erases *cnt* blocks on the MMC device starting at block *blk#* or > +the entire partition specified by *partname*. > > blk# > start block offset > cnt > block count > + partname > + partition name > > The 'mmc rescan' command scans the available MMC device. >
> Same question as for v1, are we sure that the partition block size is > the same as the MMC erase block size? Is this guaranteed by all > standards (all MMC standards, and all partition table standards)? > Otherwise I assume we should do some maths here to figure out the MMC > start block AND block size based on the partition start block AND block > size and to pass the correct information to blk_derase(). I hope they > also are guaranteed to be aligned somehow and that a partition block > size is a multiple of an MMC erase block size? This maths seems to be > done in mmc_berase() (at a quick glance, not sure it's appropriate). Hi Quentin, I'm no expert on U-Boot internals, but if you look e.g. to part_get_info_whole_disk() then you can see that blksz is directly copied from struct blk_desc to struct disk_partition. Both the start block and block size in struct disk_partition are multiples of blksz, so I think that they can be passed directly to blk_derase(), which later calls mmc_berase(). Best regards Tomas
Hi Tomas, On 9/3/24 12:14 PM, Tomas Paukrt wrote: >> Same question as for v1, are we sure that the partition block size is >> the same as the MMC erase block size? Is this guaranteed by all >> standards (all MMC standards, and all partition table standards)? >> Otherwise I assume we should do some maths here to figure out the MMC >> start block AND block size based on the partition start block AND block >> size and to pass the correct information to blk_derase(). I hope they >> also are guaranteed to be aligned somehow and that a partition block >> size is a multiple of an MMC erase block size? This maths seems to be >> done in mmc_berase() (at a quick glance, not sure it's appropriate). > > Hi Quentin, > > I'm no expert on U-Boot internals, but if you look e.g. to part_get_info_whole_disk() > then you can see that blksz is directly copied from struct blk_desc to struct disk_partition. > Both the start block and block size in struct disk_partition are multiples of blksz, so I think that > they can be passed directly to blk_derase(), which later calls mmc_berase(). > Makes sense, thanks for the pointers. Looks good to me, Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de> Thanks! Quentin
diff --git a/cmd/mmc.c b/cmd/mmc.c index 7244a90..2fc05f9 100644 --- a/cmd/mmc.c +++ b/cmd/mmc.c @@ -472,18 +472,26 @@ static int do_mmc_erase(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { struct mmc *mmc; + struct disk_partition info; u32 blk, cnt, n; - if (argc != 3) + if (argc < 2 || argc > 3) return CMD_RET_USAGE; - blk = hextoul(argv[1], NULL); - cnt = hextoul(argv[2], NULL); - mmc = init_mmc_device(curr_device, false); if (!mmc) return CMD_RET_FAILURE; + if (argc == 3) { + blk = hextoul(argv[1], NULL); + cnt = hextoul(argv[2], NULL); + } else if (part_get_info_by_name(mmc_get_blk_desc(mmc), argv[1], &info) >= 0) { + blk = info.start; + cnt = info.size; + } else { + return CMD_RET_FAILURE; + } + printf("\nMMC erase: dev # %d, block # %d, count %d ... ", curr_device, blk, cnt); @@ -1271,6 +1279,7 @@ U_BOOT_CMD( "mmc swrite addr blk#\n" #endif "mmc erase blk# cnt\n" + "mmc erase partname\n" "mmc rescan [mode]\n" "mmc part - lists available partition on current mmc device\n" "mmc dev [dev] [part] [mode] - show or set current mmc device [partition] and set mode\n" diff --git a/doc/usage/cmd/mmc.rst b/doc/usage/cmd/mmc.rst index 5a64400..55391fd 100644 --- a/doc/usage/cmd/mmc.rst +++ b/doc/usage/cmd/mmc.rst @@ -15,6 +15,7 @@ Synopsis mmc read addr blk# cnt mmc write addr blk# cnt mmc erase blk# cnt + mmc erase partname mmc rescan [mode] mmc part mmc dev [dev] [part] [mode] @@ -44,12 +45,15 @@ The 'mmc write' command writes raw data to MMC device from memory address with b cnt block count -The 'mmc erase' command erases *cnt* blocks on the MMC device starting at block *blk#*. +The 'mmc erase' command erases *cnt* blocks on the MMC device starting at block *blk#* or +the entire partition specified by *partname*. blk# start block offset cnt block count + partname + partition name The 'mmc rescan' command scans the available MMC device.
The mmc erase command currently requires blk# and cnt parameters which can be obtained using the part start and part size commands if the entire partition needs to be erased. Simplify the use of the mmc erase command by allowing the partition name to be specified directly. Signed-off-by: Tomas Paukrt <tomaspaukrt@email.cz> --- Changes since v1: - Swapped init_mmc_device call and argc check - Removed round down info.size to a multiple of erase_grp_size - Split the syntax into two separate lines --- cmd/mmc.c | 17 +++++++++++++---- doc/usage/cmd/mmc.rst | 6 +++++- 2 files changed, 18 insertions(+), 5 deletions(-)