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

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

[Andre Przywara]

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 am looking forward to any comments on this series!

Cheers,
Andre

Andre Przywara (3):
  fastboot: Allow runtime determination of MMC device
  sunxi: Implement fastboot_get_mmc_device()
  sunxi: Drop sunxi FASTBOOT_FLASH_MMC_DEV defaults

 board/sunxi/board.c           | 37 +++++++++++++++++++++++++++++++++++
 drivers/fastboot/Kconfig      |  4 +---
 drivers/fastboot/fb_command.c |  6 +++---
 drivers/fastboot/fb_common.c  |  3 ++-
 drivers/fastboot/fb_mmc.c     | 12 ++++++++----
 include/fastboot.h            |  7 +++++++
 6 files changed, 58 insertions(+), 11 deletions(-)

-- 
2.17.5

[RFC PATCH 1/3] fastboot: Allow runtime determination of MMC device

[Andre Przywara]

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
recently got more dynamic, and depends on other MMC nodes in the
devicetree, but also on mmc aliases defined. This leads to situations
where it's hard to fix this number at compile time, because a WiFi
device might enumerate before the wanted eMMC device, for instance.

To avoid this fragile situation, allow this value to be determined at
runtime. This decision is probably platform specific, so introduce a
weak function that returns the needed number, and use that everywhere
instead of the Kconfig variable.

For now the default implementation just returns this very Kconfig
variable, but this can be overwritten by platforms later.

No functional change at this point.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/fastboot/fb_command.c |  6 +++---
 drivers/fastboot/fb_common.c  |  3 ++-
 drivers/fastboot/fb_mmc.c     | 12 ++++++++----
 include/fastboot.h            |  7 +++++++
 4 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/fastboot/fb_command.c b/drivers/fastboot/fb_command.c
index 3a5db5b08fc..77184901505 100644
--- a/drivers/fastboot/fb_command.c
+++ b/drivers/fastboot/fb_command.c
@@ -452,7 +452,7 @@ static void oem_format(char *cmd_parameter, char *response)
 		fastboot_fail("partitions not set", response);
 	} else {
 		sprintf(cmdbuf, "gpt write mmc %x $partitions",
-			CONFIG_FASTBOOT_FLASH_MMC_DEV);
+			fastboot_get_mmc_device());
 		if (run_command(cmdbuf, 0))
 			fastboot_fail("", response);
 		else
@@ -479,7 +479,7 @@ static void oem_partconf(char *cmd_parameter, char *response)
 
 	/* execute 'mmc partconfg' command with cmd_parameter arguments*/
 	snprintf(cmdbuf, sizeof(cmdbuf), "mmc partconf %x %s 0",
-		 CONFIG_FASTBOOT_FLASH_MMC_DEV, cmd_parameter);
+		 fastboot_get_mmc_device(), cmd_parameter);
 	printf("Execute: %s\n", cmdbuf);
 	if (run_command(cmdbuf, 0))
 		fastboot_fail("Cannot set oem partconf", response);
@@ -506,7 +506,7 @@ static void oem_bootbus(char *cmd_parameter, char *response)
 
 	/* execute 'mmc bootbus' command with cmd_parameter arguments*/
 	snprintf(cmdbuf, sizeof(cmdbuf), "mmc bootbus %x %s",
-		 CONFIG_FASTBOOT_FLASH_MMC_DEV, cmd_parameter);
+		 fastboot_get_mmc_device(), cmd_parameter);
 	printf("Execute: %s\n", cmdbuf);
 	if (run_command(cmdbuf, 0))
 		fastboot_fail("Cannot set oem bootbus", response);
diff --git a/drivers/fastboot/fb_common.c b/drivers/fastboot/fb_common.c
index cbcc3683c47..a64fefc7d72 100644
--- a/drivers/fastboot/fb_common.c
+++ b/drivers/fastboot/fb_common.c
@@ -101,7 +101,8 @@ int __weak fastboot_set_reboot_flag(enum fastboot_reboot_reason reason)
 	if (reason >= FASTBOOT_REBOOT_REASONS_COUNT)
 		return -EINVAL;
 
-	return bcb_write_reboot_reason(CONFIG_FASTBOOT_FLASH_MMC_DEV, "misc", boot_cmds[reason]);
+	return bcb_write_reboot_reason(fastboot_get_mmc_device(), "misc",
+				       boot_cmds[reason]);
 #else
     return -EINVAL;
 #endif
diff --git a/drivers/fastboot/fb_mmc.c b/drivers/fastboot/fb_mmc.c
index 2f3837e5591..c97e88a3bbd 100644
--- a/drivers/fastboot/fb_mmc.c
+++ b/drivers/fastboot/fb_mmc.c
@@ -76,13 +76,18 @@ static int raw_part_get_info_by_name(struct blk_desc *dev_desc,
 	return 0;
 }
 
+int __weak fastboot_get_mmc_device(void)
+{
+	return CONFIG_FASTBOOT_FLASH_MMC_DEV;
+}
+
 static int do_get_part_info(struct blk_desc **dev_desc, const char *name,
 			    struct disk_partition *info)
 {
 	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("mmc", fastboot_get_mmc_device());
 	if (*dev_desc) {
 		ret = part_get_info_by_name(*dev_desc, name, info);
 		if (ret >= 0)
@@ -489,8 +494,7 @@ 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("mmc", fastboot_get_mmc_device());
 
 	if (!ret || ret->type == DEV_TYPE_UNKNOWN) {
 		pr_err("invalid mmc device\n");
@@ -641,7 +645,7 @@ void fastboot_mmc_erase(const char *cmd, char *response)
 	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(fastboot_get_mmc_device());
 
 #ifdef CONFIG_FASTBOOT_MMC_BOOT_SUPPORT
 	if (strcmp(cmd, CONFIG_FASTBOOT_MMC_BOOT1_NAME) == 0) {
diff --git a/include/fastboot.h b/include/fastboot.h
index 57daaf12982..839877f6a67 100644
--- a/include/fastboot.h
+++ b/include/fastboot.h
@@ -173,6 +173,13 @@ void fastboot_data_download(const void *fastboot_data,
  */
 void fastboot_data_complete(char *response);
 
+/**
+ * fastboot_get_mmc_device() - Get mmc device number of fastboot flash storage
+ *
+ * Return: U-Boot number of MMC device to use as the flash provider.
+ */
+int fastboot_get_mmc_device(void);
+
 #if CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT)
 void fastboot_acmd_complete(void);
 #endif
-- 
2.17.5

[RFC PATCH 2/3] sunxi: Implement fastboot_get_mmc_device()

[Andre Przywara]

The default MMC device to use for the fastboot flash command is hard to
decide at runtime, since the numbering of the MMC devices depends on
devicetree nodes. On the hardware side, the eMMC is connected to device
2, but this might be U-Boot's device 1 or 2, depending on whether the DT
contains a WiFi node for the hardware MMC1 device.

To avoid hardcoding this for each board, let's scan all MMC devices, and
try to find the eMMC device, given that this is enabled. If not, we use
the SD card.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 board/sunxi/board.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index 21651a1bfca..5f64887e48b 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -625,6 +625,43 @@ int board_mmc_init(struct bd_info *bis)
 }
 #endif
 
+#ifdef CONFIG_FASTBOOT_FLASH_MMC
+int fastboot_get_mmc_device(void)
+{
+	struct udevice *dev;
+	static int mmc_dev = -1;
+	int sd_card = -1;
+
+	if (mmc_dev != -1)
+		return mmc_dev;
+
+	for (uclass_first_device(UCLASS_MMC, &dev);
+	     dev;
+	     uclass_next_device(&dev)) {
+		struct mmc *mmc = mmc_get_mmc_dev(dev);
+
+		mmc_init(mmc);
+		if (!mmc->has_init)
+			continue;
+
+		if (IS_SD(mmc)) {
+			sd_card = dev->seq_;
+			continue;
+		} else {
+			mmc_dev =  dev->seq_;
+			break;
+		}
+	}
+
+	if (mmc_dev == -1)
+		mmc_dev = sd_card;
+	if (mmc_dev == -1)
+		mmc_dev = 0;
+
+	return mmc_dev;
+}
+#endif
+
 #ifdef CONFIG_SPL_BUILD
 
 static void sunxi_spl_store_dram_size(phys_addr_t dram_size)
-- 
2.17.5

[RFC PATCH 3/3] sunxi: Drop sunxi FASTBOOT_FLASH_MMC_DEV defaults

[Andre Przywara]

Now that we have a runtime function to decide the default MMC device to
use on Allwinner boards, we don't need the hardcoded Kconfig variable
anymore (we do the same logic at runtime now).

Drop the sunxi defaults, and since there is only one other user left,
use one generic default of 0 now. An "int" type KConfig variable needs a
default anyway, (even when this is never used), otherwise *every* user
has to specify one.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/fastboot/Kconfig | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/fastboot/Kconfig b/drivers/fastboot/Kconfig
index 2d1836a80e0..06536e249c4 100644
--- a/drivers/fastboot/Kconfig
+++ b/drivers/fastboot/Kconfig
@@ -98,9 +98,7 @@ endchoice
 config FASTBOOT_FLASH_MMC_DEV
 	int "Define FASTBOOT MMC FLASH default device"
 	depends on FASTBOOT_FLASH_MMC
-	default 0 if ARCH_ROCKCHIP
-	default 0 if ARCH_SUNXI && MMC_SUNXI_SLOT_EXTRA = -1
-	default 1 if ARCH_SUNXI && MMC_SUNXI_SLOT_EXTRA != -1
+	default 0
 	help
 	  The fastboot "flash" command requires additional information
 	  regarding the non-volatile storage device. Define this to
-- 
2.17.5

Re: [RFC PATCH 2/3] sunxi: Implement fastboot_get_mmc_device()

[Sean Anderson]

On 5/23/21 8:36 PM, Andre Przywara wrote:
> The default MMC device to use for the fastboot flash command is hard to
> decide at runtime, since the numbering of the MMC devices depends on
> devicetree nodes. On the hardware side, the eMMC is connected to device
> 2, but this might be U-Boot's device 1 or 2, depending on whether the DT
> contains a WiFi node for the hardware MMC1 device.

Can you fix this with aliases? e.g.

aliases {
	mmc0 = &mmc0;
	mmc1 = &mmc1;
	mmc2 = &mmc2;
};

That way the mmcs will always have the same number even if (e.g.) hardware mmc 1 is absent.

--Sean

> 
> To avoid hardcoding this for each board, let's scan all MMC devices, and
> try to find the eMMC device, given that this is enabled. If not, we use
> the SD card.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>   board/sunxi/board.c | 37 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 37 insertions(+)
> 
> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> index 21651a1bfca..5f64887e48b 100644
> --- a/board/sunxi/board.c
> +++ b/board/sunxi/board.c
> @@ -625,6 +625,43 @@ int board_mmc_init(struct bd_info *bis)
>   }
>   #endif
>   
> +#ifdef CONFIG_FASTBOOT_FLASH_MMC
> +int fastboot_get_mmc_device(void)
> +{
> +	struct udevice *dev;
> +	static int mmc_dev = -1;
> +	int sd_card = -1;
> +
> +	if (mmc_dev != -1)
> +		return mmc_dev;
> +
> +	for (uclass_first_device(UCLASS_MMC, &dev);
> +	     dev;
> +	     uclass_next_device(&dev)) {
> +		struct mmc *mmc = mmc_get_mmc_dev(dev);
> +
> +		mmc_init(mmc);
> +		if (!mmc->has_init)
> +			continue;
> +
> +		if (IS_SD(mmc)) {
> +			sd_card = dev->seq_;
> +			continue;
> +		} else {
> +			mmc_dev =  dev->seq_;
> +			break;
> +		}
> +	}
> +
> +	if (mmc_dev == -1)
> +		mmc_dev = sd_card;
> +	if (mmc_dev == -1)
> +		mmc_dev = 0;
> +
> +	return mmc_dev;
> +}
> +#endif
> +
>   #ifdef CONFIG_SPL_BUILD
>   
>   static void sunxi_spl_store_dram_size(phys_addr_t dram_size)
> 

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

[Sean Anderson]

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).

--Sean

 >
 > I am looking forward to any comments on this series!
 >
 > Cheers,
 > Andre
 >
 > Andre Przywara (3):
 >    fastboot: Allow runtime determination of MMC device
 >    sunxi: Implement fastboot_get_mmc_device()
 >    sunxi: Drop sunxi FASTBOOT_FLASH_MMC_DEV defaults
 >
 >   board/sunxi/board.c           | 37 +++++++++++++++++++++++++++++++++++
 >   drivers/fastboot/Kconfig      |  4 +---
 >   drivers/fastboot/fb_command.c |  6 +++---
 >   drivers/fastboot/fb_common.c  |  3 ++-
 >   drivers/fastboot/fb_mmc.c     | 12 ++++++++----
 >   include/fastboot.h            |  7 +++++++
 >   6 files changed, 58 insertions(+), 11 deletions(-)
 >

Re: [RFC PATCH 2/3] sunxi: Implement fastboot_get_mmc_device()

[Andre Przywara]

On Mon, 24 May 2021 10:33:58 -0400
Sean Anderson <sean.anderson@seco.com> wrote:

Hi Sean,

> On 5/23/21 8:36 PM, Andre Przywara wrote:
> > The default MMC device to use for the fastboot flash command is hard to
> > decide at runtime, since the numbering of the MMC devices depends on
> > devicetree nodes. On the hardware side, the eMMC is connected to device
> > 2, but this might be U-Boot's device 1 or 2, depending on whether the DT
> > contains a WiFi node for the hardware MMC1 device.  
> 
> Can you fix this with aliases? e.g.
> 
> aliases {
> 	mmc0 = &mmc0;
> 	mmc1 = &mmc1;
> 	mmc2 = &mmc2;
> };
> 
> That way the mmcs will always have the same number even if (e.g.) hardware mmc 1 is absent.

Well, that is partly what we do today: we have a "mmc1=&mmc2;"(!) in
sunxi-u-boot.dtsi, to fix the device name for the eMMC to 1 (the
eMMC hardware controller is always &mmc2).
But actually this is fragile, and might collide when for instance the
above aliases get introduced (Rockchip got those already in Linux, and
there are patches proposed for sunxi as well, but they don't see much
love).
So I would rather see we remove the dependency on certain U-Boot device
numbers, and this patch here being the first step.

As I see it this new solution expresses directly what we want: use the
eMMC is we have one. And do this without relying on certain U-Boot
implementation or hardware details.

Apart from the above aliases being much easier, do you see problems
with this patch here? Happy to hear about any issues. And as it
stands atm, this would only be used for sunxi.

Cheers,
Andre

> > 
> > To avoid hardcoding this for each board, let's scan all MMC devices, and
> > try to find the eMMC device, given that this is enabled. If not, we use
> > the SD card.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >   board/sunxi/board.c | 37 +++++++++++++++++++++++++++++++++++++
> >   1 file changed, 37 insertions(+)
> > 
> > diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> > index 21651a1bfca..5f64887e48b 100644
> > --- a/board/sunxi/board.c
> > +++ b/board/sunxi/board.c
> > @@ -625,6 +625,43 @@ int board_mmc_init(struct bd_info *bis)
> >   }
> >   #endif
> >   
> > +#ifdef CONFIG_FASTBOOT_FLASH_MMC
> > +int fastboot_get_mmc_device(void)
> > +{
> > +	struct udevice *dev;
> > +	static int mmc_dev = -1;
> > +	int sd_card = -1;
> > +
> > +	if (mmc_dev != -1)
> > +		return mmc_dev;
> > +
> > +	for (uclass_first_device(UCLASS_MMC, &dev);
> > +	     dev;
> > +	     uclass_next_device(&dev)) {
> > +		struct mmc *mmc = mmc_get_mmc_dev(dev);
> > +
> > +		mmc_init(mmc);
> > +		if (!mmc->has_init)
> > +			continue;
> > +
> > +		if (IS_SD(mmc)) {
> > +			sd_card = dev->seq_;
> > +			continue;
> > +		} else {
> > +			mmc_dev =  dev->seq_;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (mmc_dev == -1)
> > +		mmc_dev = sd_card;
> > +	if (mmc_dev == -1)
> > +		mmc_dev = 0;
> > +
> > +	return mmc_dev;
> > +}
> > +#endif
> > +
> >   #ifdef CONFIG_SPL_BUILD
> >   
> >   static void sunxi_spl_store_dram_size(phys_addr_t dram_size)
> >   

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

[Andre Przywara]

On Mon, 24 May 2021 10:37:31 -0400
Sean Anderson <sean.anderson@seco.com> 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).

Fine with me. I was actually wondering about this already, but didn't
want to disrupt every user, especially since I can't really test this
very well.

So can you use this explicit device naming from the host side already
with the current code? Can you give an example how this would look
like? The documentation I could find only speaks of the Android
partition names (like "system"), which requires environment variables
to work, IIUC?

Thanks,
Andre

>  >
>  > I am looking forward to any comments on this series!
>  >
>  > Cheers,
>  > Andre
>  >
>  > Andre Przywara (3):
>  >    fastboot: Allow runtime determination of MMC device
>  >    sunxi: Implement fastboot_get_mmc_device()
>  >    sunxi: Drop sunxi FASTBOOT_FLASH_MMC_DEV defaults
>  >
>  >   board/sunxi/board.c           | 37 +++++++++++++++++++++++++++++++++++
>  >   drivers/fastboot/Kconfig      |  4 +---
>  >   drivers/fastboot/fb_command.c |  6 +++---
>  >   drivers/fastboot/fb_common.c  |  3 ++-
>  >   drivers/fastboot/fb_mmc.c     | 12 ++++++++----
>  >   include/fastboot.h            |  7 +++++++
>  >   6 files changed, 58 insertions(+), 11 deletions(-)
>  >  

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2302 bytes --]

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 way the new requirement would be very
obvious instead of introducing a new environment variable no one really
expects?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Sean Anderson]

On 5/24/21 11:15 AM, Andre Przywara wrote:
 > On Mon, 24 May 2021 10:37:31 -0400
 > Sean Anderson <sean.anderson@seco.com> 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).
 >
 > Fine with me. I was actually wondering about this already, but didn't
 > want to disrupt every user, especially since I can't really test this
 > very well.

The ideal way would be to define aliases dynamically mapping old
partition names to new syntax. So if the U-Boot shell were a bit more
expressive, one could do

	for partition in $(mmc part list $mmcdev); do
		env set fastboot_partition_alias_${partition} ${mmcdev}\#${partition}
	done
	fastboot usb 0

which would map old partition names to new-style syntax. Unfortunately,
we don't quite have that capability yet. I think defining a bunch of
aliases when you run fastboot could be... suprising. I'm not sure if
there is a good solution here.

 > So can you use this explicit device naming from the host side already
 > with the current code?  Can you give an example how this would look
 > like?

Flash the partition named "rootfs" on MMC 0 hardware partition 0

$ fastboot flash 0#rootfs root.simg

Flash the entirety of MMC 1 hardware partition 2

$ fastboot flash 1.2:0 boot2.img

For a more thorough treatment of this syntax, see [1]

 > The documentation I could find only speaks of the Android
 > partition names (like "system"), which requires environment variables
 > to work, IIUC?

They require that you use a GPT-formatted disk with partition labels.
Alternatively, you can use aliases. There is one sentence on the
new-style syntax here [2], but I really should expand on it...

--Sean

[1] https://u-boot.readthedocs.io/en/latest/usage/partitions.html#partitions
[2] https://u-boot.readthedocs.io/en/latest/android/fastboot.html#partition-names

 >
 > Thanks,
 > Andre
 >
 >>   >
 >>   > I am looking forward to any comments on this series!
 >>   >
 >>   > Cheers,
 >>   > Andre
 >>   >
 >>   > Andre Przywara (3):
 >>   >    fastboot: Allow runtime determination of MMC device
 >>   >    sunxi: Implement fastboot_get_mmc_device()
 >>   >    sunxi: Drop sunxi FASTBOOT_FLASH_MMC_DEV defaults
 >>   >
 >>   >   board/sunxi/board.c           | 37 +++++++++++++++++++++++++++++++++++
 >>   >   drivers/fastboot/Kconfig      |  4 +---
 >>   >   drivers/fastboot/fb_command.c |  6 +++---
 >>   >   drivers/fastboot/fb_common.c  |  3 ++-
 >>   >   drivers/fastboot/fb_mmc.c     | 12 ++++++++----
 >>   >   include/fastboot.h            |  7 +++++++
 >>   >   6 files changed, 58 insertions(+), 11 deletions(-)
 >>   >
 >

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

[Sean Anderson]

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

Re: [RFC PATCH 1/3] fastboot: Allow runtime determination of MMC device

[Simon Glass]

Hi Andre,

On Sun, 23 May 2021 at 18:37, Andre Przywara <andre.przywara@arm.com> 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
> recently got more dynamic, and depends on other MMC nodes in the
> devicetree, but also on mmc aliases defined. This leads to situations
> where it's hard to fix this number at compile time, because a WiFi
> device might enumerate before the wanted eMMC device, for instance.
>
> To avoid this fragile situation, allow this value to be determined at
> runtime. This decision is probably platform specific, so introduce a
> weak function that returns the needed number, and use that everywhere
> instead of the Kconfig variable.
>
> For now the default implementation just returns this very Kconfig
> variable, but this can be overwritten by platforms later.
>
> No functional change at this point.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  drivers/fastboot/fb_command.c |  6 +++---
>  drivers/fastboot/fb_common.c  |  3 ++-
>  drivers/fastboot/fb_mmc.c     | 12 ++++++++----
>  include/fastboot.h            |  7 +++++++
>  4 files changed, 20 insertions(+), 8 deletions(-)

I wonder if this would be better done using a sysinfo driver? Then it
could be hard-coded, picked up from the DT, probed at runtime, etc.,
just as with a weak function.

Regards,
Simon

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3350 bytes --]

On Mon, May 24, 2021 at 11:43:43AM -0400, Sean Anderson wrote:
> 
> 
> 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.

I don't deny the fact that it can be convenient. What I was pointing out
is that it's been optional the whole time, that I've been using fastboot
for like 5 years and didn't realize that this feature was there, and
it's only implemented for mmc.

To make it required, without any warning, would be fairly confusing.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Sean Anderson]

On 6/7/21 11:14 AM, Maxime Ripard wrote:
 > On Mon, May 24, 2021 at 11:43:43AM -0400, Sean Anderson wrote:
 >>
 >>
 >> 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.
 >
 > I don't deny the fact that it can be convenient. What I was pointing out
 > is that it's been optional the whole time, that I've been using fastboot
 > for like 5 years and didn't realize that this feature was there, and
 > it's only implemented for mmc.
 >
 > To make it required, without any warning, would be fairly confusing.

Well, it would only be required if you need to set the MMC device at
runtime. For existing users, there is no change required. I'm
specifically pointing out that this functionality is already achievable
without any changes to C code.

--Sean