[U-Boot] [PATCH 2/9] ARM: socfpga: Add FPGA drivers for Arria 10 FPGA bitstream loading

[Chee, Tien Fong]

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 <www.intel.com
> > > > > > > > >
> > > > > > > > + * Copyright (C) 2017-2018 Intel Corporation <www.inte
> > > > > > > > l.co
> > > > > > > > m>
> > > > > > > >   */
> > > > > > > >  
> > > > > > > >  #include <asm/io.h>
> > > > > > > > @@ -10,8 +10,10 @@
> > > > > > > >  #include <asm/arch/sdram.h>
> > > > > > > >  #include <asm/arch/misc.h>
> > > > > > > >  #include <altera.h>
> > > > > > > > +#include <asm/arch/pinmux.h>
> > > > > > > >  #include <common.h>
> > > > > > > >  #include <errno.h>
> > > > > > > > +#include <fs_loader.h>
> > > > > > > >  #include <wait_bit.h>
> > > > > > > >  #include <watchdog.h>
> > > > > > > >  
> > > > > > > > @@ -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 ?
> > > 
> > > [...]
>