nuttx-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [incubator-nuttx] raiden00pl commented on a change in pull request #1051: [DO NOT MERGE] Add support for STM32G474 family
Date Sat, 16 May 2020 10:53:55 GMT

raiden00pl commented on a change in pull request #1051:
URL: https://github.com/apache/incubator-nuttx/pull/1051#discussion_r426136043



##########
File path: arch/arm/src/stm32/hardware/stm32g47xxx_pinmap.h
##########
@@ -0,0 +1,2627 @@
+/****************************************************************************************************
+ *  arch/arm/src/stm32/hardware/stm32g47xxx_pinmap.h
+ *
+ *  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.
+ *
+ ****************************************************************************************************/
+
+#ifndef __ARCH_ARM_SRC_STM32_HARDWARE_STM32G47XXX_PINMAP_H
+#define __ARCH_ARM_SRC_STM32_HARDWARE_STM32G47XXX_PINMAP_H
+
+/****************************************************************************************************
+ * Included Files
+ ****************************************************************************************************/
+
+#include <nuttx/config.h>
+
+#include "stm32_gpio.h"
+
+/****************************************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************************************/
+
+/* Alternate Pin Functions.  All members of the STM32G47xxx family share the
+ * same pin multiplexing (although they differ in the pins physically
+ * available).
+ *
+ * Alternative pin selections are provided with a numeric suffix like _1, _2,
+ * etc.  Drivers, however, will use the pin selection without the numeric
+ * suffix.  Additional definitions are required in the board.h file.  For
+ * example, if CAN1_RX connects via PA11 on some board, then the following
+ * definitions should appear in the board.h header file for that board:
+ *
+ * #define GPIO_CAN1_RX GPIO_CAN1_RX_1
+ *
+ * The driver will then automatically configure PA11 as the CAN1 RX pin.
+ */
+
+/* WARNING!!! WARNING!!! WARNING!!! WARNING!!! WARNING!!! WARNING!!! WARNING!!!
+ * Additional effort is required to select specific GPIO options such as
+ * frequency, open-drain/push-pull, and pull-up/down!  Just the basics are
+ * defined for most pins in this file.
+ */
+
+/* ADC - Analog Digital Converter *******************************************************************/
+
+/* ADC1 has IN1-IN5, IN10-IN12, IN14-IN15 on all STM32G474(C-M-Q-R-V)xxx P/Ns
+ * and also has IN6-9 on STM32G474(M-Q-R-V)xxx P/Ns:
+ */
+
+#define GPIO_ADC1_IN1                  (GPIO_ANALOG|GPIO_PORTA|GPIO_PIN0)
+#define GPIO_ADC1_IN2                  (GPIO_ANALOG|GPIO_PORTA|GPIO_PIN1)
+#define GPIO_ADC1_IN3                  (GPIO_ANALOG|GPIO_PORTA|GPIO_PIN2)
+#define GPIO_ADC1_IN4                  (GPIO_ANALOG|GPIO_PORTA|GPIO_PIN3)
+#define GPIO_ADC1_IN5                  (GPIO_ANALOG|GPIO_PORTB|GPIO_PIN14)
+#define GPIO_ADC1_IN10                 (GPIO_ANALOG|GPIO_PORTF|GPIO_PIN0)
+#define GPIO_ADC1_IN11                 (GPIO_ANALOG|GPIO_PORTB|GPIO_PIN12)
+#define GPIO_ADC1_IN12                 (GPIO_ANALOG|GPIO_PORTB|GPIO_PIN1)
+#define GPIO_ADC1_IN14                 (GPIO_ANALOG|GPIO_PORTB|GPIO_PIN11)
+#define GPIO_ADC1_IN15                 (GPIO_ANALOG|GPIO_PORTB|GPIO_PIN0)
+
+#if defined(CONFIG_ARCH_CHIP_STM32G474M) || \

Review comment:
       We should remove #if's like this one and don't care if selected chip variant support
given pin or not. This is user responsibility to select appropriate pin. These excessive conditions
make the file difficult to read and difficult to maintain.

##########
File path: arch/arm/src/stm32/hardware/stm32_adc_v2.h
##########
@@ -86,18 +86,24 @@
 
 /* Base addresses ***********************************************************************************/
 
-#define STM32_ADC1_OFFSET            0x0000
-#define STM32_ADC2_OFFSET            0x0100
-#define STM32_ADC3_OFFSET            0x0000
-#define STM32_ADC4_OFFSET            0x0100
-#define STM32_ADCCMN_OFFSET          0x0300
+/* G47x has the ADC base addresses are defined in stm32g47xxx_memorymap.h.
+ * Other P/Ns have ADC base addresses are defined here.
+ */
 
-#define STM32_ADC1_BASE              (STM32_ADC1_OFFSET + STM32_ADC12_BASE) /* ADC1 Master
ADC */
-#define STM32_ADC2_BASE              (STM32_ADC2_OFFSET + STM32_ADC12_BASE) /* ADC2 Slave
ADC */
-#define STM32_ADC3_BASE              (STM32_ADC3_OFFSET + STM32_ADC34_BASE) /* ADC3 Master
ADC */
-#define STM32_ADC4_BASE              (STM32_ADC4_OFFSET + STM32_ADC34_BASE) /* ADC4 Slave
ADC */
-#define STM32_ADC12CMN_BASE          (STM32_ADCCMN_OFFSET + STM32_ADC12_BASE) /* ADC1, ADC2
common */
-#define STM32_ADC34CMN_BASE          (STM32_ADCCMN_OFFSET + STM32_ADC34_BASE) /* ADC3, ADC4
common */
+#if !defined(CONFIG_STM32_STM32G47XX)

Review comment:
       I don't like this. We should define the ADC base addresses for G4 here or update all
other families to define this in memorymap headers. I prefer the first option and we should
modify stm32_adc_v2.h logic to support ADC345CMN variant.
   
   Support for ADC5 requires a little more work, so it's better to ignore ADC5 at this time
and leave this logic unchanged.

##########
File path: arch/arm/src/stm32/stm32_lowputc.c
##########
@@ -235,12 +235,18 @@
 #  endif
 
 #  if defined(CONFIG_STM32_STM32F30XX) || defined(CONFIG_STM32_STM32F33XX) || \
-   defined(CONFIG_STM32_STM32F37XX)
+      defined(CONFIG_STM32_STM32F37XX)
 #    define USART_CR1_CLRBITS\
       (USART_CR1_UESM | USART_CR1_RE | USART_CR1_TE | USART_CR1_PS | \
        USART_CR1_PCE | USART_CR1_WAKE | USART_CR1_M | USART_CR1_MME | \
        USART_CR1_OVER8 | USART_CR1_DEDT_MASK | USART_CR1_DEAT_MASK | \
        USART_CR1_ALLINTS)
+#  elif defined(CONFIG_STM32_STM32G47XX)

Review comment:
       Should this not be combined with the above condition? The USART_CR1_M1 bit is not used
in the current implementation anyway and it will match all other modified #ifdef's in this
file

##########
File path: arch/arm/src/stm32/hardware/stm32_dma_v1.h
##########
@@ -64,6 +64,9 @@
 #define DMA_CHAN5                  (4)
 #define DMA_CHAN6                  (5)
 #define DMA_CHAN7                  (6)
+#if defined(CONFIG_STM32_STM32G47XX)

Review comment:
       It's better to introduce a new definition in this file like this 
   
   #if defined(CONFIG_STM32_STM32G47XX)
   #define HAVE_DMA_CHAN8 
   #endif
   
   We should avoid chip-specific #ifdef's if possible.

##########
File path: arch/arm/src/stm32/stm32_dma_v1.c
##########
@@ -106,80 +114,132 @@ struct stm32_dma_s
 
 static struct stm32_dma_s g_dma[DMA_NCHANNELS] =
 {
+#if DMA1_NCHANNELS > 0
   {
     .chan     = 0,
     .irq      = STM32_IRQ_DMA1CH1,
     .base     = STM32_DMA1_BASE + STM32_DMACHAN_OFFSET(0),
   },
+#if DMA1_NCHANNELS > 1
   {
     .chan     = 1,
     .irq      = STM32_IRQ_DMA1CH2,
     .base     = STM32_DMA1_BASE + STM32_DMACHAN_OFFSET(1),
   },
+#if DMA1_NCHANNELS > 2
   {
     .chan     = 2,
     .irq      = STM32_IRQ_DMA1CH3,
     .base     = STM32_DMA1_BASE + STM32_DMACHAN_OFFSET(2),
   },
+#if DMA1_NCHANNELS > 3
   {
     .chan     = 3,
     .irq      = STM32_IRQ_DMA1CH4,
     .base     = STM32_DMA1_BASE + STM32_DMACHAN_OFFSET(3),
   },
+#if DMA1_NCHANNELS > 4
   {
     .chan     = 4,
     .irq      = STM32_IRQ_DMA1CH5,
     .base     = STM32_DMA1_BASE + STM32_DMACHAN_OFFSET(4),
   },
+#if DMA1_NCHANNELS > 5
   {
     .chan     = 5,
     .irq      = STM32_IRQ_DMA1CH6,
     .base     = STM32_DMA1_BASE + STM32_DMACHAN_OFFSET(5),
   },
+#if DMA1_NCHANNELS > 6
   {
     .chan     = 6,
     .irq      = STM32_IRQ_DMA1CH7,
     .base     = STM32_DMA1_BASE + STM32_DMACHAN_OFFSET(6),
   },
+#if DMA1_NCHANNELS > 7
+  {
+    .chan     = 7,
+    .irq      = STM32_IRQ_DMA1CH8,
+    .base     = STM32_DMA1_BASE + STM32_DMACHAN_OFFSET(7),
+  },
+#endif /* DMA1_NCHANNELS > 7 */

Review comment:
       Is there a reason why these conditions are nested? It looks strange :)




----------------------------------------------------------------
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



Mime
View raw message