Hi Mattijs, On Tue, 5 Dec 2023 at 02:16, Mattijs Korpershoek <mkorpersh...@baylibre.com> wrote: > > Hi Simon, > > Thank you for your patch. > > On dim., déc. 03, 2023 at 17:31, Simon Glass <s...@chromium.org> wrote: > > > Given the name of this variable, it should be an address, not a > > pointer. Update this, to make it easier to use with sandbox. > > > > Signed-off-by: Simon Glass <s...@chromium.org> > > Tested that I could reflash AOSP on the Khadas VIM 3 board. > > Tested-by: Mattijs Korpershoek <mkorpersh...@baylibre.com> # on vim3 > > Reviewed-by: Mattijs Korpershoek <mkorpersh...@baylibre.com> > > Some small nit/question below. > > > --- > > > > cmd/fastboot.c | 2 +- > > drivers/fastboot/fb_command.c | 13 ++++++++----- > > drivers/fastboot/fb_common.c | 15 ++++----------- > > include/fastboot-internal.h | 2 +- > > include/fastboot.h | 6 +++--- > > 5 files changed, 17 insertions(+), 21 deletions(-) > > > > diff --git a/cmd/fastboot.c b/cmd/fastboot.c > > index c3c19231c988..792e83d372c3 100644 > > --- a/cmd/fastboot.c > > +++ b/cmd/fastboot.c > > @@ -159,7 +159,7 @@ NXTARG: > > return CMD_RET_USAGE; > > } > > > > - fastboot_init((void *)buf_addr, buf_size); > > + fastboot_init(buf_addr, buf_size); > > > > if (!strcmp(argv[1], "udp")) > > return do_fastboot_udp(argc, argv, buf_addr, buf_size); > > diff --git a/drivers/fastboot/fb_command.c b/drivers/fastboot/fb_command.c > > index 5fcadcdf503d..ec030886dbb8 100644 > > --- a/drivers/fastboot/fb_command.c > > +++ b/drivers/fastboot/fb_command.c > > @@ -10,6 +10,7 @@ > > #include <fastboot-internal.h> > > #include <fb_mmc.h> > > #include <fb_nand.h> > > +#include <mapmem.h> > > #include <part.h> > > #include <stdlib.h> > > #include <linux/printk.h> > > @@ -252,7 +253,7 @@ void fastboot_data_download(const void *fastboot_data, > > return; > > } > > /* Download data to fastboot_buf_addr */ > > - memcpy(fastboot_buf_addr + fastboot_bytes_received, > > + memcpy(map_sysmem(fastboot_buf_addr, 0) + fastboot_bytes_received, > > fastboot_data, fastboot_data_len); > > I'm a little confused on why there is no call of unmap_sysmem() here but > that seems to be the case for plenty other callers of map_sysmem(). > > From what I've seen, in the sandbox unmap_sysmem() is a no-oop, but what > about other architectures ?
Yes it is always a nop. It is designed such that address 0 can be used as RAM in sandbox. For PCI the mapping can in fact do something special, so that memory-mapped peripherals can expose their registers. See drivers/misc/swap_case.c for an example of that. But we have not ended up with lots of tests. > > > > > pre_dot_num = fastboot_bytes_received / BYTES_PER_DOT; > > @@ -296,13 +297,15 @@ void fastboot_data_complete(char *response) > > */ > > static void __maybe_unused flash(char *cmd_parameter, char *response) > > { > > + void *buf = map_sysmem(fastboot_buf_addr, 0); > > + > > if (IS_ENABLED(CONFIG_FASTBOOT_FLASH_MMC)) > > - fastboot_mmc_flash_write(cmd_parameter, fastboot_buf_addr, > > - image_size, response); > > + fastboot_mmc_flash_write(cmd_parameter, buf, image_size, > > + response); > > > > if (IS_ENABLED(CONFIG_FASTBOOT_FLASH_NAND)) > > - fastboot_nand_flash_write(cmd_parameter, fastboot_buf_addr, > > - image_size, response); > > + fastboot_nand_flash_write(cmd_parameter, buf, image_size, > > + response); > > Same here. > > > } > > [..] Regards, Simon