soc: raspberrypi: rpi_pico: Improvement linker configuration #79460
Conversation
zephyrbot
requested review from
anangl,
decsny,
dkalowsk,
gmarull,
henrikbrixandersen,
kartben,
MaureenHelm,
mmahadevan108,
mnkp,
nashif,
nordic-krch,
stephanosio and
tejlmand
|
The following west manifest projects have changed revision in this Pull Request:
✅ All manifest checks OK Note: This message is automatically posted and updated by the Manifest GitHub Action. |
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
soburi
force-pushed
the
rp2350-linker-improvement
branch
from
dc8c277 to
347ea2a
Compare
zephyrbot
removed
manifest
manifest-hal_rpi_pico
DNM
soburi
force-pushed
the
rp2350-linker-improvement
branch
2 times, most recently
from
d02a706 to
e0d540b
Compare
| @@ -435,4 +435,12 @@ | |||
| compatible = "raspberrypi,pico-temp"; | |||
| status = "disabled"; | |||
| }; | |||
|
|
|||
| id_code: id_code@10000000 { | |||
There was a problem hiding this comment.
should this have image def flash in the name?
There was a problem hiding this comment.
@thedjnK I had that same debate as well when reviewing. My understanding was the id_code naming was the shorthand for ImageDef_CODE (apologies for the caps mess) which covered the same naming area.
There was a problem hiding this comment.
I would also like to see this renamed. I wrote the image definition block. ID is a common abbreviation for with "identifier/identicification", and this isn't code (or a code, or encoding...).
It is a Block of metadata (sometimes referred to as a PICOBIN_BLOCK) of type Image Definition, referred to as an IMAGE_DEF throughout the vendor's documentation.
There was a problem hiding this comment.
This was my careless mistake.
It's common to match the label here to the region name.
| @@ -435,4 +435,12 @@ | |||
| compatible = "raspberrypi,pico-temp"; | |||
| status = "disabled"; | |||
| }; | |||
|
|
|||
| id_code: id_code@10000000 { | |||
There was a problem hiding this comment.
I would also like to see this renamed. I wrote the image definition block. ID is a common abbreviation for with "identifier/identicification", and this isn't code (or a code, or encoding...).
It is a Block of metadata (sometimes referred to as a PICOBIN_BLOCK) of type Image Definition, referred to as an IMAGE_DEF throughout the vendor's documentation.
| compatible = "zephyr,memory-region"; | ||
| zephyr,memory-region = "IMAGE_DEF_FLASH"; | ||
| zephyr,memory-region-flags = "r"; | ||
| reg = <0x10000000 0x80>; | ||
| status = "okay"; |
There was a problem hiding this comment.
Please match the style here https://docs.kernel.org/devicetree/bindings/dts-coding-style.html#order-of-properties-in-device-node .
It's not mandated by the Zephyr Project, but conscious effort was made to ensure this was the case elsewhere in this file.
soburi
force-pushed
the
rp2350-linker-improvement
branch
from
e0d540b to
9d33674
Compare
The definition of memory areas will be changed to use DeviceTree, which makes it easier to coexist with other Zephyr functions. Signed-off-by: TOKITA Hiroshi <[email protected]>
Use zephyr_linker_sources() to separate definitions of soc-specific sections. Also, we can directly include `include/zephyr/arch/arm/cortex_m/scripts/linker.ld` in CMakeLists.txt, which will fit Zephyr's build system properly. Signed-off-by: TOKITA Hiroshi <[email protected]>
soburi
force-pushed
the
rp2350-linker-improvement
branch
from
9d33674 to
864d042
Compare
There was a problem hiding this comment.
I think this needs more design thought before being approved. I've highlighted some concerns below. I think the most fundamental is that the RP2350A and RP2350B don't even have flash storage, so anything related to storing IMAGE_DEF blocks in flash don't make sense in the context of rp2350.dtsi.
Edit:
In the earlier versions of the implementation, config RP2_REQUIRES_IMAGE_DEFINITION_BLOCK was a bit more meaningful, and the IMAGE_DEF section was conditionally included into the binary image. That got rebased into oblivion (pretty sure it's left my reflog by now), and that was not correct, leading to this confusing state.
| image_def_flash: flash@10000000 { | ||
| compatible = "zephyr,memory-region"; | ||
| reg = <0x10000000 0x80>; | ||
| zephyr,memory-region = "IMAGE_DEF_FLASH"; | ||
| zephyr,memory-region-flags = "r"; | ||
| status = "okay"; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Having looked at this further, I don't think this is correct. It needs changing.
boards/raspberrypi/rpi_pico2/rpi_pico2.dtsi defines the partitions in flash. The RP2350A and RP2350B don't even have flash, so anything related to defining things in flash doesn't make sense in the base file rp2350.dtsi.
Similarly, it's a board and/or application (app) specific thing to decide where this information is (it doesn't have to be at the very beginning of flash, for example).
| @@ -429,6 +429,14 @@ | |||
| io-channels = <&adc 4>; | |||
| status = "disabled"; | |||
| }; | |||
|
|
|||
| boot_flash: flash@10000000 { | |||
There was a problem hiding this comment.
If we put this in 'unit address' order, this helps to highlight that something isn't right about this approach. Similarly, a quick grep of the codebase suggests that flash@unitaddress is always used to refer to some non-volatile storage, not to something stored at that location.
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
Improve linker settings to a modern way using zephyr_linker_sources() and "zephyr,memory-region."