Re: [PATCH v6 16/25] spl: Convert fat to spl_load

[Sean Anderson <seanga2@gmail.com>]

On 11/7/23 23:24, Simon Glass wrote:
> Hi Sean,
> 
> On Sun, 5 Nov 2023 at 19:26, Sean Anderson <seanga2@gmail.com> wrote:
>>
>> This converts the fat loader to use spl_load. Some platforms are very
>> tight on space, so we take care to only include the code we really need.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> ---
>>
>> (no changes since v5)
>>
>> Changes in v5:
>> - Rework to load header in spl_load
>>
>> Changes in v3:
>> - Fix failing on success
>>
>>   common/spl/spl_fat.c     | 56 +++++++++++++---------------------------
>>   include/spl_load.h       |  1 +
>>   test/image/spl_load_fs.c |  3 +--
>>   3 files changed, 20 insertions(+), 40 deletions(-)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> nits below
> 
>>
>> diff --git a/common/spl/spl_fat.c b/common/spl/spl_fat.c
>> index a0c34eba48f..569f2b32928 100644
>> --- a/common/spl/spl_fat.c
>> +++ b/common/spl/spl_fat.c
>> @@ -11,8 +11,8 @@
>>   #include <common.h>
>>   #include <env.h>
>>   #include <log.h>
>> -#include <mapmem.h>
>>   #include <spl.h>
>> +#include <spl_load.h>
>>   #include <asm/u-boot.h>
>>   #include <fat.h>
>>   #include <errno.h>
>> @@ -66,58 +66,38 @@ int spl_load_image_fat(struct spl_image_info *spl_image,
>>                         const char *filename)
>>   {
>>          int err;
>> -       struct legacy_img_hdr *header;
>> +       loff_t size;
>> +       struct spl_load_info load;
>>
>>          err = spl_register_fat_device(block_dev, partition);
>>          if (err)
>>                  goto end;
>>
>> -       header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
>> -
>> -       err = file_fat_read(filename, header, sizeof(struct legacy_img_hdr));
>> -       if (err <= 0)
>> -               goto end;
>> -
>> -       if (IS_ENABLED(CONFIG_SPL_LOAD_FIT_FULL) &&
>> -           image_get_magic(header) == FDT_MAGIC) {
>> -               err = file_fat_read(filename,
>> -                                   map_sysmem(CONFIG_SYS_LOAD_ADDR, 0), 0);
>> -               if (err <= 0)
>> -                       goto end;
>> -               err = spl_parse_image_header(spl_image, bootdev,
>> -                                            map_sysmem(CONFIG_SYS_LOAD_ADDR,
>> -                                                       err));
>> -               if (err == -EAGAIN)
>> -                       return err;
>> -               if (err == 0)
>> -                       err = 1;
>> -       } else if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
>> -           image_get_magic(header) == FDT_MAGIC) {
>> -               struct spl_load_info load;
>> -
>> -               debug("Found FIT\n");
>> -               load.read = spl_fit_read;
>> -               spl_set_bl_len(&load, ARCH_DMA_MINALIGN);
>> -               load.priv = (void *)filename;
>> -
>> -               return spl_load_simple_fit(spl_image, &load, 0, header);
>> -       } else {
>> -               err = spl_parse_image_header(spl_image, bootdev, header);
>> +       /*
>> +        * Avoid pulling in this function for other image types since we are
>> +        * very short on space on some boards.
>> +        */
>> +       if (IS_ENABLED(CONFIG_SPL_LOAD_FIT_FULL)) {
>> +               err = fat_size(filename, &size);
>>                  if (err)
>>                          goto end;
>> -
>> -               err = file_fat_read(filename, map_sysmem(spl_image->load_addr,
>> -                                                        spl_image->size), 0);
>> +       } else {
>> +               size = 0;
>>          }
>>
>> +       load.read = spl_fit_read;
>> +       spl_set_bl_len(&load, ARCH_DMA_MINALIGN);
>> +       load.priv = (void *)filename;
>> +       err = spl_load(spl_image, bootdev, &load, size, 0);
>> +
>>   end:
>>   #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
>> -       if (err <= 0)
>> +       if (err < 0)
>>                  printf("%s: error reading image %s, err - %d\n",
>>                         __func__, filename, err);
> 
> Do we still need this sort of #ifdef? I thought printf() would melt
> away without SPL_LIBCOMMON_SUPPORT? I haven't looked though. Also it
> isn't related to your patch.

 From what I can tell, printf implementation is orthogonal to LIBCOMMON_SUPPORT.

--Sean

>>   #endif
>>
>> -       return (err <= 0);
>> +       return err;
>>   }
>>
>>   #if CONFIG_IS_ENABLED(OS_BOOT)
>> diff --git a/include/spl_load.h b/include/spl_load.h
>> index 65aa6bb4493..5e0460d956d 100644
>> --- a/include/spl_load.h
>> +++ b/include/spl_load.h
>> @@ -96,6 +96,7 @@ static inline int _spl_load(struct spl_image_info *spl_image,
>>    */
>>   #define SPL_LOAD_USERS \
>>          IS_ENABLED(CONFIG_SPL_FS_EXT4) + \
>> +       IS_ENABLED(CONFIG_SPL_FS_FAT) + \
>>          0
>>
>>   #if SPL_LOAD_USERS > 1
>> diff --git a/test/image/spl_load_fs.c b/test/image/spl_load_fs.c
>> index 01559e98c4f..333df2dfb53 100644
>> --- a/test/image/spl_load_fs.c
>> +++ b/test/image/spl_load_fs.c
>> @@ -425,8 +425,7 @@ static int spl_test_mmc(struct unit_test_state *uts, const char *test_name,
>>          if (spl_test_mmc_fs(uts, test_name, type, create_ext2, false))
>>                  return CMD_RET_FAILURE;
>>
>> -       if (type != IMX8 && type != LEGACY_LZMA &&
>> -           spl_test_mmc_fs(uts, test_name, type, create_fat, false))
>> +       if (spl_test_mmc_fs(uts, test_name, type, create_fat, false))
>>                  return CMD_RET_FAILURE;
>>
>>          if (type == LEGACY_LZMA)
>> --
>> 2.37.1
>>
> 
> Regards,
> Simon