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

[Marek Vasut]

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


-- 
Best regards,
Marek Vasut