All of lore.kernel.org
 help / color / mirror / Atom feed
From: Marek Vasut <marex@denx.de>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH 03/19] arm: socfpga: Add driver for flash to program FPGA
Date: Wed, 30 Aug 2017 10:52:31 +0200	[thread overview]
Message-ID: <be365cf8-5ee0-635d-8489-5cb9180cf2dc@denx.de> (raw)
In-Reply-To: <1504080328.7727.37.camel@intel.com>

On 08/30/2017 10:05 AM, Chee, Tien Fong wrote:
> On Sel, 2017-08-29 at 13:55 +0200, Marek Vasut wrote:
>> On 08/29/2017 12:45 PM, tien.fong.chee at intel.com wrote:
>>>
>>> From: Tien Fong Chee <tien.fong.chee@intel.com>
>>>
>>> This driver handles 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>
>>> ---
>>>  .../include/mach/fpga_manager_arria10.h            |   27 ++
>>>  drivers/fpga/socfpga_arria10.c                     |  386
>>> +++++++++++++++++++-
>>>  include/altera.h                                   |    6 +
>>>  include/configs/socfpga_common.h                   |    4 +
>>>  4 files changed, 422 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-
>>> socfpga/include/mach/fpga_manager_arria10.h b/arch/arm/mach-
>>> socfpga/include/mach/fpga_manager_arria10.h
>>> index 9cbf696..93a9122 100644
>>> --- a/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h
>>> +++ b/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h
>>> @@ -8,6 +8,8 @@
>>>  #ifndef _FPGA_MANAGER_ARRIA10_H_
>>>  #define _FPGA_MANAGER_ARRIA10_H_
>>>  
>>> +#include <asm/cache.h>
>>> +
>>>  #define ALT_FPGAMGR_IMGCFG_STAT_F2S_CRC_ERROR_SET_MSK		
>>> BIT(0)
>>>  #define ALT_FPGAMGR_IMGCFG_STAT_F2S_EARLY_USERMODE_SET_MSK	
>>> BIT(1)
>>>  #define ALT_FPGAMGR_IMGCFG_STAT_F2S_USERMODE_SET_MSK 		
>>> BIT(2)
>>> @@ -89,11 +91,36 @@ struct socfpga_fpga_manager {
>>>  	u32  imgcfg_fifo_status;
>>>  };
>>>  
>>> +#if defined(CONFIG_CMD_FPGA_LOADFS)
>>> +enum rbf_type {unknown, periph_section, core_section};
>>> +enum rbf_security {invalid, unencrypted, encrypted};
>>> +
>>> +struct rbf_info {
>>> +	enum rbf_type section;
>>> +	enum rbf_security security;
>>> +};
>>> +
>>> +struct flash_info {
>>> +	char *interface;
>>> +	char *dev_part;
>>> +	char *filename;
>>> +	int fstype;
>>> +	u32 remaining;
>>> +	u32 flash_offset;
>>> +	struct rbf_info rbfinfo;
>>> +	struct image_header header;
>>> +};
>>> +#endif
>>> +
>>>  /* Functions */
>>>  int fpgamgr_program_init(u32 * rbf_data, size_t rbf_size);
>>>  int fpgamgr_program_finish(void);
>>>  int is_fpgamgr_user_mode(void);
>>>  int fpgamgr_wait_early_user_mode(void);
>>> +#if defined(CONFIG_CMD_FPGA_LOADFS)
>>> +const char *get_cff_filename(const void *fdt, int *len, u32 core);
>>> +const char *get_cff_devpart(const void *fdt, int *len);
>>> +#endif
>>>  
>>>  #endif /* __ASSEMBLY__ */
>>>  
>>> diff --git a/drivers/fpga/socfpga_arria10.c
>>> b/drivers/fpga/socfpga_arria10.c
>>> index 5c1a68a..90c55e5 100644
>>> --- a/drivers/fpga/socfpga_arria10.c
>>> +++ b/drivers/fpga/socfpga_arria10.c
>>> @@ -13,6 +13,12 @@
>>>  #include <altera.h>
>>>  #include <common.h>
>>>  #include <errno.h>
>>> +#include <fat.h>
>>> +#include <fs.h>
>>> +#include <fdtdec.h>
>>> +#include <malloc.h>
>>> +#include <part.h>
>>> +#include <spl.h>
>>>  #include <wait_bit.h>
>>>  #include <watchdog.h>
>>>  
>>> @@ -22,6 +28,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
>>>  
>>>  DECLARE_GLOBAL_DATA_PTR;
>>>  
>>> @@ -118,7 +128,7 @@ static int
>>> wait_for_nconfig_pin_and_nstatus_pin(void)
>>>  	return wait_for_bit(__func__,
>>>  			    &fpga_manager_base->imgcfg_stat,
>>>  			    mask,
>>> -			    false, FPGA_TIMEOUT_MSEC, false);
>>> +			    true, FPGA_TIMEOUT_MSEC, false);
>>>  }
>>>  
>>>  static int wait_for_f2s_nstatus_pin(unsigned long value)
>>> @@ -453,6 +463,281 @@ int fpgamgr_program_finish(void)
>>>  	return 0;
>>>  }
>>>  
>>> +#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 = fdt_subnode_offset(fdt, 0, "chosen");
>>> +
>>> +	if (nodeoffset >= 0) {
>>> +		if (core)
>>> +			cell = fdt_getprop(fdt,
>>> +					nodeoffset,
>>> +					"cffcore-file",
>>> +					len);
>>> +		else
>>> +			cell = fdt_getprop(fdt, nodeoffset, "cff-
>>> file", len);
>> This should be a property of the FPGA , not the system . You can have
>> multiple FPGAs and then this would become a problem.
>>
> This setting is for the only one FPGA inside our SoCFPGA.

You just said it yourself, it is for the only FPGA in your SOCFPGA ,
thus it is a property of the FPGA , not a chosen .

> For external
> multiple FPGAs programming, user is adviced to store the FPGA filename
> in environment variable and programming FPGA with fpga loadfs command.
> 
> Please note that, peripheral rbf and partition are required in SPL to
> set up DDR before booting to U-boot.
> 
>>>
>>> +
>>> +		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 = fdt_subnode_offset(fdt, 0, "chosen");
>>> +
>>> +		cell = fdt_getprop(fdt, nodeoffset, "cff_devpart",
>>> len);
>> Indent ? What is this new undocumented DT node about ?
>>
> You can look the dtbinding doc on patch 12.

Ugh, the patch should be in the series before this one.

>>>
>>> +		if (cell)
>>> +			cff_devpart = cell;
>>> +
>>> +	return cff_devpart;
>>> +}
>>> +
>>> +void get_rbf_image_info(struct rbf_info *rbf, u16 *buffer)
>>> +{
>>> +	/*
>>> +	  Magic ID starting at:
>>> +	   -> 1st dword in periph.rbf
>>> +	   -> 2nd dword in core.rbf
>>> +	*/
>> Checkpatch should complain about incorrect multiline comment style
>> here ...
>>
> Okay. I will fix that.

Can you please run checkpatch before submitting your patches ?

>>>
>>> +	u32 word_reading_max = 2;
>>> +	u32 i;
>>> +
>>> +	for(i = 0; i < word_reading_max; i++)
>>> +	{
>>> +		if(RBF_UNENCRYPTED == *(buffer + i)) /* PERIPH RBF
>>> */
>>> +			rbf->security = unencrypted;
>>> +		else if (RBF_ENCRYPTED == *(buffer + i))
>>> +			rbf->security = encrypted;
>>> +		else if (RBF_UNENCRYPTED == *(buffer + i + 1)) /*
>>> CORE RBF */
>>> +					rbf->security =
>>> unencrypted;
>>> +		else if (RBF_ENCRYPTED == *(buffer + i + 1))
>>> +					rbf->security = encrypted;
>>> +		else {
>>> +			rbf->security = invalid;
>>> +			continue;
>>> +		}
>>> +
>>> +		/* PERIPH RBF */
>>> +		if (ARRIA10RBF_PERIPH == *(buffer + i + 1)) {
>>> +			rbf->section = periph_section;
>>> +			break;
>>> +		}
>>> +		else if (ARRIA10RBF_CORE == *(buffer + i + 1)) {
>>> +			rbf->section = core_section;
>>> +			break;
>>> +		} /* CORE RBF */
>>> +		else if (ARRIA10RBF_PERIPH == *(buffer + i + 2)) {
>>> +			rbf->section = periph_section;
>>> +			break;
>>> +		}
>>> +		else if (ARRIA10RBF_CORE == *(buffer + i + 2)) {
>>> +			rbf->section = core_section;
>>> +			break;
>>> +		}
>>> +		else {
>> } else { ... coding style ...
>>
> Okay, i will fix that. This apply to else if?
>>>
>>> +			rbf->section = unknown;
>>> +			break;
>>> +		}
>>> +	}
>>> +
>>> +	return;
>>> +}
>>> +
>>> +static int flash_read(struct flash_info *flashinfo,
>>> +	u32 size_read,
>>> +	u32 *buffer_ptr)
>>> +{
>>> +	size_t ret = EEXIST;
>>> +	loff_t actread = 0;
>>> +
>>> +#ifdef CONFIG_FS_FAT
>>> +		ret = fat_read_file(flashinfo->filename,
>>> +				buffer_ptr, flashinfo-
>>>> flash_offset,
>>> +				 size_read, &actread);
>>> +#endif
>> How can a generic FPGA driver depend on random FS functionality ?
>> This is broken ...
>>
> random FS? There would having FAT FS for SDMMC, and UBI FS for QSPI and
> NAND(implement later).

Driver should not depend on specific FS.

> May be i can replace #ifdef CONFIG_FS_FAT witht
> the codes below, what do you think?
> 	bootdev.boot_device = spl_boot_device();
> 
> 	if(BOOT_DEVICE_MMC1 == bootdev.boot_device)
> 	{ ... }

Nor should it depend on random boot device.

>>>
>>> +		if (ret || actread != size_read) {
>>> +			printf("Failed to read %s from flash %d ",
>>> +				flashinfo->filename,
>>> +				 ret);
>>> +			printf("!= %d.\n", size_read);
>>> +			return -EPERM;
>>> +		} else
>>> +			ret = actread;
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int fs_flash_preinit(struct flash_info *flashinfo,
>>> +	u32 *buffer, u32 *buffer_sizebytes)
>> Is this an FPGA driver or MTD driver ?
>>
> This is FPGA driver. Reading header of rbf, getting filesize and
> adjusting the beginning offset of RBF. 

Why do you pass around the struct flashinfo then ?

[...]

>>> diff --git a/include/configs/socfpga_common.h
>>> b/include/configs/socfpga_common.h
>>> index 9be9e79..c15d244 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
>> 128 MiB malloc area is nonsense, even those 64 MiB are iffy. Why
>> would
>> you ever need that in a bootloader ?
>>
> This is min require to malloc the buffer in SDRAM for core rbf. Less
> than this value, something would going wrong, i'm not recall what issue
> because i already tested this quite long time ago, problem related to
> FAT and malloc.

Isn't the loadfs supposed to read the data from the FS and program them
into the FPGA without having to read the whole bitstream into the RAM ?

>>>
>>>  #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

  reply	other threads:[~2017-08-30  8:52 UTC|newest]

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-29 10:45 [U-Boot] [PATCH 00/19] Add FPGA, SDRAM drivers and booting to U-boot tien.fong.chee at intel.com
2017-08-29 10:45 ` [U-Boot] [PATCH 01/19] configs: Add FPGA loadfs config for Arria 10 tien.fong.chee at intel.com
2017-08-29 11:51   ` Marek Vasut
2017-08-30  5:59     ` Chee, Tien Fong
2017-08-30  8:45       ` Marek Vasut
2017-09-04  5:29         ` Chee, Tien Fong
2017-08-29 10:45 ` [U-Boot] [PATCH 02/19] configs: Add FAT fs support for SPL tien.fong.chee at intel.com
2017-08-29 10:45 ` [U-Boot] [PATCH 03/19] arm: socfpga: Add driver for flash to program FPGA tien.fong.chee at intel.com
2017-08-29 11:55   ` Marek Vasut
2017-08-30  8:05     ` Chee, Tien Fong
2017-08-30  8:52       ` Marek Vasut [this message]
2017-09-04  7:08         ` Chee, Tien Fong
2017-09-04  9:39           ` Marek Vasut
2017-09-05  5:53             ` Chee, Tien Fong
2017-09-05  9:04               ` Marek Vasut
2017-09-05  9:23                 ` Chee, Tien Fong
2017-09-05  9:36                   ` Marek Vasut
2017-09-06  5:06                     ` Chee, Tien Fong
2017-09-06  7:10                       ` Marek Vasut
2017-09-06  7:15                         ` Chee, Tien Fong
2017-08-29 10:45 ` [U-Boot] [PATCH 04/19] arm: socfpga: Add FPGA loadfs command support tien.fong.chee at intel.com
2017-08-29 11:57   ` Marek Vasut
2017-08-30  8:18     ` Chee, Tien Fong
2017-08-30  8:54       ` Marek Vasut
2017-08-29 10:45 ` [U-Boot] [PATCH 05/19] arm: socfpga: Enhance FPGA program support with at least 4 byte data tien.fong.chee at intel.com
2017-08-29 11:58   ` Marek Vasut
2017-08-30  8:24     ` Chee, Tien Fong
2017-08-30  8:55       ` Marek Vasut
2017-09-04  7:09         ` Chee, Tien Fong
2017-08-29 10:45 ` [U-Boot] [PATCH 06/19] arm: socfpga: Rename the gen5 sdram driver to more specific name tien.fong.chee at intel.com
2017-08-29 11:59   ` Marek Vasut
2017-08-30  8:26     ` Chee, Tien Fong
2017-08-29 10:45 ` [U-Boot] [PATCH 07/19] arm: socfpga: Add DRAM bank size initialization function tien.fong.chee at intel.com
2017-08-29 11:59   ` Marek Vasut
2017-08-30  8:36     ` Chee, Tien Fong
2017-08-30  8:56       ` Marek Vasut
2017-09-04  7:11         ` Chee, Tien Fong
2017-09-04  9:40           ` Marek Vasut
2017-09-05  3:54             ` Chee, Tien Fong
2017-08-29 10:45 ` [U-Boot] [PATCH 08/19] arm: socfpga: Add COMPAT macro for Network on Chip(NoC) tien.fong.chee at intel.com
2017-08-29 10:45 ` [U-Boot] [PATCH 09/19] arm: socfpga: Add DDR driver for Arria 10 tien.fong.chee at intel.com
2017-08-29 10:45 ` [U-Boot] [PATCH 10/19] configs: Add DDR Kconfig support " tien.fong.chee at intel.com
2017-08-29 10:45 ` [U-Boot] [PATCH 11/19] arm: socfpga: Enable build for DDR " tien.fong.chee at intel.com
2017-08-29 10:45 ` [U-Boot] [PATCH 12/19] doc: dtbinding: Add Intel Arria 10 SoCFPGA chosen binding tien.fong.chee at intel.com
2017-08-29 10:45 ` [U-Boot] [PATCH 13/19] dts: Add the FPGA design file name to DTS tien.fong.chee at intel.com
2017-08-29 10:45 ` [U-Boot] [PATCH 14/19] dts: Add device storage and partition " tien.fong.chee at intel.com
2017-08-29 10:45 ` [U-Boot] [PATCH 15/19] arm: socfpga: Add support to memory allocation in SPL tien.fong.chee at intel.com
2017-08-29 10:45 ` [U-Boot] [PATCH 16/19] arm: socfpga: Enhance Intel SoCFPGA program header to support Arria 10 tien.fong.chee at intel.com
2017-08-29 10:45 ` [U-Boot] [PATCH 17/19] arm: socfpga: Adding clock frequency info for U-boot tien.fong.chee at intel.com
2017-08-29 10:46 ` [U-Boot] [PATCH 18/19] arm: socfpga: Adding SoCFPGA info for both SPL and U-boot tien.fong.chee at intel.com
2017-08-29 10:46 ` [U-Boot] [PATCH 19/19] arm: socfpga: Enable SPL loading U-boot to DDR and booting U-boot tien.fong.chee at intel.com

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=be365cf8-5ee0-635d-8489-5cb9180cf2dc@denx.de \
    --to=marex@denx.de \
    --cc=u-boot@lists.denx.de \
    --subject='Re: [U-Boot] [PATCH 03/19] arm: socfpga: Add driver for flash to program FPGA' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.