ipbus / ipbus-firmware

Firmware that implements a reliable high-performance control link for particle physics electronics, based on the IPbus protocol
https://ipbus.web.cern.ch
Other
39 stars 31 forks source link

Address decoder generator help needed #174

Closed jhegeman closed 4 years ago

jhegeman commented 4 years ago

Dear IPBus experts,

My apologies for the recurrent questions about the IPBus address decoder. My brain and the code still appear to be misaligned.

When I feed the following address table to the decoder:

<?xml version="1.0" encoding="ISO-8859-1"?>

<node>

  <node id="csr0" address="0x00000000" fwinfo="endpoint; width=1">
    <node id="control" address="0x00000000">
      <node id="word0" address="0x00000000" />
    </node>
    <node id="status" address="0x00000001">
      <node id="word0" address="0x00000000" />
    </node>
  </node>

  <node id="csr1" address="0x00000000" fwinfo="endpoint; width=2">
    <node id="control" address="0x00000000">
      <node id="word0" address="0x00000000" />
    </node>
    <node id="status" address="0x00000001">
      <node id="word0" address="0x00000000" />
    </node>
  </node>

</node>

the (relevant part of the) address decoder gives me:

    if    std_match(addr, "--------------------------------") then
      sel := ipbus_sel_t(to_unsigned(N_SLV_CSR0, IPBUS_SEL_WIDTH)); -- csr0 / base 0x00000000 / mask 0x00000000
    elsif std_match(addr, "--------------------------------") then
      sel := ipbus_sel_t(to_unsigned(N_SLV_CSR1, IPBUS_SEL_WIDTH)); -- csr1 / base 0x00000000 / mask 0x00000000

I would expect it to complain about the address overlap here.

On the other hand, when I move the second register to address 0x2 (which should be fine because the first register only contains two words):

<?xml version="1.0" encoding="ISO-8859-1"?>

<node>

  <node id="csr0" address="0x00000000" fwinfo="endpoint; width=1">
    <node id="control" address="0x00000000">
      <node id="word0" address="0x00000000" />
    </node>
    <node id="status" address="0x00000001">
      <node id="word0" address="0x00000000" />
    </node>
  </node>

  <node id="csr1" address="0x00000002" fwinfo="endpoint; width=2">
    <node id="control" address="0x00000000">
      <node id="word0" address="0x00000000" />
    </node>
    <node id="status" address="0x00000001">
      <node id="word0" address="0x00000000" />
    </node>
  </node>

</node>

the address decoder generator complains that

Node <<csr1>> at 0x2: Width, 0x2, produces address mask 0x4 that is not aligned with node's base address

This one should be fine, isn't it?

The latter may be a problem with my understanding, but the former must be a bug, right?

Best regards, Jeroen

tswilliams commented 4 years ago

Hi,

Thanks! No problem about asking more questions :) in particular, the 2nd one has highlighted that I need to add some more info to the documentation.

The first does indeed seem to be a bug; I'll start working on a fix for that.

In the 2nd case, with width=2 in the fwinfo attribute for csr1, gen_ipbus_addr_decode is being instructed that the address range for csr1 is 0x2 to 0x5 inclusive (i.e. base_address to base_address + 2^N -1, where width=N). The address decoder script requires that the base address for an endpoint node is a multiple of 2^width, due to the way in which the decode logic is autogenerated. That's cause of the error here - so it can be resolved by decreasing width to 1 for csr1 (assuming you don't need to add any more child nodes), or changing the address of csr1 to 0x4 (or a multiple thereof).

Cheers, Tom

jhegeman commented 4 years ago

Thanks Tom,

I think I tried to wrap my head around this once before already. It is basically a side-effect of the way the partial address decoding is done. Somehow the obsessive part of my brain objects to the fact that 'the larger the address block you want, the larger the hole you create in front of it' (to first order). But then the 32-bit address space is large, even with holes...

Cheers, Jeroen

alessandrothea commented 4 years ago

Hi Jeroen, I have been struggling with it for a while myself, I have to confess as it is somehow counterintuitive coming from the software work. The rationale is to minimise the impact of the decoder logic, avoiding complex address matching and address re-basing in the sub-decoder context. Enforcing the allocation of bit-aligned blocks allows these two steps to be simple and take minimal amount of resources (which is a plus on small devices)

Alessandro

jhegeman commented 4 years ago

Hi Alessandro,

Indeed. It is a bit counter-intuitive. I actually started playing with a marginally smarter decoder generator this morning. One can still do partial address decoding while forgetting about the alignment by ORing a few std_match() cases for each selector. In the end it does not add that many resources, but it also quickly shows that it does not really add much at all. The naive approach creates large holes under certain circumstances, but the address space is so large that there is still plenty of room left for any practical application.

Cheers, Jeroen

tswilliams commented 4 years ago

The ticket closure a few minutes ago was an automated action by GitHub, as I merged in the fix for the bug half of this ticket. I'll create a corresponding new release of the software in the next day or two, after merging in another update to other areas of the SW.

I don't want curtail any fruitful discussion about the autogeneration of address decode logic - so please do reopen the ticket if there are more comments on that thread :)

tswilliams commented 4 years ago

@jhegeman : I've now created the new SW release - v2.7.5 - that adds the missing address overlap check to gen_ipbus_addr_decode.