[multitop] Port OTTF and test_rom to multitop #26038
Conversation
pamaury
force-pushed
the
multitop_port
branch
14 times, most recently
from
f337772 to
3f694a2
Compare
pamaury
requested review from
msfschaffner,
cfrantz and
a team
as code owners
pamaury
changed the title
pamaury
force-pushed
the
multitop_port
branch
4 times, most recently
from
44d234d to
347819e
Compare
|
The PR finally passes the CI. The only thing not done in this PR is wiring english breakfast which I suggest to postpone to another PR. |
They only depend on definitions in the boot_data.h header, not on the actual functions in boot_data.c Signed-off-by: Amaury Pouly <[email protected]>
Signed-off-by: Amaury Pouly <[email protected]>
Signed-off-by: Amaury Pouly <[email protected]>
Signed-off-by: Amaury Pouly <[email protected]>
In order to support multiple ROM images, scramble_image.py now only relies on the passed top hjson file and a scrambling mode to fetch all needed parameters to scramble the image. The script maps the scrambling mode (e.g. "base-rom") to its memory controller name, memory type, module name, scrambling key and nonce names in the top HJSON file, and can then retrieve the scrambling key, nonce, base address and size of the image to generate. This allows for passing different image paths (e.g. /path/to/base_rom_*.vmem and /path/to/second_rom_*.vmem) and generate the corresponding images with a single script. Signed-off-by: Samuel Ortiz <[email protected]> Signed-off-by: Amaury Pouly <[email protected]>
Signed-off-by: Amaury Pouly <[email protected]>
Signed-off-by: Amaury Pouly <[email protected]>
Signed-off-by: Amaury Pouly <[email protected]>
Signed-off-by: Amaury Pouly <[email protected]>
Signed-off-by: Amaury Pouly <[email protected]>
Signed-off-by: Amaury Pouly <[email protected]>
Signed-off-by: Amaury Pouly <[email protected]>
Signed-off-by: Amaury Pouly <[email protected]>
The rule was correct making files `includes` available in the linker search path, but not in the preprocess search path. This made it impossible to `#include` other files. This commit fixes this behaviour to align the code with the documentation. Signed-off-by: Amaury Pouly <[email protected]>
Due to some differences between the tops, a new configuration file (`ottf_ld_top_config.ld`) is introduced which is included in all ottf linker scripts. This file sets up an alias for the ottf storage region (eflash for EG/EB, ctn for DJ) and also sets some magic symbols to disable NV scratch in EB. Signed-off-by: Amaury Pouly <[email protected]>
With the switch to using the DT in the mask_rom (via silicon_creator's drivers), we need to enable LTO to optimize away constants. Optimize for size, otherwise the ROM does not fit into 64Kb. Signed-off-by: Amaury Pouly <[email protected]>
Signed-off-by: Amaury Pouly <[email protected]>
Signed-off-by: Amaury Pouly <[email protected]>
Signed-off-by: Amaury Pouly <[email protected]>
Signed-off-by: Amaury Pouly <[email protected]>
This function is expected to be set as a breakpoint in several gdb end-to-end tests. It is not critical that this function be inlined anyway. Signed-off-by: Amaury Pouly <[email protected]>
With LTO, the linker is more eager to remove it since it's not used anywhere. Signed-off-by: Amaury Pouly <[email protected]>
The generator does not work correctly if any register description
contains `{` because it is interpreted as a format string. Fix the
code by not formatting raw content.
Signed-off-by: Amaury Pouly <[email protected]>
pamaury
force-pushed
the
multitop_port
branch
from
38cb344 to
cdb3578
Compare
There was a problem hiding this comment.
LGTM with nits.
I didn't bother continuing to nit-pick the top-specific names in every file, hehe. But do note that the changes have swapped the explicitly top-specific name parts with implicitly top-specific parts. I don't think you have to handle all of these right now, in this PR, but we should be cognizant that this isn't quite at the goal of removing top-specific identifiers.
| "earlgrey": { | ||
| 'base-rom': MemCtrlParams('rom_ctrl', | ||
| 'rom', 'rom_ctrl', | ||
| 'RndCnstScrKey', 128, | ||
| 'RndCnstScrNonce', 64), | ||
| }, |
There was a problem hiding this comment.
Hehe, you know I always get concerned when I see hard-coded parameters for known tops. We can make this better later, though.
| assert isinstance(base, dict) | ||
| base_addr_rom = base.get(memory_type) | ||
| assert isinstance(base_addr_rom, dict) | ||
| base_addr = base_addr_rom.get("hart") |
There was a problem hiding this comment.
Probably the address space should become an argument (later). I kind of wonder how the base address factors in, though, especially if it turns out that a ROM is accessible from multiple address spaces.
| key = Scrambler._get_param_value(params, 'RndCnstScrKey', 128) | ||
|
|
||
| return Scrambler(nonce, key, rom_size_words, hash_file) | ||
| print(top["name"]) |
There was a problem hiding this comment.
Is this a stray debugging print?
|
|
||
| # Load the input ELF file | ||
| clr_mem = MemFile.load_elf32(args.infile, 4 * ROM_BASE_WORD) | ||
| clr_mem = MemFile.load_elf32(args.infile, scrambler.rom_base) |
There was a problem hiding this comment.
Ah, I see, the base address is used merely for loading from the right section in an ELF. This feels slightly backwards, like the raw binary (untranslated from address 0) should be used here. But this isn't a change from the status quo, so nothing to do about it here.
| @@ -129,7 +131,7 @@ void ottf_console_init(void) { | |||
| // configured. The default is to use UART0. | |||
| if (base_addr == 0) { | |||
| CHECK(kOttfTestConfig.console.type == kOttfConsoleUart); | |||
| base_addr = TOP_EARLGREY_UART0_BASE_ADDR; | |||
| base_addr = dt_uart_primary_reg_block(kDtUart0); | |||
There was a problem hiding this comment.
nit: kDtUart0 is a top-specific name.
| } | ||
|
|
||
| void ottf_console_flow_control_enable(void) { | ||
| CHECK_DIF_OK(dif_rv_plic_init( | ||
| mmio_region_from_addr(TOP_EARLGREY_RV_PLIC_BASE_ADDR), &ottf_plic)); | ||
| CHECK_DIF_OK(dif_rv_plic_init_from_dt(kDtRvPlic, &ottf_plic)); |
There was a problem hiding this comment.
nit: Using top-specific names here.
| (uint32_t *)(TOP_EARLGREY_SRAM_CTRL_MAIN_RAM_BASE_ADDR + | ||
| TOP_EARLGREY_SRAM_CTRL_MAIN_RAM_SIZE_BYTES); | ||
| uint32_t ram_base_addr = | ||
| dt_sram_ctrl_reg_block(kDtSramCtrlMain, kDtSramCtrlRegBlockRam); |
There was a problem hiding this comment.
nit: This one is kind of going to be an interesting problem for disambiguation, but... kDtSramCtrlMain is a top-specific name.
|
|
||
| if (peripheral == kTopEarlgreyPlicPeripheralUart0 && | ||
| dt_instance_id_t devid = dt_plic_id_to_instance_id(plic_irq_id); | ||
| if (devid == dt_uart_instance_id(kDtUart0) && |
There was a problem hiding this comment.
nit: kDtUart0 is a top-specific name.
| @@ -153,8 +152,7 @@ void _ottf_main(void) { | |||
|
|
|||
| // Clear reset reason register. | |||
| dif_rstmgr_t rstmgr; | |||
| CHECK_DIF_OK(dif_rstmgr_init( | |||
| mmio_region_from_addr(TOP_EARLGREY_RSTMGR_AON_BASE_ADDR), &rstmgr)); | |||
| CHECK_DIF_OK(dif_rstmgr_init_from_dt(kDtRstmgrAon, &rstmgr)); | |||
There was a problem hiding this comment.
nit: kDtRstmgrAon is a top-specific name, and this could be especially problematic for OTTF integration. Not every integration is going to have a rstmgr with this sort of AON / non-AON power domain split. We might need to think about how init might need to split off an integration layer specific to each top. (normal OS / operating environment stuff)
Thanks for the review. You are correct that this PR does not "truly" make those files top-agnostic but the goal of that PR was to keep the semantic. I have added an item in the multitop tracking issue #26116 so that we can revisit this. Some are relatively easy (ie there should only be one rstmgr, so pick index 0), some are tricky (how do you distinguish the main ram from the aon ram?!). I think part of the solution here is to have per-top files that fill the gap to indicate what to do (same problem with linker scripts, and certain other things like |
This PR does several things:
opentitan_{binary,test}so that they become minimally multitop awareIt should be noted that even though the purpose is to make the test_rom working on multitop, this requires to touch code in silicon_creator which is shared with the mask_rom. As a result, DT usage makes its way into the mask_rom which causes its size to increase. Therefore I also had to enable LTO in the mask_rom to optimize away the DT usage.
This PR is large but most commits are very small, I would recommend that reviewers all look at commits about the code that they are familiar with and skip the others. Although it's technically possible to split this PR, it would result in untested code being merged so I think it's not worth it.
Note: I haven't yet be able to check if everything works for english breakfast, probably the final fixes for EB will be done in a future PR.