Re: [PATCH RFC u-boot-mvebu 20/59] cmd: mvebu/bubt: Add support for selecting eMMC HW partition

Stefan Roese
2023-02-23T06:17:37Z

Hi Pali,

On 2/22/23 19:06, Pali Rohár wrote:
> On Wednesday 22 February 2023 10:55:54 Stefan Roese wrote:
>> Hi Pali,
>>
>> On 2/21/23 21:18, Pali Rohár wrote:
>>> Support for burning into the correct eMMC HW boot partition was broken and
>>> removed in commit 96be2f072768 ("mvebu: bubt: Drop dead code"). Reimplement
>>> this functionality and bring it back again.
>>>
>>> Fixes: 96be2f072768 ("mvebu: bubt: Drop dead code")
>>> Signed-off-by: Pali Rohár <pali@kernel.org>
>>> ---
>>> cmd/mvebu/bubt.c | 53 ++++++++++++++++++++++++++++++++++++++----
>>> doc/mvebu/cmd/bubt.txt | 21 ++++++++++++-----
>>> 2 files changed, 63 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c
>>> index 2bcdf145f64a..4bad9a69527c 100644
>>> --- a/cmd/mvebu/bubt.c
>>> +++ b/cmd/mvebu/bubt.c
>>> @@ -189,6 +189,11 @@ static int mmc_burn_image(size_t image_size)
>>> #ifdef CONFIG_BLK
>>> struct blk_desc *blk_desc;
>>> #endif
>>> +#ifdef CONFIG_SUPPORT_EMMC_BOOT
>>> + u8 part;
>>> + u8 orig_part;
>>> +#endif
>>> +
>>> mmc = find_mmc_device(mmc_dev_num);
>>> if (!mmc) {
>>> printf("No SD/MMC/eMMC card found\n");
>>> @@ -202,6 +207,38 @@ static int mmc_burn_image(size_t image_size)
>>> return err;
>>> }
>>> +#ifdef CONFIG_BLK
>>> + blk_desc = mmc_get_blk_desc(mmc);
>>> + if (!blk_desc) {
>>> + printf("Error - failed to obtain block descriptor\n");
>>> + return -ENODEV;
>>> + }
>>> +#endif
>>
>> This #ifdef usage really gets out of hand IMHO. Yes I know, you did not
>> introduce it here. Still, perhaps some of this can be moved to using
>> IS_ENABLED or CONFIG_IS_ENABLED instead?
>>
>> And I just checked that "bubt" is not compiled for CONFIG_BLK not
>> defined. So please depend this file on CONFIG_BLK in Kconfig and remove
>> the non CONFIG_BLK code here.
>
> bubt is used for at least 4 different mvebu platforms (AXP, A38x, A3720,
> A7K) and every one has slightly different code and configuration.
>
> I really do not know if all those configurations support CONFIG_BLK, so
> I decided to let it as it was before to eliminate possible issues.

As mentioned above, I've checked this myself before sending the
suggestion yesterday. No platform compiling this cmd has CONFIG_BLK
disabled.

My understanding is that CONFIG_BLK will be required at some time and
all non CONFIG_BLK code will get dropped.

Still, if you feel you don't want to make this change, as it's not
really related to your current patchset, then this is no showstopper.

Thanks,
Stefan

>> Thanks,
>> Stefan
>>
>>> +
>>> +#ifdef CONFIG_SUPPORT_EMMC_BOOT
>>> +#ifdef CONFIG_BLK
>>> + orig_part = blk_desc->hwpart;
>>> +#else
>>> + orig_part = mmc->block_dev.hwpart;
>>> +#endif
>>> +
>>> + part = (mmc->part_config >> 3) & PART_ACCESS_MASK;
>>> +
>>> + if (part == 7)
>>> + part = 0;
>>> +
>>> +#ifdef CONFIG_BLK
>>> + err = blk_dselect_hwpart(blk_desc, part);
>>> +#else
>>> + err = mmc_switch_part(mmc, part);
>>> +#endif
>>> +
>>> + if (err) {
>>> + printf("Error - MMC partition switch failed\n");
>>> + return err;
>>> + }
>>> +#endif
>>> +
>>> /* SD reserves LBA-0 for MBR and boots from LBA-1,
>>> * MMC/eMMC boots from LBA-0
>>> */
>>> @@ -211,11 +248,6 @@ static int mmc_burn_image(size_t image_size)
>>> if (image_size % mmc->write_bl_len)
>>> blk_count += 1;
>>> - blk_desc = mmc_get_blk_desc(mmc);
>>> - if (!blk_desc) {
>>> - printf("Error - failed to obtain block descriptor\n");
>>> - return -ENODEV;
>>> - }
>>> blk_written = blk_dwrite(blk_desc, start_lba, blk_count,
>>> (void *)get_load_addr());
>>> #else
>>> @@ -227,6 +259,17 @@ static int mmc_burn_image(size_t image_size)
>>> start_lba, blk_count,
>>> (void *)get_load_addr());
>>> #endif /* CONFIG_BLK */
>>> +
>>> +#ifdef CONFIG_SUPPORT_EMMC_BOOT
>>> +#ifdef CONFIG_BLK
>>> + err = blk_dselect_hwpart(blk_desc, orig_part);
>>> +#else
>>> + err = mmc_switch_part(mmc, orig_part);
>>> +#endif
>>> + if (err)
>>> + printf("Error - MMC failed to switch back to original partition\n");
>>> +#endif
>>> +
>>> if (blk_written != blk_count) {
>>> printf("Error - written %#lx blocks\n", blk_written);
>>> return -ENOSPC;
>>> diff --git a/doc/mvebu/cmd/bubt.txt b/doc/mvebu/cmd/bubt.txt
>>> index 6051243f1165..1fe1f07dd187 100644
>>> --- a/doc/mvebu/cmd/bubt.txt
>>> +++ b/doc/mvebu/cmd/bubt.txt
>>> @@ -14,8 +14,7 @@ Examples:
>>> Notes:
>>> - For the TFTP interface set serverip and ipaddr.
>>> -- To burn image to SD/eMMC device, the target is defined
>>> - by parameters CONFIG_SYS_MMC_ENV_DEV and CONFIG_SYS_MMC_ENV_PART.
>>> +- To burn image to SD/eMMC device, the target is defined by HW partition.
>>> Bubt command details (burn image step by-step)
>>> ----------------------------------------------
>>> @@ -40,10 +39,20 @@ Notes:
>>> Number 0 is used for user data partition and should not be utilized for storing
>>> boot images and U-Boot environment in RAW mode since it will break file system
>>> structures usually located here.
>>> - The default boot partition is BOOT0. It is selected by the following parameter:
>>> - CONFIG_SYS_MMC_ENV_PART=1
>>> - Valid values for this parameter are 1 for BOOT0 and 2 for BOOT1.
>>> - Please never use partition number 0 here!
>>> +
>>> + Currently configured boot partition can be printed by command:
>>> + # mmc partconf 0
>>> + (search for BOOT_PARTITION_ACCESS output, number 7 is user data)
>>> +
>>> + Change it to BOOT0:
>>> + # mmc partconf 0 0 1 1
>>> +
>>> + Change it to BOOT1:
>>> + # mmc partconf 0 0 2 2
>>> +
>>> + Change it to user data:
>>> + # mmc partconf 0 0 7 0
>>> +
>>> - The partition number is ignored if the target device is SD card.
>>> - The boot image offset starts at block 0 for eMMC and block 1 for SD devices.
>>> The block 0 on SD devices is left for MBR storage.
>>
>> Viele Grüße,
>> Stefan Roese
>>
>> --
>> DENX Software Engineering GmbH, Managing Director: Erika Unter
>> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Viele Grüße,
Stefan Roese

--
DENX Software Engineering GmbH, Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de