From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chee, Tien Fong Date: Thu, 28 Sep 2017 02:45:46 +0000 Subject: [U-Boot] [PATCH v2 09/19] arm: socfpga: Add drivers for programing FPGA from flash In-Reply-To: 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: <1506566738.3589.38.camel@intel.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de On Rab, 2017-09-27 at 10:30 +0200, Marek Vasut wrote: > 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 . > Okay. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --- > > > > > >  .../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_F2 > > > > > > S_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 ... > Okay. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >  #define CONFIG_SYS_MEMTEST_START PHYS_SDRAM_1 > > > > > >  #define CONFIG_SYS_MEMTEST_END PHYS_SDRAM_1 > > > > > > _SIZ > > > > > > E > > > > > >  #if defined(CONFIG_TARGET_SOCFPGA_GEN5) > > > > > > >