drivers: imx: Dummy DMA driver by paulstelian97 · Pull Request #1838 · thesofproject/sof

paulstelian97 · 2019-09-17T12:50:47Z

For the communication between host and DSP a dummy DMA driver which
simply does memcpy is sufficient (given that a sizeable amount of
physical memory is shared)

This commit holds the initial implementation of the dummy DMA driver.

Signed-off-by: Paul Olaru paul.olaru@nxp.com

paulstelian97 · 2019-09-18T07:08:30Z

Remaining Checkpatch warnings are probably benign (long messages making lines too long and that one usage of memcpy which comes from the design of the driver). Otherwise, unless there is another issue this should be mergeable.

jajanusz · 2019-09-18T12:19:25Z

@paulstelian97 Yea WARNING: line over 80 characters is result of outdated checkpatch, it was lifted in kernel's checkpatch - if last argument is string literal it can have over 80chars, so feel free to ignore that one. But please fix the one that recommends to use memcpy_s instead of memcpy.

paulstelian97 · 2019-09-18T12:23:32Z

@paulstelian97 Yea WARNING: line over 80 characters is result of outdated checkpatch, it was lifted in kernel's checkpatch - if last argument is string literal it can have over 80chars, so feel free to ignore that one. But please fix the one that recommends to use memcpy_s instead of memcpy.

I'd argue that one is hard to actually fix in a semantically meaningful way. I don't have anything other than the elem itself. So I should just do something like assert(!memcpy_s(dest, size, src, size)) or the like? Just to make Checkpatch happy? (It doesn't introduce any actual safety)

paulstelian97 · 2019-09-18T12:34:35Z

Pushed, also noticed I forgot to remove the "[DO NOT MERGE]" tag from this one.

lgirdwood

The dummy driver looks like generic code that may be useful for similar platforms so should probably be drivers/generic directory ?
Also need more commit message on your first patch and the dummy driver could do with more comments explaining what it's doing and why. A top level description in the comments would be good too explaining the sue case since it may not be obvious.

paulstelian97 · 2019-09-18T13:39:02Z

The dummy driver looks like generic code that may be useful for similar platforms so should probably be drivers/generic directory ?

Will do. For the moment the i.MX platform is the only one where this can be used (on the current Intel ones the DSP cannot directly access host memory, can it?)

Also need more commit message on your first patch and the dummy driver could do with more comments explaining what it's doing and why. A top level description in the comments would be good too explaining the sue case since it may not be obvious.

For the first patch it isn't actually very obvious even to me but I needed to add it because otherwise I couldn't get actual, physical host addresses with HOST_PTABLE disabled (the descriptor would just put NULL as the first host address and I'd have no means to get the real one). The patch is actually larger than it should have been because it includes fixing some compile errors that show up by enabling that config. Any suggestions?

For the driver itself, I will think about it and will do.

dbaluta · 2019-10-08T15:02:50Z

@paulstelian97 this is in a good shape. We need one more review to push it.

@tlauda @jajanusz @plbossart @groncarolo @jlacla

tlauda

Small nitpick + copy size, which I think is important. Also checkpatch is complaining.

Enabling this config option changes the way the SG elems are generated, such that the host addresses in the elems are actual, physical addresses. This is required by the Dummy DMA driver, which can only operate with said physical addresses. This commit enables the config option and fixes compile errors which appeared after this enabling. Signed-off-by: Paul Olaru <paul.olaru@nxp.com>

The dummy DMA is a software-based DMA which acts as a host DMA. This one can be implemented in software alone because all host physical address space (at least the RAM portion) is accessible directly from the DSP on this platform. The driver works by taking the physical addresses for both the source and destination addresses from the elems. This works because no paging is enabled on the DSP side, and because the page tables interpretation allows these elements to actually have physical addresses on the host side. Given these addresses, the copy itself is done synchronously on the DSP within a memcpy_s call. Signed-off-by: Paul Olaru <paul.olaru@nxp.com>

This is to optimize memory consumption from this driver. With 32 channels we would have needed a memory block >1024 bytes in size to allocate the dma_chan_data array. However 16 channels (which means allocating a smaller block) should be enough for everyone. Signed-off-by: Paul Olaru <paul.olaru@nxp.com>

This driver can apply to any platform with the buffers in shared memory, not just to i.MX. Signed-off-by: Paul Olaru <paul.olaru@nxp.com>

This config option also encapsulates a required dependency on HOST_PTABLE. Signed-off-by: Paul Olaru <paul.olaru@nxp.com>

dbaluta · 2019-10-11T11:07:52Z

@lgirdwood @tlauda this is working well for us now. I think we fixed all the comments reported.

Stale review, changes have been applied

jajanusz · 2019-10-11T12:21:17Z

Checkpatch errors safe to ignore - 80 chars limit doesn't apply for literals in kernel (we should update our checkpatch to match it)

paulstelian97 requested review from lbetlej, lgirdwood, mmaka1 and plbossart as code owners September 17, 2019 12:50

paulstelian97 requested review from dbaluta and tlauda September 17, 2019 12:51

paulstelian97 force-pushed the topic/imx/dummydma branch 2 times, most recently from c6d0a22 to 3c52432 Compare September 17, 2019 13:34

paulstelian97 commented Sep 17, 2019

View reviewed changes

Comment thread src/drivers/imx/dummy-dma.c Outdated

paulstelian97 force-pushed the topic/imx/dummydma branch from 3c52432 to 50edfea Compare September 18, 2019 06:54

paulstelian97 requested review from akloniex, bardliao, jajanusz and xiulipan as code owners September 18, 2019 06:54

paulstelian97 force-pushed the topic/imx/dummydma branch 3 times, most recently from 4a8c702 to 53a767e Compare September 18, 2019 07:07

paulstelian97 removed request for akloniex, bardliao and xiulipan September 18, 2019 10:48

paulstelian97 force-pushed the topic/imx/dummydma branch 2 times, most recently from 9ed1ae9 to b30629b Compare September 18, 2019 12:34

paulstelian97 force-pushed the topic/imx/dummydma branch from b30629b to 520eafd Compare September 18, 2019 12:41

lgirdwood requested changes Sep 18, 2019

View reviewed changes

paulstelian97 force-pushed the topic/imx/dummydma branch from 520eafd to 2bca0f5 Compare September 18, 2019 13:55

dbaluta reviewed Oct 4, 2019

View reviewed changes

Comment thread src/drivers/imx/dummy-dma.c Outdated

paulstelian97 force-pushed the topic/imx/dummydma branch from 2e26e8d to 7124a84 Compare October 4, 2019 10:46

dbaluta reviewed Oct 4, 2019

View reviewed changes

Comment thread src/drivers/imx/dummy-dma.c Outdated

paulstelian97 force-pushed the topic/imx/dummydma branch from 7124a84 to ea5c27a Compare October 4, 2019 11:28

dbaluta reviewed Oct 4, 2019

View reviewed changes

Comment thread src/drivers/Kconfig Outdated

tlauda suggested changes Oct 4, 2019

View reviewed changes

paulstelian97 force-pushed the topic/imx/dummydma branch 3 times, most recently from 7f51f12 to b41fd4e Compare October 8, 2019 15:01

dbaluta approved these changes Oct 8, 2019

View reviewed changes

paulstelian97 removed the request for review from libinyang October 8, 2019 15:29

tlauda suggested changes Oct 8, 2019

View reviewed changes

Comment thread src/drivers/imx/dummy-dma.c Outdated

Comment thread src/drivers/imx/dummy-dma.c Outdated

paulstelian97 force-pushed the topic/imx/dummydma branch 2 times, most recently from ba8fdad to c759beb Compare October 9, 2019 15:33

paulstelian97 requested review from lgirdwood and tlauda October 10, 2019 11:28

Paul Olaru added 5 commits October 10, 2019 17:06

drivers: dummydma: Move the driver file to generic/

8be0255

This driver can apply to any platform with the buffers in shared memory, not just to i.MX. Signed-off-by: Paul Olaru <paul.olaru@nxp.com>

drivers: generic: dummydma: Add config option

28bdfe3

This config option also encapsulates a required dependency on HOST_PTABLE. Signed-off-by: Paul Olaru <paul.olaru@nxp.com>

paulstelian97 force-pushed the topic/imx/dummydma branch from c759beb to 28bdfe3 Compare October 10, 2019 14:08

tlauda approved these changes Oct 11, 2019

View reviewed changes

jajanusz approved these changes Oct 11, 2019

View reviewed changes

jajanusz merged commit 51a6c9c into thesofproject:master Oct 11, 2019

paulstelian97 deleted the topic/imx/dummydma branch October 23, 2019 15:05

Conversation

paulstelian97 commented Sep 17, 2019

Uh oh!

Uh oh!

paulstelian97 commented Sep 18, 2019

Uh oh!

jajanusz commented Sep 18, 2019

Uh oh!

paulstelian97 commented Sep 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulstelian97 commented Sep 18, 2019

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

paulstelian97 commented Sep 18, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dbaluta commented Oct 8, 2019

Uh oh!

tlauda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dbaluta commented Oct 11, 2019

Uh oh!

jajanusz commented Oct 11, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

paulstelian97 commented Sep 18, 2019 •

edited

Loading