From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chee, Tien Fong Date: Thu, 14 Feb 2019 17:26:40 +0000 Subject: [U-Boot] [PATCH v8 6/8] spl : socfpga: Implement fpga bitstream loading with socfpga loadfs In-Reply-To: References: <1550067518-2542-1-git-send-email-tien.fong.chee@intel.com> <1550067518-2542-7-git-send-email-tien.fong.chee@intel.com> <135a82d0-020a-0c76-ff63-22763eb57ea5@denx.de> <1550127034.10728.39.camel@intel.com> <76206a1a-9ecc-dd28-b666-8c12c83a82c2@denx.de> <1550144321.10728.70.camel@intel.com> <1550157353.10728.83.camel@intel.com> Message-ID: <1550165200.10728.116.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 Thu, 2019-02-14 at 16:33 +0000, Westergreen, Dalon wrote: > On Thu, 2019-02-14 at 15:15 +0000, Chee, Tien Fong wrote: > > > > On Thu, 2019-02-14 at 13:28 +0100, Marek Vasut wrote: > > > > > > On 2/14/19 12:38 PM, Chee, Tien Fong wrote: > > > > > > > > On Thu, 2019-02-14 at 11:42 +0100, Marek Vasut wrote: > > > > > > > > > > On 2/14/19 7:50 AM, Chee, Tien Fong wrote: > > > > > > > > > > > > > > > > > > On Wed, 2019-02-13 at 17:25 +0100, Marek Vasut wrote: > > > > > > > > > > > > > > > > > > > > > On 2/13/19 3:18 PM, tien.fong.chee at intel.com wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > From: Tien Fong Chee > > > > > > > > > > > > > > > > Add support for loading FPGA bitstream to get DDR up > > > > > > > > running > > > > > > > > before > > > > > > > > U-Boot is loaded into DDR. Boot device initialization, > > > > > > > > generic > > > > > > > > firmware > > > > > > > > loader and SPL FAT support are required for this whole > > > > > > > > mechanism to > > > > > > > > work. > > > > > > > > > > > > > > > > Signed-off-by: Tien Fong Chee > > > > > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > > > changes for v7 > > > > > > > > - Removed casting for get_fpga_filename > > > > > > > > - Removed hard coding DDR address for loading core > > > > > > > > bistream, > > > > > > > > using > > > > > > > > loadable > > > > > > > >   property from FIT. > > > > > > > > - Added checking for config_pins, return if error. > > > > > > > > --- > > > > > > > >  arch/arm/mach-socfpga/spl_a10.c | 41 > > > > > > > > ++++++++++++++++++++++++++++++++++++++++- > > > > > > > >  1 file changed, 40 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > diff --git a/arch/arm/mach-socfpga/spl_a10.c > > > > > > > > b/arch/arm/mach- > > > > > > > > socfpga/spl_a10.c > > > > > > > > index c97eacb..36003b1 100644 > > > > > > > > --- a/arch/arm/mach-socfpga/spl_a10.c > > > > > > > > +++ b/arch/arm/mach-socfpga/spl_a10.c > > > > > > > > @@ -1,6 +1,6 @@ > > > > > > > >  // SPDX-License-Identifier: GPL-2.0+ > > > > > > > >  /* > > > > > > > > - *  Copyright (C) 2012 Altera Corporation > > > > > > > com> > > > > > > > > + *  Copyright (C) 2012-2019 Altera Corporation > > > > > > > tera > > > > > > > > .com > > > > > > > > > > > > > > > > > > > > > > > > > >   */ > > > > > > > >   > > > > > > > >  #include > > > > > > > > @@ -23,6 +23,8 @@ > > > > > > > >  #include > > > > > > > >  #include > > > > > > > >  #include > > > > > > > > +#include > > > > > > > > +#include > > > > > > > >   > > > > > > > >  DECLARE_GLOBAL_DATA_PTR; > > > > > > > >   > > > > > > > > @@ -68,11 +70,48 @@ u32 spl_boot_mode(const u32 > > > > > > > > boot_device) > > > > > > > >   > > > > > > > >  void spl_board_init(void) > > > > > > > >  { > > > > > > > > + char buf[16 * 1024] > > > > > > > > __aligned(ARCH_DMA_MINALIGN); > > > > > > > ALLOC_CACHE_ALIGN_BUFFER() > > > > > > #define ARCH_DMA_MINALIGN CONFIG_SYS_CACHELINE_SIZE > > > > > > They are not same thing? > > > > > See include/memalign.h and other drivers, the macro is > > > > > preferred > > > > > as > > > > > it > > > > > hides the details. > > > > Okay. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > >   /* enable console uart printing */ > > > > > > > >   preloader_console_init(); > > > > > > > >   WATCHDOG_RESET(); > > > > > > > >   > > > > > > > >   arch_early_init_r(); > > > > > > > > + > > > > > > > > + /* If the full FPGA is already loaded, ie.from > > > > > > > > EPCQ, > > > > > > > > config fpga pins */ > > > > > > > > + if (is_fpgamgr_user_mode()) { > > > > > > > > + int ret = config_pins(gd->fdt_blob, > > > > > > > > "shared"); > > > > > > > > + > > > > > > > > + if (ret) > > > > > > > > + return; > > > > > > > > + > > > > > > > > + ret = config_pins(gd->fdt_blob, > > > > > > > > "fpga"); > > > > > > > > + if (ret) > > > > > > > > + return; > > > > > > > > + } else if (!is_fpgamgr_early_user_mode()) { > > > > > > > > + /* Program IOSSM(early IO release) or > > > > > > > > full > > > > > > > > FPGA */ > > > > > > > > + fpga_fs_info fpga_fsinfo; > > > > > > > > + int len; > > > > > > > > + > > > > > > > > + fpga_fsinfo.filename = > > > > > > > > get_fpga_filename(gd- > > > > > > > > > > > > > > > > > > > > > > > > > > > fdt_blob, &len); > > > > > > > > + > > > > > > > > + if (fpga_fsinfo.filename) > > > > > > > > + socfpga_loadfs(&fpga_fsinfo, > > > > > > > > buf, > > > > > > > > sizeof(buf), 0); > > > > > > > Why is this code here twice ? The same code seems to be > > > > > > > below > > > > > > > ... > > > > > > The 1st calling for periph program, then running ddr > > > > > > calibration, > > > > > > then > > > > > > 2nd calling for core program. > > > > > Then maybe the code can be deduplicated ? > > > > Hmm...seems cannot, because > > > > 1. DDR calibration is not part of fpga code. > > > > 2. fpga driver can only be used to process one bistream at a > > > > time, > > > > because different mode has different handling. > > > + } else if (!is_fpgamgr_early_user_mode()) { > > > + /* Program IOSSM(early IO release) or full FPGA > > > */ > > > + fpga_fs_info fpga_fsinfo; > > > + int len; > > > + > > > + fpga_fsinfo.filename = get_fpga_filename(gd- > > > > > > > > fdt_blob, &len); > > > + > > > + if (fpga_fsinfo.filename) > > > + socfpga_loadfs(&fpga_fsinfo, buf, > > > sizeof(buf), 0); > > > ... > > > + if (!is_fpgamgr_user_mode()) { > > > + fpga_fs_info fpga_fsinfo; > > > + int len; > > > + > > > + fpga_fsinfo.filename = get_fpga_filename(gd- > > > > > > > > fdt_blob, &len); > > > + > > > + if (fpga_fsinfo.filename) > > > + socfpga_loadfs(&fpga_fsinfo, buf, > > > sizeof(buf), 0); > > > > > > These two chunks look the same to me , no ? > > Yes, they are being called twice at different fpga mode, and > > different > > sequence, before and after DDR calibration. > I believe i understand the issue here.  The code is a little more > convoluted then it should be.  The issue actually stems from  > first_loading_rbf_to_buffer which behaves differently depending on > the state of the fpga. > ... > +               uname = fit_get_name(buffer_p, images_noffset, NULL); > +               if (uname) { > +                       debug("FPGA: %s\n", uname); > + > +                       if (strstr(uname, "fpga-periph") && > +                               (!is_fpgamgr_early_user_mode() || > +                               is_fpgamgr_user_mode())) { > +                               fpga_node_name = uname; > +                               printf("FPGA: Start to program "); > +                               printf("peripheral/full bitstream > ...\n"); > +                               break; > +                       } else if (strstr(uname, "fpga-core") && > +                                       (is_fpgamgr_early_user_mode() > && > +                                       !is_fpgamgr_user_mode())) { > +                               fpga_node_name = uname; > +                               printf("FPGA: Start to program core > "); > +                               printf("bitstream ...\n"); > +                               break; > +                       } > +               } > ... > so when called with an unprogrammed fpga or a fully programmed fpga, > it will > program the fpga-periph.  When called with a programmed periph image > and > unprogrammed core is will program the the core. > > so socfpga_loadfs needs to be called twice to fully program the fpga. > > My question is, for a configuration where only the periph image is > programmed, > would this not result in an erroneous error message? > > +               debug("FPGA: No suitable bitstream was found, count: > %d.\n", i); > > I wounder if it would not be better to move the fpga image fit > parsing out > of the socfpga_loadfs function so > > 1) you could only call the fpga-core programming if needed > 2) socfpga_loadfs is explicitly called to program either the core > or the periph image > > alternately, perhaps a call to socfpga_loadfs should program both > images if present, and initialize the ddr if required? DDR calibration is not a FPGA stuff, so i think it should not be part of FPGA driver code. I believe a call to socfpga_loadfs would be more complicated for both images. What about for the case only periph program is required? > > I am not so keen on socfpga_loadfs behaving differently like this > with > the same function call. > > --dalon > > > > > _______________________________________________ > > U-Boot mailing list > > U-Boot at lists.denx.de > > https://lists.denx.de/listinfo/u-boot