[U-Boot-Users] RFC: Xilinx Spartan3 relocation code

[U-Boot-Users] RFC: Xilinx Spartan3 relocation code

[Bruce_Leonard at selinc.com]

Hi,

Our custom board has a Spartan3 FPGA on it and we're using u-boot to 
program it.  I've come across an item (I won't call it an issue since 
there's a fairly simple work around) in the function Spartan3_sp_reloc in 
.../common/spartan3.c.  (Looks like the Spartan3_ss_reloc function has the 
same issue.)

Some of the FPGA functions are optional.  That is to say the higher level 
code checks to see if the custom function pointer is non-NULL before it 
gets used.  The 'pre' function is a good example.  I don't have any need 
for a 'pre' function in my particular setup, so in my 
Xilinx_Spartan3_Slave_Parallel_fns structure I set that pointer to NULL. 
The item comes up in the relocate functions mentioned above.  They blindly 
add gd->reloc_offset to the function pointer values in the list, making 
the relocated 'pre' function pointer non-NULL.  The 'fpga load' command 
then tries to execute the 'pre' function and causes the system to crash 
because it's trying to execute bogus code.

The simple work around is to declare a function for 'pre' that does 
nothing.  To me this defeats the purpose of the non-NULL checking that 
other code does and forces the user to add code that does nothing for 
them.  I like the non-NULL checking because it's robust and allows you to 
only include the code you actually need.

So, to the RFC: I think the code that does the relocation of the FPGA 
description structures should check for NULL before adding 
gd->reloc_offset to the function pointers.  I think it keeps things in 
line with the original intent of the authors of the FPGA and makes the 
whole thing more robust.  Comments?

I'm happy to make the changes and submit a patch, but I can only test for 
one case: the Spartan 3 parallel load method.

Bruce

[U-Boot-Users] RFC: Xilinx Spartan3 relocation code

[Matthias Fuchs]

Hi Bruce,

I ran into the same issue a couple of weeks ago. Currently I implemented
dummy functions for things that I do not need. But I agree with you
that relocating NULL pointers makes no sense but fixing the FPGA code as
you suggested would be fine.

I can test your patch against the spartan2/3 serial slave code with my board.

Matthias

On Tuesday 14 August 2007 22:58, Bruce_Leonard at selinc.com wrote:
> Hi,
> 
> Our custom board has a Spartan3 FPGA on it and we're using u-boot to 
> program it.  I've come across an item (I won't call it an issue since 
> there's a fairly simple work around) in the function Spartan3_sp_reloc in 
> .../common/spartan3.c.  (Looks like the Spartan3_ss_reloc function has the 
> same issue.)
> 
> Some of the FPGA functions are optional.  That is to say the higher level 
> code checks to see if the custom function pointer is non-NULL before it 
> gets used.  The 'pre' function is a good example.  I don't have any need 
> for a 'pre' function in my particular setup, so in my 
> Xilinx_Spartan3_Slave_Parallel_fns structure I set that pointer to NULL. 
> The item comes up in the relocate functions mentioned above.  They blindly 
> add gd->reloc_offset to the function pointer values in the list, making 
> the relocated 'pre' function pointer non-NULL.  The 'fpga load' command 
> then tries to execute the 'pre' function and causes the system to crash 
> because it's trying to execute bogus code.
> 
> The simple work around is to declare a function for 'pre' that does 
> nothing.  To me this defeats the purpose of the non-NULL checking that 
> other code does and forces the user to add code that does nothing for 
> them.  I like the non-NULL checking because it's robust and allows you to 
> only include the code you actually need.
> 
> So, to the RFC: I think the code that does the relocation of the FPGA 
> description structures should check for NULL before adding 
> gd->reloc_offset to the function pointers.  I think it keeps things in 
> line with the original intent of the authors of the FPGA and makes the 
> whole thing more robust.  Comments?
> 
> I'm happy to make the changes and submit a patch, but I can only test for 
> one case: the Spartan 3 parallel load method.
> 
> Bruce
> 
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Splunk Inc.
> Still grepping through log files to find problems?  Stop.
> Now Search log events and configuration files using AJAX and a browser.
> Download your FREE copy of Splunk now >>  http://get.splunk.com/
> _______________________________________________
> U-Boot-Users mailing list
> U-Boot-Users at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/u-boot-users
> 
> 

-- 
-----------------------------------------------------------------------
Dipl.-Ing. Matthias Fuchs             esd electronic system design gmbh
http://www.esd-electronics.com                    Vahrenwalder Str. 207
phone: +49-511-37298-0, fax: -68                30165 Hannover, Germany
-----------------------------------------------------------------------

[U-Boot-Users] RFC: Xilinx Spartan3 relocation code

[Bruce_Leonard at selinc.com]

> 
> I can test your patch against the spartan2/3 serial slave code with my 
board.
> 

Matthias,

Thanks for the offer.  I'm waiting to see if there's any more interest in 
this before I submit a patch.  Not a lot of people do this, so it may be 
that no one but you and I care ;).  If I do send one in, it'll be a couple 
of days before I can get to it.

Bruce

[U-Boot-Users] RFC: Xilinx Spartan3 relocation code

[Matthias Fuchs]

Bruce,

we are using our own U-Boot FPGA boot code for Xilinx Spartan/2 FPGAs 
from board/esd/common/fpga.c for all of our current boards.
Because this code is restricted to a single FPGA I started with the 
generic FPGA code for a new design that is still on my desk.
The U-Boot port will be submitted in a couple of days.

I fixed and added some things for the generic FPGA boot. Patches will be
posted with the new board patch.

Matthias

On Wednesday 15 August 2007 19:25, Bruce_Leonard at selinc.com wrote:
> Thanks for the offer.  I'm waiting to see if there's any more interest in 
> this before I submit a patch.  Not a lot of people do this, so it may be 
> that no one but you and I care ;).  If I do send one in, it'll be a couple 
> of days before I can get to it.
> 
> Bruce
> 
> 

[U-Boot-Users] RFC: Xilinx Spartan3 relocation code

[Bruce_Leonard at selinc.com]

> we are using our own U-Boot FPGA boot code for Xilinx Spartan/2 FPGAs 
> from board/esd/common/fpga.c for all of our current boards.
> Because this code is restricted to a single FPGA I started with the 
> generic FPGA code for a new design that is still on my desk.
> The U-Boot port will be submitted in a couple of days.
> 
> I fixed and added some things for the generic FPGA boot. Patches will be
> posted with the new board patch.

I'm just using the generic FPGA code in the common directory.  Did you 
re-write stuff because things are broken?  I ask because we're having 
trouble getting the FPGA to program.  We're currently assuming that our 
interface to the FPGA is broken, but if there's known issues in the 
generic code that would be nice to know :).

Bruce

[U-Boot-Users] RFC: Xilinx Spartan3 relocation code

[Stefan Roese]

On Thursday 16 August 2007, Bruce_Leonard at selinc.com wrote:
> > I fixed and added some things for the generic FPGA boot. Patches will be
> > posted with the new board patch.
>
> I'm just using the generic FPGA code in the common directory.  Did you
> re-write stuff because things are broken?

No. IIRC, when we started implementing this esd specific FPGA booting code, 
the common FPGA code was not available yet.

> I ask because we're having 
> trouble getting the FPGA to program.  We're currently assuming that our
> interface to the FPGA is broken, but if there's known issues in the
> generic code that would be nice to know :).

Not that I'm aware of. Matthias?

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot-Users] RFC: Xilinx Spartan3 relocation code

[Matthias Fuchs]

The generic FPGA code under common is ok. Our own code is historic.
It has been written long before the generic code made it into U-Boot.
But new boards will use the generic code. Because many of our boards
have FPGAs i assume that that will be more boards using the generic code
in the future.

On Thursday 16 August 2007 18:37, Bruce_Leonard at selinc.com wrote:
> > we are using our own U-Boot FPGA boot code for Xilinx Spartan/2 FPGAs 
> > from board/esd/common/fpga.c for all of our current boards.
> > Because this code is restricted to a single FPGA I started with the 
> > generic FPGA code for a new design that is still on my desk.
> > The U-Boot port will be submitted in a couple of days.
> > 
> > I fixed and added some things for the generic FPGA boot. Patches will be
> > posted with the new board patch.
> 
> I'm just using the generic FPGA code in the common directory.  Did you 
> re-write stuff because things are broken?  I ask because we're having 
> trouble getting the FPGA to program.  We're currently assuming that our 
> interface to the FPGA is broken, but if there's known issues in the 
> generic code that would be nice to know :).
There is one thing I had to change to get the generic code for Spartan FPGAs in
slave serial mode to work:

	/* Write data */
	(*fn->wr) ((val < 0), TRUE, cookie);

The the 'val' variable in the wr-callback parameter must be signed. When compiling
with ELDK's 4.1 gcc for a 4xx CPU I got a warning that the comparision is always true.
Perhaps that is a problem on your site, too?

Matthias

[U-Boot-Users] RFC: Xilinx Spartan3 relocation code

[Bruce_Leonard at selinc.com]

Stefan,

> > > I fixed and added some things for the generic FPGA boot. Patches 
will be
> > > posted with the new board patch.

Sorry for the late reply, had a big BBQ this weekend for my anniversary 
and haven't checked my e-mail in three or four days.  Could you please let 
me know what your patches are going to look like?  I don't want to push up 
any patches that will cover/conflict things you've got in your queue.

> > trouble getting the FPGA to program.  We're currently assuming that 
our
> > interface to the FPGA is broken, but if there's known issues in the
> > generic code that would be nice to know :).

I finally figured out what stupid thing I was doing.  Both the FPGA and 
the PPC are big endian, however if the processor is little endian the data 
needs to be bit-swapped before going to the FPGA over SelectMap.  When I 
was commenting out the code to do the bit-swapping (which we don't need to 
do) I got over zealous by one line and wasn't initializing a pointer.  So 
I was sending nothing but 0x24 to the FPGA.  Wonder why it didn't program? 
:0

I'm going to try and come up with a clean way (probably a config option) 
of doing or not doing the bit-swap thing in code.  If you're running a bit 
endian system you really don't need or want to do it.  Little endian 
systems, unless you're very carefull with your HW design, you want to do 
it.  I'll have a patch for it in a couple of days.  I was thinking 
something along the lines of "#define CFG_NO_SELECTMAP_BITSWAP".  If it's 
not defined the code does the bit-swap, which is the way everything 
currently works.  Or should it be a CONFIG instead?

Bruce

[U-Boot-Users] RFC: Xilinx Spartan3 relocation code

[Matthias Fuchs]

Bruce,

what kind of bitstream file do you download to your FPGA?
A .bit File? In this case we should put the bit mirroring into the
wdata() callback or the *_sp_load() function, because the serial slave
download is fine without mirroring. Any way there is not need to malloc
a very huge amount of memory an do the mirroring on a buffer. This 
can be done on-the-fly during download.

Matthias

On Monday 20 August 2007 19:46, Bruce_Leonard at selinc.com wrote:
> Stefan,
> 
> > > > I fixed and added some things for the generic FPGA boot. Patches 
> will be
> > > > posted with the new board patch.
> 
> Sorry for the late reply, had a big BBQ this weekend for my anniversary 
> and haven't checked my e-mail in three or four days.  Could you please let 
> me know what your patches are going to look like?  I don't want to push up 
> any patches that will cover/conflict things you've got in your queue.
> 
> > > trouble getting the FPGA to program.  We're currently assuming that 
> our
> > > interface to the FPGA is broken, but if there's known issues in the
> > > generic code that would be nice to know :).
> 
> I finally figured out what stupid thing I was doing.  Both the FPGA and 
> the PPC are big endian, however if the processor is little endian the data 
> needs to be bit-swapped before going to the FPGA over SelectMap.  When I 
> was commenting out the code to do the bit-swapping (which we don't need to 
> do) I got over zealous by one line and wasn't initializing a pointer.  So 
> I was sending nothing but 0x24 to the FPGA.  Wonder why it didn't program? 
> :0
> 
> I'm going to try and come up with a clean way (probably a config option) 
> of doing or not doing the bit-swap thing in code.  If you're running a bit 
> endian system you really don't need or want to do it.  Little endian 
> systems, unless you're very carefull with your HW design, you want to do 
> it.  I'll have a patch for it in a couple of days.  I was thinking 
> something along the lines of "#define CFG_NO_SELECTMAP_BITSWAP".  If it's 
> not defined the code does the bit-swap, which is the way everything 
> currently works.  Or should it be a CONFIG instead?
> 
> Bruce
> 
> 

[U-Boot-Users] RFC: Xilinx Spartan3 relocation code

[Laurent Pinchart]

Hi Matthias,

On Tuesday 21 August 2007 10:52, Matthias Fuchs wrote:
> Bruce,
>
> what kind of bitstream file do you download to your FPGA?
> A .bit File? In this case we should put the bit mirroring into the
> wdata() callback or the *_sp_load() function, because the serial slave
> download is fine without mirroring.

You can't do that. Data should be mirrored only if they come from bitstreams 
(fpga loadb). PROM files are already mirrored. The wdata() callback would 
then have to mirror the bits only for "fpga loadb" and not for "fpga load", 
but that information is not available in the callback.

> Any way there is not need to malloc 
> a very huge amount of memory an do the mirroring on a buffer. This
> can be done on-the-fly during download.
>
> Matthias
>
> On Monday 20 August 2007 19:46, Bruce_Leonard at selinc.com wrote:
> > Stefan,
> >
> > > > > I fixed and added some things for the generic FPGA boot. Patches
> >
> > will be
> >
> > > > > posted with the new board patch.
> >
> > Sorry for the late reply, had a big BBQ this weekend for my anniversary
> > and haven't checked my e-mail in three or four days.  Could you please
> > let me know what your patches are going to look like?  I don't want to
> > push up any patches that will cover/conflict things you've got in your
> > queue.
> >
> > > > trouble getting the FPGA to program.  We're currently assuming that
> >
> > our
> >
> > > > interface to the FPGA is broken, but if there's known issues in the
> > > > generic code that would be nice to know :).
> >
> > I finally figured out what stupid thing I was doing.  Both the FPGA and
> > the PPC are big endian, however if the processor is little endian the
> > data needs to be bit-swapped before going to the FPGA over SelectMap. 
> > When I was commenting out the code to do the bit-swapping (which we don't
> > need to do) I got over zealous by one line and wasn't initializing a
> > pointer.  So I was sending nothing but 0x24 to the FPGA.  Wonder why it
> > didn't program?
> >
> > :0
> >
> > I'm going to try and come up with a clean way (probably a config option)
> > of doing or not doing the bit-swap thing in code.  If you're running a
> > bit endian system you really don't need or want to do it.  Little endian
> > systems, unless you're very carefull with your HW design, you want to do
> > it.  I'll have a patch for it in a couple of days.  I was thinking
> > something along the lines of "#define CFG_NO_SELECTMAP_BITSWAP".  If it's
> > not defined the code does the bit-swap, which is the way everything
> > currently works.  Or should it be a CONFIG instead?
> >
> > Bruce
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Splunk Inc.
> Still grepping through log files to find problems?  Stop.
> Now Search log events and configuration files using AJAX and a browser.
> Download your FREE copy of Splunk now >>  http://get.splunk.com/
> _______________________________________________
> U-Boot-Users mailing list
> U-Boot-Users at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/u-boot-users

-- 
Laurent Pinchart
CSE Semaphore Belgium

Chauss?e de Bruxelles, 732A
B-1410 Waterloo
Belgium

T +32 (2) 387 42 59
F +32 (2) 387 42 75

[U-Boot-Users] RFC: Xilinx Spartan3 relocation code

[Matthias Fuchs]

Laurent,

I think we are talking about different types of bitstreams.
That's why I ask for the image type before.

In my understanding bitstream are those files that are generated 
by xilinx' bitgen tool. Those files always have the same bit order.
You can use U-Boot's fpga load command to download them to FPGAs
in slave serial mode. The left bit is shifted out first.

For these bitstreams it is much nicer to use the loadb option, because it
pretty prints the bitstream's header.

I noticed that you can also use the xilinx promgen tool to generate a 
programming file (I do not use those files). These files can (and need not)
have a different bit order. But do they have the nice header that is 
printed by loadb? 

So what kind of images/bitstreams are you talking about? Do we really need
to support all of them?

Matthias

On Tuesday 21 August 2007 11:04, Laurent Pinchart wrote:
> Hi Matthias,
> 
> On Tuesday 21 August 2007 10:52, Matthias Fuchs wrote:
> > Bruce,
> >
> > what kind of bitstream file do you download to your FPGA?
> > A .bit File? In this case we should put the bit mirroring into the
> > wdata() callback or the *_sp_load() function, because the serial slave
> > download is fine without mirroring.
> 
> You can't do that. Data should be mirrored only if they come from bitstreams 
> (fpga loadb). PROM files are already mirrored. The wdata() callback would 
> then have to mirror the bits only for "fpga loadb" and not for "fpga load", 
> but that information is not available in the callback.
> 
> > Any way there is not need to malloc 
> > a very huge amount of memory an do the mirroring on a buffer. This
> > can be done on-the-fly during download.
> >
> > Matthias
> >
> > On Monday 20 August 2007 19:46, Bruce_Leonard at selinc.com wrote:
> > > Stefan,
> > >
> > > > > > I fixed and added some things for the generic FPGA boot. Patches
> > >
> > > will be
> > >
> > > > > > posted with the new board patch.
> > >
> > > Sorry for the late reply, had a big BBQ this weekend for my anniversary
> > > and haven't checked my e-mail in three or four days.  Could you please
> > > let me know what your patches are going to look like?  I don't want to
> > > push up any patches that will cover/conflict things you've got in your
> > > queue.
> > >
> > > > > trouble getting the FPGA to program.  We're currently assuming that
> > >
> > > our
> > >
> > > > > interface to the FPGA is broken, but if there's known issues in the
> > > > > generic code that would be nice to know :).
> > >
> > > I finally figured out what stupid thing I was doing.  Both the FPGA and
> > > the PPC are big endian, however if the processor is little endian the
> > > data needs to be bit-swapped before going to the FPGA over SelectMap. 
> > > When I was commenting out the code to do the bit-swapping (which we don't
> > > need to do) I got over zealous by one line and wasn't initializing a
> > > pointer.  So I was sending nothing but 0x24 to the FPGA.  Wonder why it
> > > didn't program?
> > >
> > > :0
> > >
> > > I'm going to try and come up with a clean way (probably a config option)
> > > of doing or not doing the bit-swap thing in code.  If you're running a
> > > bit endian system you really don't need or want to do it.  Little endian
> > > systems, unless you're very carefull with your HW design, you want to do
> > > it.  I'll have a patch for it in a couple of days.  I was thinking
> > > something along the lines of "#define CFG_NO_SELECTMAP_BITSWAP".  If it's
> > > not defined the code does the bit-swap, which is the way everything
> > > currently works.  Or should it be a CONFIG instead?
> > >
> > > Bruce
> >
> > -------------------------------------------------------------------------
> > This SF.net email is sponsored by: Splunk Inc.
> > Still grepping through log files to find problems?  Stop.
> > Now Search log events and configuration files using AJAX and a browser.
> > Download your FREE copy of Splunk now >>  http://get.splunk.com/
> > _______________________________________________
> > U-Boot-Users mailing list
> > U-Boot-Users at lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/u-boot-users
> 

[U-Boot-Users] RFC: Xilinx Spartan3 relocation code

[Bruce_Leonard at selinc.com]

Matthias Fuchs <matthias.fuchs@esd-electronics.com> wrote on 08/21/2007 
01:52:25 AM:

> Bruce,
> 
> what kind of bitstream file do you download to your FPGA?
> A .bit File? In this case we should put the bit mirroring into the

We've used a .bit file with the loadb command and a .bin file with the 
load command.  Both work fine as long as we're not bit-swapping in the 
code because we have the FPGA wired "correctly" to the processor.  I use 
the term "correctly" with some trepidateion because I'm sure some one will 
point out to me that there are lots of "correct" ways to design something 
;) .

Bruce

[U-Boot-Users] RFC: Xilinx Spartan3 relocation code

[Bruce_Leonard at selinc.com]

> In my understanding bitstream are those files that are generated 
> by xilinx' bitgen tool. Those files always have the same bit order.
> You can use U-Boot's fpga load command to download them to FPGAs
> in slave serial mode. The left bit is shifted out first.
>

And in parallel mode as well.  Just have to keep track of how the MSB on 
the processor is wired to the MSB of the FPGA to know if you need to do 
bit-swapping or not.
 
> 
> I noticed that you can also use the xilinx promgen tool to generate a 
> programming file (I do not use those files). These files can (and need 
not)
> have a different bit order. But do they have the nice header that is 

AFAIK, those programming files are targeted to Xilinx PROMs so that an 
FPGA can be booted from a PROM rather than through a processor or 
microcontroller.  I'm sure you could write code to support them, but why 
would you want to when the bitgen tool gives you something that's already 
supported by u-boot?

> So what kind of images/bitstreams are you talking about? Do we really 
need
> to support all of them?
> 

Again, I think not.  I think we should only be supporting the 
images/bitstreams that are already supported (i.e., .bit and .bin files) 
and are well documented by Xilinx.  I'm sure the PROM files are probably 
documented somewhere, but until someone has a need for them I don't see 
any reason to be adding them to the code base.

Bruce

[U-Boot-Users] RFC: Xilinx Spartan3 relocation code

[Matthias Fuchs]

On Tuesday 21 August 2007 21:39, Bruce_Leonard at selinc.com wrote:
> > what kind of bitstream file do you download to your FPGA?
> > A .bit File? In this case we should put the bit mirroring into the
> 
> We've used a .bit file with the loadb command and a .bin file with the 
> load command.  Both work fine as long as we're not bit-swapping in the 
> code because we have the FPGA wired "correctly" to the processor.  
Well, U-Boot's fpga loadb command does bit swapping. And if you want to support .bit
and prom files (e.g. .bin) you need it or you create prom files that are 
already bit swapped.

Matthias