[U-Boot] [PATCH v8 6/8] spl : socfpga: Implement fpga bitstream loading with socfpga loadfs
Westergreen, Dalon
dalon.westergreen at intel.com
Thu Feb 14 17:26:15 UTC 2019
On Thu, 2019-02-14 at 16:59 +0000, Chee, Tien Fong wrote:
> 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 <tien.fong.chee at intel.com>
> > > > > > > > >
> > > > > > > > > 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 <tien.fong.chee at intel.com
> > > > > > > > >
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > 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 <www.altera.
> > > > > > > > > com>
> > > > > > > > > + * Copyright (C) 2012-2019 Altera Corporation <www.al
> > > > > > > > > tera
> > > > > > > > > .com
> > > > > > > > > >
> > > > > > > > > */
> > > > > > > > >
> > > > > > > > > #include <common.h>
> > > > > > > > > @@ -23,6 +23,8 @@
> > > > > > > > > #include <fdtdec.h>
> > > > > > > > > #include <watchdog.h>
> > > > > > > > > #include <asm/arch/pinmux.h>
> > > > > > > > > +#include <asm/arch/fpga_manager.h>
> > > > > > > > > +#include <mmc.h>
> > > > > > > > >
> > > > > > > > > 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);
> This is not error, value 1 would be returned instead -ve and 0.
> socfpga_loadfs would skip the core configuration when 1 is detected.
> You would see the message print out "FPGA: Skipping configuration ..."
> after "FPGA: Checking FPGA configuration setting ..."
> > 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
> I don't know would this cause more messy, parsing fpga image fit job
> supposely done inside fpga driver.
i agree, but perhaps the ddr calibration call should be in socfpga_loadfs?
and perhaps a single call to socfpga_loadfs should program both the periph
and core images if present?
> > alternately, perhaps a call to socfpga_loadfs should program both
> > images if present, and initialize the ddr if 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
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3282 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190214/dd0f3ef7/attachment.bin>
More information about the U-Boot
mailing list