Hi Yilun,
Please find my response inline.
> -----Original Message-----
> From: Xu Yilun <yilun.xu@xxxxxxxxx>
> Sent: Saturday, March 18, 2023 2:55 PM
> To: Manne, Nava kishore <nava.kishore.manne@xxxxxxx>
> Cc: mdf@xxxxxxxxxx; hao.wu@xxxxxxxxx; trix@xxxxxxxxxx;
> michal.simek@xxxxxxxxxx; linux-fpga@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> 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@xxxxxxx>
> > ---
> > 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.
