Re: [PATCH v7 4/4] fpga: versal-fpga: Add versal fpga manager driver

From: Tom Rix
Date: Fri Jun 04 2021 - 09:53:17 EST


On 6/4/21 4:48 AM, Greg KH wrote:
On Fri, Jun 04, 2021 at 05:03:32PM +0530, Nava kishore Manne wrote:
Add support for Xilinx Versal FPGA manager.

PDI source type can be DDR, OCM, QSPI flash etc..
But driver allocates memory always from DDR, Since driver supports only
DDR source type.

Signed-off-by: Appana Durga Kedareswara rao <appana.durga.rao@xxxxxxxxxx>
Signed-off-by: Nava kishore Manne <nava.manne@xxxxxxxxxx>
Reviewed-by: Moritz Fischer <mdf@xxxxxxxxxx>
---
Changes for v2:
-Updated the Fpga Mgr registrations call's
to 5.11
-Fixed some minor coding issues as suggested by
Moritz.

Changes for v3:
-Rewritten the Versal fpga Kconfig contents.

Changes for v4:
-Rebased the changes on linux-next.
No functional changes.

Changes for v5:
-None.

Changes for v6:
-None.

Changes for v7:
-Updated driver to remove unwated priv struct dependency.

drivers/fpga/Kconfig | 9 ++++
drivers/fpga/Makefile | 1 +
drivers/fpga/versal-fpga.c | 96 ++++++++++++++++++++++++++++++++++++++
3 files changed, 106 insertions(+)
create mode 100644 drivers/fpga/versal-fpga.c

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 33e15058d0dc..92c20b92357a 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -234,4 +234,13 @@ config FPGA_MGR_ZYNQMP_FPGA
to configure the programmable logic(PL) through PS
on ZynqMP SoC.
+config FPGA_MGR_VERSAL_FPGA
+ tristate "Xilinx Versal FPGA"
+ depends on ARCH_ZYNQMP || COMPILE_TEST
+ help
+ Select this option to enable FPGA manager driver support for
+ Xilinx Versal SoC. This driver uses the firmware interface to
+ configure the programmable logic(PL).
+
+ To compile this as a module, choose M here.
endif # FPGA
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 18dc9885883a..0bff783d1b61 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_FPGA_MGR_TS73XX) += ts73xx-fpga.o
obj-$(CONFIG_FPGA_MGR_XILINX_SPI) += xilinx-spi.o
obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA) += zynq-fpga.o
obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA) += zynqmp-fpga.o
+obj-$(CONFIG_FPGA_MGR_VERSAL_FPGA) += versal-fpga.o
obj-$(CONFIG_ALTERA_PR_IP_CORE) += altera-pr-ip-core.o
obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT) += altera-pr-ip-core-plat.o
diff --git a/drivers/fpga/versal-fpga.c b/drivers/fpga/versal-fpga.c
new file mode 100644
index 000000000000..1bd312a31b23
--- /dev/null
+++ b/drivers/fpga/versal-fpga.c
@@ -0,0 +1,96 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019-2021 Xilinx, Inc.
+ */
+
+#include <linux/dma-mapping.h>
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/string.h>
+#include <linux/firmware/xlnx-zynqmp.h>
+
+static int versal_fpga_ops_write_init(struct fpga_manager *mgr,
+ struct fpga_image_info *info,
+ const char *buf, size_t size)
+{
+ return 0;
Why have this if it does nothing?

+}
+
+static int versal_fpga_ops_write(struct fpga_manager *mgr,
+ const char *buf, size_t size)
+{
+ dma_addr_t dma_addr = 0;
+ char *kbuf;
+ int ret;
+
+ kbuf = dma_alloc_coherent(mgr->dev.parent, size, &dma_addr, GFP_KERNEL);
+ if (!kbuf)
+ return -ENOMEM;
+
+ memcpy(kbuf, buf, size);
+ ret = zynqmp_pm_load_pdi(PDI_SRC_DDR, dma_addr);
+ dma_free_coherent(mgr->dev.parent, size, kbuf, dma_addr);
+
+ return ret;
+}
+
+static int versal_fpga_ops_write_complete(struct fpga_manager *mgr,
+ struct fpga_image_info *info)
+{
+ return 0;
Same here, why have this at all?

+}
+
+static enum fpga_mgr_states versal_fpga_ops_state(struct fpga_manager *mgr)
+{
+ return FPGA_MGR_STATE_UNKNOWN;
Shouln't that be the default state of the fpga manager if there is no
state function callback?

This driver should just need a write and probe function, and that's it,
why make it more complex?

These empty functions are needed by each board because of an aggressive check of ops early in the bringup of the fpga mgr.

I did some deck chair shuffling here recently

https://lore.kernel.org/linux-fpga/20210524162820.2221474-1-trix@xxxxxxxxxx/

that shows location of check.

A finer grained handling of the ops would be better.

I'll add that to the next spin.

Tom

thanks,

greg k-h