mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH 0/3] firmware: zynqmp-fpga: fix fpga loading with optimized string functions
@ 2021-08-18 13:35 Michael Tretter
  2021-08-18 13:35 ` [PATCH 1/3] firmware: zynqmp-fpga: initialize flags at function start Michael Tretter
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Michael Tretter @ 2021-08-18 13:35 UTC (permalink / raw)
  To: barebox

If CONFIG_ARM_OPTIMZED_STRING_FUNCTIONS is enabled, loading the FPGA fails
with an abort, because the optimized memcpy can only be used on cached memory.
As the bitstream can be several MBs large, we want to use the optimized
functions. Fix the abort by using a cached mapping with streaming DMA.

Michael Tretter (3):
  firmware: zynqmp-fpga: initialize flags at function start
  firmware: zynqmp-fpga: avoid additional buffer for size argument
  firmware: zynqmp-fpga: do not use DMA coherent memory for bitstream

 drivers/firmware/zynqmp-fpga.c | 54 ++++++++++++++++------------------
 1 file changed, 26 insertions(+), 28 deletions(-)

-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/3] firmware: zynqmp-fpga: initialize flags at function start
  2021-08-18 13:35 [PATCH 0/3] firmware: zynqmp-fpga: fix fpga loading with optimized string functions Michael Tretter
@ 2021-08-18 13:35 ` Michael Tretter
  2021-08-18 13:35 ` [PATCH 2/3] firmware: zynqmp-fpga: avoid additional buffer for size argument Michael Tretter
  2021-08-18 13:35 ` [PATCH 3/3] firmware: zynqmp-fpga: do not use DMA coherent memory for bitstream Michael Tretter
  2 siblings, 0 replies; 6+ messages in thread
From: Michael Tretter @ 2021-08-18 13:35 UTC (permalink / raw)
  To: barebox

The ZYNQMP_FPGA_BIT_ONLY_BIN flag is always set when programming the
FPGA. Simplify the code by initializing the flags with
ZYNQMP_FPGA_BIT_ONLY_BIN already set.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/firmware/zynqmp-fpga.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/firmware/zynqmp-fpga.c b/drivers/firmware/zynqmp-fpga.c
index 0fc229bfd3dd..736d1950fa5e 100644
--- a/drivers/firmware/zynqmp-fpga.c
+++ b/drivers/firmware/zynqmp-fpga.c
@@ -205,7 +205,7 @@ static int fpgamgr_program_finish(struct firmware_handler *fh)
 	enum xilinx_byte_order byte_order;
 	u64 addr;
 	int status = 0;
-	u8 flags = 0;
+	u8 flags = ZYNQMP_FPGA_BIT_ONLY_BIN;
 
 	if (!mgr->buf) {
 		status = -ENOBUFS;
@@ -259,9 +259,6 @@ static int fpgamgr_program_finish(struct firmware_handler *fh)
 
 	addr = (u64)buf_aligned;
 
-	/* we do not provide a header */
-	flags |= ZYNQMP_FPGA_BIT_ONLY_BIN;
-
 	if (!(mgr->features & ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED) && buf_size) {
 		status = mgr->eemi_ops->fpga_load(addr,
 				(u32)(uintptr_t)buf_size,
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 2/3] firmware: zynqmp-fpga: avoid additional buffer for size argument
  2021-08-18 13:35 [PATCH 0/3] firmware: zynqmp-fpga: fix fpga loading with optimized string functions Michael Tretter
  2021-08-18 13:35 ` [PATCH 1/3] firmware: zynqmp-fpga: initialize flags at function start Michael Tretter
@ 2021-08-18 13:35 ` Michael Tretter
  2021-08-18 13:35 ` [PATCH 3/3] firmware: zynqmp-fpga: do not use DMA coherent memory for bitstream Michael Tretter
  2 siblings, 0 replies; 6+ messages in thread
From: Michael Tretter @ 2021-08-18 13:35 UTC (permalink / raw)
  To: barebox

There are two different APIs for loading an FPGA via the pmu-fw as
indicated by the ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED feature flag.
The pmu-fw expects as second argument either the size of the bitstream or a
pointer to the size of the bitstream.

The driver allocates a separate buffer for the size, which results in
the allocation of a 4k page for storing a 32 bit value.

Allocate some more memory for the bitstream and append the size of the
bitstream at the end of the bitstream to avoid the additional memory
allocation.

Add a comment to explain the surprising size of the allocation.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/firmware/zynqmp-fpga.c | 37 +++++++++++++++-------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/firmware/zynqmp-fpga.c b/drivers/firmware/zynqmp-fpga.c
index 736d1950fa5e..667910479aa7 100644
--- a/drivers/firmware/zynqmp-fpga.c
+++ b/drivers/firmware/zynqmp-fpga.c
@@ -197,8 +197,8 @@ static void zynqmp_fpga_show_header(const struct device_d *dev,
 static int fpgamgr_program_finish(struct firmware_handler *fh)
 {
 	struct fpgamgr *mgr = container_of(fh, struct fpgamgr, fh);
-	char *buf_aligned;
-	u32 *buf_size = NULL;
+	u32 *buf_aligned;
+	u32 buf_size;
 	u32 *body;
 	size_t body_length;
 	int header_length = 0;
@@ -234,17 +234,14 @@ static int fpgamgr_program_finish(struct firmware_handler *fh)
 		goto err_free;
 	}
 
-	if (!(mgr->features & ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED)) {
-		buf_size = dma_alloc_coherent(sizeof(*buf_size),
-		DMA_ADDRESS_BROKEN);
-		if (!buf_size) {
-			status = -ENOBUFS;
-			goto err_free;
-		}
-		*buf_size = body_length;
-	}
-
-	buf_aligned = dma_alloc_coherent(body_length, DMA_ADDRESS_BROKEN);
+	/*
+	 * The PMU FW variants without the ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED
+	 * feature expect a pointer to the bitstream size, which is stored in
+	 * memory. Allocate some extra space at the end of the buffer for the
+	 * bitstream size.
+	 */
+	buf_aligned = dma_alloc_coherent(body_length + sizeof(buf_size),
+					 DMA_ADDRESS_BROKEN);
 	if (!buf_aligned) {
 		status = -ENOBUFS;
 		goto err_free;
@@ -259,20 +256,18 @@ static int fpgamgr_program_finish(struct firmware_handler *fh)
 
 	addr = (u64)buf_aligned;
 
-	if (!(mgr->features & ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED) && buf_size) {
-		status = mgr->eemi_ops->fpga_load(addr,
-				(u32)(uintptr_t)buf_size,
-				flags);
-		dma_free_coherent(buf_size, 0, sizeof(*buf_size));
+	if (mgr->features & ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED) {
+		buf_size = body_length;
 	} else {
-		status = mgr->eemi_ops->fpga_load(addr, (u32)(body_length),
-						  flags);
+		buf_aligned[body_length / sizeof(*buf_aligned)] = body_length;
+		buf_size = addr + body_length;
 	}
 
+	status = mgr->eemi_ops->fpga_load(addr, buf_size, flags);
 	if (status < 0)
 		dev_err(&mgr->dev, "unable to load fpga\n");
 
-	dma_free_coherent(buf_aligned, 0, body_length);
+	dma_free_coherent(buf_aligned, 0, body_length + sizeof(buf_size));
 
  err_free:
 	free(mgr->buf);
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 3/3] firmware: zynqmp-fpga: do not use DMA coherent memory for bitstream
  2021-08-18 13:35 [PATCH 0/3] firmware: zynqmp-fpga: fix fpga loading with optimized string functions Michael Tretter
  2021-08-18 13:35 ` [PATCH 1/3] firmware: zynqmp-fpga: initialize flags at function start Michael Tretter
  2021-08-18 13:35 ` [PATCH 2/3] firmware: zynqmp-fpga: avoid additional buffer for size argument Michael Tretter
@ 2021-08-18 13:35 ` Michael Tretter
  2021-08-18 13:47   ` Lucas Stach
  2 siblings, 1 reply; 6+ messages in thread
From: Michael Tretter @ 2021-08-18 13:35 UTC (permalink / raw)
  To: barebox

Trying to do unaligned access of coherent memory on AArch64 will lead to
an abort. This can happen when the FPGA loader copies the bitstream to
the temporary buffer for the transfer to the FPGA.

Convert the driver to use regular memory for the temporary buffer to
prevent the issue.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/firmware/zynqmp-fpga.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/firmware/zynqmp-fpga.c b/drivers/firmware/zynqmp-fpga.c
index 667910479aa7..0a0e7e880849 100644
--- a/drivers/firmware/zynqmp-fpga.c
+++ b/drivers/firmware/zynqmp-fpga.c
@@ -203,7 +203,7 @@ static int fpgamgr_program_finish(struct firmware_handler *fh)
 	size_t body_length;
 	int header_length = 0;
 	enum xilinx_byte_order byte_order;
-	u64 addr;
+	dma_addr_t addr;
 	int status = 0;
 	u8 flags = ZYNQMP_FPGA_BIT_ONLY_BIN;
 
@@ -240,13 +240,19 @@ static int fpgamgr_program_finish(struct firmware_handler *fh)
 	 * memory. Allocate some extra space at the end of the buffer for the
 	 * bitstream size.
 	 */
-	buf_aligned = dma_alloc_coherent(body_length + sizeof(buf_size),
-					 DMA_ADDRESS_BROKEN);
+	buf_aligned = dma_alloc(body_length + sizeof(u32));
 	if (!buf_aligned) {
 		status = -ENOBUFS;
 		goto err_free;
 	}
 
+	addr = dma_map_single(&mgr->dev, buf_aligned,
+			      body_length + sizeof(u32), DMA_TO_DEVICE);
+	if (dma_mapping_error(&mgr->dev, addr)) {
+		status = -EFAULT;
+		goto err_free;
+	}
+
 	if (!(mgr->features & ZYNQMP_PM_FEATURE_BYTE_ORDER_IRREL) &&
 	    byte_order == XILINX_BYTE_ORDER_BIN)
 		copy_words_swapped((u32 *)buf_aligned, body,
@@ -254,8 +260,6 @@ static int fpgamgr_program_finish(struct firmware_handler *fh)
 	else
 		memcpy((u32 *)buf_aligned, body, body_length);
 
-	addr = (u64)buf_aligned;
-
 	if (mgr->features & ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED) {
 		buf_size = body_length;
 	} else {
@@ -263,11 +267,13 @@ static int fpgamgr_program_finish(struct firmware_handler *fh)
 		buf_size = addr + body_length;
 	}
 
-	status = mgr->eemi_ops->fpga_load(addr, buf_size, flags);
+	dma_sync_single_for_device(addr, body_length + sizeof(u32), DMA_TO_DEVICE);
+	status = mgr->eemi_ops->fpga_load((u64)addr, buf_size, flags);
+	dma_sync_single_for_cpu(addr, body_length + sizeof(u32), DMA_TO_DEVICE);
 	if (status < 0)
 		dev_err(&mgr->dev, "unable to load fpga\n");
 
-	dma_free_coherent(buf_aligned, 0, body_length + sizeof(buf_size));
+	dma_free(buf_aligned);
 
  err_free:
 	free(mgr->buf);
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 3/3] firmware: zynqmp-fpga: do not use DMA coherent memory for bitstream
  2021-08-18 13:35 ` [PATCH 3/3] firmware: zynqmp-fpga: do not use DMA coherent memory for bitstream Michael Tretter
@ 2021-08-18 13:47   ` Lucas Stach
  2021-08-19  8:14     ` Michael Tretter
  0 siblings, 1 reply; 6+ messages in thread
From: Lucas Stach @ 2021-08-18 13:47 UTC (permalink / raw)
  To: Michael Tretter, barebox

Am Mittwoch, dem 18.08.2021 um 15:35 +0200 schrieb Michael Tretter:
> Trying to do unaligned access of coherent memory on AArch64 will lead to
> an abort. This can happen when the FPGA loader copies the bitstream to
> the temporary buffer for the transfer to the FPGA.
> 
> Convert the driver to use regular memory for the temporary buffer to
> prevent the issue.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  drivers/firmware/zynqmp-fpga.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/firmware/zynqmp-fpga.c b/drivers/firmware/zynqmp-fpga.c
> index 667910479aa7..0a0e7e880849 100644
> --- a/drivers/firmware/zynqmp-fpga.c
> +++ b/drivers/firmware/zynqmp-fpga.c
> @@ -203,7 +203,7 @@ static int fpgamgr_program_finish(struct firmware_handler *fh)
>  	size_t body_length;
>  	int header_length = 0;
>  	enum xilinx_byte_order byte_order;
> -	u64 addr;
> +	dma_addr_t addr;
>  	int status = 0;
>  	u8 flags = ZYNQMP_FPGA_BIT_ONLY_BIN;
>  
> @@ -240,13 +240,19 @@ static int fpgamgr_program_finish(struct firmware_handler *fh)
>  	 * memory. Allocate some extra space at the end of the buffer for the
>  	 * bitstream size.
>  	 */
> -	buf_aligned = dma_alloc_coherent(body_length + sizeof(buf_size),
> -					 DMA_ADDRESS_BROKEN);
> +	buf_aligned = dma_alloc(body_length + sizeof(u32));
>  	if (!buf_aligned) {
>  		status = -ENOBUFS;
>  		goto err_free;
>  	}
>  
> +	addr = dma_map_single(&mgr->dev, buf_aligned,
> +			      body_length + sizeof(u32), DMA_TO_DEVICE);
> +	if (dma_mapping_error(&mgr->dev, addr)) {
> +		status = -EFAULT;
> +		goto err_free;
> +	}
> +
Usage of both dma_map_single and explicit dma_sync_single_for_* for a
single transfer looks odd. dma_map_single already does the cache sync,
which you then do a second time in the sync calls.

Instead you should move this dma_map_single call to the place where you
added the dma_sync_single_for_device and replace the
dma_sync_single_for_cpu with a dma_unmap_single.

Regards,
Lucas

>  	if (!(mgr->features & ZYNQMP_PM_FEATURE_BYTE_ORDER_IRREL) &&
>  	    byte_order == XILINX_BYTE_ORDER_BIN)
>  		copy_words_swapped((u32 *)buf_aligned, body,
> @@ -254,8 +260,6 @@ static int fpgamgr_program_finish(struct firmware_handler *fh)
>  	else
>  		memcpy((u32 *)buf_aligned, body, body_length);
>  
> -	addr = (u64)buf_aligned;
> -
>  	if (mgr->features & ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED) {
>  		buf_size = body_length;
>  	} else {
> @@ -263,11 +267,13 @@ static int fpgamgr_program_finish(struct firmware_handler *fh)
>  		buf_size = addr + body_length;
>  	}
>  
> -	status = mgr->eemi_ops->fpga_load(addr, buf_size, flags);
> +	dma_sync_single_for_device(addr, body_length + sizeof(u32), DMA_TO_DEVICE);
> +	status = mgr->eemi_ops->fpga_load((u64)addr, buf_size, flags);
> +	dma_sync_single_for_cpu(addr, body_length + sizeof(u32), DMA_TO_DEVICE);
>  	if (status < 0)
>  		dev_err(&mgr->dev, "unable to load fpga\n");
>  
> -	dma_free_coherent(buf_aligned, 0, body_length + sizeof(buf_size));
> +	dma_free(buf_aligned);
>  
>   err_free:
>  	free(mgr->buf);



_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 3/3] firmware: zynqmp-fpga: do not use DMA coherent memory for bitstream
  2021-08-18 13:47   ` Lucas Stach
@ 2021-08-19  8:14     ` Michael Tretter
  0 siblings, 0 replies; 6+ messages in thread
From: Michael Tretter @ 2021-08-19  8:14 UTC (permalink / raw)
  To: Lucas Stach; +Cc: barebox

On Wed, 18 Aug 2021 15:47:02 +0200, Lucas Stach wrote:
> Am Mittwoch, dem 18.08.2021 um 15:35 +0200 schrieb Michael Tretter:
> > Trying to do unaligned access of coherent memory on AArch64 will lead to
> > an abort. This can happen when the FPGA loader copies the bitstream to
> > the temporary buffer for the transfer to the FPGA.
> > 
> > Convert the driver to use regular memory for the temporary buffer to
> > prevent the issue.
> > 
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> >  drivers/firmware/zynqmp-fpga.c | 20 +++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/firmware/zynqmp-fpga.c b/drivers/firmware/zynqmp-fpga.c
> > index 667910479aa7..0a0e7e880849 100644
> > --- a/drivers/firmware/zynqmp-fpga.c
> > +++ b/drivers/firmware/zynqmp-fpga.c
> > @@ -203,7 +203,7 @@ static int fpgamgr_program_finish(struct firmware_handler *fh)
> >  	size_t body_length;
> >  	int header_length = 0;
> >  	enum xilinx_byte_order byte_order;
> > -	u64 addr;
> > +	dma_addr_t addr;
> >  	int status = 0;
> >  	u8 flags = ZYNQMP_FPGA_BIT_ONLY_BIN;
> >  
> > @@ -240,13 +240,19 @@ static int fpgamgr_program_finish(struct firmware_handler *fh)
> >  	 * memory. Allocate some extra space at the end of the buffer for the
> >  	 * bitstream size.
> >  	 */
> > -	buf_aligned = dma_alloc_coherent(body_length + sizeof(buf_size),
> > -					 DMA_ADDRESS_BROKEN);
> > +	buf_aligned = dma_alloc(body_length + sizeof(u32));
> >  	if (!buf_aligned) {
> >  		status = -ENOBUFS;
> >  		goto err_free;
> >  	}
> >  
> > +	addr = dma_map_single(&mgr->dev, buf_aligned,
> > +			      body_length + sizeof(u32), DMA_TO_DEVICE);
> > +	if (dma_mapping_error(&mgr->dev, addr)) {
> > +		status = -EFAULT;
> > +		goto err_free;
> > +	}
> > +
> Usage of both dma_map_single and explicit dma_sync_single_for_* for a
> single transfer looks odd. dma_map_single already does the cache sync,
> which you then do a second time in the sync calls.
> 
> Instead you should move this dma_map_single call to the place where you
> added the dma_sync_single_for_device and replace the
> dma_sync_single_for_cpu with a dma_unmap_single.

Thanks, I just sent a v2.

Michael

> 
> Regards,
> Lucas
> 
> >  	if (!(mgr->features & ZYNQMP_PM_FEATURE_BYTE_ORDER_IRREL) &&
> >  	    byte_order == XILINX_BYTE_ORDER_BIN)
> >  		copy_words_swapped((u32 *)buf_aligned, body,
> > @@ -254,8 +260,6 @@ static int fpgamgr_program_finish(struct firmware_handler *fh)
> >  	else
> >  		memcpy((u32 *)buf_aligned, body, body_length);
> >  
> > -	addr = (u64)buf_aligned;
> > -
> >  	if (mgr->features & ZYNQMP_PM_FEATURE_SIZE_NOT_NEEDED) {
> >  		buf_size = body_length;
> >  	} else {
> > @@ -263,11 +267,13 @@ static int fpgamgr_program_finish(struct firmware_handler *fh)
> >  		buf_size = addr + body_length;
> >  	}
> >  
> > -	status = mgr->eemi_ops->fpga_load(addr, buf_size, flags);
> > +	dma_sync_single_for_device(addr, body_length + sizeof(u32), DMA_TO_DEVICE);
> > +	status = mgr->eemi_ops->fpga_load((u64)addr, buf_size, flags);
> > +	dma_sync_single_for_cpu(addr, body_length + sizeof(u32), DMA_TO_DEVICE);
> >  	if (status < 0)
> >  		dev_err(&mgr->dev, "unable to load fpga\n");
> >  
> > -	dma_free_coherent(buf_aligned, 0, body_length + sizeof(buf_size));
> > +	dma_free(buf_aligned);
> >  
> >   err_free:
> >  	free(mgr->buf);
> 
> 
> 

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-08-19  8:16 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-18 13:35 [PATCH 0/3] firmware: zynqmp-fpga: fix fpga loading with optimized string functions Michael Tretter
2021-08-18 13:35 ` [PATCH 1/3] firmware: zynqmp-fpga: initialize flags at function start Michael Tretter
2021-08-18 13:35 ` [PATCH 2/3] firmware: zynqmp-fpga: avoid additional buffer for size argument Michael Tretter
2021-08-18 13:35 ` [PATCH 3/3] firmware: zynqmp-fpga: do not use DMA coherent memory for bitstream Michael Tretter
2021-08-18 13:47   ` Lucas Stach
2021-08-19  8:14     ` Michael Tretter

mail archive of the barebox mailing list

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lore.barebox.org/barebox/0 barebox/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 barebox barebox/ https://lore.barebox.org/barebox \
		barebox@lists.infradead.org barebox@lists.infradead.org
	public-inbox-index barebox

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git