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

[Marek Vasut]

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.

>>
>>>
>>>>
>>>>>
>>>>>
>>>>> +	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.com>
>>>>>   */
>>>>>  
>>>>>  #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

same for ... _CORE ... is that a coincidence ?

[...]

-- 
Best regards,
Marek Vasut