Re: [PATCH v2] cmd: fastboot: default the buffer address to $fastboot_addr_r

[E Shattow <e@freeshell.de>]

Hi Igor,

On 11/2/26 14:08, Igor Opaniuk wrote:
> The fastboot command downloads to CONFIG_FASTBOOT_BUF_ADDR unless -l is
> given. Platforms that only know a usable address at run time leave that
> at 0 and put the address in the fastboot_addr_r environment variable:
> mach-snapdragon allocates the buffer from free memory, and so does the
> Samsung exynos-mobile board. Their environments wrap the command in
> "fastboot -l $fastboot_addr_r usb 0", but a plain "fastboot usb 0"
> still downloads to address 0.
> 
> Use fastboot_addr_r when it is set. -l still overrides it, and boards
> without the variable keep using CONFIG_FASTBOOT_BUF_ADDR.
> 
> Signed-off-by: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>
> ---
> Tested on a Qualcomm QCM2290 board with qcom_defconfig: before, the
> first "fastboot stage" after a plain "fastboot usb 0" crashed U-Boot.
> With the patch a 100 MiB download lands at $fastboot_addr_r (CRC32 of
> the buffer matches the file), and "fastboot -l ${ramdisk_addr_r} usb 0"
> still places it at the given address.
> 
> Refusing to start with a zero address instead was not an option:
> sandbox uses address 0 as its buffer.
> ---
> Changes in v2:
> - Leave the command help text as it was: it describes what the user
>   can pass, not how the default is chosen (E Shattow)
> - Reword the fastboot_addr_r paragraph in doc/android/fastboot.rst
>   (E Shattow)
> - Link to v1: https://lore.kernel.org/r/20261102-fastboot-addr-r-v1-1-7ef1a2ac8fd1@oss.qualcomm.com
> ---
>  cmd/fastboot.c           | 4 +++-
>  doc/android/fastboot.rst | 8 +++++---
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/cmd/fastboot.c b/cmd/fastboot.c
> index ace877b6e28..795c3138cbb 100644
> --- a/cmd/fastboot.c
> +++ b/cmd/fastboot.c
> @@ -7,6 +7,7 @@
>   * Rob Herring <robh@kernel.org>
>   */
>  #include <command.h>
> +#include <env.h>
>  #include <fastboot.h>
>  #include <net.h>
>  #include <vsprintf.h>
> @@ -88,7 +89,8 @@ static int do_fastboot_usb(int argc, char *const argv[],
>  static int do_fastboot(struct cmd_tbl *cmdtp, int flag, int argc,
>  		       char *const argv[])
>  {
> -	uintptr_t buf_addr = (uintptr_t)NULL;
> +	/* Boards which place the buffer at run time put it in fastboot_addr_r */
> +	uintptr_t buf_addr = env_get_hex("fastboot_addr_r", 0);
>  	size_t buf_size = 0;
>  
>  	if (argc < 2)
> diff --git a/doc/android/fastboot.rst b/doc/android/fastboot.rst
> index 8e15f287f9b..ba817e8c1eb 100644
> --- a/doc/android/fastboot.rst
> +++ b/doc/android/fastboot.rst
> @@ -68,9 +68,11 @@ General configuration
>  The fastboot protocol requires a large memory buffer for
>  downloads. This buffer should be as large as possible for a
>  platform. The location of the buffer and size are set with
> -``CONFIG_FASTBOOT_BUF_ADDR`` and ``CONFIG_FASTBOOT_BUF_SIZE``. These
> -may be overridden on the fastboot command line using ``-l`` and
> -``-s``.
> +``CONFIG_FASTBOOT_BUF_ADDR`` and ``CONFIG_FASTBOOT_BUF_SIZE``. If the
> +``fastboot_addr_r`` environment variable is set, its value is used as
> +the buffer address instead of ``CONFIG_FASTBOOT_BUF_ADDR``, which lets
> +a platform choose the address at run time. The address and size may be
> +overridden on the fastboot command line using ``-l`` and ``-s``.

It looks better than it was, and now I do understand what you are
writing. There are three-or-four sources of information here into the logic:

1. Build time

2. Strictly U-Boot environment

3. Fastboot server/client requests  (and here, does the client request
override the server command line? so... potentially split to #3 and #4
sources of information here into the logic)

Does each source described after the previous source implicitly override
the previous source?

Is there a functional reason we did not mention both address and size
configurations for all of the above sources (notably no mention of
fastboot buffer size override as environment variable)?

The word ordering flows more naturally if it is "here is a stack of two
related things A1 and B1, now we override those with A2 and B2, and
again override with A3 and B3, and finally A4 and B4." If it's more
complex than this perhaps we should just dump this into a formatted
table and not bother describing it more than "Order of precedence" table
name. If it is not so complex then it should be said in one or two
sentences that simply list the order of precedence.

>  
>  Fastboot in SPL
>  ^^^^^^^^^^^^^^^
> 
> ---
> base-commit: 3973f3c4d24df75ca90d7c0a9f03e5e5c9b48f32
> change-id: 20261102-fastboot-addr-r-28fe6ea5615f
> 
> Best regards,

I do not want to go far beyond what you're trying to fix in this one
patch but it does open more questions for me now that I understand it.
Let me know what you think, thanks!

-E