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

[Marek Vasut]

On 11/28/2018 03:53 PM, Chee, Tien Fong wrote:
> 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.

If it can deduplicate things, that'd be good.

-- 
Best regards,
Marek Vasut