[U-Boot] [PATCH v2 09/19] arm: socfpga: Add drivers for programing FPGA from flash

[Marek Vasut]

On 09/28/2017 05:14 PM, Chee, Tien Fong wrote:
> On Rab, 2017-09-27 at 11:23 +0200, Marek Vasut wrote:
>> On 09/27/2017 11:13 AM, Chee, Tien Fong wrote:
>>>
>>> On Sel, 2017-09-26 at 12:39 +0200, Marek Vasut wrote:
>>>>
>>>> On 09/26/2017 11:52 AM, Chee, Tien Fong wrote:
>>>>>
>>>>>
>>>>> On Isn, 2017-09-25 at 11:14 +0200, Marek Vasut wrote:
>>>>>>
>>>>>>
>>>>>> On 09/25/2017 10:40 AM, tien.fong.chee at intel.com wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> From: Tien Fong Chee <tien.fong.chee at intel.com>
>>>>>>>
>>>>>>> These drivers handle FPGA program operation from flash
>>>>>>> loading
>>>>>>> RBF to memory and then to program FPGA.
>>>>>>>
>>>>>>> Signed-off-by: Tien Fong Chee <tien.fong.chee at intel.com>
>>>> [...]
>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> +const char *get_cff_devpart(const void *fdt, int *len)
>>>>>>> +{
>>>>>>> +	const char *cff_devpart = NULL;
>>>>>>> +	const char *cell;
>>>>>>> +	int nodeoffset;
>>>>>>> +	nodeoffset = fdtdec_next_compatible(fdt, 0,
>>>>>>> +			 COMPAT_ALTERA_SOCFPGA_FPGA0);
>>>>>>> +
>>>>>>> +	cell = fdt_getprop(fdt, nodeoffset,
>>>>>>> "bitstream_devpart",
>>>>>>> len);
>>>>>>> +
>>>>>>> +	if (cell)
>>>>>>> +		cff_devpart = cell;
>>>>>>> +
>>>>>>> +	return cff_devpart;
>>>>>>> +}
>>>>>> Take a look at splash*.c , I believe that can be reworked
>>>>>> into
>>>>>> generic
>>>>>> firmware loader , which you could then use here.
>>>>>>
>>>>> the devpart is hard coded in splash*.c. The function here is
>>>>> getting
>>>>> devpart info from DTS. So, is there any similar function in
>>>>> splash*.c?
>>>>> May be you can share more about your idea.
>>>> The generic loader could use some work of course ...
>>>>
>>> Sorry, i am still confusing. Allow me to ask you more:
>>> 1. Is the generic firmware loader already exists in splash*.c?
>> No
>>
>>>
>>> 2. Are you talking about get_cff_devpart or whole fpga laodfs?
>>> 3. You want me integrate get_cff_devpart function into splash*.c?
>>> 4. Are you means to hard code the devpart instead providing dynamic
>>> devpart described in DTS?
>> I am talking about factoring out generic firmware loader from
>> splash*c ,
>> since it already contains most of the parts for such a thing.
>>
>>>
>>> Current implementation are located in spl_board_init func
>>> (arcg/arm/mach-socfpga/spl.c). Based on boot device such as mmc,
>>> nand
>>> and QSPI, then reading some info from DTS, setting dev and
>>> partition
>>> with generic fs functions, and reading with generic fs function
>>> before
>>> programming RBF into FPGA. All these are in patch 19.
>> That's what splash*c also does, so adding separate parallel
>> implementation of the same functionality is a no-no.
>>
> After reading through splash*c, i found there are two functions bear a
> close similarity to.
> 1st function -->
> In /common/splash.c : 
> static struct splash_location default_splash_locations[] = {
> 	{
> 		.name = "sf",
> 		.storage = SPLASH_STORAGE_SF,
> 		.flags = SPLASH_STORAGE_RAW,
> 		.offset = 0x0,
> 	},
> 	{
> 		.name = "mmc_fs",
> 		.storage = SPLASH_STORAGE_MMC,
> 		.flags = SPLASH_STORAGE_FS,
> 		.devpart = "0:1",
> 	},
> 	{
> 		.name = "usb_fs",
> 		.storage = SPLASH_STORAGE_USB,
> 		.flags = SPLASH_STORAGE_FS,
> 		.devpart = "0:1",
> 	},
> 	{
> 		.name = "sata_fs",
> 		.storage = SPLASH_STORAGE_SATA,
> 		.flags = SPLASH_STORAGE_FS,
> 		.devpart = "0:1",
> 	},
> };
> 
> In my /arch/arm/mach-socfpga/spl.c (spl_board_init(void))
> bootdev.boot_device = spl_boot_device();
> 
> 	if (BOOT_DEVICE_MMC1 == bootdev.boot_device) {
> 		struct mmc *mmc = NULL;
> 		int err = 0;
> 
> 		spl_mmc_find_device(&mmc, bootdev.boot_device);
> 
> 		err = mmc_init(mmc);
> 
> 		if (err) {
> #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> 			printf("spl: mmc init failed with error: %d\n",
> err);
> #endif
> 		}
> 
> 		fpga_fsinfo.dev_part = (char *)get_cff_devpart(gd-
>> fdt_blob,
> 								 &len);
> 
> 		fpga_fsinfo.filename = (char *)get_cff_filename(gd-
>> fdt_blob,
> 								 &len,
> 								PERIPH_
> RBF);
> 
> 		fpga_fsinfo.interface = "mmc";
> 
> 		fpga_fsinfo.fstype = FS_TYPE_FAT;
> 	} else {
> 		printf("Invalid boot device!\n");
> 		return;
> 	}
> 
> 	/* Program peripheral RBF */
> 	if (fpga_fsinfo.filename && fpga_fsinfo.dev_part && (len > 0))
> 		rval = fpga_fsload(0, buffer, BSIZE, &fpga_fsinfo);
> 
> In /common/splash.c, dev_Part and flash type everything are hard coded
> in struct splash_location. In my spl.c, flash type are determined on
> run time and dev_part are retrived from DTS, and then assigned to
> struct fpga_fsinfo. Please note that this is in SPL, mmc need to be
> initialized 1st before loading raw file into memory. In SPL, raw file
> are coppied to OCRAM chunk by chunk, but In u-boot it would normally
> done in one big chunk to DRAM. This would be handled by fpga loadfs.
> 
> So, you want me hard code everthing like in splash.c?

No, I need you to play around with this and come up with generic
firmware loader that's flexible enough.

> 2nd function -->
> In /common/splash_source.c
> static int splash_select_fs_dev(struct splash_location *location)
> {
> 	int res;
> 
> 	switch (location->storage) {
> 	case SPLASH_STORAGE_MMC:
> 		res = fs_set_blk_dev("mmc", location->devpart,
> FS_TYPE_ANY);
> 		break;
> 	case SPLASH_STORAGE_USB:
> 		res = fs_set_blk_dev("usb", location->devpart,
> FS_TYPE_ANY);
> 		break;
> 	case SPLASH_STORAGE_SATA:
> 		res = fs_set_blk_dev("sata", location->devpart,
> FS_TYPE_ANY);
> 		break;
> 	case SPLASH_STORAGE_NAND:
> 		if (location->ubivol != NULL)
> 			res = fs_set_blk_dev("ubi", NULL,
> FS_TYPE_UBIFS);
> 		else
> 			res = -ENODEV;
> 		break;
> 	default:
> 		printf("Error: unsupported location storage.\n");
> 		return -ENODEV;
> 	}
> 
> 	if (res)
> 		printf("Error: could not access storage.\n");
> 
> 	return res;
> }
> 
> In my /drivers/fpga/socfpga_arria10.c
> static int flash_read(struct flash_info *flashinfo,
> 	u32 size_read,
> 	u32 *buffer_ptr)
> {
> 	size_t ret = EEXIST;
> 	loff_t actread = 0;
> 
> 	if (fs_set_blk_dev(flashinfo->interface, flashinfo->dev_part,
> 				flashinfo->fstype))
> 		return FPGA_FAIL;
> 
> 	ret = fs_read(flashinfo->filename,
> 			(u32) buffer_ptr, flashinfo->flash_offset,
> 			size_read, &actread);
> 
> 	if (ret || actread != size_read) {
> 		printf("Failed to read %s from flash %d ",
> 			flashinfo->filename,
> 			 ret);
> 		printf("!= %d.\n", size_read);
> 		return -EPERM;
> 		} else
> 			ret = actread;
> 
> 	return ret;
> }
> 
> Some attributes like flash type is determined on run time and and
> dev_part is retrieved from DTS, so every infos driver need to know are
> assinged into struct flashinfo and passed to fs_set_blk_dev as
> arguments. I found that function in splash_source.c some like flash
> type are getting from env variable, but we are still in SPL phase,
> those env variable is not set up yet. So, i think that is very
> ineffcient to factor out them as common.
> 
> If you want me create a generic firmware loader which is generic enough
> loading content for all components like flashes, FPGA, splash ....etc,
> i don't think that is effient enough, as fpga loadfs has different
> handling in both SPL and U-boot like copying raw into memory.

Duplicating code is nonsense and I believe generic firmware loader would
be the way to go here.

> It would be good you can direct point me out which functions have
> similirity and how to factor them out as common.
> 
> Thanks a lot.
>>>
>>> Thanks.
>>>>
>>>> [...]


-- 
Best regards,
Marek Vasut