Hi Yilun,

	Please find my response inline.

-----Original Message-----
From: Xu Yilun <yilun.xu@intel.com>
Sent: Saturday, May 28, 2022 8:51 PM
To: Nava kishore Manne <redacted>
Cc: Michal Simek <redacted>; mdf@kernel.org;
hao.wu@intel.com; trix@redhat.com; gregkh@linuxfoundation.org; Ronak
Jain [off-list ref]; Abhyuday Godhasara [off-list ref];
Rajan Vaja [off-list ref]; Sai Krishna Potthuri
[off-list ref]; Piyush Mehta [off-list ref]; Harsha
Harsha [off-list ref]; linux-arm-kernel@lists.infradead.org; linux-
kernel@vger.kernel.org; linux-fpga@vger.kernel.org; git [off-list ref]
Subject: Re: [PATCH 3/3] fpga: zynqmp-fpga: Adds status interface

On Tue, May 24, 2022 at 03:17:45PM +0530, Nava kishore Manne wrote:

quoted

Adds status interface for zynqmp-fpga, It's a read only interface
which allows the user to get the PL status.

Usage:
To read the PL configuration status
        cat /sys/class/fpga_manager/<fpga>/status

Signed-off-by: Nava kishore Manne <redacted>
---
 drivers/fpga/zynqmp-fpga.c | 52
++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/drivers/fpga/zynqmp-fpga.c b/drivers/fpga/zynqmp-fpga.c
index c60f20949c47..07c7b7326726 100644
--- a/drivers/fpga/zynqmp-fpga.c
+++ b/drivers/fpga/zynqmp-fpga.c

@@ -14,6 +14,19 @@

 /* Constant Definitions */
 #define IXR_FPGA_DONE_MASK	BIT(3)
+#define READ_DMA_SIZE		256U
+
+/* Error Register */
+#define IXR_FPGA_ERR_CRC_ERR		BIT(0)
+#define IXR_FPGA_ERR_SECURITY_ERR	BIT(16)
+
+/* Signal Status Register. For details refer ug570 */
+#define IXR_FPGA_END_OF_STARTUP		BIT(4)
+#define IXR_FPGA_GST_CFG_B		BIT(5)
+#define IXR_FPGA_INIT_B_INTERNAL	BIT(11)
+#define IXR_FPGA_DONE_INTERNAL_SIGNAL	BIT(13)
+
+#define IXR_FPGA_CONFIG_STAT_OFFSET	7U

 /**
  * struct zynqmp_fpga_priv - Private data structure @@ -77,8 +90,47

@@ static enum fpga_mgr_states zynqmp_fpga_ops_state(struct

fpga_manager *mgr)

quoted

 	return FPGA_MGR_STATE_UNKNOWN;
 }

+static u64 zynqmp_fpga_ops_status(struct fpga_manager *mgr) {
+	unsigned int *buf, reg_val;
+	dma_addr_t dma_addr;
+	u64 status = 0;
+	int ret;
+
+	buf = dma_alloc_coherent(mgr->dev.parent, READ_DMA_SIZE,
+				 &dma_addr, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = zynqmp_pm_fpga_read(IXR_FPGA_CONFIG_STAT_OFFSET,

dma_addr,

quoted

+				  PM_FPGA_READ_CONFIG_REG, &reg_val);
+	if (ret) {
+		status = FPGA_MGR_STATUS_FIRMWARE_REQ_ERR;
+		goto free_dmabuf;
+	}
+
+	if (reg_val & IXR_FPGA_ERR_CRC_ERR)
+		status |= FPGA_MGR_STATUS_CRC_ERR;
+	if (reg_val & IXR_FPGA_ERR_SECURITY_ERR)
+		status |= FPGA_MGR_STATUS_SECURITY_ERR;
+	if (!(reg_val & IXR_FPGA_INIT_B_INTERNAL))
+		status |= FPGA_MGR_STATUS_DEVICE_INIT_ERR;
+	if (!(reg_val & IXR_FPGA_DONE_INTERNAL_SIGNAL))
+		status |= FPGA_MGR_STATUS_SIGNAL_ERR;
+	if (!(reg_val & IXR_FPGA_GST_CFG_B))
+		status |= FPGA_MGR_STATUS_HIGH_Z_STATE_ERR;
+	if (!(reg_val & IXR_FPGA_END_OF_STARTUP))
+		status |= FPGA_MGR_STATUS_EOS_ERR;

I have concern about the status interface. Different vendors have differnt
error sets defined by Hardwares. If we always define the new bits when we
cannot find an exact 1:1 mapping. A 64 bits would soon be used out. Also it's
hard to understand the mixture of different error sets.

I'd rather suggest that each driver define its own error reading interface.

I agree Ideally, the core file should contain only the generic stuff and each vendor has its own set of status messages.
So status related messages should be part of the vendor specific files(not in the core files).
Will update the zynqmp_fpga_ops_status() API to popup the status messages (for ZynqMP FPGA related status messages).

Regards,
Navakishore.


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