You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/05/27 03:48:43 UTC
[GitHub] [incubator-nuttx] jlaitine commented on a change in pull request #1115: DRAFT: Spi slave update
jlaitine commented on a change in pull request #1115:
URL: https://github.com/apache/incubator-nuttx/pull/1115#discussion_r430185932
##########
File path: include/nuttx/spi/slave.h
##########
@@ -157,6 +161,23 @@
#define SPI_SCTRLR_QFLUSH(c) ((c)->ops->qflush(c))
+/****************************************************************************
+ * Name: SPI_SCTRLR_RXQPOLL
+ *
+ * Description:
+ * Tell the controller to output all the receive queue data.
+ *
+ * Input Parameters:
+ * sctrlr - SPI slave controller interface instance
+ *
+ * Returned Value:
+ * Number of bytes left in the rx queue. If the device accepted all the
+ * data, the return value will be 0
+ *
+ ****************************************************************************/
+
+#define SPI_SCTRLR_RXQPOLL(c) ((c)->ops->rxqpoll(c))
Review comment:
This will cause 1..n SPI_SDEV_RECEIVE calls back to the slave device (offering blocks of data to the device). From each call, the slave device driver will return the number of data bytes it accepted/read out. If the slave device returns that it received less bytes than what was offered to it, the poll will return. So it is up to the slave device:
If the slave device wants the poll to return and leave data into the controller driver's queue it shall return any number less than what was offered. The controller driver will discard as many bytes of data as what slave device returns.
if the slave device wants the poll to return and let the controller driver throw away the buffered data it shall return the same number of bytes that was offered to each receive call.
##########
File path: include/nuttx/spi/slave.h
##########
@@ -157,6 +161,23 @@
#define SPI_SCTRLR_QFLUSH(c) ((c)->ops->qflush(c))
+/****************************************************************************
+ * Name: SPI_SCTRLR_RXQPOLL
Review comment:
Maybe the name is not the best you can imagine, but it was not only for checking whether the queue is empty, but actually also fetching the data via callbacks... So I don't know if qempty is very descriptive either
##########
File path: include/nuttx/spi/slave.h
##########
@@ -109,18 +111,20 @@
*
* Input Parameters:
* sctrlr - SPI slave controller interface instance
- * data - Command/data mode data value to be shifted out. The width of
- * the data must be the same as the nbits parameter previously
- * provided to the bind() methods.
+ * data - Pointer to the command/data mode data to be shifted out.
+ * The alignment of the data must be the same as the nbits
Review comment:
yes, i will change this comment
##########
File path: include/nuttx/spi/slave.h
##########
@@ -109,18 +111,20 @@
*
* Input Parameters:
* sctrlr - SPI slave controller interface instance
- * data - Command/data mode data value to be shifted out. The width of
- * the data must be the same as the nbits parameter previously
- * provided to the bind() methods.
+ * data - Pointer to the command/data mode data to be shifted out.
Review comment:
yes, exactly. I will change this
##########
File path: include/nuttx/spi/slave.h
##########
@@ -245,10 +268,12 @@
*
* Input Parameters:
* sdev - SPI device interface instance
- * data - The last command/data value that was shifted in
+ * data - Pointer to the new data that has been shifted in
+ * len - Length of the new data in bytes
*
* Returned Value:
- * None
+ * Number of bytes accepted by the device. In other words,
Review comment:
That's right. Our only use was just transferring bytes from one device to another, so this caused the bias in thinking. I will change this to more generic
##########
File path: include/nuttx/spi/slave.h
##########
@@ -451,9 +477,11 @@ struct spi_sctrlrops_s
FAR struct spi_sdev_s *sdev, enum spi_smode_e mode,
int nbits);
CODE void (*unbind)(FAR struct spi_sctrlr_s *sctrlr);
- CODE int (*enqueue)(FAR struct spi_sctrlr_s *sctrlr, uint16_t data);
+ CODE int (*enqueue)(FAR struct spi_sctrlr_s *sctrlr,
+ const uint16_t *data, uint16_t len);
Review comment:
Yes, will change this
##########
File path: include/nuttx/spi/slave.h
##########
@@ -475,8 +503,10 @@ struct spi_sdevops_s
{
CODE void (*select)(FAR struct spi_sdev_s *sdev, bool selected);
CODE void (*cmddata)(FAR struct spi_sdev_s *sdev, bool data);
- CODE uint16_t (*getdata)(FAR struct spi_sdev_s *sdev);
- CODE void (*receive)(FAR struct spi_sdev_s *sdev, uint16_t cmd);
+ CODE uint16_t (*getdata)(FAR struct spi_sdev_s *sdev,
+ const uint16_t **data);
+ CODE uint16_t (*receive)(FAR struct spi_sdev_s *sdev,
Review comment:
Yes, you are right
##########
File path: include/nuttx/spi/slave.h
##########
@@ -157,6 +161,23 @@
#define SPI_SCTRLR_QFLUSH(c) ((c)->ops->qflush(c))
+/****************************************************************************
+ * Name: SPI_SCTRLR_RXQPOLL
Review comment:
SPI_SCTRLR_QFETCH and qfetch ?
##########
File path: include/nuttx/spi/slave.h
##########
@@ -157,6 +161,23 @@
#define SPI_SCTRLR_QFLUSH(c) ((c)->ops->qflush(c))
+/****************************************************************************
+ * Name: SPI_SCTRLR_RXQPOLL
Review comment:
sounds good, I will change to that
##########
File path: arch/arm/src/stm32h7/stm32_spi_slave.c
##########
@@ -0,0 +1,1796 @@
+/*****************************************************************************
+ * arch/arm/src/stm32h7/stm32_spi_slave.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ *****************************************************************************/
+
+/*****************************************************************************
+ * Included Files
+ *****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <sys/types.h>
+#include <stdint.h>
+#include <stdbool.h>
+#include <stddef.h>
+#include <semaphore.h>
+#include <errno.h>
+#include <debug.h>
+#include <string.h>
+
+#include <nuttx/irq.h>
+#include <nuttx/arch.h>
+#include <nuttx/semaphore.h>
+#include <nuttx/spi/spi.h>
+#include <nuttx/spi/slave.h>
+#include <nuttx/power/pm.h>
+
+#include <arch/board/board.h>
+
+#include "arm_internal.h"
+#include "arm_arch.h"
+
+#include "chip.h"
+#include "stm32_rcc.h"
+#include "stm32_gpio.h"
+#include "stm32_spi.h"
+#include "stm32_dma.h"
+
+#if defined(CONFIG_STM32H7_SPI1_SLAVE) || \
+ defined(CONFIG_STM32H7_SPI2_SLAVE) || \
+ defined(CONFIG_STM32H7_SPI3_SLAVE) || \
+ defined(CONFIG_STM32H7_SPI4_SLAVE) || \
+ defined(CONFIG_STM32H7_SPI5_SLAVE) || \
+ defined(CONFIG_STM32H7_SPI6_SLAVE)
+
+/*****************************************************************************
+ * Pre-processor Definitions
+ *****************************************************************************/
+
+/* Configuration *************************************************************/
+
+/* SPI interrupts */
+
+#ifdef CONFIG_STM32H7_SPI_INTERRUPTS
+# error "Interrupt driven SPI not yet supported"
+#endif
+
+/* Can't have both interrupt driven SPI and SPI DMA */
+
+#if defined(CONFIG_STM32H7_SPI_INTERRUPTS) && defined(CONFIG_STM32H7_SPI_DMA)
+# error "Cannot enable both interrupt mode and DMA mode for SPI"
+#endif
+
+/* SPI DMA priority */
+
+#ifdef CONFIG_STM32H7_SPI_DMA
+
+# if defined(CONFIG_SPI_DMAPRIO)
+# define SPI_DMA_PRIO CONFIG_SPI_DMAPRIO
+# elif defined(DMA_SCR_PRIMED)
+# define SPI_DMA_PRIO DMA_SCR_PRILO
+# else
+# error "Unknown STM32 DMA"
+# endif
+
+# if (SPI_DMA_PRIO & ~DMA_SCR_PL_MASK) != 0
+# error "Illegal value for CONFIG_SPI_DMAPRIO"
+# endif
+
+/* DMA channel configuration */
+
+# define SPI_RXDMA16_CONFIG (SPI_DMA_PRIO | DMA_SCR_MSIZE_16BITS | \
+ DMA_SCR_PSIZE_16BITS | DMA_SCR_MINC | \
+ DMA_SCR_DIR_P2M | DMA_SCR_CIRC)
+# define SPI_RXDMA8_CONFIG (SPI_DMA_PRIO | DMA_SCR_MSIZE_8BITS | \
+ DMA_SCR_PSIZE_8BITS | DMA_SCR_MINC| \
+ DMA_SCR_DIR_P2M | DMA_SCR_CIRC)
+# define SPI_TXDMA16_CONFIG (SPI_DMA_PRIO | DMA_SCR_MSIZE_16BITS| \
+ DMA_SCR_PSIZE_16BITS | DMA_SCR_MINC| \
+ DMA_SCR_DIR_M2P)
+# define SPI_TXDMA8_CONFIG (SPI_DMA_PRIO | DMA_SCR_MSIZE_8BITS| \
+ DMA_SCR_PSIZE_8BITS | DMA_SCR_MINC|DMA_SCR_DIR_M2P)
+#endif
+
+/* Kernel clock configuration
+ * TODO:
+ * - support for all kernel clock configuration
+ */
+
+#if defined(CONFIG_STM32H7_SPI1_SLAVE) || defined(CONFIG_STM32H7_SPI2_SLAVE) || \
+ defined(CONFIG_STM32H7_SPI3_SLAVE)
+# if STM32_RCC_D2CCIP1R_SPI123SRC == RCC_D2CCIP1R_SPI123SEL_PLL1
+# define SPI123_KERNEL_CLOCK_FREQ STM32_PLL1Q_FREQUENCY
+# else
+# error Not supported yet
+# endif
+# if SPI123_KERNEL_CLOCK_FREQ > 200000000
+# error Not supported SPI123 frequency
+# endif
+#endif /* SPI123 */
+
+#if defined(CONFIG_STM32H7_SPI4_SLAVE) || defined(CONFIG_STM32H7_SPI5_SLAVE)
+# if STM32_RCC_D2CCIP1R_SPI45SRC == RCC_D2CCIP1R_SPI45SEL_APB
+# define SPI45_KERNEL_CLOCK_FREQ STM32_PCLK2_FREQUENCY
+# else
+# error Not supported yet
+# endif
+# if SPI45_KERNEL_CLOCK_FREQ > 100000000
+# error Not supported SPI45 frequency
+# endif
+#endif /* SPI45 */
+
+#if defined(CONFIG_STM32H7_SPI6_SLAVE)
+# if STM32_RCC_D3CCIPR_SPI6SRC == RCC_D3CCIPR_SPI6SEL_PCLK4
+# define SPI6_KERNEL_CLOCK_FREQ STM32_PCLK4_FREQUENCY
+# else
+# error Not supported yet
+# endif
+# if SPI6_KERNEL_CLOCK_FREQ > 100000000
+# error Not supported SPI6 frequency
+# endif
+#endif /* SPI6 */
+
+#if defined (CONFIG_STM32H7_SPI_SLAVE_QSIZE)
+# if CONFIG_STM32H7_SPI_SLAVE_QSIZE > 65535
+# error CONFIG_STM32H7_SPI_SLAVE_QSIZE too large
+# endif
+#endif
+
+#define DMA_BUFFER_MASK (ARMV7M_DCACHE_LINESIZE - 1)
+#define DMA_ALIGN_UP(n) (((n) + DMA_BUFFER_MASK) & ~DMA_BUFFER_MASK)
+#define DMA_ALIGN_DOWN(n) ((n) & ~DMA_BUFFER_MASK)
+
+/*****************************************************************************
+ * Private Types
+ *****************************************************************************/
+
+enum spi_config_e
+{
+ FULL_DUPLEX = 0,
+ SIMPLEX_TX,
+ SIMPLEX_RX,
+ HALF_DUPLEX
+};
+
+struct stm32_spidev_s
+{
+ struct spi_sctrlr_s sctrlr; /* Externally visible part of the
+ * SPI slave interface */
+ struct spi_sdev_s *sdev; /* Bound SPI slave device interface */
+ uint32_t spibase; /* SPIn base address */
+ uint32_t spiclock; /* Clocking for the SPI module */
+ uint8_t irq; /* SPI IRQ number */
+ uint32_t nss_pin; /* Chip select pin configuration */
+#ifdef CONFIG_STM32H7_SPI_DMA
+ volatile uint8_t rxresult; /* Result of the RX DMA */
+ volatile uint8_t txresult; /* Result of the TX DMA */
+ uint32_t rxch; /* The RX DMA channel number */
+ uint32_t txch; /* The TX DMA channel number */
+ DMA_HANDLE rxdma; /* DMA channel handle for RX transfers */
+ DMA_HANDLE txdma; /* DMA channel handle for TX transfers */
+ sem_t rxsem; /* Wait for RX DMA to complete */
+ sem_t txsem; /* Wait for TX DMA to complete */
+ uint32_t txccr; /* DMA control register for TX transfers */
+ uint32_t rxccr; /* DMA control register for RX transfers */
+ bool dmarunning; /* DMA is started */
+#endif
+ bool initialized; /* Has SPI interface been initialized */
+ sem_t exclsem; /* Held while chip is selected for mutual
+ * exclusion */
+ int8_t nbits; /* Width of word in bits */
+ uint8_t mode; /* Mode 0,1,2,3 */
+ uint8_t bus; /* SPI bus number */
+ bool nss; /* True: Chip selected */
+#ifdef CONFIG_PM
+ struct pm_callback_s pm_cb; /* PM callbacks */
+#endif
+ enum spi_config_e config; /* full/half duplex, simplex rx/tx */
+
+ /* Output queue */
+
+ uint16_t ohead; /* Location of next value in out queue */
+ uint16_t otail; /* Index of first value in out queue */
+ uint8_t *outq;
+
+ /* Input queue */
+
+ uint16_t ihead; /* Location of next unread value */
+#ifndef CONFIG_STM32H7_SPI_DMA
+ uint16_t itail; /* Index of next free memory pointer */
+#endif
+ uint8_t *inq;
+};
+
+/*****************************************************************************
+ * Private Function Prototypes
+ *****************************************************************************/
+
+/* Helpers */
+
+static inline uint32_t spi_getreg(FAR struct stm32_spidev_s *priv,
+ uint32_t offset);
+static inline void spi_putreg(FAR struct stm32_spidev_s *priv,
+ uint32_t offset, uint32_t value);
+static inline void spi_modifyreg(FAR struct stm32_spidev_s *priv,
+ uint32_t offset, uint32_t clrbits,
+ uint32_t setbits);
+static inline uint32_t spi_readword(FAR struct stm32_spidev_s *priv);
+static inline void spi_writeword(FAR struct stm32_spidev_s *priv,
+ uint32_t byte);
+static inline bool spi_9to16bitmode(FAR struct stm32_spidev_s *priv);
+#ifdef CONFIG_DEBUG_SPI_INFO
+static inline void spi_dumpregs(FAR struct stm32_spidev_s *priv);
+#endif
+
+/* DMA support */
+
+#ifdef CONFIG_STM32H7_SPI_DMA
+static void spi_dmarxcallback(DMA_HANDLE handle, uint8_t isr,
+ void *arg);
+static void spi_dmatxcallback(DMA_HANDLE handle, uint8_t isr,
+ void *arg);
+static void spi_dmarxsetup(FAR struct stm32_spidev_s *priv,
+ size_t nwords);
+static void spi_dmatxsetup(FAR struct stm32_spidev_s *priv,
+ size_t nwords);
+static inline void spi_dmarxstart(FAR struct stm32_spidev_s *priv);
+static inline void spi_dmatxstart(FAR struct stm32_spidev_s *priv);
+#endif
+
+/* Interrupt handler detecting nss status changes */
+
+static int spi_nssinterrupt(int irq, void *context, void *arg);
+
+/* SPI slave methods */
+
+static void spi_bind(struct spi_sctrlr_s *sctrlr,
+ struct spi_sdev_s *sdev, enum spi_smode_e mode,
+ int nbits);
+static void spi_unbind(struct spi_sctrlr_s *sctrlr);
+static int spi_enqueue(struct spi_sctrlr_s *sctrlr,
+ FAR const void *data,
+ size_t len);
+static bool spi_qfull(struct spi_sctrlr_s *sctrlr);
+static void spi_qflush(struct spi_sctrlr_s *sctrlr);
+static size_t spi_qpoll(struct spi_sctrlr_s *sctrlr);
+
+/* Initialization */
+
+static void spi_slave_initialize(FAR struct stm32_spidev_s *priv);
+
+/* PM interface */
+
+#ifdef CONFIG_PM
+static int spi_pm_prepare(FAR struct pm_callback_s *cb, int domain,
+ enum pm_state_e pmstate);
+#endif
+
+/*****************************************************************************
+ * Private Data
+ *****************************************************************************/
+
+/* SPI slave controller driver operations */
+
+static const struct spi_sctrlrops_s g_sctrlr_ops =
+{
+ .bind = spi_bind,
+ .unbind = spi_unbind,
+ .enqueue = spi_enqueue,
+ .qfull = spi_qfull,
+ .qflush = spi_qflush,
+ .qpoll = spi_qpoll,
+};
+
+#define SPI_SLAVE_OUTQ(x) spi##x##_outq
+#define SPI_SLAVE_INQ(x) spi##x##_inq
+
+#ifdef CONFIG_STM32H7_SPI_DMA
+#define SPI_SLAVE_INIT_DMA(x) \
+ .rxch = DMAMAP_SPI##x##_RX, \
+ .txch = DMAMAP_SPI##x##_TX, \
+ .outq = SPI_SLAVE_OUTQ(x), \
+ .inq = SPI_SLAVE_INQ(x),
+#else
+#define SPI_SLAVE_INIT_DMA(x)
+#endif
+
+#ifdef CONFIG_PM
+#define SPI_SLAVE_INIT_PM_PREPARE .pm_cb.prepare = spi_pm_prepare,
+#else
+#define SPI_SLAVE_INIT_PM_PREPARE
+#endif
+
+#define SPI_SLAVE_INIT(x) \
+{ \
+ .sctrlr.ops = &g_sctrlr_ops, \
+ .sdev = NULL, \
+ .spibase = STM32_SPI##x##_BASE, \
+ .spiclock = SPI45_KERNEL_CLOCK_FREQ, \
+ .irq = STM32_IRQ_SPI##x, \
+ SPI_SLAVE_INIT_DMA(x) \
+ .initialized = false, \
+ SPI_SLAVE_INIT_PM_PREPARE \
+ .config = CONFIG_STM32H7_SPI##x##_COMMTYPE, \
+}
+
+#ifdef CONFIG_STM32H7_SPI1_SLAVE
+
+static uint8_t SPI_SLAVE_OUTQ(1)[DMA_ALIGN_UP(CONFIG_STM32H7_SPI_SLAVE_QSIZE)]
+__attribute__((aligned(ARMV7M_DCACHE_LINESIZE)));
+static uint8_t SPI_SLAVE_INQ(1)[DMA_ALIGN_UP(CONFIG_STM32H7_SPI_SLAVE_QSIZE)]
+__attribute__((aligned(ARMV7M_DCACHE_LINESIZE)));
+static struct stm32_spidev_s g_spi1sctrlr = SPI_SLAVE_INIT(1);
+
+#endif
+
+#ifdef CONFIG_STM32H7_SPI2_SLAVE
+
+static uint8_t SPI_SLAVE_OUTQ(2)[DMA_ALIGN_UP(CONFIG_STM32H7_SPI_SLAVE_QSIZE)]
+__attribute__((aligned(ARMV7M_DCACHE_LINESIZE)));
+static uint8_t SPI_SLAVE_INQ(2)[DMA_ALIGN_UP(CONFIG_STM32H7_SPI_SLAVE_QSIZE)]
+__attribute__((aligned(ARMV7M_DCACHE_LINESIZE)));
+static struct stm32_spidev_s g_spi2sctrlr = SPI_SLAVE_INIT(2);
+
+#endif
+
+#ifdef CONFIG_STM32H7_SPI3_SLAVE
+
+static uint8_t SPI_SLAVE_OUTQ(3)[DMA_ALIGN_UP(CONFIG_STM32H7_SPI_SLAVE_QSIZE)]
+__attribute__((aligned(ARMV7M_DCACHE_LINESIZE)));
+static uint8_t SPI_SLAVE_INQ(3)[DMA_ALIGN_UP(CONFIG_STM32H7_SPI_SLAVE_QSIZE)]
+__attribute__((aligned(ARMV7M_DCACHE_LINESIZE)));
+static struct stm32_spidev_s g_spi3sctrlr = SPI_SLAVE_INIT(3);
+
+#endif
+
+#ifdef CONFIG_STM32H7_SPI4_SLAVE
+
+static uint8_t SPI_SLAVE_OUTQ(4)[DMA_ALIGN_UP(CONFIG_STM32H7_SPI_SLAVE_QSIZE)]
+__attribute__((aligned(ARMV7M_DCACHE_LINESIZE)));
+static uint8_t SPI_SLAVE_INQ(4)[DMA_ALIGN_UP(CONFIG_STM32H7_SPI_SLAVE_QSIZE)]
+__attribute__((aligned(ARMV7M_DCACHE_LINESIZE)));
+static struct stm32_spidev_s g_spi4sctrlr = SPI_SLAVE_INIT(4);
+
+#endif
+
+#ifdef CONFIG_STM32H7_SPI5_SLAVE
+
+static uint8_t SPI_SLAVE_OUTQ(5)[DMA_ALIGN_UP(CONFIG_STM32H7_SPI_SLAVE_QSIZE)]
+__attribute__((aligned(ARMV7M_DCACHE_LINESIZE)));
+static uint8_t SPI_SLAVE_INQ(5)[DMA_ALIGN_UP(CONFIG_STM32H7_SPI_SLAVE_QSIZE)]
+__attribute__((aligned(ARMV7M_DCACHE_LINESIZE)));
+static struct stm32_spidev_s g_spi5sctrlr = SPI_SLAVE_INIT(5);
+
+#endif
+
+#ifdef CONFIG_STM32H7_SPI6_SLAVE
+
+static uint8_t SPI_SLAVE_OUTQ(6)[DMA_ALIGN_UP(CONFIG_STM32H7_SPI_SLAVE_QSIZE)]
+__attribute__((aligned(ARMV7M_DCACHE_LINESIZE)));
+static uint8_t SPI_SLAVE_INQ(6)[DMA_ALIGN_UP(CONFIG_STM32H7_SPI_SLAVE_QSIZE)]
+__attribute__((aligned(ARMV7M_DCACHE_LINESIZE)));
Review comment:
Good catch! I actually never tried to use this on SPI6 so this wasn't noticed. I am putting everything into AXI_SRAM at the moment.
In general, "locating" is indeed a very common need! But I fear that this spreads easily accross startup files, linker scripts and so on.
Maybe for this specific PR, just removing SPI6 support for the slave for now is adequate.
Actually, also for dynamic memory allocations it would be nice to be able to allocate a dma memory pool and malloc memory from there. I do know of gran allocator and "fat_dma_alloc", but those could maybe be more generic than just reserved for fat?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org