From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matt Reimer Date: Tue, 17 Feb 2015 08:55:40 -0700 Subject: [U-Boot] [PATCH v1 1/1] usb: gadget: fastboot: Add fastboot erase In-Reply-To: References: <1423558178-23105-1-git-send-email-dileep.katta@linaro.org> <54DA7735.6070605@broadcom.com> <54DE625B.8050702@broadcom.com> <54E25F52.1030403@broadcom.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Tue, Feb 17, 2015 at 5:57 AM, Dileep Katta wrote: > On 17 February 2015 at 02:51, Steve Rae wrote: > > > > > > > On 15-02-16 12:40 PM, Dileep Katta wrote: > > > >> Hi Steve, > >> > >> On 14 February 2015 at 02:15, Steve Rae wrote: > >> > >> > >>> > >>> On 15-02-12 12:29 AM, Dileep Katta wrote: > >>> > >>> Hi Steve, > >>>> > >>>> On 11 February 2015 at 05:25, Steve Rae wrote: > >>>> > >>>> > >>>>> Hi, Dileep > >>>>> > >>>>> > >>>>> On 15-02-10 12:49 AM, Dileep Katta wrote: > >>>>> > >>>>> > >>>>>> Adds the fastboot erase functionality, to erase a partition > >>>>>> specified by name. The erase is performed based on erase group size, > >>>>>> to avoid erasing other partitions. The start address and the size > >>>>>> is aligned to the erase group size for this. > >>>>>> > >>>>>> Currently only supports erasing from eMMC. > >>>>>> > >>>>>> Signed-off-by: Dileep Katta > >>>>>> --- > >>>>>> Note: The changes are on top of oem command support added by > >>>>>> robh at kernel.org > >>>>>> > >>>>>> common/fb_mmc.c | 58 > >>>>>> ++++++++++++++++++++++++++++++ > >>>>>> +++++++++++ > >>>>>> drivers/usb/gadget/f_fastboot.c | 23 ++++++++++++++++ > >>>>>> include/fb_mmc.h | 1 + > >>>>>> 3 files changed, 82 insertions(+) > >>>>>> > >>>>>> diff --git a/common/fb_mmc.c b/common/fb_mmc.c > >>>>>> index 6ea3938..3911989 100644 > >>>>>> --- a/common/fb_mmc.c > >>>>>> +++ b/common/fb_mmc.c > >>>>>> @@ -10,6 +10,7 @@ > >>>>>> #include > >>>>>> #include > >>>>>> #include > >>>>>> +#include > >>>>>> > >>>>>> #ifndef CONFIG_FASTBOOT_GPT_NAME > >>>>>> #define CONFIG_FASTBOOT_GPT_NAME GPT_ENTRY_NAME > >>>>>> @@ -110,3 +111,60 @@ void fb_mmc_flash_write(const char *cmd, void > >>>>>> *download_buffer, > >>>>>> write_raw_image(dev_desc, &info, cmd, > >>>>>> download_buffer, > >>>>>> download_bytes); > >>>>>> } > >>>>>> + > >>>>>> +void fb_mmc_erase(const char *cmd, char *response) > >>>>>> +{ > >>>>>> + int ret; > >>>>>> + block_dev_desc_t *dev_desc; > >>>>>> + disk_partition_t info; > >>>>>> + lbaint_t blks, blks_start, blks_size, grp_size; > >>>>>> + struct mmc *mmc = find_mmc_device(CONFIG_ > >>>>>> FASTBOOT_FLASH_MMC_DEV); > >>>>>> + > >>>>>> + if (mmc == NULL) { > >>>>>> + error("invalid mmc device\n"); > >>>>>> > >>>>>> > >>>>> no newline with error() > >>>>> > >>>>> Will remove > >>>> > >>>> > >>>>> + fastboot_fail("invalid mmc device"); > >>>>> > >>>>>> + return; > >>>>>> + } > >>>>>> + > >>>>>> + /* initialize the response buffer */ > >>>>>> + response_str = response; > >>>>>> + > >>>>>> + dev_desc = get_dev("mmc", CONFIG_FASTBOOT_FLASH_MMC_DEV); > >>>>>> + if (!dev_desc || dev_desc->type == DEV_TYPE_UNKNOWN) { > >>>>>> + error("invalid mmc device\n"); > >>>>>> > >>>>>> > >>>>> no newline with error() > >>>>> > >>>>> Will remove > >>>> > >>>> > >>>>> > >>>>> + fastboot_fail("invalid mmc device"); > >>>>> > >>>>>> + return; > >>>>>> + } > >>>>>> + > >>>>>> + ret = get_partition_info_efi_by_name(dev_desc, cmd, &info); > >>>>>> + if (ret) { > >>>>>> + error("cannot find partition: '%s'\n", cmd); > >>>>>> > >>>>>> > >>>>> no newline with error() > >>>>> > >>>>> Will remove > >>>> > >>>> > >>>>> + fastboot_fail("cannot find partition"); > >>>>> > >>>>>> + return; > >>>>>> + } > >>>>>> + > >>>>>> + puts("Erasing partition\n"); > >>>>>> + > >>>>>> + /* Align blocks to erase group size to avoid erasing other > >>>>>> partitions */ > >>>>>> + grp_size = mmc->erase_grp_size; > >>>>>> + blks_start = (info.start + grp_size - 1) & ~(grp_size - 1); > >>>>>> + if (info.size >= grp_size) > >>>>>> + blks_size = (info.size - (blks_start - info.start)) > & > >>>>>> + (~(grp_size - 1)); > >>>>>> + else > >>>>>> + blks_size = 0; > >>>>>> > >>>>>> > >>>>> > >>>>> Is this logic correct??? Isn't the "erase_grp_size" in bytes? and the > >>>>> info.start & info.size in LBA's? > >>>>> > >>>>> Yes, the math will take care of it. Ref: mmc_berase() > >>>> > >>>> > >>> So, I have a partition: > >>> > >>> 2 0x00000300 0x000003ff "test" > >>> attrs: 0x0000000000000000 > >>> type: 9e312af1-18fe-fa41-45f3-37b3bb1d1061 > >>> guid: 18a5d0db-d23a-aac1-0d4c-692c7ba9ab1c > >>> > >>> and 'fastboot erase test' produces: > >>> Erasing blocks 1024 to 1024 due to alignment > >>> ........ erased 0 bytes from 'test' > >>> which is not correct! > >>> > >>> Furthermore, doesn't the mmc_berase() already handle the > >>> "erase_grp_size".... > >>> > >>> > >> It does handle it, in a way, but the way it handles it is to erase more > >> blocks than requested if the request isn't aligned. In your example, you > >> requested the "test" partition to be erased (0x300 to 0x3ff), but what > was > >> actually erased (as printed in the "Caution" message from mmc_berase) > was > >> 0x0 to 0x7ff. > >> > >> If I remove your alignment logic, then it produces: > >> > >>> Erasing blocks 768 to 1024 due to alignment > >>> > >>> Caution! Your devices Erase group is 0x400 > >>> The erase range would be change to 0x0~0x7ff > >>> > >>> ........ erased 131072 bytes from 'test' > >>> which looks correct (except for the "Caution" message).... > >>> > >>> Thanks, Steve > >>> > >> > >> > >> Except that this one has now erased 0x0 to 0x300 and 0x400 to 0x7ff > also, > >> which you did not want to erase, right? > >> Aligning the start address is meant to protect this data from being > erased > >> unintentionally. The trade-off is that some small partitions won't get > >> erased at all, if they are too small and not aligned. > >> > >> Regards, Dileep > >> > >> I'm sorry, but I am really confused.... > > -- 131072 bytes agrees with 0x300 to 0x3ff... > > -- I'm suspicious that the Caution message and the actual erased bytes > do > > not match.... > > -- I'm not certain that the existing mmc_berase() code is correct... > > -- If this "erase_grp_size" is a method to erase large sections > > "quickly", then it would seem that it needs to be combined with erasing > (by > > writing "0" or "1" appropriately) to the smaller sections to erase the > > "exact specified size". And this should be handled in the mmc_berase() > code! > > > > Thanks for your suggestions Steve. I will try to send a separate patch for > mmc_berase() if required, after validating all the scenarios. > > > > Additionally, because it is not necessary to "erase before writing", I > > personally do not have any need for this "fastboot erase" command at all. > > So I think that I am going to withdraw from this conversation.... > > Thanks, Steve > > > We wish fastboot to be complete with all the commands of the > protocol supported in u-boot. > Actually according to this comment from fastboot (system/core/fastboot/fastboot.c), sometimes it is necessary to erase before flashing: /* Until we get lazy inode table init working in make_ext4fs, we need to * erase partitions of type ext4 before flashing a filesystem so no stale * inodes are left lying around. Otherwise, e2fsck gets very upset. */ In order for fastboot to know that it needs to erase the partition first, we'll also need to implement a fastboot var "partition-type" that will return "ext4" for ext4 partitions. See system/core/fastboot/engine.c:fb_format_supported(). Matt