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

[Xu Yilun <yilun.xu@intel.com>]

On Tue, May 24, 2022 at 03:17:45PM +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>
> ---
>  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)
>  	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,
> +				  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.

Thanks,
Yilun

> +
> +free_dmabuf:
> +	dma_free_coherent(mgr->dev.parent, READ_DMA_SIZE, buf, dma_addr);
> +
> +	return status;
> +}
> +
>  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