[U-Boot] [RFC PATCH] SPL: FIT: Enable SPL_FIT_LOAD for sd bootmode for fat partions

[Lokesh Vutla <lokeshvutla@ti.com>]

On Monday 02 May 2016 01:27 PM, Michal Simek wrote:
> On 2.5.2016 06:06, Lokesh Vutla wrote:
>> Hi Michal,
>>
>> On Thursday 28 April 2016 03:01 PM, Michal Simek wrote:
>>> Support U-Boot SPL to load FIT image from fat partition.
>>> Fit image can be setup via CONFIG_SPL_FS_LOAD_KERNEL_NAME.
>>> Falcon mode is not supported.
>>>
>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>> ---
>>>
>>>  common/spl/spl_fat.c | 50 ++++++++++++++++++++++++++++++++++++++++++++------
>>>  fs/fat/fat.c         |  4 ++--
>>>  2 files changed, 46 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/common/spl/spl_fat.c b/common/spl/spl_fat.c
>>> index d16cd540e38a..4e319c5fa470 100644
>>> --- a/common/spl/spl_fat.c
>>> +++ b/common/spl/spl_fat.c
>>> @@ -13,6 +13,7 @@
>>>  #include <spl.h>
>>>  #include <asm/u-boot.h>
>>>  #include <fat.h>
>>> +#include <libfdt.h>
>>>  #include <errno.h>
>>>  #include <image.h>
>>>  
>>> @@ -39,6 +40,29 @@ static int spl_register_fat_device(struct blk_desc *block_dev, int partition)
>>>  	return err;
>>>  }
>>>  
>>> +#ifdef CONFIG_SPL_LOAD_FIT
>>> +static ulong spl_fat_file_read(struct spl_load_info *load, ulong sector,
>>> +			       ulong count, void *buf)
>>> +{
>>> +	int err;
>>> +	loff_t actread;
>>> +	char *filename = (char *)load->priv;
>>> +
>>> +	debug("%s: name %s, sector %lx, count %lx, buf %lx\n",
>>> +	      __func__, filename,  sector, count, (ulong)buf);
>>> +
>>> +	err = file_fat_read_at(filename, sector, buf, count, &actread);
>>> +	if (err < 0) {
>>> +		printf("%s: error reading image %s, err - %d\n",
>>> +		       __func__, filename, err);
>>> +		return err;
>>> +	}
>>> +
>>> +	debug("actread %lx\n", (ulong)actread);
>>> +	return actread;
>>> +}
>>> +#endif
>>> +
>>>  int spl_load_image_fat(struct blk_desc *block_dev,
>>>  						int partition,
>>>  						const char *filename)
>>> @@ -57,16 +81,29 @@ int spl_load_image_fat(struct blk_desc *block_dev,
>>>  	if (err <= 0)
>>>  		goto end;
>>>  
>>> -	spl_parse_image_header(header);
>>> +	if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
>>> +	    image_get_magic(header) == FDT_MAGIC) {
>>> +		struct spl_load_info load;
>>> +
>>> +		debug("Found FIT\n");
>>> +		load.priv = (char *)filename;
>>> +		load.bl_len = 1;
>>> +		load.read = spl_fat_file_read;
>>> +		spl_load_simple_fit(&load, 0, header);
>>> +	} else {
>>> +		debug("Legacy image\n");
>>>  
>>> -	err = file_fat_read(filename, (u8 *)(uintptr_t)spl_image.load_addr, 0);
>>> +		spl_parse_image_header(header);
>>>  
>>> +		err = file_fat_read(filename,
>>> +				    (u8 *)(uintptr_t)spl_image.load_addr, 0);
>>>  end:
>>>  #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
>>> -	if (err <= 0)
>>> -		printf("%s: error reading image %s, err - %d\n",
>>> -		       __func__, filename, err);
>>> +		if (err <= 0)
>>> +			printf("%s: error reading image %s, err - %d\n",
>>> +			       __func__, filename, err);
>>>  #endif
>>> +	}
>>>  
>>>  	return (err <= 0);
>>>  }
>>> @@ -81,6 +118,7 @@ int spl_load_image_fat_os(struct blk_desc *block_dev, int partition)
>>>  	if (err)
>>>  		return err;
>>>  
>>> +#if !defined(CONFIG_SPL_LOAD_FIT)
>>>  #if defined(CONFIG_SPL_ENV_SUPPORT) && defined(CONFIG_SPL_OS_BOOT)
>>>  	file = getenv("falcon_args_file");
>>>  	if (file) {
>>> @@ -116,7 +154,7 @@ defaults:
>>>  #endif
>>>  		return -1;
>>>  	}
>>> -
>>> +#endif
>>>  	return spl_load_image_fat(block_dev, partition,
>>>  			CONFIG_SPL_FS_LOAD_KERNEL_NAME);
>>>  }
>>> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
>>> index 600a90e30922..0d987e0465ee 100644
>>> --- a/fs/fat/fat.c
>>> +++ b/fs/fat/fat.c
>>> @@ -281,9 +281,9 @@ get_cluster(fsdata *mydata, __u32 clustnum, __u8 *buffer, unsigned long size)
>>>  
>>>  	if ((unsigned long)buffer & (ARCH_DMA_MINALIGN - 1)) {
>>>  		ALLOC_CACHE_ALIGN_BUFFER(__u8, tmpbuf, mydata->sect_size);
>>> -
>>> +#if !defined(CONFIG_SPL_LOAD_FIT)
>>>  		printf("FAT: Misaligned buffer address (%p)\n", buffer);
>>> -
>>> +#endif
>>
>> IMO, this is a hack. Why should fs worry about if it as fit image or
>> not. Also the read performance will be very slow if you do not pass the
>> aligned buffer address.  I had a different approach[1] for this: first
>> copy the image to aligned buffer and then do a memcpy to the proper
>> destination(which showed a better performance). May be this is wrong.
> 
> I agree that's why this was RFC not regular patch.
> I have looked at your solution and also Simon's comments and truth is
> that your patch has a lot of duplicated stuff.
> This solution is smaller.
> Regarding buffer alignment. I think this can be simply added to read
> function to keep it in the right place.

We don't want to take care of the alignment in read function of each fs
type. IMHO, this should be handled by fit framework itself. I have
updated my DMA alignment patch to take care of FS read as well[1]. Can
you see if that helps?

[1] https://www.mail-archive.com/u-boot at lists.denx.de/msg211628.html

Thanks and regards,
Lokesh

> 
> Thanks,
> Michal
> 
>