search

m-labs / misoc

The original high performance and small footprint system-on-chip based on Migen™
https://m-labs.hk
Other
306 stars 85 forks source link

register_rom weirdness #96

Open keesj opened 5 years ago

keesj commented 5 years ago

Hi,

I am trying to get the papilo pro build working and the first error I got was :

Traceback (most recent call last):
  File "/usr/lib/python3.6/runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "/usr/lib/python3.6/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "/home/dus/projects/sdfsadfsd/migen/misoc/misoc/targets/papilio_pro.py", line 103, in <module>
    main()
  File "/home/dus/projects/sdfsadfsd/migen/misoc/misoc/targets/papilio_pro.py", line 97, in main
    soc = BaseSoC(**soc_sdram_argdict(args))
  File "/home/dus/projects/sdfsadfsd/migen/misoc/misoc/targets/papilio_pro.py", line 87, in __init__
    self.register_rom(self.spiflash.bus)
  File "/home/dus/projects/sdfsadfsd/migen/misoc/misoc/integration/soc_core.py", line 138, in register_rom
    assert self.cpu_reset_address < rom_size
AssertionError

The code in question looks like the following and really looks strange to me

    def register_rom(self, interface, rom_size=0xa000):
        self.add_wb_slave(self.mem_map["rom"], rom_size, interface)
        assert self.cpu_reset_address < rom_size
        self.add_memory_region("rom", self.cpu_reset_address,
                               rom_size-self.cpu_reset_address)

I can understand that the reset address needs to point to a valid part (and preferably the reset vectors of the CPU) but why does the address need to be smaller than the rom_size. should this code not read

assert cpu_reset_address >= mem_map["rom"] and assert cpu_reset_address < mem_map["rom"] + rom_size

the next strange thing is the next line. why is the memory region set to the start address of the cpu reset and not simply the mem_map["rom"] address. I am even more intrigued by the size of the region that is set to rom_size-self.cpu_reset_address.

To me it looks like the code only really will work if the reset_address is 0 (and this is often the case)

I have tried to create a patch that would do something more meaningful but I think I might be lacking some context.

The register_rom function (in my case) is called from https://github.com/m-labs/misoc/blob/master/misoc/targets/papilio_pro.py#L87 (to map the boot code to be flash I guess for being executed xip?).

sbourdeauducq commented 5 years ago

assert cpu_reset_address >= mem_map["rom"] and assert cpu_reset_address < mem_map["rom"] + rom_size

Should be mem_map["rom"] <= cpu_reset_address < mem_map["rom"] + rom size.

why is the memory region set to the start address of the cpu reset

So the linker knows where the startup code begins.

keesj commented 5 years ago

I updated the pull request to fix the above issues and allow booting from spi flash.