[PATCH v2 0/3]Adds status interface for zynqmp-fpga

[PATCH v2 0/3]Adds status interface for zynqmp-fpga

[Nava kishore Manne]

Adds status interface for zynqmp-fpga, It's a read only interface
which allows the user to get the PL status.
 -Device Initialization error.
 -Device internal signal error.
 -All I/Os are placed in High-Z state.
 -Device start-up sequence error.
 -Firmware error.

For more details refer the ug570.
https://docs.xilinx.com/v/u/en-US/ug570-ultrascale-configuration

This series rebased on 5.19v
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git 

Nava kishore Manne (3):
  fpga: manager: change status api prototype, don't use older
  firmware: xilinx: Add pm api function for PL readback
  fpga: zynqmp-fpga: Adds status interface

 drivers/firmware/xilinx/zynqmp.c     | 33 +++++++++++++++++
 drivers/fpga/dfl-fme-mgr.c           | 20 +++++------
 drivers/fpga/fpga-mgr.c              | 24 +++----------
 drivers/fpga/zynqmp-fpga.c           | 53 ++++++++++++++++++++++++++++
 include/linux/firmware/xlnx-zynqmp.h | 14 ++++++++
 include/linux/fpga/fpga-mgr.h        |  2 +-
 6 files changed, 116 insertions(+), 30 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] fpga: manager: change status api prototype, don't use older

[Nava kishore Manne]

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. Also, it's
hard to understand the mixture of different error sets.

To address these issues updated the status interface to handle the
vendor-specific messages in a generic way. With the updated status
interface the vendor-specific driver files can independently handle
the error messages.

Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
---
Changes for v2:
              - New patch.

 drivers/fpga/dfl-fme-mgr.c    | 20 ++++++++++----------
 drivers/fpga/fpga-mgr.c       | 24 +++++-------------------
 include/linux/fpga/fpga-mgr.h |  2 +-
 3 files changed, 16 insertions(+), 30 deletions(-)

diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
index af0785783b52..5a8e6a41c85c 100644
--- a/drivers/fpga/dfl-fme-mgr.c
+++ b/drivers/fpga/dfl-fme-mgr.c
@@ -72,22 +72,22 @@ struct fme_mgr_priv {
 	u64 pr_error;
 };
 
-static u64 pr_error_to_mgr_status(u64 err)
+static ssize_t pr_error_to_mgr_status(u64 err, char *buf)
 {
-	u64 status = 0;
+	ssize_t len = 0;
 
 	if (err & FME_PR_ERR_OPERATION_ERR)
-		status |= FPGA_MGR_STATUS_OPERATION_ERR;
+		len += sprintf(buf + len, "reconfig operation error\n");
 	if (err & FME_PR_ERR_CRC_ERR)
-		status |= FPGA_MGR_STATUS_CRC_ERR;
+		len += sprintf(buf + len, "reconfig CRC error\n");
 	if (err & FME_PR_ERR_INCOMPATIBLE_BS)
-		status |= FPGA_MGR_STATUS_INCOMPATIBLE_IMAGE_ERR;
+		len += sprintf(buf + len, "reconfig incompatible image\n");
 	if (err & FME_PR_ERR_PROTOCOL_ERR)
-		status |= FPGA_MGR_STATUS_IP_PROTOCOL_ERR;
+		len += sprintf(buf + len, "reconfig IP protocol error\n");
 	if (err & FME_PR_ERR_FIFO_OVERFLOW)
-		status |= FPGA_MGR_STATUS_FIFO_OVERFLOW_ERR;
+		len += sprintf(buf + len, "reconfig fifo overflow error\n");
 
-	return status;
+	return len;
 }
 
 static u64 fme_mgr_pr_error_handle(void __iomem *fme_pr)
@@ -252,11 +252,11 @@ static int fme_mgr_write_complete(struct fpga_manager *mgr,
 	return 0;
 }
 
-static u64 fme_mgr_status(struct fpga_manager *mgr)
+static ssize_t fme_mgr_status(struct fpga_manager *mgr, char *buf)
 {
 	struct fme_mgr_priv *priv = mgr->priv;
 
-	return pr_error_to_mgr_status(priv->pr_error);
+	return pr_error_to_mgr_status(priv->pr_error, buf);
 }
 
 static const struct fpga_manager_ops fme_mgr_ops = {
diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 08dc85fcd511..ae8de13a482e 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -38,10 +38,11 @@ static inline enum fpga_mgr_states fpga_mgr_state(struct fpga_manager *mgr)
 	return FPGA_MGR_STATE_UNKNOWN;
 }
 
-static inline u64 fpga_mgr_status(struct fpga_manager *mgr)
+static inline ssize_t fpga_mgr_status(struct fpga_manager *mgr, char *buf)
 {
 	if (mgr->mops->status)
-		return mgr->mops->status(mgr);
+		return mgr->mops->status(mgr, buf);
+
 	return 0;
 }
 
@@ -460,23 +461,8 @@ static ssize_t status_show(struct device *dev,
 			   struct device_attribute *attr, char *buf)
 {
 	struct fpga_manager *mgr = to_fpga_manager(dev);
-	u64 status;
-	int len = 0;
-
-	status = fpga_mgr_status(mgr);
-
-	if (status & FPGA_MGR_STATUS_OPERATION_ERR)
-		len += sprintf(buf + len, "reconfig operation error\n");
-	if (status & FPGA_MGR_STATUS_CRC_ERR)
-		len += sprintf(buf + len, "reconfig CRC error\n");
-	if (status & FPGA_MGR_STATUS_INCOMPATIBLE_IMAGE_ERR)
-		len += sprintf(buf + len, "reconfig incompatible image\n");
-	if (status & FPGA_MGR_STATUS_IP_PROTOCOL_ERR)
-		len += sprintf(buf + len, "reconfig IP protocol error\n");
-	if (status & FPGA_MGR_STATUS_FIFO_OVERFLOW_ERR)
-		len += sprintf(buf + len, "reconfig fifo overflow error\n");
-
-	return len;
+
+	return fpga_mgr_status(mgr, buf);
 }
 
 static DEVICE_ATTR_RO(name);
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 0f9468771bb9..42c24426fb7f 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -154,7 +154,7 @@ struct fpga_manager_info {
 struct fpga_manager_ops {
 	size_t initial_header_size;
 	enum fpga_mgr_states (*state)(struct fpga_manager *mgr);
-	u64 (*status)(struct fpga_manager *mgr);
+	ssize_t (*status)(struct fpga_manager *mgr, char *buf);
 	int (*write_init)(struct fpga_manager *mgr,
 			  struct fpga_image_info *info,
 			  const char *buf, size_t count);
-- 
2.25.1

Re: [PATCH v2 1/3] fpga: manager: change status api prototype, don't use older

[Xu Yilun]

On Tue, Jun 21, 2022 at 02:58:31PM +0530, Nava kishore Manne wrote:
> 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. Also, it's
> hard to understand the mixture of different error sets.
> 
> To address these issues updated the status interface to handle the
> vendor-specific messages in a generic way. With the updated status
> interface the vendor-specific driver files can independently handle
> the error messages.

I think we don't have to provide the vendor specific HW errors in a
generic way, maybe the vendor specific drivers could handle them by its
own device attributes.

Since the output value set of the interface is specific to each driver,
users should still interpret them in specific manners. So doesn't see
much value for a class interface.

Thanks,
Yilun

> 
> Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
> ---
> Changes for v2:
>               - New patch.
> 
>  drivers/fpga/dfl-fme-mgr.c    | 20 ++++++++++----------
>  drivers/fpga/fpga-mgr.c       | 24 +++++-------------------
>  include/linux/fpga/fpga-mgr.h |  2 +-
>  3 files changed, 16 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
> index af0785783b52..5a8e6a41c85c 100644
> --- a/drivers/fpga/dfl-fme-mgr.c
> +++ b/drivers/fpga/dfl-fme-mgr.c
> @@ -72,22 +72,22 @@ struct fme_mgr_priv {
>  	u64 pr_error;
>  };
>  
> -static u64 pr_error_to_mgr_status(u64 err)
> +static ssize_t pr_error_to_mgr_status(u64 err, char *buf)
>  {
> -	u64 status = 0;
> +	ssize_t len = 0;
>  
>  	if (err & FME_PR_ERR_OPERATION_ERR)
> -		status |= FPGA_MGR_STATUS_OPERATION_ERR;
> +		len += sprintf(buf + len, "reconfig operation error\n");
>  	if (err & FME_PR_ERR_CRC_ERR)
> -		status |= FPGA_MGR_STATUS_CRC_ERR;
> +		len += sprintf(buf + len, "reconfig CRC error\n");
>  	if (err & FME_PR_ERR_INCOMPATIBLE_BS)
> -		status |= FPGA_MGR_STATUS_INCOMPATIBLE_IMAGE_ERR;
> +		len += sprintf(buf + len, "reconfig incompatible image\n");
>  	if (err & FME_PR_ERR_PROTOCOL_ERR)
> -		status |= FPGA_MGR_STATUS_IP_PROTOCOL_ERR;
> +		len += sprintf(buf + len, "reconfig IP protocol error\n");
>  	if (err & FME_PR_ERR_FIFO_OVERFLOW)
> -		status |= FPGA_MGR_STATUS_FIFO_OVERFLOW_ERR;
> +		len += sprintf(buf + len, "reconfig fifo overflow error\n");
>  
> -	return status;
> +	return len;
>  }
>  
>  static u64 fme_mgr_pr_error_handle(void __iomem *fme_pr)
> @@ -252,11 +252,11 @@ static int fme_mgr_write_complete(struct fpga_manager *mgr,
>  	return 0;
>  }
>  
> -static u64 fme_mgr_status(struct fpga_manager *mgr)
> +static ssize_t fme_mgr_status(struct fpga_manager *mgr, char *buf)
>  {
>  	struct fme_mgr_priv *priv = mgr->priv;
>  
> -	return pr_error_to_mgr_status(priv->pr_error);
> +	return pr_error_to_mgr_status(priv->pr_error, buf);
>  }
>  
>  static const struct fpga_manager_ops fme_mgr_ops = {
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index 08dc85fcd511..ae8de13a482e 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -38,10 +38,11 @@ static inline enum fpga_mgr_states fpga_mgr_state(struct fpga_manager *mgr)
>  	return FPGA_MGR_STATE_UNKNOWN;
>  }
>  
> -static inline u64 fpga_mgr_status(struct fpga_manager *mgr)
> +static inline ssize_t fpga_mgr_status(struct fpga_manager *mgr, char *buf)
>  {
>  	if (mgr->mops->status)
> -		return mgr->mops->status(mgr);
> +		return mgr->mops->status(mgr, buf);
> +
>  	return 0;
>  }
>  
> @@ -460,23 +461,8 @@ static ssize_t status_show(struct device *dev,
>  			   struct device_attribute *attr, char *buf)
>  {
>  	struct fpga_manager *mgr = to_fpga_manager(dev);
> -	u64 status;
> -	int len = 0;
> -
> -	status = fpga_mgr_status(mgr);
> -
> -	if (status & FPGA_MGR_STATUS_OPERATION_ERR)
> -		len += sprintf(buf + len, "reconfig operation error\n");
> -	if (status & FPGA_MGR_STATUS_CRC_ERR)
> -		len += sprintf(buf + len, "reconfig CRC error\n");
> -	if (status & FPGA_MGR_STATUS_INCOMPATIBLE_IMAGE_ERR)
> -		len += sprintf(buf + len, "reconfig incompatible image\n");
> -	if (status & FPGA_MGR_STATUS_IP_PROTOCOL_ERR)
> -		len += sprintf(buf + len, "reconfig IP protocol error\n");
> -	if (status & FPGA_MGR_STATUS_FIFO_OVERFLOW_ERR)
> -		len += sprintf(buf + len, "reconfig fifo overflow error\n");
> -
> -	return len;
> +
> +	return fpga_mgr_status(mgr, buf);
>  }
>  
>  static DEVICE_ATTR_RO(name);
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index 0f9468771bb9..42c24426fb7f 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -154,7 +154,7 @@ struct fpga_manager_info {
>  struct fpga_manager_ops {
>  	size_t initial_header_size;
>  	enum fpga_mgr_states (*state)(struct fpga_manager *mgr);
> -	u64 (*status)(struct fpga_manager *mgr);
> +	ssize_t (*status)(struct fpga_manager *mgr, char *buf);
>  	int (*write_init)(struct fpga_manager *mgr,
>  			  struct fpga_image_info *info,
>  			  const char *buf, size_t count);
> -- 
> 2.25.1

RE: [PATCH v2 1/3] fpga: manager: change status api prototype, don't use older

[Manne, Nava kishore]

Hi Yilun,

	Please find my response inline.

> -----Original Message-----
> From: Xu Yilun <yilun.xu@intel.com>
> Sent: Tuesday, June 28, 2022 2:02 PM
> To: Nava kishore Manne <nava.manne@xilinx.com>
> Cc: michal.simek@xilinx.com; hao.wu@intel.com; trix@redhat.com;
> mdf@kernel.org; gregkh@linuxfoundation.org; ronak.jain@xilinx.com;
> rajan.vaja@xilinx.com; abhyuday.godhasara@xilinx.com;
> piyush.mehta@xilinx.com; harsha.harsha@xilinx.com;
> lakshmi.sai.krishna.potthuri@xilinx.com; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> fpga@vger.kernel.org; git@xilinx.com
> Subject: Re: [PATCH v2 1/3] fpga: manager: change status api prototype,
> don't use older
> 
> CAUTION: This message has originated from an External Source. Please use
> proper judgment and caution when opening attachments, clicking links, or
> responding to this email.
> 
> 
> On Tue, Jun 21, 2022 at 02:58:31PM +0530, Nava kishore Manne wrote:
> > 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. Also, it's
> > hard to understand the mixture of different error sets.
> >
> > To address these issues updated the status interface to handle the
> > vendor-specific messages in a generic way. With the updated status
> > interface the vendor-specific driver files can independently handle
> > the error messages.
> 
> I think we don't have to provide the vendor specific HW errors in a generic
> way, maybe the vendor specific drivers could handle them by its own device
> attributes.
> 
> Since the output value set of the interface is specific to each driver, users
> should still interpret them in specific manners. So doesn't see much value for
> a class interface.
>

Agree,  vendor specific drivers could handle them by its own device attributes.
If it is the case, can we remove the existing status interface relevant changes from the core?

Regards,
Navakishore

Re: [PATCH v2 1/3] fpga: manager: change status api prototype, don't use older

[Xu Yilun]

On 2022-08-17 at 11:16:14 +0000, Manne, Nava kishore wrote:
> Hi Yilun,
> 
> 	Please find my response inline.
> 
> > -----Original Message-----
> > From: Xu Yilun <yilun.xu@intel.com>
> > Sent: Tuesday, June 28, 2022 2:02 PM
> > To: Nava kishore Manne <nava.manne@xilinx.com>
> > Cc: michal.simek@xilinx.com; hao.wu@intel.com; trix@redhat.com;
> > mdf@kernel.org; gregkh@linuxfoundation.org; ronak.jain@xilinx.com;
> > rajan.vaja@xilinx.com; abhyuday.godhasara@xilinx.com;
> > piyush.mehta@xilinx.com; harsha.harsha@xilinx.com;
> > lakshmi.sai.krishna.potthuri@xilinx.com; linux-arm-
> > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> > fpga@vger.kernel.org; git@xilinx.com
> > Subject: Re: [PATCH v2 1/3] fpga: manager: change status api prototype,
> > don't use older
> > 
> > On Tue, Jun 21, 2022 at 02:58:31PM +0530, Nava kishore Manne wrote:
> > > 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. Also, it's
> > > hard to understand the mixture of different error sets.
> > >
> > > To address these issues updated the status interface to handle the
> > > vendor-specific messages in a generic way. With the updated status
> > > interface the vendor-specific driver files can independently handle
> > > the error messages.
> > 
> > I think we don't have to provide the vendor specific HW errors in a generic
> > way, maybe the vendor specific drivers could handle them by its own device
> > attributes.
> > 
> > Since the output value set of the interface is specific to each driver, users
> > should still interpret them in specific manners. So doesn't see much value for
> > a class interface.
> >
> 
> Agree,  vendor specific drivers could handle them by its own device attributes.
> If it is the case, can we remove the existing status interface relevant changes from the core?

We don't have to. Some *user* interfaces may become hard to use as time
goes, but we still try to make them stable as there are exsiting users
working on them once they are released.

That also means we should be more careful when introducing new user
interfaces.

Thanks,
Yilun

> 
> Regards,
> Navakishore

[PATCH v2 2/3] firmware: xilinx: Add pm api function for PL readback

[Nava kishore Manne]

Adds PM API for performing PL configuration readback.
It provides an interface to the pmufw to readback the
FPGA configuration registers as well as configuration
data.

For more detailed info related to the configuration
registers and configuration data refer ug570.

Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
---
Changes for v2:
              - None.

 drivers/firmware/xilinx/zynqmp.c     | 33 ++++++++++++++++++++++++++++
 include/linux/firmware/xlnx-zynqmp.h | 14 ++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
index 7977a494a651..40b99299b662 100644
--- a/drivers/firmware/xilinx/zynqmp.c
+++ b/drivers/firmware/xilinx/zynqmp.c
@@ -927,6 +927,39 @@ int zynqmp_pm_fpga_get_status(u32 *value)
 }
 EXPORT_SYMBOL_GPL(zynqmp_pm_fpga_get_status);
 
+/**
+ * zynqmp_pm_fpga_read - Perform the fpga configuration readback
+ * @reg_numframes: Configuration register offset (or) Number of frames to read
+ * @phys_address: Physical Address of the buffer
+ * @readback_type: Type of fpga readback operation
+ *                 0 - FPGA configuration register readback
+ *                 1 - FPGA configuration data readback
+ * @value: Value to read
+ *
+ * This function provides access to xilfpga library to perform
+ * fpga configuration readback.
+ *
+ * Return:	Returns status, either success or error+reason
+ */
+int zynqmp_pm_fpga_read(const u32 reg_numframes, const phys_addr_t phys_address,
+			bool readback_type, u32 *value)
+{
+	u32 ret_payload[PAYLOAD_ARG_CNT];
+	int ret;
+
+	if (!value)
+		return -EINVAL;
+
+	ret = zynqmp_pm_invoke_fn(PM_FPGA_READ, reg_numframes,
+				  lower_32_bits(phys_address),
+				  upper_32_bits(phys_address), readback_type,
+				  ret_payload);
+	*value = ret_payload[1];
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(zynqmp_pm_fpga_read);
+
 /**
  * zynqmp_pm_pinctrl_request - Request Pin from firmware
  * @pin: Pin number to request
diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h
index 1ec73d5352c3..7dc4981345dc 100644
--- a/include/linux/firmware/xlnx-zynqmp.h
+++ b/include/linux/firmware/xlnx-zynqmp.h
@@ -61,6 +61,10 @@
 #define PM_LOAD_PDI	0x701
 #define PDI_SRC_DDR	0xF
 
+/* FPGA readback type */
+#define PM_FPGA_READ_CONFIG_REG		0x0U
+#define PM_FPGA_READ_CONFIG_DATA	0x1U
+
 /*
  * Firmware FPGA Manager flags
  * XILINX_ZYNQMP_PM_FPGA_FULL:	FPGA full reconfiguration
@@ -116,6 +120,7 @@ enum pm_api_id {
 	PM_CLOCK_GETRATE = 42,
 	PM_CLOCK_SETPARENT = 43,
 	PM_CLOCK_GETPARENT = 44,
+	PM_FPGA_READ = 46,
 	PM_SECURE_AES = 47,
 	PM_FEATURE_CHECK = 63,
 };
@@ -468,6 +473,8 @@ int zynqmp_pm_feature(const u32 api_id);
 int zynqmp_pm_is_function_supported(const u32 api_id, const u32 id);
 int zynqmp_pm_set_feature_config(enum pm_feature_config_id id, u32 value);
 int zynqmp_pm_get_feature_config(enum pm_feature_config_id id, u32 *payload);
+int zynqmp_pm_fpga_read(const u32 reg_numframes, const phys_addr_t phys_address,
+			bool readback_type, u32 *value);
 #else
 static inline int zynqmp_pm_get_api_version(u32 *version)
 {
@@ -733,6 +740,13 @@ static inline int zynqmp_pm_get_feature_config(enum pm_feature_config_id id,
 {
 	return -ENODEV;
 }
+
+static int zynqmp_pm_fpga_read(const u32 reg_numframes,
+			       const phys_addr_t phys_address,
+			       bool readback_type, u32 *value);
+{
+	return -ENODEV;
+}
 #endif
 
 #endif /* __FIRMWARE_ZYNQMP_H__ */
-- 
2.25.1

Re: [PATCH v2 2/3] firmware: xilinx: Add pm api function for PL readback

[kernel test robot]

Hi Nava,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.19-rc3 next-20220621]
[cannot apply to xilinx-xlnx/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Nava-kishore-Manne/fpga-manager-change-status-api-prototype-don-t-use-older/20220621-183524
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 78ca55889a549a9a194c6ec666836329b774ab6d
config: riscv-randconfig-r042-20220619 (https://download.01.org/0day-ci/archive/20220621/202206212254.d7Dba7Nm-lkp@intel.com/config)
compiler: riscv32-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/6b66792a9e4acd718d58c0e5a33ca2426837be87
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Nava-kishore-Manne/fpga-manager-change-status-api-prototype-don-t-use-older/20220621-183524
        git checkout 6b66792a9e4acd718d58c0e5a33ca2426837be87
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/crypto/xilinx/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/crypto/xilinx/zynqmp-aes-gcm.c:18:
>> include/linux/firmware/xlnx-zynqmp.h:747:1: error: expected identifier or '(' before '{' token
     747 | {
         | ^
   include/linux/firmware/xlnx-zynqmp.h:744:12: warning: 'zynqmp_pm_fpga_read' declared 'static' but never defined [-Wunused-function]
     744 | static int zynqmp_pm_fpga_read(const u32 reg_numframes,
         |            ^~~~~~~~~~~~~~~~~~~


vim +747 include/linux/firmware/xlnx-zynqmp.h

   743	
   744	static int zynqmp_pm_fpga_read(const u32 reg_numframes,
   745				       const phys_addr_t phys_address,
   746				       bool readback_type, u32 *value);
 > 747	{
   748		return -ENODEV;
   749	}
   750	#endif
   751	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v2 2/3] firmware: xilinx: Add pm api function for PL readback

[kernel test robot]

Hi Nava,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.19-rc3 next-20220621]
[cannot apply to xilinx-xlnx/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Nava-kishore-Manne/fpga-manager-change-status-api-prototype-don-t-use-older/20220621-183524
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 78ca55889a549a9a194c6ec666836329b774ab6d
config: hexagon-randconfig-r003-20220620 (https://download.01.org/0day-ci/archive/20220621/202206212247.2Ky8WLW2-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project af6d2a0b6825e71965f3e2701a63c239fa0ad70f)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/6b66792a9e4acd718d58c0e5a33ca2426837be87
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Nava-kishore-Manne/fpga-manager-change-status-api-prototype-don-t-use-older/20220621-183524
        git checkout 6b66792a9e4acd718d58c0e5a33ca2426837be87
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/fpga/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/fpga/zynqmp-fpga.c:13:
>> include/linux/firmware/xlnx-zynqmp.h:747:1: error: expected identifier or '('
   {
   ^
   1 error generated.


vim +747 include/linux/firmware/xlnx-zynqmp.h

   743	
   744	static int zynqmp_pm_fpga_read(const u32 reg_numframes,
   745				       const phys_addr_t phys_address,
   746				       bool readback_type, u32 *value);
 > 747	{
   748		return -ENODEV;
   749	}
   750	#endif
   751	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v2 2/3] firmware: xilinx: Add pm api function for PL readback

[Peter Korsgaard]

>>>>> "Nava" == Nava kishore Manne <nava.manne@xilinx.com> writes:

 > Adds PM API for performing PL configuration readback.
 > It provides an interface to the pmufw to readback the
 > FPGA configuration registers as well as configuration
 > data.

 > For more detailed info related to the configuration
 > registers and configuration data refer ug570.

 > Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
 > ---
 > Changes for v2:
 >               - None.

 >  drivers/firmware/xilinx/zynqmp.c     | 33 ++++++++++++++++++++++++++++
 >  include/linux/firmware/xlnx-zynqmp.h | 14 ++++++++++++
 >  2 files changed, 47 insertions(+)

 > diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
 > index 7977a494a651..40b99299b662 100644
 > --- a/drivers/firmware/xilinx/zynqmp.c
 > +++ b/drivers/firmware/xilinx/zynqmp.c
 > @@ -927,6 +927,39 @@ int zynqmp_pm_fpga_get_status(u32 *value)
 >  }
 >  EXPORT_SYMBOL_GPL(zynqmp_pm_fpga_get_status);
 
 > +/**
 > + * zynqmp_pm_fpga_read - Perform the fpga configuration readback
 > + * @reg_numframes: Configuration register offset (or) Number of frames to read

An offset OR a length? That sounds odd.


 > + * @phys_address: Physical Address of the buffer
 > + * @readback_type: Type of fpga readback operation
 > + *                 0 - FPGA configuration register readback
 > + *                 1 - FPGA configuration data readback

readback_type is a boolean, so how about calling it `bool data` or
something like that?


 > + * @value: Value to read

what is the relation between phys_address and this value output
argument?

 > + *
 > + * This function provides access to xilfpga library to perform
 > + * fpga configuration readback.
 > + *
 > + * Return:	Returns status, either success or error+reason
 > + */
 > +int zynqmp_pm_fpga_read(const u32 reg_numframes, const phys_addr_t phys_address,
 > +			bool readback_type, u32 *value)
 > +{
 > +	u32 ret_payload[PAYLOAD_ARG_CNT];
 > +	int ret;
 > +
 > +	if (!value)
 > +		return -EINVAL;
 > +
 > +	ret = zynqmp_pm_invoke_fn(PM_FPGA_READ, reg_numframes,
 > +				  lower_32_bits(phys_address),
 > +				  upper_32_bits(phys_address), readback_type,

You are adding PM_FPGA_READ_CONFIG_ defines, so how about using them,
E.G.

data ? PM_FPGA_READ_CONFIG_DATA : PM_FPGA_READ_CONFIG_REG

-- 
Bye, Peter Korsgaard

[PATCH v2 3/3] fpga: zynqmp-fpga: Adds status interface

[Nava kishore Manne]

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 <nava.manne@xilinx.com>
---
Changes for v2:
              - Updated status messages handling logic as suggested by Xu Yilun.

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

diff --git a/drivers/fpga/zynqmp-fpga.c b/drivers/fpga/zynqmp-fpga.c
index c60f20949c47..e194bba91d3f 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,48 @@ static enum fpga_mgr_states zynqmp_fpga_ops_state(struct fpga_manager *mgr)
 	return FPGA_MGR_STATE_UNKNOWN;
 }
 
+static ssize_t zynqmp_fpga_ops_status(struct fpga_manager *mgr, char *buf)
+{
+	unsigned int *kbuf, reg_val;
+	dma_addr_t dma_addr;
+	ssize_t len = 0;
+	int ret;
+
+	kbuf = dma_alloc_coherent(mgr->dev.parent, READ_DMA_SIZE,
+				  &dma_addr, GFP_KERNEL);
+	if (!kbuf)
+		return -ENOMEM;
+
+	ret = zynqmp_pm_fpga_read(IXR_FPGA_CONFIG_STAT_OFFSET, dma_addr,
+				  PM_FPGA_READ_CONFIG_REG, &reg_val);
+	if (ret) {
+		len += sprintf(buf + len, "firmware error\n");
+		goto free_dmabuf;
+	}
+
+	if (reg_val & IXR_FPGA_ERR_CRC_ERR)
+		len += sprintf(buf + len, "reconfig CRC error\n");
+	if (reg_val & IXR_FPGA_ERR_SECURITY_ERR)
+		len += sprintf(buf + len, "reconfig security error\n");
+	if (!(reg_val & IXR_FPGA_INIT_B_INTERNAL))
+		len += sprintf(buf + len, "Device Initialization error\n");
+	if (!(reg_val & IXR_FPGA_DONE_INTERNAL_SIGNAL))
+		len += sprintf(buf + len, "Device internal signal error\n");
+	if (!(reg_val & IXR_FPGA_GST_CFG_B))
+		len += sprintf(buf + len,
+			       "All I/Os are placed in High-Z state\n");
+	if (!(reg_val & IXR_FPGA_END_OF_STARTUP))
+		len += sprintf(buf + len, "Device sequence error\n");
+
+free_dmabuf:
+	dma_free_coherent(mgr->dev.parent, READ_DMA_SIZE, buf, dma_addr);
+
+	return len;
+}
+
 static const struct fpga_manager_ops zynqmp_fpga_ops = {
 	.state = zynqmp_fpga_ops_state,
+	.status = zynqmp_fpga_ops_status,
 	.write_init = zynqmp_fpga_ops_write_init,
 	.write = zynqmp_fpga_ops_write,
 };
-- 
2.25.1

Re: [PATCH v2 3/3] fpga: zynqmp-fpga: Adds status interface

[Peter Korsgaard]

>>>>> "Nava" == Nava kishore Manne <nava.manne@xilinx.com> writes:

 > 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 <nava.manne@xilinx.com>
 > ---
 > Changes for v2:
 >               - Updated status messages handling logic as suggested by Xu Yilun.

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

 > diff --git a/drivers/fpga/zynqmp-fpga.c b/drivers/fpga/zynqmp-fpga.c
 > index c60f20949c47..e194bba91d3f 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,48 @@ static enum fpga_mgr_states zynqmp_fpga_ops_state(struct fpga_manager *mgr)
 >  	return FPGA_MGR_STATE_UNKNOWN;
 >  }
 
 > +static ssize_t zynqmp_fpga_ops_status(struct fpga_manager *mgr, char *buf)
 > +{
 > +	unsigned int *kbuf, reg_val;
 > +	dma_addr_t dma_addr;
 > +	ssize_t len = 0;
 > +	int ret;
 > +
 > +	kbuf = dma_alloc_coherent(mgr->dev.parent, READ_DMA_SIZE,
 > +				  &dma_addr, GFP_KERNEL);
 > +	if (!kbuf)
 > +		return -ENOMEM;

What is kbuf used for? You don't seem to ever access it?

-- 
Bye, Peter Korsgaard

Re: [PATCH v2 3/3] fpga: zynqmp-fpga: Adds status interface

[Xu Yilun]

On Tue, Jun 21, 2022 at 02:58:33PM +0530, Nava kishore Manne wrote:
> 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 <nava.manne@xilinx.com>
> ---
> Changes for v2:
>               - Updated status messages handling logic as suggested by Xu Yilun.
> 
>  drivers/fpga/zynqmp-fpga.c | 53 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/drivers/fpga/zynqmp-fpga.c b/drivers/fpga/zynqmp-fpga.c
> index c60f20949c47..e194bba91d3f 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,48 @@ static enum fpga_mgr_states zynqmp_fpga_ops_state(struct fpga_manager *mgr)
>  	return FPGA_MGR_STATE_UNKNOWN;
>  }
>  
> +static ssize_t zynqmp_fpga_ops_status(struct fpga_manager *mgr, char *buf)
> +{
> +	unsigned int *kbuf, reg_val;
> +	dma_addr_t dma_addr;
> +	ssize_t len = 0;
> +	int ret;
> +
> +	kbuf = dma_alloc_coherent(mgr->dev.parent, READ_DMA_SIZE,
> +				  &dma_addr, GFP_KERNEL);
> +	if (!kbuf)
> +		return -ENOMEM;
> +
> +	ret = zynqmp_pm_fpga_read(IXR_FPGA_CONFIG_STAT_OFFSET, dma_addr,
> +				  PM_FPGA_READ_CONFIG_REG, &reg_val);
> +	if (ret) {
> +		len += sprintf(buf + len, "firmware error\n");
> +		goto free_dmabuf;
> +	}
> +
> +	if (reg_val & IXR_FPGA_ERR_CRC_ERR)
> +		len += sprintf(buf + len, "reconfig CRC error\n");
> +	if (reg_val & IXR_FPGA_ERR_SECURITY_ERR)
> +		len += sprintf(buf + len, "reconfig security error\n");
> +	if (!(reg_val & IXR_FPGA_INIT_B_INTERNAL))
> +		len += sprintf(buf + len, "Device Initialization error\n");
> +	if (!(reg_val & IXR_FPGA_DONE_INTERNAL_SIGNAL))
> +		len += sprintf(buf + len, "Device internal signal error\n");
> +	if (!(reg_val & IXR_FPGA_GST_CFG_B))
> +		len += sprintf(buf + len,
> +			       "All I/Os are placed in High-Z state\n");
> +	if (!(reg_val & IXR_FPGA_END_OF_STARTUP))
> +		len += sprintf(buf + len, "Device sequence error\n");

Expressing multiple lines of data is discouraged, one value or an array
of values is OK. For more details, see
Documentation/filesystems/sysfs.rst

Thanks,
Yilun

> +
> +free_dmabuf:
> +	dma_free_coherent(mgr->dev.parent, READ_DMA_SIZE, buf, dma_addr);
> +
> +	return len;
> +}
> +
>  static const struct fpga_manager_ops zynqmp_fpga_ops = {
>  	.state = zynqmp_fpga_ops_state,
> +	.status = zynqmp_fpga_ops_status,
>  	.write_init = zynqmp_fpga_ops_write_init,
>  	.write = zynqmp_fpga_ops_write,
>  };
> -- 
> 2.25.1

RE: [PATCH v2 3/3] fpga: zynqmp-fpga: Adds status interface

[Manne, Nava kishore]

Hi Yilun,

	Please find my response inline.

> -----Original Message-----
> From: Xu Yilun <yilun.xu@intel.com>
> Sent: Tuesday, June 28, 2022 2:10 PM
> To: Nava kishore Manne <nava.manne@xilinx.com>
> Cc: michal.simek@xilinx.com; hao.wu@intel.com; trix@redhat.com;
> mdf@kernel.org; gregkh@linuxfoundation.org; ronak.jain@xilinx.com;
> rajan.vaja@xilinx.com; abhyuday.godhasara@xilinx.com;
> piyush.mehta@xilinx.com; harsha.harsha@xilinx.com;
> lakshmi.sai.krishna.potthuri@xilinx.com; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> fpga@vger.kernel.org; git@xilinx.com
> Subject: Re: [PATCH v2 3/3] fpga: zynqmp-fpga: Adds status interface
> 
> CAUTION: This message has originated from an External Source. Please use
> proper judgment and caution when opening attachments, clicking links, or
> responding to this email.
> 
> 
> On Tue, Jun 21, 2022 at 02:58:33PM +0530, Nava kishore Manne wrote:
> > 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 <nava.manne@xilinx.com>
> > ---
> > Changes for v2:
> >               - Updated status messages handling logic as suggested by Xu Yilun.
> >
> >  drivers/fpga/zynqmp-fpga.c | 53
> > ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 53 insertions(+)
> >
> > diff --git a/drivers/fpga/zynqmp-fpga.c b/drivers/fpga/zynqmp-fpga.c
> > index c60f20949c47..e194bba91d3f 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,48
> > @@ static enum fpga_mgr_states zynqmp_fpga_ops_state(struct
> fpga_manager *mgr)
> >       return FPGA_MGR_STATE_UNKNOWN;
> >  }
> >
> > +static ssize_t zynqmp_fpga_ops_status(struct fpga_manager *mgr, char
> > +*buf) {
> > +     unsigned int *kbuf, reg_val;
> > +     dma_addr_t dma_addr;
> > +     ssize_t len = 0;
> > +     int ret;
> > +
> > +     kbuf = dma_alloc_coherent(mgr->dev.parent, READ_DMA_SIZE,
> > +                               &dma_addr, GFP_KERNEL);
> > +     if (!kbuf)
> > +             return -ENOMEM;
> > +
> > +     ret = zynqmp_pm_fpga_read(IXR_FPGA_CONFIG_STAT_OFFSET,
> dma_addr,
> > +                               PM_FPGA_READ_CONFIG_REG, &reg_val);
> > +     if (ret) {
> > +             len += sprintf(buf + len, "firmware error\n");
> > +             goto free_dmabuf;
> > +     }
> > +
> > +     if (reg_val & IXR_FPGA_ERR_CRC_ERR)
> > +             len += sprintf(buf + len, "reconfig CRC error\n");
> > +     if (reg_val & IXR_FPGA_ERR_SECURITY_ERR)
> > +             len += sprintf(buf + len, "reconfig security error\n");
> > +     if (!(reg_val & IXR_FPGA_INIT_B_INTERNAL))
> > +             len += sprintf(buf + len, "Device Initialization error\n");
> > +     if (!(reg_val & IXR_FPGA_DONE_INTERNAL_SIGNAL))
> > +             len += sprintf(buf + len, "Device internal signal error\n");
> > +     if (!(reg_val & IXR_FPGA_GST_CFG_B))
> > +             len += sprintf(buf + len,
> > +                            "All I/Os are placed in High-Z state\n");
> > +     if (!(reg_val & IXR_FPGA_END_OF_STARTUP))
> > +             len += sprintf(buf + len, "Device sequence error\n");
> 
> Expressing multiple lines of data is discouraged, one value or an array of
> values is OK. For more details, see Documentation/filesystems/sysfs.rst
> 
Will fix.

Regards,
Navakishore.