On 17 February 2015 at 02:51, Steve Rae <s...@broadcom.com> wrote:
>
>
> On 15-02-16 12:40 PM, Dileep Katta wrote:
>
>> Hi Steve,
>>
>> On 14 February 2015 at 02:15, Steve Rae <s...@broadcom.com> wrote:
>>
>>
>>>
>>> On 15-02-12 12:29 AM, Dileep Katta wrote:
>>>
>>> Hi Steve,
>>>>
>>>> On 11 February 2015 at 05:25, Steve Rae <s...@broadcom.com> 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 <dileep.ka...@linaro.org>
>>>>>> ---
>>>>>> Note: The changes are on top of oem command support added by
>>>>>> r...@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 <part.h>
>>>>>> #include <aboot.h>
>>>>>> #include <sparse_format.h>
>>>>>> +#include <mmc.h>
>>>>>>
>>>>>> #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.
Regards,
Dileep
>
>
>
>>>
>>>
>>>
>>>>> +
>>>>>
>>>>>> + printf("Erasing blocks " LBAFU " to " LBAFU " due to
>>>>>> alignment\n",
>>>>>> + blks_start, blks_start + blks_size);
>>>>>> +
>>>>>> + blks = dev_desc->block_erase(dev_desc->dev, blks_start,
>>>>>> blks_size);
>>>>>> + if (blks != blks_size) {
>>>>>> + error("failed erasing from device %d\n",
>>>>>> dev_desc->dev);
>>>>>>
>>>>>>
>>>>> no newline with error()
>>>>>
>>>>> Will remove
>>>>
>>>>
>>>>> + fastboot_fail("failed erasing from device");
>>>>>
>>>>>> + return;
>>>>>> + }
>>>>>> +
>>>>>> + printf("........ erased " LBAFU " bytes from '%s'\n",
>>>>>> + blks_size * info.blksz, cmd);
>>>>>> + fastboot_okay("");
>>>>>> +}
>>>>>> diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_
>>>>>> fastboot.c
>>>>>> index f7d84bf..a8d8205 100644
>>>>>> --- a/drivers/usb/gadget/f_fastboot.c
>>>>>> +++ b/drivers/usb/gadget/f_fastboot.c
>>>>>> @@ -535,6 +535,26 @@ static void cb_oem(struct usb_ep *ep, struct
>>>>>> usb_request *req)
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> +static void cb_erase(struct usb_ep *ep, struct usb_request *req)
>>>>>> +{
>>>>>> + char *cmd = req->buf;
>>>>>> + char response[RESPONSE_LEN];
>>>>>> +
>>>>>> + strsep(&cmd, ":");
>>>>>> + if (!cmd) {
>>>>>> + error("missing partition name\n");
>>>>>>
>>>>>>
>>>>> no newline with error()
>>>>>
>>>>> Will remove
>>>>
>>>>
>>>>> + fastboot_tx_write_str("FAILmissing partition name");
>>>>>
>>>>>> + return;
>>>>>> + }
>>>>>> +
>>>>>> + strcpy(response, "FAILno flash device defined");
>>>>>> +
>>>>>> +#ifdef CONFIG_FASTBOOT_FLASH_MMC_DEV
>>>>>> + fb_mmc_erase(cmd, response);
>>>>>> +#endif
>>>>>> + fastboot_tx_write_str(response);
>>>>>> +}
>>>>>> +
>>>>>> struct cmd_dispatch_info {
>>>>>> char *cmd;
>>>>>> void (*cb)(struct usb_ep *ep, struct usb_request *req);
>>>>>> @@ -566,6 +586,9 @@ static const struct cmd_dispatch_info
>>>>>> cmd_dispatch_info[] = {
>>>>>> {
>>>>>> .cmd = "oem",
>>>>>> .cb = cb_oem,
>>>>>> + }, {
>>>>>> + .cmd = "erase",
>>>>>> + .cb = cb_erase,
>>>>>> },
>>>>>> };
>>>>>>
>>>>>> diff --git a/include/fb_mmc.h b/include/fb_mmc.h
>>>>>> index 1ad1d13..402ba9b 100644
>>>>>> --- a/include/fb_mmc.h
>>>>>> +++ b/include/fb_mmc.h
>>>>>> @@ -6,3 +6,4 @@
>>>>>>
>>>>>> void fb_mmc_flash_write(const char *cmd, void *download_buffer,
>>>>>> unsigned int download_bytes, char
>>>>>> *response);
>>>>>> +void fb_mmc_erase(const char *cmd, char *response);
>>>>>>
>>>>>>
>>>>>> Thanks for the review. Will send v2 with changes.
>>>>
>>>> Regards,
>>>> Dileep
>>>>
>>>>
>>>>
>>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
