Add slave for reading from Ultrascale(+) DNA primitive

[jhegeman]

This should address the IPbus wrapper for the Xilinx UltraScale(+) device DNA discussed in issue #151.

Instructions on building and running the 'device DNA' example design and script.

VCU118 with RJ45 Ethernet connection. $ ipbb init device_dna $ cd device_dna $ ipbb add git https://github.com/jhegeman/ipbus-firmware -b ipbus_issue151 $ ipbb proj create vivado -t top_vcu118_sgmii_device_dna.dep vcu118_device_dna ipbus-firmware:projects/example $ cd proj/vcu118_device_dna/ $ ipbb vivado make-project $ ipbb vivado synth -j4 impl -j 4 $ ipbb vivado package

Now program the bit file into the FPGA.

$ ipbb vivado gendecoders $ cd ../../src/ipbus-firmware/projects/example/scripts/ $ cp ../../../../../proj/vcu118_device_dna/decoders/*.xml . $ ./example_device_dna.py 192.168.200.17 example_device_dna_usp.xml

The device DNA reported should be a 96-bit number similar to this: 4002000001298e463d5102c5.

[tswilliams]

Thanks again for opening this pull request!

I spotted that the two main files implementing the slave - i.e. components/ipbus_util/firmware/hdl/{ipbus_device_dna_usp.vhd,device_dna_usp.vhd} - both have a _usp suffix. However, it looks to me that their implementations could also work on Ultrascale devices (here, I'm assuming that the DNA_PORTE2 primitive can be used on Ultrascale). Is that the case? If so, it may be clearer to alter/remove _usp suffix (but beyond just shortening to _us, which could be interpreted as "Ultrascale (not U+) only", I haven't thought of a better alternate suffix this evening - suggestions welcome).

[jhegeman]

Hi Tom,

This one is now rebased on your master after #144 got merged, and updated along the lines of what was discussed in #144. I tried the instructions from scratch, and tweaked them a bit. Should be good to go now.

Cheers, Jeroen

[jhegeman]

Thanks again for opening this pull request!

I spotted that the two main files implementing the slave - i.e. components/ipbus_util/firmware/hdl/{ipbus_device_dna_usp.vhd,device_dna_usp.vhd} - both have a _usp suffix. However, it looks to me that their implementations could also work on Ultrascale devices (here, I'm assuming that the DNA_PORTE2 primitive can be used on Ultrascale). Is that the case? If so, it may be clearer to alter/remove _usp suffix (but beyond just shortening to _us, which could be interpreted as "Ultrascale (not U+) only", I haven't thought of a better alternate suffix this evening - suggestions welcome).

Myeah.... Not sure about that one. For consistency we should probably append both '_us' and '_usp', but we will never be able to keep up with that kind of naming. (For consistency they should probably also contain 'xilinx' (even though that is inferred by the US(+) part).)

But the DNA_PORTE2 primitive indeed exists in both UltraScale and UltraScale+ families.

Should we remove the suffix completely under the pretext that 'no information is better than misleading information'?

[jhegeman]

Actually, after a bit more thought: I'd say that we should be explicit and keep both the 'us' and 'usp' suffixes. It looks a bit clumsy, but it's the only complete and consistent way to handle this, as far as I see.

[tswilliams]

To make sure there's no confusion: By keep both suffixes, you mean e.g. naming the IPbus slave ipbus_device_dna_us_usp?

[jhegeman]

Yes, indeed.

[tswilliams]

OK, thanks. Yes, I agree that it's the simplest way to avoid (or at least minimise) ambiguity.

So, to summarise, the resulting changes would be:

• rename components/ipbus_util/firmware/hdl/{ipbus_device_dna_usp.vhd,device_dna_usp.vhd} to components/ipbus_util/firmware/hdl/{ipbus_device_dna_us_usp.vhd,device_dna_us_usp.vhd}
• rename the corresponding slave depfile & address table

Also, can you remove projects/example/firmware/cfg/top_vcu118_sgmii_device_dna.dep, since I think that's leftover from before the SYSMON & DNA example designs were merged?

[jhegeman]

Ok, never been closer. Let's see what the tests make of this version.

[tswilliams]

run tests, please

[tswilliams]

One other minor thing: Any thoughts on changing the address tables nodes byte{0,1,2} to word{0,1,2}?

Although it's a relatively minor issue compared to everything discussed so far, I thought I should ask the question now, as if it's only asked after the next release it might lead to a backward incompatible change affecting more people.

[jhegeman]

Good point. Done.

[tswilliams]

Branch has been merged. Thanks again for implementing the refinements requested above!