Do we really need wrappers for the top entity? #128
Comments
Honestly, I did never see that capability being used. I always saw all ports being explicitly assigned in the port map, either using
If the capability you explained is part of the LRM and supported by all the toolchains we are currently using (GHDL+Yosys+nextpnr, Vivado, Quartus and Radiant), then I'm good. Yet, as said, I'm unsure about it.
The main purpose of Processor templates is precisely to hide interfaces. All users can use the top entity, as long as they are willing to understand all the ports and generics (which implies understand what each peripheral is about). In fact, almost any user who is going to use NEORV32 in a "real" design is expected to write their custom ProcessorTop. Conversely, the Processor templates provided in the repo allow newcomers to understand/learn the pieces of the CPU/SoC they want to focus on. They also provide them a reference about how to structure their SoC design. Therefore, although the ProcessorTop templates might be redundant for using them in the current BoardTop examples, they are useful for users to copy, paste and modify. On the other hand, Processor templates reduce code duplication when implementing the same design on different boards. Without that intermediate abstraction, all BoardTop sources for the same design would need to repeat the configuration. They are meant for people to test new boards by copying other BoardTop sources and without going into the details of NEORV32 until they want to. I agree that some templates feel as "prebuilt processor configurations". I think that is ok. It's the purpose of Minimal and MinimalBoot. Others don't have restrictions in that regard, because their purpose is not to showcase/understand boot configuration options.
I believe we want to cover all four use cases, from less experienced to most expert.
It's not exactly about "further modules" but "further logic". ProcessorTop templates don't have any hardware requirements; all the signals defined in ProcessorTop templates have no meaning in hardware, they are all wires. Conversely, in the SystemTop_axi4lite, there are additional gates and muxes. That is, it is possible to replace a ProcessorTop with a direct instance of the NEORV32 top, but it is not possible to replace a SystemTop by the NEORV32 top, unless you copy additional logic. Therefore, Overall, I understand why you want to provide a single wrappers folder, but I believe it is an oversimplification. By having ProcessorTop and SystemTop templates we are acknowleding an inherent complexity in the design process. That is an statement: you, user, need to understand the difference between a board top (specific for some device and I/O), a design/system top (specific for the peripherals added to the NEORV32, either "internal" or "external") and a processor top (or a direct top instantiation). Then, we are all free to blur everything together in a single source. It's ok if you don't want to repeat port declarations during quick iterations for prototyping. All VHDL designers will understand that. I believe we want to avoid using the term "wrapper" for naming sources/directories and maybe in the documentation too. That's because "wrapper" is not descriptive. In a hardware design with a mostly structural description, everything is a wrapper. As soon as something is instantiated somewhere, there is a wrapper by definition. I acknowledge that "templates" might neither be the best term. I think that "something used as a pattern/reference" falls into that term. Yet, in the context of software, people might expect some code-generation (since that is the usage of the term in golang, python, etc.). Therefore, I'm good with renaming and better explaining the purpose. In some sense, those are "board agnostic instantiation examples". So, they might fit in |
It works on Quartus and Radiant. I still have to test Xilinx, but I think that (and the general concept) should be ok: https://forums.xilinx.com/t5/Design-Methodologies-and/Default-values-of-input-and-output-in-VHDL-2008/td-p/855985
I agree that for beginners the whole configuration thing might be overwhelming at first. We had that problem when setting up the FOMU tests. But I think templates like "minimal" and "minimal_boot" make things even more complicated to understand. From m point of view, the best solution would be to provide two test setups:
Both setups need explicit names and a good explanation in the user guide. If newcomers work through these two setups they should feel comfortable with the boot-concepts and also the general configuration/customization concept.
Right... I did not think about that 😅 But maybe this would be something to put in
I totally agree!
I know what you mean... For me, a wrapper is "same insides, different plug". Anyway, maybe it would be better to have |
|
I have created an experimental ✔️ I have modified the processor's top entity so that all generics are disabled by default. This works with Vivado, Quartus, Radiant and the osflow. ❓ I want to move the test setup to This is what the entities might look like (they are intended for an absolute minimum): Test Setup + Bootloaderentity neorv32_testsetup_bootloader is
generic (
-- adapt these for your setup --
CLOCK_SPEED : natural := 100000000; -- clock frequency of clk_i in Hz
MEM_INT_IMEM_SIZE : natural := 16*1024; -- size of processor-internal instruction memory in bytes
MEM_INT_DMEM_SIZE : natural := 8*1024 -- size of processor-internal data memory in bytes
);
port (
-- Global control --
clk_i : in std_ulogic := '0'; -- global clock, rising edge
rstn_i : in std_ulogic := '0'; -- global reset, low-active, async
-- GPIO --
gpio_o : out std_ulogic_vector(7 downto 0); -- parallel output
-- UART0 --
uart0_txd_o : out std_ulogic; -- UART0 send data
uart0_rxd_i : in std_ulogic := '0' -- UART0 receive data
);
end entity;Test Setup + pre-initialized IMEMentity neorv32_testsetup_approm is
generic (
-- adapt these for your setup --
CLOCK_SPEED : natural := 100000000; -- clock frequency of clk_i in Hz
MEM_INT_IMEM_SIZE : natural := 16*1024; -- size of processor-internal instruction memory in bytes
MEM_INT_DMEM_SIZE : natural := 8*1024 -- size of processor-internal data memory in bytes
);
port (
-- Global control --
clk_i : in std_ulogic := '0'; -- global clock, rising edge
rstn_i : in std_ulogic := '0'; -- global reset, low-active, async
-- GPIO --
gpio_o : out std_ulogic_vector(7 downto 0) -- parallel output
);
end entity;Maybe the second entity could by directly used as top entity for the FOMU. I am not sure what osflow would do with top entity signals that are not mapped to physical pins (I guess, it might just use any free pins). However, we could connect the lowest 3 bits of |
@stnolting, I was talking with @Paebbels about this, and he confirmed that the feature you propose is in the LRM since two decades ago! The reason it's not used much in the wild is due to good practices, not a limitation. Let me explain: default values can be provided in entities, in components and in configurations. Therefore, if those three features are mixed and the defaults do not match, it is difficult to know which one is being picked. For this reason, it is typically avoided to define defaults for input ports. In VHDL, explicit behaviour is generally preferred. Nonetheless, in the specific use case of this repository it should be safe to use the feature because we want it at the top level only, and we are not dealing with components or configurations. We also commented the following:
I believe we should apply those to all the templates/wrappers.
That is exactly what we are providing now... 1 is MinimalBoot and 2 is Minimal. Naturally, we can rename them, we can change the ports that are shown by default. However, the essence is the same.
Again, this is essentially what we have at the moment. We have two folders mkdir setups/examples/common
mv rtl/templates/processor/neorv32_ProcessorTop_stdlogic.vhd setups/examples/common/
mv rtl/templates/processor/neorv32_ProcessorTop_Test.vhd setups/examples/common/
mv rtl/templates/processor/neorv32_ProcessorTop_UP5KDemo.vhd setups/examples/common/
mv rtl/templates/system/neorv32_SystemTop_axi4lite.vhd setups/examples/common/
mv rtl/templates/processor/* rtl/
Note that the stdlogic ProcessorTop template does not have any additional logic. It's just casting between std_logic and std_ulogic. With VHDL >= 2008, those are equivalent and therefore the template is unnecessary because it is "exactly" the same as instantiating the NEORV32 top. |
Note that those minimal templates prevent users from enabling CPU extensions if they want to. It's ok not to expose the generics corresponding to the peripherals whose ports are hidden. That is, it is pointless to show the IO_TWI_EN generic if the Instead of writing those minimal templates from scratch, I suggest to try the other way: pick ProcessorTop_Minimal or ProcessorTop_MinimalBoot and do a cleanup there: remove the As a matter of fact, my first iterations of Minimal and MinimalBoot were exactly as the ones you just proposed. Most of the optional generics were added afterwards. I can argue why the CPU_EXTENSION_* generics should always be visible (optional, but not "blocked"), but I don't know about the PMP, HPM, etc. |
The problem with that approach is that you are expecting the constraints file to match the top-level ports used in the entity. That is, you are expecting all the setups (regardless of the tool) to support assigning exactly the same identifiers to the same ports, including support for arrays (or not). The structure we are using is the opposite: we want to use a single constraint file per board (for multiple designs), and we want to use a single design top-level for multiple boards. That's why we have BoardTop files. It's where we make the binding between the identifiers in the constraints file for one specific board and the ports in a design top-level (either a ProcessorTop or a SystemTop). We do typically need the BoardTop for other reasons too (such as instantiating a PLL or normalising buttons/LEDs). Therefore, it is unrealistic to use a ProcessorTop or a SystemTop as the top-level of a design. Even though it might work in very specific examples, our focus should be to stress the importance of having BoardTop files, even if they might feel unnecessary in the most simple cases. That is the knowledge that users will need as soon as they try to combine NEORV32 with anything else. |
Oh, good to know! 😅
I agree.
Very good point! I will change that here.
Right now the top uses this: assert not (CLOCK_FREQUENCY = 0) report "NEORV32 PROCESSOR CONFIG ERROR! Core clock frequency (CLOCK_FREQUENCY) not specified." severity error;
Since some synthesis tools ignore those kind of errors (looking at you Radiant!) your proposal seems more reasonable. 👍
Also a very good point! I think
Yeah, you are right 😄 Maybe its obsessive, but I just want to draw a hard line between processor-sore rtl and ready-to-put-on-your-board rtl files plus I like to have you-get-what-you-see file names here (so, for the the test setups).
Basically, this is true.
So maybe we could just delete
Right, but this is intended here. :smile These test setups are meant for beginners. They should not have to worry about something like RISC-V ISA extensions and the potential side effects. The default CPU config should just work with the default makefiles, linker script and such. If a user wants to experiment with additional ISA subsets using the test-setups there is always the option to add that specific line to the code. Of course this is not as comfortable as turning
Right... Sometimes I tend to over-simplify things... |
Moreover, IEEE Std. 1164 (which defined the
So please don't invent your own logic to mark "unused/uninitialized". It's all well defined :). by default Vivado isn't evaluating assert statements. |
I think that This is a relevant difference between VHDL and Verilog. In VHDL, the language and the designers expect things to fail as fast as possible and to be as explicit as possible about it. That is inherited from Ada and it's the main motivation for both of them to be preferred in critical applications, in comparison to Verilog/C. Naturally, VHDL/Ada are sometimes annyoing, particularly for the less critical use cases, where developers might like slightly more fluid iteration during development. However, in the mid-long term it pays off. It takes longer to get an initial feature to work, but then it's much less likely to be problematic.
I understand, but I feel you are trying to compensate a lack of documentation with overthinking. Don't take me wrong, the blame is partially on my side 😉. The distinction between ProcessorTop and BoardTop is explained easy: each of the ProcessorTop can be used for different boards, but each BoardTop is specific for one board and one design. The "you-get-what-you-see" file names are impossible to get in practice, because different users with different backgrounds will see completely different things, no matter how hard you try with the naming. For instance, there is no reason for someone not to pick the UP5K ProcessorTop template and use it on an Arty-Z7. Maybe they want to use exactly that set of peripherals, so, why not? As I commented in some other issue, you can (should) use names which don't have a meaning (planets, mountains, cities, colours...). There are two solutions to ambiguity: try to be very clear or not be clear at all. In this case, we want names not to be descriptive so that users are forced to reading, at least, the README. The README might be:
Where
Neither ProcessorTop_UP5KDemo nor ProcessorTop_Test are related to any FPGA-board specific setup. The only relation is the name. If we
Maybe... Yet, I don't feel confident to tell "we need to remove it now". I don't see who might want to use it, but that does not mean it's not useful for someone. Why was it created in first place? Which was the use case that required it?
I feel we are missing one step between being an absolute beginner and experienced enough for adding a generic to two files, copying them from a third one.
In your proposal, 2 is lost. The only options are "you care about nothing" or "you care about everything". I don't see why we need to remove that step, if should just work use case (1) is well covered in any case. Note that I'm assuming we will do the cleanup modifications to the ProcessorTop templates.
The following are two designs, to be implemented on three boards. Let's say that Uranus is a NEORV32 with Stream enabled, and the Milky Way is an hypothetical SystemTop_Streams which includes an interconnect.
We need one single ProcessorTop template and one single SystemTop. It is obvious/explicit that we are dealing with one design including the CPU only and with another design adding some extra logic around it. Now, let's remove the ProcessorTop layer:
Now users cannot tell that the three designs on the left are in fact using the same subset of NEORV32 features, and they cannot tell that the three designs on the right are wrapping that one with additional external logic (not internal features). They need to read and understand the code in order to find that out. At the same time, maintainers need to be careful because boards might easily go out of sync. |
|
Thanks for clearing!
I will try 😅 😉
I am concerned only about the input signals. I want to avoid that a setup by a lazy designer (like me!) does not work because not all of the available input ports are used in the instantiation. I agree that using For example interrupt signals: not all of them might be used for a specific setup. I think it is ok to allow the designer to simply ignore the unused one as they have a default value, that will cause no harm. Therefore, Thank you for commenting in such a detailed (and patient 😄) way. 👍 I know that a hard board-top vs processor-wrapper separation might be the most straightforward concept: the board-top is the PCB and the processor-wrapper is the black box chip you solder onto that PCB. However, these wrappers feel redundant as they are just wrappers. Hiding complexity is good here, but we could also do that by always instantiating Even though I like your idea (the table), I think that this is hard to maintain. One might ask "why is feature X included in a specific wrapper while feature Y is not"? We cannot provide wrappers for every possible configuration. And we should not provide new wrappers for every new board or every new board revision. I am afraid that there might be many several top wrappers someday: one provides PWM, one provides simple GPIO LED control, another one uses some streams... I think it is better to keep that in specific board setups: In summary I think we should not use processor-top wrappers if they are just wrappers. However, we should provide additional tops when there is actually something going on inside (you already proposed that) - so for example we have logic for converting to AXI4 or there is some kind of interconnect inside. I am ok to put them into
I think that many people use |
|
By the way, what do you think about (finally) creating a new repository |
using default for ALL generics and input signals; * generics are "off" by default * input signals are 'L' for control signals and 'U' for data signals by default
The main point is that lazyness leads to mistakes/bugs that show up later in the most unexpected and unpleasant way. Therefore, if some situation might cause crashes, the sensible solution is not to avoid it but to handle it properly.
In fact, there is a PCBTop, which wraps BoardTop. So:
However, PCBTop and BenchTop are not for synthesis (VHDL is a hardware modelling language, not a synthesis language), so I did not introduce those in this repo, in order to avoid excessive complexity.
As commented, by removing that abstraction, the information about "this same design/configuration works on all these boards" is lost. Currently, we know that "MinimalBoot works on Fomu, OrangeCrab, UPduino and iCESugar", "UP5KDemo works on Fomu and UPduino", "Minimal works on Fomu and iCESugar", etc. By changing some of the hidden settings in the MinimalBoot template, we can automatically have it tested on all the boards. Conversely, if we instantiate the neorv32_top directly, the change needs to be manually applied on all the example files. Soon, we can no longer say that one setup works on multiple boards because each of them will be specific.
I believe this is arguable. If you don't want to have equivalent setups on multiple boards, I agree, removing the abstraction makes it easier to maintain. Conversely, if that is required, the intermediate abstraction precisely reduces the maintenace burden.
You did reply to that question/demand yourself: "We cannot provide wrappers for every possible configuration. And we should not provide new wrappers for every new board or every new board revision". Therefore, we do provide some wrappers for some boards. Precisely, we provide wrappes for the boards that most active contributors can test, and we select the peripherals in the wrappers according to that subset of boards/designs that we
I see we might have exactly one wrapper where one peripheral is enabled/exposed and others are hidden. I think that is correct, because we can use them for testing that one peripheral. On the one hand, we can instantiate it in a testbench; on the other hand, we can test it on multiple boards. Apart from that, I see we might have one wrapper for "as much as we can fit in a demo for each tested device". I think that is correct too, because we are not supporting more than half a dozen different devices. The largest case (everything enabled) is what we are currently using in testbenches. Last, we have the Minimal and MinimalBoot which are meant to test the load/boot mechanisms. In some sense, these can be considered in the first group. Therefore, I think that moving them around is ok. However, if the templates are moved to the
As commented before, I feel we are mixing things here. One thing is having different levels of abstraction which match the hierarchy in the design (from a more application specific top to a more generic bottom). In those abstractions there is overlap (reuse) between simulation and synthesis; that is the case of the ProcessorTop and SystemTop templates (not the BoardTop). A different thing is moving the examples which add peripherals and maybe companion IP cores (such as the USB-to-UART or LiteDRAM) to another repository. I believe it makes sense to move the specific examples and peripherals somewhere else. It might also make sense to move the whole |
The idea of having non- uart0_txd_o : out std_ulogic; -- UART0 send data
uart0_rxd_i : in std_ulogic := 'U'; -- UART0 receive data
uart0_rts_o : out std_ulogic; -- hw flow control: UART0.RX ready to receive ("RTR"), low-active, optional
uart0_cts_i : in std_ulogic := 'L'; -- hw flow control: UART0.TX allowed to transmit, low-active, optionalIf the user (lazy me) tries to get data from the UART and If all inputs are
I agree that there should be some documentation. I think I added a sentence (maybe we need more) regarding the default values in the documentation. Btw, the registers (so the real FFs) do not require a defined initialization (CPU and whole SoC). Register that require a defined reset have a defined reset in the VHDL code. The remaining registers are initialized/reset to
You (I?) convinced me 😄 I see that we need an intermediate abstraction layer between the SoC itself an the actual board.
I like the idea having different "templates" (?) that only expose one module's peripherals and I would be ok with having this in the
In the testbench we directly use the processor top (no intermediate abstraction layer). I think it is ok to always use
This is something that I would like to replace. These two minimal setups should only focus on the boot concept (IMEM_RAM + bootloader / IMEM_ROM + pre-init-image) and thus should only use basic IO (UART and GPIO, no fancy PWM). This is what I have tried to implement in
👍
I have moved them to By the way, sorry for the copy/paste/quote chaos 😄 |
I have some doubts regarding the current concept of the processor wrapper, i.e.
rtl/templates/processorandrtl/templates/system. Currently, thertl/templates/processorfolder mainly provides wrappers, which have a simplified interface (like only exposing the UART and some GPIOs) but always do some very specific CPU/processor configuration. It feels like these are "prebuilt processor configurations" - like the ATTINY 13A-PU is a prebuilt processor configuration of the AVR family.I think it would be much cleaner to always instantiate the processor top entity
neorv32_topin all examplesetups.My proposal:
rtl/templates/processorandrtl/templates/systemfolders and all wrappers inside.rtl/wrappers) that only contains the following three wrappers:rtl/templates/processor/neorv32_ProcessorTop_Test.vhd) because this is used for the tutorial in the user guide.rtl/templates/processor/neorv32_ProcessorTop_stdlogic.vhd).rtl/templates/system/neorv32_SystemTop_axi4lite.vhd). I know this is not a wrapper in the classical definitions, but this unit does not instantiate any further modules I think this is ok.The processor top entity already provides default values for all incoming & outgoing signals. If we add 1.) then all instances of the processor only have to use the generics and signals that are actually required for the setup. For example, the instantiation of the processor in the "test setup" (
rtl/templates/processor/neorv32_ProcessorTop_Test.vhd) could be simplified to look like this:I think this is much cleaner and easier to understand. I also see no real advantage for using the current
templates/processorwrappers in the os-flow setups.Of course we can add further wrappers. But only if these wrappers really provide a feature, that is not available when using "plain instantiation" of the processor (like different interface type or different interface protocols).
What do you think? 🤔
The text was updated successfully, but these errors were encountered: