Include first/last row in bitstream generation and ConfigFSM

[mole99]

Now that #306 has been merged, we need to actually make use of the first and last row of the fabric.

Therefore this PR:

• Modifies bit_gen.py to dump the config bits for the first and last row
• Modifies ConfigFSM to write that information to the FrameData's of the first and last row
  • I tested the Verilog implementation of ConfigFSM, the VHDL version is untested but should behave the same

As a bonus, I finally added a desync frame to bit_gen.py. This means after configuration has finished, ConfigFSM will actually go back into the idle state and it is possible to upload a different bitstream without resetting the circuit.

This is shown here working:

[KelvinChung2000]

The changes logically make sense to me. But I have no knowledge in the configuration side.

[mole99]

Thanks for taking a look. I would like to add that I have successfully configured and simulated a fabric with I/Os in the top row and other tiles requiring config bits in the bottom row.

[IAmMarcelJung]

Thanks for this! See my comment for why I marked this as "changes requested". The rest looks fine so if this is resolved we can merge this.

As a sidenote, I don't like our current handling of "NumberOfRows". In fabric_gen.py the value for the generated eFPGA_top.v is set to the actual number of rows - 2. Probably this is a leftover from the previous special treatment of the top and bottom row? We should maybe think about setting this value to the actual number of rows and change all calculations done with this variable (or parameter on the Verilog/VHDL side).

[IAmMarcelJung]

I would like to add that I have successfully configured and simulated a fabric with I/Os in the top row and other tiles requiring config bits in the bottom row.

If this works, then it's probably fine, but I would rather have this double checked since this is touching core parts of the config logic :)

[mole99]

@IAmMarcelJung Sorry for the confusion, with this comment I was mainly referring to the changes in bit_gen. I used the generated bitstream for that fabric, but I wrote my own configuration logic for it.

Thus the changes to ConfigFSM were only tested with the demo fabric, therefore I would really appreciate another pair of eyes on it.

[dirk-koch]

I think making the config bits mandatory is not a good idea.
The first one is that we cannot use recent FABulous to generate bitstreams for older fabrics.
The simple solution so far was to use configuration bits in the top and bottom tiles is by adding an extra row of NULL tiles in the top and bottom rows. Not beautiful but safe.

The reason why we haven't foreseen config bits by default is that in "just eFPGA" mode, we will likely not need the top and bottom columns for something "fancy" and just do a loopback (as done, for instance, by most Xilinx FPGAs). Connections to a fabric should not be just applied arbitrary but following the bottom to top vectorization scheme in mind.
Meaning: carry chains , DSP-blocks etc. all use a connection scheme where vectors have LSBs in the bottom and MSBs at the top. This is also the reason why "main connections" sit at the left or right border of the fabric.

Anyway, a clean approach is scanning through the fabric row-by-row and count the number of configuration bits needed (after we generated all tiles) and then decide for each row it it needs configuration bits. We could do that with thresholds for, let's say 8, 16, 32 FrameData bits.
This way, our old fabrics would still work as usual and nothing should break.
Additionally, we could also foresee setting an optional variable to set the FrameData bits manually. That would still require a feasibility check before generating the fabric.
There are changes to the configuration logic planned that will be rolled out (hopefully) soon and we will definitely look into the first/last row issue.

[mole99]

@dirk-koch Thank you for your reply. I understand your points, but I do not fully agree with them:

The first one is that we cannot use recent FABulous to generate bitstreams for older fabrics.

There is an easy way to still be able to generate bitstreams for older FABulous fabrics: by simply adding a "legacy" mode to bit_gen. If --legacy is passed, then the config bits of the first and last row are not output to the bitstream and the bitstream is therefore backwards compatible.

I can add that change to this PR right away.

The simple solution so far was to use configuration bits in the top and bottom tiles is by adding an extra row of NULL tiles in the top and bottom rows. Not beautiful but safe.

That workaround wasn't directly obvious to me and probably to other users of FABulous either. Aside from not being pretty, as you mentioned, it wouldn't work well with my automatic stitching approach as the row of NULL-tiles would add empty space to the fabric macro.

The reason why we haven't foreseen config bits by default is that in "just eFPGA" mode, we will likely not need the top and bottom columns for something "fancy" and just do a loopback (as done, for instance, by most Xilinx FPGAs). Connections to a fabric should not be just applied arbitrary but following the bottom to top vectorization scheme in mind.

Yes, I understand that in some situations you don't need configuration bits in the top and bottom row. But what if you need them, as in my case?

Meaning: carry chains , DSP-blocks etc. all use a connection scheme where vectors have LSBs in the bottom and MSBs at the top. This is also the reason why "main connections" sit at the left or right border of the fabric.

True, but in my fabric I am already pretty tile-space limited and I just would like to add some simple ADC or DAC tiles at the bottom. I know, it might not be best practice, but nextpnr can succesfully route my user design and that's good enough for me.

---

I understand why FABulous introduced the exception for the top and bottom row, but I strongly believe that it creates more issues than it solves. The current exception is not a generalised solution (one could even call it a hack). It already cost me a lot of time to work around a lot of issues caused by it. It needlessly complicates many of the subsystems in FABulous such as the fabric description, fabric generation, bitstream generation and even the configuration FSM. Exceptions have to be made everywhere just because these two rows must not have configuration bits.

That's why I worked on removing these exceptions everywhere, making sure the fabric is still generated correctly, and in turn making the code easier to read and therefore making it easier for people to contribute to FABulous.

With these changes users have the choice of whether they want any config bits in those rows or not. If they don't use any config bits in those rows, that's fine too. Just implement a loopback and the physical macros can be very small.

Hopefully soon can be a long time, and there are issues caused by the current implementation right now, so I urge you too reconsider these changes that improve the current situation.

If you like, we can have a short call where I can show you my current fabric and explain in detail why these changes are important to me and for the FABulous project.
Leo

[mole99]

As discussed with @dirk-koch, we proceed by adding a note to the CLI startup message regarding the legacy mode:

...

To run the complete FABulous flow with the default project, run the following command:
    load_fabric
    run_FABulous_fabric
    run_FABulous_bitstream ./user_design/sequential_16bit_en.v
    run_simulation fst ./user_design/sequential_16bit_en.bin

To generate a bitstream for a legacy FABulous 1.0 fabric,
pass --legacy to run_FABulous_bitstream.

To generate a bitstream in legacy mode, simply invoke:

run_FABulous_bitstream ./user_design/sequential_16bit_en.v --legacy

This will additionally output the following info during bitstream generation:

2025-02-12 09:08:58.425 | INFO     | FABulous.fabric_cad.bit_gen:genBitstream:128 - Legacy FABulous 1.0 bitstream generation enabled.

@KelvinChung2000 @IAmMarcelJung I would appreciate if you could re-review the changes and if everything is ok we can merge them :)

[EverythingElseWasAlreadyTaken]

Hey @mole99,
Thanks for your work!
I haven't been involved in that topic, but I followed the discussion about it and I also think this is the right way to go!

Just wanted to mention, that we also should add a note to the README about the legacy mode and also some documentation about it.

We might also need to update the docs regarding these changes, but this does not need to happen in this PR.
I think most of the stuff you touched was not really documented in the first place, but we should at least add an Issue to keep track of blind spots in the docs.

[mole99]

@EverythingElseWasAlreadyTaken thanks, I appreciate it!

I've added a note about legacy bitstream generation to the README (and removed an outdated mention of npnr).
To keep track of any needed documentation, I opened an issue here: #323

[IAmMarcelJung]

Thanks for your changes! :) I will try to look after them this evening, but can't promise anything right now.

[IAmMarcelJung]

So I looked and it and tried it out and everything looks good :) Except for that one thing I mentioned in the comment, which still needs to be resolved or acknowledged.
Special thanks for spotting and removing the deprecated npnr flag in the README!

[mole99]

Hi @IAmMarcelJung, sorry for my delayed reply!

I had a closer look at the top-level integration and found an issue with the current implementation: I didn't realize that the top-level only generates Frame_Data_Regs for all rows except the top and bottom one, those would always receive 0x12345678 for their FrameData. This would prevent the config bits for the top and bottom rows from actually reaching the tiles.
So I fixed that up, ran the simulation and verified that the frames are correctly loaded.

Regarding your question whether the RowSelect being 0x1F at the same time as a WriteStrobe is an issue: that is not the case. The WriteStrobe is actually not responsible for writing a new value into the Frame_Data_Regs, it only notifies the ConfigFSM that a new WriteData is available. The Frame_Data_Regs only react to the RowSelect signal, that's why it is set to 0x1F while waiting for new WriteData: 0x1F is an invalid frame and therefore does not change anything. Here's the code snippet:

    always @ (*) begin
        if(WriteStrobe) begin // if writing active
            RowSelect = FrameShiftState; // we write the frame
        end else begin
            RowSelect = {RowSelectWidth{1'b1}}; //otherwise, we write an invalid frame
        end
    end

You could prevent FrameShiftState from going to 0x1F during a WriteStrobe if you only decrement when != 0, but this is not necessary as explained above.

All in all, I think the whole configuration logic needs a major overhaul at some point in time, as it's hard to read and hard to debug, but that's beyond the scope of this PR... :)

[KelvinChung2000]

I agree. I happen to be working on it now, and it is unclear how they all work together.

Creating a standard specification for frame-based configuration bitstream might be a good idea. Like AXI, it tells you how it should behave but not how it should be implemented.

[IAmMarcelJung]

Hi @mole99, thanks, I did just briefly read what you wrote and it makes sense to me, but I will still try to take a closer look with your explanations this week :) Hope this is fine or do you need this merged asap?

[mole99]

@IAmMarcelJung No hurry, this or next week sounds fine :)

[IAmMarcelJung]

Hi @mole99,

so I took a closer look into how the configuration works and as far as my understanding goes it is indeed fine to have the RowSelect signal at 0x1F while writing new data.

However I couldn't fully follow your explanation. You wrote

The WriteStrobe is actually not responsible for writing a new value into the Frame_Data_Regs, it only notifies the ConfigFSM that a new WriteData is available. The Frame_Data_Regs only react to the RowSelect signal, that's why it is set to 0x1F while waiting for new WriteData: 0x1F is an invalid frame and therefore does not change anything.

This is a bit contradictory to me, since a WriteStrobe should signalize that new WriteData is available or did I get this wrong? So if both WriteStrobe is 1 and FrameShiftReg is 0x1F, RowSelect will be set to 0x1F for valid data too, not only during the "wait state", where it is usually 0x1F as you pointed out, right?

Anyway, this one occurence of WriteData == 1 and RowSelect == 0x1F just replaces what was previously the row 0, when it had no config bits and the WriteData also had no effect. That being said, should we consider to reset the FrameShiftState to {RowSelectWidth{1'b1}} instead of 0, since otherwise the bitstream header is written into row 0? This should not really be an issue since the data should be overwritten again during configuration, but just from a logical point of view it might make sense to reset to this invalid frame state. What do you think?

I hope my explanations make a bit sense and are somewhat understandable, although it was a bit hard to describe what I actually mean. Feel free to ask if anything is unclear :) But the tl;dr is that I'm now also convinced that your changes will not cause issues, if my understanding is correct!

[mole99]

Hi @IAmMarcelJung, let me try to clarify!

This is a bit contradictory to me, since a WriteStrobe should signalize that new WriteData is available or did I get this wrong?

That is almost correct. WriteStrobe only notifies the ConfigFSM that new data is available (e.g. from the bitbang or uart interface), but it is not responsible for writing to the Frame_Data_Regs.

If we take a look at the Frame_Data_Reg module, it is clear that it is only sensitive to RowSelect, there is no strobe signal:

module Frame_Data_Reg (FrameData_I, FrameData_O, RowSelect, CLK);
    parameter FrameBitsPerRow = 32;
    parameter RowSelectWidth = 5;
    parameter Row = 1;
    input [FrameBitsPerRow-1:0] FrameData_I;
    output reg [FrameBitsPerRow-1:0] FrameData_O;
    input [RowSelectWidth-1:0] RowSelect;
    input CLK;

    always @ (posedge CLK) begin
        if (RowSelect==Row)
            FrameData_O <= FrameData_I;
    end//CLK
endmodule

If RowSelect matches a specific instance of Frame_Data_Reg, then the data is written. That's why when FrameShiftReg is 0x1F no data is written, because no Frame_Data_Reg is selected.

That being said, should we consider to reset the FrameShiftState to {RowSelectWidth{1'b1}} instead of 0, since otherwise the bitstream header is written into row 0? This should not really be an issue since the data should be overwritten again during configuration, but just from a logical point of view it might make sense to reset to this invalid frame state. What do you think?

Absolutely, resetting FrameShiftState to {RowSelectWidth{1'b1}} would actually make more sense to prevent writing all of the metadata and 0xFAB0FAB1 and the bitstream header into row 0. But as you mentioned it does not matter as it will be overwritten anyways.

I wouldn't change the current implementation too much until a proper rewrite. I think it would make more sense for each Frame_Data_Reg to also have a dedicated strobe signal. Of course that's a bit more logic, but the synthesis tool can apply clock gating to those regs and it will be quite efficient. The advantage is that we don't need to rely on an invalid Frame_Data_Reg address and it is clear at which point actual data is written.

I'm currently working on an alternative config controller for my fabric, you can check it out once it's ready :)

[IAmMarcelJung]

Hi @mole99,

thanks again for your clarifications!

If RowSelect matches a specific instance of Frame_Data_Reg, then the data is written. That's why when FrameShiftReg is 0x1F no data is written, because no Frame_Data_Reg is selected.

I know and my doubt was never that wrong data is written by matching to 0x1F, but that rather that there was actually data at this occasion that should be written somewhere else. I think I never made this clear, sorry!

I wouldn't change the current implementation too much until a proper rewrite. I think it would make more sense for each Frame_Data_Reg to also have a dedicated strobe signal. Of course that's a bit more logic, but the synthesis tool can apply clock gating to those regs and it will be quite efficient. The advantage is that we don't need to rely on an invalid Frame_Data_Reg address and it is clear at which point actual data is written.

Yes you're right, we shouldn't change too much here. @dirk-koch also has plans to change the config logic, maybe he already told you that.

So from my side everything looks fine now and I will unblock this :) However, I leave it up to @KelvinChung2000 to decide when this will be merged since he did the whole rebasing and I don't want to cross his plans with the release.

Thanks you so much for your contribution and the constructive discussion!

[mole99]

@IAmMarcelJung I don't know the specifics on what Dirk wants to change. Perhaps the bitstream format could be overhauled too, it feels a bit awkward. At the moment the header looks as follows:

32-bit header for demo fabric:

| 5-bit column select | ... unused ... | 20th bit sync | 20-bit frame strobe |

The column select is the width of frameSelectWidth, which is currently hardcoded to 5, meaning that fabrics can only every be 32 columns wide. This can easily be fixed by calculating frameSelectWidth based on numberOfColumns. The 20-bit frame strobe can be customized with maxFramesPerCol, but the sync bit is currently hardcoded to bit 20. Also, support for initialising BRAMs would be great. So maybe it could be reworked with future extensions/features in mind.

But that's for another time ;)

---

It would be great if these changes could be included in the next release since my fabric builds on them. Or do you mean we simply merge this after #324? That's fine for me, just let me know if I need to rebase this PR.

Thanks a lot for your extensive code reviews!

[KelvinChung2000]

Once we make the current master stable and relaese, we will move the development to the main. It will not call a release, but it will be on master, at least.

Did #325 state all the current limitations of the bitstream format? I think we can move the conversation about the new bitstream requirement there.

[mole99]

@KelvinChung2000 Alright, that sounds fine! Btw. could you rename the development branch to just dev afterwards? That's more common with software projects.

I've added a few comments to #325 👍