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

[Marek Vasut]

On 09/27/2017 08:05 AM, Chee, Tien Fong wrote:
> On Sel, 2017-09-26 at 12:32 +0200, Marek Vasut wrote:
>> 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 ...
>>
> No complaint from checkpath. I know someone saying bad for readbility,
> but yoda-notation at this simple implementation doesn't impact the
> readbility, and having benefit to leverage detection of compiler on
> missing "=". Overall, this can help to improve coding quality. I can
> remove it if this doesn't favored in U-boot.

It is not welcome and modern gcc warns you about such things .

>>>
>>>>
>>>>>
>>>>>
>>>>> ---
>>>>>  .../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_NS
>>>>> TATU
>>>>> 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 ...
>>
> Disucss in another email reply separately. I need time to think.
>>>
>>>>
>>>> [...]
>>>>
>>>>>
>>>>>
>>>>> 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.
>>
> I forgot what reason i put this value. I need to find out.

If this was properly documented in the commit message, you would not
have to ...

>>>
>>>>
>>>>>
>>>>>
>>>>>  #define CONFIG_SYS_MEMTEST_START	PHYS_SDRAM_1
>>>>>  #define CONFIG_SYS_MEMTEST_END		PHYS_SDRAM_1_SIZ
>>>>> E
>>>>>  #if defined(CONFIG_TARGET_SOCFPGA_GEN5)
>>>>>


-- 
Best regards,
Marek Vasut