From mboxrd@z Thu Jan  1 00:00:00 1970
From: Oleh Kravchenko <oleg@kaa.org.ua>
Date: Sat, 15 May 2021 01:27:33 +0300
Subject: [PATCH v3] Fix flashing of eMMC user area with Fastboot
In-Reply-To: <2fb45bd6-b9fe-a1d8-4f9c-6a8a7f564420@kaa.org.ua>
References: <20210514211505.26722-1-oleg@kaa.org.ua>
 <fb3f1721-8092-921c-8d42-692f1c2a9636@kaa.org.ua>
 <531a3c07-9fa4-e9c0-b9cb-fc65d845283f@seco.com>
 <2fb45bd6-b9fe-a1d8-4f9c-6a8a7f564420@kaa.org.ua>
Message-ID: <7351c5d6-2545-7a78-89d6-2003b93fbebe@kaa.org.ua>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hello Sean,
Please see my comments below:

15.05.21 01:10, Oleh Kravchenko ????:
> Hello Sean,
> Could you please clarify what you have mean?
> 
> I think you pointing to this?
>> fastboot_raw_partition_<raw partition name>=<offset> <size> [mmcpart <num>]
> 
> Because I don't have idea how aliases will help to flash.
> 
> 15.05.21 00:45, Sean Anderson ????:
>> On 5/14/21 5:26 PM, Oleh Kravchenko wrote:
>>> Hello guys,
>>> Could you please review and merge this patch?
>>
>> Did you have any luck with the second suggestion [1] I made for your
>> original patch?
>>
>> [1] https://lists.denx.de/pipermail/u-boot/2021-May/449778.html
>>
>> --Sean

Hey, I've just tested these:

uboot> setenv fastboot_raw_partition_user '0 3850371072 mmcpart 0'
uboot> setenv fastboot_raw_partition_boot0 '0 16777216 mmcpart 1'
uboot> setenv fastboot_raw_partition_boot1 '0 16777216 mmcpart 2'

linux> fastboot flash core-image-minimal.wic
linux> fastboot flash boot0 u-boot.bin 
linux> fastboot flash boot1 u-boot.bin 

And it works!

-- Oleh

>>
>>>
>>> PR successfully passed CI:
>>> https://github.com/u-boot/u-boot/pull/75
>>>
>>> 15.05.21 00:15, Oleh Kravchenko ????:
>>>> 'gpt' and 'mmc0' fastboot partitions have been treated as the same device,
>>>> but it is wrong.
>>>>
>>>> Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
>>>> Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
>>>> Cc: Marek Vasut <marex@denx.de>
>>>> ---
>>>> Changes for v2:
>>>> ???? - code cleanup;
>>>> Changes for v3:
>>>> ???? - QA passed at https://github.com/u-boot/u-boot/pull/75;
>>>>
>>>> ?? drivers/fastboot/fb_mmc.c | 25 ++++++++++++++++++++-----
>>>> ?? 1 file changed, 20 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/fastboot/fb_mmc.c b/drivers/fastboot/fb_mmc.c
>>>> index 2f3837e559..647d3f6c1b 100644
>>>> --- a/drivers/fastboot/fb_mmc.c
>>>> +++ b/drivers/fastboot/fb_mmc.c
>>>> @@ -532,12 +532,7 @@ void fastboot_mmc_flash_write(const char *cmd, void *download_buffer,
>>>> ?? #endif
>>>>
>>>> ?? #if CONFIG_IS_ENABLED(EFI_PARTITION)
>>>> -#ifndef CONFIG_FASTBOOT_MMC_USER_SUPPORT
>>>> ?????? if (strcmp(cmd, CONFIG_FASTBOOT_GPT_NAME) == 0) {
>>>> -#else
>>>> -??? if (strcmp(cmd, CONFIG_FASTBOOT_GPT_NAME) == 0 ||
>>>> -??????? strcmp(cmd, CONFIG_FASTBOOT_MMC_USER_NAME) == 0) {
>>>> -#endif
>>>> ?????????? dev_desc = fastboot_mmc_get_dev(response);
>>>> ?????????? if (!dev_desc)
>>>> ?????????????? return;
>>>> @@ -599,9 +594,29 @@ void fastboot_mmc_flash_write(const char *cmd, void *download_buffer,
>>>> ?????? }
>>>> ?? #endif
>>>>
>>>> +#if CONFIG_IS_ENABLED(FASTBOOT_MMC_USER_SUPPORT)
>>
>> Is it possible to use
>>
>> if (CONFIG_IS_ENABLED(...)) { ... }
>>
>> here?
>>

No,
It will cause an error where CONFIG_FASTBOOT_MMC_USER_NAME not defined.

-- Oleh

>>>> +??? if (strcmp(cmd, CONFIG_FASTBOOT_MMC_USER_NAME) == 0) {
>>>> +??????? dev_desc = fastboot_mmc_get_dev(response);
>>>> +??????? if (!dev_desc)
>>>> +??????????? return;
>>>> +
>>>> +??????? memset(&info, 0, sizeof(info));
>>>> +??????? info.start??? = 0;
>>>> +??????? info.size??? = dev_desc->lba;
>>>> +??????? info.blksz??? = dev_desc->blksz;
>>>> +??????? strlcpy((char *)&info.name, cmd, sizeof(info.name));
>>>> +
>>>> +??????? goto write_image;
>>
>> Why do we need to skip get_part_info?
>>

fastboot_mmc_get_part_info() doesn't check for CONFIG_FASTBOOT_MMC_USER_NAME.

>>>> +??? }
>>>> +#endif
>>>> +
>>>> ?????? if (fastboot_mmc_get_part_info(cmd, &dev_desc, &info, response) < 0)
>>>> ?????????? return;
>>>>
>>>> +#if CONFIG_IS_ENABLED(FASTBOOT_MMC_USER_SUPPORT)
>>
>> Is conditionally defining this label necessary?
>>
>> --Sean
>>

Yes,
in other case some configuration causes error goto label is not used.

-- Oleh


>>>> +write_image:
>>>> +#endif
>>>> +
>>>> ?????? if (is_sparse_image(download_buffer)) {
>>>> ?????????? struct fb_mmc_sparse sparse_priv;
>>>> ?????????? struct sparse_storage sparse;
>>>>
>>>
> 

-- 
Best regards,
Oleh Kravchenko