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

[Michal Simek <michal.simek@xilinx.com>]

On 3.5.2016 11:10, Lokesh Vutla wrote:
> 
> 
> 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

Look at my responses in v4. Your implementation is not working for me.

Thanks,
Michal