Xilinx / PYNQ-Metadata

PYNQ-Metadata provides an abstract description of reconfigurable system designs.
BSD 3-Clause "New" or "Revised" License
7 stars 8 forks source link

HwhFrontend doesn't add Registers for AXI_FULL ports #14

Open TRnhld opened 1 year ago

TRnhld commented 1 year ago

In hwh_frontend.py(line 485ff) there is a check, tries to map the "INTERFACE" to a portname. If there is a AXI_Full port, the INTERFACE property will be "aximm" which does not match to AXI_FULL! therefore the variable _port_available will not be set to True.

part of hwh-File:

...
<MODULE COREREVISION="23" FULLNAME="/axi_quad_spi_0" HWVERSION="3.2" INSTANCE="axi_quad_spi_0" IPTYPE="PERIPHERAL" IS_ENABLE="1" MODCLASS="MEMORY_CNTLR" MODTYPE="axi_quad_spi" VLNV="xilinx.com:ip:axi_quad_spi:3.2">
      <DOCUMENTS>
        <DOCUMENT SOURCE="http://www.xilinx.com/cgi-bin/docs/ipdoc?c=axi_quad_spi;v=v3_2;d=pg153-axi-quad-spi.pdf"/>
      </DOCUMENTS>
      <ADDRESSBLOCKS>
        <ADDRESSBLOCK ACCESS="read-write" INTERFACE="aximm" NAME="MEM0" RANGE="4096" USAGE="register">
          <REGISTERS>
            <REGISTER NAME="SRR">
              <PROPERTY NAME="DESCRIPTION" VALUE="Software Reset Register"/>
              <PROPERTY NAME="ADDRESS_OFFSET" VALUE="0x40"/>
              <PROPERTY NAME="SIZE" VALUE="32"/>
              <PROPERTY NAME="ACCESS" VALUE="write-only"/>
              <PROPERTY NAME="IS_ENABLED" VALUE="true"/>
              <PROPERTY NAME="RESET_VALUE" VALUE="0x0"/>
              <FIELDS>
                <FIELD NAME="Reset">
                  <PROPERTY NAME="DESCRIPTION" VALUE="The only allowed operation on this register is a write of 0x0000000a, which resets the AXI Quad SPI core.&#xA;"/>
                  <PROPERTY NAME="ADDRESS_OFFSET" VALUE="0"/>
...

check in python

# addrblock.get("INTERFACE") = "aximm"
# core.ports.keys() = dict_keys(['SPI_0', 'AXI_FULL', 'ext_spi_clk', 's_axi4_aclk', 's_axi4_aresetn', 'ip2intc_irpt'])
# Line 482
_port_available = False
_portname = ""
if addrblock.get("INTERFACE").lower() in core.ports:
    _port_available = True
    _portname = addrblock.get("INTERFACE").lower()
elif addrblock.get("INTERFACE").upper() in core.ports:
    _port_available = True
    _portname = addrblock.get("INTERFACE").upper()
elif addrblock.get("INTERFACE") in core.ports:
    _port_available = True
    _portname = addrblock.get("INTERFACE")
TRnhld commented 1 year ago

btw: there is also some inconsistency of the version: version.txt != pynqmetadata.__version__

STFleming commented 1 year ago

Thanks for this. Is there any chance you could share a minimal hwh file that I can take a look at?

TRnhld commented 1 year ago

design4.zip It's not the minimal desing you asked for, but this is the one I debugged with. The component with the mentioned issue can be found under "/qspi_master_hier/axi_quad_spi_perf" (L 7278f)

Maybe a good option for a fix coud be:

elif addrblock.get("INTERFACE") == "aximm" and "AXI_FULL" in core.ports:
    _port_available = True
    _portname = "AXI_FULL"
STFleming commented 1 year ago

I have tested your suggested fix here: https://github.com/STFleming/PYNQ-Metadata/tree/axi_full_register_fix

It seems to work well: image

It also passes all current tests.

Would you like to submit a PR to get credit for the fix? I'm happy to do a PR from my branch if you'd prefer :)

TRnhld commented 1 year ago

Im fine with you doing the PR. - Thanks