From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chee, Tien Fong Date: Wed, 28 Nov 2018 14:53:47 +0000 Subject: [U-Boot] [PATCH 2/9] ARM: socfpga: Add FPGA drivers for Arria 10 FPGA bitstream loading In-Reply-To: <739d3409-15cf-742b-01d0-3a0b6194d8bf@denx.de> References: <1542796908-7947-1-git-send-email-tien.fong.chee@intel.com> <1542796908-7947-3-git-send-email-tien.fong.chee@intel.com> <23b93c57-daf8-78e0-684f-2fe006ba8e45@denx.de> <1542966198.10129.20.camel@intel.com> <64c191cb-925e-f4c8-df75-efc81ff3d656@denx.de> <1543226946.10014.11.camel@intel.com> <4f54cd3c-5231-c052-bb47-b2cb33b65c9d@denx.de> <1543308878.10323.14.camel@intel.com> <739d3409-15cf-742b-01d0-3a0b6194d8bf@denx.de> Message-ID: <1543416820.20584.8.camel@intel.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de On Tue, 2018-11-27 at 13:08 +0100, Marek Vasut wrote: > On 11/27/2018 09:54 AM, Chee, Tien Fong wrote: > > > > On Mon, 2018-11-26 at 12:18 +0100, Marek Vasut wrote: > > > > > > On 11/26/2018 11:09 AM, Chee, Tien Fong wrote: > > > [...] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/fpga/Kconfig > > > > > > > > b/drivers/fpga/Kconfig > > > > > > > > index 50e9019..06a8204 100644 > > > > > > > > --- a/drivers/fpga/Kconfig > > > > > > > > +++ b/drivers/fpga/Kconfig > > > > > > > > @@ -21,6 +21,15 @@ config FPGA_SOCFPGA > > > > > > > >   > > > > > > > >     This provides common functionality for Gen5 > > > > > > > > and > > > > > > > > Arria10 > > > > > > > > devices. > > > > > > > >   > > > > > > > > +config CHECK_FPGA_DATA_CRC > > > > > > > config FPGA_SOCFPGA_A10_CRC_CHECK > > > > > > > > > > > > > > What is this for and why shouldn't this be ON by default > > > > > > > ? > > > > > > Both periph.rbf or full.rbf are wrapped with mkimage. So, > > > > > > this > > > > > > CRC > > > > > > checking can be used to check the integrity of the loading > > > > > > bitstream > > > > > > data against checksum from mkimage. It is off for the sake > > > > > > of > > > > > > performance. > > > > > Is there a measurable performance degradation ? I presume > > > > > that's > > > > > because > > > > > caches are disabled at that point, yes? Enable caches and see > > > > > if > > > > > that > > > > > helps. > > > > Just logical sense, performance sure getting degraded, > > > > especially > > > > loading full rbf, but may be not that obvious for periph.rbf > > > > because of > > > > very small size, i can try to measure. If not much difference, > > > > i > > > > can > > > > enable checking in default. > > > Hard numbers are the only relevant argument here, please measure. > > > And try it with caches enabled as much as possible, you want > > > users to > > > boot fast. Arria10 is particularly annoyingly slow at booting. > > sure. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + bool "Enable CRC cheking on Arria10 FPGA > > > > > > > > bistream" > > > > > > > > + depends on FPGA_SOCFPGA > > > > > > > > + help > > > > > > > > +  Say Y here to enable the CRC checking on > > > > > > > > Arria 10 > > > > > > > > FPGA > > > > > > > > bitstream > > > > > > > > + > > > > > > > > +  This provides CRC checking to ensure > > > > > > > > integrated > > > > > > > > of > > > > > > > > Arria > > > > > > > > 10 FPGA > > > > > > > > +  bitstream is programmed into FPGA. > > > > > > > > + > > > > > > > >  config FPGA_CYCLON2 > > > > > > > >   bool "Enable Altera FPGA driver for Cyclone > > > > > > > > II" > > > > > > > >   depends on FPGA_ALTERA > > > > > > > > diff --git a/drivers/fpga/socfpga_arria10.c > > > > > > > > b/drivers/fpga/socfpga_arria10.c > > > > > > > > index 114dd91..d9ad237 100644 > > > > > > > > --- a/drivers/fpga/socfpga_arria10.c > > > > > > > > +++ b/drivers/fpga/socfpga_arria10.c > > > > > > > > @@ -1,6 +1,6 @@ > > > > > > > >  // SPDX-License-Identifier: GPL-2.0 > > > > > > > >  /* > > > > > > > > - * Copyright (C) 2017 Intel Corporation > > > > > > > > > > > > > > > > + * Copyright (C) 2017-2018 Intel Corporation > > > > > > > l.co > > > > > > > > m> > > > > > > > >   */ > > > > > > > >   > > > > > > > >  #include > > > > > > > > @@ -10,8 +10,10 @@ > > > > > > > >  #include > > > > > > > >  #include > > > > > > > >  #include > > > > > > > > +#include > > > > > > > >  #include > > > > > > > >  #include > > > > > > > > +#include > > > > > > > >  #include > > > > > > > >  #include > > > > > > > >   > > > > > > > > @@ -21,6 +23,10 @@ > > > > > > > >  #define COMPRESSION_OFFSET 229 > > > > > > > >  #define FPGA_TIMEOUT_MSEC 1000  /* timeout in > > > > > > > > ms */ > > > > > > > >  #define FPGA_TIMEOUT_CNT 0x1000000 > > > > > > > > +#define RBF_UNENCRYPTED 0xa65c > > > > > > > > +#define RBF_ENCRYPTED 0xa65d > > > > > > > > +#define ARRIA10RBF_PERIPH 0x0001 > > > > > > > > +#define ARRIA10RBF_CORE 0x8001 > > > > > > > This looks awfully similar to the PERIPH_RBF and CORE_RBF > > > > > > > above. > > > > > > PERIPH_RBF and CORE_RBF are the flags, so i can change them > > > > > > to > > > > > > enum. > > > > > > But above #define are magic content used to identify the > > > > > > bistream > > > > > > type. > > > > > > If above #define are not suitable, what can you suggest? > > > > > Maybe you can just align those two to avoid duplication ? > > > > What's you means with duplication, they are different thing. > > > > How about i change the name to ARRIA10RBF_PERIPH_TYPE > > > > and ARRIA10RBF_CORE_TYPE. > > > ARRIA10RBF_PERIPH = (PERIPH_RBF << 15) | 1 > > We can't use the flag PERIPH_RBF(similar TRUE/FALSE) for magic > > content, > > because they are not related each other. Magic content is defined > > by HW > > design. > You can define the flags to match the HW design, which is probably a > good idea ? I have no strong opinion of this, i can do it. > > > > > We identify the type of rbf such as periph, and core through this > > magic > > content within the rbf. After we getting the type, then only we > > setting > > the flag such as PERIPH_RBF to the function. > > > > > > > > > same for ... _CORE ... is that a coincidence ? > > > > > > [...] >