* [PATCH] fpga: zynqmp: Make word align the configuration data @ 2023-03-14 9:42 Nava kishore Manne 2023-03-18 9:25 ` Xu Yilun 0 siblings, 1 reply; 7+ messages in thread From: Nava kishore Manne @ 2023-03-14 9:42 UTC (permalink / raw) To: mdf, hao.wu, yilun.xu, trix, michal.simek, linux-fpga, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] fpga: zynqmp: Make word align the configuration data 2023-03-14 9:42 [PATCH] fpga: zynqmp: Make word align the configuration data Nava kishore Manne @ 2023-03-18 9:25 ` Xu Yilun 0 siblings, 0 replies; 7+ messages in thread From: Xu Yilun @ 2023-03-18 9:25 UTC (permalink / raw) To: Nava kishore Manne Cc: mdf, hao.wu, trix, michal.simek, linux-fpga, linux-arm-kernel, linux-kernel 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fpga: zynqmp: Make word align the configuration data @ 2023-03-18 9:25 ` Xu Yilun 0 siblings, 0 replies; 7+ messages in thread From: Xu Yilun @ 2023-03-18 9:25 UTC (permalink / raw) To: Nava kishore Manne Cc: mdf, hao.wu, trix, michal.simek, linux-fpga, linux-arm-kernel, linux-kernel 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 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] fpga: zynqmp: Make word align the configuration data 2023-03-18 9:25 ` Xu Yilun @ 2023-03-28 9:33 ` Manne, Nava kishore -1 siblings, 0 replies; 7+ messages in thread From: Manne, Nava kishore @ 2023-03-28 9:33 UTC (permalink / raw) To: Xu Yilun Cc: mdf, hao.wu, trix, michal.simek, linux-fpga, linux-arm-kernel, linux-kernel 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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] fpga: zynqmp: Make word align the configuration data @ 2023-03-28 9:33 ` Manne, Nava kishore 0 siblings, 0 replies; 7+ messages in thread From: Manne, Nava kishore @ 2023-03-28 9:33 UTC (permalink / raw) To: Xu Yilun Cc: mdf, hao.wu, trix, michal.simek, linux-fpga, linux-arm-kernel, linux-kernel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fpga: zynqmp: Make word align the configuration data 2023-03-28 9:33 ` Manne, Nava kishore @ 2023-04-01 15:43 ` Xu Yilun -1 siblings, 0 replies; 7+ messages in thread From: Xu Yilun @ 2023-04-01 15:43 UTC (permalink / raw) To: Manne, Nava kishore Cc: mdf, hao.wu, trix, michal.simek, linux-fpga, linux-arm-kernel, linux-kernel 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. > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fpga: zynqmp: Make word align the configuration data @ 2023-04-01 15:43 ` Xu Yilun 0 siblings, 0 replies; 7+ messages in thread From: Xu Yilun @ 2023-04-01 15:43 UTC (permalink / raw) To: Manne, Nava kishore Cc: mdf, hao.wu, trix, michal.simek, linux-fpga, linux-arm-kernel, linux-kernel 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. > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-04-01 15:56 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-14 9:42 [PATCH] fpga: zynqmp: Make word align the configuration data Nava kishore Manne 2023-03-18 9:25 ` Xu Yilun 2023-03-18 9:25 ` Xu Yilun 2023-03-28 9:33 ` Manne, Nava kishore 2023-03-28 9:33 ` Manne, Nava kishore 2023-04-01 15:43 ` Xu Yilun 2023-04-01 15:43 ` Xu Yilun
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.