Skip to content

drivers: dma: Add dts dma consumer support and generic DMA support for stm32 #13364

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.

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

Closed
wants to merge 8 commits into from

Conversation

cybertale
Copy link
Collaborator

@cybertale cybertale commented Feb 13, 2019

This PR adds driver support for DMA on f0/f1/f2/f3/f4/l0/l4 series stm32.
This should have been working on the f7 series SoCs, but there are some weird bugs inside the code that I cannot resolve.
There are two kinds of IP blocks are used across these stm32, one is the one that has been used on F2/F4/F7 series, and the other one is the one that has been used on F0/F1/F3/L0/L4 series.

Memory to memory transfer is only supported on the second DMA on F2/F4/F7 with 'st,mem2mem' to be declared in dts.

This driver depends on k_malloc to allocate memory for stream instances, so CONFIG_HEAP_MEM_POOL_SIZE must be big enough to hold them.

Common parts of the driver are in dma_stm32.c and SoC related parts are implemented in dma_stm32_v*.c.

This driver has been tested on multiple nucleo boards, including NUCLEO_F091RC/F103RB/F207ZG/F302R8/F401RE/L073RZ/L476RG with the loop_transfer and chan_blen_transfer test cases.

Fixes #9193

Signed-off-by: Song Qiang [email protected]

@cybertale cybertale requested a review from erwango as a code owner February 13, 2019 11:01
@cybertale cybertale changed the title [WIP]dts: arm: st: stm32: Add dts support for dma of stm32 [WIP]dts: arm: st: stm32: Add generic DMA support of stm32 Feb 13, 2019
@cybertale cybertale changed the title [WIP]dts: arm: st: stm32: Add generic DMA support of stm32 [WIP]dts: arm: st: stm32: Add generic DMA support for stm32 Feb 13, 2019
@cybertale cybertale force-pushed the dma branch 2 times, most recently from ad736c4 to 9790af5 Compare February 13, 2019 11:13
reg = <0x40020000 0x400>;
clocks = <&rcc STM32_CLOCK_BUS_AHB1 0x1>;
status = "disabled";
label = "DMA_0";
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious about the label numeration here. Why not calling fist instance DMA_1?
Usually STM32 devices numeration starts from 1 to fit the reference manuals.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was wanting to name them as DMA_1 and ADC_1 in my first place, while basically there are two reasons:

  1. When I was writing applications, I found that CONFIG_DMA_0_NAME and CONFIG_ADC_0_NAME are defined in Kconfig rather than Kconfig.stm32. So DMA_0 is always the first DMA device no matter what driver we are using from kconfig. And the default content of CONFIG_DMA_0_NAME is 'DMA_0'. If we name it as DMA_1, one has to select the second DMA device in kconfig, which is DMA_1 to enable the first DMA device on STM32, which is DMA1. I think this may bring many bugs in the future.
  2. Userspace applications can be shared between devices if all drivers use DMA_0 as its first device (What the OS wants).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just noticed that all DMA are going to be enabled if DMA is selected, but ADC can be selected which to use. I think someday DMA will have this feature, too, to select which DMA device we want to enable.

Copy link
Member

Choose a reason for hiding this comment

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

1. I found that CONFIG_DMA_0_NAME and CONFIG_ADC_0_NAME are defined in Kconfig rather

It was the same for other devices (UART/SPI/...) and it didn't prevent to set correct name for STM32 instances. Besides, once devices are fully moved to dts, CONFIG_DEV_X_NAME are no more required and deleted (cf serial driver example), so there is no more constraint on numbering.
Using UART_1 in zephyr when user looks for UART_2 in the datasheet is heavily confusing and should be avoided.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree to number it start from 1, and DMA driver should be like this.
While I think my problem was that we configure which serial to use in Kconfig.stm32, but configure which ADC to use in Kconfig under adc folder. It causes naming problems for ADC driver, though not related to DMA driver, ADC now has to be named from 0.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have to merge the selection of which device to use into device's own kconfig for ADC I think, this is going to solve this problem in my head.

@cybertale cybertale force-pushed the dma branch 2 times, most recently from ae26942 to 4146544 Compare February 14, 2019 03:12
@cybertale
Copy link
Collaborator Author

Hi @erwango , I have added dt support and tested with memcpy example on a f4 board, will add support for other socs later. Please have a look if you got sometime, mainly the dma_stm32.c. See if there is anything needs improving.

@zephyrbot
Copy link
Collaborator

zephyrbot commented Feb 14, 2019

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

@cybertale
Copy link
Collaborator Author

cybertale commented Feb 14, 2019

There are the major changes:

  1. Adding dt support of course.
  2. The original driver uses many unneccessary regs computation and passing, I replaced them with direct register access, so less code and cleaner.
  3. Since it's for all stm32 socs and everyone of them may has different number of channels or DMA devices, max_streams is dynamically computed when init, by counting irq lines.
    @erwango

@cybertale cybertale changed the title [WIP]dts: arm: st: stm32: Add generic DMA support for stm32 [WIP]drivers: dma: Add generic DMA support for stm32 Feb 17, 2019
@cybertale cybertale requested a review from galak as a code owner March 30, 2019 13:55
@cybertale cybertale changed the title [WIP]drivers: dma: Add generic DMA support for stm32 drivers: dma: Add generic DMA support for stm32 Apr 1, 2019
@cybertale cybertale requested a review from avisconti as a code owner April 1, 2019 12:50
@avisconti
Copy link
Collaborator

@cybertale
Very interesting work, but it broke the microphone support on ArgonKey.
I think that we need to:

  1. change the i2s_ll_stm32.c because now some DMA info (like the name) are taken from
    the DTS
  2. in board/96b_argonkey I guess that we need to declare dma_2 in DTS
  3. in sample for argonkey I had to define CONFIG_DMA_2 in prj.conf

After having done all of this it refuse to compile because k_malloc() is missing.
Can you help me?

@cybertale
Copy link
Collaborator Author

cybertale commented Apr 1, 2019

@cybertale
Very interesting work, but it broke the microphone support on ArgonKey.
I think that we need to:

1. change the i2s_ll_stm32.c because now some DMA info (like the name) are taken from
   the DTS

2. in board/96b_argonkey I guess that we need to declare dma_2 in DTS

3. in sample for argonkey I had to define CONFIG_DMA_2 in prj.conf

Sure, I noticed only one place and missed the others.

After having done all of this it refuse to compile because k_malloc() is missing.
Can you help me?

Hi @avisconti , thanks for the interest, I forgot to mention that this driver depends on HEAP_MEM_POOL_SIZE to be not zero. Usually 512/1024 will work.

This is because different series of STM32 has different number of DMA channels, and to only allocate channels we need, this driver uses a var 'max_streams' in the dma_stm32_data struct to count how many channels do we have on the current instance, and uses k_malloc to allocate memory for these structures.
In kconfig.stm32, I added 'depends on HEAP_MEM_POOL_SIZE != 0', but it seems like some other config options will set DMA_STM32 no matter what CONFIG_MEM_POOL_SIZE is. I think currently my solution is not good enough, the community may have some better ideas.

@avisconti
Copy link
Collaborator

OK, now it compiles.
Unfortunately the flow is broken. I had similar problem in the past which I was able to solve
disabling completely the dma logs and increasing the dma interrupt priority. I think I need to spend some
time here.... :-(

@cybertale
Copy link
Collaborator Author

@avisconti I have a f4 board with audio devices, I'll have a try today.

@cybertale
Copy link
Collaborator Author

@avisconti Do you mean that the i2s interface is working but the playing is not continuous? Sorry I have never been used this interface before, it seems like to test play audio need many other support and there isn't a generic test application in zephyr. How can I help your debug?

@avisconti
Copy link
Collaborator

@avisconti Do you mean that the i2s interface is working but the playing is not continuous? Sorry I have never been used this interface before, it seems like to test play audio need many other support and there isn't a generic test application in zephyr. How can I help your debug?

No, actually the I2S+DMA is failing immediately. Yesterday evening I saw that DMA was failing with Fifo Error immediately. Are you based in Le Mans? If so you can get one ArgonKey board from Erwan and use my microphone sample.

@cybertale
Copy link
Collaborator Author

@avisconti Unfortunately I'm in China, but since it fails from the beginning, I can try this driver with the configuration in i2s_ll_stm32.c to see if I can see the same error.

@avisconti
Copy link
Collaborator

@avisconti Unfortunately I'm in China, but since it fails from the beginning, I can try this driver with the configuration in i2s_ll_stm32.c to see if I can see the same error.

I'm debugging it and I actually found few mistakes.
I'll update you by end of my day

@cybertale
Copy link
Collaborator Author

@avisconti Great! thx!

Copy link
Collaborator

@avisconti avisconti left a comment

Choose a reason for hiding this comment

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

With this changes I2S audio is now working fine.

}

if (id == data->max_streams) {
LOG_ERR("Unknow interrupt happened.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

we may want to return here, to avoid using a bad index.

LL_DMA_Set##dev##Burstxfer(dma, table_stream[id], \
LL_DMA_##dev_short##BURST_INC16); \
break; \
default: \
Copy link
Collaborator

Choose a reason for hiding this comment

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

the cases here should be 0,1,2,3 and not 1,4,8,16

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This value represents how many data units does one burst send/receive, I don't understand why it should be 0, 1, 2, 3, comment in dma.h is blur about this. After searching 'source_burst_length' in Zephyr's tree, I see some usage in dma_qmsi.c, dma_sam_xdmac.c, dma_cavs.c, they treat this value with find_msb_set and something like that. This value should be 0 or 2^n I guess, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems again a mismatch like the data size. Before the driver was using dma_burst_index(),
which was expecting the burst to be the exponent (i.e. 0 is 8-bit burst, 1 is 16-bit burst, ...).

But all the drivers seems to expect the burst size like 1, 4, 8, 16. So, we may change I2S driver
according to the API (even if in the burst case the dma.h seems not really clear in the
explanation).


m_size = table_m_size[index];
index = find_lsb_set(config->dest_data_size) - 1;
p_size = table_p_size[index];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here m_size and p_size are wrong. For example in my case (I2S) I passed 1 for bot source and dest data size, because
it is 16-bit transfer. But this code returned m_size = 0 and p_size = 0, which is wrong.
I remove the '- 1' to have index = 1 and it worked, but I'm not sure it is always correct.
Take a closer look here...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm a bit confused about this, m_size and p_size come from source_data_size and dest_data_size, which are width of source data/dest data (in bytes) as introduced in include/dma.h:121. So if we pass 1 for source_data_size and dest_data_size then m_size and p_size just should be 0 for representing one byte, right? In your case it is 16-bits, so we should pass 2 for 2 bytes to source_data_size/dest_data_size, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i2s_ll_stm32.c:847 do gives source_data_size and dest_data_size 1 and commented them as '16 bit', but I believe this is the problem, they should be both assigned with 2, along with code in i2s_ll_stm32.c:865, do you think so?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe you are right. It seems that you have to specify the number of bytes (i.e 1 is 8-bits,
2 is 16-bits, 4 is 32-bits, 8 is 64-bits). I saw that all other DMAs drivers are doing that way.

I don't know why, but seems that before it was using dma_width_index() and in that case the
mapping was different (i.e. 0 was 8-bits, 1 was 16-bits, 2 was 32-bits and so on). But I tend
to think that this routine is not matching the API expectation.

So, yes, I think that we need to change i2s_ll_stm32.c driver to match source_data_size and
dest_data_size (i.e. put 2 there to mean 16-bit).

size_config = dma_stm32_width_config(config, true, dma, id);
LL_DMA_ConfigTransfer(dma, table_stream[id],
LL_DMA_DIRECTION_PERIPH_TO_MEMORY |
LL_DMA_MODE_CIRCULAR |
Copy link
Collaborator

Choose a reason for hiding this comment

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

I had to remove CIRCULAR here, otherwise it was not working.
I guess that in this particular case (PERIPHERAL_TO_MEMORY) it does not apply.
Maybe.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Zephyr currently does not support to configure i DMA is in circular mode. I have not experienced with many DMA peripherals so I don't know if other DMA peripherals also have this feature. Before this feature is added, I'm thinking about configuring if it's in circular mode in dts. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know what @erwango has to say here. But yes, it seems to me that all the dma
configurations sooner or later has to be put in dts. So we may start with circular mode.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here I have some doubts. We have two specific dts parts:

  1. The dma controller section, which should describe the controller capabilities
  2. The dma section inside a client driver (like I2S). Here we should describe what the client
    is using (channels, slot, direction,...)

Probably the CIRCULAR info goes here somehow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, back to Linux we have DMA controller part dts and DMA device part dts, and Zephyr supports this arch for SPI and I2C already, I think I can try to add a dma-devices.yaml to add support for this function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, seems the right direction

Copy link
Contributor

Choose a reason for hiding this comment

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

@cybertale Can you add CIRCULAR support and make it configurable? In my implementation #14916 for UART Async, the DMA is configured to use the circular mode ( which I extended).

Copy link
Collaborator Author

@cybertale cybertale Apr 12, 2019

Choose a reason for hiding this comment

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

Sure, will do it as soon as I got some time. @jli157

This commit adds driver support for DMA on f0/f1/f2/f3/f4/l0/l4
series stm32.
This should have been working on the f7 series SoCs, but there
are some weird bugs inside the code that I cannot resolve.

There are two kinds of IP blocks are used across these stm32, one is the
one that has been used on F2/F4/F7 series, and the other one is the one
that has been used on F0/F1/F3/L0/L4 series.

Memory to memory transfer is only supported on the second DMA on
F2/F4/F7 with 'st,mem2mem' to be declared in dts.

This driver depends on k_malloc to allocate memory for stream instances,
so CONFIG_HEAP_MEM_POOL_SIZE must be big enough to hold them.

Common parts of the driver are in dma_stm32.c and SoC related parts are
implemented in dma_stm32_v*.c.

This driver has been tested on multiple nucleo boards, including
NUCLEO_F091RC/F103RB/F207ZG/F302R8/F401RE/L073RZ/L476RG with the
loop_transfer and chan_blen_transfer test cases.

Signed-off-by: Song Qiang <[email protected]>
Previous defconfig for dma is DMA_STM32F4X in this board, while the new
generic driver uses DMA_STM32 to enable DMA support, and also dma driver
of stm32 now needs HEAP_MEM_POOL_SIZE to be big enough to hold dma
stream instances.

Additional .conf files are added for also adding HEAP_MEM_POOL_SIZE
configuration to two test cases.

Signed-off-by: Song Qiang <[email protected]>
i2s2 on this board depands on dma1, so add it to the dts file.

Signed-off-by: Song Qiang <[email protected]>
Add DMA test support for 'loop_transfer', 'chan_blen_transfer' on
NUCLEO_F091RC/F103RB/F207ZG/F302RF8/F401RE/F746ZG/L073RZ/L476RG.

Signed-off-by: Song Qiang <[email protected]>
Add myself as code owner of, drivers/dma/dma_stm32*, and
tests/drivers/i2s/*/96b_argonkey.conf.

Signed-off-by: Song Qiang <[email protected]>
Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

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

First batch of comments .
Let's get use of dts correct first (hope @galak and @ulfalizer can help in this regards)
and then we'll see on driver implementation details.

dma.priority = (dma.channel_config >> 16) & 0x3
dma.fifo_threshold = dma.features & 0x3

out_dev(node, "DMA_SLOT_{}".format(dma_i), dma.slot)
Copy link
Member

Choose a reason for hiding this comment

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

Feels strange to add compatible specific code at this point.
Seems to me we should use binding "#cells" to generate this.
@ulfalizer, would you help sorting this out?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I personally also think so, while I didn't find a better way of doing this. There's seems like no examples of having such behaviour.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, would be nicer to use #cells here. See the comment I posted above.

Previously, new phandle-array properties had to be hardcoded in edtlib.py, because I hadn't figured out how to deal with them in a generic way. It was fixed by #19327 though.

.dma_callback = dma_rx_callback,
},
.src_addr_increment = DT_I2S_1_DMA_SRC_ADDR_INCREMENT_RX,
Copy link
Member

Choose a reason for hiding this comment

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

Trying to understand how this works globally.
From https://git.kernel.org/pub/scm/linux/kernel/git/devicetree/devicetree-rebasing.git/plain/Bindings/dma/stm32-dma.txt, I'd expect a mask is applied to get this info from bitmap

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The bitmap is decoded in the gen_defines.py

Copy link
Member

@erwango erwango Sep 30, 2019

Choose a reason for hiding this comment

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

ok, got it, then we need maintainer point of view on this regard.
Not sure this is the right place, but this is definitely an interesrting use case.


config DMA_STM32_V1
bool
depends on SOC_SERIES_STM32F2X || SOC_SERIES_STM32F4X || SOC_SERIES_STM32F7X
Copy link
Member

Choose a reason for hiding this comment

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

Do we keep F7 for now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would like to keep it, I was thinking there maybe other people would like to help solving the buffer problem and even if the code does not work on f7 it does not breaks anything since there wasn't any support for the f7. But pushing code cannot pass the test is not ok, right? If it is not ok I'll remove this.

@@ -23,7 +23,18 @@
#include <drivers/dma.h>
#include <ztest.h>

#if defined(CONFIG_SOC_FAMILY_STM32)
#if defined(CONFIG_SOC_SERIES_STM32F2X) || \
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to use matching driver variant config symbol here (CONFIG_DMA_STM32_Vx)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unrelated nit, but could have #ifdef CONFIG_FOO instead of #if defined(CONFIG_FOO) when there's just one. I don't know if we just do it for consistency, but it looks a bit odd coming to Zephyr from the outside

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unrelated nit, but could have #ifdef CONFIG_FOO instead of #if defined(CONFIG_FOO) when there's just one. I don't know if we just do it for consistency, but it looks a bit odd coming to Zephyr from the outside

No guidelines here so I think they are all ok. usually I would use what others are using. In Zephyr I saw many examples of using #if defined(CONFIG_FOO) so I'm using it. 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Think it's more that it's just getting copied around by people though. Fight the flow. ;)

@@ -22,7 +22,17 @@
static const char tx_data[] = "The quick brown fox jumps over the lazy dog";
static char rx_data[TRANSFER_LOOPS][RX_BUFF_SIZE] = {{ 0 } };

#if defined(CONFIG_SOC_FAMILY_STM32)
Copy link
Member

Choose a reason for hiding this comment

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

Same here ?

@@ -579,6 +583,46 @@ def write_clocks(node):
controller.props["clock-frequency"].val)


def write_dmas(node):
Copy link
Collaborator

@ulfalizer ulfalizer Sep 30, 2019

Choose a reason for hiding this comment

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

Would be a good idea to reuse the generic write_phandle_val_list() here for as much stuff as possible. Hopefully, the identifiers can fit into the same scheme used for other phandle arrays (pwms, etc.).

Try just calling just write_phandle_val_list() first, see what #defines you miss, and output those manually afterwards.

That'll make the generated identifiers consistent too. Should be DMAS_... instead of DMA_... for example.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

write_phandle_val_list() should be only outputting 'channel' 'slot' 'feature' and 'channel_configuration' here, which are declared in cells. I still have to decode 'periph_inc', 'mem_inc', 'periph_inc_fixed', 'priority' and 'fifo_threshold' here, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have to get these vars from dts but for complying https://git.kernel.org/pub/scm/linux/kernel/git/devicetree/devicetree-rebasing.git/plain/Bindings/dma/stm32-dma.txt, I can only use 'channel', 'slot', 'channel_config' and 'features' in cells. write_phandle_val_list() will only save one line of code here.

Copy link
Collaborator

@ulfalizer ulfalizer Sep 30, 2019

Choose a reason for hiding this comment

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

Still better to use write_phandle_val_list() for whatever it can be used for I think (and outputting whatever else needs to be output after calling it).

It also forces things like naming consistency, which is nice (DMAS_CONTROLLER_... instead of DMA_CONTROLLER_..., etc.) We have some inconsistency there for clocks already, which we want to get rid of.

Using the same code for everything is nice if you need to change something related to the common phandle-array parts later too.

config DMA_STM32_V1
default y

endif # DMA
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the same as this btw:

config DMA_STM32_V1
	default y if DMA_STM32

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, that's better

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I would still use the style I was using, all other driver kconfig symbols in these files uses that style, should be consistent with them.

st,mem2mem;
status = "disabled";
label = "DMA_2";
};
};
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this change (dma2) apply also to F051 and F071?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But F051 and F071 has only one DMA instance according to the datasheet.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, correct.
I'm reading RM0091, and it seems infact that only STM32F091 has DMA2.
Sorry.

#dma-cells = <4>;
reg = <0x40020000 0x400>;
clocks = <&rcc STM32_CLOCK_BUS_AHB1 0x1>;
interrupts = <9 0 10 0 10 0 11 0 11 0>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it correct priority==0?
Moreover, how should I read the sequence of couples?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Priority is 0 means using the default priority I think. Sorry I don't understand what is 'sequence of couples', Could you give me a more specific description?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Like 9 0, 10 0, 10 0, 11 0, 11 0
Five couples which represent, I guess, irqs for the DMA channel/slot.
Can you describe it to me better? Thx

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Like 9 0, 10 0, 10 0, 11 0, 11 0
Five couples which represent, I guess, irqs for the DMA channel/slot.
Can you describe it to me better? Thx

Sorry for the late reply. For five couples, the first one in them represents the irq channel used in hardware, which is described in the datasheet, and the second one is the priority we would use. This DMA instance has 5 channels in hardware, so five couples of data. This is the default explanation for 'interrupts' for all dts nodes. Hope this anwsers your question.

@avisconti
Copy link
Collaborator

@cybertale,
I can try to test it on ArgonKey, as I see a commit for I2S as well.

@cybertale
Copy link
Collaborator Author

@cybertale,
I can try to test it on ArgonKey, as I see a commit for I2S as well.

Thanks! Was wanting you to help test in the first place. 😄

@avisconti
Copy link
Collaborator

@cybertale,
I can try to test it on ArgonKey, as I see a commit for I2S as well.

Thanks! Was wanting you to help test in the first place.

I decided to test it on x_nucleo_cca02m1 shield, as for the ArgonKey I lost my h/w setup.
Cherry picking your 8 commits (from 46c1d86 to 5f3cc9f) I had to change DMA_STM32F4X into DMA_STM32.
I have few warnings in dma_stm32.c:

/local/home/visconti/Projects/zephyrproject/zephyr/drivers/dma/dma_stm32.c:525:36: warning: 'dma_funcs' defined but not used [-Wunused-const-variable=]
static const struct dma_driver_api dma_funcs = {
^~~~~~~~~
/local/home/visconti/Projects/zephyrproject/zephyr/drivers/dma/dma_stm32.c:494:12: warning: 'dma_stm32_init' defined but not used [-Wunused-function]
static int dma_stm32_init(struct device *dev)
^~~~~~~~~~~~~~
/local/home/visconti/Projects/zephyrproject/zephyr/drivers/dma/dma_stm32.c:75:13: warning: 'dma_stm32_irq_handler' defined but not used [-Wunused-function]
static void dma_stm32_irq_handler(void *arg)

And it doesn't work, so I have to debug tomorrow.

default:
return -EINVAL;
}

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 that here you need to set the size, isn't it?
Like:

LL_DMA_SetDataLength(dma, table_ll_stream[id], size);

Copy link
Collaborator

Choose a reason for hiding this comment

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

@cybertale
I am now able to compile correctly, and I test it using I2S audio with microphone board.
But it does not work.
I tried to fix the dma_reload() function (see my latest review), but still does not work.
It seems that it stops receving data. Tomorrow I'll debug more.

@galak
Copy link
Collaborator

galak commented Oct 1, 2019

Can we split this PR so the DTS changes are one PR and the driver changes are another.

.dma_cfg = {
.block_count = 1,
.dma_slot = I2S1_DMA_SLOT_RX,
.dma_slot = DT_I2S_1_DMA_SLOT_RX,
.channel_direction = PERIPHERAL_TO_MEMORY,
.source_data_size = 1, /* 16bit default */
.dest_data_size = 1, /* 16bit default */
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 that .source_data_size and .dest_data_size should be 2 (16bit == 2 bytes)

config->dma_slot = 0;
}
}
DMA_InitStruct.Channel = config->dma_slot;
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 this is wrong.
DMA_InitStruct.Channel should be assigned with LL_DMA_CHANNEL_0/LL_DMA_CHANNEL_1/LL_DMA_CHANNEL_2/...

I noticed infact that the CR content was wrong here.
To fix it I added following code:

  switch (config->dma_slot) {
  case 0:
  >       DMA_InitStruct.Channel = LL_DMA_CHANNEL_0;
  >       break;
  case 1:
  >       DMA_InitStruct.Channel = LL_DMA_CHANNEL_1;
  >       break;
  case 2:
  >       DMA_InitStruct.Channel = LL_DMA_CHANNEL_2;
  >       break;

[...]

 }

}
DMA_InitStruct.Channel = config->dma_slot;

stm32_dma_get_fifo_threshold(config->head_block->fifo_mode_control);
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 you should assign the return value in DMA_InitStruct.FIFOThreshold.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Like:

DMA_InitStruct.FIFOThreshold = stm32_dma_get_fifo_threshold(config->head_block->fifo_mode_control);

@avisconti
Copy link
Collaborator

@cybertale @erwango
With the modifications reported above, especially source and data size as well as the dma_slot assignment, I was able to pass correctly the peripheral-to-memory case on both x_nucleo_cca02m1 shield and ArgonKey.

@avisconti
Copy link
Collaborator

@cybertale
Copy link
Collaborator Author

@cybertale
I pushed my test branch just in case:
https://github.com/avisconti/zephyr/pull/new/shield-microphones-dmanew

Many thanks! I fixed the problems in the DMA driver. But I think the problem in the I2S driver is not related to the DMA driver and should be fixed in a septate PR. You want to push the fix to the I2S yourself or I help to do it?

@avisconti
Copy link
Collaborator

@cybertale
I pushed my test branch just in case:
https://github.com/avisconti/zephyr/pull/new/shield-microphones-dmanew

Many thanks! I fixed the problems in the DMA driver. But I think the problem in the I2S driver is not related to the DMA driver and should be fixed in a septate PR. You want to push the fix to the I2S yourself or I help to do it?

@cybertale
If it is not a problem and you agree with the changes you can do it. Then, I'll test again.

@cybertale
Copy link
Collaborator Author

@cybertale
I pushed my test branch just in case:
https://github.com/avisconti/zephyr/pull/new/shield-microphones-dmanew

Many thanks! I fixed the problems in the DMA driver. But I think the problem in the I2S driver is not related to the DMA driver and should be fixed in a septate PR. You want to push the fix to the I2S yourself or I help to do it?

@cybertale
If it is not a problem and you agree with the changes you can do it. Then, I'll test again.

Ok.

@cybertale
Copy link
Collaborator Author

@galak Splitting the PR to #19711 and #19712 .

@cybertale cybertale closed this Oct 9, 2019
@avisconti
Copy link
Collaborator

@cybertale
So are you going to close this PR?
Are #19711 and #19712 replacing it?

@cybertale
Copy link
Collaborator Author

@galak Splitting the PR to #19711 and #19712 .

@cybertale
So are you going to close this PR?
Are #19711 and #19712 replacing it?

Yes. I'm dividing the big PR to dts changes and driver changes regarding @galak 's advice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: Boards area: Devicetree area: DMA Direct Memory Access area: I2S area: Samples Samples area: Tests Issues related to a particular existing or missing test platform: STM32 ST Micro STM32
Projects
None yet
Development

Successfully merging this pull request may close these issues.

STM32: Move DMA driver to LL/HAL and get it STM32 generic
7 participants