DMA: stm32f4: Extend meaning of DMA return value #14809

jli157 · 2019-03-22T01:33:36Z

Extend the meaning of DMA return value which is fed to user's callback function so that the meaning of non-zero return value will be defined by a specific DMA driver.

Specific to STM32's DMA driver, redefine the return value so that zero means DMA transfer is successful; positive means not only a successful DMA transfer but also how many bytes received for RX DMA cases; negative still means DMA error. Accordingly, updated STM32's I2S driver to correspond to the change.

The PR also fixes a waiting issue in STM32's DMA driver by reducing one time wait delay to 50ms and aborting after 5 seconds in total when disabling DMA.

The PR tries to be the first step for addressing the issue #13955.

codecov-io · 2019-03-22T02:09:59Z

Codecov Report

Merging #14809 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #14809   +/-   ##
=======================================
  Coverage   52.49%   52.49%           
=======================================
  Files         309      309           
  Lines       45060    45060           
  Branches    10431    10431           
=======================================
  Hits        23656    23656           
  Misses      16592    16592           
  Partials     4812     4812

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62da238...ae8393c. Read the comment docs.

erwango · 2019-03-22T07:59:35Z

include/dma.h

@@ -151,7 +151,7 @@ struct dma_config {
 	struct dma_block_config *head_block;
 	void *callback_arg;
 	void (*dma_callback)(void *callback_arg, u32_t channel,
-			     int error_code);
+			     int ret_code);


This is not compliant with current DMA API:

* dma_callback is the callback function pointer. If enabled, callback function * will be invoked at transfer completion or when error happens * (error_code: zero-transfer success, non zero-error happens).

I'm not opposed to the change, but it would require dma API alignment

Ho, I missed that sorry, should not start review before coffee.

This means that all code using the STM32F4 DMA (I2S driver for example) must be changed as well.
Shouldn't be part of the PR as well?

I noticed that you change the dma_callback() signature here, but not in "struct dma_stm32_stream" in c file.
I guess you should do that as well.

jli157

@erwango do you notice that I’ve changed the api comments as well to make both consistent?

False start

avisconti · 2019-03-22T08:48:57Z

include/dma.h

@@ -130,7 +130,7 @@ struct dma_block_config {
 *
 * dma_callback is the callback function pointer. If enabled, callback function
 *              will be invoked at transfer completion or when error happens
- *              (error_code: zero-transfer success, non zero-error happens).
+ *              (ret_code: non negative-transfer success, nenative-error happens).
 */


pls fix this typo: 'nenative' into 'negative'.

I've rewritten the comments again to minimize the affect of the changes: define non-zero return value as the one defined by a specific driver since some DMA drivers give positive values as errors as well. As for STM32 DMA, let's define negative as errors, and positive as how many bytes RX DMA has received.

avisconti · 2019-03-22T08:50:20Z

include/dma.h

@@ -151,7 +151,7 @@ struct dma_config {
 	struct dma_block_config *head_block;
 	void *callback_arg;
 	void (*dma_callback)(void *callback_arg, u32_t channel,
-			     int error_code);
+			     int ret_code);


This means that all code using the STM32F4 DMA (I2S driver for example) must be changed as well.
Shouldn't be part of the PR as well?

Extend the meaning of DMA return value which is fed to user's callback function so that the meaning of non-zero value will be defined by a specific DMA driver, for example, error happens when negative, and how many bytes RX DMA has received when positive. Signed-off-by: Jun Li <[email protected]>

The return value to the callback function is extended to not only represent successful DMA transfer but also how many bytes received for RX DMA cases. And reduced one time wait delay to 50ms and abort after 5 seconds in total when disabling DMA. Signed-off-by: Jun Li <[email protected]>

err_code is redefined as ret_code and it's an error code only when ret_code is negative. Signed-off-by: Jun Li <[email protected]>

jli157 · 2019-03-22T15:58:57Z

@avisconti changed STM32 I2S driver to correspond to the redefinition of the return value.

jli157 · 2019-04-08T17:07:54Z

@avisconti Hey do you have more comments to the changes? Thanks!

avisconti · 2019-04-09T13:48:26Z

@avisconti Hey do you have more comments to the changes? Thanks

@cybertale
There is some work here that you may want to incorporate as well.

cybertale · 2019-04-11T02:34:23Z

Interesting work, but I'm wondering shouldn't we change all dma usage in zephyr's tree since we are changing the api?

jli157 · 2019-04-11T03:18:15Z

Interesting work, but I'm wondering shouldn't we change all dma usage in zephyr's tree since we are changing the api?

It is only the meaning of the DMA callback variable that is extended so that a SoC's DMA driver will have flexibility to redefine it. However, I'm not sure if other DMA drivers of other SoCs could define the positive return value with the meanings similar to STM32's. This is why the changes to DMA clients is only limited to those of STM32's.

jli157 · 2019-04-11T04:10:23Z

Hey @cybertale I've just read your PR #13364 for implementing a general DMA driver for STM32 SoCs, nice job!
I'm currently implementing another DMA client - asynced UART for STM32F4 by the PR #14916. Its current status is still in progress. To support UART RX via DMA, I have to extend DMA API again by adding another function pending() to retrieve how many bytes have been received by UART RX since the data length on UART RX could vary among DMA transactions. Also, I hope the DMA driver to have capability to enable the circular mode which I've also extended STM32F4's DMA driver to support. Can you also consider these two requirements from my implementation while implementing the general DMA driver? Thank you!

cybertale · 2019-04-11T07:46:07Z

Interesting work, but I'm wondering shouldn't we change all dma usage in zephyr's tree since we are changing the api?

It is only the meaning of the DMA callback variable that is extended so that a SoC's DMA driver will have flexibility to redefine it. However, I'm not sure if other DMA drivers of other SoCs could define the positive return value with the meanings similar to STM32's. This is why the changes to DMA clients is only limited to those of STM32's.

Understood. My worry is that for application users, portability of code snippets is going low. What I want to say is that maybe we should make this api change a standalone PR and make it consistent to all the drivers?
Also I have some doubts, DMA should only return the number less than we expected when the DMA transfer fails, right? SNDTR should be zero if succeeded. So
stream->dma_callback(stream->callback_arg, id, leftover_len);
should be in error callback rather than the TC callback. Right?

cybertale · 2019-04-11T08:00:56Z

Hey @cybertale I've just read your PR #13364 for implementing a general DMA driver for STM32 SoCs, nice job!
I'm currently implementing another DMA client - asynced UART for STM32F4 by the PR #14916. Its current status is still in progress. To support UART RX via DMA, I have to extend DMA API again by adding another function pending() to retrieve how many bytes have been received by UART RX since the data length on UART RX could vary among DMA transactions. Also, I hope the DMA driver to have capability to enable the circular mode which I've also extended STM32F4's DMA driver to support. Can you also consider these two requirements from my implementation while implementing the general DMA driver? Thank you!

Sure! I added circular mode support but removed it after. That's because I thought circular mode is not suitable for zephyr's api. Since that you want it I can add support for it. But is 'pending' a new api in dma.h? I think api changes should take a bit more time than normal driver changes, right?

jli157 · 2019-04-11T20:41:09Z

Interesting work, but I'm wondering shouldn't we change all dma usage in zephyr's tree since we are changing the api?

It is only the meaning of the DMA callback variable that is extended so that a SoC's DMA driver will have flexibility to redefine it. However, I'm not sure if other DMA drivers of other SoCs could define the positive return value with the meanings similar to STM32's. This is why the changes to DMA clients is only limited to those of STM32's.

Understood. My worry is that for application users, portability of code snippets is going low. What I want to say is that maybe we should make this api change a standalone PR and make it consistent to all the drivers?
Also I have some doubts, DMA should only return the number less than we expected when the DMA transfer fails, right? SNDTR should be zero if succeeded. So
stream->dma_callback(stream->callback_arg, id, leftover_len);
should be in error callback rather than the TC callback. Right?

Hey @cybertale I've just read your PR #13364 for implementing a general DMA driver for STM32 SoCs, nice job!
I'm currently implementing another DMA client - asynced UART for STM32F4 by the PR #14916. Its current status is still in progress. To support UART RX via DMA, I have to extend DMA API again by adding another function pending() to retrieve how many bytes have been received by UART RX since the data length on UART RX could vary among DMA transactions. Also, I hope the DMA driver to have capability to enable the circular mode which I've also extended STM32F4's DMA driver to support. Can you also consider these two requirements from my implementation while implementing the general DMA driver? Thank you!

Sure! I added circular mode support but removed it after. That's because I thought circular mode is not suitable for zephyr's api. Since that you want it I can add support for it. But is 'pending' a new api in dma.h? I think api changes should take a bit more time than normal driver changes, right?

The issue I'm facing on implementing UART RX via DMA is that the DMA buffer could not be filled or half filled up to trigger the interrupt when a message with varying length comes from the peer. In that case, the RX transaction is defined by UART's IDLE interrupt. However, when UART IDLE interrupt happens, UART has no way to figure out how much data UART RX has received. This is why I think we need a new API for DMA's clients to figure out how much data is still pending to be sent or received.

cybertale · 2019-04-12T01:57:21Z

@jli157 I got it, so in your case we receive unknown long data and when the UART IDLE happened, we ask the DMA how many bytes left to be transfered to know how many bytes has already been received(or transmitted but I think we can easily know how many has been transmitted?). So you don't need DMA interrupts to tell you when to end but for just to alert you when the buffer is full.
You make the point, I support for this change, but I still have the worry about portability. Usually api is added with all the drivers changed to comply it.

jli157 · 2019-04-12T16:26:20Z

@avisconti @cybertale @erwango The changes in this PR seem not be necessary if we can add one more DMA API function pending() or similar to query how many bytes are still left in the current DMA transaction so that a client can check the current transmission status from time to time. The new function will be beneficial to UART RX implementation since the length of a UART RX transaction is sometimes unknown. I would like to close this one and replace it with a API change PR. What are you thoughts to this suggestion?

cybertale · 2019-04-13T09:41:26Z

@jli157 I agree with the standalone api PR idea, which should make the api changes logically standalone. While this is just my thought, decisions are reviewers to make. Let's see what do they think about this.

jli157 · 2019-04-14T00:58:34Z

This PR still can't thoroughly address the requirement for querying how many bytes are still left in a specific DMA transaction which is needed by UART RX. So, this one will be closed and another PR will be created to address the requirement. Thanks all for your comments!

cybertale · 2019-04-15T07:09:13Z

@jli157 Hi, still I have some doubts about this kind of usage. In this scenario, IDLE event happened, then we query through pending() and read data/reset DMA(Or do not reset?). But Zephyr is slow in many situations, so when the UART is fast, new data may come between IDLE event and pending(). If so, we may retrieve data when UART is receiving rather than idle, and if we reset DMA after reading, there is a risk that data may be lost. But if we do not reset DMA, unread data can be read in next read process, while buffer of DMA is limited so memory address will keep increasing and there's also a potential data lost and illegle memory access. Reset DMA means reset it's memory pointer here. Hope you understand what I'm thinking, just some thoughts as an advice!

avisconti · 2019-04-15T10:58:45Z

@avisconti @cybertale @erwango The changes in this PR seem not be necessary if we can add one more DMA API function pending() or similar to query how many bytes are still left in the current DMA transaction so that a client can check the current transmission status from time to time. The new function will be beneficial to UART RX implementation since the length of a UART RX transaction is sometimes unknown. I would like to close this one and replace it with a API change PR. What are you thoughts to this suggestion?

I like the idea. I think it would be cleaner adding a new API function rather than changing an existing one which may bring undesirable effects on existing drivers.

jli157 · 2019-04-15T18:54:30Z

@jli157 Hi, still I have some doubts about this kind of usage. In this scenario, IDLE event happened, then we query through pending() and read data/reset DMA(Or do not reset?). But Zephyr is slow in many situations, so when the UART is fast, new data may come between IDLE event and pending(). If so, we may retrieve data when UART is receiving rather than idle, and if we reset DMA after reading, there is a risk that data may be lost. But if we do not reset DMA, unread data can be read in next read process, while buffer of DMA is limited so memory address will keep increasing and there's also a potential data lost and illegle memory access. Reset DMA means reset it's memory pointer here. Hope you understand what I'm thinking, just some thoughts as an advice!

The use case where the IDLE interrupt works actually doesn't reset DMA at all. when the IDLE interrupt happens, only the incremental part of the data between two read operations will be read out, and the buffer is configured as a circular buffer to avoid overflow. You can take a look at my implementation on PR #14916 which is already working well. This is why I'm asking you to add the capability of enabling circular buffer mode.

cybertale · 2019-04-16T14:29:04Z

@jli157 You're right, that explains everything! 😄

jli157 requested a review from erwango as a code owner March 22, 2019 01:33

erwango requested a review from avisconti March 22, 2019 07:56

erwango added area: DMA Direct Memory Access platform: STM32 ST Micro STM32 labels Mar 22, 2019

erwango previously requested changes Mar 22, 2019

View reviewed changes

jli157 commented Mar 22, 2019

View reviewed changes

avisconti requested changes Mar 22, 2019

View reviewed changes

jli157 added 3 commits March 22, 2019 07:18

i2s: stm32: change err_code to ret_code

ae8393c

err_code is redefined as ret_code and it's an error code only when ret_code is negative. Signed-off-by: Jun Li <[email protected]>

jli157 force-pushed the dma_api_ext branch from 1977fd8 to ae8393c Compare March 22, 2019 15:53

jli157 mentioned this pull request Mar 26, 2019

WIP: driver: uart: stm32: implement async api #14916

Closed

5 tasks

jli157 closed this Apr 14, 2019

jli157 deleted the dma_api_ext branch April 14, 2019 00:58

DMA: stm32f4: Extend meaning of DMA return value #14809

DMA: stm32f4: Extend meaning of DMA return value #14809

Uh oh!

Conversation

jli157 commented Mar 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Mar 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

erwango Mar 22, 2019

Choose a reason for hiding this comment

Uh oh!

erwango Mar 22, 2019

Choose a reason for hiding this comment

Uh oh!

avisconti Mar 22, 2019

Choose a reason for hiding this comment

Uh oh!

avisconti Apr 11, 2019 • 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!

jli157 left a comment

Choose a reason for hiding this comment

Uh oh!

avisconti Mar 22, 2019

Choose a reason for hiding this comment

Uh oh!

jli157 Mar 22, 2019

Choose a reason for hiding this comment

Uh oh!

avisconti Mar 22, 2019

Choose a reason for hiding this comment

Uh oh!

jli157 commented Mar 22, 2019

Uh oh!

jli157 commented Apr 8, 2019

Uh oh!

avisconti commented Apr 9, 2019

Uh oh!

cybertale commented Apr 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jli157 commented Apr 11, 2019

Uh oh!

jli157 commented Apr 11, 2019

Uh oh!

cybertale commented Apr 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cybertale commented Apr 11, 2019

Uh oh!

jli157 commented Apr 11, 2019

Uh oh!

cybertale commented Apr 12, 2019

Uh oh!

jli157 commented Apr 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cybertale commented Apr 13, 2019

Uh oh!

jli157 commented Apr 14, 2019

Uh oh!

cybertale commented Apr 15, 2019

Uh oh!

avisconti commented Apr 15, 2019

Uh oh!

jli157 commented Apr 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cybertale commented Apr 16, 2019

Uh oh!

Uh oh!

jli157 commented Mar 22, 2019 •

edited

Loading

codecov-io commented Mar 22, 2019 •

edited

Loading

avisconti Apr 11, 2019 •

edited

Loading

cybertale commented Apr 11, 2019 •

edited

Loading

cybertale commented Apr 11, 2019 •

edited

Loading

jli157 commented Apr 12, 2019 •

edited

Loading

jli157 commented Apr 15, 2019 •

edited

Loading