On Wed, 2019-02-13 at 17:07 +0100, Marek Vasut wrote:

On 2/13/19 3:18 PM, tien.fong.chee@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 v8

• Removed explanation about support for altr,bitstream-core

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           | 26 +++++++++++++++++++++-  1 file changed, 25 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..da210bf 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,31 @@ 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 : Fit image file name for both FPGA peripheral bitstream,

•    FPGA core bitstream and full bitstream.
  

So the file contains three bitstreams ? I thought we can load only the core here.

Here is for telling FPGA driver which fitImage file gonna be processed. You can put whatever bitstreams in fitImage. Then, in the default configuration, you can tell SPL FPGA driver program which bitstream, it could be both periph.rbf and core.rbf or just programming periph.rbf only.

...

-Example:

•    Full bitstream, consist of peripheral bitstream
  

and core

•    bitstream.
  

•    FPGA peripheral bitstream is used to initialize
  

FPGA IOs,

•    PLL, IO48 and DDR. This bitstream is required
  

to get DDR up

•    running.
  

•    FPGA core bitstream contains FPGA design which
  

is used to

•    program FPGA CRAM and ERAM.
  

+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
  

Is the padding still required ?

Yes, i think that padding method should be sufficient for all use cases, i guess both NAND and QSPI may need this also.

You want me to support data offset(without padding) also?

...

•        -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 full release or booting with early IO release, then +  follow by entering early user mode:     fpga_mgr: fpga-mgr@ffd03000 {   compatible = "altr,socfpga-a10-fpga-mgr"; @@ -16,4 +39,5 @@ Example:          0xffcfe400 0x20>;   clocks = <&l4_mp_clk>;   resets = <&rst FPGAMGR_RESET>;

• altr,bitstream = "fit_spl_fpga.itb";
  

};

On Thu, 2019-02-14 at 11:34 +0100, Marek Vasut wrote:

On 2/14/19 6:55 AM, Chee, Tien Fong wrote:

...

On Wed, 2019-02-13 at 17:07 +0100, Marek Vasut wrote:

...

On 2/13/19 3:18 PM, tien.fong.chee@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 v8

• Removed explanation about support for altr,bitstream-core

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           | 26 +++++++++++++++++++++-  1 file changed, 25 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..da210bf 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,31 @@ 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 : Fit image file name for both FPGA peripheral bitstream,

•    FPGA core bitstream and full bitstream.
  

So the file contains three bitstreams ? I thought we can load only the core here.

Here is for telling FPGA driver which fitImage file gonna be processed. You can put whatever bitstreams in fitImage. Then, in the default configuration, you can tell SPL FPGA driver program which bitstream, it could be both periph.rbf and core.rbf or just programming periph.rbf only.

Ah, OK

...

...

...

-Example:

•    Full bitstream, consist of peripheral
  

bitstream and core

•    bitstream.
  

•    FPGA peripheral bitstream is used to
  

initialize FPGA IOs,

•    PLL, IO48 and DDR. This bitstream is
  

required to get DDR up

•    running.
  

•    FPGA core bitstream contains FPGA design
  

which is used to

•    program FPGA CRAM and ERAM.
  

+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
  

Is the padding still required ?

Yes, i think that padding method should be sufficient for all use cases, i guess both NAND and QSPI may need this also.

You want me to support data offset(without padding) also?

I think you should drop the padding, it seems to be workaround ?

I can add the data offset support, user is free to use either one of them.

On 2/13/19 3:18 PM, tien.fong.chee@intel.com wrote:

From: Tien Fong Chee tien.fong.chee@intel.com

Add default fitImage file bundling FPGA bitstreams for Arria10.

Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com

---

changes for v8

• Changed the FPGA node name to fpga-core and fpga-periph for both core and periph bitstreams respectively.

---

board/altera/arria10-socdk/fit_spl_fpga.its | 39 +++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 board/altera/arria10-socdk/fit_spl_fpga.its

diff --git a/board/altera/arria10-socdk/fit_spl_fpga.its b/board/altera/arria10-socdk/fit_spl_fpga.its new file mode 100644 index 0000000..8ce175b --- /dev/null +++ b/board/altera/arria10-socdk/fit_spl_fpga.its @@ -0,0 +1,39 @@ +// SPDX-License-Identifier: GPL-2.0

• /*
• • Copyright (C) 2019 Intel Corporation <www.intel.com>
• */

+/dts-v1/;

+/ {

• description = "FIT image with FPGA bistream";
• #address-cells = <1>;
• images {

• fpga-core@1 {
  

• 	description = "FPGA core bitstream";
  

• 	data = /incbin/("../../../ghrd_10as066n2.core.rbf");
  

• 	type = "fpga";
  

• 	arch = "arm";
  

• 	compression = "none";
  

• 	load = <0x400>;
  

Is the load address required ?

• };
  

• fpga-periph@2 {
  

• 	description = "FPGA peripheral bitstream";
  

• 	data = /incbin/("../../../ghrd_10as066n2.periph.rbf");
  

• 	type = "fpga";
  

• 	arch = "arm";
  

• 	compression = "none";
  

• };
  

• };
• configurations {

• default = "config-1";
  

• config-1 {
  

• 	description = "Boot with FPGA early IO release config";
  

• 	fpga = "fpga-periph@2", "fpga-core@1";
  

Don't you need to load the core first ?

• };
  

• };

+};

On 2/13/19 11:45 PM, Dalon L Westergreen wrote:

On Wed, 2019-02-13 at 17:10 +0100, Marek Vasut wrote:

...

On 2/13/19 3:18 PM, tien.fong.chee@intel.com wrote:

...

From: Tien Fong Chee tien.fong.chee@intel.com

Add default fitImage file bundling FPGA bitstreams for Arria10.

Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com

---

changes for v8

• Changed the FPGA node name to fpga-core and fpga-periph for both core and periph bitstreams respectively.

---

board/altera/arria10-socdk/fit_spl_fpga.its | 39 +++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 board/altera/arria10-socdk/fit_spl_fpga.its

diff --git a/board/altera/arria10-socdk/fit_spl_fpga.its b/board/altera/arria10-socdk/fit_spl_fpga.its new file mode 100644 index 0000000..8ce175b --- /dev/null +++ b/board/altera/arria10-socdk/fit_spl_fpga.its @@ -0,0 +1,39 @@ +// SPDX-License-Identifier: GPL-2.0

• /*
• • Copyright (C) 2019 Intel Corporation <www.intel.com>
• */

+/dts-v1/;

+/ {

• description = "FIT image with FPGA bistream";
• #address-cells = <1>;
• images {

• fpga-core@1 {
  

• 	description = "FPGA core bitstream";
  

• 	data = /incbin/("../../../ghrd_10as066n2.core.rbf");
  

• 	type = "fpga";
  

• 	arch = "arm";
  

• 	compression = "none";
  

• 	load = <0x400>;
  

Is the load address required ?

...

• };
  

• fpga-periph@2 {
  

• 	description = "FPGA peripheral bitstream";
  

• 	data = /incbin/("../../../ghrd_10as066n2.periph.rbf");
  

• 	type = "fpga";
  

• 	arch = "arm";
  

• 	compression = "none";
  

• };
  

• };
• configurations {

• default = "config-1";
  

• config-1 {
  

• 	description = "Boot with FPGA early IO release config";
  

• 	fpga = "fpga-periph@2", "fpga-core@1";
  

Don't you need to load the core first ?

No, the periphery is first. This brings up the dram and i/o.

Then why do we have periph@2 above ? Shouldn't those two images be swapped to make this look less confusing ?

On 2/14/19 7:04 AM, Chee, Tien Fong wrote:

On Thu, 2019-02-14 at 00:04 +0100, Marek Vasut wrote:

...

On 2/13/19 11:45 PM, Dalon L Westergreen wrote:

...

On Wed, 2019-02-13 at 17:10 +0100, Marek Vasut wrote:

...

On 2/13/19 3:18 PM, tien.fong.chee@intel.com wrote:

...

From: Tien Fong Chee tien.fong.chee@intel.com

Add default fitImage file bundling FPGA bitstreams for Arria10.

Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com

---

changes for v8

• Changed the FPGA node name to fpga-core and fpga-periph for

both core and   periph bitstreams respectively.

---

board/altera/arria10-socdk/fit_spl_fpga.its | 39 +++++++++++++++++++++++++++++  1 file changed, 39 insertions(+)  create mode 100644 board/altera/arria10-socdk/fit_spl_fpga.its

diff --git a/board/altera/arria10-socdk/fit_spl_fpga.its b/board/altera/arria10-socdk/fit_spl_fpga.its new file mode 100644 index 0000000..8ce175b --- /dev/null +++ b/board/altera/arria10-socdk/fit_spl_fpga.its @@ -0,0 +1,39 @@ +// SPDX-License-Identifier: GPL-2.0

• /*
• • Copyright (C) 2019 Intel Corporation <www.intel.com>
• */

+/dts-v1/;

+/ {

• description = "FIT image with FPGA bistream";
• #address-cells = <1>;
• images {

• fpga-core@1 {
  

• 	description = "FPGA core bitstream";
  

• 	data =
  

/incbin/("../../../ghrd_10as066n2.core.rbf");

• 	type = "fpga";
  

• 	arch = "arm";
  

• 	compression = "none";
  

• 	load = <0x400>;
  

Is the load address required ?

It is optional for telling destination address of DDR where this core.rbf going to be loaded. If load property, the default OCRAM buffer would be used, bad for performance when loading chunk by chunk.

So if I have something at 0x400 in DRAM and use this example in U-Boot, it will be overwritten ?

How is OCRAM bad for performance ?

...

...

...

...

• };
  

• fpga-periph@2 {
  

• 	description = "FPGA peripheral
  

bitstream";

• 	data =
  

/incbin/("../../../ghrd_10as066n2.periph.rbf");

• 	type = "fpga";
  

• 	arch = "arm";
  

• 	compression = "none";
  

• };
  

• };
• configurations {

• default = "config-1";
  

• config-1 {
  

• 	description = "Boot with FPGA early IO
  

release config";

• 	fpga = "fpga-periph@2", "fpga-core@1";
  

Don't you need to load the core first ?

No, the periphery is first.  This brings up the dram and i/o.

Then why do we have periph@2 above ? Shouldn't those two images be swapped to make this look less confusing ?

The ordering in configuration fpga property doesn't matter, the driver smart enough to determine what bitstream need to be programmed at what FPGA mode.

Good, then please order it naturally, @1 then @2 etc .

The image fpga-core@1 is alligned at 1st just for avoiding the performance penalty.

I thought we concluded this should be fixed elsewhere ?

On 2/14/19 12:23 PM, Chee, Tien Fong wrote:

On Thu, 2019-02-14 at 11:35 +0100, Marek Vasut wrote:

...

On 2/14/19 7:04 AM, Chee, Tien Fong wrote:

...

On Thu, 2019-02-14 at 00:04 +0100, Marek Vasut wrote:

...

On 2/13/19 11:45 PM, Dalon L Westergreen wrote:

...

On Wed, 2019-02-13 at 17:10 +0100, Marek Vasut wrote:

...

On 2/13/19 3:18 PM, tien.fong.chee@intel.com wrote: > > > From: Tien Fong Chee tien.fong.chee@intel.com > > Add default fitImage file bundling FPGA bitstreams for > Arria10. > > Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com > > --- > > changes for v8 > - Changed the FPGA node name to fpga-core and fpga-periph > for > both core and >   periph bitstreams respectively. > --- >  board/altera/arria10-socdk/fit_spl_fpga.its | 39 > +++++++++++++++++++++++++++++ >  1 file changed, 39 insertions(+) >  create mode 100644 board/altera/arria10- > socdk/fit_spl_fpga.its > > diff --git a/board/altera/arria10-socdk/fit_spl_fpga.its > b/board/altera/arria10-socdk/fit_spl_fpga.its > new file mode 100644 > index 0000000..8ce175b > --- /dev/null > +++ b/board/altera/arria10-socdk/fit_spl_fpga.its > @@ -0,0 +1,39 @@ > +// SPDX-License-Identifier: GPL-2.0 > + /* > + * Copyright (C) 2019 Intel Corporation <www.intel.com> > + * > + */ > + > +/dts-v1/; > + > +/ { > + description = "FIT image with FPGA bistream"; > + #address-cells = <1>; > + > + images { > + fpga-core@1 { > + description = "FPGA core > bitstream"; > + data = > /incbin/("../../../ghrd_10as066n2.core.rbf"); > + type = "fpga"; > + arch = "arm"; > + compression = "none"; > + load = <0x400>; Is the load address required ?

It is optional for telling destination address of DDR where this core.rbf going to be loaded. If load property, the default OCRAM buffer would be used, bad for performance when loading chunk by chunk.

So if I have something at 0x400 in DRAM and use this example in U- Boot, it will be overwritten ?

This is used for SPL only, at least for now, so that is before loading the U-Boot, DDR is blank. But, you can define the blank location. If load property is not defined, the driver would use small buffer from OCRAM.

...

How is OCRAM bad for performance ?

With DDR, the performance can up to 85-90%. It is because very limited space in OCRAM, so very small buffer is used for loading bitstream, so the bitstream has to be loaded chunk by chunk. For DDR, where whole bitstream can be loaded.

Shouldn't the logic to determine where the buffer is be in the code ? I mean, once you have DRAM available, you have all the malloc space, so you can use larger buffer. Why encode that knowledge into the fitImage ?

...

...

...

...

...

> > + }; > + > + fpga-periph@2 { > + description = "FPGA peripheral > bitstream"; > + data = > /incbin/("../../../ghrd_10as066n2.periph.rbf"); > + type = "fpga"; > + arch = "arm"; > + compression = "none"; > + }; > + }; > + > + configurations { > + default = "config-1"; > + config-1 { > + description = "Boot with FPGA > early IO > release config"; > + fpga = "fpga-periph@2", "fpga-core > @1"; Don't you need to load the core first ?

No, the periphery is first.  This brings up the dram and i/o.

Then why do we have periph@2 above ? Shouldn't those two images be swapped to make this look less confusing ?

The ordering in configuration fpga property doesn't matter, the driver smart enough to determine what bitstream need to be programmed at what FPGA mode.

Good, then please order it naturally, @1 then @2 etc .

Okay.

...

...

The image fpga-core@1 is alligned at 1st just for avoiding  the performance penalty.

I thought we concluded this should be fixed elsewhere ?

May be we can wait until there is a solution? So user can benefit the good performance with this default fitIMage?

This will only work for specific cases of specific storage devices and filesystems .

On 2/14/19 4:11 PM, Chee, Tien Fong wrote:

On Thu, 2019-02-14 at 13:24 +0100, Marek Vasut wrote:

...

On 2/14/19 12:23 PM, Chee, Tien Fong wrote:

...

On Thu, 2019-02-14 at 11:35 +0100, Marek Vasut wrote:

...

On 2/14/19 7:04 AM, Chee, Tien Fong wrote:

...

On Thu, 2019-02-14 at 00:04 +0100, Marek Vasut wrote:

...

On 2/13/19 11:45 PM, Dalon L Westergreen wrote: > > > > On Wed, 2019-02-13 at 17:10 +0100, Marek Vasut wrote: >> >> >> >> On 2/13/19 3:18 PM, tien.fong.chee@intel.com wrote: >>> >>> >>> >>> From: Tien Fong Chee tien.fong.chee@intel.com >>> >>> Add default fitImage file bundling FPGA bitstreams for >>> Arria10. >>> >>> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com >>>> >>> >>> --- >>> >>> changes for v8 >>> - Changed the FPGA node name to fpga-core and fpga- >>> periph >>> for >>> both core and >>>   periph bitstreams respectively. >>> --- >>>  board/altera/arria10-socdk/fit_spl_fpga.its | 39 >>> +++++++++++++++++++++++++++++ >>>  1 file changed, 39 insertions(+) >>>  create mode 100644 board/altera/arria10- >>> socdk/fit_spl_fpga.its >>> >>> diff --git a/board/altera/arria10- >>> socdk/fit_spl_fpga.its >>> b/board/altera/arria10-socdk/fit_spl_fpga.its >>> new file mode 100644 >>> index 0000000..8ce175b >>> --- /dev/null >>> +++ b/board/altera/arria10-socdk/fit_spl_fpga.its >>> @@ -0,0 +1,39 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> + /* >>> + * Copyright (C) 2019 Intel Corporation <www.intel.com >>>> >>> + * >>> + */ >>> + >>> +/dts-v1/; >>> + >>> +/ { >>> + description = "FIT image with FPGA bistream"; >>> + #address-cells = <1>; >>> + >>> + images { >>> + fpga-core@1 { >>> + description = "FPGA core >>> bitstream"; >>> + data = >>> /incbin/("../../../ghrd_10as066n2.core.rbf"); >>> + type = "fpga"; >>> + arch = "arm"; >>> + compression = "none"; >>> + load = <0x400>; >> Is the load address required ?

It is optional for telling destination address of DDR where this core.rbf going to be loaded. If load property, the default OCRAM buffer would be used, bad for performance when loading chunk by chunk.

So if I have something at 0x400 in DRAM and use this example in U- Boot, it will be overwritten ?

This is used for SPL only, at least for now, so that is before loading the U-Boot, DDR is blank. But, you can define the blank location. If load property is not defined, the driver would use small buffer from OCRAM.

...

How is OCRAM bad for performance ?

With DDR, the performance can up to 85-90%. It is because very limited space in OCRAM, so very small buffer is used for loading bitstream, so the bitstream has to be loaded chunk by chunk. For DDR, where whole bitstream can be loaded.

Shouldn't the logic to determine where the buffer is be in the code ? I mean, once you have DRAM available, you have all the malloc space, so you can use larger buffer.Why encode that knowledge into the fitImage ?

In previosly, we hard code the dest location for loading the core bitstream in SPL, because by that time DDR up running, but malloc is still pointed to OCRAM. Now, we let user to define the dest location through the load property.

But the user shouldn't care where the buffer is, the buffer should be in the best possible location and the SPL can determine that (if DRAM is running, in DRAM, otherwise in OCRAM), no ?

On 2/14/19 4:47 PM, Chee, Tien Fong wrote:

On Thu, 2019-02-14 at 16:13 +0100, Marek Vasut wrote:

...

On 2/14/19 4:11 PM, Chee, Tien Fong wrote:

...

On Thu, 2019-02-14 at 13:24 +0100, Marek Vasut wrote:

...

On 2/14/19 12:23 PM, Chee, Tien Fong wrote:

...

On Thu, 2019-02-14 at 11:35 +0100, Marek Vasut wrote:

...

On 2/14/19 7:04 AM, Chee, Tien Fong wrote: > > > > On Thu, 2019-02-14 at 00:04 +0100, Marek Vasut wrote: >> >> >> >> On 2/13/19 11:45 PM, Dalon L Westergreen wrote: >>> >>> >>> >>> >>> On Wed, 2019-02-13 at 17:10 +0100, Marek Vasut wrote: >>>> >>>> >>>> >>>> >>>> On 2/13/19 3:18 PM, tien.fong.chee@intel.com wrote: >>>>> >>>>> >>>>> >>>>> >>>>> From: Tien Fong Chee tien.fong.chee@intel.com >>>>> >>>>> Add default fitImage file bundling FPGA bitstreams >>>>> for >>>>> Arria10. >>>>> >>>>> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel >>>>> .com >>>>>> >>>>>> >>>>> --- >>>>> >>>>> changes for v8 >>>>> - Changed the FPGA node name to fpga-core and fpga- >>>>> periph >>>>> for >>>>> both core and >>>>>   periph bitstreams respectively. >>>>> --- >>>>>  board/altera/arria10-socdk/fit_spl_fpga.its | 39 >>>>> +++++++++++++++++++++++++++++ >>>>>  1 file changed, 39 insertions(+) >>>>>  create mode 100644 board/altera/arria10- >>>>> socdk/fit_spl_fpga.its >>>>> >>>>> diff --git a/board/altera/arria10- >>>>> socdk/fit_spl_fpga.its >>>>> b/board/altera/arria10-socdk/fit_spl_fpga.its >>>>> new file mode 100644 >>>>> index 0000000..8ce175b >>>>> --- /dev/null >>>>> +++ b/board/altera/arria10-socdk/fit_spl_fpga.its >>>>> @@ -0,0 +1,39 @@ >>>>> +// SPDX-License-Identifier: GPL-2.0 >>>>> + /* >>>>> + * Copyright (C) 2019 Intel Corporation <www.intel >>>>> .com >>>>>> >>>>>> >>>>> + * >>>>> + */ >>>>> + >>>>> +/dts-v1/; >>>>> + >>>>> +/ { >>>>> + description = "FIT image with FPGA >>>>> bistream"; >>>>> + #address-cells = <1>; >>>>> + >>>>> + images { >>>>> + fpga-core@1 { >>>>> + description = "FPGA core >>>>> bitstream"; >>>>> + data = >>>>> /incbin/("../../../ghrd_10as066n2.core.rbf"); >>>>> + type = "fpga"; >>>>> + arch = "arm"; >>>>> + compression = "none"; >>>>> + load = <0x400>; >>>> Is the load address required ? > It is optional for telling destination address of DDR where > this > core.rbf going to be loaded. If load property, the default > OCRAM > buffer > would be used, bad for performance when loading chunk by > chunk. So if I have something at 0x400 in DRAM and use this example in U- Boot, it will be overwritten ?

This is used for SPL only, at least for now, so that is before loading the U-Boot, DDR is blank. But, you can define the blank location. If load property is not defined, the driver would use small buffer from OCRAM.

...

How is OCRAM bad for performance ?

With DDR, the performance can up to 85-90%. It is because very limited space in OCRAM, so very small buffer is used for loading bitstream, so the bitstream has to be loaded chunk by chunk. For DDR, where whole bitstream can be loaded.

Shouldn't the logic to determine where the buffer is be in the code ? I mean, once you have DRAM available, you have all the malloc space, so you can use larger buffer.Why encode that knowledge into the fitImage ?

In previosly, we hard code the dest location for loading the core bitstream in SPL, because by that time DDR up running, but malloc is still pointed to OCRAM. Now, we let user to define the dest location through the load property.

But the user shouldn't care where the buffer is, the buffer should be in the best possible location and the SPL can determine that (if DRAM is running, in DRAM, otherwise in OCRAM), no ?

Yes, most of the time user don't need to change the dest value at load property, unless they have very good reason to do so. SPL would smart enough to read the load property after the DDR is up running, and loading the bitstream to dest location, and programming FPGA from there.

Then we don't need the 'load' property by default ?

On Thu, 2019-02-14 at 17:26 +0100, Marek Vasut wrote:

On 2/14/19 4:47 PM, Chee, Tien Fong wrote:

...

On Thu, 2019-02-14 at 16:13 +0100, Marek Vasut wrote:

...

On 2/14/19 4:11 PM, Chee, Tien Fong wrote:

...

On Thu, 2019-02-14 at 13:24 +0100, Marek Vasut wrote:

...

On 2/14/19 12:23 PM, Chee, Tien Fong wrote:

...

On Thu, 2019-02-14 at 11:35 +0100, Marek Vasut wrote: > > > > On 2/14/19 7:04 AM, Chee, Tien Fong wrote: > > > > > > > > > > On Thu, 2019-02-14 at 00:04 +0100, Marek Vasut wrote: > > > > > > > > > > > > > > > On 2/13/19 11:45 PM, Dalon L Westergreen wrote: > > > > > > > > > > > > > > > > > > > > > > > > On Wed, 2019-02-13 at 17:10 +0100, Marek Vasut > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 2/13/19 3:18 PM, tien.fong.chee@intel.com > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > From: Tien Fong Chee tien.fong.chee@intel.com > > > > > > > > > > > > Add default fitImage file bundling FPGA > > > > > > bitstreams > > > > > > for > > > > > > Arria10. > > > > > > > > > > > > Signed-off-by: Tien Fong Chee <tien.fong.chee@i > > > > > > ntel > > > > > > .com > > > > > > > > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > changes for v8 > > > > > > - Changed the FPGA node name to fpga-core and > > > > > > fpga- > > > > > > periph > > > > > > for > > > > > > both core and > > > > > >   periph bitstreams respectively. > > > > > > --- > > > > > >  board/altera/arria10-socdk/fit_spl_fpga.its | > > > > > > 39 > > > > > > +++++++++++++++++++++++++++++ > > > > > >  1 file changed, 39 insertions(+) > > > > > >  create mode 100644 board/altera/arria10- > > > > > > socdk/fit_spl_fpga.its > > > > > > > > > > > > diff --git a/board/altera/arria10- > > > > > > socdk/fit_spl_fpga.its > > > > > > b/board/altera/arria10-socdk/fit_spl_fpga.its > > > > > > new file mode 100644 > > > > > > index 0000000..8ce175b > > > > > > --- /dev/null > > > > > > +++ b/board/altera/arria10- > > > > > > socdk/fit_spl_fpga.its > > > > > > @@ -0,0 +1,39 @@ > > > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > > > + /* > > > > > > + * Copyright (C) 2019 Intel Corporation <www.i > > > > > > ntel > > > > > > .com > > > > > > > > > > > > > > > > > > > > > > > > > > > + * > > > > > > + */ > > > > > > + > > > > > > +/dts-v1/; > > > > > > + > > > > > > +/ { > > > > > > + description = "FIT image with FPGA > > > > > > bistream"; > > > > > > + #address-cells = <1>; > > > > > > + > > > > > > + images { > > > > > > + fpga-core@1 { > > > > > > + description = "FPGA > > > > > > core > > > > > > bitstream"; > > > > > > + data = > > > > > > /incbin/("../../../ghrd_10as066n2.core.rbf"); > > > > > > + type = "fpga"; > > > > > > + arch = "arm"; > > > > > > + compression = "none"; > > > > > > + load = <0x400>; > > > > > Is the load address required ? > > It is optional for telling destination address of DDR > > where > > this > > core.rbf going to be loaded. If load property, the > > default > > OCRAM > > buffer > > would be used, bad for performance when loading chunk > > by > > chunk. > So if I have something at 0x400 in DRAM and use this > example > in > U- > Boot, > it will be overwritten ? This is used for SPL only, at least for now, so that is before loading the U-Boot, DDR is blank. But, you can define the blank location. If load property is not defined, the driver would use small buffer from OCRAM. > > > > > How is OCRAM bad for performance ? With DDR, the performance can up to 85-90%. It is because very limited space in OCRAM, so very small buffer is used for loading bitstream, so the bitstream has to be loaded chunk by chunk. For DDR, where whole bitstream can be loaded.

Shouldn't the logic to determine where the buffer is be in the code ? I mean, once you have DRAM available, you have all the malloc space, so you can use larger buffer.Why encode that knowledge into the fitImage ?

In previosly, we hard code the dest location for loading the core bitstream in SPL, because by that time DDR up running, but malloc is still pointed to OCRAM. Now, we let user to define the dest location through the load property.

But the user shouldn't care where the buffer is, the buffer should be in the best possible location and the SPL can determine that (if DRAM is running, in DRAM, otherwise in OCRAM), no ?

Yes, most of the time user don't need to change the dest value at load property, unless they have very good reason to do so. SPL would smart enough to read the load property after the DDR is up running, and loading the bitstream to dest location, and programming FPGA from there.

Then we don't need the 'load' property by default ?

I will add a default ddr load address in the code, and enable the load property to overwrite it if it is defined.

So, that means the load property is optional.

On 2/13/19 3:18 PM, tien.fong.chee@intel.com wrote:

From: Tien Fong Chee tien.fong.chee@intel.com

Adding some function declarations to the header file, so these functions can be referred by other C files.

Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com

include/image.h | 4 ++++ 1 file changed, 4 insertions(+)

diff --git a/include/image.h b/include/image.h index 83a2d41..f839443 100644 --- a/include/image.h +++ b/include/image.h @@ -1041,6 +1041,10 @@ int fit_check_format(const void *fit);

int fit_conf_find_compat(const void *fit, const void *fdt); int fit_conf_get_node(const void *fit, const char *conf_uname); +int fit_conf_get_prop_node_count(const void *fit, int noffset,

• const char *prop_name);
  

+int fit_conf_get_prop_node_index(const void *fit, int noffset,

• const char *prop_name, int index);
  

Squash this patch with a patch adding those functions.

/**

• fit_conf_get_prop_node() - Get node refered to by a configuration

On Wed, 2019-02-13 at 17:11 +0100, Marek Vasut wrote:

On 2/13/19 3:18 PM, tien.fong.chee@intel.com wrote:

...

From: Tien Fong Chee tien.fong.chee@intel.com

Adding some function declarations to the header file, so these functions can be referred by other C files.

Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com

include/image.h | 4 ++++  1 file changed, 4 insertions(+)

diff --git a/include/image.h b/include/image.h index 83a2d41..f839443 100644 --- a/include/image.h +++ b/include/image.h @@ -1041,6 +1041,10 @@ int fit_check_format(const void *fit);    int fit_conf_find_compat(const void *fit, const void *fdt);  int fit_conf_get_node(const void *fit, const char *conf_uname); +int fit_conf_get_prop_node_count(const void *fit, int noffset,

• const char *prop_name);
  

+int fit_conf_get_prop_node_index(const void *fit, int noffset,

• const char *prop_name, int index);
  

Squash this patch with a patch adding those functions.

Just to confirm again squashing this patch to the patch calling these function?

...

/**   * fit_conf_get_prop_node() - Get node refered to by a configuration

On Thu, 2019-02-14 at 13:28 +0100, Marek Vasut wrote:

On 2/14/19 12:51 PM, Chee, Tien Fong wrote:

...

On Wed, 2019-02-13 at 17:11 +0100, Marek Vasut wrote:

...

On 2/13/19 3:18 PM, tien.fong.chee@intel.com wrote:

...

From: Tien Fong Chee tien.fong.chee@intel.com

Adding some function declarations to the header file, so these functions can be referred by other C files.

Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com

include/image.h | 4 ++++  1 file changed, 4 insertions(+)

diff --git a/include/image.h b/include/image.h index 83a2d41..f839443 100644 --- a/include/image.h +++ b/include/image.h @@ -1041,6 +1041,10 @@ int fit_check_format(const void *fit);    int fit_conf_find_compat(const void *fit, const void *fdt);  int fit_conf_get_node(const void *fit, const char *conf_uname); +int fit_conf_get_prop_node_count(const void *fit, int noffset,

• const char *prop_name);
  

+int fit_conf_get_prop_node_index(const void *fit, int noffset,

• const char *prop_name, int index);
  

Squash this patch with a patch adding those functions.

Just to confirm again squashing this patch to the patch calling these function?

Yes

Okay.

On 2/13/19 3:18 PM, tien.fong.chee@intel.com wrote:

From: Tien Fong Chee tien.fong.chee@intel.com

Add FPGA driver to support program FPGA with FPGA bitstream loading from filesystem. The driver are designed based on generic firmware loader framework. The driver can handle FPGA program operation from loading FPGA bitstream in flash to memory and then to program FPGA.

Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com

---

changes for v8

• Added codes to discern bitstream type based on fpga node name.

changes for v7

• Restructure the FPGA driver to support both peripheral bitstream and core bitstream bundled into FIT image.
• Support loadable property for core bitstream. User can set loadable in DDR for better performance. This loading would be done in one large chunk instead of chunk by chunk loading with small memory buffer.

---

arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts | 17 + .../include/mach/fpga_manager_arria10.h | 39 +- drivers/fpga/socfpga_arria10.c | 467 ++++++++++++++++++++- 3 files changed, 500 insertions(+), 23 deletions(-)

diff --git a/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts b/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts index 998d811..14f1967 100644 --- a/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts +++ b/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts @@ -18,6 +18,23 @@ /dts-v1/; #include "socfpga_arria10_socdk.dtsi"

+/ {

• chosen {

• firmware-loader = &fs_loader0;
  

Should be a phandle.

• };
• fs_loader0: fs-loader@0 {

• u-boot,dm-pre-reloc;
  

• compatible = "u-boot,fs-loader";
  

• phandlepart = <&mmc 1>;
  

• };

+};

+&fpga_mgr {

• u-boot,dm-pre-reloc;
• altr,bitstream = "fit_spl_fpga.itb";

+};

&mmc { u-boot,dm-pre-reloc; status = "okay"; diff --git a/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h b/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h index 09d13f6..5ef15bb 100644 --- a/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h +++ b/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h @@ -1,9 +1,13 @@ /* SPDX-License-Identifier: GPL-2.0 */ /*

• • Copyright (C) 2017 Intel Corporation <www.intel.com>

• • Copyright (C) 2017-2019 Intel Corporation <www.intel.com>
  • All rights reserved.
  */

+#include <asm/cache.h> +#include <altera.h> +#include <image.h>

#ifndef _FPGA_MANAGER_ARRIA10_H_ #define _FPGA_MANAGER_ARRIA10_H_

@@ -51,6 +55,10 @@ #define ALT_FPGAMGR_IMGCFG_CTL_02_CFGWIDTH_SET_MSK BIT(24) #define ALT_FPGAMGR_IMGCFG_CTL_02_CDRATIO_LSB 16

+#define FPGA_SOCFPGA_A10_RBF_UNENCRYPTED 0xa65c +#define FPGA_SOCFPGA_A10_RBF_ENCRYPTED 0xa65d +#define FPGA_SOCFPGA_A10_RBF_PERIPH 0x0001 +#define FPGA_SOCFPGA_A10_RBF_CORE 0x8001 #ifndef __ASSEMBLY__

struct socfpga_fpga_manager { @@ -88,12 +96,39 @@ struct socfpga_fpga_manager { u32 imgcfg_fifo_status; };

+enum rbf_type {

• unknown,
• periph_section,
• core_section

+};

+enum rbf_security {

• invalid,
• unencrypted,
• encrypted

+};

+struct rbf_info {

• enum rbf_type section;
• enum rbf_security security;

+};

+struct fpga_loadfs_info {

• fpga_fs_info *fpga_fsinfo;
• u32 remaining;
• u32 offset;
• struct rbf_info rbfinfo;

+};

/* Functions */ int fpgamgr_program_init(u32 * rbf_data, size_t rbf_size); int fpgamgr_program_finish(void); int is_fpgamgr_user_mode(void); int fpgamgr_wait_early_user_mode(void);

+int is_fpgamgr_early_user_mode(void); +char *get_fpga_filename(const void *fdt, int *len); +int socfpga_loadfs(fpga_fs_info *fpga_fsinfo, const void *buf, size_t bsize,

•   u32 offset);
  

#endif /* __ASSEMBLY__ */

#endif /* _FPGA_MANAGER_ARRIA10_H_ */ diff --git a/drivers/fpga/socfpga_arria10.c b/drivers/fpga/socfpga_arria10.c index 114dd91..630d5a3 100644 --- a/drivers/fpga/socfpga_arria10.c +++ b/drivers/fpga/socfpga_arria10.c @@ -1,8 +1,7 @@ // SPDX-License-Identifier: GPL-2.0 /*

• • Copyright (C) 2017 Intel Corporation <www.intel.com>

• • Copyright (C) 2017-2019 Intel Corporation <www.intel.com>
  */

#include <asm/io.h> #include <asm/arch/fpga_manager.h> #include <asm/arch/reset_manager.h> @@ -10,8 +9,11 @@ #include <asm/arch/sdram.h> #include <asm/arch/misc.h> #include <altera.h> +#include <asm/arch/pinmux.h> #include <common.h> +#include <dm/ofnode.h> #include <errno.h> +#include <fs_loader.h> #include <wait_bit.h> #include <watchdog.h>

@@ -64,7 +66,7 @@ static int wait_for_user_mode(void) 1, FPGA_TIMEOUT_MSEC, false); }

-static int is_fpgamgr_early_user_mode(void) +int is_fpgamgr_early_user_mode(void) { return (readl(&fpga_manager_base->imgcfg_stat) & ALT_FPGAMGR_IMGCFG_STAT_F2S_EARLY_USERMODE_SET_MSK) != 0; @@ -92,9 +94,10 @@ int fpgamgr_wait_early_user_mode(void) sizeof(sync_data)); udelay(FPGA_TIMEOUT_MSEC); i++;

• WATCHDOG_RESET();
  

udelay() already triggers watchdog, why is this needed here ?

}

• debug("Additional %i sync word needed\n", i);

• debug("FPGA: Additional %i sync word needed\n", i);

  /* restoring original CDRATIO */ fpgamgr_set_cd_ratio(cd_ratio);

@@ -172,9 +175,10 @@ static int fpgamgr_set_cdratio_cdwidth(unsigned int cfg_width, u32 *rbf_data, compress = (rbf_data[COMPRESSION_OFFSET] >> 1) & 1; compress = !compress;

• debug("header word %d = %08x\n", 69, rbf_data[69]);
• debug("header word %d = %08x\n", 229, rbf_data[229]);
• debug("read from rbf header: encrypt=%d compress=%d\n", encrypt, compress);

• debug("FPGA: Header word %d = %08x.\n", 69, rbf_data[69]);

• debug("FPGA: Header word %d = %08x.\n", 229, rbf_data[229]);

• debug("FPGA: Read from rbf header: encrypt=%d compress=%d.\n", encrypt,

•     compress);
  
  

  /*

  • from the register map description of cdratio in imgcfg_ctrl_02:

@@ -359,6 +363,7 @@ static int fpgamgr_program_poll_cd(void) printf("nstatus == 0 while waiting for condone\n"); return -EPERM; }

• WATCHDOG_RESET();
  

Why is this needed ?

}

if (i == FPGA_TIMEOUT_CNT) @@ -432,7 +437,6 @@ int fpgamgr_program_finish(void) printf("FPGA: Poll CD failed with error code %d\n", status); return -EPERM; }

• WATCHDOG_RESET();

  /* Ensure the FPGA entering user mode */ status = fpgamgr_program_poll_usermode();

@@ -447,27 +451,448 @@ int fpgamgr_program_finish(void) return 0; }

-/*

• • FPGA Manager to program the FPGA. This is the interface used by FPGA driver.
• • Return 0 for sucess, non-zero for error.
• */

+char *get_fpga_filename(const void *fdt, int *len) +{

• char *fpga_filename = NULL;
• int node_offset;
• fdtdec_find_aliases_for_id(gd->fdt_blob, "fpga_mgr",

• 		COMPAT_ALTERA_SOCFPGA_FPGA0,
  

• 		&node_offset, 1);
  

• ofnode fpgamgr_node = offset_to_ofnode(node_offset);
• if (ofnode_valid(fpgamgr_node))

• fpga_filename = (char *)ofnode_read_string(fpgamgr_node,
  

• 				"altr,bitstream");
  

Why is the cast needed ? Drop the two newlines.

• return fpga_filename;

+}

+static void get_rbf_image_info(struct rbf_info *rbf, u16 *buffer) +{

• /*

• * Magic ID starting at:
  

• * -> 1st dword[15:0] in periph.rbf
  

• * -> 2nd dword[15:0] in core.rbf
  

• * Note: dword == 32 bits
  

• */
  

• u32 word_reading_max = 2;
• u32 i;
• for (i = 0; i < word_reading_max; i++) {

• if (*(buffer + i) == FPGA_SOCFPGA_A10_RBF_UNENCRYPTED) {
  

• 	rbf->security = unencrypted;
  

• } else if (*(buffer + i) == FPGA_SOCFPGA_A10_RBF_ENCRYPTED) {
  

• 	rbf->security = encrypted;
  

• } else if (*(buffer + i + 1) ==
  

• 		FPGA_SOCFPGA_A10_RBF_UNENCRYPTED) {
  

• 	rbf->security = unencrypted;
  

• } else if (*(buffer + i + 1) ==
  

• 		FPGA_SOCFPGA_A10_RBF_ENCRYPTED) {
  

• 	rbf->security = encrypted;
  

• } else {
  

• 	rbf->security = invalid;
  

• 	continue;
  

• }
  

• /* PERIPH RBF(buffer + i + 1), CORE RBF(buffer + i + 2) */
  

• if (*(buffer + i + 1) == FPGA_SOCFPGA_A10_RBF_PERIPH) {
  

• 	rbf->section = periph_section;
  

• 	break;
  

• } else if (*(buffer + i + 1) == FPGA_SOCFPGA_A10_RBF_CORE) {
  

• 	rbf->section = core_section;
  

• 	break;
  

• } else if (*(buffer + i + 2) == FPGA_SOCFPGA_A10_RBF_PERIPH) {
  

• 	rbf->section = periph_section;
  

• 	break;
  

• } else if (*(buffer + i + 2) == FPGA_SOCFPGA_A10_RBF_CORE) {
  

• 	rbf->section = core_section;
  

• 	break;
  

• }
  

• rbf->section = unknown;
  

• break;
  

• WATCHDOG_RESET();
  

• }

+}

+#ifdef CONFIG_FS_LOADER +static int first_loading_rbf_to_buffer(struct udevice *dev,

• 		struct fpga_loadfs_info *fpga_loadfs,
  

• 		u32 *buffer, size_t *buffer_bsize)
  

+{

• u32 *buffer_p = (u32 *)*buffer;
• u32 *loadable = buffer_p;
• size_t buffer_size = *buffer_bsize;
• size_t fit_size;
• int ret, i, count;
• int confs_noffset, images_noffset;
• int rbf_offset;
• int rbf_size;
• const char *fpga_node_name = NULL;
• const char *uname = NULL;
• /* Load image header into buffer */
• ret = request_firmware_into_buf(dev,

• 			fpga_loadfs->fpga_fsinfo->filename,
  

• 			buffer_p,
  

• 			sizeof(struct image_header),
  

• 			0);
  

• if (ret < 0) {

• debug("FPGA: Failed to read image header from flash.\n");
  

• return -ENOENT;
  

• }
• if (image_get_magic((struct image_header *)buffer_p) != FDT_MAGIC) {

• debug("FPGA: No FDT magic was found.\n");
  

• return -EBADF;
  

• }
• fit_size = fdt_totalsize(buffer_p);
• if (fit_size > buffer_size) {

• debug("FPGA: FIT image is larger than available buffer.\n");
  

• debug("Please use FIT external data or increasing buffer.\n");
  

• return -ENOMEM;
  

• }
• /* Load entire FIT into buffer */
• ret = request_firmware_into_buf(dev,

• 			fpga_loadfs->fpga_fsinfo->filename,
  

• 			buffer_p,
  

• 			fit_size,
  

• 			0);
  

• if (ret < 0)

• return ret;
  

• ret = fit_check_format(buffer_p);
• if (!ret) {

• debug("FPGA: No valid FIT image was found.\n");
  

• return -EBADF;
  

• }
• confs_noffset = fdt_path_offset(buffer_p, FIT_CONFS_PATH);
• images_noffset = fdt_path_offset(buffer_p, FIT_IMAGES_PATH);
• if (confs_noffset < 0 || images_noffset < 0) {

• debug("FPGA: No Configurations or images nodes were found.\n");
  

• return -ENOENT;
  

• }
• /* Get default configuration unit name from default property */
• confs_noffset = fit_conf_get_node(buffer_p, NULL);
• if (confs_noffset < 0) {

• debug("FPGA: No default configuration was found in config.\n");
  

• return -ENOENT;
  

• }
• count = fit_conf_get_prop_node_count(buffer_p, confs_noffset,

• 			    FIT_FPGA_PROP);
  

• if (count < 0) {

• debug("FPGA: Invalid configuration format for FPGA node.\n");
  

• return count;
  

• } else {

• debug("FPGA: FPGA node count: %d\n", count);
  

• }
• for (i = 0; i < count; i++) {

• images_noffset = fit_conf_get_prop_node_index(buffer_p,
  

• 					     confs_noffset,
  

• 					     FIT_FPGA_PROP, i);
  

• 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;
  

• 	}
  

• }
  

• WATCHDOG_RESET();
  

• }
• if (!fpga_node_name) {

• debug("FPGA: No suitable bitstream was found, count: %d.\n", i);
  

• return 1;
  

• }
• images_noffset = fit_image_get_node(buffer_p, fpga_node_name);
• if (images_noffset < 0) {

• debug("FPGA: No node '%s' was found in FIT.\n",
  

•      fpga_node_name);
  

• return -ENOENT;
  

• }
• ret = fit_image_get_data_position(buffer_p, images_noffset,

• 			 &rbf_offset);
  

• if (ret < 0) {

• debug("FPGA: No data position was found (err=%d).\n", ret);
  

• return -ENOENT;
  

• }
• ret = fit_image_get_data_size(buffer_p, images_noffset, &rbf_size);
• if (ret < 0) {

• debug("FPGA: No data size was found (err=%d).\n", ret);
  

• return -ENOENT;
  

• }
• ret = fit_image_get_load(buffer_p, images_noffset, (ulong *)loadable);
• if (ret < 0) {

• debug("FPGA: No loadable was found (err=%d).\n", ret);
  

• debug("FPGA: Using default buffer and size.\n");
  

• } else {

• buffer_p = (u32 *)*loadable;
  

• buffer_size = rbf_size;
  

• debug("FPGA: Found loadable address = 0x%x.\n", *loadable);
  

• }
• debug("FPGA: External data: offset = 0x%x, size = 0x%x.\n",

•      rbf_offset, rbf_size);
  

• fpga_loadfs->remaining = rbf_size;
• /*

• * Determine buffer size vs bitstream size, and calculating number of
  

• * chunk by chunk transfer is required due to smaller buffer size
  

• * compare to bitstream
  

• */
  

• if (rbf_size <= buffer_size) {

• /* Loading whole bitstream into buffer */
  

• buffer_size = rbf_size;
  

• fpga_loadfs->remaining = 0;
  

• } else {

• fpga_loadfs->remaining -= buffer_size;
  

• }

Shouldn't all this parsing and calculation be done by the firmware loader code ?

• fpga_loadfs->offset = rbf_offset;
• /* Loading bitstream into buffer */
• ret = request_firmware_into_buf(dev,

• 			fpga_loadfs->fpga_fsinfo->filename,
  

• 			buffer_p,
  

• 			buffer_size,
  

• 			fpga_loadfs->offset);
  

• if (ret < 0) {

• debug("FPGA: Failed to read bitstream from flash.\n");
  

• return -ENOENT;
  

• }
• /* Getting info about bitstream types */
• get_rbf_image_info(&fpga_loadfs->rbfinfo, (u16 *)buffer_p);
• /* Update next reading bitstream offset */
• fpga_loadfs->offset += buffer_size;
• /* Update the final addr for bitstream */
• *buffer = (u32)buffer_p;
• /* Update the size of bitstream to be programmed into FPGA */
• *buffer_bsize = buffer_size;
• return 0;

+}

+static int subsequent_loading_rbf_to_buffer(struct udevice *dev,

• 			struct fpga_loadfs_info *fpga_loadfs,
  

• 			u32 *buffer, size_t *buffer_bsize)
  

+{

• int ret = 0;
• u32 *buffer_p = (u32 *)*buffer;
• /* Read the bitstream chunk by chunk. */
• if (fpga_loadfs->remaining > *buffer_bsize) {

• fpga_loadfs->remaining -= *buffer_bsize;
  

• } else {

• *buffer_bsize = fpga_loadfs->remaining;
  

• fpga_loadfs->remaining = 0;
  

• }
• ret = request_firmware_into_buf(dev,

• 			fpga_loadfs->fpga_fsinfo->filename,
  

• 			buffer_p,
  

• 			*buffer_bsize,
  

• 			fpga_loadfs->offset);
  

• if (ret < 0) {

• debug("FPGA: Failed to read bitstream from flash.\n");
  

• return -ENOENT;
  

• }
• /* Update next reading bitstream offset */
• fpga_loadfs->offset += *buffer_bsize;
• return 0;

+}

+int socfpga_loadfs(fpga_fs_info *fpga_fsinfo, const void *buf, size_t bsize,

• 	u32 offset)
  

+{

• struct fpga_loadfs_info fpga_loadfs;
• int status = 0;
• int ret = 0;
• u32 buffer = (u32)buf;

This will fail on arm64 , look at uintptr_t .

• size_t buffer_sizebytes = bsize;
• size_t buffer_sizebytes_ori = bsize;
• size_t total_sizeof_image = 0;
• struct udevice *dev;
• ret = uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0, &dev);

Shouldn't the firmware loaded instance be obtained via the DT phandle ?

• if (ret)

• return ret;
  

• memset(&fpga_loadfs, 0, sizeof(fpga_loadfs));
• fpga_loadfs.fpga_fsinfo = fpga_fsinfo;
• fpga_loadfs.offset = offset;
• printf("FPGA: Checking FPGA configuration setting ...\n");
• /*

• * Note: Both buffer and buffer_sizebytes values can be altered by
  

• * function below.
  

• */
  

• ret = first_loading_rbf_to_buffer(dev, &fpga_loadfs, &buffer,

• 			   &buffer_sizebytes);
  

• if (ret == 1) {

• printf("FPGA: Skipping configuration ...\n");
  

• return 0;
  

• } else if (ret) {

• return ret;
  

• }
• if (fpga_loadfs.rbfinfo.section == core_section &&

• !(is_fpgamgr_early_user_mode() && !is_fpgamgr_user_mode())) {
  

• debug("FPGA : Must be in Early Release mode to program ");
  

• debug("core bitstream.\n");
  

• return 0;
  

• }
• /* Disable all signals from HPS peripheral controller to FPGA */
• writel(0, &system_manager_base->fpgaintf_en_global);
• /* Disable all axi bridges (hps2fpga, lwhps2fpga & fpga2hps) */
• socfpga_bridges_reset();
• if (fpga_loadfs.rbfinfo.section == periph_section) {

• /* Initialize the FPGA Manager */
  

• status = fpgamgr_program_init((u32 *)buffer, buffer_sizebytes);
  

• if (status) {
  

• 	debug("FPGA: Init with peripheral bitstream failed.\n");
  

• 	return -EPERM;
  

• }
  

• }
• /* Transfer bitstream to FPGA Manager */
• fpgamgr_program_write((void *)buffer, buffer_sizebytes);
• total_sizeof_image += buffer_sizebytes;
• while (fpga_loadfs.remaining) {

• ret = subsequent_loading_rbf_to_buffer(dev,
  

• 					&fpga_loadfs,
  

• 					&buffer,
  

• 					&buffer_sizebytes_ori);
  

• if (ret)
  

• 	return ret;
  

• /* Transfer data to FPGA Manager */
  

• fpgamgr_program_write((void *)buffer,
  

• 			buffer_sizebytes_ori);
  

• total_sizeof_image += buffer_sizebytes_ori;
  

• WATCHDOG_RESET();
  

• }
• if (fpga_loadfs.rbfinfo.section == periph_section) {

• if (fpgamgr_wait_early_user_mode() != -ETIMEDOUT) {
  

• 	config_pins(gd->fdt_blob, "shared");
  

• 	puts("FPGA: Early Release Succeeded.\n");
  

• } else {
  

• 	debug("FPGA: Failed to see Early Release.\n");
  

• 	return -EIO;
  

• }
  

• /* For monolithic bitstream */
  

• if (is_fpgamgr_user_mode()) {
  

• 	/* Ensure the FPGA entering config done */
  

• 	status = fpgamgr_program_finish();
  

• 	if (status)
  

• 		return status;
  

• 	config_pins(gd->fdt_blob, "fpga");
  

• 	puts("FPGA: Enter user mode.\n");
  

• }
  

• } else if (fpga_loadfs.rbfinfo.section == core_section) {

• /* Ensure the FPGA entering config done */
  

• status = fpgamgr_program_finish();
  

• if (status)
  

• 	return status;
  

• config_pins(gd->fdt_blob, "fpga");
  

• puts("FPGA: Enter user mode.\n");
  

• } else {

• debug("FPGA: Config Error: Unsupported bitstream type.\n");
  

• return -ENOEXEC;
  

• }
• return (int)total_sizeof_image;

+} +#endif

+/* This function is used to load the core bitstream from the OCRAM. */ int socfpga_load(Altera_desc *desc, const void *rbf_data, size_t rbf_size) {

• int status;

• unsigned long status;
• struct rbf_info rbfinfo;

• /* disable all signals from hps peripheral controller to fpga */

• memset(&rbfinfo, 0, sizeof(rbfinfo));
• /* Disable all signals from hps peripheral controller to fpga */ writel(0, &system_manager_base->fpgaintf_en_global);

• /* disable all axi bridge (hps2fpga, lwhps2fpga & fpga2hps) */

• /* Disable all axi bridge (hps2fpga, lwhps2fpga & fpga2hps) */ socfpga_bridges_reset();

• /* Initialize the FPGA Manager */
• status = fpgamgr_program_init((u32 *)rbf_data, rbf_size);
• if (status)

• return status;
  

• /* Getting info about bitstream types */
• get_rbf_image_info(&rbfinfo, (u16 *)rbf_data);
• if (rbfinfo.section == periph_section) {

• /* Initialize the FPGA Manager */
  

• status = fpgamgr_program_init((u32 *)rbf_data, rbf_size);
  

• if (status)
  

• 	return status;
  

• }
• if (rbfinfo.section == core_section &&

• !(is_fpgamgr_early_user_mode() && !is_fpgamgr_user_mode())) {
  

• debug("FPGA : Must be in early release mode to program ");
  

• debug("core bitstream.\n");
  

• return 0;
  

• }

• /* Write the RBF data to FPGA Manager */

• /* Write the bitstream to FPGA Manager */ fpgamgr_program_write(rbf_data, rbf_size);

• return fpgamgr_program_finish();

• status = fpgamgr_program_finish();
• if (status) {

• config_pins(gd->fdt_blob, "fpga");
  

• puts("FPGA: Enter user mode.\n");
  

• }
• return status;

}

On 2/14/19 7:44 AM, Chee, Tien Fong wrote:

On Wed, 2019-02-13 at 17:20 +0100, Marek Vasut wrote:

...

On 2/13/19 3:18 PM, tien.fong.chee@intel.com wrote:

...

From: Tien Fong Chee tien.fong.chee@intel.com

Add FPGA driver to support program FPGA with FPGA bitstream loading from filesystem. The driver are designed based on generic firmware loader framework. The driver can handle FPGA program operation from loading FPGA bitstream in flash to memory and then to program FPGA.

Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com

---

changes for v8

• Added codes to discern bitstream type based on fpga node name.

changes for v7

• Restructure the FPGA driver to support both peripheral bitstream

and core   bitstream bundled into FIT image.

• Support loadable property for core bitstream. User can set

loadable   in DDR for better performance. This loading would be done in one large   chunk instead of chunk by chunk loading with small memory buffer.

---

arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts       |  17 +  .../include/mach/fpga_manager_arria10.h            |  39 +-  drivers/fpga/socfpga_arria10.c                     | 467 ++++++++++++++++++++-  3 files changed, 500 insertions(+), 23 deletions(-)

diff --git a/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts b/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts index 998d811..14f1967 100644 --- a/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts +++ b/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts @@ -18,6 +18,23 @@  /dts-v1/;  #include "socfpga_arria10_socdk.dtsi"   +/ {

• chosen {

• firmware-loader = &fs_loader0;
  

Should be a phandle.

Can we change this label to phandle stage by stage, may be after fpga driver? This requires time working on firmware loader.

We should fix this as soon as possible, otherwise people might find this bad example and wonder why it doesn't work once this is changed.

...

...

• };
• fs_loader0: fs-loader@0 {

• u-boot,dm-pre-reloc;
  

• compatible = "u-boot,fs-loader";
  

• phandlepart = <&mmc 1>;
  

• };

+};

+&fpga_mgr {

• u-boot,dm-pre-reloc;
• altr,bitstream = "fit_spl_fpga.itb";

+};

&mmc {   u-boot,dm-pre-reloc;   status = "okay";

[...]

...

...

• • FPGA Manager to program the FPGA. This is the interface used by

FPGA driver.

• • Return 0 for sucess, non-zero for error.
• */

+char *get_fpga_filename(const void *fdt, int *len) +{

• char *fpga_filename = NULL;
• int node_offset;
• fdtdec_find_aliases_for_id(gd->fdt_blob, "fpga_mgr",

• 		COMPAT_ALTERA_SOCFPGA_FPGA0,
  

• 		&node_offset, 1);
  

• ofnode fpgamgr_node = offset_to_ofnode(node_offset);
• if (ofnode_valid(fpgamgr_node))

• fpga_filename = (char
  

*)ofnode_read_string(fpgamgr_node,

• 				"altr,bitstream");
  

Why is the cast needed ?

The return string would be eventually set to the char *filename in common struct fpga_fsinfo. So, the cast here is to avoid the warning from compiler.

I presume if the compiler generates a warning, it's for a reason. What warning is that ?

...

Drop the two newlines.

Okay.

...

...

• return fpga_filename;

+}

+static void get_rbf_image_info(struct rbf_info *rbf, u16 *buffer) +{

• /*
• * Magic ID starting at:
• * -> 1st dword[15:0] in periph.rbf
• * -> 2nd dword[15:0] in core.rbf
• * Note: dword == 32 bits
• */
• u32 word_reading_max = 2;
• u32 i;
• for (i = 0; i < word_reading_max; i++) {

• if (*(buffer + i) ==
  

FPGA_SOCFPGA_A10_RBF_UNENCRYPTED) {

• 	rbf->security = unencrypted;
  

• } else if (*(buffer + i) ==
  

FPGA_SOCFPGA_A10_RBF_ENCRYPTED) {

• 	rbf->security = encrypted;
  

• } else if (*(buffer + i + 1) ==
  

• 		FPGA_SOCFPGA_A10_RBF_UNENCRYPTED)
  

{

• 	rbf->security = unencrypted;
  

• } else if (*(buffer + i + 1) ==
  

• 		FPGA_SOCFPGA_A10_RBF_ENCRYPTED) {
  

• 	rbf->security = encrypted;
  

• } else {
  

• 	rbf->security = invalid;
  

• 	continue;
  

• }
  

• /* PERIPH RBF(buffer + i + 1), CORE RBF(buffer + i
  

• 2. */

• if (*(buffer + i + 1) ==
  

FPGA_SOCFPGA_A10_RBF_PERIPH) {

• 	rbf->section = periph_section;
  

• 	break;
  

• } else if (*(buffer + i + 1) ==
  

FPGA_SOCFPGA_A10_RBF_CORE) {

• 	rbf->section = core_section;
  

• 	break;
  

• } else if (*(buffer + i + 2) ==
  

FPGA_SOCFPGA_A10_RBF_PERIPH) {

• 	rbf->section = periph_section;
  

• 	break;
  

• } else if (*(buffer + i + 2) ==
  

FPGA_SOCFPGA_A10_RBF_CORE) {

• 	rbf->section = core_section;
  

• 	break;
  

• }
  

• rbf->section = unknown;
  

• break;
  

• WATCHDOG_RESET();
  

• }

+}

+#ifdef CONFIG_FS_LOADER +static int first_loading_rbf_to_buffer(struct udevice *dev,

• 		struct fpga_loadfs_info
  

*fpga_loadfs,

• 		u32 *buffer, size_t *buffer_bsize)
  

+{

• u32 *buffer_p = (u32 *)*buffer;
• u32 *loadable = buffer_p;
• size_t buffer_size = *buffer_bsize;
• size_t fit_size;
• int ret, i, count;
• int confs_noffset, images_noffset;
• int rbf_offset;
• int rbf_size;
• const char *fpga_node_name = NULL;
• const char *uname = NULL;
• /* Load image header into buffer */
• ret = request_firmware_into_buf(dev,

• 			fpga_loadfs->fpga_fsinfo-
  

...

filename,

• 			buffer_p,
  

• 			sizeof(struct
  

image_header),

• 			0);
  

• if (ret < 0) {

• debug("FPGA: Failed to read image header from
  

flash.\n");

• return -ENOENT;
  

• }
• if (image_get_magic((struct image_header *)buffer_p) !=

FDT_MAGIC) {

• debug("FPGA: No FDT magic was found.\n");
  

• return -EBADF;
  

• }
• fit_size = fdt_totalsize(buffer_p);
• if (fit_size > buffer_size) {

• debug("FPGA: FIT image is larger than available
  

buffer.\n");

• debug("Please use FIT external data or increasing
  

buffer.\n");

• return -ENOMEM;
  

• }
• /* Load entire FIT into buffer */
• ret = request_firmware_into_buf(dev,

• 			fpga_loadfs->fpga_fsinfo-
  

...

filename,

• 			buffer_p,
  

• 			fit_size,
  

• 			0);
  

• if (ret < 0)

• return ret;
  

• ret = fit_check_format(buffer_p);
• if (!ret) {

• debug("FPGA: No valid FIT image was found.\n");
  

• return -EBADF;
  

• }
• confs_noffset = fdt_path_offset(buffer_p, FIT_CONFS_PATH);
• images_noffset = fdt_path_offset(buffer_p,

FIT_IMAGES_PATH);

• if (confs_noffset < 0 || images_noffset < 0) {

• debug("FPGA: No Configurations or images nodes
  

were found.\n");

• return -ENOENT;
  

• }
• /* Get default configuration unit name from default

property */

• confs_noffset = fit_conf_get_node(buffer_p, NULL);
• if (confs_noffset < 0) {

• debug("FPGA: No default configuration was found in
  

config.\n");

• return -ENOENT;
  

• }
• count = fit_conf_get_prop_node_count(buffer_p,

confs_noffset,

• 			    FIT_FPGA_PROP);
  

• if (count < 0) {

• debug("FPGA: Invalid configuration format for FPGA
  

node.\n");

• return count;
  

• } else {

• debug("FPGA: FPGA node count: %d\n", count);
  

• }
• for (i = 0; i < count; i++) {

• images_noffset =
  

fit_conf_get_prop_node_index(buffer_p,

• 					     confs
  

_noffset,

• 					     FIT_F
  

PGA_PROP, i);

• 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_mod
  

e() &&

• 			!is_fpgamgr_user_mode()))
  

{

• 		fpga_node_name = uname;
  

• 		printf("FPGA: Start to program
  

core ");

• 		printf("bitstream ...\n");
  

• 		break;
  

• 	}
  

• }
  

• WATCHDOG_RESET();
  

• }
• if (!fpga_node_name) {

• debug("FPGA: No suitable bitstream was found,
  

count: %d.\n", i);

• return 1;
  

• }
• images_noffset = fit_image_get_node(buffer_p,

fpga_node_name);

• if (images_noffset < 0) {

• debug("FPGA: No node '%s' was found in FIT.\n",
  

•      fpga_node_name);
  

• return -ENOENT;
  

• }
• ret = fit_image_get_data_position(buffer_p,

images_noffset,

• 			 &rbf_offset);
  

• if (ret < 0) {

• debug("FPGA: No data position was found
  

(err=%d).\n", ret);

• return -ENOENT;
  

• }
• ret = fit_image_get_data_size(buffer_p, images_noffset,

&rbf_size);

• if (ret < 0) {

• debug("FPGA: No data size was found (err=%d).\n",
  

ret);

• return -ENOENT;
  

• }
• ret = fit_image_get_load(buffer_p, images_noffset, (ulong

*)loadable);

• if (ret < 0) {

• debug("FPGA: No loadable was found (err=%d).\n",
  

ret);

• debug("FPGA: Using default buffer and size.\n");
  

• } else {

• buffer_p = (u32 *)*loadable;
  

• buffer_size = rbf_size;
  

• debug("FPGA: Found loadable address = 0x%x.\n",
  

*loadable);

• }
• debug("FPGA: External data: offset = 0x%x, size =

0x%x.\n",

• rbf_offset, rbf_size);
• fpga_loadfs->remaining = rbf_size;
• /*
• * Determine buffer size vs bitstream size, and

calculating number of

• * chunk by chunk transfer is required due to smaller

buffer size

• * compare to bitstream
• */
• if (rbf_size <= buffer_size) {

• /* Loading whole bitstream into buffer */
  

• buffer_size = rbf_size;
  

• fpga_loadfs->remaining = 0;
  

• } else {

• fpga_loadfs->remaining -= buffer_size;
  

• }

Shouldn't all this parsing and calculation be done by the firmware loader code ?

The calculation here is to determine the available memory size can be used, it could be size from OCRAM buffer, or DDR.

It seems rather that the code is parsing the fitImage structures ? How is that related to such calculations ?

...

...

• fpga_loadfs->offset = rbf_offset;
• /* Loading bitstream into buffer */
• ret = request_firmware_into_buf(dev,

• 			fpga_loadfs->fpga_fsinfo-
  

...

filename,

• 			buffer_p,
  

• 			buffer_size,
  

• 			fpga_loadfs->offset);
  

• if (ret < 0) {

• debug("FPGA: Failed to read bitstream from
  

flash.\n");

• return -ENOENT;
  

• }
• /* Getting info about bitstream types */
• get_rbf_image_info(&fpga_loadfs->rbfinfo, (u16

*)buffer_p);

• /* Update next reading bitstream offset */
• fpga_loadfs->offset += buffer_size;
• /* Update the final addr for bitstream */
• *buffer = (u32)buffer_p;
• /* Update the size of bitstream to be programmed into FPGA

*/

• *buffer_bsize = buffer_size;
• return 0;

+}

+static int subsequent_loading_rbf_to_buffer(struct udevice *dev,

• 			struct fpga_loadfs_info
  

*fpga_loadfs,

• 			u32 *buffer, size_t
  

*buffer_bsize) +{

• int ret = 0;
• u32 *buffer_p = (u32 *)*buffer;
• /* Read the bitstream chunk by chunk. */
• if (fpga_loadfs->remaining > *buffer_bsize) {

• fpga_loadfs->remaining -= *buffer_bsize;
  

• } else {

• *buffer_bsize = fpga_loadfs->remaining;
  

• fpga_loadfs->remaining = 0;
  

• }
• ret = request_firmware_into_buf(dev,

• 			fpga_loadfs->fpga_fsinfo-
  

...

filename,

• 			buffer_p,
  

• 			*buffer_bsize,
  

• 			fpga_loadfs->offset);
  

• if (ret < 0) {

• debug("FPGA: Failed to read bitstream from
  

flash.\n");

• return -ENOENT;
  

• }
• /* Update next reading bitstream offset */
• fpga_loadfs->offset += *buffer_bsize;
• return 0;

+}

+int socfpga_loadfs(fpga_fs_info *fpga_fsinfo, const void *buf, size_t bsize,

• 	u32 offset)
  

+{

• struct fpga_loadfs_info fpga_loadfs;
• int status = 0;
• int ret = 0;
• u32 buffer = (u32)buf;

This will fail on arm64 , look at uintptr_t .

This driver is only used by A10 which is arm32. You want me to use (u32)(uintptr_t)buf?

If you want to cast pointer to integer type, yes, that's uintptr_t .

...

...

• size_t buffer_sizebytes = bsize;
• size_t buffer_sizebytes_ori = bsize;
• size_t total_sizeof_image = 0;
• struct udevice *dev;
• ret = uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0,

&dev);

Shouldn't the firmware loaded instance be obtained via the DT phandle ?

It just to get the device activated. The firmware loaded itself would go to choosen node(default) for getting the label. I can change to the phandle, may be after this?

Except this always activates the first device in the list. [...]

On 2/13/19 3:18 PM, tien.fong.chee@intel.com wrote:

From: Tien Fong Chee tien.fong.chee@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@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.altera.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()

• /* 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 ...

• }
• /* If the IOSSM/full FPGA is already loaded, start DDR */
• if (is_fpgamgr_early_user_mode() || is_fpgamgr_user_mode())

• ddr_calibration_sequence();
  

• 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);
  

• }

}

void board_init_f(ulong dummy)

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@intel.com wrote:

...

From: Tien Fong Chee tien.fong.chee@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@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.altera.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.

...

...

/* 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 ?

...

...

• }
• /* If the IOSSM/full FPGA is already loaded, start DDR */
• if (is_fpgamgr_early_user_mode() ||

is_fpgamgr_user_mode())

• ddr_calibration_sequence();
  

• 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);

• }

}    void board_init_f(ulong dummy)