[PATCH] fpga: zynqmp: Make word align the configuration data

[PATCH] fpga: zynqmp: Make word align the configuration data

[Nava kishore Manne]

To avoid unwanted copies at firmware(PMUFW) this patch provides a fix
to align programmable logic(PL) configuration data if the data is not
word-aligned. To align the configuration data this patch adds a few
padding bytes and these additional padding bytes will not create any
functional impact on the PL configuration.

Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com>
---
 drivers/fpga/zynqmp-fpga.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/fpga/zynqmp-fpga.c b/drivers/fpga/zynqmp-fpga.c
index c60f20949c47..70a12dc6e15c 100644
--- a/drivers/fpga/zynqmp-fpga.c
+++ b/drivers/fpga/zynqmp-fpga.c
@@ -15,6 +15,9 @@
 /* Constant Definitions */
 #define IXR_FPGA_DONE_MASK	BIT(3)
 
+#define DUMMY_PAD_BYTE		0xFF
+#define FPGA_WORD_SIZE		4
+
 /**
  * struct zynqmp_fpga_priv - Private data structure
  * @dev:	Device data structure
@@ -41,18 +44,26 @@ static int zynqmp_fpga_ops_write(struct fpga_manager *mgr,
 				 const char *buf, size_t size)
 {
 	struct zynqmp_fpga_priv *priv;
+	int word_align, ret, index;
 	dma_addr_t dma_addr;
 	u32 eemi_flags = 0;
 	char *kbuf;
-	int ret;
 
 	priv = mgr->priv;
+	word_align = size % FPGA_WORD_SIZE;
+	if (word_align)
+		word_align = FPGA_WORD_SIZE - word_align;
+
+	size = size + word_align;
 
 	kbuf = dma_alloc_coherent(priv->dev, size, &dma_addr, GFP_KERNEL);
 	if (!kbuf)
 		return -ENOMEM;
 
-	memcpy(kbuf, buf, size);
+	for (index = 0; index < word_align; index++)
+		kbuf[index] = DUMMY_PAD_BYTE;
+
+	memcpy(&kbuf[index], buf, size - index);
 
 	wmb(); /* ensure all writes are done before initiate FW call */
 
-- 
2.25.1

Re: [PATCH] fpga: zynqmp: Make word align the configuration data

[Xu Yilun]

On 2023-03-14 at 15:12:22 +0530, Nava kishore Manne wrote:
> To avoid unwanted copies at firmware(PMUFW) this patch provides a fix

The copy happens in firmware? Please help briefly describe the firmware
operations in commit message.

> to align programmable logic(PL) configuration data if the data is not
> word-aligned. To align the configuration data this patch adds a few
> padding bytes and these additional padding bytes will not create any
> functional impact on the PL configuration.
> 
> Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com>
> ---
>  drivers/fpga/zynqmp-fpga.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/fpga/zynqmp-fpga.c b/drivers/fpga/zynqmp-fpga.c
> index c60f20949c47..70a12dc6e15c 100644
> --- a/drivers/fpga/zynqmp-fpga.c
> +++ b/drivers/fpga/zynqmp-fpga.c
> @@ -15,6 +15,9 @@
>  /* Constant Definitions */
>  #define IXR_FPGA_DONE_MASK	BIT(3)
>  
> +#define DUMMY_PAD_BYTE		0xFF
> +#define FPGA_WORD_SIZE		4
> +
>  /**
>   * struct zynqmp_fpga_priv - Private data structure
>   * @dev:	Device data structure
> @@ -41,18 +44,26 @@ static int zynqmp_fpga_ops_write(struct fpga_manager *mgr,
>  				 const char *buf, size_t size)
>  {
>  	struct zynqmp_fpga_priv *priv;
> +	int word_align, ret, index;
>  	dma_addr_t dma_addr;
>  	u32 eemi_flags = 0;
>  	char *kbuf;
> -	int ret;
>  
>  	priv = mgr->priv;
> +	word_align = size % FPGA_WORD_SIZE;
> +	if (word_align)
> +		word_align = FPGA_WORD_SIZE - word_align;
> +
> +	size = size + word_align;

Does the Macro ALIGN() help?

>  
>  	kbuf = dma_alloc_coherent(priv->dev, size, &dma_addr, GFP_KERNEL);
>  	if (!kbuf)
>  		return -ENOMEM;
>  
> -	memcpy(kbuf, buf, size);

This is historical, but why do the realloc & copy? Any better way?

> +	for (index = 0; index < word_align; index++)
> +		kbuf[index] = DUMMY_PAD_BYTE;
> +
> +	memcpy(&kbuf[index], buf, size - index);

Generally I object to massive copy in fpga_manager_ops::write if not
necessary. If there is an alignment requirement from HW, it should be
noticed to the caller in some way, before the buffer is created.

Thanks,
Yilun

>  
>  	wmb(); /* ensure all writes are done before initiate FW call */
>  
> -- 
> 2.25.1
> 

RE: [PATCH] fpga: zynqmp: Make word align the configuration data

[Manne, Nava kishore]

Hi Yilun,

	Please find my response inline.

> -----Original Message-----
> From: Xu Yilun <yilun.xu@intel.com>
> Sent: Saturday, March 18, 2023 2:55 PM
> To: Manne, Nava kishore <nava.kishore.manne@amd.com>
> Cc: mdf@kernel.org; hao.wu@intel.com; trix@redhat.com;
> michal.simek@xilinx.com; linux-fpga@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] fpga: zynqmp: Make word align the configuration data
> 
> On 2023-03-14 at 15:12:22 +0530, Nava kishore Manne wrote:
> > To avoid unwanted copies at firmware(PMUFW) this patch provides a fix
> 
> The copy happens in firmware? Please help briefly describe the firmware
> operations in commit message.
> 

Yes, If the firmware receives unaligned Bitstream file from Linux to make them align
it will do one more copy at firmware and this copy takes much time as firmware code
runs on microblaze(32-bit processor and runs at lower frequency). 
So, we suggested the users to handle the alignment issues at top layers(Before submitting request to the firmware).

Will update the description in v2.

> > to align programmable logic(PL) configuration data if the data is not
> > word-aligned. To align the configuration data this patch adds a few
> > padding bytes and these additional padding bytes will not create any
> > functional impact on the PL configuration.
> >
> > Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com>
> > ---
> >  drivers/fpga/zynqmp-fpga.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/fpga/zynqmp-fpga.c b/drivers/fpga/zynqmp-fpga.c
> > index c60f20949c47..70a12dc6e15c 100644
> > --- a/drivers/fpga/zynqmp-fpga.c
> > +++ b/drivers/fpga/zynqmp-fpga.c
> > @@ -15,6 +15,9 @@
> >  /* Constant Definitions */
> >  #define IXR_FPGA_DONE_MASK	BIT(3)
> >
> > +#define DUMMY_PAD_BYTE		0xFF
> > +#define FPGA_WORD_SIZE		4
> > +
> >  /**
> >   * struct zynqmp_fpga_priv - Private data structure
> >   * @dev:	Device data structure
> > @@ -41,18 +44,26 @@ static int zynqmp_fpga_ops_write(struct
> fpga_manager *mgr,
> >  				 const char *buf, size_t size)
> >  {
> >  	struct zynqmp_fpga_priv *priv;
> > +	int word_align, ret, index;
> >  	dma_addr_t dma_addr;
> >  	u32 eemi_flags = 0;
> >  	char *kbuf;
> > -	int ret;
> >
> >  	priv = mgr->priv;
> > +	word_align = size % FPGA_WORD_SIZE;
> > +	if (word_align)
> > +		word_align = FPGA_WORD_SIZE - word_align;
> > +
> > +	size = size + word_align;
> 
> Does the Macro ALIGN() help?
> 

Will fix in v2.

> >
> >  	kbuf = dma_alloc_coherent(priv->dev, size, &dma_addr,
> GFP_KERNEL);
> >  	if (!kbuf)
> >  		return -ENOMEM;
> >
> > -	memcpy(kbuf, buf, size);
> 
> This is historical, but why do the realloc & copy? Any better way?
> 

Firmware internally uses the AXI DMA engine to transfer PL data from memory to the device
and it supports only continues DMA-able memory access(It will not support scatter-gather memory access).
So, this extra copy is needed to copy the data from kernel memory(allocated by the firmware subsystem using page allocators)
to continues DMA-able memory.
 
> > +	for (index = 0; index < word_align; index++)
> > +		kbuf[index] = DUMMY_PAD_BYTE;
> > +
> > +	memcpy(&kbuf[index], buf, size - index);
> 
> Generally I object to massive copy in fpga_manager_ops::write if not
> necessary. If there is an alignment requirement from HW, it should be
> noticed to the caller in some way, before the buffer is created.
> 

Agree, we should find a way to support this kind of use cases. 

Regards,
Navakishore.

Re: [PATCH] fpga: zynqmp: Make word align the configuration data

[Xu Yilun]

On 2023-03-28 at 09:33:17 +0000, Manne, Nava kishore wrote:
> Hi Yilun,
> 
> 	Please find my response inline.
> 
> > -----Original Message-----
> > From: Xu Yilun <yilun.xu@intel.com>
> > Sent: Saturday, March 18, 2023 2:55 PM
> > To: Manne, Nava kishore <nava.kishore.manne@amd.com>
> > Cc: mdf@kernel.org; hao.wu@intel.com; trix@redhat.com;
> > michal.simek@xilinx.com; linux-fpga@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] fpga: zynqmp: Make word align the configuration data
> > 
> > On 2023-03-14 at 15:12:22 +0530, Nava kishore Manne wrote:
> > > To avoid unwanted copies at firmware(PMUFW) this patch provides a fix
> > 
> > The copy happens in firmware? Please help briefly describe the firmware
> > operations in commit message.
> > 
> 
> Yes, If the firmware receives unaligned Bitstream file from Linux to make them align
> it will do one more copy at firmware and this copy takes much time as firmware code
> runs on microblaze(32-bit processor and runs at lower frequency). 
> So, we suggested the users to handle the alignment issues at top layers(Before submitting request to the firmware).
> 
> Will update the description in v2.
> 
> > > to align programmable logic(PL) configuration data if the data is not
> > > word-aligned. To align the configuration data this patch adds a few
> > > padding bytes and these additional padding bytes will not create any
> > > functional impact on the PL configuration.
> > >
> > > Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com>
> > > ---
> > >  drivers/fpga/zynqmp-fpga.c | 15 +++++++++++++--
> > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/fpga/zynqmp-fpga.c b/drivers/fpga/zynqmp-fpga.c
> > > index c60f20949c47..70a12dc6e15c 100644
> > > --- a/drivers/fpga/zynqmp-fpga.c
> > > +++ b/drivers/fpga/zynqmp-fpga.c
> > > @@ -15,6 +15,9 @@
> > >  /* Constant Definitions */
> > >  #define IXR_FPGA_DONE_MASK	BIT(3)
> > >
> > > +#define DUMMY_PAD_BYTE		0xFF
> > > +#define FPGA_WORD_SIZE		4
> > > +
> > >  /**
> > >   * struct zynqmp_fpga_priv - Private data structure
> > >   * @dev:	Device data structure
> > > @@ -41,18 +44,26 @@ static int zynqmp_fpga_ops_write(struct
> > fpga_manager *mgr,
> > >  				 const char *buf, size_t size)
> > >  {
> > >  	struct zynqmp_fpga_priv *priv;
> > > +	int word_align, ret, index;
> > >  	dma_addr_t dma_addr;
> > >  	u32 eemi_flags = 0;
> > >  	char *kbuf;
> > > -	int ret;
> > >
> > >  	priv = mgr->priv;
> > > +	word_align = size % FPGA_WORD_SIZE;
> > > +	if (word_align)
> > > +		word_align = FPGA_WORD_SIZE - word_align;
> > > +
> > > +	size = size + word_align;
> > 
> > Does the Macro ALIGN() help?
> > 
> 
> Will fix in v2.
> 
> > >
> > >  	kbuf = dma_alloc_coherent(priv->dev, size, &dma_addr,
> > GFP_KERNEL);
> > >  	if (!kbuf)
> > >  		return -ENOMEM;
> > >
> > > -	memcpy(kbuf, buf, size);
> > 
> > This is historical, but why do the realloc & copy? Any better way?
> > 
> 
> Firmware internally uses the AXI DMA engine to transfer PL data from memory to the device
> and it supports only continues DMA-able memory access(It will not support scatter-gather memory access).
> So, this extra copy is needed to copy the data from kernel memory(allocated by the firmware subsystem using page allocators)

I see, but seems the issues are generic. Same issues for versal-fpga

1. fpga_mgr_buf_load_mapped(), fpgamanager_ops::write are intended for
   contiguous (and maybe dma-safe) buffer but fpga_mgr_firmware_load()
   breaks this. This may also affect spi backend drivers.

2. Some drivers needs dma addr rather than cpu addr, this could be
   generally supported. But allocating a bounce buffer by
   dma_alloc_coherent() is not a good idea.

3. fpga_manager_ops::write() may also be called for each fragment of a
   sg buffer, could the firmware OK to handle this case?

I think framework refactoring is needed for these issues. I may not have
time to do the refactoring but will support these topics in first
priority.

Thanks,
Yilun

> to continues DMA-able memory.
>  
> > > +	for (index = 0; index < word_align; index++)
> > > +		kbuf[index] = DUMMY_PAD_BYTE;
> > > +
> > > +	memcpy(&kbuf[index], buf, size - index);
> > 
> > Generally I object to massive copy in fpga_manager_ops::write if not
> > necessary. If there is an alignment requirement from HW, it should be
> > noticed to the caller in some way, before the buffer is created.
> > 
> 
> Agree, we should find a way to support this kind of use cases. 
> 
> Regards,
> Navakishore.
>