Ipc reduced structures #19877
Ipc reduced structures #19877
Conversation
|
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. |
CI InformationTo view the history of this post, clich the 'edited' button above Inputs:Sources:sdk-nrf: PR head: b749c232705f1364ec7a50ab7493b65b5d40715c more detailssdk-nrf:
Github labels
List of changed files detected by CI (8)Outputs:ToolchainVersion: 342151af73 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
mif1-nordic
force-pushed
the
Ipc_Reduced_Structures
branch
from
3f4fe33 to
860ba08
Compare
mif1-nordic
force-pushed
the
Ipc_Reduced_Structures
branch
from
860ba08 to
49791bb
Compare
mif1-nordic
force-pushed
the
Ipc_Reduced_Structures
branch
3 times, most recently
from
bcbe9c9 to
51738bc
Compare
github-actions
bot
added
changelog-entry-required
|
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
force-pushed
the
Ipc_Reduced_Structures
branch
3 times, most recently
from
b2f47e9 to
461afba
Compare
| @@ -44,3 +44,5 @@ CONFIG_SYS_CLOCK_EXISTS=n | |||
|
|
|||
| CONFIG_OUTPUT_DISASSEMBLY=y | |||
| CONFIG_COMMON_LIBC_MALLOC=n | |||
|
|
|||
| CONFIG_COMPILER_OPT="-fshort-enums" | |||
There was a problem hiding this comment.
why exactly is short-enums required ?
I see no comments on this.
There was a problem hiding this comment.
This was introduced in previous PR #19449 but it is required for IPC.
APP is transfering structures containing enums to VPR and APP is configured to have short enums so VPR couldn't read them.
mif1-nordic
force-pushed
the
Ipc_Reduced_Structures
branch
from
461afba to
000dcf7
Compare
| typedef struct { | ||
| nrfe_mspi_opcode_t opcode; /* nrfe_mspi_opcode */ | ||
| uint32_t command; | ||
| uint32_t address; | ||
| uint32_t num_bytes; | ||
| } nrfe_mspi_xfer_packet_t; |
There was a problem hiding this comment.
As I don't know the data size I couldn't add it as a field to the structure, so the data is appended at the end of structure when sending is through IPC to VPR.
There was a problem hiding this comment.
Why not send a pointer to the data then? I do not like that data is hidden in this solution
| enum mspi_duplex duplex; | ||
| enum mspi_io_mode io_mode; | ||
| enum mspi_cpp_mode cpp; | ||
| enum mspi_endian endian; |
There was a problem hiding this comment.
I would remove endianness, right now we only support big-endian.
There was a problem hiding this comment.
yes, but as we've discussed in the meeting about these structures some of the fields we are not using right now but will be used in the future
| uint8_t command_length; | ||
| uint8_t address_length; | ||
| uint8_t ce_num; //vio number | ||
| enum mspi_duplex duplex; |
There was a problem hiding this comment.
Same as endianness, we only support half-duplex for now.
There was a problem hiding this comment.
yes, but as we've discussed in the meeting about these structures some of the fields we are not using right now but will be used in the future
There was a problem hiding this comment.
But why add them now? They can be added when we add support for other modes, so when those fields can actually be used. It's not like those structures need to have everything included now, they can be changed in the future.
Also, I am afraid, that if we add it now and then we release it that way, some people may try to use it, because they will think other modes are supported.
| enum mspi_cpp_mode cpp; | ||
| enum mspi_endian endian; | ||
| bool hold_ce; | ||
| nrfe_mspi_polarity_t ce_polarities[NRFE_MSPI_CE_PINS_MAX]; |
There was a problem hiding this comment.
Why are we sending multiple CE polarities, if there is only one CE (ce_num)?
There was a problem hiding this comment.
we have to send polarities of all the pins so that VPR knows how to set them to inactive state when sending data to only one of them. ce_num is the index of the device with which we want to talk.
There was a problem hiding this comment.
I guess we have different understandings of the configuration phase, so let me explain my point of view. Zephyr's MSPI driver API has two configuration functions: mspi_config and mspi_dev_config. mspi_config is used to configure the driver itself (doc: "Configure a MSPI controller."), and it takes data from DTS, which makes sense because the driver can be limited by the hardware (DTS files are supposed to describe hardware). So in DTS files you have specified CE lines that COULD be used by the driver user, meaning which hardware pins can be used by the driver as CE. Not all of them have to be used, it is just a possibility on a given soc.
Then, we also have mspi_dev_config, which should be used to configure a peripheral MSPI device (doc: "Configure a MSPI controller with device-specific parameters."), and it takes the mspi_dev_cfg structure (which is specified by the driver's user), which contains info, which CE PIN should be used, of the ones that are possible, and its polarity. So, we can set the pin to its inactive state in the driver as soon as the device is configured by the user.
I would assume, that all devices should or even must be configured before the first TX/RX happens, because I cannot imagine a scenario when some device has to be configured after we've already sent something to another device. If there is such case, let me know, it is possible I am not aware of something. But otherwise, I would send just one pin number and one pin polarity in nrfe_mspi_xfer_config_t. Sure, there might be a case for specifying the polarity of all possible CE pins at once, which we do not know of yet, but IF such case appears, THEN we will worry about it.
include/drivers/mspi/nrfe_mspi.h
Outdated
| nrfe_mspi_opcode_t opcode; /* nrfe_mspi_opcode */ | ||
| uint8_t command_length; | ||
| uint8_t address_length; | ||
| uint8_t ce_num; //vio number |
There was a problem hiding this comment.
Nit: you can just rename it to ce_vio_pin or ce_vio_num, instead of adding this comment.
| pinctrl_soc_pin_t pin[NRFE_MSPI_PINS_MAX]; | ||
| } nrfe_mspi_pinctrl_soc_pin_t; | ||
|
|
||
| typedef struct { | ||
| nrfe_mspi_opcode_t opcode; /* nrfe_mspi_opcode */ |
There was a problem hiding this comment.
Have you tried that it works when opcode is defined as an enum in these structures, instead of uint8_t? I am skeptical about it, especially because I just checked -fshort-enums compiler option that you use, and I can't find it as an option for RISC-V, only for ARM. And you add it as a compilation flag in FLPR config file.
| typedef struct __packed { | ||
| uint8_t opcode; /* nrfe_mspi_opcode */ | ||
| typedef enum { | ||
| NRFE_MSPI_POL_UNDEFINED = 0, |
There was a problem hiding this comment.
Is this state needed? mspi_dev_cfg has both the CE number (so which CE should be used for this device) and CE polarity, so we can set the inactive level immediately when configuring a device.
| NRFE_MSPI_TX, | ||
| NRFE_MSPI_TXRX, | ||
| NRFE_MSPI_CONFIG_PINS, /*nrfe_mspi_pinctrl_soc_pin_t*/ | ||
| NRFE_MSPI_CONFIG_XFER, /*nrfe_mspi_xfer_config_t*/ |
There was a problem hiding this comment.
Nit: I would rename it to NRFE_MSPI_CONFIG_DEV, because it is more similar to mspi_dev_cfg than to mspi_xfer.
mif1-nordic
force-pushed
the
Ipc_Reduced_Structures
branch
from
000dcf7 to
61ce62d
Compare
kobj-types-enum.h was generated after VPR asm_gen, moved asm_gen to POST_BUILD. Signed-off-by: Michal Frankiewicz <[email protected]>
mif1-nordic
force-pushed
the
Ipc_Reduced_Structures
branch
from
61ce62d to
3d095bb
Compare
added mode support for SINGLE,QUAD,QUAD_1_4_4,QUAD_1_1_4 and custom Ipc mspi structures Signed-off-by: Michal Frankiewicz <[email protected]> Signed-off-by: Jakub Zymelka <[email protected]>
Added reactions to all mspi Ipc messages but NRFE_MSPI_TXRX and NRFE_MSPI_TX. The data is stored in local structures for later use. Signed-off-by: Michal Frankiewicz <[email protected]>
mif1-nordic
force-pushed
the
Ipc_Reduced_Structures
branch
from
3d095bb to
dffb578
Compare
Memory footprint analysis revealed the following potential issuesapplications.nrf_desktop.zdebug[nrf5340dk/nrf5340/cpuapp]: RAM size increased by 512[B] in comparison to the main[874962f] branch. - link (cc: @MarekPieta) Note: This message is automatically posted and updated by the CI (latest/sdk-nrf/PR-19877/16) |
Added MSPI_TX reaction to NRFE_MSPI_TXRX and NRFE_MSPI_TX. Added HRT mspi TX functionality. Signed-off-by: Michal Frankiewicz <[email protected]>
mif1-nordic
force-pushed
the
Ipc_Reduced_Structures
branch
from
dffb578 to
e5039ce
Compare
Implemented smaller structures and reduced ammount of opcodes in IPC Signed-off-by: Michal Frankiewicz <[email protected]>
mif1-nordic
force-pushed
the
Ipc_Reduced_Structures
branch
from
e5039ce to
a674e62
Compare
Implemented smaller structures and reduced ammount of opcodes in IPC Signed-off-by: Michal Frankiewicz <[email protected]>
mif1-nordic
force-pushed
the
Ipc_Reduced_Structures
branch
from
a674e62 to
b749c23
Compare
Depends on #19449