drivers: fpga: Added altera FPGA bridge support #71360

hardeepsharma95 · 2024-04-10T20:59:58Z

Added FPGA Bridge Enable/Disable functionality :

Adding support for Agilex & Agilex5

hardeepsharma95 · 2024-04-10T21:08:40Z

As discussed in the Architecture group meeting moved fpga bridges from subsystem to driver.
Old PR :: #65753

Kindly review this PR

tgorochowik · 2024-04-11T06:50:01Z

drivers/fpga/fpga_altera.c

+
+static int altera_fpga_on(const struct device *dev)
+{
+    int32_t ret = 0;


Incorrect indentation

tgorochowik · 2024-04-11T06:50:18Z

drivers/fpga/fpga_altera.c

+
+static int altera_fpga_init(const struct device *dev)
+{
+    if (!dev) {


tgorochowik · 2024-04-11T06:51:20Z

Looks nice in general, please fix the compliance checks first (note you can also run them locally, so you don't have to wait for the CI run to be approved with each iteration)

tgorochowik · 2024-04-11T06:58:29Z

drivers/fpga/fpga_altera.c

+static const struct fpga_driver_api altera_fpga_api = {
+	.on = altera_fpga_on,
+	.off = altera_fpga_off
+};


Unfortunately the subsystem currently requires the remaining callbacks to be defined, so that cannot go in like this.
At the very least we'd need to modify the common layer to allow NULLs for the rest and return -ENOTSUP. Otherwise you might see random issues when sb tries to use your driver

tgorochowik

If we want to allow having only selected capabilities, we need to expand the common layer imo, e.g, add checks for NULL here:

zephyr/include/zephyr/drivers/fpga.h

Lines 128 to 131 in 9b17a95

    
           const struct fpga_driver_api *api = 
        
           	(const struct fpga_driver_api *)dev->api; 
        
           return api->get_info(dev);

And return -ENOSUP there (in the common layer).

In addition we need to make sure the returned value is used wherever the driver is used (e.g. in the fpga shell implementation).

Otherwise I think it will too fragile.

tgorochowik · 2024-04-11T09:06:51Z

drivers/fpga/fpga_altera.c

+static const char *altera_fpga_get_info(const struct device *dev)
+{
+	ARG_UNUSED(dev);
+	return NULL;


I am afraid it still won't work, the FPGA layer is too limited at this point.

You'll get a NULL here:

zephyr/include/zephyr/drivers/fpga.h

Lines 126 to 132 in 9b17a95

static inline const char *fpga_get_info(const struct device *dev)

{

const struct fpga_driver_api *api =

(const struct fpga_driver_api *)dev->api;

return api->get_info(dev);

}

and it will propagate here:

zephyr/drivers/fpga/fpga_shell.c

Line 127 in 9b17a95

shell_print(sh, "%s", fpga_get_info(dev));

It is similar for other functions as well.

@tgorochowik Agree, NULL will be propagated to fpga_shell but we can't return -ENOSUP from the common layer i.e. fpga.h as the return type for the fpga_get_info function is of type "char *" or you are suggestion is to return "-ENOSUP" as string.
Either way, we have to return NULL or "-ENOSUP" either from the driver or from the common layer.

Is there a significant reason to return NULL or "-ENOTSUP" here?

For lack of a better option, you could return e.g. the crc of the bitstream if it is loaded, otherwise, an empty string.

zephyr/drivers/fpga/fpga_ice40.c

Line 495 in e3ff993

static const char *fpga_ice40_get_info(const struct device *dev)

Thanks @cfriedt for you valuable comment I agree with you we can return the empty string in this case

cfriedt · 2024-04-11T10:55:07Z

drivers/fpga/fpga_altera.c

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#define DT_DRV_COMPAT intel_fpga


I'm not a lawyer or anything, but as of 2024 Altera is again a separate corporate entity (still a subsidiary of Intel).

Would it make sense to maybe use altera_fpga here? If so, please ensure it's in dts/bindings/vendor-prefixes.txt

Also, given that this is an SoC FPGA (i.e. has hard-cores) and that there are many other FPGAs in the product line, I would suggest using a more differentiating DT compatible such as altera,agilex-socfpga-bridge.

Thanks for your suggestion @cfriedt I made the required changes

cfriedt · 2024-04-11T11:02:44Z

drivers/fpga/fpga_altera.c

+}
+
+/* Will send the SMC command to check the status of the FPGA */
+static int32_t is_fpga_config_not_ready(void)


It might be better to rename this so that it provides non-inverted logical operation (adjust return value as necessary).

cfriedt · 2024-04-11T11:06:20Z

drivers/fpga/fpga_altera.c

+	}
+
+	/* Check FPGA status before bridge enable/disable */
+	ret = is_fpga_config_not_ready();


I would suggest using a name that implies non-inverted logic

cfriedt · 2024-04-11T11:16:23Z

drivers/fpga/fpga_altera.c

+static const char *altera_fpga_get_info(const struct device *dev)
+{
+	ARG_UNUSED(dev);
+	return NULL;


Is there a significant reason to return NULL or "-ENOTSUP" here?

For lack of a better option, you could return e.g. the crc of the bitstream if it is loaded, otherwise, an empty string.

zephyr/drivers/fpga/fpga_ice40.c

Line 495 in e3ff993

static const char *fpga_ice40_get_info(const struct device *dev)

cfriedt · 2024-04-13T22:15:51Z

drivers/fpga/fpga_altera.h

+		 * is useful for matching responses to commands along with the ID
+		 */
+		uint32_t client_id : 4;
+	} header_t;


this is not a typedef so it probably makes sense to remove the _t

cfriedt · 2024-04-13T22:18:53Z

drivers/fpga/fpga_altera.h

+#include <zephyr/sip_svc/sip_svc.h>
+#include <zephyr/drivers/sip_svc/sip_svc_agilex_smc.h>


Are these two headers required by anything in this file? If not, please include them where they are needed.

cfriedt · 2024-04-13T22:20:26Z

dts/arm64/intel/intel_socfpga_agilex.dtsi

@@ -99,6 +99,10 @@
 		reg = <0x10000000 0x200000>;
 	};

+	fpga0: bridges {


Shouldn't there be an address associated with this device inside of the fpga SoC?

Actaully we are not directly interacting with any hardware we are calling sip_svc subsystem which further request the ATF to read and write in the bridge register so this bridge dont have any address

cfriedt · 2024-04-13T22:20:58Z

dts/arm64/intel/intel_socfpga_agilex5.dtsi

@@ -97,6 +97,10 @@
 		reg = <0x80100000 DT_SIZE_M(8)>;
 	};

+	fpga0: bridges {


Shouldn't there be an address associated with this device inside of the fpga SoC?

Same for here

cfriedt · 2024-04-13T22:21:51Z

drivers/fpga/fpga_altera.h

+};
+
+/* SIP SVC response private data */
+struct private_data_t {


This is not typedef'ed so please remove the _t

Suggested change

struct private_data_t {

struct private_data {

cfriedt · 2024-04-13T22:25:56Z

drivers/fpga/Kconfig.altera

+# Copyright (c) 2024, Intel Corporation. All rights reserved.
+# SPDX-License-Identifier: Apache-2.0
+
+config ALTERA_FPGA


Since this is only one type of interface (agilex bridge) it might be best to rename this option, because. Otherwise, it might be mistaken as support for all altera fpgas

cfriedt · 2024-04-13T22:26:35Z

drivers/fpga/fpga_altera.c

@@ -0,0 +1,463 @@
+/*


File name should have agilex bridge appended

cfriedt · 2024-04-19T10:33:51Z

drivers/fpga/fpga_altera_agilex_bridge.c

+static uint32_t mailbox_client_token;
+static struct sip_svc_controller *mailbox_smc_dev;


These are duplicates definitions and should be removed.

cfriedt · 2024-04-19T10:38:50Z

drivers/fpga/fpga_altera_agilex_bridge.c

+static uint32_t mailbox_client_token;
+static struct sip_svc_controller *mailbox_smc_dev;


The devicetree bindings for this driver should include a phandle property that refers to the sip_svc_controller.

That phandle / address should be stored as compile-time const data in a struct <driver>_config.

Similarly, the mailbox client token should be part of a struct <driver>_data assuming it is mutable and only runtime discoverable.

@cfriedt As discussed in the architecture review meeting, we have made a dummy node in the device tree for this driver as it doesn't need the information about the SiP SVC controller. As the SiP SVC device node will be
returned by "sip_svc_get_controller" function declared in "zephyr/sip_svc/sip_svc.h" file and it is a subsystem, not a driver.

Yes, for the other comment regarding keeping the information in the struct _data, we agree it should be kept the way you mentioned.

cfriedt · 2024-04-19T10:40:27Z

drivers/fpga/fpga_altera_agilex_bridge.c

+	.off = altera_fpga_off,
+};
+
+DEVICE_DT_DEFINE(DT_NODELABEL(fpga0), &altera_fpga_init, NULL, NULL, NULL, POST_KERNEL,


It would be preferable to instantiate this with a macro - DT_INST_FOREACH_STATUS_OKAY().

With that, we need an instance to be added to some devicetree in a build_all testsuite

cfriedt

Looks OK for me, with the caveat that the Arch WG approved of having no DT entry. It would be nice to have eventually, but it's not a blocking issue.

cfriedt · 2024-04-22T16:49:52Z

@hardeepsharma95 - I might have approved prematurely. We still need a "build_all" test that incorporates this driver into CI so that we know it's healthy.

Added altera FPGA bridge support Signed-off-by: Hardeep Sharma <hardeep.sharma@intel.com>

hardeepsharma95 · 2024-04-23T11:06:13Z

@hardeepsharma95 - I might have approved prematurely. We still need a "build_all" test that incorporates this driver into CI so that we know it's healthy.

done

cfriedt · 2024-04-23T11:12:28Z

tests/drivers/build_all/fpga/boards/intel_socfpga_agilex5_socdk.conf

+CONFIG_ALTERA_AGILEX_BRIDGE_FPGA=y
+CONFIG_ARM_SIP_SVC_DRIVER=y
+CONFIG_ARM_SIP_SVC_SUBSYS=y
+CONFIG_ARM_SIP_SVC_SUBSYS_SINGLY_OPEN=y
+CONFIG_HEAP_MEM_POOL_SIZE=16384


These should be added to prj.conf

cfriedt · 2024-04-23T11:13:47Z

tests/drivers/build_all/fpga/boards/intel_socfpga_agilex_socdk.conf

@@ -0,0 +1,9 @@
+# Copyright (c) 2024, Intel Corporation.


This file can probably be removed

cfriedt · 2024-04-23T11:19:46Z

tests/drivers/build_all/fpga/app.overlay

+&sip_smc {
+		status = "okay";
+		zephyr,num-clients = <2>;
+	};
+
+&fpga0 {
+	status = "okay";
+};


These nodes do not exist in native_posix or native_sim so they cannot be extended this way.

Please add the nodes directly in the devicetree.

Also, be sure to test with

twister -i -T tests/drivers/build_all/fpga

Altera FPGA driver should be built on a regular basis to ensure that there are no regressions Signed-off-by: Hardeep Sharma <hardeep.sharma@intel.com>

hardeepsharma95 · 2024-04-24T13:10:52Z

Hi @tgorochowik @cfriedt

Kindly review and approve this PR

hardeepsharma95 · 2024-04-25T11:55:17Z

Hi @tgorochowik @cfriedt

Kindly review and approve this PR

cfriedt

LGTM - thanks for your patience 🙏

hardeepsharma95 · 2024-04-26T04:54:43Z

Hello All ,
Kindly review and approve this PR

github-actions · 2024-04-26T07:31:04Z

Hi @hardeepsharma95!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

zephyrbot added area: ARM64 ARM (64-bit) Architecture area: FPGA Field-Programmable Gate Array (FPGA) platform: Intel SoC FPGA Agilex Intel Corporation, SoC FPGA Agilex labels Apr 10, 2024

zephyrbot requested review from carlocaione, cfriedt, fkokosinski, msierszulski, npitre, povergoing, SgrrZhf and tgorochowik April 10, 2024 21:00

zephyrbot assigned cfriedt Apr 10, 2024

hardeepsharma95 mentioned this pull request Apr 10, 2024

subsystem: fpga_bridge: Added FPGA Bridge #65753

Closed

tgorochowik reviewed Apr 11, 2024

View reviewed changes

tgorochowik requested changes Apr 11, 2024

View reviewed changes

hardeepsharma95 force-pushed the hps_bridges_latest branch 2 times, most recently from b657eaa to 4462789 Compare April 11, 2024 08:51

hardeepsharma95 requested a review from tgorochowik April 11, 2024 08:53

tgorochowik requested changes Apr 11, 2024

View reviewed changes

henrikbrixandersen self-requested a review April 11, 2024 10:49

cfriedt reviewed Apr 11, 2024

View reviewed changes

hardeepsharma95 force-pushed the hps_bridges_latest branch from 4462789 to 053b4e0 Compare April 12, 2024 06:25

hardeepsharma95 requested review from cfriedt, tanmaykathpalia and tgorochowik April 12, 2024 06:28

cfriedt reviewed Apr 13, 2024

View reviewed changes

hardeepsharma95 force-pushed the hps_bridges_latest branch from 053b4e0 to 264c764 Compare April 15, 2024 06:28

cfriedt reviewed Apr 19, 2024

View reviewed changes

hardeepsharma95 force-pushed the hps_bridges_latest branch 2 times, most recently from c4eb8da to 3466386 Compare April 21, 2024 17:33

zephyrbot added area: Shell Shell subsystem area: Samples Samples labels Apr 21, 2024

zephyrbot requested review from carlescufi, jakub-uC, kartben and nashif April 21, 2024 17:34

hardeepsharma95 force-pushed the hps_bridges_latest branch from 3466386 to 7f7de85 Compare April 21, 2024 17:39

hardeepsharma95 requested a review from cfriedt April 21, 2024 17:41

hardeepsharma95 force-pushed the hps_bridges_latest branch from 7f7de85 to eca7369 Compare April 21, 2024 17:43

cfriedt previously approved these changes Apr 22, 2024

View reviewed changes

drivers: fpga: Added altera FPGA bridge support

6b4f612

Added altera FPGA bridge support Signed-off-by: Hardeep Sharma <hardeep.sharma@intel.com>

hardeepsharma95 dismissed cfriedt’s stale review via 31dc1e8 April 23, 2024 11:04

hardeepsharma95 force-pushed the hps_bridges_latest branch from eca7369 to 31dc1e8 Compare April 23, 2024 11:04

hardeepsharma95 requested a review from cfriedt April 23, 2024 11:05

cfriedt reviewed Apr 23, 2024

View reviewed changes

tests: drivers: build_all: Build Altera FPGA driver

1a553c0

Altera FPGA driver should be built on a regular basis to ensure that there are no regressions Signed-off-by: Hardeep Sharma <hardeep.sharma@intel.com>

hardeepsharma95 force-pushed the hps_bridges_latest branch from 31dc1e8 to 1a553c0 Compare April 23, 2024 12:51

hardeepsharma95 requested a review from cfriedt April 23, 2024 12:55

cfriedt approved these changes Apr 25, 2024

View reviewed changes

tgorochowik approved these changes Apr 26, 2024

View reviewed changes

carlescufi merged commit 792fd57 into zephyrproject-rtos:main Apr 26, 2024
21 checks passed

	const struct fpga_driver_api *api =
	(const struct fpga_driver_api *)dev->api;

	return api->get_info(dev);

	static inline const char fpga_get_info(const struct device dev)
	{
	const struct fpga_driver_api *api =
	(const struct fpga_driver_api *)dev->api;

	return api->get_info(dev);
	}

		#include <zephyr/sip_svc/sip_svc.h>
		#include <zephyr/drivers/sip_svc/sip_svc_agilex_smc.h>

		static uint32_t mailbox_client_token;
		static struct sip_svc_controller *mailbox_smc_dev;

drivers: fpga: Added altera FPGA bridge support #71360

drivers: fpga: Added altera FPGA bridge support #71360

Conversation

hardeepsharma95 commented Apr 10, 2024

hardeepsharma95 commented Apr 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgorochowik commented Apr 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgorochowik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfriedt Apr 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfriedt Apr 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfriedt left a comment

Choose a reason for hiding this comment

cfriedt commented Apr 22, 2024

hardeepsharma95 commented Apr 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hardeepsharma95 commented Apr 24, 2024

hardeepsharma95 commented Apr 25, 2024

cfriedt left a comment

Choose a reason for hiding this comment

hardeepsharma95 commented Apr 26, 2024

github-actions bot commented Apr 26, 2024

cfriedt Apr 19, 2024 •

edited

cfriedt Apr 19, 2024 •

edited