From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Wed, 27 Sep 2017 10:30:34 +0200 Subject: [U-Boot] [PATCH v2 09/19] arm: socfpga: Add drivers for programing FPGA from flash In-Reply-To: <1506492302.3589.26.camel@intel.com> References: <1506328815-23733-1-git-send-email-tien.fong.chee@intel.com> <1506328815-23733-10-git-send-email-tien.fong.chee@intel.com> <995ab037-992e-5782-4faf-8965d039c841@denx.de> <1506414635.27760.26.camel@intel.com> <1506492302.3589.26.camel@intel.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de On 09/27/2017 08:05 AM, Chee, Tien Fong wrote: > On Sel, 2017-09-26 at 12:32 +0200, Marek Vasut wrote: >> On 09/26/2017 10:30 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 >>>>> >>>>> These drivers handle FPGA program operation from flash loading >>>>> RBF to memory and then to program FPGA. >>>>> >>>>> Signed-off-by: Tien Fong Chee >>>> Did you run checkpatch on this before submitting ? I presume no >>>> ... >>>> >>> Yeah, i run checkpatch for all patches. What's the issue here? >> It should definitely indicate problem with ie. yoda-notation >> +if (0 == flashinfo->remaining) { >> and indent ... >> > No complaint from checkpath. I know someone saying bad for readbility, > but yoda-notation at this simple implementation doesn't impact the > readbility, and having benefit to leverage detection of compiler on > missing "=". Overall, this can help to improve coding quality. I can > remove it if this doesn't favored in U-boot. It is not welcome and modern gcc warns you about such things . >>> >>>> >>>>> >>>>> >>>>> --- >>>>>  .../include/mach/fpga_manager_arria10.h            |  27 ++ >>>>>  drivers/fpga/socfpga_arria10.c                     | 391 >>>>> ++++++++++++++++++++- >>>>>  include/altera.h                                   |   6 + >>>>>  include/configs/socfpga_common.h                   |   4 + >>>>>  4 files changed, 425 insertions(+), 3 deletions(-) >>>> [...] >>>> >>>>> >>>>> >>>>> @@ -112,13 +122,14 @@ static int >>>>> wait_for_nconfig_pin_and_nstatus_pin(void) >>>>>   unsigned long mask = >>>>> ALT_FPGAMGR_IMGCFG_STAT_F2S_NCONFIG_PIN_SET_MSK | >>>>>   ALT_FPGAMGR_IMGCFG_STAT_F2S_NS >>>>> TATU >>>>> S_PIN_SET_MSK; >>>>>   >>>>> - /* Poll until f2s_nconfig_pin and f2s_nstatus_pin; >>>>> loop >>>>> until de-asserted, >>>>> -  * timeout at 1000ms >>>>> + /* >>>>> +  * Poll until f2s_nconfig_pin and f2s_nstatus_pin; >>>>> loop >>>>> until >>>>> +  * de-asserted, timeout at 1000ms >>>>>    */ >>>>>   return wait_for_bit(__func__, >>>>>       &fpga_manager_base->imgcfg_stat, >>>>>       mask, >>>>> -     false, FPGA_TIMEOUT_MSEC, false); >>>>> +     true, FPGA_TIMEOUT_MSEC, false); >>>>>  } >>>> Seems more like a fix, split this out. >>>> >>> Okay. >>>> >>>>> >>>>> >>>>>  static int wait_for_f2s_nstatus_pin(unsigned long value) >>>>> @@ -469,6 +480,7 @@ int socfpga_load(Altera_desc *desc, const >>>>> void >>>>> *rbf_data, size_t rbf_size) >>>>>   >>>>>   /* Initialize the FPGA Manager */ >>>>>   status = fpgamgr_program_init((u32 *)rbf_data, >>>>> rbf_size); >>>>> + >>>>>   if (status) >>>>>   return status; >>>>>   >>>>> @@ -477,3 +489,376 @@ int socfpga_load(Altera_desc *desc, const >>>>> void *rbf_data, size_t rbf_size) >>>>>   >>>>>   return fpgamgr_program_finish(); >>>>>  } >>>>> + >>>>> +#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 = fdtdec_next_compatible(fdt, 0, >>>>> +  COMPAT_ALTERA_SOCFPGA_FPGA0); >>>>> + >>>>> + if (nodeoffset >= 0) { >>>>> + if (core) >>>>> + cell = fdt_getprop(fdt, >>>>> + nodeoffset, >>>>> + "bitstream_core", >>>>> + len); >>>>> + else >>>>> + cell = fdt_getprop(fdt, nodeoffset, >>>>> "bitstream_periph", >>>>> +  len); >>>>> + >>>>> + 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 = 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. >> This is important here, I don't want yet another ad-hoc loader ... >> > Disucss in another email reply separately. I need time to think. >>> >>>> >>>> [...] >>>> >>>>> >>>>> >>>>> diff --git a/include/configs/socfpga_common.h >>>>> b/include/configs/socfpga_common.h >>>>> index 9897e11..eadce2d 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 >>>> You definitely don't need 128 MiB of malloc area. >>>> >>> Okay, i will try out with smaller size. >> Why do you need such massive area ? It's not a matter of "try out", >> you >> should know why this change was needed for your use-case. >> > I forgot what reason i put this value. I need to find out. If this was properly documented in the commit message, you would not have to ... >>> >>>> >>>>> >>>>> >>>>>  #define CONFIG_SYS_MEMTEST_START PHYS_SDRAM_1 >>>>>  #define CONFIG_SYS_MEMTEST_END PHYS_SDRAM_1_SIZ >>>>> E >>>>>  #if defined(CONFIG_TARGET_SOCFPGA_GEN5) >>>>> -- Best regards, Marek Vasut