On Tue, Dec 05, 2023 at 20:54, Simon Glass <s...@chromium.org> wrote:
> 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.
Understood, thank you for clarifying !
>
>>
>> >
>> > 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
