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

[Chee, Tien Fong <tien.fong.chee@intel.com>]

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@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@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?

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.

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.
> > > 
> > > [...]
>