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.intel.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 ?
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 ?
[...]