Re: [PATCH] drivers: fastboot: Enable flashing support for UFS

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

Hi Varadarajan,

Thank you for the patch.

On mar., nov. 26, 2024 at 10:46, Varadarajan Narayanan <quic_varada@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@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