[U-Boot] [PATCH v7 1/7] ARM: socfpga: Description on FPGA bitstream type and file name for Arria 10

[Chee, Tien Fong <tien.fong.chee@intel.com>]

On Mon, 2019-02-11 at 17:19 +0000, Westergreen, Dalon wrote:
> On Tue, 2019-02-12 at 01:01 +0800, Chee, Tien Fong wrote:
> > 
> > On Mon, 2019-02-11 at 12:01 +0100, Marek Vasut wrote:
> > > 
> > > On 2/11/19 6:36 AM, Chee, Tien Fong wrote:
> > > > 
> > > > On Tue, 2019-02-05 at 09:46 +0100, Marek Vasut wrote:
> > > > > 
> > > > > On 2/1/19 5:02 PM, Chee, Tien Fong wrote:
> > > > > > 
> > > > > > 
> > > > > > On Fri, 2019-02-01 at 09:25 +0100, Marek Vasut wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > On 2/1/19 4:48 AM, Chee, Tien Fong wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On Thu, 2019-01-31 at 15:54 +0100, Marek Vasut wrote:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > On 1/31/19 3:51 PM, tien.fong.chee at intel.com wrote:
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > > > > > > > > > 
> > > > > > > > > > This patch adds description on properties about
> > > > > > > > > > file
> > > > > > > > > > name
> > > > > > > > > > used
> > > > > > > > > > for
> > > > > > > > > > both
> > > > > > > > > > peripheral bitstream and core bitstream.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel
> > > > > > > > > > .com
> > > > > > > > > > 
> > > > > > > > > > ---
> > > > > > > > > > 
> > > > > > > > > > changes for v7
> > > > > > > > > > - Provided example of setting FPGA FIT image for
> > > > > > > > > > both
> > > > > > > > > > early
> > > > > > > > > > IO
> > > > > > > > > > release
> > > > > > > > > >   and full release FPGA configuration.
> > > > > > > > > > ---
> > > > > > > > > >  .../fpga/altera-socfpga-a10-fpga-
> > > > > > > > > > mgr.txt           |
> > > > > > > > > > 34
> > > > > > > > > > +++++++++++++++++++++-
> > > > > > > > > >  1 file changed, 33 insertions(+), 1 deletion(-)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/doc/device-tree-bindings/fpga/altera-
> > > > > > > > > > socfpga-
> > > > > > > > > > a10-
> > > > > > > > > > fpga-
> > > > > > > > > > mgr.txt b/doc/device-tree-bindings/fpga/altera-
> > > > > > > > > > socfpga-
> > > > > > > > > > a10-
> > > > > > > > > > fpga-
> > > > > > > > > > mgr.txt
> > > > > > > > > > index 2fd8e7a..5f81a32 100644
> > > > > > > > > > --- a/doc/device-tree-bindings/fpga/altera-socfpga-
> > > > > > > > > > a10-
> > > > > > > > > > fpga-
> > > > > > > > > > mgr.txt
> > > > > > > > > > +++ b/doc/device-tree-bindings/fpga/altera-socfpga-
> > > > > > > > > > a10-
> > > > > > > > > > fpga-
> > > > > > > > > > mgr.txt
> > > > > > > > > > @@ -7,8 +7,39 @@ Required properties:
> > > > > > > > > >                 - The second index is for writing
> > > > > > > > > > FPGA
> > > > > > > > > > configuration data.
> > > > > > > > > >  - resets     : Phandle and reset specifier for the
> > > > > > > > > > device's
> > > > > > > > > > reset.
> > > > > > > > > >  - clocks     : Clocks used by the device.
> > > > > > > > > > +- altr,bitstream : File name for FPGA peripheral
> > > > > > > > > > bitstream
> > > > > > > > > > which
> > > > > > > > > > is used
> > > > > > > > > > +		   to initialize FPGA IOs, PLL,
> > > > > > > > > > IO48
> > > > > > > > > > and
> > > > > > > > > > DDR.
> > > > > > > > > > This
> > > > > > > > > > bitstream is
> > > > > > > > > > +		   required to get DDR up running.
> > > > > > > > > > +		   or
> > > > > > > > > > +		   File name for full bitstream,
> > > > > > > > > > consist
> > > > > > > > > > of
> > > > > > > > > > peripheral bitstream
> > > > > > > > > > +		   and core bitstream.
> > > > > > > > > > +- altr,bitstream-core(optional) : File name for
> > > > > > > > > > core
> > > > > > > > > > bitstream
> > > > > > > > > > which contains
> > > > > > > > > Is the name of the property 'altr,bitstream-
> > > > > > > > > core(optional)' ?
> > > > > > > > > I
> > > > > > > > > think
> > > > > > > > > the "optional" part should be in the description.
> > > > > > > > Yes, you are right.
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > +				  FPGA design
> > > > > > > > > > which is
> > > > > > > > > > used to
> > > > > > > > > > program FPGA CRAM
> > > > > > > > > > +				  and ERAM.
> > > > > > > > > >  
> > > > > > > > > > -Example:
> > > > > > > > > > +Example: Bundles both peripheral bitstream and
> > > > > > > > > > core
> > > > > > > > > > bitstream
> > > > > > > > > > into
> > > > > > > > > > FIT image
> > > > > > > > > > +	 called fit_spl_fpga.itb. This FIT image
> > > > > > > > > > can
> > > > > > > > > > be
> > > > > > > > > > created
> > > > > > > > > > through running
> > > > > > > > > > +	 this command: tools/mkimage
> > > > > > > > > > +		       -E -p 400
> > > > > > > > > > +		       -f board/altera/arria10-
> > > > > > > > > > socdk/fit_spl_fpga.its
> > > > > > > > > > +		       fit_spl_fpga.itb
> > > > > > > > > > +
> > > > > > > > > > +	 For details of describing structure and
> > > > > > > > > > contents
> > > > > > > > > > of
> > > > > > > > > > the
> > > > > > > > > > FIT image,
> > > > > > > > > > +	 please refer board/altera/arria10-
> > > > > > > > > > socdk/fit_spl_fpga.its
> > > > > > > > > > +
> > > > > > > > > > +- Examples for booting with early IO release, and
> > > > > > > > > > enter
> > > > > > > > > > early
> > > > > > > > > > user
> > > > > > > > > > mode:
> > > > > > > > > > +
> > > > > > > > > > +	fpga_mgr: fpga-mgr at ffd03000 {
> > > > > > > > > > +		compatible = "altr,socfpga-a10-
> > > > > > > > > > fpga-
> > > > > > > > > > mgr";
> > > > > > > > > > +		reg = <0xffd03000 0x100
> > > > > > > > > > +		       0xffcfe400 0x20>;
> > > > > > > > > > +		clocks = <&l4_mp_clk>;
> > > > > > > > > > +		resets = <&rst FPGAMGR_RESET>;
> > > > > > > > > > +		altr,bitstream =
> > > > > > > > > > "fit_spl_fpga.itb";
> > > > > > > > > > +		altr,bitstream-core =
> > > > > > > > > > "fit_spl_fpga.itb";
> > > > > > > > > It's the same file, why does it use two properties ? 
> > > > > > > > 1. Allows user to run optional for program core. When
> > > > > > > > "" is
> > > > > > > > set
> > > > > > > > to 
> > > > > > > > altr,bitstream-core, then SPL would skip programming
> > > > > > > > FPGA
> > > > > > > > with
> > > > > > > > core, so
> > > > > > > > user can program it later on U-Boot or Linux.
> > > > > > > You can just pass in a fitImage with only the periph
> > > > > > > image in
> > > > > > > it
> > > > > > > in
> > > > > > > such
> > > > > > > a case.
> > > > > > What if user want to program the core on U-Boot? User need
> > > > > > to
> > > > > > create
> > > > > > two FIT images, one FIT with periph image, and another FIT
> > > > > > with
> > > > > > core
> > > > > > image only.
> > > > > > 
> > > > > > Current implementation supports one FIT image for above
> > > > > > configuration.
> > > > > What if user want to program the core on U-Boot in this
> > > > > implementation?
> > > > > It is not possible either, is it ?
> > > > There are few ways user can do:
> > > > 1. Running commands such as imxtract/fatload with socfpga load
> > > > 2. Script
> > > > 3. Env
> > > And how is that different from using a single fitImage with
> > > configuration node describing the parts of the bitstream that
> > > should
> > > be
> > > loaded ?
> > Please correct me if my answer doesn't match your question.
> > 
> > In previous implementation, fpga-1 must be periph.rbf, and fpga-2
> > must
> > always be core.rbf. If altr,bitstream-core is not defined, then SPL
> > would skip the core configuration. If user want to program core on
> > U-
> > Boot, then user need to extract the fpga-2 and program it.
> > 
> > Current proposal, altr,bitstream-core would be removed, and this
> > would
> > replaced by the rule of describing sequence of the fpga strings set
> > in
> > the fpga property in fitImage with configuration.
> > 
> > For property "fpga" = 1st string is for periph.rbf or full rbf.> > 
> > 	
> > 	    = 2nd string is for core.rbf. This is optional,
> > no> > string is found, the core configuration would be skipped.>
> > >    
> >                 =  3rd string onwards would be ignored.> > 
> > 
> > For example, if user want to program periph.rbf or full rbf, user
> > can
> > describe the fpga sequence as below
> > configurations {
> > 	default = "config-1";
> > 		config-1 {
> > 			fpga = "fpga-1"; <=perih.rbf or full rbf
> > 		};
> > 
> > };
> > 
> > if user want to program periph.rbf and core.rbf in SPL
> > configurations {
> > 	default = "config-1";
> > 		config-1 {
> > 			
> > fpga = "fpga-1", "fpga-2";
> > 		};
> > 
> > };
> > 
> > if user want to program periph.rbf in spl, but core.rbf in U-Boot
> > configurations {
> > 	default = "config-1";
> > 		config-1 {
> > 			fpga = "fpga-1"; <=perih.rbf
> > 		};
> > 		config-2 {		
> > 			fpga = "fpga-1", "fpga-2";
> > 		};
> > 
> > };
> > 
> > So for core.rbf, user need to look the config-2, then extract the
> > fpga-
> > 2 from few ways described above.
> Or, store the core rbf outside of the fit image, no?  
> 
> my preference for the fit image would be
> 
> ...
> images {
>   fpga at 1 {
> 	description = "FPGA Periph";
> 	...
> 	type = "fpga_periph";
> 	...
>   }
>   fpga at 2 {
> 	description = "FPGA Core";
> 	...
> 	type = "fpga" or
> "fpga_core";
I'm good with "fpga".
> 	...
>   }
> };
> configurations {
>   default = "config at 1"
>   config at 1 {
>       fpga = "fpga at 1";  // periph only
>   };
>   config at 2 {
>       fpga = "fpga at 1", "fpga at 2";
>   };
> };
> 
> with the expectation that the order of fpga at 1 and fpga at 2 in config at 2
> is not relevant.  the code should find the fpga_periph type and
> program
> it first.  just my comment, i dont like rellying on the order or
> name.
I can add support for above implementation although this adds more
complexity to the driver.

Marek, are you OK with this implementation?
> 
> --dalon
> 
> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > > 
> > > > > > > > 2. Allows core in different FIT file.
> > > > > > > Is this really useful ?
> > > > > > Yes, for the use case which support different core image
> > > > > > for
> > > > > > different
> > > > > > revision of board but using same periph image.
> > > > > Seems you an Dalon are discussing this and it's not a
> > > > > supported
> > > > > flow
> > > > > ?
> > > > Dalon has some concerns.
> > > > 
> > > > But, we can keep the more flexibility for users by adding
> > > > support
> > > > of
> > > > configuration in fit image, then removing the altr,bitstream-
> > > > core
> > > > properties in FPGA manager node. So, users are freely to use
> > > > this
> > > > implementation in their own use cases.
> > > > 
> > > > So, user need to add the default configuration for FPGA. 
> > > > For property "fpga" = 1st string is for periph.rbf or full rbf.
> > > > 		    = 2nd string is for core.rbf. This is
> > > > optional, no
> > > > string is found, the core configuration would be skipped.
> > > >                     =  3rd string onwards would be ignored.
> > > > 
> > > > configurations {
> > > > 	default = "config-1";
> > > > 	config-1 {
> > > > 		description = "Boot with FPGA early IO release
> > > > config";
> > > > 		fpga = "fpga-1", "fpga-2";   <= sequence for
> > > > bitstream
> > > > configuration
> > > > 		};
> > > > 	};
> > > I want to be able to do something like
> > > configuration {
> > > 	...
> > > 	config-board1 {
> > > 		...
> > > 		fpga = "fpga-core1";
> > > 	};
> > > 	config-board2 {
> > > 		...
> > > 		fpga = "fpga-core2", "fpga-periph2";
> > > 	};
> > > };
> > > 
> > > Does this make sense ?
> > 1. How user can tell the SPL which config if there is no default
> > property described?
> > 
> > 2. You want the code to recognize the string "fpga-core" for
> > core.rbf
> > and "fpga-periph" for periph.rbf?
> > 
> > 3. What's your concern with the 1st and 2nd string represent
> > periph.rbf
> > and core.rbf respectively?
> > > 
> > > Dalon, is this mix-and-match approach to bitstreams something we
> > > want
> > > to
> > > support at all ? It seems a bit dangerous to me.
> > > 
> > > > 
> > > > Anyway, please bear in mind that multi fpga nodes in one fpga
> > > > fit
> > > > image
> > > > is not good for performance. I have tried few ideas to fix the
> > > > performance penalty caused by buffer alignment at reading
> > > > cluster,
> > > > but
> > > > it didn't work.
> > > > 
> > > > What do you think? 
> > > I think if there is a performance penalty when loading file from
> > > VFAT,
> > > it is VFAT code or SD/MMC driver that needs fixing. We definitely
> > > can
> > > not hack it up by realigning fitImage to magically work around
> > > that.
> > Please see my replied in patch 2/7
> > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > And where is this
> > > > > > > > > file loaded from ?
> > > > > > > > You need to set the default source in DTS, for example
> > > > > > > > "firmware-
> > > > > > > > loader 
> > > > > > > > = &fs_loader0", that's for power boot up purpose. After
> > > > > > > > that,
> > > > > > > > generic
> > > > > > > > firmware loader would go to the dsignated storage as
> > > > > > > > described
> > > > > > > > below to
> > > > > > > > find the FPGA FIT image according description from
> > > > > > > > above.
> > > > > > > > 
> > > > > > > > 	fs_loader0: fs-loader at 0 {
> > > > > > > > 		u-boot,dm-pre-reloc;
> > > > > > > > 		compatible = "u-boot,fs-loader";
> > > > > > > > 		phandlepart = <&mmc 1>;
> > > > > > > > 	};
> > > > > > > How does the driver bound to fpga-mgr know which firmware
> > > > > > > loader
> > > > > > > instance to use ? There's no phandle.
> > > > > > Current firmware loader supports only one instance, that is
> > > > > > default
> > > > > > instance described in chosen node. It is good enough to
> > > > > > solve
> > > > > > our
> > > > > > problem where to define a default storage for FPGA images
> > > > > > and
> > > > > > how
> > > > > > to
> > > > > > tell the SPL to load it from the default storage when the
> > > > > > board
> > > > > > is
> > > > > > powered up. I don't see there is a need to support more
> > > > > > than
> > > > > > one
> > > > > > instance for fpga-mgr during SPL runtime, at least for now.
> > > > > > User
> > > > > > can
> > > > > > program the FPGA with core image from any storage with
> > > > > > series
> > > > > > of
> > > > > > commands such as fatload and socfpga load on U-Boot
> > > > > > console.
> > > > > Since you already have the label for the fs-loader _and_ you
> > > > > have
> > > > > the
> > > > > address for it (fs-loader at 0), you should supply the phandle.
> > > > > And
> > > > > eventually, someone will try to use multiple loaders, so we
> > > > > should do
> > > > > this correctly from the beginning -- by supplying the phandle
> > > > > to
> > > > > the
> > > > > loader node.
> > > > Okay, i will improve firmware loader in separate patch set.
> > > > Basically, the ideas are for
> > > > 1. For majority HWs use default storage, defined in console
> > > > node.
> > > > (Already supported)
> > > I'm not sure I understand this point, default storage in console
> > > node
> > > ?
> > For example, we have 5 fpga manager nodes.
> > 
> > fpga_mgr1, fpga_mgr2, fpga_mgr3, and fpga_mgr4 are using sdmmc as
> > storage, so we can define sdmmc as default storage in console node
> > instead of setting phandle to each of fpga_mgr nodes.
> > 
> > Let say fpga_mgr5 is using QSPI, then we can set "firmware-loader =
> > <phandle>" to fpga_mgr5.
> > > 
> > > > 
> > > > 2. For minority HWs use, other than default storage, users
> > > > should
> > > > add
> > > > this property "firmware-loader = <phandle>" to their respective
> > > > HW
> > > > nodes. This would enable overwritten of default storage,
> > > > phandle
> > > > from
> > > > console node. (planning to enhance)
> > > > 3. Enable sequence number(DM_FLAG_PRE_RELOC) support through
> > > > hard
> > > > coding. (planning to enhance)
> > > Why do we want to hard-code anything ?
> > The purpose here is to support the feature of DM_FLAG_PRE_RELOC.
> > One of
> > the use case i think of searching the file such as u-boot.img in
> > every
> > storage supported in firmware loader. I have no strong opinion for
> > item
> > 3, we can support it in later when it is needed.
> > > 
> > > > 
> > > > So, the interface is still same for this patch set, using item
> > > > 1,
> > > > default storage.
> > > > 
> > > > What do you think?
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > It is good to improve the firmware loader to support
> > > > > > the DM_FLAG_PRE_RELOC, which allow user to choose different
> > > > > > firmware
> > > > > > loader node through setting the right sequence number when
> > > > > > creating
> > > > > > the
> > > > > > firmare loader instance in the parent driver such as fpga
> > > > > > mgr,
> > > > > > but
> > > > > > i
> > > > > > don't see there is urgency need to be done now.
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > +	};
> > > > > > > > > > +
> > > > > > > > > > +- Examples for booting with full release, enter
> > > > > > > > > > user
> > > > > > > > > > mode
> > > > > > > > > > with
> > > > > > > > > > full bitstream:
> > > > > > > > > >  
> > > > > > > > > >  	fpga_mgr: fpga-mgr at ffd03000 {
> > > > > > > > > >  		compatible = "altr,socfpga-a10-
> > > > > > > > > > fpga-
> > > > > > > > > > mgr";
> > > > > > > > > > @@ -16,4 +47,5 @@ Example:
> > > > > > > > > >  		       0xffcfe400 0x20>;
> > > > > > > > > >  		clocks = <&l4_mp_clk>;
> > > > > > > > > >  		resets = <&rst FPGAMGR_RESET>;
> > > > > > > > > > +		altr,bitstream =
> > > > > > > > > > "fit_spl_fpga.itb";
> > > > > > > > > >  	};
> > > > > > > > > >