Preparatory patches for NXP platforms transition to Zephyr native drivers by LaurentiuM1234 · Pull Request #8859 · thesofproject/sof

LaurentiuM1234 · 2024-02-13T09:23:22Z

This PR is a combination of patches cherry-picked from #8520 and #8844. These patches don't require a Zephyr hash bump and are needed for the transition of NXP platforms to native Zephyr drivers.

lyakh · 2024-02-14T07:08:36Z

+	.plat_data = {
+ .dir = DMA_DIR_MEM_TO_DEV | DMA_DIR_DEV_TO_MEM,
+ .devs = DMA_DEV_SAI,
+ .channels = 64, /* TODO: this shouldn't be hardcoded */


a device tree property?

no such property ATM (dma-channels can't be used for EDMA)

this can be added later in DT and updated here when Zephyr side is ready.

Suggested change
.channels=64, /* TODO: this shouldn't be hardcoded */
.channels=64, /* TODO: add a new DT property in Zephyr */

About this: we do already have a DT property for this (there's 2 channel count-related ones actually). The issue is that they don't necessarily tell you what the max number of channels* is (which I'm assuming should be the value we want here). Anyways, for now I'd prefer sticking with the generic comment until we fix this via making use of (one) the already existing two properties.
(*) or the number of contiguous channels

It's not because you're not sure yet how to fix this TODO that you should be holding back the information that you already have. Comments are cheap and you already wrote them just now. Why not have this knowledge directly in the source code? Readers rarely go all the way back to the code review. You may or may not be the one who will fix this later and even if it's you, you will most likely have forgotten half of it in a few months as we typically all do.
Don't get me wrong: I hate pointlessly verbose comments that just paraphrase the code - which should ideally be so clear and easy to read that it does not require any comment. But this situation here looks like the exact opposite.
My 2 cents.

Fair point, tweaked the comment to make it a bit more useful while still keeping it short

lgirdwood

LGTM. Just some questions on one-shot

lgirdwood · 2024-02-19T13:39:41Z

 return ret;
 	}

+	if (IS_ENABLED(CONFIG_DMA_NXP_SOF_HOST_DMA)) {


I would say this is probably a common feature - i.e. we allocate a DMA channels and then should be able to use it for several one shot copies (with different SRC/DST addresses).
@kv2019i @lyakh @abonislawski can you see any problems if we always reload src/dst/size for every one shot copy ?

Now this is performed for all platforms. Let's see if this breaks any tests. If not, we can probably keep it this way for now.

lgirdwood · 2024-02-19T13:42:13Z

 dma_block_cfg->block_size = config->elem_array.elems[i].size;
 dma_block_cfg->source_address = config->elem_array.elems[i].src;
 dma_block_cfg->dest_address = config->elem_array.elems[i].dest;
+ if (IS_ENABLED(CONFIG_DMA_NXP_EDMA)) {


should this be a common thing we should always do ?
@abonislawski @kv2019i @lyakh pls comment.

Looks like it should be safe to make this generic - Intel drivers don't use .source_addr_adj and .dest_addr_adj

LaurentiuM1234 · 2024-02-21T09:50:07Z

V2 changes

source_addr_adj and dest_addr_adj are now set for all platforms
DMAC configuration is updated for all platforms inside host-zephyr
State transition is now done after the trigger command is processed and if there's no errors. This way, we can keep the old state if an error occurs during processing.

lgirdwood · 2024-02-23T12:03:44Z

+	.plat_data = {
+ .dir = DMA_DIR_MEM_TO_DEV | DMA_DIR_DEV_TO_MEM,
+ .devs = DMA_DEV_SAI,
+ .channels = 64, /* TODO: this shouldn't be hardcoded */


this can be added later in DT and updated here when Zephyr side is ready.

Since the DMAC is configured in a somewhat different way in the native case vs the non-native case, add a native version of the ipc_get_page_descriptors() function and remove the Zephyr-specific functions from the non-native version of said function. Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

With every copy() operation, the source and destination addresses keep getting modified. As such, for each re-configuration performed during host_copy_one_shot(), the HOST DMAC needs to be made aware of these changes. This commit changes host_copy_one_shot() such that on each dma_config() call the DMAC driver receives the updated addresses and size. Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

Source and destination addresses cannot be NULL. As such, set them to the values found in the first element of hd->config's elem_array. This is fine to do because the host component uses only 1 block. Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

LaurentiuM1234 · 2024-03-01T11:48:48Z

V3 changes

Convert host address to DSP local address before using it to copy host ring buffer.
The DAI trigger function now also checks if the component is already in the requested state.

lgirdwood · 2024-03-01T16:48:05Z

Lot of red on the CI results - lets run again to make sure it's not a DUT issue.

lgirdwood · 2024-03-01T16:48:12Z

SOFCI TEST

Since the Zephyr SAI driver is now enabled, add an entry in the zephyr_dev array, which will resolve to multiple "struct device *", one for each SAI node specified in the DTS. Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

LaurentiuM1234 · 2024-03-04T14:55:20Z

Noticing some failures in the internal Intel CI. @marc-hb @lgirdwood would it be possible to dump the output of the job here (or at least the part that fails). Hopefully that'll give us a clue as to which part of this PR breaks it.

lgirdwood · 2024-03-04T15:12:31Z

Noticing some failures in the internal Intel CI. @marc-hb @lgirdwood would it be possible to dump the output of the job here (or at least the part that fails). Hopefully that'll give us a clue as to which part of this PR breaks it.

@wszypelt @lrudyX can you provide the logs or its it a false positive result ?

marc-hb · 2024-03-04T23:06:47Z

I had a quick look at the internal version of https://sof-ci.01.org/sof-pr-viewer/#/build/PR8859/build13666634 and it does not look like false positives because less than 10% of the tests failed and I didn't see any of the usual signs of false positives, I mean no "network timeout" and no "directory not found" error message. Most failing tests seem to have "loopback" in the name in case that helps.

would it be possible to dump the output of the job here (or at least the part that fails). Hopefully that'll give us a clue as to which part of this PR breaks it.

We used to have that, then a forced change of web provider broke the (too complex) site javascript and no one still working on the project knows how to fix it (internal issue 307). Apologies.
I don't think the logs would have helped you much this time though because I couldn't find any obvious error message in them on the internal site, just "test xxxloopbackyyy FAILED". Maybe some threshold was missed?

HOWEVER there are many sof-test failures in MTL https://sof-ci.01.org/sofpr/PR8859/build3069/devicetest/index.html and in CAVS https://sof-ci.01.org/sofpr/PR8859/build3070/devicetest/index.html and you should have complete logs there. Please start there instead. Unlike the other tests, sof-test is open-source so you might even be able to reproduce?

Obviously, smaller PRs also help:
https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#defining-smaller-prs

LaurentiuM1234 · 2024-03-06T11:12:51Z

V4 changes

State change on DAI trigger is now IPC3-only. Let's see if the CI will still complain.

LaurentiuM1234 · 2024-03-06T13:06:44Z

Internal CI seems to be green. Noticing some failures on ACE and CAVS-related public tests. @marc-hb do you happen to know if these could be related to this PR? I'm assuming not since they occurred in #8903 as well.

lgirdwood

@LaurentiuM1234 your code looks clean and platform specific and I would not expect a breaker for other platforms, but I could be wrong as the CI results are not yet showing, but pls do check again in an hour or two.

marc-hb · 2024-03-07T04:09:39Z

We used to have that, then a forced change of web provider broke the (too complex) site javascript and no one still working on the project knows how to fix it (internal issue 307). Apologies.

Another unfortunate side-effect: it is NOT possible to find earlier test results after a force push. When discussing test results, always "save" the URL in the comments.

Noticing some failures on ACE and CAVS-related public tests. @marc-hb do you happen to know if these could be related to this PR?

For years our suspend/resume pass rate has been between 80% and 100%. If you see observe anything in that range then it's fairly safe to assume no regression from this PR.

There is also one frequent pause/resume failure on MTL, I think there's already a bug filed for it.

Since the EDMA and HOST DMA nodes have been introduced to the i.MX93 overlay, add entries in the dma array which will create a struct dma for each of these DMACs. Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

Since now there's a Zephyr driver for NXP's SAI, the dai_set_config() should be modified to also allow the configuration of the SAI. As such, this commit introduces a new case for NXP's SAI that does exactly that. Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

Some DMACs (e.g: NXP's EDMA) can automatically adjust the source and destination addresses upon transfer completion. As such, we need to indicate how the adjustment should be performed. In the case of playback, the source address should be decremented, while in the case of capture, the destination address should be decremented. Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

Currently, the DAI component's state is not updated on dai_trigger() operation, which leads to pipeline_comp_copy() skipping the dai_copy() operation (since the DAI component never transitions to the ACTIVE state). To fix this, add a state transition in dai_comp_trigger_internal(). Also, make sure not to trigger the DAI component if already in the requested state. Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

In the case of some DAIs, the DMA slot may be encoded differently in the DAI configuration handshake. As such, we can't count on the fact that the handshake itself can be used as the DMA slot. To fix this, add a function which parses the handshake and allows each DAI to use its own encoding of the DMA slot inside the handshake. Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

The native SAI and ESAI drivers use a different handshake encoding. As such, when using native Zephyr drivers use a different function for decoding the channel from the handshake. Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

lgirdwood · 2024-03-08T14:19:36Z

@iuliana-prodan @dbaluta good with the latest update ?

dbaluta · 2024-03-08T14:31:41Z

@lgirdwood all good on our side.

dbaluta reviewed Feb 13, 2024
View reviewed changes

Comment threadsrc/audio/dai-zephyr.c Outdated

LaurentiuM1234 force-pushed the native_drivers_transition_prep_patches branch from 8686295 to 10c85b6Compare February 13, 2024 09:52

LaurentiuM1234 marked this pull request as ready for review February 13, 2024 10:53

LaurentiuM1234 requested review from abonislawski, iuliana-prodan, kv2019i, lbetlej, lgirdwood, lyakh, mmaka1 and plbossart as code owners February 13, 2024 10:53

lyakh reviewed Feb 14, 2024
View reviewed changes

lgirdwood added this to the v2.9 milestone Feb 14, 2024

LaurentiuM1234 mentioned this pull request Feb 15, 2024
audio: host-zephyr: Allocate DMA block configuration on heap #8867
Merged

lgirdwood reviewed Feb 19, 2024
View reviewed changes

LaurentiuM1234 mentioned this pull request Feb 19, 2024
[DNM] dai: zephyr: add support for parsing new SAI/ESAI handshake encoding #8844
Closed

LaurentiuM1234 force-pushed the native_drivers_transition_prep_patches branch from 10c85b6 to bcedadeCompare February 21, 2024 09:47

lyakh reviewed Feb 22, 2024
View reviewed changes

Comment threadsrc/ipc/ipc3/host-page-table.c
Comment threadsrc/ipc/ipc3/host-page-table.c

LaurentiuM1234 force-pushed the native_drivers_transition_prep_patches branch from bcedade to 4ff4ccbCompare February 22, 2024 13:15

lyakh reviewed Feb 23, 2024
View reviewed changes

Comment threadsrc/audio/dai-zephyr.c

lgirdwood approved these changes Feb 23, 2024
View reviewed changes

LaurentiuM1234 added 3 commits March 1, 2024 13:13

LaurentiuM1234 force-pushed the native_drivers_transition_prep_patches branch from 4ff4ccb to 3c9f723Compare March 1, 2024 11:42

iuliana-prodan reviewed Mar 1, 2024
View reviewed changes

Comment threadsrc/lib/dai.c
Comment threadsrc/audio/dai-zephyr.c
Comment threadsrc/audio/host-zephyr.c

iuliana-prodan mentioned this pull request Mar 1, 2024
nxp: imx8/imx8x: switch to Zephyr native drivers #8863
Merged

LaurentiuM1234 force-pushed the native_drivers_transition_prep_patches branch from 3c9f723 to 855ac8eCompare March 4, 2024 08:37

lib: dai: Add entry for NXP's SAI
ad6bcbd
Since the Zephyr SAI driver is now enabled, add an entry in the zephyr_dev array, which will resolve to multiple "struct device *", one for each SAI node specified in the DTS. Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

kv2019i modified the milestones: v2.9, v2.10 Mar 4, 2024

LaurentiuM1234 force-pushed the native_drivers_transition_prep_patches branch from 855ac8e to 2bdb428Compare March 6, 2024 11:11

lgirdwood reviewed Mar 6, 2024
View reviewed changes

Comment threadsrc/audio/dai-zephyr.c

iuliana-prodan approved these changes Mar 6, 2024
View reviewed changes

LaurentiuM1234 mentioned this pull request Mar 7, 2024
Re-introducing state change in dai_comp_trigger_internal() breaks some tests from Intel CI #8920
Closed

LaurentiuM1234 added 6 commits March 7, 2024 23:05

lib: dma: Add entries for i.MX93's EDMA and HOST DMAs
bc832ee
Since the EDMA and HOST DMA nodes have been introduced to the i.MX93 overlay, add entries in the dma array which will create a struct dma for each of these DMACs. Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

LaurentiuM1234 force-pushed the native_drivers_transition_prep_patches branch from 2bdb428 to a0a034aCompare March 7, 2024 21:06

lgirdwood approved these changes Mar 8, 2024
View reviewed changes

lgirdwood merged commit 1f87ffc into thesofproject:mainMar 8, 2024

LaurentiuM1234 mentioned this pull request Mar 11, 2024
nxp: imx93_a55: Switch to timer domain and Zephyr native drivers #8520
Merged

	.channels=64, /* TODO: this shouldn't be hardcoded */
	.channels=64, /* TODO: add a new DT property in Zephyr */

Uh oh!

Conversation

LaurentiuM1234 commented Feb 13, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LaurentiuM1234Feb 14, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LaurentiuM1234 commented Feb 21, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LaurentiuM1234 commented Mar 1, 2024

Uh oh!

lgirdwood commented Mar 1, 2024

Uh oh!

lgirdwood commented Mar 1, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LaurentiuM1234 commented Mar 4, 2024

Uh oh!

lgirdwood commented Mar 4, 2024

Uh oh!

marc-hb commented Mar 4, 2024

Uh oh!

LaurentiuM1234 commented Mar 6, 2024

Uh oh!

LaurentiuM1234 commented Mar 6, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marc-hb commented Mar 7, 2024

Uh oh!

lgirdwood commented Mar 8, 2024

Uh oh!

dbaluta commented Mar 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

LaurentiuM1234 commented Feb 13, 2024•
edited
Loading

LaurentiuM1234Feb 14, 2024•
edited
Loading

LaurentiuM1234 commented Mar 6, 2024•
edited
Loading