Size-checking bug in address table generator script when using masked registers

[jhegeman]

Hi,

The following looks like a reasonable address table to me.

<node id="top-level"
      fwinfo="endpoint; width=2">

  <node id="test0"
        address="0x00000000"
        mask="0xffffffff"
        permission="r" />

  <node id="test1"
        address="0x00000001"
        mask="0x00000001"
        permission="r" />

  <node id="test2"
        address="0x00000002"
        mask="0x00000001"
        permission="r" />

  <node id="test3"
        address="0x00000003"
        mask="0x00000001"
        permission="r" />

  <node id="test4"
        address="0x00000004"
        mask="0x00000001"
        permission="r" />

</node>

The address decoder generation script, however, only considers the test0 register, because the others have masks that differ from 0xffffffff.
This is due to this line in the script:
https://github.com/ipbus/ipbus-software/blob/master/uhal/tools/scripts/gen_ipbus_addr_decode#L257

The result is that it happily generates an address decoder, whereas there is a size problem. The top-level node is not wide enough to hold all registers. This is not a problem at this level of the addressing, but if this file is included into another address table, things go wrong at that level.

Removing the mask check in the line mentioned above gives the expected result: no address table, and a descriptive error message.

ERROR   : Endpoint node <<TOP>> (base address 0x0, width 2) has a sub-node <<test4>> (base address 0x4, size 0x1) that overflows its address space

I have tested this suggested fix with a fairly large design, and not found any side-effects. It makes me wonder, though, why this node selection was implemented in the first place. Is there anything wrong with/bad practice about the above address table?

Cheers,
Jeroen

[tswilliams]

I have tested this suggested fix with a fairly large design, and not found any side-effects. It makes me wonder, though, why this node selection was implemented in the first place. Is there anything wrong with/bad practice about the above address table?

Your address table looks fine to me. I suspect this node selection may have just been implemented to filter out unnecessary info in the tree, based on the assumption that all masked nodes were a child of a node with the same address. I think this was typically the case in the address tables that gen_ipbus_addr_decode was initially used for. But I don't see any issue in removing that if statement. Indeed beyond your address tables, for example we also have similar address tables that specify both mask and a non-zero address in EMP, and so we should definitely include that fix. If you already have this change on a branch, feel free to open a pull request. Otherwise I can open a pull request.

[jhegeman]

Hi Tom,

Would it be possible to have a look at #331 first?
That way I don't have to disentangle the differences (although that should be straightforward).

Cheers,
Jeroen

[tswilliams]

Sure. That PR has now been merged - sorry for missing it at the time

[jhegeman]

Closes #335.