On 08/30/2017 10:05 AM, Chee, Tien Fong wrote:
> On Sel, 2017-08-29 at 13:55 +0200, Marek Vasut wrote:
>> On 08/29/2017 12:45 PM, tien.fong.c...@intel.com wrote:
>>>
>>> From: Tien Fong Chee <tien.fong.c...@intel.com>
>>>
>>> This driver handles 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>
>>> ---
>>> .../include/mach/fpga_manager_arria10.h | 27 ++
>>> drivers/fpga/socfpga_arria10.c | 386
>>> +++++++++++++++++++-
>>> include/altera.h | 6 +
>>> include/configs/socfpga_common.h | 4 +
>>> 4 files changed, 422 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-
>>> socfpga/include/mach/fpga_manager_arria10.h b/arch/arm/mach-
>>> socfpga/include/mach/fpga_manager_arria10.h
>>> index 9cbf696..93a9122 100644
>>> --- a/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h
>>> +++ b/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h
>>> @@ -8,6 +8,8 @@
>>> #ifndef _FPGA_MANAGER_ARRIA10_H_
>>> #define _FPGA_MANAGER_ARRIA10_H_
>>>
>>> +#include <asm/cache.h>
>>> +
>>> #define ALT_FPGAMGR_IMGCFG_STAT_F2S_CRC_ERROR_SET_MSK
>>> BIT(0)
>>> #define ALT_FPGAMGR_IMGCFG_STAT_F2S_EARLY_USERMODE_SET_MSK
>>> BIT(1)
>>> #define ALT_FPGAMGR_IMGCFG_STAT_F2S_USERMODE_SET_MSK
>>> BIT(2)
>>> @@ -89,11 +91,36 @@ struct socfpga_fpga_manager {
>>> u32 imgcfg_fifo_status;
>>> };
>>>
>>> +#if defined(CONFIG_CMD_FPGA_LOADFS)
>>> +enum rbf_type {unknown, periph_section, core_section};
>>> +enum rbf_security {invalid, unencrypted, encrypted};
>>> +
>>> +struct rbf_info {
>>> + enum rbf_type section;
>>> + enum rbf_security security;
>>> +};
>>> +
>>> +struct flash_info {
>>> + char *interface;
>>> + char *dev_part;
>>> + char *filename;
>>> + int fstype;
>>> + u32 remaining;
>>> + u32 flash_offset;
>>> + struct rbf_info rbfinfo;
>>> + struct image_header header;
>>> +};
>>> +#endif
>>> +
>>> /* Functions */
>>> int fpgamgr_program_init(u32 * rbf_data, size_t rbf_size);
>>> int fpgamgr_program_finish(void);
>>> int is_fpgamgr_user_mode(void);
>>> int fpgamgr_wait_early_user_mode(void);
>>> +#if defined(CONFIG_CMD_FPGA_LOADFS)
>>> +const char *get_cff_filename(const void *fdt, int *len, u32 core);
>>> +const char *get_cff_devpart(const void *fdt, int *len);
>>> +#endif
>>>
>>> #endif /* __ASSEMBLY__ */
>>>
>>> diff --git a/drivers/fpga/socfpga_arria10.c
>>> b/drivers/fpga/socfpga_arria10.c
>>> index 5c1a68a..90c55e5 100644
>>> --- a/drivers/fpga/socfpga_arria10.c
>>> +++ b/drivers/fpga/socfpga_arria10.c
>>> @@ -13,6 +13,12 @@
>>> #include <altera.h>
>>> #include <common.h>
>>> #include <errno.h>
>>> +#include <fat.h>
>>> +#include <fs.h>
>>> +#include <fdtdec.h>
>>> +#include <malloc.h>
>>> +#include <part.h>
>>> +#include <spl.h>
>>> #include <wait_bit.h>
>>> #include <watchdog.h>
>>>
>>> @@ -22,6 +28,10 @@
>>> #define COMPRESSION_OFFSET 229
>>> #define FPGA_TIMEOUT_MSEC 1000 /* timeout in ms */
>>> #define FPGA_TIMEOUT_CNT 0x1000000
>>> +#define RBF_UNENCRYPTED 0xa65c
>>> +#define RBF_ENCRYPTED 0xa65d
>>> +#define ARRIA10RBF_PERIPH 0x0001
>>> +#define ARRIA10RBF_CORE 0x8001
>>>
>>> DECLARE_GLOBAL_DATA_PTR;
>>>
>>> @@ -118,7 +128,7 @@ static int
>>> wait_for_nconfig_pin_and_nstatus_pin(void)
>>> return wait_for_bit(__func__,
>>> &fpga_manager_base->imgcfg_stat,
>>> mask,
>>> - false, FPGA_TIMEOUT_MSEC, false);
>>> + true, FPGA_TIMEOUT_MSEC, false);
>>> }
>>>
>>> static int wait_for_f2s_nstatus_pin(unsigned long value)
>>> @@ -453,6 +463,281 @@ int fpgamgr_program_finish(void)
>>> return 0;
>>> }
>>>
>>> +#if defined(CONFIG_CMD_FPGA_LOADFS)
>>> +const char *get_cff_filename(const void *fdt, int *len, u32 core)
>>> +{
>>> + const char *cff_filename = NULL;
>>> + const char *cell;
>>> + int nodeoffset;
>>> + nodeoffset = fdt_subnode_offset(fdt, 0, "chosen");
>>> +
>>> + if (nodeoffset >= 0) {
>>> + if (core)
>>> + cell = fdt_getprop(fdt,
>>> + nodeoffset,
>>> + "cffcore-file",
>>> + len);
>>> + else
>>> + cell = fdt_getprop(fdt, nodeoffset, "cff-
>>> file", len);
>> This should be a property of the FPGA , not the system . You can have
>> multiple FPGAs and then this would become a problem.
>>
> This setting is for the only one FPGA inside our SoCFPGA.
You just said it yourself, it is for the only FPGA in your SOCFPGA ,
thus it is a property of the FPGA , not a chosen .
> For external
> multiple FPGAs programming, user is adviced to store the FPGA filename
> in environment variable and programming FPGA with fpga loadfs command.
>
> Please note that, peripheral rbf and partition are required in SPL to
> set up DDR before booting to U-boot.
>
>>>
>>> +
>>> + if (cell)
>>> + cff_filename = cell;
>>> + }
>>> +
>>> + return cff_filename;
>>> +}
>>> +
>>> +const char *get_cff_devpart(const void *fdt, int *len)
>>> +{
>>> + const char *cff_devpart = NULL;
>>> + const char *cell;
>>> + int nodeoffset;
>>> + nodeoffset = fdt_subnode_offset(fdt, 0, "chosen");
>>> +
>>> + cell = fdt_getprop(fdt, nodeoffset, "cff_devpart",
>>> len);
>> Indent ? What is this new undocumented DT node about ?
>>
> You can look the dtbinding doc on patch 12.
Ugh, the patch should be in the series before this one.
>>>
>>> + if (cell)
>>> + cff_devpart = cell;
>>> +
>>> + return cff_devpart;
>>> +}
>>> +
>>> +void get_rbf_image_info(struct rbf_info *rbf, u16 *buffer)
>>> +{
>>> + /*
>>> + Magic ID starting at:
>>> + -> 1st dword in periph.rbf
>>> + -> 2nd dword in core.rbf
>>> + */
>> Checkpatch should complain about incorrect multiline comment style
>> here ...
>>
> Okay. I will fix that.
Can you please run checkpatch before submitting your patches ?
>>>
>>> + u32 word_reading_max = 2;
>>> + u32 i;
>>> +
>>> + for(i = 0; i < word_reading_max; i++)
>>> + {
>>> + if(RBF_UNENCRYPTED == *(buffer + i)) /* PERIPH RBF
>>> */
>>> + rbf->security = unencrypted;
>>> + else if (RBF_ENCRYPTED == *(buffer + i))
>>> + rbf->security = encrypted;
>>> + else if (RBF_UNENCRYPTED == *(buffer + i + 1)) /*
>>> CORE RBF */
>>> + rbf->security =
>>> unencrypted;
>>> + else if (RBF_ENCRYPTED == *(buffer + i + 1))
>>> + rbf->security = encrypted;
>>> + else {
>>> + rbf->security = invalid;
>>> + continue;
>>> + }
>>> +
>>> + /* PERIPH RBF */
>>> + if (ARRIA10RBF_PERIPH == *(buffer + i + 1)) {
>>> + rbf->section = periph_section;
>>> + break;
>>> + }
>>> + else if (ARRIA10RBF_CORE == *(buffer + i + 1)) {
>>> + rbf->section = core_section;
>>> + break;
>>> + } /* CORE RBF */
>>> + else if (ARRIA10RBF_PERIPH == *(buffer + i + 2)) {
>>> + rbf->section = periph_section;
>>> + break;
>>> + }
>>> + else if (ARRIA10RBF_CORE == *(buffer + i + 2)) {
>>> + rbf->section = core_section;
>>> + break;
>>> + }
>>> + else {
>> } else { ... coding style ...
>>
> Okay, i will fix that. This apply to else if?
>>>
>>> + rbf->section = unknown;
>>> + break;
>>> + }
>>> + }
>>> +
>>> + return;
>>> +}
>>> +
>>> +static int flash_read(struct flash_info *flashinfo,
>>> + u32 size_read,
>>> + u32 *buffer_ptr)
>>> +{
>>> + size_t ret = EEXIST;
>>> + loff_t actread = 0;
>>> +
>>> +#ifdef CONFIG_FS_FAT
>>> + ret = fat_read_file(flashinfo->filename,
>>> + buffer_ptr, flashinfo-
>>>> flash_offset,
>>> + size_read, &actread);
>>> +#endif
>> How can a generic FPGA driver depend on random FS functionality ?
>> This is broken ...
>>
> random FS? There would having FAT FS for SDMMC, and UBI FS for QSPI and
> NAND(implement later).
Driver should not depend on specific FS.
> May be i can replace #ifdef CONFIG_FS_FAT witht
> the codes below, what do you think?
> bootdev.boot_device = spl_boot_device();
>
> if(BOOT_DEVICE_MMC1 == bootdev.boot_device)
> { ... }
Nor should it depend on random boot device.
>>>
>>> + 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;
>>> +}
>>> +
>>> +static int fs_flash_preinit(struct flash_info *flashinfo,
>>> + u32 *buffer, u32 *buffer_sizebytes)
>> Is this an FPGA driver or MTD driver ?
>>
> This is FPGA driver. Reading header of rbf, getting filesize and
> adjusting the beginning offset of RBF.
Why do you pass around the struct flashinfo then ?
[...]
>>> diff --git a/include/configs/socfpga_common.h
>>> b/include/configs/socfpga_common.h
>>> index 9be9e79..c15d244 100644
>>> --- a/include/configs/socfpga_common.h
>>> +++ b/include/configs/socfpga_common.h
>>> @@ -27,7 +27,11 @@
>>> */
>>> #define CONFIG_NR_DRAM_BANKS 1
>>> #define PHYS_SDRAM_1 0x0
>>> +#if defined(CONFIG_TARGET_SOCFPGA_GEN5)
>>> #define CONFIG_SYS_MALLOC_LEN (64 * 1024 * 1024)
>>> +#elif defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
>>> +#define CONFIG_SYS_MALLOC_LEN (128 * 1024 * 1024)
>>> +#endif
>> 128 MiB malloc area is nonsense, even those 64 MiB are iffy. Why
>> would
>> you ever need that in a bootloader ?
>>
> This is min require to malloc the buffer in SDRAM for core rbf. Less
> than this value, something would going wrong, i'm not recall what issue
> because i already tested this quite long time ago, problem related to
> FAT and malloc.
Isn't the loadfs supposed to read the data from the FS and program them
into the FPGA without having to read the whole bitstream into the RAM ?
>>>
>>> #define CONFIG_SYS_MEMTEST_START PHYS_SDRAM_1
>>> #define CONFIG_SYS_MEMTEST_END PHYS_SDRAM_1_SIZE
>>> #if defined(CONFIG_TARGET_SOCFPGA_GEN5)
>>>
--
Best regards,
Marek Vasut
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
