fpga: zynq-fpga: Replacing BIT with BIN format
Subsystems (2)
PATCH 1/1 fpga: zynq-fpga: Replacing BIT with BIN format
Hi all,
as discussed, here a patch that removes the byteswapping code, leaving format conversion to userland.
However I added a crude check to bail out early if the sync word isn't found in the firmware file, to avoid kicking the hardware unneccesary.
This change requires people to use the appropriate Xilinx tools to generate the (byteswapped) .bin files.
Cheers,
Moritz
Moritz Fischer (1):
fpga: zynq-fpga: Change fw format to handle bin instead of bit.
drivers/fpga/zynq-fpga.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-)
Signature · 1 line
2.6.1
PATCH 1/1 fpga: zynq-fpga: Change fw format to handle bin instead of bit.
This gets rid of the code to strip away the header and byteswap.
--- drivers/fpga/zynq-fpga.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-)
drivers/fpga/zynq-fpga.c +10 -13
@@ -104,6 +104,8 @@#define INIT_POLL_TIMEOUT 2500000/* Delay for polling reset bits */ #define INIT_POLL_DELAY 20 +/* Sync sequence for firmware */ +#define SYNC_SEQUENCE "\x66\x55\x99\xAA"/* Masks for controlling stuff in SLCR */ /* Disable all Level shifters */
@@ -301,24 +303,19 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,memcpy (kbuf, buf, count); -/* look for the sync word */ +/* look for the sync sequence */ for (i =0 ; i < count -4 ; i++) { -if (memcmp (kbuf + i,"\xAA\x99\x55\x66" ,4 ) ==0 ) { -dev_dbg (priv->dev,"Found swapped sync word\n" ); +if (memcmp (kbuf + i,SYNC_SEQUENCE ,4 ) ==0 ) { +dev_dbg (priv->dev,"Found sync sequence" );break ; } } -/* remove the header, align the data on word boundary */ -if (i != count -4 ) { - count -= i; -memmove (kbuf, kbuf + i, count); - } - -/* fixup endianness of the data */ -for (i =0 ; i < count; i +=4 ) { -u32 *p = (u32 *)&kbuf[i]; - *p =swab32 (*p); +/* if we detect something obviously broken, bail ... */ +if ((count -4 ) == i) { +dev_err (priv->dev,"No sync sequence found, invalid bitstream." ); + err = -EINVAL ; + goto out_free; }/* enable clock */
Signature · 1 line
2.6.1
PATCH 1/1 fpga: zynq-fpga: Change fw format to handle bin instead of bit.
Quoted 24 lines · Moritz Fischer
This gets rid of the code to strip away the header and byteswap.
Signed-off-by Moritz Fischer <[email protected]>--- drivers/fpga/zynq-fpga.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-)drivers/fpga/zynq-fpga.c +2
-0@@ -104,6 +104,8 @@#define INIT_POLL_TIMEOUT 2500000/* Delay for polling reset bits */ #define INIT_POLL_DELAY 20 +/* Sync sequence for firmware */ +#define SYNC_SEQUENCE "\x66\x55\x99\xAA"/* Masks for controlling stuff in SLCR */ /* Disable all Level shifters */ @@ -301,24 +303,19 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,memcpy (kbuf, buf, count);
drivers/fpga/zynq-fpga.c static int zynq_fpga_ops_write(struct fpga_manager *mgr, · line 312
Quoted 10 lines
drivers/fpga/zynq-fpga.c +3
-3-/* look for the sync word */ +/* look for the sync sequence */ for (i =0 ; i < count -4 ; i++) { -if (memcmp (kbuf + i,"\xAA\x99\x55\x66" ,4 ) ==0 ) { -dev_dbg (priv->dev,"Found swapped sync word\n" ); +if (memcmp (kbuf + i,SYNC_SEQUENCE ,4 ) ==0 ) { +dev_dbg (priv->dev,"Found sync sequence" );break ; } }
Regarding this check, and judging from the code, I'd say that any file with a non word-aligned sync word is broken. So you could speed this up considerably by walking through the array word-by-word instread of byte-by-byte.
Checking the whole 4MB buffer from beginning to end doesn't make it really efficient either, if you're going to visit all that memory in case the stream is invalid, you might as well send it to the hardware, I wouldn't be surprised if that even takes less effort than going through it byte-by-byte.
I for one would rather see this check removed, the hardware handles it just fine and also catches more likely errors (e.g. sending a 7030 bitstream to a 7015 chip is something that the driver cannot catch anyway, but the hardware detects that error just fine).
Quoted 9 lines
drivers/fpga/zynq-fpga.c +0
-9-/* remove the header, align the data on word boundary */ -if (i != count -4 ) { - count -= i; -memmove (kbuf, kbuf + i, count); - } - -/* fixup endianness of the data */ -for (i =0 ; i < count; i +=4 ) { -u32 *p = (u32 *)&kbuf[i];
drivers/fpga/zynq-fpga.c static int zynq_fpga_ops_write(struct fpga_manager *mgr,
Quoted 10 lines
drivers/fpga/zynq-fpga.c +5
-1- *p =swab32 (*p); +/* if we detect something obviously broken, bail ... */ +if ((count -4 ) == i) { +dev_err (priv->dev,"No sync sequence found, invalid bitstream." ); + err = -EINVAL ; + goto out_free; }/* enable clock */
Kind regards,
Mike Looijmans
System Expert
TOPIC Embedded Products
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
Telefax: +31 (0) 499 33 69 70
E-mail: mike.looijmans at topicproducts.com
Website: www.topicproducts.com
Please consider the environment before printing this e-mail
Visit us at : Aerospace Electrical Systems Expo Europe which will be held from 17.11.2015 till 19.11.2015, Findorffstra?e 101 Bremen, Germany, Hall 5, stand number C65
http://www.aesexpo.eu
PATCH 1/1 fpga: zynq-fpga: Change fw format to handle bin instead of bit.
Hi Mike,
Quoted 27 lines · Mike Looijmans
Quoted 26 lines · Moritz Fischer
This gets rid of the code to strip away the header and byteswap.
Signed-off-by Moritz Fischer <[email protected]>--- drivers/fpga/zynq-fpga.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-)drivers/fpga/zynq-fpga.c +2
-0@@ -104,6 +104,8 @@#define INIT_POLL_TIMEOUT 2500000/* Delay for polling reset bits */ #define INIT_POLL_DELAY 20 +/* Sync sequence for firmware */ +#define SYNC_SEQUENCE "\x66\x55\x99\xAA"/* Masks for controlling stuff in SLCR */ /* Disable all Level shifters */ @@ -301,24 +303,19 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,memcpy (kbuf, buf, count);
Quoted 16 lines
drivers/fpga/zynq-fpga.c static int zynq_fpga_ops_write(struct fpga_manager *mgr, · line 312
Quoted 10 lines
drivers/fpga/zynq-fpga.c +3
-3-/* look for the sync word */ +/* look for the sync sequence */ for (i =0 ; i < count -4 ; i++) { -if (memcmp (kbuf + i,"\xAA\x99\x55\x66" ,4 ) ==0 ) { -dev_dbg (priv->dev,"Found swapped sync word\n" ); +if (memcmp (kbuf + i,SYNC_SEQUENCE ,4 ) ==0 ) { +dev_dbg (priv->dev,"Found sync sequence" );break ; } }Regarding this check, and judging from the code, I'd say that any file with a non word-aligned sync word is broken. So you could speed this up considerably by walking through the array word-by-word instread of byte-by-byte.
True.
Quoted 5 lines
Checking the whole 4MB buffer from beginning to end doesn't make it really efficient either, if you're going to visit all that memory in case the stream is invalid, you might as well send it to the hardware, I wouldn't be surprised if that even takes less effort than going through it byte-by-byte.
You raised a good point here. I was hesitant to put in the additional check, in my tests the reloading took no noticeable time, which was good enough for my application, so I figured rather be more 'defensive'. Probably overly defensive, as the reloading entity would have to check for success in either case before attempting to use the FPGA afterwards.
Quoted 4 lines
I for one would rather see this check removed, the hardware handles it just fine and also catches more likely errors (e.g. sending a 7030 bitstream to a 7015 chip is something that the driver cannot catch anyway, but the hardware detects that error just fine).
Fair enough. I'll remove it for v2, makes the code even easier :-)
Thanks for the feedback,
Moritz
PATCH 1/1 fpga: zynq-fpga: Change fw format to handle bin instead of bit.
In reply to Moritz FischerQuoted 24 lines · Moritz Fischer
This gets rid of the code to strip away the header and byteswap.
Signed-off-by Moritz Fischer <[email protected]>--- drivers/fpga/zynq-fpga.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-)drivers/fpga/zynq-fpga.c +2
-0@@ -104,6 +104,8 @@#define INIT_POLL_TIMEOUT 2500000/* Delay for polling reset bits */ #define INIT_POLL_DELAY 20 +/* Sync sequence for firmware */ +#define SYNC_SEQUENCE "\x66\x55\x99\xAA"/* Masks for controlling stuff in SLCR */ /* Disable all Level shifters */ @@ -301,24 +303,19 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,memcpy (kbuf, buf, count);
drivers/fpga/zynq-fpga.c static int zynq_fpga_ops_write(struct fpga_manager *mgr, · line 312
Quoted 10 lines
drivers/fpga/zynq-fpga.c +3
-3-/* look for the sync word */ +/* look for the sync sequence */ for (i =0 ; i < count -4 ; i++) { -if (memcmp (kbuf + i,"\xAA\x99\x55\x66" ,4 ) ==0 ) { -dev_dbg (priv->dev,"Found swapped sync word\n" ); +if (memcmp (kbuf + i,SYNC_SEQUENCE ,4 ) ==0 ) { +dev_dbg (priv->dev,"Found sync sequence" );break ; } }
Regarding this check, and judging from the code, I'd say that any file with a non word-aligned sync word is broken. So you could speed this up considerably by walking through the array word-by-word instread of byte-by-byte.
Checking the whole 4MB buffer from beginning to end doesn't make it really efficient either, if you're going to visit all that memory in case the stream is invalid, you might as well send it to the hardware, I wouldn't be surprised if that even takes less effort than going through it byte-by-byte.
I for one would rather see this check removed, the hardware handles it just fine and also catches more likely errors (e.g. sending a 7030 bitstream to a 7015 chip is something that the driver cannot catch anyway, but the hardware detects that error just fine).
Quoted 9 lines
drivers/fpga/zynq-fpga.c +0
-9-/* remove the header, align the data on word boundary */ -if (i != count -4 ) { - count -= i; -memmove (kbuf, kbuf + i, count); - } - -/* fixup endianness of the data */ -for (i =0 ; i < count; i +=4 ) { -u32 *p = (u32 *)&kbuf[i];
drivers/fpga/zynq-fpga.c static int zynq_fpga_ops_write(struct fpga_manager *mgr,
Quoted 10 lines
drivers/fpga/zynq-fpga.c +5
-1- *p =swab32 (*p); +/* if we detect something obviously broken, bail ... */ +if ((count -4 ) == i) { +dev_err (priv->dev,"No sync sequence found, invalid bitstream." ); + err = -EINVAL ; + goto out_free; }/* enable clock */
Kind regards,
Mike Looijmans
System Expert
TOPIC Embedded Products
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
Telefax: +31 (0) 499 33 69 70
E-mail: mike.looijmans at topicproducts.com
Website: www.topicproducts.com
Please consider the environment before printing this e-mail
Visit us at : Aerospace Electrical Systems Expo Europe which will be held from 17.11.2015 till 19.11.2015, Findorffstra?e 101 Bremen, Germany, Hall 5, stand number C65
http://www.aesexpo.eu
PATCH 1/1 fpga: zynq-fpga: Change fw format to handle bin instead of bit.
In reply to Moritz FischerQuoted 24 lines · Moritz Fischer
This gets rid of the code to strip away the header and byteswap.
Signed-off-by Moritz Fischer <[email protected]>--- drivers/fpga/zynq-fpga.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-)drivers/fpga/zynq-fpga.c +2
-0@@ -104,6 +104,8 @@#define INIT_POLL_TIMEOUT 2500000/* Delay for polling reset bits */ #define INIT_POLL_DELAY 20 +/* Sync sequence for firmware */ +#define SYNC_SEQUENCE "\x66\x55\x99\xAA"/* Masks for controlling stuff in SLCR */ /* Disable all Level shifters */ @@ -301,24 +303,19 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr,memcpy (kbuf, buf, count);
drivers/fpga/zynq-fpga.c static int zynq_fpga_ops_write(struct fpga_manager *mgr, · line 312
Quoted 10 lines
drivers/fpga/zynq-fpga.c +3
-3-/* look for the sync word */ +/* look for the sync sequence */ for (i =0 ; i < count -4 ; i++) { -if (memcmp (kbuf + i,"\xAA\x99\x55\x66" ,4 ) ==0 ) { -dev_dbg (priv->dev,"Found swapped sync word\n" ); +if (memcmp (kbuf + i,SYNC_SEQUENCE ,4 ) ==0 ) { +dev_dbg (priv->dev,"Found sync sequence" );break ; } }
Regarding this check, and judging from the code, I'd say that any file with a non word-aligned sync word is broken. So you could speed this up considerably by walking through the array word-by-word instread of byte-by-byte.
Checking the whole 4MB buffer from beginning to end doesn't make it really efficient either, if you're going to visit all that memory in case the stream is invalid, you might as well send it to the hardware, I wouldn't be surprised if that even takes less effort than going through it byte-by-byte.
I for one would rather see this check removed, the hardware handles it just fine and also catches more likely errors (e.g. sending a 7030 bitstream to a 7015 chip is something that the driver cannot catch anyway, but the hardware detects that error just fine).
Quoted 19 lines
drivers/fpga/zynq-fpga.c +5
-10-/* remove the header, align the data on word boundary */ -if (i != count -4 ) { - count -= i; -memmove (kbuf, kbuf + i, count); - } - -/* fixup endianness of the data */ -for (i =0 ; i < count; i +=4 ) { -u32 *p = (u32 *)&kbuf[i]; - *p =swab32 (*p); +/* if we detect something obviously broken, bail ... */ +if ((count -4 ) == i) { +dev_err (priv->dev,"No sync sequence found, invalid bitstream." ); + err = -EINVAL ; + goto out_free; }/* enable clock */
PATCH 1/1 fpga: zynq-fpga: Change fw format to handle bin instead of bit.
?Sorry for having sent that out about three times, I was cleverly being fooled into believing the mail didn't get delivered.
I also want to apologize for adding yet another message to that list. I'm sorry, it won't happen again.
On 20-10-15 08:57, Mike Looijmans wrote:
...
Kind regards,
Mike Looijmans
System Expert
TOPIC Embedded Products
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
Telefax: +31 (0) 499 33 69 70
E-mail: mike.looijmans at topicproducts.com
Website: www.topicproducts.com
Please consider the environment before printing this e-mail
Visit us at : Aerospace Electrical Systems Expo Europe which will be held from 17.11.2015 till 19.11.2015, Findorffstrasse 101 Bremen, Germany, Hall 5, stand number C65
http://www.aesexpo.eu