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.c...@intel.com wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> From: Tien Fong Chee <tien.fong.c...@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.c...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
