I2Cv3 LLD already stops DMA in i2c_lld_abort_operation(i2cp) Topic is solved

[FXCoder]

DMA stop is already done in i2c_lld_abort_operation(i2cp).
The code should handle stream release.

Code: Select all

Index: hal_i2c_lld.c
===================================================================
--- hal_i2c_lld.c   (revision 12469)
+++ hal_i2c_lld.c   (working copy)
@@ -983,10 +983,25 @@
 
     /* I2C disable.*/
     i2c_lld_abort_operation(i2cp);
-#if STM32_I2C_USE_DMA == TRUE
-    i2c_lld_stop_tx_dma(i2cp);
-    i2c_lld_stop_rx_dma(i2cp);
+
+#if defined(STM32_I2C_DMA_REQUIRED) && defined(STM32_I2C_BDMA_REQUIRED)
+  if(i2cp->is_bdma)
 #endif
+#if defined(STM32_I2C_BDMA_REQUIRED)
+  {
+    bdmaStreamRelease(i2cp->rx.bdma);
+    bdmaStreamRelease(i2cp->tx.bdma);
+  }
+#endif
+#if defined(STM32_I2C_DMA_REQUIRED) && defined(STM32_I2C_BDMA_REQUIRED)
+  else
+#endif
+#if defined(STM32_I2C_DMA_REQUIRED)
+  {
+    dmaStreamRelease(i2cp->rx.dma);
+    dmaStreamRelease(i2cp->tx.dma);
+  }
+#endif
 
 #if STM32_I2C_USE_I2C1
     if (&I2CD1 == i2cp) {


[Giovanni]

Hi,

Thanks for finding, patch committed, have you verified driver functionality? It is not tested yet on my side.

Giovanni

[FXCoder]

Hi Giovanni,
I2Cv3 driver is in use on L4+ (both a custom board and NUCLEO).
I wouldn’t say it has been heavily exercised but so far it seems functional.

—
Bob

[Giovanni]

Basically it is I2Cv2 with few DMA-related changes. It is targeted to devices with DMAMUX and it could be improved to use a single DMA channel (RX and TX do not happen at the same time, 2 channels are pointless), this is why I split it.

Giovanni

[FXCoder]

OK on the situation with separate TX and RX channels.
A single channel would indeed reduce halconf DMA channel juggling.

On the subject of MCUs that have DMAMUX and the DMA channel assignment in halconf...
Any thoughts on having DMA channel assigned dynamically by the DMA driver?
e.g. if STM32_XXXX_DMA_CHANNEL is set to 0 then dmaStreamAllocate(...) would select a currently free channel.
More overhead in DMA driver but good for peripheral driver DMA channel flexibility.
--
Bob

[Giovanni]

That is a good idea but would require splitting all drivers using DMAs. It could be introduced gradually, probably all new STM32s will have the DMAMUX.

Giovanni

[Giovanni]

Hi,

About the DMA point, I made changes to the DMAv1 driver (others to follow).

Now there is a new API dmaStreamAllocI() similar to the existing dmaStreamAllocate() (still present) that allows:

- Allocate a specific DMA channel.
- Allocate any DMA channel among those not in use.
- Allocate any DMA channel in DMA1.
- Allocate any DMA channel in DMA2.

It is backward compatible but it cannot really be leveraged until the various drivers use then new API, imagine some driver trying to allocate specific channels and others trying to take a specific one.

Giovanni

[FXCoder]

Hi Giovanni,
That was fast!
I saw your DMAv1_MUX experiment on the repo but didn't expect the new DMAv1 so quickly.
The new DMAv1 looks excellent.
I'm slowly working on a ChibiOS style DCMI driver but have been side tracked doing a JPEG encoder the last few days.
I'll so will get back to DCMI ASAP and adopt the new DMAv1 on the interim code and test it.

Regarding the new DMAv1 a couple of questions on use of #defines:

#1- STM32_DMA_STREAMS is defined as STM32_DMA1_NUM_CHANNELS + STM32_DMA2_NUM_CHANNELS so perhaps use it to define special code values instead of hard coded numbers

Code: Select all

/**
 * @name    Special stream identifiers
 * @{
 */
#define STM32_DMA_STREAM_ID_ANY            STM32_DMA_STREAMS
#define STM32_DMA_STREAM_ID_ANY_DMA1       (STM32_DMA_STREAM_ID_ANY + 1)
#define STM32_DMA_STREAM_ID_ANY_DMA2       (STM32_DMA_STREAM_ID_ANY_DMA1 + 1)


#2 - Following same theme of using existing #defines does the following make sense?

Code: Select all

const stm32_dma_stream_t *dmaStreamAllocI(uint32_t id,
                                          uint32_t priority,
                                          stm32_dmaisr_t func,
                                          void *param) {
  uint32_t i, startid, endid;

  osalDbgCheckClassI();

  if (id < STM32_DMA_STREAMS) {
    startid = id;
    endid   = id;
  }
  else if (id == STM32_DMA_STREAM_ID_ANY) {
    startid = 0U;
    endid   = STM32_DMA_STREAMS - 1U;
  }
  else if (id == STM32_DMA_STREAM_ID_ANY_DMA1) {
    startid = 0U;
    endid   = STM32_DMA1_NUM_CHANNELS - 1U;
  }
#if STM32_DMA2_NUM_CHANNELS > 0
  else if (id == STM32_DMA_STREAM_ID_ANY_DMA2) {
    startid = STM32_DMA1_NUM_CHANNELS;
    endid   = STM32_DMA_STREAMS - 1U;
  }
#endif

--
Bob

[Giovanni]

Hi,

I made the changes.

Now dynamic allocation is enabled for STM32H7xx (SPIv3 and I2Cv3), could you give I2C another try? I have nothing here to test it.

Note that in mcucof.h you can still specify specific channels if you want but mixing dynamic and static would not work, a dynamically allocated channel could make a static request fail. In order to support a mixed scenario significant extra work would be required, not sure it would be worth the time.

Giovanni

[FXCoder]

Hi,
Did basic tests using combinations of fixed and dynamic assignments for I2Cv3 on L4+ (no H7 here).
No problems found at this stage.

One further edit to DMAv1 (make consistent with DMAv2)...

Code: Select all

Index: stm32_dma.c
===================================================================
--- stm32_dma.c   (revision 12489)
+++ stm32_dma.c   (working copy)
@@ -551,7 +551,7 @@
   }
 #if STM32_DMA2_NUM_CHANNELS > 0
   else if (id == STM32_DMA_STREAM_ID_ANY_DMA2) {
-    startid = 7U;
+    startid = (STM32_DMA_STREAMS / 2U) - 1U;
     endid   = STM32_DMA_STREAMS - 1U;
   }
 #endif


--
Bob