diff mbox series

[v3,2/2] fpga: zynqmp-fpga: Adds status interface

Message ID 20221013090556.741357-3-nava.kishore.manne@amd.com (mailing list archive)
State New
Headers show
Series Adds status interface for zynqmp-fpga | expand

Commit Message

Manne, Nava kishore Oct. 13, 2022, 9:05 a.m. UTC 
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 <nava.kishore.manne@amd.com>
---
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.

 drivers/fpga/zynqmp-fpga.c | 87 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)

Comments

Greg Kroah-Hartman Oct. 13, 2022, 9:21 a.m. UTC | #1 
On Thu, Oct 13, 2022 at 02:35:56PM +0530, Nava kishore Manne wrote:
> 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 <nava.kishore.manne@amd.com>
> ---
> 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.
> 
>  drivers/fpga/zynqmp-fpga.c | 87 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 87 insertions(+)

You add sysfs files without a Documentation/ABI/ update as well, which
is not allowed.  Please fix that up for your next submission.

> @@ -95,6 +175,13 @@ static int zynqmp_fpga_probe(struct platform_device *pdev)
>  
>  	priv->dev = dev;
>  
> +	/* Add the device attributes */
> +	ret = sysfs_create_group(&dev->kobj, &zynqmp_fpga_attr_group);
> +	if (ret) {
> +		dev_err(dev, "Error creating sysfs files\n");
> +		return ret;
> +	}

You just raced with userspace and lost.  Do not do this, set the default
groups attribute in your platform driver and the driver core will handle
this all for you automatically.

thanks,

greg k-h
Xu Yilun Oct. 14, 2022, 4:02 p.m. UTC | #2 
On 2022-10-13 at 14:35:56 +0530, Nava kishore Manne wrote:
> 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 <nava.kishore.manne@amd.com>
> ---
> 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.
> 
>  drivers/fpga/zynqmp-fpga.c | 87 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 87 insertions(+)
> 
> diff --git a/drivers/fpga/zynqmp-fpga.c b/drivers/fpga/zynqmp-fpga.c
> index c60f20949c47..4e0295486c36 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] = "reconfig CRC error",
> +	[ZYNQMP_FPGA_STATUS_SECURITY_ERR] = "reconfig security error",
> +	[ZYNQMP_FPGA_STATUS_DEVICE_INIT_ERR] = "Device Initialization error",
> +	[ZYNQMP_FPGA_STATUS_SIGNAL_ERR] = "Device internal signal error",
> +	[ZYNQMP_FPGA_STATUS_HIGH_Z_STATE_ERR] = "All I/Os are placed in High-Z state",
> +	[ZYNQMP_FPGA_STATUS_EOS_ERR] = "Device sequence error",
> +	[ZYNQMP_FPGA_MGR_STATUS_FIRMWARE_REQ_ERR] = "firmware request error",

If you have multiple errors, the output would be a mess. You can not
even tell how many errors are there.

For string values, I suggest one word for each, and separated by a space.

Thanks,
Yilun

> +};
> +
>  /**
>   * 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)
>  	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;
> +		if (!(reg_val & IXR_FPGA_END_OF_STARTUP))
> +			status |= ZYNQMP_FPGA_STATUS_EOS_ERR;
> +	} else {
> +		status = ZYNQMP_FPGA_MGR_STATUS_FIRMWARE_REQ_ERR;
> +	}
> +
> +	for_each_set_bit(i, &status, ARRAY_SIZE(zynqmp_fpga_error_statuses))
> +		len += sysfs_emit_at(buf, len, "%s ",
> +				     zynqmp_fpga_error_statuses[i]);
> +
> +	if (len)
> +		buf[len - 1] = '\n';
> +
> +	return len;
> +}
> +
> +static DEVICE_ATTR_RO(status);
> +
> +static struct attribute *zynqmp_fpga_device_attrs[] = {
> +	&dev_attr_status.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group zynqmp_fpga_attr_group = {
> +	.attrs = zynqmp_fpga_device_attrs,
> +};
> +
>  static const struct fpga_manager_ops zynqmp_fpga_ops = {
>  	.state = zynqmp_fpga_ops_state,
>  	.write_init = zynqmp_fpga_ops_write_init,
> @@ -88,6 +167,7 @@ static int zynqmp_fpga_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct zynqmp_fpga_priv *priv;
>  	struct fpga_manager *mgr;
> +	int ret;
>  
>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
> @@ -95,6 +175,13 @@ static int zynqmp_fpga_probe(struct platform_device *pdev)
>  
>  	priv->dev = dev;
>  
> +	/* Add the device attributes */
> +	ret = sysfs_create_group(&dev->kobj, &zynqmp_fpga_attr_group);
> +	if (ret) {
> +		dev_err(dev, "Error creating sysfs files\n");
> +		return ret;
> +	}
> +
>  	mgr = devm_fpga_mgr_register(dev, "Xilinx ZynqMP FPGA Manager",
>  				     &zynqmp_fpga_ops, priv);
>  	return PTR_ERR_OR_ZERO(mgr);
> -- 
> 2.25.1
>
diff mbox series

Patch

diff --git a/drivers/fpga/zynqmp-fpga.c b/drivers/fpga/zynqmp-fpga.c
index c60f20949c47..4e0295486c36 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] = "reconfig CRC error",
+	[ZYNQMP_FPGA_STATUS_SECURITY_ERR] = "reconfig security error",
+	[ZYNQMP_FPGA_STATUS_DEVICE_INIT_ERR] = "Device Initialization error",
+	[ZYNQMP_FPGA_STATUS_SIGNAL_ERR] = "Device internal signal error",
+	[ZYNQMP_FPGA_STATUS_HIGH_Z_STATE_ERR] = "All I/Os are placed in High-Z state",
+	[ZYNQMP_FPGA_STATUS_EOS_ERR] = "Device sequence error",
+	[ZYNQMP_FPGA_MGR_STATUS_FIRMWARE_REQ_ERR] = "firmware request error",
+};
+
 /**
  * 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)
 	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;
+		if (!(reg_val & IXR_FPGA_END_OF_STARTUP))
+			status |= ZYNQMP_FPGA_STATUS_EOS_ERR;
+	} else {
+		status = ZYNQMP_FPGA_MGR_STATUS_FIRMWARE_REQ_ERR;
+	}
+
+	for_each_set_bit(i, &status, ARRAY_SIZE(zynqmp_fpga_error_statuses))
+		len += sysfs_emit_at(buf, len, "%s ",
+				     zynqmp_fpga_error_statuses[i]);
+
+	if (len)
+		buf[len - 1] = '\n';
+
+	return len;
+}
+
+static DEVICE_ATTR_RO(status);
+
+static struct attribute *zynqmp_fpga_device_attrs[] = {
+	&dev_attr_status.attr,
+	NULL,
+};
+
+static const struct attribute_group zynqmp_fpga_attr_group = {
+	.attrs = zynqmp_fpga_device_attrs,
+};
+
 static const struct fpga_manager_ops zynqmp_fpga_ops = {
 	.state = zynqmp_fpga_ops_state,
 	.write_init = zynqmp_fpga_ops_write_init,
@@ -88,6 +167,7 @@  static int zynqmp_fpga_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct zynqmp_fpga_priv *priv;
 	struct fpga_manager *mgr;
+	int ret;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -95,6 +175,13 @@  static int zynqmp_fpga_probe(struct platform_device *pdev)
 
 	priv->dev = dev;
 
+	/* Add the device attributes */
+	ret = sysfs_create_group(&dev->kobj, &zynqmp_fpga_attr_group);
+	if (ret) {
+		dev_err(dev, "Error creating sysfs files\n");
+		return ret;
+	}
+
 	mgr = devm_fpga_mgr_register(dev, "Xilinx ZynqMP FPGA Manager",
 				     &zynqmp_fpga_ops, priv);
 	return PTR_ERR_OR_ZERO(mgr);