[rtl] split executable images into package and body

[akaeba]

Hi Stephan,

at the moment i do some simulations with your processor. Therefore i need to swap high frequently the FW. Due the neorv32_application_image.vhd declaration and defintion in the same file, i got from modelsim following errors:

restart
# Loading neorv32.neorv32_package(body)
# Loading work.neorv32_tb(sim)
# Loading neorv32.neorv32_top(neorv32_top_rtl)
# Loading neorv32.neorv32_cpu(neorv32_cpu_rtl)
# Loading neorv32.neorv32_cpu_control(neorv32_cpu_control_rtl)
# Loading neorv32.neorv32_fifo(neorv32_fifo_rtl)
# Loading neorv32.neorv32_cpu_regfile(neorv32_cpu_regfile_rtl)
# Loading neorv32.neorv32_cpu_alu(neorv32_cpu_cpu_rtl)
# Loading neorv32.neorv32_cpu_cp_shifter(neorv32_cpu_cp_shifter_rtl)
# Loading neorv32.neorv32_cpu_bus(neorv32_cpu_bus_rtl)
# Loading neorv32.neorv32_busswitch(neorv32_busswitch_rtl)
# Loading neorv32.neorv32_bus_keeper(neorv32_bus_keeper_rtl)
# ** Error: (vsim-13) Recompile neorv32.neorv32_imem(neorv32_imem_rtl) because neorv32.neorv32_application_image has changed.
# Load interrupted

To avoid this, i separated the declaration from the definition (https://www.ics.uci.edu/~jmoorkan/vhdlref/packageb.html): "A constant declared in a package may be deferred. This means its value is defined in the package body. The value may be changed by re-analysing only the package body"

Feel free to comment and hopefully bring on main.

BR,
Andreas

[stnolting]

Hey there!

I'm no expert when it comes to modelsim, but I think I understand the problem (partly).

Right now we already have three VHDL files just for the IMEM:

• rtl/core/mem/neorv32_imem.default.vhd - the entity
• rtl/core/neorv32_imem.entity.vhd - the architecture
• rtl/core/neorv32_application_image.vhd - the actual memory content

Adding another file somehow feels "over the top" and I would like to avoid this. Anyway, we should fix this modelsim issue.

Can we (somehow?!) use rtl/core/mem/neorv32_imem.default.vhd to make the definitions while keeping the declaration in neorv32_application_image.vhd?

Let's try to pull in @umarcor, who is much more experienced with VHDL language issues.

[akaeba]

Hi Stephan,

Can we (somehow?!) use rtl/core/mem/neorv32_imem.default.vhd to make the definitions while keeping the declaration in neorv32_application_image.vhd?

This would also work, the only important point is, that the defintion is not recompiled because it's in the same file. If you recompile the defintion then needs the instantiating hierarchy also an recompile. Therefore i saw projects in which the entities and architectures strictly in seperate files realized. Allows changing the architecture w/o recompiling the rest of the project.

BR,
Andreas

[stnolting]

If you recompile the defintion then needs the instantiating hierarchy also an recompile.

So this is more an "improvement" rather than a problem, right?

This would also work, the only important point is, that the defintion is not recompiled because it's in the same file.

Do you have any suggestion how to implement this? 😉

[stnolting]

I have not tested that yet, but maybe this could work:

neorv32_application_image.vhd

The memory initialization image as package body:

package body neorv32_application_image is

  constant application_init_image : mem32_t := (
    00000000 => x"30005073",
    00000001 => x"80002117",
    00000002 => x"ff810113",
    ...

  );

end neorv32_application_image;

neorv32_imem.entity.vhd (append)

The actual package definition appended to the entity declaration:

-- IMEM entity declaration
-- ...

library ieee;
use ieee.std_logic_1164.all;

library neorv32;
use neorv32.neorv32_package.all;

-- declaration to enable only body compile in case of FW change
package neorv32_application_image is

constant application_init_image : mem32_t;

end neorv32_application_image;

🤔 Btw, if we decide to make this change, we should also do the same with the bootloader image.

---

edit

Another thing that came to my mind: could this make trouble with compile order?

[akaeba]

So this is more an "improvement" rather than a problem, right?

Yes.

Another thing that came to my mind: could this make trouble with compile order?

I don't think so. Because you can compile the whole project without noticing the miss of the file. The only point is, from the file names it isn't clear where this package is defined

Btw, if we decide to make this change, we should also do the same with the bootloader image.

The bootloader is also in imem?

Then i can adjust the pull request.

[stnolting]

I don't think so. Because you can compile the whole project without noticing the miss of the file. The only point is, from the file names it isn't clear where this package is defined

Thanks for clearing! 👍

The bootloader is also in imem?

Well, no, but the bootloader ROM uses the same concept in terms of memory initialization. So when you simulate the bootloader and recompile the sources you'll run into the same problem.

Then i can adjust the pull request.

That would be great! I'll convert this into a draft. Please click "ready for review" when you're done.

[akaeba]

• prototype neorv32_application_image.vhd moved to neorv32_imem.entity.vhd
• prototype neorv32_bootloader_image.vhd moved to neorv32_boot_rom.vhd
• image_gen modfied to create package bodies only
• test in my setup

In 4635402 the prototype definition needs to come first before the boot rom itself, otherwise we'll get always an compile error.

Open: I need to check on my other laptop with the complete setup. I'll drop a message when it's done.

BR,
Andreas

[akaeba]

Open: I need to check on my other laptop with the complete setup. I'll drop a message when it's done.

Done. It's working

[stnolting]

Unfortunately, this crashes GHDL: https://github.com/stnolting/neorv32/runs/6745831336?check_suite_focus=true

I am not sure if this is a GHDL bug or a VHDL language issue, but declaring neorv32_bootloader_image and actually using it in the same file seems to fail...

However, a (dirty?) workaround might be to move the neorv32_bootloader_image and neorv32_application_image package declarations to the end of neorv32_package.vhd. 🤔

[akaeba]

But it seems that only the neorv32_bootloader_image.vhd fail:

analyze ../../rtl/core/neorv32_bootloader_image.vhd
../../rtl/core/neorv32_bootloader_image.vhd:8:14: package "neorv32_bootloader_image" is obsoleted by package "neorv32_package"
package body neorv32_bootloader_image is

As first step, i would try the restore the original neorv32_bootloader_image.vhd architetcure and check if only neorv32_imem.entity.vhd architecture works. If this works, probably could a solution the same arch like the instruction memory.

[akaeba]

restore original .gitignore

thanks i forgot to unchange this.

[akaeba]

Mhh, now is something different wrong with the compile order...

../../rtl/core/neorv32_boot_rom.vhd:41:13: unit "neorv32_bootloader_image" not found in library "neorv32"
use neorv32.neorv32_bootloader_image.all; -- this file is generated by the image generator

[stnolting]

As first step, i would try the restore the original neorv32_bootloader_image.vhd architetcure and check if only neorv32_imem.entity.vhd architecture works.

This should work. However, the modifications to the image generator make the check fail as the bootloader image is also updated by the CI flow: https://github.com/stnolting/neorv32/runs/6752677586?check_suite_focus=true#step:4:423

[akaeba]

This should work. However, the modifications to the image generator make the check fail as the bootloader image is also updated by the CI flow: https://github.com/stnolting/neorv32/runs/6752677586?check_suite_focus=true#step:4:423

okay got it, this i had not in mind. Restore also the original. Now it should hopefully pass.

[stnolting]

okay got it, this i had not in mind. Restore also the original. Now it should hopefully pass.

This works again! 👍

[akaeba]

Nice 84f9007 is working :) Ho to go on? Using same approach for bootloader?

[stnolting]

Using same approach for bootloader?

I think this would require splitting the bootloader ROM into architecture-only and entity-only files. This would introduce another VHDL source file, which I would like to avoid.

We made those splitting for the IMEM because different FPGA platforms might require a different "description" of the memory (in terms of VHDL language style; the IMEM might also be read-write). In case of the bootloader ROM this is not required as it is always read-only.

What do you think about putting the neorv32_bootloader_image and neorv32_application_image package declarations solely to the end of neorv32_package.vhd? Then we would have one "central" point where those declarations are made and we could use the same body-only style for both bootloader and application images.

[akaeba]

Then we would have one "central" point where those declarations are made and we could use the same body-only style for both bootloader and application images.

If this is okay from your perspectiv, we can do this :) I prepare the next PR.

[stnolting]

If this is okay from your perspectiv, we can do this :)

It's just an idea to keep things simple 😉 But I am open for discussion.

I prepare the next PR.

No need for a new PR I think, we can modify this one.

[akaeba]

No need for a new PR I think, we can modify this one.

Yes i mean add a new commit to this PR

[stnolting]

Looks good so far!

The re-compilation issue with modelsim is resolved, right?

I'll do some synthesis tests with Quartus, Vivado and Radiant just to check we did not break anything. If everything goes fine, we can merge this I think.

[akaeba]

Hi Stephan, Looks From My Site also okay. I would say, ready for main. Thanks for collaboration.

BR,
Andreas