[U-Boot] [PATCH 0/4] fpga: collected patches
Hello everyone,
while digging in board support code for a board featuring an Altera Cyclone IV FPGA, I fixed some minor issues annoying me. Hope you find that useful. You may drop any patch you don't like, or just send comments, so I can improve it.
Greets Alex
Alexander Dahl (4): fpga: altera: Add some more device sizes fpga: altera: cyclon2: Fix indentation fpga: altera: cyclon2: Check function pointer before calling cmd: fpga: Return values of type command_ret_t only
cmd/fpga.c | 29 +++++++++++++++++------------ drivers/fpga/cyclon2.c | 41 +++++++++++++++++++++++------------------ include/ACEX1K.h | 10 ++++++++++ 3 files changed, 50 insertions(+), 30 deletions(-)
There seems to be only one place, where this is checked against: `altera_validate()`. It should be non zero. Otherwise it is only used to display it, so it probably does not really matter at the moment. But we had the datasheet open anyway …
Sizes in datasheet are bit counts, display here is in bytes.
Signed-off-by: Alexander Dahl ada@thorsis.com --- include/ACEX1K.h | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/include/ACEX1K.h b/include/ACEX1K.h index 9814bba284..7c5253c66c 100644 --- a/include/ACEX1K.h +++ b/include/ACEX1K.h @@ -60,6 +60,16 @@ typedef struct { #define Altera_EP2C35_SIZE 883905 #define Altera_EP3C5_SIZE 368011 /* .rbf size in bytes */
+#define ALTERA_EP4CE6_SIZE 368011 /* 2944088 Bits */ +#define ALTERA_EP4CE10_SIZE 368011 /* 2944088 Bits */ +#define ALTERA_EP4CE15_SIZE 510856 /* 4086848 Bits */ +#define ALTERA_EP4CE22_SIZE 718569 /* 5748552 Bits */ +#define ALTERA_EP4CE30_SIZE 1191788 /* 9534304 Bits */ +#define ALTERA_EP4CE40_SIZE 1191788 /* 9534304 Bits */ +#define ALTERA_EP4CE55_SIZE 1861195 /* 14889560 Bits */ +#define ALTERA_EP4CE75_SIZE 2495719 /* 19965752 Bits */ +#define ALTERA_EP4CE115_SIZE 3571462 /* 28571696 Bits */ + /* Descriptor Macros *********************************************************************/ /* ACEX1K devices */
Some code parts stood too far left …
Signed-off-by: Alexander Dahl ada@thorsis.com --- drivers/fpga/cyclon2.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/drivers/fpga/cyclon2.c b/drivers/fpga/cyclon2.c index 6755956e85..bd7931bbb1 100644 --- a/drivers/fpga/cyclon2.c +++ b/drivers/fpga/cyclon2.c @@ -51,7 +51,7 @@ int CYC2_load(Altera_desc *desc, const void *buf, size_t bsize) * function for FPP, too. */ PRINTF ("%s: Launching Fast Passive Parallel Loader\n", - __FUNCTION__); + __FUNCTION__); ret_val = CYC2_ps_load(desc, buf, bsize); break;
@@ -162,30 +162,30 @@ static int CYC2_ps_load(Altera_desc *desc, const void *buf, size_t bsize) putc (' '); /* terminate the dotted line */ #endif
- /* - * Checking FPGA's CONF_DONE signal - correctly booted ? - */ + /* + * Checking FPGA's CONF_DONE signal - correctly booted ? + */
- if ( ! (*fn->done) (cookie) ) { - puts ("** Booting failed! CONF_DONE is still deasserted.\n"); - (*fn->abort) (cookie); - return (FPGA_FAIL); - } + if ( ! (*fn->done) (cookie) ) { + puts ("** Booting failed! CONF_DONE is still deasserted.\n"); + (*fn->abort) (cookie); + return (FPGA_FAIL); + } #ifdef CONFIG_SYS_FPGA_PROG_FEEDBACK - puts(" OK\n"); + puts(" OK\n"); #endif
- ret_val = FPGA_SUCCESS; + ret_val = FPGA_SUCCESS;
#ifdef CONFIG_SYS_FPGA_PROG_FEEDBACK - if (ret_val == FPGA_SUCCESS) { - puts ("Done.\n"); - } - else { - puts ("Fail.\n"); - } + if (ret_val == FPGA_SUCCESS) { + puts ("Done.\n"); + } + else { + puts ("Fail.\n"); + } #endif - (*fn->post) (cookie); + (*fn->post) (cookie);
} else { printf ("%s: NULL Interface function table!\n", __FUNCTION__);
On 11. 04. 19 12:18, Alexander Dahl wrote:
Some code parts stood too far left …
Signed-off-by: Alexander Dahl ada@thorsis.com
drivers/fpga/cyclon2.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/drivers/fpga/cyclon2.c b/drivers/fpga/cyclon2.c index 6755956e85..bd7931bbb1 100644 --- a/drivers/fpga/cyclon2.c +++ b/drivers/fpga/cyclon2.c @@ -51,7 +51,7 @@ int CYC2_load(Altera_desc *desc, const void *buf, size_t bsize) * function for FPP, too. */ PRINTF ("%s: Launching Fast Passive Parallel Loader\n",
__FUNCTION__);
__FUNCTION__);
Will be good to fix things reported by checkpatch first and use proper indentation. Then vice-versa.
M
As already done for the 'pre' function, a check is added to not follow a NULL pointer, if somebody has not assigned a 'post' function.
Signed-off-by: Alexander Dahl ada@thorsis.com --- drivers/fpga/cyclon2.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/fpga/cyclon2.c b/drivers/fpga/cyclon2.c index bd7931bbb1..c0fdf52582 100644 --- a/drivers/fpga/cyclon2.c +++ b/drivers/fpga/cyclon2.c @@ -185,8 +185,12 @@ static int CYC2_ps_load(Altera_desc *desc, const void *buf, size_t bsize) puts ("Fail.\n"); } #endif - (*fn->post) (cookie);
+ /* + * Run the post configuration function if there is one. + */ + if (*fn->post) + (*fn->post) (cookie); } else { printf ("%s: NULL Interface function table!\n", __FUNCTION__); }
Passing on a return value of -1 from a called function leads to printing out usage text. In case of actually correct usage with correctly specified parameters but some fail at runtime printing out that usage text is distracting.
The reason is most 'fpga_' functions return either FPGA_SUCCESS or FPGA_FAIL, the latter is equal to -1 which is the same value as CMD_RET_USAGE. So just passing on FPGA_FAIL leads to printing out usage.
We should only return CMD_RET_USAGE in cases, where the user sent wrong input. Every other case should return CMD_RET_SUCCESS or CMD_RET_FAILURE, and not simply pass an error code.
Signed-off-by: Alexander Dahl ada@thorsis.com --- cmd/fpga.c | 42 ++++++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 14 deletions(-)
diff --git a/cmd/fpga.c b/cmd/fpga.c index 88a8e3f318..1bb41217e5 100644 --- a/cmd/fpga.c +++ b/cmd/fpga.c @@ -13,6 +13,14 @@ #include <fs.h> #include <malloc.h>
+static int do_fpga_wrap_ret(int ret) +{ + if (ret != FPGA_SUCCESS) + debug("fpga: function returned %d\n", ret); + + return ret ? CMD_RET_FAILURE : CMD_RET_SUCCESS; +} + static long do_fpga_get_device(char *arg) { long dev = FPGA_INVALID_DEVICE; @@ -59,7 +67,7 @@ static int do_fpga_check_params(long *dev, long *fpga_data, size_t *data_size, } *data_size = local_data_size;
- return 0; + return CMD_RET_SUCCESS; }
#if defined(CONFIG_CMD_FPGA_LOAD_SECURE) @@ -110,7 +118,8 @@ int do_fpga_loads(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) if (ret) return ret;
- return fpga_loads(dev, (void *)fpga_data, data_size, &fpga_sec_info); + return do_fpga_wrap_ret(fpga_loads(dev, (void *)fpga_data, + data_size, &fpga_sec_info)); } #endif
@@ -134,7 +143,8 @@ static int do_fpga_loadfs(cmd_tbl_t *cmdtp, int flag, int argc, fpga_fsinfo.dev_part = argv[5]; fpga_fsinfo.filename = argv[6];
- return fpga_fsload(dev, (void *)fpga_data, data_size, &fpga_fsinfo); + return do_fpga_wrap_ret(fpga_fsload(dev, (void *)fpga_data, + data_size, &fpga_fsinfo)); } #endif
@@ -143,7 +153,7 @@ static int do_fpga_info(cmd_tbl_t *cmdtp, int flag, int argc, { long dev = do_fpga_get_device(argv[0]);
- return fpga_info(dev); + return do_fpga_wrap_ret(fpga_info(dev)); }
static int do_fpga_dump(cmd_tbl_t *cmdtp, int flag, int argc, @@ -158,7 +168,7 @@ static int do_fpga_dump(cmd_tbl_t *cmdtp, int flag, int argc, if (ret) return ret;
- return fpga_dump(dev, (void *)fpga_data, data_size); + return do_fpga_wrap_ret(fpga_dump(dev, (void *)fpga_data, data_size)); }
static int do_fpga_load(cmd_tbl_t *cmdtp, int flag, int argc, @@ -173,7 +183,8 @@ static int do_fpga_load(cmd_tbl_t *cmdtp, int flag, int argc, if (ret) return ret;
- return fpga_load(dev, (void *)fpga_data, data_size, BIT_FULL); + return do_fpga_wrap_ret(fpga_load(dev, (void *)fpga_data, + data_size, BIT_FULL)); }
static int do_fpga_loadb(cmd_tbl_t *cmdtp, int flag, int argc, @@ -188,7 +199,8 @@ static int do_fpga_loadb(cmd_tbl_t *cmdtp, int flag, int argc, if (ret) return ret;
- return fpga_loadbitstream(dev, (void *)fpga_data, data_size, BIT_FULL); + return do_fpga_wrap_ret(fpga_loadbitstream(dev, (void *)fpga_data, + data_size, BIT_FULL)); }
#if defined(CONFIG_CMD_FPGA_LOADP) @@ -204,7 +216,8 @@ static int do_fpga_loadp(cmd_tbl_t *cmdtp, int flag, int argc, if (ret) return ret;
- return fpga_load(dev, (void *)fpga_data, data_size, BIT_PARTIAL); + return do_fpga_wrap_ret(fpga_load(dev, (void *)fpga_data, + data_size, BIT_PARTIAL)); } #endif
@@ -221,8 +234,8 @@ static int do_fpga_loadbp(cmd_tbl_t *cmdtp, int flag, int argc, if (ret) return ret;
- return fpga_loadbitstream(dev, (void *)fpga_data, data_size, - BIT_PARTIAL); + return do_fpga_wrap_ret(fpga_loadbitstream(dev, (void *)fpga_data, + data_size, BIT_PARTIAL)); } #endif
@@ -303,14 +316,14 @@ static int do_fpga_loadmk(cmd_tbl_t *cmdtp, int flag, int argc, data_size = image_size; #else puts("Gunzip image is not supported\n"); - return 1; + return CMD_RET_FAILURE; #endif } else { data = (ulong)image_get_data(hdr); data_size = image_get_data_size(hdr); } - return fpga_load(dev, (void *)data, data_size, - BIT_FULL); + return do_fpga_wrap_ret(fpga_load(dev, (void *)data, + data_size, BIT_FULL)); } #endif #if defined(CONFIG_FIT) @@ -350,7 +363,8 @@ static int do_fpga_loadmk(cmd_tbl_t *cmdtp, int flag, int argc, return CMD_RET_FAILURE; }
- return fpga_load(dev, fit_data, data_size, BIT_FULL); + return do_fpga_wrap_ret(fpga_load(dev, fit_data, + data_size, BIT_FULL)); } #endif default:
On 11. 04. 19 12:18, Alexander Dahl wrote:
Passing on a return value of -1 from a called function leads to printing out usage text. In case of actually correct usage with correctly specified parameters but some fail at runtime printing out that usage text is distracting.
The reason is most 'fpga_' functions return either FPGA_SUCCESS or FPGA_FAIL, the latter is equal to -1 which is the same value as CMD_RET_USAGE. So just passing on FPGA_FAIL leads to printing out usage.
We should only return CMD_RET_USAGE in cases, where the user sent wrong input. Every other case should return CMD_RET_SUCCESS or CMD_RET_FAILURE, and not simply pass an error code.
Signed-off-by: Alexander Dahl ada@thorsis.com
This should be enough to ensure the same. Please test.
M
diff --git a/include/fpga.h b/include/fpga.h index 51de5c55f830..ec5144334df1 100644 --- a/include/fpga.h +++ b/include/fpga.h @@ -15,7 +15,7 @@
/* fpga_xxxx function return value definitions */ #define FPGA_SUCCESS 0 -#define FPGA_FAIL -1 +#define FPGA_FAIL 1
/* device numbers must be non-negative */ #define FPGA_INVALID_DEVICE -1
participants (2)
-
Alexander Dahl -
Michal Simek