Hi Tom,

	Please find my response inline.

-----Original Message-----
From: Tom Rix <trix@redhat.com>
Sent: Wednesday, July 7, 2021 3:04 AM
To: Nava kishore Manne <redacted>; robh+dt@kernel.org; Michal
Simek [off-list ref]; mdf@kernel.org; arnd@arndb.de; Rajan Vaja
[off-list ref]; gregkh@linuxfoundation.org; Amit Sunil Dhamne
[off-list ref]; Tejas Patel [off-list ref];
zou_wei@huawei.com; Sai Krishna Potthuri [off-list ref]; Ravi
Patel [off-list ref]; iwamatsu@nigauri.org; Jiaying Liang
[off-list ref]; devicetree@vger.kernel.org; linux-arm-
kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
fpga@vger.kernel.org; git [off-list ref]; chinnikishore369@gmail.com
Cc: Appana Durga Kedareswara Rao <redacted>
Subject: Re: [PATCH v8 5/5] fpga: versal-fpga: Add versal fpga manager driver


On 6/26/21 8:52 AM, Nava kishore Manne wrote:

quoted

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
[off-list ref]
Signed-off-by: Nava kishore Manne <redacted>
Reviewed-by: Moritz Fischer <mdf@kernel.org>
---
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.

Changes for v8:
               -None.

  drivers/fpga/Kconfig       |  9 ++++
  drivers/fpga/Makefile      |  1 +
  drivers/fpga/versal-fpga.c | 96

++++++++++++++++++++++++++++++++++++++

quoted

  3 files changed, 106 insertions(+)
  create mode 100644 drivers/fpga/versal-fpga.c

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index

8cd454ee20c0..16793bfc2bb4 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

Shouldn't this depend on ZYNQMP_FIRMWARE ?

Yes it has a dependency, will fix

quoted

+	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

quoted

  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

The other CONFIG_FPGA_MGR* configs are alphabetical, versal should
follow.

Will fix.

quoted

  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;
+}
+

These empty ops should go away with my wrappers patchset

Once your patches got integrated will post one more patch to remove this empty ops.

quoted

+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);

quoted

+	if (!kbuf)
+		return -ENOMEM;
+
+	memcpy(kbuf, buf, size);
+	ret = zynqmp_pm_load_pdi(PDI_SRC_DDR, dma_addr);

why isn't the size passed ?

Size is part of PDI images header users no need to pass this info explicitly.

quoted

+	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;
+}
+
+static enum fpga_mgr_states versal_fpga_ops_state(struct fpga_manager
+*mgr) {
+	return FPGA_MGR_STATE_UNKNOWN;
+}
+
+static const struct fpga_manager_ops versal_fpga_ops = {
+	.state = versal_fpga_ops_state,
+	.write_init = versal_fpga_ops_write_init,
+	.write = versal_fpga_ops_write,
+	.write_complete = versal_fpga_ops_write_complete, };
+
+static int versal_fpga_probe(struct platform_device *pdev) {
+	struct device *dev = &pdev->dev;
+	struct fpga_manager *mgr;
+	int ret;
+
+	ret = dma_set_mask_and_coherent(&pdev->dev,

DMA_BIT_MASK(32));

quoted

+	if (ret < 0) {
+		dev_err(dev, "no usable DMA configuration\n");
+		return ret;
+	}
+
+	mgr = devm_fpga_mgr_create(dev, "Xilinx Versal FPGA Manager",
+				   &versal_fpga_ops, NULL);
+	if (!mgr)
+		return -ENOMEM;
+
+	return devm_fpga_mgr_register(dev, mgr); }
+
+static const struct of_device_id versal_fpga_of_match[] = {
+	{ .compatible = "xlnx,versal-fpga", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, versal_fpga_of_match);

needs #if defined(CONFIG_OF) wrapper

Will fix.

quoted

+
+static struct platform_driver versal_fpga_driver = {
+	.probe = versal_fpga_probe,
+	.driver = {
+		.name = "versal_fpga_manager",
+		.of_match_table = of_match_ptr(versal_fpga_of_match),
+	},
+};
+module_platform_driver(versal_fpga_driver);
+
+MODULE_AUTHOR("Nava kishore Manne [off-list ref]");
+MODULE_AUTHOR("Appana Durga Kedareswara rao
+[off-list ref]");

Rao - needs to be capitalized ?

Will fix.