[U-Boot] [PATCH v2 09/19] arm: socfpga: Add drivers for programing FPGA from flash

[Marek Vasut]

On 09/26/2017 10:30 AM, Chee, Tien Fong wrote:
> On Isn, 2017-09-25 at 11:14 +0200, Marek Vasut wrote:
>> On 09/25/2017 10:40 AM, tien.fong.chee at intel.com wrote:
>>>
>>> From: Tien Fong Chee <tien.fong.chee at intel.com>
>>>
>>> These drivers handle FPGA program operation from flash loading
>>> RBF to memory and then to program FPGA.
>>>
>>> Signed-off-by: Tien Fong Chee <tien.fong.chee at intel.com>
>> Did you run checkpatch on this before submitting ? I presume no ...
>>
> Yeah, i run checkpatch for all patches. What's the issue here?

It should definitely indicate problem with ie. yoda-notation
+if (0 == flashinfo->remaining) {
and indent ...

>>>
>>> ---
>>>  .../include/mach/fpga_manager_arria10.h            |  27 ++
>>>  drivers/fpga/socfpga_arria10.c                     | 391
>>> ++++++++++++++++++++-
>>>  include/altera.h                                   |   6 +
>>>  include/configs/socfpga_common.h                   |   4 +
>>>  4 files changed, 425 insertions(+), 3 deletions(-)
>> [...]
>>
>>>
>>> @@ -112,13 +122,14 @@ static int
>>> wait_for_nconfig_pin_and_nstatus_pin(void)
>>>  	unsigned long mask =
>>> ALT_FPGAMGR_IMGCFG_STAT_F2S_NCONFIG_PIN_SET_MSK |
>>>  				ALT_FPGAMGR_IMGCFG_STAT_F2S_NSTATU
>>> S_PIN_SET_MSK;
>>>  
>>> -	/* Poll until f2s_nconfig_pin and f2s_nstatus_pin; loop
>>> until de-asserted,
>>> -	 * timeout at 1000ms
>>> +	/*
>>> +	 * Poll until f2s_nconfig_pin and f2s_nstatus_pin; loop
>>> until
>>> +	 * de-asserted, timeout at 1000ms
>>>  	 */
>>>  	return wait_for_bit(__func__,
>>>  			    &fpga_manager_base->imgcfg_stat,
>>>  			    mask,
>>> -			    false, FPGA_TIMEOUT_MSEC, false);
>>> +			    true, FPGA_TIMEOUT_MSEC, false);
>>>  }
>> Seems more like a fix, split this out.
>>
> Okay.
>>>
>>>  static int wait_for_f2s_nstatus_pin(unsigned long value)
>>> @@ -469,6 +480,7 @@ int socfpga_load(Altera_desc *desc, const void
>>> *rbf_data, size_t rbf_size)
>>>  
>>>  	/* Initialize the FPGA Manager */
>>>  	status = fpgamgr_program_init((u32 *)rbf_data, rbf_size);
>>> +
>>>  	if (status)
>>>  		return status;
>>>  
>>> @@ -477,3 +489,376 @@ int socfpga_load(Altera_desc *desc, const
>>> void *rbf_data, size_t rbf_size)
>>>  
>>>  	return fpgamgr_program_finish();
>>>  }
>>> +
>>> +#if defined(CONFIG_CMD_FPGA_LOADFS)
>>> +const char *get_cff_filename(const void *fdt, int *len, u32 core)
>>> +{
>>> +	const char *cff_filename = NULL;
>>> +	const char *cell;
>>> +	int nodeoffset;
>>> +	nodeoffset = fdtdec_next_compatible(fdt, 0,
>>> +			 COMPAT_ALTERA_SOCFPGA_FPGA0);
>>> +
>>> +	if (nodeoffset >= 0) {
>>> +		if (core)
>>> +			cell = fdt_getprop(fdt,
>>> +					nodeoffset,
>>> +					"bitstream_core",
>>> +					len);
>>> +		else
>>> +			cell = fdt_getprop(fdt, nodeoffset,
>>> "bitstream_periph",
>>> +					 len);
>>> +
>>> +		if (cell)
>>> +			cff_filename = cell;
>>> +	}
>>> +
>>> +	return cff_filename;
>>> +}
>>> +
>>> +const char *get_cff_devpart(const void *fdt, int *len)
>>> +{
>>> +	const char *cff_devpart = NULL;
>>> +	const char *cell;
>>> +	int nodeoffset;
>>> +	nodeoffset = fdtdec_next_compatible(fdt, 0,
>>> +			 COMPAT_ALTERA_SOCFPGA_FPGA0);
>>> +
>>> +	cell = fdt_getprop(fdt, nodeoffset, "bitstream_devpart",
>>> len);
>>> +
>>> +	if (cell)
>>> +		cff_devpart = cell;
>>> +
>>> +	return cff_devpart;
>>> +}
>> Take a look at splash*.c , I believe that can be reworked into
>> generic
>> firmware loader , which you could then use here.

This is important here, I don't want yet another ad-hoc loader ...

>> [...]
>>
>>>
>>> diff --git a/include/configs/socfpga_common.h
>>> b/include/configs/socfpga_common.h
>>> index 9897e11..eadce2d 100644
>>> --- a/include/configs/socfpga_common.h
>>> +++ b/include/configs/socfpga_common.h
>>> @@ -27,7 +27,11 @@
>>>   */
>>>  #define CONFIG_NR_DRAM_BANKS		1
>>>  #define PHYS_SDRAM_1			0x0
>>> +#if defined(CONFIG_TARGET_SOCFPGA_GEN5)
>>>  #define CONFIG_SYS_MALLOC_LEN		(64 * 1024 * 1024)
>>> +#elif defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
>>> +#define CONFIG_SYS_MALLOC_LEN		(128 * 1024 * 1024)
>>> +#endif
>> You definitely don't need 128 MiB of malloc area.
>>
> Okay, i will try out with smaller size.

Why do you need such massive area ? It's not a matter of "try out", you
should know why this change was needed for your use-case.

>>>
>>>  #define CONFIG_SYS_MEMTEST_START	PHYS_SDRAM_1
>>>  #define CONFIG_SYS_MEMTEST_END		PHYS_SDRAM_1_SIZE
>>>  #if defined(CONFIG_TARGET_SOCFPGA_GEN5)
>>>


-- 
Best regards,
Marek Vasut