[U-Boot] [PATCH v2 0/4] Stratix10 FPGA reconfiguration support
From: "Ang, Chee Hong" chee.hong.ang@intel.com
Summary of v2 changes: - Addressed review comments in drivers/fpga/stratix10.c - Update SPDX tag in drivers/fpga/stratix10.c - Add parentheses in macro functions - Define default empty socfpga_fpga_add()
v1 patchsets: https://lists.denx.de/pipermail/u-boot/2018-October/343256.html
Ang, Chee Hong (4): arm: socfpga: stratix10: Add macros for mailbox's arguments arm: socfpga: stratix10: Add Stratix 10 FPGA Reconfiguration Driver arm: socfpga: stratix10: Add Stratix10 FPGA into FPGA device table arm: socfpga: stratix10: Enable Stratix10 FPGA Reconfiguration
arch/arm/mach-socfpga/include/mach/mailbox_s10.h | 6 + arch/arm/mach-socfpga/misc.c | 29 +++ arch/arm/mach-socfpga/misc_s10.c | 2 + configs/socfpga_stratix10_defconfig | 1 + drivers/fpga/Kconfig | 11 + drivers/fpga/Makefile | 1 + drivers/fpga/altera.c | 6 + drivers/fpga/stratix10.c | 288 +++++++++++++++++++++++ include/altera.h | 8 + 9 files changed, 352 insertions(+) create mode 100644 drivers/fpga/stratix10.c
From: "Ang, Chee Hong" chee.hong.ang@intel.com
Add macros for specifying number of arguments in mailbox command.
Signed-off-by: Ang, Chee Hong chee.hong.ang@intel.com --- arch/arm/mach-socfpga/include/mach/mailbox_s10.h | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/arch/arm/mach-socfpga/include/mach/mailbox_s10.h b/arch/arm/mach-socfpga/include/mach/mailbox_s10.h index 660df35..ae728a5 100644 --- a/arch/arm/mach-socfpga/include/mach/mailbox_s10.h +++ b/arch/arm/mach-socfpga/include/mach/mailbox_s10.h @@ -107,6 +107,12 @@ enum ALT_SDM_MBOX_RESP_CODE { #define RECONFIG_STATUS_PIN_STATUS 2 #define RECONFIG_STATUS_SOFTFUNC_STATUS 3
+/* Macros for specifying number of arguments in mailbox command */ +#define MBOX_NUM_ARGS(n, b) (((n) & 0xFF) << (b)) +#define MBOX_DIRECT_COUNT(n) MBOX_NUM_ARGS((n), 0) +#define MBOX_ARG_DESC_COUNT(n) MBOX_NUM_ARGS((n), 8) +#define MBOX_RESP_DESC_COUNT(n) MBOX_NUM_ARGS((n), 16) + #define MBOX_CFGSTAT_STATE_IDLE 0x00000000 #define MBOX_CFGSTAT_STATE_CONFIG 0x10000000 #define MBOX_CFGSTAT_STATE_FAILACK 0x08000000
From: "Ang, Chee Hong" chee.hong.ang@intel.com
Enable FPGA reconfiguration support for Stratix 10 SoC.
Signed-off-by: Ang, Chee Hong chee.hong.ang@intel.com --- drivers/fpga/Kconfig | 11 ++ drivers/fpga/Makefile | 1 + drivers/fpga/stratix10.c | 288 +++++++++++++++++++++++++++++++++++++++++++++++ include/altera.h | 4 + 4 files changed, 304 insertions(+) create mode 100644 drivers/fpga/stratix10.c
diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index 50e9019..8f59193 100644 --- a/drivers/fpga/Kconfig +++ b/drivers/fpga/Kconfig @@ -31,6 +31,17 @@ config FPGA_CYCLON2 Enable FPGA driver for loading bitstream in BIT and BIN format on Altera Cyclone II device.
+config FPGA_STRATIX10 + bool "Enable Altera FPGA driver for Stratix 10" + depends on TARGET_SOCFPGA_STRATIX10 + select FPGA_ALTERA + help + Say Y here to enable the Altera Stratix 10 FPGA specific driver + + This provides common functionality for Altera Stratix 10 devices. + Enable FPGA driver for writing bitstream into Altera Stratix10 + device. + config FPGA_XILINX bool "Enable Xilinx FPGA drivers" select FPGA diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index 97d7d5d..5a778c1 100644 --- a/drivers/fpga/Makefile +++ b/drivers/fpga/Makefile @@ -17,6 +17,7 @@ obj-$(CONFIG_FPGA_ACEX1K) += ACEX1K.o obj-$(CONFIG_FPGA_CYCLON2) += cyclon2.o obj-$(CONFIG_FPGA_STRATIX_II) += stratixII.o obj-$(CONFIG_FPGA_STRATIX_V) += stratixv.o +obj-$(CONFIG_FPGA_STRATIX10) += stratix10.o obj-$(CONFIG_FPGA_SOCFPGA) += socfpga.o obj-$(CONFIG_TARGET_SOCFPGA_GEN5) += socfpga_gen5.o obj-$(CONFIG_TARGET_SOCFPGA_ARRIA10) += socfpga_arria10.o diff --git a/drivers/fpga/stratix10.c b/drivers/fpga/stratix10.c new file mode 100644 index 0000000..6728291 --- /dev/null +++ b/drivers/fpga/stratix10.c @@ -0,0 +1,288 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (C) 2018 Intel Corporation <www.intel.com> + */ + +#include <common.h> +#include <altera.h> +#include <asm/arch/mailbox_s10.h> + +#define RECONFIG_STATUS_POLL_RESP_TIMEOUT_MS 60000 +#define RECONFIG_STATUS_INTERVAL_DELAY_US 1000000 + +static const struct mbox_cfgstat_state { + int err_no; + const char *error_name; +} mbox_cfgstat_state[] = { + {MBOX_CFGSTAT_STATE_IDLE, "FPGA in idle mode."}, + {MBOX_CFGSTAT_STATE_CONFIG, "FPGA in config mode."}, + {MBOX_CFGSTAT_STATE_FAILACK, "Acknowledgment failed!"}, + {MBOX_CFGSTAT_STATE_ERROR_INVALID, "Invalid bitstream!"}, + {MBOX_CFGSTAT_STATE_ERROR_CORRUPT, "Corrupted bitstream!"}, + {MBOX_CFGSTAT_STATE_ERROR_AUTH, "Authentication failed!"}, + {MBOX_CFGSTAT_STATE_ERROR_CORE_IO, "I/O error!"}, + {MBOX_CFGSTAT_STATE_ERROR_HARDWARE, "Hardware error!"}, + {MBOX_CFGSTAT_STATE_ERROR_FAKE, "Fake error!"}, + {MBOX_CFGSTAT_STATE_ERROR_BOOT_INFO, "Error in boot info!"}, + {MBOX_CFGSTAT_STATE_ERROR_QSPI_ERROR, "Error in QSPI!"}, + {MBOX_RESP_ERROR, "Mailbox general error!"}, + {-ETIMEDOUT, "I/O timeout error"}, + {-1, "Unknown error!"} +}; + +#define MBOX_CFGSTAT_MAX ARRAY_SIZE(mbox_cfgstat_state) + +static const char *mbox_cfgstat_to_str(int err) +{ + int i; + + for (i = 0; i < MBOX_CFGSTAT_MAX - 1; i++) { + if (mbox_cfgstat_state[i].err_no == err) + return mbox_cfgstat_state[i].error_name; + } + + return mbox_cfgstat_state[MBOX_CFGSTAT_MAX - 1].error_name; +} + +/* + * Add the ongoing transaction's command ID into pending list and return + * the command ID for next transfer. + */ +static u8 add_transfer(u32 *xfer_pending_list, size_t list_size, u8 id) +{ + int i; + + for (i = 0; i < list_size; i++) { + if (xfer_pending_list[i]) + continue; + xfer_pending_list[i] = id; + debug("ID(%d) added to transaction pending list\n", id); + /* + * Increment command ID for next transaction. + * Valid command ID (4 bits) is from 1 to 15. + */ + id = (id % 15) + 1; + break; + } + + return id; +} + +/* + * Check whether response ID match the command ID in the transfer + * pending list. If a match is found in the transfer pending list, + * it clears the transfer pending list and return the matched + * command ID. + */ +static int get_and_clr_transfer(u32 *xfer_pending_list, size_t list_size, + u8 id) +{ + int i; + + for (i = 0; i < list_size; i++) { + if (id != xfer_pending_list[i]) + continue; + xfer_pending_list[i] = 0; + return id; + } + + return 0; +} + +/* + * Polling the FPGA configuration status. + * Return 0 for success, non-zero for error. + */ +static int reconfig_status_polling_resp(void) +{ + int ret; + unsigned long start = get_timer(0); + + while (1) { + ret = mbox_get_fpga_config_status(MBOX_RECONFIG_STATUS); + if (!ret) + return 0; /* configuration success */ + + if (ret != MBOX_CFGSTAT_STATE_CONFIG) + return ret; + + if (get_timer(start) > RECONFIG_STATUS_POLL_RESP_TIMEOUT_MS) + break; /* time out */ + + puts("."); + udelay(RECONFIG_STATUS_INTERVAL_DELAY_US); + } + + return -ETIMEDOUT; +} + +static u32 get_resp_hdr(u32 *r_index, u32 *w_index, u32 *resp_count, + u32 *resp_buf, u32 buf_size, u32 client_id) +{ + u32 buf[MBOX_RESP_BUFFER_SIZE]; + u32 mbox_hdr; + u32 resp_len; + u32 hdr_len; + u32 i; + + if (*resp_count < buf_size) { + u32 rcv_len_max = buf_size - *resp_count; + + if (rcv_len_max > MBOX_RESP_BUFFER_SIZE) + rcv_len_max = MBOX_RESP_BUFFER_SIZE; + resp_len = mbox_rcv_resp(buf, rcv_len_max); + + for (i = 0; i < resp_len; i++) { + resp_buf[(*w_index)++] = buf[i]; + *w_index %= buf_size; + *resp_count++; + } + } + + /* No response in buffer */ + if (*resp_count == 0) + return 0; + + mbox_hdr = resp_buf[*r_index]; + + hdr_len = MBOX_RESP_LEN_GET(mbox_hdr); + + /* Insufficient header length to return a mailbox header */ + if ((*resp_count - 1) < hdr_len) + return 0; + + *r_index += (hdr_len + 1); + *r_index %= buf_size; + *resp_count -= (hdr_len + 1); + + /* Make sure response belongs to us */ + if (MBOX_RESP_CLIENT_GET(mbox_hdr) != client_id) + return 0; + + return mbox_hdr; +} + +/* Send bit stream data to SDM via RECONFIG_DATA mailbox command */ +static int send_reconfig_data(const void *rbf_data, size_t rbf_size, + u32 xfer_max, u32 buf_size_max) +{ + u32 response_buffer[MBOX_RESP_BUFFER_SIZE]; + u32 xfer_pending[MBOX_RESP_BUFFER_SIZE]; + u32 resp_rindex = 0; + u32 resp_windex = 0; + u32 resp_count = 0; + u32 xfer_count = 0; + u8 resp_err = 0; + u8 cmd_id = 1; + u32 args[3]; + int ret; + + debug("SDM xfer_max = %d\n", xfer_max); + debug("SDM buf_size_max = %x\n\n", buf_size_max); + + memset(xfer_pending, 0, sizeof(xfer_pending)); + + while (rbf_size || xfer_count) { + if (!resp_err && rbf_size && xfer_count < xfer_max) { + args[0] = MBOX_ARG_DESC_COUNT(1); + args[1] = (u32)rbf_data; + if (rbf_size >= buf_size_max) { + args[2] = buf_size_max; + rbf_size -= buf_size_max; + rbf_data += buf_size_max; + } else { + args[2] = (u32)rbf_size; + rbf_size = 0; + } + + ret = mbox_send_cmd_only(cmd_id, MBOX_RECONFIG_DATA, + MBOX_CMD_INDIRECT, 3, args); + if (ret) { + resp_err = 1; + } else { + xfer_count++; + cmd_id = add_transfer(xfer_pending, + MBOX_RESP_BUFFER_SIZE, + cmd_id); + } + puts("."); + } else { + u32 resp_hdr = get_resp_hdr(&resp_rindex, &resp_windex, + &resp_count, + response_buffer, + MBOX_RESP_BUFFER_SIZE, + MBOX_CLIENT_ID_UBOOT); + + /* + * If no valid response header found or + * non-zero length from RECONFIG_DATA + */ + if (!resp_hdr || MBOX_RESP_LEN_GET(resp_hdr)) + continue; + + /* Check for response's status */ + if (!resp_err) { + ret = MBOX_RESP_ERR_GET(resp_hdr); + debug("Response error code: %08x\n", ret); + /* Error in response */ + if (ret) + resp_err = 1; + } + + ret = get_and_clr_transfer(xfer_pending, + MBOX_RESP_BUFFER_SIZE, + MBOX_RESP_ID_GET(resp_hdr)); + if (ret) { + /* Claim and reuse the ID */ + cmd_id = (u8)ret; + xfer_count--; + } + + if (resp_err && !xfer_count) + return ret; + } + } + + return 0; +} + +/* + * This is the interface used by FPGA driver. + * Return 0 for success, non-zero for error. + */ +int stratix10_load(Altera_desc *desc, const void *rbf_data, size_t rbf_size) +{ + int ret; + u32 resp_len = 2; + u32 resp_buf[2]; + + debug("Sending MBOX_RECONFIG...\n"); + ret = mbox_send_cmd(MBOX_ID_UBOOT, MBOX_RECONFIG, MBOX_CMD_DIRECT, 0, + NULL, 0, &resp_len, resp_buf); + if (ret) { + puts("Failure in RECONFIG mailbox command!\n"); + return ret; + } + + ret = send_reconfig_data(rbf_data, rbf_size, resp_buf[0], resp_buf[1]); + if (ret) { + printf("RECONFIG_DATA error: %08x, %s\n", ret, + mbox_cfgstat_to_str(ret)); + return ret; + } + + /* Make sure we don't send MBOX_RECONFIG_STATUS too fast */ + udelay(RECONFIG_STATUS_INTERVAL_DELAY_US); + + debug("Polling with MBOX_RECONFIG_STATUS...\n"); + ret = reconfig_status_polling_resp(); + if (ret) { + printf("RECONFIG_STATUS Error: %08x, %s\n", ret, + mbox_cfgstat_to_str(ret)); + return ret; + } + + puts("FPGA reconfiguration OK!\n"); + + return ret; +} diff --git a/include/altera.h b/include/altera.h index ead5d3d..233b467 100644 --- a/include/altera.h +++ b/include/altera.h @@ -116,4 +116,8 @@ int socfpga_load(Altera_desc *desc, const void *rbf_data, size_t rbf_size); int stratixv_load(Altera_desc *desc, const void *rbf_data, size_t rbf_size); #endif
+#ifdef CONFIG_FPGA_STRATIX10 +int stratix10_load(Altera_desc *desc, const void *rbf_data, size_t rbf_size); +#endif + #endif /* _ALTERA_H_ */
From: "Ang, Chee Hong" chee.hong.ang@intel.com
Enable 'fpga' command in u-boot. User will be able to use the fpga command to program the FPGA on Stratix10 SoC.
Signed-off-by: Ang, Chee Hong chee.hong.ang@intel.com --- arch/arm/mach-socfpga/misc.c | 29 +++++++++++++++++++++++++++++ arch/arm/mach-socfpga/misc_s10.c | 2 ++ drivers/fpga/altera.c | 6 ++++++ include/altera.h | 4 ++++ 4 files changed, 41 insertions(+)
diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c index a4f6d5c..7986b58 100644 --- a/arch/arm/mach-socfpga/misc.c +++ b/arch/arm/mach-socfpga/misc.c @@ -88,6 +88,27 @@ int overwrite_console(void) #endif
#ifdef CONFIG_FPGA +#ifdef CONFIG_FPGA_STRATIX10 +/* + * FPGA programming support for SoC FPGA Stratix 10 + */ +static Altera_desc altera_fpga[] = { + { + /* Family */ + Intel_FPGA_Stratix10, + /* Interface type */ + secure_device_manager_mailbox, + /* No limitation as additional data will be ignored */ + -1, + /* No device function table */ + NULL, + /* Base interface address specified in driver */ + NULL, + /* No cookie implementation */ + 0 + }, +}; +#else /* * FPGA programming support for SoC FPGA Cyclone V */ @@ -107,6 +128,7 @@ static Altera_desc altera_fpga[] = { 0 }, }; +#endif
/* add device descriptor to FPGA device table */ void socfpga_fpga_add(void) @@ -116,6 +138,13 @@ void socfpga_fpga_add(void) for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) fpga_add(fpga_altera, &altera_fpga[i]); } + +#else + +__weak void socfpga_fpga_add(void) +{ +} + #endif
int arch_cpu_init(void) diff --git a/arch/arm/mach-socfpga/misc_s10.c b/arch/arm/mach-socfpga/misc_s10.c index 918baac..f23c0dc 100644 --- a/arch/arm/mach-socfpga/misc_s10.c +++ b/arch/arm/mach-socfpga/misc_s10.c @@ -124,6 +124,8 @@ int arch_misc_init(void)
int arch_early_init_r(void) { + socfpga_fpga_add(); + return 0; }
diff --git a/drivers/fpga/altera.c b/drivers/fpga/altera.c index 9605554..7c8f518 100644 --- a/drivers/fpga/altera.c +++ b/drivers/fpga/altera.c @@ -39,6 +39,9 @@ static const struct altera_fpga { #if defined(CONFIG_FPGA_STRATIX_V) { Altera_StratixV, "StratixV", stratixv_load, NULL, NULL }, #endif +#if defined(CONFIG_FPGA_STRATIX10) + { Intel_FPGA_Stratix10, "Stratix10", stratix10_load, NULL, NULL }, +#endif #if defined(CONFIG_FPGA_SOCFPGA) { Altera_SoCFPGA, "SoC FPGA", socfpga_load, NULL, NULL }, #endif @@ -154,6 +157,9 @@ int altera_info(Altera_desc *desc) case fast_passive_parallel_security: printf("Fast Passive Parallel with Security (FPPS)\n"); break; + case secure_device_manager_mailbox: + puts("Secure Device Manager (SDM) Mailbox\n"); + break; /* Add new interface types here */ default: printf("Unsupported interface type, %d\n", desc->iface); diff --git a/include/altera.h b/include/altera.h index 233b467..22d55cf 100644 --- a/include/altera.h +++ b/include/altera.h @@ -39,6 +39,8 @@ enum altera_iface { fast_passive_parallel, /* fast passive parallel with security (FPPS) */ fast_passive_parallel_security, + /* secure device manager (SDM) mailbox */ + secure_device_manager_mailbox, /* insert all new types before this */ max_altera_iface_type, }; @@ -54,6 +56,8 @@ enum altera_family { Altera_StratixII, /* StratixV Family */ Altera_StratixV, + /* Stratix10 Family */ + Intel_FPGA_Stratix10, /* SoCFPGA Family */ Altera_SoCFPGA,
On 10/08/2018 11:48 AM, chee.hong.ang@intel.com wrote:
From: "Ang, Chee Hong" chee.hong.ang@intel.com
Enable 'fpga' command in u-boot. User will be able to use the fpga command to program the FPGA on Stratix10 SoC.
Signed-off-by: Ang, Chee Hong chee.hong.ang@intel.com
arch/arm/mach-socfpga/misc.c | 29 +++++++++++++++++++++++++++++ arch/arm/mach-socfpga/misc_s10.c | 2 ++ drivers/fpga/altera.c | 6 ++++++ include/altera.h | 4 ++++ 4 files changed, 41 insertions(+)
diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c index a4f6d5c..7986b58 100644 --- a/arch/arm/mach-socfpga/misc.c +++ b/arch/arm/mach-socfpga/misc.c @@ -88,6 +88,27 @@ int overwrite_console(void) #endif
#ifdef CONFIG_FPGA +#ifdef CONFIG_FPGA_STRATIX10 +/*
- FPGA programming support for SoC FPGA Stratix 10
- */
+static Altera_desc altera_fpga[] = {
- {
/* Family */Intel_FPGA_Stratix10,/* Interface type */secure_device_manager_mailbox,/* No limitation as additional data will be ignored */-1,/* No device function table */NULL,/* Base interface address specified in driver */NULL,/* No cookie implementation */0- },
+}; +#else /*
- FPGA programming support for SoC FPGA Cyclone V
*/ @@ -107,6 +128,7 @@ static Altera_desc altera_fpga[] = { 0 }, }; +#endif
/* add device descriptor to FPGA device table */ void socfpga_fpga_add(void) @@ -116,6 +138,13 @@ void socfpga_fpga_add(void) for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) fpga_add(fpga_altera, &altera_fpga[i]); }
+#else
+__weak void socfpga_fpga_add(void) +{ +}
Why is a __weak function defined only in else-statement ?
It should be defined always, with a sane default implementation.
On Mon, 2018-10-08 at 11:57 +0200, Marek Vasut wrote:
On 10/08/2018 11:48 AM, chee.hong.ang@intel.com wrote:
From: "Ang, Chee Hong" chee.hong.ang@intel.com
Enable 'fpga' command in u-boot. User will be able to use the fpga command to program the FPGA on Stratix10 SoC.
Signed-off-by: Ang, Chee Hong chee.hong.ang@intel.com
arch/arm/mach-socfpga/misc.c | 29 +++++++++++++++++++++++++++++ arch/arm/mach-socfpga/misc_s10.c | 2 ++ drivers/fpga/altera.c | 6 ++++++ include/altera.h | 4 ++++ 4 files changed, 41 insertions(+)
diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach- socfpga/misc.c index a4f6d5c..7986b58 100644 --- a/arch/arm/mach-socfpga/misc.c +++ b/arch/arm/mach-socfpga/misc.c @@ -88,6 +88,27 @@ int overwrite_console(void) #endif #ifdef CONFIG_FPGA +#ifdef CONFIG_FPGA_STRATIX10 +/*
- FPGA programming support for SoC FPGA Stratix 10
- */
+static Altera_desc altera_fpga[] = {
- {
/* Family */Intel_FPGA_Stratix10,/* Interface type */secure_device_manager_mailbox,/* No limitation as additional data will beignored */
-1,/* No device function table */NULL,/* Base interface address specified in driver */NULL,/* No cookie implementation */0- },
+}; +#else /* * FPGA programming support for SoC FPGA Cyclone V */ @@ -107,6 +128,7 @@ static Altera_desc altera_fpga[] = { 0 }, }; +#endif /* add device descriptor to FPGA device table */ void socfpga_fpga_add(void) @@ -116,6 +138,13 @@ void socfpga_fpga_add(void) for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) fpga_add(fpga_altera, &altera_fpga[i]); }
+#else
+__weak void socfpga_fpga_add(void) +{ +}
Why is a __weak function defined only in else-statement ?
It should be defined always, with a sane default implementation.
I will remove the empty function in #else-statement and define the default function like this :
/* add device descriptor to FPGA device table */ void socfpga_fpga_add(void) { #ifdef CONFIG_FPGA int i; fpga_init(); for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) fpga_add(fpga_altera, &altera_fpga[i]); #endif }
Is that OK?
On 10/08/2018 05:10 PM, Ang, Chee Hong wrote:
On Mon, 2018-10-08 at 11:57 +0200, Marek Vasut wrote:
On 10/08/2018 11:48 AM, chee.hong.ang@intel.com wrote:
From: "Ang, Chee Hong" chee.hong.ang@intel.com
Enable 'fpga' command in u-boot. User will be able to use the fpga command to program the FPGA on Stratix10 SoC.
Signed-off-by: Ang, Chee Hong chee.hong.ang@intel.com
arch/arm/mach-socfpga/misc.c | 29 +++++++++++++++++++++++++++++ arch/arm/mach-socfpga/misc_s10.c | 2 ++ drivers/fpga/altera.c | 6 ++++++ include/altera.h | 4 ++++ 4 files changed, 41 insertions(+)
diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach- socfpga/misc.c index a4f6d5c..7986b58 100644 --- a/arch/arm/mach-socfpga/misc.c +++ b/arch/arm/mach-socfpga/misc.c @@ -88,6 +88,27 @@ int overwrite_console(void) #endif #ifdef CONFIG_FPGA +#ifdef CONFIG_FPGA_STRATIX10 +/*
- FPGA programming support for SoC FPGA Stratix 10
- */
+static Altera_desc altera_fpga[] = {
- {
/* Family */Intel_FPGA_Stratix10,/* Interface type */secure_device_manager_mailbox,/* No limitation as additional data will beignored */
-1,/* No device function table */NULL,/* Base interface address specified in driver */NULL,/* No cookie implementation */0- },
+}; +#else /* * FPGA programming support for SoC FPGA Cyclone V */ @@ -107,6 +128,7 @@ static Altera_desc altera_fpga[] = { 0 }, }; +#endif /* add device descriptor to FPGA device table */ void socfpga_fpga_add(void) @@ -116,6 +138,13 @@ void socfpga_fpga_add(void) for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) fpga_add(fpga_altera, &altera_fpga[i]); }
+#else
+__weak void socfpga_fpga_add(void) +{ +}
Why is a __weak function defined only in else-statement ?
It should be defined always, with a sane default implementation.
I will remove the empty function in #else-statement and define the default function like this :
/* add device descriptor to FPGA device table */ void socfpga_fpga_add(void) { #ifdef CONFIG_FPGA int i; fpga_init(); for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) fpga_add(fpga_altera, &altera_fpga[i]); #endif }
Is that OK?
Can't you have __weak empty implementation of socfpga_fpga_add() and implement a version per platform ? Would that work and make sense ?
btw. the best solution would be to fix this proper and implement a DM/DT based probing of the FPGA, including a proper driver(s) in drivers/fpga/ instead of putting all the crud into arch/arm/mach-socfpga ...
On Mon, 2018-10-08 at 22:32 +0200, Marek Vasut wrote:
On 10/08/2018 05:10 PM, Ang, Chee Hong wrote:
On Mon, 2018-10-08 at 11:57 +0200, Marek Vasut wrote:
On 10/08/2018 11:48 AM, chee.hong.ang@intel.com wrote:
From: "Ang, Chee Hong" chee.hong.ang@intel.com
Enable 'fpga' command in u-boot. User will be able to use the fpga command to program the FPGA on Stratix10 SoC.
Signed-off-by: Ang, Chee Hong chee.hong.ang@intel.com
arch/arm/mach-socfpga/misc.c | 29 +++++++++++++++++++++++++++++ arch/arm/mach-socfpga/misc_s10.c | 2 ++ drivers/fpga/altera.c | 6 ++++++ include/altera.h | 4 ++++ 4 files changed, 41 insertions(+)
diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach- socfpga/misc.c index a4f6d5c..7986b58 100644 --- a/arch/arm/mach-socfpga/misc.c +++ b/arch/arm/mach-socfpga/misc.c @@ -88,6 +88,27 @@ int overwrite_console(void) #endif #ifdef CONFIG_FPGA +#ifdef CONFIG_FPGA_STRATIX10 +/*
- FPGA programming support for SoC FPGA Stratix 10
- */
+static Altera_desc altera_fpga[] = {
- {
/* Family */Intel_FPGA_Stratix10,/* Interface type */secure_device_manager_mailbox,/* No limitation as additional data will beignored */
-1,/* No device function table */NULL,/* Base interface address specified in driver*/
NULL,/* No cookie implementation */0- },
+}; +#else /* * FPGA programming support for SoC FPGA Cyclone V */ @@ -107,6 +128,7 @@ static Altera_desc altera_fpga[] = { 0 }, }; +#endif /* add device descriptor to FPGA device table */ void socfpga_fpga_add(void) @@ -116,6 +138,13 @@ void socfpga_fpga_add(void) for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) fpga_add(fpga_altera, &altera_fpga[i]); }
+#else
+__weak void socfpga_fpga_add(void) +{ +}
Why is a __weak function defined only in else-statement ?
It should be defined always, with a sane default implementation.
I will remove the empty function in #else-statement and define the default function like this :
/* add device descriptor to FPGA device table */ void socfpga_fpga_add(void) { #ifdef CONFIG_FPGA int i; fpga_init(); for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) fpga_add(fpga_altera, &altera_fpga[i]); #endif }
Is that OK?
Can't you have __weak empty implementation of socfpga_fpga_add() and implement a version per platform ? Would that work and make sense ?
socfpga_fpga_add() as shown above is a generic function for adding FPGA devices to FPGA driver which applies to all our platforms. This is the reason why it is defined in misc.c instead of misc_<platform_name>.c.
It turned out we already have this defined in misc.h: #ifdef CONFIG_FPGA void socfpga_fpga_add(void); #else static inline void socfpga_fpga_add(void) {} #endif
So I don't think I need to make any changes to socfpga_fpga_add() in misc.c. I just have to remove ifdef CONFIG_FPGA in misc_s10.c because it was unnecessary. I will submit v3 for this patch and you can comment further. The v3 patch will be simpler. Thanks.
btw. the best solution would be to fix this proper and implement a DM/DT based probing of the FPGA, including a proper driver(s) in drivers/fpga/ instead of putting all the crud into arch/arm/mach-socfpga ...
On 10/09/2018 05:03 AM, Ang, Chee Hong wrote:
On Mon, 2018-10-08 at 22:32 +0200, Marek Vasut wrote:
On 10/08/2018 05:10 PM, Ang, Chee Hong wrote:
On Mon, 2018-10-08 at 11:57 +0200, Marek Vasut wrote:
On 10/08/2018 11:48 AM, chee.hong.ang@intel.com wrote:
From: "Ang, Chee Hong" chee.hong.ang@intel.com
Enable 'fpga' command in u-boot. User will be able to use the fpga command to program the FPGA on Stratix10 SoC.
Signed-off-by: Ang, Chee Hong chee.hong.ang@intel.com
arch/arm/mach-socfpga/misc.c | 29 +++++++++++++++++++++++++++++ arch/arm/mach-socfpga/misc_s10.c | 2 ++ drivers/fpga/altera.c | 6 ++++++ include/altera.h | 4 ++++ 4 files changed, 41 insertions(+)
diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach- socfpga/misc.c index a4f6d5c..7986b58 100644 --- a/arch/arm/mach-socfpga/misc.c +++ b/arch/arm/mach-socfpga/misc.c @@ -88,6 +88,27 @@ int overwrite_console(void) #endif #ifdef CONFIG_FPGA +#ifdef CONFIG_FPGA_STRATIX10 +/*
- FPGA programming support for SoC FPGA Stratix 10
- */
+static Altera_desc altera_fpga[] = {
- {
/* Family */Intel_FPGA_Stratix10,/* Interface type */secure_device_manager_mailbox,/* No limitation as additional data will beignored */
-1,/* No device function table */NULL,/* Base interface address specified in driver*/
NULL,/* No cookie implementation */0- },
+}; +#else /* * FPGA programming support for SoC FPGA Cyclone V */ @@ -107,6 +128,7 @@ static Altera_desc altera_fpga[] = { 0 }, }; +#endif /* add device descriptor to FPGA device table */ void socfpga_fpga_add(void) @@ -116,6 +138,13 @@ void socfpga_fpga_add(void) for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) fpga_add(fpga_altera, &altera_fpga[i]); }
+#else
+__weak void socfpga_fpga_add(void) +{ +}
Why is a __weak function defined only in else-statement ?
It should be defined always, with a sane default implementation.
I will remove the empty function in #else-statement and define the default function like this :
/* add device descriptor to FPGA device table */ void socfpga_fpga_add(void) { #ifdef CONFIG_FPGA int i; fpga_init(); for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) fpga_add(fpga_altera, &altera_fpga[i]); #endif }
Is that OK?
Can't you have __weak empty implementation of socfpga_fpga_add() and implement a version per platform ? Would that work and make sense ?
socfpga_fpga_add() as shown above is a generic function for adding FPGA devices to FPGA driver which applies to all our platforms. This is the reason why it is defined in misc.c instead of misc_<platform_name>.c.
It turned out we already have this defined in misc.h: #ifdef CONFIG_FPGA void socfpga_fpga_add(void); #else static inline void socfpga_fpga_add(void) {} #endif
Right, if you had one socfpga_fpga_add() per platform + generic empty one, you could drop that whole thing ^.
So I don't think I need to make any changes to socfpga_fpga_add() in misc.c. I just have to remove ifdef CONFIG_FPGA in misc_s10.c because it was unnecessary. I will submit v3 for this patch and you can comment further. The v3 patch will be simpler. Thanks.
Please don't submit stuff before the discussion concluded, it's pointless.
btw. the best solution would be to fix this proper and implement a DM/DT based probing of the FPGA, including a proper driver(s) in drivers/fpga/ instead of putting all the crud into arch/arm/mach-socfpga ...
What do you think about this ^
On Tue, 2018-10-09 at 14:48 +0200, Marek Vasut wrote:
On 10/09/2018 05:03 AM, Ang, Chee Hong wrote:
On Mon, 2018-10-08 at 22:32 +0200, Marek Vasut wrote:
On 10/08/2018 05:10 PM, Ang, Chee Hong wrote:
On Mon, 2018-10-08 at 11:57 +0200, Marek Vasut wrote:
On 10/08/2018 11:48 AM, chee.hong.ang@intel.com wrote:
From: "Ang, Chee Hong" chee.hong.ang@intel.com
Enable 'fpga' command in u-boot. User will be able to use the fpga command to program the FPGA on Stratix10 SoC.
Signed-off-by: Ang, Chee Hong chee.hong.ang@intel.com
arch/arm/mach-socfpga/misc.c | 29 +++++++++++++++++++++++++++++ arch/arm/mach-socfpga/misc_s10.c | 2 ++ drivers/fpga/altera.c | 6 ++++++ include/altera.h | 4 ++++ 4 files changed, 41 insertions(+)
diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach- socfpga/misc.c index a4f6d5c..7986b58 100644 --- a/arch/arm/mach-socfpga/misc.c +++ b/arch/arm/mach-socfpga/misc.c @@ -88,6 +88,27 @@ int overwrite_console(void) #endif #ifdef CONFIG_FPGA +#ifdef CONFIG_FPGA_STRATIX10 +/*
- FPGA programming support for SoC FPGA Stratix 10
- */
+static Altera_desc altera_fpga[] = {
- {
/* Family */Intel_FPGA_Stratix10,/* Interface type */secure_device_manager_mailbox,/* No limitation as additional data willbe ignored */
-1,/* No device function table */NULL,/* Base interface address specified indriver */
NULL,/* No cookie implementation */0- },
+}; +#else /* * FPGA programming support for SoC FPGA Cyclone V */ @@ -107,6 +128,7 @@ static Altera_desc altera_fpga[] = { 0 }, }; +#endif /* add device descriptor to FPGA device table */ void socfpga_fpga_add(void) @@ -116,6 +138,13 @@ void socfpga_fpga_add(void) for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) fpga_add(fpga_altera, &altera_fpga[i]); }
+#else
+__weak void socfpga_fpga_add(void) +{ +}
Why is a __weak function defined only in else-statement ?
It should be defined always, with a sane default implementation.
I will remove the empty function in #else-statement and define the default function like this :
/* add device descriptor to FPGA device table */ void socfpga_fpga_add(void) { #ifdef CONFIG_FPGA int i; fpga_init(); for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) fpga_add(fpga_altera, &altera_fpga[i]); #endif }
Is that OK?
Can't you have __weak empty implementation of socfpga_fpga_add() and implement a version per platform ? Would that work and make sense ?
socfpga_fpga_add() as shown above is a generic function for adding FPGA devices to FPGA driver which applies to all our platforms. This is the reason why it is defined in misc.c instead of misc_<platform_name>.c.
It turned out we already have this defined in misc.h: #ifdef CONFIG_FPGA void socfpga_fpga_add(void); #else static inline void socfpga_fpga_add(void) {} #endif
Right, if you had one socfpga_fpga_add() per platform + generic empty one, you could drop that whole thing ^.
Yes. It's being addressed in v3 patch: https://lists.denx.de/pipermail/u-boot/2018-October/343561.html
So I don't think I need to make any changes to socfpga_fpga_add() in misc.c. I just have to remove ifdef CONFIG_FPGA in misc_s10.c because it was unnecessary. I will submit v3 for this patch and you can comment further. The v3 patch will be simpler. Thanks.
Please don't submit stuff before the discussion concluded, it's pointless.
OK.
btw. the best solution would be to fix this proper and implement a DM/DT based probing of the FPGA, including a proper driver(s) in drivers/fpga/ instead of putting all the crud into arch/arm/mach-socfpga ...
What do you think about this ^
I do agree DM/DT is the proper way to implement driver. But right now those FPGA drivers in drivers/fpga need to be at least call fpga_add() to add themselves into FPGA device table so that their callback functions can be invoked correctly when user issue 'fpga load', 'fpga info' at the command prompt. So in other words, all drivers in drivers/fpga rely on drivers/fpga/fpga.c (FPGA core driver) to work.
We already have all our fpga drivers in drivers/fpga : drivers/fpga/stratix10.c (NEW. In this patchset) drivers/fpga/stratixII.c (upstreamed) drivers/fpga/strixv.c (upstreamed) drivers/fpga/cyclon2.c (upstreamed) and others...
We only define the FPGA device structure in arch/arm/mach- socfpga/misc.c and call fpga_add() to add our FPGA device driver into the global FPGA device table then FPGA core driver will handle the FPGA operations by invoking the FPGA driver's callback functions.
So for proper DM/DT implementation, drivers/fpga/fpga.c need to be changed as well because this is the core of the FPGA driver.I think changing the core of the FPGA driver to support DM/DT would make more sense than I only change my FPGA driver to extract info from DTB file into a device structure then specifically call fpga_add() again to add the device structure to the FPGA core driver.
On 10/10/2018 07:30 AM, Ang, Chee Hong wrote:
On Tue, 2018-10-09 at 14:48 +0200, Marek Vasut wrote:
On 10/09/2018 05:03 AM, Ang, Chee Hong wrote:
On Mon, 2018-10-08 at 22:32 +0200, Marek Vasut wrote:
On 10/08/2018 05:10 PM, Ang, Chee Hong wrote:
On Mon, 2018-10-08 at 11:57 +0200, Marek Vasut wrote:
On 10/08/2018 11:48 AM, chee.hong.ang@intel.com wrote: > > > > From: "Ang, Chee Hong" chee.hong.ang@intel.com > > Enable 'fpga' command in u-boot. User will be able to use > the > fpga > command to program the FPGA on Stratix10 SoC. > > Signed-off-by: Ang, Chee Hong chee.hong.ang@intel.com > --- > arch/arm/mach-socfpga/misc.c | 29 > +++++++++++++++++++++++++++++ > arch/arm/mach-socfpga/misc_s10.c | 2 ++ > drivers/fpga/altera.c | 6 ++++++ > include/altera.h | 4 ++++ > 4 files changed, 41 insertions(+) > > diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach- > socfpga/misc.c > index a4f6d5c..7986b58 100644 > --- a/arch/arm/mach-socfpga/misc.c > +++ b/arch/arm/mach-socfpga/misc.c > @@ -88,6 +88,27 @@ int overwrite_console(void) > #endif > > #ifdef CONFIG_FPGA > +#ifdef CONFIG_FPGA_STRATIX10 > +/* > + * FPGA programming support for SoC FPGA Stratix 10 > + */ > +static Altera_desc altera_fpga[] = { > + { > + /* Family */ > + Intel_FPGA_Stratix10, > + /* Interface type */ > + secure_device_manager_mailbox, > + /* No limitation as additional data will > be > ignored */ > + -1, > + /* No device function table */ > + NULL, > + /* Base interface address specified in > driver > */ > + NULL, > + /* No cookie implementation */ > + 0 > + }, > +}; > +#else > /* > * FPGA programming support for SoC FPGA Cyclone V > */ > @@ -107,6 +128,7 @@ static Altera_desc altera_fpga[] = { > 0 > }, > }; > +#endif > > /* add device descriptor to FPGA device table */ > void socfpga_fpga_add(void) > @@ -116,6 +138,13 @@ void socfpga_fpga_add(void) > for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) > fpga_add(fpga_altera, &altera_fpga[i]); > } > + > +#else > + > +__weak void socfpga_fpga_add(void) > +{ > +} Why is a __weak function defined only in else-statement ?
It should be defined always, with a sane default implementation.
I will remove the empty function in #else-statement and define the default function like this :
/* add device descriptor to FPGA device table */ void socfpga_fpga_add(void) { #ifdef CONFIG_FPGA int i; fpga_init(); for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) fpga_add(fpga_altera, &altera_fpga[i]); #endif }
Is that OK?
Can't you have __weak empty implementation of socfpga_fpga_add() and implement a version per platform ? Would that work and make sense ?
socfpga_fpga_add() as shown above is a generic function for adding FPGA devices to FPGA driver which applies to all our platforms. This is the reason why it is defined in misc.c instead of misc_<platform_name>.c.
It turned out we already have this defined in misc.h: #ifdef CONFIG_FPGA void socfpga_fpga_add(void); #else static inline void socfpga_fpga_add(void) {} #endif
Right, if you had one socfpga_fpga_add() per platform + generic empty one, you could drop that whole thing ^.
Yes. It's being addressed in v3 patch: https://lists.denx.de/pipermail/u-boot/2018-October/343561.html
So where did the function go in there ? I don't see any __weak anything.
btw. the best solution would be to fix this proper and implement a DM/DT based probing of the FPGA, including a proper driver(s) in drivers/fpga/ instead of putting all the crud into arch/arm/mach-socfpga ...
What do you think about this ^
I do agree DM/DT is the proper way to implement driver. But right now those FPGA drivers in drivers/fpga need to be at least call fpga_add() to add themselves into FPGA device table so that their callback functions can be invoked correctly when user issue 'fpga load', 'fpga info' at the command prompt. So in other words, all drivers in drivers/fpga rely on drivers/fpga/fpga.c (FPGA core driver) to work.
Well, that should be fixed so that they probe from DT, just like any other driver. I'm not fond of adding stuff to arch/arm/ ...
We already have all our fpga drivers in drivers/fpga : drivers/fpga/stratix10.c (NEW. In this patchset) drivers/fpga/stratixII.c (upstreamed) drivers/fpga/strixv.c (upstreamed) drivers/fpga/cyclon2.c (upstreamed) and others...
We only define the FPGA device structure in arch/arm/mach- socfpga/misc.c and call fpga_add() to add our FPGA device driver into the global FPGA device table then FPGA core driver will handle the FPGA operations by invoking the FPGA driver's callback functions.
Right, which should be moved to drivers too and which should use DT.
So for proper DM/DT implementation, drivers/fpga/fpga.c need to be changed as well because this is the core of the FPGA driver.I think changing the core of the FPGA driver to support DM/DT would make more sense than I only change my FPGA driver to extract info from DTB file into a device structure then specifically call fpga_add() again to add the device structure to the FPGA core driver.
Yes, can you add it to your list once we flesh out this patchset ?
On Wed, 2018-10-10 at 12:27 +0200, Marek Vasut wrote:
On 10/10/2018 07:30 AM, Ang, Chee Hong wrote:
On Tue, 2018-10-09 at 14:48 +0200, Marek Vasut wrote:
On 10/09/2018 05:03 AM, Ang, Chee Hong wrote:
On Mon, 2018-10-08 at 22:32 +0200, Marek Vasut wrote:
On 10/08/2018 05:10 PM, Ang, Chee Hong wrote:
On Mon, 2018-10-08 at 11:57 +0200, Marek Vasut wrote: > > > > On 10/08/2018 11:48 AM, chee.hong.ang@intel.com wrote: > > > > > > > > > > From: "Ang, Chee Hong" chee.hong.ang@intel.com > > > > Enable 'fpga' command in u-boot. User will be able to > > use > > the > > fpga > > command to program the FPGA on Stratix10 SoC. > > > > Signed-off-by: Ang, Chee Hong chee.hong.ang@intel.com > > --- > > arch/arm/mach-socfpga/misc.c | 29 > > +++++++++++++++++++++++++++++ > > arch/arm/mach-socfpga/misc_s10.c | 2 ++ > > drivers/fpga/altera.c | 6 ++++++ > > include/altera.h | 4 ++++ > > 4 files changed, 41 insertions(+) > > > > diff --git a/arch/arm/mach-socfpga/misc.c > > b/arch/arm/mach- > > socfpga/misc.c > > index a4f6d5c..7986b58 100644 > > --- a/arch/arm/mach-socfpga/misc.c > > +++ b/arch/arm/mach-socfpga/misc.c > > @@ -88,6 +88,27 @@ int overwrite_console(void) > > #endif > > > > #ifdef CONFIG_FPGA > > +#ifdef CONFIG_FPGA_STRATIX10 > > +/* > > + * FPGA programming support for SoC FPGA Stratix 10 > > + */ > > +static Altera_desc altera_fpga[] = { > > + { > > + /* Family */ > > + Intel_FPGA_Stratix10, > > + /* Interface type */ > > + secure_device_manager_mailbox, > > + /* No limitation as additional data > > will > > be > > ignored */ > > + -1, > > + /* No device function table */ > > + NULL, > > + /* Base interface address specified in > > driver > > */ > > + NULL, > > + /* No cookie implementation */ > > + 0 > > + }, > > +}; > > +#else > > /* > > * FPGA programming support for SoC FPGA Cyclone V > > */ > > @@ -107,6 +128,7 @@ static Altera_desc altera_fpga[] = > > { > > 0 > > }, > > }; > > +#endif > > > > /* add device descriptor to FPGA device table */ > > void socfpga_fpga_add(void) > > @@ -116,6 +138,13 @@ void socfpga_fpga_add(void) > > for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) > > fpga_add(fpga_altera, > > &altera_fpga[i]); > > } > > + > > +#else > > + > > +__weak void socfpga_fpga_add(void) > > +{ > > +} > Why is a __weak function defined only in else-statement ? > > It should be defined always, with a sane default > implementation. I will remove the empty function in #else-statement and define the default function like this :
/* add device descriptor to FPGA device table */ void socfpga_fpga_add(void) { #ifdef CONFIG_FPGA int i; fpga_init(); for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) fpga_add(fpga_altera, &altera_fpga[i]); #endif }
Is that OK?
Can't you have __weak empty implementation of socfpga_fpga_add() and implement a version per platform ? Would that work and make sense ?
socfpga_fpga_add() as shown above is a generic function for adding FPGA devices to FPGA driver which applies to all our platforms. This is the reason why it is defined in misc.c instead of misc_<platform_name>.c.
It turned out we already have this defined in misc.h: #ifdef CONFIG_FPGA void socfpga_fpga_add(void); #else static inline void socfpga_fpga_add(void) {} #endif
Right, if you had one socfpga_fpga_add() per platform + generic empty one, you could drop that whole thing ^.
Yes. It's being addressed in v3 patch: https://lists.denx.de/pipermail/u-boot/2018-October/343561.html
So where did the function go in there ? I don't see any __weak anything.
I don't have to add anything in my v3 patchsets to make this work. It's already taken care by arch/arm/mach-socfpga/include/mach/misc.h as shown below:
#ifdef CONFIG_FPGA void socfpga_fpga_add(void); #else static inline void socfpga_fpga_add(void) {} #endif
An empty default socfpga_fpga_add() will be defined if CONFIG_FPGA is not defined.
btw. the best solution would be to fix this proper and implement a DM/DT based probing of the FPGA, including a proper driver(s) in drivers/fpga/ instead of putting all the crud into arch/arm/mach-socfpga ...
What do you think about this ^
I do agree DM/DT is the proper way to implement driver. But right now those FPGA drivers in drivers/fpga need to be at least call fpga_add() to add themselves into FPGA device table so that their callback functions can be invoked correctly when user issue 'fpga load', 'fpga info' at the command prompt. So in other words, all drivers in drivers/fpga rely on drivers/fpga/fpga.c (FPGA core driver) to work.
Well, that should be fixed so that they probe from DT, just like any other driver. I'm not fond of adding stuff to arch/arm/ ...
We already have all our fpga drivers in drivers/fpga : drivers/fpga/stratix10.c (NEW. In this patchset) drivers/fpga/stratixII.c (upstreamed) drivers/fpga/strixv.c (upstreamed) drivers/fpga/cyclon2.c (upstreamed) and others...
We only define the FPGA device structure in arch/arm/mach- socfpga/misc.c and call fpga_add() to add our FPGA device driver into the global FPGA device table then FPGA core driver will handle the FPGA operations by invoking the FPGA driver's callback functions.
Right, which should be moved to drivers too and which should use DT.
So for proper DM/DT implementation, drivers/fpga/fpga.c need to be changed as well because this is the core of the FPGA driver.I think changing the core of the FPGA driver to support DM/DT would make more sense than I only change my FPGA driver to extract info from DTB file into a device structure then specifically call fpga_add() again to add the device structure to the FPGA core driver.
Yes, can you add it to your list once we flesh out this patchset ?
OK.
On 10/11/2018 08:21 AM, Ang, Chee Hong wrote:
On Wed, 2018-10-10 at 12:27 +0200, Marek Vasut wrote:
On 10/10/2018 07:30 AM, Ang, Chee Hong wrote:
On Tue, 2018-10-09 at 14:48 +0200, Marek Vasut wrote:
On 10/09/2018 05:03 AM, Ang, Chee Hong wrote:
On Mon, 2018-10-08 at 22:32 +0200, Marek Vasut wrote:
On 10/08/2018 05:10 PM, Ang, Chee Hong wrote: > > > > On Mon, 2018-10-08 at 11:57 +0200, Marek Vasut wrote: >> >> >> >> On 10/08/2018 11:48 AM, chee.hong.ang@intel.com wrote: >>> >>> >>> >>> >>> From: "Ang, Chee Hong" chee.hong.ang@intel.com >>> >>> Enable 'fpga' command in u-boot. User will be able to >>> use >>> the >>> fpga >>> command to program the FPGA on Stratix10 SoC. >>> >>> Signed-off-by: Ang, Chee Hong chee.hong.ang@intel.com >>> --- >>> arch/arm/mach-socfpga/misc.c | 29 >>> +++++++++++++++++++++++++++++ >>> arch/arm/mach-socfpga/misc_s10.c | 2 ++ >>> drivers/fpga/altera.c | 6 ++++++ >>> include/altera.h | 4 ++++ >>> 4 files changed, 41 insertions(+) >>> >>> diff --git a/arch/arm/mach-socfpga/misc.c >>> b/arch/arm/mach- >>> socfpga/misc.c >>> index a4f6d5c..7986b58 100644 >>> --- a/arch/arm/mach-socfpga/misc.c >>> +++ b/arch/arm/mach-socfpga/misc.c >>> @@ -88,6 +88,27 @@ int overwrite_console(void) >>> #endif >>> >>> #ifdef CONFIG_FPGA >>> +#ifdef CONFIG_FPGA_STRATIX10 >>> +/* >>> + * FPGA programming support for SoC FPGA Stratix 10 >>> + */ >>> +static Altera_desc altera_fpga[] = { >>> + { >>> + /* Family */ >>> + Intel_FPGA_Stratix10, >>> + /* Interface type */ >>> + secure_device_manager_mailbox, >>> + /* No limitation as additional data >>> will >>> be >>> ignored */ >>> + -1, >>> + /* No device function table */ >>> + NULL, >>> + /* Base interface address specified in >>> driver >>> */ >>> + NULL, >>> + /* No cookie implementation */ >>> + 0 >>> + }, >>> +}; >>> +#else >>> /* >>> * FPGA programming support for SoC FPGA Cyclone V >>> */ >>> @@ -107,6 +128,7 @@ static Altera_desc altera_fpga[] = >>> { >>> 0 >>> }, >>> }; >>> +#endif >>> >>> /* add device descriptor to FPGA device table */ >>> void socfpga_fpga_add(void) >>> @@ -116,6 +138,13 @@ void socfpga_fpga_add(void) >>> for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) >>> fpga_add(fpga_altera, >>> &altera_fpga[i]); >>> } >>> + >>> +#else >>> + >>> +__weak void socfpga_fpga_add(void) >>> +{ >>> +} >> Why is a __weak function defined only in else-statement ? >> >> It should be defined always, with a sane default >> implementation. > I will remove the empty function in #else-statement and > define > the > default function like this : > > /* add device descriptor to FPGA device table */ > void socfpga_fpga_add(void) > { > #ifdef CONFIG_FPGA > int i; > fpga_init(); > for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) > fpga_add(fpga_altera, &altera_fpga[i]); > #endif > } > > Is that OK? Can't you have __weak empty implementation of socfpga_fpga_add() and implement a version per platform ? Would that work and make sense ?
socfpga_fpga_add() as shown above is a generic function for adding FPGA devices to FPGA driver which applies to all our platforms. This is the reason why it is defined in misc.c instead of misc_<platform_name>.c.
It turned out we already have this defined in misc.h: #ifdef CONFIG_FPGA void socfpga_fpga_add(void); #else static inline void socfpga_fpga_add(void) {} #endif
Right, if you had one socfpga_fpga_add() per platform + generic empty one, you could drop that whole thing ^.
Yes. It's being addressed in v3 patch: https://lists.denx.de/pipermail/u-boot/2018-October/343561.html
So where did the function go in there ? I don't see any __weak anything.
I don't have to add anything in my v3 patchsets to make this work. It's already taken care by arch/arm/mach-socfpga/include/mach/misc.h as shown below:
#ifdef CONFIG_FPGA void socfpga_fpga_add(void); #else static inline void socfpga_fpga_add(void) {} #endif
An empty default socfpga_fpga_add() will be defined if CONFIG_FPGA is not defined.
I was hoping to turn this into __weak function.
btw. the best solution would be to fix this proper and implement a DM/DT based probing of the FPGA, including a proper driver(s) in drivers/fpga/ instead of putting all the crud into arch/arm/mach-socfpga ...
What do you think about this ^
I do agree DM/DT is the proper way to implement driver. But right now those FPGA drivers in drivers/fpga need to be at least call fpga_add() to add themselves into FPGA device table so that their callback functions can be invoked correctly when user issue 'fpga load', 'fpga info' at the command prompt. So in other words, all drivers in drivers/fpga rely on drivers/fpga/fpga.c (FPGA core driver) to work.
Well, that should be fixed so that they probe from DT, just like any other driver. I'm not fond of adding stuff to arch/arm/ ...
We already have all our fpga drivers in drivers/fpga : drivers/fpga/stratix10.c (NEW. In this patchset) drivers/fpga/stratixII.c (upstreamed) drivers/fpga/strixv.c (upstreamed) drivers/fpga/cyclon2.c (upstreamed) and others...
We only define the FPGA device structure in arch/arm/mach- socfpga/misc.c and call fpga_add() to add our FPGA device driver into the global FPGA device table then FPGA core driver will handle the FPGA operations by invoking the FPGA driver's callback functions.
Right, which should be moved to drivers too and which should use DT.
So for proper DM/DT implementation, drivers/fpga/fpga.c need to be changed as well because this is the core of the FPGA driver.I think changing the core of the FPGA driver to support DM/DT would make more sense than I only change my FPGA driver to extract info from DTB file into a device structure then specifically call fpga_add() again to add the device structure to the FPGA core driver.
Yes, can you add it to your list once we flesh out this patchset ?
OK.
Thanks
On Thu, 2018-10-11 at 10:03 +0000, Marek Vasut wrote:
On 10/11/2018 08:21 AM, Ang, Chee Hong wrote:
On Wed, 2018-10-10 at 12:27 +0200, Marek Vasut wrote:
On 10/10/2018 07:30 AM, Ang, Chee Hong wrote:
On Tue, 2018-10-09 at 14:48 +0200, Marek Vasut wrote:
On 10/09/2018 05:03 AM, Ang, Chee Hong wrote:
On Mon, 2018-10-08 at 22:32 +0200, Marek Vasut wrote: > > > > On 10/08/2018 05:10 PM, Ang, Chee Hong wrote: > > > > > > > > > > On Mon, 2018-10-08 at 11:57 +0200, Marek Vasut wrote: > > > > > > > > > > > > > > > On 10/08/2018 11:48 AM, chee.hong.ang@intel.com > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > From: "Ang, Chee Hong" chee.hong.ang@intel.com > > > > > > > > Enable 'fpga' command in u-boot. User will be able > > > > to > > > > use > > > > the > > > > fpga > > > > command to program the FPGA on Stratix10 SoC. > > > > > > > > Signed-off-by: Ang, Chee Hong <chee.hong.ang@intel. > > > > com> > > > > --- > > > > arch/arm/mach-socfpga/misc.c | 29 > > > > +++++++++++++++++++++++++++++ > > > > arch/arm/mach-socfpga/misc_s10.c | 2 ++ > > > > drivers/fpga/altera.c | 6 ++++++ > > > > include/altera.h | 4 ++++ > > > > 4 files changed, 41 insertions(+) > > > > > > > > diff --git a/arch/arm/mach-socfpga/misc.c > > > > b/arch/arm/mach- > > > > socfpga/misc.c > > > > index a4f6d5c..7986b58 100644 > > > > --- a/arch/arm/mach-socfpga/misc.c > > > > +++ b/arch/arm/mach-socfpga/misc.c > > > > @@ -88,6 +88,27 @@ int overwrite_console(void) > > > > #endif > > > > > > > > #ifdef CONFIG_FPGA > > > > +#ifdef CONFIG_FPGA_STRATIX10 > > > > +/* > > > > + * FPGA programming support for SoC FPGA Stratix > > > > 10 > > > > + */ > > > > +static Altera_desc altera_fpga[] = { > > > > + { > > > > + /* Family */ > > > > + Intel_FPGA_Stratix10, > > > > + /* Interface type */ > > > > + secure_device_manager_mailbox, > > > > + /* No limitation as additional > > > > data > > > > will > > > > be > > > > ignored */ > > > > + -1, > > > > + /* No device function table */ > > > > + NULL, > > > > + /* Base interface address > > > > specified in > > > > driver > > > > */ > > > > + NULL, > > > > + /* No cookie implementation */ > > > > + 0 > > > > + }, > > > > +}; > > > > +#else > > > > /* > > > > * FPGA programming support for SoC FPGA Cyclone V > > > > */ > > > > @@ -107,6 +128,7 @@ static Altera_desc > > > > altera_fpga[] = > > > > { > > > > 0 > > > > }, > > > > }; > > > > +#endif > > > > > > > > /* add device descriptor to FPGA device table */ > > > > void socfpga_fpga_add(void) > > > > @@ -116,6 +138,13 @@ void socfpga_fpga_add(void) > > > > for (i = 0; i < ARRAY_SIZE(altera_fpga); > > > > i++) > > > > fpga_add(fpga_altera, > > > > &altera_fpga[i]); > > > > } > > > > + > > > > +#else > > > > + > > > > +__weak void socfpga_fpga_add(void) > > > > +{ > > > > +} > > > Why is a __weak function defined only in else- > > > statement ? > > > > > > It should be defined always, with a sane default > > > implementation. > > I will remove the empty function in #else-statement and > > define > > the > > default function like this : > > > > /* add device descriptor to FPGA device table */ > > void socfpga_fpga_add(void) > > { > > #ifdef CONFIG_FPGA > > int i; > > fpga_init(); > > for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) > > fpga_add(fpga_altera, &altera_fpga[i]); > > #endif > > } > > > > Is that OK? > Can't you have __weak empty implementation of > socfpga_fpga_add() > and > implement a version per platform ? Would that work and > make > sense > ? socfpga_fpga_add() as shown above is a generic function for adding FPGA devices to FPGA driver which applies to all our platforms. This is the reason why it is defined in misc.c instead of misc_<platform_name>.c.
It turned out we already have this defined in misc.h: #ifdef CONFIG_FPGA void socfpga_fpga_add(void); #else static inline void socfpga_fpga_add(void) {} #endif
Right, if you had one socfpga_fpga_add() per platform + generic empty one, you could drop that whole thing ^.
Yes. It's being addressed in v3 patch: https://lists.denx.de/pipermail/u-boot/2018-October/343561.html
So where did the function go in there ? I don't see any __weak anything.
I don't have to add anything in my v3 patchsets to make this work. It's already taken care by arch/arm/mach- socfpga/include/mach/misc.h as shown below:
#ifdef CONFIG_FPGA void socfpga_fpga_add(void); #else static inline void socfpga_fpga_add(void) {} #endif
An empty default socfpga_fpga_add() will be defined if CONFIG_FPGA is not defined.
I was hoping to turn this into __weak function.
Below are the new changes for new patch: Empty weak function in arch/arm/mach-socfpga/misc.c:
/* add device descriptor to FPGA device table */ __weak void socfpga_fpga_add(void) { }
In arch/arm/mach-socfpga/misc_aria10.c and arch/arm/mach- socfpga/misc_gen5.c:
#ifdef CONFIG_FPGA /* * FPGA programming support for SoC FPGA Cyclone V */ static Altera_desc altera_fpga[] = { { /* Family */ Altera_SoCFPGA, /* Interface type */ fast_passive_parallel, /* No limitation as additional data will be ignored */ -1, /* No device function table */ NULL, /* Base interface address specified in driver */ NULL, /* No cookie implementation */ 0 }, };
/* add device descriptor to FPGA device table */ void socfpga_fpga_add(void) { int i; fpga_init(); for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) fpga_add(fpga_altera, &altera_fpga[i]); } #endif
In arch/arm/mach-socfpga/misc_s10.c:
#ifdef CONFIG_FPGA /* * FPGA programming support for SoC FPGA Stratix 10 */ static Altera_desc altera_fpga[] = { { /* Family */ Intel_FPGA_Stratix10, /* Interface type */ secure_device_manager_mailbox, /* No limitation as additional data will be ignored */ -1, /* No device function table */ NULL, /* Base interface address specified in driver */ NULL, /* No cookie implementation */ 0 }, };
/* add device descriptor to FPGA device table */ void socfpga_fpga_add(void) { int i; fpga_init(); for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) fpga_add(fpga_altera, &altera_fpga[i]); } #endif
With this new implementation, each platform overrides the 'socfpga_fpga_add' to add its own fpga device. The problem here is since our aria10 and gen5 are adding same fpga device, there will be duplication of code for these 2 platforms. What do you think ? If you are OK with this implementation, I can submit a new patch for review again. Thanks.
> > btw. the best solution would be to fix this proper and > implement > a > DM/DT > based probing of the FPGA, including a proper driver(s) > in > drivers/fpga/ > instead of putting all the crud into arch/arm/mach- > socfpga > ...
What do you think about this ^
I do agree DM/DT is the proper way to implement driver. But right now those FPGA drivers in drivers/fpga need to be at least call fpga_add() to add themselves into FPGA device table so that their callback functions can be invoked correctly when user issue 'fpga load', 'fpga info' at the command prompt. So in other words, all drivers in drivers/fpga rely on drivers/fpga/fpga.c (FPGA core driver) to work.
Well, that should be fixed so that they probe from DT, just like any other driver. I'm not fond of adding stuff to arch/arm/ ...
We already have all our fpga drivers in drivers/fpga : drivers/fpga/stratix10.c (NEW. In this patchset) drivers/fpga/stratixII.c (upstreamed) drivers/fpga/strixv.c (upstreamed) drivers/fpga/cyclon2.c (upstreamed) and others...
We only define the FPGA device structure in arch/arm/mach- socfpga/misc.c and call fpga_add() to add our FPGA device driver into the global FPGA device table then FPGA core driver will handle the FPGA operations by invoking the FPGA driver's callback functions.
Right, which should be moved to drivers too and which should use DT.
So for proper DM/DT implementation, drivers/fpga/fpga.c need to be changed as well because this is the core of the FPGA driver.I think changing the core of the FPGA driver to support DM/DT would make more sense than I only change my FPGA driver to extract info from DTB file into a device structure then specifically call fpga_add() again to add the device structure to the FPGA core driver.
Yes, can you add it to your list once we flesh out this patchset ?
OK.
Thanks
-- Best regards, Marek Vasut
On 11/14/2018 08:09 AM, Ang, Chee Hong wrote:
On Thu, 2018-10-11 at 10:03 +0000, Marek Vasut wrote:
On 10/11/2018 08:21 AM, Ang, Chee Hong wrote:
On Wed, 2018-10-10 at 12:27 +0200, Marek Vasut wrote:
On 10/10/2018 07:30 AM, Ang, Chee Hong wrote:
On Tue, 2018-10-09 at 14:48 +0200, Marek Vasut wrote:
On 10/09/2018 05:03 AM, Ang, Chee Hong wrote: > > > > On Mon, 2018-10-08 at 22:32 +0200, Marek Vasut wrote: >> >> >> >> On 10/08/2018 05:10 PM, Ang, Chee Hong wrote: >>> >>> >>> >>> >>> On Mon, 2018-10-08 at 11:57 +0200, Marek Vasut wrote: >>>> >>>> >>>> >>>> >>>> On 10/08/2018 11:48 AM, chee.hong.ang@intel.com >>>> wrote: >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> From: "Ang, Chee Hong" chee.hong.ang@intel.com >>>>> >>>>> Enable 'fpga' command in u-boot. User will be able >>>>> to >>>>> use >>>>> the >>>>> fpga >>>>> command to program the FPGA on Stratix10 SoC. >>>>> >>>>> Signed-off-by: Ang, Chee Hong <chee.hong.ang@intel. >>>>> com> >>>>> --- >>>>> arch/arm/mach-socfpga/misc.c | 29 >>>>> +++++++++++++++++++++++++++++ >>>>> arch/arm/mach-socfpga/misc_s10.c | 2 ++ >>>>> drivers/fpga/altera.c | 6 ++++++ >>>>> include/altera.h | 4 ++++ >>>>> 4 files changed, 41 insertions(+) >>>>> >>>>> diff --git a/arch/arm/mach-socfpga/misc.c >>>>> b/arch/arm/mach- >>>>> socfpga/misc.c >>>>> index a4f6d5c..7986b58 100644 >>>>> --- a/arch/arm/mach-socfpga/misc.c >>>>> +++ b/arch/arm/mach-socfpga/misc.c >>>>> @@ -88,6 +88,27 @@ int overwrite_console(void) >>>>> #endif >>>>> >>>>> #ifdef CONFIG_FPGA >>>>> +#ifdef CONFIG_FPGA_STRATIX10 >>>>> +/* >>>>> + * FPGA programming support for SoC FPGA Stratix >>>>> 10 >>>>> + */ >>>>> +static Altera_desc altera_fpga[] = { >>>>> + { >>>>> + /* Family */ >>>>> + Intel_FPGA_Stratix10, >>>>> + /* Interface type */ >>>>> + secure_device_manager_mailbox, >>>>> + /* No limitation as additional >>>>> data >>>>> will >>>>> be >>>>> ignored */ >>>>> + -1, >>>>> + /* No device function table */ >>>>> + NULL, >>>>> + /* Base interface address >>>>> specified in >>>>> driver >>>>> */ >>>>> + NULL, >>>>> + /* No cookie implementation */ >>>>> + 0 >>>>> + }, >>>>> +}; >>>>> +#else >>>>> /* >>>>> * FPGA programming support for SoC FPGA Cyclone V >>>>> */ >>>>> @@ -107,6 +128,7 @@ static Altera_desc >>>>> altera_fpga[] = >>>>> { >>>>> 0 >>>>> }, >>>>> }; >>>>> +#endif >>>>> >>>>> /* add device descriptor to FPGA device table */ >>>>> void socfpga_fpga_add(void) >>>>> @@ -116,6 +138,13 @@ void socfpga_fpga_add(void) >>>>> for (i = 0; i < ARRAY_SIZE(altera_fpga); >>>>> i++) >>>>> fpga_add(fpga_altera, >>>>> &altera_fpga[i]); >>>>> } >>>>> + >>>>> +#else >>>>> + >>>>> +__weak void socfpga_fpga_add(void) >>>>> +{ >>>>> +} >>>> Why is a __weak function defined only in else- >>>> statement ? >>>> >>>> It should be defined always, with a sane default >>>> implementation. >>> I will remove the empty function in #else-statement and >>> define >>> the >>> default function like this : >>> >>> /* add device descriptor to FPGA device table */ >>> void socfpga_fpga_add(void) >>> { >>> #ifdef CONFIG_FPGA >>> int i; >>> fpga_init(); >>> for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) >>> fpga_add(fpga_altera, &altera_fpga[i]); >>> #endif >>> } >>> >>> Is that OK? >> Can't you have __weak empty implementation of >> socfpga_fpga_add() >> and >> implement a version per platform ? Would that work and >> make >> sense >> ? > socfpga_fpga_add() as shown above is a generic function for > adding > FPGA > devices to FPGA driver which applies to all our platforms. > This > is > the > reason why it is defined in misc.c instead of > misc_<platform_name>.c. > > It turned out we already have this defined in misc.h: > #ifdef CONFIG_FPGA > void socfpga_fpga_add(void); > #else > static inline void socfpga_fpga_add(void) {} > #endif Right, if you had one socfpga_fpga_add() per platform + generic empty one, you could drop that whole thing ^.
Yes. It's being addressed in v3 patch: https://lists.denx.de/pipermail/u-boot/2018-October/343561.html
So where did the function go in there ? I don't see any __weak anything.
I don't have to add anything in my v3 patchsets to make this work. It's already taken care by arch/arm/mach- socfpga/include/mach/misc.h as shown below:
#ifdef CONFIG_FPGA void socfpga_fpga_add(void); #else static inline void socfpga_fpga_add(void) {} #endif
An empty default socfpga_fpga_add() will be defined if CONFIG_FPGA is not defined.
I was hoping to turn this into __weak function.
Below are the new changes for new patch: Empty weak function in arch/arm/mach-socfpga/misc.c:
/* add device descriptor to FPGA device table */ __weak void socfpga_fpga_add(void) { }
In arch/arm/mach-socfpga/misc_aria10.c and arch/arm/mach- socfpga/misc_gen5.c:
#ifdef CONFIG_FPGA /* * FPGA programming support for SoC FPGA Cyclone V */ static Altera_desc altera_fpga[] = { { /* Family */ Altera_SoCFPGA, /* Interface type */ fast_passive_parallel, /* No limitation as additional data will be ignored */ -1, /* No device function table */ NULL, /* Base interface address specified in driver */ NULL, /* No cookie implementation */ 0 }, };
/* add device descriptor to FPGA device table */ void socfpga_fpga_add(void) { int i; fpga_init(); for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) fpga_add(fpga_altera, &altera_fpga[i]); } #endif
In arch/arm/mach-socfpga/misc_s10.c:
#ifdef CONFIG_FPGA /* * FPGA programming support for SoC FPGA Stratix 10 */ static Altera_desc altera_fpga[] = { { /* Family */ Intel_FPGA_Stratix10, /* Interface type */ secure_device_manager_mailbox, /* No limitation as additional data will be ignored */ -1, /* No device function table */ NULL, /* Base interface address specified in driver */ NULL, /* No cookie implementation */ 0 }, };
/* add device descriptor to FPGA device table */ void socfpga_fpga_add(void) { int i; fpga_init(); for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) fpga_add(fpga_altera, &altera_fpga[i]); } #endif
With this new implementation, each platform overrides the 'socfpga_fpga_add' to add its own fpga device. The problem here is since our aria10 and gen5 are adding same fpga device, there will be duplication of code for these 2 platforms. What do you think ?
I think you can create a common code for Gen5 somehow, right ?
If you are OK with this implementation, I can submit a new patch for review again. Thanks.
Submit the patches, yes.
On Wed, 2018-11-14 at 12:52 +0100, Marek Vasut wrote:
On 11/14/2018 08:09 AM, Ang, Chee Hong wrote:
On Thu, 2018-10-11 at 10:03 +0000, Marek Vasut wrote:
On 10/11/2018 08:21 AM, Ang, Chee Hong wrote:
On Wed, 2018-10-10 at 12:27 +0200, Marek Vasut wrote:
On 10/10/2018 07:30 AM, Ang, Chee Hong wrote:
On Tue, 2018-10-09 at 14:48 +0200, Marek Vasut wrote: > > > > On 10/09/2018 05:03 AM, Ang, Chee Hong wrote: > > > > > > > > > > On Mon, 2018-10-08 at 22:32 +0200, Marek Vasut wrote: > > > > > > > > > > > > > > > On 10/08/2018 05:10 PM, Ang, Chee Hong wrote: > > > > > > > > > > > > > > > > > > > > > > > > On Mon, 2018-10-08 at 11:57 +0200, Marek Vasut > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 10/08/2018 11:48 AM, chee.hong.ang@intel.com > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > From: "Ang, Chee Hong" <chee.hong.ang@intel.com > > > > > > > > > > > > > > > > > > > Enable 'fpga' command in u-boot. User will be > > > > > > able > > > > > > to > > > > > > use > > > > > > the > > > > > > fpga > > > > > > command to program the FPGA on Stratix10 SoC. > > > > > > > > > > > > Signed-off-by: Ang, Chee Hong <chee.hong.ang@in > > > > > > tel. > > > > > > com> > > > > > > --- > > > > > > arch/arm/mach-socfpga/misc.c | 29 > > > > > > +++++++++++++++++++++++++++++ > > > > > > arch/arm/mach-socfpga/misc_s10.c | 2 ++ > > > > > > drivers/fpga/altera.c | 6 ++++++ > > > > > > include/altera.h | 4 ++++ > > > > > > 4 files changed, 41 insertions(+) > > > > > > > > > > > > diff --git a/arch/arm/mach-socfpga/misc.c > > > > > > b/arch/arm/mach- > > > > > > socfpga/misc.c > > > > > > index a4f6d5c..7986b58 100644 > > > > > > --- a/arch/arm/mach-socfpga/misc.c > > > > > > +++ b/arch/arm/mach-socfpga/misc.c > > > > > > @@ -88,6 +88,27 @@ int overwrite_console(void) > > > > > > #endif > > > > > > > > > > > > #ifdef CONFIG_FPGA > > > > > > +#ifdef CONFIG_FPGA_STRATIX10 > > > > > > +/* > > > > > > + * FPGA programming support for SoC FPGA > > > > > > Stratix > > > > > > 10 > > > > > > + */ > > > > > > +static Altera_desc altera_fpga[] = { > > > > > > + { > > > > > > + /* Family */ > > > > > > + Intel_FPGA_Stratix10, > > > > > > + /* Interface type */ > > > > > > + secure_device_manager_mailbox, > > > > > > + /* No limitation as additional > > > > > > data > > > > > > will > > > > > > be > > > > > > ignored */ > > > > > > + -1, > > > > > > + /* No device function table */ > > > > > > + NULL, > > > > > > + /* Base interface address > > > > > > specified in > > > > > > driver > > > > > > */ > > > > > > + NULL, > > > > > > + /* No cookie implementation */ > > > > > > + 0 > > > > > > + }, > > > > > > +}; > > > > > > +#else > > > > > > /* > > > > > > * FPGA programming support for SoC FPGA > > > > > > Cyclone V > > > > > > */ > > > > > > @@ -107,6 +128,7 @@ static Altera_desc > > > > > > altera_fpga[] = > > > > > > { > > > > > > 0 > > > > > > }, > > > > > > }; > > > > > > +#endif > > > > > > > > > > > > /* add device descriptor to FPGA device table > > > > > > */ > > > > > > void socfpga_fpga_add(void) > > > > > > @@ -116,6 +138,13 @@ void > > > > > > socfpga_fpga_add(void) > > > > > > for (i = 0; i < > > > > > > ARRAY_SIZE(altera_fpga); > > > > > > i++) > > > > > > fpga_add(fpga_altera, > > > > > > &altera_fpga[i]); > > > > > > } > > > > > > + > > > > > > +#else > > > > > > + > > > > > > +__weak void socfpga_fpga_add(void) > > > > > > +{ > > > > > > +} > > > > > Why is a __weak function defined only in else- > > > > > statement ? > > > > > > > > > > It should be defined always, with a sane default > > > > > implementation. > > > > I will remove the empty function in #else-statement > > > > and > > > > define > > > > the > > > > default function like this : > > > > > > > > /* add device descriptor to FPGA device table */ > > > > void socfpga_fpga_add(void) > > > > { > > > > #ifdef CONFIG_FPGA > > > > int i; > > > > fpga_init(); > > > > for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) > > > > fpga_add(fpga_altera, &altera_fpga[i]); > > > > #endif > > > > } > > > > > > > > Is that OK? > > > Can't you have __weak empty implementation of > > > socfpga_fpga_add() > > > and > > > implement a version per platform ? Would that work > > > and > > > make > > > sense > > > ? > > socfpga_fpga_add() as shown above is a generic function > > for > > adding > > FPGA > > devices to FPGA driver which applies to all our > > platforms. > > This > > is > > the > > reason why it is defined in misc.c instead of > > misc_<platform_name>.c. > > > > It turned out we already have this defined in misc.h: > > #ifdef CONFIG_FPGA > > void socfpga_fpga_add(void); > > #else > > static inline void socfpga_fpga_add(void) {} > > #endif > Right, if you had one socfpga_fpga_add() per platform + > generic > empty > one, you could drop that whole thing ^. Yes. It's being addressed in v3 patch: https://lists.denx.de/pipermail/u-boot/2018-October/343561. html
So where did the function go in there ? I don't see any __weak anything.
I don't have to add anything in my v3 patchsets to make this work. It's already taken care by arch/arm/mach- socfpga/include/mach/misc.h as shown below:
#ifdef CONFIG_FPGA void socfpga_fpga_add(void); #else static inline void socfpga_fpga_add(void) {} #endif
An empty default socfpga_fpga_add() will be defined if CONFIG_FPGA is not defined.
I was hoping to turn this into __weak function.
Below are the new changes for new patch: Empty weak function in arch/arm/mach-socfpga/misc.c:
/* add device descriptor to FPGA device table */ __weak void socfpga_fpga_add(void) { }
In arch/arm/mach-socfpga/misc_aria10.c and arch/arm/mach- socfpga/misc_gen5.c:
#ifdef CONFIG_FPGA /* * FPGA programming support for SoC FPGA Cyclone V */ static Altera_desc altera_fpga[] = { { /* Family */ Altera_SoCFPGA, /* Interface type */ fast_passive_parallel, /* No limitation as additional data will be ignored */ -1, /* No device function table */ NULL, /* Base interface address specified in driver */ NULL, /* No cookie implementation */ 0 }, };
/* add device descriptor to FPGA device table */ void socfpga_fpga_add(void) { int i; fpga_init(); for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) fpga_add(fpga_altera, &altera_fpga[i]); } #endif
In arch/arm/mach-socfpga/misc_s10.c:
#ifdef CONFIG_FPGA /* * FPGA programming support for SoC FPGA Stratix 10 */ static Altera_desc altera_fpga[] = { { /* Family */ Intel_FPGA_Stratix10, /* Interface type */ secure_device_manager_mailbox, /* No limitation as additional data will be ignored */ -1, /* No device function table */ NULL, /* Base interface address specified in driver */ NULL, /* No cookie implementation */ 0 }, };
/* add device descriptor to FPGA device table */ void socfpga_fpga_add(void) { int i; fpga_init(); for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) fpga_add(fpga_altera, &altera_fpga[i]); } #endif
With this new implementation, each platform overrides the 'socfpga_fpga_add' to add its own fpga device. The problem here is since our aria10 and gen5 are adding same fpga device, there will be duplication of code for these 2 platforms. What do you think ?
I think you can create a common code for Gen5 somehow, right ?
I think I will add a new file arch/arm/mach-socfpga/fpga_devices.c and put the common code in it so that different platforms can share the common implementation which override the weak 'socfpga_fpga_add' function. The new file will have the following code:
#ifdef CONFIG_FPGA_STRATIX10 /* * FPGA programming support for SoC FPGA Stratix 10 */ static Altera_desc altera_fpga[] = { { /* Family */ Intel_FPGA_Stratix10, /* Interface type */ secure_device_manager_mailbox, /* No limitation as additional data will be ignored */ -1, /* No device function table */ NULL, /* Base interface address specified in driver */ NULL, /* No cookie implementation */ 0 }, }; #else /* * FPGA programming support for SoC FPGA Cyclone V */ static Altera_desc altera_fpga[] = { { /* Family */ Altera_SoCFPGA, /* Interface type */ fast_passive_parallel, /* No limitation as additional data will be ignored */ -1, /* No device function table */ NULL, /* Base interface address specified in driver */ NULL, /* No cookie implementation */ 0 }, }; #endif
/* add device descriptor to FPGA device table */ void socfpga_fpga_add(void) { int i; fpga_init(); for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) fpga_add(fpga_altera, &altera_fpga[i]); }
If you are OK with this implementation, I can submit a new patch for review again. Thanks.
Submit the patches, yes.
On 11/15/2018 08:13 AM, Ang, Chee Hong wrote:
On Wed, 2018-11-14 at 12:52 +0100, Marek Vasut wrote:
On 11/14/2018 08:09 AM, Ang, Chee Hong wrote:
On Thu, 2018-10-11 at 10:03 +0000, Marek Vasut wrote:
On 10/11/2018 08:21 AM, Ang, Chee Hong wrote:
On Wed, 2018-10-10 at 12:27 +0200, Marek Vasut wrote:
On 10/10/2018 07:30 AM, Ang, Chee Hong wrote: > > > > On Tue, 2018-10-09 at 14:48 +0200, Marek Vasut wrote: >> >> >> >> On 10/09/2018 05:03 AM, Ang, Chee Hong wrote: >>> >>> >>> >>> >>> On Mon, 2018-10-08 at 22:32 +0200, Marek Vasut wrote: >>>> >>>> >>>> >>>> >>>> On 10/08/2018 05:10 PM, Ang, Chee Hong wrote: >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> On Mon, 2018-10-08 at 11:57 +0200, Marek Vasut >>>>> wrote: >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> On 10/08/2018 11:48 AM, chee.hong.ang@intel.com >>>>>> wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> From: "Ang, Chee Hong" <chee.hong.ang@intel.com >>>>>>>> >>>>>>> >>>>>>> Enable 'fpga' command in u-boot. User will be >>>>>>> able >>>>>>> to >>>>>>> use >>>>>>> the >>>>>>> fpga >>>>>>> command to program the FPGA on Stratix10 SoC. >>>>>>> >>>>>>> Signed-off-by: Ang, Chee Hong <chee.hong.ang@in >>>>>>> tel. >>>>>>> com> >>>>>>> --- >>>>>>> arch/arm/mach-socfpga/misc.c | 29 >>>>>>> +++++++++++++++++++++++++++++ >>>>>>> arch/arm/mach-socfpga/misc_s10.c | 2 ++ >>>>>>> drivers/fpga/altera.c | 6 ++++++ >>>>>>> include/altera.h | 4 ++++ >>>>>>> 4 files changed, 41 insertions(+) >>>>>>> >>>>>>> diff --git a/arch/arm/mach-socfpga/misc.c >>>>>>> b/arch/arm/mach- >>>>>>> socfpga/misc.c >>>>>>> index a4f6d5c..7986b58 100644 >>>>>>> --- a/arch/arm/mach-socfpga/misc.c >>>>>>> +++ b/arch/arm/mach-socfpga/misc.c >>>>>>> @@ -88,6 +88,27 @@ int overwrite_console(void) >>>>>>> #endif >>>>>>> >>>>>>> #ifdef CONFIG_FPGA >>>>>>> +#ifdef CONFIG_FPGA_STRATIX10 >>>>>>> +/* >>>>>>> + * FPGA programming support for SoC FPGA >>>>>>> Stratix >>>>>>> 10 >>>>>>> + */ >>>>>>> +static Altera_desc altera_fpga[] = { >>>>>>> + { >>>>>>> + /* Family */ >>>>>>> + Intel_FPGA_Stratix10, >>>>>>> + /* Interface type */ >>>>>>> + secure_device_manager_mailbox, >>>>>>> + /* No limitation as additional >>>>>>> data >>>>>>> will >>>>>>> be >>>>>>> ignored */ >>>>>>> + -1, >>>>>>> + /* No device function table */ >>>>>>> + NULL, >>>>>>> + /* Base interface address >>>>>>> specified in >>>>>>> driver >>>>>>> */ >>>>>>> + NULL, >>>>>>> + /* No cookie implementation */ >>>>>>> + 0 >>>>>>> + }, >>>>>>> +}; >>>>>>> +#else >>>>>>> /* >>>>>>> * FPGA programming support for SoC FPGA >>>>>>> Cyclone V >>>>>>> */ >>>>>>> @@ -107,6 +128,7 @@ static Altera_desc >>>>>>> altera_fpga[] = >>>>>>> { >>>>>>> 0 >>>>>>> }, >>>>>>> }; >>>>>>> +#endif >>>>>>> >>>>>>> /* add device descriptor to FPGA device table >>>>>>> */ >>>>>>> void socfpga_fpga_add(void) >>>>>>> @@ -116,6 +138,13 @@ void >>>>>>> socfpga_fpga_add(void) >>>>>>> for (i = 0; i < >>>>>>> ARRAY_SIZE(altera_fpga); >>>>>>> i++) >>>>>>> fpga_add(fpga_altera, >>>>>>> &altera_fpga[i]); >>>>>>> } >>>>>>> + >>>>>>> +#else >>>>>>> + >>>>>>> +__weak void socfpga_fpga_add(void) >>>>>>> +{ >>>>>>> +} >>>>>> Why is a __weak function defined only in else- >>>>>> statement ? >>>>>> >>>>>> It should be defined always, with a sane default >>>>>> implementation. >>>>> I will remove the empty function in #else-statement >>>>> and >>>>> define >>>>> the >>>>> default function like this : >>>>> >>>>> /* add device descriptor to FPGA device table */ >>>>> void socfpga_fpga_add(void) >>>>> { >>>>> #ifdef CONFIG_FPGA >>>>> int i; >>>>> fpga_init(); >>>>> for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) >>>>> fpga_add(fpga_altera, &altera_fpga[i]); >>>>> #endif >>>>> } >>>>> >>>>> Is that OK? >>>> Can't you have __weak empty implementation of >>>> socfpga_fpga_add() >>>> and >>>> implement a version per platform ? Would that work >>>> and >>>> make >>>> sense >>>> ? >>> socfpga_fpga_add() as shown above is a generic function >>> for >>> adding >>> FPGA >>> devices to FPGA driver which applies to all our >>> platforms. >>> This >>> is >>> the >>> reason why it is defined in misc.c instead of >>> misc_<platform_name>.c. >>> >>> It turned out we already have this defined in misc.h: >>> #ifdef CONFIG_FPGA >>> void socfpga_fpga_add(void); >>> #else >>> static inline void socfpga_fpga_add(void) {} >>> #endif >> Right, if you had one socfpga_fpga_add() per platform + >> generic >> empty >> one, you could drop that whole thing ^. > Yes. It's being addressed in v3 patch: > https://lists.denx.de/pipermail/u-boot/2018-October/343561. > html So where did the function go in there ? I don't see any __weak anything.
I don't have to add anything in my v3 patchsets to make this work. It's already taken care by arch/arm/mach- socfpga/include/mach/misc.h as shown below:
#ifdef CONFIG_FPGA void socfpga_fpga_add(void); #else static inline void socfpga_fpga_add(void) {} #endif
An empty default socfpga_fpga_add() will be defined if CONFIG_FPGA is not defined.
I was hoping to turn this into __weak function.
Below are the new changes for new patch: Empty weak function in arch/arm/mach-socfpga/misc.c:
/* add device descriptor to FPGA device table */ __weak void socfpga_fpga_add(void) { }
In arch/arm/mach-socfpga/misc_aria10.c and arch/arm/mach- socfpga/misc_gen5.c:
#ifdef CONFIG_FPGA /* * FPGA programming support for SoC FPGA Cyclone V */ static Altera_desc altera_fpga[] = { { /* Family */ Altera_SoCFPGA, /* Interface type */ fast_passive_parallel, /* No limitation as additional data will be ignored */ -1, /* No device function table */ NULL, /* Base interface address specified in driver */ NULL, /* No cookie implementation */ 0 }, };
/* add device descriptor to FPGA device table */ void socfpga_fpga_add(void) { int i; fpga_init(); for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) fpga_add(fpga_altera, &altera_fpga[i]); } #endif
In arch/arm/mach-socfpga/misc_s10.c:
#ifdef CONFIG_FPGA /* * FPGA programming support for SoC FPGA Stratix 10 */ static Altera_desc altera_fpga[] = { { /* Family */ Intel_FPGA_Stratix10, /* Interface type */ secure_device_manager_mailbox, /* No limitation as additional data will be ignored */ -1, /* No device function table */ NULL, /* Base interface address specified in driver */ NULL, /* No cookie implementation */ 0 }, };
/* add device descriptor to FPGA device table */ void socfpga_fpga_add(void) { int i; fpga_init(); for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) fpga_add(fpga_altera, &altera_fpga[i]); } #endif
With this new implementation, each platform overrides the 'socfpga_fpga_add' to add its own fpga device. The problem here is since our aria10 and gen5 are adding same fpga device, there will be duplication of code for these 2 platforms. What do you think ?
I think you can create a common code for Gen5 somehow, right ?
I think I will add a new file arch/arm/mach-socfpga/fpga_devices.c and put the common code in it so that different platforms can share the common implementation which override the weak 'socfpga_fpga_add' function. The new file will have the following code:
#ifdef CONFIG_FPGA_STRATIX10 /* * FPGA programming support for SoC FPGA Stratix 10 */ static Altera_desc altera_fpga[] = { { /* Family */ Intel_FPGA_Stratix10, /* Interface type */ secure_device_manager_mailbox, /* No limitation as additional data will be ignored */ -1, /* No device function table */ NULL, /* Base interface address specified in driver */ NULL, /* No cookie implementation */ 0 }, }; #else /* * FPGA programming support for SoC FPGA Cyclone V */ static Altera_desc altera_fpga[] = { { /* Family */ Altera_SoCFPGA, /* Interface type */ fast_passive_parallel, /* No limitation as additional data will be ignored */ -1, /* No device function table */ NULL, /* Base interface address specified in driver */ NULL, /* No cookie implementation */ 0 }, }; #endif
/* add device descriptor to FPGA device table */ void socfpga_fpga_add(void) { int i; fpga_init(); for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) fpga_add(fpga_altera, &altera_fpga[i]); }
Better submit the whole patch, it's hard to review pieces.
From: "Ang, Chee Hong" chee.hong.ang@intel.com
Enable Stratix10 FPGA reconfiguration support in defconfig.
Signed-off-by: Ang, Chee Hong chee.hong.ang@intel.com --- configs/socfpga_stratix10_defconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/configs/socfpga_stratix10_defconfig b/configs/socfpga_stratix10_defconfig index 5f3d733..155c406 100644 --- a/configs/socfpga_stratix10_defconfig +++ b/configs/socfpga_stratix10_defconfig @@ -10,6 +10,7 @@ CONFIG_NR_DRAM_BANKS=1 CONFIG_BOOTDELAY=5 CONFIG_SPL_SPI_LOAD=y CONFIG_HUSH_PARSER=y +CONFIG_FPGA_STRATIX10=y CONFIG_SYS_PROMPT="SOCFPGA_STRATIX10 # " CONFIG_CMD_MEMTEST=y # CONFIG_CMD_FLASH is not set
participants (3)
-
Ang, Chee Hong -
chee.hong.ang＠intel.com
-
Marek Vasut