Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

driver: flash: Network core flash driver #13917

Merged
merged 1 commit into from
Apr 17, 2024

Conversation

sigvartmh
Copy link
Contributor

Flash driver for interacting with network core's flash from the application core. This utilizes IPC and NRF_RPC to do flash commands in the network core.

@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Feb 8, 2024
@nvlsianpu nvlsianpu self-requested a review February 8, 2024 09:37
@nvlsianpu nvlsianpu added this to the 2.6.0 milestone Feb 8, 2024
@sigvartmh sigvartmh self-assigned this Feb 8, 2024
@sigvartmh sigvartmh force-pushed the nrf53_rpc_flash_driver branch 3 times, most recently from 1952ca8 to 180d09d Compare February 15, 2024 19:58
@sigvartmh sigvartmh marked this pull request as ready for review February 15, 2024 19:59
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Feb 15, 2024

Test specification

CI/Jenkins/NRF

  • Integration Platforms

CI/Jenkins/integration

Test Module File based changes Manually selected West overwrite

Detailed information of selected test modules

Note: This message is automatically posted and updated by the CI

@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

@sigvartmh sigvartmh force-pushed the nrf53_rpc_flash_driver branch 7 times, most recently from ee9c08f to 1050136 Compare February 19, 2024 09:15
@sigvartmh sigvartmh force-pushed the nrf53_rpc_flash_driver branch 2 times, most recently from 8bbf11d to 1c5b5d4 Compare February 19, 2024 17:47
drivers/flash/Kconfig Outdated Show resolved Hide resolved
drivers/flash/Kconfig Show resolved Hide resolved
drivers/flash/Kconfig Show resolved Hide resolved
drivers/flash/flash_rpc/flash_rpc_common.c Outdated Show resolved Hide resolved
drivers/flash/flash_rpc/flash_rpc_controller.c Outdated Show resolved Hide resolved
@nordicjm
Copy link
Contributor

I do wonder if we really need this here, or could it go upstream to zephyr instead?

@sigvartmh
Copy link
Contributor Author

I do wonder if we really need this here, or could it go upstream to Zephyr instead?

Does it make sense for anything upstream to use this? I wouldn't mind putting it upstream; I wonder if it would be accepted. All this is just a workaround because the application core cannot access network core flash while being the driver of updates.

@sigvartmh sigvartmh force-pushed the nrf53_rpc_flash_driver branch 3 times, most recently from 5014a5b to 665c00c Compare February 26, 2024 11:49
Copy link
Contributor

@carlescufi carlescufi left a comment

Choose a reason for hiding this comment

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

I think you should consider using a light-weight combination:

  • Stop using nrf_ipc
  • Use icmsg as a backend for the IPC service
  • Use something super light-weight like nrfs does (check with @Rafal-Nordic)

If that doesn't work to reduce flash footprint then we should just use this PR as-is.

Great work by the way!

@nvlsianpu
Copy link
Contributor

@carlescufi
Thank for your comments.

We are pursuit to optimize the footprint. This will be done (most probably) by using own serialization layer instead of zcbor and tweaks the configuration. As the result I expected something which will be possible in boot-time and in application-running time (two separate instances, separated in execution time, each with own configuration).

@carlescufi carlescufi dismissed their stale review February 28, 2024 13:44

Discussed with Andrzej, optimization will be tackled later

@sigvartmh sigvartmh force-pushed the nrf53_rpc_flash_driver branch 5 times, most recently from 9489d5d to 6c690bb Compare February 29, 2024 09:55
@SeppoTakalo
Copy link
Contributor

Dropping from 2.6.0 as there was no approvals before RC1.

@SeppoTakalo SeppoTakalo removed this from the 2.6.0 milestone Feb 29, 2024
@sigvartmh sigvartmh force-pushed the nrf53_rpc_flash_driver branch 3 times, most recently from 2d55d7b to 6a856e4 Compare March 6, 2024 06:41
Copy link
Contributor

@nvlsianpu nvlsianpu left a comment

Choose a reason for hiding this comment

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

I will

Comment on lines +27 to +28
static const struct device *const flash_controller =
DEVICE_DT_GET_OR_NULL(DT_CHOSEN(zephyr_flash_controller));
Copy link
Contributor

Choose a reason for hiding this comment

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

I need to go back there - VNFD should allow full access in boot stage (B0N's VNFD instance) and limits access in application run-time stage ( networking stack VNFD instance). Maybe we should do that in follow-up patch.

@sigvartmh sigvartmh force-pushed the nrf53_rpc_flash_driver branch 2 times, most recently from ffe5a9b to b02088d Compare April 15, 2024 07:14
Copy link
Contributor

@nvlsianpu nvlsianpu left a comment

Choose a reason for hiding this comment

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

I really appreciate how this code legible is.
Just one nit for help info.
I think access control might be introduced in follow-up PR.

drivers/flash/Kconfig Outdated Show resolved Hide resolved
Copy link
Contributor

@de-nordic de-nordic left a comment

Choose a reason for hiding this comment

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

Looks OK. Once comment left.

drivers/flash/Kconfig Outdated Show resolved Hide resolved
Flash driver for interacting with network core's flash from the
application core. This utilizes IPC and NRF_RPC to controll flash
commands in the network core.

Signed-off-by: Sigvart Hovland <[email protected]>
select NRF_RPC
select NRF_RPC_CBOR
select FLASH

Copy link
Contributor

Choose a reason for hiding this comment

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

Unneeded empty line?

@NordicBuilder
Copy link
Contributor

Memory footprint analysis revealed the following potential issues

sample.matter.template.release[nrf7002dk_nrf5340_cpuapp]: ROM size increased by 1440[B] in comparison to the main branch. - link (cc: @kkasperczyk-no @ArekBalysNordic @markaj-nordic)
sample.matter.template.release[nrf5340dk_nrf5340_cpuapp]: ROM size increased by 1448[B] in comparison to the main branch. - link (cc: @kkasperczyk-no @ArekBalysNordic @markaj-nordic)
sample.matter.template.release[nrf52840dk_nrf52840]: ROM size increased by 1452[B] in comparison to the main branch. - link (cc: @kkasperczyk-no @ArekBalysNordic @markaj-nordic)

Note: This message is automatically posted and updated by the CI (latest/sdk-nrf/PR-13917/35)

@cvinayak cvinayak merged commit a087201 into nrfconnect:main Apr 17, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants