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

[Chee, Tien Fong <tien.fong.chee@intel.com>]

On Rab, 2017-09-27 at 10:30 +0200, Marek Vasut wrote:
> 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@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@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 .
> 
Okay.
> > 
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > ---
> > > > > >  .../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_F2
> > > > > > S_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 ...
> 
Okay.
> > 
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > >  #define CONFIG_SYS_MEMTEST_START	PHYS_SDRAM_1
> > > > > >  #define CONFIG_SYS_MEMTEST_END		PHYS_SDRAM_1
> > > > > > _SIZ
> > > > > > E
> > > > > >  #if defined(CONFIG_TARGET_SOCFPGA_GEN5)
> > > > > > 
>