Skip to content

Conversation

@abonislawski
Copy link
Member

This patch will add dw-dma wrapper for zephyr driver

Opens:

  • sof dma ops names: dma_channel_put_sof? dw_dma_channel_put_zephyr?
  • zephyr/CMakeLists.txt: dma-zephyr.c should be restricted for a specific platform/s?

@teburd
Copy link

teburd commented Mar 31, 2022

This seems like a nice way to adapt the SOF <-> Zephyr APIs

Copy link
Collaborator

Choose a reason for hiding this comment

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

this doesn't look right - dma_block_cfg has been incremented block_count times in the loop above. Also, I don't think that parameter is "consumed" by dma_config() so we have to free it in the success case too.

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

Needs to be more generic and not per DMA driver.

@sys-pt1s
Copy link

Can one of the admins verify this patch?

@lgirdwood
Copy link
Member

@abonislawski any update ?
My expectation is that for each DMA API call we have today we would map current users onto the Zephyr version of that call and wrap the SOF version (for existing xtos users).
e.g. for dma_stop()

 grep -rn " dma_stop(" --include=*.c
src/trace/dma-trace.c:375:		err = dma_stop(d->dc.chan);
src/audio/host.c:604:		ret = dma_stop(hd->chan);
src/audio/dai.c:782:		ret = dma_stop(dd->chan);
src/audio/dai.c:786:		ret = dma_stop(dd->chan);
src/probe/probe.c:175:	err = dma_stop(dma->dc.chan);
src/probe/probe.c:177:		tr_err(&pr_tr, "probe_dma_deinit(): dma_stop() failed");

These would be mapped to the native Zephyr API call i.e. all user would be converted to use

int dma_stop(const struct device *dev, uint32_t channel)

We would need to convert the existing SOF dma calls to the Zephyr APIs as the first part of this work and then add in support for linking with the Zephyr implementation of this API.

@juimonen same applies to SSP and other DAI drivers.

@abonislawski
Copy link
Member Author

abonislawski commented May 3, 2022

@lgirdwood this approach will require to switch all dma or all dai drivers once. With sof api -> zephyr api wrapper we can merge and test each driver separately, it is also possible to use for example one dai driver from sof and one from zephyr.
Can we go this way until all drivers are ready and then in separate final step switch dma/dai api to native zephyr?

@lgirdwood
Copy link
Member

@lgirdwood this approach will require to switch all dma or all dai drivers once. With sof api -> zephyr api wrapper we can merge and test each driver separately, it is also possible to use for example one dai driver from sof and one from zephyr. Can we go this way until all drivers are ready and then in separate final step switch dma/dai api to native zephyr?

I want to avoid having an intermediate stage to develop and validate, I think it's fine to transition in steps with a Kconfig option "Use Zephyr drivers" with default value n. This means that all the local xtos based driver will be used unless this Kconfig is "y".
i.e. stages with Zephyr drivers enabled.

  1. HDA DMA and HDA/HDMI audio - only HDA and trace can be tested.
  2. DW DMA and DMIC, SSP - DMIC and SSP can also be tested.
  3. Remaining Intel DAIs - all features now enabled.

Copy link
Member Author

@abonislawski abonislawski left a comment

Choose a reason for hiding this comment

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

@lgirdwood please take a look, added early version but few things still needs to be changed

added host_zephyr.c and dai_zephyr.c, so both HDA and DW-DMA (similar changes)
sof dma lib not divided yet

Please ignore _sof functions, it will be reverted later

Copy link
Member Author

Choose a reason for hiding this comment

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

The main issue is dma_config, Im still using old config and translating it to the dma_config at the end, which is obviously wrong but the old config contain some fields which are not available in the new config, like is_scheduling_source, not sure if we can ignore such fields?

Copy link
Member Author

Choose a reason for hiding this comment

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

dma_get_attrubite is still missing in new version, Zephyr API does not support this.
Not sure if we can use here defines directly?

@abonislawski abonislawski changed the title dma: cavs: switch to Zephyr driver for dw-dma [RFC] dma: switch to native Zephyr API for dw-dma & HDA May 12, 2022
@abonislawski abonislawski requested review from lgirdwood and lyakh May 24, 2022 07:23
@abonislawski abonislawski changed the title [RFC] dma: switch to native Zephyr API for dw-dma & HDA dma: switch to native Zephyr API for dw-dma & HDA May 24, 2022
@lgirdwood lgirdwood added the P1 Blocker bugs or important features label May 24, 2022
@lgirdwood
Copy link
Member

Both this and #5731 are/were abstracting at the wrong point. The abstraction points for DMA are src/audio/dai.c and src/audio/host.c. This needs to be mechanically changed to

  1. create a dai-zephyr.c (starts to use native Zephyr APIs) and copy dai.c to dai-legacy.c (this last file is exact copy of dai.c today plus any legacy dai logic from lib/dai.c).
  2. same as 1) for host.c
    Good for call if needed.

One more thing, lets not use _sof suffixes for the legacy APIs. We can use _legacy instead and this would also align with the file name.

@dbaluta
Copy link
Collaborator

dbaluta commented May 25, 2022

@abonislawski , maybe this could be useful.

FYI: zephyrproject-rtos/zephyr@2035f3a

@abonislawski
Copy link
Member Author

Both this and #5731 are/were abstracting at the wrong point. The abstraction points for DMA are src/audio/dai.c and src/audio/host.c. This needs to be mechanically changed to

  1. create a dai-zephyr.c (starts to use native Zephyr APIs) and copy dai.c to dai-legacy.c (this last file is exact copy of dai.c today plus any legacy dai logic from lib/dai.c).
  2. same as 1) for host.c
    Good for call if needed.

@lgirdwood please check carefully, this is how it works now with native zephyr api in src/audio/dai_zephyr.c and src/audio/host_zephyr.c, I can also rename them to dai.c/host.c and old version to dai-legacy.c/host-legacy.c

@lgirdwood
Copy link
Member

@lgirdwood please check carefully, this is how it works now with native zephyr api in src/audio/dai_zephyr.c and src/audio/host_zephyr.c, I can also rename them to dai.c/host.c and old version to dai-legacy.c/host-legacy.c

Yes please - we really need to make it obvious what files are Zephyr and what files are legacy. Also please use the _legacy suffix. This gives us a clear dividing line.

@abonislawski abonislawski requested review from a team, RanderWang, mrajwa and singalsu as code owners May 26, 2022 17:55
@abonislawski abonislawski force-pushed the zephyr_dma branch 5 times, most recently from 8b35ad7 to a7089d0 Compare May 26, 2022 19:00
@abonislawski
Copy link
Member Author

abonislawski commented May 26, 2022

@lgirdwood updated with new legacy/zephyr suffix, also added CONFIG_ZEPHYR_NATIVE_DRIVERS as you adviced previously so we can enable it later when everything is ready along with dai PR
hopefully fixed all checkpatch issues etc

@abonislawski abonislawski requested review from kv2019i and lgirdwood and removed request for a team and lgirdwood May 26, 2022 19:16
Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

This is a good first step toward full native Zephyr. Next things (in subsequent PRs) are

  1. Reduce other SOF native RTOS APIs in host-zephyr.c and dai-zephyr.c and use the Zephyr versions, i.e for PM, notifiers memory.
  2. Fix the DAI, DMA init to have const tables of DT name strings and topology IDs so that teh CC does more of the init and not the runtime code.

@lgirdwood
Copy link
Member

SOFCI TEST

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it should be possible to split this patch, making the renaming to *legacy file and function names a separate patch?

@abonislawski
Copy link
Member Author

@lyakh I will fix most of your comments today but please note that this PR is not about dai&host refactor

@abonislawski abonislawski force-pushed the zephyr_dma branch 2 times, most recently from f48e9e3 to a54a272 Compare May 31, 2022 11:03
This patch will add new versions of host and dai components
with Zephyr native DMA support

Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>
Get device binding for DW-DMA & HDA drivers using Zephyr API

Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>
@lgirdwood lgirdwood merged commit a145313 into thesofproject:main May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P1 Blocker bugs or important features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants