wzab / agwb Goto Github PK

Right now addr_gen_wb generates constants in 3 different ways: without any prefix, with c_ prefix, with C_ prefix. It would be good to unify formatting of constants names. I suggest using C_.

The wb_block.py is getting long as a single file. It would be good to split some functionalities. For example for different backends there can be backend package with single module per targets such as Python, VHDL etc.

Currently auto generated VHDL functions converting to std_logic_vector type have following format {type_name}2stlv.
This is superfluous clutter. VHDL allows ad hoc polymorphism via function overloading.

• Functions converting to std_logic_vector should be named to_slv. The same way it is done in the standard libraries.

• The same applies to the functions converting to the custom types. Instead of stlv2{type_name} it should be to_{type_name}.

When I try to generate the Wishbone fabric containing only a single blackbox,like below:

<sysdef top="MAIN">
<constant name="NEXTERNS" val="4" />
<constant name="NSEL_BITS" val="3" />
<constant name="NSEL_MAX" val="(1 &lt;&lt; NSEL_BITS)-1" />
<!-- include block1.xml -->
<block name="MAIN">
  <blackbox name="EXTHUGE" type="HTEST" addrbits="16" />
</block>
</sysdef>

it results in an error:

ghdl -e -g --mb-comments  --std=93c -fexplicit --ieee=standard wb_test_top_tb ./wb_test_top_tb --unbuffered --stop-time=5000ns --wave=wb_test_top_tb.ghw  
general-cores/modules/wishbone/wb_crossbar/xwb_crossbar.vhd:107:7:@0ms:(assertion failure): Address space smaller than a wishbone register; slave #0[0x10000/0x1fffe]
./wb_test_top_tb:error: assertion failed
./wb_test_top_tb:error: error during elaboration
make: *** [Makefile:43: wb_test_top_tb.ghw] Error 1

The version that shows the problem is available via tag demo-bug1 in the repository, in directory test.

Adding just a single additional (in addition to VER & ID) register cures the problem.

Even if custom register type occupies less than 32 bits, conversion function returns std_logic_vector of length 32. Example:

type t_CTRL is record                                                                                                                                                                                          
  ENABLE:std_logic_vector(0 downto 0);                                                                                                                                                                      
  RX_WIDEBUS_MODE:std_logic_vector(0 downto 0);                                                                                                                                                              
  INIT_VALUE:std_logic_vector(19 downto 0);                                                                                                                                                            
end record;

function t_CTRL2stlv(x : t_CTRL) return std_logic_vector is                                                                                                                                                    
  variable res : std_logic_vector(31 downto 0);                                                                                                                                                                  
begin                                                                                                                                                                                                    
    res := (others => '0');                                                                                                                                                                                  
    res(0 downto 0) := std_logic_vector(x.ENABLE);                                                                                                                                                              
    res(1 downto 1) := std_logic_vector(x.RX_WIDEBUS_MODE);                                                                                                                                                       
    res(21 downto 2) := std_logic_vector(x.INIT_VALUE);                                                                                                                                                           
    return res;                                                                                                                                                                                                  
end t_CTRL2stlv;

There is no constant that would define how many bits are needed for custom record types. The only way to get this width right now is to define a signal, convert it to the std_logic_vector and use predefined 'length attribute, but this method always return 32 as all t_*2stlv() functions return 32 bits long std_logic_vector.

Why is it a problem?
In case of CDC 2 attributes need to be defined for signals keep and async_reg.
Xilinx Vivado is not able to remove unused signals during synthesis.

In bigger designs, the decoding of addresses and routing of data may require long combinational paths and low bus clock frequency.
The user should be able to decide, whether he wants to reduce the bus clock frequency, or to add the pipeline registers, to keep the bus clock frequency high, but to increase the number of cycles needed too complete the access.
The general-cores xwb_crossbar offers the "g_registered" generic - https://ohwr.org/project/general-cores/blob/96630ec6636b3ea17b240b6c398a1cdf6c761028/modules/wishbone/wb_crossbar/xwb_crossbar.vhd#L55 .

It would be good to have this generic supported by the AGWB, so that the user may specify whether the crossbar should be implemented as "registered" (shorter critical path, but more clock pulses needed to complete access) or "unregistered" (less number of clock pulses, but longer critical path).

If somebody sets the default value to a bitfield in the vector of control registers, the initialization code is generated incorrectly.
Many thanks to Esteban Rubio from CBM team for finding that bug.
The problem may be recreated in the "test" demo design by setting the repetition count to "1" and adding a default value to one of the bitfields in X2 register in block1.xml:

  <creg name="X2" stb="1" reps="1">
    <field name="B1" width="1" desc="Start the operation" trigger="1" />
    <field name="B2" width="1" desc="Start the operation" default="0" />
    <field name="B3" width="1" desc="Start the operation" trigger="1" />....
  </creg>

Then we get the following error in GHDL compilation:

ghdl -a -g --work=general_cores -C  --std=93c --ieee=standard general-cores/modules/wishbone/wb_register/xwb_register.vhd
ghdl -a -g --work=agwb -C  --std=93c --ieee=standard gen/agwb_pkg.vhd
ghdl -a -g --work=agwb -C  --std=93c --ieee=standard gen/MAIN_const_pkg.vhd
ghdl -a -g --work=agwb -C  --std=93c --ieee=standard gen/SYS1_pkg.vhd
ghdl -a -g --work=agwb -C  --std=93c --ieee=standard gen/SYS1.vhd
gen/SYS1.vhd:44:60:error: can't match function call with type array type "ut_x2_array"
  signal int_X2_o : ut_X2_array(g_X2_size - 1 downto 0) := to_X2(std_logic_vector(to_unsigned(0,3))); -- Hex value: 0x0
                                                           ^
gen/SYS1.vhd:122:21:error: can't match function call with type array type "ut_x2_array"
        int_X2_o <= to_X2(std_logic_vector(to_unsigned(0,3))); -- Hex value: 0x0
                    ^
ghdl:error: compilation error

The problem does not occur if the number of repetitions (reps) is higher than 1.

Current code of AGWB tends to use downto when indexing bit vectors and to for arrays.

In the code that we are using it is handled in different way. In GBT-FPGA they often use the "old" convention.
In general-cores there are different cases. Just take a look at:

https://ohwr.org/project/general-cores/blob/96630ec6636b3ea17b240b6c398a1cdf6c761028/modules/wishbone/wb_crossbar/xwb_crossbar.vhd#L65

and

https://ohwr.org/project/general-cores/blob/96630ec6636b3ea17b240b6c398a1cdf6c761028/modules/wishbone/wb_bus_fanout/xwb_bus_fanout.vhd#L22

(Both are from the same version!)

Now @m-kru suggests, that we shoud clean it up. So we are switching to using downto, wherever possible,

It looks like

  res := (others => '0');

is superfluous as we exactly know the length and assign value to all bits.

That functionality has been requested by @wfjm as the result of usage of AGWB for preparation of the CBM firmware.
The idea is that at well known positions the following registers should be located:

• TEST_RW - normal read/write register.
• TEST_WO - the write only register. Writing stores the data that may be read via the next register. Reading generates an error.
• TEST_RO - the read only register. Reading returns the last data written to TEST_WO.
• TEST_TOUT - any access to that register generates the bus timeout.

The implementation may be relatively simple and requires just simple extension of the code generation template:

The feature should be optional (so an additional attribute, e.g. testdev_ena="1" should be added).
Depending on it, the address of the first free register should be adjusted in:

For example in test directory, after generating files, in gen directory there is a file SYS1_pkg.vhd. However, the name of the package that it defines is package SYS1_wb_pkg is.
It is then easy to make a mistake in another VHDL file by typing:
use work.SYS1_pkg.all;
instead of
use work.SYS1_wb_pkg.all;

Sometimes it may be useful to automatically generated a vector of registers for register with width > 32.
Of course, that would be a dangerous feature. In case if somebody would like that feature to access wide registers in logic, there would be a necessity to generate external write strobe (so that the whole register contents is assembled before the strobe is generates). Similar problem could happen when somebody wants to read the wide register. The whole register must be latched, before it is read via multiple bus registers.
Another scenario is needed when the bits are just certain status or control bits for multiple blocks in the user logic...

The implementation and syntax should be well planned to avoid confusions...

Right now active reset value is hard coded to '0'.
if rst_n_i = '0' then
I think it would be nice to be able to generate code with active reset value '1'.

When instantiating top class generated by the agwb user needs to provide value for base parameter. base is the base address. The value is almost always 0. However, it would be valuable to document somewhere when the value may differ from 0.

During the final tests, I have found a strange thing in the GHDL.
It looks like during the elaboration it attempts to perform all assignments?

When using the "test" design (from commit f42e076 ), I get the following error:

general-cores/modules/wishbone/wb_crossbar/xwb_crossbar.vhd:114:7:@0ms:(report note): Mapping slave #33[0xf800/0x1fc00]
general-cores/modules/wishbone/wb_crossbar/xwb_crossbar.vhd:114:7:@0ms:(report note): Mapping slave #34[0xf400/0x1fc00]
general-cores/modules/wishbone/wb_crossbar/xwb_crossbar.vhd:114:7:@0ms:(report note): Mapping slave #35[0xf000/0x1fc00]
general-cores/modules/wishbone/wb_crossbar/xwb_crossbar.vhd:114:7:@0ms:(report note): Mapping slave #36[0x10000/0x10000]
general-cores/modules/wishbone/wb_crossbar/xwb_crossbar.vhd:114:7:@0ms:(report note): Mapping slave #0[0x0/0x0]
./wb_test_top_tb:error: index (9) out of bounds (0 to 8) at gen/SYS1.vhd:216
in process .wb_test_top_tb(test).dut@wb_test_top(simul).main_1@main(rtl).gl1(0).sys1_1@sys1(rtl).sys1_1@sys1(gener).P6
./wb_test_top_tb:error: error during elaboration

I have checked line 216:

It shouldn't be excuted, as the generic g_ENABLEs_size is set to 9!

So I have modified the generated SYS1.vhd:

Now that line should be never executed!
After compilation and starting the simulation, I get:

general-cores/modules/wishbone/wb_crossbar/xwb_crossbar.vhd:114:7:@0ms:(report note): Mapping slave #34[0xf400/0x1fc00]
general-cores/modules/wishbone/wb_crossbar/xwb_crossbar.vhd:114:7:@0ms:(report note): Mapping slave #35[0xf000/0x1fc00]
general-cores/modules/wishbone/wb_crossbar/xwb_crossbar.vhd:114:7:@0ms:(report note): Mapping slave #36[0x10000/0x10000]
general-cores/modules/wishbone/wb_crossbar/xwb_crossbar.vhd:114:7:@0ms:(report note): Mapping slave #0[0x0/0x0]
./wb_test_top_tb:error: index (9) out of bounds (0 to 8) at gen/SYS1.vhd:216
in process .wb_test_top_tb(test).dut@wb_test_top(simul).main_1@main(rtl).gl1(0).sys1_1@sys1(rtl).sys1_1@sys1(gener).P6
./wb_test_top_tb:error: error during elaboration

The error occurs during the elaboration, at time 0.
Of course, I can use the for loop instead of if's. It makes the generated VHDL much smaller, but less legible. Anyway the fact that generics and if (in the process, it works for if-generate) do not allow skipping the non-existing parts of an array is a little annoying.

I have verified, that the problem does not occur in the earlier version of GHDL.

Current implementation of AGWB has problems with FPGAs hosting e.g. multiple PCIe devices (an example may be huge Xilinx FPGA with two SLRs).
In that case we may have two independent and slightly different WB-based devices, which must be implemented in the same device. The simplest solution would be to prepare two separate top blocks with two different sets of parameters, but that may result in generation of entities with the same names and e.g. different types of ports.
This could be solved, by allowing the user to specify the name of the library generated for each top (e.g. instead of "agwb" in both cases, "agwb_slr1" for the first top, and "agwb_slr2" for the second top.
That solution however generates problems related to interfacing with the user logic, because the same entity can't include either one or the another library depending on generics (which could pass the SLR number).

As in title.

Currently, the generated HTML is the same independently of whether the particular name corresponds to a single object (block or register) or vector of objects.
The displayed path should be supplemented with [N] for vectors.
Additionally, the length of the vector should be shown.

Current CDC block https://github.com/wzab/agwb/blob/9070a2d2f3c2fafb12ff9276e5b36311eea217b9/hdl/wb_cdc/wb_cdc.vhd is optimized for transfering single accesses between domains driven by asynchronous clocks.
Often it is necessary to transfer accesses between domains where the clocks are synchronous, and the first one has a frequency N times higher then the second one: f_CLK1 = N * f_CLK2 .
In such a case the domain crossing circuit may be significantly simpler. It is necessary to design the CDC for both possible cases:

• f_CLKmaster = N * f_CLKslave
• f_CLKslave = N * f_CLKmaster

As @wfjm suggested, the AGWB should detect incorrect attributes.
The standard solution would be to define the XML schema for the system description, describing what attributes may be used in which nodes.
Unfortunately, the standard Python xml.etree.ElementTree and xml.parsers.expat modules do not support schema validation. We will need to use additionally lxml library - https://stackoverflow.com/questions/57073834/how-to-validate-the-xml-with-dtd-using-python-xml-etree-elementtree-library , https://stackoverflow.com/questions/8943407/validate-xml-document-with-expat
Probably we should use additional library https://lxml.de/validation.html for that purpose.

The open question is if we should use it in addition to the currently used ones, or instead of them.
Now we have quite sophisticated system to identify syntax errors in the combined XML connected with backtracking of the origin of the erroneous line (see

There is no concept of block parameter. This is to some extent emulated with using global constants within block declarations.
However, using the same block within more than single reps="N;M" with different value of a constant is not possible.
Block should have parameters, and the value should be passed during block instantiation.

Hi,
First of all, thank you for this generator!
I think it would be really nice to have the generation process generate the register address mapping in VHDL as constants in a package, and not as magic values directly in the interconnect files. Is it something that has been done somewhere? I tried to look around with no avail, but some branch named "fixed_addresses" exists, so I'm asking before trying to reimplement anything :)

Thanks!

Theoretically the current header-based backend for C may be reused for C++. The only thing that should be improved is wrapping the inconvenient bitfield-access macros with elegant C++ methods.

However, when connecting the AGWB-described control-tree to a C++ software it is likely that the end user wants to avoid recompilation of the software whenever the hardware is modified. Therefore a representation capable of resolving the addresses at runtime is needed.

My first idea is to imitate the behaviour or the current Python backend.
The AGWB output should provide the set of classes reflecting the addressable objects:

• Registers and registers with bitfields
• Vectors of registers
• Blocks (which may contain registers and/or vectors of registers)
• Vectors of blocks

The convenient feature of the Python backend is that the hierarchy of the control-tree is fully reflected by the hierarchy of objects.
Of course with run-time resolved description we can't dereference the objects like in Python, by including the names of objects in the code:

blockA.blocksB[nrb].blocksC[nrc].regsA[nrA].bitfieldA

In case if certain node in the control-tree hierarchy is supposed to be often used for looking up its children, we should be able to keep a reference to it.
For run-time dereferencing we should pass the above path as the argument to the e.g. resolve function of the addressable object from which we start looking for a child.
The resolve function takes two arguments: offset and path.

• For each node the following information is stored:
  • Information whether it is a vector objects (blocks or registers)
  • Offset (for non-vectors - its offset from the base address of its parrent, for vectors - the offset of the first element of the base address of the parrent)
  • Size of the element (only for vectors)
  • number of elements (only for vectors)
• The resolve function works as follows
  • If the path starts with "[", it is verified that the current node is a vector (if not, an exception is thrown). Then the number of the selected element is extracted (until "]") and hence its offset is calculated .
  • The path is parsed using "." and "[" as a separator.
  • The extracted token is used as a identifier of a child.
  • The resolve method of the child is called, until we reach the end of path (or we reach the register with bitfields, because bitfields must be treated in a special way).

An important issue is how we can efficiently handle dynamic indexes when accessing an object.
At the moment the only way to do that is to get the reference to the vector object (blockA.blocksB in the above example), then use its [] method to get the reference to the selected element, and repeat the above at each index.

Another possibility is to use the approach implemented in the Forth backend. We should:

• provide the vector of indices as the third argument of the resolve function,
• or add two additional arguments at the end: the pointer to the list of indices and the number of indices.

Whenever "[ index ]" is found in the path, we consume one index and locate the appropriate element in the vector, and pass the rest of the list of indices to the resolve function of that element.

This is the first description of the problem and possible solution.
Any comments and suggestions are appreciated.

Should be

    control_stb_o : out std_logic;

A lot of VHDL code is printed to the stdout during generation. It would be much easier to find errors in long build chains if addr_gen_wb didn't print VHDL to stdout.

Following xml definition has no effect in terms of default values.

<creg name="CONFIG">                                                                                                                                                                                        
     <field name="RX_WIDEBUS_MODE" width="1" default="0x0"/>                                                                                                                                                   
     <field name="INIT_VALUE" width="20" default="0x0"/>                                                                                                                                                        
</creg>

The AGWB has been equipped (in commits 5b75daa to 10c1c27 ) with a system for detection of erroneous XML description.
It is based on lxml module.
Usually it works correctly ("creeg" used instead of "creg"):

<string>:6:0:ERROR:RELAXNGV:RELAXNG_ERR_EXTRACONTENT: Element block has extra content: creeg
<string>:0:0:ERROR:RELAXNGV:RELAXNG_ERR_INTEREXTRA: Extra element block in interleave
<string>:5:0:ERROR:RELAXNGV:RELAXNG_ERR_CONTENTVALID: Element sysdef failed to validate content
  <creeg name="CTRL" desc="Control register" stb="1" used="1;0">
|
The erroneous line was produced from the following sources:
file: block1.xml, line:2

But sometimes, for nested errors it generates incorrect error information.
E.g., for the following erroneous definition ("wideth" instead of "width" in one of field definitions):

<block name="SYS1">
  <creg name="CTRL" desc="Control register" stb="1" used="1;0">
    <field name="START" width="1" desc="Start the operation"/>
    <field name="SPEED" wideth="4" default="-1" type="signed" desc="Transmission speed"/>
    <field name="STOP" width="1" desc="Stop the operation" />
  </creg>
  <creg name="ENABLEs" desc="Link enable registers" reps="9;10" default="0x0"/>
  <sreg name="STATUS" desc="Status register" ack="1" />
</block>

it reports:

<string>:7:0:ERROR:RELAXNGV:RELAXNG_ERR_EXTRACONTENT: Element creg has extra content: field
<string>:0:0:ERROR:RELAXNGV:RELAXNG_ERR_INTEREXTRA: Extra element creg in interleave
<string>:6:0:ERROR:RELAXNGV:RELAXNG_ERR_CONTENTVALID: Element block failed to validate content
<string>:0:0:ERROR:RELAXNGV:RELAXNG_ERR_INTEREXTRA: Extra element block in interleave
<string>:5:0:ERROR:RELAXNGV:RELAXNG_ERR_CONTENTVALID: Element sysdef failed to validate content
    <field name="START" width="1" desc="Start the operation"/>
|
The erroneous line was produced from the following sources:
file: block1.xml, line:3

So it complains about the first field definition in case if any field definition is incorrect.
It must be investigated if it is a bug in the lxml module, incorrect RelaxNG definition or improper usage of the validation.

subblock currently instantiates given block, however the instantiated block has the name of the block type, not the name of the subblock. Treating subblock elements as a block instantiation would allow instantiating the same block multiple times with different parameters.

An instantiation of a block that doesn't contain a status or control register causes the program to fail.

Example xml:

<sysdef top="main">

  <block name="b1">
    <creg   name="placeholder" desc="dummy"/>
  </block>

  <block name="main">
    <!-- <sreg   name="p2laceholder" desc="dummy"/> -->
    <subblock name="b1" type="b1"/>
  </block>


</sysdef>

The program works fine when the comment is removed.

As VHDL files are generated to a separate agwb library there is no longer a need to add agwb_ prefix to .vhd files and to all units names such as entities names or packages names. This is now superfluous clutter.

Up to now the Python backend always produces the maximum version
It seems reasonable to provide the variants support.
The easiest implementation may be the addition of variant argument to the constructor of the Block class.
It is then used by other objects.
If variant is None, the maximum version is implemented. Otherwise it must be a valid variant number.
Availability of subblocks and registers will be then dependent on the number of the variant.

Right now such scenarios in *_wb.vhd files are handled in the following way:

case int_addr is
...
...
...
when others =>
    int_regs_wb_m_i.dat <= x"A5A5A5A5";
    int_regs_wb_m_i.ack <= '1';
    int_regs_wb_m_i.err <= '0';
end case;

Is it correct? Wouldn't it be more appropriate to indicate an error on the bus?

Currently reps is handled in 2 different ways when its values equals 1. In case of registers it will generate vector with length 1, and in case of blocks it treats it as a type and needs force_vec parameter. If reps appears, it should always be treated as a vector.

It is likely, that AGWB will be used with HLS-generated IP cores. There may be also other sources of blocks with predefined internal addresses. AGWB supports blackboxes, that can be defined as certain areas in the address space. For IPbus and AMAP-XML backends, the user may define the internal address map of the blackbox.
What may be needed, however, is a possibility to create the AGWB internal representation of the blackbox from the user-provided IPbus or AMAP XML. It would allow generating SW interfaces for all backends - C, Forth, Python...

I have just lost a significant amount of time in one of AGWB-using designs to fix my stupid mistake caused by inconsistent naming of signals.

We usually use signals like below to name the parts of the Wishbone bus:

However sometimes one may use signals defined from the slave point of view:
LINKS_wb_s_o, LINKS_wb_s_i...

The problem appears if those two conventions meet in a single block. That happened to me, and resulted in a difficult to spot error. I had two signals - one connected to the master, and another to the slave. The synthesis tool didn't complain, but the design didn't work.

Therefore I suggest to either use only the signals named from the master perspective, or (maybe better) use suffixes describing the direction: LINKS_wb_m2s, LINKS_wb_s2m.

Implementation of the RMW functionality seems to be somehow difficult, if we want fully utilize caching.
The calling syntax on the bus interface level is:

rmw(self,address,mask,value,now=true) - schedules the read-modify-write
       operation defined as follows
       X:= (X and ~mask) | (value and mask)
       If "now" is true, the new value should be scheduled
       for writing.
       If "now" is false, the new value should be calculated
       and stored internally by the bus interface.
       The last call to rmw on the particular register should
       have "now" set to "true".

The idea is that we do not write the modified value immediately.
So a few operations may be concatenated, and only the final result may be written.
However the things are getting more complex if we realize that the read of the register value may return only the "future" that will be filled with real value only after dispatch.
It means, that the bus interface must accumulate the operations without knowing the initial value of the register...

All *_pkg.vhd depend on the

library general_cores;
  use general_cores.wishbone_pkg.all;

even if they do not need to.

Generation of C headers has been added last time. That allows you to directly access registers from C/C++ code like below:

agwb_MAIN * pp = 0x8000000;
 pp->CTRL.CLK_FREQ=2;
__sync_synchronize();
pp->CTRL.CLK_ENABLE=5;
__sync_synchronize();

Please note, that use of __sync_synchronize() may be needed to avoid performing operations out of order by the CPU.

The current generation of --python target supports only agwb target.
How to support agwb_dca?
It seems that the better solution will be an addition of --python-dca option for generation of the agwb_dca.

To support blocks generated with wbgen2 or with other methods, the addr_gen_wb should support "blackboxes".

The blackbox definition should look in the following way:
<blackbox name="EXTERN" type="EXTTEST" addrbits="10" reps="2">
The name attribute is used like in other definitions.
The type attribute will be used only to point to user provided address map defition files.
E.g. for IPbus-like access, the user should provide the EXTTEST_address.xml file, defining the address map for the block.
To enable correct allocation of address space, the user must define the addrbits attribute - it is the number of address bits used to access internal registers and subblocks in the "blackbox".
The reps attribute allows creating the vector of blackboxes.

Shouldn't we use req_m instead of req in

and in

?

Otherwise we get a timing error in path:

BTW, why this problem was not found earlier?

In one of designs, where the AGWB is used (namely the firmware for CBM experiment), it was needed to provide version IDs for the individual blocks generated by AGWB. It seems relatively easy, but its implementation may be quite complex. The description below is a result of my discussion with Walter F. J. Müller (@wfjm). Thank's a lot for his remarks and suggestions.

The current implementation (that repeats the same version ID in all blocks) takes the whole XML description resulting from combining the main XML file with all included XML sub-descriptions.
Unfortunately it is not easy to split such description into the parts describing the individual blocks.

The first solution could be to take the XML sub-tree, describing the particular block and all its sub-block, convert it to certain standard string form, (e.g. using the write or tostring methods of the ElementTree module) and calculate its hash.
Such approach seems to be viable, but there are certain traps hidden:

• The output format of the above functions may change, and the calculated hash function will be different, even though the description is still the same.
• The description may contain expressions based on constants, defined in the main part of the XML. A solution could be to add definitions of constants to the output string. However, the hash will change even if a changed constant is not used in the particular block.

The better approach could be to use the checksum of the generated VHDL code (the WB interface module and the associated package). This code uses the evaluated expressions, so there is no problem with false dependency on unused constants.
Unfortunately, that approach has also certain disadvantages after the variants have been introduced.
The same VHDL code is generated for all variants, and the right variant is selected using the generics.

Therefore the block version should be generated basing on the representation that is produced from the block definition including the constant values and evaluated expressions. Currently the best choice seems to be the output of the AMAP XML target.

As separate AMAP XML maps are generated for each variant, it is possible to put a different version ID into each of them.
Because the hash values have to be put into the AMAP XML, they are computed for the hash value set to 0 (0x00000000).
After the hash is calculated, the value is replaced with the right one.
The first version is implemented in commit a2e7998 .

For systems that use variants, the additional g_ver_id generic is provided. By default it is set to the value corresponding to the "full" implementation. If the user implements variant nvar, he should set this generic to the corresponding value from the v_BLOCK_ver_id array, as shown in the example below:

  MAIN_1 : entity agwb.MAIN
    generic map(
      g_ver_id => v_MAIN_ver_id(nvar),
      g_LINKS_size => v_LINKS_size(nvar),
      g_EXTHUGE_size => v_EXTHUGE_size(nvar)
)
    port map (
      slave_i	     => wb_s_in,
      slave_o	     => wb_s_out,
      LINKS_wb_m_o   => LINKS_wb_m_o,
      LINKS_wb_m_i   => LINKS_wb_m_i,
      EXTHUGE_wb_m_o => EXTHUGE_wb_m_o,
      EXTHUGE_wb_m_i => EXTHUGE_wb_m_i,
      EXTERN_wb_m_o  => CDC_wb_m_o,
      EXTERN_wb_m_i  => CDC_wb_m_i,
      CTRL_o	     => CTRL_o,
      TEST_IN_i	     => TEST_IN_i,
      TEST_OUT_o     => TEST_OUT_o,
      rst_n_i	     => rst_n_i,
      clk_sys_i	     => clk_sys_i);

The hash of the combined XML is stored in the {TOP}_const_pkg.vhd as

constant C_{TOP}_system_ver : std_logic_vector(31 downto 0)

For the software the hash values of the block id, block version and system version are stored in the block node like in the example below:

<node id="MAIN" id_hash="0x89bd20d0" ver_hash="0xa3285016" system_hash="0xe70dc7
31" addr_bits="17">

Current implementation incorrectly handles the situation when the same blackbox is instantiated a few times with different addrbits. In fact the first instance defines the addrbits for the generated C header and probably for other files.
The better solution could be requiring a single declaration of the blackbox with addrbits (and optionally also the path to the XML address map?) and then using so defined blackbox:

<blackbox name="BB1" addrbits=12 xmlpath="../BB1.xml" />
<block name="AnyBlock">
     ...
     <subblock name="BB1_Instance1" type="BB1" />
     <subblock name=Instance_2" type="BB2" />
     ...
</block>

Instead of out_regs it should be regs_o or registers_o.

As output registers are always control registers, then these are also potential valid names cregs_o, control_regs_o, control_registers_o.

It is desired, that the system definition XML should contain also the constants (e.g. the number of subblocks or size of register vectors). Those constants should be used in generated code and included in HDL and SW packages.

It looks like this parameter makes no sense, as it has to be '.'. Revise if it can be completely removed.

When working on support for "external blocks with fixed internal structure" ( #61 ), I have found certain problems with RelaxNG validation:

• cb3baa6
• c69a8e5

Trying to spot the issue, I have found that even in the master branch, when I modify the line:

wzab@WZabHP:/tmp/agwb/tests/test$ ./generate.sh
<string>:6:0:ERROR:RELAXNGV:RELAXNG_ERR_NOELEM: Expecting an element sreg, got nothing
<string>:0:0:ERROR:RELAXNGV:RELAXNG_ERR_INTEREXTRA: Extra element block in interleave
<string>:6:0:ERROR:RELAXNGV:RELAXNG_ERR_CONTENTVALID: Element sysdef failed to validate content
<block nafme="SYS1">
|
The erroneous line was produced from the following sources:
file: block1.xml, line:1

which is not related to the real cause of the problem.
The problem is the incorrect attribute of the "block" element. There is no "sreg" expected!

Each regeneration of registers changes the register ID.

Proposed change:
Use hash of all input files and application sources.

It would be good to add pytest. The project may soon become fragile in terms of keeping regression.

I have discovered a problem with parameterized vectors of subblocks.
Currently AGWB generates the Wishbone subbus as a pair of:

t_wishbone_master_out;
t_wishbone_master_in;

signals when a subblock is not repeated, or a pair of vectors of signals:

t_wishbone_master_out_array(0 to NREPS-1);
t_wishbone_master_in_array(0 to NREPS-1);

when the subblock is repeated NREPS times.
It creates a problem when number of repetitions is parameterized, and both "1" and greater values are possible, because the implementations for NREPS=1 and for NREPS>1 are different.
Possible solution is the addition of a special attribute, that enforces implementation of the subbus as a vector even for NREPS=1:

t_wishbone_master_out_array(0 to 0);
t_wishbone_master_in_array(0 to 0);

The proposed name of the attribute is "force_vec". The example definition of the subblock may be:

<subblock name="bl1" type="adder_14" reps="1" force_vec="1" />