[PATCH] drivers: fastboot: Enable flashing support for UFS
Mattijs Korpershoek
mkorpershoek at baylibre.com
Tue Nov 26 10:19:56 CET 2024
Hi Varadarajan,
Thank you for the patch.
On mar., nov. 26, 2024 at 10:46, Varadarajan Narayanan <quic_varada at quicinc.com> wrote:
> On Mon, Nov 25, 2024 at 06:59:40PM +0100, Caleb Connolly wrote:
>> Hi Varadarajan,
>>
>> On 25/11/2024 12:03, Varadarajan Narayanan wrote:
>> > MMC and UFS seem to execute very similar steps to identify the
>> > partition and write to it. Hence try to enable fastboot flashing
>> > support in UFS leveraging the MMC functions.
>>
>> Thanks for this! Do you think we could make this dynamic at runtime?
>> Since it's currently possible to build a single binary with
>> qcom_defconfig that will work across a wide range of boards (both with
>> eMMC and UFS), I don't think it makes sense to pick a fastboot backend
>> at build time.
>>
>> Some additional comments below.
>> >
>> > Tested on rb3gen2 platform.
>> >
>> > Signed-off-by: Varadarajan Narayanan <quic_varada at quicinc.com>
Seems there is a bit of an overlap with Dmitrii's work here:
https://patchwork.ozlabs.org/project/uboot/patch/20240306185921.1854109-2-dimorinny@google.com/
>> > ---
>> > drivers/fastboot/Kconfig | 19 +++++++++++++++---
>> > drivers/fastboot/Makefile | 1 +
>> > drivers/fastboot/fb_command.c | 6 ++++--
>> > drivers/fastboot/fb_mmc.c | 36 ++++++++++++++++++++++++-----------
>> > 4 files changed, 46 insertions(+), 16 deletions(-)
>> >
>> > diff --git a/drivers/fastboot/Kconfig b/drivers/fastboot/Kconfig
>> > index 1eb460f5a0..5c41fc4684 100644
>> > --- a/drivers/fastboot/Kconfig
>> > +++ b/drivers/fastboot/Kconfig
>> > @@ -92,7 +92,7 @@ config FASTBOOT_USB_DEV
>> > config FASTBOOT_FLASH
>> > bool "Enable FASTBOOT FLASH command"
>> > default y if ARCH_SUNXI || ARCH_ROCKCHIP
>> > - depends on MMC || (MTD_RAW_NAND && CMD_MTDPARTS)
>> > + depends on MMC || UFS || (MTD_RAW_NAND && CMD_MTDPARTS)
>> > select IMAGE_SPARSE
>> > help
>> > The fastboot protocol includes a "flash" command for writing
>> > @@ -120,6 +120,10 @@ config FASTBOOT_FLASH_NAND
>> > bool "FASTBOOT on NAND"
>> > depends on MTD_RAW_NAND && CMD_MTDPARTS
>> >
>> > +config FASTBOOT_FLASH_UFS
>> > + bool "FASTBOOT on UFS"
>> > + depends on UFS && EFI_PARTITION
>> > +
>> > endchoice
>> >
>> > config FASTBOOT_FLASH_MMC_DEV
>> > @@ -133,6 +137,15 @@ config FASTBOOT_FLASH_MMC_DEV
>> > regarding the non-volatile storage device. Define this to
>> > the eMMC device that fastboot should use to store the image.
>> >
>> > +config FASTBOOT_FLASH_UFS_DEV
>> > + int "Define FASTBOOT UFS FLASH default device"
>> > + depends on FASTBOOT_FLASH_UFS
>> > + default 0 if CONFIG_ARCH_SNAPDRAGON
Remove CONFIG_*
Should be:
default 0 if ARCH_SNAPDRAGON
>> > + help
>> > + The fastboot "flash" command requires additional information
>> > + regarding the non-volatile storage device. Define this to
>> > + the UFS device that fastboot should use to store the image.
>>
>> This doesn't map at all to how fastboot is intended to work. We would
>> expect to see all partitions exposed not just those of a specific LUN.
>
> Yes. Took this approach as this would easily align with the MMC's
> way of searching for a partition. Will fix this.
I agree with Caleb's point.
>
>> > +
>> > config FASTBOOT_FLASH_NAND_TRIMFFS
>> > bool "Skip empty pages when flashing NAND"
>> > depends on FASTBOOT_FLASH_NAND
>> > @@ -196,7 +209,7 @@ config FASTBOOT_MMC_USER_NAME
>> >
>> > config FASTBOOT_GPT_NAME
>> > string "Target name for updating GPT"
>> > - depends on FASTBOOT_FLASH_MMC && EFI_PARTITION
>> > + depends on (FASTBOOT_FLASH_MMC || FASTBOOT_FLASH_UFS) && EFI_PARTITION
>> > default "gpt"
>> > help
>> > The fastboot "flash" command supports writing the downloaded
>> > @@ -209,7 +222,7 @@ config FASTBOOT_GPT_NAME
>> >
>> > config FASTBOOT_MBR_NAME
>> > string "Target name for updating MBR"
>> > - depends on FASTBOOT_FLASH_MMC && DOS_PARTITION
>> > + depends on (FASTBOOT_FLASH_MMC || FASTBOOT_FLASH_UFS) && DOS_PARTITION
>> > default "mbr"
>> > help
>> > The fastboot "flash" command allows to write the downloaded image
>> > diff --git a/drivers/fastboot/Makefile b/drivers/fastboot/Makefile
>> > index 048af5aa82..772276cb6c 100644
>> > --- a/drivers/fastboot/Makefile
>> > +++ b/drivers/fastboot/Makefile
>> > @@ -4,4 +4,5 @@ obj-y += fb_common.o
>> > obj-y += fb_getvar.o
>> > obj-y += fb_command.o
>> > obj-$(CONFIG_FASTBOOT_FLASH_MMC) += fb_mmc.o
>> > +obj-$(CONFIG_FASTBOOT_FLASH_UFS) += fb_mmc.o
>> > obj-$(CONFIG_FASTBOOT_FLASH_NAND) += fb_nand.o
>> > diff --git a/drivers/fastboot/fb_command.c b/drivers/fastboot/fb_command.c
>> > index e4484d65ac..a6fa8299b8 100644
>> > --- a/drivers/fastboot/fb_command.c
>> > +++ b/drivers/fastboot/fb_command.c
>> > @@ -336,7 +336,8 @@ void fastboot_data_complete(char *response)
>> > */
>> > static void __maybe_unused flash(char *cmd_parameter, char *response)
>> > {
>> > - if (IS_ENABLED(CONFIG_FASTBOOT_FLASH_MMC))
>> > + if (IS_ENABLED(CONFIG_FASTBOOT_FLASH_MMC) ||
>> > + IS_ENABLED(CONFIG_FASTBOOT_FLASH_UFS))
>> > fastboot_mmc_flash_write(cmd_parameter, fastboot_buf_addr,
>> > image_size, response);
>> >
>> > @@ -356,7 +357,8 @@ static void __maybe_unused flash(char *cmd_parameter, char *response)
>> > */
>> > static void __maybe_unused erase(char *cmd_parameter, char *response)
>> > {
>> > - if (IS_ENABLED(CONFIG_FASTBOOT_FLASH_MMC))
>> > + if (IS_ENABLED(CONFIG_FASTBOOT_FLASH_MMC) ||
>> > + IS_ENABLED(CONFIG_FASTBOOT_FLASH_UFS))
>> > fastboot_mmc_erase(cmd_parameter, response);
>> >
>> > if (IS_ENABLED(CONFIG_FASTBOOT_FLASH_NAND))
>> > diff --git a/drivers/fastboot/fb_mmc.c b/drivers/fastboot/fb_mmc.c
>> > index f11eb66761..bf242e4b2a 100644
>> > --- a/drivers/fastboot/fb_mmc.c
>> > +++ b/drivers/fastboot/fb_mmc.c
>> > @@ -20,6 +20,16 @@
>> >
>> > #define BOOT_PARTITION_NAME "boot"
>> >
>> > +#if defined(CONFIG_FASTBOOT_FLASH_MMC)
>> > +#define fb_flash_str "mmc"
>>
>> Macros be in upper case.
I agree with Caleb's point.
Also, if this has to stay at compile time, I prefer Dmitrii's approach
using FASTBOOT_FLASH_BLOCK_INTERFACE_NAME.
See: https://patchwork.ozlabs.org/project/uboot/patch/20240306185921.1854109-2-dimorinny@google.com/
>>
>> This is something I think would make more sense as a variable. Perhaps
>> the fastboot command could be extended to allow passing in a device (or
>> list of devices?) to use.
>
> Considered that approach but didn't follow through since it might
> break the existing command syntax. Should we extend the 'flash'
> command itself or should we add an 'oem' command to select mmc or
> ufs.
I don't think we should extend the 'flash' command since the fastboot
documentation states:
flash %s Flash a given partition. Optional arguments include
--slot-other, {filename_path}, --apply-vbmeta
See:
https://android.googlesource.com/platform/system/core/+/main/fastboot/README.md#flashing-logic
If this is made dynamically (which seems like a good idea to me), then
I'd prefer this to be an oem command.
>
>> > +#define fb_flash_dev CONFIG_FASTBOOT_FLASH_MMC_DEV
>> > +#elif defined(CONFIG_FASTBOOT_FLASH_UFS)
>> > +#define fb_flash_str "scsi"
>> > +#define fb_flash_dev CONFIG_FASTBOOT_FLASH_UFS_DEV
>> > +#else
>> > +#error "Incorrect flash type"
>> > +#endif
With the current implementation, if both CONFIG_FASTBOOT_FLASH_MMC and
CONFIG_FASTBOOT_FLASH_UFS are enabled, we cannot use UFS because MMC
takes priority.
>> > +
>> > struct fb_mmc_sparse {
>> > struct blk_desc *dev_desc;
>> > };
>> > @@ -78,7 +88,7 @@ static int do_get_part_info(struct blk_desc **dev_desc, const char *name,
>> > int ret;
>> >
>> > /* First try partition names on the default device */
>> > - *dev_desc = blk_get_dev("mmc", CONFIG_FASTBOOT_FLASH_MMC_DEV);
>> > + *dev_desc = blk_get_dev(fb_flash_str, fb_flash_dev);
>> > if (*dev_desc) {
>> > ret = part_get_info_by_name(*dev_desc, name, info);
>> > if (ret >= 0)
>> > @@ -91,8 +101,8 @@ static int do_get_part_info(struct blk_desc **dev_desc, const char *name,
>> > }
>> >
>> > /* Then try dev.hwpart:part */
>> > - ret = part_get_info_by_dev_and_name_or_num("mmc", name, dev_desc,
>> > - info, true);
>> > + ret = part_get_info_by_dev_and_name_or_num(fb_flash_str, name,
>> > + dev_desc, info, true);
>> > return ret;
>> > }
>> >
>> > @@ -202,17 +212,20 @@ static int fb_mmc_erase_mmc_hwpart(struct blk_desc *dev_desc)
>> > {
>> > lbaint_t blks;
>> >
>> > - debug("Start Erasing mmc hwpart[%u]...\n", dev_desc->hwpart);
>> > + debug("Start Erasing %s hwpart[%u]...\n",
>> > + fb_flash_str, dev_desc->hwpart);
>> >
>> > blks = fb_mmc_blk_write(dev_desc, 0, dev_desc->lba, NULL);
>> >
>> > if (blks != dev_desc->lba) {
>> > - pr_err("Failed to erase mmc hwpart[%u]\n", dev_desc->hwpart);
>> > + pr_err("Failed to erase %s hwpart[%u]\n",
>> > + fb_flash_str, dev_desc->hwpart);
>> > return 1;
>> > }
>> >
>> > - printf("........ erased %lu bytes from mmc hwpart[%u]\n",
>> > - dev_desc->lba * dev_desc->blksz, dev_desc->hwpart);
>> > + printf("........ erased %lu bytes from %s hwpart[%u]\n",
>> > + dev_desc->lba * dev_desc->blksz, fb_flash_str,
>> > + dev_desc->hwpart);
>> >
>> > return 0;
>> > }
>> > @@ -487,11 +500,10 @@ int fastboot_mmc_get_part_info(const char *part_name,
>> >
>> > static struct blk_desc *fastboot_mmc_get_dev(char *response)
>> > {
>> > - struct blk_desc *ret = blk_get_dev("mmc",
>> > - CONFIG_FASTBOOT_FLASH_MMC_DEV);
>> > + struct blk_desc *ret = blk_get_dev(fb_flash_str, fb_flash_dev);
>> >
>> > if (!ret || ret->type == DEV_TYPE_UNKNOWN) {
>> > - pr_err("invalid mmc device\n");
>> > + pr_err("invalid %s device\n", fb_flash_str);
>>
>> pr_err("invalid "fb_flash_str" device\n"); -- and the same for all
>> other cases.
>
> Not sure if this will be applicable if we move to dynamic mmc or
> ufs selection. Will see.
I agree with Caleb's comment.
>
>> > fastboot_fail("invalid mmc device", response);
>> > return NULL;
>> > }
>> > @@ -644,10 +656,11 @@ void fastboot_mmc_flash_write(const char *cmd, void *download_buffer,
>> > */
>> > void fastboot_mmc_erase(const char *cmd, char *response)
>> > {
>> > +#ifdef CONFIG_FASTBOOT_FLASH_MMC
>> > struct blk_desc *dev_desc;
>> > struct disk_partition info;
>> > lbaint_t blks, blks_start, blks_size, grp_size;
>> > - struct mmc *mmc = find_mmc_device(CONFIG_FASTBOOT_FLASH_MMC_DEV);
>> > + struct mmc *mmc = find_mmc_device(fb_flash_dev);
>> >
>> > #ifdef CONFIG_FASTBOOT_MMC_BOOT_SUPPORT
>> > if (strcmp(cmd, CONFIG_FASTBOOT_MMC_BOOT1_NAME) == 0) {
>> > @@ -706,5 +719,6 @@ void fastboot_mmc_erase(const char *cmd, char *response)
>> >
>> > printf("........ erased " LBAFU " bytes from '%s'\n",
>> > blks_size * info.blksz, cmd);
>> > +#endif
>> > fastboot_okay(NULL, response);
>>
>> I don't think it's really acceptable to just lie that we erase a
>> partition when in fact we did nothing at all.
>>
>> This should respond with some error indicating that the command isn't
>> supported.
>
> Sure.
I agree with Caleb's comment.
>
> Will post a new version shortly.
>
> Thanks for the feedback.
>
> -Varada
More information about the U-Boot
mailing list