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.

Sign up for GitHub

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

Jump to bottom

applications: sdp: no copy IPC #20116

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

applications: sdp: no copy IPC #20116

wants to merge 4 commits into from
+351 −160

Conversation

mif1-nordic
Copy link
Contributor

Depends on #20017

@mif1-nordic mif1-nordic requested review from a team as code owners January 29, 2025 14:43
@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 Jan 29, 2025
@masz-nordic masz-nordic changed the title No copy ipc applications: sdp: no copy IPC Jan 29, 2025
@masz-nordic masz-nordic added this to the 3.0.0 milestone Jan 29, 2025
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Jan 29, 2025 •
edited
Loading

CI Information

To view the history of this post, clich the 'edited' button above
Build number: 20

Inputs:

Sources:

sdk-nrf: PR head: 523a617b014febf9bf538fda3debe39f0f318473

more details

sdk-nrf:

PR head: 523a617b014febf9bf538fda3debe39f0f318473
merge base: 529429bc90946738df1f2f612a1d9d9a4f27ccd2
target head (main): e24e07c4abc14f75b2f4e64e57ce2e083b0dc285
Diff

Github labels

Enabled Name Description
ci-disabled Disable the ci execution
ci-all-test Run all of ci, no test spec filtering will be done
ci-force-downstream Force execution of downstream even if twister fails
ci-run-twister Force run twister
ci-run-zephyr-twister Force run zephyr twister
List of changed files detected by CI (6) 
applications
│  ├── sdp
│  │  ├── mspi
│  │  │  ├── src
│  │  │  │  ├── hrt
│  │  │  │  │  ├── hrt-nrf54l15.s
│  │  │  │  │  ├── hrt.c
│  │  │  │  │  │ hrt.h
│  │  │  │  │ main.c
drivers
│  ├── mspi
│  │  │ mspi_nrfe.c
include
│  ├── drivers
│  │  ├── mspi
│  │  │  │ nrfe_mspi.h

Outputs:

Toolchain

Version: 342151af73
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:342151af73_bbe5b33786

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

  • ◻️ Toolchain - Skipped: existing toolchain is used
  • ✅ Build twister
    • sdk-nrf test count: 26
  • ❌ Integration tests
    • ❌ test-low-level
Disabled integration tests
    • desktop52_verification
    • doc-internal
    • test_ble_nrf_config
    • test-fw-nrfconnect-apps
    • test-fw-nrfconnect-ble_mesh
    • test-fw-nrfconnect-ble_samples
    • test-fw-nrfconnect-boot
    • test-fw-nrfconnect-chip
    • test-fw-nrfconnect-fem
    • test-fw-nrfconnect-nfc
    • test-fw-nrfconnect-nrf-iot_lwm2m
    • test-fw-nrfconnect-nrf-iot_mosh
    • test-fw-nrfconnect-nrf-iot_positioning
    • test-fw-nrfconnect-nrf-iot_samples
    • test-fw-nrfconnect-nrf-iot_serial_lte_modem
    • test-fw-nrfconnect-nrf-iot_thingy91
    • test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
    • test-fw-nrfconnect-nrf_crypto
    • test-fw-nrfconnect-ps
    • test-fw-nrfconnect-rpc
    • test-fw-nrfconnect-rs
    • test-fw-nrfconnect-tfm
    • test-fw-nrfconnect-thread
    • test-fw-nrfconnect-zigbee
    • test-sdk-audio
    • test-sdk-dfu
    • test-sdk-find-my
    • test-sdk-mcuboot
    • test-sdk-pmic-samples
    • test-sdk-sidewalk
    • test-sdk-wifi
    • test-secdom-samples-public

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.

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

@mif1-nordic mif1-nordic force-pushed the no_copy_ipc branch 7 times, most recently from bdeae8e to 5d6c142 Compare January 30, 2025 13:05
jaz1-nordic
jaz1-nordic reviewed Jan 30, 2025
View reviewed changes
include/drivers/mspi/nrfe_mspi.h Outdated
@@ -16,6 +16,8 @@ extern "C" {

#ifdef CONFIG_SOC_NRF54L15

#define NRFE_MSPI_NO_COPY_IPC
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be an option in Kconfig, set to default value depending on the platform.

@mif1-nordic mif1-nordic force-pushed the no_copy_ipc branch 9 times, most recently from 116dc9c to f2fd5ad Compare February 5, 2025 08:12
@mif1-nordic
applications: sdp: mspi: Fixed bit blocking on bus width change
f2aa8d6 
Fixed bug where after increasing bus width using buffered registers,
in first clock cycle the mask for previous bus width was still present.

Signed-off-by: Michal Frankiewicz <[email protected]>
magp-nordic
magp-nordic reviewed Feb 6, 2025
View reviewed changes
drivers/mspi/mspi_nrfe.c Show resolved Hide resolved
drivers/mspi/mspi_nrfe.c Show resolved Hide resolved
applications/sdp/mspi/src/main.c
@@ -359,9 +366,12 @@ static void ep_recv(const void *data, size_t len, void *priv)
xfer_config->xfer_config.command_length != 0 ||
xfer_config->xfer_config.address_length != 0);

#ifdef NRFE_MSPI_NO_COPY_IPC
nrfe_mspi_xfer_config_ptr = &xfer_config->xfer_config;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is such a hacky move, I do not like it. mspi_xfer_config_msg (and xfer_config, as it is part of it) is created in nrfe_mspi.c right before calling send_config, which means, it is a local variable, and it will be deallocated the moment APP leaves xfer_packet. I admit, it will work now, as xfer_packet also sends packets right after sending xfer_config. However, this is such a big risk in terms of memory violation. What if someone decides, that they will split xfer_packet function into two, one that will send xfer_config, and the other, that would send the packets? Then this will no longer work.
@masz-nordic do you think we can accept the risk that comes with this solution?

@mif1-nordic
applications: sdp: mspi: Fixed additional clock edge in modes 1-3
21718da 
Added workarounds to hardware issue causing,
additional clock edge in spi modes 1-3.

Signed-off-by: Michal Frankiewicz <[email protected]>
@mif1-nordic
drivers: mspi: No copy IPC-drivers
aee6ebb 
Implemented option to pass data between cores
through IPC by reference.

Signed-off-by: Michal Frankiewicz <[email protected]>
@mif1-nordic
applications: sdp: mspi: No copy IPC-applications
523a617 
Implemented option to pass data between cores
through IPC by reference.

Signed-off-by: Michal Frankiewicz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@magp-nordic magp-nordic magp-nordic left review comments

@jaz1-nordic jaz1-nordic jaz1-nordic left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added.
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@mif1-nordic @NordicBuilder @magp-nordic @jaz1-nordic @masz-nordic