Re: [RFC PATCH 0/3] fastboot: sunxi: Determine MMC device at runtime

From: Sean Anderson <sean.anderson@seco.com>
To: Maxime Ripard <maxime@cerno.tech>
Cc: Andre Przywara <andre.przywara@arm.com>,
	Simon Glass <sjg@chromium.org>,
	Jagan Teki <jagan@amarulasolutions.com>,
	u-boot@lists.denx.de, linux-sunxi@lists.linux.dev,
	Patrick Delaunay <patrick.delaunay@foss.st.com>,
	Heiko Schocher <hs@denx.de>,
	Kever Yang <kever.yang@rock-chips.com>,
	Philipp Tomsich <philipp.tomsich@vrull.eu>
Subject: Re: [RFC PATCH 0/3] fastboot: sunxi: Determine MMC device at runtime
Date: Mon, 24 May 2021 11:43:43 -0400	[thread overview]
Message-ID: <9f77283d-109a-25ad-d4be-4867ee47d0db@seco.com> (raw)
In-Reply-To: <20210524152831.unrg7zexsdbz6uki@gilmour>

On 5/24/21 11:28 AM, Maxime Ripard wrote:
 > On Mon, May 24, 2021 at 10:37:31AM -0400, Sean Anderson wrote:
 >>
 >>
 >> On 5/23/21 8:36 PM, Andre Przywara wrote:
 >>> At the moment the fastboot code relies on the Kconfig variable
 >>> CONFIG_FASTBOOT_FLASH_MMC_DEV to point to the MMC device to use for the
 >>> flash command. This value needs to be the *U-Boot device number*, which
 >>> is picked by the U-Boot device model at runtime. This makes it quite
 >>> tricky and fragile to fix this variable at compile time, as other DT
 >>> nodes and aliases influence the enumeration process.
 >>>
 >>> To make this process more robust, allow the device number to be picked at
 >>> runtime, which sounds like a better fit to find this device number. Patch
 >>> 1/3 introduces a weak function for that purpose.
 >>> Patch 2/3 then implements this function for the Allwinner platform. The
 >>> code follows the idea behind the existing Kconfig defaults: Use the eMMC
 >>> device, if that exists, or the SD card otherwise. This patch is actually
 >>> not sunxi specific, so might be adopted by other platforms as well.
 >>> Patch 3/3 then drops the existing Kconfig defaults for sunxi, to clean
 >>> this up and remove the implicit assumption that the eMMC device is always
 >>> device 1 (as least for the fastboot code).
 >>>
 >>> I would be curious if others think this is the right way forward.
 >>> The fact that the U-Boot device numbers are determined at runtime
 >>> seems to conflict with the idea of a Kconfig variable in the first place,
 >>> hence this series. This brings us one step closer to the end goal of
 >>> removing the "eMMC is device 1" assumption.
 >>
 >> I would actually favor removing CONFIG_FASTBOOT_FLASH_MMC_DEV
 >> altogether, and just specifying the device explicitly in fastboot
 >> commands. If you need to dynamically change the device, you can create
 >> some aliases. E.g. you could have something like
 >>
 >> "fastboot_aliases=setenv fastboot_partition_alias_user ${mmcdev}.0:0"
 >>
 >> and then run this variable just before calling `fastboot 0` (or whatever
 >> your usb device is).
 >
 > If we're going that way, maybe we can just pass the interface on the
 > command line like dfu does?

That could work, but I don't think it's necessary. At the moment there
are many different ways to specify partitions (KConfig, Aliases, "U-boot
syntax", GPT partition labels). I would rather pare things down to the
minimum necessary ways than add yet another bit of state to specify
partitions.

 > That way the new requirement would be very obvious instead of
 > introducing a new environment variable no one really expects?

I'm not sure what you mean here. This alias system has been in place for
a while, and it's very convenient for mapping a stable name to some
arbitrary device and partition.

--Sean