[PATCH 1/3] drivers: firmware: Add Pdi load API support

[PATCH 1/3] drivers: firmware: Add Pdi load API support

[Nava kishore Manne]

This patch adds load pdi api support to enable pdi/partial loading from
linux. Programmable Device Image (PDI) is combination of headers, images
and bitstream files to be loaded. Partial PDI is partial set of image/
images to be loaded.

Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
---
 drivers/firmware/xilinx/zynqmp.c     | 17 +++++++++++++++++
 include/linux/firmware/xlnx-zynqmp.h |  9 +++++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
index 7eb9958662dd..a466225b9f9e 100644
--- a/drivers/firmware/xilinx/zynqmp.c
+++ b/drivers/firmware/xilinx/zynqmp.c
@@ -897,6 +897,23 @@ int zynqmp_pm_set_requirement(const u32 node, const u32 capabilities,
 }
 EXPORT_SYMBOL_GPL(zynqmp_pm_set_requirement);
 
+/**
+ * zynqmp_pm_load_pdi - Load and process pdi
+ * @src:       Source device where PDI is located
+ * @address:   Pdi src address
+ *
+ * This function provides support to load pdi from linux
+ *
+ * Return: Returns status, either success or error+reason
+ */
+int zynqmp_pm_load_pdi(const u32 src, const u64 address)
+{
+	return zynqmp_pm_invoke_fn(PM_LOAD_PDI, src,
+				   lower_32_bits(address),
+				   upper_32_bits(address), 0, NULL);
+}
+EXPORT_SYMBOL_GPL(zynqmp_pm_load_pdi);
+
 /**
  * zynqmp_pm_aes - Access AES hardware to encrypt/decrypt the data using
  * AES-GCM core.
diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h
index 2a0da841c942..87114ee645b1 100644
--- a/include/linux/firmware/xlnx-zynqmp.h
+++ b/include/linux/firmware/xlnx-zynqmp.h
@@ -52,6 +52,9 @@
 #define	ZYNQMP_PM_CAPABILITY_WAKEUP	0x4U
 #define	ZYNQMP_PM_CAPABILITY_UNUSABLE	0x8U
 
+/* Loader commands */
+#define PM_LOAD_PDI	0x701
+
 /*
  * Firmware FPGA Manager flags
  * XILINX_ZYNQMP_PM_FPGA_FULL:	FPGA full reconfiguration
@@ -354,6 +357,7 @@ int zynqmp_pm_write_pggs(u32 index, u32 value);
 int zynqmp_pm_read_pggs(u32 index, u32 *value);
 int zynqmp_pm_system_shutdown(const u32 type, const u32 subtype);
 int zynqmp_pm_set_boot_health_status(u32 value);
+int zynqmp_pm_load_pdi(const u32 src, const u64 address);
 #else
 static inline struct zynqmp_eemi_ops *zynqmp_pm_get_eemi_ops(void)
 {
@@ -538,6 +542,11 @@ static inline int zynqmp_pm_set_boot_health_status(u32 value)
 {
 	return -ENODEV;
 }
+
+static inline int zynqmp_pm_load_pdi(const u32 src, const u64 address)
+{
+	return -ENODEV;
+}
 #endif
 
 #endif /* __FIRMWARE_ZYNQMP_H__ */
-- 
2.18.0

[PATCH 1/3] drivers: firmware: Add Pdi load API support

[Nava kishore Manne]

This patch adds load pdi api support to enable pdi/partial loading from
linux. Programmable Device Image (PDI) is combination of headers, images
and bitstream files to be loaded. Partial PDI is partial set of image/
images to be loaded.

Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
---
 drivers/firmware/xilinx/zynqmp.c     | 17 +++++++++++++++++
 include/linux/firmware/xlnx-zynqmp.h |  9 +++++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
index 7eb9958662dd..a466225b9f9e 100644
--- a/drivers/firmware/xilinx/zynqmp.c
+++ b/drivers/firmware/xilinx/zynqmp.c
@@ -897,6 +897,23 @@ int zynqmp_pm_set_requirement(const u32 node, const u32 capabilities,
 }
 EXPORT_SYMBOL_GPL(zynqmp_pm_set_requirement);
 
+/**
+ * zynqmp_pm_load_pdi - Load and process pdi
+ * @src:       Source device where PDI is located
+ * @address:   Pdi src address
+ *
+ * This function provides support to load pdi from linux
+ *
+ * Return: Returns status, either success or error+reason
+ */
+int zynqmp_pm_load_pdi(const u32 src, const u64 address)
+{
+	return zynqmp_pm_invoke_fn(PM_LOAD_PDI, src,
+				   lower_32_bits(address),
+				   upper_32_bits(address), 0, NULL);
+}
+EXPORT_SYMBOL_GPL(zynqmp_pm_load_pdi);
+
 /**
  * zynqmp_pm_aes - Access AES hardware to encrypt/decrypt the data using
  * AES-GCM core.
diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h
index 2a0da841c942..87114ee645b1 100644
--- a/include/linux/firmware/xlnx-zynqmp.h
+++ b/include/linux/firmware/xlnx-zynqmp.h
@@ -52,6 +52,9 @@
 #define	ZYNQMP_PM_CAPABILITY_WAKEUP	0x4U
 #define	ZYNQMP_PM_CAPABILITY_UNUSABLE	0x8U
 
+/* Loader commands */
+#define PM_LOAD_PDI	0x701
+
 /*
  * Firmware FPGA Manager flags
  * XILINX_ZYNQMP_PM_FPGA_FULL:	FPGA full reconfiguration
@@ -354,6 +357,7 @@ int zynqmp_pm_write_pggs(u32 index, u32 value);
 int zynqmp_pm_read_pggs(u32 index, u32 *value);
 int zynqmp_pm_system_shutdown(const u32 type, const u32 subtype);
 int zynqmp_pm_set_boot_health_status(u32 value);
+int zynqmp_pm_load_pdi(const u32 src, const u64 address);
 #else
 static inline struct zynqmp_eemi_ops *zynqmp_pm_get_eemi_ops(void)
 {
@@ -538,6 +542,11 @@ static inline int zynqmp_pm_set_boot_health_status(u32 value)
 {
 	return -ENODEV;
 }
+
+static inline int zynqmp_pm_load_pdi(const u32 src, const u64 address)
+{
+	return -ENODEV;
+}
 #endif
 
 #endif /* __FIRMWARE_ZYNQMP_H__ */
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] dt-bindings: fpga: Add binding doc for versal fpga manager

[Nava kishore Manne]

From: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>

This patch adds binding doc for versal fpga manager driver.

Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
Signed-off-by: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>
---
 .../bindings/fpga/xlnx,versal-fpga.yaml       | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml

diff --git a/Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml b/Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml
new file mode 100644
index 000000000000..cf3aa7917488
--- /dev/null
+++ b/Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml
@@ -0,0 +1,33 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+
+$id: http://devicetree.org/schemas/fpga/xilinx/xlnx,versal-fpga.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Xilinx versal-fpga driver.
+
+maintainers:
+  - Nava kishore Manne <nava.manne@xilinx.com>
+
+description: |
+Device Tree versal-fpga bindings for the Versal SOC, Controlled
+using Versal SoC firmware interface.
+
+properties:
+  compatible:
+    items:
+      - enum:
+        - xlnx,versal-fpga
+
+required:
+  - compatible
+
+Required properties:
+- compatible: should contain "xlnx,versal-fpga"
+
+examples:
+  - |
+    versal_fpga: fpga {
+         compatible = "xlnx,versal-fpga";
+    };
-- 
2.18.0

[PATCH 2/3] dt-bindings: fpga: Add binding doc for versal fpga manager

[Nava kishore Manne]

From: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>

This patch adds binding doc for versal fpga manager driver.

Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
Signed-off-by: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>
---
 .../bindings/fpga/xlnx,versal-fpga.yaml       | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml

diff --git a/Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml b/Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml
new file mode 100644
index 000000000000..cf3aa7917488
--- /dev/null
+++ b/Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml
@@ -0,0 +1,33 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+
+$id: http://devicetree.org/schemas/fpga/xilinx/xlnx,versal-fpga.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Xilinx versal-fpga driver.
+
+maintainers:
+  - Nava kishore Manne <nava.manne@xilinx.com>
+
+description: |
+Device Tree versal-fpga bindings for the Versal SOC, Controlled
+using Versal SoC firmware interface.
+
+properties:
+  compatible:
+    items:
+      - enum:
+        - xlnx,versal-fpga
+
+required:
+  - compatible
+
+Required properties:
+- compatible: should contain "xlnx,versal-fpga"
+
+examples:
+  - |
+    versal_fpga: fpga {
+         compatible = "xlnx,versal-fpga";
+    };
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] fpga: versal-fpga: Add versal fpga manager driver

[Nava kishore Manne]

This patch adds driver for 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@xilinx.com>
Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
---
 drivers/fpga/Kconfig       |   8 ++
 drivers/fpga/Makefile      |   1 +
 drivers/fpga/versal-fpga.c | 149 +++++++++++++++++++++++++++++++++++++
 3 files changed, 158 insertions(+)
 create mode 100644 drivers/fpga/versal-fpga.c

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 5645226ca3ce..9f779c3a6739 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -216,4 +216,12 @@ 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 versal soc firmware
+          interface to load programmable logic(PL) images
+          on versal soc.
 endif # FPGA
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index d8e21dfc6778..40c9adb6a644 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..2a42aa78b182
--- /dev/null
+++ b/drivers/fpga/versal-fpga.c
@@ -0,0 +1,149 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 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>
+
+/* Constant Definitions */
+#define PDI_SOURCE_TYPE	0xF
+
+/**
+ * struct versal_fpga_priv - Private data structure
+ * @dev:	Device data structure
+ * @flags:	flags which is used to identify the PL Image type
+ */
+struct versal_fpga_priv {
+	struct device *dev;
+	u32 flags;
+};
+
+static int versal_fpga_ops_write_init(struct fpga_manager *mgr,
+				      struct fpga_image_info *info,
+				      const char *buf, size_t size)
+{
+	struct versal_fpga_priv *priv;
+
+	priv = mgr->priv;
+	priv->flags = info->flags;
+
+	return 0;
+}
+
+static int versal_fpga_ops_write(struct fpga_manager *mgr,
+				 const char *buf, size_t size)
+{
+	struct versal_fpga_priv *priv;
+	dma_addr_t dma_addr = 0;
+	char *kbuf;
+	int ret;
+
+	priv = mgr->priv;
+
+	kbuf = dma_alloc_coherent(priv->dev, size, &dma_addr, GFP_KERNEL);
+	if (!kbuf)
+		return -ENOMEM;
+
+	memcpy(kbuf, buf, size);
+
+	wmb(); /* ensure all writes are done before initiate FW call */
+
+	ret = zynqmp_pm_load_pdi(PDI_SOURCE_TYPE, dma_addr);
+
+	dma_free_coherent(priv->dev, 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_OPERATING;
+}
+
+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 versal_fpga_priv *priv;
+	struct fpga_manager *mgr;
+	int err, ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
+	if (ret < 0) {
+		dev_err(dev, "no usable DMA configuration");
+		return ret;
+	}
+
+	mgr = devm_fpga_mgr_create(dev, "Xilinx Versal FPGA Manager",
+				   &versal_fpga_ops, priv);
+	if (!mgr)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, mgr);
+
+	err = fpga_mgr_register(mgr);
+	if (err) {
+		dev_err(dev, "unable to register FPGA manager");
+		fpga_mgr_free(mgr);
+		return err;
+	}
+
+	return 0;
+}
+
+static int versal_fpga_remove(struct platform_device *pdev)
+{
+	struct fpga_manager *mgr = platform_get_drvdata(pdev);
+
+	fpga_mgr_unregister(mgr);
+	fpga_mgr_free(mgr);
+
+	return 0;
+}
+
+static const struct of_device_id versal_fpga_of_match[] = {
+	{ .compatible = "xlnx,versal-fpga", },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, versal_fpga_of_match);
+
+static struct platform_driver versal_fpga_driver = {
+	.probe = versal_fpga_probe,
+	.remove = versal_fpga_remove,
+	.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 <nava.manne@xilinx.com>");
+MODULE_AUTHOR("Appana Durga Kedareswara rao <appanad.durga.rao@xilinx.com>");
+MODULE_DESCRIPTION("Xilinx Versal FPGA Manager");
+MODULE_LICENSE("GPL");
-- 
2.18.0

[PATCH 3/3] fpga: versal-fpga: Add versal fpga manager driver

[Nava kishore Manne]

This patch adds driver for 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@xilinx.com>
Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
---
 drivers/fpga/Kconfig       |   8 ++
 drivers/fpga/Makefile      |   1 +
 drivers/fpga/versal-fpga.c | 149 +++++++++++++++++++++++++++++++++++++
 3 files changed, 158 insertions(+)
 create mode 100644 drivers/fpga/versal-fpga.c

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 5645226ca3ce..9f779c3a6739 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -216,4 +216,12 @@ 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 versal soc firmware
+          interface to load programmable logic(PL) images
+          on versal soc.
 endif # FPGA
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index d8e21dfc6778..40c9adb6a644 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..2a42aa78b182
--- /dev/null
+++ b/drivers/fpga/versal-fpga.c
@@ -0,0 +1,149 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 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>
+
+/* Constant Definitions */
+#define PDI_SOURCE_TYPE	0xF
+
+/**
+ * struct versal_fpga_priv - Private data structure
+ * @dev:	Device data structure
+ * @flags:	flags which is used to identify the PL Image type
+ */
+struct versal_fpga_priv {
+	struct device *dev;
+	u32 flags;
+};
+
+static int versal_fpga_ops_write_init(struct fpga_manager *mgr,
+				      struct fpga_image_info *info,
+				      const char *buf, size_t size)
+{
+	struct versal_fpga_priv *priv;
+
+	priv = mgr->priv;
+	priv->flags = info->flags;
+
+	return 0;
+}
+
+static int versal_fpga_ops_write(struct fpga_manager *mgr,
+				 const char *buf, size_t size)
+{
+	struct versal_fpga_priv *priv;
+	dma_addr_t dma_addr = 0;
+	char *kbuf;
+	int ret;
+
+	priv = mgr->priv;
+
+	kbuf = dma_alloc_coherent(priv->dev, size, &dma_addr, GFP_KERNEL);
+	if (!kbuf)
+		return -ENOMEM;
+
+	memcpy(kbuf, buf, size);
+
+	wmb(); /* ensure all writes are done before initiate FW call */
+
+	ret = zynqmp_pm_load_pdi(PDI_SOURCE_TYPE, dma_addr);
+
+	dma_free_coherent(priv->dev, 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_OPERATING;
+}
+
+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 versal_fpga_priv *priv;
+	struct fpga_manager *mgr;
+	int err, ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
+	if (ret < 0) {
+		dev_err(dev, "no usable DMA configuration");
+		return ret;
+	}
+
+	mgr = devm_fpga_mgr_create(dev, "Xilinx Versal FPGA Manager",
+				   &versal_fpga_ops, priv);
+	if (!mgr)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, mgr);
+
+	err = fpga_mgr_register(mgr);
+	if (err) {
+		dev_err(dev, "unable to register FPGA manager");
+		fpga_mgr_free(mgr);
+		return err;
+	}
+
+	return 0;
+}
+
+static int versal_fpga_remove(struct platform_device *pdev)
+{
+	struct fpga_manager *mgr = platform_get_drvdata(pdev);
+
+	fpga_mgr_unregister(mgr);
+	fpga_mgr_free(mgr);
+
+	return 0;
+}
+
+static const struct of_device_id versal_fpga_of_match[] = {
+	{ .compatible = "xlnx,versal-fpga", },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, versal_fpga_of_match);
+
+static struct platform_driver versal_fpga_driver = {
+	.probe = versal_fpga_probe,
+	.remove = versal_fpga_remove,
+	.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 <nava.manne@xilinx.com>");
+MODULE_AUTHOR("Appana Durga Kedareswara rao <appanad.durga.rao@xilinx.com>");
+MODULE_DESCRIPTION("Xilinx Versal FPGA Manager");
+MODULE_LICENSE("GPL");
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] dt-bindings: fpga: Add binding doc for versal fpga manager

[Michal Simek]

On 1/18/21 3:43 AM, Nava kishore Manne wrote:
> From: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>
> 
> This patch adds binding doc for versal fpga manager driver.
> 
> Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
> Signed-off-by: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>
> ---
>  .../bindings/fpga/xlnx,versal-fpga.yaml       | 33 +++++++++++++++++++
>  1 file changed, 33 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml
> 
> diff --git a/Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml b/Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml
> new file mode 100644
> index 000000000000..cf3aa7917488
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml
> @@ -0,0 +1,33 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +
> +$id: http://devicetree.org/schemas/fpga/xilinx/xlnx,versal-fpga.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xilinx versal-fpga driver.
> +
> +maintainers:
> +  - Nava kishore Manne <nava.manne@xilinx.com>
> +
> +description: |
> +Device Tree versal-fpga bindings for the Versal SOC, Controlled
> +using Versal SoC firmware interface.
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +        - xlnx,versal-fpga
> +
> +required:
> +  - compatible
> +
> +Required properties:
> +- compatible: should contain "xlnx,versal-fpga"
> +
> +examples:
> +  - |
> +    versal_fpga: fpga {
> +         compatible = "xlnx,versal-fpga";
> +    };
> 

There are issues with the binding
Run
make
DT_SCHEMA_FILES=Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml
dt_binding_check

and fix
./Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml:15:1:
[error] syntax error: could not find expected ':' (syntax)
and maybe others.

Thanks,
Michal

Re: [PATCH 2/3] dt-bindings: fpga: Add binding doc for versal fpga manager

[Michal Simek]

On 1/18/21 3:43 AM, Nava kishore Manne wrote:
> From: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>
> 
> This patch adds binding doc for versal fpga manager driver.
> 
> Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
> Signed-off-by: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>
> ---
>  .../bindings/fpga/xlnx,versal-fpga.yaml       | 33 +++++++++++++++++++
>  1 file changed, 33 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml
> 
> diff --git a/Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml b/Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml
> new file mode 100644
> index 000000000000..cf3aa7917488
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml
> @@ -0,0 +1,33 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +
> +$id: http://devicetree.org/schemas/fpga/xilinx/xlnx,versal-fpga.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xilinx versal-fpga driver.
> +
> +maintainers:
> +  - Nava kishore Manne <nava.manne@xilinx.com>
> +
> +description: |
> +Device Tree versal-fpga bindings for the Versal SOC, Controlled
> +using Versal SoC firmware interface.
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +        - xlnx,versal-fpga
> +
> +required:
> +  - compatible
> +
> +Required properties:
> +- compatible: should contain "xlnx,versal-fpga"
> +
> +examples:
> +  - |
> +    versal_fpga: fpga {
> +         compatible = "xlnx,versal-fpga";
> +    };
> 

There are issues with the binding
Run
make
DT_SCHEMA_FILES=Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml
dt_binding_check

and fix
./Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml:15:1:
[error] syntax error: could not find expected ':' (syntax)
and maybe others.

Thanks,
Michal


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] dt-bindings: fpga: Add binding doc for versal fpga manager

[Rob Herring]

On Mon, 18 Jan 2021 08:13:17 +0530, Nava kishore Manne wrote:
> From: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>
> 
> This patch adds binding doc for versal fpga manager driver.
> 
> Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
> Signed-off-by: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>
> ---
>  .../bindings/fpga/xlnx,versal-fpga.yaml       | 33 +++++++++++++++++++
>  1 file changed, 33 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml:15:1: [error] syntax error: could not find expected ':' (syntax)

dtschema/dtc warnings/errors:
make[1]: *** Deleting file 'Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.example.dts'
Traceback (most recent call last):
  File "/usr/local/bin/dt-extract-example", line 45, in <module>
    binding = yaml.load(open(args.yamlfile, encoding='utf-8').read())
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/main.py", line 343, in load
    return constructor.get_single_data()
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 111, in get_single_data
    node = self.composer.get_single_node()
  File "_ruamel_yaml.pyx", line 706, in _ruamel_yaml.CParser.get_single_node
  File "_ruamel_yaml.pyx", line 724, in _ruamel_yaml.CParser._compose_document
  File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node
  File "_ruamel_yaml.pyx", line 891, in _ruamel_yaml.CParser._compose_mapping_node
  File "_ruamel_yaml.pyx", line 904, in _ruamel_yaml.CParser._parse_next_event
ruamel.yaml.scanner.ScannerError: while scanning a simple key
  in "<unicode string>", line 14, column 1
could not find expected ':'
  in "<unicode string>", line 15, column 1
make[1]: *** [Documentation/devicetree/bindings/Makefile:20: Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml:  while scanning a simple key
  in "<unicode string>", line 14, column 1
could not find expected ':'
  in "<unicode string>", line 15, column 1
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml: ignoring, error parsing file
warning: no schema found in file: ./Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml
make: *** [Makefile:1370: dt_binding_check] Error 2

See https://patchwork.ozlabs.org/patch/1427979

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 2/3] dt-bindings: fpga: Add binding doc for versal fpga manager

[Rob Herring]

On Mon, 18 Jan 2021 08:13:17 +0530, Nava kishore Manne wrote:
> From: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>
> 
> This patch adds binding doc for versal fpga manager driver.
> 
> Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
> Signed-off-by: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>
> ---
>  .../bindings/fpga/xlnx,versal-fpga.yaml       | 33 +++++++++++++++++++
>  1 file changed, 33 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml:15:1: [error] syntax error: could not find expected ':' (syntax)

dtschema/dtc warnings/errors:
make[1]: *** Deleting file 'Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.example.dts'
Traceback (most recent call last):
  File "/usr/local/bin/dt-extract-example", line 45, in <module>
    binding = yaml.load(open(args.yamlfile, encoding='utf-8').read())
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/main.py", line 343, in load
    return constructor.get_single_data()
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 111, in get_single_data
    node = self.composer.get_single_node()
  File "_ruamel_yaml.pyx", line 706, in _ruamel_yaml.CParser.get_single_node
  File "_ruamel_yaml.pyx", line 724, in _ruamel_yaml.CParser._compose_document
  File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node
  File "_ruamel_yaml.pyx", line 891, in _ruamel_yaml.CParser._compose_mapping_node
  File "_ruamel_yaml.pyx", line 904, in _ruamel_yaml.CParser._parse_next_event
ruamel.yaml.scanner.ScannerError: while scanning a simple key
  in "<unicode string>", line 14, column 1
could not find expected ':'
  in "<unicode string>", line 15, column 1
make[1]: *** [Documentation/devicetree/bindings/Makefile:20: Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml:  while scanning a simple key
  in "<unicode string>", line 14, column 1
could not find expected ':'
  in "<unicode string>", line 15, column 1
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml: ignoring, error parsing file
warning: no schema found in file: ./Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml
make: *** [Makefile:1370: dt_binding_check] Error 2

See https://patchwork.ozlabs.org/patch/1427979

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Moritz Fischer]

Hi Nava,

On Mon, Jan 18, 2021 at 08:13:18AM +0530, Nava kishore Manne wrote:
> This patch adds driver for versal fpga manager.
Nit: Add support for Xilinx Versal FPGA manager
> 
> PDI source type can be DDR, OCM, QSPI flash etc..
No idea what PDI is :)
> But driver allocates memory always from DDR, Since driver supports only
> DDR source type.
> 
> Signed-off-by: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>
> Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
> ---
>  drivers/fpga/Kconfig       |   8 ++
>  drivers/fpga/Makefile      |   1 +
>  drivers/fpga/versal-fpga.c | 149 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 158 insertions(+)
>  create mode 100644 drivers/fpga/versal-fpga.c
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 5645226ca3ce..9f779c3a6739 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -216,4 +216,12 @@ 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 versal soc firmware
> +          interface to load programmable logic(PL) images
> +          on versal soc.
>  endif # FPGA
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index d8e21dfc6778..40c9adb6a644 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..2a42aa78b182
> --- /dev/null
> +++ b/drivers/fpga/versal-fpga.c
> @@ -0,0 +1,149 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 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>
> +
> +/* Constant Definitions */
> +#define PDI_SOURCE_TYPE	0xF
> +
> +/**
> + * struct versal_fpga_priv - Private data structure
> + * @dev:	Device data structure
> + * @flags:	flags which is used to identify the PL Image type
> + */
> +struct versal_fpga_priv {
> +	struct device *dev;
> +	u32 flags;
This seems unused ... please introduce them when/if you start using
them.
> +};
> +
> +static int versal_fpga_ops_write_init(struct fpga_manager *mgr,
> +				      struct fpga_image_info *info,
> +				      const char *buf, size_t size)
> +{
> +	struct versal_fpga_priv *priv;
> +
> +	priv = mgr->priv;
> +	priv->flags = info->flags;
? What uses this ? It seems this function could just be 'return 0' right
now.
> +
> +	return 0;
> +}
> +
> +static int versal_fpga_ops_write(struct fpga_manager *mgr,
> +				 const char *buf, size_t size)
> +{
> +	struct versal_fpga_priv *priv;
> +	dma_addr_t dma_addr = 0;
> +	char *kbuf;
> +	int ret;
> +
> +	priv = mgr->priv;
> +
> +	kbuf = dma_alloc_coherent(priv->dev, size, &dma_addr, GFP_KERNEL);
> +	if (!kbuf)
> +		return -ENOMEM;
> +
> +	memcpy(kbuf, buf, size);
> +
> +	wmb(); /* ensure all writes are done before initiate FW call */
> +
> +	ret = zynqmp_pm_load_pdi(PDI_SOURCE_TYPE, dma_addr);
> +
> +	dma_free_coherent(priv->dev, 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_OPERATING;
Is that always the case? Shouldn't that be 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 versal_fpga_priv *priv;
> +	struct fpga_manager *mgr;
> +	int err, ret;
Please pick one, err or ret. 'err' seems unused?
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = dev;
> +	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> +	if (ret < 0) {
> +		dev_err(dev, "no usable DMA configuration");
Nit: "no usable DMA configuration\n"
> +		return ret;
> +	}
> +
> +	mgr = devm_fpga_mgr_create(dev, "Xilinx Versal FPGA Manager",
> +				   &versal_fpga_ops, priv);
> +	if (!mgr)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, mgr);
> +

Replace this part:
> +	err = fpga_mgr_register(mgr);
> +	if (err) {
> +		dev_err(dev, "unable to register FPGA manager");
> +		fpga_mgr_free(mgr);
> +		return err;
> +	}

with:
	return devm_fpga_mgr_register(mgr);

I tried to get rid of the boilerplate, since every driver repeats it
(and above calling fpga_mgr_free(mgr) on a devm_fpga_mgr_create()
created FPGA manager is wrong?) :)
> +
> +	return 0;
> +}
> +

Then
> +static int versal_fpga_remove(struct platform_device *pdev)
> +{
> +	struct fpga_manager *mgr = platform_get_drvdata(pdev);
> +
> +	fpga_mgr_unregister(mgr);
> +	fpga_mgr_free(mgr);
> +
> +	return 0;
> +}
drop this since cleanup is now automatic.
> +
> +static const struct of_device_id versal_fpga_of_match[] = {
> +	{ .compatible = "xlnx,versal-fpga", },
> +	{},
> +};
> +
Nit: Drop the newline
> +MODULE_DEVICE_TABLE(of, versal_fpga_of_match);
> +
> +static struct platform_driver versal_fpga_driver = {
> +	.probe = versal_fpga_probe,
> +	.remove = versal_fpga_remove,
> +	.driver = {
> +		.name = "versal_fpga_manager",
> +		.of_match_table = of_match_ptr(versal_fpga_of_match),
> +	},
> +};
> +
Nit: Drop the newline
> +module_platform_driver(versal_fpga_driver);
> +
> +MODULE_AUTHOR("Nava kishore Manne <nava.manne@xilinx.com>");
> +MODULE_AUTHOR("Appana Durga Kedareswara rao <appanad.durga.rao@xilinx.com>");
> +MODULE_DESCRIPTION("Xilinx Versal FPGA Manager");
> +MODULE_LICENSE("GPL");
> -- 
> 2.18.0
> 
Thanks,
Moritz

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

[Moritz Fischer]

Hi Nava,

On Mon, Jan 18, 2021 at 08:13:18AM +0530, Nava kishore Manne wrote:
> This patch adds driver for versal fpga manager.
Nit: Add support for Xilinx Versal FPGA manager
> 
> PDI source type can be DDR, OCM, QSPI flash etc..
No idea what PDI is :)
> But driver allocates memory always from DDR, Since driver supports only
> DDR source type.
> 
> Signed-off-by: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>
> Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
> ---
>  drivers/fpga/Kconfig       |   8 ++
>  drivers/fpga/Makefile      |   1 +
>  drivers/fpga/versal-fpga.c | 149 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 158 insertions(+)
>  create mode 100644 drivers/fpga/versal-fpga.c
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 5645226ca3ce..9f779c3a6739 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -216,4 +216,12 @@ 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 versal soc firmware
> +          interface to load programmable logic(PL) images
> +          on versal soc.
>  endif # FPGA
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index d8e21dfc6778..40c9adb6a644 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..2a42aa78b182
> --- /dev/null
> +++ b/drivers/fpga/versal-fpga.c
> @@ -0,0 +1,149 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 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>
> +
> +/* Constant Definitions */
> +#define PDI_SOURCE_TYPE	0xF
> +
> +/**
> + * struct versal_fpga_priv - Private data structure
> + * @dev:	Device data structure
> + * @flags:	flags which is used to identify the PL Image type
> + */
> +struct versal_fpga_priv {
> +	struct device *dev;
> +	u32 flags;
This seems unused ... please introduce them when/if you start using
them.
> +};
> +
> +static int versal_fpga_ops_write_init(struct fpga_manager *mgr,
> +				      struct fpga_image_info *info,
> +				      const char *buf, size_t size)
> +{
> +	struct versal_fpga_priv *priv;
> +
> +	priv = mgr->priv;
> +	priv->flags = info->flags;
? What uses this ? It seems this function could just be 'return 0' right
now.
> +
> +	return 0;
> +}
> +
> +static int versal_fpga_ops_write(struct fpga_manager *mgr,
> +				 const char *buf, size_t size)
> +{
> +	struct versal_fpga_priv *priv;
> +	dma_addr_t dma_addr = 0;
> +	char *kbuf;
> +	int ret;
> +
> +	priv = mgr->priv;
> +
> +	kbuf = dma_alloc_coherent(priv->dev, size, &dma_addr, GFP_KERNEL);
> +	if (!kbuf)
> +		return -ENOMEM;
> +
> +	memcpy(kbuf, buf, size);
> +
> +	wmb(); /* ensure all writes are done before initiate FW call */
> +
> +	ret = zynqmp_pm_load_pdi(PDI_SOURCE_TYPE, dma_addr);
> +
> +	dma_free_coherent(priv->dev, 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_OPERATING;
Is that always the case? Shouldn't that be 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 versal_fpga_priv *priv;
> +	struct fpga_manager *mgr;
> +	int err, ret;
Please pick one, err or ret. 'err' seems unused?
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = dev;
> +	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> +	if (ret < 0) {
> +		dev_err(dev, "no usable DMA configuration");
Nit: "no usable DMA configuration\n"
> +		return ret;
> +	}
> +
> +	mgr = devm_fpga_mgr_create(dev, "Xilinx Versal FPGA Manager",
> +				   &versal_fpga_ops, priv);
> +	if (!mgr)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, mgr);
> +

Replace this part:
> +	err = fpga_mgr_register(mgr);
> +	if (err) {
> +		dev_err(dev, "unable to register FPGA manager");
> +		fpga_mgr_free(mgr);
> +		return err;
> +	}

with:
	return devm_fpga_mgr_register(mgr);

I tried to get rid of the boilerplate, since every driver repeats it
(and above calling fpga_mgr_free(mgr) on a devm_fpga_mgr_create()
created FPGA manager is wrong?) :)
> +
> +	return 0;
> +}
> +

Then
> +static int versal_fpga_remove(struct platform_device *pdev)
> +{
> +	struct fpga_manager *mgr = platform_get_drvdata(pdev);
> +
> +	fpga_mgr_unregister(mgr);
> +	fpga_mgr_free(mgr);
> +
> +	return 0;
> +}
drop this since cleanup is now automatic.
> +
> +static const struct of_device_id versal_fpga_of_match[] = {
> +	{ .compatible = "xlnx,versal-fpga", },
> +	{},
> +};
> +
Nit: Drop the newline
> +MODULE_DEVICE_TABLE(of, versal_fpga_of_match);
> +
> +static struct platform_driver versal_fpga_driver = {
> +	.probe = versal_fpga_probe,
> +	.remove = versal_fpga_remove,
> +	.driver = {
> +		.name = "versal_fpga_manager",
> +		.of_match_table = of_match_ptr(versal_fpga_of_match),
> +	},
> +};
> +
Nit: Drop the newline
> +module_platform_driver(versal_fpga_driver);
> +
> +MODULE_AUTHOR("Nava kishore Manne <nava.manne@xilinx.com>");
> +MODULE_AUTHOR("Appana Durga Kedareswara rao <appanad.durga.rao@xilinx.com>");
> +MODULE_DESCRIPTION("Xilinx Versal FPGA Manager");
> +MODULE_LICENSE("GPL");
> -- 
> 2.18.0
> 
Thanks,
Moritz

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 2/3] dt-bindings: fpga: Add binding doc for versal fpga manager

[Nava kishore Manne]

Hi Michal,

	Thanks for the review.
Please find my response inline.

> -----Original Message-----
> From: Michal Simek <michal.simek@xilinx.com>
> Sent: Monday, January 18, 2021 2:22 PM
> To: Nava kishore Manne <navam@xilinx.com>; mdf@kernel.org;
> trix@redhat.com; robh+dt@kernel.org; Michal Simek <michals@xilinx.com>;
> linux-fpga@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Cc: git <git@xilinx.com>; chinnikishore369@gmail.com; Appana Durga
> Kedareswara Rao <appanad@xilinx.com>
> Subject: Re: [PATCH 2/3] dt-bindings: fpga: Add binding doc for versal fpga
> manager
> 
> 
> 
> On 1/18/21 3:43 AM, Nava kishore Manne wrote:
> > From: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>
> >
> > This patch adds binding doc for versal fpga manager driver.
> >
> > Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
> > Signed-off-by: Appana Durga Kedareswara rao
> > <appana.durga.rao@xilinx.com>
> > ---
> >  .../bindings/fpga/xlnx,versal-fpga.yaml       | 33 +++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml
> > b/Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml
> > new file mode 100644
> > index 000000000000..cf3aa7917488
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml
> > @@ -0,0 +1,33 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +
> > +$id: http://devicetree.org/schemas/fpga/xilinx/xlnx,versal-fpga.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Xilinx versal-fpga driver.
> > +
> > +maintainers:
> > +  - Nava kishore Manne <nava.manne@xilinx.com>
> > +
> > +description: |
> > +Device Tree versal-fpga bindings for the Versal SOC, Controlled using
> > +Versal SoC firmware interface.
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +        - xlnx,versal-fpga
> > +
> > +required:
> > +  - compatible
> > +
> > +Required properties:
> > +- compatible: should contain "xlnx,versal-fpga"
> > +
> > +examples:
> > +  - |
> > +    versal_fpga: fpga {
> > +         compatible = "xlnx,versal-fpga";
> > +    };
> >
> 
> There are issues with the binding
> Run
> make
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/fpga/xlnx,versal-
> fpga.yaml
> dt_binding_check
> 

Thanks for pointing it. Will fix in v2.

> and fix
> ./Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml:15:1:
> [error] syntax error: could not find expected ':' (syntax) and maybe others.
> 

Will fix in v2.

Regards,
Navakishore.

RE: [PATCH 2/3] dt-bindings: fpga: Add binding doc for versal fpga manager

[Nava kishore Manne]

Hi Michal,

	Thanks for the review.
Please find my response inline.

> -----Original Message-----
> From: Michal Simek <michal.simek@xilinx.com>
> Sent: Monday, January 18, 2021 2:22 PM
> To: Nava kishore Manne <navam@xilinx.com>; mdf@kernel.org;
> trix@redhat.com; robh+dt@kernel.org; Michal Simek <michals@xilinx.com>;
> linux-fpga@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Cc: git <git@xilinx.com>; chinnikishore369@gmail.com; Appana Durga
> Kedareswara Rao <appanad@xilinx.com>
> Subject: Re: [PATCH 2/3] dt-bindings: fpga: Add binding doc for versal fpga
> manager
> 
> 
> 
> On 1/18/21 3:43 AM, Nava kishore Manne wrote:
> > From: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>
> >
> > This patch adds binding doc for versal fpga manager driver.
> >
> > Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
> > Signed-off-by: Appana Durga Kedareswara rao
> > <appana.durga.rao@xilinx.com>
> > ---
> >  .../bindings/fpga/xlnx,versal-fpga.yaml       | 33 +++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml
> > b/Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml
> > new file mode 100644
> > index 000000000000..cf3aa7917488
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml
> > @@ -0,0 +1,33 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +
> > +$id: http://devicetree.org/schemas/fpga/xilinx/xlnx,versal-fpga.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Xilinx versal-fpga driver.
> > +
> > +maintainers:
> > +  - Nava kishore Manne <nava.manne@xilinx.com>
> > +
> > +description: |
> > +Device Tree versal-fpga bindings for the Versal SOC, Controlled using
> > +Versal SoC firmware interface.
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +        - xlnx,versal-fpga
> > +
> > +required:
> > +  - compatible
> > +
> > +Required properties:
> > +- compatible: should contain "xlnx,versal-fpga"
> > +
> > +examples:
> > +  - |
> > +    versal_fpga: fpga {
> > +         compatible = "xlnx,versal-fpga";
> > +    };
> >
> 
> There are issues with the binding
> Run
> make
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/fpga/xlnx,versal-
> fpga.yaml
> dt_binding_check
> 

Thanks for pointing it. Will fix in v2.

> and fix
> ./Documentation/devicetree/bindings/fpga/xlnx,versal-fpga.yaml:15:1:
> [error] syntax error: could not find expected ':' (syntax) and maybe others.
> 

Will fix in v2.

Regards,
Navakishore.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager driver

[Nava kishore Manne]

Hi Moritz,

	Thanks for the review.
Please find my response inline.

> -----Original Message-----
> From: Moritz Fischer <mdf@kernel.org>
> Sent: Tuesday, January 19, 2021 6:03 AM
> To: Nava kishore Manne <navam@xilinx.com>
> Cc: mdf@kernel.org; trix@redhat.com; robh+dt@kernel.org; Michal Simek
> <michals@xilinx.com>; linux-fpga@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; git <git@xilinx.com>; chinnikishore369@gmail.com;
> Appana Durga Kedareswara Rao <appanad@xilinx.com>
> Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager driver
> 
> Hi Nava,
> 
> On Mon, Jan 18, 2021 at 08:13:18AM +0530, Nava kishore Manne wrote:
> > This patch adds driver for versal fpga manager.
> Nit: Add support for Xilinx Versal FPGA manager

Will fix in v2.

> >
> > PDI source type can be DDR, OCM, QSPI flash etc..
> No idea what PDI is :)

Programmable device image (PDI). 
This file is generated by Xilinx Vivado tool and it contains configuration data objects.

> > But driver allocates memory always from DDR, Since driver supports
> > only DDR source type.
> >
> > Signed-off-by: Appana Durga Kedareswara rao
> > <appana.durga.rao@xilinx.com>
> > Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
> > ---
> >  drivers/fpga/Kconfig       |   8 ++
> >  drivers/fpga/Makefile      |   1 +
> >  drivers/fpga/versal-fpga.c | 149
> > +++++++++++++++++++++++++++++++++++++
> >  3 files changed, 158 insertions(+)
> >  create mode 100644 drivers/fpga/versal-fpga.c
> >
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index
> > 5645226ca3ce..9f779c3a6739 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -216,4 +216,12 @@ 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 versal soc firmware
> > +          interface to load programmable logic(PL) images
> > +          on versal soc.
> >  endif # FPGA
> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index
> > d8e21dfc6778..40c9adb6a644 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..2a42aa78b182
> > --- /dev/null
> > +++ b/drivers/fpga/versal-fpga.c
> > @@ -0,0 +1,149 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (C) 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>
> > +
> > +/* Constant Definitions */
> > +#define PDI_SOURCE_TYPE	0xF
> > +
> > +/**
> > + * struct versal_fpga_priv - Private data structure
> > + * @dev:	Device data structure
> > + * @flags:	flags which is used to identify the PL Image type
> > + */
> > +struct versal_fpga_priv {
> > +	struct device *dev;
> > +	u32 flags;
> This seems unused ... please introduce them when/if you start using them.

Will fix in v2.

> > +};
> > +
> > +static int versal_fpga_ops_write_init(struct fpga_manager *mgr,
> > +				      struct fpga_image_info *info,
> > +				      const char *buf, size_t size) {
> > +	struct versal_fpga_priv *priv;
> > +
> > +	priv = mgr->priv;
> > +	priv->flags = info->flags;
> ? What uses this ? It seems this function could just be 'return 0' right now.

Will fix in v2.

> > +
> > +	return 0;
> > +}
> > +
> > +static int versal_fpga_ops_write(struct fpga_manager *mgr,
> > +				 const char *buf, size_t size)
> > +{
> > +	struct versal_fpga_priv *priv;
> > +	dma_addr_t dma_addr = 0;
> > +	char *kbuf;
> > +	int ret;
> > +
> > +	priv = mgr->priv;
> > +
> > +	kbuf = dma_alloc_coherent(priv->dev, size, &dma_addr,
> GFP_KERNEL);
> > +	if (!kbuf)
> > +		return -ENOMEM;
> > +
> > +	memcpy(kbuf, buf, size);
> > +
> > +	wmb(); /* ensure all writes are done before initiate FW call */
> > +
> > +	ret = zynqmp_pm_load_pdi(PDI_SOURCE_TYPE, dma_addr);
> > +
> > +	dma_free_coherent(priv->dev, 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_OPERATING;
> Is that always the case? Shouldn't that be FPGA_MGR_STATE_UNKNOWN?

For Versal SoC base PDI is always configured prior to Linux boot up. So I make the fpga state as OPERATING.
Please let know if it is not a proper implementation will think about the alternate solution. 

> > +}
> > +
> > +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 versal_fpga_priv *priv;
> > +	struct fpga_manager *mgr;
> > +	int err, ret;
> Please pick one, err or ret. 'err' seems unused?

Will fix in v2.

> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->dev = dev;
> > +	ret = dma_set_mask_and_coherent(&pdev->dev,
> DMA_BIT_MASK(32));
> > +	if (ret < 0) {
> > +		dev_err(dev, "no usable DMA configuration");
> Nit: "no usable DMA configuration\n"

Will fix in v2.

> > +		return ret;
> > +	}
> > +
> > +	mgr = devm_fpga_mgr_create(dev, "Xilinx Versal FPGA Manager",
> > +				   &versal_fpga_ops, priv);
> > +	if (!mgr)
> > +		return -ENOMEM;
> > +
> > +	platform_set_drvdata(pdev, mgr);
> > +
> 
> Replace this part:
> > +	err = fpga_mgr_register(mgr);
> > +	if (err) {
> > +		dev_err(dev, "unable to register FPGA manager");
> > +		fpga_mgr_free(mgr);
> > +		return err;
> > +	}
> 
> with:
> 	return devm_fpga_mgr_register(mgr);
> 
> I tried to get rid of the boilerplate, since every driver repeats it (and above
> calling fpga_mgr_free(mgr) on a devm_fpga_mgr_create() created FPGA
> manager is wrong?) :)

Thanks for pointing it. Will fix in v2.

> > +
> > +	return 0;
> > +}
> > +
> 
> Then
> > +static int versal_fpga_remove(struct platform_device *pdev) {
> > +	struct fpga_manager *mgr = platform_get_drvdata(pdev);
> > +
> > +	fpga_mgr_unregister(mgr);
> > +	fpga_mgr_free(mgr);
> > +
> > +	return 0;
> > +}
> drop this since cleanup is now automatic.

Thanks for pointing it. Will fix in v2.

> > +
> > +static const struct of_device_id versal_fpga_of_match[] = {
> > +	{ .compatible = "xlnx,versal-fpga", },
> > +	{},
> > +};
> > +
> Nit: Drop the newline

Will fix in v2.

> > +MODULE_DEVICE_TABLE(of, versal_fpga_of_match);
> > +
> > +static struct platform_driver versal_fpga_driver = {
> > +	.probe = versal_fpga_probe,
> > +	.remove = versal_fpga_remove,
> > +	.driver = {
> > +		.name = "versal_fpga_manager",
> > +		.of_match_table = of_match_ptr(versal_fpga_of_match),
> > +	},
> > +};
> > +
> Nit: Drop the newline

Will fix in v2.

Regards,
Navakishore.

RE: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager driver

[Nava kishore Manne]

Hi Moritz,

	Thanks for the review.
Please find my response inline.

> -----Original Message-----
> From: Moritz Fischer <mdf@kernel.org>
> Sent: Tuesday, January 19, 2021 6:03 AM
> To: Nava kishore Manne <navam@xilinx.com>
> Cc: mdf@kernel.org; trix@redhat.com; robh+dt@kernel.org; Michal Simek
> <michals@xilinx.com>; linux-fpga@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; git <git@xilinx.com>; chinnikishore369@gmail.com;
> Appana Durga Kedareswara Rao <appanad@xilinx.com>
> Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager driver
> 
> Hi Nava,
> 
> On Mon, Jan 18, 2021 at 08:13:18AM +0530, Nava kishore Manne wrote:
> > This patch adds driver for versal fpga manager.
> Nit: Add support for Xilinx Versal FPGA manager

Will fix in v2.

> >
> > PDI source type can be DDR, OCM, QSPI flash etc..
> No idea what PDI is :)

Programmable device image (PDI). 
This file is generated by Xilinx Vivado tool and it contains configuration data objects.

> > But driver allocates memory always from DDR, Since driver supports
> > only DDR source type.
> >
> > Signed-off-by: Appana Durga Kedareswara rao
> > <appana.durga.rao@xilinx.com>
> > Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
> > ---
> >  drivers/fpga/Kconfig       |   8 ++
> >  drivers/fpga/Makefile      |   1 +
> >  drivers/fpga/versal-fpga.c | 149
> > +++++++++++++++++++++++++++++++++++++
> >  3 files changed, 158 insertions(+)
> >  create mode 100644 drivers/fpga/versal-fpga.c
> >
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index
> > 5645226ca3ce..9f779c3a6739 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -216,4 +216,12 @@ 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 versal soc firmware
> > +          interface to load programmable logic(PL) images
> > +          on versal soc.
> >  endif # FPGA
> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index
> > d8e21dfc6778..40c9adb6a644 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..2a42aa78b182
> > --- /dev/null
> > +++ b/drivers/fpga/versal-fpga.c
> > @@ -0,0 +1,149 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (C) 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>
> > +
> > +/* Constant Definitions */
> > +#define PDI_SOURCE_TYPE	0xF
> > +
> > +/**
> > + * struct versal_fpga_priv - Private data structure
> > + * @dev:	Device data structure
> > + * @flags:	flags which is used to identify the PL Image type
> > + */
> > +struct versal_fpga_priv {
> > +	struct device *dev;
> > +	u32 flags;
> This seems unused ... please introduce them when/if you start using them.

Will fix in v2.

> > +};
> > +
> > +static int versal_fpga_ops_write_init(struct fpga_manager *mgr,
> > +				      struct fpga_image_info *info,
> > +				      const char *buf, size_t size) {
> > +	struct versal_fpga_priv *priv;
> > +
> > +	priv = mgr->priv;
> > +	priv->flags = info->flags;
> ? What uses this ? It seems this function could just be 'return 0' right now.

Will fix in v2.

> > +
> > +	return 0;
> > +}
> > +
> > +static int versal_fpga_ops_write(struct fpga_manager *mgr,
> > +				 const char *buf, size_t size)
> > +{
> > +	struct versal_fpga_priv *priv;
> > +	dma_addr_t dma_addr = 0;
> > +	char *kbuf;
> > +	int ret;
> > +
> > +	priv = mgr->priv;
> > +
> > +	kbuf = dma_alloc_coherent(priv->dev, size, &dma_addr,
> GFP_KERNEL);
> > +	if (!kbuf)
> > +		return -ENOMEM;
> > +
> > +	memcpy(kbuf, buf, size);
> > +
> > +	wmb(); /* ensure all writes are done before initiate FW call */
> > +
> > +	ret = zynqmp_pm_load_pdi(PDI_SOURCE_TYPE, dma_addr);
> > +
> > +	dma_free_coherent(priv->dev, 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_OPERATING;
> Is that always the case? Shouldn't that be FPGA_MGR_STATE_UNKNOWN?

For Versal SoC base PDI is always configured prior to Linux boot up. So I make the fpga state as OPERATING.
Please let know if it is not a proper implementation will think about the alternate solution. 

> > +}
> > +
> > +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 versal_fpga_priv *priv;
> > +	struct fpga_manager *mgr;
> > +	int err, ret;
> Please pick one, err or ret. 'err' seems unused?

Will fix in v2.

> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->dev = dev;
> > +	ret = dma_set_mask_and_coherent(&pdev->dev,
> DMA_BIT_MASK(32));
> > +	if (ret < 0) {
> > +		dev_err(dev, "no usable DMA configuration");
> Nit: "no usable DMA configuration\n"

Will fix in v2.

> > +		return ret;
> > +	}
> > +
> > +	mgr = devm_fpga_mgr_create(dev, "Xilinx Versal FPGA Manager",
> > +				   &versal_fpga_ops, priv);
> > +	if (!mgr)
> > +		return -ENOMEM;
> > +
> > +	platform_set_drvdata(pdev, mgr);
> > +
> 
> Replace this part:
> > +	err = fpga_mgr_register(mgr);
> > +	if (err) {
> > +		dev_err(dev, "unable to register FPGA manager");
> > +		fpga_mgr_free(mgr);
> > +		return err;
> > +	}
> 
> with:
> 	return devm_fpga_mgr_register(mgr);
> 
> I tried to get rid of the boilerplate, since every driver repeats it (and above
> calling fpga_mgr_free(mgr) on a devm_fpga_mgr_create() created FPGA
> manager is wrong?) :)

Thanks for pointing it. Will fix in v2.

> > +
> > +	return 0;
> > +}
> > +
> 
> Then
> > +static int versal_fpga_remove(struct platform_device *pdev) {
> > +	struct fpga_manager *mgr = platform_get_drvdata(pdev);
> > +
> > +	fpga_mgr_unregister(mgr);
> > +	fpga_mgr_free(mgr);
> > +
> > +	return 0;
> > +}
> drop this since cleanup is now automatic.

Thanks for pointing it. Will fix in v2.

> > +
> > +static const struct of_device_id versal_fpga_of_match[] = {
> > +	{ .compatible = "xlnx,versal-fpga", },
> > +	{},
> > +};
> > +
> Nit: Drop the newline

Will fix in v2.

> > +MODULE_DEVICE_TABLE(of, versal_fpga_of_match);
> > +
> > +static struct platform_driver versal_fpga_driver = {
> > +	.probe = versal_fpga_probe,
> > +	.remove = versal_fpga_remove,
> > +	.driver = {
> > +		.name = "versal_fpga_manager",
> > +		.of_match_table = of_match_ptr(versal_fpga_of_match),
> > +	},
> > +};
> > +
> Nit: Drop the newline

Will fix in v2.

Regards,
Navakishore.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Moritz Fischer]

Hi Nava,

On Fri, Jan 22, 2021 at 10:34:15AM +0000, Nava kishore Manne wrote:
> Hi Moritz,
> 
> 	Thanks for the review.
> Please find my response inline.
> 
> > -----Original Message-----
> > From: Moritz Fischer <mdf@kernel.org>
> > Sent: Tuesday, January 19, 2021 6:03 AM
> > To: Nava kishore Manne <navam@xilinx.com>
> > Cc: mdf@kernel.org; trix@redhat.com; robh+dt@kernel.org; Michal Simek
> > <michals@xilinx.com>; linux-fpga@vger.kernel.org;
> > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> > kernel@vger.kernel.org; git <git@xilinx.com>; chinnikishore369@gmail.com;
> > Appana Durga Kedareswara Rao <appanad@xilinx.com>
> > Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager driver
> > 
> > Hi Nava,
> > 
> > On Mon, Jan 18, 2021 at 08:13:18AM +0530, Nava kishore Manne wrote:
> > > This patch adds driver for versal fpga manager.
> > Nit: Add support for Xilinx Versal FPGA manager
> 
> Will fix in v2.
> 
> > >
> > > PDI source type can be DDR, OCM, QSPI flash etc..
> > No idea what PDI is :)
> 
> Programmable device image (PDI). 
> This file is generated by Xilinx Vivado tool and it contains configuration data objects.
> 
> > > But driver allocates memory always from DDR, Since driver supports
> > > only DDR source type.
> > >
> > > Signed-off-by: Appana Durga Kedareswara rao
> > > <appana.durga.rao@xilinx.com>
> > > Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
> > > ---
> > >  drivers/fpga/Kconfig       |   8 ++
> > >  drivers/fpga/Makefile      |   1 +
> > >  drivers/fpga/versal-fpga.c | 149
> > > +++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 158 insertions(+)
> > >  create mode 100644 drivers/fpga/versal-fpga.c
> > >
> > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index
> > > 5645226ca3ce..9f779c3a6739 100644
> > > --- a/drivers/fpga/Kconfig
> > > +++ b/drivers/fpga/Kconfig
> > > @@ -216,4 +216,12 @@ 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 versal soc firmware
> > > +          interface to load programmable logic(PL) images
> > > +          on versal soc.
> > >  endif # FPGA
> > > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index
> > > d8e21dfc6778..40c9adb6a644 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..2a42aa78b182
> > > --- /dev/null
> > > +++ b/drivers/fpga/versal-fpga.c
> > > @@ -0,0 +1,149 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright (C) 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>
> > > +
> > > +/* Constant Definitions */
> > > +#define PDI_SOURCE_TYPE	0xF
> > > +
> > > +/**
> > > + * struct versal_fpga_priv - Private data structure
> > > + * @dev:	Device data structure
> > > + * @flags:	flags which is used to identify the PL Image type
> > > + */
> > > +struct versal_fpga_priv {
> > > +	struct device *dev;
> > > +	u32 flags;
> > This seems unused ... please introduce them when/if you start using them.
> 
> Will fix in v2.
> 
> > > +};
> > > +
> > > +static int versal_fpga_ops_write_init(struct fpga_manager *mgr,
> > > +				      struct fpga_image_info *info,
> > > +				      const char *buf, size_t size) {
> > > +	struct versal_fpga_priv *priv;
> > > +
> > > +	priv = mgr->priv;
> > > +	priv->flags = info->flags;
> > ? What uses this ? It seems this function could just be 'return 0' right now.
> 
> Will fix in v2.
> 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int versal_fpga_ops_write(struct fpga_manager *mgr,
> > > +				 const char *buf, size_t size)
> > > +{
> > > +	struct versal_fpga_priv *priv;
> > > +	dma_addr_t dma_addr = 0;
> > > +	char *kbuf;
> > > +	int ret;
> > > +
> > > +	priv = mgr->priv;
> > > +
> > > +	kbuf = dma_alloc_coherent(priv->dev, size, &dma_addr,
> > GFP_KERNEL);
> > > +	if (!kbuf)
> > > +		return -ENOMEM;
> > > +
> > > +	memcpy(kbuf, buf, size);
> > > +
> > > +	wmb(); /* ensure all writes are done before initiate FW call */
> > > +
> > > +	ret = zynqmp_pm_load_pdi(PDI_SOURCE_TYPE, dma_addr);
> > > +
> > > +	dma_free_coherent(priv->dev, 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_OPERATING;
> > Is that always the case? Shouldn't that be FPGA_MGR_STATE_UNKNOWN?
> 
> For Versal SoC base PDI is always configured prior to Linux boot up. So I make the fpga state as OPERATING.
> Please let know if it is not a proper implementation will think about the alternate solution. 

So you're saying I can't boot a Versal SoC without a PDI / Bitstream
loaded? Interesting :)
> 
> > > +}
> > > +
> > > +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 versal_fpga_priv *priv;
> > > +	struct fpga_manager *mgr;
> > > +	int err, ret;
> > Please pick one, err or ret. 'err' seems unused?
> 
> Will fix in v2.
> 
> > > +
> > > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > +	if (!priv)
> > > +		return -ENOMEM;
> > > +
> > > +	priv->dev = dev;
> > > +	ret = dma_set_mask_and_coherent(&pdev->dev,
> > DMA_BIT_MASK(32));
> > > +	if (ret < 0) {
> > > +		dev_err(dev, "no usable DMA configuration");
> > Nit: "no usable DMA configuration\n"
> 
> Will fix in v2.
> 
> > > +		return ret;
> > > +	}
> > > +
> > > +	mgr = devm_fpga_mgr_create(dev, "Xilinx Versal FPGA Manager",
> > > +				   &versal_fpga_ops, priv);
> > > +	if (!mgr)
> > > +		return -ENOMEM;
> > > +
> > > +	platform_set_drvdata(pdev, mgr);
> > > +
> > 
> > Replace this part:
> > > +	err = fpga_mgr_register(mgr);
> > > +	if (err) {
> > > +		dev_err(dev, "unable to register FPGA manager");
> > > +		fpga_mgr_free(mgr);
> > > +		return err;
> > > +	}
> > 
> > with:
> > 	return devm_fpga_mgr_register(mgr);
> > 
> > I tried to get rid of the boilerplate, since every driver repeats it (and above
> > calling fpga_mgr_free(mgr) on a devm_fpga_mgr_create() created FPGA
> > manager is wrong?) :)
> 
> Thanks for pointing it. Will fix in v2.
> 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > 
> > Then
> > > +static int versal_fpga_remove(struct platform_device *pdev) {
> > > +	struct fpga_manager *mgr = platform_get_drvdata(pdev);
> > > +
> > > +	fpga_mgr_unregister(mgr);
> > > +	fpga_mgr_free(mgr);
> > > +
> > > +	return 0;
> > > +}
> > drop this since cleanup is now automatic.
> 
> Thanks for pointing it. Will fix in v2.
> 
> > > +
> > > +static const struct of_device_id versal_fpga_of_match[] = {
> > > +	{ .compatible = "xlnx,versal-fpga", },
> > > +	{},
> > > +};
> > > +
> > Nit: Drop the newline
> 
> Will fix in v2.
> 
> > > +MODULE_DEVICE_TABLE(of, versal_fpga_of_match);
> > > +
> > > +static struct platform_driver versal_fpga_driver = {
> > > +	.probe = versal_fpga_probe,
> > > +	.remove = versal_fpga_remove,
> > > +	.driver = {
> > > +		.name = "versal_fpga_manager",
> > > +		.of_match_table = of_match_ptr(versal_fpga_of_match),
> > > +	},
> > > +};
> > > +
> > Nit: Drop the newline
> 
> Will fix in v2.
> 
> Regards,
> Navakishore.
- Moritz

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

[Moritz Fischer]

Hi Nava,

On Fri, Jan 22, 2021 at 10:34:15AM +0000, Nava kishore Manne wrote:
> Hi Moritz,
> 
> 	Thanks for the review.
> Please find my response inline.
> 
> > -----Original Message-----
> > From: Moritz Fischer <mdf@kernel.org>
> > Sent: Tuesday, January 19, 2021 6:03 AM
> > To: Nava kishore Manne <navam@xilinx.com>
> > Cc: mdf@kernel.org; trix@redhat.com; robh+dt@kernel.org; Michal Simek
> > <michals@xilinx.com>; linux-fpga@vger.kernel.org;
> > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> > kernel@vger.kernel.org; git <git@xilinx.com>; chinnikishore369@gmail.com;
> > Appana Durga Kedareswara Rao <appanad@xilinx.com>
> > Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager driver
> > 
> > Hi Nava,
> > 
> > On Mon, Jan 18, 2021 at 08:13:18AM +0530, Nava kishore Manne wrote:
> > > This patch adds driver for versal fpga manager.
> > Nit: Add support for Xilinx Versal FPGA manager
> 
> Will fix in v2.
> 
> > >
> > > PDI source type can be DDR, OCM, QSPI flash etc..
> > No idea what PDI is :)
> 
> Programmable device image (PDI). 
> This file is generated by Xilinx Vivado tool and it contains configuration data objects.
> 
> > > But driver allocates memory always from DDR, Since driver supports
> > > only DDR source type.
> > >
> > > Signed-off-by: Appana Durga Kedareswara rao
> > > <appana.durga.rao@xilinx.com>
> > > Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
> > > ---
> > >  drivers/fpga/Kconfig       |   8 ++
> > >  drivers/fpga/Makefile      |   1 +
> > >  drivers/fpga/versal-fpga.c | 149
> > > +++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 158 insertions(+)
> > >  create mode 100644 drivers/fpga/versal-fpga.c
> > >
> > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index
> > > 5645226ca3ce..9f779c3a6739 100644
> > > --- a/drivers/fpga/Kconfig
> > > +++ b/drivers/fpga/Kconfig
> > > @@ -216,4 +216,12 @@ 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 versal soc firmware
> > > +          interface to load programmable logic(PL) images
> > > +          on versal soc.
> > >  endif # FPGA
> > > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index
> > > d8e21dfc6778..40c9adb6a644 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..2a42aa78b182
> > > --- /dev/null
> > > +++ b/drivers/fpga/versal-fpga.c
> > > @@ -0,0 +1,149 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright (C) 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>
> > > +
> > > +/* Constant Definitions */
> > > +#define PDI_SOURCE_TYPE	0xF
> > > +
> > > +/**
> > > + * struct versal_fpga_priv - Private data structure
> > > + * @dev:	Device data structure
> > > + * @flags:	flags which is used to identify the PL Image type
> > > + */
> > > +struct versal_fpga_priv {
> > > +	struct device *dev;
> > > +	u32 flags;
> > This seems unused ... please introduce them when/if you start using them.
> 
> Will fix in v2.
> 
> > > +};
> > > +
> > > +static int versal_fpga_ops_write_init(struct fpga_manager *mgr,
> > > +				      struct fpga_image_info *info,
> > > +				      const char *buf, size_t size) {
> > > +	struct versal_fpga_priv *priv;
> > > +
> > > +	priv = mgr->priv;
> > > +	priv->flags = info->flags;
> > ? What uses this ? It seems this function could just be 'return 0' right now.
> 
> Will fix in v2.
> 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int versal_fpga_ops_write(struct fpga_manager *mgr,
> > > +				 const char *buf, size_t size)
> > > +{
> > > +	struct versal_fpga_priv *priv;
> > > +	dma_addr_t dma_addr = 0;
> > > +	char *kbuf;
> > > +	int ret;
> > > +
> > > +	priv = mgr->priv;
> > > +
> > > +	kbuf = dma_alloc_coherent(priv->dev, size, &dma_addr,
> > GFP_KERNEL);
> > > +	if (!kbuf)
> > > +		return -ENOMEM;
> > > +
> > > +	memcpy(kbuf, buf, size);
> > > +
> > > +	wmb(); /* ensure all writes are done before initiate FW call */
> > > +
> > > +	ret = zynqmp_pm_load_pdi(PDI_SOURCE_TYPE, dma_addr);
> > > +
> > > +	dma_free_coherent(priv->dev, 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_OPERATING;
> > Is that always the case? Shouldn't that be FPGA_MGR_STATE_UNKNOWN?
> 
> For Versal SoC base PDI is always configured prior to Linux boot up. So I make the fpga state as OPERATING.
> Please let know if it is not a proper implementation will think about the alternate solution. 

So you're saying I can't boot a Versal SoC without a PDI / Bitstream
loaded? Interesting :)
> 
> > > +}
> > > +
> > > +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 versal_fpga_priv *priv;
> > > +	struct fpga_manager *mgr;
> > > +	int err, ret;
> > Please pick one, err or ret. 'err' seems unused?
> 
> Will fix in v2.
> 
> > > +
> > > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > +	if (!priv)
> > > +		return -ENOMEM;
> > > +
> > > +	priv->dev = dev;
> > > +	ret = dma_set_mask_and_coherent(&pdev->dev,
> > DMA_BIT_MASK(32));
> > > +	if (ret < 0) {
> > > +		dev_err(dev, "no usable DMA configuration");
> > Nit: "no usable DMA configuration\n"
> 
> Will fix in v2.
> 
> > > +		return ret;
> > > +	}
> > > +
> > > +	mgr = devm_fpga_mgr_create(dev, "Xilinx Versal FPGA Manager",
> > > +				   &versal_fpga_ops, priv);
> > > +	if (!mgr)
> > > +		return -ENOMEM;
> > > +
> > > +	platform_set_drvdata(pdev, mgr);
> > > +
> > 
> > Replace this part:
> > > +	err = fpga_mgr_register(mgr);
> > > +	if (err) {
> > > +		dev_err(dev, "unable to register FPGA manager");
> > > +		fpga_mgr_free(mgr);
> > > +		return err;
> > > +	}
> > 
> > with:
> > 	return devm_fpga_mgr_register(mgr);
> > 
> > I tried to get rid of the boilerplate, since every driver repeats it (and above
> > calling fpga_mgr_free(mgr) on a devm_fpga_mgr_create() created FPGA
> > manager is wrong?) :)
> 
> Thanks for pointing it. Will fix in v2.
> 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > 
> > Then
> > > +static int versal_fpga_remove(struct platform_device *pdev) {
> > > +	struct fpga_manager *mgr = platform_get_drvdata(pdev);
> > > +
> > > +	fpga_mgr_unregister(mgr);
> > > +	fpga_mgr_free(mgr);
> > > +
> > > +	return 0;
> > > +}
> > drop this since cleanup is now automatic.
> 
> Thanks for pointing it. Will fix in v2.
> 
> > > +
> > > +static const struct of_device_id versal_fpga_of_match[] = {
> > > +	{ .compatible = "xlnx,versal-fpga", },
> > > +	{},
> > > +};
> > > +
> > Nit: Drop the newline
> 
> Will fix in v2.
> 
> > > +MODULE_DEVICE_TABLE(of, versal_fpga_of_match);
> > > +
> > > +static struct platform_driver versal_fpga_driver = {
> > > +	.probe = versal_fpga_probe,
> > > +	.remove = versal_fpga_remove,
> > > +	.driver = {
> > > +		.name = "versal_fpga_manager",
> > > +		.of_match_table = of_match_ptr(versal_fpga_of_match),
> > > +	},
> > > +};
> > > +
> > Nit: Drop the newline
> 
> Will fix in v2.
> 
> Regards,
> Navakishore.
- Moritz

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] drivers: firmware: Add Pdi load API support

[Moritz Fischer]

On Mon, Jan 18, 2021 at 08:13:16AM +0530, Nava kishore Manne wrote:
> This patch adds load pdi api support to enable pdi/partial loading from
> linux. Programmable Device Image (PDI) is combination of headers, images
> and bitstream files to be loaded. Partial PDI is partial set of image/
> images to be loaded.
> 
> Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
> ---
>  drivers/firmware/xilinx/zynqmp.c     | 17 +++++++++++++++++
>  include/linux/firmware/xlnx-zynqmp.h |  9 +++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
> index 7eb9958662dd..a466225b9f9e 100644
> --- a/drivers/firmware/xilinx/zynqmp.c
> +++ b/drivers/firmware/xilinx/zynqmp.c
> @@ -897,6 +897,23 @@ int zynqmp_pm_set_requirement(const u32 node, const u32 capabilities,
>  }
>  EXPORT_SYMBOL_GPL(zynqmp_pm_set_requirement);
>  
> +/**
> + * zynqmp_pm_load_pdi - Load and process pdi
Nit: Is it pdi or PDI. Pick one :)
> + * @src:       Source device where PDI is located
> + * @address:   Pdi src address
> + *
> + * This function provides support to load pdi from linux
> + *
> + * Return: Returns status, either success or error+reason
> + */
> +int zynqmp_pm_load_pdi(const u32 src, const u64 address)
> +{
> +	return zynqmp_pm_invoke_fn(PM_LOAD_PDI, src,
> +				   lower_32_bits(address),
> +				   upper_32_bits(address), 0, NULL);
> +}
> +EXPORT_SYMBOL_GPL(zynqmp_pm_load_pdi);
> +
>  /**
>   * zynqmp_pm_aes - Access AES hardware to encrypt/decrypt the data using
>   * AES-GCM core.
> diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h
> index 2a0da841c942..87114ee645b1 100644
> --- a/include/linux/firmware/xlnx-zynqmp.h
> +++ b/include/linux/firmware/xlnx-zynqmp.h
> @@ -52,6 +52,9 @@
>  #define	ZYNQMP_PM_CAPABILITY_WAKEUP	0x4U
>  #define	ZYNQMP_PM_CAPABILITY_UNUSABLE	0x8U
>  
> +/* Loader commands */
> +#define PM_LOAD_PDI	0x701
> +
>  /*
>   * Firmware FPGA Manager flags
>   * XILINX_ZYNQMP_PM_FPGA_FULL:	FPGA full reconfiguration
> @@ -354,6 +357,7 @@ int zynqmp_pm_write_pggs(u32 index, u32 value);
>  int zynqmp_pm_read_pggs(u32 index, u32 *value);
>  int zynqmp_pm_system_shutdown(const u32 type, const u32 subtype);
>  int zynqmp_pm_set_boot_health_status(u32 value);
> +int zynqmp_pm_load_pdi(const u32 src, const u64 address);
>  #else
>  static inline struct zynqmp_eemi_ops *zynqmp_pm_get_eemi_ops(void)
>  {
> @@ -538,6 +542,11 @@ static inline int zynqmp_pm_set_boot_health_status(u32 value)
>  {
>  	return -ENODEV;
>  }
> +
> +static inline int zynqmp_pm_load_pdi(const u32 src, const u64 address)
> +{
> +	return -ENODEV;
> +}
>  #endif
>  
>  #endif /* __FIRMWARE_ZYNQMP_H__ */
> -- 
> 2.18.0
> 

Re: [PATCH 1/3] drivers: firmware: Add Pdi load API support

[Moritz Fischer]

On Mon, Jan 18, 2021 at 08:13:16AM +0530, Nava kishore Manne wrote:
> This patch adds load pdi api support to enable pdi/partial loading from
> linux. Programmable Device Image (PDI) is combination of headers, images
> and bitstream files to be loaded. Partial PDI is partial set of image/
> images to be loaded.
> 
> Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
> ---
>  drivers/firmware/xilinx/zynqmp.c     | 17 +++++++++++++++++
>  include/linux/firmware/xlnx-zynqmp.h |  9 +++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
> index 7eb9958662dd..a466225b9f9e 100644
> --- a/drivers/firmware/xilinx/zynqmp.c
> +++ b/drivers/firmware/xilinx/zynqmp.c
> @@ -897,6 +897,23 @@ int zynqmp_pm_set_requirement(const u32 node, const u32 capabilities,
>  }
>  EXPORT_SYMBOL_GPL(zynqmp_pm_set_requirement);
>  
> +/**
> + * zynqmp_pm_load_pdi - Load and process pdi
Nit: Is it pdi or PDI. Pick one :)
> + * @src:       Source device where PDI is located
> + * @address:   Pdi src address
> + *
> + * This function provides support to load pdi from linux
> + *
> + * Return: Returns status, either success or error+reason
> + */
> +int zynqmp_pm_load_pdi(const u32 src, const u64 address)
> +{
> +	return zynqmp_pm_invoke_fn(PM_LOAD_PDI, src,
> +				   lower_32_bits(address),
> +				   upper_32_bits(address), 0, NULL);
> +}
> +EXPORT_SYMBOL_GPL(zynqmp_pm_load_pdi);
> +
>  /**
>   * zynqmp_pm_aes - Access AES hardware to encrypt/decrypt the data using
>   * AES-GCM core.
> diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h
> index 2a0da841c942..87114ee645b1 100644
> --- a/include/linux/firmware/xlnx-zynqmp.h
> +++ b/include/linux/firmware/xlnx-zynqmp.h
> @@ -52,6 +52,9 @@
>  #define	ZYNQMP_PM_CAPABILITY_WAKEUP	0x4U
>  #define	ZYNQMP_PM_CAPABILITY_UNUSABLE	0x8U
>  
> +/* Loader commands */
> +#define PM_LOAD_PDI	0x701
> +
>  /*
>   * Firmware FPGA Manager flags
>   * XILINX_ZYNQMP_PM_FPGA_FULL:	FPGA full reconfiguration
> @@ -354,6 +357,7 @@ int zynqmp_pm_write_pggs(u32 index, u32 value);
>  int zynqmp_pm_read_pggs(u32 index, u32 *value);
>  int zynqmp_pm_system_shutdown(const u32 type, const u32 subtype);
>  int zynqmp_pm_set_boot_health_status(u32 value);
> +int zynqmp_pm_load_pdi(const u32 src, const u64 address);
>  #else
>  static inline struct zynqmp_eemi_ops *zynqmp_pm_get_eemi_ops(void)
>  {
> @@ -538,6 +542,11 @@ static inline int zynqmp_pm_set_boot_health_status(u32 value)
>  {
>  	return -ENODEV;
>  }
> +
> +static inline int zynqmp_pm_load_pdi(const u32 src, const u64 address)
> +{
> +	return -ENODEV;
> +}
>  #endif
>  
>  #endif /* __FIRMWARE_ZYNQMP_H__ */
> -- 
> 2.18.0
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager driver

[Nava kishore Manne]

Hi Moritz,

	Please find my response inline.

> -----Original Message-----
> From: Moritz Fischer <mdf@kernel.org>
> Sent: Sunday, January 24, 2021 5:04 AM
> To: Nava kishore Manne <navam@xilinx.com>
> Cc: Moritz Fischer <mdf@kernel.org>; trix@redhat.com;
> robh+dt@kernel.org; Michal Simek <michals@xilinx.com>; linux-
> fpga@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git
> <git@xilinx.com>; chinnikishore369@gmail.com; Appana Durga Kedareswara
> Rao <appanad@xilinx.com>
> Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager driver
> 
> Hi Nava,
> 
> On Fri, Jan 22, 2021 at 10:34:15AM +0000, Nava kishore Manne wrote:
> > Hi Moritz,
> >
> > 	Thanks for the review.
> > Please find my response inline.
> >
> > > -----Original Message-----
> > > From: Moritz Fischer <mdf@kernel.org>
> > > Sent: Tuesday, January 19, 2021 6:03 AM
> > > To: Nava kishore Manne <navam@xilinx.com>
> > > Cc: mdf@kernel.org; trix@redhat.com; robh+dt@kernel.org; Michal
> > > Simek <michals@xilinx.com>; linux-fpga@vger.kernel.org;
> > > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > > linux- kernel@vger.kernel.org; git <git@xilinx.com>;
> > > chinnikishore369@gmail.com; Appana Durga Kedareswara Rao
> > > <appanad@xilinx.com>
> > > Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager
> > > driver
> > >
> > > Hi Nava,
> > >
> > > On Mon, Jan 18, 2021 at 08:13:18AM +0530, Nava kishore Manne wrote:
> > > > This patch adds driver for versal fpga manager.
> > > Nit: Add support for Xilinx Versal FPGA manager
> >
> > Will fix in v2.
> >
> > > >
> > > > PDI source type can be DDR, OCM, QSPI flash etc..
> > > No idea what PDI is :)
> >
> > Programmable device image (PDI).
> > This file is generated by Xilinx Vivado tool and it contains configuration data
> objects.
> >
> > > > But driver allocates memory always from DDR, Since driver supports
> > > > only DDR source type.
> > > >
> > > > Signed-off-by: Appana Durga Kedareswara rao
> > > > <appana.durga.rao@xilinx.com>
> > > > Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
> > > > ---
> > > >  drivers/fpga/Kconfig       |   8 ++
> > > >  drivers/fpga/Makefile      |   1 +
> > > >  drivers/fpga/versal-fpga.c | 149
> > > > +++++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 158 insertions(+)  create mode 100644
> > > > drivers/fpga/versal-fpga.c
> > > >
> > > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index
> > > > 5645226ca3ce..9f779c3a6739 100644
> > > > --- a/drivers/fpga/Kconfig
> > > > +++ b/drivers/fpga/Kconfig
> > > > @@ -216,4 +216,12 @@ 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 versal soc firmware
> > > > +          interface to load programmable logic(PL) images
> > > > +          on versal soc.
> > > >  endif # FPGA
> > > > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index
> > > > d8e21dfc6778..40c9adb6a644 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..2a42aa78b182
> > > > --- /dev/null
> > > > +++ b/drivers/fpga/versal-fpga.c
> > > > @@ -0,0 +1,149 @@
> > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > +/*
> > > > + * Copyright (C) 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>
> > > > +
> > > > +/* Constant Definitions */
> > > > +#define PDI_SOURCE_TYPE	0xF
> > > > +
> > > > +/**
> > > > + * struct versal_fpga_priv - Private data structure
> > > > + * @dev:	Device data structure
> > > > + * @flags:	flags which is used to identify the PL Image type
> > > > + */
> > > > +struct versal_fpga_priv {
> > > > +	struct device *dev;
> > > > +	u32 flags;
> > > This seems unused ... please introduce them when/if you start using
> them.
> >
> > Will fix in v2.
> >
> > > > +};
> > > > +
> > > > +static int versal_fpga_ops_write_init(struct fpga_manager *mgr,
> > > > +				      struct fpga_image_info *info,
> > > > +				      const char *buf, size_t size) {
> > > > +	struct versal_fpga_priv *priv;
> > > > +
> > > > +	priv = mgr->priv;
> > > > +	priv->flags = info->flags;
> > > ? What uses this ? It seems this function could just be 'return 0' right now.
> >
> > Will fix in v2.
> >
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int versal_fpga_ops_write(struct fpga_manager *mgr,
> > > > +				 const char *buf, size_t size) {
> > > > +	struct versal_fpga_priv *priv;
> > > > +	dma_addr_t dma_addr = 0;
> > > > +	char *kbuf;
> > > > +	int ret;
> > > > +
> > > > +	priv = mgr->priv;
> > > > +
> > > > +	kbuf = dma_alloc_coherent(priv->dev, size, &dma_addr,
> > > GFP_KERNEL);
> > > > +	if (!kbuf)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	memcpy(kbuf, buf, size);
> > > > +
> > > > +	wmb(); /* ensure all writes are done before initiate FW call */
> > > > +
> > > > +	ret = zynqmp_pm_load_pdi(PDI_SOURCE_TYPE, dma_addr);
> > > > +
> > > > +	dma_free_coherent(priv->dev, 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_OPERATING;
> > > Is that always the case? Shouldn't that be
> FPGA_MGR_STATE_UNKNOWN?
> >
> > For Versal SoC base PDI is always configured prior to Linux boot up. So I
> make the fpga state as OPERATING.
> > Please let know if it is not a proper implementation will think about the
> alternate solution.
> 
> So you're saying I can't boot a Versal SoC without a PDI / Bitstream loaded?
> Interesting :)
> >

For Versal SoC Vivado generated base PDI is always needed to bring-up the board. 

Regards,
Navakishore.

RE: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager driver

[Nava kishore Manne]

Hi Moritz,

	Please find my response inline.

> -----Original Message-----
> From: Moritz Fischer <mdf@kernel.org>
> Sent: Sunday, January 24, 2021 5:04 AM
> To: Nava kishore Manne <navam@xilinx.com>
> Cc: Moritz Fischer <mdf@kernel.org>; trix@redhat.com;
> robh+dt@kernel.org; Michal Simek <michals@xilinx.com>; linux-
> fpga@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git
> <git@xilinx.com>; chinnikishore369@gmail.com; Appana Durga Kedareswara
> Rao <appanad@xilinx.com>
> Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager driver
> 
> Hi Nava,
> 
> On Fri, Jan 22, 2021 at 10:34:15AM +0000, Nava kishore Manne wrote:
> > Hi Moritz,
> >
> > 	Thanks for the review.
> > Please find my response inline.
> >
> > > -----Original Message-----
> > > From: Moritz Fischer <mdf@kernel.org>
> > > Sent: Tuesday, January 19, 2021 6:03 AM
> > > To: Nava kishore Manne <navam@xilinx.com>
> > > Cc: mdf@kernel.org; trix@redhat.com; robh+dt@kernel.org; Michal
> > > Simek <michals@xilinx.com>; linux-fpga@vger.kernel.org;
> > > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > > linux- kernel@vger.kernel.org; git <git@xilinx.com>;
> > > chinnikishore369@gmail.com; Appana Durga Kedareswara Rao
> > > <appanad@xilinx.com>
> > > Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager
> > > driver
> > >
> > > Hi Nava,
> > >
> > > On Mon, Jan 18, 2021 at 08:13:18AM +0530, Nava kishore Manne wrote:
> > > > This patch adds driver for versal fpga manager.
> > > Nit: Add support for Xilinx Versal FPGA manager
> >
> > Will fix in v2.
> >
> > > >
> > > > PDI source type can be DDR, OCM, QSPI flash etc..
> > > No idea what PDI is :)
> >
> > Programmable device image (PDI).
> > This file is generated by Xilinx Vivado tool and it contains configuration data
> objects.
> >
> > > > But driver allocates memory always from DDR, Since driver supports
> > > > only DDR source type.
> > > >
> > > > Signed-off-by: Appana Durga Kedareswara rao
> > > > <appana.durga.rao@xilinx.com>
> > > > Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
> > > > ---
> > > >  drivers/fpga/Kconfig       |   8 ++
> > > >  drivers/fpga/Makefile      |   1 +
> > > >  drivers/fpga/versal-fpga.c | 149
> > > > +++++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 158 insertions(+)  create mode 100644
> > > > drivers/fpga/versal-fpga.c
> > > >
> > > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index
> > > > 5645226ca3ce..9f779c3a6739 100644
> > > > --- a/drivers/fpga/Kconfig
> > > > +++ b/drivers/fpga/Kconfig
> > > > @@ -216,4 +216,12 @@ 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 versal soc firmware
> > > > +          interface to load programmable logic(PL) images
> > > > +          on versal soc.
> > > >  endif # FPGA
> > > > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index
> > > > d8e21dfc6778..40c9adb6a644 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..2a42aa78b182
> > > > --- /dev/null
> > > > +++ b/drivers/fpga/versal-fpga.c
> > > > @@ -0,0 +1,149 @@
> > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > +/*
> > > > + * Copyright (C) 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>
> > > > +
> > > > +/* Constant Definitions */
> > > > +#define PDI_SOURCE_TYPE	0xF
> > > > +
> > > > +/**
> > > > + * struct versal_fpga_priv - Private data structure
> > > > + * @dev:	Device data structure
> > > > + * @flags:	flags which is used to identify the PL Image type
> > > > + */
> > > > +struct versal_fpga_priv {
> > > > +	struct device *dev;
> > > > +	u32 flags;
> > > This seems unused ... please introduce them when/if you start using
> them.
> >
> > Will fix in v2.
> >
> > > > +};
> > > > +
> > > > +static int versal_fpga_ops_write_init(struct fpga_manager *mgr,
> > > > +				      struct fpga_image_info *info,
> > > > +				      const char *buf, size_t size) {
> > > > +	struct versal_fpga_priv *priv;
> > > > +
> > > > +	priv = mgr->priv;
> > > > +	priv->flags = info->flags;
> > > ? What uses this ? It seems this function could just be 'return 0' right now.
> >
> > Will fix in v2.
> >
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int versal_fpga_ops_write(struct fpga_manager *mgr,
> > > > +				 const char *buf, size_t size) {
> > > > +	struct versal_fpga_priv *priv;
> > > > +	dma_addr_t dma_addr = 0;
> > > > +	char *kbuf;
> > > > +	int ret;
> > > > +
> > > > +	priv = mgr->priv;
> > > > +
> > > > +	kbuf = dma_alloc_coherent(priv->dev, size, &dma_addr,
> > > GFP_KERNEL);
> > > > +	if (!kbuf)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	memcpy(kbuf, buf, size);
> > > > +
> > > > +	wmb(); /* ensure all writes are done before initiate FW call */
> > > > +
> > > > +	ret = zynqmp_pm_load_pdi(PDI_SOURCE_TYPE, dma_addr);
> > > > +
> > > > +	dma_free_coherent(priv->dev, 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_OPERATING;
> > > Is that always the case? Shouldn't that be
> FPGA_MGR_STATE_UNKNOWN?
> >
> > For Versal SoC base PDI is always configured prior to Linux boot up. So I
> make the fpga state as OPERATING.
> > Please let know if it is not a proper implementation will think about the
> alternate solution.
> 
> So you're saying I can't boot a Versal SoC without a PDI / Bitstream loaded?
> Interesting :)
> >

For Versal SoC Vivado generated base PDI is always needed to bring-up the board. 

Regards,
Navakishore.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Michal Simek]

Hi

On 1/27/21 9:57 AM, Nava kishore Manne wrote:
> Hi Moritz,
> 
> 	Please find my response inline.
> 
>> -----Original Message-----
>> From: Moritz Fischer <mdf@kernel.org>
>> Sent: Sunday, January 24, 2021 5:04 AM
>> To: Nava kishore Manne <navam@xilinx.com>
>> Cc: Moritz Fischer <mdf@kernel.org>; trix@redhat.com;
>> robh+dt@kernel.org; Michal Simek <michals@xilinx.com>; linux-
>> fpga@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
>> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git
>> <git@xilinx.com>; chinnikishore369@gmail.com; Appana Durga Kedareswara
>> Rao <appanad@xilinx.com>
>> Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager driver
>>
>> Hi Nava,
>>
>> On Fri, Jan 22, 2021 at 10:34:15AM +0000, Nava kishore Manne wrote:
>>> Hi Moritz,
>>>
>>> 	Thanks for the review.
>>> Please find my response inline.
>>>
>>>> -----Original Message-----
>>>> From: Moritz Fischer <mdf@kernel.org>
>>>> Sent: Tuesday, January 19, 2021 6:03 AM
>>>> To: Nava kishore Manne <navam@xilinx.com>
>>>> Cc: mdf@kernel.org; trix@redhat.com; robh+dt@kernel.org; Michal
>>>> Simek <michals@xilinx.com>; linux-fpga@vger.kernel.org;
>>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>>>> linux- kernel@vger.kernel.org; git <git@xilinx.com>;
>>>> chinnikishore369@gmail.com; Appana Durga Kedareswara Rao
>>>> <appanad@xilinx.com>
>>>> Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager
>>>> driver
>>>>
>>>> Hi Nava,
>>>>
>>>> On Mon, Jan 18, 2021 at 08:13:18AM +0530, Nava kishore Manne wrote:
>>>>> This patch adds driver for versal fpga manager.
>>>> Nit: Add support for Xilinx Versal FPGA manager
>>>
>>> Will fix in v2.
>>>
>>>>>
>>>>> PDI source type can be DDR, OCM, QSPI flash etc..
>>>> No idea what PDI is :)
>>>
>>> Programmable device image (PDI).
>>> This file is generated by Xilinx Vivado tool and it contains configuration data
>> objects.
>>>
>>>>> But driver allocates memory always from DDR, Since driver supports
>>>>> only DDR source type.
>>>>>
>>>>> Signed-off-by: Appana Durga Kedareswara rao
>>>>> <appana.durga.rao@xilinx.com>
>>>>> Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
>>>>> ---
>>>>>  drivers/fpga/Kconfig       |   8 ++
>>>>>  drivers/fpga/Makefile      |   1 +
>>>>>  drivers/fpga/versal-fpga.c | 149
>>>>> +++++++++++++++++++++++++++++++++++++
>>>>>  3 files changed, 158 insertions(+)  create mode 100644
>>>>> drivers/fpga/versal-fpga.c
>>>>>
>>>>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index
>>>>> 5645226ca3ce..9f779c3a6739 100644
>>>>> --- a/drivers/fpga/Kconfig
>>>>> +++ b/drivers/fpga/Kconfig
>>>>> @@ -216,4 +216,12 @@ 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 versal soc firmware
>>>>> +          interface to load programmable logic(PL) images
>>>>> +          on versal soc.
>>>>>  endif # FPGA
>>>>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index
>>>>> d8e21dfc6778..40c9adb6a644 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..2a42aa78b182
>>>>> --- /dev/null
>>>>> +++ b/drivers/fpga/versal-fpga.c
>>>>> @@ -0,0 +1,149 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>>> +/*
>>>>> + * Copyright (C) 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>
>>>>> +
>>>>> +/* Constant Definitions */
>>>>> +#define PDI_SOURCE_TYPE	0xF
>>>>> +
>>>>> +/**
>>>>> + * struct versal_fpga_priv - Private data structure
>>>>> + * @dev:	Device data structure
>>>>> + * @flags:	flags which is used to identify the PL Image type
>>>>> + */
>>>>> +struct versal_fpga_priv {
>>>>> +	struct device *dev;
>>>>> +	u32 flags;
>>>> This seems unused ... please introduce them when/if you start using
>> them.
>>>
>>> Will fix in v2.
>>>
>>>>> +};
>>>>> +
>>>>> +static int versal_fpga_ops_write_init(struct fpga_manager *mgr,
>>>>> +				      struct fpga_image_info *info,
>>>>> +				      const char *buf, size_t size) {
>>>>> +	struct versal_fpga_priv *priv;
>>>>> +
>>>>> +	priv = mgr->priv;
>>>>> +	priv->flags = info->flags;
>>>> ? What uses this ? It seems this function could just be 'return 0' right now.
>>>
>>> Will fix in v2.
>>>
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static int versal_fpga_ops_write(struct fpga_manager *mgr,
>>>>> +				 const char *buf, size_t size) {
>>>>> +	struct versal_fpga_priv *priv;
>>>>> +	dma_addr_t dma_addr = 0;
>>>>> +	char *kbuf;
>>>>> +	int ret;
>>>>> +
>>>>> +	priv = mgr->priv;
>>>>> +
>>>>> +	kbuf = dma_alloc_coherent(priv->dev, size, &dma_addr,
>>>> GFP_KERNEL);
>>>>> +	if (!kbuf)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	memcpy(kbuf, buf, size);
>>>>> +
>>>>> +	wmb(); /* ensure all writes are done before initiate FW call */
>>>>> +
>>>>> +	ret = zynqmp_pm_load_pdi(PDI_SOURCE_TYPE, dma_addr);
>>>>> +
>>>>> +	dma_free_coherent(priv->dev, 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_OPERATING;
>>>> Is that always the case? Shouldn't that be
>> FPGA_MGR_STATE_UNKNOWN?
>>>
>>> For Versal SoC base PDI is always configured prior to Linux boot up. So I
>> make the fpga state as OPERATING.
>>> Please let know if it is not a proper implementation will think about the
>> alternate solution.
>>
>> So you're saying I can't boot a Versal SoC without a PDI / Bitstream loaded?
>> Interesting :)
>>>
> 
> For Versal SoC Vivado generated base PDI is always needed to bring-up the board. 

Look at PDI as ps7_init/psu_init file but in different format. And
bitstream is optional part of it (like a one partition).

Thanks,
Michal

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

[Michal Simek]

Hi

On 1/27/21 9:57 AM, Nava kishore Manne wrote:
> Hi Moritz,
> 
> 	Please find my response inline.
> 
>> -----Original Message-----
>> From: Moritz Fischer <mdf@kernel.org>
>> Sent: Sunday, January 24, 2021 5:04 AM
>> To: Nava kishore Manne <navam@xilinx.com>
>> Cc: Moritz Fischer <mdf@kernel.org>; trix@redhat.com;
>> robh+dt@kernel.org; Michal Simek <michals@xilinx.com>; linux-
>> fpga@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
>> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git
>> <git@xilinx.com>; chinnikishore369@gmail.com; Appana Durga Kedareswara
>> Rao <appanad@xilinx.com>
>> Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager driver
>>
>> Hi Nava,
>>
>> On Fri, Jan 22, 2021 at 10:34:15AM +0000, Nava kishore Manne wrote:
>>> Hi Moritz,
>>>
>>> 	Thanks for the review.
>>> Please find my response inline.
>>>
>>>> -----Original Message-----
>>>> From: Moritz Fischer <mdf@kernel.org>
>>>> Sent: Tuesday, January 19, 2021 6:03 AM
>>>> To: Nava kishore Manne <navam@xilinx.com>
>>>> Cc: mdf@kernel.org; trix@redhat.com; robh+dt@kernel.org; Michal
>>>> Simek <michals@xilinx.com>; linux-fpga@vger.kernel.org;
>>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>>>> linux- kernel@vger.kernel.org; git <git@xilinx.com>;
>>>> chinnikishore369@gmail.com; Appana Durga Kedareswara Rao
>>>> <appanad@xilinx.com>
>>>> Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager
>>>> driver
>>>>
>>>> Hi Nava,
>>>>
>>>> On Mon, Jan 18, 2021 at 08:13:18AM +0530, Nava kishore Manne wrote:
>>>>> This patch adds driver for versal fpga manager.
>>>> Nit: Add support for Xilinx Versal FPGA manager
>>>
>>> Will fix in v2.
>>>
>>>>>
>>>>> PDI source type can be DDR, OCM, QSPI flash etc..
>>>> No idea what PDI is :)
>>>
>>> Programmable device image (PDI).
>>> This file is generated by Xilinx Vivado tool and it contains configuration data
>> objects.
>>>
>>>>> But driver allocates memory always from DDR, Since driver supports
>>>>> only DDR source type.
>>>>>
>>>>> Signed-off-by: Appana Durga Kedareswara rao
>>>>> <appana.durga.rao@xilinx.com>
>>>>> Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
>>>>> ---
>>>>>  drivers/fpga/Kconfig       |   8 ++
>>>>>  drivers/fpga/Makefile      |   1 +
>>>>>  drivers/fpga/versal-fpga.c | 149
>>>>> +++++++++++++++++++++++++++++++++++++
>>>>>  3 files changed, 158 insertions(+)  create mode 100644
>>>>> drivers/fpga/versal-fpga.c
>>>>>
>>>>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index
>>>>> 5645226ca3ce..9f779c3a6739 100644
>>>>> --- a/drivers/fpga/Kconfig
>>>>> +++ b/drivers/fpga/Kconfig
>>>>> @@ -216,4 +216,12 @@ 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 versal soc firmware
>>>>> +          interface to load programmable logic(PL) images
>>>>> +          on versal soc.
>>>>>  endif # FPGA
>>>>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index
>>>>> d8e21dfc6778..40c9adb6a644 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..2a42aa78b182
>>>>> --- /dev/null
>>>>> +++ b/drivers/fpga/versal-fpga.c
>>>>> @@ -0,0 +1,149 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>>> +/*
>>>>> + * Copyright (C) 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>
>>>>> +
>>>>> +/* Constant Definitions */
>>>>> +#define PDI_SOURCE_TYPE	0xF
>>>>> +
>>>>> +/**
>>>>> + * struct versal_fpga_priv - Private data structure
>>>>> + * @dev:	Device data structure
>>>>> + * @flags:	flags which is used to identify the PL Image type
>>>>> + */
>>>>> +struct versal_fpga_priv {
>>>>> +	struct device *dev;
>>>>> +	u32 flags;
>>>> This seems unused ... please introduce them when/if you start using
>> them.
>>>
>>> Will fix in v2.
>>>
>>>>> +};
>>>>> +
>>>>> +static int versal_fpga_ops_write_init(struct fpga_manager *mgr,
>>>>> +				      struct fpga_image_info *info,
>>>>> +				      const char *buf, size_t size) {
>>>>> +	struct versal_fpga_priv *priv;
>>>>> +
>>>>> +	priv = mgr->priv;
>>>>> +	priv->flags = info->flags;
>>>> ? What uses this ? It seems this function could just be 'return 0' right now.
>>>
>>> Will fix in v2.
>>>
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static int versal_fpga_ops_write(struct fpga_manager *mgr,
>>>>> +				 const char *buf, size_t size) {
>>>>> +	struct versal_fpga_priv *priv;
>>>>> +	dma_addr_t dma_addr = 0;
>>>>> +	char *kbuf;
>>>>> +	int ret;
>>>>> +
>>>>> +	priv = mgr->priv;
>>>>> +
>>>>> +	kbuf = dma_alloc_coherent(priv->dev, size, &dma_addr,
>>>> GFP_KERNEL);
>>>>> +	if (!kbuf)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	memcpy(kbuf, buf, size);
>>>>> +
>>>>> +	wmb(); /* ensure all writes are done before initiate FW call */
>>>>> +
>>>>> +	ret = zynqmp_pm_load_pdi(PDI_SOURCE_TYPE, dma_addr);
>>>>> +
>>>>> +	dma_free_coherent(priv->dev, 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_OPERATING;
>>>> Is that always the case? Shouldn't that be
>> FPGA_MGR_STATE_UNKNOWN?
>>>
>>> For Versal SoC base PDI is always configured prior to Linux boot up. So I
>> make the fpga state as OPERATING.
>>> Please let know if it is not a proper implementation will think about the
>> alternate solution.
>>
>> So you're saying I can't boot a Versal SoC without a PDI / Bitstream loaded?
>> Interesting :)
>>>
> 
> For Versal SoC Vivado generated base PDI is always needed to bring-up the board. 

Look at PDI as ps7_init/psu_init file but in different format. And
bitstream is optional part of it (like a one partition).

Thanks,
Michal


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Moritz Fischer]

On Wed, Jan 27, 2021 at 10:16:32AM +0100, Michal Simek wrote:
> Hi
> 
> On 1/27/21 9:57 AM, Nava kishore Manne wrote:
> > Hi Moritz,
> > 
> > 	Please find my response inline.
> > 
> >> -----Original Message-----
> >> From: Moritz Fischer <mdf@kernel.org>
> >> Sent: Sunday, January 24, 2021 5:04 AM
> >> To: Nava kishore Manne <navam@xilinx.com>
> >> Cc: Moritz Fischer <mdf@kernel.org>; trix@redhat.com;
> >> robh+dt@kernel.org; Michal Simek <michals@xilinx.com>; linux-
> >> fpga@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> >> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git
> >> <git@xilinx.com>; chinnikishore369@gmail.com; Appana Durga Kedareswara
> >> Rao <appanad@xilinx.com>
> >> Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager driver
> >>
> >> Hi Nava,
> >>
> >> On Fri, Jan 22, 2021 at 10:34:15AM +0000, Nava kishore Manne wrote:
> >>> Hi Moritz,
> >>>
> >>> 	Thanks for the review.
> >>> Please find my response inline.
> >>>
> >>>> -----Original Message-----
> >>>> From: Moritz Fischer <mdf@kernel.org>
> >>>> Sent: Tuesday, January 19, 2021 6:03 AM
> >>>> To: Nava kishore Manne <navam@xilinx.com>
> >>>> Cc: mdf@kernel.org; trix@redhat.com; robh+dt@kernel.org; Michal
> >>>> Simek <michals@xilinx.com>; linux-fpga@vger.kernel.org;
> >>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> >>>> linux- kernel@vger.kernel.org; git <git@xilinx.com>;
> >>>> chinnikishore369@gmail.com; Appana Durga Kedareswara Rao
> >>>> <appanad@xilinx.com>
> >>>> Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager
> >>>> driver
> >>>>
> >>>> Hi Nava,
> >>>>
> >>>> On Mon, Jan 18, 2021 at 08:13:18AM +0530, Nava kishore Manne wrote:
> >>>>> This patch adds driver for versal fpga manager.
> >>>> Nit: Add support for Xilinx Versal FPGA manager
> >>>
> >>> Will fix in v2.
> >>>
> >>>>>
> >>>>> PDI source type can be DDR, OCM, QSPI flash etc..
> >>>> No idea what PDI is :)
> >>>
> >>> Programmable device image (PDI).
> >>> This file is generated by Xilinx Vivado tool and it contains configuration data
> >> objects.
> >>>
> >>>>> But driver allocates memory always from DDR, Since driver supports
> >>>>> only DDR source type.
> >>>>>
> >>>>> Signed-off-by: Appana Durga Kedareswara rao
> >>>>> <appana.durga.rao@xilinx.com>
> >>>>> Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
> >>>>> ---
> >>>>>  drivers/fpga/Kconfig       |   8 ++
> >>>>>  drivers/fpga/Makefile      |   1 +
> >>>>>  drivers/fpga/versal-fpga.c | 149
> >>>>> +++++++++++++++++++++++++++++++++++++
> >>>>>  3 files changed, 158 insertions(+)  create mode 100644
> >>>>> drivers/fpga/versal-fpga.c
> >>>>>
> >>>>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index
> >>>>> 5645226ca3ce..9f779c3a6739 100644
> >>>>> --- a/drivers/fpga/Kconfig
> >>>>> +++ b/drivers/fpga/Kconfig
> >>>>> @@ -216,4 +216,12 @@ 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 versal soc firmware
> >>>>> +          interface to load programmable logic(PL) images
> >>>>> +          on versal soc.
> >>>>>  endif # FPGA
> >>>>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index
> >>>>> d8e21dfc6778..40c9adb6a644 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..2a42aa78b182
> >>>>> --- /dev/null
> >>>>> +++ b/drivers/fpga/versal-fpga.c
> >>>>> @@ -0,0 +1,149 @@
> >>>>> +// SPDX-License-Identifier: GPL-2.0+
> >>>>> +/*
> >>>>> + * Copyright (C) 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>
> >>>>> +
> >>>>> +/* Constant Definitions */
> >>>>> +#define PDI_SOURCE_TYPE	0xF
> >>>>> +
> >>>>> +/**
> >>>>> + * struct versal_fpga_priv - Private data structure
> >>>>> + * @dev:	Device data structure
> >>>>> + * @flags:	flags which is used to identify the PL Image type
> >>>>> + */
> >>>>> +struct versal_fpga_priv {
> >>>>> +	struct device *dev;
> >>>>> +	u32 flags;
> >>>> This seems unused ... please introduce them when/if you start using
> >> them.
> >>>
> >>> Will fix in v2.
> >>>
> >>>>> +};
> >>>>> +
> >>>>> +static int versal_fpga_ops_write_init(struct fpga_manager *mgr,
> >>>>> +				      struct fpga_image_info *info,
> >>>>> +				      const char *buf, size_t size) {
> >>>>> +	struct versal_fpga_priv *priv;
> >>>>> +
> >>>>> +	priv = mgr->priv;
> >>>>> +	priv->flags = info->flags;
> >>>> ? What uses this ? It seems this function could just be 'return 0' right now.
> >>>
> >>> Will fix in v2.
> >>>
> >>>>> +
> >>>>> +	return 0;
> >>>>> +}
> >>>>> +
> >>>>> +static int versal_fpga_ops_write(struct fpga_manager *mgr,
> >>>>> +				 const char *buf, size_t size) {
> >>>>> +	struct versal_fpga_priv *priv;
> >>>>> +	dma_addr_t dma_addr = 0;
> >>>>> +	char *kbuf;
> >>>>> +	int ret;
> >>>>> +
> >>>>> +	priv = mgr->priv;
> >>>>> +
> >>>>> +	kbuf = dma_alloc_coherent(priv->dev, size, &dma_addr,
> >>>> GFP_KERNEL);
> >>>>> +	if (!kbuf)
> >>>>> +		return -ENOMEM;
> >>>>> +
> >>>>> +	memcpy(kbuf, buf, size);
> >>>>> +
> >>>>> +	wmb(); /* ensure all writes are done before initiate FW call */
> >>>>> +
> >>>>> +	ret = zynqmp_pm_load_pdi(PDI_SOURCE_TYPE, dma_addr);
> >>>>> +
> >>>>> +	dma_free_coherent(priv->dev, 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_OPERATING;
> >>>> Is that always the case? Shouldn't that be
> >> FPGA_MGR_STATE_UNKNOWN?
> >>>
> >>> For Versal SoC base PDI is always configured prior to Linux boot up. So I
> >> make the fpga state as OPERATING.
> >>> Please let know if it is not a proper implementation will think about the
> >> alternate solution.
> >>
> >> So you're saying I can't boot a Versal SoC without a PDI / Bitstream loaded?
> >> Interesting :)
> >>>
> > 
> > For Versal SoC Vivado generated base PDI is always needed to bring-up the board. 
> 
> Look at PDI as ps7_init/psu_init file but in different format. And
> bitstream is optional part of it (like a one partition).

So at that point I could still have no bitstream loaded (optional), and
my status would be 'unknown' not 'operating' if I cannot tell the two
cases apart. What am I missing? :)

Thanks,
Moritz

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

[Moritz Fischer]

On Wed, Jan 27, 2021 at 10:16:32AM +0100, Michal Simek wrote:
> Hi
> 
> On 1/27/21 9:57 AM, Nava kishore Manne wrote:
> > Hi Moritz,
> > 
> > 	Please find my response inline.
> > 
> >> -----Original Message-----
> >> From: Moritz Fischer <mdf@kernel.org>
> >> Sent: Sunday, January 24, 2021 5:04 AM
> >> To: Nava kishore Manne <navam@xilinx.com>
> >> Cc: Moritz Fischer <mdf@kernel.org>; trix@redhat.com;
> >> robh+dt@kernel.org; Michal Simek <michals@xilinx.com>; linux-
> >> fpga@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> >> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git
> >> <git@xilinx.com>; chinnikishore369@gmail.com; Appana Durga Kedareswara
> >> Rao <appanad@xilinx.com>
> >> Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager driver
> >>
> >> Hi Nava,
> >>
> >> On Fri, Jan 22, 2021 at 10:34:15AM +0000, Nava kishore Manne wrote:
> >>> Hi Moritz,
> >>>
> >>> 	Thanks for the review.
> >>> Please find my response inline.
> >>>
> >>>> -----Original Message-----
> >>>> From: Moritz Fischer <mdf@kernel.org>
> >>>> Sent: Tuesday, January 19, 2021 6:03 AM
> >>>> To: Nava kishore Manne <navam@xilinx.com>
> >>>> Cc: mdf@kernel.org; trix@redhat.com; robh+dt@kernel.org; Michal
> >>>> Simek <michals@xilinx.com>; linux-fpga@vger.kernel.org;
> >>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> >>>> linux- kernel@vger.kernel.org; git <git@xilinx.com>;
> >>>> chinnikishore369@gmail.com; Appana Durga Kedareswara Rao
> >>>> <appanad@xilinx.com>
> >>>> Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager
> >>>> driver
> >>>>
> >>>> Hi Nava,
> >>>>
> >>>> On Mon, Jan 18, 2021 at 08:13:18AM +0530, Nava kishore Manne wrote:
> >>>>> This patch adds driver for versal fpga manager.
> >>>> Nit: Add support for Xilinx Versal FPGA manager
> >>>
> >>> Will fix in v2.
> >>>
> >>>>>
> >>>>> PDI source type can be DDR, OCM, QSPI flash etc..
> >>>> No idea what PDI is :)
> >>>
> >>> Programmable device image (PDI).
> >>> This file is generated by Xilinx Vivado tool and it contains configuration data
> >> objects.
> >>>
> >>>>> But driver allocates memory always from DDR, Since driver supports
> >>>>> only DDR source type.
> >>>>>
> >>>>> Signed-off-by: Appana Durga Kedareswara rao
> >>>>> <appana.durga.rao@xilinx.com>
> >>>>> Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
> >>>>> ---
> >>>>>  drivers/fpga/Kconfig       |   8 ++
> >>>>>  drivers/fpga/Makefile      |   1 +
> >>>>>  drivers/fpga/versal-fpga.c | 149
> >>>>> +++++++++++++++++++++++++++++++++++++
> >>>>>  3 files changed, 158 insertions(+)  create mode 100644
> >>>>> drivers/fpga/versal-fpga.c
> >>>>>
> >>>>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index
> >>>>> 5645226ca3ce..9f779c3a6739 100644
> >>>>> --- a/drivers/fpga/Kconfig
> >>>>> +++ b/drivers/fpga/Kconfig
> >>>>> @@ -216,4 +216,12 @@ 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 versal soc firmware
> >>>>> +          interface to load programmable logic(PL) images
> >>>>> +          on versal soc.
> >>>>>  endif # FPGA
> >>>>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index
> >>>>> d8e21dfc6778..40c9adb6a644 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..2a42aa78b182
> >>>>> --- /dev/null
> >>>>> +++ b/drivers/fpga/versal-fpga.c
> >>>>> @@ -0,0 +1,149 @@
> >>>>> +// SPDX-License-Identifier: GPL-2.0+
> >>>>> +/*
> >>>>> + * Copyright (C) 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>
> >>>>> +
> >>>>> +/* Constant Definitions */
> >>>>> +#define PDI_SOURCE_TYPE	0xF
> >>>>> +
> >>>>> +/**
> >>>>> + * struct versal_fpga_priv - Private data structure
> >>>>> + * @dev:	Device data structure
> >>>>> + * @flags:	flags which is used to identify the PL Image type
> >>>>> + */
> >>>>> +struct versal_fpga_priv {
> >>>>> +	struct device *dev;
> >>>>> +	u32 flags;
> >>>> This seems unused ... please introduce them when/if you start using
> >> them.
> >>>
> >>> Will fix in v2.
> >>>
> >>>>> +};
> >>>>> +
> >>>>> +static int versal_fpga_ops_write_init(struct fpga_manager *mgr,
> >>>>> +				      struct fpga_image_info *info,
> >>>>> +				      const char *buf, size_t size) {
> >>>>> +	struct versal_fpga_priv *priv;
> >>>>> +
> >>>>> +	priv = mgr->priv;
> >>>>> +	priv->flags = info->flags;
> >>>> ? What uses this ? It seems this function could just be 'return 0' right now.
> >>>
> >>> Will fix in v2.
> >>>
> >>>>> +
> >>>>> +	return 0;
> >>>>> +}
> >>>>> +
> >>>>> +static int versal_fpga_ops_write(struct fpga_manager *mgr,
> >>>>> +				 const char *buf, size_t size) {
> >>>>> +	struct versal_fpga_priv *priv;
> >>>>> +	dma_addr_t dma_addr = 0;
> >>>>> +	char *kbuf;
> >>>>> +	int ret;
> >>>>> +
> >>>>> +	priv = mgr->priv;
> >>>>> +
> >>>>> +	kbuf = dma_alloc_coherent(priv->dev, size, &dma_addr,
> >>>> GFP_KERNEL);
> >>>>> +	if (!kbuf)
> >>>>> +		return -ENOMEM;
> >>>>> +
> >>>>> +	memcpy(kbuf, buf, size);
> >>>>> +
> >>>>> +	wmb(); /* ensure all writes are done before initiate FW call */
> >>>>> +
> >>>>> +	ret = zynqmp_pm_load_pdi(PDI_SOURCE_TYPE, dma_addr);
> >>>>> +
> >>>>> +	dma_free_coherent(priv->dev, 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_OPERATING;
> >>>> Is that always the case? Shouldn't that be
> >> FPGA_MGR_STATE_UNKNOWN?
> >>>
> >>> For Versal SoC base PDI is always configured prior to Linux boot up. So I
> >> make the fpga state as OPERATING.
> >>> Please let know if it is not a proper implementation will think about the
> >> alternate solution.
> >>
> >> So you're saying I can't boot a Versal SoC without a PDI / Bitstream loaded?
> >> Interesting :)
> >>>
> > 
> > For Versal SoC Vivado generated base PDI is always needed to bring-up the board. 
> 
> Look at PDI as ps7_init/psu_init file but in different format. And
> bitstream is optional part of it (like a one partition).

So at that point I could still have no bitstream loaded (optional), and
my status would be 'unknown' not 'operating' if I cannot tell the two
cases apart. What am I missing? :)

Thanks,
Moritz

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager driver

[Nava kishore Manne]

Hi,

> -----Original Message-----
> From: Moritz Fischer <mdf@kernel.org>
> Sent: Thursday, January 28, 2021 3:15 AM
> To: Michal Simek <michals@xilinx.com>
> Cc: Nava kishore Manne <navam@xilinx.com>; Moritz Fischer
> <mdf@kernel.org>; trix@redhat.com; robh+dt@kernel.org; linux-
> fpga@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git
> <git@xilinx.com>; chinnikishore369@gmail.com; Appana Durga Kedareswara
> Rao <appanad@xilinx.com>
> Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager driver
> 
> On Wed, Jan 27, 2021 at 10:16:32AM +0100, Michal Simek wrote:
> > Hi
> >
> > On 1/27/21 9:57 AM, Nava kishore Manne wrote:
> > > Hi Moritz,
> > >
> > > 	Please find my response inline.
> > >
> > >> -----Original Message-----
> > >> From: Moritz Fischer <mdf@kernel.org>
> > >> Sent: Sunday, January 24, 2021 5:04 AM
> > >> To: Nava kishore Manne <navam@xilinx.com>
> > >> Cc: Moritz Fischer <mdf@kernel.org>; trix@redhat.com;
> > >> robh+dt@kernel.org; Michal Simek <michals@xilinx.com>; linux-
> > >> fpga@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> > >> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git
> > >> <git@xilinx.com>; chinnikishore369@gmail.com; Appana Durga
> > >> Kedareswara Rao <appanad@xilinx.com>
> > >> Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager
> > >> driver
> > >>
> > >> Hi Nava,
> > >>
> > >> On Fri, Jan 22, 2021 at 10:34:15AM +0000, Nava kishore Manne wrote:
> > >>> Hi Moritz,
> > >>>
> > >>> 	Thanks for the review.
> > >>> Please find my response inline.
> > >>>
> > >>>> -----Original Message-----
> > >>>> From: Moritz Fischer <mdf@kernel.org>
> > >>>> Sent: Tuesday, January 19, 2021 6:03 AM
> > >>>> To: Nava kishore Manne <navam@xilinx.com>
> > >>>> Cc: mdf@kernel.org; trix@redhat.com; robh+dt@kernel.org; Michal
> > >>>> Simek <michals@xilinx.com>; linux-fpga@vger.kernel.org;
> > >>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > >>>> linux- kernel@vger.kernel.org; git <git@xilinx.com>;
> > >>>> chinnikishore369@gmail.com; Appana Durga Kedareswara Rao
> > >>>> <appanad@xilinx.com>
> > >>>> Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga
> > >>>> manager driver
> > >>>>
> > >>>> Hi Nava,
> > >>>>
> > >>>> On Mon, Jan 18, 2021 at 08:13:18AM +0530, Nava kishore Manne
> wrote:
> > >>>>> This patch adds driver for versal fpga manager.
> > >>>> Nit: Add support for Xilinx Versal FPGA manager
> > >>>
> > >>> Will fix in v2.
> > >>>
> > >>>>>
> > >>>>> PDI source type can be DDR, OCM, QSPI flash etc..
> > >>>> No idea what PDI is :)
> > >>>
> > >>> Programmable device image (PDI).
> > >>> This file is generated by Xilinx Vivado tool and it contains
> > >>> configuration data
> > >> objects.
> > >>>
> > >>>>> But driver allocates memory always from DDR, Since driver
> > >>>>> supports only DDR source type.
> > >>>>>
> > >>>>> Signed-off-by: Appana Durga Kedareswara rao
> > >>>>> <appana.durga.rao@xilinx.com>
> > >>>>> Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
> > >>>>> ---
> > >>>>>  drivers/fpga/Kconfig       |   8 ++
> > >>>>>  drivers/fpga/Makefile      |   1 +
> > >>>>>  drivers/fpga/versal-fpga.c | 149
> > >>>>> +++++++++++++++++++++++++++++++++++++
> > >>>>>  3 files changed, 158 insertions(+)  create mode 100644
> > >>>>> drivers/fpga/versal-fpga.c
> > >>>>>
> > >>>>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index
> > >>>>> 5645226ca3ce..9f779c3a6739 100644
> > >>>>> --- a/drivers/fpga/Kconfig
> > >>>>> +++ b/drivers/fpga/Kconfig
> > >>>>> @@ -216,4 +216,12 @@ 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 versal soc firmware
> > >>>>> +          interface to load programmable logic(PL) images
> > >>>>> +          on versal soc.
> > >>>>>  endif # FPGA
> > >>>>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index
> > >>>>> d8e21dfc6778..40c9adb6a644 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..2a42aa78b182
> > >>>>> --- /dev/null
> > >>>>> +++ b/drivers/fpga/versal-fpga.c
> > >>>>> @@ -0,0 +1,149 @@
> > >>>>> +// SPDX-License-Identifier: GPL-2.0+
> > >>>>> +/*
> > >>>>> + * Copyright (C) 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>
> > >>>>> +
> > >>>>> +/* Constant Definitions */
> > >>>>> +#define PDI_SOURCE_TYPE	0xF
> > >>>>> +
> > >>>>> +/**
> > >>>>> + * struct versal_fpga_priv - Private data structure
> > >>>>> + * @dev:	Device data structure
> > >>>>> + * @flags:	flags which is used to identify the PL Image type
> > >>>>> + */
> > >>>>> +struct versal_fpga_priv {
> > >>>>> +	struct device *dev;
> > >>>>> +	u32 flags;
> > >>>> This seems unused ... please introduce them when/if you start
> > >>>> using
> > >> them.
> > >>>
> > >>> Will fix in v2.
> > >>>
> > >>>>> +};
> > >>>>> +
> > >>>>> +static int versal_fpga_ops_write_init(struct fpga_manager *mgr,
> > >>>>> +				      struct fpga_image_info *info,
> > >>>>> +				      const char *buf, size_t size) {
> > >>>>> +	struct versal_fpga_priv *priv;
> > >>>>> +
> > >>>>> +	priv = mgr->priv;
> > >>>>> +	priv->flags = info->flags;
> > >>>> ? What uses this ? It seems this function could just be 'return 0' right
> now.
> > >>>
> > >>> Will fix in v2.
> > >>>
> > >>>>> +
> > >>>>> +	return 0;
> > >>>>> +}
> > >>>>> +
> > >>>>> +static int versal_fpga_ops_write(struct fpga_manager *mgr,
> > >>>>> +				 const char *buf, size_t size) {
> > >>>>> +	struct versal_fpga_priv *priv;
> > >>>>> +	dma_addr_t dma_addr = 0;
> > >>>>> +	char *kbuf;
> > >>>>> +	int ret;
> > >>>>> +
> > >>>>> +	priv = mgr->priv;
> > >>>>> +
> > >>>>> +	kbuf = dma_alloc_coherent(priv->dev, size, &dma_addr,
> > >>>> GFP_KERNEL);
> > >>>>> +	if (!kbuf)
> > >>>>> +		return -ENOMEM;
> > >>>>> +
> > >>>>> +	memcpy(kbuf, buf, size);
> > >>>>> +
> > >>>>> +	wmb(); /* ensure all writes are done before initiate FW call
> > >>>>> +*/
> > >>>>> +
> > >>>>> +	ret = zynqmp_pm_load_pdi(PDI_SOURCE_TYPE, dma_addr);
> > >>>>> +
> > >>>>> +	dma_free_coherent(priv->dev, 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_OPERATING;
> > >>>> Is that always the case? Shouldn't that be
> > >> FPGA_MGR_STATE_UNKNOWN?
> > >>>
> > >>> For Versal SoC base PDI is always configured prior to Linux boot
> > >>> up. So I
> > >> make the fpga state as OPERATING.
> > >>> Please let know if it is not a proper implementation will think
> > >>> about the
> > >> alternate solution.
> > >>
> > >> So you're saying I can't boot a Versal SoC without a PDI / Bitstream
> loaded?
> > >> Interesting :)
> > >>>
> > >
> > > For Versal SoC Vivado generated base PDI is always needed to bring-up
> the board.
> >
> > Look at PDI as ps7_init/psu_init file but in different format. And
> > bitstream is optional part of it (like a one partition).
> 
> So at that point I could still have no bitstream loaded (optional), and my
> status would be 'unknown' not 'operating' if I cannot tell the two cases apart.
> What am I missing? :)
> 

Agree, In few cases Bitstream partition is optional, So we can't say exactly whether the PL is having Bitstream or Not . So here the ideal default PL state should be Unknown.
Will fix in v2.

Regards,
Navakishore.

 

RE: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager driver

[Nava kishore Manne]

Hi,

> -----Original Message-----
> From: Moritz Fischer <mdf@kernel.org>
> Sent: Thursday, January 28, 2021 3:15 AM
> To: Michal Simek <michals@xilinx.com>
> Cc: Nava kishore Manne <navam@xilinx.com>; Moritz Fischer
> <mdf@kernel.org>; trix@redhat.com; robh+dt@kernel.org; linux-
> fpga@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git
> <git@xilinx.com>; chinnikishore369@gmail.com; Appana Durga Kedareswara
> Rao <appanad@xilinx.com>
> Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager driver
> 
> On Wed, Jan 27, 2021 at 10:16:32AM +0100, Michal Simek wrote:
> > Hi
> >
> > On 1/27/21 9:57 AM, Nava kishore Manne wrote:
> > > Hi Moritz,
> > >
> > > 	Please find my response inline.
> > >
> > >> -----Original Message-----
> > >> From: Moritz Fischer <mdf@kernel.org>
> > >> Sent: Sunday, January 24, 2021 5:04 AM
> > >> To: Nava kishore Manne <navam@xilinx.com>
> > >> Cc: Moritz Fischer <mdf@kernel.org>; trix@redhat.com;
> > >> robh+dt@kernel.org; Michal Simek <michals@xilinx.com>; linux-
> > >> fpga@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> > >> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git
> > >> <git@xilinx.com>; chinnikishore369@gmail.com; Appana Durga
> > >> Kedareswara Rao <appanad@xilinx.com>
> > >> Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga manager
> > >> driver
> > >>
> > >> Hi Nava,
> > >>
> > >> On Fri, Jan 22, 2021 at 10:34:15AM +0000, Nava kishore Manne wrote:
> > >>> Hi Moritz,
> > >>>
> > >>> 	Thanks for the review.
> > >>> Please find my response inline.
> > >>>
> > >>>> -----Original Message-----
> > >>>> From: Moritz Fischer <mdf@kernel.org>
> > >>>> Sent: Tuesday, January 19, 2021 6:03 AM
> > >>>> To: Nava kishore Manne <navam@xilinx.com>
> > >>>> Cc: mdf@kernel.org; trix@redhat.com; robh+dt@kernel.org; Michal
> > >>>> Simek <michals@xilinx.com>; linux-fpga@vger.kernel.org;
> > >>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > >>>> linux- kernel@vger.kernel.org; git <git@xilinx.com>;
> > >>>> chinnikishore369@gmail.com; Appana Durga Kedareswara Rao
> > >>>> <appanad@xilinx.com>
> > >>>> Subject: Re: [PATCH 3/3] fpga: versal-fpga: Add versal fpga
> > >>>> manager driver
> > >>>>
> > >>>> Hi Nava,
> > >>>>
> > >>>> On Mon, Jan 18, 2021 at 08:13:18AM +0530, Nava kishore Manne
> wrote:
> > >>>>> This patch adds driver for versal fpga manager.
> > >>>> Nit: Add support for Xilinx Versal FPGA manager
> > >>>
> > >>> Will fix in v2.
> > >>>
> > >>>>>
> > >>>>> PDI source type can be DDR, OCM, QSPI flash etc..
> > >>>> No idea what PDI is :)
> > >>>
> > >>> Programmable device image (PDI).
> > >>> This file is generated by Xilinx Vivado tool and it contains
> > >>> configuration data
> > >> objects.
> > >>>
> > >>>>> But driver allocates memory always from DDR, Since driver
> > >>>>> supports only DDR source type.
> > >>>>>
> > >>>>> Signed-off-by: Appana Durga Kedareswara rao
> > >>>>> <appana.durga.rao@xilinx.com>
> > >>>>> Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
> > >>>>> ---
> > >>>>>  drivers/fpga/Kconfig       |   8 ++
> > >>>>>  drivers/fpga/Makefile      |   1 +
> > >>>>>  drivers/fpga/versal-fpga.c | 149
> > >>>>> +++++++++++++++++++++++++++++++++++++
> > >>>>>  3 files changed, 158 insertions(+)  create mode 100644
> > >>>>> drivers/fpga/versal-fpga.c
> > >>>>>
> > >>>>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index
> > >>>>> 5645226ca3ce..9f779c3a6739 100644
> > >>>>> --- a/drivers/fpga/Kconfig
> > >>>>> +++ b/drivers/fpga/Kconfig
> > >>>>> @@ -216,4 +216,12 @@ 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 versal soc firmware
> > >>>>> +          interface to load programmable logic(PL) images
> > >>>>> +          on versal soc.
> > >>>>>  endif # FPGA
> > >>>>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index
> > >>>>> d8e21dfc6778..40c9adb6a644 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..2a42aa78b182
> > >>>>> --- /dev/null
> > >>>>> +++ b/drivers/fpga/versal-fpga.c
> > >>>>> @@ -0,0 +1,149 @@
> > >>>>> +// SPDX-License-Identifier: GPL-2.0+
> > >>>>> +/*
> > >>>>> + * Copyright (C) 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>
> > >>>>> +
> > >>>>> +/* Constant Definitions */
> > >>>>> +#define PDI_SOURCE_TYPE	0xF
> > >>>>> +
> > >>>>> +/**
> > >>>>> + * struct versal_fpga_priv - Private data structure
> > >>>>> + * @dev:	Device data structure
> > >>>>> + * @flags:	flags which is used to identify the PL Image type
> > >>>>> + */
> > >>>>> +struct versal_fpga_priv {
> > >>>>> +	struct device *dev;
> > >>>>> +	u32 flags;
> > >>>> This seems unused ... please introduce them when/if you start
> > >>>> using
> > >> them.
> > >>>
> > >>> Will fix in v2.
> > >>>
> > >>>>> +};
> > >>>>> +
> > >>>>> +static int versal_fpga_ops_write_init(struct fpga_manager *mgr,
> > >>>>> +				      struct fpga_image_info *info,
> > >>>>> +				      const char *buf, size_t size) {
> > >>>>> +	struct versal_fpga_priv *priv;
> > >>>>> +
> > >>>>> +	priv = mgr->priv;
> > >>>>> +	priv->flags = info->flags;
> > >>>> ? What uses this ? It seems this function could just be 'return 0' right
> now.
> > >>>
> > >>> Will fix in v2.
> > >>>
> > >>>>> +
> > >>>>> +	return 0;
> > >>>>> +}
> > >>>>> +
> > >>>>> +static int versal_fpga_ops_write(struct fpga_manager *mgr,
> > >>>>> +				 const char *buf, size_t size) {
> > >>>>> +	struct versal_fpga_priv *priv;
> > >>>>> +	dma_addr_t dma_addr = 0;
> > >>>>> +	char *kbuf;
> > >>>>> +	int ret;
> > >>>>> +
> > >>>>> +	priv = mgr->priv;
> > >>>>> +
> > >>>>> +	kbuf = dma_alloc_coherent(priv->dev, size, &dma_addr,
> > >>>> GFP_KERNEL);
> > >>>>> +	if (!kbuf)
> > >>>>> +		return -ENOMEM;
> > >>>>> +
> > >>>>> +	memcpy(kbuf, buf, size);
> > >>>>> +
> > >>>>> +	wmb(); /* ensure all writes are done before initiate FW call
> > >>>>> +*/
> > >>>>> +
> > >>>>> +	ret = zynqmp_pm_load_pdi(PDI_SOURCE_TYPE, dma_addr);
> > >>>>> +
> > >>>>> +	dma_free_coherent(priv->dev, 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_OPERATING;
> > >>>> Is that always the case? Shouldn't that be
> > >> FPGA_MGR_STATE_UNKNOWN?
> > >>>
> > >>> For Versal SoC base PDI is always configured prior to Linux boot
> > >>> up. So I
> > >> make the fpga state as OPERATING.
> > >>> Please let know if it is not a proper implementation will think
> > >>> about the
> > >> alternate solution.
> > >>
> > >> So you're saying I can't boot a Versal SoC without a PDI / Bitstream
> loaded?
> > >> Interesting :)
> > >>>
> > >
> > > For Versal SoC Vivado generated base PDI is always needed to bring-up
> the board.
> >
> > Look at PDI as ps7_init/psu_init file but in different format. And
> > bitstream is optional part of it (like a one partition).
> 
> So at that point I could still have no bitstream loaded (optional), and my
> status would be 'unknown' not 'operating' if I cannot tell the two cases apart.
> What am I missing? :)
> 

Agree, In few cases Bitstream partition is optional, So we can't say exactly whether the PL is having Bitstream or Not . So here the ideal default PL state should be Unknown.
Will fix in v2.

Regards,
Navakishore.

 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel