Hi Greg,

	Thanks for providing the review comments.
Please find my response inline.

-----Original Message-----
From: Greg KH <gregkh@linuxfoundation.org>
Sent: Tuesday, January 31, 2023 5:13 PM
To: Manne, Nava kishore <redacted>
Cc: michal.simek@xilinx.com; mdf@kernel.org; hao.wu@intel.com;
yilun.xu@intel.com; trix@redhat.com; ronak.jain@xilinx.com;
tanmay.shah@xilinx.com; Levinsky, Ben [off-list ref];
rajan.vaja@xilinx.com; harsha.harsha@xilinx.com;
mathieu.poirier@linaro.org; lakshmi.sai.krishna.potthuri@xilinx.com;
arnd@arndb.de; linux-arm-kernel@lists.infradead.org; linux-
kernel@vger.kernel.org; linux-fpga@vger.kernel.org
Subject: Re: [PATCH v5 2/2] fpga: zynqmp-fpga: Adds status interface

On Tue, Jan 31, 2023 at 03:18:10PM +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 Programmable Logic(PL) configuration
status.

Usage:
To read the Programmable Logic(PL) configuration status
        cat /sys/class/fpga_manager/<fpga>/device/status

Signed-off-by: Nava kishore Manne <redacted>
---
Changes for v2:
              - Updated status messages handling logic as suggested by Xu Yilun.

Changes for v3:
              - Updated status interface handling logic (Restrict the status
                interface to the device-specific instead of handled by the core)
                as suggested by Xu Yilun.

Changes for v4:
              - Limit the error strings to one word for each as suggested by
                Xu Yilun

Changes for v5:
              - Added new sysfs-driver-zynqmp-fpga file.

 .../ABI/testing/sysfs-driver-zynqmp-fpga      | 18 ++++
 drivers/fpga/zynqmp-fpga.c                    | 87 +++++++++++++++++++
 2 files changed, 105 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-zynqmp-fpga

diff --git a/Documentation/ABI/testing/sysfs-driver-zynqmp-fpga

b/Documentation/ABI/testing/sysfs-driver-zynqmp-fpga
new file mode 100644
index 000000000000..66c98bb8dfba

--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-zynqmp-fpga

@@ -0,0 +1,18 @@
+What:		/sys/class/fpga_manager/<fpga>/device/status
+Date:		Jan 2023
+KernelVersion:	6.2
+Contact:	Nava kishore Manne <nava.kishore.manne@amd.com>
+Description:	Read fpga status as a string.
+		If FPGA programming operation fails, it could be caused by

crc

quoted

+		error or incompatible bitstream image. The intent of this
+		interface is to provide more detailed information for FPGA
+		programming errors to userspace. This is a list of strings for
+		the supported status.
+
+		* CRC-Error		- CRC error detected by hardware.
+		* Security-Error	- Security error detected by hardware.
+		* Initialization-Error	- Device Initialization error.
+		* Internal-Signal-Error	- Device internal signal error.
+		* I/Os-High-Z-state	- All I/Os are placed in High-Z state.
+		* Sequence-Error	- Device start-up sequence error.
+		* Firmware-Error	- Firmware error.

diff --git a/drivers/fpga/zynqmp-fpga.c b/drivers/fpga/zynqmp-fpga.c
index c60f20949c47..81d3e18527ee 100644
--- a/drivers/fpga/zynqmp-fpga.c
+++ b/drivers/fpga/zynqmp-fpga.c

@@ -15,6 +15,37 @@
 /* Constant Definitions */
 #define IXR_FPGA_DONE_MASK	BIT(3)

+/* Error Register */
+#define IXR_FPGA_ERR_CRC_ERR		BIT(0)
+#define IXR_FPGA_ERR_SECURITY_ERR	BIT(16)
+
+/* Signal Status Register */
+#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)
+
+/* FPGA error status. */
+enum {
+	ZYNQMP_FPGA_STATUS_CRC_ERR,
+	ZYNQMP_FPGA_STATUS_SECURITY_ERR,
+	ZYNQMP_FPGA_STATUS_DEVICE_INIT_ERR,
+	ZYNQMP_FPGA_STATUS_SIGNAL_ERR,
+	ZYNQMP_FPGA_STATUS_HIGH_Z_STATE_ERR,
+	ZYNQMP_FPGA_STATUS_EOS_ERR,
+	ZYNQMP_FPGA_MGR_STATUS_FIRMWARE_REQ_ERR,
+};
+
+static const char * const zynqmp_fpga_error_statuses[] = {
+	[ZYNQMP_FPGA_STATUS_CRC_ERR] = "CRC-Error",
+	[ZYNQMP_FPGA_STATUS_SECURITY_ERR] = "Security-Error",
+	[ZYNQMP_FPGA_STATUS_DEVICE_INIT_ERR] = "Initialization-Error",
+	[ZYNQMP_FPGA_STATUS_SIGNAL_ERR] = "Internal-Signal-Error",
+	[ZYNQMP_FPGA_STATUS_HIGH_Z_STATE_ERR] = "I/Os-High-Z-state",
+	[ZYNQMP_FPGA_STATUS_EOS_ERR] = "Sequence-Error",
+	[ZYNQMP_FPGA_MGR_STATUS_FIRMWARE_REQ_ERR] = "Firmware-

Error", };

quoted

+
 /**
  * struct zynqmp_fpga_priv - Private data structure
  * @dev:	Device data structure

@@ -77,6 +108,54 @@ static enum fpga_mgr_states

zynqmp_fpga_ops_state(struct fpga_manager *mgr)

quoted

 	return FPGA_MGR_STATE_UNKNOWN;
 }

+static ssize_t status_show(struct device *dev,
+			   struct device_attribute *attr, char *buf) {
+	unsigned long status = 0;
+	ssize_t len = 0;
+	u32 reg_val;
+	int ret;
+	u8 i;
+
+	ret = zynqmp_pm_fpga_get_config_status(&reg_val);
+	if (!ret) {
+		if (reg_val & IXR_FPGA_ERR_CRC_ERR)
+			status |= ZYNQMP_FPGA_STATUS_CRC_ERR;
+		if (reg_val & IXR_FPGA_ERR_SECURITY_ERR)
+			status |= ZYNQMP_FPGA_STATUS_SECURITY_ERR;
+		if (!(reg_val & IXR_FPGA_INIT_B_INTERNAL))
+			status |= ZYNQMP_FPGA_STATUS_DEVICE_INIT_ERR;
+		if (!(reg_val & IXR_FPGA_DONE_INTERNAL_SIGNAL))
+			status |= ZYNQMP_FPGA_STATUS_SIGNAL_ERR;
+		if (!(reg_val & IXR_FPGA_GST_CFG_B))
+			status |=

ZYNQMP_FPGA_STATUS_HIGH_Z_STATE_ERR;

quoted

+		if (!(reg_val & IXR_FPGA_END_OF_STARTUP))
+			status |= ZYNQMP_FPGA_STATUS_EOS_ERR;
+	} else {
+		status =

ZYNQMP_FPGA_MGR_STATUS_FIRMWARE_REQ_ERR;

quoted

+	}
+
+	for_each_set_bit(i, &status,

ARRAY_SIZE(zynqmp_fpga_error_statuses))

quoted

+		len += sysfs_emit_at(buf, len, "%s ",
+				     zynqmp_fpga_error_statuses[i]);
+

Sorry, but no, you can not export a list of strings that userspace is somehow
going to parse.

As this is all a bit field, why not just export the raw hex value instead and let
userspace parse what bits are set?

This implementation is align with the fpga core status interface(https://git.kernel.org/pub/scm/linux/kernel/git/fpga/linux-fpga.git/tree/drivers/fpga/fpga-mgr.c?h=for-6.2#n632)
I Agree to export the raw hex value instead of multiple error strings.
In case of exporting the raw hex value for status can we update this fpga-mgr.c core file (status_show() API) 
instead of creating individual sysfs file per driver/device? and with this proposed implementation
will also fix the issues/Limitations with generic status interface.

Existing limitation with the generic fpga status interface: 
Different vendors have different error sets defined by Hardware. If we always define the new bits when we cannot find an exact 1:1
mapping in the core the 64 bits would soon be used out. 

Xu Yilun: Can you please share your thoughts here?  

Regards,
Navakishore.

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