Skip to content
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

invert tx and rx in spi_dma_setup #11307

Merged
merged 1 commit into from Dec 9, 2023

Conversation

bertvoldenuit
Copy link
Contributor

Summary

According to SAMD21 Errata:

1.7.2 Linked Descriptor

When at least one channel using linked descriptors is already active, enabling another DMA channel (with or without
linked descriptors) can result in a channel Fetch Error (FERR) or an incorrect descriptor fetch.
This occurs if the channel number of the channel being enabled is lower than the channel already active.

Workaround

When enabling a DMA channel while other channels using linked descriptors are already active, the channel number
of the new channel enabled must be greater than the other channel numbers.`

In the spi driver, in the description and in the code it is always Tx and then Rx except in one place where it is Rx and then Tx which could lead in the FERR according to Errata

I think it would be better to swap Rx/Tx in the following code:

static void spi_dma_setup(struct sam_spidev_s *priv)
{
  /* Allocate a pair of DMA channels */

    priv->dma_rx = sam_dmachannel(DMACH_FLAG_BEATSIZE_BYTE |
                                                           DMACH_FLAG_MEM_INCREMENT |
                                                           DMACH_FLAG_PERIPH_RXTRIG(priv->dma_rx_trig)); 
    priv->dma_tx = sam_dmachannel(DMACH_FLAG_BEATSIZE_BYTE |
                                                           DMACH_FLAG_MEM_INCREMENT |
                                                           DMACH_FLAG_PERIPH_TXTRIG(priv->dma_tx_trig));
                               
}

into

static void spi_dma_setup(struct sam_spidev_s *priv)
{
  /* Allocate a pair of DMA channels */

  priv->dma_tx = sam_dmachannel(DMACH_FLAG_BEATSIZE_BYTE |
                                                        DMACH_FLAG_MEM_INCREMENT |
                                                        DMACH_FLAG_PERIPH_TXTRIG(priv->dma_tx_trig));

  priv->dma_rx = sam_dmachannel(DMACH_FLAG_BEATSIZE_BYTE |
                                                         DMACH_FLAG_MEM_INCREMENT |
                                                         DMACH_FLAG_PERIPH_RXTRIG(priv->dma_rx_trig));                                
}

Impact

In my case (driving led strip ws2812), I still get an error TERR on the Rx channel. It has no particular impact but it makes more sense and should avoid potential FERR according to errata.

Testing

Compiles and runs

arch/arm/src/samd2l2/sam_spi.c Outdated Show resolved Hide resolved
arch/arm/src/samd2l2/sam_spi.c Outdated Show resolved Hide resolved
@xiaoxiang781216
Copy link
Contributor

but, let's sqaush into one patch, @bertvoldenuit .

@pkarashchenko
Copy link
Contributor

Please squash, so we can merge

remove indent sam_spi.c

removed indent
@bertvoldenuit
Copy link
Contributor Author

Done!
Sorry for the delay, I am not familiar with squashing. I hope I did it good!

@xiaoxiang781216
Copy link
Contributor

let's ignore the new gcc report warning:

bas_token.l: In function 'yylex':
Error: bas_token.l:[121](https://github.com/apache/nuttx/actions/runs/7145281334/job/19460719816?pr=11307#step:7:122)0:31: error: 'strcpy' writing 1 or more bytes into a region of size 0 overflows the destination [-Werror=stringop-overflow=]
 1210 |                             }
      |                               ^                                  
In file included from bas_auto.h:77,
                 from bas_token.l:16:
bas_token.h:103:8: note: at offset 2 into destination object 'name' of size 2
  103 |   char name[2/* ... */];
      |        ^~~~

I will fix it soon.

@xiaoxiang781216 xiaoxiang781216 merged commit 0ca8ae8 into apache:master Dec 9, 2023
11 of 26 checks passed
@jerpelea jerpelea added this to To-Add in Release Notes - 12.4.0 Dec 27, 2023
@jerpelea jerpelea moved this from To-Add to drivers in Release Notes - 12.4.0 Jan 8, 2024
@jerpelea jerpelea moved this from drivers to processed in Release Notes - 12.4.0 Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants