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

[Nava kishore Manne <navam@xilinx.com>]

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 <navam@xilinx.com>
> Cc: Michal Simek <michals@xilinx.com>; mdf@kernel.org;
> hao.wu@intel.com; trix@redhat.com; gregkh@linuxfoundation.org; Ronak
> Jain <ronakj@xilinx.com>; Abhyuday Godhasara <agodhasa@xilinx.com>;
> Rajan Vaja <rajanv@xlnx.xilinx.com>; Sai Krishna Potthuri
> <lakshmis@xilinx.com>; Piyush Mehta <piyushm@xilinx.com>; Harsha
> Harsha <harshah@xilinx.com>; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; linux-fpga@vger.kernel.org; git <git@xilinx.com>
> 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:
> > 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.
> 
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