Skip to content

Conversation

@maass-hamburg
Copy link
Member

@maass-hamburg maass-hamburg commented Nov 12, 2024

this enables the use of mcuboot on riscv with ram load mode.

for that a function to get the flash_img_get_upload_slot() is needed.

Fixes: #83941

Copy link
Contributor

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look OK, one nit.

nordicjm
nordicjm previously approved these changes Nov 12, 2024
@maass-hamburg
Copy link
Member Author

We have some problems in ci, now that CONFIG_ROM_START_OFFSET isn't ignored anymore on RISCV platforms and actually used.

@d3zd3z d3zd3z changed the title mcuboot: enable use on RICSV with ram load mode mcuboot: enable use on RISC-V with ram load mode Nov 13, 2024
@d3zd3z
Copy link
Contributor

d3zd3z commented Nov 18, 2024

The CI issues will have to be addressed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is CONFIG_ROM_START_OFFSET > 0 needed here but only for risc?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I don't wanted to change it for the other archs
  • there are some riscv soc, that don't work, so the part to add a offset is only used, if that offset is needed

Copy link
Contributor

@nordicjm nordicjm Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the others are failing, they should be fixed, this is adding a hack (would suggest getting build logs from CI then pinging the maintainers of those platforms since they would be broken)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the problem was mainly opentitan, which needs to put a other rom header in front. maybe @snematbakhsh can help

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nordicjm the opentitan one is not compatible with having a CONFIG_ROM_START_OFFSET, because it has to put something soc specific in front in the linker.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tkng-rivos @snematbakhsh as submitters of that board, can you take a look/fix?

Copy link
Contributor

@snematbakhsh snematbakhsh Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maass-hamburg Once we turn on CONFIG_ROM_START_OFFSET for risc-v, I think we need to account for the manifest (similar to what is done here for another SOC). So I think we want to change soc/lowrisc/opentitan/Kconfig.defconfig as follows:

# The OpenTitan SoC requires a manifest in front of the
# application binary.
config ROM_START_OFFSET
	default 0x404

.. and subsequently we no longer need the "GREATER 0" special case.

Why 0x404 and not 0x400 for 1024 byte manifest, you ask? It's this bug, I will take care of it after you land your PR (so just use 0x404).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@snematbakhsh the CONFIG_ROM_START_OFFSET will result in everything before the offset being filled with zeros. But you need to write the start address in the manifest see here:

.word(__start - __rom_header)

We would also have to remove the part for creating the manifest, because it would conflict with the CONFIG_ROM_START_OFFSET code.

Copy link
Contributor

@snematbakhsh snematbakhsh Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maass-hamburg

the CONFIG_ROM_START_OFFSET will result in everything before the offset being filled with zeros.

It seems to work fine on my side, did I miss something?
Applying patches 1d65a59 (to generate .bin) and 387478c (my proposed change as previously discussed).

$ west build -b opentitan_earlgrey samples/hello_world/
$ hexdump build/zephyr/zephyr.bin | more
0000000 0000 0000 0000 0000 0000 0000 0000 0000
*
0000380 0000 0000 0404 0000 0000 0000 0000 0000
0000390 0000 0000 0000 0000 0000 0000 0000 0000
*
0000400 0000 0000 1197 f000 8193 4541 0297 0000

I think this works because the SORT_KEY for the OpenTitan manifest ("000...") precedes the SORT_KEY for rom_start_offset.ld ("0x0").

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like its working

@maass-hamburg
Copy link
Member Author

@tkng-rivos @snematbakhsh ping ??

snematbakhsh
snematbakhsh previously approved these changes Dec 12, 2024
Copy link
Contributor

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nit.

@maass-hamburg
Copy link
Member Author

fixed nit.

Comment on lines 34 to 36
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems completely unrelated to the PR. Please revert.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cfriedt these tests are now failing, and I don't know why. You may want to take a look.

add flash_img_get_upload_slot() to get current
upload slot.
when CONFIG_MCUBOOT_BOOTLOADER_MODE_RAM_LOAD
is enabled, it is not based on the DT.

Signed-off-by: Fin Maaß <[email protected]>
don't assume, that the slot to upload is the second.

Signed-off-by: Fin Maaß <[email protected]>
don't select USE_DT_CODE_PARTITION,
when MCUBOOT_BOOTLOADER_MODE_RAM_LOAD

Signed-off-by: Fin Maaß <[email protected]>
@maass-hamburg maass-hamburg force-pushed the mcuboot_ram_flash_img branch 2 times, most recently from 489d6f2 to 7d89f50 Compare January 7, 2025 14:05
@maass-hamburg maass-hamburg requested a review from cfriedt January 7, 2025 14:56
nordicjm
nordicjm previously approved these changes Jan 8, 2025
cfriedt
cfriedt previously approved these changes Jan 8, 2025
be able to use ROM_START_OFFSET on RISCV.

Signed-off-by: Fin Maaß <[email protected]>
@maass-hamburg maass-hamburg dismissed stale reviews from cfriedt and nordicjm via fc22d71 January 13, 2025 18:39
@maass-hamburg
Copy link
Member Author

maass-hamburg commented Jan 13, 2025

qemu_riscv64 and qemu_riscv32 do not support a different a ROM offset. The start has to be at 0x80000000. It is not using the Entry point address from the elf file. It might be possible to set a start address via command line/options, but that's not done in this PR.

@kartben kartben merged commit b10781b into zephyrproject-rtos:main Jan 17, 2025
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mcuboot: Don't select CONFIG_USE_DT_CODE_PARTITION, when CONFIG_MCUBOOT_BOOTLOADER_MODE_RAM_LOAD

9 participants